Re: [PATCH 2/5] soc/fsl/qe: qe.c: reduce static memory footprint by 1.7K

[Rasmus Villemoes]

On 30/04/2019 19.12, Christophe Leroy wrote:
>  
> Le 30/04/2019 Ã 15:36, Rasmus Villemoes a ÃcritÂ:
>> The current array of struct qe_snum use 256*4 bytes for just keeping
>> track of the free/used state of each index, and the struct layout
>> means there's another 768 bytes of padding. If we just unzip that
>> structure, the array of snum values just use 256 bytes, while the
>> free/inuse state can be tracked in a 32 byte bitmap.
>>
>> So this reduces the .data footprint by 1760 bytes. It also serves as
>> preparation for introducing another DT binding for specifying the snum
>> values.
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@xxxxxxxxx>
>> ---
>> -
>> Â /* We allocate this here because it is used almost exclusively for
>> ÂÂ * the communication processor devices.
>> ÂÂ */
>> Â struct qe_immap __iomem *qe_immr;
>> Â EXPORT_SYMBOL(qe_immr);
>> Â -static struct qe_snum snums[QE_NUM_OF_SNUM];ÂÂÂ /* Dynamically
>> allocated SNUMs */
>> +static u8 snums[QE_NUM_OF_SNUM];ÂÂÂ /* Dynamically allocated SNUMs */
>> +static DECLARE_BITMAP(snum_state, QE_NUM_OF_SNUM);
>> Â static unsigned int qe_num_of_snum;
>> Â Â static phys_addr_t qebase = -1;
>> @@ -308,6 +298,7 @@ static void qe_snums_init(void)
>> ÂÂÂÂÂ };
>> ÂÂÂÂÂ const u8 *snum_init;
>> Â +ÂÂÂ bitmap_zero(snum_state, QE_NUM_OF_SNUM);
> 
> Doesn't make much importance, but wouldn't it be more logical to add
> this line where the setting of .state = QE_SNUM_STATE_FREE was done
> previously, ie around the for() loop below ?

This was on purpose, to avoid having to move it up in patch 4, where we
don't necessarily reach the for loop.

>> ÂÂÂÂÂ qe_num_of_snum = qe_get_num_of_snums();
>> Â ÂÂÂÂÂ if (qe_num_of_snum == 76)
>> @@ -315,10 +306,8 @@ static void qe_snums_init(void)
>> ÂÂÂÂÂ else
>> ÂÂÂÂÂÂÂÂÂ snum_init = snum_init_46;
>> Â -ÂÂÂ for (i = 0; i < qe_num_of_snum; i++) {
>> -ÂÂÂÂÂÂÂ snums[i].num = snum_init[i];
>> -ÂÂÂÂÂÂÂ snums[i].state = QE_SNUM_STATE_FREE;
>> -ÂÂÂ }
>> +ÂÂÂ for (i = 0; i < qe_num_of_snum; i++)
>> +ÂÂÂÂÂÂÂ snums[i] = snum_init[i];
> 
> Could use memcpy() instead ?

Yes, I switch to that in 5/5. Sure, I could do it here already, but I
did it this way to keep close to the current style. I don't care either
way, so if you prefer introducing memcpy here, fine by me.


>> ÂÂÂÂÂ spin_unlock_irqrestore(&qe_lock, flags);
>> Â @@ -346,8 +333,8 @@ void qe_put_snum(u8 snum)
>> ÂÂÂÂÂ int i;
>> Â ÂÂÂÂÂ for (i = 0; i < qe_num_of_snum; i++) {
>> -ÂÂÂÂÂÂÂ if (snums[i].num == snum) {
>> -ÂÂÂÂÂÂÂÂÂÂÂ snums[i].state = QE_SNUM_STATE_FREE;
>> +ÂÂÂÂÂÂÂ if (snums[i] == snum) {
>> +ÂÂÂÂÂÂÂÂÂÂÂ clear_bit(i, snum_state);
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂ break;
>> ÂÂÂÂÂÂÂÂÂ }
>> ÂÂÂÂÂ }
> 
> Can we replace this loop by memchr() ?

Hm, yes. So that would be

  const u8 *p = memchr(snums, snum, qe_num_of_snum)
  if (p)
    clear_bit(p - snums, snum_state);

I guess. Let me fold that in and see how it looks.

Thanks,
Rasmus