| Message ID | 1443727970-10347-1-git-send-email-festevam@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On 01/10/15 03:32 PM, Fabio Estevam wrote: > From: Fabio Estevam <fabio.estevam@freescale.com> > > This reverts commit 623d96e89aca64c2762150087f4e872c55481f13. > > commit 623d96e89aca6("imx: wdog: correct wcr register settings") > introduced the usage of clrsetbits_le16(), which causes a regression > on LS1021 systems. > > Unlike i.MX, LS1021 uses big-endian ordering for the watchdog > controller, which means we cannot use the little endian accessors. > > Reported-by: Sinan Akman <sinan@writeme.com> > Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com> > --- > drivers/watchdog/imx_watchdog.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/watchdog/imx_watchdog.c b/drivers/watchdog/imx_watchdog.c > index 9a77a54..1d18d4b 100644 > --- a/drivers/watchdog/imx_watchdog.c > +++ b/drivers/watchdog/imx_watchdog.c > @@ -55,8 +55,7 @@ void reset_cpu(ulong addr) > { > struct watchdog_regs *wdog = (struct watchdog_regs *)WDOG1_BASE_ADDR; > > - clrsetbits_le16(&wdog->wcr, 0, WCR_WDE); > - > + writew(WCR_WDE, &wdog->wcr); > writew(0x5555, &wdog->wsr); > writew(0xaaaa, &wdog->wsr); /* load minimum 1/2 second timeout */ > while (1) { Hi Fabio, I just wanted to point out that with this revert we don't only break imx again (whatever the initial bug was for this commit) but also we are having ls1021atwr working accidentally, just because writew(WCR_WDE, &wdog->wcr) clears SRS bit which is the only requirement for reset if watchdog is not running. I don't have any strong opinion on this but i just wanted to make it clear that we are leaving both imx6 and ls1021atwr not properly implemented. Regards Sinan Akman
Hi Sinan, On Thu, Oct 1, 2015 at 4:39 PM, Sinan Akman <sinan@writeme.com> wrote: > Hi Fabio, I just wanted to point out that with this revert we don't only > break imx again We are not breaking imx by doing the revert. The reset still works. 623d96e89aca64c2 appeared only in 2015.10-rc4. > (whatever the initial bug was for this commit) but also we are having > ls1021atwr > working accidentally, just because writew(WCR_WDE, &wdog->wcr) clears SRS > bit which is the only requirement for reset if watchdog is not running. > > I don't have any strong opinion on this but i just wanted to make it clear > that > we are leaving both imx6 and ls1021atwr not properly implemented. I think it is the best/safest we can do at -rc4 to avoid the reset regression on ls1021. Then a proper implementation can be done for 2016.01. Do you agree? Regards, Fabio Estevam
On 01/10/15 03:45 PM, Fabio Estevam wrote: > Hi Sinan, > > On Thu, Oct 1, 2015 at 4:39 PM, Sinan Akman <sinan@writeme.com> wrote: > >> Hi Fabio, I just wanted to point out that with this revert we don't only >> break imx again > We are not breaking imx by doing the revert. The reset still works. > 623d96e89aca64c2 appeared only in 2015.10-rc4. > >> (whatever the initial bug was for this commit) but also we are having >> ls1021atwr >> working accidentally, just because writew(WCR_WDE, &wdog->wcr) clears SRS >> bit which is the only requirement for reset if watchdog is not running. >> >> I don't have any strong opinion on this but i just wanted to make it clear >> that >> we are leaving both imx6 and ls1021atwr not properly implemented. > I think it is the best/safest we can do at -rc4 to avoid the reset > regression on ls1021. > > Then a proper implementation can be done for 2016.01. > > Do you agree? Hi Fabio, yes this seems to be the best thing to do for now. Let's implement then this thing properly soon after. Thanks Sinan Akman
On Thu, Oct 1, 2015 at 4:50 PM, Sinan Akman <sinan@writeme.com> wrote: > Hi Fabio, yes this seems to be the best thing to do for now. > Let's implement then this thing properly soon after. Could you please reply with a Tested-by tag? Thanks
Dear Fabio, In message <1443727970-10347-1-git-send-email-festevam@gmail.com> you wrote: > > Unlike i.MX, LS1021 uses big-endian ordering for the watchdog > controller, which means we cannot use the little endian accessors. ... > - clrsetbits_le16(&wdog->wcr, 0, WCR_WDE); > - > + writew(WCR_WDE, &wdog->wcr); I'm sorry, but I fail to understand how writew() can be better than another I/O accessor. Neither of these has the capability to detect the endianess of this specific register interface ? Best regards, Wolfgang Denk
Hi Wolfgang, On Thu, Oct 1, 2015 at 5:11 PM, Wolfgang Denk <wd@denx.de> wrote: > I'm sorry, but I fail to understand how writew() can be better than > another I/O accessor. Neither of these has the capability to detect > the endianess of this specific register interface ? It's not that writew() is better. The problem is that we cannot use clrsetbits_le16() for a big endian controller. Regards, Fabio Estevam
Dear Fabio, In message <CAOMZO5BEfS10XVztnigMejMVJYLvv+jqDLZYom9K8-G+Zi1TXA@mail.gmail.com> you wrote: > > > I'm sorry, but I fail to understand how writew() can be better than > > another I/O accessor. Neither of these has the capability to detect > > the endianess of this specific register interface ? > > It's not that writew() is better. The problem is that we cannot use > clrsetbits_le16() for a big endian controller. 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? Best regards, Wolfgang Denk
On Thu, Oct 1, 2015 at 5:50 PM, Wolfgang Denk <wd@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. 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
On 01/10/15 07:11 PM, Fabio Estevam wrote: > On Thu, Oct 1, 2015 at 5:50 PM, Wolfgang Denk <wd@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 >
Dear Fabio, In message <CAOMZO5CAQXVzpX9mW7uHKB98JnMBeuT3ZjiUe4-V4=adjRk0zg@mail.gmail.com> you wrote: > > > 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. But if we agree that both are LE accessors, and that the register is BE, then how does it work at all - we would be writing the wrong bit? Best regards, Wolfgang Denk
On Fri, Oct 2, 2015 at 1:30 AM, Wolfgang Denk <wd@denx.de> wrote: > In message <CAOMZO5CAQXVzpX9mW7uHKB98JnMBeuT3ZjiUe4-V4=adjRk0zg@mail.gmail.com> you > But if we agree that both are LE accessors, and that the register is > BE, then how does it work at all - we would be writing the wrong bit? Watchdog on LS1021 works by accident rather than by design. What we are trying to do is to avoid the regression on LS1021 for the 2015.10 release. Then a proper watchdog driver implementation is needed for 2016.01 so that it takes care of the endianness. Is this approach acceptable? Regards, Fabio Estevam
diff --git a/drivers/watchdog/imx_watchdog.c b/drivers/watchdog/imx_watchdog.c index 9a77a54..1d18d4b 100644 --- a/drivers/watchdog/imx_watchdog.c +++ b/drivers/watchdog/imx_watchdog.c @@ -55,8 +55,7 @@ void reset_cpu(ulong addr) { struct watchdog_regs *wdog = (struct watchdog_regs *)WDOG1_BASE_ADDR; - clrsetbits_le16(&wdog->wcr, 0, WCR_WDE); - + writew(WCR_WDE, &wdog->wcr); writew(0x5555, &wdog->wsr); writew(0xaaaa, &wdog->wsr); /* load minimum 1/2 second timeout */ while (1) {