Skip to contentSkip to conversation

rtc: pl031: fix the missing operation on enable

PATCH New Haojian Zhuang 3 participants 7 messages Jan 30, 2013 – Feb 4, 2013

Subsystems (2)
Real time clock (RTC) Arm/nomadik/Ux500 architectures
lore
Share & tools
lore

Haojian Zhuang

author
#0 lore raw

PATCH 1/1 rtc: pl031: fix the missing operation on enable

RTC control register should be enabled in the process of initliazing.

Signed-off-by Haojian Zhuang <[email protected]>
---
 drivers/rtc/rtc-pl031.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/rtc/rtc-pl031.c +5 -3
index 08378e3..10c1a34 100644
@@ -44,6 +44,7 @@
 #define RTC_YMR		0x34	/* Year match register */
 #define RTC_YLR		0x38	/* Year data load register */
 
+#define RTC_CR_EN	(1 << 0)	/* counter enable bit */
 #define RTC_CR_CWEN	(1 << 26)	/* Clockwatch enable bit */
 
 #define RTC_TCR_EN	(1 << 1) /* Periodic timer enable bit */

@@ -320,7 +321,7 @@ static int pl031_probe(struct amba_device *adev, const struct amba_id *id)
 	struct pl031_local *ldata;
 	struct pl031_vendor_data *vendor = id->data;
 	struct rtc_class_ops *ops = &vendor->ops;
-	unsigned long time;
+	unsigned long time, data;
 
 	ret = amba_request_regions(adev, NULL);
 	if (ret)

@@ -345,10 +346,11 @@ static int pl031_probe(struct amba_device *adev, const struct amba_id *id)
 	dev_dbg(&adev->dev, "designer ID = 0x%02x\n", amba_manf(adev));
 	dev_dbg(&adev->dev, "revision = 0x%01x\n", amba_rev(adev));
 
+	data = readl(ldata->base + RTC_CR);
 	/* Enable the clockwatch on ST Variants */
 	if (vendor->clockwatch)
-		writel(readl(ldata->base + RTC_CR) | RTC_CR_CWEN,
-		       ldata->base + RTC_CR);
+		data |= RTC_CR_CWEN;
+	writel(data | RTC_CR_EN, ldata->base + RTC_CR);
 
 	/*
 	 * On ST PL031 variants, the RTC reset value does not provide correct
Signature · 1 line
1.7.10.4
#1 lore raw

PATCH 1/1 rtc: pl031: fix the missing operation on enable

Quoted 24 lines · On Wed, 30 Jan 2013 09:04:25 +0800 Haojian Zhuang <[email protected]>…

RTC control register should be enabled in the process of initliazing.

...

drivers/rtc/rtc-pl031.c +2 -1
@@ -44,6 +44,7 @@
 #define RTC_YMR		0x34	/* Year match register */
 #define RTC_YLR		0x38	/* Year data load register */
 
+#define RTC_CR_EN	(1 << 0)	/* counter enable bit */
 #define RTC_CR_CWEN	(1 << 26)	/* Clockwatch enable bit */
 
 #define RTC_TCR_EN	(1 << 1) /* Periodic timer enable bit */

@@ -320,7 +321,7 @@ static int pl031_probe(struct amba_device *adev, const struct amba_id *id)
 	struct pl031_local *ldata;
 	struct pl031_vendor_data *vendor = id->data;
 	struct rtc_class_ops *ops = &vendor->ops;
-	unsigned long time;
+	unsigned long time, data;
 
 	ret = amba_request_regions(adev, NULL);
 	if (ret)

drivers/rtc/rtc-pl031.c static int pl031_probe(struct amba_device *adev, const struct amba_id *id) · line 353

Quoted 10 lines
drivers/rtc/rtc-pl031.c +3 -2
@@ -345,10 +346,11 @@ static int pl031_probe(struct amba_device *adev, const struct amba_id *id)
 	dev_dbg(&adev->dev, "designer ID = 0x%02x\n", amba_manf(adev));
 	dev_dbg(&adev->dev, "revision = 0x%01x\n", amba_rev(adev));
 
+	data = readl(ldata->base + RTC_CR);
 	/* Enable the clockwatch on ST Variants */
 	if (vendor->clockwatch)
-		writel(readl(ldata->base + RTC_CR) | RTC_CR_CWEN,
-		       ldata->base + RTC_CR);
+		data |= RTC_CR_CWEN;
+	writel(data | RTC_CR_EN, ldata->base + RTC_CR);

Does this patch fix some user-visible misbehaviour? If so, please fully describe that misbehaviour.

author
#2 lore raw

PATCH 1/1 rtc: pl031: fix the missing operation on enable

Quoted 30 lines · Andrew Morton
Quoted 27 lines · On Wed, 30 Jan 2013 09:04:25 +0800 Haojian Zhuang <[email protected]>…

RTC control register should be enabled in the process of initliazing.

...

drivers/rtc/rtc-pl031.c +2 -1
@@ -44,6 +44,7 @@
 #define RTC_YMR              0x34    /* Year match register */
 #define RTC_YLR              0x38    /* Year data load register */
 
+#define RTC_CR_EN    (1 << 0)        /* counter enable bit */
 #define RTC_CR_CWEN  (1 << 26)       /* Clockwatch enable bit */
 
 #define RTC_TCR_EN   (1 << 1) /* Periodic timer enable bit */

@@ -320,7 +321,7 @@ static int pl031_probe(struct amba_device *adev, const struct amba_id *id)
      struct pl031_local *ldata;
      struct pl031_vendor_data *vendor = id->data;
      struct rtc_class_ops *ops = &vendor->ops;
-     unsigned long time;
+     unsigned long time, data;
 
      ret = amba_request_regions(adev, NULL);
      if (ret)

