2012-11-29 02:21:16

by Chao Xie

[permalink] [raw]
Subject: [PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device

The original sa1100_rtc_open/sa1100_rtc_release will be called
when the /dev/rtc0 is opened or closed.
In fact, these two functions will enable/disable the clock, and
register/unregister the irqs.
User application will use /dev/rtc0 to read the rtc time or set
the alarm. The rtc should still run indepent of open/close the
rtc device.
So only enable clock and register the irqs when probe the device,
and disable clock and unregister the irqs when remove the device.

Signed-off-by: Chao Xie <[email protected]>
---
drivers/rtc/rtc-sa1100.c | 9 +++++++--
1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-sa1100.c b/drivers/rtc/rtc-sa1100.c
index 50a5c4a..e933327 100644
--- a/drivers/rtc/rtc-sa1100.c
+++ b/drivers/rtc/rtc-sa1100.c
@@ -218,8 +218,6 @@ static int sa1100_rtc_proc(struct device *dev, struct seq_file *seq)
}

static const struct rtc_class_ops sa1100_rtc_ops = {
- .open = sa1100_rtc_open,
- .release = sa1100_rtc_release,
.read_time = sa1100_rtc_read_time,
.set_time = sa1100_rtc_set_time,
.read_alarm = sa1100_rtc_read_alarm,
@@ -253,6 +251,10 @@ static int sa1100_rtc_probe(struct platform_device *pdev)
spin_lock_init(&info->lock);
platform_set_drvdata(pdev, info);

+ ret = sa1100_rtc_open(&pdev->dev);
+ if (ret)
+ goto err_open;
+
/*
* According to the manual we should be able to let RTTR be zero
* and then a default diviser for a 32.768KHz clock is used.
@@ -305,7 +307,9 @@ static int sa1100_rtc_probe(struct platform_device *pdev)

return 0;
err_dev:
+ sa1100_rtc_release(&pdev->dev);
platform_set_drvdata(pdev, NULL);
+err_open:
clk_put(info->clk);
err_clk:
kfree(info);
@@ -318,6 +322,7 @@ static int sa1100_rtc_remove(struct platform_device *pdev)

if (info) {
rtc_device_unregister(info->rtc);
+ sa1100_rtc_release(&pdev->dev);
clk_put(info->clk);
platform_set_drvdata(pdev, NULL);
kfree(info);
--
1.7.4.1


2012-11-29 02:21:36

by Chao Xie

[permalink] [raw]
Subject: [PATCH 2/4] rtc: pxa: fix rtc caculation issue

The original calculation does not take care of week-of-month and
day-of-week. It will make rdar/rdcr not matched when set the
alarm.

Signed-off-by: Chao Xie <[email protected]>
---
drivers/rtc/rtc-pxa.c | 10 +++++++++-
1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/drivers/rtc/rtc-pxa.c b/drivers/rtc/rtc-pxa.c
index f771b2e..22ea4f5 100644
--- a/drivers/rtc/rtc-pxa.c
+++ b/drivers/rtc/rtc-pxa.c
@@ -62,6 +62,10 @@
#define RYxR_MONTH_S 5
#define RYxR_MONTH_MASK (0xf << RYxR_MONTH_S)
#define RYxR_DAY_MASK 0x1f
+#define RDxR_WOM_S 20
+#define RDxR_WOM_MASK (0x7 << RDxR_WOM_S)
+#define RDxR_DOW_S 17
+#define RDxR_DOW_MASK (0x7 << RDxR_DOW_S)
#define RDxR_HOUR_S 12
#define RDxR_HOUR_MASK (0x1f << RDxR_HOUR_S)
#define RDxR_MIN_S 6
@@ -100,7 +104,10 @@ static u32 ryxr_calc(struct rtc_time *tm)

static u32 rdxr_calc(struct rtc_time *tm)
{
- return (tm->tm_hour << RDxR_HOUR_S) | (tm->tm_min << RDxR_MIN_S)
+ return ((((tm->tm_mday + 6) / 7) << RDxR_WOM_S) & RDxR_WOM_MASK)
+ | (((tm->tm_wday + 1) << RDxR_DOW_S) & RDxR_DOW_MASK)
+ | (tm->tm_hour << RDxR_HOUR_S)
+ | (tm->tm_min << RDxR_MIN_S)
| tm->tm_sec;
}

@@ -109,6 +116,7 @@ static void tm_calc(u32 rycr, u32 rdcr, struct rtc_time *tm)
tm->tm_year = ((rycr & RYxR_YEAR_MASK) >> RYxR_YEAR_S) - 1900;
tm->tm_mon = (((rycr & RYxR_MONTH_MASK) >> RYxR_MONTH_S)) - 1;
tm->tm_mday = (rycr & RYxR_DAY_MASK);
+ tm->tm_wday = ((rdcr & RDxR_DOW_MASK) >> RDxR_DOW_S) - 1;
tm->tm_hour = (rdcr & RDxR_HOUR_MASK) >> RDxR_HOUR_S;
tm->tm_min = (rdcr & RDxR_MIN_MASK) >> RDxR_MIN_S;
tm->tm_sec = rdcr & RDxR_SEC_MASK;
--
1.7.4.1

2012-11-29 02:21:47

by Chao Xie

[permalink] [raw]
Subject: [PATCH 3/4] rtc: pxa: add pxa95x rtc support

the pxa95x rtc need access PBSR register before write to
RTTR, RCNR, RDCR, and RYCR registers.

Signed-off-by: Chao Xie <[email protected]>
---
drivers/rtc/rtc-pxa.c | 97 +++++++++++++++++++++++++++++++++++++++++++------
1 files changed, 85 insertions(+), 12 deletions(-)

diff --git a/drivers/rtc/rtc-pxa.c b/drivers/rtc/rtc-pxa.c
index 22ea4f5..29646af 100644
--- a/drivers/rtc/rtc-pxa.c
+++ b/drivers/rtc/rtc-pxa.c
@@ -29,6 +29,7 @@
#include <linux/slab.h>
#include <linux/of.h>
#include <linux/of_device.h>
+#include <linux/delay.h>

#include <mach/hardware.h>

@@ -81,14 +82,28 @@
#define RTCPICR 0x34
#define PIAR 0x38

+#define PSBR_RTC 0x00
+
#define rtc_readl(pxa_rtc, reg) \
__raw_readl((pxa_rtc)->base + (reg))
#define rtc_writel(pxa_rtc, reg, value) \
__raw_writel((value), (pxa_rtc)->base + (reg))
+#define rtc_readl_psbr(pxa_rtc, reg) \
+ __raw_readl((pxa_rtc)->base_psbr + (reg))
+#define rtc_writel_psbr(pxa_rtc, reg, value) \
+ __raw_writel((value), (pxa_rtc)->base_psbr + (reg))
+
+enum {
+ RTC_PXA27X,
+ RTC_PXA95X,
+};

struct pxa_rtc {
struct resource *ress;
+ struct resource *ress_psbr;
+ unsigned int id;
void __iomem *base;
+ void __iomem *base_psbr;
int irq_1Hz;
int irq_Alrm;
struct rtc_device *rtc;
@@ -250,9 +265,26 @@ static int pxa_rtc_set_time(struct device *dev, struct rtc_time *tm)
{
struct pxa_rtc *pxa_rtc = dev_get_drvdata(dev);

+ /*
+ * sequence to wirte pxa rtc register RCNR RDCR RYCR is
+ * 1. set PSBR[RWE] bit, take 2x32-khz to complete
+ * 2. write to RTC register,take 2x32-khz to complete
+ * 3. clear PSBR[RWE] bit,take 2x32-khz to complete
+ */
+ if (pxa_rtc->id == RTC_PXA95X) {
+ rtc_writel_psbr(pxa_rtc, PSBR_RTC, 0x01);
+ udelay(100);
+ }
+
rtc_writel(pxa_rtc, RYCR, ryxr_calc(tm));
rtc_writel(pxa_rtc, RDCR, rdxr_calc(tm));

+ if (pxa_rtc->id == RTC_PXA95X) {
+ udelay(100);
+ rtc_writel_psbr(pxa_rtc, PSBR_RTC, 0x00);
+ udelay(100);
+ }
+
return 0;
}

