| Message ID | 1286494879-2932-2-git-send-email-kgene.kim@samsung.com |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, Oct 8, 2010 at 8:41 AM, Kukjin Kim <kgene.kim@samsung.com> wrote: > From: Changhwan Youn <chaos.youn@samsung.com> > > S3C2410_RTCCON of TYPE_S3C64XX RTC should be read/written by > readw and writew, because TYPE_S3C64XX RTC uses bit 8 and 9. > And TYPE_S3C2410 RTC also can access it by readw and writew. > > Signed-off-by: Changhwan Youn <chaos.youn@samsung.com> > [atul.dahiya@samsung.com: tested on smdk2416] > Tested-by: Atul Dahiya <atul.dahiya@samsung.com> > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com> > Cc: Ben Dooks <ben-linux@fluff.org> > --- > drivers/rtc/rtc-s3c.c | 36 ++++++++++++++++++------------------ > 1 files changed, 18 insertions(+), 18 deletions(-) Hello, Sorry for a late reply... Anyway, I have a small question in this rtc-s3c.c driver. Is there any reason to use read/write b/w to access registers of rtc-s3c? Why don't we use readl/writel when accessing registers in this drivers and just forget which registers require at least 8 or 16 or 32 bits? In fact, it appears that readw/writew accesses 16bits, not 32bits in ARM machines, which may incur problems with TICCNT/CURTICCNT registers. > > diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c > index f57a87f..4a0b875 100644 > --- a/drivers/rtc/rtc-s3c.c > +++ b/drivers/rtc/rtc-s3c.c > @@ -100,7 +100,7 @@ static int s3c_rtc_setpie(struct device *dev, int enabled) > spin_lock_irq(&s3c_rtc_pie_lock); > > if (s3c_rtc_cpu_type == TYPE_S3C64XX) { > - tmp = readb(s3c_rtc_base + S3C2410_RTCCON); > + tmp = readw(s3c_rtc_base + S3C2410_RTCCON); > tmp &= ~S3C64XX_RTCCON_TICEN; > > if (enabled) > @@ -318,7 +318,7 @@ static int s3c_rtc_proc(struct device *dev, struct seq_file *seq) > unsigned int ticnt; > > if (s3c_rtc_cpu_type == TYPE_S3C64XX) { > - ticnt = readb(s3c_rtc_base + S3C2410_RTCCON); > + ticnt = readw(s3c_rtc_base + S3C2410_RTCCON); > ticnt &= S3C64XX_RTCCON_TICEN; > } else { > ticnt = readb(s3c_rtc_base + S3C2410_TICNT); > @@ -391,11 +391,11 @@ static void s3c_rtc_enable(struct platform_device *pdev, int en) > return; > > if (!en) { > - tmp = readb(base + S3C2410_RTCCON); > + tmp = readw(base + S3C2410_RTCCON); > if (s3c_rtc_cpu_type == TYPE_S3C64XX) > tmp &= ~S3C64XX_RTCCON_TICEN; > tmp &= ~S3C2410_RTCCON_RTCEN; > - writeb(tmp, base + S3C2410_RTCCON); > + writew(tmp, base + S3C2410_RTCCON); > > if (s3c_rtc_cpu_type == TYPE_S3C2410) { > tmp = readb(base + S3C2410_TICNT); > @@ -405,25 +405,25 @@ static void s3c_rtc_enable(struct platform_device *pdev, int en) > } else { > /* re-enable the device, and check it is ok */ > > - if ((readb(base+S3C2410_RTCCON) & S3C2410_RTCCON_RTCEN) == 0){ > + if ((readw(base+S3C2410_RTCCON) & S3C2410_RTCCON_RTCEN) == 0){ > dev_info(&pdev->dev, "rtc disabled, re-enabling\n"); > > - tmp = readb(base + S3C2410_RTCCON); > - writeb(tmp|S3C2410_RTCCON_RTCEN, base+S3C2410_RTCCON); > + tmp = readw(base + S3C2410_RTCCON); > + writew(tmp|S3C2410_RTCCON_RTCEN, base+S3C2410_RTCCON); > } > > - if ((readb(base + S3C2410_RTCCON) & S3C2410_RTCCON_CNTSEL)){ > + if ((readw(base + S3C2410_RTCCON) & S3C2410_RTCCON_CNTSEL)){ > dev_info(&pdev->dev, "removing RTCCON_CNTSEL\n"); > > - tmp = readb(base + S3C2410_RTCCON); > - writeb(tmp& ~S3C2410_RTCCON_CNTSEL, base+S3C2410_RTCCON); > + tmp = readw(base + S3C2410_RTCCON); > + writew(tmp& ~S3C2410_RTCCON_CNTSEL, base+S3C2410_RTCCON); > } > > - if ((readb(base + S3C2410_RTCCON) & S3C2410_RTCCON_CLKRST)){ > + if ((readw(base + S3C2410_RTCCON) & S3C2410_RTCCON_CLKRST)){ > dev_info(&pdev->dev, "removing RTCCON_CLKRST\n"); > > - tmp = readb(base + S3C2410_RTCCON); > - writeb(tmp & ~S3C2410_RTCCON_CLKRST, base+S3C2410_RTCCON); > + tmp = readw(base + S3C2410_RTCCON); > + writew(tmp & ~S3C2410_RTCCON_CLKRST, base+S3C2410_RTCCON); > } > } > } > @@ -514,8 +514,8 @@ static int __devinit s3c_rtc_probe(struct platform_device *pdev) > > s3c_rtc_enable(pdev, 1); > > - pr_debug("s3c2410_rtc: RTCCON=%02x\n", > - readb(s3c_rtc_base + S3C2410_RTCCON)); > + pr_debug("s3c2410_rtc: RTCCON=%02x\n", > + readw(s3c_rtc_base + S3C2410_RTCCON)); > > device_init_wakeup(&pdev->dev, 1); > > @@ -578,7 +578,7 @@ static int s3c_rtc_suspend(struct platform_device *pdev, pm_message_t state) > /* save TICNT for anyone using periodic interrupts */ > ticnt_save = readb(s3c_rtc_base + S3C2410_TICNT); > if (s3c_rtc_cpu_type == TYPE_S3C64XX) { > - ticnt_en_save = readb(s3c_rtc_base + S3C2410_RTCCON); > + ticnt_en_save = readw(s3c_rtc_base + S3C2410_RTCCON); > ticnt_en_save &= S3C64XX_RTCCON_TICEN; > } > s3c_rtc_enable(pdev, 0); > @@ -596,8 +596,8 @@ static int s3c_rtc_resume(struct platform_device *pdev) > s3c_rtc_enable(pdev, 1); > writeb(ticnt_save, s3c_rtc_base + S3C2410_TICNT); > if (s3c_rtc_cpu_type == TYPE_S3C64XX && ticnt_en_save) { > - tmp = readb(s3c_rtc_base + S3C2410_RTCCON); > - writeb(tmp | ticnt_en_save, s3c_rtc_base + S3C2410_RTCCON); > + tmp = readw(s3c_rtc_base + S3C2410_RTCCON); > + writew(tmp | ticnt_en_save, s3c_rtc_base + S3C2410_RTCCON); > } > > if (device_may_wakeup(&pdev->dev)) > -- > 1.6.2.5 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
MyungJoo Ham wrote: > > On Fri, Oct 8, 2010 at 8:41 AM, Kukjin Kim <kgene.kim@samsung.com> wrote: > > From: Changhwan Youn <chaos.youn@samsung.com> > > > > S3C2410_RTCCON of TYPE_S3C64XX RTC should be read/written by > > readw and writew, because TYPE_S3C64XX RTC uses bit 8 and 9. > > And TYPE_S3C2410 RTC also can access it by readw and writew. > > > > Signed-off-by: Changhwan Youn <chaos.youn@samsung.com> > > [atul.dahiya@samsung.com: tested on smdk2416] > > Tested-by: Atul Dahiya <atul.dahiya@samsung.com> > > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com> > > Cc: Ben Dooks <ben-linux@fluff.org> > > --- > > drivers/rtc/rtc-s3c.c | 36 ++++++++++++++++++------------------ > > 1 files changed, 18 insertions(+), 18 deletions(-) > > Hello, > Hi, > > Sorry for a late reply... > Yeah, too late :-( > Anyway, I have a small question in this rtc-s3c.c driver. > > Is there any reason to use read/write b/w to access registers of rtc-s3c? > See the git comment. > Why don't we use readl/writel when accessing registers in this drivers I don't know why we should use readl/writel for all case... even though we can use just word or byte access. > and just forget which registers require at least 8 or 16 or 32 bits? > > In fact, it appears that readw/writew accesses 16bits, not 32bits in Yes... > ARM machines, which may incur problems with TICCNT/CURTICCNT > registers. > I can't get your comment... Thanks. Best regards, Kgene. -- Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.
On Wed, Oct 27, 2010 at 4:58 PM, Kukjin Kim <kgene.kim@samsung.com> wrote: > MyungJoo Ham wrote: >> >> On Fri, Oct 8, 2010 at 8:41 AM, Kukjin Kim <kgene.kim@samsung.com> wrote: >> > From: Changhwan Youn <chaos.youn@samsung.com> >> > >> > S3C2410_RTCCON of TYPE_S3C64XX RTC should be read/written by >> > readw and writew, because TYPE_S3C64XX RTC uses bit 8 and 9. >> > And TYPE_S3C2410 RTC also can access it by readw and writew. >> > >> > Signed-off-by: Changhwan Youn <chaos.youn@samsung.com> >> > [atul.dahiya@samsung.com: tested on smdk2416] >> > Tested-by: Atul Dahiya <atul.dahiya@samsung.com> >> > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com> >> > Cc: Ben Dooks <ben-linux@fluff.org> >> > --- >> > drivers/rtc/rtc-s3c.c | 36 ++++++++++++++++++------------------ >> > 1 files changed, 18 insertions(+), 18 deletions(-) >> >> Hello, >> > Hi, >> >> Sorry for a late reply... >> > Yeah, too late :-( > >> Anyway, I have a small question in this rtc-s3c.c driver. >> >> Is there any reason to use read/write b/w to access registers of rtc-s3c? >> > See the git comment. > >> Why don't we use readl/writel when accessing registers in this drivers > > I don't know why we should use readl/writel for all case... > even though we can use just word or byte access. > >> and just forget which registers require at least 8 or 16 or 32 bits? >> >> In fact, it appears that readw/writew accesses 16bits, not 32bits in > > Yes... > >> ARM machines, which may incur problems with TICCNT(S3C2410_TICNT in the code)/CURTICCNT >> registers. >> > I can't get your comment... It is because TICCNT and CURTICCNT RTC registers of S5PC210 require 32bit access according to the manual while this patch still uses 16bit accesses for them. Besides, ALMYEAR/BCDYEAR(S3C2410_RTCYEAR) requires at least 16bit access. This issue was found today while testing suspend-to-mem with s5pc210 and rtc-wakeup loop. My guess is that saving and restoring only 8 bits of TICNT at suspend/resume function incurred the instable suspend-to-mem (kernel hangs with about 1/100 probability) Using readl/writel for TICNT solved the kernel hang issue. > > Thanks. > > Best regards, > Kgene. > -- > Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer, > SW Solution Development Team, Samsung Electronics Co., Ltd. > >
MyungJoo Ham wrote: > > On Wed, Oct 27, 2010 at 4:58 PM, Kukjin Kim <kgene.kim@samsung.com> wrote: > > MyungJoo Ham wrote: > >> > >> On Fri, Oct 8, 2010 at 8:41 AM, Kukjin Kim <kgene.kim@samsung.com> wrote: > >> > From: Changhwan Youn <chaos.youn@samsung.com> > >> > > >> > S3C2410_RTCCON of TYPE_S3C64XX RTC should be read/written by > >> > readw and writew, because TYPE_S3C64XX RTC uses bit 8 and 9. > >> > And TYPE_S3C2410 RTC also can access it by readw and writew. > >> > > >> > Signed-off-by: Changhwan Youn <chaos.youn@samsung.com> > >> > [atul.dahiya@samsung.com: tested on smdk2416] > >> > Tested-by: Atul Dahiya <atul.dahiya@samsung.com> > >> > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com> > >> > Cc: Ben Dooks <ben-linux@fluff.org> > >> > --- > >> > drivers/rtc/rtc-s3c.c | 36 ++++++++++++++++++------------------ > >> > 1 files changed, 18 insertions(+), 18 deletions(-) > >> > >> Hello, > >> > > Hi, > >> > >> Sorry for a late reply... > >> > > Yeah, too late :-( > > > >> Anyway, I have a small question in this rtc-s3c.c driver. > >> > >> Is there any reason to use read/write b/w to access registers of rtc-s3c? > >> > > See the git comment. > > > >> Why don't we use readl/writel when accessing registers in this drivers > > > > I don't know why we should use readl/writel for all case... > > even though we can use just word or byte access. > > > >> and just forget which registers require at least 8 or 16 or 32 bits? > >> > >> In fact, it appears that readw/writew accesses 16bits, not 32bits in > > > > Yes... > > > >> ARM machines, which may incur problems with TICCNT(S3C2410_TICNT in the > code)/CURTICCNT > >> registers. > >> > > I can't get your comment... > > It is because TICCNT and CURTICCNT RTC registers of S5PC210 require > 32bit access according to the manual while this patch still uses 16bit > accesses for them. > > Besides, ALMYEAR/BCDYEAR(S3C2410_RTCYEAR) requires at least 16bit access. > > > This issue was found today while testing suspend-to-mem with s5pc210 > and rtc-wakeup loop. My guess is that saving and restoring only 8 bits > of TICNT at suspend/resume function incurred the instable > suspend-to-mem (kernel hangs with about 1/100 probability) Using > readl/writel for TICNT solved the kernel hang issue. > I got it. You mean need to fix readb(TICNT) of s3c_rtc_suspend() and s3c_rtc_resume()... Ok...but it's different some kind of bug fix with this patch. And...need to check side effect to all of Samsung SoCs. Hmm...and first of all, not supported S5PV310/S5PC210 PM in mainline now. Anyway if any bug, we can fix it during -rc. I always welcome to bug fix to our codes. :-) Thanks. Best regards, Kgene. -- Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.
diff --git a/drivers/rtc/rtc-s3c.c b/drivers/rtc/rtc-s3c.c index f57a87f..4a0b875 100644 --- a/drivers/rtc/rtc-s3c.c +++ b/drivers/rtc/rtc-s3c.c @@ -100,7 +100,7 @@ static int s3c_rtc_setpie(struct device *dev, int enabled) spin_lock_irq(&s3c_rtc_pie_lock); if (s3c_rtc_cpu_type == TYPE_S3C64XX) { - tmp = readb(s3c_rtc_base + S3C2410_RTCCON); + tmp = readw(s3c_rtc_base + S3C2410_RTCCON); tmp &= ~S3C64XX_RTCCON_TICEN; if (enabled) @@ -318,7 +318,7 @@ static int s3c_rtc_proc(struct device *dev, struct seq_file *seq) unsigned int ticnt; if (s3c_rtc_cpu_type == TYPE_S3C64XX) { - ticnt = readb(s3c_rtc_base + S3C2410_RTCCON); + ticnt = readw(s3c_rtc_base + S3C2410_RTCCON); ticnt &= S3C64XX_RTCCON_TICEN; } else { ticnt = readb(s3c_rtc_base + S3C2410_TICNT); @@ -391,11 +391,11 @@ static void s3c_rtc_enable(struct platform_device *pdev, int en) return; if (!en) { - tmp = readb(base + S3C2410_RTCCON); + tmp = readw(base + S3C2410_RTCCON); if (s3c_rtc_cpu_type == TYPE_S3C64XX) tmp &= ~S3C64XX_RTCCON_TICEN; tmp &= ~S3C2410_RTCCON_RTCEN; - writeb(tmp, base + S3C2410_RTCCON); + writew(tmp, base + S3C2410_RTCCON); if (s3c_rtc_cpu_type == TYPE_S3C2410) { tmp = readb(base + S3C2410_TICNT); @@ -405,25 +405,25 @@ static void s3c_rtc_enable(struct platform_device *pdev, int en) } else { /* re-enable the device, and check it is ok */ - if ((readb(base+S3C2410_RTCCON) & S3C2410_RTCCON_RTCEN) == 0){ + if ((readw(base+S3C2410_RTCCON) & S3C2410_RTCCON_RTCEN) == 0){ dev_info(&pdev->dev, "rtc disabled, re-enabling\n"); - tmp = readb(base + S3C2410_RTCCON); - writeb(tmp|S3C2410_RTCCON_RTCEN, base+S3C2410_RTCCON); + tmp = readw(base + S3C2410_RTCCON); + writew(tmp|S3C2410_RTCCON_RTCEN, base+S3C2410_RTCCON); } - if ((readb(base + S3C2410_RTCCON) & S3C2410_RTCCON_CNTSEL)){ + if ((readw(base + S3C2410_RTCCON) & S3C2410_RTCCON_CNTSEL)){ dev_info(&pdev->dev, "removing RTCCON_CNTSEL\n"); - tmp = readb(base + S3C2410_RTCCON); - writeb(tmp& ~S3C2410_RTCCON_CNTSEL, base+S3C2410_RTCCON); + tmp = readw(base + S3C2410_RTCCON); + writew(tmp& ~S3C2410_RTCCON_CNTSEL, base+S3C2410_RTCCON); } - if ((readb(base + S3C2410_RTCCON) & S3C2410_RTCCON_CLKRST)){ + if ((readw(base + S3C2410_RTCCON) & S3C2410_RTCCON_CLKRST)){ dev_info(&pdev->dev, "removing RTCCON_CLKRST\n"); - tmp = readb(base + S3C2410_RTCCON); - writeb(tmp & ~S3C2410_RTCCON_CLKRST, base+S3C2410_RTCCON); + tmp = readw(base + S3C2410_RTCCON); + writew(tmp & ~S3C2410_RTCCON_CLKRST, base+S3C2410_RTCCON); } } } @@ -514,8 +514,8 @@ static int __devinit s3c_rtc_probe(struct platform_device *pdev) s3c_rtc_enable(pdev, 1); - pr_debug("s3c2410_rtc: RTCCON=%02x\n", - readb(s3c_rtc_base + S3C2410_RTCCON)); + pr_debug("s3c2410_rtc: RTCCON=%02x\n", + readw(s3c_rtc_base + S3C2410_RTCCON)); device_init_wakeup(&pdev->dev, 1); @@ -578,7 +578,7 @@ static int s3c_rtc_suspend(struct platform_device *pdev, pm_message_t state) /* save TICNT for anyone using periodic interrupts */ ticnt_save = readb(s3c_rtc_base + S3C2410_TICNT); if (s3c_rtc_cpu_type == TYPE_S3C64XX) { - ticnt_en_save = readb(s3c_rtc_base + S3C2410_RTCCON); + ticnt_en_save = readw(s3c_rtc_base + S3C2410_RTCCON); ticnt_en_save &= S3C64XX_RTCCON_TICEN; } s3c_rtc_enable(pdev, 0); @@ -596,8 +596,8 @@ static int s3c_rtc_resume(struct platform_device *pdev) s3c_rtc_enable(pdev, 1); writeb(ticnt_save, s3c_rtc_base + S3C2410_TICNT); if (s3c_rtc_cpu_type == TYPE_S3C64XX && ticnt_en_save) { - tmp = readb(s3c_rtc_base + S3C2410_RTCCON); - writeb(tmp | ticnt_en_save, s3c_rtc_base + S3C2410_RTCCON); + tmp = readw(s3c_rtc_base + S3C2410_RTCCON); + writew(tmp | ticnt_en_save, s3c_rtc_base + S3C2410_RTCCON); } if (device_may_wakeup(&pdev->dev))