[PATCH] rtc: pl031: fix the missing operation on enable

[PATCH] rtc: pl031: fix the missing operation on enable

[Haojian Zhuang]

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

Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
---
 drivers/rtc/rtc-pl031.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/rtc/rtc-pl031.c b/drivers/rtc/rtc-pl031.c
index 08378e3..10c1a34 100644
--- a/drivers/rtc/rtc-pl031.c
+++ b/drivers/rtc/rtc-pl031.c
@@ -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
-- 
1.7.10.4

[rtc-linux] [PATCH] rtc: pl031: fix the missing operation on enable

[Andrew Morton]

On Wed, 30 Jan 2013 09:04:25 +0800
Haojian Zhuang <haojian.zhuang@linaro.org> wrote:

> RTC control register should be enabled in the process of initliazing.
> 
> ...
>
> --- a/drivers/rtc/rtc-pl031.c
> +++ b/drivers/rtc/rtc-pl031.c
> @@ -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);

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

[rtc-linux] [PATCH] rtc: pl031: fix the missing operation on enable

[Haojian Zhuang]

On 1 February 2013 06:44, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed, 30 Jan 2013 09:04:25 +0800
> Haojian Zhuang <haojian.zhuang@linaro.org> wrote:
>
>> RTC control register should be enabled in the process of initliazing.
>>
>> ...
>>
>> --- a/drivers/rtc/rtc-pl031.c
>> +++ b/drivers/rtc/rtc-pl031.c
>> @@ -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);
>
> 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.
-----------------------------------------------------------------------------------------------------------------------

>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

[rtc-linux] [PATCH] rtc: pl031: fix the missing operation on enable

[Andrew Morton]

On Fri, 1 Feb 2013 09:32:59 +0800
Haojian Zhuang <haojian.zhuang@linaro.org> wrote:

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

[PATCH] rtc: pl031: fix the missing operation on enable

[Linus Walleij]

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

On Wed, Jan 30, 2013 at 2:04 AM, Haojian Zhuang
<haojian.zhuang@linaro.org> wrote:

> RTC control register should be enabled in the process of initliazing.
>
> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>

(...)
> +       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

[PATCH] rtc: pl031: fix the missing operation on enable

[Haojian Zhuang]

On 4 February 2013 20:25, Linus Walleij <linus.walleij@linaro.org> wrote:
> Hi Haojian, sorry for taking too long to reply...
>
> On Wed, Jan 30, 2013 at 2:04 AM, Haojian Zhuang
> <haojian.zhuang@linaro.org> wrote:
>
>> RTC control register should be enabled in the process of initliazing.
>>
>> Signed-off-by: Haojian Zhuang <haojian.zhuang@linaro.org>
>
> (...)
>> +       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

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

[PATCH] rtc: pl031: fix the missing operation on enable

[Linus Walleij]

On Mon, Feb 4, 2013 at 1:34 PM, Haojian Zhuang
<haojian.zhuang@linaro.org> wrote:

> I prefer you can submit a follow up patch.

OK!

> 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