* [PATCH 1/1] libata: ahci_start_engine compliant to AHCI spec
@ 2011-04-19 21:44 Jian Peng
2011-04-21 13:40 ` Tejun Heo
0 siblings, 1 reply; 6+ messages in thread
From: Jian Peng @ 2011-04-19 21:44 UTC (permalink / raw)
To: Jeff Garzik, Tejun Heo; +Cc: linux-ide, linux-kernel, Jian Peng
At the end of section 10.1 of AHCI spec (rev 1.3), it states
Software shall not set PxCMD.ST to 1 until it is determined that a
functoinal device is present on the port as determined by
PxTFD.STS.BSY=0, PxTFD.STS.DRQ=0 and PxSSTS.DET=3h
Even though most AHCI host controller works without this additional
check, specific controller will fail without this.
Signed-off-by: Jian Peng <jipeng2005@gmail.com>
---
drivers/ata/libahci.c | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 26d4523..39e0698 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -539,6 +539,12 @@ void ahci_start_engine(struct ata_port *ap)
{
void __iomem *port_mmio = ahci_port_base(ap);
u32 tmp;
+ u8 status = readl(port_mmio + PORT_TFDATA) & 0xFF;
+
+ if (status & (ATA_BUSY | ATA_DRQ) ||
+ ahci_scr_read(&ap->link, SCR_STATUS, &tmp) ||
+ (tmp & 0x0f) != 0x03)
+ return;
/* start DMA */
tmp = readl(port_mmio + PORT_CMD);
--
1.7.4.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] libata: ahci_start_engine compliant to AHCI spec
2011-04-19 21:44 [PATCH 1/1] libata: ahci_start_engine compliant to AHCI spec Jian Peng
@ 2011-04-21 13:40 ` Tejun Heo
0 siblings, 0 replies; 6+ messages in thread
From: Tejun Heo @ 2011-04-21 13:40 UTC (permalink / raw)
To: Jian Peng; +Cc: Jeff Garzik, linux-ide, linux-kernel
On Tue, Apr 19, 2011 at 02:44:59PM -0700, Jian Peng wrote:
> At the end of section 10.1 of AHCI spec (rev 1.3), it states
>
> Software shall not set PxCMD.ST to 1 until it is determined that a
> functoinal device is present on the port as determined by
> PxTFD.STS.BSY=0, PxTFD.STS.DRQ=0 and PxSSTS.DET=3h
>
> Even though most AHCI host controller works without this additional
> check, specific controller will fail without this.
>
> Signed-off-by: Jian Peng <jipeng2005@gmail.com>
Acked-by: Tejun Heo <tj@kernel.org>
But some nitpicks below.
> + u8 status = readl(port_mmio + PORT_TFDATA) & 0xFF;
It usually isn't customary to initialize variable with a statement
which has side effect, especially hardware access. Please make the
readl a separate statement.
> + if (status & (ATA_BUSY | ATA_DRQ) ||
> + ahci_scr_read(&ap->link, SCR_STATUS, &tmp) ||
Also, ahci_scr_read() is known not to fail on host port in ahci and
it's customary to skip error return in such cases in low level
drivers.
> + (tmp & 0x0f) != 0x03)
> + return;
And please add comment explaining what condition is being checked.
It's much easier to read comment than digging through changelog and
find out what the code is trying to achieve.
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] libata: ahci_start_engine compliant to AHCI spec
@ 2011-04-23 6:58 Jian Peng
2011-04-23 7:08 ` Jeff Garzik
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jian Peng @ 2011-04-23 6:58 UTC (permalink / raw)
To: Jeff Garzik, Tejun Heo; +Cc: linux-ide, linux-kernel, Jian Peng
At the end of section 10.1 of AHCI spec (rev 1.3), it states
Software shall not set PxCMD.ST to 1 until it is determined that
a functoinal device is present on the port as determined by
PxTFD.STS.BSY=0, PxTFD.STS.DRQ=0 and PxSSTS.DET=3h
Even though most AHCI host controller works without this check,
specific controller will fail under this condition.
Signed-off-by: Jian Peng <jipeng2005@gmail.com>
---
drivers/ata/libahci.c | 21 +++++++++++++++++++++
1 files changed, 21 insertions(+), 0 deletions(-)
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 26d4523..83ed544 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -539,6 +539,27 @@ void ahci_start_engine(struct ata_port *ap)
{
void __iomem *port_mmio = ahci_port_base(ap);
u32 tmp;
+ u8 status;
+
+ status = readl(port_mmio + PORT_TFDATA) & 0xFF;
+
+ /*
+ * At end of section 10.1 of AHCI spec (rev 1.3), it states
+ * Software shall not set PxCMD.ST to 1 until it is determined
+ * that a functoinal device is present on the port as determined by
+ * PxTFD.STS.BSY=0, PxTFD.STS.DRQ=0 and PxSSTS.DET=3h
+ *
+ * Even though most AHCI host controllers work without this check,
+ * specific controller will fail under this condition
+ */
+ if (status & (ATA_BUSY | ATA_DRQ))
+ return;
+ else {
+ ahci_scr_read(&ap->link, SCR_STATUS, &tmp);
+
+ if ((tmp & 0xf) != 0x3)
+ return;
+ }
/* start DMA */
tmp = readl(port_mmio + PORT_CMD);
--
1.7.4.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] libata: ahci_start_engine compliant to AHCI spec
2011-04-23 6:58 Jian Peng
@ 2011-04-23 7:08 ` Jeff Garzik
2011-04-23 14:40 ` Tejun Heo
2011-04-25 10:26 ` Sergei Shtylyov
2 siblings, 0 replies; 6+ messages in thread
From: Jeff Garzik @ 2011-04-23 7:08 UTC (permalink / raw)
To: Jian Peng; +Cc: Tejun Heo, linux-ide, linux-kernel
On 04/23/2011 02:58 AM, Jian Peng wrote:
> At the end of section 10.1 of AHCI spec (rev 1.3), it states
>
> Software shall not set PxCMD.ST to 1 until it is determined that
> a functoinal device is present on the port as determined by
> PxTFD.STS.BSY=0, PxTFD.STS.DRQ=0 and PxSSTS.DET=3h
>
> Even though most AHCI host controller works without this check,
> specific controller will fail under this condition.
>
> Signed-off-by: Jian Peng<jipeng2005@gmail.com>
> ---
> drivers/ata/libahci.c | 21 +++++++++++++++++++++
> 1 files changed, 21 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 26d4523..83ed544 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -539,6 +539,27 @@ void ahci_start_engine(struct ata_port *ap)
> {
> void __iomem *port_mmio = ahci_port_base(ap);
> u32 tmp;
> + u8 status;
> +
> + status = readl(port_mmio + PORT_TFDATA)& 0xFF;
> +
> + /*
> + * At end of section 10.1 of AHCI spec (rev 1.3), it states
> + * Software shall not set PxCMD.ST to 1 until it is determined
> + * that a functoinal device is present on the port as determined by
> + * PxTFD.STS.BSY=0, PxTFD.STS.DRQ=0 and PxSSTS.DET=3h
> + *
> + * Even though most AHCI host controllers work without this check,
> + * specific controller will fail under this condition
> + */
> + if (status& (ATA_BUSY | ATA_DRQ))
> + return;
> + else {
> + ahci_scr_read(&ap->link, SCR_STATUS,&tmp);
> +
> + if ((tmp& 0xf) != 0x3)
> + return;
> + }
Looks good, I'll get this queued up...
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/1] libata: ahci_start_engine compliant to AHCI spec
2011-04-23 6:58 Jian Peng
2011-04-23 7:08 ` Jeff Garzik
@ 2011-04-23 14:40 ` Tejun Heo
2011-04-25 10:26 ` Sergei Shtylyov
2 siblings, 0 replies; 6+ messages in thread
From: Tejun Heo @ 2011-04-23 14:40 UTC (permalink / raw)
To: Jian Peng; +Cc: Jeff Garzik, linux-ide, linux-kernel
On Sat, Apr 23, 2011 at 8:58 AM, Jian Peng <jipeng2005@gmail.com> wrote:
> At the end of section 10.1 of AHCI spec (rev 1.3), it states
>
> Software shall not set PxCMD.ST to 1 until it is determined that
> a functoinal device is present on the port as determined by
> PxTFD.STS.BSY=0, PxTFD.STS.DRQ=0 and PxSSTS.DET=3h
>
> Even though most AHCI host controller works without this check,
> specific controller will fail under this condition.
>
> Signed-off-by: Jian Peng <jipeng2005@gmail.com>
Acked-by: Tejun Heo <tj@kernel.org>
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] libata: ahci_start_engine compliant to AHCI spec
2011-04-23 6:58 Jian Peng
2011-04-23 7:08 ` Jeff Garzik
2011-04-23 14:40 ` Tejun Heo
@ 2011-04-25 10:26 ` Sergei Shtylyov
2 siblings, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2011-04-25 10:26 UTC (permalink / raw)
To: Jian Peng; +Cc: Jeff Garzik, Tejun Heo, linux-ide, linux-kernel
Hello.
On 23-04-2011 10:58, Jian Peng wrote:
> At the end of section 10.1 of AHCI spec (rev 1.3), it states
> Software shall not set PxCMD.ST to 1 until it is determined that
> a functoinal device is present on the port as determined by
> PxTFD.STS.BSY=0, PxTFD.STS.DRQ=0 and PxSSTS.DET=3h
> Even though most AHCI host controller works without this check,
"Most" requires plural noun, no?
> specific controller will fail under this condition.
> Signed-off-by: Jian Peng<jipeng2005@gmail.com>
[...]
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 26d4523..83ed544 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -539,6 +539,27 @@ void ahci_start_engine(struct ata_port *ap)
> {
> void __iomem *port_mmio = ahci_port_base(ap);
> u32 tmp;
> + u8 status;
> +
> + status = readl(port_mmio + PORT_TFDATA) & 0xFF;
Masking is not necessary.
> +
> + /*
> + * At end of section 10.1 of AHCI spec (rev 1.3), it states
> + * Software shall not set PxCMD.ST to 1 until it is determined
> + * that a functoinal device is present on the port as determined by
> + * PxTFD.STS.BSY=0, PxTFD.STS.DRQ=0 and PxSSTS.DET=3h
> + *
> + * Even though most AHCI host controllers work without this check,
Same about grammar here.
> + * specific controller will fail under this condition
> + */
> + if (status& (ATA_BUSY | ATA_DRQ))
> + return;
> + else {
{} should be used on both branches, according to CodingStyle.
WBR, Sergei
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-04-25 10:27 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-04-19 21:44 [PATCH 1/1] libata: ahci_start_engine compliant to AHCI spec Jian Peng
2011-04-21 13:40 ` Tejun Heo
-- strict thread matches above, loose matches on Subject: below --
2011-04-23 6:58 Jian Peng
2011-04-23 7:08 ` Jeff Garzik
2011-04-23 14:40 ` Tejun Heo
2011-04-25 10:26 ` Sergei Shtylyov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).