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

[Hans de Goede <hdegoede@redhat.com>]

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.

Regards,

Hans