make stmp-style devices mach-independent

Signed-off-by Wolfram Sang <[email protected]>

 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

drivers/base/stmp_device.c +6 -0

 
+/*
+ * 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)

drivers/base/stmp_device.c +7 -0

+	/* 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);

Quoted 4 lines

include/linux/stmp_device.h new file +1 -0

@@ -0,0 +1,19 @@
+#ifndef __STMP_DEVICE_H__

include/linux/stmp_device.h line 6

Quoted 5 lines

include/linux/stmp_device.h new file +5 -0

+#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.

Quoted 7 lines

Review

Quoted 4 lines

(quoted diff) +3 -0

+#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.

Quoted 1 line · Wolfram Sang

Signed-off-by Wolfram Sang <[email protected]>

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

drivers/base/stmp_device.c line 39

Quoted 11 lines

drivers/base/stmp_device.c new file +11 -0

+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?

drivers/base/stmp_device.c line 49

Quoted 3 lines

drivers/base/stmp_device.c new file +3 -0

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

Review

Quoted 2 lines

(quoted diff) +2 -0

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

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

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

Quoted 1 line

+EXPORT_SYMBOL(stmp_reset_block);

EXPORT_SYMBOL_GPL?

include/linux/stmp_device.h line 16

Quoted 3 lines

include/linux/stmp_device.h new file +3 -0

+#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.

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 :)

Quoted 1 line

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.

-----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 :)

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