Hi Pali,
On 11/23/21 16:59, Pali Rohár wrote:
On Friday 19 November 2021 07:55:00 Stefan Roese wrote:
On 11/18/21 19:01, Pali Rohár wrote:
On Friday 12 November 2021 15:01:57 Stefan Roese wrote:
On 11/11/21 16:35, Marek Behún wrote:
From: Pali Rohár <p...@kernel.org>
As explained in commit 3bedbcc3aa18 ("arm: mvebu: a38x: serdes: Don't
overwrite read-only SAR PCIe registers") it is required to set Maximum Link
Width bits of PCIe Root Port Link Capabilities Register depending of number
of used serdes lanes. As this register is part of PCIe address space and
not serdes address space, move it into pci_mvebu.c driver.
Read number of PCIe lanes from DT propery "num-lanes" which is used also by
other PCIe controller drivers in Linux kernel. If this property is absent.
default to 1. This property needs to be set to 4 for every mvebu board
which use PEX_ROOT_COMPLEX_X4. Currently in U-Boot there is no such board.
Signed-off-by: Pali Rohár <p...@kernel.org>
Signed-off-by: Marek Behún <marek.be...@nic.cz>
---
arch/arm/mach-mvebu/serdes/a38x/ctrl_pex.h | 4 ----
.../serdes/a38x/high_speed_env_spec.c | 15 ---------------
drivers/pci/pci_mvebu.c | 18 ++++++++++++++++++
3 files changed, 18 insertions(+), 19 deletions(-)
I'm wondering now, if and how this works on Armada XP, which uses the
same PCIe driver but a different serdes/axp/*. Did you take this into
account?
It looks like that axp serdes code also touches register of PCIe Root
Ports, e.g. in section /* 6.2 PCI Express Link Capabilities */. So it is
something which could be improved and cleaned. But it should not cause
any issue if registers are configures two times, once in serdes code and
once in pci-mvebu.c code. Moreover registers in pci-mvebu space,
including config space of PCIe Root Ports are reconfigured by kernel, so
I think that it should not cause any issue.
I assume that the AXP serdes code is very similar to the A38x code that
you recently overhauled very thoroughly. For the AXP serdes code, I know
that the PCIe link is already established in the serdes code right now.
And since we had some link-up issues on the theadorable AXP board, we
have been trying to optimize / tune this in this ugly serdes code at few
years ago. From what I understand, you've removed all this PCIe specific
code in the A38x serdes part, so that the link establishment now happens
in the PCIe driver itself (pci_mvebu.c). Which makes perfect sense IMHO.
Since A38x and AXP share the same PCIe driver in U-Boot, I would very
much like to see some similar changes being made to the AXP serdes code
as well, so that the link establishment solely will happen for all
these platforms in the PCIe driver.
I have looked into AXP serdes code and seems to be similar mess like it
was in A38x serdes code. Unfortunately I do not want to touch this code
and do movement without heavy hardware testing as it can be easy to
break. And I do not have Theadorable board for testing.
I fully agree, that such a rework / cleanup, as you have done for a38x,
can only be done with intensive testing. I might try to find some time
in the future to work on this, as theadorable is still actively used and
PCIe is always a potential basket of trouble here.
Anyway, all changes done in pci_mvebu.c driver just configures registers
to correct or expected values, so they should have low probability of
breaking existing hardware...
Okay. Please see below...
But what could cause issues is X1 vs X4 configuration in pci-mvebu.c if
"num-lanes" property is not properly set. As is mentioned in commit
message there is no A38x board in U-Boot which uses X4.
Now with your comment for Armada XP I checked that serdes code and find
out that AXP uses macro 'PEX_BUS_MODE_X4' for X4 mode (A38x serdes uses
PEX_ROOT_COMPLEX_X4). And in U-Boot there are two boards which sets this
macro: board/Synology/ds414/ds414.c and board/theadorable/theadorable.c
So it would be needed to adjust this patch for these two boards. Please
hold this one patch for now. I will try to prepare new fixed version.
Other patches should be OK and independent of this one.
Thanks. And yes, theadorable has one x4 and one x1.
I think that this change should be enough:
diff --git a/arch/arm/dts/armada-xp-synology-ds414.dts
b/arch/arm/dts/armada-xp-synology-ds414.dts
index 861967cd7e87..35909e3c69c6 100644
--- a/arch/arm/dts/armada-xp-synology-ds414.dts
+++ b/arch/arm/dts/armada-xp-synology-ds414.dts
@@ -187,6 +187,7 @@
pcie@1,0 {
/* Port 0, Lane 0 */
status = "okay";
+ num-lanes = <4>;
};
/*
diff --git a/arch/arm/dts/armada-xp-theadorable.dts
b/arch/arm/dts/armada-xp-theadorable.dts
index 6a1df870ab56..726676b3e1d5 100644
--- a/arch/arm/dts/armada-xp-theadorable.dts
+++ b/arch/arm/dts/armada-xp-theadorable.dts
@@ -202,5 +202,6 @@
pcie@9,0 {
/* Port 2, Lane 0 */
status = "okay";
+ num-lanes = <4>;
};
};
After this DTS change, pci-mvebu.c will just replace value of current
number of lanes (which is set to 4 by serdes code) to value from DTS,
which is 4. Therefore there should be no change.
Could you test whole patch series with above DTS change if it works
properly on Theadorable board?
Yes, I don't see any issues with this patchset applied plus this DT
patch on theadorable. The PCIe links are up and with the correct width.
What I'm wondering is, when exactly does the PCIe RP start the link
establishment. In my case with AXP this is still in the AXP serdes code
of course. But in the A38x code, it should be in the PCIe controller
driver now AFAIU. I see that you configure the link width in the
controller and do some other configuration (address windows etc), but
at the end you "simply" wait for the link to come up via
mvebu_pcie_wait_for_link(). I would have expected here some special
command (config bit?) to the PCIe controller to start the link
establishment. So when exactly does the A38x start this action?
Thanks,
Stefan
