diff mbox series

[v16,01/22] PCI: Add PCI_EXP_LNKCAP_MLW macros

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

Commit Message

Yoshihiro Shimoda May 10, 2023, 6:22 a.m. UTC 
Add macros defining Maximum Link Width bits in Link Capabilities
Register.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Acked-by: Bjorn Helgaas <bhelgaas@google.com>
---
 include/uapi/linux/pci_regs.h | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Serge Semin June 4, 2023, 10:50 p.m. UTC | #1 
On Wed, May 10, 2023 at 03:22:13PM +0900, Yoshihiro Shimoda wrote:
> Add macros defining Maximum Link Width bits in Link Capabilities
> Register.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>

You haven't been using these macros in the following up patches since
v9. Why do you keep submitting this change then? I would suggest to
drop the patch especially seeing the PCI_EXP_LNKCAP_MLW field directly
encodes the link width thus these macros unlikely will be of much use.

-Serge(y)

> ---
>  include/uapi/linux/pci_regs.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index dc2000e0fe3a..5d48413ac28f 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -538,6 +538,12 @@
>  #define  PCI_EXP_LNKCAP_SLS_16_0GB 0x00000004 /* LNKCAP2 SLS Vector bit 3 */
>  #define  PCI_EXP_LNKCAP_SLS_32_0GB 0x00000005 /* LNKCAP2 SLS Vector bit 4 */
>  #define  PCI_EXP_LNKCAP_SLS_64_0GB 0x00000006 /* LNKCAP2 SLS Vector bit 5 */
> +#define  PCI_EXP_LNKCAP_MLW_X1	0x00000010 /* Maximum Link Width x1 */
> +#define  PCI_EXP_LNKCAP_MLW_X2	0x00000020 /* Maximum Link Width x2 */
> +#define  PCI_EXP_LNKCAP_MLW_X4	0x00000040 /* Maximum Link Width x4 */
> +#define  PCI_EXP_LNKCAP_MLW_X8	0x00000080 /* Maximum Link Width x8 */
> +#define  PCI_EXP_LNKCAP_MLW_X12	0x000000c0 /* Maximum Link Width x12 */
> +#define  PCI_EXP_LNKCAP_MLW_X16	0x00000100 /* Maximum Link Width x16 */
>  #define  PCI_EXP_LNKCAP_MLW	0x000003f0 /* Maximum Link Width */
>  #define  PCI_EXP_LNKCAP_ASPMS	0x00000c00 /* ASPM Support */
>  #define  PCI_EXP_LNKCAP_ASPM_L0S 0x00000400 /* ASPM L0s Support */
> -- 
> 2.25.1
>
Yoshihiro Shimoda June 5, 2023, 12:14 a.m. UTC | #2 
Hello Serge,

> From: Serge Semin, Sent: Monday, June 5, 2023 7:51 AM
> 
> On Wed, May 10, 2023 at 03:22:13PM +0900, Yoshihiro Shimoda wrote:
> > Add macros defining Maximum Link Width bits in Link Capabilities
> > Register.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> 
> You haven't been using these macros in the following up patches since
> v9. Why do you keep submitting this change then? I would suggest to
> drop the patch especially seeing the PCI_EXP_LNKCAP_MLW field directly
> encodes the link width thus these macros unlikely will be of much use.

Thank you for your review! You're correct. I didn't realized that
the macros were not used on the patch series..

However, I also realized that the patch 11/22 used the PCI_EXP_LNKSTA_NLW_SHIFT
macro for the PCI_EXP_LNKCAP register. So, I'm thinking that I should modify
this patch as adding PCI_EXP_LNKCAP_MLW_SHIFT instead. But, what do you think?

--- on the patch 11/22 ---
> +	cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +	val = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP);
> +	val &= ~PCI_EXP_LNKCAP_MLW;
> +	val |= num_lanes << PCI_EXP_LNKSTA_NLW_SHIFT;

Perhaps, this should be PCI_EXP_LNKCAP_MLW_SHIFT instead.
---

Best regards,
Yoshihiro Shimoda

