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