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

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

[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 <andriy.shevchenko@linux.intel.com>
---
 drivers/usb/cdns3/cdns3-pci-wrap.c |  5 ++---
 drivers/usb/cdns3/cdnsp-pci.c      | 29 +++++++++++++++--------------
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/cdns3/cdns3-pci-wrap.c b/drivers/usb/cdns3/cdns3-pci-wrap.c
index 1f6320d98a76..591d149de8f3 100644
--- a/drivers/usb/cdns3/cdns3-pci-wrap.c
+++ b/drivers/usb/cdns3/cdns3-pci-wrap.c
@@ -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, }
 };
 
diff --git a/drivers/usb/cdns3/cdnsp-pci.c b/drivers/usb/cdns3/cdnsp-pci.c
index 225540fc81ba..2d05368a6745 100644
--- a/drivers/usb/cdns3/cdnsp-pci.c
+++ b/drivers/usb/cdns3/cdnsp-pci.c
@@ -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, }
 };
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 1/1] sub: cdns3: Use predefined PCI vendor ID constant

[Roger Quadros]

Hi,

On 13/09/2024 16:17, Andy Shevchenko wrote:
> 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 <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/usb/cdns3/cdns3-pci-wrap.c |  5 ++---
>  drivers/usb/cdns3/cdnsp-pci.c      | 29 +++++++++++++++--------------
>  2 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/cdns3-pci-wrap.c b/drivers/usb/cdns3/cdns3-pci-wrap.c
> index 1f6320d98a76..591d149de8f3 100644
> --- a/drivers/usb/cdns3/cdns3-pci-wrap.c
> +++ b/drivers/usb/cdns3/cdns3-pci-wrap.c
> @@ -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.

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

>  	{ 0, }
>  };
>  
> diff --git a/drivers/usb/cdns3/cdnsp-pci.c b/drivers/usb/cdns3/cdnsp-pci.c
> index 225540fc81ba..2d05368a6745 100644
> --- a/drivers/usb/cdns3/cdnsp-pci.c
> +++ b/drivers/usb/cdns3/cdnsp-pci.c
> @@ -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.

> +#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

> +
> +#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, }
>  };
>  

-- 
cheers,
-roger

Re: [PATCH v1 1/1] sub: cdns3: Use predefined PCI vendor ID constant

[Andy Shevchenko]

On Mon, Sep 23, 2024 at 03:42:20PM +0300, Roger Quadros wrote:
> On 13/09/2024 16:17, Andy Shevchenko wrote:
> > 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.

...

> > +#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."

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

...

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

...

> > -#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.

> I would call this PCI_DEVICE_ID_CDNS_USBSSP	0x200	to match with PCI driver name.
> 
> > +#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).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] sub: cdns3: Use predefined PCI vendor ID constant

[Roger Quadros]

On 23/09/2024 15:53, Andy Shevchenko wrote:
> On Mon, Sep 23, 2024 at 03:42:20PM +0300, Roger Quadros wrote:
>> On 13/09/2024 16:17, Andy Shevchenko wrote:
>>> 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.
> 
> ...
> 
>>> +#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."
> 
>> 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.
> 
> ...
> 
>>> -	{ 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).
> 

:)

> ...
> 
>>> -#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.

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

> 
>> I would call this PCI_DEVICE_ID_CDNS_USBSSP	0x200	to match with PCI driver name.
>>
>>> +#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.

Agreed.

> 
> 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).
> 
-- 
cheers,
-roger

Re: [PATCH v1 1/1] sub: cdns3: Use predefined PCI vendor ID constant

[Andy Shevchenko]

On Mon, Sep 23, 2024 at 04:24:15PM +0300, Roger Quadros wrote:
> On 23/09/2024 15:53, Andy Shevchenko wrote:
> > On Mon, Sep 23, 2024 at 03:42:20PM +0300, Roger Quadros wrote:
> >> On 13/09/2024 16:17, Andy Shevchenko wrote:

...

> >>> -#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.
> 
> 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.

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

-- 
With Best Regards,
Andy Shevchenko