linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device
@ 2012-11-29  2:21 Chao Xie
  2012-11-29  2:21 ` [PATCH 2/4] rtc: pxa: fix rtc caculation issue Chao Xie
                   ` (4 more replies)
  0 siblings, 5 replies; 21+ messages in thread
From: Chao Xie @ 2012-11-29  2:21 UTC (permalink / raw)
  To: a.zummo, rtc-linux, linux-kernel, haojian.zhuang,
	linux-arm-kernel, xiechao.mail

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 <chao.xie@marvell.com>
---
 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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 2/4] rtc: pxa: fix rtc caculation issue
  2012-11-29  2:21 [PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device Chao Xie
@ 2012-11-29  2:21 ` Chao Xie
  2012-11-29 20:04   ` Robert Jarzmik
  2012-11-29  2:21 ` [PATCH 3/4] rtc: pxa: add pxa95x rtc support Chao Xie
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 21+ messages in thread
From: Chao Xie @ 2012-11-29  2:21 UTC (permalink / raw)
  To: a.zummo, rtc-linux, linux-kernel, haojian.zhuang,
	linux-arm-kernel, xiechao.mail

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 <chao.xie@marvell.com>
---
 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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 3/4] rtc: pxa: add pxa95x rtc support
  2012-11-29  2:21 [PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device Chao Xie
  2012-11-29  2:21 ` [PATCH 2/4] rtc: pxa: fix rtc caculation issue Chao Xie
@ 2012-11-29  2:21 ` Chao Xie
  2012-12-04  7:03   ` Haojian Zhuang
  2012-11-29  2:21 ` [PATCH 4/4] rtc: pxa: request rtc irqs when probe/remove the device Chao Xie
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 21+ messages in thread
From: Chao Xie @ 2012-11-29  2:21 UTC (permalink / raw)
  To: a.zummo, rtc-linux, linux-kernel, haojian.zhuang,
	linux-arm-kernel, xiechao.mail

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

Signed-off-by: Chao Xie <chao.xie@marvell.com>
---
 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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 4/4] rtc: pxa: request rtc irqs when probe/remove the device
  2012-11-29  2:21 [PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device Chao Xie
  2012-11-29  2:21 ` [PATCH 2/4] rtc: pxa: fix rtc caculation issue Chao Xie
  2012-11-29  2:21 ` [PATCH 3/4] rtc: pxa: add pxa95x rtc support Chao Xie
@ 2012-11-29  2:21 ` Chao Xie
  2012-11-29 10:26   ` Russell King - ARM Linux
  2012-11-29 20:10   ` Robert Jarzmik
  2012-11-29 10:25 ` [PATCH 1/4] rtc: sa1100: enable/disable rtc " Russell King - ARM Linux
  2012-12-03  5:02 ` devendra.aaru
  4 siblings, 2 replies; 21+ messages in thread
From: Chao Xie @ 2012-11-29  2:21 UTC (permalink / raw)
  To: a.zummo, rtc-linux, linux-kernel, haojian.zhuang,
	linux-arm-kernel, xiechao.mail

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 <chao.xie@marvell.com>
---
 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


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device
  2012-11-29  2:21 [PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device Chao Xie
                   ` (2 preceding siblings ...)
  2012-11-29  2:21 ` [PATCH 4/4] rtc: pxa: request rtc irqs when probe/remove the device Chao Xie
@ 2012-11-29 10:25 ` Russell King - ARM Linux
  2012-11-30  7:04   ` Haojian Zhuang
  2012-12-03  5:02 ` devendra.aaru
  4 siblings, 1 reply; 21+ messages in thread
From: Russell King - ARM Linux @ 2012-11-29 10:25 UTC (permalink / raw)
  To: Chao Xie
  Cc: a.zummo, rtc-linux, linux-kernel, haojian.zhuang,
	linux-arm-kernel, xiechao.mail

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/4] rtc: pxa: request rtc irqs when probe/remove the device
  2012-11-29  2:21 ` [PATCH 4/4] rtc: pxa: request rtc irqs when probe/remove the device Chao Xie
@ 2012-11-29 10:26   ` Russell King - ARM Linux
  2012-11-29 20:10   ` Robert Jarzmik
  1 sibling, 0 replies; 21+ messages in thread
From: Russell King - ARM Linux @ 2012-11-29 10:26 UTC (permalink / raw)
  To: Chao Xie
  Cc: a.zummo, rtc-linux, linux-kernel, haojian.zhuang,
	linux-arm-kernel, xiechao.mail

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/4] rtc: pxa: fix rtc caculation issue
  2012-11-29  2:21 ` [PATCH 2/4] rtc: pxa: fix rtc caculation issue Chao Xie
@ 2012-11-29 20:04   ` Robert Jarzmik
  2012-12-03  2:40     ` Chao Xie
  0 siblings, 1 reply; 21+ messages in thread
From: Robert Jarzmik @ 2012-11-29 20:04 UTC (permalink / raw)
  To: Chao Xie
  Cc: a.zummo, rtc-linux, linux-kernel, haojian.zhuang,
	linux-arm-kernel, xiechao.mail

Chao Xie <chao.xie@marvell.com> 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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/4] rtc: pxa: request rtc irqs when probe/remove the device
  2012-11-29  2:21 ` [PATCH 4/4] rtc: pxa: request rtc irqs when probe/remove the device Chao Xie
  2012-11-29 10:26   ` Russell King - ARM Linux
@ 2012-11-29 20:10   ` Robert Jarzmik
  1 sibling, 0 replies; 21+ messages in thread
From: Robert Jarzmik @ 2012-11-29 20:10 UTC (permalink / raw)
  To: Chao Xie
  Cc: a.zummo, rtc-linux, linux-kernel, haojian.zhuang,
	linux-arm-kernel, xiechao.mail

Chao Xie <chao.xie@marvell.com> 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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device
  2012-11-29 10:25 ` [PATCH 1/4] rtc: sa1100: enable/disable rtc " Russell King - ARM Linux
@ 2012-11-30  7:04   ` Haojian Zhuang
  2012-12-03  1:39     ` Chao Xie
  0 siblings, 1 reply; 21+ messages in thread
From: Haojian Zhuang @ 2012-11-30  7:04 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Chao Xie, Alessandro Zummo, rtc-linux, linux-kernel,
	linux-arm-kernel, xie chao

On Thu, Nov 29, 2012 at 6:25 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> 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().

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device
  2012-11-30  7:04   ` Haojian Zhuang
@ 2012-12-03  1:39     ` Chao Xie
  2012-12-03  2:53       ` Chao Xie
  2012-12-03  5:33       ` Haojian Zhuang
  0 siblings, 2 replies; 21+ messages in thread
From: Chao Xie @ 2012-12-03  1:39 UTC (permalink / raw)
  To: Haojian Zhuang
  Cc: Russell King - ARM Linux, Chao Xie, Alessandro Zummo, rtc-linux,
	linux-kernel, linux-arm-kernel

On Fri, Nov 30, 2012 at 3:04 PM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:
> On Thu, Nov 29, 2012 at 6:25 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> 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().

How can user to choose one RTC?
The user API, for example "date" will open the device, get the time
and then close the device.
WHen set a alarm, it is the same routine. open->set->close.
The open routine will enable clock and register irq, close will
disable the clock and unregister irq. It means that if only open is
invoked, rtc begins to work, and after close is invoked rtc stops
working. It means that the user can not get correct time and can not
get right alarm.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/4] rtc: pxa: fix rtc caculation issue
  2012-11-29 20:04   ` Robert Jarzmik
@ 2012-12-03  2:40     ` Chao Xie
  2012-12-04  2:53       ` Chao Xie
  0 siblings, 1 reply; 21+ messages in thread
From: Chao Xie @ 2012-12-03  2:40 UTC (permalink / raw)
  To: Robert Jarzmik
  Cc: Chao Xie, a.zummo, rtc-linux, linux-kernel, haojian.zhuang,
	linux-arm-kernel

On Fri, Nov 30, 2012 at 4:04 AM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
> Chao Xie <chao.xie@marvell.com> 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.

hi
I am sorry, i just use get_maintainer.pl to get the "to" list.
I have go through the spec. The spec has the desctiption about the
invalid data writing.
I am a little confused about the "wrting 0 to DOW". The descrption is
confused. first it said that "If zero is written into any of these
fields, it is ignored
while generating the alarm", then it gives a example, that if writing
0 to DOW, "For example, if a zero is written into a DOW field, the
alarm is set
every day at the time written in the Hours, Minutes, and Seconds
field”. It seems that the Year/Month/Week will not take effect.
I will do the test on the board again, and send out the update.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device
  2012-12-03  1:39     ` Chao Xie
@ 2012-12-03  2:53       ` Chao Xie
  2012-12-03  5:35         ` Haojian Zhuang
  2012-12-03  9:48         ` Russell King - ARM Linux
  2012-12-03  5:33       ` Haojian Zhuang
  1 sibling, 2 replies; 21+ messages in thread
From: Chao Xie @ 2012-12-03  2:53 UTC (permalink / raw)
  To: Haojian Zhuang
  Cc: Russell King - ARM Linux, Chao Xie, Alessandro Zummo, rtc-linux,
	linux-kernel, linux-arm-kernel

On Mon, Dec 3, 2012 at 9:39 AM, Chao Xie <xiechao.mail@gmail.com> wrote:
> On Fri, Nov 30, 2012 at 3:04 PM, Haojian Zhuang
> <haojian.zhuang@gmail.com> wrote:
>> On Thu, Nov 29, 2012 at 6:25 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> 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().
>
> How can user to choose one RTC?
> The user API, for example "date" will open the device, get the time
> and then close the device.
> WHen set a alarm, it is the same routine. open->set->close.
> The open routine will enable clock and register irq, close will
> disable the clock and unregister irq. It means that if only open is
> invoked, rtc begins to work, and after close is invoked rtc stops
> working. It means that the user can not get correct time and can not
> get right alarm.

hi
I want to correct what i said. For the irq register/unregister i think
can be done at open/release. But for clock enable/disable, i do not
think so. If clock is disabled, as i think RTC will not work. User API
still use open->get_time->close for "date" command. It means that RTC
will not return correct date to user.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device
  2012-11-29  2:21 [PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device Chao Xie
                   ` (3 preceding siblings ...)
  2012-11-29 10:25 ` [PATCH 1/4] rtc: sa1100: enable/disable rtc " Russell King - ARM Linux
@ 2012-12-03  5:02 ` devendra.aaru
  4 siblings, 0 replies; 21+ messages in thread
From: devendra.aaru @ 2012-12-03  5:02 UTC (permalink / raw)
  To: Chao Xie
  Cc: Alessandro Zummo, rtc-linux, linux-kernel, haojian.zhuang,
	linux-arm-kernel, xiechao.mail, Andrew Morton

CCing Andrew as he is the maintainer of the rtc subsystem

On Wed, Nov 28, 2012 at 9:21 PM, Chao Xie <chao.xie@marvell.com> 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.
>
> Signed-off-by: Chao Xie <chao.xie@marvell.com>
> ---
>  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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device
  2012-12-03  1:39     ` Chao Xie
  2012-12-03  2:53       ` Chao Xie
@ 2012-12-03  5:33       ` Haojian Zhuang
  1 sibling, 0 replies; 21+ messages in thread
From: Haojian Zhuang @ 2012-12-03  5:33 UTC (permalink / raw)
  To: Chao Xie
  Cc: Russell King - ARM Linux, Chao Xie, Alessandro Zummo, rtc-linux,
	linux-kernel, linux-arm-kernel

On Mon, Dec 3, 2012 at 9:39 AM, Chao Xie <xiechao.mail@gmail.com> wrote:
> On Fri, Nov 30, 2012 at 3:04 PM, Haojian Zhuang
> <haojian.zhuang@gmail.com> wrote:
>> On Thu, Nov 29, 2012 at 6:25 PM, Russell King - ARM Linux
>> <linux@arm.linux.org.uk> 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().
>
> How can user to choose one RTC?

/dev/rtc0 & /dev/rtc1.

The user can choose the rtc by iteself. Is it right?

> The user API, for example "date" will open the device, get the time
> and then close the device.

"date" command is always using /dev/rtc. It's the alias of rtc0 or rtc1.

> WHen set a alarm, it is the same routine. open->set->close.
> The open routine will enable clock and register irq, close will
> disable the clock and unregister irq. It means that if only open is
> invoked, rtc begins to work, and after close is invoked rtc stops
> working. It means that the user can not get correct time and can not
> get right alarm.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device
  2012-12-03  2:53       ` Chao Xie
@ 2012-12-03  5:35         ` Haojian Zhuang
  2012-12-03  5:42           ` Chao Xie
  2012-12-03  9:48         ` Russell King - ARM Linux
  1 sibling, 1 reply; 21+ messages in thread
From: Haojian Zhuang @ 2012-12-03  5:35 UTC (permalink / raw)
  To: Chao Xie
  Cc: Russell King - ARM Linux, Chao Xie, Alessandro Zummo, rtc-linux,
	linux-kernel, linux-arm-kernel

On Mon, Dec 3, 2012 at 10:53 AM, Chao Xie <xiechao.mail@gmail.com> wrote:
> On Mon, Dec 3, 2012 at 9:39 AM, Chao Xie <xiechao.mail@gmail.com> wrote:
>> On Fri, Nov 30, 2012 at 3:04 PM, Haojian Zhuang
>> <haojian.zhuang@gmail.com> wrote:
>>> On Thu, Nov 29, 2012 at 6:25 PM, Russell King - ARM Linux
>>> <linux@arm.linux.org.uk> 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().
>>
>> How can user to choose one RTC?
>> The user API, for example "date" will open the device, get the time
>> and then close the device.
>> WHen set a alarm, it is the same routine. open->set->close.
>> The open routine will enable clock and register irq, close will
>> disable the clock and unregister irq. It means that if only open is
>> invoked, rtc begins to work, and after close is invoked rtc stops
>> working. It means that the user can not get correct time and can not
>> get right alarm.
>
> hi
> I want to correct what i said. For the irq register/unregister i think
> can be done at open/release. But for clock enable/disable, i do not
> think so. If clock is disabled, as i think RTC will not work. User API
> still use open->get_time->close for "date" command. It means that RTC
> will not return correct date to user.

I didn't get your point. Do you mean that RTC can't work without your patch?

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device
  2012-12-03  5:35         ` Haojian Zhuang
@ 2012-12-03  5:42           ` Chao Xie
  0 siblings, 0 replies; 21+ messages in thread
From: Chao Xie @ 2012-12-03  5:42 UTC (permalink / raw)
  To: Haojian Zhuang
  Cc: Russell King - ARM Linux, Chao Xie, Alessandro Zummo, rtc-linux,
	linux-kernel, linux-arm-kernel

On Mon, Dec 3, 2012 at 1:35 PM, Haojian Zhuang <haojian.zhuang@gmail.com> wrote:
> On Mon, Dec 3, 2012 at 10:53 AM, Chao Xie <xiechao.mail@gmail.com> wrote:
>> On Mon, Dec 3, 2012 at 9:39 AM, Chao Xie <xiechao.mail@gmail.com> wrote:
>>> On Fri, Nov 30, 2012 at 3:04 PM, Haojian Zhuang
>>> <haojian.zhuang@gmail.com> wrote:
>>>> On Thu, Nov 29, 2012 at 6:25 PM, Russell King - ARM Linux
>>>> <linux@arm.linux.org.uk> 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().
>>>
>>> How can user to choose one RTC?
>>> The user API, for example "date" will open the device, get the time
>>> and then close the device.
>>> WHen set a alarm, it is the same routine. open->set->close.
>>> The open routine will enable clock and register irq, close will
>>> disable the clock and unregister irq. It means that if only open is
>>> invoked, rtc begins to work, and after close is invoked rtc stops
>>> working. It means that the user can not get correct time and can not
>>> get right alarm.
>>
>> hi
>> I want to correct what i said. For the irq register/unregister i think
>> can be done at open/release. But for clock enable/disable, i do not
>> think so. If clock is disabled, as i think RTC will not work. User API
>> still use open->get_time->close for "date" command. It means that RTC
>> will not return correct date to user.
>
> I didn't get your point. Do you mean that RTC can't work without your patch?
hi
open/release will done two things
1. enable/disable clock
2. register/unregister the irq
i checked the rtc dev code. register/unregister irq is prefered to be
done at open/release routine, and the users will keep rtc to be opened
if he want to make use of the interrupts.
the only problemis the clock. It should be on always.
If user want to get the rtc value, or set alarm. it will  follow
open->ioctrl->release. After release the clock is offed, and i do not
think rtc will has its value updated if the clock is offed.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device
  2012-12-03  2:53       ` Chao Xie
  2012-12-03  5:35         ` Haojian Zhuang
@ 2012-12-03  9:48         ` Russell King - ARM Linux
  2012-12-04  2:51           ` Chao Xie
  1 sibling, 1 reply; 21+ messages in thread
From: Russell King - ARM Linux @ 2012-12-03  9:48 UTC (permalink / raw)
  To: Chao Xie
  Cc: Haojian Zhuang, Chao Xie, Alessandro Zummo, rtc-linux,
	linux-kernel, linux-arm-kernel

On Mon, Dec 03, 2012 at 10:53:07AM +0800, Chao Xie wrote:
> I want to correct what i said. For the irq register/unregister i think
> can be done at open/release. But for clock enable/disable, i do not
> think so. If clock is disabled, as i think RTC will not work. User API
> still use open->get_time->close for "date" command. It means that RTC
> will not return correct date to user.

"I think" is not good enough for patches like this.  Please test it.

On SA11x0 and PXA platforms, the clock for the sa1100-rtc is a dummy
clock; it has no effect.  For MMP, I don't have access to the TRMs so
that's something you're going to have to find out.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device
  2012-12-03  9:48         ` Russell King - ARM Linux
@ 2012-12-04  2:51           ` Chao Xie
  2012-12-04  7:01             ` Haojian Zhuang
  0 siblings, 1 reply; 21+ messages in thread
From: Chao Xie @ 2012-12-04  2:51 UTC (permalink / raw)
  To: Russell King - ARM Linux
  Cc: Haojian Zhuang, Chao Xie, Alessandro Zummo, rtc-linux,
	linux-kernel, linux-arm-kernel

On Mon, Dec 3, 2012 at 5:48 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Mon, Dec 03, 2012 at 10:53:07AM +0800, Chao Xie wrote:
>> I want to correct what i said. For the irq register/unregister i think
>> can be done at open/release. But for clock enable/disable, i do not
>> think so. If clock is disabled, as i think RTC will not work. User API
>> still use open->get_time->close for "date" command. It means that RTC
>> will not return correct date to user.
>
> "I think" is not good enough for patches like this.  Please test it.
>
> On SA11x0 and PXA platforms, the clock for the sa1100-rtc is a dummy
> clock; it has no effect.  For MMP, I don't have access to the TRMs so
> that's something you're going to have to find out.

I tested at pxa910 which uses rtc-sa1100 as driver.
the test procedure is simple
open->ioctl(RTC_RD_TIME)->close
With the original code, the rtc will not update, i always get the same value

remove clock disable/enable in release/open, and enable/disable clock
at probe/remove.
the rtc can update, and i can read the correct rtc value.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 2/4] rtc: pxa: fix rtc caculation issue
  2012-12-03  2:40     ` Chao Xie
@ 2012-12-04  2:53       ` Chao Xie
  0 siblings, 0 replies; 21+ messages in thread
From: Chao Xie @ 2012-12-04  2:53 UTC (permalink / raw)
  To: Robert Jarzmik
  Cc: Chao Xie, a.zummo, rtc-linux, linux-kernel, haojian.zhuang,
	linux-arm-kernel

On Mon, Dec 3, 2012 at 10:40 AM, Chao Xie <xiechao.mail@gmail.com> wrote:
> On Fri, Nov 30, 2012 at 4:04 AM, Robert Jarzmik <robert.jarzmik@free.fr> wrote:
>> Chao Xie <chao.xie@marvell.com> 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.
>
> hi
> I am sorry, i just use get_maintainer.pl to get the "to" list.
> I have go through the spec. The spec has the desctiption about the
> invalid data writing.
> I am a little confused about the "wrting 0 to DOW". The descrption is
> confused. first it said that "If zero is written into any of these
> fields, it is ignored
> while generating the alarm", then it gives a example, that if writing
> 0 to DOW, "For example, if a zero is written into a DOW field, the
> alarm is set
> every day at the time written in the Hours, Minutes, and Seconds
> field”. It seems that the Year/Month/Week will not take effect.
> I will do the test on the board again, and send out the update.

hi, Robert
You are right. it does not matter to set WOM and DOW. please ignore this patch.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device
  2012-12-04  2:51           ` Chao Xie
@ 2012-12-04  7:01             ` Haojian Zhuang
  0 siblings, 0 replies; 21+ messages in thread
From: Haojian Zhuang @ 2012-12-04  7:01 UTC (permalink / raw)
  To: Chao Xie
  Cc: Russell King - ARM Linux, Chao Xie, Alessandro Zummo, rtc-linux,
	linux-kernel, linux-arm-kernel

On Tuesday, December 4, 2012, Chao Xie wrote:
>
> On Mon, Dec 3, 2012 at 5:48 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
> > On Mon, Dec 03, 2012 at 10:53:07AM +0800, Chao Xie wrote:
> >> I want to correct what i said. For the irq register/unregister i think
> >> can be done at open/release. But for clock enable/disable, i do not
> >> think so. If clock is disabled, as i think RTC will not work. User API
> >> still use open->get_time->close for "date" command. It means that RTC
> >> will not return correct date to user.
> >
> > "I think" is not good enough for patches like this.  Please test it.
> >
> > On SA11x0 and PXA platforms, the clock for the sa1100-rtc is a dummy
> > clock; it has no effect.  For MMP, I don't have access to the TRMs so
> > that's something you're going to have to find out.
>
> I tested at pxa910 which uses rtc-sa1100 as driver.
> the test procedure is simple
> open->ioctl(RTC_RD_TIME)->close
> With the original code, the rtc will not update, i always get the same value
>
> remove clock disable/enable in release/open, and enable/disable clock
> at probe/remove.
> the rtc can update, and i can read the correct rtc value.

