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@xxxxxxxxxx> 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@xxxxxxxxxxxxxx> > > > > Reviewed-by: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx> > > > > > > > > > --- > > > 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
