ron minnich wrote: > On Wed, Jun 4, 2008 at 9:46 AM, Marc Jones <[EMAIL PROTECTED]> wrote: >> ron minnich wrote: >>> This change adds some debug prints, and a comment warning to dts on cs5536. >>> >>> Most importantly it fixes a simple programming error which made it so most >>> of >>> the sets on the USB were not doing anything. The bug is also in V2. >>> >> >>> /* the /sizeof(u32) is to convert byte offsets into u32 offsets */ >>> #define HCCPARAMS (0x08/sizeof(u32)) >> >> >> I don't understand this change. This is standard MMIO. The memory mapped >> registers are defined 0h, 04h, 08h, 0Ah... >> >> You could >> *(bar + 08h) |= 1 << 9; >> or >> *(bar + 09h) |= 1 << 1; >> > > > This is an old C gotcha. Offsets are scaled by the pointer size. bar > is a u32 *. So you see this: > *(bar + HCCPARAMS) > > And it's easy to think that it is this: > *(bar + 0xc) > > but actually, because bar is a u32 *, it will actually dereference this: > *(bar + 0xc*4) i.e. the sizeof a u32 ... > > It has to work this way, because when you do bar++, it increments bar > by 4, not 1. > > This is one of the reasons why, for mmio, we ought to be using structs. > > thanks > > ron >
Doh! I see. It should be a byte pointer and then use the readl() abd writel() etc from io.h. I'll post a patch for v2 in a few minutes. A struct for the entire device seems overkill for a couple of registers. Marc -- Marc Jones Senior Firmware Engineer (970) 226-9684 Office mailto:[EMAIL PROTECTED] http://www.amd.com/embeddedprocessors -- coreboot mailing list [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
