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

[Krzysztof Kozlowski]

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@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.

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