[U-Boot] [PATCH V2] mvsata_ide: adjust port init sequence
mvsata_ide_initialize_port(): adjust init sequence (SStatus should be checked only after all writes to SControl) and return success/failure to ide_preinit().
Also, as some tests showed init durations in the hundreds of us, raise the time-out to 10 ms to be on the safe side.
Signed-off-by: Albert Aribaud albert.aribaud@free.fr --- PATCH HISTORY
V1 Initial patch. V2 Fixed wrong placement of comment. Fixed wrong description (was 01 ms, should have been 10). Added Signed-off-by.
drivers/block/mvsata_ide.c | 22 ++++++++++++++-------- 1 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/block/mvsata_ide.c b/drivers/block/mvsata_ide.c index 077b278..470659d 100644 --- a/drivers/block/mvsata_ide.c +++ b/drivers/block/mvsata_ide.c @@ -97,23 +97,27 @@ struct mvsata_port_registers { * DET back to "no action". */
-static void mvsata_ide_initialize_port(struct mvsata_port_registers *port) +static int mvsata_ide_initialize_port(struct mvsata_port_registers *port) { u32 control; u32 status; - u32 tout = 50; /* wait at most 50 us for SATA reset to complete */ + u32 tout = 10000; /* wait at most 10 ms for SATA reset to complete */
+ /* Set control IPM to 3 (no low power) and DET to 1 (initialize) */ control = readl(&port->scontrol); control = (control & ~MVSATA_SCONTROL_MASK) | MVSATA_PORT_INIT; writel(control, &port->scontrol); + /* Toggle control DET back to 0 (normal operation) */ + control = (control & ~MVSATA_SCONTROL_MASK) | MVSATA_PORT_USE; + writel(control, &port->scontrol); + /* wait for status DET to become 3 (device and communication OK) */ while (--tout) { status = readl(&port->sstatus) & MVSATA_SSTATUS_DET_MASK; if (status == MVSATA_SSTATUS_DET_DEVCOMM) break; udelay(1); } - control = (control & ~MVSATA_SCONTROL_MASK) | MVSATA_PORT_USE; - writel(control, &port->scontrol); + return (tout? 0: 1); }
/* @@ -125,15 +129,17 @@ int ide_preinit(void) { /* Enable ATA port 0 (could be SATA port 0 or 1) if declared */ #if defined(CONFIG_SYS_ATA_IDE0_OFFSET) - mvsata_ide_initialize_port( + if (mvsata_ide_initialize_port( (struct mvsata_port_registers *) - (CONFIG_SYS_ATA_BASE_ADDR + CONFIG_SYS_ATA_IDE0_OFFSET)); + (CONFIG_SYS_ATA_BASE_ADDR + CONFIG_SYS_ATA_IDE0_OFFSET))) + return 1; #endif /* Enable ATA port 1 (could be SATA port 0 or 1) if declared */ #if defined(CONFIG_SYS_ATA_IDE1_OFFSET) - mvsata_ide_initialize_port( + if (mvsata_ide_initialize_port( (struct mvsata_port_registers *) - (CONFIG_SYS_ATA_BASE_ADDR + CONFIG_SYS_ATA_IDE1_OFFSET)); + (CONFIG_SYS_ATA_BASE_ADDR + CONFIG_SYS_ATA_IDE1_OFFSET))) + return 1; #endif /* return 0 as we always succeed */ return 0;
-----Original Message----- From: u-boot-bounces@lists.denx.de [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Albert Aribaud Sent: Tuesday, September 07, 2010 4:33 AM To: u-boot@lists.denx.de Subject: [U-Boot] [PATCH V2] mvsata_ide: adjust port init sequence
mvsata_ide_initialize_port(): adjust init sequence (SStatus should be checked only after all writes to SControl) and return success/failure to ide_preinit().
Also, as some tests showed init durations in the hundreds of us, raise the time-out to 10 ms to be on the safe side.
Signed-off-by: Albert Aribaud albert.aribaud@free.fr
PATCH HISTORY
V1 Initial patch. V2 Fixed wrong placement of comment. Fixed wrong description (was 01 ms, should have been 10). Added Signed-off-by.
drivers/block/mvsata_ide.c | 22 ++++++++++++++-------- 1 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/block/mvsata_ide.c b/drivers/block/mvsata_ide.c index 077b278..470659d 100644 --- a/drivers/block/mvsata_ide.c +++ b/drivers/block/mvsata_ide.c @@ -97,23 +97,27 @@ struct mvsata_port_registers {
- DET back to "no action".
*/
-static void mvsata_ide_initialize_port(struct mvsata_port_registers *port) +static int mvsata_ide_initialize_port(struct mvsata_port_registers *port) { u32 control; u32 status;
- u32 tout = 50; /* wait at most 50 us for SATA reset to
complete */
- u32 tout = 10000; /* wait at most 10 ms for SATA reset
to complete */
- /* Set control IPM to 3 (no low power) and DET to 1
(initialize) */ control = readl(&port->scontrol); control = (control & ~MVSATA_SCONTROL_MASK) | MVSATA_PORT_INIT; writel(control, &port->scontrol);
- /* Toggle control DET back to 0 (normal operation) */
- control = (control & ~MVSATA_SCONTROL_MASK) | MVSATA_PORT_USE;
- writel(control, &port->scontrol);
- /* wait for status DET to become 3 (device and
communication OK) */ while (--tout) { status = readl(&port->sstatus) & MVSATA_SSTATUS_DET_MASK; if (status == MVSATA_SSTATUS_DET_DEVCOMM) break; udelay(1); }
- control = (control & ~MVSATA_SCONTROL_MASK) | MVSATA_PORT_USE;
- writel(control, &port->scontrol);
- return (tout? 0: 1);
As suggested in earlier review, can you pls use !tout instead. Or please add a space for ":0"
}
/* @@ -125,15 +129,17 @@ int ide_preinit(void) { /* Enable ATA port 0 (could be SATA port 0 or 1) if declared */ #if defined(CONFIG_SYS_ATA_IDE0_OFFSET)
- mvsata_ide_initialize_port(
- if (mvsata_ide_initialize_port( (struct mvsata_port_registers *)
(CONFIG_SYS_ATA_BASE_ADDR +CONFIG_SYS_ATA_IDE0_OFFSET));
(CONFIG_SYS_ATA_BASE_ADDR +CONFIG_SYS_ATA_IDE0_OFFSET)))
return 1;
How about returning negative values for errors ?
#endif /* Enable ATA port 1 (could be SATA port 0 or 1) if declared */ #if defined(CONFIG_SYS_ATA_IDE1_OFFSET)
- mvsata_ide_initialize_port(
- if (mvsata_ide_initialize_port( (struct mvsata_port_registers *)
(CONFIG_SYS_ATA_BASE_ADDR +CONFIG_SYS_ATA_IDE1_OFFSET));
(CONFIG_SYS_ATA_BASE_ADDR +CONFIG_SYS_ATA_IDE1_OFFSET)))
return 1;
Same here
Regards.. Prafulla . .
#endif /* return 0 as we always succeed */ return 0; -- 1.7.0.4
U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Le 07/09/2010 08:42, Prafulla Wadaskar a écrit :
diff --git a/drivers/block/mvsata_ide.c b/drivers/block/mvsata_ide.c
@@ -125,15 +129,17 @@ int ide_preinit(void)
return 1;How about returning negative values for errors ?
Function ide_preinit() is called from cmd_ide.c:ide_init(), which does not distinguish positive vs negative return values, ony zero vs non-zero. So what would be the point of returning negative rather than positive values?
Amicalement,
-----Original Message----- From: Albert ARIBAUD [mailto:albert.aribaud@free.fr] Sent: Tuesday, September 07, 2010 4:53 PM To: Prafulla Wadaskar Cc: u-boot@lists.denx.de; Prabhanjan Sarnaik; Ashish Karkare Subject: Re: [PATCH V2] mvsata_ide: adjust port init sequence
Le 07/09/2010 08:42, Prafulla Wadaskar a écrit :
diff --git a/drivers/block/mvsata_ide.c
b/drivers/block/mvsata_ide.c
@@ -125,15 +129,17 @@ int ide_preinit(void)
return 1;How about returning negative values for errors ?
Function ide_preinit() is called from cmd_ide.c:ide_init(), which does not distinguish positive vs negative return values, ony zero vs non-zero. So what would be the point of returning negative rather than positive values?
Negative always represents errors, whereas positive may represent some valid return state. In this case it may not be that important.
Regards.. Prafulla . .
Amicalement,
Albert.
Dear Prafulla Wadaskar,
In message F766E4F80769BD478052FB6533FA745D19A6879824@SC-VEXCH4.marvell.com you wrote:
Negative always represents errors, whereas positive may represent some valid return state.
He. This is _not_quite_ correct. Not in U-Boot, and not in genreal.
[But your comment asking for a negative return code is valid, of course.]
Best regards,
Wolfgang Denk
Le 07/09/2010 16:06, Wolfgang Denk a écrit :
Negative always represents errors, whereas positive may represent some valid return state.
He. This is _not_quite_ correct. Not in U-Boot, and not in genreal.
[But your comment asking for a negative return code is valid, of course.]
I'm a bit lost at the logic of this last sentence. If errors are not always represented as negative return values, and especially so in U-boot, then what is the rationale for supporting a request for specifically negative error return values, rather than positive, here? Not that I mind much -- as I said, ide_init() does not mind, and that's my reference -- and I'll happily put negative error codes in V3, but I like to understand why people want things.
Amicalement,
Dear Albert ARIBAUD,
In message 4C867741.6060906@free.fr you wrote:
I'm a bit lost at the logic of this last sentence. If errors are not always represented as negative return values, and especially so in U-boot, then what is the rationale for supporting a request for specifically negative error return values, rather than positive, here?
Even though ther eis lots of code that could need improvement, we should not take such bad examples, but rather try to apply standard techniques instead.
Not that I mind much -- as I said, ide_init() does not mind, and that's my reference -- and I'll happily put negative error codes in V3, but I like to understand why people want things.
It's just a pretty common thing to do, and instead of doing the same thing in many different ways it makes some sense to use a standard way.
Best regards,
Wolfgang Denk
Le 07/09/2010 21:00, Wolfgang Denk a écrit :
Dear Albert ARIBAUD,
In message4C867741.6060906@free.fr you wrote:
I'm a bit lost at the logic of this last sentence. If errors are not always represented as negative return values, and especially so in U-boot, then what is the rationale for supporting a request for specifically negative error return values, rather than positive, here?
Even though ther eis lots of code that could need improvement, we should not take such bad examples, but rather try to apply standard techniques instead.
Not that I mind much -- as I said, ide_init() does not mind, and that's my reference -- and I'll happily put negative error codes in V3, but I like to understand why people want things.
It's just a pretty common thing to do, and instead of doing the same thing in many different ways it makes some sense to use a standard way.
Understood.
Amicalement,
Hello.
On 07-09-2010 3:02, Albert Aribaud wrote:
mvsata_ide_initialize_port(): adjust init sequence (SStatus should be checked only after all writes to SControl) and return success/failure to ide_preinit().
Also, as some tests showed init durations in the hundreds of us, raise the time-out to 10 ms to be on the safe side.
Signed-off-by: Albert Aribaudalbert.aribaud@free.fr
[...]
diff --git a/drivers/block/mvsata_ide.c b/drivers/block/mvsata_ide.c index 077b278..470659d 100644 --- a/drivers/block/mvsata_ide.c +++ b/drivers/block/mvsata_ide.c @@ -97,23 +97,27 @@ struct mvsata_port_registers {
- DET back to "no action".
*/
-static void mvsata_ide_initialize_port(struct mvsata_port_registers *port) +static int mvsata_ide_initialize_port(struct mvsata_port_registers *port) { u32 control; u32 status;
- u32 tout = 50; /* wait at most 50 us for SATA reset to complete */
u32 tout = 10000; /* wait at most 10 ms for SATA reset to complete */
/* Set control IPM to 3 (no low power) and DET to 1 (initialize) */ control = readl(&port->scontrol); control = (control& ~MVSATA_SCONTROL_MASK) | MVSATA_PORT_INIT; writel(control,&port->scontrol);
/* Toggle control DET back to 0 (normal operation) */
control = (control& ~MVSATA_SCONTROL_MASK) | MVSATA_PORT_USE;
writel(control,&port->scontrol);
/* wait for status DET to become 3 (device and communication OK) */ while (--tout) { status = readl(&port->sstatus)& MVSATA_SSTATUS_DET_MASK; if (status == MVSATA_SSTATUS_DET_DEVCOMM) break; udelay(1); }
- control = (control& ~MVSATA_SCONTROL_MASK) | MVSATA_PORT_USE;
- writel(control,&port->scontrol);
- return (tout? 0: 1);
Weren't you going to replace this with '!tout'? And anyway, you omitted space before '?' this time, not only before ':'.
WBR, Sergei
participants (5)
-
Albert ARIBAUD -
Albert Aribaud
-
Prafulla Wadaskar -
Sergei Shtylyov -
Wolfgang Denk