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

[Wolfgang Denk]

Dear Marek,

In message <201409152348.15411.marex at denx.de> you wrote:
>
> >         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));
...
> > 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 ... ?

Well, frankly speaking, the code above (and many similar use cases)
are an unreadable and unmaintainable mess.  I understand that parts of
this are computer-generated definitions, but in my opinion there are
several serious disadvantages:

- The macro names are so long that the code becomes unreadable; the
  CodingStyle recommens in "Chapter 4: Naming":

        C is a Spartan language, and so should your naming be. Unlike
        Modula-2 and Pascal programmers, C programmers do not use
        cute names like ThisVariableIsATemporaryCounter. A C
        programmer would call that variable "tmp", which is much
        easier to write, and not the least more difficult to
        understand.
	...
	LOCAL variable names should be short, and to the point.

- There is virtually no visual difference between macro names that
  take arguments (like CLKMGR_BYPASS_PERPLLSRC_SET()) and macro names
  that are used as arguments (like CLKMGR_BYPASS_PERPLLSRC_SELECT_EOSC1)
  which makes parsing the code even more difficult.

  We should especially keep in mind that things like
  CLKMGR_BYPASS_PERPLLSRC_SELECT_EOSC1 are deined in a single C source
  file only ("arch/arm/cpu/armv7/socfpga/clock_manager.c"), i. e. they
  have strictly local scope, so the long names make even less sense.

- The code not only difficult to read, but even more difficult to
  debug.  Assume you are single stepping through this code, and you
  reach this statement:

> >         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));

  In the debugger, you see

  	cm_write_bypass(0x0000000D);

  How long does it take you to figure out if this is what we expect
  or not?

  The macros are probably intended to offer unlimited flexibility, but
  they come at a cost.  Do we really need this flexibility?  How many
  of the macros are actually used und how many different ways.


Under normal conditions I would simple NAK such a patch and demand
that the code is cleaned up before it gets accepted for mainline.
Unfortunately, we already have this mess in mainline - somehow
"arch/arm/cpu/armv7/socfpga/clock_manager.c" slipped through the
reviews without anybody noticing this stuff.

So accepting this patch now does not make things dramatically worse,
but I understand it would help to prepare the ground for other,
urgently needed cleanup.

So as far as I am concerned I do not intend to block this with a plain
NAK, but I would be really happy if this whole clock manager code
would be put on the list of things that need rework, and we don't
forget this about more recent tasks coming.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Everybody is talking about the  weather  but  nobody  does  anything
about it."                                               - Mark Twain