[PATCH u-boot v2019.04-aspeed-openbmc v2 0/3] ram: Configure ECC

[PATCH u-boot v2019.04-aspeed-openbmc v2 0/3] ram: Configure ECC

[Joel Stanley]

v2: Rename property to include size
    Use correct size in printf
    Add patch to enable ECC in p10bmc

Joel Stanley (3):
  aspeed/sdram: Use device tree to configure ECC
  ram/aspeed: Remove ECC config option
  ARM: dts: aspeed: p10bmc: Enable ECC

 drivers/ram/aspeed/sdram_ast2500.c | 18 ++++++++++--------
 drivers/ram/aspeed/sdram_ast2600.c | 19 +++++++++++--------
 arch/arm/dts/ast2600-p10bmc.dts    |  1 +
 drivers/ram/aspeed/Kconfig         | 20 --------------------
 4 files changed, 22 insertions(+), 36 deletions(-)

-- 
2.35.1

[PATCH u-boot v2019.04-aspeed-openbmc v2 1/3] aspeed/sdram: Use device tree to configure ECC

[Joel Stanley]

Instead of configuring ECC based on the build config, use a device tree
property to selectively enable ECC at runtime.

There are two properties:

  aspeed,ecc-enabled;
  aspeed,ecc-size-mb = "512";

The enabled property is a boolean that enables ECC if it is present.

The size is the number of MB that should be covered by ECC. Setting it
to zero, or omitting it, defaults the ECC size to "auto detect".

  edac: sdram@1e6e0000 {
    compatible = "aspeed,ast2600-sdram-edac";
    reg = <0x1e6e0000 0x174>;
    interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
    aspeed,ecc-enabled;
    aspeed,ecc-size-mb = "512";
  };

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
v2:
  Change property to be aspeed,ecc-size-mb
  Fix printing of size to use mb

 drivers/ram/aspeed/sdram_ast2500.c | 14 ++++++++++----
 drivers/ram/aspeed/sdram_ast2600.c | 14 ++++++++++----
 drivers/ram/aspeed/Kconfig         | 13 -------------
 3 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/drivers/ram/aspeed/sdram_ast2500.c b/drivers/ram/aspeed/sdram_ast2500.c
index 435e1a8cfc1d..9bf4906b5939 100644
--- a/drivers/ram/aspeed/sdram_ast2500.c
+++ b/drivers/ram/aspeed/sdram_ast2500.c
@@ -279,16 +279,16 @@ static void ast2500_sdrammc_calc_size(struct dram_info *info)
 }
 
 #ifdef CONFIG_ASPEED_ECC
