Re: [PATCH v5 38/44] usb: pci-quirks: handle HAS_IOPORT dependencies

From: Greg Kroah-Hartman
Date: Mon May 29 2023 - 10:35:02 EST

Next message: Guillaume Ranquet: "[PATCH v4 0/8] Add MT8195 HDMI support"
Previous message: Franziska Naepelt: "[PATCH] ARM: ep93xx: Fix whitespace issues"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, May 22, 2023 at 12:50:43PM +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.

This is really messy and hard to review in one commit. I know I got
lost a bunch, and it's still messy:

>
> Co-developed-by: Arnd Bergmann <arnd@xxxxxxxxxx>
> Signed-off-by: Arnd Bergmann <arnd@xxxxxxxxxx>
> Signed-off-by: Niklas Schnelle <schnelle@xxxxxxxxxxxxx>
> ---
> 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..abf8c6cdea9e 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 USB_PCI && 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.

This is fine, make one commit for this, and then we can argue if you
really need it. :)

> +
> 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 */

No #ifdef in .c files if at all possible please. Why is this needed?

And I hate ? : logic, please spell it out.

>
> 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)

Moving these is odd, why? They are just doing pci accesses.

> -{
> - 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)

Is this really needed? This feels wrong, why is ioport odd like this?

> /*
> * 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 */

Again, the changes here are hard to follow, why?

Please break up into smaller pieces.

thanks,

greg k-h

Next message: Guillaume Ranquet: "[PATCH v4 0/8] Add MT8195 HDMI support"
Previous message: Franziska Naepelt: "[PATCH] ARM: ep93xx: Fix whitespace issues"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]