diff mbox series

[v2,6/6] misc: pci_endpoint_test: Add Device ID for R-Car V4H PCIe controller

Message ID 20240326024540.2336155-7-yoshihiro.shimoda.uh@renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series PCI: dwc: rcar-gen4: Add R-Car V4H support | expand

Commit Message

Yoshihiro Shimoda March 26, 2024, 2:45 a.m. UTC 
Add Renesas R8A779G0 in pci_device_id table so that pci-epf-test
can be used for testing PCIe EP on R-Car V4H.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/misc/pci_endpoint_test.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Frank Li March 26, 2024, 3:21 a.m. UTC | #1 
On Tue, Mar 26, 2024 at 11:45:40AM +0900, Yoshihiro Shimoda wrote:
> Add Renesas R8A779G0 in pci_device_id table so that pci-epf-test
> can be used for testing PCIe EP on R-Car V4H.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/misc/pci_endpoint_test.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> index c38a6083f0a7..2fa3c6473c7d 100644
> --- a/drivers/misc/pci_endpoint_test.c
> +++ b/drivers/misc/pci_endpoint_test.c
> @@ -83,6 +83,7 @@
>  #define PCI_DEVICE_ID_RENESAS_R8A774C0		0x002d
>  #define PCI_DEVICE_ID_RENESAS_R8A774E1		0x0025
>  #define PCI_DEVICE_ID_RENESAS_R8A779F0		0x0031
> +#define PCI_DEVICE_ID_RENESAS_R8A779G0		0x0030
>  
>  static DEFINE_IDA(pci_endpoint_test_ida);
>  
> @@ -1005,6 +1006,9 @@ static const struct pci_device_id pci_endpoint_test_tbl[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779F0),
>  	  .driver_data = (kernel_ulong_t)&default_data,
>  	},
> +	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779G0),
> +	  .driver_data = (kernel_ulong_t)&default_data,
> +	},

You use default_data, why need new device_id? I think you can use 0x0031
to do test.

Frank

>  	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_J721E),
>  	  .driver_data = (kernel_ulong_t)&j721e_data,
>  	},
> -- 
> 2.25.1
>
Yoshihiro Shimoda March 26, 2024, 5:47 a.m. UTC | #2 
Hi Frank,

> From: Frank Li, Sent: Tuesday, March 26, 2024 12:21 PM
 
> On Tue, Mar 26, 2024 at 11:45:40AM +0900, Yoshihiro Shimoda wrote:
> > Add Renesas R8A779G0 in pci_device_id table so that pci-epf-test
> > can be used for testing PCIe EP on R-Car V4H.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> >  drivers/misc/pci_endpoint_test.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> > index c38a6083f0a7..2fa3c6473c7d 100644
> > --- a/drivers/misc/pci_endpoint_test.c
> > +++ b/drivers/misc/pci_endpoint_test.c
> > @@ -83,6 +83,7 @@
> >  #define PCI_DEVICE_ID_RENESAS_R8A774C0		0x002d
> >  #define PCI_DEVICE_ID_RENESAS_R8A774E1		0x0025
> >  #define PCI_DEVICE_ID_RENESAS_R8A779F0		0x0031
> > +#define PCI_DEVICE_ID_RENESAS_R8A779G0		0x0030
> >
> >  static DEFINE_IDA(pci_endpoint_test_ida);
> >
> > @@ -1005,6 +1006,9 @@ static const struct pci_device_id pci_endpoint_test_tbl[] = {
> >  	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779F0),
> >  	  .driver_data = (kernel_ulong_t)&default_data,
> >  	},
> > +	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779G0),
> > +	  .driver_data = (kernel_ulong_t)&default_data,
> > +	},
> 
> You use default_data, why need new device_id? I think you can use 0x0031
> to do test.

I thought we can add a new device_id freely like other devices.
Since the PCIe controller's endpoint mode can configure the device id,
I can use 0x0031 to do test though.

If such a reusable entry exists, is adding a new device id into the driver prohibited?

Best regards,
Yoshihiro Shimoda

