| Message ID | 1397212815-16068-19-git-send-email-daniel.lezcano@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday 11 April 2014, Daniel Lezcano wrote: > No more dependency on the arch code. The platform_data field is used to set the > PM callback as the other cpuidle drivers. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org> > Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> This has just shown up in linux-next and broken randconfig builds. > diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c > index fe8dac8..d22f0e4 100644 > --- a/arch/arm/mach-exynos/exynos.c > +++ b/arch/arm/mach-exynos/exynos.c > @@ -221,8 +221,9 @@ void exynos_restart(enum reboot_mode mode, const char *cmd) > } > > static struct platform_device exynos_cpuidle = { > - .name = "exynos_cpuidle", > - .id = -1, > + .name = "exynos_cpuidle", > + .dev.platform_data = exynos_enter_aftr, > + .id = -1, > }; > This is wrong on many levels, can we please do this properly? * The exynos_enter_aftr function is compiled conditionally, so you can't just reference it from generic code, or you get a link error. * 'static struct platform_device ...' has been deprecated for at least a decade, stop doing that. For any platform devices that get registered, there is platform_device_register_simple(). * There shouldn't need to be a platform_device to start with, this should all come from DT. We can't do this on arm64 anyway, so any code that may be shared between arm32 and arm64 should have proper abstractions. Daniel, you should really know better than this. Why are you still adding code to drivers/cpuidle that uses legacy platform devices rather than DT probing? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd, On 09.05.2014 12:56, Arnd Bergmann wrote: > On Friday 11 April 2014, Daniel Lezcano wrote: >> No more dependency on the arch code. The platform_data field is used to set the >> PM callback as the other cpuidle drivers. >> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org> >> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > > This has just shown up in linux-next and broken randconfig builds. > >> diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c >> index fe8dac8..d22f0e4 100644 >> --- a/arch/arm/mach-exynos/exynos.c >> +++ b/arch/arm/mach-exynos/exynos.c >> @@ -221,8 +221,9 @@ void exynos_restart(enum reboot_mode mode, const char *cmd) >> } >> >> static struct platform_device exynos_cpuidle = { >> - .name = "exynos_cpuidle", >> - .id = -1, >> + .name = "exynos_cpuidle", >> + .dev.platform_data = exynos_enter_aftr, >> + .id = -1, >> }; >> > > This is wrong on many levels, can we please do this properly? > > * The exynos_enter_aftr function is compiled conditionally, so you can't just > reference it from generic code, or you get a link error. +1 > * 'static struct platform_device ...' has been deprecated for at least a decade, > stop doing that. For any platform devices that get registered, there is > platform_device_register_simple(). +0.5 The missing 0.5 is because you can't pass platform data using platform_device_register_simple(). There is platform_device_register_resndata(), though. > * There shouldn't need to be a platform_device to start with, this should all > come from DT. We can't do this on arm64 anyway, so any code that may be > shared between arm32 and arm64 should have proper abstractions. -1 Exynos cpuidle is not a device on the SoC, so I don't think there is any way to represent it in DT. The only thing I could see this is matching root node with a central SoC driver that instantiates specific subdevices, such as cpufreq and cpuidle, but I don't see any available infrastructure for this. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/09/14 19:56, Arnd Bergmann wrote: > On Friday 11 April 2014, Daniel Lezcano wrote: >> No more dependency on the arch code. The platform_data field is used to set the >> PM callback as the other cpuidle drivers. >> >> Signed-off-by: Daniel Lezcano<daniel.lezcano@linaro.org> >> Reviewed-by: Viresh Kumar<viresh.kumar@linaro.org> >> Reviewed-by: Bartlomiej Zolnierkiewicz<b.zolnierkie@samsung.com> > > This has just shown up in linux-next and broken randconfig builds. > Hi Arnd, Hmm...I just reverted this series in for-next of samsung tree and will hold on until solving the randconfig build breakage. I need to look at this again and think about your suggestion... Thanks, Kukjin -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Friday, May 09, 2014 02:02:14 PM Tomasz Figa wrote: > Hi Arnd, > > On 09.05.2014 12:56, Arnd Bergmann wrote: > > On Friday 11 April 2014, Daniel Lezcano wrote: > >> No more dependency on the arch code. The platform_data field is used to set the > >> PM callback as the other cpuidle drivers. > >> > >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > >> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org> > >> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > > > > This has just shown up in linux-next and broken randconfig builds. Could you please give some more details about these issues? If this is a build breakage for CONFIG_PM_SLEEP=n && CONFIG_CPU_IDLE=y configuration then this is not a new problem introduced by the current patchset (please see below). > >> diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c > >> index fe8dac8..d22f0e4 100644 > >> --- a/arch/arm/mach-exynos/exynos.c > >> +++ b/arch/arm/mach-exynos/exynos.c > >> @@ -221,8 +221,9 @@ void exynos_restart(enum reboot_mode mode, const char *cmd) > >> } > >> > >> static struct platform_device exynos_cpuidle = { > >> - .name = "exynos_cpuidle", > >> - .id = -1, > >> + .name = "exynos_cpuidle", > >> + .dev.platform_data = exynos_enter_aftr, > >> + .id = -1, > >> }; > >> > > > > This is wrong on many levels, can we please do this properly? > > > > * The exynos_enter_aftr function is compiled conditionally, so you can't just > > reference it from generic code, or you get a link error. > > +1 Even without Daniel's patchset we are getting a link error for CONFIG_PM_SLEEP=n && CONFIG_CPU_IDLE=y configuration. In arch/arm/mach-exynos/Makefile we have: ... obj-$(CONFIG_PM_SLEEP) += pm.o sleep.o ... obj-$(CONFIG_CPU_IDLE) += cpuidle.o ... and since cpuidle.c is referencing exynos_cpu_resume() from sleep.S the build results in: arch/arm/kernel/return_address.c:63:2: warning: #warning "TODO: return_address should use unwind tables" [-Wcpp] arch/arm/mach-exynos/built-in.o: In function `exynos4_enter_core0_aftr': /home/bzolnier/linux-sprc/arch/arm/mach-exynos/cpuidle.c:141: undefined reference to `cpu_suspend' arch/arm/mach-exynos/built-in.o: In function `exynos4_enter_lowpower': /home/bzolnier/linux-sprc/arch/arm/mach-exynos/cpuidle.c:176: undefined reference to `exynos_cpu_resume' make: *** [vmlinux] Error 1 linkage errors. [ The other error for the current code is for cpu_suspend() which is also (indirectly) dependent on CONFIG_PM_SLEEP. ] > > * 'static struct platform_device ...' has been deprecated for at least a decade, > > stop doing that. For any platform devices that get registered, there is > > platform_device_register_simple(). > > +0.5 > > The missing 0.5 is because you can't pass platform data using > platform_device_register_simple(). There is > platform_device_register_resndata(), though. Agreed but this can be fixed trivially (even in in the incremental patch). > > * There shouldn't need to be a platform_device to start with, this should all > > come from DT. We can't do this on arm64 anyway, so any code that may be > > shared between arm32 and arm64 should have proper abstractions. > > -1 > > Exynos cpuidle is not a device on the SoC, so I don't think there is any > way to represent it in DT. The only thing I could see this is matching > root node with a central SoC driver that instantiates specific > subdevices, such as cpufreq and cpuidle, but I don't see any available > infrastructure for this. Moreover this code is not going to be shared between arm32 and arm64 in the near future (if ever) and doing things by using platform device has such benefit that the current code can be changed without a problem when the need arise. With DT there is no such flexibility and it also takes some time to design DT bindings properly. The build problems should be fixed but if indeed they are not a new ones then the patchset should be brought back (Kukjin has dropped it entirely for now). This is important work which serves as a base for more cpuidle improvements from both Daniel and me therefore it would be great to have it merged in v3.16 and not delay it unnecessarily. 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/09/2014 02:02 PM, Tomasz Figa wrote: > Hi Arnd, > > On 09.05.2014 12:56, Arnd Bergmann wrote: >> On Friday 11 April 2014, Daniel Lezcano wrote: >>> No more dependency on the arch code. The platform_data field is used to set the >>> PM callback as the other cpuidle drivers. >>> >>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org> >>> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> >> >> This has just shown up in linux-next and broken randconfig builds. >> >>> diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c >>> index fe8dac8..d22f0e4 100644 >>> --- a/arch/arm/mach-exynos/exynos.c >>> +++ b/arch/arm/mach-exynos/exynos.c >>> @@ -221,8 +221,9 @@ void exynos_restart(enum reboot_mode mode, const char *cmd) >>> } >>> >>> static struct platform_device exynos_cpuidle = { >>> - .name = "exynos_cpuidle", >>> - .id = -1, >>> + .name = "exynos_cpuidle", >>> + .dev.platform_data = exynos_enter_aftr, >>> + .id = -1, >>> }; >>> >> >> This is wrong on many levels, can we please do this properly? >> >> * The exynos_enter_aftr function is compiled conditionally, so you can't just >> reference it from generic code, or you get a link error. > > +1 That is true but still we have a link error without this patch. We shouldn't register and declare this structure if CONFIG_PM / CONFIG_CPU_IDLE are not set. >> * 'static struct platform_device ...' has been deprecated for at least a decade, >> stop doing that. For any platform devices that get registered, there is >> platform_device_register_simple(). > > +0.5 > > The missing 0.5 is because you can't pass platform data using > platform_device_register_simple(). There is > platform_device_register_resndata(), though. > >> * There shouldn't need to be a platform_device to start with, this should all >> come from DT. We can't do this on arm64 anyway, so any code that may be >> shared between arm32 and arm64 should have proper abstractions. > > -1 > > Exynos cpuidle is not a device on the SoC, so I don't think there is any > way to represent it in DT. The only thing I could see this is matching > root node with a central SoC driver that instantiates specific > subdevices, such as cpufreq and cpuidle, but I don't see any available > infrastructure for this. There is a RFC for defining generic idle states [1]. The idea behind using the platform driver framework is to unify the code across the different drivers and separate the PM / cpuidle code. By this way, we can move the different drivers to drivers/cpuidle and store them in a single place. That make easier the tracking, the review and the maintenance. I am ok to by using platform_device_register_resndata() but I would prefer to do that a bit later by converting the other drivers too. That will be easier if we have them grouped in a single directory (this is what does this patchset at the end). As there are some more work based on this patchset and the link error could be fixed as an independent patch, I would recommend to re-integrate it in the tree as asked by Bartlomiej. Thanks -- Daniel [1] http://www.spinics.net/lists/arm-kernel/msg328747.html
Arnd, Kukjin, Daniel, On 12.05.2014 17:18, Daniel Lezcano wrote: > On 05/09/2014 02:02 PM, Tomasz Figa wrote: >> Hi Arnd, >> >> On 09.05.2014 12:56, Arnd Bergmann wrote: >>> On Friday 11 April 2014, Daniel Lezcano wrote: >>>> No more dependency on the arch code. The platform_data field is used to set the >>>> PM callback as the other cpuidle drivers. >>>> >>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> >>>> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org> >>>> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> >>> >>> This has just shown up in linux-next and broken randconfig builds. >>> >>>> diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c >>>> index fe8dac8..d22f0e4 100644 >>>> --- a/arch/arm/mach-exynos/exynos.c >>>> +++ b/arch/arm/mach-exynos/exynos.c >>>> @@ -221,8 +221,9 @@ void exynos_restart(enum reboot_mode mode, const char *cmd) >>>> } >>>> >>>> static struct platform_device exynos_cpuidle = { >>>> - .name = "exynos_cpuidle", >>>> - .id = -1, >>>> + .name = "exynos_cpuidle", >>>> + .dev.platform_data = exynos_enter_aftr, >>>> + .id = -1, >>>> }; >>>> >>> >>> This is wrong on many levels, can we please do this properly? >>> >>> * The exynos_enter_aftr function is compiled conditionally, so you can't just >>> reference it from generic code, or you get a link error. >> >> +1 > > That is true but still we have a link error without this patch. We > shouldn't register and declare this structure if CONFIG_PM / > CONFIG_CPU_IDLE are not set. > >>> * 'static struct platform_device ...' has been deprecated for at least a decade, >>> stop doing that. For any platform devices that get registered, there is >>> platform_device_register_simple(). >> >> +0.5 >> >> The missing 0.5 is because you can't pass platform data using >> platform_device_register_simple(). There is >> platform_device_register_resndata(), though. >> >>> * There shouldn't need to be a platform_device to start with, this should all >>> come from DT. We can't do this on arm64 anyway, so any code that may be >>> shared between arm32 and arm64 should have proper abstractions. >> >> -1 >> >> Exynos cpuidle is not a device on the SoC, so I don't think there is any >> way to represent it in DT. The only thing I could see this is matching >> root node with a central SoC driver that instantiates specific >> subdevices, such as cpufreq and cpuidle, but I don't see any available >> infrastructure for this. > > There is a RFC for defining generic idle states [1]. > > The idea behind using the platform driver framework is to unify the code > across the different drivers and separate the PM / cpuidle code. > > By this way, we can move the different drivers to drivers/cpuidle and > store them in a single place. That make easier the tracking, the review > and the maintenance. > > I am ok to by using platform_device_register_resndata() but I would > prefer to do that a bit later by converting the other drivers too. That > will be easier if we have them grouped in a single directory (this is > what does this patchset at the end). > > As there are some more work based on this patchset and the link error > could be fixed as an independent patch, I would recommend to > re-integrate it in the tree as asked by Bartlomiej. In general, it would be nice to have everything done properly, but I'd consider Daniel's series as a huge improvement already and a nice intermediate step towards further clean-up. So based on the comments quoted above, instead of stalling the development, I'd suggest to accept this series and then move forward. Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/15/14 23:07, Tomasz Figa wrote: > Arnd, Kukjin, Daniel, > > On 12.05.2014 17:18, Daniel Lezcano wrote: >> On 05/09/2014 02:02 PM, Tomasz Figa wrote: >>> Hi Arnd, >>> >>> On 09.05.2014 12:56, Arnd Bergmann wrote: >>>> On Friday 11 April 2014, Daniel Lezcano wrote: >>>>> No more dependency on the arch code. The platform_data field is used to set the >>>>> PM callback as the other cpuidle drivers. >>>>> >>>>> Signed-off-by: Daniel Lezcano<daniel.lezcano@linaro.org> >>>>> Reviewed-by: Viresh Kumar<viresh.kumar@linaro.org> >>>>> Reviewed-by: Bartlomiej Zolnierkiewicz<b.zolnierkie@samsung.com> >>>> >>>> This has just shown up in linux-next and broken randconfig builds. >>>> >>>>> diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c >>>>> index fe8dac8..d22f0e4 100644 >>>>> --- a/arch/arm/mach-exynos/exynos.c >>>>> +++ b/arch/arm/mach-exynos/exynos.c >>>>> @@ -221,8 +221,9 @@ void exynos_restart(enum reboot_mode mode, const char *cmd) >>>>> } >>>>> >>>>> static struct platform_device exynos_cpuidle = { >>>>> - .name = "exynos_cpuidle", >>>>> - .id = -1, >>>>> + .name = "exynos_cpuidle", >>>>> + .dev.platform_data = exynos_enter_aftr, >>>>> + .id = -1, >>>>> }; >>>>> >>>> >>>> This is wrong on many levels, can we please do this properly? >>>> >>>> * The exynos_enter_aftr function is compiled conditionally, so you can't just >>>> reference it from generic code, or you get a link error. >>> >>> +1 >> >> That is true but still we have a link error without this patch. We >> shouldn't register and declare this structure if CONFIG_PM / >> CONFIG_CPU_IDLE are not set. >> >>>> * 'static struct platform_device ...' has been deprecated for at least a decade, >>>> stop doing that. For any platform devices that get registered, there is >>>> platform_device_register_simple(). >>> >>> +0.5 >>> >>> The missing 0.5 is because you can't pass platform data using >>> platform_device_register_simple(). There is >>> platform_device_register_resndata(), though. >>> >>>> * There shouldn't need to be a platform_device to start with, this should all >>>> come from DT. We can't do this on arm64 anyway, so any code that may be >>>> shared between arm32 and arm64 should have proper abstractions. >>> >>> -1 >>> >>> Exynos cpuidle is not a device on the SoC, so I don't think there is any >>> way to represent it in DT. The only thing I could see this is matching >>> root node with a central SoC driver that instantiates specific >>> subdevices, such as cpufreq and cpuidle, but I don't see any available >>> infrastructure for this. >> >> There is a RFC for defining generic idle states [1]. >> >> The idea behind using the platform driver framework is to unify the code >> across the different drivers and separate the PM / cpuidle code. >> >> By this way, we can move the different drivers to drivers/cpuidle and >> store them in a single place. That make easier the tracking, the review >> and the maintenance. >> >> I am ok to by using platform_device_register_resndata() but I would >> prefer to do that a bit later by converting the other drivers too. That >> will be easier if we have them grouped in a single directory (this is >> what does this patchset at the end). >> >> As there are some more work based on this patchset and the link error >> could be fixed as an independent patch, I would recommend to >> re-integrate it in the tree as asked by Bartlomiej. > > In general, it would be nice to have everything done properly, but I'd > consider Daniel's series as a huge improvement already and a nice > intermediate step towards further clean-up. > > So based on the comments quoted above, instead of stalling the > development, I'd suggest to accept this series and then move forward. > I'm fine. Arnd, how about you? - Kukjin -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/15/2014 10:40 PM, Kukjin Kim wrote: [ ... ] >>>> Exynos cpuidle is not a device on the SoC, so I don't think there is >>>> any >>>> way to represent it in DT. The only thing I could see this is matching >>>> root node with a central SoC driver that instantiates specific >>>> subdevices, such as cpufreq and cpuidle, but I don't see any available >>>> infrastructure for this. >>> >>> There is a RFC for defining generic idle states [1]. >>> >>> The idea behind using the platform driver framework is to unify the code >>> across the different drivers and separate the PM / cpuidle code. >>> >>> By this way, we can move the different drivers to drivers/cpuidle and >>> store them in a single place. That make easier the tracking, the review >>> and the maintenance. >>> >>> I am ok to by using platform_device_register_resndata() but I would >>> prefer to do that a bit later by converting the other drivers too. That >>> will be easier if we have them grouped in a single directory (this is >>> what does this patchset at the end). >>> >>> As there are some more work based on this patchset and the link error >>> could be fixed as an independent patch, I would recommend to >>> re-integrate it in the tree as asked by Bartlomiej. >> >> In general, it would be nice to have everything done properly, but I'd >> consider Daniel's series as a huge improvement already and a nice >> intermediate step towards further clean-up. >> >> So based on the comments quoted above, instead of stalling the >> development, I'd suggest to accept this series and then move forward. >> > I'm fine. > > Arnd, how about you? > > - Kukjin Arnd ?
On Wednesday 21 May 2014 09:15:34 Daniel Lezcano wrote: > On 05/15/2014 10:40 PM, Kukjin Kim wrote: > > [ ... ] > > >>>> Exynos cpuidle is not a device on the SoC, so I don't think there is > >>>> any > >>>> way to represent it in DT. The only thing I could see this is matching > >>>> root node with a central SoC driver that instantiates specific > >>>> subdevices, such as cpufreq and cpuidle, but I don't see any available > >>>> infrastructure for this. > >>> > >>> There is a RFC for defining generic idle states [1]. > >>> > >>> The idea behind using the platform driver framework is to unify the code > >>> across the different drivers and separate the PM / cpuidle code. > >>> > >>> By this way, we can move the different drivers to drivers/cpuidle and > >>> store them in a single place. That make easier the tracking, the review > >>> and the maintenance. Yes, that would be great. I only looked briefly at the series now, doesn't that require the use of psci? That's not a bad idea of course, but it doesn't solve the problem I raised here. > >>> I am ok to by using platform_device_register_resndata() but I would > >>> prefer to do that a bit later by converting the other drivers too. That > >>> will be easier if we have them grouped in a single directory (this is > >>> what does this patchset at the end). > >>> > >>> As there are some more work based on this patchset and the link error > >>> could be fixed as an independent patch, I would recommend to > >>> re-integrate it in the tree as asked by Bartlomiej. > >> > >> In general, it would be nice to have everything done properly, but I'd > >> consider Daniel's series as a huge improvement already and a nice > >> intermediate step towards further clean-up. > >> > >> So based on the comments quoted above, instead of stalling the > >> development, I'd suggest to accept this series and then move forward. > >> > > I'm fine. > > > > Arnd, how about you? > > > > - Kukjin > > Arnd ? Sorry for the delay. Yes, let's do it this way once more, but please come up with something better for the future that doesn't tie the cpuidle method to the root compatible string as this does. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/21/2014 10:10 AM, Arnd Bergmann wrote: > On Wednesday 21 May 2014 09:15:34 Daniel Lezcano wrote: >> On 05/15/2014 10:40 PM, Kukjin Kim wrote: >> >> [ ... ] >> >>>>>> Exynos cpuidle is not a device on the SoC, so I don't think there is >>>>>> any >>>>>> way to represent it in DT. The only thing I could see this is matching >>>>>> root node with a central SoC driver that instantiates specific >>>>>> subdevices, such as cpufreq and cpuidle, but I don't see any available >>>>>> infrastructure for this. >>>>> >>>>> There is a RFC for defining generic idle states [1]. >>>>> >>>>> The idea behind using the platform driver framework is to unify the code >>>>> across the different drivers and separate the PM / cpuidle code. >>>>> >>>>> By this way, we can move the different drivers to drivers/cpuidle and >>>>> store them in a single place. That make easier the tracking, the review >>>>> and the maintenance. > > Yes, that would be great. I only looked briefly at the series now, doesn't > that require the use of psci? No, because PSCI is for some specific platform (eg. calxeda), all the other drivers are legacy and manually handling the PM via some low level callbacks. This is why all drivers were implemented all over the place making so difficult to maintain them. Little by little, we split the PM callbacks from the idle algorithm so reducing the platform dependency with the generic code. The PSCI implements such PM callbacks in the firmware directly and are accessed through an API. PSCI is, let's say some kindof nextgen cpuidle. It is similar than mwait on Intel. But if a new platform does not have such firmware, then the cpuidle driver will have the legacy format. > That's not a bad idea of course, but it > doesn't solve the problem I raised here. > >>>>> I am ok to by using platform_device_register_resndata() but I would >>>>> prefer to do that a bit later by converting the other drivers too. That >>>>> will be easier if we have them grouped in a single directory (this is >>>>> what does this patchset at the end). >>>>> >>>>> As there are some more work based on this patchset and the link error >>>>> could be fixed as an independent patch, I would recommend to >>>>> re-integrate it in the tree as asked by Bartlomiej. >>>> >>>> In general, it would be nice to have everything done properly, but I'd >>>> consider Daniel's series as a huge improvement already and a nice >>>> intermediate step towards further clean-up. >>>> >>>> So based on the comments quoted above, instead of stalling the >>>> development, I'd suggest to accept this series and then move forward. >>>> >>> I'm fine. >>> >>> Arnd, how about you? >>> >>> - Kukjin >> >> Arnd ? > > Sorry for the delay. No problem. > Yes, let's do it this way once more, but please > come up with something better for the future that doesn't tie the > cpuidle method to the root compatible string as this does. ok. Thanks -- Daniel
Arnd Bergmann wrote: > > On Wednesday 21 May 2014 09:15:34 Daniel Lezcano wrote: > > On 05/15/2014 10:40 PM, Kukjin Kim wrote: > > > > [ ... ] > > > > >>>> Exynos cpuidle is not a device on the SoC, so I don't think there > is > > >>>> any > > >>>> way to represent it in DT. The only thing I could see this is > matching > > >>>> root node with a central SoC driver that instantiates specific > > >>>> subdevices, such as cpufreq and cpuidle, but I don't see any > available > > >>>> infrastructure for this. > > >>> > > >>> There is a RFC for defining generic idle states [1]. > > >>> > > >>> The idea behind using the platform driver framework is to unify the > code > > >>> across the different drivers and separate the PM / cpuidle code. > > >>> > > >>> By this way, we can move the different drivers to drivers/cpuidle > and > > >>> store them in a single place. That make easier the tracking, the > review > > >>> and the maintenance. > > Yes, that would be great. I only looked briefly at the series now, doesn't > that require the use of psci? That's not a bad idea of course, but it > doesn't solve the problem I raised here. > > > >>> I am ok to by using platform_device_register_resndata() but I would > > >>> prefer to do that a bit later by converting the other drivers too. > That > > >>> will be easier if we have them grouped in a single directory (this > is > > >>> what does this patchset at the end). > > >>> > > >>> As there are some more work based on this patchset and the link > error > > >>> could be fixed as an independent patch, I would recommend to > > >>> re-integrate it in the tree as asked by Bartlomiej. > > >> > > >> In general, it would be nice to have everything done properly, but > I'd > > >> consider Daniel's series as a huge improvement already and a nice > > >> intermediate step towards further clean-up. > > >> > > >> So based on the comments quoted above, instead of stalling the > > >> development, I'd suggest to accept this series and then move forward. > > >> > > > I'm fine. > > > > > > Arnd, how about you? > > > > > > - Kukjin > > > > Arnd ? > > Sorry for the delay. Yes, let's do it this way once more, but please > come up with something better for the future that doesn't tie the > cpuidle method to the root compatible string as this does. > Good! I will include this series into for-next and 2nd round of samsung pull-request for 3.16. Thanks, Kukjin -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 21 May 2014 11:02:29 Daniel Lezcano wrote: > On 05/21/2014 10:10 AM, Arnd Bergmann wrote: > > On Wednesday 21 May 2014 09:15:34 Daniel Lezcano wrote: > >> On 05/15/2014 10:40 PM, Kukjin Kim wrote: > >> > >> [ ... ] > >> > >>>>>> Exynos cpuidle is not a device on the SoC, so I don't think there is > >>>>>> any > >>>>>> way to represent it in DT. The only thing I could see this is matching > >>>>>> root node with a central SoC driver that instantiates specific > >>>>>> subdevices, such as cpufreq and cpuidle, but I don't see any available > >>>>>> infrastructure for this. > >>>>> > >>>>> There is a RFC for defining generic idle states [1]. > >>>>> > >>>>> The idea behind using the platform driver framework is to unify the code > >>>>> across the different drivers and separate the PM / cpuidle code. > >>>>> > >>>>> By this way, we can move the different drivers to drivers/cpuidle and > >>>>> store them in a single place. That make easier the tracking, the review > >>>>> and the maintenance. > > > > Yes, that would be great. I only looked briefly at the series now, doesn't > > that require the use of psci? > > No, because PSCI is for some specific platform (eg. calxeda), all the > other drivers are legacy and manually handling the PM via some low level > callbacks. This is why all drivers were implemented all over the place > making so difficult to maintain them. Little by little, we split the PM > callbacks from the idle algorithm so reducing the platform dependency > with the generic code. > > The PSCI implements such PM callbacks in the firmware directly and are > accessed through an API. PSCI is, let's say some kindof nextgen cpuidle. > It is similar than mwait on Intel. But if a new platform does not have > such firmware, then the cpuidle driver will have the legacy format. Ok, thanks for the exlanation. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c index 02609ac..1d1222e 100644 --- a/arch/arm/mach-exynos/cpuidle.c +++ b/arch/arm/mach-exynos/cpuidle.c @@ -27,7 +27,7 @@ #include <mach/map.h> -#include "common.h" +static void (*exynos_enter_aftr)(void); static int idle_finisher(unsigned long flags) { @@ -86,6 +86,8 @@ static int exynos_cpuidle_probe(struct platform_device *pdev) { int ret; + exynos_enter_aftr = (void *)(pdev->dev.platform_data); + ret = cpuidle_register(&exynos_idle_driver, NULL); if (ret) { dev_err(&pdev->dev, "failed to register cpuidle driver\n"); diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c index fe8dac8..d22f0e4 100644 --- a/arch/arm/mach-exynos/exynos.c +++ b/arch/arm/mach-exynos/exynos.c @@ -221,8 +221,9 @@ void exynos_restart(enum reboot_mode mode, const char *cmd) } static struct platform_device exynos_cpuidle = { - .name = "exynos_cpuidle", - .id = -1, + .name = "exynos_cpuidle", + .dev.platform_data = exynos_enter_aftr, + .id = -1, }; void __init exynos_cpuidle_init(void)