* [RFC PATCH] AHCI: Workaround for ATAPI on buggy AHCI controller
@ 2013-04-12 19:34 Marc C
2013-04-12 20:58 ` Robin H. Johnson
2013-04-13 5:05 ` Robert Hancock
0 siblings, 2 replies; 4+ messages in thread
From: Marc C @ 2013-04-12 19:34 UTC (permalink / raw)
To: jeff; +Cc: linux-ide
Hello Jeff,
I'm working with a proprietary but AHCI-compatible SATA controller that uses
the libata driver. Early versions of this controller have issues with processing
non-data ATAPI commands. A workaround was identified which requires some
register pokes in the command completion path of the driver. I don't expect to
push this patch upstream (yet). However, I would like to get some feedback
regarding the workaround, and check if the placement of the code is "acceptable"
or if there would be a better place to put it.
The workaround itself is rather simple: toggle the START bit immediately after a
non-data ATAPI command completes. The ST bit toggle would be performed within
atomic context in the AHCI port interrupt ISR.
Signed-off-by: Marc C <marc.ceeeee@gmail.com>
---
linux/drivers/ata/libahci.c | 26 ++++++++++++++++++++++++++
linux/drivers/ata/libata-core.c | 6 ++++++
2 files changed, 32 insertions(+)
diff --git a/linux/drivers/ata/libahci.c b/linux/drivers/ata/libahci.c
index f9eaa82..20b3a9a 100644
--- a/linux/drivers/ata/libahci.c
+++ b/linux/drivers/ata/libahci.c
@@ -2215,6 +2215,32 @@ void ahci_set_em_messages(struct ahci_host_priv *hpriv,
}
EXPORT_SYMBOL_GPL(ahci_set_em_messages);
+void ahci_apply_atapi_workaround(struct ata_queued_cmd *qc)
+{
+ if (!(qc->flags & ATA_QCFLAG_DMAMAP)) {
+ /*
+ * Non-data ATAPI commands need to kick the DMA engine
+ */
+ struct ata_port *ap = qc->ap;
+ void __iomem *port_mmio = ahci_port_base(ap);
+ u32 tmp = readl(port_mmio + PORT_CMD);
+
+ /* check if the HBA is idle */
+ if ((tmp & (PORT_CMD_START | PORT_CMD_LIST_ON)) != 0) {
+ /* setting HBA to idle */
+ tmp &= ~PORT_CMD_START;
+ writel(tmp, port_mmio + PORT_CMD);
+
+ /* wait for engine to stop */
+ do {
+ tmp = readl(port_mmio + PORT_CMD);
+ } while (tmp & PORT_CMD_LIST_ON);
+ }
+ ahci_start_engine(ap);
+ }
+}
+EXPORT_SYMBOL_GPL(ahci_apply_atapi_workaround);
+
MODULE_AUTHOR("Jeff Garzik");
MODULE_DESCRIPTION("Common AHCI SATA low-level routines");
MODULE_LICENSE("GPL");
diff --git a/linux/drivers/ata/libata-core.c b/linux/drivers/ata/libata-core.c
index 4e73e1b..383de21 100644
--- a/linux/drivers/ata/libata-core.c
+++ b/linux/drivers/ata/libata-core.c
@@ -4827,6 +4827,12 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc->ap;
+ if (ata_is_atapi(qc->tf.protocol)) {
+ void ahci_apply_atapi_workaround(struct ata_queued_cmd *qc);
+ ahci_apply_atapi_workaround(qc);
+ }
+
/* XXX: New EH and old EH use different mechanisms to
* synchronize EH with regular execution path.
*
--
1.7.9.5
--
Regards,
Marc
marc.ceeeee@gmail.com
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [RFC PATCH] AHCI: Workaround for ATAPI on buggy AHCI controller
2013-04-12 19:34 [RFC PATCH] AHCI: Workaround for ATAPI on buggy AHCI controller Marc C
@ 2013-04-12 20:58 ` Robin H. Johnson
2013-04-13 5:05 ` Robert Hancock
1 sibling, 0 replies; 4+ messages in thread
From: Robin H. Johnson @ 2013-04-12 20:58 UTC (permalink / raw)
To: Marc C, linux-ide
On Fri, Apr 12, 2013 at 12:34:24PM -0700, Marc C wrote:
> Hello Jeff,
..
> index 4e73e1b..383de21 100644
> --- a/linux/drivers/ata/libata-core.c
> +++ b/linux/drivers/ata/libata-core.c
> @@ -4827,6 +4827,12 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
> {
> struct ata_port *ap = qc->ap;
>
> + if (ata_is_atapi(qc->tf.protocol)) {
> + void ahci_apply_atapi_workaround(struct ata_queued_cmd *qc);
This prototype really doesn't belong here.
Should probably be in include/linux/libata.h
--
Robin Hugh Johnson
Gentoo Linux: Developer, Trustee & Infrastructure Lead
E-Mail : robbat2@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [RFC PATCH] AHCI: Workaround for ATAPI on buggy AHCI controller
2013-04-12 19:34 [RFC PATCH] AHCI: Workaround for ATAPI on buggy AHCI controller Marc C
2013-04-12 20:58 ` Robin H. Johnson
@ 2013-04-13 5:05 ` Robert Hancock
2013-04-14 1:21 ` Jeff Garzik
1 sibling, 1 reply; 4+ messages in thread
From: Robert Hancock @ 2013-04-13 5:05 UTC (permalink / raw)
To: Marc C; +Cc: jeff, linux-ide
On 04/12/2013 01:34 PM, Marc C wrote:
> Hello Jeff,
>
> I'm working with a proprietary but AHCI-compatible SATA controller that uses
> the libata driver. Early versions of this controller have issues with processing
> non-data ATAPI commands. A workaround was identified which requires some
> register pokes in the command completion path of the driver. I don't expect to
> push this patch upstream (yet). However, I would like to get some feedback
> regarding the workaround, and check if the placement of the code is "acceptable"
> or if there would be a better place to put it.
>
> The workaround itself is rather simple: toggle the START bit immediately after a
> non-data ATAPI command completes. The ST bit toggle would be performed within
> atomic context in the AHCI port interrupt ISR.
>
> Signed-off-by: Marc C <marc.ceeeee@gmail.com>
This should likely be triggered off a host flag like some of the ones
already in the driver so it can be done just for the affected
controllers, not unconditionally.
> ---
> linux/drivers/ata/libahci.c | 26 ++++++++++++++++++++++++++
> linux/drivers/ata/libata-core.c | 6 ++++++
> 2 files changed, 32 insertions(+)
>
> diff --git a/linux/drivers/ata/libahci.c b/linux/drivers/ata/libahci.c
> index f9eaa82..20b3a9a 100644
> --- a/linux/drivers/ata/libahci.c
> +++ b/linux/drivers/ata/libahci.c
> @@ -2215,6 +2215,32 @@ void ahci_set_em_messages(struct ahci_host_priv *hpriv,
> }
> EXPORT_SYMBOL_GPL(ahci_set_em_messages);
>
> +void ahci_apply_atapi_workaround(struct ata_queued_cmd *qc)
> +{
> + if (!(qc->flags & ATA_QCFLAG_DMAMAP)) {
> + /*
> + * Non-data ATAPI commands need to kick the DMA engine
> + */
> + struct ata_port *ap = qc->ap;
> + void __iomem *port_mmio = ahci_port_base(ap);
> + u32 tmp = readl(port_mmio + PORT_CMD);
> +
> + /* check if the HBA is idle */
> + if ((tmp & (PORT_CMD_START | PORT_CMD_LIST_ON)) != 0) {
> + /* setting HBA to idle */
> + tmp &= ~PORT_CMD_START;
> + writel(tmp, port_mmio + PORT_CMD);
> +
> + /* wait for engine to stop */
> + do {
> + tmp = readl(port_mmio + PORT_CMD);
> + } while (tmp & PORT_CMD_LIST_ON);
> + }
> + ahci_start_engine(ap);
> + }
> +}
> +EXPORT_SYMBOL_GPL(ahci_apply_atapi_workaround);
> +
> MODULE_AUTHOR("Jeff Garzik");
> MODULE_DESCRIPTION("Common AHCI SATA low-level routines");
> MODULE_LICENSE("GPL");
> diff --git a/linux/drivers/ata/libata-core.c b/linux/drivers/ata/libata-core.c
> index 4e73e1b..383de21 100644
> --- a/linux/drivers/ata/libata-core.c
> +++ b/linux/drivers/ata/libata-core.c
> @@ -4827,6 +4827,12 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
> {
> struct ata_port *ap = qc->ap;
>
> + if (ata_is_atapi(qc->tf.protocol)) {
> + void ahci_apply_atapi_workaround(struct ata_queued_cmd *qc);
> + ahci_apply_atapi_workaround(qc);
> + }
> +
> /* XXX: New EH and old EH use different mechanisms to
> * synchronize EH with regular execution path.
> *
> --
> 1.7.9.5
>
>
>
>
> --
> Regards,
> Marc
> marc.ceeeee@gmail.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [RFC PATCH] AHCI: Workaround for ATAPI on buggy AHCI controller
2013-04-13 5:05 ` Robert Hancock
@ 2013-04-14 1:21 ` Jeff Garzik
0 siblings, 0 replies; 4+ messages in thread
From: Jeff Garzik @ 2013-04-14 1:21 UTC (permalink / raw)
To: Robert Hancock; +Cc: Marc C, jeff, linux-ide
On 04/13/2013 01:05 AM, Robert Hancock wrote:
> On 04/12/2013 01:34 PM, Marc C wrote:
>> Hello Jeff,
>>
>> I'm working with a proprietary but AHCI-compatible SATA controller
>> that uses
>> the libata driver. Early versions of this controller have issues with
>> processing
>> non-data ATAPI commands. A workaround was identified which requires some
>> register pokes in the command completion path of the driver. I don't
>> expect to
>> push this patch upstream (yet). However, I would like to get some
>> feedback
>> regarding the workaround, and check if the placement of the code is
>> "acceptable"
>> or if there would be a better place to put it.
>>
>> The workaround itself is rather simple: toggle the START bit
>> immediately after a
>> non-data ATAPI command completes. The ST bit toggle would be performed
>> within
>> atomic context in the AHCI port interrupt ISR.
>>
>> Signed-off-by: Marc C <marc.ceeeee@gmail.com>
>
> This should likely be triggered off a host flag like some of the ones
> already in the driver so it can be done just for the affected
> controllers, not unconditionally.
Indeed -- you don't want to punish the non-buggy 99% case for one weird
buggy hardware rev.
See AHCI_HFLAG_xxx
Jeff
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-04-14 1:21 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-04-12 19:34 [RFC PATCH] AHCI: Workaround for ATAPI on buggy AHCI controller Marc C
2013-04-12 20:58 ` Robin H. Johnson
2013-04-13 5:05 ` Robert Hancock
2013-04-14 1:21 ` Jeff Garzik
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).