@@ -318,6 +350,20 @@ static const struct rtc_class_ops pxa_rtc_ops = {
.proc = pxa_rtc_proc,
};

+static struct of_device_id pxa_rtc_dt_ids[] = {
+ { .compatible = "marvell,pxa-rtc", .data = (void *)RTC_PXA27X },
+ { .compatible = "marvell,pxa95x-rtc", .data = (void *)RTC_PXA95X },
+ {}
+};
+MODULE_DEVICE_TABLE(of, pxa_rtc_dt_ids);
+
+static const struct platform_device_id pxa_rtc_id_table[] = {
+ { "pxa-rtc", RTC_PXA27X },
+ { "pxa95x-rtc", RTC_PXA95X },
+ { },
+};
+MODULE_DEVICE_TABLE(platform, pxa_rtc_id_table);
+
static int __init pxa_rtc_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -332,13 +378,34 @@ static int __init pxa_rtc_probe(struct platform_device *pdev)
spin_lock_init(&pxa_rtc->lock);
platform_set_drvdata(pdev, pxa_rtc);

+ if (pdev->dev.of_node) {
+ const struct of_device_id *of_id =
+ of_match_device(pxa_rtc_dt_ids, &pdev->dev);
+
+ pxa_rtc->id = (unsigned int)(of_id->data);
+ } else {
+ const struct platform_device_id *id =
+ platform_get_device_id(pdev);
+
+ pxa_rtc->id = id->driver_data;
+ }
+
ret = -ENXIO;
pxa_rtc->ress = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!pxa_rtc->ress) {
- dev_err(dev, "No I/O memory resource defined\n");
+ dev_err(dev, "No I/O memory resource(id=0) defined\n");
goto err_ress;
}

