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

[Stefan Roese]

On 11/29/21 15:28, Pali Rohár wrote:
> 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 at 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.

Okay. Sounds like a plan to me.

> Just I'm not sure if this "enable port functionality" should be
> implemented via Reset Controller API...

How else should / could this be done then? Do you have alterative ideas?

Thanks,
Stefan

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