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

[Bin Meng]

Hi Leo,

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

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