[RFC 0/1] i2c: omap: Add support for switching to slave mode

[RFC 0/1] i2c: omap: Add support for switching to slave mode

[Ravikumar Kattekola]

I2C controller on most of the omap devices has both master and slave
capability but the i2c framework has been missing support for registering
a bus in slave mode for long.
Recently the i2c slave support has been added to i2c framework, the following
patch adds the required support for omap_i2c driver to register a controller
as a slave device and be deriven by an external/internal master.

The slave interface requires us to add following mandatory events

1. I2C_SLAVE_WRITE_REQUESTED
2. I2C_SLAVE_READ_REQUESTED
3. I2C_SLAVE_WRITE_RECEIVED
4. I2C_SLAVE_READ_PROCESSED

and 

5. I2C_SLAVE_STOP

The omap i2c controller (at least on dra7x devices)
doesn't have  start/stop (STT/STP) support for slave mode
so event  #5 is not implemented in the driver.

Refer to Documentation/i2c/slave-interface for more info on 
i2c-slave-interface and Documentation/i2c/slave-eeprom-backend for 
sample backend driver.

Tested on:
DRA75x EVM Rev G3 and DRA72x EVM Rev B1 connected over i2c3 using DCAN2 lines [JP3]

Ravikumar Kattekola (1):
  drivers: i2c: omap: Add slave support

 drivers/i2c/busses/i2c-omap.c | 144 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 144 insertions(+)

-- 
2.8.2.396.g5fe494c

[RFC 1/1] drivers: i2c: omap: Add slave support

[Ravikumar Kattekola]

I2C controller on most of the omap devices has both master and slave
capability but the i2c framework has been missing support for registering
a bus in slave mode for long.

Recently the i2c slave support has been added to i2c framework,
the following patch adds the required support for omap_i2c driver to
register a controller as a slave device and be deriven by
an external/internal master.

The slave interface requires us to add following mandatory events

1. I2C_SLAVE_WRITE_REQUESTED
2. I2C_SLAVE_READ_REQUESTED
3. I2C_SLAVE_WRITE_RECEIVED
4. I2C_SLAVE_READ_PROCESSED

and

5. I2C_SLAVE_STOP

The omap i2c controller (at least on dra7x devices)
doesn't have  start/stop (STT/STP) support for slave mode
so event  #5 is not implemented in the driver.

