[Skiboot] [PATCH] hw/p8-i2c: Fix OCC locking

[William Kennington]

I've applied locally and it looks like this reduces the number of i2c
conflicts we have with the occ but it's not perfect and we fail to read
some of the dimm spds.

I've noticed lots of these type messages, which appear to mostly be useful
for information but shouldn't be at error or warning level:
[ 1742.429554610,3] I2C: c0e3: Master in use by OCC
[ 1742.446787039,3] I2C: c0e3: Master in use by OCC
[ 1742.446849435,3] I2C: c0e3: Master in use by OCC
[ 1742.459099356,3] I2C: c0e3: Master in use by OCC
[ 1742.459146931,3] I2C: c0e3: Master in use by OCC
[ 1742.490614096,3] I2C: c0e3: Master in use by OCC
[ 1742.490682664,3] I2C: c0e3: Master in use by OCC

I noticed that in the cases where we have broken spd data the skiboot
message buffer contains the following interlaced with the above messages:
[ 1743.471148533,3] I2C: Request complete with residual data req=1 done=2

It appears that we get 2 of those extra data messages when we get bad SPD
data on the userspace side.

On Wed, Aug 23, 2017 at 7:11 AM Oliver O'Halloran <oohall at gmail.com> wrote:

> There's a few issues with the Host<->OCC I2C bus handshaking. First up,
> skiboot is currently examining the wrong bit when checking if the OCC
> is currently using the bus. Secondly, when we need to wait for the OCC
> to release the bus we are scheduling a recovery timer to run zero
> timebase ticks after the current moment so the next poller run will
> always time out the current request. The is also an issue with the
> order of operations in the recovery timeout handler which can cause
> an assertion failure when the cause of the recovery is a timeout
> waiting for the OCC.
>
> This patch addresses all these issues and sets the recovery timeout to
> 10ms.
>
> Signed-off-by: Oliver O'Halloran <oohall at gmail.com>
> ---
>  hw/p8-i2c.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/hw/p8-i2c.c b/hw/p8-i2c.c
> index f4666e24953e..69d0984e65cb 100644
> --- a/hw/p8-i2c.c
> +++ b/hw/p8-i2c.c
> @@ -1063,8 +1063,11 @@ static int occ_i2c_lock(struct p8_i2c_master
> *master)
>
>         busflag = PPC_BIT(16 + (master->engine_id - 1) * 2);
>
> -       DBG("occflags = %llx (locks = %.6llx)\n", (u64)occflags,
> -           GETFIELD(PPC_BITMASK(16, 22), occflags));
> +       DBG("I2C: c%de%d: occflags = %llx (locks = %x:%x:%x)\n",
> +               master->chip_id, master->engine_id, (u64) occflags,
> +               (u32) GETFIELD(PPC_BITMASK(16, 17), occflags),
> +               (u32) GETFIELD(PPC_BITMASK(18, 19), occflags),
> +               (u32) GETFIELD(PPC_BITMASK(20, 21), occflags));
>
>         rc = xscom_write(master->chip_id, OCCFLG_SET, busflag);
>         if (rc) {
> @@ -1073,8 +1076,11 @@ static int occ_i2c_lock(struct p8_i2c_master
> *master)
>         }
>
>         /* If the OCC also has this bus locked then wait for IRQ */
> -       if (occflags & (busflag << 1))
> +       if (occflags & (busflag >> 1)) {
> +               prerror("I2C: c%de%d: Master in use by OCC\n",
> +                       master->chip_id, master->engine_id);
>                 return 1;
> +       }
>
>         master->occ_lock_acquired = true;
>
> @@ -1161,7 +1167,7 @@ static int p8_i2c_start_request(struct p8_i2c_master
> *master,
>         if (rc > 0) {
>                 /* Wait for OCC IRQ */
>                 master->state = state_occache_dis;
> -               schedule_timer(&master->recovery, rc);
> +               schedule_timer(&master->recovery, msecs_to_tb(10));
>                 return 0;
>         }
>
> @@ -1298,11 +1304,13 @@ void p9_i2c_bus_owner_change(u32 chip_id)
>                         continue;
>                 }
>
> -               /* Run the state machine */
> -               p8_i2c_check_status(master);
> +               /* clear the existing wait timer */
> +               cancel_timer(&master->recovery);
>
> -               /* Check for new work */
> +               /* start the request now that we own the master */
> +               master->state = state_idle;
>                 p8_i2c_check_work(master);
> +               p8_i2c_check_status(master);
>
>                 unlock(&master->lock);
>         }
> --
> 2.9.5
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/skiboot/attachments/20170823/31841c4c/attachment-0001.html>