diff mbox series

[V4,8/8] cpufreq: Add Rust based cpufreq-dt driver

Message ID b1601c1dbdf68a4b812fcfaf73f3b5dea67f2025.1720680252.git.viresh.kumar@linaro.org (mailing list archive)
State Superseded, archived
Headers show
Series Rust bindings for cpufreq and OPP core + sample driver | expand

Commit Message

Viresh Kumar July 11, 2024, 6:57 a.m. UTC
This commit adds a Rust based cpufreq-dt driver, which covers most of
the functionality of the existing C based driver. Only a handful of
things are left, like fetching platform data from cpufreq-dt-platdev.c.

This is tested with the help of QEMU for now and switching of
frequencies work as expected.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/Kconfig        |  12 ++
 drivers/cpufreq/Makefile       |   1 +
 drivers/cpufreq/rcpufreq_dt.rs | 222 +++++++++++++++++++++++++++++++++
 3 files changed, 235 insertions(+)
 create mode 100644 drivers/cpufreq/rcpufreq_dt.rs

Comments

Danilo Krummrich July 11, 2024, 10:43 a.m. UTC | #1
On Thu, Jul 11, 2024 at 12:27:50PM +0530, Viresh Kumar wrote:
> This commit adds a Rust based cpufreq-dt driver, which covers most of
> the functionality of the existing C based driver. Only a handful of
> things are left, like fetching platform data from cpufreq-dt-platdev.c.
> 
> This is tested with the help of QEMU for now and switching of
> frequencies work as expected.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/Kconfig        |  12 ++
>  drivers/cpufreq/Makefile       |   1 +
>  drivers/cpufreq/rcpufreq_dt.rs | 222 +++++++++++++++++++++++++++++++++
>  3 files changed, 235 insertions(+)
>  create mode 100644 drivers/cpufreq/rcpufreq_dt.rs
> 
> diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs
> new file mode 100644
> index 000000000000..3e86ad134e13
> --- /dev/null
> +++ b/drivers/cpufreq/rcpufreq_dt.rs
> +
> +impl platform::Driver for CPUFreqDTDriver {
> +    type Data = ();
> +
> +    define_of_id_table! {(), [
> +        (of::DeviceId(b_str!("operating-points-v2")), None),
> +    ]}
> +
> +    fn probe(dev: &mut platform::Device, _id_info: Option<&Self::IdInfo>) -> Result<Self::Data> {
> +        let drv = cpufreq::Registration::<CPUFreqDTDriver>::register(

Please just call this function `cpufreq::Registration::new`.

The existance of a `cpufreq::Registration` means that it's registered. Once it
is dropped, it's unregistered. It's the whole point of a `Registration` type
to bind the period of a driver being registered to the lifetime of a
`Registration` instance.

Having `Registration::register` implies a bit, that we could ever have an
unregistered `Registration`, which can never happen.

Besides that, it'd be nice to follow the same naming scheme everywhere.

> +            c_str!("cpufreq-dt"),
> +            (),
> +            cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV,
> +            true,
> +        )?;
> +
> +        Devres::new_foreign_owned(dev.as_ref(), drv, GFP_KERNEL)?;

This should be called by `cpufreq::Registration` directly, otherwise it's every
driver's responsibility to take care of the registration lifetime.

> +
> +        pr_info!("CPUFreq DT driver registered\n");
> +
> +        Ok(())
> +    }
> +}
> +
> +module_platform_driver! {
> +    type: CPUFreqDTDriver,
> +    name: "cpufreq_dt",
> +    author: "Viresh Kumar <viresh.kumar@linaro.org>",
> +    description: "Generic CPUFreq DT driver",
> +    license: "GPL v2",
> +}
> -- 
> 2.31.1.272.g89b43f80a514
>
Viresh Kumar July 11, 2024, 1:08 p.m. UTC | #2
On 11-07-24, 12:43, Danilo Krummrich wrote:
> Please just call this function `cpufreq::Registration::new`.
> 
> The existance of a `cpufreq::Registration` means that it's registered. Once it
> is dropped, it's unregistered. It's the whole point of a `Registration` type
> to bind the period of a driver being registered to the lifetime of a
> `Registration` instance.
> 
> Having `Registration::register` implies a bit, that we could ever have an
> unregistered `Registration`, which can never happen.
> 
> Besides that, it'd be nice to follow the same naming scheme everywhere.

Sure, ::new() looks fine.

> > +            c_str!("cpufreq-dt"),
> > +            (),
> > +            cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV,
> > +            true,
> > +        )?;
> > +
> > +        Devres::new_foreign_owned(dev.as_ref(), drv, GFP_KERNEL)?;
> 
> This should be called by `cpufreq::Registration` directly, otherwise it's every
> driver's responsibility to take care of the registration lifetime.

Some details were shared in another thread [1] earlier and I understand that
they are not very clear otherwise.

