[PATCH 4/4] ram: sifive: Avoid using hardcoded ram base and size

[Leo Liang]

Hi Bin,

This whole patch set looks pretty good to me.

Just out of curiosity and as being rather new to the u-boot community,
would the following fix be more direct and avoid modifying general code?

On Wed, Jul 15, 2020 at 08:23:03PM -0700, Bin Meng wrote:
> From: Bin Meng <bin.meng at windriver.com>
> 
> At present the SiFive FU540 RAM driver uses hard-coded memory base
> address and size to initialize the DDR controller. This may not be
> true when this driver is used on another board based on FU540.
> 
> Update the driver to read the memory information from DT and use
> that during the initialization.
> 
> Signed-off-by: Bin Meng <bin.meng at windriver.com>
> ---
> 
>  drivers/ram/sifive/fu540_ddr.c | 28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/ram/sifive/fu540_ddr.c b/drivers/ram/sifive/fu540_ddr.c
> index f8f8ca9..2f38023 100644
> --- a/drivers/ram/sifive/fu540_ddr.c
> +++ b/drivers/ram/sifive/fu540_ddr.c
> @@ -8,6 +8,7 @@
>  
>  #include <common.h>
>  #include <dm.h>
> +#include <fdtdec.h>
>  #include <init.h>
>  #include <ram.h>
>  #include <regmap.h>
> @@ -39,9 +40,6 @@
>  #define DENALI_PHY_1152	1152
>  #define DENALI_PHY_1214	1214
>  
> -#define PAYLOAD_DEST	0x80000000
> -#define DDR_MEM_SIZE	(8UL * 1024UL * 1024UL * 1024UL)
> -
>  #define DRAM_CLASS_OFFSET			8
>  #define DRAM_CLASS_DDR4				0xA
>  #define OPTIMAL_RMODW_EN_OFFSET			0
> @@ -65,6 +63,8 @@
>  #define PHY_RX_CAL_DQ0_0_OFFSET			0
>  #define PHY_RX_CAL_DQ1_0_OFFSET			16
>  
> +DECLARE_GLOBAL_DATA_PTR;
> +
>  struct fu540_ddrctl {
>  	volatile u32 denali_ctl[265];
>  };
> @@ -235,8 +235,8 @@ static int fu540_ddr_setup(struct udevice *dev)
>  	struct fu540_ddr_params *params = &plat->ddr_params;
>  	volatile u32 *denali_ctl =  priv->ctl->denali_ctl;
>  	volatile u32 *denali_phy =  priv->phy->denali_phy;
> -	const u64 ddr_size = DDR_MEM_SIZE;
> -	const u64 ddr_end = PAYLOAD_DEST + ddr_size;
> +	const u64 ddr_size = priv->info.size;
> +	const u64 ddr_end = priv->info.base + ddr_size;
>  	int ret, i;
>  	u32 physet;
>  
> @@ -302,7 +302,7 @@ static int fu540_ddr_setup(struct udevice *dev)
>  		     | (1 << MULTIPLE_OUT_OF_RANGE_OFFSET));
>  
>  	/* set up range protection */
> -	fu540_ddr_setup_range_protection(denali_ctl, DDR_MEM_SIZE);
> +	fu540_ddr_setup_range_protection(denali_ctl, priv->info.size);
>  
>  	/* Mask off port command error interrupt DENALI_CTL_136 */
>  	setbits_le32(DENALI_CTL_136 + denali_ctl,
> @@ -314,14 +314,14 @@ static int fu540_ddr_setup(struct udevice *dev)
>  
>  	/* check size */
>  	priv->info.size = get_ram_size((long *)priv->info.base,
> -				       DDR_MEM_SIZE);
> +				       ddr_size);
>  
>  	debug("%s : %lx\n", __func__, priv->info.size);
>  
>  	/* check memory access for all memory */
> -	if (priv->info.size != DDR_MEM_SIZE) {
> +	if (priv->info.size != ddr_size) {
>  		printf("DDR invalid size : 0x%lx, expected 0x%lx\n",
> -		       priv->info.size, DDR_MEM_SIZE);
> +		       priv->info.size, (uintptr_t)ddr_size);
>  		return -EINVAL;
>  	}
>  
> @@ -333,6 +333,9 @@ static int fu540_ddr_probe(struct udevice *dev)
>  {
>  	struct fu540_ddr_info *priv = dev_get_priv(dev);
>  
> +	fdtdec_get_mem_size_base(gd->fdt_blob, (phys_size_t *)&priv->info.size,
> +				 (unsigned long *)&priv->info.base);
> +

Instead of introducing new API,
could we do something as such with the existing API?

fdtdec_setup_mem_size_base(gd->blob);
priv->info.base = gd->ram_base;
priv->info.size = gd->ram_size;

>  #if defined(CONFIG_SPL_BUILD)
>  	struct regmap *map;
>  	int ret;
> @@ -368,14 +371,9 @@ static int fu540_ddr_probe(struct udevice *dev)
>  	priv->phy = regmap_get_range(map, 1);
>  	priv->physical_filter_ctrl = regmap_get_range(map, 2);
>  
> -	priv->info.base = CONFIG_SYS_SDRAM_BASE;
> -
> -	priv->info.size = 0;
>  	return fu540_ddr_setup(dev);
> -#else
> -	priv->info.base = CONFIG_SYS_SDRAM_BASE;
> -	priv->info.size = DDR_MEM_SIZE;
>  #endif
> +
>  	return 0;
>  }
>  
> -- 
> 2.7.4
>

Best regards,
Leo