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

[Pavel Machek]

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

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html