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

[Tali Perry <tali.perry1@gmail.com>]

> 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