Re: [PATCH v3] cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs

2016-12-01 Thread Markus Mayer 
On 28 November 2016 at 02:14, Arnd Bergmann  wrote:
> On Tuesday, November 22, 2016 1:32:45 PM CET Markus Mayer wrote:
>> From: Markus Mayer 
>>
>> This CPUfreq driver provides basic frequency scaling for older Broadcom
>> STB SoCs that do not use AVS firmware with DVFS support. There is no
>> support for voltage scaling.
>>
>> Signed-off-by: Markus Mayer 
>> Acked-by: Viresh Kumar 
>
> This causes multiple build errors in linux-next, please fix asap or
> drop the patch again. My feeling is that it's probably too late to
> fix it for v4.10, but that's up to Viresh and Rafael of course.
>
>> +#define BRCMSTB_CPUFREQ_PREFIX   "brcmstb"
>> +#define BRCMSTB_CPUFREQ_NAME BRCMSTB_CPUFREQ_PREFIX "-cpufreq"
>> +
>> +/* We search for these compatible strings. */
>> +#define BRCMSTB_DT_CPU_CLK_CTRL  "brcm,brcmstb-cpu-clk-div"
>> +#define BRCMSTB_DT_MEMC_DDR  "brcm,brcmstb-memc-ddr"
>> +#define BRCM_AVS_CPU_DATA"brcm,avs-cpu-data-mem"
>> +
>> +/* We also need a few clocks in device tree. These are node names. */
>> +#define BRCMSTB_CLK_MDIV_CH0 "cpu_mdiv_ch0"
>> +#define BRCMSTB_CLK_NDIV_INT "cpu_ndiv_int"
>> +#define BRCMSTB_CLK_SW_SCB   "sw_scb"
>
> Not critical but the use of those macros obfuscates the DT interfaces
> here and made it harder to analyse what was going on.
>
> Also, a couple of them are lacking a DT binding.
>
>> +static int get_frequencies(const struct cpufreq_policy *policy,
>> +unsigned int *vco_freq, unsigned int *cpu_freq,
>> +unsigned int *scb_freq)
>> +{
>> + struct clk *cpu_ndiv_int, *sw_scb;
>> +
>> + cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
>> + if (!cpu_ndiv_int)
>> + return -ENODEV;
>> +
>> + sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB);
>> + if (!sw_scb)
>> + return -ENODEV;
>> +
>> + /* return frequencies in kHz */
>> + *vco_freq = clk_get_rate(cpu_ndiv_int) / 1000;
>> + *cpu_freq = clk_get_rate(policy->clk) / 1000;
>> + *scb_freq = clk_get_rate(sw_scb) / 1000;
>> +
>> + return 0;
>> +}
>
> You really can't do this:
>
> ../drivers/cpufreq/brcmstb-cpufreq.c: In function 'get_frequencies':
> ../drivers/cpufreq/brcmstb-cpufreq.c:71:17: error: implicit declaration of 
> function '__clk_lookup';did you mean 'key_lookup'? 
> [-Werror=implicit-function-declaration]
>   cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
>  ^~~~
>
> __clk_lookup is an internal API for the clk providers.
>
> In particular, relying on undocumented internal names of the
> clk provider in a device driver is inappropriate.

Do you happen to know of an "approved" way of looking up a clock node?
Everything we need is in device tree. We can certainly add bindings
for the missing nodes. It just seems somewhat difficult to get at the
information in a clean way.

>> +static const struct of_device_id brcmstb_cpufreq_match[] = {
>> + { .compatible = BRCMSTB_DT_CPU_CLK_CTRL },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match);
>
> This is a simple typo, also causing the build to fail:
>
> FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct 
> platform_device_id)=24 is not a modulo of the size of section 
> __mod_platform___device_table=392.
>
> Arnd

Re: [PATCH v3] cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs

2016-12-01 Thread Markus Mayer 
On 28 November 2016 at 02:14, Arnd Bergmann  wrote:
> On Tuesday, November 22, 2016 1:32:45 PM CET Markus Mayer wrote:
>> From: Markus Mayer 
>>
>> This CPUfreq driver provides basic frequency scaling for older Broadcom
>> STB SoCs that do not use AVS firmware with DVFS support. There is no
>> support for voltage scaling.
>>
>> Signed-off-by: Markus Mayer 
>> Acked-by: Viresh Kumar 
>
> This causes multiple build errors in linux-next, please fix asap or
> drop the patch again. My feeling is that it's probably too late to
> fix it for v4.10, but that's up to Viresh and Rafael of course.
>
>> +#define BRCMSTB_CPUFREQ_PREFIX   "brcmstb"
>> +#define BRCMSTB_CPUFREQ_NAME BRCMSTB_CPUFREQ_PREFIX "-cpufreq"
>> +
>> +/* We search for these compatible strings. */
>> +#define BRCMSTB_DT_CPU_CLK_CTRL  "brcm,brcmstb-cpu-clk-div"
>> +#define BRCMSTB_DT_MEMC_DDR  "brcm,brcmstb-memc-ddr"
>> +#define BRCM_AVS_CPU_DATA"brcm,avs-cpu-data-mem"
>> +
>> +/* We also need a few clocks in device tree. These are node names. */
>> +#define BRCMSTB_CLK_MDIV_CH0 "cpu_mdiv_ch0"
>> +#define BRCMSTB_CLK_NDIV_INT "cpu_ndiv_int"
>> +#define BRCMSTB_CLK_SW_SCB   "sw_scb"
>
> Not critical but the use of those macros obfuscates the DT interfaces
> here and made it harder to analyse what was going on.
>
> Also, a couple of them are lacking a DT binding.
>
>> +static int get_frequencies(const struct cpufreq_policy *policy,
>> +unsigned int *vco_freq, unsigned int *cpu_freq,
>> +unsigned int *scb_freq)
>> +{
>> + struct clk *cpu_ndiv_int, *sw_scb;
>> +
>> + cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
>> + if (!cpu_ndiv_int)
>> + return -ENODEV;
>> +
>> + sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB);
>> + if (!sw_scb)
>> + return -ENODEV;
>> +
>> + /* return frequencies in kHz */
>> + *vco_freq = clk_get_rate(cpu_ndiv_int) / 1000;
>> + *cpu_freq = clk_get_rate(policy->clk) / 1000;
>> + *scb_freq = clk_get_rate(sw_scb) / 1000;
>> +
>> + return 0;
>> +}
>
> You really can't do this:
>
> ../drivers/cpufreq/brcmstb-cpufreq.c: In function 'get_frequencies':
> ../drivers/cpufreq/brcmstb-cpufreq.c:71:17: error: implicit declaration of 
> function '__clk_lookup';did you mean 'key_lookup'? 
> [-Werror=implicit-function-declaration]
>   cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
>  ^~~~
>
> __clk_lookup is an internal API for the clk providers.
>
> In particular, relying on undocumented internal names of the
> clk provider in a device driver is inappropriate.

Do you happen to know of an "approved" way of looking up a clock node?
Everything we need is in device tree. We can certainly add bindings
for the missing nodes. It just seems somewhat difficult to get at the
information in a clean way.

>> +static const struct of_device_id brcmstb_cpufreq_match[] = {
>> + { .compatible = BRCMSTB_DT_CPU_CLK_CTRL },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match);
>
> This is a simple typo, also causing the build to fail:
>
> FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct 
> platform_device_id)=24 is not a modulo of the size of section 
> __mod_platform___device_table=392.
>
> Arnd

Re: [PATCH v3] cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs

2016-11-28 Thread Markus Mayer 
On 28 November 2016 at 12:58, Arnd Bergmann  wrote:
> On Monday, November 28, 2016 9:12:05 AM CET Markus Mayer wrote:
>> >> +static int get_frequencies(const struct cpufreq_policy *policy,
>> >> +unsigned int *vco_freq, unsigned int *cpu_freq,
>> >> +unsigned int *scb_freq)
>> >> +{
>> >> + struct clk *cpu_ndiv_int, *sw_scb;
>> >> +
>> >> + cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
>> >> + if (!cpu_ndiv_int)
>> >> + return -ENODEV;
>> >> +
>> >> + sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB);
>> >> + if (!sw_scb)
>> >> + return -ENODEV;
>> >> +
>> >> + /* return frequencies in kHz */
>> >> + *vco_freq = clk_get_rate(cpu_ndiv_int) / 1000;
>> >> + *cpu_freq = clk_get_rate(policy->clk) / 1000;
>> >> + *scb_freq = clk_get_rate(sw_scb) / 1000;
>> >> +
>> >> + return 0;
>> >> +}
>> >
>> > You really can't do this:
>> >
>> > ../drivers/cpufreq/brcmstb-cpufreq.c: In function 'get_frequencies':
>> > ../drivers/cpufreq/brcmstb-cpufreq.c:71:17: error: implicit declaration of 
>> > function '__clk_lookup';did you mean 'key_lookup'? 
>> > [-Werror=implicit-function-declaration]
>> >   cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
>> >  ^~~~
>> >
>> > __clk_lookup is an internal API for the clk providers.
>> >
>> > In particular, relying on undocumented internal names of the
>> > clk provider in a device driver is inappropriate.
>>
>> What compiler are you using? I didn't get any warnings. Otherwise I
>> would have known right away that something isn't right.
>
> This is a randconfig build with CONFIG_COMMON_CLK=n. There is a different
> problem with COMMON_CLK=y and the cpufreq driver as a loadable module,
> where the symbol causes a link error.
>
> I did not get any warnings either, these are both hard errors.
>
>> >> +static const struct of_device_id brcmstb_cpufreq_match[] = {
>> >> + { .compatible = BRCMSTB_DT_CPU_CLK_CTRL },
>> >> + { }
>> >> +};
>> >> +MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match);
>> >
>> > This is a simple typo, also causing the build to fail:
>> >
>> > FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct 
>> > platform_device_id)=24 is not a modulo of the size of section 
>> > __mod_platform___device_table=392.
>>
>> What is the typo, if I may ask. Again strange, since the build doesn't
>> fail for me. What was the configuration you used?
>
> MODULE_DEVICE_TABLE() is only used when building a loadable module,
> e.g. in allmodconfig.
>
> MODULE_DEVICE_TABLE(platform, ...) is for 'struct platform_device_id'.
> You need to use MODULE_DEVICE_TABLE(of, ...) for 'struct of_device_id'.

Got it. Thanks.

I fixed this problem and the
IS_ENABLED(CONFIG_ARM_BRCMSTB_AVS_CPUFREQ) issue in my copy. I'll need
to look a bit more what I can do instead of calling __clk_lookup().

Regards,
-Markus

Re: [PATCH v3] cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs

2016-11-28 Thread Markus Mayer 
On 28 November 2016 at 12:58, Arnd Bergmann  wrote:
> On Monday, November 28, 2016 9:12:05 AM CET Markus Mayer wrote:
>> >> +static int get_frequencies(const struct cpufreq_policy *policy,
>> >> +unsigned int *vco_freq, unsigned int *cpu_freq,
>> >> +unsigned int *scb_freq)
>> >> +{
>> >> + struct clk *cpu_ndiv_int, *sw_scb;
>> >> +
>> >> + cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
>> >> + if (!cpu_ndiv_int)
>> >> + return -ENODEV;
>> >> +
>> >> + sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB);
>> >> + if (!sw_scb)
>> >> + return -ENODEV;
>> >> +
>> >> + /* return frequencies in kHz */
>> >> + *vco_freq = clk_get_rate(cpu_ndiv_int) / 1000;
>> >> + *cpu_freq = clk_get_rate(policy->clk) / 1000;
>> >> + *scb_freq = clk_get_rate(sw_scb) / 1000;
>> >> +
>> >> + return 0;
>> >> +}
>> >
>> > You really can't do this:
>> >
>> > ../drivers/cpufreq/brcmstb-cpufreq.c: In function 'get_frequencies':
>> > ../drivers/cpufreq/brcmstb-cpufreq.c:71:17: error: implicit declaration of 
>> > function '__clk_lookup';did you mean 'key_lookup'? 
>> > [-Werror=implicit-function-declaration]
>> >   cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
>> >  ^~~~
>> >
>> > __clk_lookup is an internal API for the clk providers.
>> >
>> > In particular, relying on undocumented internal names of the
>> > clk provider in a device driver is inappropriate.
>>
>> What compiler are you using? I didn't get any warnings. Otherwise I
>> would have known right away that something isn't right.
>
> This is a randconfig build with CONFIG_COMMON_CLK=n. There is a different
> problem with COMMON_CLK=y and the cpufreq driver as a loadable module,
> where the symbol causes a link error.
>
> I did not get any warnings either, these are both hard errors.
>
>> >> +static const struct of_device_id brcmstb_cpufreq_match[] = {
>> >> + { .compatible = BRCMSTB_DT_CPU_CLK_CTRL },
>> >> + { }
>> >> +};
>> >> +MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match);
>> >
>> > This is a simple typo, also causing the build to fail:
>> >
>> > FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct 
>> > platform_device_id)=24 is not a modulo of the size of section 
>> > __mod_platform___device_table=392.
>>
>> What is the typo, if I may ask. Again strange, since the build doesn't
>> fail for me. What was the configuration you used?
>
> MODULE_DEVICE_TABLE() is only used when building a loadable module,
> e.g. in allmodconfig.
>
> MODULE_DEVICE_TABLE(platform, ...) is for 'struct platform_device_id'.
> You need to use MODULE_DEVICE_TABLE(of, ...) for 'struct of_device_id'.

Got it. Thanks.

I fixed this problem and the
IS_ENABLED(CONFIG_ARM_BRCMSTB_AVS_CPUFREQ) issue in my copy. I'll need
to look a bit more what I can do instead of calling __clk_lookup().

Regards,
-Markus

Re: [PATCH v3] cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs

2016-11-28 Thread Arnd Bergmann 
On Monday, November 28, 2016 9:12:05 AM CET Markus Mayer wrote:
> >> +static int get_frequencies(const struct cpufreq_policy *policy,
> >> +unsigned int *vco_freq, unsigned int *cpu_freq,
> >> +unsigned int *scb_freq)
> >> +{
> >> + struct clk *cpu_ndiv_int, *sw_scb;
> >> +
> >> + cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
> >> + if (!cpu_ndiv_int)
> >> + return -ENODEV;
> >> +
> >> + sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB);
> >> + if (!sw_scb)
> >> + return -ENODEV;
> >> +
> >> + /* return frequencies in kHz */
> >> + *vco_freq = clk_get_rate(cpu_ndiv_int) / 1000;
> >> + *cpu_freq = clk_get_rate(policy->clk) / 1000;
> >> + *scb_freq = clk_get_rate(sw_scb) / 1000;
> >> +
> >> + return 0;
> >> +}
> >
> > You really can't do this:
> >
> > ../drivers/cpufreq/brcmstb-cpufreq.c: In function 'get_frequencies':
> > ../drivers/cpufreq/brcmstb-cpufreq.c:71:17: error: implicit declaration of 
> > function '__clk_lookup';did you mean 'key_lookup'? 
> > [-Werror=implicit-function-declaration]
> >   cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
> >  ^~~~
> >
> > __clk_lookup is an internal API for the clk providers.
> >
> > In particular, relying on undocumented internal names of the
> > clk provider in a device driver is inappropriate.
> 
> What compiler are you using? I didn't get any warnings. Otherwise I
> would have known right away that something isn't right.