> Frank
> 
> >  	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_J721E),
> >  	  .driver_data = (kernel_ulong_t)&j721e_data,
> >  	},
> > --
> > 2.25.1
> >
Frank Li March 26, 2024, 2:08 p.m. UTC | #3 
On Tue, Mar 26, 2024 at 05:47:23AM +0000, Yoshihiro Shimoda wrote:
> Hi Frank,
> 
> > From: Frank Li, Sent: Tuesday, March 26, 2024 12:21 PM
>  
> > On Tue, Mar 26, 2024 at 11:45:40AM +0900, Yoshihiro Shimoda wrote:
> > > Add Renesas R8A779G0 in pci_device_id table so that pci-epf-test
> > > can be used for testing PCIe EP on R-Car V4H.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > ---
> > >  drivers/misc/pci_endpoint_test.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> > > index c38a6083f0a7..2fa3c6473c7d 100644
> > > --- a/drivers/misc/pci_endpoint_test.c
> > > +++ b/drivers/misc/pci_endpoint_test.c
> > > @@ -83,6 +83,7 @@
> > >  #define PCI_DEVICE_ID_RENESAS_R8A774C0		0x002d
> > >  #define PCI_DEVICE_ID_RENESAS_R8A774E1		0x0025
> > >  #define PCI_DEVICE_ID_RENESAS_R8A779F0		0x0031
> > > +#define PCI_DEVICE_ID_RENESAS_R8A779G0		0x0030
> > >
> > >  static DEFINE_IDA(pci_endpoint_test_ida);
> > >
> > > @@ -1005,6 +1006,9 @@ static const struct pci_device_id pci_endpoint_test_tbl[] = {
> > >  	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779F0),
> > >  	  .driver_data = (kernel_ulong_t)&default_data,
> > >  	},
> > > +	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779G0),
> > > +	  .driver_data = (kernel_ulong_t)&default_data,
> > > +	},
> > 
> > You use default_data, why need new device_id? I think you can use 0x0031
> > to do test.
> 
> I thought we can add a new device_id freely like other devices.
> Since the PCIe controller's endpoint mode can configure the device id,
> I can use 0x0031 to do test though.
> 
> If such a reusable entry exists, is adding a new device id into the driver prohibited?

I just think it is not necessary. This list will become longer and longer.
And difference device id can't help us at all. 

We should use difference production as difference functions, or difference
configuration.  Such as usb gadget product id, we use 0x4545 for all mass
storage. 

Using difference devices id for difference function, such as 0x31 for
ep_test 0x30 for virtual net, 0x29 for virtual console ...

Or using difference devices id indicate some features. For example, use
0x30 means support write to EP MSI ITS to trigger irq.

Donate a device_id to more valuable things.

Frank

> 
> Best regards,
> Yoshihiro Shimoda
> 
> > Frank
> > 
> > >  	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_J721E),
> > >  	  .driver_data = (kernel_ulong_t)&j721e_data,
> > >  	},
> > > --
> > > 2.25.1
> > >
Yoshihiro Shimoda March 27, 2024, 5:02 a.m. UTC | #4 
Hi Frank,

