CX24116 i2c patch

CX24116 i2c patch

[Steven Toth]

Mauro,

> Subject: [media] cx24116: add config option to split firmware download
> Author:  Antti Palosaari <crope@iki.fi>
> Date:    Wed Apr 27 21:03:07 2011 -0300
>
> It is very rare I2C adapter hardware which can provide 32kB I2C write
> as one write. Add .i2c_wr_max option to set desired max packet size.
> Split transaction to smaller pieces according to that option.

This is none-sense. I'm naking this patch, please unqueue, regress or whatever.

The entire point of the i2c message send is that the i2c drivers know
nothing about the host i2c implementation, and they should not need
to. I2C SEND and RECEIVE are abstract and require no knowledge of the
hardware. This is dangerous and generates non-atomic register writes.
You cannot guarantee that another thread isn't reading/writing to
other registers in the part - breaking the driver.

Please fix the host controller to split the i2c messages accordingly
(and thus keeping the entire transaction atomic).

This is the second time I've seen the 'fix' to a problem by patching
the i2c driver. Fix the i2c bridge else we'll see this behavior
spreading to multiple i2c driver. It's just wrong.

Best,

-- 
Steven Toth - Kernel Labs
http://www.kernellabs.com

Re: CX24116 i2c patch

[Mauro Carvalho Chehab]

Hi Steven,

Em 05-05-2011 09:28, Steven Toth escreveu:
> Mauro,
> 
>> Subject: [media] cx24116: add config option to split firmware download
>> Author:  Antti Palosaari <crope@iki.fi>
>> Date:    Wed Apr 27 21:03:07 2011 -0300
>>
>> It is very rare I2C adapter hardware which can provide 32kB I2C write
>> as one write. Add .i2c_wr_max option to set desired max packet size.
>> Split transaction to smaller pieces according to that option.
> 
> This is none-sense. I'm naking this patch, please unqueue, regress or whatever.
> 
> The entire point of the i2c message send is that the i2c drivers know
> nothing about the host i2c implementation, and they should not need
> to. I2C SEND and RECEIVE are abstract and require no knowledge of the
> hardware. This is dangerous and generates non-atomic register writes.
> You cannot guarantee that another thread isn't reading/writing to
> other registers in the part - breaking the driver.
> 
> Please fix the host controller to split the i2c messages accordingly
> (and thus keeping the entire transaction atomic).
> 
> This is the second time I've seen the 'fix' to a problem by patching
> the i2c driver. Fix the i2c bridge else we'll see this behavior
> spreading to multiple i2c driver. It's just wrong.

As you pointed, there are two ways of solving this issue: at the I2C device
side, and at the I2C adapter side. By looking on the existing code, you'll
see that some drivers solve this issue at one side, others solve on the other
side, and there are even some cases where both sides implement I2C splits.
On very few places, this is implemented well.

As you pointed, if the I2C split is implemented inside the I2C driver, extra
care is needed to avoid having an I2C transaction from another device in the
middle of an I2C transaction. With the current API, this generally means that
the I2C driver will need to use i2c_transfer() with a large block of transactions.

Also, in general, we don't want to use a full I2C transaction with stop and start
bits, so an extra flag is generally needed to indicate that should that we're using
a "fast" i2c transaction (I2C_M_NOSTART) - more about this subject bellow.

On the other hand, if the split is done at the I2C adapter, this means that the
adapter code can't be generic anymore, as the way I2C transactions are broken
depend on how the I2C device works. For example, on xc2028/3028, when a transaction
is broken, the next transaction needs an I2C "header" with the register bank
that are being updated. Other devices don't need that. Also, as not all I2C
devices accept to work with I2C_M_NOSTART, the logic is more complicated.


So, the I2C adapter xfer code will end by being something like:

switch(i2c_device) {
	case FOO:
		use_split_code_foo();
		break;
	case BAR:
		use_splic_code_bar();
		break;
	...
}


(if you want to see one example of the above, take a look at drivers/media/video/cx231xx/cx231xx-i2c.c).

The end result is very bad, due to:

1) this requires a high couple between the I2C subdriver. If the subdriver code changes,
   all I2C adapters that use that driver also need changes;
2) the same logic should be replicated for all devices that use an specific I2C subdriver;
3) analyzing the code and tracking for troubles is more complex, as the code is splitted on
   two parts;
4) the i2c xfer callback become big, confusing and hard to understand.

On the other hand, in order to warrant atomic operations at the I2C driver, we would need to do
something like:

struct i2c_msg msg[5] = {
	.addr = props->addr, .flags = 0, .buf = buf[0], .len = len[0] },
	.addr = props->addr, .flags = I2C_M_NOSTART, .buf = buf[1], .len = len[1] },
	.addr = props->addr, .flags = I2C_M_NOSTART, .buf = buf[2], .len = len[2] },
	.addr = props->addr, .flags = I2C_M_NOSTART, .buf = buf[3], .len = len[3] },
}
ret = i2c_transfer(props->adap, &msg, 5);

While this warrants that the I2C adapter won't have any transaction from the other devices, in the
cases like firmware transfers, the above API is not optimal.

For example, assuming a 32768 firmware, on an I2C adapter capable of sending up to 16 bytes by each
transaction[1], and on a device that doesn't need to add an I2C header when a transaction is broken,
we would need to do something like:

	struct i2c_msg *msg = kzalloc(sizeof(struct i2c_msg) * 2048, GFP_KERNEL);
	for (i = 0; i < 2048; i++) {
		msg[i].addr = i2c_addr;
		msg[i].buf = &fw_buf[i * 16];
		msg[i].len = 16;
		if (i)
			msg[i].flags = I2C_M_NOSTART;
	}
	ret = i2c_transfer(props->adap, &msg, 2048);
	kfree(msg);

[1] I used fixed values here just to simplify the code. On a real case, the static constants should
be calculated by the send function.

So, it should allocating a very big buffer just to comply with the current I2C API.

