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

[Pragnesh Patel]

Hi Bin,

>-----Original Message-----
>From: Bin Meng <bmeng.cn at gmail.com>
>Sent: 20 July 2020 15:04
>To: Pragnesh Patel <pragnesh.patel at sifive.com>
>Cc: Bin Meng <bin.meng at windriver.com>; Sagar Kadam
><sagar.kadam at sifive.com>; Rick Chen <rick at andestech.com>; U-Boot
>Mailing List <u-boot at lists.denx.de>
>Subject: Re: [PATCH v2 2/2] ram: sifive: Avoid using hardcoded ram base and
>size
>
>[External Email] Do not click links or attachments unless you recognize the
>sender and know the content is safe
>
>Hi Pragnesh,
>
>On Mon, Jul 20, 2020 at 5:28 PM Pragnesh Patel <pragnesh.patel at sifive.com>
>wrote:
>>
>> Hi Bin,
>>
>> >-----Original Message-----
>> >From: Bin Meng <bmeng.cn at gmail.com>
>> >Sent: 20 July 2020 11:37
>> >To: Rick Chen <rick at andestech.com>; Pragnesh Patel
>> ><pragnesh.patel at sifive.com>; Sagar Kadam <sagar.kadam at sifive.com>;
>U-
>> >Boot Mailing List <u-boot at lists.denx.de>
>> >Cc: Bin Meng <bin.meng at windriver.com>
>> >Subject: [PATCH v2 2/2] ram: sifive: Avoid using hardcoded ram base
>> >and size
>> >
>> >[External Email] Do not click links or attachments unless you
>> >recognize the sender and know the content is safe
>> >
>> >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>
>> >---
>> >
>> >Changes in v2:
>> >- Change to use fdtdec_setup_mem_size_base() API
>> >- Drop the 2 patches that added a new API in fdtdec
>> >
>> > drivers/ram/sifive/fu540_ddr.c | 30 +++++++++++++++---------------
>> > 1 file changed, 15 insertions(+), 15 deletions(-)
>> >
>> >diff --git a/drivers/ram/sifive/fu540_ddr.c
>> >b/drivers/ram/sifive/fu540_ddr.c index f8f8ca9..2eef1e7 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,11 @@ static int fu540_ddr_probe(struct udevice *dev)  {
>> >        struct fu540_ddr_info *priv = dev_get_priv(dev);
>> >
>> >+       /* Read memory base and size from DT */
>> >+       fdtdec_setup_mem_size_base();
>> >+       priv->info.base = gd->ram_base;
>> >+       priv->info.size = gd->ram_size;
>> >+
>> > #if defined(CONFIG_SPL_BUILD)
>> >        struct regmap *map;
>> >        int ret;
>> >@@ -368,14 +373,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;
>>
>> This "else" part also need to be updated rather than removing it
>> because it used by
>> fu540_ddr_get_info() function in case of U-Boot.
>>
>
>It's already assigned at the beginning of this function. Could you please
>recheck?

Ohhh yes, my bad.

Reviewed-by: Pragnesh Patel <pragnesh.patel at sifive.com>

>
>Regards,
>Bin