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

[Bin Meng]

Hi Alex,

On Fri, Jun 7, 2019 at 10:41 PM Alex Marginean <alexm.osslist at gmail.com> wrote:
>
> 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.

Yep, I believe this is not an NXP specific problem. It applies to
other ARM platforms, so I was hoping Tom or Simon could give some
generic solution.

Regards,
Bin