Hi Ilpo, Thank you for your review. On 7/8/2024 3:45 PM, Ilpo Järvinen wrote:
Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. On Thu, 27 Jun 2024, Suma Hegde wrote:Separate the probes for ACPI and platform device drivers. Provide a Kconfig option to select either the ACPI or the platform device based driver. Signed-off-by: Suma Hegde <suma.hegde@xxxxxxx> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@xxxxxxx> --- arch/x86/include/asm/amd_hsmp.h | 2 +- drivers/platform/x86/amd/hsmp/Kconfig | 25 ++++- drivers/platform/x86/amd/hsmp/Makefile | 10 +- drivers/platform/x86/amd/hsmp/acpi.c | 114 ++++++++++++++++++++++- drivers/platform/x86/amd/hsmp/hsmp.c | 25 ++--- drivers/platform/x86/amd/hsmp/hsmp.h | 8 +- drivers/platform/x86/amd/hsmp/plat.c | 122 +++++++------------------ 7 files changed, 183 insertions(+), 123 deletions(-) diff --git a/arch/x86/include/asm/amd_hsmp.h b/arch/x86/include/asm/amd_hsmp.h index 03c2ce3edaf5..ada14e55f9f4 100644 --- a/arch/x86/include/asm/amd_hsmp.h +++ b/arch/x86/include/asm/amd_hsmp.h @@ -5,7 +5,7 @@ #include <uapi/asm/amd_hsmp.h> -#if IS_ENABLED(CONFIG_AMD_HSMP) +#if IS_ENABLED(CONFIG_AMD_HSMP) || IS_ENABLED(CONFIG_AMD_HSMP_ACPI) int hsmp_send_message(struct hsmp_message *msg); #else static inline int hsmp_send_message(struct hsmp_message *msg) diff --git a/drivers/platform/x86/amd/hsmp/Kconfig b/drivers/platform/x86/amd/hsmp/Kconfig index b55d4ed9bceb..1cb10d2aac77 100644 --- a/drivers/platform/x86/amd/hsmp/Kconfig +++ b/drivers/platform/x86/amd/hsmp/Kconfig @@ -3,9 +3,30 @@ # AMD HSMP Driver # +menu "AMD Host System Management Port driver" + depends on AMD_NB + +config AMD_HSMP_ACPI + tristate "AMD HSMP ACPI driver" + depends on ACPI + help + The driver provides a way for user space tools to monitor and manage + system management functionality on EPYC server CPUs from AMD. + + Host System Management Port (HSMP) interface is a mailbox interface + between the x86 core and the System Management Unit (SMU) firmware. + + This driver supports ACPI based probing. + + You may enable this, if your platform bios provides an ACPI object + as described in the documentation. + + If you choose to compile this driver as a module the module will be + called amd_hsmp. + config AMD_HSMP tristate "AMD HSMP Driver" - depends on AMD_NB && X86_64 && ACPI + depends on !(AMD_HSMP_ACPI || AMD_HSMP_ACPI=m)depends on AMD_HSMP_ACPI=n is simpler.
ok, will change it to AMD_HSMP_ACPI=n.
I'm trying to keep the config symbols changes minimal. Existing users need not change their build setup. New ACPI users can include the ACPI one instead.help The driver provides a way for user space tools to monitor and manage system management functionality on EPYC server CPUs from AMD. @@ -15,3 +36,5 @@ config AMD_HSMP If you choose to compile this driver as a module the module will be called amd_hsmp. + +endmenu diff --git a/drivers/platform/x86/amd/hsmp/Makefile b/drivers/platform/x86/amd/hsmp/Makefile index 0cc92865c0a2..53ebc462b0f9 100644 --- a/drivers/platform/x86/amd/hsmp/Makefile +++ b/drivers/platform/x86/amd/hsmp/Makefile @@ -4,5 +4,11 @@ # AMD HSMP Driver # -obj-$(CONFIG_AMD_HSMP) += amd_hsmp.o -amd_hsmp-objs := hsmp.o plat.o acpi.o +ifneq ($(CONFIG_AMD_HSMP), ) +obj-$(CONFIG_AMD_HSMP) += amd_hsmp.o +amd_hsmp-objs = hsmp.o plat.o +endif +ifneq ($(CONFIG_AMD_HSMP_ACPI), ) +obj-$(CONFIG_AMD_HSMP_ACPI) += amd_hsmp.o +amd_hsmp-objs = hsmp.o acpi.o +endifYou should add a third symbol for the core that is selected to make the Makefile side less convoluted. The build system might work with something as simple as this (I'm not sure if ...-m postfix works for composite objects, ...-y is documented to work): obj-$(CONFIG_xx) += amd_hsmp.o amd_hsmp-$(CONFIG_xx) = hsmp.o amd_hsmp-$(CONFIG_AMD_HSMP) += plat.o amd_hsmp-$(CONFIG_AMD_HSMP_ACPI) += acpi.o
diff --git a/drivers/platform/x86/amd/hsmp/acpi.c b/drivers/platform/x86/amd/hsmp/acpi.c index 90bfa1ddadbf..0307f4e7176d 100644 --- a/drivers/platform/x86/amd/hsmp/acpi.c +++ b/drivers/platform/x86/amd/hsmp/acpi.c @@ -12,21 +12,34 @@ #include "hsmp.h" #include <linux/acpi.h> +#include <asm/amd_nb.h> +#include <linux/platform_device.h> + +#define DRIVER_NAME "amd_hsmp" +#define DRIVER_VERSION "2.3" +#define ACPI_HSMP_DEVICE_HID "AMDI0097" /* These are the strings specified in ACPI table */ #define MSG_IDOFF_STR "MsgIdOffset" #define MSG_ARGOFF_STR "MsgArgOffset" #define MSG_RESPOFF_STR "MsgRspOffset" -void amd_hsmp_acpi_rdwr(struct hsmp_socket *sock, u32 offset, - u32 *value, bool write) +static int 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); + return 0; } +static const struct file_operations hsmp_fops = { + .owner = THIS_MODULE, + .unlocked_ioctl = hsmp_ioctl, + .compat_ioctl = hsmp_ioctl, +}; + /* This is the UUID used for HSMP */ static const guid_t acpi_hsmp_uuid = GUID_INIT(0xb74d619d, 0x5707, 0x48bd, 0xa6, 0x9f, 0x4e, 0xa2, @@ -189,7 +202,7 @@ static int hsmp_parse_acpi_table(struct device *dev, u16 sock_ind) sock->sock_ind = sock_ind; sock->dev = dev; - plat_dev.is_acpi_device = true; + sock->amd_hsmp_rdwr = amd_hsmp_acpi_rdwr; sema_init(&sock->hsmp_sem, 1); @@ -202,7 +215,7 @@ static int hsmp_parse_acpi_table(struct device *dev, u16 sock_ind) return hsmp_read_acpi_dsd(sock); } -int hsmp_create_acpi_sysfs_if(struct device *dev) +static int hsmp_create_acpi_sysfs_if(struct device *dev) { struct attribute_group *attr_grp; u16 sock_ind; @@ -225,7 +238,7 @@ int hsmp_create_acpi_sysfs_if(struct device *dev) return devm_device_add_group(dev, attr_grp); } -int init_acpi(struct device *dev) +static int init_acpi(struct device *dev) { u16 sock_ind; int ret; @@ -259,3 +272,94 @@ int init_acpi(struct device *dev) return ret; } + +static const struct acpi_device_id amd_hsmp_acpi_ids[] = { + {ACPI_HSMP_DEVICE_HID, 0}, + {} +}; +MODULE_DEVICE_TABLE(acpi, amd_hsmp_acpi_ids); + +static bool check_acpi_support(struct device *dev) +{ + struct acpi_device *adev = ACPI_COMPANION(dev); + + if (adev && !acpi_match_device_ids(adev, amd_hsmp_acpi_ids)) + return true; + + return false; +} + +static int hsmp_acpi_probe(struct platform_device *pdev) +{ + int ret; + + if (!plat_dev.is_probed) { + plat_dev.num_sockets = amd_nb_num(); + if (plat_dev.num_sockets == 0 || plat_dev.num_sockets > MAX_AMD_SOCKETS) + return -ENODEV; + + plat_dev.sock = devm_kcalloc(&pdev->dev, plat_dev.num_sockets, + sizeof(*plat_dev.sock), + GFP_KERNEL); + if (!plat_dev.sock) + return -ENOMEM; + } + + if (!check_acpi_support(&pdev->dev)) { + dev_err(&pdev->dev, "Not ACPI device?\n"); + return -ENODEV; + } + + ret = init_acpi(&pdev->dev); + if (ret) { + dev_err(&pdev->dev, "Failed to initialize HSMP interface.\n"); + return ret; + } + + ret = hsmp_create_acpi_sysfs_if(&pdev->dev); + if (ret) + dev_err(&pdev->dev, "Failed to create HSMP sysfs interface\n"); + + if (!plat_dev.is_probed) { + 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; + + ret = misc_register(&plat_dev.hsmp_device); + if (ret) + return ret; + plat_dev.is_probed = true; + } + + return 0; +} + +static void hsmp_acpi_remove(struct platform_device *pdev) +{ + /* + * We register only one misc_device even on multi socket system.multi-socket
will correct it. Could you please also review the other patches in this patch series?
+ * So, deregister should happen only once. + */ + if (plat_dev.is_probed) { + misc_deregister(&plat_dev.hsmp_device); + plat_dev.is_probed = false; + } +} + +static struct platform_driver amd_hsmp_driver = { + .probe = hsmp_acpi_probe, + .remove_new = hsmp_acpi_remove, + .driver = { + .name = DRIVER_NAME, + .acpi_match_table = amd_hsmp_acpi_ids, + }, +}; + +module_platform_driver(amd_hsmp_driver); + +MODULE_DESCRIPTION("AMD HSMP Platform Interface Driver"); +MODULE_VERSION(DRIVER_VERSION); +MODULE_LICENSE("GPL v2");-- i.
Thanks and Regards, Suma