> From: Frank Li, Sent: Tuesday, March 26, 2024 11:09 PM
> 
> On Tue, Mar 26, 2024 at 05:47:23AM +0000, Yoshihiro Shimoda wrote:
> > Hi Frank,
> >
> > > From: Frank Li, Sent: Tuesday, March 26, 2024 12:21 PM
> >
> > > On Tue, Mar 26, 2024 at 11:45:40AM +0900, Yoshihiro Shimoda wrote:
> > > > Add Renesas R8A779G0 in pci_device_id table so that pci-epf-test
> > > > can be used for testing PCIe EP on R-Car V4H.
> > > >
> > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > ---
> > > >  drivers/misc/pci_endpoint_test.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> > > > index c38a6083f0a7..2fa3c6473c7d 100644
> > > > --- a/drivers/misc/pci_endpoint_test.c
> > > > +++ b/drivers/misc/pci_endpoint_test.c
> > > > @@ -83,6 +83,7 @@
> > > >  #define PCI_DEVICE_ID_RENESAS_R8A774C0		0x002d
> > > >  #define PCI_DEVICE_ID_RENESAS_R8A774E1		0x0025
> > > >  #define PCI_DEVICE_ID_RENESAS_R8A779F0		0x0031
> > > > +#define PCI_DEVICE_ID_RENESAS_R8A779G0		0x0030
> > > >
> > > >  static DEFINE_IDA(pci_endpoint_test_ida);
> > > >
> > > > @@ -1005,6 +1006,9 @@ static const struct pci_device_id pci_endpoint_test_tbl[] = {
> > > >  	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779F0),
> > > >  	  .driver_data = (kernel_ulong_t)&default_data,
> > > >  	},
> > > > +	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779G0),
> > > > +	  .driver_data = (kernel_ulong_t)&default_data,
> > > > +	},
> > >
> > > You use default_data, why need new device_id? I think you can use 0x0031
> > > to do test.
> >
> > I thought we can add a new device_id freely like other devices.
> > Since the PCIe controller's endpoint mode can configure the device id,
> > I can use 0x0031 to do test though.
> >
> > If such a reusable entry exists, is adding a new device id into the driver prohibited?
> 
> I just think it is not necessary. This list will become longer and longer.
> And difference device id can't help us at all.

I agreed. To record it, I'll make a patch to add such description in the pci_endpoint_test.c.

> We should use difference production as difference functions, or difference
> configuration.  Such as usb gadget product id, we use 0x4545 for all mass
> storage.

I see.

> Using difference devices id for difference function, such as 0x31 for
> ep_test 0x30 for virtual net, 0x29 for virtual console ...
> 
> Or using difference devices id indicate some features. For example, use
> 0x30 means support write to EP MSI ITS to trigger irq.
> 
> Donate a device_id to more valuable things.

I think so.

Best regards,
Yoshihiro Shimoda

> Frank
> 
> >
> > Best regards,
> > Yoshihiro Shimoda
> >
> > > Frank
> > >
> > > >  	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_J721E),
> > > >  	  .driver_data = (kernel_ulong_t)&j721e_data,
> > > >  	},
> > > > --
> > > > 2.25.1
> > > >
Geert Uytterhoeven March 27, 2024, 8:11 a.m. UTC | #5 
Hi Shimoda-san,

On Tue, Mar 26, 2024 at 6:47 AM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> > From: Frank Li, Sent: Tuesday, March 26, 2024 12:21 PM
> > On Tue, Mar 26, 2024 at 11:45:40AM +0900, Yoshihiro Shimoda wrote:
> > > Add Renesas R8A779G0 in pci_device_id table so that pci-epf-test
> > > can be used for testing PCIe EP on R-Car V4H.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > ---
> > >  drivers/misc/pci_endpoint_test.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> > > index c38a6083f0a7..2fa3c6473c7d 100644
> > > --- a/drivers/misc/pci_endpoint_test.c
> > > +++ b/drivers/misc/pci_endpoint_test.c
> > > @@ -83,6 +83,7 @@
> > >  #define PCI_DEVICE_ID_RENESAS_R8A774C0             0x002d
> > >  #define PCI_DEVICE_ID_RENESAS_R8A774E1             0x0025
> > >  #define PCI_DEVICE_ID_RENESAS_R8A779F0             0x0031
> > > +#define PCI_DEVICE_ID_RENESAS_R8A779G0             0x0030
> > >
> > >  static DEFINE_IDA(pci_endpoint_test_ida);
> > >
> > > @@ -1005,6 +1006,9 @@ static const struct pci_device_id pci_endpoint_test_tbl[] = {
> > >     { PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779F0),
> > >       .driver_data = (kernel_ulong_t)&default_data,
> > >     },
> > > +   { PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779G0),
> > > +     .driver_data = (kernel_ulong_t)&default_data,
> > > +   },
> >
> > You use default_data, why need new device_id? I think you can use 0x0031
> > to do test.
>
> I thought we can add a new device_id freely like other devices.
> Since the PCIe controller's endpoint mode can configure the device id,
> I can use 0x0031 to do test though.