Signed-off-by: Ravikumar Kattekola <rk@ti.com>
---
 drivers/i2c/busses/i2c-omap.c | 144 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 144 insertions(+)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index ab1279b..ccfc49f 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -89,6 +89,7 @@ enum {
 /* I2C Interrupt Enable Register (OMAP_I2C_IE): */
 #define OMAP_I2C_IE_XDR		(1 << 14)	/* TX Buffer drain int enable */
 #define OMAP_I2C_IE_RDR		(1 << 13)	/* RX Buffer drain int enable */
+#define OMAP_I2C_IE_AAS		(1 << 9)	/* Addressed as Slave int enable */
 #define OMAP_I2C_IE_XRDY	(1 << 4)	/* TX data ready int enable */
 #define OMAP_I2C_IE_RRDY	(1 << 3)	/* RX data ready int enable */
 #define OMAP_I2C_IE_ARDY	(1 << 2)	/* Access ready int enable */
@@ -202,6 +203,7 @@ struct omap_i2c_dev {
 	u8			*regs;
 	size_t			buf_len;
 	struct i2c_adapter	adapter;
+	struct i2c_client	*slave;
 	u8			threshold;
 	u8			fifo_size;	/* use as flag and value
 						 * fifo_size==0 implies no fifo
@@ -1003,6 +1005,62 @@ omap_i2c_isr(int irq, void *dev_id)
 	return ret;
 }
 
+#ifdef CONFIG_I2C_SLAVE
+static int omap_i2c_slave_irq(struct omap_i2c_dev *omap)
+{
+	u16 stat_raw;
+	u16 stat;
+	u16 bits;
+	u8 value;
+
+	stat_raw = omap_i2c_read_reg(omap, OMAP_I2C_IP_V2_IRQSTATUS_RAW);
+	bits = omap_i2c_read_reg(omap, OMAP_I2C_IE_REG);
+	stat_raw &= bits;
+
+	if (stat_raw & OMAP_I2C_STAT_AAS) {
+		omap_i2c_ack_stat(omap, OMAP_I2C_STAT_AAS);
+		stat_raw &= ~OMAP_I2C_STAT_AAS;
+	}
+
+	/* Someone's just sayin Hi? okay bye..*/
+	if (!stat_raw)
+		goto out;
+
+	dev_dbg(omap->dev, "IRQ (ISR = 0x%04x)\n", stat_raw);
+
+	if (stat_raw & OMAP_I2C_STAT_RRDY)
+		i2c_slave_event(omap->slave, I2C_SLAVE_WRITE_REQUESTED, &value);
+
+	do {
+		bits = omap_i2c_read_reg(omap, OMAP_I2C_IE_REG);
+		stat = omap_i2c_read_reg(omap, OMAP_I2C_STAT_REG);
+		stat &= bits;
+
+		if (stat & OMAP_I2C_STAT_AAS)
+			omap_i2c_ack_stat(omap, OMAP_I2C_STAT_AAS);
+
+		if (stat & OMAP_I2C_STAT_RRDY) {
+			value = omap_i2c_read_reg(omap, OMAP_I2C_DATA_REG);
+			i2c_slave_event(omap->slave, I2C_SLAVE_WRITE_RECEIVED,
+					&value);
+			omap_i2c_ack_stat(omap, OMAP_I2C_STAT_RRDY);
+		}
+
+		if (stat & OMAP_I2C_STAT_XRDY) {
+			i2c_slave_event(omap->slave, I2C_SLAVE_READ_REQUESTED,
+					&value);
+			omap_i2c_write_reg(omap, OMAP_I2C_DATA_REG, value);
+			i2c_slave_event(omap->slave, I2C_SLAVE_READ_PROCESSED,
+					&value);
+			omap_i2c_ack_stat(omap, OMAP_I2C_STAT_XRDY);
+		}
+
+	} while (stat);
+out:
+	return 0;
+}
+#endif
+
 static irqreturn_t
 omap_i2c_isr_thread(int this_irq, void *dev_id)
 {
@@ -1011,6 +1069,13 @@ omap_i2c_isr_thread(int this_irq, void *dev_id)
 	u16 stat;
 	int err = 0, count = 0;
 
+#ifdef CONFIG_I2C_SLAVE
+	if (omap->slave) {
+		/* If a slave is registered pass on the interrupt */
+		err = omap_i2c_slave_irq(omap);
+		goto out;
+	}
+#endif
 	do {
 		bits = omap_i2c_read_reg(omap, OMAP_I2C_IE_REG);
 		stat = omap_i2c_read_reg(omap, OMAP_I2C_STAT_REG);
@@ -1139,9 +1204,88 @@ out:
 	return IRQ_HANDLED;
 }
 
+#ifdef CONFIG_I2C_SLAVE
+static int omap_i2c_reg_slave(struct i2c_client *slave)
+{
+	struct omap_i2c_dev *omap = i2c_get_adapdata(slave->adapter);
+	u16 reg;
+	int ret = 0;
+
+	dev_info(omap->dev, "Registering as a slave @ %x\n", slave->addr);
+
+	/* Already registered as a slave?
+	 * XXX: OMAP I2c controller supports up to four
+	 * different OA, can we register as four different
+	 * slaves?
+	 */
+	if (omap->slave)
+		return -EBUSY;
+
+	ret = pm_runtime_get_sync(omap->dev);
+	if (ret < 0)
+		return ret;
+
+	omap->slave = slave;
+
+	/* Write OA: So that master(s) can talk to us */
+	omap_i2c_write_reg(omap, OMAP_I2C_OA_REG, slave->addr);
+
+	/* Set / Switch to slave mode */
+	reg = omap_i2c_read_reg(omap, OMAP_I2C_CON_REG);
+	reg &= ~OMAP_I2C_CON_MST;
+	omap_i2c_write_reg(omap, OMAP_I2C_CON_REG, reg);
+
+	/* Clear status */
+	omap_i2c_write_reg(omap, OMAP_I2C_SYSS_REG, 0);
+
+	/* As of now, We dont need all interrupts be enabled */
+	omap->iestate = OMAP_I2C_IE_AAS | OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY;
+
+	/* Clear interrupt mask */
+	omap_i2c_write_reg(omap, OMAP_I2C_IP_V2_IRQENABLE_CLR, 0xffff);
+
+	dev_dbg(omap->dev, "omap->iestate 0x%04x\n", omap->iestate);
+
+	/* Enable necessary interrupts */
+	omap_i2c_write_reg(omap, OMAP_I2C_IE_REG, omap->iestate);
+
+	return 0;
+
+}
+
+static int omap_i2c_unreg_slave(struct i2c_client *slave)
+{
+	struct omap_i2c_dev *omap = i2c_get_adapdata(slave->adapter);
+	u16 reg;
+
+	WARN_ON(!omap->slave);
+	omap->slave = NULL;
+
+	/* Switch to master mode */
+	reg = omap_i2c_read_reg(omap, OMAP_I2C_CON_REG);
+	reg |= OMAP_I2C_CON_MST;
+	omap_i2c_write_reg(omap, OMAP_I2C_CON_REG, reg);
+
+	/* clear status */
+	omap_i2c_write_reg(omap, OMAP_I2C_SYSS_REG, 0);
+
+	/* Just to make sure re-init in master mode
+	 * Should we do a reset here?
+	 */
+	omap_i2c_init(omap);
+
+	pm_runtime_put(omap->dev);
+	return 0;
+}
+#endif
+
 static const struct i2c_algorithm omap_i2c_algo = {
 	.master_xfer	= omap_i2c_xfer,
 	.functionality	= omap_i2c_func,
+#ifdef CONFIG_I2C_SLAVE
+	.reg_slave	= omap_i2c_reg_slave,
+	.unreg_slave	= omap_i2c_unreg_slave,
+#endif /* CONFIG_I2C_SLAVE */
 };
 
 #ifdef CONFIG_OF
-- 
2.8.2.396.g5fe494c

Re: [RFC 1/1] drivers: i2c: omap: Add slave support

[Manish Badarkhe]

Hi Ravikumar

Some sanity comments, just good to have.

> +#ifdef CONFIG_I2C_SLAVE
> +static int omap_i2c_slave_irq(struct omap_i2c_dev *omap)
> +{
> +       u16 stat_raw;
> +       u16 stat;
> +       u16 bits;
> +       u8 value;
> +
> +       stat_raw = omap_i2c_read_reg(omap, OMAP_I2C_IP_V2_IRQSTATUS_RAW);
> +       bits = omap_i2c_read_reg(omap, OMAP_I2C_IE_REG);
> +       stat_raw &= bits;
> +
> +       if (stat_raw & OMAP_I2C_STAT_AAS) {
> +               omap_i2c_ack_stat(omap, OMAP_I2C_STAT_AAS);
> +               stat_raw &= ~OMAP_I2C_STAT_AAS;
> +       }
> +
> +out:
> +       return 0;
> +}
> +#endif

This function always return 0.

> +#ifdef CONFIG_I2C_SLAVE
> +static int omap_i2c_reg_slave(struct i2c_client *slave)
> +{
> +       struct omap_i2c_dev *omap = i2c_get_adapdata(slave->adapter);
> +       u16 reg;
> +       int ret = 0;
> +       /* Enable necessary interrupts */
> +       omap_i2c_write_reg(omap, OMAP_I2C_IE_REG, omap->iestate);
> +
> +       return 0;
> +
> +}

Better to return "ret" here as already been initialized to 0

> +
> +static int omap_i2c_unreg_slave(struct i2c_client *slave)
> +{
> +       struct omap_i2c_dev *omap = i2c_get_adapdata(slave->adapter);
> +       u16 reg;
> +
> +       pm_runtime_put(omap->dev);
> +       return 0;
> +}
> +#endif

This function always return 0


Regards
Manish Badarkhe

Re: [RFC 1/1] drivers: i2c: omap: Add slave support

[Wolfram Sang]

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

Hi,

> The omap i2c controller (at least on dra7x devices)
> doesn't have  start/stop (STT/STP) support for slave mode
> so event  #5 is not implemented in the driver.

I think you can deduce that. If a new {READ|WRITE}_REQUESTED slave event
comes in when you had *_PROCESSED events before, there must have been a
STOP on the bus inbetween.

> +		if (stat & OMAP_I2C_STAT_XRDY) {
> +			i2c_slave_event(omap->slave, I2C_SLAVE_READ_REQUESTED,
> +					&value);
> +			omap_i2c_write_reg(omap, OMAP_I2C_DATA_REG, value);
> +			i2c_slave_event(omap->slave, I2C_SLAVE_READ_PROCESSED,
> +					&value);

This looks fishy. READ_REQUESTED is only sent after the address phase.
Have you read the documentation (Documentation/i2c/slave-interface)?
Please say if it was unclear.

> +	/* As of now, We dont need all interrupts be enabled */
> +	omap->iestate = OMAP_I2C_IE_AAS | OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY;

This looks even more fishy. Are you disabling the master interrupts?
That's a no (unless there are HW constraints). Your driver should be
able to switch between master and slave depending on what happens on the
bus.

For more guidance, here is my talk at ELCE 2015:
https://www.youtube.com/watch?v=JdQ21jlwb58

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 1/1] drivers: i2c: omap: Add slave support

[Matthijs van Duin]

Greetings, unfortunate souls trying to use the omap-i2c peripheral in
slave mode! :-)

I recently posted some stuff about exactly that topic on TI's E2E
forum, you may want to read this warning:
http://e2e.ti.com/support/arm/sitara_arm/f/791/p/514961/1955843#1955843

and post contains suggestions on using slave mode and details on the
peripheral actually behaves:
http://e2e.ti.com/support/arm/sitara_arm/f/791/p/514961/1959417#1959417

Matthijs

Re: [RFC 1/1] drivers: i2c: omap: Add slave support

[Wolfram Sang]

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


> Greetings, unfortunate souls trying to use the omap-i2c peripheral in
> slave mode! :-)