> -Serge(y)
> 
> > ---
> >  include/uapi/linux/pci_regs.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> > index dc2000e0fe3a..5d48413ac28f 100644
> > --- a/include/uapi/linux/pci_regs.h
> > +++ b/include/uapi/linux/pci_regs.h
> > @@ -538,6 +538,12 @@
> >  #define  PCI_EXP_LNKCAP_SLS_16_0GB 0x00000004 /* LNKCAP2 SLS Vector bit 3 */
> >  #define  PCI_EXP_LNKCAP_SLS_32_0GB 0x00000005 /* LNKCAP2 SLS Vector bit 4 */
> >  #define  PCI_EXP_LNKCAP_SLS_64_0GB 0x00000006 /* LNKCAP2 SLS Vector bit 5 */
> > +#define  PCI_EXP_LNKCAP_MLW_X1	0x00000010 /* Maximum Link Width x1 */
> > +#define  PCI_EXP_LNKCAP_MLW_X2	0x00000020 /* Maximum Link Width x2 */
> > +#define  PCI_EXP_LNKCAP_MLW_X4	0x00000040 /* Maximum Link Width x4 */
> > +#define  PCI_EXP_LNKCAP_MLW_X8	0x00000080 /* Maximum Link Width x8 */
> > +#define  PCI_EXP_LNKCAP_MLW_X12	0x000000c0 /* Maximum Link Width x12 */
> > +#define  PCI_EXP_LNKCAP_MLW_X16	0x00000100 /* Maximum Link Width x16 */
> >  #define  PCI_EXP_LNKCAP_MLW	0x000003f0 /* Maximum Link Width */
> >  #define  PCI_EXP_LNKCAP_ASPMS	0x00000c00 /* ASPM Support */
> >  #define  PCI_EXP_LNKCAP_ASPM_L0S 0x00000400 /* ASPM L0s Support */
> > --
> > 2.25.1
> >
Serge Semin June 5, 2023, 12:25 a.m. UTC | #3 
On Mon, Jun 05, 2023 at 12:14:26AM +0000, Yoshihiro Shimoda wrote:
> Hello Serge,
> 
> > From: Serge Semin, Sent: Monday, June 5, 2023 7:51 AM
> > 
> > On Wed, May 10, 2023 at 03:22:13PM +0900, Yoshihiro Shimoda wrote:
> > > Add macros defining Maximum Link Width bits in Link Capabilities
> > > Register.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > You haven't been using these macros in the following up patches since
> > v9. Why do you keep submitting this change then? I would suggest to
> > drop the patch especially seeing the PCI_EXP_LNKCAP_MLW field directly
> > encodes the link width thus these macros unlikely will be of much use.
> 
> Thank you for your review! You're correct. I didn't realized that
> the macros were not used on the patch series..
> 
> However, I also realized that the patch 11/22 used the PCI_EXP_LNKSTA_NLW_SHIFT
> macro for the PCI_EXP_LNKCAP register. So, I'm thinking that I should modify
> this patch as adding PCI_EXP_LNKCAP_MLW_SHIFT instead. But, what do you think?
> 
> --- on the patch 11/22 ---
> > +	cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > +	val = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP);
> > +	val &= ~PCI_EXP_LNKCAP_MLW;
> > +	val |= num_lanes << PCI_EXP_LNKSTA_NLW_SHIFT;
> 
> Perhaps, this should be PCI_EXP_LNKCAP_MLW_SHIFT instead.

I'll get to that patch tomorrow, but in any case there is no need in
defining an additional macro here. There is a handy helper FIELD_PREP()
for that: FIELD_PREP(PCI_EXP_LNKSTA_MLW, num_lanes).

IMO for the same reason the PCI_EXP_LNKSTA_NLW_* macros are
unnecessary and can be easily either dropped or replaced by the
FIELD_{PREP,GET} macros usage.

-Serge(y)

