[PATCH] arm64: dts: marvell: armada-37xx: Increase PCIe IO size from 64 KiB to 1 MiB

[PATCH] arm64: dts: marvell: armada-37xx: Increase PCIe IO size from 64 KiB to 1 MiB

[Pali Rohár]

Commit 514ef1e62d65 ("arm64: dts: marvell: armada-37xx: Extend PCIe MEM
space") increased size of PCIe MEM to 127 MiB, which is the maximal
possible size for allocated 128 MiB PCIe window. PCIe IO size in that
commit was unchanged.

Armada 3720 PCIe controller supports 32-bit IO space mapping so it is
possible to assign more than 64 KiB if address space for IO.

Currently controller has assigned 127 MiB + 64 KiB memory and therefore
there is 960 KiB of unused memory. So assign it to IO space by increasing
IO window from 64 KiB to 1 MiB.

DTS file armada-3720-turris-mox.dts already uses whole 128 MiB space, so
only update comment about 32-bit IO space mapping.

Signed-off-by: Pali Rohár <pali@kernel.org>
Fixes: 514ef1e62d65 ("arm64: dts: marvell: armada-37xx: Extend PCIe MEM space")
---
 arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts | 2 +-
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi           | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
index dd01409d4bb7..23e1b07c060a 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
@@ -153,7 +153,7 @@
 	 * 2 size cells and also expects that the second range starts at 16 MB offset. If these
 	 * conditions are not met then U-Boot crashes during loading kernel DTB file. PCIe address
 	 * space is 128 MB long, so the best split between MEM and IO is to use fixed 16 MB window
-	 * for IO and the rest 112 MB (64+32+16) for MEM, despite that maximal IO size is just 64 kB.
+	 * for IO and the rest 112 MB (64+32+16) for MEM. Controller supports 32-bit IO mapping.
 	 * This bug is not present in U-Boot ports for other Armada 3700 devices and is fixed in
 	 * U-Boot version 2021.07. See relevant U-Boot commits (the last one contains fix):
 	 * https://source.denx.de/u-boot/u-boot/-/commit/cb2ddb291ee6fcbddd6d8f4ff49089dfe580f5d7
diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index 1c74f02535c6..8d59eabadce6 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -508,12 +508,12 @@
 			/*
 			 * The 128 MiB address range [0xe8000000-0xf0000000] is
 			 * dedicated for PCIe and can be assigned to 8 windows
-			 * with size a power of two. Use one 64 KiB window for
+			 * with size a power of two. Use one 1 MiB window for
 			 * IO at the end and the remaining seven windows
 			 * (totaling 127 MiB) for MEM.
 			 */
 			ranges = <0x82000000 0 0xe8000000   0 0xe8000000   0 0x07f00000   /* Port 0 MEM */
-				  0x81000000 0 0xefff0000   0 0xefff0000   0 0x00010000>; /* Port 0 IO */
+				  0x81000000 0 0xeff00000   0 0xeff00000   0 0x00100000>; /* Port 0 IO*/
 			interrupt-map-mask = <0 0 0 7>;
 			interrupt-map = <0 0 0 1 &pcie_intc 0>,
 					<0 0 0 2 &pcie_intc 1>,
-- 
2.20.1

Re: [PATCH] arm64: dts: marvell: armada-37xx: Increase PCIe IO size from 64 KiB to 1 MiB

[Gregory CLEMENT]

Pali Rohár <pali@kernel.org> writes:

> Commit 514ef1e62d65 ("arm64: dts: marvell: armada-37xx: Extend PCIe MEM
> space") increased size of PCIe MEM to 127 MiB, which is the maximal
> possible size for allocated 128 MiB PCIe window. PCIe IO size in that
> commit was unchanged.
>
> Armada 3720 PCIe controller supports 32-bit IO space mapping so it is
> possible to assign more than 64 KiB if address space for IO.
>
> Currently controller has assigned 127 MiB + 64 KiB memory and therefore
> there is 960 KiB of unused memory. So assign it to IO space by increasing
> IO window from 64 KiB to 1 MiB.
>
> DTS file armada-3720-turris-mox.dts already uses whole 128 MiB space, so
> only update comment about 32-bit IO space mapping.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Fixes: 514ef1e62d65 ("arm64: dts: marvell: armada-37xx: Extend PCIe
> MEM space")

Applied on mvebu/fixes

Thanks,

Gregory

> ---
>  arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts | 2 +-
>  arch/arm64/boot/dts/marvell/armada-37xx.dtsi           | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> index dd01409d4bb7..23e1b07c060a 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> @@ -153,7 +153,7 @@
>  	 * 2 size cells and also expects that the second range starts at 16 MB offset. If these
>  	 * conditions are not met then U-Boot crashes during loading kernel DTB file. PCIe address
>  	 * space is 128 MB long, so the best split between MEM and IO is to use fixed 16 MB window
> -	 * for IO and the rest 112 MB (64+32+16) for MEM, despite that maximal IO size is just 64 kB.
> +	 * for IO and the rest 112 MB (64+32+16) for MEM. Controller supports 32-bit IO mapping.
>  	 * This bug is not present in U-Boot ports for other Armada 3700 devices and is fixed in
>  	 * U-Boot version 2021.07. See relevant U-Boot commits (the last one contains fix):
>  	 * https://source.denx.de/u-boot/u-boot/-/commit/cb2ddb291ee6fcbddd6d8f4ff49089dfe580f5d7
> diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> index 1c74f02535c6..8d59eabadce6 100644
> --- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> @@ -508,12 +508,12 @@
>  			/*
>  			 * The 128 MiB address range [0xe8000000-0xf0000000] is
>  			 * dedicated for PCIe and can be assigned to 8 windows
> -			 * with size a power of two. Use one 64 KiB window for
> +			 * with size a power of two. Use one 1 MiB window for
>  			 * IO at the end and the remaining seven windows
>  			 * (totaling 127 MiB) for MEM.
>  			 */
>  			ranges = <0x82000000 0 0xe8000000   0 0xe8000000   0 0x07f00000   /* Port 0 MEM */
> -				  0x81000000 0 0xefff0000   0 0xefff0000   0 0x00010000>; /* Port 0 IO */
> +				  0x81000000 0 0xeff00000   0 0xeff00000   0 0x00100000>; /* Port 0 IO*/
>  			interrupt-map-mask = <0 0 0 7>;
>  			interrupt-map = <0 0 0 1 &pcie_intc 0>,
>  					<0 0 0 2 &pcie_intc 1>,
> -- 
> 2.20.1
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH] arm64: dts: marvell: armada-37xx: Increase PCIe IO size from 64 KiB to 1 MiB

[Arnd Bergmann]

On Thu, Jan 13, 2022 at 6:07 PM Pali Rohár <pali@kernel.org> wrote:
>
> Commit 514ef1e62d65 ("arm64: dts: marvell: armada-37xx: Extend PCIe MEM
> space") increased size of PCIe MEM to 127 MiB, which is the maximal
> possible size for allocated 128 MiB PCIe window. PCIe IO size in that
> commit was unchanged.
>
> Armada 3720 PCIe controller supports 32-bit IO space mapping so it is
> possible to assign more than 64 KiB if address space for IO.
>
> Currently controller has assigned 127 MiB + 64 KiB memory and therefore
> there is 960 KiB of unused memory. So assign it to IO space by increasing
> IO window from 64 KiB to 1 MiB.
>
> DTS file armada-3720-turris-mox.dts already uses whole 128 MiB space, so
> only update comment about 32-bit IO space mapping.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Fixes: 514ef1e62d65 ("arm64: dts: marvell: armada-37xx: Extend PCIe MEM space")

I just saw this is the fixes pull request, and it seems very odd. Does this
fix an actual bug? Note that Linux normally doesn't map more than 64KB
of I/O space per PCI domain, so it should not make a difference to us.

Also, note that having a high bus address for the I/O space (0xefff0000,
as as the CPU physical address here) means that a lot of the older
devices that actually require I/O space won't work, because they need a
low bus address in the first few KB.

Is this mapping a requirement from a broken bootloader, or can you change
the mapping of the I/O port window in the physical space to the usual
bus address 0?

        Arnd

Re: [PATCH] arm64: dts: marvell: armada-37xx: Increase PCIe IO size from 64 KiB to 1 MiB

[Pali Rohár]

On Friday 18 February 2022 17:43:04 Arnd Bergmann wrote:
> On Thu, Jan 13, 2022 at 6:07 PM Pali Rohár <pali@kernel.org> wrote:
> >
> > Commit 514ef1e62d65 ("arm64: dts: marvell: armada-37xx: Extend PCIe MEM
> > space") increased size of PCIe MEM to 127 MiB, which is the maximal
> > possible size for allocated 128 MiB PCIe window. PCIe IO size in that
> > commit was unchanged.
> >
> > Armada 3720 PCIe controller supports 32-bit IO space mapping so it is
> > possible to assign more than 64 KiB if address space for IO.
> >
> > Currently controller has assigned 127 MiB + 64 KiB memory and therefore
> > there is 960 KiB of unused memory. So assign it to IO space by increasing
> > IO window from 64 KiB to 1 MiB.
> >
> > DTS file armada-3720-turris-mox.dts already uses whole 128 MiB space, so
> > only update comment about 32-bit IO space mapping.
> >
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Fixes: 514ef1e62d65 ("arm64: dts: marvell: armada-37xx: Extend PCIe MEM space")
> 
> I just saw this is the fixes pull request, and it seems very odd. Does this
> fix an actual bug?

Do you mean this patch or commit 514ef1e62d65?

> Note that Linux normally doesn't map more than 64KB
> of I/O space per PCI domain, so it should not make a difference to us.

Last time I looked into ARM code, it can allocate more than 64 kB for IO.

> Also, note that having a high bus address for the I/O space (0xefff0000,
> as as the CPU physical address here) means that a lot of the older
> devices that actually require I/O space won't work, because they need a
> low bus address in the first few KB.
> 
> Is this mapping a requirement from a broken bootloader, or can you change
> the mapping of the I/O port window in the physical space to the usual
> bus address 0?

At physical address 0x0 it is not possible as at this address is mapped
DDR.

ARM Trusted-Firmware sets PCIe space to range [0xe8000000-0xf0000000].
This (default) configuration is specified in DTS file. Which parts of
this range is used for IO and which MEM is up to the a3720 PCIe kernel
driver and currently it configures it based on how sub-ranges are
specified in DT.

In some cases (e.g. when board has 4 GB of RAM), TF-A relocates this
PCIe range to different location (otherwise it cannot activate more than
2 GB of RAM) and U-Boot during loading of kernel DTB file, is patching
it.

It could be possible to change TF-A code to move PCIe space to different
location (from [0xe8000000-0xf0000000]) but not to 0x0. But changing it
means to move other parts and invent mapping in which most of RAM can be
mapped to...

> 
>         Arnd

Re: [PATCH] arm64: dts: marvell: armada-37xx: Increase PCIe IO size from 64 KiB to 1 MiB

[Arnd Bergmann]

On Fri, Feb 18, 2022 at 5:55 PM Pali Rohár <pali@kernel.org> wrote:
> On Friday 18 February 2022 17:43:04 Arnd Bergmann wrote:
> > On Thu, Jan 13, 2022 at 6:07 PM Pali Rohár <pali@kernel.org> wrote:
> > >
> > > Commit 514ef1e62d65 ("arm64: dts: marvell: armada-37xx: Extend PCIe MEM
> > > space") increased size of PCIe MEM to 127 MiB, which is the maximal
> > > possible size for allocated 128 MiB PCIe window. PCIe IO size in that
> > > commit was unchanged.
> > >
> > > Armada 3720 PCIe controller supports 32-bit IO space mapping so it is
> > > possible to assign more than 64 KiB if address space for IO.
> > >
> > > Currently controller has assigned 127 MiB + 64 KiB memory and therefore
> > > there is 960 KiB of unused memory. So assign it to IO space by increasing
> > > IO window from 64 KiB to 1 MiB.
> > >
> > > DTS file armada-3720-turris-mox.dts already uses whole 128 MiB space, so
> > > only update comment about 32-bit IO space mapping.
> > >
> > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > Fixes: 514ef1e62d65 ("arm64: dts: marvell: armada-37xx: Extend PCIe MEM space")
> >
> > I just saw this is the fixes pull request, and it seems very odd. Does this
> > fix an actual bug?
>
> Do you mean this patch or commit 514ef1e62d65?

This one. 514ef1e62d65 looks fine.

> > Note that Linux normally doesn't map more than 64KB
> > of I/O space per PCI domain, so it should not make a difference to us.
>
> Last time I looked into ARM code, it can allocate more than 64 kB for IO.



> > Also, note that having a high bus address for the I/O space (0xefff0000,
> > as as the CPU physical address here) means that a lot of the older
> > devices that actually require I/O space won't work, because they need a
> > low bus address in the first few KB.
> >
> > Is this mapping a requirement from a broken bootloader, or can you change
> > the mapping of the I/O port window in the physical space to the usual
> > bus address 0?
>
> At physical address 0x0 it is not possible as at this address is mapped
> DDR.

I meant bus address 0, not CPU physical address 0 of course. We don't
care where in physical space the I/O window is.

> ARM Trusted-Firmware sets PCIe space to range [0xe8000000-0xf0000000].
> This (default) configuration is specified in DTS file. Which parts of
> this range is used for IO and which MEM is up to the a3720 PCIe kernel
> driver and currently it configures it based on how sub-ranges are
> specified in DT.
>
> In some cases (e.g. when board has 4 GB of RAM), TF-A relocates this
> PCIe range to different location (otherwise it cannot activate more than
> 2 GB of RAM) and U-Boot during loading of kernel DTB file, is patching
> it.
>
> It could be possible to change TF-A code to move PCIe space to different
> location (from [0xe8000000-0xf0000000]) but not to 0x0. But changing it
> means to move other parts and invent mapping in which most of RAM can be
> mapped to...

Can't you change the mapping to have a bus address that is different
the physical address?

       Arnd

Re: [PATCH] arm64: dts: marvell: armada-37xx: Increase PCIe IO size from 64 KiB to 1 MiB

[Pali Rohár]

On Friday 18 February 2022 18:19:57 Arnd Bergmann wrote:
> On Fri, Feb 18, 2022 at 5:55 PM Pali Rohár <pali@kernel.org> wrote:
> > On Friday 18 February 2022 17:43:04 Arnd Bergmann wrote:
> > > On Thu, Jan 13, 2022 at 6:07 PM Pali Rohár <pali@kernel.org> wrote:
> > > >
> > > > Commit 514ef1e62d65 ("arm64: dts: marvell: armada-37xx: Extend PCIe MEM
> > > > space") increased size of PCIe MEM to 127 MiB, which is the maximal
> > > > possible size for allocated 128 MiB PCIe window. PCIe IO size in that
> > > > commit was unchanged.
> > > >
> > > > Armada 3720 PCIe controller supports 32-bit IO space mapping so it is
> > > > possible to assign more than 64 KiB if address space for IO.
> > > >
> > > > Currently controller has assigned 127 MiB + 64 KiB memory and therefore
> > > > there is 960 KiB of unused memory. So assign it to IO space by increasing
> > > > IO window from 64 KiB to 1 MiB.
> > > >
> > > > DTS file armada-3720-turris-mox.dts already uses whole 128 MiB space, so
> > > > only update comment about 32-bit IO space mapping.
> > > >
> > > > Signed-off-by: Pali Rohár <pali@kernel.org>
> > > > Fixes: 514ef1e62d65 ("arm64: dts: marvell: armada-37xx: Extend PCIe MEM space")
> > >
> > > I just saw this is the fixes pull request, and it seems very odd. Does this
> > > fix an actual bug?
> >
> > Do you mean this patch or commit 514ef1e62d65?
> 
> This one. 514ef1e62d65 looks fine.

Well, this patch just increase size of IO window, nothing more. What
what is wrong with this patch if is just moves end of the window?

> > > Note that Linux normally doesn't map more than 64KB
> > > of I/O space per PCI domain, so it should not make a difference to us.
> >
> > Last time I looked into ARM code, it can allocate more than 64 kB for IO.
> 
> 
> 
> > > Also, note that having a high bus address for the I/O space (0xefff0000,
> > > as as the CPU physical address here) means that a lot of the older
> > > devices that actually require I/O space won't work, because they need a
> > > low bus address in the first few KB.
> > >
> > > Is this mapping a requirement from a broken bootloader, or can you change
> > > the mapping of the I/O port window in the physical space to the usual
> > > bus address 0?
> >
> > At physical address 0x0 it is not possible as at this address is mapped
> > DDR.
> 
> I meant bus address 0, not CPU physical address 0 of course. We don't
> care where in physical space the I/O window is.

Currently all mapping between CPU and PCIe is 1:1.

There are registers for adding remapping, but nobody played with it yet.
And it needs to be done again in TF-A (or probably in U-Boot could be too).

> > ARM Trusted-Firmware sets PCIe space to range [0xe8000000-0xf0000000].
> > This (default) configuration is specified in DTS file. Which parts of
> > this range is used for IO and which MEM is up to the a3720 PCIe kernel
> > driver and currently it configures it based on how sub-ranges are
> > specified in DT.
> >
> > In some cases (e.g. when board has 4 GB of RAM), TF-A relocates this
> > PCIe range to different location (otherwise it cannot activate more than
> > 2 GB of RAM) and U-Boot during loading of kernel DTB file, is patching
> > it.
> >
> > It could be possible to change TF-A code to move PCIe space to different
> > location (from [0xe8000000-0xf0000000]) but not to 0x0. But changing it
> > means to move other parts and invent mapping in which most of RAM can be
> > mapped to...
> 
> Can't you change the mapping to have a bus address that is different
> the physical address?
> 
>        Arnd

That could be possible, need to investigate... but I think it would be
done in bootloader and then by patching DTB file on the fly.

Re: [PATCH] arm64: dts: marvell: armada-37xx: Increase PCIe IO size from 64 KiB to 1 MiB

[Pali Rohár]

On Friday 18 February 2022 17:43:04 Arnd Bergmann wrote:
> On Thu, Jan 13, 2022 at 6:07 PM Pali Rohár <pali@kernel.org> wrote:
> >
> > Commit 514ef1e62d65 ("arm64: dts: marvell: armada-37xx: Extend PCIe MEM
> > space") increased size of PCIe MEM to 127 MiB, which is the maximal
> > possible size for allocated 128 MiB PCIe window. PCIe IO size in that
> > commit was unchanged.
> >
> > Armada 3720 PCIe controller supports 32-bit IO space mapping so it is
> > possible to assign more than 64 KiB if address space for IO.
> >
> > Currently controller has assigned 127 MiB + 64 KiB memory and therefore
> > there is 960 KiB of unused memory. So assign it to IO space by increasing
> > IO window from 64 KiB to 1 MiB.
> >
> > DTS file armada-3720-turris-mox.dts already uses whole 128 MiB space, so
> > only update comment about 32-bit IO space mapping.
> >
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Fixes: 514ef1e62d65 ("arm64: dts: marvell: armada-37xx: Extend PCIe MEM space")
> 
> I just saw this is the fixes pull request, and it seems very odd. Does this
> fix an actual bug? Note that Linux normally doesn't map more than 64KB
> of I/O space per PCI domain, so it should not make a difference to us.
> 
> Also, note that having a high bus address for the I/O space (0xefff0000,
> as as the CPU physical address here) means that a lot of the older
> devices that actually require I/O space won't work, because they need a
> low bus address in the first few KB.
> 
> Is this mapping a requirement from a broken bootloader, or can you change
> the mapping of the I/O port window in the physical space to the usual
> bus address 0?
> 
>         Arnd

As we discussed on IRC changing mapping of I/O to bus address 0x0 is
possible and together with recent changes of pci-aardvark.c driver
should work out-of-the box.

I sent patch which is changing I/O bus address to 0x0:
https://lore.kernel.org/linux-arm-kernel/20220218212526.16021-1-pali@kernel.org/

Please look at it.