sub: cdns3: Use predefined PCI vendor ID constant

PATCH In review Andy Shevchenko 2 participants 5 messages Sep 13, 2024 – Sep 23, 2024

Subsystems (3)

USB Cadence USB3 DRD IP driver Cadence usbssp DRD IP driver

lore

Share & tools

lore

Andy Shevchenko

author

2024-09-13 13:17 UTC #0 lore raw

PATCH 1/1 sub: cdns3: Use predefined PCI vendor ID constant

The PCI vendor ID for Cadence is defined in pci_ids.h. Use it. While at it, move to PCI_DEVICE() macro and usual pattern for PCI class and device IDs.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/usb/cdns3/cdns3-pci-wrap.c |  5 ++---
 drivers/usb/cdns3/cdnsp-pci.c      | 29 +++++++++++++++--------------
 2 files changed, 17 insertions(+), 17 deletions(-)

drivers/usb/cdns3/cdns3-pci-wrap.c +2 -3

index 1f6320d98a76..591d149de8f3 100644
@@ -37,8 +37,7 @@ struct cdns3_wrap {
 #define PCI_DRIVER_NAME		"cdns3-pci-usbss"
 #define PLAT_DRIVER_NAME	"cdns-usb3"
 
-#define CDNS_VENDOR_ID		0x17cd
-#define CDNS_DEVICE_ID		0x0100
+#define PCI_DEVICE_ID_CDNS_USB3	0x0100
 
 static struct pci_dev *cdns3_get_second_fun(struct pci_dev *pdev)
 {

@@ -190,7 +189,7 @@ static void cdns3_pci_remove(struct pci_dev *pdev)
 }
 
 static const struct pci_device_id cdns3_pci_ids[] = {
-	{ PCI_DEVICE(CDNS_VENDOR_ID, CDNS_DEVICE_ID), },
+	{ PCI_VDEVICE(CDNS, PCI_DEVICE_ID_CDNS_USB3) },
 	{ 0, }
 };

drivers/usb/cdns3/cdnsp-pci.c +15 ~~-14~~

index 225540fc81ba..2d05368a6745 100644
@@ -28,10 +28,11 @@
 #define PCI_DRIVER_NAME		"cdns-pci-usbssp"
 #define PLAT_DRIVER_NAME	"cdns-usbssp"
 
-#define CDNS_VENDOR_ID		0x17cd
-#define CDNS_DEVICE_ID		0x0200
-#define CDNS_DRD_ID		0x0100
-#define CDNS_DRD_IF		(PCI_CLASS_SERIAL_USB << 8 | 0x80)
+#define PCI_DEVICE_ID_CDNS_USB3		0x0100
+#define PCI_DEVICE_ID_CDNS_UDC		0x0200
+
+#define PCI_CLASS_SERIAL_USB_CDNS_USB3	(PCI_CLASS_SERIAL_USB << 8 | 0x80)
+#define PCI_CLASS_SERIAL_USB_CDNS_UDC	PCI_CLASS_SERIAL_USB_DEVICE
 
 static struct pci_dev *cdnsp_get_second_fun(struct pci_dev *pdev)
 {

@@ -40,10 +41,10 @@ static struct pci_dev *cdnsp_get_second_fun(struct pci_dev *pdev)
 	 * Platform has two function. The fist keeps resources for
 	 * Host/Device while the secon keeps resources for DRD/OTG.
 	 */
-	if (pdev->device == CDNS_DEVICE_ID)
-		return  pci_get_device(pdev->vendor, CDNS_DRD_ID, NULL);
-	else if (pdev->device == CDNS_DRD_ID)
-		return pci_get_device(pdev->vendor, CDNS_DEVICE_ID, NULL);
+	if (pdev->device == PCI_DEVICE_ID_CDNS_UDC)
+		return pci_get_device(pdev->vendor, PCI_DEVICE_ID_CDNS_USB3, NULL);
+	if (pdev->device == PCI_DEVICE_ID_CDNS_USB3)
+		return pci_get_device(pdev->vendor, PCI_DEVICE_ID_CDNS_UDC, NULL);
 
 	return NULL;
 }

@@ -220,12 +221,12 @@ static const struct dev_pm_ops cdnsp_pci_pm_ops = {
 };
 
 static const struct pci_device_id cdnsp_pci_ids[] = {
-	{ PCI_VENDOR_ID_CDNS, CDNS_DEVICE_ID, PCI_ANY_ID, PCI_ANY_ID,
-	  PCI_CLASS_SERIAL_USB_DEVICE, PCI_ANY_ID },
-	{ PCI_VENDOR_ID_CDNS, CDNS_DEVICE_ID, PCI_ANY_ID, PCI_ANY_ID,
-	  CDNS_DRD_IF, PCI_ANY_ID },
-	{ PCI_VENDOR_ID_CDNS, CDNS_DRD_ID, PCI_ANY_ID, PCI_ANY_ID,
-	  CDNS_DRD_IF, PCI_ANY_ID },
+	{ PCI_DEVICE(PCI_VENDOR_ID_CDNS, PCI_DEVICE_ID_CDNS_UDC),
+	  .class = PCI_CLASS_SERIAL_USB_CDNS_UDC },
+	{ PCI_DEVICE(PCI_VENDOR_ID_CDNS, PCI_DEVICE_ID_CDNS_UDC),
+	  .class = PCI_CLASS_SERIAL_USB_CDNS_USB3 },
+	{ PCI_DEVICE(PCI_VENDOR_ID_CDNS, PCI_DEVICE_ID_CDNS_USB3),
+	  .class = PCI_CLASS_SERIAL_USB_CDNS_USB3 },
 	{ 0, }
 };

Signature · 1 line

2.43.0.rc1.1336.g36b5255a03ac

Roger Quadros

reviewer

2024-09-23 12:42 UTC #1 lore raw

Hi,

Quoted 15 lines · Andy Shevchenko

The PCI vendor ID for Cadence is defined in pci_ids.h. Use it. While at it, move to PCI_DEVICE() macro and usual pattern for PCI class and device IDs.
Signed-off-by Andy Shevchenko <[email protected]>
---
 drivers/usb/cdns3/cdns3-pci-wrap.c |  5 ++---
 drivers/usb/cdns3/cdnsp-pci.c      | 29 +++++++++++++++--------------
 2 files changed, 17 insertions(+), 17 deletions(-)
drivers/usb/cdns3/cdns3-pci-wrap.c +0 -0
index 1f6320d98a76..591d149de8f3 100644

drivers/usb/cdns3/cdns3-pci-wrap.c struct cdns3_wrap { · line 40

Quoted 6 lines

drivers/usb/cdns3/cdns3-pci-wrap.c +1 -2

@@ -37,8 +37,7 @@ struct cdns3_wrap {
 #define PCI_DRIVER_NAME		"cdns3-pci-usbss"
 #define PLAT_DRIVER_NAME	"cdns-usb3"
 
-#define CDNS_VENDOR_ID		0x17cd
-#define CDNS_DEVICE_ID		0x0100
+#define PCI_DEVICE_ID_CDNS_USB3	0x0100

Why do we need to change this? You did not explain in commit log.

I would call this PCI_DEVICE_ID_CDNS_USBSS3. Also see later why to differentiate with USBSSP.

drivers/usb/cdns3/cdns3-pci-wrap.c static void cdns3_pci_remove(struct pci_dev *pdev) · line 192

Quoted 9 lines

drivers/usb/cdns3/cdns3-pci-wrap.c +1 -1

 static struct pci_dev *cdns3_get_second_fun(struct pci_dev *pdev)
 {

@@ -190,7 +189,7 @@ static void cdns3_pci_remove(struct pci_dev *pdev)
 }
 
 static const struct pci_device_id cdns3_pci_ids[] = {
-	{ PCI_DEVICE(CDNS_VENDOR_ID, CDNS_DEVICE_ID), },
+	{ PCI_VDEVICE(CDNS, PCI_DEVICE_ID_CDNS_USB3) },

For better readability I still prefer
PCI_DEVICE(PCI_VENDOR_ID_CDNS, PCI_DEVICE_ID_CDNS_USBSS3)

Quoted 8 lines

drivers/usb/cdns3/cdns3-pci-wrap.c +0 -0
 	{ 0, }
 };
 
drivers/usb/cdns3/cdnsp-pci.c +0 -0
index 225540fc81ba..2d05368a6745 100644

drivers/usb/cdns3/cdnsp-pci.c line 31

Quoted 8 lines

drivers/usb/cdns3/cdnsp-pci.c +1 -4

@@ -28,10 +28,11 @@
 #define PCI_DRIVER_NAME		"cdns-pci-usbssp"
 #define PLAT_DRIVER_NAME	"cdns-usbssp"
 
-#define CDNS_VENDOR_ID		0x17cd
-#define CDNS_DEVICE_ID		0x0200
-#define CDNS_DRD_ID		0x0100
-#define CDNS_DRD_IF		(PCI_CLASS_SERIAL_USB << 8 | 0x80)
+#define PCI_DEVICE_ID_CDNS_USB3		0x0100

This is an entirely different card who's device ID should be 0x200? Also I don't think this card supports USB3 so it is a wrong name choice.

I would call this PCI_DEVICE_ID_CDNS_USBSSP 0x200 to match with PCI driver name.

drivers/usb/cdns3/cdnsp-pci.c line 32

Quoted 1 line

drivers/usb/cdns3/cdnsp-pci.c +1 -0
+#define PCI_DEVICE_ID_CDNS_UDC		0x0200

UDC is used for Peripheral controller only. Is that really the case here? originally it was called DRD.
So how about?

	PCI_DEVICE_ID_CDNS_DRD		0x0100

Quoted 40 lines

drivers/usb/cdns3/cdnsp-pci.c +13 ~~-10~~

+
+#define PCI_CLASS_SERIAL_USB_CDNS_USB3	(PCI_CLASS_SERIAL_USB << 8 | 0x80)
+#define PCI_CLASS_SERIAL_USB_CDNS_UDC	PCI_CLASS_SERIAL_USB_DEVICE
 
 static struct pci_dev *cdnsp_get_second_fun(struct pci_dev *pdev)
 {

@@ -40,10 +41,10 @@ static struct pci_dev *cdnsp_get_second_fun(struct pci_dev *pdev)
 	 * Platform has two function. The fist keeps resources for
 	 * Host/Device while the secon keeps resources for DRD/OTG.
 	 */
-	if (pdev->device == CDNS_DEVICE_ID)
-		return  pci_get_device(pdev->vendor, CDNS_DRD_ID, NULL);
-	else if (pdev->device == CDNS_DRD_ID)
-		return pci_get_device(pdev->vendor, CDNS_DEVICE_ID, NULL);
+	if (pdev->device == PCI_DEVICE_ID_CDNS_UDC)
+		return pci_get_device(pdev->vendor, PCI_DEVICE_ID_CDNS_USB3, NULL);
+	if (pdev->device == PCI_DEVICE_ID_CDNS_USB3)
+		return pci_get_device(pdev->vendor, PCI_DEVICE_ID_CDNS_UDC, NULL);
 
 	return NULL;
 }

@@ -220,12 +221,12 @@ static const struct dev_pm_ops cdnsp_pci_pm_ops = {
 };
 
 static const struct pci_device_id cdnsp_pci_ids[] = {
-	{ PCI_VENDOR_ID_CDNS, CDNS_DEVICE_ID, PCI_ANY_ID, PCI_ANY_ID,
-	  PCI_CLASS_SERIAL_USB_DEVICE, PCI_ANY_ID },
-	{ PCI_VENDOR_ID_CDNS, CDNS_DEVICE_ID, PCI_ANY_ID, PCI_ANY_ID,
-	  CDNS_DRD_IF, PCI_ANY_ID },
-	{ PCI_VENDOR_ID_CDNS, CDNS_DRD_ID, PCI_ANY_ID, PCI_ANY_ID,
-	  CDNS_DRD_IF, PCI_ANY_ID },
+	{ PCI_DEVICE(PCI_VENDOR_ID_CDNS, PCI_DEVICE_ID_CDNS_UDC),
+	  .class = PCI_CLASS_SERIAL_USB_CDNS_UDC },
+	{ PCI_DEVICE(PCI_VENDOR_ID_CDNS, PCI_DEVICE_ID_CDNS_UDC),
+	  .class = PCI_CLASS_SERIAL_USB_CDNS_USB3 },
+	{ PCI_DEVICE(PCI_VENDOR_ID_CDNS, PCI_DEVICE_ID_CDNS_USB3),
+	  .class = PCI_CLASS_SERIAL_USB_CDNS_USB3 },
 	{ 0, }
 };

Signature · 2 lines

cheers,
-roger

Andy Shevchenko

author

2024-09-23 12:53 UTC #2 lore raw

Quoted 4 lines · Roger Quadros

Quoted 3 lines · Andy Shevchenko
The PCI vendor ID for Cadence is defined in pci_ids.h. Use it. While at it, move to PCI_DEVICE() macro and usual pattern for PCI class and device IDs.

...

Quoted 3 lines

Quoted 1 line
+#define PCI_DEVICE_ID_CDNS_USB3 0x0100
Why do we need to change this? You did not explain in commit log.

It's explained: "...usual pattern for PCI class and device IDs."

Quoted 1 line

I would call this PCI_DEVICE_ID_CDNS_USBSS3. Also see later why to differentiate with USBSSP.

It's good to know that there are semantic differences, but it is already applied, feel free to update.

...

Quoted 5 lines

drivers/usb/cdns3/cdns3-pci-wrap.c static void cdns3_pci_remove(struct pci_dev *pdev) · line 192
Quoted 2 lines
drivers/usb/cdns3/cdns3-pci-wrap.c +1 -1
-	{ PCI_DEVICE(CDNS_VENDOR_ID, CDNS_DEVICE_ID), },
+	{ PCI_VDEVICE(CDNS, PCI_DEVICE_ID_CDNS_USB3) },
For better readability I still prefer
PCI_DEVICE(PCI_VENDOR_ID_CDNS, PCI_DEVICE_ID_CDNS_USBSS3)

I disagree. The PCI_VDEVICE() has less letters and much easier to get the vendor from the (less power to parse and decode is required).

...

Quoted 7 lines

drivers/usb/cdns3/cdnsp-pci.c line 31
Quoted 4 lines
drivers/usb/cdns3/cdnsp-pci.c +1 -3
-#define CDNS_DEVICE_ID		0x0200
-#define CDNS_DRD_ID		0x0100
-#define CDNS_DRD_IF		(PCI_CLASS_SERIAL_USB << 8 | 0x80)
+#define PCI_DEVICE_ID_CDNS_USB3		0x0100
This is an entirely different card who's device ID should be 0x200? Also I don't think this card supports USB3 so it is a wrong name choice.

Are you stating that 0x0100 in both cases points to the *different* devices?! This is unbelievable, however possible abuse of PCI IDs.

Quoted 8 lines

I would call this PCI_DEVICE_ID_CDNS_USBSSP 0x200 to match with PCI driver name.
Quoted 1 line
+#define PCI_DEVICE_ID_CDNS_UDC 0x0200
UDC is used for Peripheral controller only. Is that really the case here? originally it was called DRD.
So how about?
	PCI_DEVICE_ID_CDNS_DRD		0x0100

I strongly disagree. The same PCI IDs should be named the same independently on how many drivers use them.

The only possibility to have what you propose is the complete screwed up PCI IDs allocations done by vendor (I do not believe this is the case with Cadence).

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Roger Quadros

reviewer

2024-09-23 13:24 UTC #3 lore raw

Quoted 21 lines · Andy Shevchenko

Quoted 4 lines · Roger Quadros
Quoted 3 lines · Andy Shevchenko
The PCI vendor ID for Cadence is defined in pci_ids.h. Use it. While at it, move to PCI_DEVICE() macro and usual pattern for PCI class and device IDs.
...
Quoted 3 lines
Quoted 1 line
+#define PCI_DEVICE_ID_CDNS_USB3 0x0100
Why do we need to change this? You did not explain in commit log.
It's explained: "...usual pattern for PCI class and device IDs."
Quoted 1 line
I would call this PCI_DEVICE_ID_CDNS_USBSS3. Also see later why to differentiate with USBSSP.
It's good to know that there are semantic differences, but it is already applied, feel free to update.
...

Quoted 9 lines

Quoted 5 lines
drivers/usb/cdns3/cdns3-pci-wrap.c static void cdns3_pci_remove(struct pci_dev *pdev) · line 192
Quoted 2 lines
drivers/usb/cdns3/cdns3-pci-wrap.c +1 -1
-	{ PCI_DEVICE(CDNS_VENDOR_ID, CDNS_DEVICE_ID), },
+	{ PCI_VDEVICE(CDNS, PCI_DEVICE_ID_CDNS_USB3) },
For better readability I still prefer
PCI_DEVICE(PCI_VENDOR_ID_CDNS, PCI_DEVICE_ID_CDNS_USBSS3)
I disagree. The PCI_VDEVICE() has less letters and much easier to get the vendor from the (less power to parse and decode is required).

Quoted 10 lines

...
Quoted 7 lines
drivers/usb/cdns3/cdnsp-pci.c line 31
Quoted 4 lines
drivers/usb/cdns3/cdnsp-pci.c +1 -3
-#define CDNS_DEVICE_ID		0x0200
-#define CDNS_DRD_ID		0x0100
-#define CDNS_DRD_IF		(PCI_CLASS_SERIAL_USB << 8 | 0x80)
+#define PCI_DEVICE_ID_CDNS_USB3		0x0100
This is an entirely different card who's device ID should be 0x200? Also I don't think this card supports USB3 so it is a wrong name choice.

Quoted 2 lines

Are you stating that 0x0100 in both cases points to the *different* devices?! This is unbelievable, however possible abuse of PCI IDs.

I am not entirely sure.
What I do know is that one card should be USBSS (0x100) and other should be USBSSP (0x200). P for super-speed-Plus.

Also please see commit 96b96b2a567f ("usb: cdnsp: changes PCI Device ID to fix conflict with CNDS3 driver")

Quoted 5 lines

Quoted 5 lines
I would call this PCI_DEVICE_ID_CDNS_USBSSP 0x200 to match with PCI driver name.
Quoted 1 line
+#define PCI_DEVICE_ID_CDNS_UDC 0x0200

Quoted 7 lines

Quoted 4 lines
UDC is used for Peripheral controller only. Is that really the case here? originally it was called DRD.
So how about?
	PCI_DEVICE_ID_CDNS_DRD		0x0100
I strongly disagree. The same PCI IDs should be named the same independently on how many drivers use them.

Agreed.

Quoted 4 lines

The only possibility to have what you propose is the complete screwed up PCI IDs allocations done by vendor (I do not believe this is the case with Cadence).

Signature · 2 lines

cheers,
-roger

Andy Shevchenko

author

2024-09-23 13:44 UTC #4 lore raw

Quoted 3 lines · Roger Quadros

Quoted 2 lines · Andy Shevchenko
Quoted 1 line · Roger Quadros
On 13/09/2024 16:17, Andy Shevchenko wrote:

...

Quoted 8 lines

Quoted 8 lines
Quoted 7 lines
drivers/usb/cdns3/cdnsp-pci.c line 31
Quoted 4 lines
drivers/usb/cdns3/cdnsp-pci.c +1 -3
-#define CDNS_DEVICE_ID		0x0200
-#define CDNS_DRD_ID		0x0100
-#define CDNS_DRD_IF		(PCI_CLASS_SERIAL_USB << 8 | 0x80)
+#define PCI_DEVICE_ID_CDNS_USB3		0x0100
This is an entirely different card who's device ID should be 0x200? Also I don't think this card supports USB3 so it is a wrong name choice.

Quoted 6 lines

Quoted 2 lines
Are you stating that 0x0100 in both cases points to the *different* devices?! This is unbelievable, however possible abuse of PCI IDs.
I am not entirely sure.
What I do know is that one card should be USBSS (0x100) and other should be USBSSP (0x200). P for super-speed-Plus.

That's my understanding as well.

Quoted 2 lines

Also please see commit 96b96b2a567f ("usb: cdnsp: changes PCI Device ID to fix conflict with CNDS3 driver")

I believe this is an interesting way to solve the issue "enumeration with two or more drivers for the same HW". So, 0x100 in here is used just to see which driver is in use (has been chosen at build time?).

That said, the 0x100 in both cases seems to me the _same_ device in question. Hence it should be called the same, whatever you prefer. So, since the patches are already in USB Next, feel free to update the constant definition names as it looks like you are much more familiar with the hardware than me.

Signature · 2 lines

With Best Regards,
Andy Shevchenko