On 01/10/15 07:11 PM, Fabio Estevam wrote:
On Thu, Oct 1, 2015 at 5:50 PM, Wolfgang Denk <w...@denx.de> wrote:On ARM (a LE architecture), clrsetbits_le16() maps down into: clrsetbits_le16 -> out_le16 / in_le16 -> out_arch, w,le16 / in_arch, w,le16 -> __raw_writew(cpu_to_le16()) / le16__to_cpu(__raw_readw()) -> __raw_writew() / __raw_readw() while writew() -> __raw_writew(cpu_to_le16(v),__mem_pci(c)) __raw_writew() Both map into __raw_writew() [which then boild down into __arch_putw() which is just a volatile unsigned short write access. So both clrsetbits_le16() and writew() are little endian accessors. In which way would one write other data to the device than the other?Yes, you are right. The issue seems to be related to the effect of writing doing 'writew(WCR_WDE, &wdog->wcr)', which writes 0x40 to the register WCR versus 'clrsetbits_le16(&wdog->wcr, 0, WCR_WDE)' which only enables the WDE bit.
Unfortunately, I believe this is not exactly true. After the revert with writew(WCR_WDE, &wdog->wcr) we are not really writing 0x4 or setting WDE but we are writing 0x0400 and setting the WT bits (wcr[8:15] to 0x04 and this is accidentally also clearing the SRS bit. In addition, even if it was setting the WDE correctly it wouldn't be relevant to ls1021atwr as we are not setting the timeout. Bottom line is the code is broken for ls1021 both before and after the revert and it just happens that the broken code after the revert (with writew) clears a bit we didn't intend to and that generates a wdog_rst so it hides the real bug. The correct implementation would be clearing the SRS bit via an _be16 operation. Anyhow, let's move on with this revert if you all agree with this. Fabio, I will send you the test-by to your commit but we will have to clean up this mini mess soon after :) Thanks Sinan Akman
The kernel driver also does the full write to the WCR register(like we used to have prior to 623d96e89aca6) https://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/tree/drivers/watchdog/imx2_wdt.c?id=refs/tags/v4.2.2#n86 This has also the effect of clearing the SRS bit. By the way we need to implement erratum ERR004346 (WDOG: WDOG SRS bit requires to be written twice) in U-boot, but this is an unrelated issue. So the revert seems to be appropriate. The commit log should be adjusted though. Regards, Fabio Estevam _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