Uh, that sounds like bad HW...

> I recently posted some stuff about exactly that topic on TI's E2E
> forum, you may want to read this warning:
> http://e2e.ti.com/support/arm/sitara_arm/f/791/p/514961/1955843#1955843
> 
> and post contains suggestions on using slave mode and details on the
> peripheral actually behaves:
> http://e2e.ti.com/support/arm/sitara_arm/f/791/p/514961/1959417#1959417

While it surely is nice to have super detailed information, can you give
this overloaded maintainer a few-line, high level summary of what is
written there?

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 1/1] drivers: i2c: omap: Add slave support

[Matthijs van Duin]

On 27 August 2016 at 19:22, Wolfram Sang <wsa@the-dreams.de> wrote:
> Uh, that sounds like bad HW...

Making a mess of I2C controllers seems to be a popular hobby among
chip designers :P

( I also really like how the RPi handles clock stretching... *cough* )

> While it surely is nice to have super detailed information, can you give
> this overloaded maintainer a few-line, high level summary of what is
> written there?

What's written there is what should have been in the docs: correct (as
in reality-matching) details of how the thing actually works in slave
mode, and how to operate it without risking race conditions. They're
not comprehensive, and certainly not polished since they are my
personal notes I've made while studying the peripheral in detail, but
I figured both on E2E and here I might perhaps save time and
frustration by making them available.

