On Friday 04 May 2012 18:21:31 Timur Tabi wrote:

Introduce board_start_saveenv() and board_finish_saveenv(), two "weak" functions that are called before and after saving the environment. This allows for board-specific functions that "prepare" the board for saving the environment. This is useful if, for some reason, the non-volatile storage is normally unavailable (e.g. blocked via a mux).

all these board hooks are paper-cutting us to death with unused bloat

--- a/common/cmd_nvedit.c +++ b/common/cmd_nvedit.c

#if defined(CONFIG_CMD_SAVEENV) && !defined(CONFIG_ENV_IS_NOWHERE)

+static int __board_start_saveenv(void) +{

• return 0;

+} +int board_start_saveenv(void) __attribute__((weak, alias("__board_start_saveenv"))); + +static void __board_finish_saveenv(void) +{ +} +void board_finish_saveenv(void) __attribute__((weak, alias("__board_finish_saveenv"))); + int do_env_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) {

• int ret;
• printf("Saving Environment to %s...\n", env_name_spec);

• return saveenv() ? 1 : 0;

• ret = board_start_saveenv();
• if (ret)

• return 0;
  

• ret = saveenv() ? 1 : 0;
• board_finish_saveenv();
• return ret;

}

this is less bloat: int board_start_saveenv(void) __attribute__((weak, alias("saveenv")));

int do_env_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { printf("Saving Environment to %s...\n", env_name_spec); return board_saveenv() ? 1 : 0; }

On Monday 14 May 2012 12:10:48 Timur Tabi wrote:

Mike Frysinger wrote:

...

this is less bloat: int board_start_saveenv(void) __attribute__((weak, alias("saveenv")));

int do_env_save(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) {

printf("Saving Environment to %s...\n", env_name_spec); return board_saveenv() ? 1 : 0;

}

Ah, I see. This forces the board-specific function to call saveenv(). That gives us more flexibility in the board code.

However, I was trying to mimic what we have in the NAND layer, with nand_get_device() and nand_release_device(). That is, before we save the environment, we have to "get" it, and then after we save it, we can "release" it.

i don't think the nand approach is terribly applicable. different solutions for different cases.

Your approach, although it eliminates two weak functions, is not as "architecturally clean" as mine, IMHO.

i'd prefer my approach because it keeps down the bloat for the vast majority of people out there while not restricting boards who want this override -mike

Wolfgang Denk wrote:

I think whoever told you this was right. Let it break.

Come on, Wolfgang. That's not acceptable.

We cannot add pre- and post-hooks all ever the place for brain-dead designs that need to do this and that before and after doing perfectly things.

Well, I already have code in U-boot that does this. If you look at board/freescale/p1022ds/diu.c, you'll see that I override each of the NOR flash accessors. This is horribly inefficient, but it works. Unfortunately, it only covers NOR flash. The new design covers NOR and NAND.

The last two patches of this patchset are a vast improvement, but they require a board hook (and using Mike's idea, only one hook is necessary, not two).

As for 'all over the place", I think it's unfair to say my one board hook function is going to result in "all over the place" hacks.

It makes no sense adding this to saveenv, because there will be othe rplaces in the code that need to to the same - like if it's NAND flash, you will probabaly need to do the same for all NAND related commands.

Actually, the same code works for saving the environment to NAND flash. This is how the board code will look:

/* * While the DIU is active, the localbus is not available. Therefore, in * order to support the saveenv command to localbus devices, we need to * temporarily disable the DIU and enable the localbus. To do this, we * provide our own implementation of board_saveenv(). This function is called * by do_env_save(). */ #if defined(CONFIG_ENV_IS_IN_NAND) || defined(CONFIG_ENV_IS_IN_FLASH) int board_saveenv(void) { int switched, ret;

switched = set_mux_to_lbc();

ret = saveenv();

if (switched) set_mux_to_diu();

return ret; } #endif

cmd_nvedit.c is definitely the wrong place for this.

If you have a better idea, then I'm all ears. I could implement my own version of saveenv(), but the current design of U-boot does not allow me to have two functions called saveenv().

Dear Timur,

In message 4FB57D2D.3090902@freescale.com you wrote:

...

I think whoever told you this was right. Let it break.

Come on, Wolfgang. That's not acceptable.

Why not? But adding arbitrary complexity and ugliness into U-Boot mainline is acceptable? Why?

...

We cannot add pre- and post-hooks all ever the place for brain-dead designs that need to do this and that before and after doing perfectly things.

Well, I already have code in U-boot that does this. If you look at board/freescale/p1022ds/diu.c, you'll see that I override each of the NOR flash accessors. This is horribly inefficient, but it works.

This is already streching the code to the limits, and the only reason you ever got this thrugh is that it's FSL specific code, you you have to deal yourself with this ugliness. I don;t think you will find good arguments to convince me adding such stuff into common code, though.

Unfortunately, it only covers NOR flash. The new design covers NOR and NAND.

Well, your code does not really cover NOR flash. I think it covers only a small subset of the use cases, but horribly fails else.

For example, what happens when I just use "md" or "itest *addr" or similar trying to read NOR flash while the display is on?

You _do_ have a broken hardware design, and if you cannot access NOR flash with display running, and vice versa, than just accept this fact.

Feel free to provide commands to switch mode, but don't lard the code with hooks here and there trying to fix what cannot be fixed.

The last two patches of this patchset are a vast improvement, but they require a board hook (and using Mike's idea, only one hook is necessary, not two).

As for 'all over the place", I think it's unfair to say my one board hook function is going to result in "all over the place" hacks.

Well, you try again to fix just a single use-case, leaving all the others unsolved. It makes zero sense to fix "saveenv" while not fixing all other access modes to the same storage device.

Yes, it is a pain if you have to run something like

=> busmux flash;saveenv;busmux diu

but the ugliness comes from the hardware design, so we have no reasoin to camouflage it. Let people see what is going on under the hood - if this is done in a clean way, you don't have to be ashamed.

...

It makes no sense adding this to saveenv, because there will be othe rplaces in the code that need to to the same - like if it's NAND flash, you will probabaly need to do the same for all NAND related commands.

Actually, the same code works for saving the environment to NAND flash. This is how the board code will look:

But this again works only for thesingle use case of the "saveenv" command.

This makes no sense - it's still broken, and trying to fix it using this approach does not scale and/or does not work.

If you have a better idea, then I'm all ears. I could implement my own version of saveenv(), but the current design of U-boot does not allow me to have two functions called saveenv().

Your problem is not just "saveenv". Your problem is that you need to switch modes. So provide a command to switch modes, and use this.

Yes, this is a pain, and it is less from perfect. But you cannot have a perfect solution on broken hardware. It makes no sense to waste lots of efforts on this. If you're ugly, you are still ugly even if wearing expensive clothes.

Best regards,

Wolfgang Denk

On 05/18/2012 05:58 PM, Timur Tabi wrote:

Is there a way to switch the console

setenv stdout serial

works for me..

Regards, Jeroen

Dear Timur,

In message 4FB67191.4030000@freescale.com you wrote:

...

For example, what happens when I just use "md" or "itest *addr" or similar trying to read NOR flash while the display is on?

The reason I make a big deal about saveenv is because I use an environment variable ("video-mode") to enable video support. Once the console is switched to the video display, the only way to switch it back to the serial port is to delete the environment variable, save the environment, and reboot.

Please try to look over the rim of your plate. Even if you make "saveenv" work with some magic, your end users may have a zillion other ideas why they want to access the flash - for example, to store a copy of the environment created using "env export", or to reload such a saved profile using "env import". Why should their needs be ignored?

...

Feel free to provide commands to switch mode, but don't lard the code with hooks here and there trying to fix what cannot be fixed.

Is there a way to switch the console

Sure. Delete the env var, switch mode to flash access, run saveenv, run reset.

Best regards,

Wolfgang Denk

On 05/17/2012 05:18 PM, Wolfgang Denk wrote:

Dear Timur Tabi,

In message 4FB12E88.1050906@freescale.com you wrote:

...

...

all these board hooks are paper-cutting us to death with unused bloat

I know, and I don't like it either. I hate how our hardware designers are always breaking the "rules", forcing us software developers to hack up our software more and more. The muxing on this chip is a like a cruel joke being played on me. I've even been told that I'm trying too hard to make it work.

I think whoever told you this was right. Let it break.

We cannot add pre- and post-hooks all ever the place for brain-dead designs that need to do this and that before and after doing perfectly things.

It makes no sense adding this to saveenv, because there will be othe rplaces in the code that need to to the same - like if it's NAND flash, you will probabaly need to do the same for all NAND related commands.

NAND doesn't need it because NAND goes through an API rather than direct memory-mapped access, and has more coarse-grained operations. NAND should be able to take care of this entirely in the driver using the select_chip() callback.

Timur, is there any reason to use NOR rather than NAND with this chip?

-Scott

On 05/17/2012 05:53 PM, Timur Tabi wrote:

Scott Wood wrote:

...

NAND doesn't need it because NAND goes through an API rather than direct memory-mapped access, and has more coarse-grained operations. NAND should be able to take care of this entirely in the driver using the select_chip() callback.

Fair enough. How do I enable that feature? Do I create my own board_nand_init() and then do this:

this->select_chip = p1022ds_nand_select_chip;

board_nand_init() is implemented in drivers/mtd/nand/fsl_elbc_nand.c. You'll need to add some board hook there.

...

Timur, is there any reason to use NOR rather than NAND with this chip?

Well, as with most of our boards, NOR is the default configuration. Also, there's no NAND support upstream yet.

What isn't upstream, besides the muxing hack? Does it need the 4K page hack?

-Scott

On 05/17/2012 09:21 PM, Tabi Timur-B04825 wrote:

Scott Wood wrote:

...

...

...

Well, as with most of our boards, NOR is the default configuration. Also, there's no NAND support upstream yet.

What isn't upstream, besides the muxing hack? Does it need the 4K page hack?

There's no NAND support at all.

Of course there's NAND support. I was asking what, besides the mux, makes that existing support not work on this board.

However, I just tried the two SDK patches that add it, and they apply cleanly, so that's an easy fix.

Which patches are those?

-Scott

On 05/18/2012 11:00 AM, Timur Tabi wrote:

Scott Wood wrote:

...

...

There's no NAND support at all.

Of course there's NAND support. I was asking what, besides the mux, makes that existing support not work on this board.

No, there is no NAND support for the P1022DS upstream.

[b04825@efes u-boot.0]$ grep -i nand include/configs/P1022DS.h [b04825@efes u-boot.0]$

That's the equivalent of saying Linux doesn't support something because nobody bothered to enable it in a certain defconfig.

...

...

...

However, I just tried the two SDK patches that add it, and they apply cleanly, so that's an easy fix.

Which patches are those?

powerpc/85xx: add SPI and SD builds for P1022DS
powerpc/p1022ds: Add support for NAND and NAND boot

I'm not sure what SPI and SD have to do with it...

Most of the latter patch is concerned with NAND boot, which is a different issue from NAND support, but still pretty important if you're going NAND-only. It looks like the patches were actually initially separate, but Kumar oh-so-helpfully squashed them together.

One thing I would like to see fixed in at upstream version of p1022ds NAND boot support is for it to use SPD like a normal p1022ds boot. This will likely require reviving the three-stage boot discussion (TPL).

-Scott

On 05/18/2012 11:17 AM, Timur Tabi wrote:

Scott Wood wrote:

...

That's the equivalent of saying Linux doesn't support something because nobody bothered to enable it in a certain defconfig.

Well, that's exactly what I meant. When you boot an upstream U-boot on a P1022DS, there is no support for NAND chips. The 'nand' command does not exist. You cannot build a u-boot.bin that will boot from NAND. That pretty much means "there is no NAND support".

I was under the impression that this was a developer discussion about what it would take to get this working, not an end user support forum.

...

One thing I would like to see fixed in at upstream version of p1022ds NAND boot support is for it to use SPD like a normal p1022ds boot. This will likely require reviving the three-stage boot discussion (TPL).

I just posted those two patches for upstream. I don't want the TPL work to hold up these patches.

It was over a year ago that I made that request internally. And still the answer is "I need this now now now!".

NACK any non-SPD NAND boot for a board that otherwise uses SPD, particularly if it has socketed RAM. It's not as if we don't know how to make this work.

Wolfgang will also probably object to adding another board to the old nand_spl infrastrucutre.

-Scott

On 05/18/2012 12:08 PM, Timur Tabi wrote:

Scott Wood wrote:

...

It was over a year ago that I made that request internally. And still the answer is "I need this now now now!".

Who's going to do that work, if not you?

Matthew had something working a while ago, I thought.

...

NACK any non-SPD NAND boot for a board that otherwise uses SPD, particularly if it has socketed RAM. It's not as if we don't know how to make this work.

Well, *I* don't know how to do that.

Use a three stage boot where the middle stage runs out of L2 SRAM.

-Scott

Dear Scott,

In message 4FB678CF.4030501@freescale.com you wrote:

Wolfgang will also probably object to adding another board to the old nand_spl infrastrucutre.

True. This gives a NAK reliably.

Best regards,

Wolfgang Denk