| Message ID | 20221114111513.1436165-3-herve.codina@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add the Renesas USBF controller support | expand |
On Mon, Nov 14, 2022 at 12:15 PM Herve Codina <herve.codina@bootlin.com> wrote: > Add the h2mode property to force the USBs mode ie: > - 2 hosts > or > - 1 host and 1 device > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 14/11/2022 12:15, Herve Codina wrote: > Add the h2mode property to force the USBs mode ie: > - 2 hosts > or > - 1 host and 1 device > > Signed-off-by: Herve Codina <herve.codina@bootlin.com> > --- > .../bindings/clock/renesas,r9a06g032-sysctrl.yaml | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml > index 95bf485c6cec..f9e0a58aa4fb 100644 > --- a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml > +++ b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml > @@ -39,6 +39,16 @@ properties: > '#power-domain-cells': > const: 0 > > + renesas,h2mode: > + description: | > + Configure the USBs mode. > + - <0> : the USBs are in 1 host and 1 device mode. > + - <1> : the USBs are in 2 host mode. > + If the property is not present, the value used is the one already present > + in the CFG_USB register (from reset or set by the bootloader). > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [0, 1] 0/1 are quite cryptic. Why not making it a string which is easy to read and understand? Can be something like "two-hosts" and "one-host". Or anything you find more readable... Best regards, Krzysztof
On 15/11/2022 14:05, Krzysztof Kozlowski wrote: > On 14/11/2022 12:15, Herve Codina wrote: >> Add the h2mode property to force the USBs mode ie: >> - 2 hosts >> or >> - 1 host and 1 device >> >> Signed-off-by: Herve Codina <herve.codina@bootlin.com> >> --- >> .../bindings/clock/renesas,r9a06g032-sysctrl.yaml | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml >> index 95bf485c6cec..f9e0a58aa4fb 100644 >> --- a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml >> +++ b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml >> @@ -39,6 +39,16 @@ properties: >> '#power-domain-cells': >> const: 0 >> >> + renesas,h2mode: >> + description: | >> + Configure the USBs mode. >> + - <0> : the USBs are in 1 host and 1 device mode. >> + - <1> : the USBs are in 2 host mode. >> + If the property is not present, the value used is the one already present >> + in the CFG_USB register (from reset or set by the bootloader). >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + enum: [0, 1] > > 0/1 are quite cryptic. Why not making it a string which is easy to read > and understand? Can be something like "two-hosts" and "one-host". Or > anything you find more readable... ...but actually you should rather make it a property of your USB controller, not clock controller. You have two controllers and we have a generic property for them - dr_mode. Best regards, Krzysztof
Hi Krzysztof, On Tue, 15 Nov 2022 14:07:52 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 15/11/2022 14:05, Krzysztof Kozlowski wrote: > > On 14/11/2022 12:15, Herve Codina wrote: > >> Add the h2mode property to force the USBs mode ie: > >> - 2 hosts > >> or > >> - 1 host and 1 device > >> > >> Signed-off-by: Herve Codina <herve.codina@bootlin.com> > >> --- > >> .../bindings/clock/renesas,r9a06g032-sysctrl.yaml | 10 ++++++++++ > >> 1 file changed, 10 insertions(+) > >> > >> diff --git a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml > >> index 95bf485c6cec..f9e0a58aa4fb 100644 > >> --- a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml > >> +++ b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml > >> @@ -39,6 +39,16 @@ properties: > >> '#power-domain-cells': > >> const: 0 > >> > >> + renesas,h2mode: > >> + description: | > >> + Configure the USBs mode. > >> + - <0> : the USBs are in 1 host and 1 device mode. > >> + - <1> : the USBs are in 2 host mode. > >> + If the property is not present, the value used is the one already present > >> + in the CFG_USB register (from reset or set by the bootloader). > >> + $ref: /schemas/types.yaml#/definitions/uint32 > >> + enum: [0, 1] > > > > 0/1 are quite cryptic. Why not making it a string which is easy to read > > and understand? Can be something like "two-hosts" and "one-host". Or > > anything you find more readable... > > ...but actually you should rather make it a property of your USB > controller, not clock controller. You have two controllers and we have a > generic property for them - dr_mode. > > Best regards, > Krzysztof > IMHO, this property in the USB controllers does not make sense. Indeed each controller cannot have a different 'mode'. Some controllers are USB host only (EHCI and OHCI) and the USBF controller I worked on is device only. 'h2mode' allows to choose between host or device on one of the USB but not at the USB controller level. This property should be handle outside the USB controller nodes. Currently, this node (declared as a clock node) is in fact a sysctrl node and can do some configuration not related to clocks. I agree with you something related to choosing USB Host/Device in a clock node seems strange. Some discussion were already opened related to this property and how to handle it: https://lore.kernel.org/all/20221107182642.05a09f2f@bootlin.com/ https://lore.kernel.org/all/20221107173614.474707d7@bootlin.com/ Regards, Hervé
Hi Krzysztof, Geert, On Tue, 15 Nov 2022 15:04:17 +0100 Herve Codina <herve.codina@bootlin.com> wrote: > Hi Krzysztof, > > On Tue, 15 Nov 2022 14:07:52 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > > On 15/11/2022 14:05, Krzysztof Kozlowski wrote: > > > On 14/11/2022 12:15, Herve Codina wrote: > > >> Add the h2mode property to force the USBs mode ie: > > >> - 2 hosts > > >> or > > >> - 1 host and 1 device > > >> > > >> Signed-off-by: Herve Codina <herve.codina@bootlin.com> > > >> --- > > >> .../bindings/clock/renesas,r9a06g032-sysctrl.yaml | 10 ++++++++++ > > >> 1 file changed, 10 insertions(+) > > >> > > >> diff --git a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml > > >> index 95bf485c6cec..f9e0a58aa4fb 100644 > > >> --- a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml > > >> +++ b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml > > >> @@ -39,6 +39,16 @@ properties: > > >> '#power-domain-cells': > > >> const: 0 > > >> > > >> + renesas,h2mode: > > >> + description: | > > >> + Configure the USBs mode. > > >> + - <0> : the USBs are in 1 host and 1 device mode. > > >> + - <1> : the USBs are in 2 host mode. > > >> + If the property is not present, the value used is the one already present > > >> + in the CFG_USB register (from reset or set by the bootloader). > > >> + $ref: /schemas/types.yaml#/definitions/uint32 > > >> + enum: [0, 1] > > > > > > 0/1 are quite cryptic. Why not making it a string which is easy to read > > > and understand? Can be something like "two-hosts" and "one-host". Or > > > anything you find more readable... > > > > ...but actually you should rather make it a property of your USB > > controller, not clock controller. You have two controllers and we have a > > generic property for them - dr_mode. > > > > Best regards, > > Krzysztof > > > > IMHO, this property in the USB controllers does not make sense. > Indeed each controller cannot have a different 'mode'. > Some controllers are USB host only (EHCI and OHCI) and the USBF > controller I worked on is device only. > 'h2mode' allows to choose between host or device on one of the USB > but not at the USB controller level. > > This property should be handle outside the USB controller nodes. > > Currently, this node (declared as a clock node) is in fact a sysctrl > node and can do some configuration not related to clocks. > > I agree with you something related to choosing USB Host/Device in > a clock node seems strange. > > Some discussion were already opened related to this property and how > to handle it: > https://lore.kernel.org/all/20221107182642.05a09f2f@bootlin.com/ > https://lore.kernel.org/all/20221107173614.474707d7@bootlin.com/ > We advanced on this topic. First, even if 'renesas,r9a06g032-sysctrl.yaml' is present in the devicetree/bindings/clock/ directory, this node is really a 'system controller' node: - title: Renesas RZ/N1D (R9A06G032) System Controller - compatible: renesas,r9a06g032-sysctrl It handles clocks, power domains, some DMA routing, ... Now, the property 'h2mode' allows to choose between: - 2 USB hosts or - 1 USB host and 1 USB device. This switching is system wide and has no reason to be done in one specific USB controller. It can impact multiple devices and PLL settings. The 'renesas,r9a06g032-sysctrl' node, as the system control node of our system, is the best candidate to handle the property. In order to be less cryptic in the property value, what do you think about: renesas,h2mode: - one-dev : the USBs are in 1 host and 1 device mode. - only-hosts : the USBs are in 2 hosts mode. With these details and change on the property value, Is it ok for you to have the 'renesas,h2mode' property in the 'renesas,r9a06g032-sysctrl' node ? Regards, Hervé
On 18/11/2022 11:23, Herve Codina wrote: > Hi Krzysztof, Geert, > > On Tue, 15 Nov 2022 15:04:17 +0100 > Herve Codina <herve.codina@bootlin.com> wrote: > >> Hi Krzysztof, >> >> On Tue, 15 Nov 2022 14:07:52 +0100 >> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >> >>> On 15/11/2022 14:05, Krzysztof Kozlowski wrote: >>>> On 14/11/2022 12:15, Herve Codina wrote: >>>>> Add the h2mode property to force the USBs mode ie: >>>>> - 2 hosts >>>>> or >>>>> - 1 host and 1 device >>>>> >>>>> Signed-off-by: Herve Codina <herve.codina@bootlin.com> >>>>> --- >>>>> .../bindings/clock/renesas,r9a06g032-sysctrl.yaml | 10 ++++++++++ >>>>> 1 file changed, 10 insertions(+) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml >>>>> index 95bf485c6cec..f9e0a58aa4fb 100644 >>>>> --- a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml >>>>> +++ b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml >>>>> @@ -39,6 +39,16 @@ properties: >>>>> '#power-domain-cells': >>>>> const: 0 >>>>> >>>>> + renesas,h2mode: >>>>> + description: | >>>>> + Configure the USBs mode. >>>>> + - <0> : the USBs are in 1 host and 1 device mode. >>>>> + - <1> : the USBs are in 2 host mode. >>>>> + If the property is not present, the value used is the one already present >>>>> + in the CFG_USB register (from reset or set by the bootloader). >>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>> + enum: [0, 1] >>>> >>>> 0/1 are quite cryptic. Why not making it a string which is easy to read >>>> and understand? Can be something like "two-hosts" and "one-host". Or >>>> anything you find more readable... >>> >>> ...but actually you should rather make it a property of your USB >>> controller, not clock controller. You have two controllers and we have a >>> generic property for them - dr_mode. >>> >>> Best regards, >>> Krzysztof >>> >> >> IMHO, this property in the USB controllers does not make sense. >> Indeed each controller cannot have a different 'mode'. >> Some controllers are USB host only (EHCI and OHCI) and the USBF >> controller I worked on is device only. >> 'h2mode' allows to choose between host or device on one of the USB >> but not at the USB controller level. >> >> This property should be handle outside the USB controller nodes. >> >> Currently, this node (declared as a clock node) is in fact a sysctrl >> node and can do some configuration not related to clocks. >> >> I agree with you something related to choosing USB Host/Device in >> a clock node seems strange. >> >> Some discussion were already opened related to this property and how >> to handle it: >> https://lore.kernel.org/all/20221107182642.05a09f2f@bootlin.com/ >> https://lore.kernel.org/all/20221107173614.474707d7@bootlin.com/ >> > > We advanced on this topic. > > First, even if 'renesas,r9a06g032-sysctrl.yaml' is present in > the devicetree/bindings/clock/ directory, this node is really > a 'system controller' node: > - title: Renesas RZ/N1D (R9A06G032) System Controller > - compatible: renesas,r9a06g032-sysctrl > > It handles clocks, power domains, some DMA routing, ... > > Now, the property 'h2mode' allows to choose between: > - 2 USB hosts > or > - 1 USB host and 1 USB device. > > This switching is system wide and has no reason to be done in > one specific USB controller. It can impact multiple devices and > PLL settings. > > The 'renesas,r9a06g032-sysctrl' node, as the system control > node of our system, is the best candidate to handle the property. Not necessarily. IIUC, you have: 1. sysctrl with some register(s) for choosing device mode 2. usb device or host at one address 3. usb host at separate address If so then: A. Pretty often we have wrapper nodes for this purpose (USB, phy wrappers or glues) which are usually needed to configure something for a generic block (like Synopsys etc). B. Pretty often the device (so your USB host or device) needs to poke something in system controller registers, e.g. for power or some other setup. Your case looks a lot like (B). We have many, many of such examples already. Actually it is exactly like that, except that it affects possibility of another device (e.g. choosing USB device blocks having host there). C. It looks a bit like a multi-serial-protocol interfaces (so UART+I2C+SPI). The difference is that such cases have all these nodes defined as a children of the protocol-wrapping device. Not here. I would propose to go with (B) unless of course it's causes some crazy architecture/code choices. Why? Because with exception of (C) we should not define properties which represent DT node choices. IOW, Choosing a node and compatible (e.g. usb controller as device) is enough to describe the hardware. No need for other properties to control some register in other block. > > In order to be less cryptic in the property value, what do you > think about: > renesas,h2mode: > - one-dev : the USBs are in 1 host and 1 device mode. > - only-hosts : the USBs are in 2 hosts mode. Name looks better, if we go this path. > > With these details and change on the property value, > Is it ok for you to have the 'renesas,h2mode' property > in the 'renesas,r9a06g032-sysctrl' node ? Best regards, Krzysztof
Hi, On Mon, 21 Nov 2022 12:43:16 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 18/11/2022 11:23, Herve Codina wrote: > > Hi Krzysztof, Geert, > > > > On Tue, 15 Nov 2022 15:04:17 +0100 > > Herve Codina <herve.codina@bootlin.com> wrote: > > > >> Hi Krzysztof, > >> > >> On Tue, 15 Nov 2022 14:07:52 +0100 > >> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> > >>> On 15/11/2022 14:05, Krzysztof Kozlowski wrote: > >>>> On 14/11/2022 12:15, Herve Codina wrote: > >>>>> Add the h2mode property to force the USBs mode ie: > >>>>> - 2 hosts > >>>>> or > >>>>> - 1 host and 1 device > >>>>> > >>>>> Signed-off-by: Herve Codina <herve.codina@bootlin.com> > >>>>> --- > >>>>> .../bindings/clock/renesas,r9a06g032-sysctrl.yaml | 10 ++++++++++ > >>>>> 1 file changed, 10 insertions(+) > >>>>> > >>>>> diff --git a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml > >>>>> index 95bf485c6cec..f9e0a58aa4fb 100644 > >>>>> --- a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml > >>>>> +++ b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml > >>>>> @@ -39,6 +39,16 @@ properties: > >>>>> '#power-domain-cells': > >>>>> const: 0 > >>>>> > >>>>> + renesas,h2mode: > >>>>> + description: | > >>>>> + Configure the USBs mode. > >>>>> + - <0> : the USBs are in 1 host and 1 device mode. > >>>>> + - <1> : the USBs are in 2 host mode. > >>>>> + If the property is not present, the value used is the one already present > >>>>> + in the CFG_USB register (from reset or set by the bootloader). > >>>>> + $ref: /schemas/types.yaml#/definitions/uint32 > >>>>> + enum: [0, 1] > >>>> > >>>> 0/1 are quite cryptic. Why not making it a string which is easy to read > >>>> and understand? Can be something like "two-hosts" and "one-host". Or > >>>> anything you find more readable... > >>> > >>> ...but actually you should rather make it a property of your USB > >>> controller, not clock controller. You have two controllers and we have a > >>> generic property for them - dr_mode. > >>> > >>> Best regards, > >>> Krzysztof > >>> > >> > >> IMHO, this property in the USB controllers does not make sense. > >> Indeed each controller cannot have a different 'mode'. > >> Some controllers are USB host only (EHCI and OHCI) and the USBF > >> controller I worked on is device only. > >> 'h2mode' allows to choose between host or device on one of the USB > >> but not at the USB controller level. > >> > >> This property should be handle outside the USB controller nodes. > >> > >> Currently, this node (declared as a clock node) is in fact a sysctrl > >> node and can do some configuration not related to clocks. > >> > >> I agree with you something related to choosing USB Host/Device in > >> a clock node seems strange. > >> > >> Some discussion were already opened related to this property and how > >> to handle it: > >> https://lore.kernel.org/all/20221107182642.05a09f2f@bootlin.com/ > >> https://lore.kernel.org/all/20221107173614.474707d7@bootlin.com/ > >> > > > > We advanced on this topic. > > > > First, even if 'renesas,r9a06g032-sysctrl.yaml' is present in > > the devicetree/bindings/clock/ directory, this node is really > > a 'system controller' node: > > - title: Renesas RZ/N1D (R9A06G032) System Controller > > - compatible: renesas,r9a06g032-sysctrl > > > > It handles clocks, power domains, some DMA routing, ... > > > > Now, the property 'h2mode' allows to choose between: > > - 2 USB hosts > > or > > - 1 USB host and 1 USB device. > > > > This switching is system wide and has no reason to be done in > > one specific USB controller. It can impact multiple devices and > > PLL settings. > > > > The 'renesas,r9a06g032-sysctrl' node, as the system control > > node of our system, is the best candidate to handle the property. > > Not necessarily. IIUC, you have: > > 1. sysctrl with some register(s) for choosing device mode > 2. usb device or host at one address > 3. usb host at separate address > Just to clarify, usb device and host controller are not provided by the same IP. We have an USB host at some address range (PCI OHCI/EHCI USB host below a PCI bridge) and the USB device at some other address range (below a AHB to someting bridge). And I am not sure that only USB host or devices are affected by this property change. > If so then: > A. Pretty often we have wrapper nodes for this purpose (USB, phy > wrappers or glues) which are usually needed to configure something for a > generic block (like Synopsys etc). > > B. Pretty often the device (so your USB host or device) needs to poke > something in system controller registers, e.g. for power or some other > setup. And we did it for some items (clocks and power). > > Your case looks a lot like (B). We have many, many of such examples > already. Actually it is exactly like that, except that it affects > possibility of another device (e.g. choosing USB device blocks having > host there). > > C. It looks a bit like a multi-serial-protocol interfaces (so > UART+I2C+SPI). The difference is that such cases have all these nodes > defined as a children of the protocol-wrapping device. Not here. > > I would propose to go with (B) unless of course it's causes some crazy > architecture/code choices. Why? Because with exception of (C) we should > not define properties which represent DT node choices. IOW, Choosing a > node and compatible (e.g. usb controller as device) is enough to > describe the hardware. No need for other properties to control some > register in other block. The issue with h2mode is that it affects several devices and these devices should not be in a "running" state when the h2mode is changed. PCI devices (host controllers) itself are not described in the DT. They are automatically enumerated. Changing the property in USB device controller can leads to hang on other busses. Indeed, changing this property when a device affected by the property is running can lead to a bus hang. In order to do that from the USB device controller I need to synchronize the other devices to wait for this setting before running. 1) probe sysctrl without setting h2mode 2) probe some devices (USB host and probably others) Stop at some point and wait for the h2mode property setting. 3) probe usb device -> Set h2mode property 4) allow devices waiting for the property setting to continue. This synchronization seems pretty tricky and what to do if nobody set the property (USB device controller not present or status="disabled" for instance) ? Setting this property in sysctrl probe avoid the need for all of this synchronization: 1) probe sysctrl and set h2mode. 2) probe other devices (no need to wait for the setting as it is already done) The probing of the other devices (or the starting of they running state) is guaranteed as they all need some clocks and so cannot start without having the sysctrl node already probed. This sysctrl node handles the clocks. So, I think that this property must be set during sysctrl probe call. From the DT point of view, I can put the property in the USB device controller node but I have the feeling that, if I do that, I will do some ugly things at sysctrl probe such as parsing nodes other than the sysctrl one to find the property and set h2mode accordingly :( > > > > > > In order to be less cryptic in the property value, what do you > > think about: > > renesas,h2mode: > > - one-dev : the USBs are in 1 host and 1 device mode. > > - only-hosts : the USBs are in 2 hosts mode. > > Name looks better, if we go this path. Ok, I will take care if we go this path. > > > > > With these details and change on the property value, > > Is it ok for you to have the 'renesas,h2mode' property > > in the 'renesas,r9a06g032-sysctrl' node ? > > Regards, Hervé
On 21/11/2022 16:59, Herve Codina wrote: > Hi, > > On Mon, 21 Nov 2022 12:43:16 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> On 18/11/2022 11:23, Herve Codina wrote: >>> Hi Krzysztof, Geert, >>> >>> On Tue, 15 Nov 2022 15:04:17 +0100 >>> Herve Codina <herve.codina@bootlin.com> wrote: >>> >>>> Hi Krzysztof, >>>> >>>> On Tue, 15 Nov 2022 14:07:52 +0100 >>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>>> >>>>> On 15/11/2022 14:05, Krzysztof Kozlowski wrote: >>>>>> On 14/11/2022 12:15, Herve Codina wrote: >>>>>>> Add the h2mode property to force the USBs mode ie: >>>>>>> - 2 hosts >>>>>>> or >>>>>>> - 1 host and 1 device >>>>>>> >>>>>>> Signed-off-by: Herve Codina <herve.codina@bootlin.com> >>>>>>> --- >>>>>>> .../bindings/clock/renesas,r9a06g032-sysctrl.yaml | 10 ++++++++++ >>>>>>> 1 file changed, 10 insertions(+) >>>>>>> >>>>>>> diff --git a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml >>>>>>> index 95bf485c6cec..f9e0a58aa4fb 100644 >>>>>>> --- a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml >>>>>>> +++ b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml >>>>>>> @@ -39,6 +39,16 @@ properties: >>>>>>> '#power-domain-cells': >>>>>>> const: 0 >>>>>>> >>>>>>> + renesas,h2mode: >>>>>>> + description: | >>>>>>> + Configure the USBs mode. >>>>>>> + - <0> : the USBs are in 1 host and 1 device mode. >>>>>>> + - <1> : the USBs are in 2 host mode. >>>>>>> + If the property is not present, the value used is the one already present >>>>>>> + in the CFG_USB register (from reset or set by the bootloader). >>>>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>>>> + enum: [0, 1] >>>>>> >>>>>> 0/1 are quite cryptic. Why not making it a string which is easy to read >>>>>> and understand? Can be something like "two-hosts" and "one-host". Or >>>>>> anything you find more readable... >>>>> >>>>> ...but actually you should rather make it a property of your USB >>>>> controller, not clock controller. You have two controllers and we have a >>>>> generic property for them - dr_mode. >>>>> >>>>> Best regards, >>>>> Krzysztof >>>>> >>>> >>>> IMHO, this property in the USB controllers does not make sense. >>>> Indeed each controller cannot have a different 'mode'. >>>> Some controllers are USB host only (EHCI and OHCI) and the USBF >>>> controller I worked on is device only. >>>> 'h2mode' allows to choose between host or device on one of the USB >>>> but not at the USB controller level. >>>> >>>> This property should be handle outside the USB controller nodes. >>>> >>>> Currently, this node (declared as a clock node) is in fact a sysctrl >>>> node and can do some configuration not related to clocks. >>>> >>>> I agree with you something related to choosing USB Host/Device in >>>> a clock node seems strange. >>>> >>>> Some discussion were already opened related to this property and how >>>> to handle it: >>>> https://lore.kernel.org/all/20221107182642.05a09f2f@bootlin.com/ >>>> https://lore.kernel.org/all/20221107173614.474707d7@bootlin.com/ >>>> >>> >>> We advanced on this topic. >>> >>> First, even if 'renesas,r9a06g032-sysctrl.yaml' is present in >>> the devicetree/bindings/clock/ directory, this node is really >>> a 'system controller' node: >>> - title: Renesas RZ/N1D (R9A06G032) System Controller >>> - compatible: renesas,r9a06g032-sysctrl >>> >>> It handles clocks, power domains, some DMA routing, ... >>> >>> Now, the property 'h2mode' allows to choose between: >>> - 2 USB hosts >>> or >>> - 1 USB host and 1 USB device. >>> >>> This switching is system wide and has no reason to be done in >>> one specific USB controller. It can impact multiple devices and >>> PLL settings. >>> >>> The 'renesas,r9a06g032-sysctrl' node, as the system control >>> node of our system, is the best candidate to handle the property. >> >> Not necessarily. IIUC, you have: >> >> 1. sysctrl with some register(s) for choosing device mode >> 2. usb device or host at one address >> 3. usb host at separate address >> > > Just to clarify, usb device and host controller are not provided by > the same IP. > We have an USB host at some address range (PCI OHCI/EHCI USB host > below a PCI bridge) and the USB device at some other address range > (below a AHB to someting bridge). > And I am not sure that only USB host or devices are affected by this > property change. > >> If so then: >> A. Pretty often we have wrapper nodes for this purpose (USB, phy >> wrappers or glues) which are usually needed to configure something for a >> generic block (like Synopsys etc). >> >> B. Pretty often the device (so your USB host or device) needs to poke >> something in system controller registers, e.g. for power or some other >> setup. > > And we did it for some items (clocks and power). > >> >> Your case looks a lot like (B). We have many, many of such examples >> already. Actually it is exactly like that, except that it affects >> possibility of another device (e.g. choosing USB device blocks having >> host there). >> >> C. It looks a bit like a multi-serial-protocol interfaces (so >> UART+I2C+SPI). The difference is that such cases have all these nodes >> defined as a children of the protocol-wrapping device. Not here. >> >> I would propose to go with (B) unless of course it's causes some crazy >> architecture/code choices. Why? Because with exception of (C) we should >> not define properties which represent DT node choices. IOW, Choosing a >> node and compatible (e.g. usb controller as device) is enough to >> describe the hardware. No need for other properties to control some >> register in other block. > > The issue with h2mode is that it affects several devices and these > devices should not be in a "running" state when the h2mode is changed. Why the change should happen when device is running? And why this should be anyway different than your existing hsmode property - it also will happen when system and device are running. > PCI devices (host controllers) itself are not described in the DT. They > are automatically enumerated. Aren't we talking about USB controller in a MMIO-based SoC? > Changing the property in USB device controller can leads to hang on > other busses. Indeed, changing this property when a device affected > by the property is running can lead to a bus hang.> > In order to do that from the USB device controller I need to synchronize > the other devices to wait for this setting before running. > 1) probe sysctrl without setting h2mode > 2) probe some devices (USB host and probably others) > Stop at some point and wait for the h2mode property setting. Why do you need to wait? Which device needs to wait? There are no such devices... if they are then please bring entire DTS, not some pieces in this patchset. > 3) probe usb device -> Set h2mode property > 4) allow devices waiting for the property setting to continue. I don't get why do you need such order. Your sysctrl also probes any time so old solution has exactly the same problem, doesn't it? > This synchronization seems pretty tricky and what to do if nobody > set the property (USB device controller not present or status="disabled" > for instance) ? > > Setting this property in sysctrl probe avoid the need for all of this > synchronization: > 1) probe sysctrl and set h2mode. > 2) probe other devices (no need to wait for the setting as it is already done) No, because other devices probe before sysctrl. If you bring here any manual ordering, you are doing it wrong. > The probing of the other devices (or the starting of they running state) > is guaranteed as they all need some clocks and so cannot start without > having the sysctrl node already probed. > This sysctrl node handles the clocks. Ah, so sysctrl is a clock controller for these? Then still there are no other devices depending on your USB. The USB is the owner of this property (specific bits in register), no one else. Best regards, Krzysztof
Hi Krzysztof, On Mon, Nov 21, 2022 at 5:33 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 21/11/2022 16:59, Herve Codina wrote: > > On Mon, 21 Nov 2022 12:43:16 +0100 > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >> On 18/11/2022 11:23, Herve Codina wrote: > >>> On Tue, 15 Nov 2022 15:04:17 +0100 > >>> Herve Codina <herve.codina@bootlin.com> wrote: > >>>> On Tue, 15 Nov 2022 14:07:52 +0100 > >>>>> On 15/11/2022 14:05, Krzysztof Kozlowski wrote: > >>>>>> On 14/11/2022 12:15, Herve Codina wrote: > >>>>>>> Add the h2mode property to force the USBs mode ie: > >>>>>>> - 2 hosts > >>>>>>> or > >>>>>>> - 1 host and 1 device > >>>>>>> > >>>>>>> Signed-off-by: Herve Codina <herve.codina@bootlin.com> > >>>>>>> --- > >>>>>>> .../bindings/clock/renesas,r9a06g032-sysctrl.yaml | 10 ++++++++++ > >>>>>>> 1 file changed, 10 insertions(+) > >>>>>>> > >>>>>>> diff --git a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml > >>>>>>> index 95bf485c6cec..f9e0a58aa4fb 100644 > >>>>>>> --- a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml > >>>>>>> +++ b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml > >>>>>>> @@ -39,6 +39,16 @@ properties: > >>>>>>> '#power-domain-cells': > >>>>>>> const: 0 > >>>>>>> > >>>>>>> + renesas,h2mode: > >>>>>>> + description: | > >>>>>>> + Configure the USBs mode. > >>>>>>> + - <0> : the USBs are in 1 host and 1 device mode. > >>>>>>> + - <1> : the USBs are in 2 host mode. > >>>>>>> + If the property is not present, the value used is the one already present > >>>>>>> + in the CFG_USB register (from reset or set by the bootloader). > >>>>>>> + $ref: /schemas/types.yaml#/definitions/uint32 > >>>>>>> + enum: [0, 1] > >>>>>> > >>>>>> 0/1 are quite cryptic. Why not making it a string which is easy to read > >>>>>> and understand? Can be something like "two-hosts" and "one-host". Or > >>>>>> anything you find more readable... > >>>>> > >>>>> ...but actually you should rather make it a property of your USB > >>>>> controller, not clock controller. You have two controllers and we have a > >>>>> generic property for them - dr_mode. > >>>>> > >>>>> Best regards, > >>>>> Krzysztof > >>>>> > >>>> > >>>> IMHO, this property in the USB controllers does not make sense. > >>>> Indeed each controller cannot have a different 'mode'. > >>>> Some controllers are USB host only (EHCI and OHCI) and the USBF > >>>> controller I worked on is device only. > >>>> 'h2mode' allows to choose between host or device on one of the USB > >>>> but not at the USB controller level. > >>>> > >>>> This property should be handle outside the USB controller nodes. > >>>> > >>>> Currently, this node (declared as a clock node) is in fact a sysctrl > >>>> node and can do some configuration not related to clocks. > >>>> > >>>> I agree with you something related to choosing USB Host/Device in > >>>> a clock node seems strange. > >>>> > >>>> Some discussion were already opened related to this property and how > >>>> to handle it: > >>>> https://lore.kernel.org/all/20221107182642.05a09f2f@bootlin.com/ > >>>> https://lore.kernel.org/all/20221107173614.474707d7@bootlin.com/ > >>>> > >>> > >>> We advanced on this topic. > >>> > >>> First, even if 'renesas,r9a06g032-sysctrl.yaml' is present in > >>> the devicetree/bindings/clock/ directory, this node is really > >>> a 'system controller' node: > >>> - title: Renesas RZ/N1D (R9A06G032) System Controller > >>> - compatible: renesas,r9a06g032-sysctrl > >>> > >>> It handles clocks, power domains, some DMA routing, ... > >>> > >>> Now, the property 'h2mode' allows to choose between: > >>> - 2 USB hosts > >>> or > >>> - 1 USB host and 1 USB device. > >>> > >>> This switching is system wide and has no reason to be done in > >>> one specific USB controller. It can impact multiple devices and > >>> PLL settings. > >>> > >>> The 'renesas,r9a06g032-sysctrl' node, as the system control > >>> node of our system, is the best candidate to handle the property. > >> > >> Not necessarily. IIUC, you have: > >> > >> 1. sysctrl with some register(s) for choosing device mode > >> 2. usb device or host at one address > >> 3. usb host at separate address > >> > > > > Just to clarify, usb device and host controller are not provided by > > the same IP. > > We have an USB host at some address range (PCI OHCI/EHCI USB host > > below a PCI bridge) and the USB device at some other address range > > (below a AHB to someting bridge). > > And I am not sure that only USB host or devices are affected by this > > property change. > > > >> If so then: > >> A. Pretty often we have wrapper nodes for this purpose (USB, phy > >> wrappers or glues) which are usually needed to configure something for a > >> generic block (like Synopsys etc). > >> > >> B. Pretty often the device (so your USB host or device) needs to poke > >> something in system controller registers, e.g. for power or some other > >> setup. > > > > And we did it for some items (clocks and power). > > > >> > >> Your case looks a lot like (B). We have many, many of such examples > >> already. Actually it is exactly like that, except that it affects > >> possibility of another device (e.g. choosing USB device blocks having > >> host there). > >> > >> C. It looks a bit like a multi-serial-protocol interfaces (so > >> UART+I2C+SPI). The difference is that such cases have all these nodes > >> defined as a children of the protocol-wrapping device. Not here. > >> > >> I would propose to go with (B) unless of course it's causes some crazy > >> architecture/code choices. Why? Because with exception of (C) we should > >> not define properties which represent DT node choices. IOW, Choosing a > >> node and compatible (e.g. usb controller as device) is enough to > >> describe the hardware. No need for other properties to control some > >> register in other block. > > > > The issue with h2mode is that it affects several devices and these > > devices should not be in a "running" state when the h2mode is changed. > > Why the change should happen when device is running? And why this should > be anyway different than your existing hsmode property - it also will > happen when system and device are running. > > > > PCI devices (host controllers) itself are not described in the DT. They > > are automatically enumerated. > > Aren't we talking about USB controller in a MMIO-based SoC? > > > Changing the property in USB device controller can leads to hang on > > other busses. Indeed, changing this property when a device affected > > by the property is running can lead to a bus hang.> > > In order to do that from the USB device controller I need to synchronize > > the other devices to wait for this setting before running. > > 1) probe sysctrl without setting h2mode > > 2) probe some devices (USB host and probably others) > > Stop at some point and wait for the h2mode property setting. > > Why do you need to wait? Which device needs to wait? There are no such > devices... if they are then please bring entire DTS, not some pieces in > this patchset. > > > 3) probe usb device -> Set h2mode property > > 4) allow devices waiting for the property setting to continue. > > I don't get why do you need such order. Your sysctrl also probes any > time so old solution has exactly the same problem, doesn't it? > > > This synchronization seems pretty tricky and what to do if nobody > > set the property (USB device controller not present or status="disabled" > > for instance) ? > > > > Setting this property in sysctrl probe avoid the need for all of this > > synchronization: > > 1) probe sysctrl and set h2mode. > > 2) probe other devices (no need to wait for the setting as it is already done) > > No, because other devices probe before sysctrl. If you bring here any > manual ordering, you are doing it wrong. > > > The probing of the other devices (or the starting of they running state) > > is guaranteed as they all need some clocks and so cannot start without > > having the sysctrl node already probed. > > This sysctrl node handles the clocks. > > Ah, so sysctrl is a clock controller for these? > > Then still there are no other devices depending on your USB. The USB is > the owner of this property (specific bits in register), no one else. 1. There are two USB devices. 2. The USB drivers can be modular, the sysctrl driver cannot, as it is the main clock controller. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 21/11/2022 17:36, Geert Uytterhoeven wrote: > Hi Krzysztof, > > On Mon, Nov 21, 2022 at 5:33 PM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> On 21/11/2022 16:59, Herve Codina wrote: >>> On Mon, 21 Nov 2022 12:43:16 +0100 >>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: >>>> On 18/11/2022 11:23, Herve Codina wrote: >>>>> On Tue, 15 Nov 2022 15:04:17 +0100 >>>>> Herve Codina <herve.codina@bootlin.com> wrote: >>>>>> On Tue, 15 Nov 2022 14:07:52 +0100 >>>>>>> On 15/11/2022 14:05, Krzysztof Kozlowski wrote: >>>>>>>> On 14/11/2022 12:15, Herve Codina wrote: >>>>>>>>> Add the h2mode property to force the USBs mode ie: >>>>>>>>> - 2 hosts >>>>>>>>> or >>>>>>>>> - 1 host and 1 device >>>>>>>>> >>>>>>>>> Signed-off-by: Herve Codina <herve.codina@bootlin.com> >>>>>>>>> --- >>>>>>>>> .../bindings/clock/renesas,r9a06g032-sysctrl.yaml | 10 ++++++++++ >>>>>>>>> 1 file changed, 10 insertions(+) >>>>>>>>> >>>>>>>>> diff --git a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml >>>>>>>>> index 95bf485c6cec..f9e0a58aa4fb 100644 >>>>>>>>> --- a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml >>>>>>>>> +++ b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml >>>>>>>>> @@ -39,6 +39,16 @@ properties: >>>>>>>>> '#power-domain-cells': >>>>>>>>> const: 0 >>>>>>>>> >>>>>>>>> + renesas,h2mode: >>>>>>>>> + description: | >>>>>>>>> + Configure the USBs mode. >>>>>>>>> + - <0> : the USBs are in 1 host and 1 device mode. >>>>>>>>> + - <1> : the USBs are in 2 host mode. >>>>>>>>> + If the property is not present, the value used is the one already present >>>>>>>>> + in the CFG_USB register (from reset or set by the bootloader). >>>>>>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>>>>>> + enum: [0, 1] >>>>>>>> >>>>>>>> 0/1 are quite cryptic. Why not making it a string which is easy to read >>>>>>>> and understand? Can be something like "two-hosts" and "one-host". Or >>>>>>>> anything you find more readable... >>>>>>> >>>>>>> ...but actually you should rather make it a property of your USB >>>>>>> controller, not clock controller. You have two controllers and we have a >>>>>>> generic property for them - dr_mode. >>>>>>> >>>>>>> Best regards, >>>>>>> Krzysztof >>>>>>> >>>>>> >>>>>> IMHO, this property in the USB controllers does not make sense. >>>>>> Indeed each controller cannot have a different 'mode'. >>>>>> Some controllers are USB host only (EHCI and OHCI) and the USBF >>>>>> controller I worked on is device only. >>>>>> 'h2mode' allows to choose between host or device on one of the USB >>>>>> but not at the USB controller level. >>>>>> >>>>>> This property should be handle outside the USB controller nodes. >>>>>> >>>>>> Currently, this node (declared as a clock node) is in fact a sysctrl >>>>>> node and can do some configuration not related to clocks. >>>>>> >>>>>> I agree with you something related to choosing USB Host/Device in >>>>>> a clock node seems strange. >>>>>> >>>>>> Some discussion were already opened related to this property and how >>>>>> to handle it: >>>>>> https://lore.kernel.org/all/20221107182642.05a09f2f@bootlin.com/ >>>>>> https://lore.kernel.org/all/20221107173614.474707d7@bootlin.com/ >>>>>> >>>>> >>>>> We advanced on this topic. >>>>> >>>>> First, even if 'renesas,r9a06g032-sysctrl.yaml' is present in >>>>> the devicetree/bindings/clock/ directory, this node is really >>>>> a 'system controller' node: >>>>> - title: Renesas RZ/N1D (R9A06G032) System Controller >>>>> - compatible: renesas,r9a06g032-sysctrl >>>>> >>>>> It handles clocks, power domains, some DMA routing, ... >>>>> >>>>> Now, the property 'h2mode' allows to choose between: >>>>> - 2 USB hosts >>>>> or >>>>> - 1 USB host and 1 USB device. >>>>> >>>>> This switching is system wide and has no reason to be done in >>>>> one specific USB controller. It can impact multiple devices and >>>>> PLL settings. >>>>> >>>>> The 'renesas,r9a06g032-sysctrl' node, as the system control >>>>> node of our system, is the best candidate to handle the property. >>>> >>>> Not necessarily. IIUC, you have: >>>> >>>> 1. sysctrl with some register(s) for choosing device mode >>>> 2. usb device or host at one address >>>> 3. usb host at separate address >>>> >>> >>> Just to clarify, usb device and host controller are not provided by >>> the same IP. >>> We have an USB host at some address range (PCI OHCI/EHCI USB host >>> below a PCI bridge) and the USB device at some other address range >>> (below a AHB to someting bridge). >>> And I am not sure that only USB host or devices are affected by this >>> property change. >>> >>>> If so then: >>>> A. Pretty often we have wrapper nodes for this purpose (USB, phy >>>> wrappers or glues) which are usually needed to configure something for a >>>> generic block (like Synopsys etc). >>>> >>>> B. Pretty often the device (so your USB host or device) needs to poke >>>> something in system controller registers, e.g. for power or some other >>>> setup. >>> >>> And we did it for some items (clocks and power). >>> >>>> >>>> Your case looks a lot like (B). We have many, many of such examples >>>> already. Actually it is exactly like that, except that it affects >>>> possibility of another device (e.g. choosing USB device blocks having >>>> host there). >>>> >>>> C. It looks a bit like a multi-serial-protocol interfaces (so >>>> UART+I2C+SPI). The difference is that such cases have all these nodes >>>> defined as a children of the protocol-wrapping device. Not here. >>>> >>>> I would propose to go with (B) unless of course it's causes some crazy >>>> architecture/code choices. Why? Because with exception of (C) we should >>>> not define properties which represent DT node choices. IOW, Choosing a >>>> node and compatible (e.g. usb controller as device) is enough to >>>> describe the hardware. No need for other properties to control some >>>> register in other block. >>> >>> The issue with h2mode is that it affects several devices and these >>> devices should not be in a "running" state when the h2mode is changed. >> >> Why the change should happen when device is running? And why this should >> be anyway different than your existing hsmode property - it also will >> happen when system and device are running. >> >> >>> PCI devices (host controllers) itself are not described in the DT. They >>> are automatically enumerated. >> >> Aren't we talking about USB controller in a MMIO-based SoC? >> >>> Changing the property in USB device controller can leads to hang on >>> other busses. Indeed, changing this property when a device affected >>> by the property is running can lead to a bus hang.> >>> In order to do that from the USB device controller I need to synchronize >>> the other devices to wait for this setting before running. >>> 1) probe sysctrl without setting h2mode >>> 2) probe some devices (USB host and probably others) >>> Stop at some point and wait for the h2mode property setting. >> >> Why do you need to wait? Which device needs to wait? There are no such >> devices... if they are then please bring entire DTS, not some pieces in >> this patchset. >> >>> 3) probe usb device -> Set h2mode property >>> 4) allow devices waiting for the property setting to continue. >> >> I don't get why do you need such order. Your sysctrl also probes any >> time so old solution has exactly the same problem, doesn't it? >> >>> This synchronization seems pretty tricky and what to do if nobody >>> set the property (USB device controller not present or status="disabled" >>> for instance) ? >>> >>> Setting this property in sysctrl probe avoid the need for all of this >>> synchronization: >>> 1) probe sysctrl and set h2mode. >>> 2) probe other devices (no need to wait for the setting as it is already done) >> >> No, because other devices probe before sysctrl. If you bring here any >> manual ordering, you are doing it wrong. >> >>> The probing of the other devices (or the starting of they running state) >>> is guaranteed as they all need some clocks and so cannot start without >>> having the sysctrl node already probed. >>> This sysctrl node handles the clocks. >> >> Ah, so sysctrl is a clock controller for these? >> >> Then still there are no other devices depending on your USB. The USB is >> the owner of this property (specific bits in register), no one else. > > 1. There are two USB devices. > 2. The USB drivers can be modular, the sysctrl driver cannot, as it is > the main clock controller. 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? Best regards, Krzysztof
Hi Krzysztof, On Mon, Nov 21, 2022 at 6:11 PM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 21/11/2022 17:36, Geert Uytterhoeven wrote: > > On Mon, Nov 21, 2022 at 5:33 PM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> On 21/11/2022 16:59, Herve Codina wrote: > >>> On Mon, 21 Nov 2022 12:43:16 +0100 > >>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > >>>> On 18/11/2022 11:23, Herve Codina wrote: > >>>>> On Tue, 15 Nov 2022 15:04:17 +0100 > >>>>> Herve Codina <herve.codina@bootlin.com> wrote: > >>>>>> On Tue, 15 Nov 2022 14:07:52 +0100 > >>>>>>> On 15/11/2022 14:05, Krzysztof Kozlowski wrote: > >>>>>>>> On 14/11/2022 12:15, Herve Codina wrote: > >>>>>>>>> Add the h2mode property to force the USBs mode ie: > >>>>>>>>> - 2 hosts > >>>>>>>>> or > >>>>>>>>> - 1 host and 1 device > >>>>>>>>> > >>>>>>>>> Signed-off-by: Herve Codina <herve.codina@bootlin.com> > >>>>>>>>> --- > >>>>>>>>> .../bindings/clock/renesas,r9a06g032-sysctrl.yaml | 10 ++++++++++ > >>>>>>>>> 1 file changed, 10 insertions(+) > >>>>>>>>> > >>>>>>>>> diff --git a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml > >>>>>>>>> index 95bf485c6cec..f9e0a58aa4fb 100644 > >>>>>>>>> --- a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml > >>>>>>>>> +++ b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml > >>>>>>>>> @@ -39,6 +39,16 @@ properties: > >>>>>>>>> '#power-domain-cells': > >>>>>>>>> const: 0 > >>>>>>>>> > >>>>>>>>> + renesas,h2mode: > >>>>>>>>> + description: | > >>>>>>>>> + Configure the USBs mode. > >>>>>>>>> + - <0> : the USBs are in 1 host and 1 device mode. > >>>>>>>>> + - <1> : the USBs are in 2 host mode. > >>>>>>>>> + If the property is not present, the value used is the one already present > >>>>>>>>> + in the CFG_USB register (from reset or set by the bootloader). > >>>>>>>>> + $ref: /schemas/types.yaml#/definitions/uint32 > >>>>>>>>> + enum: [0, 1] > >>>>>>>> > >>>>>>>> 0/1 are quite cryptic. Why not making it a string which is easy to read > >>>>>>>> and understand? Can be something like "two-hosts" and "one-host". Or > >>>>>>>> anything you find more readable... > >>>>>>> > >>>>>>> ...but actually you should rather make it a property of your USB > >>>>>>> controller, not clock controller. You have two controllers and we have a > >>>>>>> generic property for them - dr_mode. > >>>>>>> > >>>>>>> Best regards, > >>>>>>> Krzysztof > >>>>>>> > >>>>>> > >>>>>> IMHO, this property in the USB controllers does not make sense. > >>>>>> Indeed each controller cannot have a different 'mode'. > >>>>>> Some controllers are USB host only (EHCI and OHCI) and the USBF > >>>>>> controller I worked on is device only. > >>>>>> 'h2mode' allows to choose between host or device on one of the USB > >>>>>> but not at the USB controller level. > >>>>>> > >>>>>> This property should be handle outside the USB controller nodes. > >>>>>> > >>>>>> Currently, this node (declared as a clock node) is in fact a sysctrl > >>>>>> node and can do some configuration not related to clocks. > >>>>>> > >>>>>> I agree with you something related to choosing USB Host/Device in > >>>>>> a clock node seems strange. > >>>>>> > >>>>>> Some discussion were already opened related to this property and how > >>>>>> to handle it: > >>>>>> https://lore.kernel.org/all/20221107182642.05a09f2f@bootlin.com/ > >>>>>> https://lore.kernel.org/all/20221107173614.474707d7@bootlin.com/ > >>>>>> > >>>>> > >>>>> We advanced on this topic. > >>>>> > >>>>> First, even if 'renesas,r9a06g032-sysctrl.yaml' is present in > >>>>> the devicetree/bindings/clock/ directory, this node is really > >>>>> a 'system controller' node: > >>>>> - title: Renesas RZ/N1D (R9A06G032) System Controller > >>>>> - compatible: renesas,r9a06g032-sysctrl > >>>>> > >>>>> It handles clocks, power domains, some DMA routing, ... > >>>>> > >>>>> Now, the property 'h2mode' allows to choose between: > >>>>> - 2 USB hosts > >>>>> or > >>>>> - 1 USB host and 1 USB device. > >>>>> > >>>>> This switching is system wide and has no reason to be done in > >>>>> one specific USB controller. It can impact multiple devices and > >>>>> PLL settings. > >>>>> > >>>>> The 'renesas,r9a06g032-sysctrl' node, as the system control > >>>>> node of our system, is the best candidate to handle the property. > >>>> > >>>> Not necessarily. IIUC, you have: > >>>> > >>>> 1. sysctrl with some register(s) for choosing device mode > >>>> 2. usb device or host at one address > >>>> 3. usb host at separate address > >>>> > >>> > >>> Just to clarify, usb device and host controller are not provided by > >>> the same IP. > >>> We have an USB host at some address range (PCI OHCI/EHCI USB host > >>> below a PCI bridge) and the USB device at some other address range > >>> (below a AHB to someting bridge). > >>> And I am not sure that only USB host or devices are affected by this > >>> property change. > >>> > >>>> If so then: > >>>> A. Pretty often we have wrapper nodes for this purpose (USB, phy > >>>> wrappers or glues) which are usually needed to configure something for a > >>>> generic block (like Synopsys etc). > >>>> > >>>> B. Pretty often the device (so your USB host or device) needs to poke > >>>> something in system controller registers, e.g. for power or some other > >>>> setup. > >>> > >>> And we did it for some items (clocks and power). > >>> > >>>> > >>>> Your case looks a lot like (B). We have many, many of such examples > >>>> already. Actually it is exactly like that, except that it affects > >>>> possibility of another device (e.g. choosing USB device blocks having > >>>> host there). > >>>> > >>>> C. It looks a bit like a multi-serial-protocol interfaces (so > >>>> UART+I2C+SPI). The difference is that such cases have all these nodes > >>>> defined as a children of the protocol-wrapping device. Not here. > >>>> > >>>> I would propose to go with (B) unless of course it's causes some crazy > >>>> architecture/code choices. Why? Because with exception of (C) we should > >>>> not define properties which represent DT node choices. IOW, Choosing a > >>>> node and compatible (e.g. usb controller as device) is enough to > >>>> describe the hardware. No need for other properties to control some > >>>> register in other block. > >>> > >>> The issue with h2mode is that it affects several devices and these > >>> devices should not be in a "running" state when the h2mode is changed. > >> > >> Why the change should happen when device is running? And why this should > >> be anyway different than your existing hsmode property - it also will > >> happen when system and device are running. > >> > >> > >>> PCI devices (host controllers) itself are not described in the DT. They > >>> are automatically enumerated. > >> > >> Aren't we talking about USB controller in a MMIO-based SoC? > >> > >>> Changing the property in USB device controller can leads to hang on > >>> other busses. Indeed, changing this property when a device affected > >>> by the property is running can lead to a bus hang.> > >>> In order to do that from the USB device controller I need to synchronize > >>> the other devices to wait for this setting before running. > >>> 1) probe sysctrl without setting h2mode > >>> 2) probe some devices (USB host and probably others) > >>> Stop at some point and wait for the h2mode property setting. > >> > >> Why do you need to wait? Which device needs to wait? There are no such > >> devices... if they are then please bring entire DTS, not some pieces in > >> this patchset. > >> > >>> 3) probe usb device -> Set h2mode property > >>> 4) allow devices waiting for the property setting to continue. > >> > >> I don't get why do you need such order. Your sysctrl also probes any > >> time so old solution has exactly the same problem, doesn't it? > >> > >>> This synchronization seems pretty tricky and what to do if nobody > >>> set the property (USB device controller not present or status="disabled" > >>> for instance) ? > >>> > >>> Setting this property in sysctrl probe avoid the need for all of this > >>> synchronization: > >>> 1) probe sysctrl and set h2mode. > >>> 2) probe other devices (no need to wait for the setting as it is already done) > >> > >> No, because other devices probe before sysctrl. If you bring here any > >> manual ordering, you are doing it wrong. > >> > >>> The probing of the other devices (or the starting of they running state) > >>> is guaranteed as they all need some clocks and so cannot start without > >>> having the sysctrl node already probed. > >>> This sysctrl node handles the clocks. > >> > >> Ah, so sysctrl is a clock controller for these? > >> > >> Then still there are no other devices depending on your USB. The USB is > >> the owner of this property (specific bits in register), no one else. > > > > 1. There are two USB devices. > > 2. The USB drivers can be modular, the sysctrl driver cannot, as it is > > the main clock controller. > > 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. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
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. Best regards, Krzysztof
Hi Krzysztof, On Tue, Nov 22, 2022 at 8:45 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 22/11/2022 09:25, Geert Uytterhoeven wrote: > Hi Krzysztof, > > On Tue, Nov 22, 2022 at 8:45 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> 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. You have here in such case parent-child dependency, not provider-consumer. Just like for all serial-protocol engines (I2C/UART/SPI). Best regards, Krzysztof
Hi Krzysztof, On Tue, Nov 22, 2022 at 9:42 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 22/11/2022 09:25, Geert Uytterhoeven wrote: > > On Tue, Nov 22, 2022 at 8:45 AM Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> 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? Exactly. > 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? Enabling the clocks does not have anything to do with this ordering. Clock consumers that are part of the clock domain are probed after clock providers. If the clock is missing, that would be an incorrect description in DTS. > Broken dependency. What if you want to use this in a different SoC, > where the sysctrl does not provide clocks? Broken dependency. This is the "renesas,r9a06g032-sysctrl" DT bindings document. It talks about this SoC implementation specifically. This is not a random synthesizable IP Core that can appear anywhere. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Tue, 22 Nov 2022 09:42:48 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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@linaro.org> 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. > > You have here in such case parent-child dependency, not > provider-consumer. Just like for all serial-protocol engines (I2C/UART/SPI). > > Best regards, > Krzysztof >
On 22/11/2022 10:01, Geert Uytterhoeven wrote: >>>>> 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? > > Exactly. > >> 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? > > Enabling the clocks does not have anything to do with this ordering. That was the argument from Herve, that ordering is guaranteed by clocks. > Clock consumers that are part of the clock domain are probed after > clock providers. If the clock is missing, that would be an incorrect > description in DTS. If not clocks, what else is guaranteeing the ordering? You did not express it in DT. Best regards, Krzysztof
Hi Krzysztof, On Tue, Nov 22, 2022 at 11:23 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 22/11/2022 10:01, Geert Uytterhoeven wrote: > >>>>> 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? > > > > Exactly. > > > >> 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? > > > > Enabling the clocks does not have anything to do with this ordering. > > That was the argument from Herve, that ordering is guaranteed by clocks. > > > Clock consumers that are part of the clock domain are probed after > > clock providers. If the clock is missing, that would be an incorrect > > description in DTS. > > If not clocks, what else is guaranteeing the ordering? You did not > express it in DT. clocks and power-domains And if not clocks and power-domains... Oops, we didn't express in DT that the SoC needs to be powered at all ;-) Can we please stop this pointless discussion? Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 22/11/2022 10:07, Herve Codina wrote: > On Tue, 22 Nov 2022 09:42:48 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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@linaro.org> 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. 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. Best regards, Krzysztof
On 22/11/2022 11:26, Geert Uytterhoeven wrote: >> >> If not clocks, what else is guaranteeing the ordering? You did not >> express it in DT. > > clocks and power-domains > > And if not clocks and power-domains... Oops, we didn't express in DT > that the SoC needs to be powered at all ;-) The SoC is a parent of that device, so you have an expressed dependency... Best regards, Krzysztof
Hi Krzysztof, On Tue, Nov 22, 2022 at 11:30 AM Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 22/11/2022 10:07, Herve Codina wrote: > > On Tue, 22 Nov 2022 09:42:48 +0100 > > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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@linaro.org> 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... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 22/11/2022 11:47, Geert Uytterhoeven wrote: > Hi Krzysztof, > > On Tue, Nov 22, 2022 at 11:30 AM Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> On 22/11/2022 10:07, Herve Codina wrote: >>> On Tue, 22 Nov 2022 09:42:48 +0100 >>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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@linaro.org> 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... 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. Anyway, I appreciate your explanations. I don't oppose this and I defer the decision to Rob (for this or for v3 patch with descriptive strings). Best regards, Krzysztof
Hi Krzysztof, krzysztof.kozlowski@linaro.org 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@linaro.org> wrote: > >> On 22/11/2022 10:07, Herve Codina wrote: > >>> On Tue, 22 Nov 2022 09:42:48 +0100 > >>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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@linaro.org> 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 = ? }; }; 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). 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>; }; 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
On 24/11/2022 10:36, Miquel Raynal wrote: > Hi Krzysztof, > > krzysztof.kozlowski@linaro.org 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@linaro.org> wrote: >>>> On 22/11/2022 10:07, Herve Codina wrote: >>>>> On Tue, 22 Nov 2022 09:42:48 +0100 >>>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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@linaro.org> 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... > 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. > > 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
Hi Krzysztof, krzysztof.kozlowski@linaro.org wrote on Thu, 24 Nov 2022 10:46:14 +0100: > On 24/11/2022 10:36, Miquel Raynal wrote: > > Hi Krzysztof, > > > > krzysztof.kozlowski@linaro.org 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@linaro.org> wrote: > >>>> On 22/11/2022 10:07, Herve Codina wrote: > >>>>> On Tue, 22 Nov 2022 09:42:48 +0100 > >>>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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@linaro.org> 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@bootlin.com/ 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
Hi Krzysztof, miquel.raynal@bootlin.com wrote on Thu, 24 Nov 2022 11:27:57 +0100: > Hi Krzysztof, > > krzysztof.kozlowski@linaro.org wrote on Thu, 24 Nov 2022 10:46:14 +0100: > > > On 24/11/2022 10:36, Miquel Raynal wrote: > > > Hi Krzysztof, > > > > > > krzysztof.kozlowski@linaro.org 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@linaro.org> wrote: > > >>>> On 22/11/2022 10:07, Herve Codina wrote: > > >>>>> On Tue, 22 Nov 2022 09:42:48 +0100 > > >>>>> Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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@linaro.org> 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: I'm partially wrong here. I forgot that we somehow need to know whether the second USB controller is host or device, and decide this before the USB controllers probe. It is still possible to do it without the property by looking up the tree against the USB controller compatible or node name. We would do this in the sysctrl probe and set the right mode depending on its presence. I would definitely go for a property instead, but if this is a no-go, I guess that's the last resort. > - 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@bootlin.com/ > > 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 Thanks, Miquèl
diff --git a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml index 95bf485c6cec..f9e0a58aa4fb 100644 --- a/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml +++ b/Documentation/devicetree/bindings/clock/renesas,r9a06g032-sysctrl.yaml @@ -39,6 +39,16 @@ properties: '#power-domain-cells': const: 0 + renesas,h2mode: + description: | + Configure the USBs mode. + - <0> : the USBs are in 1 host and 1 device mode. + - <1> : the USBs are in 2 host mode. + If the property is not present, the value used is the one already present + in the CFG_USB register (from reset or set by the bootloader). + $ref: /schemas/types.yaml#/definitions/uint32 + enum: [0, 1] + '#address-cells': const: 1
Add the h2mode property to force the USBs mode ie: - 2 hosts or - 1 host and 1 device Signed-off-by: Herve Codina <herve.codina@bootlin.com> --- .../bindings/clock/renesas,r9a06g032-sysctrl.yaml | 10 ++++++++++ 1 file changed, 10 insertions(+)