From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Mon, 15 Sep 2014 23:48:15 +0200
Subject: [U-Boot] [PATCH 12/35] arm: socfpga: clock: Implant order into
	bit definitions
In-Reply-To: <20140915212122.GA32588@amd>
References: <1410779188-6880-1-git-send-email-marex@denx.de>
	<20140915152626.7B3D03823E1@gemini.denx.de>
	<20140915212122.GA32588@amd>
Message-ID: <201409152348.15411.marex@denx.de>
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

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