The problem is that it is not guaranteed that a struct device will be available
to the cpufreq core all the time, to which a platform driver (or other bus) can
be bound. And so this has to be taken care of by the individual drivers only.
Danilo Krummrich July 11, 2024, 1:21 p.m. UTC | #3
On Thu, Jul 11, 2024 at 06:38:02PM +0530, Viresh Kumar wrote:
> On 11-07-24, 12:43, Danilo Krummrich wrote:
> > Please just call this function `cpufreq::Registration::new`.
> > 
> > The existance of a `cpufreq::Registration` means that it's registered. Once it
> > is dropped, it's unregistered. It's the whole point of a `Registration` type
> > to bind the period of a driver being registered to the lifetime of a
> > `Registration` instance.
> > 
> > Having `Registration::register` implies a bit, that we could ever have an
> > unregistered `Registration`, which can never happen.
> > 
> > Besides that, it'd be nice to follow the same naming scheme everywhere.
> 
> Sure, ::new() looks fine.
> 
> > > +            c_str!("cpufreq-dt"),
> > > +            (),
> > > +            cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV,
> > > +            true,
> > > +        )?;
> > > +
> > > +        Devres::new_foreign_owned(dev.as_ref(), drv, GFP_KERNEL)?;
> > 
> > This should be called by `cpufreq::Registration` directly, otherwise it's every
> > driver's responsibility to take care of the registration lifetime.
> 
> Some details were shared in another thread [1] earlier and I understand that
> they are not very clear otherwise.
> 
> The problem is that it is not guaranteed that a struct device will be available
> to the cpufreq core all the time, to which a platform driver (or other bus) can
> be bound. And so this has to be taken care of by the individual drivers only.

I guess you are referring to the case where you want to register a CPUfreq
driver directly from `Module::init`. I see two possible options for that, with
one of them being the preference.

(1) You simply provide an additional `Registration::new_foreign_owed` function.

(2) You require drivers to always implement a "dummy" struct platform_device,
there is platform_device_register_simple() for that purpose.

I think (2) is the preferred option.

> 
> -- 
> viresh
> 
> [1] https://lore.kernel.org/all/20240620100556.xsehtd7ii25rtn7k@vireshk-i7/
>
Greg KH July 11, 2024, 2:37 p.m. UTC | #4
On Thu, Jul 11, 2024 at 03:21:31PM +0200, Danilo Krummrich wrote:
> (2) You require drivers to always implement a "dummy" struct platform_device,
> there is platform_device_register_simple() for that purpose.

No, NEVER do that.  platform devices are only for real platform devices,
do not abuse that interface any more than it already is.

> I think (2) is the preferred option.

No, not at all, sorry.

Use a real device, you have one somewhere that relates to this hardware,
otherwise you aren't controlling anything and then you can use a virtual
device.

Again, do NOT abuse the platform subsystem.  It's one reason I am loath
to even want to allow rust bindings to it.

greg k-h
Danilo Krummrich July 11, 2024, 4:12 p.m. UTC | #5
On Thu, Jul 11, 2024 at 04:37:50PM +0200, Greg KH wrote:
> On Thu, Jul 11, 2024 at 03:21:31PM +0200, Danilo Krummrich wrote:
> > (2) You require drivers to always implement a "dummy" struct platform_device,
> > there is platform_device_register_simple() for that purpose.
> 
> No, NEVER do that.  platform devices are only for real platform devices,
> do not abuse that interface any more than it already is.

I thought we're talking about cases like [1] or [2], but please correct me if
those are considered abusing the platform bus as well.

(Those drivers read the CPU OF nodes, instead of OF nodes that represent a
separate device.)

[1] https://elixir.bootlin.com/linux/latest/source/drivers/cpuidle/cpuidle-riscv-sbi.c#L586
[2] https://elixir.bootlin.com/linux/latest/source/drivers/cpuidle/cpuidle-psci.c#L441

> 
> > I think (2) is the preferred option.
> 
> No, not at all, sorry.
> 
> Use a real device, you have one somewhere that relates to this hardware,
> otherwise you aren't controlling anything and then you can use a virtual
> device.

Of course we should stick to a real device if there is one, I didn't meant to
say anything else.

But since it came up now, some virtual drivers still require a parent device.

For instance, in DRM we have vGEM [3] and vKMS [4], that use
platform_device_register_simple() for this purpose.

What should they use instead? I'm happy to fix things up if required.

