Re: [PATCH v3 01/12] misc: add driver for sequencer serial port
On 10/21/2010 07:12 PM, Andrew Morton wrote:
...
+/* Register Access Helpers */
+static inline u32 ssp_read(struct ti_ssp *ssp, int reg)
+{
+return __raw_readl(ssp-regs + reg);
+}
+
+static inline void ssp_write(struct ti_ssp *ssp, int reg, u32 val)
+{
+__raw_writel(val, ssp-regs + reg);
+}
Why are the __raw functions used here?
These registers are to be accessed native endian at all times, and
therefore the le32 conversion done otherwise is inappropriate.
Thanks for the feedback. I will post an updated series with these changes.
Regards
Cyril.
--
Nokia and ATT present the 2010 Calling All Innovators-North America contest
Create new apps games for the Nokia N8 for consumers in U.S. and Canada
$10 million total in prizes - $4M cash, 500 devices, nearly $6M in marketing
Develop with Nokia Qt SDK, Web Runtime, or Java and Publish to Ovi Store
http://p.sf.net/sfu/nokia-dev2dev
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH v3 01/12] misc: add driver for sequencer serial port
On Friday 22 October 2010 14:39:33 Cyril Chemparathy wrote:
+/* Register Access Helpers */
+static inline u32 ssp_read(struct ti_ssp *ssp, int reg)
+{
+return __raw_readl(ssp-regs + reg);
+}
+
+static inline void ssp_write(struct ti_ssp *ssp, int reg, u32 val)
+{
+__raw_writel(val, ssp-regs + reg);
+}
Why are the __raw functions used here?
These registers are to be accessed native endian at all times, and
therefore the le32 conversion done otherwise is inappropriate.
Won't that break on out-of-order CPUs that need the extra synchronization
done in readl/writel?
Arnd
--
Nokia and ATT present the 2010 Calling All Innovators-North America contest
Create new apps games for the Nokia N8 for consumers in U.S. and Canada
$10 million total in prizes - $4M cash, 500 devices, nearly $6M in marketing
Develop with Nokia Qt SDK, Web Runtime, or Java and Publish to Ovi Store
http://p.sf.net/sfu/nokia-dev2dev
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH v3 01/12] misc: add driver for sequencer serial port
On Thu, 21 Oct 2010 17:01:02 -0400 Cyril Chemparathy cy...@ti.com wrote: TI's sequencer serial port (TI-SSP) is a jack-of-all-trades type of serial port device. It has a built-in programmable execution engine that can be programmed to operate as almost any serial bus (I2C, SPI, EasyScale, and others). This patch adds a driver for this controller device. The driver does not expose a user-land interface. Protocol drivers built on top of this layer are expected to remain in-kernel. I suspect the driver belongs with the mfd drivers not with drivers/misc ? otherwise it looks very nice. -- Nokia and ATT present the 2010 Calling All Innovators-North America contest Create new apps games for the Nokia N8 for consumers in U.S. and Canada $10 million total in prizes - $4M cash, 500 devices, nearly $6M in marketing Develop with Nokia Qt SDK, Web Runtime, or Java and Publish to Ovi Store http://p.sf.net/sfu/nokia-dev2dev ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH v3 01/12] misc: add driver for sequencer serial port
On 10/22/2010 10:20 AM, Alan Cox wrote: On Thu, 21 Oct 2010 17:01:02 -0400 Cyril Chemparathy cy...@ti.com wrote: TI's sequencer serial port (TI-SSP) is a jack-of-all-trades type of serial port device. It has a built-in programmable execution engine that can be programmed to operate as almost any serial bus (I2C, SPI, EasyScale, and others). This patch adds a driver for this controller device. The driver does not expose a user-land interface. Protocol drivers built on top of this layer are expected to remain in-kernel. I suspect the driver belongs with the mfd drivers not with drivers/misc ? otherwise it looks very nice. Unlike MFDs, this device doesn't have cells with differing functionality. Instead it has functionally identical ports that can operate in a variety of modes. That said, does this still fit in with other MFD drivers? If so, I don't see a problem with moving it there. Thanks Cyril. -- Nokia and ATT present the 2010 Calling All Innovators-North America contest Create new apps games for the Nokia N8 for consumers in U.S. and Canada $10 million total in prizes - $4M cash, 500 devices, nearly $6M in marketing Develop with Nokia Qt SDK, Web Runtime, or Java and Publish to Ovi Store http://p.sf.net/sfu/nokia-dev2dev ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH v3 01/12] misc: add driver for sequencer serial port
On 10/22/2010 08:48 AM, Arnd Bergmann wrote:
On Friday 22 October 2010 14:39:33 Cyril Chemparathy wrote:
+/* Register Access Helpers */
+static inline u32 ssp_read(struct ti_ssp *ssp, int reg)
+{
+return __raw_readl(ssp-regs + reg);
+}
+
+static inline void ssp_write(struct ti_ssp *ssp, int reg, u32 val)
+{
+__raw_writel(val, ssp-regs + reg);
+}
Why are the __raw functions used here?
These registers are to be accessed native endian at all times, and
therefore the le32 conversion done otherwise is inappropriate.
Won't that break on out-of-order CPUs that need the extra synchronization
done in readl/writel?
AFAICS, ioremap()ed space on ARMv6 should be strongly ordered.
...
Thanks
Cyril.
--
Nokia and ATT present the 2010 Calling All Innovators-North America contest
Create new apps games for the Nokia N8 for consumers in U.S. and Canada
$10 million total in prizes - $4M cash, 500 devices, nearly $6M in marketing
Develop with Nokia Qt SDK, Web Runtime, or Java and Publish to Ovi Store
http://p.sf.net/sfu/nokia-dev2dev
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH v3 01/12] misc: add driver for sequencer serial port
On Fri, Oct 22, 2010 at 03:33:43PM -0400, Cyril Chemparathy wrote:
On 10/22/2010 08:48 AM, Arnd Bergmann wrote:
On Friday 22 October 2010 14:39:33 Cyril Chemparathy wrote:
+/* Register Access Helpers */
+static inline u32 ssp_read(struct ti_ssp *ssp, int reg)
+{
+return __raw_readl(ssp-regs + reg);
+}
+
+static inline void ssp_write(struct ti_ssp *ssp, int reg, u32 val)
+{
+__raw_writel(val, ssp-regs + reg);
+}
Why are the __raw functions used here?
These registers are to be accessed native endian at all times, and
therefore the le32 conversion done otherwise is inappropriate.
Won't that break on out-of-order CPUs that need the extra synchronization
done in readl/writel?
AFAICS, ioremap()ed space on ARMv6 should be strongly ordered.
No. ioremap'd space is device memory on ARMv6 and above, which means
if you care about the ordering of writes to device vs memory, you
need barriers.
Nevertheless, individual reads/writes to devices will be in program
order, but writes may be delayed.
--
Nokia and ATT present the 2010 Calling All Innovators-North America contest
Create new apps games for the Nokia N8 for consumers in U.S. and Canada
$10 million total in prizes - $4M cash, 500 devices, nearly $6M in marketing
Develop with Nokia Qt SDK, Web Runtime, or Java and Publish to Ovi Store
http://p.sf.net/sfu/nokia-dev2dev
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
[PATCH v3 01/12] misc: add driver for sequencer serial port
TI's sequencer serial port (TI-SSP) is a jack-of-all-trades type of serial port
device. It has a built-in programmable execution engine that can be programmed
to operate as almost any serial bus (I2C, SPI, EasyScale, and others).
This patch adds a driver for this controller device. The driver does not
expose a user-land interface. Protocol drivers built on top of this layer are
expected to remain in-kernel.
Acked-by: Grant Likely grant.lik...@secretlab.ca
Signed-off-by: Cyril Chemparathy cy...@ti.com
---
arch/arm/mach-davinci/include/mach/ti_ssp.h | 89 ++
drivers/misc/Kconfig| 11 +
drivers/misc/Makefile |1 +
drivers/misc/ti_ssp.c | 436 +++
4 files changed, 537 insertions(+), 0 deletions(-)
create mode 100644 arch/arm/mach-davinci/include/mach/ti_ssp.h
create mode 100644 drivers/misc/ti_ssp.c
diff --git a/arch/arm/mach-davinci/include/mach/ti_ssp.h
b/arch/arm/mach-davinci/include/mach/ti_ssp.h
new file mode 100644
index 000..51afc42
--- /dev/null
+++ b/arch/arm/mach-davinci/include/mach/ti_ssp.h
@@ -0,0 +1,89 @@
+/*
+ * Sequencer Serial Port (SSP) driver for Texas Instruments' SoCs
+ *
+ * Copyright (C) 2010 Texas Instruments Inc
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#ifndef __TI_SSP_H__
+#define __TI_SSP_H__
+
+struct ti_ssp_dev_data {
+ const char *dev_name;
+ unsigned long iosel; /* see note below */
+ unsigned long config;
+ const void *pdata;
+ int pdata_sz;
+};
+
+struct ti_ssp_data {
+ unsigned long out_clock;
+ struct ti_ssp_dev_data dev_data[2];
+};
+
+/*
+ * Sequencer port IO pin configuration bits. These do not correlate 1-1 with
+ * the hardware. The iosel field in the port data combines iosel1 and iosel2,
+ * and is therefore not a direct map to register space. It is best to use the
+ * macros below to construct iosel values.
+ *
+ * least significant 16 bits -- iosel1
+ * most significant 16 bits -- iosel2
+ */
+
+#define SSP_IN 0x
+#define SSP_DATA 0x0001
+#define SSP_CLOCK 0x0002
+#define SSP_CHIPSEL0x0003
+#define SSP_OUT0x0004
+#define SSP_PIN_SEL(pin, v)((v) ((pin) * 3))
+#define SSP_PIN_MASK(pin) SSP_PIN_SEL(pin, 0x7)
+#define SSP_INPUT_SEL(pin) ((pin) 16)
+
+/* Sequencer port config bits */
+#define SSP_EARLY_DIN BIT(8)
+#define SSP_DELAY_DOUT BIT(9)
+
+/* Sequence map definitions */
+#define SSP_CLK_HIGH BIT(0)
+#define SSP_CLK_LOW0
+#define SSP_DATA_HIGH BIT(1)
+#define SSP_DATA_LOW 0
+#define SSP_CS_HIGHBIT(2)
+#define SSP_CS_LOW 0
+#define SSP_OUT_MODE BIT(3)
+#define SSP_IN_MODE0
+#define SSP_DATA_REG BIT(4)
+#define SSP_ADDR_REG 0
+
+#define SSP_OPCODE_DIRECT ((0x0) 5)
+#define SSP_OPCODE_TOGGLE ((0x1) 5)
+#define SSP_OPCODE_SHIFT ((0x2) 5)
+#define SSP_OPCODE_BRANCH0 ((0x4) 5)
+#define SSP_OPCODE_BRANCH1 ((0x5) 5)
+#define SSP_OPCODE_BRANCH ((0x6) 5)
+#define SSP_OPCODE_STOP((0x7) 5)
+#define SSP_BRANCH(addr) ((addr) 8)
+#define SSP_COUNT(cycles) ((cycles) 8)
+
+int ti_ssp_raw_read(struct device *dev);
+int ti_ssp_raw_write(struct device *dev, u32 val);
+int ti_ssp_load(struct device *dev, int offs, u32* prog, int len);
+int ti_ssp_run(struct device *dev, u32 pc, u32 input, u32 *output);
+int ti_ssp_set_mode(struct device *dev, int mode);
+int ti_ssp_set_iosel(struct device *dev, u32 iosel);
+
+#endif /* __TI_SSP_H__ */
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index b743312..9fb8470 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -390,6 +390,17 @@ config BMP085
To compile this driver as a module, choose M here: the
module will be called bmp085.
+config TI_SSP
+ depends on ARCH_DAVINCI_TNETV107X
+ tristate Sequencer Serial Port support
+ default y
+ ---help---
+ Say Y here if you want support for the Sequencer Serial Port
+ in a Texas Instruments TNETV107X SoC.
+
+ To compile this driver as a module, choose M
Re: [PATCH v3 01/12] misc: add driver for sequencer serial port
On Thu, 21 Oct 2010 17:01:02 -0400
Cyril Chemparathy cy...@ti.com wrote:
TI's sequencer serial port (TI-SSP) is a jack-of-all-trades type of serial
port
device. It has a built-in programmable execution engine that can be
programmed
to operate as almost any serial bus (I2C, SPI, EasyScale, and others).
This patch adds a driver for this controller device. The driver does not
expose a user-land interface. Protocol drivers built on top of this layer are
expected to remain in-kernel.
...
+struct ti_ssp_dev_data {
+ const char *dev_name;
+ unsigned long iosel; /* see note below */
+ unsigned long config;
+ const void *pdata;
+ int pdata_sz;
I suppose this really should have type size_t. Also a better name is
pdata_size - we prefer to avoid this random omission of vowels from
kernel identifiers. Just spell it out; it makes it easier to remember.
+};
+
+struct ti_ssp_data {
+ unsigned long out_clock;
+ struct ti_ssp_dev_data dev_data[2];
+};
+
...
+config TI_SSP
+ depends on ARCH_DAVINCI_TNETV107X
+ tristate Sequencer Serial Port support
+ default y
Was `y' a good choice?
+ ---help---
+ Say Y here if you want support for the Sequencer Serial Port
+ in a Texas Instruments TNETV107X SoC.
+
+ To compile this driver as a module, choose M here: the
+ module will be called ti_ssp.
...
+#define dev2ssp(dev) dev_get_drvdata(dev-parent)
+#define dev2port(dev)(to_platform_device(dev)-id)
These could be implemented as C funtions. That's superior because of
the typechecking. At present dev2ssp() will happily compile and fail
at runtime if passed anystructure which has a 'const struct device
*parent'.
+/* Register Access Helpers */
+static inline u32 ssp_read(struct ti_ssp *ssp, int reg)
+{
+ return __raw_readl(ssp-regs + reg);
+}
+
+static inline void ssp_write(struct ti_ssp *ssp, int reg, u32 val)
+{
+ __raw_writel(val, ssp-regs + reg);
+}
Why are the __raw functions used here?
+static inline void ssp_rmw(struct ti_ssp *ssp, int reg, u32 mask, u32 bits)
+{
+ u32 val = ssp_read(ssp, reg);
+ val = ~mask;
+ val |= bits;
+ ssp_write(ssp, reg, val);
+}
Locking? Perhaps this function must be called under ssp-lock? If so,
that should be documented here and it appears that not all callsites
actually do that correctly.
...
+static int __set_iosel(struct ti_ssp *ssp, int port, u32 iosel)
+{
+ unsigned val;
+
+ /* IOSEL1 gets the least significant 16 bits */
+ val = ssp_read(ssp, REG_IOSEL_1);
+ val = 0x (port ? 0 : 16);
+ val |= (iosel 0x) (port ? 16 : 0);
+ ssp_write(ssp, REG_IOSEL_1, val);
+
+ /* IOSEL2 gets the most significant 16 bits */
+ val = ssp_read(ssp, REG_IOSEL_2);
+ val = 0x0007 (port ? 0 : 16);
+ val |= (iosel 0x0007) (port ? 0 : 16);
+ ssp_write(ssp, REG_IOSEL_2, val);
More rmw's which need locking. It should be documented please. Both
callers get it right this time.
+ return 0;
+}
+
...
+int ti_ssp_run(struct device *dev, u32 pc, u32 input, u32 *output)
+{
+ struct ti_ssp *ssp = dev2ssp(dev);
+ int port = dev2port(dev);
+ int count;
+
+ if (pc ~(0x3f))
+ return -EINVAL;
+
+ ssp_port_write(ssp, port, PORT_ADDR, input 16);
+ ssp_port_write(ssp, port, PORT_DATA, input 0x);
+ ssp_port_rmw(ssp, port, PORT_CFG_1, 0x3f, pc);
+
+ ssp_port_set_bits(ssp, port, PORT_CFG_1, SSP_START);
+
+ for (count = 1; count; count--) {
+ if ((ssp_port_read(ssp, port, PORT_CFG_1) SSP_BUSY) == 0)
+ break;
+ udelay(1);
+ }
+
+ if (output) {
+ *(output) = (ssp_port_read(ssp, port, PORT_ADDR) 16) |
+ (ssp_port_read(ssp, port, PORT_DATA) 0x);
+ }
+
+ if (!count) {
+ dev_err(ssp-dev, timed out waiting for SSP operation\n);
+ return -EIO;
+ }
There doesn't seem much point in writing to *output if the port_read()
timed out?
...
That's all fairly minor stuff. It looks Good Enough For Linux to me.
--
Nokia and ATT present the 2010 Calling All Innovators-North America contest
Create new apps games for the Nokia N8 for consumers in U.S. and Canada
$10 million total in prizes - $4M cash, 500 devices, nearly $6M in marketing
Develop with Nokia Qt SDK, Web Runtime, or Java and Publish to Ovi Store
http://p.sf.net/sfu/nokia-dev2dev
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
