[PATCH] sunxi: H616: DRAM: refactor mctl_phy_configure_odt()

[Jernej Škrabec]

Dne sobota, 14. oktober 2023 ob 19:02:36 CEST je Andre Przywara napisal(a):
> The original H616 DDR3 ODT configuration code wrote board specific values
> into a sequence of paired registers.
> For LPDDR3 support we needed to special-case one group of registers,
> because for that DRAM type we need to write 0 into the lower register of
> each pair. That already made the code less readable.
> 
> LPDDR4 support will make things even messier, so let's refactor that
> code now: We allow to write different values into the lower and upper
> half of each pair. The masking is moved into a macro, and used in each
> write statement.
> 
> The effect is not as obvious yet, as we don't need the full flexibility at
> the moment, but the motivation will become clearer with LPDDR4 support.
> 
> The generated binary is identical with and without the patch.
> 
> Signed-off-by: Andre Przywara <andre.przywara at arm.com>

This looks much nicer.

Reviewed-by: Jernej Skrabec <jernej.skrabec at gmail.com>

Best regards,
Jernej

> ---
>  arch/arm/mach-sunxi/dram_sun50i_h616.c | 84 ++++++++++----------------
>  1 file changed, 31 insertions(+), 53 deletions(-)
> 
> diff --git a/arch/arm/mach-sunxi/dram_sun50i_h616.c b/arch/arm/mach-sunxi/dram_sun50i_h616.c
> index 7e580b62dca..ba5659d4094 100644
> --- a/arch/arm/mach-sunxi/dram_sun50i_h616.c
> +++ b/arch/arm/mach-sunxi/dram_sun50i_h616.c
> @@ -241,61 +241,39 @@ static const u8 phy_init[] = {
>  #endif
>  };
>  
> +#define MASK_BYTE(reg, nr) (((reg) >> ((nr) * 8)) & 0x1f)
>  static void mctl_phy_configure_odt(const struct dram_para *para)
>  {
> -	unsigned int val;
> -
> -	val = para->dx_dri & 0x1f;
> -	writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x388);
> -	writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x38c);
> -
> -	val = (para->dx_dri >> 8) & 0x1f;
> -	writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x3c8);
> -	writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x3cc);
> -
> -	val = (para->dx_dri >> 16) & 0x1f;
> -	writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x408);
> -	writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x40c);
> -
> -	val = (para->dx_dri >> 24) & 0x1f;
> -	writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x448);
> -	writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x44c);
> -
> -	val = para->ca_dri & 0x1f;
> -	writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x340);
> -	writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x344);
> -
> -	val = (para->ca_dri >> 8) & 0x1f;
> -	writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x348);
> -	writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x34c);
> -
> -	val = para->dx_odt & 0x1f;
> -	if (para->type == SUNXI_DRAM_TYPE_LPDDR3)
> -		writel_relaxed(0, SUNXI_DRAM_PHY0_BASE + 0x380);
> -	else
> -		writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x380);
> -	writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x384);
> -
> -	val = (para->dx_odt >> 8) & 0x1f;
> -	if (para->type == SUNXI_DRAM_TYPE_LPDDR3)
> -		writel_relaxed(0, SUNXI_DRAM_PHY0_BASE + 0x3c0);
> -	else
> -		writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x3c0);
> -	writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x3c4);
> -
> -	val = (para->dx_odt >> 16) & 0x1f;
> -	if (para->type == SUNXI_DRAM_TYPE_LPDDR3)
> -		writel_relaxed(0, SUNXI_DRAM_PHY0_BASE + 0x400);
> -	else
> -		writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x400);
> -	writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x404);
> -
> -	val = (para->dx_odt >> 24) & 0x1f;
> -	if (para->type == SUNXI_DRAM_TYPE_LPDDR3)
> -		writel_relaxed(0, SUNXI_DRAM_PHY0_BASE + 0x440);
> -	else
> -		writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x440);
> -	writel_relaxed(val, SUNXI_DRAM_PHY0_BASE + 0x444);
> +	uint32_t val_lo, val_hi;
> +
> +	val_lo = para->dx_dri;
> +	val_hi = para->dx_dri;
> +	writel_relaxed(MASK_BYTE(val_lo, 0), SUNXI_DRAM_PHY0_BASE + 0x388);
> +	writel_relaxed(MASK_BYTE(val_hi, 0), SUNXI_DRAM_PHY0_BASE + 0x38c);
> +	writel_relaxed(MASK_BYTE(val_lo, 1), SUNXI_DRAM_PHY0_BASE + 0x3c8);
> +	writel_relaxed(MASK_BYTE(val_hi, 1), SUNXI_DRAM_PHY0_BASE + 0x3cc);
> +	writel_relaxed(MASK_BYTE(val_lo, 2), SUNXI_DRAM_PHY0_BASE + 0x408);
> +	writel_relaxed(MASK_BYTE(val_hi, 2), SUNXI_DRAM_PHY0_BASE + 0x40c);
> +	writel_relaxed(MASK_BYTE(val_lo, 3), SUNXI_DRAM_PHY0_BASE + 0x448);
> +	writel_relaxed(MASK_BYTE(val_hi, 3), SUNXI_DRAM_PHY0_BASE + 0x44c);
> +
> +	val_lo = para->ca_dri;
> +	val_hi = para->ca_dri;
> +	writel_relaxed(MASK_BYTE(val_lo, 0), SUNXI_DRAM_PHY0_BASE + 0x340);
> +	writel_relaxed(MASK_BYTE(val_hi, 0), SUNXI_DRAM_PHY0_BASE + 0x344);
> +	writel_relaxed(MASK_BYTE(val_lo, 1), SUNXI_DRAM_PHY0_BASE + 0x348);
> +	writel_relaxed(MASK_BYTE(val_hi, 1), SUNXI_DRAM_PHY0_BASE + 0x34c);
> +
> +	val_lo = (para->type == SUNXI_DRAM_TYPE_LPDDR3) ? 0 : para->dx_odt;
> +	val_hi = para->dx_odt;
> +	writel_relaxed(MASK_BYTE(val_lo, 0), SUNXI_DRAM_PHY0_BASE + 0x380);
> +	writel_relaxed(MASK_BYTE(val_hi, 0), SUNXI_DRAM_PHY0_BASE + 0x384);
> +	writel_relaxed(MASK_BYTE(val_lo, 1), SUNXI_DRAM_PHY0_BASE + 0x3c0);
> +	writel_relaxed(MASK_BYTE(val_hi, 1), SUNXI_DRAM_PHY0_BASE + 0x3c4);
> +	writel_relaxed(MASK_BYTE(val_lo, 2), SUNXI_DRAM_PHY0_BASE + 0x400);
> +	writel_relaxed(MASK_BYTE(val_hi, 2), SUNXI_DRAM_PHY0_BASE + 0x404);
> +	writel_relaxed(MASK_BYTE(val_lo, 3), SUNXI_DRAM_PHY0_BASE + 0x440);
> +	writel_relaxed(MASK_BYTE(val_hi, 3), SUNXI_DRAM_PHY0_BASE + 0x444);
>  
>  	dmb();
>  }
>