lkml.org 
[lkml]   [2010]   [Jun]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 15/26] RTC: Add JZ4740 RTC driver
    Marek Vasut wrote:
    > Dne So 19. června 2010 15:05:18 Lars-Peter Clausen napsal(a):
    >
    >> Hi
    >>
    >> Marek Vasut wrote:
    >>
    >>> Dne So 19. června 2010 07:08:20 Lars-Peter Clausen napsal(a):
    >>>
    >>>> This patch adds support for the RTC unit on JZ4740 SoCs.
    >>>>
    >>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
    >>>> Cc: Alessandro Zummo <a.zummo@towertech.it>
    >>>> Cc: Paul Gortmaker <p_gortmaker@yahoo.com>
    >>>> Cc: Wan ZongShun <mcuos.com@gmail.com>
    >>>> Cc: Marek Vasut <marek.vasut@gmail.com>
    >>>> Cc: rtc-linux@googlegroups.com
    >>>>
    >>>> ---
    >>>> Changes since v1
    >>>> - Use dev_get_drvdata directly instead of wrapping it in dev_to_rtc
    >>>> - Add common implementation for jz4740_rtc_{alarm,update}_irq_enable
    >>>> - Check whether rtc structure could be allocated
    >>>> - Fix deadlocks which could occur if the HW was broken
    >>>> ---
    >>>>
    >>>> drivers/rtc/Kconfig | 11 ++
    >>>> drivers/rtc/Makefile | 1 +
    >>>> drivers/rtc/rtc-jz4740.c | 341
    >>>>
    >>>> ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 353
    >>>> insertions(+), 0 deletions(-)
    >>>>
    >>>> create mode 100644 drivers/rtc/rtc-jz4740.c
    >>>>
    >>>> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
    >>>> index 10ba12c..d0ed7e6 100644
    >>>> --- a/drivers/rtc/Kconfig
    >>>> +++ b/drivers/rtc/Kconfig
    >>>> @@ -905,4 +905,15 @@ config RTC_DRV_MPC5121
    >>>>
    >>>> This driver can also be built as a module. If so, the module
    >>>> will be called rtc-mpc5121.
    >>>>
    >>>> +config RTC_DRV_JZ4740
    >>>> + tristate "Ingenic JZ4740 SoC"
    >>>> + depends on RTC_CLASS
    >>>> + depends on MACH_JZ4740
    >>>> + help
    >>>> + If you say yes here you get support for the Ingenic JZ4740 SoC RTC
    >>>> + controller.
    >>>> +
    >>>> + This driver can also be buillt as a module. If so, the module
    >>>> + will be called rtc-jz4740.
    >>>> +
    >>>>
    >>>> endif # RTC_CLASS
    >>>>
    >>>> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
    >>>> index 5adbba7..fedf9bb 100644
    >>>> --- a/drivers/rtc/Makefile
    >>>> +++ b/drivers/rtc/Makefile
    >>>> @@ -47,6 +47,7 @@ obj-$(CONFIG_RTC_DRV_EP93XX) += rtc-ep93xx.o
    >>>>
    >>>> obj-$(CONFIG_RTC_DRV_FM3130) += rtc-fm3130.o
    >>>> obj-$(CONFIG_RTC_DRV_GENERIC) += rtc-generic.o
    >>>> obj-$(CONFIG_RTC_DRV_ISL1208) += rtc-isl1208.o
    >>>>
    >>>> +obj-$(CONFIG_RTC_DRV_JZ4740) += rtc-jz4740.o
    >>>>
    >>>> obj-$(CONFIG_RTC_DRV_M41T80) += rtc-m41t80.o
    >>>> obj-$(CONFIG_RTC_DRV_M41T94) += rtc-m41t94.o
    >>>> obj-$(CONFIG_RTC_DRV_M48T35) += rtc-m48t35.o
    >>>>
    >>>> diff --git a/drivers/rtc/rtc-jz4740.c b/drivers/rtc/rtc-jz4740.c
    >>>> new file mode 100644
    >>>> index 0000000..720afb2
    >>>> --- /dev/null
    >>>> +++ b/drivers/rtc/rtc-jz4740.c
    >>>> @@ -0,0 +1,341 @@
    >>>> +/*
    >>>> + * Copyright (C) 2009-2010, Lars-Peter Clausen <lars@metafoo.de>
    >>>> + * JZ4740 SoC RTC driver
    >>>> + *
    >>>> + * This program is free software; you can redistribute it and/or
    >>>> modify it + * under the terms of the GNU General Public License as
    >>>> published by the + * Free Software Foundation; either version 2 of
    >>>> the License, or (at your + * option) any later version.
    >>>> + *
    >>>> + * You should have received a copy of the GNU General Public License
    >>>> along + * with this program; if not, write to the Free Software
    >>>> Foundation, Inc., + * 675 Mass Ave, Cambridge, MA 02139, USA.
    >>>> + *
    >>>> + */
    >>>> +
    >>>> +#include <linux/kernel.h>
    >>>> +#include <linux/module.h>
    >>>> +#include <linux/platform_device.h>
    >>>> +#include <linux/rtc.h>
    >>>> +#include <linux/slab.h>
    >>>> +#include <linux/spinlock.h>
    >>>> +
    >>>> +#define JZ_REG_RTC_CTRL 0x00
    >>>> +#define JZ_REG_RTC_SEC 0x04
    >>>> +#define JZ_REG_RTC_SEC_ALARM 0x08
    >>>> +#define JZ_REG_RTC_REGULATOR 0x0C
    >>>> +#define JZ_REG_RTC_HIBERNATE 0x20
    >>>> +#define JZ_REG_RTC_SCRATCHPAD 0x34
    >>>> +
    >>>> +#define JZ_RTC_CTRL_WRDY BIT(7)
    >>>> +#define JZ_RTC_CTRL_1HZ BIT(6)
    >>>> +#define JZ_RTC_CTRL_1HZ_IRQ BIT(5)
    >>>> +#define JZ_RTC_CTRL_AF BIT(4)
    >>>> +#define JZ_RTC_CTRL_AF_IRQ BIT(3)
    >>>> +#define JZ_RTC_CTRL_AE BIT(2)
    >>>> +#define JZ_RTC_CTRL_ENABLE BIT(0)
    >>>> +
    >>>> +struct jz4740_rtc {
    >>>> + struct resource *mem;
    >>>> + void __iomem *base;
    >>>> +
    >>>> + struct rtc_device *rtc;
    >>>> +
    >>>> + unsigned int irq;
    >>>> +
    >>>> + spinlock_t lock;
    >>>> +};
    >>>> +
    >>>> +static inline uint32_t jz4740_rtc_reg_read(struct jz4740_rtc *rtc,
    >>>> size_t reg) +{
    >>>> + return readl(rtc->base + reg);
    >>>> +}
    >>>> +
    >>>> +static inline void jz4740_rtc_wait_write_ready(struct jz4740_rtc *rtc)
    >>>> +{
    >>>> + uint32_t ctrl;
    >>>> + int timeout = 1000;
    >>>> +
    >>>> + do {
    >>>> + ctrl = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_CTRL);
    >>>> + } while (!(ctrl & JZ_RTC_CTRL_WRDY) && --timeout);
    >>>>
    >>> if (!timeout) {
    >>>
    >>> scream_and_die_in_pain();
    >>> dev_err("I died");
    >>>
    >>> ... or something like that ... what if it times out, in this
    >>> implementation, noone will know this failed.
    >>>
    >>> I haven't looked through the whole source code, but can't this be wrapped
    >>> into the reg_write() ?
    >>> }
    >>>
    >> Well IF it will ever die, you'll notice cause your rtc clock won't work
    >> anymore.
    >>
    >
    > Then maybe some dev_err() would be good to have there.
    >
    That would spam the kernel log. The best solution would be to propagate
    a -EIO up to rtc_ops callers. But I'm not sure if the overhead is worth
    it given that the case will virtually never happen. But I'll think about it.
    >> It could be wrapped into reg_write, but there is a different version of
    >> the SoC with the only difference of the RTC unit being that a different
    >> mechanism is used determine whether it is ok to write or not. So it
    >> makes sense to keep it seperate.
    >>
    >
    > OK
    >
    >>>> +}
    >>>> +
    >>>> +static inline void jz4740_rtc_reg_write(struct jz4740_rtc *rtc, size_t
    >>>> reg, + uint32_t val)
    >>>> +{
    >>>> + jz4740_rtc_wait_write_ready(rtc);
    >>>> + writel(val, rtc->base + reg);
    >>>> +}
    >>>> +
    >>>> +static void jz4740_rtc_ctrl_set_bits(struct jz4740_rtc *rtc, uint32_t
    >>>> mask, + uint32_t val)
    >>>> +{
    >>>> + unsigned long flags;
    >>>> + uint32_t ctrl;
    >>>> +
    >>>> + spin_lock_irqsave(&rtc->lock, flags);
    >>>>
    >>> Can't we use local_irq_save()/local_irq_restore() ?
    >>>
    >> Why would that be preferable? In the non-debug, non-rt case this will
    >> expand to local_irq_{save,restore} anyway, but you'll lose the semantics
    >> of an lock.
    >>
    >
    > I believe on SMP systems, local_irq_save will give you finer locking
    > granularity.
    >
    Hm, not sure about that. But this is on a non SMP system anyway.
    >>>> +
    >>>> + ctrl = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_CTRL);
    >>>> +
    >>>> + /* Don't clear interrupt flags by accident */
    >>>> + ctrl |= JZ_RTC_CTRL_1HZ | JZ_RTC_CTRL_AF;
    >>>> +
    >>>> + ctrl &= ~mask;
    >>>> + ctrl |= val;
    >>>> +
    >>>> + jz4740_rtc_reg_write(rtc, JZ_REG_RTC_CTRL, ctrl);
    >>>> +
    >>>> + spin_unlock_irqrestore(&rtc->lock, flags);
    >>>> +}
    >>>> +
    >>>> +static int jz4740_rtc_read_time(struct device *dev, struct rtc_time
    >>>> *time) +{
    >>>> + struct jz4740_rtc *rtc = dev_get_drvdata(dev);
    >>>> + uint32_t secs, secs2;
    >>>> + int timeout = 5;
    >>>> +
    >>>> + /* If the seconds register is read while it is updated, it can contain
    >>>> a + * bogus value. This can be avoided by making sure that two
    >>>> consecutive + * reads have the same value.
    >>>> + */
    >>>> + secs = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SEC);
    >>>> + secs2 = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SEC);
    >>>> +
    >>>> + while (secs != secs2 && --timeout) {
    >>>> + secs = secs2;
    >>>> + secs2 = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SEC);
    >>>> + }
    >>>> +
    >>>> + if (timeout == 0)
    >>>> + return -EIO;
    >>>> +
    >>>> + rtc_time_to_tm(secs, time);
    >>>> +
    >>>> + return rtc_valid_tm(time);
    >>>> +}
    >>>> +
    >>>> +static int jz4740_rtc_set_mmss(struct device *dev, unsigned long secs)
    >>>> +{
    >>>> + struct jz4740_rtc *rtc = dev_get_drvdata(dev);
    >>>> +
    >>>> + if ((uint32_t)secs != secs)
    >>>> + return -EINVAL;
    >>>>
    >>> Is the typecast here necessary ?
    >>>
    >> Strictly speaking not.
    >>
    >
    > OK
    >
    >>>> +
    >>>> + jz4740_rtc_reg_write(rtc, JZ_REG_RTC_SEC, secs);
    >>>> +
    >>>> + return 0;
    >>>> +}
    >>>> +
    >>>> +static int jz4740_rtc_read_alarm(struct device *dev, struct rtc_wkalrm
    >>>> *alrm) +{
    >>>> + struct jz4740_rtc *rtc = dev_get_drvdata(dev);
    >>>> + uint32_t secs;
    >>>> + uint32_t ctrl;
    >>>> +
    >>>> + secs = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SEC_ALARM);
    >>>> +
    >>>> + ctrl = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_CTRL);
    >>>> +
    >>>> + alrm->enabled = !!(ctrl & JZ_RTC_CTRL_AE);
    >>>> + alrm->pending = !!(ctrl & JZ_RTC_CTRL_AF);
    >>>> +
    >>>>
    >>> Is the double negation (!!) here necessary ?
    >>>
    >> To quote rtc.h "/* 0 = alarm disabled, 1 = alarm enabled */", so yes.
    >>
    >
    > Oh my ... well, maybe someone should fix that to take 0 for NO, !0 for YES.
    >
    Well, it's part of the userspace api.
    >>>> + rtc_time_to_tm(secs, &alrm->time);
    >>>> +
    >>>> + return rtc_valid_tm(&alrm->time);
    >>>> +}
    >>>> +
    >>>> +static int jz4740_rtc_set_alarm(struct device *dev, struct rtc_wkalrm
    >>>> *alrm) +{
    >>>> + struct jz4740_rtc *rtc = dev_get_drvdata(dev);
    >>>> + unsigned long secs;
    >>>> +
    >>>> + rtc_tm_to_time(&alrm->time, &secs);
    >>>> +
    >>>> + if ((uint32_t)secs != secs)
    >>>> + return -EINVAL;
    >>>>
    >>> DTTO above
    >>>
    >>>
    >>>> +
    >>>> + jz4740_rtc_reg_write(rtc, JZ_REG_RTC_SEC_ALARM, (uint32_t)secs);
    >>>>
    >>> DTTO
    >>>
    >>>
    >>>> + jz4740_rtc_ctrl_set_bits(rtc, JZ_RTC_CTRL_AE,
    >>>> + alrm->enabled ? JZ_RTC_CTRL_AE : 0);
    >>>>
    >>> Possibly the double negation above wasn't necessary
    >>>
    >>>
    >>>> +
    >>>> + return 0;
    >>>> +}
    >>>> +
    >>>> +static inline int jz4740_irq_enable(struct device *dev, int irq,
    >>>> + unsigned int enable)
    >>>> +{
    >>>> + struct jz4740_rtc *rtc = dev_get_drvdata(dev);
    >>>> + jz4740_rtc_ctrl_set_bits(rtc, irq, enable ? irq : 0);
    >>>> +
    >>>> + return 0;
    >>>> +}
    >>>> +
    >>>> +static int jz4740_rtc_update_irq_enable(struct device *dev, unsigned
    >>>> int enable) +{
    >>>> + return jz4740_irq_enable(dev, JZ_RTC_CTRL_1HZ_IRQ, enable);
    >>>> +}
    >>>> +
    >>>> +static int jz4740_rtc_alarm_irq_enable(struct device *dev, unsigned int
    >>>> enable) +{
    >>>> + return jz4740_irq_enable(dev, JZ_RTC_CTRL_AF_IRQ, enable);
    >>>> +}
    >>>> +
    >>>> +static struct rtc_class_ops jz4740_rtc_ops = {
    >>>> + .read_time = jz4740_rtc_read_time,
    >>>> + .set_mmss = jz4740_rtc_set_mmss,
    >>>> + .read_alarm = jz4740_rtc_read_alarm,
    >>>> + .set_alarm = jz4740_rtc_set_alarm,
    >>>> + .update_irq_enable = jz4740_rtc_update_irq_enable,
    >>>> + .alarm_irq_enable = jz4740_rtc_alarm_irq_enable,
    >>>> +};
    >>>> +
    >>>> +static irqreturn_t jz4740_rtc_irq(int irq, void *data)
    >>>> +{
    >>>> + struct jz4740_rtc *rtc = data;
    >>>> + uint32_t ctrl;
    >>>> + unsigned long events = 0;
    >>>> + ctrl = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_CTRL);
    >>>> +
    >>>> + if (ctrl & JZ_RTC_CTRL_1HZ)
    >>>> + events |= (RTC_UF | RTC_IRQF);
    >>>> +
    >>>> + if (ctrl & JZ_RTC_CTRL_AF)
    >>>> + events |= (RTC_AF | RTC_IRQF);
    >>>> +
    >>>> + rtc_update_irq(rtc->rtc, 1, events);
    >>>> +
    >>>> + jz4740_rtc_ctrl_set_bits(rtc, JZ_RTC_CTRL_1HZ | JZ_RTC_CTRL_AF, 0);
    >>>> +
    >>>> + return IRQ_HANDLED;
    >>>> +}
    >>>> +
    >>>> +void jz4740_rtc_poweroff(struct device *dev)
    >>>> +{
    >>>> + struct jz4740_rtc *rtc = dev_get_drvdata(dev);
    >>>> + jz4740_rtc_reg_write(rtc, JZ_REG_RTC_HIBERNATE, 1);
    >>>> +}
    >>>> +EXPORT_SYMBOL_GPL(jz4740_rtc_poweroff);
    >>>> +
    >>>> +static int __devinit jz4740_rtc_probe(struct platform_device *pdev)
    >>>> +{
    >>>> + int ret;
    >>>> + struct jz4740_rtc *rtc;
    >>>> + uint32_t scratchpad;
    >>>> +
    >>>> + rtc = kmalloc(sizeof(*rtc), GFP_KERNEL);
    >>>> + if (!rtc)
    >>>> + return -ENOMEM;
    >>>> +
    >>>> + rtc->irq = platform_get_irq(pdev, 0);
    >>>> + if (rtc->irq < 0) {
    >>>> + ret = -ENOENT;
    >>>> + dev_err(&pdev->dev, "Failed to get platform irq\n");
    >>>> + goto err_free;
    >>>> + }
    >>>> +
    >>>> + rtc->mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
    >>>> + if (!rtc->mem) {
    >>>> + ret = -ENOENT;
    >>>> + dev_err(&pdev->dev, "Failed to get platform mmio memory\n");
    >>>> + goto err_free;
    >>>> + }
    >>>> +
    >>>> + rtc->mem = request_mem_region(rtc->mem->start,
    >>>> resource_size(rtc->mem), + pdev->name);
    >>>> + if (!rtc->mem) {
    >>>> + ret = -EBUSY;
    >>>> + dev_err(&pdev->dev, "Failed to request mmio memory region\n");
    >>>> + goto err_free;
    >>>> + }
    >>>> +
    >>>> + rtc->base = ioremap_nocache(rtc->mem->start, resource_size(rtc->mem));
    >>>> + if (!rtc->base) {
    >>>> + ret = -EBUSY;
    >>>> + dev_err(&pdev->dev, "Failed to ioremap mmio memory\n");
    >>>> + goto err_release_mem_region;
    >>>> + }
    >>>> +
    >>>> + spin_lock_init(&rtc->lock);
    >>>> +
    >>>> + platform_set_drvdata(pdev, rtc);
    >>>>
    >>> dev_set_drvdata()?
    >>>
    >> No.
    >>
    >
    > Why not ?
    >
    platform_set_drvdata(pdev, data) expands to dev_set_drvdata(&(pdev)->dev, data)

    - Lars
    --
    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/

    \
     
     \ /
      Last update: 2010-06-19 19:45    [W:3.043 / U:0.156 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site