Re: [PATCH v3 34/38] usb: handle HAS_IOPORT dependencies

["Arnd Bergmann" <arnd@arndb.de>]

On Tue, Mar 14, 2023, at 13:12, 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. Similarly
> drivers requiring these functions need to depend on HAS_IOPORT.
>
> Co-developed-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Niklas Schnelle <schnelle@linux.ibm.com>

I would suggest splitting this patch up into separate bits for the AMD
quirks and the UHCI driver, possibly more if there are other parts
unrelated to that.

> +#ifdef CONFIG_HAS_IOPORT
>  /*
>   * Make sure the controller is completely inactive, unable to
>   * generate interrupts or do DMA.

Maybe check for both HAS_IOPORT and USB_UHCI_HCD ?

>  static inline int io_type_enabled(struct pci_dev *pdev, unsigned int 
> mask)
>  {
> @@ -725,6 +730,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 HAS_IOPORT
>  	unsigned long base = 0;
>  	int i;
> 
> @@ -739,6 +745,7 @@ static void quirk_usb_handoff_uhci(struct pci_dev *pdev)
> 
>  	if (base)
>  		uhci_check_and_reset_hc(pdev, base);
> +#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));
>  }
> 

This looks a bit ugly, though I can't think of a version I
really like here though. Possibly merging this together with the
generic version would result in something better, like

#if defined(CONFIG_USB_UHCI_PCI) && defined(CONFIG_USB_UHCI_SUPPORT_NON_PCI_HC)
/* Support PCI and non-PCI host controllers */
#define uhci_has_pci_registers(u)       ((u)->io_addr != 0)
#elif defined(CONFIG_USB_UHCI_PCI)
#define uhci_has_pci_registers(u)       1
#else           
/* Support non-PCI host controllers only */
#define uhci_has_pci_registers(u)       0
#endif 

static inline u32 uhci_readl(const struct uhci_hcd *uhci, int reg)
{
#ifdef CONfIG_USB_UHCI_PCI
        if (uhci_has_pci_registers(uhci))
                return inl(uhci->io_addr + reg);
        else
#endif
        if (uhci_is_aspeed(uhci))
                return readl(uhci->regs + uhci_aspeed_reg(reg));
        else
#ifdef CONFIG_USB_UHCI_BIG_ENDIAN_MMIO
        if (uhci_big_endian_mmio(uhci))
                return readl_be(uhci->regs + reg);
        else
#endif
        return readl(uhci->regs + reg);
}

Obviously still ugly, not sure if anyone can come up with a better
version.

      Arnd