[U-Boot] [PATCH] driver: net: fsl-mc: Use aligned address for MC FW load
Ashish Kumar
ashish.kumar at nxp.com
Wed Nov 9 13:03:10 CET 2016
Hello York,
Please see inline.
Regards
-----Original Message-----
From: york sun
Sent: Tuesday, November 08, 2016 11:38 PM
To: Ashish Kumar <ashish.kumar at nxp.com>; Priyanka Jain <priyanka.jain at nxp.com>; u-boot at lists.denx.de
Cc: Prabhakar Kushwaha <prabhakar.kushwaha at 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 at nxp.com>; u-boot at lists.denx.de
> Cc: Prabhakar Kushwaha <prabhakar.kushwaha at nxp.com>; Ashish Kumar
> <ashish.kumar at 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 at nxp.com>
>> Signed-off-by: Prabhakar Kushwaha <prabhakar.kushwaha at nxp.com>
>> Signed-off-by: Ashish Kumar <Ashish.Kumar at 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
More information about the U-Boot
mailing list