[RFC PATCH v3 3/3] rpi4: add a mapping for the PCIe XHCI controller MMIO registers (ARM 32bit)

Simon Glass sjg at chromium.org
Mon May 25 16:57:30 CEST 2020


Hi Marek,

On Mon, 25 May 2020 at 04:48, Marek Szyprowski <m.szyprowski at samsung.com> wrote:
>
> Hi Simon,
>
> On 22.05.2020 15:42, Simon Glass wrote:
> > On Thu, 21 May 2020 at 23:56, Marek Szyprowski <m.szyprowski at samsung.com> wrote:
> >> On 19.05.2020 18:47, Simon Glass wrote:
> >>> On Tue, 19 May 2020 at 06:00, Marek Szyprowski <m.szyprowski at samsung.com> wrote:
> >>>> On 19.05.2020 00:38, Simon Glass wrote:
> >>>>> On Mon, 18 May 2020 at 07:18, Marek Szyprowski <m.szyprowski at samsung.com> wrote:
> >>>>>> Create a non-cacheable mapping for the 0x600000000 physical memory region,
> >>>>>> where MMIO registers for the PCIe XHCI controller are instantiated by the
> >>>>>> PCIe bridge. Due to 32bit limit in the CPU virtual address space in ARM
> >>>>>> 32bit mode, this region is mapped at 0xff800000 CPU virtual address.
> >>>>>>
> >>>>>> Signed-off-by: Marek Szyprowski <m.szyprowski at samsung.com>
> >>>>>> ---
> >>>>>>     arch/arm/mach-bcm283x/Kconfig             |  1 +
> >>>>>>     arch/arm/mach-bcm283x/include/mach/base.h |  8 ++++++++
> >>>>>>     arch/arm/mach-bcm283x/init.c              | 20 ++++++++++++++++++++
> >>>>>>     include/configs/rpi.h                     |  7 +++++++
> >>>>>>     4 files changed, 36 insertions(+)
> >>>>>>
> >>>>>> diff --git a/arch/arm/mach-bcm283x/Kconfig b/arch/arm/mach-bcm283x/Kconfig
> >>>>>> index 00419bf..bcb7f1d 100644
> >>>>>> --- a/arch/arm/mach-bcm283x/Kconfig
> >>>>>> +++ b/arch/arm/mach-bcm283x/Kconfig
> >>>>>> @@ -36,6 +36,7 @@ config BCM2711_32B
> >>>>>>            select BCM2711
> >>>>>>            select ARMV7_LPAE
> >>>>>>            select CPU_V7A
> >>>>>> +       select PHYS_64BIT
> >>>>>>
> >>>>>>     config BCM2711_64B
> >>>>>>            bool "Broadcom BCM2711 SoC 64-bit support"
> >>>>>> diff --git a/arch/arm/mach-bcm283x/include/mach/base.h b/arch/arm/mach-bcm283x/include/mach/base.h
> >>>>>> index c4ae398..4ccaf69 100644
> >>>>>> --- a/arch/arm/mach-bcm283x/include/mach/base.h
> >>>>>> +++ b/arch/arm/mach-bcm283x/include/mach/base.h
> >>>>>> @@ -8,4 +8,12 @@
> >>>>>>
> >>>>>>     extern unsigned long rpi_bcm283x_base;
> >>>>>>
> >>>>>> +#ifdef CONFIG_ARMV7_LPAE
> >>>>>> +#ifdef CONFIG_TARGET_RPI_4_32B
> >>>>>> +#include <addr_map.h>
> >>>>>> +#define phys_to_virt addrmap_phys_to_virt
> >>>>>> +#define virt_to_phys addrmap_virt_to_phys
> >>>>>> +#endif
> >>>>>> +#endif
> >>>>>> +
> >>>>>>     #endif
> >>>>>> diff --git a/arch/arm/mach-bcm283x/init.c b/arch/arm/mach-bcm283x/init.c
> >>>>>> index 9f5bca3..008b312 100644
> >>>>>> --- a/arch/arm/mach-bcm283x/init.c
> >>>>>> +++ b/arch/arm/mach-bcm283x/init.c
> >>>>>> @@ -145,6 +145,26 @@ int mach_cpu_init(void)
> >>>>>>     }
> >>>>>>
> >>>>>>     #ifdef CONFIG_ARMV7_LPAE
> >>>>>> +#ifdef CONFIG_TARGET_RPI_4_32B
> >>>>>> +#define BCM2711_RPI4_PCIE_XHCI_MMIO_VIRT       0xff800000UL
> >>>>>> +#include <addr_map.h>
> >>>>>> +
> >>>>>> +void init_addr_map(void)
> >>>>>> +{
> >>>>>> +       mmu_set_region_dcache_behaviour_phys(BCM2711_RPI4_PCIE_XHCI_MMIO_VIRT,
> >>>>>> +                                            BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS,
> >>>>>> +                                            BCM2711_RPI4_PCIE_XHCI_MMIO_SIZE,
> >>>>>> +                                            DCACHE_OFF);
> >>>>>> +
> >>>>>> +       /* identity mapping for 0..BCM2711_RPI4_PCIE_XHCI_MMIO_VIRT */
> >>>>>> +       addrmap_set_entry(0, 0, BCM2711_RPI4_PCIE_XHCI_MMIO_VIRT, 0);
> >>>>>> +       /* XHCI MMIO on PCIe at BCM2711_RPI4_PCIE_XHCI_MMIO_VIRT */
> >>>>>> +       addrmap_set_entry(BCM2711_RPI4_PCIE_XHCI_MMIO_VIRT,
> >>>>>> +                         BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS,
> >>>>>> +                         BCM2711_RPI4_PCIE_XHCI_MMIO_SIZE, 1);
> >>>>>> +}
> >>>>>> +#endif
> >>>>>> +
> >>>>>>     void enable_caches(void)
> >>>>>>     {
> >>>>>>            dcache_enable();
> >>>>>> diff --git a/include/configs/rpi.h b/include/configs/rpi.h
> >>>>>> index b53a4b6..296e8ee 100644
> >>>>>> --- a/include/configs/rpi.h
> >>>>>> +++ b/include/configs/rpi.h
> >>>>>> @@ -63,6 +63,13 @@
> >>>>>>     #define CONFIG_SYS_BOOTM_LEN           SZ_64M
> >>>>>>     #endif
> >>>>>>
> >>>>>> +#ifdef CONFIG_ARMV7_LPAE
> >>>>>> +#ifdef CONFIG_TARGET_RPI_4_32B
> >>>>>> +#define CONFIG_ADDR_MAP 1
> >>>>>> +#define CONFIG_SYS_NUM_ADDR_MAP 2
> >>>>>> +#endif
> >>>>>> +#endif
> >>>>> We should be removing things from the config files. Can you move this
> >>>>> to devicetree or Kconfig?
> >>>> I can move them to Kconfig, no problem. However I would like to get some
> >>>> comments if the approach I presented in this patchset is fine.
> >>> Yes, no problem.
> >>>
> >>> I suspect we may need to expand the DMA drivers, perhaps, or some
> >>> other way to map memory on a per-device basis using driver model.
> >> I'm not sure that we really need such a complex solution. Usually the
> >> board (or even SoC), if ever, needs one or two such non-identity
> >> mappings, which can be easily created by the respective init code. The
> >> PowerPC case mentioned here looks a bit different, because it simply
> >> copies the mappings already configured in the CPU registers by the
> >> earlier firmware. Anyway, I don't think that we would ever need to
> >> manage physical/virtual mapping dynamically in the u-boot. All that
> >> cover current (and most future?) cases would be a simple function to map
> >> an arbitrary physical address at the predefined virtual one.
> > OK, well see how you go. When I see CONFIG options that specify values
> > I often think there should be a driver and device-tree node. We can
> > see how things go in the future as to whether we need a driver.
> In this case (CONFIG_ADDR_MAP) the values provided by the board's
> config.h (CONFIG_SYS_NUM_ADDR_MAP) only allows the code to use a simple
> static array instead of allocating things dynamically. It has no
> relation to the drivers nor device-tree.

We are working to remove the board config.h files and move everything
to Kconfig.

If there really is no need for a driver here, then we could perhaps
use a weak function for the interface? (Tom, note :-)

Regards,
Simon


More information about the U-Boot mailing list