[coreboot] [PATCH EHCI Debug Port setup for all AMD SB600/SB700 boards

[Uwe Hermann]

On Thu, Sep 23, 2010 at 11:39:58PM +0200, Peter Stuge wrote:
> Uwe Hermann wrote:
> > I also cleaned up the Debug Port page in the wiki a bit.
> 
> Thanks for that! A link to #55 in Trac could be added, that's a
> libusb program to read the device, instead of the usb_debug module.

Will do, thanks.


> > +++ src/southbridge/amd/sb600/sb600_enable_usbdebug.c	(Arbeitskopie)
> ..
> > +/* Required for successful build, but currently empty. */
> > +void set_debug_port(unsigned int port)
> > +{
> > +}
> 
> Maybe make that a weak function in common code instead? Is there
> actually any instance of the function which is not empty?

Maybe, or maybe eliminate it all-together, I haven't looked at the code
in detail.

There is one implementation where it's non-empty, the MCP55 (which I
cannot check for correctness against, of course, as there's no public
datasheet). But I assume the MCP55 code to be correct (and I guess it
might also work for CK804, will try that and report back).

On MCP55 it seems that it's configurable on which physical USB port
the Debug Port is located, thus a set_debug_port() is required.
On ICH7 (probably all ICH*) this is not the case, Debug Port is always on
physical USB port 1 (hardcoded as per datasheet).
I _think_ the same might be true for SB600/SB700, the datasheet says the
default is "1" (port 1), but they don't clearly say whether a board
designer or firmware can change that register bit.


> > +++ src/mainboard/asrock/939a785gmh/romstage.c	(Arbeitskopie)
> ..
> > +#if CONFIG_USBDEBUG
> > +#include "southbridge/amd/sb700/sb700_enable_usbdebug.c"
> > +#include "pc80/usbdebug_serial.c"
> > +#endif
> 
> Can this go somewhere outside the mainboard directory?

Maybe, yes, but that's probably a follow-up patch.


> If anything does need to go in the mainboard dir now, then would it
> be enough to add pc80/usbdebug_serial.c, and without #if? The #if
> could easily be moved into that file to save repetitive lines. And
> I'm thinking that maybe sb700_enable_usbdebug.c could be pulled in
> when both SOUTHBRIDGE_AMD_SB700 and CONFIG_USBDEBUG are selected?

I agree, that would be nice (if it can be done).


> > +++ src/mainboard/gigabyte/ma78gm/romstage.c	(Arbeitskopie)
> ..
> > @@ -139,6 +145,12 @@
> >  	it8718f_disable_reboot();
> >  	uart_init();
> >  	console_init();
> > +
> > +#if CONFIG_USBDEBUG
> > +	sb700_enable_usbdebug(0);
> > +	early_usbdebug_init();
> > +#endif
> > +
> >  	printk(BIOS_DEBUG, "\n");
> 
> Here console_init() comes before early_usbdebug_init(), that's
> different from above. Does it matter?

Don't know, that was an error on my side, I intended all such blocks to
be in the same place. I don't really know how early or late the
functions need to or should be called, It just sounded like it'd make
sense to do it before console_init(), but maybe there's a better place.


I know the Debug Port stuff can use more work, I'll try to fix further
issues in follow-up patches, but IMHO this one can go in as it improves
the situation (multiple boards with Debug Port debug in coreboot vs. none).

 
Uwe.
-- 
http://hermann-uwe.de     | http://sigrok.org
http://randomprojects.org | http://unmaintained-free-software.org