On Wed, Jan 4, 2017 at 11:46 AM, Maxim Sloyko maxims@google.com wrote:

Signed-off-by: Maxim Sloyko maxims@google.com

arch/arm/Kconfig | 7 +++++++ arch/arm/Makefile | 1 + arch/arm/mach-aspeed/Kconfig | 15 +++++++++++++++ arch/arm/mach-aspeed/Makefile | 8 ++++++++ 4 files changed, 31 insertions(+) create mode 100644 arch/arm/mach-aspeed/Kconfig create mode 100644 arch/arm/mach-aspeed/Makefile

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 714dd8b514..135c544335 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -882,8 +882,15 @@ config TARGET_THUNDERX_88XX select OF_CONTROL select SYS_CACHE_SHIFT_7

+config ARCH_ASPEED

•   bool "Support Aspeed SoCs"
  

•   select OF_CONTROL
  

•   select DM
  

endchoice

+source "arch/arm/mach-aspeed/Kconfig"

source "arch/arm/mach-at91/Kconfig"

source "arch/arm/mach-bcm283x/Kconfig" diff --git a/arch/arm/Makefile b/arch/arm/Makefile index 236debb452..cc73e1038e 100644 --- a/arch/arm/Makefile +++ b/arch/arm/Makefile @@ -50,6 +50,7 @@ PLATFORM_CPPFLAGS += $(arch-y) $(tune-y)

# Machine directory name. This list is sorted alphanumerically # by CONFIG_* macro name. +machine-$(CONFIG_ARCH_ASPEED) += aspeed machine-$(CONFIG_ARCH_AT91) += at91 machine-$(CONFIG_ARCH_BCM283X) += bcm283x machine-$(CONFIG_ARCH_DAVINCI) += davinci diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach-aspeed/Kconfig new file mode 100644 index 0000000000..43cdbeda84 --- /dev/null +++ b/arch/arm/mach-aspeed/Kconfig @@ -0,0 +1,15 @@ +if ARCH_ASPEED

+config SYS_ARCH

•   default "arm"
  

+config SYS_SOC

•   default "aspeed"
  

+config ASPEED_AST2500

•   bool "Support Aspeed AST2500 SoC"
  

•   select CPU_ARM1176
  

•   help
  

•     The Aspeed AST2500 is a ARM-based SoC with arm1176 CPU
  

+endif diff --git a/arch/arm/mach-aspeed/Makefile b/arch/arm/mach-aspeed/Makefile new file mode 100644 index 0000000000..8e276b4a9f --- /dev/null +++ b/arch/arm/mach-aspeed/Makefile @@ -0,0 +1,8 @@ +# +# Copyright (c) 2014 Google, Inc +# +# SPDX-License-Identifier: GPL-2.0+ +#

+obj-$(CONFIG_ARCH_ASPEED) += ast_wdt.o +obj-$(CONFIG_ASPEED_AST2500) += ast2500/ ast2500-board.o

These files don't exist yet.

-- 2.11.0.390.gc69c2f50cf-goog

Hi Maxim,

On 4 January 2017 at 12:46, Maxim Sloyko maxims@google.com wrote:

Signed-off-by: Maxim Sloyko maxims@google.com

arch/arm/Kconfig | 7 +++++++ arch/arm/Makefile | 1 + arch/arm/mach-aspeed/Kconfig | 15 +++++++++++++++ arch/arm/mach-aspeed/Makefile | 8 ++++++++ 4 files changed, 31 insertions(+) create mode 100644 arch/arm/mach-aspeed/Kconfig create mode 100644 arch/arm/mach-aspeed/Makefile

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 714dd8b514..135c544335 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -882,8 +882,15 @@ config TARGET_THUNDERX_88XX select OF_CONTROL select SYS_CACHE_SHIFT_7

+config ARCH_ASPEED

•   bool "Support Aspeed SoCs"
  

•   select OF_CONTROL
  

•   select DM
  

endchoice

+source "arch/arm/mach-aspeed/Kconfig"

source "arch/arm/mach-at91/Kconfig"

source "arch/arm/mach-bcm283x/Kconfig" diff --git a/arch/arm/Makefile b/arch/arm/Makefile index 236debb452..cc73e1038e 100644 --- a/arch/arm/Makefile +++ b/arch/arm/Makefile @@ -50,6 +50,7 @@ PLATFORM_CPPFLAGS += $(arch-y) $(tune-y)

# Machine directory name. This list is sorted alphanumerically # by CONFIG_* macro name. +machine-$(CONFIG_ARCH_ASPEED) += aspeed machine-$(CONFIG_ARCH_AT91) += at91 machine-$(CONFIG_ARCH_BCM283X) += bcm283x machine-$(CONFIG_ARCH_DAVINCI) += davinci diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach-aspeed/Kconfig new file mode 100644 index 0000000000..43cdbeda84 --- /dev/null +++ b/arch/arm/mach-aspeed/Kconfig @@ -0,0 +1,15 @@ +if ARCH_ASPEED

+config SYS_ARCH

•   default "arm"
  

+config SYS_SOC

•   default "aspeed"
  

+config ASPEED_AST2500

•   bool "Support Aspeed AST2500 SoC"
  

•   select CPU_ARM1176
  

•   help
  

•     The Aspeed AST2500 is a ARM-based SoC with arm1176 CPU
  

Can you please expand this a bit? E.g. a summary of peripherals it has, what the chip is used for...

+endif diff --git a/arch/arm/mach-aspeed/Makefile b/arch/arm/mach-aspeed/Makefile new file mode 100644 index 0000000000..8e276b4a9f --- /dev/null +++ b/arch/arm/mach-aspeed/Makefile @@ -0,0 +1,8 @@ +# +# Copyright (c) 2014 Google, Inc +# +# SPDX-License-Identifier: GPL-2.0+ +#

+obj-$(CONFIG_ARCH_ASPEED) += ast_wdt.o

+obj-$(CONFIG_ASPEED_AST2500) += ast2500/ ast2500-board.o

2.11.0.390.gc69c2f50cf-goog

Regards, Simon

Hi Maxim,

On 4 January 2017 at 12:46, Maxim Sloyko maxims@google.com wrote:

The driver is compatible with AST2400 and AST2500 watchdogs. There is no uclass for Watchdog yet, so the driver does not follow the driver model. It also uses fixed clock, so no clock driver is needed.

# Conflicts: # arch/arm/mach-aspeed/Makefile

Signed-off-by: Maxim Sloyko maxims@google.com

arch/arm/include/asm/arch-aspeed/wdt.h | 89 ++++++++++++++++++++++++++++++++++ arch/arm/mach-aspeed/Makefile | 3 +- arch/arm/mach-aspeed/ast_wdt.c | 44 +++++++++++++++++ 3 files changed, 134 insertions(+), 2 deletions(-) create mode 100644 arch/arm/include/asm/arch-aspeed/wdt.h create mode 100644 arch/arm/mach-aspeed/ast_wdt.c

