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

[Uwe Hermann]

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.

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

[Peter Stuge]

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

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

[Peter Stuge]

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

[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.

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

[Stefan Reinauer]

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

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

[Peter Stuge]

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

[ron minnich]

On Fri, Sep 24, 2010 at 8:43 AM, Stefan Reinauer
 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

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

[Stefan Reinauer]

On 9/24/10 7:16 PM, ron minnich wrote:
> On Fri, Sep 24, 2010 at 8:43 AM, Stefan Reinauer
>  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

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

[Patrick Georgi]

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

Stop #include of source files

[Peter Stuge]

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

Stop #include of source files

[Patrick Georgi]

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

Stop #include of source files

[Marc Jones]

On Sat, Sep 25, 2010 at 1:30 PM, Peter Stuge  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

Stop #include of source files

[Patrick Georgi]

Am 25.09.2010 21:30, schrieb Peter Stuge:
> > 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!

Okay, that part is done and passed abuild and boot test on kontron/986lcd-m.
The build system now requires you to refer to source files in the
Makefile.incs, and uses the following variables:
- in Makefile.inc:
romstage-y	ramstage-y	driver-y	smm-y

- global source lists (with full paths):
romstage-srcs	ramstage-srcs	driver-srcs	smm-srcs

- global object lists (with full paths):
romstage-objs	ramstage-objs	driver-objs	smm-objs

object file names are mangled similarily: for foo.{c,S,asl,...} it's
foo.romstage.o	foo.ramstage.o	foo.driver.o	foo.smm.o

This does away with the somewhat unobvious behaviour of the build system
that required users to declare "uart8250.o", while "uart8250.initobj.o"
was built.
It also gives some more intuitive names for files.

Further improvements to naming and placement of files are possible, as
some things are still somewhat non-sensical - mostly, because it was
brought over from the old build system without much thought.

I attached a complete patch and three patches to apply on top of each
other, signifying various steps in the process, for easier review. I'd
want to commit them in one go to avoid having three significantly
different build system setups (before, intermediate, after).

20100929-1-rework-buildsystem: complete patch

01-rename-obj-variables.diff:
rename obj-y to ramstage-y, etc. but still let them list object files.
Rename object files from foo.initobj.o to foo.romstage.o etc. (except
ramstage files which are still just foo.o)

02-rename-ramstage-obj-files.diff:
rename foo.o (of ramstage variety) to foo.ramstage.o

03-sources-in-make.diff:
Use source files in Makefiles instead of object files. This also
significantly changes the Makefile.inc parser in the toplevel Makefile.

04-remove-acpi-in-epiam700.diff:
via/epia-m700 stubbed out ACPI. Just drop it completely.

With this in place, I'll work next at moving .c-#includes out of romcc
romstages by using romstage-srcs instead.

Signed-off-by: Patrick Georgi

Patrick

Stop #include of source files

[Stefan Reinauer]

On 9/29/10 11:25 PM, Patrick Georgi wrote:
> Am 25.09.2010 21:30, schrieb Peter Stuge:
> > > 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!
>
> Okay, that part is done and passed abuild and boot test on kontron/986lcd-m.
> The build system now requires you to refer to source files in the
> Makefile.incs, and uses the following variables:
> in Makefile.inc:
>
> romstage-y	ramstage-y	driver-y	smm-y
> global source lists (with full paths):
>
> romstage-srcs	ramstage-srcs	driver-srcs	smm-srcs
> global object lists (with full paths):
>
> romstage-objs	ramstage-objs	driver-objs	smm-objs
>
> object file names are mangled similarily: for foo.{c,S,asl,...} it's
> foo.romstage.o	foo.ramstage.o	foo.driver.o	foo.smm.o
>
> This does away with the somewhat unobvious behaviour of the build system
> that required users to declare "uart8250.o", while "uart8250.initobj.o"
> was built.
> It also gives some more intuitive names for files.
>
> Further improvements to naming and placement of files are possible, as
> some things are still somewhat non-sensical - mostly, because it was
> brought over from the old build system without much thought.
>
> I attached a complete patch and three patches to apply on top of each
> other, signifying various steps in the process, for easier review. I'd
> want to commit them in one go to avoid having three significantly
> different build system setups (before, intermediate, after).
>
> 20100929-1-rework-buildsystem: complete patch
>
> 01-rename-obj-variables.diff:
> rename obj-y to ramstage-y, etc. but still let them list object files.
> Rename object files from foo.initobj.o to foo.romstage.o etc. (except
> ramstage files which are still just foo.o)
>
> 02-rename-ramstage-obj-files.diff:
> rename foo.o (of ramstage variety) to foo.ramstage.o
>
> 03-sources-in-make.diff:
> Use source files in Makefiles instead of object files. This also
> significantly changes the Makefile.inc parser in the toplevel Makefile.
>
> 04-remove-acpi-in-epiam700.diff:
> via/epia-m700 stubbed out ACPI. Just drop it completely.
>
> With this in place, I'll work next at moving .c-#includes out of romcc
> romstages by using romstage-srcs instead.
>
> Signed-off-by: Patrick Georgi

All are
Acked-by: Stefan Reinauer

Stefan

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

[Stefan Reinauer]

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

Acked-by: Stefan Reinauer

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

[Uwe Hermann]

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

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.