> ---
> 
> Best regards,
> Yoshihiro Shimoda
> 
> > -Serge(y)
> > 
> > > ---
> > >  include/uapi/linux/pci_regs.h | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> > > index dc2000e0fe3a..5d48413ac28f 100644
> > > --- a/include/uapi/linux/pci_regs.h
> > > +++ b/include/uapi/linux/pci_regs.h
> > > @@ -538,6 +538,12 @@
> > >  #define  PCI_EXP_LNKCAP_SLS_16_0GB 0x00000004 /* LNKCAP2 SLS Vector bit 3 */
> > >  #define  PCI_EXP_LNKCAP_SLS_32_0GB 0x00000005 /* LNKCAP2 SLS Vector bit 4 */
> > >  #define  PCI_EXP_LNKCAP_SLS_64_0GB 0x00000006 /* LNKCAP2 SLS Vector bit 5 */
> > > +#define  PCI_EXP_LNKCAP_MLW_X1	0x00000010 /* Maximum Link Width x1 */
> > > +#define  PCI_EXP_LNKCAP_MLW_X2	0x00000020 /* Maximum Link Width x2 */
> > > +#define  PCI_EXP_LNKCAP_MLW_X4	0x00000040 /* Maximum Link Width x4 */
> > > +#define  PCI_EXP_LNKCAP_MLW_X8	0x00000080 /* Maximum Link Width x8 */
> > > +#define  PCI_EXP_LNKCAP_MLW_X12	0x000000c0 /* Maximum Link Width x12 */
> > > +#define  PCI_EXP_LNKCAP_MLW_X16	0x00000100 /* Maximum Link Width x16 */
> > >  #define  PCI_EXP_LNKCAP_MLW	0x000003f0 /* Maximum Link Width */
> > >  #define  PCI_EXP_LNKCAP_ASPMS	0x00000c00 /* ASPM Support */
> > >  #define  PCI_EXP_LNKCAP_ASPM_L0S 0x00000400 /* ASPM L0s Support */
> > > --
> > > 2.25.1
> > >
Yoshihiro Shimoda June 5, 2023, 1:38 a.m. UTC | #4 
Hello Serge,

> From: Serge Semin, Sent: Monday, June 5, 2023 9:26 AM
> 
> On Mon, Jun 05, 2023 at 12:14:26AM +0000, Yoshihiro Shimoda wrote:
> > Hello Serge,
> >
> > > From: Serge Semin, Sent: Monday, June 5, 2023 7:51 AM
> > >
> > > On Wed, May 10, 2023 at 03:22:13PM +0900, Yoshihiro Shimoda wrote:
> > > > Add macros defining Maximum Link Width bits in Link Capabilities
> > > > Register.
> > > >
> > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > > Acked-by: Bjorn Helgaas <bhelgaas@google.com>
> > >
> > > You haven't been using these macros in the following up patches since
> > > v9. Why do you keep submitting this change then? I would suggest to
> > > drop the patch especially seeing the PCI_EXP_LNKCAP_MLW field directly
> > > encodes the link width thus these macros unlikely will be of much use.
> >
> > Thank you for your review! You're correct. I didn't realized that
> > the macros were not used on the patch series..
> >
> > However, I also realized that the patch 11/22 used the PCI_EXP_LNKSTA_NLW_SHIFT
> > macro for the PCI_EXP_LNKCAP register. So, I'm thinking that I should modify
> > this patch as adding PCI_EXP_LNKCAP_MLW_SHIFT instead. But, what do you think?
> >
> > --- on the patch 11/22 ---
> > > +	cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> > > +	val = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP);
> > > +	val &= ~PCI_EXP_LNKCAP_MLW;
> > > +	val |= num_lanes << PCI_EXP_LNKSTA_NLW_SHIFT;
> >
> > Perhaps, this should be PCI_EXP_LNKCAP_MLW_SHIFT instead.
> 
> I'll get to that patch tomorrow, but in any case there is no need in
> defining an additional macro here. There is a handy helper FIELD_PREP()
> for that: FIELD_PREP(PCI_EXP_LNKSTA_MLW, num_lanes).

Thank you for the reply. TBH, I didn't know the helper FIELD_PREP(),
but now I understood it. Since the pci_regs.h has already had
the PCI_EXP_LNKCAP_MLW macro, we don't need to add any macro here.