[3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/vgem/vgem_drv.c
[4] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/vkms/vkms_drv.c

> 
> Again, do NOT abuse the platform subsystem.  It's one reason I am loath
> to even want to allow rust bindings to it.

How is that related to Rust?

> 
> greg k-h
>
Greg KH July 11, 2024, 4:34 p.m. UTC | #6
On Thu, Jul 11, 2024 at 06:12:08PM +0200, Danilo Krummrich wrote:
> On Thu, Jul 11, 2024 at 04:37:50PM +0200, Greg KH wrote:
> > On Thu, Jul 11, 2024 at 03:21:31PM +0200, Danilo Krummrich wrote:
> > > (2) You require drivers to always implement a "dummy" struct platform_device,
> > > there is platform_device_register_simple() for that purpose.
> > 
> > No, NEVER do that.  platform devices are only for real platform devices,
> > do not abuse that interface any more than it already is.
> 
> I thought we're talking about cases like [1] or [2], but please correct me if
> those are considered abusing the platform bus as well.
> 
> (Those drivers read the CPU OF nodes, instead of OF nodes that represent a
> separate device.)
> 
> [1] https://elixir.bootlin.com/linux/latest/source/drivers/cpuidle/cpuidle-riscv-sbi.c#L586
> [2] https://elixir.bootlin.com/linux/latest/source/drivers/cpuidle/cpuidle-psci.c#L441

Yes, these are abuses of that and should be virtual devices as they have
nothing to do with the platform bus.

> > > I think (2) is the preferred option.
> > 
> > No, not at all, sorry.
> > 
> > Use a real device, you have one somewhere that relates to this hardware,
> > otherwise you aren't controlling anything and then you can use a virtual
> > device.
> 
> Of course we should stick to a real device if there is one, I didn't meant to
> say anything else.
> 
> But since it came up now, some virtual drivers still require a parent device.

Great, use the default one that the kernel gives you :)

> For instance, in DRM we have vGEM [3] and vKMS [4], that use
> platform_device_register_simple() for this purpose.

Again, abuse, please do not do so.

