Message-ID: <c5d3bc2d-8df3-6435-48d6-a87af1ce847e@roeck-us.net>
Date: Mon, 23 Oct 2023 06:35:37 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: "Knitter, Konrad" <konrad.knitter@...el.com>,
 "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 "jdelvare@...e.com" <jdelvare@...e.com>,
 Marcin Domagala <marcinx.domagala@...el.com>,
 "Joyner, Eric" <eric.joyner@...el.com>,
 Marcin Szycik <marcin.szycik@...ux.intel.com>,
 "Kitszel, Przemyslaw" <przemyslaw.kitszel@...el.com>
Subject: Re: [PATCH iwl-next v4] ice: read internal temperature sensor

On 10/23/23 04:37, Knitter, Konrad wrote:
>> -----Original Message-----
>> From: Guenter Roeck <groeck7@...il.com> On Behalf Of Guenter Roeck
>> Sent: Sunday, October 22, 2023 6:46 PM
>> To: Knitter, Konrad <konrad.knitter@...el.com>; intel-wired-
>> lan@...ts.osuosl.org
>> Cc: netdev@...r.kernel.org; jdelvare@...e.com; Marcin Domagala
>> <marcinx.domagala@...el.com>; Joyner, Eric <eric.joyner@...el.com>; Marcin
>> Szycik <marcin.szycik@...ux.intel.com>; Kitszel, Przemyslaw
>> <przemyslaw.kitszel@...el.com>
>> Subject: Re: [PATCH iwl-next v4] ice: read internal temperature sensor
>>
>> On 10/16/23 03:29, Konrad Knitter wrote:
>>> Since 4.30 firmware exposes internal thermal sensor reading via admin
>>> queue commands. Expose those readouts via hwmon API when supported.
>>>
>>> Driver provides current reading from HW as well as device specific
>>> thresholds for thermal alarm (Warning, Critical, Fatal) events.
>>>
>>> $ sensors
>>>
>>> Output
>>> =========================================================
>>> ice-pci-b100
>>> Adapter: PCI adapter
>>> temp1:        +62.0°C  (high = +95.0°C, crit = +105.0°C)
>>>                          (emerg = +115.0°C)
>>>
>>> Co-developed-by: Marcin Domagala <marcinx.domagala@...el.com>
>>> Signed-off-by: Marcin Domagala <marcinx.domagala@...el.com>
>>> Co-developed-by: Eric Joyner <eric.joyner@...el.com>
>>> Signed-off-by: Eric Joyner <eric.joyner@...el.com>
>>> Reviewed-by: Marcin Szycik <marcin.szycik@...ux.intel.com>
>>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@...el.com>
>>> Signed-off-by: Konrad Knitter <konrad.knitter@...el.com>
>>> ---
>>> v4: added dependency config_hwmon, cleanups
>>
>>
>> Have yo tried to compile this with CONFIG_ICE=y and CONFIG_HWMON=m ?
>>
>> Guenter
> 
> CONFIG_ICE can be set to M/n only.
> When I try CONFIG_ICE=y, it's forced to CONFIG_ICE=m.
>   

Is that so ?

$ make allyesconfig
$ grep CONFIG_ICE .config
CONFIG_ICE=y

Guenter

