From: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
To: Vladimir Zapolskiy <vz@mleia.com>
Cc: Shawn Guo <shawnguo@kernel.org>, Wim Van Sebroeck <wim@iguana.be>,
Guenter Roeck <linux@roeck-us.net>,
Fabio Estevam <fabio.estevam@nxp.com>,
Sascha Hauer <kernel@pengutronix.de>,
Magnus Lilja <lilja.magnus@gmail.com>,
linux-watchdog@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs
Date: Sun, 11 Dec 2016 11:26:34 +0100 [thread overview]
Message-ID: <20161211102634.jn3awmlzdrm3pqai@pengutronix.de> (raw)
In-Reply-To: <d6d987f1-3461-214a-0ca1-16d310ddd370@mleia.com>
Hello Vladimir,
On Sun, Dec 11, 2016 at 12:01:08PM +0200, Vladimir Zapolskiy wrote:
> On 12/11/2016 11:35 AM, Uwe Kleine-König wrote:
> > On Sun, Dec 11, 2016 at 03:06:48AM +0200, Vladimir Zapolskiy wrote:
> >> Power down counter enable/disable bit switch is located in WMCR
> >> register, but watchdog controllers found on legacy i.MX21, i.MX27 and
> >> i.MX31 SoCs don't have this register. As a result of writing data to
> >> the non-existing register on driver probe the SoC hangs up, to fix the
> >> problem add more OF compatible strings and on this basis get
> >> information about availability of the WMCR register.
> >>
> >> Fixes: 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
> >> Signed-off-by: Vladimir Zapolskiy <vz@mleia.com>
> >> ---
>
> [snip]
>
> >>
> >> static const struct of_device_id imx2_wdt_dt_ids[] = {
> >> - { .compatible = "fsl,imx21-wdt", },
> >> + { .compatible = "fsl,imx21-wdt", .data = IMX2_WDT_NO_WMCR },
> >> + { .compatible = "fsl,imx25-wdt", },
> >> + { .compatible = "fsl,imx27-wdt", .data = IMX2_WDT_NO_WMCR },
> >> + { .compatible = "fsl,imx31-wdt", .data = IMX2_WDT_NO_WMCR },
> >> + { .compatible = "fsl,imx35-wdt", },
> >> + { .compatible = "fsl,imx50-wdt", },
> >> + { .compatible = "fsl,imx51-wdt", },
> >> + { .compatible = "fsl,imx53-wdt", },
> >> + { .compatible = "fsl,imx6q-wdt", },
> >> + { .compatible = "fsl,imx6sl-wdt", },
> >> + { .compatible = "fsl,imx6sx-wdt", },
> >> + { .compatible = "fsl,imx6ul-wdt", },
> >> + { .compatible = "fsl,imx7d-wdt", },
> >> + { .compatible = "fsl,vf610-wdt", },
> >
> > Up to now we only had fsl,imx21-wdt, now it seems that iMX31, i.MX27 and
> > i.MX21 form a group of equal watchdog IPs and the others form another
> > group, right?
> >
> > So conceptually we should differenciate between
> >
> > fsl,imx21-wdt (which is used on i.MX21, i.MX27 and i.MX31)
> > fsl,imx35-wdt (which is used on all others)
> >
>
> The problem is that "fsl,imx35-wdt" is not used on all others in the
> existing DTS, i.e. in the old firmware, which shall be compatible with
> new changes.
So old i.MX53 has
compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
. With the change to the driver it's like using the watchdog driver in
it's pre-5fe65ce7ccbb4-state on it. If this is bad, we must indeed make
fsl,imx53-wdt known to the driver. If not, just adding a single new
compatible to the driver is just fine.
If you want to call it imx25 or imx35 doesn't matter much. The reason I
picked imx35 is that this one was already picked before for cspi. This
suggests that i.MX35 is older than i.MX25 and so this is the canonical
choice.
> $ grep -H 'fsl,imx.*-wdt' arch/arm/boot/dts/*.dtsi
> arch/arm/boot/dts/imx25.dtsi: compatible = "fsl,imx25-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx27.dtsi: compatible = "fsl,imx27-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx35.dtsi: compatible = "fsl,imx35-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx50.dtsi: compatible = "fsl,imx50-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx51.dtsi: compatible = "fsl,imx51-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx53.dtsi: compatible = "fsl,imx53-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6qdl.dtsi: compatible = "fsl,imx6q-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6sl.dtsi: compatible = "fsl,imx6sl-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6sx.dtsi: compatible = "fsl,imx6sx-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx6ul.dtsi: compatible = "fsl,imx6ul-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/imx7s.dtsi: compatible = "fsl,imx7d-wdt", "fsl,imx21-wdt";
> arch/arm/boot/dts/ls1021a.dtsi: compatible = "fsl,imx21-wdt";
> arch/arm/boot/dts/vfxxx.dtsi: compatible = "fsl,vf610-wdt", "fsl,imx21-wdt";
>
> I don't see a confirmation of the fact that "fsl,imx35-wdt" is the best
> selection, hence definitely "fsl,imx25-wdt" overscores it.
I stated my reason to pick imx35 over imx25 above. Why do yo prefer
imx25?
> > A reason to add e.g. fsl,imx6sl-wdt is that dtbs in the wild use
> >
> > "fsl,imx6sl-wdt", "fsl,imx21-wdt"
> >
> > . But then I wonder if
> >
> > 5fe65ce7ccbb ("watchdog: imx2_wdt: Disable power down counter on boot")
> >
> > is that important to justify to add these all.
>
> About this comment I don't know, you may have better insight about the original
> change. By the way, in barebox you may want to fix the same hang-up, because
> you touch WMCR unconditionally.
Sure.
> > Independant of this I'd like to have all dtsi for the newer SoCs changed
> > to get
> >
> > "fsl,imx6sl-wdt", "fsl,imx35-wdt"
> >
> > and if you really want to add all these compatibles, add a comment that
> > these are really fsl,imx35-wdt and were added to work around broken
> > dtbs.
>
> No objections, but
>
> 1) I'd like to see "fsl,imx25-wdt" as a new common compatible, and that's
> what have been proposed and shared in RFC 2/2.
Yeah, I missed that other patch (which was not part of this series).
> 2) Please remind me, why do you ask to drop "fsl,imx21-wdt" from the list?
>
> compatible = "fsl,imx6sx-wdt", "fsl,imx25-wdt", "fsl,imx21-wdt";
>
> The list of compatibles above is valid (from the most specific on the left
> to the most generic on the right), and that compatible property is rightly
> handled in the driver with this change applied. I don't see a need to
> drop "fsl,imx21-wdt".
If the wdt in the i.MX6SX can be operated by the watchdog driver in it's
imx21 mode, you should keep the imx21 entry. If not, you shouldn't.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
next prev parent reply other threads:[~2016-12-11 10:26 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-11 1:06 [PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs Vladimir Zapolskiy
2016-12-11 2:18 ` Guenter Roeck
2016-12-11 8:58 ` Magnus Lilja
2016-12-11 9:35 ` Uwe Kleine-König
2016-12-11 10:01 ` Vladimir Zapolskiy
2016-12-11 10:26 ` Uwe Kleine-König [this message]
2016-12-11 11:21 ` Vladimir Zapolskiy
2016-12-11 12:28 ` Uwe Kleine-König
2016-12-23 1:55 ` Guenter Roeck
2016-12-23 8:20 ` Vladimir Zapolskiy
2016-12-23 8:32 ` Uwe Kleine-König
2016-12-23 9:27 ` Vladimir Zapolskiy
2016-12-23 10:01 ` Uwe Kleine-König
2016-12-23 11:21 ` Vladimir Zapolskiy
2016-12-23 14:11 ` Uwe Kleine-König
2016-12-24 5:08 ` Vladimir Zapolskiy
2016-12-28 20:54 ` Uwe Kleine-König
2016-12-28 21:33 ` Fabio Estevam
2016-12-28 21:46 ` Uwe Kleine-König
2016-12-28 23:39 ` Fabio Estevam
2016-12-29 22:04 ` Uwe Kleine-König
2016-12-29 23:40 ` Fabio Estevam
2016-12-24 13:00 ` Fabio Estevam
2016-12-28 20:38 ` Uwe Kleine-König
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=20161211102634.jn3awmlzdrm3pqai@pengutronix.de \
--to=u.kleine-koenig@pengutronix.de \
--cc=fabio.estevam@nxp.com \
--cc=kernel@pengutronix.de \
--cc=lilja.magnus@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=shawnguo@kernel.org \
--cc=vz@mleia.com \
--cc=wim@iguana.be \
/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).