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

[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 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] introduce stmp-style devices

[Wolfram Sang]

This series makes support for a certain type of devices mach independant. We
want that because such devices (having a special register layout) have been
found in mach-mxs and mach-mx6 meanwhile. Please refer to patch 1/3 for a more
detailed description.

Patch 1/3 adds the basic support and is of main interest to get upstream.
Patches 2/3 and 3/3 are usage examples and will be send to the relevant
subsystems once the basic support is accepted.

Main changes since the RFC:

* renamed the macros to STMP_OFFSET_*
* rebased to 3.3-rc6

Tested with a TX28 board.

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.9.1

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

[Wolfram Sang]

MX23/28 use IP cores which follow a register layout I have first seen on
STMP3xxx SoCs. In this layout, every register actually has four u32:

 1.) to store a value directly
 2.) a SET register where every 1-bit sets the corresponding bit,
     others are unaffected
 3.) same with a CLR register
 4.) same with a TOG (toggle) register

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

All this is strictly speaking not mach-specific (but IP core specific) and,
thus, doesn't need to be in mach-mxs/include. At least mx6 also uses IP cores
following this stmp-style. So:

Introduce a stmp-style device, put the code and defines for that in a public
place (drivers/base/), and let drivers for stmp-style devices select that code.
To avoid regressions and ease reviewing, the actual code is simply copied from
mach-mxs. It definately wants updates, but those need a seperate patch series.

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

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 7be9f79..bcd3356 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -176,6 +176,9 @@ config GENERIC_CPU_DEVICES
 	bool
 	default n
 
+config STMP_DEVICE
+	bool
+
 source "drivers/base/regmap/Kconfig"
 
 config DMA_SHARED_BUFFER
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index 610f999..86e4134 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -18,6 +18,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..8ac9bcc
--- /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_OFFSET_REG_CLR);
+	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_OFFSET_REG_CLR);
+
+	/* set SFTRST to reset the block */
+	writel(STMP_MODULE_SFTRST, reset_addr + STMP_OFFSET_REG_SET);
+	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..d088de2
--- /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_OFFSET_REG_SET	0x4
+#define STMP_OFFSET_REG_CLR	0x8
+#define STMP_OFFSET_REG_TOG	0xc
+
+extern int stmp_reset_block(void __iomem *);
+#endif /* __STMP_DEVICE_H__ */
-- 
1.7.9.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 3101dd5..d4378cf 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 3d471d5..0834802 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"
 
@@ -111,13 +110,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.9.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_OFFSET_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 3a125b8..e124f93 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 1028786..e755d3e 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_OFFSET_REG_SET : STMP_OFFSET_REG_CLR;
+
+	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.9.1

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

[Fabio Estevam]

On Wed, Mar 7, 2012 at 7:28 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
...
> +static int stmp_clear_poll_bit(void __iomem *addr, u32 mask)
> +{
> + ? ? ? int timeout = 0x400;

Could you please add a proper timeout mechanism here?

Something like: http://patchwork.ozlabs.org/patch/137365/

,as you suggested in the first place ;-)

> +
> + ? ? ? writel(mask, addr + STMP_OFFSET_REG_CLR);
> + ? ? ? udelay(1);
> + ? ? ? while ((readl(addr) & mask) && --timeout)
> + ? ? ? ? ? ? ? /* nothing */;
> +
> + ? ? ? return !timeout;
> +}
> +
> +int stmp_reset_block(void __iomem *reset_addr)
> +{
> + ? ? ? int ret;
> + ? ? ? int timeout = 0x400;

Same here.

Regards,

Fabio Estevam

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

[Huang Shijie]

? 2012?03?08? 06:28, Wolfram Sang ??:
> All this is strictly speaking not mach-specific (but IP core specific) and,
> thus, doesn't need to be in mach-mxs/include. At least mx6 also uses IP cores
> following this stmp-style. So:
>
mx50 also uses the same IP core.

BR
Huang Shijie

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

[Wolfram Sang]

On Wed, Mar 07, 2012 at 07:40:22PM -0300, Fabio Estevam wrote:
> On Wed, Mar 7, 2012 at 7:28 PM, Wolfram Sang <w.sang@pengutronix.de> wrote:
> ...
> > +static int stmp_clear_poll_bit(void __iomem *addr, u32 mask)
> > +{
> > + ? ? ? int timeout = 0x400;
> 
> Could you please add a proper timeout mechanism here?
> 
> Something like: http://patchwork.ozlabs.org/patch/137365/

>From the patch description:

===

To avoid regressions and ease reviewing, the actual code is simply copied from
mach-mxs. It definately wants updates, but those need a seperate patch series.

===

Or is that considered a wrong path?

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/20120308/cb794861/attachment.sig>

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

[Shawn Guo]

On Wed, Mar 07, 2012 at 11:28:32PM +0100, Wolfram Sang wrote:
> MX23/28 use IP cores which follow a register layout I have first seen on
> STMP3xxx SoCs. In this layout, every register actually has four u32:
> 
>  1.) to store a value directly
>  2.) a SET register where every 1-bit sets the corresponding bit,
>      others are unaffected
>  3.) same with a CLR register
>  4.) same with a TOG (toggle) register
> 
> Also, the 2 MSBs in register 0 are always the same and can be used to reset
> the IP core.
> 
> All this is strictly speaking not mach-specific (but IP core specific) and,
> thus, doesn't need to be in mach-mxs/include. At least mx6 also uses IP cores
> following this stmp-style. So:
> 
> Introduce a stmp-style device, put the code and defines for that in a public
> place (drivers/base/), and let drivers for stmp-style devices select that code.
> To avoid regressions and ease reviewing, the actual code is simply copied from
> mach-mxs. It definately wants updates, but those need a seperate patch series.
> 
> Voila, mach dependency gone, reusable code introduced. Note that I didn't
> remove the duplicated code from mach-mxs yet, first the drivers have to be
> converted.
> 
> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de>

Acked-by: Shawn Guo <shawn.guo@linaro.org>

[PATCH 0/3] introduce stmp-style devices

[Arnd Bergmann]

On Wednesday 07 March 2012, Wolfram Sang wrote:
> 
> This series makes support for a certain type of devices mach independant. We
> want that because such devices (having a special register layout) have been
> found in mach-mxs and mach-mx6 meanwhile. Please refer to patch 1/3 for a more
> detailed description.
> 
> Patch 1/3 adds the basic support and is of main interest to get upstream.
> Patches 2/3 and 3/3 are usage examples and will be send to the relevant
> subsystems once the basic support is accepted.
> 
> Main changes since the RFC:
> 
> * renamed the macros to STMP_OFFSET_*
> * rebased to 3.3-rc6
> 
> Tested with a TX28 board.

I think having the stmp_device code is good, but I'm not sure about the
location in drivers/base, which is otherwise reserved for anything related
to 'struct device'. How about putting the same file into lib/stmpregs.c?

	Arnd

[PATCH 0/3] introduce stmp-style devices

[Wolfram Sang]

> I think having the stmp_device code is good, but I'm not sure about the
> location in drivers/base, which is otherwise reserved for anything related
> to 'struct device'. How about putting the same file into lib/stmpregs.c?

Fine with me in general, except that I'd go for lib/stmp_device.c.

-- 
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/20120309/f09d8161/attachment-0001.sig>