[PATCH] PCI: keystone: Link training retries initiation

[PATCH] PCI: keystone: Link training retries initiation

[Yurii Monakov]

ks_pcie_stop_link function never cleared LTSSM_EN_VAL bit so
link training was never triggered more than once after startup.
In configurations where link can be unstable during early boot,
for example, under low temperature, it will never be established.

Signed-off-by: Yurii Monakov <monakov.y@gmail.com>
---
 drivers/pci/controller/dwc/pci-keystone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
index f19de60ac991..ea8e7ebd8c4f 100644
--- a/drivers/pci/controller/dwc/pci-keystone.c
+++ b/drivers/pci/controller/dwc/pci-keystone.c
@@ -510,7 +510,7 @@ static void ks_pcie_stop_link(struct dw_pcie *pci)
 	/* Disable Link training */
 	val = ks_pcie_app_readl(ks_pcie, CMD_STATUS);
 	val &= ~LTSSM_EN_VAL;
-	ks_pcie_app_writel(ks_pcie, CMD_STATUS, LTSSM_EN_VAL | val);
+	ks_pcie_app_writel(ks_pcie, CMD_STATUS, val);
 }
 
 static int ks_pcie_start_link(struct dw_pcie *pci)
-- 
2.17.1

Re: [PATCH] PCI: keystone: Link training retries initiation

[Andrew Murray]

On Mon, Oct 07, 2019 at 11:41:59AM +0300, Yurii Monakov wrote:
> ks_pcie_stop_link function never cleared LTSSM_EN_VAL bit so
> link training was never triggered more than once after startup.
> In configurations where link can be unstable during early boot,
> for example, under low temperature, it will never be established.
> 
> Signed-off-by: Yurii Monakov <monakov.y@gmail.com>
> ---
>  drivers/pci/controller/dwc/pci-keystone.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index f19de60ac991..ea8e7ebd8c4f 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -510,7 +510,7 @@ static void ks_pcie_stop_link(struct dw_pcie *pci)
>  	/* Disable Link training */
>  	val = ks_pcie_app_readl(ks_pcie, CMD_STATUS);
>  	val &= ~LTSSM_EN_VAL;
> -	ks_pcie_app_writel(ks_pcie, CMD_STATUS, LTSSM_EN_VAL | val);

Oh yeah, that doesn't look right to me. Good spot. Thanks for this!

> +	ks_pcie_app_writel(ks_pcie, CMD_STATUS, val);

As far as I can work out, this bug existed from the beginning - can
you please resend with this tag?

Fixes: 0c4ffcfe1fbc ("PCI: keystone: Add TI Keystone PCIe driver")

Can you also update the commit subject to emphasise it's a bug fix,
e.g. PCI: keystone: Fix ... or similar.

Thanks,

Andrew Murray

>  }
>  
>  static int ks_pcie_start_link(struct dw_pcie *pci)
> -- 
> 2.17.1
> 

Re: [PATCH] PCI: keystone: Link training retries initiation

[Yurii Monakov]

On Mon, 16 Dec 2019 11:08:22 +0000, Andrew Murray <andrew.murray@arm.com> wrote:

> On Mon, Oct 07, 2019 at 11:41:59AM +0300, Yurii Monakov wrote:
> > ks_pcie_stop_link function never cleared LTSSM_EN_VAL bit so
> > link training was never triggered more than once after startup.
> > In configurations where link can be unstable during early boot,
> > for example, under low temperature, it will never be established.
> > 
> > Signed-off-by: Yurii Monakov <monakov.y@gmail.com>
> > ---
> >  drivers/pci/controller/dwc/pci-keystone.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pci-keystone.c
> > b/drivers/pci/controller/dwc/pci-keystone.c index
> > f19de60ac991..ea8e7ebd8c4f 100644 ---
> > a/drivers/pci/controller/dwc/pci-keystone.c +++
> > b/drivers/pci/controller/dwc/pci-keystone.c @@ -510,7 +510,7 @@
> > static void ks_pcie_stop_link(struct dw_pcie *pci) /* Disable Link
> > training */ val = ks_pcie_app_readl(ks_pcie, CMD_STATUS);
> >  	val &= ~LTSSM_EN_VAL;
> > -	ks_pcie_app_writel(ks_pcie, CMD_STATUS, LTSSM_EN_VAL |
> > val);  
> 
> Oh yeah, that doesn't look right to me. Good spot. Thanks for this!
> 
> > +	ks_pcie_app_writel(ks_pcie, CMD_STATUS, val);  
> 
> As far as I can work out, this bug existed from the beginning - can
> you please resend with this tag?
> 
> Fixes: 0c4ffcfe1fbc ("PCI: keystone: Add TI Keystone PCIe driver")
I have to add this line prior to 'Signed-off-by' tag?

> Can you also update the commit subject to emphasise it's a bug fix,
> e.g. PCI: keystone: Fix ... or similar.
Do you mean that I have to create new patch from scratch and send it again?

Best Regards,
Yurii Monakov

PS. I'm new to LKLM, so sorry for dumb questions.

> 
> Thanks,
> 
> Andrew Murray
> 
> >  }
> >  
> >  static int ks_pcie_start_link(struct dw_pcie *pci)
> > -- 
> > 2.17.1
> >   

Re: [PATCH] PCI: keystone: Link training retries initiation

[Andrew Murray]

On Tue, Dec 17, 2019 at 10:40:42AM +0300, Yurii Monakov wrote:
> On Mon, 16 Dec 2019 11:08:22 +0000, Andrew Murray <andrew.murray@arm.com> wrote:
> 
> > On Mon, Oct 07, 2019 at 11:41:59AM +0300, Yurii Monakov wrote:
> > > ks_pcie_stop_link function never cleared LTSSM_EN_VAL bit so
> > > link training was never triggered more than once after startup.
> > > In configurations where link can be unstable during early boot,
> > > for example, under low temperature, it will never be established.
> > > 
> > > Signed-off-by: Yurii Monakov <monakov.y@gmail.com>
> > > ---
> > >  drivers/pci/controller/dwc/pci-keystone.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pci-keystone.c
> > > b/drivers/pci/controller/dwc/pci-keystone.c index
> > > f19de60ac991..ea8e7ebd8c4f 100644 ---
> > > a/drivers/pci/controller/dwc/pci-keystone.c +++
> > > b/drivers/pci/controller/dwc/pci-keystone.c @@ -510,7 +510,7 @@
> > > static void ks_pcie_stop_link(struct dw_pcie *pci) /* Disable Link
> > > training */ val = ks_pcie_app_readl(ks_pcie, CMD_STATUS);
> > >  	val &= ~LTSSM_EN_VAL;
> > > -	ks_pcie_app_writel(ks_pcie, CMD_STATUS, LTSSM_EN_VAL |
> > > val);  
> > 
> > Oh yeah, that doesn't look right to me. Good spot. Thanks for this!
> > 
> > > +	ks_pcie_app_writel(ks_pcie, CMD_STATUS, val);  
> > 
> > As far as I can work out, this bug existed from the beginning - can
> > you please resend with this tag?
> > 
> > Fixes: 0c4ffcfe1fbc ("PCI: keystone: Add TI Keystone PCIe driver")
> I have to add this line prior to 'Signed-off-by' tag?

Yes please. Please also see this [1] email for more details on the
suggested order of tags for future reference (the tags are generally in
chronological order.

[1] https://www.spinics.net/lists/linux-pci/msg65938.html


> 
> > Can you also update the commit subject to emphasise it's a bug fix,
> > e.g. PCI: keystone: Fix ... or similar.
> Do you mean that I have to create new patch from scratch and send it again?

Yes please create a new patch and send it again (such that the subject starts
with [PATCH v2]. Please make sure the patch will be able to apply cleanly ontop
of:

https://git.kernel.org/pub/scm/linux/kernel/git/lpieralisi/pci.git/log/?h=pci/dwc

Also as this fix will be beneficial for older kernels it can be applied to the
-stable release, you can do that by adding:

CC: stable@vger.kernel.org

After the Signed-off-by tag, and including that address on CC. See [2] for more
details.

[2] https://www.kernel.org/doc/html/v4.10/process/stable-kernel-rules.html


> 
> Best Regards,
> Yurii Monakov
> 
> PS. I'm new to LKLM, so sorry for dumb questions.

Welcome! Please - ask as many as you like. You can also learn a lot by looking
at what other people do, so always check git history and other patches on the
mailing lists to get an idea of the way things are done.

Thanks,

Andrew Murray

> 
> > 
> > Thanks,
> > 
> > Andrew Murray
> > 
> > >  }
> > >  
> > >  static int ks_pcie_start_link(struct dw_pcie *pci)
> > > -- 
> > > 2.17.1
> > >   
>