> -----Original Message----- > From: Marco Felsch <m.felsch@xxxxxxxxxxxxxx> > Sent: Wednesday, August 24, 2022 5:06 PM > To: Alice Guo (OSS) <alice.guo@xxxxxxxxxxx> > Cc: Guenter Roeck <linux@xxxxxxxxxxxx>; wim@xxxxxxxxxxxxxxxxxx; > shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > dl-linux-imx <linux-imx@xxxxxxx>; kernel@xxxxxxxxxxxxxx; > linux-watchdog@xxxxxxxxxxxxxxx > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory barrier for > unlock sequence > > Hi Alice, > > On 22-08-24, Alice Guo (OSS) wrote: > > > -----Original Message----- > > > From: Marco Felsch <m.felsch@xxxxxxxxxxxxxx> > > > Sent: Wednesday, August 24, 2022 4:04 PM > > > To: Alice Guo (OSS) <alice.guo@xxxxxxxxxxx> > > > Cc: Guenter Roeck <linux@xxxxxxxxxxxx>; wim@xxxxxxxxxxxxxxxxxx; > > > shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx; festevam@xxxxxxxxx; > > > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > > > dl-linux-imx <linux-imx@xxxxxxx>; kernel@xxxxxxxxxxxxxx; > > > linux-watchdog@xxxxxxxxxxxxxxx > > > Subject: Re: [PATCH 2/7] watchdog: imx7ulp: Add explict memory > > > barrier for unlock sequence > > > > > > Hi Alice, > > > > > > On 22-08-24, Alice Guo (OSS) wrote: > > > > > > ... > > > > > > > > > > Hi Guenter and Marco, > > > > > > > > > > > > > > 1. did you see any issues? > > > > > > > This WDOG Timer first appeared in i.MX7ULP, no one report > > > > > > > issues probably because few people use i.MX7ULP. This issue > > > > > > > was found when we did a stress test on it. When we > > > > > > > reconfigure the WDOG Timer, there is a certain probability > > > > > > > that it reset. The reason for the error is that when > > > > > > > WDOG_CS[CMD32EN] is 0, the unlock sequence is two 16-bit > > > > > > > writes (0xC520, 0xD928) to the CNT register within 16 bus > > > > > > > clocks, and improper unlock sequence causes the > > > WDOG to reset. > > > > > > > Adding mb() is to guarantee that two 16-bit writes are > > > > > > > finished within 16 > > > > > bus clocks. > > > > > > > > > > > > After this explanation the whole imx7ulp_wdt_init() seems a > > > > > > bit buggy because writel_relaxed() as well as writel() are > > > > > > 32bit access > > > functions. > > > > > > So the very first thing to do is to enable the 32-bit mode. > > > > > > > > > > > Agreed. This is much better than having extra code to deal with > > > > > both 16-bit and 32-bit access. > > > > > > > > > > > Also this is a explanation worth to be added to the commit > > > > > > message > > > > > > ;) > > > > > > > > > > > > > > > > Definitely. Also, the use of mb(), if it should indeed be > > > > > needed, would have to be explained in a code comment. > > > > > > > > > > Thanks, > > > > > Guenter > > > > > > > > Hi Marco and Guenter, > > > > > > > > Thank you for your comments. I plan to enable support for 32-bit > > > > unlock command write words in bootloader. In this way, there is no > > > > need to distinguish whether the unlock command is a 32-bit command > > > > or a 16-bit command in driver. > > > > > > Please don't move this into the bootloader, enabling it within the > > > init seq. is just fine. If you move it into the bootloader then you > > > can't ensure that the bit is set since there are plenty of bootloaders out > there. > > > > > > As I said, just drop the "16bit" unlock sequence from the init > > > function because the unlock is handled just fine in all the watchdog_ops. > > > > > > Regards, > > > Marco > > > > Hi Marco, > > > > Sorry, I did not tell you that all watchdog control bits, timeout > > value, and window value cannot be set until the watchdog is unlocked. > > You don't have to according the RM: > 8<---------------------------------------------------------------------- > 59.5.2 Disable Watchdog after Reset > > All of watchdog registers are unlocked by reset. Therefore, unlock sequence is > unnecessary, but it needs to write all of watchdog registers to make the new > configuration take effect. The code snippet below shows an example of > disabling watchdog after reset. > 8<---------------------------------------------------------------------- > > also the RM tells us: > 8<---------------------------------------------------------------------- > 59.4.3.1 Configuring the Watchdog Once > > The new configuration takes effect only after all registers except CNT are > written after reset. Otherwise, the WDOG uses the reset values by default. If > window mode is not used (CS[WIN] is 0), writing to WIN is not required to > make the new configuration take effect. > 8<---------------------------------------------------------------------- > > > Support for 32-bit unlock command write words in enabled in > > imx7ulp_wdt_init now. > > So.. after reading the IMX7ULP RM, which was not my intention, I found out > that most of the WDOG_CS regiter bits are write-once bits. This means if you > didn't set it within the bootloader you still in case "59.4.3.1". > > So the imx7ulp_wdt_init() function just needs to check if the > WDOG_CS_UPDATE bit was set. If it is not the case, then you need to write the > WDOG_CS register as currently done. If the bit is set, than you need know that > the bootloader did the job for you and you can exit > imx7ulp_wdt_init() early. In both cases the unlock is not required. > > Can you please check/test if this is working for you? > > Regards, > Marco > Hi Marco, Rom code has already configured the WDOG once, so we cannot use " Configuring the Watchdog Once". Best Regards, Alice Guo
