[PATCH] i2c: cadence: Support PEC for SMBus block read

[PATCH] i2c: cadence: Support PEC for SMBus block read

[Lars-Peter Clausen]

SMBus packet error checking (PEC) is implemented by appending one
additional byte of checksum data at the end of the message. This provides
additional protection and allows to detect data corruption on the I2C bus.

SMBus block reads support variable length reads. The first byte in the read
message is the number of available data bytes.

The combination of PEC and block read is currently not supported by the
Cadence I2C driver.
 * When PEC is enabled the maximum transfer length for block reads
   increases from 33 to 34 bytes.
 * The I2C core smbus emulation layer relies on the driver updating the
   `i2c_msg` `len` field with the number of received bytes. The updated
   length is used when checking the PEC.

Add support to the Cadence I2C driver for handling SMBus block reads with
PEC. To determine the maximum transfer length uses the initial `len` value
of the `i2c_msg`. When PEC is enabled this will be 2, when it is disabled
it will be 1.

Once a read transfer is done also increment the `len` field by the amount
of received data bytes.

This change has been tested with a UCM90320 PMBus power monitor, which
requires block reads to access certain data fields, but also has PEC
enabled by default.

Fixes: df8eb5691c48 ("i2c: Add driver for Cadence I2C controller")
Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/i2c/busses/i2c-cadence.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index 630cfa4ddd46..33f5588a50c0 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -573,8 +573,13 @@ static void cdns_i2c_mrecv(struct cdns_i2c *id)
 	ctrl_reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET);
 	ctrl_reg |= CDNS_I2C_CR_RW | CDNS_I2C_CR_CLR_FIFO;
 
+	/*
+	 * Receive up to I2C_SMBUS_BLOCK_MAX data bytes, plus one message length
+	 * byte, plus one checksum byte if PEC is enabled. p_msg->len will be 2 if
+	 * PEC is enabled, otherwise 1.
+	 */
 	if (id->p_msg->flags & I2C_M_RECV_LEN)
-		id->recv_count = I2C_SMBUS_BLOCK_MAX + 1;
+		id->recv_count = I2C_SMBUS_BLOCK_MAX + id->p_msg->len;
 
 	id->curr_recv_count = id->recv_count;
 
@@ -789,6 +794,9 @@ static int cdns_i2c_process_msg(struct cdns_i2c *id, struct i2c_msg *msg,
 	if (id->err_status & CDNS_I2C_IXR_ARB_LOST)
 		return -EAGAIN;
 
+	if (msg->flags & I2C_M_RECV_LEN)
+		msg->len += min_t(unsigned int, msg->buf[0], I2C_SMBUS_BLOCK_MAX);
+
 	return 0;
 }
 
-- 
2.30.2

Re: [PATCH] i2c: cadence: Support PEC for SMBus block read

[Michal Simek]

On 7/17/22 16:52, Lars-Peter Clausen wrote:
> SMBus packet error checking (PEC) is implemented by appending one
> additional byte of checksum data at the end of the message. This provides
> additional protection and allows to detect data corruption on the I2C bus.
> 
> SMBus block reads support variable length reads. The first byte in the read
> message is the number of available data bytes.
> 
> The combination of PEC and block read is currently not supported by the
> Cadence I2C driver.
>   * When PEC is enabled the maximum transfer length for block reads
>     increases from 33 to 34 bytes.
>   * The I2C core smbus emulation layer relies on the driver updating the
>     `i2c_msg` `len` field with the number of received bytes. The updated
>     length is used when checking the PEC.
> 
> Add support to the Cadence I2C driver for handling SMBus block reads with
> PEC. To determine the maximum transfer length uses the initial `len` value
> of the `i2c_msg`. When PEC is enabled this will be 2, when it is disabled
> it will be 1.
> 
> Once a read transfer is done also increment the `len` field by the amount
> of received data bytes.
> 
> This change has been tested with a UCM90320 PMBus power monitor, which
> requires block reads to access certain data fields, but also has PEC
> enabled by default.
> 
> Fixes: df8eb5691c48 ("i2c: Add driver for Cadence I2C controller")

Subject is saying that you adding support for PEC and here you are saying that 
it is fixing initial commit.

If this is adding new support I think Fixes tag shouldn't be here.

If it is fixing issue subject should be updated and this Fixes tag kept here.

The rest looks good to me.

Shubhrajyoti: Can you please test?

Thanks,
Michal

RE: [PATCH] i2c: cadence: Support PEC for SMBus block read

[Datta, Shubhrajyoti]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Simek, Michal <michal.simek@amd.com>
> Sent: Monday, July 18, 2022 2:56 PM
> To: Lars-Peter Clausen <lars@metafoo.de>; Wolfram Sang
> <wsa@kernel.org>; Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>
> Cc: linux-i2c@vger.kernel.org; git <git@xilinx.com>
> Subject: Re: [PATCH] i2c: cadence: Support PEC for SMBus block read
> 
> 
> 
> On 7/17/22 16:52, Lars-Peter Clausen wrote:
> > SMBus packet error checking (PEC) is implemented by appending one
> > additional byte of checksum data at the end of the message. This
> > provides additional protection and allows to detect data corruption on the
> I2C bus.
> >
> > SMBus block reads support variable length reads. The first byte in the
> > read message is the number of available data bytes.
> >
> > The combination of PEC and block read is currently not supported by
> > the Cadence I2C driver.
> >   * When PEC is enabled the maximum transfer length for block reads
> >     increases from 33 to 34 bytes.
> >   * The I2C core smbus emulation layer relies on the driver updating the
> >     `i2c_msg` `len` field with the number of received bytes. The updated
> >     length is used when checking the PEC.
> >
> > Add support to the Cadence I2C driver for handling SMBus block reads
> > with PEC. To determine the maximum transfer length uses the initial
> > `len` value of the `i2c_msg`. When PEC is enabled this will be 2, when
> > it is disabled it will be 1.
> >
> > Once a read transfer is done also increment the `len` field by the
> > amount of received data bytes.
> >
> > This change has been tested with a UCM90320 PMBus power monitor,
> which
> > requires block reads to access certain data fields, but also has PEC
> > enabled by default.
> >
> > Fixes: df8eb5691c48 ("i2c: Add driver for Cadence I2C controller")
> 
> Subject is saying that you adding support for PEC and here you are saying
> that it is fixing initial commit.
> 
> If this is adding new support I think Fixes tag shouldn't be here.
> 
> If it is fixing issue subject should be updated and this Fixes tag kept here.
> 
> The rest looks good to me.
> 
> Shubhrajyoti: Can you please test?

I have tested the reads and write smbus  without packet error check.


> 
> Thanks,
> Michal

Re: [PATCH] i2c: cadence: Support PEC for SMBus block read

[Michal Simek]

On 7/18/22 14:37, Datta, Shubhrajyoti wrote:
> [AMD Official Use Only - General]
> 
> 
> 
>> -----Original Message-----
>> From: Simek, Michal <michal.simek@amd.com>
>> Sent: Monday, July 18, 2022 2:56 PM
>> To: Lars-Peter Clausen <lars@metafoo.de>; Wolfram Sang
>> <wsa@kernel.org>; Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>
>> Cc: linux-i2c@vger.kernel.org; git <git@xilinx.com>
>> Subject: Re: [PATCH] i2c: cadence: Support PEC for SMBus block read
>>
>>
>>
>> On 7/17/22 16:52, Lars-Peter Clausen wrote:
>>> SMBus packet error checking (PEC) is implemented by appending one
>>> additional byte of checksum data at the end of the message. This
>>> provides additional protection and allows to detect data corruption on the
>> I2C bus.
>>>
>>> SMBus block reads support variable length reads. The first byte in the
>>> read message is the number of available data bytes.
>>>
>>> The combination of PEC and block read is currently not supported by
>>> the Cadence I2C driver.
>>>    * When PEC is enabled the maximum transfer length for block reads
>>>      increases from 33 to 34 bytes.
>>>    * The I2C core smbus emulation layer relies on the driver updating the
>>>      `i2c_msg` `len` field with the number of received bytes. The updated
>>>      length is used when checking the PEC.
>>>
>>> Add support to the Cadence I2C driver for handling SMBus block reads
>>> with PEC. To determine the maximum transfer length uses the initial
>>> `len` value of the `i2c_msg`. When PEC is enabled this will be 2, when
>>> it is disabled it will be 1.
>>>
>>> Once a read transfer is done also increment the `len` field by the
>>> amount of received data bytes.
>>>
>>> This change has been tested with a UCM90320 PMBus power monitor,
>> which
>>> requires block reads to access certain data fields, but also has PEC
>>> enabled by default.
>>>
>>> Fixes: df8eb5691c48 ("i2c: Add driver for Cadence I2C controller")
>>
>> Subject is saying that you adding support for PEC and here you are saying
>> that it is fixing initial commit.
>>
>> If this is adding new support I think Fixes tag shouldn't be here.
>>
>> If it is fixing issue subject should be updated and this Fixes tag kept here.
>>
>> The rest looks good to me.
>>
>> Shubhrajyoti: Can you please test?
> 
> I have tested the reads and write smbus  without packet error check.

Can you please switch it to formal Tested-by: tag?

M

RE: [PATCH] i2c: cadence: Support PEC for SMBus block read

[Datta, Shubhrajyoti]

[AMD Official Use Only - General]



> -----Original Message-----
> From: Simek, Michal <michal.simek@amd.com>
> Sent: Monday, July 18, 2022 6:25 PM
> To: Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>; Lars-Peter Clausen
> <lars@metafoo.de>; Wolfram Sang <wsa@kernel.org>
> Cc: linux-i2c@vger.kernel.org; git <git@xilinx.com>
> Subject: Re: [PATCH] i2c: cadence: Support PEC for SMBus block read
> 
> 
> 
> On 7/18/22 14:37, Datta, Shubhrajyoti wrote:
> > [AMD Official Use Only - General]
> >
> >
> >
> >> -----Original Message-----
> >> From: Simek, Michal <michal.simek@amd.com>
> >> Sent: Monday, July 18, 2022 2:56 PM
> >> To: Lars-Peter Clausen <lars@metafoo.de>; Wolfram Sang
> >> <wsa@kernel.org>; Datta, Shubhrajyoti <shubhrajyoti.datta@amd.com>
> >> Cc: linux-i2c@vger.kernel.org; git <git@xilinx.com>
> >> Subject: Re: [PATCH] i2c: cadence: Support PEC for SMBus block read
> >>
> >>
> >>
> >> On 7/17/22 16:52, Lars-Peter Clausen wrote:
> >>> SMBus packet error checking (PEC) is implemented by appending one
> >>> additional byte of checksum data at the end of the message. This
> >>> provides additional protection and allows to detect data corruption
> >>> on the
> >> I2C bus.
> >>>
> >>> SMBus block reads support variable length reads. The first byte in
> >>> the read message is the number of available data bytes.
> >>>
> >>> The combination of PEC and block read is currently not supported by
> >>> the Cadence I2C driver.
> >>>    * When PEC is enabled the maximum transfer length for block reads
> >>>      increases from 33 to 34 bytes.
> >>>    * The I2C core smbus emulation layer relies on the driver updating the
> >>>      `i2c_msg` `len` field with the number of received bytes. The updated
> >>>      length is used when checking the PEC.
> >>>
> >>> Add support to the Cadence I2C driver for handling SMBus block reads
> >>> with PEC. To determine the maximum transfer length uses the initial
> >>> `len` value of the `i2c_msg`. When PEC is enabled this will be 2,
> >>> when it is disabled it will be 1.
> >>>
> >>> Once a read transfer is done also increment the `len` field by the
> >>> amount of received data bytes.
> >>>
> >>> This change has been tested with a UCM90320 PMBus power monitor,
> >> which
> >>> requires block reads to access certain data fields, but also has PEC
> >>> enabled by default.
> >>>
> >>> Fixes: df8eb5691c48 ("i2c: Add driver for Cadence I2C controller")
> >>
> >> Subject is saying that you adding support for PEC and here you are
> >> saying that it is fixing initial commit.
> >>
> >> If this is adding new support I think Fixes tag shouldn't be here.
> >>
> >> If it is fixing issue subject should be updated and this Fixes tag kept here.
> >>
> >> The rest looks good to me.
> >>
> >> Shubhrajyoti: Can you please test?
> >
> > I have tested the reads and write smbus  without packet error check.
> 
> Can you please switch it to formal Tested-by: tag?
Tested-by:  Shubhrajyoti Datta <Shubhrajyoti.datta@amd.com >

> 
> M

Re: [PATCH] i2c: cadence: Support PEC for SMBus block read

