[PATCH 0/5] rtc: s5m: Bug fixes of the driver after tests

[PATCH 0/5] rtc: s5m: Bug fixes of the driver after tests

[Krzysztof Kozlowski]

Hi,

This is a patchset of small but important fixes for rtc-s5m driver. Last
patches also add waking up on RTC alarm interrupts.

The patchset is based on v3.13-rc1-95-gb975dc3 and depends on patch from 
Geert Uytterhoeven:
 - rtc: s5m-rtc: Fix info->rtc assignment
   http://article.gmane.org/gmane.linux.kernel/1595905

Best regards,
Krzysztof

Krzysztof Kozlowski (5):
  rtc: s5m: Fix register updating by adding regmap for RTC
  rtc: s5m: Fix unsuccesful IRQ request during probe
  rtc: s5m: Limit endless loop waiting for register update
  mfd: sec: Add PM ops and make it a wake up source
  rtc: s5m: Enable IRQ wake during suspend

 drivers/mfd/sec-core.c           |   54 +++++++++++++++++++++++++++++
 drivers/rtc/rtc-s5m.c            |   70 +++++++++++++++++++++++++++++++++-----
 include/linux/mfd/samsung/core.h |    1 +
 3 files changed, 116 insertions(+), 9 deletions(-)

-- 
1.7.9.5

[PATCH 1/5] rtc: s5m: Fix register updating by adding regmap for RTC

[Krzysztof Kozlowski]

On S5M8767A registers were not properly updated and read due to usage of
the same regmap as the PMIC. This could be observed in various hangs,
e.g. in infinite loop during waiting for UDR field change.

On this chip family the RTC has different I2C address than PMIC so
additional regmap is needed.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/mfd/sec-core.c           |   14 ++++++++++++++
 drivers/rtc/rtc-s5m.c            |    2 +-
 include/linux/mfd/samsung/core.h |    1 +
 3 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c
index 34c18fb..a1a413a 100644
--- a/drivers/mfd/sec-core.c
+++ b/drivers/mfd/sec-core.c
@@ -166,6 +166,11 @@ static struct regmap_config s5m8767_regmap_config = {
 	.cache_type = REGCACHE_FLAT,
 };
 
+static struct regmap_config sec_rtc_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
 #ifdef CONFIG_OF
 /*
  * Only the common platform data elements for s5m8767 are parsed here from the
@@ -277,6 +282,15 @@ static int sec_pmic_probe(struct i2c_client *i2c,
 	sec_pmic->rtc = i2c_new_dummy(i2c->adapter, RTC_I2C_ADDR);
 	i2c_set_clientdata(sec_pmic->rtc, sec_pmic);
 
+	sec_pmic->regmap_rtc = devm_regmap_init_i2c(sec_pmic->rtc,
+			&sec_rtc_regmap_config);
+	if (IS_ERR(sec_pmic->regmap_rtc)) {
+		ret = PTR_ERR(sec_pmic->regmap_rtc);
+		dev_err(&i2c->dev, "Failed to allocate RTC register map: %d\n",
+			ret);
+		return ret;
+	}
+
 	if (pdata && pdata->cfg_pmic_irq)
 		pdata->cfg_pmic_irq();
 
diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
index 1dfa488..088cf3a 100644
--- a/drivers/rtc/rtc-s5m.c
+++ b/drivers/rtc/rtc-s5m.c
@@ -542,7 +542,7 @@ static int s5m_rtc_probe(struct platform_device *pdev)
 
 	info->dev = &pdev->dev;
 	info->s5m87xx = s5m87xx;
-	info->regmap = s5m87xx->regmap;
+	info->regmap = s5m87xx->regmap_rtc;
 	info->device_type = s5m87xx->device_type;
 	info->wtsr_smpl = s5m87xx->wtsr_smpl;
 
diff --git a/include/linux/mfd/samsung/core.h b/include/linux/mfd/samsung/core.h
index 2d0c907..692acad 100644
--- a/include/linux/mfd/samsung/core.h
+++ b/include/linux/mfd/samsung/core.h
@@ -40,6 +40,7 @@ struct sec_pmic_dev {
 	struct device *dev;
 	struct sec_platform_data *pdata;
 	struct regmap *regmap;
+	struct regmap *regmap_rtc;
 	struct i2c_client *i2c;
 	struct i2c_client *rtc;
 
-- 
1.7.9.5

Re: [PATCH 1/5] rtc: s5m: Fix register updating by adding regmap for RTC

[Geert Uytterhoeven]

On Tue, Nov 26, 2013 at 2:50 PM, Krzysztof Kozlowski
<k.kozlowski@samsung.com> wrote:
> On S5M8767A registers were not properly updated and read due to usage of
> the same regmap as the PMIC. This could be observed in various hangs,
> e.g. in infinite loop during waiting for UDR field change.
>
> On this chip family the RTC has different I2C address than PMIC so
> additional regmap is needed.
>
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/mfd/sec-core.c           |   14 ++++++++++++++
>  drivers/rtc/rtc-s5m.c            |    2 +-
>  include/linux/mfd/samsung/core.h |    1 +
>  3 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c
> index 34c18fb..a1a413a 100644
> --- a/drivers/mfd/sec-core.c
> +++ b/drivers/mfd/sec-core.c
> @@ -166,6 +166,11 @@ static struct regmap_config s5m8767_regmap_config = {
>         .cache_type = REGCACHE_FLAT,
>  };
>
> +static struct regmap_config sec_rtc_regmap_config = {

const please

> --- a/include/linux/mfd/samsung/core.h
> +++ b/include/linux/mfd/samsung/core.h
> @@ -40,6 +40,7 @@ struct sec_pmic_dev {
>         struct device *dev;
>         struct sec_platform_data *pdata;
>         struct regmap *regmap;
> +       struct regmap *regmap_rtc;

Do you think it makes sense to rename the plain "regmap" to "regmap_pmic"?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/5] rtc: s5m: Fix register updating by adding regmap for RTC

[Krzysztof Kozlowski]

On Tue, 2013-11-26 at 15:07 +0100, Geert Uytterhoeven wrote:
> On Tue, Nov 26, 2013 at 2:50 PM, Krzysztof Kozlowski
> <k.kozlowski@samsung.com> wrote:
> > On S5M8767A registers were not properly updated and read due to usage of
> > the same regmap as the PMIC. This could be observed in various hangs,
> > e.g. in infinite loop during waiting for UDR field change.
> >
> > On this chip family the RTC has different I2C address than PMIC so
> > additional regmap is needed.
> >
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >  drivers/mfd/sec-core.c           |   14 ++++++++++++++
> >  drivers/rtc/rtc-s5m.c            |    2 +-
> >  include/linux/mfd/samsung/core.h |    1 +
> >  3 files changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c
> > index 34c18fb..a1a413a 100644
> > --- a/drivers/mfd/sec-core.c
> > +++ b/drivers/mfd/sec-core.c
> > @@ -166,6 +166,11 @@ static struct regmap_config s5m8767_regmap_config = {
> >         .cache_type = REGCACHE_FLAT,
> >  };
> >
> > +static struct regmap_config sec_rtc_regmap_config = {
> 
> const please

Sure.

> > --- a/include/linux/mfd/samsung/core.h
> > +++ b/include/linux/mfd/samsung/core.h
> > @@ -40,6 +40,7 @@ struct sec_pmic_dev {
> >         struct device *dev;
> >         struct sec_platform_data *pdata;
> >         struct regmap *regmap;
> > +       struct regmap *regmap_rtc;
> 
> Do you think it makes sense to rename the plain "regmap" to "regmap_pmic"?

Yes, I think it would make the code more readable. I'll change it.

Best regards,
Krzysztof

[PATCH 2/5] rtc: s5m: Fix unsuccesful IRQ request during probe

[Krzysztof Kozlowski]

Probe failed for rtc-s5m:
	s5m-rtc s5m-rtc: Failed to request alarm IRQ: 12: -22
	s5m-rtc: probe of s5m-rtc failed with error -22

Fix rtc-s5m interrupt request by using regmap_irq_get_virq() for mapping
the IRQ.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/rtc/rtc-s5m.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
index 088cf3a..66686d9 100644
--- a/drivers/rtc/rtc-s5m.c
+++ b/drivers/rtc/rtc-s5m.c
@@ -548,11 +548,13 @@ static int s5m_rtc_probe(struct platform_device *pdev)
 
 	switch (pdata->device_type) {
 	case S5M8763X:
-		info->irq = s5m87xx->irq_base + S5M8763_IRQ_ALARM0;
+		info->irq = regmap_irq_get_virq(s5m87xx->irq_data,
+				S5M8763_IRQ_ALARM0);
 		break;
 
 	case S5M8767X:
-		info->irq = s5m87xx->irq_base + S5M8767_IRQ_RTCA1;
+		info->irq = regmap_irq_get_virq(s5m87xx->irq_data,
+				S5M8767_IRQ_RTCA1);
 		break;
 
 	default:
-- 
1.7.9.5

[PATCH 3/5] rtc: s5m: Limit endless loop waiting for register update

[Krzysztof Kozlowski]

After setting alarm or time the driver is waiting for UDR register to be
cleared indicating that registers data have been transferred.

Limit the endless loop to only 5 retries.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/rtc/rtc-s5m.c |   37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
index 66686d9..51471c1 100644
--- a/drivers/rtc/rtc-s5m.c
+++ b/drivers/rtc/rtc-s5m.c
@@ -28,6 +28,16 @@
 #include <linux/mfd/samsung/irq.h>
 #include <linux/mfd/samsung/rtc.h>
 
+/*
+ * Maximum number of retries for checking changes in UDR field
+ * of SEC_RTC_UDR_CON register (to limit possible endless loop).
+ *
+ * After writing to RTC registers (setting time or alarm) read the UDR field
+ * in SEC_RTC_UDR_CON register. UDR is auto-cleared when data have
+ * been transferred.
+ */
+#define UDR_READ_RETRY_CNT	5
+
 struct s5m_rtc_info {
 	struct device *dev;
 	struct sec_pmic_dev *s5m87xx;
@@ -84,6 +94,25 @@ static int s5m8767_tm_to_data(struct rtc_time *tm, u8 *data)
 	}
 }
 
+/*
+ * Read RTC_UDR_CON register and wait till UDR field is cleared.
+ * This indicates that time/alarm update ended.
+ */
+static inline int s5m8767_wait_for_udr_update(struct s5m_rtc_info *info)
+{
+	int ret, retry = UDR_READ_RETRY_CNT;
+	unsigned int data;
+
+	do {
+		ret = regmap_read(info->regmap, SEC_RTC_UDR_CON, &data);
+	} while (--retry && (data & RTC_UDR_MASK) && !ret);
+
+	if (!retry)
+		dev_err(info->dev, "waiting for UDR update, reached max number of retries\n");
+
+	return ret;
+}
+
 static inline int s5m8767_rtc_set_time_reg(struct s5m_rtc_info *info)
 {
 	int ret;
@@ -104,9 +133,7 @@ static inline int s5m8767_rtc_set_time_reg(struct s5m_rtc_info *info)
 		return ret;
 	}
 
-	do {
-		ret = regmap_read(info->regmap, SEC_RTC_UDR_CON, &data);
-	} while ((data & RTC_UDR_MASK) && !ret);
+	ret = s5m8767_wait_for_udr_update(info);
 
 	return ret;
 }
@@ -133,9 +160,7 @@ static inline int s5m8767_rtc_set_alarm_reg(struct s5m_rtc_info *info)
 		return ret;
 	}
 
-	do {
-		ret = regmap_read(info->regmap, SEC_RTC_UDR_CON, &data);
-	} while ((data & RTC_UDR_MASK) && !ret);
+	ret = s5m8767_wait_for_udr_update(info);
 
 	return ret;
 }
-- 
1.7.9.5

[PATCH 4/5] mfd: sec: Add PM ops and make it a wake up source

[Krzysztof Kozlowski]

Add PM suspend/resume ops to the sec MFD core driver and make it a wake
up source. This allows proper waking from suspend to RAM and also fixes
broken interrupts after resuming:
[   42.705703] sec_pmic 7-0066: Failed to read IRQ status: -5

Interrupts stop working after first resume initiated by them (e.g. by
RTC Alarm interrupt) because interrupt registers were not cleared properly.

When device is woken up from suspend by RTC Alarm, an interrupt occurs
before resuming I2C bus controller. The interrupt is handled by
regmap_irq_thread which tries to read RTC registers. This read fails
(I2C is still suspended) and RTC Alarm interrupt is disabled.

Disable the S5M8767 interrupts during suspend (disable_irq()) and enable
them during resume so the device will be still woken up but the interrupt
won't happen before resuming I2C bus.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/mfd/sec-core.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/mfd/sec-core.c b/drivers/mfd/sec-core.c
index a1a413a..8c86da4 100644
--- a/drivers/mfd/sec-core.c
+++ b/drivers/mfd/sec-core.c
@@ -323,6 +323,8 @@ static int sec_pmic_probe(struct i2c_client *i2c,
 	if (ret)
 		goto err;
 
+	device_init_wakeup(sec_pmic->dev, sec_pmic->wakeup);
+
 	return ret;
 
 err:
@@ -341,6 +343,43 @@ static int sec_pmic_remove(struct i2c_client *i2c)
 	return 0;
 }
 
