[PATCH 0/2] ahci minor quirk cleanups

[PATCH 0/2] ahci minor quirk cleanups

[Niklas Cassel]

Hello all,

Here comes some minor AHCI quirk cleanups.


Kind regards,
Niklas


Niklas Cassel (2):
  ahci: rename board_ahci_nosntf
  ahci: clean up ahci_broken_devslp quirk

 drivers/ata/ahci.c | 32 +++++++++++++-------------------
 1 file changed, 13 insertions(+), 19 deletions(-)

-- 
2.43.0

[PATCH 1/2] ahci: rename board_ahci_nosntf

[Niklas Cassel]

Commit 7edbb6059274 ("ahci: clean up intel_pcs_quirk") added a new board
type (board_ahci_pcs_quirk) which applies the Intel PCS quirk for legacy
platforms. However, it also modified board_ahci_avn and board_ahci_nosntf
to apply the same quirk.

board_ahci_avn is defined under the label:
/* board IDs for specific chipsets in alphabetical order */
This is a board for a specific chipset, so the naming is perfectly fine.
(The name does not need to be suffixed with _pcs_quirk, since all
controllers for this chipset require the quirk to be applied).

board_ahci_nosntf is defined under the label:
/* board IDs by feature in alphabetical order */
This is a board for a specific feature/quirk. However, it is used to
apply two different quirks.

Rename board_ahci_nosntf to more clearly highlight that this board ID
applies two different quirks.

Fixes: 7edbb6059274 ("ahci: clean up intel_pcs_quirk")
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/ahci.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 41b4c9777c85..c28ad3f4b59e 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -53,7 +53,7 @@ enum board_ids {
 	board_ahci_no_debounce_delay,
 	board_ahci_nomsi,
 	board_ahci_noncq,
-	board_ahci_nosntf,
+	board_ahci_nosntf_pcs_quirk,
 	/*
 	 * board_ahci_pcs_quirk is for legacy Intel platforms.
 	 * Modern Intel platforms should use board_ahci instead.
@@ -165,7 +165,7 @@ static const struct ata_port_info ahci_port_info[] = {
 		.udma_mask	= ATA_UDMA6,
 		.port_ops	= &ahci_ops,
 	},
-	[board_ahci_nosntf] = {
+	[board_ahci_nosntf_pcs_quirk] = {
 		AHCI_HFLAGS	(AHCI_HFLAG_NO_SNTF |
 				 AHCI_HFLAG_INTEL_PCS_QUIRK),
 		.flags		= AHCI_FLAG_COMMON,
@@ -271,7 +271,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ PCI_VDEVICE(INTEL, 0x2683), board_ahci_pcs_quirk }, /* ESB2 */
 	{ PCI_VDEVICE(INTEL, 0x27c6), board_ahci_pcs_quirk }, /* ICH7-M DH */
 	{ PCI_VDEVICE(INTEL, 0x2821), board_ahci_pcs_quirk }, /* ICH8 */
-	{ PCI_VDEVICE(INTEL, 0x2822), board_ahci_nosntf }, /* ICH8/Lewisburg RAID*/
+	{ PCI_VDEVICE(INTEL, 0x2822), board_ahci_nosntf_pcs_quirk }, /* ICH8/Lewisburg RAID*/
 	{ PCI_VDEVICE(INTEL, 0x2824), board_ahci_pcs_quirk }, /* ICH8 */
 	{ PCI_VDEVICE(INTEL, 0x2829), board_ahci_pcs_quirk }, /* ICH8M */
 	{ PCI_VDEVICE(INTEL, 0x282a), board_ahci_pcs_quirk }, /* ICH8M */
-- 
2.43.0

Re: [PATCH 1/2] ahci: rename board_ahci_nosntf

[Damien Le Moal]

On 2/13/24 22:07, Niklas Cassel wrote:
> Commit 7edbb6059274 ("ahci: clean up intel_pcs_quirk") added a new board
> type (board_ahci_pcs_quirk) which applies the Intel PCS quirk for legacy
> platforms. However, it also modified board_ahci_avn and board_ahci_nosntf
> to apply the same quirk.
> 
> board_ahci_avn is defined under the label:
> /* board IDs for specific chipsets in alphabetical order */
> This is a board for a specific chipset, so the naming is perfectly fine.
> (The name does not need to be suffixed with _pcs_quirk, since all
> controllers for this chipset require the quirk to be applied).
> 
> board_ahci_nosntf is defined under the label:
> /* board IDs by feature in alphabetical order */
> This is a board for a specific feature/quirk. However, it is used to
> apply two different quirks.
> 
> Rename board_ahci_nosntf to more clearly highlight that this board ID
> applies two different quirks.
> 
> Fixes: 7edbb6059274 ("ahci: clean up intel_pcs_quirk")
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/ata/ahci.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 41b4c9777c85..c28ad3f4b59e 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -53,7 +53,7 @@ enum board_ids {
>  	board_ahci_no_debounce_delay,
>  	board_ahci_nomsi,
>  	board_ahci_noncq,
> -	board_ahci_nosntf,
> +	board_ahci_nosntf_pcs_quirk,
>  	/*
>  	 * board_ahci_pcs_quirk is for legacy Intel platforms.
>  	 * Modern Intel platforms should use board_ahci instead.
> @@ -165,7 +165,7 @@ static const struct ata_port_info ahci_port_info[] = {
>  		.udma_mask	= ATA_UDMA6,
>  		.port_ops	= &ahci_ops,
>  	},
> -	[board_ahci_nosntf] = {
> +	[board_ahci_nosntf_pcs_quirk] = {
>  		AHCI_HFLAGS	(AHCI_HFLAG_NO_SNTF |
>  				 AHCI_HFLAG_INTEL_PCS_QUIRK),
>  		.flags		= AHCI_FLAG_COMMON,
> @@ -271,7 +271,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>  	{ PCI_VDEVICE(INTEL, 0x2683), board_ahci_pcs_quirk }, /* ESB2 */
>  	{ PCI_VDEVICE(INTEL, 0x27c6), board_ahci_pcs_quirk }, /* ICH7-M DH */
>  	{ PCI_VDEVICE(INTEL, 0x2821), board_ahci_pcs_quirk }, /* ICH8 */
> -	{ PCI_VDEVICE(INTEL, 0x2822), board_ahci_nosntf }, /* ICH8/Lewisburg RAID*/
> +	{ PCI_VDEVICE(INTEL, 0x2822), board_ahci_nosntf_pcs_quirk }, /* ICH8/Lewisburg RAID*/

Minor nit: I would name the board board_ahci_pcs_quirk_nosntf to make it easier
to see that this is a sub case of the general board_ahci_pcs_quirk.

>  	{ PCI_VDEVICE(INTEL, 0x2824), board_ahci_pcs_quirk }, /* ICH8 */
>  	{ PCI_VDEVICE(INTEL, 0x2829), board_ahci_pcs_quirk }, /* ICH8M */
>  	{ PCI_VDEVICE(INTEL, 0x282a), board_ahci_pcs_quirk }, /* ICH8M */

With that, this look good.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

[PATCH 2/2] ahci: clean up ahci_broken_devslp quirk

[Niklas Cassel]

Most quirks are applied using a specific board type board_ahci_no*
(e.g. board_ahci_nomsi, board_ahci_noncq), which then sets a flag
representing the specific quirk.

ahci_pci_tbl (which is the table of all supported PCI devices), then
uses that board type for the PCI vendor and device IDs which need to
be quirked.

The ahci_broken_devslp quirk is not implemented in this standard way.

Modify the ahci_broken_devslp quirk to be implemented like the other
quirks. This way, we will not have the same PCI device and vendor ID
scattered over ahci.c. It will simply be defined in a single location.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/ata/ahci.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index c28ad3f4b59e..1e1533d01803 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -51,6 +51,7 @@ enum board_ids {
 	board_ahci_43bit_dma,
 	board_ahci_ign_iferr,
 	board_ahci_no_debounce_delay,
+	board_ahci_no_devslp_pcs_quirk,
 	board_ahci_nomsi,
 	board_ahci_noncq,
 	board_ahci_nosntf_pcs_quirk,
@@ -151,6 +152,14 @@ static const struct ata_port_info ahci_port_info[] = {
 		.udma_mask	= ATA_UDMA6,
 		.port_ops	= &ahci_ops,
 	},
+	[board_ahci_no_devslp_pcs_quirk] = {
+		AHCI_HFLAGS	(AHCI_HFLAG_NO_DEVSLP |
+				 AHCI_HFLAG_INTEL_PCS_QUIRK),
+		.flags		= AHCI_FLAG_COMMON,
+		.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,
@@ -420,7 +429,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ PCI_VDEVICE(INTEL, 0x06d7), board_ahci_pcs_quirk }, /* Comet Lake-H RAID */
 	{ PCI_VDEVICE(INTEL, 0xa386), board_ahci_pcs_quirk }, /* Comet Lake PCH-V RAID */
 	{ PCI_VDEVICE(INTEL, 0x0f22), board_ahci_pcs_quirk }, /* Bay Trail AHCI */
-	{ PCI_VDEVICE(INTEL, 0x0f23), board_ahci_pcs_quirk }, /* Bay Trail AHCI */
+	{ PCI_VDEVICE(INTEL, 0x0f23), board_ahci_no_devslp_pcs_quirk }, /* Bay Trail AHCI */
 	{ PCI_VDEVICE(INTEL, 0x22a3), board_ahci_pcs_quirk }, /* Cherry Tr. AHCI */
 	{ PCI_VDEVICE(INTEL, 0x5ae3), board_ahci_pcs_quirk }, /* ApolloLake AHCI */
 	{ PCI_VDEVICE(INTEL, 0x34d3), board_ahci_pcs_quirk }, /* Ice Lake LP AHCI */
@@ -1420,17 +1429,6 @@ static bool ahci_broken_online(struct pci_dev *pdev)
 	return pdev->bus->number == (val >> 8) && pdev->devfn == (val & 0xff);
 }
 
-static bool ahci_broken_devslp(struct pci_dev *pdev)
-{
-	/* device with broken DEVSLP but still showing SDS capability */
-	static const struct pci_device_id ids[] = {
-		{ PCI_VDEVICE(INTEL, 0x0f23)}, /* Valleyview SoC */
-		{}
-	};
-
-	return pci_match_id(ids, pdev);
-}
-
 #ifdef CONFIG_ATA_ACPI
 static void ahci_gtf_filter_workaround(struct ata_host *host)
 {
@@ -1823,10 +1821,6 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 				&dev_attr_remapped_nvme.attr,
 				NULL);
 
-	/* must set flag prior to save config in order to take effect */
-	if (ahci_broken_devslp(pdev))
-		hpriv->flags |= AHCI_HFLAG_NO_DEVSLP;
-
 #ifdef CONFIG_ARM64
 	if (pdev->vendor == PCI_VENDOR_ID_HUAWEI &&
 	    pdev->device == 0xa235 &&
-- 
2.43.0

Re: [PATCH 2/2] ahci: clean up ahci_broken_devslp quirk

[Damien Le Moal]

On 2/13/24 22:07, Niklas Cassel wrote:
> Most quirks are applied using a specific board type board_ahci_no*
> (e.g. board_ahci_nomsi, board_ahci_noncq), which then sets a flag
> representing the specific quirk.
> 
> ahci_pci_tbl (which is the table of all supported PCI devices), then
> uses that board type for the PCI vendor and device IDs which need to
> be quirked.
> 
> The ahci_broken_devslp quirk is not implemented in this standard way.
> 
> Modify the ahci_broken_devslp quirk to be implemented like the other
> quirks. This way, we will not have the same PCI device and vendor ID
> scattered over ahci.c. It will simply be defined in a single location.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>
> ---
>  drivers/ata/ahci.c | 26 ++++++++++----------------
>  1 file changed, 10 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index c28ad3f4b59e..1e1533d01803 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -51,6 +51,7 @@ enum board_ids {
>  	board_ahci_43bit_dma,
>  	board_ahci_ign_iferr,
>  	board_ahci_no_debounce_delay,
> +	board_ahci_no_devslp_pcs_quirk,
>  	board_ahci_nomsi,
>  	board_ahci_noncq,
>  	board_ahci_nosntf_pcs_quirk,
> @@ -151,6 +152,14 @@ static const struct ata_port_info ahci_port_info[] = {
>  		.udma_mask	= ATA_UDMA6,
>  		.port_ops	= &ahci_ops,
>  	},
> +	[board_ahci_no_devslp_pcs_quirk] = {
> +		AHCI_HFLAGS	(AHCI_HFLAG_NO_DEVSLP |
> +				 AHCI_HFLAG_INTEL_PCS_QUIRK),
> +		.flags		= AHCI_FLAG_COMMON,
> +		.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,
> @@ -420,7 +429,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>  	{ PCI_VDEVICE(INTEL, 0x06d7), board_ahci_pcs_quirk }, /* Comet Lake-H RAID */
>  	{ PCI_VDEVICE(INTEL, 0xa386), board_ahci_pcs_quirk }, /* Comet Lake PCH-V RAID */
>  	{ PCI_VDEVICE(INTEL, 0x0f22), board_ahci_pcs_quirk }, /* Bay Trail AHCI */
> -	{ PCI_VDEVICE(INTEL, 0x0f23), board_ahci_pcs_quirk }, /* Bay Trail AHCI */
> +	{ PCI_VDEVICE(INTEL, 0x0f23), board_ahci_no_devslp_pcs_quirk }, /* Bay Trail AHCI */

Same nit as the previous patch: let's name this board_ahci_pcs_quirk_no_devslp.

With that, looks OK to me.

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

>  	{ PCI_VDEVICE(INTEL, 0x22a3), board_ahci_pcs_quirk }, /* Cherry Tr. AHCI */
>  	{ PCI_VDEVICE(INTEL, 0x5ae3), board_ahci_pcs_quirk }, /* ApolloLake AHCI */
>  	{ PCI_VDEVICE(INTEL, 0x34d3), board_ahci_pcs_quirk }, /* Ice Lake LP AHCI */
> @@ -1420,17 +1429,6 @@ static bool ahci_broken_online(struct pci_dev *pdev)
>  	return pdev->bus->number == (val >> 8) && pdev->devfn == (val & 0xff);
>  }
>  
> -static bool ahci_broken_devslp(struct pci_dev *pdev)
> -{
> -	/* device with broken DEVSLP but still showing SDS capability */
> -	static const struct pci_device_id ids[] = {
> -		{ PCI_VDEVICE(INTEL, 0x0f23)}, /* Valleyview SoC */
> -		{}
> -	};
> -
> -	return pci_match_id(ids, pdev);
> -}
> -
>  #ifdef CONFIG_ATA_ACPI
>  static void ahci_gtf_filter_workaround(struct ata_host *host)
>  {
> @@ -1823,10 +1821,6 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  				&dev_attr_remapped_nvme.attr,
>  				NULL);
>  
> -	/* must set flag prior to save config in order to take effect */
> -	if (ahci_broken_devslp(pdev))
> -		hpriv->flags |= AHCI_HFLAG_NO_DEVSLP;
> -
>  #ifdef CONFIG_ARM64
>  	if (pdev->vendor == PCI_VENDOR_ID_HUAWEI &&
>  	    pdev->device == 0xa235 &&

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 0/2] ahci minor quirk cleanups

[Mika Westerberg]

On Tue, Feb 13, 2024 at 02:07:29PM +0100, Niklas Cassel wrote:
> Hello all,
> 
> Here comes some minor AHCI quirk cleanups.
> 
> 
> Kind regards,
> Niklas
> 
> 
> Niklas Cassel (2):
>   ahci: rename board_ahci_nosntf
>   ahci: clean up ahci_broken_devslp quirk

Both,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 0/2] ahci minor quirk cleanups

[Dan Williams]

Niklas Cassel wrote:
> Hello all,
> 
> Here comes some minor AHCI quirk cleanups.
> 
> 
> Kind regards,
> Niklas
> 
> 
> Niklas Cassel (2):
>   ahci: rename board_ahci_nosntf
>   ahci: clean up ahci_broken_devslp quirk
> 
>  drivers/ata/ahci.c | 32 +++++++++++++-------------------
>  1 file changed, 13 insertions(+), 19 deletions(-)

Looks good and net lines removed, nice.

Reviewed-by: Dan Williams <dan.j.williams@intel.com>