[PATCH 0/3] make stmp-style devices mach-independent

[PATCH 0/3] make stmp-style devices mach-independent

[Wolfram Sang]

It seems I haven't been following the lists too closely lately, so I might have
missed similar patches. Yet, I noticed some patches dealing with drivers using
mxs-specific features, so they need to include mach-specific files which is
troublesome. I started to address the problem as well some time ago, so I push
out the patches now as RFC to enrich the discussion. The patches are rebased to
linux 3.2-rc2 and are boot tested.

The basics: mach-mxs uses ip-cores which follow a register layout I have
first seen on STMP SoCs. That means, every register has four incarnations:
 - one u32 to store a value
 - a SET register where every 1-bit sets the corresponding bit,
   others are unaffected
 - similar with a CLR register
 - and a TGL (toggle) register

Also, the 2 MSBs in register 0 are always the same and can be used to reset the
block.

All this is strictly speaking not mach-specific (but ip-core specific) and,
thus, doesn't need to be in mach-mxs/include. As I have been told, mx50 and
mx6(?) might also use IP cores following the STMP-style; so I wondered if it
can't be exported like the following patch series does:

Introduce a stmp-style device, put the code around that in a public place
(currently drivers/base/; dunno if that is the best place, though), and let
drivers for stmp-style devices select that code.

Voila, mach dependency gone, reusable code introduced. Note that I didn't
remove the duplicated code from mach-mxs yet, first all drivers have to be
converted.

Right thing to do? If so, I'll repost properly to the right lists. Fishing for
feedback right now :)

Thanks,

   Wolfram

Wolfram Sang (3):
  drivers: base: add support for stmp-style devices
  i2c: mxs: use global reset function
  rtc: stmp3xxx: use global stmp_device functionality

 drivers/base/Kconfig         |    3 ++
 drivers/base/Makefile        |    1 +
 drivers/base/stmp_device.c   |   80 ++++++++++++++++++++++++++++++++++++++++++
 drivers/i2c/busses/Kconfig   |    1 +
 drivers/i2c/busses/i2c-mxs.c |    9 +----
 drivers/rtc/Kconfig          |    1 +
 drivers/rtc/rtc-stmp3xxx.c   |   29 +++++----------
 include/linux/stmp_device.h  |   19 ++++++++++
 8 files changed, 117 insertions(+), 26 deletions(-)
 create mode 100644 drivers/base/stmp_device.c
 create mode 100644 include/linux/stmp_device.h

-- 
1.7.7.1

[PATCH 1/3] drivers: base: add support for stmp-style devices

[Wolfram Sang]

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/base/Kconfig        |    3 ++
 drivers/base/Makefile       |    1 +
 drivers/base/stmp_device.c  |   80 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/stmp_device.h |   19 ++++++++++
 4 files changed, 103 insertions(+), 0 deletions(-)
 create mode 100644 drivers/base/stmp_device.c
 create mode 100644 include/linux/stmp_device.h

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 21cf46f..331f78f 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -172,6 +172,9 @@ config SYS_HYPERVISOR
 	bool
 	default n
 
+config STMP_DEVICE
+	bool
+
 source "drivers/base/regmap/Kconfig"
 
 endmenu
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 99a375a..b865072 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -17,6 +17,7 @@ ifeq ($(CONFIG_SYSFS),y)
 obj-$(CONFIG_MODULES)	+= module.o
 endif
 obj-$(CONFIG_SYS_HYPERVISOR) += hypervisor.o
+obj-$(CONFIG_STMP_DEVICE) += stmp_device.o
 obj-$(CONFIG_REGMAP)	+= regmap/
 
 ccflags-$(CONFIG_DEBUG_DRIVER) := -DDEBUG
diff --git a/drivers/base/stmp_device.c b/drivers/base/stmp_device.c
new file mode 100644
index 0000000..3e261e6
--- /dev/null
+++ b/drivers/base/stmp_device.c
@@ -0,0 +1,80 @@
+/*
+ * Copyright (C) 1999 ARM Limited
+ * Copyright (C) 2000 Deep Blue Solutions Ltd
+ * Copyright 2006-2007,2010 Freescale Semiconductor, Inc. All Rights Reserved.
+ * Copyright 2008 Juergen Beisert, kernel at pengutronix.de
+ * Copyright 2009 Ilya Yanok, Emcraft Systems Ltd, yanok at emcraft.com
+ * Copyright (C) 2011 Wolfram Sang, Pengutronix e.K.
+ *
+ * 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.
+ */
+
+#include <linux/io.h>
+#include <linux/errno.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/stmp_device.h>
+
+#define STMP_MODULE_CLKGATE	(1 << 30)
+#define STMP_MODULE_SFTRST	(1 << 31)
+
+/*
+ * Clear the bit and poll it cleared.  This is usually called with
+ * a reset address and mask being either SFTRST(bit 31) or CLKGATE
+ * (bit 30).
+ */
+static int stmp_clear_poll_bit(void __iomem *addr, u32 mask)
+{
+	int timeout = 0x400;
+
+	writel(mask, addr + STMP_CLR_ADDR);
+	udelay(1);
+	while ((readl(addr) & mask) && --timeout)
+		/* nothing */;
+
+	return !timeout;
+}
+
+int stmp_reset_block(void __iomem *reset_addr)
+{
+	int ret;
+	int timeout = 0x400;
+
+	/* clear and poll SFTRST */
+	ret = stmp_clear_poll_bit(reset_addr, STMP_MODULE_SFTRST);
+	if (unlikely(ret))
+		goto error;
+
+	/* clear CLKGATE */
+	writel(STMP_MODULE_CLKGATE, reset_addr + STMP_CLR_ADDR);
+
+	/* set SFTRST to reset the block */
+	writel(STMP_MODULE_SFTRST, reset_addr + STMP_SET_ADDR);
+	udelay(1);
+
+	/* poll CLKGATE becoming set */
+	while ((!(readl(reset_addr) & STMP_MODULE_CLKGATE)) && --timeout)
+		/* nothing */;
+	if (unlikely(!timeout))
+		goto error;
+
+	/* clear and poll SFTRST */
+	ret = stmp_clear_poll_bit(reset_addr, STMP_MODULE_SFTRST);
+	if (unlikely(ret))
+		goto error;
+
+	/* clear and poll CLKGATE */
+	ret = stmp_clear_poll_bit(reset_addr, STMP_MODULE_CLKGATE);
+	if (unlikely(ret))
+		goto error;
+
+	return 0;
+
+error:
+	pr_err("%s(%p): module reset timeout\n", __func__, reset_addr);
+	return -ETIMEDOUT;
+}
+EXPORT_SYMBOL(stmp_reset_block);
diff --git a/include/linux/stmp_device.h b/include/linux/stmp_device.h
new file mode 100644
index 0000000..330f8d8
--- /dev/null
+++ b/include/linux/stmp_device.h
@@ -0,0 +1,19 @@
+/*
+ * basic functions for devices following the "stmp" style register layout
+ *
+ * Copyright (C) 2011 Wolfram Sang, Pengutronix e.K.
+ *
+ * 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; version 2 of the License.
+ */
+
+#ifndef __STMP_DEVICE_H__
+#define __STMP_DEVICE_H__
+
+#define STMP_SET_ADDR		0x4
+#define STMP_CLR_ADDR		0x8
+#define STMP_TOG_ADDR		0xc
+
+extern int stmp_reset_block(void __iomem *);
+#endif /* __STMP_DEVICE_H__ */
-- 
1.7.7.1

[PATCH 2/3] i2c: mxs: use global reset function

[Wolfram Sang]

The former mach specific reset_block function has been converted to a global
one. Use the new one to remove mach dependency from the driver.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/i2c/busses/Kconfig   |    1 +
 drivers/i2c/busses/i2c-mxs.c |    9 ++-------
 2 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index a3afac4..76714a8 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -467,6 +467,7 @@ config I2C_MV64XXX
 config I2C_MXS
 	tristate "Freescale i.MX28 I2C interface"
 	depends on SOC_IMX28
+	select STMP_DEVICE
 	help
 	  Say Y here if you want to use the I2C bus controller on
 	  the Freescale i.MX28 processors.
diff --git a/drivers/i2c/busses/i2c-mxs.c b/drivers/i2c/busses/i2c-mxs.c
index 7e78f7c..8b92e40 100644
--- a/drivers/i2c/busses/i2c-mxs.c
+++ b/drivers/i2c/busses/i2c-mxs.c
@@ -26,8 +26,7 @@
 #include <linux/platform_device.h>
 #include <linux/jiffies.h>
 #include <linux/io.h>
