Re: [rtc-linux] [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

Re: [rtc-linux] [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Eddie Huang]

Hi Sascha,

On Fri, 2015-03-13 at 11:57 +0100, Sascha Hauer wrote:
> Hi Eddie,
> 
> On Fri, Mar 13, 2015 at 06:29:23PM +0800, Eddie Huang wrote:
> > > regmap_read() and regmap_write() can return errors.  There is no
> > > checking for this.
> > > 
> > 
> > I encounter some trouble when I add code to check return value of
> > regmap_read and regmap_write. Every RTC register access through regmap,
> > and there are many register read/write in this driver. If I check every
> > return value, the driver will become ugly. I try to make this driver
> > clean using following macro.
> > 
> > static int __rtc_read(struct mt6397_rtc *rtc, u32 offset, u32 *data)
> > {
> >         u32 addr = rtc->addr_base + offset;
> > 
> >         if (offset < rtc->addr_range)
> >                 return regmap_read(rtc->regmap, addr, data);
> > 
> >         return -EINVAL;
> > }
> > 
> > #define rtc_read(ret, rtc, offset, data)                \
> > ({                                                      \
> >         ret = __rtc_read(rtc, offset, data);            \
> >         if (ret < 0)                                    \
> >                 goto rtc_exit;                          \
> > })                                                      \
> 
> Hiding a goto (or return) in a macro is a very bad idea.
> 
> what you can do is
> 
> 	ret |= regmap_read(rtc->regmap, RTC_TC_SEC, &tm->tm_sec);
> 	ret |= regmap_read(rtc->regmap, RTC_TC_MIN, &tm->tm_min);
> 
> 	if (ret)
> 		return -EIO;
> 
> (Don't return ret in this case though as it might contain different
> error codes orred together)
> 

OK, I will drop macro, and check regmap_read, regmap_write return value
in each function.

> Another possibilty at least for contiguous registers would be
> regmap_bulk_read().
> 
> Sascha
> 

Contiguous registers access occurs in reading and writing time. I think
Matthias's suggestion is a good way: 

do {
	ret = __mtk_rtc_read_time(rtc, &tm, &sec);
	if (ret < 0)
		goto rtc_exit;
} while (sec < tm->tm_sec);

Eddie


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Eddie Huang]

Hi Uwe,

Thanks your review.

On Mon, 2015-03-16 at 16:30 +0100, Uwe Kleine-König wrote:
> Hello Eddie,
> 
> On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote:
> > From: Tianping Fang <tianping.fang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > 
> > Add Mediatek MT63xx RTC driver
> MT6397?

Yes, it is better to use MT6397

> > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> > index f15cddf..8ac52d8 100644
> > --- a/drivers/rtc/Kconfig
> > +++ b/drivers/rtc/Kconfig
> > @@ -1427,6 +1427,16 @@ config RTC_DRV_MOXART
> >  	   This driver can also be built as a module. If so, the module
> >  	   will be called rtc-moxart
> >  
> > +config RTC_DRV_MT63XX
> > +	tristate "Mediatek Real Time Clock driver"
> > +	depends on MFD_MT6397
> I suggest:
> 
> 	depends on MFD_MT6397 || COMPILE_TEST
> 
> (maybe + any hard dependencies you need for compilation).

OK, will fix it

> 
> > +	help
> > +	  This selects the Mediatek(R) RTC driver, you should add support
> > +	  for Mediatek MT6397 PMIC before select Mediatek(R) RTC driver.
> > +
> > +	  If you want to use Mediatek(R) RTC interface, select Y or M here.
> > +	  If unsure, Please select N.
> Given the dependency above I'd say choosing y here is fine. Instead of
> recommending that I'd just drop this line.

ok, will drop.

> 
> > [...]
> > +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset)
> rtc_read is a bad name for a driver. There are already 6 functions with
> this name in the kernel. Better use a unique prefix.

I will use prefix mtk_

