Re: [PATCH v8 06/34] dt-bindings: clock: tegra-car: Document new tegra-clocks sub-node

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3850 bytes --]

On Wed, Aug 18, 2021 at 07:57:04PM +0300, Dmitry Osipenko wrote:
> 18.08.2021 19:39, Thierry Reding пишет:
> >> We don't have a platform device for CaR. I don't see how it's going to
> >> work. We need to create a platform device for each RPM-capable clock
> >> because that's how RPM works. The compatible string is required for
> >> instantiating OF-devices from a node, otherwise we will have to
> >> re-invent the OF core.
> > I think we do have a platform device for CAR. It's just not bound
> > against by the driver because these clock drivers are "special". But
> > from other parts of the series you're already trying to fix that, at
> > least partially.
> > 
> > But it doesn't seem right to create a platform device for each RPM-
> > capable clock. Why do they need to be devices? They aren't, so why
> > pretend? Is it that some API that we want to use here requires the
> > struct device?
> 
> The "device" representation is internal to the kernel. It's okay to me
> to have PLLs represented by a device, it's a distinct h/w by itself.
> 
> CCF supports managing of clock's RPM and it requires to have clock to be
> backed by a device. That's what we are using here.
> 
> Please see
> https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/clk/clk.c#L109

Looking at the implementation of __clk_register() and where that device
pointer typically comes from, I don't think the way this is used here is
what was intended. The way I interpret the code is that a clock is
registered with a parent device (i.e. its provider) and
clk_pm_runtime_get() is then used internally as a way to make sure that
when a clock is prepared, it's parent device is runtime resumed. This is
presumably to ensure that any registers that the driver might need to
access in order to prepare and enable the clock are accessible (i.e. the
CAR is not powered off or in reset).

So the struct device that is passed to __clk_register() (or its callers)
should be that of the CAR rather than virtual struct devices created by
the CAR.

And it's a bit debatable whether or not PLLs represent distinct
hardware. Ultimately every transistor on a chip could be considered
distinct hardware. But a platform device is a device on a platform bus,
which is really just another way of saying it's a hardware block that's
accessible from the CPU via a memory-mapped address. A PLL (just like
other clocks) is merely a resource exposed by means of access to these
registers. So I don't think they should be platform devices. Even making
them struct device:s seems a bit of a stretch.

Is there any reason why struct clk can't be used for this? I mean, the
whole purpose of that structure is to represent clocks. Why do we need
to make them special?

> >>> Also, I don't think the tegra- prefix is necessary here. The parent node
> >>> is already identified as Tegra via the compatible string.
> >>>
> >>> In the case of CAR, I'd imagine something like:
> >>>
> >>> 	clocks {
> >>> 		sclk {
> >>> 			operating-points-v2 = <&opp_table>;
> >>> 			power-domains = <&domain>;
> >>> 		};
> >>> 	};
> >>>
> >>> Now you've only got the bare minimum in here that you actually add. All
> >>> the other data that you used to have is simply derived from the parent.
> >> 'clocks' is already a generic keyword in DT. It's probably not okay to
> >> redefine it.
> > "clocks" is not a generic keyword. It's the name of a property and given
> > that we're talking about the clock provider here, it doesn't need a
> > clocks property of its own, so it should be fine to use that for the
> > node.
> 
> I'm curious what Rob thinks about it. Rob, does this sound okay to you?

Another alternative would be to omit that level altogether and just make
sclk and siblings direct children of the CAR node.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]