diff mbox

rtc: pl031: fix the missing operation on enable

Message ID 1359507865-29808-1-git-send-email-haojian.zhuang@linaro.org
State Accepted
Commit e7e034e18a0ab6bafb2425c3242cac311164f4d6
Headers show

Commit Message

Haojian Zhuang Jan. 30, 2013, 1:04 a.m. UTC
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(-)

Comments

Andrew Morton Jan. 31, 2013, 10:44 p.m. UTC | #1
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.
Haojian Zhuang Feb. 1, 2013, 1:32 a.m. UTC | #2
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
Andrew Morton Feb. 1, 2013, 11:47 p.m. UTC | #3
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.
Linus Walleij Feb. 4, 2013, 12:25 p.m. UTC | #4
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
Haojian Zhuang Feb. 4, 2013, 12:34 p.m. UTC | #5
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
Linus Walleij Feb. 4, 2013, 12:43 p.m. UTC | #6
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
diff mbox

Patch

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