IMHO, the better would be if the I2C API would provide a "low level" way to call the lock (perhaps
it is already there, but we currently don't use), like:

	struct i2c_msg *msg;
	lock_i2c_transactions();
	msg.len = 16;
	msg.flags = 0;
	msg.addr = i2c_addr;
	for (i = 0; i < 2048; i++) {
		if (i)
			msg.flags = I2C_M_NOSTART;
		msg.buf = &fw_buf[i * 16];
		ret = i2c_transfer(props->adap, &msg, 2048);
	}
	unlock_i2c_transactions();

Mauro.

Re: CX24116 i2c patch

[Jean Delvare]

Hi Mauro, Steven,

On Thu, 05 May 2011 10:15:04 -0300, Mauro Carvalho Chehab wrote:
> Hi Steven,
> 
> Em 05-05-2011 09:28, Steven Toth escreveu:
> > Mauro,
> > 
> >> Subject: [media] cx24116: add config option to split firmware download
> >> Author:  Antti Palosaari <crope@iki.fi>
> >> Date:    Wed Apr 27 21:03:07 2011 -0300
> >>
> >> It is very rare I2C adapter hardware which can provide 32kB I2C write
> >> as one write. Add .i2c_wr_max option to set desired max packet size.
> >> Split transaction to smaller pieces according to that option.
> > 
> > This is none-sense. I'm naking this patch, please unqueue, regress or whatever.
> > 
> > The entire point of the i2c message send is that the i2c drivers know
> > nothing about the host i2c implementation, and they should not need
> > to. I2C SEND and RECEIVE are abstract and require no knowledge of the
> > hardware. This is dangerous and generates non-atomic register writes.
> > You cannot guarantee that another thread isn't reading/writing to
> > other registers in the part - breaking the driver.
> > 
> > Please fix the host controller to split the i2c messages accordingly
> > (and thus keeping the entire transaction atomic).
> > 
> > This is the second time I've seen the 'fix' to a problem by patching
> > the i2c driver. Fix the i2c bridge else we'll see this behavior
> > spreading to multiple i2c driver. It's just wrong.
> 
> As you pointed, there are two ways of solving this issue: at the I2C device
> side, and at the I2C adapter side. By looking on the existing code, you'll
> see that some drivers solve this issue at one side, others solve on the other
> side, and there are even some cases where both sides implement I2C splits.
> On very few places, this is implemented well.
> 
> As you pointed, if the I2C split is implemented inside the I2C driver, extra
> care is needed to avoid having an I2C transaction from another device in the
> middle of an I2C transaction.

Really? At least for common EEPROM chips, they keep an internal pointer
up-to-date, and direct access will always restart from where the
previous transaction stopped. It really doesn't matter if another
messages flies on the I2C bus meanwhile, as long as said message is
targeted at another chip. Serializing access to the chip can be
implemented easily in the I2C device driver itself, and this should be
sufficient on single-master topologies if all drivers properly request
each I2C address they talk to (by instantiating real or dummy i2c
clients for them.) An example of this is in drivers/misc/eeprom/at24.c.

I'd expect other I2C devices to behave in a similar way. But I can
imagine that some chips are brain-dead enough to actually be distracted
by traffic not aimed at them :(

> With the current API, this generally means that
> the I2C driver will need to use i2c_transfer() with a large block of transactions.
>
> Also, in general, we don't want to use a full I2C transaction with stop and start
> bits, so an extra flag is generally needed to indicate that should that we're using
> a "fast" i2c transaction (I2C_M_NOSTART) - more about this subject bellow.

You lost me here. I2C_M_NOSTART should only be needed to deal with I2C
chips which do not actually comply with the I2C standard and do
arbitrary direction changes (while the I2C standard doesn't allow this
without a repeated start condition.) This is a very rare case, thankfully.

A second use case which is tolerated is when your message is initially
split in multiple buffers and you don't want to waste time merging them
into a single buffer to pass it to i2c_transfer. This is merely a
performance optimization and the same could be achieved without using
I2C_M_NOSTART.

Do you really have a 3rd use case for I2C_M_NOSTART, which I didn't
know about?

> On the other hand, if the split is done at the I2C adapter, this means that the
> adapter code can't be generic anymore, as the way I2C transactions are broken
> depend on how the I2C device works. For example, on xc2028/3028, when a transaction
> is broken, the next transaction needs an I2C "header" with the register bank
> that are being updated. Other devices don't need that. Also, as not all I2C
> devices accept to work with I2C_M_NOSTART, the logic is more complicated.
> 
> 
> So, the I2C adapter xfer code will end by being something like:
> 
> switch(i2c_device) {
> 	case FOO:
> 		use_split_code_foo();
> 		break;
> 	case BAR:
> 		use_splic_code_bar();
> 		break;
> 	...
> }
> 
> 
> (if you want to see one example of the above, take a look at drivers/media/video/cx231xx/cx231xx-i2c.c).

This is horrible, things should never be implemented that way. I2C
adapter drivers should NEVER replace the transaction they were asked
for by another one. If an I2C adapter driver cannot achieve what an I2C
device driver asked for, it should simply return an error code
(-EOPNOTSUPP according to Documentation/i2c/fault-codes). As Mauro just
explained, there is no single way to "rephrase" an I2C transaction. It
all depends on the I2C device.

As a quick summary, to ensure we are all on the same track:
* Message splitting which doesn't affect what is sent on the wire
  should be transparently implemented by the I2C _adapter_ drivers.
  Typically this is required when the message is larger than a hardware
  buffer and possible only if you have enough control on the hardware
  to have it send partial messages on the wire.
* Alternative transaction formats should be implemented by the I2C
  _device_ drivers, and used whenever the underlying I2C adapter
  doesn't support the ideal transaction format.

> The end result is very bad, due to:
> 
> 1) this requires a high couple between the I2C subdriver. If the subdriver code changes,
>    all I2C adapters that use that driver also need changes;
> 2) the same logic should be replicated for all devices that use an specific I2C subdriver;
> 3) analyzing the code and tracking for troubles is more complex, as the code is splitted on
>    two parts;
> 4) the i2c xfer callback become big, confusing and hard to understand.

Amen.

> On the other hand, in order to warrant atomic operations at the I2C driver, we would need to do
> something like:
> 
> struct i2c_msg msg[5] = {
> 	.addr = props->addr, .flags = 0, .buf = buf[0], .len = len[0] },
> 	.addr = props->addr, .flags = I2C_M_NOSTART, .buf = buf[1], .len = len[1] },
> 	.addr = props->addr, .flags = I2C_M_NOSTART, .buf = buf[2], .len = len[2] },
> 	.addr = props->addr, .flags = I2C_M_NOSTART, .buf = buf[3], .len = len[3] },
> }
> ret = i2c_transfer(props->adap, &msg, 5);

As said before, I fail to see how the above is different from a single
message with all buffers concatenated. It should really be the same bits
pushed on the wire. If I a missing something - and I might be, really -
please explain.

> While this warrants that the I2C adapter won't have any transaction from the other devices, in the
> cases like firmware transfers, the above API is not optimal.
> 
> For example, assuming a 32768 firmware, on an I2C adapter capable of sending up to 16 bytes by each
> transaction[1], and on a device that doesn't need to add an I2C header when a transaction is broken,
> we would need to do something like:
> 
> 	struct i2c_msg *msg = kzalloc(sizeof(struct i2c_msg) * 2048, GFP_KERNEL);
> 	for (i = 0; i < 2048; i++) {
> 		msg[i].addr = i2c_addr;
> 		msg[i].buf = &fw_buf[i * 16];
> 		msg[i].len = 16;
> 		if (i)
> 			msg[i].flags = I2C_M_NOSTART;
> 	}
> 	ret = i2c_transfer(props->adap, &msg, 2048);
> 	kfree(msg);
> 
> [1] I used fixed values here just to simplify the code. On a real case, the static constants should
> be calculated by the send function.
> 
> So, it should allocating a very big buffer just to comply with the current I2C API.

If the underlying adapter driver supports I2C_M_NOSTART, then I fail to
see what prevents said driver from transparently splitting the message
to accommodate its hardware buffer limitations.

The case which is harder to solve is if the underlying adapter driver
doesn't support I2C_M_NOSTART, and has a limitation on the size of the
messages it can transmit, and I2C device drivers would eventually like
to send messages larger than this. The right way to handle this case,
with the existing API is as follows:
* The I2C device driver attempts to send or receive the largest message
  it would like.
* The I2C adapter driver replies with -EOPNOTSUPP if the message is too
  large.
* The I2C device driver retries with different code needing smaller
  messages.
* The I2C adapter driver may return -EOPNOTSUPP again if it's still too
  large, or obey.
* etc.

If this is mainly needed for firmware upload, which is an infrequent
operation, that should work just fine. If you need to do this
repeatedly then the performance drop (caused by repeated round trips
between device driver and adapter driver) may be problematic. In that
case, you could record which message size finally worked, and start
from there next time.

