[U-Boot] [PATCH] driver: net: fsl-mc: Update MC memory allocation code

[york sun]

On 12/19/2016 10:57 PM, Priyanka Jain wrote:
>
>
>> -----Original Message-----
>> From: york sun
>> Sent: Monday, December 19, 2016 10:43 PM
>> To: Priyanka Jain <priyanka.jain at nxp.com>; u-boot at lists.denx.de
>> Subject: Re: [PATCH] driver: net: fsl-mc: Update MC memory allocation code
>>
>> On 12/18/2016 09:11 PM, Priyanka Jain wrote:
>>> Firmware of Management Complex (MC) should be loaded at 512MB aligned
>>> privately allocated memory at DRAM end.
>>> And this memory should be reduced from total memory available for
>>> general purposes.
>>
>> It is already reserved at the alignment of CONFIG_SYS_MC_RSV_MEM_ALIGN,
>> which is 512MB. What are you changing?
>>
>>>
>>> Update memory allocation code in MC driver to support above
>>> requirements.
>>>
>>> Signed-off-by: Priyanka Jain <priyanka.jain at nxp.com>
>>> ---
>>>  drivers/net/fsl-mc/mc.c |   98 +++++++++++++++++++++-------------------------
>>>  1 files changed, 45 insertions(+), 53 deletions(-)
>>>
>>> diff --git a/drivers/net/fsl-mc/mc.c b/drivers/net/fsl-mc/mc.c index
>>> 46b8a6b..df3f7fa 100644
>>> --- a/drivers/net/fsl-mc/mc.c
>>> +++ b/drivers/net/fsl-mc/mc.c
>>> @@ -1,5 +1,5 @@
>>>  /*
>>> - * Copyright (C) 2014 Freescale Semiconductor
>>> + * Copyright (C) 2014, 2016 Freescale Semiconductor
>>>   *
>>>   * SPDX-License-Identifier:	GPL-2.0+
>>>   */
>>> @@ -41,6 +41,7 @@ struct fsl_dpbp_obj *dflt_dpbp = NULL;  struct
>>> fsl_dpio_obj *dflt_dpio = NULL;  struct fsl_dpni_obj *dflt_dpni =
>>> NULL;  static u64 mc_lazy_dpl_addr;
>>> +static u64 mc_ram_addr;
>>>
>>>  #ifdef DEBUG
>>>  void dump_ram_words(const char *title, void *addr) @@ -89,11 +90,11
>>> @@ 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)
>>> +			 u64 image_addr, u32 image_size, u64 mc_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_addr);
>>> +	memcpy((void *)mc_addr, (void *)image_addr, image_size);
>>> +	flush_dcache_range(mc_addr, mc_addr + image_size);
>>>  	return 0;
>>>  }
>>>
>>> @@ -156,16 +157,14 @@ int parse_mc_firmware_fit_image(u64
>> mc_fw_addr,
>>>  /*
>>>   * Calculates the values to be used to specify the address range
>>>   * for the MC private DRAM block, in the MCFBALR/MCFBAHR registers.
>>> - * It returns the highest 512MB-aligned address within the given
>>> - * address range, in '*aligned_base_addr', and the number of 256 MiB
>>> - * blocks in it, in 'num_256mb_blocks'.
>>> + * It stores the highest 512MB-aligned address within the given
>>> + * address range, in 'mc_ram_addr', and returns the number of 256 MB
>>> + * blocks in 'num_256mb_blocks'.
>>>   */
>>> -static int calculate_mc_private_ram_params(u64
>> mc_private_ram_start_addr,
>>> -					   size_t mc_ram_size,
>>> -					   u64 *aligned_base_addr,
>>> +static int calculate_mc_private_ram_params(size_t mc_ram_size,
>>>  					   u8 *num_256mb_blocks)
>>>  {
>>> -	u64 addr;
>>> +	u64 mc_ram_end_addr;
>>>  	u16 num_blocks;
>>>
>>>  	if (mc_ram_size % MC_RAM_SIZE_ALIGNMENT != 0) { @@ -180,18
>> +179,39
>>> @@ static int calculate_mc_private_ram_params(u64
>> mc_private_ram_start_addr,
>>>  		       mc_ram_size);
>>>  		return -EINVAL;
>>>  	}
>>> +	*num_256mb_blocks = num_blocks;
>>>
>>> -	addr = (mc_private_ram_start_addr + mc_ram_size - 1) &
>>> +	/*
>>> +	 * The MC private DRAM block will be carved at the end of DRAM
>>> +	 */
>>> +	if (gd->bd->bi_dram[1].start) {
>>> +		mc_ram_end_addr =
>>> +			gd->bd->bi_dram[1].start + gd->bd->bi_dram[1].size;
>>> +	} else {
>>> +		mc_ram_end_addr =
>>> +			gd->bd->bi_dram[0].start + gd->bd->bi_dram[0].size;
>>> +	}
>>> +
>>> +	mc_ram_addr = (mc_ram_end_addr - mc_ram_size) &
>>>  		MC_RAM_BASE_ADDR_ALIGNMENT_MASK;
>>>
>>> -	if (addr < mc_private_ram_start_addr) {
>>> -		printf("fsl-mc: ERROR: bad start address %#llx\n",
>>> -		       mc_private_ram_start_addr);
>>> +	if (mc_ram_addr > mc_ram_end_addr) {
>>> +		printf("fsl-mc: ERROR: bad end address %#llx\n",
>>> +		       mc_ram_end_addr);
>>>  		return -EFAULT;
>>>  	}
>>>
>>> -	*aligned_base_addr = addr;
>>> -	*num_256mb_blocks = num_blocks;
>>> +	/*
>>> +	 * Total DRAM available for general purpose will get
>>> +	 * reduced by the size of private DRAM allocated to MC.
>>> +	 */
>>> +	if (gd->bd->bi_dram[1].start) {
>>> +			gd->bd->bi_dram[1].size =
>>> +				mc_ram_addr - gd->bd->bi_dram[1].start;
>>> +	} else {
>>> +			gd->bd->bi_dram[0].size =
>>> +				mc_ram_addr - gd->bd->bi_dram[0].start;
>>> +	}
>>
>>
>> NAK. You cannot change the global memory size within MC driver. Please
>> explain what is not working with current memory reservation.
>>
>> York
>
> With initial code, mc_ram_addr is getting value of end address of non-secure memory which is 0x83c000,0000.
> At further end we have 2GB secure memory, which is intended for PPA.
>
> Based on value of mcmemsize the code is allocating memory mc_ram_addr + mcmemsize which means memory
> is allocated out of secure region.  PPA fits in last 512M block. So, we are left with maximum 1.5GB of secure memory
> available for MC private allocated memory in current code.

MC doesn't use secure memory. It has its own reserved memory under 
secure memory.

>
> Now we have requirement to support upto 5GB memory MC memory. So, I have redesigned the logic to allocate memory from
> end of non-secure memory and reduce this memory available for general purpose.
>
> Please suggest if you think this should be implemented differently

First, I don't know why MC need such huge size of memory. It doesn't 
sound right.

Second, if you do need such huge memory, all you have to do is to return 
correct size from mc_get_dram_block_size().

York