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

[Danilo Krummrich <dakr@kernel.org>]

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
>