Re: [PATCH v3 2/6] dt-bindings: usb: dwc3: Add the Amlogic A1 Family DWC3 Glue Bindings

[Hanjie Lin <hanjie.lin@amlogic.com>]

On 2020/1/4 8:32, Rob Herring wrote:
> On Fri, Dec 27, 2019 at 02:36:42PM +0800, Hanjie Lin wrote:
>> The Amlogic A1 SoC Family embeds 1 USB Controllers:
>>  - a DWC3 IP configured as Host for USB2 and USB3
>>
>> A glue connects the controllers to the USB2 PHY of A1 SoC.
>>
>> Signed-off-by: Hanjie Lin <hanjie.lin@amlogic.com>
>> Signed-off-by: Yue Wang <yue.wang@amlogic.com>
>> ---
>>  .../bindings/usb/amlogic,meson-g12a-usb-ctrl.yaml  | 57 +++++++++++++++++++---
>>  1 file changed, 51 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/amlogic,meson-g12a-usb-ctrl.yaml b/Documentation/devicetree/bindings/usb/amlogic,meson-g12a-usb-ctrl.yaml
>> index 4efb77b..6103cc2 100644
>> --- a/Documentation/devicetree/bindings/usb/amlogic,meson-g12a-usb-ctrl.yaml
>> +++ b/Documentation/devicetree/bindings/usb/amlogic,meson-g12a-usb-ctrl.yaml
>> @@ -9,6 +9,8 @@ title: Amlogic Meson G12A DWC3 USB SoC Controller Glue
>>  
>>  maintainers:
>>    - Neil Armstrong <narmstrong@baylibre.com>
>> +  - Hanjie Lin <hanjie.lin@amlogic.com>
>> +  - Yue Wang <yue.wang@amlogic.com>
>>  
>>  description: |
>>    The Amlogic G12A embeds a DWC3 USB IP Core configured for USB2 and USB3
>> @@ -22,10 +24,14 @@ description: |
>>    The DWC3 Glue controls the PHY routing and power, an interrupt line is
>>    connected to the Glue to serve as OTG ID change detection.
>>  
>> +  The Amlogic A1 embeds a DWC3 USB IP Core configured for USB2 in
>> +  host-only mode.
>> +
>>  properties:
>>    compatible:
>>      enum:
>>        - amlogic,meson-g12a-usb-ctrl
>> +      - amlogic,meson-a1-usb-ctrl
>>  
>>    ranges: true
>>  
>> @@ -37,6 +43,11 @@ properties:
>>  
>>    clocks:
>>      minItems: 1
>> +    maxItems: 4
>> +
>> +  clock-names:
>> +    minItems: 1
>> +    maxItems: 4
>>  
>>    resets:
>>      minItems: 1
>> @@ -47,17 +58,22 @@ properties:
>>    interrupts:
>>      maxItems: 1
>>  
>> +  phys:
>> +    minItems: 1
>> +    maxItems: 3
>> +
>>    phy-names:
>>      items:
>>        - const: usb2-phy0 # USB2 PHY0 if USBHOST_A port is used
>>        - const: usb2-phy1 # USB2 PHY1 if USBOTG_B port is used
>>        - const: usb3-phy0 # USB3 PHY if USB3_0 is used
>>  
>> -  phys:
>> -    minItems: 1
>> -    maxItems: 3
>> -
> 
> Why the unnecessary move?
> 

I saw most "phys" attributes are front of "phy-names" in dts, maybe looks pretty no other reasons.

>> -  dr_mode: true
>> +  dr_mode:
>> +    description: usb mode for G12A
>> +    enum:
>> +      - host
>> +      - peripheral
>> +      - otg
> 
> No, this is a common property that doesn't need to be redefined here. It 
> was correct as-is.
> 

Ok, I will modify it.

>>  
>>    power-domains:
>>      maxItems: 1
>> @@ -80,9 +96,9 @@ required:
>>    - resets
>>    - reg
>>    - interrupts
>> -  - phy-names
>>    - phys
>>    - dr_mode
>> +  - phy-names
> 
> Again, unnecessary change.
> 

Ok

>>  
>>  examples:
>>    - |
>> @@ -124,4 +140,33 @@ examples:
>>                snps,quirk-frame-length-adjustment;
>>            };
>>      };
>> +  - |
>> +    a1_usb: usb@ffe09000 {
> 
> You are only adding a compatible. No need for a whole new example.
> 

Ok, I will fix it.

Thanks, 

Hanjie

>> +          status = "okay";
>> +          compatible = "amlogic,meson-a1-usb-ctrl";
>> +          reg = <0 0xffe09000 0x0 0xa0>;
>> +          #address-cells = <1>;
>> +          #size-cells = <1>;
>> +          ranges;
>>  
>> +          clocks = <&clkc_periphs 36>,
>> +                   <&clkc_periphs 85>,
>> +                   <&clkc_periphs 2>,
>> +                   <&clkc_periphs 3>;
>> +          clock-names = "usb_ctrl", "usb_bus", "xtal_usb_phy",
>> +                        "xtal_usb_ctrl";
>> +
>> +          resets = <&reset 36>;
>> +
>> +          phys = <&usb2_phy1>;
>> +          phy-names = "usb2-phy1";
>> +
>> +          a1_dwc3: usb@ff400000 {
>> +                  compatible = "snps,dwc3";
>> +                  reg = <0xff400000 0x100000>;
>> +                  interrupts = <0 90 4>;
>> +                  dr_mode = "host";
>> +                  snps,dis_u2_susphy_quirk;
>> +                  snps,quirk-frame-length-adjustment = <0x20>;
>> +          };
>> +    };
>> -- 
>> 2.7.4
>>
> 
> .
> 

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic