Re: [PATCH] i2c: mux: pca954x: Disable cacheing of the last channel

[Shubhrajyoti Datta <shubhrajyoti.datta@gmail.com>]

Hi Peter ,
thanks for the review,

On Tue, Nov 19, 2019 at 4:35 AM Peter Rosin <peda@axentia.se> wrote:
>
> On 2019-11-14 09:17, shubhrajyoti.datta@gmail.com wrote:
> > From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> >
> > In case of multimaster configuration the last channel cached value is
> > not reliable. Basically the first processor/master does a write to the
> > mux and then to the intended slave, it caches the value.
> > Now the second processor/processor does a write to mux on another
> > channel and writes to another slave.
> > The first processor/master when it attempts to write the slave
> > skips the mux as it relies on the mux channel being the same as the
> > intended. This causes an issue.
> >
> > To fix that write always to the mux address.
>
> Thanks for your patch.
>
> However, I don't really see how this fixes anything. If you have
> multiple masters competing for the same mux, all bets are off and any
> solution not involving an out-of-band channel where the masters can
> coordinate will be racy, broken and dangerous.
> And since you need that
> extra channel anyway, it might as well also be used to coordinate when
> the cache needs to be invalidated.
>
> At the very least, all limitations needs to be carefully documented,
> but that does not mean that I will ever like it. In short, I'm extremely
> reluctant to add a glgllike this.
>
> Cheers,
> Peter

I agree does the below patch make sense.

From 0ca65420b65514594a8252d1e9eeba64bea01da6 Mon Sep 17 00:00:00 2001
From: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Date: Fri, 30 Aug 2019 11:15:25 +0530
Subject: [PATCH] i2c: mux: pca954x: Disable cacheing of the last channel

In case of multimaster configuration the last channel cached value is
not reliable. To fix that write always to the mux address.
Also use the 0 channel disable to arbitrate. If disabled only then write.
Else wait for it to be disabled.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
---

 .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |  1 +
 drivers/i2c/muxes/i2c-mux-pca954x.c                | 49 +++++++++++++++++++---
 2 files changed, 45 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c
b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 923aa3a..a7e3aa9 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -42,6 +42,7 @@
 #include <linux/i2c-mux.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
+#include <linux/jiffies.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
@@ -55,6 +56,8 @@

 #define PCA954X_IRQ_OFFSET 4

+#define MUX_CHAN_TIMEOUT_US 1000000
+
 enum pca_type {
  pca_9540,
  pca_9542,
@@ -85,6 +88,8 @@ struct pca954x {
  const struct chip_desc *chip;

  u8 last_chan; /* last register value */
+ u8 last_chan_unchached; /* write channel register always */
+ bool channel_sel;
  /* MUX_IDLE_AS_IS, MUX_IDLE_DISCONNECT or >= 0 for channel */
  s8 idle_state;

@@ -229,6 +234,23 @@ static int pca954x_reg_write(struct i2c_adapter *adap,
  I2C_SMBUS_BYTE, &dummy);
 }

+/*
+ * Read from chip register. Don't use i2c_transfer()/i2c_smbus_xfer()
+ * as they will try to lock adapter a second time.
+ */
+static int pca954x_reg_read(struct i2c_adapter *adap,
+     struct i2c_client *client, u8 val)
+{
+ union i2c_smbus_data data;
+ int ret;
+
+ ret = __i2c_smbus_xfer(adap, client->addr, client->flags,
+        I2C_SMBUS_READ, val,
+        I2C_SMBUS_BYTE, &data);
+
+ return ret ? ret :  data.byte;
+}
+
 static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
 {
  struct pca954x *data = i2c_mux_priv(muxc);
@@ -236,6 +258,7 @@ static int pca954x_select_chan(struct i2c_mux_core
*muxc, u32 chan)
  const struct chip_desc *chip = data->chip;
  u8 regval;
  int ret = 0;
+ unsigned long timeout = jiffies + usecs_to_jiffies(MUX_CHAN_TIMEOUT_US);

  /* we make switches look like muxes, not sure how to be smarter */
  if (chip->muxtype == pca954x_ismux)
@@ -243,13 +266,26 @@ static int pca954x_select_chan(struct
i2c_mux_core *muxc, u32 chan)
  else
  regval = 1 << chan;

+ if (!data->last_chan_unchached) {
  /* Only select the channel if its different from the last channel */
- if (data->last_chan != regval) {
- ret = pca954x_reg_write(muxc->parent, client, regval);
- data->last_chan = ret < 0 ? 0 : regval;
+ if (data->last_chan != regval) {
+ ret = pca954x_reg_write(muxc->parent, client, regval);
+ data->last_chan = ret < 0 ? 0 : regval;
+ }
+ return ret;
  }

- return ret;
+ do {
+ ret = pca954x_reg_read(muxc->parent, client, 0);
+ if (ret == 0) {
+ ret = pca954x_reg_write(muxc->parent, client, regval);
+ data->channel_sel = true;
+ return ret;
+ }
+ msleep(20);
+ } while (time_is_after_eq_jiffies(timeout));
+ data->channel_sel = false;
+ return -ETIMEDOUT;
 }

 static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
@@ -259,7 +295,7 @@ static int pca954x_deselect_mux(struct
i2c_mux_core *muxc, u32 chan)
  s8 idle_state;

  idle_state = READ_ONCE(data->idle_state);
- if (idle_state >= 0)
+ if (idle_state >= 0 &&  data->channel_sel)
  /* Set the mux back to a predetermined channel */
  return pca954x_select_chan(muxc, idle_state);

@@ -479,6 +515,9 @@ static int pca954x_probe(struct i2c_client *client,
  if (idle_disconnect_dt)
  data->idle_state = MUX_IDLE_DISCONNECT;

+ data->last_chan_unchached = np &&
+ of_property_read_bool(np, "no-channel-cache");
+
  ret = pca954x_irq_setup(muxc);
  if (ret)
  goto fail_cleanup;
-- 
2.1.1