[PATCH u-boot-marvell 02/10] arm: mvebu: a38x: serdes: Move non-serdes PCIe code to pci_mvebu.c

[Pali Rohár]

On Monday 29 November 2021 08:46:47 Stefan Roese wrote:
> 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 <pali at 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 <pali at kernel.org>
> > > > > > Signed-off-by: Marek Behún <marek.behun at 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 at 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 at 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?

That is interesting question... While I'm reading it again, I really do
not know. Because you are right that mvebu_pcie_wait_for_link() is just
waiting for a link and it "magically" comes up. I have tested it on A385
and it is stable with different Compex Atheros cards which caused issues
in past also on A3720.