Re: [PATCH v3 2/2] hwmon: add AMD Promontory 21 xHCI temperature sensor support

[Jihong Min <hurryman2212@icloud.com>]

I believe I have addressed the other review comments for v4, including the
remaining discussion from v2 2/2, your comments, and Mathias's suggestion to
move the PROM21-specific xHCI PCI handling into a separate glue driver.

I agree that "prom21_hwmon" is not a good name.

I think just "prom21" may also be too broad, because Promontory 21 is a
chipset/IP block with multiple I/O functions. This driver monitors the
temperature value exposed through the PROM21 xHCI PCI function's MMIO 
BAR, so
the xHCI part should probably be visible in the hwmon naming.

I am considering:

   - drivers/hwmon/prom21-xhci.c
   - CONFIG_SENSORS_PROM21_XHCI
   - hwmon name: prom21_xhci

while keeping the USB glue as xhci-pci-prom21.c.

Does that sound reasonable?

Sincerely,
Jihong Min

On 5/8/26 00:53, Guenter Roeck wrote:
> On Thu, May 07, 2026 at 12:31:59PM +0900, Jihong Min wrote:
>> PROM21 xHCI controllers expose an 8-bit temperature value through a
>> vendor-specific index/data register pair in the xHCI PCI MMIO BAR
>> region. Add an auxiliary hwmon driver for PROM21 controllers with PCI
>> ID 1022:43fd.
>>
>> PROM21 is an AMD chipset IP used in single-chip or daisy-chained
>> configurations to build AMD 6xx/8xx series chipsets.
>>
>> The vendor index register is at byte offset 0x3000 from the xHCI MMIO
>> BAR base and the vendor data register is at byte offset 0x3008. The
>> driver writes register selector 0x0001e520 to the index register, reads
>> the raw temperature value from the low 8 bits of the data register, and
>> restores the previous index before returning. Expose temp1_input and an
>> xHCI label through hwmon.
>>
>> Register the hwmon device under the parent PCI function so userspace
>> reports it as a PCI adapter, while the auxiliary driver still owns the
>> hwmon lifetime and unregisters it from the auxiliary remove path.
>>
>> No public AMD reference is available for this value. The conversion
>> formula is derived from observed temperature readings:
>>
>>    temp[C] = raw * 0.9066 - 78.624
>>
>> Testing showed that the temperature register does not return a valid
>> value while the xHCI PCI function is runtime suspended. By default, the
>> driver does not wake the parent PCI device from hwmon reads and returns
>> -EPERM while the device is suspended.
> Seriously ? Why would this be a permission issue ? Make it -ENODATA.
>
>> Document the supported device, register access, conversion formula,
>> module parameter, sysfs attributes, and sysfs lookup method.
>>
>> Assisted-by: Codex:gpt-5.5
>> Signed-off-by: Jihong Min <hurryman2212@gmail.com>
>> ---
>>   Documentation/hwmon/index.rst        |   1 +
>>   Documentation/hwmon/prom21-hwmon.rst |  86 ++++++++
>>   drivers/hwmon/Kconfig                |  11 +
>>   drivers/hwmon/Makefile               |   1 +
>>   drivers/hwmon/prom21-hwmon.c         | 293 +++++++++++++++++++++++++++
>>   5 files changed, 392 insertions(+)
>>   create mode 100644 Documentation/hwmon/prom21-hwmon.rst
>>   create mode 100644 drivers/hwmon/prom21-hwmon.c
>>
>> diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
>> index 8b655e5d6b68..41072977f0ef 100644
>> --- a/Documentation/hwmon/index.rst
>> +++ b/Documentation/hwmon/index.rst
>> @@ -216,6 +216,7 @@ Hardware Monitoring Kernel Drivers
>>      pmbus
>>      powerz
>>      powr1220
>> +   prom21-hwmon
>>      pt5161l
>>      pxe1610
>>      pwm-fan
>> diff --git a/Documentation/hwmon/prom21-hwmon.rst b/Documentation/hwmon/prom21-hwmon.rst
>> new file mode 100644
>> index 000000000000..0ba763e68ae9
>> --- /dev/null
>> +++ b/Documentation/hwmon/prom21-hwmon.rst
>> @@ -0,0 +1,86 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +Kernel driver prom21-hwmon
>> +==========================
>> +
>> +Supported chips:
>> +
>> +  * AMD Promontory 21 (PROM21) xHCI
>> +
>> +    Prefix: 'prom21_hwmon'
> The "hwmon" in this name is redundant. Yes, I know, others like it too,
> but it is still redundant. I won't comment on it further, though.
>
>> +
>> +    PCI ID: 1022:43fd
>> +
>> +Author:
>> +
>> +  - Jihong Min <hurryman2212@gmail.com>
>> +
>> +Description
>> +-----------
>> +
>> +This driver exposes the temperature sensor in AMD PROM21 xHCI controllers.
>> +
>> +The driver binds to an auxiliary device created by the xHCI PCI driver for
>> +supported controllers. The sensor value is accessed through a vendor-specific
>> +index/data register pair in the controller's PCI MMIO BAR.
>> +
>> +PROM21 is an AMD chipset IP used in single-chip or daisy-chained configurations
>> +to build AMD 6xx/8xx series chipsets. Since the xHCI controllers are
>> +integrated in PROM21, this temperature can also be used as a monitor for a
>> +temperature close to the AMD chipset temperature.
>> +
>> +Register access
>> +---------------
>> +
>> +The temperature value is read through a vendor-specific index/data register
>> +pair in the xHCI PCI MMIO BAR. The driver uses the following byte offsets from
>> +the MMIO BAR base:
>> +
>> +======================= =====================================================
>> +0x3000			Vendor index register
>> +0x3008			Vendor data register
>> +======================= =====================================================
>> +
>> +The driver saves the current vendor index register value, writes the
>> +temperature selector ``0x0001e520`` to the vendor index register, reads the
>> +vendor data register, and restores the previous vendor index value before
>> +returning. The raw temperature value is the low 8 bits of the vendor data
>> +register value.
>> +
>> +No public AMD reference is available for the raw value. The temperature
>> +conversion formula is derived from observed PROM21 xHCI temperature readings:
>> +
>> +  temp[C] = raw * 0.9066 - 78.624
>> +
>> +Module parameters
>> +-----------------
>> +
>> +pm: bool
>> +  Allow runtime PM state changes for device memory access. This is disabled
>> +  by default. If disabled, the driver does not wake the xHCI PCI device from
>> +  a temperature read. It reads the temperature only when the device is active.
>> +  A read from a suspended device returns ``-EPERM``.
>> +
>> +Sysfs entries
>> +-------------
>> +
>> +======================= =====================================================
>> +temp1_input		Temperature in millidegrees Celsius
>> +temp1_label		"xHCI"
> This is pointless and not the idea behind having a "label" attribute.
> The driver name itself already associates the sensor with xhci.
> Please drop.
>
>> +======================= =====================================================
>> +
>> +The hwmon device name is ``prom21_hwmon``. The sysfs path depends on the hwmon
>> +device number assigned by the kernel. Userspace can locate the device by
>> +matching the ``name`` attribute:
>> +
>> +.. code-block:: sh
>> +
>> +   for hwmon in /sys/class/hwmon/hwmon*; do
>> +           [ "$(cat "$hwmon/name")" = "prom21_hwmon" ] || continue
>> +           cat "$hwmon/temp1_label"
>> +           cat "$hwmon/temp1_input"
>> +   done
>> +
>> +``temp1_input`` reports millidegrees Celsius, so a value of ``50113`` means
>> +50.113 degrees Celsius. If the raw register value is invalid, ``temp1_input``
>> +returns ``-ENODATA``.
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index 14e4cea48acc..06d81cc29fec 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -940,6 +940,17 @@ config SENSORS_POWERZ
>>   	  This driver can also be built as a module. If so, the module
>>   	  will be called powerz.
>>   
>> +config SENSORS_PROM21
>> +	tristate "AMD Promontory 21 xHCI temperature sensor"
>> +	depends on USB_XHCI_PCI
>> +	select USB_XHCI_PCI_AUXDEV
>> +	help
>> +	  If you say yes here you get support for the AMD Promontory 21
>> +	  (PROM21) xHCI temperature sensor.
>> +
>> +	  This driver can also be built as a module. If so, the module
>> +	  will be called prom21-hwmon.
>> +
>>   config SENSORS_POWR1220
>>   	tristate "Lattice POWR1220 Power Monitoring"
>>   	depends on I2C
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 4788996aa137..7693ed3b3f72 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -196,6 +196,7 @@ obj-$(CONFIG_SENSORS_PC87427)	+= pc87427.o
>>   obj-$(CONFIG_SENSORS_PCF8591)	+= pcf8591.o
>>   obj-$(CONFIG_SENSORS_POWERZ)	+= powerz.o
>>   obj-$(CONFIG_SENSORS_POWR1220)  += powr1220.o
>> +obj-$(CONFIG_SENSORS_PROM21)	+= prom21-hwmon.o
>>   obj-$(CONFIG_SENSORS_PT5161L)	+= pt5161l.o
>>   obj-$(CONFIG_SENSORS_PWM_FAN)	+= pwm-fan.o
>>   obj-$(CONFIG_SENSORS_QNAP_MCU_HWMON)	+= qnap-mcu-hwmon.o
>> diff --git a/drivers/hwmon/prom21-hwmon.c b/drivers/hwmon/prom21-hwmon.c
>> new file mode 100644
>> index 000000000000..1c137304d65d
>> --- /dev/null
>> +++ b/drivers/hwmon/prom21-hwmon.c
>> @@ -0,0 +1,293 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * AMD PROM21 xHCI Hwmon Implementation
>> + * (only temperature monitoring is supported)
>> + *
>> + * This can be effectively used as the alternative chipset temperature monitor.
>> + *
>> + * Copyright (C) 2026 Jihong Min <hurryman2212@gmail.com>
>> + */
>> +
>> +#include <linux/auxiliary_bus.h>
>> +#include <linux/device.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/io.h>
>> +#include <linux/math.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/pci.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/slab.h>
>> +#include <linux/usb.h>
>> +#include <linux/usb/hcd.h>
>> +
>> +#define PROM21_INDEX 0x3000
>> +#define PROM21_DATA 0x3008
>> +#define PROM21_TEMP_REG 0x0001e520
>> +
>> +#define PROM21_HWMON_NAME "prom21_hwmon"
>> +#define PROM21_TEMP_LABEL "xHCI"
>> +
>> +struct prom21_hwmon {
>> +	struct pci_dev *pdev;
>> +	struct device *hwmon_dev;
>> +	void __iomem *regs;
>> +	bool removing;
>> +	struct mutex lock; /* protects removing and the index/data registers */
> It is difficult to believe that auxiliary device management is so unstable
> that it needs all that complexity. This will require confirmation from
> someone who knows how this is supposed to work, and a detailed explanation
> in the driver explaining why it is necessary.
>
>> +};
>> +
>> +static bool pm;
>> +module_param(pm, bool, 0444);
>> +MODULE_PARM_DESC(pm, "Allow runtime PM state changes for device memory access");
> No. Either enable it or don't, but please don't add such module parameters.
> The pm complexity in the driver, as written, makes it all but impossible
> to determine what is going on.
>
>> +
>> +static void prom21_hwmon_invalidate(struct prom21_hwmon *hwmon)
>> +{
>> +	mutex_lock(&hwmon->lock);
>> +	hwmon->removing = true;
>> +	mutex_unlock(&hwmon->lock);
>> +}
>> +
>> +static int prom21_hwmon_pm_get(struct prom21_hwmon *hwmon, bool *pm_ref)
>> +{
>> +	struct device *dev = &hwmon->pdev->dev;
>> +	int ret;
>> +
>> +	*pm_ref = false;
>> +
>> +	/*
>> +	 * PROM21 temperature register access does not return a valid value while
>> +	 * the parent xHCI PCI function is suspended. By default, only read when
>> +	 * runtime PM reports the device as active, or when runtime PM is disabled
>> +	 * and the device is not marked as suspended. If pm=Y, allow runtime PM
>> +	 * state changes while accessing the temperature register.
>> +	 */
>> +	if (pm) {
>> +		ret = pm_runtime_resume_and_get(dev);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		*pm_ref = true;
>> +		return 0;
>> +	}
>> +
>> +	ret = pm_runtime_get_if_active(dev);
>> +	if (ret > 0) {
>> +		*pm_ref = true;
>> +		return 0;
>> +	}
>> +
>> +	if (ret == -EINVAL && !pm_runtime_status_suspended(dev))
>> +		return 0;
>> +
>> +	if (!ret || pm_runtime_status_suspended(dev))
>> +		return -EPERM;
>> +
>> +	return ret;
>> +}
>> +
>> +/*
>> + * This is not a pure MMIO read. The PROM21 vendor data register is selected
>> + * by temporarily writing PROM21_TEMP_REG to the vendor index register. Keep
>> + * the sequence short and restore the previous index before returning.
>> + */
>> +static int prom21_hwmon_read_temp_raw_restore_index(struct prom21_hwmon *hwmon,
>> +						    u8 *raw)
>> +{
>> +	struct device *dev = &hwmon->pdev->dev;
>> +	bool pm_ref;
>> +	u32 index;
>> +	u32 data;
>> +	int ret;
>> +
>> +	/*
>> +	 * The xHCI PCI remove path destroys the auxiliary device before HCD
>> +	 * teardown. Keep runtime PM and MMIO inside the critical section so a
>> +	 * sysfs read cannot use the vendor register pair after remove starts.
>> +	 */
>> +	mutex_lock(&hwmon->lock);
>> +	if (hwmon->removing) {
>> +		mutex_unlock(&hwmon->lock);
>> +		return -ENODEV;
>> +	}
>> +
>> +	ret = prom21_hwmon_pm_get(hwmon, &pm_ref);
>> +	if (ret) {
>> +		mutex_unlock(&hwmon->lock);
>> +		return ret;
>> +	}
>> +
>> +	index = readl(hwmon->regs + PROM21_INDEX);
>> +	/* Select the PROM21 temperature register through the vendor index. */
>> +	writel(PROM21_TEMP_REG, hwmon->regs + PROM21_INDEX);
>> +	data = readl(hwmon->regs + PROM21_DATA);
>> +	/* Restore the previous vendor index register value. */
>> +	writel(index, hwmon->regs + PROM21_INDEX);
>> +	readl(hwmon->regs + PROM21_INDEX);
>> +
>> +	if (pm_ref) {
>> +		/*
>> +		 * Use autosuspend so repeated sysfs reads do not suspend the
>> +		 * controller immediately after each successful register access.
>> +		 */
>> +		pm_runtime_mark_last_busy(dev);
>> +		pm_runtime_put_autosuspend(dev);
>> +	}
>> +	mutex_unlock(&hwmon->lock);
>> +
>> +	*raw = data & 0xff;
>> +	if (!*raw || *raw == 0xff)
>> +		return -ENODATA;
>> +
>> +	return 0;
>> +}
>> +
>> +static long prom21_hwmon_raw_to_millicelsius(u8 raw)
>> +{
>> +	/*
>> +	 * No public AMD reference is available for this value.
>> +	 * The scale was derived from observed PROM21 xHCI temperature readings:
>> +	 *  temp[C] = raw * 0.9066 - 78.624
>> +	 */
>> +	return DIV_ROUND_CLOSEST(raw * 9066, 10) - 78624;
>> +}
>> +
>> +static umode_t prom21_hwmon_is_visible(const void *drvdata,
>> +				       enum hwmon_sensor_types type, u32 attr,
>> +				       int channel)
>> +{
>> +	if (type != hwmon_temp || channel)
>> +		return 0;
>> +
>> +	switch (attr) {
>> +	case hwmon_temp_input:
>> +	case hwmon_temp_label:
>> +		return 0444;
>> +	default:
>> +		return 0;
>> +	}
>> +}
>> +
>> +static int prom21_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
>> +			     u32 attr, int channel, long *val)
>> +{
>> +	struct prom21_hwmon *hwmon = dev_get_drvdata(dev);
>> +	u8 raw;
>> +	int ret;
>> +
>> +	if (type != hwmon_temp || attr != hwmon_temp_input || channel)
>> +		return -EOPNOTSUPP;
>> +
>> +	ret = prom21_hwmon_read_temp_raw_restore_index(hwmon, &raw);
>> +	if (ret)
>> +		return ret;
>> +
>> +	*val = prom21_hwmon_raw_to_millicelsius(raw);
>> +	return 0;
>> +}
>> +
>> +static int prom21_hwmon_read_string(struct device *dev,
>> +				    enum hwmon_sensor_types type, u32 attr,
>> +				    int channel, const char **str)
>> +{
>> +	if (type != hwmon_temp || attr != hwmon_temp_label || channel)
>> +		return -EOPNOTSUPP;
>> +
>> +	*str = PROM21_TEMP_LABEL;
>> +	return 0;
>> +}
>> +
>> +static const struct hwmon_ops prom21_hwmon_ops = {
>> +	.is_visible = prom21_hwmon_is_visible,
>> +	.read = prom21_hwmon_read,
>> +	.read_string = prom21_hwmon_read_string,
>> +};
>> +
>> +static const struct hwmon_channel_info *const prom21_hwmon_info[] = {
>> +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT | HWMON_T_LABEL),
>> +	NULL,
>> +};
>> +
>> +static const struct hwmon_chip_info prom21_hwmon_chip_info = {
>> +	.ops = &prom21_hwmon_ops,
>> +	.info = prom21_hwmon_info,
>> +};
>> +
>> +static int prom21_hwmon_probe(struct auxiliary_device *auxdev,
>> +			      const struct auxiliary_device_id *id)
>> +{
>> +	struct device *dev = &auxdev->dev;
>> +	struct device *parent = dev->parent;
>> +	struct prom21_hwmon *hwmon;
>> +	struct pci_dev *pdev;
>> +	struct usb_hcd *hcd;
>> +	int ret;
>> +
>> +	if (!parent || !dev_is_pci(parent))
>> +		return -ENODEV;
>> +
>> +	pdev = to_pci_dev(parent);
>> +	hcd = pci_get_drvdata(pdev);
>> +	if (!hcd)
>> +		return dev_err_probe(dev, -ENODEV,
>> +				     "xHCI HCD data unavailable\n");
>> +
>> +	if (!hcd->regs || hcd->rsrc_len < PROM21_DATA + sizeof(u32))
>> +		return dev_err_probe(dev, -ENODEV, "invalid MMIO resource\n");
>> +
>> +	hwmon = devm_kzalloc(dev, sizeof(*hwmon), GFP_KERNEL);
>> +	if (!hwmon)
>> +		return -ENOMEM;
>> +
>> +	ret = devm_mutex_init(dev, &hwmon->lock);
>> +	if (ret)
>> +		return ret;
>> +
>> +	hwmon->pdev = pdev;
>> +	hwmon->regs = hcd->regs;
>> +	auxiliary_set_drvdata(auxdev, hwmon);
>> +
>> +	/*
>> +	 * Use the PCI function as the hwmon parent so user space reports it as
>> +	 * a PCI adapter. Lifetime is still owned by this auxiliary driver;
>> +	 * remove() unregisters the hwmon device before xhci-pci tears down the
>> +	 * HCD.
>> +	 */
>> +	hwmon->hwmon_dev =
>> +		hwmon_device_register_with_info(&pdev->dev, PROM21_HWMON_NAME,
>> +						hwmon, &prom21_hwmon_chip_info,
>> +						NULL);
>> +	if (IS_ERR(hwmon->hwmon_dev))
>> +		return PTR_ERR(hwmon->hwmon_dev);
>> +
>> +	return 0;
>> +}
>> +
>> +static void prom21_hwmon_remove(struct auxiliary_device *auxdev)
>> +{
>> +	struct prom21_hwmon *hwmon = auxiliary_get_drvdata(auxdev);
>> +
>> +	if (hwmon) {
>> +		prom21_hwmon_invalidate(hwmon);
>> +		hwmon_device_unregister(hwmon->hwmon_dev);
>> +	}
>> +}
>> +
>> +static const struct auxiliary_device_id prom21_hwmon_id_table[] = {
>> +	{ .name = "xhci_pci." PROM21_HWMON_NAME },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(auxiliary, prom21_hwmon_id_table);
>> +
>> +static struct auxiliary_driver prom21_hwmon_driver = {
>> +	.name = "prom21-hwmon",
>> +	.probe = prom21_hwmon_probe,
>> +	.remove = prom21_hwmon_remove,
>> +	.id_table = prom21_hwmon_id_table,
>> +};
>> +module_auxiliary_driver(prom21_hwmon_driver);
>> +
>> +MODULE_AUTHOR("Jihong Min <hurryman2212@gmail.com>");
>> +MODULE_DESCRIPTION("AMD PROM21 xHCI hwmon driver");
>> +MODULE_LICENSE("GPL");