| Message ID | 20130809094832.6530.9438.sendpatchset@w520 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Magnus, Thank you for the patch. On Friday 09 August 2013 18:48:32 Magnus Damm wrote: > From: Magnus Damm <damm@opensource.se> > > No need to special case sh73a0 ->init_machine(), > so get rid of undesired cpufreq platform device > from the generic long term sh73a0 DT support code. > > For short term support on KZM9D the DT reference > implementation now adds a "cpufreq-cpu0" platform > device so that can be used for development. Doesn't this go against the spirit of the -reference platforms that don't use DT devices in board code ? I don't see an urgent need for this, how far along are the DT cpufreq-related bindings ? > Signed-off-by: Magnus Damm <damm@opensource.se> > --- > > arch/arm/mach-shmobile/board-kzm9g-reference.c | 9 ++++++++- > arch/arm/mach-shmobile/include/mach/sh73a0.h | 2 +- > arch/arm/mach-shmobile/setup-sh73a0.c | 23 ++++++-------------- > 3 files changed, 15 insertions(+), 19 deletions(-) > > --- 0001/arch/arm/mach-shmobile/board-kzm9g-reference.c > +++ work/arch/arm/mach-shmobile/board-kzm9g-reference.c 2013-08-08 > 15:56:28.000000000 +0900 @@ -33,7 +33,14 @@ > > static void __init kzm_init(void) > { > - sh73a0_add_standard_devices_dt(); > + /* clocks are setup late during boot in the case of DT */ > + sh73a0_clock_init(); > + > + sh73a0_add_dt_devices(); > + > + of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > + > + platform_device_register_simple("cpufreq-cpu0", -1, NULL, 0); > > #ifdef CONFIG_CACHE_L2X0 > /* Early BRESP enable, Shared attribute override enable, 64K*8way */ > --- 0001/arch/arm/mach-shmobile/include/mach/sh73a0.h > +++ work/arch/arm/mach-shmobile/include/mach/sh73a0.h 2013-08-08 > 15:54:26.000000000 +0900 @@ -78,7 +78,7 @@ extern void sh73a0_map_io(void); > extern void sh73a0_earlytimer_init(void); > extern void sh73a0_add_early_devices(void); > extern void sh73a0_add_standard_devices(void); > -extern void sh73a0_add_standard_devices_dt(void); > +extern void sh73a0_add_dt_devices(void); > extern void sh73a0_clock_init(void); > extern void sh73a0_pinmux_init(void); > extern void sh73a0_pm_init(void); > --- 0001/arch/arm/mach-shmobile/setup-sh73a0.c > +++ work/arch/arm/mach-shmobile/setup-sh73a0.c 2013-08-08 15:57:42.000000000 > +0900 @@ -23,7 +23,6 @@ > #include <linux/interrupt.h> > #include <linux/irq.h> > #include <linux/platform_device.h> > -#include <linux/of_platform.h> > #include <linux/delay.h> > #include <linux/input.h> > #include <linux/io.h> > @@ -901,6 +900,12 @@ static struct platform_device *sh73a0_la > > #define SRCR2 IOMEM(0xe61580b0) > > +void __init sh73a0_add_dt_devices(void) > +{ > + platform_add_devices(sh73a0_devices_dt, > + ARRAY_SIZE(sh73a0_devices_dt)); > +} > + > void __init sh73a0_add_standard_devices(void) > { > /* Clear software reset bit on SY-DMAC module */ > @@ -943,21 +948,6 @@ void __init sh73a0_add_early_devices(voi > > #ifdef CONFIG_USE_OF > > -void __init sh73a0_add_standard_devices_dt(void) > -{ > - struct platform_device_info devinfo = { .name = "cpufreq-cpu0", .id = -1, > }; - > - /* clocks are setup late during boot in the case of DT */ > - sh73a0_clock_init(); > - > - platform_add_devices(sh73a0_devices_dt, > - ARRAY_SIZE(sh73a0_devices_dt)); > - of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > - > - /* Instantiate cpufreq-cpu0 */ > - platform_device_register_full(&devinfo); > -} > - > static const char *sh73a0_boards_compat_dt[] __initdata = { > "renesas,sh73a0", > NULL, > @@ -968,7 +958,6 @@ DT_MACHINE_START(SH73A0_DT, "Generic SH7 > .map_io = sh73a0_map_io, > .init_early = sh73a0_init_delay, > .nr_irqs = NR_IRQS_LEGACY, > - .init_machine = sh73a0_add_standard_devices_dt, > .dt_compat = sh73a0_boards_compat_dt, > MACHINE_END > #endif /* CONFIG_USE_OF */
Hi Laurent, On Fri, Aug 9, 2013 at 7:47 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Magnus, > > Thank you for the patch. > > On Friday 09 August 2013 18:48:32 Magnus Damm wrote: >> From: Magnus Damm <damm@opensource.se> >> >> No need to special case sh73a0 ->init_machine(), >> so get rid of undesired cpufreq platform device >> from the generic long term sh73a0 DT support code. >> >> For short term support on KZM9D the DT reference >> implementation now adds a "cpufreq-cpu0" platform >> device so that can be used for development. > > Doesn't this go against the spirit of the -reference platforms that don't use > DT devices in board code ? I don't see an urgent need for this, how far along > are the DT cpufreq-related bindings ? I'm not sure what the latest state of DT cpufreq bindings are. Actually, it seems to me that the cpufreq platform device is a software policy that shouldn't be described with DT. And to be honest, I can't really see how this policy has anything to do with any particular SoC. Regarding MACHINE_START on mach-shmobile I'd like to have 3 levels: 1) MACHINE_START in setup-xxxx.c This is where we aim to be in the long term. We should be as theoretically correct as ever possible here. I'd like to avoid all platform devices here. Also, I'd like to avoid short term random policy setting here. 2) MACHINE_START in board-xxx-reference.c This is where we build DT support for the board. In case we don't have DT bindings for a certain device then using platform device here is acceptable. Other short term workarounds are also acceptable here. 3) MACHINE_START in board-xxx.c This is the location of the C-based board support. Some SoC-specific bits like CPU cores and interrupt controllers may be initialized via DT. Otherwise platform devices are used. Hope this clarifies my view! Cheers, / magnus
Hi Magnus, On Wednesday 28 August 2013 15:40:50 Magnus Damm wrote: > On Fri, Aug 9, 2013 at 7:47 PM, Laurent Pinchart wrote: > > On Friday 09 August 2013 18:48:32 Magnus Damm wrote: > >> From: Magnus Damm <damm@opensource.se> > >> > >> No need to special case sh73a0 ->init_machine(), > >> so get rid of undesired cpufreq platform device > >> from the generic long term sh73a0 DT support code. > >> > >> For short term support on KZM9D the DT reference > >> implementation now adds a "cpufreq-cpu0" platform > >> device so that can be used for development. > > > > Doesn't this go against the spirit of the -reference platforms that don't > > use DT devices in board code ? I don't see an urgent need for this, how > > far along are the DT cpufreq-related bindings ? > > I'm not sure what the latest state of DT cpufreq bindings are. Actually, it > seems to me that the cpufreq platform device is a software policy that > shouldn't be described with DT. And to be honest, I can't really see how > this policy has anything to do with any particular SoC. I'm no cpufreq expert, but if we need to register the device in C code, I'm pretty uneasy with having that code in board files. One of the DT goals is to get rid of most board files. > Regarding MACHINE_START on mach-shmobile I'd like to have 3 levels: > > 1) MACHINE_START in setup-xxxx.c > > This is where we aim to be in the long term. We should be as > theoretically correct as ever possible here. I'd like to avoid all > platform devices here. Also, I'd like to avoid short term random > policy setting here. > > 2) MACHINE_START in board-xxx-reference.c > > This is where we build DT support for the board. In case we don't have > DT bindings for a certain device then using platform device here is > acceptable. Other short term workarounds are also acceptable here. > > 3) MACHINE_START in board-xxx.c > > This is the location of the C-based board support. Some SoC-specific > bits like CPU cores and interrupt controllers may be initialized via > DT. Otherwise platform devices are used. > > Hope this clarifies my view!
Hi Laurent, On Wed, Aug 28, 2013 at 9:08 PM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Magnus, > > On Wednesday 28 August 2013 15:40:50 Magnus Damm wrote: >> On Fri, Aug 9, 2013 at 7:47 PM, Laurent Pinchart wrote: >> > On Friday 09 August 2013 18:48:32 Magnus Damm wrote: >> >> From: Magnus Damm <damm@opensource.se> >> >> >> >> No need to special case sh73a0 ->init_machine(), >> >> so get rid of undesired cpufreq platform device >> >> from the generic long term sh73a0 DT support code. >> >> >> >> For short term support on KZM9D the DT reference >> >> implementation now adds a "cpufreq-cpu0" platform >> >> device so that can be used for development. >> > >> > Doesn't this go against the spirit of the -reference platforms that don't >> > use DT devices in board code ? I don't see an urgent need for this, how >> > far along are the DT cpufreq-related bindings ? >> >> I'm not sure what the latest state of DT cpufreq bindings are. Actually, it >> seems to me that the cpufreq platform device is a software policy that >> shouldn't be described with DT. And to be honest, I can't really see how >> this policy has anything to do with any particular SoC. > > I'm no cpufreq expert, but if we need to register the device in C code, I'm > pretty uneasy with having that code in board files. One of the DT goals is to > get rid of most board files. I'm not sure if we actually _have_to_ register via the platform device, or if it just happens to be like that today because no one has bothered creating a better abstraction. It is a mystery to me that both a platform device is used to select actual driver, and then DT is used to provide frequency and voltage information. The cpufreq software policy is neither board nor SoC specific. It must be application specific. I can understand that putting it in a board file seems odd, but putting it in a SoC file is IMO equally odd, and with the added damage that people starting to write generic DT code may assume that the SoC will keep on using the same cpufreq software policy in the future. Perhaps the cpufreq registration interface should be reworked somehow? Cheers, / magnus
Hi Magnus, On Wednesday 28 August 2013 21:19:50 Magnus Damm wrote: > On Wed, Aug 28, 2013 at 9:08 PM, Laurent Pinchart wrote: > > On Wednesday 28 August 2013 15:40:50 Magnus Damm wrote: > >> On Fri, Aug 9, 2013 at 7:47 PM, Laurent Pinchart wrote: > >> > On Friday 09 August 2013 18:48:32 Magnus Damm wrote: > >> >> From: Magnus Damm <damm@opensource.se> > >> >> > >> >> No need to special case sh73a0 ->init_machine(), > >> >> so get rid of undesired cpufreq platform device > >> >> from the generic long term sh73a0 DT support code. > >> >> > >> >> For short term support on KZM9D the DT reference > >> >> implementation now adds a "cpufreq-cpu0" platform > >> >> device so that can be used for development. > >> > > >> > Doesn't this go against the spirit of the -reference platforms that > >> > don't use DT devices in board code ? I don't see an urgent need for > >> > this, how far along are the DT cpufreq-related bindings ? > >> > >> I'm not sure what the latest state of DT cpufreq bindings are. Actually, > >> it seems to me that the cpufreq platform device is a software policy that > >> shouldn't be described with DT. And to be honest, I can't really see how > >> this policy has anything to do with any particular SoC. > > > > I'm no cpufreq expert, but if we need to register the device in C code, > > I'm pretty uneasy with having that code in board files. One of the DT > > goals is to get rid of most board files. > > I'm not sure if we actually _have_to_ register via the platform device, or > if it just happens to be like that today because no one has bothered > creating a better abstraction. It is a mystery to me that both a platform > device is used to select actual driver, and then DT is used to provide > frequency and voltage information. > > The cpufreq software policy is neither board nor SoC specific. It must be > application specific. I can understand that putting it in a board file seems > odd, but putting it in a SoC file is IMO equally odd, and with the added > damage that people starting to write generic DT code may assume that the SoC > will keep on using the same cpufreq software policy in the future. > > Perhaps the cpufreq registration interface should be reworked somehow? Perhaps :-) The situation needs to be at least clarified. Feel free to CC me :-)
Hi Laurent, On Fri, Aug 30, 2013 at 9:30 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > Hi Magnus, > > On Wednesday 28 August 2013 21:19:50 Magnus Damm wrote: >> On Wed, Aug 28, 2013 at 9:08 PM, Laurent Pinchart wrote: >> > On Wednesday 28 August 2013 15:40:50 Magnus Damm wrote: >> >> On Fri, Aug 9, 2013 at 7:47 PM, Laurent Pinchart wrote: >> >> > On Friday 09 August 2013 18:48:32 Magnus Damm wrote: >> >> >> From: Magnus Damm <damm@opensource.se> >> >> >> >> >> >> No need to special case sh73a0 ->init_machine(), >> >> >> so get rid of undesired cpufreq platform device >> >> >> from the generic long term sh73a0 DT support code. >> >> >> >> >> >> For short term support on KZM9D the DT reference >> >> >> implementation now adds a "cpufreq-cpu0" platform >> >> >> device so that can be used for development. >> >> > >> >> > Doesn't this go against the spirit of the -reference platforms that >> >> > don't use DT devices in board code ? I don't see an urgent need for >> >> > this, how far along are the DT cpufreq-related bindings ? >> >> >> >> I'm not sure what the latest state of DT cpufreq bindings are. Actually, >> >> it seems to me that the cpufreq platform device is a software policy that >> >> shouldn't be described with DT. And to be honest, I can't really see how >> >> this policy has anything to do with any particular SoC. >> > >> > I'm no cpufreq expert, but if we need to register the device in C code, >> > I'm pretty uneasy with having that code in board files. One of the DT >> > goals is to get rid of most board files. >> >> I'm not sure if we actually _have_to_ register via the platform device, or >> if it just happens to be like that today because no one has bothered >> creating a better abstraction. It is a mystery to me that both a platform >> device is used to select actual driver, and then DT is used to provide >> frequency and voltage information. >> >> The cpufreq software policy is neither board nor SoC specific. It must be >> application specific. I can understand that putting it in a board file seems >> odd, but putting it in a SoC file is IMO equally odd, and with the added >> damage that people starting to write generic DT code may assume that the SoC >> will keep on using the same cpufreq software policy in the future. >> >> Perhaps the cpufreq registration interface should be reworked somehow? > > Perhaps :-) The situation needs to be at least clarified. Feel free to CC me > :-) Thanks. =) There are CPUFreq dependencies on SMP and multi-cluster frequency scaling for some SoCs, so to begin with I prefer to allow selected boards use whatever old interface that is available now and over time incrementally improve the situation. I don't however want the shared per-SoC DT_MACHINE code in setup-xxx.c to enable anything by default for now though - this portion we can modify when we have SMP and multi-cluster working. Cheers, / magnus
--- 0001/arch/arm/mach-shmobile/board-kzm9g-reference.c +++ work/arch/arm/mach-shmobile/board-kzm9g-reference.c 2013-08-08 15:56:28.000000000 +0900 @@ -33,7 +33,14 @@ static void __init kzm_init(void) { - sh73a0_add_standard_devices_dt(); + /* clocks are setup late during boot in the case of DT */ + sh73a0_clock_init(); + + sh73a0_add_dt_devices(); + + of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); + + platform_device_register_simple("cpufreq-cpu0", -1, NULL, 0); #ifdef CONFIG_CACHE_L2X0 /* Early BRESP enable, Shared attribute override enable, 64K*8way */ --- 0001/arch/arm/mach-shmobile/include/mach/sh73a0.h +++ work/arch/arm/mach-shmobile/include/mach/sh73a0.h 2013-08-08 15:54:26.000000000 +0900 @@ -78,7 +78,7 @@ extern void sh73a0_map_io(void); extern void sh73a0_earlytimer_init(void); extern void sh73a0_add_early_devices(void); extern void sh73a0_add_standard_devices(void); -extern void sh73a0_add_standard_devices_dt(void); +extern void sh73a0_add_dt_devices(void); extern void sh73a0_clock_init(void); extern void sh73a0_pinmux_init(void); extern void sh73a0_pm_init(void); --- 0001/arch/arm/mach-shmobile/setup-sh73a0.c +++ work/arch/arm/mach-shmobile/setup-sh73a0.c 2013-08-08 15:57:42.000000000 +0900 @@ -23,7 +23,6 @@ #include <linux/interrupt.h> #include <linux/irq.h> #include <linux/platform_device.h> -#include <linux/of_platform.h> #include <linux/delay.h> #include <linux/input.h> #include <linux/io.h> @@ -901,6 +900,12 @@ static struct platform_device *sh73a0_la #define SRCR2 IOMEM(0xe61580b0) +void __init sh73a0_add_dt_devices(void) +{ + platform_add_devices(sh73a0_devices_dt, + ARRAY_SIZE(sh73a0_devices_dt)); +} + void __init sh73a0_add_standard_devices(void) { /* Clear software reset bit on SY-DMAC module */ @@ -943,21 +948,6 @@ void __init sh73a0_add_early_devices(voi #ifdef CONFIG_USE_OF -void __init sh73a0_add_standard_devices_dt(void) -{ - struct platform_device_info devinfo = { .name = "cpufreq-cpu0", .id = -1, }; - - /* clocks are setup late during boot in the case of DT */ - sh73a0_clock_init(); - - platform_add_devices(sh73a0_devices_dt, - ARRAY_SIZE(sh73a0_devices_dt)); - of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); - - /* Instantiate cpufreq-cpu0 */ - platform_device_register_full(&devinfo); -} - static const char *sh73a0_boards_compat_dt[] __initdata = { "renesas,sh73a0", NULL, @@ -968,7 +958,6 @@ DT_MACHINE_START(SH73A0_DT, "Generic SH7 .map_io = sh73a0_map_io, .init_early = sh73a0_init_delay, .nr_irqs = NR_IRQS_LEGACY, - .init_machine = sh73a0_add_standard_devices_dt, .dt_compat = sh73a0_boards_compat_dt, MACHINE_END #endif /* CONFIG_USE_OF */