[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 13:30:45 Stefan Roese wrote:
> Hi Pali,
> 
> On 11/29/21 12:47, Pali Rohár wrote:
> > Hello!
> > 
> > On Monday 29 November 2021 10:22:58 Stefan Roese wrote:
> > > On 11/29/21 10:06, Pali Rohár wrote:
> > > 
> > > <snip>
> > > 
> > > > > > 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.
> > > 
> > > I would prefer, to fully understand when exactly the link establishment
> > > is started. Since this is crucial for the setup of the controller that
> > > needs to be done *before* the link starts to come up.
> > 
> > I try to dig as more information as possible and finally I find out that
> > important information is available also in now removed, but originally
> > public A38x documentation. Thankfully web archive has copy of it:
> > 
> > https://web.archive.org/web/20200420191927/https://www.marvell.com/content/dam/marvell/en/public-collateral/embedded-processors/marvell-embedded-processors-armada-38x-functional-specifications-2015-11.pdf
> > 
> >    17.3 Link Initialization
> > 
> >    In case the initialization fails and no link is established, the PHY
> >    will keep on trying to initiate a link forever unless the port is
> >    disabled. As long as the port is enabled, the PHY will continue trying
> >    to establish a link; once the PHY identifies that a device is
> >    connected to it, a link will be established.
> > 
> > PCIe port is enabled by bits in SoC Control 1 Register, which is done in
> > U-Boot SerDes initialization code. This is IIRC SoC specific, and reason
> > why every Armada SoC has own SerDes init code.
> > 
> > And looks like that due to "the PHY will keep on trying to initiate a
> > link forever", the PCIe link comes up when pci-mvebu.c sets all required
> > registers to correct values. E.g. set correct mode (RC vs endpoint),
> > link width (x1 vs x4), etc...
> > 
> > > Could you perhaps try to remove some of the register configurations in
> > > the MVEBU PCIe driver to see, if the link establishment relies on this
> > > register to be written to (e.g. PCI_EXP_LNKCAP)?
> > 
> > First port on A385 is by default set to X4 width, other ports to X1
> > width. Without updating LNKCAP to correct width, card in first PCIe port
> > never initialize. And cards in other ports are initialized even before
> > pci-mvebu.c starts configuration.
> 
> So the PCIe ports are now trying to establish the links, even when the
> correct configuration is not yet done. This might work but sound far
> from perfect to me IMHO.

Yes, it looks like (based on behavior of the first port). And it is not
perfect, just another mess :-(

> > So seems that this matches above behavior. SerDes init code enabled all
> > PCIe ports. Ports which are using default configuration (second, third)
> > are immediately initialized and link is established. Port which requires
> > additional configuration (first port, for switching from X4 to X1) just
> > stay in that "keep on trying to initiate a link forever" state until
> > pci-mvebu starts and set PCI_EXP_LNKCAP register, after which PHY try X1
> > width and success. And seems that this is the reason why 100ms timeout
> > is needed... As at this stage when pci-mvebu.c switches X4 to X1, init
> > timeout as defined in PCIe spec (that 100ms) starts ticking. For other
> > ports it starts ticking when serdes init code enables ports.
> > 
> > 
> > I have looked into all PCIe registers which are present in functional
> > spec, but it looks like that there is no pci-mvebu register which can
> > turn of LTSSM and link training, like it is in other PCIe controllers.
> > It looks like that only SoC-specific port enable bits are there.
> > 
> > It is starting to be bigger mess as before... Any suggestion how to
> > continue with it?
> > 
> > We cannot (easily) move that code which flips PCIe bits in SoC Control 1
> > Register from SerDes init code to pci-mvebu.c as this is outside of
> > pci-mvebu.c address space and also it is different on every SoC.
> > pci-mvebu.c registers are same on all Marvell SoCs, starting from Orion
> > up to the A39x.
> 
> One idea would be, to use a "reset-controller" driver on the Armada
> platforms, that is capable to at least reset and release the PCIe ports.
> Via the SoC Control 1 reg on A38x and via the SoC Control Register
> on AXP.

In that specification is also written:

  Enable the PCI Express interface by setting the <PCIe0En>, <PCIe1En>,
  <PCIe2En>, or <PCIe3En> field in the SoC Control 1 Register (Table
  1888 p. 1395). This allows programming of link parameters before the
  start of link initialization. The highest common link width is
  established according to the following order: x4 to x1.

So I think the correct behavior should be:

1. pci-mvebu.c configures all controller registers to correct values
2. PCIe port is enabled via SoC-specific register
3. pci-mvebu.c waits for link up

I guess that reset-controller does not help, as core release this reset
prior starting driver initialization.

Anyway, this A385 SoC Control 1 Register is at address 0x18204 which is
part of following device defined in kernel DTS file:

			systemc: system-controller at 18200 {
				compatible = "marvell,armada-380-system-controller",
					     "marvell,armada-370-xp-system-controller";
				reg = <0x18200 0x100>;
			};

Linux kernel has driver for this DTS device is file:
arch/arm/mach-mvebu/system-controller.c

U-Boot does not have any driver for this compatible string.

So PCIe port enable/disable should be in this driver. I can write simple
driver also for U-Boot which can control this register. But I really do
not know which interface should it use.

Has somebody else any idea?

> I just looked into some Linux PCIe DT bindings and found e.g. this in
> the mediatek spec:
> 
> Documentation/devicetree/bindings/pci/mediatek-pcie.txt
> ...
> Required properties for MT7623/MT2701:
> ...
> - resets: Must contain an entry for each entry in reset-names.
>   See ../reset/reset.txt for details.
> - reset-names: Must be "pcie-rst0", "pcie-rst1", "pcie-rstN".. based on the
>   number of root ports.
> ...
> 		resets = <&hifsys MT2701_HIFSYS_PCIE0_RST>,
> 			 <&hifsys MT2701_HIFSYS_PCIE1_RST>,
> 			 <&hifsys MT2701_HIFSYS_PCIE2_RST>;
> 		reset-names = "pcie-rst0", "pcie-rst1", "pcie-rst2";
> 		phys = <&pcie0_phy PHY_TYPE_PCIE>, <&pcie1_phy PHY_TYPE_PCIE>,
> 		       <&pcie2_phy PHY_TYPE_PCIE>;
> 		phy-names = "pcie-phy0", "pcie-phy1", "pcie-phy2";
> 
> 
> And make sure in the serdes code keeps (or actively sets?) these PCIe
> ports into the reset state. The PCIe driver would then release the ports
> out of reset after their configuration.
> 
> Or is some other serdes code missing in between "get PCIe port out of
> reset" and the MVEBU PCIe driver taking over the control?
> 
> What do you think? I might be missing something here.
> 
> Thanks,
> Stefan