diff --git a/arch/arm/include/asm/arch-aspeed/wdt.h b/arch/arm/include/asm/arch-aspeed/wdt.h new file mode 100644 index 0000000000..32774b1a70 --- /dev/null +++ b/arch/arm/include/asm/arch-aspeed/wdt.h @@ -0,0 +1,89 @@ +/*

• • (C) Copyright 2016 Google, Inc
• • SPDX-License-Identifier: GPL-2.0+
• */

+#ifndef _ASM_ARCH_WDT_H +#define _ASM_ARCH_WDT_H

+#define WDT_BASE 0x1e785000

This should come from DT. I suggest creating a simple uclass for the watchdog timer, so you can make this a proper driver-model driver.

Regards, Simon

Hi Simon,

This is older series, that I merged into a smaller number of patches and already addressed all of Tom's comments: http://lists.denx.de/pipermail/u-boot/2017-January/277883.html

Most of your comments still apply, just want to make sure we are on the same page.

On Sat, Jan 14, 2017 at 9:13 AM, Simon Glass sjg@chromium.org wrote:

Hi Maxim,

On 4 January 2017 at 12:46, Maxim Sloyko maxims@google.com wrote:

...

Add support for timer for Aspeed ast2400/ast2500 devices. The driver actually controls several devices, but because all devices share the same Control Register, it is somewhat difficult to completely decouple them. Since only one timer is needed at the moment, this should be OK.

The timer uses fixed clock, so does not rely on a clock driver.

Signed-off-by: Maxim Sloyko maxims@google.com

arch/arm/include/asm/arch-aspeed/timer.h | 54 ++++++++++++++++++ drivers/timer/Kconfig | 7 +++ drivers/timer/Makefile | 1 + drivers/timer/ast_timer.c | 96

++++++++++++++++++++++++++++++++

...

4 files changed, 158 insertions(+) create mode 100644 arch/arm/include/asm/arch-aspeed/timer.h create mode 100644 drivers/timer/ast_timer.c

Reviewed-by: Simon Glass sjg@chromium.org

nits below.

...

diff --git a/arch/arm/include/asm/arch-aspeed/timer.h

b/arch/arm/include/asm/arch-aspeed/timer.h

...

new file mode 100644 index 0000000000..87c5b354ec --- /dev/null +++ b/arch/arm/include/asm/arch-aspeed/timer.h @@ -0,0 +1,54 @@ +/*

• • Copyright (c) 2016 Google, Inc
• • SPDX-License-Identifier: GPL-2.0+
• */

+#ifndef _ASM_ARCH_TIMER_H +#define _ASM_ARCH_TIMER_H

+/* Each timer has 4 control bits in ctrl1 register.

• • Timer1 uses bits 0:3, Timer2 uses bits 4:7 and so on,
• • such that timer X uses bits (4 * X - 4):(4 * X - 1)
• • If the timer does not support PWM, bit 4 is reserved.
• */

+#define AST_TMC_EN (1 << 0) +#define AST_TMC_1MHZ (1 << 1) +#define AST_TMC_OVFINTR (1 << 2) +#define AST_TMC_PWM (1 << 3)

+/* Timers are counted from 1 in the datasheet. */ +#define AST_TMC_CTRL1_SHIFT(n) (4 * ((n) - 1))

+#define AST_TMC_RATE (1000*1000)

+#ifndef __ASSEMBLY__

+/*

• • All timers share control registers, which makes it harder to make

them

...

• • separate devices. Since only one timer is needed at the moment,

making

...

• • it this just one device.
• */

+struct ast_timer_counter {

•   u32 status;
  

•   u32 reload_val;
  

•   u32 match1;
  

•   u32 match2;
  

+};

+struct ast_timer {

•   struct ast_timer_counter timers1[3];
  

•   u32 ctrl1;
  

•   u32 ctrl2;
  

+#ifdef CONFIG_ASPEED_AST2500

•   u32 ctrl3;
  

•   u32 ctrl1_clr;
  

+#else

•   u32 reserved[2];
  

+#endif

•   struct ast_timer_counter timers2[5];
  

+};

+#endif /* __ASSEMBLY__ */

+#endif /* _ASM_ARCH_TIMER_H */ diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig index cb18f12fc9..9c5f98bb88 100644 --- a/drivers/timer/Kconfig +++ b/drivers/timer/Kconfig @@ -46,4 +46,11 @@ config OMAP_TIMER help Select this to enable an timer for Omap devices.

+config AST_TIMER

•   bool "Aspeed ast2400/ast2500 timer support"
  

•   depends on TIMER
  

•   default y if ARCH_ASPEED
  

•   help
  

•     Select this to enable timer for Aspeed ast2400/ast2500 devices.
  

Can you add more detail? How many channels? What features are supported by the driver?

...

endmenu diff --git a/drivers/timer/Makefile b/drivers/timer/Makefile index f351fbb4e0..a4b1a486b0 100644 --- a/drivers/timer/Makefile +++ b/drivers/timer/Makefile @@ -9,3 +9,4 @@ obj-$(CONFIG_ALTERA_TIMER) += altera_timer.o obj-$(CONFIG_SANDBOX_TIMER) += sandbox_timer.o obj-$(CONFIG_X86_TSC_TIMER) += tsc_timer.o obj-$(CONFIG_OMAP_TIMER) += omap-timer.o +obj-$(CONFIG_AST_TIMER) += ast_timer.o diff --git a/drivers/timer/ast_timer.c b/drivers/timer/ast_timer.c new file mode 100644 index 0000000000..f644882f40 --- /dev/null +++ b/drivers/timer/ast_timer.c @@ -0,0 +1,96 @@ +/*

• • Copyright 2016 Google Inc.
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <dm.h> +#include <errno.h> +#include <timer.h> +#include <asm/io.h> +#include <asm/arch/timer.h>

+DECLARE_GLOBAL_DATA_PTR;

+#define AST_TICK_TIMER 1 +#define AST_TMC_RELOAD_VAL 0xffffffff

+struct ast_timer_priv {

•   struct ast_timer *regs;
  

+};

+static struct ast_timer_counter *ast_get_timer_counter(struct ast_timer

*timer,

...

•                                                  int n)
  

+{

•   if (n > 3)
  

•           return &timer->timers2[n - 4];
  

•   else
  

•           return &timer->timers1[n - 1];
  

+}

+static int ast_timer_probe(struct udevice *dev) +{

•   struct ast_timer_priv *priv = dev_get_priv(dev);
  

•   struct ast_timer_counter *tmc = ast_get_timer_counter(priv->
  

regs,

...

AST_TICK_TIMER);

I suppose tmc could be in your struct ast_timer_priv to save you doing this each time?

...

•   struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
  

•   writel(AST_TMC_RELOAD_VAL, &tmc->reload_val);
  

•   /*
  

•    * Stop the timer. This will also load reload_val into
  

•    * the status register.
  

•    */
  

•   clrbits_le32(&priv->regs->ctrl1,
  

•                AST_TMC_EN << AST_TMC_CTRL1_SHIFT(AST_TICK_TIMER));
  

•   /* Start the timer from the fixed 1MHz clock. */
  

