Re: [PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode for i2c-read

[Krzysztof Adamski]

W dniu 30.06.2022 o 10:23, Datta, Shubhrajyoti pisze:
> [AMD Official Use Only - General]
>
> Hi Krzysztof,
>
> > -----Original Message-----
> > From: Krzysztof Adamski <krzysztof.adamski@xxxxxxxxx>
> > Sent: Wednesday, June 29, 2022 7:40 PM
> > To: Marek Vasut <marex@xxxxxxx>; Raviteja Narayanam
> > <raviteja.narayanam@xxxxxxxxxx>; linux-i2c@xxxxxxxxxxxxxxx;
> > michal.simek@xxxxxxxxxx
> > Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > git@xxxxxxxxxx; joe@xxxxxxxxxxx
> > Subject: Re: [PATCH v2 03/10] i2c: xiic: Switch to Xiic standard mode for i2c-
> > read
> >
> > [CAUTION: External Email]
> >
> > CAUTION: This message has originated from an External Source. Please use
> > proper judgment and caution when opening attachments, clicking links, or
> > responding to this email.
> >
> >
> > Hi Marek,
> >
> > W dniu 29.06.2022 o 16:05, Marek Vasut pisze:
> > > > [...]
> > > >
> > > > If those two modes only differ in software complexity but we are not
> > > > able to support only the simpler one and we have support for the more
> > > > complicated (standard mode) anyways, we know that standard mode can
> > > > handle or the cases while dynamic mode cannot, we also know that
> > > > dynamic mode is broken on some versions of the core, why do we
> > > > actually keep support for dynamic mode?
> > > If I recall it right, the dynamic mode was supposed to handle
> > > transfers longer than 255 Bytes, which the core cannot do in Standard
> > > mode. It is needed e.g. by Atmel MXT touch controller. I spent a lot
> > > of time debugging the race conditions in the XIIC, which I ultimately
> > > fixed (the patches are upstream), but the long transfers I rather
> > > fixed in the MXT driver instead.
> > >
> > > I also recall there was supposed to be some update for the XIIC core
> > > coming with newer vivado, but I might be wrong about that.
> > It seems to be the other way around - dynamic mode is limited to 255 bytes -
> > when you trigger dynamic mode you first write the address of the slave to
> > the FIFO, then you write the length as one byte so you can't request more
> > than 255 bytes. So *standard* mode is used for those messages. In other
> > words - dynamic mode is the one that is more limited
> > - everything that you can do in dynamic mode you can also do in standard
> > mode. So why don't we use standard mode always for everything?
> However  the current mode is dynamic mode so for less than 255 we can use dynamic mode.(the current behavior will not change)
> Also the dynamic mode  is  nicer on the processor resources. We set the bytes and the controller takes care of
> transferring.
>
> However do not have any strong views open to suggestions.

All I'm saying is that before this patchset, the dynamic mode was used 
in all cases and it made sense - it is easier to work with. But it 
turned out it has its limitations and support for standard mode was 
added with several cases that switch to that mode. The commit message 
suggests that the only difference is in how complicated the code for 
handling them is, does not say why dynamic mode might still be 
preferred. And supporting both of them complicates the code noticeably. 
My understanding now is that the code struggles to use the dynamic mode 
in all cases that it can because that produces less interrupts and so it 
is slightly lighter on resources. So it is a code complication vs 
effectiveness tradeoff. Since this is I2C - a slow bus, I'm not sure it 
is worth it but also don't have strong opinion on that. If nothing else, 
I think it would make sense to update the commit message a little bit to 
better explain why it is worth keeping both modes.

Krzysztof