-
-#include <mach/common.h>
+#include <linux/stmp_device.h>
 
 #define DRIVER_NAME "mxs-i2c"
 
@@ -110,13 +109,9 @@ struct mxs_i2c_dev {
 	struct i2c_adapter adapter;
 };
 
-/*
- * TODO: check if calls to here are really needed. If not, we could get rid of
- * mxs_reset_block and the mach-dependency. Needs an I2C analyzer, probably.
- */
 static void mxs_i2c_reset(struct mxs_i2c_dev *i2c)
 {
-	mxs_reset_block(i2c->regs);
+	stmp_reset_block(i2c->regs);
 	writel(MXS_I2C_IRQ_MASK << 8, i2c->regs + MXS_I2C_CTRL1_SET);
 	writel(MXS_I2C_QUEUECTRL_PIO_QUEUE_MODE,
 			i2c->regs + MXS_I2C_QUEUECTRL_SET);
-- 
1.7.7.1

[PATCH 3/3] rtc: stmp3xxx: use global stmp_device functionality

[Wolfram Sang]

The former mach specific reset_block function has been converted to a global
one. Use the new one to remove mach dependency from the driver. Also simplify
code using the new STMP_REG_* macros.

Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
---
 drivers/rtc/Kconfig        |    1 +
 drivers/rtc/rtc-stmp3xxx.c |   29 ++++++++++-------------------
 2 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
index 53eb4e5..babdd5d 100644
--- a/drivers/rtc/Kconfig
+++ b/drivers/rtc/Kconfig
@@ -983,6 +983,7 @@ config RTC_DRV_COH901331
 config RTC_DRV_STMP
 	tristate "Freescale STMP3xxx/i.MX23/i.MX28 RTC"
 	depends on ARCH_MXS
+	select STMP_DEVICE
 	help
 	  If you say yes here you will get support for the onboard
 	  STMP3xxx/i.MX23/i.MX28 RTC.
diff --git a/drivers/rtc/rtc-stmp3xxx.c b/drivers/rtc/rtc-stmp3xxx.c
index 7315068..23db0ed 100644
--- a/drivers/rtc/rtc-stmp3xxx.c
+++ b/drivers/rtc/rtc-stmp3xxx.c
@@ -25,11 +25,9 @@
 #include <linux/interrupt.h>
 #include <linux/rtc.h>
 #include <linux/slab.h>
-
-#include <mach/common.h>
+#include <linux/stmp_device.h>
 
 #define STMP3XXX_RTC_CTRL			0x0
-#define STMP3XXX_RTC_CTRL_SET			0x4
 #define STMP3XXX_RTC_CTRL_CLR			0x8
 #define STMP3XXX_RTC_CTRL_ALARM_IRQ_EN		0x00000001
 #define STMP3XXX_RTC_CTRL_ONEMSEC_IRQ_EN	0x00000002
@@ -44,7 +42,6 @@
 #define STMP3XXX_RTC_ALARM			0x40
 
 #define STMP3XXX_RTC_PERSISTENT0		0x60
-#define STMP3XXX_RTC_PERSISTENT0_SET		0x64
 #define STMP3XXX_RTC_PERSISTENT0_CLR		0x68
 #define STMP3XXX_RTC_PERSISTENT0_ALARM_WAKE_EN	0x00000002
 #define STMP3XXX_RTC_PERSISTENT0_ALARM_EN	0x00000004
@@ -106,20 +103,14 @@ static irqreturn_t stmp3xxx_rtc_interrupt(int irq, void *dev_id)
 static int stmp3xxx_alarm_irq_enable(struct device *dev, unsigned int enabled)
 {
 	struct stmp3xxx_rtc_data *rtc_data = dev_get_drvdata(dev);
+	u32 set_clr_offset = enabled ? STMP_SET_ADDR : STMP_CLR_ADDR;
+
+	writel(STMP3XXX_RTC_PERSISTENT0_ALARM_EN |
+		STMP3XXX_RTC_PERSISTENT0_ALARM_WAKE_EN,
+		rtc_data->io + STMP3XXX_RTC_PERSISTENT0 + set_clr_offset);
+	writel(STMP3XXX_RTC_CTRL_ALARM_IRQ_EN,
+		rtc_data->io + STMP3XXX_RTC_CTRL + set_clr_offset);
 
-	if (enabled) {
-		writel(STMP3XXX_RTC_PERSISTENT0_ALARM_EN |
-				STMP3XXX_RTC_PERSISTENT0_ALARM_WAKE_EN,
-				rtc_data->io + STMP3XXX_RTC_PERSISTENT0_SET);
-		writel(STMP3XXX_RTC_CTRL_ALARM_IRQ_EN,
-				rtc_data->io + STMP3XXX_RTC_CTRL_SET);
-	} else {
-		writel(STMP3XXX_RTC_PERSISTENT0_ALARM_EN |
-				STMP3XXX_RTC_PERSISTENT0_ALARM_WAKE_EN,
-				rtc_data->io + STMP3XXX_RTC_PERSISTENT0_CLR);
-		writel(STMP3XXX_RTC_CTRL_ALARM_IRQ_EN,
-				rtc_data->io + STMP3XXX_RTC_CTRL_CLR);
-	}
 	return 0;
 }
 
@@ -206,7 +197,7 @@ static int stmp3xxx_rtc_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, rtc_data);
 
-	mxs_reset_block(rtc_data->io);
+	stmp_reset_block(rtc_data->io);
 	writel(STMP3XXX_RTC_PERSISTENT0_ALARM_EN |
 			STMP3XXX_RTC_PERSISTENT0_ALARM_WAKE_EN |
 			STMP3XXX_RTC_PERSISTENT0_ALARM_WAKE,
@@ -253,7 +244,7 @@ static int stmp3xxx_rtc_resume(struct platform_device *dev)
 {
 	struct stmp3xxx_rtc_data *rtc_data = platform_get_drvdata(dev);
 
-	mxs_reset_block(rtc_data->io);
+	stmp_reset_block(rtc_data->io);
 	writel(STMP3XXX_RTC_PERSISTENT0_ALARM_EN |
 			STMP3XXX_RTC_PERSISTENT0_ALARM_WAKE_EN |
 			STMP3XXX_RTC_PERSISTENT0_ALARM_WAKE,
-- 
1.7.7.1

[PATCH 1/3] drivers: base: add support for stmp-style devices

[Arnd Bergmann]

On Wednesday 16 November 2011, Wolfram Sang wrote:
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>

Introducing new core code definitely requires a long patch description.
How about copying the description from the introductory mail here?

>  drivers/base/Kconfig        |    3 ++
>  drivers/base/Makefile       |    1 +
>  drivers/base/stmp_device.c  |   80 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/stmp_device.h |   19 ++++++++++
>  4 files changed, 103 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/base/stmp_device.c
>  create mode 100644 include/linux/stmp_device.h

I'm not convinced that drivers/base is the right place, but I also can't
think of anything better right now.

> diff --git a/drivers/base/stmp_device.c b/drivers/base/stmp_device.c

> +/*
> + * Clear the bit and poll it cleared.  This is usually called with
> + * a reset address and mask being either SFTRST(bit 31) or CLKGATE
> + * (bit 30).
> + */
> +static int stmp_clear_poll_bit(void __iomem *addr, u32 mask)
> +{
> +	int timeout = 0x400;
> +
> +	writel(mask, addr + STMP_CLR_ADDR);
> +	udelay(1);
> +	while ((readl(addr) & mask) && --timeout)
> +		/* nothing */;
> +
> +	return !timeout;
> +}

For portable code, you should use cpu_relax() inside of the loop.

Is the udelay() actually necessary here?

> +int stmp_reset_block(void __iomem *reset_addr)
> +{
> +	int ret;
> +	int timeout = 0x400;
> +
> +	/* clear and poll SFTRST */
> +	ret = stmp_clear_poll_bit(reset_addr, STMP_MODULE_SFTRST);
> +	if (unlikely(ret))
> +		goto error;

Please don't use likely()/unlikely() in code that is not very
performance sensitive. It will usually just increase the code size
but not actually have a measurable benefit.

> +	/* clear CLKGATE */
> +	writel(STMP_MODULE_CLKGATE, reset_addr + STMP_CLR_ADDR);
> +
> +	/* set SFTRST to reset the block */
> +	writel(STMP_MODULE_SFTRST, reset_addr + STMP_SET_ADDR);
> +	udelay(1);
> +
> +	/* poll CLKGATE becoming set */
> +	while ((!(readl(reset_addr) & STMP_MODULE_CLKGATE)) && --timeout)
> +		/* nothing */;
> +	if (unlikely(!timeout))
> +		goto error;

Since the run-time of a readl() may vary greatly, counting to 400
for a timeout seems completely arbitrary and unhelpful.

A better construct is 

	long timeout = jiffies + HZ / 10; /* wait for at most 100ms */

	do {
		...
	} while (time_before(jiffies, timeout));

or the ktime equivalent of that if you are waiting for very short times.

> +	/* clear and poll SFTRST */
> +	ret = stmp_clear_poll_bit(reset_addr, STMP_MODULE_SFTRST);
> +	if (unlikely(ret))
> +		goto error;
> +
> +	/* clear and poll CLKGATE */
> +	ret = stmp_clear_poll_bit(reset_addr, STMP_MODULE_CLKGATE);
> +	if (unlikely(ret))
> +		goto error;
> +
> +	return 0;
> +
> +error:
> +	pr_err("%s(%p): module reset timeout\n", __func__, reset_addr);
> +	return -ETIMEDOUT;
> +}
> +EXPORT_SYMBOL(stmp_reset_block);

EXPORT_SYMBOL_GPL?

> diff --git a/include/linux/stmp_device.h b/include/linux/stmp_device.h
> new file mode 100644
> index 0000000..330f8d8
> --- /dev/null
> +++ b/include/linux/stmp_device.h
> @@ -0,0 +1,19 @@
> +#ifndef __STMP_DEVICE_H__
> +#define __STMP_DEVICE_H__
> +
> +#define STMP_SET_ADDR		0x4
> +#define STMP_CLR_ADDR		0x8
> +#define STMP_TOG_ADDR		0xc

The register numbers should probably go into the implementation file,
they are not an interface.

> +extern int stmp_reset_block(void __iomem *);
> +#endif /* __STMP_DEVICE_H__ */


	Arnd

[PATCH 1/3] drivers: base: add support for stmp-style devices

[Michał Mirosław]

2011/11/16 Arnd Bergmann <arnd@arndb.de>:
> On Wednesday 16 November 2011, Wolfram Sang wrote:
[...]
>> --- /dev/null
>> +++ b/include/linux/stmp_device.h
>> @@ -0,0 +1,19 @@
>> +#ifndef __STMP_DEVICE_H__
>> +#define __STMP_DEVICE_H__
>> +
>> +#define STMP_SET_ADDR ? ? ? ? ? ? ? ?0x4
>> +#define STMP_CLR_ADDR ? ? ? ? ? ? ? ?0x8
>> +#define STMP_TOG_ADDR ? ? ? ? ? ? ? ?0xc
>
> The register numbers should probably go into the implementation file,
> they are not an interface.

Those are offsets - for every register there are "shadow" registers
that do implicit OR, AND NOT, XOR of that register with value written.
There might be better names for those.

Best Regards,
Micha? Miros?aw

[PATCH 1/3] drivers: base: add support for stmp-style devices

[Wolfram Sang]

On Wed, Nov 16, 2011 at 05:44:19PM +0000, Arnd Bergmann wrote:
> On Wednesday 16 November 2011, Wolfram Sang wrote:
> > Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>
> 
> Introducing new core code definitely requires a long patch description.
> How about copying the description from the introductory mail here?

What I mainly forgot was s/PATCH/RFC/, and I'm sorry about that. I know this
series is not suitable to be applied directly, I was mainly looking for
comments from the arm people who are affected by this change.

> > +static int stmp_clear_poll_bit(void __iomem *addr, u32 mask)
> > +{
> > +	int timeout = 0x400;
> > +
> > +	writel(mask, addr + STMP_CLR_ADDR);
> > +	udelay(1);
> > +	while ((readl(addr) & mask) && --timeout)
> > +		/* nothing */;
> > +
> > +	return !timeout;
> > +}
> 
> For portable code, you should use cpu_relax() inside of the loop.
> 
> Is the udelay() actually necessary here?

I am just copying the code from the current mxs-implementation. I think fixups
(yes, needed!) should go in with seperate patches. Should have said so
explicitly.

> > +	ret = stmp_clear_poll_bit(reset_addr, STMP_MODULE_SFTRST);
> > +	if (unlikely(ret))
> > +		goto error;
> 
> Please don't use likely()/unlikely() in code that is not very
> performance sensitive. It will usually just increase the code size
> but not actually have a measurable benefit.

Ditto.

> > +	if (unlikely(!timeout))
> > +		goto error;
> 
> Since the run-time of a readl() may vary greatly, counting to 400
> for a timeout seems completely arbitrary and unhelpful.

Ditto, I know. I talked about such things in Prague this year :)

> 	long timeout = jiffies + HZ / 10; /* wait for at most 100ms */
> 
> 	do {
> 		...
> 	} while (time_before(jiffies, timeout));

Better, but not perfect ;) But I'll skip the discussion here...

> > +EXPORT_SYMBOL(stmp_reset_block);
> 
> EXPORT_SYMBOL_GPL?

Fine with me.

> > +#define STMP_SET_ADDR		0x4
> > +#define STMP_CLR_ADDR		0x8
> > +#define STMP_TOG_ADDR		0xc
> 
> The register numbers should probably go into the implementation file,
> they are not an interface.

As said, those are offsets. Especially useful for:

	offset = enabled ? STMP_SET_ADDR : STMP_CLR_ADDR;
	writel(bits1, reg1 + offset);
	writel(bits2, reg2 + offset);
	...

That will either set or clear bits, depending on 'enabled'.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111116/64590afd/attachment.sig>

[PATCH 1/3] drivers: base: add support for stmp-style devices

[Wolfram Sang]

> >> +#define STMP_SET_ADDR ? ? ? ? ? ? ? ?0x4
> >> +#define STMP_CLR_ADDR ? ? ? ? ? ? ? ?0x8
> >> +#define STMP_TOG_ADDR ? ? ? ? ? ? ? ?0xc
> >
> > The register numbers should probably go into the implementation file,
> > they are not an interface.
> 
> Those are offsets - for every register there are "shadow" registers
> that do implicit OR, AND NOT, XOR of that register with value written.
> There might be better names for those.

I'm all ears ;)

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111116/84a4d8e0/attachment.sig>

[PATCH 0/3] make stmp-style devices mach-independent

[Shawn Guo]

On Wed, Nov 16, 2011 at 11:47:36AM +0100, Wolfram Sang wrote:
> It seems I haven't been following the lists too closely lately, so I might have
> missed similar patches. Yet, I noticed some patches dealing with drivers using
> mxs-specific features, so they need to include mach-specific files which is
> troublesome. I started to address the problem as well some time ago, so I push
> out the patches now as RFC to enrich the discussion. The patches are rebased to
> linux 3.2-rc2 and are boot tested.
> 
> The basics: mach-mxs uses ip-cores which follow a register layout I have
> first seen on STMP SoCs. That means, every register has four incarnations:
>  - one u32 to store a value
>  - a SET register where every 1-bit sets the corresponding bit,
>    others are unaffected
>  - similar with a CLR register
>  - and a TGL (toggle) register
> 
> Also, the 2 MSBs in register 0 are always the same and can be used to reset the
> block.
> 
> All this is strictly speaking not mach-specific (but ip-core specific) and,
> thus, doesn't need to be in mach-mxs/include. As I have been told, mx50 and
> mx6(?) might also use IP cores following the STMP-style; so I wondered if it
> can't be exported like the following patch series does:
> 
> Introduce a stmp-style device, put the code around that in a public place
> (currently drivers/base/; dunno if that is the best place, though), and let
> drivers for stmp-style devices select that code.
> 
> Voila, mach dependency gone, reusable code introduced. Note that I didn't
> remove the duplicated code from mach-mxs yet, first all drivers have to be
> converted.
> 
> Right thing to do?

I think it's the right thing to do.  But I do not like the name as
stmp-style device.  Why not just mxs-style device?  Name mxs is born
to replace stmp, and keeping using two names may confuse people.
Also sticking to mxs may save the diffs like the one below.

-       mxs_reset_block(i2c->regs);
+       stmp_reset_block(i2c->regs);

Regards,
Shawn

> If so, I'll repost properly to the right lists. Fishing for
> feedback right now :)
> 