> What should they use instead? I'm happy to fix things up if required.
> 
> [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/vgem/vgem_drv.c
> [4] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/vkms/vkms_drv.c

Use the virtual device interface please, that's what it is there for.

thanks,

greg k-h
Greg KH July 11, 2024, 4:41 p.m. UTC | #7
On Thu, Jul 11, 2024 at 06:34:22PM +0200, Greg KH wrote:
> On Thu, Jul 11, 2024 at 06:12:08PM +0200, Danilo Krummrich wrote:
> > On Thu, Jul 11, 2024 at 04:37:50PM +0200, Greg KH wrote:
> > > On Thu, Jul 11, 2024 at 03:21:31PM +0200, Danilo Krummrich wrote:
> > > > (2) You require drivers to always implement a "dummy" struct platform_device,
> > > > there is platform_device_register_simple() for that purpose.
> > > 
> > > No, NEVER do that.  platform devices are only for real platform devices,
> > > do not abuse that interface any more than it already is.
> > 
> > I thought we're talking about cases like [1] or [2], but please correct me if
> > those are considered abusing the platform bus as well.
> > 
> > (Those drivers read the CPU OF nodes, instead of OF nodes that represent a
> > separate device.)
> > 
> > [1] https://elixir.bootlin.com/linux/latest/source/drivers/cpuidle/cpuidle-riscv-sbi.c#L586
> > [2] https://elixir.bootlin.com/linux/latest/source/drivers/cpuidle/cpuidle-psci.c#L441
> 
> Yes, these are abuses of that and should be virtual devices as they have
> nothing to do with the platform bus.
> 
> > > > I think (2) is the preferred option.
> > > 
> > > No, not at all, sorry.
> > > 
> > > Use a real device, you have one somewhere that relates to this hardware,
> > > otherwise you aren't controlling anything and then you can use a virtual
> > > device.
> > 
> > Of course we should stick to a real device if there is one, I didn't meant to
> > say anything else.
> > 
> > But since it came up now, some virtual drivers still require a parent device.
> 
> Great, use the default one that the kernel gives you :)
> 
> > For instance, in DRM we have vGEM [3] and vKMS [4], that use
> > platform_device_register_simple() for this purpose.
> 
> Again, abuse, please do not do so.
> 
> > What should they use instead? I'm happy to fix things up if required.
> > 
> > [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/vgem/vgem_drv.c
> > [4] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/vkms/vkms_drv.c
> 
> Use the virtual device interface please, that's what it is there for.

To be specific, look at the devices in /sys/devices/virtual/ that's
where yours should be showing up in, not in the root of /sys/devices/
like they are by creating a "fake" platform device at the root of the
device tree.

thanks,

greg k-h
Greg KH July 11, 2024, 5:21 p.m. UTC | #8
On Thu, Jul 11, 2024 at 06:41:29PM +0200, Greg KH wrote:
> On Thu, Jul 11, 2024 at 06:34:22PM +0200, Greg KH wrote:
> > On Thu, Jul 11, 2024 at 06:12:08PM +0200, Danilo Krummrich wrote:
> > > On Thu, Jul 11, 2024 at 04:37:50PM +0200, Greg KH wrote:
> > > > On Thu, Jul 11, 2024 at 03:21:31PM +0200, Danilo Krummrich wrote:
> > > > > (2) You require drivers to always implement a "dummy" struct platform_device,
> > > > > there is platform_device_register_simple() for that purpose.
> > > > 
> > > > No, NEVER do that.  platform devices are only for real platform devices,
> > > > do not abuse that interface any more than it already is.
> > > 
> > > I thought we're talking about cases like [1] or [2], but please correct me if
> > > those are considered abusing the platform bus as well.
> > > 
> > > (Those drivers read the CPU OF nodes, instead of OF nodes that represent a
> > > separate device.)
> > > 
> > > [1] https://elixir.bootlin.com/linux/latest/source/drivers/cpuidle/cpuidle-riscv-sbi.c#L586
> > > [2] https://elixir.bootlin.com/linux/latest/source/drivers/cpuidle/cpuidle-psci.c#L441
> > 
> > Yes, these are abuses of that and should be virtual devices as they have
> > nothing to do with the platform bus.
> > 
> > > > > I think (2) is the preferred option.
> > > > 
> > > > No, not at all, sorry.
> > > > 
> > > > Use a real device, you have one somewhere that relates to this hardware,
> > > > otherwise you aren't controlling anything and then you can use a virtual
> > > > device.
> > > 
> > > Of course we should stick to a real device if there is one, I didn't meant to
> > > say anything else.
> > > 
> > > But since it came up now, some virtual drivers still require a parent device.
> > 
> > Great, use the default one that the kernel gives you :)
> > 
> > > For instance, in DRM we have vGEM [3] and vKMS [4], that use
> > > platform_device_register_simple() for this purpose.
> > 
> > Again, abuse, please do not do so.
> > 
> > > What should they use instead? I'm happy to fix things up if required.
> > > 
> > > [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/vgem/vgem_drv.c
> > > [4] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/vkms/vkms_drv.c
> > 
> > Use the virtual device interface please, that's what it is there for.
> 
> To be specific, look at the devices in /sys/devices/virtual/ that's
> where yours should be showing up in, not in the root of /sys/devices/
> like they are by creating a "fake" platform device at the root of the
> device tree.

Ok, at first glance this seems a little bit more complex than what the
platform api gives you, let me knock something up next week during the
merge window to make this more simple and then let some interns at it to
sweep the kernel tree and fix up this proliferation of platform device
abuse.

thanks,

greg k-h
Danilo Krummrich July 11, 2024, 5:42 p.m. UTC | #9
On Thu, Jul 11, 2024 at 07:21:59PM +0200, Greg KH wrote:
> On Thu, Jul 11, 2024 at 06:41:29PM +0200, Greg KH wrote:
> > On Thu, Jul 11, 2024 at 06:34:22PM +0200, Greg KH wrote:
> > > On Thu, Jul 11, 2024 at 06:12:08PM +0200, Danilo Krummrich wrote:
> > > > On Thu, Jul 11, 2024 at 04:37:50PM +0200, Greg KH wrote:
> > > > > On Thu, Jul 11, 2024 at 03:21:31PM +0200, Danilo Krummrich wrote:
> > > > > > (2) You require drivers to always implement a "dummy" struct platform_device,
> > > > > > there is platform_device_register_simple() for that purpose.
> > > > > 
> > > > > No, NEVER do that.  platform devices are only for real platform devices,
> > > > > do not abuse that interface any more than it already is.
> > > > 
> > > > I thought we're talking about cases like [1] or [2], but please correct me if
> > > > those are considered abusing the platform bus as well.
> > > > 
> > > > (Those drivers read the CPU OF nodes, instead of OF nodes that represent a
> > > > separate device.)
> > > > 
> > > > [1] https://elixir.bootlin.com/linux/latest/source/drivers/cpuidle/cpuidle-riscv-sbi.c#L586
> > > > [2] https://elixir.bootlin.com/linux/latest/source/drivers/cpuidle/cpuidle-psci.c#L441
> > > 
> > > Yes, these are abuses of that and should be virtual devices as they have
> > > nothing to do with the platform bus.
> > > 
> > > > > > I think (2) is the preferred option.
> > > > > 
> > > > > No, not at all, sorry.
> > > > > 
> > > > > Use a real device, you have one somewhere that relates to this hardware,
> > > > > otherwise you aren't controlling anything and then you can use a virtual
> > > > > device.
> > > > 
> > > > Of course we should stick to a real device if there is one, I didn't meant to
> > > > say anything else.
> > > > 
> > > > But since it came up now, some virtual drivers still require a parent device.
> > > 
> > > Great, use the default one that the kernel gives you :)
> > > 
> > > > For instance, in DRM we have vGEM [3] and vKMS [4], that use
> > > > platform_device_register_simple() for this purpose.
> > > 
> > > Again, abuse, please do not do so.
> > > 
> > > > What should they use instead? I'm happy to fix things up if required.
> > > > 
> > > > [3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/vgem/vgem_drv.c
> > > > [4] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/vkms/vkms_drv.c
> > > 
> > > Use the virtual device interface please, that's what it is there for.
> > 
> > To be specific, look at the devices in /sys/devices/virtual/ that's
> > where yours should be showing up in, not in the root of /sys/devices/
> > like they are by creating a "fake" platform device at the root of the
> > device tree.
> 
> Ok, at first glance this seems a little bit more complex than what the
> platform api gives you, let me knock something up next week during the
> merge window to make this more simple and then let some interns at it to
> sweep the kernel tree and fix up this proliferation of platform device
> abuse.

Yeah, I stared at this for the last 30 minutes and was just about to reply.

I think that we probably want to get rid of this abuse, as there are quite a lot
of examples for this.

And considering that I wasn't able to find a rather straight forward replacement
that makes it go into /sys/devices/virtual/ I think it's not super unexpected
that this spreads.

It looks like we probably want some kind virtual device API for that purpose?

> 
> thanks,
> 
> greg k-h
>
Danilo Krummrich July 16, 2024, 3:15 p.m. UTC | #10
On Thu, Jul 11, 2024 at 06:34:22PM +0200, Greg KH wrote:
> On Thu, Jul 11, 2024 at 06:12:08PM +0200, Danilo Krummrich wrote:
> > On Thu, Jul 11, 2024 at 04:37:50PM +0200, Greg KH wrote:
> > > On Thu, Jul 11, 2024 at 03:21:31PM +0200, Danilo Krummrich wrote:
> > > > (2) You require drivers to always implement a "dummy" struct platform_device,
> > > > there is platform_device_register_simple() for that purpose.
> > > 
> > > No, NEVER do that.  platform devices are only for real platform devices,
> > > do not abuse that interface any more than it already is.
> > 
> > I thought we're talking about cases like [1] or [2], but please correct me if
> > those are considered abusing the platform bus as well.
> > 
> > (Those drivers read the CPU OF nodes, instead of OF nodes that represent a
> > separate device.)
> > 
> > [1] https://elixir.bootlin.com/linux/latest/source/drivers/cpuidle/cpuidle-riscv-sbi.c#L586
> > [2] https://elixir.bootlin.com/linux/latest/source/drivers/cpuidle/cpuidle-psci.c#L441
> 
> Yes, these are abuses of that and should be virtual devices as they have
> nothing to do with the platform bus.

For those drivers, wouldn't it be better if proper devices would be derived from
the CPU OF nodes directly? This seems to be a common problem for cpuidle and
cpufreq drivers.

But it's quite a while ago I dealt with such drivers, maybe there are reasons
not to do so.

Anyway, using a virtual device for those seems a bit wrong to me.

- Danilo
Greg KH July 16, 2024, 3:22 p.m. UTC | #11
On Tue, Jul 16, 2024 at 05:15:25PM +0200, Danilo Krummrich wrote:
> On Thu, Jul 11, 2024 at 06:34:22PM +0200, Greg KH wrote:
> > On Thu, Jul 11, 2024 at 06:12:08PM +0200, Danilo Krummrich wrote:
> > > On Thu, Jul 11, 2024 at 04:37:50PM +0200, Greg KH wrote:
> > > > On Thu, Jul 11, 2024 at 03:21:31PM +0200, Danilo Krummrich wrote:
> > > > > (2) You require drivers to always implement a "dummy" struct platform_device,
> > > > > there is platform_device_register_simple() for that purpose.
> > > > 
> > > > No, NEVER do that.  platform devices are only for real platform devices,
> > > > do not abuse that interface any more than it already is.
> > > 
> > > I thought we're talking about cases like [1] or [2], but please correct me if
> > > those are considered abusing the platform bus as well.
> > > 
> > > (Those drivers read the CPU OF nodes, instead of OF nodes that represent a
> > > separate device.)
> > > 
> > > [1] https://elixir.bootlin.com/linux/latest/source/drivers/cpuidle/cpuidle-riscv-sbi.c#L586
> > > [2] https://elixir.bootlin.com/linux/latest/source/drivers/cpuidle/cpuidle-psci.c#L441
> > 
> > Yes, these are abuses of that and should be virtual devices as they have
> > nothing to do with the platform bus.
> 
> For those drivers, wouldn't it be better if proper devices would be derived from
> the CPU OF nodes directly? This seems to be a common problem for cpuidle and
> cpufreq drivers.

Yes they should.

> But it's quite a while ago I dealt with such drivers, maybe there are reasons
> not to do so.

I think people just got lazy :)

