diff mbox series

[v2,4/7] cpufreq: add driver for Raspbery Pi

Message ID 20190606142255.29454-5-nsaenzjulienne@suse.de (mailing list archive)
State Awaiting Upstream, archived
Headers show
Series cpufreq support for Raspberry Pi | expand

Commit Message

Nicolas Saenz Julienne June 6, 2019, 2:22 p.m. UTC
Raspberry Pi's firmware offers and interface though which update it's
performance requirements. It allows us to request for specific runtime
frequencies, which the firmware might or might not respect, depending on
the firmware configuration and thermals.

As the maximum and minimum frequencies are configurable in the firmware
there is no way to know in advance their values. So the Raspberry Pi
cpufreq driver queries them, builds an opp frequency table to then
launch cpufreq-dt.

Also, as the firmware interface might be configured as a module, making
the cpu clock unavailable during init, this implements a full fledged
driver, as opposed to most drivers registering cpufreq-dt, which only
make use of an init routine.

Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
Acked-by: Eric Anholt <eric@anholt.net>

---

Changes since v1:
  - Remove compatible checks
  - Add module support, now full fledged driver
  - Use NULL in clk_get()

 drivers/cpufreq/Kconfig.arm           |   8 +++
 drivers/cpufreq/Makefile              |   1 +
 drivers/cpufreq/raspberrypi-cpufreq.c | 100 ++++++++++++++++++++++++++
 3 files changed, 109 insertions(+)
 create mode 100644 drivers/cpufreq/raspberrypi-cpufreq.c

Comments

Stephen Boyd June 6, 2019, 5:09 p.m. UTC | #1
Quoting Nicolas Saenz Julienne (2019-06-06 07:22:56)
> diff --git a/drivers/cpufreq/raspberrypi-cpufreq.c b/drivers/cpufreq/raspberrypi-cpufreq.c
> new file mode 100644
> index 000000000000..99b59d5a50aa
> --- /dev/null
> +++ b/drivers/cpufreq/raspberrypi-cpufreq.c
[...]
> +
> +/*
> + * Since the driver depends on clk-raspberrypi, which may return EPROBE_DEFER,
> + * all the activity is performed in the probe, which may be defered as well.
> + */
> +static struct platform_driver raspberrypi_cpufreq_driver = {
> +       .driver = {
> +               .name = "raspberrypi-cpufreq",
> +       },
> +       .probe          = raspberrypi_cpufreq_probe,
> +       .remove         = raspberrypi_cpufreq_remove,
> +};
> +module_platform_driver(raspberrypi_cpufreq_driver);

How does this driver probe? Do you have a node in DT named
raspberrypi-cpufreq that matches and probes this? I would think this
would follow the drivers/cpufreq/cpufreq-dt-platdev.c design where it's
an initcall that probes the board compatible string.

Or, if it depends on clk-raspberrypi probing, maybe it could create the
platform device in that drivers probe function.

> +
> +MODULE_AUTHOR("Nicolas Saenz Julienne <nsaenzjulienne@suse.de");
> +MODULE_DESCRIPTION("Raspberry Pi cpufreq driver");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:raspberrypi-cpufreq");

I don't think the module alias is needed anymore.
Nicolas Saenz Julienne June 6, 2019, 5:22 p.m. UTC | #2
Hi Stephen,
Thanks for the review.

On Thu, 2019-06-06 at 10:09 -0700, Stephen Boyd wrote:
> Quoting Nicolas Saenz Julienne (2019-06-06 07:22:56)
> > diff --git a/drivers/cpufreq/raspberrypi-cpufreq.c
> > b/drivers/cpufreq/raspberrypi-cpufreq.c
> > new file mode 100644
> > index 000000000000..99b59d5a50aa
> > --- /dev/null
> > +++ b/drivers/cpufreq/raspberrypi-cpufreq.c
> [...]
> > +
> > +/*
> > + * Since the driver depends on clk-raspberrypi, which may return
> > EPROBE_DEFER,
> > + * all the activity is performed in the probe, which may be defered as
> > well.
> > + */
> > +static struct platform_driver raspberrypi_cpufreq_driver = {
> > +       .driver = {
> > +               .name = "raspberrypi-cpufreq",
> > +       },
> > +       .probe          = raspberrypi_cpufreq_probe,
> > +       .remove         = raspberrypi_cpufreq_remove,
> > +};
> > +module_platform_driver(raspberrypi_cpufreq_driver);
> 
> How does this driver probe? Do you have a node in DT named
> raspberrypi-cpufreq that matches and probes this? I would think this
> would follow the drivers/cpufreq/cpufreq-dt-platdev.c design where it's
> an initcall that probes the board compatible string.
>
> Or, if it depends on clk-raspberrypi probing, maybe it could create the
> platform device in that drivers probe function.

Well you just reviewed that patch :)

> > +
> > +MODULE_AUTHOR("Nicolas Saenz Julienne <nsaenzjulienne@suse.de");
> > +MODULE_DESCRIPTION("Raspberry Pi cpufreq driver");
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:raspberrypi-cpufreq");
> 
> I don't think the module alias is needed anymore.

That's surprising. I remember the driver not being loaded by udev without it.

Regards,
Nicolas
Stephen Boyd June 6, 2019, 5:36 p.m. UTC | #3
Quoting Nicolas Saenz Julienne (2019-06-06 10:22:16)
> Hi Stephen,
> Thanks for the review.
> 
> On Thu, 2019-06-06 at 10:09 -0700, Stephen Boyd wrote:
> > Quoting Nicolas Saenz Julienne (2019-06-06 07:22:56)
> > > diff --git a/drivers/cpufreq/raspberrypi-cpufreq.c
> > > b/drivers/cpufreq/raspberrypi-cpufreq.c
> > > new file mode 100644
> > > index 000000000000..99b59d5a50aa
> > > --- /dev/null
> > > +++ b/drivers/cpufreq/raspberrypi-cpufreq.c
> > [...]
> > > +
> > > +/*
> > > + * Since the driver depends on clk-raspberrypi, which may return
> > > EPROBE_DEFER,
> > > + * all the activity is performed in the probe, which may be defered as
> > > well.
> > > + */
> > > +static struct platform_driver raspberrypi_cpufreq_driver = {
> > > +       .driver = {
> > > +               .name = "raspberrypi-cpufreq",
> > > +       },
> > > +       .probe          = raspberrypi_cpufreq_probe,
> > > +       .remove         = raspberrypi_cpufreq_remove,
> > > +};
> > > +module_platform_driver(raspberrypi_cpufreq_driver);
> > 
> > How does this driver probe? Do you have a node in DT named
> > raspberrypi-cpufreq that matches and probes this? I would think this
> > would follow the drivers/cpufreq/cpufreq-dt-platdev.c design where it's
> > an initcall that probes the board compatible string.
> >
> > Or, if it depends on clk-raspberrypi probing, maybe it could create the
> > platform device in that drivers probe function.
> 
> Well you just reviewed that patch :)

Ok. So what's your plan?

> 
> > > +
> > > +MODULE_AUTHOR("Nicolas Saenz Julienne <nsaenzjulienne@suse.de");
> > > +MODULE_DESCRIPTION("Raspberry Pi cpufreq driver");
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_ALIAS("platform:raspberrypi-cpufreq");
> > 
> > I don't think the module alias is needed anymore.
> 
> That's surprising. I remember the driver not being loaded by udev without it.
> 

Maybe I'm wrong. Could be not needed for DT based platform devices with
an OF table.
Nicolas Saenz Julienne June 6, 2019, 6:10 p.m. UTC | #4
On Thu, 2019-06-06 at 10:36 -0700, Stephen Boyd wrote:
> Quoting Nicolas Saenz Julienne (2019-06-06 10:22:16)
> > Hi Stephen,
> > Thanks for the review.
> > 
> > On Thu, 2019-06-06 at 10:09 -0700, Stephen Boyd wrote:
> > > Quoting Nicolas Saenz Julienne (2019-06-06 07:22:56)
> > > > diff --git a/drivers/cpufreq/raspberrypi-cpufreq.c
> > > > b/drivers/cpufreq/raspberrypi-cpufreq.c
> > > > new file mode 100644
> > > > index 000000000000..99b59d5a50aa
> > > > --- /dev/null
> > > > +++ b/drivers/cpufreq/raspberrypi-cpufreq.c
> > > [...]
> > > > +
> > > > +/*
> > > > + * Since the driver depends on clk-raspberrypi, which may return
> > > > EPROBE_DEFER,
> > > > + * all the activity is performed in the probe, which may be defered as
> > > > well.
> > > > + */
> > > > +static struct platform_driver raspberrypi_cpufreq_driver = {
> > > > +       .driver = {
> > > > +               .name = "raspberrypi-cpufreq",
> > > > +       },
> > > > +       .probe          = raspberrypi_cpufreq_probe,
> > > > +       .remove         = raspberrypi_cpufreq_remove,
> > > > +};
> > > > +module_platform_driver(raspberrypi_cpufreq_driver);
> > > 
> > > How does this driver probe? Do you have a node in DT named
> > > raspberrypi-cpufreq that matches and probes this? I would think this
> > > would follow the drivers/cpufreq/cpufreq-dt-platdev.c design where it's
> > > an initcall that probes the board compatible string.
> > > 
> > > Or, if it depends on clk-raspberrypi probing, maybe it could create the
> > > platform device in that drivers probe function.
> > 
> > Well you just reviewed that patch :)
> 
> Ok. So what's your plan?

So as discussed previously with the RPi mantainers, they preferred for the
platform device for raspberrypi-clk to be created by the firmware interface
driver. IIRC Stefan said it was more flexible and the approach used with RPi's
hwmon driver already. Also, it's not really clear whether this driver really
fits the device tree as it wouldn't be describing hardware.

As far as raspberrypi-cpufreq is concerned the max and min frequencies are
configurable in the firmware. So we can't really integrate cpufreq into the
device tree as we need to create the opp table dynamically. Hence the dedicated
driver. On top of that the CPU might not have a clock during the init process,
as both the firmware interface and raspberrypi-clk can be compiled as modules.
So I decided the simplest solution was to create the raspberrypi-cpufreq
platform device at the end of raspberrypi-clk's probe.

Once raspberrypi-cpufreq is loaded it queries the min/max frequencies,
populates the CPU's opp table and creates an instance of cpufreq-dt. Which
finally can operate, without the need of any dt info, as opp tables are
populated and CPUs have a clock.

I hope this makes it a little more clear :).

> > > > +
> > > > +MODULE_AUTHOR("Nicolas Saenz Julienne <nsaenzjulienne@suse.de");
> > > > +MODULE_DESCRIPTION("Raspberry Pi cpufreq driver");
> > > > +MODULE_LICENSE("GPL");
> > > > +MODULE_ALIAS("platform:raspberrypi-cpufreq");
> > > 
> > > I don't think the module alias is needed anymore.
> > 
> > That's surprising. I remember the driver not being loaded by udev without
> > it.
> > 
> 
> Maybe I'm wrong. Could be not needed for DT based platform devices with
> an OF table.

As explained in the previous paragraph, I'm not using DT.

Regards,
Nicolas
Stephen Boyd June 6, 2019, 6:23 p.m. UTC | #5
Quoting Nicolas Saenz Julienne (2019-06-06 11:10:04)
> On Thu, 2019-06-06 at 10:36 -0700, Stephen Boyd wrote:
> > Quoting Nicolas Saenz Julienne (2019-06-06 10:22:16)
> > > Hi Stephen,
> > > Thanks for the review.
> > > 
> > > On Thu, 2019-06-06 at 10:09 -0700, Stephen Boyd wrote:
> > > > Quoting Nicolas Saenz Julienne (2019-06-06 07:22:56)
> > > > > diff --git a/drivers/cpufreq/raspberrypi-cpufreq.c
> > > > > b/drivers/cpufreq/raspberrypi-cpufreq.c
> > > > > new file mode 100644
> > > > > index 000000000000..99b59d5a50aa
> > > > > --- /dev/null
> > > > > +++ b/drivers/cpufreq/raspberrypi-cpufreq.c
> > > > [...]
> > > > > +
> > > > > +/*
> > > > > + * Since the driver depends on clk-raspberrypi, which may return
> > > > > EPROBE_DEFER,
> > > > > + * all the activity is performed in the probe, which may be defered as
> > > > > well.
> > > > > + */
> > > > > +static struct platform_driver raspberrypi_cpufreq_driver = {
> > > > > +       .driver = {
> > > > > +               .name = "raspberrypi-cpufreq",
> > > > > +       },
> > > > > +       .probe          = raspberrypi_cpufreq_probe,
> > > > > +       .remove         = raspberrypi_cpufreq_remove,
> > > > > +};
> > > > > +module_platform_driver(raspberrypi_cpufreq_driver);
> > > > 
> > > > How does this driver probe? Do you have a node in DT named
> > > > raspberrypi-cpufreq that matches and probes this? I would think this
> > > > would follow the drivers/cpufreq/cpufreq-dt-platdev.c design where it's
> > > > an initcall that probes the board compatible string.
> > > > 
> > > > Or, if it depends on clk-raspberrypi probing, maybe it could create the
> > > > platform device in that drivers probe function.
> > > 
> > > Well you just reviewed that patch :)
> > 
> > Ok. So what's your plan?
> 
> So as discussed previously with the RPi mantainers, they preferred for the
> platform device for raspberrypi-clk to be created by the firmware interface
> driver. IIRC Stefan said it was more flexible and the approach used with RPi's
> hwmon driver already. Also, it's not really clear whether this driver really
> fits the device tree as it wouldn't be describing hardware.
> 
> As far as raspberrypi-cpufreq is concerned the max and min frequencies are
> configurable in the firmware. So we can't really integrate cpufreq into the
> device tree as we need to create the opp table dynamically. Hence the dedicated
> driver. On top of that the CPU might not have a clock during the init process,
> as both the firmware interface and raspberrypi-clk can be compiled as modules.
> So I decided the simplest solution was to create the raspberrypi-cpufreq
> platform device at the end of raspberrypi-clk's probe.
> 
> Once raspberrypi-cpufreq is loaded it queries the min/max frequencies,
> populates the CPU's opp table and creates an instance of cpufreq-dt. Which
> finally can operate, without the need of any dt info, as opp tables are
> populated and CPUs have a clock.
> 
> I hope this makes it a little more clear :).
> 

