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

[Herve Codina <herve.codina@bootlin.com>]

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é

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com