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