•   setbits_le32(&priv->regs->ctrl1,
  

•                (AST_TMC_EN | AST_TMC_1MHZ) <<
  

•                AST_TMC_CTRL1_SHIFT(AST_TICK_TIMER));
  

•   uc_priv->clock_rate = AST_TMC_RATE;
  

•   return 0;
  

+}

+static int ast_timer_get_count(struct udevice *dev, u64 *count) +{

•   struct ast_timer_priv *priv = dev_get_priv(dev);
  

•   struct ast_timer_counter *tmc = ast_get_timer_counter(priv->
  

regs,

...

AST_TICK_TIMER);

...

•   *count = AST_TMC_RELOAD_VAL - readl(&tmc->status);
  

•   return 0;
  

+}

+static int ast_timer_ofdata_to_platdata(struct udevice *dev) +{

•   struct ast_timer_priv *priv = dev_get_priv(dev);
  

•   priv->regs = (struct ast_timer *)dev_get_addr(dev);
  

You can use dev_get_addr_ptr() if you like.

...

•   return 0;
  

+}

+static const struct timer_ops ast_timer_ops = {

•   .get_count = ast_timer_get_count,
  

+};

+static const struct udevice_id ast_timer_ids[] = {

•   { .compatible = "aspeed,ast2500-timer" },
  

•   { .compatible = "aspeed,ast2400-timer" },
  

•   { }
  

+};

+U_BOOT_DRIVER(sandbox_timer) = {

s/sandbox/ast/ or something, as Tom, mentioned.

...

•   .name = "ast_timer",
  

•   .id = UCLASS_TIMER,
  

•   .of_match = ast_timer_ids,
  

•   .probe = ast_timer_probe,
  

•   .priv_auto_alloc_size = sizeof(struct ast_timer_priv),
  

•   .ofdata_to_platdata = ast_timer_ofdata_to_platdata,
  

•   .ops = &ast_timer_ops,
  

•   .flags = DM_FLAG_PRE_RELOC,
  

+};

2.11.0.390.gc69c2f50cf-goog

Regards, Simon

...

On Sat, Jan 14, 2017 at 9:13 AM, Simon Glass sjg@chromium.org wrote:

Hi Maxim,

On 4 January 2017 at 12:46, Maxim Sloyko maxims@google.com wrote:

...

Add support for timer for Aspeed ast2400/ast2500 devices. The driver actually controls several devices, but because all devices share the same Control Register, it is somewhat difficult to completely decouple them. Since only one timer is needed at the moment, this should be OK.

The timer uses fixed clock, so does not rely on a clock driver.

Signed-off-by: Maxim Sloyko maxims@google.com

arch/arm/include/asm/arch-aspeed/timer.h | 54 ++++++++++++++++++ drivers/timer/Kconfig | 7 +++ drivers/timer/Makefile | 1 + drivers/timer/ast_timer.c | 96

++++++++++++++++++++++++++++++++

...

4 files changed, 158 insertions(+) create mode 100644 arch/arm/include/asm/arch-aspeed/timer.h create mode 100644 drivers/timer/ast_timer.c

Reviewed-by: Simon Glass sjg@chromium.org

nits below.

...

diff --git a/arch/arm/include/asm/arch-aspeed/timer.h

b/arch/arm/include/asm/arch-aspeed/timer.h

...

new file mode 100644 index 0000000000..87c5b354ec --- /dev/null +++ b/arch/arm/include/asm/arch-aspeed/timer.h @@ -0,0 +1,54 @@ +/*

• • Copyright (c) 2016 Google, Inc
• • SPDX-License-Identifier: GPL-2.0+
• */

+#ifndef _ASM_ARCH_TIMER_H +#define _ASM_ARCH_TIMER_H

+/* Each timer has 4 control bits in ctrl1 register.

• • Timer1 uses bits 0:3, Timer2 uses bits 4:7 and so on,
• • such that timer X uses bits (4 * X - 4):(4 * X - 1)
• • If the timer does not support PWM, bit 4 is reserved.
• */

+#define AST_TMC_EN (1 << 0) +#define AST_TMC_1MHZ (1 << 1) +#define AST_TMC_OVFINTR (1 << 2) +#define AST_TMC_PWM (1 << 3)

+/* Timers are counted from 1 in the datasheet. */ +#define AST_TMC_CTRL1_SHIFT(n) (4 * ((n) - 1))

+#define AST_TMC_RATE (1000*1000)

+#ifndef __ASSEMBLY__

+/*

• • All timers share control registers, which makes it harder to make

them

...

• • separate devices. Since only one timer is needed at the moment,

making

...

• • it this just one device.
• */

+struct ast_timer_counter {

•   u32 status;
  

•   u32 reload_val;
  

•   u32 match1;
  

•   u32 match2;
  

+};

+struct ast_timer {

•   struct ast_timer_counter timers1[3];
  

•   u32 ctrl1;
  

•   u32 ctrl2;
  

+#ifdef CONFIG_ASPEED_AST2500

•   u32 ctrl3;
  

•   u32 ctrl1_clr;
  

+#else

•   u32 reserved[2];
  

+#endif

•   struct ast_timer_counter timers2[5];
  

+};

+#endif /* __ASSEMBLY__ */

+#endif /* _ASM_ARCH_TIMER_H */ diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig index cb18f12fc9..9c5f98bb88 100644 --- a/drivers/timer/Kconfig +++ b/drivers/timer/Kconfig @@ -46,4 +46,11 @@ config OMAP_TIMER help Select this to enable an timer for Omap devices.

+config AST_TIMER

•   bool "Aspeed ast2400/ast2500 timer support"
  

•   depends on TIMER
  

•   default y if ARCH_ASPEED
  

•   help
  

•     Select this to enable timer for Aspeed ast2400/ast2500 devices.
  

Can you add more detail? How many channels? What features are supported by the driver?

Expanded the description.

...

endmenu diff --git a/drivers/timer/Makefile b/drivers/timer/Makefile index f351fbb4e0..a4b1a486b0 100644 --- a/drivers/timer/Makefile +++ b/drivers/timer/Makefile @@ -9,3 +9,4 @@ obj-$(CONFIG_ALTERA_TIMER) += altera_timer.o obj-$(CONFIG_SANDBOX_TIMER) += sandbox_timer.o obj-$(CONFIG_X86_TSC_TIMER) += tsc_timer.o obj-$(CONFIG_OMAP_TIMER) += omap-timer.o +obj-$(CONFIG_AST_TIMER) += ast_timer.o diff --git a/drivers/timer/ast_timer.c b/drivers/timer/ast_timer.c new file mode 100644 index 0000000000..f644882f40 --- /dev/null +++ b/drivers/timer/ast_timer.c @@ -0,0 +1,96 @@ +/*

• • Copyright 2016 Google Inc.
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <dm.h> +#include <errno.h> +#include <timer.h> +#include <asm/io.h> +#include <asm/arch/timer.h>

+DECLARE_GLOBAL_DATA_PTR;

+#define AST_TICK_TIMER 1 +#define AST_TMC_RELOAD_VAL 0xffffffff

+struct ast_timer_priv {

•   struct ast_timer *regs;
  

+};

+static struct ast_timer_counter *ast_get_timer_counter(struct ast_timer

*timer,

...

•                                                  int n)
  

+{

•   if (n > 3)
  

•           return &timer->timers2[n - 4];
  

•   else
  

•           return &timer->timers1[n - 1];
  

+}

+static int ast_timer_probe(struct udevice *dev) +{

•   struct ast_timer_priv *priv = dev_get_priv(dev);
  

•   struct ast_timer_counter *tmc = ast_get_timer_counter(priv->
  

regs,

...

AST_TICK_TIMER);

I suppose tmc could be in your struct ast_timer_priv to save you doing this each time?

Fixed.

...

•   struct timer_dev_priv *uc_priv = dev_get_uclass_priv(dev);
  

•   writel(AST_TMC_RELOAD_VAL, &tmc->reload_val);
  

•   /*
  

•    * Stop the timer. This will also load reload_val into
  

•    * the status register.
  

•    */
  