thanks,

greg k-h
Rob Herring (Arm) July 16, 2024, 3:53 p.m. UTC | #12
On Tue, Jul 16, 2024 at 9:22 AM Greg KH <greg@kroah.com> wrote:
>
> On Tue, Jul 16, 2024 at 05:15:25PM +0200, Danilo Krummrich wrote:
> > On Thu, Jul 11, 2024 at 06:34:22PM +0200, Greg KH wrote:
> > > On Thu, Jul 11, 2024 at 06:12:08PM +0200, Danilo Krummrich wrote:
> > > > On Thu, Jul 11, 2024 at 04:37:50PM +0200, Greg KH wrote:
> > > > > On Thu, Jul 11, 2024 at 03:21:31PM +0200, Danilo Krummrich wrote:
> > > > > > (2) You require drivers to always implement a "dummy" struct platform_device,
> > > > > > there is platform_device_register_simple() for that purpose.
> > > > >
> > > > > No, NEVER do that.  platform devices are only for real platform devices,
> > > > > do not abuse that interface any more than it already is.
> > > >
> > > > I thought we're talking about cases like [1] or [2], but please correct me if
> > > > those are considered abusing the platform bus as well.
> > > >
> > > > (Those drivers read the CPU OF nodes, instead of OF nodes that represent a
> > > > separate device.)
> > > >
> > > > [1] https://elixir.bootlin.com/linux/latest/source/drivers/cpuidle/cpuidle-riscv-sbi.c#L586
> > > > [2] https://elixir.bootlin.com/linux/latest/source/drivers/cpuidle/cpuidle-psci.c#L441
> > >
> > > Yes, these are abuses of that and should be virtual devices as they have
> > > nothing to do with the platform bus.
> >
> > For those drivers, wouldn't it be better if proper devices would be derived from
> > the CPU OF nodes directly? This seems to be a common problem for cpuidle and
> > cpufreq drivers.
>
> Yes they should.

