[PATCH 0/2] MIPS: malta-time: Take seconds into account

[PATCH 0/2] MIPS: malta-time: Take seconds into account

[James Hogan]

This patchset improves the robustness of the Malta timer frequencies
calculation, particularly under virtualisation on slow targets, where
the RTC UIP flag may not get noticed the first time.

James Hogan (2):
  MIPS: malta-time: Don't switch RTC to BCD mode
  MIPS: malta-time: Take seconds into account

 arch/mips/mti-malta/malta-time.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Paul Burton <paul.burton@imgtec.com>
Cc: Steven J. Hill <Steven.Hill@imgtec.com>
Cc: linux-mips@linux-mips.org
-- 
2.3.6

[PATCH 0/2] MIPS: malta-time: Take seconds into account

[James Hogan]

This patchset improves the robustness of the Malta timer frequencies
calculation, particularly under virtualisation on slow targets, where
the RTC UIP flag may not get noticed the first time.

James Hogan (2):
  MIPS: malta-time: Don't switch RTC to BCD mode
  MIPS: malta-time: Take seconds into account

 arch/mips/mti-malta/malta-time.c | 25 ++++++++++++++++++++++---
 1 file changed, 22 insertions(+), 3 deletions(-)

Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Paul Burton <paul.burton@imgtec.com>
Cc: Steven J. Hill <Steven.Hill@imgtec.com>
Cc: linux-mips@linux-mips.org
-- 
2.3.6

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

[James Hogan]

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.

In a future patch we want to take seconds into account when estimating
the frequency, however that would treat a transition from the invalid
BCD 0x1f (calculated as 25) to BCD 0x20 (20) as -5 seconds, which is
wrapped to 55 seconds and results in a very underestimated frequency.

Therefore read-modify-write the control register so that the previous
mode is preserved instead of being forced into BCD mode. This avoids the
possibility of invalid BCD values immediately afterwards.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Paul Burton <paul.burton@imgtec.com>
Cc: linux-mips@linux-mips.org
---
 arch/mips/mti-malta/malta-time.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/mips/mti-malta/malta-time.c b/arch/mips/mti-malta/malta-time.c
index 185e68261f45..a030d41eb5a1 100644
--- a/arch/mips/mti-malta/malta-time.c
+++ b/arch/mips/mti-malta/malta-time.c
@@ -165,14 +165,18 @@ unsigned int get_c0_compare_int(void)
 
 static void __init init_rtc(void)
 {
+	unsigned char ctrl = CMOS_READ(RTC_CONTROL);
+
 	/* stop the clock whilst setting it up */
-	CMOS_WRITE(RTC_SET | RTC_24H, RTC_CONTROL);
+	ctrl |= RTC_24H | RTC_SET;
+	CMOS_WRITE(ctrl, RTC_CONTROL);
 
 	/* 32KHz time base */
 	CMOS_WRITE(RTC_REF_CLCK_32KHZ, RTC_FREQ_SELECT);
 
 	/* start the clock */
-	CMOS_WRITE(RTC_24H, RTC_CONTROL);
+	ctrl &= ~RTC_SET;
+	CMOS_WRITE(ctrl, RTC_CONTROL);
 }
 
 void __init plat_time_init(void)
-- 
2.3.6

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

[James Hogan]

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.

In a future patch we want to take seconds into account when estimating
the frequency, however that would treat a transition from the invalid
BCD 0x1f (calculated as 25) to BCD 0x20 (20) as -5 seconds, which is
wrapped to 55 seconds and results in a very underestimated frequency.

Therefore read-modify-write the control register so that the previous
mode is preserved instead of being forced into BCD mode. This avoids the
possibility of invalid BCD values immediately afterwards.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Paul Burton <paul.burton@imgtec.com>
Cc: linux-mips@linux-mips.org
---
 arch/mips/mti-malta/malta-time.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/mips/mti-malta/malta-time.c b/arch/mips/mti-malta/malta-time.c
