Re: [PATCH 2/2] usb: dwc3: add clock and resets

[Masahiro Yamada]

Hi Rob,

2018-03-18 21:52 GMT+09:00 Rob Herring <robh@xxxxxxxxxx>:
> On Thu, Mar 15, 2018 at 08:39:58PM +0900, Masahiro Yamada wrote:
>> dwc3-of-simple.c only handles arbitrary number of clocks and resets.
>> They are both generic enough to be put into the dwc3 core.  For simple
>> cases, a nested node structure like follows:
>>
>>   dwc3-glue {
>>           compatible = "foo,dwc3";
>>           clocks = ...;
>>           resets = ...;
>>           ...
>>
>>           dwc3 {
>>                   compatible = "snps,dwc3";
>>                   ...
>>           };
>
> I'm not a fan of how this was done.
>
>
>>   }
>>
>> would be turned into a single node:
>>
>>   dwc3 {
>>           compatible = "foo,dwc3", "snps,dwc3";
>>           clocks = ...;
>>           resets = ...;
>>           ...
>>   }
>
> And yes, this is what I'd prefer.



Not only dwc3-of-simple.c, but all dwc3 nodes are
written like this.


omap_dwc3_1: omap_dwc3_1@48880000 {
        compatible = "ti,dwc3";
        reg = <0x48880000 0x10000>;
        #address-cells = <1>;
        #size-cells = <1>;
        ranges;
        ...

        usb1: usb@48890000 {
                compatible = "snps,dwc3";
                reg = <0x48890000 0x17000>;
                ...
        };
};


The glue layer initializes SoC-specific things,
then populates the child "snps,dwc3".


I think the following structure should work
by handling EPROBE_DEFER properly.

omap_dwc3_1: omap_dwc3_1@48880000 {
        compatible = "ti,dwc3"; (should be "ti,dwc3-glue" or something)
        reg = <0x48880000 0x10000>;
        ...
};

usb1: usb@48890000 {
        compatible = "snps,dwc3";
        reg = <0x48890000 0x17000>;
        ...
};



>>
>> I inserted reset_control_deassert() and clk_enable() before the first
>> register access, i.e. dwc3_cache_hwparams().
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@xxxxxxxxxxxxx>
>> ---
>>
>>  Documentation/devicetree/bindings/usb/dwc3.txt |   2 +
>>  drivers/usb/dwc3/core.c                        | 127 ++++++++++++++++++++++++-
>>  drivers/usb/dwc3/core.h                        |   5 +
>>  3 files changed, 132 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>> index 44e8bab..67e9cfb 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> @@ -9,12 +9,14 @@ Required properties:
>>   - interrupts: Interrupts used by the dwc3 controller.
>>
>>  Optional properties:
>> + - clocks: list of phandle and clock specifier pairs
>
> However, this should be specific as to how many clocks and their
> function. This should be readily available to someone with access to
> Synopsys datasheet. The number of clocks should generally be the same
> across SoCs, it is just some SoCs either tie clocks together or don't
> provide s/w control of some of the clocks.


Make sense.
You also implies this property is mandatory.
The number of clocks should be available in the datasheet
and no hardware can work with zero clock.

However, making it mandatory breaks the binding
since the existing DT files do not specify clocks at all
in the "snps,dwc3" node.



Anyway, our current situation:

- We have the dwc3-core under the glue layer node
  despite they are independent in the CPU address view
- We add all sorts of clocks and resets in the glue layer node,
  and nothing in the dwc3-core node.

If these are design mistake, what should we do?

Continue development based on it?
If we fix it, how to change the course?





>>   - usb-phy : array of phandle for the PHY device.  The first element
>>     in the array is expected to be a handle to the USB2/HS PHY and
>>     the second element is expected to be a handle to the USB3/SS PHY
>>   - phys: from the *Generic PHY* bindings
>>   - phy-names: from the *Generic PHY* bindings; supported names are "usb2-phy"
>>       or "usb3-phy".
>> + - resets: list of phandle and reset specifier pairs
>
> ditto
>

Same issue as the clocks.



-- 
Best Regards
Masahiro Yamada