From: "Limonciello, Mario" <mario.limonciello@amd.com>
To: Paul Menzel <pmenzel@molgen.mpg.de>, Hans de Goede <hdegoede@redhat.com>
Cc: linux-ide@vger.kernel.org, LKML <linux-kernel@vger.kernel.org>,
Damien Le Moal <damien.lemoal@opensource.wdc.com>,
Nehal-bakulchandra Shah <Nehal-bakulchandra.Shah@amd.com>
Subject: Re: [PATCH 1/2] ahci: Add Green Sardine vendor ID as board_ahci_mobile
Date: Tue, 15 Feb 2022 20:56:44 -0600 [thread overview]
Message-ID: <cb4bf44e-1399-277f-807b-1a7f26f80c1c@amd.com> (raw)
In-Reply-To: <6c846941-151d-e8a5-3ce8-a392b97186a8@molgen.mpg.de>
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&data=04%7C01%7Cm
>>> ario.limonciello%40amd.com%7Ca32a202d437544cd2cbb08d9ef9112c0%7C3d
>>> d8961fe4884e608e11a82d994e183d%7C0%7C0%7C637804228648002522%7CU
>>> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
>>> 6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=CbfImBnwc8uV1L5QRBuV
>>> PLkP72wpS9yif1UbUwykhNI%3D&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&data=04%7C01%7CMario.Limonciello%40amd.com%7Cf25ec205dd2e46253a2208d9f0519b39%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637805055570772355%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=POXfSI626inWB7k73CqJF3IMp6y31r%2F%2BugdCXOkvydo%3D&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&data=04%7C01%7CMario.Limonciello%40amd.com%7Cf25ec205dd2e46253a2208d9f0519b39%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637805055570772355%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=vqX1NjUKE0STzvmKSWtJxhrrdQnH%2BmqAiXGuqNMQAt0%3D&reserved=0
>
> [2]:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Freview.coreboot.org%2F20200&data=04%7C01%7CMario.Limonciello%40amd.com%7Cf25ec205dd2e46253a2208d9f0519b39%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637805055570772355%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=oz3K76NGXrp41wq%2B5QcPUtJG%2BfqxCGOTVojfYb%2BMIlw%3D&reserved=0
>
> [3]:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpci-ids.ucw.cz%2Fread%2FPC%2F1022%2F7900&data=04%7C01%7CMario.Limonciello%40amd.com%7Cf25ec205dd2e46253a2208d9f0519b39%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637805055570772355%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000&sdata=Y32YS5rIxBXTyNQ8VOuTxBTbfsOipBcIPAvmhBhSz7E%3D&reserved=0
>
next prev parent reply other threads:[~2022-02-16 2:57 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-12 20:15 [PATCH 1/2] ahci: Add Green Sardine vendor ID as board_ahci_mobile Mario Limonciello
2021-11-12 20:15 ` [PATCH 2/2] ata: Adjust behavior when StorageD3Enable _DSD is set Mario Limonciello
2022-02-14 8:07 ` [PATCH 1/2] ahci: Add Green Sardine vendor ID as board_ahci_mobile Paul Menzel
2022-02-14 16:07 ` Limonciello, Mario
2022-02-15 7:05 ` Paul Menzel
2022-02-15 11:43 ` Hans de Goede
2022-02-15 12:27 ` Paul Menzel
2022-02-15 12:52 ` Hans de Goede
2022-02-16 2:56 ` Limonciello, Mario [this message]
2022-02-15 12:55 ` Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cb4bf44e-1399-277f-807b-1a7f26f80c1c@amd.com \
--to=mario.limonciello@amd.com \
--cc=Nehal-bakulchandra.Shah@amd.com \
--cc=damien.lemoal@opensource.wdc.com \
--cc=hdegoede@redhat.com \
--cc=linux-ide@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pmenzel@molgen.mpg.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).