Well, which one do we bind? The cpufreq driver or cpuidle driver? Or
there's the thermal f/w throttling as well. It's messy. Also, the CPUs
already have a struct device associated with them for the topology
stuff, but no driver IIRC.

Another complication is it is not the CPU that determines what
cpufreq/cpuidle drivers to use, but a platform decision. That decision
may evolve as well which means it can't be driven from the DT.

> > But it's quite a while ago I dealt with such drivers, maybe there are reasons
> > not to do so.
>
> I think people just got lazy :)

Virtual device was probably the right thing given there isn't directly
any device we are controlling/programming. This driver is just built
on top of other subsystems (clock and regulator).

Rob
Danilo Krummrich July 16, 2024, 10:33 p.m. UTC | #13
On Tue, Jul 16, 2024 at 09:53:15AM -0600, Rob Herring wrote:
> On Tue, Jul 16, 2024 at 9:22 AM Greg KH <greg@kroah.com> wrote:
> >
> > On Tue, Jul 16, 2024 at 05:15:25PM +0200, Danilo Krummrich wrote:
> > > On Thu, Jul 11, 2024 at 06:34:22PM +0200, Greg KH wrote:
> > > > On Thu, Jul 11, 2024 at 06:12:08PM +0200, Danilo Krummrich wrote:
> > > > > On Thu, Jul 11, 2024 at 04:37:50PM +0200, Greg KH wrote:
> > > > > > On Thu, Jul 11, 2024 at 03:21:31PM +0200, Danilo Krummrich wrote:
> > > > > > > (2) You require drivers to always implement a "dummy" struct platform_device,
> > > > > > > there is platform_device_register_simple() for that purpose.
> > > > > >
> > > > > > No, NEVER do that.  platform devices are only for real platform devices,
> > > > > > do not abuse that interface any more than it already is.
> > > > >
> > > > > I thought we're talking about cases like [1] or [2], but please correct me if
> > > > > those are considered abusing the platform bus as well.
> > > > >
> > > > > (Those drivers read the CPU OF nodes, instead of OF nodes that represent a
> > > > > separate device.)
> > > > >
> > > > > [1] https://elixir.bootlin.com/linux/latest/source/drivers/cpuidle/cpuidle-riscv-sbi.c#L586
> > > > > [2] https://elixir.bootlin.com/linux/latest/source/drivers/cpuidle/cpuidle-psci.c#L441
> > > >
> > > > Yes, these are abuses of that and should be virtual devices as they have
> > > > nothing to do with the platform bus.
> > >
> > > For those drivers, wouldn't it be better if proper devices would be derived from
> > > the CPU OF nodes directly? This seems to be a common problem for cpuidle and
> > > cpufreq drivers.
> >
> > Yes they should.
> 
> Well, which one do we bind? The cpufreq driver or cpuidle driver? Or
> there's the thermal f/w throttling as well. It's messy. Also, the CPUs
> already have a struct device associated with them for the topology
> stuff, but no driver IIRC.

I did not mean to say that they should bind to the CPU nodes compatible string,
but rather have devices populated from sub-nodes of the CPU / CPU cluster nodes
or from the SoC's 'simple-bus' or whatever makes sense for the specific HW.

Generally, I think there should be something in the DT that populates the
corresponding device, rather than having a virtual device. The device isn't
really virtual, it controls some real HW.

> 
> Another complication is it is not the CPU that determines what
> cpufreq/cpuidle drivers to use, but a platform decision. That decision
> may evolve as well which means it can't be driven from the DT.

Often it's SoC specific, but that should be fine, right? Or do you mean
something else?

> 
> > > But it's quite a while ago I dealt with such drivers, maybe there are reasons
> > > not to do so.
> >
> > I think people just got lazy :)
> 
> Virtual device was probably the right thing given there isn't directly
> any device we are controlling/programming. This driver is just built
> on top of other subsystems (clock and regulator).

