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

[Alex Elder <elder@ieee.org>]

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");