Question about patch "i2c: omap: resize fifos before each message"

Question about patch "i2c: omap: resize fifos before each message"

[Alexander Kochetkov]

Felipe,

Question about the patch[1].

I want to change the code in a way to not touch fifo thresholds for each message.
Because:
1. dev->threshold is valid only with checking of transfer direction.
   So, if last transfer was transmission and ISR get RRDY interrupt from slave receiver,
   then dev->threshold is invalid. We must read threshold value from BUF register, to
   process correctly.
2. I want to avoid changing fifos before message submission, because IP can start receiving
   message in a slave mode (race).
3. dev->threshold is changed in range 1-fifo_size/2. So instead of RDR we get RRDY and
   for messages larger then fifo_size/2 we still get RRDY and RDR.

Felipe, do you have in mind why do you want to avoid RDR and XDR events?
Something about errata?

[1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/i2c/busses/i2c-omap.c?id=dd74548ddece4b9d68e5528287a272fa552c81d0

Re: Question about patch "i2c: omap: resize fifos before each message"

[Felipe Balbi]

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

Hi,

On Wed, Dec 03, 2014 at 06:11:18PM +0300, Alexander Kochetkov wrote:
> Felipe,
> 
> Question about the patch[1].
> 
> I want to change the code in a way to not touch fifo thresholds for
> each message.  Because:
>
> 1. dev->threshold is valid only with checking of transfer direction.
> So, if last transfer was transmission and ISR get RRDY interrupt from
> slave receiver, then dev->threshold is invalid. We must read threshold
> value from BUF register, to process correctly.

What I noticed, however, is that threshold value from BUF wasn't very
reliable. My memory is now really fuzzy, but when I talked to the person
who maintained this IP RTL inside TI, there were some "interesting"
requirements wrt when BUF's threshold was valid and I had a hard time
ensuring that access time.

> 2. I want to avoid changing fifos before message submission, because
> IP can start receiving message in a slave mode (race).

I2C is not full-duplex. There's no way it will receive any data while
you're transmitting, right ?

> 3. dev->threshold is changed in range 1-fifo_size/2. So instead of RDR
> we get RRDY and for messages larger then fifo_size/2 we still get RRDY
> and RDR.

we will only get RDR if message_size % threshold > 0. If we have a 16
byte transfer and we program threshold to 8 bytes, we will get two RRDY
IRQs.

> Felipe, do you have in mind why do you want to avoid RDR and XDR events?
> Something about errata?

nothing about errata. As the commit log say (or tried to say), if the
entire message fits into the FIFO we save one interrupt. It's a
micro-optimization.

> [1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/i2c/busses/i2c-omap.c?id=dd74548ddece4b9d68e5528287a272fa552c81d0

-- 
balbi

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

Re: Question about patch "i2c: omap: resize fifos before each message"

[Alexander Kochetkov]

03 дек. 2014 г., в 18:49, Felipe Balbi <balbi@ti.com> написал(а):

> 
>> 2. I want to avoid changing fifos before message submission, because
>> IP can start receiving message in a slave mode (race).
> 
> I2C is not full-duplex. There's no way it will receive any data while
> you're transmitting, right ?
> 

I2C is half duplex, right. But, IP work in slave receiver and master transmitter modes.
And IP switch to slave receiver mode after master transfer (simply clear MST bit and ready for
reception and that TRM state about). 
And question sounds like: what happen if we reset or change FIFO threshold value
(in order to submit new master transfer) when IP start receiving data to the fifo.
How many bytes we have to read on RRDY?
That race I'am talking about. And there is only one place where race couldn't happen: it's
ISR (thread). So I want to move almost all master initialization code into ISR.


>> 3. dev->threshold is changed in range 1-fifo_size/2. So instead of RDR
>> we get RRDY and for messages larger then fifo_size/2 we still get RRDY
>> and RDR.
> 
> we will only get RDR if message_size % threshold > 0. If we have a 16
> byte transfer and we program threshold to 8 bytes, we will get two RRDY
> IRQs.
> 
>> Felipe, do you have in mind why do you want to avoid RDR and XDR events?
>> Something about errata?
> 
> nothing about errata. As the commit log say (or tried to say), if the
> entire message fits into the FIFO we save one interrupt. It's a
> micro-optimization.

I see, thank you. But due to error only half of fifo is utilized for that.
And, I'll try to move fifo threshold init code into ISR. Don't see something wrong.

Thank you a lot!

Regards,
Alexander.

Re: Question about patch "i2c: omap: resize fifos before each message"

[Felipe Balbi]

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

Hi,

On Wed, Dec 03, 2014 at 08:34:21PM +0300, Alexander Kochetkov wrote:
> >> 2. I want to avoid changing fifos before message submission, because
> >> IP can start receiving message in a slave mode (race).
> > 
> > I2C is not full-duplex. There's no way it will receive any data while
> > you're transmitting, right ?
> > 
> 
> I2C is half duplex, right. But, IP work in slave receiver and master
> transmitter modes.  And IP switch to slave receiver mode after master

it has more modes than that. It can also be a master receiver and a
slave transmitter. Note also that MST bit does *not* auto clear. This
means that as the driver is today, IP will always be in Master mode
which further strengthens my suspicion that what you describe can't
possibly happen.

> transfer (simply clear MST bit and ready for reception and that TRM
> state about). 

this is wrong. Clear MST bit and IP switches to slave mode. Transmit or
receive is selected through another bit (TRX -> bit 9 on I2C_CON). Also,
the IP won't do anything (considering it's always in master mode) until
STT bit is set again, so there's really no way for what you describe
below to ever happen with current driver.

> And question sounds like: what happen if we reset or change FIFO
> threshold value (in order to submit new master transfer) when IP start
> receiving data to the fifo.  How many bytes we have to read on RRDY?

I don't see how this would ever happen.

> That race I'am talking about. And there is only one place where race
> couldn't happen: it's ISR (thread). So I want to move almost all
> master initialization code into ISR.

if you do that, you'll end up with an IRQ handler that takes a lot of
time to run, because at every IRQ you'll reprogram the thing, it's
better to program things from omap_i2c_xfer_msg() and wait for the IRQ
to happen. Just as it is today.

> >> 3. dev->threshold is changed in range 1-fifo_size/2. So instead of RDR
> >> we get RRDY and for messages larger then fifo_size/2 we still get RRDY
> >> and RDR.
> > 
> > we will only get RDR if message_size % threshold > 0. If we have a 16
> > byte transfer and we program threshold to 8 bytes, we will get two RRDY
> > IRQs.
> > 
> >> Felipe, do you have in mind why do you want to avoid RDR and XDR events?
> >> Something about errata?
> > 
> > nothing about errata. As the commit log say (or tried to say), if the
> > entire message fits into the FIFO we save one interrupt. It's a
> > micro-optimization.
> 
> I see, thank you. But due to error only half of fifo is utilized for
> that.

right, it also tries to use double buffering, so that while IP is
shifting data onto SDA, we can feed the other half of the FIFO with more
data.

> And, I'll try to move fifo threshold init code into ISR. Don't see
> something wrong.

I wouldn't do that. It's just too late... look, IRQs won't fire until
I2C_CON is setup to start a transfer (transmit or receive), you *must*
resize FIFO before starting a transfer otherwise, well, you know...

-- 
balbi

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

Re: Question about patch "i2c: omap: resize fifos before each message"

[Alexander Kochetkov]

03 дек. 2014 г., в 20:49, Felipe Balbi <balbi@ti.com> написал(а):


>  It can also be a master receiver and a
> slave transmitter. 
Agree, but this unrelated to races.

>  Note also that MST bit does *not* auto clear.

Yes, it *does* auto clear!
MST bit automatically clear when IP receive AL.
See TRM [1]. All other TRMs (omap3530, omap3430, 
omap4, omap5) has the same picture.

>From TRM:
2Ci.I2C_CON[10] MST bits are cleared by hardware

And MST bit clear after IP send STP (success transfer).

Did you see what value is in CON register after successful master transfer?
Apply my patch and see that.
It doesn't have MST bit set. That mean IP is in slave mode
(receiver or transmitter - doesn't matter) after *any* master transfer.

And simple test show that. IP respond to GC address (address 0).
And respond to SA address (if programmed).

And TRM[1] figure has comment for 'end' state:
"I2C controller goes into slave receiver mode."

And IP keeps into slave mode until 'suspend'.

Then, after 'resume' IP initialized into slave receiver mode. There is short 
time after resume and master start initialize new transfer.

So, then we start new transfer IP could start receiving slave command.

> Also,
> the IP won't do anything (considering it's always in master mode) until
> STT bit is set again
Yes, it do slave reception or tranmittion with STT bit set to 0.

You could set STT bit to 1, and then it got cleared to 0, you
now, that IP received Start condition with slave transfer.

You could leave STT bit set to 0, but IP still respond to slave transfer.
(at least the IP on dm3730).

And we can't set MST bit again after master transfer to leave IP in the
master mode. We must disable IP (clear EN bit) before transfers to
disable slave mode, or we must handle slave interrupts.
Because un handled  slave interrupt leave SCL low.

[1] AM-DM37x Multimedia Device Silicon Revision 1.x  - sprugn4r,
    p.2815, Figure 17-32. HS I2C Master Receiver Mode, 
    Interrupt Method, in F/S and HS Modes (I2C Mode)

Re: Question about patch "i2c: omap: resize fifos before each message"

[Felipe Balbi]

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

Hi,

On Wed, Dec 03, 2014 at 10:01:33PM +0300, Alexander Kochetkov wrote:
> 03 дек. 2014 г., в 20:49, Felipe Balbi <balbi@ti.com> написал(а):
> 
> 
> >  It can also be a master receiver and a
> > slave transmitter. 
> Agree, but this unrelated to races.
> 
> >  Note also that MST bit does *not* auto clear.
> 
> Yes, it *does* auto clear!
> MST bit automatically clear when IP receive AL.
> See TRM [1]. All other TRMs (omap3530, omap3430, 
> omap4, omap5) has the same picture.
> 
> From TRM:
> 2Ci.I2C_CON[10] MST bits are cleared by hardware
> 
> And MST bit clear after IP send STP (success transfer).
> 
> Did you see what value is in CON register after successful master
> transfer?  Apply my patch and see that.
> It doesn't have MST bit set. That mean IP is in slave mode (receiver
> or transmitter - doesn't matter) after *any* master transfer.
> 
> And simple test show that. IP respond to GC address (address 0).
> And respond to SA address (if programmed).
> 
> And TRM[1] figure has comment for 'end' state:
> "I2C controller goes into slave receiver mode."

had completely missed that comment. Right, IP will switch over into
slave mode. Still, as of today, this can't happen because Multimaster
isn't supported :-)

> And IP keeps into slave mode until 'suspend'.
> 
> Then, after 'resume' IP initialized into slave receiver mode. There is
> short time after resume and master start initialize new transfer.
> 
> So, then we start new transfer IP could start receiving slave command.

you'd first receive an interrupt which would let you figure out if the
interrupt was for Slave or Master modes. Then you can do anything you
want (set a flag that you're going into slave mode and return -EAGAIN on
any master transfer request for example).

> > Also,
> > the IP won't do anything (considering it's always in master mode) until
> > STT bit is set again
> Yes, it do slave reception or tranmittion with STT bit set to 0.
> 
> You could set STT bit to 1, and then it got cleared to 0, you
> now, that IP received Start condition with slave transfer.
> 
> You could leave STT bit set to 0, but IP still respond to slave transfer.
> (at least the IP on dm3730).
> 
> And we can't set MST bit again after master transfer to leave IP in the
> master mode. We must disable IP (clear EN bit) before transfers to
> disable slave mode, or we must handle slave interrupts.

