* [U-Boot] [PATCH] driver: net: fsl-mc: Use aligned address for MC FW load
@ 2016-10-24 8:01 Priyanka Jain
2016-11-07 18:53 ` york sun
0 siblings, 1 reply; 6+ messages in thread
From: Priyanka Jain @ 2016-10-24 8:01 UTC (permalink / raw)
To: u-boot
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;
--
1.7.4.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] driver: net: fsl-mc: Use aligned address for MC FW load
2016-10-24 8:01 [U-Boot] [PATCH] driver: net: fsl-mc: Use aligned address for MC FW load Priyanka Jain
@ 2016-11-07 18:53 ` york sun
2016-11-08 5:37 ` Ashish Kumar
0 siblings, 1 reply; 6+ messages in thread
From: york sun @ 2016-11-07 18:53 UTC (permalink / raw)
To: u-boot
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);
> 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;
>
>
Do you have substantial change beside the changing name from mc_ram_addr
to mc_ram_aligned_base_addr?
York
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] driver: net: fsl-mc: Use aligned address for MC FW load
2016-11-07 18:53 ` york sun
@ 2016-11-08 5:37 ` Ashish Kumar
2016-11-08 18:07 ` york sun
0 siblings, 1 reply; 6+ messages in thread
From: Ashish Kumar @ 2016-11-08 5:37 UTC (permalink / raw)
To: u-boot
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 at 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);
> 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;
>
>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] driver: net: fsl-mc: Use aligned address for MC FW load
2016-11-08 5:37 ` Ashish Kumar
@ 2016-11-08 18:07 ` york sun
2016-11-09 12:03 ` Ashish Kumar
0 siblings, 1 reply; 6+ messages in thread
From: york sun @ 2016-11-08 18:07 UTC (permalink / raw)
To: u-boot
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 at 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);
>> 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;
>>
>>
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] driver: net: fsl-mc: Use aligned address for MC FW load
2016-11-08 18:07 ` york sun
@ 2016-11-09 12:03 ` Ashish Kumar
2016-11-09 17:41 ` york sun
0 siblings, 1 reply; 6+ messages in thread
From: Ashish Kumar @ 2016-11-09 12:03 UTC (permalink / raw)
To: u-boot
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 at 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 at 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);
>> 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;
>>
>>
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] driver: net: fsl-mc: Use aligned address for MC FW load
2016-11-09 12:03 ` Ashish Kumar
@ 2016-11-09 17:41 ` york sun
0 siblings, 0 replies; 6+ messages in thread
From: york sun @ 2016-11-09 17:41 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-11-09 17:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-24 8:01 [U-Boot] [PATCH] driver: net: fsl-mc: Use aligned address for MC FW load Priyanka Jain
2016-11-07 18:53 ` york sun
2016-11-08 5:37 ` Ashish Kumar
2016-11-08 18:07 ` york sun
2016-11-09 12:03 ` Ashish Kumar
2016-11-09 17:41 ` york sun
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.