[U-Boot] [PATCH] driver: net: fsl-mc: Use aligned address for MC FW load
Firmware of Management Complex (MC) should be loaded at 512MB aligned address. So use aligned address for firmware load.
Signed-off-by: Priyanka Jain priyanka.jain@nxp.com Signed-off-by: Prabhakar Kushwaha prabhakar.kushwaha@nxp.com Signed-off-by: Ashish Kumar Ashish.Kumar@nxp.com --- drivers/net/fsl-mc/mc.c | 62 ++++++++++++++++++++++++++-------------------- 1 files changed, 35 insertions(+), 27 deletions(-)
diff --git a/drivers/net/fsl-mc/mc.c b/drivers/net/fsl-mc/mc.c index 1811b0f..c66340b 100644 --- a/drivers/net/fsl-mc/mc.c +++ b/drivers/net/fsl-mc/mc.c @@ -29,6 +29,7 @@ DECLARE_GLOBAL_DATA_PTR; static int mc_boot_status = -1; static int mc_dpl_applied = -1; +static u64 mc_ram_aligned_base_addr; #ifdef CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET static int mc_aiop_applied = -1; #endif @@ -87,12 +88,15 @@ void dump_mc_ccsr_regs(struct mc_ccsr_registers __iomem *mc_ccsr_regs) /** * Copying MC firmware or DPL image to DDR */ -static int mc_copy_image(const char *title, - u64 image_addr, u32 image_size, u64 mc_ram_addr) +static int mc_copy_image(const char *title, u64 image_addr, + u32 image_size, u64 mc_ram_aligned_base_addr) { - debug("%s copied to address %p\n", title, (void *)mc_ram_addr); - memcpy((void *)mc_ram_addr, (void *)image_addr, image_size); - flush_dcache_range(mc_ram_addr, mc_ram_addr + image_size); + debug("%s copied to address %p\n", title, + (void *)mc_ram_aligned_base_addr); + memcpy((void *)mc_ram_aligned_base_addr, + (void *)image_addr, image_size); + flush_dcache_range(mc_ram_aligned_base_addr, + mc_ram_aligned_base_addr + image_size); return 0; }
@@ -224,7 +228,8 @@ static int mc_fixup_dpc(u64 dpc_addr) return 0; }
-static int load_mc_dpc(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpc_addr) +static int load_mc_dpc(u64 mc_ram_aligned_base_addr, size_t mc_ram_size, + u64 mc_dpc_addr) { u64 mc_dpc_offset; #ifndef CONFIG_SYS_LS_MC_DPC_IN_DDR @@ -246,7 +251,8 @@ static int load_mc_dpc(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpc_addr) * Load the MC DPC blob in the MC private DRAM block: */ #ifdef CONFIG_SYS_LS_MC_DPC_IN_DDR - printf("MC DPC is preloaded to %#llx\n", mc_ram_addr + mc_dpc_offset); + printf("MC DPC is preloaded to %#llx\n", + mc_ram_aligned_base_addr + mc_dpc_offset); #else /* * Get address and size of the DPC blob stored in flash: @@ -270,18 +276,20 @@ static int load_mc_dpc(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpc_addr) return -EINVAL; }
- mc_copy_image("MC DPC blob", - (u64)dpc_fdt_hdr, dpc_size, mc_ram_addr + mc_dpc_offset); + mc_copy_image("MC DPC blob", (u64)dpc_fdt_hdr, dpc_size, + mc_ram_aligned_base_addr + mc_dpc_offset); #endif /* not defined CONFIG_SYS_LS_MC_DPC_IN_DDR */
- if (mc_fixup_dpc(mc_ram_addr + mc_dpc_offset)) + if (mc_fixup_dpc(mc_ram_aligned_base_addr + mc_dpc_offset)) return -EINVAL;
- dump_ram_words("DPC", (void *)(mc_ram_addr + mc_dpc_offset)); + dump_ram_words("DPC", + (void *)(mc_ram_aligned_base_addr + mc_dpc_offset)); return 0; }
-static int load_mc_dpl(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpl_addr) +static int load_mc_dpl(u64 mc_ram_aligned_base_addr, size_t mc_ram_size, + u64 mc_dpl_addr) { u64 mc_dpl_offset; #ifndef CONFIG_SYS_LS_MC_DPL_IN_DDR @@ -303,7 +311,8 @@ static int load_mc_dpl(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpl_addr) * Load the MC DPL blob in the MC private DRAM block: */ #ifdef CONFIG_SYS_LS_MC_DPL_IN_DDR - printf("MC DPL is preloaded to %#llx\n", mc_ram_addr + mc_dpl_offset); + printf("MC DPL is preloaded to %#llx\n", + mc_ram_aligned_base_addr + mc_dpl_offset); #else /* * Get address and size of the DPL blob stored in flash: @@ -323,11 +332,12 @@ static int load_mc_dpl(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpl_addr) return -EINVAL; }
- mc_copy_image("MC DPL blob", - (u64)dpl_fdt_hdr, dpl_size, mc_ram_addr + mc_dpl_offset); + mc_copy_image("MC DPL blob", (u64)dpl_fdt_hdr, dpl_size, + mc_ram_aligned_base_addr + mc_dpl_offset); #endif /* not defined CONFIG_SYS_LS_MC_DPL_IN_DDR */
- dump_ram_words("DPL", (void *)(mc_ram_addr + mc_dpl_offset)); + dump_ram_words("DPL", + (void *)(mc_ram_aligned_base_addr + mc_dpl_offset)); return 0; }
@@ -364,7 +374,6 @@ __weak bool soc_has_aiop(void)
static int load_mc_aiop_img(u64 aiop_fw_addr) { - u64 mc_ram_addr = mc_get_dram_addr(); #ifndef CONFIG_SYS_LS_MC_DPC_IN_DDR void *aiop_img; #endif @@ -377,13 +386,14 @@ static int load_mc_aiop_img(u64 aiop_fw_addr) */
#ifdef CONFIG_SYS_LS_MC_DPC_IN_DDR - printf("MC AIOP is preloaded to %#llx\n", mc_ram_addr + + printf("MC AIOP is preloaded to %#llx\n", mc_ram_aligned_base_addr + CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET); #else aiop_img = (void *)aiop_fw_addr; mc_copy_image("MC AIOP image", (u64)aiop_img, CONFIG_SYS_LS_MC_AIOP_IMG_MAX_LENGTH, - mc_ram_addr + CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET); + mc_ram_aligned_base_addr + + CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET); #endif mc_aiop_applied = 0;
@@ -450,7 +460,6 @@ int mc_init(u64 mc_fw_addr, u64 mc_dpc_addr) size_t raw_image_size = 0; #endif struct mc_version mc_ver_info; - u64 mc_ram_aligned_base_addr; u8 mc_ram_num_256mb_blocks; size_t mc_ram_size = mc_get_dram_block_size();
@@ -478,7 +487,7 @@ int mc_init(u64 mc_fw_addr, u64 mc_dpc_addr) dmb();
#ifdef CONFIG_SYS_LS_MC_FW_IN_DDR - printf("MC firmware is preloaded to %#llx\n", mc_ram_addr); + printf("MC firmware is preloaded to %#llx\n", mc_ram_aligned_base_addr); #else error = parse_mc_firmware_fit_image(mc_fw_addr, &raw_image_addr, &raw_image_size); @@ -487,12 +496,12 @@ int mc_init(u64 mc_fw_addr, u64 mc_dpc_addr) /* * Load the MC FW at the beginning of the MC private DRAM block: */ - mc_copy_image("MC Firmware", - (u64)raw_image_addr, raw_image_size, mc_ram_addr); + mc_copy_image("MC Firmware", (u64)raw_image_addr, raw_image_size, + mc_ram_aligned_base_addr); #endif - dump_ram_words("firmware", (void *)mc_ram_addr); + dump_ram_words("firmware", (void *)mc_ram_aligned_base_addr);
- error = load_mc_dpc(mc_ram_addr, mc_ram_size, mc_dpc_addr); + error = load_mc_dpc(mc_ram_aligned_base_addr, mc_ram_size, mc_dpc_addr); if (error != 0) goto out;
@@ -569,10 +578,9 @@ int mc_apply_dpl(u64 mc_dpl_addr) struct mc_ccsr_registers __iomem *mc_ccsr_regs = MC_CCSR_BASE_ADDR; int error = 0; u32 reg_gsr; - u64 mc_ram_addr = mc_get_dram_addr(); size_t mc_ram_size = mc_get_dram_block_size();
- error = load_mc_dpl(mc_ram_addr, mc_ram_size, mc_dpl_addr); + error = load_mc_dpl(mc_ram_aligned_base_addr, mc_ram_size, mc_dpl_addr); if (error != 0) return error;
On 10/24/2016 01:05 AM, Priyanka Jain wrote:
Firmware of Management Complex (MC) should be loaded at 512MB aligned address. So use aligned address for firmware load.
Signed-off-by: Priyanka Jain priyanka.jain@nxp.com Signed-off-by: Prabhakar Kushwaha prabhakar.kushwaha@nxp.com Signed-off-by: Ashish Kumar Ashish.Kumar@nxp.com
drivers/net/fsl-mc/mc.c | 62 ++++++++++++++++++++++++++-------------------- 1 files changed, 35 insertions(+), 27 deletions(-)
diff --git a/drivers/net/fsl-mc/mc.c b/drivers/net/fsl-mc/mc.c index 1811b0f..c66340b 100644 --- a/drivers/net/fsl-mc/mc.c +++ b/drivers/net/fsl-mc/mc.c @@ -29,6 +29,7 @@ DECLARE_GLOBAL_DATA_PTR; static int mc_boot_status = -1; static int mc_dpl_applied = -1; +static u64 mc_ram_aligned_base_addr;
Why do you need this static variable? You didn't use it.
#ifdef CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET static int mc_aiop_applied = -1; #endif @@ -87,12 +88,15 @@ void dump_mc_ccsr_regs(struct mc_ccsr_registers __iomem *mc_ccsr_regs) /**
- Copying MC firmware or DPL image to DDR
*/ -static int mc_copy_image(const char *title,
u64 image_addr, u32 image_size, u64 mc_ram_addr)+static int mc_copy_image(const char *title, u64 image_addr,
u32 image_size, u64 mc_ram_aligned_base_addr){
- debug("%s copied to address %p\n", title, (void *)mc_ram_addr);
- memcpy((void *)mc_ram_addr, (void *)image_addr, image_size);
- flush_dcache_range(mc_ram_addr, mc_ram_addr + image_size);
- debug("%s copied to address %p\n", title,
(void *)mc_ram_aligned_base_addr);- memcpy((void *)mc_ram_aligned_base_addr,
(void *)image_addr, image_size);- flush_dcache_range(mc_ram_aligned_base_addr,
mc_ram_aligned_base_addr + image_size);}
@@ -224,7 +228,8 @@ static int mc_fixup_dpc(u64 dpc_addr) return 0; }
-static int load_mc_dpc(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpc_addr) +static int load_mc_dpc(u64 mc_ram_aligned_base_addr, size_t mc_ram_size,
u64 mc_dpc_addr){ u64 mc_dpc_offset; #ifndef CONFIG_SYS_LS_MC_DPC_IN_DDR @@ -246,7 +251,8 @@ static int load_mc_dpc(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpc_addr) * Load the MC DPC blob in the MC private DRAM block: */ #ifdef CONFIG_SYS_LS_MC_DPC_IN_DDR
- printf("MC DPC is preloaded to %#llx\n", mc_ram_addr + mc_dpc_offset);
- printf("MC DPC is preloaded to %#llx\n",
mc_ram_aligned_base_addr + mc_dpc_offset);#else /* * Get address and size of the DPC blob stored in flash: @@ -270,18 +276,20 @@ static int load_mc_dpc(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpc_addr) return -EINVAL; }
- mc_copy_image("MC DPC blob",
(u64)dpc_fdt_hdr, dpc_size, mc_ram_addr + mc_dpc_offset);
- mc_copy_image("MC DPC blob", (u64)dpc_fdt_hdr, dpc_size,
mc_ram_aligned_base_addr + mc_dpc_offset);#endif /* not defined CONFIG_SYS_LS_MC_DPC_IN_DDR */
- if (mc_fixup_dpc(mc_ram_addr + mc_dpc_offset))
- if (mc_fixup_dpc(mc_ram_aligned_base_addr + mc_dpc_offset)) return -EINVAL;
- dump_ram_words("DPC", (void *)(mc_ram_addr + mc_dpc_offset));
- dump_ram_words("DPC",
(void *)(mc_ram_aligned_base_addr + mc_dpc_offset));}
-static int load_mc_dpl(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpl_addr) +static int load_mc_dpl(u64 mc_ram_aligned_base_addr, size_t mc_ram_size,
u64 mc_dpl_addr){ u64 mc_dpl_offset; #ifndef CONFIG_SYS_LS_MC_DPL_IN_DDR @@ -303,7 +311,8 @@ static int load_mc_dpl(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpl_addr) * Load the MC DPL blob in the MC private DRAM block: */ #ifdef CONFIG_SYS_LS_MC_DPL_IN_DDR
- printf("MC DPL is preloaded to %#llx\n", mc_ram_addr + mc_dpl_offset);
- printf("MC DPL is preloaded to %#llx\n",
mc_ram_aligned_base_addr + mc_dpl_offset);#else /* * Get address and size of the DPL blob stored in flash: @@ -323,11 +332,12 @@ static int load_mc_dpl(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpl_addr) return -EINVAL; }
- mc_copy_image("MC DPL blob",
(u64)dpl_fdt_hdr, dpl_size, mc_ram_addr + mc_dpl_offset);
- mc_copy_image("MC DPL blob", (u64)dpl_fdt_hdr, dpl_size,
mc_ram_aligned_base_addr + mc_dpl_offset);#endif /* not defined CONFIG_SYS_LS_MC_DPL_IN_DDR */
- dump_ram_words("DPL", (void *)(mc_ram_addr + mc_dpl_offset));
- dump_ram_words("DPL",
(void *)(mc_ram_aligned_base_addr + mc_dpl_offset));}
@@ -364,7 +374,6 @@ __weak bool soc_has_aiop(void)
static int load_mc_aiop_img(u64 aiop_fw_addr) {
- u64 mc_ram_addr = mc_get_dram_addr();
#ifndef CONFIG_SYS_LS_MC_DPC_IN_DDR void *aiop_img; #endif @@ -377,13 +386,14 @@ static int load_mc_aiop_img(u64 aiop_fw_addr) */
#ifdef CONFIG_SYS_LS_MC_DPC_IN_DDR
- printf("MC AIOP is preloaded to %#llx\n", mc_ram_addr +
- printf("MC AIOP is preloaded to %#llx\n", mc_ram_aligned_base_addr + CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET);
#else aiop_img = (void *)aiop_fw_addr; mc_copy_image("MC AIOP image", (u64)aiop_img, CONFIG_SYS_LS_MC_AIOP_IMG_MAX_LENGTH,
mc_ram_addr + CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET);
mc_ram_aligned_base_addr +CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET);#endif mc_aiop_applied = 0;
@@ -450,7 +460,6 @@ int mc_init(u64 mc_fw_addr, u64 mc_dpc_addr) size_t raw_image_size = 0; #endif struct mc_version mc_ver_info;
- u64 mc_ram_aligned_base_addr; u8 mc_ram_num_256mb_blocks; size_t mc_ram_size = mc_get_dram_block_size();
@@ -478,7 +487,7 @@ int mc_init(u64 mc_fw_addr, u64 mc_dpc_addr) dmb();
#ifdef CONFIG_SYS_LS_MC_FW_IN_DDR
- printf("MC firmware is preloaded to %#llx\n", mc_ram_addr);
- printf("MC firmware is preloaded to %#llx\n", mc_ram_aligned_base_addr);
#else error = parse_mc_firmware_fit_image(mc_fw_addr, &raw_image_addr, &raw_image_size); @@ -487,12 +496,12 @@ int mc_init(u64 mc_fw_addr, u64 mc_dpc_addr) /* * Load the MC FW at the beginning of the MC private DRAM block: */
- mc_copy_image("MC Firmware",
(u64)raw_image_addr, raw_image_size, mc_ram_addr);
- mc_copy_image("MC Firmware", (u64)raw_image_addr, raw_image_size,
mc_ram_aligned_base_addr);#endif
- dump_ram_words("firmware", (void *)mc_ram_addr);
- dump_ram_words("firmware", (void *)mc_ram_aligned_base_addr);
- error = load_mc_dpc(mc_ram_addr, mc_ram_size, mc_dpc_addr);
- error = load_mc_dpc(mc_ram_aligned_base_addr, mc_ram_size, mc_dpc_addr); if (error != 0) goto out;
@@ -569,10 +578,9 @@ int mc_apply_dpl(u64 mc_dpl_addr) struct mc_ccsr_registers __iomem *mc_ccsr_regs = MC_CCSR_BASE_ADDR; int error = 0; u32 reg_gsr;
u64 mc_ram_addr = mc_get_dram_addr(); size_t mc_ram_size = mc_get_dram_block_size();
error = load_mc_dpl(mc_ram_addr, mc_ram_size, mc_dpl_addr);
- error = load_mc_dpl(mc_ram_aligned_base_addr, mc_ram_size, mc_dpl_addr); if (error != 0) return error;
Do you have substantial change beside the changing name from mc_ram_addr to mc_ram_aligned_base_addr?
York
Hello York,
Please see inline.
Regards Ashish
-----Original Message----- From: york sun Sent: Tuesday, November 08, 2016 12:23 AM To: Priyanka Jain priyanka.jain@nxp.com; u-boot@lists.denx.de Cc: Prabhakar Kushwaha prabhakar.kushwaha@nxp.com; Ashish Kumar ashish.kumar@nxp.com Subject: Re: [PATCH] driver: net: fsl-mc: Use aligned address for MC FW load
On 10/24/2016 01:05 AM, Priyanka Jain wrote:
Firmware of Management Complex (MC) should be loaded at 512MB aligned address. So use aligned address for firmware load.
Signed-off-by: Priyanka Jain priyanka.jain@nxp.com Signed-off-by: Prabhakar Kushwaha prabhakar.kushwaha@nxp.com Signed-off-by: Ashish Kumar Ashish.Kumar@nxp.com
drivers/net/fsl-mc/mc.c | 62 ++++++++++++++++++++++++++-------------------- 1 files changed, 35 insertions(+), 27 deletions(-)
diff --git a/drivers/net/fsl-mc/mc.c b/drivers/net/fsl-mc/mc.c index 1811b0f..c66340b 100644 --- a/drivers/net/fsl-mc/mc.c +++ b/drivers/net/fsl-mc/mc.c @@ -29,6 +29,7 @@ DECLARE_GLOBAL_DATA_PTR; static int mc_boot_status = -1; static int mc_dpl_applied = -1; +static u64 mc_ram_aligned_base_addr;
Why do you need this static variable? You didn't use it. [Ashish Kumar] following two function uses global static variable -357,7 +369,6 @@ static unsigned long get_mc_boot_timeout_ms(void) #ifdef CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET static int load_mc_aiop_img(u64 aiop_fw_addr) { - u64 mc_ram_addr = mc_get_dram_addr(); #ifndef CONFIG_SYS_LS_MC_DPC_IN_DDR void *aiop_img; #endif
@@ -440,7 +451,6 @@ int mc_init(u64 mc_fw_addr, u64 mc_dpc_addr) size_t raw_image_size = 0; #endif struct mc_version mc_ver_info; - u64 mc_ram_aligned_base_addr; u8 mc_ram_num_256mb_blocks; size_t mc_ram_size = mc_get_dram_block_size();
#ifdef CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET static int mc_aiop_applied = -1; #endif @@ -87,12 +88,15 @@ void dump_mc_ccsr_regs(struct mc_ccsr_registers __iomem *mc_ccsr_regs) /**
- Copying MC firmware or DPL image to DDR
*/ -static int mc_copy_image(const char *title,
u64 image_addr, u32 image_size, u64 mc_ram_addr)+static int mc_copy_image(const char *title, u64 image_addr,
u32 image_size, u64 mc_ram_aligned_base_addr){
- debug("%s copied to address %p\n", title, (void *)mc_ram_addr);
- memcpy((void *)mc_ram_addr, (void *)image_addr, image_size);
- flush_dcache_range(mc_ram_addr, mc_ram_addr + image_size);
- debug("%s copied to address %p\n", title,
(void *)mc_ram_aligned_base_addr);- memcpy((void *)mc_ram_aligned_base_addr,
(void *)image_addr, image_size);- flush_dcache_range(mc_ram_aligned_base_addr,
mc_ram_aligned_base_addr + image_size);}
@@ -224,7 +228,8 @@ static int mc_fixup_dpc(u64 dpc_addr) return 0; }
-static int load_mc_dpc(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpc_addr) +static int load_mc_dpc(u64 mc_ram_aligned_base_addr, size_t mc_ram_size,
u64 mc_dpc_addr){ u64 mc_dpc_offset; #ifndef CONFIG_SYS_LS_MC_DPC_IN_DDR @@ -246,7 +251,8 @@ static int load_mc_dpc(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpc_addr) * Load the MC DPC blob in the MC private DRAM block: */ #ifdef CONFIG_SYS_LS_MC_DPC_IN_DDR
- printf("MC DPC is preloaded to %#llx\n", mc_ram_addr + mc_dpc_offset);
- printf("MC DPC is preloaded to %#llx\n",
mc_ram_aligned_base_addr + mc_dpc_offset);#else /* * Get address and size of the DPC blob stored in flash: @@ -270,18 +276,20 @@ static int load_mc_dpc(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpc_addr) return -EINVAL; }
- mc_copy_image("MC DPC blob",
(u64)dpc_fdt_hdr, dpc_size, mc_ram_addr + mc_dpc_offset);
- mc_copy_image("MC DPC blob", (u64)dpc_fdt_hdr, dpc_size,
mc_ram_aligned_base_addr + mc_dpc_offset);#endif /* not defined CONFIG_SYS_LS_MC_DPC_IN_DDR */
- if (mc_fixup_dpc(mc_ram_addr + mc_dpc_offset))
- if (mc_fixup_dpc(mc_ram_aligned_base_addr + mc_dpc_offset)) return -EINVAL;
- dump_ram_words("DPC", (void *)(mc_ram_addr + mc_dpc_offset));
- dump_ram_words("DPC",
(void *)(mc_ram_aligned_base_addr + mc_dpc_offset));}
-static int load_mc_dpl(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpl_addr) +static int load_mc_dpl(u64 mc_ram_aligned_base_addr, size_t mc_ram_size,
u64 mc_dpl_addr){ u64 mc_dpl_offset; #ifndef CONFIG_SYS_LS_MC_DPL_IN_DDR @@ -303,7 +311,8 @@ static int load_mc_dpl(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpl_addr) * Load the MC DPL blob in the MC private DRAM block: */ #ifdef CONFIG_SYS_LS_MC_DPL_IN_DDR
- printf("MC DPL is preloaded to %#llx\n", mc_ram_addr + mc_dpl_offset);
- printf("MC DPL is preloaded to %#llx\n",
mc_ram_aligned_base_addr + mc_dpl_offset);#else /* * Get address and size of the DPL blob stored in flash: @@ -323,11 +332,12 @@ static int load_mc_dpl(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpl_addr) return -EINVAL; }
- mc_copy_image("MC DPL blob",
(u64)dpl_fdt_hdr, dpl_size, mc_ram_addr + mc_dpl_offset);
- mc_copy_image("MC DPL blob", (u64)dpl_fdt_hdr, dpl_size,
mc_ram_aligned_base_addr + mc_dpl_offset);#endif /* not defined CONFIG_SYS_LS_MC_DPL_IN_DDR */
- dump_ram_words("DPL", (void *)(mc_ram_addr + mc_dpl_offset));
- dump_ram_words("DPL",
(void *)(mc_ram_aligned_base_addr + mc_dpl_offset));}
@@ -364,7 +374,6 @@ __weak bool soc_has_aiop(void)
static int load_mc_aiop_img(u64 aiop_fw_addr) {
- u64 mc_ram_addr = mc_get_dram_addr();
#ifndef CONFIG_SYS_LS_MC_DPC_IN_DDR void *aiop_img; #endif @@ -377,13 +386,14 @@ static int load_mc_aiop_img(u64 aiop_fw_addr) */
#ifdef CONFIG_SYS_LS_MC_DPC_IN_DDR
- printf("MC AIOP is preloaded to %#llx\n", mc_ram_addr +
- printf("MC AIOP is preloaded to %#llx\n", mc_ram_aligned_base_addr + CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET);
#else aiop_img = (void *)aiop_fw_addr; mc_copy_image("MC AIOP image", (u64)aiop_img, CONFIG_SYS_LS_MC_AIOP_IMG_MAX_LENGTH,
mc_ram_addr + CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET);
mc_ram_aligned_base_addr +CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET);#endif mc_aiop_applied = 0;
@@ -450,7 +460,6 @@ int mc_init(u64 mc_fw_addr, u64 mc_dpc_addr) size_t raw_image_size = 0; #endif struct mc_version mc_ver_info;
- u64 mc_ram_aligned_base_addr; u8 mc_ram_num_256mb_blocks; size_t mc_ram_size = mc_get_dram_block_size();
@@ -478,7 +487,7 @@ int mc_init(u64 mc_fw_addr, u64 mc_dpc_addr) dmb();
#ifdef CONFIG_SYS_LS_MC_FW_IN_DDR
- printf("MC firmware is preloaded to %#llx\n", mc_ram_addr);
- printf("MC firmware is preloaded to %#llx\n",
+mc_ram_aligned_base_addr); #else error = parse_mc_firmware_fit_image(mc_fw_addr, &raw_image_addr, &raw_image_size); @@ -487,12 +496,12 @@ int mc_init(u64 mc_fw_addr, u64 mc_dpc_addr) /* * Load the MC FW at the beginning of the MC private DRAM block: */
- mc_copy_image("MC Firmware",
(u64)raw_image_addr, raw_image_size, mc_ram_addr);
- mc_copy_image("MC Firmware", (u64)raw_image_addr, raw_image_size,
mc_ram_aligned_base_addr);#endif
- dump_ram_words("firmware", (void *)mc_ram_addr);
- dump_ram_words("firmware", (void *)mc_ram_aligned_base_addr);
- error = load_mc_dpc(mc_ram_addr, mc_ram_size, mc_dpc_addr);
- error = load_mc_dpc(mc_ram_aligned_base_addr, mc_ram_size,
+mc_dpc_addr); if (error != 0) goto out;
@@ -569,10 +578,9 @@ int mc_apply_dpl(u64 mc_dpl_addr) struct mc_ccsr_registers __iomem *mc_ccsr_regs = MC_CCSR_BASE_ADDR; int error = 0; u32 reg_gsr;
u64 mc_ram_addr = mc_get_dram_addr(); size_t mc_ram_size = mc_get_dram_block_size();
error = load_mc_dpl(mc_ram_addr, mc_ram_size, mc_dpl_addr);
- error = load_mc_dpl(mc_ram_aligned_base_addr, mc_ram_size,
+mc_dpl_addr); if (error != 0) return error;
Do you have substantial change beside the changing name from mc_ram_addr to mc_ram_aligned_base_addr? [Ashish Kumar] It is not exactly name change. Here intent is to use userdefine memory size for MC before this only 512MB of memory can be allocated to MC since it incorrectly used mc_ram_addr in place of mc_aligned_base_addr. Ok, Name changes are there only in the function parameters to avoid confusion and retain the function signatures.
York
On 11/07/2016 09:37 PM, Ashish Kumar wrote:
Hello York,
Please see inline.
Regards Ashish
-----Original Message----- From: york sun Sent: Tuesday, November 08, 2016 12:23 AM To: Priyanka Jain priyanka.jain@nxp.com; u-boot@lists.denx.de Cc: Prabhakar Kushwaha prabhakar.kushwaha@nxp.com; Ashish Kumar ashish.kumar@nxp.com Subject: Re: [PATCH] driver: net: fsl-mc: Use aligned address for MC FW load
On 10/24/2016 01:05 AM, Priyanka Jain wrote:
Firmware of Management Complex (MC) should be loaded at 512MB aligned address. So use aligned address for firmware load.
Signed-off-by: Priyanka Jain priyanka.jain@nxp.com Signed-off-by: Prabhakar Kushwaha prabhakar.kushwaha@nxp.com Signed-off-by: Ashish Kumar Ashish.Kumar@nxp.com
drivers/net/fsl-mc/mc.c | 62 ++++++++++++++++++++++++++-------------------- 1 files changed, 35 insertions(+), 27 deletions(-)
diff --git a/drivers/net/fsl-mc/mc.c b/drivers/net/fsl-mc/mc.c index 1811b0f..c66340b 100644 --- a/drivers/net/fsl-mc/mc.c +++ b/drivers/net/fsl-mc/mc.c @@ -29,6 +29,7 @@ DECLARE_GLOBAL_DATA_PTR; static int mc_boot_status = -1; static int mc_dpl_applied = -1; +static u64 mc_ram_aligned_base_addr;
Why do you need this static variable? You didn't use it. [Ashish Kumar] following two function uses global static variable -357,7 +369,6 @@ static unsigned long get_mc_boot_timeout_ms(void) #ifdef CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET static int load_mc_aiop_img(u64 aiop_fw_addr) {
u64 mc_ram_addr = mc_get_dram_addr();#ifndef CONFIG_SYS_LS_MC_DPC_IN_DDR void *aiop_img; #endif
@@ -440,7 +451,6 @@ int mc_init(u64 mc_fw_addr, u64 mc_dpc_addr) size_t raw_image_size = 0; #endif struct mc_version mc_ver_info;
u64 mc_ram_aligned_base_addr; u8 mc_ram_num_256mb_blocks; size_t mc_ram_size = mc_get_dram_block_size();
You showed the deletion of local variable. You added a static variable, but also use local variable within function. The only place where this static variable gets assigned is by a pointer in calculate_mc_private_ram_params(). I don't see the need to declare it as a global variable.
#ifdef CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET static int mc_aiop_applied = -1; #endif @@ -87,12 +88,15 @@ void dump_mc_ccsr_regs(struct mc_ccsr_registers __iomem *mc_ccsr_regs) /**
- Copying MC firmware or DPL image to DDR
*/ -static int mc_copy_image(const char *title,
u64 image_addr, u32 image_size, u64 mc_ram_addr)+static int mc_copy_image(const char *title, u64 image_addr,
u32 image_size, u64 mc_ram_aligned_base_addr){
- debug("%s copied to address %p\n", title, (void *)mc_ram_addr);
- memcpy((void *)mc_ram_addr, (void *)image_addr, image_size);
- flush_dcache_range(mc_ram_addr, mc_ram_addr + image_size);
- debug("%s copied to address %p\n", title,
(void *)mc_ram_aligned_base_addr);- memcpy((void *)mc_ram_aligned_base_addr,
(void *)image_addr, image_size);- flush_dcache_range(mc_ram_aligned_base_addr,
mc_ram_aligned_base_addr + image_size);}
@@ -224,7 +228,8 @@ static int mc_fixup_dpc(u64 dpc_addr) return 0; }
-static int load_mc_dpc(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpc_addr) +static int load_mc_dpc(u64 mc_ram_aligned_base_addr, size_t mc_ram_size,
u64 mc_dpc_addr){ u64 mc_dpc_offset; #ifndef CONFIG_SYS_LS_MC_DPC_IN_DDR @@ -246,7 +251,8 @@ static int load_mc_dpc(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpc_addr) * Load the MC DPC blob in the MC private DRAM block: */ #ifdef CONFIG_SYS_LS_MC_DPC_IN_DDR
- printf("MC DPC is preloaded to %#llx\n", mc_ram_addr + mc_dpc_offset);
- printf("MC DPC is preloaded to %#llx\n",
mc_ram_aligned_base_addr + mc_dpc_offset);#else /* * Get address and size of the DPC blob stored in flash: @@ -270,18 +276,20 @@ static int load_mc_dpc(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpc_addr) return -EINVAL; }
- mc_copy_image("MC DPC blob",
(u64)dpc_fdt_hdr, dpc_size, mc_ram_addr + mc_dpc_offset);
- mc_copy_image("MC DPC blob", (u64)dpc_fdt_hdr, dpc_size,
mc_ram_aligned_base_addr + mc_dpc_offset);#endif /* not defined CONFIG_SYS_LS_MC_DPC_IN_DDR */
- if (mc_fixup_dpc(mc_ram_addr + mc_dpc_offset))
- if (mc_fixup_dpc(mc_ram_aligned_base_addr + mc_dpc_offset)) return -EINVAL;
- dump_ram_words("DPC", (void *)(mc_ram_addr + mc_dpc_offset));
- dump_ram_words("DPC",
(void *)(mc_ram_aligned_base_addr + mc_dpc_offset));}
-static int load_mc_dpl(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpl_addr) +static int load_mc_dpl(u64 mc_ram_aligned_base_addr, size_t mc_ram_size,
u64 mc_dpl_addr){ u64 mc_dpl_offset; #ifndef CONFIG_SYS_LS_MC_DPL_IN_DDR @@ -303,7 +311,8 @@ static int load_mc_dpl(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpl_addr) * Load the MC DPL blob in the MC private DRAM block: */ #ifdef CONFIG_SYS_LS_MC_DPL_IN_DDR
- printf("MC DPL is preloaded to %#llx\n", mc_ram_addr + mc_dpl_offset);
- printf("MC DPL is preloaded to %#llx\n",
mc_ram_aligned_base_addr + mc_dpl_offset);#else /* * Get address and size of the DPL blob stored in flash: @@ -323,11 +332,12 @@ static int load_mc_dpl(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpl_addr) return -EINVAL; }
- mc_copy_image("MC DPL blob",
(u64)dpl_fdt_hdr, dpl_size, mc_ram_addr + mc_dpl_offset);
- mc_copy_image("MC DPL blob", (u64)dpl_fdt_hdr, dpl_size,
mc_ram_aligned_base_addr + mc_dpl_offset);#endif /* not defined CONFIG_SYS_LS_MC_DPL_IN_DDR */
- dump_ram_words("DPL", (void *)(mc_ram_addr + mc_dpl_offset));
- dump_ram_words("DPL",
(void *)(mc_ram_aligned_base_addr + mc_dpl_offset));}
@@ -364,7 +374,6 @@ __weak bool soc_has_aiop(void)
static int load_mc_aiop_img(u64 aiop_fw_addr) {
- u64 mc_ram_addr = mc_get_dram_addr();
#ifndef CONFIG_SYS_LS_MC_DPC_IN_DDR void *aiop_img; #endif @@ -377,13 +386,14 @@ static int load_mc_aiop_img(u64 aiop_fw_addr) */
#ifdef CONFIG_SYS_LS_MC_DPC_IN_DDR
- printf("MC AIOP is preloaded to %#llx\n", mc_ram_addr +
- printf("MC AIOP is preloaded to %#llx\n", mc_ram_aligned_base_addr + CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET);
#else aiop_img = (void *)aiop_fw_addr; mc_copy_image("MC AIOP image", (u64)aiop_img, CONFIG_SYS_LS_MC_AIOP_IMG_MAX_LENGTH,
mc_ram_addr + CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET);
mc_ram_aligned_base_addr +CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET);#endif mc_aiop_applied = 0;
@@ -450,7 +460,6 @@ int mc_init(u64 mc_fw_addr, u64 mc_dpc_addr) size_t raw_image_size = 0; #endif struct mc_version mc_ver_info;
- u64 mc_ram_aligned_base_addr; u8 mc_ram_num_256mb_blocks; size_t mc_ram_size = mc_get_dram_block_size();
@@ -478,7 +487,7 @@ int mc_init(u64 mc_fw_addr, u64 mc_dpc_addr) dmb();
#ifdef CONFIG_SYS_LS_MC_FW_IN_DDR
- printf("MC firmware is preloaded to %#llx\n", mc_ram_addr);
- printf("MC firmware is preloaded to %#llx\n",
+mc_ram_aligned_base_addr); #else error = parse_mc_firmware_fit_image(mc_fw_addr, &raw_image_addr, &raw_image_size); @@ -487,12 +496,12 @@ int mc_init(u64 mc_fw_addr, u64 mc_dpc_addr) /* * Load the MC FW at the beginning of the MC private DRAM block: */
- mc_copy_image("MC Firmware",
(u64)raw_image_addr, raw_image_size, mc_ram_addr);
- mc_copy_image("MC Firmware", (u64)raw_image_addr, raw_image_size,
mc_ram_aligned_base_addr);#endif
- dump_ram_words("firmware", (void *)mc_ram_addr);
- dump_ram_words("firmware", (void *)mc_ram_aligned_base_addr);
- error = load_mc_dpc(mc_ram_addr, mc_ram_size, mc_dpc_addr);
- error = load_mc_dpc(mc_ram_aligned_base_addr, mc_ram_size,
+mc_dpc_addr); if (error != 0) goto out;
@@ -569,10 +578,9 @@ int mc_apply_dpl(u64 mc_dpl_addr) struct mc_ccsr_registers __iomem *mc_ccsr_regs = MC_CCSR_BASE_ADDR; int error = 0; u32 reg_gsr;
u64 mc_ram_addr = mc_get_dram_addr(); size_t mc_ram_size = mc_get_dram_block_size();
error = load_mc_dpl(mc_ram_addr, mc_ram_size, mc_dpl_addr);
- error = load_mc_dpl(mc_ram_aligned_base_addr, mc_ram_size,
+mc_dpl_addr); if (error != 0) return error;
Do you have substantial change beside the changing name from mc_ram_addr to mc_ram_aligned_base_addr? [Ashish Kumar] It is not exactly name change. Here intent is to use userdefine memory size for MC before this only 512MB of memory can be allocated to MC since it incorrectly used mc_ram_addr in place of mc_aligned_base_addr. Ok, Name changes are there only in the function parameters to avoid confusion and retain the function signatures.
Actually your change is more confusing. Let us try another way, for example not changing the name, shall we?
York
Hello York,
Please see inline.
Regards
-----Original Message----- From: york sun Sent: Tuesday, November 08, 2016 11:38 PM To: Ashish Kumar ashish.kumar@nxp.com; Priyanka Jain priyanka.jain@nxp.com; u-boot@lists.denx.de Cc: Prabhakar Kushwaha prabhakar.kushwaha@nxp.com Subject: Re: [PATCH] driver: net: fsl-mc: Use aligned address for MC FW load
On 11/07/2016 09:37 PM, Ashish Kumar wrote:
Hello York,
Please see inline.
Regards Ashish
-----Original Message----- From: york sun Sent: Tuesday, November 08, 2016 12:23 AM To: Priyanka Jain priyanka.jain@nxp.com; u-boot@lists.denx.de Cc: Prabhakar Kushwaha prabhakar.kushwaha@nxp.com; Ashish Kumar ashish.kumar@nxp.com Subject: Re: [PATCH] driver: net: fsl-mc: Use aligned address for MC FW load
On 10/24/2016 01:05 AM, Priyanka Jain wrote:
Firmware of Management Complex (MC) should be loaded at 512MB aligned address. So use aligned address for firmware load.
Signed-off-by: Priyanka Jain priyanka.jain@nxp.com Signed-off-by: Prabhakar Kushwaha prabhakar.kushwaha@nxp.com Signed-off-by: Ashish Kumar Ashish.Kumar@nxp.com
drivers/net/fsl-mc/mc.c | 62 ++++++++++++++++++++++++++-------------------- 1 files changed, 35 insertions(+), 27 deletions(-)
diff --git a/drivers/net/fsl-mc/mc.c b/drivers/net/fsl-mc/mc.c index 1811b0f..c66340b 100644 --- a/drivers/net/fsl-mc/mc.c +++ b/drivers/net/fsl-mc/mc.c @@ -29,6 +29,7 @@ DECLARE_GLOBAL_DATA_PTR; static int mc_boot_status = -1; static int mc_dpl_applied = -1; +static u64 mc_ram_aligned_base_addr;
Why do you need this static variable? You didn't use it. [Ashish Kumar] following two function uses global static variable -357,7 +369,6 @@ static unsigned long get_mc_boot_timeout_ms(void) #ifdef CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET static int load_mc_aiop_img(u64 aiop_fw_addr) {
u64 mc_ram_addr = mc_get_dram_addr();#ifndef CONFIG_SYS_LS_MC_DPC_IN_DDR void *aiop_img; #endif
@@ -440,7 +451,6 @@ int mc_init(u64 mc_fw_addr, u64 mc_dpc_addr) size_t raw_image_size = 0; #endif struct mc_version mc_ver_info;
u64 mc_ram_aligned_base_addr; u8 mc_ram_num_256mb_blocks; size_t mc_ram_size = mc_get_dram_block_size();
You showed the deletion of local variable. You added a static variable, but also use local variable within function. The only place where this static variable gets assigned is by a pointer in calculate_mc_private_ram_params(). I don't see the need to declare it as a global variable. [Ashish Kumar] Yes true, it is first set here then used from global instance in func: mc_apply_dpl(64 mc_dpl_addr) { ... error = load_mc_dpl(mc_ram_aligned_base_addr, mc_ram_size, mc_dpl_addr);... } and
Then in func: load_mc_aiop_img((u64 aiop_fw_addr) {... mc_copy_image(...) ...}
#ifdef CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET static int mc_aiop_applied = -1; #endif @@ -87,12 +88,15 @@ void dump_mc_ccsr_regs(struct mc_ccsr_registers __iomem *mc_ccsr_regs) /**
- Copying MC firmware or DPL image to DDR
*/ -static int mc_copy_image(const char *title,
u64 image_addr, u32 image_size, u64 mc_ram_addr)+static int mc_copy_image(const char *title, u64 image_addr,
u32 image_size, u64 mc_ram_aligned_base_addr){
- debug("%s copied to address %p\n", title, (void *)mc_ram_addr);
- memcpy((void *)mc_ram_addr, (void *)image_addr, image_size);
- flush_dcache_range(mc_ram_addr, mc_ram_addr + image_size);
- debug("%s copied to address %p\n", title,
(void *)mc_ram_aligned_base_addr);- memcpy((void *)mc_ram_aligned_base_addr,
(void *)image_addr, image_size);- flush_dcache_range(mc_ram_aligned_base_addr,
mc_ram_aligned_base_addr + image_size);}
@@ -224,7 +228,8 @@ static int mc_fixup_dpc(u64 dpc_addr) return 0; }
-static int load_mc_dpc(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpc_addr) +static int load_mc_dpc(u64 mc_ram_aligned_base_addr, size_t mc_ram_size,
u64 mc_dpc_addr){ u64 mc_dpc_offset; #ifndef CONFIG_SYS_LS_MC_DPC_IN_DDR @@ -246,7 +251,8 @@ static int load_mc_dpc(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpc_addr) * Load the MC DPC blob in the MC private DRAM block: */ #ifdef CONFIG_SYS_LS_MC_DPC_IN_DDR
- printf("MC DPC is preloaded to %#llx\n", mc_ram_addr + mc_dpc_offset);
- printf("MC DPC is preloaded to %#llx\n",
mc_ram_aligned_base_addr + mc_dpc_offset);#else /* * Get address and size of the DPC blob stored in flash: @@ -270,18 +276,20 @@ static int load_mc_dpc(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpc_addr) return -EINVAL; }
- mc_copy_image("MC DPC blob",
(u64)dpc_fdt_hdr, dpc_size, mc_ram_addr + mc_dpc_offset);
- mc_copy_image("MC DPC blob", (u64)dpc_fdt_hdr, dpc_size,
mc_ram_aligned_base_addr + mc_dpc_offset);#endif /* not defined CONFIG_SYS_LS_MC_DPC_IN_DDR */
- if (mc_fixup_dpc(mc_ram_addr + mc_dpc_offset))
- if (mc_fixup_dpc(mc_ram_aligned_base_addr + mc_dpc_offset)) return -EINVAL;
- dump_ram_words("DPC", (void *)(mc_ram_addr + mc_dpc_offset));
- dump_ram_words("DPC",
(void *)(mc_ram_aligned_base_addr + mc_dpc_offset));}
-static int load_mc_dpl(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpl_addr) +static int load_mc_dpl(u64 mc_ram_aligned_base_addr, size_t mc_ram_size,
u64 mc_dpl_addr){ u64 mc_dpl_offset; #ifndef CONFIG_SYS_LS_MC_DPL_IN_DDR @@ -303,7 +311,8 @@ static int load_mc_dpl(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpl_addr) * Load the MC DPL blob in the MC private DRAM block: */ #ifdef CONFIG_SYS_LS_MC_DPL_IN_DDR
- printf("MC DPL is preloaded to %#llx\n", mc_ram_addr + mc_dpl_offset);
- printf("MC DPL is preloaded to %#llx\n",
mc_ram_aligned_base_addr + mc_dpl_offset);#else /* * Get address and size of the DPL blob stored in flash: @@ -323,11 +332,12 @@ static int load_mc_dpl(u64 mc_ram_addr, size_t mc_ram_size, u64 mc_dpl_addr) return -EINVAL; }
- mc_copy_image("MC DPL blob",
(u64)dpl_fdt_hdr, dpl_size, mc_ram_addr + mc_dpl_offset);
- mc_copy_image("MC DPL blob", (u64)dpl_fdt_hdr, dpl_size,
mc_ram_aligned_base_addr + mc_dpl_offset);#endif /* not defined CONFIG_SYS_LS_MC_DPL_IN_DDR */
- dump_ram_words("DPL", (void *)(mc_ram_addr + mc_dpl_offset));
- dump_ram_words("DPL",
(void *)(mc_ram_aligned_base_addr + mc_dpl_offset));}
@@ -364,7 +374,6 @@ __weak bool soc_has_aiop(void)
static int load_mc_aiop_img(u64 aiop_fw_addr) {
- u64 mc_ram_addr = mc_get_dram_addr();
#ifndef CONFIG_SYS_LS_MC_DPC_IN_DDR void *aiop_img; #endif @@ -377,13 +386,14 @@ static int load_mc_aiop_img(u64 aiop_fw_addr) */
#ifdef CONFIG_SYS_LS_MC_DPC_IN_DDR
- printf("MC AIOP is preloaded to %#llx\n", mc_ram_addr +
- printf("MC AIOP is preloaded to %#llx\n", mc_ram_aligned_base_addr
++ CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET); #else aiop_img = (void *)aiop_fw_addr; mc_copy_image("MC AIOP image", (u64)aiop_img, CONFIG_SYS_LS_MC_AIOP_IMG_MAX_LENGTH,
mc_ram_addr + CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET);
mc_ram_aligned_base_addr +CONFIG_SYS_LS_MC_DRAM_AIOP_IMG_OFFSET);#endif mc_aiop_applied = 0;
@@ -450,7 +460,6 @@ int mc_init(u64 mc_fw_addr, u64 mc_dpc_addr) size_t raw_image_size = 0; #endif struct mc_version mc_ver_info;
- u64 mc_ram_aligned_base_addr; u8 mc_ram_num_256mb_blocks; size_t mc_ram_size = mc_get_dram_block_size();
@@ -478,7 +487,7 @@ int mc_init(u64 mc_fw_addr, u64 mc_dpc_addr) dmb();
#ifdef CONFIG_SYS_LS_MC_FW_IN_DDR
- printf("MC firmware is preloaded to %#llx\n", mc_ram_addr);
- printf("MC firmware is preloaded to %#llx\n",
+mc_ram_aligned_base_addr); #else error = parse_mc_firmware_fit_image(mc_fw_addr, &raw_image_addr, &raw_image_size); @@ -487,12 +496,12 @@ int mc_init(u64 mc_fw_addr, u64 mc_dpc_addr) /* * Load the MC FW at the beginning of the MC private DRAM block: */
- mc_copy_image("MC Firmware",
(u64)raw_image_addr, raw_image_size, mc_ram_addr);
- mc_copy_image("MC Firmware", (u64)raw_image_addr, raw_image_size,
mc_ram_aligned_base_addr);#endif
- dump_ram_words("firmware", (void *)mc_ram_addr);
- dump_ram_words("firmware", (void *)mc_ram_aligned_base_addr);
- error = load_mc_dpc(mc_ram_addr, mc_ram_size, mc_dpc_addr);
- error = load_mc_dpc(mc_ram_aligned_base_addr, mc_ram_size,
+mc_dpc_addr); if (error != 0) goto out;
@@ -569,10 +578,9 @@ int mc_apply_dpl(u64 mc_dpl_addr) struct mc_ccsr_registers __iomem *mc_ccsr_regs = MC_CCSR_BASE_ADDR; int error = 0; u32 reg_gsr;
u64 mc_ram_addr = mc_get_dram_addr(); size_t mc_ram_size = mc_get_dram_block_size();
error = load_mc_dpl(mc_ram_addr, mc_ram_size, mc_dpl_addr);
- error = load_mc_dpl(mc_ram_aligned_base_addr, mc_ram_size,
+mc_dpl_addr); if (error != 0) return error;
Do you have substantial change beside the changing name from mc_ram_addr to mc_ram_aligned_base_addr? [Ashish Kumar] It is not exactly name change. Here intent is to use userdefine memory size for MC before this only 512MB of memory can be allocated to MC since it incorrectly used mc_ram_addr in place of mc_aligned_base_addr. Ok, Name changes are there only in the function parameters to avoid confusion and retain the function signatures.
Actually your change is more confusing. Let us try another way, for example not changing the name, shall we? [Ashish Kumar] If we do not rename function params "mc_ram_addr" to this "mc_ram_aligned_base_addr", we will be actually using aligned_base_addr but in function params it will be denoted as mc_ram_addr will that be correct?
York
On 11/09/2016 04:03 AM, Ashish Kumar wrote:
Do you have substantial change beside the changing name from mc_ram_addr to mc_ram_aligned_base_addr? [Ashish Kumar] It is not exactly name change. Here intent is to use userdefine memory size for MC before this only 512MB of memory can be allocated to MC since it incorrectly used mc_ram_addr in place of mc_aligned_base_addr. Ok, Name changes are there only in the function parameters to avoid confusion and retain the function signatures.
Actually your change is more confusing. Let us try another way, for example not changing the name, shall we? [Ashish Kumar] If we do not rename function params "mc_ram_addr" to this "mc_ram_aligned_base_addr", we will be actually using aligned_base_addr but in function params it will be denoted as mc_ram_addr will that be correct?
I think the only substantial change you have is in mc_init() function, where you moved the local variable mc_ram_aligned_base_addr to global. You started to use this global variable below and passed it as function parameters several times. For those passed as parameters, you don't need to rename them. If you take out those, you don't have to reformat the line wrap. So most of your changes can be avoided. Try that, you will have very small change and easy to review. Maybe you can find a better way to deal with alignment.
York
participants (3)
-
Ashish Kumar -
Priyanka Jain -
york sun