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

[Prafulla Wadaskar]

> -----Original Message-----
> From: Valentin Longchamp [mailto:valentin.longchamp at keymile.com]
> Sent: 29 May 2012 18:20
> 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
> 
> 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

New ideas are always welcomed, let's keep is separate from this context.

> 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.

I think, Kirkwood_mpp_config (already supported) finally writes the said MPP configurations to the SoC,

If we can have just Kirkwood_mpp_read() function that reads the MPP configuration for the said MPP will be enough from mpp.c prospective.
This function should return the MPP config value in the same way it is being programmed by Kirkwood_mpp_config() function.

In SPI driver we will use these functions to handle the claim/release/backup the MPP attributes.

Regards..
Prafulla . . .