* [PATCH 0/3] hwmon: Add driver for Astera Labs PT516XX retimer
@ 2023-12-05 7:47 Cosmo Chou
2023-12-05 7:47 ` [PATCH 1/3] dt-bindings: vendor-prefixes: add asteralabs Cosmo Chou
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Cosmo Chou @ 2023-12-05 7:47 UTC (permalink / raw)
To: jdelvare, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt,
corbet, heiko, jernej.skrabec, macromorgan, linus.walleij
Cc: linux-hwmon, devicetree, linux-kernel, linux-doc, chou.cosmo,
cosmo.chou
This driver implements support for temperature monitoring of Astera Labs
PT5161L series PCIe retimer chips.
Cosmo Chou (3):
dt-bindings: vendor-prefixes: add asteralabs
dt-bindings: hwmon: pt516xx: add bindings
hwmon: Add driver for Astera Labs PT516XX retimer
.../bindings/hwmon/asteralabs,pt516xx.yaml | 36 +
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
Documentation/hwmon/index.rst | 1 +
Documentation/hwmon/pt516xx.rst | 48 ++
MAINTAINERS | 8 +
drivers/hwmon/Kconfig | 10 +
drivers/hwmon/Makefile | 1 +
drivers/hwmon/pt516xx.c | 648 ++++++++++++++++++
8 files changed, 754 insertions(+)
create mode 100644 Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml
create mode 100644 Documentation/hwmon/pt516xx.rst
create mode 100644 drivers/hwmon/pt516xx.c
--
2.34.1
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/3] dt-bindings: vendor-prefixes: add asteralabs 2023-12-05 7:47 [PATCH 0/3] hwmon: Add driver for Astera Labs PT516XX retimer Cosmo Chou @ 2023-12-05 7:47 ` Cosmo Chou 2023-12-05 16:45 ` Conor Dooley 2023-12-05 7:47 ` [PATCH 2/3] dt-bindings: hwmon: pt516xx: add bindings Cosmo Chou ` (2 subsequent siblings) 3 siblings, 1 reply; 12+ messages in thread From: Cosmo Chou @ 2023-12-05 7:47 UTC (permalink / raw) To: jdelvare, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt, corbet, heiko, jernej.skrabec, macromorgan, linus.walleij Cc: linux-hwmon, devicetree, linux-kernel, linux-doc, chou.cosmo, cosmo.chou Add vendor prefix for Astera Labs, Inc. https://www.asteralabs.com Signed-off-by: Cosmo Chou <chou.cosmo@gmail.com> --- Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml index 309b94c328c8..5c49f63d4ef0 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml @@ -155,6 +155,8 @@ patternProperties: description: ASPEED Technology Inc. "^asrock,.*": description: ASRock Inc. + "^asteralabs,.*": + description: Astera Labs, Inc. "^asus,.*": description: AsusTek Computer Inc. "^atheros,.*": -- 2.34.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] dt-bindings: vendor-prefixes: add asteralabs 2023-12-05 7:47 ` [PATCH 1/3] dt-bindings: vendor-prefixes: add asteralabs Cosmo Chou @ 2023-12-05 16:45 ` Conor Dooley 0 siblings, 0 replies; 12+ messages in thread From: Conor Dooley @ 2023-12-05 16:45 UTC (permalink / raw) To: Cosmo Chou Cc: jdelvare, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt, corbet, heiko, jernej.skrabec, macromorgan, linus.walleij, linux-hwmon, devicetree, linux-kernel, linux-doc, cosmo.chou [-- Attachment #1: Type: text/plain, Size: 264 bytes --] On Tue, Dec 05, 2023 at 03:47:21PM +0800, Cosmo Chou wrote: > Add vendor prefix for Astera Labs, Inc. > https://www.asteralabs.com > > Signed-off-by: Cosmo Chou <chou.cosmo@gmail.com> Acked-by: Conor Dooley <conor.dooley@microchip.com> Cheers, Conor. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] dt-bindings: hwmon: pt516xx: add bindings 2023-12-05 7:47 [PATCH 0/3] hwmon: Add driver for Astera Labs PT516XX retimer Cosmo Chou 2023-12-05 7:47 ` [PATCH 1/3] dt-bindings: vendor-prefixes: add asteralabs Cosmo Chou @ 2023-12-05 7:47 ` Cosmo Chou 2023-12-05 16:46 ` Conor Dooley 2023-12-05 7:47 ` [PATCH 3/3] hwmon: Add driver for Astera Labs PT516XX retimer Cosmo Chou 2023-12-05 14:45 ` [PATCH 0/3] " Guenter Roeck 3 siblings, 1 reply; 12+ messages in thread From: Cosmo Chou @ 2023-12-05 7:47 UTC (permalink / raw) To: jdelvare, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt, corbet, heiko, jernej.skrabec, macromorgan, linus.walleij Cc: linux-hwmon, devicetree, linux-kernel, linux-doc, chou.cosmo, cosmo.chou Add dt-bindings for pt516xx temperature monitor. Signed-off-by: Cosmo Chou <chou.cosmo@gmail.com> --- .../bindings/hwmon/asteralabs,pt516xx.yaml | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml diff --git a/Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml b/Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml new file mode 100644 index 000000000000..5700d4c91a0d --- /dev/null +++ b/Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml @@ -0,0 +1,36 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/hwmon/asteralabs,pt516xx.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: PT5161L hwmon sensor + +maintainers: + - Cosmo Chou <cosmo.chou@quantatw.com> + +properties: + compatible: + enum: + - asteralabs,pt5161l + + reg: + maxItems: 1 + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + sensor@24 { + compatible = "asteralabs,pt5161l"; + reg = <0x24>; + }; + }; -- 2.34.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] dt-bindings: hwmon: pt516xx: add bindings 2023-12-05 7:47 ` [PATCH 2/3] dt-bindings: hwmon: pt516xx: add bindings Cosmo Chou @ 2023-12-05 16:46 ` Conor Dooley 2023-12-06 8:02 ` Cosmo Chou 0 siblings, 1 reply; 12+ messages in thread From: Conor Dooley @ 2023-12-05 16:46 UTC (permalink / raw) To: Cosmo Chou Cc: jdelvare, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt, corbet, heiko, jernej.skrabec, macromorgan, linus.walleij, linux-hwmon, devicetree, linux-kernel, linux-doc, cosmo.chou [-- Attachment #1: Type: text/plain, Size: 1538 bytes --] On Tue, Dec 05, 2023 at 03:47:22PM +0800, Cosmo Chou wrote: > Add dt-bindings for pt516xx temperature monitor. > > Signed-off-by: Cosmo Chou <chou.cosmo@gmail.com> This can just go into trivial-devices.yaml, no? Thanks, Conor. > --- > .../bindings/hwmon/asteralabs,pt516xx.yaml | 36 +++++++++++++++++++ > 1 file changed, 36 insertions(+) > create mode 100644 Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml > > diff --git a/Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml b/Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml > new file mode 100644 > index 000000000000..5700d4c91a0d > --- /dev/null > +++ b/Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml > @@ -0,0 +1,36 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/hwmon/asteralabs,pt516xx.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: PT5161L hwmon sensor > + > +maintainers: > + - Cosmo Chou <cosmo.chou@quantatw.com> > + > +properties: > + compatible: > + enum: > + - asteralabs,pt5161l > + > + reg: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + > +additionalProperties: false > + > +examples: > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + sensor@24 { > + compatible = "asteralabs,pt5161l"; > + reg = <0x24>; > + }; > + }; > -- > 2.34.1 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] dt-bindings: hwmon: pt516xx: add bindings 2023-12-05 16:46 ` Conor Dooley @ 2023-12-06 8:02 ` Cosmo Chou 0 siblings, 0 replies; 12+ messages in thread From: Cosmo Chou @ 2023-12-06 8:02 UTC (permalink / raw) To: Conor Dooley Cc: jdelvare, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt, corbet, heiko, jernej.skrabec, macromorgan, linus.walleij, linux-hwmon, devicetree, linux-kernel, linux-doc, cosmo.chou OK. I will change to trivial-devices.yaml. Thanks Cosmo On Tue, 5 Dec 2023 16:46:42 +0000, Conor Dooley <conor@kernel.org> wrote: > > On Tue, Dec 05, 2023 at 03:47:22PM +0800, Cosmo Chou wrote: > > Add dt-bindings for pt516xx temperature monitor. > > > > Signed-off-by: Cosmo Chou <chou.cosmo@gmail.com> > > This can just go into trivial-devices.yaml, no? > > Thanks, > Conor. > > > --- > > .../bindings/hwmon/asteralabs,pt516xx.yaml | 36 +++++++++++++++++++ > > 1 file changed, 36 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml > > > > diff --git a/Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml b/Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml > > new file mode 100644 > > index 000000000000..5700d4c91a0d > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml > > @@ -0,0 +1,36 @@ > > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/hwmon/asteralabs,pt516xx.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: PT5161L hwmon sensor > > + > > +maintainers: > > + - Cosmo Chou <cosmo.chou@quantatw.com> > > + > > +properties: > > + compatible: > > + enum: > > + - asteralabs,pt5161l > > + > > + reg: > > + maxItems: 1 > > + > > +required: > > + - compatible > > + - reg > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + i2c { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + sensor@24 { > > + compatible = "asteralabs,pt5161l"; > > + reg = <0x24>; > > + }; > > + }; > > -- > > 2.34.1 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] hwmon: Add driver for Astera Labs PT516XX retimer 2023-12-05 7:47 [PATCH 0/3] hwmon: Add driver for Astera Labs PT516XX retimer Cosmo Chou 2023-12-05 7:47 ` [PATCH 1/3] dt-bindings: vendor-prefixes: add asteralabs Cosmo Chou 2023-12-05 7:47 ` [PATCH 2/3] dt-bindings: hwmon: pt516xx: add bindings Cosmo Chou @ 2023-12-05 7:47 ` Cosmo Chou 2023-12-05 15:55 ` Guenter Roeck ` (2 more replies) 2023-12-05 14:45 ` [PATCH 0/3] " Guenter Roeck 3 siblings, 3 replies; 12+ messages in thread From: Cosmo Chou @ 2023-12-05 7:47 UTC (permalink / raw) To: jdelvare, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt, corbet, heiko, jernej.skrabec, macromorgan, linus.walleij Cc: linux-hwmon, devicetree, linux-kernel, linux-doc, chou.cosmo, cosmo.chou This driver implements support for temperature monitoring of Astera Labs PT5161L series PCIe retimer chips. This driver implementation originates from the CSDK available at Link: https://github.com/facebook/openbmc/tree/helium/common/recipes-lib/retimer-v2.14 The communication protocol utilized is based on the I2C/SMBus standard. Signed-off-by: Cosmo Chou <chou.cosmo@gmail.com> --- Documentation/hwmon/index.rst | 1 + Documentation/hwmon/pt516xx.rst | 48 +++ MAINTAINERS | 8 + drivers/hwmon/Kconfig | 10 + drivers/hwmon/Makefile | 1 + drivers/hwmon/pt516xx.c | 648 ++++++++++++++++++++++++++++++++ 6 files changed, 716 insertions(+) create mode 100644 Documentation/hwmon/pt516xx.rst create mode 100644 drivers/hwmon/pt516xx.c diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst index 72f4e6065bae..2c4df18db663 100644 --- a/Documentation/hwmon/index.rst +++ b/Documentation/hwmon/index.rst @@ -181,6 +181,7 @@ Hardware Monitoring Kernel Drivers pmbus powerz powr1220 + pt516xx pxe1610 pwm-fan q54sj108a2 diff --git a/Documentation/hwmon/pt516xx.rst b/Documentation/hwmon/pt516xx.rst new file mode 100644 index 000000000000..945194f9a804 --- /dev/null +++ b/Documentation/hwmon/pt516xx.rst @@ -0,0 +1,48 @@ +.. SPDX-License-Identifier: GPL-2.0-or-later + +Kernel driver pt516xx +==================== + +Supported chips: + + * Astera Labs PT5161L + + Prefix: 'pt5161l' + + Addresses: I2C 0x24 + + Datasheet: http://www.asteralabs.com/wp-content/uploads/2021/03/Astera_Labs_PT5161L_Product_Brief.pdf + +Authors: Cosmo Chou <cosmo.chou@quantatw.com> + +Description +----------- + +This driver implements support for temperature monitoring of Astera Labs +PT5161L series PCIe retimer chips. + +This driver implementation originates from the CSDK available at +https://github.com/facebook/openbmc/tree/helium/common/recipes-lib/retimer-v2.14 +The communication protocol utilized is based on the I2C/SMBus standard. + +For more detailed information and specific implementation details, it is +recommended to refer to the CSDK source code available at the provided GitHub +link. + +Sysfs entries +---------------- + +================ ============================================== +temp1_input Measured temperature (in millidegrees Celsius) +================ ============================================== + +Debugfs entries +---------------- + +================ ==================================== +fw_ver Firmware version of the retimer +health Health status (8 bits) + [0]: Heartbeat Okay (1'b1: OK) + [1]: Firmware loaded Okay (1'b1: OK) + [7:2]: Reserved +================ ==================================== diff --git a/MAINTAINERS b/MAINTAINERS index 788be9ab5b73..492002ffd12c 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17482,6 +17482,14 @@ F: fs/pstore/ F: include/linux/pstore* K: \b(pstore|ramoops) +PT516XX HARDWARE MONITOR DRIVER +M: Cosmo Chou <cosmo.chou@quantatw.com> +L: linux-hwmon@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml +F: Documentation/hwmon/pt516xx.rst +F: drivers/hwmon/pt516xx.c + PTP HARDWARE CLOCK SUPPORT M: Richard Cochran <richardcochran@gmail.com> L: netdev@vger.kernel.org diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig index cf27523eed5a..3965bec7774a 100644 --- a/drivers/hwmon/Kconfig +++ b/drivers/hwmon/Kconfig @@ -1703,6 +1703,16 @@ source "drivers/hwmon/peci/Kconfig" source "drivers/hwmon/pmbus/Kconfig" +config SENSORS_PT516XX + tristate "Astera Labs PT516XX PCIe retimer hardware monitoring" + depends on I2C + help + If you say yes here you get support for temperature monitoring + on the Astera Labs PT516XX PCIe retimer. + + This driver can also be built as a module. If so, the module + will be called pt516xx. + config SENSORS_PWM_FAN tristate "PWM fan" depends on (PWM && OF) || COMPILE_TEST diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile index e84bd9685b5c..1942064cd4e9 100644 --- a/drivers/hwmon/Makefile +++ b/drivers/hwmon/Makefile @@ -179,6 +179,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_PT516XX) += pt516xx.o obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o obj-$(CONFIG_SENSORS_SBTSI) += sbtsi_temp.o diff --git a/drivers/hwmon/pt516xx.c b/drivers/hwmon/pt516xx.c new file mode 100644 index 000000000000..824798559fe1 --- /dev/null +++ b/drivers/hwmon/pt516xx.c @@ -0,0 +1,648 @@ +// SPDX-License-Identifier: GPL-2.0-or-later + +#include <linux/debugfs.h> +#include <linux/delay.h> +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/init.h> +#include <linux/hwmon.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/of_device.h> + +/** Main-micro-assisted access command codes */ +// Wide register reads +#define ARIES_MM_RD_WIDE_REG_2B 0x1d +#define ARIES_MM_RD_WIDE_REG_3B 0x1e +#define ARIES_MM_RD_WIDE_REG_4B 0x1f +#define ARIES_MM_RD_WIDE_REG_5B 0x20 +// Wide register writes +#define ARIES_MM_WR_WIDE_REG_2B 0x21 +#define ARIES_MM_WR_WIDE_REG_3B 0x22 +#define ARIES_MM_WR_WIDE_REG_4B 0x23 +#define ARIES_MM_WR_WIDE_REG_5B 0x24 + +/** Temperature measurement constants */ +// Aries current average temp ADC code CSR +#define ARIES_CURRENT_AVG_TEMP_ADC_CSR 0x42c + +// Main Micro Heartbeat reg +#define ARIES_MM_HEARTBEAT_ADDR 0x923 + +/** Main SRAM */ +// AL Main SRAM DMEM offset (A0) +#define AL_MAIN_SRAM_DMEM_OFFSET (64 * 1024) +// SRAM read command +#define AL_TG_RD_LOC_IND_SRAM 0x16 + +/** Micros */ +// Offset for main micro FW info +#define ARIES_MAIN_MICRO_FW_INFO (96 * 1024 - 128) + +/** Path Micro Members */ +// FW Info (Major) offset location in struct +#define ARIES_MM_FW_VERSION_MAJOR 0 +// FW Info (Minor) offset location in struct +#define ARIES_MM_FW_VERSION_MINOR 1 +// FW Info (Build no.) offset location in struct +#define ARIES_MM_FW_VERSION_BUILD 2 + +/** Offsets for MM assisted accesses */ +// Legacy Address and Data registers (using wide registers) +// Reg offset to specify Address for MM assisted accesses +#define ARIES_MM_ASSIST_REG_ADDR_OFFSET 0xd99 +// Reg offset to specify Command +#define ARIES_MM_ASSIST_CMD_OFFSET 0xd9d + +// New Address and Data registers (not using wide registers) +// Reg offset to MM SPARE 0 used specify Address[7:0] +#define ARIES_MM_ASSIST_SPARE_0_OFFSET 0xd9f +// Reg offset to MM SPARE 3 used specify Data Byte 0 +#define ARIES_MM_ASSIST_SPARE_3_OFFSET 0xda2 + +/** Misc block offsets */ +// Device Load check register +#define ARIES_CODE_LOAD_REG 0x605 + +/** Value indicating FW was loaded properly */ +#define ARIES_LOAD_CODE 0xe + +/** EEPROM parameters */ +// Time delay between checking MM status of EEPROM write (microseconds) +#define ARIES_MM_STATUS_TIME 5000 + +#define ARIES_TEMP_CAL_CODE_DEFAULT 84 + +/* Struct defining FW version loaded on an Aries device */ +struct pt516xx_fw_ver { + u8 major; // FW version major release value + u8 minor; // FW version minor release value + u16 build; // FW version build release value +}; + +/* Each client has this additional data */ +struct pt516xx_data { + struct i2c_client *client; + struct dentry *debugfs; + struct pt516xx_fw_ver fw_ver; + struct mutex lock; + bool init_done; + bool pec_enable; // Enable PEC + bool code_load_okay; // indicate if code load reg value is expected + bool mm_heartbeat_okay; // indicate if Main Micro heartbeat is good + bool mm_wide_reg_valid; // MM assisted Wide Register access + u8 temp_cal_code_avg; +}; + +static struct dentry *pt516xx_debugfs_dir; + +/* + * Write multiple data bytes to Aries over I2C + */ +static int pt516xx_write_block_data(struct pt516xx_data *data, u32 address, + u8 len, u8 *val) +{ + struct i2c_client *client = data->client; + int ret; + u8 remain_len = len; + u8 xfer_len, curr_len; + u8 buf[16]; + u8 cmd = 0x0F; // [7]:pec_en, [6:5]:rsvd, [4:2]:func, [1]:start, [0]:end + u8 config = 0x40; // [6]:cfg_type, [4:1]:burst_len, [0]:bit16 of address + + if (data->pec_enable) + cmd |= 0x80; + + // If byte count is greater than 4, perform multiple iterations + while (remain_len > 0) { + if (remain_len > 4) { + curr_len = 4; + remain_len -= 4; + } else { + curr_len = remain_len; + remain_len = 0; + } + + buf[0] = config | (curr_len - 1) << 1 | ((address >> 16) & 0x1); + buf[1] = (address >> 8) & 0xff; + buf[2] = address & 0xff; + memcpy(&buf[3], val, curr_len); + + xfer_len = 3 + curr_len; + ret = i2c_smbus_write_block_data(client, cmd, xfer_len, buf); + if (ret) + return ret; + + val += curr_len; + address += curr_len; + } + + return 0; +} + +/* + * Read multiple data bytes from Aries over I2C + */ +static int pt516xx_read_block_data(struct pt516xx_data *data, u32 address, + u8 len, u8 *val) +{ + struct i2c_client *client = data->client; + int ret, tries; + u8 remain_len = len; + u8 curr_len; + u8 wbuf[16], rbuf[24]; + u8 cmd = 0x08; // [7]:pec_en, [6:5]:rsvd, [4:2]:func, [1]:start, [0]:end + u8 config = 0x00; // [6]:cfg_type, [4:1]:burst_len, [0]:bit16 of address + + if (data->pec_enable) + cmd |= 0x80; + + // If byte count is greater than 16, perform multiple iterations + while (remain_len > 0) { + if (remain_len > 16) { + curr_len = 16; + remain_len -= 16; + } else { + curr_len = remain_len; + remain_len = 0; + } + + wbuf[0] = config | (curr_len - 1) << 1 | + ((address >> 16) & 0x1); + wbuf[1] = (address >> 8) & 0xff; + wbuf[2] = address & 0xff; + + // Perform read operation + for (tries = 0; tries < 3; tries++) { + ret = i2c_smbus_write_block_data(client, (cmd | 0x2), 3, + wbuf); + if (ret) + return ret; + + ret = i2c_smbus_read_block_data(client, (cmd | 0x1), + rbuf); + if (ret == curr_len) + break; + } + if (tries >= 3) + return -ENODATA; + + memcpy(val, rbuf, curr_len); + val += curr_len; + address += curr_len; + } + + return 0; +} + +static int pt516xx_read_wide_reg(struct pt516xx_data *data, u32 address, + u8 width, u8 *val) +{ + int ret, tries; + u8 buf[8]; + u8 status; + + if (data->mm_wide_reg_valid) { + // Write address (3 bytes) + buf[0] = address & 0xff; + buf[1] = (address >> 8) & 0xff; + buf[2] = (address >> 16) & 0x1; + ret = pt516xx_write_block_data( + data, ARIES_MM_ASSIST_SPARE_0_OFFSET, 3, buf); + if (ret) + return ret; + + // Set command based on width + switch (width) { + case 2: + buf[0] = ARIES_MM_RD_WIDE_REG_2B; + break; + case 3: + buf[0] = ARIES_MM_RD_WIDE_REG_3B; + break; + case 4: + buf[0] = ARIES_MM_RD_WIDE_REG_4B; + break; + case 5: + buf[0] = ARIES_MM_RD_WIDE_REG_5B; + break; + default: + return -EINVAL; + } + ret = pt516xx_write_block_data(data, ARIES_MM_ASSIST_CMD_OFFSET, + 1, buf); + if (ret) + return ret; + + // Check access status + status = 0xff; + for (tries = 0; tries < 100; tries++) { + ret = pt516xx_read_block_data( + data, ARIES_MM_ASSIST_CMD_OFFSET, 1, &status); + if (ret) + return ret; + + if (status == 0) + break; + + usleep_range(ARIES_MM_STATUS_TIME, + ARIES_MM_STATUS_TIME + 1000); + } + if (status != 0) + return -ETIMEDOUT; + + // Read N bytes of data based on width + ret = pt516xx_read_block_data( + data, ARIES_MM_ASSIST_SPARE_3_OFFSET, width, val); + if (ret) + return ret; + } else { + return pt516xx_read_block_data(data, address, width, val); + } + + return 0; +} + +/* + * Read multiple (up to eight) data bytes from micro SRAM over I2C + */ +static int +pt516xx_read_block_data_main_micro_indirect(struct pt516xx_data *data, + u32 address, u8 len, u8 *val) +{ + int ret, tries; + u8 buf[8]; + u8 i, status; + u32 uind_offs = ARIES_MM_ASSIST_REG_ADDR_OFFSET; + u32 eeprom_base, eeprom_addr; + + // No multi-byte indirect support here. Hence read a byte at a time + eeprom_base = address - AL_MAIN_SRAM_DMEM_OFFSET; + for (i = 0; i < len; i++) { + // Write eeprom addr + eeprom_addr = eeprom_base + i; + buf[0] = eeprom_addr & 0xff; + buf[1] = (eeprom_addr >> 8) & 0xff; + buf[2] = (eeprom_addr >> 16) & 0xff; + ret = pt516xx_write_block_data(data, uind_offs, 3, buf); + if (ret) + return ret; + + // Write eeprom cmd + buf[0] = AL_TG_RD_LOC_IND_SRAM; + ret = pt516xx_write_block_data(data, uind_offs + 4, 1, buf); + if (ret) + return ret; + + // Test successful access + status = 0xff; + for (tries = 0; tries < 255; tries++) { + ret = pt516xx_read_block_data(data, uind_offs + 4, 1, + &status); + if (ret) + return ret; + + if (status == 0) + break; + } + if (status != 0) + return -ETIMEDOUT; + + ret = pt516xx_read_block_data(data, uind_offs + 3, 1, buf); + if (ret) + return ret; + + val[i] = buf[0]; + } + + return 0; +} + +/* + * Check the status of firmware + */ +static int pt516xx_fwsts_check(struct pt516xx_data *data) +{ + int ret, tries; + u8 buf[8]; + u8 heartbeat, major = 0, minor = 0; + u16 build = 0; + bool hb_changed = false; + + // Read Code Load reg + ret = pt516xx_read_block_data(data, ARIES_CODE_LOAD_REG, 1, buf); + if (ret) + return ret; + + if (buf[0] < ARIES_LOAD_CODE) { + dev_warn( + &data->client->dev, + "Code Load reg unexpected. Not all modules are loaded %x\n", + buf[0]); + data->code_load_okay = false; + } else { + data->code_load_okay = true; + } + + // Check Main Micro heartbeat + // If heartbeat value does not change for 100 tries, no MM heartbeat + // Else heartbeat present even if one value changes + ret = pt516xx_read_block_data(data, ARIES_MM_HEARTBEAT_ADDR, 1, buf); + if (ret) + return ret; + + heartbeat = buf[0]; + for (tries = 0; tries < 100; tries++) { + ret = pt516xx_read_block_data(data, ARIES_MM_HEARTBEAT_ADDR, 1, + buf); + if (ret) + return ret; + + if (buf[0] != heartbeat) { + hb_changed = true; + break; + } + } + data->mm_heartbeat_okay = hb_changed; + + // Read FW version + // If heartbeat not there, set default FW values to 0.0.0 + // and return success + if (data->mm_heartbeat_okay) { + // Get FW version (major) + ret = pt516xx_read_block_data_main_micro_indirect( + data, + ARIES_MAIN_MICRO_FW_INFO + ARIES_MM_FW_VERSION_MAJOR, 1, + &major); + if (ret) + return ret; + + // Get FW version (minor) + ret = pt516xx_read_block_data_main_micro_indirect( + data, + ARIES_MAIN_MICRO_FW_INFO + ARIES_MM_FW_VERSION_MINOR, 1, + &minor); + if (ret) + return ret; + + // Get FW version (build) + ret = pt516xx_read_block_data_main_micro_indirect( + data, + ARIES_MAIN_MICRO_FW_INFO + ARIES_MM_FW_VERSION_BUILD, 2, + (u8 *)&build); + if (ret) + return ret; + } else { + dev_warn(&data->client->dev, "No Main Micro Heartbeat\n"); + } + data->fw_ver.major = major; + data->fw_ver.minor = minor; + data->fw_ver.build = build; + + return 0; +} + +static int pt516xx_fw_is_at_least(struct pt516xx_data *data, u8 major, u8 minor, + u16 build) +{ + u32 ver = major << 24 | minor << 16 | build; + u32 curr_ver = data->fw_ver.major << 24 | data->fw_ver.minor << 16 | + data->fw_ver.build; + + if (curr_ver >= ver) + return true; + + return false; +} + +static int pt516xx_init_dev(struct pt516xx_data *data) +{ + int ret; + + mutex_lock(&data->lock); + ret = pt516xx_fwsts_check(data); + mutex_unlock(&data->lock); + if (ret) + return ret; + + dev_info(&data->client->dev, "fw_ver: %u.%u.%u\n", data->fw_ver.major, + data->fw_ver.minor, data->fw_ver.build); + + if (pt516xx_fw_is_at_least(data, 2, 2, 0)) + data->mm_wide_reg_valid = true; + + data->temp_cal_code_avg = ARIES_TEMP_CAL_CODE_DEFAULT; + data->init_done = true; + + return 0; +} + +static int pt516xx_read(struct device *dev, enum hwmon_sensor_types type, + u32 attr, int channel, long *val) +{ + struct pt516xx_data *data = dev_get_drvdata(dev); + int ret = 0; + long adc_code = 0; + + if (!data->init_done) { + ret = pt516xx_init_dev(data); + if (ret) { + dev_err(dev, "pt516xx_init_dev failed %d\n", ret); + return ret; + } + } + + switch (attr) { + case hwmon_temp_input: + mutex_lock(&data->lock); + ret = pt516xx_read_wide_reg(data, + ARIES_CURRENT_AVG_TEMP_ADC_CSR, 4, + (u8 *)&adc_code); + mutex_unlock(&data->lock); + break; + default: + return -EOPNOTSUPP; + } + if (ret) { + dev_err(dev, "Read adc_code failed %d\n", ret); + return ret; + } + if (adc_code == 0 || adc_code >= 0x3ff) { + dev_err(dev, "Invalid adc_code %lx\n", adc_code); + return -EINVAL; + } + + *val = 110000 + ((adc_code - (data->temp_cal_code_avg + 250)) * -320); + + return 0; +} + +static umode_t pt516xx_is_visible(const void *data, + enum hwmon_sensor_types type, u32 attr, + int channel) +{ + switch (attr) { + case hwmon_temp_input: + return 0444; + default: + break; + } + + return 0; +} + +static const struct hwmon_channel_info *pt516xx_info[] = { + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT), + NULL +}; + +static const struct hwmon_ops pt516xx_hwmon_ops = { + .is_visible = pt516xx_is_visible, + .read = pt516xx_read, +}; + +static const struct hwmon_chip_info pt516xx_chip_info = { + .ops = &pt516xx_hwmon_ops, + .info = pt516xx_info, +}; + +static ssize_t pt516xx_debugfs_read_fw_ver(struct file *file, char __user *buf, + size_t count, loff_t *ppos) +{ + struct pt516xx_data *data = file->private_data; + int ret; + char ver[32]; + + mutex_lock(&data->lock); + ret = pt516xx_fwsts_check(data); + mutex_unlock(&data->lock); + if (ret) + return ret; + + ret = snprintf(ver, sizeof(ver), "%u.%u.%u\n", data->fw_ver.major, + data->fw_ver.minor, data->fw_ver.build); + if (ret < 0) + return ret; + + return simple_read_from_buffer(buf, count, ppos, ver, ret + 1); +} + +static const struct file_operations pt516xx_debugfs_ops_fw_ver = { + .read = pt516xx_debugfs_read_fw_ver, + .open = simple_open, +}; + +static ssize_t pt516xx_debugfs_read_health(struct file *file, char __user *buf, + size_t count, loff_t *ppos) +{ + struct pt516xx_data *data = file->private_data; + int ret; + u8 status = 0; + char health[8]; + + mutex_lock(&data->lock); + ret = pt516xx_fwsts_check(data); + mutex_unlock(&data->lock); + if (ret == 0) { + status |= data->mm_heartbeat_okay ? 1 : 0; // bit0 + status |= data->code_load_okay ? 2 : 0; // bit1 + } + + ret = snprintf(health, sizeof(health), "0x%02x\n", status); + if (ret < 0) + return ret; + + return simple_read_from_buffer(buf, count, ppos, health, ret + 1); +} + +static const struct file_operations pt516xx_debugfs_ops_health = { + .read = pt516xx_debugfs_read_health, + .open = simple_open, +}; + +static int pt516xx_init_debugfs(struct pt516xx_data *data) +{ + if (!pt516xx_debugfs_dir) + return -ENOENT; + + data->debugfs = debugfs_create_dir(dev_name(&data->client->dev), + pt516xx_debugfs_dir); + if (IS_ERR_OR_NULL(data->debugfs)) + return -ENOENT; + + debugfs_create_file("fw_ver", 0444, data->debugfs, data, + &pt516xx_debugfs_ops_fw_ver); + + debugfs_create_file("health", 0444, data->debugfs, data, + &pt516xx_debugfs_ops_health); + + return 0; +} + +static int pt516xx_probe(struct i2c_client *client) +{ + struct device *dev = &client->dev; + struct device *hwmon_dev; + struct pt516xx_data *data; + + data = devm_kzalloc(dev, sizeof(struct pt516xx_data), GFP_KERNEL); + if (!data) + return -ENOMEM; + + data->client = client; + mutex_init(&data->lock); + pt516xx_init_dev(data); + + hwmon_dev = devm_hwmon_device_register_with_info( + dev, client->name, data, &pt516xx_chip_info, NULL); + + if (pt516xx_init_debugfs(data)) + dev_warn(dev, "Failed to register debugfs\n"); + + return PTR_ERR_OR_ZERO(hwmon_dev); +} + +static const struct i2c_device_id pt516xx_id[] = { + { "pt5161l", 0 }, + {} +}; +MODULE_DEVICE_TABLE(i2c, pt516xx_id); + +static const struct of_device_id __maybe_unused pt516xx_of_match[] = { + { .compatible = "asteralabs,pt5161l" }, + {}, +}; +MODULE_DEVICE_TABLE(of, pt516xx_of_match); + +static struct i2c_driver pt516xx_driver = { + .class = I2C_CLASS_HWMON, + .driver = { + .name = "pt516xx", + .of_match_table = of_match_ptr(pt516xx_of_match), + }, + .probe = pt516xx_probe, + .id_table = pt516xx_id, +}; + +module_i2c_driver(pt516xx_driver); + +static int __init pt516xx_core_init(void) +{ + pt516xx_debugfs_dir = debugfs_create_dir("pt516xx", NULL); + if (IS_ERR(pt516xx_debugfs_dir)) + pt516xx_debugfs_dir = NULL; + + return 0; +} + +static void __exit pt516xx_core_exit(void) +{ + debugfs_remove_recursive(pt516xx_debugfs_dir); +} + +module_init(pt516xx_core_init); +module_exit(pt516xx_core_exit); + +MODULE_AUTHOR("Cosmo Chou <cosmo.chou@quantatw.com>"); +MODULE_DESCRIPTION("Hwmon driver for Astera Labs Aries PCIe retimer"); +MODULE_LICENSE("GPL"); -- 2.34.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] hwmon: Add driver for Astera Labs PT516XX retimer 2023-12-05 7:47 ` [PATCH 3/3] hwmon: Add driver for Astera Labs PT516XX retimer Cosmo Chou @ 2023-12-05 15:55 ` Guenter Roeck 2023-12-09 8:02 ` Cosmo Chou 2023-12-05 23:24 ` kernel test robot 2023-12-11 17:43 ` Rob Herring 2 siblings, 1 reply; 12+ messages in thread From: Guenter Roeck @ 2023-12-05 15:55 UTC (permalink / raw) To: Cosmo Chou, jdelvare, robh+dt, krzysztof.kozlowski+dt, conor+dt, corbet, heiko, jernej.skrabec, macromorgan, linus.walleij Cc: linux-hwmon, devicetree, linux-kernel, linux-doc, cosmo.chou On 12/4/23 23:47, Cosmo Chou wrote: > This driver implements support for temperature monitoring of Astera Labs > PT5161L series PCIe retimer chips. > > This driver implementation originates from the CSDK available at > Link: https://github.com/facebook/openbmc/tree/helium/common/recipes-lib/retimer-v2.14 > The communication protocol utilized is based on the I2C/SMBus standard. > > Signed-off-by: Cosmo Chou <chou.cosmo@gmail.com> > --- > Documentation/hwmon/index.rst | 1 + > Documentation/hwmon/pt516xx.rst | 48 +++ > MAINTAINERS | 8 + > drivers/hwmon/Kconfig | 10 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/pt516xx.c | 648 ++++++++++++++++++++++++++++++++ > 6 files changed, 716 insertions(+) > create mode 100644 Documentation/hwmon/pt516xx.rst > create mode 100644 drivers/hwmon/pt516xx.c > > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst > index 72f4e6065bae..2c4df18db663 100644 > --- a/Documentation/hwmon/index.rst > +++ b/Documentation/hwmon/index.rst > @@ -181,6 +181,7 @@ Hardware Monitoring Kernel Drivers > pmbus > powerz > powr1220 > + pt516xx > pxe1610 > pwm-fan > q54sj108a2 > diff --git a/Documentation/hwmon/pt516xx.rst b/Documentation/hwmon/pt516xx.rst > new file mode 100644 > index 000000000000..945194f9a804 > --- /dev/null > +++ b/Documentation/hwmon/pt516xx.rst > @@ -0,0 +1,48 @@ > +.. SPDX-License-Identifier: GPL-2.0-or-later > + > +Kernel driver pt516xx > +==================== > + > +Supported chips: > + > + * Astera Labs PT5161L > + > + Prefix: 'pt5161l' > + This should be the driver name and be used for function/variable prefixes. No idea where XX comes into play. The driver for sure won't support PT516[0-9][A-Z], and should not suggest that it does. > + Addresses: I2C 0x24 > + > + Datasheet: http://www.asteralabs.com/wp-content/uploads/2021/03/Astera_Labs_PT5161L_Product_Brief.pdf > + > +Authors: Cosmo Chou <cosmo.chou@quantatw.com> > + > +Description > +----------- > + > +This driver implements support for temperature monitoring of Astera Labs > +PT5161L series PCIe retimer chips. > + > +This driver implementation originates from the CSDK available at > +https://github.com/facebook/openbmc/tree/helium/common/recipes-lib/retimer-v2.14 > +The communication protocol utilized is based on the I2C/SMBus standard. > + That strongly suggests that the code has a copyright and license associated with it. None of it is mentioned here. > +For more detailed information and specific implementation details, it is > +recommended to refer to the CSDK source code available at the provided GitHub > +link. > + Linux kernel drivers should be self contained. It is fine to reference documentation, but not out-of-tree source code. > +Sysfs entries > +---------------- > + > +================ ============================================== > +temp1_input Measured temperature (in millidegrees Celsius) > +================ ============================================== > + > +Debugfs entries > +---------------- > + > +================ ==================================== > +fw_ver Firmware version of the retimer > +health Health status (8 bits) > + [0]: Heartbeat Okay (1'b1: OK) > + [1]: Firmware loaded Okay (1'b1: OK) This is not from the chip but arbitrarily assigned. It should be reported in separate debugfs files (or all in a single file with multiple lines and human readable information). > + [7:2]: Reserved > +================ ==================================== > diff --git a/MAINTAINERS b/MAINTAINERS > index 788be9ab5b73..492002ffd12c 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -17482,6 +17482,14 @@ F: fs/pstore/ > F: include/linux/pstore* > K: \b(pstore|ramoops) > > +PT516XX HARDWARE MONITOR DRIVER > +M: Cosmo Chou <cosmo.chou@quantatw.com> > +L: linux-hwmon@vger.kernel.org > +S: Maintained > +F: Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml > +F: Documentation/hwmon/pt516xx.rst > +F: drivers/hwmon/pt516xx.c > + > PTP HARDWARE CLOCK SUPPORT > M: Richard Cochran <richardcochran@gmail.com> > L: netdev@vger.kernel.org > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > index cf27523eed5a..3965bec7774a 100644 > --- a/drivers/hwmon/Kconfig > +++ b/drivers/hwmon/Kconfig > @@ -1703,6 +1703,16 @@ source "drivers/hwmon/peci/Kconfig" > > source "drivers/hwmon/pmbus/Kconfig" > > +config SENSORS_PT516XX > + tristate "Astera Labs PT516XX PCIe retimer hardware monitoring" > + depends on I2C > + help > + If you say yes here you get support for temperature monitoring > + on the Astera Labs PT516XX PCIe retimer. > + > + This driver can also be built as a module. If so, the module > + will be called pt516xx. > + > config SENSORS_PWM_FAN > tristate "PWM fan" > depends on (PWM && OF) || COMPILE_TEST > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > index e84bd9685b5c..1942064cd4e9 100644 > --- a/drivers/hwmon/Makefile > +++ b/drivers/hwmon/Makefile > @@ -179,6 +179,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_PT516XX) += pt516xx.o > obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o > obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o > obj-$(CONFIG_SENSORS_SBTSI) += sbtsi_temp.o > diff --git a/drivers/hwmon/pt516xx.c b/drivers/hwmon/pt516xx.c > new file mode 100644 > index 000000000000..824798559fe1 > --- /dev/null > +++ b/drivers/hwmon/pt516xx.c > @@ -0,0 +1,648 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > + > +#include <linux/debugfs.h> > +#include <linux/delay.h> > +#include <linux/err.h> > +#include <linux/i2c.h> > +#include <linux/init.h> > +#include <linux/hwmon.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of_device.h> > + > +/** Main-micro-assisted access command codes */ This is not a proper kernel documentation. Either case, use C style comments or C++ style comments, but not both. > +// Wide register reads > +#define ARIES_MM_RD_WIDE_REG_2B 0x1d > +#define ARIES_MM_RD_WIDE_REG_3B 0x1e > +#define ARIES_MM_RD_WIDE_REG_4B 0x1f > +#define ARIES_MM_RD_WIDE_REG_5B 0x20 > +// Wide register writes > +#define ARIES_MM_WR_WIDE_REG_2B 0x21 > +#define ARIES_MM_WR_WIDE_REG_3B 0x22 > +#define ARIES_MM_WR_WIDE_REG_4B 0x23 > +#define ARIES_MM_WR_WIDE_REG_5B 0x24 Please use #define<space>NAME<tab>value throughout, please. Also, at least ARIES_MM_WR_WIDE_REG_ defines are not used. Please refrain from declaring unused defines. And where does ARIES come from ? > + > +/** Temperature measurement constants */ > +// Aries current average temp ADC code CSR > +#define ARIES_CURRENT_AVG_TEMP_ADC_CSR 0x42c > + > +// Main Micro Heartbeat reg > +#define ARIES_MM_HEARTBEAT_ADDR 0x923 > + > +/** Main SRAM */ > +// AL Main SRAM DMEM offset (A0) > +#define AL_MAIN_SRAM_DMEM_OFFSET (64 * 1024) > +// SRAM read command > +#define AL_TG_RD_LOC_IND_SRAM 0x16 > + > +/** Micros */ > +// Offset for main micro FW info > +#define ARIES_MAIN_MICRO_FW_INFO (96 * 1024 - 128) > + > +/** Path Micro Members */ > +// FW Info (Major) offset location in struct > +#define ARIES_MM_FW_VERSION_MAJOR 0 > +// FW Info (Minor) offset location in struct > +#define ARIES_MM_FW_VERSION_MINOR 1 > +// FW Info (Build no.) offset location in struct > +#define ARIES_MM_FW_VERSION_BUILD 2 > + > +/** Offsets for MM assisted accesses */ > +// Legacy Address and Data registers (using wide registers) > +// Reg offset to specify Address for MM assisted accesses > +#define ARIES_MM_ASSIST_REG_ADDR_OFFSET 0xd99 > +// Reg offset to specify Command > +#define ARIES_MM_ASSIST_CMD_OFFSET 0xd9d > + > +// New Address and Data registers (not using wide registers) > +// Reg offset to MM SPARE 0 used specify Address[7:0] > +#define ARIES_MM_ASSIST_SPARE_0_OFFSET 0xd9f > +// Reg offset to MM SPARE 3 used specify Data Byte 0 > +#define ARIES_MM_ASSIST_SPARE_3_OFFSET 0xda2 > + > +/** Misc block offsets */ > +// Device Load check register > +#define ARIES_CODE_LOAD_REG 0x605 > + > +/** Value indicating FW was loaded properly */ > +#define ARIES_LOAD_CODE 0xe > + > +/** EEPROM parameters */ > +// Time delay between checking MM status of EEPROM write (microseconds) > +#define ARIES_MM_STATUS_TIME 5000 > + > +#define ARIES_TEMP_CAL_CODE_DEFAULT 84 > + > +/* Struct defining FW version loaded on an Aries device */ > +struct pt516xx_fw_ver { > + u8 major; // FW version major release value > + u8 minor; // FW version minor release value > + u16 build; // FW version build release value > +}; > + > +/* Each client has this additional data */ > +struct pt516xx_data { > + struct i2c_client *client; > + struct dentry *debugfs; > + struct pt516xx_fw_ver fw_ver; > + struct mutex lock; > + bool init_done; > + bool pec_enable; // Enable PEC > + bool code_load_okay; // indicate if code load reg value is expected > + bool mm_heartbeat_okay; // indicate if Main Micro heartbeat is good > + bool mm_wide_reg_valid; // MM assisted Wide Register access > + u8 temp_cal_code_avg; > +}; > + > +static struct dentry *pt516xx_debugfs_dir; > + > +/* > + * Write multiple data bytes to Aries over I2C > + */ > +static int pt516xx_write_block_data(struct pt516xx_data *data, u32 address, > + u8 len, u8 *val) > +{ > + struct i2c_client *client = data->client; > + int ret; > + u8 remain_len = len; > + u8 xfer_len, curr_len; > + u8 buf[16]; > + u8 cmd = 0x0F; // [7]:pec_en, [6:5]:rsvd, [4:2]:func, [1]:start, [0]:end > + u8 config = 0x40; // [6]:cfg_type, [4:1]:burst_len, [0]:bit16 of address > + > + if (data->pec_enable) > + cmd |= 0x80; > + > + // If byte count is greater than 4, perform multiple iterations Most of those comments are pointless. This is obvious from the code. > + while (remain_len > 0) { > + if (remain_len > 4) { > + curr_len = 4; > + remain_len -= 4; > + } else { > + curr_len = remain_len; > + remain_len = 0; > + } > + > + buf[0] = config | (curr_len - 1) << 1 | ((address >> 16) & 0x1); > + buf[1] = (address >> 8) & 0xff; > + buf[2] = address & 0xff; > + memcpy(&buf[3], val, curr_len); > + > + xfer_len = 3 + curr_len; > + ret = i2c_smbus_write_block_data(client, cmd, xfer_len, buf); > + if (ret) > + return ret; > + > + val += curr_len; > + address += curr_len; > + } > + > + return 0; > +} > + > +/* > + * Read multiple data bytes from Aries over I2C > + */ > +static int pt516xx_read_block_data(struct pt516xx_data *data, u32 address, > + u8 len, u8 *val) > +{ > + struct i2c_client *client = data->client; > + int ret, tries; > + u8 remain_len = len; > + u8 curr_len; > + u8 wbuf[16], rbuf[24]; > + u8 cmd = 0x08; // [7]:pec_en, [6:5]:rsvd, [4:2]:func, [1]:start, [0]:end > + u8 config = 0x00; // [6]:cfg_type, [4:1]:burst_len, [0]:bit16 of address > + > + if (data->pec_enable) > + cmd |= 0x80; > + > + // If byte count is greater than 16, perform multiple iterations > + while (remain_len > 0) { > + if (remain_len > 16) { > + curr_len = 16; > + remain_len -= 16; > + } else { > + curr_len = remain_len; > + remain_len = 0; > + } > + > + wbuf[0] = config | (curr_len - 1) << 1 | > + ((address >> 16) & 0x1); > + wbuf[1] = (address >> 8) & 0xff; > + wbuf[2] = address & 0xff; > + > + // Perform read operation > + for (tries = 0; tries < 3; tries++) { > + ret = i2c_smbus_write_block_data(client, (cmd | 0x2), 3, > + wbuf); > + if (ret) > + return ret; > + > + ret = i2c_smbus_read_block_data(client, (cmd | 0x1), > + rbuf); > + if (ret == curr_len) > + break; > + } > + if (tries >= 3) > + return -ENODATA; > + > + memcpy(val, rbuf, curr_len); > + val += curr_len; > + address += curr_len; > + } > + > + return 0; > +} > + > +static int pt516xx_read_wide_reg(struct pt516xx_data *data, u32 address, > + u8 width, u8 *val) > +{ > + int ret, tries; > + u8 buf[8]; > + u8 status; > + > + if (data->mm_wide_reg_valid) { > + // Write address (3 bytes) > + buf[0] = address & 0xff; > + buf[1] = (address >> 8) & 0xff; > + buf[2] = (address >> 16) & 0x1; > + ret = pt516xx_write_block_data( > + data, ARIES_MM_ASSIST_SPARE_0_OFFSET, 3, buf); > + if (ret) > + return ret; > + > + // Set command based on width > + switch (width) { > + case 2: > + buf[0] = ARIES_MM_RD_WIDE_REG_2B; > + break; > + case 3: > + buf[0] = ARIES_MM_RD_WIDE_REG_3B; > + break; > + case 4: > + buf[0] = ARIES_MM_RD_WIDE_REG_4B; > + break; > + case 5: > + buf[0] = ARIES_MM_RD_WIDE_REG_5B; > + break; > + default: > + return -EINVAL; > + } > + ret = pt516xx_write_block_data(data, ARIES_MM_ASSIST_CMD_OFFSET, > + 1, buf); > + if (ret) > + return ret; > + > + // Check access status > + status = 0xff; > + for (tries = 0; tries < 100; tries++) { > + ret = pt516xx_read_block_data( > + data, ARIES_MM_ASSIST_CMD_OFFSET, 1, &status); > + if (ret) > + return ret; > + > + if (status == 0) > + break; > + > + usleep_range(ARIES_MM_STATUS_TIME, > + ARIES_MM_STATUS_TIME + 1000); > + } > + if (status != 0) > + return -ETIMEDOUT; > + > + // Read N bytes of data based on width > + ret = pt516xx_read_block_data( > + data, ARIES_MM_ASSIST_SPARE_3_OFFSET, width, val); > + if (ret) > + return ret; > + } else { > + return pt516xx_read_block_data(data, address, width, val); > + } > + > + return 0; > +} > + > +/* > + * Read multiple (up to eight) data bytes from micro SRAM over I2C > + */ > +static int > +pt516xx_read_block_data_main_micro_indirect(struct pt516xx_data *data, > + u32 address, u8 len, u8 *val) > +{ > + int ret, tries; > + u8 buf[8]; > + u8 i, status; > + u32 uind_offs = ARIES_MM_ASSIST_REG_ADDR_OFFSET; > + u32 eeprom_base, eeprom_addr; > + > + // No multi-byte indirect support here. Hence read a byte at a time > + eeprom_base = address - AL_MAIN_SRAM_DMEM_OFFSET; > + for (i = 0; i < len; i++) { > + // Write eeprom addr > + eeprom_addr = eeprom_base + i; > + buf[0] = eeprom_addr & 0xff; > + buf[1] = (eeprom_addr >> 8) & 0xff; > + buf[2] = (eeprom_addr >> 16) & 0xff; > + ret = pt516xx_write_block_data(data, uind_offs, 3, buf); > + if (ret) > + return ret; > + > + // Write eeprom cmd > + buf[0] = AL_TG_RD_LOC_IND_SRAM; > + ret = pt516xx_write_block_data(data, uind_offs + 4, 1, buf); > + if (ret) > + return ret; > + > + // Test successful access > + status = 0xff; > + for (tries = 0; tries < 255; tries++) { > + ret = pt516xx_read_block_data(data, uind_offs + 4, 1, > + &status); > + if (ret) > + return ret; > + > + if (status == 0) > + break; > + } > + if (status != 0) > + return -ETIMEDOUT; > + > + ret = pt516xx_read_block_data(data, uind_offs + 3, 1, buf); > + if (ret) > + return ret; > + > + val[i] = buf[0]; > + } > + > + return 0; > +} > + > +/* > + * Check the status of firmware > + */ > +static int pt516xx_fwsts_check(struct pt516xx_data *data) > +{ > + int ret, tries; > + u8 buf[8]; > + u8 heartbeat, major = 0, minor = 0; > + u16 build = 0; > + bool hb_changed = false; > + > + // Read Code Load reg > + ret = pt516xx_read_block_data(data, ARIES_CODE_LOAD_REG, 1, buf); > + if (ret) > + return ret; > + > + if (buf[0] < ARIES_LOAD_CODE) { > + dev_warn( > + &data->client->dev, > + "Code Load reg unexpected. Not all modules are loaded %x\n", > + buf[0]); > + data->code_load_okay = false; > + } else { > + data->code_load_okay = true; > + } > + > + // Check Main Micro heartbeat > + // If heartbeat value does not change for 100 tries, no MM heartbeat > + // Else heartbeat present even if one value changes > + ret = pt516xx_read_block_data(data, ARIES_MM_HEARTBEAT_ADDR, 1, buf); > + if (ret) > + return ret; > + > + heartbeat = buf[0]; > + for (tries = 0; tries < 100; tries++) { > + ret = pt516xx_read_block_data(data, ARIES_MM_HEARTBEAT_ADDR, 1, > + buf); > + if (ret) > + return ret; > + > + if (buf[0] != heartbeat) { > + hb_changed = true; > + break; > + } > + } > + data->mm_heartbeat_okay = hb_changed; > + > + // Read FW version > + // If heartbeat not there, set default FW values to 0.0.0 > + // and return success > + if (data->mm_heartbeat_okay) { > + // Get FW version (major) > + ret = pt516xx_read_block_data_main_micro_indirect( > + data, > + ARIES_MAIN_MICRO_FW_INFO + ARIES_MM_FW_VERSION_MAJOR, 1, > + &major); > + if (ret) > + return ret; > + > + // Get FW version (minor) > + ret = pt516xx_read_block_data_main_micro_indirect( > + data, > + ARIES_MAIN_MICRO_FW_INFO + ARIES_MM_FW_VERSION_MINOR, 1, > + &minor); > + if (ret) > + return ret; > + > + // Get FW version (build) > + ret = pt516xx_read_block_data_main_micro_indirect( > + data, > + ARIES_MAIN_MICRO_FW_INFO + ARIES_MM_FW_VERSION_BUILD, 2, > + (u8 *)&build); > + if (ret) > + return ret; > + } else { > + dev_warn(&data->client->dev, "No Main Micro Heartbeat\n"); This and other messages above would create a lot of noise if persistent since the function is called repeatedly. > + } > + data->fw_ver.major = major; > + data->fw_ver.minor = minor; > + data->fw_ver.build = build; > + > + return 0; > +} > + > +static int pt516xx_fw_is_at_least(struct pt516xx_data *data, u8 major, u8 minor, > + u16 build) > +{ > + u32 ver = major << 24 | minor << 16 | build; > + u32 curr_ver = data->fw_ver.major << 24 | data->fw_ver.minor << 16 | > + data->fw_ver.build; > + > + if (curr_ver >= ver) > + return true; > + > + return false; > +} > + > +static int pt516xx_init_dev(struct pt516xx_data *data) > +{ > + int ret; > + > + mutex_lock(&data->lock); > + ret = pt516xx_fwsts_check(data); > + mutex_unlock(&data->lock); > + if (ret) > + return ret; > + > + dev_info(&data->client->dev, "fw_ver: %u.%u.%u\n", data->fw_ver.major, > + data->fw_ver.minor, data->fw_ver.build); > + Please no such noise > + if (pt516xx_fw_is_at_least(data, 2, 2, 0)) > + data->mm_wide_reg_valid = true; > + > + data->temp_cal_code_avg = ARIES_TEMP_CAL_CODE_DEFAULT; What is the point of this ? temp_cal_code_avg will never be anything else. > + data->init_done = true; > + > + return 0; > +} > + > +static int pt516xx_read(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long *val) > +{ > + struct pt516xx_data *data = dev_get_drvdata(dev); > + int ret = 0; > + long adc_code = 0; > + This assumes a specific endianness which isn't given. I am quite sure that the code as written won't work in a big endian system. Also, "long" may be 4 or 8 byte depending on the CPU architecture. > + if (!data->init_done) { > + ret = pt516xx_init_dev(data); > + if (ret) { > + dev_err(dev, "pt516xx_init_dev failed %d\n", ret); > + return ret; > + } > + } This is really the wrong place for this code. It should be checked and initialized in the probe function, and probe should bail out if it fails. Also, the only reason to call pt516xx_init_dev() seems to be to determine if "wide reegister access" is supported. If it isn't, pt516xx_read_block_data() is used. Why not call pt516xx_read_block_data() below directly and not bother with wide register access ? > + > + switch (attr) { > + case hwmon_temp_input: > + mutex_lock(&data->lock); > + ret = pt516xx_read_wide_reg(data, > + ARIES_CURRENT_AVG_TEMP_ADC_CSR, 4, > + (u8 *)&adc_code); > + mutex_unlock(&data->lock); > + break; > + default: > + return -EOPNOTSUPP; > + } > + if (ret) { > + dev_err(dev, "Read adc_code failed %d\n", ret); > + return ret; > + } > + if (adc_code == 0 || adc_code >= 0x3ff) { > + dev_err(dev, "Invalid adc_code %lx\n", adc_code); > + return -EINVAL; I guess this is supposed to cover an error return from pt516xx_read_wide_reg() returned an error, assuming that adc_code was not overwritten in that case. This is inappropriate. Error returns from pt516xx_read_wide_reg() should be handled explicitly (and don't indicate "Invalid argument"). > + } > + The above will create logging noise if the chip has a problem. Please make it dev_dbg. > + *val = 110000 + ((adc_code - (data->temp_cal_code_avg + 250)) * -320); > + > + return 0; > +} > + > +static umode_t pt516xx_is_visible(const void *data, > + enum hwmon_sensor_types type, u32 attr, > + int channel) > +{ > + switch (attr) { > + case hwmon_temp_input: > + return 0444; > + default: > + break; > + } > + > + return 0; > +} > + > +static const struct hwmon_channel_info *pt516xx_info[] = { > + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT), > + NULL > +}; > + > +static const struct hwmon_ops pt516xx_hwmon_ops = { > + .is_visible = pt516xx_is_visible, > + .read = pt516xx_read, > +}; > + > +static const struct hwmon_chip_info pt516xx_chip_info = { > + .ops = &pt516xx_hwmon_ops, > + .info = pt516xx_info, > +}; > + > +static ssize_t pt516xx_debugfs_read_fw_ver(struct file *file, char __user *buf, > + size_t count, loff_t *ppos) > +{ > + struct pt516xx_data *data = file->private_data; > + int ret; > + char ver[32]; > + > + mutex_lock(&data->lock); > + ret = pt516xx_fwsts_check(data); > + mutex_unlock(&data->lock); What is the point of (re-)reading the firmware version repeatedly ? > + if (ret) > + return ret; > + > + ret = snprintf(ver, sizeof(ver), "%u.%u.%u\n", data->fw_ver.major, > + data->fw_ver.minor, data->fw_ver.build); > + if (ret < 0) > + return ret; > + > + return simple_read_from_buffer(buf, count, ppos, ver, ret + 1); > +} > + > +static const struct file_operations pt516xx_debugfs_ops_fw_ver = { > + .read = pt516xx_debugfs_read_fw_ver, > + .open = simple_open, > +}; > + > +static ssize_t pt516xx_debugfs_read_health(struct file *file, char __user *buf, > + size_t count, loff_t *ppos) > +{ > + struct pt516xx_data *data = file->private_data; > + int ret; > + u8 status = 0; > + char health[8]; > + > + mutex_lock(&data->lock); > + ret = pt516xx_fwsts_check(data); > + mutex_unlock(&data->lock); > + if (ret == 0) { > + status |= data->mm_heartbeat_okay ? 1 : 0; // bit0 > + status |= data->code_load_okay ? 2 : 0; // bit1 Those should really be separate debugfs files. > + } > + > + ret = snprintf(health, sizeof(health), "0x%02x\n", status); > + if (ret < 0) > + return ret; > + > + return simple_read_from_buffer(buf, count, ppos, health, ret + 1); > +} > + > +static const struct file_operations pt516xx_debugfs_ops_health = { > + .read = pt516xx_debugfs_read_health, > + .open = simple_open, > +}; > + > +static int pt516xx_init_debugfs(struct pt516xx_data *data) > +{ > + if (!pt516xx_debugfs_dir) > + return -ENOENT; > + debugfs functions handle this situation, and return values therefore do not need to be checked. > + data->debugfs = debugfs_create_dir(dev_name(&data->client->dev), > + pt516xx_debugfs_dir); > + if (IS_ERR_OR_NULL(data->debugfs)) > + return -ENOENT; > + Again, unnecessary. > + debugfs_create_file("fw_ver", 0444, data->debugfs, data, > + &pt516xx_debugfs_ops_fw_ver); > + > + debugfs_create_file("health", 0444, data->debugfs, data, > + &pt516xx_debugfs_ops_health); > + > + return 0; > +} > + > +static int pt516xx_probe(struct i2c_client *client) > +{ > + struct device *dev = &client->dev; > + struct device *hwmon_dev; > + struct pt516xx_data *data; > + > + data = devm_kzalloc(dev, sizeof(struct pt516xx_data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->client = client; > + mutex_init(&data->lock); > + pt516xx_init_dev(data); > + > + hwmon_dev = devm_hwmon_device_register_with_info( > + dev, client->name, data, &pt516xx_chip_info, NULL); > + > + if (pt516xx_init_debugfs(data)) > + dev_warn(dev, "Failed to register debugfs\n"); debugfs should fail silently. debugfs files are not removed if a single device is unloaded. Did you check what happens in this case ? > + > + return PTR_ERR_OR_ZERO(hwmon_dev); > +} > + > +static const struct i2c_device_id pt516xx_id[] = { > + { "pt5161l", 0 }, > + {} > +}; > +MODULE_DEVICE_TABLE(i2c, pt516xx_id); > + > +static const struct of_device_id __maybe_unused pt516xx_of_match[] = { > + { .compatible = "asteralabs,pt5161l" }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, pt516xx_of_match); > + > +static struct i2c_driver pt516xx_driver = { > + .class = I2C_CLASS_HWMON, > + .driver = { > + .name = "pt516xx", > + .of_match_table = of_match_ptr(pt516xx_of_match), This means the driver won't instantiate on an ACPI system if devicetree support is disabled. Is this intentional ? > + }, > + .probe = pt516xx_probe, > + .id_table = pt516xx_id, > +}; > + > +module_i2c_driver(pt516xx_driver); > + > +static int __init pt516xx_core_init(void) > +{ > + pt516xx_debugfs_dir = debugfs_create_dir("pt516xx", NULL); > + if (IS_ERR(pt516xx_debugfs_dir)) > + pt516xx_debugfs_dir = NULL; > + > + return 0; > +} > + > +static void __exit pt516xx_core_exit(void) > +{ > + debugfs_remove_recursive(pt516xx_debugfs_dir); > +} > + > +module_init(pt516xx_core_init); > +module_exit(pt516xx_core_exit); > + > +MODULE_AUTHOR("Cosmo Chou <cosmo.chou@quantatw.com>"); > +MODULE_DESCRIPTION("Hwmon driver for Astera Labs Aries PCIe retimer"); > +MODULE_LICENSE("GPL"); ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] hwmon: Add driver for Astera Labs PT516XX retimer 2023-12-05 15:55 ` Guenter Roeck @ 2023-12-09 8:02 ` Cosmo Chou 0 siblings, 0 replies; 12+ messages in thread From: Cosmo Chou @ 2023-12-09 8:02 UTC (permalink / raw) To: Guenter Roeck Cc: jdelvare, robh+dt, krzysztof.kozlowski+dt, conor+dt, corbet, heiko, jernej.skrabec, macromorgan, linus.walleij, linux-hwmon, devicetree, linux-kernel, linux-doc, cosmo.chou On 12/5/2023 07:55:50 -0800 Guenter Roeck <linux@roeck-us.net> wrote: > > On 12/4/23 23:47, Cosmo Chou wrote: > > This driver implements support for temperature monitoring of Astera Labs > > PT5161L series PCIe retimer chips. > > > > This driver implementation originates from the CSDK available at > > Link: https://github.com/facebook/openbmc/tree/helium/common/recipes-lib/retimer-v2.14 > > The communication protocol utilized is based on the I2C/SMBus standard. > > > > Signed-off-by: Cosmo Chou <chou.cosmo@gmail.com> > > --- > > Documentation/hwmon/index.rst | 1 + > > Documentation/hwmon/pt516xx.rst | 48 +++ > > MAINTAINERS | 8 + > > drivers/hwmon/Kconfig | 10 + > > drivers/hwmon/Makefile | 1 + > > drivers/hwmon/pt516xx.c | 648 ++++++++++++++++++++++++++++++++ > > 6 files changed, 716 insertions(+) > > create mode 100644 Documentation/hwmon/pt516xx.rst > > create mode 100644 drivers/hwmon/pt516xx.c > > > > diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst > > index 72f4e6065bae..2c4df18db663 100644 > > --- a/Documentation/hwmon/index.rst > > +++ b/Documentation/hwmon/index.rst > > @@ -181,6 +181,7 @@ Hardware Monitoring Kernel Drivers > > pmbus > > powerz > > powr1220 > > + pt516xx > > pxe1610 > > pwm-fan > > q54sj108a2 > > diff --git a/Documentation/hwmon/pt516xx.rst b/Documentation/hwmon/pt516xx.rst > > new file mode 100644 > > index 000000000000..945194f9a804 > > --- /dev/null > > +++ b/Documentation/hwmon/pt516xx.rst > > @@ -0,0 +1,48 @@ > > +.. SPDX-License-Identifier: GPL-2.0-or-later > > + > > +Kernel driver pt516xx > > +==================== > > + > > +Supported chips: > > + > > + * Astera Labs PT5161L > > + > > + Prefix: 'pt5161l' > > + > > This should be the driver name and be used for function/variable prefixes. > No idea where XX comes into play. The driver for sure won't support > PT516[0-9][A-Z], and should not suggest that it does. > OK, I will correct it to pt5161l. > > + Addresses: I2C 0x24 > > + > > + Datasheet: http://www.asteralabs.com/wp-content/uploads/2021/03/Astera_Labs_PT5161L_Product_Brief.pdf > > + > > +Authors: Cosmo Chou <cosmo.chou@quantatw.com> > > + > > +Description > > +----------- > > + > > +This driver implements support for temperature monitoring of Astera Labs > > +PT5161L series PCIe retimer chips. > > + > > +This driver implementation originates from the CSDK available at > > +https://github.com/facebook/openbmc/tree/helium/common/recipes-lib/retimer-v2.14 > > +The communication protocol utilized is based on the I2C/SMBus standard. > > + > > That strongly suggests that the code has a copyright and license associated > with it. None of it is mentioned here. > It looks like the base code (the link above) declared GPL2.0+ and Apache2.0. There is already a declaration in the pt516xx.c: (line 1) // SPDX-License-Identifier: GPL-2.0-or-later Could you give some advice about adding the copyright and license? Thanks. > > +For more detailed information and specific implementation details, it is > > +recommended to refer to the CSDK source code available at the provided GitHub > > +link. > > + > > Linux kernel drivers should be self contained. It is fine to reference documentation, > but not out-of-tree source code. > OK. I will remove this paragraph ("For more detailed information...") > > +Sysfs entries > > +---------------- > > + > > +================ ============================================== > > +temp1_input Measured temperature (in millidegrees Celsius) > > +================ ============================================== > > + > > +Debugfs entries > > +---------------- > > + > > +================ ==================================== > > +fw_ver Firmware version of the retimer > > +health Health status (8 bits) > > + [0]: Heartbeat Okay (1'b1: OK) > > + [1]: Firmware loaded Okay (1'b1: OK) > > This is not from the chip but arbitrarily assigned. It should be > reported in separate debugfs files (or all in a single file with > multiple lines and human readable information). > OK. I will separate it to "fw_load_status" and "heartbeat_status". > > + [7:2]: Reserved > > +================ ==================================== > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 788be9ab5b73..492002ffd12c 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -17482,6 +17482,14 @@ F: fs/pstore/ > > F: include/linux/pstore* > > K: \b(pstore|ramoops) > > > > +PT516XX HARDWARE MONITOR DRIVER > > +M: Cosmo Chou <cosmo.chou@quantatw.com> > > +L: linux-hwmon@vger.kernel.org > > +S: Maintained > > +F: Documentation/devicetree/bindings/hwmon/asteralabs,pt516xx.yaml > > +F: Documentation/hwmon/pt516xx.rst > > +F: drivers/hwmon/pt516xx.c > > + > > PTP HARDWARE CLOCK SUPPORT > > M: Richard Cochran <richardcochran@gmail.com> > > L: netdev@vger.kernel.org > > diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig > > index cf27523eed5a..3965bec7774a 100644 > > --- a/drivers/hwmon/Kconfig > > +++ b/drivers/hwmon/Kconfig > > @@ -1703,6 +1703,16 @@ source "drivers/hwmon/peci/Kconfig" > > > > source "drivers/hwmon/pmbus/Kconfig" > > > > +config SENSORS_PT516XX > > + tristate "Astera Labs PT516XX PCIe retimer hardware monitoring" > > + depends on I2C > > + help > > + If you say yes here you get support for temperature monitoring > > + on the Astera Labs PT516XX PCIe retimer. > > + > > + This driver can also be built as a module. If so, the module > > + will be called pt516xx. > > + > > config SENSORS_PWM_FAN > > tristate "PWM fan" > > depends on (PWM && OF) || COMPILE_TEST > > diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile > > index e84bd9685b5c..1942064cd4e9 100644 > > --- a/drivers/hwmon/Makefile > > +++ b/drivers/hwmon/Makefile > > @@ -179,6 +179,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_PT516XX) += pt516xx.o > > obj-$(CONFIG_SENSORS_PWM_FAN) += pwm-fan.o > > obj-$(CONFIG_SENSORS_RASPBERRYPI_HWMON) += raspberrypi-hwmon.o > > obj-$(CONFIG_SENSORS_SBTSI) += sbtsi_temp.o > > diff --git a/drivers/hwmon/pt516xx.c b/drivers/hwmon/pt516xx.c > > new file mode 100644 > > index 000000000000..824798559fe1 > > --- /dev/null > > +++ b/drivers/hwmon/pt516xx.c > > @@ -0,0 +1,648 @@ > > +// SPDX-License-Identifier: GPL-2.0-or-later > > + > > +#include <linux/debugfs.h> > > +#include <linux/delay.h> > > +#include <linux/err.h> > > +#include <linux/i2c.h> > > +#include <linux/init.h> > > +#include <linux/hwmon.h> > > +#include <linux/module.h> > > +#include <linux/mutex.h> > > +#include <linux/of_device.h> > > + > > +/** Main-micro-assisted access command codes */ > > This is not a proper kernel documentation. Either case, use C style comments > or C++ style comments, but not both. > Got it. I will correct the style of comments. > > +// Wide register reads > > +#define ARIES_MM_RD_WIDE_REG_2B 0x1d > > +#define ARIES_MM_RD_WIDE_REG_3B 0x1e > > +#define ARIES_MM_RD_WIDE_REG_4B 0x1f > > +#define ARIES_MM_RD_WIDE_REG_5B 0x20 > > +// Wide register writes > > +#define ARIES_MM_WR_WIDE_REG_2B 0x21 > > +#define ARIES_MM_WR_WIDE_REG_3B 0x22 > > +#define ARIES_MM_WR_WIDE_REG_4B 0x23 > > +#define ARIES_MM_WR_WIDE_REG_5B 0x24 > > Please use > > #define<space>NAME<tab>value > Got it. > throughout, please. Also, at least ARIES_MM_WR_WIDE_REG_ defines are > not used. Please refrain from declaring unused defines. > Oops... I will remove the unused code. > And where does ARIES come from ? > It's the SoC name of the PCIe retimer. > > + > > +/** Temperature measurement constants */ > > +// Aries current average temp ADC code CSR > > +#define ARIES_CURRENT_AVG_TEMP_ADC_CSR 0x42c > > + > > +// Main Micro Heartbeat reg > > +#define ARIES_MM_HEARTBEAT_ADDR 0x923 > > + > > +/** Main SRAM */ > > +// AL Main SRAM DMEM offset (A0) > > +#define AL_MAIN_SRAM_DMEM_OFFSET (64 * 1024) > > +// SRAM read command > > +#define AL_TG_RD_LOC_IND_SRAM 0x16 > > + > > +/** Micros */ > > +// Offset for main micro FW info > > +#define ARIES_MAIN_MICRO_FW_INFO (96 * 1024 - 128) > > + > > +/** Path Micro Members */ > > +// FW Info (Major) offset location in struct > > +#define ARIES_MM_FW_VERSION_MAJOR 0 > > +// FW Info (Minor) offset location in struct > > +#define ARIES_MM_FW_VERSION_MINOR 1 > > +// FW Info (Build no.) offset location in struct > > +#define ARIES_MM_FW_VERSION_BUILD 2 > > + > > +/** Offsets for MM assisted accesses */ > > +// Legacy Address and Data registers (using wide registers) > > +// Reg offset to specify Address for MM assisted accesses > > +#define ARIES_MM_ASSIST_REG_ADDR_OFFSET 0xd99 > > +// Reg offset to specify Command > > +#define ARIES_MM_ASSIST_CMD_OFFSET 0xd9d > > + > > +// New Address and Data registers (not using wide registers) > > +// Reg offset to MM SPARE 0 used specify Address[7:0] > > +#define ARIES_MM_ASSIST_SPARE_0_OFFSET 0xd9f > > +// Reg offset to MM SPARE 3 used specify Data Byte 0 > > +#define ARIES_MM_ASSIST_SPARE_3_OFFSET 0xda2 > > + > > +/** Misc block offsets */ > > +// Device Load check register > > +#define ARIES_CODE_LOAD_REG 0x605 > > + > > +/** Value indicating FW was loaded properly */ > > +#define ARIES_LOAD_CODE 0xe > > + > > +/** EEPROM parameters */ > > +// Time delay between checking MM status of EEPROM write (microseconds) > > +#define ARIES_MM_STATUS_TIME 5000 > > + > > +#define ARIES_TEMP_CAL_CODE_DEFAULT 84 > > + > > +/* Struct defining FW version loaded on an Aries device */ > > +struct pt516xx_fw_ver { > > + u8 major; // FW version major release value > > + u8 minor; // FW version minor release value > > + u16 build; // FW version build release value > > +}; > > + > > +/* Each client has this additional data */ > > +struct pt516xx_data { > > + struct i2c_client *client; > > + struct dentry *debugfs; > > + struct pt516xx_fw_ver fw_ver; > > + struct mutex lock; > > + bool init_done; > > + bool pec_enable; // Enable PEC > > + bool code_load_okay; // indicate if code load reg value is expected > > + bool mm_heartbeat_okay; // indicate if Main Micro heartbeat is good > > + bool mm_wide_reg_valid; // MM assisted Wide Register access > > + u8 temp_cal_code_avg; > > +}; > > + > > +static struct dentry *pt516xx_debugfs_dir; > > + > > +/* > > + * Write multiple data bytes to Aries over I2C > > + */ > > +static int pt516xx_write_block_data(struct pt516xx_data *data, u32 address, > > + u8 len, u8 *val) > > +{ > > + struct i2c_client *client = data->client; > > + int ret; > > + u8 remain_len = len; > > + u8 xfer_len, curr_len; > > + u8 buf[16]; > > + u8 cmd = 0x0F; // [7]:pec_en, [6:5]:rsvd, [4:2]:func, [1]:start, [0]:end > > + u8 config = 0x40; // [6]:cfg_type, [4:1]:burst_len, [0]:bit16 of address > > + > > + if (data->pec_enable) > > + cmd |= 0x80; > > + > > + // If byte count is greater than 4, perform multiple iterations > > Most of those comments are pointless. This is obvious from the code. > OK. I will reduce the pointless comments. > > + while (remain_len > 0) { > > + if (remain_len > 4) { > > + curr_len = 4; > > + remain_len -= 4; > > + } else { > > + curr_len = remain_len; > > + remain_len = 0; > > + } > > + > > + buf[0] = config | (curr_len - 1) << 1 | ((address >> 16) & 0x1); > > + buf[1] = (address >> 8) & 0xff; > > + buf[2] = address & 0xff; > > + memcpy(&buf[3], val, curr_len); > > + > > + xfer_len = 3 + curr_len; > > + ret = i2c_smbus_write_block_data(client, cmd, xfer_len, buf); > > + if (ret) > > + return ret; > > + > > + val += curr_len; > > + address += curr_len; > > + } > > + > > + return 0; > > +} > > + > > +/* > > + * Read multiple data bytes from Aries over I2C > > + */ > > +static int pt516xx_read_block_data(struct pt516xx_data *data, u32 address, > > + u8 len, u8 *val) > > +{ > > + struct i2c_client *client = data->client; > > + int ret, tries; > > + u8 remain_len = len; > > + u8 curr_len; > > + u8 wbuf[16], rbuf[24]; > > + u8 cmd = 0x08; // [7]:pec_en, [6:5]:rsvd, [4:2]:func, [1]:start, [0]:end > > + u8 config = 0x00; // [6]:cfg_type, [4:1]:burst_len, [0]:bit16 of address > > + > > + if (data->pec_enable) > > + cmd |= 0x80; > > + > > + // If byte count is greater than 16, perform multiple iterations > > + while (remain_len > 0) { > > + if (remain_len > 16) { > > + curr_len = 16; > > + remain_len -= 16; > > + } else { > > + curr_len = remain_len; > > + remain_len = 0; > > + } > > + > > + wbuf[0] = config | (curr_len - 1) << 1 | > > + ((address >> 16) & 0x1); > > + wbuf[1] = (address >> 8) & 0xff; > > + wbuf[2] = address & 0xff; > > + > > + // Perform read operation > > + for (tries = 0; tries < 3; tries++) { > > + ret = i2c_smbus_write_block_data(client, (cmd | 0x2), 3, > > + wbuf); > > + if (ret) > > + return ret; > > + > > + ret = i2c_smbus_read_block_data(client, (cmd | 0x1), > > + rbuf); > > + if (ret == curr_len) > > + break; > > + } > > + if (tries >= 3) > > + return -ENODATA; > > + > > + memcpy(val, rbuf, curr_len); > > + val += curr_len; > > + address += curr_len; > > + } > > + > > + return 0; > > +} > > + > > +static int pt516xx_read_wide_reg(struct pt516xx_data *data, u32 address, > > + u8 width, u8 *val) > > +{ > > + int ret, tries; > > + u8 buf[8]; > > + u8 status; > > + > > + if (data->mm_wide_reg_valid) { > > + // Write address (3 bytes) > > + buf[0] = address & 0xff; > > + buf[1] = (address >> 8) & 0xff; > > + buf[2] = (address >> 16) & 0x1; > > + ret = pt516xx_write_block_data( > > + data, ARIES_MM_ASSIST_SPARE_0_OFFSET, 3, buf); > > + if (ret) > > + return ret; > > + > > + // Set command based on width > > + switch (width) { > > + case 2: > > + buf[0] = ARIES_MM_RD_WIDE_REG_2B; > > + break; > > + case 3: > > + buf[0] = ARIES_MM_RD_WIDE_REG_3B; > > + break; > > + case 4: > > + buf[0] = ARIES_MM_RD_WIDE_REG_4B; > > + break; > > + case 5: > > + buf[0] = ARIES_MM_RD_WIDE_REG_5B; > > + break; > > + default: > > + return -EINVAL; > > + } > > + ret = pt516xx_write_block_data(data, ARIES_MM_ASSIST_CMD_OFFSET, > > + 1, buf); > > + if (ret) > > + return ret; > > + > > + // Check access status > > + status = 0xff; > > + for (tries = 0; tries < 100; tries++) { > > + ret = pt516xx_read_block_data( > > + data, ARIES_MM_ASSIST_CMD_OFFSET, 1, &status); > > + if (ret) > > + return ret; > > + > > + if (status == 0) > > + break; > > + > > + usleep_range(ARIES_MM_STATUS_TIME, > > + ARIES_MM_STATUS_TIME + 1000); > > + } > > + if (status != 0) > > + return -ETIMEDOUT; > > + > > + // Read N bytes of data based on width > > + ret = pt516xx_read_block_data( > > + data, ARIES_MM_ASSIST_SPARE_3_OFFSET, width, val); > > + if (ret) > > + return ret; > > + } else { > > + return pt516xx_read_block_data(data, address, width, val); > > + } > > + > > + return 0; > > +} > > + > > +/* > > + * Read multiple (up to eight) data bytes from micro SRAM over I2C > > + */ > > +static int > > +pt516xx_read_block_data_main_micro_indirect(struct pt516xx_data *data, > > + u32 address, u8 len, u8 *val) > > +{ > > + int ret, tries; > > + u8 buf[8]; > > + u8 i, status; > > + u32 uind_offs = ARIES_MM_ASSIST_REG_ADDR_OFFSET; > > + u32 eeprom_base, eeprom_addr; > > + > > + // No multi-byte indirect support here. Hence read a byte at a time > > + eeprom_base = address - AL_MAIN_SRAM_DMEM_OFFSET; > > + for (i = 0; i < len; i++) { > > + // Write eeprom addr > > + eeprom_addr = eeprom_base + i; > > + buf[0] = eeprom_addr & 0xff; > > + buf[1] = (eeprom_addr >> 8) & 0xff; > > + buf[2] = (eeprom_addr >> 16) & 0xff; > > + ret = pt516xx_write_block_data(data, uind_offs, 3, buf); > > + if (ret) > > + return ret; > > + > > + // Write eeprom cmd > > + buf[0] = AL_TG_RD_LOC_IND_SRAM; > > + ret = pt516xx_write_block_data(data, uind_offs + 4, 1, buf); > > + if (ret) > > + return ret; > > + > > + // Test successful access > > + status = 0xff; > > + for (tries = 0; tries < 255; tries++) { > > + ret = pt516xx_read_block_data(data, uind_offs + 4, 1, > > + &status); > > + if (ret) > > + return ret; > > + > > + if (status == 0) > > + break; > > + } > > + if (status != 0) > > + return -ETIMEDOUT; > > + > > + ret = pt516xx_read_block_data(data, uind_offs + 3, 1, buf); > > + if (ret) > > + return ret; > > + > > + val[i] = buf[0]; > > + } > > + > > + return 0; > > +} > > + > > +/* > > + * Check the status of firmware > > + */ > > +static int pt516xx_fwsts_check(struct pt516xx_data *data) > > +{ > > + int ret, tries; > > + u8 buf[8]; > > + u8 heartbeat, major = 0, minor = 0; > > + u16 build = 0; > > + bool hb_changed = false; > > + > > + // Read Code Load reg > > + ret = pt516xx_read_block_data(data, ARIES_CODE_LOAD_REG, 1, buf); > > + if (ret) > > + return ret; > > + > > + if (buf[0] < ARIES_LOAD_CODE) { > > + dev_warn( > > + &data->client->dev, > > + "Code Load reg unexpected. Not all modules are loaded %x\n", > > + buf[0]); > > + data->code_load_okay = false; > > + } else { > > + data->code_load_okay = true; > > + } > > + > > + // Check Main Micro heartbeat > > + // If heartbeat value does not change for 100 tries, no MM heartbeat > > + // Else heartbeat present even if one value changes > > + ret = pt516xx_read_block_data(data, ARIES_MM_HEARTBEAT_ADDR, 1, buf); > > + if (ret) > > + return ret; > > + > > + heartbeat = buf[0]; > > + for (tries = 0; tries < 100; tries++) { > > + ret = pt516xx_read_block_data(data, ARIES_MM_HEARTBEAT_ADDR, 1, > > + buf); > > + if (ret) > > + return ret; > > + > > + if (buf[0] != heartbeat) { > > + hb_changed = true; > > + break; > > + } > > + } > > + data->mm_heartbeat_okay = hb_changed; > > + > > + // Read FW version > > + // If heartbeat not there, set default FW values to 0.0.0 > > + // and return success > > + if (data->mm_heartbeat_okay) { > > + // Get FW version (major) > > + ret = pt516xx_read_block_data_main_micro_indirect( > > + data, > > + ARIES_MAIN_MICRO_FW_INFO + ARIES_MM_FW_VERSION_MAJOR, 1, > > + &major); > > + if (ret) > > + return ret; > > + > > + // Get FW version (minor) > > + ret = pt516xx_read_block_data_main_micro_indirect( > > + data, > > + ARIES_MAIN_MICRO_FW_INFO + ARIES_MM_FW_VERSION_MINOR, 1, > > + &minor); > > + if (ret) > > + return ret; > > + > > + // Get FW version (build) > > + ret = pt516xx_read_block_data_main_micro_indirect( > > + data, > > + ARIES_MAIN_MICRO_FW_INFO + ARIES_MM_FW_VERSION_BUILD, 2, > > + (u8 *)&build); > > + if (ret) > > + return ret; > > + } else { > > + dev_warn(&data->client->dev, "No Main Micro Heartbeat\n"); > > This and other messages above would create a lot of noise if persistent > since the function is called repeatedly. > OK. I will reduce the debug messages or change to use dev_dbg(). > > + } > > + data->fw_ver.major = major; > > + data->fw_ver.minor = minor; > > + data->fw_ver.build = build; > > + > > + return 0; > > +} > > + > > +static int pt516xx_fw_is_at_least(struct pt516xx_data *data, u8 major, u8 minor, > > + u16 build) > > +{ > > + u32 ver = major << 24 | minor << 16 | build; > > + u32 curr_ver = data->fw_ver.major << 24 | data->fw_ver.minor << 16 | > > + data->fw_ver.build; > > + > > + if (curr_ver >= ver) > > + return true; > > + > > + return false; > > +} > > + > > +static int pt516xx_init_dev(struct pt516xx_data *data) > > +{ > > + int ret; > > + > > + mutex_lock(&data->lock); > > + ret = pt516xx_fwsts_check(data); > > + mutex_unlock(&data->lock); > > + if (ret) > > + return ret; > > + > > + dev_info(&data->client->dev, "fw_ver: %u.%u.%u\n", data->fw_ver.major, > > + data->fw_ver.minor, data->fw_ver.build); > > + > > Please no such noise > OK. > > + if (pt516xx_fw_is_at_least(data, 2, 2, 0)) > > + data->mm_wide_reg_valid = true; > > + > > + data->temp_cal_code_avg = ARIES_TEMP_CAL_CODE_DEFAULT; > > What is the point of this ? temp_cal_code_avg will never be anything else. > I will change to directly use ARIES_TEMP_CAL_CODE_DEFAULT when calculating. > > + data->init_done = true; > > + > > + return 0; > > +} > > + > > +static int pt516xx_read(struct device *dev, enum hwmon_sensor_types type, > > + u32 attr, int channel, long *val) > > +{ > > + struct pt516xx_data *data = dev_get_drvdata(dev); > > + int ret = 0; > > + long adc_code = 0; > > + > > This assumes a specific endianness which isn't given. I am quite sure that the > code as written won't work in a big endian system. Also, "long" may be 4 or 8 byte > depending on the CPU architecture. > Oops, I will revise it to be compatible with the big endian system. Using long is just to match the prototype of read(): "long *val". > > + if (!data->init_done) { > > + ret = pt516xx_init_dev(data); > > + if (ret) { > > + dev_err(dev, "pt516xx_init_dev failed %d\n", ret); > > + return ret; > > + } > > + } > > This is really the wrong place for this code. It should be checked and initialized > in the probe function, and probe should bail out if it fails. > > Also, the only reason to call pt516xx_init_dev() seems to be to determine > if "wide reegister access" is supported. If it isn't, pt516xx_read_block_data() > is used. Why not call pt516xx_read_block_data() below directly and not bother > with wide register access ? > Previously I followed CSDK using wide_reg to access new FW. Also tried old FW, but old FW only support read_block_data. After further testing with the new FW, it also does support read_block_data. So I will revise it to use read_block_data directly for temperature. Thanks. > > + > > + switch (attr) { > > + case hwmon_temp_input: > > + mutex_lock(&data->lock); > > + ret = pt516xx_read_wide_reg(data, > > + ARIES_CURRENT_AVG_TEMP_ADC_CSR, 4, > > + (u8 *)&adc_code); > > + mutex_unlock(&data->lock); > > + break; > > + default: > > + return -EOPNOTSUPP; > > + } > > + if (ret) { > > + dev_err(dev, "Read adc_code failed %d\n", ret); > > + return ret; > > + } The error return has been checked here. > > + if (adc_code == 0 || adc_code >= 0x3ff) { > > + dev_err(dev, "Invalid adc_code %lx\n", adc_code); > > + return -EINVAL; > OK. I will change to use -ENODATA here. > I guess this is supposed to cover an error return from pt516xx_read_wide_reg() > returned an error, assuming that adc_code was not overwritten in that case. > This is inappropriate. Error returns from pt516xx_read_wide_reg() should be > handled explicitly (and don't indicate "Invalid argument"). > > > + } > > + > > The above will create logging noise if the chip has a problem. Please make it dev_dbg. > OK. > > + *val = 110000 + ((adc_code - (data->temp_cal_code_avg + 250)) * -320); > > + > > + return 0; > > +} > > + > > +static umode_t pt516xx_is_visible(const void *data, > > + enum hwmon_sensor_types type, u32 attr, > > + int channel) > > +{ > > + switch (attr) { > > + case hwmon_temp_input: > > + return 0444; > > + default: > > + break; > > + } > > + > > + return 0; > > +} > > + > > +static const struct hwmon_channel_info *pt516xx_info[] = { > > + HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT), > > + NULL > > +}; > > + > > +static const struct hwmon_ops pt516xx_hwmon_ops = { > > + .is_visible = pt516xx_is_visible, > > + .read = pt516xx_read, > > +}; > > + > > +static const struct hwmon_chip_info pt516xx_chip_info = { > > + .ops = &pt516xx_hwmon_ops, > > + .info = pt516xx_info, > > +}; > > + > > +static ssize_t pt516xx_debugfs_read_fw_ver(struct file *file, char __user *buf, > > + size_t count, loff_t *ppos) > > +{ > > + struct pt516xx_data *data = file->private_data; > > + int ret; > > + char ver[32]; > > + > > + mutex_lock(&data->lock); > > + ret = pt516xx_fwsts_check(data); > > + mutex_unlock(&data->lock); > > What is the point of (re-)reading the firmware version repeatedly ? > The PCIe retimer uses normal power rail, that is, it's turned off when the system is powered-off. Consider that the PCIe retimer is used on a server system, and monitored by a BMC. (an independent management controller, with embedded Linux) The BMC is still working when the system is powered-off but remains standby power rail. When the retimer FW is updated and reload, it can read the correct FW version. > > + if (ret) > > + return ret; > > + > > + ret = snprintf(ver, sizeof(ver), "%u.%u.%u\n", data->fw_ver.major, > > + data->fw_ver.minor, data->fw_ver.build); > > + if (ret < 0) > > + return ret; > > + > > + return simple_read_from_buffer(buf, count, ppos, ver, ret + 1); > > +} > > + > > +static const struct file_operations pt516xx_debugfs_ops_fw_ver = { > > + .read = pt516xx_debugfs_read_fw_ver, > > + .open = simple_open, > > +}; > > + > > +static ssize_t pt516xx_debugfs_read_health(struct file *file, char __user *buf, > > + size_t count, loff_t *ppos) > > +{ > > + struct pt516xx_data *data = file->private_data; > > + int ret; > > + u8 status = 0; > > + char health[8]; > > + > > + mutex_lock(&data->lock); > > + ret = pt516xx_fwsts_check(data); > > + mutex_unlock(&data->lock); > > + if (ret == 0) { > > + status |= data->mm_heartbeat_okay ? 1 : 0; // bit0 > > + status |= data->code_load_okay ? 2 : 0; // bit1 > > Those should really be separate debugfs files. > OK. > > + } > > + > > + ret = snprintf(health, sizeof(health), "0x%02x\n", status); > > + if (ret < 0) > > + return ret; > > + > > + return simple_read_from_buffer(buf, count, ppos, health, ret + 1); > > +} > > + > > +static const struct file_operations pt516xx_debugfs_ops_health = { > > + .read = pt516xx_debugfs_read_health, > > + .open = simple_open, > > +}; > > + > > +static int pt516xx_init_debugfs(struct pt516xx_data *data) > > +{ > > + if (!pt516xx_debugfs_dir) > > + return -ENOENT; > > + > > debugfs functions handle this situation, and return values therefore do not > need to be checked. > Got it. I will remove it. > > + data->debugfs = debugfs_create_dir(dev_name(&data->client->dev), > > + pt516xx_debugfs_dir); > > + if (IS_ERR_OR_NULL(data->debugfs)) > > + return -ENOENT; > > + > Again, unnecessary. > Got it. > > + debugfs_create_file("fw_ver", 0444, data->debugfs, data, > > + &pt516xx_debugfs_ops_fw_ver); > > + > > + debugfs_create_file("health", 0444, data->debugfs, data, > > + &pt516xx_debugfs_ops_health); > > + > > + return 0; > > +} > > + > > +static int pt516xx_probe(struct i2c_client *client) > > +{ > > + struct device *dev = &client->dev; > > + struct device *hwmon_dev; > > + struct pt516xx_data *data; > > + > > + data = devm_kzalloc(dev, sizeof(struct pt516xx_data), GFP_KERNEL); > > + if (!data) > > + return -ENOMEM; > > + > > + data->client = client; > > + mutex_init(&data->lock); > > + pt516xx_init_dev(data); > > + > > + hwmon_dev = devm_hwmon_device_register_with_info( > > + dev, client->name, data, &pt516xx_chip_info, NULL); > > + > > + if (pt516xx_init_debugfs(data)) > > + dev_warn(dev, "Failed to register debugfs\n"); > > debugfs should fail silently. > > debugfs files are not removed if a single device is unloaded. Did you check > what happens in this case ? > Oops, I will add the remove callback function to remove the debugfs files. > > + > > + return PTR_ERR_OR_ZERO(hwmon_dev); > > +} > > + > > +static const struct i2c_device_id pt516xx_id[] = { > > + { "pt5161l", 0 }, > > + {} > > +}; > > +MODULE_DEVICE_TABLE(i2c, pt516xx_id); > > + > > +static const struct of_device_id __maybe_unused pt516xx_of_match[] = { > > + { .compatible = "asteralabs,pt5161l" }, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(of, pt516xx_of_match); > > + > > +static struct i2c_driver pt516xx_driver = { > > + .class = I2C_CLASS_HWMON, > > + .driver = { > > + .name = "pt516xx", > > + .of_match_table = of_match_ptr(pt516xx_of_match), > > This means the driver won't instantiate on an ACPI system if devicetree > support is disabled. Is this intentional ? > OK. I will add the acpi_match_table. > > + }, > > + .probe = pt516xx_probe, > > + .id_table = pt516xx_id, > > +}; > > + > > +module_i2c_driver(pt516xx_driver); > > + > > +static int __init pt516xx_core_init(void) > > +{ > > + pt516xx_debugfs_dir = debugfs_create_dir("pt516xx", NULL); > > + if (IS_ERR(pt516xx_debugfs_dir)) > > + pt516xx_debugfs_dir = NULL; > > + > > + return 0; > > +} > > + > > +static void __exit pt516xx_core_exit(void) > > +{ > > + debugfs_remove_recursive(pt516xx_debugfs_dir); > > +} > > + > > +module_init(pt516xx_core_init); > > +module_exit(pt516xx_core_exit); > > + > > +MODULE_AUTHOR("Cosmo Chou <cosmo.chou@quantatw.com>"); > > +MODULE_DESCRIPTION("Hwmon driver for Astera Labs Aries PCIe retimer"); > > +MODULE_LICENSE("GPL"); > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] hwmon: Add driver for Astera Labs PT516XX retimer 2023-12-05 7:47 ` [PATCH 3/3] hwmon: Add driver for Astera Labs PT516XX retimer Cosmo Chou 2023-12-05 15:55 ` Guenter Roeck @ 2023-12-05 23:24 ` kernel test robot 2023-12-11 17:43 ` Rob Herring 2 siblings, 0 replies; 12+ messages in thread From: kernel test robot @ 2023-12-05 23:24 UTC (permalink / raw) To: Cosmo Chou, jdelvare, linux, robh+dt, krzysztof.kozlowski+dt, conor+dt, corbet, heiko, jernej.skrabec, macromorgan, linus.walleij Cc: oe-kbuild-all, linux-hwmon, devicetree, linux-kernel, linux-doc, chou.cosmo, cosmo.chou Hi Cosmo, kernel test robot noticed the following build warnings: [auto build test WARNING on groeck-staging/hwmon-next] [also build test WARNING on robh/for-next linus/master v6.7-rc4 next-20231205] [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/Cosmo-Chou/dt-bindings-vendor-prefixes-add-asteralabs/20231205-155020 base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next patch link: https://lore.kernel.org/r/20231205074723.3546295-4-chou.cosmo%40gmail.com patch subject: [PATCH 3/3] hwmon: Add driver for Astera Labs PT516XX retimer reproduce: (https://download.01.org/0day-ci/archive/20231206/202312060716.Zh0AHxKs-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/202312060716.Zh0AHxKs-lkp@intel.com/ All warnings (new ones prefixed by >>): >> Documentation/hwmon/pt516xx.rst:4: WARNING: Title underline too short. vim +4 Documentation/hwmon/pt516xx.rst 2 3 Kernel driver pt516xx > 4 ==================== 5 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3] hwmon: Add driver for Astera Labs PT516XX retimer 2023-12-05 7:47 ` [PATCH 3/3] hwmon: Add driver for Astera Labs PT516XX retimer Cosmo Chou 2023-12-05 15:55 ` Guenter Roeck 2023-12-05 23:24 ` kernel test robot @ 2023-12-11 17:43 ` Rob Herring 2 siblings, 0 replies; 12+ messages in thread From: Rob Herring @ 2023-12-11 17:43 UTC (permalink / raw) To: Cosmo Chou Cc: jdelvare, linux, krzysztof.kozlowski+dt, conor+dt, corbet, heiko, jernej.skrabec, macromorgan, linus.walleij, linux-hwmon, devicetree, linux-kernel, linux-doc, cosmo.chou On Tue, Dec 5, 2023 at 1:49 AM Cosmo Chou <chou.cosmo@gmail.com> wrote: > > This driver implements support for temperature monitoring of Astera Labs > PT5161L series PCIe retimer chips. > > This driver implementation originates from the CSDK available at > Link: https://github.com/facebook/openbmc/tree/helium/common/recipes-lib/retimer-v2.14 > The communication protocol utilized is based on the I2C/SMBus standard. > > Signed-off-by: Cosmo Chou <chou.cosmo@gmail.com> > --- > Documentation/hwmon/index.rst | 1 + > Documentation/hwmon/pt516xx.rst | 48 +++ > MAINTAINERS | 8 + > drivers/hwmon/Kconfig | 10 + > drivers/hwmon/Makefile | 1 + > drivers/hwmon/pt516xx.c | 648 ++++++++++++++++++++++++++++++++ > 6 files changed, 716 insertions(+) > create mode 100644 Documentation/hwmon/pt516xx.rst > create mode 100644 drivers/hwmon/pt516xx.c > diff --git a/drivers/hwmon/pt516xx.c b/drivers/hwmon/pt516xx.c > new file mode 100644 > index 000000000000..824798559fe1 > --- /dev/null > +++ b/drivers/hwmon/pt516xx.c > @@ -0,0 +1,648 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > + > +#include <linux/debugfs.h> > +#include <linux/delay.h> > +#include <linux/err.h> > +#include <linux/i2c.h> > +#include <linux/init.h> > +#include <linux/hwmon.h> > +#include <linux/module.h> > +#include <linux/mutex.h> > +#include <linux/of_device.h> You probably don't need this header and the implicit includes it makes are dropped now in linux-next. Please check what you actually need and make them explicit. Rob ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3] hwmon: Add driver for Astera Labs PT516XX retimer 2023-12-05 7:47 [PATCH 0/3] hwmon: Add driver for Astera Labs PT516XX retimer Cosmo Chou ` (2 preceding siblings ...) 2023-12-05 7:47 ` [PATCH 3/3] hwmon: Add driver for Astera Labs PT516XX retimer Cosmo Chou @ 2023-12-05 14:45 ` Guenter Roeck 3 siblings, 0 replies; 12+ messages in thread From: Guenter Roeck @ 2023-12-05 14:45 UTC (permalink / raw) To: Cosmo Chou Cc: jdelvare, robh+dt, krzysztof.kozlowski+dt, conor+dt, corbet, heiko, jernej.skrabec, macromorgan, linus.walleij, linux-hwmon, devicetree, linux-kernel, linux-doc, cosmo.chou On Tue, Dec 05, 2023 at 03:47:20PM +0800, Cosmo Chou wrote: > This driver implements support for temperature monitoring of Astera Labs > PT5161L series PCIe retimer chips. > > Cosmo Chou (3): > dt-bindings: vendor-prefixes: add asteralabs > dt-bindings: hwmon: pt516xx: add bindings > hwmon: Add driver for Astera Labs PT516XX retimer Please refrain from using wildcards in file, variable, or function names. Guenter ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-12-11 17:43 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-12-05 7:47 [PATCH 0/3] hwmon: Add driver for Astera Labs PT516XX retimer Cosmo Chou 2023-12-05 7:47 ` [PATCH 1/3] dt-bindings: vendor-prefixes: add asteralabs Cosmo Chou 2023-12-05 16:45 ` Conor Dooley 2023-12-05 7:47 ` [PATCH 2/3] dt-bindings: hwmon: pt516xx: add bindings Cosmo Chou 2023-12-05 16:46 ` Conor Dooley 2023-12-06 8:02 ` Cosmo Chou 2023-12-05 7:47 ` [PATCH 3/3] hwmon: Add driver for Astera Labs PT516XX retimer Cosmo Chou 2023-12-05 15:55 ` Guenter Roeck 2023-12-09 8:02 ` Cosmo Chou 2023-12-05 23:24 ` kernel test robot 2023-12-11 17:43 ` Rob Herring 2023-12-05 14:45 ` [PATCH 0/3] " Guenter Roeck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).