patch: fix USB ports on DBE62, and other cs5536-based platforms

[ron minnich]

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.

With this fix, the DBE62 USB ports all work!

If someone clones the fix to V2, it will also fix V2. Or, we can just convince
you to move forward to V3 :-)

Signed-off-by: Ronald G. Minnich

Also, please note, my remaining issue is to fix X11 on DBE62, then I
believe we are done. Has someone tested audio however?

patch: fix USB ports on DBE62, and other cs5536-based platforms

[Carl-Daniel Hailfinger]

On 04.06.2008 06:28, 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.
>
> With this fix, the DBE62 USB ports all work!
>
> If someone clones the fix to V2, it will also fix V2. Or, we can just convince
> you to move forward to V3 :-)
>
> Signed-off-by: Ronald G. Minnich

If you fix the problems outlined below, this is
Acked-by: Carl-Daniel Hailfinger
> Also, please note, my remaining issue is to fix X11 on DBE62, then I
> believe we are done. Has someone tested audio however?
> Index: southbridge/amd/cs5536/cs5536.c
>
> --- southbridge/amd/cs5536/cs5536.c	(revision 688)
> +++ southbridge/amd/cs5536/cs5536.c	(working copy)
> @@ -395,18 +395,19 @@
>    }
>  }
>
> -#define HCCPARAMS		0x08
> -#define IPREG04			0xA0
> +/* the /sizeof(unsigned long) is to convert byte offsets into u32 offsets */

sizeof(unsigned long) is u64 for 64bit architectures. I suggest either
sizeof(u32) or sizeof(int).
> +#define HCCPARAMS		(0x08/sizeof(unsigned long))
> +#define IPREG04			(0xA0/sizeof(unsigned long))

same here. The indentation looks strange as well.
> #define USB_HCCPW_SET		(1 << 1)
>  #define UOCCAP			0x00
>  #define APU_SET			(1 << 15)
> -#define UOCMUX			0x04
> +#define UOCMUX			(0x04/sizeof(unsigned long))

same here
> #define PMUX_HOST		0x02
>  #define PMUX_DEVICE		0x03
>  #define PUEN_SET		(1 << 2)
> -#define UDCDEVCTL		0x404
> +#define UDCDEVCTL		(0x404/sizeof(unsigned long))

same here
> #define UDC_SD_SET		(1 << 10)
> -#define UOCCTL			0x0C
> +#define UOCCTL			(0x0C/sizeof(unsigned long))

same here.
> #define PADEN_SET		(1 << 7)
>
> /**
> @@ -445,6 +446,8 @@
>    if (dev) {
>    	bar = (u32 *) pci_read_config32(dev, PCI_BASE_ADDRESS_0);
> printk(BIOS_DEBUG, "UOCMUX is %x\n", *(bar + UOCMUX));
>
> *(bar + UOCMUX) &= PUEN_SET;
>
> /* Host or Device? */
>
> @@ -463,6 +466,7 @@
>    		*(bar + UOCCAP) |= sb->pph;
>    	}
>    	printk(BIOS_DEBUG, "UOCCAP is %x\n", *(bar + UOCCAP));
> printk(BIOS_DEBUG, "UOCMUX is %x\n", *(bar + UOCMUX));
>
> }
>
> @@ -489,6 +493,8 @@
>    	}
>    }
> printk(BIOS_DEBUG, "UOCCTL is %x\n", *(bar + UOCCTL));
> /* Disable virtual PCI UDC and OTG headers.  The kernel never
> sees a header for this device.  It used to provide an OS
> visible device, but that was defeatured.  There are still
> Index: southbridge/amd/cs5536/dts
>
> --- southbridge/amd/cs5536/dts	(revision 688)
> +++ southbridge/amd/cs5536/dts	(working copy)
> @@ -66,6 +66,10 @@
>    pph = "0";
>    /* 0:off, xxxx:overcurrent setting, e.g. 0x3FEA.
>     * See CS5536 - Data Book (pages 380-381).
> * And don't just set this to "1". You have to set it 
> * to values that make sense for the register. Do not set this
> * for your mainboard unless you have made sure of the register
> * settings!
> */
>  enable_USBP4_overcurrent = "0";

Regards,
Carl-Daniel

[Peter Stuge]

On Wed, Jun 04, 2008 at 02:24:23PM +0200, Carl-Daniel Hailfinger wrote:
> > +++ southbridge/amd/cs5536/cs5536.c	(working copy)
> > @@ -395,18 +395,19 @@
> >    }
> >  }
> >
> > -#define HCCPARAMS		0x08
> > -#define IPREG04			0xA0
> > +/* the /sizeof(unsigned long) is to convert byte offsets into u32 offsets */
>
> sizeof(unsigned long) is u64 for 64bit architectures.

Note which file this is in. I doubt the 5536 will be used on a 64 bit
CPU anytime soon.
> I suggest either sizeof(u32) or sizeof(int).

Though I consider this only cosmetic, I agree that u32 is nicer.
Also since it is mentioned in the comment.
> The indentation looks strange as well.

Maybe because patch is being viewed and there are some odd tabs or
other spacing in the original file.

I found that applying the patch is the only way to reliably detect
whitespace problems.

Ron, if you feel like it, make those unsigned longs u32s, if not,
still:

Acked-by: Peter Stuge

[ron minnich]

Committed revision 689.

A little meta-note here. I am seeing, more frequently now than 20
years ago, this kind of style in drivers and bios code:

base = (u32 *)pci_read_config_long(whatever);
*(base + xyz) =

Somehow, in all the usage of pragmas, attributes, segment names, and
all the other gadgets that have been added to C in the last while,
we've completely lost track of one of the most important reasons to
use C in systems programming!

What's the right way to work on memory-mapped registers? Well, in this case:

struct usb_otg{
  u32 uoc;
  u32 uocmux;
  u32 _1; /* A Plan 9 trick, and  if it's good enough for the guys who
designed C,
                       it's good enough for me (this is a reserved register) */
  u32 uocctl;
};

and so on. So, just a note: don't forget about structs as templates
for memory mapped registers. That capability has been there and been
used since, oh, 1973 or so :-)

thanks

ron

patch: fix USB ports on DBE62, and other cs5536-based platforms

[Marc Jones]

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;

Can you please explain?

Marc

[ron minnich]

On Wed, Jun 4, 2008 at 9:46 AM, Marc Jones  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

patch: fix USB ports on DBE62, and other cs5536-based platforms

[Marc Jones]

ron minnich wrote:
> On Wed, Jun 4, 2008 at 9:46 AM, Marc Jones  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

[ron minnich]

On Wed, Jun 4, 2008 at 10:41 AM, Marc Jones  wrote:
> A struct for the entire device seems overkill for a couple of registers.

What can I say? In the early Unix days, and even now, it's how we've done it :-)

but anyway ... I'm not that worried about it. It's more interesting to
me that a once-common technique is no longer used that much.

thanks!

ron

patch: fix USB ports on DBE62, and other cs5536-based platforms

[Stefan Reinauer]

Marc Jones 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;

Yes, this looks much saner...

on i945 i am using a couple of defines for accessing MMIO registers (in
this case the MCHBAR registers)

#define MCHBAR8(x) *((volatile u8 *)(DEFAULT_MCHBAR + x))
#define MCHBAR16(x) *((volatile u16 *)(DEFAULT_MCHBAR + x))
#define MCHBAR32(x) *((volatile u32 *)(DEFAULT_MCHBAR + x))

so you can write

MCHBAR8(DCC) |= (1<<1);

for maximum readablility
> Can you please explain?

[ron minnich]

On Wed, Jun 4, 2008 at 10:32 AM, Stefan Reinauer  wrote:
> #define MCHBAR8(x) *((volatile u8 *)(DEFAULT_MCHBAR + x))
> #define MCHBAR16(x) *((volatile u16 *)(DEFAULT_MCHBAR + x))
> #define MCHBAR32(x) *((volatile u32 *)(DEFAULT_MCHBAR + x))
>
> so you can write
>
> MCHBAR8(DCC) |= (1<<1);
>
> for maximum readablility

yes but as I mentioned .... one of the very earliest uses of struct,
from the guy who designed C, was to avoid this kind of construct.
Register structures accessed via mmio? set up a struct.

thanks

ron

patch: fix USB ports on DBE62, and other cs5536-based platforms

[Stefan Reinauer]

ron minnich wrote:
> On Wed, Jun 4, 2008 at 10:32 AM, Stefan Reinauer  wrote:
> > #define MCHBAR8(x) *((volatile u8 *)(DEFAULT_MCHBAR + x))
> > #define MCHBAR16(x) *((volatile u16 *)(DEFAULT_MCHBAR + x))
> > #define MCHBAR32(x) *((volatile u32 *)(DEFAULT_MCHBAR + x))
> >
> > so you can write
> >
> > MCHBAR8(DCC) |= (1<<1);
> >
> > for maximum readablility
>
> yes but as I mentioned .... one of the very earliest uses of struct,
> from the guy who designed C, was to avoid this kind of construct.
> Register structures accessed via mmio? set up a struct.

The one disadvantage of structs for this kind of thing is that you have
no way to cleanly describe offsets except through padding bytes.
So you end up recalculating offsets of registers and numbers of bytes of
reserved1 - reserved100 arrays within your struct, one for every hole.
And when you add a register, you do the work again, and it's error
prone, a single byte off, and all your registers are wrong.

I'm not against using structs, in fact maybe we should just do it, but
they're not the optimal solution either. The guy who invented struct was
not a firmware developer.

[ron minnich]

On Wed, Jun 4, 2008 at 11:10 AM, Stefan Reinauer  wrote:
> I'm not against using structs, in fact maybe we should just do it, but
> they're not the optimal solution either. The guy who invented struct was
> not a firmware developer.

you gotta trust me on this, but, back then, writing an OS was not that
much different. In fact Unix V6 and coreboot are just about the same
size ...

ron

patch: fix USB ports on DBE62, and other cs5536-based platforms

[Stefan Reinauer]

ron minnich wrote:
> On Wed, Jun 4, 2008 at 11:10 AM, Stefan Reinauer  wrote:
> > I'm not against using structs, in fact maybe we should just do it, but
> > they're not the optimal solution either. The guy who invented struct was
> > not a firmware developer
>
> you gotta trust me on this, but, back then, writing an OS was not that
> much different. In fact Unix V6 and coreboot are just about the same
> size ...

So are you saying we should make this part of our coding guidelines?

I believe we really have not used this anywhere in v2 so far. Maybe we
should start doing this for v3?

How do we handle tables like the resource setup in the K8 code? It's
mostly int arrays of

{
   offset, andvalue, orvalue,
   offset, andvalue, orvalue,
   offset, andvalue, orvalue,
}

This kind of partial filling would have to be done in code completely

mystruct.member1 &= andvalue1;
mystruct.member1 |= orvalue1;
...

Or is there a better way?

[Jordan Crouse]

On 04/06/08 20:30 +0200, Stefan Reinauer wrote:
> ron minnich wrote:
> > On Wed, Jun 4, 2008 at 11:10 AM, Stefan Reinauer  wrote:
> > > I'm not against using structs, in fact maybe we should just do it, but
> > > they're not the optimal solution either. The guy who invented struct was
> > > not a firmware developer
> >
> > you gotta trust me on this, but, back then, writing an OS was not that
> > much different. In fact Unix V6 and coreboot are just about the same
> > size ...
>
> So are you saying we should make this part of our coding guidelines?

NAK, NAK, NAK, NAK.

Sometimes, there are good reasons for practices to fall out of 
use.  Using a struct for mmio is not in common usage today for a
variety of reasons (see Linux kernel, Xorg, various RTOSen both
closed and opened).

That said, we shouldn't pick our guidelines based on tradition or what
Linus happens to be doing today, we should pick them based on common sense
and what works for our project.

Common sense dictates to me that it is far easier to keep a datasheet
synced with a list of #defines then a structure.  Verifying that you
have the right offsets by counting member sizes in the structure is
a recipe for disaster.  Not all MMIO spaces are sequential - sometimes the
register offsets are bunched together on 0x100 or 0x1000 boundaries -
take the Geode LX display filter for example - the regular display
registers are from offsets 0x00 - 0x120, but the panel control registers
start at 0x400. There is no clean way to map that in a structure without
causing some WTF sirens to go off.

Let us allow everybody to use what they want and try not to legislate
ourselves too badly.  And for the love of crumbcake, please start using
void * (or failing that u8 *) for MMIO pointers!  :)

Jordan

[ron minnich]

On Wed, Jun 4, 2008 at 12:31 PM, Jordan Crouse  wrote:
> On 04/06/08 20:30 +0200, Stefan Reinauer wrote:
> > So are you saying we should make this part of our coding guidelines?
>
> NAK, NAK, NAK, NAK.

yes, Jordan is right on this point. It's not common practice any more,
and nobody has been using it, no need to make it a guideline.

ron