On Wed, Nov 22, 2017 at 05:45:14PM +0800, Joe Lee wrote:
> For AMD Promontory xHCI host,
> although you can disable USB 2.0 ports in BIOSsettings,
> those ports will be enabled anyway after you remove a device on
> that port and re-plug it in again. It's a known limitation of the chip.
> As a workaround we can clear the PORT_WAKE_BITS.
>
> Signed-off-by: Joe Lee <asmt.swfae@xxxxxxxxx>
Minor coding style nits below. Did you run the patch through
scripts/checkpatch.pl?
> ---
> v7: add a empty function for the case
> when CONFIG_USB_PCI is not defined in pci-quirks.h
> v6: Fix coding format.
> v5: Add check disable port setting before set PORT_WAKE_BITS
> v4: Remove the patch code in case USB_PORT_FEAT_REMOTE_WAKE_MASK
> v3: Fix some checkpatch.pl
> ---
> drivers/usb/host/pci-quirks.c | 118 ++++++++++++++++++++++++++++++++++++++++++
> drivers/usb/host/pci-quirks.h | 2 +
> drivers/usb/host/xhci-hub.c | 6 +++
> drivers/usb/host/xhci-pci.c | 12 +++++
> drivers/usb/host/xhci.h | 2 +-
> 5 files changed, 139 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/host/pci-quirks.c b/drivers/usb/host/pci-quirks.c
> index 6dda362..6909d05 100644
> --- a/drivers/usb/host/pci-quirks.c
> +++ b/drivers/usb/host/pci-quirks.c
> @@ -65,6 +65,23 @@
> #define AX_INDXC 0x30
> #define AX_DATAC 0x34
>
> +#define PT_ADDR_INDEX 0xE8
> +#define PT_READ_INDEX 0xE4
> +#define PT_SIG_1_ADDR 0xA520
> +#define PT_SIG_2_ADDR 0xA521
> +#define PT_SIG_3_ADDR 0xA522
> +#define PT_SIG_4_ADDR 0xA523
> +#define PT_SIG_1_DATA 0x78
> +#define PT_SIG_2_DATA 0x56
> +#define PT_SIG_3_DATA 0x34
> +#define PT_SIG_4_DATA 0x12
> +#define PT_4_PORT_1_REG 0xB521
> +#define PT_4_PORT_2_REG 0xB522
> +#define PT_2_PORT_1_REG 0xD520
> +#define PT_2_PORT_2_REG 0xD521
> +#define PT_1_PORT_1_REG 0xD522
> +#define PT_1_PORT_2_REG 0xD523
> +
> #define NB_PCIE_INDX_ADDR 0xe0
> #define NB_PCIE_INDX_DATA 0xe4
> #define PCIE_P_CNTL 0x10040
> @@ -511,6 +528,107 @@ void usb_amd_dev_put(void)
> }
> EXPORT_SYMBOL_GPL(usb_amd_dev_put);
>
> +int usb_amd_pt_check_port(struct device *device, int port)
Should this function return a bool?
> +{
> + unsigned char value;
> + struct pci_dev *pdev;
> +
> + pdev = to_pci_dev(device);
> + pci_write_config_word(pdev, PT_ADDR_INDEX, PT_SIG_1_ADDR);
> + pci_read_config_byte(pdev, PT_READ_INDEX, &value);
> + if (value != PT_SIG_1_DATA)
> + return 0;
> + pci_write_config_word(pdev, PT_ADDR_INDEX, PT_SIG_2_ADDR);
> + pci_read_config_byte(pdev, PT_READ_INDEX, &value);
> + if (value != PT_SIG_2_DATA)
> + return 0;
> + pci_write_config_word(pdev, PT_ADDR_INDEX, PT_SIG_3_ADDR);
> + pci_read_config_byte(pdev, PT_READ_INDEX, &value);
> + if (value != PT_SIG_3_DATA)
> + return 0;
> + pci_write_config_word(pdev, PT_ADDR_INDEX, PT_SIG_4_ADDR);
> + pci_read_config_byte(pdev, PT_READ_INDEX, &value);
> + if (value != PT_SIG_4_DATA)
> + return 0;
> + if ((pdev->device == 0x43b9) || (pdev->device == 0x43ba)) {
> + /* device is AMD_PROMONTORYA_4(0x43b9) or
> + * PROMONTORYA_3(0x43ba)
> + * disable port setting PT_4_PORT_1_REG[7..1] is
> + * USB2.0 port6 to 0
> + * PT_4_PORT_2_REG[6..0] is USB 13 to port 7
> + * 0 : disable ;1 : enable
> + */
Odd comment style, please match the other multi-line comment style that
is in this file.
> + if (port > 6) {
> + pci_write_config_word(pdev, PT_ADDR_INDEX,
> + PT_4_PORT_2_REG);
> + pci_read_config_byte(pdev, PT_READ_INDEX, &value);
> + if (value & (1<<(port - 7)))
> + return 0;
> + else
> + return 1;
> + } else {
> + pci_write_config_word(pdev, PT_ADDR_INDEX,
> + PT_4_PORT_1_REG);
> + pci_read_config_byte(pdev, PT_READ_INDEX, &value);
> + if (value & (1<<(port + 1)))
> + return 0;
> + else
> + return 1;
> + }
> + } else if (pdev->device == 0x43bb) {
> + /* device is AMD_PROMONTORYA_2(0x43bb)
> + * disable port setting PT_2_PORT_1_REG[7..5] is
> + * USB2.0 port2 to
> + * PT_2_PORT_2_REG[5..0] is USB9 to port3
> + * 0 : disable ;1 : enable
> + */
> + if (port > 2) {
> + pci_write_config_word(pdev, PT_ADDR_INDEX,
> + PT_2_PORT_2_REG);
> + pci_read_config_byte(pdev, PT_READ_INDEX, &value);
> + if (value & (1<<(port - 3)))
> + return 0;
> + else
> + return 1;
> + } else {
> + pci_write_config_word(pdev, PT_ADDR_INDEX,
> + PT_2_PORT_1_REG);
> + pci_read_config_byte(pdev, PT_READ_INDEX, &value);
> + if (value & (1<<(port + 5)))
> + return 0;
> + else
> + return 1;
> + }
> + } else {
> + /* device is AMD_PROMONTORYA_1(0x43bc)
> + * disable port setting PT_1_PORT_1_REG[7..4] is
> + * USB2.0 port3 to 0
> + * PT_1_PORT_2_REG[5..0] is USB9 to port4
> + * 0 : disable ;1 : enable
> + */
> + if (port > 3) {
> + pci_write_config_word(pdev, PT_ADDR_INDEX,
> + PT_1_PORT_2_REG);
> + pci_read_config_byte(pdev, PT_READ_INDEX, &value);
> + if (value & (1<<(port - 4)))
> + return 0;
> + else
> + return 1;
> + } else {
> + pci_write_config_word(pdev, PT_ADDR_INDEX,
> + PT_1_PORT_1_REG);
> + pci_read_config_byte(pdev, PT_READ_INDEX, &value);
> + if (value & (1<<(port + 4)))
> + return 0;
> + else
> + return 1;
> + }
> + }
> +}
> +EXPORT_SYMBOL_GPL(usb_amd_pt_check_port);
> +
> +
> +
Only 1 blank line between functions please.
> /*
> * Make sure the controller is completely inactive, unable to
> * generate interrupts or do DMA.
> diff --git a/drivers/usb/host/pci-quirks.h b/drivers/usb/host/pci-quirks.h
> index b68dcb5..7df1b0b 100644
> --- a/drivers/usb/host/pci-quirks.h
> +++ b/drivers/usb/host/pci-quirks.h
> @@ -10,6 +10,7 @@ int usb_hcd_amd_remote_wakeup_quirk(struct pci_dev *pdev);
> bool usb_amd_hang_symptom_quirk(void);
> bool usb_amd_prefetch_quirk(void);
> void usb_amd_dev_put(void);
> +int usb_amd_pt_check_port(struct device *device, int port);
> void usb_amd_quirk_pll_disable(void);
> void usb_amd_quirk_pll_enable(void);
> void usb_asmedia_modifyflowcontrol(struct pci_dev *pdev);
> @@ -23,6 +24,7 @@ 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 int usb_amd_pt_check_port(struct device *device, int port) {}
> static inline void usb_disable_xhci_ports(struct pci_dev *xhci_pdev) {}
> static inline void sb800_prefetch(struct device *dev, int on) {}
> #endif /* CONFIG_USB_PCI */
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index a2336de..89ee574 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -1528,6 +1528,12 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
> t2 |= PORT_WKOC_E | PORT_WKCONN_E;
> t2 &= ~PORT_WKDISC_E;
> }
> + if ((xhci->quirks & XHCI_U2_DISABLE_WAKE) &&
> + (hcd->speed < HCD_USB3)) {
Horrid indentation, I could have forgiven everything else, but not this :)
> + if (usb_amd_pt_check_port(hcd->self.controller,
> + port_index))
> + t2 &= ~PORT_WAKE_BITS;
> + }
> } else
> t2 &= ~PORT_WAKE_BITS;
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 76f3929..8071c8f 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -54,6 +54,11 @@
> #define PCI_DEVICE_ID_INTEL_APL_XHCI 0x5aa8
> #define PCI_DEVICE_ID_INTEL_DNV_XHCI 0x19d0
>
> +#define PCI_DEVICE_ID_AMD_PROMONTORYA_4 0x43b9
> +#define PCI_DEVICE_ID_AMD_PROMONTORYA_3 0x43ba
> +#define PCI_DEVICE_ID_AMD_PROMONTORYA_2 0x43bb
> +#define PCI_DEVICE_ID_AMD_PROMONTORYA_1 0x43bc
> +
> #define PCI_DEVICE_ID_ASMEDIA_1042A_XHCI 0x1142
>
> static const char hcd_name[] = "xhci_hcd";
> @@ -137,6 +142,13 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
> if (pdev->vendor == PCI_VENDOR_ID_AMD)
> xhci->quirks |= XHCI_TRUST_TX_LENGTH;
>
> + if ((pdev->vendor == PCI_VENDOR_ID_AMD) &&
> + ((pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_4) ||
> + (pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_3) ||
> + (pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_2) ||
> + (pdev->device == PCI_DEVICE_ID_AMD_PROMONTORYA_1)))
> + xhci->quirks |= XHCI_U2_DISABLE_WAKE;
> +
> if (pdev->vendor == PCI_VENDOR_ID_INTEL) {
> xhci->quirks |= XHCI_LPM_SUPPORT;
> xhci->quirks |= XHCI_INTEL_HOST;
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 2b48aa4..7c87189 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1828,7 +1828,7 @@ struct xhci_hcd {
> /* For controller with a broken Port Disable implementation */
> #define XHCI_BROKEN_PORT_PED (1 << 25)
> #define XHCI_LIMIT_ENDPOINT_INTERVAL_7 (1 << 26)
> -/* Reserved. It was XHCI_U2_DISABLE_WAKE */
> +#define XHCI_U2_DISABLE_WAKE (1 << 27)
Just a question, why are you all not using the BIT() macro here? Don't
change this now, just an overall question...
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