Yes, clock shouldn't be controlled in open/close. Otherwise, RTC can't
work since
clock is already gated. I didn't find the issue since there're two RTC
enabled in my
board. The default one impacts the test.

I think that you need send the patch that only move clock operation
into probe().

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/4] rtc: pxa: add pxa95x rtc support
  2012-11-29  2:21 ` [PATCH 3/4] rtc: pxa: add pxa95x rtc support Chao Xie
@ 2012-12-04  7:03   ` Haojian Zhuang
  0 siblings, 0 replies; 21+ messages in thread
From: Haojian Zhuang @ 2012-12-04  7:03 UTC (permalink / raw)
  To: Chao Xie
  Cc: Alessandro Zummo, rtc-linux, linux-kernel, linux-arm-kernel, xie chao

On Thu, Nov 29, 2012 at 10:21 AM, Chao Xie <chao.xie@marvell.com> wrote:
> the pxa95x rtc need access PBSR register before write to
> RTTR, RCNR, RDCR, and RYCR registers.
>
> Signed-off-by: Chao Xie <chao.xie@marvell.com>
> ---
>  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())

Don't use cpu_is_xxx() any more. If there's some different thing, we
should parse it
from DTS.

>                 return platform_driver_probe(&pxa_rtc_driver, pxa_rtc_probe);
>
>         return -ENODEV;
> --
> 1.7.4.1
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2012-12-04  7:03 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-11-29  2:21 [PATCH 1/4] rtc: sa1100: enable/disable rtc when probe/remove the device Chao Xie
2012-11-29  2:21 ` [PATCH 2/4] rtc: pxa: fix rtc caculation issue Chao Xie
2012-11-29 20:04   ` Robert Jarzmik
2012-12-03  2:40     ` Chao Xie
2012-12-04  2:53       ` Chao Xie
2012-11-29  2:21 ` [PATCH 3/4] rtc: pxa: add pxa95x rtc support Chao Xie
2012-12-04  7:03   ` Haojian Zhuang
2012-11-29  2:21 ` [PATCH 4/4] rtc: pxa: request rtc irqs when probe/remove the device Chao Xie
2012-11-29 10:26   ` Russell King - ARM Linux
2012-11-29 20:10   ` Robert Jarzmik
2012-11-29 10:25 ` [PATCH 1/4] rtc: sa1100: enable/disable rtc " Russell King - ARM Linux
2012-11-30  7:04   ` Haojian Zhuang
2012-12-03  1:39     ` Chao Xie
2012-12-03  2:53       ` Chao Xie
2012-12-03  5:35         ` Haojian Zhuang
2012-12-03  5:42           ` Chao Xie
2012-12-03  9:48         ` Russell King - ARM Linux
2012-12-04  2:51           ` Chao Xie
2012-12-04  7:01             ` Haojian Zhuang
2012-12-03  5:33       ` Haojian Zhuang
2012-12-03  5:02 ` devendra.aaru

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).