diff mbox series

[v5,08/10] i2c: npcm: Remove own slave addresses 2:10

Message ID 20220517101142.28421-9-warp5tw@gmail.com
State New
Headers show
Series i2c: npcm: Bug fixes timeout, spurious interrupts | expand

Commit Message

Tyrone Ting May 17, 2022, 10:11 a.m. UTC 
From: Tali Perry <tali.perry1@gmail.com>

NPCM can support up to 10 own slave addresses. In practice, only one
address is actually being used. In order to access addresses 2 and above,
need to switch register banks. The switch needs spinlock.
To avoid using spinlock for this useless feature removed support of SA >=
2. Also fix returned slave event enum.

Remove some comment since the bank selection is not required. The bank
selection is not required since the supported slave addresses are reduced.

Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by: Tali Perry <tali.perry1@gmail.com>
Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 43 +++++++++++++++-----------------
 1 file changed, 20 insertions(+), 23 deletions(-)

Comments

Wolfram Sang May 21, 2022, 5:58 a.m. UTC | #1 
> NPCM can support up to 10 own slave addresses. In practice, only one
> address is actually being used. In order to access addresses 2 and above,
> need to switch register banks. The switch needs spinlock.
> To avoid using spinlock for this useless feature removed support of SA >=
> 2. Also fix returned slave event enum.

Is the spinlock contention so high? The code paths do not really look
like hot paths to me. A bit sad to see this feature go.

>  static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
>  	NPCM_I2CADDR1, NPCM_I2CADDR2, NPCM_I2CADDR3, NPCM_I2CADDR4,
>  	NPCM_I2CADDR5, NPCM_I2CADDR6, NPCM_I2CADDR7, NPCM_I2CADDR8,

Why do we keep this array if we drop the support?

> @@ -604,8 +602,7 @@ static int npcm_i2c_slave_enable(struct npcm_i2c *bus, enum i2c_addr addr_type,
>  			i2cctl1 &= ~NPCM_I2CCTL1_GCMEN;
>  		iowrite8(i2cctl1, bus->reg + NPCM_I2CCTL1);
>  		return 0;
> -	}
> -	if (addr_type == I2C_ARP_ADDR) {
> +	} else if (addr_type == I2C_ARP_ADDR) {

I might be wrong but this looks like a seperate change?

> @@ -924,11 +918,15 @@ static int npcm_i2c_slave_get_wr_buf(struct npcm_i2c *bus)
>  	for (i = 0; i < I2C_HW_FIFO_SIZE; i++) {
>  		if (bus->slv_wr_size >= I2C_HW_FIFO_SIZE)
>  			break;
> -		i2c_slave_event(bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
> +		if (bus->state == I2C_SLAVE_MATCH) {
> +			i2c_slave_event(bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
> +			bus->state = I2C_OPER_STARTED;
> +		} else {
> +			i2c_slave_event(bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
> +		}
>  		ind = (bus->slv_wr_ind + bus->slv_wr_size) % I2C_HW_FIFO_SIZE;
>  		bus->slv_wr_buf[ind] = value;
>  		bus->slv_wr_size++;
> -		i2c_slave_event(bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
>  	}
>  	return I2C_HW_FIFO_SIZE - ret;
>  }
> @@ -976,7 +974,6 @@ static void npcm_i2c_slave_xmit(struct npcm_i2c *bus, u16 nwrite,
>  	if (nwrite == 0)
>  		return;
>  
> -	bus->state = I2C_OPER_STARTED;
>  	bus->operation = I2C_WRITE_OPER;

This is definately a seperate change!

All the best!
Tali Perry May 22, 2022, 6:56 a.m. UTC | #2 
> On Sat, May 21, 2022 at 8:58 AM Wolfram Sang <wsa@kernel.org> wrote:
>
> > NPCM can support up to 10 own slave addresses. In practice, only one
> > address is actually being used. In order to access addresses 2 and above,
> > need to switch register banks. The switch needs spinlock.
> > To avoid using spinlock for this useless feature removed support of SA >=
> > 2. Also fix returned slave event enum.
>
> Is the spinlock contention so high? The code paths do not really look
> like hot paths to me. A bit sad to see this feature go.
>

The module has two seperate banks, accessible with a bit change. The
first one is used
for most of the runtime operations. Second bank is used mostly during init.
Unfortunetly, the first two own slave addresses are in the first bank
and the other
8 are in the second bank.

Every time the module switchs from master to slave mode, those
registers are accessed.
In theory, a spinlock can be used to protect those registers.
In practice, none of our customers use the extra addresses.
In fact they only need one.

The driver also does not allow you to register more then one slave per bus,
so this HW feature was not fully available to begin with.

So when we encounter a deadlock with this spinlock we decided to get rid of this
unused feature and get both a stable fix for the issue + performance benefits.
We work closely with all our customers so we know that this HW
feature is useless to them.

> >  static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
> >       NPCM_I2CADDR1, NPCM_I2CADDR2, NPCM_I2CADDR3, NPCM_I2CADDR4,
> >       NPCM_I2CADDR5, NPCM_I2CADDR6, NPCM_I2CADDR7, NPCM_I2CADDR8,
>
> Why do we keep this array if we drop the support?
>
This array represents the HW so we left it as-is. But I agree it can
be shortened to one\two.

> > @@ -604,8 +602,7 @@ static int npcm_i2c_slave_enable(struct npcm_i2c *bus, enum i2c_addr addr_type,
> >                       i2cctl1 &= ~NPCM_I2CCTL1_GCMEN;
> >               iowrite8(i2cctl1, bus->reg + NPCM_I2CCTL1);
> >               return 0;
> > -     }
> > -     if (addr_type == I2C_ARP_ADDR) {
> > +     } else if (addr_type == I2C_ARP_ADDR) {
>

addr_type type can be one of several options.
The code was
if (addr_type is 1st option)
...
if (addr_type is 2st option)
...
etc.

Adding that else is more accurate, but ommiting this change works as well.

> I might be wrong but this looks like a seperate change?
>
> > @@ -924,11 +918,15 @@ static int npcm_i2c_slave_get_wr_buf(struct npcm_i2c *bus)
> >       for (i = 0; i < I2C_HW_FIFO_SIZE; i++) {
> >               if (bus->slv_wr_size >= I2C_HW_FIFO_SIZE)
> >                       break;
> > -             i2c_slave_event(bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
> > +             if (bus->state == I2C_SLAVE_MATCH) {
> > +                     i2c_slave_event(bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
> > +                     bus->state = I2C_OPER_STARTED;
> > +             } else {
> > +                     i2c_slave_event(bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
> > +             }
> >               ind = (bus->slv_wr_ind + bus->slv_wr_size) % I2C_HW_FIFO_SIZE;
> >               bus->slv_wr_buf[ind] = value;
> >               bus->slv_wr_size++;
> > -             i2c_slave_event(bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
> >       }
> >       return I2C_HW_FIFO_SIZE - ret;
> >  }
> > @@ -976,7 +974,6 @@ static void npcm_i2c_slave_xmit(struct npcm_i2c *bus, u16 nwrite,
> >       if (nwrite == 0)
> >               return;
> >
> > -     bus->state = I2C_OPER_STARTED;
> >       bus->operation = I2C_WRITE_OPER;
>
> This is definately a seperate change!
>

OK, we will move the last two to a separate patch. BTW, this change
appears in the title as well.

But now I'm not sure: if you already apply for-next patches [1:7], and
we change patch [8:10]
do we need to re-submit [1:7]?

> All the best!

Thanks, Wolfram, for your review!
Much appreciated

Tali
Wolfram Sang May 22, 2022, 8:09 p.m. UTC | #3 
Hi Tali,

> So when we encounter a deadlock with this spinlock we decided to get rid of this
> unused feature and get both a stable fix for the issue + performance benefits.
> We work closely with all our customers so we know that this HW
> feature is useless to them.

Okay, fair enough. Thanks for the detailed explanation!

> > Why do we keep this array if we drop the support?
> >
> This array represents the HW so we left it as-is. But I agree it can
> be shortened to one\two.

Would be nice, I think.

> OK, we will move the last two to a separate patch. BTW, this change
> appears in the title as well.

Yes, but I still think it should be a seperate change.

> But now I'm not sure: if you already apply for-next patches [1:7], and
> we change patch [8:10]
> do we need to re-submit [1:7]?

Nope, they are already in linux-next. They seemed like good fixes even
without the support for the new SoC, so I applied them right away. I
hope this was okay.

> Thanks, Wolfram, for your review!
> Much appreciated

You are welcome :)

Happy hacking,

   Wolfram
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 5960ccde6574..3f86895f5e64 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -124,6 +124,8 @@  enum i2c_addr {
  * use this array to get the address or each register.
  */
 #define I2C_NUM_OWN_ADDR 10
+#define I2C_NUM_OWN_ADDR_SUPPORTED 2
+
 static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
 	NPCM_I2CADDR1, NPCM_I2CADDR2, NPCM_I2CADDR3, NPCM_I2CADDR4,
 	NPCM_I2CADDR5, NPCM_I2CADDR6, NPCM_I2CADDR7, NPCM_I2CADDR8,
@@ -392,14 +394,10 @@  static void npcm_i2c_disable(struct npcm_i2c *bus)
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 	int i;
 
-	/* select bank 0 for I2C addresses */
-	npcm_i2c_select_bank(bus, I2C_BANK_0);
-
 	/* Slave addresses removal */
-	for (i = I2C_SLAVE_ADDR1; i < I2C_NUM_OWN_ADDR; i++)
+	for (i = I2C_SLAVE_ADDR1; i < I2C_NUM_OWN_ADDR_SUPPORTED; i++)
 		iowrite8(0, bus->reg + npcm_i2caddr[i]);
 
-	npcm_i2c_select_bank(bus, I2C_BANK_1);
 #endif
 	/* Disable module */
 	i2cctl2 = ioread8(bus->reg + NPCM_I2CCTL2);
@@ -604,8 +602,7 @@  static int npcm_i2c_slave_enable(struct npcm_i2c *bus, enum i2c_addr addr_type,
 			i2cctl1 &= ~NPCM_I2CCTL1_GCMEN;
 		iowrite8(i2cctl1, bus->reg + NPCM_I2CCTL1);
 		return 0;
-	}
-	if (addr_type == I2C_ARP_ADDR) {
+	} else if (addr_type == I2C_ARP_ADDR) {
 		i2cctl3 = ioread8(bus->reg + NPCM_I2CCTL3);
 		if (enable)
 			i2cctl3 |= I2CCTL3_ARPMEN;
@@ -614,16 +611,17 @@  static int npcm_i2c_slave_enable(struct npcm_i2c *bus, enum i2c_addr addr_type,
 		iowrite8(i2cctl3, bus->reg + NPCM_I2CCTL3);
 		return 0;
 	}
+	if (addr_type > I2C_SLAVE_ADDR2 && addr_type <= I2C_SLAVE_ADDR10)
+		dev_err(bus->dev, "try to enable more than 2 SA not supported\n");
+
 	if (addr_type >= I2C_ARP_ADDR)
 		return -EFAULT;
 	/* select bank 0 for address 3 to 10 */
-	if (addr_type > I2C_SLAVE_ADDR2)
-		npcm_i2c_select_bank(bus, I2C_BANK_0);
+
 	/* Set and enable the address */
 	iowrite8(sa_reg, bus->reg + npcm_i2caddr[addr_type]);
 	npcm_i2c_slave_int_enable(bus, enable);
-	if (addr_type > I2C_SLAVE_ADDR2)
-		npcm_i2c_select_bank(bus, I2C_BANK_1);
+
 	return 0;
 }
 #endif
@@ -846,15 +844,11 @@  static u8 npcm_i2c_get_slave_addr(struct npcm_i2c *bus, enum i2c_addr addr_type)
 {
 	u8 slave_add;
 
-	/* select bank 0 for address 3 to 10 */
-	if (addr_type > I2C_SLAVE_ADDR2)
-		npcm_i2c_select_bank(bus, I2C_BANK_0);
+	if (addr_type > I2C_SLAVE_ADDR2 && addr_type <= I2C_SLAVE_ADDR10)
+		dev_err(bus->dev, "get slave: try to use more than 2 SA not supported\n");
 
 	slave_add = ioread8(bus->reg + npcm_i2caddr[(int)addr_type]);
 
-	if (addr_type > I2C_SLAVE_ADDR2)
-		npcm_i2c_select_bank(bus, I2C_BANK_1);
-
 	return slave_add;
 }
 
@@ -864,12 +858,12 @@  static int npcm_i2c_remove_slave_addr(struct npcm_i2c *bus, u8 slave_add)
 
 	/* Set the enable bit */
 	slave_add |= 0x80;
-	npcm_i2c_select_bank(bus, I2C_BANK_0);
-	for (i = I2C_SLAVE_ADDR1; i < I2C_NUM_OWN_ADDR; i++) {
+
+	for (i = I2C_SLAVE_ADDR1; i < I2C_NUM_OWN_ADDR_SUPPORTED; i++) {
 		if (ioread8(bus->reg + npcm_i2caddr[i]) == slave_add)
 			iowrite8(0, bus->reg + npcm_i2caddr[i]);
 	}
-	npcm_i2c_select_bank(bus, I2C_BANK_1);
+
 	return 0;
 }
 
@@ -924,11 +918,15 @@  static int npcm_i2c_slave_get_wr_buf(struct npcm_i2c *bus)
 	for (i = 0; i < I2C_HW_FIFO_SIZE; i++) {
 		if (bus->slv_wr_size >= I2C_HW_FIFO_SIZE)
 			break;
-		i2c_slave_event(bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
+		if (bus->state == I2C_SLAVE_MATCH) {
+			i2c_slave_event(bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
+			bus->state = I2C_OPER_STARTED;
+		} else {
+			i2c_slave_event(bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
+		}
 		ind = (bus->slv_wr_ind + bus->slv_wr_size) % I2C_HW_FIFO_SIZE;
 		bus->slv_wr_buf[ind] = value;
 		bus->slv_wr_size++;
-		i2c_slave_event(bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
 	}
 	return I2C_HW_FIFO_SIZE - ret;
 }
@@ -976,7 +974,6 @@  static void npcm_i2c_slave_xmit(struct npcm_i2c *bus, u16 nwrite,
 	if (nwrite == 0)
 		return;
 
-	bus->state = I2C_OPER_STARTED;
 	bus->operation = I2C_WRITE_OPER;
 
 	/* get the next buffer */