Patchwork [U-Boot] mpc8xxx: lc_common_dimm_params: make less verbose

login
register
mail settings
Submitter Wolfgang Denk
Date July 25, 2011, 8:14 a.m.
Message ID <1311581699-6301-1-git-send-email-wd@denx.de>
Download mbox | patch
Permalink /patch/106619/
State Rejected, archived
Delegated to: Andy Fleming
Headers show

Comments

Wolfgang Denk - July 25, 2011, 8:14 a.m.
lc_common_dimm_params.c was too verbose and corrupted the boot
message display like this:

	...
	DRAM:  Detected UDIMM M2U25664DS88C3G-6K
	DDR: 256 MiB (DDR1, 64-bit, CL=2, ECC off)
	...

Turn printf() into debug() so we het the expected output again:

	...
	DRAM:  256 MiB (DDR1, 64-bit, CL=2, ECC off)
	...

Signed-off-by: Wolfgang Denk <wd@denx.de>
Cc: Kumar Gala <galak@kernel.crashing.org>

---
 .../cpu/mpc8xxx/ddr/lc_common_dimm_params.c        |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
York Sun - July 28, 2011, 5:46 p.m.
On Mon, 2011-07-25 at 10:14 +0200, Wolfgang Denk wrote:
> lc_common_dimm_params.c was too verbose and corrupted the boot
> message display like this:
> 
> 	...
> 	DRAM:  Detected UDIMM M2U25664DS88C3G-6K
> 	DDR: 256 MiB (DDR1, 64-bit, CL=2, ECC off)
> 	...
> 
> Turn printf() into debug() so we het the expected output again:
> 
> 	...
> 	DRAM:  256 MiB (DDR1, 64-bit, CL=2, ECC off)
> 	...
> 
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> 
> ---
>  .../cpu/mpc8xxx/ddr/lc_common_dimm_params.c        |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/cpu/mpc8xxx/ddr/lc_common_dimm_params.c b/arch/powerpc/cpu/mpc8xxx/ddr/lc_common_dimm_params.c
> index 8132e68..e66cc05 100644
> --- a/arch/powerpc/cpu/mpc8xxx/ddr/lc_common_dimm_params.c
> +++ b/arch/powerpc/cpu/mpc8xxx/ddr/lc_common_dimm_params.c
> @@ -209,11 +209,11 @@ compute_lowest_common_dimm_parameters(const dimm_params_t *dimm_params,
>  		if (dimm_params[i].n_ranks) {
>  			if (dimm_params[i].registered_dimm) {
>  				temp1 = 1;
> -				printf("Detected RDIMM %s\n",
> +				debug("Detected RDIMM %s\n",
>  					dimm_params[i].mpart);
>  			} else {
>  				temp2 = 1;
> -				printf("Detected UDIMM %s\n",
> +				debug("Detected UDIMM %s\n",
>  					dimm_params[i].mpart);
>  			}
>  		}

NAK.

We need to log module part number for testing and verification,
especially when comparing with different parts. It is also helpful for
support, without asking customers to recompile to enable debugging.

York
Wolfgang Denk - July 28, 2011, 6:45 p.m.
Dear York Sun,

In message <1311875176.29459.14.camel@oslab-l1> you wrote:
>
> > -				printf("Detected RDIMM %s\n",
> > +				debug("Detected RDIMM %s\n",
> >  					dimm_params[i].mpart);
> >  			} else {
> >  				temp2 = 1;
> > -				printf("Detected UDIMM %s\n",
> > +				debug("Detected UDIMM %s\n",
> >  					dimm_params[i].mpart);
> >  			}
> >  		}
> 
> NAK.

NAK the NAK.

> We need to log module part number for testing and verification,

debug() is exactly what is intended to be used for testing.

I don't know what exactly you mean by "verification", but I guess that
covers it, too.

Keeping this as prontf() corrupts the format of the boot messages, and
adds to the boot time without real value.

> especially when comparing with different parts. It is also helpful for
> support, without asking customers to recompile to enable debugging.

Then feel free to add a custom command to display this specific
information, but keep it off the regular, unconditionally printed boot
messages.

Best regards,

Wolfgang Denk
York Sun - July 28, 2011, 7:02 p.m.
On Thu, 2011-07-28 at 20:45 +0200, Wolfgang Denk wrote:
> Dear York Sun,
> 
> In message <1311875176.29459.14.camel@oslab-l1> you wrote:
> >
> > > -				printf("Detected RDIMM %s\n",
> > > +				debug("Detected RDIMM %s\n",
> > >  					dimm_params[i].mpart);
> > >  			} else {
> > >  				temp2 = 1;
> > > -				printf("Detected UDIMM %s\n",
> > > +				debug("Detected UDIMM %s\n",
> > >  					dimm_params[i].mpart);
> > >  			}
> > >  		}
> > 
> > NAK.
> 
> NAK the NAK.
> 
> > We need to log module part number for testing and verification,
> 
> debug() is exactly what is intended to be used for testing.

I didn't mean testing the U-boot. I meant to test the board and overall
software. As we keep the test logs for later comparison, it is helpful
to keep the DDR module information.

> 
> I don't know what exactly you mean by "verification", but I guess that
> covers it, too.
> 
> Keeping this as prontf() corrupts the format of the boot messages, and
> adds to the boot time without real value.

I don't agree one line with the module information corrupts the boot
messages. Do we have fixed format? Am I missing something?

I didn't count the time but don't think the one line message will add
much to boot time. And it does have real value even you don't use it.

> 
> > especially when comparing with different parts. It is also helpful for
> > support, without asking customers to recompile to enable debugging.
> 
> Then feel free to add a custom command to display this specific
> information, but keep it off the regular, unconditionally printed boot
> messages.

That's another way to do it. But we don't have to add a new command if a
simple message does the job.

York
Wolfgang Denk - July 28, 2011, 7:11 p.m.
Dear York Sun,

In message <1311879751.29459.24.camel@oslab-l1> you wrote:
>
> I don't agree one line with the module information corrupts the boot
> messages. Do we have fixed format? Am I missing something?

Yes, we have a well defined format with clear indentation.  Just see
my commit message for what I mean.

> I didn't count the time but don't think the one line message will add
> much to boot time. And it does have real value even you don't use it.

No, it will not add much. But 100 characters at 115kbps is another 10
milliseconds wasted for nothing.

It may have value to you, but for 99.X % of the users it is completey
meaningless.

> > Then feel free to add a custom command to display this specific
> > information, but keep it off the regular, unconditionally printed boot
> > messages.
> 
> That's another way to do it. But we don't have to add a new command if a
> simple message does the job.

Guess you have to, as I will not accept adding too much verbosity in
the boot messages. On contrary, I'm planning for some more patches to
get rid of the uncontrolled rank growth that happened in the past.

Best regards,

Wolfgang Denk
Kumar Gala - July 29, 2011, 1:01 p.m.
On Jul 28, 2011, at 2:11 PM, Wolfgang Denk wrote:

> Dear York Sun,
> 
> In message <1311879751.29459.24.camel@oslab-l1> you wrote:
>> 
>> I don't agree one line with the module information corrupts the boot
>> messages. Do we have fixed format? Am I missing something?
> 
> Yes, we have a well defined format with clear indentation.  Just see
> my commit message for what I mean.
> 
>> I didn't count the time but don't think the one line message will add
>> much to boot time. And it does have real value even you don't use it.
> 
> No, it will not add much. But 100 characters at 115kbps is another 10
> milliseconds wasted for nothing.
> 
> It may have value to you, but for 99.X % of the users it is completey
> meaningless.

Its quite helpful for debug of customer issues.  Should we add a CONFIG_ option for this?

>>> Then feel free to add a custom command to display this specific
>>> information, but keep it off the regular, unconditionally printed boot
>>> messages.
>> 
>> That's another way to do it. But we don't have to add a new command if a
>> simple message does the job.
> 
> Guess you have to, as I will not accept adding too much verbosity in
> the boot messages. On contrary, I'm planning for some more patches to
> get rid of the uncontrolled rank growth that happened in the past.
> 
> Best regards,
> 
> Wolfgang Denk

- k
Wolfgang Denk - July 29, 2011, 8:02 p.m.
Dear Kumar,

In message <73174EA1-7CBE-4C0F-A426-08F3C0EB7890@kernel.crashing.org> you wrote:
> 
> > It may have value to you, but for 99.X % of the users it is completey
> > meaningless.
> 
> Its quite helpful for debug of customer issues.  Should we add a CONFIG_ =
> option for this?

I already stated my position:

> >>> Then feel free to add a custom command to display this specific
> >>> information, but keep it off the regular, unconditionally printed boot
> >>> messages.

I don't want to see this in the boot messages.  It can be added to a
command that is used to display such information upon user's request.

For example, detailled output about the memory configuration could be
added to the "bdinfo" command.


I also would like to see to get a lot of the clocks information get
out of the standard boot messages. We don;t need to print 5 or more
lines like on 85xx:

	Clock Configuration:
	       CPU0:833.333 MHz, 
	       CCB:333.333 MHz,
	       DDR:166.667 MHz (333.333 MT/s data rate), LBC:41.667 MHz
	CPM:   333.333 MHz

We have the (optional) "clocks" command that can be used to print
this if anybody is interested in this.

I guess you get the intention: reduce the boot messages to the
essential parts.

Viele Grüße,

Wolfgang Denk
Wolfgang Denk - Aug. 31, 2011, 8:37 p.m.
Dear Kumar,

In message <1311581699-6301-1-git-send-email-wd@denx.de> I wrote:
> lc_common_dimm_params.c was too verbose and corrupted the boot
> message display like this:
> 
> 	...
> 	DRAM:  Detected UDIMM M2U25664DS88C3G-6K
> 	DDR: 256 MiB (DDR1, 64-bit, CL=2, ECC off)
> 	...
> 
> Turn printf() into debug() so we het the expected output again:
> 
> 	...
> 	DRAM:  256 MiB (DDR1, 64-bit, CL=2, ECC off)
> 	...
> 
> Signed-off-by: Wolfgang Denk <wd@denx.de>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> 
> ---
>  .../cpu/mpc8xxx/ddr/lc_common_dimm_params.c        |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)

Ping?

Best regards,

Wolfgang Denk
Kumar Gala - Sept. 1, 2011, 4:29 a.m.
On Aug 31, 2011, at 3:37 PM, Wolfgang Denk wrote:

> Dear Kumar,
> 
> In message <1311581699-6301-1-git-send-email-wd@denx.de> I wrote:
>> lc_common_dimm_params.c was too verbose and corrupted the boot
>> message display like this:
>> 
>> 	...
>> 	DRAM:  Detected UDIMM M2U25664DS88C3G-6K
>> 	DDR: 256 MiB (DDR1, 64-bit, CL=2, ECC off)
>> 	...
>> 
>> Turn printf() into debug() so we het the expected output again:
>> 
>> 	...
>> 	DRAM:  256 MiB (DDR1, 64-bit, CL=2, ECC off)
>> 	...
>> 
>> Signed-off-by: Wolfgang Denk <wd@denx.de>
>> Cc: Kumar Gala <galak@kernel.crashing.org>
>> 
>> ---
>> .../cpu/mpc8xxx/ddr/lc_common_dimm_params.c        |    4 ++--
>> 1 files changed, 2 insertions(+), 2 deletions(-)
> 
> Ping?

Can we defer for now and I'll ask York to look at adding a meminfo command that move this and what board_add_ram_info() is doing to.  And reduce this down to just printing out size.

- k

Patch

diff --git a/arch/powerpc/cpu/mpc8xxx/ddr/lc_common_dimm_params.c b/arch/powerpc/cpu/mpc8xxx/ddr/lc_common_dimm_params.c
index 8132e68..e66cc05 100644
--- a/arch/powerpc/cpu/mpc8xxx/ddr/lc_common_dimm_params.c
+++ b/arch/powerpc/cpu/mpc8xxx/ddr/lc_common_dimm_params.c
@@ -209,11 +209,11 @@  compute_lowest_common_dimm_parameters(const dimm_params_t *dimm_params,
 		if (dimm_params[i].n_ranks) {
 			if (dimm_params[i].registered_dimm) {
 				temp1 = 1;
-				printf("Detected RDIMM %s\n",
+				debug("Detected RDIMM %s\n",
 					dimm_params[i].mpart);
 			} else {
 				temp2 = 1;
-				printf("Detected UDIMM %s\n",
+				debug("Detected UDIMM %s\n",
 					dimm_params[i].mpart);
 			}
 		}