| Message ID | 20211006182314.10585-1-andrew_gabbasov@mentor.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [v2] i2c: rcar: add SMBus block read support | expand |
Hi Andrew, first sorry that it took so long. The reason here is that my original plan was to add 256-byte support to RECV_LEN in the I2C core and enable it on R-Car afterwards. Sadly, I never found the time to drive this forward. So, all RECV_LEN things got stuck for a while :( > 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. 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). From: Wolfram Sang <wsa+renesas@sang-engineering.com> 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@sang-engineering.com> --- 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 (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); if (priv->flags & ID_P_HOST_NOTIFY) func |= I2C_FUNC_SMBUS_HOST_NOTIFY; Happy hacking, Wolfram
Hi Wolfram! Thank you for your feedback! See my responses below. > -----Original Message----- > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > Sent: Thursday, February 17, 2022 10:45 PM > To: Gabbasov, Andrew <Andrew_Gabbasov@mentor.com> > Cc: linux-renesas-soc@vger.kernel.org; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org; Geert > Uytterhoeven <geert+renesas@glider.be>; Surachari, Bhuvanesh <Bhuvanesh_Surachari@mentor.com> > 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@sang-engineering.com> > 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@sang-engineering.com> > --- > 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
Hi Wolfram,
Could you please provide feedback to our response at,
https://lore.kernel.org/all/0a07902900bc4ecc84bd93a6b85a2e0c@svr-ies-mbx-02.mgc.mentorg.com/
Thank you,
Regards,
Bhuvanesh
-----Original Message-----
From: Gabbasov, Andrew <Andrew_Gabbasov@mentor.com>
Sent: 18 February 2022 16:33
To: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: linux-renesas-soc@vger.kernel.org; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org; Geert Uytterhoeven <geert+renesas@glider.be>; Surachari, Bhuvanesh <Bhuvanesh_Surachari@mentor.com>
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@sang-engineering.com>
> Sent: Thursday, February 17, 2022 10:45 PM
> To: Gabbasov, Andrew <Andrew_Gabbasov@mentor.com>
> Cc: linux-renesas-soc@vger.kernel.org; linux-i2c@vger.kernel.org;
> linux-kernel@vger.kernel.org; Geert Uytterhoeven
> <geert+renesas@glider.be>; Surachari, Bhuvanesh
> <Bhuvanesh_Surachari@mentor.com>
> 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@sang-engineering.com>
> 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@sang-engineering.com>
> ---
> 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
Dear Wolfram, Dear Andrew, Dear Geert, A couple of questions and test results below. On Thu, Feb 17, 2022 at 08:44:51PM +0100, Wolfram Sang wrote: > Hi Andrew, > > first sorry that it took so long. The reason here is that my original > plan was to add 256-byte support to RECV_LEN in the I2C core and enable > it on R-Car afterwards. Sadly, I never found the time to drive this > forward. So, all RECV_LEN things got stuck for a while :( > > > 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. I am obviously not an SMBus expert, but I wonder if simply testing the PCA9654 I/O Expander with SMBus support on the H3-Salvator-X target could be acceptable as a test procedure? See some test results below. > > 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). > > From: Wolfram Sang <wsa+renesas@sang-engineering.com> > 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@sang-engineering.com> > --- > 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 (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); > > if (priv->flags & ID_P_HOST_NOTIFY) > func |= I2C_FUNC_SMBUS_HOST_NOTIFY; > ############################################################ ################# PATCH-INDEPENDENT OUTPUT ################# ############################################################ ## .config: https://gist.github.com/erosca/690c3e6065b55546e511f9ef8ba59625 ## i2c-tools: https://git.kernel.org/pub/scm/utils/i2c-tools/i2c-tools.git/commit/?id=cf3541b8a7 root@rcar-gen3:# uname -r 5.17.0+ root@rcar-gen3:# cat /sys/firmware/devicetree/base/model Renesas Salvator-X board based on r8a77951 root@rcar-gen3:# i2cdetect -l i2c-7 i2c e60b0000.i2c I2C adapter i2c-2 i2c e6510000.i2c I2C adapter i2c-4 i2c e66d8000.i2c I2C adapter ^ ` i2c-4 is the PCA9654 I/O Expander with SMBus protocol support root@rcar-gen3:# i2cdetect -y -r 4 0 1 2 3 4 5 6 7 8 9 a b c d e f 00: -- -- -- -- -- -- -- -- 10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 20: UU -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 60: UU UU UU UU UU UU -- -- 68 -- UU -- -- -- -- -- 70: UU UU UU UU UU UU -- -- ############################################################ ###################### VANILLA v5.17 ####################### ############################################################ root@rcar-gen3:# i2cdetect -F 4 Functionalities implemented by /dev/i2c-4: I2C yes SMBus Quick Command no SMBus Send Byte yes SMBus Receive Byte yes SMBus Write Byte yes SMBus Read Byte yes SMBus Write Word yes SMBus Read Word yes SMBus Process Call yes SMBus Block Write yes SMBus Block Read no <<<--- We aim to enable this SMBus Block Process Call no SMBus PEC yes I2C Block Write yes I2C Block Read yes root@rcar-gen3:# i2cget -y 4 0x68 0 i 8 0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08 root@rcar-gen3:# i2cget -y 4 0x68 0 s Error: Adapter does not have SMBus block read capability ############################################################ #################### ANDREW'S V2 PATCH ##################### ############################################################ root@rcar-gen3:# i2cdetect -F 4 Functionalities implemented by /dev/i2c-4: I2C yes SMBus Quick Command no SMBus Send Byte yes SMBus Receive Byte yes SMBus Write Byte yes SMBus Read Byte yes SMBus Write Word yes SMBus Read Word yes SMBus Process Call yes SMBus Block Write yes SMBus Block Read yes <<<--- Enabled (tested below) SMBus Block Process Call no SMBus PEC yes I2C Block Write yes I2C Block Read yes root@rcar-gen3:# i2cget -y 4 0x68 0 i 8 0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08 root@rcar-gen3:# i2cget -y 4 0x68 0 s 0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08 ############################################################ ##################### WOLFRAM'S PATCH ###################### ############################################################ root@rcar-gen3:# i2cdetect -F 4 Functionalities implemented by /dev/i2c-4: I2C yes SMBus Quick Command no SMBus Send Byte yes SMBus Receive Byte yes SMBus Write Byte yes SMBus Read Byte yes SMBus Write Word yes SMBus Read Word yes SMBus Process Call yes SMBus Block Write yes SMBus Block Read yes <<<--- Enabled (tested) SMBus Block Process Call yes <<<--- Enabled (not tested) SMBus PEC yes I2C Block Write yes I2C Block Read yes root@rcar-gen3:# i2cget -y 4 0x68 0 i 8 0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08 root@rcar-gen3:# i2cget -y 4 0x68 0 s 0xff 0x06 0xff 0x5f 0xff 0x11 0x08 0x08 ############################################################ Any comments? Best regards, Eugeniu
Hi Eugeniu, coming back to this topic, thanks for your patience everyone. > > > > 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. > > I am obviously not an SMBus expert, but I wonder if simply testing the > PCA9654 I/O Expander with SMBus support on the H3-Salvator-X target > could be acceptable as a test procedure? See some test results below. As long as the first read value is 8 (or lower than 32), it will work. But it is testing only this one value while my method above is more flexible and allows for arbitrary test patterns. However, your tests already showed that Andrew's patch seems to be not correct. > ############################################################ > #################### ANDREW'S V2 PATCH ##################### > ############################################################ > root@rcar-gen3:# i2cget -y 4 0x68 0 i 8 > 0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08 > > root@rcar-gen3:# i2cget -y 4 0x68 0 s > 0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08 This is wrong. The first byte is the length byte and should not be seen here. Check the i2c_smbus_read_block_data() implementation in i2c-tools. > ############################################################ > ##################### WOLFRAM'S PATCH ###################### > ############################################################ > root@rcar-gen3:# i2cget -y 4 0x68 0 i 8 > 0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08 > > root@rcar-gen3:# i2cget -y 4 0x68 0 s > 0xff 0x06 0xff 0x5f 0xff 0x11 0x08 0x08 This is how it should look like IMO. Happy hacking, Wolfram
Hi Andrew, thanks for your patience, I can finally work on this issue. > > 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? I have two instances on the same board wired. > 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? Probably you understood correctly. Which boards do you have access to? I use the EXIO connectors on a Salvator-X(S). > 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). I will have a look into RECV_LEN and DMA. I already started looking into it but will need to dive in some more. Stay tuned, I hope to have the next response ready this week. > > /* 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. I also need to look into this but thank you already for the detailed explanation! > > 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 ;-) ). Yes, the final version, whatever it will be, should use this new macro. Until soon, Wolfram
BTW... > > ############################################################ > > ##################### WOLFRAM'S PATCH ###################### > > ############################################################ > > root@rcar-gen3:# i2cget -y 4 0x68 0 i 8 > > 0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08 ... for further tests I think the last parameter should be "i 9" here... > > > > root@rcar-gen3:# i2cget -y 4 0x68 0 s > > 0xff 0x06 0xff 0x5f 0xff 0x11 0x08 0x08 ... to see if these 8 bytes match the last 8 bytes from above. The first byte from above is returned internally as the length. It is not a data byte.
Hello Wolfram, Thanks for your feedback! On Wed, Mar 30, 2022 at 01:09:17PM +0200, Wolfram Sang wrote: > > BTW... > > > > ############################################################ > > > ##################### WOLFRAM'S PATCH ###################### > > > ############################################################ > > > root@rcar-gen3:# i2cget -y 4 0x68 0 i 8 > > > 0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08 > > ... for further tests I think the last parameter should be "i 9" here... Your patch re-tested on your request (we'll use -i 9 in the future): root@rcar-gen3:# i2cget -y 4 0x68 0 s 0xff 0x06 0xff 0x5f 0xff 0x11 0x08 0x08 root@rcar-gen3:# i2cget -y 4 0x68 0 i 9 0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08 0x08 root@rcar-gen3:# i2cget -y 4 0x68 0 i 0x08 0xff 0x06 0xff 0x5f 0xff 0x11 0x08 0x08 0xff 0xff 0x7e 0x99 0x3e 0x00 0x00 0xfb 0xff 0xff 0x87 0x27 0xff 0x04 0xff 0x30 0x03 0xfd 0xff 0xff 0xff 0xff 0xff > > > > > > root@rcar-gen3:# i2cget -y 4 0x68 0 s > > > 0xff 0x06 0xff 0x5f 0xff 0x11 0x08 0x08 > > ... to see if these 8 bytes match the last 8 bytes from above. The first > byte from above is returned internally as the length. It is not a data > byte. > BTW, thanks to Bhuvanesh, we've got another patch [*] which tries to combine the best of both worlds: * DMA support in the v1/v2 patches from Andrew/Bhuvanesh * Simplicity of your proposal in https://lore.kernel.org/lkml/Yg6ls0zyTDe7LQbK@kunai/ Unfortunately, this patch has a dependency to the rcar_i2c_is_pio() in https://github.com/renesas-rcar/linux-bsp/commit/55d2d2fb8b0 (which should be resolvable by extracting the function). Do you think we are on the right track with this new approach or do you feel the implementation is still overly complicated? Appreciate your time/feedback. Best regards, Eugeniu [*] v3 From: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com> Date: Wed, 26 May 2021 11:36:36 +0530 Subject: [PATCH v3] i2c: rcar: add SMBus block read support The SMBus block read is currently not supported for rcar i2c devices. Add appropriate support to R-Car i2c driver, so that blocks of data can be read using SMbus block reads (using i2c_smbus_read_block_data() function from i2c-core-smbus.c). Inspired from: - commit 8e8782c71595a5 ("i2c: imx: add SMBus block read support") - https://lore.kernel.org/lkml/Yg6ls0zyTDe7LQbK@kunai/ (proposal/suggestion from Wolfram Sang) Suggested-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com> Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com> --- drivers/i2c/busses/i2c-rcar.c | 39 ++++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c index f71c730f9838..f4f36689464c 100644 --- a/drivers/i2c/busses/i2c-rcar.c +++ b/drivers/i2c/busses/i2c-rcar.c @@ -105,6 +105,7 @@ #define ID_DONE (1 << 2) #define ID_ARBLOST (1 << 3) #define ID_NACK (1 << 4) +#define ID_EPROTO (1 << 5) /* persistent flags */ #define ID_P_HOST_NOTIFY BIT(28) #define ID_P_REP_AFTER_RD BIT(29) @@ -522,6 +523,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)) @@ -535,12 +537,37 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr) rcar_i2c_dma(priv); } else if (priv->pos < msg->len) { /* get received data */ - msg->buf[priv->pos] = rcar_i2c_read(priv, ICRXTX); + u8 data = rcar_i2c_read(priv, ICRXTX); + + if (recv_len_init) { + /* + * First byte is the length of remaining packet + * in the SMBus block data read. Add it to + * msg->len. + */ + if (data == 0 || data > I2C_SMBUS_BLOCK_MAX) { + priv->flags |= ID_DONE | ID_EPROTO; + return; + } + msg->len += data; + + if (!rcar_i2c_is_pio(priv)) { + /* + * Still try to use DMA to receive the rest of + * data + */ + rcar_i2c_dma(priv); + goto next_txn; + } else { + recv_len_init = false; + } + } + msg->buf[priv->pos] = data; 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 (priv->flags & ID_LAST_MSG) { rcar_i2c_write(priv, ICMCR, RCAR_BUS_PHASE_STOP); } else { @@ -549,6 +576,7 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr) } } +next_txn: if (priv->pos == msg->len && !(priv->flags & ID_LAST_MSG)) rcar_i2c_next_msg(priv); else @@ -847,6 +875,8 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap, ret = -ENXIO; } else if (priv->flags & ID_ARBLOST) { ret = -EAGAIN; + } else if (priv->flags & ID_EPROTO) { + ret = -EPROTO; } else { ret = num - priv->msgs_left; /* The number of transfer */ } @@ -909,6 +939,8 @@ static int rcar_i2c_master_xfer_atomic(struct i2c_adapter *adap, ret = -ENXIO; } else if (priv->flags & ID_ARBLOST) { ret = -EAGAIN; + } else if (priv->flags & ID_EPROTO) { + ret = -EPROTO; } else { ret = num - priv->msgs_left; /* The number of transfer */ } @@ -975,7 +1007,8 @@ 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 & ~I2C_FUNC_SMBUS_QUICK) | + I2C_FUNC_SMBUS_READ_BLOCK_DATA; if (priv->flags & ID_P_HOST_NOTIFY) func |= I2C_FUNC_SMBUS_HOST_NOTIFY;
> I use the EXIO connectors on a Salvator-X(S).
Sorry, I confused things. The EXIO connectors on Salvator-X(S) are not
so helpful. I use the EXIO connectors on a Lager board (R-Car H2) for
testing.
> > /* 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 Confirmed. Tests fail with only one extra byte and clearing 'recv_len_init' fixes the issue. I don't think this is the proper solution, though. I think it will create more readable code if we update the checks. So people will understand what we are aiming for. The current code is already implicit enough.
Hi Eugeniu, > BTW, thanks to Bhuvanesh, we've got another patch [*] which tries > to combine the best of both worlds: > > * DMA support in the v1/v2 patches from Andrew/Bhuvanesh > * Simplicity of your proposal in https://lore.kernel.org/lkml/Yg6ls0zyTDe7LQbK@kunai/ This was nice to see. But where does it come from? I don't see it on this list and I also couldn't find it in the regular BSP? > Unfortunately, this patch has a dependency to the rcar_i2c_is_pio() > in https://github.com/renesas-rcar/linux-bsp/commit/55d2d2fb8b0 > (which should be resolvable by extracting the function). This patch is obsolete since March 2019. It has been properly fixed with 94e290b0e9a6 ("i2c: rcar: wait for data empty before starting DMA"). I am still trying to feed this information back. > Do you think we are on the right track with this new approach or do > you feel the implementation is still overly complicated? The approach is much better but there are still things I don't like. The use of 'goto next_txn' is bad. I hope it could be done better with refactoring the code, so DMA will be tried at one place (with two conditions then). Not sure yet, I am still working on refactoring the one-byte transfer which is broken with my patch. What we surely can use from this patch is the -EPROTO handling because I have given up on converting the max read block size first. We can still remove it from this driver if that gets implemented somewhen. > + if (!rcar_i2c_is_pio(priv)) { > + /* > + * Still try to use DMA to receive the rest of > + * data > + */ > + rcar_i2c_dma(priv); > + goto next_txn; > + } else { > + recv_len_init = false; > + } So, I'd like to get rid of this block with refactoring. > u32 func = I2C_FUNC_I2C | I2C_FUNC_SLAVE | > - (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK); > + (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK) | > + I2C_FUNC_SMBUS_READ_BLOCK_DATA; Still not using the new macro to include PROC_CALL, but that is easy to change. All the best, Wolfram
Hi Wolfram, On Fri, Apr 01, 2022 at 06:38:56PM +0200, Wolfram Sang wrote: > > BTW, thanks to Bhuvanesh, we've got another patch [*] which tries > > to combine the best of both worlds: > > > > * DMA support in the v1/v2 patches from Andrew/Bhuvanesh > > * Simplicity of your proposal in https://lore.kernel.org/lkml/Yg6ls0zyTDe7LQbK@kunai/ > > This was nice to see. But where does it come from? I don't see it on > this list and I also couldn't find it in the regular BSP? The patch was worked on and tested collaboratively w/o submission. The idea was to push it to LKML, once/after you are happy with it. > > Unfortunately, this patch has a dependency to the rcar_i2c_is_pio() > > in https://github.com/renesas-rcar/linux-bsp/commit/55d2d2fb8b0 > > (which should be resolvable by extracting the function). > > This patch is obsolete since March 2019. It has been properly fixed with > 94e290b0e9a6 ("i2c: rcar: wait for data empty before starting DMA"). I > am still trying to feed this information back. Thanks for the precious feedback. We've requested Renesas to revert the obsolete BSP commit, based on your recommendation. In general, the Renesas kernel always carries a set of patches with non-mainlined changes, Fortunately, for i2c specifically (as opposed to other subsystems), it is narrow enough to not raise major concerns: $ git log --oneline v5.10.41..rcar-5.1.2 -- drivers/i2c/busses/i2c-rcar.c 6745303b2bfa i2c: rcar: Add support for r8a77961 (R-Car M3-W+) 3422d3131700 i2c: rcar: Support the suspend/resume 5680e77f2427 i2c: rcar: Tidy up the register order for hardware specification ver1.00. 41394ab7420f i2c: rcar: Fix I2C DMA transmission by setting sequence > > > Do you think we are on the right track with this new approach or do > > you feel the implementation is still overly complicated? > > The approach is much better but there are still things I don't like. The > use of 'goto next_txn' is bad. I hope it could be done better with > refactoring the code, so DMA will be tried at one place (with two > conditions then). Not sure yet, I am still working on refactoring the > one-byte transfer which is broken with my patch. What we surely can use > from this patch is the -EPROTO handling because I have given up on > converting the max read block size first. We can still remove it from > this driver if that gets implemented somewhen. Thank you for the review comments. We are still working on a cleaner solution. In case it comes from you first, we are very much keen to give it a try on the target and report the results. Best regards, Eugeniu
Hi Eugeniu, > The idea was to push it to LKML, once/after you are happy with it. I see. > Thanks for the precious feedback. We've requested Renesas to revert the > obsolete BSP commit, based on your recommendation. Thank you! > In general, the Renesas kernel always carries a set of patches with > non-mainlined changes, Fortunately, for i2c specifically (as opposed > to other subsystems), it is narrow enough to not raise major concerns: Well, yes, that shows that I am mostly successful with reporting back to the BSP team :D But some are still missing, as you can see. > $ git log --oneline v5.10.41..rcar-5.1.2 -- drivers/i2c/busses/i2c-rcar.c > 6745303b2bfa i2c: rcar: Add support for r8a77961 (R-Car M3-W+) Can be dropped: "Driver matches against family-specific compatible value, which has always been present in upstream DTS" > 3422d3131700 i2c: rcar: Support the suspend/resume Already upstream: "18569fa89a4db9ed6b5181624788a1574a9b6ed7 # i2c: rcar: add suspend/resume support" > 5680e77f2427 i2c: rcar: Tidy up the register order for hardware specification ver1.00. Relevant parts upstream: "e7f4264821a4ee07775f3775f8530cfa9a6d4b5d # i2c: rcar: enable interrupts before starting transfer" Other parts can be dropped. > 41394ab7420f i2c: rcar: Fix I2C DMA transmission by setting sequence Upstream: "94e290b0e9a6c360a5660c480c1ba996d892c650 # i2c: rcar: wait for data empty before starting DMA" > Thank you for the review comments. We are still working on a cleaner > solution. In case it comes from you first, we are very much keen to > give it a try on the target and report the results. I have a cleaner solution quite ready. Give me another hour for testing before I send it out. All the best, Wolfram
Hi Wolfram, On Tue, Apr 05, 2022 at 11:43:29AM +0200, Wolfram Sang wrote: > > In general, the Renesas kernel always carries a set of patches with > > non-mainlined changes, Fortunately, for i2c specifically (as opposed > > to other subsystems), it is narrow enough to not raise major concerns: > > Well, yes, that shows that I am mostly successful with reporting back to > the BSP team :D 100%! I remember the early days of Renesas R-Car Gen3 kernels, with literally thousands of patches on top of v4.2/v4.4/v4.6 vanilla tags. It felt like mission impossible to upstream those. But here we are. Kudos! Best regards, Eugeniu
> > Well, yes, that shows that I am mostly successful with reporting back to > > the BSP team :D > > 100%! I remember the early days of Renesas R-Car Gen3 kernels, with > literally thousands of patches on top of v4.2/v4.4/v4.6 vanilla tags. > > It felt like mission impossible to upstream those. But here we are. Kudos! Wow, thanks! I am glad you appreciate our work! All the best, Wolfram
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c index bff9913c37b8..e4b603f425fa 100644 --- a/drivers/i2c/busses/i2c-rcar.c +++ b/drivers/i2c/busses/i2c-rcar.c @@ -105,6 +105,7 @@ #define ID_DONE (1 << 2) #define ID_ARBLOST (1 << 3) #define ID_NACK (1 << 4) +#define ID_EPROTO (1 << 5) /* persistent flags */ #define ID_P_HOST_NOTIFY BIT(28) #define ID_P_REP_AFTER_RD BIT(29) @@ -412,6 +413,7 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv) struct device *dev = rcar_i2c_priv_to_dev(priv); struct i2c_msg *msg = priv->msg; bool read = msg->flags & I2C_M_RD; + bool block_data = msg->flags & I2C_M_RECV_LEN; enum dma_data_direction dir = read ? DMA_FROM_DEVICE : DMA_TO_DEVICE; struct dma_chan *chan = read ? priv->dma_rx : priv->dma_tx; struct dma_async_tx_descriptor *txdesc; @@ -425,19 +427,22 @@ static bool rcar_i2c_dma(struct rcar_i2c_priv *priv) !(msg->flags & I2C_M_DMA_SAFE) || (read && priv->flags & ID_P_NO_RXDMA)) return false; + buf = priv->msg->buf; + len = priv->msg->len; + if (read) { /* * The last two bytes needs to be fetched using PIO in * order for the STOP phase to work. */ - buf = priv->msg->buf; - len = priv->msg->len - 2; - } else { + len -= 2; + } + if (!read || block_data) { /* - * First byte in message was sent using PIO. + * First byte in message was sent or received using PIO. */ - buf = priv->msg->buf + 1; - len = priv->msg->len - 1; + buf++; + len--; } dma_addr = dma_map_single(chan->device->dev, buf, len, dir); @@ -530,6 +535,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 block_data = msg->flags & I2C_M_RECV_LEN; /* FIXME: sometimes, unknown interrupt happened. Do nothing */ if (!(msr & MDR)) @@ -538,8 +544,29 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr) if (msr & MAT) { /* * Address transfer phase finished, but no data at this point. - * Try to use DMA to receive data. + * Try to use DMA to receive data if it is not SMBus block + * data read. */ + if (block_data) + goto next_txn; + + rcar_i2c_dma(priv); + } else if (priv->pos == 0 && block_data) { + /* + * First byte is the length of remaining packet + * in the SMBus block data read. Add it to + * msg->len. + */ + u8 data = rcar_i2c_read(priv, ICRXTX); + + if (data == 0 || data > I2C_SMBUS_BLOCK_MAX) { + priv->flags |= ID_DONE | ID_EPROTO; + return; + } + msg->len += data; + msg->buf[priv->pos] = data; + priv->pos++; + /* Still try to use DMA to receive the rest of data */ rcar_i2c_dma(priv); } else if (priv->pos < msg->len) { /* get received data */ @@ -557,6 +584,7 @@ static void rcar_i2c_irq_recv(struct rcar_i2c_priv *priv, u32 msr) } } +next_txn: if (priv->pos == msg->len && !(priv->flags & ID_LAST_MSG)) rcar_i2c_next_msg(priv); else @@ -855,6 +883,8 @@ static int rcar_i2c_master_xfer(struct i2c_adapter *adap, ret = -ENXIO; } else if (priv->flags & ID_ARBLOST) { ret = -EAGAIN; + } else if (priv->flags & ID_EPROTO) { + ret = -EPROTO; } else { ret = num - priv->msgs_left; /* The number of transfer */ } @@ -917,6 +947,8 @@ static int rcar_i2c_master_xfer_atomic(struct i2c_adapter *adap, ret = -ENXIO; } else if (priv->flags & ID_ARBLOST) { ret = -EAGAIN; + } else if (priv->flags & ID_EPROTO) { + ret = -EPROTO; } else { ret = num - priv->msgs_left; /* The number of transfer */ } @@ -983,7 +1015,8 @@ 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 & ~I2C_FUNC_SMBUS_QUICK) | + I2C_FUNC_SMBUS_READ_BLOCK_DATA; if (priv->flags & ID_P_HOST_NOTIFY) func |= I2C_FUNC_SMBUS_HOST_NOTIFY;
The smbus block read is not currently supported for rcar i2c devices. This patchset adds the support to rcar i2c bus so that blocks of data can be read using SMbus block reads.(using i2c_smbus_read_block_data() function from the i2c-core-smbus.c). Inspired by commit 8e8782c71595 ("i2c: imx: add SMBus block read support") 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. Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com> Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com> --- drivers/i2c/busses/i2c-rcar.c | 49 +++++++++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 8 deletions(-)