implementing a slave driver

implementing a slave driver

[Marc Dietrich]

Hi,

the HW I'm using (tegra) can be configured to act as a slave. This function is 
needed to communicate with an embedded controller (which is the master). I 
like to know if this slave function can/should be implemented in the 
i2c/busses/i2c-tegra.c driver or better outside, e.g. in the driver for the 
embedded controller.

I haven't found any other hw so far which has this function and it seems that 
the i2c layer does not provide any interface for the communication with a 
master.

Thanks for an advice

Marvin

Re: implementing a slave driver

[Ben Dooks]

On Sat, Feb 19, 2011 at 10:53:12PM +0100, Marc Dietrich wrote:
> Hi,
> 
> the HW I'm using (tegra) can be configured to act as a slave. This function is 
> needed to communicate with an embedded controller (which is the master). I 
> like to know if this slave function can/should be implemented in the 
> i2c/busses/i2c-tegra.c driver or better outside, e.g. in the driver for the 
> embedded controller.
> 
> I haven't found any other hw so far which has this function and it seems that 
> the i2c layer does not provide any interface for the communication with a 
> master.

There's a couple of adapters that do this in the ARM world, all the 
samsung ones and the most of the pxas (if not all)

It might be worth doing a simple slave driver, but I'm not sure how
many would really want to use it?

-- 
Ben Dooks, ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

Re: implementing a slave driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 499 bytes --]


> It might be worth doing a simple slave driver, but I'm not sure how
> many would really want to use it?

I sense a rising interest in slave support. Sadly, only during
requirement talks so far, no actual project. But in those talks, a
generic solution is favoured, because it makes switching SoC easier.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

RE: implementing a slave driver

[Fillod Stephane]

[-- Attachment #1: Type: text/plain, Size: 2343 bytes --]

Marc Dietrich wrote:
> the HW I'm using (tegra) can be configured to act as a slave. This
function is 
> needed to communicate with an embedded controller (which is the
master). I 
> like to know if this slave function can/should be implemented in the 
> i2c/busses/i2c-tegra.c driver or better outside, e.g. in the driver
for the 
> embedded controller.

Do you need to implement a slave for write-only or also read messages?

> I haven't found any other hw so far which has this function and it
seems that 
> the i2c layer does not provide any interface for the communication
with a 
> master.

In fact, the Freescale's MPC8xxx I2C controllers are also capable of
slave 
operation. I've implemented it, and it's working fine.

Please find attached a patch against 2.6.35.

 drivers/i2c/busses/i2c-mpc.c |   79
+++++++++++++++++++++++++++++++++++++++++--
 drivers/i2c/i2c-dev.c        |    8 +++-
 include/linux/i2c.h          |    1
 3 files changed, 84 insertions(+), 4 deletions(-)

The proposal sent last year[1] relies on a generic API extension.
<<
The i2c-dev subsystem is extended with a new flag I2C_M_SLAVE.
Basically, a struct i2c_msg holding flag I2C_M_SLAVE will wait
in slave mode for a write request on specified target address,
and will report the received content to the user-land application.

An example of driver implementation has been done for i2c-mpc. 
There's no support for read request yet, and I'm wondering whether
it would make sense. The patch has been tested on powerpc MPC8313E. 
Note: the present i2c-mpc implementation is RFC and may be improved,
esp. regarding current timing issues.

[...]
Talking about i2c-dev, does the I2C_M_SLAVE flag is the appropriate 
way of modelling the API extension? Is there any interest for
this extension to be accepted in the linux kernel?
>>

[1] http://thread.gmane.org/gmane.linux.drivers.i2c/6417

Until now, I have no good proposal for the read request.
It should be doable to pass a static array and let the kernel read from
it,
but I see no easy way to have a user task respond with
compute-as-you-go.

Still, the write message is satisfactory for my use case.

Comments welcome.
-- 
Stephane

PS: sorry about my broken MUA, it's about to be changed in the coming
weeks,
as well as my e-mail address.

[-- Attachment #2: mpc-i2c-slave.patch --]
[-- Type: application/octet-stream, Size: 4818 bytes --]

Subject: [PATCH] mpc-i2c: I2C slave handling

From: Stephane Fillod <stephane.fillod@thomson-networks.com>

I2C slave handling for Freescale 8xxx.

Signed-off-by: Stephane Fillod <stephane.fillod@thomson-networks.com>

---

I2C host adapter slave support was raised 2 years ago:
http://thread.gmane.org/gmane.linux.drivers.i2c/43/focus=43

--- a/include/linux/i2c.h	24 Feb 2010 18:52:17 -0000
+++ b/include/linux/i2c.h	26 Feb 2010 15:50:46 -0000
@@ -492,6 +492,7 @@ struct i2c_msg {
 	__u16 flags;
 #define I2C_M_TEN		0x0010	/* this is a ten bit chip address */
 #define I2C_M_RD		0x0001	/* read data, from slave to master */
+#define I2C_M_SLAVE		0x0002	/* from master client to slave adap */
 #define I2C_M_NOSTART		0x4000	/* if I2C_FUNC_PROTOCOL_MANGLING */
 #define I2C_M_REV_DIR_ADDR	0x2000	/* if I2C_FUNC_PROTOCOL_MANGLING */
 #define I2C_M_IGNORE_NAK	0x1000	/* if I2C_FUNC_PROTOCOL_MANGLING */
--- a/drivers/i2c/i2c-dev.c	24 Feb 2010 18:52:17 -0000
+++ b/drivers/i2c/i2c-dev.c	26 Feb 2010 15:49:51 -0000
@@ -270,12 +270,18 @@ static noinline int i2cdev_ioctl_rdrw(st
 
 	res = i2c_transfer(client->adapter, rdwr_pa, rdwr_arg.nmsgs);
 	while (i-- > 0) {
-		if (res >= 0 && (rdwr_pa[i].flags & I2C_M_RD)) {
+		if (res >= 0 && (rdwr_pa[i].flags & (I2C_M_RD|I2C_M_SLAVE))) {
 			if (copy_to_user(data_ptrs[i], rdwr_pa[i].buf,
 					 rdwr_pa[i].len))
 				res = -EFAULT;
 		}
 		kfree(rdwr_pa[i].buf);
+		if (res >= 0 && (rdwr_pa[i].flags & I2C_M_SLAVE)) {
+			struct i2c_msg __user *umsgs;
+			umsgs = ((struct i2c_rdwr_ioctl_data __user *)arg)->msgs;
+			if (put_user(rdwr_pa[i].len, &umsgs[i].len))
+				res = -EFAULT;
+		}
 	}
 	kfree(data_ptrs);
 	kfree(rdwr_pa);
--- a/drivers/i2c/busses/i2c-mpc.c	3 Dec 2009 03:51:21 -0000
+++ b/drivers/i2c/busses/i2c-mpc.c	27 Apr 2010 11:01:47 -0000
@@ -4,7 +4,7 @@
 
  * This is a combined i2c adapter and algorithm driver for the
  * MPC107/Tsi107 PowerPC northbridge and processors that include
- * the same I2C unit (8240, 8245, 85xx).
+ * the same I2C unit (8240, 8245, 83xx, 85xx).
  *
  * Release 0.8
  *
@@ -31,6 +31,7 @@
 
 #define DRV_NAME "mpc-i2c"
 
+#define MPC_I2C_ADDR  0x00
 #define MPC_I2C_FDR   0x04
 #define MPC_I2C_CR    0x08
 #define MPC_I2C_SR    0x0c
@@ -73,6 +74,11 @@ struct mpc_i2c_match_data {
 	u32 prescaler;
 };
 
+static inline void writeaddr(struct mpc_i2c *i2c, u32 x)
+{
+	writeb(x, i2c->base + MPC_I2C_ADDR);
+}
+
 static inline void writeccr(struct mpc_i2c *i2c, u32 x)
 {
 	writeb(x, i2c->base + MPC_I2C_CR);
@@ -428,6 +434,68 @@ static int mpc_read(struct mpc_i2c *i2c,
 	return length;
 }
 
+static int mpc_slave_xrecv(struct mpc_i2c *i2c, int target,
+		    u8 *data, int length, int restart)
+{
+	unsigned timeout = i2c->adap.timeout;
+	int i, result, srw;
+	int readlen = 0;
+	u32 flags;
+
+	/* Listening address */
+	writeaddr(i2c, target);
+	/* Start with MEN */
+	writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_MTX);
+
+	/* Give I2C signals sufficent time to settle */
+	udelay(30);
+
+	result = i2c_wait(i2c, timeout, 1);
+	if (result < 0) {
+		writeccr(i2c, 0);
+		return result;
+	}
+#if 0
+	srw = readb(i2c->base + MPC_I2C_SR) & CSR_SRW;
+#else
+	/* TODO: handle and test SRW!=0 */
+	srw = 0;
+#endif
+	flags = srw ? CCR_MTX : 0;
+
+	if (length) {
+		if (length == 1)
+			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_TXAK | flags);
+		else
+			writeccr(i2c, CCR_MIEN | CCR_MEN | flags);
+		/* Dummy read/write */
+		if (srw)
+			writeb(0, i2c->base + MPC_I2C_DR);
+		else
+			readb(i2c->base + MPC_I2C_DR);
+	}
+
+	for (i = 0; i < length; i++) {
+		result = i2c_wait(i2c, timeout, 0);
+		if (result < 0)
+			return i > 0 && result == -ETIMEDOUT ? i-1 : result;
+		/* Generate stop on last byte */
+		if (i == length - 1)
+			writeccr(i2c, CCR_MIEN | CCR_MEN | CCR_TXAK | flags);
+		data[i] = readb(i2c->base + MPC_I2C_DR);
+		readlen++;
+		/* check early STOP after 2 clock cycles at 100 kHz */
+		udelay(20);
+		if (!(readb(i2c->base + MPC_I2C_SR) & CSR_MBB))
+			break;
+	}
+	/* clear I2CSR register */
+	out_8(i2c->base + MPC_I2C_SR, 0);
+	/* hang up */
+	writeccr(i2c, CCR_MEN);
+	return readlen;
+}
+
 static int mpc_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
 	struct i2c_msg *pmsg;
@@ -459,9 +527,14 @@ static int mpc_xfer(struct i2c_adapter *
 		pmsg = &msgs[i];
 		dev_dbg(i2c->dev,
 			"Doing %s %d bytes to 0x%02x - %d of %d messages\n",
-			pmsg->flags & I2C_M_RD ? "read" : "write",
+			pmsg->flags & I2C_M_SLAVE ? "recv" :
+				pmsg->flags & I2C_M_RD ? "read" : "write",
 			pmsg->len, pmsg->addr, i + 1, num);
-		if (pmsg->flags & I2C_M_RD)
+		if (pmsg->flags & I2C_M_SLAVE) {
+			ret = mpc_slave_xrecv(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
+			if (ret >= 0)
+				pmsg->len = ret;
+		} else if (pmsg->flags & I2C_M_RD)
 			ret =
 			    mpc_read(i2c, pmsg->addr, pmsg->buf, pmsg->len, i);
 		else

Re: implementing a slave driver

[Marc Dietrich]

[-- Attachment #1: Type: Text/Plain, Size: 2799 bytes --]

Fillod Stephane wrote:
> Marc Dietrich wrote:
> > the HW I'm using (tegra) can be configured to act as a slave. This
> > function is
> > needed to communicate with an embedded controller (which is the
> > master). I
> > like to know if this slave function can/should be implemented in the
> > i2c/busses/i2c-tegra.c driver or better outside, e.g. in the driver
> > for the
> > embedded controller.
> 
> Do you need to implement a slave for write-only or also read messages?

it's mostly slave -> master communication. If slave wants to send something to 
the master, it triggers gpio line and master requests the data via interrupt.

> > I haven't found any other hw so far which has this function and it
> > seems that
> > the i2c layer does not provide any interface for the communication
> > with master.
> 
> In fact, the Freescale's MPC8xxx I2C controllers are also capable of
> slave
> operation. I've implemented it, and it's working fine.
> 
> Please find attached a patch against 2.6.35.

For tegra it seems not no simple (see attached patch). The question is if the 
driver should be added to the controller source (i2c-tegra) or in the mfd, where 
the data is getting processed and handed to the appropriate drivers (keyboard, 
mouse, ...). The first method needs some software interface, while the latter 
will tear the code for the same hardware.

> The proposal sent last year[1] relies on a generic API extension.
> <<
> The i2c-dev subsystem is extended with a new flag I2C_M_SLAVE.
> Basically, a struct i2c_msg holding flag I2C_M_SLAVE will wait
> in slave mode for a write request on specified target address,
> and will report the received content to the user-land application.
> 
> An example of driver implementation has been done for i2c-mpc.
> There's no support for read request yet, and I'm wondering whether
> it would make sense. The patch has been tested on powerpc MPC8313E.
> Note: the present i2c-mpc implementation is RFC and may be improved,
> esp. regarding current timing issues.
> 
> [...]
> Talking about i2c-dev, does the I2C_M_SLAVE flag is the appropriate
> way of modelling the API extension? Is there any interest for
> this extension to be accepted in the linux kernel?

I wonder if this API will fit into the scenario descriped above.
 
> Until now, I have no good proposal for the read request.
> It should be doable to pass a static array and let the kernel read from
> it,
> but I see no easy way to have a user task respond with
> compute-as-you-go.

well, I don't care about user-space as the mfd driver will only communicate with 
the master (maybe one can add a user-space interface to the mfd later).

I'm thinking of to let the mfd pass a simple pointer with the msg_buf to the 
slave driver, which should be simple to implement. 

Thanks

Marc

[-- Attachment #2: tegra_slave.patch --]
[-- Type: text/x-patch, Size: 6513 bytes --]

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index e950adf..cfd326f 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -25,6 +27,8 @@
 #include <linux/delay.h>
 #include <linux/slab.h>
 #include <linux/i2c-tegra.h>
+#include <linux/gpio.h>
+#include <linux/err.h>
 
 #include <asm/unaligned.h>
 
@@ -39,8 +43,19 @@
 #define I2C_CNFG_NEW_MASTER_FSM			(1<<11)
 #define I2C_STATUS				0x01C
 #define I2C_SL_CNFG				0x020
+#define I2C_SL_CNFG_NACK			(1<<1)
 #define I2C_SL_CNFG_NEWSL			(1<<2)
+
+#define I2C_SL_INT_RNW				(1<<1)
+#define I2C_SL_INT_RCVD				(1<<2)
+#define I2C_SL_IRQ				(1<<3)
+#define I2C_SL_INT_END_TRANS			(1<<4)
+
+#define I2C_SL_RCVD				0x024
+#define I2C_SL_STATUS				0x028
 #define I2C_SL_ADDR1				0x02c
+#define I2C_SL_ADDR2				0x030
+#define I2C_SL_DELAY_COUNT			0x03c
 #define I2C_TX_FIFO				0x050
 #define I2C_RX_FIFO				0x054
 #define I2C_PACKET_TRANSFER_STATUS		0x058
@@ -116,6 +131,7 @@ struct tegra_i2c_dev {
 	int irq;
 	bool irq_disabled;
 	int is_dvc;
+	int slave_addr;
 	struct completion msg_complete;
 	int msg_err;
 	u8 *msg_buf;
@@ -298,6 +314,23 @@ static void tegra_dvc_init(struct tegra_i2c_dev *i2c_dev)
 	dvc_writel(i2c_dev, val, DVC_CTRL_REG1);
 }
 
+static int tegra_i2c_slave_init(struct tegra_i2c_dev *i2c_dev)
+{
+	printk(KERN_INFO "init slave at 0x%02x\n", i2c_dev->slave_addr);
+
+	i2c_writel(i2c_dev, i2c_dev->slave_addr, I2C_SL_ADDR1);
+	i2c_writel(i2c_dev, 0, I2C_SL_ADDR2);
+
+	i2c_writel(i2c_dev, 0x1E, I2C_SL_DELAY_COUNT);
+	i2c_writel(i2c_dev, I2C_CNFG_NEW_MASTER_FSM, I2C_CNFG);
+	i2c_writel(i2c_dev, I2C_SL_CNFG_NEWSL, I2C_SL_CNFG);
+
+	gpio_request(0xaa, "nvec gpio");
+	gpio_direction_output(0xaa, 0);
+
+	return 0;
+}
+
 static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 {
 	u32 val;
@@ -320,7 +353,6 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 	if (!i2c_dev->is_dvc) {
 		u32 sl_cfg = i2c_readl(i2c_dev, I2C_SL_CNFG);
 		i2c_writel(i2c_dev, sl_cfg | I2C_SL_CNFG_NEWSL, I2C_SL_CNFG);
-		printk(KERN_ALERT "slave cfg: %d\n", sl_cfg | I2C_SL_CNFG_NEWSL);
 	}
 
 	val = 7 << I2C_FIFO_CONTROL_TX_TRIG_SHIFT |
@@ -340,6 +372,97 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 	return 0;
 }
 
+static unsigned char tx_buf[8] = {0x03, 0x05, 0xed, 0x07};
+static unsigned char rx_buf[255];
+static int tx_pos = 0;
+static int rx_pos = 0;
+
+static void enqueue_rx_data(struct work_struct *work)
+{
+#ifdef DEBUG
+	int i;
+	printk(KERN_ALERT "incomming message (%d)\t", rx_pos);
+
+	for(i=0; i<rx_pos; ++i)
+		printk("0x%02x ", rx_buf[i]);
+
+	printk("\n");
+#endif
+}
+
+static DECLARE_WORK(worker, enqueue_rx_data);
+
+static irqreturn_t tegra_i2c_slave_isr(int irq, void *dev_id)
+{
+	u32 status;
+	struct tegra_i2c_dev *i2c_dev = dev_id;
+	unsigned char received;
+
+	status = i2c_readl(i2c_dev, I2C_SL_STATUS);
+
+	if (unlikely(!(status & I2C_SL_IRQ))) {
+		dev_warn(i2c_dev->dev, "irq status 0 %08x %08x %08x\n",
+			 i2c_readl(i2c_dev, I2C_SL_RCVD),
+			 i2c_readl(i2c_dev, I2C_SL_STATUS),
+			 i2c_readl(i2c_dev, I2C_SL_CNFG));
+		i2c_dev->msg_err |= I2C_ERR_UNKNOWN_INTERRUPT;
+
+		if (! i2c_dev->irq_disabled) {
+			disable_irq_nosync(i2c_dev->irq);
+			i2c_dev->irq_disabled = 1;
+		}
+
+		goto err;
+	}
+
+	dev_dbg(i2c_dev->dev, "received int from slave, status: 0x%08x\n", status);
+
+	gpio_direction_output(0xaa, 1);
+
+	if ((status & I2C_SL_INT_END_TRANS) &&
+	   !(status & I2C_SL_INT_RCVD)) {
+		dev_dbg(i2c_dev->dev, "transmission ended here\n");
+		schedule_work(&worker);
+		return IRQ_HANDLED;
+	} else if (status & I2C_SL_INT_RNW) {
+		if (status & I2C_SL_INT_RCVD) {
+			dev_dbg(i2c_dev->dev, "new read comm!\n");
+		} else {
+			dev_dbg(i2c_dev->dev, "read comm continued!\n");
+		}
+		if (tx_pos<sizeof(tx_buf)) {
+			dev_dbg(i2c_dev->dev, "sending 0x%02x\n", tx_buf[tx_pos]);
+			i2c_writel(i2c_dev, tx_buf[tx_pos++], I2C_SL_RCVD);
+		} else {
+			dev_dbg(i2c_dev->dev, "nothing to send\n");
+			i2c_writel(i2c_dev, 0x01, I2C_SL_RCVD);
+		}
+		return IRQ_HANDLED;
+	} else {
+		received = i2c_readl(i2c_dev, I2C_SL_RCVD);
+		i2c_writel(i2c_dev, 0, I2C_SL_RCVD);
+		if (status & I2C_SL_INT_RCVD) {
+			dev_dbg(i2c_dev->dev,
+				"Received new transaction \
+				destined to %02x (we're 0x8a)\n", received);
+			rx_pos = 0;
+			return IRQ_HANDLED;
+		}
+		dev_dbg(i2c_dev->dev, "Got %02x from Master\n", received);
+		rx_buf[rx_pos++] = (unsigned char)received;
+	}
+
+	return IRQ_HANDLED;
+
+err:
+	tegra_i2c_mask_irq(i2c_dev, I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST |
+		I2C_INT_PACKET_XFER_COMPLETE | I2C_INT_TX_FIFO_DATA_REQ |
+		I2C_INT_RX_FIFO_DATA_REQ);
+	i2c_writel(i2c_dev, status, I2C_INT_STATUS);
+
+	return IRQ_HANDLED;
+}
+
 static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 {
 	u32 status;
@@ -620,22 +743,41 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	rt_mutex_init(&i2c_dev->dev_lock);
 
 	i2c_dev->is_dvc = plat->is_dvc;
+	i2c_dev->slave_addr = plat->slave_addr;
 	init_completion(&i2c_dev->msg_complete);
 
 	platform_set_drvdata(pdev, i2c_dev);
 
-	ret = tegra_i2c_init(i2c_dev);
-	if (ret)
-		goto err_free;
+	if (i2c_dev->slave_addr) {
+		printk(KERN_INFO "%s configured as slave\n", pdev->name);
+		ret = tegra_i2c_slave_init(i2c_dev);
+		if (ret)
+			goto err_free;
 
-	ret = request_irq(i2c_dev->irq, tegra_i2c_isr, IRQF_DISABLED,
-		pdev->name, i2c_dev);
-	if (ret) {
-		dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
-		goto err_free;
-	}
+		ret = request_irq(i2c_dev->irq, tegra_i2c_slave_isr, 0,
+				pdev->name, i2c_dev);
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
+			goto err_free;
+		}
 
-	clk_enable(i2c_dev->i2c_clk);
+		clk_enable(clk);
+
+	} else {
+		ret = tegra_i2c_init(i2c_dev);
+		if (ret)
+			goto err_free;
+
+		ret = request_irq(i2c_dev->irq, tegra_i2c_isr, IRQF_DISABLED,
+			pdev->name, i2c_dev);
+
+		if (ret) {
+			dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq);
+			goto err_free;
+		}
+
+		clk_enable(i2c_dev->i2c_clk);
+	}
 
 	for (i = 0; i < nbus; i++) {
 		struct tegra_i2c_bus *i2c_bus = &i2c_dev->busses[i];
diff --git a/include/linux/i2c-tegra.h b/include/linux/i2c-tegra.h
index 6123502..8b07b01 100644
--- a/include/linux/i2c-tegra.h
+++ b/include/linux/i2c-tegra.h
@@ -29,6 +29,7 @@ struct tegra_i2c_platform_data {
 	int bus_mux_len[TEGRA_I2C_MAX_BUS];
 	unsigned long bus_clk_rate[TEGRA_I2C_MAX_BUS];
 	bool is_dvc;
+	int slave_addr;
 };
 
 #endif /* _LINUX_I2C_TEGRA_H */