•   clrbits_le32(&priv->regs->ctrl1,
  

•                AST_TMC_EN << AST_TMC_CTRL1_SHIFT(AST_TICK_TIMER));
  

•   /* Start the timer from the fixed 1MHz clock. */
  

•   setbits_le32(&priv->regs->ctrl1,
  

•                (AST_TMC_EN | AST_TMC_1MHZ) <<
  

•                AST_TMC_CTRL1_SHIFT(AST_TICK_TIMER));
  

•   uc_priv->clock_rate = AST_TMC_RATE;
  

•   return 0;
  

+}

+static int ast_timer_get_count(struct udevice *dev, u64 *count) +{

•   struct ast_timer_priv *priv = dev_get_priv(dev);
  

•   struct ast_timer_counter *tmc = ast_get_timer_counter(priv->
  

regs,

...

AST_TICK_TIMER);

...

•   *count = AST_TMC_RELOAD_VAL - readl(&tmc->status);
  

•   return 0;
  

+}

+static int ast_timer_ofdata_to_platdata(struct udevice *dev) +{

•   struct ast_timer_priv *priv = dev_get_priv(dev);
  

•   priv->regs = (struct ast_timer *)dev_get_addr(dev);
  

You can use dev_get_addr_ptr() if you like.

Done.

...

•   return 0;
  

+}

+static const struct timer_ops ast_timer_ops = {

•   .get_count = ast_timer_get_count,
  

+};

+static const struct udevice_id ast_timer_ids[] = {

•   { .compatible = "aspeed,ast2500-timer" },
  

•   { .compatible = "aspeed,ast2400-timer" },
  

•   { }
  

+};

+U_BOOT_DRIVER(sandbox_timer) = {

s/sandbox/ast/ or something, as Tom, mentioned.

Addressed earlier.

...

•   .name = "ast_timer",
  

•   .id = UCLASS_TIMER,
  

•   .of_match = ast_timer_ids,
  

•   .probe = ast_timer_probe,
  

•   .priv_auto_alloc_size = sizeof(struct ast_timer_priv),
  

•   .ofdata_to_platdata = ast_timer_ofdata_to_platdata,
  

•   .ops = &ast_timer_ops,
  

•   .flags = DM_FLAG_PRE_RELOC,
  

+};

2.11.0.390.gc69c2f50cf-goog

Regards, Simon

...

On Wed, Jan 04, 2017 at 11:46:49AM -0800, Maxim Sloyko wrote:

Signed-off-by: Maxim Sloyko maxims@google.com

arch/arm/dts/ast2500.dtsi | 423 ++++++++++++++++++++++++++++++++ include/dt-bindings/clock/ast2500-scu.h | 29 +++ 2 files changed, 452 insertions(+) create mode 100644 arch/arm/dts/ast2500.dtsi create mode 100644 include/dt-bindings/clock/ast2500-scu.h

