diff mbox series

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

Message ID 1594869783-20189-4-git-send-email-bmeng.cn@gmail.com
State Superseded
Delegated to: Andes
Headers show
Series [1/4] fdtdec: Add fdtdec_get_mem_size_base() | expand

Commit Message

Bin Meng July 16, 2020, 3:23 a.m. UTC 
From: Bin Meng <bin.meng@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@windriver.com>
---

 drivers/ram/sifive/fu540_ddr.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

Comments

Leo Liang July 17, 2020, 5:58 a.m. UTC | #1 
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@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@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
Bin Meng July 17, 2020, 6:09 a.m. UTC | #2 
Hi Leo,

On Fri, Jul 17, 2020 at 1:58 PM Leo Liang <ycliang@andestech.com> wrote:
>
> 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@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@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;

Yes, I think that works too. Maybe it's not worth introducing a new
API in fdtdec.

>
> >  #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;
> >  }
> >

Regards,
Bin
diff mbox series

Patch

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);
+
 #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;
 }