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

[Marek Szyprowski]

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.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland