Re: [U-Boot] [PATCH v1] x86: ahci: Make sure interface is not busy after enabling the port
Hi Bin,
On 23 December 2014 at 18:42, Bin Meng bmeng...@gmail.com wrote:
Hi Simon,
On Wed, Dec 24, 2014 at 3:42 AM, Simon Glass s...@chromium.org wrote:
Hi Bin,
On 23 December 2014 at 01:01, Bin Meng bmeng...@gmail.com wrote:
Each time U-Boot boots on Intel Crown Bay board, the displayed hard
drive information is wrong. It could be either wrong capacity or just
a 'Capacity: not available' message. After enabling the debug switch,
we can see the scsi inquiry command did not execute successfully.
However, doing a 'scsi scan' in the U-Boot shell does not expose
this issue.
This sounds like an error condition is not being propagated.
Actually an error condition not checked.
SCSI: Target spinup took 0 ms.
SATA link 1 timeout.
AHCI 0001.0100 32 slots 2 ports 3 Gbps 0x3 impl SATA mode
flags: ncq stag pm led clo only pmp pio slum part ccc apst
scanning bus for devices...
ahci_device_data_io: 0 byte transferred. --- scsi inquiry fails
ahci_device_data_io: 512 byte transferred.
ahci_device_data_io: 512 byte transferred.
ahci_device_data_io: 512 byte transferred.
Device 0: (0:0) Vendor: ATA Prod.: Rev: ?8
Type: Hard Disk
Capacity: 912968.3 MB = 891.5 GB (1869759264 x
512)
Found 1 device(s).
So uninitialized contents on the stack were passed to dev_print() to
display those wrong information.
The symptom were observed on two hard drives (one is Seagate, the
other one is Western Digital). The fix is to make sure the AHCI
interface is not busy by checking the error and status information
from task file register after enabling the port in ahci_port_start()
before proceeding other operations like scsi_scan().
Signed-off-by: Bin Meng bmeng...@gmail.com
---
drivers/block/ahci.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/block/ahci.c b/drivers/block/ahci.c
index c9a3beb..7ca8f40 100644
--- a/drivers/block/ahci.c
+++ b/drivers/block/ahci.c
@@ -505,8 +505,9 @@ static int ahci_port_start(u8 port)
{
struct ahci_ioports *pp = (probe_ent-port[port]);
volatile u8 *port_mmio = (volatile u8 *)pp-port_mmio;
- u32 port_status;
+ u32 port_status, tf_data;
u32 mem;
+ int i = 0;
debug(Enter start port: %d\n, port);
port_status = readl(port_mmio + PORT_SCR_STAT);
@@ -564,6 +565,18 @@ static int ahci_port_start(u8 port)
PORT_CMD_POWER_ON | PORT_CMD_SPIN_UP |
PORT_CMD_START, port_mmio + PORT_CMD);
+ /*
+* Make sure interface is not busy based on error and status
+* information from task file data register before proceeding
+*/
+ while (i WAIT_MS_SPINUP) {
+ tf_data = readl(port_mmio + PORT_TFDATA);
+ if (!(tf_data ATA_BUSY))
+ break;
+ udelay(1000);
+ i++;
+ }
+
Doesn't this fall through after a timeout and fail to report an error?
Ah, yes! We should return error code when timeout. But some other
routines in the scsi initialization path do no check return values,
like initr_scsi()-scsi_init()-scsi_low_level_init().
I suppose that could be improved separately but does it affect this patch?
As a matter of style I think something like the below is better
because it the timeout will be more accurate in the case where you
have a lot of processing each time around the loop.
static int wait_spinup(void)
{
ulong start;
start = get_timer(0);
do {
tf_data = ...;
if (!((tf_data ATA_BUSY))
return 0;
while (get_timer(start) WAIT_MS_SPINUP);
return -ETIMEDOUT;
}
Looks like the original codes there are using i++ style for the
timeout check instead of get_timer().
Also how does this relate to the existing spinup delay code in
ahci_host_init()?
This seems to me they are not related. Per my understanding, the check
we need here is because we write something to the port command
register, but we missed the task file data busy indication.
writel_with_flush(PORT_CMD_ICC_ACTIVE | PORT_CMD_FIS_RX |
PORT_CMD_POWER_ON | PORT_CMD_SPIN_UP |
PORT_CMD_START, port_mmio + PORT_CMD);
[snip]
OK.
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1] x86: ahci: Make sure interface is not busy after enabling the port
Hi Simon,
On Tue, Dec 30, 2014 at 12:33 AM, Simon Glass s...@chromium.org wrote:
Hi Bin,
On 23 December 2014 at 18:42, Bin Meng bmeng...@gmail.com wrote:
Hi Simon,
On Wed, Dec 24, 2014 at 3:42 AM, Simon Glass s...@chromium.org wrote:
Hi Bin,
On 23 December 2014 at 01:01, Bin Meng bmeng...@gmail.com wrote:
Each time U-Boot boots on Intel Crown Bay board, the displayed hard
drive information is wrong. It could be either wrong capacity or just
a 'Capacity: not available' message. After enabling the debug switch,
we can see the scsi inquiry command did not execute successfully.
However, doing a 'scsi scan' in the U-Boot shell does not expose
this issue.
This sounds like an error condition is not being propagated.
Actually an error condition not checked.
SCSI: Target spinup took 0 ms.
SATA link 1 timeout.
AHCI 0001.0100 32 slots 2 ports 3 Gbps 0x3 impl SATA mode
flags: ncq stag pm led clo only pmp pio slum part ccc apst
scanning bus for devices...
ahci_device_data_io: 0 byte transferred. --- scsi inquiry fails
ahci_device_data_io: 512 byte transferred.
ahci_device_data_io: 512 byte transferred.
ahci_device_data_io: 512 byte transferred.
Device 0: (0:0) Vendor: ATA Prod.: Rev: ?8
Type: Hard Disk
Capacity: 912968.3 MB = 891.5 GB (1869759264 x
512)
Found 1 device(s).
So uninitialized contents on the stack were passed to dev_print() to
display those wrong information.
The symptom were observed on two hard drives (one is Seagate, the
other one is Western Digital). The fix is to make sure the AHCI
interface is not busy by checking the error and status information
from task file register after enabling the port in ahci_port_start()
before proceeding other operations like scsi_scan().
Signed-off-by: Bin Meng bmeng...@gmail.com
---
drivers/block/ahci.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/block/ahci.c b/drivers/block/ahci.c
index c9a3beb..7ca8f40 100644
--- a/drivers/block/ahci.c
+++ b/drivers/block/ahci.c
@@ -505,8 +505,9 @@ static int ahci_port_start(u8 port)
{
struct ahci_ioports *pp = (probe_ent-port[port]);
volatile u8 *port_mmio = (volatile u8 *)pp-port_mmio;
- u32 port_status;
+ u32 port_status, tf_data;
u32 mem;
+ int i = 0;
debug(Enter start port: %d\n, port);
port_status = readl(port_mmio + PORT_SCR_STAT);
@@ -564,6 +565,18 @@ static int ahci_port_start(u8 port)
PORT_CMD_POWER_ON | PORT_CMD_SPIN_UP |
PORT_CMD_START, port_mmio + PORT_CMD);
+ /*
+* Make sure interface is not busy based on error and status
+* information from task file data register before proceeding
+*/
+ while (i WAIT_MS_SPINUP) {
+ tf_data = readl(port_mmio + PORT_TFDATA);
+ if (!(tf_data ATA_BUSY))
+ break;
+ udelay(1000);
+ i++;
+ }
+
Doesn't this fall through after a timeout and fail to report an error?
Ah, yes! We should return error code when timeout. But some other
routines in the scsi initialization path do no check return values,
like initr_scsi()-scsi_init()-scsi_low_level_init().
I suppose that could be improved separately but does it affect this patch?
No, it doesn't affect this patch. Yes, we could improve those places
with a separate patch.
As a matter of style I think something like the below is better
because it the timeout will be more accurate in the case where you
have a lot of processing each time around the loop.
static int wait_spinup(void)
{
ulong start;
start = get_timer(0);
do {
tf_data = ...;
if (!((tf_data ATA_BUSY))
return 0;
while (get_timer(start) WAIT_MS_SPINUP);
return -ETIMEDOUT;
}
Looks like the original codes there are using i++ style for the
timeout check instead of get_timer().
Also how does this relate to the existing spinup delay code in
ahci_host_init()?
This seems to me they are not related. Per my understanding, the check
we need here is because we write something to the port command
register, but we missed the task file data busy indication.
writel_with_flush(PORT_CMD_ICC_ACTIVE | PORT_CMD_FIS_RX |
PORT_CMD_POWER_ON | PORT_CMD_SPIN_UP |
PORT_CMD_START, port_mmio + PORT_CMD);
[snip]
OK.
Regards,
Bin
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v1] x86: ahci: Make sure interface is not busy after enabling the port
Each time U-Boot boots on Intel Crown Bay board, the displayed hard
drive information is wrong. It could be either wrong capacity or just
a 'Capacity: not available' message. After enabling the debug switch,
we can see the scsi inquiry command did not execute successfully.
However, doing a 'scsi scan' in the U-Boot shell does not expose
this issue.
SCSI: Target spinup took 0 ms.
SATA link 1 timeout.
AHCI 0001.0100 32 slots 2 ports 3 Gbps 0x3 impl SATA mode
flags: ncq stag pm led clo only pmp pio slum part ccc apst
scanning bus for devices...
ahci_device_data_io: 0 byte transferred. --- scsi inquiry fails
ahci_device_data_io: 512 byte transferred.
ahci_device_data_io: 512 byte transferred.
ahci_device_data_io: 512 byte transferred.
Device 0: (0:0) Vendor: ATA Prod.: Rev: ?8
Type: Hard Disk
Capacity: 912968.3 MB = 891.5 GB (1869759264 x 512)
Found 1 device(s).
So uninitialized contents on the stack were passed to dev_print() to
display those wrong information.
The symptom were observed on two hard drives (one is Seagate, the
other one is Western Digital). The fix is to make sure the AHCI
interface is not busy by checking the error and status information
from task file register after enabling the port in ahci_port_start()
before proceeding other operations like scsi_scan().
Signed-off-by: Bin Meng bmeng...@gmail.com
---
drivers/block/ahci.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/block/ahci.c b/drivers/block/ahci.c
index c9a3beb..7ca8f40 100644
--- a/drivers/block/ahci.c
+++ b/drivers/block/ahci.c
@@ -505,8 +505,9 @@ static int ahci_port_start(u8 port)
{
struct ahci_ioports *pp = (probe_ent-port[port]);
volatile u8 *port_mmio = (volatile u8 *)pp-port_mmio;
- u32 port_status;
+ u32 port_status, tf_data;
u32 mem;
+ int i = 0;
debug(Enter start port: %d\n, port);
port_status = readl(port_mmio + PORT_SCR_STAT);
@@ -564,6 +565,18 @@ static int ahci_port_start(u8 port)
PORT_CMD_POWER_ON | PORT_CMD_SPIN_UP |
PORT_CMD_START, port_mmio + PORT_CMD);
+ /*
+* Make sure interface is not busy based on error and status
+* information from task file data register before proceeding
+*/
+ while (i WAIT_MS_SPINUP) {
+ tf_data = readl(port_mmio + PORT_TFDATA);
+ if (!(tf_data ATA_BUSY))
+ break;
+ udelay(1000);
+ i++;
+ }
+
debug(Exit start port %d\n, port);
return 0;
--
1.8.2.1
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1] x86: ahci: Make sure interface is not busy after enabling the port
Hi Bin,
On 23 December 2014 at 01:01, Bin Meng bmeng...@gmail.com wrote:
Each time U-Boot boots on Intel Crown Bay board, the displayed hard
drive information is wrong. It could be either wrong capacity or just
a 'Capacity: not available' message. After enabling the debug switch,
we can see the scsi inquiry command did not execute successfully.
However, doing a 'scsi scan' in the U-Boot shell does not expose
this issue.
This sounds like an error condition is not being propagated.
SCSI: Target spinup took 0 ms.
SATA link 1 timeout.
AHCI 0001.0100 32 slots 2 ports 3 Gbps 0x3 impl SATA mode
flags: ncq stag pm led clo only pmp pio slum part ccc apst
scanning bus for devices...
ahci_device_data_io: 0 byte transferred. --- scsi inquiry fails
ahci_device_data_io: 512 byte transferred.
ahci_device_data_io: 512 byte transferred.
ahci_device_data_io: 512 byte transferred.
Device 0: (0:0) Vendor: ATA Prod.: Rev: ?8
Type: Hard Disk
Capacity: 912968.3 MB = 891.5 GB (1869759264 x 512)
Found 1 device(s).
So uninitialized contents on the stack were passed to dev_print() to
display those wrong information.
The symptom were observed on two hard drives (one is Seagate, the
other one is Western Digital). The fix is to make sure the AHCI
interface is not busy by checking the error and status information
from task file register after enabling the port in ahci_port_start()
before proceeding other operations like scsi_scan().
Signed-off-by: Bin Meng bmeng...@gmail.com
---
drivers/block/ahci.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/block/ahci.c b/drivers/block/ahci.c
index c9a3beb..7ca8f40 100644
--- a/drivers/block/ahci.c
+++ b/drivers/block/ahci.c
@@ -505,8 +505,9 @@ static int ahci_port_start(u8 port)
{
struct ahci_ioports *pp = (probe_ent-port[port]);
volatile u8 *port_mmio = (volatile u8 *)pp-port_mmio;
- u32 port_status;
+ u32 port_status, tf_data;
u32 mem;
+ int i = 0;
debug(Enter start port: %d\n, port);
port_status = readl(port_mmio + PORT_SCR_STAT);
@@ -564,6 +565,18 @@ static int ahci_port_start(u8 port)
PORT_CMD_POWER_ON | PORT_CMD_SPIN_UP |
PORT_CMD_START, port_mmio + PORT_CMD);
+ /*
+* Make sure interface is not busy based on error and status
+* information from task file data register before proceeding
+*/
+ while (i WAIT_MS_SPINUP) {
+ tf_data = readl(port_mmio + PORT_TFDATA);
+ if (!(tf_data ATA_BUSY))
+ break;
+ udelay(1000);
+ i++;
+ }
+
Doesn't this fall through after a timeout and fail to report an error?
As a matter of style I think something like the below is better
because it the timeout will be more accurate in the case where you
have a lot of processing each time around the loop.
static int wait_spinup(void)
{
ulong start;
start = get_timer(0);
do {
tf_data = ...;
if (!((tf_data ATA_BUSY))
return 0;
while (get_timer(start) WAIT_MS_SPINUP);
return -ETIMEDOUT;
}
Also how does this relate to the existing spinup delay code in ahci_host_init()?
debug(Exit start port %d\n, port);
return 0;
--
1.8.2.1
Regards,
Simon
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1] x86: ahci: Make sure interface is not busy after enabling the port
Hi Simon,
On Wed, Dec 24, 2014 at 3:42 AM, Simon Glass s...@chromium.org wrote:
Hi Bin,
On 23 December 2014 at 01:01, Bin Meng bmeng...@gmail.com wrote:
Each time U-Boot boots on Intel Crown Bay board, the displayed hard
drive information is wrong. It could be either wrong capacity or just
a 'Capacity: not available' message. After enabling the debug switch,
we can see the scsi inquiry command did not execute successfully.
However, doing a 'scsi scan' in the U-Boot shell does not expose
this issue.
This sounds like an error condition is not being propagated.
Actually an error condition not checked.
SCSI: Target spinup took 0 ms.
SATA link 1 timeout.
AHCI 0001.0100 32 slots 2 ports 3 Gbps 0x3 impl SATA mode
flags: ncq stag pm led clo only pmp pio slum part ccc apst
scanning bus for devices...
ahci_device_data_io: 0 byte transferred. --- scsi inquiry fails
ahci_device_data_io: 512 byte transferred.
ahci_device_data_io: 512 byte transferred.
ahci_device_data_io: 512 byte transferred.
Device 0: (0:0) Vendor: ATA Prod.: Rev: ?8
Type: Hard Disk
Capacity: 912968.3 MB = 891.5 GB (1869759264 x 512)
Found 1 device(s).
So uninitialized contents on the stack were passed to dev_print() to
display those wrong information.
The symptom were observed on two hard drives (one is Seagate, the
other one is Western Digital). The fix is to make sure the AHCI
interface is not busy by checking the error and status information
from task file register after enabling the port in ahci_port_start()
before proceeding other operations like scsi_scan().
Signed-off-by: Bin Meng bmeng...@gmail.com
---
drivers/block/ahci.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/block/ahci.c b/drivers/block/ahci.c
index c9a3beb..7ca8f40 100644
--- a/drivers/block/ahci.c
+++ b/drivers/block/ahci.c
@@ -505,8 +505,9 @@ static int ahci_port_start(u8 port)
{
struct ahci_ioports *pp = (probe_ent-port[port]);
volatile u8 *port_mmio = (volatile u8 *)pp-port_mmio;
- u32 port_status;
+ u32 port_status, tf_data;
u32 mem;
+ int i = 0;
debug(Enter start port: %d\n, port);
port_status = readl(port_mmio + PORT_SCR_STAT);
@@ -564,6 +565,18 @@ static int ahci_port_start(u8 port)
PORT_CMD_POWER_ON | PORT_CMD_SPIN_UP |
PORT_CMD_START, port_mmio + PORT_CMD);
+ /*
+* Make sure interface is not busy based on error and status
+* information from task file data register before proceeding
+*/
+ while (i WAIT_MS_SPINUP) {
+ tf_data = readl(port_mmio + PORT_TFDATA);
+ if (!(tf_data ATA_BUSY))
+ break;
+ udelay(1000);
+ i++;
+ }
+
Doesn't this fall through after a timeout and fail to report an error?
Ah, yes! We should return error code when timeout. But some other
routines in the scsi initialization path do no check return values,
like initr_scsi()-scsi_init()-scsi_low_level_init().
As a matter of style I think something like the below is better
because it the timeout will be more accurate in the case where you
have a lot of processing each time around the loop.
static int wait_spinup(void)
{
ulong start;
start = get_timer(0);
do {
tf_data = ...;
if (!((tf_data ATA_BUSY))
return 0;
while (get_timer(start) WAIT_MS_SPINUP);
return -ETIMEDOUT;
}
Looks like the original codes there are using i++ style for the
timeout check instead of get_timer().
Also how does this relate to the existing spinup delay code in
ahci_host_init()?
This seems to me they are not related. Per my understanding, the check
we need here is because we write something to the port command
register, but we missed the task file data busy indication.
writel_with_flush(PORT_CMD_ICC_ACTIVE | PORT_CMD_FIS_RX |
PORT_CMD_POWER_ON | PORT_CMD_SPIN_UP |
PORT_CMD_START, port_mmio + PORT_CMD);
[snip]
Regards,
Bin
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