This is a randconfig build with CONFIG_COMMON_CLK=n. There is a different
problem with COMMON_CLK=y and the cpufreq driver as a loadable module,
where the symbol causes a link error.

I did not get any warnings either, these are both hard errors.

> >> +static const struct of_device_id brcmstb_cpufreq_match[] = {
> >> + { .compatible = BRCMSTB_DT_CPU_CLK_CTRL },
> >> + { }
> >> +};
> >> +MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match);
> >
> > This is a simple typo, also causing the build to fail:
> >
> > FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct 
> > platform_device_id)=24 is not a modulo of the size of section 
> > __mod_platform___device_table=392.
> 
> What is the typo, if I may ask. Again strange, since the build doesn't
> fail for me. What was the configuration you used?

MODULE_DEVICE_TABLE() is only used when building a loadable module,
e.g. in allmodconfig.

MODULE_DEVICE_TABLE(platform, ...) is for 'struct platform_device_id'.
You need to use MODULE_DEVICE_TABLE(of, ...) for 'struct of_device_id'.

Arnd

Re: [PATCH v3] cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs

2016-11-28 Thread Arnd Bergmann 
On Monday, November 28, 2016 9:12:05 AM CET Markus Mayer wrote:
> >> +static int get_frequencies(const struct cpufreq_policy *policy,
> >> +unsigned int *vco_freq, unsigned int *cpu_freq,
> >> +unsigned int *scb_freq)
> >> +{
> >> + struct clk *cpu_ndiv_int, *sw_scb;
> >> +
> >> + cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
> >> + if (!cpu_ndiv_int)
> >> + return -ENODEV;
> >> +
> >> + sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB);
> >> + if (!sw_scb)
> >> + return -ENODEV;
> >> +
> >> + /* return frequencies in kHz */
> >> + *vco_freq = clk_get_rate(cpu_ndiv_int) / 1000;
> >> + *cpu_freq = clk_get_rate(policy->clk) / 1000;
> >> + *scb_freq = clk_get_rate(sw_scb) / 1000;
> >> +
> >> + return 0;
> >> +}
> >
> > You really can't do this:
> >
> > ../drivers/cpufreq/brcmstb-cpufreq.c: In function 'get_frequencies':
> > ../drivers/cpufreq/brcmstb-cpufreq.c:71:17: error: implicit declaration of 
> > function '__clk_lookup';did you mean 'key_lookup'? 
> > [-Werror=implicit-function-declaration]
> >   cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
> >  ^~~~
> >
> > __clk_lookup is an internal API for the clk providers.
> >
> > In particular, relying on undocumented internal names of the
> > clk provider in a device driver is inappropriate.
> 
> What compiler are you using? I didn't get any warnings. Otherwise I
> would have known right away that something isn't right.

This is a randconfig build with CONFIG_COMMON_CLK=n. There is a different
problem with COMMON_CLK=y and the cpufreq driver as a loadable module,
where the symbol causes a link error.

I did not get any warnings either, these are both hard errors.

> >> +static const struct of_device_id brcmstb_cpufreq_match[] = {
> >> + { .compatible = BRCMSTB_DT_CPU_CLK_CTRL },
> >> + { }
> >> +};
> >> +MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match);
> >
> > This is a simple typo, also causing the build to fail:
> >
> > FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct 
> > platform_device_id)=24 is not a modulo of the size of section 
> > __mod_platform___device_table=392.
> 
> What is the typo, if I may ask. Again strange, since the build doesn't
> fail for me. What was the configuration you used?

MODULE_DEVICE_TABLE() is only used when building a loadable module,
e.g. in allmodconfig.

MODULE_DEVICE_TABLE(platform, ...) is for 'struct platform_device_id'.
You need to use MODULE_DEVICE_TABLE(of, ...) for 'struct of_device_id'.

Arnd

Re: [PATCH v3] cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs

2016-11-28 Thread Markus Mayer 
On 28 November 2016 at 02:14, Arnd Bergmann  wrote:
> On Tuesday, November 22, 2016 1:32:45 PM CET Markus Mayer wrote:
>> From: Markus Mayer 
>>
>> This CPUfreq driver provides basic frequency scaling for older Broadcom
>> STB SoCs that do not use AVS firmware with DVFS support. There is no
>> support for voltage scaling.
>>
>> Signed-off-by: Markus Mayer 
>> Acked-by: Viresh Kumar 
>
> This causes multiple build errors in linux-next, please fix asap or
> drop the patch again. My feeling is that it's probably too late to
> fix it for v4.10, but that's up to Viresh and Rafael of course.
>
>> +#define BRCMSTB_CPUFREQ_PREFIX   "brcmstb"
>> +#define BRCMSTB_CPUFREQ_NAME BRCMSTB_CPUFREQ_PREFIX "-cpufreq"
>> +
>> +/* We search for these compatible strings. */
>> +#define BRCMSTB_DT_CPU_CLK_CTRL  "brcm,brcmstb-cpu-clk-div"
>> +#define BRCMSTB_DT_MEMC_DDR  "brcm,brcmstb-memc-ddr"
>> +#define BRCM_AVS_CPU_DATA"brcm,avs-cpu-data-mem"
>> +
>> +/* We also need a few clocks in device tree. These are node names. */
>> +#define BRCMSTB_CLK_MDIV_CH0 "cpu_mdiv_ch0"
>> +#define BRCMSTB_CLK_NDIV_INT "cpu_ndiv_int"
>> +#define BRCMSTB_CLK_SW_SCB   "sw_scb"
>
> Not critical but the use of those macros obfuscates the DT interfaces
> here and made it harder to analyse what was going on.
>
> Also, a couple of them are lacking a DT binding.
>
>> +static int get_frequencies(const struct cpufreq_policy *policy,
>> +unsigned int *vco_freq, unsigned int *cpu_freq,
>> +unsigned int *scb_freq)
>> +{
>> + struct clk *cpu_ndiv_int, *sw_scb;
>> +
>> + cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
>> + if (!cpu_ndiv_int)
>> + return -ENODEV;
>> +
>> + sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB);
>> + if (!sw_scb)
>> + return -ENODEV;
>> +
>> + /* return frequencies in kHz */
>> + *vco_freq = clk_get_rate(cpu_ndiv_int) / 1000;
>> + *cpu_freq = clk_get_rate(policy->clk) / 1000;
>> + *scb_freq = clk_get_rate(sw_scb) / 1000;
>> +
>> + return 0;
>> +}
>
> You really can't do this:
>
> ../drivers/cpufreq/brcmstb-cpufreq.c: In function 'get_frequencies':
> ../drivers/cpufreq/brcmstb-cpufreq.c:71:17: error: implicit declaration of 
> function '__clk_lookup';did you mean 'key_lookup'? 
> [-Werror=implicit-function-declaration]
>   cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
>  ^~~~
>
> __clk_lookup is an internal API for the clk providers.
>
> In particular, relying on undocumented internal names of the
> clk provider in a device driver is inappropriate.

What compiler are you using? I didn't get any warnings. Otherwise I
would have known right away that something isn't right.

>> +static const struct of_device_id brcmstb_cpufreq_match[] = {
>> + { .compatible = BRCMSTB_DT_CPU_CLK_CTRL },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match);
>
> This is a simple typo, also causing the build to fail:
>
> FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct 
> platform_device_id)=24 is not a modulo of the size of section 
> __mod_platform___device_table=392.

What is the typo, if I may ask. Again strange, since the build doesn't
fail for me. What was the configuration you used?

> Arnd

Re: [PATCH v3] cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs

2016-11-28 Thread Markus Mayer 
On 28 November 2016 at 02:14, Arnd Bergmann  wrote:
> On Tuesday, November 22, 2016 1:32:45 PM CET Markus Mayer wrote:
>> From: Markus Mayer 
>>
>> This CPUfreq driver provides basic frequency scaling for older Broadcom
>> STB SoCs that do not use AVS firmware with DVFS support. There is no
>> support for voltage scaling.
>>
>> Signed-off-by: Markus Mayer 
>> Acked-by: Viresh Kumar 
>
> This causes multiple build errors in linux-next, please fix asap or
> drop the patch again. My feeling is that it's probably too late to
> fix it for v4.10, but that's up to Viresh and Rafael of course.
>
>> +#define BRCMSTB_CPUFREQ_PREFIX   "brcmstb"
>> +#define BRCMSTB_CPUFREQ_NAME BRCMSTB_CPUFREQ_PREFIX "-cpufreq"
>> +
>> +/* We search for these compatible strings. */
>> +#define BRCMSTB_DT_CPU_CLK_CTRL  "brcm,brcmstb-cpu-clk-div"
>> +#define BRCMSTB_DT_MEMC_DDR  "brcm,brcmstb-memc-ddr"
>> +#define BRCM_AVS_CPU_DATA"brcm,avs-cpu-data-mem"
>> +
>> +/* We also need a few clocks in device tree. These are node names. */
>> +#define BRCMSTB_CLK_MDIV_CH0 "cpu_mdiv_ch0"
>> +#define BRCMSTB_CLK_NDIV_INT "cpu_ndiv_int"
>> +#define BRCMSTB_CLK_SW_SCB   "sw_scb"
>
> Not critical but the use of those macros obfuscates the DT interfaces
> here and made it harder to analyse what was going on.
>
> Also, a couple of them are lacking a DT binding.
>
>> +static int get_frequencies(const struct cpufreq_policy *policy,
>> +unsigned int *vco_freq, unsigned int *cpu_freq,
>> +unsigned int *scb_freq)
>> +{
>> + struct clk *cpu_ndiv_int, *sw_scb;
>> +
>> + cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
>> + if (!cpu_ndiv_int)
>> + return -ENODEV;
>> +
>> + sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB);
>> + if (!sw_scb)
>> + return -ENODEV;
>> +
>> + /* return frequencies in kHz */
>> + *vco_freq = clk_get_rate(cpu_ndiv_int) / 1000;
>> + *cpu_freq = clk_get_rate(policy->clk) / 1000;
>> + *scb_freq = clk_get_rate(sw_scb) / 1000;
>> +
>> + return 0;
>> +}
>
> You really can't do this:
>
> ../drivers/cpufreq/brcmstb-cpufreq.c: In function 'get_frequencies':
> ../drivers/cpufreq/brcmstb-cpufreq.c:71:17: error: implicit declaration of 
> function '__clk_lookup';did you mean 'key_lookup'? 
> [-Werror=implicit-function-declaration]
>   cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
>  ^~~~
>
> __clk_lookup is an internal API for the clk providers.
>
> In particular, relying on undocumented internal names of the
> clk provider in a device driver is inappropriate.

What compiler are you using? I didn't get any warnings. Otherwise I
would have known right away that something isn't right.

>> +static const struct of_device_id brcmstb_cpufreq_match[] = {
>> + { .compatible = BRCMSTB_DT_CPU_CLK_CTRL },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match);
>
> This is a simple typo, also causing the build to fail:
>
> FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct 
> platform_device_id)=24 is not a modulo of the size of section 
> __mod_platform___device_table=392.

What is the typo, if I may ask. Again strange, since the build doesn't
fail for me. What was the configuration you used?

> Arnd

Re: [PATCH v3] cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs

2016-11-28 Thread Rafael J. Wysocki 
On Mon, Nov 28, 2016 at 11:14 AM, Arnd Bergmann  wrote:
> On Tuesday, November 22, 2016 1:32:45 PM CET Markus Mayer wrote:
>> From: Markus Mayer 
>>
>> This CPUfreq driver provides basic frequency scaling for older Broadcom
>> STB SoCs that do not use AVS firmware with DVFS support. There is no
>> support for voltage scaling.
>>
>> Signed-off-by: Markus Mayer 
>> Acked-by: Viresh Kumar 
>
> This causes multiple build errors in linux-next, please fix asap or
> drop the patch again. My feeling is that it's probably too late to
> fix it for v4.10, but that's up to Viresh and Rafael of course.
>
>> +#define BRCMSTB_CPUFREQ_PREFIX   "brcmstb"
>> +#define BRCMSTB_CPUFREQ_NAME BRCMSTB_CPUFREQ_PREFIX "-cpufreq"
>> +
>> +/* We search for these compatible strings. */
>> +#define BRCMSTB_DT_CPU_CLK_CTRL  "brcm,brcmstb-cpu-clk-div"
>> +#define BRCMSTB_DT_MEMC_DDR  "brcm,brcmstb-memc-ddr"
>> +#define BRCM_AVS_CPU_DATA"brcm,avs-cpu-data-mem"
>> +
>> +/* We also need a few clocks in device tree. These are node names. */
>> +#define BRCMSTB_CLK_MDIV_CH0 "cpu_mdiv_ch0"
>> +#define BRCMSTB_CLK_NDIV_INT "cpu_ndiv_int"
>> +#define BRCMSTB_CLK_SW_SCB   "sw_scb"
>
> Not critical but the use of those macros obfuscates the DT interfaces
> here and made it harder to analyse what was going on.
>
> Also, a couple of them are lacking a DT binding.
>
>> +static int get_frequencies(const struct cpufreq_policy *policy,
>> +unsigned int *vco_freq, unsigned int *cpu_freq,
>> +unsigned int *scb_freq)
>> +{
>> + struct clk *cpu_ndiv_int, *sw_scb;
>> +
>> + cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
>> + if (!cpu_ndiv_int)
>> + return -ENODEV;
>> +
>> + sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB);
>> + if (!sw_scb)
>> + return -ENODEV;
>> +
>> + /* return frequencies in kHz */
>> + *vco_freq = clk_get_rate(cpu_ndiv_int) / 1000;
>> + *cpu_freq = clk_get_rate(policy->clk) / 1000;
>> + *scb_freq = clk_get_rate(sw_scb) / 1000;
>> +
>> + return 0;
>> +}
>
> You really can't do this:
>
> ../drivers/cpufreq/brcmstb-cpufreq.c: In function 'get_frequencies':
> ../drivers/cpufreq/brcmstb-cpufreq.c:71:17: error: implicit declaration of 
> function '__clk_lookup';did you mean 'key_lookup'? 
> [-Werror=implicit-function-declaration]
>   cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
>  ^~~~
>
> __clk_lookup is an internal API for the clk providers.
>
> In particular, relying on undocumented internal names of the
> clk provider in a device driver is inappropriate.
>
>> +static const struct of_device_id brcmstb_cpufreq_match[] = {
>> + { .compatible = BRCMSTB_DT_CPU_CLK_CTRL },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match);
>
> This is a simple typo, also causing the build to fail:
>
> FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct 
> platform_device_id)=24 is not a modulo of the size of section 
> __mod_platform___device_table=392.
>

I've dropped the patch.

Markus, please fix the problems pointed out by Arnd and resend.

Thanks,
Rafael

Re: [PATCH v3] cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs

2016-11-28 Thread Rafael J. Wysocki 
On Mon, Nov 28, 2016 at 11:14 AM, Arnd Bergmann  wrote:
> On Tuesday, November 22, 2016 1:32:45 PM CET Markus Mayer wrote:
>> From: Markus Mayer 
>>
>> This CPUfreq driver provides basic frequency scaling for older Broadcom
>> STB SoCs that do not use AVS firmware with DVFS support. There is no
>> support for voltage scaling.
>>
>> Signed-off-by: Markus Mayer 
>> Acked-by: Viresh Kumar 
>
> This causes multiple build errors in linux-next, please fix asap or
> drop the patch again. My feeling is that it's probably too late to
> fix it for v4.10, but that's up to Viresh and Rafael of course.
>
>> +#define BRCMSTB_CPUFREQ_PREFIX   "brcmstb"
>> +#define BRCMSTB_CPUFREQ_NAME BRCMSTB_CPUFREQ_PREFIX "-cpufreq"
>> +
>> +/* We search for these compatible strings. */
>> +#define BRCMSTB_DT_CPU_CLK_CTRL  "brcm,brcmstb-cpu-clk-div"
>> +#define BRCMSTB_DT_MEMC_DDR  "brcm,brcmstb-memc-ddr"
>> +#define BRCM_AVS_CPU_DATA"brcm,avs-cpu-data-mem"
>> +
>> +/* We also need a few clocks in device tree. These are node names. */
>> +#define BRCMSTB_CLK_MDIV_CH0 "cpu_mdiv_ch0"
>> +#define BRCMSTB_CLK_NDIV_INT "cpu_ndiv_int"
>> +#define BRCMSTB_CLK_SW_SCB   "sw_scb"
>
> Not critical but the use of those macros obfuscates the DT interfaces
> here and made it harder to analyse what was going on.
>
> Also, a couple of them are lacking a DT binding.
>
>> +static int get_frequencies(const struct cpufreq_policy *policy,
>> +unsigned int *vco_freq, unsigned int *cpu_freq,
>> +unsigned int *scb_freq)
>> +{
>> + struct clk *cpu_ndiv_int, *sw_scb;
>> +
>> + cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
>> + if (!cpu_ndiv_int)
>> + return -ENODEV;
>> +
>> + sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB);
>> + if (!sw_scb)
>> + return -ENODEV;
>> +
>> + /* return frequencies in kHz */
>> + *vco_freq = clk_get_rate(cpu_ndiv_int) / 1000;
>> + *cpu_freq = clk_get_rate(policy->clk) / 1000;
>> + *scb_freq = clk_get_rate(sw_scb) / 1000;
>> +
>> + return 0;
>> +}
>
> You really can't do this:
>
> ../drivers/cpufreq/brcmstb-cpufreq.c: In function 'get_frequencies':
> ../drivers/cpufreq/brcmstb-cpufreq.c:71:17: error: implicit declaration of 
> function '__clk_lookup';did you mean 'key_lookup'? 
> [-Werror=implicit-function-declaration]
>   cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
>  ^~~~
>
> __clk_lookup is an internal API for the clk providers.
>
> In particular, relying on undocumented internal names of the
> clk provider in a device driver is inappropriate.
>
>> +static const struct of_device_id brcmstb_cpufreq_match[] = {
>> + { .compatible = BRCMSTB_DT_CPU_CLK_CTRL },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match);
>
> This is a simple typo, also causing the build to fail:
>
> FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct 
> platform_device_id)=24 is not a modulo of the size of section 
> __mod_platform___device_table=392.
>

I've dropped the patch.

Markus, please fix the problems pointed out by Arnd and resend.

Thanks,
Rafael

fwd, Re: [PATCH v3] cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs

2016-11-28 Thread Arnd Bergmann 
[resending my mail, this time with devicetree, linux-clk, and linux-arm-kernel
on cc]

On Tuesday, November 22, 2016 1:32:45 PM CET Markus Mayer wrote:
> From: Markus Mayer 
> 
> This CPUfreq driver provides basic frequency scaling for older Broadcom
> STB SoCs that do not use AVS firmware with DVFS support. There is no
> support for voltage scaling.
> 
> Signed-off-by: Markus Mayer 
> Acked-by: Viresh Kumar 

This causes multiple build errors in linux-next, please fix asap or
drop the patch again. My feeling is that it's probably too late to
fix it for v4.10, but that's up to Viresh and Rafael of course.

> +#define BRCMSTB_CPUFREQ_PREFIX   "brcmstb"
> +#define BRCMSTB_CPUFREQ_NAME BRCMSTB_CPUFREQ_PREFIX "-cpufreq"
> +
> +/* We search for these compatible strings. */
> +#define BRCMSTB_DT_CPU_CLK_CTRL  "brcm,brcmstb-cpu-clk-div"
> +#define BRCMSTB_DT_MEMC_DDR  "brcm,brcmstb-memc-ddr"
> +#define BRCM_AVS_CPU_DATA"brcm,avs-cpu-data-mem"
> +
> +/* We also need a few clocks in device tree. These are node names. */
> +#define BRCMSTB_CLK_MDIV_CH0 "cpu_mdiv_ch0"
> +#define BRCMSTB_CLK_NDIV_INT "cpu_ndiv_int"
> +#define BRCMSTB_CLK_SW_SCB   "sw_scb"

Not critical but the use of those macros obfuscates the DT interfaces
here and made it harder to analyse what was going on.

Also, a couple of them are lacking a DT binding.

> +static int get_frequencies(const struct cpufreq_policy *policy,
> +unsigned int *vco_freq, unsigned int *cpu_freq,
> +unsigned int *scb_freq)
> +{
> + struct clk *cpu_ndiv_int, *sw_scb;
> +
> + cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
> + if (!cpu_ndiv_int)
> + return -ENODEV;
> +
> + sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB);
> + if (!sw_scb)
> + return -ENODEV;
> +
> + /* return frequencies in kHz */
> + *vco_freq = clk_get_rate(cpu_ndiv_int) / 1000;
> + *cpu_freq = clk_get_rate(policy->clk) / 1000;
> + *scb_freq = clk_get_rate(sw_scb) / 1000;
> +
> + return 0;
> +}

You really can't do this: 

../drivers/cpufreq/brcmstb-cpufreq.c: In function 'get_frequencies':
../drivers/cpufreq/brcmstb-cpufreq.c:71:17: error: implicit declaration of 
function '__clk_lookup';did you mean 'key_lookup'? 
[-Werror=implicit-function-declaration]
  cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
 ^~~~

__clk_lookup is an internal API for the clk providers.

In particular, relying on undocumented internal names of the
clk provider in a device driver is inappropriate.

> +static const struct of_device_id brcmstb_cpufreq_match[] = {
> + { .compatible = BRCMSTB_DT_CPU_CLK_CTRL },
> + { }
> +};
> +MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match);

This is a simple typo, also causing the build to fail:

FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct 
platform_device_id)=24 is not a modulo of the size of section 
__mod_platform___device_table=392.

Arnd

fwd, Re: [PATCH v3] cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs

2016-11-28 Thread Arnd Bergmann 
[resending my mail, this time with devicetree, linux-clk, and linux-arm-kernel
on cc]

On Tuesday, November 22, 2016 1:32:45 PM CET Markus Mayer wrote:
> From: Markus Mayer 
> 
> This CPUfreq driver provides basic frequency scaling for older Broadcom
> STB SoCs that do not use AVS firmware with DVFS support. There is no
> support for voltage scaling.
> 
> Signed-off-by: Markus Mayer 
> Acked-by: Viresh Kumar 

This causes multiple build errors in linux-next, please fix asap or
drop the patch again. My feeling is that it's probably too late to
fix it for v4.10, but that's up to Viresh and Rafael of course.

> +#define BRCMSTB_CPUFREQ_PREFIX   "brcmstb"
> +#define BRCMSTB_CPUFREQ_NAME BRCMSTB_CPUFREQ_PREFIX "-cpufreq"
> +
> +/* We search for these compatible strings. */
> +#define BRCMSTB_DT_CPU_CLK_CTRL  "brcm,brcmstb-cpu-clk-div"
> +#define BRCMSTB_DT_MEMC_DDR  "brcm,brcmstb-memc-ddr"
> +#define BRCM_AVS_CPU_DATA"brcm,avs-cpu-data-mem"
> +
> +/* We also need a few clocks in device tree. These are node names. */
> +#define BRCMSTB_CLK_MDIV_CH0 "cpu_mdiv_ch0"
> +#define BRCMSTB_CLK_NDIV_INT "cpu_ndiv_int"
> +#define BRCMSTB_CLK_SW_SCB   "sw_scb"

Not critical but the use of those macros obfuscates the DT interfaces
here and made it harder to analyse what was going on.

Also, a couple of them are lacking a DT binding.

> +static int get_frequencies(const struct cpufreq_policy *policy,
> +unsigned int *vco_freq, unsigned int *cpu_freq,
> +unsigned int *scb_freq)
> +{
> + struct clk *cpu_ndiv_int, *sw_scb;
> +
> + cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
> + if (!cpu_ndiv_int)
> + return -ENODEV;
> +
> + sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB);
> + if (!sw_scb)
> + return -ENODEV;
> +
> + /* return frequencies in kHz */
> + *vco_freq = clk_get_rate(cpu_ndiv_int) / 1000;
> + *cpu_freq = clk_get_rate(policy->clk) / 1000;
> + *scb_freq = clk_get_rate(sw_scb) / 1000;
> +
> + return 0;
> +}

You really can't do this: 

../drivers/cpufreq/brcmstb-cpufreq.c: In function 'get_frequencies':
../drivers/cpufreq/brcmstb-cpufreq.c:71:17: error: implicit declaration of 
function '__clk_lookup';did you mean 'key_lookup'? 
[-Werror=implicit-function-declaration]
  cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
 ^~~~

__clk_lookup is an internal API for the clk providers.

In particular, relying on undocumented internal names of the
clk provider in a device driver is inappropriate.

> +static const struct of_device_id brcmstb_cpufreq_match[] = {
> + { .compatible = BRCMSTB_DT_CPU_CLK_CTRL },
> + { }
> +};
> +MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match);

This is a simple typo, also causing the build to fail:

FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct 
platform_device_id)=24 is not a modulo of the size of section 
__mod_platform___device_table=392.

Arnd

Re: [PATCH v3] cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs

2016-11-28 Thread Arnd Bergmann 
On Tuesday, November 22, 2016 1:32:45 PM CET Markus Mayer wrote:
> From: Markus Mayer 
> 
> This CPUfreq driver provides basic frequency scaling for older Broadcom
> STB SoCs that do not use AVS firmware with DVFS support. There is no
> support for voltage scaling.
> 
> Signed-off-by: Markus Mayer 
> Acked-by: Viresh Kumar 

This causes multiple build errors in linux-next, please fix asap or
drop the patch again. My feeling is that it's probably too late to
fix it for v4.10, but that's up to Viresh and Rafael of course.

> +#define BRCMSTB_CPUFREQ_PREFIX   "brcmstb"
> +#define BRCMSTB_CPUFREQ_NAME BRCMSTB_CPUFREQ_PREFIX "-cpufreq"
> +
> +/* We search for these compatible strings. */
> +#define BRCMSTB_DT_CPU_CLK_CTRL  "brcm,brcmstb-cpu-clk-div"
> +#define BRCMSTB_DT_MEMC_DDR  "brcm,brcmstb-memc-ddr"
> +#define BRCM_AVS_CPU_DATA"brcm,avs-cpu-data-mem"
> +
> +/* We also need a few clocks in device tree. These are node names. */
> +#define BRCMSTB_CLK_MDIV_CH0 "cpu_mdiv_ch0"
> +#define BRCMSTB_CLK_NDIV_INT "cpu_ndiv_int"
> +#define BRCMSTB_CLK_SW_SCB   "sw_scb"

Not critical but the use of those macros obfuscates the DT interfaces
here and made it harder to analyse what was going on.

Also, a couple of them are lacking a DT binding.

> +static int get_frequencies(const struct cpufreq_policy *policy,
> +unsigned int *vco_freq, unsigned int *cpu_freq,
> +unsigned int *scb_freq)
> +{
> + struct clk *cpu_ndiv_int, *sw_scb;
> +
> + cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
> + if (!cpu_ndiv_int)
> + return -ENODEV;
> +
> + sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB);
> + if (!sw_scb)
> + return -ENODEV;
> +
> + /* return frequencies in kHz */
> + *vco_freq = clk_get_rate(cpu_ndiv_int) / 1000;
> + *cpu_freq = clk_get_rate(policy->clk) / 1000;
> + *scb_freq = clk_get_rate(sw_scb) / 1000;
> +
> + return 0;
> +}

You really can't do this: 

../drivers/cpufreq/brcmstb-cpufreq.c: In function 'get_frequencies':
../drivers/cpufreq/brcmstb-cpufreq.c:71:17: error: implicit declaration of 
function '__clk_lookup';did you mean 'key_lookup'? 
[-Werror=implicit-function-declaration]
  cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
 ^~~~

__clk_lookup is an internal API for the clk providers.

In particular, relying on undocumented internal names of the
clk provider in a device driver is inappropriate.

> +static const struct of_device_id brcmstb_cpufreq_match[] = {
> + { .compatible = BRCMSTB_DT_CPU_CLK_CTRL },
> + { }
> +};
> +MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match);

This is a simple typo, also causing the build to fail:

FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct 
platform_device_id)=24 is not a modulo of the size of section 
__mod_platform___device_table=392.

Arnd

Re: [PATCH v3] cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs

2016-11-28 Thread Arnd Bergmann 
On Tuesday, November 22, 2016 1:32:45 PM CET Markus Mayer wrote:
> From: Markus Mayer 
> 
> This CPUfreq driver provides basic frequency scaling for older Broadcom
> STB SoCs that do not use AVS firmware with DVFS support. There is no
> support for voltage scaling.
> 
> Signed-off-by: Markus Mayer 
> Acked-by: Viresh Kumar 

This causes multiple build errors in linux-next, please fix asap or
drop the patch again. My feeling is that it's probably too late to
fix it for v4.10, but that's up to Viresh and Rafael of course.

