Feedback-ID: i56a14606:Fastmail
User-Agent: Cyrus-JMAP/3.9.0-alpha0-221-gec32977366-fm-20230306.001-gec329773
Mime-Version: 1.0
Message-Id: <9e89c4af-7682-4d30-93e7-703fde28180f@app.fastmail.com>
In-Reply-To: <20230314121216.413434-35-schnelle@linux.ibm.com>
References: <20230314121216.413434-1-schnelle@linux.ibm.com>
 <20230314121216.413434-35-schnelle@linux.ibm.com>
Date:   Tue, 14 Mar 2023 15:56:56 +0100
From:   "Arnd Bergmann" <arnd@arndb.de>
To:     "Niklas Schnelle" <schnelle@linux.ibm.com>,
        "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
        "Mathias Nyman" <mathias.nyman@intel.com>,
        "Alan Stern" <stern@rowland.harvard.edu>
Cc:     "Bjorn Helgaas" <bhelgaas@google.com>,
        =?UTF-8?Q?Uwe_Kleine-K=C3=B6nig?= <u.kleine-koenig@pengutronix.de>,
        "Mauro Carvalho Chehab" <mchehab@kernel.org>,
        "Rafael J . Wysocki" <rafael@kernel.org>,
        "Geert Uytterhoeven" <geert@linux-m68k.org>,
        "Paul Walmsley" <paul.walmsley@sifive.com>,
        "Palmer Dabbelt" <palmer@dabbelt.com>,
        "Albert Ou" <aou@eecs.berkeley.edu>, linux-kernel@vger.kernel.org,
        Linux-Arch <linux-arch@vger.kernel.org>,
        linux-pci@vger.kernel.org, "Arnd Bergmann" <arnd@kernel.org>,
        linux-usb@vger.kernel.org
Subject: Re: [PATCH v3 34/38] usb: handle HAS_IOPORT dependencies
Content-Type: text/plain
Precedence: bulk

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