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

[Lars-Peter Clausen <lars@metafoo.de>]

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