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();
> #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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
