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

[Stefan Roese]

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?

Thanks,
Stefan