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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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.

> +		return guid_equal((guid_t *)obj->buffer.pointer, &acpi_hsmp_uuid);
> +
> +	return false;
> +}


>  static int hsmp_pltdrv_probe(struct platform_device *pdev)
>  {
> +	struct acpi_device *adev;
> +	u16 sock_ind = 0;
>  	int ret;
>  
> -	plat_dev.sock = devm_kzalloc(&pdev->dev,
> -				     (plat_dev.num_sockets * sizeof(struct hsmp_socket)),
> -				     GFP_KERNEL);
> -	if (!plat_dev.sock)
> -		return -ENOMEM;
> -
> -	ret = init_socket_objects(&pdev->dev);
> -	if (ret)
> -		return ret;
> -
> -	plat_dev.hsmp_device.name	= HSMP_CDEV_NAME;
> -	plat_dev.hsmp_device.minor	= MISC_DYNAMIC_MINOR;
> -	plat_dev.hsmp_device.fops	= &hsmp_fops;
> -	plat_dev.hsmp_device.parent	= &pdev->dev;
> -	plat_dev.hsmp_device.nodename	= HSMP_DEVNODE_NAME;
> -	plat_dev.hsmp_device.mode	= 0644;
> +	/*
> +	 * On ACPI supported BIOS, there is an ACPI HSMP device added for
> +	 * each socket, so the per socket probing, but the memory allocated for
> +	 * sockets should be contiguous to access it as an array,
> +	 * Hence allocate memory for all the sockets at once instead of allocating
> +	 * on each probe.
> +	 */
> +	if (!plat_dev.is_probed) {
> +		plat_dev.sock = devm_kzalloc(&pdev->dev,
> +					     (plat_dev.num_sockets * sizeof(struct hsmp_socket)),
> +					     GFP_KERNEL);

I know you're just moving code around here so no need to do it in this 
patchset but this should use devm_kcalloc() and there are also extra 
parenthesis.


-- 
 i.