[RFC PATCH 0/2] usb: ehci: Allow ehci accessors to be overridden.

[RFC PATCH 0/2] usb: ehci: Allow ehci accessors to be overridden.

[Daniel Palmer]

In the kernel tree at the moment there is a driver for fotg210.
There was, I think, at some point a driver for the host only
version of this too. With a few tweaks the host part can actually
just use the ehci driver and a bunch of code can be dropped.

The weird ARM SoCs I have been trying to mainline have the host
only version of this IP and have been running with a modified
version of the normal ehci driver for years.

To use the ehci driver we need to be able to override the accessors
for the registers, override the standard locations of a few of them
and hardcode values for some things that are not needed.

Before I clean up and send the rest of this I wanted to show
how I did the overriding of the accessors and how it cleans up the
hack that is already in there. If this way of doing it is acceptable
I'll clean up the whole thing and send it.

The benefit of this is that we should be able to drop one mangled
copy of the ehci driver.

Daniel Palmer (2):
  usb: ehci: Add option to override ehci_readl()/ehci_writel()
  usb: chipidea: use the generic ehci_writel override for i.MX28

 drivers/usb/chipidea/Kconfig |  1 +
 drivers/usb/chipidea/host.c  | 17 ++++++++++++++++-
 drivers/usb/host/Kconfig     |  3 +++
 drivers/usb/host/ehci.h      | 31 ++++++++++++++++---------------
 4 files changed, 36 insertions(+), 16 deletions(-)

-- 
2.53.0

[RFC PATCH 1/2] usb: ehci: Add option to override ehci_readl()/ehci_writel()

[Daniel Palmer]

From: Daniel Palmer <daniel@0x0f.com>

There are EHCI-like hosts, like fotg210, out there that should
be able to use the shared ehci code but can't because they need
to override some parts of the ehci code to work properly.

Let ehci_readl() and ehci_writel() be overridden first.
This allows the ehci code to be used with hosts that need
to do special dances when accessing their registers.

The change is hidden behind a boolean kconfig option
so that the extra function pointer NULL check and call
only happens when a broken controller is enabled.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
---
 drivers/usb/host/Kconfig |  3 +++
 drivers/usb/host/ehci.h  | 16 ++++++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index b3b1ec696bf5..8c88354e91e5 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -362,6 +362,9 @@ config USB_OCTEON_EHCI
 	  USB 2.0 device support.  All CN6XXX based chips with USB are
 	  supported.
 
+config HAVE_BROKEN_EHCI_HCD
+	bool
+
 endif # USB_EHCI_HCD
 
 config USB_OXU210HP_HCD
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index d7a3c8d13f6b..f592cc26e494 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -258,6 +258,14 @@ struct ehci_hcd {			/* one per controller */
 						/* us budgeted per uframe */
 	struct list_head	tt_list;
 
+#ifdef CONFIG_HAVE_BROKEN_EHCI_HCD
+	/* Overrides to fix up broken implementations */
+	/* Broken IO */
+	void (*ehci_writel)(const struct ehci_hcd *ehci,
+			    const unsigned int val, __u32 __iomem *regs);
+	unsigned int (*ehci_readl)(const struct ehci_hcd *ehci, __u32 __iomem *regs);
+#endif
+
 	/* platform-specific data -- must come last */
 	unsigned long		priv[] __aligned(sizeof(s64));
 };
@@ -762,6 +770,10 @@ static inline unsigned int ehci_readl(const struct ehci_hcd *ehci,
 		readl_be(regs) :
 		readl(regs);
 #else
+#ifdef CONFIG_HAVE_BROKEN_EHCI_HCD
+	if (ehci->ehci_readl)
+		return ehci->ehci_readl(ehci, regs);
+#endif
 	return readl(regs);
 #endif
 }
@@ -788,6 +800,10 @@ static inline void ehci_writel(const struct ehci_hcd *ehci,
 #else
 	if (ehci->imx28_write_fix)
 		imx28_ehci_writel(val, regs);
+#ifdef CONFIG_HAVE_BROKEN_EHCI_HCD
+	else if (ehci->ehci_writel)
+		ehci->ehci_writel(ehci, val, regs);
+#endif
 	else
 		writel(val, regs);
 #endif
-- 
2.53.0

[RFC PATCH 2/2] usb: chipidea: use the generic ehci_writel override for i.MX28

[Daniel Palmer]

From: Daniel Palmer <daniel@0x0f.com>

The EHCI core carries an imx28_write_fix flag baked into ehci_writel()
that special-cases the i.MX28's need for a swp instruction when writing
the EHCI registers.

Now that the EHCI core provides a generic ehci_writel override hook,
convert the chipidea host glue to install its own writel via that hook
and drop the i.MX28 special case from the core, keeping ehci_writel()
free of SoC-specific knowledge.

Signed-off-by: Daniel Palmer <daniel@0x0f.com>
---
 drivers/usb/chipidea/Kconfig |  1 +
 drivers/usb/chipidea/host.c  | 17 ++++++++++++++++-
 drivers/usb/host/ehci.h      | 19 ++-----------------
 3 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
index bab45bc62361..5bada84943e2 100644
--- a/drivers/usb/chipidea/Kconfig
+++ b/drivers/usb/chipidea/Kconfig
@@ -29,6 +29,7 @@ config USB_CHIPIDEA_HOST
 	bool "ChipIdea host controller"
 	depends on USB_EHCI_HCD
 	select USB_EHCI_ROOT_HUB_TT
+	select HAVE_BROKEN_EHCI_HCD if SOC_IMX28
 	help
 	  Say Y here to enable host controller functionality of the
 	  ChipIdea driver.
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index ced6076a8248..820bdea5da12 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -117,6 +117,18 @@ static irqreturn_t host_irq(struct ci_hdrc *ci)
 	return usb_hcd_irq(ci->irq, ci->hcd);
 }
 