> IMHO, the better would be if the I2C API would provide a "low level" way to call the lock (perhaps
> it is already there, but we currently don't use), like:
> 
> 	struct i2c_msg *msg;
> 	lock_i2c_transactions();
> 	msg.len = 16;
> 	msg.flags = 0;
> 	msg.addr = i2c_addr;
> 	for (i = 0; i < 2048; i++) {
> 		if (i)
> 			msg.flags = I2C_M_NOSTART;
> 		msg.buf = &fw_buf[i * 16];
> 		ret = i2c_transfer(props->adap, &msg, 2048);
> 	}
> 	unlock_i2c_transactions();

i2c_lock_adapter(), i2c_trylock_adapter() and i2c_unlock_adapter() are
available. However, when you call i2c_lock_adapter(), you can't call
i2c_transfer() as it would deadlock by design. You would have to call
the i2c_adapter's transfer function directly instead, as in:

	ret = i2c_adapter->algo->master_xfer(i2c_adapter, msgs, num);

Note that you lose the automatic retry mechanism though. That being
said, I don't think this is the right approach in general, as explained
above.

Hope I helped a bit, if you have more questions, feel free!

-- 
Jean Delvare

Re: CX24116 i2c patch

[Mauro Carvalho Chehab]

Em 05-05-2011 12:09, Jean Delvare escreveu:
> Hi Mauro, Steven,
> 
> On Thu, 05 May 2011 10:15:04 -0300, Mauro Carvalho Chehab wrote:
>> Hi Steven,
>>
>> Em 05-05-2011 09:28, Steven Toth escreveu:
>>> Mauro,
>>>
>>>> Subject: [media] cx24116: add config option to split firmware download
>>>> Author:  Antti Palosaari <crope@iki.fi>
>>>> Date:    Wed Apr 27 21:03:07 2011 -0300
>>>>
>>>> It is very rare I2C adapter hardware which can provide 32kB I2C write
>>>> as one write. Add .i2c_wr_max option to set desired max packet size.
>>>> Split transaction to smaller pieces according to that option.
>>>
>>> This is none-sense. I'm naking this patch, please unqueue, regress or whatever.
>>>
>>> The entire point of the i2c message send is that the i2c drivers know
>>> nothing about the host i2c implementation, and they should not need
>>> to. I2C SEND and RECEIVE are abstract and require no knowledge of the
>>> hardware. This is dangerous and generates non-atomic register writes.
>>> You cannot guarantee that another thread isn't reading/writing to
>>> other registers in the part - breaking the driver.
>>>
>>> Please fix the host controller to split the i2c messages accordingly
>>> (and thus keeping the entire transaction atomic).
>>>
>>> This is the second time I've seen the 'fix' to a problem by patching
>>> the i2c driver. Fix the i2c bridge else we'll see this behavior
>>> spreading to multiple i2c driver. It's just wrong.
>>
>> As you pointed, there are two ways of solving this issue: at the I2C device
>> side, and at the I2C adapter side. By looking on the existing code, you'll
>> see that some drivers solve this issue at one side, others solve on the other
>> side, and there are even some cases where both sides implement I2C splits.
>> On very few places, this is implemented well.
>>
>> As you pointed, if the I2C split is implemented inside the I2C driver, extra
>> care is needed to avoid having an I2C transaction from another device in the
>> middle of an I2C transaction.
> 
> Really? At least for common EEPROM chips, they keep an internal pointer
> up-to-date, and direct access will always restart from where the
> previous transaction stopped. It really doesn't matter if another
> messages flies on the I2C bus meanwhile, as long as said message is
> targeted at another chip. Serializing access to the chip can be
> implemented easily in the I2C device driver itself, and this should be
> sufficient on single-master topologies if all drivers properly request
> each I2C address they talk to (by instantiating real or dummy i2c
> clients for them.) An example of this is in drivers/misc/eeprom/at24.c.
> 
> I'd expect other I2C devices to behave in a similar way. But I can
> imagine that some chips are brain-dead enough to actually be distracted
> by traffic not aimed at them :(

Yes, that happens. For example, NXP tda18271 states that certain operations,
like the initialization of a sequence of 16 registers should be done into an
atomic operation, otherwise the net result is not reliable [1].  However, (some of) the
I2C bridges found at cx231xx don't support any write with more than 4 data bytes of
data. So, the solution is to break it into 4 consecutive I2C packets, properly serialized.

The serialization is also needed because of the I2C switches that the device may have.

[1] those registers initialize a series of calibration data that, among other things,
estimate some parameters based on the current device temperature.

>> With the current API, this generally means that
>> the I2C driver will need to use i2c_transfer() with a large block of transactions.
>>
>> Also, in general, we don't want to use a full I2C transaction with stop and start
>> bits, so an extra flag is generally needed to indicate that should that we're using
>> a "fast" i2c transaction (I2C_M_NOSTART) - more about this subject bellow.
> 
> You lost me here. I2C_M_NOSTART should only be needed to deal with I2C
> chips which do not actually comply with the I2C standard and do
> arbitrary direction changes (while the I2C standard doesn't allow this
> without a repeated start condition.) This is a very rare case, thankfully.
> 
> A second use case which is tolerated is when your message is initially
> split in multiple buffers and you don't want to waste time merging them
> into a single buffer to pass it to i2c_transfer. This is merely a
> performance optimization and the same could be achieved without using
> I2C_M_NOSTART.
> 
> Do you really have a 3rd use case for I2C_M_NOSTART, which I didn't
> know about?

Perhaps with a wrong meaning, but some drivers use it to use the repeated-start mode.

So, instead of sending:
	start + addr + data + stop + start + addr + data + stop
they send:
	start + addr + data + stop + data + stop

(see for example saa7134-i2c and dib0700-core).

>> On the other hand, if the split is done at the I2C adapter, this means that the
>> adapter code can't be generic anymore, as the way I2C transactions are broken
>> depend on how the I2C device works. For example, on xc2028/3028, when a transaction
>> is broken, the next transaction needs an I2C "header" with the register bank
>> that are being updated. Other devices don't need that. Also, as not all I2C
>> devices accept to work with I2C_M_NOSTART, the logic is more complicated.
>>
>>
>> So, the I2C adapter xfer code will end by being something like:
>>
>> switch(i2c_device) {
>> 	case FOO:
>> 		use_split_code_foo();
>> 		break;
>> 	case BAR:
>> 		use_splic_code_bar();
>> 		break;
>> 	...
>> }
>>
>>
>> (if you want to see one example of the above, take a look at drivers/media/video/cx231xx/cx231xx-i2c.c).
> 
> This is horrible, things should never be implemented that way. I2C
> adapter drivers should NEVER replace the transaction they were asked
> for by another one. If an I2C adapter driver cannot achieve what an I2C
> device driver asked for, it should simply return an error code
> (-EOPNOTSUPP according to Documentation/i2c/fault-codes). As Mauro just
> explained, there is no single way to "rephrase" an I2C transaction. It
> all depends on the I2C device.
> 
> As a quick summary, to ensure we are all on the same track:
> * Message splitting which doesn't affect what is sent on the wire
>   should be transparently implemented by the I2C _adapter_ drivers.
>   Typically this is required when the message is larger than a hardware
>   buffer and possible only if you have enough control on the hardware
>   to have it send partial messages on the wire.
> * Alternative transaction formats should be implemented by the I2C
>   _device_ drivers, and used whenever the underlying I2C adapter
>   doesn't support the ideal transaction format.
> 
>> The end result is very bad, due to:
>>
>> 1) this requires a high couple between the I2C subdriver. If the subdriver code changes,
>>    all I2C adapters that use that driver also need changes;
>> 2) the same logic should be replicated for all devices that use an specific I2C subdriver;
>> 3) analyzing the code and tracking for troubles is more complex, as the code is splitted on
>>    two parts;
>> 4) the i2c xfer callback become big, confusing and hard to understand.
> 
> Amen.
> 
>> On the other hand, in order to warrant atomic operations at the I2C driver, we would need to do
>> something like:
>>
>> struct i2c_msg msg[5] = {
>> 	.addr = props->addr, .flags = 0, .buf = buf[0], .len = len[0] },
>> 	.addr = props->addr, .flags = I2C_M_NOSTART, .buf = buf[1], .len = len[1] },
>> 	.addr = props->addr, .flags = I2C_M_NOSTART, .buf = buf[2], .len = len[2] },
>> 	.addr = props->addr, .flags = I2C_M_NOSTART, .buf = buf[3], .len = len[3] },
>> }
>> ret = i2c_transfer(props->adap, &msg, 5);
> 
> As said before, I fail to see how the above is different from a single
> message with all buffers concatenated. It should really be the same bits
> pushed on the wire. If I a missing something - and I might be, really -
> please explain.

What happens here is that the adapter bridge xfer callback will receive 5 messages instead of one,
and will handle each one independent. However, due to I2C mutex and due to its internal logic,
the message will be serialized.

So, for each message, the hardware will send I2C start, I2C address, I2C data and I2C stop (or the
repeated-start mode, if the adapter supports I2C_M_NOSTART).

>> While this warrants that the I2C adapter won't have any transaction from the other devices, in the
>> cases like firmware transfers, the above API is not optimal.
>>
>> For example, assuming a 32768 firmware, on an I2C adapter capable of sending up to 16 bytes by each
>> transaction[1], and on a device that doesn't need to add an I2C header when a transaction is broken,
>> we would need to do something like:
>>
>> 	struct i2c_msg *msg = kzalloc(sizeof(struct i2c_msg) * 2048, GFP_KERNEL);
>> 	for (i = 0; i < 2048; i++) {
>> 		msg[i].addr = i2c_addr;
>> 		msg[i].buf = &fw_buf[i * 16];
>> 		msg[i].len = 16;
>> 		if (i)
>> 			msg[i].flags = I2C_M_NOSTART;
>> 	}
>> 	ret = i2c_transfer(props->adap, &msg, 2048);
>> 	kfree(msg);
>>
>> [1] I used fixed values here just to simplify the code. On a real case, the static constants should
>> be calculated by the send function.
>>
>> So, it should allocating a very big buffer just to comply with the current I2C API.
> 
> If the underlying adapter driver supports I2C_M_NOSTART, then I fail to
> see what prevents said driver from transparently splitting the message
> to accommodate its hardware buffer limitations.

