On 14/05/15 09:44, Paul Burton wrote: > On Wed, May 13, 2015 at 11:19:56PM +0100, James Hogan wrote: >> Hi Maciej, >> >> On Wed, May 13, 2015 at 07:03:51PM +0100, Maciej W. Rozycki wrote: >>> On Wed, 13 May 2015, James Hogan wrote: >>> >>>> On Malta, the RTC is forced into binary coded decimal (BCD) mode during >>>> init, even if the bootloader put it into binary mode (as YAMON does). >>>> This can result in the RTC seconds being an invalid BCD (e.g. >>>> 0x1a..0x1f) for up to 6 seconds. >>> >>> Sigh. No sooner I had fixed the breakage (with 636221b8 and a fat >>> comment) it got put back (with a87ea88d). Even though it's easily >>> spotted as it breaks the system time (all the fields, including the date >>> too, not only the seconds!) across a reboot due to YAMON eagerly >>> switching the mode back. And that'd be the first item I'd check when >>> validating a change touching the RTC. >> >> Indeed, a quick bit of experimentation confirms a discrepancy before >> this patch is applied before YAMON's "date" command and the RTC clock as >> read by Linux (with year, day of month, hour & minute all going 22 -> 16 >> (22 == 0x16), and presumably different rates of time depending on which >> mode its in. After this patch it appears to work again as it should. >> >>> Is there an actual need to reinitialise the RTC at all? The RTC >>> registers are readable, so the current configuration can be obtained, >>> the RTC driver copes with any valid arrangement, so can any other code >>> using the clock as a reference. >>> >>> YAMON OTOH is not as flexible, its clock management commands expect the >>> format the monitor itself set the chip to, so I think the kernel has to >>> respect that (just as it doesn't randomly flip bits in the RTC on x86 >>> PCs for example). >>> >>> So unless proven otherwise I'll ask for `init_rtc' to be dropped >>> altogether >> >> I suspect it comes down to what U-Boot does with RTC (possibly very >> little), but I'll leave that to you and Paul to discuss. > > The reason for init_rtc was touched upon in the commit message for > a87ea88d8f6c (MIPS: Malta: initialise the RTC at boot) but could perhaps > have been clearer. Straight out of reset the RTC may not be running at > all, but the code in estimate_frequencies (note: not the RTC driver) > relies upon the RTC having been started. This was an issue when using > earlier builds of U-boot which didn't touch the RTC at all, and is still > an issue if you do something like load the kernel via a JTAG probe > rather than using U-boot or YAMON. If the kernel doesn't ensure the RTC > is started then it'll just hang in estimate_frequencies waiting for the > UIP bit to change even though it never will. YAMON isn't the only way to > boot on Malta, and dependencies such as this between the kernel & the > bootloader should really be minimised. > >>> and any changes required made to `estimate_frequencies' >>> instead. Which I believe you already did with 2/2. >> >> As long as the mode doesn't get changed, my change to >> estimate_frequencies() should be happy enough. Before patch 2/2 it only >> reads UIP flag to try to time a single second, so it shouldn't have >> cared about such details as the RTC mode. > > ...and since it seems the U-boot & YAMON RTC drivers use it in different > modes, I'd consider this patch reasonable. Feel free to have my: > > Reviewed-by: Paul Burton <paul.burton@xxxxxxxxxx> Thanks Paul, Assuming Maciej is satisfied I'll rewrite the commit message, add a stable tag, and resend. Cheers James
Attachment:
signature.asc
Description: OpenPGP digital signature
