[PATCH v1] watchdog: imx2: fix hang-up on boot for i.MX21, i.MX27 and i.MX31 SoCs

[Guenter Roeck]

On 12/11/2016 03:21 AM, Vladimir Zapolskiy wrote:
> On 12/11/2016 12:26 PM, Uwe Kleine-König wrote:
>> 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 at mleia.com>
>>>>> ---
>>>


What is the ultimate conclusion of this exchange ?

Are we going to get another version of the patch, or did everyone agree that
the patch is good as it is and does not require further changes ?

Guenter

>>> [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.
>
> The change under discussion preserves the current runtime behaviour for
> i.MX53 and its friends, the watchdog power down counter in WMCR is
> disabled on boot (you may want to confirm it by testing though), another
> question is if it is wanted for e.g. i.MX53 right from the beginning.
>
> To keep the runtime compatibility of a newer kernel with old DTBs such
> a long list of device compatibles has to be inserted.
>
>>
>> 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.
>
> Accepted, from the Wikipedia both i.MX25 and i.MX35 are released in 2009,
> which one is the first I don't know for sure, I supposed it could be
> i.MX25 as a SoC with a weaker core.
>
>>
>>> $ 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?
>
> As you said right now and until it is added into DTS files "fsl,imx25-wdt" and
> "fsl,imx35-wdt" are equal as a compatible representation of WMCR-aware watchdogs.
>
> I may be wrong, if I assume that i.MX25 is released before i.MX35 (looks like
> they are released in parallel), but "imx25" precedes in alphanumerically sorted
> list of SoC names.
>
>>>> 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.
>
> The DTS description shall be rather independent from any particular software
> implementation, someone may want to reuse the same DTB in bootloaders and OS
> kernels of different versions. Assuming that I have a bootloader/kernel
> with a pure i.MX21 watchdog driver and run it on i.MX6SX, I'd prefer to match
> "fsl,imx21-wdt" compatible.
>
> --
> With best wishes,
> Vladimir
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>