diff --git a/arch/arm/dts/ast2500.dtsi b/arch/arm/dts/ast2500.dtsi new file mode 100644 index 0000000000..1a2a3f7ee3 --- /dev/null +++ b/arch/arm/dts/ast2500.dtsi @@ -0,0 +1,423 @@ +/* This device tree is copied from

• • https://github.com/openbmc/linux/blob/c5682cb/arch/arm/boot/dts/aspeed-g5.dt...

Is this also found in the Linux kernel or not yet? Thanks!

On Wed, Jan 04, 2017 at 05:18:42PM -0800, Maxim Sloyko wrote:

On Wed, Jan 4, 2017 at 12:58 PM, Tom Rini trini@konsulko.com wrote:

...

On Wed, Jan 04, 2017 at 11:46:49AM -0800, Maxim Sloyko wrote:

...

Signed-off-by: Maxim Sloyko maxims@google.com

arch/arm/dts/ast2500.dtsi | 423 ++++++++++++++++++++++++++++++++ include/dt-bindings/clock/ast2500-scu.h | 29 +++ 2 files changed, 452 insertions(+) create mode 100644 arch/arm/dts/ast2500.dtsi create mode 100644 include/dt-bindings/clock/ast2500-scu.h

diff --git a/arch/arm/dts/ast2500.dtsi b/arch/arm/dts/ast2500.dtsi new file mode 100644 index 0000000000..1a2a3f7ee3 --- /dev/null +++ b/arch/arm/dts/ast2500.dtsi @@ -0,0 +1,423 @@ +/* This device tree is copied from

• • https://github.com/openbmc/linux/blob/c5682cb/arch/arm/boot/dts/aspeed-g5.dt...

Is this also found in the Linux kernel or not yet? Thanks!

Yes, this is also in in main Linux kernel now, as I've found out, but actually there is a number of differences, most notably there is no pin configuration in this device tree, because there is no pinctrl driver.

Should I remove this reference or modify it?

Ideally, we will take the kernel dts files and then add what we need on top of that in one of CONFIG_SYS_CPU/CONFIG_SYS_SOC/CONFIG_SYS_VENDOR -u-boot.dtsi files, see for example arch/arm/dts/sunxi-u-boot.dtsi or arch/arm/dts/tegra124-nyan-big-u-boot.dtsi

Hi Maxim,

On 5 January 2017 at 15:20, Maxim Sloyko maxims@google.com wrote:

On Wed, Jan 4, 2017 at 7:26 PM, Tom Rini trini@konsulko.com wrote:

...

On Wed, Jan 04, 2017 at 05:18:42PM -0800, Maxim Sloyko wrote:

...

On Wed, Jan 4, 2017 at 12:58 PM, Tom Rini trini@konsulko.com wrote:

...

On Wed, Jan 04, 2017 at 11:46:49AM -0800, Maxim Sloyko wrote:

...

Signed-off-by: Maxim Sloyko maxims@google.com

arch/arm/dts/ast2500.dtsi | 423 ++++++++++++++++++++++++++++++++ include/dt-bindings/clock/ast2500-scu.h | 29 +++ 2 files changed, 452 insertions(+) create mode 100644 arch/arm/dts/ast2500.dtsi create mode 100644 include/dt-bindings/clock/ast2500-scu.h

diff --git a/arch/arm/dts/ast2500.dtsi b/arch/arm/dts/ast2500.dtsi new file mode 100644 index 0000000000..1a2a3f7ee3 --- /dev/null +++ b/arch/arm/dts/ast2500.dtsi @@ -0,0 +1,423 @@ +/* This device tree is copied from

• • https://github.com/openbmc/linux/blob/c5682cb/arch/arm/boot/dts/aspeed-g5.dt...

Is this also found in the Linux kernel or not yet? Thanks!

Yes, this is also in in main Linux kernel now, as I've found out, but actually there is a number of differences, most notably there is no pin configuration in this device tree, because there is no pinctrl driver.

Actually, I take that back, I was looking at the wrong linux Linux kernel tree still... Only basic version of device tree has made it to mainline kernel, but it's enough at the moment, so I used that instead.

...

...

Should I remove this reference or modify it?

Ideally, we will take the kernel dts files and then add what we need on top of that in one of CONFIG_SYS_CPU/CONFIG_SYS_SOC/CONFIG_SYS_VENDOR -u-boot.dtsi files, see for example arch/arm/dts/sunxi-u-boot.dtsi or arch/arm/dts/tegra124-nyan-big-u-boot.dtsi

OK, so I took the device tree from the Linux kernel, (ast2500.dtsi), added modifications in ast2500-u-boot.dtsi and now include ast2500-u-boot.dtsi in ast2500-evb.dts. Let me know if I misunderstood you.

There is some magic in the Makefile which automatically includes the .dtsi if you name it correctly:

# Try these files in order to find the U-Boot-specific .dtsi include file u_boot_dtsi_options = $(wildcard $(dts_dir)/$(basename $(notdir $<))-u-boot.dtsi) \ $(wildcard $(dts_dir)/$(subst $",,$(CONFIG_SYS_SOC))-u-boot.dtsi) \ $(wildcard $(dts_dir)/$(subst $",,$(CONFIG_SYS_CPU))-u-boot.dtsi) \ $(wildcard $(dts_dir)/$(subst $",,$(CONFIG_SYS_VENDOR))-u-boot.dtsi) \ $(wildcard $(dts_dir)/u-boot.dtsi)

So you should not need to include it explicitly.

Regards, Simon

Hi Maxim,

On 17 January 2017 at 16:27, Maxim Sloyko maxims@google.com wrote:

On Sat, Jan 14, 2017 at 9:13 AM, Simon Glass sjg@chromium.org wrote:

...

Hi Maxim,

On 5 January 2017 at 15:20, Maxim Sloyko maxims@google.com wrote:

...

On Wed, Jan 4, 2017 at 7:26 PM, Tom Rini trini@konsulko.com wrote:

...

On Wed, Jan 04, 2017 at 05:18:42PM -0800, Maxim Sloyko wrote:

...

On Wed, Jan 4, 2017 at 12:58 PM, Tom Rini trini@konsulko.com wrote:

...

On Wed, Jan 04, 2017 at 11:46:49AM -0800, Maxim Sloyko wrote:

> Signed-off-by: Maxim Sloyko maxims@google.com > --- > > arch/arm/dts/ast2500.dtsi | 423 > ++++++++++++++++++++++++++++++++ > include/dt-bindings/clock/ast2500-scu.h | 29 +++ > 2 files changed, 452 insertions(+) > create mode 100644 arch/arm/dts/ast2500.dtsi > create mode 100644 include/dt-bindings/clock/ast2500-scu.h > > diff --git a/arch/arm/dts/ast2500.dtsi b/arch/arm/dts/ast2500.dtsi > new file mode 100644 > index 0000000000..1a2a3f7ee3 > --- /dev/null > +++ b/arch/arm/dts/ast2500.dtsi > @@ -0,0 +1,423 @@ > +/* This device tree is copied from > + * > https://github.com/openbmc/linux/blob/c5682cb/arch/arm/boot/dts/aspeed-g5.dt...

Is this also found in the Linux kernel or not yet? Thanks!

Yes, this is also in in main Linux kernel now, as I've found out, but actually there is a number of differences, most notably there is no pin configuration in this device tree, because there is no pinctrl driver.

Actually, I take that back, I was looking at the wrong linux Linux kernel tree still... Only basic version of device tree has made it to mainline kernel, but it's enough at the moment, so I used that instead.

...

...

Should I remove this reference or modify it?

Ideally, we will take the kernel dts files and then add what we need on top of that in one of CONFIG_SYS_CPU/CONFIG_SYS_SOC/CONFIG_SYS_VENDOR -u-boot.dtsi files, see for example arch/arm/dts/sunxi-u-boot.dtsi or arch/arm/dts/tegra124-nyan-big-u-boot.dtsi

OK, so I took the device tree from the Linux kernel, (ast2500.dtsi), added modifications in ast2500-u-boot.dtsi and now include ast2500-u-boot.dtsi in ast2500-evb.dts. Let me know if I misunderstood you.

There is some magic in the Makefile which automatically includes the .dtsi if you name it correctly:

# Try these files in order to find the U-Boot-specific .dtsi include file u_boot_dtsi_options = $(wildcard $(dts_dir)/$(basename $(notdir $<))-u-boot.dtsi) \ $(wildcard $(dts_dir)/$(subst $",,$(CONFIG_SYS_SOC))-u-boot.dtsi) \ $(wildcard $(dts_dir)/$(subst $",,$(CONFIG_SYS_CPU))-u-boot.dtsi) \ $(wildcard $(dts_dir)/$(subst $",,$(CONFIG_SYS_VENDOR))-u-boot.dtsi) \ $(wildcard $(dts_dir)/u-boot.dtsi)

So you should not need to include it explicitly.

Well, it looks like the only applicable for this case would be CONFIG_SYS_SOC, but it looks like this setting also affects the mach- and arch- directories used. In this case I want different aspeed SoCs to share those directories, but they will have different DTs.

The first option is to a .dtsi derived from the .dts filename. Does that help?

Regards, Simon

Hi Maxim,

On 23 January 2017 at 10:52, Maxim Sloyko maxims@google.com wrote:

On Fri, Jan 20, 2017 at 7:52 PM, Simon Glass sjg@chromium.org wrote:

...

Hi Maxim,

On 17 January 2017 at 16:27, Maxim Sloyko maxims@google.com wrote:

...

On Sat, Jan 14, 2017 at 9:13 AM, Simon Glass sjg@chromium.org wrote:

...

Hi Maxim,

On 5 January 2017 at 15:20, Maxim Sloyko maxims@google.com wrote:

...

On Wed, Jan 4, 2017 at 7:26 PM, Tom Rini trini@konsulko.com wrote:

...

On Wed, Jan 04, 2017 at 05:18:42PM -0800, Maxim Sloyko wrote: > On Wed, Jan 4, 2017 at 12:58 PM, Tom Rini trini@konsulko.com wrote: > > On Wed, Jan 04, 2017 at 11:46:49AM -0800, Maxim Sloyko wrote: > > > >> Signed-off-by: Maxim Sloyko maxims@google.com > >> --- > >> > >> arch/arm/dts/ast2500.dtsi | 423 > >> ++++++++++++++++++++++++++++++++ > >> include/dt-bindings/clock/ast2500-scu.h | 29 +++ > >> 2 files changed, 452 insertions(+) > >> create mode 100644 arch/arm/dts/ast2500.dtsi > >> create mode 100644 include/dt-bindings/clock/ast2500-scu.h > >> > >> diff --git a/arch/arm/dts/ast2500.dtsi b/arch/arm/dts/ast2500.dtsi > >> new file mode 100644 > >> index 0000000000..1a2a3f7ee3 > >> --- /dev/null > >> +++ b/arch/arm/dts/ast2500.dtsi > >> @@ -0,0 +1,423 @@ > >> +/* This device tree is copied from > >> + * > >> https://github.com/openbmc/linux/blob/c5682cb/arch/arm/boot/dts/aspeed-g5.dt... > > > > Is this also found in the Linux kernel or not yet? Thanks! > > Yes, this is also in in main Linux kernel now, as I've found out, but > actually there is a number of differences, most notably there is no > pin configuration in this device tree, because there is no pinctrl > driver.

Actually, I take that back, I was looking at the wrong linux Linux kernel tree still... Only basic version of device tree has made it to mainline kernel, but it's enough at the moment, so I used that instead.

...

> > Should I remove this reference or modify it?

Ideally, we will take the kernel dts files and then add what we need on top of that in one of CONFIG_SYS_CPU/CONFIG_SYS_SOC/CONFIG_SYS_VENDOR -u-boot.dtsi files, see for example arch/arm/dts/sunxi-u-boot.dtsi or arch/arm/dts/tegra124-nyan-big-u-boot.dtsi

OK, so I took the device tree from the Linux kernel, (ast2500.dtsi), added modifications in ast2500-u-boot.dtsi and now include ast2500-u-boot.dtsi in ast2500-evb.dts. Let me know if I misunderstood you.

There is some magic in the Makefile which automatically includes the .dtsi if you name it correctly:

# Try these files in order to find the U-Boot-specific .dtsi include file u_boot_dtsi_options = $(wildcard $(dts_dir)/$(basename $(notdir $<))-u-boot.dtsi) \ $(wildcard $(dts_dir)/$(subst $",,$(CONFIG_SYS_SOC))-u-boot.dtsi) \ $(wildcard $(dts_dir)/$(subst $",,$(CONFIG_SYS_CPU))-u-boot.dtsi) \ $(wildcard $(dts_dir)/$(subst $",,$(CONFIG_SYS_VENDOR))-u-boot.dtsi) \ $(wildcard $(dts_dir)/u-boot.dtsi)

So you should not need to include it explicitly.

Well, it looks like the only applicable for this case would be CONFIG_SYS_SOC, but it looks like this setting also affects the mach- and arch- directories used. In this case I want different aspeed SoCs to share those directories, but they will have different DTs.

The first option is to a .dtsi derived from the .dts filename. Does that help?

Hmm, probably not... if I understand that correctly.

This is what I have right now:

ast2500.dtsi -- device tree from the linux kernel, used by all ast2500-based boards. ast2500-u-boot.dtsi -- these are u-boot specific additions, with u-boot,dm-pre-reloc labels, clock specification and SDRAM driver node. These are still need to be shared by all ast2500-based boards. ast2500-evb.dts -- Eval board specific stuff, right now basically just chosen node with stdout-path and clock frequency specification for SDRAM.

I don't this it would make sense to have ast2500-evb-u-boot.dtsi just to avoid include, because when I later create ast2500-zaius-bmc.dts or something like that, I would have the same problem.

Yes you may as well have your own include.

The idea is to use the same files as Linux, and put the U-Boot additions in a separate file. But it's not critical - just a new idea we had. Also if you can get the U-Boot additions into the upstream Linux version then you won't need this.

Regards, Simon

Hi Maxim,

On 4 January 2017 at 12:46, Maxim Sloyko maxims@google.com wrote:

This driver is ast2500 specific and is not compatible with earlier

ast2500-specific

versions of this chip. The differences are not that large, but they are in somewhat random places, so making it compatible with ast2400 is not worth the effort at the moment.

Signed-off-by: Maxim Sloyko maxims@google.com

arch/arm/include/asm/arch-aspeed/scu_ast2500.h | 108 +++++++++++ drivers/clk/Makefile | 2 + drivers/clk/aspeed/Makefile | 7 + drivers/clk/aspeed/clk_ast2500.c | 255 +++++++++++++++++++++++++ 4 files changed, 372 insertions(+) create mode 100644 arch/arm/include/asm/arch-aspeed/scu_ast2500.h create mode 100644 drivers/clk/aspeed/Makefile create mode 100644 drivers/clk/aspeed/clk_ast2500.c

diff --git a/arch/arm/include/asm/arch-aspeed/scu_ast2500.h b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h new file mode 100644 index 0000000000..febff9d2d3 --- /dev/null +++ b/arch/arm/include/asm/arch-aspeed/scu_ast2500.h @@ -0,0 +1,108 @@ +#ifndef _ASM_ARCH_SCU_AST2500_H +#define _ASM_ARCH_SCU_AST2500_H

+#define SCU_UNLOCK_VALUE 0x1688a8a8

+#define SCU_HWSTRAP_VGAMEM_MASK 3 +#define SCU_HWSTRAP_VGAMEM_SHIFT 2 +#define SCU_HWSTRAP_DDR4 (1 << 24) +#define SCU_HWSTRAP_CLKIN_25MHZ (1 << 23)

+#define SCU_MPLL_DENUM_SHIFT 0 +#define SCU_MPLL_DENUM_MASK 0x1f +#define SCU_MPLL_NUM_SHIFT 5 +#define SCU_MPLL_NUM_MASK 0xff +#define SCU_MPLL_POST_SHIFT 13 +#define SCU_MPLL_POST_MASK 0x3f

+#define SCU_HPLL_DENUM_SHIFT 0 +#define SCU_HPLL_DENUM_MASK 0x1f +#define SCU_HPLL_NUM_SHIFT 5 +#define SCU_HPLL_NUM_MASK 0xff +#define SCU_HPLL_POST_SHIFT 13 +#define SCU_HPLL_POST_MASK 0x3f

+#define SCU_MISC2_UARTCLK_SHIFT 24

+#define SCU_MISC_UARTCLK_DIV13 (1 << 12)

+#ifndef __ASSEMBLY__

+struct ast2500_clk_priv {

•   struct ast2500_scu *scu;
  

+};

+struct ast2500_scu {

•   u32 protection_key;
  

•   u32 sysreset_ctrl1;
  

•   u32 clk_sel1;
  

•   u32 clk_stop_ctrl1;
  

•   u32 freq_counter_ctrl;
  

•   u32 freq_counter_cmp;
  

•   u32 intr_ctrl;
  

•   u32 d2_pll_param;
  

•   u32 m_pll_param;
  

•   u32 h_pll_param;
  

•   u32 d_pll_param;
  

•   u32 misc_ctrl1;
  

•   u32 pci_config[3];
  

•   u32 sysreset_status;
  

•   u32 vga_handshake[2];
  

•   u32 mac_clk_delay;
  

•   u32 misc_ctrl2;
  

•   u32 vga_scratch[8];
  

•   u32 hwstrap;
  

•   u32 rng_ctrl;
  

•   u32 rng_data;
  

•   u32 rev_id;
  

•   u32 pinmux_ctrl[6];
  

•   u32 reserved0;
  

•   u32 extrst_sel;
  

•   u32 pinmux_ctrl1[4];
  

•   u32 reserved1[2];
  

•   u32 mac_clk_delay_100M;
  

•   u32 mac_clk_delay_10M;
  

•   u32 wakeup_enable;
  

•   u32 wakeup_control;
  

•   u32 reserved2[3];
  

•   u32 sysreset_ctrl2;
  

•   u32 clk_sel2;
  

•   u32 clk_stop_ctrl2;
  

•   u32 freerun_counter;
  

•   u32 freerun_counter_ext;
  

•   u32 clk_duty_meas_ctrl;
  

•   u32 clk_duty_meas_res;
  

•   u32 reserved3[4];
  

•   /* The next registers are not key protected */
  

