[U-Boot] [RESEND PATCH v2 1/5] Tegra2: Add macros to calculate bitfield shifts and masks

[Anton Staaf]

On Thu, Jul 14, 2011 at 11:44 AM, Wolfgang Denk <wd at denx.de> wrote:
> Dear Anton Staaf,
>
> In message <CAF6FioVOEuNcEsr=3jyxoVs__dO3Ox+q00PnDBRHdu+UmJZF2g at mail.gmail.com> you wrote:
>>
>> In both cases the value 20 needs to come from somewhere.  So you would
>> probably end up having:
>>
>>    #define SCFR1_IPS_DIV_MASK      0x03800000
>>    #define SCFR1_IPS_DIV_SHIFT      20
>>
>> and
>>
>>    (value & SCFR1_IPS_DIV_MASK) >> SCFR1_IPS_DIV_SHIFT
>>    (value & ~SCFR1_IPS_DIV_MASK) | (5 << SCFR1_IPS_DIV_SHIFT)
>
> BTW - you are correct about this.  The file I grabbed the examples
> from is "arch/powerpc/include/asm/immap_512x.h"; here is the full
> context:
>
>  229 /* SCFR1 System Clock Frequency Register 1 */
>  230 #define SCFR1_IPS_DIV           0x3
>  231 #define SCFR1_IPS_DIV_MASK      0x03800000
>  232 #define SCFR1_IPS_DIV_SHIFT     23
>  233
>  234 #define SCFR1_PCI_DIV           0x6
>  235 #define SCFR1_PCI_DIV_MASK      0x00700000
>  236 #define SCFR1_PCI_DIV_SHIFT     20
>  237
>  238 #define SCFR1_LPC_DIV_MASK      0x00003800
>  239 #define SCFR1_LPC_DIV_SHIFT     11
>  240
>  241 /* SCFR2 System Clock Frequency Register 2 */
>  242 #define SCFR2_SYS_DIV           0xFC000000
>  243 #define SCFR2_SYS_DIV_SHIFT     26
>
> And indeed we see code using this for example in
> arch/powerpc/cpu/mpc512x/speed.c:
>
>  98         reg = in_be32(&im->clk.scfr[0]);
>  99         ips_div = (reg & SCFR1_IPS_DIV_MASK) >> SCFR1_IPS_DIV_SHIFT;

I agree, this line is completely obvious and if it were the only sort
of GET (or it's equivalent SET) version used I wouldn't have suggested
the macro at all.  It's the lines that are setting many fields
simultaneously, sometimes with constant field values and sometimes
with computed values that become a bit hard to parse, and would
benefit from the abstraction.  But good coding practice can break
these statements up into a collection of simple ones to do the
manipulations one at a time.  Then the real benefit of the macro
becomes a compression of the syntax, leading to shorter functions, and
in my option a reduced time to parse and understand the actions.  But
as you say, it hides part of the implementation, but this is also true
for any other function, such as the fact that writel does a memory
barrier.  But such functions are part of the existing U-Boot knowledge
base, so their behavior is expected and understood by it's users.  I
see no reason that the same could not eventually be the case for field
access macros.

But as I've said, I'm OK with dropping this.  Any indication above to
the prior is simply me being an engineer who perceives a problem that
I can solve and desiring to have my perspective validated.  :)  And
now back to sending actual useful patches to the list.

Thanks,
    Anton

> The nice thing (for me) here is, that without even thinking for a
> second I know exactly what is going on - there is nothing in this
> statements that require me too look up some macro definition. [Yes,
> of course this is based on the assumption that macro names
> <register>_MASK and <register>_SHIFT just do what they are suggest
> they are doing.  But any such things get filtered out during the
> reviews.]
>
> 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
> Vulcans never bluff.
>        -- Spock, "The Doomsday Machine", stardate 4202.1
>