On Thu, Jul 14, 2011 at 11:44 AM, Wolfgang Denk wd@denx.de wrote:
Dear Anton Staaf,
In message CAF6FioVOEuNcEsr=3jyxoVs__dO3Ox+q00PnDBRHdu+UmJZF2g@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@denx.de Vulcans never bluff. -- Spock, "The Doomsday Machine", stardate 4202.1