Re: [PATCH v3] ata: ahci: Skip 200 ms debounce delay for Marvell 88SE9235

[Paul Menzel]

Dear Damien,


Am 02.02.22 um 09:14 schrieb Damien Le Moal:
> On 2/1/22 16:12, Paul Menzel wrote:
> > The 200 ms delay before debouncing the PHY in `sata_link_resume()` is
> > not needed for the Marvell 88SE9235.
> >
> >      $ lspci -nn -s 0021:0e:00.0
> >      0021:0e:00.0 SATA controller [0106]: Marvell Technology Group Ltd. 88SE9235 PCIe 2.0 x2 4-port SATA 6 Gb/s Controller [1b4b:9235] (rev 11)
> >
> > So, remove it. Tested on IBM S822LC with current Linux 5.17-rc1:
> >
> > Currently, without this patch (with 200 ms delay), device probe for ata1
> > takes 485 ms:
> >
> >      [    3.358158] ata1: SATA max UDMA/133 abar m2048@0x3fe881000000 port 0x3fe881000100 irq 39
> >      [    3.358175] ata2: SATA max UDMA/133 abar m2048@0x3fe881000000 port 0x3fe881000180 irq 39
> >      [    3.358191] ata3: SATA max UDMA/133 abar m2048@0x3fe881000000 port 0x3fe881000200 irq 39
> >      [    3.358207] ata4: SATA max UDMA/133 abar m2048@0x3fe881000000 port 0x3fe881000280 irq 39
> >      […]
> >      [    3.677542] ata3: SATA link down (SStatus 0 SControl 300)
> >      [    3.677719] ata4: SATA link down (SStatus 0 SControl 300)
> >      [    3.839242] ata2: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
> >      [    3.839828] ata2.00: ATA-10: ST1000NX0313         00LY266 00LY265IBM, BE33, max UDMA/133
> >      [    3.840029] ata2.00: 1953525168 sectors, multi 0: LBA48 NCQ (depth 32), AA
> >      [    3.841796] ata2.00: configured for UDMA/133
> >      [    3.843231] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
> >      [    3.844083] ata1.00: ATA-10: ST1000NX0313         00LY266 00LY265IBM, BE33, max UDMA/133
> >      [    3.844313] ata1.00: 1953525168 sectors, multi 0: LBA48 NCQ (depth 32), AA
> >      [    3.846043] ata1.00: configured for UDMA/133
> >
> > With this patch (no delay) device probe for ata1 takes 273 ms:
> >
> >      [    3.624259] ata1: SATA max UDMA/133 abar m2048@0x3fe881000000 port 0x3f e881000100 irq 39
> >      [    3.624436] ata2: SATA max UDMA/133 abar m2048@0x3fe881000000 port 0x3f e881000180 irq 39
> >      [    3.624452] ata3: SATA max UDMA/133 abar m2048@0x3fe881000000 port 0x3f e881000200 irq 39
> >      [    3.624468] ata4: SATA max UDMA/133 abar m2048@0x3fe881000000 port 0x3f e881000280 irq 39
> >      […]
> >      [    3.731966] ata3: SATA link down (SStatus 0 SControl 300)
> >      [    3.732069] ata4: SATA link down (SStatus 0 SControl 300)
> >      [    3.897448] ata1: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
> >      [    3.897678] ata2: SATA link up 6.0 Gbps (SStatus 133 SControl 300)
> >      [    3.898140] ata1.00: ATA-10: ST1000NX0313         00LY266 00LY265IBM, BE33, max UDMA/133
> >      [    3.898175] ata2.00: ATA-10: ST1000NX0313         00LY266 00LY265IBM, BE33, max UDMA/133
> >      [    3.898287] ata1.00: 1953525168 sectors, multi 0: LBA48 NCQ (depth 32), AA
> >      [    3.898349] ata2.00: 1953525168 sectors, multi 0: LBA48 NCQ (depth 32), AA
> >      [    3.900070] ata1.00: configured for UDMA/133
> >      [    3.900166] ata2.00: configured for UDMA/133
> >
> > Signed-off-by: Paul Menzel <pmenzel@xxxxxxxxxxxxx>
> > ---
> > v2: address comments for commit message (but forgot v2 tag)
> > v3: resend with v3 tag in subject line/commit message summary
> >
> >   drivers/ata/ahci.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> > index ab5811ef5a53..edca4e8fd44e 100644
> > --- a/drivers/ata/ahci.c
> > +++ b/drivers/ata/ahci.c
> > @@ -582,6 +582,8 @@ static const struct pci_device_id ahci_pci_tbl[] = {
> >   	  .driver_data = board_ahci_yes_fbs },
> >   	{ PCI_DEVICE(PCI_VENDOR_ID_MARVELL_EXT, 0x9230),
> >   	  .driver_data = board_ahci_yes_fbs },
> > +	{ PCI_DEVICE(PCI_VENDOR_ID_MARVELL_EXT, 0x9235),
> > +	  .driver_data = board_ahci_no_debounce_delay },
> >   	{ PCI_DEVICE(PCI_VENDOR_ID_TTI, 0x0642), /* highpoint rocketraid 642L */
> >   	  .driver_data = board_ahci_yes_fbs },
> >   	{ PCI_DEVICE(PCI_VENDOR_ID_TTI, 0x0645), /* highpoint rocketraid 644L */
>
> Applied to for-5.18 with commit title and message changes. The title is now:
>
> ata: ahci: Add support for Marvell 88SE9235 adapter
>
> Since it is exactly what this patch is doing by adding a PCI ID.

Thank you for applying the patch. I saw the summary/title change also 
with the other patch. I am sorry, but I totally disagree. Reading that 
summary/title in `git log --oneline`, it’s not clear at all, what the 
patch does, and the full description or diff has to be read. “Add 
support” for me means, that it was unsupported before, which is not true 
at all as the defaults were used.

> The comments about the 200ms debounce delay not being needed is kept as
> a description of how this new adapter support is defined, using the
> board_ahci_no_debounce_delay board definition.


Kind regards,

Paul