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

Yes, this patch allows to reduce code size and make it easier to read.

To further integrate the code for LPDDR4, will need to change two lines instead of adding 24 lines.

Reviewed-by: Mikhail Kalashnikov iuncuim@gmail.com

On 14.10.2023 20:02, Andre Przywara wrote:

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@arm.com

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(); }