[U-Boot] [PATCH 1/3] kirkwood: add kirkwood_mpp_save/restore functions

[Valentin Longchamp]

On 05/29/2012 02:06 PM, Prafulla Wadaskar wrote:
> 
> 
>> -----Original Message-----
>> From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
>> Sent: 29 May 2012 16:59
>> To: Prafulla Wadaskar
>> Cc: holger.brunck at keymile.com; u-boot at lists.denx.de
>> Subject: Re: [PATCH 1/3] kirkwood: add kirkwood_mpp_save/restore
>> functions
>>
>>>> -----Original Message-----
>>>> From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
>>>> Sent: 29 May 2012 14:15
>>>> To: Prafulla Wadaskar
>>>> Cc: holger.brunck at keymile.com; u-boot at lists.denx.de
>>>> Subject: Re: [PATCH 1/3] kirkwood: add kirkwood_mpp_save/restore
>>>> functions
>>>>
>>> ...snip...
>>>>>
>>>>>> +
>>>>>> +void kirkwood_mpp_save(void)
>>>>> This should be
>>>>> void kirkwood_mpp_save(unsigned int *mpp_ctrl, int len)
>>>>
>>>> Here we save _all_ mpp registers, with direct access to the
>> registers.
>>>> With your
>>>> proposed solution, I would save it in a dynamically allocated table
>> of
>>>> size len.
>>>>
>>>> That's fine for me, but I would then need to export MPP_NR_REGS,
>>>> because that is
>>>> what I would pass as len arg, is that OK ?
>>>
>>> I think in your case you need configuration of 4 MPPs, i.e. from
>> MPP6 to MPP11, so you may declare array of length 7 and backup and
>> restore the same MPPs using len = 6.
>>>
>>> No Need to backup and restore all MPPs.
>>>
>>
>> Sorry, but this is exactly what you did in the kirkwood_mpp_conf
>> function, you
>> read and write all the registers every time you need to change only
>> one pin, I
>> took it from there for consistency:
>>
>>> 4efb77d4 cpu/arm926ejs/kirkwood/mpp.c          (Prafulla Wadaskar
>> 2009-06-20 11:01:53 +0200  76)       for (i = 0; i < MPP_NR_REGS; i++)
>> {
>>> 4efb77d4 cpu/arm926ejs/kirkwood/mpp.c          (Prafulla Wadaskar
>> 2009-06-20 11:01:53 +0200  77)               writel(mpp_ctrl[i],
>> MPP_CTRL(i));
>>> 4efb77d4 cpu/arm926ejs/kirkwood/mpp.c          (Prafulla Wadaskar
>> 2009-06-20 11:01:53 +0200  78)               debug(" %08x",
>> mpp_ctrl[i]);
>>> 4efb77d4 cpu/arm926ejs/kirkwood/mpp.c          (Prafulla Wadaskar
>> 2009-06-20 11:01:53 +0200  79)       }
>>
>> And the way you did it is logical, if it was not done like that, a lot
>> of
>> reading/masking/rewriting would be needed, and this for every single
>> pin, so it
>> would be much less efficient than just reading all the regs and write
>> them all back.
> 
> Yes, but the idea is that mpp_config function is called just once during initialization, we have exposed a simple array to do this configuration.
> 
> but save/restore will be called very frequently, may be for each SPI transaction. So there must be some optimisation.

> Secondly, we should only tweak only needed MPPs in run time, why all? It may lead to some other side effects (I don't know).
> 

With the proposed code, all are saved and restored, but not all are tweaked.
Furthermore, maybe reading and writing back 7 registers and is more efficient
than determining which one of the 7 have to be read/written back and then
performing the accesses.

However, if you tell me that mpp_config function is supposed to be called just
once during initialization, I will propose a new function that optimizes these
reg accesses as you would prefer it, but the best optimization potential is to
avoid to call the 2 mpp_config and mpp_save functions.