[U-Boot] [PATCH 12/35] arm: socfpga: clock: Implant order into bit definitions

[Marek Vasut]

On Monday, September 15, 2014 at 11:21:22 PM, Pavel Machek wrote:
> Hi!
> 
> > Dear Marek Vasut,
> > 
> > In message <1410779188-6880-13-git-send-email-marex at denx.de> you wrote:
> > > The bit definitions for clock manager are complete chaos. Implement
> > > some basic logical order into them.
> > 
> > ...
> > 
> > > +#define CLKMGR_BYPASS_MAINPLL_SET(x)		(((x) << 0) & 0x00000001)
> > > +#define CLKMGR_BYPASS_PERPLLSRC_SET(x)	(((x) << 4) & 0x00000010)
> > > +#define CLKMGR_BYPASS_PERPLL_SET(x)		(((x) << 3) & 0x00000008)
> > > +#define CLKMGR_BYPASS_SDRPLLSRC_SET(x)	(((x) << 2) & 0x00000004)
> > > +#define CLKMGR_BYPASS_SDRPLL_SET(x)		(((x) << 1) & 0x00000002)
> > 
> > What is the purpose of all these funny shift operation shere?  Is it
> > just to obfuscate the meaning of the code, or is the code eventually
> > wrong?
> > 
> > As is above could be rewritten much simpler without the shifts:
> > 
> > #define CLKMGR_BYPASS_MAINPLL_SET(x)	((x) & 1)
> > #define CLKMGR_BYPASS_PERPLLSRC_SET(x)	((x) & 1)
> > #define CLKMGR_BYPASS_PERPLL_SET(x)	((x) & 1)
> > #define CLKMGR_BYPASS_SDRPLLSRC_SET(x)	((x) & 1)
> > #define CLKMGR_BYPASS_SDRPLL_SET(x)	((x) & 1)
> > 
> > 
> > Note also that the macros are misnamed - "<something>_SET" means an
> > action, i. e. the macro is supposed to set some bits in the argument,
> > but here you actually perform a test if something "is set".
> 
> Not the way macros are used; they really use them to set bits:
> 
>         /* Put all plls in bypass */
>         cm_write_bypass(
>                 CLKMGR_BYPASS_PERPLLSRC_SET(
>                 CLKMGR_BYPASS_PERPLLSRC_SELECT_EOSC1) |
>                 CLKMGR_BYPASS_SDRPLLSRC_SET(
>                 CLKMGR_BYPASS_SDRPLLSRC_SELECT_EOSC1) |
> 		CLKMGR_BYPASS_PERPLL_SET(CLKMGR_BYPASS_ENABLE)
> 
>                 CLKMGR_BYPASS_SDRPLL_SET(CLKMGR_BYPASS_ENABLE) |
>                 CLKMGR_BYPASS_MAINPLL_SET(CLKMGR_BYPASS_ENABLE));
> 
> 
> So replacing with ((x) & 1) would not work.
> 
> But yes, there are more cleanups that could be done there.

You surely wanted to say "must be done where" ;-) I've yet to decide if I'll do 
it before this patchset or after ; I am more inclined doing it as a subsequent 
patch to allow Altera guys review the changes and match them with their existing 
code. And only then do the cleanup , probably automatically using some script.

What do you think, Wolfgang, Pavel, Dinh, others ... ?

Best regards,
Marek Vasut