Received: by 2002:a05:6a10:5bc5:0:0:0:0 with SMTP id os5csp839540pxb; Thu, 21 Oct 2021 10:25:28 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxnc44SXKlQcH3G6no/hH6y1lciKdeOzUpcY19T0vnkkh+c0sk1QcXxKLqFbiNQ+lanuGEP X-Received: by 2002:a05:6402:84d:: with SMTP id b13mr9748330edz.6.1634837128703; Thu, 21 Oct 2021 10:25:28 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1634837128; cv=none; d=google.com; s=arc-20160816; b=FPdg7i1rum/IgZMCTLsav/qcUrqsw0WlnTQJFHQNl/jrCDeBw9S1dMvZUMyhe+gjAe bcO4mtuhuPWwCHaqjZXAbE+MYu8WG2v8QuTanTDjkDuRg/qjxE4CnpJhG3VkcF4xHFtI vMQBn2vFaJc4Dtp3m/SyXYh7WfPyN2LlaOV8oCxsIDprHEhNW5GPc8EsC1CNM2RPDHbQ tqtxa/Wbu9snI/RkBePgsWzjKiF4tubjvBj4q98BJb+8HmuLAMu8yj6S0wq1X9V7SqI5 IPmfsNdGlRzpZomeGLAcdgxYd/J6hxgpB4Mk4H+/OY82vT8pyHRmOSI18s1JmEw45KDV Rdwg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:user-agent:in-reply-to:content-disposition :mime-version:references:message-id:subject:cc:to:from:date; bh=gMIjx0El3DnT+tWgAMz02lhkgUjBgeUOZr6SE3sAm7k=; b=LNcgGnPvtJ9WmTNRdfne/LPdTzCF0Tc95XhesBisVKwQ4ocljXlQK2i8oGIPKeLHEm DyqGvGapPM9cv/ywiQP58XUkhoAwO7Wry0gH3k4qBwhftlWDp/0t8JRlz7eWNcmd6Pob veLQQm6OFLGOsLSmZ0yJxr63b/euJYXrebYKktXGYmldeCMKiN8nSpAhmgbsjbVhjW5P sNFwngeGMshOKkNPYVKn3p2/vS2mG/RI7X8y5ThT1/A3sN+UcSECNotfLhdlfRslYMbC 5LEdVzXI5+1hvjDz7AIhL511qkE3je14AkG504T35tL1Yd+GIy6vU05DGVKZRUraaOq3 53Wg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id zl20si7664151ejb.417.2021.10.21.10.25.01; Thu, 21 Oct 2021 10:25:28 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231579AbhJURXR (ORCPT + 99 others); Thu, 21 Oct 2021 13:23:17 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52136 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229567AbhJURXP (ORCPT ); Thu, 21 Oct 2021 13:23:15 -0400 Received: from metis.ext.pengutronix.de (metis.ext.pengutronix.de [IPv6:2001:67c:670:201:290:27ff:fe1d:cc33]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2794FC061764 for ; Thu, 21 Oct 2021 10:20:59 -0700 (PDT) Received: from ptx.hi.pengutronix.de ([2001:67c:670:100:1d::c0]) by metis.ext.pengutronix.de with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1mdbkJ-0004MS-WF; Thu, 21 Oct 2021 19:20:52 +0200 Received: from ore by ptx.hi.pengutronix.de with local (Exim 4.92) (envelope-from ) id 1mdbkG-0003Xy-Ud; Thu, 21 Oct 2021 19:20:48 +0200 Date: Thu, 21 Oct 2021 19:20:48 +0200 From: Oleksij Rempel To: Petr Benes Cc: Daniel Lezcano , Michal =?utf-8?B?Vm9rw6HEjQ==?= , linux-pm@vger.kernel.org, Shawn Guo , Sascha Hauer , Amit Kucheria , linux-kernel@vger.kernel.org, Andrzej Pietrasiewicz , NXP Linux Team , Pengutronix Kernel Team , David Jander , Zhang Rui , Fabio Estevam , linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2] thermal: imx: implement runtime PM support Message-ID: <20211021172048.GE2298@pengutronix.de> References: <20211019130809.21281-1-o.rempel@pengutronix.de> <20211020050459.GE16320@pengutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: X-Sent-From: Pengutronix Hildesheim X-URL: http://www.pengutronix.de/ X-IRC: #ptxdist @freenode X-Accept-Language: de,en X-Accept-Content-Type: text/plain X-Uptime: 19:12:24 up 245 days, 20:36, 111 users, load average: 0.18, 0.25, 0.25 User-Agent: Mutt/1.10.1 (2018-07-13) X-SA-Exim-Connect-IP: 2001:67c:670:100:1d::c0 X-SA-Exim-Mail-From: ore@pengutronix.de X-SA-Exim-Scanned: No (on metis.ext.pengutronix.de); SAEximRunCond expanded to false X-PTX-Original-Recipient: linux-kernel@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Petr, On Wed, Oct 20, 2021 at 05:53:03PM +0200, Petr Benes wrote: > On Wed, 20 Oct 2021 at 07:05, Oleksij Rempel wrote: > > > > Hi Petr and Michal, > > > > I forgot to add you for v2 in CC. Please test/review this version. > > Hi Oleksij, > > It works good. with PM as well as without PM. The only minor issue I found is, > that the first temperature reading (when the driver probes) fails. That is > (val & soc_data->temp_valid_mask) == 0) holds true. How does > pm_runtime_resume_and_get() behave in imx_thermal_probe()? > Does it go through imx_thermal_runtime_resume() with usleep_range()? How exactly did you reproduce it? Even more or less understanding how this can potentially happen, i never had this issue on my HW. Is it something HW specific? Even after executing this command: echo disabled > /sys/class/thermal/thermal_zone0/mode cat /sys/class/thermal/thermal_zone0/temp In this case, IRQ is disabled and on each manual temp read, driver starts actively using runtime PM. I still never get (val & soc_data->temp_valid_mask) == 0).. > > > > On Tue, Oct 19, 2021 at 03:08:09PM +0200, Oleksij Rempel wrote: > > > Starting with commit d92ed2c9d3ff ("thermal: imx: Use driver's local > > > data to decide whether to run a measurement") this driver stared using > > > irq_enabled flag to make decision to power on/off the thermal core. This > > > triggered a regression, where after reaching critical temperature, alarm > > > IRQ handler set irq_enabled to false, disabled thermal core and was not > > > able read temperature and disable cooling sequence. > > > > > > In case the cooling device is "CPU/GPU freq", the system will run with > > > reduce performance until next reboot. > > > > > > To solve this issue, we need to move all parts implementing hand made > > > runtime power management and let it handle actual runtime PM framework. > > > > > > Fixes: d92ed2c9d3ff ("thermal: imx: Use driver's local data to decide whether to run a measurement") > > > Signed-off-by: Oleksij Rempel > > > --- > > > drivers/thermal/imx_thermal.c | 143 +++++++++++++++++++++------------- > > > 1 file changed, 89 insertions(+), 54 deletions(-) > > > > > > diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c > > > index 2c7473d86a59..cb5a4354fc75 100644 > > > --- a/drivers/thermal/imx_thermal.c > > > +++ b/drivers/thermal/imx_thermal.c > > > @@ -15,6 +15,7 @@ > > > #include > > > #include > > > #include > > > +#include > > > > > > #define REG_SET 0x4 > > > #define REG_CLR 0x8 > > > @@ -194,6 +195,7 @@ static struct thermal_soc_data thermal_imx7d_data = { > > > }; > > > > > > struct imx_thermal_data { > > > + struct device *dev; > > > struct cpufreq_policy *policy; > > > struct thermal_zone_device *tz; > > > struct thermal_cooling_device *cdev; > > > @@ -252,44 +254,15 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp) > > > const struct thermal_soc_data *soc_data = data->socdata; > > > struct regmap *map = data->tempmon; > > > unsigned int n_meas; > > > - bool wait, run_measurement; > > > u32 val; > > > + int ret; > > > > > > - run_measurement = !data->irq_enabled; > > > - if (!run_measurement) { > > > - /* Check if a measurement is currently in progress */ > > > - regmap_read(map, soc_data->temp_data, &val); > > > - wait = !(val & soc_data->temp_valid_mask); > > > - } else { > > > - /* > > > - * Every time we measure the temperature, we will power on the > > > - * temperature sensor, enable measurements, take a reading, > > > - * disable measurements, power off the temperature sensor. > > > - */ > > > - regmap_write(map, soc_data->sensor_ctrl + REG_CLR, > > > - soc_data->power_down_mask); > > > - regmap_write(map, soc_data->sensor_ctrl + REG_SET, > > > - soc_data->measure_temp_mask); > > > - > > > - wait = true; > > > - } > > > - > > > - /* > > > - * According to the temp sensor designers, it may require up to ~17us > > > - * to complete a measurement. > > > - */ > > > - if (wait) > > > - usleep_range(20, 50); > > > + ret = pm_runtime_resume_and_get(data->dev); > > > + if (ret < 0) > > > + return ret; > > > > > > regmap_read(map, soc_data->temp_data, &val); > > > > > > - if (run_measurement) { > > > - regmap_write(map, soc_data->sensor_ctrl + REG_CLR, > > > - soc_data->measure_temp_mask); > > > - regmap_write(map, soc_data->sensor_ctrl + REG_SET, > > > - soc_data->power_down_mask); > > > - } > > > - > > > if ((val & soc_data->temp_valid_mask) == 0) { > > > dev_dbg(&tz->device, "temp measurement never finished\n"); > > > return -EAGAIN; > > > @@ -328,6 +301,8 @@ static int imx_get_temp(struct thermal_zone_device *tz, int *temp) > > > enable_irq(data->irq); > > > } > > > > > > + pm_runtime_put(data->dev); > > > + > > > return 0; > > > } > > > > > > @@ -335,24 +310,16 @@ static int imx_change_mode(struct thermal_zone_device *tz, > > > enum thermal_device_mode mode) > > > { > > > struct imx_thermal_data *data = tz->devdata; > > > - struct regmap *map = data->tempmon; > > > - const struct thermal_soc_data *soc_data = data->socdata; > > > > > > if (mode == THERMAL_DEVICE_ENABLED) { > > > - regmap_write(map, soc_data->sensor_ctrl + REG_CLR, > > > - soc_data->power_down_mask); > > > - regmap_write(map, soc_data->sensor_ctrl + REG_SET, > > > - soc_data->measure_temp_mask); > > > + pm_runtime_get(data->dev); > > > > > > if (!data->irq_enabled) { > > > data->irq_enabled = true; > > > enable_irq(data->irq); > > > } > > > } else { > > > - regmap_write(map, soc_data->sensor_ctrl + REG_CLR, > > > - soc_data->measure_temp_mask); > > > - regmap_write(map, soc_data->sensor_ctrl + REG_SET, > > > - soc_data->power_down_mask); > > > + pm_runtime_put(data->dev); > > > > > > if (data->irq_enabled) { > > > disable_irq(data->irq); > > > @@ -393,6 +360,11 @@ static int imx_set_trip_temp(struct thermal_zone_device *tz, int trip, > > > int temp) > > > { > > > struct imx_thermal_data *data = tz->devdata; > > > + int ret; > > > + > > > + ret = pm_runtime_resume_and_get(data->dev); > > > + if (ret < 0) > > > + return ret; > > > > > > /* do not allow changing critical threshold */ > > > if (trip == IMX_TRIP_CRITICAL) > > > @@ -406,6 +378,8 @@ static int imx_set_trip_temp(struct thermal_zone_device *tz, int trip, > > > > > > imx_set_alarm_temp(data, temp); > > > > > > + pm_runtime_put(data->dev); > > > + > > > return 0; > > > } > > > > > > @@ -681,6 +655,8 @@ static int imx_thermal_probe(struct platform_device *pdev) > > > if (!data) > > > return -ENOMEM; > > > > > > + data->dev = &pdev->dev; > > > + > > > map = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, "fsl,tempmon"); > > > if (IS_ERR(map)) { > > > ret = PTR_ERR(map); > > > @@ -801,6 +777,14 @@ static int imx_thermal_probe(struct platform_device *pdev) > > > regmap_write(map, data->socdata->sensor_ctrl + REG_SET, > > > data->socdata->measure_temp_mask); > > > > > > + /* the core was configured and enabled just before */ > > > + pm_runtime_set_active(&pdev->dev); > > > + pm_runtime_enable(data->dev); > > > + > > > + ret = pm_runtime_resume_and_get(data->dev); > > > + if (ret < 0) > > > + goto disable_runtime_pm; > > > + > > > data->irq_enabled = true; > > > ret = thermal_zone_device_enable(data->tz); > > > if (ret) > > > @@ -814,10 +798,15 @@ static int imx_thermal_probe(struct platform_device *pdev) > > > goto thermal_zone_unregister; > > > } > > > > > > + pm_runtime_put(data->dev); > > > + > > > return 0; > > > > > > thermal_zone_unregister: > > > thermal_zone_device_unregister(data->tz); > > > +disable_runtime_pm: > > > + pm_runtime_put_noidle(data->dev); > > > + pm_runtime_disable(data->dev); > > > clk_disable: > > > clk_disable_unprepare(data->thermal_clk); > > > legacy_cleanup: > > > @@ -829,13 +818,9 @@ static int imx_thermal_probe(struct platform_device *pdev) > > > static int imx_thermal_remove(struct platform_device *pdev) > > > { > > > struct imx_thermal_data *data = platform_get_drvdata(pdev); > > > - struct regmap *map = data->tempmon; > > > > > > - /* Disable measurements */ > > > - regmap_write(map, data->socdata->sensor_ctrl + REG_SET, > > > - data->socdata->power_down_mask); > > > - if (!IS_ERR(data->thermal_clk)) > > > - clk_disable_unprepare(data->thermal_clk); > > > + pm_runtime_put_noidle(data->dev); > > > + pm_runtime_disable(data->dev); > > > > > > thermal_zone_device_unregister(data->tz); > > > imx_thermal_unregister_legacy_cooling(data); > > > @@ -858,29 +843,79 @@ static int __maybe_unused imx_thermal_suspend(struct device *dev) > > > ret = thermal_zone_device_disable(data->tz); > > > if (ret) > > > return ret; > > > + > > > + return pm_runtime_force_suspend(data->dev); > > > +} > > > + > > > +static int __maybe_unused imx_thermal_resume(struct device *dev) > > > +{ > > > + struct imx_thermal_data *data = dev_get_drvdata(dev); > > > + int ret; > > > + > > > + ret = pm_runtime_force_resume(data->dev); > > > + if (ret) > > > + return ret; > > > + /* Enabled thermal sensor after resume */ > > > + return thermal_zone_device_enable(data->tz); > > > +} > > > + > > > +static int __maybe_unused imx_thermal_runtime_suspend(struct device *dev) > > > +{ > > > + struct imx_thermal_data *data = dev_get_drvdata(dev); > > > + const struct thermal_soc_data *socdata = data->socdata; > > > + struct regmap *map = data->tempmon; > > > + int ret; > > > + > > > + ret = regmap_write(map, socdata->sensor_ctrl + REG_CLR, > > > + socdata->measure_temp_mask); > > > + if (ret) > > > + return ret; > > > + > > > + ret = regmap_write(map, socdata->sensor_ctrl + REG_SET, > > > + socdata->power_down_mask); > > > + if (ret) > > > + return ret; > > > + > > > clk_disable_unprepare(data->thermal_clk); > > > > > > return 0; > > > } > > > > > > -static int __maybe_unused imx_thermal_resume(struct device *dev) > > > +static int __maybe_unused imx_thermal_runtime_resume(struct device *dev) > > > { > > > struct imx_thermal_data *data = dev_get_drvdata(dev); > > > + const struct thermal_soc_data *socdata = data->socdata; > > > + struct regmap *map = data->tempmon; > > > int ret; > > > > > > ret = clk_prepare_enable(data->thermal_clk); > > > if (ret) > > > return ret; > > > - /* Enabled thermal sensor after resume */ > > > - ret = thermal_zone_device_enable(data->tz); > > > + > > > + ret = regmap_write(map, socdata->sensor_ctrl + REG_CLR, > > > + socdata->power_down_mask); > > > + if (ret) > > > + return ret; > > > + > > > + ret = regmap_write(map, socdata->sensor_ctrl + REG_SET, > > > + socdata->measure_temp_mask); > > > if (ret) > > > return ret; > > > > > > + /* > > > + * According to the temp sensor designers, it may require up to ~17us > > > + * to complete a measurement. > > > + */ > > > + usleep_range(20, 50); > > > + > > > return 0; > > > } > > > > > > -static SIMPLE_DEV_PM_OPS(imx_thermal_pm_ops, > > > - imx_thermal_suspend, imx_thermal_resume); > > > +static const struct dev_pm_ops imx_thermal_pm_ops = { > > > + SET_SYSTEM_SLEEP_PM_OPS(imx_thermal_suspend, imx_thermal_resume) > > > + SET_RUNTIME_PM_OPS(imx_thermal_runtime_suspend, > > > + imx_thermal_runtime_resume, NULL) > > > +}; > > > > > > static struct platform_driver imx_thermal = { > > > .driver = { > > > -- > > > 2.30.2 > > > > > > > > > > -- > > Pengutronix e.K. | | > > Steuerwalder Str. 21 | http://www.pengutronix.de/ | > > 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | > > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | > > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |