Skip to contentSkip to conversation

usb: xhci-pci: Add TUSB73x0 definitions

PATCH In review Fabio Estevam 4 participants 5 messages Oct 9 – Oct 13

Subsystems (3)
USB PCI USB xHCI driver
lore
Share & tools
lore

Fabio Estevam

author
#0 lore raw

PATCH v3 1/1 usb: xhci-pci: Add TUSB73x0 definitions

Instead of hard-coding the TUSB73X0 PCI ID, USB_CTRL register address and the PWRON_POLARITY, introduce definitions for them to make the code easier to read.

No functional change.

Signed-off-by Fabio Estevam <[email protected]>

---
Changes since v2:
- Move the TUSB73x0 PCI device ID to the shared PCI ID header and use it in pci-quirks.c too, as suggested by Sashiko.

Changes since v1:
- None. Only sent it as a standalone patch instead of part of a series.

 drivers/usb/host/pci-quirks.c | 3 ++-
 drivers/usb/host/xhci-pci.c   | 9 +++++++--
 include/linux/pci_ids.h       | 1 +
 3 files changed, 10 insertions(+), 3 deletions(-)
drivers/usb/host/pci-quirks.c +2 -1
index ba158d63bb8b..f670e7aab984 100644
@@ -1189,7 +1189,8 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
 	val = readl(base + ext_cap_offset);
 
 	/* Auto handoff never worked for these devices. Force it and continue */
-	if ((pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241) ||
+	if ((pdev->vendor == PCI_VENDOR_ID_TI &&
+	     pdev->device == PCI_DEVICE_ID_TI_TUSB73X0) ||
 			(pdev->vendor == PCI_VENDOR_ID_RENESAS
 			 && pdev->device == 0x0014)) {
 		val = (val | XHCI_HC_OS_OWNED) & ~XHCI_HC_BIOS_OWNED;
drivers/usb/host/xhci-pci.c +7 -2
index a8889081ae82..54b76e2bddd2 100644
@@ -28,6 +28,9 @@
 #define SPARSE_DISABLE_BIT	17
 #define SPARSE_CNTL_ENABLE	0xC12C
 
+#define TUSB73X0_USB_CTRL			0xe0
+#define TUSB73X0_PWRON_POLARITY			BIT(22)
+
 /* Device for a quirk */
 #define PCI_VENDOR_ID_FRESCO_LOGIC		0x1b73
 #define PCI_DEVICE_ID_FRESCO_LOGIC_PDK		0x1000

@@ -479,7 +482,8 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 	    pdev->device == PCI_DEVICE_ID_ASMEDIA_3042_XHCI)
 		xhci->quirks |= XHCI_RESET_ON_RESUME;
 
-	if (pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241)
+	if (pdev->vendor == PCI_VENDOR_ID_TI &&
+	    pdev->device == PCI_DEVICE_ID_TI_TUSB73X0)
 		xhci->quirks |= XHCI_LIMIT_ENDPOINT_INTERVAL_7;
 
 	if ((pdev->vendor == PCI_VENDOR_ID_BROADCOM ||

@@ -678,7 +682,8 @@ int xhci_pci_common_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	dma_set_max_seg_size(&dev->dev, UINT_MAX);
 
 	if (device_property_read_bool(&dev->dev, "ti,pwron-active-high"))
-		pci_clear_and_set_config_dword(dev, 0xE0, 0, 1 << 22);
+		pci_clear_and_set_config_dword(dev, TUSB73X0_USB_CTRL, 0,
+					       TUSB73X0_PWRON_POLARITY);
 
 	return 0;
include/linux/pci_ids.h +1 -0
index 0693e4532a68..030c86dd6d67 100644
@@ -889,6 +889,7 @@
 #define PCI_DEVICE_ID_TI_XX12		0x8039
 #define PCI_DEVICE_ID_TI_XX12_FM	0x803b
 #define PCI_DEVICE_ID_TI_XIO2000A	0x8231
+#define PCI_DEVICE_ID_TI_TUSB73X0	0x8241
 #define PCI_DEVICE_ID_TI_1130		0xac12
 #define PCI_DEVICE_ID_TI_1031		0xac13
 #define PCI_DEVICE_ID_TI_1131		0xac15
Signature · 1 line
2.43.0
AI review
#1 lore raw
Sashiko has reviewed this patch and found no issues. It looks great!
Quoted 7 lines

Instead of hard-coding the TUSB73X0 PCI ID, USB_CTRL register address and the PWRON_POLARITY, introduce definitions for them to make the code easier to read.

No functional change.

Signed-off-by Fabio Estevam <[email protected]>

Sashiko has reviewed this patch and found no issues. It looks great!

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Acked-by on v3 1/1
#2 lore raw
In reply to Fabio Estevam
Quoted 7 lines · Fabio Estevam

Instead of hard-coding the TUSB73X0 PCI ID, USB_CTRL register address and the PWRON_POLARITY, introduce definitions for them to make the code easier to read.

No functional change.

Signed-off-by Fabio Estevam <[email protected]>
Acked-by Bjorn Helgaas <[email protected]> # pci_ids.h
Quoted 77 lines

---
Changes since v2:
- Move the TUSB73x0 PCI device ID to the shared PCI ID header and use it in pci-quirks.c too, as suggested by Sashiko.

Changes since v1:
- None. Only sent it as a standalone patch instead of part of a series.

 drivers/usb/host/pci-quirks.c | 3 ++-
 drivers/usb/host/xhci-pci.c   | 9 +++++++--
 include/linux/pci_ids.h       | 1 +
 3 files changed, 10 insertions(+), 3 deletions(-)
drivers/usb/host/pci-quirks.c +2 -1
index ba158d63bb8b..f670e7aab984 100644
@@ -1189,7 +1189,8 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
 	val = readl(base + ext_cap_offset);
 
 	/* Auto handoff never worked for these devices. Force it and continue */
-	if ((pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241) ||
+	if ((pdev->vendor == PCI_VENDOR_ID_TI &&
+	     pdev->device == PCI_DEVICE_ID_TI_TUSB73X0) ||
 			(pdev->vendor == PCI_VENDOR_ID_RENESAS
 			 && pdev->device == 0x0014)) {
 		val = (val | XHCI_HC_OS_OWNED) & ~XHCI_HC_BIOS_OWNED;
drivers/usb/host/xhci-pci.c +7 -2
index a8889081ae82..54b76e2bddd2 100644
@@ -28,6 +28,9 @@
 #define SPARSE_DISABLE_BIT	17
 #define SPARSE_CNTL_ENABLE	0xC12C
 
+#define TUSB73X0_USB_CTRL			0xe0
+#define TUSB73X0_PWRON_POLARITY			BIT(22)
+
 /* Device for a quirk */
 #define PCI_VENDOR_ID_FRESCO_LOGIC		0x1b73
 #define PCI_DEVICE_ID_FRESCO_LOGIC_PDK		0x1000

@@ -479,7 +482,8 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 	    pdev->device == PCI_DEVICE_ID_ASMEDIA_3042_XHCI)
 		xhci->quirks |= XHCI_RESET_ON_RESUME;
 
-	if (pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241)
+	if (pdev->vendor == PCI_VENDOR_ID_TI &&
+	    pdev->device == PCI_DEVICE_ID_TI_TUSB73X0)
 		xhci->quirks |= XHCI_LIMIT_ENDPOINT_INTERVAL_7;
 
 	if ((pdev->vendor == PCI_VENDOR_ID_BROADCOM ||

@@ -678,7 +682,8 @@ int xhci_pci_common_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	dma_set_max_seg_size(&dev->dev, UINT_MAX);
 
 	if (device_property_read_bool(&dev->dev, "ti,pwron-active-high"))
-		pci_clear_and_set_config_dword(dev, 0xE0, 0, 1 << 22);
+		pci_clear_and_set_config_dword(dev, TUSB73X0_USB_CTRL, 0,
+					       TUSB73X0_PWRON_POLARITY);
 
 	return 0;
include/linux/pci_ids.h +1 -0
index 0693e4532a68..030c86dd6d67 100644
@@ -889,6 +889,7 @@
 #define PCI_DEVICE_ID_TI_XX12		0x8039
 #define PCI_DEVICE_ID_TI_XX12_FM	0x803b
 #define PCI_DEVICE_ID_TI_XIO2000A	0x8231
+#define PCI_DEVICE_ID_TI_TUSB73X0	0x8241
 #define PCI_DEVICE_ID_TI_1130		0xac12
 #define PCI_DEVICE_ID_TI_1031		0xac13
 #define PCI_DEVICE_ID_TI_1131		0xac15
Signature · 1 line
2.43.0
#3 lore raw
In reply to Fabio Estevam
Quoted 26 lines · Fabio Estevam

Instead of hard-coding the TUSB73X0 PCI ID, USB_CTRL register address and the PWRON_POLARITY, introduce definitions for them to make the code easier to read.

No functional change.

Signed-off-by Fabio Estevam <[email protected]>

---
Changes since v2:
- Move the TUSB73x0 PCI device ID to the shared PCI ID header and use it in pci-quirks.c too, as suggested by Sashiko.

Changes since v1:
- None. Only sent it as a standalone patch instead of part of a series.

 drivers/usb/host/pci-quirks.c | 3 ++-
 drivers/usb/host/xhci-pci.c   | 9 +++++++--
 include/linux/pci_ids.h       | 1 +
 3 files changed, 10 insertions(+), 3 deletions(-)
drivers/usb/host/pci-quirks.c +0 -0
index ba158d63bb8b..f670e7aab984 100644

drivers/usb/host/pci-quirks.c static void quirk_usb_handoff_xhci(struct pci_dev *pdev) · line 1193

Quoted 6 lines
drivers/usb/host/pci-quirks.c +2 -1
@@ -1189,7 +1189,8 @@ static void quirk_usb_handoff_xhci(struct pci_dev *pdev)
 	val = readl(base + ext_cap_offset);
 
 	/* Auto handoff never worked for these devices. Force it and continue */
-	if ((pdev->vendor == PCI_VENDOR_ID_TI && pdev->device == 0x8241) ||
+	if ((pdev->vendor == PCI_VENDOR_ID_TI &&
+	     pdev->device == PCI_DEVICE_ID_TI_TUSB73X0) ||

You mentioned working with TUSB7340 before. Was there any board firmware involved which made use of this chip before the OS?

I'm looking for someone who can tell whether this line should be touched at all or deleted. It was added by c289d0eff3d5a, which is very suspicious - it mentions "cards" (like in "PCIe expansion cards") and adds two IDs in one go. I feel it could be a BIOS bug affecting all cards and no problem with those chips.

[Adding the author in case he remembers what happened there.]

For example, can a PCIe card raise SMI interrupts? AFAIK it can't. But if a BIOS thinks otherwise, and enables "SMI on OS ownership request", it will never learn about our request, so the handoff times out and falls into the "forced" branch. Or the BIOS may not rely on this but poll the card, and then "forget" about polling.

Regards,
Michal

author
#4 lore raw
Quoted 2 lines · Michal Pecio

You mentioned working with TUSB7340 before. Was there any board firmware involved which made use of this chip before the OS?

I used a TUSB7320 on an i.MX95-based board, and used the TUSB7320 only in Linux, not in the bootloader.