+static int sec_pmic_suspend(struct device *dev)
+{
+	struct i2c_client *i2c = container_of(dev, struct i2c_client, dev);
+	struct sec_pmic_dev *sec_pmic = i2c_get_clientdata(i2c);
+
+	if (device_may_wakeup(dev)) {
+		enable_irq_wake(sec_pmic->irq);
+		/*
+		 * PMIC IRQ must be disabled during suspend for RTC alarm
+		 * to work properly.
+		 * When device is woken up from suspend by RTC Alarm, an
+		 * interrupt occurs before resuming I2C bus controller.
+		 * The interrupt is handled by regmap_irq_thread which tries
+		 * to read RTC registers. This read fails (I2C is still
+		 * suspended) and RTC Alarm interrupt is disabled.
+		 */
+		disable_irq(sec_pmic->irq);
+	}
+
+	return 0;
+}
+
+static int sec_pmic_resume(struct device *dev)
+{
+	struct i2c_client *i2c = container_of(dev, struct i2c_client, dev);
+	struct sec_pmic_dev *sec_pmic = i2c_get_clientdata(i2c);
+
+	if (device_may_wakeup(dev)) {
+		disable_irq_wake(sec_pmic->irq);
+		enable_irq(sec_pmic->irq);
+	}
+
+	return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(sec_pmic_pm_ops, sec_pmic_suspend, sec_pmic_resume);
+
 static const struct i2c_device_id sec_pmic_id[] = {
 	{ "sec_pmic", 0 },
 	{ }
@@ -351,6 +390,7 @@ static struct i2c_driver sec_pmic_driver = {
 	.driver = {
 		   .name = "sec_pmic",
 		   .owner = THIS_MODULE,
+		   .pm = &sec_pmic_pm_ops,
 		   .of_match_table = of_match_ptr(sec_dt_match),
 	},
 	.probe = sec_pmic_probe,
-- 
1.7.9.5

Re: [PATCH 4/5] mfd: sec: Add PM ops and make it a wake up source

[Lee Jones]

On Tue, 26 Nov 2013, Krzysztof Kozlowski wrote:

> Add PM suspend/resume ops to the sec MFD core driver and make it a wake
> up source. This allows proper waking from suspend to RAM and also fixes
> broken interrupts after resuming:
> [   42.705703] sec_pmic 7-0066: Failed to read IRQ status: -5
> 
> Interrupts stop working after first resume initiated by them (e.g. by
> RTC Alarm interrupt) because interrupt registers were not cleared properly.
> 
> When device is woken up from suspend by RTC Alarm, an interrupt occurs
> before resuming I2C bus controller. The interrupt is handled by
> regmap_irq_thread which tries to read RTC registers. This read fails
> (I2C is still suspended) and RTC Alarm interrupt is disabled.
> 
> Disable the S5M8767 interrupts during suspend (disable_irq()) and enable
> them during resume so the device will be still woken up but the interrupt
> won't happen before resuming I2C bus.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/mfd/sec-core.c |   40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)

Patch looks good to me and appears to be orthogonal therefore I have
tentatively applied it.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 5/5] rtc: s5m: Enable IRQ wake during suspend

[Krzysztof Kozlowski]

Add PM suspend/resume ops to rtc-s5m driver and enable IRQ wake during
suspend so the RTC would act like a wake up source. This allows waking
up from suspend to RAM on RTC alarm interrupt.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/rtc/rtc-s5m.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/rtc/rtc-s5m.c b/drivers/rtc/rtc-s5m.c
index 51471c1..ae8119d 100644
--- a/drivers/rtc/rtc-s5m.c
+++ b/drivers/rtc/rtc-s5m.c
@@ -639,6 +639,30 @@ static void s5m_rtc_shutdown(struct platform_device *pdev)
 	s5m_rtc_enable_smpl(info, false);
 }
 
+static int s5m_rtc_resume(struct device *dev)
+{
+	struct s5m_rtc_info *info = dev_get_drvdata(dev);
+	int ret = 0;
+
+	if (device_may_wakeup(dev))
+		ret = disable_irq_wake(info->irq);
+
+	return ret;
+}
+
+static int s5m_rtc_suspend(struct device *dev)
+{
+	struct s5m_rtc_info *info = dev_get_drvdata(dev);
+	int ret = 0;
+
+	if (device_may_wakeup(dev))
+		ret = enable_irq_wake(info->irq);
+
+	return ret;
+}
+
+static SIMPLE_DEV_PM_OPS(s5m_rtc_pm_ops, s5m_rtc_suspend, s5m_rtc_resume);
+
 static const struct platform_device_id s5m_rtc_id[] = {
 	{ "s5m-rtc", 0 },
 };
@@ -647,6 +671,7 @@ static struct platform_driver s5m_rtc_driver = {
 	.driver		= {
 		.name	= "s5m-rtc",
 		.owner	= THIS_MODULE,
+		.pm	= &s5m_rtc_pm_ops,
 	},
 	.probe		= s5m_rtc_probe,
 	.shutdown	= s5m_rtc_shutdown,
-- 
1.7.9.5