| Message ID | 20241007135508.3143756-2-joychakr@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc3: Add USB3 Gen2 De-emphasis setting from DT | expand |
On 07/10/2024 15:55, Joy Chakraborty wrote: > PIPE4 spec defines an 18bit de-emphasis setting to be passed from > controller to the PHY. > TxDeemph[17:0] is split as [5:0] C-1, [11:6] C0, [17:12] C+1 for 3 tap > filter used for USB Gen2(10GT/s). > > Signed-off-by: Joy Chakraborty <joychakr@google.com> > --- > Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml > index 1cd0ca90127d..a1f1bbcf1467 100644 > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml > @@ -190,6 +190,18 @@ properties: > - 1 # -3.5dB de-emphasis > - 2 # No de-emphasis > > + snps,tx_gen2_de_emphasis_quirk: No underscores. > + description: When set core will set Tx de-emphasis for USB Gen2 And why it cannot be implied by compatible? > + type: boolean > + Best regards, Krzysztof
On Mon, Oct 7, 2024 at 8:26 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On 07/10/2024 15:55, Joy Chakraborty wrote: > > PIPE4 spec defines an 18bit de-emphasis setting to be passed from > > controller to the PHY. > > TxDeemph[17:0] is split as [5:0] C-1, [11:6] C0, [17:12] C+1 for 3 tap > > filter used for USB Gen2(10GT/s). > > > > Signed-off-by: Joy Chakraborty <joychakr@google.com> > > --- > > Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml > > index 1cd0ca90127d..a1f1bbcf1467 100644 > > --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml > > +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml > > @@ -190,6 +190,18 @@ properties: > > - 1 # -3.5dB de-emphasis > > - 2 # No de-emphasis > > > > + snps,tx_gen2_de_emphasis_quirk: > > No underscores. Ack, will fix it with a follow up patch. > > > + description: When set core will set Tx de-emphasis for USB Gen2 > > And why it cannot be implied by compatible? As per my understanding these are tuning coefficients for de-emphasis particular to a platform and not the dwc3 controller, hence should not be a controller compatible. Similar to the property defined right above this definition which is from PIPE3 spec for USB Gen1. Thanks Joy > > > + type: boolean > > + > Best regards, > Krzysztof >
On 08/10/2024 12:23, Joy Chakraborty wrote: > On Mon, Oct 7, 2024 at 8:26 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: >> >> On 07/10/2024 15:55, Joy Chakraborty wrote: >>> PIPE4 spec defines an 18bit de-emphasis setting to be passed from >>> controller to the PHY. >>> TxDeemph[17:0] is split as [5:0] C-1, [11:6] C0, [17:12] C+1 for 3 tap >>> filter used for USB Gen2(10GT/s). >>> >>> Signed-off-by: Joy Chakraborty <joychakr@google.com> >>> --- >>> Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml >>> index 1cd0ca90127d..a1f1bbcf1467 100644 >>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml >>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml >>> @@ -190,6 +190,18 @@ properties: >>> - 1 # -3.5dB de-emphasis >>> - 2 # No de-emphasis >>> >>> + snps,tx_gen2_de_emphasis_quirk: >> >> No underscores. > > Ack, will fix it with a follow up patch. > >> >>> + description: When set core will set Tx de-emphasis for USB Gen2 >> >> And why it cannot be implied by compatible? > > As per my understanding these are tuning coefficients for de-emphasis > particular to a platform and not the dwc3 controller, hence should not > be a controller compatible. Platforms must have specific compatible, so this should be implied by compatible. > Similar to the property defined right above this definition which is > from PIPE3 spec for USB Gen1. Best regards, Krzysztof
On Tue, Oct 8, 2024 at 4:53 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On 08/10/2024 12:23, Joy Chakraborty wrote: > > On Mon, Oct 7, 2024 at 8:26 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > >> > >> On 07/10/2024 15:55, Joy Chakraborty wrote: > >>> PIPE4 spec defines an 18bit de-emphasis setting to be passed from > >>> controller to the PHY. > >>> TxDeemph[17:0] is split as [5:0] C-1, [11:6] C0, [17:12] C+1 for 3 tap > >>> filter used for USB Gen2(10GT/s). > >>> > >>> Signed-off-by: Joy Chakraborty <joychakr@google.com> > >>> --- > >>> Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 12 ++++++++++++ > >>> 1 file changed, 12 insertions(+) > >>> > >>> diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml > >>> index 1cd0ca90127d..a1f1bbcf1467 100644 > >>> --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml > >>> +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml > >>> @@ -190,6 +190,18 @@ properties: > >>> - 1 # -3.5dB de-emphasis > >>> - 2 # No de-emphasis > >>> > >>> + snps,tx_gen2_de_emphasis_quirk: > >> > >> No underscores. > > > > Ack, will fix it with a follow up patch. > > > >> > >>> + description: When set core will set Tx de-emphasis for USB Gen2 > >> > >> And why it cannot be implied by compatible? > > > > As per my understanding these are tuning coefficients for de-emphasis > > particular to a platform and not the dwc3 controller, hence should not > > be a controller compatible. > > Platforms must have specific compatible, so this should be implied by > compatible. Maybe I am using the word "platform" incorrectly here, what I understand is that the same controller(in a chip) when used on 2 different physical form factors might need different deemphasis coefficient values to be passed to its Phy. Someone could correct me from the USB link stand point if I am mistaken here. Thanks Joy > > > Similar to the property defined right above this definition which is > > from PIPE3 spec for USB Gen1. > > > Best regards, > Krzysztof >
On 08/10/2024 13:59, Joy Chakraborty wrote: >>> >>>> >>>>> + description: When set core will set Tx de-emphasis for USB Gen2 >>>> >>>> And why it cannot be implied by compatible? >>> >>> As per my understanding these are tuning coefficients for de-emphasis >>> particular to a platform and not the dwc3 controller, hence should not >>> be a controller compatible. >> >> Platforms must have specific compatible, so this should be implied by >> compatible. > > Maybe I am using the word "platform" incorrectly here, what I > understand is that the same controller(in a chip) when used on 2 > different physical form factors might need different deemphasis You mean boards? This is board-level property? > coefficient values to be passed to its Phy. Someone could correct me > from the USB link stand point if I am mistaken here. > Then please point me to the upstream DTS boards using these properties. Lore link is enough, if board or DTS change is being upstreamed. Best regards, Krzysztof
On Tue, Oct 8, 2024 at 5:40 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On 08/10/2024 13:59, Joy Chakraborty wrote: > >>> > >>>> > >>>>> + description: When set core will set Tx de-emphasis for USB Gen2 > >>>> > >>>> And why it cannot be implied by compatible? > >>> > >>> As per my understanding these are tuning coefficients for de-emphasis > >>> particular to a platform and not the dwc3 controller, hence should not > >>> be a controller compatible. > >> > >> Platforms must have specific compatible, so this should be implied by > >> compatible. > > > > Maybe I am using the word "platform" incorrectly here, what I > > understand is that the same controller(in a chip) when used on 2 > > different physical form factors might need different deemphasis > > You mean boards? This is board-level property? Yes, the USB controller can be paired with different phys in a SoC and used on different board layouts where we should be able to drive different de-emphasis coefficients to the phy as per the link equalization requirements is my understanding. > > > coefficient values to be passed to its Phy. Someone could correct me > > from the USB link stand point if I am mistaken here. > > > > Then please point me to the upstream DTS boards using these properties. > Lore link is enough, if board or DTS change is being upstreamed. > The DTS change is not being upstreamed currently. But this change would help bring up a new or development board where USB compliance is being run and this parameter needs tuning, hence being able to upstream this would help. > Best regards, > Krzysztof >
On 08/10/2024 14:40, Joy Chakraborty wrote: > On Tue, Oct 8, 2024 at 5:40 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: >> >> On 08/10/2024 13:59, Joy Chakraborty wrote: >>>>> >>>>>> >>>>>>> + description: When set core will set Tx de-emphasis for USB Gen2 >>>>>> >>>>>> And why it cannot be implied by compatible? >>>>> >>>>> As per my understanding these are tuning coefficients for de-emphasis >>>>> particular to a platform and not the dwc3 controller, hence should not >>>>> be a controller compatible. >>>> >>>> Platforms must have specific compatible, so this should be implied by >>>> compatible. >>> >>> Maybe I am using the word "platform" incorrectly here, what I >>> understand is that the same controller(in a chip) when used on 2 >>> different physical form factors might need different deemphasis >> >> You mean boards? This is board-level property? > > Yes, the USB controller can be paired with different phys in a SoC and That's not board specific, but SoC. > used on different board layouts where we should be able to drive > different de-emphasis coefficients to the phy as per the link > equalization requirements is my understanding. You keep mixing different stories, so I am not convinced. > >> >>> coefficient values to be passed to its Phy. Someone could correct me >>> from the USB link stand point if I am mistaken here. >>> >> >> Then please point me to the upstream DTS boards using these properties. >> Lore link is enough, if board or DTS change is being upstreamed. >> > > The DTS change is not being upstreamed currently. Why do we want code without any user? > But this change would help bring up a new or development board where > USB compliance is being run and this parameter needs tuning, hence > being able to upstream this would help. To me whatever Google or any other vendor is doing downstream does not matter, just "does not exist". Upstream the DTS so we can verify how this is exactly used. To me it looks so far as SoC specific and your earlier comment about pairing USB controller with phy is confirming this. That's a common practice from Google (but not Chromium folks, they are awesome!) and few other vendors to upstream whatever they have in their GKI downstream, regardless whether it is right or not, whether it follows rules or not, whether there is any user or not (and again: users are upstream). Rationale for all this is the same - "we have downstream some crap thus we want it". Nah, upstream your stuff to be considered as a user. That's a NAK, sorry. Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml index 1cd0ca90127d..a1f1bbcf1467 100644 --- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml +++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml @@ -190,6 +190,18 @@ properties: - 1 # -3.5dB de-emphasis - 2 # No de-emphasis + snps,tx_gen2_de_emphasis_quirk: + description: When set core will set Tx de-emphasis for USB Gen2 + type: boolean + + snps,tx_gen2_de_emphasis: + description: + The 18bit value of Tx deemphasis defined in PIPE4 spec driven to PHY + for normal operation. + $ref: /schemas/types.yaml#/definitions/uint32 + minimum: 0 + maximum: 0x3ffff + snps,dis_u3_susphy_quirk: description: When set core will disable USB3 suspend phy type: boolean
PIPE4 spec defines an 18bit de-emphasis setting to be passed from controller to the PHY. TxDeemph[17:0] is split as [5:0] C-1, [11:6] C0, [17:12] C+1 for 3 tap filter used for USB Gen2(10GT/s). Signed-off-by: Joy Chakraborty <joychakr@google.com> --- Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 12 ++++++++++++ 1 file changed, 12 insertions(+)