> +#define BRCMSTB_CPUFREQ_PREFIX   "brcmstb"
> +#define BRCMSTB_CPUFREQ_NAME BRCMSTB_CPUFREQ_PREFIX "-cpufreq"
> +
> +/* We search for these compatible strings. */
> +#define BRCMSTB_DT_CPU_CLK_CTRL  "brcm,brcmstb-cpu-clk-div"
> +#define BRCMSTB_DT_MEMC_DDR  "brcm,brcmstb-memc-ddr"
> +#define BRCM_AVS_CPU_DATA"brcm,avs-cpu-data-mem"
> +
> +/* We also need a few clocks in device tree. These are node names. */
> +#define BRCMSTB_CLK_MDIV_CH0 "cpu_mdiv_ch0"
> +#define BRCMSTB_CLK_NDIV_INT "cpu_ndiv_int"
> +#define BRCMSTB_CLK_SW_SCB   "sw_scb"

Not critical but the use of those macros obfuscates the DT interfaces
here and made it harder to analyse what was going on.

Also, a couple of them are lacking a DT binding.

> +static int get_frequencies(const struct cpufreq_policy *policy,
> +unsigned int *vco_freq, unsigned int *cpu_freq,
> +unsigned int *scb_freq)
> +{
> + struct clk *cpu_ndiv_int, *sw_scb;
> +
> + cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
> + if (!cpu_ndiv_int)
> + return -ENODEV;
> +
> + sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB);
> + if (!sw_scb)
> + return -ENODEV;
> +
> + /* return frequencies in kHz */
> + *vco_freq = clk_get_rate(cpu_ndiv_int) / 1000;
> + *cpu_freq = clk_get_rate(policy->clk) / 1000;
> + *scb_freq = clk_get_rate(sw_scb) / 1000;
> +
> + return 0;
> +}

You really can't do this: 

../drivers/cpufreq/brcmstb-cpufreq.c: In function 'get_frequencies':
../drivers/cpufreq/brcmstb-cpufreq.c:71:17: error: implicit declaration of 
function '__clk_lookup';did you mean 'key_lookup'? 
[-Werror=implicit-function-declaration]
  cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
 ^~~~

__clk_lookup is an internal API for the clk providers.

In particular, relying on undocumented internal names of the
clk provider in a device driver is inappropriate.

> +static const struct of_device_id brcmstb_cpufreq_match[] = {
> + { .compatible = BRCMSTB_DT_CPU_CLK_CTRL },
> + { }
> +};
> +MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match);

This is a simple typo, also causing the build to fail:

FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct 
platform_device_id)=24 is not a modulo of the size of section 
__mod_platform___device_table=392.

Arnd

Re: [PATCH v3] cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs

2016-11-28 Thread Andreas Ziegler 
Forgot to CC linux-kernel and linux-pm in my first message.

On 11/28/2016 10:28, Andreas Ziegler wrote:
> Hi Markus,
> 
> your patch "cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB
> SoCs" showed up in linux-next today, and I noticed a small error in it.
> 
> In drivers/cpufreq/brcmstb-cpufreq.c, line 214 there is an IS_ENABLED()
> statement with CONFIG_ARM_BRCM_AVS_CPUFREQ. This symbol, however, does not 
> exist
> - making the if condition always evaluate to false. Did you mean to use
> CONFIG_ARM_BRCMSTB_AVS_CPUFREQ (note the STB after BRCM)?
> 
> I noticed this mistake by running 'scripts/checkkconfigsymbols -f --force -f
> next-20161124..next-20161128', which essentialy diffs the last two
> linux-next releases and looks for undefined/unknown Kconfig symbols.
> You can also run the script on single commits with -c to test them.
> 
> Best regards,
> 
> Andreas
>

Re: [PATCH v3] cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs

2016-11-28 Thread Andreas Ziegler 
Forgot to CC linux-kernel and linux-pm in my first message.

On 11/28/2016 10:28, Andreas Ziegler wrote:
> Hi Markus,
> 
> your patch "cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB
> SoCs" showed up in linux-next today, and I noticed a small error in it.
> 
> In drivers/cpufreq/brcmstb-cpufreq.c, line 214 there is an IS_ENABLED()
> statement with CONFIG_ARM_BRCM_AVS_CPUFREQ. This symbol, however, does not 
> exist
> - making the if condition always evaluate to false. Did you mean to use
> CONFIG_ARM_BRCMSTB_AVS_CPUFREQ (note the STB after BRCM)?
> 
> I noticed this mistake by running 'scripts/checkkconfigsymbols -f --force -f
> next-20161124..next-20161128', which essentialy diffs the last two
> linux-next releases and looks for undefined/unknown Kconfig symbols.
> You can also run the script on single commits with -c to test them.
> 
> Best regards,
> 
> Andreas
>

[PATCH v3] cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs

2016-11-22 Thread Markus Mayer 
From: Markus Mayer 

This CPUfreq driver provides basic frequency scaling for older Broadcom
STB SoCs that do not use AVS firmware with DVFS support. There is no
support for voltage scaling.

Signed-off-by: Markus Mayer 
Acked-by: Viresh Kumar 
---

This patch is based on Rafael's linux-next.

Changes since v2:
- fixed naming-inconsistency for function brcmstb_cpufreq_init()
  (it was called brcmstb_cpu_init() before)

Changes since v1:
- removed brcmstb_cpufreq_get(), using cpufreq_generic_get() instead
- replaced calls to cpufreq_table_validate_and_show() and
  cpumask_setall() with a call to cpufreq_generic_init()
- removed code that would set policy->cur, leaving it up to the
  framework to do so
- simplified show_brcmstb_safe_freq(), re-using already existing data

 drivers/cpufreq/Kconfig.arm   |  12 ++
 drivers/cpufreq/Makefile  |   1 +
 drivers/cpufreq/brcmstb-cpufreq.c | 381 ++
 3 files changed, 394 insertions(+)
 create mode 100644 drivers/cpufreq/brcmstb-cpufreq.c

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 920c469..36422af 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -33,6 +33,18 @@ config ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
 
  If in doubt, say N.
 
+config ARM_BRCMSTB_CPUFREQ
+   tristate "Broadcom STB CPUfreq driver"
+   depends on ARCH_BRCMSTB || COMPILE_TEST
+   default y
+   help
+ Some Broadcom SoCs offer multiple operating frequencies that CPUfreq
+ can take advantage of to improve energy efficiency.
+
+ Say Y, if you have a supported Broadcom SoC. If your Broadcom SoC
+ has AVS firmware with support for frequency and voltage scaling,
+ say N here and enable ARM_BRCMSTB_AVS_CPUFREQ instead.
+
 config ARM_DT_BL_CPUFREQ
tristate "Generic probing via DT for ARM big LITTLE CPUfreq driver"
depends on ARM_BIG_LITTLE_CPUFREQ && OF
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 1e46c39..23700aa 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_ARM_BIG_LITTLE_CPUFREQ)  += arm_big_little.o
 obj-$(CONFIG_ARM_DT_BL_CPUFREQ)+= arm_big_little_dt.o
 
 obj-$(CONFIG_ARM_BRCMSTB_AVS_CPUFREQ)  += brcmstb-avs-cpufreq.o
+obj-$(CONFIG_ARM_BRCMSTB_CPUFREQ)  += brcmstb-cpufreq.o
 obj-$(CONFIG_ARCH_DAVINCI) += davinci-cpufreq.o
 obj-$(CONFIG_UX500_SOC_DB8500) += dbx500-cpufreq.o
 obj-$(CONFIG_ARM_EXYNOS5440_CPUFREQ)   += exynos5440-cpufreq.o
