RE: [PATCH v18 09/20] PCI: dwc: Add PCI_EXP_LNKCAP_MLW handling

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:

quoted

Hi Serge,

quoted

From: Serge Semin, Sent: Saturday, July 29, 2023 1:07 AM

On Fri, Jul 28, 2023 at 04:19:38AM +0000, Yoshihiro Shimoda wrote:

quoted

Hi Manivannan,

quoted

From: Manivannan Sadhasivam, Sent: Friday, July 28, 2023 11:51 AM

On Wed, Jul 26, 2023 at 02:12:15AM +0000, Yoshihiro Shimoda wrote:

quoted

Hi Manivannan,

quoted

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.

quoted

On Fri, Jul 21, 2023 at 04:44:41PM +0900, Yoshihiro Shimoda wrote:

quoted

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 <redacted>

Add Reported-by also?

I don't think so because Serge suggested the commit description from my submitted patch [1].

[1]

<snip URL>

quoted

quoted

Fine then.

quoted

quoted

quoted

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.

quoted

quoted

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.

quoted

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.

quoted

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.

quoted

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)

quoted

Best regards,
Yoshihiro Shimoda

quoted

-Serge(y)

quoted

Best regards,
Yoshihiro Shimoda

quoted

- Mani

quoted

Best regards,
Yoshihiro Shimoda

quoted

- Mani

quoted

Reviewed-by: Serge Semin <redacted>
---
 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

--
மணிவண்ணன் சதாசிவம்

--
மணிவண்ணன் சதாசிவம்