index 185e68261f45..a030d41eb5a1 100644
--- a/arch/mips/mti-malta/malta-time.c
+++ b/arch/mips/mti-malta/malta-time.c
@@ -165,14 +165,18 @@ unsigned int get_c0_compare_int(void)
 
 static void __init init_rtc(void)
 {
+	unsigned char ctrl = CMOS_READ(RTC_CONTROL);
+
 	/* stop the clock whilst setting it up */
-	CMOS_WRITE(RTC_SET | RTC_24H, RTC_CONTROL);
+	ctrl |= RTC_24H | RTC_SET;
+	CMOS_WRITE(ctrl, RTC_CONTROL);
 
 	/* 32KHz time base */
 	CMOS_WRITE(RTC_REF_CLCK_32KHZ, RTC_FREQ_SELECT);
 
 	/* start the clock */
-	CMOS_WRITE(RTC_24H, RTC_CONTROL);
+	ctrl &= ~RTC_SET;
+	CMOS_WRITE(ctrl, RTC_CONTROL);
 }
 
 void __init plat_time_init(void)
-- 
2.3.6

[PATCH 2/2] MIPS: malta-time: Take seconds into account

[James Hogan]

When estimating the clock frequency based on the RTC, take seconds into
account in case the Update In Progress (UIP) bit wasn't seen. This can
happen in virtual machines (which may get pre-empted by the hypervisor
at inopportune times) with QEMU emulating the RTC (and in fact not
setting the UIP bit for very long), especially on slow hosts such as
FPGA systems and emulators. This results in several seconds actually
having passed before seeing the UIP bit instead of just one second, and
exaggerated timer frequencies.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Steven J. Hill <Steven.Hill@imgtec.com>
Cc: linux-mips@linux-mips.org
---
 arch/mips/mti-malta/malta-time.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/arch/mips/mti-malta/malta-time.c b/arch/mips/mti-malta/malta-time.c
index a030d41eb5a1..1aaa56162ba0 100644
--- a/arch/mips/mti-malta/malta-time.c
+++ b/arch/mips/mti-malta/malta-time.c
@@ -21,6 +21,7 @@
 #include <linux/i8253.h>
 #include <linux/init.h>
 #include <linux/kernel_stat.h>
+#include <linux/math64.h>
 #include <linux/sched.h>
 #include <linux/spinlock.h>
 #include <linux/interrupt.h>
@@ -72,6 +73,8 @@ static void __init estimate_frequencies(void)
 {
 	unsigned long flags;
 	unsigned int count, start;
+	unsigned char secs1, secs2, ctrl;
+	int secs;
 	cycle_t giccount = 0, gicstart = 0;
 
 #if defined(CONFIG_KVM_GUEST) && CONFIG_KVM_GUEST_TIMER_FREQ
@@ -91,6 +94,7 @@ static void __init estimate_frequencies(void)
 		gic_start_count();
 		gicstart = gic_read_count();
 	}
+	secs1 = CMOS_READ(RTC_SECONDS);
 
 	/* Read counter exactly on falling edge of update flag. */
 	while (CMOS_READ(RTC_REG_A) & RTC_UIP);
@@ -99,14 +103,25 @@ static void __init estimate_frequencies(void)
 	count = read_c0_count();
 	if (gic_present)
 		giccount = gic_read_count();
+	secs2 = CMOS_READ(RTC_SECONDS);
+	ctrl = CMOS_READ(RTC_CONTROL);
 
 	local_irq_restore(flags);
 
+	if (!(ctrl & RTC_DM_BINARY) || RTC_ALWAYS_BCD) {
+		secs1 = bcd2bin(secs1);
+		secs2 = bcd2bin(secs2);
+	}
+	secs = secs2 - secs1;
+	if (secs < 1)
+		secs += 60;
+
 	count -= start;
+	count /= secs;
 	mips_hpt_frequency = count;
 
 	if (gic_present) {
-		giccount -= gicstart;
+		giccount = div_u64(giccount - gicstart, secs);
 		gic_frequency = giccount;
 	}
 }
-- 
2.3.6

[PATCH 2/2] MIPS: malta-time: Take seconds into account

[James Hogan]

