Re: [PATCH V2 2/9] sunxi: H616: DRAM: Add alternative pin mapping

[Andre Przywara <andre.przywara@arm.com>]

On Mon, 19 Aug 2024 09:59:31 -0500
Chris Morgan <macroalpha82@gmail.com> wrote:

Hi,

> From: Jernej Skrabec <jernej.skrabec@gmail.com>
> 
> It seems that different dies need different PHY pin mapping. Select
> alternatives based on "bond ID".

Do we really need this to determined at runtime? I appreciate the idea
of making the code more versatile, but we have far more board specific
parameters fixed at build time, so detecting a SoC type at runtime
sounds a bit pointless. I am asking because this increases the SPL size
by like 115 bytes.

> Signed-off-by: Jernej Skrabec <jernej.skrabec@gmail.com>
> Tested-by: Chris Morgan <macromorgan@hotmail.com>
> ---
>  arch/arm/mach-sunxi/dram_sun50i_h616.c | 59 +++++++++++++++++++-------
>  1 file changed, 44 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/arm/mach-sunxi/dram_sun50i_h616.c b/arch/arm/mach-sunxi/dram_sun50i_h616.c
> index 5be2887a06..dfaa270d96 100644
> --- a/arch/arm/mach-sunxi/dram_sun50i_h616.c
> +++ b/arch/arm/mach-sunxi/dram_sun50i_h616.c
> @@ -225,22 +225,43 @@ static void mctl_set_addrmap(const struct dram_config *config)
>  	mctl_ctl->addrmap[8] = 0x3F3F;
>  }
>  
> -static const u8 phy_init[] = {
> +static const u8 phy_addr_maps[2][27] = {
>  #ifdef CONFIG_SUNXI_DRAM_H616_DDR3_1333
> -	0x07, 0x0b, 0x02, 0x16, 0x0d, 0x0e, 0x14, 0x19,
> -	0x0a, 0x15, 0x03, 0x13, 0x04, 0x0c, 0x10, 0x06,
> -	0x0f, 0x11, 0x1a, 0x01, 0x12, 0x17, 0x00, 0x08,
> -	0x09, 0x05, 0x18
> +	{
> +		0x07, 0x0b, 0x02, 0x16, 0x0d, 0x0e, 0x14, 0x19,
> +		0x0a, 0x15, 0x03, 0x13, 0x04, 0x0c, 0x10, 0x06,
> +		0x0f, 0x11, 0x1a, 0x01, 0x12, 0x17, 0x00, 0x08,
> +		0x09, 0x05, 0x18
> +	}, {
> +		0x08, 0x02, 0x12, 0x05, 0x15, 0x17, 0x18, 0x0b,
> +		0x14, 0x07, 0x04, 0x13, 0x0c, 0x00, 0x16, 0x1a,
> +		0x0a, 0x11, 0x03, 0x10, 0x0e, 0x01, 0x0d, 0x19,
> +		0x06, 0x09, 0x0f
> +	}
>  #elif defined(CONFIG_SUNXI_DRAM_H616_LPDDR3)
> -	0x18, 0x06, 0x00, 0x05, 0x04, 0x03, 0x09, 0x02,
> -	0x08, 0x01, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f,
> -	0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x07,
> -	0x17, 0x19, 0x1a
> +	{
> +		0x18, 0x06, 0x00, 0x05, 0x04, 0x03, 0x09, 0x02,
> +		0x08, 0x01, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f,
> +		0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x07,
> +		0x17, 0x19, 0x1a
> +	}, {
> +		0x18, 0x00, 0x04, 0x09, 0x06, 0x05, 0x02, 0x19,
> +		0x17, 0x03, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f,
> +		0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x07,
> +		0x08, 0x01, 0x1a
> +	}
>  #elif defined(CONFIG_SUNXI_DRAM_H616_LPDDR4)
> -	0x02, 0x00, 0x17, 0x05, 0x04, 0x19, 0x06, 0x07,
> -	0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f,
> -	0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x01,
> -	0x18, 0x03, 0x1a
> +	{
> +		0x02, 0x00, 0x17, 0x05, 0x04, 0x19, 0x06, 0x07,
> +		0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f,
> +		0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x01,
> +		0x18, 0x03, 0x1a
> +	}, {
> +		0x03, 0x00, 0x17, 0x05, 0x02, 0x19, 0x06, 0x07,
> +		0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f,
> +		0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x01,
> +		0x18, 0x04, 0x1a
> +	}
>  #endif
>  };
>  
> @@ -887,6 +908,7 @@ static bool mctl_phy_init(const struct dram_para *para,
>  	struct sunxi_mctl_ctl_reg * const mctl_ctl =
>  			(struct sunxi_mctl_ctl_reg *)SUNXI_DRAM_CTL0_BASE;
>  	u32 val, val2, *ptr, mr0, mr2;
> +	const u8 *map;
>  	int i;
>  
>  	if (para->type == SUNXI_DRAM_TYPE_LPDDR4)
> @@ -942,8 +964,15 @@ static bool mctl_phy_init(const struct dram_para *para,
>  	writel(val2, SUNXI_DRAM_PHY0_BASE + 0x37c);
>  
>  	ptr = (u32 *)(SUNXI_DRAM_PHY0_BASE + 0xc0);
> -	for (i = 0; i < ARRAY_SIZE(phy_init); i++)
> -		writel(phy_init[i], &ptr[i]);
> +	val = readl(SUNXI_SID_BASE);
> +	if (((val & 0xfbff) == 0x5000) ||
> +	    ((val & 0xfeff) == 0x5c00) ||
> +	    ((val & 0xf7ff) == 0x2000))

so for the records, looking at
https://linux-sunxi.org/SID_Register_Guide#Currently_known_SID.27s
this means: H616, H313, H618, respectively. Which seems to be fixed for
each board.

> +		map = phy_addr_maps[0];
> +	else
> +		map = phy_addr_maps[1];
> +	for (i = 0; i < ARRAY_SIZE(phy_addr_maps[0]); i++)
> +		writel(map[i], &ptr[i]);

If I drop the SID read above, and replace this code with something
based on CONFIG_SUNXI_DRAM_H616_PHY_ADDR_MAP, it only grows by 35 bytes.
If I put #if's in the array definitions above, it stays entirely at the
old size, but at the cost of being hard to read - at least in the
version I came up with.

What do other people say here?

Cheers,
Andre

>  
>  	if (para->tpr10 & TPR10_CA_BIT_DELAY)
>  		mctl_phy_ca_bit_delay_compensation(para, config);