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

See patch.

I also cleaned up the Debug Port page in the wiki a bit.

  http://www.coreboot.org/EHCI_Debug_Port

For now I marked all chipsets where we have Debug Port code as untested,
I have a feeling the functionality was rarely tested recently (or at
all) so we should only mark those chipsets as tested where we actually
have recent test reports.

I'm not sure about the SiS966 Debug Port code, it looks copy-pasted from
MCP55 to me, not sure if the functionality actually exists on that
chipset and if the code is correct for that. Maybe someone with a SiS966
board could test this?

I'll be able to test on two more chipsets (MCP55, ICH7) and mark them
as tested in the wiki then, will report back. Can somebody test SB700?
I guess it works fine as the SB600 does too, but a test report would be
nice.


Uwe.

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.


> +++ 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?


> +++ 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?

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?


>  	sio_init();
>  	w83627dhg_enable_serial(SERIAL_DEV, CONFIG_TTYS0_BASE);
>  	uart_init();
> +
> +#if CONFIG_USBDEBUG
> +	sb700_enable_usbdebug(0);
> +	early_usbdebug_init();
> +#endif
> +
>  	console_init();

Same question. Maybe the call to sb700_enable_usbdebug() can go into
early_usbdebug_init() ?


> +++ 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?


//Peter

Peter Stuge 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.

Make that #57.


//Peter

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.

On 9/23/10 11:39 PM, Peter Stuge wrote:
>> +#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?
>
> 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?
>
We could do a lot of such or similar improvements if we started not
including code but adding it to an object list.

Who's gonna do it?

Stefan

On 9/23/10 11:09 PM, Uwe Hermann wrote:
> For now I marked all chipsets where we have Debug Port code as untested,
> I have a feeling the functionality was rarely tested recently (or at
> all) so we should only mark those chipsets as tested where we actually
> have recent test reports.
Last time I tested on i945 I was able to get output if I called the
debug port output function directly, but it seemed that you would not
get any output from printk as it simply was not hooked up. Has that
changed since then?

> Hook up all AMD SB600/SB700 boards to the EHCI Debug Port infrastructure.
>
> Without a (currently) dummy set_debug_port() function the build fails,
> this may or may not be fixed differently in the future.
>
> Manually build-tested on all SB600/SB700 boards, and tested on hardware on
> one SB600 board I own, works fine.
>
> Signed-off-by: Uwe Hermann <uwe@hermann-uwe.de>
>
Acked-by: Stefan Reinauer <stepan@coresystems.de>

Stefan Reinauer wrote:
> > I'm thinking that maybe sb700_enable_usbdebug.c could be pulled in
> > when both SOUTHBRIDGE_AMD_SB700 and CONFIG_USBDEBUG are selected?
> 
> We could do a lot of such or similar improvements if we started not
> including code but adding it to an object list.

Would it actually work? I guess it doesn't work with romcc, which I
think is still being used by some boards.


//Peter

On Fri, Sep 24, 2010 at 8:43 AM, Stefan Reinauer
<stefan.reinauer@coresystems.de> wrote:

> We could do a lot of such or similar improvements if we started not
> including code but adding it to an object list.
>
> Who's gonna do it?


First you have to stop using romcc, right? Or is this one of those
legacy cases where we included code even though it was gcc-based (that
happened once in a while if people got mixed up)

ron

On Fri, Sep 24, 2010 at 05:43:57PM +0200, Stefan Reinauer wrote:
>  On 9/23/10 11:09 PM, Uwe Hermann wrote:
> > For now I marked all chipsets where we have Debug Port code as untested,
> > I have a feeling the functionality was rarely tested recently (or at
> > all) so we should only mark those chipsets as tested where we actually
> > have recent test reports.
> Last time I tested on i945 I was able to get output if I called the
> debug port output function directly, but it seemed that you would not
> get any output from printk as it simply was not hooked up. Has that
> changed since then?

Hm, looks like it's hooked up in romstage.c on the Kontrol 986lcd-m at
the moment, yes. I'll test on another i945 board I have here.

 
> Acked-by: Stefan Reinauer <stepan@coresystems.de>

Thanks, r5833.

I fixed the order of the function calls to be the same in all romstage.c
files for now, further patches will appear soon I hope.

 
Uwe.

On 9/24/10 7:16 PM, ron minnich wrote:
> On Fri, Sep 24, 2010 at 8:43 AM, Stefan Reinauer
> <stefan.reinauer@coresystems.de> wrote:
>
>> We could do a lot of such or similar improvements if we started not
>> including code but adding it to an object list.
>>
>> Who's gonna do it?
> First you have to stop using romcc, right? Or is this one of those
> legacy cases where we included code even though it was gcc-based (that
> happened once in a while if people got mixed up)
Most (all new) boards are gcc based. Since this is a per mainboard/per
chipset thing anyways, we don't need to drop romcc from all boards in
order to fix this.

Alternatively, we could collect object lists for gcc based builds and
for romcc based builds we create a list of source files and include them
in an autogenerated file. Same effect, but much more readable. i.e.

initobj-y += pc80/serial.o
initobj-y += <mainboard>/romstage.o

would lead to a file "romstage_includes.c" containing

#include "src/pc80/serial.c"
#include "src/<mainboard>/romstage.c"
...

Then, in the next (or prior) step we can also collect source files
instead of objects (initsrc instead of initobj)
and it'll be even easier. Plus, it will allow GCC to do better
optimization, too (Especially for size even though it starts being less
of an issue than it used to be)

Stefan

Am 25.09.2010 12:24, schrieb Stefan Reinauer:
> Alternatively, we could collect object lists for gcc based builds and
> for romcc based builds we create a list of source files and include them
> in an autogenerated file. Same effect, but much more readable. i.e.
Brilliant plan!
It requires a switch over for the *obj* variables to source files -
should we do that for all of them as a first step?

Also, what should we call them? This is a brilliant opportunity to
rename them - we have:
initobj	(rename to romstage-src?)
obj	(rename to ramstage-src?)
driver	(rename to driver-src?)
smmobj	(rename to smm-src?)

After that is done and settled, the romcc part of the build system could
be adapted to work the way you lined out.

So avoid issues with incorrect #include-order, the files should, in
romcc mode, include their dependencies and guard against multiple
inclusions.

> and it'll be even easier. Plus, it will allow GCC to do better
> optimization, too (Especially for size even though it starts being less
> of an issue than it used to be)
This optimization could come afterwards - you're thinking of -combine,
right? That should also speed up the build on mingw (where process
creation is relatively slow)


Patrick

Patrick Georgi wrote:
> > Alternatively, we could collect object lists for gcc based builds and
> > for romcc based builds we create a list of source files and include them
> > in an autogenerated file. Same effect, but much more readable. i.e.
> Brilliant plan!

+1

> It requires a switch over for the *obj* variables to source files -
> should we do that for all of them as a first step?

Hm aren't the obj lists still needed for the link?


> Also, what should we call them? This is a brilliant opportunity to
> rename them - we have:
> initobj	(rename to romstage-src?)
> obj	(rename to ramstage-src?)
> driver	(rename to driver-src?)
> smmobj	(rename to smm-src?)

I like the suggested names. Big improvement!


//Peter

Am 25.09.2010 21:30, schrieb Peter Stuge:
> Hm aren't the obj lists still needed for the link?
We already derive sources from objects - that could be inverted.
The reason for moving over the tree:
We already mangle object names. When we add foo.o to initobjs, it isn't
actually foo.o that's built, but foo.initobj.o.

> I like the suggested names. Big improvement!
I'll take a shot at it.


Patrick

On Sat, Sep 25, 2010 at 1:30 PM, Peter Stuge <peter@stuge.se> wrote:
> Patrick Georgi wrote:
>> > Alternatively, we could collect object lists for gcc based builds and
>> > for romcc based builds we create a list of source files and include them
>> > in an autogenerated file. Same effect, but much more readable. i.e.
>> Brilliant plan!
>
> +1
>

+1 me too!

>> It requires a switch over for the *obj* variables to source files -
>> should we do that for all of them as a first step?
>
> Hm aren't the obj lists still needed for the link?
>
>
>> Also, what should we call them? This is a brilliant opportunity to
>> rename them - we have:
>> initobj       (rename to romstage-src?)
>> obj   (rename to ramstage-src?)
>> driver        (rename to driver-src?)
>> smmobj        (rename to smm-src?)
>
> I like the suggested names. Big improvement!

I think so too!

Marc