Re: [PATCH v3 6/7] platform/x86: Convert to ACPI based probing

["Hegde, Suma" <Suma.Hegde@amd.com>]

Hi Hans,

On 1/2/2024 6:11 PM, Hans de Goede wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> Hi Suma,
>
> On 12/22/23 09:51, Ilpo Järvinen wrote:
>> On Thu, 21 Dec 2023, Suma Hegde wrote:
>>
>>> ACPI table provides mailbox base address and register offset
>>> information. The base address is provided as part of CRS method
>>> and mailbox offsets are provided through DSD table.
>>> Sockets are differentiated by UIDs.
>>>
>>> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
>>> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
>>> ---
>>> Changes since v1:
>>> 1. Define amd_hsmp_acpi_rdwr() for doing mailbox memory mapped io
>>> 2. Add a check to see if mailbox register offsets are set in
>>>     hsmp_read_acpi_dsd()
>>> 3. Add a check to see if sock->mbinfo.base_addr sockck->mbinfo.size are
>>>     set in hsmp_read_acpi_crs()
>>> 4. Change order of the statements in switch case ACPI_RESOURCE_TYPE_FIXED_MEMORY32
>>>     in hsmp_resource()
>>> 5. Add hsmp_test() after hsmp_parse_acpi_table() call
>>> 6. Add r.end < r.start check in hsmp_resource()
>>> 7. Add !dsd error check in hsmp_read_acpi_dsd
>>> Changes since v2:
>>> 1. Change EINVAL to ENODEV in hsmp_read_acpi_dsd()
>>> 2. Change EINVAL to ENOENT in hsmp_read_acpi_dsd()
>>> 3. Use resource_size() in hsmp_resource()
>>>
>>>   drivers/platform/x86/amd/hsmp.c | 324 +++++++++++++++++++++++++++++---
>>>   1 file changed, 296 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
>>> index e77d4cd83a07..46924c572055 100644
>>> --- a/drivers/platform/x86/amd/hsmp.c
>>> +++ b/drivers/platform/x86/amd/hsmp.c
>>> @@ -18,9 +18,11 @@
>>>   #include <linux/pci.h>
>>>   #include <linux/platform_device.h>
>>>   #include <linux/semaphore.h>
>>> +#include <linux/acpi.h>
>>>
>>>   #define DRIVER_NAME         "amd_hsmp"
>>> -#define DRIVER_VERSION              "2.0"
>>> +#define DRIVER_VERSION              "2.2"
>>> +#define ACPI_HSMP_DEVICE_HID        "AMDI0097"
>>>
>>>   /* HSMP Status / Error codes */
>>>   #define HSMP_STATUS_NOT_READY       0x00
>>> @@ -54,6 +56,11 @@
>>>
>>>   #define HSMP_ATTR_GRP_NAME_SIZE     10
>>>
>>> +/* These are the strings specified in ACPI table */
>>> +#define MSG_IDOFF_STR               "MsgIdOffset"
>>> +#define MSG_ARGOFF_STR              "MsgArgOffset"
>>> +#define MSG_RESPOFF_STR             "MsgRspOffset"
>>> +
>>>   struct hsmp_mbaddr_info {
>>>       u32 base_addr;
>>>       u32 msg_id_off;
>>> @@ -66,6 +73,7 @@ struct hsmp_socket {
>>>       struct bin_attribute hsmp_attr;
>>>       struct hsmp_mbaddr_info mbinfo;
>>>       void __iomem *metric_tbl_addr;
>>> +    void __iomem *virt_base_addr;
>>>       struct semaphore hsmp_sem;
>>>       char name[HSMP_ATTR_GRP_NAME_SIZE];
>>>       struct pci_dev *root;
>>> @@ -78,12 +86,14 @@ struct hsmp_plat_device {
>>>       struct hsmp_socket *sock;
>>>       u32 proto_ver;
>>>       u16 num_sockets;
>>> +    bool is_acpi_device;
>>> +    bool is_probed;
>>>   };
>>>
>>>   static struct hsmp_plat_device plat_dev;
>>>
>>> -static int amd_hsmp_rdwr(struct hsmp_socket *sock, u32 offset,
>>> -                     u32 *value, bool write)
>>> +static int amd_hsmp_pci_rdwr(struct hsmp_socket *sock, u32 offset,
>>> +                         u32 *value, bool write)
>>>   {
>>>       int ret;
>>>
>>> @@ -101,8 +111,29 @@ static int amd_hsmp_rdwr(struct hsmp_socket *sock, u32 offset,
>>>       return ret;
>>>   }
>>>
>>> +static void amd_hsmp_acpi_rdwr(struct hsmp_socket *sock, u32 offset,
>>> +                           u32 *value, bool write)
>>> +{
>>> +    if (write)
>>> +            iowrite32(*value, sock->virt_base_addr + offset);
>>> +    else
>>> +            *value = ioread32(sock->virt_base_addr + offset);
>>> +}
>>> +
>>> +static int amd_hsmp_rdwr(struct hsmp_socket *sock, u32 offset,
>>> +                     u32 *value, bool write)
>>> +{
>>> +    if (plat_dev.is_acpi_device)
>>> +            amd_hsmp_acpi_rdwr(sock, offset, value, write);
>>> +    else
>>> +            return amd_hsmp_pci_rdwr(sock, offset, value, write);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   /*
>>> - * Send a message to the HSMP port via PCI-e config space registers.
>>> + * Send a message to the HSMP port via PCI-e config space registers
>>> + * or by writing to MMIO space.
>>>    *
>>>    * The caller is expected to zero out any unused arguments.
>>>    * If a response is expected, the number of response words should be greater than 0.
>>> @@ -450,6 +481,9 @@ static int hsmp_create_sysfs_interface(void)
>>>       int ret;
>>>       u16 i;
>>>
>>> +    if (plat_dev.is_acpi_device)
>>> +            return 0;
>> This comes out of nowhere... Why proto_ver isn't enough to handle this?
>> If it's needed, would the check perhaps belong to
>> hsmp_is_sock_attr_visible() instead?
>>
>>>       /* String formatting is currently limited to u8 sockets */
>>>       if (WARN_ON(plat_dev.num_sockets > U8_MAX))
>>>               return -ERANGE;
>>> @@ -487,13 +521,188 @@ static int hsmp_create_sysfs_interface(void)
>>>       return devm_device_add_groups(plat_dev.sock[0].dev, hsmp_attr_grps);
>>>   }
>>>
>>> -static int hsmp_cache_proto_ver(void)
>>> +/* This is the UUID used for HSMP */
>>> +static const guid_t acpi_hsmp_uuid = GUID_INIT(0xb74d619d, 0x5707, 0x48bd,
>>> +                                            0xa6, 0x9f, 0x4e, 0xa2,
>>> +                                            0x87, 0x1f, 0xc2, 0xf6);
>>> +
>>> +static inline bool is_acpi_hsmp_uuid(union acpi_object *obj)
>>> +{
>>> +    if (obj->type == ACPI_TYPE_BUFFER && obj->buffer.length == 16)
>> 16 -> UUID_SIZE.
> Please submit a v4 addressing Ilpo's review remarks on this patch.
>
> And while at it please also fix the subject-prefix to be:
>
> platform/x86/amd/hsmp:
>
> As suggested by Thomas.
>
> There have been a bunch of drivers/platform/x86/amd changes merged
> recently, please base your v4 on top of:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
>
> to avoid conflicts.
Sure, I will send v4 with addressing review comments and re basing on top of

https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Thanks and Regards,

Suma

> Regards,
>
> Hans
>
>