then handle slave interrupts :-) But handle them so that it won't race
with a master transfer request. Moving omap_i2c_xfer() inside the IRQ
handler isn't the best way to do it, however.

-- 
balbi

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

Re: Question about patch "i2c: omap: resize fifos before each message"

[Alexander Kochetkov]

03 дек. 2014 г., в 22:38, Felipe Balbi <balbi@ti.com> написал(а):

> then handle slave interrupts :-) But handle them so that it won't race
> with a master transfer request. Moving omap_i2c_xfer() inside the IRQ
> handler isn't the best way to do it, however.

I do that with care :)

Currently, only 'resize fifo' and 'clear fifo' is the one thing what must 
be moved into ISR (I'll check is it possible or late). 
Because I can't ask ISR to generate interrupt to start master transfer.
I thought about that without luck.
I have to write to CON register. I can lock xfer for short time to
check STAT (for AAS) and either write CON or flag to start master after
slave complete.

'clear fifo' must be done in response to NACK. TRM states this[1]"
"TX and RX FIFOs must be cleared (the I2Ci.I2C_BUF[6] TXFIFO_CLR
and I2Ci.I2C_BUF[14] RXFIFO_CLR bits are set to 1)."

'resize fifo' could be avoided at all, but, it so nice feature.

And yes, if we could utilize full fifo to send message, we could set
threshold to message size and get only RRDY at the end.

If message size is greater than fifo size, we want to use double
buffer scheme to minimize ISR latencies.

But now, variable 'fifo_size' is set to half of IP real fifo size.
And for messages with fifo_size/2 ... fifo_size we use drainig
feature (get RDR, XRD). While we could receive only one IRQ.
I'll fix that.

>> And, I'll try to move fifo threshold init code into ISR. Don't see
>> something wrong.
> 
> I wouldn't do that. It's just too late... look, IRQs won't fire until
> I2C_CON is setup to start a transfer (transmit or receive), you *must*
> resize FIFO before starting a transfer otherwise, well, you know...
Looks, like RRDY is fired after simple compare with threshold.
I'll check is this possible (but, that doesn't cover into TRM).
Or may be it is late to change it.

[1] AM-DM37x Multimedia Device Silicon Revision 1.x  - sprugn4r,
     p. 2796, Table 17-9. HS I2C Interrupt Events