[PATCH 0/2] soc: hisilicon: Add power control support for kunpeng HBM

[PATCH 0/2] soc: hisilicon: Add power control support for kunpeng HBM

[Zhang Zekun]

Add power control support for High Bandwidth Memory (HBM) for Kunpeng SoC
platform. HBM devices on Kunpeng SoC can provide higher bandwidth at the
cost of higher power consumption. Providing power control methods can help
reducing the power when the workload does not need use HBM.

Zhang Zekun (2):
  soc: hisilicon: kunpeng_hbmdev: Add support for controling the power
    of hbm memory
  soc: hisilicon: kunpeng_hbmcache: Add support for online and offline
    the hbm cache

 MAINTAINERS                              |   7 +
 drivers/soc/hisilicon/Kconfig            |  23 +++
 drivers/soc/hisilicon/Makefile           |   2 +
 drivers/soc/hisilicon/kunpeng_hbm.h      |  31 ++++
 drivers/soc/hisilicon/kunpeng_hbmcache.c | 136 +++++++++++++++
 drivers/soc/hisilicon/kunpeng_hbmdev.c   | 210 +++++++++++++++++++++++
 6 files changed, 409 insertions(+)
 create mode 100644 drivers/soc/hisilicon/kunpeng_hbm.h
 create mode 100644 drivers/soc/hisilicon/kunpeng_hbmcache.c
 create mode 100644 drivers/soc/hisilicon/kunpeng_hbmdev.c

-- 
2.17.1

[PATCH 1/2] soc: hisilicon: kunpeng_hbmdev: Add support for controling the power of hbm memory

[Zhang Zekun]

Add a driver for High Bandwidth Memory (HBM) devices, which will provide
user space interfaces to power on/off the HBM devices. In Kunpeng servers,
we need to control the power of HBM devices which can be power consuming
and will only be used in some specialized scenarios, such as HPC. HBM
memory devices in a socket are in the same power domain, and should be
power off/on together.

HBM devices will be configured with ACPI device id "PNP0C80", and be used
as a cpuless numa node. HBM devices in the same power domain will be put
into the same container. ACPI function "_ON" and "_OFF" are reponsible
for power on/off the HBM device, and notify the OS to fully online/offline
the HBM memory.

Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
---
 MAINTAINERS                            |   6 +
 drivers/soc/hisilicon/Kconfig          |  12 ++
 drivers/soc/hisilicon/Makefile         |   1 +
 drivers/soc/hisilicon/kunpeng_hbm.h    |  31 ++++
 drivers/soc/hisilicon/kunpeng_hbmdev.c | 210 +++++++++++++++++++++++++
 5 files changed, 260 insertions(+)
 create mode 100644 drivers/soc/hisilicon/kunpeng_hbm.h
 create mode 100644 drivers/soc/hisilicon/kunpeng_hbmdev.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 0456a33ef657..e8b4cf7d7162 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10283,6 +10283,12 @@ F:	Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs
 F:	drivers/soc/hisilicon/kunpeng_hccs.c
 F:	drivers/soc/hisilicon/kunpeng_hccs.h
 
+HISILICON KUNPENG SOC KUNPENG HBMDEV DRIVER
+M:	Zhang Zekun <zhangzekun11@huawei.com>
+S:	Maintained
+F:	drivers/soc/hisilicon/kunpeng_hbm.h
+F:	drivers/soc/hisilicon/kunpeng_hbmdev.c
+
 HISILICON LPC BUS DRIVER
 M:	Jay Fang <f.fangjian@huawei.com>
 S:	Maintained
diff --git a/drivers/soc/hisilicon/Kconfig b/drivers/soc/hisilicon/Kconfig
index 6d7c244d2e78..b3ca7d6f5d01 100644
--- a/drivers/soc/hisilicon/Kconfig
+++ b/drivers/soc/hisilicon/Kconfig
@@ -21,4 +21,16 @@ config KUNPENG_HCCS
 	  health status and port information of HCCS, or reducing system
 	  power consumption on Kunpeng SoC.
 
+config KUNPENG_HBMDEV
+	bool "add extra support for hbm memory device"
+	depends on ACPI_HOTPLUG_MEMORY
+	select ACPI_CONTAINER
+	help
+	  The driver provides methods for userpace to control the power
+	  of HBM memory devices on Kunpeng soc, which can help to save
+	  energy. The functionality of the driver would require dedicated
+	  BIOS configuration.
+
+	  If not sure, say N.
+
 endmenu
