| Message ID | 20240201120332.4811-5-rogerq@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: dts: ti: am62: Add USB support for k3-am62p | expand |
On Thu, Feb 01, 2024 at 02:03:31PM +0200, Roger Quadros wrote: > So far this was not required but due to the newly identified > Errata i2409 [1] we need to poke this register space. > > [1] https://www.ti.com/lit/er/sprz487d/sprz487d.pdf > > Signed-off-by: Roger Quadros <rogerq@kernel.org> Acked-by: Conor Dooley <conor.dooley@microchip.com> > --- > > Notes: > Changelog: > > v3 - new patch > > Documentation/devicetree/bindings/usb/ti,am62-usb.yaml | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml b/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml > index fec5651f5602..c02d9d467d9c 100644 > --- a/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml > +++ b/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml > @@ -14,7 +14,9 @@ properties: > const: ti,am62-usb > > reg: > - maxItems: 1 > + items: > + - description: USB CFG register space > + - description: USB PHY2 register space > > ranges: true > > @@ -82,7 +84,8 @@ examples: > > usbss1: usb@f910000 { > compatible = "ti,am62-usb"; > - reg = <0x00 0x0f910000 0x00 0x800>; > + reg = <0x00 0x0f910000 0x00 0x800>, > + <0x00 0x0f918000 0x00 0x400>; Why the double zeros btw? Cheers, Conor. > clocks = <&k3_clks 162 3>; > clock-names = "ref"; > ti,syscon-phy-pll-refclk = <&wkup_conf 0x4018>; > -- > 2.34.1 >
On Thu, Feb 01, 2024 at 06:15:20PM +0000, Conor Dooley wrote: > On Thu, Feb 01, 2024 at 02:03:31PM +0200, Roger Quadros wrote: > > So far this was not required but due to the newly identified > > Errata i2409 [1] we need to poke this register space. > > > > [1] https://www.ti.com/lit/er/sprz487d/sprz487d.pdf > > > > Signed-off-by: Roger Quadros <rogerq@kernel.org> > > Acked-by: Conor Dooley <conor.dooley@microchip.com> Actually, where is the user for this that actually pokes the register space? You're adding another register region, so I went to check how you were handling that in drivers, but there's no driver patch. > > > --- > > > > Notes: > > Changelog: > > > > v3 - new patch > > > > Documentation/devicetree/bindings/usb/ti,am62-usb.yaml | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml b/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml > > index fec5651f5602..c02d9d467d9c 100644 > > --- a/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml > > +++ b/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml > > @@ -14,7 +14,9 @@ properties: > > const: ti,am62-usb > > > > reg: > > - maxItems: 1 > > + items: > > + - description: USB CFG register space > > + - description: USB PHY2 register space > > > > ranges: true > > > > @@ -82,7 +84,8 @@ examples: > > > > usbss1: usb@f910000 { > > compatible = "ti,am62-usb"; > > - reg = <0x00 0x0f910000 0x00 0x800>; > > + reg = <0x00 0x0f910000 0x00 0x800>, > > + <0x00 0x0f918000 0x00 0x400>; > > Why the double zeros btw? > > Cheers, > Conor. > > > clocks = <&k3_clks 162 3>; > > clock-names = "ref"; > > ti,syscon-phy-pll-refclk = <&wkup_conf 0x4018>; > > -- > > 2.34.1 > >
On Thu, Feb 01, 2024 at 06:18:05PM +0000, Conor Dooley wrote: > On Thu, Feb 01, 2024 at 06:15:20PM +0000, Conor Dooley wrote: > > On Thu, Feb 01, 2024 at 02:03:31PM +0200, Roger Quadros wrote: > > > So far this was not required but due to the newly identified > > > Errata i2409 [1] we need to poke this register space. > > > > > > [1] https://www.ti.com/lit/er/sprz487d/sprz487d.pdf > > > > > > Signed-off-by: Roger Quadros <rogerq@kernel.org> > > > > Acked-by: Conor Dooley <conor.dooley@microchip.com> > > Actually, where is the user for this that actually pokes the register > space? > You're adding another register region, so I went to check how you were > handling that in drivers, but there's no driver patch. See Roger's another patch set 'Add workaround for Errata i2409' posted on 16th. -Bin. > > > > > > > --- > > > > > > Notes: > > > Changelog: > > > > > > v3 - new patch > > > > > > Documentation/devicetree/bindings/usb/ti,am62-usb.yaml | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml b/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml > > > index fec5651f5602..c02d9d467d9c 100644 > > > --- a/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml > > > +++ b/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml > > > @@ -14,7 +14,9 @@ properties: > > > const: ti,am62-usb > > > > > > reg: > > > - maxItems: 1 > > > + items: > > > + - description: USB CFG register space > > > + - description: USB PHY2 register space > > > > > > ranges: true > > > > > > @@ -82,7 +84,8 @@ examples: > > > > > > usbss1: usb@f910000 { > > > compatible = "ti,am62-usb"; > > > - reg = <0x00 0x0f910000 0x00 0x800>; > > > + reg = <0x00 0x0f910000 0x00 0x800>, > > > + <0x00 0x0f918000 0x00 0x400>; > > > > Why the double zeros btw? > > > > Cheers, > > Conor. > > > > > clocks = <&k3_clks 162 3>; > > > clock-names = "ref"; > > > ti,syscon-phy-pll-refclk = <&wkup_conf 0x4018>; > > > -- > > > 2.34.1 > > > > >
On Thu, Feb 01, 2024 at 12:35:22PM -0600, Bin Liu wrote: > On Thu, Feb 01, 2024 at 06:18:05PM +0000, Conor Dooley wrote: > > On Thu, Feb 01, 2024 at 06:15:20PM +0000, Conor Dooley wrote: > > > On Thu, Feb 01, 2024 at 02:03:31PM +0200, Roger Quadros wrote: > > > > So far this was not required but due to the newly identified > > > > Errata i2409 [1] we need to poke this register space. > > > > > > > > [1] https://www.ti.com/lit/er/sprz487d/sprz487d.pdf > > > > > > > > Signed-off-by: Roger Quadros <rogerq@kernel.org> > > > > > > Acked-by: Conor Dooley <conor.dooley@microchip.com> > > > > Actually, where is the user for this that actually pokes the register > > space? > > You're adding another register region, so I went to check how you were > > handling that in drivers, but there's no driver patch. > > See Roger's another patch set 'Add workaround for Errata i2409' posted > on 16th. This patch should be with that series, not with these dts patches. Thanks, Conor.
On 01/02/2024 21:13, Conor Dooley wrote: > On Thu, Feb 01, 2024 at 12:35:22PM -0600, Bin Liu wrote: >> On Thu, Feb 01, 2024 at 06:18:05PM +0000, Conor Dooley wrote: >>> On Thu, Feb 01, 2024 at 06:15:20PM +0000, Conor Dooley wrote: >>>> On Thu, Feb 01, 2024 at 02:03:31PM +0200, Roger Quadros wrote: >>>>> So far this was not required but due to the newly identified >>>>> Errata i2409 [1] we need to poke this register space. >>>>> >>>>> [1] https://www.ti.com/lit/er/sprz487d/sprz487d.pdf >>>>> >>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org> >>>> >>>> Acked-by: Conor Dooley <conor.dooley@microchip.com> >>> >>> Actually, where is the user for this that actually pokes the register >>> space? https://lore.kernel.org/all/20240201121220.5523-5-rogerq@kernel.org/ >>> You're adding another register region, so I went to check how you were >>> handling that in drivers, but there's no driver patch. >> >> See Roger's another patch set 'Add workaround for Errata i2409' posted >> on 16th. > > This patch should be with that series, not with these dts patches. > Why not? There should be no dependency between DTS and driver implementation. As DTS and driver will be merged by separate maintainers I thought it would be easier for maintainers this way.
On Fri, Feb 02, 2024 at 11:36:55AM +0200, Roger Quadros wrote: > > > On 01/02/2024 21:13, Conor Dooley wrote: > > On Thu, Feb 01, 2024 at 12:35:22PM -0600, Bin Liu wrote: > >> On Thu, Feb 01, 2024 at 06:18:05PM +0000, Conor Dooley wrote: > >>> On Thu, Feb 01, 2024 at 06:15:20PM +0000, Conor Dooley wrote: > >>>> On Thu, Feb 01, 2024 at 02:03:31PM +0200, Roger Quadros wrote: > >>>>> So far this was not required but due to the newly identified > >>>>> Errata i2409 [1] we need to poke this register space. > >>>>> > >>>>> [1] https://www.ti.com/lit/er/sprz487d/sprz487d.pdf > >>>>> > >>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org> > >>>> > >>>> Acked-by: Conor Dooley <conor.dooley@microchip.com> > >>> > >>> Actually, where is the user for this that actually pokes the register > >>> space? > > https://lore.kernel.org/all/20240201121220.5523-5-rogerq@kernel.org/ > > >>> You're adding another register region, so I went to check how you were > >>> handling that in drivers, but there's no driver patch. > >> > >> See Roger's another patch set 'Add workaround for Errata i2409' posted > >> on 16th. > > > > This patch should be with that series, not with these dts patches. > > > > Why not? There should be no dependency between DTS and driver implementation. > > As DTS and driver will be merged by separate maintainers I thought it > would be easier for maintainers this way. dts and driver might be merged by different people, but dt-bindings and drivers are merged by the same people. This is a bindings patch, not a dts patch. Look at what get_maintainer says for this file: Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM) Rob Herring <robh+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) Conor Dooley <conor+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) Aswath Govindraju <a-govindraju@ti.com> (in file) linux-usb@vger.kernel.org (open list:USB SUBSYSTEM) devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) linux-kernel@vger.kernel.org (open list) Greg and linux-usb are on there, but you have not CCed them. Being with the driver also allows bindings maintainers to check that you don't break backwards compatibility. It also prevents me having to ask for the driver patch, then be given just a subject line that I have to go and look up myself! Thanks, Conor.
On 02/02/2024 11:53, Conor Dooley wrote: > On Fri, Feb 02, 2024 at 11:36:55AM +0200, Roger Quadros wrote: >> >> >> On 01/02/2024 21:13, Conor Dooley wrote: >>> On Thu, Feb 01, 2024 at 12:35:22PM -0600, Bin Liu wrote: >>>> On Thu, Feb 01, 2024 at 06:18:05PM +0000, Conor Dooley wrote: >>>>> On Thu, Feb 01, 2024 at 06:15:20PM +0000, Conor Dooley wrote: >>>>>> On Thu, Feb 01, 2024 at 02:03:31PM +0200, Roger Quadros wrote: >>>>>>> So far this was not required but due to the newly identified >>>>>>> Errata i2409 [1] we need to poke this register space. >>>>>>> >>>>>>> [1] https://www.ti.com/lit/er/sprz487d/sprz487d.pdf >>>>>>> >>>>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org> >>>>>> >>>>>> Acked-by: Conor Dooley <conor.dooley@microchip.com> >>>>> >>>>> Actually, where is the user for this that actually pokes the register >>>>> space? >> >> https://lore.kernel.org/all/20240201121220.5523-5-rogerq@kernel.org/ >> >>>>> You're adding another register region, so I went to check how you were >>>>> handling that in drivers, but there's no driver patch. >>>> >>>> See Roger's another patch set 'Add workaround for Errata i2409' posted >>>> on 16th. >>> >>> This patch should be with that series, not with these dts patches. >>> >> >> Why not? There should be no dependency between DTS and driver implementation. >> >> As DTS and driver will be merged by separate maintainers I thought it >> would be easier for maintainers this way. > > dts and driver might be merged by different people, but dt-bindings and > drivers are merged by the same people. This is a bindings patch, not a If we do that then I get a bunch of dtbs_check warnings dwc3-usb@f900000: reg: [[0, 261095424, 0, 2048], [0, 261128192, 0, 1024]] is too long > dts patch. Look at what get_maintainer says for this file: > Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:USB SUBSYSTEM) > Rob Herring <robh+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > Conor Dooley <conor+dt@kernel.org> (maintainer:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > Aswath Govindraju <a-govindraju@ti.com> (in file) > linux-usb@vger.kernel.org (open list:USB SUBSYSTEM) > devicetree@vger.kernel.org (open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS) > linux-kernel@vger.kernel.org (open list) > Greg and linux-usb are on there, but you have not CCed them. My bad. Will be more careful next time. > > Being with the driver also allows bindings maintainers to check that you > don't break backwards compatibility. It also prevents me having to ask > for the driver patch, then be given just a subject line that I have to > go and look up myself! > Sorry about that. It took a bit longer but I did point you directly to the patch on lore.kernel.org.
On 12:13-20240202, Roger Quadros wrote: [..] > >> > >> As DTS and driver will be merged by separate maintainers I thought it > >> would be easier for maintainers this way. > > > > dts and driver might be merged by different people, but dt-bindings and > > drivers are merged by the same people. This is a bindings patch, not a > > If we do that then I get a bunch of dtbs_check warnings > > dwc3-usb@f900000: reg: [[0, 261095424, 0, 2048], [0, 261128192, 0, 1024]] is too long Just my 2 cents: If the binding (and driver) change was truly backward compatible (which it should be - for example: errata can only be applied if the second property is described), then you want to control that reg property to add minItems? - thatm I think will allow the dts change to come in at the next cycle once the binding has been merged.
On 02/02/2024 14:18, Nishanth Menon wrote: > On 12:13-20240202, Roger Quadros wrote: > [..] >>>> >>>> As DTS and driver will be merged by separate maintainers I thought it >>>> would be easier for maintainers this way. >>> >>> dts and driver might be merged by different people, but dt-bindings and >>> drivers are merged by the same people. This is a bindings patch, not a >> >> If we do that then I get a bunch of dtbs_check warnings >> >> dwc3-usb@f900000: reg: [[0, 261095424, 0, 2048], [0, 261128192, 0, 1024]] is too long > > Just my 2 cents: If the binding (and driver) change was truly backward > compatible (which it should be - for example: errata can only be > applied if the second property is described), then you want to control > that reg property to add minItems? - thatm I think will allow the dts > change to come in at the next cycle once the binding has been merged. > Thanks for the hint. Please drop patches 4 and 5 in case you pick this series. I'll send patch 4 along with the driver series v2. Patch 5, I'll send after the DT binding has been merged.
On 15:59-20240202, Roger Quadros wrote: > > > On 02/02/2024 14:18, Nishanth Menon wrote: > > On 12:13-20240202, Roger Quadros wrote: > > [..] > >>>> > >>>> As DTS and driver will be merged by separate maintainers I thought it > >>>> would be easier for maintainers this way. > >>> > >>> dts and driver might be merged by different people, but dt-bindings and > >>> drivers are merged by the same people. This is a bindings patch, not a > >> > >> If we do that then I get a bunch of dtbs_check warnings > >> > >> dwc3-usb@f900000: reg: [[0, 261095424, 0, 2048], [0, 261128192, 0, 1024]] is too long > > > > Just my 2 cents: If the binding (and driver) change was truly backward > > compatible (which it should be - for example: errata can only be > > applied if the second property is described), then you want to control > > that reg property to add minItems? - thatm I think will allow the dts > > change to come in at the next cycle once the binding has been merged. > > > > Thanks for the hint. > Please drop patches 4 and 5 in case you pick this series. > > I'll send patch 4 along with the driver series v2. > Patch 5, I'll send after the DT binding has been merged. I suggest to resubmit requisite series (with patches +- or what ever) specific to appropriate maintainers (I don't typically like folks sending driver change along with dts change in a single series without indicating to driver maintainer that dts is something they shouldn't be picking) and avoid any confusion ;)
On Fri, Feb 02, 2024 at 12:13:22PM +0200, Roger Quadros wrote: > > > On 02/02/2024 11:53, Conor Dooley wrote: > > On Fri, Feb 02, 2024 at 11:36:55AM +0200, Roger Quadros wrote: > >> > >> > >> On 01/02/2024 21:13, Conor Dooley wrote: > >>> On Thu, Feb 01, 2024 at 12:35:22PM -0600, Bin Liu wrote: > >>>> On Thu, Feb 01, 2024 at 06:18:05PM +0000, Conor Dooley wrote: > >>>>> On Thu, Feb 01, 2024 at 06:15:20PM +0000, Conor Dooley wrote: > >>>>>> On Thu, Feb 01, 2024 at 02:03:31PM +0200, Roger Quadros wrote: > >>>>>>> So far this was not required but due to the newly identified > >>>>>>> Errata i2409 [1] we need to poke this register space. > >>>>>>> > >>>>>>> [1] https://www.ti.com/lit/er/sprz487d/sprz487d.pdf > >>>>>>> > >>>>>>> Signed-off-by: Roger Quadros <rogerq@kernel.org> > >>>>>> > >>>>>> Acked-by: Conor Dooley <conor.dooley@microchip.com> > >>>>> > >>>>> Actually, where is the user for this that actually pokes the register > >>>>> space? > >> > >> https://lore.kernel.org/all/20240201121220.5523-5-rogerq@kernel.org/ > >> > >>>>> You're adding another register region, so I went to check how you were > >>>>> handling that in drivers, but there's no driver patch. > >>>> > >>>> See Roger's another patch set 'Add workaround for Errata i2409' posted > >>>> on 16th. > >>> > >>> This patch should be with that series, not with these dts patches. > >>> > >> > >> Why not? There should be no dependency between DTS and driver implementation. > >> > >> As DTS and driver will be merged by separate maintainers I thought it > >> would be easier for maintainers this way. > > > > dts and driver might be merged by different people, but dt-bindings and > > drivers are merged by the same people. This is a bindings patch, not a > > If we do that then I get a bunch of dtbs_check warnings > > dwc3-usb@f900000: reg: [[0, 261095424, 0, 2048], [0, 261128192, 0, 1024]] is too long I don't know what your platform maintainers view is, but to me it is fine as long as linux-next is clean.
> All AM62 devices have Errata i2409 [1] due to which > USB2 PHY may lock up due to short suspend. > > Workaround involves setting bit 5 and 4 PLL_REG12 > in PHY2 register space after USB controller is brought > out of LPSC reset but before controller initialization. > > Handle this workaround. Tested-by: Andrejs Cainikovs <andrejs.cainikovs@toradex.com> You have requested [1] to check whether this patch fixes issues I observed, and I can confirm it does. [1]: https://lore.kernel.org/all/2629cd30-23aa-4f03-8452-ae13297fd6b6@kernel.org/
diff --git a/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml b/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml index fec5651f5602..c02d9d467d9c 100644 --- a/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml +++ b/Documentation/devicetree/bindings/usb/ti,am62-usb.yaml @@ -14,7 +14,9 @@ properties: const: ti,am62-usb reg: - maxItems: 1 + items: + - description: USB CFG register space + - description: USB PHY2 register space ranges: true @@ -82,7 +84,8 @@ examples: usbss1: usb@f910000 { compatible = "ti,am62-usb"; - reg = <0x00 0x0f910000 0x00 0x800>; + reg = <0x00 0x0f910000 0x00 0x800>, + <0x00 0x0f918000 0x00 0x400>; clocks = <&k3_clks 162 3>; clock-names = "ref"; ti,syscon-phy-pll-refclk = <&wkup_conf 0x4018>;
So far this was not required but due to the newly identified Errata i2409 [1] we need to poke this register space. [1] https://www.ti.com/lit/er/sprz487d/sprz487d.pdf Signed-off-by: Roger Quadros <rogerq@kernel.org> --- Notes: Changelog: v3 - new patch Documentation/devicetree/bindings/usb/ti,am62-usb.yaml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)