[PATCH] Update udbg_progress() to display the integer

[PATCH] Update udbg_progress() to display the integer

[Timur Tabi]

Although udbg_progress() takes a string and a short as parameters, only
the string is displayed.  This patch also displays the integer, if it's not
equal to 0xFFFF.  This gives callers the option to display only the string.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 arch/powerpc/kernel/udbg.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/powerpc/kernel/udbg.c b/arch/powerpc/kernel/udbg.c
index 5730906..c474961 100644
--- a/arch/powerpc/kernel/udbg.c
+++ b/arch/powerpc/kernel/udbg.c
@@ -121,6 +121,11 @@ void udbg_printf(const char *fmt, ...)
 void __init udbg_progress(char *s, unsigned short hex)
 {
 	udbg_puts(s);
+	if (hex != 0xffff) {
+		char buf[8];
+		sprintf(buf, " : %04X", hex);
+		udbg_puts(buf);
+	}
 	udbg_puts("\n");
 }
 
-- 
1.4.4

Re: [PATCH] Update udbg_progress() to display the integer

[Olof Johansson]

On Mon, Feb 05, 2007 at 01:51:55PM -0600, Timur Tabi wrote:
> Although udbg_progress() takes a string and a short as parameters, only
> the string is displayed.  This patch also displays the integer, if it's not
> equal to 0xFFFF.  This gives callers the option to display only the string.

Some platforms do this already, but they output the numbers first. It's
a good idea to stay consistent with that.

See maple_progress, ps3_progress and friends for templates. They're
quite verbose though.


-Olof


> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
>  arch/powerpc/kernel/udbg.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/udbg.c b/arch/powerpc/kernel/udbg.c
> index 5730906..c474961 100644
> --- a/arch/powerpc/kernel/udbg.c
> +++ b/arch/powerpc/kernel/udbg.c
> @@ -121,6 +121,11 @@ void udbg_printf(const char *fmt, ...)
>  void __init udbg_progress(char *s, unsigned short hex)
>  {
>  	udbg_puts(s);
> +	if (hex != 0xffff) {
> +		char buf[8];
> +		sprintf(buf, " : %04X", hex);
> +		udbg_puts(buf);
> +	}
>  	udbg_puts("\n");
>  }
>  
> -- 
> 1.4.4
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] Update udbg_progress() to display the integer

[Timur Tabi]

Olof Johansson wrote:
> On Mon, Feb 05, 2007 at 01:51:55PM -0600, Timur Tabi wrote:
>> Although udbg_progress() takes a string and a short as parameters, only
>> the string is displayed.  This patch also displays the integer, if it's not
>> equal to 0xFFFF.  This gives callers the option to display only the string.
> 
> Some platforms do this already, but they output the numbers first. It's
> a good idea to stay consistent with that.
> 
> See maple_progress, ps3_progress and friends for templates. They're
> quite verbose though.

Actually, they're just one line each:

static void __init maple_progress(char *s, unsigned short hex)
{
	printk("*** %04x : %s\n", hex, s ? s : "");
}

static void __init ps3_progress(char *s, unsigned short hex)
{
	printk("*** %04x : %s\n", hex, s ? s : "");
}

The reason I can't use the same exact printk() is because of this code in 
function ppc_init():

	/* clear the progress line */
	if ( ppc_md.progress ) ppc_md.progress("             ", 0xffff);

If I could get rid of this line, then I wouldn't need to check for "hex == 
0xFFFF", but I don't know why ppc_init() thinks it needs to "clear the progress 
line".  Maybe this is for some kind of LED display.

I'll submit a new patch in a minute.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] Update udbg_progress() to display the integer

[Paul Mackerras]

Timur Tabi writes:

> The reason I can't use the same exact printk() is because of this code in 
> function ppc_init():
> 
> 	/* clear the progress line */
> 	if ( ppc_md.progress ) ppc_md.progress("             ", 0xffff);
> 
> If I could get rid of this line, then I wouldn't need to check for "hex == 
> 0xFFFF", but I don't know why ppc_init() thinks it needs to "clear the progress 
> line".  Maybe this is for some kind of LED display.

It would have been for the 2-line display on IBM RS/6000 machines.  If
that's still needed, we can put it in arch/powerpc/platforms/chrp
somewhere, so feel free to remove it from ppc_init().

Paul.

Re: [PATCH] Update udbg_progress() to display the integer

[Benjamin Herrenschmidt]

> It would have been for the 2-line display on IBM RS/6000 machines.  If
> that's still needed, we can put it in arch/powerpc/platforms/chrp
> somewhere, so feel free to remove it from ppc_init().

