[PATCH v4 0/3] platform/x86/amd/pmf: Updates to AMD PMF driver

[PATCH v4 0/3] platform/x86/amd/pmf: Updates to AMD PMF driver

[Shyam Sundar S K]

Updates include:

- Add support for newer platforms
- Update the MAINTAINERS record, and
- Simplify certain code handling.


v4:
----
- Update Reviewed-by tags
- Address Mario comments
- Drop 5/5 of v3 and look for better alternatives for firmware loading.
- Drop 1/5 of v3 as it is already tagged for stable.

v3:
----
- Add a comment to answer "why" resource_size() is not used for getting the
  ACPI resource length.

v2:
----
- drop resource_size() usage
- use right format specifier

Shyam Sundar S K (3):
  platform/x86/amd/pmf: Use dev_err_probe() to simplify error handling
  MAINTAINERS: Change AMD PMF driver status to "Supported"
  platform/x86/amd/pmf: Switch to platform_get_resource() and
    devm_ioremap_resource()

 MAINTAINERS                           |  2 +-
 drivers/platform/x86/amd/pmf/Kconfig  |  1 +
 drivers/platform/x86/amd/pmf/acpi.c   | 46 +++++++++++----------------
 drivers/platform/x86/amd/pmf/core.c   |  8 ++---
 drivers/platform/x86/amd/pmf/pmf.h    |  6 ++--
 drivers/platform/x86/amd/pmf/tee-if.c |  8 ++---
 6 files changed, 33 insertions(+), 38 deletions(-)

-- 
2.34.1

[PATCH v4 1/3] platform/x86/amd/pmf: Use dev_err_probe() to simplify error handling

[Shyam Sundar S K]