-static void ast2500_sdrammc_ecc_enable(struct dram_info *info)
+static void ast2500_sdrammc_ecc_enable(struct dram_info *info, u32 conf_size_mb)
 {
 	struct ast2500_sdrammc_regs *regs = info->regs;
 	size_t conf_size;
 	u32 reg;
 	
-	conf_size = CONFIG_ASPEED_ECC_SIZE * SDRAM_SIZE_1MB;
+	conf_size = conf_size_mb * SDRAM_SIZE_1MB;
 	if (conf_size > info->info.size) {
 		printf("warning: ECC configured %dMB but actual size is %dMB\n",
-		       CONFIG_ASPEED_ECC_SIZE,
+		       conf_size_mb,
 		       info->info.size / SDRAM_SIZE_1MB);
 		conf_size = info->info.size;
 	} else if (conf_size == 0) {
@@ -371,8 +371,14 @@ static int ast2500_sdrammc_init_ddr4(struct dram_info *info)
 	writel(SDRAM_MISC_DDR4_TREFRESH, &info->regs->misc_control);
 
 #ifdef CONFIG_ASPEED_ECC
-	ast2500_sdrammc_ecc_enable(info);
+	if (dev_read_bool(dev, "aspeed,ecc-enabled")) {
+		u32 ecc_size;
+
+		ecc_size = dev_read_u32_default(dev, "aspeed,ecc-size-mb", 0);
+		ast2500_sdrammc_ecc_enable(priv, ecc_size);
+	}
 #endif
+
 	/* Enable all requests except video & display */
 	writel(SDRAM_REQ_USB20_EHCI1
 	       | SDRAM_REQ_USB20_EHCI2
diff --git a/drivers/ram/aspeed/sdram_ast2600.c b/drivers/ram/aspeed/sdram_ast2600.c
index 5118b14f8708..2a4d6af57eb3 100644
--- a/drivers/ram/aspeed/sdram_ast2600.c
+++ b/drivers/ram/aspeed/sdram_ast2600.c
@@ -860,16 +860,16 @@ static void ast2600_sdrammc_update_size(struct dram_info *info)
 	info->info.size = hw_size;
 }
 #ifdef CONFIG_ASPEED_ECC
-static void ast2600_sdrammc_ecc_enable(struct dram_info *info)
+static void ast2600_sdrammc_ecc_enable(struct dram_info *info, u32 conf_size_mb)
 {
 	struct ast2600_sdrammc_regs *regs = info->regs;
 	size_t conf_size;
 	u32 reg;
 
-	conf_size = CONFIG_ASPEED_ECC_SIZE * SDRAM_SIZE_1MB;
+	conf_size = conf_size_mb * SDRAM_SIZE_1MB;
 	if (conf_size > info->info.size) {
 		printf("warning: ECC configured %dMB but actual size is %dMB\n",
-		       CONFIG_ASPEED_ECC_SIZE,
+		       conf_size_mb,
 		       info->info.size / SDRAM_SIZE_1MB);
 		conf_size = info->info.size;
 	} else if (conf_size == 0) {
@@ -989,8 +989,14 @@ L_ast2600_sdramphy_train:
 #endif
 
 #ifdef CONFIG_ASPEED_ECC
-	ast2600_sdrammc_ecc_enable(priv);
+	if (dev_read_bool(dev, "aspeed,ecc-enabled")) {
+		u32 ecc_size;
+
+		ecc_size = dev_read_u32_default(dev, "aspeed,ecc-size-mb", 0);
+		ast2600_sdrammc_ecc_enable(priv, ecc_size);
+	}
 #endif
+
 	setbits_le32(priv->scu + AST_SCU_HANDSHAKE, SCU_HANDSHAKE_MASK);
 	clrbits_le32(&regs->intr_ctrl, MCR50_RESET_ALL_INTR);
 	ast2600_sdrammc_lock(priv);
diff --git a/drivers/ram/aspeed/Kconfig b/drivers/ram/aspeed/Kconfig
index 924e82b19430..54c7769b5bbe 100644
--- a/drivers/ram/aspeed/Kconfig
+++ b/drivers/ram/aspeed/Kconfig
@@ -51,19 +51,6 @@ config ASPEED_ECC
 	help
 	  enable SDRAM ECC function
 
-if ASPEED_ECC
-config ASPEED_ECC_SIZE
-	int "ECC size: 0=driver auto-caluated"
-	depends on ASPEED_ECC
-	default 0
-	help
-	  SDRAM size with the error correcting code enabled. The unit is 
-	  in Megabytes.  Noted that only the 8/9 of the configured size 
-	  can be used by the system.  The remaining 1/9 will be used by 
-	  the ECC engine.  If the size is set to 0, the sdram driver will 
-	  calculate the SDRAM size and set the whole range be ECC enabled.
-endif
-
 choice
 	prompt "DDR4 PHY side ODT"
 	default ASPEED_DDR4_PHY_ODT40
-- 
2.35.1

Re: [PATCH u-boot v2019.04-aspeed-openbmc v2 1/3] aspeed/sdram: Use device tree to configure ECC

[Andrew Jeffery]

On Wed, 21 Sep 2022, at 14:24, Joel Stanley wrote:
> Instead of configuring ECC based on the build config, use a device tree
> property to selectively enable ECC at runtime.
>
> There are two properties:
>
>   aspeed,ecc-enabled;
>   aspeed,ecc-size-mb = "512";
>
> The enabled property is a boolean that enables ECC if it is present.
>
> The size is the number of MB that should be covered by ECC. Setting it
> to zero, or omitting it, defaults the ECC size to "auto detect".
>
>   edac: sdram@1e6e0000 {
>     compatible = "aspeed,ast2600-sdram-edac";
>     reg = <0x1e6e0000 0x174>;
>     interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
>     aspeed,ecc-enabled;
>     aspeed,ecc-size-mb = "512";
>   };
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> v2:
>   Change property to be aspeed,ecc-size-mb
>   Fix printing of size to use mb
>
>  drivers/ram/aspeed/sdram_ast2500.c | 14 ++++++++++----
>  drivers/ram/aspeed/sdram_ast2600.c | 14 ++++++++++----
>  drivers/ram/aspeed/Kconfig         | 13 -------------
>  3 files changed, 20 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/ram/aspeed/sdram_ast2500.c 
> b/drivers/ram/aspeed/sdram_ast2500.c
> index 435e1a8cfc1d..9bf4906b5939 100644
> --- a/drivers/ram/aspeed/sdram_ast2500.c
> +++ b/drivers/ram/aspeed/sdram_ast2500.c
> @@ -279,16 +279,16 @@ static void ast2500_sdrammc_calc_size(struct 
> dram_info *info)
>  }
> 
>  #ifdef CONFIG_ASPEED_ECC
> -static void ast2500_sdrammc_ecc_enable(struct dram_info *info)
> +static void ast2500_sdrammc_ecc_enable(struct dram_info *info, u32 
> conf_size_mb)
>  {
>  	struct ast2500_sdrammc_regs *regs = info->regs;
>  	size_t conf_size;
>  	u32 reg;
> 	
> -	conf_size = CONFIG_ASPEED_ECC_SIZE * SDRAM_SIZE_1MB;
> +	conf_size = conf_size_mb * SDRAM_SIZE_1MB;
>  	if (conf_size > info->info.size) {
>  		printf("warning: ECC configured %dMB but actual size is %dMB\n",
> -		       CONFIG_ASPEED_ECC_SIZE,
> +		       conf_size_mb,
>  		       info->info.size / SDRAM_SIZE_1MB);
>  		conf_size = info->info.size;
>  	} else if (conf_size == 0) {
> @@ -371,8 +371,14 @@ static int ast2500_sdrammc_init_ddr4(struct 
> dram_info *info)
>  	writel(SDRAM_MISC_DDR4_TREFRESH, &info->regs->misc_control);
> 
>  #ifdef CONFIG_ASPEED_ECC
> -	ast2500_sdrammc_ecc_enable(info);
> +	if (dev_read_bool(dev, "aspeed,ecc-enabled")) {
> +		u32 ecc_size;
> +
> +		ecc_size = dev_read_u32_default(dev, "aspeed,ecc-size-mb", 0);
> +		ast2500_sdrammc_ecc_enable(priv, ecc_size);

The code you deleted passes `info` to ast2500_sdrammc_ecc_enable() as
the first argument but you're passing `priv` here. Do they alias?

Ah, actually, I think you copied the code from sdram_ast2600.c into
sdram_ast2500.c. I suspect this might not compile?

Andrew

Re: [PATCH u-boot v2019.04-aspeed-openbmc v2 1/3] aspeed/sdram: Use device tree to configure ECC

[Joel Stanley]

On Wed, 21 Sept 2022 at 06:10, Andrew Jeffery <andrew@aj.id.au> wrote:

> >  #ifdef CONFIG_ASPEED_ECC
> > -     ast2500_sdrammc_ecc_enable(info);
> > +     if (dev_read_bool(dev, "aspeed,ecc-enabled")) {
> > +             u32 ecc_size;
> > +
> > +             ecc_size = dev_read_u32_default(dev, "aspeed,ecc-size-mb", 0);
> > +             ast2500_sdrammc_ecc_enable(priv, ecc_size);
>
> The code you deleted passes `info` to ast2500_sdrammc_ecc_enable() as
> the first argument but you're passing `priv` here. Do they alias?
>
> Ah, actually, I think you copied the code from sdram_ast2600.c into
> sdram_ast2500.c. I suspect this might not compile?

Well spotted. I was build testing, but it seems it wasn't building
what I thought it was.

Cheers,

Joel

[PATCH u-boot v2019.04-aspeed-openbmc v2 2/3] ram/aspeed: Remove ECC config option

[Joel Stanley]

Always build the code now that it is enabled by device tree.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/ram/aspeed/sdram_ast2500.c | 4 ----
 drivers/ram/aspeed/sdram_ast2600.c | 5 +----
 drivers/ram/aspeed/Kconfig         | 7 -------
 3 files changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/ram/aspeed/sdram_ast2500.c b/drivers/ram/aspeed/sdram_ast2500.c
index 9bf4906b5939..ffad4fb18c8e 100644
--- a/drivers/ram/aspeed/sdram_ast2500.c
+++ b/drivers/ram/aspeed/sdram_ast2500.c
@@ -278,7 +278,6 @@ static void ast2500_sdrammc_calc_size(struct dram_info *info)
 			 << SDRAM_CONF_CAP_SHIFT));
 }
 
-#ifdef CONFIG_ASPEED_ECC
 static void ast2500_sdrammc_ecc_enable(struct dram_info *info, u32 conf_size_mb)
 {
 	struct ast2500_sdrammc_regs *regs = info->regs;
@@ -313,7 +312,6 @@ static void ast2500_sdrammc_ecc_enable(struct dram_info *info, u32 conf_size_mb)
 	writel(0x400, &regs->ecc_test_ctrl);
 	printf("ECC enable, ");
 }
-#endif
 
 static int ast2500_sdrammc_init_ddr4(struct dram_info *info)
 {
@@ -370,14 +368,12 @@ static int ast2500_sdrammc_init_ddr4(struct dram_info *info)
 
 	writel(SDRAM_MISC_DDR4_TREFRESH, &info->regs->misc_control);
 
-#ifdef CONFIG_ASPEED_ECC
 	if (dev_read_bool(dev, "aspeed,ecc-enabled")) {
 		u32 ecc_size;
 
 		ecc_size = dev_read_u32_default(dev, "aspeed,ecc-size-mb", 0);
 		ast2500_sdrammc_ecc_enable(priv, ecc_size);
 	}
-#endif
 
 	/* Enable all requests except video & display */
 	writel(SDRAM_REQ_USB20_EHCI1
diff --git a/drivers/ram/aspeed/sdram_ast2600.c b/drivers/ram/aspeed/sdram_ast2600.c
index 2a4d6af57eb3..5f7b160faddf 100644
--- a/drivers/ram/aspeed/sdram_ast2600.c
+++ b/drivers/ram/aspeed/sdram_ast2600.c
@@ -859,7 +859,7 @@ static void ast2600_sdrammc_update_size(struct dram_info *info)
 
 	info->info.size = hw_size;
 }
-#ifdef CONFIG_ASPEED_ECC
+
 static void ast2600_sdrammc_ecc_enable(struct dram_info *info, u32 conf_size_mb)
 {
 	struct ast2600_sdrammc_regs *regs = info->regs;
@@ -890,7 +890,6 @@ static void ast2600_sdrammc_ecc_enable(struct dram_info *info, u32 conf_size_mb)
 	writel(BIT(31), &regs->intr_ctrl);
 	writel(0, &regs->intr_ctrl);
 }
-#endif
 
 static int ast2600_sdrammc_probe(struct udevice *dev)
 {
@@ -988,14 +987,12 @@ L_ast2600_sdramphy_train:
 	}
 #endif
 
-#ifdef CONFIG_ASPEED_ECC
 	if (dev_read_bool(dev, "aspeed,ecc-enabled")) {
 		u32 ecc_size;
 
 		ecc_size = dev_read_u32_default(dev, "aspeed,ecc-size-mb", 0);
 		ast2600_sdrammc_ecc_enable(priv, ecc_size);
 	}
-#endif
 
 	setbits_le32(priv->scu + AST_SCU_HANDSHAKE, SCU_HANDSHAKE_MASK);
 	clrbits_le32(&regs->intr_ctrl, MCR50_RESET_ALL_INTR);
diff --git a/drivers/ram/aspeed/Kconfig b/drivers/ram/aspeed/Kconfig
index 54c7769b5bbe..25238bf28d32 100644
--- a/drivers/ram/aspeed/Kconfig
+++ b/drivers/ram/aspeed/Kconfig
@@ -44,13 +44,6 @@ config ASPEED_BYPASS_SELFTEST
 	  Say Y here to bypass DRAM self test to speed up the boot time
 endif
 
-config ASPEED_ECC
-	bool "aspeed SDRAM error correcting code"
-	depends on DM && OF_CONTROL && ARCH_ASPEED
-	default n
-	help
-	  enable SDRAM ECC function
-
 choice
 	prompt "DDR4 PHY side ODT"
 	default ASPEED_DDR4_PHY_ODT40
-- 
2.35.1

Re: [PATCH u-boot v2019.04-aspeed-openbmc v2 2/3] ram/aspeed: Remove ECC config option

[Andrew Jeffery]

On Wed, 21 Sep 2022, at 14:24, Joel Stanley wrote:
> Always build the code now that it is enabled by device tree.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

[PATCH u-boot v2019.04-aspeed-openbmc v2 3/3] ARM: dts: aspeed: p10bmc: Enable ECC

[Joel Stanley]

Enable ECC to cover the entire DRAM by not setting the size property.

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 arch/arm/dts/ast2600-p10bmc.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/dts/ast2600-p10bmc.dts b/arch/arm/dts/ast2600-p10bmc.dts
index 23e3bd8ecfbd..1d0f88bf9628 100755
--- a/arch/arm/dts/ast2600-p10bmc.dts
+++ b/arch/arm/dts/ast2600-p10bmc.dts
@@ -41,6 +41,7 @@
 
 &sdrammc {
 	clock-frequency = <400000000>;
+	aspeed,ecc-enabled;
 };
 
 &wdt2 {
-- 
2.35.1

Re: [PATCH u-boot v2019.04-aspeed-openbmc v2 3/3] ARM: dts: aspeed: p10bmc: Enable ECC

[Andrew Jeffery]

On Wed, 21 Sep 2022, at 14:24, Joel Stanley wrote:
> Enable ECC to cover the entire DRAM by not setting the size property.
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>