Re: [PATCH 1/2] MIPS: malta-time: Don't switch RTC to BCD mode

[James Hogan <james.hogan@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 3635 bytes --]

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@imgtec.com>

Thanks Paul,

Assuming Maciej is satisfied I'll rewrite the commit message, add a
stable tag, and resend.

Cheers
James


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]