Re: [PATCH v2 2/7] dt-bindings: clock: renesas,r9a06g032-sysctrl: Add h2mode property

[Miquel Raynal]

Hi Krzysztof,

krzysztof.kozlowski@xxxxxxxxxx wrote on Thu, 24 Nov 2022 10:46:14 +0100:

> On 24/11/2022 10:36, Miquel Raynal wrote:
> > Hi Krzysztof,
> > 
> > krzysztof.kozlowski@xxxxxxxxxx wrote on Wed, 23 Nov 2022 10:39:41 +0100:
> >   
> >> On 22/11/2022 11:47, Geert Uytterhoeven wrote:  
> >>> Hi Krzysztof,
> >>>
> >>> On Tue, Nov 22, 2022 at 11:30 AM Krzysztof Kozlowski
> >>> <krzysztof.kozlowski@xxxxxxxxxx> wrote:    
> >>>> On 22/11/2022 10:07, Herve Codina wrote:    
> >>>>> On Tue, 22 Nov 2022 09:42:48 +0100
> >>>>> Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> wrote:
> >>>>>    
> >>>>>> On 22/11/2022 09:25, Geert Uytterhoeven wrote:    
> >>>>>>> Hi Krzysztof,
> >>>>>>>
> >>>>>>> On Tue, Nov 22, 2022 at 8:45 AM Krzysztof Kozlowski
> >>>>>>> <krzysztof.kozlowski@xxxxxxxxxx> wrote:    
> >>>>>>>> On 21/11/2022 21:46, Geert Uytterhoeven wrote:    
> >>>>>>>>>> This does not change anything. Herve wrote:
> >>>>>>>>>>    
> >>>>>>>>>>> probe some devices (USB host and probably others)    
> >>>>>>>>>>
> >>>>>>>>>> Why some can be probed earlier and some not, if there are no
> >>>>>>>>>> dependencies? If there are dependencies, it's the same case with sysctrl
> >>>>>>>>>> touching the register bit and the USB controller touching it (as well
> >>>>>>>>>> via syscon, but that's obvious, I assume).
> >>>>>>>>>>
> >>>>>>>>>> Where is the synchronization problem?    
> >>>>>>>>>
> >>>>>>>>> The h2mode bit (and probably a few other controls we haven't figured out
> >>>>>>>>> yet) in the sysctrl must be set before any of the USB devices is active.
> >>>>>>>>> Hence it's safest for the sysctrl to do this before any of the USB drivers
> >>>>>>>>> probes.    
> >>>>>>>>
> >>>>>>>> Again, this does not differ from many, many of other devices. All of
> >>>>>>>> them must set something in system controller block, before they start
> >>>>>>>> operating (or at specific time). It's exactly the same everywhere.    
> >>>>>>>
> >>>>>>> The issue here is that there are two _different drivers_ (USB host
> >>>>>>> and device). When both are modular, and the driver that depends on the
> >>>>>>> sysctrl setting is loaded second, you have a problem: the sysctrl change
> >>>>>>> must not be done when the first driver is already using the hardware.
> >>>>>>>
> >>>>>>> Hence the sysctrl driver should take care of it itself during early
> >>>>>>> initialization (it's the main clock controller, so it's a dependency
> >>>>>>> for all other I/O device drivers).    
> >>>>>>
> >>>>>> I assumed you have there bit for the first device (which can switch
> >>>>>> between USB host and USB device) to choose appropriate mode. The
> >>>>>> bindings also expressed this - "the USBs are". Never said anything about
> >>>>>> dependency between these USBs.
> >>>>>>
> >>>>>> Are you saying that the mode for first device cannot be changed once the
> >>>>>> second device (which is only host) is started? IOW, the mode setup must
> >>>>>> happen before any of these devices are started?
> >>>>>>
> >>>>>> Anyway with sysctrl approach you will have dependency and you cannot
> >>>>>> rely on clock provider-consumer relationship to order that dependency.
> >>>>>> What if you make all clocks on and do not take any clocks in USB device?
> >>>>>> Broken dependency. What if you want to use this in a different SoC,
> >>>>>> where the sysctrl does not provide clocks? Broken dependency.    
> >>>>>
> >>>>> The issue is really related to the Renesas sysctrl itself and not related
> >>>>> to the USB drivers themselves.
> >>>>> From the drivers themselves, the issue is not seen (I mean the driver
> >>>>> takes no specific action related to this issue).
> >>>>> If we change the SOC, the issue will probably not exist anymore.    
> >>>>
> >>>> Yeah, and in the next SoC you will bring 10 of such properties to
> >>>> sysctrl arguing that if one was approved, 10 is also fine. Somehow
> >>>> people on the lists like to use that argument - I saw it somewhere, so I
> >>>> am allowed to do here the same.    
> >>>
> >>> Like pin control properties? ;-)
> >>> This property represents a wiring on the board...
> >>> I.e. a system integration issue.
> >>>     
> >>>> I understand that the registers responsible for configuration are in
> >>>> sysctrl block, but it does not mean that it should be described as part
> >>>> of sysctrl Devicetree node. If there was no synchronization problem,
> >>>> this would be regular example of register in syscon which is handled
> >>>> (toggled) by the device (so USB device/host controller). Since there is
> >>>> synchronization problem, you argue that it is correct representation of
> >>>> hardware. No, it is not, because logically in DT you do not describe
> >>>> mode or existence of other devices in some other node and it still does
> >>>> not describe this ordering.    
> >>>
> >>> So we have to drop the property, and let the sysctrl block look
> >>> for <name>@<reg> nodes, and check which ones are enabled?
> >>>
> >>> Running out of ideas...    
> > 
> > I'm stepping in, hopefully I won't just be bikeshedding on something
> > that has already been discussed but here is my grain of salt.
> >   
> >> One solution could be making USB nodes children of the sysctrl block which:
> >> 1. Gives proper ordering (children cannot start before parent)
> >> regardless of any other shared resources,
> >> 2. Allows to drop this mode property and instead check what type of
> >> children you have and configure mode depending on them.
> >>
> >> However this also might not be correct representation of hardware
> >> (dunno...), so I am also running out of ideas.  
> > 
> > I see what you mean here, but AFAICS that is clearly a wrong
> > representation of the hardware. Sorting nodes by bus seems the aim of
> > device tree because there is a physical relationship, that's why we
> > have (i2c as an example):
> > 
> > 	ahb {
> > 		foo-controller@xxx {
> > 			reg = <xxx>;
> > 		};
> > 	};
> > 
> > But what you are describing now is conceptually closer to:
> > 
> > 	clk-controller {
> > 		foo-controller {
> > 			reg = ?
> > 		};
> > 	};  
> 
> Which is not a problem. reg can be anything - offset from sysctrl node
> or absolute offset. We have it in many places already. What's the issue
> here?
>
> > Not mentioning that this only works once, because foo-controller might
> > also need other blocks to be ready before probing and those might
> > be different blocks (they are the same in the rzn1 case, but
> > more generally, they are not).  
> 
> But what is the problem of needing other blocks? All devices need
> something and we solve it...

