On 7/18/2024 21:13, Yazen Ghannam wrote:
> On Thu, Jul 18, 2024 at 07:32:58PM +0530, Shyam Sundar S K wrote:
>> Add the new PCI Device IDs to the root IDs and misc ids list to support
>> new generation of AMD 1Ah family 60h Models of processors.
>
> Please be consistent with formatting.
>
> "Device" -> "device"
>
> "misc ids" -> "misc IDs"
>
> "Models" -> "models"
>
> Also, you have "0x1A" in the $SUBJECT, but you have "1Ah" in the commit
> message. I suggest staying with "1Ah" as that is the format used in AMD
> documentation.
>
> And "v1" is not necessary in the "[PATCH]" prefix.
>
> Furthermore, if you CC the "x86" alias, then you don't need to CC the
> individual x86 maintainers.
I used get_maintainer.pl to send it. I can remove individual names and
send it only to the x86 maintainers.
>
>>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
>> ---
>> (As the amd_nb functions are used by PMC and PMF drivers, without these IDs
>> being present AMD PMF/PMC probe shall fail.)
>
> This comment can go in the commit message. Otherwise, it'll be lost from
> the git history.
>
> The comment is helpful in that it gives a reason *why* these new IDs are
> needed.
>
My previous commit 0e640f0a47d8 ("x86/amd_nb: Add new PCI IDs for AMD
family 0x1a") included this note in the commit message, but Boris had
to trim it. Therefore, I excluded it this time.
Should I include or exclude this note?
I can do a re-spin based on your further remarks.
Thanks,
Shyam
>>
>> arch/x86/kernel/amd_nb.c | 3 +++
>> include/linux/pci_ids.h | 1 +
>> 2 files changed, 4 insertions(+)
>>
>> diff --git a/arch/x86/kernel/amd_nb.c b/arch/x86/kernel/amd_nb.c
>> index 059e5c16af05..61eadde08511 100644
>> --- a/arch/x86/kernel/amd_nb.c
>> +++ b/arch/x86/kernel/amd_nb.c
>> @@ -26,6 +26,7 @@
>> #define PCI_DEVICE_ID_AMD_19H_M70H_ROOT 0x14e8
>> #define PCI_DEVICE_ID_AMD_1AH_M00H_ROOT 0x153a
>> #define PCI_DEVICE_ID_AMD_1AH_M20H_ROOT 0x1507
>> +#define PCI_DEVICE_ID_AMD_1AH_M60H_ROOT 0x1122
>> #define PCI_DEVICE_ID_AMD_MI200_ROOT 0x14bb
>> #define PCI_DEVICE_ID_AMD_MI300_ROOT 0x14f8
>>
>> @@ -63,6 +64,7 @@ static const struct pci_device_id amd_root_ids[] = {
>> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_19H_M70H_ROOT) },
>> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_1AH_M00H_ROOT) },
>> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_1AH_M20H_ROOT) },
>> + { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_1AH_M60H_ROOT) },
>> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_MI200_ROOT) },
>> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_MI300_ROOT) },
>> {}
>> @@ -95,6 +97,7 @@ static const struct pci_device_id amd_nb_misc_ids[] = {
>> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_19H_M78H_DF_F3) },
>> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_1AH_M00H_DF_F3) },
>> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_1AH_M20H_DF_F3) },
>> + { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_1AH_M60H_DF_F3) },
>> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_1AH_M70H_DF_F3) },
>> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_MI200_DF_F3) },
>> { PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_MI300_DF_F3) },
>> diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
>> index 76a8f2d6bd64..bbe8f3dfa813 100644
>> --- a/include/linux/pci_ids.h
>> +++ b/include/linux/pci_ids.h
>> @@ -580,6 +580,7 @@
>> #define PCI_DEVICE_ID_AMD_19H_M78H_DF_F3 0x12fb
>> #define PCI_DEVICE_ID_AMD_1AH_M00H_DF_F3 0x12c3
>> #define PCI_DEVICE_ID_AMD_1AH_M20H_DF_F3 0x16fb
>> +#define PCI_DEVICE_ID_AMD_1AH_M60H_DF_F3 0x124b
>> #define PCI_DEVICE_ID_AMD_1AH_M70H_DF_F3 0x12bb
>> #define PCI_DEVICE_ID_AMD_MI200_DF_F3 0x14d3
>> #define PCI_DEVICE_ID_AMD_MI300_DF_F3 0x152b
>> --
>
> I can confirm that the IDs are correct.
>
> Besides the formatting issues, this looks good to me.
>
> Reviewed-by: Yazen Ghannam <yazen.ghannam@xxxxxxx>>
> Thanks,
> Yazen