The adapter code sends each transaction in separate. It doesn't try to merge
them. So, it will program the hardware to send the first buffer, then the second
and so on. 

> The case which is harder to solve is if the underlying adapter driver
> doesn't support I2C_M_NOSTART, and has a limitation on the size of the
> messages it can transmit, and I2C device drivers would eventually like
> to send messages larger than this. The right way to handle this case,
> with the existing API is as follows:
> * The I2C device driver attempts to send or receive the largest message
>   it would like.
> * The I2C adapter driver replies with -EOPNOTSUPP if the message is too
>   large.
> * The I2C device driver retries with different code needing smaller
>   messages.
> * The I2C adapter driver may return -EOPNOTSUPP again if it's still too
>   large, or obey.
> * etc.

Hmm... this works, but we still need to serialize transactions, due to the
reasons explained before. Yet, playing ping-pong like that doesn't seem to
be very efficient.

> If this is mainly needed for firmware upload, which is an infrequent
> operation, that should work just fine. If you need to do this
> repeatedly then the performance drop (caused by repeated round trips
> between device driver and adapter driver) may be problematic. In that
> case, you could record which message size finally worked, and start
> from there next time.

It depends on the hardware. Firmware upload is the worse case, but there
are other cases where larger messages are desired. It should be noticed
that some devices like xc3028 require firmware loads every time you start
watching TV, or a standard is changed, as it have a very small memory for
firmware inside the chip. So, there are about 80 different firmwares, one
for each different TV (or radio) standard.

>> IMHO, the better would be if the I2C API would provide a "low level" way to call the lock (perhaps
>> it is already there, but we currently don't use), like:
>>
>> 	struct i2c_msg *msg;
>> 	lock_i2c_transactions();
>> 	msg.len = 16;
>> 	msg.flags = 0;
>> 	msg.addr = i2c_addr;
>> 	for (i = 0; i < 2048; i++) {
>> 		if (i)
>> 			msg.flags = I2C_M_NOSTART;
>> 		msg.buf = &fw_buf[i * 16];
>> 		ret = i2c_transfer(props->adap, &msg, 2048);
>> 	}
>> 	unlock_i2c_transactions();
> 
> i2c_lock_adapter(), i2c_trylock_adapter() and i2c_unlock_adapter() are
> available. However, when you call i2c_lock_adapter(), you can't call
> i2c_transfer() as it would deadlock by design. You would have to call
> the i2c_adapter's transfer function directly instead, as in:
> 
> 	ret = i2c_adapter->algo->master_xfer(i2c_adapter, msgs, num);
> 
> Note that you lose the automatic retry mechanism though. That being
> said, I don't think this is the right approach in general, as explained
> above.

What about adding a:
	__i2c_transfer()

that would do pretty much the same as i2c_transfer(), but without locking the adapter?

> 
> Hope I helped a bit, if you have more questions, feel free!

Yes, it helped, thanks!

Mauro.

Re: CX24116 i2c patch

[Jean Delvare]

Hi Mauro,

Sorry for the late reply.

On Thu, 05 May 2011 13:18:04 -0300, Mauro Carvalho Chehab wrote:
> Em 05-05-2011 12:09, Jean Delvare escreveu:
> > Hi Mauro, Steven,
> > 
> > On Thu, 05 May 2011 10:15:04 -0300, Mauro Carvalho Chehab wrote:
> >> As you pointed, there are two ways of solving this issue: at the I2C device
> >> side, and at the I2C adapter side. By looking on the existing code, you'll
> >> see that some drivers solve this issue at one side, others solve on the other
> >> side, and there are even some cases where both sides implement I2C splits.
> >> On very few places, this is implemented well.
> >>
> >> As you pointed, if the I2C split is implemented inside the I2C driver, extra
> >> care is needed to avoid having an I2C transaction from another device in the
> >> middle of an I2C transaction.
> > 
> > Really? At least for common EEPROM chips, they keep an internal pointer
> > up-to-date, and direct access will always restart from where the
> > previous transaction stopped. It really doesn't matter if another
> > messages flies on the I2C bus meanwhile, as long as said message is
> > targeted at another chip. Serializing access to the chip can be
> > implemented easily in the I2C device driver itself, and this should be
> > sufficient on single-master topologies if all drivers properly request
> > each I2C address they talk to (by instantiating real or dummy i2c
> > clients for them.) An example of this is in drivers/misc/eeprom/at24.c.
> > 
> > I'd expect other I2C devices to behave in a similar way. But I can
> > imagine that some chips are brain-dead enough to actually be distracted
> > by traffic not aimed at them :(
> 
> Yes, that happens. For example, NXP tda18271 states that certain operations,
> like the initialization of a sequence of 16 registers should be done into an
> atomic operation, otherwise the net result is not reliable [1].  However, (some of) the
> I2C bridges found at cx231xx don't support any write with more than 4 data bytes of
> data. So, the solution is to break it into 4 consecutive I2C packets, properly serialized.

How is this limitation implemented? I'm looking at
drivers/media/video/cx231xx/cx231xx-i2c.c:cx231xx_i2c_xfer() and I
don't see any size check for writes.

As a side note (may or may not be relevant in this specific case), when
a controller is limited in what it can do, it may be better to pretend
it's an SMBus controller rather than an I2C controller (i.e. implement
algo.smbus_xfer rather algo.master_xfer). Of course this only works if
the transaction types you need are a subset of SMBus.

> The serialization is also needed because of the I2C switches that the device may have.

The problem should be independent from the bus topology: bus switching
is transparent to device drivers.

> [1] those registers initialize a series of calibration data that, among other things,
> estimate some parameters based on the current device temperature.
> 
> >> With the current API, this generally means that
> >> the I2C driver will need to use i2c_transfer() with a large block of transactions.
> >>
> >> Also, in general, we don't want to use a full I2C transaction with stop and start
> >> bits, so an extra flag is generally needed to indicate that should that we're using
> >> a "fast" i2c transaction (I2C_M_NOSTART) - more about this subject bellow.
> > 
> > You lost me here. I2C_M_NOSTART should only be needed to deal with I2C
> > chips which do not actually comply with the I2C standard and do
> > arbitrary direction changes (while the I2C standard doesn't allow this
> > without a repeated start condition.) This is a very rare case, thankfully.
> > 
> > A second use case which is tolerated is when your message is initially
> > split in multiple buffers and you don't want to waste time merging them
> > into a single buffer to pass it to i2c_transfer. This is merely a
> > performance optimization and the same could be achieved without using
> > I2C_M_NOSTART.
> > 
> > Do you really have a 3rd use case for I2C_M_NOSTART, which I didn't
> > know about?
> 
> Perhaps with a wrong meaning, but some drivers use it to use the repeated-start mode.
> 
> So, instead of sending:
> 	start + addr + data + stop + start + addr + data + stop
> they send:
> 	start + addr + data + stop + data + stop

The above is not a valid I2C transaction, and I2C_M_NOSTART can't be used
to construct such a transaction. "stop" really means stop and nothing
can go on the wire after it.

For the example above, the message that goes on the wire without
I2C_M_NOSTART is:
	start + addr + data + (repeated) start + addr + data + stop
While with I2C_M_NOSTART you get:
	start + addr + data + data + stop

In other words. I2C_M_NOSTART strips the repeated start and repeated
address byte between messages within a given transaction.

It is important to make a difference between the two use cases I
mentioned earlier (quoted in full above for convenience.) Your notation
doesn't mention the directions, while this is what is most important.

For the first use case, there is a direction change in the middle of
the message. We'll assume a typical "master writes subaddress and reads
back a value":
	start + addr + data from master to slave + data from slave to master + stop
This is _not_ a valid I2C transaction, as arbitrary direction changes
aren't allowed by I2C. However it is compatible enough that, if an I2C
device expects this and the I2C master supports it, the other I2C
devices on the bus shouldn't have a problem with it.

For the second use case, there is no direction change, i.e. we have for
example:
	start + addr + data from master to slave + data from master to slave + stop
While this can be implemented with two separate messages and
I2C_M_NOSTART, this can _also_ be implemented as a single message with
all the data in it. Seen from the wire, the result is exactly the same.
This use of I2C_M_NOSTART could be seen as an abuse, as it's not
needed, but it is tolerated because it can be convenient at times for
performance reasons.

> (see for example saa7134-i2c and dib0700-core).

These implementations look good, but the interesting part is the I2C
device driver part. This will show the use case for I2C_M_NOSTART. But
a quick grep in drivers/media did not reveal _any_ driver using this
flag. What am I missing?

> >> (...)
> >> On the other hand, in order to warrant atomic operations at the I2C driver, we would need to do
> >> something like:
> >>
> >> struct i2c_msg msg[5] = {
> >> 	.addr = props->addr, .flags = 0, .buf = buf[0], .len = len[0] },
> >> 	.addr = props->addr, .flags = I2C_M_NOSTART, .buf = buf[1], .len = len[1] },
> >> 	.addr = props->addr, .flags = I2C_M_NOSTART, .buf = buf[2], .len = len[2] },
> >> 	.addr = props->addr, .flags = I2C_M_NOSTART, .buf = buf[3], .len = len[3] },
> >> }
> >> ret = i2c_transfer(props->adap, &msg, 5);
> > 
> > As said before, I fail to see how the above is different from a single
> > message with all buffers concatenated. It should really be the same bits
> > pushed on the wire. If I a missing something - and I might be, really -
> > please explain.
> 
> What happens here is that the adapter bridge xfer callback will receive 5 messages instead of one,
> and will handle each one independent. However, due to I2C mutex and due to its internal logic,
> the message will be serialized.

