[PATCH 1/2] ahci: Add Green Sardine vendor ID as board_ahci_mobile

[PATCH 1/2] ahci: Add Green Sardine vendor ID as board_ahci_mobile

[Mario Limonciello]

AMD requires that the SATA controller be configured for devsleep in order
for S0i3 entry to work properly.

commit b1a9585cc396 ("ata: ahci: Enable DEVSLP by default on x86 with
SLP_S0") sets up a kernel policy to enable devsleep on Intel mobile
platforms that are using s0ix.  Add the PCI ID for the SATA controller in
Green Sardine platforms to extend this policy by default for AMD based
systems using s0i3 as well.

Cc: Nehal-bakulchandra Shah <Nehal-bakulchandra.Shah@amd.com>
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=214091
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/ata/ahci.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index d60f34718b5d..1e1167e725a4 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -438,6 +438,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	/* AMD */
 	{ PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
 	{ 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 */
 	{ PCI_VENDOR_ID_AMD, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
 	  PCI_CLASS_STORAGE_RAID << 8, 0xffffff, board_ahci },
-- 
2.25.1

[PATCH 2/2] ata: Adjust behavior when StorageD3Enable _DSD is set

[Mario Limonciello]

The StorageD3Enable _DSD is used for the vendor to indicate that the disk
should be opted into or out of a different behavior based upon the platform
design.

For AMD's Renoir and Green Sardine platforms it's important that any
attached SATA storage has transitioned into DevSlp when s2idle is used.

If the disk is left in active/partial/slumber, then the system is not able
to resume properly.

When the StorageD3Enable _DSD is detected, check the system is using s2idle
and DevSlp is enabled and if so explicitly wait long enough for the disk to
enter DevSlp.

Cc: Nehal-bakulchandra Shah <Nehal-bakulchandra.Shah@amd.com>
BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=214091
Link: https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-intro
Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/ata/libahci.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 28430c093a7f..15c293da30ca 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2322,6 +2322,18 @@ int ahci_port_resume(struct ata_port *ap)
 }
 EXPORT_SYMBOL_GPL(ahci_port_resume);
 
+static void ahci_handle_s2idle(struct ata_port *ap)
+{
+	void __iomem *port_mmio = ahci_port_base(ap);
+	u32 devslp;
+
+	if (pm_suspend_via_firmware())
+		return;
+	devslp = readl(port_mmio + PORT_DEVSLP);
+	if ((devslp & PORT_DEVSLP_ADSE))
+		ata_msleep(ap, devslp_idle_timeout);
+}
+
 #ifdef CONFIG_PM
 static int ahci_port_suspend(struct ata_port *ap, pm_message_t mesg)
 {
@@ -2336,6 +2348,9 @@ static int ahci_port_suspend(struct ata_port *ap, pm_message_t mesg)
 		ata_port_freeze(ap);
 	}
 
+	if (acpi_storage_d3(ap->host->dev))
+		ahci_handle_s2idle(ap);
+
 	ahci_rpm_put_port(ap);
 	return rc;
 }
-- 
2.25.1

Re: [PATCH 2/2] ata: Adjust behavior when StorageD3Enable _DSD is set

[Damien Le Moal]

On 2021/11/13 5:15, Mario Limonciello wrote:
> The StorageD3Enable _DSD is used for the vendor to indicate that the disk
> should be opted into or out of a different behavior based upon the platform
> design.
> 
> For AMD's Renoir and Green Sardine platforms it's important that any
> attached SATA storage has transitioned into DevSlp when s2idle is used.
> 
> If the disk is left in active/partial/slumber, then the system is not able
> to resume properly.
> 
> When the StorageD3Enable _DSD is detected, check the system is using s2idle
> and DevSlp is enabled and if so explicitly wait long enough for the disk to
> enter DevSlp.
> 
> Cc: Nehal-bakulchandra Shah <Nehal-bakulchandra.Shah@amd.com>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=214091
> Link: https://docs.microsoft.com/en-us/windows-hardware/design/component-guidelines/power-management-for-storage-hardware-devices-intro
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/ata/libahci.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 28430c093a7f..15c293da30ca 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -2322,6 +2322,18 @@ int ahci_port_resume(struct ata_port *ap)
>  }
>  EXPORT_SYMBOL_GPL(ahci_port_resume);
>  
> +static void ahci_handle_s2idle(struct ata_port *ap)
> +{
> +	void __iomem *port_mmio = ahci_port_base(ap);
> +	u32 devslp;
> +
> +	if (pm_suspend_via_firmware())
> +		return;
> +	devslp = readl(port_mmio + PORT_DEVSLP);
> +	if ((devslp & PORT_DEVSLP_ADSE))
> +		ata_msleep(ap, devslp_idle_timeout);
> +}

This generates a warning at compilation if CONFIG_PM is not enabled. This
function must be under the #ifdef CONFIG_PM below.
I can fix this up, or you can resend a v2 if you prefer.

> +
>  #ifdef CONFIG_PM
>  static int ahci_port_suspend(struct ata_port *ap, pm_message_t mesg)
>  {
> @@ -2336,6 +2348,9 @@ static int ahci_port_suspend(struct ata_port *ap, pm_message_t mesg)
>  		ata_port_freeze(ap);
>  	}
>  
> +	if (acpi_storage_d3(ap->host->dev))
> +		ahci_handle_s2idle(ap);
> +
>  	ahci_rpm_put_port(ap);
>  	return rc;
>  }
> 


-- 
Damien Le Moal
Western Digital Research

RE: [PATCH 2/2] ata: Adjust behavior when StorageD3Enable _DSD is set

[Limonciello, Mario]

[Public]



> -----Original Message-----
> From: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Sent: Monday, November 15, 2021 19:41
> To: Limonciello, Mario <Mario.Limonciello@amd.com>
> Cc: open list:LIBATA SUBSYSTEM (Serial and Parallel ATA drivers) <linux-
> ide@vger.kernel.org>; Shah, Nehal-bakulchandra <Nehal-
> bakulchandra.Shah@amd.com>
> Subject: Re: [PATCH 2/2] ata: Adjust behavior when StorageD3Enable _DSD is set
> 
> On 2021/11/13 5:15, Mario Limonciello wrote:
> > The StorageD3Enable _DSD is used for the vendor to indicate that the disk
> > should be opted into or out of a different behavior based upon the platform
> > design.
> >
> > For AMD's Renoir and Green Sardine platforms it's important that any
> > attached SATA storage has transitioned into DevSlp when s2idle is used.
> >
> > If the disk is left in active/partial/slumber, then the system is not able
> > to resume properly.
> >
> > When the StorageD3Enable _DSD is detected, check the system is using s2idle
> > and DevSlp is enabled and if so explicitly wait long enough for the disk to
> > enter DevSlp.
> >
> > Cc: Nehal-bakulchandra Shah <Nehal-bakulchandra.Shah@amd.com>
> > BugLink:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
> kernel.org%2Fshow_bug.cgi%3Fid%3D214091&amp;data=04%7C01%7Cmario.li
> monciello%40amd.com%7Cb8d8f7b189a8471d2f0408d9a8a224ff%7C3dd8961f
> e4884e608e11a82d994e183d%7C0%7C0%7C637726236637477756%7CUnknow
> n%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haW
> wiLCJXVCI6Mn0%3D%7C3000&amp;sdata=NPnRafAYSFrIRlDAlM2K6WDzk9Mr3
> Rnn65n5wREiUZA%3D&amp;reserved=0
> > Link:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdocs.mi
> crosoft.com%2Fen-us%2Fwindows-hardware%2Fdesign%2Fcomponent-
> guidelines%2Fpower-management-for-storage-hardware-devices-
> intro&amp;data=04%7C01%7Cmario.limonciello%40amd.com%7Cb8d8f7b189a
> 8471d2f0408d9a8a224ff%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0
> %7C637726236637487711%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata
> =qTZOAUQFzKnXn9i6r9noM17YPvfZ3kRtAztZjtrdxvU%3D&amp;reserved=0
> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > ---
> >  drivers/ata/libahci.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> > index 28430c093a7f..15c293da30ca 100644
> > --- a/drivers/ata/libahci.c
> > +++ b/drivers/ata/libahci.c
> > @@ -2322,6 +2322,18 @@ int ahci_port_resume(struct ata_port *ap)
> >  }
> >  EXPORT_SYMBOL_GPL(ahci_port_resume);
> >
> > +static void ahci_handle_s2idle(struct ata_port *ap)
> > +{
> > +	void __iomem *port_mmio = ahci_port_base(ap);
> > +	u32 devslp;
> > +
> > +	if (pm_suspend_via_firmware())
> > +		return;
> > +	devslp = readl(port_mmio + PORT_DEVSLP);
> > +	if ((devslp & PORT_DEVSLP_ADSE))
> > +		ata_msleep(ap, devslp_idle_timeout);
> > +}
> 
> This generates a warning at compilation if CONFIG_PM is not enabled. This
> function must be under the #ifdef CONFIG_PM below.
> I can fix this up, or you can resend a v2 if you prefer.

Thanks for checking the detail and the offer to fix on your end.
That's fine with me.

Thanks!

> 
> > +
> >  #ifdef CONFIG_PM
> >  static int ahci_port_suspend(struct ata_port *ap, pm_message_t mesg)
> >  {
> > @@ -2336,6 +2348,9 @@ static int ahci_port_suspend(struct ata_port *ap,
> pm_message_t mesg)
> >  		ata_port_freeze(ap);
> >  	}
> >
> > +	if (acpi_storage_d3(ap->host->dev))
> > +		ahci_handle_s2idle(ap);
> > +
> >  	ahci_rpm_put_port(ap);
> >  	return rc;
> >  }
> >
> 
> 
> --
> Damien Le Moal
> Western Digital Research

Re: [PATCH 1/2] ahci: Add Green Sardine vendor ID as board_ahci_mobile

[Paul Menzel]

Dear Mario,


(For the records, is part of Linux since 5.16-rc2 (commit 1527f69204fe).)

Am 12.11.21 um 21:15 schrieb Mario Limonciello:
> AMD requires that the SATA controller be configured for devsleep in order
> for S0i3 entry to work properly.
> 
> commit b1a9585cc396 ("ata: ahci: Enable DEVSLP by default on x86 with
> SLP_S0") sets up a kernel policy to enable devsleep on Intel mobile
> platforms that are using s0ix.  Add the PCI ID for the SATA controller in
> Green Sardine platforms to extend this policy by default for AMD based
> systems using s0i3 as well.
> 
> Cc: Nehal-bakulchandra Shah <Nehal-bakulchandra.Shah@amd.com>
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=214091
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>   drivers/ata/ahci.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index d60f34718b5d..1e1167e725a4 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -438,6 +438,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>   	/* AMD */
>   	{ PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
>   	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
> +	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */

Aren’t 0x7900 and 0x7901 the same device only in different modes? I 
wonder, why different boards and different comments are used.

Additionally, the device 0x7901 is also present in desktop systems like 
Dell OptiPlex 5055 and MSI B350 MORTAR. Is `board_ahci_mobile` the right 
board then? Or should the flag `AHCI_HFLAG_IS_MOBILE` be renamed to 
avoid confusion?

>   	/* AMD is using RAID class only for ahci controllers */
>   	{ PCI_VENDOR_ID_AMD, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
>   	  PCI_CLASS_STORAGE_RAID << 8, 0xffffff, board_ahci },


Kind regards,

Paul

RE: [PATCH 1/2] ahci: Add Green Sardine vendor ID as board_ahci_mobile

[Limonciello, Mario]

[Public]

+Hans

> (For the records, is part of Linux since 5.16-rc2 (commit 1527f69204fe).)
> 
> Am 12.11.21 um 21:15 schrieb Mario Limonciello:
> > AMD requires that the SATA controller be configured for devsleep in order
> > for S0i3 entry to work properly.
> >
> > commit b1a9585cc396 ("ata: ahci: Enable DEVSLP by default on x86 with
> > SLP_S0") sets up a kernel policy to enable devsleep on Intel mobile
> > platforms that are using s0ix.  Add the PCI ID for the SATA controller in
> > Green Sardine platforms to extend this policy by default for AMD based
> > systems using s0i3 as well.
> >
> > Cc: Nehal-bakulchandra Shah <Nehal-bakulchandra.Shah@amd.com>
> > BugLink:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugz
> illa.kernel.org%2Fshow_bug.cgi%3Fid%3D214091&amp;data=04%7C01%7Cm
> ario.limonciello%40amd.com%7Ca32a202d437544cd2cbb08d9ef9112c0%7C3d
> d8961fe4884e608e11a82d994e183d%7C0%7C0%7C637804228648002522%7CU
> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
> 6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=CbfImBnwc8uV1L5QRBuV
> PLkP72wpS9yif1UbUwykhNI%3D&amp;reserved=0
> > Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> > ---
> >   drivers/ata/ahci.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> > index d60f34718b5d..1e1167e725a4 100644
> > --- a/drivers/ata/ahci.c
> > +++ b/drivers/ata/ahci.c
> > @@ -438,6 +438,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
> >   	/* AMD */
> >   	{ PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
> >   	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
> > +	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green
> Sardine */
> 
> Aren't 0x7900 and 0x7901 the same device only in different modes? I
> wonder, why different boards and different comments are used.

No they aren't the same device in different modes.

Reference:
https://www.amd.com/en/support/tech-docs/processor-programming-reference-ppr-for-amd-family-19h-model-51h-revision-a1
Page 33 has a table.

> 
> Additionally, the device 0x7901 is also present in desktop systems like
> Dell OptiPlex 5055 and MSI B350 MORTAR. Is `board_ahci_mobile` the right
> board then? Or should the flag `AHCI_HFLAG_IS_MOBILE` be renamed to
> avoid confusion?

Are you having a problem or just want clarity in the enum definition?

This was introduced by Hans in commit ebb82e3c79d2a to set LPM policy properly
for a number of mobile devices.

My opinion here is that the policy being for "mobile" devices only is short sighted as power
consumption policy on desktops is also relevant as OEMs ship desktops that need to meet
various power regulations for those too.

So I would be in the camp for renaming the flags.

> 
> >   	/* AMD is using RAID class only for ahci controllers */
> >   	{ PCI_VENDOR_ID_AMD, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
> >   	  PCI_CLASS_STORAGE_RAID << 8, 0xffffff, board_ahci },
> 
> 
> Kind regards,
> 
> Paul

Re: [PATCH 1/2] ahci: Add Green Sardine vendor ID as board_ahci_mobile

[Paul Menzel]

Dear Mario,


Am 14.02.22 um 17:07 schrieb Limonciello, Mario:

> +Hans
> 
>> (For the records, is part of Linux since 5.16-rc2 (commit 1527f69204fe).)
>>
>> Am 12.11.21 um 21:15 schrieb Mario Limonciello:
>>> AMD requires that the SATA controller be configured for devsleep in order
>>> for S0i3 entry to work properly.
>>>
>>> commit b1a9585cc396 ("ata: ahci: Enable DEVSLP by default on x86 with
>>> SLP_S0") sets up a kernel policy to enable devsleep on Intel mobile
>>> platforms that are using s0ix.  Add the PCI ID for the SATA controller in
>>> Green Sardine platforms to extend this policy by default for AMD based
>>> systems using s0i3 as well.
>>>
>>> Cc: Nehal-bakulchandra Shah <Nehal-bakulchandra.Shah@amd.com>
>>> BugLink:
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugz
>> illa.kernel.org%2Fshow_bug.cgi%3Fid%3D214091&amp;data=04%7C01%7Cm
>> ario.limonciello%40amd.com%7Ca32a202d437544cd2cbb08d9ef9112c0%7C3d
>> d8961fe4884e608e11a82d994e183d%7C0%7C0%7C637804228648002522%7CU
>> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
>> 6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=CbfImBnwc8uV1L5QRBuV
>> PLkP72wpS9yif1UbUwykhNI%3D&amp;reserved=0

You have to love Microsoft Outlook.

>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>> ---
>>>    drivers/ata/ahci.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>>> index d60f34718b5d..1e1167e725a4 100644
>>> --- a/drivers/ata/ahci.c
>>> +++ b/drivers/ata/ahci.c
>>> @@ -438,6 +438,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>>>    	/* AMD */
>>>    	{ PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
>>>    	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
>>> +	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */
>>
>> Aren't 0x7900 and 0x7901 the same device only in different modes? I
>> wonder, why different boards and different comments are used.
> 
> No they aren't the same device in different modes.
> 
> Reference:
> https://www.amd.com/en/support/tech-docs/processor-programming-reference-ppr-for-amd-family-19h-model-51h-revision-a1
> Page 33 has a table.

That table misses 0x7900h. (Where can I find it?) coreboot has 0x7900 
defined for Cezanne:

     src/include/device/pci_ids.h:#define PCI_DEVICE_ID_AMD_CZ_SATA  0x7900
     src/soc/amd/stoneyridge/include/soc/pci_devs.h: * SATA_IDE_IDEVID 
              0x7900

The PCI database too [3]:

> FCH SATA Controller [IDE mode]


>> Additionally, the device 0x7901 is also present in desktop systems like
>> Dell OptiPlex 5055 and MSI B350 MORTAR. Is `board_ahci_mobile` the right
>> board then? Or should the flag `AHCI_HFLAG_IS_MOBILE` be renamed to
>> avoid confusion?
> 
> Are you having a problem or just want clarity in the enum definition?

It’s more about clarity, and understanding the two different entries.

> This was introduced by Hans in commit ebb82e3c79d2a to set LPM policy properly
> for a number of mobile devices.
> 
> My opinion here is that the policy being for "mobile" devices only is short sighted as power
> consumption policy on desktops is also relevant as OEMs ship desktops that need to meet
> various power regulations for those too.
> 
> So I would be in the camp for renaming the flags.

Why can’t the LPM policy, if available(?), not be set for `board_ahci` 
by default, so `board_ahci_mobile` could be removed?

>>>    	/* AMD is using RAID class only for ahci controllers */
>>>    	{ PCI_VENDOR_ID_AMD, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
>>>    	  PCI_CLASS_STORAGE_RAID << 8, 0xffffff, board_ahci },


Kind regards,

Paul


[1]: https://review.coreboot.org/10418
[2]: https://review.coreboot.org/20200
[3]: https://pci-ids.ucw.cz/read/PC/1022/7900

Re: [PATCH 1/2] ahci: Add Green Sardine vendor ID as board_ahci_mobile

[Hans de Goede]

Hi all,

On 2/15/22 08:05, Paul Menzel wrote:
> Dear Mario,
> 
> 
> Am 14.02.22 um 17:07 schrieb Limonciello, Mario:
> 
>> +Hans
>>
>>> (For the records, is part of Linux since 5.16-rc2 (commit 1527f69204fe).)
>>>
>>> Am 12.11.21 um 21:15 schrieb Mario Limonciello:
>>>> AMD requires that the SATA controller be configured for devsleep in order
>>>> for S0i3 entry to work properly.
>>>>
>>>> commit b1a9585cc396 ("ata: ahci: Enable DEVSLP by default on x86 with
>>>> SLP_S0") sets up a kernel policy to enable devsleep on Intel mobile
>>>> platforms that are using s0ix.  Add the PCI ID for the SATA controller in
>>>> Green Sardine platforms to extend this policy by default for AMD based
>>>> systems using s0i3 as well.
>>>>
>>>> Cc: Nehal-bakulchandra Shah <Nehal-bakulchandra.Shah@amd.com>
>>>> BugLink:
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugz
>>> illa.kernel.org%2Fshow_bug.cgi%3Fid%3D214091&amp;data=04%7C01%7Cm
>>> ario.limonciello%40amd.com%7Ca32a202d437544cd2cbb08d9ef9112c0%7C3d
>>> d8961fe4884e608e11a82d994e183d%7C0%7C0%7C637804228648002522%7CU
>>> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
>>> 6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=CbfImBnwc8uV1L5QRBuV
>>> PLkP72wpS9yif1UbUwykhNI%3D&amp;reserved=0
> 
> You have to love Microsoft Outlook.
> 
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> ---
>>>>    drivers/ata/ahci.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>>>> index d60f34718b5d..1e1167e725a4 100644
>>>> --- a/drivers/ata/ahci.c
>>>> +++ b/drivers/ata/ahci.c
>>>> @@ -438,6 +438,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>>>>        /* AMD */
>>>>        { PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
>>>>        { PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
>>>> +    { PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */
>>>
>>> Aren't 0x7900 and 0x7901 the same device only in different modes? I
>>> wonder, why different boards and different comments are used.
>>
>> No they aren't the same device in different modes.
>>
>> Reference:
>> https://www.amd.com/en/support/tech-docs/processor-programming-reference-ppr-for-amd-family-19h-model-51h-revision-a1
>> Page 33 has a table.
> 
> That table misses 0x7900h. (Where can I find it?) coreboot has 0x7900 defined for Cezanne:
> 
>     src/include/device/pci_ids.h:#define PCI_DEVICE_ID_AMD_CZ_SATA  0x7900
>     src/soc/amd/stoneyridge/include/soc/pci_devs.h: * SATA_IDE_IDEVID              0x7900
> 
> The PCI database too [3]:
> 
>> FCH SATA Controller [IDE mode]
> 
> 
>>> Additionally, the device 0x7901 is also present in desktop systems like
>>> Dell OptiPlex 5055 and MSI B350 MORTAR. Is `board_ahci_mobile` the right
>>> board then? Or should the flag `AHCI_HFLAG_IS_MOBILE` be renamed to
>>> avoid confusion?
>>
>> Are you having a problem or just want clarity in the enum definition?
> 
> It’s more about clarity, and understanding the two different entries.
> 
>> This was introduced by Hans in commit ebb82e3c79d2a to set LPM policy properly
>> for a number of mobile devices.
>>
>> My opinion here is that the policy being for "mobile" devices only is short sighted as power
>> consumption policy on desktops is also relevant as OEMs ship desktops that need to meet
>> various power regulations for those too.
>>
>> So I would be in the camp for renaming the flags.
> 
> Why can’t the LPM policy, if available(?), not be set for `board_ahci` by default, so `board_ahci_mobile` could be removed?

When I added this, which was around the Haswell / Broadwell times, enabling
LPM on mobile devices was not so much important for the direct power-saving,
but to allow the entire package (integrated southbridge-ish) to go to deeper
sleep (aka PC) states.

Without this laptops would drain their batteries much faster, but at the same
time there were reports of LPM causing crashes and data corruption on some
systems, also see the list of ATA ids with a ATA_HORKAGE_NOLPM flag in
drivers/ata/libata-core.c . Which was added and grown over time to allow
enabling LPM by default without causing regressions.

So when adding support for enabling LPM modes by default, to get the
desired power-savings by default I tried to do this on a minimal set of
devices, to avoid causing regressions.

Another factor here is that in some cases LPM related issues went away
after either BIOS or disk/ssd firmware updates. So the motherboard firmware
is also a factor here and enabling LPM by default on non laptop(ish)
motherboards has never been tested.

Also enabling some of the deeper LPM modes comes at a latency cost which
may be undesirable at servers. Note this just sets the initial default
LPM mode, this can always be changed by userspace later.

Regards,

Hans





> 
>>>>        /* AMD is using RAID class only for ahci controllers */
>>>>        { PCI_VENDOR_ID_AMD, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
>>>>          PCI_CLASS_STORAGE_RAID << 8, 0xffffff, board_ahci },
> 
> 
> Kind regards,
> 
> Paul
> 
> 
> [1]: https://review.coreboot.org/10418
> [2]: https://review.coreboot.org/20200
> [3]: https://pci-ids.ucw.cz/read/PC/1022/7900
> 

Re: [PATCH 1/2] ahci: Add Green Sardine vendor ID as board_ahci_mobile

[Paul Menzel]

Dear Hans,


Am 15.02.22 um 12:43 schrieb Hans de Goede:

> On 2/15/22 08:05, Paul Menzel wrote:

>> Am 14.02.22 um 17:07 schrieb Limonciello, Mario:
>>
>>> +Hans
>>>
>>>> (For the records, is part of Linux since 5.16-rc2 (commit 1527f69204fe).)
>>>>
>>>> Am 12.11.21 um 21:15 schrieb Mario Limonciello:
>>>>> AMD requires that the SATA controller be configured for devsleep in order
>>>>> for S0i3 entry to work properly.
>>>>>
>>>>> commit b1a9585cc396 ("ata: ahci: Enable DEVSLP by default on x86 with
>>>>> SLP_S0") sets up a kernel policy to enable devsleep on Intel mobile
>>>>> platforms that are using s0ix.  Add the PCI ID for the SATA controller in
>>>>> Green Sardine platforms to extend this policy by default for AMD based
>>>>> systems using s0i3 as well.
>>>>>
>>>>> Cc: Nehal-bakulchandra Shah <Nehal-bakulchandra.Shah@amd.com>
>>>>> BugLink:
>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugz
>>>> illa.kernel.org%2Fshow_bug.cgi%3Fid%3D214091&amp;data=04%7C01%7Cm
>>>> ario.limonciello%40amd.com%7Ca32a202d437544cd2cbb08d9ef9112c0%7C3d
>>>> d8961fe4884e608e11a82d994e183d%7C0%7C0%7C637804228648002522%7CU
>>>> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
>>>> 6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=CbfImBnwc8uV1L5QRBuV
>>>> PLkP72wpS9yif1UbUwykhNI%3D&amp;reserved=0
>>
>> You have to love Microsoft Outlook.
>>
>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>> ---
>>>>>     drivers/ata/ahci.c | 1 +
>>>>>     1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>>>>> index d60f34718b5d..1e1167e725a4 100644
>>>>> --- a/drivers/ata/ahci.c
>>>>> +++ b/drivers/ata/ahci.c
>>>>> @@ -438,6 +438,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>>>>>         /* AMD */
>>>>>         { PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
>>>>>         { PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
>>>>> +    { PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */
>>>>
>>>> Aren't 0x7900 and 0x7901 the same device only in different modes? I
>>>> wonder, why different boards and different comments are used.
>>>
>>> No they aren't the same device in different modes.
>>>
>>> Reference:
>>> https://www.amd.com/en/support/tech-docs/processor-programming-reference-ppr-for-amd-family-19h-model-51h-revision-a1
>>> Page 33 has a table.
>>
>> That table misses 0x7900h. (Where can I find it?) coreboot has 0x7900 defined for Cezanne:
>>
>>      src/include/device/pci_ids.h:#define PCI_DEVICE_ID_AMD_CZ_SATA  0x7900
>>      src/soc/amd/stoneyridge/include/soc/pci_devs.h: * SATA_IDE_IDEVID              0x7900
>>
>> The PCI database too [3]:
>>
>>> FCH SATA Controller [IDE mode]
>>
>>
>>>> Additionally, the device 0x7901 is also present in desktop systems like
>>>> Dell OptiPlex 5055 and MSI B350 MORTAR. Is `board_ahci_mobile` the right
>>>> board then? Or should the flag `AHCI_HFLAG_IS_MOBILE` be renamed to
>>>> avoid confusion?
>>>
>>> Are you having a problem or just want clarity in the enum definition?
>>
>> It’s more about clarity, and understanding the two different entries.
>>
>>> This was introduced by Hans in commit ebb82e3c79d2a to set LPM policy properly
>>> for a number of mobile devices.
>>>
>>> My opinion here is that the policy being for "mobile" devices only is short sighted as power
>>> consumption policy on desktops is also relevant as OEMs ship desktops that need to meet
>>> various power regulations for those too.
>>>
>>> So I would be in the camp for renaming the flags.
>>
>> Why can’t the LPM policy, if available(?), not be set for `board_ahci` by default, so `board_ahci_mobile` could be removed?
> 
> When I added this, which was around the Haswell / Broadwell times, enabling
> LPM on mobile devices was not so much important for the direct power-saving,
> but to allow the entire package (integrated southbridge-ish) to go to deeper
> sleep (aka PC) states.
> 
> Without this laptops would drain their batteries much faster, but at the same
> time there were reports of LPM causing crashes and data corruption on some
> systems, also see the list of ATA ids with a ATA_HORKAGE_NOLPM flag in
> drivers/ata/libata-core.c . Which was added and grown over time to allow
> enabling LPM by default without causing regressions.

This seems to be related to the hard driver though, and not the chipset?

> So when adding support for enabling LPM modes by default, to get the
> desired power-savings by default I tried to do this on a minimal set of
> devices, to avoid causing regressions.
> 
> Another factor here is that in some cases LPM related issues went away
> after either BIOS or disk/ssd firmware updates. So the motherboard firmware
> is also a factor here and enabling LPM by default on non laptop(ish)
> motherboards has never been tested.
> 
> Also enabling some of the deeper LPM modes comes at a latency cost which
> may be undesirable at servers. Note this just sets the initial default
> LPM mode, this can always be changed by userspace later.

Thank you for the elaborate explanation. What do you suggest in regards 
the 1022:7901, present in laptops and desktop systems?

>>>>>         /* AMD is using RAID class only for ahci controllers */
>>>>>         { PCI_VENDOR_ID_AMD, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
>>>>>           PCI_CLASS_STORAGE_RAID << 8, 0xffffff, board_ahci },


Kind regards,

Paul


>> [1]: https://review.coreboot.org/10418
>> [2]: https://review.coreboot.org/20200
>> [3]: https://pci-ids.ucw.cz/read/PC/1022/7900

Re: [PATCH 1/2] ahci: Add Green Sardine vendor ID as board_ahci_mobile

[Hans de Goede]

Hi Paul,

On 2/15/22 13:27, Paul Menzel wrote:
> Dear Hans,
> 
> 
> Am 15.02.22 um 12:43 schrieb Hans de Goede:
> 
>> On 2/15/22 08:05, Paul Menzel wrote:
> 
>>> Am 14.02.22 um 17:07 schrieb Limonciello, Mario:
>>>
>>>> +Hans
>>>>
>>>>> (For the records, is part of Linux since 5.16-rc2 (commit 1527f69204fe).)
>>>>>
>>>>> Am 12.11.21 um 21:15 schrieb Mario Limonciello:
>>>>>> AMD requires that the SATA controller be configured for devsleep in order
>>>>>> for S0i3 entry to work properly.
>>>>>>
>>>>>> commit b1a9585cc396 ("ata: ahci: Enable DEVSLP by default on x86 with
>>>>>> SLP_S0") sets up a kernel policy to enable devsleep on Intel mobile
>>>>>> platforms that are using s0ix.  Add the PCI ID for the SATA controller in
>>>>>> Green Sardine platforms to extend this policy by default for AMD based
>>>>>> systems using s0i3 as well.
>>>>>>
>>>>>> Cc: Nehal-bakulchandra Shah <Nehal-bakulchandra.Shah@amd.com>
>>>>>> BugLink:
>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugz
>>>>> illa.kernel.org%2Fshow_bug.cgi%3Fid%3D214091&amp;data=04%7C01%7Cm
>>>>> ario.limonciello%40amd.com%7Ca32a202d437544cd2cbb08d9ef9112c0%7C3d
>>>>> d8961fe4884e608e11a82d994e183d%7C0%7C0%7C637804228648002522%7CU
>>>>> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
>>>>> 6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=CbfImBnwc8uV1L5QRBuV
>>>>> PLkP72wpS9yif1UbUwykhNI%3D&amp;reserved=0
>>>
>>> You have to love Microsoft Outlook.
>>>
>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>>> ---
>>>>>>     drivers/ata/ahci.c | 1 +
>>>>>>     1 file changed, 1 insertion(+)
>>>>>>
>>>>>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>>>>>> index d60f34718b5d..1e1167e725a4 100644
>>>>>> --- a/drivers/ata/ahci.c
>>>>>> +++ b/drivers/ata/ahci.c
>>>>>> @@ -438,6 +438,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>>>>>>         /* AMD */
>>>>>>         { PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
>>>>>>         { PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
>>>>>> +    { PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green Sardine */
>>>>>
>>>>> Aren't 0x7900 and 0x7901 the same device only in different modes? I
>>>>> wonder, why different boards and different comments are used.
>>>>
>>>> No they aren't the same device in different modes.
>>>>
>>>> Reference:
>>>> https://www.amd.com/en/support/tech-docs/processor-programming-reference-ppr-for-amd-family-19h-model-51h-revision-a1
>>>> Page 33 has a table.
>>>
>>> That table misses 0x7900h. (Where can I find it?) coreboot has 0x7900 defined for Cezanne:
>>>
>>>      src/include/device/pci_ids.h:#define PCI_DEVICE_ID_AMD_CZ_SATA  0x7900
>>>      src/soc/amd/stoneyridge/include/soc/pci_devs.h: * SATA_IDE_IDEVID              0x7900
>>>
>>> The PCI database too [3]:
>>>
>>>> FCH SATA Controller [IDE mode]
>>>
>>>
>>>>> Additionally, the device 0x7901 is also present in desktop systems like
>>>>> Dell OptiPlex 5055 and MSI B350 MORTAR. Is `board_ahci_mobile` the right
>>>>> board then? Or should the flag `AHCI_HFLAG_IS_MOBILE` be renamed to
>>>>> avoid confusion?
>>>>
>>>> Are you having a problem or just want clarity in the enum definition?
>>>
>>> It’s more about clarity, and understanding the two different entries.
>>>
>>>> This was introduced by Hans in commit ebb82e3c79d2a to set LPM policy properly
>>>> for a number of mobile devices.
>>>>
>>>> My opinion here is that the policy being for "mobile" devices only is short sighted as power
>>>> consumption policy on desktops is also relevant as OEMs ship desktops that need to meet
>>>> various power regulations for those too.
>>>>
>>>> So I would be in the camp for renaming the flags.
>>>
>>> Why can’t the LPM policy, if available(?), not be set for `board_ahci` by default, so `board_ahci_mobile` could be removed?
>>
>> When I added this, which was around the Haswell / Broadwell times, enabling
>> LPM on mobile devices was not so much important for the direct power-saving,
>> but to allow the entire package (integrated southbridge-ish) to go to deeper
>> sleep (aka PC) states.
>>
>> Without this laptops would drain their batteries much faster, but at the same
>> time there were reports of LPM causing crashes and data corruption on some
>> systems, also see the list of ATA ids with a ATA_HORKAGE_NOLPM flag in
>> drivers/ata/libata-core.c . Which was added and grown over time to allow
>> enabling LPM by default without causing regressions.
> 
> This seems to be related to the hard driver though, and not the chipset?

It is related to both, as mentioned sometimes BIOS updates (presumably
involving SATA chipset settings tweaks) resolved LPM issues.

A drive having the ATA_HORKAGE_NOLPM flag basically means that it is know
to cause (serious) issues with at least one of the mobile marked chipsets.

For another example of sometimes issues being chipset specific see the
recently added ATA_HORKAGE_NO_NCQ_ON_ATI quirk, which fixes a bad
interaction between early AMD/ATI sata chihpsets and Samsung SSD 860
and Samsung SSD 870 series drives.

>> So when adding support for enabling LPM modes by default, to get the
>> desired power-savings by default I tried to do this on a minimal set of
>> devices, to avoid causing regressions.
>>
>> Another factor here is that in some cases LPM related issues went away
>> after either BIOS or disk/ssd firmware updates. So the motherboard firmware
>> is also a factor here and enabling LPM by default on non laptop(ish)
>> motherboards has never been tested.
>>
>> Also enabling some of the deeper LPM modes comes at a latency cost which
>> may be undesirable at servers. Note this just sets the initial default
>> LPM mode, this can always be changed by userspace later.
> 
> Thank you for the elaborate explanation. What do you suggest in regards the 1022:7901, present in laptops and desktop systems?

I would suggest to go with board_ahci_mobile as is done in Mario's
original patch. We really want this for laptops and for desktops
using the same chipset I don't expect this to cause any issues.

Regards,

Hans

Re: [PATCH 1/2] ahci: Add Green Sardine vendor ID as board_ahci_mobile

[Limonciello, Mario]

On 2/15/2022 01:05, Paul Menzel wrote:
> Dear Mario,
> 
> 
> Am 14.02.22 um 17:07 schrieb Limonciello, Mario:
> 
>> +Hans
>>
>>> (For the records, is part of Linux since 5.16-rc2 (commit 
>>> 1527f69204fe).)
>>>
>>> Am 12.11.21 um 21:15 schrieb Mario Limonciello:
>>>> AMD requires that the SATA controller be configured for devsleep in 
>>>> order
>>>> for S0i3 entry to work properly.
>>>>
>>>> commit b1a9585cc396 ("ata: ahci: Enable DEVSLP by default on x86 with
>>>> SLP_S0") sets up a kernel policy to enable devsleep on Intel mobile
>>>> platforms that are using s0ix.  Add the PCI ID for the SATA 
>>>> controller in
>>>> Green Sardine platforms to extend this policy by default for AMD based
>>>> systems using s0i3 as well.
>>>>
>>>> Cc: Nehal-bakulchandra Shah <Nehal-bakulchandra.Shah@amd.com>
>>>> BugLink:
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugz
>>> illa.kernel.org%2Fshow_bug.cgi%3Fid%3D214091&amp;data=04%7C01%7Cm
>>> ario.limonciello%40amd.com%7Ca32a202d437544cd2cbb08d9ef9112c0%7C3d
>>> d8961fe4884e608e11a82d994e183d%7C0%7C0%7C637804228648002522%7CU
>>> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
>>> 6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=CbfImBnwc8uV1L5QRBuV
>>> PLkP72wpS9yif1UbUwykhNI%3D&amp;reserved=0
> 
> You have to love Microsoft Outlook.
> 
>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>> ---
>>>>    drivers/ata/ahci.c | 1 +
>>>>    1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>>>> index d60f34718b5d..1e1167e725a4 100644
>>>> --- a/drivers/ata/ahci.c
>>>> +++ b/drivers/ata/ahci.c
>>>> @@ -438,6 +438,7 @@ static const struct pci_device_id ahci_pci_tbl[] 
>>>> = {
>>>>        /* AMD */
>>>>        { PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
>>>>        { PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
>>>> +    { PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green 
>>>> Sardine */
>>>
>>> Aren't 0x7900 and 0x7901 the same device only in different modes? I
>>> wonder, why different boards and different comments are used.
>>
>> No they aren't the same device in different modes.
>>
>> Reference:
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.amd.com%2Fen%2Fsupport%2Ftech-docs%2Fprocessor-programming-reference-ppr-for-amd-family-19h-model-51h-revision-a1&amp;data=04%7C01%7CMario.Limonciello%40amd.com%7Cf25ec205dd2e46253a2208d9f0519b39%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637805055570772355%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=POXfSI626inWB7k73CqJF3IMp6y31r%2F%2BugdCXOkvydo%3D&amp;reserved=0 
>>
>> Page 33 has a table.
> 
> That table misses 0x7900h. (Where can I find it?) coreboot has 0x7900 
> defined for Cezanne:
> 
>      src/include/device/pci_ids.h:#define PCI_DEVICE_ID_AMD_CZ_SATA  0x7900
>      src/soc/amd/stoneyridge/include/soc/pci_devs.h: * SATA_IDE_IDEVID 
>               0x7900
> 

You can see that 0x7900 was introduced in the kernel back in 2013; well 
in advance of the controller used in Cezanne.

So I really don't believe that CZ in that context for coreboot means 
Cezanne.  It's from an earlier product.  I haven't referenced any older 
documentation to confirm anything but if I were to venture a guess based 
on those names you pasted it's probably introduced with Carrizo and 
continues to be used up until Stoney Ridge.


> The PCI database too [3]:
> 
>> FCH SATA Controller [IDE mode]

Just FYI the strings in the PCI database is based on empirical user 
submissions.  You shouldn't rely upon it to tell you what generations 
different devices are in.

For example I found recently on some family 19h laptops that they claim 
to have a family 17h DMIC according to GNOME Settings which stems back 
to a string that was in the PCI database.

When I encountered this I submitted a correction to make it more 
generic, but I'm sure there are plenty more like this.

> 
> 
>>> Additionally, the device 0x7901 is also present in desktop systems like
>>> Dell OptiPlex 5055 and MSI B350 MORTAR. Is `board_ahci_mobile` the right
>>> board then? Or should the flag `AHCI_HFLAG_IS_MOBILE` be renamed to
>>> avoid confusion?
>>
>> Are you having a problem or just want clarity in the enum definition?
> 
> It’s more about clarity, and understanding the two different entries.
> 
>> This was introduced by Hans in commit ebb82e3c79d2a to set LPM policy 
>> properly
>> for a number of mobile devices.
>>
>> My opinion here is that the policy being for "mobile" devices only is 
>> short sighted as power
>> consumption policy on desktops is also relevant as OEMs ship desktops 
>> that need to meet
>> various power regulations for those too.
>>
>> So I would be in the camp for renaming the flags.
> 
> Why can’t the LPM policy, if available(?), not be set for `board_ahci` 
> by default, so `board_ahci_mobile` could be removed?
> 
>>>>        /* AMD is using RAID class only for ahci controllers */
>>>>        { PCI_VENDOR_ID_AMD, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID,
>>>>          PCI_CLASS_STORAGE_RAID << 8, 0xffffff, board_ahci },
> 
> 
> Kind regards,
> 
> Paul
> 
> 
> [1]: 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.coreboot.org%2F10418&amp;data=04%7C01%7CMario.Limonciello%40amd.com%7Cf25ec205dd2e46253a2208d9f0519b39%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637805055570772355%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=vqX1NjUKE0STzvmKSWtJxhrrdQnH%2BmqAiXGuqNMQAt0%3D&amp;reserved=0 
> 
> [2]: 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.coreboot.org%2F20200&amp;data=04%7C01%7CMario.Limonciello%40amd.com%7Cf25ec205dd2e46253a2208d9f0519b39%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637805055570772355%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=oz3K76NGXrp41wq%2B5QcPUtJG%2BfqxCGOTVojfYb%2BMIlw%3D&amp;reserved=0 
> 
> [3]: 
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpci-ids.ucw.cz%2Fread%2FPC%2F1022%2F7900&amp;data=04%7C01%7CMario.Limonciello%40amd.com%7Cf25ec205dd2e46253a2208d9f0519b39%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637805055570772355%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=Y32YS5rIxBXTyNQ8VOuTxBTbfsOipBcIPAvmhBhSz7E%3D&amp;reserved=0 
> 

Re: [PATCH 1/2] ahci: Add Green Sardine vendor ID as board_ahci_mobile

[Hans de Goede]

Hi,

On 2/14/22 17:07, Limonciello, Mario wrote:
> [Public]
> 
> +Hans
> 
>> (For the records, is part of Linux since 5.16-rc2 (commit 1527f69204fe).)
>>
>> Am 12.11.21 um 21:15 schrieb Mario Limonciello:
>>> AMD requires that the SATA controller be configured for devsleep in order
>>> for S0i3 entry to work properly.
>>>
>>> commit b1a9585cc396 ("ata: ahci: Enable DEVSLP by default on x86 with
>>> SLP_S0") sets up a kernel policy to enable devsleep on Intel mobile
>>> platforms that are using s0ix.  Add the PCI ID for the SATA controller in
>>> Green Sardine platforms to extend this policy by default for AMD based
>>> systems using s0i3 as well.
>>>
>>> Cc: Nehal-bakulchandra Shah <Nehal-bakulchandra.Shah@amd.com>
>>> BugLink:
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugz
>> illa.kernel.org%2Fshow_bug.cgi%3Fid%3D214091&amp;data=04%7C01%7Cm
>> ario.limonciello%40amd.com%7Ca32a202d437544cd2cbb08d9ef9112c0%7C3d
>> d8961fe4884e608e11a82d994e183d%7C0%7C0%7C637804228648002522%7CU
>> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
>> 6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&amp;sdata=CbfImBnwc8uV1L5QRBuV
>> PLkP72wpS9yif1UbUwykhNI%3D&amp;reserved=0
>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>> ---
>>>   drivers/ata/ahci.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
>>> index d60f34718b5d..1e1167e725a4 100644
>>> --- a/drivers/ata/ahci.c
>>> +++ b/drivers/ata/ahci.c
>>> @@ -438,6 +438,7 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>>>   	/* AMD */
>>>   	{ PCI_VDEVICE(AMD, 0x7800), board_ahci }, /* AMD Hudson-2 */
>>>   	{ PCI_VDEVICE(AMD, 0x7900), board_ahci }, /* AMD CZ */
>>> +	{ PCI_VDEVICE(AMD, 0x7901), board_ahci_mobile }, /* AMD Green
>> Sardine */
>>
>> Aren't 0x7900 and 0x7901 the same device only in different modes? I
>> wonder, why different boards and different comments are used.
> 
> No they aren't the same device in different modes.
> 
> Reference:
> https://www.amd.com/en/support/tech-docs/processor-programming-reference-ppr-for-amd-family-19h-model-51h-revision-a1
> Page 33 has a table.
> 
>>
>> Additionally, the device 0x7901 is also present in desktop systems like
>> Dell OptiPlex 5055 and MSI B350 MORTAR. Is `board_ahci_mobile` the right
>> board then? Or should the flag `AHCI_HFLAG_IS_MOBILE` be renamed to
>> avoid confusion?
> 
> Are you having a problem or just want clarity in the enum definition?
> 
> This was introduced by Hans in commit ebb82e3c79d2a to set LPM policy properly
> for a number of mobile devices.
> 
> My opinion here is that the policy being for "mobile" devices only is short sighted as power
> consumption policy on desktops is also relevant as OEMs ship desktops that need to meet
> various power regulations for those too.
> 
> So I would be in the camp for renaming the flags.

I have no objection against renaming the flags, maybe rename "mobile" to
"enable_lpm_by_default" ? That is a bit long, but it is what it is all
about. Alternatively we could go with "low_power" but that feels much less
descriptive.

Regards,

Hans