Re: [PATCH v2 2/6] thermal/drivers/mediatek/lvts_thermal: Honor sensors in immediate mode

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 04/05/2023 02:48, Nícolas F. R. A. Prado wrote:
> Each controller can be configured to operate on immediate or filtered
> mode. On filtered mode, the sensors are enabled by setting the
> corresponding bits in MONCTL0, while on immediate mode, by setting
> MSRCTL1.
> 
> Previously, the code would set MSRCTL1 for all four sensors when
> configured to immediate mode, but given that the controller might not
> have all four sensors connected, this would cause interrupts to trigger
> for non-existent sensors. Fix this by handling the MSRCTL1 register
> analogously to the MONCTL0: only enable the sensors that were declared.
> 
> Fixes: f5f633b18234 ("thermal/drivers/mediatek: Add the Low Voltage Thermal Sensor driver")
> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Tested-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> 
> (no changes since v1)
> 
>   drivers/thermal/mediatek/lvts_thermal.c | 50 +++++++++++++------------
>   1 file changed, 26 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c
> index 2988f201633a..f7d998a45ea0 100644
> --- a/drivers/thermal/mediatek/lvts_thermal.c
> +++ b/drivers/thermal/mediatek/lvts_thermal.c
> @@ -922,24 +922,6 @@ static int lvts_ctrl_configure(struct device *dev, struct lvts_ctrl *lvts_ctrl)
>   			LVTS_HW_FILTER << 3 | LVTS_HW_FILTER;
>   	writel(value, LVTS_MSRCTL0(lvts_ctrl->base));
>   
> -	/*
> -	 * LVTS_MSRCTL1 : Measurement control
> -	 *
> -	 * Bits:
> -	 *
> -	 * 9: Ignore MSRCTL0 config and do immediate measurement on sensor3
> -	 * 6: Ignore MSRCTL0 config and do immediate measurement on sensor2
> -	 * 5: Ignore MSRCTL0 config and do immediate measurement on sensor1
> -	 * 4: Ignore MSRCTL0 config and do immediate measurement on sensor0
> -	 *
> -	 * That configuration will ignore the filtering and the delays
> -	 * introduced below in MONCTL1 and MONCTL2
> -	 */
> -	if (lvts_ctrl->mode == LVTS_MSR_IMMEDIATE_MODE) {
> -		value = BIT(9) | BIT(6) | BIT(5) | BIT(4);
> -		writel(value, LVTS_MSRCTL1(lvts_ctrl->base));
> -	}
> -
>   	/*
>   	 * LVTS_MONCTL1 : Period unit and group interval configuration
>   	 *
> @@ -1004,6 +986,8 @@ static int lvts_ctrl_start(struct device *dev, struct lvts_ctrl *lvts_ctrl)
>   	struct lvts_sensor *lvts_sensors = lvts_ctrl->sensors;
>   	struct thermal_zone_device *tz;
>   	u32 sensor_map = 0;
> +	u32 sensor_map_bits[][4] = {{BIT(4), BIT(5), BIT(6), BIT(9)},
> +				    {BIT(0), BIT(1), BIT(2), BIT(3)}};

Even correct, this initialization sounds prone to error and a bit 
obfuscating (eg. it assumes LVTS_MSR_IMMEDIATE_MODE is 1).

What about?

	/*
	 * A description
	 */
	u32 sensor_imm_bitmap[] = { BIT(0), BIT(1), BIT(2), BIT(3) };
	u32 sensor_filt_bitmap[] = { BIT(4), BIT(5), BIT(6), BIT(9) };

	u32 *sensor_bitmap = lvts_ctrl->mode ? LVTS_MSR_IMMEDIATE_MODE : 
sensor_imm_bitmap : sensor_filt_bitmap;

>   	int i;
>   
>   	for (i = 0; i < lvts_ctrl->num_lvts_sensor; i++) {
> @@ -1040,20 +1024,38 @@ static int lvts_ctrl_start(struct device *dev, struct lvts_ctrl *lvts_ctrl)
>   		 * map, so we can enable the temperature monitoring in
>   		 * the hardware thermal controller.
>   		 */
> -		sensor_map |= BIT(i);
> +		sensor_map |= sensor_map_bits[lvts_ctrl->mode][i];

		sensor_map |= sensor_bitmap[i];
>   	}
>   
>   	/*
> -	 * Bits:
> -	 *      9: Single point access flow
> -	 *    0-3: Enable sensing point 0-3
> -	 *
>   	 * The initialization of the thermal zones give us
>   	 * which sensor point to enable. If any thermal zone
>   	 * was not described in the device tree, it won't be
>   	 * enabled here in the sensor map.
>   	 */
> -	writel(sensor_map | BIT(9), LVTS_MONCTL0(lvts_ctrl->base));
> +	if (lvts_ctrl->mode == LVTS_MSR_IMMEDIATE_MODE) {
> +		/*
> +		 * LVTS_MSRCTL1 : Measurement control
> +		 *
> +		 * Bits:
> +		 *
> +		 * 9: Ignore MSRCTL0 config and do immediate measurement on sensor3
> +		 * 6: Ignore MSRCTL0 config and do immediate measurement on sensor2
> +		 * 5: Ignore MSRCTL0 config and do immediate measurement on sensor1
> +		 * 4: Ignore MSRCTL0 config and do immediate measurement on sensor0
> +		 *
> +		 * That configuration will ignore the filtering and the delays
> +		 * introduced in MONCTL1 and MONCTL2
> +		 */
> +		writel(sensor_map, LVTS_MSRCTL1(lvts_ctrl->base));
> +	} else {
> +		/*
> +		 * Bits:
> +		 *      9: Single point access flow
> +		 *    0-3: Enable sensing point 0-3
> +		 */
> +		writel(sensor_map | BIT(9), LVTS_MONCTL0(lvts_ctrl->base));
> +	}
>   
>   	return 0;
>   }

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog