[PATCH v3 0/3] thermal: rockchip: fix up thermal driver

[PATCH v3 0/3] thermal: rockchip: fix up thermal driver

[Elaine Zhang]

1. add pinctrl control.
2. support PX30 soc

change in V3:
PATCH V3 1/3: remove panic.
PATCH V3 2/3: No change in V3.
PATCH V3 2/3: No change in V3

change in V2:
PATCH V2 1/3: keep tshut_mode TSHUT_MODE_GPIO;
              In case of pinctrl get or lookup error, just bail out;
              No need to use the thermal_pinctrl_select_otp/gpio wrappers,
              just replace them with:
PATCH V2 2/3: No change in V2.
PATCH V2 2/3: keep tshut_mode TSHUT_MODE_GPIO;
              Remove the grf in 'rk_tsadcv4_initialize' function.

Elaine Zhang (3):
  thermal: rockchip: fix up the tsadc pinctrl setting error
  dt-bindings: rockchip-thermal: Support the PX30 SoC compatible
  thermal: rockchip: Support the PX30 SoC in thermal driver

 .../bindings/thermal/rockchip-thermal.txt          |  1 +
 drivers/thermal/rockchip_thermal.c                 | 74 +++++++++++++++++++++-
 2 files changed, 72 insertions(+), 3 deletions(-)

-- 
1.9.1




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 1/3] thermal: rockchip: fix up the tsadc pinctrl setting error

[Elaine Zhang]

Explicitly use the pinctrl to set/unset the right mode
instead of relying on the pinctrl init mode.
And it requires setting the tshut polarity before select pinctrl.

When the temperature sensor mode is set to 0, it will automatically
reset the board via the Clock-Reset-Unit (CRU) if the over temperature
threshold is reached. However, when the pinctrl initializes, it does a
transition to "otp_out" which may lead the SoC restart all the time.

"otp_out" IO may be connected to the RESET circuit on the hardware.
If the IO is in the wrong state, it will trigger RESET.
(similar to the effect of pressing the RESET button)
which will cause the soc to restart all the time.

Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
---
 drivers/thermal/rockchip_thermal.c | 36 +++++++++++++++++++++++++++++++++---
 1 file changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
index 9c7643d62ed7..6dc7fc516abf 100644
--- a/drivers/thermal/rockchip_thermal.c
+++ b/drivers/thermal/rockchip_thermal.c
@@ -172,6 +172,9 @@ struct rockchip_thermal_data {
 	int tshut_temp;
 	enum tshut_mode tshut_mode;
 	enum tshut_polarity tshut_polarity;
+	struct pinctrl *pinctrl;
+	struct pinctrl_state *gpio_state;
+	struct pinctrl_state *otp_state;
 };
 
 /**
@@ -1242,6 +1245,8 @@ static int rockchip_thermal_probe(struct platform_device *pdev)
 		return error;
 	}
 
+	thermal->chip->control(thermal->regs, false);
+
 	error = clk_prepare_enable(thermal->clk);
 	if (error) {
 		dev_err(&pdev->dev, "failed to enable converter clock: %d\n",
@@ -1267,6 +1272,30 @@ static int rockchip_thermal_probe(struct platform_device *pdev)
 	thermal->chip->initialize(thermal->grf, thermal->regs,
 				  thermal->tshut_polarity);
 
+	if (thermal->tshut_mode == TSHUT_MODE_GPIO) {
+		thermal->pinctrl = devm_pinctrl_get(&pdev->dev);
+		if (IS_ERR(thermal->pinctrl)) {
+			dev_err(&pdev->dev, "failed to find thermal pinctrl\n");
+			return PTR_ERR(thermal->pinctrl);
+		}
+
+		thermal->gpio_state = pinctrl_lookup_state(thermal->pinctrl,
+							   "gpio");
+		if (IS_ERR_OR_NULL(thermal->gpio_state)) {
+			dev_err(&pdev->dev, "failed to find thermal gpio state\n");
+			return -EINVAL;
+		}
+
+		thermal->otp_state = pinctrl_lookup_state(thermal->pinctrl,
+							  "otpout");
+		if (IS_ERR_OR_NULL(thermal->otp_state)) {
+			dev_err(&pdev->dev, "failed to find thermal otpout state\n");
+			return -EINVAL;
+		}
+
+		pinctrl_select_state(thermal->pinctrl, thermal->otp_state);
+	}
+
 	for (i = 0; i < thermal->chip->chn_num; i++) {
 		error = rockchip_thermal_register_sensor(pdev, thermal,
 						&thermal->sensors[i],
@@ -1337,8 +1366,8 @@ static int __maybe_unused rockchip_thermal_suspend(struct device *dev)
 
 	clk_disable(thermal->pclk);
 	clk_disable(thermal->clk);
-
-	pinctrl_pm_select_sleep_state(dev);
+	if (thermal->tshut_mode == TSHUT_MODE_GPIO)
+		pinctrl_select_state(thermal->pinctrl, thermal->gpio_state);
 
 	return 0;
 }
@@ -1383,7 +1412,8 @@ static int __maybe_unused rockchip_thermal_resume(struct device *dev)
 	for (i = 0; i < thermal->chip->chn_num; i++)
 		rockchip_thermal_toggle_sensor(&thermal->sensors[i], true);
 
-	pinctrl_pm_select_default_state(dev);
+	if (thermal->tshut_mode == TSHUT_MODE_GPIO)
+		pinctrl_select_state(thermal->pinctrl, thermal->otp_state);
 
 	return 0;
 }
-- 
1.9.1




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 2/3] dt-bindings: rockchip-thermal: Support the PX30 SoC compatible

[Elaine Zhang]

Add a new compatible for thermal founding on PX30 SoCs.

Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/thermal/rockchip-thermal.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
index 43d744e5305e..c6aac9bcacf1 100644
--- a/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
+++ b/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
@@ -2,6 +2,7 @@
 
 Required properties:
 - compatible : should be "rockchip,<name>-tsadc"
+   "rockchip,px30-tsadc":   found on PX30 SoCs
    "rockchip,rv1108-tsadc": found on RV1108 SoCs
    "rockchip,rk3228-tsadc": found on RK3228 SoCs
    "rockchip,rk3288-tsadc": found on RK3288 SoCs
-- 
1.9.1




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 3/3] thermal: rockchip: Support the PX30 SoC in thermal driver

[Elaine Zhang]

PX30 SOC has two Temperature Sensors for CPU and GPU.

Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
---
 drivers/thermal/rockchip_thermal.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
index 6dc7fc516abf..bda1ca199abd 100644
--- a/drivers/thermal/rockchip_thermal.c
+++ b/drivers/thermal/rockchip_thermal.c
@@ -225,11 +225,15 @@ struct rockchip_thermal_data {
 #define GRF_TSADC_TESTBIT_L			0x0e648
 #define GRF_TSADC_TESTBIT_H			0x0e64c
 
+#define PX30_GRF_SOC_CON2			0x0408
+
 #define GRF_SARADC_TESTBIT_ON			(0x10001 << 2)
 #define GRF_TSADC_TESTBIT_H_ON			(0x10001 << 2)
 #define GRF_TSADC_VCM_EN_L			(0x10001 << 7)
 #define GRF_TSADC_VCM_EN_H			(0x10001 << 7)
 
+#define GRF_CON_TSADC_CH_INV			(0x10001 << 1)
+
 /**
  * struct tsadc_table - code to temperature conversion table
  * @code: the value of adc channel
@@ -692,6 +696,13 @@ static void rk_tsadcv3_initialize(struct regmap *grf, void __iomem *regs,
 			       regs + TSADCV2_AUTO_CON);
 }
 
+static void rk_tsadcv4_initialize(struct regmap *grf, void __iomem *regs,
+				  enum tshut_polarity tshut_polarity)
+{
+	rk_tsadcv2_initialize(grf, regs, tshut_polarity);
+	regmap_write(grf, PX30_GRF_SOC_CON2, GRF_CON_TSADC_CH_INV);
+}
+
 static void rk_tsadcv2_irq_ack(void __iomem *regs)
 {
 	u32 val;
@@ -821,6 +832,30 @@ static void rk_tsadcv2_tshut_mode(int chn, void __iomem *regs,
 	writel_relaxed(val, regs + TSADCV2_INT_EN);
 }
 
+static const struct rockchip_tsadc_chip px30_tsadc_data = {
+	.chn_id[SENSOR_CPU] = 0, /* cpu sensor is channel 0 */
+	.chn_id[SENSOR_GPU] = 1, /* gpu sensor is channel 1 */
+	.chn_num = 2, /* 2 channels for tsadc */
+
+	.tshut_mode = TSHUT_MODE_CRU, /* default TSHUT via CRU */
+	.tshut_temp = 95000,
+
+	.initialize = rk_tsadcv4_initialize,
+	.irq_ack = rk_tsadcv3_irq_ack,
+	.control = rk_tsadcv3_control,
+	.get_temp = rk_tsadcv2_get_temp,
+	.set_alarm_temp = rk_tsadcv2_alarm_temp,
+	.set_tshut_temp = rk_tsadcv2_tshut_temp,
+	.set_tshut_mode = rk_tsadcv2_tshut_mode,
+
+	.table = {
+		.id = rk3328_code_table,
+		.length = ARRAY_SIZE(rk3328_code_table),
+		.data_mask = TSADCV2_DATA_MASK,
+		.mode = ADC_INCREMENT,
+	},
+};
+
 static const struct rockchip_tsadc_chip rv1108_tsadc_data = {
 	.chn_id[SENSOR_CPU] = 0, /* cpu sensor is channel 0 */
 	.chn_num = 1, /* one channel for tsadc */
@@ -993,6 +1028,9 @@ static void rk_tsadcv2_tshut_mode(int chn, void __iomem *regs,
 };
 
 static const struct of_device_id of_rockchip_thermal_match[] = {
+	{	.compatible = "rockchip,px30-tsadc",
+		.data = (void *)&px30_tsadc_data,
+	},
 	{
 		.compatible = "rockchip,rv1108-tsadc",
 		.data = (void *)&rv1108_tsadc_data,
-- 
1.9.1




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/3] thermal: rockchip: fix up the tsadc pinctrl setting error

[Daniel Lezcano]

On 30/04/2019 12:09, Elaine Zhang wrote:
> Explicitly use the pinctrl to set/unset the right mode
> instead of relying on the pinctrl init mode.
> And it requires setting the tshut polarity before select pinctrl.
> 
> When the temperature sensor mode is set to 0, it will automatically
> reset the board via the Clock-Reset-Unit (CRU) if the over temperature
> threshold is reached. However, when the pinctrl initializes, it does a
> transition to "otp_out" which may lead the SoC restart all the time.
> 
> "otp_out" IO may be connected to the RESET circuit on the hardware.
> If the IO is in the wrong state, it will trigger RESET.
> (similar to the effect of pressing the RESET button)
> which will cause the soc to restart all the time.
> 
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>

Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>



> ---
>  drivers/thermal/rockchip_thermal.c | 36 +++++++++++++++++++++++++++++++++---
>  1 file changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> index 9c7643d62ed7..6dc7fc516abf 100644
> --- a/drivers/thermal/rockchip_thermal.c
> +++ b/drivers/thermal/rockchip_thermal.c
> @@ -172,6 +172,9 @@ struct rockchip_thermal_data {
>  	int tshut_temp;
>  	enum tshut_mode tshut_mode;
>  	enum tshut_polarity tshut_polarity;
> +	struct pinctrl *pinctrl;
> +	struct pinctrl_state *gpio_state;
> +	struct pinctrl_state *otp_state;
>  };
>  
>  /**
> @@ -1242,6 +1245,8 @@ static int rockchip_thermal_probe(struct platform_device *pdev)
>  		return error;
>  	}
>  
> +	thermal->chip->control(thermal->regs, false);
> +
>  	error = clk_prepare_enable(thermal->clk);
>  	if (error) {
>  		dev_err(&pdev->dev, "failed to enable converter clock: %d\n",
> @@ -1267,6 +1272,30 @@ static int rockchip_thermal_probe(struct platform_device *pdev)
>  	thermal->chip->initialize(thermal->grf, thermal->regs,
>  				  thermal->tshut_polarity);
>  
> +	if (thermal->tshut_mode == TSHUT_MODE_GPIO) {
> +		thermal->pinctrl = devm_pinctrl_get(&pdev->dev);
> +		if (IS_ERR(thermal->pinctrl)) {
> +			dev_err(&pdev->dev, "failed to find thermal pinctrl\n");
> +			return PTR_ERR(thermal->pinctrl);
> +		}
> +
> +		thermal->gpio_state = pinctrl_lookup_state(thermal->pinctrl,
> +							   "gpio");
> +		if (IS_ERR_OR_NULL(thermal->gpio_state)) {
> +			dev_err(&pdev->dev, "failed to find thermal gpio state\n");
> +			return -EINVAL;
> +		}
> +
> +		thermal->otp_state = pinctrl_lookup_state(thermal->pinctrl,
> +							  "otpout");
> +		if (IS_ERR_OR_NULL(thermal->otp_state)) {
> +			dev_err(&pdev->dev, "failed to find thermal otpout state\n");
> +			return -EINVAL;
> +		}
> +
> +		pinctrl_select_state(thermal->pinctrl, thermal->otp_state);
> +	}
> +
>  	for (i = 0; i < thermal->chip->chn_num; i++) {
>  		error = rockchip_thermal_register_sensor(pdev, thermal,
>  						&thermal->sensors[i],
> @@ -1337,8 +1366,8 @@ static int __maybe_unused rockchip_thermal_suspend(struct device *dev)
>  
>  	clk_disable(thermal->pclk);
>  	clk_disable(thermal->clk);
> -
> -	pinctrl_pm_select_sleep_state(dev);
> +	if (thermal->tshut_mode == TSHUT_MODE_GPIO)
> +		pinctrl_select_state(thermal->pinctrl, thermal->gpio_state);
>  
>  	return 0;
>  }
> @@ -1383,7 +1412,8 @@ static int __maybe_unused rockchip_thermal_resume(struct device *dev)
>  	for (i = 0; i < thermal->chip->chn_num; i++)
>  		rockchip_thermal_toggle_sensor(&thermal->sensors[i], true);
>  
> -	pinctrl_pm_select_default_state(dev);
> +	if (thermal->tshut_mode == TSHUT_MODE_GPIO)
> +		pinctrl_select_state(thermal->pinctrl, thermal->otp_state);
>  
>  	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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/3] thermal: rockchip: fix up the tsadc pinctrl setting error

[Enric Balletbo Serra]

Hi all,

As pointed by [1] and [2] this commit, that now is upstream, breaks
veyron (rk3288) and kevin (rk3399) boards. The problem is especially
critical for veyron boards because they don't boot anymore.

I didn't look deep at the problem but I have some concerns about this
patch, see below.

[1] https://www.spinics.net/lists/linux-rockchip/msg24657.html
[2] https://www.spinics.net/lists/linux-rockchip/msg24735.html

Missatge de Daniel Lezcano <daniel.lezcano@linaro.org> del dia dt., 30
d’abr. 2019 a les 15:39:
>
> On 30/04/2019 12:09, Elaine Zhang wrote:
> > Explicitly use the pinctrl to set/unset the right mode
> > instead of relying on the pinctrl init mode.
> > And it requires setting the tshut polarity before select pinctrl.
> >
> > When the temperature sensor mode is set to 0, it will automatically
> > reset the board via the Clock-Reset-Unit (CRU) if the over temperature
> > threshold is reached. However, when the pinctrl initializes, it does a
> > transition to "otp_out" which may lead the SoC restart all the time.
> >
> > "otp_out" IO may be connected to the RESET circuit on the hardware.
> > If the IO is in the wrong state, it will trigger RESET.
> > (similar to the effect of pressing the RESET button)
> > which will cause the soc to restart all the time.
> >
> > Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
>
> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>
>
>
> > ---
> >  drivers/thermal/rockchip_thermal.c | 36 +++++++++++++++++++++++++++++++++---
> >  1 file changed, 33 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> > index 9c7643d62ed7..6dc7fc516abf 100644
> > --- a/drivers/thermal/rockchip_thermal.c
> > +++ b/drivers/thermal/rockchip_thermal.c
> > @@ -172,6 +172,9 @@ struct rockchip_thermal_data {
> >       int tshut_temp;
> >       enum tshut_mode tshut_mode;
> >       enum tshut_polarity tshut_polarity;
> > +     struct pinctrl *pinctrl;
> > +     struct pinctrl_state *gpio_state;
> > +     struct pinctrl_state *otp_state;
> >  };
> >
> >  /**
> > @@ -1242,6 +1245,8 @@ static int rockchip_thermal_probe(struct platform_device *pdev)
> >               return error;
> >       }
> >
> > +     thermal->chip->control(thermal->regs, false);
> > +

That's the line that causes the hang. Commenting this makes the veyron
boot again. Probably this needs to go after chip->initialize?

> >       error = clk_prepare_enable(thermal->clk);
> >       if (error) {
> >               dev_err(&pdev->dev, "failed to enable converter clock: %d\n",
> > @@ -1267,6 +1272,30 @@ static int rockchip_thermal_probe(struct platform_device *pdev)
> >       thermal->chip->initialize(thermal->grf, thermal->regs,
> >                                 thermal->tshut_polarity);
> >
> > +     if (thermal->tshut_mode == TSHUT_MODE_GPIO) {
> > +             thermal->pinctrl = devm_pinctrl_get(&pdev->dev);
> > +             if (IS_ERR(thermal->pinctrl)) {
> > +                     dev_err(&pdev->dev, "failed to find thermal pinctrl\n");
> > +                     return PTR_ERR(thermal->pinctrl);
> > +             }
> > +
> > +             thermal->gpio_state = pinctrl_lookup_state(thermal->pinctrl,
> > +                                                        "gpio");

Shouldn't this mode be documented properly in the binding first?

The binding [3] talks about init, default and sleep states but *not*
gpio and otpout. The patch series looks incomplete to me or not using
the proper names.

[3] https://elixir.bootlin.com/linux/v5.2-rc1/source/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt

> > +             if (IS_ERR_OR_NULL(thermal->gpio_state)) {
> > +                     dev_err(&pdev->dev, "failed to find thermal gpio state\n");
> > +                     return -EINVAL;
> > +             }
> > +
> > +             thermal->otp_state = pinctrl_lookup_state(thermal->pinctrl,
> > +                                                       "otpout");
> > +             if (IS_ERR_OR_NULL(thermal->otp_state)) {
> > +                     dev_err(&pdev->dev, "failed to find thermal otpout state\n");
> > +                     return -EINVAL;
> > +             }
> > +

Same here otpout is not a documented.

As this change is now in mainline and is causing veyron to hang I'd
suggest reverting this change for now. Even fixing the root cause
(maybe the one I pointed above) after this patch we will have the
thermal driver to fail because "gpio" and "otpout" states are not
defined nor documented (a change on this will need some reviews and
acks and time I guess).

Cheers,
 Enric

> > +             pinctrl_select_state(thermal->pinctrl, thermal->otp_state);
> > +     }
> > +
> >       for (i = 0; i < thermal->chip->chn_num; i++) {
> >               error = rockchip_thermal_register_sensor(pdev, thermal,
> >                                               &thermal->sensors[i],
> > @@ -1337,8 +1366,8 @@ static int __maybe_unused rockchip_thermal_suspend(struct device *dev)
> >
> >       clk_disable(thermal->pclk);
> >       clk_disable(thermal->clk);
> > -
> > -     pinctrl_pm_select_sleep_state(dev);
> > +     if (thermal->tshut_mode == TSHUT_MODE_GPIO)
> > +             pinctrl_select_state(thermal->pinctrl, thermal->gpio_state);
> >
> >       return 0;
> >  }
> > @@ -1383,7 +1412,8 @@ static int __maybe_unused rockchip_thermal_resume(struct device *dev)
> >       for (i = 0; i < thermal->chip->chn_num; i++)
> >               rockchip_thermal_toggle_sensor(&thermal->sensors[i], true);
> >
> > -     pinctrl_pm_select_default_state(dev);
> > +     if (thermal->tshut_mode == TSHUT_MODE_GPIO)
> > +             pinctrl_select_state(thermal->pinctrl, thermal->otp_state);
> >
> >       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
>
>
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/3] thermal: rockchip: fix up the tsadc pinctrl setting error

[Daniel Lezcano]

Elaine,

are you taking care of the issue related to this patch. If not fixed, it
will be reverted.


On 30/04/2019 12:09, Elaine Zhang wrote:
> Explicitly use the pinctrl to set/unset the right mode
> instead of relying on the pinctrl init mode.
> And it requires setting the tshut polarity before select pinctrl.
> 
> When the temperature sensor mode is set to 0, it will automatically
> reset the board via the Clock-Reset-Unit (CRU) if the over temperature
> threshold is reached. However, when the pinctrl initializes, it does a
> transition to "otp_out" which may lead the SoC restart all the time.
> 
> "otp_out" IO may be connected to the RESET circuit on the hardware.
> If the IO is in the wrong state, it will trigger RESET.
> (similar to the effect of pressing the RESET button)
> which will cause the soc to restart all the time.
> 
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> ---
>  drivers/thermal/rockchip_thermal.c | 36 +++++++++++++++++++++++++++++++++---
>  1 file changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> index 9c7643d62ed7..6dc7fc516abf 100644
> --- a/drivers/thermal/rockchip_thermal.c
> +++ b/drivers/thermal/rockchip_thermal.c
> @@ -172,6 +172,9 @@ struct rockchip_thermal_data {
>  	int tshut_temp;
>  	enum tshut_mode tshut_mode;
>  	enum tshut_polarity tshut_polarity;
> +	struct pinctrl *pinctrl;
> +	struct pinctrl_state *gpio_state;
> +	struct pinctrl_state *otp_state;
>  };
>  
>  /**
> @@ -1242,6 +1245,8 @@ static int rockchip_thermal_probe(struct platform_device *pdev)
>  		return error;
>  	}
>  
> +	thermal->chip->control(thermal->regs, false);
> +
>  	error = clk_prepare_enable(thermal->clk);
>  	if (error) {
>  		dev_err(&pdev->dev, "failed to enable converter clock: %d\n",
> @@ -1267,6 +1272,30 @@ static int rockchip_thermal_probe(struct platform_device *pdev)
>  	thermal->chip->initialize(thermal->grf, thermal->regs,
>  				  thermal->tshut_polarity);
>  
> +	if (thermal->tshut_mode == TSHUT_MODE_GPIO) {
> +		thermal->pinctrl = devm_pinctrl_get(&pdev->dev);
> +		if (IS_ERR(thermal->pinctrl)) {
> +			dev_err(&pdev->dev, "failed to find thermal pinctrl\n");
> +			return PTR_ERR(thermal->pinctrl);
> +		}
> +
> +		thermal->gpio_state = pinctrl_lookup_state(thermal->pinctrl,
> +							   "gpio");
> +		if (IS_ERR_OR_NULL(thermal->gpio_state)) {
> +			dev_err(&pdev->dev, "failed to find thermal gpio state\n");
> +			return -EINVAL;
> +		}
> +
> +		thermal->otp_state = pinctrl_lookup_state(thermal->pinctrl,
> +							  "otpout");
> +		if (IS_ERR_OR_NULL(thermal->otp_state)) {
> +			dev_err(&pdev->dev, "failed to find thermal otpout state\n");
> +			return -EINVAL;
> +		}
> +
> +		pinctrl_select_state(thermal->pinctrl, thermal->otp_state);
> +	}
> +
>  	for (i = 0; i < thermal->chip->chn_num; i++) {
>  		error = rockchip_thermal_register_sensor(pdev, thermal,
>  						&thermal->sensors[i],
> @@ -1337,8 +1366,8 @@ static int __maybe_unused rockchip_thermal_suspend(struct device *dev)
>  
>  	clk_disable(thermal->pclk);
>  	clk_disable(thermal->clk);
> -
> -	pinctrl_pm_select_sleep_state(dev);
> +	if (thermal->tshut_mode == TSHUT_MODE_GPIO)
> +		pinctrl_select_state(thermal->pinctrl, thermal->gpio_state);
>  
>  	return 0;
>  }
> @@ -1383,7 +1412,8 @@ static int __maybe_unused rockchip_thermal_resume(struct device *dev)
>  	for (i = 0; i < thermal->chip->chn_num; i++)
>  		rockchip_thermal_toggle_sensor(&thermal->sensors[i], true);
>  
> -	pinctrl_pm_select_default_state(dev);
> +	if (thermal->tshut_mode == TSHUT_MODE_GPIO)
> +		pinctrl_select_state(thermal->pinctrl, thermal->otp_state);
>  
>  	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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/3] thermal: rockchip: fix up the tsadc pinctrl setting error

[Heiko Stuebner]

Hi Enric,

Am Montag, 20. Mai 2019, 15:38:32 CEST schrieb Enric Balletbo Serra:
> Hi all,
> 
> As pointed by [1] and [2] this commit, that now is upstream, breaks
> veyron (rk3288) and kevin (rk3399) boards. The problem is especially
> critical for veyron boards because they don't boot anymore.
> 
> I didn't look deep at the problem but I have some concerns about this
> patch, see below.
> 
> [1] https://www.spinics.net/lists/linux-rockchip/msg24657.html
> [2] https://www.spinics.net/lists/linux-rockchip/msg24735.html
> 
> Missatge de Daniel Lezcano <daniel.lezcano@linaro.org> del dia dt., 30
> d’abr. 2019 a les 15:39:
> >
> > On 30/04/2019 12:09, Elaine Zhang wrote:
> > > Explicitly use the pinctrl to set/unset the right mode
> > > instead of relying on the pinctrl init mode.
> > > And it requires setting the tshut polarity before select pinctrl.
> > >
> > > When the temperature sensor mode is set to 0, it will automatically
> > > reset the board via the Clock-Reset-Unit (CRU) if the over temperature
> > > threshold is reached. However, when the pinctrl initializes, it does a
> > > transition to "otp_out" which may lead the SoC restart all the time.
> > >
> > > "otp_out" IO may be connected to the RESET circuit on the hardware.
> > > If the IO is in the wrong state, it will trigger RESET.
> > > (similar to the effect of pressing the RESET button)
> > > which will cause the soc to restart all the time.
> > >
> > > Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> >
> > Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >
> >
> >
> > > ---
> > >  drivers/thermal/rockchip_thermal.c | 36 +++++++++++++++++++++++++++++++++---
> > >  1 file changed, 33 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> > > index 9c7643d62ed7..6dc7fc516abf 100644
> > > --- a/drivers/thermal/rockchip_thermal.c
> > > +++ b/drivers/thermal/rockchip_thermal.c
> > > @@ -172,6 +172,9 @@ struct rockchip_thermal_data {
> > >       int tshut_temp;
> > >       enum tshut_mode tshut_mode;
> > >       enum tshut_polarity tshut_polarity;
> > > +     struct pinctrl *pinctrl;
> > > +     struct pinctrl_state *gpio_state;
> > > +     struct pinctrl_state *otp_state;
> > >  };
> > >
> > >  /**
> > > @@ -1242,6 +1245,8 @@ static int rockchip_thermal_probe(struct platform_device *pdev)
> > >               return error;
> > >       }
> > >
> > > +     thermal->chip->control(thermal->regs, false);
> > > +
> 
> That's the line that causes the hang. Commenting this makes the veyron
> boot again. Probably this needs to go after chip->initialize?

It needs to go after the clk_enable calls.
At this point the tsadc may still be unclocked.

> 
> > >       error = clk_prepare_enable(thermal->clk);
> > >       if (error) {
> > >               dev_err(&pdev->dev, "failed to enable converter clock: %d\n",
> > > @@ -1267,6 +1272,30 @@ static int rockchip_thermal_probe(struct platform_device *pdev)
> > >       thermal->chip->initialize(thermal->grf, thermal->regs,
> > >                                 thermal->tshut_polarity);
> > >
> > > +     if (thermal->tshut_mode == TSHUT_MODE_GPIO) {
> > > +             thermal->pinctrl = devm_pinctrl_get(&pdev->dev);
> > > +             if (IS_ERR(thermal->pinctrl)) {
> > > +                     dev_err(&pdev->dev, "failed to find thermal pinctrl\n");
> > > +                     return PTR_ERR(thermal->pinctrl);
> > > +             }
> > > +
> > > +             thermal->gpio_state = pinctrl_lookup_state(thermal->pinctrl,
> > > +                                                        "gpio");
> 
> Shouldn't this mode be documented properly in the binding first?

More importantly, it should be _backwards-compatible_, aka work with
old devicetrees without that property and not break thermal handling for
them entirely.


> 
> The binding [3] talks about init, default and sleep states but *not*
> gpio and otpout. The patch series looks incomplete to me or not using
> the proper names.
> 
> [3] https://elixir.bootlin.com/linux/v5.2-rc1/source/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> 
> > > +             if (IS_ERR_OR_NULL(thermal->gpio_state)) {
> > > +                     dev_err(&pdev->dev, "failed to find thermal gpio state\n");
> > > +                     return -EINVAL;
> > > +             }
> > > +
> > > +             thermal->otp_state = pinctrl_lookup_state(thermal->pinctrl,
> > > +                                                       "otpout");
> > > +             if (IS_ERR_OR_NULL(thermal->otp_state)) {
> > > +                     dev_err(&pdev->dev, "failed to find thermal otpout state\n");
> > > +                     return -EINVAL;
> > > +             }
> > > +
> 
> Same here otpout is not a documented.
> 
> As this change is now in mainline and is causing veyron to hang I'd
> suggest reverting this change for now. Even fixing the root cause
> (maybe the one I pointed above) after this patch we will have the
> thermal driver to fail because "gpio" and "otpout" states are not
> defined nor documented (a change on this will need some reviews and
> acks and time I guess).

I definitly agree here. Handling + checking the binding change
as well as needed fallback code is definitly not material for -rc-kernels
so we should just revert for now and let Elaine fix the issues for 5.3.

Anyone volunteering for sending a revert-patch to Eduardo? :-)

Heiko





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/3] thermal: rockchip: fix up the tsadc pinctrl setting error

[Daniel Lezcano]

On 22/05/2019 14:27, Heiko Stuebner wrote:

[ ... ]

>> As this change is now in mainline and is causing veyron to hang I'd
>> suggest reverting this change for now. Even fixing the root cause
>> (maybe the one I pointed above) after this patch we will have the
>> thermal driver to fail because "gpio" and "otpout" states are not
>> defined nor documented (a change on this will need some reviews and
>> acks and time I guess).
> 
> I definitly agree here. Handling + checking the binding change
> as well as needed fallback code is definitly not material for -rc-kernels
> so we should just revert for now and let Elaine fix the issues for 5.3.
> 
> Anyone volunteering for sending a revert-patch to Eduardo? :-)

I can't right now :/


-- 
 <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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/3] thermal: rockchip: fix up the tsadc pinctrl setting error

[Heiko Stuebner]

Am Mittwoch, 22. Mai 2019, 14:30:16 CEST schrieb Daniel Lezcano:
> On 22/05/2019 14:27, Heiko Stuebner wrote:
> 
> [ ... ]
> 
> >> As this change is now in mainline and is causing veyron to hang I'd
> >> suggest reverting this change for now. Even fixing the root cause
> >> (maybe the one I pointed above) after this patch we will have the
> >> thermal driver to fail because "gpio" and "otpout" states are not
> >> defined nor documented (a change on this will need some reviews and
> >> acks and time I guess).
> > 
> > I definitly agree here. Handling + checking the binding change
> > as well as needed fallback code is definitly not material for -rc-kernels
> > so we should just revert for now and let Elaine fix the issues for 5.3.
> > 
> > Anyone volunteering for sending a revert-patch to Eduardo? :-)
> 
> I can't right now :/

ok, I'll do the revert patch then, so that we get this sorted.


Heiko



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/3] thermal: rockchip: fix up the tsadc pinctrl setting error

[elaine.zhang]

hi, Heiko & Enric:

在 2019/5/22 下午8:27, Heiko Stuebner 写道:
> Hi Enric,
>
> Am Montag, 20. Mai 2019, 15:38:32 CEST schrieb Enric Balletbo Serra:
>> Hi all,
>>
>> As pointed by [1] and [2] this commit, that now is upstream, breaks
>> veyron (rk3288) and kevin (rk3399) boards. The problem is especially
>> critical for veyron boards because they don't boot anymore.
>>
>> I didn't look deep at the problem but I have some concerns about this
>> patch, see below.
>>
>> [1] https://www.spinics.net/lists/linux-rockchip/msg24657.html
>> [2] https://www.spinics.net/lists/linux-rockchip/msg24735.html
>>
>> Missatge de Daniel Lezcano <daniel.lezcano@linaro.org> del dia dt., 30
>> d’abr. 2019 a les 15:39:
>>> On 30/04/2019 12:09, Elaine Zhang wrote:
>>>> Explicitly use the pinctrl to set/unset the right mode
>>>> instead of relying on the pinctrl init mode.
>>>> And it requires setting the tshut polarity before select pinctrl.
>>>>
>>>> When the temperature sensor mode is set to 0, it will automatically
>>>> reset the board via the Clock-Reset-Unit (CRU) if the over temperature
>>>> threshold is reached. However, when the pinctrl initializes, it does a
>>>> transition to "otp_out" which may lead the SoC restart all the time.
>>>>
>>>> "otp_out" IO may be connected to the RESET circuit on the hardware.
>>>> If the IO is in the wrong state, it will trigger RESET.
>>>> (similar to the effect of pressing the RESET button)
>>>> which will cause the soc to restart all the time.
>>>>
>>>> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
>>> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>
>>>
>>>
>>>> ---
>>>>   drivers/thermal/rockchip_thermal.c | 36 +++++++++++++++++++++++++++++++++---
>>>>   1 file changed, 33 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
>>>> index 9c7643d62ed7..6dc7fc516abf 100644
>>>> --- a/drivers/thermal/rockchip_thermal.c
>>>> +++ b/drivers/thermal/rockchip_thermal.c
>>>> @@ -172,6 +172,9 @@ struct rockchip_thermal_data {
>>>>        int tshut_temp;
>>>>        enum tshut_mode tshut_mode;
>>>>        enum tshut_polarity tshut_polarity;
>>>> +     struct pinctrl *pinctrl;
>>>> +     struct pinctrl_state *gpio_state;
>>>> +     struct pinctrl_state *otp_state;
>>>>   };
>>>>
>>>>   /**
>>>> @@ -1242,6 +1245,8 @@ static int rockchip_thermal_probe(struct platform_device *pdev)
>>>>                return error;
>>>>        }
>>>>
>>>> +     thermal->chip->control(thermal->regs, false);
>>>> +
>> That's the line that causes the hang. Commenting this makes the veyron
>> boot again. Probably this needs to go after chip->initialize?
> It needs to go after the clk_enable calls.
> At this point the tsadc may still be unclocked.

The clk is enable by default.


The reason for this modification:

The otp Pin polarity setting for tsadc must be set when tsadc is turned off.

The order:

Close the tsadc->Set the otp pin polarity ->Set the pinctrl->initialize 
the tsadc->Open the tsadc


As for the problem you mentioned, I guess: The default polarity of otp 
does not match the default state, that is, the otp is triggered by 
default, and then the reset circuit of the hardware takes effect and is 
restarted all the time.
Modification:
1. For this hardware, otp pin default state is modified.
2. The mode of using CRU is rockchip,hw-tshut-mode = <0> in DTS;
/* tshut mode 0:CRU 1:GPIO */

Recommended use method 2. You can try it.

>
>>>>        error = clk_prepare_enable(thermal->clk);
>>>>        if (error) {
>>>>                dev_err(&pdev->dev, "failed to enable converter clock: %d\n",
>>>> @@ -1267,6 +1272,30 @@ static int rockchip_thermal_probe(struct platform_device *pdev)
>>>>        thermal->chip->initialize(thermal->grf, thermal->regs,
>>>>                                  thermal->tshut_polarity);
>>>>
>>>> +     if (thermal->tshut_mode == TSHUT_MODE_GPIO) {
>>>> +             thermal->pinctrl = devm_pinctrl_get(&pdev->dev);
>>>> +             if (IS_ERR(thermal->pinctrl)) {
>>>> +                     dev_err(&pdev->dev, "failed to find thermal pinctrl\n");
>>>> +                     return PTR_ERR(thermal->pinctrl);
>>>> +             }
>>>> +
>>>> +             thermal->gpio_state = pinctrl_lookup_state(thermal->pinctrl,
>>>> +                                                        "gpio");
>> Shouldn't this mode be documented properly in the binding first?
> More importantly, it should be _backwards-compatible_, aka work with
> old devicetrees without that property and not break thermal handling for
> them entirely.
If need  _backwards-compatible_,  It's can't return 
PTR_ERR(thermal->pinctrl) when get

devm_pinctrl_get failed.

>
>> The binding [3] talks about init, default and sleep states but *not*
>> gpio and otpout. The patch series looks incomplete to me or not using
>> the proper names.
>>
>> [3] https://elixir.bootlin.com/linux/v5.2-rc1/source/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
>>
>>>> +             if (IS_ERR_OR_NULL(thermal->gpio_state)) {
>>>> +                     dev_err(&pdev->dev, "failed to find thermal gpio state\n");
>>>> +                     return -EINVAL;
>>>> +             }
>>>> +
>>>> +             thermal->otp_state = pinctrl_lookup_state(thermal->pinctrl,
>>>> +                                                       "otpout");
>>>> +             if (IS_ERR_OR_NULL(thermal->otp_state)) {
>>>> +                     dev_err(&pdev->dev, "failed to find thermal otpout state\n");
>>>> +                     return -EINVAL;
>>>> +             }
>>>> +
>> Same here otpout is not a documented.
>>
>> As this change is now in mainline and is causing veyron to hang I'd
>> suggest reverting this change for now. Even fixing the root cause
>> (maybe the one I pointed above) after this patch we will have the
>> thermal driver to fail because "gpio" and "otpout" states are not
>> defined nor documented (a change on this will need some reviews and
>> acks and time I guess).
> I definitly agree here. Handling + checking the binding change
> as well as needed fallback code is definitly not material for -rc-kernels
> so we should just revert for now and let Elaine fix the issues for 5.3.
>
> Anyone volunteering for sending a revert-patch to Eduardo? :-)

I agree to revert the patch,and I will correct it and push it later.

Do I need to commit the revert the patch now?@Heiko

>
> Heiko
>
>
>
>
>
>
>



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/3] thermal: rockchip: fix up the tsadc pinctrl setting error

[Eduardo Valentin]

On Thu, May 23, 2019 at 09:34:37AM +0800, elaine.zhang wrote:
> hi, Heiko & Enric:
> 
> 在 2019/5/22 下午8:27, Heiko Stuebner 写道:
> >Hi Enric,
> >
> >Am Montag, 20. Mai 2019, 15:38:32 CEST schrieb Enric Balletbo Serra:
> >>Hi all,
> >>
> >>As pointed by [1] and [2] this commit, that now is upstream, breaks
> >>veyron (rk3288) and kevin (rk3399) boards. The problem is especially
> >>critical for veyron boards because they don't boot anymore.
> >>
> >>I didn't look deep at the problem but I have some concerns about this
> >>patch, see below.
> >>
> >>[1] https://www.spinics.net/lists/linux-rockchip/msg24657.html
> >>[2] https://www.spinics.net/lists/linux-rockchip/msg24735.html
> >>
> >>Missatge de Daniel Lezcano <daniel.lezcano@linaro.org> del dia dt., 30
> >>d’abr. 2019 a les 15:39:
> >>>On 30/04/2019 12:09, Elaine Zhang wrote:
> >>>>Explicitly use the pinctrl to set/unset the right mode
> >>>>instead of relying on the pinctrl init mode.
> >>>>And it requires setting the tshut polarity before select pinctrl.
> >>>>
> >>>>When the temperature sensor mode is set to 0, it will automatically
> >>>>reset the board via the Clock-Reset-Unit (CRU) if the over temperature
> >>>>threshold is reached. However, when the pinctrl initializes, it does a
> >>>>transition to "otp_out" which may lead the SoC restart all the time.
> >>>>
> >>>>"otp_out" IO may be connected to the RESET circuit on the hardware.
> >>>>If the IO is in the wrong state, it will trigger RESET.
> >>>>(similar to the effect of pressing the RESET button)
> >>>>which will cause the soc to restart all the time.
> >>>>
> >>>>Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> >>>Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>>
> >>>
> >>>
> >>>>---
> >>>>  drivers/thermal/rockchip_thermal.c | 36 +++++++++++++++++++++++++++++++++---
> >>>>  1 file changed, 33 insertions(+), 3 deletions(-)
> >>>>
> >>>>diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> >>>>index 9c7643d62ed7..6dc7fc516abf 100644
> >>>>--- a/drivers/thermal/rockchip_thermal.c
> >>>>+++ b/drivers/thermal/rockchip_thermal.c
> >>>>@@ -172,6 +172,9 @@ struct rockchip_thermal_data {
> >>>>       int tshut_temp;
> >>>>       enum tshut_mode tshut_mode;
> >>>>       enum tshut_polarity tshut_polarity;
> >>>>+     struct pinctrl *pinctrl;
> >>>>+     struct pinctrl_state *gpio_state;
> >>>>+     struct pinctrl_state *otp_state;
> >>>>  };
> >>>>
> >>>>  /**
> >>>>@@ -1242,6 +1245,8 @@ static int rockchip_thermal_probe(struct platform_device *pdev)
> >>>>               return error;
> >>>>       }
> >>>>
> >>>>+     thermal->chip->control(thermal->regs, false);
> >>>>+
> >>That's the line that causes the hang. Commenting this makes the veyron
> >>boot again. Probably this needs to go after chip->initialize?
> >It needs to go after the clk_enable calls.
> >At this point the tsadc may still be unclocked.
> 
> The clk is enable by default.
> 
> 
> The reason for this modification:
> 
> The otp Pin polarity setting for tsadc must be set when tsadc is turned off.
> 
> The order:
> 
> Close the tsadc->Set the otp pin polarity ->Set the pinctrl->initialize the
> tsadc->Open the tsadc
> 
> 
> As for the problem you mentioned, I guess: The default polarity of otp does
> not match the default state, that is, the otp is triggered by default, and
> then the reset circuit of the hardware takes effect and is restarted all the
> time.
> Modification:
> 1. For this hardware, otp pin default state is modified.
> 2. The mode of using CRU is rockchip,hw-tshut-mode = <0> in DTS;
> /* tshut mode 0:CRU 1:GPIO */
> 
> Recommended use method 2. You can try it.
> 
> >
> >>>>       error = clk_prepare_enable(thermal->clk);
> >>>>       if (error) {
> >>>>               dev_err(&pdev->dev, "failed to enable converter clock: %d\n",
> >>>>@@ -1267,6 +1272,30 @@ static int rockchip_thermal_probe(struct platform_device *pdev)
> >>>>       thermal->chip->initialize(thermal->grf, thermal->regs,
> >>>>                                 thermal->tshut_polarity);
> >>>>
> >>>>+     if (thermal->tshut_mode == TSHUT_MODE_GPIO) {
> >>>>+             thermal->pinctrl = devm_pinctrl_get(&pdev->dev);
> >>>>+             if (IS_ERR(thermal->pinctrl)) {
> >>>>+                     dev_err(&pdev->dev, "failed to find thermal pinctrl\n");
> >>>>+                     return PTR_ERR(thermal->pinctrl);
> >>>>+             }
> >>>>+
> >>>>+             thermal->gpio_state = pinctrl_lookup_state(thermal->pinctrl,
> >>>>+                                                        "gpio");
> >>Shouldn't this mode be documented properly in the binding first?
> >More importantly, it should be _backwards-compatible_, aka work with
> >old devicetrees without that property and not break thermal handling for
> >them entirely.
> If need  _backwards-compatible_,  It's can't return
> PTR_ERR(thermal->pinctrl) when get
> 
> devm_pinctrl_get failed.
> 
> >
> >>The binding [3] talks about init, default and sleep states but *not*
> >>gpio and otpout. The patch series looks incomplete to me or not using
> >>the proper names.
> >>
> >>[3] https://elixir.bootlin.com/linux/v5.2-rc1/source/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> >>
> >>>>+             if (IS_ERR_OR_NULL(thermal->gpio_state)) {
> >>>>+                     dev_err(&pdev->dev, "failed to find thermal gpio state\n");
> >>>>+                     return -EINVAL;
> >>>>+             }
> >>>>+
> >>>>+             thermal->otp_state = pinctrl_lookup_state(thermal->pinctrl,
> >>>>+                                                       "otpout");
> >>>>+             if (IS_ERR_OR_NULL(thermal->otp_state)) {
> >>>>+                     dev_err(&pdev->dev, "failed to find thermal otpout state\n");
> >>>>+                     return -EINVAL;
> >>>>+             }
> >>>>+
> >>Same here otpout is not a documented.
> >>
> >>As this change is now in mainline and is causing veyron to hang I'd
> >>suggest reverting this change for now. Even fixing the root cause
> >>(maybe the one I pointed above) after this patch we will have the
> >>thermal driver to fail because "gpio" and "otpout" states are not
> >>defined nor documented (a change on this will need some reviews and
> >>acks and time I guess).
> >I definitly agree here. Handling + checking the binding change
> >as well as needed fallback code is definitly not material for -rc-kernels
> >so we should just revert for now and let Elaine fix the issues for 5.3.
> >
> >Anyone volunteering for sending a revert-patch to Eduardo? :-)
> 
> I agree to revert the patch,and I will correct it and push it later.


Great! Collecting the revert that was already sent I will send out to
coming rc so we clear the breakage.

> 
> Do I need to commit the revert the patch now?@Heiko
> 


Yeah, you should see it in the next rc after I send this to Linus.

Meanwhile, it would be good if you good send another version of your
patch that does not break the other boards.

> >
> >Heiko
> >
> >
> >
> >
> >
> >
> >
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 1/3] thermal: rockchip: fix up the tsadc pinctrl setting error

[Heiko Stuebner]

Hi Elaine,

Am Donnerstag, 23. Mai 2019, 03:34:37 CEST schrieb elaine.zhang:
> hi, Heiko & Enric:
> 
> 在 2019/5/22 下午8:27, Heiko Stuebner 写道:
> > Hi Enric,
> >
> > Am Montag, 20. Mai 2019, 15:38:32 CEST schrieb Enric Balletbo Serra:
> >> Hi all,
> >>
> >> As pointed by [1] and [2] this commit, that now is upstream, breaks
> >> veyron (rk3288) and kevin (rk3399) boards. The problem is especially
> >> critical for veyron boards because they don't boot anymore.
> >>
> >> I didn't look deep at the problem but I have some concerns about this
> >> patch, see below.
> >>
> >> [1] https://www.spinics.net/lists/linux-rockchip/msg24657.html
> >> [2] https://www.spinics.net/lists/linux-rockchip/msg24735.html
> >>
> >> Missatge de Daniel Lezcano <daniel.lezcano@linaro.org> del dia dt., 30
> >> d’abr. 2019 a les 15:39:
> >>> On 30/04/2019 12:09, Elaine Zhang wrote:
> >>>> Explicitly use the pinctrl to set/unset the right mode
> >>>> instead of relying on the pinctrl init mode.
> >>>> And it requires setting the tshut polarity before select pinctrl.
> >>>>
> >>>> When the temperature sensor mode is set to 0, it will automatically
> >>>> reset the board via the Clock-Reset-Unit (CRU) if the over temperature
> >>>> threshold is reached. However, when the pinctrl initializes, it does a
> >>>> transition to "otp_out" which may lead the SoC restart all the time.
> >>>>
> >>>> "otp_out" IO may be connected to the RESET circuit on the hardware.
> >>>> If the IO is in the wrong state, it will trigger RESET.
> >>>> (similar to the effect of pressing the RESET button)
> >>>> which will cause the soc to restart all the time.
> >>>>
> >>>> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> >>> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>>
> >>>
> >>>
> >>>> ---
> >>>>   drivers/thermal/rockchip_thermal.c | 36 +++++++++++++++++++++++++++++++++---
> >>>>   1 file changed, 33 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/thermal/rockchip_thermal.c b/drivers/thermal/rockchip_thermal.c
> >>>> index 9c7643d62ed7..6dc7fc516abf 100644
> >>>> --- a/drivers/thermal/rockchip_thermal.c
> >>>> +++ b/drivers/thermal/rockchip_thermal.c
> >>>> @@ -172,6 +172,9 @@ struct rockchip_thermal_data {
> >>>>        int tshut_temp;
> >>>>        enum tshut_mode tshut_mode;
> >>>>        enum tshut_polarity tshut_polarity;
> >>>> +     struct pinctrl *pinctrl;
> >>>> +     struct pinctrl_state *gpio_state;
> >>>> +     struct pinctrl_state *otp_state;
> >>>>   };
> >>>>
> >>>>   /**
> >>>> @@ -1242,6 +1245,8 @@ static int rockchip_thermal_probe(struct platform_device *pdev)
> >>>>                return error;
> >>>>        }
> >>>>
> >>>> +     thermal->chip->control(thermal->regs, false);
> >>>> +
> >> That's the line that causes the hang. Commenting this makes the veyron
> >> boot again. Probably this needs to go after chip->initialize?
> > It needs to go after the clk_enable calls.
> > At this point the tsadc may still be unclocked.
> 
> The clk is enable by default.

Not necessarily. A driver needing a clock should not rely on it being
magically on :-) . Especially rk3288 devices were affected by this ...
see the number of people "complaining".

As you need to have the pclk enabled to access the tsadc registers
I'd suggest changing the control to something like

clk_prepare_enable(pclk)
->control(..., false)
clk_prepare_enable(tsadc)

So just swapping the clk-enable order (right now pclk only gets enabled
later as second clock)

> The reason for this modification:
> 
> The otp Pin polarity setting for tsadc must be set when tsadc is turned off.
> 
> The order:
> 
> Close the tsadc->Set the otp pin polarity ->Set the pinctrl->initialize 
> the tsadc->Open the tsadc

I'm still trying to understand why you need the additional pin-states
though. I.e. you introduce an additional gpio and otpout state, but
instead could just select the "init" state when needed?
(when the tsadc gets disabled)

Staying backwards compatible with existing devicetrees is really important
and changing the pinctrl requirements makes tsadc probe fail on these.

Heiko


> 
> 
> As for the problem you mentioned, I guess: The default polarity of otp 
> does not match the default state, that is, the otp is triggered by 
> default, and then the reset circuit of the hardware takes effect and is 
> restarted all the time.
> Modification:
> 1. For this hardware, otp pin default state is modified.
> 2. The mode of using CRU is rockchip,hw-tshut-mode = <0> in DTS;
> /* tshut mode 0:CRU 1:GPIO */
> 
> Recommended use method 2. You can try it.
> 
> >
> >>>>        error = clk_prepare_enable(thermal->clk);
> >>>>        if (error) {
> >>>>                dev_err(&pdev->dev, "failed to enable converter clock: %d\n",
> >>>> @@ -1267,6 +1272,30 @@ static int rockchip_thermal_probe(struct platform_device *pdev)
> >>>>        thermal->chip->initialize(thermal->grf, thermal->regs,
> >>>>                                  thermal->tshut_polarity);
> >>>>
> >>>> +     if (thermal->tshut_mode == TSHUT_MODE_GPIO) {
> >>>> +             thermal->pinctrl = devm_pinctrl_get(&pdev->dev);
> >>>> +             if (IS_ERR(thermal->pinctrl)) {
> >>>> +                     dev_err(&pdev->dev, "failed to find thermal pinctrl\n");
> >>>> +                     return PTR_ERR(thermal->pinctrl);
> >>>> +             }
> >>>> +
> >>>> +             thermal->gpio_state = pinctrl_lookup_state(thermal->pinctrl,
> >>>> +                                                        "gpio");
> >> Shouldn't this mode be documented properly in the binding first?
> > More importantly, it should be _backwards-compatible_, aka work with
> > old devicetrees without that property and not break thermal handling for
> > them entirely.
> If need  _backwards-compatible_,  It's can't return 
> PTR_ERR(thermal->pinctrl) when get
> 
> devm_pinctrl_get failed.
> 
> >
> >> The binding [3] talks about init, default and sleep states but *not*
> >> gpio and otpout. The patch series looks incomplete to me or not using
> >> the proper names.
> >>
> >> [3] https://elixir.bootlin.com/linux/v5.2-rc1/source/Documentation/devicetree/bindings/thermal/rockchip-thermal.txt
> >>
> >>>> +             if (IS_ERR_OR_NULL(thermal->gpio_state)) {
> >>>> +                     dev_err(&pdev->dev, "failed to find thermal gpio state\n");
> >>>> +                     return -EINVAL;
> >>>> +             }
> >>>> +
> >>>> +             thermal->otp_state = pinctrl_lookup_state(thermal->pinctrl,
> >>>> +                                                       "otpout");
> >>>> +             if (IS_ERR_OR_NULL(thermal->otp_state)) {
> >>>> +                     dev_err(&pdev->dev, "failed to find thermal otpout state\n");
> >>>> +                     return -EINVAL;
> >>>> +             }
> >>>> +
> >> Same here otpout is not a documented.
> >>
> >> As this change is now in mainline and is causing veyron to hang I'd
> >> suggest reverting this change for now. Even fixing the root cause
> >> (maybe the one I pointed above) after this patch we will have the
> >> thermal driver to fail because "gpio" and "otpout" states are not
> >> defined nor documented (a change on this will need some reviews and
> >> acks and time I guess).
> > I definitly agree here. Handling + checking the binding change
> > as well as needed fallback code is definitly not material for -rc-kernels
> > so we should just revert for now and let Elaine fix the issues for 5.3.
> >
> > Anyone volunteering for sending a revert-patch to Eduardo? :-)
> 
> I agree to revert the patch,and I will correct it and push it later.
> 
> Do I need to commit the revert the patch now?@Heiko





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel