| Message ID | 20230828133033.11988-3-quic_kriskura@quicinc.com |
|---|---|
| State | Accepted |
| Commit | eb3f1d9e42b1499152442e97b51bc1bcfee29d71 |
| Headers | show |
| Series | Add multiport support for DWC3 controllers | expand |
On Mon, Aug 28, 2023 at 07:00:22PM +0530, Krishna Kurapati wrote: > Add bindings to indicate properties required to support multiport > on Synopsys DWC3 controller. > > Suggested-by: Bjorn Andersson <quic_bjorande@quicinc.com> > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > Reviewed-by: Rob Herring <robh@kernel.org> > --- > .../devicetree/bindings/usb/snps,dwc3.yaml | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml > index a696f23730d3..5bc941355b43 100644 > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml > @@ -85,15 +85,16 @@ properties: > > phys: > minItems: 1 > - maxItems: 2 > + maxItems: 8 > > phy-names: > minItems: 1 > - maxItems: 2 > - items: > - enum: > - - usb2-phy > - - usb3-phy > + maxItems: 8 > + oneOf: > + - items: > + enum: [ usb2-phy, usb3-phy ] > + - items: > + pattern: "^usb[23]-port[0-3]$" Shouldn't this just be pattern: "^usb[23]-[0-3]$" so that it matches the names that are used by the nvidia bindings? We already have some inconsistency in that Amlogic uses a variant based on the legacy names that needlessly includes "phy" in the names: const: usb2-phy0 const: usb2-phy1 const: usb3-phy0 ... I don't think we should be introducing a third naming scheme here so I suggest just following the nvidia bindings. Johan
On 11/10/2023 6:58 PM, Johan Hovold wrote: > On Mon, Aug 28, 2023 at 07:00:22PM +0530, Krishna Kurapati wrote: >> Add bindings to indicate properties required to support multiport >> on Synopsys DWC3 controller. >> >> Suggested-by: Bjorn Andersson <quic_bjorande@quicinc.com> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> Reviewed-by: Rob Herring <robh@kernel.org> >> --- >> .../devicetree/bindings/usb/snps,dwc3.yaml | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml >> index a696f23730d3..5bc941355b43 100644 >> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml >> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml >> @@ -85,15 +85,16 @@ properties: >> >> phys: >> minItems: 1 >> - maxItems: 2 >> + maxItems: 8 >> >> phy-names: >> minItems: 1 >> - maxItems: 2 >> - items: >> - enum: >> - - usb2-phy >> - - usb3-phy >> + maxItems: 8 >> + oneOf: >> + - items: >> + enum: [ usb2-phy, usb3-phy ] >> + - items: >> + pattern: "^usb[23]-port[0-3]$" > > Shouldn't this just be > > pattern: "^usb[23]-[0-3]$" > > so that it matches the names that are used by the nvidia bindings? > > We already have some inconsistency in that Amlogic uses a variant based > on the legacy names that needlessly includes "phy" in the names: > > const: usb2-phy0 > const: usb2-phy1 > const: usb3-phy0 > ... > > I don't think we should be introducing a third naming scheme here so I > suggest just following the nvidia bindings. > In that case, why don't we use "^usb[23]-phy[0-3]$". I think its close to what we have on dwc3 core already today (usb2-phy/usb3-phy). Regards, Krishna,
On 11/11/2023 2:00 PM, Krishna Kurapati PSSNV wrote: > > > On 11/10/2023 6:58 PM, Johan Hovold wrote: >> On Mon, Aug 28, 2023 at 07:00:22PM +0530, Krishna Kurapati wrote: >>> Add bindings to indicate properties required to support multiport >>> on Synopsys DWC3 controller. >>> >>> Suggested-by: Bjorn Andersson <quic_bjorande@quicinc.com> >>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >>> Reviewed-by: Rob Herring <robh@kernel.org> >>> --- >>> .../devicetree/bindings/usb/snps,dwc3.yaml | 13 +++++++------ >>> 1 file changed, 7 insertions(+), 6 deletions(-) >>> >>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml >>> b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml >>> index a696f23730d3..5bc941355b43 100644 >>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml >>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml >>> @@ -85,15 +85,16 @@ properties: >>> phys: >>> minItems: 1 >>> - maxItems: 2 >>> + maxItems: 8 >>> phy-names: >>> minItems: 1 >>> - maxItems: 2 >>> - items: >>> - enum: >>> - - usb2-phy >>> - - usb3-phy >>> + maxItems: 8 >>> + oneOf: >>> + - items: >>> + enum: [ usb2-phy, usb3-phy ] >>> + - items: >>> + pattern: "^usb[23]-port[0-3]$" >> >> Shouldn't this just be >> >> pattern: "^usb[23]-[0-3]$" >> >> so that it matches the names that are used by the nvidia bindings? >> >> We already have some inconsistency in that Amlogic uses a variant based >> on the legacy names that needlessly includes "phy" in the names: >> >> const: usb2-phy0 >> const: usb2-phy1 >> const: usb3-phy0 >> ... >> >> I don't think we should be introducing a third naming scheme here so I >> suggest just following the nvidia bindings. >> > In that case, why don't we use "^usb[23]-phy[0-3]$". I think its close > to what we have on dwc3 core already today (usb2-phy/usb3-phy). > I mean, it isn't needless. It is a phy and shouldn't the binding suggest that and include "-phy" in the name ? Regards, Krishna,
On Sat, Nov 11, 2023 at 03:17:40PM +0530, Krishna Kurapati PSSNV wrote: > On 11/11/2023 2:00 PM, Krishna Kurapati PSSNV wrote: > > On 11/10/2023 6:58 PM, Johan Hovold wrote: > >>> phy-names: > >>> minItems: 1 > >>> - maxItems: 2 > >>> - items: > >>> - enum: > >>> - - usb2-phy > >>> - - usb3-phy > >>> + maxItems: 8 > >>> + oneOf: > >>> + - items: > >>> + enum: [ usb2-phy, usb3-phy ] > >>> + - items: > >>> + pattern: "^usb[23]-port[0-3]$" > >> > >> Shouldn't this just be > >> > >> pattern: "^usb[23]-[0-3]$" > >> > >> so that it matches the names that are used by the nvidia bindings? > >> > >> We already have some inconsistency in that Amlogic uses a variant based > >> on the legacy names that needlessly includes "phy" in the names: > >> > >> const: usb2-phy0 > >> const: usb2-phy1 > >> const: usb3-phy0 > >> ... > >> > >> I don't think we should be introducing a third naming scheme here so I > >> suggest just following the nvidia bindings. > >> > In that case, why don't we use "^usb[23]-phy[0-3]$". I think its close > > to what we have on dwc3 core already today (usb2-phy/usb3-phy). > > I mean, it isn't needless. It is a phy and shouldn't the binding suggest > that and include "-phy" in the name ? No, adding a '-phy' suffix to each name is unnecessary since the property is called 'phy-names'. This is also documented: For names used in {clock,dma,interrupt,reset}-names, do not add any suffix, e.g.: "tx" instead of "txirq" (for interrupt). https://docs.kernel.org/devicetree/bindings/writing-bindings.html and we've already discussed this when I asked you to drop the likewise redundant '_irq' suffix from the interrupt names. Johan
On 11/11/2023 4:25 PM, Johan Hovold wrote: > On Sat, Nov 11, 2023 at 03:17:40PM +0530, Krishna Kurapati PSSNV wrote: >> On 11/11/2023 2:00 PM, Krishna Kurapati PSSNV wrote: >>> On 11/10/2023 6:58 PM, Johan Hovold wrote: > >>>>> phy-names: >>>>> minItems: 1 >>>>> - maxItems: 2 >>>>> - items: >>>>> - enum: >>>>> - - usb2-phy >>>>> - - usb3-phy >>>>> + maxItems: 8 >>>>> + oneOf: >>>>> + - items: >>>>> + enum: [ usb2-phy, usb3-phy ] >>>>> + - items: >>>>> + pattern: "^usb[23]-port[0-3]$" >>>> >>>> Shouldn't this just be >>>> >>>> pattern: "^usb[23]-[0-3]$" >>>> >>>> so that it matches the names that are used by the nvidia bindings? >>>> >>>> We already have some inconsistency in that Amlogic uses a variant based >>>> on the legacy names that needlessly includes "phy" in the names: >>>> >>>> const: usb2-phy0 >>>> const: usb2-phy1 >>>> const: usb3-phy0 >>>> ... >>>> >>>> I don't think we should be introducing a third naming scheme here so I >>>> suggest just following the nvidia bindings. > >>>>> In that case, why don't we use "^usb[23]-phy[0-3]$". I think its close >>> to what we have on dwc3 core already today (usb2-phy/usb3-phy). >> >> I mean, it isn't needless. It is a phy and shouldn't the binding suggest >> that and include "-phy" in the name ? > > No, adding a '-phy' suffix to each name is unnecessary since the > property is called 'phy-names'. > > This is also documented: > > For names used in {clock,dma,interrupt,reset}-names, do not add > any suffix, e.g.: "tx" instead of "txirq" (for interrupt). > > https://docs.kernel.org/devicetree/bindings/writing-bindings.html > Thanks for the explanation. > and we've already discussed this when I asked you to drop the likewise > redundant '_irq' suffix from the interrupt names. Yes, we did discuss this in irq context. But in this case I thought it was fine because we already have usb(2/3)-"phy" already present. When pushing v14, will make this change to usb(2/3)-(0-3) and skip port/phy. Regards, Krishna,
diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml index a696f23730d3..5bc941355b43 100644 --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml @@ -85,15 +85,16 @@ properties: phys: minItems: 1 - maxItems: 2 + maxItems: 8 phy-names: minItems: 1 - maxItems: 2 - items: - enum: - - usb2-phy - - usb3-phy + maxItems: 8 + oneOf: + - items: + enum: [ usb2-phy, usb3-phy ] + - items: + pattern: "^usb[23]-port[0-3]$" power-domains: description: