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@xxxxxxx>
>> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@xxxxxxx>
>> ---
>> 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.
Regards,
Hans
