[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a03dba0e-1739-41e0-8607-7e4371d6df78@amd.com>
Date: Mon, 11 Aug 2025 16:33:55 +0530
From: "Gupta, Akshay" <Akshay.Gupta@....com>
To: Krzysztof Kozlowski <krzk@...nel.org>, linux-kernel@...r.kernel.org,
linux-hwmon@...r.kernel.org
Cc: gregkh@...uxfoundation.org, arnd@...db.de, linux@...ck-us.net,
Anand.Umarji@....com,
Naveen Krishna Chatradhi <naveenkrishna.chatradhi@....com>
Subject: Re: [PATCH v1 2/5] misc: amd-sbi: Add support for SB-RMI over I3C
On 7/29/2025 12:03 PM, Krzysztof Kozlowski wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On 28/07/2025 08:10, Akshay Gupta wrote:
>> AMD EPYC platforms with zen5 and later support APML(SB-RMI)
>> connection to the BMC over I3C bus for improved efficiency.
>> Add the same functionality supported by rmi-i2c.c over I3C bus.
>>
>> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@....com>
>> Signed-off-by: Akshay Gupta <akshay.gupta@....com>
>> ---
>> drivers/misc/amd-sbi/Kconfig | 15 +++-
>> drivers/misc/amd-sbi/Makefile | 2 +
>> drivers/misc/amd-sbi/rmi-i3c.c | 133 +++++++++++++++++++++++++++++++++
>> 3 files changed, 148 insertions(+), 2 deletions(-)
>> create mode 100644 drivers/misc/amd-sbi/rmi-i3c.c
>>
>> diff --git a/drivers/misc/amd-sbi/Kconfig b/drivers/misc/amd-sbi/Kconfig
>> index 9b1abeb6ab1a..838cf98805d9 100644
>> --- a/drivers/misc/amd-sbi/Kconfig
>> +++ b/drivers/misc/amd-sbi/Kconfig
>> @@ -15,10 +15,21 @@ config AMD_SBRMI_I2C
>> This driver can also be built as a module. If so, the module will
>> be called sbrmi-i2c.
>>
>> +config AMD_SBRMI_I3C
>> + tristate "AMD side band RMI support over I3C"
>> + depends on I3C
>> + select AMD_SBRMI
>> + select REGMAP_I3C
>> + help
>> + Side band RMI over I3C support for AMD out of band management.
>> +
>> + This driver can also be built as a module. If so, the module will
>> + be called sbrmi-i3c.
>> +
>> config AMD_SBRMI_HWMON
>> bool "SBRMI hardware monitoring"
>> - depends on AMD_SBRMI_I2C && HWMON
>> - depends on !(AMD_SBRMI_I2C=y && HWMON=m)
>> + depends on (AMD_SBRMI_I2C || AMD_SBRMI_I3C) && HWMON
>> + depends on !(AMD_SBRMI_I2C=y && HWMON=m) || !(AMD_SBRMI_I3C=y && HWMON=m)
>> help
>> This provides support for RMI device hardware monitoring. If enabled,
>> a hardware monitoring device will be created for each socket in
>> diff --git a/drivers/misc/amd-sbi/Makefile b/drivers/misc/amd-sbi/Makefile
>> index 6f3090fb9ff3..e43d4058a0f0 100644
>> --- a/drivers/misc/amd-sbi/Makefile
>> +++ b/drivers/misc/amd-sbi/Makefile
>> @@ -4,3 +4,5 @@ sbrmi_core-y := rmi-core.o
>> obj-$(CONFIG_AMD_SBRMI_HWMON) += rmi-hwmon.o
>> sbrmi-i2c-y := rmi-i2c.o
>> obj-$(CONFIG_AMD_SBRMI_I2C) += sbrmi-i2c.o
>> +sbrmi-i3c-y := rmi-i3c.o
>> +obj-$(CONFIG_AMD_SBRMI_I3C) += sbrmi-i3c.o
>> diff --git a/drivers/misc/amd-sbi/rmi-i3c.c b/drivers/misc/amd-sbi/rmi-i3c.c
>> new file mode 100644
>> index 000000000000..b960743afad1
>> --- /dev/null
>> +++ b/drivers/misc/amd-sbi/rmi-i3c.c
>> @@ -0,0 +1,133 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * rmi-i3c.c - Side band RMI over I3C support for AMD out
>> + * of band management
>> + *
>> + * Copyright (C) 2025 Advanced Micro Devices, Inc.
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/i3c/device.h>
>> +#include <linux/i3c/master.h>
>> +#include <linux/init.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/of.h>
> Where do you use it?
Ack, will remove.
>
>> +#include <linux/regmap.h>
>> +#include "rmi-core.h"
>
> ...
>
>> +static int sbrmi_i3c_probe(struct i3c_device *i3cdev)
>> +{
>> + struct device *dev = &i3cdev->dev;
>> + struct sbrmi_data *data;
>> + struct regmap_config sbrmi_i3c_regmap_config = {
>> + .reg_bits = 8,
>> + .val_bits = 8,
>> + };
>> + int ret;
>> +
>> + data = devm_kzalloc(dev, sizeof(struct sbrmi_data), GFP_KERNEL);
> sizeof(*). Use recent coding style, not some old, downstream code.
Ack, will update.
>
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + mutex_init(&data->lock);
>> +
>> + data->regmap = devm_regmap_init_i3c(i3cdev, &sbrmi_i3c_regmap_config);
>> + if (IS_ERR(data->regmap))
>> + return PTR_ERR(data->regmap);
>> +
>> + /* Enable alert for SB-RMI sequence */
>> + ret = sbrmi_enable_alert(data);
>> + if (ret < 0)
>> + return ret;
>> +
>> + /* Cache maximum power limit */
>> + ret = sbrmi_get_max_pwr_limit(data);
>> + if (ret < 0)
>> + return ret;
>> +
>> + /*
>> + * AMD APML I3C devices support static address
>> + */
>> + if (i3cdev->desc->info.static_addr)
>> + data->dev_static_addr = i3cdev->desc->info.static_addr;
>> + else
>> + data->dev_static_addr = i3cdev->desc->info.dyn_addr;
>> +
>> + dev_set_drvdata(dev, data);
>> +
>> + ret = create_hwmon_sensor_device(dev, data);
>> + if (ret < 0)
>> + return ret;
>> + return create_misc_rmi_device(data, dev);
>> +}
>> +
>> +static void sbrmi_i3c_remove(struct i3c_device *i3cdev)
>> +{
>> + struct sbrmi_data *data = dev_get_drvdata(&i3cdev->dev);
>> +
>> + misc_deregister(&data->sbrmi_misc_dev);
>> + /* Assign fops and parent of misc dev to NULL */
>> + data->sbrmi_misc_dev.fops = NULL;
>> + data->sbrmi_misc_dev.parent = NULL;
>> + dev_info(&i3cdev->dev, "Removed sbrmi-i3c driver\n");
> No, drop. This is useless.
Ack, will update.
>
>
>
> Best regards,
> Krzysztof
Powered by blists - more mailing lists