| Message ID | 1717135657-120818-1-git-send-email-dh10.jung@samsung.com |
|---|---|
| Headers | show |
| Series | usb: Add quirk for writing high-low order | expand |
On 31/05/2024 08:07, Daehwan Jung wrote: > xHCI specification 5.1 "Register Conventions" states that 64 bit > registers should be written in low-high order. All writing operations > in xhci is done low-high order following the spec. What is high-low / low-high order? Are you talking about endianness? > > Add a new quirk to support workaround for high-low order. Why? If they should be written low-high, then why breaking the spec? Why this cannot be deduced from compatible? Which *upstream* hardware is affected? Best regards, Krzysztof
On Fri, May 31, 2024 at 10:10:30AM +0200, Krzysztof Kozlowski wrote: > On 31/05/2024 08:07, Daehwan Jung wrote: > > Add a new quirk for dwc3 core to support writing high-low order. > > This does not tell me more. Could be OS property as well... please > describe hardware and provide rationale why this is suitable for > bindings (also cannot be deduced from compatible). > > Hi, I'm sorry I didn't describe it in dt-bindings patches. It's described in cover-letter and other patches except in dt-bindings. I will add it in next submission. I've found out the limitation of Synopsys dwc3 controller. This can work on Host mode using xHCI. A Register related to ERST should be written high-low order not low-high order. Registers are always written low-high order following xHCI spec.(64-bit written is done in each 2 of 32-bit) That's why new quirk is needed for workaround. This quirk is used not in dwc3 controller itself, but passed to xhci quirk eventually. That's because this issue occurs in Host mode using xHCI. Below are answers from Synopsys support center. [Synopsys]- The host controller was design to support ERST setting during the RUN state. But since there is a limitation in controller in supporting separate ERSTBA_HI and ERSTBA_LO programming, It is supported when the ERSTBA is programmed in 64bit, or in 32 bit mode ERSTBA_HI before ERSTBA_LO [Synopsys]- The internal initialization of event ring fetches the "Event Ring Segment Table Entry" based on the indication of ERSTBA_LO written. Best Regards, Jung Daehwan > > Best regards, > Krzysztof > > >
On Fri, May 31, 2024 at 10:12:03AM +0200, Krzysztof Kozlowski wrote: > On 31/05/2024 08:07, Daehwan Jung wrote: > > xHCI specification 5.1 "Register Conventions" states that 64 bit > > registers should be written in low-high order. All writing operations > > in xhci is done low-high order following the spec. > > What is high-low / low-high order? Are you talking about endianness? > It's not about endianness. It means 64 bit is written by 2 of 32 bit. It's when low-high / high-low order is needed. > > > > Add a new quirk to support workaround for high-low order. > > Why? If they should be written low-high, then why breaking the spec? Why > this cannot be deduced from compatible? > This quirk is for the controller that has a limitation in supporting separate ERSTBA_HI and ERSTBA_LO programming. I've copied below from other reply to tell why this is needed. I've found out the limitation of Synopsys dwc3 controller. This can work on Host mode using xHCI. A Register related to ERST should be written high-low order not low-high order. Registers are always written low-high order following xHCI spec.(64-bit written is done in each 2 of 32-bit) That's why new quirk is needed for workaround. This quirk is used not in dwc3 controller itself, but passed to xhci quirk eventually. That's because this issue occurs in Host mode using xHCI. > Which *upstream* hardware is affected? > dwc3 and xhci are affected. Best Regards, Jung Daehwan > > > Best regards, > Krzysztof > > >
On 03/06/2024 05:03, Jung Daehwan wrote: > On Fri, May 31, 2024 at 10:10:30AM +0200, Krzysztof Kozlowski wrote: >> On 31/05/2024 08:07, Daehwan Jung wrote: >>> Add a new quirk for dwc3 core to support writing high-low order. >> >> This does not tell me more. Could be OS property as well... please >> describe hardware and provide rationale why this is suitable for >> bindings (also cannot be deduced from compatible). >> >> > > Hi, > > I'm sorry I didn't describe it in dt-bindings patches. > It's described in cover-letter and other patches except in dt-bindings. > I will add it in next submission. > > I've found out the limitation of Synopsys dwc3 controller. This can work > on Host mode using xHCI. A Register related to ERST should be written > high-low order not low-high order. Registers are always written low-high order > following xHCI spec.(64-bit written is done in each 2 of 32-bit) > That's why new quirk is needed for workaround. This quirk is used not in > dwc3 controller itself, but passed to xhci quirk eventually. That's because > this issue occurs in Host mode using xHCI. > If there is only one register then you should just program it differently and it does not warrant quirk property. Best regards, Krzysztof
On 03/06/2024 05:34, Jung Daehwan wrote: > On Fri, May 31, 2024 at 10:12:03AM +0200, Krzysztof Kozlowski wrote: >> On 31/05/2024 08:07, Daehwan Jung wrote: >>> xHCI specification 5.1 "Register Conventions" states that 64 bit >>> registers should be written in low-high order. All writing operations >>> in xhci is done low-high order following the spec. >> >> What is high-low / low-high order? Are you talking about endianness? >> > > It's not about endianness. It means 64 bit is written by 2 of 32 bit. > It's when low-high / high-low order is needed. > >>> >>> Add a new quirk to support workaround for high-low order. >> >> Why? If they should be written low-high, then why breaking the spec? Why >> this cannot be deduced from compatible? >> > > This quirk is for the controller that has a limitation in supporting > separate ERSTBA_HI and ERSTBA_LO programming. > > I've copied below from other reply to tell why this is needed. > > I've found out the limitation of Synopsys dwc3 controller. This can work > on Host mode using xHCI. A Register related to ERST should be written > high-low order not low-high order. Registers are always written low-high order > following xHCI spec.(64-bit written is done in each 2 of 32-bit) > That's why new quirk is needed for workaround. This quirk is used not in > dwc3 controller itself, but passed to xhci quirk eventually. That's because > this issue occurs in Host mode using xHCI. > >> Which *upstream* hardware is affected? >> > > dwc3 and xhci are affected. Which upstream controllers or SoCs are affected. You did not post any user of this. Best regards, Krzysztof
On Mon, Jun 03, 2024 at 08:58:10AM +0200, Krzysztof Kozlowski wrote: > On 03/06/2024 05:34, Jung Daehwan wrote: > > On Fri, May 31, 2024 at 10:12:03AM +0200, Krzysztof Kozlowski wrote: > >> On 31/05/2024 08:07, Daehwan Jung wrote: > >>> xHCI specification 5.1 "Register Conventions" states that 64 bit > >>> registers should be written in low-high order. All writing operations > >>> in xhci is done low-high order following the spec. > >> > >> What is high-low / low-high order? Are you talking about endianness? > >> > > > > It's not about endianness. It means 64 bit is written by 2 of 32 bit. > > It's when low-high / high-low order is needed. > > > >>> > >>> Add a new quirk to support workaround for high-low order. > >> > >> Why? If they should be written low-high, then why breaking the spec? Why > >> this cannot be deduced from compatible? > >> > > > > This quirk is for the controller that has a limitation in supporting > > separate ERSTBA_HI and ERSTBA_LO programming. > > > > I've copied below from other reply to tell why this is needed. > > > > I've found out the limitation of Synopsys dwc3 controller. This can work > > on Host mode using xHCI. A Register related to ERST should be written > > high-low order not low-high order. Registers are always written low-high order > > following xHCI spec.(64-bit written is done in each 2 of 32-bit) > > That's why new quirk is needed for workaround. This quirk is used not in > > dwc3 controller itself, but passed to xhci quirk eventually. That's because > > this issue occurs in Host mode using xHCI. > > > >> Which *upstream* hardware is affected? > >> > > > > dwc3 and xhci are affected. > > Which upstream controllers or SoCs are affected. You did not post any > user of this. > This issue is not specific on SoC side but dwc3 controller. I think it's better to add it to dwc3 directly but I can't find dts for dwc3. Dts for SoC, which uses dwc3 would be needed in this case, right? Best Regards, Jung Daehwan > Best regards, > Krzysztof > >
On Mon, Jun 03, 2024 at 08:57:16AM +0200, Krzysztof Kozlowski wrote: > On 03/06/2024 05:03, Jung Daehwan wrote: > > On Fri, May 31, 2024 at 10:10:30AM +0200, Krzysztof Kozlowski wrote: > >> On 31/05/2024 08:07, Daehwan Jung wrote: > >>> Add a new quirk for dwc3 core to support writing high-low order. > >> > >> This does not tell me more. Could be OS property as well... please > >> describe hardware and provide rationale why this is suitable for > >> bindings (also cannot be deduced from compatible). > >> > >> > > > > Hi, > > > > I'm sorry I didn't describe it in dt-bindings patches. > > It's described in cover-letter and other patches except in dt-bindings. > > I will add it in next submission. > > > > I've found out the limitation of Synopsys dwc3 controller. This can work > > on Host mode using xHCI. A Register related to ERST should be written > > high-low order not low-high order. Registers are always written low-high order > > following xHCI spec.(64-bit written is done in each 2 of 32-bit) > > That's why new quirk is needed for workaround. This quirk is used not in > > dwc3 controller itself, but passed to xhci quirk eventually. That's because > > this issue occurs in Host mode using xHCI. > > > > If there is only one register then you should just program it > differently and it does not warrant quirk property. > Could you tell me why you think it does not warrant? I think this is good to use quirk. Best Regards, Jung Deahwan > Best regards, > Krzysztof > >
On Fri, May 31, 2024, Daehwan Jung wrote: > Set xhci "write-64-hi-lo-quirk" property via > "snps,xhci-write-64-hi-lo-quirk" property. Please describe the change as if the reader has no context of the other patches in the series. > > Signed-off-by: Daehwan Jung <dh10.jung@samsung.com> > --- Please provide change note for v2. > drivers/usb/dwc3/core.c | 3 +++ > drivers/usb/dwc3/core.h | 2 ++ > drivers/usb/dwc3/host.c | 5 ++++- > 3 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 7ee61a8..89985fd 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1716,6 +1716,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) > dwc->dis_split_quirk = device_property_read_bool(dev, > "snps,dis-split-quirk"); > > + dwc->xhci_write_64_hi_lo_quirk = device_property_read_bool(dev, > + "snps,xhci-write-64-hi-lo-quirk"); > + > dwc->lpm_nyet_threshold = lpm_nyet_threshold; > dwc->tx_de_emphasis = tx_de_emphasis; > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 3781c73..ab5913c 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -1142,6 +1142,7 @@ struct dwc3_scratchpad_array { > * 3 - Reserved > * @dis_metastability_quirk: set to disable metastability quirk. > * @dis_split_quirk: set to disable split boundary. > + * @xhci_write_64_hi_lo_quirk: set if we enable quirk for writing in high-low order. The description should be more detail here. But I don't think we need this. Just pass the PROPERTY_ENTRY_BOOL("write-64-hi-lo-quirk") to xhci platform unconditionally. This should apply to all released versions (at the moment) of DWC_usb3x. > * @sys_wakeup: set if the device may do system wakeup. > * @wakeup_configured: set if the device is configured for remote wakeup. > * @suspended: set to track suspend event due to U3/L2. > @@ -1369,6 +1370,7 @@ struct dwc3 { > unsigned dis_metastability_quirk:1; > > unsigned dis_split_quirk:1; > + unsigned xhci_write_64_hi_lo_quirk:1; > unsigned async_callbacks:1; > unsigned sys_wakeup:1; > unsigned wakeup_configured:1; > diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > index a171b27..8cc0def 100644 > --- a/drivers/usb/dwc3/host.c > +++ b/drivers/usb/dwc3/host.c > @@ -126,7 +126,7 @@ static int dwc3_host_get_irq(struct dwc3 *dwc) > > int dwc3_host_init(struct dwc3 *dwc) > { > - struct property_entry props[5]; > + struct property_entry props[6]; > struct platform_device *xhci; > int ret, irq; > int prop_idx = 0; > @@ -162,6 +162,9 @@ int dwc3_host_init(struct dwc3 *dwc) > > props[prop_idx++] = PROPERTY_ENTRY_BOOL("xhci-sg-trb-cache-size-quirk"); > > + if (dwc->xhci_write_64_hi_lo_quirk) > + props[prop_idx++] = PROPERTY_ENTRY_BOOL("write-64-hi-lo-quirk"); > + > if (dwc->usb3_lpm_capable) > props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb3-lpm-capable"); > > -- > 2.7.4 > Thanks, Thinh
On Tue, Jun 04, 2024 at 12:16:33AM +0000, Thinh Nguyen wrote: > On Fri, May 31, 2024, Daehwan Jung wrote: > > Set xhci "write-64-hi-lo-quirk" property via > > "snps,xhci-write-64-hi-lo-quirk" property. > > Please describe the change as if the reader has no context of the other > patches in the series. > Thanks for the comment. I will add it in next submission. > > > > Signed-off-by: Daehwan Jung <dh10.jung@samsung.com> > > --- > > Please provide change note for v2. I will do it. > > > drivers/usb/dwc3/core.c | 3 +++ > > drivers/usb/dwc3/core.h | 2 ++ > > drivers/usb/dwc3/host.c | 5 ++++- > > 3 files changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index 7ee61a8..89985fd 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -1716,6 +1716,9 @@ static void dwc3_get_properties(struct dwc3 *dwc) > > dwc->dis_split_quirk = device_property_read_bool(dev, > > "snps,dis-split-quirk"); > > > > + dwc->xhci_write_64_hi_lo_quirk = device_property_read_bool(dev, > > + "snps,xhci-write-64-hi-lo-quirk"); > > + > > dwc->lpm_nyet_threshold = lpm_nyet_threshold; > > dwc->tx_de_emphasis = tx_de_emphasis; > > > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > > index 3781c73..ab5913c 100644 > > --- a/drivers/usb/dwc3/core.h > > +++ b/drivers/usb/dwc3/core.h > > @@ -1142,6 +1142,7 @@ struct dwc3_scratchpad_array { > > * 3 - Reserved > > * @dis_metastability_quirk: set to disable metastability quirk. > > * @dis_split_quirk: set to disable split boundary. > > + * @xhci_write_64_hi_lo_quirk: set if we enable quirk for writing in high-low order. > > The description should be more detail here. But I don't think we need > this. Just pass the PROPERTY_ENTRY_BOOL("write-64-hi-lo-quirk") to xhci > platform unconditionally. This should apply to all released versions (at > the moment) of DWC_usb3x. > I got it. If so, I also think it's not needed. I will remove this. > > * @sys_wakeup: set if the device may do system wakeup. > > * @wakeup_configured: set if the device is configured for remote wakeup. > > * @suspended: set to track suspend event due to U3/L2. > > @@ -1369,6 +1370,7 @@ struct dwc3 { > > unsigned dis_metastability_quirk:1; > > > > unsigned dis_split_quirk:1; > > + unsigned xhci_write_64_hi_lo_quirk:1; > > unsigned async_callbacks:1; > > unsigned sys_wakeup:1; > > unsigned wakeup_configured:1; > > diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > > index a171b27..8cc0def 100644 > > --- a/drivers/usb/dwc3/host.c > > +++ b/drivers/usb/dwc3/host.c > > @@ -126,7 +126,7 @@ static int dwc3_host_get_irq(struct dwc3 *dwc) > > > > int dwc3_host_init(struct dwc3 *dwc) > > { > > - struct property_entry props[5]; > > + struct property_entry props[6]; > > struct platform_device *xhci; > > int ret, irq; > > int prop_idx = 0; > > @@ -162,6 +162,9 @@ int dwc3_host_init(struct dwc3 *dwc) > > > > props[prop_idx++] = PROPERTY_ENTRY_BOOL("xhci-sg-trb-cache-size-quirk"); > > > > + if (dwc->xhci_write_64_hi_lo_quirk) > > + props[prop_idx++] = PROPERTY_ENTRY_BOOL("write-64-hi-lo-quirk"); > > + > > if (dwc->usb3_lpm_capable) > > props[prop_idx++] = PROPERTY_ENTRY_BOOL("usb3-lpm-capable"); > > > > -- > > 2.7.4 > > > Best Regards, Jung Daehwan > Thanks, > Thinh
On Tue, Jun 04, 2024 at 12:30:32AM +0000, Thinh Nguyen wrote: > On Mon, Jun 03, 2024, Jung Daehwan wrote: > > > > This issue is not specific on SoC side but dwc3 controller. I think it's > > better to add it to dwc3 directly but I can't find dts for dwc3. Dts for > > SoC, which uses dwc3 would be needed in this case, right? > > > > This quirk applies across different DWC_usb3x versions. IMO, you can > just pass the xhci quirk flag along the dwc3_xhci_plat_quirk->quirks > without needing to introduce a new xhci DTS binding. However, to do > this, you should extract the xhci quirk flags to a separate header so > that dwc3 can include and use them. > > BR, > Thinh I haven't got using dwc3_xhci_plat_quirk. Let me try to use it. Thanks for the comment. Best Regards, Jung Daehwan
On 03/06/2024 10:36, Jung Daehwan wrote: > On Mon, Jun 03, 2024 at 08:57:16AM +0200, Krzysztof Kozlowski wrote: >> On 03/06/2024 05:03, Jung Daehwan wrote: >>> On Fri, May 31, 2024 at 10:10:30AM +0200, Krzysztof Kozlowski wrote: >>>> On 31/05/2024 08:07, Daehwan Jung wrote: >>>>> Add a new quirk for dwc3 core to support writing high-low order. >>>> >>>> This does not tell me more. Could be OS property as well... please >>>> describe hardware and provide rationale why this is suitable for >>>> bindings (also cannot be deduced from compatible). >>>> >>>> >>> >>> Hi, >>> >>> I'm sorry I didn't describe it in dt-bindings patches. >>> It's described in cover-letter and other patches except in dt-bindings. >>> I will add it in next submission. >>> >>> I've found out the limitation of Synopsys dwc3 controller. This can work >>> on Host mode using xHCI. A Register related to ERST should be written >>> high-low order not low-high order. Registers are always written low-high order >>> following xHCI spec.(64-bit written is done in each 2 of 32-bit) >>> That's why new quirk is needed for workaround. This quirk is used not in >>> dwc3 controller itself, but passed to xhci quirk eventually. That's because >>> this issue occurs in Host mode using xHCI. >>> >> >> If there is only one register then you should just program it >> differently and it does not warrant quirk property. >> > > Could you tell me why you think it does not warrant? I think this is > good to use quirk. Because it is a fixed case, deduced from compatible. No need for a property for this. Best regards, Krzysztof
There's the limitation of Synopsys dwc3 controller with ERST programming in supporting separate ERSTBA_HI and ERSTBA_LO programming. It's supported when the ERSTBA is programmed ERSTBA_HI before ERSTBA_LO. But, writing operations in xHCI is done low-high order following xHCI spec. xHCI specification 5.1 "Register Conventions" states that 64 bit registers should be written in low-high order. Synopsys dwc3 needs workaround for high-low order. That's why I add new quirk to support this. --- Changes in v2: - add a quirk in dwc3 - add dt-bindings of dwc3/xhci - set the quirk in xhci-plat from dwc3 Link to v1: https://lore.kernel.org/r/1716875836-186791-1-git-send-email-dh10.jung@samsung.com/ Changes in v1: - add a quirk in xhci - use the quirk for programming ERST high-low order Link to RFC: https://lore.kernel.org/r/1716339839-44022-1-git-send-email-dh10.jung@samsung.com/ --- Daehwan Jung (5): dt-bindings: usb: snps,dwc3: Add 'snps,xhci-write-64-hi-lo-quirk' quirk usb: dwc3: Support quirk for writing high-low order dt-bindings: usb: xhci: Add 'write-64-hi-lo-quirk' quirk xhci: Add a quirk for writing ERST in high-low order usb: host: xhci-plat: Add support for XHCI_WRITE_64_HI_LO_QUIRK Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 5 +++++ Documentation/devicetree/bindings/usb/usb-xhci.yaml | 4 ++++ drivers/usb/dwc3/core.c | 3 +++ drivers/usb/dwc3/core.h | 2 ++ drivers/usb/dwc3/host.c | 5 ++++- drivers/usb/host/xhci-mem.c | 5 ++++- drivers/usb/host/xhci-plat.c | 3 +++ drivers/usb/host/xhci.h | 2 ++ 8 files changed, 27 insertions(+), 2 deletions(-)