diff --git a/drivers/soc/hisilicon/Makefile b/drivers/soc/hisilicon/Makefile
index 226e747e70d6..08048d73586e 100644
--- a/drivers/soc/hisilicon/Makefile
+++ b/drivers/soc/hisilicon/Makefile
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_KUNPENG_HCCS)	+= kunpeng_hccs.o
+obj-$(CONFIG_KUNPENG_HBMDEV)	+= kunpeng_hbmdev.o
diff --git a/drivers/soc/hisilicon/kunpeng_hbm.h b/drivers/soc/hisilicon/kunpeng_hbm.h
new file mode 100644
index 000000000000..ef306c888480
--- /dev/null
+++ b/drivers/soc/hisilicon/kunpeng_hbm.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2024. Huawei Technologies Co., Ltd
+ */
+
+#ifndef _HISI_INTERNAL_H
+#define _HISI_INTERNAL_H
+
+enum {
+	STATE_ONLINE,
+	STATE_OFFLINE,
+};
+
+static const char *const online_type_to_str[] = {
+	[STATE_ONLINE] = "online",
+	[STATE_OFFLINE] = "offline",
+};
+
+static inline int online_type_from_str(const char *str)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(online_type_to_str); i++) {
+		if (sysfs_streq(str, online_type_to_str[i]))
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+#endif
diff --git a/drivers/soc/hisilicon/kunpeng_hbmdev.c b/drivers/soc/hisilicon/kunpeng_hbmdev.c
new file mode 100644
index 000000000000..1945676ff502
--- /dev/null
+++ b/drivers/soc/hisilicon/kunpeng_hbmdev.c
@@ -0,0 +1,210 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2024 Huawei Technologies Co., Ltd
+ */
+
+#include <linux/kobject.h>
+#include <linux/module.h>
+#include <linux/nodemask.h>
+#include <linux/acpi.h>
+#include <linux/container.h>
+
+#include "kunpeng_hbm.h"
+
+#define ACPI_MEMORY_DEVICE_HID			"PNP0C80"
+#define ACPI_GENERIC_CONTAINER_DEVICE_HID	"PNP0A06"
+
+struct cdev_node {
+	struct device *dev;
+	struct list_head clist;
+};
+
+struct cdev_node cdev_list;
+
+static int get_pxm(struct acpi_device *acpi_device, void *arg)
+{
+	acpi_handle handle = acpi_device->handle;
+	nodemask_t *mask = arg;
+	unsigned long long sta;
+	acpi_status status;
+	int nid;
+
+	status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
+	if (ACPI_SUCCESS(status) && (sta & ACPI_STA_DEVICE_ENABLED)) {
+		nid = acpi_get_node(handle);
+		if (nid != NUMA_NO_NODE)
+			node_set(nid, *mask);
+	}
+
+	return 0;
+}
+
+static ssize_t pxms_show(struct device *dev,
+			 struct device_attribute *attr,
+			 char *buf)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	nodemask_t mask;
+
+	nodes_clear(mask);
+	acpi_dev_for_each_child(adev, get_pxm, &mask);
+
+	return sysfs_emit(buf, "%*pbl\n", nodemask_pr_args(&mask));
+}
+static DEVICE_ATTR_RO(pxms);
+
+static int memdev_power_on(struct acpi_device *adev)
+{
+	acpi_handle handle = adev->handle;
+	acpi_status status;
+
+	/* Power on and online the devices */
+	status = acpi_evaluate_object(handle, "_ON", NULL, NULL);
+	if (ACPI_FAILURE(status)) {
+		acpi_handle_warn(handle, "Power on failed (0x%x)\n", status);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int hbmdev_check(struct acpi_device *adev, void *arg)
+{
+	const char *hid = acpi_device_hid(adev);
+
+	if (!strcmp(hid, ACPI_MEMORY_DEVICE_HID)) {
+		bool *found = arg;
+		*found = true;
+		return -1;
+	}
+
+	return 0;
+}
+
+static int memdev_power_off(struct acpi_device *adev)
+{
+	acpi_handle handle = adev->handle;
+	acpi_status status;
+
+	/* Eject the devices and power off */
+	status = acpi_evaluate_object(handle, "_OFF", NULL, NULL);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	return 0;
+}
+
+static ssize_t state_store(struct device *dev, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	const int type = online_type_from_str(buf);
+	int ret = -EINVAL;
+
+	/*
+	 * Take the lock to avoid race on underlying PCC operation region
+	 * used in ACPI function "_ON" and "_OFF".
+	 */
+	ret = lock_device_hotplug_sysfs();
+	if (ret)
+		return ret;
+
+	switch (type) {
+	case STATE_ONLINE:
+		ret = memdev_power_on(adev);
+		break;
+	case STATE_OFFLINE:
+		ret  = memdev_power_off(adev);
+		break;
+	default:
+		break;
+	}
+	unlock_device_hotplug();
+
+	if (ret)
+		return ret;
+
+	return count;
+}
+static DEVICE_ATTR_WO(state);
+
+static bool has_hbmdev(struct device *dev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	const char *hid = acpi_device_hid(adev);
+	bool found = false;
+
+	if (strcmp(hid, ACPI_GENERIC_CONTAINER_DEVICE_HID))
+		return found;
+
+	acpi_dev_for_each_child(adev, hbmdev_check, &found);
+	return found;
+}
+
+static int container_add(struct device *dev, void *data)
+{
+	struct cdev_node *cnode;
+
+	if (!has_hbmdev(dev))
+		return 0;
+
+	cnode = kmalloc(sizeof(struct cdev_node), GFP_KERNEL);
+	if (!cnode)
+		return -ENOMEM;
+
+	cnode->dev = dev;
+	list_add_tail(&cnode->clist, &cdev_list.clist);
+
+	return 0;
+}
+
+static void container_remove(void)
+{
+	struct cdev_node *cnode, *tmp;
+
+	list_for_each_entry_safe(cnode, tmp, &cdev_list.clist, clist) {
+		device_remove_file(cnode->dev, &dev_attr_state);
+		device_remove_file(cnode->dev, &dev_attr_pxms);
+		list_del(&cnode->clist);
+		kfree(cnode);
+	}
+}
+
+static int container_init(void)
+{
+	struct cdev_node *cnode;
+
+	INIT_LIST_HEAD(&cdev_list.clist);
+
+	if (bus_for_each_dev(&container_subsys, NULL, NULL, container_add)) {
+		container_remove();
+		return -ENOMEM;
+	}
+
+	if (list_empty(&cdev_list.clist))
+		return -ENODEV;
+
+	list_for_each_entry(cnode, &cdev_list.clist, clist) {
+		device_create_file(cnode->dev, &dev_attr_state);
+		device_create_file(cnode->dev, &dev_attr_pxms);
+	}
+
+	return 0;
+}
+
+static struct acpi_platform_list kunpeng_hbm_plat_info[] = {
+	{"HISI  ", "HIP11   ", 0, ACPI_SIG_IORT, all_versions, NULL, 0},
+	{ }
+};
+
+static int __init hbmdev_init(void)
+{
+	if (acpi_match_platform_list(kunpeng_hbm_plat_info) < 0)
+		return 0;
+
+	return container_init();
+}
+module_init(hbmdev_init);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Zhang Zekun <zhangzekun11@huawei.com>");
-- 
2.17.1

Re: [PATCH 1/2] soc: hisilicon: kunpeng_hbmdev: Add support for controling the power of hbm memory

[Alex Elder]

On 12/6/24 5:28 AM, Zhang Zekun wrote:
> Add a driver for High Bandwidth Memory (HBM) devices, which will provide
> user space interfaces to power on/off the HBM devices. In Kunpeng servers,
> we need to control the power of HBM devices which can be power consuming
> and will only be used in some specialized scenarios, such as HPC. HBM
> memory devices in a socket are in the same power domain, and should be
> power off/on together.
> 
> HBM devices will be configured with ACPI device id "PNP0C80", and be used
> as a cpuless numa node. HBM devices in the same power domain will be put
> into the same container. ACPI function "_ON" and "_OFF" are reponsible
> for power on/off the HBM device, and notify the OS to fully online/offline
> the HBM memory.
> 
> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
> ---
>   MAINTAINERS                            |   6 +
>   drivers/soc/hisilicon/Kconfig          |  12 ++
>   drivers/soc/hisilicon/Makefile         |   1 +
>   drivers/soc/hisilicon/kunpeng_hbm.h    |  31 ++++
>   drivers/soc/hisilicon/kunpeng_hbmdev.c | 210 +++++++++++++++++++++++++
>   5 files changed, 260 insertions(+)
>   create mode 100644 drivers/soc/hisilicon/kunpeng_hbm.h
>   create mode 100644 drivers/soc/hisilicon/kunpeng_hbmdev.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0456a33ef657..e8b4cf7d7162 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10283,6 +10283,12 @@ F:	Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs
>   F:	drivers/soc/hisilicon/kunpeng_hccs.c
>   F:	drivers/soc/hisilicon/kunpeng_hccs.h
>   
> +HISILICON KUNPENG SOC KUNPENG HBMDEV DRIVER
> +M:	Zhang Zekun <zhangzekun11@huawei.com>
> +S:	Maintained
> +F:	drivers/soc/hisilicon/kunpeng_hbm.h
> +F:	drivers/soc/hisilicon/kunpeng_hbmdev.c
> +
>   HISILICON LPC BUS DRIVER
>   M:	Jay Fang <f.fangjian@huawei.com>
>   S:	Maintained
> diff --git a/drivers/soc/hisilicon/Kconfig b/drivers/soc/hisilicon/Kconfig
> index 6d7c244d2e78..b3ca7d6f5d01 100644
> --- a/drivers/soc/hisilicon/Kconfig
> +++ b/drivers/soc/hisilicon/Kconfig
> @@ -21,4 +21,16 @@ config KUNPENG_HCCS
>   	  health status and port information of HCCS, or reducing system
>   	  power consumption on Kunpeng SoC.
>   
> +config KUNPENG_HBMDEV
> +	bool "add extra support for hbm memory device"

s/add extra/Add/
s/hbm/HBM

Can there be more than one HBM memory device?  If so:
s/device/devices/

> +	depends on ACPI_HOTPLUG_MEMORY
> +	select ACPI_CONTAINER
> +	help
> +	  The driver provides methods for userpace to control the power
> +	  of HBM memory devices on Kunpeng soc, which can help to save

Perhaps you can expand "HBM" here to be "high-bandwidth memory (HBM)".

> +	  energy. The functionality of the driver would require dedicated
> +	  BIOS configuration.
> +
> +	  If not sure, say N.
> +
>   endmenu
> diff --git a/drivers/soc/hisilicon/Makefile b/drivers/soc/hisilicon/Makefile
> index 226e747e70d6..08048d73586e 100644
> --- a/drivers/soc/hisilicon/Makefile
> +++ b/drivers/soc/hisilicon/Makefile
> @@ -1,2 +1,3 @@
>   # SPDX-License-Identifier: GPL-2.0-only
>   obj-$(CONFIG_KUNPENG_HCCS)	+= kunpeng_hccs.o
> +obj-$(CONFIG_KUNPENG_HBMDEV)	+= kunpeng_hbmdev.o
> diff --git a/drivers/soc/hisilicon/kunpeng_hbm.h b/drivers/soc/hisilicon/kunpeng_hbm.h
> new file mode 100644
> index 000000000000..ef306c888480
> --- /dev/null
> +++ b/drivers/soc/hisilicon/kunpeng_hbm.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2024. Huawei Technologies Co., Ltd
> + */
> +
> +#ifndef _HISI_INTERNAL_H
> +#define _HISI_INTERNAL_H
> +
> +enum {
> +	STATE_ONLINE,

While it technically doesn't matter, I would rather see 0 mean
offline, 1 mean online.  It suggests that the default state is
most likely offline as well.

> +	STATE_OFFLINE,

Do you anticipate that the state of an HBM device will be
anything other than online or offline?  (For example, it
could be in error state, or some other degraded state or
something.)  If not, this would be better implemented
simply as a Boolean attribute instead (with a name that
makes sense, such as "online" rather than "state").  It
would eliminate the need for the function defined below.

> +};
> +
> +static const char *const online_type_to_str[] = {
> +	[STATE_ONLINE] = "online",
> +	[STATE_OFFLINE] = "offline",
> +};
> +
> +static inline int online_type_from_str(const char *str)

Why is this inlined here, rather than just making it a
proper function?

> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(online_type_to_str); i++) {
> +		if (sysfs_streq(str, online_type_to_str[i]))
> +			return i;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +#endif
> diff --git a/drivers/soc/hisilicon/kunpeng_hbmdev.c b/drivers/soc/hisilicon/kunpeng_hbmdev.c
> new file mode 100644
> index 000000000000..1945676ff502
> --- /dev/null
> +++ b/drivers/soc/hisilicon/kunpeng_hbmdev.c
> @@ -0,0 +1,210 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024 Huawei Technologies Co., Ltd
> + */
> +
> +#include <linux/kobject.h>
> +#include <linux/module.h>
> +#include <linux/nodemask.h>
> +#include <linux/acpi.h>
> +#include <linux/container.h>
> +
> +#include "kunpeng_hbm.h"
> +
> +#define ACPI_MEMORY_DEVICE_HID			"PNP0C80"
> +#define ACPI_GENERIC_CONTAINER_DEVICE_HID	"PNP0A06"
> +
> +struct cdev_node {
> +	struct device *dev;
> +	struct list_head clist;
> +};
> +
> +struct cdev_node cdev_list;

This should be defined with private (static) scope.

Why isn't this just a struct list_head?  You don't ever use
the dev field in this list header, right?.  And in that case
you could define this with:

     static LIST_HEAD(cdev_list);

> +
> +static int get_pxm(struct acpi_device *acpi_device, void *arg)
> +{
> +	acpi_handle handle = acpi_device->handle;
> +	nodemask_t *mask = arg;
> +	unsigned long long sta;
> +	acpi_status status;
> +	int nid;
> +
> +	status = acpi_evaluate_integer(handle, "_STA", NULL, &sta);
> +	if (ACPI_SUCCESS(status) && (sta & ACPI_STA_DEVICE_ENABLED)) {
> +		nid = acpi_get_node(handle);
> +		if (nid != NUMA_NO_NODE)
> +			node_set(nid, *mask);
> +	}
> +
> +	return 0;
> +}
> +
> +static ssize_t pxms_show(struct device *dev,
> +			 struct device_attribute *attr,
> +			 char *buf)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> +	nodemask_t mask;
> +
> +	nodes_clear(mask);
> +	acpi_dev_for_each_child(adev, get_pxm, &mask);
> +
> +	return sysfs_emit(buf, "%*pbl\n", nodemask_pr_args(&mask));
> +}
> +static DEVICE_ATTR_RO(pxms);
> +
> +static int memdev_power_on(struct acpi_device *adev)
> +{
> +	acpi_handle handle = adev->handle;
> +	acpi_status status;
> +
> +	/* Power on and online the devices */
> +	status = acpi_evaluate_object(handle, "_ON", NULL, NULL);
> +	if (ACPI_FAILURE(status)) {
> +		acpi_handle_warn(handle, "Power on failed (0x%x)\n", status);
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +static int hbmdev_check(struct acpi_device *adev, void *arg)
> +{
> +	const char *hid = acpi_device_hid(adev);
> +
> +	if (!strcmp(hid, ACPI_MEMORY_DEVICE_HID)) {
> +		bool *found = arg;
> +		*found = true;
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static int memdev_power_off(struct acpi_device *adev)
> +{
> +	acpi_handle handle = adev->handle;
> +	acpi_status status;
> +
> +	/* Eject the devices and power off */
> +	status = acpi_evaluate_object(handle, "_OFF", NULL, NULL);
> +	if (ACPI_FAILURE(status))
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static ssize_t state_store(struct device *dev, struct device_attribute *attr,
> +			   const char *buf, size_t count)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> +	const int type = online_type_from_str(buf);
> +	int ret = -EINVAL;

This assignment is overwritten immediately, so don't bother.

> +
> +	/*
> +	 * Take the lock to avoid race on underlying PCC operation region
> +	 * used in ACPI function "_ON" and "_OFF".
> +	 */
> +	ret = lock_device_hotplug_sysfs();
> +	if (ret)
> +		return ret;
> +
> +	switch (type) {
> +	case STATE_ONLINE:
> +		ret = memdev_power_on(adev);
> +		break;
> +	case STATE_OFFLINE:
> +		ret  = memdev_power_off(adev);
> +		break;
> +	default:

		ret = -EINVAL;

> +		break;
> +	}
> +	unlock_device_hotplug();
> +
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +static DEVICE_ATTR_WO(state);
> +
> +static bool has_hbmdev(struct device *dev)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(dev);
> +	const char *hid = acpi_device_hid(adev);
> +	bool found = false;
> +
> +	if (strcmp(hid, ACPI_GENERIC_CONTAINER_DEVICE_HID))
> +		return found;

		return false;

OR maybe better, just do this:

     if (!strcmp(hid, ACPI_GENERIC_CONTAINER_DEVICE_HID))
         acpi_dev_for_each_child(adev, hbmdev_check, &found);

     return found;

> +
> +	acpi_dev_for_each_child(adev, hbmdev_check, &found);
> +	return found;
> +}
> +
> +static int container_add(struct device *dev, void *data)
> +{
> +	struct cdev_node *cnode;
> +
> +	if (!has_hbmdev(dev))
> +		return 0;
> +
> +	cnode = kmalloc(sizeof(struct cdev_node), GFP_KERNEL);
> +	if (!cnode)
> +		return -ENOMEM;
> +
> +	cnode->dev = dev;
> +	list_add_tail(&cnode->clist, &cdev_list.clist);
> +
> +	return 0;
> +}
> +
> +static void container_remove(void)

You add just one device in container_add(), but remove all devices
in this function.  Maybe differentiate the names, e.g. use
container_add_one() or container_remove_all() or something.

					-Alex

> +{
> +	struct cdev_node *cnode, *tmp;
> +
> +	list_for_each_entry_safe(cnode, tmp, &cdev_list.clist, clist) {
> +		device_remove_file(cnode->dev, &dev_attr_state);
> +		device_remove_file(cnode->dev, &dev_attr_pxms);
> +		list_del(&cnode->clist);
> +		kfree(cnode);
> +	}
> +}
> +
> +static int container_init(void)
> +{
> +	struct cdev_node *cnode;
> +
> +	INIT_LIST_HEAD(&cdev_list.clist);
> +
> +	if (bus_for_each_dev(&container_subsys, NULL, NULL, container_add)) {
> +		container_remove();
> +		return -ENOMEM;
> +	}
> +
> +	if (list_empty(&cdev_list.clist))
> +		return -ENODEV;
> +
> +	list_for_each_entry(cnode, &cdev_list.clist, clist) {
> +		device_create_file(cnode->dev, &dev_attr_state);
> +		device_create_file(cnode->dev, &dev_attr_pxms);
> +	}
> +
> +	return 0;
> +}
> +
> +static struct acpi_platform_list kunpeng_hbm_plat_info[] = {
> +	{"HISI  ", "HIP11   ", 0, ACPI_SIG_IORT, all_versions, NULL, 0},
> +	{ }
> +};
> +
> +static int __init hbmdev_init(void)
> +{
> +	if (acpi_match_platform_list(kunpeng_hbm_plat_info) < 0)
> +		return 0;
> +
> +	return container_init();
> +}
> +module_init(hbmdev_init);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Zhang Zekun <zhangzekun11@huawei.com>");

Re: [PATCH 1/2] soc: hisilicon: kunpeng_hbmdev: Add support for controling the power of hbm memory

[Jeff Johnson]

On 12/6/24 03:28, Zhang Zekun wrote:
> Add a driver for High Bandwidth Memory (HBM) devices, which will provide
> user space interfaces to power on/off the HBM devices. In Kunpeng servers,
> we need to control the power of HBM devices which can be power consuming
> and will only be used in some specialized scenarios, such as HPC. HBM
> memory devices in a socket are in the same power domain, and should be
> power off/on together.
> 
> HBM devices will be configured with ACPI device id "PNP0C80", and be used
> as a cpuless numa node. HBM devices in the same power domain will be put
> into the same container. ACPI function "_ON" and "_OFF" are reponsible
> for power on/off the HBM device, and notify the OS to fully online/offline
> the HBM memory.
> 
> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
> ---

...

> diff --git a/drivers/soc/hisilicon/Kconfig b/drivers/soc/hisilicon/Kconfig
> index 6d7c244d2e78..b3ca7d6f5d01 100644
> --- a/drivers/soc/hisilicon/Kconfig
> +++ b/drivers/soc/hisilicon/Kconfig
> @@ -21,4 +21,16 @@ config KUNPENG_HCCS
>  	  health status and port information of HCCS, or reducing system
>  	  power consumption on Kunpeng SoC.
>  
> +config KUNPENG_HBMDEV
> +	bool "add extra support for hbm memory device"
> +	depends on ACPI_HOTPLUG_MEMORY
> +	select ACPI_CONTAINER
> +	help
> +	  The driver provides methods for userpace to control the power
> +	  of HBM memory devices on Kunpeng soc, which can help to save
> +	  energy. The functionality of the driver would require dedicated
> +	  BIOS configuration.
> +
> +	  If not sure, say N.
> +
>  endmenu
> diff --git a/drivers/soc/hisilicon/Makefile b/drivers/soc/hisilicon/Makefile
> index 226e747e70d6..08048d73586e 100644
> --- a/drivers/soc/hisilicon/Makefile
> +++ b/drivers/soc/hisilicon/Makefile
> @@ -1,2 +1,3 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  obj-$(CONFIG_KUNPENG_HCCS)	+= kunpeng_hccs.o
> +obj-$(CONFIG_KUNPENG_HBMDEV)	+= kunpeng_hbmdev.o
...
> diff --git a/drivers/soc/hisilicon/kunpeng_hbmdev.c b/drivers/soc/hisilicon/kunpeng_hbmdev.c
> new file mode 100644
> index 000000000000..1945676ff502
> --- /dev/null
> +++ b/drivers/soc/hisilicon/kunpeng_hbmdev.c
> @@ -0,0 +1,210 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024 Huawei Technologies Co., Ltd
> + */

...

> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Zhang Zekun <zhangzekun11@huawei.com>");

Since commit 1fffe7a34c89 ("script: modpost: emit a warning when the
description is missing"), a module without a MODULE_DESCRIPTION() will
result in a warning with make W=1. My usual guidance is to add the
missing MODULE_DESCRIPTION() to avoid such warnings. But in this case,
due to how your Kconfig & Makefile are defined, this can NEVER be built
as a module, and hence I'd remove all of the MODULE_*() macros.

/jeff

[PATCH 2/2] soc: hisilicon: kunpeng_hbmcache: Add support for online and offline the hbm cache

[Zhang Zekun]

Add a driver for High Bandwidth Memory (HBM) cache, which provides user
space interfaces to power on/off the HBM cache. Use HBM as a cache can
take advantage of the high bandwidth of HBM in normal memory access, and
OS does not need to aware of the existence of HBM cache. For workloads
which does not require a high memory access bandwidth, power off the HBM
cache device can help save energy.

Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
---
 MAINTAINERS                              |   3 +-
 drivers/soc/hisilicon/Kconfig            |  11 ++
 drivers/soc/hisilicon/Makefile           |   1 +
 drivers/soc/hisilicon/kunpeng_hbmcache.c | 136 +++++++++++++++++++++++
 4 files changed, 150 insertions(+), 1 deletion(-)
 create mode 100644 drivers/soc/hisilicon/kunpeng_hbmcache.c

diff --git a/MAINTAINERS b/MAINTAINERS
index e8b4cf7d7162..4819d04badd7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10283,10 +10283,11 @@ F:	Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs
 F:	drivers/soc/hisilicon/kunpeng_hccs.c
 F:	drivers/soc/hisilicon/kunpeng_hccs.h
 
-HISILICON KUNPENG SOC KUNPENG HBMDEV DRIVER
+HISILICON KUNPENG SOC KUNPENG HBM DRIVER
 M:	Zhang Zekun <zhangzekun11@huawei.com>
 S:	Maintained
 F:	drivers/soc/hisilicon/kunpeng_hbm.h
+F:	drivers/soc/hisilicon/kunpeng_hbmcache.c
 F:	drivers/soc/hisilicon/kunpeng_hbmdev.c
 
 HISILICON LPC BUS DRIVER
diff --git a/drivers/soc/hisilicon/Kconfig b/drivers/soc/hisilicon/Kconfig
index b3ca7d6f5d01..f12f3e42d908 100644
--- a/drivers/soc/hisilicon/Kconfig
+++ b/drivers/soc/hisilicon/Kconfig
@@ -21,6 +21,17 @@ config KUNPENG_HCCS
 	  health status and port information of HCCS, or reducing system
 	  power consumption on Kunpeng SoC.
 
+config KUNPENG_HBMCACHE
+	tristate "HBM cache memory device"
+	depends on ACPI
+	help
+	  This driver provids methods to control the power of High Bandwidth
+	  Memory (HBM) cache device in Kunpeng SoC. Use HBM as a cache can
+	  take advantage of the high bandwidth of HBM in normal memory access.
+
+	  To compile the driver as a module, choose M here:
+	  the module will be called kunpeng_hbmcache.
+
 config KUNPENG_HBMDEV
 	bool "add extra support for hbm memory device"
 	depends on ACPI_HOTPLUG_MEMORY
diff --git a/drivers/soc/hisilicon/Makefile b/drivers/soc/hisilicon/Makefile
index 08048d73586e..b7c7c1682979 100644
--- a/drivers/soc/hisilicon/Makefile
+++ b/drivers/soc/hisilicon/Makefile
@@ -1,3 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_KUNPENG_HCCS)	+= kunpeng_hccs.o
 obj-$(CONFIG_KUNPENG_HBMDEV)	+= kunpeng_hbmdev.o
+obj-$(CONFIG_KUNPENG_HBMCACHE)	+= kunpeng_hbmcache.o
diff --git a/drivers/soc/hisilicon/kunpeng_hbmcache.c b/drivers/soc/hisilicon/kunpeng_hbmcache.c
new file mode 100644
index 000000000000..32eb7e781fd7
--- /dev/null
+++ b/drivers/soc/hisilicon/kunpeng_hbmcache.c
@@ -0,0 +1,136 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2024. Huawei Technologies Co., Ltd
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/acpi.h>
+#include <linux/device.h>
+
+#include "kunpeng_hbm.h"
+
+#define MODULE_NAME            "hbm_cache"
+
+static struct kobject *cache_kobj;
+static struct mutex cache_lock;
+
+static ssize_t state_store(struct device *d, struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct acpi_device *adev = ACPI_COMPANION(d);
+	const int type = online_type_from_str(buf);
+	acpi_handle handle = adev->handle;
+	acpi_status status = AE_OK;
+
+	if (!mutex_trylock(&cache_lock))
+		return restart_syscall();
+
+	switch (type) {
+	case STATE_ONLINE:
+		status = acpi_evaluate_object(handle, "_ON", NULL, NULL);
+		break;
+	case STATE_OFFLINE:
+		status = acpi_evaluate_object(handle, "_OFF", NULL, NULL);
+		break;
+	default:
+		break;
+	}
+	mutex_unlock(&cache_lock);
+
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	return count;
+}
+static DEVICE_ATTR_WO(state);
+
+static ssize_t socket_id_show(struct device *d, struct device_attribute *attr,
+				char *buf)
+{
+	int socket_id;
+
+	if (device_property_read_u32(d, "socket_id", &socket_id))
+		return -EINVAL;
+
+	return sysfs_emit(buf, "%d\n", socket_id);
+}
+static DEVICE_ATTR_RO(socket_id);
+
+static struct attribute *attrs[] = {
+	&dev_attr_state.attr,
+	&dev_attr_socket_id.attr,
+	NULL,
+};
+
+static struct attribute_group attr_group = {
+	.attrs = attrs,
+};
+
+static int cache_probe(struct platform_device *pdev)
+{
+	int ret;
+
+	ret = sysfs_create_group(&pdev->dev.kobj, &attr_group);
+	if (ret)
+		return ret;
+
+	ret = sysfs_create_link(cache_kobj,
+				&pdev->dev.kobj,
+				kobject_name(&pdev->dev.kobj));
+	if (ret) {
+		sysfs_remove_group(&pdev->dev.kobj, &attr_group);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void cache_remove(struct platform_device *pdev)
+{
+	sysfs_remove_group(&pdev->dev.kobj, &attr_group);
+	sysfs_remove_link(&pdev->dev.kobj,
+			  kobject_name(&pdev->dev.kobj));
+}
+
+static const struct acpi_device_id cache_acpi_ids[] = {
+	{"HISI04A1", 0},
+	{"", 0},
+};
+
+static struct platform_driver hbm_cache_driver = {
+	.probe = cache_probe,
+	.remove = cache_remove,
+	.driver = {
+		.name = MODULE_NAME,
+		.acpi_match_table = ACPI_PTR(cache_acpi_ids),
+	},
+};
+
+static int __init hbm_cache_module_init(void)
+{
+	int ret;
+
+	cache_kobj = kobject_create_and_add("hbm_cache", kernel_kobj);
+	if (!cache_kobj)
+		return -ENOMEM;
+
+	mutex_init(&cache_lock);
+
+	ret = platform_driver_register(&hbm_cache_driver);
+	if (ret) {
+		kobject_put(cache_kobj);
+		return ret;
+	}
+	return 0;
+}
+module_init(hbm_cache_module_init);
+
+static void __exit hbm_cache_module_exit(void)
+{
+	kobject_put(cache_kobj);
+	platform_driver_unregister(&hbm_cache_driver);
+}
+module_exit(hbm_cache_module_exit);
+MODULE_LICENSE("GPL");
-- 
2.17.1

Re: [PATCH 2/2] soc: hisilicon: kunpeng_hbmcache: Add support for online and offline the hbm cache

[kernel test robot]

Hi Zhang,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.13-rc1 next-20241206]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Zhang-Zekun/soc-hisilicon-kunpeng_hbmdev-Add-support-for-controling-the-power-of-hbm-memory/20241206-193643
base:   linus/master
patch link:    https://lore.kernel.org/r/20241206112812.32618-3-zhangzekun11%40huawei.com
patch subject: [PATCH 2/2] soc: hisilicon: kunpeng_hbmcache: Add support for online and offline the hbm cache
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20241207/202412070443.dYzNQNfY-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241207/202412070443.dYzNQNfY-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412070443.dYzNQNfY-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/soc/hisilicon/kunpeng_hbmcache.c: In function 'state_store':
>> drivers/soc/hisilicon/kunpeng_hbmcache.c:28:24: error: implicit declaration of function 'restart_syscall'; did you mean 'do_no_restart_syscall'? [-Werror=implicit-function-declaration]
      28 |                 return restart_syscall();
         |                        ^~~~~~~~~~~~~~~
         |                        do_no_restart_syscall
   cc1: some warnings being treated as errors


vim +28 drivers/soc/hisilicon/kunpeng_hbmcache.c

    18	
    19	static ssize_t state_store(struct device *d, struct device_attribute *attr,
    20				   const char *buf, size_t count)
    21	{
    22		struct acpi_device *adev = ACPI_COMPANION(d);
    23		const int type = online_type_from_str(buf);
    24		acpi_handle handle = adev->handle;
    25		acpi_status status = AE_OK;
    26	
    27		if (!mutex_trylock(&cache_lock))
  > 28			return restart_syscall();
    29	
    30		switch (type) {
    31		case STATE_ONLINE:
    32			status = acpi_evaluate_object(handle, "_ON", NULL, NULL);
    33			break;
    34		case STATE_OFFLINE:
    35			status = acpi_evaluate_object(handle, "_OFF", NULL, NULL);
    36			break;
    37		default:
    38			break;
    39		}
    40		mutex_unlock(&cache_lock);
    41	
    42		if (ACPI_FAILURE(status))
    43			return -ENODEV;
    44	
    45		return count;
    46	}
    47	static DEVICE_ATTR_WO(state);
    48	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 2/2] soc: hisilicon: kunpeng_hbmcache: Add support for online and offline the hbm cache

[Alex Elder]

On 12/6/24 5:28 AM, Zhang Zekun wrote:
> Add a driver for High Bandwidth Memory (HBM) cache, which provides user
> space interfaces to power on/off the HBM cache. Use HBM as a cache can
> take advantage of the high bandwidth of HBM in normal memory access, and
> OS does not need to aware of the existence of HBM cache. For workloads
> which does not require a high memory access bandwidth, power off the HBM
> cache device can help save energy.
> 
> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>
> ---
>   MAINTAINERS                              |   3 +-
>   drivers/soc/hisilicon/Kconfig            |  11 ++
>   drivers/soc/hisilicon/Makefile           |   1 +
>   drivers/soc/hisilicon/kunpeng_hbmcache.c | 136 +++++++++++++++++++++++
>   4 files changed, 150 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/soc/hisilicon/kunpeng_hbmcache.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e8b4cf7d7162..4819d04badd7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10283,10 +10283,11 @@ F:	Documentation/ABI/testing/sysfs-devices-platform-kunpeng_hccs
>   F:	drivers/soc/hisilicon/kunpeng_hccs.c
>   F:	drivers/soc/hisilicon/kunpeng_hccs.h
>   
> -HISILICON KUNPENG SOC KUNPENG HBMDEV DRIVER
> +HISILICON KUNPENG SOC KUNPENG HBM DRIVER
>   M:	Zhang Zekun <zhangzekun11@huawei.com>
>   S:	Maintained
>   F:	drivers/soc/hisilicon/kunpeng_hbm.h
> +F:	drivers/soc/hisilicon/kunpeng_hbmcache.c
>   F:	drivers/soc/hisilicon/kunpeng_hbmdev.c
>   
>   HISILICON LPC BUS DRIVER
> diff --git a/drivers/soc/hisilicon/Kconfig b/drivers/soc/hisilicon/Kconfig
> index b3ca7d6f5d01..f12f3e42d908 100644
> --- a/drivers/soc/hisilicon/Kconfig
> +++ b/drivers/soc/hisilicon/Kconfig
> @@ -21,6 +21,17 @@ config KUNPENG_HCCS
>   	  health status and port information of HCCS, or reducing system
>   	  power consumption on Kunpeng SoC.
>   
> +config KUNPENG_HBMCACHE
> +	tristate "HBM cache memory device"
> +	depends on ACPI
> +	help
> +	  This driver provids methods to control the power of High Bandwidth

s/provids/provides/

> +	  Memory (HBM) cache device in Kunpeng SoC. Use HBM as a cache can

If there can be more than one:
s/device/devices/

s/in Kunpeng/in the Kunpeng/
s/Use HBM/Using HBM/

> +	  take advantage of the high bandwidth of HBM in normal memory access.
> +
> +	  To compile the driver as a module, choose M here:
> +	  the module will be called kunpeng_hbmcache.
> +
>   config KUNPENG_HBMDEV
>   	bool "add extra support for hbm memory device"
>   	depends on ACPI_HOTPLUG_MEMORY
> diff --git a/drivers/soc/hisilicon/Makefile b/drivers/soc/hisilicon/Makefile
> index 08048d73586e..b7c7c1682979 100644
> --- a/drivers/soc/hisilicon/Makefile
> +++ b/drivers/soc/hisilicon/Makefile
> @@ -1,3 +1,4 @@
>   # SPDX-License-Identifier: GPL-2.0-only
>   obj-$(CONFIG_KUNPENG_HCCS)	+= kunpeng_hccs.o
>   obj-$(CONFIG_KUNPENG_HBMDEV)	+= kunpeng_hbmdev.o
> +obj-$(CONFIG_KUNPENG_HBMCACHE)	+= kunpeng_hbmcache.o
> diff --git a/drivers/soc/hisilicon/kunpeng_hbmcache.c b/drivers/soc/hisilicon/kunpeng_hbmcache.c
> new file mode 100644
> index 000000000000..32eb7e781fd7
> --- /dev/null
> +++ b/drivers/soc/hisilicon/kunpeng_hbmcache.c
> @@ -0,0 +1,136 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2024. Huawei Technologies Co., Ltd
> + */
> +
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/platform_device.h>
> +#include <linux/acpi.h>
> +#include <linux/device.h>
> +
> +#include "kunpeng_hbm.h"
> +
> +#define MODULE_NAME            "hbm_cache"
> +
> +static struct kobject *cache_kobj;
> +static struct mutex cache_lock;
> +
> +static ssize_t state_store(struct device *d, struct device_attribute *attr,
> +			   const char *buf, size_t count)
> +{
> +	struct acpi_device *adev = ACPI_COMPANION(d);
> +	const int type = online_type_from_str(buf);
> +	acpi_handle handle = adev->handle;
> +	acpi_status status = AE_OK;
> +
> +	if (!mutex_trylock(&cache_lock))
> +		return restart_syscall();
> +
> +	switch (type) {
> +	case STATE_ONLINE:
> +		status = acpi_evaluate_object(handle, "_ON", NULL, NULL);
> +		break;
> +	case STATE_OFFLINE:
> +		status = acpi_evaluate_object(handle, "_OFF", NULL, NULL);
> +		break;
> +	default:
> +		break;
> +	}
> +	mutex_unlock(&cache_lock);
> +
> +	if (ACPI_FAILURE(status))
> +		return -ENODEV;
> +
> +	return count;
> +}
> +static DEVICE_ATTR_WO(state);

Here too, could this just be defined as a Boolean attribute instead?
Do you anticipate an HBM cache device being in more than two possible
states someday?  Also, this is a write-only property?  Who is expected
to write this file?  Can it be written while the device is open?

> +
> +static ssize_t socket_id_show(struct device *d, struct device_attribute *attr,
> +				char *buf)
> +{
> +	int socket_id;
> +
> +	if (device_property_read_u32(d, "socket_id", &socket_id))
> +		return -EINVAL;

So an HBM cache device has a (required?) socket ID property in
its DTB? Did you define this in a binding document somewhere?
(I might just be jumping in late, without proper context, so
I apologize if I've just missed something.)

Does the socket ID affect/define/restrict something about
the functionality provided by a HBM cache device?

					-Alex

> +
> +	return sysfs_emit(buf, "%d\n", socket_id);
> +}
> +static DEVICE_ATTR_RO(socket_id);
> +
> +static struct attribute *attrs[] = {
> +	&dev_attr_state.attr,
> +	&dev_attr_socket_id.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group attr_group = {
> +	.attrs = attrs,
> +};
> +
> +static int cache_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +
> +	ret = sysfs_create_group(&pdev->dev.kobj, &attr_group);
> +	if (ret)
> +		return ret;
> +
> +	ret = sysfs_create_link(cache_kobj,
> +				&pdev->dev.kobj,
> +				kobject_name(&pdev->dev.kobj));
> +	if (ret) {
> +		sysfs_remove_group(&pdev->dev.kobj, &attr_group);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void cache_remove(struct platform_device *pdev)
> +{
> +	sysfs_remove_group(&pdev->dev.kobj, &attr_group);
> +	sysfs_remove_link(&pdev->dev.kobj,
> +			  kobject_name(&pdev->dev.kobj));
> +}
> +
> +static const struct acpi_device_id cache_acpi_ids[] = {
> +	{"HISI04A1", 0},
> +	{"", 0},
> +};
> +
> +static struct platform_driver hbm_cache_driver = {
> +	.probe = cache_probe,
> +	.remove = cache_remove,
> +	.driver = {
> +		.name = MODULE_NAME,
> +		.acpi_match_table = ACPI_PTR(cache_acpi_ids),
> +	},
> +};
> +
> +static int __init hbm_cache_module_init(void)
> +{
> +	int ret;
> +
> +	cache_kobj = kobject_create_and_add("hbm_cache", kernel_kobj);
> +	if (!cache_kobj)
> +		return -ENOMEM;
> +
> +	mutex_init(&cache_lock);
> +
> +	ret = platform_driver_register(&hbm_cache_driver);
> +	if (ret) {
> +		kobject_put(cache_kobj);
> +		return ret;
> +	}
> +	return 0;
> +}
> +module_init(hbm_cache_module_init);
> +
> +static void __exit hbm_cache_module_exit(void)
> +{
> +	kobject_put(cache_kobj);
> +	platform_driver_unregister(&hbm_cache_driver);
> +}
> +module_exit(hbm_cache_module_exit);
> +MODULE_LICENSE("GPL");

Re: [PATCH 2/2] soc: hisilicon: kunpeng_hbmcache: Add support for online and offline the hbm cache

[Jeff Johnson]

On 12/6/24 03:28, Zhang Zekun wrote:
> Add a driver for High Bandwidth Memory (HBM) cache, which provides user
> space interfaces to power on/off the HBM cache. Use HBM as a cache can
> take advantage of the high bandwidth of HBM in normal memory access, and
> OS does not need to aware of the existence of HBM cache. For workloads
> which does not require a high memory access bandwidth, power off the HBM
> cache device can help save energy.
> 
> Signed-off-by: Zhang Zekun <zhangzekun11@huawei.com>

...

> +module_exit(hbm_cache_module_exit);
> +MODULE_LICENSE("GPL");

Unlike the 1/2 patch, this one CAN be built as a module, so please add the
missing MODULE_DESCRIPTION() macro to avoid the warning with make W=1.

/jeff

Re: [PATCH 0/2] soc: hisilicon: Add power control support for kunpeng HBM

[Alex Elder]

On 12/6/24 5:28 AM, Zhang Zekun wrote:
> Add power control support for High Bandwidth Memory (HBM) for Kunpeng SoC
> platform. HBM devices on Kunpeng SoC can provide higher bandwidth at the
> cost of higher power consumption. Providing power control methods can help
> reducing the power when the workload does not need use HBM.

Could you explain a little more here how HBM is represented in the
system?  When it's powered on, it seems like it's "just memory".
And what you're doing here is enabling a power optimization to
allow this type of memory to be powered off when not in use.
How do you know whether it is in use?  What entity is meant to
be able to power this memory on and off?

In addition, it looks like there can be more than one instance of
an HBM device, and each is available to be used only for certain
CPUs.  Can you provide more information about that sort of
architectural detail?

Finally, the second patch enables "cache" functionality.  Maybe
this is something defined by ACPI and is well understood by others
but it's not clear to me what this even means.  How is an HBM
used, and how does its cache enabled/disabled state interact
with the device enabled/disabled state?

Is an HBM device something completely different from an HBM cache
device?  I guess I just lack a big-picture overview of how this
HBM fits into a system.

					-Alex

> 
> Zhang Zekun (2):
>    soc: hisilicon: kunpeng_hbmdev: Add support for controling the power
>      of hbm memory
>    soc: hisilicon: kunpeng_hbmcache: Add support for online and offline
>      the hbm cache
> 
>   MAINTAINERS                              |   7 +
>   drivers/soc/hisilicon/Kconfig            |  23 +++
>   drivers/soc/hisilicon/Makefile           |   2 +
>   drivers/soc/hisilicon/kunpeng_hbm.h      |  31 ++++
>   drivers/soc/hisilicon/kunpeng_hbmcache.c | 136 +++++++++++++++
>   drivers/soc/hisilicon/kunpeng_hbmdev.c   | 210 +++++++++++++++++++++++
>   6 files changed, 409 insertions(+)
>   create mode 100644 drivers/soc/hisilicon/kunpeng_hbm.h
>   create mode 100644 drivers/soc/hisilicon/kunpeng_hbmcache.c
>   create mode 100644 drivers/soc/hisilicon/kunpeng_hbmdev.c
>