SATA/IDE/etc... init

the SATA/IDE/etc... layers seem to go against the u-boot policy when it comes to initialization. in other words, if support for them is enabled, then they are unconditionally initialized in board code (see ide_init / sata_initialize in lib_*/board.c:board_init_r). am i missing something or should these init functions be delayed until the sata/ide/whatever functions need them ? or

should they be made explicit like some commands where you first have to do "sata init" before you can access the device ?

this bugs me most because normally u-boot starts up nice & fast, but now that we've added SATA support, there's an annoying second or 3 pause while the disk is probed ...

Rather than have the board code initialize SATA automatically during boot,
make the user manually run "sata init". This brings the SATA subsystem in
line with common U-Boot policy.

Along these lines, the "is_sata_supported()" hook is no longer needed, so
scrub it from the tree.

Signed-off-by Mike Frysinger <[email protected]>

---

board/freescale/mpc8536ds/mpc8536ds.c |   11 -----------
common/cmd_sata.c                     |    7 +++++--
include/sata.h                        |    2 --
lib_ppc/board.c                       |   20 --------------------
4 files changed, 5 insertions(+), 35 deletions(-)

Quoted 10 lines · Mike Frysinger

Rather than have the board code initialize SATA automatically during boot,
make the user manually run "sata init". This brings the SATA subsystem in
line with common U-Boot policy.

Along these lines, the "is_sata_supported()" hook is no longer needed, so
scrub it from the tree.

Quoted 7 lines

Signed-off-by Mike Frysinger <[email protected]>

---

board/freescale/mpc8536ds/mpc8536ds.c |   11 -----------
common/cmd_sata.c                     |    7 +++++--
include/sata.h                        |    2 --
lib_ppc/board.c                       |   20 --------------------
4 files changed, 5 insertions(+), 35 deletions(-)

Why should we make the user have to do 'sata init'? That seems annoying and easy to forget.

Quoted 19 lines · On Thursday 11 December 2008 11:17:56 Kumar Gala wrote:

Quoted 17 lines · Mike Frysinger

Rather than have the board code initialize SATA automatically during boot,
make the user manually run "sata init". This brings the SATA subsystem in
line with common U-Boot policy.

Along these lines, the "is_sata_supported()" hook is no longer needed, so
scrub it from the tree.

Signed-off-by Mike Frysinger <[email protected]>

---

board/freescale/mpc8536ds/mpc8536ds.c |   11 -----------
common/cmd_sata.c                     |    7 +++++--
include/sata.h                        |    2 --
lib_ppc/board.c                       |   20 --------------------
4 files changed, 5 insertions(+), 35 deletions(-)

Quoted 2 lines

Why should we make the user have to do 'sata init'? That seems annoying and easy to forget.

it's already standard behavior with most mass storage things (like mmc and sf
and ...), so it isnt a big deal imo. i asked if Wolfgang wanted it automated
and he preferred making the user do it themselves.

Quoted 20 lines · Mike Frysinger

Quoted 19 lines · On Thursday 11 December 2008 11:17:56 Kumar Gala wrote:

Quoted 17 lines · Mike Frysinger

Rather than have the board code initialize SATA automatically during boot,
make the user manually run "sata init". This brings the SATA subsystem in
line with common U-Boot policy.

Along these lines, the "is_sata_supported()" hook is no longer needed, so
scrub it from the tree.

Signed-off-by Mike Frysinger <[email protected]>

---

board/freescale/mpc8536ds/mpc8536ds.c |   11 -----------
common/cmd_sata.c                     |    7 +++++--
include/sata.h                        |    2 --
lib_ppc/board.c                       |   20 --------------------
4 files changed, 5 insertions(+), 35 deletions(-)

Quoted 6 lines

Quoted 2 lines

Why should we make the user have to do 'sata init'? That seems annoying and easy to forget.

it's already standard behavior with most mass storage things (like mmc and sf and ...), so it isnt a big deal imo. i asked if Wolfgang wanted it automated and he preferred making the user do it themselves.

This seems backwards to me..

'sata init' isn't safe. It seems like you should only be able to call it once. However I think we can keep issuing it and cause bad things to happen.

Also, in the code you removed we do a runtime check on 8536 to see if SATA is even available. That check is still valid.

Quoted 3 lines · On Thursday 11 December 2008 15:51:32 Kumar Gala wrote:

'sata init' isn't safe. It seems like you should only be able to call it once. However I think we can keep issuing it and cause bad things to happen.

i dont think so. the SATA driver should be doing the right thing: init_sata() should get the hardware into a usable state regardless of how it was before.

Quoted 2 lines

Also, in the code you removed we do a runtime check on 8536 to see if SATA is even available. That check is still valid.

why ? if the hardware doesnt support it, then the user shouldnt be attempting to use it. if they do, that's their fault for doing something stupid.

Quoted 5 lines · Mike Frysinger

Quoted 3 lines · On Thursday 11 December 2008 15:51:32 Kumar Gala wrote:

'sata init' isn't safe. It seems like you should only be able to call it once. However I think we can keep issuing it and cause bad things to happen.

Quoted 3 lines

i dont think so. the SATA driver should be doing the right thing:
init_sata() should get the hardware into a usable state regardless of how it was before.

So then what's the downside to having any sata access automatically call init_sata()?

Quoted 6 lines

Quoted 2 lines

Also, in the code you removed we do a runtime check on 8536 to see if SATA is even available. That check is still valid.

why ? if the hardware doesnt support it, then the user shouldnt be attempting to use it. if they do, that's their fault for doing something stupid.

There's no need to be unnecessarily user-hostile.

Quoted 5 lines · On Thursday 11 December 2008 16:01:33 Scott Wood wrote:

Quoted 3 lines · Mike Frysinger

why ? if the hardware doesnt support it, then the user shouldnt be attempting to use it. if they do, that's their fault for doing something stupid.

There's no need to be unnecessarily user-hostile.

there's no need to make u-boot unnecessarily safe for the people who dont understand the system

Quoted 8 lines · Mike Frysinger

Quoted 6 lines · On Thursday 11 December 2008 16:01:33 Scott Wood wrote:

Quoted 3 lines · Mike Frysinger

why ? if the hardware doesnt support it, then the user shouldnt be attempting to use it. if they do, that's their fault for doing something stupid.

There's no need to be unnecessarily user-hostile.

Quoted 2 lines

there's no need to make u-boot unnecessarily safe for the people who dont understand the system

Oh, please. It's not like it's stopping any reasonable usage (quite the opposite -- removing the check removes a way for the user to find out whether SATA is present); it's just avoiding exposing a device that does not actually exist.

it made sense when the init step was automatic. but you're telling me users of your board are incapable of looking at it and going "hmm, this has a SATA disk" ? it isnt like disks are tiny and they have to scan a board for some obscure IC. disks are friggin huge.

Quoted 4 lines · Mike Frysinger

it made sense when the init step was automatic. but you're telling me users of your board are incapable of looking at it and going "hmm, this has a SATA disk" ? it isnt like disks are tiny and they have to scan a board for some obscure IC. disks are friggin huge.

It doesn't matter how big it is, if it's in some remote board farm -- or if I'm considering adding a disk, and I want to see if the board is configured to use those pins for SATA or for something else (and thus whether I need to dig out the board manual and look at DIP switch configuration).

What harm does the check do?

it made sense when the init step was automatic. but you're telling me users

of your board are incapable of looking at it and going "hmm, this has a SATA disk" ? it isnt like disks are tiny and they have to scan a board for some obscure IC. disks are friggin huge.

Quoted 2 lines · In message <[email protected]> you wrote:

it made sense when the init step was automatic. but you're telling me users

And we still want to have it automatic, just deferred.

Quoted 3 lines

of your board are incapable of looking at it and going "hmm, this has a SATA disk" ? it isnt like disks are tiny and they have to scan a board for some obscure IC. disks are friggin huge.

Come on. Have you never seen a system that came in a case, and you didn't know if it included a disk, or how many of these?

having init_sata() be automatic though would prevent hotplugging ... but maybe people dont care about that ...

there's no need to make u-boot unnecessarily safe for the people who dont understand the system

Quoted 3 lines

Also, in the code you removed we do a runtime check on 8536 to see if SATA is even available. That check is still valid.

why ? if the hardware doesnt support it, then the user shouldnt be attempting to use it. if they do, that's their fault for doing something stupid.

Quoted 6 lines · In message Mike Frysinger you

Quoted 2 lines

Also, in the code you removed we do a runtime check on 8536 to see if SATA is even available. That check is still valid.

why ? if the hardware doesnt support it, then the user shouldnt be attempting to use it. if they do, that's their fault for doing something stupid.

And if they do, the code should fail gracefully, i. e. print some friendly error message like that S-ATA is not available.

Quoted 2 lines · On Thursday 11 December 2008 18:53:00 Wolfgang Denk wrote:

And if they do, the code should fail gracefully, i. e. print some friendly error message like that S-ATA is not available.

in this case, i think that's up to the controller. i.e. the controller doesnt find anything and you get a message that nothing was found.

i dont have a board to test with to know what will happen exactly, nor does the current code make any statement that i can see. Scott ?

Quoted 7 lines · Mike Frysinger

Quoted 2 lines · On Thursday 11 December 2008 18:53:00 Wolfgang Denk wrote:

And if they do, the code should fail gracefully, i. e. print some friendly error message like that S-ATA is not available.

in this case, i think that's up to the controller. i.e. the controller doesnt find anything and you get a message that nothing was found.

Quoted 2 lines

i dont have a board to test with to know what will happen exactly, nor does the current code make any statement that i can see. Scott ?

IIUC, if it's "not available" the driver will still see the controller regs, but it will be trying to control pins that are actually routed to another device. That said, it should be the driver calling into SoC-specific code to find out, rather than a global SATA-blocker (what if a PCIe SATA card were used?).

Quoted 4 lines

and ...), so it isnt a big deal imo. i asked if Wolfgang wanted it automated
and he preferred making the user do it themselves.

This seems backwards to me..

'sata init' isn't safe. It seems like you should only be able to call it once. However I think we can keep issuing it and cause bad things to happen.

it's already standard behavior with most mass storage things (like mmc and sf and ...), so it isnt a big deal imo. i asked if Wolfgang wanted it automated and he preferred making the user do it themselves.

board/freescale/ renamed from board/freescale/mpc8536ds/mpc8536ds.c +0 -0

mpc8536ds/mpc8536ds.c
index 2b17612..ada8020 100644

board/freescale/mpc8536ds/mpc8536ds.c +0 -1

@@ -582,17 +582,6 @@ get_board_ddr_clk(ulong dummy)
 }
 #endif
 
-int is_sata_supported(void)

Quoted 9 lines · Mike Frysinger

board/freescale/ renamed from board/freescale/mpc8536ds/mpc8536ds.c +0 -0

mpc8536ds/mpc8536ds.c
index 2b17612..ada8020 100644

board/freescale/mpc8536ds/mpc8536ds.c +0 -0

@@ -582,17 +582,6 @@ get_board_ddr_clk(ulong dummy)
 }
 #endif
 

board/freescale/mpc8536ds/mpc8536ds.c get_board_ddr_clk(ulong dummy) · line 595

Quoted 11 lines

board/freescale/mpc8536ds/mpc8536ds.c +0 -11

-int is_sata_supported(void)
-{
-	volatile ccsr_gur_t *gur = (void *)(CONFIG_SYS_MPC85xx_GUTS_ADDR);
-	uint sdrs2_io_sel =
-		(gur->pordevsr & MPC85xx_PORDEVSR_SRDS2_IO_SEL) >> 27;
-	if (sdrs2_io_sel & 0x04)
-		return 0;
-
-	return 1;
-}
-

you removed this but didn't put it anywhere.. that's not going to work.

Rather than have the board code initialize SATA automatically during boot, make the user manually run "sata init". This brings the SATA subsystem in line with common U-Boot policy.

Dear Mike Frysinger,

In message <[email protected]> you

Quoted 5 lines · wrote:

Rather than have the board code initialize SATA automatically during boot,
make the user manually run "sata init". This brings the SATA subsystem in
line with common U-Boot policy.

I agree with Kumare - having to run "sata init" manually is annoying. We should auto-run it upon the first "sata" command if it hasn't been called before (but still keep the option to run it manually, for
example to (re-) initialize the system after changing the
configuration while the system is running).

Quoted 3 lines

I agree with Kumare - having to run "sata init" manually is annoying.

Quoted 4 lines

We should auto-run it upon the first "sata" command if it hasn't been called before (but still keep the option to run it manually, for
example to (re-) initialize the system after changing the
configuration while the system is running).

But its not just sata commands. I know I've used a SATA disk w/ext2 load commands.

Dear Kumar,

In message <29E3D6BF-AA38-40C3-

Quoted 4 lines · AC21-87DCBBB82CAE at kernel.crashing.org> you wrote:

Quoted 4 lines

I agree with Kumare - having to run "sata init" manually is annoying.

Sorry for the bodus 'e' here.

Quoted 8 lines

Quoted 7 lines

We should auto-run it upon the first "sata" command if it hasn't been
called before (but still keep the option to run it manually, for
example to (re-) initialize the system after changing
the
configuration while the system is running).

Quoted 2 lines

But its not just sata commands. I know I've used a SATA disk w/ext2 load commands.

But this will eventually call some S-ATA function intenrally,w hcih can check that S-ATA has not been initialized yet, and run the init function?

btw, does nand_init() fall into the same category ?

should i convert that to a "nand init" step ?

btw, does nand_init() fall into the same category ? should i convert that to a "nand init" step ?

Quoted 2 lines · In message Mike Frysinger you

btw, does nand_init() fall into the same category ? should i convert that to a "nand init" step ?

Yes, it does.

I'm not sure here, though. Many people seem to find at least the NAND flash size output useful.

Rather than have the board code initialize SATA automatically during boot,
make the user manually run "sata init". This brings the SATA subsystem in
line with common U-Boot policy.

Along these lines, the "is_sata_supported()" hook is no longer needed, so
scrub it from the tree.

Rather than have the board code initialize SATA automatically during boot, make the user manually run "sata init". This brings the SATA subsystem in line with common U-Boot policy.

Rather than having a dedicated weak function "is_sata_supported", people can override sata_initialize() to do their weird board stuff. Then they can call the actual __sata_initialize().

Rather than have the board code initialize SATA automatically during boot, make the user manually run "sata init". This brings the SATA subsystem in line with common U-Boot policy.

Rather than having a dedicated weak function "is_sata_supported", people can override sata_initialize() to do their weird board stuff. Then they can call the actual __sata_initialize().

Signed-off-by Mike Frysinger <[email protected]>

---
v3
- redo how the board weak is handled to be less bloated when screwing over everyone else

v2
- auto run sata init if needed when doing other sata ... cmds

 board/freescale/mpc8536ds/mpc8536ds.c |    6 +++---
 common/cmd_sata.c                     |   12 +++++++++++-
 include/sata.h                        |    1 +
 lib_ppc/board.c                       |   20 --------------------
 4 files changed, 15 insertions(+), 24 deletions(-)

Rather than have the board code initialize SATA automatically during boot, make the user manually run "sata init". This brings the SATA subsystem in line with common U-Boot policy.

Rather than having a dedicated weak function "is_sata_supported", people can override sata_initialize() to do their weird board stuff. Then they can call the actual __sata_initialize().

Signed-off-by Mike Frysinger <[email protected]>

---
v4
- rebase against latest master

v3
- redo how the board weak is handled to be less bloated when screwing over everyone else

v2
- auto run sata init if needed when doing other sata ...

 board/freescale/mpc8536ds/mpc8536ds.c |    6 +++---
 common/cmd_sata.c                     |   12 +++++++++++-
 include/sata.h                        |    1 +
 lib_ppc/board.c                       |   20 --------------------
 4 files changed, 15 insertions(+), 24 deletions(-)

Quoted 2 lines

btw, does nand_init() fall into the same category ? should i convert that to a "nand init" step ?

Yes, it does.

I'm not sure here, though. Many people seem to find at least the NAND flash size output useful.

Quoted 3 lines · Wolfgang Denk

Quoted 2 lines

btw, does nand_init() fall into the same category ? should i convert that to a "nand init" step ?

Quoted 4 lines

Yes, it does.

I'm not sure here, though. Many people seem to find at least the NAND flash size output useful.

Yes. And please don't forget that some boards, especially the ones with NAND booting support use NAND for environment storage. So nand_init() needs to be called here pretty early for env to work.