* Re: [PATCH v2] PCI: Cadence: Add quirk for Gen2 controller to do autonomous speed change.
2020-09-23 18:34 [PATCH v2] PCI: Cadence: Add quirk for Gen2 controller to do autonomous speed change Nadeem Athani
@ 2020-09-23 19:46 ` Rob Herring
2020-09-30 16:58 ` Athani Nadeem Ladkhan
2020-09-23 20:38 ` Bjorn Helgaas
2020-09-24 4:14 ` Kishon Vijay Abraham I
2 siblings, 1 reply; 7+ messages in thread
From: Rob Herring @ 2020-09-23 19:46 UTC (permalink / raw)
To: Nadeem Athani
Cc: Tom Joseph, Lorenzo Pieralisi, Bjorn Helgaas, PCI, linux-kernel,
Kishon Vijay Abraham I, Milind Parab, Swapnil Kashinath Jakhade
On Wed, Sep 23, 2020 at 12:34 PM Nadeem Athani <nadeem@cadence.com> wrote:
>
> Cadence controller will not initiate autonomous speed change if
> strapped as Gen2. The Retrain bit is set as a quirk to trigger
> this speed change.
>
> Signed-off-by: Nadeem Athani <nadeem@cadence.com>
> ---
> drivers/pci/controller/cadence/pcie-cadence-host.c | 14 ++++++++++++++
> drivers/pci/controller/cadence/pcie-cadence.h | 15 +++++++++++++++
> 2 files changed, 29 insertions(+)
>
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index 4550e0d469ca..a2317614268d 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -83,6 +83,9 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
> struct cdns_pcie *pcie = &rc->pcie;
> u32 value, ctrl;
> u32 id;
> + u32 link_cap = CDNS_PCIE_LINK_CAP_OFFSET;
> + u8 sls;
> + u16 lnk_ctl;
>
> /*
> * Set the root complex BAR configuration register:
> @@ -111,6 +114,17 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
> if (rc->device_id != 0xffff)
> cdns_pcie_rp_writew(pcie, PCI_DEVICE_ID, rc->device_id);
>
> + /* Quirk to enable autonomous speed change for GEN2 controller */
> + /* Reading Supported Link Speed value */
> + sls = PCI_EXP_LNKCAP_SLS &
> + cdns_pcie_rp_readb(pcie, link_cap + PCI_EXP_LNKCAP);
A 32-bit register, right?
> + if (sls == PCI_EXP_LNKCAP_SLS_5_0GB) {
> + /* Since this a Gen2 controller, set Retrain Link(RL) bit */
> + lnk_ctl = cdns_pcie_rp_readw(pcie, link_cap + PCI_EXP_LNKCTL);
> + lnk_ctl |= PCI_EXP_LNKCTL_RL;
> + cdns_pcie_rp_writew(pcie, link_cap + PCI_EXP_LNKCTL, lnk_ctl);
> + }
> +
> cdns_pcie_rp_writeb(pcie, PCI_CLASS_REVISION, 0);
> cdns_pcie_rp_writeb(pcie, PCI_CLASS_PROG, 0);
> cdns_pcie_rp_writew(pcie, PCI_CLASS_DEVICE, PCI_CLASS_BRIDGE_PCI);
> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
> index feed1e3038f4..fe560480c573 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.h
> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> @@ -120,6 +120,7 @@
> */
> #define CDNS_PCIE_RP_BASE 0x00200000
>
> +#define CDNS_PCIE_LINK_CAP_OFFSET 0xC0
>
> /*
> * Address Translation Registers
> @@ -413,6 +414,20 @@ static inline void cdns_pcie_rp_writew(struct cdns_pcie *pcie,
> cdns_pcie_write_sz(addr, 0x2, value);
> }
>
> +static inline u8 cdns_pcie_rp_readb(struct cdns_pcie *pcie, u32 reg)
> +{
> + void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg;
> +
> + return cdns_pcie_read_sz(addr, 0x1);
> +}
> +
> +static inline u16 cdns_pcie_rp_readw(struct cdns_pcie *pcie, u32 reg)
> +{
> + void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg;
> +
> + return cdns_pcie_read_sz(addr, 0x2);
> +}
> +
> /* Endpoint Function register access */
> static inline void cdns_pcie_ep_fn_writeb(struct cdns_pcie *pcie, u8 fn,
> u32 reg, u8 value)
> --
> 2.15.0
>
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [PATCH v2] PCI: Cadence: Add quirk for Gen2 controller to do autonomous speed change.
2020-09-23 19:46 ` Rob Herring
@ 2020-09-30 16:58 ` Athani Nadeem Ladkhan
0 siblings, 0 replies; 7+ messages in thread
From: Athani Nadeem Ladkhan @ 2020-09-30 16:58 UTC (permalink / raw)
To: Rob Herring
Cc: Tom Joseph, Lorenzo Pieralisi, Bjorn Helgaas, PCI, linux-kernel,
Kishon Vijay Abraham I, Milind Parab, Swapnil Kashinath Jakhade
Hi Rob,
> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Thursday, September 24, 2020 1:16 AM
> To: Athani Nadeem Ladkhan <nadeem@cadence.com>
> Cc: Tom Joseph <tjoseph@cadence.com>; Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com>; Bjorn Helgaas <bhelgaas@google.com>; PCI
> <linux-pci@vger.kernel.org>; linux-kernel@vger.kernel.org; Kishon Vijay
> Abraham I <kishon@ti.com>; Milind Parab <mparab@cadence.com>;
> Swapnil Kashinath Jakhade <sjakhade@cadence.com>
> Subject: Re: [PATCH v2] PCI: Cadence: Add quirk for Gen2 controller to do
> autonomous speed change.
>
> EXTERNAL MAIL
>
>
> On Wed, Sep 23, 2020 at 12:34 PM Nadeem Athani <nadeem@cadence.com>
> wrote:
> >
> > Cadence controller will not initiate autonomous speed change if
> > strapped as Gen2. The Retrain bit is set as a quirk to trigger this
> > speed change.
> >
> > Signed-off-by: Nadeem Athani <nadeem@cadence.com>
> > ---
> > drivers/pci/controller/cadence/pcie-cadence-host.c | 14 ++++++++++++++
> > drivers/pci/controller/cadence/pcie-cadence.h | 15 +++++++++++++++
> > 2 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c
> > b/drivers/pci/controller/cadence/pcie-cadence-host.c
> > index 4550e0d469ca..a2317614268d 100644
> > --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> > +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> > @@ -83,6 +83,9 @@ static int cdns_pcie_host_init_root_port(struct
> cdns_pcie_rc *rc)
> > struct cdns_pcie *pcie = &rc->pcie;
> > u32 value, ctrl;
> > u32 id;
> > + u32 link_cap = CDNS_PCIE_LINK_CAP_OFFSET;
> > + u8 sls;
> > + u16 lnk_ctl;
> >
> > /*
> > * Set the root complex BAR configuration register:
> > @@ -111,6 +114,17 @@ static int cdns_pcie_host_init_root_port(struct
> cdns_pcie_rc *rc)
> > if (rc->device_id != 0xffff)
> > cdns_pcie_rp_writew(pcie, PCI_DEVICE_ID,
> > rc->device_id);
> >
> > + /* Quirk to enable autonomous speed change for GEN2 controller */
> > + /* Reading Supported Link Speed value */
> > + sls = PCI_EXP_LNKCAP_SLS &
> > + cdns_pcie_rp_readb(pcie, link_cap + PCI_EXP_LNKCAP);
>
> A 32-bit register, right?
Right, this will be corrected in patch version 3.
>
> > + if (sls == PCI_EXP_LNKCAP_SLS_5_0GB) {
> > + /* Since this a Gen2 controller, set Retrain Link(RL) bit */
> > + lnk_ctl = cdns_pcie_rp_readw(pcie, link_cap + PCI_EXP_LNKCTL);
> > + lnk_ctl |= PCI_EXP_LNKCTL_RL;
> > + cdns_pcie_rp_writew(pcie, link_cap + PCI_EXP_LNKCTL, lnk_ctl);
> > + }
> > +
> > cdns_pcie_rp_writeb(pcie, PCI_CLASS_REVISION, 0);
> > cdns_pcie_rp_writeb(pcie, PCI_CLASS_PROG, 0);
> > cdns_pcie_rp_writew(pcie, PCI_CLASS_DEVICE,
> > PCI_CLASS_BRIDGE_PCI); diff --git
> > a/drivers/pci/controller/cadence/pcie-cadence.h
> > b/drivers/pci/controller/cadence/pcie-cadence.h
> > index feed1e3038f4..fe560480c573 100644
> > --- a/drivers/pci/controller/cadence/pcie-cadence.h
> > +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> > @@ -120,6 +120,7 @@
> > */
> > #define CDNS_PCIE_RP_BASE 0x00200000
> >
> > +#define CDNS_PCIE_LINK_CAP_OFFSET 0xC0
> >
> > /*
> > * Address Translation Registers
> > @@ -413,6 +414,20 @@ static inline void cdns_pcie_rp_writew(struct
> cdns_pcie *pcie,
> > cdns_pcie_write_sz(addr, 0x2, value); }
> >
> > +static inline u8 cdns_pcie_rp_readb(struct cdns_pcie *pcie, u32 reg)
> > +{
> > + void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg;
> > +
> > + return cdns_pcie_read_sz(addr, 0x1); }
> > +
> > +static inline u16 cdns_pcie_rp_readw(struct cdns_pcie *pcie, u32 reg)
> > +{
> > + void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg;
> > +
> > + return cdns_pcie_read_sz(addr, 0x2); }
> > +
> > /* Endpoint Function register access */ static inline void
> > cdns_pcie_ep_fn_writeb(struct cdns_pcie *pcie, u8 fn,
> > u32 reg, u8 value)
> > --
> > 2.15.0
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] PCI: Cadence: Add quirk for Gen2 controller to do autonomous speed change.
2020-09-23 18:34 [PATCH v2] PCI: Cadence: Add quirk for Gen2 controller to do autonomous speed change Nadeem Athani
2020-09-23 19:46 ` Rob Herring
@ 2020-09-23 20:38 ` Bjorn Helgaas
2020-09-30 17:06 ` Athani Nadeem Ladkhan
2020-09-24 4:14 ` Kishon Vijay Abraham I
2 siblings, 1 reply; 7+ messages in thread
From: Bjorn Helgaas @ 2020-09-23 20:38 UTC (permalink / raw)
To: Nadeem Athani
Cc: tjoseph, lorenzo.pieralisi, robh, bhelgaas, linux-pci,
linux-kernel, kishon, mparab, sjakhade
Something like:
PCI: cadence: Retrain Link to work around Gen2 training defect
to match history (see "git log --oneline
drivers/pci/controller/cadence/pcie-cadence-host.c").
On Wed, Sep 23, 2020 at 08:34:27PM +0200, Nadeem Athani wrote:
> Cadence controller will not initiate autonomous speed change if
> strapped as Gen2. The Retrain bit is set as a quirk to trigger
> this speed change.
To match the spec terminology:
Set the Retrain Link bit ...
Obviously I don't know the details of your device or even how PCIe
works at this level. But IIUC a link always comes up at 2.5 GT/s
first and then the upstream and downstream components negotiate the
highest speed they both support. It sounds like your controller
doesn't actually do this negotiation unless you set the Retrain Link
bit?
Is cdns_pcie_host_init_root_port() the only time this needs to be
done? We don't have to worry about doing this again after a reset,
hot-add event, etc?
> Signed-off-by: Nadeem Athani <nadeem@cadence.com>
> ---
> drivers/pci/controller/cadence/pcie-cadence-host.c | 14 ++++++++++++++
> drivers/pci/controller/cadence/pcie-cadence.h | 15 +++++++++++++++
> 2 files changed, 29 insertions(+)
>
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index 4550e0d469ca..a2317614268d 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -83,6 +83,9 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
> struct cdns_pcie *pcie = &rc->pcie;
> u32 value, ctrl;
> u32 id;
> + u32 link_cap = CDNS_PCIE_LINK_CAP_OFFSET;
This is not actually the link cap offset. Based on the usage, this
appears to be the offset of the PCIe Capability.
> + u8 sls;
> + u16 lnk_ctl;
>
> /*
> * Set the root complex BAR configuration register:
> @@ -111,6 +114,17 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
> if (rc->device_id != 0xffff)
> cdns_pcie_rp_writew(pcie, PCI_DEVICE_ID, rc->device_id);
>
> + /* Quirk to enable autonomous speed change for GEN2 controller */
> + /* Reading Supported Link Speed value */
> + sls = PCI_EXP_LNKCAP_SLS &
> + cdns_pcie_rp_readb(pcie, link_cap + PCI_EXP_LNKCAP);
The conventional way to write this would be
sls = cdns_pcie_rp_readb(pcie, link_cap + PCI_EXP_LNKCAP) &
PCI_EXP_LNKCAP_SLS;
> + if (sls == PCI_EXP_LNKCAP_SLS_5_0GB) {
> + /* Since this a Gen2 controller, set Retrain Link(RL) bit */
> + lnk_ctl = cdns_pcie_rp_readw(pcie, link_cap + PCI_EXP_LNKCTL);
> + lnk_ctl |= PCI_EXP_LNKCTL_RL;
> + cdns_pcie_rp_writew(pcie, link_cap + PCI_EXP_LNKCTL, lnk_ctl);
> + }
> +
> cdns_pcie_rp_writeb(pcie, PCI_CLASS_REVISION, 0);
> cdns_pcie_rp_writeb(pcie, PCI_CLASS_PROG, 0);
> cdns_pcie_rp_writew(pcie, PCI_CLASS_DEVICE, PCI_CLASS_BRIDGE_PCI);
> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
> index feed1e3038f4..fe560480c573 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.h
> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> @@ -120,6 +120,7 @@
> */
> #define CDNS_PCIE_RP_BASE 0x00200000
>
> +#define CDNS_PCIE_LINK_CAP_OFFSET 0xC0
Use lower-case in hex as the rest of the file does.
> /*
> * Address Translation Registers
> @@ -413,6 +414,20 @@ static inline void cdns_pcie_rp_writew(struct cdns_pcie *pcie,
> cdns_pcie_write_sz(addr, 0x2, value);
> }
>
> +static inline u8 cdns_pcie_rp_readb(struct cdns_pcie *pcie, u32 reg)
> +{
> + void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg;
> +
> + return cdns_pcie_read_sz(addr, 0x1);
> +}
> +
> +static inline u16 cdns_pcie_rp_readw(struct cdns_pcie *pcie, u32 reg)
> +{
> + void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg;
> +
> + return cdns_pcie_read_sz(addr, 0x2);
> +}
> +
> /* Endpoint Function register access */
> static inline void cdns_pcie_ep_fn_writeb(struct cdns_pcie *pcie, u8 fn,
> u32 reg, u8 value)
> --
> 2.15.0
>
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [PATCH v2] PCI: Cadence: Add quirk for Gen2 controller to do autonomous speed change.
2020-09-23 20:38 ` Bjorn Helgaas
@ 2020-09-30 17:06 ` Athani Nadeem Ladkhan
0 siblings, 0 replies; 7+ messages in thread
From: Athani Nadeem Ladkhan @ 2020-09-30 17:06 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Tom Joseph, lorenzo.pieralisi, robh, bhelgaas, linux-pci,
linux-kernel, kishon, Milind Parab, Swapnil Kashinath Jakhade
Hi Bjorn,
> -----Original Message-----
> From: Bjorn Helgaas <helgaas@kernel.org>
> Sent: Thursday, September 24, 2020 2:08 AM
> To: Athani Nadeem Ladkhan <nadeem@cadence.com>
> Cc: Tom Joseph <tjoseph@cadence.com>; lorenzo.pieralisi@arm.com;
> robh@kernel.org; bhelgaas@google.com; linux-pci@vger.kernel.org; linux-
> kernel@vger.kernel.org; kishon@ti.com; Milind Parab
> <mparab@cadence.com>; Swapnil Kashinath Jakhade
> <sjakhade@cadence.com>
> Subject: Re: [PATCH v2] PCI: Cadence: Add quirk for Gen2 controller to do
> autonomous speed change.
>
> EXTERNAL MAIL
>
>
> Something like:
>
> PCI: cadence: Retrain Link to work around Gen2 training defect
>
> to match history (see "git log --oneline drivers/pci/controller/cadence/pcie-
> cadence-host.c").
This will be corrected in patch version 3.
>
> On Wed, Sep 23, 2020 at 08:34:27PM +0200, Nadeem Athani wrote:
> > Cadence controller will not initiate autonomous speed change if
> > strapped as Gen2. The Retrain bit is set as a quirk to trigger this
> > speed change.
>
> To match the spec terminology:
>
> Set the Retrain Link bit ...
This will be corrected in patch version 3.
>
> Obviously I don't know the details of your device or even how PCIe works at
> this level. But IIUC a link always comes up at 2.5 GT/s first and then the
> upstream and downstream components negotiate the highest speed they
> both support. It sounds like your controller doesn't actually do this
> negotiation unless you set the Retrain Link bit?
Some Gen2 controller variants doesn't support this negotiation. Hence required to set the Retrain Link bit.
This will be handled in much better way in patch version 3.
>
> Is cdns_pcie_host_init_root_port() the only time this needs to be done? We
> don't have to worry about doing this again after a reset, hot-add event, etc?
Yes. Only during init.
>
> > Signed-off-by: Nadeem Athani <nadeem@cadence.com>
> > ---
> > drivers/pci/controller/cadence/pcie-cadence-host.c | 14 ++++++++++++++
> > drivers/pci/controller/cadence/pcie-cadence.h | 15 +++++++++++++++
> > 2 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c
> > b/drivers/pci/controller/cadence/pcie-cadence-host.c
> > index 4550e0d469ca..a2317614268d 100644
> > --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> > +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> > @@ -83,6 +83,9 @@ static int cdns_pcie_host_init_root_port(struct
> cdns_pcie_rc *rc)
> > struct cdns_pcie *pcie = &rc->pcie;
> > u32 value, ctrl;
> > u32 id;
> > + u32 link_cap = CDNS_PCIE_LINK_CAP_OFFSET;
>
> This is not actually the link cap offset. Based on the usage, this appears to be
> the offset of the PCIe Capability.
This will be corrected in patch version 3.
>
> > + u8 sls;
> > + u16 lnk_ctl;
> >
> > /*
> > * Set the root complex BAR configuration register:
> > @@ -111,6 +114,17 @@ static int cdns_pcie_host_init_root_port(struct
> cdns_pcie_rc *rc)
> > if (rc->device_id != 0xffff)
> > cdns_pcie_rp_writew(pcie, PCI_DEVICE_ID, rc->device_id);
> >
> > + /* Quirk to enable autonomous speed change for GEN2 controller */
> > + /* Reading Supported Link Speed value */
> > + sls = PCI_EXP_LNKCAP_SLS &
> > + cdns_pcie_rp_readb(pcie, link_cap + PCI_EXP_LNKCAP);
>
> The conventional way to write this would be
>
> sls = cdns_pcie_rp_readb(pcie, link_cap + PCI_EXP_LNKCAP) &
> PCI_EXP_LNKCAP_SLS;
This will be corrected in patch version 3.
>
> > + if (sls == PCI_EXP_LNKCAP_SLS_5_0GB) {
> > + /* Since this a Gen2 controller, set Retrain Link(RL) bit */
> > + lnk_ctl = cdns_pcie_rp_readw(pcie, link_cap +
> PCI_EXP_LNKCTL);
> > + lnk_ctl |= PCI_EXP_LNKCTL_RL;
> > + cdns_pcie_rp_writew(pcie, link_cap + PCI_EXP_LNKCTL,
> lnk_ctl);
> > + }
> > +
> > cdns_pcie_rp_writeb(pcie, PCI_CLASS_REVISION, 0);
> > cdns_pcie_rp_writeb(pcie, PCI_CLASS_PROG, 0);
> > cdns_pcie_rp_writew(pcie, PCI_CLASS_DEVICE,
> PCI_CLASS_BRIDGE_PCI);
> > diff --git a/drivers/pci/controller/cadence/pcie-cadence.h
> > b/drivers/pci/controller/cadence/pcie-cadence.h
> > index feed1e3038f4..fe560480c573 100644
> > --- a/drivers/pci/controller/cadence/pcie-cadence.h
> > +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> > @@ -120,6 +120,7 @@
> > */
> > #define CDNS_PCIE_RP_BASE 0x00200000
> >
> > +#define CDNS_PCIE_LINK_CAP_OFFSET 0xC0
>
> Use lower-case in hex as the rest of the file does.
This will be corrected in patch version 3.
>
> > /*
> > * Address Translation Registers
> > @@ -413,6 +414,20 @@ static inline void cdns_pcie_rp_writew(struct
> cdns_pcie *pcie,
> > cdns_pcie_write_sz(addr, 0x2, value); }
> >
> > +static inline u8 cdns_pcie_rp_readb(struct cdns_pcie *pcie, u32 reg)
> > +{
> > + void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg;
> > +
> > + return cdns_pcie_read_sz(addr, 0x1); }
> > +
> > +static inline u16 cdns_pcie_rp_readw(struct cdns_pcie *pcie, u32 reg)
> > +{
> > + void __iomem *addr = pcie->reg_base + CDNS_PCIE_RP_BASE + reg;
> > +
> > + return cdns_pcie_read_sz(addr, 0x2); }
> > +
> > /* Endpoint Function register access */ static inline void
> > cdns_pcie_ep_fn_writeb(struct cdns_pcie *pcie, u8 fn,
> > u32 reg, u8 value)
> > --
> > 2.15.0
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] PCI: Cadence: Add quirk for Gen2 controller to do autonomous speed change.
2020-09-23 18:34 [PATCH v2] PCI: Cadence: Add quirk for Gen2 controller to do autonomous speed change Nadeem Athani
2020-09-23 19:46 ` Rob Herring
2020-09-23 20:38 ` Bjorn Helgaas
@ 2020-09-24 4:14 ` Kishon Vijay Abraham I
2020-09-30 17:11 ` Athani Nadeem Ladkhan
2 siblings, 1 reply; 7+ messages in thread
From: Kishon Vijay Abraham I @ 2020-09-24 4:14 UTC (permalink / raw)
To: Nadeem Athani, tjoseph, lorenzo.pieralisi, robh, bhelgaas,
linux-pci, linux-kernel
Cc: mparab, sjakhade
Hi Nadeem,
On 24/09/20 12:04 am, Nadeem Athani wrote:
> Cadence controller will not initiate autonomous speed change if
> strapped as Gen2. The Retrain bit is set as a quirk to trigger
> this speed change.
>
> Signed-off-by: Nadeem Athani <nadeem@cadence.com>
> ---
> drivers/pci/controller/cadence/pcie-cadence-host.c | 14 ++++++++++++++
> drivers/pci/controller/cadence/pcie-cadence.h | 15 +++++++++++++++
> 2 files changed, 29 insertions(+)
>
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c
> index 4550e0d469ca..a2317614268d 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> @@ -83,6 +83,9 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
> struct cdns_pcie *pcie = &rc->pcie;
> u32 value, ctrl;
> u32 id;
> + u32 link_cap = CDNS_PCIE_LINK_CAP_OFFSET;
> + u8 sls;
> + u16 lnk_ctl;
>
> /*
> * Set the root complex BAR configuration register:
> @@ -111,6 +114,17 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc)
> if (rc->device_id != 0xffff)
> cdns_pcie_rp_writew(pcie, PCI_DEVICE_ID, rc->device_id);
>
> + /* Quirk to enable autonomous speed change for GEN2 controller */
> + /* Reading Supported Link Speed value */
> + sls = PCI_EXP_LNKCAP_SLS &
> + cdns_pcie_rp_readb(pcie, link_cap + PCI_EXP_LNKCAP);
> + if (sls == PCI_EXP_LNKCAP_SLS_5_0GB) {
> + /* Since this a Gen2 controller, set Retrain Link(RL) bit */
> + lnk_ctl = cdns_pcie_rp_readw(pcie, link_cap + PCI_EXP_LNKCTL);
> + lnk_ctl |= PCI_EXP_LNKCTL_RL;
> + cdns_pcie_rp_writew(pcie, link_cap + PCI_EXP_LNKCTL, lnk_ctl);
> + }
Is this workaround required for all Cadence controller? If not, enable
this workaround only for versions which doesn't do autonomous speed change.
I think this workaround should also be applied only after checking for
link status (cdns_pcie_link_up()).
And this is also applicable for GEN3/GEN4 controller. So the check
should be to see the capability of the connected PCIe device and not the
controller itself.
Thanks
Kishon
^ permalink raw reply [flat|nested] 7+ messages in thread* RE: [PATCH v2] PCI: Cadence: Add quirk for Gen2 controller to do autonomous speed change.
2020-09-24 4:14 ` Kishon Vijay Abraham I
@ 2020-09-30 17:11 ` Athani Nadeem Ladkhan
0 siblings, 0 replies; 7+ messages in thread
From: Athani Nadeem Ladkhan @ 2020-09-30 17:11 UTC (permalink / raw)
To: Kishon Vijay Abraham I, Tom Joseph, lorenzo.pieralisi, robh,
bhelgaas, linux-pci, linux-kernel
Cc: Milind Parab, Swapnil Kashinath Jakhade
Hi Kishon,
> -----Original Message-----
> From: Kishon Vijay Abraham I <kishon@ti.com>
> Sent: Thursday, September 24, 2020 9:45 AM
> To: Athani Nadeem Ladkhan <nadeem@cadence.com>; Tom Joseph
> <tjoseph@cadence.com>; lorenzo.pieralisi@arm.com; robh@kernel.org;
> bhelgaas@google.com; linux-pci@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Cc: Milind Parab <mparab@cadence.com>; Swapnil Kashinath Jakhade
> <sjakhade@cadence.com>
> Subject: Re: [PATCH v2] PCI: Cadence: Add quirk for Gen2 controller to do
> autonomous speed change.
>
> EXTERNAL MAIL
>
>
> Hi Nadeem,
>
> On 24/09/20 12:04 am, Nadeem Athani wrote:
> > Cadence controller will not initiate autonomous speed change if
> > strapped as Gen2. The Retrain bit is set as a quirk to trigger this
> > speed change.
> >
> > Signed-off-by: Nadeem Athani <nadeem@cadence.com>
> > ---
> > drivers/pci/controller/cadence/pcie-cadence-host.c | 14 ++++++++++++++
> > drivers/pci/controller/cadence/pcie-cadence.h | 15 +++++++++++++++
> > 2 files changed, 29 insertions(+)
> >
> > diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c
> > b/drivers/pci/controller/cadence/pcie-cadence-host.c
> > index 4550e0d469ca..a2317614268d 100644
> > --- a/drivers/pci/controller/cadence/pcie-cadence-host.c
> > +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c
> > @@ -83,6 +83,9 @@ static int cdns_pcie_host_init_root_port(struct
> cdns_pcie_rc *rc)
> > struct cdns_pcie *pcie = &rc->pcie;
> > u32 value, ctrl;
> > u32 id;
> > + u32 link_cap = CDNS_PCIE_LINK_CAP_OFFSET;
> > + u8 sls;
> > + u16 lnk_ctl;
> >
> > /*
> > * Set the root complex BAR configuration register:
> > @@ -111,6 +114,17 @@ static int cdns_pcie_host_init_root_port(struct
> cdns_pcie_rc *rc)
> > if (rc->device_id != 0xffff)
> > cdns_pcie_rp_writew(pcie, PCI_DEVICE_ID, rc->device_id);
> >
> > + /* Quirk to enable autonomous speed change for GEN2 controller */
> > + /* Reading Supported Link Speed value */
> > + sls = PCI_EXP_LNKCAP_SLS &
> > + cdns_pcie_rp_readb(pcie, link_cap + PCI_EXP_LNKCAP);
> > + if (sls == PCI_EXP_LNKCAP_SLS_5_0GB) {
> > + /* Since this a Gen2 controller, set Retrain Link(RL) bit */
> > + lnk_ctl = cdns_pcie_rp_readw(pcie, link_cap +
> PCI_EXP_LNKCTL);
> > + lnk_ctl |= PCI_EXP_LNKCTL_RL;
> > + cdns_pcie_rp_writew(pcie, link_cap + PCI_EXP_LNKCTL,
> lnk_ctl);
> > + }
>
> Is this workaround required for all Cadence controller? If not, enable this
> workaround only for versions which doesn't do autonomous speed change.
No. This is taken care in patch version 3.
>
> I think this workaround should also be applied only after checking for link
> status (cdns_pcie_link_up()).
Yes. This is taken care in patch version 3.
>
> And this is also applicable for GEN3/GEN4 controller. So the check should be
> to see the capability of the connected PCIe device and not the controller
> itself.
This is taken care in patch version 3.
>
> Thanks
> Kishon
^ permalink raw reply [flat|nested] 7+ messages in thread