A lot of the details (including the completely bizarre behaviour of
its innocuous-looking irq registers) would be quite non-trivial to
figure out without putting in a similar effort.

Matthijs

Re: [RFC 1/1] drivers: i2c: omap: Add slave support

[Wolfram Sang]

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


> Making a mess of I2C controllers seems to be a popular hobby among
> chip designers :P

Well, I2C is simple, what could go wrong? :/

> A lot of the details (including the completely bizarre behaviour of
> its innocuous-looking irq registers) would be quite non-trivial to
> figure out without putting in a similar effort.

Thanks. So, it is possible to make a proper I2C slave with OMAP, but you
need to know those 100 gory details?

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [RFC 1/1] drivers: i2c: omap: Add slave support

[Matthijs van Duin]

On 28 August 2016 at 07:35, Wolfram Sang <wsa@the-dreams.de> wrote:
> Well, I2C is simple, what could go wrong? :/

Actually I2C is elegant and *seems* simple, but in all its
asynchronicity there are actually a surprising number of fine details
you can trip over.  Maybe that's why so many i2c controllers suck: since
i2c looks simple enough manufacturers are easily tempted to roll their
own instead of licensing a good implementation.

Having said that, most of the inconsistency and obnoxiousness of the TI
I2C controller is not even excusable by that argument.  For example its
irq registers *look* like the usual set { rawstatus, status, en, dis }
that's their current standard ("Highlander") for peripherals. They do
not however *behave* like the standard set however:
  1. status isn't always (rawstatus & enabled)
  2. status != 0 does not always imply the irq output is asserted
  3. some enable-bits also change the behaviour of rawstatus
All of these misbehaviours are unprecedented afaik.

Normally you'd also expect each irq (raw)status bit to either
  a. be an event, set by hw and can be cleared by software any time, or
  b. be a level status, unaffected by software attempts to set/clear.
Again the i2c controller decided this is far too little diversity.

> So, it is possible to make a proper I2C slave with OMAP, but you need
> to know those 100 gory details?

Mostly.  There are some limitations such as:

* No ability to selectively ACK/NACK when addressed as slave. If you're
unable to respond for some time then you'd end up blocking the bus with
clock stretching.  You could temporarily deconfigure your slave address
but the TRM states changing slave address is forbidden while bus busy.

* According to my notes it always ACKs a General Call and this cannot
even be stalled using the SBLOCK register.  Since I don't care about GC
there's no more details in my notes, but if this is true then on any bus
where GC is used, irq handling will have real-time deadlines to avoid
losing track of transaction boundaries and misinterpreting data.

Finally, as my first link pointed out, various protocol errors can lock
up the peripheral's internal state machine.  When operating as slave
this is basically undetectable: all registers look normal and the
bus-busy bit will continue to track start/stop, but the peripheral will
not ACK any slave address anymore until you reset it.

You could argue "well, but that requires bus protocol errors" but it is
nevertheless a direct violation of the I2C standard:

	I2C-bus compatible devices must reset their bus logic on receipt
	of a START or repeated START condition such that they all
	anticipate the sending of a slave address, even if these START
	conditions are not positioned according to the proper format.

Also, my testing showed pulsing SDA low on an idle bus sufficed to
trigger this state.  It needs to pass the glitch filter of course, but
this filter is implemented by sampling the bus requiring two consecutive
samples to agree.  Two small glitches with just the right timing would
therefore suffice.  Rather unlikely for random noise, but having lots of
signals on your pcb that ultimately derive from the same clock source
probably makes the odds a lot more favorable.

Matthijs

Re: [RFC 1/1] drivers: i2c: omap: Add slave support

[Ravikumar]

On Monday 29 August 2016 09:13 AM, Matthijs van Duin wrote:
> On 28 August 2016 at 07:35, Wolfram Sang <wsa@the-dreams.de> wrote:
>> Well, I2C is simple, what could go wrong? :/
> Actually I2C is elegant and *seems* simple, but in all its
> asynchronicity there are actually a surprising number of fine details
> you can trip over.  Maybe that's why so many i2c controllers suck: since
> i2c looks simple enough manufacturers are easily tempted to roll their
> own instead of licensing a good implementation.
>
> Having said that, most of the inconsistency and obnoxiousness of the TI
> I2C controller is not even excusable by that argument.  For example its
> irq registers *look* like the usual set { rawstatus, status, en, dis }
> that's their current standard ("Highlander") for peripherals. They do
> not however *behave* like the standard set however:
>    1. status isn't always (rawstatus & enabled)
>    2. status != 0 does not always imply the irq output is asserted
>    3. some enable-bits also change the behaviour of rawstatus
> All of these misbehaviours are unprecedented afaik.
If I understand #1 correctly, you mean that bit value is different in 
raw vs status registers.
I've seen some times there was a delay in the value reflecting the 
status register.
So I choose to use the raw register.


Now #2 and #3 would be crazy, do you have further notes on this?
If I can reproduce these then I will follow up with the IP/HW team.
> Normally you'd also expect each irq (raw)status bit to either
>    a. be an event, set by hw and can be cleared by software any time, or
>    b. be a level status, unaffected by software attempts to set/clear.
> Again the i2c controller decided this is far too little diversity.

yeah, seems so on dm814x.

But, at least the description has been updated on Jacinto 6 device.

I see all 'status' bits are write 1 to clear except for Bus Busy (intended).

While the 'raw' status register bits can not be cleared by writing 1,
the description says write 1 to set the bit for debug purpose.

>> So, it is possible to make a proper I2C slave with OMAP, but you need
>> to know those 100 gory details?
> Mostly.  There are some limitations such as:
>
> * No ability to selectively ACK/NACK when addressed as slave. If you're
> unable to respond for some time then you'd end up blocking the bus with
> clock stretching.  You could temporarily deconfigure your slave address
> but the TRM states changing slave address is forbidden while bus busy.
Does this lead to bus lock up?
> * According to my notes it always ACKs a General Call and this cannot
> even be stalled using the SBLOCK register.  Since I don't care about GC
> there's no more details in my notes, but if this is true then on any bus
> where GC is used, irq handling will have real-time deadlines to avoid
> losing track of transaction boundaries and misinterpreting data.
>
> Finally, as my first link pointed out, various protocol errors can lock
> up the peripheral's internal state machine.  When operating as slave
> this is basically undetectable: all registers look normal and the
> bus-busy bit will continue to track start/stop, but the peripheral will
> not ACK any slave address anymore until you reset it.
>
> You could argue "well, but that requires bus protocol errors" but it is
> nevertheless a direct violation of the I2C standard:
>
> 	I2C-bus compatible devices must reset their bus logic on receipt
> 	of a START or repeated START condition such that they all
> 	anticipate the sending of a slave address, even if these START
> 	conditions are not positioned according to the proper format.
>
> Also, my testing showed pulsing SDA low on an idle bus sufficed to
> trigger this state.  It needs to pass the glitch filter of course, but
> this filter is implemented by sampling the bus requiring two consecutive
> samples to agree.  Two small glitches with just the right timing would
> therefore suffice.  Rather unlikely for random noise, but having lots of
> signals on your pcb that ultimately derive from the same clock source
> probably makes the odds a lot more favorable.
>
> Matthijs
Thanks for sharing the steps to reproduce.

Regards,
RK

Re: [RFC 1/1] drivers: i2c: omap: Add slave support

[Matthijs van Duin]

On 14 October 2016 at 09:56, Ravikumar <a0131654@ti.com> wrote:
> Dynamic switching on OMAP I2C IP could be a difficult task.

In fact I wouldn't even bother trying. The thread I linked to received
some follow-up by someone wrestling with this and it actually resulted
in the suggestion to use separate master-only and slave-only peripherals
on the same bus just to avoid the horrible race conditions:
https://e2e.ti.com/support/arm/sitara_arm/f/791/p/514961/1990938#1990938


On 14 October 2016 at 10:57, Ravikumar <rk@ti.com> wrote:
>
> On Monday 29 August 2016 09:13 AM, Matthijs van Duin wrote:
>> its irq registers *look* like the usual set { rawstatus, status, en, dis }
>> that's their current standard ("Highlander") for peripherals. They do
>> not however *behave* like the standard set however:
>>    1. status isn't always (rawstatus & enabled)
>>    2. status != 0 does not always imply the irq output is asserted
>>    3. some enable-bits also change the behaviour of rawstatus
>> All of these misbehaviours are unprecedented afaik.
>
> If I understand #1 correctly, you mean that bit value is different in raw vs
> status registers. I've seen some times there was a delay in the value
> reflecting the status register.

I've never seen that myself, in fact how do you even observe that?  I'd
expect such propagation to require at most one cycle, and I don't expect
an irq output would get updated until it does.

> Now #2 and #3 would be crazy, do you have further notes on this?

I'll clarify what I meant.

Normally you can partition the irq bits into two types: level and event.
Level bits reflect some internal status line and cannot be manually set
or cleared.  Handling these involves taking some action to clear the
underlying condition, or disabling its reporting if you no longer care.

Event bits are set by a peripheral when some event occurs, and need to
be manually cleared as part of irq handling.  They can also be manually
set, e.g. for debugging.

The standard irq combiner in modern ("Highlander") TI peripherals can be
modeled roughly by the following code:


#define IRQ_BITS (EVENT_BITS | LEVEL_BITS)

// irq combiner private state
static u32 latched;	// subset of EVENT_BITS
static u32 rawstatus;	// subset of IRQ_BITS
static u32 enabled;	// subset of IRQ_BITS

// signals from peripheral
extern u32 input;	// subset of IRQ_BITS

// irq output signal
bool output;

// propagate signals
void update()
{
	latched |= input & EVENT_BITS;
	rawstatus = latched | input;
	output = !!(rawstatus & enabled);
}

// register interface
u32 read_rawstatus() {  return rawstatus;  }
u32 read_status() {  return rawstatus & enabled;  }
u32 read_enable() {  return enabled;  }
u32 read_disable() {  return enabled;  }
void write_rawstatus(u32 x) {  latched |= x & EVENT_BITS;  update();  }
void write_status(u32 x) {  latched &= ~(x & EVENT_BITS);  update();  }
void write_enable(u32 x) {  enabled |= x & IRQ_BITS;  update();  }
void write_disable(u32 x) {  enabled &= ~(x & IRQ_BITS);  update();  }


(Some variations exist, e.g. u64 instead of u32.)



Now the omap-i2c peripheral's irq combiner looks like this at first
sight, but it violates its semantics in several ways:


#define LEVEL_BITS (BB | ((XUDF | ROVR) & ~enabled))
#define EVENT_BITS (0x7fff & ~BB)
#define IRQ_BITS EVENT_BITS  // BB isn't an irq at all

void update()
{
	latched |= input & EVENT_BITS;
	rawstatus = (latched & ~LEVEL_BITS) | (input & LEVEL_BITS);
	// note: (rawstatus & enabled) == (latched & enabled)
	output = !!(rawstatus & enabled);
}

u32 read_status() {  return rawstatus & (enabled | LEVEL_BITS);  }


Hence:

1. level irqs are visible in status even if not enabled.

2. as direct consequence, read_status() != 0 doesn't necessarily mean
irq output is asserted.

3. when XUDF or ROVR is disabled it shows the level version in both
rawstatus and status.  It is then however still latched, and the latched
version will show up when you enable it.  You can set and clear the
latched bit also when it is disabled (hence invisible).

Also:

4. BB isn't even something reported via irq, what is it doing here?

5. Even though all inputs are latched, some of them are actually level
status signals from the peripheral, which means that to handle them you
have to clear the underlying condition and then additionally clear the
latch.  Very convenient.  XRDY behaves like this in master mode, but is
a normal event (pulsed) in slave mode.  Yay for consistency!

I also have more comments on it in this thread:

https://e2e.ti.com/support/arm/sitara_arm/f/791/p/533394/1966122#1966122

