[RFC] add DMA setup FIS auto-activate feature

On Thu, Jul 23, 2009 at 02:48:08PM +0800, Jeff Garzik wrote:
> Tejun Heo wrote:
> > Jeff Garzik wrote:
> >> AA was added in SATA II, and the SATA II specs specifically mention that
> >> AA was not present in SATA 1.0.
> > 
> > Yeap, it wasn't in SATA 1.0 but it was in ahci 1.0 so _theoretically_
> > all ahcis should be fine with it.  We can introduce ATA_FLAG_FPDMA_AA
> > and let the drivers set it.
> 
> 
> Yep -- that was the logical conclusion of my rhetorical question at the 
> beginning of this thread ;-)
Something like below?

Add SATA DMA setup FIS auto-activate feature in AHCI.

Signed-off-by: Shaohua Li <shaohua.li@intel.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

On 07/23/2009 01:25 AM, Shaohua Li wrote:
> On Thu, Jul 23, 2009 at 02:48:08PM +0800, Jeff Garzik wrote:
>> Tejun Heo wrote:
>>> Jeff Garzik wrote:
>>>> AA was added in SATA II, and the SATA II specs specifically mention that
>>>> AA was not present in SATA 1.0.
>>> Yeap, it wasn't in SATA 1.0 but it was in ahci 1.0 so _theoretically_
>>> all ahcis should be fine with it.  We can introduce ATA_FLAG_FPDMA_AA
>>> and let the drivers set it.
>>
>> Yep -- that was the logical conclusion of my rhetorical question at the
>> beginning of this thread ;-)
> Something like below?
>
> Add SATA DMA setup FIS auto-activate feature in AHCI.
>
> Signed-off-by: Shaohua Li<shaohua.li@intel.com>
>
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 336eb1e..d5d67b8 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -2802,7 +2802,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>
>   	/* prepare host */
>   	if (hpriv->cap&  HOST_CAP_NCQ)
> -		pi.flags |= ATA_FLAG_NCQ;
> +		pi.flags |= ATA_FLAG_NCQ | ATA_FLAG_FPDMA_AA;
>
>   	if (hpriv->cap&  HOST_CAP_PMP)
>   		pi.flags |= ATA_FLAG_PMP;
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 2c6aeda..62355cf 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2303,6 +2303,7 @@ static void ata_dev_config_ncq(struct ata_device *dev,
>   {
>   	struct ata_port *ap = dev->link->ap;
>   	int hdepth = 0, ddepth = ata_id_queue_depth(dev->id);
> +	const u16 *id = dev->id;
>
>   	if (!ata_id_has_ncq(dev->id)) {
>   		desc[0] = '\0';
> @@ -2317,6 +2318,10 @@ static void ata_dev_config_ncq(struct ata_device *dev,
>   		dev->flags |= ATA_DFLAG_NCQ;
>   	}
>
> +	if ((ap->flags&  ATA_FLAG_FPDMA_AA)&&  ata_id_has_fpdma_aa(id))
> +		ata_dev_set_feature(dev, SETFEATURES_SATA_ENABLE,
> +			SATA_FPDMA_AA);
> +
>   	if (hdepth>= ddepth)
>   		snprintf(desc, desc_sz, "NCQ (depth %d)", ddepth);
>   	else
> diff --git a/include/linux/ata.h b/include/linux/ata.h
> index 9c75921..f549405 100644
> --- a/include/linux/ata.h
> +++ b/include/linux/ata.h
> @@ -306,6 +306,7 @@ enum {
>   	/* SETFEATURE Sector counts for SATA features */
>   	SATA_AN			= 0x05,  /* Asynchronous Notification */
>   	SATA_DIPM		= 0x03,  /* Device Initiated Power Management */
> +	SATA_FPDMA_AA		= 0x02,  /* DMA Setup FIS Auto-Activate */
>
>   	/* feature values for SET_MAX */
>   	ATA_SET_MAX_ADDR	= 0x00,
> @@ -525,6 +526,9 @@ static inline int ata_is_data(u8 prot)
>   #define ata_id_has_atapi_AN(id)	\
>   	( (((id)[76] != 0x0000)&&  ((id)[76] != 0xffff))&&  \
>   	  ((id)[78]&  (1<<  5)) )
> +#define ata_id_has_fpdma_aa(id)	\
> +	( (((id)[76] != 0x0000)&&  ((id)[76] != 0xffff))&&  \
> +	  ((id)[78]&  (1<<  2)) )
>   #define ata_id_iordy_disable(id) ((id)[ATA_ID_CAPABILITY]&  (1<<  10))
>   #define ata_id_has_iordy(id) ((id)[ATA_ID_CAPABILITY]&  (1<<  11))
>   #define ata_id_u32(id,n)	\
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 79b6d7f..ca210d8 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -190,6 +190,7 @@ enum {
>   	ATA_FLAG_NO_POWEROFF_SPINDOWN = (1<<  11), /* don't spindown before poweroff */
>   	ATA_FLAG_NO_HIBERNATE_SPINDOWN = (1<<  12), /* don't spindown before hibernation */
>   	ATA_FLAG_DEBUGMSG	= (1<<  13),
> +	ATA_FLAG_FPDMA_AA		= (1<<  14), /* driver supports Auto-Activate */
>   	ATA_FLAG_IGN_SIMPLEX	= (1<<  15), /* ignore SIMPLEX */
>   	ATA_FLAG_NO_IORDY	= (1<<  16), /* controller lacks iordy */
>   	ATA_FLAG_ACPI_SATA	= (1<<  17), /* need native SATA ACPI layout */

Seems reasonable to me, though we may want to print something out during 
device identification to indicate that we're using AA (like we do for 
ATAPI AN).
--
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

Robert Hancock wrote:
> Seems reasonable to me, though we may want to print something out during 
> device identification to indicate that we're using AA (like we do for 
> ATAPI AN).

Agreed -- the user needs some indication, somewhere, that this feature 
is enabled.

	Jeff


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

Jeff Garzik wrote:
> Robert Hancock wrote:
>> Seems reasonable to me, though we may want to print something out
>> during device identification to indicate that we're using AA (like we
>> do for ATAPI AN).
> 
> Agreed -- the user needs some indication, somewhere, that this feature
> is enabled.

Also, the return value from ata_dev_set_feature() needs to be checked.
AC_ERR_DEV can be ignored.  Other errors should probably set
inhinit-AA bit and fail the current recovery trial.

Thanks.