Message ID | 20200504124523.23484-6-s.nawrocki@samsung.com |
---|---|
State | Superseded |
Headers | show |
Series | USB host support for Raspberry Pi 4 board | expand |
On 04/05/2020 14:45, Sylwester Nawrocki wrote: > From: Marek Szyprowski <m.szyprowski at 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 at samsung.com> > Signed-off-by: Sylwester Nawrocki <s.nawrocki at samsung.com> > Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne at 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. > + .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:00, Matthias Brugger wrote: > > > On 04/05/2020 14:45, Sylwester Nawrocki wrote: >> From: Marek Szyprowski <m.szyprowski at 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 at samsung.com> >> Signed-off-by: Sylwester Nawrocki <s.nawrocki at samsung.com> >> Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne at 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; >>
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 at 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 at samsung.com> >> Signed-off-by: Sylwester Nawrocki <s.nawrocki at samsung.com> >> Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne at 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. Best regards
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 at 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 at samsung.com> >>> Signed-off-by: Sylwester Nawrocki <s.nawrocki at samsung.com> >>> Reviewed-by: Nicolas Saenz Julienne <nsaenzjulienne at 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
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) + +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 }, { + .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++) { mem_map[i].virt = pd[i].virt; mem_map[i].phys = pd[i].phys; mem_map[i].size = pd[i].size;