This doesn't answer my question: why aren't you using a single message?
What's the benefit of the split?

> So, for each message, the hardware will send I2C start, I2C address, I2C data and I2C stop (or the
> repeated-start mode, if the adapter supports I2C_M_NOSTART).

Actually, I2C_M_NOSTART means: NO repeated-start.

> 
> >> While this warrants that the I2C adapter won't have any transaction from the other devices, in the
> >> cases like firmware transfers, the above API is not optimal.
> >>
> >> For example, assuming a 32768 firmware, on an I2C adapter capable of sending up to 16 bytes by each
> >> transaction[1], and on a device that doesn't need to add an I2C header when a transaction is broken,
> >> we would need to do something like:
> >>
> >> 	struct i2c_msg *msg = kzalloc(sizeof(struct i2c_msg) * 2048, GFP_KERNEL);
> >> 	for (i = 0; i < 2048; i++) {
> >> 		msg[i].addr = i2c_addr;
> >> 		msg[i].buf = &fw_buf[i * 16];
> >> 		msg[i].len = 16;
> >> 		if (i)
> >> 			msg[i].flags = I2C_M_NOSTART;
> >> 	}
> >> 	ret = i2c_transfer(props->adap, &msg, 2048);
> >> 	kfree(msg);
> >>
> >> [1] I used fixed values here just to simplify the code. On a real case, the static constants should
> >> be calculated by the send function.
> >>
> >> So, it should allocating a very big buffer just to comply with the current I2C API.
> > 
> > If the underlying adapter driver supports I2C_M_NOSTART, then I fail to
> > see what prevents said driver from transparently splitting the message
> > to accommodate its hardware buffer limitations.
> 
> The adapter code sends each transaction in separate. It doesn't try to merge
> them. So, it will program the hardware to send the first buffer, then the second
> and so on.

And your point is...?

> > The case which is harder to solve is if the underlying adapter driver
> > doesn't support I2C_M_NOSTART, and has a limitation on the size of the
> > messages it can transmit, and I2C device drivers would eventually like
> > to send messages larger than this. The right way to handle this case,
> > with the existing API is as follows:
> > * The I2C device driver attempts to send or receive the largest message
> >   it would like.
> > * The I2C adapter driver replies with -EOPNOTSUPP if the message is too
> >   large.
> > * The I2C device driver retries with different code needing smaller
> >   messages.
> > * The I2C adapter driver may return -EOPNOTSUPP again if it's still too
> >   large, or obey.
> > * etc.
> 
> Hmm... this works, but we still need to serialize transactions, due to the
> reasons explained before. Yet, playing ping-pong like that doesn't seem to
> be very efficient.

Serialization is not an issue that I can see. If you have to sent
multiple smaller messages, you'll still enclose them in a single
message array, with a single call to i2c_transfer, so serialization
will be guaranteed by i2c-core.

The performance issue I have discussed below. If this isn't good enough
we could add a callback to let I2C adapter drivers report their buffer
size. But I don't see an immediate need for it.

