| Message ID | 1311581699-6301-1-git-send-email-wd@denx.de |
|---|---|
| State | Rejected, archived |
| Delegated to: | Andy Fleming |
| Headers | show |
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
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
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
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
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
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
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
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
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); } }
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(-)