> IMO for the same reason the PCI_EXP_LNKSTA_NLW_* macros are
> unnecessary and can be easily either dropped or replaced by the
> FIELD_{PREP,GET} macros usage.

I got it.

Best regards,
Yoshihiro Shimoda

> -Serge(y)
> 
> > ---
> >
> > Best regards,
> > Yoshihiro Shimoda
> >
> > > -Serge(y)
> > >
> > > > ---
> > > >  include/uapi/linux/pci_regs.h | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> > > > index dc2000e0fe3a..5d48413ac28f 100644
> > > > --- a/include/uapi/linux/pci_regs.h
> > > > +++ b/include/uapi/linux/pci_regs.h
> > > > @@ -538,6 +538,12 @@
> > > >  #define  PCI_EXP_LNKCAP_SLS_16_0GB 0x00000004 /* LNKCAP2 SLS Vector bit 3 */
> > > >  #define  PCI_EXP_LNKCAP_SLS_32_0GB 0x00000005 /* LNKCAP2 SLS Vector bit 4 */
> > > >  #define  PCI_EXP_LNKCAP_SLS_64_0GB 0x00000006 /* LNKCAP2 SLS Vector bit 5 */
> > > > +#define  PCI_EXP_LNKCAP_MLW_X1	0x00000010 /* Maximum Link Width x1 */
> > > > +#define  PCI_EXP_LNKCAP_MLW_X2	0x00000020 /* Maximum Link Width x2 */
> > > > +#define  PCI_EXP_LNKCAP_MLW_X4	0x00000040 /* Maximum Link Width x4 */
> > > > +#define  PCI_EXP_LNKCAP_MLW_X8	0x00000080 /* Maximum Link Width x8 */
> > > > +#define  PCI_EXP_LNKCAP_MLW_X12	0x000000c0 /* Maximum Link Width x12 */
> > > > +#define  PCI_EXP_LNKCAP_MLW_X16	0x00000100 /* Maximum Link Width x16 */
> > > >  #define  PCI_EXP_LNKCAP_MLW	0x000003f0 /* Maximum Link Width */
> > > >  #define  PCI_EXP_LNKCAP_ASPMS	0x00000c00 /* ASPM Support */
> > > >  #define  PCI_EXP_LNKCAP_ASPM_L0S 0x00000400 /* ASPM L0s Support */
> > > > --
> > > > 2.25.1
> > > >
diff mbox series

Patch

diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index dc2000e0fe3a..5d48413ac28f 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -538,6 +538,12 @@ 
 #define  PCI_EXP_LNKCAP_SLS_16_0GB 0x00000004 /* LNKCAP2 SLS Vector bit 3 */
 #define  PCI_EXP_LNKCAP_SLS_32_0GB 0x00000005 /* LNKCAP2 SLS Vector bit 4 */
 #define  PCI_EXP_LNKCAP_SLS_64_0GB 0x00000006 /* LNKCAP2 SLS Vector bit 5 */
+#define  PCI_EXP_LNKCAP_MLW_X1	0x00000010 /* Maximum Link Width x1 */
+#define  PCI_EXP_LNKCAP_MLW_X2	0x00000020 /* Maximum Link Width x2 */
+#define  PCI_EXP_LNKCAP_MLW_X4	0x00000040 /* Maximum Link Width x4 */
+#define  PCI_EXP_LNKCAP_MLW_X8	0x00000080 /* Maximum Link Width x8 */
+#define  PCI_EXP_LNKCAP_MLW_X12	0x000000c0 /* Maximum Link Width x12 */
+#define  PCI_EXP_LNKCAP_MLW_X16	0x00000100 /* Maximum Link Width x16 */
 #define  PCI_EXP_LNKCAP_MLW	0x000003f0 /* Maximum Link Width */
 #define  PCI_EXP_LNKCAP_ASPMS	0x00000c00 /* ASPM Support */
 #define  PCI_EXP_LNKCAP_ASPM_L0S 0x00000400 /* ASPM L0s Support */