The two examples I gave use a firmware interface to control the CPU's idle
state.

But even for the case you mention here, I'd still argue that the driver controls
some real hardware, just not directly. It controls the semantics and is still HW
specific.

Having a dedicated DT node also makes it easy to provide the resources, e.g.
regulators and clocks.

- Danilo

> 
> Rob
>
diff mbox series

Patch

diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
index 94e55c40970a..eb9359bd3c5c 100644
--- a/drivers/cpufreq/Kconfig
+++ b/drivers/cpufreq/Kconfig
@@ -217,6 +217,18 @@  config CPUFREQ_DT
 
 	  If in doubt, say N.
 
+config CPUFREQ_DT_RUST
+	tristate "Rust based Generic DT based cpufreq driver"
+	depends on HAVE_CLK && OF && RUST
+	select CPUFREQ_DT_PLATDEV
+	select PM_OPP
+	help
+	  This adds a Rust based generic DT based cpufreq driver for frequency
+	  management.  It supports both uniprocessor (UP) and symmetric
+	  multiprocessor (SMP) systems.
+
+	  If in doubt, say N.
+
 config CPUFREQ_DT_PLATDEV
 	tristate "Generic DT based cpufreq platdev driver"
 	depends on OF
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 8d141c71b016..4981d908b803 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -15,6 +15,7 @@  obj-$(CONFIG_CPU_FREQ_GOV_COMMON)		+= cpufreq_governor.o
 obj-$(CONFIG_CPU_FREQ_GOV_ATTR_SET)	+= cpufreq_governor_attr_set.o
 
 obj-$(CONFIG_CPUFREQ_DT)		+= cpufreq-dt.o
+obj-$(CONFIG_CPUFREQ_DT_RUST)		+= rcpufreq_dt.o
 obj-$(CONFIG_CPUFREQ_DT_PLATDEV)	+= cpufreq-dt-platdev.o
 
 # Traces
