On 10/23/2024 10:14, Shyam Sundar S K wrote:
On 10/23/2024 20:20, Mario Limonciello wrote:
On 10/23/2024 09:37, Shyam Sundar S K wrote:
On 10/23/2024 19:35, Mario Limonciello wrote:
On 10/23/2024 01:32, 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@xxxxxxx>
Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@xxxxxxx>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@xxxxxxx>
---
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..62f984fe40c6 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_err(pmf_dev->dev, "Failed to get I/O memory
resource\n");
here ^^^^^^^
+ 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");
This upgrades the previous message from debug to error.
It is dev_err() even before this change.
TL;DR I feel this error should stay as dev_dbg() if no function checks
are present for Smart PC.
I don't think it's necessarily an error though.
Smart PC checks are a bit different than the other checks. There
isn't a check for a bit being set to indicate the function is
supported.
So if the BIOS has the declaration for the region but it's not
populated it might not have a Smart PC policy and this shouldn't be
reported as a BIOS bug.
This should be included in the CPM package, and the BIOS team is
responsible for packaging a policy binary.
From a driver design standpoint, the absence of the policy binary
should be treated as an error, as there's no reason for the BIOS to
advertise the Smart PC bits without providing the policy binary.
Therefore, this should trigger a `dev_err()` and be considered a
BIOS bug.
OK I agree with this specific error, but I took a closer look at the
bug associated with
03cea821b82cb ("platform/x86/amd: pmf: Decrease error message to debug")
ah! but your comment was just inline to:
dev_err(pmf_dev->dev, "Incorrect policy params, possibly a SBIOS bug\n");
So, I was thinking you are saying to downgrade this to dev_dbg() and
hence the above clarification.
if the comment is for:
dev_err(pmf_dev->dev, "Failed to get I/O memory resource\n");
then Yes, I agree we should have dev_dbg() and I will respin a new
version.
It was originally for that line, but you corrected me.
It looks that we reached the conclusion on the right line that should be
fixed.
Thanks!
