linux-kernel - Re: [PATCH 2/3] i2c: rcar: introduce Gen4 devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZPhKxsj6VTmIlKUY@shikoro>
Date:   Wed, 6 Sep 2023 11:47:50 +0200
From:   Wolfram Sang <wsa+renesas@...g-engineering.com>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>
Cc:     linux-renesas-soc@...r.kernel.org,
        Andi Shyti <andi.shyti@...nel.org>,
        Magnus Damm <magnus.damm@...il.com>, linux-i2c@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] i2c: rcar: introduce Gen4 devices

Hi Geert,

thank you for the review!

> Note that R-Car Gen4 (incl. R-Car S4) has ICFBSCR bits related to
> Slave Clock Stretch Select (which is not yet supported by the driver).

Thanks for the heads up. I'd need more information about the use case of
these bits. Seperate task.

> According to the Programming Examples in the docs for R-Car Gen3,
> R-Car V3U, S4-8, and V4H, I2C must be reset "at the beginning of
> transmission and reception procedure", so not only for DMA.

Sadly, this is vague. If you look at the example for a combined
write-then-read transfer, then you see that only one reset is done,

i.e.: reset -> write -> rep_start -> read

That would mean that we don't need a reset per read/write message of a
transfer. But a reset per transfer then? I would wonder why because we
could also have a super long transfer with lots of read/write messages
in it. Do we need a reset then inbetween? Or is it really dependant on
the STOP bit being transferred? I guess these are all questions for the
HW team, though.

I was reluctant to add the reset too often because my measurements back
then showed that it costs around 5us every time. Annoying. Maybe I
should take it easy and follow the documentation. But then I am still
not sure if a large transfer with way more than two messages are OK
without reset? I will ask the HW team.

> Also, you didn't the touch the checks in rcar_i2c_cleanup_dma():
...
> and rcar_i2c_master_xfer():
...
> 
> Don't these apply to R-Car Gen4? I can't easily find where this quirk
> is documented (perhaps just as a commit in the BSP?), but at least the
> "Usage note for DMA mode of Receive Operation" looks identical for
> R-Car Gen3 and for the various R-Car Gen4 variants.

My memory played a trick on me here. I asked Shimoda-san about this
issue on Gen4. I thought I got an answer that it was fixed, so I left
the code Gen3 only. But he actually never got a reply and I forgot to
ping about it.

The latest documentation has now a "usage note for DMA mode" about it
implying that the issue is still present on Gen4 :(

> BTW, depending on the answers to my questions above, you may want to
> replace the rcar_i2c_type enum by a feature mask...

That might be an option. I need to reshuffle my I2C patches first,
though. I'll send some cleanups first to have them out of the way. Then,
I will respin Gen4 support and take care of the DMA RX issue and the new
reset handling there. Thank you for your input!

All the best,

   Wolfram

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)