To simplify error handling in the amd_pmf probe function and reduce code
size, dev_err() is replaced with dev_err_probe().

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmf/core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
index d6af0ca036f1..7ab8b7f74629 100644
--- a/drivers/platform/x86/amd/pmf/core.c
+++ b/drivers/platform/x86/amd/pmf/core.c
@@ -429,18 +429,18 @@ static int amd_pmf_probe(struct platform_device *pdev)
 
 	err = amd_smn_read(0, AMD_PMF_BASE_ADDR_LO, &val);
 	if (err) {
-		dev_err(dev->dev, "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_LO);
 		pci_dev_put(rdev);
-		return pcibios_err_to_errno(err);
+		return dev_err_probe(dev->dev, pcibios_err_to_errno(err),
+				     "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_LO);
 	}
 
 	base_addr_lo = val & AMD_PMF_BASE_ADDR_HI_MASK;
 
 	err = amd_smn_read(0, AMD_PMF_BASE_ADDR_HI, &val);
 	if (err) {
-		dev_err(dev->dev, "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_HI);
 		pci_dev_put(rdev);
-		return pcibios_err_to_errno(err);
+		return dev_err_probe(dev->dev, pcibios_err_to_errno(err),
+				     "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_HI);
 	}
 
 	base_addr_hi = val & AMD_PMF_BASE_ADDR_LO_MASK;
-- 
2.34.1

[PATCH v4 2/3] MAINTAINERS: Change AMD PMF driver status to "Supported"

[Shyam Sundar S K]

The AMD PMF driver is actively being developed, so the MAINTAINERS record
should reflect "Supported" instead of "Maintained." Update the MAINTAINERS
database to reflect this change.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index a27407950242..87f67c52d68c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1124,7 +1124,7 @@ F:	drivers/platform/x86/amd/pmc/
 AMD PMF DRIVER
 M:	Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
 L:	platform-driver-x86@vger.kernel.org
-S:	Maintained
+S:	Supported
 F:	Documentation/ABI/testing/sysfs-amd-pmf
 F:	drivers/platform/x86/amd/pmf/
 
-- 
2.34.1

[PATCH v4 3/3] platform/x86/amd/pmf: Switch to platform_get_resource() and devm_ioremap_resource()

[Shyam Sundar S K]

Use platform_get_resource() to fetch the memory resource instead of
acpi_walk_resources() and devm_ioremap_resource() for mapping the
resources.

PS: We cannot use resource_size() here because it adds an extra byte to
round off the size. In the case of PMF ResourceTemplate(), this rounding
is already handled within the _CRS. Using resource_size() would increase
the resource size by 1, causing a mismatch with the length field and
leading to issues. Therefore, simply use end-start of the ACPI resource to
obtain the actual length.

Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmf/Kconfig  |  1 +
 drivers/platform/x86/amd/pmf/acpi.c   | 46 +++++++++++----------------
 drivers/platform/x86/amd/pmf/pmf.h    |  6 ++--
 drivers/platform/x86/amd/pmf/tee-if.c |  8 ++---
 4 files changed, 28 insertions(+), 33 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
index f4fa8bd8bda8..99d67cdbd91e 100644
--- a/drivers/platform/x86/amd/pmf/Kconfig
+++ b/drivers/platform/x86/amd/pmf/Kconfig
@@ -11,6 +11,7 @@ config AMD_PMF
 	select ACPI_PLATFORM_PROFILE
 	depends on TEE && AMDTEE
 	depends on AMD_SFH_HID
+	depends on HAS_IOMEM
 	help
 	  This driver provides support for the AMD Platform Management Framework.
 	  The goal is to enhance end user experience by making AMD PCs smarter,
diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
index d5b496433d69..1b9c7acf0ddf 100644
--- a/drivers/platform/x86/amd/pmf/acpi.c
+++ b/drivers/platform/x86/amd/pmf/acpi.c
@@ -433,37 +433,29 @@ int apmf_install_handler(struct amd_pmf_dev *pmf_dev)
 	return 0;
 }
 
-static acpi_status apmf_walk_resources(struct acpi_resource *res, void *data)
+int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
 {
-	struct amd_pmf_dev *dev = data;
+	struct platform_device *pdev = to_platform_device(pmf_dev->dev);
 
-	switch (res->type) {
-	case ACPI_RESOURCE_TYPE_ADDRESS64:
-		dev->policy_addr = res->data.address64.address.minimum;
-		dev->policy_sz = res->data.address64.address.address_length;
-		break;
-	case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
-		dev->policy_addr = res->data.fixed_memory32.address;
-		dev->policy_sz = res->data.fixed_memory32.address_length;
-		break;
-	}
-
-	if (!dev->policy_addr || dev->policy_sz > POLICY_BUF_MAX_SZ || dev->policy_sz == 0) {
-		pr_err("Incorrect Policy params, possibly a SBIOS bug\n");
-		return AE_ERROR;
+	pmf_dev->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!pmf_dev->res) {
+		dev_dbg(pmf_dev->dev, "Failed to get I/O memory resource\n");
+		return -EINVAL;
 	}
 
-	return AE_OK;
-}
-
-int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
-{
-	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
-	acpi_status status;
-
-	status = acpi_walk_resources(ahandle, METHOD_NAME__CRS, apmf_walk_resources, pmf_dev);
-	if (ACPI_FAILURE(status)) {
-		dev_dbg(pmf_dev->dev, "acpi_walk_resources failed :%d\n", status);
+	pmf_dev->policy_addr = pmf_dev->res->start;
+	/*
+	 * We cannot use resource_size() here because it adds an extra byte to round off the size.
+	 * In the case of PMF ResourceTemplate(), this rounding is already handled within the _CRS.
+	 * Using resource_size() would increase the resource size by 1, causing a mismatch with the
+	 * length field and leading to issues. Therefore, simply use end-start of the ACPI resource
+	 * to obtain the actual length.
+	 */
+	pmf_dev->policy_sz = pmf_dev->res->end - pmf_dev->res->start;
+
+	if (!pmf_dev->policy_addr || pmf_dev->policy_sz > POLICY_BUF_MAX_SZ ||
+	    pmf_dev->policy_sz == 0) {
+		dev_err(pmf_dev->dev, "Incorrect policy params, possibly a SBIOS bug\n");
 		return -EINVAL;
 	}
 
diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index 8ce8816da9c1..a79808fda1d8 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -13,6 +13,7 @@
 
 #include <linux/acpi.h>
 #include <linux/input.h>
+#include <linux/platform_device.h>
 #include <linux/platform_profile.h>
 
 #define POLICY_BUF_MAX_SZ		0x4b000
@@ -355,19 +356,20 @@ struct amd_pmf_dev {
 	/* Smart PC solution builder */
 	struct dentry *esbin;
 	unsigned char *policy_buf;
-	u32 policy_sz;
+	resource_size_t policy_sz;
 	struct tee_context *tee_ctx;
 	struct tee_shm *fw_shm_pool;
 	u32 session_id;
 	void *shbuf;
 	struct delayed_work pb_work;
 	struct pmf_action_table *prev_data;
-	u64 policy_addr;
+	resource_size_t policy_addr;
 	void __iomem *policy_base;
 	bool smart_pc_enabled;
 	u16 pmf_if_version;
 	struct input_dev *pmf_idev;
 	size_t mtable_size;
+	struct resource *res;
 };
 
 struct apmf_sps_prop_granular_v2 {
diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
index 19c27b6e4666..555b8d6314e0 100644
--- a/drivers/platform/x86/amd/pmf/tee-if.c
+++ b/drivers/platform/x86/amd/pmf/tee-if.c
@@ -257,7 +257,7 @@ static int amd_pmf_invoke_cmd_init(struct amd_pmf_dev *dev)
 		return -ENODEV;
 	}
 
-	dev_dbg(dev->dev, "Policy Binary size: %u bytes\n", dev->policy_sz);
+	dev_dbg(dev->dev, "Policy Binary size: %llu bytes\n", dev->policy_sz);
 	memset(dev->shbuf, 0, dev->policy_sz);
 	ta_sm = dev->shbuf;
 	in = &ta_sm->pmf_input.init_table;
@@ -512,9 +512,9 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
 	if (ret)
 		goto error;
 
-	dev->policy_base = devm_ioremap(dev->dev, dev->policy_addr, dev->policy_sz);
-	if (!dev->policy_base) {
-		ret = -ENOMEM;
+	dev->policy_base = devm_ioremap_resource(dev->dev, dev->res);
+	if (IS_ERR(dev->policy_base)) {
+		ret = PTR_ERR(dev->policy_base);
 		goto error;
 	}
 
-- 
2.34.1

Re: [PATCH v4 3/3] platform/x86/amd/pmf: Switch to platform_get_resource() and devm_ioremap_resource()

[Ilpo Järvinen]

On Mon, 4 Nov 2024, Shyam Sundar S K wrote:

> Use platform_get_resource() to fetch the memory resource instead of
> acpi_walk_resources() and devm_ioremap_resource() for mapping the
> resources.
> 
> PS: We cannot use resource_size() here because it adds an extra byte to
> round off the size. In the case of PMF ResourceTemplate(), this rounding
> is already handled within the _CRS. Using resource_size() would increase
> the resource size by 1, causing a mismatch with the length field and
> leading to issues. Therefore, simply use end-start of the ACPI resource to
> obtain the actual length.
> 
> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd/pmf/Kconfig  |  1 +
>  drivers/platform/x86/amd/pmf/acpi.c   | 46 +++++++++++----------------
>  drivers/platform/x86/amd/pmf/pmf.h    |  6 ++--
>  drivers/platform/x86/amd/pmf/tee-if.c |  8 ++---
>  4 files changed, 28 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
> index f4fa8bd8bda8..99d67cdbd91e 100644
> --- a/drivers/platform/x86/amd/pmf/Kconfig
> +++ b/drivers/platform/x86/amd/pmf/Kconfig
> @@ -11,6 +11,7 @@ config AMD_PMF
>  	select ACPI_PLATFORM_PROFILE
>  	depends on TEE && AMDTEE
>  	depends on AMD_SFH_HID
> +	depends on HAS_IOMEM
>  	help
>  	  This driver provides support for the AMD Platform Management Framework.
>  	  The goal is to enhance end user experience by making AMD PCs smarter,
> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
> index d5b496433d69..1b9c7acf0ddf 100644
> --- a/drivers/platform/x86/amd/pmf/acpi.c
> +++ b/drivers/platform/x86/amd/pmf/acpi.c
> @@ -433,37 +433,29 @@ int apmf_install_handler(struct amd_pmf_dev *pmf_dev)
>  	return 0;
>  }
>  
> -static acpi_status apmf_walk_resources(struct acpi_resource *res, void *data)
> +int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
>  {
> -	struct amd_pmf_dev *dev = data;
> +	struct platform_device *pdev = to_platform_device(pmf_dev->dev);
>  
> -	switch (res->type) {
> -	case ACPI_RESOURCE_TYPE_ADDRESS64:
> -		dev->policy_addr = res->data.address64.address.minimum;
> -		dev->policy_sz = res->data.address64.address.address_length;
> -		break;
> -	case ACPI_RESOURCE_TYPE_FIXED_MEMORY32:
> -		dev->policy_addr = res->data.fixed_memory32.address;
> -		dev->policy_sz = res->data.fixed_memory32.address_length;
> -		break;
> -	}
> -
> -	if (!dev->policy_addr || dev->policy_sz > POLICY_BUF_MAX_SZ || dev->policy_sz == 0) {
> -		pr_err("Incorrect Policy params, possibly a SBIOS bug\n");
> -		return AE_ERROR;
> +	pmf_dev->res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!pmf_dev->res) {
> +		dev_dbg(pmf_dev->dev, "Failed to get I/O memory resource\n");
> +		return -EINVAL;
>  	}
>  
> -	return AE_OK;
> -}
> -
> -int apmf_check_smart_pc(struct amd_pmf_dev *pmf_dev)
> -{
> -	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
> -	acpi_status status;
> -
> -	status = acpi_walk_resources(ahandle, METHOD_NAME__CRS, apmf_walk_resources, pmf_dev);
> -	if (ACPI_FAILURE(status)) {
> -		dev_dbg(pmf_dev->dev, "acpi_walk_resources failed :%d\n", status);
> +	pmf_dev->policy_addr = pmf_dev->res->start;
> +	/*
> +	 * We cannot use resource_size() here because it adds an extra byte to round off the size.
> +	 * In the case of PMF ResourceTemplate(), this rounding is already handled within the _CRS.
> +	 * Using resource_size() would increase the resource size by 1, causing a mismatch with the
> +	 * length field and leading to issues. Therefore, simply use end-start of the ACPI resource
> +	 * to obtain the actual length.
> +	 */
> +	pmf_dev->policy_sz = pmf_dev->res->end - pmf_dev->res->start;
> +
> +	if (!pmf_dev->policy_addr || pmf_dev->policy_sz > POLICY_BUF_MAX_SZ ||
> +	    pmf_dev->policy_sz == 0) {
> +		dev_err(pmf_dev->dev, "Incorrect policy params, possibly a SBIOS bug\n");
>  		return -EINVAL;
>  	}
>  
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 8ce8816da9c1..a79808fda1d8 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -13,6 +13,7 @@
>  
>  #include <linux/acpi.h>
>  #include <linux/input.h>
> +#include <linux/platform_device.h>
>  #include <linux/platform_profile.h>
>  
>  #define POLICY_BUF_MAX_SZ		0x4b000
> @@ -355,19 +356,20 @@ struct amd_pmf_dev {
>  	/* Smart PC solution builder */
>  	struct dentry *esbin;
>  	unsigned char *policy_buf;
> -	u32 policy_sz;
> +	resource_size_t policy_sz;
>  	struct tee_context *tee_ctx;
>  	struct tee_shm *fw_shm_pool;
>  	u32 session_id;
>  	void *shbuf;
>  	struct delayed_work pb_work;
>  	struct pmf_action_table *prev_data;
> -	u64 policy_addr;
> +	resource_size_t policy_addr;
>  	void __iomem *policy_base;
>  	bool smart_pc_enabled;
>  	u16 pmf_if_version;
>  	struct input_dev *pmf_idev;
>  	size_t mtable_size;
> +	struct resource *res;
>  };
>  
>  struct apmf_sps_prop_granular_v2 {
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index 19c27b6e4666..555b8d6314e0 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -257,7 +257,7 @@ static int amd_pmf_invoke_cmd_init(struct amd_pmf_dev *dev)
>  		return -ENODEV;
>  	}
>  
> -	dev_dbg(dev->dev, "Policy Binary size: %u bytes\n", dev->policy_sz);
> +	dev_dbg(dev->dev, "Policy Binary size: %llu bytes\n", dev->policy_sz);

Hi,

I applied these to review-ilpo. While applying, I added cast to this line 
since %llu and resource_size_t are not guaranteed to be same.

I should really find time to look at adding a printf formatter for 
resource sizes as Andy once suggested.

-- 
 i.

>  	memset(dev->shbuf, 0, dev->policy_sz);
>  	ta_sm = dev->shbuf;
>  	in = &ta_sm->pmf_input.init_table;
> @@ -512,9 +512,9 @@ int amd_pmf_init_smart_pc(struct amd_pmf_dev *dev)
>  	if (ret)
>  		goto error;
>  
> -	dev->policy_base = devm_ioremap(dev->dev, dev->policy_addr, dev->policy_sz);
> -	if (!dev->policy_base) {
> -		ret = -ENOMEM;
> +	dev->policy_base = devm_ioremap_resource(dev->dev, dev->res);
> +	if (IS_ERR(dev->policy_base)) {
> +		ret = PTR_ERR(dev->policy_base);
>  		goto error;
>  	}
>  
>