Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b0576c2c-9249-1759-642f-b0accabfc773@linaro.org>
Date:   Fri, 2 Jun 2023 10:48:31 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH v2 2/6] thermal/drivers/mediatek/lvts_thermal: Honor
 sensors in immediate mode
Content-Language: en-US
To:     =?UTF-8?B?TsOtY29sYXMgRi4gUi4gQS4gUHJhZG8=?= 
        <nfraprado@collabora.com>
Cc:     kernel@collabora.com, Alexandre Mergnat <amergnat@baylibre.com>,
        Balsam CHIHI <bchihi@baylibre.com>,
        Chen-Yu Tsai <wenst@chromium.org>,
        Alexandre Bailon <abailon@baylibre.com>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        Amit Kucheria <amitk@kernel.org>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Zhang Rui <rui.zhang@intel.com>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-mediatek@lists.infradead.org, linux-pm@vger.kernel.org
References: <20230504004852.627049-1-nfraprado@collabora.com>
 <20230504004852.627049-3-nfraprado@collabora.com>
From:   Daniel Lezcano <daniel.lezcano@linaro.org>
In-Reply-To: <20230504004852.627049-3-nfraprado@collabora.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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