Hi Krzysztof, Geert, On Tue, 15 Nov 2022 15:04:17 +0100 Herve Codina <herve.codina@xxxxxxxxxxx> wrote: > Hi Krzysztof, > > On Tue, 15 Nov 2022 14:07:52 +0100 > Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> 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@xxxxxxxxxxx> > > >> --- > > >> .../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@xxxxxxxxxxx/ > https://lore.kernel.org/all/20221107173614.474707d7@xxxxxxxxxxx/ > 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é -- Hervé Codina, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
