[U-Boot] [PATCH] arm: ls1028a: define the integrated PCI bus (ECAM)

[Alex Marginean]

On 6/5/2019 4:09 AM, Bin Meng wrote:
> Hi Alex,
> 
> On Tue, Jun 4, 2019 at 9:59 PM Alex Marginean <alexm.osslist at gmail.com> wrote:
>>
>> Hi Bin,
>>
>> On 6/2/2019 5:22 PM, Bin Meng wrote:
>>> Hi Alex,
>>>
>>> On Fri, May 31, 2019 at 12:27 AM Alex Marginean <alexm.osslist at gmail.com> wrote:
>>>>
>>>> LS1028A includes an integrated PCI bus with 8M of ECAM space plus register
>>>> space for the integrated devices.  This integrated PCI bus is driven using
>>>> the generic ECAM driver.
>>>>
>>>> Signed-off-by: Alex Marginean <alexm.osslist at gmail.com>
>>>> ---
>>>>    arch/arm/dts/fsl-ls1028a.dtsi                          | 10 ++++++++++
>>>>    arch/arm/include/asm/arch-fsl-layerscape/cpu.h         |  2 ++
>>>>    arch/arm/include/asm/arch-fsl-layerscape/immap_lsch3.h |  2 ++
>>>>    configs/ls1028aqds_tfa_defconfig                       |  1 +
>>>>    configs/ls1028ardb_tfa_defconfig                       |  1 +
>>>>    5 files changed, 16 insertions(+)
>>>>
>>>> diff --git a/arch/arm/dts/fsl-ls1028a.dtsi b/arch/arm/dts/fsl-ls1028a.dtsi
>>>> index e6a443aa77..263c29af23 100644
>>>> --- a/arch/arm/dts/fsl-ls1028a.dtsi
>>>> +++ b/arch/arm/dts/fsl-ls1028a.dtsi
>>>> @@ -108,6 +108,16 @@
>>>>                          0x82000000 0x0 0x40000000 0x88 0x40000000 0x0 0x40000000>; /* non-prefetchable memory */
>>>>           };
>>>>
>>>> +       pcie at 1f0000000 {
>>>> +               compatible = "pci-host-ecam-generic";
>>>> +               reg = <0x01 0xf0000000 0x0 0x100000>;
>>>> +               #address-cells = <3>;
>>>> +               #size-cells = <2>;
>>>> +               device_type = "pci";
>>>> +               bus-range = <0x0 0x0>;
>>>
>>> I think this should be <0x0 0x7> since you mentioned in the commit
>>> message that only an 8M ECAM space is allocated.
>>
>> I think I was looking at the wrong spec, it seems the platform actually
>> has 256MB of address space reserved for ECAM.  It's not all implemented
>> of course, in fact all the devices we care about in u-boot are on bus 0.
>> Anyway, I can just remove bus-range, the default is 0-255 and that does
>> match the HW.
>>
>>>
>>>> +               ranges= <0x82000000 0x0 0x00000000 0x1 0xf8000000 0x0 0x160000>;
>>>> +       };
>>>> +
>>>>           i2c0: i2c at 2000000 {
>>>>                   compatible = "fsl,vf610-i2c";
>>>>                   #address-cells = <1>;
>>>> diff --git a/arch/arm/include/asm/arch-fsl-layerscape/cpu.h b/arch/arm/include/asm/arch-fsl-layerscape/cpu.h
>>>> index bdeb62576c..7759acdb8f 100644
>>>> --- a/arch/arm/include/asm/arch-fsl-layerscape/cpu.h
>>>> +++ b/arch/arm/include/asm/arch-fsl-layerscape/cpu.h
>>>> @@ -42,7 +42,9 @@
>>>>    #else
>>>>    #define CONFIG_SYS_PCIE1_PHYS_SIZE     0x800000000
>>>>    #define CONFIG_SYS_PCIE2_PHYS_SIZE     0x800000000
>>>> +#ifndef CONFIG_SYS_PCIE3_PHYS_SIZE
>>>>    #define CONFIG_SYS_PCIE3_PHYS_SIZE     0x800000000
>>>> +#endif
>>>>    #define CONFIG_SYS_PCIE4_PHYS_SIZE     0x800000000
>>>>    #define SYS_PCIE5_PHYS_SIZE            0x800000000
>>>>    #define SYS_PCIE6_PHYS_SIZE            0x800000000
>>>> diff --git a/arch/arm/include/asm/arch-fsl-layerscape/immap_lsch3.h b/arch/arm/include/asm/arch-fsl-layerscape/immap_lsch3.h
>>>> index 24c1b0e482..273157230f 100644
>>>> --- a/arch/arm/include/asm/arch-fsl-layerscape/immap_lsch3.h
>>>> +++ b/arch/arm/include/asm/arch-fsl-layerscape/immap_lsch3.h
>>>> @@ -186,6 +186,8 @@
>>>>    #elif CONFIG_ARCH_LS1028A
>>>>    #define CONFIG_SYS_PCIE1_PHYS_ADDR             0x8000000000ULL
>>>>    #define CONFIG_SYS_PCIE2_PHYS_ADDR             0x8800000000ULL
>>>> +#define CONFIG_SYS_PCIE3_PHYS_ADDR             0x01f0000000ULL
>>>> +#define CONFIG_SYS_PCIE3_PHYS_SIZE             0x0010000000ULL
>>>
>>> DT says the size is 0x100000. This does not match.
>>
>> I'll extend reg to 256MB along with removing bus-range.
>>
>>> These macros really look to me this platform is still using lots of
>>> non-DM approaches when it comes to driver support. These hard coded
>>> values should really be dropped and retrieved from DT instead via
>>> proper DM drivers.
>>
>> ECAM is driven by the ecam generic host driver and it doesn't care about
>> these macros.
>> These are used int arch/arm/cpu/armv8/fsl-layerscape/cpu.c to set up
>> the MMU at boot though, accessing ECAM doesn't work without them.
> 
> Yes, but what I was trying to say that with driver model things like
> register base and size should be retrieved from DT. Can we consider
> doing an ARMv8 DM MMU driver that reads DT and set up the page table
> for everything?

I've dug a bit more into the address ranges used here, there is indeed a
256MB space but that's not just for ECAM.  In fact 2nd half contains
device registers for the devices presented as PCI through ECAM.
Given than only bus 0 is populated I kept bus-range = <0x0 0x0>.

On the other hand both ECAM and device registers have to be mapped in
MMU, so I didn't change the 256MB size macro.

I agree the existing solution for mapping in MMU isn't great, since it
duplicates information between dts and code.  I'm not sure what would
be a good alternative.
Mapping based on dts could work, but the code would have to deal with
various corner cases.  Some nodes don't use reg for addresses, for
integrated PCI with EA mapping ECAM alone is not sufficient, we also
need the BAR equivalents mapped, those kind of things.  That means more
knowledge added one way or another to the arm MMU code, I'm not sure
that would end up being so much cleaner than what is there now.

Thanks!
Alex

> 
> Simon/Tom, do you have any better ideas?
> 
> Regards,
> Bin
>