Yes, thanks. I see that largely follows the commit description so it
looks OK to me.
Nicolas Saenz Julienne June 6, 2019, 6:31 p.m. UTC | #6
On Thu, 2019-06-06 at 11:23 -0700, Stephen Boyd wrote:
> Quoting Nicolas Saenz Julienne (2019-06-06 11:10:04)
> > On Thu, 2019-06-06 at 10:36 -0700, Stephen Boyd wrote:
> > > Quoting Nicolas Saenz Julienne (2019-06-06 10:22:16)
> > > > Hi Stephen,
> > > > Thanks for the review.
> > > > 
> > > > On Thu, 2019-06-06 at 10:09 -0700, Stephen Boyd wrote:
> > > > > Quoting Nicolas Saenz Julienne (2019-06-06 07:22:56)
> > > > > > diff --git a/drivers/cpufreq/raspberrypi-cpufreq.c
> > > > > > b/drivers/cpufreq/raspberrypi-cpufreq.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..99b59d5a50aa
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/cpufreq/raspberrypi-cpufreq.c
> > > > > [...]
> > > > > > +
> > > > > > +/*
> > > > > > + * Since the driver depends on clk-raspberrypi, which may return
> > > > > > EPROBE_DEFER,
> > > > > > + * all the activity is performed in the probe, which may be defered
> > > > > > as
> > > > > > well.
> > > > > > + */
> > > > > > +static struct platform_driver raspberrypi_cpufreq_driver = {
> > > > > > +       .driver = {
> > > > > > +               .name = "raspberrypi-cpufreq",
> > > > > > +       },
> > > > > > +       .probe          = raspberrypi_cpufreq_probe,
> > > > > > +       .remove         = raspberrypi_cpufreq_remove,
> > > > > > +};
> > > > > > +module_platform_driver(raspberrypi_cpufreq_driver);
> > > > > 
> > > > > How does this driver probe? Do you have a node in DT named
> > > > > raspberrypi-cpufreq that matches and probes this? I would think this
> > > > > would follow the drivers/cpufreq/cpufreq-dt-platdev.c design where
> > > > > it's
> > > > > an initcall that probes the board compatible string.
> > > > > 
> > > > > Or, if it depends on clk-raspberrypi probing, maybe it could create
> > > > > the
> > > > > platform device in that drivers probe function.
> > > > 
> > > > Well you just reviewed that patch :)
> > > 
> > > Ok. So what's your plan?
> > 
> > So as discussed previously with the RPi mantainers, they preferred for the
> > platform device for raspberrypi-clk to be created by the firmware interface
> > driver. IIRC Stefan said it was more flexible and the approach used with
> > RPi's
> > hwmon driver already. Also, it's not really clear whether this driver really
> > fits the device tree as it wouldn't be describing hardware.
> > 
> > As far as raspberrypi-cpufreq is concerned the max and min frequencies are
> > configurable in the firmware. So we can't really integrate cpufreq into the
> > device tree as we need to create the opp table dynamically. Hence the
> > dedicated
> > driver. On top of that the CPU might not have a clock during the init
> > process,
> > as both the firmware interface and raspberrypi-clk can be compiled as
> > modules.
> > So I decided the simplest solution was to create the raspberrypi-cpufreq
> > platform device at the end of raspberrypi-clk's probe.
> > 
> > Once raspberrypi-cpufreq is loaded it queries the min/max frequencies,
> > populates the CPU's opp table and creates an instance of cpufreq-dt. Which
> > finally can operate, without the need of any dt info, as opp tables are
> > populated and CPUs have a clock.
> > 
> > I hope this makes it a little more clear :).
> > 
> 
> Yes, thanks. I see that largely follows the commit description so it
> looks OK to me.
> 

