[U-Boot] [PATCH 6/7 v2] OMAP3: Add OMAP3 core changes for MUSB

Wolfgang Denk
2009-03-01T19:18:39Z

Dear Dirk,

In message <49AA9B19.6060907@googlemail.com> you wrote:
>
> > Will it get an ACK if we change
...
> > and then call enable/disable from MUSB code at appropriate places?
>
> Any hint if changing the patch doing something like above as proposed
> some days ago [1] would get an ack?

I'd like to see the patch, but so far I don;t see any problems with
that.

Umm.... but this posting triggered me to look up what this function
sr32() might be doing...

I think this could need some cleanup...

"cpu/arm_cortexa8/omap3/syslib.c":

43 /*****************************************************************
44 * sr32 - clear & set a value in a bit range for a 32 bit address
45 *****************************************************************/
46 void sr32(void *addr, u32 start_bit, u32 num_bits, u32 value)
47 {
48 u32 tmp, msk = 0;
49 msk = 1 << num_bits;

It makes no sense to initialize msk with 0 when you set another value
right in the next statement.

50 --msk;

I think the code would be clearer if you wrote:

msk = (1 << num_bits) - 1;

51 tmp = readl((u32)addr) & ~(msk << start_bit);
52 tmp |= value << start_bit;

Wouldn't it be better to make sure that "value" does not exceed the
bit range? Like that:

tmp |= (value & ~msk) << start_bit;

?

Also, I find code that is based on bit numbers, bit widths and values
*very* difficult to read. Do you really think that for example

sr32(&prcm_base->iclken1_core, 4, 1, 0x1);

is easier to read than

clrsetbits(u32, &prcm_base->iclken1_core, 0x10, 0x10);

?

And actually, just to set or clear values we don't even need that, a
plain

setbits(u32, &prcm_base->iclken1_core, 0x10);

would do as well.

[Look up the definitions in include/asm-ppc/io.h if needed].

And probably 0x10 could be even replaced by some MUSB_INTERFACE_CLK
#define from some header file?

I am aware that this is work for a future release, but I think it
would *greatly* improve the code. It would, most of all, make it
readable.

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
Space is big. You just won't believe how vastly, hugely, mind-
bogglingly big it is. I mean, you may think it's a long way down the
road to the drug store, but that's just peanuts to space.
-- The Hitchhiker's Guide to the Galaxy