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/[email protected]/
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/[email protected]/
---
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(-)
--
2.7.4
Add a new quirk for dwc3 core to support writing high-low order.
Signed-off-by: Daehwan Jung <[email protected]>
---
Documentation/devicetree/bindings/usb/snps,dwc3.yaml | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
index 1cd0ca9..56091f4 100644
--- a/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/snps,dwc3.yaml
@@ -254,6 +254,11 @@ properties:
avoid -EPROTO errors with usbhid on some devices (Hikey 970).
type: boolean
+ snps,xhci-write-64-hi-lo-quirk:
+ description:
+ When set, enable quirk for writing in high-low order.
+ type: boolean
+
snps,gfladj-refclk-lpm-sel-quirk:
description:
When set, run the SOF/ITP counter based on ref_clk.
--
2.7.4
This quirk is for the controller that has a limitation in supporting
separate ERSTBA_HI and ERSTBA_LO programming. It's supported when
the ERSTBA is programmed ERSTBA_HI before ERSTBA_LO. That's because
the internal initialization of event ring fetches the
"Event Ring Segment Table Entry" based on the indication of ERSTBA_LO
written.
Signed-off-by: Daehwan Jung <[email protected]>
---
RFC -> v1:
- add a quirk in xhci
- use the quirk for programming ERST high-low order
---
drivers/usb/host/xhci-mem.c | 5 ++++-
drivers/usb/host/xhci.h | 2 ++
2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 3100219..ef768e6 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -2325,7 +2325,10 @@ xhci_add_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir,
erst_base = xhci_read_64(xhci, &ir->ir_set->erst_base);
erst_base &= ERST_BASE_RSVDP;
erst_base |= ir->erst.erst_dma_addr & ~ERST_BASE_RSVDP;
- xhci_write_64(xhci, erst_base, &ir->ir_set->erst_base);
+ if (xhci->quirks & XHCI_WRITE_64_HI_LO)
+ hi_lo_writeq(erst_base, &ir->ir_set->erst_base);
+ else
+ xhci_write_64(xhci, erst_base, &ir->ir_set->erst_base);
/* Set the event ring dequeue address of this interrupter */
xhci_set_hc_event_deq(xhci, ir);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 3041515..8664dd1 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -17,6 +17,7 @@
#include <linux/kernel.h>
#include <linux/usb/hcd.h>
#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/io-64-nonatomic-hi-lo.h>
/* Code sharing between pci-quirks and xhci hcd */
#include "xhci-ext-caps.h"
@@ -1627,6 +1628,7 @@ struct xhci_hcd {
#define XHCI_RESET_TO_DEFAULT BIT_ULL(44)
#define XHCI_ZHAOXIN_TRB_FETCH BIT_ULL(45)
#define XHCI_ZHAOXIN_HOST BIT_ULL(46)
+#define XHCI_WRITE_64_HI_LO BIT_ULL(47)
unsigned int num_active_eps;
unsigned int limit_active_eps;
--
2.7.4
This is set by dwc3 parent node to support writing high-low order.
Signed-off-by: Daehwan Jung <[email protected]>
---
drivers/usb/host/xhci-plat.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 3d071b8..31bdfa5 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -256,6 +256,9 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s
if (device_property_read_bool(tmpdev, "xhci-sg-trb-cache-size-quirk"))
xhci->quirks |= XHCI_SG_TRB_CACHE_SIZE_QUIRK;
+ if (device_property_read_bool(tmpdev, "write-64-hi-lo-quirk"))
+ xhci->quirks |= XHCI_WRITE_64_HI_LO;
+
device_property_read_u32(tmpdev, "imod-interval-ns",
&xhci->imod_interval);
}
--
2.7.4
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).
Best regards,
Krzysztof
On 31/05/2024 08:07, Daehwan Jung wrote:
> This is set by dwc3 parent node to support writing high-low order.
>
> Signed-off-by: Daehwan Jung <[email protected]>
> ---
> drivers/usb/host/xhci-plat.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 3d071b8..31bdfa5 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -256,6 +256,9 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s
> if (device_property_read_bool(tmpdev, "xhci-sg-trb-cache-size-quirk"))
> xhci->quirks |= XHCI_SG_TRB_CACHE_SIZE_QUIRK;
>
> + if (device_property_read_bool(tmpdev, "write-64-hi-lo-quirk"))
> + xhci->quirks |= XHCI_WRITE_64_HI_LO;
Where is any user of this property (DTS)? Just to clarify: your
downstream does not matter really.
Best regards,
Krzysztof
On 03/06/2024 05:44, Jung Daehwan wrote:
> On Fri, May 31, 2024 at 10:12:36AM +0200, Krzysztof Kozlowski wrote:
>> On 31/05/2024 08:07, Daehwan Jung wrote:
>>> This is set by dwc3 parent node to support writing high-low order.
>>>
>>> Signed-off-by: Daehwan Jung <[email protected]>
>>> ---
>>> drivers/usb/host/xhci-plat.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>>> index 3d071b8..31bdfa5 100644
>>> --- a/drivers/usb/host/xhci-plat.c
>>> +++ b/drivers/usb/host/xhci-plat.c
>>> @@ -256,6 +256,9 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s
>>> if (device_property_read_bool(tmpdev, "xhci-sg-trb-cache-size-quirk"))
>>> xhci->quirks |= XHCI_SG_TRB_CACHE_SIZE_QUIRK;
>>>
>>> + if (device_property_read_bool(tmpdev, "write-64-hi-lo-quirk"))
>>> + xhci->quirks |= XHCI_WRITE_64_HI_LO;
>>
>> Where is any user of this property (DTS)? Just to clarify: your
>> downstream does not matter really.
>>
>
> This is set by dwc3 parent node by software node.
>
> [PATCH v2 1/5] dt-bindings: usb: snps,dwc3: Add 'snps,xhci-write-64-hi-lo-quirk' quirk
> https://lore.kernel.org/r/[email protected]/
This is not a patch to DTS.
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 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
On 03/06/2024 10:51, Jung Daehwan wrote:
>>>>>
>>>>> + if (device_property_read_bool(tmpdev, "write-64-hi-lo-quirk"))
>>>>> + xhci->quirks |= XHCI_WRITE_64_HI_LO;
>>>>
>>>> Where is any user of this property (DTS)? Just to clarify: your
>>>> downstream does not matter really.
>>>>
>>>
>>> This is set by dwc3 parent node by software node.
>>>
>>> [PATCH v2 1/5] dt-bindings: usb: snps,dwc3: Add 'snps,xhci-write-64-hi-lo-quirk' quirk
>>> https://lore.kernel.org/r/[email protected]/
>>
>> This is not a patch to DTS.
>>
>>
>
> This is set by software node from dwc3. That's why I think this patch doesn't
> need DTS patch. I would add DTS patch in dwc3 not xhci if it's needed.
>
What?
I asked you question which upstream SoC (link to DTS) uses it, and you
say that "no need for DTS patch"? That's not an answer.
Best regards,
Krzysztof
On 05/06/2024 03:50, Jung Daehwan wrote:
> On Tue, Jun 04, 2024 at 08:20:44AM +0200, Krzysztof Kozlowski wrote:
>> On 03/06/2024 10:51, Jung Daehwan wrote:
>>>>>>>
>>>>>>> + if (device_property_read_bool(tmpdev, "write-64-hi-lo-quirk"))
>>>>>>> + xhci->quirks |= XHCI_WRITE_64_HI_LO;
>>>>>>
>>>>>> Where is any user of this property (DTS)? Just to clarify: your
>>>>>> downstream does not matter really.
>>>>>>
>>>>>
>>>>> This is set by dwc3 parent node by software node.
>>>>>
>>>>> [PATCH v2 1/5] dt-bindings: usb: snps,dwc3: Add 'snps,xhci-write-64-hi-lo-quirk' quirk
>>>>> https://lore.kernel.org/r/[email protected]/
>>>>
>>>> This is not a patch to DTS.
>>>>
>>>>
>>>
>>> This is set by software node from dwc3. That's why I think this patch doesn't
>>> need DTS patch. I would add DTS patch in dwc3 not xhci if it's needed.
>>>
>>
>> What?
>>
>> I asked you question which upstream SoC (link to DTS) uses it, and you
>> say that "no need for DTS patch"? That's not an answer.
>>
>> Best regards,
>> Krzysztof
>>
>>
>
> I'm sorry I didn't get your point. I've been working on Exynos SoC.
> But there's no upstream user of this property yet in this patchset.
That's the point... it makes quite tricky to evaluate whether the
binding is reasonable or not. Upstream your DTS. Otherwise I say this
can be deduced from existing compatible and property is not needed.
Sorry, that's a no.
Best regards,
Krzysztof