When estimating the clock frequency based on the RTC, take seconds into
account in case the Update In Progress (UIP) bit wasn't seen. This can
happen in virtual machines (which may get pre-empted by the hypervisor
at inopportune times) with QEMU emulating the RTC (and in fact not
setting the UIP bit for very long), especially on slow hosts such as
FPGA systems and emulators. This results in several seconds actually
having passed before seeing the UIP bit instead of just one second, and
exaggerated timer frequencies.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: Steven J. Hill <Steven.Hill@imgtec.com>
Cc: linux-mips@linux-mips.org
---
 arch/mips/mti-malta/malta-time.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/arch/mips/mti-malta/malta-time.c b/arch/mips/mti-malta/malta-time.c
index a030d41eb5a1..1aaa56162ba0 100644
--- a/arch/mips/mti-malta/malta-time.c
+++ b/arch/mips/mti-malta/malta-time.c
@@ -21,6 +21,7 @@
 #include <linux/i8253.h>
 #include <linux/init.h>
 #include <linux/kernel_stat.h>
+#include <linux/math64.h>
 #include <linux/sched.h>
 #include <linux/spinlock.h>
 #include <linux/interrupt.h>
@@ -72,6 +73,8 @@ static void __init estimate_frequencies(void)
 {
 	unsigned long flags;
 	unsigned int count, start;
+	unsigned char secs1, secs2, ctrl;
+	int secs;
 	cycle_t giccount = 0, gicstart = 0;
 
 #if defined(CONFIG_KVM_GUEST) && CONFIG_KVM_GUEST_TIMER_FREQ
@@ -91,6 +94,7 @@ static void __init estimate_frequencies(void)
 		gic_start_count();
 		gicstart = gic_read_count();
 	}
+	secs1 = CMOS_READ(RTC_SECONDS);
 
 	/* Read counter exactly on falling edge of update flag. */
 	while (CMOS_READ(RTC_REG_A) & RTC_UIP);
@@ -99,14 +103,25 @@ static void __init estimate_frequencies(void)
 	count = read_c0_count();
 	if (gic_present)
 		giccount = gic_read_count();
+	secs2 = CMOS_READ(RTC_SECONDS);
+	ctrl = CMOS_READ(RTC_CONTROL);
 
 	local_irq_restore(flags);
 
+	if (!(ctrl & RTC_DM_BINARY) || RTC_ALWAYS_BCD) {
+		secs1 = bcd2bin(secs1);
+		secs2 = bcd2bin(secs2);
+	}
+	secs = secs2 - secs1;
+	if (secs < 1)
+		secs += 60;
+
 	count -= start;
+	count /= secs;
 	mips_hpt_frequency = count;
 
 	if (gic_present) {
-		giccount -= gicstart;
+		giccount = div_u64(giccount - gicstart, secs);
 		gic_frequency = giccount;
 	}
 }
-- 
2.3.6

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

[Maciej W. Rozycki]

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.

 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 and any changes required made to `estimate_frequencies' 
instead.  Which I believe you already did with 2/2.

  Maciej

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

[James Hogan]

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

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.

> 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.

Cheers
James

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

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

[James Hogan]

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

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.

> 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.

Cheers
James

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

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

[Paul Burton]

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

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

[Paul Burton]

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

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

[James Hogan]

[-- 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 --]

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

[James Hogan]

[-- 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 --]

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

[Maciej W. Rozycki]

On Thu, 14 May 2015, James Hogan wrote:

> > 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.

 You do need to take reverse dependencies into account though, such as 
this one where YAMON relies upon a certain state of hardware (that it 
does initialise) to function correctly.  Granted, this RTC issue is 
probably the only one as other hardware on the Malta board does not 
carry initialised state over reset I believe.

> > ...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.

 I'd prefer RTC state not to be touched at all if its state is sane.  
That is read Register B, check for the only valid divider setting 
(32kHz), and if so then exit right away, and otherwise initialise the 
chip from scratch.  Consistency with YAMON might be a good idea in that 
initialisation, but I have no strong feeling towards that.  If you think 
there's value in having the chip set to the BCD mode, then feel free to 
keep that option.

 Note that any inhibition of the RTC previously initialised by 
temporarily setting the SET bit in Register B during bootstrap will 
disturb timekeeping that the system may carry over reset using 
adjtimex(8).

  Maciej

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

[Ralf Baechle]

On Thu, May 14, 2015 at 11:53:22AM +0100, Maciej W. Rozycki wrote:

> > > 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.
> 
>  You do need to take reverse dependencies into account though, such as 
> this one where YAMON relies upon a certain state of hardware (that it 
> does initialise) to function correctly.  Granted, this RTC issue is 
> probably the only one as other hardware on the Malta board does not 
> carry initialised state over reset I believe.
> 
> > > ...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.
> 
>  I'd prefer RTC state not to be touched at all if its state is sane.  
> That is read Register B, check for the only valid divider setting 
> (32kHz), and if so then exit right away, and otherwise initialise the 
> chip from scratch.  Consistency with YAMON might be a good idea in that 
> initialisation, but I have no strong feeling towards that.  If you think 
> there's value in having the chip set to the BCD mode, then feel free to 
> keep that option.
> 
>  Note that any inhibition of the RTC previously initialised by 
> temporarily setting the SET bit in Register B during bootstrap will 
> disturb timekeeping that the system may carry over reset using 
> adjtimex(8).

So you're instead suggesting to revoke a87ea88d8f6c ?

If YAMON and U-Boot are differing in RTC handling then I suggest to
treat that as a U-Boot bug.  YAMON was there first.  However these
Malta kernels are also frequently booted without firmware in Qemu.
No idea how Qemu initializes the RTC.

  Ralf

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

[Paul Burton]

On Fri, May 15, 2015 at 08:03:51PM +0200, Ralf Baechle wrote:
> >  I'd prefer RTC state not to be touched at all if its state is sane.  
> > That is read Register B, check for the only valid divider setting 
> > (32kHz), and if so then exit right away, and otherwise initialise the 
> > chip from scratch.  Consistency with YAMON might be a good idea in that 
> > initialisation, but I have no strong feeling towards that.  If you think 
> > there's value in having the chip set to the BCD mode, then feel free to 
> > keep that option.
> > 
> >  Note that any inhibition of the RTC previously initialised by 
> > temporarily setting the SET bit in Register B during bootstrap will 
> > disturb timekeeping that the system may carry over reset using 
> > adjtimex(8).
> 
> So you're instead suggesting to revoke a87ea88d8f6c ?
> 
> If YAMON and U-Boot are differing in RTC handling then I suggest to
> treat that as a U-Boot bug. YAMON was there first.

That would be fair enough, and is why I added RTC handling to Malta
U-boot at all. I could see logic in suggesting U-boot be changed to use
the binary mode instead of BCD. But...

> However these Malta kernels are also frequently booted without firmware
> in Qemu. No idea how Qemu initializes the RTC.

...kernels can also be booted on real Malta boards with minimal prodding
over JTAG, and the RTC is one more thing that you need to prod if the
kernel doesn't ensure it's running. That's what motivated a87ea88d8f6c
and the other patches from the same series at all.

Thanks,
    Paul

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

[Paul Burton]

On Fri, May 15, 2015 at 08:03:51PM +0200, Ralf Baechle wrote:
> >  I'd prefer RTC state not to be touched at all if its state is sane.  
> > That is read Register B, check for the only valid divider setting 
> > (32kHz), and if so then exit right away, and otherwise initialise the 
> > chip from scratch.  Consistency with YAMON might be a good idea in that 
> > initialisation, but I have no strong feeling towards that.  If you think 
> > there's value in having the chip set to the BCD mode, then feel free to 
> > keep that option.
> > 
> >  Note that any inhibition of the RTC previously initialised by 
> > temporarily setting the SET bit in Register B during bootstrap will 
> > disturb timekeeping that the system may carry over reset using 
> > adjtimex(8).
> 
> So you're instead suggesting to revoke a87ea88d8f6c ?
> 
> If YAMON and U-Boot are differing in RTC handling then I suggest to
> treat that as a U-Boot bug. YAMON was there first.

That would be fair enough, and is why I added RTC handling to Malta
U-boot at all. I could see logic in suggesting U-boot be changed to use
the binary mode instead of BCD. But...

> However these Malta kernels are also frequently booted without firmware
> in Qemu. No idea how Qemu initializes the RTC.

...kernels can also be booted on real Malta boards with minimal prodding
over JTAG, and the RTC is one more thing that you need to prod if the
kernel doesn't ensure it's running. That's what motivated a87ea88d8f6c
and the other patches from the same series at all.

Thanks,
    Paul

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

[Maciej W. Rozycki]

On Fri, 15 May 2015, Paul Burton wrote:

> > >  I'd prefer RTC state not to be touched at all if its state is sane.  
> > > That is read Register B, check for the only valid divider setting 
> > > (32kHz), and if so then exit right away, and otherwise initialise the 
> > > chip from scratch.  Consistency with YAMON might be a good idea in that 
> > > initialisation, but I have no strong feeling towards that.  If you think 
> > > there's value in having the chip set to the BCD mode, then feel free to 
> > > keep that option.
> > > 
> > >  Note that any inhibition of the RTC previously initialised by 
> > > temporarily setting the SET bit in Register B during bootstrap will 
> > > disturb timekeeping that the system may carry over reset using 
> > > adjtimex(8).
> > 
> > So you're instead suggesting to revoke a87ea88d8f6c ?

 No, now that I have the full picture -- everyone, please try to include 
as much details with our commit messages as required for the reader to be 
able to have a full understanding of the reasons behind the change without 
having to guess or ask around.  You may not be around in a few years' 
time, having moved on with your career or interests, or suchlike, and 
people will have to cope without your assistance.  It's OK to give these 
messages a meaning, they are not GNU ChangeLogs!

> > If YAMON and U-Boot are differing in RTC handling then I suggest to
> > treat that as a U-Boot bug. YAMON was there first.

 And that is a grand first, 15+ years!

> That would be fair enough, and is why I added RTC handling to Malta
> U-boot at all. I could see logic in suggesting U-boot be changed to use
> the binary mode instead of BCD. But...

 I find it a shame BTW that the handling of RTC has fallen so bad recently 
across various platforms.  Here you shouldn't have been required to do 
anything beyond enabling the right RTC driver (the Motorola clone has been 
so ubiquitous that a driver must have been done eons ago), defining its 
mapping on the bus (again 0x70/0x71 in the PCI I/O space is the vastly 
most common arrangement) and maybe setting some configuration flags for 
the mode (binary vs BCD and the like).

 A while ago I had to deal with some U-Boot-based Freescale Power boards 
and the firmware had no commands to set the clock/date in the RTC at all.  
Furthermore the installed userland had no access to the RTC, either 
because of the /dev/rtc ABI change that happened a while ago (a different 
error code is returned these days for some kind of a failure than it used 
to be and unpatched `hwclock' bails out), or because no driver claimed the 
clock (I don't remember anymore), even though some platform code did 
initialise the system time from the RTC.

 Consequently the board booted with nonsensical time stamps recorded in 
logs up to the point the system time was synchronised from a networked 
source and any time stamps on files modified in the single user-mode were 
useless unless you first ran `date' to set the time manually.  Very 
frustrating when I had to investigate what happened when.

 Also related to clock handling, though not RTC this time, I had some 
system clock issues with SEAD-3, with time drifting so much (amounting to 
minutes per hour IIRC) and so irregularly that the NTP daemon couldn't 
cope and eventually bailed out.  Have they been addressed now?

> > However these Malta kernels are also frequently booted without firmware
> > in Qemu. No idea how Qemu initializes the RTC.

 No idea offhand, although reasonably I expect the clock/calendar to have 
been initialised from the host's system clock and the control registers 
according to target hardware platform settings.  For many platforms these 
are fixed according to some kind of an industry standard (e.g. IBM PC/AT) 
or implementation (e.g. workstations and servers), so QEMU has to reflect 
that in its environment or software dedicated to individual systems will 
malfunction.  So I expect QEMU to have a way to deal with it.

> ...kernels can also be booted on real Malta boards with minimal prodding
> over JTAG, and the RTC is one more thing that you need to prod if the
> kernel doesn't ensure it's running. That's what motivated a87ea88d8f6c
> and the other patches from the same series at all.

 So these kind of details (including a reference to U-Boot as well) are 
ones I expect to have been included with a87ea88d8f6c.  Please try and 
keep it in mind in the future, I'll appreciate that.

 So to recap and FAOD I think `init_rtc' needs to stay after all, but I 
request that it is updated so that a sane RTC state is not changed.  I 
hope that either of you, James and Paul, can handle it.

  Maciej

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

[Joshua Kinard]

On 05/15/2015 17:13, Maciej W. Rozycki wrote:
> On Fri, 15 May 2015, Paul Burton wrote:
> 
>>>>  I'd prefer RTC state not to be touched at all if its state is sane.  
>>>> That is read Register B, check for the only valid divider setting 
>>>> (32kHz), and if so then exit right away, and otherwise initialise the 
>>>> chip from scratch.  Consistency with YAMON might be a good idea in that 
>>>> initialisation, but I have no strong feeling towards that.  If you think 
>>>> there's value in having the chip set to the BCD mode, then feel free to 
>>>> keep that option.
>>>>
>>>>  Note that any inhibition of the RTC previously initialised by 
>>>> temporarily setting the SET bit in Register B during bootstrap will 
>>>> disturb timekeeping that the system may carry over reset using 
>>>> adjtimex(8).
>>>
>>> So you're instead suggesting to revoke a87ea88d8f6c ?
> 
>  No, now that I have the full picture -- everyone, please try to include 
> as much details with our commit messages as required for the reader to be 
> able to have a full understanding of the reasons behind the change without 
> having to guess or ask around.  You may not be around in a few years' 
> time, having moved on with your career or interests, or suchlike, and 
> people will have to cope without your assistance.  It's OK to give these 
> messages a meaning, they are not GNU ChangeLogs!

Maybe not in the commit message for rtc-ds1685, but a good chunk of that driver
is comments.  I found one of the most frustrating things in working on that
driver to be not understanding what other RTC drivers were doing because no one
else commented their code clearly.  Hopefully, if someone winds up in the same
position I was in, they'll be able to understand quicker what rtc-ds1685 does
and maybe use some of its code in their setup.


>>> If YAMON and U-Boot are differing in RTC handling then I suggest to
>>> treat that as a U-Boot bug. YAMON was there first.
> 
>  And that is a grand first, 15+ years!
> 
>> That would be fair enough, and is why I added RTC handling to Malta
>> U-boot at all. I could see logic in suggesting U-boot be changed to use
>> the binary mode instead of BCD. But...
> 
>  I find it a shame BTW that the handling of RTC has fallen so bad recently 
> across various platforms.  Here you shouldn't have been required to do 
> anything beyond enabling the right RTC driver (the Motorola clone has been 
> so ubiquitous that a driver must have been done eons ago), defining its 
> mapping on the bus (again 0x70/0x71 in the PCI I/O space is the vastly 
> most common arrangement) and maybe setting some configuration flags for 
> the mode (binary vs BCD and the like).

SGI O2 and SGI Octane both use the same RTC, a DS1687-5.  But with the O2, the
ARCS PROM forces the RTC to BCD on startup, while the Octane's ARCS PROM forces
it to binary on startup.  And if the Octane's PROM discovers the RTC is in BCD
mode, it resets it to 1969.  That really confused e2fsck...

Also on the O2, you can MMIO the RTC and make life easy.  But on the Octane,
the RTC is kludged onto the IOC3 ByteBus...so you have to write the address of
interest to an addr port, then read the data from the data port.  I've
discovered since then that, if you spam 'hwclock --show' on the shell, the O2
never misses a beat, but on the Octane, because the IOC3 ByteBus is so slow, I
can actually catch the RTC in the middle of an update cycle, which locks out
access for 1 second until the update completes.

--J