[PATCH v4 34/41] usb: add HAS_IOPORT dependencies

[PATCH v4 34/41] usb: add HAS_IOPORT dependencies

[Niklas Schnelle]

In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
not being declared. We thus need to add HAS_IOPORT as dependency for
those drivers using them.

Co-developed-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
Note: The HAS_IOPORT Kconfig option was added in v6.4-rc1 so
      per-subsystem patches may be applied independently

 drivers/usb/host/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index c170672f847e..4448d0ab06f0 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -376,7 +376,7 @@ config USB_ISP116X_HCD
 
 config USB_ISP1362_HCD
 	tristate "ISP1362 HCD support"
-	depends on HAS_IOMEM
+	depends on HAS_IOPORT
 	depends on COMPILE_TEST # nothing uses this
 	help
 	  Supports the Philips ISP1362 chip as a host controller
@@ -578,7 +578,7 @@ endif # USB_OHCI_HCD
 
 config USB_UHCI_HCD
 	tristate "UHCI HCD (most Intel and VIA) support"
-	depends on USB_PCI || USB_UHCI_SUPPORT_NON_PCI_HC
+	depends on (USB_PCI && HAS_IOPORT) || USB_UHCI_SUPPORT_NON_PCI_HC
 	help
 	  The Universal Host Controller Interface is a standard by Intel for
 	  accessing the USB hardware in the PC (which is also called the USB
-- 
2.39.2

[PATCH v4 35/41] usb: uhci: handle HAS_IOPORT dependencies

[Niklas Schnelle]

In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
not being declared. We thus need to guard sections of code calling them
as alternative access methods with CONFIG_HAS_IOPORT checks. For
uhci-hcd there are a lot of I/O port uses that do have MMIO alternatives
all selected by uhci_has_pci_registers() so this can be handled by
UHCI_IN/OUT macros and making uhci_has_pci_registers() constant 0 if
CONFIG_HAS_IOPORT is unset.

Co-developed-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
Note: The HAS_IOPORT Kconfig option was added in v6.4-rc1 so
      per-subsystem patches may be applied independently

 drivers/usb/host/uhci-hcd.c |  2 +-
 drivers/usb/host/uhci-hcd.h | 36 +++++++++++++++++++++++-------------
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
index 7cdc2fa7c28f..fd2408b553cf 100644
--- a/drivers/usb/host/uhci-hcd.c
+++ b/drivers/usb/host/uhci-hcd.c
@@ -841,7 +841,7 @@ static int uhci_count_ports(struct usb_hcd *hcd)
 
 static const char hcd_name[] = "uhci_hcd";
 
-#ifdef CONFIG_USB_PCI
+#if defined(CONFIG_USB_PCI) && defined(CONFIG_HAS_IOPORT)
 #include "uhci-pci.c"
 #define	PCI_DRIVER		uhci_pci_driver
 #endif
diff --git a/drivers/usb/host/uhci-hcd.h b/drivers/usb/host/uhci-hcd.h
index 0688c3e5bfe2..c77705d03ed0 100644
--- a/drivers/usb/host/uhci-hcd.h
+++ b/drivers/usb/host/uhci-hcd.h
@@ -505,41 +505,49 @@ static inline bool uhci_is_aspeed(const struct uhci_hcd *uhci)
  * we use memory mapped registers.
  */
 
+#ifdef CONFIG_HAS_IOPORT
+#define UHCI_IN(x)	x
+#define UHCI_OUT(x)	x
+#else
+#define UHCI_IN(x)	0
+#define UHCI_OUT(x)
+#endif
+
 #ifndef CONFIG_USB_UHCI_SUPPORT_NON_PCI_HC
 /* Support PCI only */
 static inline u32 uhci_readl(const struct uhci_hcd *uhci, int reg)
 {
-	return inl(uhci->io_addr + reg);
+	return UHCI_IN(inl(uhci->io_addr + reg));
 }
 
 static inline void uhci_writel(const struct uhci_hcd *uhci, u32 val, int reg)
 {
-	outl(val, uhci->io_addr + reg);
+	UHCI_OUT(outl(val, uhci->io_addr + reg));
 }
 
 static inline u16 uhci_readw(const struct uhci_hcd *uhci, int reg)
 {
-	return inw(uhci->io_addr + reg);
+	return UHCI_IN(inw(uhci->io_addr + reg));
 }
 
 static inline void uhci_writew(const struct uhci_hcd *uhci, u16 val, int reg)
 {
-	outw(val, uhci->io_addr + reg);
+	UHCI_OUT(outw(val, uhci->io_addr + reg));
 }
 
 static inline u8 uhci_readb(const struct uhci_hcd *uhci, int reg)
 {
-	return inb(uhci->io_addr + reg);
+	return UHCI_IN(inb(uhci->io_addr + reg));
 }
 
 static inline void uhci_writeb(const struct uhci_hcd *uhci, u8 val, int reg)
 {
-	outb(val, uhci->io_addr + reg);
+	UHCI_OUT(outb(val, uhci->io_addr + reg));
 }
 
 #else
 /* Support non-PCI host controllers */
-#ifdef CONFIG_USB_PCI
+#if defined(CONFIG_USB_PCI) && defined(HAS_IOPORT)
 /* Support PCI and non-PCI host controllers */
 #define uhci_has_pci_registers(u)	((u)->io_addr != 0)
 #else
@@ -587,7 +595,7 @@ static inline int uhci_aspeed_reg(unsigned int reg)
 static inline u32 uhci_readl(const struct uhci_hcd *uhci, int reg)
 {
 	if (uhci_has_pci_registers(uhci))
-		return inl(uhci->io_addr + reg);
+		return UHCI_IN(inl(uhci->io_addr + reg));
 	else if (uhci_is_aspeed(uhci))
 		return readl(uhci->regs + uhci_aspeed_reg(reg));
 #ifdef CONFIG_USB_UHCI_BIG_ENDIAN_MMIO
@@ -601,7 +609,7 @@ static inline u32 uhci_readl(const struct uhci_hcd *uhci, int reg)
 static inline void uhci_writel(const struct uhci_hcd *uhci, u32 val, int reg)
 {
 	if (uhci_has_pci_registers(uhci))
-		outl(val, uhci->io_addr + reg);
+		UHCI_OUT(outl(val, uhci->io_addr + reg));
 	else if (uhci_is_aspeed(uhci))
 		writel(val, uhci->regs + uhci_aspeed_reg(reg));
 #ifdef CONFIG_USB_UHCI_BIG_ENDIAN_MMIO
@@ -615,7 +623,7 @@ static inline void uhci_writel(const struct uhci_hcd *uhci, u32 val, int reg)
 static inline u16 uhci_readw(const struct uhci_hcd *uhci, int reg)
 {
 	if (uhci_has_pci_registers(uhci))
-		return inw(uhci->io_addr + reg);
+		return UHCI_IN(inw(uhci->io_addr + reg));
 	else if (uhci_is_aspeed(uhci))
 		return readl(uhci->regs + uhci_aspeed_reg(reg));
 #ifdef CONFIG_USB_UHCI_BIG_ENDIAN_MMIO
@@ -629,7 +637,7 @@ static inline u16 uhci_readw(const struct uhci_hcd *uhci, int reg)
 static inline void uhci_writew(const struct uhci_hcd *uhci, u16 val, int reg)
 {
 	if (uhci_has_pci_registers(uhci))
-		outw(val, uhci->io_addr + reg);
+		UHCI_OUT(outw(val, uhci->io_addr + reg));
 	else if (uhci_is_aspeed(uhci))
 		writel(val, uhci->regs + uhci_aspeed_reg(reg));
 #ifdef CONFIG_USB_UHCI_BIG_ENDIAN_MMIO
@@ -643,7 +651,7 @@ static inline void uhci_writew(const struct uhci_hcd *uhci, u16 val, int reg)
 static inline u8 uhci_readb(const struct uhci_hcd *uhci, int reg)
 {
 	if (uhci_has_pci_registers(uhci))
-		return inb(uhci->io_addr + reg);
+		return UHCI_IN(inb(uhci->io_addr + reg));
 	else if (uhci_is_aspeed(uhci))
 		return readl(uhci->regs + uhci_aspeed_reg(reg));
 #ifdef CONFIG_USB_UHCI_BIG_ENDIAN_MMIO
@@ -657,7 +665,7 @@ static inline u8 uhci_readb(const struct uhci_hcd *uhci, int reg)
 static inline void uhci_writeb(const struct uhci_hcd *uhci, u8 val, int reg)
 {
 	if (uhci_has_pci_registers(uhci))
-		outb(val, uhci->io_addr + reg);
+		UHCI_OUT(outb(val, uhci->io_addr + reg));
 	else if (uhci_is_aspeed(uhci))
 		writel(val, uhci->regs + uhci_aspeed_reg(reg));
 #ifdef CONFIG_USB_UHCI_BIG_ENDIAN_MMIO
@@ -668,6 +676,8 @@ static inline void uhci_writeb(const struct uhci_hcd *uhci, u8 val, int reg)
 		writeb(val, uhci->regs + reg);
 }
 #endif /* CONFIG_USB_UHCI_SUPPORT_NON_PCI_HC */
+#undef UHCI_IN
+#undef UHCI_OUT
 
 /*
  * The GRLIB GRUSBHC controller can use big endian format for its descriptors.
-- 
2.39.2

Re: [PATCH v4 35/41] usb: uhci: handle HAS_IOPORT dependencies

[Greg Kroah-Hartman]

On Tue, May 16, 2023 at 01:00:31PM +0200, Niklas Schnelle wrote:
> In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> not being declared. We thus need to guard sections of code calling them
> as alternative access methods with CONFIG_HAS_IOPORT checks. For
> uhci-hcd there are a lot of I/O port uses that do have MMIO alternatives
> all selected by uhci_has_pci_registers() so this can be handled by
> UHCI_IN/OUT macros and making uhci_has_pci_registers() constant 0 if
> CONFIG_HAS_IOPORT is unset.
> 
> Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
> Note: The HAS_IOPORT Kconfig option was added in v6.4-rc1 so
>       per-subsystem patches may be applied independently
> 
>  drivers/usb/host/uhci-hcd.c |  2 +-
>  drivers/usb/host/uhci-hcd.h | 36 +++++++++++++++++++++++-------------
>  2 files changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
> index 7cdc2fa7c28f..fd2408b553cf 100644
> --- a/drivers/usb/host/uhci-hcd.c
> +++ b/drivers/usb/host/uhci-hcd.c
> @@ -841,7 +841,7 @@ static int uhci_count_ports(struct usb_hcd *hcd)
>  
>  static const char hcd_name[] = "uhci_hcd";
>  
> -#ifdef CONFIG_USB_PCI
> +#if defined(CONFIG_USB_PCI) && defined(CONFIG_HAS_IOPORT)
>  #include "uhci-pci.c"
>  #define	PCI_DRIVER		uhci_pci_driver
>  #endif
> diff --git a/drivers/usb/host/uhci-hcd.h b/drivers/usb/host/uhci-hcd.h
> index 0688c3e5bfe2..c77705d03ed0 100644
> --- a/drivers/usb/host/uhci-hcd.h
> +++ b/drivers/usb/host/uhci-hcd.h
> @@ -505,41 +505,49 @@ static inline bool uhci_is_aspeed(const struct uhci_hcd *uhci)
>   * we use memory mapped registers.
>   */
>  
> +#ifdef CONFIG_HAS_IOPORT
> +#define UHCI_IN(x)	x
> +#define UHCI_OUT(x)	x
> +#else
> +#define UHCI_IN(x)	0
> +#define UHCI_OUT(x)
> +#endif
> +
>  #ifndef CONFIG_USB_UHCI_SUPPORT_NON_PCI_HC
>  /* Support PCI only */
>  static inline u32 uhci_readl(const struct uhci_hcd *uhci, int reg)
>  {
> -	return inl(uhci->io_addr + reg);
> +	return UHCI_IN(inl(uhci->io_addr + reg));
>  }
>  
>  static inline void uhci_writel(const struct uhci_hcd *uhci, u32 val, int reg)
>  {
> -	outl(val, uhci->io_addr + reg);
> +	UHCI_OUT(outl(val, uhci->io_addr + reg));

I'm confused now.

So if CONFIG_HAS_IOPORT is enabled, wonderful, all is good.

But if it isn't, then these are just no-ops that do nothing?  So then
the driver will fail to work?  Why have these stubs at all?

Why not just not build the driver at all if this option is not enabled?

thanks,

greg k-h

Re: [PATCH v4 35/41] usb: uhci: handle HAS_IOPORT dependencies

[Arnd Bergmann]

On Tue, May 16, 2023, at 18:29, Greg Kroah-Hartman wrote:
> On Tue, May 16, 2023 at 01:00:31PM +0200, Niklas Schnelle wrote:

>>  #ifndef CONFIG_USB_UHCI_SUPPORT_NON_PCI_HC
>>  /* Support PCI only */
>>  static inline u32 uhci_readl(const struct uhci_hcd *uhci, int reg)
>>  {
>> -	return inl(uhci->io_addr + reg);
>> +	return UHCI_IN(inl(uhci->io_addr + reg));
>>  }
>>  
>>  static inline void uhci_writel(const struct uhci_hcd *uhci, u32 val, int reg)
>>  {
>> -	outl(val, uhci->io_addr + reg);
>> +	UHCI_OUT(outl(val, uhci->io_addr + reg));
>
> I'm confused now.
>
> So if CONFIG_HAS_IOPORT is enabled, wonderful, all is good.
>
> But if it isn't, then these are just no-ops that do nothing?  So then
> the driver will fail to work?  Why have these stubs at all?
>
> Why not just not build the driver at all if this option is not enabled?

If I remember correctly, the problem here is the lack of
abstractions in the uhci driver, it instead supports all
combinations of on-chip non-PCI devices using readb()/writeb()
and PCI devices using inb()/outb() in a shared codebase.

A particularly tricky combination is a kernel that supports on-chip
UHCI as well as CONFIG_USB_PCI (for EHCI/XHCI) but does not support
I/O ports because of platform limitations. The trick is to come up
with a set of changes that doesn't have to rewrite the entire logic
but also doesn't add an obscene number of #ifdef checks.

That said, there is a minor problem with the empty definition

+#define UHCI_OUT(x)

I think this should be "do { } while (0)" to avoid warnings
about empty if/else blocks.

    Arnd

Re: [PATCH v4 35/41] usb: uhci: handle HAS_IOPORT dependencies

[Alan Stern]

On Tue, May 16, 2023 at 06:44:34PM +0200, Arnd Bergmann wrote:
> On Tue, May 16, 2023, at 18:29, Greg Kroah-Hartman wrote:
> > On Tue, May 16, 2023 at 01:00:31PM +0200, Niklas Schnelle wrote:
> 
> >>  #ifndef CONFIG_USB_UHCI_SUPPORT_NON_PCI_HC
> >>  /* Support PCI only */
> >>  static inline u32 uhci_readl(const struct uhci_hcd *uhci, int reg)
> >>  {
> >> -	return inl(uhci->io_addr + reg);
> >> +	return UHCI_IN(inl(uhci->io_addr + reg));
> >>  }
> >>  
> >>  static inline void uhci_writel(const struct uhci_hcd *uhci, u32 val, int reg)
> >>  {
> >> -	outl(val, uhci->io_addr + reg);
> >> +	UHCI_OUT(outl(val, uhci->io_addr + reg));
> >
> > I'm confused now.
> >
> > So if CONFIG_HAS_IOPORT is enabled, wonderful, all is good.
> >
> > But if it isn't, then these are just no-ops that do nothing?  So then
> > the driver will fail to work?  Why have these stubs at all?
> >
> > Why not just not build the driver at all if this option is not enabled?
> 
> If I remember correctly, the problem here is the lack of
> abstractions in the uhci driver, it instead supports all
> combinations of on-chip non-PCI devices using readb()/writeb()
> and PCI devices using inb()/outb() in a shared codebase.

Isn't that an abstraction?  A single set of operations (uhci_readl(), 
uhci_writel(), etc.) that always does the right sort of I/O even when 
talking to different buses?

So I'm not sure what you mean by "the lack of abstractions".

> A particularly tricky combination is a kernel that supports on-chip
> UHCI as well as CONFIG_USB_PCI (for EHCI/XHCI) but does not support
> I/O ports because of platform limitations. The trick is to come up
> with a set of changes that doesn't have to rewrite the entire logic
> but also doesn't add an obscene number of #ifdef checks.

Indeed, in a kernel supporting that tricky combination the no-op code 
would be generated.  But it would never execute at runtime because the 
uhci_has_pci_registers(uhci) test would always return 0, and so the 
driver wouldn't fail.

> That said, there is a minor problem with the empty definition
> 
> +#define UHCI_OUT(x)
> 
> I think this should be "do { } while (0)" to avoid warnings
> about empty if/else blocks.

I'm sure Niklas wouldn't mind making such a change.  But do we really 
get such warnings?  Does the compiler really think that this kind of 
(macro-expanded) code:

	if (uhci_has_pci_registers(uhci))
		;
	else if (uhci_is_aspeed(uhci))
		writel(val, uhci->regs + uhci_aspeed_reg(reg));

deserves a warning?  I write stuff like that fairly often; it's a good 
way to showcase a high-probability do-nothing pathway at the start of a 
series of conditional cases.  And I haven't noticed any complaints from 
the compiler.

Alan Stern

Re: [PATCH v4 35/41] usb: uhci: handle HAS_IOPORT dependencies

[Niklas Schnelle]

On Tue, 2023-05-16 at 15:51 -0400, Alan Stern wrote:
> On Tue, May 16, 2023 at 06:44:34PM +0200, Arnd Bergmann wrote:
> > On Tue, May 16, 2023, at 18:29, Greg Kroah-Hartman wrote:
> > > On Tue, May 16, 2023 at 01:00:31PM +0200, Niklas Schnelle wrote:
> > 
> > > >  #ifndef CONFIG_USB_UHCI_SUPPORT_NON_PCI_HC
> > > >  /* Support PCI only */
> > > >  static inline u32 uhci_readl(const struct uhci_hcd *uhci, int reg)
> > > >  {
> > > > -	return inl(uhci->io_addr + reg);
> > > > +	return UHCI_IN(inl(uhci->io_addr + reg));
> > > >  }
> > > >  
> > > >  static inline void uhci_writel(const struct uhci_hcd *uhci, u32 val, int reg)
> > > >  {
> > > > -	outl(val, uhci->io_addr + reg);
> > > > +	UHCI_OUT(outl(val, uhci->io_addr + reg));
> > > 
> > > I'm confused now.
> > > 
> > > So if CONFIG_HAS_IOPORT is enabled, wonderful, all is good.
> > > 
> > > But if it isn't, then these are just no-ops that do nothing?  So then
> > > the driver will fail to work?  Why have these stubs at all?
> > > 
> > > Why not just not build the driver at all if this option is not enabled?

The driver supports multiple access methods in several functions
similar to the following:

static inline void uhci_writel(const struct uhci_hcd *uhci, u32 val, int reg)
{
	if (uhci_has_pci_registers(uhci))
		UHCI_OUT(outl(val, uhci->io_addr + reg));
	else if (uhci_is_aspeed(uhci))
		writel(val, uhci->regs + uhci_aspeed_reg(reg));
#ifdef CONFIG_USB_UHCI_BIG_ENDIAN_MMIO
	else if (uhci_big_endian_mmio(uhci))
		writel_be(val, uhci->regs + reg);
#endif
	else
		writel(val, uhci->regs + reg);
}

Instead of adding more #ifdefs Alan Stern suggested to just stub out
both uhci_has_pci_registers() and the access itself. So with a half way
optimizing compiler this shouldn't even leave no-ops in the binary.


> 
> > That said, there is a minor problem with the empty definition
> > 
> > +#define UHCI_OUT(x)
> > 
> > I think this should be "do { } while (0)" to avoid warnings
> > about empty if/else blocks.
> 
> I'm sure Niklas wouldn't mind making such a change.  But do we really 
> get such warnings?  Does the compiler really think that this kind of 
> (macro-expanded) code:
> 
> 	if (uhci_has_pci_registers(uhci))
> 		;
> 	else if (uhci_is_aspeed(uhci))
> 		writel(val, uhci->regs + uhci_aspeed_reg(reg));
> 
> deserves a warning?  I write stuff like that fairly often; it's a good 
> way to showcase a high-probability do-nothing pathway at the start of a 
> series of conditional cases.  And I haven't noticed any complaints from 
> the compiler.
> 
> Alan Stern

I changed it to "do {} while (0)" for v5 but agree I haven't seen
warnings for this either. Still doesn't hurt.

Thanks,
Niklas

Re: [PATCH v4 35/41] usb: uhci: handle HAS_IOPORT dependencies

[Alan Stern]

On Tue, May 16, 2023 at 06:29:56PM +0200, Greg Kroah-Hartman wrote:
> On Tue, May 16, 2023 at 01:00:31PM +0200, Niklas Schnelle wrote:
> > In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> > not being declared. We thus need to guard sections of code calling them
> > as alternative access methods with CONFIG_HAS_IOPORT checks. For
> > uhci-hcd there are a lot of I/O port uses that do have MMIO alternatives
> > all selected by uhci_has_pci_registers() so this can be handled by
> > UHCI_IN/OUT macros and making uhci_has_pci_registers() constant 0 if
> > CONFIG_HAS_IOPORT is unset.
> > 
> > Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> > Signed-off-by: Arnd Bergmann <arnd@kernel.org>
> > Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> > ---
> > Note: The HAS_IOPORT Kconfig option was added in v6.4-rc1 so
> >       per-subsystem patches may be applied independently
> > 
> >  drivers/usb/host/uhci-hcd.c |  2 +-
> >  drivers/usb/host/uhci-hcd.h | 36 +++++++++++++++++++++++-------------
> >  2 files changed, 24 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/usb/host/uhci-hcd.c b/drivers/usb/host/uhci-hcd.c
> > index 7cdc2fa7c28f..fd2408b553cf 100644
> > --- a/drivers/usb/host/uhci-hcd.c
> > +++ b/drivers/usb/host/uhci-hcd.c
> > @@ -841,7 +841,7 @@ static int uhci_count_ports(struct usb_hcd *hcd)
> >  
> >  static const char hcd_name[] = "uhci_hcd";
> >  
> > -#ifdef CONFIG_USB_PCI
> > +#if defined(CONFIG_USB_PCI) && defined(CONFIG_HAS_IOPORT)
> >  #include "uhci-pci.c"
> >  #define	PCI_DRIVER		uhci_pci_driver
> >  #endif
> > diff --git a/drivers/usb/host/uhci-hcd.h b/drivers/usb/host/uhci-hcd.h
> > index 0688c3e5bfe2..c77705d03ed0 100644
> > --- a/drivers/usb/host/uhci-hcd.h
> > +++ b/drivers/usb/host/uhci-hcd.h
> > @@ -505,41 +505,49 @@ static inline bool uhci_is_aspeed(const struct uhci_hcd *uhci)
> >   * we use memory mapped registers.
> >   */
> >  
> > +#ifdef CONFIG_HAS_IOPORT
> > +#define UHCI_IN(x)	x
> > +#define UHCI_OUT(x)	x
> > +#else
> > +#define UHCI_IN(x)	0
> > +#define UHCI_OUT(x)
> > +#endif
> > +
> >  #ifndef CONFIG_USB_UHCI_SUPPORT_NON_PCI_HC
> >  /* Support PCI only */
> >  static inline u32 uhci_readl(const struct uhci_hcd *uhci, int reg)
> >  {
> > -	return inl(uhci->io_addr + reg);
> > +	return UHCI_IN(inl(uhci->io_addr + reg));
> >  }
> >  
> >  static inline void uhci_writel(const struct uhci_hcd *uhci, u32 val, int reg)
> >  {
> > -	outl(val, uhci->io_addr + reg);
> > +	UHCI_OUT(outl(val, uhci->io_addr + reg));
> 
> I'm confused now.
> 
> So if CONFIG_HAS_IOPORT is enabled, wonderful, all is good.
> 
> But if it isn't, then these are just no-ops that do nothing?  So then
> the driver will fail to work?  Why have these stubs at all?
> 
> Why not just not build the driver at all if this option is not enabled?

I should add something to my previous email.  This particular section of 
code is protected by:

#ifndef CONFIG_USB_UHCI_SUPPORT_NON_PCI_HC
/* Support PCI only */

So it gets used only in cases where the driver supports just a PCI bus 
-- no other sorts of non-PCI on-chip devices.  But the preceding patch 
in this series changes the Kconfig file to say:

 config USB_UHCI_HCD
	tristate "UHCI HCD (most Intel and VIA) support"
	depends on (USB_PCI && HAS_IOPORT) || USB_UHCI_SUPPORT_NON_PCI_HC

As a result, when the configuration includes support only for PCI 
controllers the driver won't get built unless HAS_IOPORT is set.  Thus 
the no-op case (in this part of the code) can't arise.

Which is a long-winded way of saying that you're right; the UHCI_IN() 
and UHCI_OUT() wrappers aren't needed in this part of the driver.  I 
guess Niklas put them in either for consistency with the rest of the 
code or because it didn't occur to him that they could be omitted.  (And 
I didn't spot it either.)

Alan Stern

Re: [PATCH v4 35/41] usb: uhci: handle HAS_IOPORT dependencies

[Arnd Bergmann]

On Tue, May 16, 2023, at 22:17, Alan Stern wrote:
> On Tue, May 16, 2023 at 06:29:56PM +0200, Greg Kroah-Hartman wrote:
>> On Tue, May 16, 2023 at 01:00:31PM +0200, Niklas Schnelle wrote:
>
>> I'm confused now.
>> 
>> So if CONFIG_HAS_IOPORT is enabled, wonderful, all is good.
>> 
>> But if it isn't, then these are just no-ops that do nothing?  So then
>> the driver will fail to work?  Why have these stubs at all?
>> 
>> Why not just not build the driver at all if this option is not enabled?
>
> I should add something to my previous email.  This particular section of 
> code is protected by:
>
> #ifndef CONFIG_USB_UHCI_SUPPORT_NON_PCI_HC
> /* Support PCI only */
>
> So it gets used only in cases where the driver supports just a PCI bus 
> -- no other sorts of non-PCI on-chip devices.  But the preceding patch 
> in this series changes the Kconfig file to say:
>
>  config USB_UHCI_HCD
> 	tristate "UHCI HCD (most Intel and VIA) support"
> 	depends on (USB_PCI && HAS_IOPORT) || USB_UHCI_SUPPORT_NON_PCI_HC
>
> As a result, when the configuration includes support only for PCI 
> controllers the driver won't get built unless HAS_IOPORT is set.  Thus 
> the no-op case (in this part of the code) can't arise.

Indeed, that makes sense.

> Which is a long-winded way of saying that you're right; the UHCI_IN() 
> and UHCI_OUT() wrappers aren't needed in this part of the driver.  I 
> guess Niklas put them in either for consistency with the rest of the 
> code or because it didn't occur to him that they could be omitted.  (And 
> I didn't spot it either.)

It's probably less confusing to leave out the PCI-only part of
the patch then and only modify the generic portion.

      Arnd

Re: [PATCH v4 35/41] usb: uhci: handle HAS_IOPORT dependencies

[Niklas Schnelle]

On Wed, 2023-05-17 at 14:17 +0200, Arnd Bergmann wrote:
> On Tue, May 16, 2023, at 22:17, Alan Stern wrote:
> > On Tue, May 16, 2023 at 06:29:56PM +0200, Greg Kroah-Hartman wrote:
> > > On Tue, May 16, 2023 at 01:00:31PM +0200, Niklas Schnelle wrote:
> > 
> > > I'm confused now.
> > > 
> > > So if CONFIG_HAS_IOPORT is enabled, wonderful, all is good.
> > > 
> > > But if it isn't, then these are just no-ops that do nothing?  So then
> > > the driver will fail to work?  Why have these stubs at all?
> > > 
> > > Why not just not build the driver at all if this option is not enabled?
> > 
> > I should add something to my previous email.  This particular section of 
> > code is protected by:
> > 
> > #ifndef CONFIG_USB_UHCI_SUPPORT_NON_PCI_HC
> > /* Support PCI only */
> > 
> > So it gets used only in cases where the driver supports just a PCI bus 
> > -- no other sorts of non-PCI on-chip devices.  But the preceding patch 
> > in this series changes the Kconfig file to say:
> > 
> >  config USB_UHCI_HCD
> > 	tristate "UHCI HCD (most Intel and VIA) support"
> > 	depends on (USB_PCI && HAS_IOPORT) || USB_UHCI_SUPPORT_NON_PCI_HC
> > 
> > As a result, when the configuration includes support only for PCI 
> > controllers the driver won't get built unless HAS_IOPORT is set.  Thus 
> > the no-op case (in this part of the code) can't arise.
> 
> Indeed, that makes sense.
> 
> > Which is a long-winded way of saying that you're right; the UHCI_IN() 
> > and UHCI_OUT() wrappers aren't needed in this part of the driver.  I 
> > guess Niklas put them in either for consistency with the rest of the 
> > code or because it didn't occur to him that they could be omitted.  (And 
> > I didn't spot it either.)
> 
> It's probably less confusing to leave out the PCI-only part of
> the patch then and only modify the generic portion.
> 
>       Arnd

Yes I agree that way the UHCI_IN/OUT() macro is also only used directly
in combination with uhci_has_pci_registers(). I've done this for v5.

Thanks,
Niklas

[PATCH v4 36/41] usb: pci-quirks: handle HAS_IOPORT dependencies

[Niklas Schnelle]

In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
not being declared. In the pci-quirks case the I/O port acceses are
used in the quirks for several AMD south bridges. Move unrelated
ASMEDIA quirks out of the way and introduce an additional config option
for the AMD quirks that depends on HAS_IOPORT.

Co-developed-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
---
Note: The HAS_IOPORT Kconfig option was added in v6.4-rc1 so
      per-subsystem patches may be applied independently

 drivers/usb/Kconfig           |  10 +++
 drivers/usb/core/hcd-pci.c    |   2 +
 drivers/usb/host/pci-quirks.c | 125 ++++++++++++++++++----------------
 drivers/usb/host/pci-quirks.h |  30 ++++++--
 4 files changed, 101 insertions(+), 66 deletions(-)

diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
index 7f33bcc315f2..765093112ed8 100644
--- a/drivers/usb/Kconfig
+++ b/drivers/usb/Kconfig
@@ -91,6 +91,16 @@ config USB_PCI
 	  If you have such a device you may say N here and PCI related code
 	  will not be built in the USB driver.
 
+config USB_PCI_AMD
+	bool "AMD PCI USB host support"
+	depends on HAS_IOPORT
+	default X86 || MACH_LOONGSON64 || PPC_PASEMI
+	help
+	  Enable workarounds for USB implementation quirks in SB600/SB700/SB800
+	  and later south bridge implementations. These are common on x86 PCs
+	  with AMD CPUs but rarely used elsewhere, with the exception of a few
+	  powerpc and mips desktop machines.
+
 if USB
 
 source "drivers/usb/core/Kconfig"
diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index ab2f3737764e..85a0aeae85cd 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -206,8 +206,10 @@ int usb_hcd_pci_probe(struct pci_dev *dev, const struct hc_driver *driver)
 		goto free_irq_vectors;
 	}
 
+#ifdef CONFIG_USB_PCI_AMD
 	hcd->amd_resume_bug = (usb_hcd_amd_remote_wakeup_quirk(dev) &&
 			driver->flags & (HCD_USB11 | HCD_USB3)) ? 1 : 0;
+#endif /* CONFIG_USB_PCI_AMD */
 
 	if (driver->flags & HCD_MEMORY) {
 		/* EHCI, OHCI */
diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
index 2665832f9add..e0612f909fad 100644
--- a/drivers/usb/host/pci-quirks.c
+++ b/drivers/usb/host/pci-quirks.c
@@ -60,6 +60,23 @@
 #define EHCI_USBLEGCTLSTS	4		/* legacy control/status */
 #define EHCI_USBLEGCTLSTS_SOOE	(1 << 13)	/* SMI on ownership change */
 
+/* ASMEDIA quirk use */
+#define ASMT_DATA_WRITE0_REG	0xF8
+#define ASMT_DATA_WRITE1_REG	0xFC
+#define ASMT_CONTROL_REG	0xE0
+#define ASMT_CONTROL_WRITE_BIT	0x02
+#define ASMT_WRITEREG_CMD	0x10423
+#define ASMT_FLOWCTL_ADDR	0xFA30
+#define ASMT_FLOWCTL_DATA	0xBA
+#define ASMT_PSEUDO_DATA	0
+
+/* Intel quirk use */
+#define USB_INTEL_XUSB2PR      0xD0
+#define USB_INTEL_USB2PRM      0xD4
+#define USB_INTEL_USB3_PSSEN   0xD8
+#define USB_INTEL_USB3PRM      0xDC
+
+#ifdef CONFIG_USB_PCI_AMD
 /* AMD quirk use */
 #define	AB_REG_BAR_LOW		0xe0
 #define	AB_REG_BAR_HIGH		0xe1
@@ -93,21 +110,6 @@
 #define	NB_PIF0_PWRDOWN_0	0x01100012
 #define	NB_PIF0_PWRDOWN_1	0x01100013
 
-#define USB_INTEL_XUSB2PR      0xD0
-#define USB_INTEL_USB2PRM      0xD4
-#define USB_INTEL_USB3_PSSEN   0xD8
-#define USB_INTEL_USB3PRM      0xDC
-
-/* ASMEDIA quirk use */
-#define ASMT_DATA_WRITE0_REG	0xF8
-#define ASMT_DATA_WRITE1_REG	0xFC
-#define ASMT_CONTROL_REG	0xE0
-#define ASMT_CONTROL_WRITE_BIT	0x02
-#define ASMT_WRITEREG_CMD	0x10423
-#define ASMT_FLOWCTL_ADDR	0xFA30
-#define ASMT_FLOWCTL_DATA	0xBA
-#define ASMT_PSEUDO_DATA	0
-
 /*
  * amd_chipset_gen values represent AMD different chipset generations
  */
@@ -458,50 +460,6 @@ void usb_amd_quirk_pll_disable(void)
 }
 EXPORT_SYMBOL_GPL(usb_amd_quirk_pll_disable);
 
-static int usb_asmedia_wait_write(struct pci_dev *pdev)
-{
-	unsigned long retry_count;
-	unsigned char value;
-
-	for (retry_count = 1000; retry_count > 0; --retry_count) {
-
-		pci_read_config_byte(pdev, ASMT_CONTROL_REG, &value);
-
-		if (value == 0xff) {
-			dev_err(&pdev->dev, "%s: check_ready ERROR", __func__);
-			return -EIO;
-		}
-
-		if ((value & ASMT_CONTROL_WRITE_BIT) == 0)
-			return 0;
-
-		udelay(50);
-	}
-
-	dev_warn(&pdev->dev, "%s: check_write_ready timeout", __func__);
-	return -ETIMEDOUT;
-}
-
-void usb_asmedia_modifyflowcontrol(struct pci_dev *pdev)
-{
-	if (usb_asmedia_wait_write(pdev) != 0)
-		return;
-
-	/* send command and address to device */
-	pci_write_config_dword(pdev, ASMT_DATA_WRITE0_REG, ASMT_WRITEREG_CMD);
-	pci_write_config_dword(pdev, ASMT_DATA_WRITE1_REG, ASMT_FLOWCTL_ADDR);
-	pci_write_config_byte(pdev, ASMT_CONTROL_REG, ASMT_CONTROL_WRITE_BIT);
-
-	if (usb_asmedia_wait_write(pdev) != 0)
-		return;
-
-	/* send data to device */
-	pci_write_config_dword(pdev, ASMT_DATA_WRITE0_REG, ASMT_FLOWCTL_DATA);
-	pci_write_config_dword(pdev, ASMT_DATA_WRITE1_REG, ASMT_PSEUDO_DATA);
-	pci_write_config_byte(pdev, ASMT_CONTROL_REG, ASMT_CONTROL_WRITE_BIT);
-}
-EXPORT_SYMBOL_GPL(usb_asmedia_modifyflowcontrol);
-
 void usb_amd_quirk_pll_enable(void)
 {
 	usb_amd_quirk_pll(0);
@@ -630,7 +588,53 @@ bool usb_amd_pt_check_port(struct device *device, int port)
 	return !(value & BIT(port_shift));
 }
 EXPORT_SYMBOL_GPL(usb_amd_pt_check_port);
+#endif /* CONFIG_USB_PCI_AMD */
 
+static int usb_asmedia_wait_write(struct pci_dev *pdev)
+{
+	unsigned long retry_count;
+	unsigned char value;
+
+	for (retry_count = 1000; retry_count > 0; --retry_count) {
+
+		pci_read_config_byte(pdev, ASMT_CONTROL_REG, &value);
+
+		if (value == 0xff) {
+			dev_err(&pdev->dev, "%s: check_ready ERROR", __func__);
+			return -EIO;
+		}
+
+		if ((value & ASMT_CONTROL_WRITE_BIT) == 0)
+			return 0;
+
+		udelay(50);
+	}
+
+	dev_warn(&pdev->dev, "%s: check_write_ready timeout", __func__);
+	return -ETIMEDOUT;
+}
+
+void usb_asmedia_modifyflowcontrol(struct pci_dev *pdev)
+{
+	if (usb_asmedia_wait_write(pdev) != 0)
+		return;
+
+	/* send command and address to device */
+	pci_write_config_dword(pdev, ASMT_DATA_WRITE0_REG, ASMT_WRITEREG_CMD);
+	pci_write_config_dword(pdev, ASMT_DATA_WRITE1_REG, ASMT_FLOWCTL_ADDR);
+	pci_write_config_byte(pdev, ASMT_CONTROL_REG, ASMT_CONTROL_WRITE_BIT);
+
+	if (usb_asmedia_wait_write(pdev) != 0)
+		return;
+
+	/* send data to device */
+	pci_write_config_dword(pdev, ASMT_DATA_WRITE0_REG, ASMT_FLOWCTL_DATA);
+	pci_write_config_dword(pdev, ASMT_DATA_WRITE1_REG, ASMT_PSEUDO_DATA);
+	pci_write_config_byte(pdev, ASMT_CONTROL_REG, ASMT_CONTROL_WRITE_BIT);
+}
+EXPORT_SYMBOL_GPL(usb_asmedia_modifyflowcontrol);
+
+#if defined(CONFIG_HAS_IOPORT) && defined(CONFIG_USB_UHCI_HCD)
 /*
  * Make sure the controller is completely inactive, unable to
  * generate interrupts or do DMA.
@@ -711,6 +715,7 @@ int uhci_check_and_reset_hc(struct pci_dev *pdev, unsigned long base)
 	return 1;
 }
 EXPORT_SYMBOL_GPL(uhci_check_and_reset_hc);
+#endif /* defined(CONFIG_HAS_IOPORT && defined(CONFIG_USB_UHCI_HCD) */
 
 static inline int io_type_enabled(struct pci_dev *pdev, unsigned int mask)
 {
@@ -723,6 +728,7 @@ static inline int io_type_enabled(struct pci_dev *pdev, unsigned int mask)
 
 static void quirk_usb_handoff_uhci(struct pci_dev *pdev)
 {
+#ifdef CONFIG_HAS_IOPORT
 	unsigned long base = 0;
 	int i;
 
@@ -737,6 +743,7 @@ static void quirk_usb_handoff_uhci(struct pci_dev *pdev)
 
 	if (base)
 		uhci_check_and_reset_hc(pdev, base);
+#endif /* CONFIG_HAS_IOPORT */
 }
 
 static int mmio_resource_enabled(struct pci_dev *pdev, int idx)
diff --git a/drivers/usb/host/pci-quirks.h b/drivers/usb/host/pci-quirks.h
index e729de21fad7..8c87505f0abc 100644
--- a/drivers/usb/host/pci-quirks.h
+++ b/drivers/usb/host/pci-quirks.h
@@ -2,9 +2,10 @@
 #ifndef __LINUX_USB_PCI_QUIRKS_H
 #define __LINUX_USB_PCI_QUIRKS_H
 
-#ifdef CONFIG_USB_PCI
 void uhci_reset_hc(struct pci_dev *pdev, unsigned long base);
 int uhci_check_and_reset_hc(struct pci_dev *pdev, unsigned long base);
+
+#ifdef CONFIG_USB_PCI_AMD
 int usb_hcd_amd_remote_wakeup_quirk(struct pci_dev *pdev);
 bool usb_amd_hang_symptom_quirk(void);
 bool usb_amd_prefetch_quirk(void);
@@ -12,23 +13,38 @@ void usb_amd_dev_put(void);
 bool usb_amd_quirk_pll_check(void);
 void usb_amd_quirk_pll_disable(void);
 void usb_amd_quirk_pll_enable(void);
-void usb_asmedia_modifyflowcontrol(struct pci_dev *pdev);
-void usb_enable_intel_xhci_ports(struct pci_dev *xhci_pdev);
-void usb_disable_xhci_ports(struct pci_dev *xhci_pdev);
 void sb800_prefetch(struct device *dev, int on);
 bool usb_amd_pt_check_port(struct device *device, int port);
 #else
-struct pci_dev;
+static inline bool usb_amd_hang_symptom_quirk(void)
+{
+	return false;
+};
+static inline bool usb_amd_prefetch_quirk(void)
+{
+	return false;
+}
+static inline bool usb_amd_quirk_pll_check(void)
+{
+	return false;
+}
 static inline void usb_amd_quirk_pll_disable(void) {}
 static inline void usb_amd_quirk_pll_enable(void) {}
-static inline void usb_asmedia_modifyflowcontrol(struct pci_dev *pdev) {}
 static inline void usb_amd_dev_put(void) {}
-static inline void usb_disable_xhci_ports(struct pci_dev *xhci_pdev) {}
 static inline void sb800_prefetch(struct device *dev, int on) {}
 static inline bool usb_amd_pt_check_port(struct device *device, int port)
 {
 	return false;
 }
+#endif /* CONFIG_USB_PCI_AMD */
+
+#ifdef CONFIG_USB_PCI
+void usb_asmedia_modifyflowcontrol(struct pci_dev *pdev);
+void usb_enable_intel_xhci_ports(struct pci_dev *xhci_pdev);
+void usb_disable_xhci_ports(struct pci_dev *xhci_pdev);
+#else
+static inline void usb_asmedia_modifyflowcontrol(struct pci_dev *pdev) {}
+static inline void usb_disable_xhci_ports(struct pci_dev *xhci_pdev) {}
 #endif  /* CONFIG_USB_PCI */
 
 #endif  /*  __LINUX_USB_PCI_QUIRKS_H  */
-- 
2.39.2

Re: [PATCH v4 36/41] usb: pci-quirks: handle HAS_IOPORT dependencies

[Niklas Schnelle]

On Tue, 2023-05-16 at 13:00 +0200, Niklas Schnelle wrote:
> In a future patch HAS_IOPORT=n will result in inb()/outb() and friends
> not being declared. In the pci-quirks case the I/O port acceses are
> used in the quirks for several AMD south bridges. Move unrelated
> ASMEDIA quirks out of the way and introduce an additional config option
> for the AMD quirks that depends on HAS_IOPORT.
> 
> Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>
> ---
> Note: The HAS_IOPORT Kconfig option was added in v6.4-rc1 so
>       per-subsystem patches may be applied independently
> 
>  drivers/usb/Kconfig           |  10 +++
>  drivers/usb/core/hcd-pci.c    |   2 +
>  drivers/usb/host/pci-quirks.c | 125 ++++++++++++++++++----------------
>  drivers/usb/host/pci-quirks.h |  30 ++++++--
>  4 files changed, 101 insertions(+), 66 deletions(-)
> 
> diff --git a/drivers/usb/Kconfig b/drivers/usb/Kconfig
> index 7f33bcc315f2..765093112ed8 100644
> --- a/drivers/usb/Kconfig
> +++ b/drivers/usb/Kconfig
> 
---8<---
>  
>  static inline int io_type_enabled(struct pci_dev *pdev, unsigned int mask)
>  {
> @@ -723,6 +728,7 @@ static inline int io_type_enabled(struct pci_dev *pdev, unsigned int mask)
>  
>  static void quirk_usb_handoff_uhci(struct pci_dev *pdev)
>  {
> +#ifdef CONFIG_HAS_IOPORT
>  	unsigned long base = 0;
>  	int i;
>  
> @@ -737,6 +743,7 @@ static void quirk_usb_handoff_uhci(struct pci_dev *pdev)
>  
>  	if (base)
>  		uhci_check_and_reset_hc(pdev, base);

I got a kernel test robot message for the above function call being
undefined on an ARM config. Will have to investigate the details but I
think this is still missing a stub or an #ifdef here.

> +#endif /* CONFIG_HAS_IOPORT */
>  }
>  
>  static int mmio_resource_enabled(struct pci_dev *pdev, int idx)
---8<---