key-protected

•   struct ast2500_cpu2 {
  

•           u32 ctrl;
  

•           u32 base_addr[9];
  

•           u32 cache_ctrl;
  

•   } cpu2;
  

•   u32 reserved4;
  

•   u32 d_pll_ext_param[3];
  

•   u32 d2_pll_ext_param[3];
  

•   u32 mh_pll_ext_param;
  

•   u32 reserved5;
  

•   u32 chip_id[2];
  

•   u32 reserved6[2];
  

•   u32 uart_clk_ctrl;
  

•   u32 reserved7[7];
  

•   u32 pcie_config;
  

•   u32 mmio_decode;
  

•   u32 reloc_ctrl_decode[2];
  

•   u32 mailbox_addr;
  

•   u32 shared_sram_decode[2];
  

•   u32 bmc_rev_id;
  

•   u32 reserved8;
  

•   u32 bmc_device_id;
  

•   u32 reserved9[13];
  

•   u32 clk_duty_sel;
  

+};

+int ast_get_clk(struct udevice **devp); +void *ast_get_scu(void);

Function comments please.

+#endif /* __ASSEMBLY__ */

+#endif /* _ASM_ARCH_SCU_AST2500_H */ diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile index 40a5e8cae8..625513789c 100644 --- a/drivers/clk/Makefile +++ b/drivers/clk/Makefile @@ -16,3 +16,5 @@ obj-$(CONFIG_CLK_UNIPHIER) += uniphier/ obj-$(CONFIG_CLK_EXYNOS) += exynos/ obj-$(CONFIG_CLK_AT91) += at91/ obj-$(CONFIG_CLK_BOSTON) += clk_boston.o

