| Message ID | 1716339839-44022-1-git-send-email-dh10.jung@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] usb: host: xhci-mem: Write high first on erst base of secondary interrupter | expand |
On 22.5.2024 4.03, Daehwan Jung wrote: > ERSTBA_HI should be written first on secondary interrupter. > That's why secondary interrupter could be set while Host Controller > is already running. > > [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 xHCI specification 5.1 "Register Conventions "states that 64 bit registers should be written in low-high order > > [Synopsys]- The internal initialization of event ring fetches > the "Event Ring Segment Table Entry" based on the indication of > ERSTBA_LO written. > Any idea if this is a common issue with this host? Should other 64 bit registers also be written in reverse order. > Signed-off-by: Daehwan Jung <dh10.jung@samsung.com> > --- > drivers/usb/host/xhci-mem.c | 5 ++++- > drivers/usb/host/xhci.h | 6 ++++++ > 2 files changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c > index 3100219..36ee704 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 (intr_num == 0) > + xhci_write_64(xhci, erst_base, &ir->ir_set->erst_base); > + else > + xhci_write_64_r(xhci, erst_base, &ir->ir_set->erst_base); This may cause issues with other hosts expecting low-high order as stated in the specification. If all 64 bit registers should be written in high-low order for this host then maybe set a quirk flag and change xhci_write_64()instead. xhci_write_64(...) { if (xhci->quirks & XHCI_WRITE_64_HI_LO) hi_lo_writeq(val, regs); else lo_hi_writeq(val, regs); } Thanks Mathias
On Wed, May 22, 2024 at 04:40:56PM +0300, Mathias Nyman wrote: > On 22.5.2024 4.03, Daehwan Jung wrote: > >ERSTBA_HI should be written first on secondary interrupter. > >That's why secondary interrupter could be set while Host Controller > >is already running. > > > >[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 > > xHCI specification 5.1 "Register Conventions "states that 64 bit > registers should be written in low-high order > > > > >[Synopsys]- The internal initialization of event ring fetches > >the "Event Ring Segment Table Entry" based on the indication of > >ERSTBA_LO written. > > > > Any idea if this is a common issue with this host? > Should other 64 bit registers also be written in reverse order. > > >Signed-off-by: Daehwan Jung <dh10.jung@samsung.com> > >--- > > drivers/usb/host/xhci-mem.c | 5 ++++- > > drivers/usb/host/xhci.h | 6 ++++++ > > 2 files changed, 10 insertions(+), 1 deletion(-) > > > >diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c > >index 3100219..36ee704 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 (intr_num == 0) > >+ xhci_write_64(xhci, erst_base, &ir->ir_set->erst_base); > >+ else > >+ xhci_write_64_r(xhci, erst_base, &ir->ir_set->erst_base); > > This may cause issues with other hosts expecting low-high order as stated > in the specification. > > If all 64 bit registers should be written in high-low order for this host then > maybe set a quirk flag and change xhci_write_64()instead. > > xhci_write_64(...) > { > if (xhci->quirks & XHCI_WRITE_64_HI_LO) > hi_lo_writeq(val, regs); > else > lo_hi_writeq(val, regs); > } > Mathias, Thanks for the comment. I've seen this issue only writing the base address of ERST. It's better to use a quirk flag as you said. How about using the quirk only in xhci_add_interrupter? @@ -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) xhci_write_64_r(xhci, erst_base, &ir->ir_set->erst_base); else xhci_write_64(xhci, erst_base, &ir->ir_set->erst_base); OR @@ -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) xhci_write_64(xhci, erst_base, &ir->ir_set->erst_base); else xhci_write_64_r(xhci, erst_base, &ir->ir_set->erst_base); } > Thanks > Mathias >
On 23.5.2024 7.43, Jung Daehwan wrote: > On Wed, May 22, 2024 at 04:40:56PM +0300, Mathias Nyman wrote: >> On 22.5.2024 4.03, Daehwan Jung wrote: >>> ERSTBA_HI should be written first on secondary interrupter. >>> That's why secondary interrupter could be set while Host Controller >>> is already running. >>> >>> [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 >> >> xHCI specification 5.1 "Register Conventions "states that 64 bit >> registers should be written in low-high order >> >>> >>> [Synopsys]- The internal initialization of event ring fetches >>> the "Event Ring Segment Table Entry" based on the indication of >>> ERSTBA_LO written. >>> >> >> Any idea if this is a common issue with this host? >> Should other 64 bit registers also be written in reverse order. >> >>> Signed-off-by: Daehwan Jung <dh10.jung@samsung.com> >>> --- >>> drivers/usb/host/xhci-mem.c | 5 ++++- >>> drivers/usb/host/xhci.h | 6 ++++++ >>> 2 files changed, 10 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c >>> index 3100219..36ee704 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 (intr_num == 0) >>> + xhci_write_64(xhci, erst_base, &ir->ir_set->erst_base); >>> + else >>> + xhci_write_64_r(xhci, erst_base, &ir->ir_set->erst_base); >> >> This may cause issues with other hosts expecting low-high order as stated >> in the specification. >> >> If all 64 bit registers should be written in high-low order for this host then >> maybe set a quirk flag and change xhci_write_64()instead. >> >> xhci_write_64(...) >> { >> if (xhci->quirks & XHCI_WRITE_64_HI_LO) >> hi_lo_writeq(val, regs); >> else >> lo_hi_writeq(val, regs); >> } >> > > Mathias, Thanks for the comment. > > I've seen this issue only writing the base address of ERST. > It's better to use a quirk flag as you said. > How about using the quirk only in xhci_add_interrupter? > > @@ -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) > xhci_write_64_r(xhci, erst_base, &ir->ir_set->erst_base); > else > xhci_write_64(xhci, erst_base, &ir->ir_set->erst_base); > This works. Maybe even skip the xhci_write_64_r() helper and just use hi_lo_writeq() directly. Thanks Mathias
On Thu, May 23, 2024 at 04:38:48PM +0300, Mathias Nyman wrote: > On 23.5.2024 7.43, Jung Daehwan wrote: > >On Wed, May 22, 2024 at 04:40:56PM +0300, Mathias Nyman wrote: > >>On 22.5.2024 4.03, Daehwan Jung wrote: > >>>ERSTBA_HI should be written first on secondary interrupter. > >>>That's why secondary interrupter could be set while Host Controller > >>>is already running. > >>> > >>>[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 > >> > >>xHCI specification 5.1 "Register Conventions "states that 64 bit > >>registers should be written in low-high order > >> > >>> > >>>[Synopsys]- The internal initialization of event ring fetches > >>>the "Event Ring Segment Table Entry" based on the indication of > >>>ERSTBA_LO written. > >>> > >> > >>Any idea if this is a common issue with this host? > >>Should other 64 bit registers also be written in reverse order. > >> > >>>Signed-off-by: Daehwan Jung <dh10.jung@samsung.com> > >>>--- > >>> drivers/usb/host/xhci-mem.c | 5 ++++- > >>> drivers/usb/host/xhci.h | 6 ++++++ > >>> 2 files changed, 10 insertions(+), 1 deletion(-) > >>> > >>>diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c > >>>index 3100219..36ee704 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 (intr_num == 0) > >>>+ xhci_write_64(xhci, erst_base, &ir->ir_set->erst_base); > >>>+ else > >>>+ xhci_write_64_r(xhci, erst_base, &ir->ir_set->erst_base); > >> > >>This may cause issues with other hosts expecting low-high order as stated > >>in the specification. > >> > >>If all 64 bit registers should be written in high-low order for this host then > >>maybe set a quirk flag and change xhci_write_64()instead. > >> > >>xhci_write_64(...) > >>{ > >> if (xhci->quirks & XHCI_WRITE_64_HI_LO) > >> hi_lo_writeq(val, regs); > >> else > >> lo_hi_writeq(val, regs); > >>} > >> > > > >Mathias, Thanks for the comment. > > > >I've seen this issue only writing the base address of ERST. > >It's better to use a quirk flag as you said. > >How about using the quirk only in xhci_add_interrupter? > > > >@@ -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) > > xhci_write_64_r(xhci, erst_base, &ir->ir_set->erst_base); > > else > > xhci_write_64(xhci, erst_base, &ir->ir_set->erst_base); > > > > This works. > Maybe even skip the xhci_write_64_r() helper and just use hi_lo_writeq() directly. > > Thanks > Mathias > > Thanks. I will send the quirk patch soon. Best Regards, Jung Daehwan > >
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c index 3100219..36ee704 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 (intr_num == 0) + xhci_write_64(xhci, erst_base, &ir->ir_set->erst_base); + else + xhci_write_64_r(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..7951c0e 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" @@ -1747,6 +1748,11 @@ static inline void xhci_write_64(struct xhci_hcd *xhci, { lo_hi_writeq(val, regs); } +static inline void xhci_write_64_r(struct xhci_hcd *xhci, + const u64 val, __le64 __iomem *regs) +{ + hi_lo_writeq(val, regs); +} static inline int xhci_link_trb_quirk(struct xhci_hcd *xhci) {
ERSTBA_HI should be written first on secondary interrupter. That's why secondary interrupter could be set while Host Controller is already running. [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. Signed-off-by: Daehwan Jung <dh10.jung@samsung.com> --- drivers/usb/host/xhci-mem.c | 5 ++++- drivers/usb/host/xhci.h | 6 ++++++ 2 files changed, 10 insertions(+), 1 deletion(-)