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