From mboxrd@z Thu Jan  1 00:00:00 1970
From: Pavel Machek <pavel@denx.de>
Date: Mon, 15 Sep 2014 23:21:22 +0200
Subject: [U-Boot] [PATCH 12/35] arm: socfpga: clock: Implant order into
 bit definitions
In-Reply-To: <20140915152626.7B3D03823E1@gemini.denx.de>
References: <1410779188-6880-1-git-send-email-marex@denx.de>
	<1410779188-6880-13-git-send-email-marex@denx.de>
	<20140915152626.7B3D03823E1@gemini.denx.de>
Message-ID: <20140915212122.GA32588@amd>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi!

> Dear Marek Vasut,
> In message <1410779188-6880-13-git-send-email-marex@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