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