diff mbox series

[v5,2/6] dt-bindings: usb: dwc3: Add the Amlogic A1 Family DWC3 Glue Bindings

Message ID 1578634957-54826-3-git-send-email-hanjie.lin@amlogic.com (mailing list archive)
State Superseded
Headers show
Series arm64: meson: Add support for USB on Amlogic A1 | expand

Commit Message

Hanjie Lin Jan. 10, 2020, 5:42 a.m. UTC
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>
---
 .../devicetree/bindings/usb/amlogic,meson-g12a-usb-ctrl.yaml  | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Martin Blumenstingl Jan. 11, 2020, 8:50 p.m. UTC | #1
Hi Hanjie,

On Fri, Jan 10, 2020 at 6:43 AM Hanjie Lin <hanjie.lin@amlogic.com> wrote:
[...]
> @@ -37,6 +43,11 @@ properties:
>
>    clocks:
>      minItems: 1
> +    maxItems: 4
the driver parses one clock for G12A/G12B/SM1 and three clocks for A1
if there is a fourth clock: do we need to manage it in the driver?
(note: dt-bindings always represent the hardware, so if there's a
fourth clock which the driver doesn't need then it's perfectly valid
to describe it here. a comment which clock this is helps in the
code-review process)

> +  clock-names:
> +    minItems: 1
> +    maxItems: 4
I let Rob comment on this, personally I prefer naming the clocks explicitly
also I think clock-names has to be a mandatory property for A1 (see
Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-codec.yaml
for an example which makes properties mandatory depending on the
compatible string)


Martin
Hanjie Lin Jan. 13, 2020, 1:23 a.m. UTC | #2
On 2020/1/12 4:50, Martin Blumenstingl wrote:
> Hi Hanjie,
> 
> On Fri, Jan 10, 2020 at 6:43 AM Hanjie Lin <hanjie.lin@amlogic.com> wrote:
> [...]
>> @@ -37,6 +43,11 @@ properties:
>>
>>    clocks:
>>      minItems: 1
>> +    maxItems: 4
> the driver parses one clock for G12A/G12B/SM1 and three clocks for A1
> if there is a fourth clock: do we need to manage it in the driver?
> (note: dt-bindings always represent the hardware, so if there's a
> fourth clock which the driver doesn't need then it's perfectly valid
> to describe it here. a comment which clock this is helps in the
> code-review process)
> 

Hi Martin,

Sorry for this confusing, I moved xtal_usb_phy clock from glue driver to phy,
but I missed this binding modification. 
So actually A1 do only need these three clocks and no fourth clock exist, clock
and clock-names maxItems should be three here for A1.

>> +  clock-names:
>> +    minItems: 1
>> +    maxItems: 4
> I let Rob comment on this, personally I prefer naming the clocks explicitly
> also I think clock-names has to be a mandatory property for A1 (see
> Documentation/devicetree/bindings/sound/allwinner,sun4i-a10-codec.yaml
> for an example which makes properties mandatory depending on the
> compatible string)
> 
> 
> Martin
> 
> .
> 

Thanks for your patient and detailed advice.

Do you mean something like this:

+allOf:
+  - if:
+      properties:
+        compatible:
+          enum:
+            - amlogic,meson-g12a-usb-ctrl
+
+    then:
+      properties:
+        clocks:
+         minItems: 1
+
+  - if:
+      properties:
+        compatible:
+          enum:
+            - amlogic,meson-a1-usb-ctrl
+
+    then:
+      properties:
+        clocks:
+          items:
+            minItems: 3
+       clock-names:
+          items:
+            - const: usb_ctrl
+            - const: usb_bus
+            - const: xtal_usb_ctrl
+      required:
+        - clock-names


Hanjie
Martin Blumenstingl Jan. 14, 2020, 8:10 p.m. UTC | #3
Hi Hanjie,

On Mon, Jan 13, 2020 at 2:23 AM Hanjie Lin <hanjie.lin@amlogic.com> wrote:
>
>
>
> On 2020/1/12 4:50, Martin Blumenstingl wrote:
> > Hi Hanjie,
> >
> > On Fri, Jan 10, 2020 at 6:43 AM Hanjie Lin <hanjie.lin@amlogic.com> wrote:
> > [...]
> >> @@ -37,6 +43,11 @@ properties:
> >>
> >>    clocks:
> >>      minItems: 1
> >> +    maxItems: 4
> > the driver parses one clock for G12A/G12B/SM1 and three clocks for A1
> > if there is a fourth clock: do we need to manage it in the driver?
> > (note: dt-bindings always represent the hardware, so if there's a
> > fourth clock which the driver doesn't need then it's perfectly valid
> > to describe it here. a comment which clock this is helps in the
> > code-review process)
> >
>
> Hi Martin,
>
> Sorry for this confusing, I moved xtal_usb_phy clock from glue driver to phy,
> but I missed this binding modification.
> So actually A1 do only need these three clocks and no fourth clock exist, clock
> and clock-names maxItems should be three here for A1.
I see, thank you for clarifying this!

[...]
> Do you mean something like this:
>
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - amlogic,meson-g12a-usb-ctrl
> +
> +    then:
> +      properties:
> +        clocks:
> +         minItems: 1
> +
> +  - if:
> +      properties:
> +        compatible:
> +          enum:
> +            - amlogic,meson-a1-usb-ctrl
> +
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            minItems: 3
> +       clock-names:
> +          items:
> +            - const: usb_ctrl
> +            - const: usb_bus
> +            - const: xtal_usb_ctrl
> +      required:
> +        - clock-names
this looks good to me (but keep in mind that I'm no expert on these
yaml schemas)
I wonder if we are allowed to shorten this by having one clocks
property with minItems: 1 and maxItems: 3 (like you have in the
original patch) and then only have a
amlogic,meson-a1-usb-ctrl-specific "clock-names" property (and make
that mandatory for A1 SoCs)


Martin
diff mbox series

Patch

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