* [PATCH V1 0/2] add PEC support on slave side
@ 2020-07-16 8:08 Rayagonda Kokatanur
2020-07-16 8:08 ` [PATCH V1 1/2] i2c: add PEC error event Rayagonda Kokatanur
2020-07-16 8:08 ` [PATCH V1 2/2] i2c: iproc: add slave pec support Rayagonda Kokatanur
0 siblings, 2 replies; 6+ messages in thread
From: Rayagonda Kokatanur @ 2020-07-16 8:08 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c, linux-kernel, Ray Jui, Scott Branden,
bcm-kernel-feedback-list, Lori Hikichi, Robert Richter,
Nishka Dasgupta, Andy Shevchenko, linux-arm-kernel
Cc: Rayagonda Kokatanur
This patch set adds support for PEC on Slave side.
Rayagonda Kokatanur (2):
i2c: add PEC error event
i2c: iproc: add slave pec support
drivers/i2c/busses/i2c-bcm-iproc.c | 50 +++++++++++++++++++++++++++---
include/linux/i2c.h | 1 +
2 files changed, 47 insertions(+), 4 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH V1 1/2] i2c: add PEC error event
2020-07-16 8:08 [PATCH V1 0/2] add PEC support on slave side Rayagonda Kokatanur
@ 2020-07-16 8:08 ` Rayagonda Kokatanur
2020-07-16 8:08 ` [PATCH V1 2/2] i2c: iproc: add slave pec support Rayagonda Kokatanur
1 sibling, 0 replies; 6+ messages in thread
From: Rayagonda Kokatanur @ 2020-07-16 8:08 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c, linux-kernel, Ray Jui, Scott Branden,
bcm-kernel-feedback-list, Lori Hikichi, Robert Richter,
Nishka Dasgupta, Andy Shevchenko, linux-arm-kernel
Cc: Rayagonda Kokatanur
Add new event I2C_SLAVE_PEC_ERR to list of slave events.
This event will be used by slave bus driver to indicate
PEC error to slave client or backend driver.
Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
---
include/linux/i2c.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index b8b8963f8bb9..e04acd04eb48 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -370,6 +370,7 @@ enum i2c_slave_event {
I2C_SLAVE_READ_PROCESSED,
I2C_SLAVE_WRITE_RECEIVED,
I2C_SLAVE_STOP,
+ I2C_SLAVE_PEC_ERR,
};
int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH V1 2/2] i2c: iproc: add slave pec support
2020-07-16 8:08 [PATCH V1 0/2] add PEC support on slave side Rayagonda Kokatanur
2020-07-16 8:08 ` [PATCH V1 1/2] i2c: add PEC error event Rayagonda Kokatanur
@ 2020-07-16 8:08 ` Rayagonda Kokatanur
2020-07-16 10:14 ` Andy Shevchenko
1 sibling, 1 reply; 6+ messages in thread
From: Rayagonda Kokatanur @ 2020-07-16 8:08 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c, linux-kernel, Ray Jui, Scott Branden,
bcm-kernel-feedback-list, Lori Hikichi, Robert Richter,
Nishka Dasgupta, Andy Shevchenko, linux-arm-kernel
Cc: Rayagonda Kokatanur
Iproc supports PEC computation and checking in both Master
and Slave mode.
This patch adds support for PEC in slave mode.
Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
---
drivers/i2c/busses/i2c-bcm-iproc.c | 50 +++++++++++++++++++++++++++---
1 file changed, 46 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 8a3c98866fb7..51c8b165bb5e 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -93,6 +93,7 @@
#define S_CMD_STATUS_MASK 0x07
#define S_CMD_STATUS_SUCCESS 0x0
#define S_CMD_STATUS_TIMEOUT 0x5
+#define S_CMD_PEC_SHIFT 8
#define IE_OFFSET 0x38
#define IE_M_RX_FIFO_FULL_SHIFT 31
@@ -138,7 +139,9 @@
#define S_RX_OFFSET 0x4c
#define S_RX_STATUS_SHIFT 30
#define S_RX_STATUS_MASK 0x03
-#define S_RX_PEC_ERR_SHIFT 29
+#define S_RX_PEC_ERR_SHIFT 28
+#define S_RX_PEC_ERR_MASK 0x3
+#define S_RX_PEC_ERR 0x1
#define S_RX_DATA_SHIFT 0
#define S_RX_DATA_MASK 0xff
@@ -205,6 +208,8 @@ struct bcm_iproc_i2c_dev {
/* bytes that have been read */
unsigned int rx_bytes;
unsigned int thld_bytes;
+
+ bool en_s_pec;
};
/*
@@ -321,6 +326,24 @@ static void bcm_iproc_i2c_check_slave_status(
}
}
+static int bcm_iproc_smbus_check_slave_pec(struct bcm_iproc_i2c_dev *iproc_i2c,
+ u32 val)
+{
+ u8 err_status;
+ int ret = 0;
+
+ if (!iproc_i2c->en_s_pec)
+ return ret;
+
+ err_status = (u8)((val >> S_RX_PEC_ERR_SHIFT) & S_RX_PEC_ERR_MASK);
+ if (err_status == S_RX_PEC_ERR) {
+ dev_err(iproc_i2c->device, "Slave PEC error\n");
+ ret = -EBADMSG;
+ }
+
+ return ret;
+}
+
static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
u32 status)
{
@@ -347,6 +370,8 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
iproc_i2c_wr_reg(iproc_i2c, S_TX_OFFSET, value);
val = BIT(S_CMD_START_BUSY_SHIFT);
+ if (iproc_i2c->en_s_pec)
+ val |= BIT(S_CMD_PEC_SHIFT);
iproc_i2c_wr_reg(iproc_i2c, S_CMD_OFFSET, val);
/*
@@ -361,9 +386,19 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
value = (u8)((val >> S_RX_DATA_SHIFT) & S_RX_DATA_MASK);
i2c_slave_event(iproc_i2c->slave,
I2C_SLAVE_WRITE_RECEIVED, &value);
- if (rx_status == I2C_SLAVE_RX_END)
- i2c_slave_event(iproc_i2c->slave,
- I2C_SLAVE_STOP, &value);
+ if (rx_status == I2C_SLAVE_RX_END) {
+ int ret;
+
+ ret = bcm_iproc_smbus_check_slave_pec(iproc_i2c,
+ val);
+ if (!ret)
+ i2c_slave_event(iproc_i2c->slave,
+ I2C_SLAVE_STOP, &value);
+ else
+ i2c_slave_event(iproc_i2c->slave,
+ I2C_SLAVE_PEC_ERR,
+ &value);
+ }
}
} else if (status & BIT(IS_S_TX_UNDERRUN_SHIFT)) {
/* Master read other than start */
@@ -372,6 +407,8 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
iproc_i2c_wr_reg(iproc_i2c, S_TX_OFFSET, value);
val = BIT(S_CMD_START_BUSY_SHIFT);
+ if (iproc_i2c->en_s_pec)
+ val |= BIT(S_CMD_PEC_SHIFT);
iproc_i2c_wr_reg(iproc_i2c, S_CMD_OFFSET, val);
}
@@ -1065,6 +1102,11 @@ static int bcm_iproc_i2c_reg_slave(struct i2c_client *slave)
if (slave->flags & I2C_CLIENT_TEN)
return -EAFNOSUPPORT;
+ /* Enable partial slave HW PEC support if requested by the client */
+ iproc_i2c->en_s_pec = !!(slave->flags & I2C_CLIENT_PEC);
+ if (iproc_i2c->en_s_pec)
+ dev_info(iproc_i2c->device, "Enable PEC\n");
+
iproc_i2c->slave = slave;
bcm_iproc_i2c_slave_init(iproc_i2c, false);
return 0;
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH V1 2/2] i2c: iproc: add slave pec support
2020-07-16 8:08 ` [PATCH V1 2/2] i2c: iproc: add slave pec support Rayagonda Kokatanur
@ 2020-07-16 10:14 ` Andy Shevchenko
2020-07-16 17:19 ` Rayagonda Kokatanur
0 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2020-07-16 10:14 UTC (permalink / raw)
To: Rayagonda Kokatanur
Cc: Wolfram Sang, linux-i2c, Linux Kernel Mailing List, Ray Jui,
Scott Branden, bcm-kernel-feedback-list, Lori Hikichi,
Robert Richter, Nishka Dasgupta, Andy Shevchenko,
linux-arm Mailing List
On Thu, Jul 16, 2020 at 11:14 AM Rayagonda Kokatanur
<rayagonda.kokatanur@broadcom.com> wrote:
>
> Iproc supports PEC computation and checking in both Master
> and Slave mode.
>
> This patch adds support for PEC in slave mode.
...
> -#define S_RX_PEC_ERR_SHIFT 29
> +#define S_RX_PEC_ERR_SHIFT 28
> +#define S_RX_PEC_ERR_MASK 0x3
> +#define S_RX_PEC_ERR 0x1
This needs to be explained in the commit message, in particular why
this change makes no regression.
...
> +static int bcm_iproc_smbus_check_slave_pec(struct bcm_iproc_i2c_dev *iproc_i2c,
> + u32 val)
> +{
> + u8 err_status;
> + int ret = 0;
Completely redundant variable.
> + if (!iproc_i2c->en_s_pec)
> + return ret;
return 0;
> + err_status = (u8)((val >> S_RX_PEC_ERR_SHIFT) & S_RX_PEC_ERR_MASK);
Why casting?
> + if (err_status == S_RX_PEC_ERR) {
> + dev_err(iproc_i2c->device, "Slave PEC error\n");
> + ret = -EBADMSG;
return ...
> + }
> +
> + return ret;
return 0;
> +}
...
> + if (rx_status == I2C_SLAVE_RX_END) {
> + int ret;
> +
> + ret = bcm_iproc_smbus_check_slave_pec(iproc_i2c,
> + val);
One line looks better.
> + if (!ret)
Why not positive conditional?
> + i2c_slave_event(iproc_i2c->slave,
> + I2C_SLAVE_STOP, &value);
> + else
> + i2c_slave_event(iproc_i2c->slave,
> + I2C_SLAVE_PEC_ERR,
> + &value);
> + }
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V1 2/2] i2c: iproc: add slave pec support
2020-07-16 10:14 ` Andy Shevchenko
@ 2020-07-16 17:19 ` Rayagonda Kokatanur
2020-07-16 18:26 ` Andy Shevchenko
0 siblings, 1 reply; 6+ messages in thread
From: Rayagonda Kokatanur @ 2020-07-16 17:19 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Wolfram Sang, linux-i2c, Linux Kernel Mailing List, Ray Jui,
Scott Branden, bcm-kernel-feedback-list, Lori Hikichi,
Robert Richter, Nishka Dasgupta, Andy Shevchenko,
linux-arm Mailing List
Hi Andy,
On Thu, Jul 16, 2020 at 3:44 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Thu, Jul 16, 2020 at 11:14 AM Rayagonda Kokatanur
> <rayagonda.kokatanur@broadcom.com> wrote:
> >
> > Iproc supports PEC computation and checking in both Master
> > and Slave mode.
> >
> > This patch adds support for PEC in slave mode.
>
> ...
>
> > -#define S_RX_PEC_ERR_SHIFT 29
> > +#define S_RX_PEC_ERR_SHIFT 28
> > +#define S_RX_PEC_ERR_MASK 0x3
> > +#define S_RX_PEC_ERR 0x1
>
> This needs to be explained in the commit message, in particular why
> this change makes no regression.
I didn't get what do you mean by "no regression", please elaborate.
>
> ...
>
> > +static int bcm_iproc_smbus_check_slave_pec(struct bcm_iproc_i2c_dev *iproc_i2c,
> > + u32 val)
> > +{
> > + u8 err_status;
>
> > + int ret = 0;
>
> Completely redundant variable.
>
> > + if (!iproc_i2c->en_s_pec)
> > + return ret;
>
> return 0;
>
> > + err_status = (u8)((val >> S_RX_PEC_ERR_SHIFT) & S_RX_PEC_ERR_MASK);
>
> Why casting?
>
> > + if (err_status == S_RX_PEC_ERR) {
> > + dev_err(iproc_i2c->device, "Slave PEC error\n");
>
> > + ret = -EBADMSG;
>
> return ...
>
> > + }
> > +
> > + return ret;
>
> return 0;
>
> > +}
>
> ...
>
> > + if (rx_status == I2C_SLAVE_RX_END) {
> > + int ret;
> > +
> > + ret = bcm_iproc_smbus_check_slave_pec(iproc_i2c,
> > + val);
>
> One line looks better.
Yes, but to have 80 char per line, I have to do this.
>
> > + if (!ret)
>
> Why not positive conditional?
Thank you for your review.
Will fix all above.
Best regards,
Rayagonda
>
> > + i2c_slave_event(iproc_i2c->slave,
> > + I2C_SLAVE_STOP, &value);
> > + else
> > + i2c_slave_event(iproc_i2c->slave,
> > + I2C_SLAVE_PEC_ERR,
> > + &value);
> > + }
>
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH V1 2/2] i2c: iproc: add slave pec support
2020-07-16 17:19 ` Rayagonda Kokatanur
@ 2020-07-16 18:26 ` Andy Shevchenko
0 siblings, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2020-07-16 18:26 UTC (permalink / raw)
To: Rayagonda Kokatanur
Cc: Wolfram Sang, linux-i2c, Linux Kernel Mailing List, Ray Jui,
Scott Branden, bcm-kernel-feedback-list, Lori Hikichi,
Robert Richter, Nishka Dasgupta, linux-arm Mailing List
On Thu, Jul 16, 2020 at 10:49:14PM +0530, Rayagonda Kokatanur wrote:
> On Thu, Jul 16, 2020 at 3:44 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Thu, Jul 16, 2020 at 11:14 AM Rayagonda Kokatanur
> > <rayagonda.kokatanur@broadcom.com> wrote:
...
> > > -#define S_RX_PEC_ERR_SHIFT 29
> > > +#define S_RX_PEC_ERR_SHIFT 28
> > This needs to be explained in the commit message, in particular why
> > this change makes no regression.
>
> I didn't get what do you mean by "no regression", please elaborate.
The definition above has been changed. The point is you have to point out in
the commit message why it's okay and makes no regression.
For example, "..._SHIFT is changed to ... according to documentation. Since
there was no user of it no regression will be made." Provide proper text, b/c
I have no idea what is exactly the reason of the change and if it's indeed used
to have no users.
...
> > > + ret = bcm_iproc_smbus_check_slave_pec(iproc_i2c,
> > > + val);
> >
> > One line looks better.
>
> Yes, but to have 80 char per line, I have to do this.
We have more, but even if you stick with 80 the above is harder to get than if it is one line.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-16 18:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-16 8:08 [PATCH V1 0/2] add PEC support on slave side Rayagonda Kokatanur
2020-07-16 8:08 ` [PATCH V1 1/2] i2c: add PEC error event Rayagonda Kokatanur
2020-07-16 8:08 ` [PATCH V1 2/2] i2c: iproc: add slave pec support Rayagonda Kokatanur
2020-07-16 10:14 ` Andy Shevchenko
2020-07-16 17:19 ` Rayagonda Kokatanur
2020-07-16 18:26 ` Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).