[PATCH v4 1/2] ahci: Rename flag `ATA_LFLAG_NO_DB_DELAY` to `ATA_LFLAG_NO_DEBOUNCE_DELAY`

[PATCH v4 1/2] ahci: Rename flag `ATA_LFLAG_NO_DB_DELAY` to `ATA_LFLAG_NO_DEBOUNCE_DELAY`

[Paul Menzel]

The new name is longer, but clearer.

Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
---
 drivers/ata/ahci_brcm.c   | 2 +-
 drivers/ata/libata-sata.c | 2 +-
 include/linux/libata.h    | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
index 6e9c5ade4c2ea..649815c196ed0 100644
--- a/drivers/ata/ahci_brcm.c
+++ b/drivers/ata/ahci_brcm.c
@@ -333,7 +333,7 @@ static struct ata_port_operations ahci_brcm_platform_ops = {
 
 static const struct ata_port_info ahci_brcm_port_info = {
 	.flags		= AHCI_FLAG_COMMON | ATA_FLAG_NO_DIPM,
-	.link_flags	= ATA_LFLAG_NO_DB_DELAY,
+	.link_flags	= ATA_LFLAG_NO_DEBOUNCE_DELAY,
 	.pio_mask	= ATA_PIO4,
 	.udma_mask	= ATA_UDMA6,
 	.port_ops	= &ahci_brcm_platform_ops,
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
index b9c77885b8726..67b2e7cf3cc4e 100644
--- a/drivers/ata/libata-sata.c
+++ b/drivers/ata/libata-sata.c
@@ -317,7 +317,7 @@ int sata_link_resume(struct ata_link *link, const unsigned long *params,
 		 * immediately after resuming.  Delay 200ms before
 		 * debouncing.
 		 */
-		if (!(link->flags & ATA_LFLAG_NO_DB_DELAY))
+		if (!(link->flags & ATA_LFLAG_NO_DEBOUNCE_DELAY))
 			ata_msleep(link->ap, 200);
 
 		/* is SControl restored correctly? */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 2a8404b26083c..15802e644962d 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -191,7 +191,7 @@ enum {
 	ATA_LFLAG_NO_LPM	= (1 << 8), /* disable LPM on this link */
 	ATA_LFLAG_RST_ONCE	= (1 << 9), /* limit recovery to one reset */
 	ATA_LFLAG_CHANGED	= (1 << 10), /* LPM state changed on this link */
-	ATA_LFLAG_NO_DB_DELAY	= (1 << 11), /* no debounce delay on link resume */
+	ATA_LFLAG_NO_DEBOUNCE_DELAY = (1 << 11), /* no debounce delay on link resume */
 
 	/* struct ata_port flags */
 	ATA_FLAG_SLAVE_POSS	= (1 << 0), /* host supports slave dev */
-- 
2.30.2

[PATCH v4 2/2] ahci: AMD A85 FCH (Hudson D4): Skip 200 ms debounce delay in `sata_link_resume()`

[Paul Menzel]

Commit 4effb658a0 from October 2003 [1, historical git archive] with the
commit message

> [libata] Merge Serial ATA core, and drivers for:
>
> Intel ICH5 (production)
> ServerWorks / Apple K2 (beta)
> VIA (beta)
> Silicon Image 3112 (broken!)
> Various Promise (alpha/beta)

adds the code below:

    void sata_phy_reset(struct ata_port *ap)
    {
    […]
        /* wait for phy to become ready, if necessary */
        do {
            msleep(200);
            sstatus = scr_read(ap, SCR_STATUS);
            if ((sstatus & 0xf) != 1)
                break;
        } while (time_before(jiffies, timeout));
    […]
    }

Later on in commit d7bb4cc75759 ([PATCH] libata-hp-prep: implement
sata_phy_debounce()) the commit is refactored [2], and the comment
clarified.

     /*
      * Writes to SControl sometimes get ignored under certain
      * controllers (ata_piix SIDPR).  Make sure DET actually is
      * cleared.
      */
     do {
             scontrol = (scontrol & 0x0f0) | 0x300;
             if ((rc = sata_scr_write(link, SCR_CONTROL, scontrol)))
                     return rc;
             /*
              * Some PHYs react badly if SStatus is pounded
              * immediately after resuming.  Delay 200ms before
              * debouncing.
              */
             if (!(link->flags & ATA_LFLAG_NO_DEBOUNCE_DELAY))
                     ata_msleep(link->ap, 200);

             /* is SControl restored correctly? */
             if ((rc = sata_scr_read(link, SCR_CONTROL, &scontrol)))
                     return rc;
     } while ((scontrol & 0xf0f) != 0x300 && --tries);

A lot of PHYs do not need a delay though, so delaying 200 ms increases
the boot time by 30 percent unnecessarily for a lot of systems, making
“instant booting” quite hard.

As it’s unknown for what PHY the delay was added, create a new board
`board_ahci_no_debounce_delay` with the link flag
`ATA_LFLAG_NO_DEBOUNCE_DELAY,`, and, for now, configure the AMD A85 FCH
(Hudson D4) to use it.

On the ASUS F2A85-M PRO it reduces the Linux kernel boot time by the
expected 200 ms from 787 ms to 585 ms.

Tested on ASUS F2A85-M PRO:

Without patch, i. e., with 200 ms debounce delay:

    […]
    [    0.000000] DMI: ASUS F2A85-M_PRO/F2A85-M_PRO, BIOS 4.15-671-g7b043ef855 12/27/2021
    […]
    [    0.404885] ahci 0000:00:11.0: version 3.0
    [    0.405466] ahci 0000:00:11.0: AHCI 0001.0300 32 slots 8 ports 6 Gbps 0x40 impl SATA mode
    [    0.405470] ahci 0000:00:11.0: flags: 64bit ncq sntf ilck led clo pio
    [    0.408036] scsi host0: ahci
    [    0.408537] scsi host1: ahci
    [    0.408932] scsi host2: ahci
    [    0.409444] scsi host3: ahci
    [    0.409841] scsi host4: ahci
    [    0.410266] scsi host5: ahci
    [    0.410661] scsi host6: ahci
    [    0.411052] scsi host7: ahci
    [    0.411284] ata1: DUMMY
    [    0.411286] ata2: DUMMY
    [    0.411286] ata3: DUMMY
    [    0.411287] ata4: DUMMY
    [    0.411288] ata5: DUMMY
    [    0.411289] ata6: DUMMY
    [    0.411291] ata7: SATA max UDMA/133 abar m2048@0xf01cc000 port 0xf01cc400 irq 19
    [    0.411292] ata8: DUMMY
    […]
    [    0.422362] Key type encrypted registered
    [    0.424903] PM:   Magic number: 1:28:636
    [    0.723979] ata7: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
    [    0.724268] ata7.00: ATA-9: SanDisk SDSSDP064G, 2.0.0, max UDMA/133
    [    0.724271] ata7.00: 125045424 sectors, multi 1: LBA48 NCQ (depth 32)
    [    0.725537] ata7.00: configured for UDMA/133
    [    0.725898] scsi 6:0:0:0: Direct-Access     ATA      SanDisk SDSSDP06 0    PQ: 0 ANSI: 5
    [    0.726428] sd 6:0:0:0: [sda] 125045424 512-byte logical blocks: (64.0 GB/59.6 GiB)
    [    0.726442] sd 6:0:0:0: [sda] Write Protect is off
    [    0.726446] sd 6:0:0:0: [sda] Mode Sense: 00 3a 00 00
    [    0.726464] sd 6:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
    [    0.727985]  sda: sda1 sda2 sda3
    [    0.728588] sd 6:0:0:0: [sda] Attached SCSI disk
    [    0.738495] EXT4-fs (sda3): mounted filesystem with ordered data mode. Opts: (null). Quota mode: none.
    […]
    [    0.786812] Run /sbin/init as init process

With patch, i. e., skipping the debounce delay saves 200 ms from the boot
as expected.

    […]
    [    0.000000] DMI: ASUS F2A85-M_PRO/F2A85-M_PRO, BIOS 4.15-671-g7b043ef855 12/27/2021
    […]
    [    0.407372] ahci 0000:00:11.0: version 3.0
    [    0.407909] ahci 0000:00:11.0: AHCI 0001.0300 32 slots 8 ports 6 Gbps 0x40 impl SATA mode
    [    0.407913] ahci 0000:00:11.0: flags: 64bit ncq sntf ilck led clo pio
    [    0.410520] scsi host0: ahci
    [    0.411017] scsi host1: ahci
    [    0.411418] scsi host2: ahci
    [    0.411810] scsi host3: ahci
    [    0.412225] scsi host4: ahci
    [    0.412614] scsi host5: ahci
    [    0.413005] scsi host6: ahci
    [    0.413488] scsi host7: ahci
    [    0.413713] ata1: DUMMY
    [    0.413715] ata2: DUMMY
    [    0.413716] ata3: DUMMY
    [    0.413716] ata4: DUMMY
    [    0.413717] ata5: DUMMY
    [    0.413718] ata6: DUMMY
    [    0.413720] ata7: SATA max UDMA/133 abar m2048@0xf01cc000 port 0xf01cc400 irq 19
    [    0.413722] ata8: DUMMY
    […]
    [    0.425414] Key type encrypted registered
    [    0.427873] PM:   Magic number: 1:234:838
    [    0.522131] ata7: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
    [    0.522415] ata7.00: ATA-9: SanDisk SDSSDP064G, 2.0.0, max UDMA/133
    [    0.522418] ata7.00: 125045424 sectors, multi 1: LBA48 NCQ (depth 32)
    [    0.523636] ata7.00: configured for UDMA/133
    [    0.523993] scsi 6:0:0:0: Direct-Access     ATA      SanDisk SDSSDP06 0    PQ: 0 ANSI: 5
    [    0.524497] sd 6:0:0:0: [sda] 125045424 512-byte logical blocks: (64.0 GB/59.6 GiB)
    [    0.524511] sd 6:0:0:0: [sda] Write Protect is off
    [    0.524515] sd 6:0:0:0: [sda] Mode Sense: 00 3a 00 00
    [    0.524534] sd 6:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
    [    0.525953]  sda: sda1 sda2 sda3
    [    0.526541] sd 6:0:0:0: [sda] Attached SCSI disk
    [    0.536245] EXT4-fs (sda3): mounted filesystem with ordered data mode. Opts: (null). Quota mode: none.
    […]
    [    0.585327] Run /sbin/init as init process

[1]: https://git.kernel.org/pub/scm/linux/kernel/git/history/history.git/commit/?id=4effb658a0f800e159c29a2d881cac76c326087a
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d7bb4cc7575929a60b0a718daa1bce87bea9a9cc

Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
Cc: Tejun Heo <tj@kernel.org>
---
v4:
1.  Use 0x7801 instead of macros
2.  Use clearer name `no_debounce_delay` over `nodbdelay`
3.  Provide more history in commit message

 drivers/ata/ahci.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 1e1167e725a40..836b763c640fc 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -51,6 +51,7 @@ enum board_ids {
 	board_ahci,
 	board_ahci_ign_iferr,
 	board_ahci_mobile,
+	board_ahci_no_debounce_delay,
 	board_ahci_nomsi,
 	board_ahci_noncq,
 	board_ahci_nosntf,
@@ -141,6 +142,13 @@ static const struct ata_port_info ahci_port_info[] = {
 		.udma_mask	= ATA_UDMA6,
 		.port_ops	= &ahci_ops,
 	},
+	[board_ahci_no_debounce_delay] = {
+		.flags		= AHCI_FLAG_COMMON,
+		.link_flags	= ATA_LFLAG_NO_DEBOUNCE_DELAY,
+		.pio_mask	= ATA_PIO4,
+		.udma_mask	= ATA_UDMA6,
+		.port_ops	= &ahci_ops,
+	},
 	[board_ahci_nomsi] = {
 		AHCI_HFLAGS	(AHCI_HFLAG_NO_MSI),
 		.flags		= AHCI_FLAG_COMMON,
@@ -437,6 +445,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 		board_ahci_al },
 	/* AMD */
 	{ PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
+	{ PCI_VDEVICE(AMD, 0x7801), board_ahci_no_debounce_delay }, /* AMD Hudson-2 (AHCI mode) */
 	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
 	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */
 	/* AMD is using RAID class only for ahci controllers */
-- 
2.30.2

Re: [PATCH v4 1/2] ahci: Rename flag `ATA_LFLAG_NO_DB_DELAY` to `ATA_LFLAG_NO_DEBOUNCE_DELAY`

[Damien Le Moal]

On 1/6/22 00:36, Paul Menzel wrote:
> The new name is longer, but clearer.

It looks like you sent an incremental change on top of your previous
patches. Please send a cleaned up series. One patch to introduce the
ATA_LFLAG_NO_DEBOUNCE_DELAY flag & msleep bypass and another to add the
AMD chipset.

The commit message of this version patch 2 should be the explanation for
the ATA_LFLAG_NO_DEBOUNCE_DELAY flag, not for adding a new chipset ID.

> 
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
> ---
>  drivers/ata/ahci_brcm.c   | 2 +-
>  drivers/ata/libata-sata.c | 2 +-
>  include/linux/libata.h    | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
> index 6e9c5ade4c2ea..649815c196ed0 100644
> --- a/drivers/ata/ahci_brcm.c
> +++ b/drivers/ata/ahci_brcm.c
> @@ -333,7 +333,7 @@ static struct ata_port_operations ahci_brcm_platform_ops = {
>  
>  static const struct ata_port_info ahci_brcm_port_info = {
>  	.flags		= AHCI_FLAG_COMMON | ATA_FLAG_NO_DIPM,
> -	.link_flags	= ATA_LFLAG_NO_DB_DELAY,
> +	.link_flags	= ATA_LFLAG_NO_DEBOUNCE_DELAY,
>  	.pio_mask	= ATA_PIO4,
>  	.udma_mask	= ATA_UDMA6,
>  	.port_ops	= &ahci_brcm_platform_ops,
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index b9c77885b8726..67b2e7cf3cc4e 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -317,7 +317,7 @@ int sata_link_resume(struct ata_link *link, const unsigned long *params,
>  		 * immediately after resuming.  Delay 200ms before
>  		 * debouncing.
>  		 */
> -		if (!(link->flags & ATA_LFLAG_NO_DB_DELAY))
> +		if (!(link->flags & ATA_LFLAG_NO_DEBOUNCE_DELAY))
>  			ata_msleep(link->ap, 200);
>  
>  		/* is SControl restored correctly? */
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 2a8404b26083c..15802e644962d 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -191,7 +191,7 @@ enum {
>  	ATA_LFLAG_NO_LPM	= (1 << 8), /* disable LPM on this link */
>  	ATA_LFLAG_RST_ONCE	= (1 << 9), /* limit recovery to one reset */
>  	ATA_LFLAG_CHANGED	= (1 << 10), /* LPM state changed on this link */
> -	ATA_LFLAG_NO_DB_DELAY	= (1 << 11), /* no debounce delay on link resume */
> +	ATA_LFLAG_NO_DEBOUNCE_DELAY = (1 << 11), /* no debounce delay on link resume */
>  
>  	/* struct ata_port flags */
>  	ATA_FLAG_SLAVE_POSS	= (1 << 0), /* host supports slave dev */


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 1/2] ahci: Rename flag `ATA_LFLAG_NO_DB_DELAY` to `ATA_LFLAG_NO_DEBOUNCE_DELAY`

[Paul Menzel]

Dear Damien,


Am 06.01.22 um 03:52 schrieb Damien Le Moal:
> On 1/6/22 00:36, Paul Menzel wrote:
>> The new name is longer, but clearer.
> 
> It looks like you sent an incremental change on top of your previous
> patches. Please send a cleaned up series. One patch to introduce the
> ATA_LFLAG_NO_DEBOUNCE_DELAY flag & msleep bypass and another to add the
> AMD chipset.
> 
> The commit message of this version patch 2 should be the explanation for
> the ATA_LFLAG_NO_DEBOUNCE_DELAY flag, not for adding a new chipset ID.

Sorry, but I think you misread the patch. The flag 
`ATA_LFLAG_NO_DB_DELAY` exists since commit e39b2bb3b7 (libata: skip 
debounce delay on link resume).


Kind regards,

Paul


>> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
>> ---
>>   drivers/ata/ahci_brcm.c   | 2 +-
>>   drivers/ata/libata-sata.c | 2 +-
>>   include/linux/libata.h    | 2 +-
>>   3 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
>> index 6e9c5ade4c2ea..649815c196ed0 100644
>> --- a/drivers/ata/ahci_brcm.c
>> +++ b/drivers/ata/ahci_brcm.c
>> @@ -333,7 +333,7 @@ static struct ata_port_operations ahci_brcm_platform_ops = {
>>   
>>   static const struct ata_port_info ahci_brcm_port_info = {
>>   	.flags		= AHCI_FLAG_COMMON | ATA_FLAG_NO_DIPM,
>> -	.link_flags	= ATA_LFLAG_NO_DB_DELAY,
>> +	.link_flags	= ATA_LFLAG_NO_DEBOUNCE_DELAY,
>>   	.pio_mask	= ATA_PIO4,
>>   	.udma_mask	= ATA_UDMA6,
>>   	.port_ops	= &ahci_brcm_platform_ops,
>> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
>> index b9c77885b8726..67b2e7cf3cc4e 100644
>> --- a/drivers/ata/libata-sata.c
>> +++ b/drivers/ata/libata-sata.c
>> @@ -317,7 +317,7 @@ int sata_link_resume(struct ata_link *link, const unsigned long *params,
>>   		 * immediately after resuming.  Delay 200ms before
>>   		 * debouncing.
>>   		 */
>> -		if (!(link->flags & ATA_LFLAG_NO_DB_DELAY))
>> +		if (!(link->flags & ATA_LFLAG_NO_DEBOUNCE_DELAY))
>>   			ata_msleep(link->ap, 200);
>>   
>>   		/* is SControl restored correctly? */
>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>> index 2a8404b26083c..15802e644962d 100644
>> --- a/include/linux/libata.h
>> +++ b/include/linux/libata.h
>> @@ -191,7 +191,7 @@ enum {
>>   	ATA_LFLAG_NO_LPM	= (1 << 8), /* disable LPM on this link */
>>   	ATA_LFLAG_RST_ONCE	= (1 << 9), /* limit recovery to one reset */
>>   	ATA_LFLAG_CHANGED	= (1 << 10), /* LPM state changed on this link */
>> -	ATA_LFLAG_NO_DB_DELAY	= (1 << 11), /* no debounce delay on link resume */
>> +	ATA_LFLAG_NO_DEBOUNCE_DELAY = (1 << 11), /* no debounce delay on link resume */
>>   
>>   	/* struct ata_port flags */
>>   	ATA_FLAG_SLAVE_POSS	= (1 << 0), /* host supports slave dev */
> 
> 

Re: [PATCH v4 1/2] ahci: Rename flag `ATA_LFLAG_NO_DB_DELAY` to `ATA_LFLAG_NO_DEBOUNCE_DELAY`

[Damien Le Moal]

On 1/6/22 16:35, Paul Menzel wrote:
> Dear Damien,
> 
> 
> Am 06.01.22 um 03:52 schrieb Damien Le Moal:
>> On 1/6/22 00:36, Paul Menzel wrote:
>>> The new name is longer, but clearer.
>>
>> It looks like you sent an incremental change on top of your previous
>> patches. Please send a cleaned up series. One patch to introduce the
>> ATA_LFLAG_NO_DEBOUNCE_DELAY flag & msleep bypass and another to add the
>> AMD chipset.
>>
>> The commit message of this version patch 2 should be the explanation for
>> the ATA_LFLAG_NO_DEBOUNCE_DELAY flag, not for adding a new chipset ID.
> 
> Sorry, but I think you misread the patch. The flag 
> `ATA_LFLAG_NO_DB_DELAY` exists since commit e39b2bb3b7 (libata: skip 
> debounce delay on link resume).

Arg. Indeed. Ignore my comment :)

Let's wait to see if there are any more comments and I will queue this
up for 5.17.

Going forward, it would be interesting to work on reversing the default:
no delay by default and mark all old & unknown adapters as needing the
delay. As you mentioned, most modern AHCI adapters most likely do not
need it.

Are you willing to take this and send a patches ?

> 
> 
> Kind regards,
> 
> Paul
> 
> 
>>> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
>>> ---
>>>   drivers/ata/ahci_brcm.c   | 2 +-
>>>   drivers/ata/libata-sata.c | 2 +-
>>>   include/linux/libata.h    | 2 +-
>>>   3 files changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
>>> index 6e9c5ade4c2ea..649815c196ed0 100644
>>> --- a/drivers/ata/ahci_brcm.c
>>> +++ b/drivers/ata/ahci_brcm.c
>>> @@ -333,7 +333,7 @@ static struct ata_port_operations ahci_brcm_platform_ops = {
>>>   
>>>   static const struct ata_port_info ahci_brcm_port_info = {
>>>   	.flags		= AHCI_FLAG_COMMON | ATA_FLAG_NO_DIPM,
>>> -	.link_flags	= ATA_LFLAG_NO_DB_DELAY,
>>> +	.link_flags	= ATA_LFLAG_NO_DEBOUNCE_DELAY,
>>>   	.pio_mask	= ATA_PIO4,
>>>   	.udma_mask	= ATA_UDMA6,
>>>   	.port_ops	= &ahci_brcm_platform_ops,
>>> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
>>> index b9c77885b8726..67b2e7cf3cc4e 100644
>>> --- a/drivers/ata/libata-sata.c
>>> +++ b/drivers/ata/libata-sata.c
>>> @@ -317,7 +317,7 @@ int sata_link_resume(struct ata_link *link, const unsigned long *params,
>>>   		 * immediately after resuming.  Delay 200ms before
>>>   		 * debouncing.
>>>   		 */
>>> -		if (!(link->flags & ATA_LFLAG_NO_DB_DELAY))
>>> +		if (!(link->flags & ATA_LFLAG_NO_DEBOUNCE_DELAY))
>>>   			ata_msleep(link->ap, 200);
>>>   
>>>   		/* is SControl restored correctly? */
>>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>>> index 2a8404b26083c..15802e644962d 100644
>>> --- a/include/linux/libata.h
>>> +++ b/include/linux/libata.h
>>> @@ -191,7 +191,7 @@ enum {
>>>   	ATA_LFLAG_NO_LPM	= (1 << 8), /* disable LPM on this link */
>>>   	ATA_LFLAG_RST_ONCE	= (1 << 9), /* limit recovery to one reset */
>>>   	ATA_LFLAG_CHANGED	= (1 << 10), /* LPM state changed on this link */
>>> -	ATA_LFLAG_NO_DB_DELAY	= (1 << 11), /* no debounce delay on link resume */
>>> +	ATA_LFLAG_NO_DEBOUNCE_DELAY = (1 << 11), /* no debounce delay on link resume */
>>>   
>>>   	/* struct ata_port flags */
>>>   	ATA_FLAG_SLAVE_POSS	= (1 << 0), /* host supports slave dev */
>>
>>


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v4 1/2] ahci: Rename flag `ATA_LFLAG_NO_DB_DELAY` to `ATA_LFLAG_NO_DEBOUNCE_DELAY`

[Paul Menzel]

Dear Damien,


Am 06.01.22 um 08:55 schrieb Damien Le Moal:
> On 1/6/22 16:35, Paul Menzel wrote:

>> Am 06.01.22 um 03:52 schrieb Damien Le Moal:
>>> On 1/6/22 00:36, Paul Menzel wrote:
>>>> The new name is longer, but clearer.
>>>
>>> It looks like you sent an incremental change on top of your previous
>>> patches. Please send a cleaned up series. One patch to introduce the
>>> ATA_LFLAG_NO_DEBOUNCE_DELAY flag & msleep bypass and another to add the
>>> AMD chipset.
>>>
>>> The commit message of this version patch 2 should be the explanation for
>>> the ATA_LFLAG_NO_DEBOUNCE_DELAY flag, not for adding a new chipset ID.
>>
>> Sorry, but I think you misread the patch. The flag
>> `ATA_LFLAG_NO_DB_DELAY` exists since commit e39b2bb3b7 (libata: skip
>> debounce delay on link resume).
> 
> Arg. Indeed. Ignore my comment :)
> 
> Let's wait to see if there are any more comments and I will queue this
> up for 5.17.

Thank you. In the second patch, I should have probably mentioned, that 
the flag already exists. Maybe you can amend that before applying them.

> Going forward, it would be interesting to work on reversing the default:
> no delay by default and mark all old & unknown adapters as needing the
> delay. As you mentioned, most modern AHCI adapters most likely do not
> need it.
> 
> Are you willing to take this and send a patches ?

Yes, I have it on my to-do list, but cannot promise anything.


Kind regards,

Paul


>>>> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
>>>> ---
>>>>    drivers/ata/ahci_brcm.c   | 2 +-
>>>>    drivers/ata/libata-sata.c | 2 +-
>>>>    include/linux/libata.h    | 2 +-
>>>>    3 files changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
>>>> index 6e9c5ade4c2ea..649815c196ed0 100644
>>>> --- a/drivers/ata/ahci_brcm.c
>>>> +++ b/drivers/ata/ahci_brcm.c
>>>> @@ -333,7 +333,7 @@ static struct ata_port_operations ahci_brcm_platform_ops = {
>>>>    
>>>>    static const struct ata_port_info ahci_brcm_port_info = {
>>>>    	.flags		= AHCI_FLAG_COMMON | ATA_FLAG_NO_DIPM,
>>>> -	.link_flags	= ATA_LFLAG_NO_DB_DELAY,
>>>> +	.link_flags	= ATA_LFLAG_NO_DEBOUNCE_DELAY,
>>>>    	.pio_mask	= ATA_PIO4,
>>>>    	.udma_mask	= ATA_UDMA6,
>>>>    	.port_ops	= &ahci_brcm_platform_ops,
>>>> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
>>>> index b9c77885b8726..67b2e7cf3cc4e 100644
>>>> --- a/drivers/ata/libata-sata.c
>>>> +++ b/drivers/ata/libata-sata.c
>>>> @@ -317,7 +317,7 @@ int sata_link_resume(struct ata_link *link, const unsigned long *params,
>>>>    		 * immediately after resuming.  Delay 200ms before
>>>>    		 * debouncing.
>>>>    		 */
>>>> -		if (!(link->flags & ATA_LFLAG_NO_DB_DELAY))
>>>> +		if (!(link->flags & ATA_LFLAG_NO_DEBOUNCE_DELAY))
>>>>    			ata_msleep(link->ap, 200);
>>>>    
>>>>    		/* is SControl restored correctly? */
>>>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>>>> index 2a8404b26083c..15802e644962d 100644
>>>> --- a/include/linux/libata.h
>>>> +++ b/include/linux/libata.h
>>>> @@ -191,7 +191,7 @@ enum {
>>>>    	ATA_LFLAG_NO_LPM	= (1 << 8), /* disable LPM on this link */
>>>>    	ATA_LFLAG_RST_ONCE	= (1 << 9), /* limit recovery to one reset */
>>>>    	ATA_LFLAG_CHANGED	= (1 << 10), /* LPM state changed on this link */
>>>> -	ATA_LFLAG_NO_DB_DELAY	= (1 << 11), /* no debounce delay on link resume */
>>>> +	ATA_LFLAG_NO_DEBOUNCE_DELAY = (1 << 11), /* no debounce delay on link resume */
>>>>    
>>>>    	/* struct ata_port flags */
>>>>    	ATA_FLAG_SLAVE_POSS	= (1 << 0), /* host supports slave dev */
>>>
>>>

Re: [PATCH v4 1/2] ahci: Rename flag `ATA_LFLAG_NO_DB_DELAY` to `ATA_LFLAG_NO_DEBOUNCE_DELAY`

[Damien Le Moal]

On 1/6/22 00:36, Paul Menzel wrote:
> The new name is longer, but clearer.
> 
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
> ---
>  drivers/ata/ahci_brcm.c   | 2 +-
>  drivers/ata/libata-sata.c | 2 +-
>  include/linux/libata.h    | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/ahci_brcm.c b/drivers/ata/ahci_brcm.c
> index 6e9c5ade4c2ea..649815c196ed0 100644
> --- a/drivers/ata/ahci_brcm.c
> +++ b/drivers/ata/ahci_brcm.c
> @@ -333,7 +333,7 @@ static struct ata_port_operations ahci_brcm_platform_ops = {
>  
>  static const struct ata_port_info ahci_brcm_port_info = {
>  	.flags		= AHCI_FLAG_COMMON | ATA_FLAG_NO_DIPM,
> -	.link_flags	= ATA_LFLAG_NO_DB_DELAY,
> +	.link_flags	= ATA_LFLAG_NO_DEBOUNCE_DELAY,
>  	.pio_mask	= ATA_PIO4,
>  	.udma_mask	= ATA_UDMA6,
>  	.port_ops	= &ahci_brcm_platform_ops,
> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
> index b9c77885b8726..67b2e7cf3cc4e 100644
> --- a/drivers/ata/libata-sata.c
> +++ b/drivers/ata/libata-sata.c
> @@ -317,7 +317,7 @@ int sata_link_resume(struct ata_link *link, const unsigned long *params,
>  		 * immediately after resuming.  Delay 200ms before
>  		 * debouncing.
>  		 */
> -		if (!(link->flags & ATA_LFLAG_NO_DB_DELAY))
> +		if (!(link->flags & ATA_LFLAG_NO_DEBOUNCE_DELAY))
>  			ata_msleep(link->ap, 200);
>  
>  		/* is SControl restored correctly? */
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 2a8404b26083c..15802e644962d 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -191,7 +191,7 @@ enum {
>  	ATA_LFLAG_NO_LPM	= (1 << 8), /* disable LPM on this link */
>  	ATA_LFLAG_RST_ONCE	= (1 << 9), /* limit recovery to one reset */
>  	ATA_LFLAG_CHANGED	= (1 << 10), /* LPM state changed on this link */
> -	ATA_LFLAG_NO_DB_DELAY	= (1 << 11), /* no debounce delay on link resume */
> +	ATA_LFLAG_NO_DEBOUNCE_DELAY = (1 << 11), /* no debounce delay on link resume */
>  
>  	/* struct ata_port flags */
>  	ATA_FLAG_SLAVE_POSS	= (1 << 0), /* host supports slave dev */

I applied both patches to for-5.17 (sorry, I am late, this fell through
the cracks...).

I simplified the commit messages though, especially patch 2, since the
main point of that patch is to introduce support for a new controller
rather than discussing the debounce delay thingy.

If you are OK with it, I will reuse your well written commit message for
a patch removing the debounce delay though :) Working on it now, I will
post something by week end after testing.


-- 
Damien Le Moal
Western Digital Research