| Message ID | 20230721074452.65545-10-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 |
Subject should contain the word "missing". Like, "Add missing PCI_EXP_LNKCAP_MLW handling". On Fri, Jul 21, 2023 at 04:44:41PM +0900, Yoshihiro Shimoda wrote: > Update dw_pcie_link_set_max_link_width() to set PCI_EXP_LNKCAP_MLW. > In accordance with the DW PCIe RC/EP HW manuals [1,2,3,...] aside with > the PORT_LINK_CTRL_OFF.LINK_CAPABLE and GEN2_CTRL_OFF.NUM_OF_LANES[8:0] > field there is another one which needs to be updated. It's > LINK_CAPABILITIES_REG.PCIE_CAP_MAX_LINK_WIDTH. If it isn't done at > the very least the maximum link-width capability CSR won't expose > the actual maximum capability. > > [1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > Version 4.60a, March 2015, p.1032 > [2] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > Version 4.70a, March 2016, p.1065 > [3] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > Version 4.90a, March 2016, p.1057 > ... > [X] DesignWare Cores PCI Express Controller Databook - DWC PCIe Endpoint, > Version 5.40a, March 2019, p.1396 > [X+1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > Version 5.40a, March 2019, p.1266 > > Suggested-by: Serge Semin <fancer.lancer@gmail.com> Add Reported-by also? > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> This looks like a potential bug fix to me. So please move this change before the previous patch that introduces dw_pcie_link_set_max_link_width(), tag fixes and CC stable list for backporting. - Mani > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > --- > drivers/pci/controller/dwc/pcie-designware.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > index 5cca34140d2a..c4998194fe74 100644 > --- a/drivers/pci/controller/dwc/pcie-designware.c > +++ b/drivers/pci/controller/dwc/pcie-designware.c > @@ -730,7 +730,8 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen) > > static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) > { > - u32 lwsc, plc; > + u32 lnkcap, lwsc, plc; > + u8 cap; > > if (!num_lanes) > return; > @@ -766,6 +767,12 @@ static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) > } > dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc); > dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, lwsc); > + > + cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); > + lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP); > + lnkcap &= ~PCI_EXP_LNKCAP_MLW; > + lnkcap |= FIELD_PREP(PCI_EXP_LNKCAP_MLW, num_lanes); > + dw_pcie_writel_dbi(pci, cap + PCI_EXP_LNKCAP, lnkcap); > } > > void dw_pcie_iatu_detect(struct dw_pcie *pci) > -- > 2.25.1 >
Hi Manivannan, > From: Manivannan Sadhasivam, Sent: Monday, July 24, 2023 8:04 PM > > Subject should contain the word "missing". Like, "Add missing PCI_EXP_LNKCAP_MLW > handling". I got it. > On Fri, Jul 21, 2023 at 04:44:41PM +0900, Yoshihiro Shimoda wrote: > > Update dw_pcie_link_set_max_link_width() to set PCI_EXP_LNKCAP_MLW. > > In accordance with the DW PCIe RC/EP HW manuals [1,2,3,...] aside with > > the PORT_LINK_CTRL_OFF.LINK_CAPABLE and GEN2_CTRL_OFF.NUM_OF_LANES[8:0] > > field there is another one which needs to be updated. It's > > LINK_CAPABILITIES_REG.PCIE_CAP_MAX_LINK_WIDTH. If it isn't done at > > the very least the maximum link-width capability CSR won't expose > > the actual maximum capability. > > > > [1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > Version 4.60a, March 2015, p.1032 > > [2] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > Version 4.70a, March 2016, p.1065 > > [3] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > Version 4.90a, March 2016, p.1057 > > ... > > [X] DesignWare Cores PCI Express Controller Databook - DWC PCIe Endpoint, > > Version 5.40a, March 2019, p.1396 > > [X+1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > Version 5.40a, March 2019, p.1266 > > > > Suggested-by: Serge Semin <fancer.lancer@gmail.com> > > Add Reported-by also? I don't think so because Serge suggested the commit description from my submitted patch [1]. [1] https://lore.kernel.org/linux-pci/20230322065701.po7owyzwisntalyz@mobilestation/ > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > This looks like a potential bug fix to me. So please move this change before the > previous patch that introduces dw_pcie_link_set_max_link_width(), tag fixes and > CC stable list for backporting. I think that this patch should be a next branch because this is possible to cause side effective. Almost all drivers/pcie/controller/dwc/ host drivers except pcie-tegra194.c doesn't have this setting, but I assume that the drivers work correctly without this setting. Also, to be honest, I could not find a suitable commit ID for this patch's "Fixes" tag. Additionally, I could not determine which old kernel versions should have this patch applied as backporting. Best regards, Yoshihiro Shimoda > - Mani > > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > --- > > drivers/pci/controller/dwc/pcie-designware.c | 9 ++++++++- > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > index 5cca34140d2a..c4998194fe74 100644 > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > @@ -730,7 +730,8 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen) > > > > static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) > > { > > - u32 lwsc, plc; > > + u32 lnkcap, lwsc, plc; > > + u8 cap; > > > > if (!num_lanes) > > return; > > @@ -766,6 +767,12 @@ static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) > > } > > dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc); > > dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, lwsc); > > + > > + cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); > > + lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP); > > + lnkcap &= ~PCI_EXP_LNKCAP_MLW; > > + lnkcap |= FIELD_PREP(PCI_EXP_LNKCAP_MLW, num_lanes); > > + dw_pcie_writel_dbi(pci, cap + PCI_EXP_LNKCAP, lnkcap); > > } > > > > void dw_pcie_iatu_detect(struct dw_pcie *pci) > > -- > > 2.25.1 > > > > -- > மணிவண்ணன் சதாசிவம்
On Wed, Jul 26, 2023 at 02:12:15AM +0000, Yoshihiro Shimoda wrote: > Hi Manivannan, > > > From: Manivannan Sadhasivam, Sent: Monday, July 24, 2023 8:04 PM > > > > Subject should contain the word "missing". Like, "Add missing PCI_EXP_LNKCAP_MLW > > handling". > > I got it. > > > On Fri, Jul 21, 2023 at 04:44:41PM +0900, Yoshihiro Shimoda wrote: > > > Update dw_pcie_link_set_max_link_width() to set PCI_EXP_LNKCAP_MLW. > > > In accordance with the DW PCIe RC/EP HW manuals [1,2,3,...] aside with > > > the PORT_LINK_CTRL_OFF.LINK_CAPABLE and GEN2_CTRL_OFF.NUM_OF_LANES[8:0] > > > field there is another one which needs to be updated. It's > > > LINK_CAPABILITIES_REG.PCIE_CAP_MAX_LINK_WIDTH. If it isn't done at > > > the very least the maximum link-width capability CSR won't expose > > > the actual maximum capability. > > > > > > [1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > Version 4.60a, March 2015, p.1032 > > > [2] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > Version 4.70a, March 2016, p.1065 > > > [3] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > Version 4.90a, March 2016, p.1057 > > > ... > > > [X] DesignWare Cores PCI Express Controller Databook - DWC PCIe Endpoint, > > > Version 5.40a, March 2019, p.1396 > > > [X+1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > Version 5.40a, March 2019, p.1266 > > > > > > Suggested-by: Serge Semin <fancer.lancer@gmail.com> > > > > Add Reported-by also? > > I don't think so because Serge suggested the commit description from my submitted patch [1]. > > [1] > https://lore.kernel.org/linux-pci/20230322065701.po7owyzwisntalyz@mobilestation/ > Fine then. > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > This looks like a potential bug fix to me. So please move this change before the > > previous patch that introduces dw_pcie_link_set_max_link_width(), tag fixes and > > CC stable list for backporting. > > I think that this patch should be a next branch because this is possible to > cause side effective. Almost all drivers/pcie/controller/dwc/ host drivers except > pcie-tegra194.c doesn't have this setting, but I assume that the drivers work correctly > without this setting. > > Also, to be honest, I could not find a suitable commit ID for this patch's "Fixes" tag. > Additionally, I could not determine which old kernel versions should have this patch > applied as backporting. > Ok. But you can still move this patch as I suggested. If we happen to hit any issue with this setting, then we can easily revert it. - Mani > Best regards, > Yoshihiro Shimoda > > > - Mani > > > > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > > --- > > > drivers/pci/controller/dwc/pcie-designware.c | 9 ++++++++- > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > index 5cca34140d2a..c4998194fe74 100644 > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > @@ -730,7 +730,8 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen) > > > > > > static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) > > > { > > > - u32 lwsc, plc; > > > + u32 lnkcap, lwsc, plc; > > > + u8 cap; > > > > > > if (!num_lanes) > > > return; > > > @@ -766,6 +767,12 @@ static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) > > > } > > > dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc); > > > dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, lwsc); > > > + > > > + cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); > > > + lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP); > > > + lnkcap &= ~PCI_EXP_LNKCAP_MLW; > > > + lnkcap |= FIELD_PREP(PCI_EXP_LNKCAP_MLW, num_lanes); > > > + dw_pcie_writel_dbi(pci, cap + PCI_EXP_LNKCAP, lnkcap); > > > } > > > > > > void dw_pcie_iatu_detect(struct dw_pcie *pci) > > > -- > > > 2.25.1 > > > > > > > -- > > மணிவண்ணன் சதாசிவம்
Hi Manivannan, > From: Manivannan Sadhasivam, Sent: Friday, July 28, 2023 11:51 AM > > On Wed, Jul 26, 2023 at 02:12:15AM +0000, Yoshihiro Shimoda wrote: > > Hi Manivannan, > > > > > From: Manivannan Sadhasivam, Sent: Monday, July 24, 2023 8:04 PM > > > > > > Subject should contain the word "missing". Like, "Add missing PCI_EXP_LNKCAP_MLW > > > handling". > > > > I got it. > > > > > On Fri, Jul 21, 2023 at 04:44:41PM +0900, Yoshihiro Shimoda wrote: > > > > Update dw_pcie_link_set_max_link_width() to set PCI_EXP_LNKCAP_MLW. > > > > In accordance with the DW PCIe RC/EP HW manuals [1,2,3,...] aside with > > > > the PORT_LINK_CTRL_OFF.LINK_CAPABLE and GEN2_CTRL_OFF.NUM_OF_LANES[8:0] > > > > field there is another one which needs to be updated. It's > > > > LINK_CAPABILITIES_REG.PCIE_CAP_MAX_LINK_WIDTH. If it isn't done at > > > > the very least the maximum link-width capability CSR won't expose > > > > the actual maximum capability. > > > > > > > > [1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > Version 4.60a, March 2015, p.1032 > > > > [2] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > Version 4.70a, March 2016, p.1065 > > > > [3] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > Version 4.90a, March 2016, p.1057 > > > > ... > > > > [X] DesignWare Cores PCI Express Controller Databook - DWC PCIe Endpoint, > > > > Version 5.40a, March 2019, p.1396 > > > > [X+1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > Version 5.40a, March 2019, p.1266 > > > > > > > > Suggested-by: Serge Semin <fancer.lancer@gmail.com> > > > > > > Add Reported-by also? > > > > I don't think so because Serge suggested the commit description from my submitted patch [1]. > > > > [1] > > <snip URL> > > > > Fine then. > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > > > This looks like a potential bug fix to me. So please move this change before the > > > previous patch that introduces dw_pcie_link_set_max_link_width(), tag fixes and > > > CC stable list for backporting. > > > > I think that this patch should be a next branch because this is possible to > > cause side effective. Almost all drivers/pcie/controller/dwc/ host drivers except > > pcie-tegra194.c doesn't have this setting, but I assume that the drivers work correctly > > without this setting. > > > > Also, to be honest, I could not find a suitable commit ID for this patch's "Fixes" tag. > > Additionally, I could not determine which old kernel versions should have this patch > > applied as backporting. > > > > Ok. But you can still move this patch as I suggested. If we happen to hit any > issue with this setting, then we can easily revert it. I got it. I'll move this patch as you suggested. Best regards, Yoshihiro Shimoda > - Mani > > > Best regards, > > Yoshihiro Shimoda > > > > > - Mani > > > > > > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > > > --- > > > > drivers/pci/controller/dwc/pcie-designware.c | 9 ++++++++- > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > > index 5cca34140d2a..c4998194fe74 100644 > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > > @@ -730,7 +730,8 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen) > > > > > > > > static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) > > > > { > > > > - u32 lwsc, plc; > > > > + u32 lnkcap, lwsc, plc; > > > > + u8 cap; > > > > > > > > if (!num_lanes) > > > > return; > > > > @@ -766,6 +767,12 @@ static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) > > > > } > > > > dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc); > > > > dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, lwsc); > > > > + > > > > + cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); > > > > + lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP); > > > > + lnkcap &= ~PCI_EXP_LNKCAP_MLW; > > > > + lnkcap |= FIELD_PREP(PCI_EXP_LNKCAP_MLW, num_lanes); > > > > + dw_pcie_writel_dbi(pci, cap + PCI_EXP_LNKCAP, lnkcap); > > > > } > > > > > > > > void dw_pcie_iatu_detect(struct dw_pcie *pci) > > > > -- > > > > 2.25.1 > > > > > > > > > > -- > > > மணிவண்ணன் சதாசிவம் > > -- > மணிவண்ணன் சதாசிவம்
On Fri, Jul 28, 2023 at 04:19:38AM +0000, Yoshihiro Shimoda wrote: > Hi Manivannan, > > > From: Manivannan Sadhasivam, Sent: Friday, July 28, 2023 11:51 AM > > > > On Wed, Jul 26, 2023 at 02:12:15AM +0000, Yoshihiro Shimoda wrote: > > > Hi Manivannan, > > > > > > > From: Manivannan Sadhasivam, Sent: Monday, July 24, 2023 8:04 PM > > > > > > > > Subject should contain the word "missing". Like, "Add missing PCI_EXP_LNKCAP_MLW > > > > handling". > > > > > > I got it. > > > > > > > On Fri, Jul 21, 2023 at 04:44:41PM +0900, Yoshihiro Shimoda wrote: > > > > > Update dw_pcie_link_set_max_link_width() to set PCI_EXP_LNKCAP_MLW. > > > > > In accordance with the DW PCIe RC/EP HW manuals [1,2,3,...] aside with > > > > > the PORT_LINK_CTRL_OFF.LINK_CAPABLE and GEN2_CTRL_OFF.NUM_OF_LANES[8:0] > > > > > field there is another one which needs to be updated. It's > > > > > LINK_CAPABILITIES_REG.PCIE_CAP_MAX_LINK_WIDTH. If it isn't done at > > > > > the very least the maximum link-width capability CSR won't expose > > > > > the actual maximum capability. > > > > > > > > > > [1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > > Version 4.60a, March 2015, p.1032 > > > > > [2] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > > Version 4.70a, March 2016, p.1065 > > > > > [3] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > > Version 4.90a, March 2016, p.1057 > > > > > ... > > > > > [X] DesignWare Cores PCI Express Controller Databook - DWC PCIe Endpoint, > > > > > Version 5.40a, March 2019, p.1396 > > > > > [X+1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > > Version 5.40a, March 2019, p.1266 > > > > > > > > > > Suggested-by: Serge Semin <fancer.lancer@gmail.com> > > > > > > > > Add Reported-by also? > > > > > > I don't think so because Serge suggested the commit description from my submitted patch [1]. > > > > > > [1] > > > > <snip URL> > > > > > > > Fine then. > > > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > > > > > This looks like a potential bug fix to me. So please move this change before the > > > > previous patch that introduces dw_pcie_link_set_max_link_width(), tag fixes and > > > > CC stable list for backporting. > > > > > > I think that this patch should be a next branch because this is possible to > > > cause side effective. Almost all drivers/pcie/controller/dwc/ host drivers except > > > pcie-tegra194.c doesn't have this setting, but I assume that the drivers work correctly > > > without this setting. > > > > > > Also, to be honest, I could not find a suitable commit ID for this patch's "Fixes" tag. > > > Additionally, I could not determine which old kernel versions should have this patch > > > applied as backporting. > > > > > > > Ok. But you can still move this patch as I suggested. If we happen to hit any > > issue with this setting, then we can easily revert it. > > I got it. I'll move this patch as you suggested. No. By moving this patch to be implemented before the patch: [PATCH v18 08/20] PCI: dwc: Add dw_pcie_link_set_max_link_width() you won't be able to easily revert it afterwards because the patch #8 will move the code added by the patch #9 to the dw_pcie_link_set_max_link_width() function. Basically you suggest to switch the preparation and functional patches order which doesn't look right. Basically the Link-width-related part of this series currently implies the next logic: 1. Prepare the DW PCIe core driver to implementing a comprehensive Max-link-width setup methods (aka as it's done in dw_pcie_link_set_max_speed()) by moving the Link-width related code to a dedicated method: [PATCH v18 08/20] PCI: dwc: Add dw_pcie_link_set_max_link_width() 2. Add the PCI_EXP_LNKCAP_MLW field update, which dw_pcie_link_set_max_link_width() lacks to be comprehensive: [PATCH v18 09/20] PCI: dwc: Add PCI_EXP_LNKCAP_MLW handling 3. Drop the duplicating code from the Tegra194 PCIe driver: [PATCH v18 10/20] PCI: tegra194: Drop PCI_EXP_LNKSTA_NLW setting In case if the patch #9 appears to be a bug fix, then it will need to be backported together with patch #8 which isn't a problem at all (though it's doubtfully to happen since nobody reported any problem with that so far). But if patch #9 turns out to break something in current circumstances we'll be able to either easily revert it (since it's applied after the preparation patch) or fix somehow. If you switch patch #8 and #9 order, the reversion will require to be performed for both these patches to avoid the conflicts. Thus I'd suggest to leave the patches order as is which looks more natural and won't cause any problems to revert the functional change or to backport it. -Serge(y) > > Best regards, > Yoshihiro Shimoda > > > - Mani > > > > > Best regards, > > > Yoshihiro Shimoda > > > > > > > - Mani > > > > > > > > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > > > > --- > > > > > drivers/pci/controller/dwc/pcie-designware.c | 9 ++++++++- > > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > > > index 5cca34140d2a..c4998194fe74 100644 > > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > > > @@ -730,7 +730,8 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen) > > > > > > > > > > static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) > > > > > { > > > > > - u32 lwsc, plc; > > > > > + u32 lnkcap, lwsc, plc; > > > > > + u8 cap; > > > > > > > > > > if (!num_lanes) > > > > > return; > > > > > @@ -766,6 +767,12 @@ static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) > > > > > } > > > > > dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc); > > > > > dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, lwsc); > > > > > + > > > > > + cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); > > > > > + lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP); > > > > > + lnkcap &= ~PCI_EXP_LNKCAP_MLW; > > > > > + lnkcap |= FIELD_PREP(PCI_EXP_LNKCAP_MLW, num_lanes); > > > > > + dw_pcie_writel_dbi(pci, cap + PCI_EXP_LNKCAP, lnkcap); > > > > > } > > > > > > > > > > void dw_pcie_iatu_detect(struct dw_pcie *pci) > > > > > -- > > > > > 2.25.1 > > > > > > > > > > > > > -- > > > > மணிவண்ணன் சதாசிவம் > > > > -- > > மணிவண்ணன் சதாசிவம்
Hi Serge, > From: Serge Semin, Sent: Saturday, July 29, 2023 1:07 AM > > On Fri, Jul 28, 2023 at 04:19:38AM +0000, Yoshihiro Shimoda wrote: > > Hi Manivannan, > > > > > From: Manivannan Sadhasivam, Sent: Friday, July 28, 2023 11:51 AM > > > > > > On Wed, Jul 26, 2023 at 02:12:15AM +0000, Yoshihiro Shimoda wrote: > > > > Hi Manivannan, > > > > > > > > > From: Manivannan Sadhasivam, Sent: Monday, July 24, 2023 8:04 PM > > > > > > > > > > Subject should contain the word "missing". Like, "Add missing PCI_EXP_LNKCAP_MLW > > > > > handling". > > > > > > > > I got it. > > > > > > > > > On Fri, Jul 21, 2023 at 04:44:41PM +0900, Yoshihiro Shimoda wrote: > > > > > > Update dw_pcie_link_set_max_link_width() to set PCI_EXP_LNKCAP_MLW. > > > > > > In accordance with the DW PCIe RC/EP HW manuals [1,2,3,...] aside with > > > > > > the PORT_LINK_CTRL_OFF.LINK_CAPABLE and GEN2_CTRL_OFF.NUM_OF_LANES[8:0] > > > > > > field there is another one which needs to be updated. It's > > > > > > LINK_CAPABILITIES_REG.PCIE_CAP_MAX_LINK_WIDTH. If it isn't done at > > > > > > the very least the maximum link-width capability CSR won't expose > > > > > > the actual maximum capability. > > > > > > > > > > > > [1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > > > Version 4.60a, March 2015, p.1032 > > > > > > [2] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > > > Version 4.70a, March 2016, p.1065 > > > > > > [3] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > > > Version 4.90a, March 2016, p.1057 > > > > > > ... > > > > > > [X] DesignWare Cores PCI Express Controller Databook - DWC PCIe Endpoint, > > > > > > Version 5.40a, March 2019, p.1396 > > > > > > [X+1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > > > Version 5.40a, March 2019, p.1266 > > > > > > > > > > > > Suggested-by: Serge Semin <fancer.lancer@gmail.com> > > > > > > > > > > Add Reported-by also? > > > > > > > > I don't think so because Serge suggested the commit description from my submitted patch [1]. > > > > > > > > [1] > > > > > > <snip URL> > > > > > > > > > > Fine then. > > > > > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > > > > > > > This looks like a potential bug fix to me. So please move this change before the > > > > > previous patch that introduces dw_pcie_link_set_max_link_width(), tag fixes and > > > > > CC stable list for backporting. > > > > > > > > I think that this patch should be a next branch because this is possible to > > > > cause side effective. Almost all drivers/pcie/controller/dwc/ host drivers except > > > > pcie-tegra194.c doesn't have this setting, but I assume that the drivers work correctly > > > > without this setting. > > > > > > > > Also, to be honest, I could not find a suitable commit ID for this patch's "Fixes" tag. > > > > Additionally, I could not determine which old kernel versions should have this patch > > > > applied as backporting. > > > > > > > > > > > Ok. But you can still move this patch as I suggested. If we happen to hit any > > > issue with this setting, then we can easily revert it. > > > > I got it. I'll move this patch as you suggested. > > No. By moving this patch to be implemented before the patch: > [PATCH v18 08/20] PCI: dwc: Add dw_pcie_link_set_max_link_width() > you won't be able to easily revert it afterwards because the patch #8 > will move the code added by the patch #9 to the > dw_pcie_link_set_max_link_width() function. Basically you suggest to > switch the preparation and functional patches order which doesn't look > right. You're correct. If moving this patch to the top of this series and then still apply the original #8, it's difficult to revert this patch. > Basically the Link-width-related part of this series currently implies > the next logic: > > 1. Prepare the DW PCIe core driver to implementing a comprehensive > Max-link-width setup methods (aka as it's done in > dw_pcie_link_set_max_speed()) by moving the Link-width related code to > a dedicated method: > [PATCH v18 08/20] PCI: dwc: Add dw_pcie_link_set_max_link_width() > > 2. Add the PCI_EXP_LNKCAP_MLW field update, which > dw_pcie_link_set_max_link_width() lacks to be comprehensive: > [PATCH v18 09/20] PCI: dwc: Add PCI_EXP_LNKCAP_MLW handling > > 3. Drop the duplicating code from the Tegra194 PCIe driver: > [PATCH v18 10/20] PCI: tegra194: Drop PCI_EXP_LNKSTA_NLW setting Yes. > In case if the patch #9 appears to be a bug fix, then it will need to > be backported together with patch #8 which isn't a problem at all > (though it's doubtfully to happen since nobody reported any problem > with that so far). Basically, I don't think that backporting #8 is good as backport because the #8 patch is a clean up code for readability. > But if patch #9 turns out to break something in > current circumstances we'll be able to either easily revert it (since > it's applied after the preparation patch) or fix somehow. If you > switch patch #8 and #9 order, the reversion will require to be > performed for both these patches to avoid the conflicts. Thus I'd > suggest to leave the patches order as is which looks more natural and > won't cause any problems to revert the functional change or to > backport it. To follow Manivannan's suggestion and your comments, I'm thinking that - drop the #8 because this is just clean up code for readability. -- After this patch series is merged and worked correctly without any regression on other platforms, we can apply the #8. - move the #9 to the top of this series as Manivannan suggested. -- This mean adding this code into dw_pcie_setup(). But, what do you think? Best regards, Yoshihiro Shimoda > -Serge(y) > > > > > Best regards, > > Yoshihiro Shimoda > > > > > - Mani > > > > > > > Best regards, > > > > Yoshihiro Shimoda > > > > > > > > > - Mani > > > > > > > > > > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > > > > > --- > > > > > > drivers/pci/controller/dwc/pcie-designware.c | 9 ++++++++- > > > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > > > > index 5cca34140d2a..c4998194fe74 100644 > > > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > > > > @@ -730,7 +730,8 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen) > > > > > > > > > > > > static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) > > > > > > { > > > > > > - u32 lwsc, plc; > > > > > > + u32 lnkcap, lwsc, plc; > > > > > > + u8 cap; > > > > > > > > > > > > if (!num_lanes) > > > > > > return; > > > > > > @@ -766,6 +767,12 @@ static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) > > > > > > } > > > > > > dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc); > > > > > > dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, lwsc); > > > > > > + > > > > > > + cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); > > > > > > + lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP); > > > > > > + lnkcap &= ~PCI_EXP_LNKCAP_MLW; > > > > > > + lnkcap |= FIELD_PREP(PCI_EXP_LNKCAP_MLW, num_lanes); > > > > > > + dw_pcie_writel_dbi(pci, cap + PCI_EXP_LNKCAP, lnkcap); > > > > > > } > > > > > > > > > > > > void dw_pcie_iatu_detect(struct dw_pcie *pci) > > > > > > -- > > > > > > 2.25.1 > > > > > > > > > > > > > > > > -- > > > > > மணிவண்ணன் சதாசிவம் > > > > > > -- > > > மணிவண்ணன் சதாசிவம்
On Mon, Jul 31, 2023 at 01:15:02AM +0000, Yoshihiro Shimoda wrote: > Hi Serge, > > > From: Serge Semin, Sent: Saturday, July 29, 2023 1:07 AM > > > > On Fri, Jul 28, 2023 at 04:19:38AM +0000, Yoshihiro Shimoda wrote: > > > Hi Manivannan, > > > > > > > From: Manivannan Sadhasivam, Sent: Friday, July 28, 2023 11:51 AM > > > > > > > > On Wed, Jul 26, 2023 at 02:12:15AM +0000, Yoshihiro Shimoda wrote: > > > > > Hi Manivannan, > > > > > > > > > > > From: Manivannan Sadhasivam, Sent: Monday, July 24, 2023 8:04 PM > > > > > > > > > > > > Subject should contain the word "missing". Like, "Add missing PCI_EXP_LNKCAP_MLW > > > > > > handling". > > > > > > > > > > I got it. > > > > > > > > > > > On Fri, Jul 21, 2023 at 04:44:41PM +0900, Yoshihiro Shimoda wrote: > > > > > > > Update dw_pcie_link_set_max_link_width() to set PCI_EXP_LNKCAP_MLW. > > > > > > > In accordance with the DW PCIe RC/EP HW manuals [1,2,3,...] aside with > > > > > > > the PORT_LINK_CTRL_OFF.LINK_CAPABLE and GEN2_CTRL_OFF.NUM_OF_LANES[8:0] > > > > > > > field there is another one which needs to be updated. It's > > > > > > > LINK_CAPABILITIES_REG.PCIE_CAP_MAX_LINK_WIDTH. If it isn't done at > > > > > > > the very least the maximum link-width capability CSR won't expose > > > > > > > the actual maximum capability. > > > > > > > > > > > > > > [1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > > > > Version 4.60a, March 2015, p.1032 > > > > > > > [2] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > > > > Version 4.70a, March 2016, p.1065 > > > > > > > [3] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > > > > Version 4.90a, March 2016, p.1057 > > > > > > > ... > > > > > > > [X] DesignWare Cores PCI Express Controller Databook - DWC PCIe Endpoint, > > > > > > > Version 5.40a, March 2019, p.1396 > > > > > > > [X+1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > > > > Version 5.40a, March 2019, p.1266 > > > > > > > > > > > > > > Suggested-by: Serge Semin <fancer.lancer@gmail.com> > > > > > > > > > > > > Add Reported-by also? > > > > > > > > > > I don't think so because Serge suggested the commit description from my submitted patch [1]. > > > > > > > > > > [1] > > > > > > > > <snip URL> > > > > > > > > > > > > > Fine then. > > > > > > > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > > > > > > > > > This looks like a potential bug fix to me. So please move this change before the > > > > > > previous patch that introduces dw_pcie_link_set_max_link_width(), tag fixes and > > > > > > CC stable list for backporting. > > > > > > > > > > I think that this patch should be a next branch because this is possible to > > > > > cause side effective. Almost all drivers/pcie/controller/dwc/ host drivers except > > > > > pcie-tegra194.c doesn't have this setting, but I assume that the drivers work correctly > > > > > without this setting. > > > > > > > > > > Also, to be honest, I could not find a suitable commit ID for this patch's "Fixes" tag. > > > > > Additionally, I could not determine which old kernel versions should have this patch > > > > > applied as backporting. > > > > > > > > > > > > > > > Ok. But you can still move this patch as I suggested. If we happen to hit any > > > > issue with this setting, then we can easily revert it. > > > > > > I got it. I'll move this patch as you suggested. > > > > No. By moving this patch to be implemented before the patch: > > [PATCH v18 08/20] PCI: dwc: Add dw_pcie_link_set_max_link_width() > > you won't be able to easily revert it afterwards because the patch #8 > > will move the code added by the patch #9 to the > > dw_pcie_link_set_max_link_width() function. Basically you suggest to > > switch the preparation and functional patches order which doesn't look > > right. > > You're correct. If moving this patch to the top of this series and then > still apply the original #8, it's difficult to revert this patch. > > > Basically the Link-width-related part of this series currently implies > > the next logic: > > > > 1. Prepare the DW PCIe core driver to implementing a comprehensive > > Max-link-width setup methods (aka as it's done in > > dw_pcie_link_set_max_speed()) by moving the Link-width related code to > > a dedicated method: > > [PATCH v18 08/20] PCI: dwc: Add dw_pcie_link_set_max_link_width() > > > > 2. Add the PCI_EXP_LNKCAP_MLW field update, which > > dw_pcie_link_set_max_link_width() lacks to be comprehensive: > > [PATCH v18 09/20] PCI: dwc: Add PCI_EXP_LNKCAP_MLW handling > > > > 3. Drop the duplicating code from the Tegra194 PCIe driver: > > [PATCH v18 10/20] PCI: tegra194: Drop PCI_EXP_LNKSTA_NLW setting > > Yes. > > > In case if the patch #9 appears to be a bug fix, then it will need to > > be backported together with patch #8 which isn't a problem at all > > (though it's doubtfully to happen since nobody reported any problem > > with that so far). > > Basically, I don't think that backporting #8 is good as backport because > the #8 patch is a clean up code for readability. > > > But if patch #9 turns out to break something in > > current circumstances we'll be able to either easily revert it (since > > it's applied after the preparation patch) or fix somehow. If you > > switch patch #8 and #9 order, the reversion will require to be > > performed for both these patches to avoid the conflicts. Thus I'd > > suggest to leave the patches order as is which looks more natural and > > won't cause any problems to revert the functional change or to > > backport it. > > To follow Manivannan's suggestion and your comments, I'm thinking that > - drop the #8 because this is just clean up code for readability. > -- After this patch series is merged and worked correctly without any > regression on other platforms, we can apply the #8. > - move the #9 to the top of this series as Manivannan suggested. > -- This mean adding this code into dw_pcie_setup(). > > But, what do you think? No. It's better to leave the preparation patch and the order as is. Once again this patch doesn't look as a bug-fix since nobody reported any related problem so far. If anyone decides to back it port there won't a problem with porting both #8 and #9. It's a common practice. -Serge(y) > > Best regards, > Yoshihiro Shimoda > > > -Serge(y) > > > > > > > > Best regards, > > > Yoshihiro Shimoda > > > > > > > - Mani > > > > > > > > > Best regards, > > > > > Yoshihiro Shimoda > > > > > > > > > > > - Mani > > > > > > > > > > > > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > > > > > > --- > > > > > > > drivers/pci/controller/dwc/pcie-designware.c | 9 ++++++++- > > > > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > > > > > index 5cca34140d2a..c4998194fe74 100644 > > > > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > > > > > @@ -730,7 +730,8 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen) > > > > > > > > > > > > > > static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) > > > > > > > { > > > > > > > - u32 lwsc, plc; > > > > > > > + u32 lnkcap, lwsc, plc; > > > > > > > + u8 cap; > > > > > > > > > > > > > > if (!num_lanes) > > > > > > > return; > > > > > > > @@ -766,6 +767,12 @@ static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) > > > > > > > } > > > > > > > dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc); > > > > > > > dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, lwsc); > > > > > > > + > > > > > > > + cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); > > > > > > > + lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP); > > > > > > > + lnkcap &= ~PCI_EXP_LNKCAP_MLW; > > > > > > > + lnkcap |= FIELD_PREP(PCI_EXP_LNKCAP_MLW, num_lanes); > > > > > > > + dw_pcie_writel_dbi(pci, cap + PCI_EXP_LNKCAP, lnkcap); > > > > > > > } > > > > > > > > > > > > > > void dw_pcie_iatu_detect(struct dw_pcie *pci) > > > > > > > -- > > > > > > > 2.25.1 > > > > > > > > > > > > > > > > > > > -- > > > > > > மணிவண்ணன் சதாசிவம் > > > > > > > > -- > > > > மணிவண்ணன் சதாசிவம்
Hi Serge, > From: Serge Semin, Sent: Tuesday, August 1, 2023 9:01 AM > > On Mon, Jul 31, 2023 at 01:15:02AM +0000, Yoshihiro Shimoda wrote: > > Hi Serge, > > > > > From: Serge Semin, Sent: Saturday, July 29, 2023 1:07 AM > > > > > > On Fri, Jul 28, 2023 at 04:19:38AM +0000, Yoshihiro Shimoda wrote: > > > > Hi Manivannan, > > > > > > > > > From: Manivannan Sadhasivam, Sent: Friday, July 28, 2023 11:51 AM > > > > > > > > > > On Wed, Jul 26, 2023 at 02:12:15AM +0000, Yoshihiro Shimoda wrote: > > > > > > Hi Manivannan, > > > > > > > > > > > > > From: Manivannan Sadhasivam, Sent: Monday, July 24, 2023 8:04 PM > > > > > > > > > > > > > > Subject should contain the word "missing". Like, "Add missing PCI_EXP_LNKCAP_MLW > > > > > > > handling". > > > > > > > > > > > > I got it. > > > > > > > > > > > > > On Fri, Jul 21, 2023 at 04:44:41PM +0900, Yoshihiro Shimoda wrote: > > > > > > > > Update dw_pcie_link_set_max_link_width() to set PCI_EXP_LNKCAP_MLW. > > > > > > > > In accordance with the DW PCIe RC/EP HW manuals [1,2,3,...] aside with > > > > > > > > the PORT_LINK_CTRL_OFF.LINK_CAPABLE and GEN2_CTRL_OFF.NUM_OF_LANES[8:0] > > > > > > > > field there is another one which needs to be updated. It's > > > > > > > > LINK_CAPABILITIES_REG.PCIE_CAP_MAX_LINK_WIDTH. If it isn't done at > > > > > > > > the very least the maximum link-width capability CSR won't expose > > > > > > > > the actual maximum capability. > > > > > > > > > > > > > > > > [1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > > > > > Version 4.60a, March 2015, p.1032 > > > > > > > > [2] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > > > > > Version 4.70a, March 2016, p.1065 > > > > > > > > [3] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > > > > > Version 4.90a, March 2016, p.1057 > > > > > > > > ... > > > > > > > > [X] DesignWare Cores PCI Express Controller Databook - DWC PCIe Endpoint, > > > > > > > > Version 5.40a, March 2019, p.1396 > > > > > > > > [X+1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > > > > > Version 5.40a, March 2019, p.1266 > > > > > > > > > > > > > > > > Suggested-by: Serge Semin <fancer.lancer@gmail.com> > > > > > > > > > > > > > > Add Reported-by also? > > > > > > > > > > > > I don't think so because Serge suggested the commit description from my submitted patch [1]. > > > > > > > > > > > > [1] > > > > > > > > > > <snip URL> > > > > > > > > > > > > > > > > Fine then. > > > > > > > > > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > > > > > > > > > > > This looks like a potential bug fix to me. So please move this change before the > > > > > > > previous patch that introduces dw_pcie_link_set_max_link_width(), tag fixes and > > > > > > > CC stable list for backporting. > > > > > > > > > > > > I think that this patch should be a next branch because this is possible to > > > > > > cause side effective. Almost all drivers/pcie/controller/dwc/ host drivers except > > > > > > pcie-tegra194.c doesn't have this setting, but I assume that the drivers work correctly > > > > > > without this setting. > > > > > > > > > > > > Also, to be honest, I could not find a suitable commit ID for this patch's "Fixes" tag. > > > > > > Additionally, I could not determine which old kernel versions should have this patch > > > > > > applied as backporting. > > > > > > > > > > > > > > > > > > > Ok. But you can still move this patch as I suggested. If we happen to hit any > > > > > issue with this setting, then we can easily revert it. > > > > > > > > I got it. I'll move this patch as you suggested. > > > > > > No. By moving this patch to be implemented before the patch: > > > [PATCH v18 08/20] PCI: dwc: Add dw_pcie_link_set_max_link_width() > > > you won't be able to easily revert it afterwards because the patch #8 > > > will move the code added by the patch #9 to the > > > dw_pcie_link_set_max_link_width() function. Basically you suggest to > > > switch the preparation and functional patches order which doesn't look > > > right. > > > > You're correct. If moving this patch to the top of this series and then > > still apply the original #8, it's difficult to revert this patch. > > > > > Basically the Link-width-related part of this series currently implies > > > the next logic: > > > > > > 1. Prepare the DW PCIe core driver to implementing a comprehensive > > > Max-link-width setup methods (aka as it's done in > > > dw_pcie_link_set_max_speed()) by moving the Link-width related code to > > > a dedicated method: > > > [PATCH v18 08/20] PCI: dwc: Add dw_pcie_link_set_max_link_width() > > > > > > 2. Add the PCI_EXP_LNKCAP_MLW field update, which > > > dw_pcie_link_set_max_link_width() lacks to be comprehensive: > > > [PATCH v18 09/20] PCI: dwc: Add PCI_EXP_LNKCAP_MLW handling > > > > > > 3. Drop the duplicating code from the Tegra194 PCIe driver: > > > [PATCH v18 10/20] PCI: tegra194: Drop PCI_EXP_LNKSTA_NLW setting > > > > Yes. > > > > > In case if the patch #9 appears to be a bug fix, then it will need to > > > be backported together with patch #8 which isn't a problem at all > > > (though it's doubtfully to happen since nobody reported any problem > > > with that so far). > > > > Basically, I don't think that backporting #8 is good as backport because > > the #8 patch is a clean up code for readability. > > > > > But if patch #9 turns out to break something in > > > current circumstances we'll be able to either easily revert it (since > > > it's applied after the preparation patch) or fix somehow. If you > > > switch patch #8 and #9 order, the reversion will require to be > > > performed for both these patches to avoid the conflicts. Thus I'd > > > suggest to leave the patches order as is which looks more natural and > > > won't cause any problems to revert the functional change or to > > > backport it. > > > > To follow Manivannan's suggestion and your comments, I'm thinking that > > - drop the #8 because this is just clean up code for readability. > > -- After this patch series is merged and worked correctly without any > > regression on other platforms, we can apply the #8. > > - move the #9 to the top of this series as Manivannan suggested. > > -- This mean adding this code into dw_pcie_setup(). > > > > But, what do you think? > > No. It's better to leave the preparation patch and the order as is. > Once again this patch doesn't look as a bug-fix since nobody reported > any related problem so far. If anyone decides to back it port there > won't a problem with porting both #8 and #9. It's a common practice. I got it. I'll keep the order #8 and #9 as-is, because the #9 is not a bug-fix patch. Best regards, Yoshihiro Shimoda > -Serge(y) > > > > > Best regards, > > Yoshihiro Shimoda > > > > > -Serge(y) > > > > > > > > > > > Best regards, > > > > Yoshihiro Shimoda > > > > > > > > > - Mani > > > > > > > > > > > Best regards, > > > > > > Yoshihiro Shimoda > > > > > > > > > > > > > - Mani > > > > > > > > > > > > > > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > > > > > > > --- > > > > > > > > drivers/pci/controller/dwc/pcie-designware.c | 9 ++++++++- > > > > > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > > > > > > index 5cca34140d2a..c4998194fe74 100644 > > > > > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > > > > > > @@ -730,7 +730,8 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen) > > > > > > > > > > > > > > > > static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) > > > > > > > > { > > > > > > > > - u32 lwsc, plc; > > > > > > > > + u32 lnkcap, lwsc, plc; > > > > > > > > + u8 cap; > > > > > > > > > > > > > > > > if (!num_lanes) > > > > > > > > return; > > > > > > > > @@ -766,6 +767,12 @@ static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) > > > > > > > > } > > > > > > > > dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc); > > > > > > > > dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, lwsc); > > > > > > > > + > > > > > > > > + cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); > > > > > > > > + lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP); > > > > > > > > + lnkcap &= ~PCI_EXP_LNKCAP_MLW; > > > > > > > > + lnkcap |= FIELD_PREP(PCI_EXP_LNKCAP_MLW, num_lanes); > > > > > > > > + dw_pcie_writel_dbi(pci, cap + PCI_EXP_LNKCAP, lnkcap); > > > > > > > > } > > > > > > > > > > > > > > > > void dw_pcie_iatu_detect(struct dw_pcie *pci) > > > > > > > > -- > > > > > > > > 2.25.1 > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > மணிவண்ணன் சதாசிவம் > > > > > > > > > > -- > > > > > மணிவண்ணன் சதாசிவம்
On Fri, Jul 28, 2023 at 07:07:03PM +0300, Serge Semin wrote: > On Fri, Jul 28, 2023 at 04:19:38AM +0000, Yoshihiro Shimoda wrote: > > Hi Manivannan, > > > > > From: Manivannan Sadhasivam, Sent: Friday, July 28, 2023 11:51 AM > > > > > > On Wed, Jul 26, 2023 at 02:12:15AM +0000, Yoshihiro Shimoda wrote: > > > > Hi Manivannan, > > > > > > > > > From: Manivannan Sadhasivam, Sent: Monday, July 24, 2023 8:04 PM > > > > > > > > > > Subject should contain the word "missing". Like, "Add missing PCI_EXP_LNKCAP_MLW > > > > > handling". > > > > > > > > I got it. > > > > > > > > > On Fri, Jul 21, 2023 at 04:44:41PM +0900, Yoshihiro Shimoda wrote: > > > > > > Update dw_pcie_link_set_max_link_width() to set PCI_EXP_LNKCAP_MLW. > > > > > > In accordance with the DW PCIe RC/EP HW manuals [1,2,3,...] aside with > > > > > > the PORT_LINK_CTRL_OFF.LINK_CAPABLE and GEN2_CTRL_OFF.NUM_OF_LANES[8:0] > > > > > > field there is another one which needs to be updated. It's > > > > > > LINK_CAPABILITIES_REG.PCIE_CAP_MAX_LINK_WIDTH. If it isn't done at > > > > > > the very least the maximum link-width capability CSR won't expose > > > > > > the actual maximum capability. > > > > > > > > > > > > [1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > > > Version 4.60a, March 2015, p.1032 > > > > > > [2] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > > > Version 4.70a, March 2016, p.1065 > > > > > > [3] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > > > Version 4.90a, March 2016, p.1057 > > > > > > ... > > > > > > [X] DesignWare Cores PCI Express Controller Databook - DWC PCIe Endpoint, > > > > > > Version 5.40a, March 2019, p.1396 > > > > > > [X+1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, > > > > > > Version 5.40a, March 2019, p.1266 > > > > > > > > > > > > Suggested-by: Serge Semin <fancer.lancer@gmail.com> > > > > > > > > > > Add Reported-by also? > > > > > > > > I don't think so because Serge suggested the commit description from my submitted patch [1]. > > > > > > > > [1] > > > > > > <snip URL> > > > > > > > > > > Fine then. > > > > > > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > > > > > > > > This looks like a potential bug fix to me. So please move this change before the > > > > > previous patch that introduces dw_pcie_link_set_max_link_width(), tag fixes and > > > > > CC stable list for backporting. > > > > > > > > I think that this patch should be a next branch because this is possible to > > > > cause side effective. Almost all drivers/pcie/controller/dwc/ host drivers except > > > > pcie-tegra194.c doesn't have this setting, but I assume that the drivers work correctly > > > > without this setting. > > > > > > > > Also, to be honest, I could not find a suitable commit ID for this patch's "Fixes" tag. > > > > Additionally, I could not determine which old kernel versions should have this patch > > > > applied as backporting. > > > > > > > > > > > Ok. But you can still move this patch as I suggested. If we happen to hit any > > > issue with this setting, then we can easily revert it. > > > > I got it. I'll move this patch as you suggested. > > No. By moving this patch to be implemented before the patch: > [PATCH v18 08/20] PCI: dwc: Add dw_pcie_link_set_max_link_width() > you won't be able to easily revert it afterwards because the patch #8 > will move the code added by the patch #9 to the > dw_pcie_link_set_max_link_width() function. Basically you suggest to > switch the preparation and functional patches order which doesn't look > right. > > Basically the Link-width-related part of this series currently implies > the next logic: > > 1. Prepare the DW PCIe core driver to implementing a comprehensive > Max-link-width setup methods (aka as it's done in > dw_pcie_link_set_max_speed()) by moving the Link-width related code to > a dedicated method: > [PATCH v18 08/20] PCI: dwc: Add dw_pcie_link_set_max_link_width() > > 2. Add the PCI_EXP_LNKCAP_MLW field update, which > dw_pcie_link_set_max_link_width() lacks to be comprehensive: > [PATCH v18 09/20] PCI: dwc: Add PCI_EXP_LNKCAP_MLW handling > > 3. Drop the duplicating code from the Tegra194 PCIe driver: > [PATCH v18 10/20] PCI: tegra194: Drop PCI_EXP_LNKSTA_NLW setting > > In case if the patch #9 appears to be a bug fix, then it will need to > be backported together with patch #8 which isn't a problem at all > (though it's doubtfully to happen since nobody reported any problem > with that so far). But if patch #9 turns out to break something in > current circumstances we'll be able to either easily revert it (since > it's applied after the preparation patch) or fix somehow. If you > switch patch #8 and #9 order, the reversion will require to be > performed for both these patches to avoid the conflicts. Thus I'd > suggest to leave the patches order as is which looks more natural and > won't cause any problems to revert the functional change or to > backport it. > Hmm, I overlooked the dependency. Let's keep the order as it is. - Mani > -Serge(y) > > > > > Best regards, > > Yoshihiro Shimoda > > > > > - Mani > > > > > > > Best regards, > > > > Yoshihiro Shimoda > > > > > > > > > - Mani > > > > > > > > > > > Reviewed-by: Serge Semin <fancer.lancer@gmail.com> > > > > > > --- > > > > > > drivers/pci/controller/dwc/pcie-designware.c | 9 ++++++++- > > > > > > 1 file changed, 8 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c > > > > > > index 5cca34140d2a..c4998194fe74 100644 > > > > > > --- a/drivers/pci/controller/dwc/pcie-designware.c > > > > > > +++ b/drivers/pci/controller/dwc/pcie-designware.c > > > > > > @@ -730,7 +730,8 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen) > > > > > > > > > > > > static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) > > > > > > { > > > > > > - u32 lwsc, plc; > > > > > > + u32 lnkcap, lwsc, plc; > > > > > > + u8 cap; > > > > > > > > > > > > if (!num_lanes) > > > > > > return; > > > > > > @@ -766,6 +767,12 @@ static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) > > > > > > } > > > > > > dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc); > > > > > > dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, lwsc); > > > > > > + > > > > > > + cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); > > > > > > + lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP); > > > > > > + lnkcap &= ~PCI_EXP_LNKCAP_MLW; > > > > > > + lnkcap |= FIELD_PREP(PCI_EXP_LNKCAP_MLW, num_lanes); > > > > > > + dw_pcie_writel_dbi(pci, cap + PCI_EXP_LNKCAP, lnkcap); > > > > > > } > > > > > > > > > > > > void dw_pcie_iatu_detect(struct dw_pcie *pci) > > > > > > -- > > > > > > 2.25.1 > > > > > > > > > > > > > > > > -- > > > > > மணிவண்ணன் சதாசிவம் > > > > > > -- > > > மணிவண்ணன் சதாசிவம்
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c index 5cca34140d2a..c4998194fe74 100644 --- a/drivers/pci/controller/dwc/pcie-designware.c +++ b/drivers/pci/controller/dwc/pcie-designware.c @@ -730,7 +730,8 @@ static void dw_pcie_link_set_max_speed(struct dw_pcie *pci, u32 link_gen) static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) { - u32 lwsc, plc; + u32 lnkcap, lwsc, plc; + u8 cap; if (!num_lanes) return; @@ -766,6 +767,12 @@ static void dw_pcie_link_set_max_link_width(struct dw_pcie *pci, u32 num_lanes) } dw_pcie_writel_dbi(pci, PCIE_PORT_LINK_CONTROL, plc); dw_pcie_writel_dbi(pci, PCIE_LINK_WIDTH_SPEED_CONTROL, lwsc); + + cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP); + lnkcap = dw_pcie_readl_dbi(pci, cap + PCI_EXP_LNKCAP); + lnkcap &= ~PCI_EXP_LNKCAP_MLW; + lnkcap |= FIELD_PREP(PCI_EXP_LNKCAP_MLW, num_lanes); + dw_pcie_writel_dbi(pci, cap + PCI_EXP_LNKCAP, lnkcap); } void dw_pcie_iatu_detect(struct dw_pcie *pci)
Update dw_pcie_link_set_max_link_width() to set PCI_EXP_LNKCAP_MLW. In accordance with the DW PCIe RC/EP HW manuals [1,2,3,...] aside with the PORT_LINK_CTRL_OFF.LINK_CAPABLE and GEN2_CTRL_OFF.NUM_OF_LANES[8:0] field there is another one which needs to be updated. It's LINK_CAPABILITIES_REG.PCIE_CAP_MAX_LINK_WIDTH. If it isn't done at the very least the maximum link-width capability CSR won't expose the actual maximum capability. [1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, Version 4.60a, March 2015, p.1032 [2] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, Version 4.70a, March 2016, p.1065 [3] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, Version 4.90a, March 2016, p.1057 ... [X] DesignWare Cores PCI Express Controller Databook - DWC PCIe Endpoint, Version 5.40a, March 2019, p.1396 [X+1] DesignWare Cores PCI Express Controller Databook - DWC PCIe Root Port, Version 5.40a, March 2019, p.1266 Suggested-by: Serge Semin <fancer.lancer@gmail.com> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Reviewed-by: Serge Semin <fancer.lancer@gmail.com> --- drivers/pci/controller/dwc/pcie-designware.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)