On Monday, September 02, 2013 05:52:15 PM Daniel Lezcano wrote: > On 09/02/2013 04:43 PM, Bartlomiej Zolnierkiewicz wrote: > > On Monday, September 02, 2013 04:24:23 PM Daniel Lezcano wrote: > >> On 09/02/2013 03:48 PM, Bartlomiej Zolnierkiewicz wrote: > >>> On Monday, September 02, 2013 03:18:51 PM Daniel Lezcano wrote: > >>>> On 09/02/2013 11:41 AM, Bartlomiej Zolnierkiewicz wrote: > >>>>> On Monday, September 02, 2013 10:54:17 AM Daniel Lezcano wrote: > >>>>>> On 08/30/2013 12:21 PM, Bartlomiej Zolnierkiewicz wrote: > >>>>>>> Add "cpuidle-exynos.max_states=" parameter to allow user to specify > >>>>>>> the maximum of allowed CPU idle states for ARM EXYNOS cpuidle driver. > >>>>>>> > >>>>>>> This change is needed because C1 state (AFTR mode) is often not able > >>>>>>> to work properly due to incompatibility with some bootloader versions. > >>>>>>> > >>>>>>> Usage examples: > >>>>>>> > >>>>>>> "cpuidle-exynos.max_states=1" disables C1 state (AFTR mode). > >>>>>>> > >>>>>>> "cpuidle-exynos.max_states=0" disables the driver completely. > >>>>>>> > >>>>>>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx> > >>>>>>> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx> > >>>>>>> Cc: Tomasz Figa <t.figa@xxxxxxxxxxx> > >>>>>>> Cc: Amit Daniel Kachhap <amit.daniel@xxxxxxxxxxx> > >>>>>> > >>>>>> There is a max_cstate option for acpi and intel idle. There is also the > >>>>>> cpuidle.off=1 option. As the semantic is the same, I think adding a > >>>>>> common cpuidle option usable for all the drivers is better. > >>>>> > >>>>> I thought about making the option common for all cpuidle drivers first > >>>>> but due to support for multiple cpuidle drivers on one machine (i.e. > >>>>> big.LITTLE), per-driver option looked like a better approach. > >>>>> > >>>>> Should I make the option common and not worry about multiple drivers on > >>>>> one machine support? > >>>> > >>>> Mmh, that's a good point. > >>>> > >>>> I am not in favor of multiple options spread across the different > >>>> drivers. Furthermore the max_cstate is used in the intel platform to > >>>> 'discover' what states the firmware supports which is not the case of > >>>> the cpuidle ARM drivers (except new PSCI based). This option does not > >>>> really fits well here. > >>>> > >>>> There is the kernel parameter 'cpuidle.off', so disabling the driver is ok. > >>>> > >>>> You converted the cpuidle driver to a platform driver. Isn't possible to > >>>> pass information in the platform data field at boot time to tell AFTR is > >>>> not supported and then act on the 'disabled' field of this state ? > >>> > >>> It might be possible but I don't know where the source of this data would > >>> be, platform specific kernel parameter? It sounds just like moving the code > >>> around and adding superfluous platform->driver code because the similar > >>> kernel parameter to disable just AFTR can be added in cpuidle-exynos driver > >>> as well. > >> > >> It is to prevent to add a new kernel parameter (with the documentation) > >> for a single driver which has a bogus idle state. If that could be > >> handled internally that would be cleaner. > > > > If I believed that it could be handled internally I wouldn't be trying to > > add a kernel parameter to handle it.. Would I? ;) > > > >> Can you shortly describe what happens with the bootloader and AFTR ? > > > > AFTR just doesn't work with the custom U-Boot version that we are using > > (attempts to go into AFTR mode result in lockup) and using the upstream > > version of U-Boot is not an option since it doesn't support the hardware > > that we are using AFAIK. I also don't know exactly why it doesn't work > > (I just suspect that it reuses INFORM registers for some other purposes). > > You want to add a kernel option as a work around for a bug in U-Boot ? > > IMO, you should drop the hot potato to the u-boot guys :) Well, if it only was so simple. :) [ Unfortunately, they have higher priority tasks so this is unlikely to get fixed in the foreseeable future. ] Problems with AFTR are also not limited to our U-Boot problem (i.e. issue with Hypervisor enabled on EXYNOS5 or problem with CONFIG_THUMB2_KERNEL=y compiled kernels) so it would be nice to have a way to disable it without the need for recompiling the kernel. Limiting maximum allowed cpuidle state is also useful for testing purposes (this is not limited to EXYNOS driver but a generally useful thing). > >> I guess you are not interested in cpuidle.off=1 because you want cpuidle > >> statistics for WFI, right ? > > > > Right. :) Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |