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

[Bin Meng]

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?

Simon/Tom, do you have any better ideas?

Regards,
Bin