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

[Stefan Roese <sr@denx.de>]

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