On 05/05/2020 16:00, Matthias Brugger wrote:

On 04/05/2020 14:45, Sylwester Nawrocki wrote:

...

From: Marek Szyprowski m.szyprowski@samsung.com

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.

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com Reviewed-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de

---

Changes since v1:

• none.

---

arch/arm/mach-bcm283x/init.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-bcm283x/init.c b/arch/arm/mach-bcm283x/init.c index 4295356..6a748da 100644 --- a/arch/arm/mach-bcm283x/init.c +++ b/arch/arm/mach-bcm283x/init.c @@ -11,10 +11,15 @@ #include <dm/device.h> #include <fdt_support.h>

+#define BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS 0x600000000UL +#define BCM2711_RPI4_PCIE_XHCI_MMIO_SIZE 0x800000UL

#ifdef CONFIG_ARM64 #include <asm/armv8/mmu.h>

-static struct mm_region bcm283x_mem_map[] = { +#define MAX_MAP_MAX_ENTRIES (4)

What stands the second 'MAX' for?

...

+static struct mm_region bcm283x_mem_map[MAX_MAP_MAX_ENTRIES] = { { .virt = 0x00000000UL, .phys = 0x00000000UL, @@ -34,7 +39,7 @@ static struct mm_region bcm283x_mem_map[] = { } };

-static struct mm_region bcm2711_mem_map[] = { +static struct mm_region bcm2711_mem_map[MAX_MAP_MAX_ENTRIES] = { { .virt = 0x00000000UL, .phys = 0x00000000UL, @@ -49,6 +54,13 @@ static struct mm_region bcm2711_mem_map[] = { PTE_BLOCK_NON_SHARE | PTE_BLOCK_PXN | PTE_BLOCK_UXN }, {

I'd prefer a comment instead of using the BCM2711_RPI4_PCIE_XHCI_MMIO_* defines.

Ok, I see now you use the defines later on, forget my comment. It's ok as is.

Regards, Matthias

...

• .virt = BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS> +		.phys = BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS,
  

• .size = BCM2711_RPI4_PCIE_XHCI_MMIO_SIZE,
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE) |
  

• 	 PTE_BLOCK_NON_SHARE |
  

• 	 PTE_BLOCK_PXN | PTE_BLOCK_UXN
  

• }, { /* List terminator */ 0, }

@@ -71,7 +83,7 @@ static void _rpi_update_mem_map(struct mm_region *pd) { int i;

• for (i = 0; i < 2; i++) {

• for (i = 0; i < MAX_MAP_MAX_ENTRIES; i++) {

Variable mem_map points to bcm283x_mem_map which only holds two mm_region's (plus list terminator). So we have an overflow here. I think we should just define bcm2711_mem_map and bcm283x_mem_map with a fixed array size of 4 (see comment on the define naming above).

Regards, Matthias

...

mem_map[i].virt = pd[i].virt;
mem_map[i].phys = pd[i].phys;
mem_map[i].size = pd[i].size;

On 05/05/2020 16:10, Marek Szyprowski wrote:

Hi Matthias,

On 05.05.2020 16:00, Matthias Brugger wrote:

...

On 04/05/2020 14:45, Sylwester Nawrocki wrote:

...

From: Marek Szyprowski m.szyprowski@samsung.com

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.

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com Reviewed-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de

---

Changes since v1:

• none.

---

arch/arm/mach-bcm283x/init.c | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mach-bcm283x/init.c b/arch/arm/mach-bcm283x/init.c index 4295356..6a748da 100644 --- a/arch/arm/mach-bcm283x/init.c +++ b/arch/arm/mach-bcm283x/init.c @@ -11,10 +11,15 @@ #include <dm/device.h> #include <fdt_support.h>

+#define BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS 0x600000000UL +#define BCM2711_RPI4_PCIE_XHCI_MMIO_SIZE 0x800000UL

• #ifdef CONFIG_ARM64 #include <asm/armv8/mmu.h>

-static struct mm_region bcm283x_mem_map[] = { +#define MAX_MAP_MAX_ENTRIES (4)

What stands the second 'MAX' for?

a simple copy/paste issue. I will fix it.

...

...

+static struct mm_region bcm283x_mem_map[MAX_MAP_MAX_ENTRIES] = { { .virt = 0x00000000UL, .phys = 0x00000000UL, @@ -34,7 +39,7 @@ static struct mm_region bcm283x_mem_map[] = { } };

-static struct mm_region bcm2711_mem_map[] = { +static struct mm_region bcm2711_mem_map[MAX_MAP_MAX_ENTRIES] = { { .virt = 0x00000000UL, .phys = 0x00000000UL, @@ -49,6 +54,13 @@ static struct mm_region bcm2711_mem_map[] = { PTE_BLOCK_NON_SHARE | PTE_BLOCK_PXN | PTE_BLOCK_UXN }, {

I'd prefer a comment instead of using the BCM2711_RPI4_PCIE_XHCI_MMIO_* defines.

Those defines are also used in ARM 32bit code.

...

...

• .virt = BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS> +		.phys = BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS,
  

• .size = BCM2711_RPI4_PCIE_XHCI_MMIO_SIZE,
  

• .attrs = PTE_BLOCK_MEMTYPE(MT_DEVICE_NGNRNE) |
  

• 	 PTE_BLOCK_NON_SHARE |
  

• 	 PTE_BLOCK_PXN | PTE_BLOCK_UXN
  

• }, { /* List terminator */ 0, }

@@ -71,7 +83,7 @@ static void _rpi_update_mem_map(struct mm_region *pd) { int i;

• for (i = 0; i < 2; i++) {

• for (i = 0; i < MAX_MAP_MAX_ENTRIES; i++) {

Variable mem_map points to bcm283x_mem_map which only holds two mm_region's (plus list terminator). So we have an overflow here.

Nope, I've changed the bcm283x_mem_map to be large enough, see the above diff.

...

I think we should just define bcm2711_mem_map and bcm283x_mem_map with a fixed array size of 4 (see comment on the define naming above).

That's exactly what I did.

You are right, sorry for the noise!

Matthias

On 04/05/2020 14:45, Sylwester Nawrocki wrote:

From: Marek Szyprowski m.szyprowski@samsung.com

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@samsung.com Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com

---

Changes since v1:

• none.

---

arch/arm/mach-bcm283x/Kconfig | 1 + arch/arm/mach-bcm283x/include/mach/base.h | 7 +++++ arch/arm/mach-bcm283x/init.c | 52 +++++++++++++++++++++++++++++++ 3 files changed, 60 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..1d10dc9 100644 --- a/arch/arm/mach-bcm283x/include/mach/base.h +++ b/arch/arm/mach-bcm283x/include/mach/base.h @@ -6,6 +6,13 @@ #ifndef _BCM283x_BASE_H_ #define _BCM283x_BASE_H_

+#include <linux/types.h>

extern unsigned long rpi_bcm283x_base;

+#ifdef CONFIG_ARMV7_LPAE +extern void *rpi4_phys_to_virt(phys_addr_t paddr); +#define phys_to_virt(x) rpi4_phys_to_virt(x) +#endif

#endif diff --git a/arch/arm/mach-bcm283x/init.c b/arch/arm/mach-bcm283x/init.c index 6a748da..5d0d160 100644 --- a/arch/arm/mach-bcm283x/init.c +++ b/arch/arm/mach-bcm283x/init.c @@ -145,6 +145,58 @@ int mach_cpu_init(void) }

#ifdef CONFIG_ARMV7_LPAE

+#define BCM2711_RPI4_PCIE_XHCI_MMIO_VIRT 0xff800000UL

+void *rpi4_phys_to_virt(phys_addr_t paddr) +{

• if (paddr >= BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS)

• paddr = paddr - BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS +
  

• 	BCM2711_RPI4_PCIE_XHCI_MMIO_VIRT;
  

• return (void *)(unsigned long)paddr;

+}

+static void set_section_phys(unsigned int section, phys_addr_t phys,

• 	     enum dcache_option option)
  

+{

• u64 *page_table = (u64 *)gd->arch.tlb_addr;
• /* Need to set the access flag to not fault */
• u64 value = TTB_SECT_AP | TTB_SECT_AF;
• /* Add the page offset */
• value |= (phys);
• /* Add caching bits */
• value |= option;
• /* Set PTE */
• page_table[section] = value;

+}

+static void rpi4_create_pcie_xhci_mapping(void) +{

• unsigned sect = BCM2711_RPI4_PCIE_XHCI_MMIO_VIRT >> MMU_SECTION_SHIFT;
• phys_addr_t phys_addr = BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS;
• unsigned int size = BCM2711_RPI4_PCIE_XHCI_MMIO_SIZE;
• while (size) {

• set_section_phys(sect, phys_addr, DCACHE_OFF);
  

• sect++;
  

• phys_addr += MMU_SECTION_SIZE;
  

• size -= MMU_SECTION_SIZE;
  

• }

Why can't we reuse mmu_set_region_dcache_behaviour()?

+}

+void arm_init_domains(void) +{

• /*

• * Hijack this function to prepare a mappings for the PCIe MMIO
  

• * region for the XHCI controller on RPi4 board.
  

• * This code is called before enabling the MMU in ARM 32bit mode.
  

• */
  

• rpi4_create_pcie_xhci_mapping();

I think this indirection is not necessary.

Regards, Matthias

+}

void enable_caches(void) { dcache_enable();

Adding Tom as he is the arm maintainer.

On 04/05/2020 14:45, Sylwester Nawrocki wrote:

From: Marek Szyprowski m.szyprowski@samsung.com

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@samsung.com Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com

---

Changes since v1:

• none.

---

arch/arm/mach-bcm283x/Kconfig | 1 + arch/arm/mach-bcm283x/include/mach/base.h | 7 +++++ arch/arm/mach-bcm283x/init.c | 52 +++++++++++++++++++++++++++++++ 3 files changed, 60 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..1d10dc9 100644 --- a/arch/arm/mach-bcm283x/include/mach/base.h +++ b/arch/arm/mach-bcm283x/include/mach/base.h @@ -6,6 +6,13 @@ #ifndef _BCM283x_BASE_H_ #define _BCM283x_BASE_H_

+#include <linux/types.h>

extern unsigned long rpi_bcm283x_base;

+#ifdef CONFIG_ARMV7_LPAE +extern void *rpi4_phys_to_virt(phys_addr_t paddr); +#define phys_to_virt(x) rpi4_phys_to_virt(x) +#endif

#endif diff --git a/arch/arm/mach-bcm283x/init.c b/arch/arm/mach-bcm283x/init.c index 6a748da..5d0d160 100644 --- a/arch/arm/mach-bcm283x/init.c +++ b/arch/arm/mach-bcm283x/init.c @@ -145,6 +145,58 @@ int mach_cpu_init(void) }

#ifdef CONFIG_ARMV7_LPAE

+#define BCM2711_RPI4_PCIE_XHCI_MMIO_VIRT 0xff800000UL

+void *rpi4_phys_to_virt(phys_addr_t paddr) +{

• if (paddr >= BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS)

• paddr = paddr - BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS +
  

• 	BCM2711_RPI4_PCIE_XHCI_MMIO_VIRT;
  

• return (void *)(unsigned long)paddr;

I think for this cases we have addrmap_phys_to_virt() which up to now is only used by powerpc.

+}

+static void set_section_phys(unsigned int section, phys_addr_t phys,

• 	     enum dcache_option option)
  

+{

• u64 *page_table = (u64 *)gd->arch.tlb_addr;
• /* Need to set the access flag to not fault */
• u64 value = TTB_SECT_AP | TTB_SECT_AF;
• /* Add the page offset */
• value |= (phys);
• /* Add caching bits */
• value |= option;
• /* Set PTE */
• page_table[section] = value;

+}

+static void rpi4_create_pcie_xhci_mapping(void) +{

• unsigned sect = BCM2711_RPI4_PCIE_XHCI_MMIO_VIRT >> MMU_SECTION_SHIFT;
• phys_addr_t phys_addr = BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS;
• unsigned int size = BCM2711_RPI4_PCIE_XHCI_MMIO_SIZE;
• while (size) {

• set_section_phys(sect, phys_addr, DCACHE_OFF);
  

• sect++;
  

• phys_addr += MMU_SECTION_SIZE;
  

• size -= MMU_SECTION_SIZE;
  

• }

+}

I wonder if we can't do all this in the pcie driver probe function. Maybe we can create a new function like mmu_set_region_dcache_behaviour_phys which allows us to update a mapping that's not 1:1.

Tom what do you think?

Regards, Matthias

+void arm_init_domains(void) +{

• /*

• * Hijack this function to prepare a mappings for the PCIe MMIO
  

• * region for the XHCI controller on RPi4 board.
  

• * This code is called before enabling the MMU in ARM 32bit mode.
  

• */
  

• rpi4_create_pcie_xhci_mapping();

+}

void enable_caches(void) { dcache_enable();

On 11/05/2020 21:44, Tom Rini wrote:

On Fri, May 08, 2020 at 11:26:36PM +0200, Matthias Brugger wrote:

...

Adding Tom as he is the arm maintainer.

On 04/05/2020 14:45, Sylwester Nawrocki wrote:

...

From: Marek Szyprowski m.szyprowski@samsung.com

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@samsung.com Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com

---

Changes since v1:

• none.

---

arch/arm/mach-bcm283x/Kconfig | 1 + arch/arm/mach-bcm283x/include/mach/base.h | 7 +++++ arch/arm/mach-bcm283x/init.c | 52 +++++++++++++++++++++++++++++++ 3 files changed, 60 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..1d10dc9 100644 --- a/arch/arm/mach-bcm283x/include/mach/base.h +++ b/arch/arm/mach-bcm283x/include/mach/base.h @@ -6,6 +6,13 @@ #ifndef _BCM283x_BASE_H_ #define _BCM283x_BASE_H_

+#include <linux/types.h>

extern unsigned long rpi_bcm283x_base;

+#ifdef CONFIG_ARMV7_LPAE +extern void *rpi4_phys_to_virt(phys_addr_t paddr); +#define phys_to_virt(x) rpi4_phys_to_virt(x) +#endif

#endif diff --git a/arch/arm/mach-bcm283x/init.c b/arch/arm/mach-bcm283x/init.c index 6a748da..5d0d160 100644 --- a/arch/arm/mach-bcm283x/init.c +++ b/arch/arm/mach-bcm283x/init.c @@ -145,6 +145,58 @@ int mach_cpu_init(void) }

#ifdef CONFIG_ARMV7_LPAE

+#define BCM2711_RPI4_PCIE_XHCI_MMIO_VIRT 0xff800000UL

+void *rpi4_phys_to_virt(phys_addr_t paddr) +{

• if (paddr >= BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS)

• paddr = paddr - BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS +
  

• 	BCM2711_RPI4_PCIE_XHCI_MMIO_VIRT;
  

• return (void *)(unsigned long)paddr;

I think for this cases we have addrmap_phys_to_virt() which up to now is only used by powerpc.

...

+}

+static void set_section_phys(unsigned int section, phys_addr_t phys,

• 	     enum dcache_option option)
  

+{

• u64 *page_table = (u64 *)gd->arch.tlb_addr;
• /* Need to set the access flag to not fault */
• u64 value = TTB_SECT_AP | TTB_SECT_AF;
• /* Add the page offset */
• value |= (phys);
• /* Add caching bits */
• value |= option;
• /* Set PTE */
• page_table[section] = value;

+}

+static void rpi4_create_pcie_xhci_mapping(void) +{

• unsigned sect = BCM2711_RPI4_PCIE_XHCI_MMIO_VIRT >> MMU_SECTION_SHIFT;
• phys_addr_t phys_addr = BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS;
• unsigned int size = BCM2711_RPI4_PCIE_XHCI_MMIO_SIZE;
• while (size) {

• set_section_phys(sect, phys_addr, DCACHE_OFF);
  

• sect++;
  

• phys_addr += MMU_SECTION_SIZE;
  

• size -= MMU_SECTION_SIZE;
  

• }

+}

I wonder if we can't do all this in the pcie driver probe function. Maybe we can create a new function like mmu_set_region_dcache_behaviour_phys which allows us to update a mapping that's not 1:1.

Tom what do you think?

I think a harder look at how PowerPC handled this situation is in order, and then following / extending that path is in order.

Thanks Tom for the feedback. Sylwester, I propose to split the series in two. One for adding the driver to 64-bit U-Boot and another one to add support for rpi_4_32b_defconfig. This way we could get the driver merged for 2020.07 for sure, while 32-bit parts could take more cycles to be ready. What do you think?

Regards, Matthias

Hi Matthias,

On 08.05.2020 23:26, Matthias Brugger wrote:

Adding Tom as he is the arm maintainer.

On 04/05/2020 14:45, Sylwester Nawrocki wrote:

...

From: Marek Szyprowski m.szyprowski@samsung.com

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@samsung.com Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com

---

Changes since v1:

• none.

---

arch/arm/mach-bcm283x/Kconfig | 1 + arch/arm/mach-bcm283x/include/mach/base.h | 7 +++++ arch/arm/mach-bcm283x/init.c | 52 +++++++++++++++++++++++++++++++ 3 files changed, 60 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..1d10dc9 100644 --- a/arch/arm/mach-bcm283x/include/mach/base.h +++ b/arch/arm/mach-bcm283x/include/mach/base.h @@ -6,6 +6,13 @@ #ifndef _BCM283x_BASE_H_ #define _BCM283x_BASE_H_

+#include <linux/types.h>

• extern unsigned long rpi_bcm283x_base;

+#ifdef CONFIG_ARMV7_LPAE +extern void *rpi4_phys_to_virt(phys_addr_t paddr); +#define phys_to_virt(x) rpi4_phys_to_virt(x) +#endif

• #endif

diff --git a/arch/arm/mach-bcm283x/init.c b/arch/arm/mach-bcm283x/init.c index 6a748da..5d0d160 100644 --- a/arch/arm/mach-bcm283x/init.c +++ b/arch/arm/mach-bcm283x/init.c @@ -145,6 +145,58 @@ int mach_cpu_init(void) }

#ifdef CONFIG_ARMV7_LPAE

+#define BCM2711_RPI4_PCIE_XHCI_MMIO_VIRT 0xff800000UL

+void *rpi4_phys_to_virt(phys_addr_t paddr) +{

• if (paddr >= BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS)

• paddr = paddr - BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS +
  

• 	BCM2711_RPI4_PCIE_XHCI_MMIO_VIRT;
  

• return (void *)(unsigned long)paddr;

I think for this cases we have addrmap_phys_to_virt() which up to now is only used by powerpc.

...

+}

+static void set_section_phys(unsigned int section, phys_addr_t phys,

• 	     enum dcache_option option)
  

+{

• u64 *page_table = (u64 *)gd->arch.tlb_addr;
• /* Need to set the access flag to not fault */
• u64 value = TTB_SECT_AP | TTB_SECT_AF;
• /* Add the page offset */
• value |= (phys);
• /* Add caching bits */
• value |= option;
• /* Set PTE */
• page_table[section] = value;

+}

+static void rpi4_create_pcie_xhci_mapping(void) +{

• unsigned sect = BCM2711_RPI4_PCIE_XHCI_MMIO_VIRT >> MMU_SECTION_SHIFT;
• phys_addr_t phys_addr = BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS;
• unsigned int size = BCM2711_RPI4_PCIE_XHCI_MMIO_SIZE;
• while (size) {

• set_section_phys(sect, phys_addr, DCACHE_OFF);
  

• sect++;
  

• phys_addr += MMU_SECTION_SIZE;
  

• size -= MMU_SECTION_SIZE;
  

• }

+}

I wonder if we can't do all this in the pcie driver probe function. Maybe we can create a new function like mmu_set_region_dcache_behaviour_phys which allows us to update a mapping that's not 1:1.

Tom what do you think?

In theory this could be moved to pcie probe, but then how the pcie driver would know WHERE in the virtual address space it should create a mapping for the needed MMIO area? IMHO this is something SoC or board specific and creating such mapping in board init is justified.

I will adapt the code to use addr_map helpers.

Best regards

Provide a function for setting arbitrary virtual-physical MMU mapping for the given region.

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- arch/arm/include/asm/mmu.h | 8 ++++++++ arch/arm/include/asm/system.h | 18 ++++++++++++++++-- arch/arm/lib/cache-cp15.c | 18 ++++++++++++------ 3 files changed, 36 insertions(+), 8 deletions(-) create mode 100644 arch/arm/include/asm/mmu.h

diff --git a/arch/arm/include/asm/mmu.h b/arch/arm/include/asm/mmu.h new file mode 100644 index 0000000..fe3d793 --- /dev/null +++ b/arch/arm/include/asm/mmu.h @@ -0,0 +1,8 @@ +#ifndef __ASM_ARM_MMU_H +#define __ASM_ARM_MMU_H + +#ifdef CONFIG_ADDR_MAP +extern void init_addr_map(void); +#endif + +#endif diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h index 81ccead..a513f4a 100644 --- a/arch/arm/include/asm/system.h +++ b/arch/arm/include/asm/system.h @@ -573,14 +573,28 @@ s32 psci_features(u32 function_id, u32 psci_fid); void save_boot_params_ret(void);

/** + * Change the virt/phys mapping and cache settings for a region. + * + * \param virt virtual start address of memory region to change + * \param phys physical address for the memory region to set + * \param size size of memory region to change + * \param option dcache option to select + */ +void mmu_set_region_dcache_behaviour_phys(phys_addr_t virt, phys_addr_t phys, + size_t size, enum dcache_option option); + +/** * Change the cache settings for a region. * * \param start start address of memory region to change * \param size size of memory region to change * \param option dcache option to select */ -void mmu_set_region_dcache_behaviour(phys_addr_t start, size_t size, - enum dcache_option option); +static inline void mmu_set_region_dcache_behaviour(phys_addr_t start, + size_t size, enum dcache_option option) +{ + mmu_set_region_dcache_behaviour_phys(start, start, size, option); +}

#ifdef CONFIG_SYS_NONCACHED_MEMORY void noncached_init(void); diff --git a/arch/arm/lib/cache-cp15.c b/arch/arm/lib/cache-cp15.c index f8d2096..7c14d1d 100644 --- a/arch/arm/lib/cache-cp15.c +++ b/arch/arm/lib/cache-cp15.c @@ -24,7 +24,8 @@ __weak void arm_init_domains(void) { }

-void set_section_dcache(int section, enum dcache_option option) +static void set_section_phys(int section, phys_addr_t phys, + enum dcache_option option) { #ifdef CONFIG_ARMV7_LPAE u64 *page_table = (u64 *)gd->arch.tlb_addr; @@ -36,7 +37,7 @@ void set_section_dcache(int section, enum dcache_option option) #endif

/* Add the page offset */ - value |= ((u32)section << MMU_SECTION_SHIFT); + value |= phys;

/* Add caching bits */ value |= option; @@ -45,13 +46,18 @@ void set_section_dcache(int section, enum dcache_option option) page_table[section] = value; }

+void set_section_dcache(int section, enum dcache_option option) +{ + set_section_phys(section, (u32)section << MMU_SECTION_SHIFT, option); +} + __weak void mmu_page_table_flush(unsigned long start, unsigned long stop) { debug("%s: Warning: not implemented\n", __func__); }

-void mmu_set_region_dcache_behaviour(phys_addr_t start, size_t size, - enum dcache_option option) +void mmu_set_region_dcache_behaviour_phys(phys_addr_t start, phys_addr_t phys, + size_t size, enum dcache_option option) { #ifdef CONFIG_ARMV7_LPAE u64 *page_table = (u64 *)gd->arch.tlb_addr; @@ -70,8 +76,8 @@ void mmu_set_region_dcache_behaviour(phys_addr_t start, size_t size, debug("%s: start=%pa, size=%zu, option=0x%x\n", __func__, &start, size, option); #endif - for (upto = start; upto < end; upto++) - set_section_dcache(upto, option); + for (upto = start; upto < end; upto++, phys += MMU_SECTION_SIZE) + set_section_phys(upto, phys, option);

/* * Make sure range is cache line aligned

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@samsung.com --- arch/arm/mach-bcm283x/Kconfig | 1 + arch/arm/mach-bcm283x/include/mach/base.h | 6 ++++++ arch/arm/mach-bcm283x/init.c | 14 ++++++++++++++ include/configs/rpi.h | 5 +++++ 4 files changed, 26 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..1bf89db 100644 --- a/arch/arm/mach-bcm283x/include/mach/base.h +++ b/arch/arm/mach-bcm283x/include/mach/base.h @@ -8,4 +8,10 @@

extern unsigned long rpi_bcm283x_base;

+#ifdef CONFIG_ARMV7_LPAE +#include <addr_map.h> +#define phys_to_virt addrmap_phys_to_virt +#define virt_to_phys addrmap_virt_to_phys +#endif + #endif diff --git a/arch/arm/mach-bcm283x/init.c b/arch/arm/mach-bcm283x/init.c index 6a748da..4b9c831 100644 --- a/arch/arm/mach-bcm283x/init.c +++ b/arch/arm/mach-bcm283x/init.c @@ -145,6 +145,20 @@ int mach_cpu_init(void) }

#ifdef CONFIG_ARMV7_LPAE +#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); + addrmap_set_entry(BCM2711_RPI4_PCIE_XHCI_MMIO_VIRT, + BCM2711_RPI4_PCIE_XHCI_MMIO_PHYS, + BCM2711_RPI4_PCIE_XHCI_MMIO_SIZE, 0); +} + void enable_caches(void) { dcache_enable(); diff --git a/include/configs/rpi.h b/include/configs/rpi.h index b53a4b6..7da2cff 100644 --- a/include/configs/rpi.h +++ b/include/configs/rpi.h @@ -63,6 +63,11 @@ #define CONFIG_SYS_BOOTM_LEN SZ_64M #endif

+#ifdef CONFIG_ARMV7_LPAE +#define CONFIG_ADDR_MAP 1 +#define CONFIG_SYS_NUM_ADDR_MAP 2 +#endif + /* Devices */ /* GPIO */ #define CONFIG_BCM2835_GPIO

Provide a function for setting arbitrary virtual-physical MMU mapping for the given region.

Signed-off-by: Marek Szyprowski m.szyprowski@samsung.com --- arch/arm/include/asm/mmu.h | 8 ++++++++ arch/arm/include/asm/system.h | 11 +++++++++++ arch/arm/lib/cache-cp15.c | 24 ++++++++++++++++++------ 3 files changed, 37 insertions(+), 6 deletions(-) create mode 100644 arch/arm/include/asm/mmu.h

diff --git a/arch/arm/include/asm/mmu.h b/arch/arm/include/asm/mmu.h new file mode 100644 index 0000000..fe3d793 --- /dev/null +++ b/arch/arm/include/asm/mmu.h @@ -0,0 +1,8 @@ +#ifndef __ASM_ARM_MMU_H +#define __ASM_ARM_MMU_H + +#ifdef CONFIG_ADDR_MAP +extern void init_addr_map(void); +#endif + +#endif diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h index 81ccead..5b9f31c 100644 --- a/arch/arm/include/asm/system.h +++ b/arch/arm/include/asm/system.h @@ -573,6 +573,17 @@ s32 psci_features(u32 function_id, u32 psci_fid); void save_boot_params_ret(void);

/** + * Change the virt/phys mapping and cache settings for a region. + * + * \param virt virtual start address of memory region to change + * \param phys physical address for the memory region to set + * \param size size of memory region to change + * \param option dcache option to select + */ +void mmu_set_region_dcache_behaviour_phys(phys_addr_t virt, phys_addr_t phys, + size_t size, enum dcache_option option); + +/** * Change the cache settings for a region. * * \param start start address of memory region to change diff --git a/arch/arm/lib/cache-cp15.c b/arch/arm/lib/cache-cp15.c index f8d2096..84ddad3 100644 --- a/arch/arm/lib/cache-cp15.c +++ b/arch/arm/lib/cache-cp15.c @@ -24,7 +24,8 @@ __weak void arm_init_domains(void) { }

-void set_section_dcache(int section, enum dcache_option option) +static void set_section_phys(int section, phys_addr_t phys, + enum dcache_option option) { #ifdef CONFIG_ARMV7_LPAE u64 *page_table = (u64 *)gd->arch.tlb_addr; @@ -36,7 +37,7 @@ void set_section_dcache(int section, enum dcache_option option) #endif

/* Add the page offset */ - value |= ((u32)section << MMU_SECTION_SHIFT); + value |= phys;

/* Add caching bits */ value |= option; @@ -45,13 +46,18 @@ void set_section_dcache(int section, enum dcache_option option) page_table[section] = value; }

+void set_section_dcache(int section, enum dcache_option option) +{ + set_section_phys(section, (u32)section << MMU_SECTION_SHIFT, option); +} + __weak void mmu_page_table_flush(unsigned long start, unsigned long stop) { debug("%s: Warning: not implemented\n", __func__); }

-void mmu_set_region_dcache_behaviour(phys_addr_t start, size_t size, - enum dcache_option option) +void mmu_set_region_dcache_behaviour_phys(phys_addr_t start, phys_addr_t phys, + size_t size, enum dcache_option option) { #ifdef CONFIG_ARMV7_LPAE u64 *page_table = (u64 *)gd->arch.tlb_addr; @@ -70,8 +76,8 @@ void mmu_set_region_dcache_behaviour(phys_addr_t start, size_t size, debug("%s: start=%pa, size=%zu, option=0x%x\n", __func__, &start, size, option); #endif - for (upto = start; upto < end; upto++) - set_section_dcache(upto, option); + for (upto = start; upto < end; upto++, phys += MMU_SECTION_SIZE) + set_section_phys(upto, phys, option);

/* * Make sure range is cache line aligned @@ -86,6 +92,12 @@ void mmu_set_region_dcache_behaviour(phys_addr_t start, size_t size, mmu_page_table_flush(startpt, stoppt); }

+void mmu_set_region_dcache_behaviour(phys_addr_t start, size_t size, + enum dcache_option option) +{ + mmu_set_region_dcache_behaviour_phys(start, start, size, option); +} + __weak void dram_bank_mmu_setup(int bank) { bd_t *bd = gd->bd;

On Mon, 2020-05-04 at 14:45 +0200, Sylwester Nawrocki wrote:

This patch adds basic driver for the Broadcom STB PCIe host controller. The code is based on Linux upstream driver (pcie-brcmstb.c) with MSI handling removed. The inbound access memory region is not currently parsed from dma-ranges DT property and a fixed 4GB region is used.

The patch has been tested on RPI4 board, i.e. on BCM2711 SoC with VL805 USB Host Controller.

Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com

I don't know if it's a little redundant already having a Signed-off-by tag, but if relevant you can add my:

Reviewed-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de

Regards, Nicolas

---

Changes since v1:

• fixed argument in brcm_pcie_set_ssc() function call
• changed rc_bar2_size assignment to value 0xC0000000, as in upstream devicetre

Changes since RFC:

• reworked to align with current Linux mainline version and u-boot driver by Nicolas Saenz Julienne

brcmstb pcie

drivers/pci/Kconfig | 6 + drivers/pci/Makefile | 1 + drivers/pci/pcie_brcmstb.c | 594 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 601 insertions(+) create mode 100644 drivers/pci/pcie_brcmstb.c

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig index 437cd9a..056a021 100644 --- a/drivers/pci/Kconfig +++ b/drivers/pci/Kconfig @@ -197,4 +197,10 @@ config PCIE_MEDIATEK Say Y here if you want to enable Gen2 PCIe controller, which could be found on MT7623 SoC family.

+config PCI_BRCMSTB

• bool "Broadcom STB PCIe controller"
• depends on DM_PCI
• depends on ARCH_BCM283X
• help

•  Say Y here if you want to enable Broadcom STB PCIe controller support.
  

endif diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile index c051ecc..3e53b1f 100644 --- a/drivers/pci/Makefile +++ b/drivers/pci/Makefile @@ -43,3 +43,4 @@ obj-$(CONFIG_PCI_PHYTIUM) += pcie_phytium.o obj-$(CONFIG_PCIE_INTEL_FPGA) += pcie_intel_fpga.o obj-$(CONFIG_PCI_KEYSTONE) += pcie_dw_ti.o obj-$(CONFIG_PCIE_MEDIATEK) += pcie_mediatek.o +obj-$(CONFIG_PCI_BRCMSTB) += pcie_brcmstb.o diff --git a/drivers/pci/pcie_brcmstb.c b/drivers/pci/pcie_brcmstb.c new file mode 100644 index 0000000..c6ddf92 --- /dev/null +++ b/drivers/pci/pcie_brcmstb.c @@ -0,0 +1,594 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Broadcom STB PCIe controller driver
• • Copyright (C) 2020 Samsung Electronics Co., Ltd.
• • Based on upstream Linux kernel driver:
• • drivers/pci/controller/pcie-brcmstb.c
• • Copyright (C) 2009 - 2017 Broadcom
• • Based driver by Nicolas Saenz Julienne
• • Copyright (C) 2020 Nicolas Saenz Julienne nsaenzjulienne@suse.de
• */

+#include <asm/io.h> +#include <common.h> +#include <dm.h> +#include <dm/ofnode.h> +#include <errno.h> +#include <linux/bitfield.h> +#include <linux/log2.h> +#include <pci.h>

+/* BRCM_PCIE_CAP_REGS - Offset for the mandatory capability config regs */ +#define BRCM_PCIE_CAP_REGS 0x00ac

+/* Broadcom STB PCIe Register Offsets */ +#define PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1 0x0188 +#define PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1_ENDIAN_MODE_BAR2_MASK 0xc +#define PCIE_RC_CFG_VENDOR_SPCIFIC_REG1_LITTLE_ENDIAN 0x0

+#define PCIE_RC_CFG_PRIV1_ID_VAL3 0x043c +#define PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK 0xffffff

+#define PCIE_RC_DL_MDIO_ADDR 0x1100 +#define PCIE_RC_DL_MDIO_WR_DATA 0x1104 +#define PCIE_RC_DL_MDIO_RD_DATA 0x1108

+#define PCIE_MISC_MISC_CTRL 0x4008 +#define PCIE_MISC_MISC_CTRL_SCB_ACCESS_EN_MASK 0x1000 +#define PCIE_MISC_MISC_CTRL_CFG_READ_UR_MODE_MASK 0x2000 +#define PCIE_MISC_MISC_CTRL_MAX_BURST_SIZE_MASK 0x300000 +#define PCIE_MISC_MISC_CTRL_MAX_BURST_SIZE_128 0x0 +#define PCIE_MISC_MISC_CTRL_SCB0_SIZE_MASK 0xf8000000

+#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LO 0x400c +#define PCIE_MEM_WIN0_LO(win) \

• PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LO + ((win) * 4)
  

+#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI 0x4010 +#define PCIE_MEM_WIN0_HI(win) \

• PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI + ((win) * 4)
  

+#define PCIE_MISC_RC_BAR1_CONFIG_LO 0x402c +#define PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK 0x1f

+#define PCIE_MISC_RC_BAR2_CONFIG_LO 0x4034 +#define PCIE_MISC_RC_BAR2_CONFIG_LO_SIZE_MASK 0x1f +#define PCIE_MISC_RC_BAR2_CONFIG_HI 0x4038

+#define PCIE_MISC_RC_BAR3_CONFIG_LO 0x403c +#define PCIE_MISC_RC_BAR3_CONFIG_LO_SIZE_MASK 0x1f

+#define PCIE_MISC_PCIE_STATUS 0x4068 +#define PCIE_MISC_PCIE_STATUS_PCIE_PORT_MASK 0x80 +#define PCIE_MISC_PCIE_STATUS_PCIE_DL_ACTIVE_MASK 0x20 +#define PCIE_MISC_PCIE_STATUS_PCIE_PHYLINKUP_MASK 0x10 +#define PCIE_MISC_PCIE_STATUS_PCIE_LINK_IN_L23_MASK 0x40

+#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT 0x4070 +#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT_LIMIT_MASK 0xfff00000 +#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT_BASE_MASK 0xfff0 +#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT_BASE_HI_SHIFT 12 +#define PCIE_MEM_WIN0_BASE_LIMIT(win) \

• PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT + ((win) * 4)
  

+#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_HI 0x4080 +#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_HI_BASE_MASK 0xff +#define PCIE_MEM_WIN0_BASE_HI(win) \

• PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_HI + ((win) * 8)
  

+#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LIMIT_HI 0x4084 +#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LIMIT_HI_LIMIT_MASK 0xff +#define PCIE_MEM_WIN0_LIMIT_HI(win) \

• PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LIMIT_HI + ((win) * 8)
  

+#define PCIE_MISC_HARD_PCIE_HARD_DEBUG 0x4204 +#define PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK 0x2 +#define PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK 0x08000000

+#define PCIE_MSI_INTR2_CLR 0x4508 +#define PCIE_MSI_INTR2_MASK_SET 0x4510

+#define PCIE_EXT_CFG_DATA 0x8000

+#define PCIE_EXT_CFG_INDEX 0x9000 +#define PCIE_EXT_BUSNUM_SHIFT 20 +#define PCIE_EXT_SLOT_SHIFT 15 +#define PCIE_EXT_FUNC_SHIFT 12

+#define PCIE_RGR1_SW_INIT_1 0x9210 +#define PCIE_RGR1_SW_INIT_1_PERST_MASK 0x1 +#define PCIE_RGR1_SW_INIT_1_INIT_MASK 0x2

+/* PCIe parameters */ +#define BRCM_NUM_PCIE_OUT_WINS 0x4

+/* MDIO registers */ +#define MDIO_PORT0 0x0 +#define MDIO_DATA_MASK 0x7fffffff +#define MDIO_PORT_MASK 0xf0000 +#define MDIO_REGAD_MASK 0xffff +#define MDIO_CMD_MASK 0xfff00000 +#define MDIO_CMD_READ 0x1 +#define MDIO_CMD_WRITE 0x0 +#define MDIO_DATA_DONE_MASK 0x80000000 +#define MDIO_RD_DONE(x) (((x) & MDIO_DATA_DONE_MASK) ? 1 : 0) +#define MDIO_WT_DONE(x) (((x) & MDIO_DATA_DONE_MASK) ? 0 : 1) +#define SSC_REGS_ADDR 0x1100 +#define SET_ADDR_OFFSET 0x1f +#define SSC_CNTL_OFFSET 0x2 +#define SSC_CNTL_OVRD_EN_MASK 0x8000 +#define SSC_CNTL_OVRD_VAL_MASK 0x4000 +#define SSC_STATUS_OFFSET 0x1 +#define SSC_STATUS_SSC_MASK 0x400 +#define SSC_STATUS_PLL_LOCK_MASK 0x800

+struct brcm_pcie {

• void __iomem *base;
• int gen;
• bool ssc;

+};

+#define msleep(a) udelay((a) * 1000)

+/*

• • This is to convert the size of the inbound "BAR" region to the
• • non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE
• */

+static int brcm_pcie_encode_ibar_size(u64 size) +{

• int log2_in = ilog2(size);
• if (log2_in >= 12 && log2_in <= 15)

• /* Covers 4KB to 32KB (inclusive) */
  

• return (log2_in - 12) + 0x1c;
  

• else if (log2_in >= 16 && log2_in <= 37)

• /* Covers 64KB to 32GB, (inclusive) */
  

• return log2_in - 15;
  

• /* Something is awry so disable */
• return 0;

+}

+/* Configuration space read/write support */ +static inline int brcm_pcie_cfg_index(pci_dev_t bdf, int reg) +{

• return (PCI_DEV(bdf) << PCIE_EXT_SLOT_SHIFT)

• | (PCI_FUNC(bdf) << PCIE_EXT_FUNC_SHIFT)
  

• | (PCI_BUS(bdf) << PCIE_EXT_BUSNUM_SHIFT)
  

• | (reg & ~3);
  

+}

+/* The controller is capable of serving in both RC and EP roles */ +static bool brcm_pcie_rc_mode(struct brcm_pcie *pcie) +{

• u32 val = readl(pcie->base + PCIE_MISC_PCIE_STATUS);
• return !!FIELD_GET(PCIE_MISC_PCIE_STATUS_PCIE_PORT_MASK, val);

+}

+static bool brcm_pcie_link_up(struct brcm_pcie *pcie) +{

• u32 val = readl(pcie->base + PCIE_MISC_PCIE_STATUS);
• u32 dla = FIELD_GET(PCIE_MISC_PCIE_STATUS_PCIE_DL_ACTIVE_MASK, val);
• u32 plu = FIELD_GET(PCIE_MISC_PCIE_STATUS_PCIE_PHYLINKUP_MASK, val);
• return dla && plu;

+}

+static int brcm_pcie_config_address(const struct udevice *udev, pci_dev_t bdf,

• 		    uint offset, void **paddress)
  

+{

• struct brcm_pcie *pcie = dev_get_priv(udev);
• unsigned int bus = PCI_BUS(bdf);
• unsigned int dev = PCI_DEV(bdf);
• int idx;
• /*

• * Busses 0 (host PCIe bridge) and 1 (its immediate child)
  

• * are limited to a single device each
  

• */
  

• if ((bus == (udev->seq + 1)) && dev > 0)

• return -ENODEV;
  

• /* Accesses to the RC go right to the RC registers if PCI device == 0 */
• if (bus == udev->seq) {

• if (PCI_DEV(bdf))
  

• 	return -ENODEV;
  

• *paddress = pcie->base + offset;
  

• return 0;
  

• }
• /* For devices, write to the config space index register */
• idx = brcm_pcie_cfg_index(bdf, 0);
• writel(idx, pcie->base + PCIE_EXT_CFG_INDEX);
• *paddress = pcie->base + PCIE_EXT_CFG_DATA + offset;
• return 0;

+}

+static int brcm_pcie_read_config(const struct udevice *bus, pci_dev_t bdf,

• 		 uint offset, ulong *valuep,
  

• 		 enum pci_size_t size)
  

+{

• return pci_generic_mmap_read_config(bus, brcm_pcie_config_address,

• 			    bdf, offset, valuep, size);
  

+}

+static int brcm_pcie_write_config(struct udevice *bus, pci_dev_t bdf,

• 		  uint offset, ulong value,
  

• 		  enum pci_size_t size)
  

+{

• return pci_generic_mmap_write_config(bus, brcm_pcie_config_address,

• 			     bdf, offset, value, size);
  

+}

+static const char *link_speed_to_str(unsigned int s) +{

• static const char * const speed_str[] = { "??", "2.5", "5.0", "8.0" };
• if (s >= ARRAY_SIZE(speed_str))

• s = 0;
  

• return speed_str[s];

+}

+static inline void brcm_pcie_bridge_sw_init_set(struct brcm_pcie *pcie, u32 val) +{

• u32 tmp;
• tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1);
• u32p_replace_bits(&tmp, val, PCIE_RGR1_SW_INIT_1_INIT_MASK);
• writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1);

+}

+static inline void brcm_pcie_perst_set(struct brcm_pcie *pcie, u32 val) +{

• u32 tmp;
• tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1);
• u32p_replace_bits(&tmp, val, PCIE_RGR1_SW_INIT_1_PERST_MASK);
• writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1);

+}

+static u32 brcm_pcie_mdio_form_pkt(int port, int regad, int cmd) +{

• u32 pkt = 0;
• pkt |= FIELD_PREP(MDIO_PORT_MASK, port);
• pkt |= FIELD_PREP(MDIO_REGAD_MASK, regad);
• pkt |= FIELD_PREP(MDIO_CMD_MASK, cmd);
• return pkt;

+}

+/* Negative return value indicates error */ +static int brcm_pcie_mdio_read(void __iomem *base, u8 port, u8 regad, u32 *val) +{

• int tries;
• u32 data;
• writel(brcm_pcie_mdio_form_pkt(port, regad, MDIO_CMD_READ),

•       base + PCIE_RC_DL_MDIO_ADDR);
  

• readl(base + PCIE_RC_DL_MDIO_ADDR);
• data = readl(base + PCIE_RC_DL_MDIO_RD_DATA);
• for (tries = 0; !MDIO_RD_DONE(data) && tries < 10; tries++) {

• udelay(10);
  

• data = readl(base + PCIE_RC_DL_MDIO_RD_DATA);
  

• }
• *val = FIELD_GET(MDIO_DATA_MASK, data);
• return MDIO_RD_DONE(data) ? 0 : -EIO;

+}

+/* Negative return value indicates error */ +static int brcm_pcie_mdio_write(void __iomem *base, u8 port,

• 		u8 regad, u16 wrdata)
  

+{

• int tries;
• u32 data;
• writel(brcm_pcie_mdio_form_pkt(port, regad, MDIO_CMD_WRITE),

•       base + PCIE_RC_DL_MDIO_ADDR);
  

• readl(base + PCIE_RC_DL_MDIO_ADDR);
• writel(MDIO_DATA_DONE_MASK | wrdata, base + PCIE_RC_DL_MDIO_WR_DATA);
• data = readl(base + PCIE_RC_DL_MDIO_WR_DATA);
• for (tries = 0; !MDIO_WT_DONE(data) && tries < 10; tries++) {

• udelay(10);
  

• data = readl(base + PCIE_RC_DL_MDIO_WR_DATA);
  

• }
• return MDIO_WT_DONE(data) ? 0 : -EIO;

+}

+/*

• • Configures device for Spread Spectrum Clocking (SSC) mode; negative
• • return value indicates error.
• */

+static int brcm_pcie_set_ssc(struct brcm_pcie *pcie) +{

• void __iomem *base = pcie->base;
• int pll, ssc;
• int ret;
• u32 tmp;
• ret = brcm_pcie_mdio_write(base, MDIO_PORT0, SET_ADDR_OFFSET,

• 		   SSC_REGS_ADDR);
  

• if (ret < 0)

• return ret;
  

• ret = brcm_pcie_mdio_read(base, MDIO_PORT0, SSC_CNTL_OFFSET, &tmp);
• if (ret < 0)

• return ret;
  

• u32p_replace_bits(&tmp, 1, SSC_CNTL_OVRD_EN_MASK);
• u32p_replace_bits(&tmp, 1, SSC_CNTL_OVRD_VAL_MASK);
• ret = brcm_pcie_mdio_write(base, MDIO_PORT0, SSC_CNTL_OFFSET, tmp);
• if (ret < 0)

• return ret;
  

• udelay(1000);
• ret = brcm_pcie_mdio_read(base, MDIO_PORT0, SSC_STATUS_OFFSET, &tmp);
• if (ret < 0)

• return ret;
  

• ssc = FIELD_GET(SSC_STATUS_SSC_MASK, tmp);
• pll = FIELD_GET(SSC_STATUS_PLL_LOCK_MASK, tmp);
• return ssc && pll ? 0 : -EIO;

+}

+/* Limits operation to a specific generation (1, 2, or 3) */ +static void brcm_pcie_set_gen(struct brcm_pcie *pcie, int gen) +{

• void __iomem *base = pcie->base;
• u16 lnkctl2 = readw(base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCTL2);
• u32 lnkcap = readl(base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCAP);
• lnkcap = (lnkcap & ~PCI_EXP_LNKCAP_SLS) | gen;
• writel(lnkcap, base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCAP);
• lnkctl2 = (lnkctl2 & ~0xf) | gen;
• writew(lnkctl2, base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCTL2);

+}

+static void brcm_pcie_set_outbound_win(struct brcm_pcie *pcie,

• 		       unsigned int win, u64 phys_addr,
  

• 		       u64 pcie_addr, u64 size)
  

+{

• void __iomem *base = pcie->base;
• u32 phys_addr_mb_high, limit_addr_mb_high;
• phys_addr_t phys_addr_mb, limit_addr_mb;
• int high_addr_shift;
• u32 tmp;
• /* Set the base of the pcie_addr window */
• writel(lower_32_bits(pcie_addr), base + PCIE_MEM_WIN0_LO(win));
• writel(upper_32_bits(pcie_addr), base + PCIE_MEM_WIN0_HI(win));
• /* Write the addr base & limit lower bits (in MBs) */
• phys_addr_mb = phys_addr / SZ_1M;
• limit_addr_mb = (phys_addr + size - 1) / SZ_1M;
• tmp = readl(base + PCIE_MEM_WIN0_BASE_LIMIT(win));
• u32p_replace_bits(&tmp, phys_addr_mb,

• 	  PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT_BASE_MASK);
  

• u32p_replace_bits(&tmp, limit_addr_mb,

• 	  PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT_LIMIT_MASK);
  

• writel(tmp, base + PCIE_MEM_WIN0_BASE_LIMIT(win));
• /* Write the cpu & limit addr upper bits */
• high_addr_shift =

PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT_BASE_HI_SHIFT;

• phys_addr_mb_high = phys_addr_mb >> high_addr_shift;
• tmp = readl(base + PCIE_MEM_WIN0_BASE_HI(win));
• u32p_replace_bits(&tmp, phys_addr_mb_high,

• 	  PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_HI_BASE_MASK);
  

• writel(tmp, base + PCIE_MEM_WIN0_BASE_HI(win));
• limit_addr_mb_high = limit_addr_mb >> high_addr_shift;
• tmp = readl(base + PCIE_MEM_WIN0_LIMIT_HI(win));
• u32p_replace_bits(&tmp, limit_addr_mb_high,

• 	  PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LIMIT_HI_LIMIT_MASK);
  

• writel(tmp, base + PCIE_MEM_WIN0_LIMIT_HI(win));

+}

+static int brcm_pcie_probe(struct udevice *dev) +{

• struct udevice *ctlr = pci_get_controller(dev);
• struct pci_controller *hose = dev_get_uclass_priv(ctlr);
• struct brcm_pcie *pcie = dev_get_priv(dev);
• void __iomem *base = pcie->base;
• bool ssc_good = false;
• int num_out_wins = 0;
• u64 rc_bar2_offset, rc_bar2_size;
• unsigned int scb_size_val;
• int i, ret;
• u16 nlw, cls, lnksta;
• u32 tmp;
• /* Reset the bridge */
• brcm_pcie_bridge_sw_init_set(pcie, 1);
• udelay(150);
• /* Take the bridge out of reset */
• brcm_pcie_bridge_sw_init_set(pcie, 0);
• tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
• tmp &= ~PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK;
• writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
• /* Wait for SerDes to be stable */
• udelay(150);
• /* Set SCB_MAX_BURST_SIZE, CFG_READ_UR_MODE, SCB_ACCESS_EN */
• u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_SCB_ACCESS_EN_MASK);
• u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_CFG_READ_UR_MODE_MASK);
• u32p_replace_bits(&tmp, PCIE_MISC_MISC_CTRL_MAX_BURST_SIZE_128,

• 	  PCIE_MISC_MISC_CTRL_MAX_BURST_SIZE_MASK);
  

• writel(tmp, base + PCIE_MISC_MISC_CTRL);
• /*

• * TODO: When support for other SoCs than BCM2711 is added we may
  

• * need to use the base address and size(s) provided in the dma-ranges
  

• * property.
  

• */
  

• rc_bar2_offset = 0;
• rc_bar2_size = 0xc0000000;
• tmp = lower_32_bits(rc_bar2_offset);
• u32p_replace_bits(&tmp, brcm_pcie_encode_ibar_size(rc_bar2_size),

• 	  PCIE_MISC_RC_BAR2_CONFIG_LO_SIZE_MASK);
  

• writel(tmp, base + PCIE_MISC_RC_BAR2_CONFIG_LO);
• writel(upper_32_bits(rc_bar2_offset),

•       base + PCIE_MISC_RC_BAR2_CONFIG_HI);
  

• scb_size_val = rc_bar2_size ?

•        ilog2(rc_bar2_size) - 15 : 0xf; /* 0xf is 1GB */
  

• tmp = readl(base + PCIE_MISC_MISC_CTRL);
• u32p_replace_bits(&tmp, scb_size_val,

• 	  PCIE_MISC_MISC_CTRL_SCB0_SIZE_MASK);
  

• writel(tmp, base + PCIE_MISC_MISC_CTRL);
• /* Disable the PCIe->GISB memory window (RC_BAR1) */
• tmp = readl(base + PCIE_MISC_RC_BAR1_CONFIG_LO);
• tmp &= ~PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK;
• writel(tmp, base + PCIE_MISC_RC_BAR1_CONFIG_LO);
• /* Disable the PCIe->SCB memory window (RC_BAR3) */
• tmp = readl(base + PCIE_MISC_RC_BAR3_CONFIG_LO);
• tmp &= ~PCIE_MISC_RC_BAR3_CONFIG_LO_SIZE_MASK;
• writel(tmp, base + PCIE_MISC_RC_BAR3_CONFIG_LO);
• /* Mask all interrupts since we are not handling any yet */
• writel(0xffffffff, base + PCIE_MSI_INTR2_MASK_SET);
• /* Clear any interrupts we find on boot */
• writel(0xffffffff, base + PCIE_MSI_INTR2_CLR);
• if (pcie->gen)

• brcm_pcie_set_gen(pcie, pcie->gen);
  

• /* Unassert the fundamental reset */
• brcm_pcie_perst_set(pcie, 0);
• /* Give the RC/EP time to wake up, before trying to configure RC.

• * Intermittently check status for link-up, up to a total of 100ms.
  

• */
  

• for (i = 0; i < 100 && !brcm_pcie_link_up(pcie); i += 5)

• msleep(5);
  

• if (!brcm_pcie_link_up(pcie)) {

• printf("PCIe BRCM: link down\n");
  

• return -ENODEV;
  

• }
• if (!brcm_pcie_rc_mode(pcie)) {

• printf("PCIe misconfigured; is in EP mode\n");
  

• return -EINVAL;
  

• }
• for (i = 0; i < hose->region_count; i++) {

• struct pci_region *reg = &hose->regions[i];
  

• if (reg->flags != PCI_REGION_MEM)
  

• 	continue;
  

• if (num_out_wins >= BRCM_NUM_PCIE_OUT_WINS)
  

• 	return -EINVAL;
  

• brcm_pcie_set_outbound_win(pcie, num_out_wins, reg->phys_start,
  

• 			   reg->bus_start, reg->size);
  

• num_out_wins++;
  

• }
• /*

• * For config space accesses on the RC, show the right class for
  

• * a PCIe-PCIe bridge (the default setting is to be EP mode).
  

• */
  

• tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
• u32p_replace_bits(&tmp, 0x060400,

• 	  PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
  

• writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
• if (pcie->ssc) {

• ret = brcm_pcie_set_ssc(pcie);
  

• if (ret == 0)
  

• 	ssc_good = true;
  

• else
  

• 	printf("PCIe BRCM: failed attempt to enter SSC mode\n");
  

• }
• lnksta = readw(base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKSTA);
• cls = FIELD_GET(PCI_EXP_LNKSTA_CLS, lnksta);
• nlw = FIELD_GET(PCI_EXP_LNKSTA_NLW, lnksta);
• printf("PCIe BRCM: link up, %s Gbps x%u %s\n", link_speed_to_str(cls),

•       nlw, ssc_good ? "(SSC)" : "(!SSC)");
  

• /* PCIe->SCB endian mode for BAR */
• tmp = readl(base + PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1);
• u32p_replace_bits(&tmp, PCIE_RC_CFG_VENDOR_SPCIFIC_REG1_LITTLE_ENDIAN,

• PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1_ENDIAN_MODE_BAR2_MASK);
  

• writel(tmp, base + PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1);
• /*

• * Refclk from RC should be gated with CLKREQ# input when ASPM L0s,L1
  

• * is enabled => setting the CLKREQ_DEBUG_ENABLE field to 1.
  

• */
  

• tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
• tmp |= PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK;
• writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
• return 0;

+}

+static int brcm_pcie_ofdata_to_platdata(struct udevice *dev) +{

• struct brcm_pcie *pcie = dev_get_priv(dev);
• ofnode dn = dev_ofnode(dev);
• u32 max_link_speed;
• int ret;
• /* Get the controller base address */
• pcie->base = dev_read_addr_ptr(dev);
• if (!pcie->base)

• return -EINVAL;
  

• pcie->ssc = ofnode_read_bool(dn, "brcm,enable-ssc");
• ret = ofnode_read_u32(dn, "max-link-speed", &max_link_speed);
• if (ret < 0 || max_link_speed > 4)

• pcie->gen = 0;
  

• else

• pcie->gen = max_link_speed;
  

• return 0;

+}

+static const struct dm_pci_ops brcm_pcie_ops = {

• .read_config = brcm_pcie_read_config,
• .write_config = brcm_pcie_write_config,

+};

+static const struct udevice_id brcm_pcie_ids[] = {

• { .compatible = "brcm,bcm2711-pcie" },
• { }

+};

+U_BOOT_DRIVER(pcie_brcm_base) = {

• .name = "pcie_brcm",
• .id = UCLASS_PCI,
• .ops = &brcm_pcie_ops,
• .of_match = brcm_pcie_ids,
• .probe = brcm_pcie_probe,
• .ofdata_to_platdata = brcm_pcie_ofdata_to_platdata,
• .priv_auto_alloc_size = sizeof(struct brcm_pcie),

+};

Hi Sylwester,

On Mon, 4 May 2020 at 06:45, Sylwester Nawrocki s.nawrocki@samsung.com wrote:

This patch adds basic driver for the Broadcom STB PCIe host controller. The code is based on Linux upstream driver (pcie-brcmstb.c) with MSI handling removed. The inbound access memory region is not currently parsed from dma-ranges DT property and a fixed 4GB region is used.

The patch has been tested on RPI4 board, i.e. on BCM2711 SoC with VL805 USB Host Controller.

Signed-off-by: Nicolas Saenz Julienne nsaenzjulienne@suse.de Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com

---

Changes since v1:

• fixed argument in brcm_pcie_set_ssc() function call
• changed rc_bar2_size assignment to value 0xC0000000, as in upstream devicetre

Changes since RFC:

• reworked to align with current Linux mainline version and u-boot driver by Nicolas Saenz Julienne

brcmstb pcie

drivers/pci/Kconfig | 6 + drivers/pci/Makefile | 1 + drivers/pci/pcie_brcmstb.c | 594 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 601 insertions(+) create mode 100644 drivers/pci/pcie_brcmstb.c

A few small comments.

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig index 437cd9a..056a021 100644 --- a/drivers/pci/Kconfig +++ b/drivers/pci/Kconfig @@ -197,4 +197,10 @@ config PCIE_MEDIATEK Say Y here if you want to enable Gen2 PCIe controller, which could be found on MT7623 SoC family.

+config PCI_BRCMSTB

•   bool "Broadcom STB PCIe controller"
  

•   depends on DM_PCI
  

•   depends on ARCH_BCM283X
  

•   help
  

•     Say Y here if you want to enable Broadcom STB PCIe controller support.
  

What is STB? What features does this support? You should get a warning here to write at least three lines.

endif diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile index c051ecc..3e53b1f 100644 --- a/drivers/pci/Makefile +++ b/drivers/pci/Makefile @@ -43,3 +43,4 @@ obj-$(CONFIG_PCI_PHYTIUM) += pcie_phytium.o obj-$(CONFIG_PCIE_INTEL_FPGA) += pcie_intel_fpga.o obj-$(CONFIG_PCI_KEYSTONE) += pcie_dw_ti.o obj-$(CONFIG_PCIE_MEDIATEK) += pcie_mediatek.o +obj-$(CONFIG_PCI_BRCMSTB) += pcie_brcmstb.o diff --git a/drivers/pci/pcie_brcmstb.c b/drivers/pci/pcie_brcmstb.c new file mode 100644 index 0000000..c6ddf92 --- /dev/null +++ b/drivers/pci/pcie_brcmstb.c @@ -0,0 +1,594 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Broadcom STB PCIe controller driver
• • Copyright (C) 2020 Samsung Electronics Co., Ltd.
• • Based on upstream Linux kernel driver:
• • drivers/pci/controller/pcie-brcmstb.c
• • Copyright (C) 2009 - 2017 Broadcom
• • Based driver by Nicolas Saenz Julienne
• • Copyright (C) 2020 Nicolas Saenz Julienne nsaenzjulienne@suse.de
• */

+#include <asm/io.h> +#include <common.h> +#include <dm.h> +#include <dm/ofnode.h> +#include <errno.h> +#include <linux/bitfield.h> +#include <linux/log2.h> +#include <pci.h>

Check ordering of include files:

https://www.denx.de/wiki/U-Boot/CodingStyle

+/* BRCM_PCIE_CAP_REGS - Offset for the mandatory capability config regs */ +#define BRCM_PCIE_CAP_REGS 0x00ac

+/* Broadcom STB PCIe Register Offsets */ +#define PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1 0x0188 +#define PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1_ENDIAN_MODE_BAR2_MASK 0xc +#define PCIE_RC_CFG_VENDOR_SPCIFIC_REG1_LITTLE_ENDIAN 0x0

+#define PCIE_RC_CFG_PRIV1_ID_VAL3 0x043c +#define PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK 0xffffff

+#define PCIE_RC_DL_MDIO_ADDR 0x1100 +#define PCIE_RC_DL_MDIO_WR_DATA 0x1104 +#define PCIE_RC_DL_MDIO_RD_DATA 0x1108

+#define PCIE_MISC_MISC_CTRL 0x4008 +#define PCIE_MISC_MISC_CTRL_SCB_ACCESS_EN_MASK 0x1000 +#define PCIE_MISC_MISC_CTRL_CFG_READ_UR_MODE_MASK 0x2000 +#define PCIE_MISC_MISC_CTRL_MAX_BURST_SIZE_MASK 0x300000 +#define PCIE_MISC_MISC_CTRL_MAX_BURST_SIZE_128 0x0 +#define PCIE_MISC_MISC_CTRL_SCB0_SIZE_MASK 0xf8000000

If you have a _MASK, don't you need a _SHIFT to allow you to read from the field?

Can you drop the PCIE_MISC prefix? These are very long and local to this file.

+#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LO 0x400c +#define PCIE_MEM_WIN0_LO(win) \

•           PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LO + ((win) * 4)
  

+#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI 0x4010 +#define PCIE_MEM_WIN0_HI(win) \

•           PCIE_MISC_CPU_2_PCIE_MEM_WIN0_HI + ((win) * 4)
  

+#define PCIE_MISC_RC_BAR1_CONFIG_LO 0x402c +#define PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK 0x1f

+#define PCIE_MISC_RC_BAR2_CONFIG_LO 0x4034 +#define PCIE_MISC_RC_BAR2_CONFIG_LO_SIZE_MASK 0x1f +#define PCIE_MISC_RC_BAR2_CONFIG_HI 0x4038

+#define PCIE_MISC_RC_BAR3_CONFIG_LO 0x403c +#define PCIE_MISC_RC_BAR3_CONFIG_LO_SIZE_MASK 0x1f

+#define PCIE_MISC_PCIE_STATUS 0x4068 +#define PCIE_MISC_PCIE_STATUS_PCIE_PORT_MASK 0x80 +#define PCIE_MISC_PCIE_STATUS_PCIE_DL_ACTIVE_MASK 0x20 +#define PCIE_MISC_PCIE_STATUS_PCIE_PHYLINKUP_MASK 0x10 +#define PCIE_MISC_PCIE_STATUS_PCIE_LINK_IN_L23_MASK 0x40

+#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT 0x4070 +#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT_LIMIT_MASK 0xfff00000 +#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT_BASE_MASK 0xfff0 +#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT_BASE_HI_SHIFT 12 +#define PCIE_MEM_WIN0_BASE_LIMIT(win) \

•           PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT + ((win) * 4)
  

+#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_HI 0x4080 +#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_HI_BASE_MASK 0xff +#define PCIE_MEM_WIN0_BASE_HI(win) \

•           PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_HI + ((win) * 8)
  

+#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LIMIT_HI 0x4084 +#define PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LIMIT_HI_LIMIT_MASK 0xff +#define PCIE_MEM_WIN0_LIMIT_HI(win) \

•           PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LIMIT_HI + ((win) * 8)
  

+#define PCIE_MISC_HARD_PCIE_HARD_DEBUG 0x4204 +#define PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK 0x2 +#define PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK 0x08000000

+#define PCIE_MSI_INTR2_CLR 0x4508 +#define PCIE_MSI_INTR2_MASK_SET 0x4510

+#define PCIE_EXT_CFG_DATA 0x8000

+#define PCIE_EXT_CFG_INDEX 0x9000 +#define PCIE_EXT_BUSNUM_SHIFT 20 +#define PCIE_EXT_SLOT_SHIFT 15 +#define PCIE_EXT_FUNC_SHIFT 12

+#define PCIE_RGR1_SW_INIT_1 0x9210 +#define PCIE_RGR1_SW_INIT_1_PERST_MASK 0x1 +#define PCIE_RGR1_SW_INIT_1_INIT_MASK 0x2

+/* PCIe parameters */ +#define BRCM_NUM_PCIE_OUT_WINS 0x4

+/* MDIO registers */ +#define MDIO_PORT0 0x0 +#define MDIO_DATA_MASK 0x7fffffff +#define MDIO_PORT_MASK 0xf0000 +#define MDIO_REGAD_MASK 0xffff +#define MDIO_CMD_MASK 0xfff00000 +#define MDIO_CMD_READ 0x1 +#define MDIO_CMD_WRITE 0x0 +#define MDIO_DATA_DONE_MASK 0x80000000 +#define MDIO_RD_DONE(x) (((x) & MDIO_DATA_DONE_MASK) ? 1 : 0) +#define MDIO_WT_DONE(x) (((x) & MDIO_DATA_DONE_MASK) ? 0 : 1)

Are these two worth it? You can do this in your code:

if (readl(xxx) & MDIO_DATA_DONE_MASK)

If you must use these, then I think true/false are better than 1/0.

+#define SSC_REGS_ADDR 0x1100 +#define SET_ADDR_OFFSET 0x1f +#define SSC_CNTL_OFFSET 0x2 +#define SSC_CNTL_OVRD_EN_MASK 0x8000 +#define SSC_CNTL_OVRD_VAL_MASK 0x4000 +#define SSC_STATUS_OFFSET 0x1 +#define SSC_STATUS_SSC_MASK 0x400 +#define SSC_STATUS_PLL_LOCK_MASK 0x800

+struct brcm_pcie {

struct comment

•   void __iomem            *base;
  

•   int                     gen;
  

•   bool                    ssc;
  

+};

+#define msleep(a) udelay((a) * 1000)

This is already defined in U-Boot.

+/*

• • This is to convert the size of the inbound "BAR" region to the
• • non-linear values of PCIE_X_MISC_RC_BAR[123]_CONFIG_LO.SIZE

Please use a proper function comments with args and return value.

• */

+static int brcm_pcie_encode_ibar_size(u64 size) +{

•   int log2_in = ilog2(size);
  

•   if (log2_in >= 12 && log2_in <= 15)
  

•           /* Covers 4KB to 32KB (inclusive) */
  

•           return (log2_in - 12) + 0x1c;
  

•   else if (log2_in >= 16 && log2_in <= 37)
  

•           /* Covers 64KB to 32GB, (inclusive) */
  

•           return log2_in - 15;
  

•   /* Something is awry so disable */
  

blank line always before return at end of function

•   return 0;
  

+}

+/* Configuration space read/write support */ +static inline int brcm_pcie_cfg_index(pci_dev_t bdf, int reg) +{

•   return (PCI_DEV(bdf) << PCIE_EXT_SLOT_SHIFT)
  

•           | (PCI_FUNC(bdf) << PCIE_EXT_FUNC_SHIFT)
  

•           | (PCI_BUS(bdf) << PCIE_EXT_BUSNUM_SHIFT)
  

•           | (reg & ~3);
  

+}

+/* The controller is capable of serving in both RC and EP roles */ +static bool brcm_pcie_rc_mode(struct brcm_pcie *pcie) +{

•   u32 val = readl(pcie->base + PCIE_MISC_PCIE_STATUS);
  

Consider using a struct for your registers which would reduce the size of this code.

•   return !!FIELD_GET(PCIE_MISC_PCIE_STATUS_PCIE_PORT_MASK, val);
  

What is FIELD_GET? We have this sort of thing rejected in the past.

Here, since you return a bool, the !! should not be necessary.

+}

+static bool brcm_pcie_link_up(struct brcm_pcie *pcie) +{

•   u32 val = readl(pcie->base + PCIE_MISC_PCIE_STATUS);
  

•   u32 dla = FIELD_GET(PCIE_MISC_PCIE_STATUS_PCIE_DL_ACTIVE_MASK, val);
  

•   u32 plu = FIELD_GET(PCIE_MISC_PCIE_STATUS_PCIE_PHYLINKUP_MASK, val);
  

PCIE_MISC_PCIE_STATUS_PCIE_PHYLINKUP_MASK is way too long.

How about STATUS_PCIE_PHYLINKUP_MASK?

•   return dla && plu;
  

+}

+static int brcm_pcie_config_address(const struct udevice *udev, pci_dev_t bdf,

Please use 'dev' instead of udev for the device.

•                               uint offset, void **paddress)
  

+{

•   struct brcm_pcie *pcie = dev_get_priv(udev);
  

•   unsigned int bus = PCI_BUS(bdf);
  

•   unsigned int dev = PCI_DEV(bdf);
  

•   int idx;
  

•   /*
  

•    * Busses 0 (host PCIe bridge) and 1 (its immediate child)
  

•    * are limited to a single device each
  

•    */
  

•   if ((bus == (udev->seq + 1)) && dev > 0)
  

•           return -ENODEV;
  

•   /* Accesses to the RC go right to the RC registers if PCI device == 0 */
  

•   if (bus == udev->seq) {
  

•           if (PCI_DEV(bdf))
  

•                   return -ENODEV;
  

You can't return that as there is a device. Perhaps ENXIO? Does this indicate an internal error? If so you could add a comment here as to how to fix it.

•           *paddress = pcie->base + offset;
  

•           return 0;
  

•   }
  

•   /* For devices, write to the config space index register */
  

•   idx = brcm_pcie_cfg_index(bdf, 0);
  

•   writel(idx, pcie->base + PCIE_EXT_CFG_INDEX);
  

•   *paddress = pcie->base + PCIE_EXT_CFG_DATA + offset;
  

•   return 0;
  

+}

+static int brcm_pcie_read_config(const struct udevice *bus, pci_dev_t bdf,

•                            uint offset, ulong *valuep,
  

•                            enum pci_size_t size)
  

+{

•   return pci_generic_mmap_read_config(bus, brcm_pcie_config_address,
  

•                                       bdf, offset, valuep, size);
  

+}

+static int brcm_pcie_write_config(struct udevice *bus, pci_dev_t bdf,

•                             uint offset, ulong value,
  

•                             enum pci_size_t size)
  

+{

•   return pci_generic_mmap_write_config(bus, brcm_pcie_config_address,
  

•                                        bdf, offset, value, size);
  

+}

+static const char *link_speed_to_str(unsigned int s) +{

•   static const char * const speed_str[] = { "??", "2.5", "5.0", "8.0" };
  

•   if (s >= ARRAY_SIZE(speed_str))
  

•           s = 0;
  

•   return speed_str[s];
  

+}

+static inline void brcm_pcie_bridge_sw_init_set(struct brcm_pcie *pcie, u32 val) +{

•   u32 tmp;
  

•   tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1);
  

•   u32p_replace_bits(&tmp, val, PCIE_RGR1_SW_INIT_1_INIT_MASK);
  

•   writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1);
  

See clrsetbits_le32(), etc. Below alos.

+}

+static inline void brcm_pcie_perst_set(struct brcm_pcie *pcie, u32 val) +{

•   u32 tmp;
  

•   tmp = readl(pcie->base + PCIE_RGR1_SW_INIT_1);
  

•   u32p_replace_bits(&tmp, val, PCIE_RGR1_SW_INIT_1_PERST_MASK);
  

•   writel(tmp, pcie->base + PCIE_RGR1_SW_INIT_1);
  

+}

+static u32 brcm_pcie_mdio_form_pkt(int port, int regad, int cmd) +{

•   u32 pkt = 0;
  

•   pkt |= FIELD_PREP(MDIO_PORT_MASK, port);
  

•   pkt |= FIELD_PREP(MDIO_REGAD_MASK, regad);
  

•   pkt |= FIELD_PREP(MDIO_CMD_MASK, cmd);
  

•   return pkt;
  

+}

+/* Negative return value indicates error */

This would be incorporated in a full function comment

+static int brcm_pcie_mdio_read(void __iomem *base, u8 port, u8 regad, u32 *val) +{

•   int tries;
  

•   u32 data;
  

•   writel(brcm_pcie_mdio_form_pkt(port, regad, MDIO_CMD_READ),
  

•          base + PCIE_RC_DL_MDIO_ADDR);
  

•   readl(base + PCIE_RC_DL_MDIO_ADDR);
  

•   data = readl(base + PCIE_RC_DL_MDIO_RD_DATA);
  

•   for (tries = 0; !MDIO_RD_DONE(data) && tries < 10; tries++) {
  

•           udelay(10);
  

•           data = readl(base + PCIE_RC_DL_MDIO_RD_DATA);
  

•   }
  

•   *val = FIELD_GET(MDIO_DATA_MASK, data);
  

•   return MDIO_RD_DONE(data) ? 0 : -EIO;
  

+}

+/* Negative return value indicates error */ +static int brcm_pcie_mdio_write(void __iomem *base, u8 port,

•                           u8 regad, u16 wrdata)
  

+{

•   int tries;
  

•   u32 data;
  

•   writel(brcm_pcie_mdio_form_pkt(port, regad, MDIO_CMD_WRITE),
  

•          base + PCIE_RC_DL_MDIO_ADDR);
  

•   readl(base + PCIE_RC_DL_MDIO_ADDR);
  

•   writel(MDIO_DATA_DONE_MASK | wrdata, base + PCIE_RC_DL_MDIO_WR_DATA);
  

•   data = readl(base + PCIE_RC_DL_MDIO_WR_DATA);
  

•   for (tries = 0; !MDIO_WT_DONE(data) && tries < 10; tries++) {
  

•           udelay(10);
  

•           data = readl(base + PCIE_RC_DL_MDIO_WR_DATA);
  

•   }
  

•   return MDIO_WT_DONE(data) ? 0 : -EIO;
  

+}

+/*

• • Configures device for Spread Spectrum Clocking (SSC) mode; negative
• • return value indicates error.
• */

+static int brcm_pcie_set_ssc(struct brcm_pcie *pcie) +{

•   void __iomem *base = pcie->base;
  

•   int pll, ssc;
  

•   int ret;
  

•   u32 tmp;
  

•   ret = brcm_pcie_mdio_write(base, MDIO_PORT0, SET_ADDR_OFFSET,
  

•                              SSC_REGS_ADDR);
  

•   if (ret < 0)
  

•           return ret;
  

•   ret = brcm_pcie_mdio_read(base, MDIO_PORT0, SSC_CNTL_OFFSET, &tmp);
  

•   if (ret < 0)
  

•           return ret;
  

•   u32p_replace_bits(&tmp, 1, SSC_CNTL_OVRD_EN_MASK);
  

•   u32p_replace_bits(&tmp, 1, SSC_CNTL_OVRD_VAL_MASK);
  

•   ret = brcm_pcie_mdio_write(base, MDIO_PORT0, SSC_CNTL_OFFSET, tmp);
  

•   if (ret < 0)
  

•           return ret;
  

•   udelay(1000);
  

•   ret = brcm_pcie_mdio_read(base, MDIO_PORT0, SSC_STATUS_OFFSET, &tmp);
  

•   if (ret < 0)
  

•           return ret;
  

•   ssc = FIELD_GET(SSC_STATUS_SSC_MASK, tmp);
  

•   pll = FIELD_GET(SSC_STATUS_PLL_LOCK_MASK, tmp);
  

•   return ssc && pll ? 0 : -EIO;
  

+}

+/* Limits operation to a specific generation (1, 2, or 3) */ +static void brcm_pcie_set_gen(struct brcm_pcie *pcie, int gen) +{

•   void __iomem *base = pcie->base;
  

•   u16 lnkctl2 = readw(base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCTL2);
  

•   u32 lnkcap = readl(base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCAP);
  

•   lnkcap = (lnkcap & ~PCI_EXP_LNKCAP_SLS) | gen;
  

•   writel(lnkcap, base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCAP);
  

•   lnkctl2 = (lnkctl2 & ~0xf) | gen;
  

•   writew(lnkctl2, base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKCTL2);
  

+}

+static void brcm_pcie_set_outbound_win(struct brcm_pcie *pcie,

•                                  unsigned int win, u64 phys_addr,
  

•                                  u64 pcie_addr, u64 size)
  

+{

•   void __iomem *base = pcie->base;
  

•   u32 phys_addr_mb_high, limit_addr_mb_high;
  

•   phys_addr_t phys_addr_mb, limit_addr_mb;
  

•   int high_addr_shift;
  

•   u32 tmp;
  

•   /* Set the base of the pcie_addr window */
  

•   writel(lower_32_bits(pcie_addr), base + PCIE_MEM_WIN0_LO(win));
  

•   writel(upper_32_bits(pcie_addr), base + PCIE_MEM_WIN0_HI(win));
  

•   /* Write the addr base & limit lower bits (in MBs) */
  

•   phys_addr_mb = phys_addr / SZ_1M;
  

•   limit_addr_mb = (phys_addr + size - 1) / SZ_1M;
  

•   tmp = readl(base + PCIE_MEM_WIN0_BASE_LIMIT(win));
  

•   u32p_replace_bits(&tmp, phys_addr_mb,
  

•                     PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT_BASE_MASK);
  

•   u32p_replace_bits(&tmp, limit_addr_mb,
  

•                     PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT_LIMIT_MASK);
  

•   writel(tmp, base + PCIE_MEM_WIN0_BASE_LIMIT(win));
  

•   /* Write the cpu & limit addr upper bits */
  

•   high_addr_shift = PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_LIMIT_BASE_HI_SHIFT;
  

•   phys_addr_mb_high = phys_addr_mb >> high_addr_shift;
  

•   tmp = readl(base + PCIE_MEM_WIN0_BASE_HI(win));
  

•   u32p_replace_bits(&tmp, phys_addr_mb_high,
  

•                     PCIE_MISC_CPU_2_PCIE_MEM_WIN0_BASE_HI_BASE_MASK);
  

•   writel(tmp, base + PCIE_MEM_WIN0_BASE_HI(win));
  

•   limit_addr_mb_high = limit_addr_mb >> high_addr_shift;
  

•   tmp = readl(base + PCIE_MEM_WIN0_LIMIT_HI(win));
  

•   u32p_replace_bits(&tmp, limit_addr_mb_high,
  

•                     PCIE_MISC_CPU_2_PCIE_MEM_WIN0_LIMIT_HI_LIMIT_MASK);
  

•   writel(tmp, base + PCIE_MEM_WIN0_LIMIT_HI(win));
  

+}

+static int brcm_pcie_probe(struct udevice *dev) +{

•   struct udevice *ctlr = pci_get_controller(dev);
  

•   struct pci_controller *hose = dev_get_uclass_priv(ctlr);
  

•   struct brcm_pcie *pcie = dev_get_priv(dev);
  

•   void __iomem *base = pcie->base;
  

•   bool ssc_good = false;
  

•   int num_out_wins = 0;
  

•   u64 rc_bar2_offset, rc_bar2_size;
  

•   unsigned int scb_size_val;
  

•   int i, ret;
  

•   u16 nlw, cls, lnksta;
  

•   u32 tmp;
  

•   /* Reset the bridge */
  

•   brcm_pcie_bridge_sw_init_set(pcie, 1);
  

•   udelay(150);
  

Please add a comment as to how you chose the value, and below.

•   /* Take the bridge out of reset */
  

•   brcm_pcie_bridge_sw_init_set(pcie, 0);
  

•   tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
  

•   tmp &= ~PCIE_MISC_HARD_PCIE_HARD_DEBUG_SERDES_IDDQ_MASK;
  

•   writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
  

•   /* Wait for SerDes to be stable */
  

•   udelay(150);
  

•   /* Set SCB_MAX_BURST_SIZE, CFG_READ_UR_MODE, SCB_ACCESS_EN */
  

•   u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_SCB_ACCESS_EN_MASK);
  

•   u32p_replace_bits(&tmp, 1, PCIE_MISC_MISC_CTRL_CFG_READ_UR_MODE_MASK);
  

•   u32p_replace_bits(&tmp, PCIE_MISC_MISC_CTRL_MAX_BURST_SIZE_128,
  

•                     PCIE_MISC_MISC_CTRL_MAX_BURST_SIZE_MASK);
  

•   writel(tmp, base + PCIE_MISC_MISC_CTRL);
  

•   /*
  

•    * TODO: When support for other SoCs than BCM2711 is added we may
  

•    * need to use the base address and size(s) provided in the dma-ranges
  

•    * property.
  

•    */
  

•   rc_bar2_offset = 0;
  

•   rc_bar2_size = 0xc0000000;
  

•   tmp = lower_32_bits(rc_bar2_offset);
  

•   u32p_replace_bits(&tmp, brcm_pcie_encode_ibar_size(rc_bar2_size),
  

•                     PCIE_MISC_RC_BAR2_CONFIG_LO_SIZE_MASK);
  

•   writel(tmp, base + PCIE_MISC_RC_BAR2_CONFIG_LO);
  

•   writel(upper_32_bits(rc_bar2_offset),
  

•          base + PCIE_MISC_RC_BAR2_CONFIG_HI);
  

•   scb_size_val = rc_bar2_size ?
  

•                  ilog2(rc_bar2_size) - 15 : 0xf; /* 0xf is 1GB */
  

•   tmp = readl(base + PCIE_MISC_MISC_CTRL);
  

•   u32p_replace_bits(&tmp, scb_size_val,
  

•                     PCIE_MISC_MISC_CTRL_SCB0_SIZE_MASK);
  

•   writel(tmp, base + PCIE_MISC_MISC_CTRL);
  

•   /* Disable the PCIe->GISB memory window (RC_BAR1) */
  

•   tmp = readl(base + PCIE_MISC_RC_BAR1_CONFIG_LO);
  

•   tmp &= ~PCIE_MISC_RC_BAR1_CONFIG_LO_SIZE_MASK;
  

•   writel(tmp, base + PCIE_MISC_RC_BAR1_CONFIG_LO);
  

•   /* Disable the PCIe->SCB memory window (RC_BAR3) */
  

•   tmp = readl(base + PCIE_MISC_RC_BAR3_CONFIG_LO);
  

•   tmp &= ~PCIE_MISC_RC_BAR3_CONFIG_LO_SIZE_MASK;
  

•   writel(tmp, base + PCIE_MISC_RC_BAR3_CONFIG_LO);
  

•   /* Mask all interrupts since we are not handling any yet */
  

•   writel(0xffffffff, base + PCIE_MSI_INTR2_MASK_SET);
  

•   /* Clear any interrupts we find on boot */
  

•   writel(0xffffffff, base + PCIE_MSI_INTR2_CLR);
  

•   if (pcie->gen)
  

•           brcm_pcie_set_gen(pcie, pcie->gen);
  

•   /* Unassert the fundamental reset */
  

•   brcm_pcie_perst_set(pcie, 0);
  

•   /* Give the RC/EP time to wake up, before trying to configure RC.
  

•    * Intermittently check status for link-up, up to a total of 100ms.
  

•    */
  

•   for (i = 0; i < 100 && !brcm_pcie_link_up(pcie); i += 5)
  

•           msleep(5);
  

•   if (!brcm_pcie_link_up(pcie)) {
  

•           printf("PCIe BRCM: link down\n");
  

•           return -ENODEV;
  

Again, cannot return that error

•   }
  

•   if (!brcm_pcie_rc_mode(pcie)) {
  

•           printf("PCIe misconfigured; is in EP mode\n");
  

•           return -EINVAL;
  

•   }
  

•   for (i = 0; i < hose->region_count; i++) {
  

•           struct pci_region *reg = &hose->regions[i];
  

•           if (reg->flags != PCI_REGION_MEM)
  

•                   continue;
  

•           if (num_out_wins >= BRCM_NUM_PCIE_OUT_WINS)
  

•                   return -EINVAL;
  

•           brcm_pcie_set_outbound_win(pcie, num_out_wins, reg->phys_start,
  

•                                      reg->bus_start, reg->size);
  

•           num_out_wins++;
  

•   }
  

•   /*
  

•    * For config space accesses on the RC, show the right class for
  

•    * a PCIe-PCIe bridge (the default setting is to be EP mode).
  

•    */
  

•   tmp = readl(base + PCIE_RC_CFG_PRIV1_ID_VAL3);
  

•   u32p_replace_bits(&tmp, 0x060400,
  

•                     PCIE_RC_CFG_PRIV1_ID_VAL3_CLASS_CODE_MASK);
  

•   writel(tmp, base + PCIE_RC_CFG_PRIV1_ID_VAL3);
  

•   if (pcie->ssc) {
  

•           ret = brcm_pcie_set_ssc(pcie);
  

•           if (ret == 0)
  

•                   ssc_good = true;
  

•           else
  

•                   printf("PCIe BRCM: failed attempt to enter SSC mode\n");
  

Should this return an error?

•   }
  

•   lnksta = readw(base + BRCM_PCIE_CAP_REGS + PCI_EXP_LNKSTA);
  

•   cls = FIELD_GET(PCI_EXP_LNKSTA_CLS, lnksta);
  

•   nlw = FIELD_GET(PCI_EXP_LNKSTA_NLW, lnksta);
  

•   printf("PCIe BRCM: link up, %s Gbps x%u %s\n", link_speed_to_str(cls),
  

•          nlw, ssc_good ? "(SSC)" : "(!SSC)");
  

•   /* PCIe->SCB endian mode for BAR */
  

•   tmp = readl(base + PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1);
  

•   u32p_replace_bits(&tmp, PCIE_RC_CFG_VENDOR_SPCIFIC_REG1_LITTLE_ENDIAN,
  

•           PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1_ENDIAN_MODE_BAR2_MASK);
  

•   writel(tmp, base + PCIE_RC_CFG_VENDOR_VENDOR_SPECIFIC_REG1);
  

•   /*
  

•    * Refclk from RC should be gated with CLKREQ# input when ASPM L0s,L1
  

•    * is enabled => setting the CLKREQ_DEBUG_ENABLE field to 1.
  

•    */
  

•   tmp = readl(base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
  

•   tmp |= PCIE_MISC_HARD_PCIE_HARD_DEBUG_CLKREQ_DEBUG_ENABLE_MASK;
  

•   writel(tmp, base + PCIE_MISC_HARD_PCIE_HARD_DEBUG);
  

•   return 0;
  

+}

+static int brcm_pcie_ofdata_to_platdata(struct udevice *dev) +{

•   struct brcm_pcie *pcie = dev_get_priv(dev);
  

•   ofnode dn = dev_ofnode(dev);
  

•   u32 max_link_speed;
  

•   int ret;
  

•   /* Get the controller base address */
  

•   pcie->base = dev_read_addr_ptr(dev);
  

•   if (!pcie->base)
  

•           return -EINVAL;
  

•   pcie->ssc = ofnode_read_bool(dn, "brcm,enable-ssc");
  

•   ret = ofnode_read_u32(dn, "max-link-speed", &max_link_speed);
  

•   if (ret < 0 || max_link_speed > 4)
  

•           pcie->gen = 0;
  

•   else
  

•           pcie->gen = max_link_speed;
  

•   return 0;
  

+}

+static const struct dm_pci_ops brcm_pcie_ops = {

•   .read_config    = brcm_pcie_read_config,
  

•   .write_config   = brcm_pcie_write_config,
  

+};

+static const struct udevice_id brcm_pcie_ids[] = {

•   { .compatible = "brcm,bcm2711-pcie" },
  

•   { }
  

+};

+U_BOOT_DRIVER(pcie_brcm_base) = {

•   .name                   = "pcie_brcm",
  

•   .id                     = UCLASS_PCI,
  

•   .ops                    = &brcm_pcie_ops,
  

•   .of_match               = brcm_pcie_ids,
  

•   .probe                  = brcm_pcie_probe,
  

•   .ofdata_to_platdata     = brcm_pcie_ofdata_to_platdata,
  

•   .priv_auto_alloc_size   = sizeof(struct brcm_pcie),
  

+};

2.7.4

Regards, Simon

Hi Sylwester,

On Fri, 8 May 2020 at 05:46, Sylwester Nawrocki s.nawrocki@samsung.com wrote:

Hi Simon,

On 06.05.2020 16:47, Simon Glass wrote:

...

On Mon, 4 May 2020 at 06:45, Sylwester Nawrocki s.nawrocki@samsung.com wrote:

...

...

---

drivers/pci/Kconfig | 6 + drivers/pci/Makefile | 1 + drivers/pci/pcie_brcmstb.c | 594 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 601 insertions(+) create mode 100644 drivers/pci/pcie_brcmstb.c

A few small comments.

Thank you for time and a valuable review.

...

...

diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig

...

...

+config PCI_BRCMSTB

•   bool "Broadcom STB PCIe controller"
  

•   depends on DM_PCI
  

•   depends on ARCH_BCM283X
  

•   help
  

•     Say Y here if you want to enable Broadcom STB PCIe controller support.
  

What is STB? What features does this support? You should get a warning here to write at least three lines.

I'm going to change that help text to something along the lines of:

Say Y here if you want to enable support for PCIe controller on Broadcom set-top-box (STB) SoCs. This driver currently supports only BCM2711 SoC and RC mode of the controller.

...

...

diff --git a/drivers/pci/pcie_brcmstb.c b/drivers/pci/pcie_brcmstb.c new file mode 100644 index 0000000..c6ddf92 --- /dev/null +++ b/drivers/pci/pcie_brcmstb.c @@ -0,0 +1,594 @@

...

...

+#include <asm/io.h> +#include <common.h> +#include <dm.h> +#include <dm/ofnode.h> +#include <errno.h> +#include <linux/bitfield.h> +#include <linux/log2.h> +#include <pci.h>

Check ordering of include files:

https://protect2.fireeye.com/url?k=c3a0292d-9e737093-c3a1a262-0cc47a31ba82-6...

Thanks for the hint, it felt there was something wrong with the ordering.

...

...

+#define PCIE_MISC_MISC_CTRL 0x4008 +#define PCIE_MISC_MISC_CTRL_SCB_ACCESS_EN_MASK 0x1000 +#define PCIE_MISC_MISC_CTRL_CFG_READ_UR_MODE_MASK 0x2000 +#define PCIE_MISC_MISC_CTRL_MAX_BURST_SIZE_MASK 0x300000 +#define PCIE_MISC_MISC_CTRL_MAX_BURST_SIZE_128 0x0 +#define PCIE_MISC_MISC_CTRL_SCB0_SIZE_MASK 0xf8000000

If you have a _MASK, don't you need a _SHIFT to allow you to read from the field?

I had shift definitions originally but these got removed when we started to use the FIELD_GET macro and similar. Shifts are retrieved there from a mask by ffs() call (find first bit set in a word). If that's not preferred in u-boot I will switch back to using explicit shift definitions.

+Tom Rini

We've had a policy of avoiding these for some time, but now that Linux has them, should we pull this in and start using it? It is not that widely used in Linux but has been there for a few years.

Regards, Simon