| Message ID | 1359507865-29808-1-git-send-email-haojian.zhuang@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
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.
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
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.
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
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
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 --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
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(-)