[U-Boot] [PATCH 08/12] aspeed/ast2500: Add SDRAM MC driver

Simon Glass
2017-01-14T17:14:13Z

Hi Maxim,

On 4 January 2017 at 12:46, Maxim Sloyko <maxims@google.com> wrote:
> The driver is very ast2500 specific and is completely incompatible

ast2500-specific

> with previous versions of the chip.
>
> The memory controller is very poorly documented by Aspeed in the
> datasheet, with any mention of the whole range of registers missing. The
> initialization procedure has been basically taken from Aspeed SDK, where
> it is implemented in assembly and rewritten in C, with very limited
> understanding of what exacly it is doing.

Here I think you mean:
where
it is implemented in assembly. Here it is rewritten in C, with very limited
understanding of what exactly (spelling) it is doing.

>
> Signed-off-by: Maxim Sloyko <maxims@google.com>
> ---
>
> arch/arm/include/asm/arch-aspeed/sdram_ast2500.h | 133 +++++++
> arch/arm/mach-aspeed/ast2500/Makefile | 2 +-
> arch/arm/mach-aspeed/ast2500/sdram_ast2500.c | 443 +++++++++++++++++++++++
> 3 files changed, 577 insertions(+), 1 deletion(-)
> create mode 100644 arch/arm/include/asm/arch-aspeed/sdram_ast2500.h
> create mode 100644 arch/arm/mach-aspeed/ast2500/sdram_ast2500.c
>
> diff --git a/arch/arm/include/asm/arch-aspeed/sdram_ast2500.h b/arch/arm/include/asm/arch-aspeed/sdram_ast2500.h
> new file mode 100644
> index 0000000000..ca70898df6
> --- /dev/null
> +++ b/arch/arm/include/asm/arch-aspeed/sdram_ast2500.h
> @@ -0,0 +1,133 @@
> +#ifndef _ASM_ARCH_SDRAM_AST2500_H
> +#define _ASM_ARCH_SDRAM_AST2500_H
> +
> +#define SDRAM_UNLOCK_KEY 0xfc600309
> +#define SDRAM_VIDEO_UNLOCK_KEY 0x2003000f
> +
> +#define SDRAM_PCR_CKE_EN (1 << 0)
> +#define SDRAM_PCR_AUTOPWRDN_EN (1 << 1)
> +#define SDRAM_PCR_CKE_DELAY_SHIFT 4
> +#define SDRAM_PCR_CKE_DELAY_MASK 7
> +#define SDRAM_PCR_RESETN_DIS (1 << 7)
> +#define SDRAM_PCR_ODT_EN (1 << 8)
> +#define SDRAM_PCR_ODT_AUTO_ON (1 << 10)
> +#define SDRAM_PCR_ODT_EXT_EN (1 << 11)
> +#define SDRAM_PCR_TCKE_PW_SHIFT 12
> +#define SDRAM_PCR_TCKE_PW_MASK 7
> +#define SDRAM_PCR_RGAP_CTRL_EN (1 << 15)
> +#define SDRAM_PCR_MREQI_DIS (1 << 17)
> +
> +/* Fixed priority DRAM Requests mask */
> +#define SDRAM_REQ_VGA_HW_CURSOR (1 << 0)
> +#define SDRAM_REQ_VGA_TEXT_CG_FONT (1 << 1)
> +#define SDRAM_REQ_VGA_TEXT_ASCII (1 << 2)
> +#define SDRAM_REQ_VGA_CRT (1 << 3)
> +#define SDRAM_REQ_SOC_DC_CURSOR (1 << 4)
> +#define SDRAM_REQ_SOC_DC_OCD (1 << 5)
> +#define SDRAM_REQ_SOC_DC_CRT (1 << 6)
> +#define SDRAM_REQ_VIDEO_HIPRI_WRITE (1 << 7)
> +#define SDRAM_REQ_USB20_EHCI1 (1 << 8)
> +#define SDRAM_REQ_USB20_EHCI2 (1 << 9)
> +#define SDRAM_REQ_CPU (1 << 10)
> +#define SDRAM_REQ_AHB2 (1 << 11)
> +#define SDRAM_REQ_AHB (1 << 12)
> +#define SDRAM_REQ_MAC0 (1 << 13)
> +#define SDRAM_REQ_MAC1 (1 << 14)
> +#define SDRAM_REQ_PCIE (1 << 16)
> +#define SDRAM_REQ_XDMA (1 << 17)
> +#define SDRAM_REQ_ENCRYPTION (1 << 18)
> +#define SDRAM_REQ_VIDEO_FLAG (1 << 21)
> +#define SDRAM_REQ_VIDEO_LOW_PRI_WRITE (1 << 28)
> +#define SDRAM_REQ_2D_RW (1 << 29)
> +#define SDRAM_REQ_MEMCHECK (1 << 30)
> +
> +#define SDRAM_ICR_RESET_ALL (1 << 31)
> +
> +#define SDRAM_CONF_CAP_SHIFT 0
> +#define SDRAM_CONF_CAP_MASK 3
> +#define SDRAM_CONF_DDR4 (1 << 4)
> +#define SDRAM_CONF_SCRAMBLE (1 << 8)
> +#define SDRAM_CONF_SCRAMBLE_PAT2 (1 << 9)
> +#define SDRAM_CONF_CACHE_EN (1 << 10)
> +#define SDRAM_CONF_CACHE_INIT_EN (1 << 12)
> +#define SDRAM_CONF_DUALX8 (1 << 13)
> +#define SDRAM_CONF_CACHE_INIT_DONE (1 << 19)
> +
> +#define SDRAM_CONF_CAP_128M 0
> +#define SDRAM_CONF_CAP_256M 1
> +#define SDRAM_CONF_CAP_512M 2
> +#define SDRAM_CONF_CAP_1024M 3
> +
> +#define SDRAM_MISC_DDR4_TREFRESH (1 << 3)
> +
> +#define SDRAM_PHYCTRL0_INIT (1 << 0)
> +#define SDRAM_PHYCTRL0_AUTO_UPDATE (1 << 1)
> +#define SDRAM_PHYCTRL0_NRST (1 << 2)
> +
> +#define SDRAM_REFRESH_CYCLES_SHIFT 0
> +#define SDRAM_REFRESH_CYCLES_MASK 0xf
> +#define SDRAM_REFRESH_ZQCS_EN (1 << 7)
> +#define SDRAM_REFRESH_PERIOD_SHIFT 8
> +#define SDRAM_REFRESH_PERIOD_MASK 0xf
> +
> +#define SDRAM_TEST_LEN_SHIFT 4
> +#define SDRAM_TEST_LEN_MASK 0xfffff
> +#define SDRAM_TEST_START_ADDR_SHIFT 24
> +#define SDRAM_TEST_START_ADDR_MASK 0x3f
> +
> +#define SDRAM_TEST_EN (1 << 0)
> +#define SDRAM_TEST_MODE_SHIFT 1
> +#define SDRAM_TEST_MODE_MASK 3
> +#define SDRAM_TEST_MODE_WO 0
> +#define SDRAM_TEST_MODE_RB 1
> +#define SDRAM_TEST_MODE_RW 2
> +#define SDRAM_TEST_GEN_MODE_SHIFT 3
> +#define SDRAM_TEST_GEN_MODE_MASK 7
> +#define SDRAM_TEST_TWO_MODES (1 << 6)
> +#define SDRAM_TEST_ERRSTOP (1 << 7)
> +#define SDRAM_TEST_DONE (1 << 12)
> +#define SDRAM_TEST_FAIL (1 << 13)
> +
> +#define SDRAM_AC_TRFC_SHIFT 0
> +#define SDRAM_AC_TRFC_MASK 0xff
> +
> +#ifndef __ASSEMBLY__
> +
> +struct ast2500_sdrammc_regs {
> + u32 protection_key;
> + u32 config;
> + u32 gm_protection_key;
> + u32 refresh_timing;
> + u32 ac_timing[3];
> + u32 misc_control;
> + u32 mr46_mode_setting;
> + u32 mr5_mode_setting;
> + u32 mode_setting_control;
> + u32 mr02_mode_setting;
> + u32 mr13_mode_setting;
> + u32 power_control;
> + u32 req_limit_mask;
> + u32 pri_group_setting;
> + u32 max_grant_len[4];
> + u32 intr_ctrl;
> + u32 ecc_range_ctrl;
> + u32 first_ecc_err_addr;
> + u32 last_ecc_err_addr;
> + u32 phy_ctrl[4];
> + u32 ecc_test_ctrl;
> + u32 test_addr;
> + u32 test_fail_dq_bit;
> + u32 test_init_val;
> + u32 phy_debug_ctrl;
> + u32 phy_debug_data;
> + u32 reserved1[30];
> + u32 scu_passwd;
> + u32 reserved2[7];
> + u32 scu_mpll;
> + u32 reserved3[19];
> + u32 scu_hwstrap;
> +};
> +
> +#endif /* __ASSEMBLY__ */
> +
> +#endif /* _ASM_ARCH_SDRAM_AST2500_H */
> diff --git a/arch/arm/mach-aspeed/ast2500/Makefile b/arch/arm/mach-aspeed/ast2500/Makefile
> index 97d4078ddd..a35b239ef3 100644
> --- a/arch/arm/mach-aspeed/ast2500/Makefile
> +++ b/arch/arm/mach-aspeed/ast2500/Makefile
> @@ -1 +1 @@
> -obj-y += clk_ast2500.o
> +obj-y += clk_ast2500.o sdram_ast2500.o
> diff --git a/arch/arm/mach-aspeed/ast2500/sdram_ast2500.c b/arch/arm/mach-aspeed/ast2500/sdram_ast2500.c
> new file mode 100644
> index 0000000000..da25756e94
> --- /dev/null
> +++ b/arch/arm/mach-aspeed/ast2500/sdram_ast2500.c
> @@ -0,0 +1,443 @@
> +/*
> + * Copyright (C) 2012-2020 ASPEED Technology Inc.
> + *
> + * Copyright 2016 Google, Inc
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
> +
> +#include <common.h>
> +#include <clk.h>
> +#include <dm.h>
> +#include <dt-bindings/clock/ast2500-scu.h>
> +#include <errno.h>
> +#include <ram.h>
> +#include <asm/io.h>
> +#include <asm/arch/scu_ast2500.h>
> +#include <asm/arch/sdram_ast2500.h>
> +#include <asm/arch/wdt.h>
> +#include <linux/err.h>
> +#include <linux/kernel.h>
> +#include <regmap.h>

Check ordering: http://www.denx.de/wiki/U-Boot/CodingStyle

> +
> +/* These configuration parameters are taken from Aspeed SDK */
> +#define DDR4_MR46_MODE 0x08000000
> +#define DDR4_MR5_MODE 0x400
> +#define DDR4_MR13_MODE 0x101
> +#define DDR4_MR02_MODE 0x410
> +#define DDR4_TRFC 0x45457188
> +
> +#define PHY_CFG_SIZE 15
> +
> +static const u32 ddr4_ac_timing[3] = {0x63604e37, 0xe97afa99, 0x00019000};
> +static const struct {
> + u32 index[PHY_CFG_SIZE];
> + u32 value[PHY_CFG_SIZE];
> +} ddr4_phy_config = {
> + .index = {0, 1, 3, 4, 5, 56, 57, 58, 59, 60, 61, 62, 36, 49, 50},
> + .value = {
> + 0x42492aae, 0x09002000, 0x55e00b0b, 0x20000000, 0x24,
> + 0x03002900, 0x0e0000a0, 0x000e001c, 0x35b8c106, 0x08080607,
> + 0x9b000900, 0x0e400a00, 0x00100008, 0x3c183c3c, 0x00631e0e,
> + },
> +};
> +
> +#define SDRAM_MAX_SIZE (1024 * 1024 * 1024)
> +#define SDRAM_MIN_SIZE (128 * 1024 * 1024)
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +/*
> + * Bandwidth configuration parameters for different SDRAM requests.
> + * These are hardcoded settings taken from Aspeed SDK.
> + */
> +static const u32 ddr_max_grant_params[4] = {
> + 0x88448844, 0x24422288, 0x22222222, 0x22222222
> +};
> +
> +/*
> + * These registers are not documented by Aspeed at all.
> + * All writes and reads are taken pretty much as is from SDK.
> + */
> +struct ast2500_ddr_phy {
> + u32 phy[117];
> +};
> +
> +struct dram_info {
> + struct ram_info info;
> + struct clk ddr_clk;
> + struct ast2500_sdrammc_regs *regs;
> + struct ast2500_scu *scu;
> + struct ast2500_ddr_phy *phy;
> + ulong clock_rate;
> +};
> +
> +static int ast2500_sdrammc_reset(void)
> +{
> + struct ast_wdt *wdt = ast_get_wdt(0);

blank line. But can you pass this in as a parameter?

> + if (IS_ERR(wdt))
> + return PTR_ERR(wdt);
> +
> + /* Only Reset SDRAM */
> + writel(WDT_RESET_SDRAM, &wdt->reset_mask);
> + clrbits_le32(&wdt->ctrl,
> + WDT_CTRL_RESET_MASK << WDT_CTRL_RESET_MODE_SHIFT);
> + wdt_start(wdt, 1);
> +
> + /* Wait for WDT to reset */
> + while (readl(&wdt->ctrl) & WDT_CTRL_EN)
> + ;
> + wdt_stop(wdt);

Feels like you should have a helper function or driver to do the
reset, not access the watchdog here.

> +
> + return 0;
> +}
> +
> +static int ast2500_sdrammc_init_phy(struct ast2500_ddr_phy *phy)
> +{
> + writel(0, &phy->phy[2]);
> + writel(0, &phy->phy[6]);
> + writel(0, &phy->phy[8]);
> + writel(0, &phy->phy[10]);
> + writel(0, &phy->phy[12]);
> + writel(0, &phy->phy[42]);
> + writel(0, &phy->phy[44]);
> +
> + writel(0x86000000, &phy->phy[16]);
> + writel(0x00008600, &phy->phy[17]);
> + writel(0x80000000, &phy->phy[18]);
> + writel(0x80808080, &phy->phy[19]);
> +
> + return 0;
> +}
> +
> +static void ast2500_ddr_phy_init_process(struct dram_info *info)
> +{
> + struct ast2500_sdrammc_regs *regs = info->regs;

blank line. Please fix globally - need a line between decls and code.

> + writel(0, &regs->phy_ctrl[0]);
> + writel(0x4040, &info->phy->phy[51]);
> +
> + writel(SDRAM_PHYCTRL0_NRST | SDRAM_PHYCTRL0_INIT, &regs->phy_ctrl[0]);
> + while ((readl(&regs->phy_ctrl[0]) & SDRAM_PHYCTRL0_INIT))
> + ;
> + writel(SDRAM_PHYCTRL0_NRST | SDRAM_PHYCTRL0_AUTO_UPDATE,
> + &regs->phy_ctrl[0]);
> +}
> +
> +static void ast2500_sdrammc_set_vref(struct dram_info *info, u32 vref)
> +{
> + writel(0, &info->regs->phy_ctrl[0]);
> + writel((vref << 8) | 0x6, &info->phy->phy[48]);
> + ast2500_ddr_phy_init_process(info);
> +}
> +
> +static int ast2500_ddr_cbr_test(struct dram_info *info)
> +{
> + struct ast2500_sdrammc_regs *regs = info->regs;
> + int i;
> + const u32 test_params = SDRAM_TEST_EN
> + | SDRAM_TEST_ERRSTOP
> + | SDRAM_TEST_TWO_MODES;
> + int ret = 0;
> +
> + writel((1 << SDRAM_REFRESH_CYCLES_SHIFT) |
> + (0x5c << SDRAM_REFRESH_PERIOD_SHIFT), &regs->refresh_timing);
> + writel((0xfff << SDRAM_TEST_LEN_SHIFT), &regs->test_addr);
> + writel(0xff00ff00, &regs->test_init_val);
> + writel(SDRAM_TEST_EN | (SDRAM_TEST_MODE_RW << SDRAM_TEST_MODE_SHIFT) |
> + SDRAM_TEST_ERRSTOP, &regs->ecc_test_ctrl);
> +
> + while (!(readl(&regs->ecc_test_ctrl) & SDRAM_TEST_DONE))
> + ;
> +
> + if (readl(&regs->ecc_test_ctrl) & SDRAM_TEST_FAIL) {
> + ret = -EIO;
> + } else {
> + for (i = 0; i <= SDRAM_TEST_GEN_MODE_MASK; ++i) {
> + writel((i << SDRAM_TEST_GEN_MODE_SHIFT) | test_params,
> + &regs->ecc_test_ctrl);
> + while (!(readl(&regs->ecc_test_ctrl) & SDRAM_TEST_DONE))
> + ;
> + if (readl(&regs->ecc_test_ctrl) & SDRAM_TEST_FAIL) {
> + ret = -EIO;
> + break;
> + }
> + }
> + }
> +
> + writel(0, &regs->refresh_timing);
> + writel(0, &regs->ecc_test_ctrl);

blank line before 'return' line.

> + return ret;
> +}
> +
> +static int ast2500_sdrammc_ddr4_calibrate_vref(struct dram_info *info)
> +{
> + int i;
> + int vref_min = 0xff;
> + int vref_max = 0;
> + int range_size = 0;
> +
> + for (i = 1; i < 0x40; ++i) {
> + ast2500_sdrammc_set_vref(info, i);
> +
> + int res = ast2500_ddr_cbr_test(info);
> + if (res < 0) {
> + if (range_size > 0)
> + break;
> + } else {
> + ++range_size;
> + vref_min = min(vref_min, i);
> + vref_max = max(vref_max, i);
> + }
> + }
> +
> + /* Pick average setting */
> + ast2500_sdrammc_set_vref(info, (vref_min + vref_max + 1) / 2);
> +
> + return 0;
> +}
> +
> +static size_t ast2500_sdrammc_get_vga_mem_size(struct dram_info *info)
> +{
> + size_t vga_mem_size_base = 8 * 1024 * 1024;
> + u32 vga_hwconf = (readl(&info->scu->hwstrap)
> + >> SCU_HWSTRAP_VGAMEM_SHIFT)
> + & SCU_HWSTRAP_VGAMEM_MASK;
> +
> + return vga_mem_size_base << vga_hwconf;
> +}
> +
> +/*
> + * Find out RAM size and save it in dram_info
> + *
> + * The procedure is taken from Aspeed SDK
> + */
> +static void ast2500_sdrammc_calc_size(struct dram_info *info)
> +{
> + /* The controller supports 128/256/512/1024 MB ram */
> + size_t ram_size = SDRAM_MIN_SIZE;
> + const int write_test_offset = 0x100000;
> + u32 test_pattern = 0xdeadbeef;
> + u32 cap_param = SDRAM_CONF_CAP_1024M;
> + u32 refresh_timing_param = DDR4_TRFC;
> + const u32 write_addr_base = CONFIG_SYS_SDRAM_BASE + write_test_offset;
> +
> + for (ram_size = SDRAM_MAX_SIZE; ram_size > SDRAM_MIN_SIZE;
> + ram_size >>= 1) {
> + writel(test_pattern, write_addr_base + (ram_size >> 1));
> + test_pattern = (test_pattern >> 4) | (test_pattern << 28);
> + }
> +
> + /* One last write to overwrite all wrapped values */
> + writel(test_pattern, write_addr_base);
> +
> + /* Reset the pattern and see which value was really written */
> + test_pattern = 0xdeadbeef;
> + for (ram_size = SDRAM_MAX_SIZE; ram_size > SDRAM_MIN_SIZE;
> + ram_size >>= 1) {
> + if (readl(write_addr_base + (ram_size >> 1)) == test_pattern)
> + break;
> +
> + --cap_param;
> + refresh_timing_param >>= 8;
> + test_pattern = (test_pattern >> 4) | (test_pattern << 28);
> + }
> +
> + clrsetbits_le32(&info->regs->ac_timing[1],
> + (SDRAM_AC_TRFC_MASK << SDRAM_AC_TRFC_SHIFT),
> + ((refresh_timing_param & SDRAM_AC_TRFC_MASK)
> + << SDRAM_AC_TRFC_SHIFT));
> +
> + info->info.base = CONFIG_SYS_SDRAM_BASE;
> + info->info.size = ram_size - ast2500_sdrammc_get_vga_mem_size(info);
> + clrsetbits_le32(&info->regs->config,
> + (SDRAM_CONF_CAP_MASK << SDRAM_CONF_CAP_SHIFT),
> + ((cap_param & SDRAM_CONF_CAP_MASK)
> + << SDRAM_CONF_CAP_SHIFT));
> +}
> +
> +static int ast2500_sdrammc_init_ddr4(struct dram_info *info)
> +{
> + int i;
> + const u32 power_control = SDRAM_PCR_CKE_EN
> + | (1 << SDRAM_PCR_CKE_DELAY_SHIFT)
> + | (2 << SDRAM_PCR_TCKE_PW_SHIFT)
> + | SDRAM_PCR_RESETN_DIS
> + | SDRAM_PCR_RGAP_CTRL_EN | SDRAM_PCR_ODT_EN | SDRAM_PCR_ODT_EXT_EN;
> + const u32 conf = (SDRAM_CONF_CAP_1024M << SDRAM_CONF_CAP_SHIFT)
> +#ifdef CONFIG_DUALX8_RAM
> + | SDRAM_CONF_DUALX8
> +#endif
> + | SDRAM_CONF_SCRAMBLE | SDRAM_CONF_SCRAMBLE_PAT2 | SDRAM_CONF_DDR4;
> +
> + writel(conf, &info->regs->config);
> + for (i = 0; i < ARRAY_SIZE(ddr4_ac_timing); ++i)
> + writel(ddr4_ac_timing[i], &info->regs->ac_timing[i]);
> +
> + writel(DDR4_MR46_MODE, &info->regs->mr46_mode_setting);
> + writel(DDR4_MR5_MODE, &info->regs->mr5_mode_setting);
> + writel(DDR4_MR02_MODE, &info->regs->mr02_mode_setting);
> + writel(DDR4_MR13_MODE, &info->regs->mr13_mode_setting);
> +
> + for (i = 0; i < PHY_CFG_SIZE; ++i) {
> + writel(ddr4_phy_config.value[i],
> + &info->phy->phy[ddr4_phy_config.index[i]]);
> + }
> +
> + writel(power_control, &info->regs->power_control);
> +
> + ast2500_ddr_phy_init_process(info);
> +
> + int ret = ast2500_sdrammc_ddr4_calibrate_vref(info);

please declare variables at the top of functions if you can, or inside
the block that needs them.

> + if (ret < 0) {
> + debug("Vref calibration failed!\n");
> + return ret;
> + }
> +
> + writel((1 << SDRAM_REFRESH_CYCLES_SHIFT)
> + | SDRAM_REFRESH_ZQCS_EN | (0x2f << SDRAM_REFRESH_PERIOD_SHIFT),
> + &info->regs->refresh_timing);
> +
> + setbits_le32(&info->regs->power_control,
> + SDRAM_PCR_AUTOPWRDN_EN | SDRAM_PCR_ODT_AUTO_ON);
> +
> + ast2500_sdrammc_calc_size(info);
> +
> + setbits_le32(&info->regs->config, SDRAM_CONF_CACHE_INIT_EN);
> + while (!(readl(&info->regs->config) & SDRAM_CONF_CACHE_INIT_DONE))
> + ;
> + setbits_le32(&info->regs->config, SDRAM_CONF_CACHE_EN);
> +
> + writel(SDRAM_MISC_DDR4_TREFRESH, &info->regs->misc_control);
> +
> + /* Enable all requests except video & display */
> + writel(SDRAM_REQ_USB20_EHCI1
> + | SDRAM_REQ_USB20_EHCI2
> + | SDRAM_REQ_CPU
> + | SDRAM_REQ_AHB2
> + | SDRAM_REQ_AHB
> + | SDRAM_REQ_MAC0
> + | SDRAM_REQ_MAC1
> + | SDRAM_REQ_PCIE
> + | SDRAM_REQ_XDMA
> + | SDRAM_REQ_ENCRYPTION
> + | SDRAM_REQ_VIDEO_FLAG
> + | SDRAM_REQ_VIDEO_LOW_PRI_WRITE
> + | SDRAM_REQ_2D_RW
> + | SDRAM_REQ_MEMCHECK, &info->regs->req_limit_mask);
> +
> + return 0;
> +}
> +
> +static void ast2500_sdrammc_unlock(struct dram_info *info)
> +{
> + writel(SDRAM_UNLOCK_KEY, &info->regs->protection_key);
> + while (!readl(&info->regs->protection_key))
> + ;
> +}
> +
> +static void ast2500_sdrammc_lock(struct dram_info *info)
> +{
> + writel(~SDRAM_UNLOCK_KEY, &info->regs->protection_key);
> + while (readl(&info->regs->protection_key))
> + ;
> +}
> +
> +static int ast2500_sdrammc_probe(struct udevice *dev)
> +{
> + struct dram_info *priv = (struct dram_info *)dev_get_priv(dev);
> + struct ast2500_sdrammc_regs *regs = priv->regs;
> + int ret = clk_get_by_index(dev, 0, &priv->ddr_clk);

Move this assignment to just before your 'if' below.

> + int i;
> +
> + if (ret) {
> + debug("DDR:No CLK\n");
> + return ret;
> + }
> +
> + priv->scu = ast_get_scu();
> + if (IS_ERR(priv->scu))

debug()

> + return PTR_ERR(priv->scu);
> +
> + clk_set_rate(&priv->ddr_clk, priv->clock_rate);
> + ret = ast2500_sdrammc_reset();
> + if (ret)

debug()

> + return ret;
> +
> + ast2500_sdrammc_unlock(priv);
> +
> + writel(SDRAM_PCR_MREQI_DIS | SDRAM_PCR_RESETN_DIS,
> + &regs->power_control);
> + writel(SDRAM_VIDEO_UNLOCK_KEY, &regs->gm_protection_key);
> +
> + /* Mask all requests except CPU and AHB during PHY init */
> + writel(~(SDRAM_REQ_CPU | SDRAM_REQ_AHB), &regs->req_limit_mask);
> +
> + for (i = 0; i < ARRAY_SIZE(ddr_max_grant_params); ++i)
> + writel(ddr_max_grant_params[i], &regs->max_grant_len[i]);
> +
> + setbits_le32(&regs->intr_ctrl, SDRAM_ICR_RESET_ALL);
> +
> + ast2500_sdrammc_init_phy(priv->phy);
> + if (readl(&priv->scu->hwstrap) & SCU_HWSTRAP_DDR4) {
> + ast2500_sdrammc_init_ddr4(priv);
> + } else {
> + debug("Unsupported DRAM3\n");
> + return -EINVAL;
> + }
> +
> + clrbits_le32(&regs->intr_ctrl, SDRAM_ICR_RESET_ALL);
> + ast2500_sdrammc_lock(priv);
> +
> + return 0;
> +}
> +
> +static int ast2500_sdrammc_ofdata_to_platdata(struct udevice *dev)
> +{
> + struct dram_info *priv = dev_get_priv(dev);
> + struct regmap *map;
> + int ret = regmap_init_mem(dev, &map);
> +

Move assignment or ret to here.

> + if (ret)
> + return ret;
> +
> + priv->regs = regmap_get_range(map, 0);
> + priv->phy = regmap_get_range(map, 1);
> +
> + priv->clock_rate = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
> + "clock-frequency", 0);
> +
> + if (!priv->clock_rate) {
> + debug("DDR Clock Rate not defined\n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int ast2500_sdrammc_get_info(struct udevice *dev, struct ram_info *info)
> +{
> + struct dram_info *priv = dev_get_priv(dev);
> +
> + *info = priv->info;
> +
> + return 0;
> +}
> +
> +static struct ram_ops ast2500_sdrammc_ops = {
> + .get_info = ast2500_sdrammc_get_info,
> +};
> +
> +static const struct udevice_id ast2500_sdrammc_ids[] = {
> + { .compatible = "aspeed,ast2500-sdrammc" },
> + { }
> +};
> +
> +U_BOOT_DRIVER(sdrammc_ast2500) = {
> + .name = "aspeed_ast2500_sdrammc",
> + .id = UCLASS_RAM,
> + .of_match = ast2500_sdrammc_ids,
> + .ops = &ast2500_sdrammc_ops,
> + .ofdata_to_platdata = ast2500_sdrammc_ofdata_to_platdata,
> + .probe = ast2500_sdrammc_probe,
> + .priv_auto_alloc_size = sizeof(struct dram_info),
> +};
> --
> 2.11.0.390.gc69c2f50cf-goog
>

Regards,
Simon