diff --git a/drivers/cpufreq/brcmstb-cpufreq.c 
b/drivers/cpufreq/brcmstb-cpufreq.c
new file mode 100644
index 000..470b073
--- /dev/null
+++ b/drivers/cpufreq/brcmstb-cpufreq.c
@@ -0,0 +1,381 @@
+/*
+ * CPU frequency scaling for Broadcom set top box SoCs
+ *
+ * Copyright (c) 2016 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define BRCMSTB_CPUFREQ_PREFIX "brcmstb"
+#define BRCMSTB_CPUFREQ_NAME   BRCMSTB_CPUFREQ_PREFIX "-cpufreq"
+
+/* We search for these compatible strings. */
+#define BRCMSTB_DT_CPU_CLK_CTRL"brcm,brcmstb-cpu-clk-div"
+#define BRCMSTB_DT_MEMC_DDR"brcm,brcmstb-memc-ddr"
+#define BRCM_AVS_CPU_DATA  "brcm,avs-cpu-data-mem"
+
+/* We also need a few clocks in device tree. These are node names. */
+#define BRCMSTB_CLK_MDIV_CH0   "cpu_mdiv_ch0"
+#define BRCMSTB_CLK_NDIV_INT   "cpu_ndiv_int"
+#define BRCMSTB_CLK_SW_SCB "sw_scb"
+
+#define BRCMSTB_TBL_SAFE_MODE  BIT(0)
+#define BRCMSTB_REG_SAFE_MODE  BIT(4)
+
+#define TRANSITION_LATENCY (25 * 1000) /* 25 us */
+
+/* This is as low as we'll go in the frequency table. */
+#define MIN_CPU_FREQ   (100 * 1000)/* in kHz */
+
+struct private_data {
+   void __iomem *cpu_clk_ctrl_reg;
+   struct device *dev;
+};
+
+/* Count the active memory controllers in the system. */
+static int count_memory_controllers(void)
+{
+   struct device_node *np = NULL;
+   int i = 0;
+
+   do {
+   np = of_find_compatible_node(np, NULL, BRCMSTB_DT_MEMC_DDR);
+   if (of_device_is_available(np))
+   i++;
+   of_node_put(np);
+   } while (np);
+
+   return i;
+}
+
+static int get_frequencies(const struct cpufreq_policy *policy,
+  unsigned int *vco_freq, unsigned int *cpu_freq,
+  unsigned

[PATCH v3] cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs

2016-11-22 Thread Markus Mayer 
From: Markus Mayer 

This CPUfreq driver provides basic frequency scaling for older Broadcom
STB SoCs that do not use AVS firmware with DVFS support. There is no
support for voltage scaling.

Signed-off-by: Markus Mayer 
Acked-by: Viresh Kumar 
---

This patch is based on Rafael's linux-next.

Changes since v2:
- fixed naming-inconsistency for function brcmstb_cpufreq_init()
  (it was called brcmstb_cpu_init() before)

Changes since v1:
- removed brcmstb_cpufreq_get(), using cpufreq_generic_get() instead
- replaced calls to cpufreq_table_validate_and_show() and
  cpumask_setall() with a call to cpufreq_generic_init()
- removed code that would set policy->cur, leaving it up to the
  framework to do so
- simplified show_brcmstb_safe_freq(), re-using already existing data

 drivers/cpufreq/Kconfig.arm   |  12 ++
 drivers/cpufreq/Makefile  |   1 +
 drivers/cpufreq/brcmstb-cpufreq.c | 381 ++
 3 files changed, 394 insertions(+)
 create mode 100644 drivers/cpufreq/brcmstb-cpufreq.c

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 920c469..36422af 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -33,6 +33,18 @@ config ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
 
  If in doubt, say N.
 
+config ARM_BRCMSTB_CPUFREQ
+   tristate "Broadcom STB CPUfreq driver"
+   depends on ARCH_BRCMSTB || COMPILE_TEST
+   default y
+   help
+ Some Broadcom SoCs offer multiple operating frequencies that CPUfreq
+ can take advantage of to improve energy efficiency.
+
+ Say Y, if you have a supported Broadcom SoC. If your Broadcom SoC
+ has AVS firmware with support for frequency and voltage scaling,
+ say N here and enable ARM_BRCMSTB_AVS_CPUFREQ instead.
+
 config ARM_DT_BL_CPUFREQ
tristate "Generic probing via DT for ARM big LITTLE CPUfreq driver"
depends on ARM_BIG_LITTLE_CPUFREQ && OF
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 1e46c39..23700aa 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_ARM_BIG_LITTLE_CPUFREQ)  += arm_big_little.o
 obj-$(CONFIG_ARM_DT_BL_CPUFREQ)+= arm_big_little_dt.o
 
 obj-$(CONFIG_ARM_BRCMSTB_AVS_CPUFREQ)  += brcmstb-avs-cpufreq.o
+obj-$(CONFIG_ARM_BRCMSTB_CPUFREQ)  += brcmstb-cpufreq.o
 obj-$(CONFIG_ARCH_DAVINCI) += davinci-cpufreq.o
 obj-$(CONFIG_UX500_SOC_DB8500) += dbx500-cpufreq.o
 obj-$(CONFIG_ARM_EXYNOS5440_CPUFREQ)   += exynos5440-cpufreq.o
diff --git a/drivers/cpufreq/brcmstb-cpufreq.c 
b/drivers/cpufreq/brcmstb-cpufreq.c
new file mode 100644
index 000..470b073
--- /dev/null
+++ b/drivers/cpufreq/brcmstb-cpufreq.c
@@ -0,0 +1,381 @@
+/*
+ * CPU frequency scaling for Broadcom set top box SoCs
+ *
+ * Copyright (c) 2016 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#define BRCMSTB_CPUFREQ_PREFIX "brcmstb"
+#define BRCMSTB_CPUFREQ_NAME   BRCMSTB_CPUFREQ_PREFIX "-cpufreq"
+
+/* We search for these compatible strings. */
+#define BRCMSTB_DT_CPU_CLK_CTRL"brcm,brcmstb-cpu-clk-div"
+#define BRCMSTB_DT_MEMC_DDR"brcm,brcmstb-memc-ddr"
+#define BRCM_AVS_CPU_DATA  "brcm,avs-cpu-data-mem"
+
+/* We also need a few clocks in device tree. These are node names. */
+#define BRCMSTB_CLK_MDIV_CH0   "cpu_mdiv_ch0"
+#define BRCMSTB_CLK_NDIV_INT   "cpu_ndiv_int"
+#define BRCMSTB_CLK_SW_SCB "sw_scb"
+
+#define BRCMSTB_TBL_SAFE_MODE  BIT(0)
+#define BRCMSTB_REG_SAFE_MODE  BIT(4)
+
+#define TRANSITION_LATENCY (25 * 1000) /* 25 us */
+
+/* This is as low as we'll go in the frequency table. */
+#define MIN_CPU_FREQ   (100 * 1000)/* in kHz */
+
+struct private_data {
+   void __iomem *cpu_clk_ctrl_reg;
+   struct device *dev;
+};
+
+/* Count the active memory controllers in the system. */
+static int count_memory_controllers(void)
+{
+   struct device_node *np = NULL;
+   int i = 0;
+
+   do {
+   np = of_find_compatible_node(np, NULL, BRCMSTB_DT_MEMC_DDR);
+   if (of_device_is_available(np))
+   i++;
+   of_node_put(np);
+   } while (np);
+
+   return i;
+}
+
+static int get_frequencies(const struct cpufreq_policy *policy,
+  unsigned int *vco_freq, unsigned int *cpu_freq,
+  unsigned int *scb_freq)
+{
+   struct clk *cpu_ndiv_int, *sw_scb;
+
+