> > If this is mainly needed for firmware upload, which is an infrequent
> > operation, that should work just fine. If you need to do this
> > repeatedly then the performance drop (caused by repeated round trips
> > between device driver and adapter driver) may be problematic. In that
> > case, you could record which message size finally worked, and start
> > from there next time.
> 
> It depends on the hardware. Firmware upload is the worse case, but there
> are other cases where larger messages are desired. It should be noticed
> that some devices like xc3028 require firmware loads every time you start
> watching TV, or a standard is changed, as it have a very small memory for
> firmware inside the chip. So, there are about 80 different firmwares, one
> for each different TV (or radio) standard.

Wow :/

> >> IMHO, the better would be if the I2C API would provide a "low level" way to call the lock (perhaps
> >> it is already there, but we currently don't use), like:
> >>
> >> 	struct i2c_msg *msg;
> >> 	lock_i2c_transactions();
> >> 	msg.len = 16;
> >> 	msg.flags = 0;
> >> 	msg.addr = i2c_addr;
> >> 	for (i = 0; i < 2048; i++) {
> >> 		if (i)
> >> 			msg.flags = I2C_M_NOSTART;
> >> 		msg.buf = &fw_buf[i * 16];
> >> 		ret = i2c_transfer(props->adap, &msg, 2048);
> >> 	}
> >> 	unlock_i2c_transactions();
> > 
> > i2c_lock_adapter(), i2c_trylock_adapter() and i2c_unlock_adapter() are
> > available. However, when you call i2c_lock_adapter(), you can't call
> > i2c_transfer() as it would deadlock by design. You would have to call
> > the i2c_adapter's transfer function directly instead, as in:
> > 
> > 	ret = i2c_adapter->algo->master_xfer(i2c_adapter, msgs, num);
> > 
> > Note that you lose the automatic retry mechanism though. That being
> > said, I don't think this is the right approach in general, as explained
> > above.
> 
> What about adding a:
> 	__i2c_transfer()
> 
> that would do pretty much the same as i2c_transfer(), but without locking the adapter?

I am always reluctant when it comes to exporting internal functions, as
there is a risk of improper use. I would be OK to export
__i2c_transfer() though, if you have an actual need for it. But so far
I don't see any scenario which the current options won't let you solve.

If you have a concrete case at hand where you think you can't do (or
can't do efficiently) without __i2c_transfer(), please show it to me
and I'll take a look.

-- 
Jean Delvare

Re: CX24116 i2c patch

[Devin Heitmueller]

On Thu, May 5, 2011 at 9:15 AM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> So, the I2C adapter xfer code will end by being something like:
>
> switch(i2c_device) {
>        case FOO:
>                use_split_code_foo();
>                break;
>        case BAR:
>                use_splic_code_bar();
>                break;
>        ...
> }
>
>
> (if you want to see one example of the above, take a look at drivers/media/video/cx231xx/cx231xx-i2c.c).

The cx231xx is actually an example of a poor implementation rather
than a deficiency in the chip.  The device does support sending
arbitrarily long sequences, but because of a lack of support for i2c
clock stretching they hacked in their own GPIO based bitbang
implementation which only gets used in certain cases.  If somebody
wanted to clean it up I believe it could be done much more cleanly.
That said, it hasn't happened because the code as-is "works" and in
reality I don't think there are any shipping products which use
cx231xx and xc5000 (they are all Conexant reference designs).

If somebody really wants to clean this up, they should have a board
profile field which indicates whether to create an i2c adapter which
uses the onboard i2c controller, or alternatively to setup an i2c
adapter which uses the real Linux i2c-bitbang implementation.  That
would make the implementation much easier to understand as well as
eliminating all the crap code which makes decisions based on the
destination i2c address.

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: CX24116 i2c patch

[Mauro Carvalho Chehab]

Em 05-05-2011 12:34, Devin Heitmueller escreveu:
> On Thu, May 5, 2011 at 9:15 AM, Mauro Carvalho Chehab
> <mchehab@redhat.com> wrote:
>> So, the I2C adapter xfer code will end by being something like:
>>
>> switch(i2c_device) {
>>        case FOO:
>>                use_split_code_foo();
>>                break;
>>        case BAR:
>>                use_splic_code_bar();
>>                break;
>>        ...
>> }
>>
>>
>> (if you want to see one example of the above, take a look at drivers/media/video/cx231xx/cx231xx-i2c.c).
> 
> The cx231xx is actually an example of a poor implementation

Yes.

> rather
> than a deficiency in the chip.  The device does support sending
> arbitrarily long sequences, but because of a lack of support for i2c
> clock stretching they hacked in their own GPIO based bitbang
> implementation which only gets used in certain cases. 

No. There are two different situations here: they use GPIO bitbang for
one device type, due to clock stretching, but also the hardware doesn't accept
long I2C messages. I've double-checked this with the vendor developers, and
double-checked the sniffed messages from the original driver.

The case I detected troubles were with tda18271 device (that doesn't use
clock stretching), and using the direct hardware support for I2C, at the
I2C bus 2. The Hauppauge devices that you've worked have the tuner connected
to the I2C bus 1. Perhaps bus 2 has a smaller hardware buffer, or perhaps
the chip revision present on the device I tested is more limited.

In any case, every time more than 4 data bytes were sent to tda18271, the
hardware returned an error.

If you take a look at cx231xx-core, you'll see that other types of non-GPIO
messages with more than 4 bytes are broken into smaller messages there.

> If somebody
> wanted to clean it up I believe it could be done much more cleanly.
> That said, it hasn't happened because the code as-is "works" and in
> reality I don't think there are any shipping products which use
> cx231xx and xc5000 (they are all Conexant reference designs).
> 
> If somebody really wants to clean this up, they should have a board
> profile field which indicates whether to create an i2c adapter which
> uses the onboard i2c controller, or alternatively to setup an i2c
> adapter which uses the real Linux i2c-bitbang implementation.  That
> would make the implementation much easier to understand as well as
> eliminating all the crap code which makes decisions based on the
> destination i2c address.

Yes, the code can be cleaned, but this won't solve the issue: messages still
need to be broken to be used by (some) I2C buses on it. 

Also, cx231xx is not an exception: there are very few hardware that would 
accept a 32 KB message to be sent directly via I2C. The hardware were this
is done via bit-bang will support. Also a few other devices like saa7134,
where data is sent byte by byte and kernel waits for the I2C device to indicate
that one byte were sent, and it can then forward the next byte. Yet, on
both cases, I don't think it is a good idea to send a 32 KB data into just
one I2C transaction.

However, on the devices where you pass a buffer to the hardware/firmware
(like all USB devices), the size of the I2C message is limited. The upper
limit is 80 bytes for an I2C control message, but several devices have
a lower limit for it.

Mauro.

Re: CX24116 i2c patch

[Devin Heitmueller]

On Thu, May 5, 2011 at 8:28 AM, Steven Toth <stoth@kernellabs.com> wrote:
> Mauro,
>
>> Subject: [media] cx24116: add config option to split firmware download
>> Author:  Antti Palosaari <crope@iki.fi>
>> Date:    Wed Apr 27 21:03:07 2011 -0300
>>
>> It is very rare I2C adapter hardware which can provide 32kB I2C write
>> as one write. Add .i2c_wr_max option to set desired max packet size.
>> Split transaction to smaller pieces according to that option.
>
> This is none-sense. I'm naking this patch, please unqueue, regress or whatever.
>
> The entire point of the i2c message send is that the i2c drivers know
> nothing about the host i2c implementation, and they should not need
> to. I2C SEND and RECEIVE are abstract and require no knowledge of the
> hardware. This is dangerous and generates non-atomic register writes.
> You cannot guarantee that another thread isn't reading/writing to
> other registers in the part - breaking the driver.
>
> Please fix the host controller to split the i2c messages accordingly
> (and thus keeping the entire transaction atomic).
>
> This is the second time I've seen the 'fix' to a problem by patching
> the i2c driver. Fix the i2c bridge else we'll see this behavior
> spreading to multiple i2c driver. It's just wrong.

I tend to agree with Steven on this one.  That said, these sorts of
things usually get introduced in cases where the i2c master is not
well enough understood to know how to split the transactions and still
preserve the repeat start (common with reverse engineered drivers).
It can also occur in cases where there really is a hardware limitation
that prevents the caller from making multiple requests to the chip
while not sending the stop bit (although this is less common).

Do we know this to be the case with the anysee bridge?  Is this a
reverse engineered device?  Is there documentation/datasheets to
reference?

Do we know if this is an issue with the i2c master driver not being
fully baked, or if it's a hardware limitation?

Devin

-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com

Re: CX24116 i2c patch

[Mauro Carvalho Chehab]

Em 05-05-2011 12:25, Devin Heitmueller escreveu:
> On Thu, May 5, 2011 at 8:28 AM, Steven Toth <stoth@kernellabs.com> wrote:
>> Mauro,
>>
>>> Subject: [media] cx24116: add config option to split firmware download
>>> Author:  Antti Palosaari <crope@iki.fi>
>>> Date:    Wed Apr 27 21:03:07 2011 -0300
>>>
>>> It is very rare I2C adapter hardware which can provide 32kB I2C write
>>> as one write. Add .i2c_wr_max option to set desired max packet size.
>>> Split transaction to smaller pieces according to that option.
>>
>> This is none-sense. I'm naking this patch, please unqueue, regress or whatever.
>>
>> The entire point of the i2c message send is that the i2c drivers know
>> nothing about the host i2c implementation, and they should not need
>> to. I2C SEND and RECEIVE are abstract and require no knowledge of the
>> hardware. This is dangerous and generates non-atomic register writes.
>> You cannot guarantee that another thread isn't reading/writing to
>> other registers in the part - breaking the driver.
>>
>> Please fix the host controller to split the i2c messages accordingly
>> (and thus keeping the entire transaction atomic).
>>
>> This is the second time I've seen the 'fix' to a problem by patching
>> the i2c driver. Fix the i2c bridge else we'll see this behavior
>> spreading to multiple i2c driver. It's just wrong.
> 
> I tend to agree with Steven on this one.  That said, these sorts of
> things usually get introduced in cases where the i2c master is not
> well enough understood to know how to split the transactions and still
> preserve the repeat start (common with reverse engineered drivers).
> It can also occur in cases where there really is a hardware limitation
> that prevents the caller from making multiple requests to the chip
> while not sending the stop bit (although this is less common).
> 
> Do we know this to be the case with the anysee bridge?  Is this a
> reverse engineered device?  Is there documentation/datasheets to
> reference?

> 
> Do we know if this is an issue with the i2c master driver not being
> fully baked, or if it's a hardware limitation?

I can't tell you how Antti is working, but, since this is a USB device,
and cx24116 is trying to send a 32KB message via one single I2C transfer,
I can tell you for sure that that this won't work.

USB control messages can have, at maximum, 80 bytes of data on it. So,
the message needs to be broken into 80-byte payloads (assuming that
Anysee accepts the maximum size).

Mauro.

Re: CX24116 i2c patch

[Antti Palosaari]

to 5.5.2011 19:55 Mauro Carvalho Chehab kirjoitti:
> Em 05-05-2011 12:25, Devin Heitmueller escreveu:
>> Do we know this to be the case with the anysee bridge?  Is this a
>> reverse engineered device?  Is there documentation/datasheets to
>> reference?
>
>>
>> Do we know if this is an issue with the i2c master driver not being
>> fully baked, or if it's a hardware limitation?
>
> I can't tell you how Antti is working, but, since this is a USB device,
> and cx24116 is trying to send a 32KB message via one single I2C transfer,
> I can tell you for sure that that this won't work.
>
> USB control messages can have, at maximum, 80 bytes of data on it. So,
> the message needs to be broken into 80-byte payloads (assuming that
> Anysee accepts the maximum size).

Anysee have Cypress 'FX2' -bridge running their own custom firmware. It
uses BULK messages for data transfer - so there is no such small limit as
control messages have.

But the API FW implements limits that size, it is just max I configured to
DVB-S/S2 driver in question. Thats since there is static meaning bytes
before and after I2C data. Something like (as example near real, I cannot
check now easily since I am on weekend trip):

~bytes:
1-10 len, I2C addr, command, etc...
11-58 I2C data
59 packet sequence number
60 some other value
61 some other value

Whole message size is around 60 bytes.

Anyhow, the point is that used message size is static and there is static
bytes at the end of each message which does have some meaning.

Antti