What I am saying is that parenting only works once. All the other
dependencies must be described by properties.

The h2mode register, no matter its content, should be set early in the
boot process, at least before any of the concerned controllers, which
are totally independent hardware blocks, probe. If one of them has
started, a change to the h2mode property could just stall the system.
The USB controllers do not *need* this property nor want to change it
(see below).

The fact that the USB controllers are totally independent hardware
blocks make me thing that they should *not* be children of the sysctrl.
In our case one of them even is a PCI device! Would you represent a PCI
device within the sysctrl node? The other is somehow memory mapped
behind a bridge. Again, this has to be described somewhere, and
parenting usually is the right fit for that.

Hence, the only real thing that remains to be described, as you
rightly pointed out in your earlier reviews, is the probe order which
is nothing related to any kind of parenting in this case.

> > So in the end I am not in favor of this
> > solution.
> > 
> > If we compare the dependency between the USB device controller and the
> > sysctrl block which contains the h2mode register to existing
> > dependencies, they are all treated with properties. These properties,
> > eg:
> > 
> > 	foo-controller {
> > 		clocks = <&provider [index]>;
> > 	};
> > 
> > were initially used to just tell the consumer which resource it should
> > grab/enable. If the device was not yet ready, we would rely on the
> > probe deferral mechanism to try again later. Not optimal, but not
> > bad either as it made things work. Since v5.11 and the addition of
> > automatic device links, the probe order is explicitly ordered.
> > <provider> could always get probed before <foo-controller>. So, isn't
> > what we need here? What about the following:
> > 
> > 	sysctrl {
> > 		h2mode = "something";
> > 	};
> > 
> > 	usb-device {
> > 		h2mode-provider = <&sysctrl>;
> > 	};  
> 
> No, because next time one will add 10 of such properties:
> sysctrl {
> 	h2mode = ""
> 	g2mode = ""
> 	i2mode = ""
> 	....
> }
> 
> and keep arguing that because these registers are in sysctrl, so they
> should have their own property in sysctrl mode.
> 
> That's not correct representation of hardware.

Actually my main focus here was more on the "sysctrl-provider" logic.
We need a probe dependency so we have two choices:
- pointing
- parenting
For the reasons above, I bet the former is the most accurate approach.

If the h2mode property bothers you, it's fine, we can just drop it. The
USB device controller can do without it:
- either it just probes without knowing the mode, its bus will remain
  empty so the device is useless, but nothing will break.
- or (this is my favorite) we add another sysctrl helper that exposes
  the h2mode, very much like we've done with the dmamux [2] and we just
  avoid probing if the mode that we receive does not ask for a USB
  device controller. Speeds-up the boot process.

[2] https://lore.kernel.org/all/20220427095653.91804-5-miquel.raynal@xxxxxxxxxxx/

Either ways, we would still need the probe order to be enforced,
which might be achieved on Linux side with the below explanations.

> > We can initially just make this work with some additional logic on both
> > sides. The USB device controller would manually check whether sysctrl
> > has been probed or not (in practice, because of the clocks and power
> > domains being described this will always be a yes, but IIUC we want to
> > avoid relying on it) and otherwise, defer its probe. On the sysctrl side
> > it is just a matter of checking (like we already do):
> > 
> > 	if (!sysctrl_priv)
> > 		return -EPROBE_DEFER;
> > 
> > To be honest I would love to see the device link mechanism extended to
> > "custom" phandle properties like that, it would avoid the burden of
> > checking for deferrals manually, aside with boot time improvements. If
> > we go this way, we shall decide whether we want to:
> > * extend the list of properties that will lead to a dependency creation [1]
> > * or maybe settle on a common suffix that could always be used,
> >   especially for specific cases like this one where there is an
> >   explicit provider-consumer dependency that must be fulfilled:
> > 
> > 	DEFINE_SUFFIX_PROP(provider, "-provider", "#provider-cells")
> > 
> > * or perhaps extend struct of_device_id to contain the name of the
> >   properties pointing to phandles that describe probe dependencies with:
> > 
> > 	char *provider_prop_name;
> > 	char *provider_cells_prop_name;
> > 
> >   and use them from of/property.c to generate the links when relevant.
> > 
> > [1] https://elixir.bootlin.com/linux/v6.0/source/drivers/of/property.c#L1298
> > 
> > 
> > Thanks,
> > Miquèl  
> 
> Best regards,
> Krzysztof
> 

Thanks,
Miquèl