diff mbox series

[v2] arm64: dts: marvell: armada-37xx: Remap IO space to bus address 0x0

Message ID 20220304163027.29357-1-pali@kernel.org (mailing list archive)
State New, archived
Headers show
Series [v2] arm64: dts: marvell: armada-37xx: Remap IO space to bus address 0x0 | expand

Commit Message

Pali Rohár March 4, 2022, 4:30 p.m. UTC
Legacy and old PCI I/O based cards do not support 32-bit I/O addressing.

Since commit 64f160e19e92 ("PCI: aardvark: Configure PCIe resources from
'ranges' DT property") kernel can set different PCIe address on CPU and
different on the bus for the one A37xx address mapping without any firmware
support in case the bus address does not conflict with other A37xx mapping.

So remap I/O space to the bus address 0x0 to enable support for old legacy
I/O port based cards which have hardcoded I/O ports in low address space.

Note that DDR on A37xx is mapped to bus address 0x0. And mapping of I/O
space can be set to address 0x0 too because MEM space and I/O space are
separate and so do not conflict.

Remapping IO space on Turris Mox to different address is not possible to
due bootloader bug.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reported-by: Arnd Bergmann <arnd@arndb.de>
Fixes: 76f6386b25cc ("arm64: dts: marvell: Add Aardvark PCIe support for Armada 3700")
Cc: stable@vger.kernel.org # 64f160e19e92 ("PCI: aardvark: Configure PCIe resources from 'ranges' DT property")
Cc: stable@vger.kernel.org # 514ef1e62d65 ("arm64: dts: marvell: armada-37xx: Extend PCIe MEM space")
Cc: stable@vger.kernel.org # ???????????? ("arm64: dts: marvell: armada-37xx: Increase PCIe IO size from 64 KiB to 1 MiB")

---
Changes in v2:
* Do not remap IO space on Turris Mox
---
 arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts | 7 ++++++-
 arch/arm64/boot/dts/marvell/armada-37xx.dtsi           | 2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)

Comments

Pali Rohár March 8, 2022, 11:41 a.m. UTC | #1
Hello! Could you review this change?