diff --git a/drivers/cpufreq/rcpufreq_dt.rs b/drivers/cpufreq/rcpufreq_dt.rs
new file mode 100644
index 000000000000..3e86ad134e13
--- /dev/null
+++ b/drivers/cpufreq/rcpufreq_dt.rs
@@ -0,0 +1,222 @@ 
+// SPDX-License-Identifier: GPL-2.0
+
+//! Rust based implementation of the cpufreq-dt driver.
+
+use core::format_args;
+
+use kernel::{
+    b_str, c_str, clk, cpufreq, cpumask::Cpumask, define_of_id_table, device::Device,
+    devres::Devres, error::code::*, fmt, macros::vtable, module_platform_driver, of, opp, platform,
+    prelude::*, str::CString, sync::Arc,
+};
+
+// Finds exact supply name from the OF node.
+fn find_supply_name_exact(np: &of::DeviceNode, name: &str) -> Option<CString> {
+    let name_cstr = CString::try_from_fmt(fmt!("{}-supply", name)).ok()?;
+
+    np.find_property(&name_cstr).ok()?;
+    CString::try_from_fmt(fmt!("{}", name)).ok()
+}
+
+// Finds supply name for the CPU from DT.
+fn find_supply_names(dev: &Device, cpu: u32) -> Option<Vec<CString>> {
+    let np = of::DeviceNode::from_dev(dev).ok()?;
+
+    // Try "cpu0" for older DTs.
+    let name = match cpu {
+        0 => find_supply_name_exact(&np, "cpu0"),
+        _ => None,
+    }
+    .or(find_supply_name_exact(&np, "cpu"))?;
+
+    let mut list = Vec::with_capacity(1, GFP_KERNEL).ok()?;
+    list.push(name, GFP_KERNEL).ok()?;
+
+    Some(list)
+}
+
+// Represents the cpufreq dt device.
+struct CPUFreqDTDevice {
+    opp_table: opp::Table,
+    freq_table: opp::FreqTable,
+    #[allow(dead_code)]
+    mask: Cpumask,
+    #[allow(dead_code)]
+    token: Option<opp::ConfigToken>,
+    #[allow(dead_code)]
+    clk: clk::Clk,
+}
+
+struct CPUFreqDTDriver;
+
+#[vtable]
+impl opp::ConfigOps for CPUFreqDTDriver {}
+
+#[vtable]
+impl cpufreq::Driver for CPUFreqDTDriver {
+    type Data = ();
+    type PData = Arc<CPUFreqDTDevice>;
+
+    fn init(policy: &mut cpufreq::Policy) -> Result<Self::PData> {
+        let cpu = policy.cpu();
+        let dev = Device::from_cpu(cpu)?;
+        let mut mask = Cpumask::new()?;
+
+        mask.set(cpu);
+
+        let token = match find_supply_names(&dev, cpu) {
+            Some(names) => Some(
+                opp::Config::<Self>::new()
+                    .set_regulator_names(names)?
+                    .set(&dev)?,
+            ),
+            _ => None,
+        };
+
+        // Get OPP-sharing information from "operating-points-v2" bindings.
+        let fallback = match opp::Table::of_sharing_cpus(&dev, &mut mask) {
+            Ok(_) => false,
+            Err(e) => {
+                if e != ENOENT {
+                    return Err(e);
+                }
+
+                // "operating-points-v2" not supported. If the platform hasn't
+                // set sharing CPUs, fallback to all CPUs share the `Policy`
+                // for backward compatibility.
+                opp::Table::sharing_cpus(&dev, &mut mask).is_err()
+            }
+        };
+
+        // Initialize OPP tables for all policy cpus.
+        //
+        // For platforms not using "operating-points-v2" bindings, we do this
+        // before updating policy cpus. Otherwise, we will end up creating
+        // duplicate OPPs for the CPUs.
+        //
+        // OPPs might be populated at runtime, don't fail for error here unless
+        // it is -EPROBE_DEFER.
+        let mut opp_table = match opp::Table::from_of_cpumask(&dev, &mask) {
+            Ok(table) => table,
+            Err(e) => {
+                if e == EPROBE_DEFER {
+                    return Err(e);
+                }
+
+                // The table is added dynamically ?
+                opp::Table::from_dev(&dev)?
+            }
+        };
+
+        // The OPP table must be initialized, statically or dynamically, by this point.
+        opp_table.opp_count()?;
+
+        // Set sharing cpus for fallback scenario.
+        if fallback {
+            mask.set_all();
+            opp_table.set_sharing_cpus(&mask)?;
+        }
+
+        let mut transition_latency = opp_table.max_transition_latency() as u32;
+        if transition_latency == 0 {
+            transition_latency = cpufreq::ETERNAL_LATENCY;
+        }
+
+        let freq_table = opp_table.to_cpufreq_table()?;
+        let clk = policy
+            .set_freq_table(freq_table.table())
+            .set_dvfs_possible_from_any_cpu()
+            .set_suspend_freq((opp_table.suspend_freq() / 1000) as u32)
+            .set_transition_latency(transition_latency)
+            .set_clk(&dev, None)?;
+
+        mask.copy(policy.cpus());
+
+        Ok(Arc::new(
+            CPUFreqDTDevice {
+                opp_table,
+                freq_table,
+                mask,
+                token,
+                clk,
+            },
+            GFP_KERNEL,
+        )?)
+    }
+
+    fn exit(_policy: &mut cpufreq::Policy, _data: Option<Self::PData>) -> Result<()> {
+        Ok(())
+    }
+
+    fn online(_policy: &mut cpufreq::Policy) -> Result<()> {
+        // We did light-weight tear down earlier, nothing to do here.
+        Ok(())
+    }
+
+    fn offline(_policy: &mut cpufreq::Policy) -> Result<()> {
+        // Preserve policy->data and don't free resources on light-weight
+        // tear down.
+        Ok(())
+    }
+
+    fn suspend(policy: &mut cpufreq::Policy) -> Result<()> {
+        policy.generic_suspend()
+    }
+
+    fn verify(data: &mut cpufreq::PolicyData) -> Result<()> {
+        data.generic_verify()
+    }
+
+    fn target_index(policy: &mut cpufreq::Policy, index: u32) -> Result<()> {
+        let data = match policy.data::<Self::PData>() {
+            Some(data) => data,
+            None => return Err(ENOENT),
+        };
+
+        let freq = data.freq_table.freq(index.try_into().unwrap())? as u64;
+        data.opp_table.set_rate(freq * 1000)
+    }
+
+    fn get(policy: &mut cpufreq::Policy) -> Result<u32> {
+        policy.generic_get()
+    }
+
+    fn set_boost(_policy: &mut cpufreq::Policy, _state: i32) -> Result<()> {
+        Ok(())
+    }
+
+    fn register_em(policy: &mut cpufreq::Policy) {
+        policy.register_em_opp()
+    }
+}
+
+impl platform::Driver for CPUFreqDTDriver {
+    type Data = ();
+
+    define_of_id_table! {(), [
+        (of::DeviceId(b_str!("operating-points-v2")), None),
+    ]}
+
+    fn probe(dev: &mut platform::Device, _id_info: Option<&Self::IdInfo>) -> Result<Self::Data> {
+        let drv = cpufreq::Registration::<CPUFreqDTDriver>::register(
+            c_str!("cpufreq-dt"),
+            (),
+            cpufreq::flags::NEED_INITIAL_FREQ_CHECK | cpufreq::flags::IS_COOLING_DEV,
+            true,
+        )?;
+
+        Devres::new_foreign_owned(dev.as_ref(), drv, GFP_KERNEL)?;
+
+        pr_info!("CPUFreq DT driver registered\n");
+
+        Ok(())
+    }
+}
+
+module_platform_driver! {
+    type: CPUFreqDTDriver,
+    name: "cpufreq_dt",
+    author: "Viresh Kumar <viresh.kumar@linaro.org>",
+    description: "Generic CPUFreq DT driver",
+    license: "GPL v2",
+}