> But, at least the description has been updated on Jacinto 6 device.
>
> I see all 'status' bits are write 1 to clear except for Bus Busy (intended).
>
> While the 'raw' status register bits can not be cleared by writing 1,
> the description says write 1 to set the bit for debug purpose.

That looks like mostly a copy-paste of the documentation of the standard
register interface.  It still appears to be wrong, unless the behaviour
has actually changed since the dm814x.


>> * No ability to selectively ACK/NACK when addressed as slave. If you're
>> unable to respond for some time then you'd end up blocking the bus with
>> clock stretching.  You could temporarily deconfigure your slave address
>> but the TRM states changing slave address is forbidden while bus busy.
>
> Does this lead to bus lock up?

No idea.


Matthijs

Re: [RFC 1/1] drivers: i2c: omap: Add slave support

[Ravikumar]

On Saturday 27 August 2016 07:29 PM, Matthijs van Duin wrote:
> Greetings, unfortunate souls trying to use the omap-i2c peripheral in
> slave mode! :-)
That would be me :(  and greetings to you too :)
> I recently posted some stuff about exactly that topic on TI's E2E
> forum, you may want to read this warning:
> http://e2e.ti.com/support/arm/sitara_arm/f/791/p/514961/1955843#1955843
> and post contains suggestions on using slave mode and details on the
> peripheral actually behaves:
> http://e2e.ti.com/support/arm/sitara_arm/f/791/p/514961/1959417#1959417

Thanks for doing extensive testing on the OMAP I2C and preserving your 
notes.
I really appreciate sharing the detailed notes and workarounds to avoid 
lockups.

I'll following up with the HW team on your observations.
>
> Matthijs

Re: [RFC 1/1] drivers: i2c: omap: Add slave support

[Ravikumar]

On Thursday 25 August 2016 10:44 PM, Wolfram Sang wrote:
> Hi,
>
>> The omap i2c controller (at least on dra7x devices)
>> doesn't have  start/stop (STT/STP) support for slave mode
>> so event  #5 is not implemented in the driver.
> I think you can deduce that. If a new {READ|WRITE}_REQUESTED slave event
> comes in when you had *_PROCESSED events before, there must have been a
> STOP on the bus inbetween.
I've found that the Bus free interrupt can be used for STOP condition in 
slave mode.
So this shouldn't be a problem anymore.
>
>> +		if (stat & OMAP_I2C_STAT_XRDY) {
>> +			i2c_slave_event(omap->slave, I2C_SLAVE_READ_REQUESTED,
>> +					&value);
>> +			omap_i2c_write_reg(omap, OMAP_I2C_DATA_REG, value);
>> +			i2c_slave_event(omap->slave, I2C_SLAVE_READ_PROCESSED,
>> +					&value);
> This looks fishy. READ_REQUESTED is only sent after the address phase.
> Have you read the documentation (Documentation/i2c/slave-interface)?
> Please say if it was unclear.
Will fix this.
>> +	/* As of now, We dont need all interrupts be enabled */
>> +	omap->iestate = OMAP_I2C_IE_AAS | OMAP_I2C_IE_XRDY | OMAP_I2C_IE_RRDY;
> This looks even more fishy. Are you disabling the master interrupts?
> That's a no (unless there are HW constraints). Your driver should be
> able to switch between master and slave depending on what happens on the
> bus.
Dynamic switching on OMAP I2C IP could be a  difficult task.
There is no separate status register for master mode vs slave mode, it's 
a common register.
Even the interrupt status bits are reused.

So i cant do a check on status like if(!MSR) slave_irq_handler();

I think instead of status I may need to check the MST(1:master mode, 
0:slave mode] bit in I2C_CON
to take a decision on whether to call slave irq_handler or not.
> For more guidance, here is my talk at ELCE 2015:
> https://www.youtube.com/watch?v=JdQ21jlwb58
Thanks for sharing the video.
>
> Regards,
>
>     Wolfram
>

Regards,
RK