linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Enric Balletbo Serra <eballetbo@gmail.com>
To: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: "Mark Rutland" <mark.rutland@arm.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"Doug Anderson" <dianders@chromium.org>,
	huangtao@rock-chips.com, "Heiko Stübner" <heiko@sntech.de>,
	"Linux PM list" <linux-pm@vger.kernel.org>,
	xxx@rock-chips.com, "Elaine Zhang" <zhangqing@rock-chips.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	vicencb@gmail.com, xf@rock-chips.com,
	"Eduardo Valentin" <edubezval@gmail.com>,
	"open list:ARM/Rockchip SoC..."
	<linux-rockchip@lists.infradead.org>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Zhang Rui" <rui.zhang@intel.com>,
	"Linux ARM" <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v3 1/3] thermal: rockchip: fix up the tsadc pinctrl setting error
Date: Mon, 20 May 2019 15:38:32 +0200	[thread overview]
Message-ID: <CAFqH_53nbiwzQKctNa7MBzgCcsRFn1p8g31Xgvo3E9k6eA8AKw@mail.gmail.com> (raw)
In-Reply-To: <785392a0-282a-1e51-a4d6-a6d5ca478949@linaro.org>

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

  reply	other threads:[~2019-05-20 13:38 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-30 10:09 [PATCH v3 0/3] thermal: rockchip: fix up thermal driver Elaine Zhang
2019-04-30 10:09 ` [PATCH v3 1/3] thermal: rockchip: fix up the tsadc pinctrl setting error Elaine Zhang
2019-04-30 13:38   ` Daniel Lezcano
2019-05-20 13:38     ` Enric Balletbo Serra [this message]
2019-05-22 12:27       ` Heiko Stuebner
2019-05-22 12:30         ` Daniel Lezcano
2019-05-22 12:34           ` Heiko Stuebner
2019-05-23  1:34         ` elaine.zhang
2019-05-24  2:23           ` Eduardo Valentin
2019-05-24  6:45           ` Heiko Stuebner
2019-05-22 12:20   ` Daniel Lezcano
2019-04-30 10:09 ` [PATCH v3 2/3] dt-bindings: rockchip-thermal: Support the PX30 SoC compatible Elaine Zhang
2019-04-30 10:09 ` [PATCH v3 3/3] thermal: rockchip: Support the PX30 SoC in thermal driver Elaine Zhang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAFqH_53nbiwzQKctNa7MBzgCcsRFn1p8g31Xgvo3E9k6eA8AKw@mail.gmail.com \
    --to=eballetbo@gmail.com \
    --cc=daniel.lezcano@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dianders@chromium.org \
    --cc=edubezval@gmail.com \
    --cc=heiko@sntech.de \
    --cc=huangtao@rock-chips.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=mark.rutland@arm.com \
    --cc=robh+dt@kernel.org \
    --cc=rui.zhang@intel.com \
    --cc=vicencb@gmail.com \
    --cc=xf@rock-chips.com \
    --cc=xxx@rock-chips.com \
    --cc=zhangqing@rock-chips.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).