On Monday 29 November 2021 14:27:48 Pali Rohár wrote:
> 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.
Ok, it looks like that reset controller API allows to do this. It would
mean to define that "system-controller@18200" as reset controller,
exports from it for each PCIe port reset functionality and implements
assert and deassert functions which disable and enable port.
And because DTS for pci-mvebu.c driver is defined as multi-root-port,
"resets" property would need to be defined for each port separately.
Just I'm not sure if this "enable port functionality" should be
implemented via Reset Controller API...
> 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@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
