| Message ID | 20191007114159.61ad83ea@monakov-y.office.kontur-niirs.ru (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: keystone: Link training retries initiation | expand |
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 >
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 > >
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 > > > >
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)
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(-)