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

[Andrew Jeffery]

On Wed, 17 Aug 2022, at 11:29, 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 = "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 at 1e6e0000 {
>     compatible = "aspeed,ast2600-sdram-edac";
>     reg = <0x1e6e0000 0x174>;
>     interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
>     aspeed,ecc-enabled;
>     aspeed,ecc-size = "512";
>   };
>
> Signed-off-by: Joel Stanley <joel at jms.id.au>

Nice. Sure makes building generic SPL easier.

> ---
>  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..ca3b54295bf0 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", 0);

Bit of a nitpick, but given there's no binding documentation, can we 
encode the unit in the property name ("aspeed,ecc-size-mb"?)? Or 
perhaps switch to specifying it in bytes? Or amend the kernel binding 
documentation to add the property and get that merged (troll option)?

> +		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..6287c45365dd 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,
>  		       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", 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
> -

Are there any defconfigs and devictrees that need updating?

Otherwise, LGTM.

Andrew