+ if (pxa_rtc->id == RTC_PXA95X) {
+ pxa_rtc->ress_psbr =
+ platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (!pxa_rtc->ress_psbr) {
+ dev_err(dev, "No I/O memory resource(id=1) defined\n");
+ goto err_ress;
+ }
+ }
+
pxa_rtc->irq_1Hz = platform_get_irq(pdev, 0);
if (pxa_rtc->irq_1Hz < 0) {
dev_err(dev, "No 1Hz IRQ resource defined\n");
@@ -355,7 +422,17 @@ static int __init pxa_rtc_probe(struct platform_device *pdev)
resource_size(pxa_rtc->ress));
if (!pxa_rtc->base) {
dev_err(&pdev->dev, "Unable to map pxa RTC I/O memory\n");
- goto err_map;
+ goto err_map_base;
+ }
+
+ if (pxa_rtc->id == RTC_PXA95X) {
+ pxa_rtc->base_psbr = ioremap(pxa_rtc->ress_psbr->start,
+ resource_size(pxa_rtc->ress_psbr));
+ if (!pxa_rtc->base_psbr) {
+ dev_err(&pdev->dev,
+ "Unable to map pxa RTC PSBR I/O memory\n");
+ goto err_map_base_psbr;
+ }
}

/*
@@ -384,9 +461,12 @@ static int __init pxa_rtc_probe(struct platform_device *pdev)
return 0;

err_rtc_reg:
+ if (pxa_rtc->id == RTC_PXA95X)
+ iounmap(pxa_rtc->base_psbr);
+err_map_base_psbr:
iounmap(pxa_rtc->base);
+err_map_base:
err_ress:
-err_map:
kfree(pxa_rtc);
return ret;
}
@@ -406,14 +486,6 @@ static int __exit pxa_rtc_remove(struct platform_device *pdev)
return 0;
}

-#ifdef CONFIG_OF
-static struct of_device_id pxa_rtc_dt_ids[] = {
- { .compatible = "marvell,pxa-rtc" },
- {}
-};
-MODULE_DEVICE_TABLE(of, pxa_rtc_dt_ids);
-#endif
-
#ifdef CONFIG_PM
static int pxa_rtc_suspend(struct device *dev)
{
@@ -448,11 +520,12 @@ static struct platform_driver pxa_rtc_driver = {
.pm = &pxa_rtc_pm_ops,
#endif
},
+ .id_table = pxa_rtc_id_table,
};

static int __init pxa_rtc_init(void)
{
- if (cpu_is_pxa27x() || cpu_is_pxa3xx())
+ if (cpu_is_pxa27x() || cpu_is_pxa3xx() || cpu_is_pxa95x())
return platform_driver_probe(&pxa_rtc_driver, pxa_rtc_probe);

return -ENODEV;
--
1.7.4.1

2012-11-29 02:25:56

by Chao Xie

[permalink] [raw]
Subject: [PATCH 4/4] rtc: pxa: request rtc irqs when probe/remove the device

The original pxa_rtc_open/pxa_rtc_release will be called
when the /dev/rtc0 is opened or closed.
In fact, these two functions will register/unregister the irqs.
User application will use /dev/rtc0 to read the rtc time or set
the alarm. The rtc should still run indepent of open/close the
rtc device.
So only register the irqs when probe the device,
and disable clock and unregister the irqs when remove the device.

Signed-off-by: Chao Xie <[email protected]>
---
drivers/rtc/rtc-pxa.c | 11 +++++++++--
1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-pxa.c b/drivers/rtc/rtc-pxa.c
index 29646af..19abeb8 100644
--- a/drivers/rtc/rtc-pxa.c
+++ b/drivers/rtc/rtc-pxa.c
@@ -340,8 +340,6 @@ static int pxa_rtc_proc(struct device *dev, struct seq_file *seq)
}

static const struct rtc_class_ops pxa_rtc_ops = {
- .open = pxa_rtc_open,
- .release = pxa_rtc_release,
.read_time = pxa_rtc_read_time,
.set_time = pxa_rtc_set_time,
.read_alarm = pxa_rtc_read_alarm,
@@ -435,6 +433,11 @@ static int __init pxa_rtc_probe(struct platform_device *pdev)
}
}

+ ret = pxa_rtc_open(&pdev->dev);
+ if (ret) {
+ dev_err(&pdev->dev, "Unable to request irqs for rtc\n");
+ goto err_open;
+ }
/*
* If the clock divider is uninitialized then reset it to the
* default value to get the 1Hz clock.
@@ -461,6 +464,8 @@ static int __init pxa_rtc_probe(struct platform_device *pdev)
return 0;

err_rtc_reg:
+ pxa_rtc_release(&pdev->dev);
+err_open:
if (pxa_rtc->id == RTC_PXA95X)
iounmap(pxa_rtc->base_psbr);
err_map_base_psbr:
@@ -477,6 +482,8 @@ static int __exit pxa_rtc_remove(struct platform_device *pdev)

rtc_device_unregister(pxa_rtc->rtc);

+ pxa_rtc_release(&pdev->dev);
+
spin_lock_irq(&pxa_rtc->lock);
iounmap(pxa_rtc->base);
spin_unlock_irq(&pxa_rtc->lock);
--
1.7.4.1

2012-11-29 10:26:59

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device

On Wed, Nov 28, 2012 at 09:21:07PM -0500, Chao Xie wrote:
> The original sa1100_rtc_open/sa1100_rtc_release will be called
> when the /dev/rtc0 is opened or closed.
> In fact, these two functions will enable/disable the clock, and
> register/unregister the irqs.
> User application will use /dev/rtc0 to read the rtc time or set
> the alarm. The rtc should still run indepent of open/close the
> rtc device.
> So only enable clock and register the irqs when probe the device,
> and disable clock and unregister the irqs when remove the device.

NAK. I don't think you properly understand what's going on here if you
think moving the entire open and release functions into the probe and
remove functions is the right thing to do.

2012-11-29 10:28:06

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: [PATCH 4/4] rtc: pxa: request rtc irqs when probe/remove the device

On Wed, Nov 28, 2012 at 09:21:10PM -0500, Chao Xie wrote:
> The original pxa_rtc_open/pxa_rtc_release will be called
> when the /dev/rtc0 is opened or closed.
> In fact, these two functions will register/unregister the irqs.
> User application will use /dev/rtc0 to read the rtc time or set
> the alarm. The rtc should still run indepent of open/close the
> rtc device.
> So only register the irqs when probe the device,
> and disable clock and unregister the irqs when remove the device.

Again, this is wrong.

2012-11-29 20:04:25

by Robert Jarzmik

[permalink] [raw]
Subject: Re: [PATCH 2/4] rtc: pxa: fix rtc caculation issue

Chao Xie <[email protected]> writes:

Hi Chao Xie,

First of all, could you please send patches from rtc-pxa to me also, as I'm
maintaining that driver ?

Second point, the original design of the driver relies on the special case of
writing zeroes to WOM and DOM, as mentionned in PXA27x Developers Guide, chapter
21.4.2.3.5 "Writing Alarm Registers with Invalid (Zero) Data", which states :
> Day-Of-Week (DOW), or Week-Of-Month (WOM), Day of Month (DOM), Month or Year
> fields—Zero is not valid for these fields. If zero is written into any of
> these fields, it is ignored while generating the alarm.

I'd like to know if your patch fixes something, or is an enhancement ?

Cheers.

--
Robert

PS: I've not checked the patch yet, that's just a prelimary comment on the patch
message.

2012-11-29 20:10:44

by Robert Jarzmik

[permalink] [raw]
Subject: Re: [PATCH 4/4] rtc: pxa: request rtc irqs when probe/remove the device

Chao Xie <[email protected]> writes:

> The original pxa_rtc_open/pxa_rtc_release will be called
> when the /dev/rtc0 is opened or closed.
> In fact, these two functions will register/unregister the irqs.
> User application will use /dev/rtc0 to read the rtc time or set
> the alarm. The rtc should still run indepent of open/close the
> rtc device.
> So only register the irqs when probe the device,
> and disable clock and unregister the irqs when remove the device.
No, as Russell I think that's not correct.

This is not how RTC API should be used. And on top of RTC API considerations,
consider this : today, rtc-pxa and rtc-sa1100 _can_ coexist on a PXA platform,
and be used alternatively (I know, it's a bit borderline because of IO space,
but anyway it does work). How will that be possible with your patch ?

Cheers.

--
Robert

2012-11-30 07:04:15

by Haojian Zhuang

[permalink] [raw]
Subject: Re: [PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device

On Thu, Nov 29, 2012 at 6:25 PM, Russell King - ARM Linux
<[email protected]> wrote:
> On Wed, Nov 28, 2012 at 09:21:07PM -0500, Chao Xie wrote:
>> The original sa1100_rtc_open/sa1100_rtc_release will be called
>> when the /dev/rtc0 is opened or closed.
>> In fact, these two functions will enable/disable the clock, and
>> register/unregister the irqs.
>> User application will use /dev/rtc0 to read the rtc time or set
>> the alarm. The rtc should still run indepent of open/close the
>> rtc device.
>> So only enable clock and register the irqs when probe the device,
>> and disable clock and unregister the irqs when remove the device.
>
> NAK. I don't think you properly understand what's going on here if you
> think moving the entire open and release functions into the probe and
> remove functions is the right thing to do.

Since PXA27x & PXA3xx supports dual rtc device at the same time,
user could choose use either of rtc at run time. Then clk & irq are setup
in open().

Chao,
So you shouldn't remove them into probe().