+obj-$(CONFIG_ARCH_ASPEED) += aspeed/ diff --git a/drivers/clk/aspeed/Makefile b/drivers/clk/aspeed/Makefile new file mode 100644 index 0000000000..65d1cd6e29 --- /dev/null +++ b/drivers/clk/aspeed/Makefile @@ -0,0 +1,7 @@ +# +# Copyright (c) 2016 Google, Inc +# +# SPDX-License-Identifier: GPL-2.0+ +#

+obj-$(CONFIG_ASPEED_AST2500) += clk_ast2500.o diff --git a/drivers/clk/aspeed/clk_ast2500.c b/drivers/clk/aspeed/clk_ast2500.c new file mode 100644 index 0000000000..c888a6d35b --- /dev/null +++ b/drivers/clk/aspeed/clk_ast2500.c @@ -0,0 +1,255 @@ +/*

• • (C) Copyright 2016 Google, Inc
• • SPDX-License-Identifier: GPL-2.0
• */

+#include <common.h> +#include <asm/arch/scu_ast2500.h> +#include <asm/io.h> +#include <clk-uclass.h> +#include <dm.h> +#include <dm/lists.h> +#include <dt-bindings/clock/ast2500-scu.h>

+DECLARE_GLOBAL_DATA_PTR;

+/*

• • For H-PLL and M-PLL the formula is
• • (Output Frequency) = CLKIN * ((M + 1) / (N + 1)) / (P + 1)
• • M - Numerator
• • N - Denumerator
• • P - Post Divider
• • They have the same layout in their control register.
• */

+/*

• • Get the rate of the M-PLL clock from input clock frequency and
• • the value of the M-PLL Parameter Register.
• */

+static ulong ast2500_get_mpll_rate(ulong clkin, u32 mpll_reg) +{

•   const ulong num = (mpll_reg >> SCU_MPLL_NUM_SHIFT) & SCU_MPLL_NUM_MASK;
  

•   const ulong denum = (mpll_reg >> SCU_MPLL_DENUM_SHIFT)
  

•                   & SCU_MPLL_DENUM_MASK;
  

•   const ulong post_div = (mpll_reg >> SCU_MPLL_POST_SHIFT)
  

•                   & SCU_MPLL_POST_MASK;
  

•   return (clkin * ((num + 1) / (denum + 1))) / post_div;
  

+}

+/*

• • Get the rate of the H-PLL clock from input clock frequency and
• • the value of the H-PLL Parameter Register.
• */

+static ulong ast2500_get_hpll_rate(ulong clkin, u32 hpll_reg) +{

•   const ulong num = (hpll_reg >> SCU_HPLL_NUM_SHIFT) & SCU_HPLL_NUM_MASK;
  

•   const ulong denum = (hpll_reg >> SCU_HPLL_DENUM_SHIFT)
  

•                   & SCU_HPLL_DENUM_MASK;
  

•   const ulong post_div = (hpll_reg >> SCU_HPLL_POST_SHIFT)
  

•                   & SCU_HPLL_POST_MASK;
  

•   return (clkin * ((num + 1) / (denum + 1))) / post_div;
  

+}

+static ulong ast2500_get_clkin(struct ast2500_scu *scu) +{

•   return readl(&scu->hwstrap) & SCU_HWSTRAP_CLKIN_25MHZ
  

•                   ? 25*1000*1000 : 24*1000*1000;
  

Spaces around *

+}

+static ulong ast2500_get_uart_clk_rate(struct ast2500_scu *scu, int uart)

Function comment. What is 'uart' for?

+{

•   /*
  

•    * ast2500 datasheet is very confusing when it comes to UART clocks,
  

•    * especially when CLKIN = 25 MHz. The settings are in
  

•    * different registers and it is unclear how they interact.
  

•    *
  

•    * This has only been tested with default settings and CLKIN = 24 MHz.
  

Can you detect this and return -EINVAL if it can't work? If not, that's fine.

•    */
  

•   ulong uart_clkin;
  

•   if (readl(&scu->misc_ctrl2) & (1 << (uart + SCU_MISC2_UARTCLK_SHIFT)))
  

•           uart_clkin = 192 * 1000 * 1000;
  

•   else
  

•           uart_clkin = 24 * 1000 * 1000;
  

•   if (readl(&scu->misc_ctrl1) & SCU_MISC_UARTCLK_DIV13)
  

•           uart_clkin /= 13;
  

•   return uart_clkin;
  

+}