Thanks!
Viresh Kumar June 7, 2019, 3:09 a.m. UTC | #7
On 06-06-19, 11:23, Stephen Boyd wrote:
> Yes, thanks. I see that largely follows the commit description so it
> looks OK to me.

Do you want to provide your Reviewed/Acked-by tag before I apply it ?
Stefan Wahren June 7, 2019, 9:13 a.m. UTC | #8
Hi Viresh,

Am 07.06.19 um 05:09 schrieb Viresh Kumar:
> On 06-06-19, 11:23, Stephen Boyd wrote:
>> Yes, thanks. I see that largely follows the commit description so it
>> looks OK to me.
> Do you want to provide your Reviewed/Acked-by tag before I apply it ?

Nicolas wanted to send a V3 of this series and as a platform maintainer
i need some time for testing this version.

Stefan
Stefan Wahren June 7, 2019, 11:42 a.m. UTC | #9
Hi Nicolas,

Am 06.06.19 um 16:22 schrieb Nicolas Saenz Julienne:
> Raspberry Pi's firmware offers and interface though which update it's
> performance requirements. It allows us to request for specific runtime
> frequencies, which the firmware might or might not respect, depending on
> the firmware configuration and thermals.
>
> As the maximum and minimum frequencies are configurable in the firmware
> there is no way to know in advance their values. So the Raspberry Pi
> cpufreq driver queries them, builds an opp frequency table to then
> launch cpufreq-dt.
>
> Also, as the firmware interface might be configured as a module, making
> the cpu clock unavailable during init, this implements a full fledged
> driver, as opposed to most drivers registering cpufreq-dt, which only
> make use of an init routine.
>
> Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> Acked-by: Eric Anholt <eric@anholt.net>
>
> ---
>
> Changes since v1:
>   - Remove compatible checks
>   - Add module support, now full fledged driver
>   - Use NULL in clk_get()
>
>  drivers/cpufreq/Kconfig.arm           |   8 +++
>  drivers/cpufreq/Makefile              |   1 +
>  drivers/cpufreq/raspberrypi-cpufreq.c | 100 ++++++++++++++++++++++++++
>  3 files changed, 109 insertions(+)
>  create mode 100644 drivers/cpufreq/raspberrypi-cpufreq.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index f8129edc145e..5e9204d443ff 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -133,6 +133,14 @@ config ARM_QCOM_CPUFREQ_HW
>  	  The driver implements the cpufreq interface for this HW engine.
>  	  Say Y if you want to support CPUFreq HW.
>  
> +config ARM_RASPBERRYPI_CPUFREQ
> +	tristate "Raspberry Pi cpufreq support"
> +	depends on CLK_RASPBERRYPI || COMPILE_TEST
> +	help
> +	  This adds the CPUFreq driver for Raspberry Pi
> +
> +	  If in doubt, say N.
> +
>  config ARM_S3C_CPUFREQ
>  	bool
>  	help
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index 689b26c6f949..121c1acb66c0 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -64,6 +64,7 @@ obj-$(CONFIG_ARM_PXA2xx_CPUFREQ)	+= pxa2xx-cpufreq.o
>  obj-$(CONFIG_PXA3xx)			+= pxa3xx-cpufreq.o
>  obj-$(CONFIG_ARM_QCOM_CPUFREQ_HW)	+= qcom-cpufreq-hw.o
>  obj-$(CONFIG_ARM_QCOM_CPUFREQ_KRYO)	+= qcom-cpufreq-kryo.o
> +obj-$(CONFIG_ARM_RASPBERRYPI_CPUFREQ) 	+= raspberrypi-cpufreq.o
>  obj-$(CONFIG_ARM_S3C2410_CPUFREQ)	+= s3c2410-cpufreq.o
>  obj-$(CONFIG_ARM_S3C2412_CPUFREQ)	+= s3c2412-cpufreq.o
>  obj-$(CONFIG_ARM_S3C2416_CPUFREQ)	+= s3c2416-cpufreq.o
> diff --git a/drivers/cpufreq/raspberrypi-cpufreq.c b/drivers/cpufreq/raspberrypi-cpufreq.c
> new file mode 100644
> index 000000000000..99b59d5a50aa
> --- /dev/null
> +++ b/drivers/cpufreq/raspberrypi-cpufreq.c
> @@ -0,0 +1,100 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Raspberry Pi cpufreq driver
> + *
> + * Copyright (C) 2019, Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/cpu.h>
> +#include <linux/cpufreq.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +
> +static struct platform_device *cpufreq_dt;
> +
> +static int raspberrypi_cpufreq_probe(struct platform_device *pdev)
> +{
> +	struct device *cpu_dev;
> +	unsigned long min, max;
> +	unsigned long rate;
> +	struct clk *clk;
> +	int ret;
> +
> +	cpu_dev = get_cpu_device(0);
> +	if (!cpu_dev) {
> +		pr_err("Cannot get CPU for cpufreq driver\n");
> +		return -ENODEV;
> +	}
> +
> +	clk = clk_get(cpu_dev, NULL);
> +	if (IS_ERR(clk)) {
> +		dev_err(cpu_dev, "Cannot get clock for CPU0\n");
> +		return PTR_ERR(clk);
> +	}
> +
> +	/*
> +	 * The max and min frequencies are configurable in the Raspberry Pi
> +	 * firmware, so we query them at runtime
> +	 */
> +	min = clk_round_rate(clk, 0);
> +	max = clk_round_rate(clk, ULONG_MAX);
> +	clk_put(clk);
> +
> +	for (rate = min; rate < max; rate += 100000000) {
> +		ret = dev_pm_opp_add(cpu_dev, rate, 0);
> +		if (ret)
> +			goto remove_opp;
> +	}

i played a little bit with my Raspberry Pi Zero W and this series. Looks
fine so far.

Sorry for this nitpicking, but i expect user questions about the
differences between sysfs and vcgencmd measure_clock.

scaling_available_frequencies gives

699999 799999 899999 999999

but vcgencmd measure_clock return the rounded up values.

I know we shouldn't fake anything, but adding the OPPs rounded up may
avoid confusion.

Stefan
Nicolas Saenz Julienne June 7, 2019, 11:57 a.m. UTC | #10
On Fri, 2019-06-07 at 13:42 +0200, Stefan Wahren wrote:
> Hi Nicolas,
> 
> Am 06.06.19 um 16:22 schrieb Nicolas Saenz Julienne:
> > Raspberry Pi's firmware offers and interface though which update it's
> > performance requirements. It allows us to request for specific runtime
> > frequencies, which the firmware might or might not respect, depending on
> > the firmware configuration and thermals.
> > 
> > As the maximum and minimum frequencies are configurable in the firmware
> > there is no way to know in advance their values. So the Raspberry Pi
> > cpufreq driver queries them, builds an opp frequency table to then
> > launch cpufreq-dt.
> > 
> > Also, as the firmware interface might be configured as a module, making
> > the cpu clock unavailable during init, this implements a full fledged
> > driver, as opposed to most drivers registering cpufreq-dt, which only
> > make use of an init routine.
> > 
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > Acked-by: Eric Anholt <eric@anholt.net>
> > 
> > ---
> > 
> > Changes since v1:
> >   - Remove compatible checks
> >   - Add module support, now full fledged driver
> >   - Use NULL in clk_get()
> > 
> >  drivers/cpufreq/Kconfig.arm           |   8 +++
> >  drivers/cpufreq/Makefile              |   1 +
> >  drivers/cpufreq/raspberrypi-cpufreq.c | 100 ++++++++++++++++++++++++++
> >  3 files changed, 109 insertions(+)
> >  create mode 100644 drivers/cpufreq/raspberrypi-cpufreq.c
> > 
> > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> > index f8129edc145e..5e9204d443ff 100644
> > --- a/drivers/cpufreq/Kconfig.arm
> > +++ b/drivers/cpufreq/Kconfig.arm
> > @@ -133,6 +133,14 @@ config ARM_QCOM_CPUFREQ_HW
> >  	  The driver implements the cpufreq interface for this HW engine.
> >  	  Say Y if you want to support CPUFreq HW.
> >  
> > +config ARM_RASPBERRYPI_CPUFREQ
> > +	tristate "Raspberry Pi cpufreq support"
> > +	depends on CLK_RASPBERRYPI || COMPILE_TEST
> > +	help
> > +	  This adds the CPUFreq driver for Raspberry Pi
> > +
> > +	  If in doubt, say N.
> > +
> >  config ARM_S3C_CPUFREQ
> >  	bool
> >  	help
> > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> > index 689b26c6f949..121c1acb66c0 100644
> > --- a/drivers/cpufreq/Makefile
> > +++ b/drivers/cpufreq/Makefile
> > @@ -64,6 +64,7 @@ obj-$(CONFIG_ARM_PXA2xx_CPUFREQ)	+= pxa2xx-cpufreq.o
> >  obj-$(CONFIG_PXA3xx)			+= pxa3xx-cpufreq.o
> >  obj-$(CONFIG_ARM_QCOM_CPUFREQ_HW)	+= qcom-cpufreq-hw.o
> >  obj-$(CONFIG_ARM_QCOM_CPUFREQ_KRYO)	+= qcom-cpufreq-kryo.o
> > +obj-$(CONFIG_ARM_RASPBERRYPI_CPUFREQ) 	+= raspberrypi-cpufreq.o
> >  obj-$(CONFIG_ARM_S3C2410_CPUFREQ)	+= s3c2410-cpufreq.o
> >  obj-$(CONFIG_ARM_S3C2412_CPUFREQ)	+= s3c2412-cpufreq.o
> >  obj-$(CONFIG_ARM_S3C2416_CPUFREQ)	+= s3c2416-cpufreq.o
> > diff --git a/drivers/cpufreq/raspberrypi-cpufreq.c
> > b/drivers/cpufreq/raspberrypi-cpufreq.c
> > new file mode 100644
> > index 000000000000..99b59d5a50aa
> > --- /dev/null
> > +++ b/drivers/cpufreq/raspberrypi-cpufreq.c
> > @@ -0,0 +1,100 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Raspberry Pi cpufreq driver
> > + *
> > + * Copyright (C) 2019, Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/cpu.h>
> > +#include <linux/cpufreq.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_opp.h>
> > +
> > +static struct platform_device *cpufreq_dt;
> > +
> > +static int raspberrypi_cpufreq_probe(struct platform_device *pdev)
> > +{
> > +	struct device *cpu_dev;
> > +	unsigned long min, max;
> > +	unsigned long rate;
> > +	struct clk *clk;
> > +	int ret;
> > +
> > +	cpu_dev = get_cpu_device(0);
> > +	if (!cpu_dev) {
> > +		pr_err("Cannot get CPU for cpufreq driver\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	clk = clk_get(cpu_dev, NULL);
> > +	if (IS_ERR(clk)) {
> > +		dev_err(cpu_dev, "Cannot get clock for CPU0\n");
> > +		return PTR_ERR(clk);
> > +	}
> > +
> > +	/*
> > +	 * The max and min frequencies are configurable in the Raspberry Pi
> > +	 * firmware, so we query them at runtime
> > +	 */
> > +	min = clk_round_rate(clk, 0);
> > +	max = clk_round_rate(clk, ULONG_MAX);
> > +	clk_put(clk);
> > +
> > +	for (rate = min; rate < max; rate += 100000000) {
> > +		ret = dev_pm_opp_add(cpu_dev, rate, 0);
> > +		if (ret)
> > +			goto remove_opp;
> > +	}
> 
> i played a little bit with my Raspberry Pi Zero W and this series. Looks
> fine so far.
> 
> Sorry for this nitpicking, but i expect user questions about the
> differences between sysfs and vcgencmd measure_clock.
> 
> scaling_available_frequencies gives
> 
> 699999 799999 899999 999999
> 
> but vcgencmd measure_clock return the rounded up values.
> 
> I know we shouldn't fake anything, but adding the OPPs rounded up may
> avoid confusion.
> 
> Stefan

Agree, I'll change this in v3.
Stephen Boyd June 7, 2019, 7:02 p.m. UTC | #11
Quoting Stefan Wahren (2019-06-07 02:13:54)
> Hi Viresh,
> 
> Am 07.06.19 um 05:09 schrieb Viresh Kumar:
> > On 06-06-19, 11:23, Stephen Boyd wrote:
> >> Yes, thanks. I see that largely follows the commit description so it
> >> looks OK to me.
> > Do you want to provide your Reviewed/Acked-by tag before I apply it ?
> 
> Nicolas wanted to send a V3 of this series and as a platform maintainer
> i need some time for testing this version.
> 

You can add my review tag.

Reviewed-by: Stephen Boyd <sboyd@kernel.org>
diff mbox series

Patch

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index f8129edc145e..5e9204d443ff 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -133,6 +133,14 @@  config ARM_QCOM_CPUFREQ_HW
 	  The driver implements the cpufreq interface for this HW engine.
 	  Say Y if you want to support CPUFreq HW.
 
+config ARM_RASPBERRYPI_CPUFREQ
+	tristate "Raspberry Pi cpufreq support"
+	depends on CLK_RASPBERRYPI || COMPILE_TEST
+	help
+	  This adds the CPUFreq driver for Raspberry Pi
+
+	  If in doubt, say N.
+
 config ARM_S3C_CPUFREQ
 	bool
 	help
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 689b26c6f949..121c1acb66c0 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -64,6 +64,7 @@  obj-$(CONFIG_ARM_PXA2xx_CPUFREQ)	+= pxa2xx-cpufreq.o
 obj-$(CONFIG_PXA3xx)			+= pxa3xx-cpufreq.o
 obj-$(CONFIG_ARM_QCOM_CPUFREQ_HW)	+= qcom-cpufreq-hw.o
 obj-$(CONFIG_ARM_QCOM_CPUFREQ_KRYO)	+= qcom-cpufreq-kryo.o
+obj-$(CONFIG_ARM_RASPBERRYPI_CPUFREQ) 	+= raspberrypi-cpufreq.o
 obj-$(CONFIG_ARM_S3C2410_CPUFREQ)	+= s3c2410-cpufreq.o
 obj-$(CONFIG_ARM_S3C2412_CPUFREQ)	+= s3c2412-cpufreq.o
 obj-$(CONFIG_ARM_S3C2416_CPUFREQ)	+= s3c2416-cpufreq.o
diff --git a/drivers/cpufreq/raspberrypi-cpufreq.c b/drivers/cpufreq/raspberrypi-cpufreq.c
new file mode 100644
index 000000000000..99b59d5a50aa
--- /dev/null
+++ b/drivers/cpufreq/raspberrypi-cpufreq.c
@@ -0,0 +1,100 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Raspberry Pi cpufreq driver
+ *
+ * Copyright (C) 2019, Nicolas Saenz Julienne <nsaenzjulienne@suse.de>
+ */
+
+#include <linux/clk.h>
+#include <linux/cpu.h>
+#include <linux/cpufreq.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pm_opp.h>
+
+static struct platform_device *cpufreq_dt;
+
+static int raspberrypi_cpufreq_probe(struct platform_device *pdev)
+{
+	struct device *cpu_dev;
+	unsigned long min, max;
+	unsigned long rate;
+	struct clk *clk;
+	int ret;
+
+	cpu_dev = get_cpu_device(0);
+	if (!cpu_dev) {
+		pr_err("Cannot get CPU for cpufreq driver\n");
+		return -ENODEV;
+	}
+
+	clk = clk_get(cpu_dev, NULL);
+	if (IS_ERR(clk)) {
+		dev_err(cpu_dev, "Cannot get clock for CPU0\n");
+		return PTR_ERR(clk);
+	}
+
+	/*
+	 * The max and min frequencies are configurable in the Raspberry Pi
+	 * firmware, so we query them at runtime
+	 */
+	min = clk_round_rate(clk, 0);
+	max = clk_round_rate(clk, ULONG_MAX);
+	clk_put(clk);
+
+	for (rate = min; rate < max; rate += 100000000) {
+		ret = dev_pm_opp_add(cpu_dev, rate, 0);
+		if (ret)
+			goto remove_opp;
+	}
+
+	ret = dev_pm_opp_add(cpu_dev, max, 0);
+	if (ret)
+		goto remove_opp;
+
+	cpufreq_dt = platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
+	ret = PTR_ERR_OR_ZERO(cpufreq_dt);
+	if (ret) {
+		dev_err(cpu_dev, "Failed to create platform device, %d\n", ret);
+		goto remove_opp;
+	}
+
+	return 0;
+
+remove_opp:
+	dev_pm_opp_remove_all_dynamic(cpu_dev);
+
+	return ret;
+}
+
+static int raspberrypi_cpufreq_remove(struct platform_device *pdev)
+{
+	struct device *cpu_dev;
+
+	cpu_dev = get_cpu_device(0);
+	if (cpu_dev)
+		dev_pm_opp_remove_all_dynamic(cpu_dev);
+
+	platform_device_unregister(cpufreq_dt);
+	cpufreq_dt = NULL;
+
+	return 0;
+}
+
+/*
+ * Since the driver depends on clk-raspberrypi, which may return EPROBE_DEFER,
+ * all the activity is performed in the probe, which may be defered as well.
+ */
+static struct platform_driver raspberrypi_cpufreq_driver = {
+	.driver = {
+		.name = "raspberrypi-cpufreq",
+	},
+	.probe          = raspberrypi_cpufreq_probe,
+	.remove		= raspberrypi_cpufreq_remove,
+};
+module_platform_driver(raspberrypi_cpufreq_driver);
+
+MODULE_AUTHOR("Nicolas Saenz Julienne <nsaenzjulienne@suse.de");
+MODULE_DESCRIPTION("Raspberry Pi cpufreq driver");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:raspberrypi-cpufreq");