On 05/29/2012 02:06 PM, Prafulla Wadaskar wrote: > > >> -----Original Message----- >> From: Valentin Longchamp [mailto:[email protected]] >> Sent: 29 May 2012 16:59 >> To: Prafulla Wadaskar >> Cc: [email protected]; [email protected] >> Subject: Re: [PATCH 1/3] kirkwood: add kirkwood_mpp_save/restore >> functions >> >>>> -----Original Message----- >>>> From: Valentin Longchamp [mailto:[email protected]] >>>> Sent: 29 May 2012 14:15 >>>> To: Prafulla Wadaskar >>>> Cc: [email protected]; [email protected] >>>> 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. _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
