| Message ID | 20220127233527.445659-1-pmenzel@molgen.mpg.de |
|---|---|
| State | New |
| Headers | show |
| Series | ata: ahci: Skip 200 ms debounce delay for Marvell 88SE9235 | expand |
On 1/28/22 08:35, 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: > > Without this patch (with 200 ms delay): > > [ 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 patch (no delay): > > [ 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@molgen.mpg.de> > --- > 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 */ Looks good. But for the commit message, instead of the dmesg copy-paste, could you simply write the gains in terms of shortened scan time ? That would make it easier to understand the benefits of the patch. Also, there is no need for the lspci output.
Dear Damien, Thank you for the quick reply. Am 28.01.22 um 00:40 schrieb Damien Le Moal: > On 1/28/22 08:35, 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: >> >> Without this patch (with 200 ms delay): >> >> [ 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 patch (no delay): >> >> [ 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@molgen.mpg.de> >> --- >> 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 */ > > Looks good. But for the commit message, instead of the dmesg copy-paste, > could you simply write the gains in terms of shortened scan time ? That > would make it easier to understand the benefits of the patch. I can do: > Tested on IBM S822LC with current Linux 5.17-rc1, and the 200 ms is > gone, and the drives are still detected. I would still like to keep the Linux logs, as then it’s clear what I tested with (drives), and what ports were populated. > Also, there is no need for the lspci output. In my opinion, it prooves I used the correct PCI vendor and device codes, and also shows the revision number of the device I tested with. Kind regards, Paul
On 1/28/22 18:58, Paul Menzel wrote: > Dear Damien, > > > Thank you for the quick reply. > > > Am 28.01.22 um 00:40 schrieb Damien Le Moal: >> On 1/28/22 08:35, 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: >>> >>> Without this patch (with 200 ms delay): >>> >>> [ 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 patch (no delay): >>> >>> [ 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@molgen.mpg.de> >>> --- >>> 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 */ >> >> Looks good. But for the commit message, instead of the dmesg copy-paste, >> could you simply write the gains in terms of shortened scan time ? That >> would make it easier to understand the benefits of the patch. > > I can do: > >> Tested on IBM S822LC with current Linux 5.17-rc1, and the 200 ms is >> gone, and the drives are still detected. > I would still like to keep the Linux logs, as then it’s clear what I > tested with (drives), and what ports were populated. OK. Keep the dmesg log if you want, but add the summary. Something like: Without this patch (with 200 ms delay): device probe takes X ms <dmesg> With patch (no delay): device probe takes Y ms <dmesg> > >> Also, there is no need for the lspci output. > > In my opinion, it prooves I used the correct PCI vendor and device > codes, and also shows the revision number of the device I tested with. Fine. > > > Kind regards, > > Paul
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 */
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: Without this patch (with 200 ms delay): [ 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 patch (no delay): [ 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@molgen.mpg.de> --- drivers/ata/ahci.c | 2 ++ 1 file changed, 2 insertions(+)