+static ulong ast2500_clk_get_rate(struct clk *clk) +{

•   struct ast2500_clk_priv *priv = dev_get_priv(clk->dev);
  

•   ulong clkin = ast2500_get_clkin(priv->scu);
  

•   ulong rate;
  

•   switch (clk->id) {
  

•   case PLL_HPLL:
  

•   case ARMCLK:
  

•           /*
  

•            * This ignores dynamic/static slowdown of ARMCLK and may
  

•            * be inaccurate.
  

•            */
  

•           rate = ast2500_get_hpll_rate(clkin,
  

•                                        readl(&priv->scu->h_pll_param));
  

•           break;
  

•   case MCLK_DDR:
  

•           rate = ast2500_get_mpll_rate(clkin,
  

•                                        readl(&priv->scu->m_pll_param));
  

•           break;
  

•   case PCLK_UART1:
  

•           rate = ast2500_get_uart_clk_rate(priv->scu, 1);
  

•           break;
  

•   case PCLK_UART2:
  

•           rate = ast2500_get_uart_clk_rate(priv->scu, 2);
  

•           break;
  

•   case PCLK_UART3:
  

•           rate = ast2500_get_uart_clk_rate(priv->scu, 3);
  

•           break;
  

•   case PCLK_UART4:
  

•           rate = ast2500_get_uart_clk_rate(priv->scu, 4);
  

•           break;
  

•   case PCLK_UART5:
  

•           rate = ast2500_get_uart_clk_rate(priv->scu, 5);
  

•           break;
  

•   default:
  

•           return -ENOENT;
  

•   }
  

•   return rate;
  

+}

+static void ast2500_scu_unlock(struct ast2500_scu *scu) +{

•   writel(SCU_UNLOCK_VALUE, &scu->protection_key);
  

•   while (!readl(&scu->protection_key))
  

•           ;
  

Can this ever timeout?

+}

+static void ast2500_scu_lock(struct ast2500_scu *scu) +{

•   writel(~SCU_UNLOCK_VALUE, &scu->protection_key);
  

•   while (readl(&scu->protection_key))
  

•           ;
  

+}

+static ulong ast2500_configure_ddr(struct ast2500_scu *scu, ulong rate) +{

•   ulong clkin = ast2500_get_clkin(scu);
  

•   u32 mpll_reg;
  

•   /*
  

•    * There are not that many combinations of numerator, denumerator
  

•    * and post divider, so just brute force the best combination.
  

•    * However, to avoid overflow when multiplying, use kHz.
  

•    */
  

•   const ulong clkin_khz = clkin / 1000;
  

•   const ulong rate_khz = rate / 1000;
  

drop blank line

•   ulong best_num = 0;
  

•   ulong best_denum = 0;
  

•   ulong best_post = 0;
  

•   ulong delta = rate;
  

drop blank line

•   ulong num, denum, post;
  

add blank line

•   for (denum = 0; denum <= SCU_MPLL_DENUM_MASK; ++denum) {
  

•           for (post = 0; post <= SCU_MPLL_POST_MASK; ++post) {
  

•                   num = (rate_khz * (post + 1) / clkin_khz) * (denum + 1);
  

•                   ulong new_rate_khz = (clkin_khz
  

•                                         * ((num + 1) / (denum + 1)))
  

•                                        / (post + 1);
  

•                   /* Keep the rate below requested one. */
  

•                   if (new_rate_khz > rate_khz)
  

•                           continue;
  

•                   if (new_rate_khz - rate_khz < delta) {
  

•                           delta = new_rate_khz - rate_khz;
  

•                           best_num = num;
  

•                           best_denum = denum;
  

•                           best_post = post;
  

•                           if (delta == 0)
  

•                                   goto rate_calc_done;
  

•                   }
  

•           }
  

•   }
  

• rate_calc_done:

Drop blank line

Can it fail? Do you need to check delta?

•   mpll_reg = readl(&scu->m_pll_param);
  

•   mpll_reg &= ~((SCU_MPLL_POST_MASK << SCU_MPLL_POST_SHIFT)
  

•                 | (SCU_MPLL_NUM_MASK << SCU_MPLL_NUM_SHIFT)
  

•                 | (SCU_MPLL_DENUM_MASK << SCU_MPLL_DENUM_SHIFT));
  

•   mpll_reg |= (best_post << SCU_MPLL_POST_SHIFT)
  

•       | (best_num << SCU_MPLL_NUM_SHIFT)
  

•       | (best_denum << SCU_MPLL_DENUM_SHIFT);
  

•   ast2500_scu_unlock(scu);
  

•   writel(mpll_reg, &scu->m_pll_param);
  

•   ast2500_scu_lock(scu);
  

•   return ast2500_get_mpll_rate(clkin, mpll_reg);
  

+}

+static ulong ast2500_clk_set_rate(struct clk *clk, ulong rate) +{

•   struct ast2500_clk_priv *priv = dev_get_priv(clk->dev);
  

•   ulong new_rate;
  

•   switch (clk->id) {
  

•   case PLL_MPLL:
  

•   case MCLK_DDR:
  

•           new_rate = ast2500_configure_ddr(priv->scu, rate);
  

•           break;
  

•   default:
  

•           return -ENOENT;
  

•   }
  

•   return new_rate;
  

+}

+struct clk_ops ast2500_clk_ops = {

•   .get_rate = ast2500_clk_get_rate,
  

•   .set_rate = ast2500_clk_set_rate,
  

+};

+static int ast2500_clk_probe(struct udevice *dev) +{

•   struct ast2500_clk_priv *priv = dev_get_priv(dev);
  

blank line

•   priv->scu = (struct ast2500_scu *)dev_get_addr(dev);
  

Maybe dev_get_addr_ptr()

Also should check that it is not FDT_ADDR_NONE.

•   return 0;
  

+}

+static int ast2500_clk_bind(struct udevice *dev) +{

•   int ret;
  

•   /* The reset driver does not have a device node, so bind it here */
  

•   ret = device_bind_driver(gd->dm_root, "ast_sysreset", "reset", &dev);
  

•   if (ret)
  

•           debug("Warning: No reset driver: ret=%d\n", ret);
  

•   return 0;
  

+}

+static const struct udevice_id ast2500_clk_ids[] = {

•   { .compatible = "aspeed,ast2500-scu" },
  

•   { }
  

+};

+U_BOOT_DRIVER(aspeed_ast2500_scu) = {

•   .name           = "aspeed_ast2500_scu",
  

•   .id             = UCLASS_CLK,
  

•   .of_match       = ast2500_clk_ids,
  

•   .priv_auto_alloc_size = sizeof(struct ast2500_clk_priv),
  

•   .ops            = &ast2500_clk_ops,
  

•   .bind           = ast2500_clk_bind,
  

•   .probe          = ast2500_clk_probe,
  

+};

2.11.0.390.gc69c2f50cf-goog

Regards, Simon

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