linux-mips
[Top] [All Lists]

Re: [PATCH v2 15/26] RTC: Add JZ4740 RTC driver

To: "Lars-Peter Clausen" <lars@metafoo.de>
Subject: Re: [PATCH v2 15/26] RTC: Add JZ4740 RTC driver
From: Marek Vasut <marek.vasut@gmail.com>
Date: Sat, 19 Jun 2010 16:04:25 +0200
Cc: Ralf Baechle <ralf@linux-mips.org>, linux-mips@linux-mips.org, linux-kernel@vger.kernel.org, Alessandro Zummo <a.zummo@towertech.it>, Paul Gortmaker <p_gortmaker@yahoo.com>, Wan ZongShun <mcuos.com@gmail.com>, rtc-linux@googlegroups.com
Dkim-signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:from:organization:to:subject :date:user-agent:cc:references:in-reply-to:mime-version:content-type :content-transfer-encoding:message-id; bh=jsu+sJNEjJZ/XbCo1ffrGEE6CRFGzj3Xb7gHA2z0JjA=; b=NelNXlVX2R0bJ3S0ahshqxxYe0bsSj/v0KG4Ojc2u0FpRiPmEaNE63mEbZtyShbRbq bCbT2OylcBagfDp+/j8Hf7yyJr9/0raEDXX2UlzoAJz6rj3Ivi1+ObsL2aHJp6MRJ0Wk 7arRXQ3byLm4K+iFksg2nBZXHSE6UvWgmmiqI=
Domainkey-signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=from:organization:to:subject:date:user-agent:cc:references :in-reply-to:mime-version:content-type:content-transfer-encoding :message-id; b=XsGUYnOE3LXlMneOc14JeBX2UKkHjEj7uoWwS4S7maNMWDonmLGa3tmJmONlSzuZ4l LgSJCPU1J331hjHwfDKaQIKjet/lhVUaucXLeD3xfaJxxkIosbs8U/LfX6M+SxPn7J9P cOLmfZvjbsJNZH3Sb+fCIRGOU/LMb2aIt901s=
In-reply-to: <4C1CC08E.5050009@metafoo.de>
Organization: Hack&Dev
References: <1276924111-11158-1-git-send-email-lars@metafoo.de> <201006191243.39793.marek.vasut@gmail.com> <4C1CC08E.5050009@metafoo.de>
Sender: linux-mips-bounce@linux-mips.org
User-agent: KMail/1.13.3 (Linux/2.6.33-2-amd64; KDE/4.4.3; x86_64; ; )
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.
> 
> 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.
> 
> >> +
> >> +  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.
> 
> >> +  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 ?
> 
> >> +
> >> +  rtc->rtc = rtc_device_register(pdev->name, &pdev->dev,
> >> &jz4740_rtc_ops, +                                 THIS_MODULE);
> >> +  if (IS_ERR(rtc->rtc)) {
> >> +          ret = PTR_ERR(rtc->rtc);
> >> +          dev_err(&pdev->dev, "Failed to register rtc device: %d\n", ret);
> >> +          goto err_iounmap;
> >> +  }
> >> +
> >> +  ret = request_irq(rtc->irq, jz4740_rtc_irq, 0,
> >> +                          pdev->name, rtc);
> >> +  if (ret) {
> >> +          dev_err(&pdev->dev, "Failed to request rtc irq: %d\n", ret);
> >> +          goto err_unregister_rtc;
> >> +  }
> >> +
> >> +  scratchpad = jz4740_rtc_reg_read(rtc, JZ_REG_RTC_SCRATCHPAD);
> >> +  if (scratchpad != 0x12345678) {
> >> +          jz4740_rtc_reg_write(rtc, JZ_REG_RTC_SCRATCHPAD, 0x12345678);
> >> +          jz4740_rtc_reg_write(rtc, JZ_REG_RTC_SEC, 0);
> >> +  }
> >> +
> >> +  return 0;
> >> +
> >> +err_unregister_rtc:
> >> +  rtc_device_unregister(rtc->rtc);
> >> +err_iounmap:
> >> +  platform_set_drvdata(pdev, NULL);
> >> +  iounmap(rtc->base);
> >> +err_release_mem_region:
> >> +  release_mem_region(rtc->mem->start, resource_size(rtc->mem));
> >> +err_free:
> >> +  kfree(rtc);
> >> +
> >> +  return ret;
> >> +}
> >> +
> >> +static int __devexit jz4740_rtc_remove(struct platform_device *pdev)
> >> +{
> >> +  struct jz4740_rtc *rtc = platform_get_drvdata(pdev);
> > 
> > dev_get_drvdata();
> > 
> >> +
> >> +  free_irq(rtc->irq, rtc);
> >> +
> >> +  rtc_device_unregister(rtc->rtc);
> >> +
> >> +  iounmap(rtc->base);
> >> +  release_mem_region(rtc->mem->start, resource_size(rtc->mem));
> >> +
> >> +  kfree(rtc);
> >> +
> >> +  platform_set_drvdata(pdev, NULL);
> > 
> > DTTO
> > 
> >> +
> >> +  return 0;
> >> +}
> >> +
> >> +struct platform_driver jz4740_rtc_driver = {
> >> +  .probe = jz4740_rtc_probe,
> >> +  .remove = __devexit_p(jz4740_rtc_remove),
> >> +  .driver = {
> >> +          .name = "jz4740-rtc",
> >> +          .owner = THIS_MODULE,
> >> +  },
> >> +};
> >> +
> >> +static int __init jz4740_rtc_init(void)
> >> +{
> >> +  return platform_driver_register(&jz4740_rtc_driver);
> >> +}
> >> +module_init(jz4740_rtc_init);
> >> +
> >> +static void __exit jz4740_rtc_exit(void)
> >> +{
> >> +  platform_driver_unregister(&jz4740_rtc_driver);
> >> +}
> >> +module_exit(jz4740_rtc_exit);
> >> +
> >> +MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
> >> +MODULE_LICENSE("GPL");
> >> +MODULE_DESCRIPTION("RTC driver for the JZ4740 SoC\n");
> >> +MODULE_ALIAS("platform:jz4740-rtc");
> > 
> > Cheers
> 
> Thanks for reviewing
> 
> - Lars

<Prev in Thread] Current Thread [Next in Thread>