Can it? The documentation for the PCICONF0Fi register states both the
DEVICE_ID and VENDOR_ID bits are read-only.

Gr{oetje,eeting}s,

                        Geert
Yoshihiro Shimoda March 27, 2024, 8:30 a.m. UTC | #6 
Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Wednesday, March 27, 2024 5:12 PM
> 
> Hi Shimoda-san,
> 
> On Tue, Mar 26, 2024 at 6:47 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@renesas.com> wrote:
> > > From: Frank Li, Sent: Tuesday, March 26, 2024 12:21 PM
> > > On Tue, Mar 26, 2024 at 11:45:40AM +0900, Yoshihiro Shimoda wrote:
> > > > Add Renesas R8A779G0 in pci_device_id table so that pci-epf-test
> > > > can be used for testing PCIe EP on R-Car V4H.
> > > >
> > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > ---
> > > >  drivers/misc/pci_endpoint_test.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> > > > index c38a6083f0a7..2fa3c6473c7d 100644
> > > > --- a/drivers/misc/pci_endpoint_test.c
> > > > +++ b/drivers/misc/pci_endpoint_test.c
> > > > @@ -83,6 +83,7 @@
> > > >  #define PCI_DEVICE_ID_RENESAS_R8A774C0             0x002d
> > > >  #define PCI_DEVICE_ID_RENESAS_R8A774E1             0x0025
> > > >  #define PCI_DEVICE_ID_RENESAS_R8A779F0             0x0031
> > > > +#define PCI_DEVICE_ID_RENESAS_R8A779G0             0x0030
> > > >
> > > >  static DEFINE_IDA(pci_endpoint_test_ida);
> > > >
> > > > @@ -1005,6 +1006,9 @@ static const struct pci_device_id pci_endpoint_test_tbl[] = {
> > > >     { PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779F0),
> > > >       .driver_data = (kernel_ulong_t)&default_data,
> > > >     },
> > > > +   { PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779G0),
> > > > +     .driver_data = (kernel_ulong_t)&default_data,
> > > > +   },
> > >
> > > You use default_data, why need new device_id? I think you can use 0x0031
> > > to do test.
> >
> > I thought we can add a new device_id freely like other devices.
> > Since the PCIe controller's endpoint mode can configure the device id,
> > I can use 0x0031 to do test though.
> 
> Can it?

Yes, the following function can write the device id in endpoint mode.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-designware-ep.c?h=v6.9-rc1#n108

> The documentation for the PCICONF0Fi register states both the
> DEVICE_ID and VENDOR_ID bits are read-only.

You're correct. The documentation (R-Car V4H hardware manual) said these bits are read-only.
However, actual hardware IP seems to allow writing this register if the write protect is disabled in endpoint mode.

Best regards,
Yoshihiro Shimoda

> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
diff mbox series

Patch

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index c38a6083f0a7..2fa3c6473c7d 100644
--- a/drivers/misc/pci_endpoint_test.c
+++ b/drivers/misc/pci_endpoint_test.c
@@ -83,6 +83,7 @@ 
 #define PCI_DEVICE_ID_RENESAS_R8A774C0		0x002d
 #define PCI_DEVICE_ID_RENESAS_R8A774E1		0x0025
 #define PCI_DEVICE_ID_RENESAS_R8A779F0		0x0031
+#define PCI_DEVICE_ID_RENESAS_R8A779G0		0x0030
 
 static DEFINE_IDA(pci_endpoint_test_ida);
 
@@ -1005,6 +1006,9 @@  static const struct pci_device_id pci_endpoint_test_tbl[] = {
 	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779F0),
 	  .driver_data = (kernel_ulong_t)&default_data,
 	},
+	{ PCI_DEVICE(PCI_VENDOR_ID_RENESAS, PCI_DEVICE_ID_RENESAS_R8A779G0),
+	  .driver_data = (kernel_ulong_t)&default_data,
+	},
 	{ PCI_DEVICE(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_J721E),
 	  .driver_data = (kernel_ulong_t)&j721e_data,
 	},