@@ -345,10 +346,11 @@ static int pl031_probe(struct amba_device *adev, const struct amba_id *id)
      dev_dbg(&adev->dev, "designer ID = 0x%02x\n", amba_manf(adev));
      dev_dbg(&adev->dev, "revision = 0x%01x\n", amba_rev(adev));
Quoted 11 lines

drivers/rtc/rtc-pl031.c static int pl031_probe(struct amba_device *adev, const struct amba_id *id) · line 353

Quoted 7 lines
drivers/rtc/rtc-pl031.c +3 -2
+     data = readl(ldata->base + RTC_CR);
      /* Enable the clockwatch on ST Variants */
      if (vendor->clockwatch)
-             writel(readl(ldata->base + RTC_CR) | RTC_CR_CWEN,
-                    ldata->base + RTC_CR);
+             data |= RTC_CR_CWEN;
+     writel(data | RTC_CR_EN, ldata->base + RTC_CR);

Does this patch fix some user-visible misbehaviour? If so, please fully describe that misbehaviour.

Hi Andrew,

I copy the description from rtc pl031 user manual (page 33 of DDI0224.pdf) in below.

RTCCR is a 1-bit control register. When HIGH, the counter enable signal is asserted to
enable the counter. Table 3-5 shows the bit assignments for the RTCCR register.

Table 3-5 RTCCR register
-----------------------------------------------------------------------------------------------------------------------
Bits Name Type Function
31:1 - Read/write Reserved. Read
unpredictable. Should
be written as 0.
0 RTC start Read/write If set to 1, the
RTC is enabled. Once it is

enabled, any writes to this bit have no
effect
on the RTC until a system reset.
A read
returns the status of the RTC.
-----------------------------------------------------------------------------------------------------------------------

Quoted 1 line

From this document, RTCCR must be enabled before usage. Without this

patch, I really
failed to enable RTC in Hisilicon Hi3620 SoC. It results that the register mapping section
in RTC is always read as zero. So I doubt that ST guys may already enable this register
in bootloader. So they won't meet this issue.

Best Regards
Haojian

#3 lore raw

PATCH 1/1 rtc: pl031: fix the missing operation on enable

Quoted 7 lines · On Fri, 1 Feb 2013 09:32:59 +0800 Haojian Zhuang <[email protected]>…

Without this
patch, I really
failed to enable RTC in Hisilicon Hi3620 SoC. It results that the register mapping section
in RTC is always read as zero. So I doubt that ST guys may already enable this register
in bootloader. So they won't meet this issue.

OK, thanks, that sounds pretty serious so I tagged the patch for backporting into -stable kernels as well.

#4 lore raw

PATCH 1/1 rtc: pl031: fix the missing operation on enable

In reply to Haojian Zhuang

Hi Haojian, sorry for taking too long to reply...

Quoted 3 lines · Haojian Zhuang

RTC control register should be enabled in the process of initliazing.

Signed-off-by Haojian Zhuang <[email protected]>

(...)

drivers/rtc/rtc-pl031.c static int pl031_probe(struct amba_device *adev, const struct amba_id *id) · line 353

Quoted 7 lines
drivers/rtc/rtc-pl031.c +3 -2
+       data = readl(ldata->base + RTC_CR);
        /* Enable the clockwatch on ST Variants */
        if (vendor->clockwatch)
-               writel(readl(ldata->base + RTC_CR) | RTC_CR_CWEN,
-                      ldata->base + RTC_CR);
+               data |= RTC_CR_CWEN;
+       writel(data | RTC_CR_EN, ldata->base + RTC_CR);

This last line is *not* OK on the ST Variant. In our hardware that bit is part of the clock divider, which means it will affect our timekeeping.

Do you want me to submit a follow-up patch?

Yours,
Linus Walleij

author
#5 lore raw

PATCH 1/1 rtc: pl031: fix the missing operation on enable

Quoted 18 lines · Linus Walleij

Hi Haojian, sorry for taking too long to reply...

Quoted 3 lines · Haojian Zhuang

RTC control register should be enabled in the process of initliazing.

Signed-off-by Haojian Zhuang <[email protected]>

(...)

drivers/rtc/rtc-pl031.c static int pl031_probe(struct amba_device *adev, const struct amba_id *id) · line 353

Quoted 7 lines
drivers/rtc/rtc-pl031.c +3 -2
+       data = readl(ldata->base + RTC_CR);
        /* Enable the clockwatch on ST Variants */
        if (vendor->clockwatch)
-               writel(readl(ldata->base + RTC_CR) | RTC_CR_CWEN,
-                      ldata->base + RTC_CR);
+               data |= RTC_CR_CWEN;
+       writel(data | RTC_CR_EN, ldata->base + RTC_CR);
Quoted 7 lines

This last line is *not* OK on the ST Variant. In our hardware that bit is part of the clock divider, which means it will affect our timekeeping.

Do you want me to submit a follow-up patch?

Yours,
Linus Walleij

I prefer you can submit a follow up patch. And I think that you can use a compatible
name to distinguish from original "arm,rtc-pl031". Then we can get two branches to
handle the difference in the probe function. What's your opinion?

Regards
Haojian

#6 lore raw

PATCH 1/1 rtc: pl031: fix the missing operation on enable

Quoted 1 line · Haojian Zhuang

I prefer you can submit a follow up patch.

OK!

Quoted 5 lines

And I think that you can
use a compatible
name to distinguish from original "arm,rtc-pl031". Then we can get two branches to
handle the difference in the probe function. What's your opinion?

No that is wrong for PrimeCells.

PrimeCells have special ID numbers that we use to identify the different variants already.

Yours,
Linus Walleij