>>> v3: add SPDX identification to ice_hwmon files
>>> v2: fix formmating issues, added hwmon maintainers to Cc
>>> ---
>>>    drivers/net/ethernet/intel/ice/Makefile       |   1 +
>>>    drivers/net/ethernet/intel/ice/ice.h          |   1 +
>>>    .../net/ethernet/intel/ice/ice_adminq_cmd.h   |  28 ++++
>>>    drivers/net/ethernet/intel/ice/ice_common.c   |  56 +++++++-
>>>    drivers/net/ethernet/intel/ice/ice_common.h   |   2 +
>>>    drivers/net/ethernet/intel/ice/ice_hwmon.c    | 126
>> ++++++++++++++++++
>>>    drivers/net/ethernet/intel/ice/ice_hwmon.h    |  15 +++
>>>    drivers/net/ethernet/intel/ice/ice_main.c     |   5 +
>>>    drivers/net/ethernet/intel/ice/ice_type.h     |   4 +
>>>    9 files changed, 237 insertions(+), 1 deletion(-)
>>>    create mode 100644 drivers/net/ethernet/intel/ice/ice_hwmon.c
>>>    create mode 100644 drivers/net/ethernet/intel/ice/ice_hwmon.h
>>>
>>> diff --git a/drivers/net/ethernet/intel/ice/Makefile
>> b/drivers/net/ethernet/intel/ice/Makefile
>>> index 8757bec23fb3..c6072080e548 100644
>>> --- a/drivers/net/ethernet/intel/ice/Makefile
>>> +++ b/drivers/net/ethernet/intel/ice/Makefile
>>> @@ -51,3 +51,4 @@ ice-$(CONFIG_RFS_ACCEL) += ice_arfs.o
>>>    ice-$(CONFIG_XDP_SOCKETS) += ice_xsk.o
>>>    ice-$(CONFIG_ICE_SWITCHDEV) += ice_eswitch.o ice_eswitch_br.o
>>>    ice-$(CONFIG_GNSS) += ice_gnss.o
>>> +ice-$(CONFIG_HWMON) += ice_hwmon.o
>>> diff --git a/drivers/net/ethernet/intel/ice/ice.h
>> b/drivers/net/ethernet/intel/ice/ice.h
>>> index ad5614d4449c..61d26be502b2 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice.h
>>> +++ b/drivers/net/ethernet/intel/ice/ice.h
>>> @@ -650,6 +650,7 @@ struct ice_pf {
>>>    #define ICE_MAX_VF_AGG_NODES		32
>>>    	struct ice_agg_node vf_agg_node[ICE_MAX_VF_AGG_NODES];
>>>    	struct ice_dplls dplls;
>>> +	struct device *hwmon_dev;
>>>    };
>>>
>>>    extern struct workqueue_struct *ice_lag_wq;
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>> b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>>> index 1202abfb9eb3..de2e6508f330 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>>> +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
>>> @@ -117,6 +117,7 @@ struct ice_aqc_list_caps_elem {
>>>    #define ICE_AQC_CAPS_NET_VER				0x004C
>>>    #define ICE_AQC_CAPS_PENDING_NET_VER			0x004D
>>>    #define ICE_AQC_CAPS_RDMA				0x0051
>>> +#define ICE_AQC_CAPS_SENSOR_READING			0x0067
>>>    #define ICE_AQC_CAPS_PCIE_RESET_AVOIDANCE		0x0076
>>>    #define ICE_AQC_CAPS_POST_UPDATE_RESET_RESTRICT
>> 	0x0077
>>>    #define ICE_AQC_CAPS_NVM_MGMT
>> 	0x0080
>>> @@ -1393,6 +1394,30 @@ struct ice_aqc_get_phy_rec_clk_out {
>>>    	__le16 node_handle;
>>>    };
>>>
>>> +/* Get sensor reading (direct 0x0632) */
>>> +struct ice_aqc_get_sensor_reading {
>>> +	u8 sensor;
>>> +	u8 format;
>>> +	u8 reserved[6];
>>> +	__le32 addr_high;
>>> +	__le32 addr_low;
>>> +};
>>> +
>>> +/* Get sensor reading response (direct 0x0632) */
>>> +struct ice_aqc_get_sensor_reading_resp {
>>> +	union {
>>> +		u8 raw[8];
>>> +		/* Output data for sensor 0x00, format 0x00 */
>>> +		struct _packed {
>>> +			s8 temp;
>>> +			u8 temp_warning_threshold;
>>> +			u8 temp_critical_threshold;
>>> +			u8 temp_fatal_threshold;
>>> +			u8 reserved[4];
>>> +		} s0f0;
>>> +	} data;
>>> +};
>>> +
>>>    struct ice_aqc_link_topo_params {
>>>    	u8 lport_num;
>>>    	u8 lport_num_valid;
>>> @@ -2438,6 +2463,8 @@ struct ice_aq_desc {
>>>    		struct ice_aqc_restart_an restart_an;
>>>    		struct ice_aqc_set_phy_rec_clk_out set_phy_rec_clk_out;
>>>    		struct ice_aqc_get_phy_rec_clk_out get_phy_rec_clk_out;
>>> +		struct ice_aqc_get_sensor_reading get_sensor_reading;
>>> +		struct ice_aqc_get_sensor_reading_resp
>> get_sensor_reading_resp;
>>>    		struct ice_aqc_gpio read_write_gpio;
>>>    		struct ice_aqc_sff_eeprom read_write_sff_param;
>>>    		struct ice_aqc_set_port_id_led set_port_id_led;
>>> @@ -2617,6 +2644,7 @@ enum ice_adminq_opc {
>>>    	ice_aqc_opc_set_mac_lb				= 0x0620,
>>>    	ice_aqc_opc_set_phy_rec_clk_out			= 0x0630,
>>>    	ice_aqc_opc_get_phy_rec_clk_out			= 0x0631,
>>> +	ice_aqc_opc_get_sensor_reading			= 0x0632,
>>>    	ice_aqc_opc_get_link_topo			= 0x06E0,
>>>    	ice_aqc_opc_read_i2c				= 0x06E2,
>>>    	ice_aqc_opc_write_i2c				= 0x06E3,
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c
>> b/drivers/net/ethernet/intel/ice/ice_common.c
>>> index 283492314215..6564038e2665 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_common.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
>>> @@ -2462,6 +2462,26 @@ ice_parse_fdir_dev_caps(struct ice_hw *hw,
>> struct ice_hw_dev_caps *dev_p,
>>>    		  dev_p->num_flow_director_fltr);
>>>    }
>>>
>>> +/**
>>> + * ice_parse_sensor_reading_cap - Parse ICE_AQC_CAPS_SENSOR_READING
>> cap
>>> + * @hw: pointer to the HW struct
>>> + * @dev_p: pointer to device capabilities structure
>>> + * @cap: capability element to parse
>>> + *
>>> + * Parse ICE_AQC_CAPS_SENSOR_READING for device capability for reading
>>> + * enabled sensors.
>>> + */
>>> +static void
>>> +ice_parse_sensor_reading_cap(struct ice_hw *hw, struct ice_hw_dev_caps
>> *dev_p,
>>> +			     struct ice_aqc_list_caps_elem *cap)
>>> +{
>>> +	dev_p->supported_sensors = le32_to_cpu(cap->number);
>>> +
>>> +	ice_debug(hw, ICE_DBG_INIT,
>>> +		  "dev caps: supported sensors (bitmap) = 0x%x\n",
>>> +		  dev_p->supported_sensors);
>>> +}
>>> +
>>>    /**
>>>     * ice_parse_dev_caps - Parse device capabilities
>>>     * @hw: pointer to the HW struct
>>> @@ -2507,9 +2527,12 @@ ice_parse_dev_caps(struct ice_hw *hw, struct
>> ice_hw_dev_caps *dev_p,
>>>    		case ICE_AQC_CAPS_1588:
>>>    			ice_parse_1588_dev_caps(hw, dev_p, &cap_resp[i]);
>>>    			break;
>>> -		case  ICE_AQC_CAPS_FD:
>>> +		case ICE_AQC_CAPS_FD:
>>>    			ice_parse_fdir_dev_caps(hw, dev_p, &cap_resp[i]);
>>>    			break;
>>> +		case ICE_AQC_CAPS_SENSOR_READING:
>>> +			ice_parse_sensor_reading_cap(hw, dev_p,
>> &cap_resp[i]);
>>> +			break;
>>>    		default:
>>>    			/* Don't list common capabilities as unknown */
>>>    			if (!found)
>>> @@ -5292,6 +5315,37 @@ ice_aq_get_phy_rec_clk_out(struct ice_hw *hw,
>> u8 *phy_output, u8 *port_num,
>>>    	return status;
>>>    }
>>>
>>> +/**
>>> + * ice_aq_get_sensor_reading
>>> + * @hw: pointer to the HW struct
>>> + * @sensor: sensor type
>>> + * @format: requested response format
>>> + * @data: pointer to data to be read from the sensor
>>> + *
>>> + * Get sensor reading (0x0632)
>>> + */
>>> +int ice_aq_get_sensor_reading(struct ice_hw *hw,
>>> +			      struct ice_aqc_get_sensor_reading_resp *data)
>>> +{
>>> +	struct ice_aqc_get_sensor_reading *cmd;
>>> +	struct ice_aq_desc desc;
>>> +	int status;
>>> +
>>> +	ice_fill_dflt_direct_cmd_desc(&desc,
>> ice_aqc_opc_get_sensor_reading);
>>> +	cmd = &desc.params.get_sensor_reading;
>>> +#define ICE_INTERNAL_TEMP_SENSOR_FORMAT	0
>>> +#define ICE_INTERNAL_TEMP_SENSOR	0
>>> +	cmd->sensor = ICE_INTERNAL_TEMP_SENSOR;
>>> +	cmd->format = ICE_INTERNAL_TEMP_SENSOR_FORMAT;
>>> +
>>> +	status = ice_aq_send_cmd(hw, &desc, NULL, 0, NULL);
>>> +	if (!status)
>>> +		memcpy(data, &desc.params.get_sensor_reading_resp,
>>> +		       sizeof(*data));
>>> +
>>> +	return status;
>>> +}
>>> +
>>>    /**
>>>     * ice_replay_pre_init - replay pre initialization
>>>     * @hw: pointer to the HW struct
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_common.h
>> b/drivers/net/ethernet/intel/ice/ice_common.h
>>> index 4a75c0c89301..9696ed59d1a8 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_common.h
>>> +++ b/drivers/net/ethernet/intel/ice/ice_common.h
>>> @@ -240,6 +240,8 @@ ice_aq_set_phy_rec_clk_out(struct ice_hw *hw, u8
>> phy_output, bool enable,
>>>    int
>>>    ice_aq_get_phy_rec_clk_out(struct ice_hw *hw, u8 *phy_output, u8
>> *port_num,
>>>    			   u8 *flags, u16 *node_handle);
>>> +int ice_aq_get_sensor_reading(struct ice_hw *hw,
>>> +			      struct ice_aqc_get_sensor_reading_resp *data);
>>>    void
>>>    ice_stat_update40(struct ice_hw *hw, u32 reg, bool prev_stat_loaded,
>>>    		  u64 *prev_stat, u64 *cur_stat);
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_hwmon.c
>> b/drivers/net/ethernet/intel/ice/ice_hwmon.c
>>> new file mode 100644
>>> index 000000000000..14b4070663db
>>> --- /dev/null
>>> +++ b/drivers/net/ethernet/intel/ice/ice_hwmon.c
>>> @@ -0,0 +1,126 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/* Copyright (C) 2023, Intel Corporation. */
>>> +
>>> +#include "ice.h"
>>> +#include "ice_hwmon.h"
>>> +#include "ice_adminq_cmd.h"
>>> +
>>> +#include <linux/hwmon.h>
>>> +
>>> +#define TEMP_FROM_REG(reg) ((reg) * 1000)
>>> +
>>> +static const struct hwmon_channel_info *ice_hwmon_info[] = {
>>> +	HWMON_CHANNEL_INFO(temp,
>>> +			   HWMON_T_INPUT | HWMON_T_MAX |
>>> +			   HWMON_T_CRIT | HWMON_T_EMERGENCY),
>>> +	NULL
>>> +};
>>> +
>>> +static int ice_hwmon_read(struct device *dev, enum hwmon_sensor_types
>> type,
>>> +			  u32 attr, int channel, long *val)
>>> +{
>>> +	struct ice_aqc_get_sensor_reading_resp resp;
>>> +	struct ice_pf *pf = dev_get_drvdata(dev);
>>> +	int ret;
>>> +
>>> +	if (type != hwmon_temp)
>>> +		return -EOPNOTSUPP;
>>> +
>>> +	ret = ice_aq_get_sensor_reading(&pf->hw, &resp);
>>> +	if (ret) {
>>> +		dev_warn_ratelimited(dev,
>>> +				     "%s HW read failure (%d)\n",
>>> +				     __func__,
>>> +				     ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	switch (attr) {
>>> +	case hwmon_temp_input:
>>> +		*val = TEMP_FROM_REG(resp.data.s0f0.temp);
>>> +		break;
>>> +	case hwmon_temp_max:
>>> +		*val =
>> TEMP_FROM_REG(resp.data.s0f0.temp_warning_threshold);
>>> +		break;
>>> +	case hwmon_temp_crit:
>>> +		*val =
>> TEMP_FROM_REG(resp.data.s0f0.temp_critical_threshold);
>>> +		break;
>>> +	case hwmon_temp_emergency:
>>> +		*val =
>> TEMP_FROM_REG(resp.data.s0f0.temp_fatal_threshold);
>>> +		break;
>>> +	default:
>>> +		dev_dbg(dev, "%s unsupported attribute (%d)\n",
>>> +			__func__, attr);
>>> +		return -EOPNOTSUPP;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static umode_t ice_hwmon_is_visible(const void *data,
>>> +				    enum hwmon_sensor_types type, u32 attr,
>>> +				    int channel)
>>> +{
>>> +	if (type != hwmon_temp)
>>> +		return 0;
>>> +
>>> +	switch (attr) {
>>> +	case hwmon_temp_input:
>>> +	case hwmon_temp_crit:
>>> +	case hwmon_temp_max:
>>> +	case hwmon_temp_emergency:
>>> +		return 0444;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct hwmon_ops ice_hwmon_ops = {
>>> +	.is_visible = ice_hwmon_is_visible,
>>> +	.read = ice_hwmon_read
>>> +};
>>> +
>>> +static const struct hwmon_chip_info ice_chip_info = {
>>> +	.ops = &ice_hwmon_ops,
>>> +	.info = ice_hwmon_info
>>> +};
>>> +
>>> +static bool ice_is_internal_reading_supported(struct ice_pf *pf)
>>> +{
>>> +	/* Only the first PF will report temperature for a chip.
>>> +	 * Note that internal temp reading are not supported
>>> +	 * for older FW (< v4.30).
>>> +	 */
>>> +	if (pf->hw.pf_id)
>>> +		return false;
>>> +
>>> +	unsigned long sensors = pf->hw.dev_caps.supported_sensors;
>>> +
>>> +	return _test_bit(ICE_SENSOR_SUPPORT_E810_INT_TEMP_BIT,
>> &sensors);
>>> +};
>>> +
>>> +void ice_hwmon_init(struct ice_pf *pf)
>>> +{
>>> +	struct device *dev = ice_pf_to_dev(pf);
>>> +	struct device *hdev;
>>> +
>>> +	if (!ice_is_internal_reading_supported(pf))
>>> +		return;
>>> +
>>> +	hdev = hwmon_device_register_with_info(dev, "ice", pf,
>> &ice_chip_info,
>>> +					       NULL);
>>> +	if (IS_ERR(hdev)) {
>>> +		dev_warn(dev,
>>> +			 "hwmon_device_register_with_info returns error
>> (%ld)",
>>> +			 PTR_ERR(hdev));
>>> +		return;
>>> +	}
>>> +	pf->hwmon_dev = hdev;
>>> +}
>>> +
>>> +void ice_hwmon_exit(struct ice_pf *pf)
>>> +{
>>> +	if (!pf->hwmon_dev)
>>> +		return;
>>> +	hwmon_device_unregister(pf->hwmon_dev);
>>> +}
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_hwmon.h
>> b/drivers/net/ethernet/intel/ice/ice_hwmon.h
>>> new file mode 100644
>>> index 000000000000..d7ad54e12136
>>> --- /dev/null
>>> +++ b/drivers/net/ethernet/intel/ice/ice_hwmon.h
>>> @@ -0,0 +1,15 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/* Copyright (C) 2023, Intel Corporation. */
>>> +
>>> +#ifndef _ICE_HWMON_H_
>>> +#define _ICE_HWMON_H_
>>> +
>>> +#if IS_ENABLED(CONFIG_HWMON)
>>> +void ice_hwmon_init(struct ice_pf *pf);
>>> +void ice_hwmon_exit(struct ice_pf *pf);
>>> +#else
>>> +static void ice_hwmon_init(struct ice_pf *pf) { }
>>> +static void ice_hwmon_exit(struct ice_pf *pf) { }
>>> +#endif
>>> +
>>> +#endif /* _ICE_HWMON_H_ */
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
>> b/drivers/net/ethernet/intel/ice/ice_main.c
>>> index a58da0024fe5..071a7da4b3b4 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>>> @@ -14,6 +14,7 @@
>>>    #include "ice_dcb_lib.h"
>>>    #include "ice_dcb_nl.h"
>>>    #include "ice_devlink.h"
>>> +#include "ice_hwmon.h"
>>>    /* Including ice_trace.h with CREATE_TRACE_POINTS defined will generate
>> the
>>>     * ice tracepoint functions. This must be done exactly once across the
>>>     * ice driver.
>>> @@ -4785,6 +4786,8 @@ static void ice_init_features(struct ice_pf *pf)
>>>
>>>    	if (ice_init_lag(pf))
>>>    		dev_warn(dev, "Failed to init link aggregation support\n");
>>> +
>>> +	ice_hwmon_init(pf);
>>>    }
>>>
>>>    static void ice_deinit_features(struct ice_pf *pf)
>>> @@ -5310,6 +5313,8 @@ static void ice_remove(struct pci_dev *pdev)
>>>    		ice_free_vfs(pf);
>>>    	}
>>>
>>> +	ice_hwmon_exit(pf);
>>> +
>>>    	ice_service_task_stop(pf);
>>>    	ice_aq_cancel_waiting_tasks(pf);
>>>    	set_bit(ICE_DOWN, pf->state);
>>> diff --git a/drivers/net/ethernet/intel/ice/ice_type.h
>> b/drivers/net/ethernet/intel/ice/ice_type.h
>>> index 877a92099ef0..0b5425d33adf 100644
>>> --- a/drivers/net/ethernet/intel/ice/ice_type.h
>>> +++ b/drivers/net/ethernet/intel/ice/ice_type.h
>>> @@ -378,6 +378,8 @@ struct ice_hw_func_caps {
>>>    	struct ice_ts_func_info ts_func_info;
>>>    };
>>>
>>> +#define ICE_SENSOR_SUPPORT_E810_INT_TEMP_BIT	0
>>> +
>>>    /* Device wide capabilities */
>>>    struct ice_hw_dev_caps {
>>>    	struct ice_hw_common_caps common_cap;
>>> @@ -386,6 +388,8 @@ struct ice_hw_dev_caps {
>>>    	u32 num_flow_director_fltr;	/* Number of FD filters available */
>>>    	struct ice_ts_dev_info ts_dev_info;
>>>    	u32 num_funcs;
>>> +	/* bitmap of supported sensors */
>>> +	u32 supported_sensors;
>>>    };
>>>
>>>    /* MAC info */
>>>
>>> base-commit: ac4dec3fd63c7da703c244698fc92efb411ff0d4
> 

Powered by blists - more mailing lists