[Lars-Peter Clausen]

On 7/18/22 11:25, Michal Simek wrote:
>
>
> On 7/17/22 16:52, Lars-Peter Clausen wrote:
>> SMBus packet error checking (PEC) is implemented by appending one
>> additional byte of checksum data at the end of the message. This 
>> provides
>> additional protection and allows to detect data corruption on the I2C 
>> bus.
>>
>> SMBus block reads support variable length reads. The first byte in 
>> the read
>> message is the number of available data bytes.
>>
>> The combination of PEC and block read is currently not supported by the
>> Cadence I2C driver.
>>   * When PEC is enabled the maximum transfer length for block reads
>>     increases from 33 to 34 bytes.
>>   * The I2C core smbus emulation layer relies on the driver updating the
>>     `i2c_msg` `len` field with the number of received bytes. The updated
>>     length is used when checking the PEC.
>>
>> Add support to the Cadence I2C driver for handling SMBus block reads 
>> with
>> PEC. To determine the maximum transfer length uses the initial `len` 
>> value
>> of the `i2c_msg`. When PEC is enabled this will be 2, when it is 
>> disabled
>> it will be 1.
>>
>> Once a read transfer is done also increment the `len` field by the 
>> amount
>> of received data bytes.
>>
>> This change has been tested with a UCM90320 PMBus power monitor, which
>> requires block reads to access certain data fields, but also has PEC
>> enabled by default.
>>
>> Fixes: df8eb5691c48 ("i2c: Add driver for Cadence I2C controller")
>
> Subject is saying that you adding support for PEC and here you are 
> saying that it is fixing initial commit.
>
> If this is adding new support I think Fixes tag shouldn't be here.
>
> If it is fixing issue subject should be updated and this Fixes tag 
> kept here.

I added it because I was afraid Wolfram would ask where is the fixes tag.

This change arguably somewhere between new feature and fix. The driver 
reports that it supports SMBus block read, but it does not work under 
the case that PEC is enabled. You can argue that it is a new feature 
because it never worked, but you can also argue it is a fix because the 
current implementation is broken. I'm fine either way with or without 
Fixes tag. I'll let Wolfram make the decision what he prefers.

Re: [PATCH] i2c: cadence: Support PEC for SMBus block read

[Wolfram Sang]

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


> Tested-by:  Shubhrajyoti Datta <Shubhrajyoti.datta@amd.com >

WARNING: Use a single space after Tested-by:
#30: 
Tested-by:  Shubhrajyoti Datta <Shubhrajyoti.datta@amd.com >

ERROR: Unrecognized email address: 'Shubhrajyoti Datta <Shubhrajyoti.datta@amd.com >'
#30: 
Tested-by:  Shubhrajyoti Datta <Shubhrajyoti.datta@amd.com >

Please start using macros. You never got it right so far :(


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] i2c: cadence: Support PEC for SMBus block read

[Wolfram Sang]

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

On Sun, Jul 17, 2022 at 04:52:44PM +0200, Lars-Peter Clausen wrote:
> SMBus packet error checking (PEC) is implemented by appending one
> additional byte of checksum data at the end of the message. This provides
> additional protection and allows to detect data corruption on the I2C bus.
> 
> SMBus block reads support variable length reads. The first byte in the read
> message is the number of available data bytes.
> 
> The combination of PEC and block read is currently not supported by the
> Cadence I2C driver.
>  * When PEC is enabled the maximum transfer length for block reads
>    increases from 33 to 34 bytes.
>  * The I2C core smbus emulation layer relies on the driver updating the
>    `i2c_msg` `len` field with the number of received bytes. The updated
>    length is used when checking the PEC.
> 
> Add support to the Cadence I2C driver for handling SMBus block reads with
> PEC. To determine the maximum transfer length uses the initial `len` value
> of the `i2c_msg`. When PEC is enabled this will be 2, when it is disabled
> it will be 1.
> 
> Once a read transfer is done also increment the `len` field by the amount
> of received data bytes.
> 
> This change has been tested with a UCM90320 PMBus power monitor, which
> requires block reads to access certain data fields, but also has PEC
> enabled by default.
> 
> Fixes: df8eb5691c48 ("i2c: Add driver for Cadence I2C controller")
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

Applied to for-next with the Fixes tag, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]