| Message ID | 1373038006-19973-4-git-send-email-g.liakhovetski@gmx.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
Hi Guennadi, On Sat, Jul 6, 2013 at 12:26 AM, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > Add CPUFreq support to ape6evm-reference, using a max8973 regulator, that > is supplying V_DVFS for the 4 CA15 cores on r8a73a4. > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com> > --- > > v2: extracted from patch 2 from v1 > > arch/arm/boot/dts/r8a73a4-ape6evm-reference.dts | 22 ++++++++++++++++++++++ > arch/arm/mach-shmobile/board-ape6evm-reference.c | 1 + > 2 files changed, 23 insertions(+), 0 deletions(-) > > diff --git a/arch/arm/boot/dts/r8a73a4-ape6evm-reference.dts b/arch/arm/boot/dts/r8a73a4-ape6evm-reference.dts > index 4990076..aa84b09 100644 > --- a/arch/arm/boot/dts/r8a73a4-ape6evm-reference.dts > +++ b/arch/arm/boot/dts/r8a73a4-ape6evm-reference.dts > @@ -31,3 +31,25 @@ > ranges = <0 0 0 0x80000000>; > }; > }; > + > +&i2c5 { > + vdd_dvfs: max8973@1b { > + compatible = "maxim,max8973"; > + reg = <0x1b>; > + > + regulator-min-microvolt = <935000>; > + regulator-max-microvolt = <1200000>; > + regulator-boot-on; > + regulator-always-on; > + }; > +}; > + > +&cpu0 { > + cpu0-supply = <&vdd_dvfs>; > + operating-points = < > + /* kHz uV */ > + 1950000 1115000 > + 1462500 995000 > + >; > + voltage-tolerance = <1>; /* 1% */ > +}; > diff --git a/arch/arm/mach-shmobile/board-ape6evm-reference.c b/arch/arm/mach-shmobile/board-ape6evm-reference.c > index 7e3e0d7..ee9f75d 100644 > --- a/arch/arm/mach-shmobile/board-ape6evm-reference.c > +++ b/arch/arm/mach-shmobile/board-ape6evm-reference.c > @@ -56,6 +56,7 @@ static void __init ape6evm_add_standard_devices(void) > r8a73a4_pinmux_init(); > r8a73a4_add_aux_devices_dt(); > of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > + r8a73a4_add_cpufreq_device_dt(); Instead of adding a special function with ifdefs and whatnot in the r8a73a4 code and calling r8a73a4_add_cpufreq_device_dt() from this file, please update the code to simply use the following directly from this file: platform_device_register_simple("cpufreq-cpu0", -1, NULL, 0); Thanks, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 8 Jul 2013, Magnus Damm wrote: > Hi Guennadi, > > On Sat, Jul 6, 2013 at 12:26 AM, Guennadi Liakhovetski > <g.liakhovetski@gmx.de> wrote: > > Add CPUFreq support to ape6evm-reference, using a max8973 regulator, that > > is supplying V_DVFS for the 4 CA15 cores on r8a73a4. > > > > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com> > > --- > > > > v2: extracted from patch 2 from v1 > > > > arch/arm/boot/dts/r8a73a4-ape6evm-reference.dts | 22 ++++++++++++++++++++++ > > arch/arm/mach-shmobile/board-ape6evm-reference.c | 1 + > > 2 files changed, 23 insertions(+), 0 deletions(-) > > > > diff --git a/arch/arm/boot/dts/r8a73a4-ape6evm-reference.dts b/arch/arm/boot/dts/r8a73a4-ape6evm-reference.dts > > index 4990076..aa84b09 100644 > > --- a/arch/arm/boot/dts/r8a73a4-ape6evm-reference.dts > > +++ b/arch/arm/boot/dts/r8a73a4-ape6evm-reference.dts > > @@ -31,3 +31,25 @@ > > ranges = <0 0 0 0x80000000>; > > }; > > }; > > + > > +&i2c5 { > > + vdd_dvfs: max8973@1b { > > + compatible = "maxim,max8973"; > > + reg = <0x1b>; > > + > > + regulator-min-microvolt = <935000>; > > + regulator-max-microvolt = <1200000>; > > + regulator-boot-on; > > + regulator-always-on; > > + }; > > +}; > > + > > +&cpu0 { > > + cpu0-supply = <&vdd_dvfs>; > > + operating-points = < > > + /* kHz uV */ > > + 1950000 1115000 > > + 1462500 995000 > > + >; > > + voltage-tolerance = <1>; /* 1% */ > > +}; > > diff --git a/arch/arm/mach-shmobile/board-ape6evm-reference.c b/arch/arm/mach-shmobile/board-ape6evm-reference.c > > index 7e3e0d7..ee9f75d 100644 > > --- a/arch/arm/mach-shmobile/board-ape6evm-reference.c > > +++ b/arch/arm/mach-shmobile/board-ape6evm-reference.c > > @@ -56,6 +56,7 @@ static void __init ape6evm_add_standard_devices(void) > > r8a73a4_pinmux_init(); > > r8a73a4_add_aux_devices_dt(); > > of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); > > + r8a73a4_add_cpufreq_device_dt(); > > Instead of adding a special function with ifdefs and whatnot in the > r8a73a4 code and calling r8a73a4_add_cpufreq_device_dt() from this > file, please update the code to simply use the following directly from > this file: > > platform_device_register_simple("cpufreq-cpu0", -1, NULL, 0); I thought it was your idea to hide the details of CPUFreq device registration from boards in an SoC-specific exported function: <quote> In the particular case of cpufreq, I expect a platform device to be added to board-yyy.c or since it is a per-SoC property it may be better to use setup-xxx.c. Please note that I don't want the DT_MACHINE bits in setup-xxx.c to make use of that platform device, instead a SoC specific function shall be used that can be called from ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the C board code. ^^^^^^^^^^^^^^^^ </quote> Or have I misunderstood? As for your other idea: <quote> Also, in parallel with platform device in C board code you should add DT nodes for the DT reference board support. </quote> Do you mean adding that "cpufreq-cpu0" device from DT? That's (currently) impossible. The cpufreq-cpu0.c driver doesn't have an .of_match_table field. Since otherwise this is a DT-specific driver - it parses CPU0 DT data - I don't think that was an oversight of the author, instead it must have been their conscious decision. Exact reasons are unknown to me. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 8 Jul 2013, Guennadi Liakhovetski wrote: [snip] > Or have I misunderstood? As for your other idea: > > <quote> > Also, in parallel with platform device in C board > code you should add DT nodes for the DT reference board support. > </quote> > > Do you mean adding that "cpufreq-cpu0" device from DT? That's (currently) > impossible. The cpufreq-cpu0.c driver doesn't have an .of_match_table > field. Since otherwise this is a DT-specific driver - it parses CPU0 DT > data - I don't think that was an oversight of the author, instead it must > have been their conscious decision. Exact reasons are unknown to me. Ok, I think, I know why. Probably, because that "device" doesn't describe "real hardware," instead, it's just a driver activation helper. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 8, 2013 at 5:00 PM, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > On Mon, 8 Jul 2013, Guennadi Liakhovetski wrote: > > [snip] > >> Or have I misunderstood? As for your other idea: >> >> <quote> >> Also, in parallel with platform device in C board >> code you should add DT nodes for the DT reference board support. >> </quote> >> >> Do you mean adding that "cpufreq-cpu0" device from DT? That's (currently) >> impossible. The cpufreq-cpu0.c driver doesn't have an .of_match_table >> field. Since otherwise this is a DT-specific driver - it parses CPU0 DT >> data - I don't think that was an oversight of the author, instead it must >> have been their conscious decision. Exact reasons are unknown to me. > > Ok, I think, I know why. Probably, because that "device" doesn't describe > "real hardware," instead, it's just a driver activation helper. Well, it is most likely a software application configuration thing hidden as a platform device, so obviously it should not be described via DT. But regardless of that: - I don't want it called from DT_MACHINE in setup-xxx.c (It is not a SoC-specific thing, so don't make my SoC code ugly, solve it in a generic way instead) - I don't want you to add tiny functions and header files that just result in a single line of code. (Call it directly from the DT reference board code until you figure out how to do it in a generic way) Thanks, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 8 Jul 2013, Magnus Damm wrote: > On Mon, Jul 8, 2013 at 5:00 PM, Guennadi Liakhovetski > <g.liakhovetski@gmx.de> wrote: > > On Mon, 8 Jul 2013, Guennadi Liakhovetski wrote: > > > > [snip] > > > >> Or have I misunderstood? As for your other idea: > >> > >> <quote> > >> Also, in parallel with platform device in C board > >> code you should add DT nodes for the DT reference board support. > >> </quote> > >> > >> Do you mean adding that "cpufreq-cpu0" device from DT? That's (currently) > >> impossible. The cpufreq-cpu0.c driver doesn't have an .of_match_table > >> field. Since otherwise this is a DT-specific driver - it parses CPU0 DT > >> data - I don't think that was an oversight of the author, instead it must > >> have been their conscious decision. Exact reasons are unknown to me. > > > > Ok, I think, I know why. Probably, because that "device" doesn't describe > > "real hardware," instead, it's just a driver activation helper. > > Well, it is most likely a software application configuration thing > hidden as a platform device, so obviously it should not be described > via DT. exactly > But regardless of that: > - I don't want it called from DT_MACHINE in setup-xxx.c > (It is not a SoC-specific thing, so don't make my SoC code ugly, solve > it in a generic way instead) I think, this is exactly how it is designed atm: it is assumed to be an SoC-specific thing which driver to use for CPUFreq, and that's exactly what other cpufreq-cpu0 users (imx) do now to register the device - instantiate it from .init_machine in their DT_MACHINE_START() block. So, I don't see an alternative way to add cpufreq-cpu0 support to generic SoC support. But if you think, we might have boards with APE6, where we don't want this device to be registered at all, sure, we can remove it. But then we won't be able to use cpufreq-cpu0 on any APE6 boards with the generic DT_MACHINE at all. Basically, I see the following 3 possibilities in our case: 1. .config - you don't want to depend on this for multiplatform support 2. DT - you cannot regiater the cpufreq-cpu0 device from it, because it's not describing hardware, and they don't want to run _any_ code from this driver on unsupported hardware 3. DT_MACHINE Other possibilities like kernel parameter or user-space - not sure they are good enough either. > - I don't want you to add tiny functions and header files that just > result in a single line of code. > (Call it directly from the DT reference board code until you figure > out how to do it in a generic way) That I don't care that much about. If I misunderstood your wish - sorry, I can remove that function and register the device directly. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guenandi, On Mon, Jul 8, 2013 at 6:26 PM, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote: > On Mon, 8 Jul 2013, Magnus Damm wrote: > >> On Mon, Jul 8, 2013 at 5:00 PM, Guennadi Liakhovetski >> <g.liakhovetski@gmx.de> wrote: >> > On Mon, 8 Jul 2013, Guennadi Liakhovetski wrote: >> > >> > [snip] >> > >> >> Or have I misunderstood? As for your other idea: >> >> >> >> <quote> >> >> Also, in parallel with platform device in C board >> >> code you should add DT nodes for the DT reference board support. >> >> </quote> >> >> >> >> Do you mean adding that "cpufreq-cpu0" device from DT? That's (currently) >> >> impossible. The cpufreq-cpu0.c driver doesn't have an .of_match_table >> >> field. Since otherwise this is a DT-specific driver - it parses CPU0 DT >> >> data - I don't think that was an oversight of the author, instead it must >> >> have been their conscious decision. Exact reasons are unknown to me. >> > >> > Ok, I think, I know why. Probably, because that "device" doesn't describe >> > "real hardware," instead, it's just a driver activation helper. >> >> Well, it is most likely a software application configuration thing >> hidden as a platform device, so obviously it should not be described >> via DT. > > exactly > >> But regardless of that: >> - I don't want it called from DT_MACHINE in setup-xxx.c >> (It is not a SoC-specific thing, so don't make my SoC code ugly, solve >> it in a generic way instead) > > I think, this is exactly how it is designed atm: it is assumed to be an > SoC-specific thing which driver to use for CPUFreq, and that's exactly > what other cpufreq-cpu0 users (imx) do now to register the device - We want to use cpufreq to scale the CPU clock frequency. As you probably know, all ARM cores are driven by clocks, so this is not a unique problem to a certain SoC. However, depending on the hardware clock scaling may or may not be available. In some cases you may have multiple clocks that need scaling. So it's a not a SoC specific issue IMO. It needs to be solved somehow in a coherent way for MULTIPLATFORM. > instantiate it from .init_machine in their DT_MACHINE_START() block. So, I > don't see an alternative way to add cpufreq-cpu0 support to generic SoC > support. But if you think, we might have boards with APE6, where we don't > want this device to be registered at all, sure, we can remove it. But then > we won't be able to use cpufreq-cpu0 on any APE6 boards with the generic > DT_MACHINE at all. It boils down to that I don't want anyone to use the DT_MACHINE stuff in setup-xxx.c and assume something "special" will be added. We want to be standard. Is the standard not good enough? Then improve it, on a higher level. > Basically, I see the following 3 possibilities in our case: > > 1. .config - you don't want to depend on this for multiplatform support > 2. DT - you cannot regiater the cpufreq-cpu0 device from it, because it's > not describing hardware, and they don't want to run _any_ code from this > driver on unsupported hardware But you already have the following information in DT, no? +&cpu0 { + cpu0-supply = <&vdd_dvfs>; + operating-points = < + /* kHz uV */ + 1950000 1115000 + 1462500 995000 + >; + voltage-tolerance = <1>; /* 1% */ +}; So which software is going to consume this information? > 3. DT_MACHINE Or you can work with the community to figure out a more generic way how to enable this in a better way. > Other possibilities like kernel parameter or user-space - not sure they > are good enough either. Well, to me anything is better than closet hacking where non-SoC properties are put in SoC specific code. >> - I don't want you to add tiny functions and header files that just >> result in a single line of code. >> (Call it directly from the DT reference board code until you figure >> out how to do it in a generic way) > > That I don't care that much about. If I misunderstood your wish - sorry, I > can remove that function and register the device directly. Please do. Thanks, / magnus -- To unsubscribe from this list: send the line "unsubscribe linux-sh" 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/boot/dts/r8a73a4-ape6evm-reference.dts b/arch/arm/boot/dts/r8a73a4-ape6evm-reference.dts index 4990076..aa84b09 100644 --- a/arch/arm/boot/dts/r8a73a4-ape6evm-reference.dts +++ b/arch/arm/boot/dts/r8a73a4-ape6evm-reference.dts @@ -31,3 +31,25 @@ ranges = <0 0 0 0x80000000>; }; }; + +&i2c5 { + vdd_dvfs: max8973@1b { + compatible = "maxim,max8973"; + reg = <0x1b>; + + regulator-min-microvolt = <935000>; + regulator-max-microvolt = <1200000>; + regulator-boot-on; + regulator-always-on; + }; +}; + +&cpu0 { + cpu0-supply = <&vdd_dvfs>; + operating-points = < + /* kHz uV */ + 1950000 1115000 + 1462500 995000 + >; + voltage-tolerance = <1>; /* 1% */ +}; diff --git a/arch/arm/mach-shmobile/board-ape6evm-reference.c b/arch/arm/mach-shmobile/board-ape6evm-reference.c index 7e3e0d7..ee9f75d 100644 --- a/arch/arm/mach-shmobile/board-ape6evm-reference.c +++ b/arch/arm/mach-shmobile/board-ape6evm-reference.c @@ -56,6 +56,7 @@ static void __init ape6evm_add_standard_devices(void) r8a73a4_pinmux_init(); r8a73a4_add_aux_devices_dt(); of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL); + r8a73a4_add_cpufreq_device_dt(); } static const char *ape6evm_boards_compat_dt[] __initdata = {
Add CPUFreq support to ape6evm-reference, using a max8973 regulator, that is supplying V_DVFS for the 4 CA15 cores on r8a73a4. Signed-off-by: Guennadi Liakhovetski <g.liakhovetski+renesas@gmail.com> --- v2: extracted from patch 2 from v1 arch/arm/boot/dts/r8a73a4-ape6evm-reference.dts | 22 ++++++++++++++++++++++ arch/arm/mach-shmobile/board-ape6evm-reference.c | 1 + 2 files changed, 23 insertions(+), 0 deletions(-)