Re: [PATCH V2 2/3] i2c: mxs: Rework the PIO mode operation

[Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>]

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

On Tue, Jul 30, 2013 at 11:20:36PM +0200, Marek Vasut wrote:
> Analyze and rework the PIO mode operation. The PIO mode operation
> was unreliable on MX28, by analyzing the bus with LA, the checks
> for when data were available or were to be sent were wrong.
> 
> The PIO WRITE has to be completely reworked as it multiple problems.
> The MX23 datasheet helped here, see comments in the code for details.
> The problems boil down to:
> - RUN bit in CTRL0 must be set after DATA register was written
> - The PIO transfer must be 4 bytes long tops, otherwise use
>   clock stretching.
> Both of these fixes are implemented.
> 
> The PIO READ operation can only be done for up to four bytes as
> we are unable to read out the data from the DATA register fast
> enough.
> 
> This patch also tries to document the investigation within the
> code.
> 
> Signed-off-by: Marek Vasut <marex-ynQEQJNshbs@public.gmane.org>

Thanks. I'd love to see Acked, Reviewed or Tested tags cause I know ppl
have used these patches. Don't be shy :)

> Cc: Alexandre Belloni <alexandre.belloni-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> Cc: Christoph Baumann <cb-/RsSufbtIHM@public.gmane.org>
> Cc: Fabio Estevam <r49496-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> Cc: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Cc: Torsten Fleischer <to-fleischer-zqRNUXuvxA0b1SvskN2V4Q@public.gmane.org>
> Cc: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-mxs.c |  263 ++++++++++++++++++++++++++----------------
>  1 file changed, 166 insertions(+), 97 deletions(-)
> 
> V2: - Fix the data register shift computation algorithm and document that
>       some more. The original algo failed for short 1-byte writes as seen
>       in http://www.mail-archive.com/linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg12812.html
>     - Add me a copyright/authorship line, since according to git blame, I
>       have quite a lot of authored lines in the driver. Wolfram, I brought
>       this up some time ago already, but finally got to it. You're OK with
>       this, right?

Sure.

One question: Wouldn't it be more logical to have this patch first (fix
pio) and then squash patches 1 and 3 as one on the top (add mx23 to now
working pio)? I am not pushing too hard if this means a lot of work, but
it sounds a bit more logical to me.

>  	if (msg->flags & I2C_M_RD) {
> +		/*
> +		 * PIO READ transfer:
> +		 *
> +		 * This transfer MUST be limited to 4 bytes maximum. It is not
> +		 * possible to transfer more than four bytes via PIO, since we
> +		 * can not in any way make sure we can read the data from the
> +		 * DATA register fast enough. Besides, the RX FIFO is only four
> +		 * bytes deep, thus we can only really read up to four bytes at
> +		 * time. Finally, there is no bit indicating us that new data
> +		 * arrived at the FIFO and can thus be fetched from the DATA
> +		 * register.
> +		 */
> +		BUG_ON(msg->len > 4);

How could this happen? There is a check in mxs_i2c_xfer_msg.


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