On Friday 04 March 2022 17:30:27 Pali Rohár wrote:
> Legacy and old PCI I/O based cards do not support 32-bit I/O addressing.
> 
> Since commit 64f160e19e92 ("PCI: aardvark: Configure PCIe resources from
> 'ranges' DT property") kernel can set different PCIe address on CPU and
> different on the bus for the one A37xx address mapping without any firmware
> support in case the bus address does not conflict with other A37xx mapping.
> 
> So remap I/O space to the bus address 0x0 to enable support for old legacy
> I/O port based cards which have hardcoded I/O ports in low address space.
> 
> Note that DDR on A37xx is mapped to bus address 0x0. And mapping of I/O
> space can be set to address 0x0 too because MEM space and I/O space are
> separate and so do not conflict.
> 
> Remapping IO space on Turris Mox to different address is not possible to
> due bootloader bug.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: 76f6386b25cc ("arm64: dts: marvell: Add Aardvark PCIe support for Armada 3700")
> Cc: stable@vger.kernel.org # 64f160e19e92 ("PCI: aardvark: Configure PCIe resources from 'ranges' DT property")
> Cc: stable@vger.kernel.org # 514ef1e62d65 ("arm64: dts: marvell: armada-37xx: Extend PCIe MEM space")
> Cc: stable@vger.kernel.org # ???????????? ("arm64: dts: marvell: armada-37xx: Increase PCIe IO size from 64 KiB to 1 MiB")
> 
> ---
> Changes in v2:
> * Do not remap IO space on Turris Mox
> ---
>  arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts | 7 ++++++-
>  arch/arm64/boot/dts/marvell/armada-37xx.dtsi           | 2 +-
>  2 files changed, 7 insertions(+), 2 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 6581092c2c90..2838e3f65ada 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> @@ -150,17 +150,22 @@
>  	slot-power-limit = <10000>;
>  	/*
>  	 * U-Boot port for Turris Mox has a bug which always expects that "ranges" DT property
>  	 * contains exactly 2 ranges with 3 (child) address cells, 2 (parent) address cells and
> -	 * 2 size cells and also expects that the second range starts at 16 MB offset. If these
> +	 * 2 size cells and also expects that the second range starts at 16 MB offset. Also it
> +	 * expects that first range uses same address for PCI (child) and CPU (parent) cells (so
> +	 * no remapping) and that this address is the lowest from all specified ranges. 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. 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
>  	 * https://source.denx.de/u-boot/u-boot/-/commit/c64ac3b3185aeb3846297ad7391fc6df8ecd73bf
>  	 * https://source.denx.de/u-boot/u-boot/-/commit/4a82fca8e330157081fc132a591ebd99ba02ee33
> +	 * Bug related to requirement of same child and parent addresses for first range is fixed
> +	 * in U-Boot version 2022.04 by following commit:
> +	 * https://source.denx.de/u-boot/u-boot/-/commit/1fd54253bca7d43d046bba4853fe5fafd034bc17
>  	 */
>  	#address-cells = <3>;
>  	#size-cells = <2>;
>  	ranges = <0x81000000 0 0xe8000000   0 0xe8000000   0 0x01000000   /* Port 0 IO */
> diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> index 549c3f7c5b27..a099b7787429 100644
> --- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> @@ -514,9 +514,9 @@
>  			 * 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 0xeff00000   0 0xeff00000   0 0x00100000>; /* Port 0 IO*/
> +				  0x81000000 0 0x00000000   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>,
>  					<0 0 0 3 &pcie_intc 2>,
> -- 
> 2.20.1
>
Gregory CLEMENT March 10, 2022, 10:05 a.m. UTC | #2
Hello Pali,

> Legacy and old PCI I/O based cards do not support 32-bit I/O addressing.
>
> Since commit 64f160e19e92 ("PCI: aardvark: Configure PCIe resources from
> 'ranges' DT property") kernel can set different PCIe address on CPU and
> different on the bus for the one A37xx address mapping without any firmware
> support in case the bus address does not conflict with other A37xx mapping.
>
> So remap I/O space to the bus address 0x0 to enable support for old legacy
> I/O port based cards which have hardcoded I/O ports in low address space.
>
> Note that DDR on A37xx is mapped to bus address 0x0. And mapping of I/O
> space can be set to address 0x0 too because MEM space and I/O space are
> separate and so do not conflict.
>
> Remapping IO space on Turris Mox to different address is not possible to
> due bootloader bug.
>
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Reported-by: Arnd Bergmann <arnd@arndb.de>
> Fixes: 76f6386b25cc ("arm64: dts: marvell: Add Aardvark PCIe support for Armada 3700")
> Cc: stable@vger.kernel.org # 64f160e19e92 ("PCI: aardvark: Configure PCIe resources from 'ranges' DT property")
> Cc: stable@vger.kernel.org # 514ef1e62d65 ("arm64: dts: marvell: armada-37xx: Extend PCIe MEM space")
>
Cc: stable@vger.kernel.org # ???????????? ("arm64: dts: marvell: armada-37xx: Increase PCIe IO size from 64 KiB to 1 MiB")

This patch has been refused by Arnd so I removed it from the mvebu/fixes
branch so you should not apply anything on top of it.

Actually I still try to first apply the old patch and then this one but
it still fail. And it is also failed when I applied this one on a
v5.17-rc1, so I wondered on which did create this patch.

Grégory

> ---
> Changes in v2:
> * Do not remap IO space on Turris Mox
> ---
>  arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts | 7 ++++++-
>  arch/arm64/boot/dts/marvell/armada-37xx.dtsi           | 2 +-
>  2 files changed, 7 insertions(+), 2 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 6581092c2c90..2838e3f65ada 100644
> --- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> +++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> @@ -150,17 +150,22 @@
>  	slot-power-limit = <10000>;
>  	/*
>  	 * U-Boot port for Turris Mox has a bug which always expects that "ranges" DT property
>  	 * contains exactly 2 ranges with 3 (child) address cells, 2 (parent) address cells and
> -	 * 2 size cells and also expects that the second range starts at 16 MB offset. If these
> +	 * 2 size cells and also expects that the second range starts at 16 MB offset. Also it
> +	 * expects that first range uses same address for PCI (child) and CPU (parent) cells (so
> +	 * no remapping) and that this address is the lowest from all specified ranges. 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. 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
>  	 * https://source.denx.de/u-boot/u-boot/-/commit/c64ac3b3185aeb3846297ad7391fc6df8ecd73bf
>  	 * https://source.denx.de/u-boot/u-boot/-/commit/4a82fca8e330157081fc132a591ebd99ba02ee33
> +	 * Bug related to requirement of same child and parent addresses for first range is fixed
> +	 * in U-Boot version 2022.04 by following commit:
> +	 * https://source.denx.de/u-boot/u-boot/-/commit/1fd54253bca7d43d046bba4853fe5fafd034bc17
>  	 */
>  	#address-cells = <3>;
>  	#size-cells = <2>;
>  	ranges = <0x81000000 0 0xe8000000   0 0xe8000000   0 0x01000000   /* Port 0 IO */
> diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> index 549c3f7c5b27..a099b7787429 100644
> --- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> +++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> @@ -514,9 +514,9 @@
>  			 * 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 0xeff00000   0 0xeff00000   0 0x00100000>; /* Port 0 IO*/
> +				  0x81000000 0 0x00000000   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>,
>  					<0 0 0 3 &pcie_intc 2>,
> -- 
> 2.20.1
>
Pali Rohár March 10, 2022, 10:09 a.m. UTC | #3
On Thursday 10 March 2022 11:05:00 Gregory CLEMENT wrote:
> Hello Pali,
> 
> > Legacy and old PCI I/O based cards do not support 32-bit I/O addressing.
> >
> > Since commit 64f160e19e92 ("PCI: aardvark: Configure PCIe resources from
> > 'ranges' DT property") kernel can set different PCIe address on CPU and
> > different on the bus for the one A37xx address mapping without any firmware
> > support in case the bus address does not conflict with other A37xx mapping.
> >
> > So remap I/O space to the bus address 0x0 to enable support for old legacy
> > I/O port based cards which have hardcoded I/O ports in low address space.
> >
> > Note that DDR on A37xx is mapped to bus address 0x0. And mapping of I/O
> > space can be set to address 0x0 too because MEM space and I/O space are
> > separate and so do not conflict.
> >
> > Remapping IO space on Turris Mox to different address is not possible to
> > due bootloader bug.
> >
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Reported-by: Arnd Bergmann <arnd@arndb.de>
> > Fixes: 76f6386b25cc ("arm64: dts: marvell: Add Aardvark PCIe support for Armada 3700")
> > Cc: stable@vger.kernel.org # 64f160e19e92 ("PCI: aardvark: Configure PCIe resources from 'ranges' DT property")
> > Cc: stable@vger.kernel.org # 514ef1e62d65 ("arm64: dts: marvell: armada-37xx: Extend PCIe MEM space")
> >
> Cc: stable@vger.kernel.org # ???????????? ("arm64: dts: marvell: armada-37xx: Increase PCIe IO size from 64 KiB to 1 MiB")
> 
> This patch has been refused by Arnd so I removed it from the mvebu/fixes
> branch so you should not apply anything on top of it.

Ok, so what is wrong with a change which increase size of IO space to 1 MB?

> Actually I still try to first apply the old patch and then this one but
> it still fail. And it is also failed when I applied this one on a
> v5.17-rc1, so I wondered on which did create this patch.

Ok, at which branch / commit should I rebase it?

> Grégory
> 
> > ---
> > Changes in v2:
> > * Do not remap IO space on Turris Mox
> > ---
> >  arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts | 7 ++++++-
> >  arch/arm64/boot/dts/marvell/armada-37xx.dtsi           | 2 +-
> >  2 files changed, 7 insertions(+), 2 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 6581092c2c90..2838e3f65ada 100644
> > --- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> > +++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> > @@ -150,17 +150,22 @@
> >  	slot-power-limit = <10000>;
> >  	/*
> >  	 * U-Boot port for Turris Mox has a bug which always expects that "ranges" DT property
> >  	 * contains exactly 2 ranges with 3 (child) address cells, 2 (parent) address cells and
> > -	 * 2 size cells and also expects that the second range starts at 16 MB offset. If these
> > +	 * 2 size cells and also expects that the second range starts at 16 MB offset. Also it
> > +	 * expects that first range uses same address for PCI (child) and CPU (parent) cells (so
> > +	 * no remapping) and that this address is the lowest from all specified ranges. 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. 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
> >  	 * https://source.denx.de/u-boot/u-boot/-/commit/c64ac3b3185aeb3846297ad7391fc6df8ecd73bf
> >  	 * https://source.denx.de/u-boot/u-boot/-/commit/4a82fca8e330157081fc132a591ebd99ba02ee33
> > +	 * Bug related to requirement of same child and parent addresses for first range is fixed
> > +	 * in U-Boot version 2022.04 by following commit:
> > +	 * https://source.denx.de/u-boot/u-boot/-/commit/1fd54253bca7d43d046bba4853fe5fafd034bc17
> >  	 */
> >  	#address-cells = <3>;
> >  	#size-cells = <2>;
> >  	ranges = <0x81000000 0 0xe8000000   0 0xe8000000   0 0x01000000   /* Port 0 IO */
> > diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> > index 549c3f7c5b27..a099b7787429 100644
> > --- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> > +++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> > @@ -514,9 +514,9 @@
> >  			 * 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 0xeff00000   0 0xeff00000   0 0x00100000>; /* Port 0 IO*/
> > +				  0x81000000 0 0x00000000   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>,
> >  					<0 0 0 3 &pcie_intc 2>,
> > -- 
> > 2.20.1
> >
> 
> -- 
> Gregory Clement, Bootlin
> Embedded Linux and Kernel engineering
> http://bootlin.com
Arnd Bergmann March 10, 2022, 10:22 a.m. UTC | #4
On Thu, Mar 10, 2022 at 11:09 AM Pali Rohár <pali@kernel.org> wrote:
> On Thursday 10 March 2022 11:05:00 Gregory CLEMENT wrote:

> >
> > This patch has been refused by Arnd so I removed it from the mvebu/fixes
> > branch so you should not apply anything on top of it.
>
> Ok, so what is wrong with a change which increase size of IO space to 1 MB?

It should not cause any harm, but there is really no point in this if no known
devices use more than a few bytes, and Linux only maps the first 64KB of
the I/O space for each host bridge. I don't actually see where we limit the
size to 64KB, so maybe that changed recently.

        Arnd
Gregory CLEMENT March 10, 2022, 10:23 a.m. UTC | #5
Pali Rohár <pali@kernel.org> writes:

> On Thursday 10 March 2022 11:05:00 Gregory CLEMENT wrote:
>> Hello Pali,
>> 
>> > Legacy and old PCI I/O based cards do not support 32-bit I/O addressing.
>> >
>> > Since commit 64f160e19e92 ("PCI: aardvark: Configure PCIe resources from
>> > 'ranges' DT property") kernel can set different PCIe address on CPU and
>> > different on the bus for the one A37xx address mapping without any firmware
>> > support in case the bus address does not conflict with other A37xx mapping.
>> >
>> > So remap I/O space to the bus address 0x0 to enable support for old legacy
>> > I/O port based cards which have hardcoded I/O ports in low address space.
>> >
>> > Note that DDR on A37xx is mapped to bus address 0x0. And mapping of I/O
>> > space can be set to address 0x0 too because MEM space and I/O space are
>> > separate and so do not conflict.
>> >
>> > Remapping IO space on Turris Mox to different address is not possible to
>> > due bootloader bug.
>> >
>> > Signed-off-by: Pali Rohár <pali@kernel.org>
>> > Reported-by: Arnd Bergmann <arnd@arndb.de>
>> > Fixes: 76f6386b25cc ("arm64: dts: marvell: Add Aardvark PCIe support for Armada 3700")
>> > Cc: stable@vger.kernel.org # 64f160e19e92 ("PCI: aardvark: Configure PCIe resources from 'ranges' DT property")
>> > Cc: stable@vger.kernel.org # 514ef1e62d65 ("arm64: dts: marvell: armada-37xx: Extend PCIe MEM space")
>> >
>> Cc: stable@vger.kernel.org # ???????????? ("arm64: dts: marvell: armada-37xx: Increase PCIe IO size from 64 KiB to 1 MiB")
>> 
>> This patch has been refused by Arnd so I removed it from the mvebu/fixes
>> branch so you should not apply anything on top of it.
>
> Ok, so what is wrong with a change which increase size of IO space to 1 MB?

https://lore.kernel.org/linux-arm-kernel/CAK8P3a2D8Yv+KpM4NJyP9mosieqbhHh08=mdEy+OA84Vx6FVCQ@mail.gmail.com/

>
>> Actually I still try to first apply the old patch and then this one but
>> it still fail. And it is also failed when I applied this one on a
>> v5.17-rc1, so I wondered on which did create this patch.
>
> Ok, at which branch / commit should I rebase it?

Please create only one single patch on top of v5.17-rc1.

Thanks!

Gregory

>
>> Grégory
>> 
>> > ---
>> > Changes in v2:
>> > * Do not remap IO space on Turris Mox
>> > ---
>> >  arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts | 7 ++++++-
>> >  arch/arm64/boot/dts/marvell/armada-37xx.dtsi           | 2 +-
>> >  2 files changed, 7 insertions(+), 2 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 6581092c2c90..2838e3f65ada 100644
>> > --- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
>> > +++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
>> > @@ -150,17 +150,22 @@
>> >  	slot-power-limit = <10000>;
>> >  	/*
>> >  	 * U-Boot port for Turris Mox has a bug which always expects that "ranges" DT property
>> >  	 * contains exactly 2 ranges with 3 (child) address cells, 2 (parent) address cells and
>> > -	 * 2 size cells and also expects that the second range starts at 16 MB offset. If these
>> > +	 * 2 size cells and also expects that the second range starts at 16 MB offset. Also it
>> > +	 * expects that first range uses same address for PCI (child) and CPU (parent) cells (so
>> > +	 * no remapping) and that this address is the lowest from all specified ranges. 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. 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
>> >  	 * https://source.denx.de/u-boot/u-boot/-/commit/c64ac3b3185aeb3846297ad7391fc6df8ecd73bf
>> >  	 * https://source.denx.de/u-boot/u-boot/-/commit/4a82fca8e330157081fc132a591ebd99ba02ee33
>> > +	 * Bug related to requirement of same child and parent addresses for first range is fixed
>> > +	 * in U-Boot version 2022.04 by following commit:
>> > +	 * https://source.denx.de/u-boot/u-boot/-/commit/1fd54253bca7d43d046bba4853fe5fafd034bc17
>> >  	 */
>> >  	#address-cells = <3>;
>> >  	#size-cells = <2>;
>> >  	ranges = <0x81000000 0 0xe8000000   0 0xe8000000   0 0x01000000   /* Port 0 IO */
>> > diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
>> > index 549c3f7c5b27..a099b7787429 100644
>> > --- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
>> > +++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
>> > @@ -514,9 +514,9 @@
>> >  			 * 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 0xeff00000   0 0xeff00000   0 0x00100000>; /* Port 0 IO*/
>> > +				  0x81000000 0 0x00000000   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>,
>> >  					<0 0 0 3 &pcie_intc 2>,
>> > -- 
>> > 2.20.1
>> >
>> 
>> -- 
>> Gregory Clement, Bootlin
>> Embedded Linux and Kernel engineering
>> http://bootlin.com
Pali Rohár March 10, 2022, 10:41 a.m. UTC | #6
On Thursday 10 March 2022 11:22:38 Arnd Bergmann wrote:
> On Thu, Mar 10, 2022 at 11:09 AM Pali Rohár <pali@kernel.org> wrote:
> > On Thursday 10 March 2022 11:05:00 Gregory CLEMENT wrote:
> 
> > >
> > > This patch has been refused by Arnd so I removed it from the mvebu/fixes
> > > branch so you should not apply anything on top of it.
> >
> > Ok, so what is wrong with a change which increase size of IO space to 1 MB?
> 
> It should not cause any harm, but there is really no point in this if no known
> devices use more than a few bytes, and Linux only maps the first 64KB of
> the I/O space for each host bridge. I don't actually see where we limit the
> size to 64KB, so maybe that changed recently.
> 
>         Arnd

Ok. Anyway, I was told that DTS should describe HW properties and not to
be bound with SW implementation or SW limitations, like case here if
Linux SW limits some sizes.
Pali Rohár March 10, 2022, 10:47 a.m. UTC | #7
On Thursday 10 March 2022 11:23:26 Gregory CLEMENT wrote:
> Pali Rohár <pali@kernel.org> writes:
> 
> > On Thursday 10 March 2022 11:05:00 Gregory CLEMENT wrote:
> >> Hello Pali,
> >> 
> >> > Legacy and old PCI I/O based cards do not support 32-bit I/O addressing.
> >> >
> >> > Since commit 64f160e19e92 ("PCI: aardvark: Configure PCIe resources from
> >> > 'ranges' DT property") kernel can set different PCIe address on CPU and
> >> > different on the bus for the one A37xx address mapping without any firmware
> >> > support in case the bus address does not conflict with other A37xx mapping.
> >> >
> >> > So remap I/O space to the bus address 0x0 to enable support for old legacy
> >> > I/O port based cards which have hardcoded I/O ports in low address space.
> >> >
> >> > Note that DDR on A37xx is mapped to bus address 0x0. And mapping of I/O
> >> > space can be set to address 0x0 too because MEM space and I/O space are
> >> > separate and so do not conflict.
> >> >
> >> > Remapping IO space on Turris Mox to different address is not possible to
> >> > due bootloader bug.
> >> >
> >> > Signed-off-by: Pali Rohár <pali@kernel.org>
> >> > Reported-by: Arnd Bergmann <arnd@arndb.de>
> >> > Fixes: 76f6386b25cc ("arm64: dts: marvell: Add Aardvark PCIe support for Armada 3700")
> >> > Cc: stable@vger.kernel.org # 64f160e19e92 ("PCI: aardvark: Configure PCIe resources from 'ranges' DT property")
> >> > Cc: stable@vger.kernel.org # 514ef1e62d65 ("arm64: dts: marvell: armada-37xx: Extend PCIe MEM space")
> >> >
> >> Cc: stable@vger.kernel.org # ???????????? ("arm64: dts: marvell: armada-37xx: Increase PCIe IO size from 64 KiB to 1 MiB")
> >> 
> >> This patch has been refused by Arnd so I removed it from the mvebu/fixes
> >> branch so you should not apply anything on top of it.
> >
> > Ok, so what is wrong with a change which increase size of IO space to 1 MB?
> 
> https://lore.kernel.org/linux-arm-kernel/CAK8P3a2D8Yv+KpM4NJyP9mosieqbhHh08=mdEy+OA84Vx6FVCQ@mail.gmail.com/

The whole discussion was about remapping IO to 0 and this is addressed
in this patch. Therefore I thought that it is resolved...

Now I see that Arnd wrote also about size increasing, so some
misunderstanding from my side.

> >
> >> Actually I still try to first apply the old patch and then this one but
> >> it still fail. And it is also failed when I applied this one on a
> >> v5.17-rc1, so I wondered on which did create this patch.
> >
> > Ok, at which branch / commit should I rebase it?
> 
> Please create only one single patch on top of v5.17-rc1.

Done in v3. Hopefully now it is correct.

> Thanks!
> 
> Gregory
> 
> >
> >> Grégory
> >> 
> >> > ---
> >> > Changes in v2:
> >> > * Do not remap IO space on Turris Mox
> >> > ---
> >> >  arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts | 7 ++++++-
> >> >  arch/arm64/boot/dts/marvell/armada-37xx.dtsi           | 2 +-
> >> >  2 files changed, 7 insertions(+), 2 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 6581092c2c90..2838e3f65ada 100644
> >> > --- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> >> > +++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
> >> > @@ -150,17 +150,22 @@
> >> >  	slot-power-limit = <10000>;
> >> >  	/*
> >> >  	 * U-Boot port for Turris Mox has a bug which always expects that "ranges" DT property
> >> >  	 * contains exactly 2 ranges with 3 (child) address cells, 2 (parent) address cells and
> >> > -	 * 2 size cells and also expects that the second range starts at 16 MB offset. If these
> >> > +	 * 2 size cells and also expects that the second range starts at 16 MB offset. Also it
> >> > +	 * expects that first range uses same address for PCI (child) and CPU (parent) cells (so
> >> > +	 * no remapping) and that this address is the lowest from all specified ranges. 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. 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
> >> >  	 * https://source.denx.de/u-boot/u-boot/-/commit/c64ac3b3185aeb3846297ad7391fc6df8ecd73bf
> >> >  	 * https://source.denx.de/u-boot/u-boot/-/commit/4a82fca8e330157081fc132a591ebd99ba02ee33
> >> > +	 * Bug related to requirement of same child and parent addresses for first range is fixed
> >> > +	 * in U-Boot version 2022.04 by following commit:
> >> > +	 * https://source.denx.de/u-boot/u-boot/-/commit/1fd54253bca7d43d046bba4853fe5fafd034bc17
> >> >  	 */
> >> >  	#address-cells = <3>;
> >> >  	#size-cells = <2>;
> >> >  	ranges = <0x81000000 0 0xe8000000   0 0xe8000000   0 0x01000000   /* Port 0 IO */
> >> > diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> >> > index 549c3f7c5b27..a099b7787429 100644
> >> > --- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> >> > +++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
> >> > @@ -514,9 +514,9 @@
> >> >  			 * 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 0xeff00000   0 0xeff00000   0 0x00100000>; /* Port 0 IO*/
> >> > +				  0x81000000 0 0x00000000   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>,
> >> >  					<0 0 0 3 &pcie_intc 2>,
> >> > -- 
> >> > 2.20.1
> >> >
> >> 
> >> -- 
> >> Gregory Clement, Bootlin
> >> Embedded Linux and Kernel engineering
> >> http://bootlin.com
> 
> -- 
> Gregory Clement, Bootlin
> Embedded Linux and Kernel engineering
> http://bootlin.com
diff mbox series

Patch

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 6581092c2c90..2838e3f65ada 100644
--- a/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
+++ b/arch/arm64/boot/dts/marvell/armada-3720-turris-mox.dts
@@ -150,17 +150,22 @@ 
 	slot-power-limit = <10000>;
 	/*
 	 * U-Boot port for Turris Mox has a bug which always expects that "ranges" DT property
 	 * contains exactly 2 ranges with 3 (child) address cells, 2 (parent) address cells and
-	 * 2 size cells and also expects that the second range starts at 16 MB offset. If these
+	 * 2 size cells and also expects that the second range starts at 16 MB offset. Also it
+	 * expects that first range uses same address for PCI (child) and CPU (parent) cells (so
+	 * no remapping) and that this address is the lowest from all specified ranges. 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. 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
 	 * https://source.denx.de/u-boot/u-boot/-/commit/c64ac3b3185aeb3846297ad7391fc6df8ecd73bf
 	 * https://source.denx.de/u-boot/u-boot/-/commit/4a82fca8e330157081fc132a591ebd99ba02ee33
+	 * Bug related to requirement of same child and parent addresses for first range is fixed
+	 * in U-Boot version 2022.04 by following commit:
+	 * https://source.denx.de/u-boot/u-boot/-/commit/1fd54253bca7d43d046bba4853fe5fafd034bc17
 	 */
 	#address-cells = <3>;
 	#size-cells = <2>;
 	ranges = <0x81000000 0 0xe8000000   0 0xe8000000   0 0x01000000   /* Port 0 IO */
diff --git a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
index 549c3f7c5b27..a099b7787429 100644
--- a/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
+++ b/arch/arm64/boot/dts/marvell/armada-37xx.dtsi
@@ -514,9 +514,9 @@ 
 			 * 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 0xeff00000   0 0xeff00000   0 0x00100000>; /* Port 0 IO*/
+				  0x81000000 0 0x00000000   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>,
 					<0 0 0 3 &pcie_intc 2>,