[PATCH 0/3] make stmp-style devices mach-independent

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Wolfram Sang [mailto:w.sang at pengutronix.de]
> Sent: Wednesday, November 16, 2011 6:48 PM
> To: linux-arm-kernel at lists.infradead.org
> Cc: Sascha Hauer; Guo Shawn-R65073; Dong Aisheng-B29396; Arnd Bergmann;
> Wolfram Sang
> Subject: [PATCH 0/3] make stmp-style devices mach-independent
> 
> It seems I haven't been following the lists too closely lately, so I
> might have missed similar patches. Yet, I noticed some patches dealing
> with drivers using mxs-specific features, so they need to include mach-
> specific files which is troublesome. I started to address the problem as
> well some time ago, so I push out the patches now as RFC to enrich the
> discussion. The patches are rebased to linux 3.2-rc2 and are boot tested.
> 
> The basics: mach-mxs uses ip-cores which follow a register layout I have
> first seen on STMP SoCs. That means, every register has four incarnations:
>  - one u32 to store a value
>  - a SET register where every 1-bit sets the corresponding bit,
>    others are unaffected
>  - similar with a CLR register
>  - and a TGL (toggle) register
> 
> Also, the 2 MSBs in register 0 are always the same and can be used to
> reset the block.
> 
> All this is strictly speaking not mach-specific (but ip-core specific)
> and, thus, doesn't need to be in mach-mxs/include. As I have been told,
> mx50 and
> mx6(?) might also use IP cores following the STMP-style; so I wondered if
> it can't be exported like the following patch series does:
> 
> Introduce a stmp-style device, put the code around that in a public place
> (currently drivers/base/; dunno if that is the best place, though), and
> let drivers for stmp-style devices select that code.
> 
> Voila, mach dependency gone, reusable code introduced. Note that I didn't
> remove the duplicated code from mach-mxs yet, first all drivers have to
> be converted.
> 
> Right thing to do? If so, I'll repost properly to the right lists.
> Fishing for feedback right now :)

Good thing to do.
I think it addressed my concern which raised in another patch
[PATCH 1/1] gpio: gpio-mxs: remove the duplicated micro define.

One thing is that for some people not know the history
between smtp and mxs, it maybe a little confused to see calling smtp_xx code
in mxs code.
Just as Shawn said, if we will final move to mxs in the long term,
maybe mxs name is better, right?


> Thanks,
> 
>    Wolfram
> 
> Wolfram Sang (3):
>   drivers: base: add support for stmp-style devices
>   i2c: mxs: use global reset function
>   rtc: stmp3xxx: use global stmp_device functionality
> 
>  drivers/base/Kconfig         |    3 ++
>  drivers/base/Makefile        |    1 +
>  drivers/base/stmp_device.c   |   80
> ++++++++++++++++++++++++++++++++++++++++++
>  drivers/i2c/busses/Kconfig   |    1 +
>  drivers/i2c/busses/i2c-mxs.c |    9 +----
>  drivers/rtc/Kconfig          |    1 +
>  drivers/rtc/rtc-stmp3xxx.c   |   29 +++++----------
>  include/linux/stmp_device.h  |   19 ++++++++++
>  8 files changed, 117 insertions(+), 26 deletions(-)  create mode 100644
> drivers/base/stmp_device.c  create mode 100644
> include/linux/stmp_device.h
> 
> --
> 1.7.7.1
> 

[PATCH 0/3] make stmp-style devices mach-independent

[Wolfram Sang]

> > Right thing to do?
> 
> I think it's the right thing to do.  But I do not like the name as
> stmp-style device.  Why not just mxs-style device?  Name mxs is born
> to replace stmp, and keeping using two names may confuse people.

I chose it because it was the first platform where I saw this kind of
layout. I don't like it too much as well, but couldn't find any better.
What I liked about it being different to 'mxs' is that you could easily
grep which drivers still need to be converted :) I am also not sure if
it is less confusing if you have mxs-style devices (compared to
stmp-style devices) on, say, MX6. Something platform-independent would
be cool...

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20111117/e6f443f0/attachment.sig>