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

[U-Boot] [PATCH] 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 01 ms to be on the safe side.
---
 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..2b6b706 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;
+	/* Toggle control DET back to 0 (normal operation) */
 	writel(control, &port->scontrol);
+	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] mvsata_ide: adjust port init sequence

[Sergei Shtylyov]

Hello.

On 04-09-2010 14:34, 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 01 ms to be on the safe side.
> ---
>   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..2b6b706 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;
> +	/* Toggle control DET back to 0 (normal operation) */

    But you haven't set it to 1 yet. Maybe the comment is misplaced?

>   	writel(control, &port->scontrol);
> +	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);

    Why not !tout? (And you forgot a space before colon. :-)

WBR, Sergei

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

[Albert ARIBAUD]

Hi Sergei,

Le 05/09/2010 12:54, Sergei Shtylyov a ?crit :

>> + /* 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;
>> + /* Toggle control DET back to 0 (normal operation) */
>
> But you haven't set it to 1 yet. Maybe the comment is misplaced?

Look at the comment above, which said DET was set to 1, and at 
MVSATA_PORT_INIT, which combines IPM=3 and DET=1.

>> + return (tout? 0: 1);
>
> Why not !tout? (And you forgot a space before colon. :-)

Why not indeed? I'll fix this in the next version.

> WBR, Sergei

Thanks!

Amicalement,
-- 
Albert.

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

[Sergei Shtylyov]

Hello.

On 05.09.2010 23:33, Albert ARIBAUD wrote:

>>> + /* 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;
>>> + /* Toggle control DET back to 0 (normal operation) */

>> But you haven't set it to 1 yet. Maybe the comment is misplaced?

> Look at the comment above, which said DET was set to 1, and at
> MVSATA_PORT_INIT, which combines IPM=3 and DET=1.

    But you didn't write that value yet before your comment saying that you're 
resetting DET back to 0.

WBR, Sergei

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

[Albert ARIBAUD]

Le 06/09/2010 11:18, Sergei Shtylyov a ?crit :
> Hello.
>
> On 05.09.2010 23:33, Albert ARIBAUD wrote:
>
>>>> + /* 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;
>>>> + /* Toggle control DET back to 0 (normal operation) */
>
>>> But you haven't set it to 1 yet. Maybe the comment is misplaced?
>
>> Look at the comment above, which said DET was set to 1, and at
>> MVSATA_PORT_INIT, which combines IPM=3 and DET=1.
>
> But you didn't write that value yet before your comment saying that
> you're resetting DET back to 0.

Yes, I did:

#define MVSATA_PORT_INIT \
	(MVSATA_SCONTROL_DET_INIT|MVSATA_SCONTROL_IPM_NO_LP_ALLOWED)

MVSATA_PORT_INIT has both IPM = 3 (no low power allowed) and DET = 1 
(initialize).

Amicalement,
-- 
Albert.

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

[Sergei Shtylyov]

Hello.

Albert ARIBAUD wrote:

>> On 05.09.2010 23:33, Albert ARIBAUD wrote:

>>>>> + /* 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;
>>>>> + /* Toggle control DET back to 0 (normal operation) */

>>>> But you haven't set it to 1 yet. Maybe the comment is misplaced?

>>> Look at the comment above, which said DET was set to 1, and at
>>> MVSATA_PORT_INIT, which combines IPM=3 and DET=1.

>> But you didn't write that value yet before your comment saying that
>> you're resetting DET back to 0.

> Yes, I did:

> #define MVSATA_PORT_INIT \
>     (MVSATA_SCONTROL_DET_INIT|MVSATA_SCONTROL_IPM_NO_LP_ALLOWED)

> MVSATA_PORT_INIT has both IPM = 3 (no low power allowed) and DET = 1 
> (initialize).

    Sigh. Your "resetting DET" comment is before the first writel().

> Amicalement,

WBR, Sergei

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

[Albert ARIBAUD]

Le 06/09/2010 14:45, Sergei Shtylyov a ?crit :
> Hello.
>
> Albert ARIBAUD wrote:
>
>>> On 05.09.2010 23:33, Albert ARIBAUD wrote:
>
>>>>>> + /* 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;
>>>>>> + /* Toggle control DET back to 0 (normal operation) */
>
>>>>> But you haven't set it to 1 yet. Maybe the comment is misplaced?
>
>>>> Look at the comment above, which said DET was set to 1, and at
>>>> MVSATA_PORT_INIT, which combines IPM=3 and DET=1.
>
>>> But you didn't write that value yet before your comment saying that
>>> you're resetting DET back to 0.
>
>> Yes, I did:
>
>> #define MVSATA_PORT_INIT \
>> (MVSATA_SCONTROL_DET_INIT|MVSATA_SCONTROL_IPM_NO_LP_ALLOWED)
>
>> MVSATA_PORT_INIT has both IPM = 3 (no low power allowed) and DET = 1
>> (initialize).
>
> Sigh. Your "resetting DET" comment is before the first writel().
>
>> Amicalement,
>
> WBR, Sergei

Oh, I see! I'll push a new version with the comment moved down one 
notch. Thanks for your patience. :)

Amicalement,
-- 
Albert.

[U-Boot] [PATCH] 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] mvsata_ide: adjust port init sequence

[Albert ARIBAUD]

Le 07/09/2010 00:53, Albert Aribaud a ?crit :
>

Sorry, missed the "V2" part in the tag -- ignore this post.

Amicalement,
-- 
Albert.

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

[Wolfgang Denk]

Dear Albert Aribaud,

In message <1283813585-6565-1-git-send-email-albert.aribaud@free.fr> you 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>
> ---
> 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.

Could you _please_ include the patch version information in the
Subject: line?

Thanks.


Note: the "--subject-prefix" option for git-format-patch allows to do
this easily, without manual editing of the patch(es).

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
A good marriage would be between a blind wife and deaf husband.
                                               -- Michel de Montaigne

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

[Wolfgang Denk]

Dear Albert ARIBAUD,

In message <4C8572F1.8030106@free.fr> you wrote:
> Le 07/09/2010 00:53, Albert Aribaud a =E9crit :
> >
> 
> Sorry, missed the "V2" part in the tag -- ignore this post.

Argh... And I missed the followup.

Please ignore my mail, too. :-(

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
"If that makes any sense to you, you have a big problem."
                                  -- C. Durance, Computer Science 234