Re: Subject: [PATCH 1/2] ARM: add LS1021A to Layerscape machine support

[Sascha Hauer <sha@pengutronix.de>]

On Thu, Feb 23, 2023 at 01:58:44PM +0000, Renaud Barbier wrote:
> This updates the Layerscape support in preparation for the
> introduction of the LS1021A-IOT:
> 
> - Makefile/Kconfig
> - LS1021A specific register maps and configurations
> - errata workarounds update
> 
> Many existing functions used for the ls1046a now use the
> common prefix layerscape for both machines. Consequently,
> the ls1046 board supports are updated.
> 
> Signed-off-by: Renaud Barbier <renaud.barbier@ametek.com>

[...]

> -static void erratum_a008997_ls1046a(void)
> +static void erratum_a008997_layerscape(void)
>  {
>  	u32 __iomem *scfg = (u32 __iomem *)LSCH2_SCFG_ADDR;
>  
>  	set_usb_pcstxswingfull(scfg, SCFG_USB3PRM2CR_USB1);
> -	set_usb_pcstxswingfull(scfg, SCFG_USB3PRM2CR_USB2);
> -	set_usb_pcstxswingfull(scfg, SCFG_USB3PRM2CR_USB3);
> +	if (IS_ENABLED(CONFIG_ARCH_LS1046)) {
> +		set_usb_pcstxswingfull(scfg, SCFG_USB3PRM2CR_USB2);
> +		set_usb_pcstxswingfull(scfg, SCFG_USB3PRM2CR_USB3);
> +	}
>  }

You are distinuishing the different SoCs using the preprocessor which
prevents us from ever building a barebox that runs on both SoCs.


> -void ls1046a_errata(void)
> +void layerscape_errata(void)
>  {
>  	erratum_a008850_early();
> -	erratum_a009008_ls1046a();
> -	erratum_a009798_ls1046a();
> -	erratum_a008997_ls1046a();
> -	erratum_a009007_ls1046a();
> +	erratum_a009008_layerscape();
> +	erratum_a009798_layerscape();
> +	erratum_a008997_layerscape();
> +	erratum_a009007_layerscape();
>  }

The pattern should rather be:

static void layerscape_errata(void)
{
	/* do common stuff */
}

void ls1046a_errata(void)
{
	layerscape_errata();

	/* do ls1046a specific stuff */
}

void ls1021a_errata(void)
{
	layerscape_errata();

	/* do ls1021a specific stuff */
}

The rationale is that this is called from board code and the board
already knows which SoC it runs with, so we can better call SoC specific
functions and do the common stuff from there.

This avoids ifdeffery and we can create a barebox that runs on different
layerscape SoCs (well at least we could if they had the same architecture)

> diff --git a/arch/arm/mach-layerscape/include/mach/layerscape.h b/arch/arm/mach-layerscape/include/mach/layerscape.h
> index 447417a266..d53d01cd5f 100644
> --- a/arch/arm/mach-layerscape/include/mach/layerscape.h
> +++ b/arch/arm/mach-layerscape/include/mach/layerscape.h
> @@ -3,10 +3,12 @@
>  #ifndef __MACH_LAYERSCAPE_H
>  #define __MACH_LAYERSCAPE_H
>  
> -#define LS1046A_DDR_SDRAM_BASE	0x80000000
> -#define LS1046A_DDR_FREQ	2100000000
> +enum bootsource layerscape_bootsource_get(void);
> +
> +#define LAYERSCAPE_DDR_SDRAM_BASE	0x80000000
>  
> -enum bootsource ls1046_bootsource_get(void);
> +#ifdef CONFIG_ARCH_LS1046
> +#define LS1046A_DDR_FREQ	2100000000

This define is properly prefixed with the SoC type, no need to ifdef it.

>  
>  #ifdef CONFIG_ARCH_LAYERSCAPE_PPA
>  int ls1046a_ppa_init(resource_size_t ppa_start, resource_size_t ppa_size);
> @@ -17,5 +19,10 @@ static inline int ls1046a_ppa_init(resource_size_t ppa_start,
>  	return -ENOSYS;
>  }
>  #endif
> +#endif
> +
> +#ifdef CONFIG_ARCH_LS1021
> +#define LS1021A_DDR_FREQ	1600000000
> +#endif

ditto.

> +static int ls102xa_smmu_stream_id_init(void)
> +{
> +	ls102xa_config_smmu_stream_id(dev_stream_id, ARRAY_SIZE(dev_stream_id));
> +
> +	return 0;
> +}
> +mmu_initcall(ls102xa_smmu_stream_id_init);

This should be protected from running on the wrong SoC, with something
like

	if (!of_machine_is_compatible("fsl,ls1021a"))
		return 0;

> +static int restart_register_feature(void)
> +{
> +	restart_handler_register_fn("soc-reset", ls102xa_restart);
> +
> +	return 0;
> +}
> +coredevice_initcall(restart_register_feature);

ditto


> diff --git a/arch/arm/mach-layerscape/xload-qspi.c b/arch/arm/mach-layerscape/xload-qspi.c
> index 192aea64b4..af2834f35e 100644
> --- a/arch/arm/mach-layerscape/xload-qspi.c
> +++ b/arch/arm/mach-layerscape/xload-qspi.c
> @@ -13,11 +13,11 @@
>   */
>  #define BAREBOX_START	(128 * 1024)
>  
> -int ls1046a_qspi_start_image(unsigned long r0, unsigned long r1,
> +int layerscape_qspi_start_image(unsigned long r0, unsigned long r1,
>  					     unsigned long r2)
>  {
>  	void *qspi_reg_base = IOMEM(LSCH2_QSPI0_BASE_ADDR);
> -	void *membase = (void *)LS1046A_DDR_SDRAM_BASE;
> +	void *membase = (void *)LAYERSCAPE_DDR_SDRAM_BASE;
>  	void *qspi_mem_base = IOMEM(0x40000000);
>  	void (*barebox)(unsigned long, unsigned long, unsigned long) = membase;
>

As above: keep the name ls1046a_qspi_start_image(), add
ls1021a_qspi_start_image(), if appropriate factor out common stuff to a
layerscape_qspi_start_image(), for example by passing the base address
as argument if necessary.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |