[PATCH u-boot-marvell v2] arm: mvebu: turris_omnia: Fixup SATA or PCIe nodes at runtime in DT blob

[PATCH u-boot-marvell v2] arm: mvebu: turris_omnia: Fixup SATA or PCIe nodes at runtime in DT blob

[Marek Behún]

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

On of the MiniPCIe ports on Turris Omnia is also a mSATA port. Whether
it works in SATA or PCIe mode is determined by a strapping pin, which
value is read from the MCU.

We already determine which type of card is connected when configuring
SerDeses.

But until now we left both SATA and PCIe port 0 nodes in device tree
enabled, and so the SATA driver is probed in U-Boot / Linux even if we
know there is no mSATA card, and similarly PCIe driver tries to link on
port 0 even if we know there is mSATA card, not a PCIe card.

Fixup device tree blob to disable SATA node if mSATA card is not
present, and to disable PCIe port 0 node if mSATA card is present.

Do this for U-Boot's DT blob before relocation and also for kernel DT
blob before booting.

This ensures that software does not try to use SATA or PCIe HW when
corresponding PHY is not configured.

Signed-off-by: Pali Rohár <pali@kernel.org>
[ refactored and fixed some issues ]
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
Sorry about this, there is a bug in v1 which got there when I was
testing this.

This depends on patch
  fdt_support: Add fdt_for_each_node_by_compatible() helper macro
which I recently sent to mailing list:
  http://patchwork.ozlabs.org/project/uboot/patch/20220107101329.25836-1-kabel@kernel.org/

Stefan, the dependency touches DM stuff, so I added u-boot-dm tag, but
it currently applies only to marvell/next. Could we sync with Simon and
get both merged via marvell/next? Or do we wait till it gets merged
there, then into master, then into marvell?
---
 board/CZ.NIC/turris_omnia/turris_omnia.c | 91 +++++++++++++++++++++++-
 configs/turris_omnia_defconfig           |  1 +
 2 files changed, 90 insertions(+), 2 deletions(-)

diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c
index ae24d14b76..80235e8b66 100644
--- a/board/CZ.NIC/turris_omnia/turris_omnia.c
+++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
@@ -490,6 +490,86 @@ void spl_board_init(void)
 	}
 }
 
+#if IS_ENABLED(CONFIG_OF_BOARD_FIXUP) || IS_ENABLED(CONFIG_OF_BOARD_SETUP)
+
+static void fixup_serdes_0_nodes(void *blob)
+{
+	bool mode_sata;
+	int node;
+
+	/*
+	 * Determine if SerDes 0 is configured to SATA mode.
+	 * We do this instead of calling omnia_detect_sata() to avoid another
+	 * call to the MCU. By this time the common PHYs are initialized (it is
+	 * done in SPL), so we can read this common PHY register.
+	 */
+	mode_sata = (readl(MVEBU_REGISTER(0x183fc)) & GENMASK(3, 0)) == 2;
+
+	/*
+	 * We're either adding status = "disabled" property, or changing
+	 * status = "okay" to status = "disabled". In both cases we'll need more
+	 * space. Increase the size a little.
+	 */
+	if (fdt_increase_size(blob, 32) < 0) {
+		printf("Cannot increase FDT size!\n");
+		return;
+	}
+
+	/* If mSATA card is not present, disable SATA DT node */
+	if (!mode_sata) {
+		fdt_for_each_node_by_compatible (node, blob, -1,
+						 "marvell,armada-380-ahci") {
+			if (!fdtdec_get_is_enabled(blob, node))
+				continue;
+
+			if (fdt_status_disabled(blob, node) < 0)
+				printf("Cannot disable SATA DT node!\n");
+			else
+				debug("Disabled SATA DT node\n");
+
+			break;
+		}
+
+		return;
+	}
+
+	/* Otherwise disable PCIe port 0 DT node (MiniPCIe / mSATA port) */
+	fdt_for_each_node_by_compatible (node, blob, -1,
+					 "marvell,armada-370-pcie") {
+		int port;
+
+		if (!fdtdec_get_is_enabled(blob, node))
+			continue;
+
+		fdt_for_each_subnode (port, blob, node) {
+			if (!fdtdec_get_is_enabled(blob, port))
+				continue;
+
+			if (fdtdec_get_int(blob, port, "marvell,pcie-port",
+					   -1) != 0)
+				continue;
+
+			if (fdt_status_disabled(blob, port) < 0)
+				printf("Cannot disable PCIe port 0 DT node!\n");
+			else
+				debug("Disabled PCIe port 0 DT node\n");
+
+			return;
+		}
+	}
+}
+
+#endif
+
+#if IS_ENABLED(CONFIG_OF_BOARD_FIXUP)
+int board_fix_fdt(void *blob)
+{
+	fixup_serdes_0_nodes(blob);
+
+	return 0;
+}
+#endif
+
 int board_init(void)
 {
 	/* address of boot parameters */
@@ -694,7 +774,7 @@ static bool fixup_mtd_partitions(void *blob, int offset, struct mtd_info *mtd)
 	return true;
 }
 
