Hello.
[email protected] wrote:
From: Sandeep Paulraj <[email protected]>
Patch adds support for the RTC Driver in DM365.
Apart from receiving comments on the driver itself
I would also like comments on where i am registering the
RTC Driver for DM365. At present i register the RTC
device in SOC specific file(dm365.c).
RTC INTMUX should most probably be done in some other way
than what i am doing at the moment.
I Added this just before registering the RTC.
Did some basic testing using this driver. "hwclock"
command works. Also transitions between months/years/leap years
has been verified
Unfortunately, the very first test in rtc-test doesn't work -- program
hangs. I'll now point out the bugs in the driver leading to this...
Signed-off-by: Sandeep Paulraj <[email protected]>
diff --git a/arch/arm/mach-davinci/dm365.c b/arch/arm/mach-davinci/dm365.c
index 4d430a6..bb85129 100644
--- a/arch/arm/mach-davinci/dm365.c
+++ b/arch/arm/mach-davinci/dm365.c
[...]
@@ -593,6 +594,25 @@ static struct platform_device dm365_emac_device = {
.resource = dm365_emac_resources,
};
+static struct resource dm365_rtc_resources[] = {
+ {
+ .start = DM365_RTC_BASE,
+ .end = DM365_RTC_BASE + SZ_1K - 1,
+ .flags = IORESOURCE_MEM,
+ },
+ {
+ .start = 29,
Isn't there a macro for that?
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
[...]
diff --git a/drivers/rtc/rtc-dm365.c b/drivers/rtc/rtc-dm365.c
new file mode 100644
index 0000000..e417678
--- /dev/null
+++ b/drivers/rtc/rtc-dm365.c
@@ -0,0 +1,671 @@
[...]
+
+#define DM365_RTC_BASE 0x01c69000
Shouldn't be here (as already noted)...
[...]
+/* DM365_RTCIF_DMA_CMD_REG bit fields */
+#define DM365_RTCIF_DMA_CMD_BUSY (1<<31)
Please put spaces before and after << to comply with the kernel style.
Or use BIT().
+#define DM365_RTCIF_DMA_CMD_SIZE_2WORD (1<<25)
+#define DM365_RTCIF_DMA_CMD_DIR_READ (1<<24)
+#define DM365_RTCIF_DMA_CMD_BYTEENA1_LSB (1<<20)
+#define DM365_RTCIF_DMA_CMD_BYTEENA1_2ND_LSB (1<<21)
+#define DM365_RTCIF_DMA_CMD_BYTEENA1_3RD_LSB (1<<22)
+#define DM365_RTCIF_DMA_CMD_BYTEENA1_MSB (1<<23)
+#define DM365_RTCIF_DMA_CMD_BYTEENA1_MASK (0x00F00000)
+#define DM365_RTCIF_DMA_CMD_BYTEENA0_LSB (1<<16)
+#define DM365_RTCIF_DMA_CMD_BYTEENA0_2ND_LSB (1<<17)
+#define DM365_RTCIF_DMA_CMD_BYTEENA0_3RD_LSB (1<<18)
+#define DM365_RTCIF_DMA_CMD_BYTEENA0_MSB (1<<19)
+#define DM365_RTCIF_DMA_CMD_BYTEENA0_MASK (0x000F0000)
+
+#define DM365_RTCIF_INT_ENA_RTCSS_INTENA (1<<1)
+#define DM365_RTCIF_INT_ENA_RTCIF_INTENA (1<<0)
An empty line missing?
+#define DM365_RTCIF_INT_FLG_RTCSS_INTFLG (1<<1)
+#define DM365_RTCIF_INT_FLG_RTCIF_INTFLG (1<<0)
[...]
+/* RTCSS_RTC_CTRL_REG bit fields: */
+#define RTCSS_RTC_CTRL_WDTBUS (1<<7)
The bit is called WDTBUSY, not WDTBUS...
+#define rtcif_read(addr) __raw_readl( \
+ (unsigned int *)((u32)dm365_rtc_base + (u32)(addr)))
+#define rtcif_write(val, addr) __raw_writel(val, \
+ (unsigned int *)((u32)dm365_rtc_base + (u32)(addr)))
Should be inline functions...
+static irqreturn_t dm365_rtc_isr(int irq, void *class_dev)
+{
+ unsigned long events = 0;
+ u32 irq_flg;
+
+ irq_flg = rtcif_read(DM365_RTCIF_INT_FLG_REG);
+
+ if ((irq_flg & DM365_RTCIF_INT_FLG_MASK)) {
+ if (irq_flg & DM365_RTCIF_INT_FLG_RTCIF_INTFLG) {
+ rtcif_write(DM365_RTCIF_INT_ENA_RTCSS_INTENA |
+ DM365_RTCIF_INT_ENA_RTCIF_INTENA,
+ DM365_RTCIF_INT_FLG_REG);
+ } else
+ events |= RTC_IRQF | RTC_AF;
I don't see where you set RTC_UF despite handling RTC_UIE_{ON,OFF} ioctl.
+static int dm365_rtc_update_timer(unsigned int cmd)
+{
+ u8 rtc_ctrl;
+
+ rtc_ctrl = rtcss_read_rtc(RTCSS_RTC_CTRL_REG);
+
+ switch (cmd) {
+ case RTC_UIE_ON:
+ while
+ (rtcss_read_rtc(RTCSS_RTC_CTRL_REG) &
+ RTCSS_RTC_CTRL_WDTBUS);
+ rtc_ctrl |= RTCSS_RTC_CTRL_TE;
+ rtcss_write_rtc(rtc_ctrl, RTCSS_RTC_CTRL_REG);
+ rtcss_write_rtc(0x0, RTCSS_RTC_CLKC_CNT);
+ rtc_ctrl |= RTCSS_RTC_CTRL_TIEN | RTCSS_RTC_CTRL_TMMD_FREERUN;
Wait, the source of the update interrupt is not just free-running
counter set to 1 second -- it must be synchronized to the RTC time regisers
update. This timer doesn't have a real update interrupt, so, as David ahve
already suggested, you shouldn't handle these ioctl's, relying on
CONFIG_RTC_DEV_UIE_EMUL intead...
+ rtcss_write_rtc(rtc_ctrl, RTCSS_RTC_CTRL_REG);
+ rtcss_write_rtc(0x80, RTCSS_RTC_TMR0_REG);
+ rtcss_write_rtc(0x0, RTCSS_RTC_TMR1_REG);
It appears that you write the register values in the reverse order --
doesn't this set 256 ms period instead of 1 second?
+static int
+dm365_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
+{
+ u8 rtc_ctrl;
You actually need 2 variables, not one -- see below...
+ unsigned long flags;
+
+ switch (cmd) {
[...]
+ case RTC_IRQP_READ:
+ case RTC_IRQP_SET:
+ case RTC_PIE_OFF:
+ case RTC_PIE_ON:
I don't see where you handle these...
+ spin_lock_irqsave(&dm365_rtc_lock, flags);
+ rtc_ctrl = rtcss_read_rtc(RTCSS_RTC_CCTRL_REG);
You've called the variable rtc_ctrl but are reading from the *CCTRL*
register...
+
+ switch (cmd) {
+ case RTC_AIE_OFF:
+ rtc_ctrl &= ~RTCSS_RTC_CCTRL_AIEN;
+ break;
+ case RTC_AIE_ON:
+ rtc_ctrl |= RTCSS_RTC_CCTRL_AIEN;
+ break;
+ case RTC_WIE_ON:
+ rtc_ctrl |= (RTCSS_RTC_CTRL_WEN | RTCSS_RTC_CTRL_WDTFLG);
+ break;
+ case RTC_WIE_OFF:
+ rtc_ctrl &= ~RTCSS_RTC_CTRL_WEN;
Wait, CTRL is a different register from CCTRL!
+ break;
+ case RTC_UIE_OFF:
+ case RTC_UIE_ON:
+ dm365_rtc_update_timer(cmd);
+ }
+
+ rtcss_write_rtc(rtc_ctrl, RTCSS_RTC_CTRL_REG);
Here you are:
1) writing the value read from the CCTRL into the CTRL register;
2) over-writing what has been just written by dm365_rtc_update_timer() --
effectively negating its effect...
+static int dm365_rtc_read_time(struct device *dev, struct rtc_time *tm)
+{
+ u16 days = 0;
+ u8 day0, day1;
+ unsigned long flags;
+
+ spin_lock_irqsave(&dm365_rtc_lock, flags);
+
+ while
Should be no new line here...
+ (rtcss_read_rtc(RTCSS_RTC_CCTRL_REG) &
+ RTCSS_RTC_CCTRL_CALBUSY);
+
+ tm->tm_sec = bcd2bin(rtcss_read_rtc(RTCSS_RTC_SEC_REG));
+
+ while
... here...
+ (rtcss_read_rtc(RTCSS_RTC_CCTRL_REG) &
+ RTCSS_RTC_CCTRL_CALBUSY);
+
+ tm->tm_min = bcd2bin(rtcss_read_rtc(RTCSS_RTC_MIN_REG));
+
+ while
... here...
+ (rtcss_read_rtc(RTCSS_RTC_CCTRL_REG) &
+ RTCSS_RTC_CCTRL_CALBUSY);
+
+ tm->tm_hour = bcd2bin(rtcss_read_rtc(RTCSS_RTC_HOUR_REG));
+
+ while
... here...
+ (rtcss_read_rtc(RTCSS_RTC_CCTRL_REG) &
+ RTCSS_RTC_CCTRL_CALBUSY);
+
+ day0 = rtcss_read_rtc(RTCSS_RTC_DAY0_REG);
+
+ while
... here...
+static int dm365_rtc_set_time(struct device *dev, struct rtc_time *tm)
+{
+ u16 days;
+ u8 new_cctrl;
+ unsigned long flags;
+
+ if (convert2days(&days, tm) < 0)
+ return -EINVAL;
+
+ spin_lock_irqsave(&dm365_rtc_lock, flags);
+
+ while
... here...
+ (rtcss_read_rtc(RTCSS_RTC_CCTRL_REG) &
+ RTCSS_RTC_CCTRL_CALBUSY);
+
+ rtcss_write_rtc(bin2bcd(tm->tm_sec), RTCSS_RTC_SEC_REG);
+
+ while
... here...
+ (rtcss_read_rtc(RTCSS_RTC_CCTRL_REG) &
+ RTCSS_RTC_CCTRL_CALBUSY);
+
+ rtcss_write_rtc(bin2bcd(tm->tm_min), RTCSS_RTC_MIN_REG);
+
+ while
... here...
+ (rtcss_read_rtc(RTCSS_RTC_CCTRL_REG) &
+ RTCSS_RTC_CCTRL_CALBUSY);
+
+ rtcss_write_rtc(bin2bcd(tm->tm_hour), RTCSS_RTC_HOUR_REG);
+
+ while
... here...
+ (rtcss_read_rtc(RTCSS_RTC_CCTRL_REG) &
+ RTCSS_RTC_CCTRL_CALBUSY);
+
+ rtcss_write_rtc(days & 0xFF, RTCSS_RTC_DAY0_REG);
+
+ while
... and here.
+static int dm365_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
+{
+ u16 days = 0;
+ u8 day0, day1;
+ unsigned long flags;
+
+ spin_lock_irqsave(&dm365_rtc_lock, flags);
+
+ while
Again...
+static int __init dm365_rtc_probe(struct platform_device *pdev)
+{
+ struct resource *res, *mem;
+ struct rtc_device *rtc;
+ u8 new_ctrl = 0;
+
+ dm365_rtc_irq = platform_get_irq(pdev, 0);
+ if (dm365_rtc_irq <= 0) {
+ pr_debug("%s: no RTC irq?\n", pdev->name);
+ return -ENOENT;
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (res && res->start != DM365_RTC_BASE) {
Why check this?
+ pr_debug("%s: RTC registers at %08x, expected %08x\n",
+ pdev->name, (unsigned)res->start, DM365_RTC_BASE);
+ return -ENOENT;
+ }
+
+ dm365_rtc_pbase = res->start;
+ dm365_rtc_base_size = res->end - res->start + 1;
There's resource_size() in <linux/ioport.h> for this now...
+ if (res)
+ mem = request_mem_region(res->start,
+ dm365_rtc_base_size, pdev->name);
+ else
+ mem = NULL;
What's the point of assigning mem right before the test?
+ if (!mem) {
+ pr_debug("%s: RTC registers at %08x are not free\n",
+ pdev->name, DM365_RTC_BASE);
+ return -EBUSY;
+ }
+
+ dm365_rtc_base = ioremap(res->start, dm365_rtc_base_size);
+ if (dm365_rtc_base == NULL) {
+ pr_debug("%s: Can't ioremap MEM resource.\n", pdev->name);
+ goto fail;
+ }
+
+ rtc =
+ rtc_device_register(pdev->name, &pdev->dev, &dm365_rtc_ops,
+ THIS_MODULE);
+ if (IS_ERR(rtc)) {
+ pr_debug("%s: can't register RTC device, err %ld\n",
+ pdev->name, PTR_ERR(rtc));
+ goto fail1;
+ }
+ platform_set_drvdata(pdev, rtc);
+ dev_set_drvdata(&rtc->dev, mem);
+
+ rtcif_write(0, DM365_RTCIF_INT_ENA_REG);
+ rtcif_write(0, DM365_RTCIF_INT_FLG_REG);
+
+ rtcss_write_rtc(0, RTCSS_RTC_CTRL_REG);
+ rtcss_write_rtc(0, RTCSS_RTC_CCTRL_REG);
+
+ if (request_irq(dm365_rtc_irq, dm365_rtc_isr, IRQF_DISABLED,
+ dev_name(&rtc->dev), rtc)) {
+ pr_debug("%s: RTC timer interrupt IRQ%d already claimed\n",
+ pdev->name, dm365_rtc_irq);
+ goto fail0;
+ }
+
+ new_ctrl |= (DM365_RTCIF_INT_ENA_RTCSS_INTENA
+ | DM365_RTCIF_INT_ENA_RTCIF_INTENA);
I'd suggest another, more common style:
new_ctrl |= (DM365_RTCIF_INT_ENA_RTCSS_INTENA |
DM365_RTCIF_INT_ENA_RTCIF_INTENA);
+fail0:
+ rtc_device_unregister(rtc);
+fail1:
+ iounmap(dm365_rtc_base);
+fail:
+ release_mem_region(dm365_rtc_pbase, dm365_rtc_base_size);
Better label names wouldn't hurt too...
WBR, Sergei
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