Do we actually care about this progress stuff ? I've been tempted more
than once to rip it all off... we have console access as early as we can
do progress output on most powerpc platforms nowadays.

If we really want some way of ack'ing that the kernel reached known
known steps with something different than a printk-type interface, then
we should probably have somewhere a list of well defined numeric
constants and use those, but then, I don't like magic numbers.

Ben.

Re: [PATCH] Update udbg_progress() to display the integer

[Kumar Gala]

On Feb 6, 2007, at 5:38 PM, Benjamin Herrenschmidt wrote:

>
>> It would have been for the 2-line display on IBM RS/6000  
>> machines.  If
>> that's still needed, we can put it in arch/powerpc/platforms/chrp
>> somewhere, so feel free to remove it from ppc_init().
>
> Do we actually care about this progress stuff ? I've been tempted more
> than once to rip it all off... we have console access as early as  
> we can
> do progress output on most powerpc platforms nowadays.
>
> If we really want some way of ack'ing that the kernel reached known
> known steps with something different than a printk-type interface,  
> then
> we should probably have somewhere a list of well defined numeric
> constants and use those, but then, I don't like magic numbers.

I'm with Ben on this.  I'm not aware of any in tree user that just  
doesn't print this stuff out to a console port.  The concept is nice,  
but does anyone really use it?

I know I end up using a JTAG and dumping the memory at logbuf before  
using an LED to figure out where I'm at.

- k

Re: [PATCH] Update udbg_progress() to display the integer

[Timur Tabi]

Kumar Gala wrote:

> I'm with Ben on this.  I'm not aware of any in tree user that just 
> doesn't print this stuff out to a console port.  The concept is nice, 
> but does anyone really use it?

I just ran a quick test on an 8349 board, and it appears that everywhere 
ppc_md.progress() is called, printk() works, too.

Is there any platform where ppc_md.progress() does something different than 
printk()?  Is there any point in the boot process where ppc_md.progress() works 
but printk() doesn't?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] Update udbg_progress() to display the integer

[Olof Johansson]

On Tue, Feb 06, 2007 at 05:54:06PM -0600, Timur Tabi wrote:
> Kumar Gala wrote:
> 
> >I'm with Ben on this.  I'm not aware of any in tree user that just 
> >doesn't print this stuff out to a console port.  The concept is nice, 
> >but does anyone really use it?
> 
> I just ran a quick test on an 8349 board, and it appears that everywhere 
> ppc_md.progress() is called, printk() works, too.
> 
> Is there any platform where ppc_md.progress() does something different than 
> printk()?  Is there any point in the boot process where ppc_md.progress() 
> works but printk() doesn't?

On some IBM machines the progress will also print the status on the hardware
maintenance console's status line for the partition/machine in question.


-Olof

Re: [PATCH] Update udbg_progress() to display the integer

[Timur Tabi]

Olof Johansson wrote:

> On some IBM machines the progress will also print the status on the hardware
> maintenance console's status line for the partition/machine in question.

Ah, yes, the HMC.

Looking at the code, there doesn't appear to be much rhyme or reason as to when 
ppc_md.progress() is called, but it is called from quite a few files.

Is there any reason why we can't just re-route printk() to HMC?

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] Update udbg_progress() to display the integer

[Linas Vepstas]

On Tue, Feb 06, 2007 at 06:04:11PM -0600, Timur Tabi wrote:
> Olof Johansson wrote:
> 
> > On some IBM machines the progress will also print the status on the hardware
> > maintenance console's status line for the partition/machine in question.
> 
> Ah, yes, the HMC.
> 
> Looking at the code, there doesn't appear to be much rhyme or reason as to when 
> ppc_md.progress() is called, but it is called from quite a few files.
> 
> Is there any reason why we can't just re-route printk() to HMC?

Yes.

The HMC progress display is the "same as" the LED's 
i.e. one line of 30 or 40 chars. Its enough to tell you 
the kernel version you've booted, or if the machine is
stuck in firmware. but that's about it. 

--linas

Re: [PATCH] Update udbg_progress() to display the integer

[Timur Tabi]

Linas Vepstas wrote:

> The HMC progress display is the "same as" the LED's 
> i.e. one line of 30 or 40 chars. Its enough to tell you 
> the kernel version you've booted, or if the machine is
> stuck in firmware. but that's about it. 

In that case, it looks like we need to keep pcc_md.progress(), but I still think 
it should be restricted to platforms where it outputs to a device other than the 
device that printk() is outputting to.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] Update udbg_progress() to display the integer

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 479 bytes --]

On Tue, 6 Feb 2007 17:41:24 -0600 Kumar Gala <galak@kernel.crashing.org> wrote:
>
>
> I'm with Ben on this.  I'm not aware of any in tree user that just
> doesn't print this stuff out to a console port.  The concept is nice,
> but does anyone really use it?

Legacy iSeries prints it *and* send it to the HV which displays it as an
SRC value in its various monitoring screen.

--
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Update udbg_progress() to display the integer

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 701 bytes --]

On Wed, 7 Feb 2007 11:00:22 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> On Tue, 6 Feb 2007 17:41:24 -0600 Kumar Gala <galak@kernel.crashing.org> wrote:
> >
> > I'm with Ben on this.  I'm not aware of any in tree user that just
> > doesn't print this stuff out to a console port.  The concept is nice,
> > but does anyone really use it?
>
> Legacy iSeries prints it *and* send it to the HV which displays it as an
> SRC value in its various monitoring screen.

And if the RTAS set-indicator call is available, then it is called on
machines with RTAS.  I have no idea if that is useful.

--
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Update udbg_progress() to display the integer

[Timur Tabi]

Benjamin Herrenschmidt wrote:
>> It would have been for the 2-line display on IBM RS/6000 machines.  If
>> that's still needed, we can put it in arch/powerpc/platforms/chrp
>> somewhere, so feel free to remove it from ppc_init().
> 
> Do we actually care about this progress stuff ? 

I think support for LED progress displays is a nice feature to have.

The only reason I noticed it is because on Freescale boards, it's the only way 
to get printk-like functionality early in the boot process.

> I've been tempted more
> than once to rip it all off... we have console access as early as we can
> do progress output on most powerpc platforms nowadays.

Is that what udbg_progress() uses?  I'm not really familiar with the early 
console stuff.

> If we really want some way of ack'ing that the kernel reached known
> known steps with something different than a printk-type interface, then
> we should probably have somewhere a list of well defined numeric
> constants and use those, but then, I don't like magic numbers.

Me neither.  I think on systems like RS6000, the progress codes have already 
been defined, so I don't think we can redefine them.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] Update udbg_progress() to display the integer

[Mike Strosaker]

Timur Tabi wrote:
> Benjamin Herrenschmidt wrote:
>>If we really want some way of ack'ing that the kernel reached known
>>known steps with something different than a printk-type interface, then
>>we should probably have somewhere a list of well defined numeric
>>constants and use those, but then, I don't like magic numbers.
> 
> 
> Me neither.  I think on systems like RS6000, the progress codes have already 
> been defined, so I don't think we can redefine them.
> 

I think that the LCD panel (op panel, in IBM's parlance) still has its 
uses.  As alluded to by others, the displayed messages are stored by the 
service processor, and a history of messages (the past 200 or so, I 
think) are thus visible from service processor and HMC interfaces. 
Should an error occur when a console session was not attached, these op 
panel codes can sometimes provide enough information to resolve the 
problem without needing to reproduce the error with a console 
attached... maybe not for kernel developers, but possibly for users.

The op panel on recent systems has 2 lines; the first can display 16 
characters, the second, 80.  The first line is usually used to display 
an 8 character hexadecimal progress/error message (called an SRC: System 
Reference Code), and the second line is used to display a location code 
when appropriate (e.g. when the SRC indicates a device failure).  IBM 
documents many of their OF and RTAS SRCs deep in the Hardware 
Information Center:

Error codes: 
http://publib.boulder.ibm.com/infocenter/eserver/v1r3s/index.jsp?topic=/ipha6/refcodelist.htm
Progress codes: 
http://publib.boulder.ibm.com/infocenter/eserver/v1r3s/index.jsp?topic=/ipha6/progcodesmain.htm

I've found some of theses codes useful in the past.  For example, 
CA00E1AE indicates that a partition is waiting for user input at the SMS 
menu.  An analogous SRC to indicate that it is waiting for user input at 
the yaboot prompt might be useful.

IBM reserved these SRC ranges for the exclusive use of Linux; no one 
else (AIX, RTAS, i5/OS, etc.) uses codes in these ranges:

AFxxxxxx - attention codes
BFxxxxxx - error codes
CFxxxxxx - progress codes
DFxxxxxx - dump progress codes

Yeah, magic numbers suck, but the op panel does have some physical 
constraints that prevent the printing of verbose messages.  These codes 
could be mapped to useful messages in a file in Documentation/powerpc, 
for example.

- Mike

Re: [PATCH] Update udbg_progress() to display the integer

[Timur Tabi]

Mike Strosaker wrote:

> The op panel on recent systems has 2 lines; the first can display 16 
> characters, the second, 80.  The first line is usually used to display 
> an 8 character hexadecimal progress/error message (called an SRC: System 
> Reference Code), and the second line is used to display a location code 
> when appropriate (e.g. when the SRC indicates a device failure).  IBM 
> documents many of their OF and RTAS SRCs deep in the Hardware 
> Information Center:

In that case, the big question is: does the kernel conform to this standard? 
Looking at the current usage of ppc_md.progress(), I can't help but think that 
it being called the same way printk() is being called, i.e. at the whim of the 
developer who wrote the code.

So let's say that we need to keep ppc_md.progress().  I think we should have 
some way of restricting its usage to systems where it does something different 
than printk().  On an Freescale 83xx reference board, for example, its output 
goes to the same place as printk(), so it doesn't serve any purpose.

Perhaps we should define ppc_md.progress() such that it never sends the output 
to the same place as printk().  If the only output device is the serial port, 
and printk() already outputs there, then ppc_md.progress() should do nothing. 
This would eliminate any "accidental" use of ppc_md.progress(), when printk() is 
the better choice.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: [PATCH] Update udbg_progress() to display the integer

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2088 bytes --]

On Wed, 2007-02-07 at 10:28 -0600, Timur Tabi wrote:
> Mike Strosaker wrote:
> 
> > The op panel on recent systems has 2 lines; the first can display 16 
> > characters, the second, 80.  The first line is usually used to display 
> > an 8 character hexadecimal progress/error message (called an SRC: System 
> > Reference Code), and the second line is used to display a location code 
> > when appropriate (e.g. when the SRC indicates a device failure).  IBM 
> > documents many of their OF and RTAS SRCs deep in the Hardware 
> > Information Center:
> 
> In that case, the big question is: does the kernel conform to this standard? 
> Looking at the current usage of ppc_md.progress(), I can't help but think that 
> it being called the same way printk() is being called, i.e. at the whim of the 
> developer who wrote the code.
> 
> So let's say that we need to keep ppc_md.progress().  I think we should have 
> some way of restricting its usage to systems where it does something different 
> than printk().  On an Freescale 83xx reference board, for example, its output 
> goes to the same place as printk(), so it doesn't serve any purpose.
> 
> Perhaps we should define ppc_md.progress() such that it never sends the output 
> to the same place as printk().  If the only output device is the serial port, 
> and printk() already outputs there, then ppc_md.progress() should do nothing. 
> This would eliminate any "accidental" use of ppc_md.progress(), when printk() is 
> the better choice.

But then you'll end up with code that does:

 printk("Setup done");
 ppc_md.progress("Setup done");

Because on systems where the output _does_ go to two places, you'll want
the messages to appear in both.

I don't see what the problem is with progress messages going to the
printk buffer?

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Update udbg_progress() to display the integer

[Timur Tabi]

Michael Ellerman wrote:

>> Perhaps we should define ppc_md.progress() such that it never sends the output 
>> to the same place as printk().  If the only output device is the serial port, 
>> and printk() already outputs there, then ppc_md.progress() should do nothing. 
>> This would eliminate any "accidental" use of ppc_md.progress(), when printk() is 
>> the better choice.
> 
> But then you'll end up with code that does:
> 
>  printk("Setup done");
>  ppc_md.progress("Setup done");

Technically that's true, but I think this will be rare.  The printk() 
messages are supposed to be more verbose than the progress() messages.

Besides, progress() always takes two parameters - a string and a number. 
  So it's not exactly a clone of printk().

> Because on systems where the output _does_ go to two places, you'll want
> the messages to appear in both.
> 
> I don't see what the problem is with progress messages going to the
> printk buffer?

The problem is that currently, a lot of code uses ppc_md.progress() when 
it should use printk() instead.  If progress() is intended for 2-len LED 
displays, then it's not really a substitute for printk().  progress() is 
supposed to display specialized messages, so I think it makes sense for 
it to be treated like a specialized function.

Re: [PATCH] Update udbg_progress() to display the integer

[Benjamin Herrenschmidt]

> Yeah, magic numbers suck, but the op panel does have some physical 
> constraints that prevent the printing of verbose messages.  These codes 
> could be mapped to useful messages in a file in Documentation/powerpc, 
> for example.

Or at least a .h with well defined constants instead of the current
mess :-)

Ben.