-int ft_board_setup(void *blob, struct bd_info *bd)
+static void fixup_spi_nor_partitions(void *blob)
 {
 	struct mtd_info *mtd;
 	int node;
@@ -711,12 +791,19 @@ int ft_board_setup(void *blob, struct bd_info *bd)
 		goto fail;
 
 	put_mtd_device(mtd);
-	return 0;
+	return;
 
 fail:
 	printf("Failed fixing SPI NOR partitions!\n");
 	if (!IS_ERR_OR_NULL(mtd))
 		put_mtd_device(mtd);
+}
+
+int ft_board_setup(void *blob, struct bd_info *bd)
+{
+	fixup_spi_nor_partitions(blob);
+	fixup_serdes_0_nodes(blob);
+
 	return 0;
 }
 #endif
diff --git a/configs/turris_omnia_defconfig b/configs/turris_omnia_defconfig
index e6f901ea77..6b1081c9bd 100644
--- a/configs/turris_omnia_defconfig
+++ b/configs/turris_omnia_defconfig
@@ -23,6 +23,7 @@ CONFIG_DEBUG_UART_BASE=0xd0012000
 CONFIG_DEBUG_UART_CLOCK=250000000
 CONFIG_DEBUG_UART=y
 CONFIG_AHCI=y
+CONFIG_OF_BOARD_FIXUP=y
 CONFIG_DISTRO_DEFAULTS=y
 CONFIG_SYS_LOAD_ADDR=0x800000
 CONFIG_FIT=y
-- 
2.34.1

Re: [PATCH u-boot-marvell v2] arm: mvebu: turris_omnia: Fixup SATA or PCIe nodes at runtime in DT blob

[Stefan Roese]

A few minor comments below...

On 1/7/22 13:53, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> On of the MiniPCIe ports on Turris Omnia is also a mSATA port. Whether
> it works in SATA or PCIe mode is determined by a strapping pin, which
> value is read from the MCU.
> 
> We already determine which type of card is connected when configuring
> SerDeses.
> 
> But until now we left both SATA and PCIe port 0 nodes in device tree
> enabled, and so the SATA driver is probed in U-Boot / Linux even if we
> know there is no mSATA card, and similarly PCIe driver tries to link on
> port 0 even if we know there is mSATA card, not a PCIe card.
> 
> Fixup device tree blob to disable SATA node if mSATA card is not
> present, and to disable PCIe port 0 node if mSATA card is present.
> 
> Do this for U-Boot's DT blob before relocation and also for kernel DT
> blob before booting.
> 
> This ensures that software does not try to use SATA or PCIe HW when
> corresponding PHY is not configured.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> [ refactored and fixed some issues ]
> Signed-off-by: Marek Behún <marek.behun@nic.cz>
> ---
> Sorry about this, there is a bug in v1 which got there when I was
> testing this.
> 
> This depends on patch
>    fdt_support: Add fdt_for_each_node_by_compatible() helper macro
> which I recently sent to mailing list:
>    http://patchwork.ozlabs.org/project/uboot/patch/20220107101329.25836-1-kabel@kernel.org/
> 
> Stefan, the dependency touches DM stuff, so I added u-boot-dm tag, but
> it currently applies only to marvell/next. Could we sync with Simon and
> get both merged via marvell/next? Or do we wait till it gets merged
> there, then into master, then into marvell?
> ---
>   board/CZ.NIC/turris_omnia/turris_omnia.c | 91 +++++++++++++++++++++++-
>   configs/turris_omnia_defconfig           |  1 +
>   2 files changed, 90 insertions(+), 2 deletions(-)
> 
> diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c
> index ae24d14b76..80235e8b66 100644
> --- a/board/CZ.NIC/turris_omnia/turris_omnia.c
> +++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
> @@ -490,6 +490,86 @@ void spl_board_init(void)
>   	}
>   }
>   
> +#if IS_ENABLED(CONFIG_OF_BOARD_FIXUP) || IS_ENABLED(CONFIG_OF_BOARD_SETUP)
> +
> +static void fixup_serdes_0_nodes(void *blob)
> +{
> +	bool mode_sata;
> +	int node;
> +
> +	/*
> +	 * Determine if SerDes 0 is configured to SATA mode.
> +	 * We do this instead of calling omnia_detect_sata() to avoid another
> +	 * call to the MCU. By this time the common PHYs are initialized (it is
> +	 * done in SPL), so we can read this common PHY register.
> +	 */
> +	mode_sata = (readl(MVEBU_REGISTER(0x183fc)) & GENMASK(3, 0)) == 2;
> +
> +	/*
> +	 * We're either adding status = "disabled" property, or changing
> +	 * status = "okay" to status = "disabled". In both cases we'll need more
> +	 * space. Increase the size a little.
> +	 */
> +	if (fdt_increase_size(blob, 32) < 0) {
> +		printf("Cannot increase FDT size!\n");
> +		return;
> +	}
> +
> +	/* If mSATA card is not present, disable SATA DT node */
> +	if (!mode_sata) {
> +		fdt_for_each_node_by_compatible (node, blob, -1,
> +						 "marvell,armada-380-ahci") {

Why are adding the space before the "(" here? This does not seem the
common and preferred coding style AFAICT.

> +			if (!fdtdec_get_is_enabled(blob, node))
> +				continue;
> +
> +			if (fdt_status_disabled(blob, node) < 0)
> +				printf("Cannot disable SATA DT node!\n");
> +			else
> +				debug("Disabled SATA DT node\n");
> +
> +			break;
> +		}
> +
> +		return;
> +	}
> +
> +	/* Otherwise disable PCIe port 0 DT node (MiniPCIe / mSATA port) */
> +	fdt_for_each_node_by_compatible (node, blob, -1,
> +					 "marvell,armada-370-pcie") {

Same here.

> +		int port;
> +
> +		if (!fdtdec_get_is_enabled(blob, node))
> +			continue;
> +
> +		fdt_for_each_subnode (port, blob, node) {
> +			if (!fdtdec_get_is_enabled(blob, port))
> +				continue;
> +
> +			if (fdtdec_get_int(blob, port, "marvell,pcie-port",
> +					   -1) != 0)
> +				continue;
> +
> +			if (fdt_status_disabled(blob, port) < 0)
> +				printf("Cannot disable PCIe port 0 DT node!\n");
> +			else
> +				debug("Disabled PCIe port 0 DT node\n");
> +
> +			return;
> +		}
> +	}
> +}
> +
> +#endif
> +
> +#if IS_ENABLED(CONFIG_OF_BOARD_FIXUP)
> +int board_fix_fdt(void *blob)
> +{
> +	fixup_serdes_0_nodes(blob);
> +
> +	return 0;
> +}
> +#endif
> +
>   int board_init(void)
>   {
>   	/* address of boot parameters */
> @@ -694,7 +774,7 @@ static bool fixup_mtd_partitions(void *blob, int offset, struct mtd_info *mtd)
>   	return true;
>   }
>   
> -int ft_board_setup(void *blob, struct bd_info *bd)
> +static void fixup_spi_nor_partitions(void *blob)
>   {
>   	struct mtd_info *mtd;
>   	int node;
> @@ -711,12 +791,19 @@ int ft_board_setup(void *blob, struct bd_info *bd)
>   		goto fail;
>   
>   	put_mtd_device(mtd);
> -	return 0;
> +	return;
>   
>   fail:
>   	printf("Failed fixing SPI NOR partitions!\n");
>   	if (!IS_ERR_OR_NULL(mtd))
>   		put_mtd_device(mtd);
> +}
> +
> +int ft_board_setup(void *blob, struct bd_info *bd)
> +{
> +	fixup_spi_nor_partitions(blob);
> +	fixup_serdes_0_nodes(blob);
> +
>   	return 0;
>   }
>   #endif
> diff --git a/configs/turris_omnia_defconfig b/configs/turris_omnia_defconfig
> index e6f901ea77..6b1081c9bd 100644
> --- a/configs/turris_omnia_defconfig
> +++ b/configs/turris_omnia_defconfig
> @@ -23,6 +23,7 @@ CONFIG_DEBUG_UART_BASE=0xd0012000
>   CONFIG_DEBUG_UART_CLOCK=250000000
>   CONFIG_DEBUG_UART=y
>   CONFIG_AHCI=y
> +CONFIG_OF_BOARD_FIXUP=y
>   CONFIG_DISTRO_DEFAULTS=y
>   CONFIG_SYS_LOAD_ADDR=0x800000
>   CONFIG_FIT=y
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell v2] arm: mvebu: turris_omnia: Fixup SATA or PCIe nodes at runtime in DT blob

[Pali Rohár]

On Monday 10 January 2022 08:43:17 Stefan Roese wrote:
> > +	/* If mSATA card is not present, disable SATA DT node */
> > +	if (!mode_sata) {
> > +		fdt_for_each_node_by_compatible (node, blob, -1,
> > +						 "marvell,armada-380-ahci") {
> 
> Why are adding the space before the "(" here? This does not seem the
> common and preferred coding style AFAICT.

Hello Stefan! Just to note that I'm adding this space too when writing
for-loops. E.g.:

for (i = 0; i < 10; i++) {
...
}

or:

fdt_for_each_node_by_compatible (node, blob, -1, compatible) {
...
}

> > +			if (!fdtdec_get_is_enabled(blob, node))
> > +				continue;
> > +
> > +			if (fdt_status_disabled(blob, node) < 0)
> > +				printf("Cannot disable SATA DT node!\n");
> > +			else
> > +				debug("Disabled SATA DT node\n");
> > +
> > +			break;
> > +		}
> > +
> > +		return;
> > +	}

Re: [PATCH u-boot-marvell v2] arm: mvebu: turris_omnia: Fixup SATA or PCIe nodes at runtime in DT blob

[Stefan Roese]

On 1/10/22 10:06, Pali Rohár wrote:
> On Monday 10 January 2022 08:43:17 Stefan Roese wrote:
>>> +	/* If mSATA card is not present, disable SATA DT node */
>>> +	if (!mode_sata) {
>>> +		fdt_for_each_node_by_compatible (node, blob, -1,
>>> +						 "marvell,armada-380-ahci") {
>>
>> Why are adding the space before the "(" here? This does not seem the
>> common and preferred coding style AFAICT.
> 
> Hello Stefan! Just to note that I'm adding this space too when writing
> for-loops. E.g.:
> 
> for (i = 0; i < 10; i++) {
> ...
> }
> 
> or:
> 
> fdt_for_each_node_by_compatible (node, blob, -1, compatible) {
> ...
> }

Right. But we already have multiple of these for () helpers in U-Boot
and Linux. And AFACIT all are used without this space:

[stefan@ryzen u-boot (master)]$ git grep for_each drivers/
drivers/block/blkcache.c:       list_for_each_entry(node, &block_cache, lh)
drivers/block/blkcache.c:       list_for_each_safe(entry, n, &block_cache) {
drivers/button/button-adc.c:    dev_for_each_subnode(node, dev->parent) {
drivers/button/button-adc.c:    dev_for_each_subnode(node, parent) {
drivers/button/button-gpio.c:   dev_for_each_subnode(node, parent) {
drivers/clk/clk-uclass.c:       list_for_each_entry(child_dev, 
&clk->dev->child_head, sibling_node) {
drivers/clk/clk_stm32mp1.c:     ofnode_for_each_subnode(subnode, node) {
drivers/core/device-remove.c:   list_for_each_entry_safe(pos, n, 
&dev->child_head, sibling_node) {
drivers/core/device-remove.c:   list_for_each_entry_safe(pos, n, 
&dev->child_head, sibling_node) {
drivers/core/device.c:  list_for_each_entry_safe(pos, n, 
&dev->parent->child_head,
drivers/core/device.c:  list_for_each_entry(uc, gd->uclass_root, 
sibling_node) {

So I would prefer to also use this new helper without the extra space as
well.

Thanks,
Stefan

Re: [PATCH u-boot-marvell v2] arm: mvebu: turris_omnia: Fixup SATA or PCIe nodes at runtime in DT blob

[Stefan Roese]

On 1/10/22 11:47, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> On of the MiniPCIe ports on Turris Omnia is also a mSATA port. Whether
> it works in SATA or PCIe mode is determined by a strapping pin, which
> value is read from the MCU.
> 
> We already determine which type of card is connected when configuring
> SerDeses.
> 
> But until now we left both SATA and PCIe port 0 nodes in device tree
> enabled, and so the SATA driver is probed in U-Boot / Linux even if we
> know there is no mSATA card, and similarly PCIe driver tries to link on
> port 0 even if we know there is mSATA card, not a PCIe card.
> 
> Fixup device tree blob to disable SATA node if mSATA card is not
> present, and to disable PCIe port 0 node if mSATA card is present.
> 
> Do this for U-Boot's DT blob before relocation and also for kernel DT
> blob before booting.
> 
> This ensures that software does not try to use SATA or PCIe HW when
> corresponding PHY is not configured.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> [ refactored and fixed some issues ]
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Applied to u-boot-marvell/master

Thanks,
Stefan

> ---
> Changes since v1:
> - removed extra space after macro name:
>      fdt_for_each_node_by_compatible (...)
>    to
>      fdt_for_each_node_by_compatible(...)
>    as requested by Stefan
> ---
>   board/CZ.NIC/turris_omnia/turris_omnia.c | 91 +++++++++++++++++++++++-
>   configs/turris_omnia_defconfig           |  1 +
>   2 files changed, 90 insertions(+), 2 deletions(-)
> 
> diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c
> index ae24d14b76..33cec6587e 100644
> --- a/board/CZ.NIC/turris_omnia/turris_omnia.c
> +++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
> @@ -490,6 +490,86 @@ void spl_board_init(void)
>   	}
>   }
>   
> +#if IS_ENABLED(CONFIG_OF_BOARD_FIXUP) || IS_ENABLED(CONFIG_OF_BOARD_SETUP)
> +
> +static void fixup_serdes_0_nodes(void *blob)
> +{
> +	bool mode_sata;
> +	int node;
> +
> +	/*
> +	 * Determine if SerDes 0 is configured to SATA mode.
> +	 * We do this instead of calling omnia_detect_sata() to avoid another
> +	 * call to the MCU. By this time the common PHYs are initialized (it is
> +	 * done in SPL), so we can read this common PHY register.
> +	 */
> +	mode_sata = (readl(MVEBU_REGISTER(0x183fc)) & GENMASK(3, 0)) == 2;
> +
> +	/*
> +	 * We're either adding status = "disabled" property, or changing
> +	 * status = "okay" to status = "disabled". In both cases we'll need more
> +	 * space. Increase the size a little.
> +	 */
> +	if (fdt_increase_size(blob, 32) < 0) {
> +		printf("Cannot increase FDT size!\n");
> +		return;
> +	}
> +
> +	/* If mSATA card is not present, disable SATA DT node */
> +	if (!mode_sata) {
> +		fdt_for_each_node_by_compatible(node, blob, -1,
> +						"marvell,armada-380-ahci") {
> +			if (!fdtdec_get_is_enabled(blob, node))
> +				continue;
> +
> +			if (fdt_status_disabled(blob, node) < 0)
> +				printf("Cannot disable SATA DT node!\n");
> +			else
> +				debug("Disabled SATA DT node\n");
> +
> +			break;
> +		}
> +
> +		return;
> +	}
> +
> +	/* Otherwise disable PCIe port 0 DT node (MiniPCIe / mSATA port) */
> +	fdt_for_each_node_by_compatible(node, blob, -1,
> +					"marvell,armada-370-pcie") {
> +		int port;
> +
> +		if (!fdtdec_get_is_enabled(blob, node))
> +			continue;
> +
> +		fdt_for_each_subnode (port, blob, node) {
> +			if (!fdtdec_get_is_enabled(blob, port))
> +				continue;
> +
> +			if (fdtdec_get_int(blob, port, "marvell,pcie-port",
> +					   -1) != 0)
> +				continue;
> +
> +			if (fdt_status_disabled(blob, port) < 0)
> +				printf("Cannot disable PCIe port 0 DT node!\n");
> +			else
> +				debug("Disabled PCIe port 0 DT node\n");
> +
> +			return;
> +		}
> +	}
> +}
> +
> +#endif
> +
> +#if IS_ENABLED(CONFIG_OF_BOARD_FIXUP)
> +int board_fix_fdt(void *blob)
> +{
> +	fixup_serdes_0_nodes(blob);
> +
> +	return 0;
> +}
> +#endif
> +
>   int board_init(void)
>   {
>   	/* address of boot parameters */
> @@ -694,7 +774,7 @@ static bool fixup_mtd_partitions(void *blob, int offset, struct mtd_info *mtd)
>   	return true;
>   }
>   
> -int ft_board_setup(void *blob, struct bd_info *bd)
> +static void fixup_spi_nor_partitions(void *blob)
>   {
>   	struct mtd_info *mtd;
>   	int node;
> @@ -711,12 +791,19 @@ int ft_board_setup(void *blob, struct bd_info *bd)
>   		goto fail;
>   
>   	put_mtd_device(mtd);
> -	return 0;
> +	return;
>   
>   fail:
>   	printf("Failed fixing SPI NOR partitions!\n");
>   	if (!IS_ERR_OR_NULL(mtd))
>   		put_mtd_device(mtd);
> +}
> +
> +int ft_board_setup(void *blob, struct bd_info *bd)
> +{
> +	fixup_spi_nor_partitions(blob);
> +	fixup_serdes_0_nodes(blob);
> +
>   	return 0;
>   }
>   #endif
> diff --git a/configs/turris_omnia_defconfig b/configs/turris_omnia_defconfig
> index e6f901ea77..6b1081c9bd 100644
> --- a/configs/turris_omnia_defconfig
> +++ b/configs/turris_omnia_defconfig
> @@ -23,6 +23,7 @@ CONFIG_DEBUG_UART_BASE=0xd0012000
>   CONFIG_DEBUG_UART_CLOCK=250000000
>   CONFIG_DEBUG_UART=y
>   CONFIG_AHCI=y
> +CONFIG_OF_BOARD_FIXUP=y
>   CONFIG_DISTRO_DEFAULTS=y
>   CONFIG_SYS_LOAD_ADDR=0x800000
>   CONFIG_FIT=y
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

Re: [PATCH u-boot-marvell v2] arm: mvebu: turris_omnia: Fixup SATA or PCIe nodes at runtime in DT blob

[Stefan Roese]

On 1/10/22 11:47, Marek Behún wrote:
> From: Pali Rohár <pali@kernel.org>
> 
> On of the MiniPCIe ports on Turris Omnia is also a mSATA port. Whether
> it works in SATA or PCIe mode is determined by a strapping pin, which
> value is read from the MCU.
> 
> We already determine which type of card is connected when configuring
> SerDeses.
> 
> But until now we left both SATA and PCIe port 0 nodes in device tree
> enabled, and so the SATA driver is probed in U-Boot / Linux even if we
> know there is no mSATA card, and similarly PCIe driver tries to link on
> port 0 even if we know there is mSATA card, not a PCIe card.
> 
> Fixup device tree blob to disable SATA node if mSATA card is not
> present, and to disable PCIe port 0 node if mSATA card is present.
> 
> Do this for U-Boot's DT blob before relocation and also for kernel DT
> blob before booting.
> 
> This ensures that software does not try to use SATA or PCIe HW when
> corresponding PHY is not configured.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> [ refactored and fixed some issues ]
> Signed-off-by: Marek Behún <marek.behun@nic.cz>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
> Changes since v1:
> - removed extra space after macro name:
>      fdt_for_each_node_by_compatible (...)
>    to
>      fdt_for_each_node_by_compatible(...)
>    as requested by Stefan
> ---
>   board/CZ.NIC/turris_omnia/turris_omnia.c | 91 +++++++++++++++++++++++-
>   configs/turris_omnia_defconfig           |  1 +
>   2 files changed, 90 insertions(+), 2 deletions(-)
> 
> diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c
> index ae24d14b76..33cec6587e 100644
> --- a/board/CZ.NIC/turris_omnia/turris_omnia.c
> +++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
> @@ -490,6 +490,86 @@ void spl_board_init(void)
>   	}
>   }
>   
> +#if IS_ENABLED(CONFIG_OF_BOARD_FIXUP) || IS_ENABLED(CONFIG_OF_BOARD_SETUP)
> +
> +static void fixup_serdes_0_nodes(void *blob)
> +{
> +	bool mode_sata;
> +	int node;
> +
> +	/*
> +	 * Determine if SerDes 0 is configured to SATA mode.
> +	 * We do this instead of calling omnia_detect_sata() to avoid another
> +	 * call to the MCU. By this time the common PHYs are initialized (it is
> +	 * done in SPL), so we can read this common PHY register.
> +	 */
> +	mode_sata = (readl(MVEBU_REGISTER(0x183fc)) & GENMASK(3, 0)) == 2;
> +
> +	/*
> +	 * We're either adding status = "disabled" property, or changing
> +	 * status = "okay" to status = "disabled". In both cases we'll need more
> +	 * space. Increase the size a little.
> +	 */
> +	if (fdt_increase_size(blob, 32) < 0) {
> +		printf("Cannot increase FDT size!\n");
> +		return;
> +	}
> +
> +	/* If mSATA card is not present, disable SATA DT node */
> +	if (!mode_sata) {
> +		fdt_for_each_node_by_compatible(node, blob, -1,
> +						"marvell,armada-380-ahci") {
> +			if (!fdtdec_get_is_enabled(blob, node))
> +				continue;
> +
> +			if (fdt_status_disabled(blob, node) < 0)
> +				printf("Cannot disable SATA DT node!\n");
> +			else
> +				debug("Disabled SATA DT node\n");
> +
> +			break;
> +		}
> +
> +		return;
> +	}
> +
> +	/* Otherwise disable PCIe port 0 DT node (MiniPCIe / mSATA port) */
> +	fdt_for_each_node_by_compatible(node, blob, -1,
> +					"marvell,armada-370-pcie") {
> +		int port;
> +
> +		if (!fdtdec_get_is_enabled(blob, node))
> +			continue;
> +
> +		fdt_for_each_subnode (port, blob, node) {
> +			if (!fdtdec_get_is_enabled(blob, port))
> +				continue;
> +
> +			if (fdtdec_get_int(blob, port, "marvell,pcie-port",
> +					   -1) != 0)
> +				continue;
> +
> +			if (fdt_status_disabled(blob, port) < 0)
> +				printf("Cannot disable PCIe port 0 DT node!\n");
> +			else
> +				debug("Disabled PCIe port 0 DT node\n");
> +
> +			return;
> +		}
> +	}
> +}
> +
> +#endif
> +
> +#if IS_ENABLED(CONFIG_OF_BOARD_FIXUP)
> +int board_fix_fdt(void *blob)
> +{
> +	fixup_serdes_0_nodes(blob);
> +
> +	return 0;
> +}
> +#endif
> +
>   int board_init(void)
>   {
>   	/* address of boot parameters */
> @@ -694,7 +774,7 @@ static bool fixup_mtd_partitions(void *blob, int offset, struct mtd_info *mtd)
>   	return true;
>   }
>   
> -int ft_board_setup(void *blob, struct bd_info *bd)
> +static void fixup_spi_nor_partitions(void *blob)
>   {
>   	struct mtd_info *mtd;
>   	int node;
> @@ -711,12 +791,19 @@ int ft_board_setup(void *blob, struct bd_info *bd)
>   		goto fail;
>   
>   	put_mtd_device(mtd);
> -	return 0;
> +	return;
>   
>   fail:
>   	printf("Failed fixing SPI NOR partitions!\n");
>   	if (!IS_ERR_OR_NULL(mtd))
>   		put_mtd_device(mtd);
> +}
> +
> +int ft_board_setup(void *blob, struct bd_info *bd)
> +{
> +	fixup_spi_nor_partitions(blob);
> +	fixup_serdes_0_nodes(blob);
> +
>   	return 0;
>   }
>   #endif
> diff --git a/configs/turris_omnia_defconfig b/configs/turris_omnia_defconfig
> index e6f901ea77..6b1081c9bd 100644
> --- a/configs/turris_omnia_defconfig
> +++ b/configs/turris_omnia_defconfig
> @@ -23,6 +23,7 @@ CONFIG_DEBUG_UART_BASE=0xd0012000
>   CONFIG_DEBUG_UART_CLOCK=250000000
>   CONFIG_DEBUG_UART=y
>   CONFIG_AHCI=y
> +CONFIG_OF_BOARD_FIXUP=y
>   CONFIG_DISTRO_DEFAULTS=y
>   CONFIG_SYS_LOAD_ADDR=0x800000
>   CONFIG_FIT=y
> 

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[PATCH u-boot-marvell v2] arm: mvebu: turris_omnia: Fixup SATA or PCIe nodes at runtime in DT blob

[Marek Behún]

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

On of the MiniPCIe ports on Turris Omnia is also a mSATA port. Whether
it works in SATA or PCIe mode is determined by a strapping pin, which
value is read from the MCU.

We already determine which type of card is connected when configuring
SerDeses.

But until now we left both SATA and PCIe port 0 nodes in device tree
enabled, and so the SATA driver is probed in U-Boot / Linux even if we
know there is no mSATA card, and similarly PCIe driver tries to link on
port 0 even if we know there is mSATA card, not a PCIe card.

Fixup device tree blob to disable SATA node if mSATA card is not
present, and to disable PCIe port 0 node if mSATA card is present.

Do this for U-Boot's DT blob before relocation and also for kernel DT
blob before booting.

This ensures that software does not try to use SATA or PCIe HW when
corresponding PHY is not configured.

Signed-off-by: Pali Rohár <pali@kernel.org>
[ refactored and fixed some issues ]
Signed-off-by: Marek Behún <marek.behun@nic.cz>
---
Changes since v1:
- removed extra space after macro name:
    fdt_for_each_node_by_compatible (...)
  to
    fdt_for_each_node_by_compatible(...)
  as requested by Stefan
---
 board/CZ.NIC/turris_omnia/turris_omnia.c | 91 +++++++++++++++++++++++-
 configs/turris_omnia_defconfig           |  1 +
 2 files changed, 90 insertions(+), 2 deletions(-)

diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c
index ae24d14b76..33cec6587e 100644
--- a/board/CZ.NIC/turris_omnia/turris_omnia.c
+++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
@@ -490,6 +490,86 @@ void spl_board_init(void)
 	}
 }
 
+#if IS_ENABLED(CONFIG_OF_BOARD_FIXUP) || IS_ENABLED(CONFIG_OF_BOARD_SETUP)
+
+static void fixup_serdes_0_nodes(void *blob)
+{
+	bool mode_sata;
+	int node;
+
+	/*
+	 * Determine if SerDes 0 is configured to SATA mode.
+	 * We do this instead of calling omnia_detect_sata() to avoid another
+	 * call to the MCU. By this time the common PHYs are initialized (it is
+	 * done in SPL), so we can read this common PHY register.
+	 */
+	mode_sata = (readl(MVEBU_REGISTER(0x183fc)) & GENMASK(3, 0)) == 2;
+
+	/*
+	 * We're either adding status = "disabled" property, or changing
+	 * status = "okay" to status = "disabled". In both cases we'll need more
+	 * space. Increase the size a little.
+	 */
+	if (fdt_increase_size(blob, 32) < 0) {
+		printf("Cannot increase FDT size!\n");
+		return;
+	}
+
+	/* If mSATA card is not present, disable SATA DT node */
+	if (!mode_sata) {
+		fdt_for_each_node_by_compatible(node, blob, -1,
+						"marvell,armada-380-ahci") {
+			if (!fdtdec_get_is_enabled(blob, node))
+				continue;
+
+			if (fdt_status_disabled(blob, node) < 0)
+				printf("Cannot disable SATA DT node!\n");
+			else
+				debug("Disabled SATA DT node\n");
+
+			break;
+		}
+
+		return;
+	}
+
+	/* Otherwise disable PCIe port 0 DT node (MiniPCIe / mSATA port) */
+	fdt_for_each_node_by_compatible(node, blob, -1,
+					"marvell,armada-370-pcie") {
+		int port;
+
+		if (!fdtdec_get_is_enabled(blob, node))
+			continue;
+
+		fdt_for_each_subnode (port, blob, node) {
+			if (!fdtdec_get_is_enabled(blob, port))
+				continue;
+
+			if (fdtdec_get_int(blob, port, "marvell,pcie-port",
+					   -1) != 0)
+				continue;
+
+			if (fdt_status_disabled(blob, port) < 0)
+				printf("Cannot disable PCIe port 0 DT node!\n");
+			else
+				debug("Disabled PCIe port 0 DT node\n");
+
+			return;
+		}
+	}
+}
+
+#endif
+
+#if IS_ENABLED(CONFIG_OF_BOARD_FIXUP)
+int board_fix_fdt(void *blob)
+{
+	fixup_serdes_0_nodes(blob);
+
+	return 0;
+}
+#endif
+
 int board_init(void)
 {
 	/* address of boot parameters */
@@ -694,7 +774,7 @@ static bool fixup_mtd_partitions(void *blob, int offset, struct mtd_info *mtd)
 	return true;
 }
 
-int ft_board_setup(void *blob, struct bd_info *bd)
+static void fixup_spi_nor_partitions(void *blob)
 {
 	struct mtd_info *mtd;
 	int node;
@@ -711,12 +791,19 @@ int ft_board_setup(void *blob, struct bd_info *bd)
 		goto fail;
 
 	put_mtd_device(mtd);
-	return 0;
+	return;
 
 fail:
 	printf("Failed fixing SPI NOR partitions!\n");
 	if (!IS_ERR_OR_NULL(mtd))
 		put_mtd_device(mtd);
+}
+
+int ft_board_setup(void *blob, struct bd_info *bd)
+{
+	fixup_spi_nor_partitions(blob);
+	fixup_serdes_0_nodes(blob);
+
 	return 0;
 }
 #endif
diff --git a/configs/turris_omnia_defconfig b/configs/turris_omnia_defconfig
index e6f901ea77..6b1081c9bd 100644
--- a/configs/turris_omnia_defconfig
+++ b/configs/turris_omnia_defconfig
@@ -23,6 +23,7 @@ CONFIG_DEBUG_UART_BASE=0xd0012000
 CONFIG_DEBUG_UART_CLOCK=250000000
 CONFIG_DEBUG_UART=y
 CONFIG_AHCI=y
+CONFIG_OF_BOARD_FIXUP=y
 CONFIG_DISTRO_DEFAULTS=y
 CONFIG_SYS_LOAD_ADDR=0x800000
 CONFIG_FIT=y
-- 
2.34.1