Hi Wolfram,
Could you please provide feedback to our response at,
https://lore.kernel.org/all/0a07902900bc4ecc84bd93a6b85a2e0c@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
Thank you,
Regards,
Bhuvanesh
-----Original Message-----
From: Gabbasov, Andrew <Andrew_Gabbasov@xxxxxxxxxx>
Sent: 18 February 2022 16:33
To: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
Cc: linux-renesas-soc@xxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Geert Uytterhoeven <geert+renesas@xxxxxxxxx>; Surachari, Bhuvanesh <Bhuvanesh_Surachari@xxxxxxxxxx>
Subject: RE: [PATCH v2] i2c: rcar: add SMBus block read support
Hi Wolfram!
Thank you for your feedback!
See my responses below.
> -----Original Message-----
> From: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> Sent: Thursday, February 17, 2022 10:45 PM
> To: Gabbasov, Andrew <Andrew_Gabbasov@xxxxxxxxxx>
> Cc: linux-renesas-soc@xxxxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; Geert Uytterhoeven
> <geert+renesas@xxxxxxxxx>; Surachari, Bhuvanesh
> <Bhuvanesh_Surachari@xxxxxxxxxx>
> Subject: Re: [PATCH v2] i2c: rcar: add SMBus block read support
>
[skipped]
>
> > This patch (adapted) was tested with v4.14, but due to lack of real
> > hardware with SMBus block read operations support, using
> > "simulation", that is manual analysis of data, read from plain I2C
> > devices with SMBus block read request.
>
> You could wire up two R-Car I2C instances, set up one as an I2C slave
> handled by the I2C testunit and then use the other instance with
> SMBUS_BLOCK_PROC_CALL which also needs RECV_LEN. Check
> Documentation/i2c/slave-testunit-backend.rst for details.
You mean physical connection of two R-Car boards via I2C bus, or physical connection of I2C bus wires on the single board, right?
It looks like all the boards, that I have access to, do not have I2C bus wires exposed to some connectors, so both variants would require hardware re-wiring modification of the boards, which is not an option for me. Or do I understand you incorrectly and you mean something different?
> I wonder a bit about the complexity of your patch. In my WIP-branch
> for 256-byte transfers, I have the following patch. It is only missing
> the range check for the received byte, but that it easy to add. Do you
> see anything else missing? If not, I prefer this simpler version
> because it is less intrusive and the state machine is a bit fragile
> (due to HW issues with old HW).
Most of complexity in my patch is related to DMA transfers support, that I'm trying to retain for SMBus block data transfers too (for the rest of bytes after the first "length" byte). Your simple patch makes the driver perform all M_RECV_LEN transfers in PIO mode only (with no DMA at all), which is probably not quite good (it's a pity to loose existing HW capability, already supported by the driver).
Also, see a couple of comments below.
> From: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> Date: Sun, 2 Aug 2020 00:24:52 +0200
> Subject: [PATCH] i2c: rcar: add support for I2C_M_RECV_LEN
>
> Signed-off-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> ---
> drivers/i2c/busses/i2c-rcar.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-rcar.c
> b/drivers/i2c/busses/i2c-rcar.c index 217def2d7cb4..e473f5c0a708
> 100644
> --- a/drivers/i2c/busses/i2c-rcar.c
> +++ b/drivers/i2c/busses/i2c-rcar.c
> @@ -528,6 +528,7 @@ static void rcar_i2c_irq_send(struct rcar_i2c_priv
> *priv, u32 msr) static void rcar_i2c_irq_recv(struct rcar_i2c_priv
> *priv, u32 msr) {
> struct i2c_msg *msg = priv->msg;
> + bool recv_len_init = priv->pos == 0 && msg->flags & I2C_M_RECV_LEN;
>
> /* FIXME: sometimes, unknown interrupt happened. Do nothing */
> if (!(msr & MDR))
> @@ -542,11 +543,13 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr)
> } else if (priv->pos < msg->len) {
> /* get received data */
> msg->buf[priv->pos] = rcar_i2c_read(priv, ICRXTX);
> + if (recv_len_init)
> + msg->len += msg->buf[0];
> priv->pos++;
> }
>
> /* If next received data is the _LAST_, go to new phase. */
> - if (priv->pos + 1 == msg->len) {
> + if (priv->pos + 1 == msg->len && !recv_len_init) {
If a message contains a single byte after the length byte, when we come here after processing the length (in the same function call), "pos" is 1, "len" is 2, and we indeed are going to process the last byte.
However, "recv_len_init" is still "true", and we skip these corresponding register writes, which is probably incorrect.
The flag in this case should be re-set back to "false" after length processing and "pos" moving, but I think the variant in my patch (leaving this "if" unchanged, but skipping it on the first pass with "goto") may be even simpler.
> if (priv->flags & ID_LAST_MSG) {
> rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP);
> } else {
> @@ -889,7 +892,7 @@ static u32 rcar_i2c_func(struct i2c_adapter *adap)
> * I2C_M_IGNORE_NAK (automatically sends STOP after NAK)
> */
> u32 func = I2C_FUNC_I2C | I2C_FUNC_SLAVE |
> - (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> + (I2C_FUNC_SMBUS_EMUL_ALL & ~I2C_FUNC_SMBUS_QUICK);
This flags setting adds also I2C_FUNC_SMBUS_BLOCK_PROC_CALL flag, which is missed in my patch. My patch should probably be updated to include it too (if you'll agree to take my variant ;-) ).
>
> if (priv->flags & ID_P_HOST_NOTIFY)
> func |= I2C_FUNC_SMBUS_HOST_NOTIFY;
>
> Happy hacking,
>
> Wolfram
Thanks!
Best regards,
Andrew