+#ifdef CONFIG_SOC_IMX28
+/*
+ * The i.MX28 needs a swp instruction rather than a normal write to the
+ * EHCI registers; provide it through the generic ehci_writel override.
+ */
+static void ci_hdrc_ehci_writel(const struct ehci_hcd *ehci,
+				const unsigned int val, __u32 __iomem *regs)
+{
+	asm("swp %0, %0, [%1]" : : "r"(val), "r"(regs));
+}
+#endif
+
 static int host_start(struct ci_hdrc *ci)
 {
 	struct usb_hcd *hcd;
@@ -150,7 +162,10 @@ static int host_start(struct ci_hdrc *ci)
 	ehci->caps = ci->hw_bank.cap;
 	ehci->has_hostpc = ci->hw_bank.lpm;
 	ehci->has_tdi_phy_lpm = ci->hw_bank.lpm;
-	ehci->imx28_write_fix = ci->imx28_write_fix;
+#ifdef CONFIG_SOC_IMX28
+	if (ci->imx28_write_fix)
+		ehci->ehci_writel = ci_hdrc_ehci_writel;
+#endif
 	ehci->has_ci_pec_bug = ci->has_portsc_pec_bug;
 
 	priv = (struct ehci_ci_priv *)ehci->priv;
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index f592cc26e494..616b01314bfe 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -218,7 +218,6 @@ struct ehci_hcd {			/* one per controller */
 	unsigned		has_synopsys_hc_bug:1; /* Synopsys HC */
 	unsigned		frame_index_bug:1; /* MosChip (AKA NetMos) */
 	unsigned		need_oc_pp_cycle:1; /* MPC834X port power */
-	unsigned		imx28_write_fix:1; /* For Freescale i.MX28 */
 	unsigned		spurious_oc:1;
 	unsigned		is_aspeed:1;
 	unsigned		zx_wakeup_clear_needed:1;
@@ -778,18 +777,6 @@ static inline unsigned int ehci_readl(const struct ehci_hcd *ehci,
 #endif
 }
 
-#ifdef CONFIG_SOC_IMX28
-static inline void imx28_ehci_writel(const unsigned int val,
-		volatile __u32 __iomem *addr)
-{
-	__asm__ ("swp %0, %0, [%1]" : : "r"(val), "r"(addr));
-}
-#else
-static inline void imx28_ehci_writel(const unsigned int val,
-		volatile __u32 __iomem *addr)
-{
-}
-#endif
 static inline void ehci_writel(const struct ehci_hcd *ehci,
 		const unsigned int val, __u32 __iomem *regs)
 {
@@ -798,13 +785,11 @@ static inline void ehci_writel(const struct ehci_hcd *ehci,
 		writel_be(val, regs) :
 		writel(val, regs);
 #else
-	if (ehci->imx28_write_fix)
-		imx28_ehci_writel(val, regs);
 #ifdef CONFIG_HAVE_BROKEN_EHCI_HCD
-	else if (ehci->ehci_writel)
+	if (ehci->ehci_writel)
 		ehci->ehci_writel(ehci, val, regs);
-#endif
 	else
+#endif
 		writel(val, regs);
 #endif
 }
-- 
2.53.0

Re: [RFC PATCH 1/2] usb: ehci: Add option to override ehci_readl()/ehci_writel()

[Alan Stern]

On Mon, Jul 13, 2026 at 09:22:04PM +0900, Daniel Palmer wrote:
> From: Daniel Palmer <daniel@0x0f.com>
> 
> There are EHCI-like hosts, like fotg210, out there that should
> be able to use the shared ehci code but can't because they need
> to override some parts of the ehci code to work properly.
> 
> Let ehci_readl() and ehci_writel() be overridden first.
> This allows the ehci code to be used with hosts that need
> to do special dances when accessing their registers.
> 
> The change is hidden behind a boolean kconfig option
> so that the extra function pointer NULL check and call
> only happens when a broken controller is enabled.
> 
> Signed-off-by: Daniel Palmer <daniel@0x0f.com>
> ---
>  drivers/usb/host/Kconfig |  3 +++
>  drivers/usb/host/ehci.h  | 16 ++++++++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index b3b1ec696bf5..8c88354e91e5 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -362,6 +362,9 @@ config USB_OCTEON_EHCI
>  	  USB 2.0 device support.  All CN6XXX based chips with USB are
>  	  supported.
>  
> +config HAVE_BROKEN_EHCI_HCD
> +	bool

That is not a good choice of name; it's far too generic.  EHCI 
controllers can be broken in so many different ways...

How about USB_EHCI_MMIO_OVERRIDES instead?

Also, the way this is implemented below implicitly assumes that the new 
CONFIG flag will never be set at the same time as 
USB_EHCI_BIG_ENDIAN_MMIO.  That should be enforced here.  Even though 
IMX28 doesn't do it, someone else might do it in the future.

> +
>  endif # USB_EHCI_HCD
>  
>  config USB_OXU210HP_HCD
> diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
> index d7a3c8d13f6b..f592cc26e494 100644
> --- a/drivers/usb/host/ehci.h
> +++ b/drivers/usb/host/ehci.h
> @@ -258,6 +258,14 @@ struct ehci_hcd {			/* one per controller */
>  						/* us budgeted per uframe */
>  	struct list_head	tt_list;
>  
> +#ifdef CONFIG_HAVE_BROKEN_EHCI_HCD
> +	/* Overrides to fix up broken implementations */
> +	/* Broken IO */

Similarly, the comment should be improved.  "Overrides to fix up broken 
MMIO implementations".

Alan Stern

> +	void (*ehci_writel)(const struct ehci_hcd *ehci,
> +			    const unsigned int val, __u32 __iomem *regs);
> +	unsigned int (*ehci_readl)(const struct ehci_hcd *ehci, __u32 __iomem *regs);
> +#endif
> +
>  	/* platform-specific data -- must come last */
>  	unsigned long		priv[] __aligned(sizeof(s64));
>  };
> @@ -762,6 +770,10 @@ static inline unsigned int ehci_readl(const struct ehci_hcd *ehci,
>  		readl_be(regs) :
>  		readl(regs);
>  #else
> +#ifdef CONFIG_HAVE_BROKEN_EHCI_HCD
> +	if (ehci->ehci_readl)
> +		return ehci->ehci_readl(ehci, regs);
> +#endif
>  	return readl(regs);
>  #endif
>  }
> @@ -788,6 +800,10 @@ static inline void ehci_writel(const struct ehci_hcd *ehci,
>  #else
>  	if (ehci->imx28_write_fix)
>  		imx28_ehci_writel(val, regs);
> +#ifdef CONFIG_HAVE_BROKEN_EHCI_HCD
> +	else if (ehci->ehci_writel)
> +		ehci->ehci_writel(ehci, val, regs);
> +#endif
>  	else
>  		writel(val, regs);
>  #endif
> -- 
> 2.53.0
> 

Re: [RFC PATCH 1/2] usb: ehci: Add option to override ehci_readl()/ehci_writel()

[Daniel Palmer]

Hi Alan,

On Mon, 13 Jul 2026 at 23:21, Alan Stern <stern@rowland.harvard.edu> wrote:
> > +config HAVE_BROKEN_EHCI_HCD
> > +     bool
>
> That is not a good choice of name; it's far too generic.  EHCI
> controllers can be broken in so many different ways...
>
> How about USB_EHCI_MMIO_OVERRIDES instead?

Noted. Based on that we'd have a config for each piece that needs to
be overridden so drivers can pick the ones they need?

> Also, the way this is implemented below implicitly assumes that the new
> CONFIG flag will never be set at the same time as
> USB_EHCI_BIG_ENDIAN_MMIO.  That should be enforced here.  Even though
> IMX28 doesn't do it, someone else might do it in the future.

Sashiko picked this up as well suggesting there are some pre-existing
issues.. I'll work out what to do here.
I think the situation sashiko was complaining about was imx28 running
a big endian kernel so its workaround never gets applied but I think
that might be impossible. :)

> > +
> >  endif # USB_EHCI_HCD
> >
> >  config USB_OXU210HP_HCD
> > diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
> > index d7a3c8d13f6b..f592cc26e494 100644
> > --- a/drivers/usb/host/ehci.h
> > +++ b/drivers/usb/host/ehci.h
> > @@ -258,6 +258,14 @@ struct ehci_hcd {                        /* one per controller */
> >                                               /* us budgeted per uframe */
> >       struct list_head        tt_list;
> >
> > +#ifdef CONFIG_HAVE_BROKEN_EHCI_HCD
> > +     /* Overrides to fix up broken implementations */
> > +     /* Broken IO */
>
> Similarly, the comment should be improved.  "Overrides to fix up broken
> MMIO implementations".

Noted. My intention was to have all of the brokenness inside the
single #ifdef and then have a comment for what each function pointer
allows you to fix up.
If there is a config per workaround I'll put a comment in each saying
what that workaround is for.

Thanks,

Daniel

Re: [RFC PATCH 1/2] usb: ehci: Add option to override ehci_readl()/ehci_writel()

[Alan Stern]

On Mon, Jul 13, 2026 at 11:37:19PM +0900, Daniel Palmer wrote:
> Hi Alan,
> 
> On Mon, 13 Jul 2026 at 23:21, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > +config HAVE_BROKEN_EHCI_HCD
> > > +     bool
> >
> > That is not a good choice of name; it's far too generic.  EHCI
> > controllers can be broken in so many different ways...
> >
> > How about USB_EHCI_MMIO_OVERRIDES instead?
> 
> Noted. Based on that we'd have a config for each piece that needs to
> be overridden so drivers can pick the ones they need?

Maybe.  What other things will need to be overridden?  It's hard to tell 
what you're planning from just a single sample.

Note that some of the things that are already present in the code can be 
thought of as overrides (although generally not for a broken feature).

Alan Stern

Re: [RFC PATCH 1/2] usb: ehci: Add option to override ehci_readl()/ehci_writel()

[Daniel Palmer]

Hi Alan,

On Mon, 13 Jul 2026 at 23:46, Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Mon, Jul 13, 2026 at 11:37:19PM +0900, Daniel Palmer wrote:
> > Hi Alan,
> >
> > On Mon, 13 Jul 2026 at 23:21, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > +config HAVE_BROKEN_EHCI_HCD
> > > > +     bool
> > >
> > > That is not a good choice of name; it's far too generic.  EHCI
> > > controllers can be broken in so many different ways...
> > >
> > > How about USB_EHCI_MMIO_OVERRIDES instead?
> >
> > Noted. Based on that we'd have a config for each piece that needs to
> > be overridden so drivers can pick the ones they need?
>
> Maybe.  What other things will need to be overridden?  It's hard to tell
> what you're planning from just a single sample.

For the fotg210 at least the location of the port status register
needs to be overridden, it doesn't support sITDs so that has to be
blocked, and the hardware has a bug where the length of ITDs is broken
so it needs to have a workaround to write the value it wants.

Cheers,

Daniel

Re: [RFC PATCH 1/2] usb: ehci: Add option to override ehci_readl()/ehci_writel()

[Alan Stern]

On Tue, Jul 14, 2026 at 12:07:11AM +0900, Daniel Palmer wrote:
> Hi Alan,
> 
> On Mon, 13 Jul 2026 at 23:46, Alan Stern <stern@rowland.harvard.edu> wrote:
> >
> > On Mon, Jul 13, 2026 at 11:37:19PM +0900, Daniel Palmer wrote:
> > > Hi Alan,
> > >
> > > On Mon, 13 Jul 2026 at 23:21, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > > +config HAVE_BROKEN_EHCI_HCD
> > > > > +     bool
> > > >
> > > > That is not a good choice of name; it's far too generic.  EHCI
> > > > controllers can be broken in so many different ways...
> > > >
> > > > How about USB_EHCI_MMIO_OVERRIDES instead?
> > >
> > > Noted. Based on that we'd have a config for each piece that needs to
> > > be overridden so drivers can pick the ones they need?
> >
> > Maybe.  What other things will need to be overridden?  It's hard to tell
> > what you're planning from just a single sample.
> 
> For the fotg210 at least the location of the port status register
> needs to be overridden, it doesn't support sITDs so that has to be
> blocked, and the hardware has a bug where the length of ITDs is broken
> so it needs to have a workaround to write the value it wants.

Okay.  I think it makes sense to have a separate CONFIG flag for each of 
these.

Alan Stern