Hello York,
Please see inline.
Regards
-----Original Message-----
From: york sun
Sent: Tuesday, November 08, 2016 11:38 PM
To: Ashish Kumar <ashish.ku...@nxp.com>; Priyanka Jain <priyanka.j...@nxp.com>;
u-boot@lists.denx.de
Cc: Prabhakar Kushwaha <prabhakar.kushw...@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.j...@nxp.com>; u-boot@lists.denx.de
> Cc: Prabhakar Kushwaha <prabhakar.kushw...@nxp.com>; Ashish Kumar
> <ashish.ku...@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.j...@nxp.com>
>> Signed-off-by: Prabhakar Kushwaha <prabhakar.kushw...@nxp.com>
>> Signed-off-by: Ashish Kumar <ashish.ku...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