> 
> > [...]
> > +static irqreturn_t rtc_irq_handler_thread(int irq, void *data)
> > +{
> > +	struct mt6397_rtc *rtc = data;
> > +	u16 irqsta, irqen;
> > +
> > +	mutex_lock(&rtc->lock);
> > +	irqsta = rtc_read(rtc, RTC_IRQ_STA);
> Do you really need to lock for a single read access?

I think this lock is necessary, because other thread may access rtc
register at the same time, for example, call mtk_rtc_set_alarm to modify
alarm time.

> 
> > +	mutex_unlock(&rtc->lock);
> > +
> > +	if (irqsta & RTC_IRQ_STA_AL) {
> > +		rtc_update_irq(rtc->rtc_dev, 1, RTC_IRQF | RTC_AF);
> > +		irqen = irqsta & ~RTC_IRQ_EN_AL;
> > +		rtc_write(rtc, RTC_IRQ_EN, irqen);
> > +		rtc_write_trigger(rtc);
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	return IRQ_NONE;
> > +}
> > +
> > +static int mtk_rtc_read_time(struct device *dev, struct rtc_time *tm)
> > +{
> > +	unsigned long time;
> > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > +
> > +	mutex_lock(&rtc->lock);
> > +	do {
> > +		tm->tm_sec = rtc_read(rtc, RTC_TC_SEC);
> > +		tm->tm_min = rtc_read(rtc, RTC_TC_MIN);
> > +		tm->tm_hour = rtc_read(rtc, RTC_TC_HOU);
> > +		tm->tm_mday = rtc_read(rtc, RTC_TC_DOM);
> > +		tm->tm_mon = rtc_read(rtc, RTC_TC_MTH);
> > +		tm->tm_year = rtc_read(rtc, RTC_TC_YEA);
> > +	} while (rtc_read(rtc, RTC_TC_SEC) < tm->tm_sec);
> > +	mutex_unlock(&rtc->lock);
> > +
> > +	tm->tm_year += RTC_MIN_YEAR_OFFSET;
> > +	tm->tm_mon--;
> > +	rtc_tm_to_time(tm, &time);
> rtc_tm_to_time is deprecated, better use rtc_tm_to_time64.

OK, will change to rtc_tm_to_time64

> 
> > +	tm->tm_wday = (time / 86400 + 4) % 7;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > +{
> > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > +
> > +	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> > +	tm->tm_mon++;
> > +	mutex_lock(&rtc->lock);
> > +	rtc_write(rtc, RTC_TC_YEA, tm->tm_year);
> > +	rtc_write(rtc, RTC_TC_MTH, tm->tm_mon);
> > +	rtc_write(rtc, RTC_TC_DOM, tm->tm_mday);
> > +	rtc_write(rtc, RTC_TC_HOU, tm->tm_hour);
> > +	rtc_write(rtc, RTC_TC_MIN, tm->tm_min);
> > +	rtc_write(rtc, RTC_TC_SEC, tm->tm_sec);
> Is this racy? I.e. what happens if RTC_TC_SEC overflows just before you
> write to it but after you wrote RTC_TC_MIN?

register value will write to hardware after rtc_write_trigger, so the
racy condition not exist.

> 
> > +	rtc_write_trigger(rtc);
> > +	mutex_unlock(&rtc->lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alm)
> > +{
> > +	struct rtc_time *tm = &alm->time;
> > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > +	u16 irqen, pdn2;
> > +
> > +	mutex_lock(&rtc->lock);
> > +	irqen = rtc_read(rtc, RTC_IRQ_EN);
> > +	pdn2 = rtc_read(rtc, RTC_PDN2);
> > +	tm->tm_sec  = rtc_read(rtc, RTC_AL_SEC);
> > +	tm->tm_min  = rtc_read(rtc, RTC_AL_MIN);
> > +	tm->tm_hour = rtc_read(rtc, RTC_AL_HOU) & RTC_AL_HOU_MASK;
> > +	tm->tm_mday = rtc_read(rtc, RTC_AL_DOM) & RTC_AL_DOM_MASK;
> > +	tm->tm_mon  = rtc_read(rtc, RTC_AL_MTH) & RTC_AL_MTH_MASK;
> > +	tm->tm_year = rtc_read(rtc, RTC_AL_YEA);
> > +	mutex_unlock(&rtc->lock);
> > +
> > +	alm->enabled = !!(irqen & RTC_IRQ_EN_AL);
> > +	alm->pending = !!(pdn2 & RTC_PDN2_PWRON_ALARM);
> > +
> > +	tm->tm_year += RTC_MIN_YEAR_OFFSET;
> > +	tm->tm_mon--;
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alm)
> > +{
> > +	struct rtc_time *tm = &alm->time;
> > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > +	u16 irqen;
> > +
> > +	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> > +	tm->tm_mon++;
> > +
> > +	if (alm->enabled) {
> > +		mutex_lock(&rtc->lock);
> > +		rtc_write(rtc, RTC_AL_YEA, tm->tm_year);
> > +		rtc_write(rtc, RTC_AL_MTH, (rtc_read(rtc, RTC_AL_MTH) &
> > +				RTC_NEW_SPARE3) | tm->tm_mon);
> This looks strange. Why doesn't RTC_NEW_SPARE3 contain the register
> name? I would have expected:
> 
> 	(rtc_read(rtc, RTC_AL_MTH) & ~RTC_AL_MTH_MASK) | tm->tm_mon;

I will remove RTC_NEW_SPARE3. Hardware will return 0 if register bit is
useless. So the bit clear is redundant. 

> 
> > +		rtc_write(rtc, RTC_AL_DOM, (rtc_read(rtc, RTC_AL_DOM) &
> > +				RTC_NEW_SPARE1) | tm->tm_mday);
> > +		rtc_write(rtc, RTC_AL_HOU, (rtc_read(rtc, RTC_AL_HOU) &
> > +				RTC_NEW_SPARE_FG_MASK) | tm->tm_hour);
> > +		rtc_write(rtc, RTC_AL_MIN, tm->tm_min);
> > +		rtc_write(rtc, RTC_AL_SEC, tm->tm_sec);
> > +		rtc_write(rtc, RTC_AL_MASK, RTC_AL_MASK_DOW);
> Is this racy? I.e. if the previous set alarm is
> 
> 	2015-03-13 14:15:00
> 
> and you write
> 
> 	2015-03.14 17:17:00
> 
> is it possible that this triggers an alarm if the update happens at
> 
> 	2015-03-14 14:15:00
> 
> ?
> 
All rtc register value write to RTC hardware after rtc_write_trigger.
Race condition should not happen. 


> > +		rtc_write_trigger(rtc);
> > +		irqen = rtc_read(rtc, RTC_IRQ_EN) | RTC_IRQ_EN_ONESHOT_AL;
> > +		rtc_write(rtc, RTC_IRQ_EN, irqen);
> > +		rtc_write_trigger(rtc);
> > +		mutex_unlock(&rtc->lock);
> 	} else {
> 		/* disable alarm here */
> 
OK, should clear RTC_IRQ_EN

> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static struct rtc_class_ops mtk_rtc_ops = {
> > +	.read_time  = mtk_rtc_read_time,
> > +	.set_time   = mtk_rtc_set_time,
> > +	.read_alarm = mtk_rtc_read_alarm,
> > +	.set_alarm  = mtk_rtc_set_alarm,
> > +};
> > +
> > +static int mtk_rtc_probe(struct platform_device *pdev)
> > +{
> > +	struct mt6397_chip *mt6397_chip = dev_get_drvdata(pdev->dev.parent);
> > +	struct mt6397_rtc *rtc;
> > +	u32 reg[2];
> > +	int ret = 0;
> > +
> > +	rtc = devm_kzalloc(&pdev->dev, sizeof(struct mt6397_rtc), GFP_KERNEL);
> > +	if (!rtc)
> > +		return -ENOMEM;
> > +
> > +	ret = of_property_read_u32_array(pdev->dev.of_node, "reg", reg, 2);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "couldn't read rtc base address!\n");
> > +		return -EINVAL;
> > +	}
> > +	rtc->addr_base = reg[0];
> > +	rtc->addr_range = reg[1];
> This looks strange, but maybe that's right as you reuse the parent's
> regmap.

According Sascha and Mark Brown's discussion:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-February/323239.html

Address and interrupt will move from device tree to mfd_cell in
mt6397-core.c

> 
> > +	rtc->regmap = mt6397_chip->regmap;
> > +	rtc->dev = &pdev->dev;
> > +	mutex_init(&rtc->lock);
> > +
> > +	platform_set_drvdata(pdev, rtc);
> > +
> > +	rtc->rtc_dev = rtc_device_register("mt6397-rtc", &pdev->dev,
> > +				&mtk_rtc_ops, THIS_MODULE);
> > +	if (IS_ERR(rtc->rtc_dev)) {
> > +		dev_err(&pdev->dev, "register rtc device failed\n");
> > +		return PTR_ERR(rtc->rtc_dev);
> > +	}
> > +
> > +	rtc->irq = platform_get_irq(pdev, 0);
> > +	if (rtc->irq < 0) {
> platform_get_irq(pdev, 0) = 0 should be treated as error, too.

OK, will fix it

> 
> > +		ret = rtc->irq;
> > +		goto out_rtc;
> > +	}
> 
> Best regards
> Uwe
> 


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Uwe Kleine-König]

Hello Eddie,

On Tue, Mar 17, 2015 at 08:31:14PM +0800, Eddie Huang wrote:
> On Mon, 2015-03-16 at 16:30 +0100, Uwe Kleine-König wrote:
> > On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote:
> > > [...]
> > > +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset)
> > rtc_read is a bad name for a driver. There are already 6 functions with
> > this name in the kernel. Better use a unique prefix.
> 
> I will use prefix mtk_
I would prefer a prefix that is unique to the driver. "mtk_" doesn't
work to distinguish between the rtc and a (say) spi driver. What you
want here is that if someone reports a bug on any mailinglist with a
backtrace you are able to immediately see which driver is affected.

> > > [...]
> > > +static irqreturn_t rtc_irq_handler_thread(int irq, void *data)
> > > +{
> > > +	struct mt6397_rtc *rtc = data;
> > > +	u16 irqsta, irqen;
> > > +
> > > +	mutex_lock(&rtc->lock);
> > > +	irqsta = rtc_read(rtc, RTC_IRQ_STA);
> > Do you really need to lock for a single read access?
> 
> I think this lock is necessary, because other thread may access rtc
> register at the same time, for example, call mtk_rtc_set_alarm to modify
> alarm time.
That would be a valid reason if mtk_rtc_set_alarm touched that register
twice in a single critical section and the handler must not read the
value of the first write. Otherwise it should be fine, shouldn't it?

> > > +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > > +{
> > > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > > +
> > > +	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> > > +	tm->tm_mon++;
> > > +	mutex_lock(&rtc->lock);
> > > +	rtc_write(rtc, RTC_TC_YEA, tm->tm_year);
> > > +	rtc_write(rtc, RTC_TC_MTH, tm->tm_mon);
> > > +	rtc_write(rtc, RTC_TC_DOM, tm->tm_mday);
> > > +	rtc_write(rtc, RTC_TC_HOU, tm->tm_hour);
> > > +	rtc_write(rtc, RTC_TC_MIN, tm->tm_min);
> > > +	rtc_write(rtc, RTC_TC_SEC, tm->tm_sec);
> > Is this racy? I.e. what happens if RTC_TC_SEC overflows just before you
> > write to it but after you wrote RTC_TC_MIN?
> 
> register value will write to hardware after rtc_write_trigger, so the
> racy condition not exist.
Ah, it seems the hardware guys did their job. Nice.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Eddie Huang]

Hi Uwe,

On Tue, 2015-03-17 at 14:43 +0100, Uwe Kleine-König wrote:
> Hello Eddie,
> 
> On Tue, Mar 17, 2015 at 08:31:14PM +0800, Eddie Huang wrote:
> > On Mon, 2015-03-16 at 16:30 +0100, Uwe Kleine-König wrote:
> > > On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote:
> > > > [...]
> > > > +static u16 rtc_read(struct mt6397_rtc *rtc, u32 offset)
> > > rtc_read is a bad name for a driver. There are already 6 functions with
> > > this name in the kernel. Better use a unique prefix.
> > 
> > I will use prefix mtk_
> I would prefer a prefix that is unique to the driver. "mtk_" doesn't
> work to distinguish between the rtc and a (say) spi driver. What you
> want here is that if someone reports a bug on any mailinglist with a
> backtrace you are able to immediately see which driver is affected.
> 

My meaning is mtk_rtc_read, mtk_rtc_write.

> > > > [...]
> > > > +static irqreturn_t rtc_irq_handler_thread(int irq, void *data)
> > > > +{
> > > > +	struct mt6397_rtc *rtc = data;
> > > > +	u16 irqsta, irqen;
> > > > +
> > > > +	mutex_lock(&rtc->lock);
> > > > +	irqsta = rtc_read(rtc, RTC_IRQ_STA);
> > > Do you really need to lock for a single read access?
> > 
> > I think this lock is necessary, because other thread may access rtc
> > register at the same time, for example, call mtk_rtc_set_alarm to modify
> > alarm time.
> That would be a valid reason if mtk_rtc_set_alarm touched that register
> twice in a single critical section and the handler must not read the
> value of the first write. Otherwise it should be fine, shouldn't it?
> 

My original though is if disable alarm in mtk_rtc_set_alarm function,
RTC_IRQ_STA may be affected, this is why I add mutex. After checking
with designer, RTC_IRQ_STA will not be affected. I will remove the
mutex.

> > > > +static int mtk_rtc_set_time(struct device *dev, struct rtc_time *tm)
> > > > +{
> > > > +	struct mt6397_rtc *rtc = dev_get_drvdata(dev);
> > > > +
> > > > +	tm->tm_year -= RTC_MIN_YEAR_OFFSET;
> > > > +	tm->tm_mon++;
> > > > +	mutex_lock(&rtc->lock);
> > > > +	rtc_write(rtc, RTC_TC_YEA, tm->tm_year);
> > > > +	rtc_write(rtc, RTC_TC_MTH, tm->tm_mon);
> > > > +	rtc_write(rtc, RTC_TC_DOM, tm->tm_mday);
> > > > +	rtc_write(rtc, RTC_TC_HOU, tm->tm_hour);
> > > > +	rtc_write(rtc, RTC_TC_MIN, tm->tm_min);
> > > > +	rtc_write(rtc, RTC_TC_SEC, tm->tm_sec);
> > > Is this racy? I.e. what happens if RTC_TC_SEC overflows just before you
> > > write to it but after you wrote RTC_TC_MIN?
> > 
> > register value will write to hardware after rtc_write_trigger, so the
> > racy condition not exist.
> Ah, it seems the hardware guys did their job. Nice.
> 
> Best regards
> Uwe
> 

Re: [PATCH 2/2] rtc: mediatek: Add MT63xx RTC driver

[Uwe Kleine-König]

Hello Eddie,

On Wed, Mar 18, 2015 at 11:27:40AM +0800, Eddie Huang wrote:
> On Tue, 2015-03-17 at 14:43 +0100, Uwe Kleine-König wrote:
> > On Tue, Mar 17, 2015 at 08:31:14PM +0800, Eddie Huang wrote:
> > > On Mon, 2015-03-16 at 16:30 +0100, Uwe Kleine-König wrote:
> > > > On Wed, Jan 28, 2015 at 05:27:56PM +0800, Eddie Huang wrote:
> > > > > [...]
> > > > > +static irqreturn_t rtc_irq_handler_thread(int irq, void *data)
> > > > > +{
> > > > > +	struct mt6397_rtc *rtc = data;
> > > > > +	u16 irqsta, irqen;
> > > > > +
> > > > > +	mutex_lock(&rtc->lock);
> > > > > +	irqsta = rtc_read(rtc, RTC_IRQ_STA);
> > > > Do you really need to lock for a single read access?
> > > 
> > > I think this lock is necessary, because other thread may access rtc
> > > register at the same time, for example, call mtk_rtc_set_alarm to modify
> > > alarm time.
> > That would be a valid reason if mtk_rtc_set_alarm touched that register
> > twice in a single critical section and the handler must not read the
> > value of the first write. Otherwise it should be fine, shouldn't it?
> > 
> 
> My original though is if disable alarm in mtk_rtc_set_alarm function,
> RTC_IRQ_STA may be affected, this is why I add mutex. After checking
> with designer, RTC_IRQ_STA will not be affected. I will remove the
> mutex.
Even if IRQ_STA is changed there is no problem. With the lock the
following can happen:

Either the irq thread comes first:

	thread1			thread2

	lock
	rtc_read
	unlock   ------------>  lock
				dosomething with IRQ_STA
				unlock

or the other thread comes first:

	thread1			thread2

				lock
				dosomething with IRQ_STA
	lock   <-------------	unlock
	rtc_read
	unlock

So thread1 either reads the old or the new value of IRQ_STA.

Without locking you have the same! Either the write in thread2 comes
first or not. And depending on that you either read the new or the old
value respectively.

Of course this assumes that a write is atomic in the sense that if you
update a value from 0x01 to 0x10 there is no point in time a reader can
see 0x00 or 0x11. But this is usually given. Also reading the register
in question must not have side effects that affect the other threads.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |