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

[Nicolas Saenz Julienne]

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@xxxxxxx");
> > > > +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

Attachment: signature.asc
Description: This is a digitally signed message part