[U-Boot] [PATCH V2] mvsata_ide: adjust port init sequence

[U-Boot] [PATCH V2] mvsata_ide: adjust port init sequence

[Albert Aribaud]

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;
-- 
1.7.0.4

[U-Boot] [PATCH V2] mvsata_ide: adjust port init sequence

[Prafulla Wadaskar]

 

> -----Original Message-----
> From: u-boot-bounces at lists.denx.de 
> [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Albert Aribaud
> Sent: Tuesday, September 07, 2010 4:33 AM
> To: u-boot at 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 at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> 

[U-Boot] [PATCH V2] mvsata_ide: adjust port init sequence

[Albert ARIBAUD]

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,
-- 
Albert.

[U-Boot] [PATCH V2] mvsata_ide: adjust port init sequence

[Prafulla Wadaskar]

 

> -----Original Message-----
> From: Albert ARIBAUD [mailto:albert.aribaud at free.fr] 
> Sent: Tuesday, September 07, 2010 4:53 PM
> To: Prafulla Wadaskar
> Cc: u-boot at 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.
> 

[U-Boot] [PATCH V2] mvsata_ide: adjust port init sequence

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Zero is an enigmatic value. It can mean success (fclose)  or  failure
(scanf).  It  can  mean  black  or  white. It can mean no permissions
(chmod) or all permissions (umask). It can mean now (setjmp) or later
(atexit). It can mean the beginning (lseek) or the end (read). It can
mean myself (getpgrp) or child (fork). It can mean  all  (kill's  1st
argument)  or  nothing  (kill's  2nd argument). It can mean `default'
(SIG_IGN) or `I don't care' (waitpid) or  `try  to  guess'  (strtol).
Indeed 0 lets you talk to God (setuid).
Verily is 0 all things to all people.

[U-Boot] [PATCH V2] mvsata_ide: adjust port init sequence

[Albert ARIBAUD]

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,
-- 
Albert.

[U-Boot] [PATCH V2] mvsata_ide: adjust port init sequence

[Wolfgang Denk]

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

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Absolute: Independent, irresponsible. An absolute monarchy is one  in
which  the  sovereign  does  as  he pleases so long as he pleases the
assassins. Not many absolute monarchies are left, most of them having
been replaced by limited monarchies, where the soverign's  power  for
evil (and for good) is greatly curtailed, and by republics, which are
governed by chance.                                  - Ambrose Bierce

[U-Boot] [PATCH V2] mvsata_ide: adjust port init sequence

[Albert ARIBAUD]

Le 07/09/2010 21:00, Wolfgang Denk a ?crit :
> 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.

Understood.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH V2] mvsata_ide: adjust port init sequence

[Sergei Shtylyov]

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 Aribaud<albert.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