[PATCH v5 0/5] i2c: mux: pca954x: Add interrupt controller support

[PATCH v5 0/5] i2c: mux: pca954x: Add interrupt controller support

[Phil Reid]

Various muxes can aggregate multiple interrupts from each i2c bus.
All of the muxes with interrupt support combine the active low irq lines
using an internal 'and' function and generate a combined active low
output. The muxes do provide the ability to read a control register to
determine which irq is active. By making the mux an irq controller isr
latenct can potentially be reduced by reading the status register and 
then only calling the registered isr on that bus segment.

In addition an additional enable mask is added to work around devices
that assert irq immediately before being setup by disabling the irq
from the mux until all devices are registered.

Changes from v4:
- p4: Change definition of irq_mask_enable to an array.
- p4: Removed acks due to change requested by Peter
- p5: Parse array of enables. Currently only supports 1 chip
      But dt specification will allow expansion to handle
      multple irq consume chips to be registered on a bus segment
- p5: Fix up logic related to enabling and disable irq's.
      Use a flag to indicate when irq has been enabled.

Changes from v3:
- p3: Add spin lock to irq mask / unmask.
- p4: Add Rob's ack.

Changes from v2:
- p1: Added Acked-by
- p5: fixup 2 typos

Changes from v1:
- Update for new ACPI table
- Fix typo in documentation
- Fix typo in function names
- Fix typo in irq name
- Added spaces around '+' / '='
- Change goto label names
- Change property name from i2c-mux-irq-mask-en to nxp,irq-mask-enable
- Change variable name irq_mask_en to irq_mask_enable
- Add commentt about irq_mask_enable
- Added Acked-By's

Phil Reid (5):
  i2c: mux: pca954x: Add missing pca9542 definition to chip_desc
  dt: bindings: i2c-mux-pca954x: Add documentation for interrupt
    controller
  i2c: mux: pca954x: Add interrupt controller support
  dt: bindings: i2c-mux-pca954x: Add documentation for
    nxp,irq-mask-enable
  i2c: mux: pca954x: Add irq-mask-enable to delay enabling irqs

 .../devicetree/bindings/i2c/i2c-mux-pca954x.txt    |  17 +-
 drivers/i2c/muxes/i2c-mux-pca954x.c                | 177 ++++++++++++++++++++-
 2 files changed, 189 insertions(+), 5 deletions(-)

-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 1/5] i2c: mux: pca954x: Add missing pca9542 definition to chip_desc

[Phil Reid]

The spec for the pca954x was missing. This chip is the same as the pca9540
except that it has interrupt lines. While the i2c_device_id table mapped
the pca9542 to the pca9540 definition the compatible table did not. In
preparation for irq support add the pca9542 definition.

Acked-by: Peter Rosin <peda@axentia.se>
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index dd18b9c..bbf088e 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -84,6 +84,11 @@ struct pca954x {
 		.enable = 0x4,
 		.muxtype = pca954x_ismux,
 	},
+	[pca_9542] = {
+		.nchans = 2,
+		.enable = 0x4,
+		.muxtype = pca954x_ismux,
+	},
 	[pca_9543] = {
 		.nchans = 2,
 		.muxtype = pca954x_isswi,
@@ -110,7 +115,7 @@ struct pca954x {
 
 static const struct i2c_device_id pca954x_id[] = {
 	{ "pca9540", pca_9540 },
-	{ "pca9542", pca_9540 },
+	{ "pca9542", pca_9542 },
 	{ "pca9543", pca_9543 },
 	{ "pca9544", pca_9544 },
 	{ "pca9545", pca_9545 },
@@ -124,7 +129,7 @@ struct pca954x {
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id pca954x_acpi_ids[] = {
 	{ .id = "PCA9540", .driver_data = pca_9540 },
-	{ .id = "PCA9542", .driver_data = pca_9540 },
+	{ .id = "PCA9542", .driver_data = pca_9542 },
 	{ .id = "PCA9543", .driver_data = pca_9543 },
 	{ .id = "PCA9544", .driver_data = pca_9544 },
 	{ .id = "PCA9545", .driver_data = pca_9545 },
-- 
1.8.3.1

[PATCH v5 2/5] dt: bindings: i2c-mux-pca954x: Add documentation for interrupt controller

[Phil Reid]

Various muxes can aggregate multiple irq lines and provide a control
register to determine the active line. Add bindings for interrupt
controller support.

Acked-by: Peter Rosin <peda@axentia.se>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index cf53d5f..aa09704 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -19,7 +19,14 @@ Optional Properties:
   - i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all
     children in idle state. This is necessary for example, if there are several
     multiplexers on the bus and the devices behind them use same I2C addresses.
-
+  - interrupt-parent: Phandle for the interrupt controller that services
+    interrupts for this device.
+  - interrupts: Interrupt mapping for IRQ.
+  - interrupt-controller: Marks the device node as an interrupt controller.
+  - #interrupt-cells : Should be two.
+    - first cell is the pin number
+    - second cell is used to specify flags.
+    See also Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
 
 Example:
 
@@ -29,6 +36,11 @@ Example:
 		#size-cells = <0>;
 		reg = <0x74>;
 
+		interrupt-parent = <&ipic>;
+		interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+
 		i2c@2 {
 			#address-cells = <1>;
 			#size-cells = <0>;
-- 
1.8.3.1

[PATCH v5 3/5] i2c: mux: pca954x: Add interrupt controller support

[Phil Reid]

Various muxes can aggregate multiple interrupts from each i2c bus.
All of the muxes with interrupt support combine the active low irq lines
using an internal 'and' function and generate a combined active low
output. The muxes do provide the ability to read a control register to
determine which irq is active. By making the mux an irq controller isr
latency can potentially be reduced by reading the status register and
then only calling the registered isr on that bus segment.

As there is no irq masking on the mux irq are disabled until irq_unmask is
called at least once.

Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 141 +++++++++++++++++++++++++++++++++++-
 1 file changed, 139 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index bbf088e..f55da88 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -41,14 +41,20 @@
 #include <linux/i2c.h>
 #include <linux/i2c-mux.h>
 #include <linux/i2c/pca954x.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/of_irq.h>
 #include <linux/pm.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 
 #define PCA954X_MAX_NCHANS 8
 
+#define PCA954X_IRQ_OFFSET 4
+
 enum pca_type {
 	pca_9540,
 	pca_9542,
@@ -63,6 +69,7 @@ enum pca_type {
 struct chip_desc {
 	u8 nchans;
 	u8 enable;	/* used for muxes only */
+	u8 has_irq;
 	enum muxtype {
 		pca954x_ismux = 0,
 		pca954x_isswi
@@ -75,6 +82,10 @@ struct pca954x {
 	u8 last_chan;		/* last register value */
 	u8 deselect;
 	struct i2c_client *client;
+
+	struct irq_domain *irq;
+	unsigned int irq_mask;
+	spinlock_t lock;
 };
 
 /* Provide specs for the PCA954x types we know about */
@@ -87,19 +98,23 @@ struct pca954x {
 	[pca_9542] = {
 		.nchans = 2,
 		.enable = 0x4,
+		.has_irq = 1,
 		.muxtype = pca954x_ismux,
 	},
 	[pca_9543] = {
 		.nchans = 2,
+		.has_irq = 1,
 		.muxtype = pca954x_isswi,
 	},
 	[pca_9544] = {
 		.nchans = 4,
 		.enable = 0x4,
+		.has_irq = 1,
 		.muxtype = pca954x_ismux,
 	},
 	[pca_9545] = {
 		.nchans = 4,
+		.has_irq = 1,
 		.muxtype = pca954x_isswi,
 	},
 	[pca_9547] = {
@@ -222,6 +237,114 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
 	return pca954x_reg_write(muxc->parent, client, data->last_chan);
 }
 
+static irqreturn_t pca954x_irq_handler(int irq, void *dev_id)
+{
+	struct pca954x *data = dev_id;
+	unsigned int child_irq;
+	int ret, i, handled;
+
+	ret = i2c_smbus_read_byte(data->client);
+	if (ret < 0)
+		return IRQ_NONE;
+
+	for (i = 0; i < data->chip->nchans; i++) {
+		if (ret & BIT(PCA954X_IRQ_OFFSET + i)) {
+			child_irq = irq_linear_revmap(data->irq, i);
+			handle_nested_irq(child_irq);
+			handled++;
+		}
+	}
+	return handled ? IRQ_HANDLED : IRQ_NONE;
+}
+
+static void pca954x_irq_mask(struct irq_data *idata)
+{
+	struct pca954x *data = irq_data_get_irq_chip_data(idata);
+	unsigned int pos = idata->hwirq;
+	unsigned long flags;
+
+	spin_lock_irqsave(&data->lock, flags);
+
+	data->irq_mask &= ~BIT(pos);
+	if (!data->irq_mask)
+		disable_irq(data->client->irq);
+
+	spin_unlock_irqrestore(&data->lock, flags);
+}
+
+static void pca954x_irq_unmask(struct irq_data *idata)
+{
+	struct pca954x *data = irq_data_get_irq_chip_data(idata);
+	unsigned int pos = idata->hwirq;
+	unsigned long flags;
+
+	spin_lock_irqsave(&data->lock, flags);
+
+	if (!data->irq_mask)
+		enable_irq(data->client->irq);
+	data->irq_mask |= BIT(pos);
+
+	spin_unlock_irqrestore(&data->lock, flags);
+}
+
+static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
+{
+	if ((type & IRQ_TYPE_SENSE_MASK) != IRQ_TYPE_LEVEL_LOW)
+		return -EINVAL;
+	return 0;
+}
+
+static struct irq_chip pca954x_irq_chip = {
+	.name = "i2c-mux-pca954x",
+	.irq_mask = pca954x_irq_mask,
+	.irq_unmask = pca954x_irq_unmask,
+	.irq_set_type = pca954x_irq_set_type,
+};
+
+static int pca954x_irq_setup(struct i2c_mux_core *muxc)
+{
+	struct pca954x *data = i2c_mux_priv(muxc);
+	struct i2c_client *client = data->client;
+	int c, err, irq;
+
+	if (!data->chip->has_irq || client->irq <= 0)
+		return 0;
+
+	spin_lock_init(&data->lock);
+
+	data->irq = irq_domain_add_linear(client->dev.of_node,
+					  data->chip->nchans,
+					  &irq_domain_simple_ops, data);
+	if (!data->irq)
+		return -ENODEV;
+
+	for (c = 0; c < data->chip->nchans; c++) {
+		irq = irq_create_mapping(data->irq, c);
+		irq_set_chip_data(irq, data);
+		irq_set_chip_and_handler(irq, &pca954x_irq_chip,
+			handle_simple_irq);
+	}
+
+	err = devm_request_threaded_irq(&client->dev, data->client->irq, NULL,
+					pca954x_irq_handler,
+					IRQF_ONESHOT | IRQF_SHARED,
+					"pca954x", data);
+	if (err)
+		goto err_req_irq;
+
+	disable_irq(data->client->irq);
+
+	return 0;
+err_req_irq:
+	for (c = 0; c < data->chip->nchans; c++) {
+		irq = irq_find_mapping(data->irq, c);
+		irq_dispose_mapping(irq);
+	}
+	irq_domain_remove(data->irq);
+
+	return err;
+}
+
 /*
  * I2C init/probing/exit functions
  */
@@ -286,6 +409,10 @@ static int pca954x_probe(struct i2c_client *client,
 	idle_disconnect_dt = of_node &&
 		of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
 
+	ret = pca954x_irq_setup(muxc);
+	if (ret)
+		goto fail_del_adapters;
+
 	/* Now create an adapter for each channel */
 	for (num = 0; num < data->chip->nchans; num++) {
 		bool idle_disconnect_pd = false;
@@ -311,7 +438,7 @@ static int pca954x_probe(struct i2c_client *client,
 			dev_err(&client->dev,
 				"failed to register multiplexed adapter"
 				" %d as bus %d\n", num, force);
-			goto virt_reg_failed;
+			goto fail_del_adapters;
 		}
 	}
 
@@ -322,7 +449,7 @@ static int pca954x_probe(struct i2c_client *client,
 
 	return 0;
 
-virt_reg_failed:
+fail_del_adapters:
 	i2c_mux_del_adapters(muxc);
 	return ret;
 }
@@ -330,6 +457,16 @@ static int pca954x_probe(struct i2c_client *client,
 static int pca954x_remove(struct i2c_client *client)
 {
 	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
+	struct pca954x *data = i2c_mux_priv(muxc);
+	int c, irq;
+
+	if (data->irq) {
+		for (c = 0; c < data->chip->nchans; c++) {
+			irq = irq_find_mapping(data->irq, c);
+			irq_dispose_mapping(irq);
+		}
+		irq_domain_remove(data->irq);
+	}
 
 	i2c_mux_del_adapters(muxc);
 	return 0;
-- 
1.8.3.1

[PATCH v5 4/5] dt: bindings: i2c-mux-pca954x: Add documentation for nxp,irq-mask-enable

[Phil Reid]

Unfortunately some hardware device will assert their irq line immediately
on power on and provide no mechanism to mask the irq. As the i2c muxes
provide no method to mask irq line this provides a work around by keeping
the parent irq masked until enough device drivers have loaded to service
all pending interrupts.

For example the the ltc1760 assert its SMBALERT irq immediately on power
on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
device is registered irq are enabled and fire continuously as the second
device driver has not yet loaded. Setting this parameter to <1 1> will
delay the irq being enabled until both devices are ready.

Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
---
 Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index aa09704..ac71be6 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -19,6 +19,8 @@ Optional Properties:
   - i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all
     children in idle state. This is necessary for example, if there are several
     multiplexers on the bus and the devices behind them use same I2C addresses.
+  - nxp,irq-mask-enable: array; Defines the minimum number of chips that must
+    register an irq for each channel before the parent irq line in enabled.
   - interrupt-parent: Phandle for the interrupt controller that services
     interrupts for this device.
   - interrupts: Interrupt mapping for IRQ.
@@ -36,6 +38,7 @@ Example:
 		#size-cells = <0>;
 		reg = <0x74>;
 
+		nxp,irq-mask-enable = <0 0 0 0 1 0 0 0>;
 		interrupt-parent = <&ipic>;
 		interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
 		interrupt-controller;
-- 
1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5 5/5] i2c: mux: pca954x: Add irq-mask-enable to delay enabling irqs

[Phil Reid]

Unfortunately some hardware device will assert their irq line immediately
on power on and provide no mechanism to mask the irq. As the i2c muxes
provide no method to mask irq line this provides a work around by keeping
the parent irq masked until enough device drivers have loaded to service
all pending interrupts.

For example the the ltc1760 assert its SMBALERT irq immediately on power
on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
device is registered irq are enabled and fire continuously as the second
device driver has not yet loaded. Setting this parameter to <1 1> will
delay the irq being enabled until both devices are ready.

Signed-off-by: Phil Reid <preid@electromag.com.au>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index f55da88..012b2ef 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -76,6 +76,19 @@ struct chip_desc {
 	} muxtype;
 };
 
+/*
+ * irq_mask_enable: Provides a mechanism to work around hardware that asserts
+ * their irq immediately on power on. It allows the enabling of the irq to be
+ * delayed until the corresponding bits in the the irq_mask are set thru
+ * irq_unmask.
+ * For example the ltc1760 assert its SMBALERT irq immediately on power on.
+ * With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
+ * device is registered irq are enabled and fire continuously as the second
+ * device driver has not yet loaded. Setting this parameter to 0x3 while
+ * delay the irq being enabled until both devices are ready.
+ * This workaround will not work if two devices share an interrupt on the
+ * same bus segment.
+ */
 struct pca954x {
 	const struct chip_desc *chip;
 
@@ -84,7 +97,9 @@ struct pca954x {
 	struct i2c_client *client;
 
 	struct irq_domain *irq;
+	unsigned int irq_mask_enable;
 	unsigned int irq_mask;
+	bool irq_enabled;
 	spinlock_t lock;
 };
 
@@ -266,8 +281,10 @@ static void pca954x_irq_mask(struct irq_data *idata)
 	spin_lock_irqsave(&data->lock, flags);
 
 	data->irq_mask &= ~BIT(pos);
-	if (!data->irq_mask)
+	if (data->irq_enabled && !data->irq_mask) {
 		disable_irq(data->client->irq);
+		data->irq_enabled = false;
+	}
 
 	spin_unlock_irqrestore(&data->lock, flags);
 }
@@ -275,14 +292,18 @@ static void pca954x_irq_mask(struct irq_data *idata)
 static void pca954x_irq_unmask(struct irq_data *idata)
 {
 	struct pca954x *data = irq_data_get_irq_chip_data(idata);
+	unsigned int mask_enable = data->irq_mask_enable;
 	unsigned int pos = idata->hwirq;
 	unsigned long flags;
 
 	spin_lock_irqsave(&data->lock, flags);
 
-	if (!data->irq_mask)
-		enable_irq(data->client->irq);
 	data->irq_mask |= BIT(pos);
+	if (!data->irq_enabled
+	    && (data->irq_mask & mask_enable) == mask_enable) {
+		enable_irq(data->client->irq);
+		data->irq_enabled = true;
+	}
 
 	spin_unlock_irqrestore(&data->lock, flags);
 }
@@ -305,6 +326,7 @@ static int pca954x_irq_setup(struct i2c_mux_core *muxc)
 {
 	struct pca954x *data = i2c_mux_priv(muxc);
 	struct i2c_client *client = data->client;
+	u32 irq_mask_enable[PCA954X_MAX_NCHANS] = { 0 };
 	int c, err, irq;
 
 	if (!data->chip->has_irq || client->irq <= 0)
@@ -312,6 +334,9 @@ static int pca954x_irq_setup(struct i2c_mux_core *muxc)
 
 	spin_lock_init(&data->lock);
 
+	of_property_read_u32_array(client->dev.of_node, "nxp,irq-mask-enable",
+		irq_mask_enable, data->chip->nchans);
+
 	data->irq = irq_domain_add_linear(client->dev.of_node,
 					  data->chip->nchans,
 					  &irq_domain_simple_ops, data);
@@ -319,6 +344,8 @@ static int pca954x_irq_setup(struct i2c_mux_core *muxc)
 		return -ENODEV;
 
 	for (c = 0; c < data->chip->nchans; c++) {
+		data->irq_mask_enable |= irq_mask_enable[c] ? BIT(c) : 0;
+		WARN_ON(irq_mask_enable[c] > 1);
 		irq = irq_create_mapping(data->irq, c);
 		irq_set_chip_data(irq, data);
 		irq_set_chip_and_handler(irq, &pca954x_irq_chip,
-- 
1.8.3.1

Re: [PATCH v5 4/5] dt: bindings: i2c-mux-pca954x: Add documentation for nxp,irq-mask-enable

[Peter Rosin]

On 2017-01-17 09:00, Phil Reid wrote:
> Unfortunately some hardware device will assert their irq line immediately
> on power on and provide no mechanism to mask the irq. As the i2c muxes
> provide no method to mask irq line this provides a work around by keeping
> the parent irq masked until enough device drivers have loaded to service
> all pending interrupts.
> 
> For example the the ltc1760 assert its SMBALERT irq immediately on power
> on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
> device is registered irq are enabled and fire continuously as the second
> device driver has not yet loaded. Setting this parameter to <1 1> will
> delay the irq being enabled until both devices are ready.

Hang on, does this suggestion I made make any sense at all? Maybe it does,
but does the pca954x driver even get notified of any but the first irq client
that unmasks interrupts on a mux segment? How can it count the number of
active irq clients if not?

I'm truly sorry for the trouble I'm causing by not just saying how it should
be done from the start, but I feel like I've been thrown in at the deep end
when it comes to interrupt controllers...

Cheers,
peda

> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> index aa09704..ac71be6 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
> @@ -19,6 +19,8 @@ Optional Properties:
>    - i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all
>      children in idle state. This is necessary for example, if there are several
>      multiplexers on the bus and the devices behind them use same I2C addresses.
> +  - nxp,irq-mask-enable: array; Defines the minimum number of chips that must
> +    register an irq for each channel before the parent irq line in enabled.
>    - interrupt-parent: Phandle for the interrupt controller that services
>      interrupts for this device.
>    - interrupts: Interrupt mapping for IRQ.
> @@ -36,6 +38,7 @@ Example:
>  		#size-cells = <0>;
>  		reg = <0x74>;
>  
> +		nxp,irq-mask-enable = <0 0 0 0 1 0 0 0>;
>  		interrupt-parent = <&ipic>;
>  		interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
>  		interrupt-controller;
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 4/5] dt: bindings: i2c-mux-pca954x: Add documentation for nxp,irq-mask-enable

[Phil Reid]

On 17/01/2017 16:57, Peter Rosin wrote:
> On 2017-01-17 09:00, Phil Reid wrote:
>> Unfortunately some hardware device will assert their irq line immediately
>> on power on and provide no mechanism to mask the irq. As the i2c muxes
>> provide no method to mask irq line this provides a work around by keeping
>> the parent irq masked until enough device drivers have loaded to service
>> all pending interrupts.
>>
>> For example the the ltc1760 assert its SMBALERT irq immediately on power
>> on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
>> device is registered irq are enabled and fire continuously as the second
>> device driver has not yet loaded. Setting this parameter to <1 1> will
>> delay the irq being enabled until both devices are ready.
>
G'day Peter,


> Hang on, does this suggestion I made make any sense at all? Maybe it does,
> but does the pca954x driver even get notified of any but the first irq client
> that unmasks interrupts on a mux segment? How can it count the number of
> active irq clients if not?

Good question.

So what I did to test is setup my 2 ltc1760s to use the same irq on the pca954x.
Using the latest patch series.

Adding a log message into the irq_unmask function got the following.
	dev_err(&data->client->dev, "irq_unmask %d %x %d", pos, data->irq_mask, data->irq_enabled);

dmesg | grep irq_unmask
[    4.392098] pca954x 4-0070: irq_unmask 0 1 1


But Looks like both got registered ok to the same irq.
cat /proc/interrupts
161:          0          0  i2c-mux-pca954x   0 Level     15-000a, 16-000a

So from this testing, it doesn't look like it gets called multiple times.
So back to the bitmask for the dt do you think.
I think the interrupt enablelogic is correct now.

>
> I'm truly sorry for the trouble I'm causing by not just saying how it should
> be done from the start, but I feel like I've been thrown in at the deep end
> when it comes to interrupt controllers...

No problem. I'm learning a couple things as we go.
Should help me out on other drivers :)


-- 
Regards
Phil Reid

Re: [PATCH v5 4/5] dt: bindings: i2c-mux-pca954x: Add documentation for nxp,irq-mask-enable

[Peter Rosin]

On 2017-01-17 10:28, Phil Reid wrote:
> On 17/01/2017 16:57, Peter Rosin wrote:
>> On 2017-01-17 09:00, Phil Reid wrote:
>>> Unfortunately some hardware device will assert their irq line immediately
>>> on power on and provide no mechanism to mask the irq. As the i2c muxes
>>> provide no method to mask irq line this provides a work around by keeping
>>> the parent irq masked until enough device drivers have loaded to service
>>> all pending interrupts.
>>>
>>> For example the the ltc1760 assert its SMBALERT irq immediately on power
>>> on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
>>> device is registered irq are enabled and fire continuously as the second
>>> device driver has not yet loaded. Setting this parameter to <1 1> will
>>> delay the irq being enabled until both devices are ready.
>>
> G'day Peter,
> 
> 
>> Hang on, does this suggestion I made make any sense at all? Maybe it does,
>> but does the pca954x driver even get notified of any but the first irq client
>> that unmasks interrupts on a mux segment? How can it count the number of
>> active irq clients if not?
> 
> Good question.
> 
> So what I did to test is setup my 2 ltc1760s to use the same irq on the pca954x.
> Using the latest patch series.
> 
> Adding a log message into the irq_unmask function got the following.
> 	dev_err(&data->client->dev, "irq_unmask %d %x %d", pos, data->irq_mask, data->irq_enabled);
> 
> dmesg | grep irq_unmask
> [    4.392098] pca954x 4-0070: irq_unmask 0 1 1
> 
> 
> But Looks like both got registered ok to the same irq.
> cat /proc/interrupts
> 161:          0          0  i2c-mux-pca954x   0 Level     15-000a, 16-000a
> 
> So from this testing, it doesn't look like it gets called multiple times.

As I suspected, thanks for verifying!

> So back to the bitmask for the dt do you think.

Looking at kernel/irq/chip.c:irq_enable (and struct irq_chip docs), I get the
feeling that if you provide the irq_enable operation (and maybe irq_disable too?),
that might get us more info?

> I think the interrupt enablelogic is correct now.
> 
>>
>> I'm truly sorry for the trouble I'm causing by not just saying how it should
>> be done from the start, but I feel like I've been thrown in at the deep end
>> when it comes to interrupt controllers...
> 
> No problem. I'm learning a couple things as we go.
> Should help me out on other drivers :)

Yes, I'm also picking up a few bits here and there...

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 4/5] dt: bindings: i2c-mux-pca954x: Add documentation for nxp,irq-mask-enable

[Peter Rosin]

On 2017-01-17 10:43, Peter Rosin wrote:
> On 2017-01-17 10:28, Phil Reid wrote:
>> On 17/01/2017 16:57, Peter Rosin wrote:
>>> On 2017-01-17 09:00, Phil Reid wrote:
>>>> Unfortunately some hardware device will assert their irq line immediately
>>>> on power on and provide no mechanism to mask the irq. As the i2c muxes
>>>> provide no method to mask irq line this provides a work around by keeping
>>>> the parent irq masked until enough device drivers have loaded to service
>>>> all pending interrupts.
>>>>
>>>> For example the the ltc1760 assert its SMBALERT irq immediately on power
>>>> on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
>>>> device is registered irq are enabled and fire continuously as the second
>>>> device driver has not yet loaded. Setting this parameter to <1 1> will
>>>> delay the irq being enabled until both devices are ready.
>>>
>> G'day Peter,
>>
>>
>>> Hang on, does this suggestion I made make any sense at all? Maybe it does,
>>> but does the pca954x driver even get notified of any but the first irq client
>>> that unmasks interrupts on a mux segment? How can it count the number of
>>> active irq clients if not?
>>
>> Good question.
>>
>> So what I did to test is setup my 2 ltc1760s to use the same irq on the pca954x.
>> Using the latest patch series.
>>
>> Adding a log message into the irq_unmask function got the following.
>> 	dev_err(&data->client->dev, "irq_unmask %d %x %d", pos, data->irq_mask, data->irq_enabled);
>>
>> dmesg | grep irq_unmask
>> [    4.392098] pca954x 4-0070: irq_unmask 0 1 1
>>
>>
>> But Looks like both got registered ok to the same irq.
>> cat /proc/interrupts
>> 161:          0          0  i2c-mux-pca954x   0 Level     15-000a, 16-000a
>>
>> So from this testing, it doesn't look like it gets called multiple times.
> 
> As I suspected, thanks for verifying!
> 
>> So back to the bitmask for the dt do you think.
> 
> Looking at kernel/irq/chip.c:irq_enable (and struct irq_chip docs), I get the
> feeling that if you provide the irq_enable operation (and maybe irq_disable too?),
> that might get us more info?

No, I no longer think that. I think we need to get at the irq descriptor "depth".
But that feels like poking at the wrong level. Crap.

And to answer the question if I think we should go back to a dt bitmask, then
no I do not think that. The array describes what we want to do, it's the linux
implementation that gives us difficulties. Agreed?

>> I think the interrupt enablelogic is correct now.
>>
>>>
>>> I'm truly sorry for the trouble I'm causing by not just saying how it should
>>> be done from the start, but I feel like I've been thrown in at the deep end
>>> when it comes to interrupt controllers...
>>
>> No problem. I'm learning a couple things as we go.
>> Should help me out on other drivers :)
> 
> Yes, I'm also picking up a few bits here and there...
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 4/5] dt: bindings: i2c-mux-pca954x: Add documentation for nxp,irq-mask-enable

[Phil Reid]

On 17/01/2017 18:14, Peter Rosin wrote:
> On 2017-01-17 10:43, Peter Rosin wrote:
>> On 2017-01-17 10:28, Phil Reid wrote:
>>> On 17/01/2017 16:57, Peter Rosin wrote:
>>>> On 2017-01-17 09:00, Phil Reid wrote:
>>>>> Unfortunately some hardware device will assert their irq line immediately
>>>>> on power on and provide no mechanism to mask the irq. As the i2c muxes
>>>>> provide no method to mask irq line this provides a work around by keeping
>>>>> the parent irq masked until enough device drivers have loaded to service
>>>>> all pending interrupts.
>>>>>
>>>>> For example the the ltc1760 assert its SMBALERT irq immediately on power
>>>>> on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
>>>>> device is registered irq are enabled and fire continuously as the second
>>>>> device driver has not yet loaded. Setting this parameter to <1 1> will
>>>>> delay the irq being enabled until both devices are ready.
>>>>
>>> G'day Peter,
>>>
>>>
>>>> Hang on, does this suggestion I made make any sense at all? Maybe it does,
>>>> but does the pca954x driver even get notified of any but the first irq client
>>>> that unmasks interrupts on a mux segment? How can it count the number of
>>>> active irq clients if not?
>>>
>>> Good question.
>>>
>>> So what I did to test is setup my 2 ltc1760s to use the same irq on the pca954x.
>>> Using the latest patch series.
>>>
>>> Adding a log message into the irq_unmask function got the following.
>>> 	dev_err(&data->client->dev, "irq_unmask %d %x %d", pos, data->irq_mask, data->irq_enabled);
>>>
>>> dmesg | grep irq_unmask
>>> [    4.392098] pca954x 4-0070: irq_unmask 0 1 1
>>>
>>>
>>> But Looks like both got registered ok to the same irq.
>>> cat /proc/interrupts
>>> 161:          0          0  i2c-mux-pca954x   0 Level     15-000a, 16-000a
>>>
>>> So from this testing, it doesn't look like it gets called multiple times.
>>
>> As I suspected, thanks for verifying!
>>
>>> So back to the bitmask for the dt do you think.
>>
>> Looking at kernel/irq/chip.c:irq_enable (and struct irq_chip docs), I get the
>> feeling that if you provide the irq_enable operation (and maybe irq_disable too?),
>> that might get us more info?
>
> No, I no longer think that. I think we need to get at the irq descriptor "depth".
> But that feels like poking at the wrong level. Crap.
>
> And to answer the question if I think we should go back to a dt bitmask, then
> no I do not think that. The array describes what we want to do, it's the linux
> implementation that gives us difficulties. Agreed?

G'day Peter,

Yes agreed, that makes sense.

I had a play with using the irq_enable call back.
But that doesn't seem to offer any more feedback then unmask.
Called just once on the first irq that registers.

The only way I can see to count the number of registered handlers on a
shared irq is to count the action handlers on the irq_desc.
The only irq_chip handler that seems guaranteed to be called on irq request is
irq_bus_lock, irq_bus_sync_unlock callbacks, but these are called elsewhere
at various times.

We have a dt spec that supports this future proofing of the irq count.
But as you say the kernel code prevents us at the moment.
The count of 1 bitmask implementation works for my particular situation at the moment.

Perhaps the > 1 case can be tackled in the future when someone has a use case?

Thoughts?



>
>>> I think the interrupt enablelogic is correct now.
>>>
>>>>
>>>> I'm truly sorry for the trouble I'm causing by not just saying how it should
>>>> be done from the start, but I feel like I've been thrown in at the deep end
>>>> when it comes to interrupt controllers...
>>>
>>> No problem. I'm learning a couple things as we go.
>>> Should help me out on other drivers :)
>>
>> Yes, I'm also picking up a few bits here and there...



-- 
Regards
Phil Reid

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 5/5] i2c: mux: pca954x: Add irq-mask-enable to delay enabling irqs

[Peter Rosin]

On 2017-01-17 09:00, Phil Reid wrote:
> Unfortunately some hardware device will assert their irq line immediately
> on power on and provide no mechanism to mask the irq. As the i2c muxes
> provide no method to mask irq line this provides a work around by keeping
> the parent irq masked until enough device drivers have loaded to service
> all pending interrupts.
> 
> For example the the ltc1760 assert its SMBALERT irq immediately on power
> on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
> device is registered irq are enabled and fire continuously as the second
> device driver has not yet loaded. Setting this parameter to <1 1> will
> delay the irq being enabled until both devices are ready.
> 
> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
> ---
>  drivers/i2c/muxes/i2c-mux-pca954x.c | 33 ++++++++++++++++++++++++++++++---
>  1 file changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index f55da88..012b2ef 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -76,6 +76,19 @@ struct chip_desc {
>  	} muxtype;
>  };
>  
> +/*
> + * irq_mask_enable: Provides a mechanism to work around hardware that asserts
> + * their irq immediately on power on. It allows the enabling of the irq to be
> + * delayed until the corresponding bits in the the irq_mask are set thru
> + * irq_unmask.
> + * For example the ltc1760 assert its SMBALERT irq immediately on power on.
> + * With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
> + * device is registered irq are enabled and fire continuously as the second
> + * device driver has not yet loaded. Setting this parameter to 0x3 while
> + * delay the irq being enabled until both devices are ready.
> + * This workaround will not work if two devices share an interrupt on the
> + * same bus segment.

It will also not work if something shares the interrupt with the pca954x mux,
on the parent side of the mux, so to speak. Then that other driver may
potentially enable the irq "behind the back" of the pca954x driver.

> + */
>  struct pca954x {
>  	const struct chip_desc *chip;
>  
> @@ -84,7 +97,9 @@ struct pca954x {
>  	struct i2c_client *client;
>  
>  	struct irq_domain *irq;
> +	unsigned int irq_mask_enable;
>  	unsigned int irq_mask;
> +	bool irq_enabled;
>  	spinlock_t lock;
>  };
>  
> @@ -266,8 +281,10 @@ static void pca954x_irq_mask(struct irq_data *idata)
>  	spin_lock_irqsave(&data->lock, flags);
>  
>  	data->irq_mask &= ~BIT(pos);
> -	if (!data->irq_mask)
> +	if (data->irq_enabled && !data->irq_mask) {
>  		disable_irq(data->client->irq);
> +		data->irq_enabled = false;
> +	}

When irq_mask_enable is non-zero, I think the parent irq should be masked
when the first irq from the set in irq_mask_enable is masked. For symmetry.

Like so (untested):

	if (data->irq_enabled) {
		if (!data->irq_mask ||
			(data->irq_mask & mask_enable) != mask_enable) {
			disable_irq(data->client->irq);
			data->irq_enabled = false;
		}
	}

Hmm, this whole thing is fiddly and while it solves your problem it doesn't
allow for solving the more general problem when there are "problematic"
devices mixed with other devices. At least, I don't see it. And the
limitations we are walking into with tracking number of enables etc suggests
that we are attacking this at the wrong level. Maybe you should try to work
around the hw limitations not in the pca954x driver, but in the irq core?

I.e. have the irq core check, for each irq, for a property that specifies
the depth at which each irq should be unmasked. This new property should
probably be located in the interrupt-controller node? Then the code can
unmask interrupts when the depth hits this mark, instead of always unmasking
the interrupt when the depth changes from zero to one. You are then adding
the workaround at a level where there is enough information to fix the
more general problem. I think?

But, once again, I'm no irq expert and would desperately like a second
opinion on this stuff...

>  
>  	spin_unlock_irqrestore(&data->lock, flags);
>  }
> @@ -275,14 +292,18 @@ static void pca954x_irq_mask(struct irq_data *idata)
>  static void pca954x_irq_unmask(struct irq_data *idata)
>  {
>  	struct pca954x *data = irq_data_get_irq_chip_data(idata);
> +	unsigned int mask_enable = data->irq_mask_enable;
>  	unsigned int pos = idata->hwirq;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&data->lock, flags);
>  
> -	if (!data->irq_mask)
> -		enable_irq(data->client->irq);
>  	data->irq_mask |= BIT(pos);
> +	if (!data->irq_enabled
> +	    && (data->irq_mask & mask_enable) == mask_enable) {

I think the coding standard says that the && should be at the end of the
first line. Didn't checkpatch complain?

Cheers,
peda

> +		enable_irq(data->client->irq);
> +		data->irq_enabled = true;
> +	}
>  
>  	spin_unlock_irqrestore(&data->lock, flags);
>  }
> @@ -305,6 +326,7 @@ static int pca954x_irq_setup(struct i2c_mux_core *muxc)
>  {
>  	struct pca954x *data = i2c_mux_priv(muxc);
>  	struct i2c_client *client = data->client;
> +	u32 irq_mask_enable[PCA954X_MAX_NCHANS] = { 0 };
>  	int c, err, irq;
>  
>  	if (!data->chip->has_irq || client->irq <= 0)
> @@ -312,6 +334,9 @@ static int pca954x_irq_setup(struct i2c_mux_core *muxc)
>  
>  	spin_lock_init(&data->lock);
>  
> +	of_property_read_u32_array(client->dev.of_node, "nxp,irq-mask-enable",
> +		irq_mask_enable, data->chip->nchans);
> +
>  	data->irq = irq_domain_add_linear(client->dev.of_node,
>  					  data->chip->nchans,
>  					  &irq_domain_simple_ops, data);
> @@ -319,6 +344,8 @@ static int pca954x_irq_setup(struct i2c_mux_core *muxc)
>  		return -ENODEV;
>  
>  	for (c = 0; c < data->chip->nchans; c++) {
> +		data->irq_mask_enable |= irq_mask_enable[c] ? BIT(c) : 0;
> +		WARN_ON(irq_mask_enable[c] > 1);
>  		irq = irq_create_mapping(data->irq, c);
>  		irq_set_chip_data(irq, data);
>  		irq_set_chip_and_handler(irq, &pca954x_irq_chip,
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 5/5] i2c: mux: pca954x: Add irq-mask-enable to delay enabling irqs

[Phil Reid]

On 18/01/2017 20:19, Peter Rosin wrote:
> On 2017-01-17 09:00, Phil Reid wrote:
>> Unfortunately some hardware device will assert their irq line immediately
>> on power on and provide no mechanism to mask the irq. As the i2c muxes
>> provide no method to mask irq line this provides a work around by keeping
>> the parent irq masked until enough device drivers have loaded to service
>> all pending interrupts.
>>
>> For example the the ltc1760 assert its SMBALERT irq immediately on power
>> on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
>> device is registered irq are enabled and fire continuously as the second
>> device driver has not yet loaded. Setting this parameter to <1 1> will
>> delay the irq being enabled until both devices are ready.
>>
>> Signed-off-by: Phil Reid <preid@electromag.com.au>
>> ---
>>  drivers/i2c/muxes/i2c-mux-pca954x.c | 33 ++++++++++++++++++++++++++++++---
>>  1 file changed, 30 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
>> index f55da88..012b2ef 100644
>> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
>> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
>> @@ -76,6 +76,19 @@ struct chip_desc {
>>  	} muxtype;
>>  };
>>
>> +/*
>> + * irq_mask_enable: Provides a mechanism to work around hardware that asserts
>> + * their irq immediately on power on. It allows the enabling of the irq to be
>> + * delayed until the corresponding bits in the the irq_mask are set thru
>> + * irq_unmask.
>> + * For example the ltc1760 assert its SMBALERT irq immediately on power on.
>> + * With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
>> + * device is registered irq are enabled and fire continuously as the second
>> + * device driver has not yet loaded. Setting this parameter to 0x3 while
>> + * delay the irq being enabled until both devices are ready.
>> + * This workaround will not work if two devices share an interrupt on the
>> + * same bus segment.
>
> It will also not work if something shares the interrupt with the pca954x mux,
> on the parent side of the mux, so to speak. Then that other driver may
> potentially enable the irq "behind the back" of the pca954x driver.
>
>> + */
>>  struct pca954x {
>>  	const struct chip_desc *chip;
>>
>> @@ -84,7 +97,9 @@ struct pca954x {
>>  	struct i2c_client *client;
>>
>>  	struct irq_domain *irq;
>> +	unsigned int irq_mask_enable;
>>  	unsigned int irq_mask;
>> +	bool irq_enabled;
>>  	spinlock_t lock;
>>  };
>>
>> @@ -266,8 +281,10 @@ static void pca954x_irq_mask(struct irq_data *idata)
>>  	spin_lock_irqsave(&data->lock, flags);
>>
>>  	data->irq_mask &= ~BIT(pos);
>> -	if (!data->irq_mask)
>> +	if (data->irq_enabled && !data->irq_mask) {
>>  		disable_irq(data->client->irq);
>> +		data->irq_enabled = false;
>> +	}
>
> When irq_mask_enable is non-zero, I think the parent irq should be masked
> when the first irq from the set in irq_mask_enable is masked. For symmetry.
>
> Like so (untested):
>
> 	if (data->irq_enabled) {
> 		if (!data->irq_mask ||
> 			(data->irq_mask & mask_enable) != mask_enable) {
> 			disable_irq(data->client->irq);
> 			data->irq_enabled = false;
> 		}
> 	}
Yeap this make sense.

>
> Hmm, this whole thing is fiddly and while it solves your problem it doesn't
> allow for solving the more general problem when there are "problematic"
> devices mixed with other devices. At least, I don't see it. And the
> limitations we are walking into with tracking number of enables etc suggests
> that we are attacking this at the wrong level. Maybe you should try to work
> around the hw limitations not in the pca954x driver, but in the irq core?

I'm looking at the option of getting the hardware changed to not route
the irq for my chips thru the i2c mux. Fortunately the hardware is going thru a
revision for some other changes. Messing with the irq core sounds dangerous
with my level of knowledge.

The other way I think I can tackle it after reading the datasheet for the ltc1760 is that
it'll deassert it's irq (smbalert) line when the host sends a ARA request on the bus segment.

There's a driver in the kernel for this already, but it's not DT enable and doesn't
handle multiple bus segments. I'll have a look at that as well.
Pretty sure it would need the mux to become an irq parent as per patch 1-3 of this series.
This would be so the system can figure out which segment to do the poll on.
But p4-5 could be dropped which is where we're stuck I think.

Looking at this approach it shouldn't matter if the ltc1760 driver has registered yet or not.
This approach possibly has a lot more generic appeal I think.

Thoughts on just submitting p1-3 for now while I figure out the SMB alert approach?

>
> I.e. have the irq core check, for each irq, for a property that specifies
> the depth at which each irq should be unmasked. This new property should
> probably be located in the interrupt-controller node? Then the code can
> unmask interrupts when the depth hits this mark, instead of always unmasking
> the interrupt when the depth changes from zero to one. You are then adding
> the workaround at a level where there is enough information to fix the
> more general problem. I think?
>
> But, once again, I'm no irq expert and would desperately like a second
> opinion on this stuff...
>
>>
>>  	spin_unlock_irqrestore(&data->lock, flags);
>>  }
>> @@ -275,14 +292,18 @@ static void pca954x_irq_mask(struct irq_data *idata)
>>  static void pca954x_irq_unmask(struct irq_data *idata)
>>  {
>>  	struct pca954x *data = irq_data_get_irq_chip_data(idata);
>> +	unsigned int mask_enable = data->irq_mask_enable;
>>  	unsigned int pos = idata->hwirq;
>>  	unsigned long flags;
>>
>>  	spin_lock_irqsave(&data->lock, flags);
>>
>> -	if (!data->irq_mask)
>> -		enable_irq(data->client->irq);
>>  	data->irq_mask |= BIT(pos);
>> +	if (!data->irq_enabled
>> +	    && (data->irq_mask & mask_enable) == mask_enable) {
>
> I think the coding standard says that the && should be at the end of the
> first line. Didn't checkpatch complain?

No it didn't complain. and I wasn't sure which way to do this.
So I had a look at some other code in the i2c folders (which is not definitive I know).
regexp "^\s*&&" showed some hits with this style.
Name	Line	Text	Path
drivers\i2c\busses\i2c-ali1535.c  294	    && (timeout++ < MAX_TIMEOUT));	
drivers\i2c\busses\i2c-ali15x3.c  301	    && (timeout++ < MAX_TIMEOUT));	
drivers\i2c\busses\i2c-ismt.c     408	      && (size != I2C_SMBUS_I2C_BLOCK_DATA))	
drivers\i2c\busses\i2c-mv64xxx.c  258	      && (drv_data->byte_posn != 0))) {	
...



>
> Cheers,
> peda
>
>> +		enable_irq(data->client->irq);
>> +		data->irq_enabled = true;
>> +	}
>>
>>  	spin_unlock_irqrestore(&data->lock, flags);
>>  }
>> @@ -305,6 +326,7 @@ static int pca954x_irq_setup(struct i2c_mux_core *muxc)
>>  {
>>  	struct pca954x *data = i2c_mux_priv(muxc);
>>  	struct i2c_client *client = data->client;
>> +	u32 irq_mask_enable[PCA954X_MAX_NCHANS] = { 0 };
>>  	int c, err, irq;
>>
>>  	if (!data->chip->has_irq || client->irq <= 0)
>> @@ -312,6 +334,9 @@ static int pca954x_irq_setup(struct i2c_mux_core *muxc)
>>
>>  	spin_lock_init(&data->lock);
>>
>> +	of_property_read_u32_array(client->dev.of_node, "nxp,irq-mask-enable",
>> +		irq_mask_enable, data->chip->nchans);
>> +
>>  	data->irq = irq_domain_add_linear(client->dev.of_node,
>>  					  data->chip->nchans,
>>  					  &irq_domain_simple_ops, data);
>> @@ -319,6 +344,8 @@ static int pca954x_irq_setup(struct i2c_mux_core *muxc)
>>  		return -ENODEV;
>>
>>  	for (c = 0; c < data->chip->nchans; c++) {
>> +		data->irq_mask_enable |= irq_mask_enable[c] ? BIT(c) : 0;
>> +		WARN_ON(irq_mask_enable[c] > 1);
>>  		irq = irq_create_mapping(data->irq, c);
>>  		irq_set_chip_data(irq, data);
>>  		irq_set_chip_and_handler(irq, &pca954x_irq_chip,
>>
>
>
>


-- 
Regards
Phil Reid

Re: [PATCH v5 3/5] i2c: mux: pca954x: Add interrupt controller support

[Peter Rosin]

On 2017-01-17 09:00, Phil Reid wrote:
> Various muxes can aggregate multiple interrupts from each i2c bus.
> All of the muxes with interrupt support combine the active low irq lines
> using an internal 'and' function and generate a combined active low
> output. The muxes do provide the ability to read a control register to
> determine which irq is active. By making the mux an irq controller isr
> latency can potentially be reduced by reading the status register and
> then only calling the registered isr on that bus segment.
> 
> As there is no irq masking on the mux irq are disabled until irq_unmask is
> called at least once.
> 
> Signed-off-by: Phil Reid <preid@electromag.com.au>

Acked-by: Peter Rosin <peda@axentia.se>

Cheers,
peda

> ---
>  drivers/i2c/muxes/i2c-mux-pca954x.c | 141 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 139 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index bbf088e..f55da88 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -41,14 +41,20 @@
>  #include <linux/i2c.h>
>  #include <linux/i2c-mux.h>
>  #include <linux/i2c/pca954x.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/of_irq.h>
>  #include <linux/pm.h>
>  #include <linux/slab.h>
> +#include <linux/spinlock.h>
>  
>  #define PCA954X_MAX_NCHANS 8
>  
> +#define PCA954X_IRQ_OFFSET 4
> +
>  enum pca_type {
>  	pca_9540,
>  	pca_9542,
> @@ -63,6 +69,7 @@ enum pca_type {
>  struct chip_desc {
>  	u8 nchans;
>  	u8 enable;	/* used for muxes only */
> +	u8 has_irq;
>  	enum muxtype {
>  		pca954x_ismux = 0,
>  		pca954x_isswi
> @@ -75,6 +82,10 @@ struct pca954x {
>  	u8 last_chan;		/* last register value */
>  	u8 deselect;
>  	struct i2c_client *client;
> +
> +	struct irq_domain *irq;
> +	unsigned int irq_mask;
> +	spinlock_t lock;
>  };
>  
>  /* Provide specs for the PCA954x types we know about */
> @@ -87,19 +98,23 @@ struct pca954x {
>  	[pca_9542] = {
>  		.nchans = 2,
>  		.enable = 0x4,
> +		.has_irq = 1,
>  		.muxtype = pca954x_ismux,
>  	},
>  	[pca_9543] = {
>  		.nchans = 2,
> +		.has_irq = 1,
>  		.muxtype = pca954x_isswi,
>  	},
>  	[pca_9544] = {
>  		.nchans = 4,
>  		.enable = 0x4,
> +		.has_irq = 1,
>  		.muxtype = pca954x_ismux,
>  	},
>  	[pca_9545] = {
>  		.nchans = 4,
> +		.has_irq = 1,
>  		.muxtype = pca954x_isswi,
>  	},
>  	[pca_9547] = {
> @@ -222,6 +237,114 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
>  	return pca954x_reg_write(muxc->parent, client, data->last_chan);
>  }
>  
> +static irqreturn_t pca954x_irq_handler(int irq, void *dev_id)
> +{
> +	struct pca954x *data = dev_id;
> +	unsigned int child_irq;
> +	int ret, i, handled;
> +
> +	ret = i2c_smbus_read_byte(data->client);
> +	if (ret < 0)
> +		return IRQ_NONE;
> +
> +	for (i = 0; i < data->chip->nchans; i++) {
> +		if (ret & BIT(PCA954X_IRQ_OFFSET + i)) {
> +			child_irq = irq_linear_revmap(data->irq, i);
> +			handle_nested_irq(child_irq);
> +			handled++;
> +		}
> +	}
> +	return handled ? IRQ_HANDLED : IRQ_NONE;
> +}
> +
> +static void pca954x_irq_mask(struct irq_data *idata)
> +{
> +	struct pca954x *data = irq_data_get_irq_chip_data(idata);
> +	unsigned int pos = idata->hwirq;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&data->lock, flags);
> +
> +	data->irq_mask &= ~BIT(pos);
> +	if (!data->irq_mask)
> +		disable_irq(data->client->irq);
> +
> +	spin_unlock_irqrestore(&data->lock, flags);
> +}
> +
> +static void pca954x_irq_unmask(struct irq_data *idata)
> +{
> +	struct pca954x *data = irq_data_get_irq_chip_data(idata);
> +	unsigned int pos = idata->hwirq;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&data->lock, flags);
> +
> +	if (!data->irq_mask)
> +		enable_irq(data->client->irq);
> +	data->irq_mask |= BIT(pos);
> +
> +	spin_unlock_irqrestore(&data->lock, flags);
> +}
> +
> +static int pca954x_irq_set_type(struct irq_data *idata, unsigned int type)
> +{
> +	if ((type & IRQ_TYPE_SENSE_MASK) != IRQ_TYPE_LEVEL_LOW)
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +static struct irq_chip pca954x_irq_chip = {
> +	.name = "i2c-mux-pca954x",
> +	.irq_mask = pca954x_irq_mask,
> +	.irq_unmask = pca954x_irq_unmask,
> +	.irq_set_type = pca954x_irq_set_type,
> +};
> +
> +static int pca954x_irq_setup(struct i2c_mux_core *muxc)
> +{
> +	struct pca954x *data = i2c_mux_priv(muxc);
> +	struct i2c_client *client = data->client;
> +	int c, err, irq;
> +
> +	if (!data->chip->has_irq || client->irq <= 0)
> +		return 0;
> +
> +	spin_lock_init(&data->lock);
> +
> +	data->irq = irq_domain_add_linear(client->dev.of_node,
> +					  data->chip->nchans,
> +					  &irq_domain_simple_ops, data);
> +	if (!data->irq)
> +		return -ENODEV;
> +
> +	for (c = 0; c < data->chip->nchans; c++) {
> +		irq = irq_create_mapping(data->irq, c);
> +		irq_set_chip_data(irq, data);
> +		irq_set_chip_and_handler(irq, &pca954x_irq_chip,
> +			handle_simple_irq);
> +	}
> +
> +	err = devm_request_threaded_irq(&client->dev, data->client->irq, NULL,
> +					pca954x_irq_handler,
> +					IRQF_ONESHOT | IRQF_SHARED,
> +					"pca954x", data);
> +	if (err)
> +		goto err_req_irq;
> +
> +	disable_irq(data->client->irq);
> +
> +	return 0;
> +err_req_irq:
> +	for (c = 0; c < data->chip->nchans; c++) {
> +		irq = irq_find_mapping(data->irq, c);
> +		irq_dispose_mapping(irq);
> +	}
> +	irq_domain_remove(data->irq);
> +
> +	return err;
> +}
> +
>  /*
>   * I2C init/probing/exit functions
>   */
> @@ -286,6 +409,10 @@ static int pca954x_probe(struct i2c_client *client,
>  	idle_disconnect_dt = of_node &&
>  		of_property_read_bool(of_node, "i2c-mux-idle-disconnect");
>  
> +	ret = pca954x_irq_setup(muxc);
> +	if (ret)
> +		goto fail_del_adapters;
> +
>  	/* Now create an adapter for each channel */
>  	for (num = 0; num < data->chip->nchans; num++) {
>  		bool idle_disconnect_pd = false;
> @@ -311,7 +438,7 @@ static int pca954x_probe(struct i2c_client *client,
>  			dev_err(&client->dev,
>  				"failed to register multiplexed adapter"
>  				" %d as bus %d\n", num, force);
> -			goto virt_reg_failed;
> +			goto fail_del_adapters;
>  		}
>  	}
>  
> @@ -322,7 +449,7 @@ static int pca954x_probe(struct i2c_client *client,
>  
>  	return 0;
>  
> -virt_reg_failed:
> +fail_del_adapters:
>  	i2c_mux_del_adapters(muxc);
>  	return ret;
>  }
> @@ -330,6 +457,16 @@ static int pca954x_probe(struct i2c_client *client,
>  static int pca954x_remove(struct i2c_client *client)
>  {
>  	struct i2c_mux_core *muxc = i2c_get_clientdata(client);
> +	struct pca954x *data = i2c_mux_priv(muxc);
> +	int c, irq;
> +
> +	if (data->irq) {
> +		for (c = 0; c < data->chip->nchans; c++) {
> +			irq = irq_find_mapping(data->irq, c);
> +			irq_dispose_mapping(irq);
> +		}
> +		irq_domain_remove(data->irq);
> +	}
>  
>  	i2c_mux_del_adapters(muxc);
>  	return 0;
> 

Re: [PATCH v5 5/5] i2c: mux: pca954x: Add irq-mask-enable to delay enabling irqs

[Peter Rosin]

On 2017-01-19 08:48, Phil Reid wrote:
> On 18/01/2017 20:19, Peter Rosin wrote:
>> On 2017-01-17 09:00, Phil Reid wrote:
>>> Unfortunately some hardware device will assert their irq line immediately
>>> on power on and provide no mechanism to mask the irq. As the i2c muxes
>>> provide no method to mask irq line this provides a work around by keeping
>>> the parent irq masked until enough device drivers have loaded to service
>>> all pending interrupts.
>>>
>>> For example the the ltc1760 assert its SMBALERT irq immediately on power
>>> on. With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
>>> device is registered irq are enabled and fire continuously as the second
>>> device driver has not yet loaded. Setting this parameter to <1 1> will
>>> delay the irq being enabled until both devices are ready.
>>>
>>> Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
>>> ---
>>>  drivers/i2c/muxes/i2c-mux-pca954x.c | 33 ++++++++++++++++++++++++++++++---
>>>  1 file changed, 30 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
>>> index f55da88..012b2ef 100644
>>> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
>>> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
>>> @@ -76,6 +76,19 @@ struct chip_desc {
>>>  	} muxtype;
>>>  };
>>>
>>> +/*
>>> + * irq_mask_enable: Provides a mechanism to work around hardware that asserts
>>> + * their irq immediately on power on. It allows the enabling of the irq to be
>>> + * delayed until the corresponding bits in the the irq_mask are set thru
>>> + * irq_unmask.
>>> + * For example the ltc1760 assert its SMBALERT irq immediately on power on.
>>> + * With two ltc1760 attached to bus 0 & 1 on a pca954x mux when the first
>>> + * device is registered irq are enabled and fire continuously as the second
>>> + * device driver has not yet loaded. Setting this parameter to 0x3 while
>>> + * delay the irq being enabled until both devices are ready.
>>> + * This workaround will not work if two devices share an interrupt on the
>>> + * same bus segment.
>>
>> It will also not work if something shares the interrupt with the pca954x mux,
>> on the parent side of the mux, so to speak. Then that other driver may
>> potentially enable the irq "behind the back" of the pca954x driver.
>>
>>> + */
>>>  struct pca954x {
>>>  	const struct chip_desc *chip;
>>>
>>> @@ -84,7 +97,9 @@ struct pca954x {
>>>  	struct i2c_client *client;
>>>
>>>  	struct irq_domain *irq;
>>> +	unsigned int irq_mask_enable;
>>>  	unsigned int irq_mask;
>>> +	bool irq_enabled;
>>>  	spinlock_t lock;
>>>  };
>>>
>>> @@ -266,8 +281,10 @@ static void pca954x_irq_mask(struct irq_data *idata)
>>>  	spin_lock_irqsave(&data->lock, flags);
>>>
>>>  	data->irq_mask &= ~BIT(pos);
>>> -	if (!data->irq_mask)
>>> +	if (data->irq_enabled && !data->irq_mask) {
>>>  		disable_irq(data->client->irq);
>>> +		data->irq_enabled = false;
>>> +	}
>>
>> When irq_mask_enable is non-zero, I think the parent irq should be masked
>> when the first irq from the set in irq_mask_enable is masked. For symmetry.
>>
>> Like so (untested):
>>
>> 	if (data->irq_enabled) {
>> 		if (!data->irq_mask ||
>> 			(data->irq_mask & mask_enable) != mask_enable) {
>> 			disable_irq(data->client->irq);
>> 			data->irq_enabled = false;
>> 		}
>> 	}
> Yeap this make sense.
> 
>>
>> Hmm, this whole thing is fiddly and while it solves your problem it doesn't
>> allow for solving the more general problem when there are "problematic"
>> devices mixed with other devices. At least, I don't see it. And the
>> limitations we are walking into with tracking number of enables etc suggests
>> that we are attacking this at the wrong level. Maybe you should try to work
>> around the hw limitations not in the pca954x driver, but in the irq core?
> 
> I'm looking at the option of getting the hardware changed to not route
> the irq for my chips thru the i2c mux. Fortunately the hardware is going thru a
> revision for some other changes. Messing with the irq core sounds dangerous
> with my level of knowledge.

Yeah, but I bet you'd get some attention from people with more irq
experience. That can't be bad :-)

> The other way I think I can tackle it after reading the datasheet for the ltc1760 is that
> it'll deassert it's irq (smbalert) line when the host sends a ARA request on the bus segment.
> There's a driver in the kernel for this already, but it's not DT enable and doesn't
> handle multiple bus segments. I'll have a look at that as well.
> Pretty sure it would need the mux to become an irq parent as per patch 1-3 of this series.
> This would be so the system can figure out which segment to do the poll on.

Yeah sounds neater. It has the slight drawback that it may not work
for pure i2c buses since it an SMB thing??

BTW, why do you need special treatment for multiple segments? Will it not
simply have an ARA appear on whatever i2c bus the device sits on? And if
something requests to send an ARA message on a bus that happens to be a
muxed segment, my mental picture is that the mux will be operated as usual
so that the ARA appears on the muxed segment. Maybe I'm missing something?

> But p4-5 could be dropped which is where we're stuck I think.

Yes, I dislike to add a workaround for a specific case that might get
in the way for anybody wishing to fix a bigger, more generic, problem...

> Looking at this approach it shouldn't matter if the ltc1760 driver has registered yet or not.
> This approach possibly has a lot more generic appeal I think.
> 
> Thoughts on just submitting p1-3 for now while I figure out the SMB alert approach?

Yes, looks like a plan. Thanks in advance!

>> I.e. have the irq core check, for each irq, for a property that specifies
>> the depth at which each irq should be unmasked. This new property should
>> probably be located in the interrupt-controller node? Then the code can
>> unmask interrupts when the depth hits this mark, instead of always unmasking
>> the interrupt when the depth changes from zero to one. You are then adding
>> the workaround at a level where there is enough information to fix the
>> more general problem. I think?
>>
>> But, once again, I'm no irq expert and would desperately like a second
>> opinion on this stuff...
>>
>>>
>>>  	spin_unlock_irqrestore(&data->lock, flags);
>>>  }
>>> @@ -275,14 +292,18 @@ static void pca954x_irq_mask(struct irq_data *idata)
>>>  static void pca954x_irq_unmask(struct irq_data *idata)
>>>  {
>>>  	struct pca954x *data = irq_data_get_irq_chip_data(idata);
>>> +	unsigned int mask_enable = data->irq_mask_enable;
>>>  	unsigned int pos = idata->hwirq;
>>>  	unsigned long flags;
>>>
>>>  	spin_lock_irqsave(&data->lock, flags);
>>>
>>> -	if (!data->irq_mask)
>>> -		enable_irq(data->client->irq);
>>>  	data->irq_mask |= BIT(pos);
>>> +	if (!data->irq_enabled
>>> +	    && (data->irq_mask & mask_enable) == mask_enable) {
>>
>> I think the coding standard says that the && should be at the end of the
>> first line. Didn't checkpatch complain?
> 
> No it didn't complain. and I wasn't sure which way to do this.

Ah, you need the --strict option for that to show up...

Cheers,
peda

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v5 5/5] i2c: mux: pca954x: Add irq-mask-enable to delay enabling irqs

[Phil Reid]

On 20/01/2017 06:56, Peter Rosin wrote:
> On 2017-01-19 08:48, Phil Reid wrote:
>> On 18/01/2017 20:19, Peter Rosin wrote:
>>> On 2017-01-17 09:00, Phil Reid wrote:

[snip]

>>> Hmm, this whole thing is fiddly and while it solves your problem it doesn't
>>> allow for solving the more general problem when there are "problematic"
>>> devices mixed with other devices. At least, I don't see it. And the
>>> limitations we are walking into with tracking number of enables etc suggests
>>> that we are attacking this at the wrong level. Maybe you should try to work
>>> around the hw limitations not in the pca954x driver, but in the irq core?
>>
>> I'm looking at the option of getting the hardware changed to not route
>> the irq for my chips thru the i2c mux. Fortunately the hardware is going thru a
>> revision for some other changes. Messing with the irq core sounds dangerous
>> with my level of knowledge.
>
> Yeah, but I bet you'd get some attention from people with more irq
> experience. That can't be bad :-)
>
>> The other way I think I can tackle it after reading the datasheet for the ltc1760 is that
>> it'll deassert it's irq (smbalert) line when the host sends a ARA request on the bus segment.
>> There's a driver in the kernel for this already, but it's not DT enable and doesn't
>> handle multiple bus segments. I'll have a look at that as well.
>> Pretty sure it would need the mux to become an irq parent as per patch 1-3 of this series.
>> This would be so the system can figure out which segment to do the poll on.
>
> Yeah sounds neater. It has the slight drawback that it may not work
> for pure i2c buses since it an SMB thing??
>
> BTW, why do you need special treatment for multiple segments? Will it not
> simply have an ARA appear on whatever i2c bus the device sits on? And if
> something requests to send an ARA message on a bus that happens to be a
> muxed segment, my mental picture is that the mux will be operated as usual
> so that the ARA appears on the muxed segment. Maybe I'm missing something?

My think was the following.
When the SMBALERT is asserted a ARA needs to be sent by the master.
If the device sending the SMBALERT is behind a mux when need to know which segment of the bus to enable.
Using shared interrupts should work I think, but you have to iterate thru each bus segment.
If the alert device is nested behind a couple of muxes this could get expensive.
But yeah otherwise I think the correct mux segment will get enabled automatically.
The current SMBALERT driver only seems to attached to the root i2c adapter.


>
>> But p4-5 could be dropped which is where we're stuck I think.
>
> Yes, I dislike to add a workaround for a specific case that might get
> in the way for anybody wishing to fix a bigger, more generic, problem...
>
>> Looking at this approach it shouldn't matter if the ltc1760 driver has registered yet or not.
>> This approach possibly has a lot more generic appeal I think.
>>
>> Thoughts on just submitting p1-3 for now while I figure out the SMB alert approach?
>
> Yes, looks like a plan. Thanks in advance!

Thanks, I'll do a new version with just p1-3.


>>>>
>>>> -	if (!data->irq_mask)
>>>> -		enable_irq(data->client->irq);
>>>>  	data->irq_mask |= BIT(pos);
>>>> +	if (!data->irq_enabled
>>>> +	    && (data->irq_mask & mask_enable) == mask_enable) {
>>>
>>> I think the coding standard says that the && should be at the end of the
>>> first line. Didn't checkpatch complain?
>>
>> No it didn't complain. and I wasn't sure which way to do this.
>
> Ah, you need the --strict option for that to show up...
>
Haven't come across that option, I'll give it a try in future.

-- 
Regards
Phil Reid

Re: [PATCH v5 5/5] i2c: mux: pca954x: Add irq-mask-enable to delay enabling irqs

[Danielle Costantino]

On Mon, Jan 23, 2017 at 1:02 AM, Phil Reid <preid@electromag.com.au> wrote:
>
> On 20/01/2017 06:56, Peter Rosin wrote:
>>
>> On 2017-01-19 08:48, Phil Reid wrote:
>>>
>>> On 18/01/2017 20:19, Peter Rosin wrote:
>>>>
>>>> On 2017-01-17 09:00, Phil Reid wrote:
>
>
> [snip]
>
>
>>>> Hmm, this whole thing is fiddly and while it solves your problem it doesn't
>>>> allow for solving the more general problem when there are "problematic"
>>>> devices mixed with other devices. At least, I don't see it. And the
>>>> limitations we are walking into with tracking number of enables etc suggests
>>>> that we are attacking this at the wrong level. Maybe you should try to work
>>>> around the hw limitations not in the pca954x driver, but in the irq core?
>>>
>>>
>>> I'm looking at the option of getting the hardware changed to not route
>>> the irq for my chips thru the i2c mux. Fortunately the hardware is going thru a
>>> revision for some other changes. Messing with the irq core sounds dangerous
>>> with my level of knowledge.
>>
>>
>> Yeah, but I bet you'd get some attention from people with more irq
>> experience. That can't be bad :-)
>>
>>> The other way I think I can tackle it after reading the datasheet for the ltc1760 is that
>>> it'll deassert it's irq (smbalert) line when the host sends a ARA request on the bus segment.
>>> There's a driver in the kernel for this already, but it's not DT enable and doesn't
>>> handle multiple bus segments. I'll have a look at that as well.
>>> Pretty sure it would need the mux to become an irq parent as per patch 1-3 of this series.
>>> This would be so the system can figure out which segment to do the poll on.
>>
>>
>> Yeah sounds neater. It has the slight drawback that it may not work
>> for pure i2c buses since it an SMB thing??

If you need to send SMBus commands like ARA, you should be using an
SMBus 2.0+ compatible bus multiplexer. muxes like the pca954x do not
automatically select the bus segment that the ARA is destined for. It
usually is more efficient to only request the data you need from each
device, rather than checking every segment on each interrupt for the
cause, one could implement a delayed worker to schedule
checking+clearing the interrupt at a time when the bus is selected for
use by another slave device on that segment when possible. This will
reduce the number of bus setup operations per transfer to slaves on
deeper busses, reducing your i2c latency.

>>
>> BTW, why do you need special treatment for multiple segments? Will it not
>> simply have an ARA appear on whatever i2c bus the device sits on? And if
>> something requests to send an ARA message on a bus that happens to be a
>> muxed segment, my mental picture is that the mux will be operated as usual
>> so that the ARA appears on the muxed segment. Maybe I'm missing something?
>
>
> My think was the following.
> When the SMBALERT is asserted a ARA needs to be sent by the master.
> If the device sending the SMBALERT is behind a mux when need to know which segment of the bus to enable.
> Using shared interrupts should work I think, but you have to iterate thru each bus segment.
> If the alert device is nested behind a couple of muxes this could get expensive.
> But yeah otherwise I think the correct mux segment will get enabled automatically.
> The current SMBALERT driver only seems to attached to the root i2c adapter.

Using ARA in a multi mux environment is very expensive, setting up
each mux segment and then sending ARA will consume more time than it
would take if the mux structure was optimized to service devices on
similar busses, reducing the setup operation count. ARA was only
implemented on the root bus due to the design of the arbiter and mux
cannot guarantee that you are the only owner of the bus when you are
disconnected. You would also need to handle the cases where segments
lock up and must be released using the bus reset mechanism, this
resets the IRQs as well. The added cost of a I2C read to coincide the
write operation to the mux when the irq pin is asserted is
unacceptable for low latency applications.

>>
>>
>>> But p4-5 could be dropped which is where we're stuck I think.
>>
>>
>> Yes, I dislike to add a workaround for a specific case that might get
>> in the way for anybody wishing to fix a bigger, more generic, problem...
>>
>>> Looking at this approach it shouldn't matter if the ltc1760 driver has registered yet or not.
>>> This approach possibly has a lot more generic appeal I think.
>>>
>>> Thoughts on just submitting p1-3 for now while I figure out the SMB alert approach?
>>
>>
>> Yes, looks like a plan. Thanks in advance!
>
>
> Thanks, I'll do a new version with just p1-3.
>
>
>>>>>
>>>>> -       if (!data->irq_mask)
>>>>> -               enable_irq(data->client->irq);
>>>>>         data->irq_mask |= BIT(pos);
>>>>> +       if (!data->irq_enabled
>>>>> +           && (data->irq_mask & mask_enable) == mask_enable) {
>>>>
>>>>
>>>> I think the coding standard says that the && should be at the end of the
>>>> first line. Didn't checkpatch complain?
>>>
>>>
>>> No it didn't complain. and I wasn't sure which way to do this.
>>
>>
>> Ah, you need the --strict option for that to show up...
>>
> Haven't come across that option, I'll give it a try in future.
>
> --
> Regards
> Phil Reid
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html




-- 
- Danielle Costantino

Re: [PATCH v5 5/5] i2c: mux: pca954x: Add irq-mask-enable to delay enabling irqs

[Peter Rosin]

On 2017-01-25 04:50, Danielle Costantino wrote:
> On Mon, Jan 23, 2017 at 1:02 AM, Phil Reid <preid@electromag.com.au> wrote:
>>
>> On 20/01/2017 06:56, Peter Rosin wrote:
>>>
>>> On 2017-01-19 08:48, Phil Reid wrote:
>>>>
>>>> On 18/01/2017 20:19, Peter Rosin wrote:
>>>>>
>>>>> On 2017-01-17 09:00, Phil Reid wrote:
>>
>>
>> [snip]
>>
>>
>>>>> Hmm, this whole thing is fiddly and while it solves your problem it doesn't
>>>>> allow for solving the more general problem when there are "problematic"
>>>>> devices mixed with other devices. At least, I don't see it. And the
>>>>> limitations we are walking into with tracking number of enables etc suggests
>>>>> that we are attacking this at the wrong level. Maybe you should try to work
>>>>> around the hw limitations not in the pca954x driver, but in the irq core?
>>>>
>>>>
>>>> I'm looking at the option of getting the hardware changed to not route
>>>> the irq for my chips thru the i2c mux. Fortunately the hardware is going thru a
>>>> revision for some other changes. Messing with the irq core sounds dangerous
>>>> with my level of knowledge.
>>>
>>>
>>> Yeah, but I bet you'd get some attention from people with more irq
>>> experience. That can't be bad :-)
>>>
>>>> The other way I think I can tackle it after reading the datasheet for the ltc1760 is that
>>>> it'll deassert it's irq (smbalert) line when the host sends a ARA request on the bus segment.
>>>> There's a driver in the kernel for this already, but it's not DT enable and doesn't
>>>> handle multiple bus segments. I'll have a look at that as well.
>>>> Pretty sure it would need the mux to become an irq parent as per patch 1-3 of this series.
>>>> This would be so the system can figure out which segment to do the poll on.
>>>
>>>
>>> Yeah sounds neater. It has the slight drawback that it may not work
>>> for pure i2c buses since it an SMB thing??
> 
> If you need to send SMBus commands like ARA, you should be using an
> SMBus 2.0+ compatible bus multiplexer. muxes like the pca954x do not
> automatically select the bus segment that the ARA is destined for. It
> usually is more efficient to only request the data you need from each
> device, rather than checking every segment on each interrupt for the

We are not talking about checking every segment on every interrupt, we
are talking about checking the segments indicated by the INTx bits in
the control register.

> cause, one could implement a delayed worker to schedule
> checking+clearing the interrupt at a time when the bus is selected for
> use by another slave device on that segment when possible. This will
> reduce the number of bus setup operations per transfer to slaves on
> deeper busses, reducing your i2c latency.

i2c traffic scheduling does not exist in linux to the best of my knowledge,
it's all handled with a simple mutex AFAIK. So, while traffic scheduling
is an interesting problem, I think it is out of scope at this time.

If you happen to have a pca954x mux (with irq support) and happen to
have devices behind it that needs ARA support, I just don't see how any
of the above is relevant. Yes, it could be more efficient to have the
hardware done differently, but that is in many cases not a possibility.
You have to make do with what you have, and if that costs latency, then
there is little to do about that. You only have to make the new stuff
optional so that old working setups don't suffer.

>>>
>>> BTW, why do you need special treatment for multiple segments? Will it not
>>> simply have an ARA appear on whatever i2c bus the device sits on? And if
>>> something requests to send an ARA message on a bus that happens to be a
>>> muxed segment, my mental picture is that the mux will be operated as usual
>>> so that the ARA appears on the muxed segment. Maybe I'm missing something?
>>
>>
>> My think was the following.
>> When the SMBALERT is asserted a ARA needs to be sent by the master.
>> If the device sending the SMBALERT is behind a mux when need to know which segment of the bus to enable.
>> Using shared interrupts should work I think, but you have to iterate thru each bus segment.
>> If the alert device is nested behind a couple of muxes this could get expensive.
>> But yeah otherwise I think the correct mux segment will get enabled automatically.
>> The current SMBALERT driver only seems to attached to the root i2c adapter.
> 
> Using ARA in a multi mux environment is very expensive, setting up
> each mux segment and then sending ARA will consume more time than it
> would take if the mux structure was optimized to service devices on
> similar busses, reducing the setup operation count. ARA was only
> implemented on the root bus due to the design of the arbiter and mux
> cannot guarantee that you are the only owner of the bus when you are
> disconnected.

ARA handling should take the irq register of the pca95x into account and
only send ARAs to indicated mux segments. If more than one segment needs
servicing, then of course one ARA for each segment needs to be sent. If
someone builds hardware like this and then expect no latency, that someone
will hopefully at least learn something.

I do not know how this fits with the existing ARA handling (it probably
doesn't), but what needs to happen is fairly easy to picture.

>    You would also need to handle the cases where segments
> lock up and must be released using the bus reset mechanism, this
> resets the IRQs as well. The added cost of a I2C read to coincide the
> write operation to the mux when the irq pin is asserted is
> unacceptable for low latency applications.

I bet there are a lot of corner cases, and yes, this added cost has to be
optional. Perhaps with each mux child segment opting in for ARA support?
If a mux then has a way to determine that an ARA isn't needed for some of
its child segments (by reading some status register), then linux is free
to not send ARAs there. And more importantly, if no mux child segment opts
in for ARA support, it should be possible to preserve current behavior...

Cheers,
peda

Re: [PATCH v5 5/5] i2c: mux: pca954x: Add irq-mask-enable to delay enabling irqs

[Danielle Costantino]

[-- Attachment #1.1: Type: text/plain, Size: 7584 bytes --]



On 01/25/2017 12:15 AM, Peter Rosin wrote:
> On 2017-01-25 04:50, Danielle Costantino wrote:
>> On Mon, Jan 23, 2017 at 1:02 AM, Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org> wrote:
>>> On 20/01/2017 06:56, Peter Rosin wrote:
>>>> On 2017-01-19 08:48, Phil Reid wrote:
>>>>> On 18/01/2017 20:19, Peter Rosin wrote:
>>>>>> On 2017-01-17 09:00, Phil Reid wrote:
>>>
>>> [snip]
>>>
>>>
>>>>>> Hmm, this whole thing is fiddly and while it solves your problem it doesn't
>>>>>> allow for solving the more general problem when there are "problematic"
>>>>>> devices mixed with other devices. At least, I don't see it. And the
>>>>>> limitations we are walking into with tracking number of enables etc suggests
>>>>>> that we are attacking this at the wrong level. Maybe you should try to work
>>>>>> around the hw limitations not in the pca954x driver, but in the irq core?
>>>>>
>>>>> I'm looking at the option of getting the hardware changed to not route
>>>>> the irq for my chips thru the i2c mux. Fortunately the hardware is going thru a
>>>>> revision for some other changes. Messing with the irq core sounds dangerous
>>>>> with my level of knowledge.
>>>>
>>>> Yeah, but I bet you'd get some attention from people with more irq
>>>> experience. That can't be bad :-)
>>>>
>>>>> The other way I think I can tackle it after reading the datasheet for the ltc1760 is that
>>>>> it'll deassert it's irq (smbalert) line when the host sends a ARA request on the bus segment.
>>>>> There's a driver in the kernel for this already, but it's not DT enable and doesn't
>>>>> handle multiple bus segments. I'll have a look at that as well.
>>>>> Pretty sure it would need the mux to become an irq parent as per patch 1-3 of this series.
>>>>> This would be so the system can figure out which segment to do the poll on.
>>>>
>>>> Yeah sounds neater. It has the slight drawback that it may not work
>>>> for pure i2c buses since it an SMB thing??
>> If you need to send SMBus commands like ARA, you should be using an
>> SMBus 2.0+ compatible bus multiplexer. muxes like the pca954x do not
>> automatically select the bus segment that the ARA is destined for. It
>> usually is more efficient to only request the data you need from each
>> device, rather than checking every segment on each interrupt for the
> We are not talking about checking every segment on every interrupt, we
> are talking about checking the segments indicated by the INTx bits in
> the control register.
>
>> cause, one could implement a delayed worker to schedule
>> checking+clearing the interrupt at a time when the bus is selected for
>> use by another slave device on that segment when possible. This will
>> reduce the number of bus setup operations per transfer to slaves on
>> deeper busses, reducing your i2c latency.
> i2c traffic scheduling does not exist in linux to the best of my knowledge,
> it's all handled with a simple mutex AFAIK. So, while traffic scheduling
> is an interesting problem, I think it is out of scope at this time.
>
> If you happen to have a pca954x mux (with irq support) and happen to
> have devices behind it that needs ARA support, I just don't see how any
> of the above is relevant. Yes, it could be more efficient to have the
> hardware done differently, but that is in many cases not a possibility.
> You have to make do with what you have, and if that costs latency, then
> there is little to do about that. You only have to make the new stuff
> optional so that old working setups don't suffer.
Yes traffic scheduling of I2C transactions is out of the scope of the
current problem but I thought that it may be useful to bring it up.
>>>> BTW, why do you need special treatment for multiple segments? Will it not
>>>> simply have an ARA appear on whatever i2c bus the device sits on? And if
>>>> something requests to send an ARA message on a bus that happens to be a
>>>> muxed segment, my mental picture is that the mux will be operated as usual
>>>> so that the ARA appears on the muxed segment. Maybe I'm missing something?
>>>
>>> My think was the following.
>>> When the SMBALERT is asserted a ARA needs to be sent by the master.
>>> If the device sending the SMBALERT is behind a mux when need to know which segment of the bus to enable.
>>> Using shared interrupts should work I think, but you have to iterate thru each bus segment.
>>> If the alert device is nested behind a couple of muxes this could get expensive.
>>> But yeah otherwise I think the correct mux segment will get enabled automatically.
>>> The current SMBALERT driver only seems to attached to the root i2c adapter.
>> Using ARA in a multi mux environment is very expensive, setting up
>> each mux segment and then sending ARA will consume more time than it
>> would take if the mux structure was optimized to service devices on
>> similar busses, reducing the setup operation count. ARA was only
>> implemented on the root bus due to the design of the arbiter and mux
>> cannot guarantee that you are the only owner of the bus when you are
>> disconnected.
> ARA handling should take the irq register of the pca95x into account and
> only send ARAs to indicated mux segments. If more than one segment needs
> servicing, then of course one ARA for each segment needs to be sent. If
> someone builds hardware like this and then expect no latency, that someone
> will hopefully at least learn something.
>
> I do not know how this fits with the existing ARA handling (it probably
> doesn't), but what needs to happen is fairly easy to picture.
>
>>    You would also need to handle the cases where segments
>> lock up and must be released using the bus reset mechanism, this
>> resets the IRQs as well. The added cost of a I2C read to coincide the
>> write operation to the mux when the irq pin is asserted is
>> unacceptable for low latency applications.
> I bet there are a lot of corner cases, and yes, this added cost has to be
> optional. Perhaps with each mux child segment opting in for ARA support?
> If a mux then has a way to determine that an ARA isn't needed for some of
> its child segments (by reading some status register), then linux is free
> to not send ARAs there. And more importantly, if no mux child segment opts
> in for ARA support, it should be possible to preserve current behavior...
I agree with making ARA handling for child bus segments optional based
on the need of ARA or other SMBus features. We have needed IRQ support
for muxes for a while now, I was always concerned about dead locks
between setting up the bus and servicing the IRQ. The current patch v6
0/3 appears to handle child IRQs without selecting the bus associated
with it. Is the plan to have the irq handling of the child require an
i2c transaction on its registered bus, which will in turn select that
bus? I also thought i2c transactions were not allowed inside an IRQ
handler? Could a delayed work struct could be used to fire off the read
transaction that would then trigger the nested IRQs (based on the set
bits)? Also the PCA954x has an internal OR of the INT signals coming
into it, not an AND like the patch says. Before allowing IRQs to be
handled by the assertion of the active low INT out signal from the
PCA954x all child interrupts must be de-asserted else the IRQ will
always be set. You could read the value of the interrupt register to see
if all bits are clear before allowing unmasking of the devices interrupts.
>
> Cheers,
> peda
>



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 213 bytes --]

Re: [PATCH v5 5/5] i2c: mux: pca954x: Add irq-mask-enable to delay enabling irqs

[Peter Rosin]

On 2017-01-25 10:17, Danielle Costantino wrote:
> 
> 
> On 01/25/2017 12:15 AM, Peter Rosin wrote:
>> On 2017-01-25 04:50, Danielle Costantino wrote:
>>> On Mon, Jan 23, 2017 at 1:02 AM, Phil Reid <preid@electromag.com.au> wrote:
>>>> On 20/01/2017 06:56, Peter Rosin wrote:
>>>>> On 2017-01-19 08:48, Phil Reid wrote:
>>>>>> On 18/01/2017 20:19, Peter Rosin wrote:
>>>>>>> On 2017-01-17 09:00, Phil Reid wrote:
>>>>
>>>> [snip]
>>>>
>>>>
>>>>>>> Hmm, this whole thing is fiddly and while it solves your problem it doesn't
>>>>>>> allow for solving the more general problem when there are "problematic"
>>>>>>> devices mixed with other devices. At least, I don't see it. And the
>>>>>>> limitations we are walking into with tracking number of enables etc suggests
>>>>>>> that we are attacking this at the wrong level. Maybe you should try to work
>>>>>>> around the hw limitations not in the pca954x driver, but in the irq core?
>>>>>>
>>>>>> I'm looking at the option of getting the hardware changed to not route
>>>>>> the irq for my chips thru the i2c mux. Fortunately the hardware is going thru a
>>>>>> revision for some other changes. Messing with the irq core sounds dangerous
>>>>>> with my level of knowledge.
>>>>>
>>>>> Yeah, but I bet you'd get some attention from people with more irq
>>>>> experience. That can't be bad :-)
>>>>>
>>>>>> The other way I think I can tackle it after reading the datasheet for the ltc1760 is that
>>>>>> it'll deassert it's irq (smbalert) line when the host sends a ARA request on the bus segment.
>>>>>> There's a driver in the kernel for this already, but it's not DT enable and doesn't
>>>>>> handle multiple bus segments. I'll have a look at that as well.
>>>>>> Pretty sure it would need the mux to become an irq parent as per patch 1-3 of this series.
>>>>>> This would be so the system can figure out which segment to do the poll on.
>>>>>
>>>>> Yeah sounds neater. It has the slight drawback that it may not work
>>>>> for pure i2c buses since it an SMB thing??
>>> If you need to send SMBus commands like ARA, you should be using an
>>> SMBus 2.0+ compatible bus multiplexer. muxes like the pca954x do not
>>> automatically select the bus segment that the ARA is destined for. It
>>> usually is more efficient to only request the data you need from each
>>> device, rather than checking every segment on each interrupt for the
>> We are not talking about checking every segment on every interrupt, we
>> are talking about checking the segments indicated by the INTx bits in
>> the control register.
>>
>>> cause, one could implement a delayed worker to schedule
>>> checking+clearing the interrupt at a time when the bus is selected for
>>> use by another slave device on that segment when possible. This will
>>> reduce the number of bus setup operations per transfer to slaves on
>>> deeper busses, reducing your i2c latency.
>> i2c traffic scheduling does not exist in linux to the best of my knowledge,
>> it's all handled with a simple mutex AFAIK. So, while traffic scheduling
>> is an interesting problem, I think it is out of scope at this time.
>>
>> If you happen to have a pca954x mux (with irq support) and happen to
>> have devices behind it that needs ARA support, I just don't see how any
>> of the above is relevant. Yes, it could be more efficient to have the
>> hardware done differently, but that is in many cases not a possibility.
>> You have to make do with what you have, and if that costs latency, then
>> there is little to do about that. You only have to make the new stuff
>> optional so that old working setups don't suffer.
> Yes traffic scheduling of I2C transactions is out of the scope of the
> current problem but I thought that it may be useful to bring it up.
>>>>> BTW, why do you need special treatment for multiple segments? Will it not
>>>>> simply have an ARA appear on whatever i2c bus the device sits on? And if
>>>>> something requests to send an ARA message on a bus that happens to be a
>>>>> muxed segment, my mental picture is that the mux will be operated as usual
>>>>> so that the ARA appears on the muxed segment. Maybe I'm missing something?
>>>>
>>>> My think was the following.
>>>> When the SMBALERT is asserted a ARA needs to be sent by the master.
>>>> If the device sending the SMBALERT is behind a mux when need to know which segment of the bus to enable.
>>>> Using shared interrupts should work I think, but you have to iterate thru each bus segment.
>>>> If the alert device is nested behind a couple of muxes this could get expensive.
>>>> But yeah otherwise I think the correct mux segment will get enabled automatically.
>>>> The current SMBALERT driver only seems to attached to the root i2c adapter.
>>> Using ARA in a multi mux environment is very expensive, setting up
>>> each mux segment and then sending ARA will consume more time than it
>>> would take if the mux structure was optimized to service devices on
>>> similar busses, reducing the setup operation count. ARA was only
>>> implemented on the root bus due to the design of the arbiter and mux
>>> cannot guarantee that you are the only owner of the bus when you are
>>> disconnected.
>> ARA handling should take the irq register of the pca95x into account and
>> only send ARAs to indicated mux segments. If more than one segment needs
>> servicing, then of course one ARA for each segment needs to be sent. If
>> someone builds hardware like this and then expect no latency, that someone
>> will hopefully at least learn something.
>>
>> I do not know how this fits with the existing ARA handling (it probably
>> doesn't), but what needs to happen is fairly easy to picture.
>>
>>>    You would also need to handle the cases where segments
>>> lock up and must be released using the bus reset mechanism, this
>>> resets the IRQs as well. The added cost of a I2C read to coincide the
>>> write operation to the mux when the irq pin is asserted is
>>> unacceptable for low latency applications.
>> I bet there are a lot of corner cases, and yes, this added cost has to be
>> optional. Perhaps with each mux child segment opting in for ARA support?
>> If a mux then has a way to determine that an ARA isn't needed for some of
>> its child segments (by reading some status register), then linux is free
>> to not send ARAs there. And more importantly, if no mux child segment opts
>> in for ARA support, it should be possible to preserve current behavior...
> I agree with making ARA handling for child bus segments optional based
> on the need of ARA or other SMBus features. We have needed IRQ support
> for muxes for a while now, I was always concerned about dead locks
> between setting up the bus and servicing the IRQ. The current patch v6
> 0/3 appears to handle child IRQs without selecting the bus associated
> with it. Is the plan to have the irq handling of the child require an
> i2c transaction on its registered bus, which will in turn select that
> bus?

You are correct in that the current proposed patches do not support
having a device being master on a mux child segment and expect that
any traffic hits the root adapter. A device has no way to lock the
mux in the correct position short of having its driver issue an
ordinary transaction (i.e. ->i2c_transfer or ->smbus_xfer) on the
adapter it sits on. And after the transaction, there is no guarantee
that the mux will stay in the desired position.

>      I also thought i2c transactions were not allowed inside an IRQ
> handler?

They are allowed in a threaded handler, which is very similar to a
delayed work. If it is not in fact exactly that?

>          Could a delayed work struct could be used to fire off the read
> transaction that would then trigger the nested IRQs (based on the set
> bits)? Also the PCA954x has an internal OR of the INT signals coming
> into it, not an AND like the patch says.

All INT signals on the pca954x are active-low. The only sensible way for
the mux to combine its active-low inputs into an active-low output is
with an AND gate. If any of the inputs fire, you want the output to
also fire. So, I'd say that commit message is fine.

>                                          Before allowing IRQs to be
> handled by the assertion of the active low INT out signal from the
> PCA954x all child interrupts must be de-asserted else the IRQ will
> always be set. You could read the value of the interrupt register to see
> if all bits are clear before allowing unmasking of the devices interrupts.

This is not possible with the pca954x. There is no way to prevent them
from asserting their output if one of their inputs is asserted. Which
means that if the irq is shared, and if something else have activated
it, there will be an irq flood. This is exactly the kind of trouble
Phil is trying to work around since his chargers continuously fire
interrupts when there is no driver to quiet them down.

I don't know if going with ARAs can help him, but I think it should be
possible. I also don't know how difficult it is going to be to make
this work and also be compatible with whatever ARA support there is
already. I'm not familiar with how the current ARA support works...

Cheers,
peda

Re: [PATCH v5 5/5] i2c: mux: pca954x: Add irq-mask-enable to delay enabling irqs

[Phil Reid]

On 25/01/2017 19:30, Peter Rosin wrote:
> On 2017-01-25 10:17, Danielle Costantino wrote:
>>
>>
>> On 01/25/2017 12:15 AM, Peter Rosin wrote:
>>> On 2017-01-25 04:50, Danielle Costantino wrote:
>>>> On Mon, Jan 23, 2017 at 1:02 AM, Phil Reid <preid@electromag.com.au> wrote:
>>>>> On 20/01/2017 06:56, Peter Rosin wrote:
>>>>>> On 2017-01-19 08:48, Phil Reid wrote:
>>>>>>> On 18/01/2017 20:19, Peter Rosin wrote:
>>>>>>>> On 2017-01-17 09:00, Phil Reid wrote:
>>>>>
>>>>> [snip]
>>>>>
>>>>>
>>>>>>>> Hmm, this whole thing is fiddly and while it solves your problem it doesn't
>>>>>>>> allow for solving the more general problem when there are "problematic"
>>>>>>>> devices mixed with other devices. At least, I don't see it. And the
>>>>>>>> limitations we are walking into with tracking number of enables etc suggests
>>>>>>>> that we are attacking this at the wrong level. Maybe you should try to work
>>>>>>>> around the hw limitations not in the pca954x driver, but in the irq core?
>>>>>>>
>>>>>>> I'm looking at the option of getting the hardware changed to not route
>>>>>>> the irq for my chips thru the i2c mux. Fortunately the hardware is going thru a
>>>>>>> revision for some other changes. Messing with the irq core sounds dangerous
>>>>>>> with my level of knowledge.
>>>>>>
>>>>>> Yeah, but I bet you'd get some attention from people with more irq
>>>>>> experience. That can't be bad :-)
>>>>>>
>>>>>>> The other way I think I can tackle it after reading the datasheet for the ltc1760 is that
>>>>>>> it'll deassert it's irq (smbalert) line when the host sends a ARA request on the bus segment.
>>>>>>> There's a driver in the kernel for this already, but it's not DT enable and doesn't
>>>>>>> handle multiple bus segments. I'll have a look at that as well.
>>>>>>> Pretty sure it would need the mux to become an irq parent as per patch 1-3 of this series.
>>>>>>> This would be so the system can figure out which segment to do the poll on.
>>>>>>
>>>>>> Yeah sounds neater. It has the slight drawback that it may not work
>>>>>> for pure i2c buses since it an SMB thing??
>>>> If you need to send SMBus commands like ARA, you should be using an
>>>> SMBus 2.0+ compatible bus multiplexer. muxes like the pca954x do not
>>>> automatically select the bus segment that the ARA is destined for. It
>>>> usually is more efficient to only request the data you need from each
>>>> device, rather than checking every segment on each interrupt for the
>>> We are not talking about checking every segment on every interrupt, we
>>> are talking about checking the segments indicated by the INTx bits in
>>> the control register.
>>>
>>>> cause, one could implement a delayed worker to schedule
>>>> checking+clearing the interrupt at a time when the bus is selected for
>>>> use by another slave device on that segment when possible. This will
>>>> reduce the number of bus setup operations per transfer to slaves on
>>>> deeper busses, reducing your i2c latency.
>>> i2c traffic scheduling does not exist in linux to the best of my knowledge,
>>> it's all handled with a simple mutex AFAIK. So, while traffic scheduling
>>> is an interesting problem, I think it is out of scope at this time.
>>>
>>> If you happen to have a pca954x mux (with irq support) and happen to
>>> have devices behind it that needs ARA support, I just don't see how any
>>> of the above is relevant. Yes, it could be more efficient to have the
>>> hardware done differently, but that is in many cases not a possibility.
>>> You have to make do with what you have, and if that costs latency, then
>>> there is little to do about that. You only have to make the new stuff
>>> optional so that old working setups don't suffer.
>> Yes traffic scheduling of I2C transactions is out of the scope of the
>> current problem but I thought that it may be useful to bring it up.
>>>>>> BTW, why do you need special treatment for multiple segments? Will it not
>>>>>> simply have an ARA appear on whatever i2c bus the device sits on? And if
>>>>>> something requests to send an ARA message on a bus that happens to be a
>>>>>> muxed segment, my mental picture is that the mux will be operated as usual
>>>>>> so that the ARA appears on the muxed segment. Maybe I'm missing something?
>>>>>
>>>>> My think was the following.
>>>>> When the SMBALERT is asserted a ARA needs to be sent by the master.
>>>>> If the device sending the SMBALERT is behind a mux when need to know which segment of the bus to enable.
>>>>> Using shared interrupts should work I think, but you have to iterate thru each bus segment.
>>>>> If the alert device is nested behind a couple of muxes this could get expensive.
>>>>> But yeah otherwise I think the correct mux segment will get enabled automatically.
>>>>> The current SMBALERT driver only seems to attached to the root i2c adapter.
>>>> Using ARA in a multi mux environment is very expensive, setting up
>>>> each mux segment and then sending ARA will consume more time than it
>>>> would take if the mux structure was optimized to service devices on
>>>> similar busses, reducing the setup operation count. ARA was only
>>>> implemented on the root bus due to the design of the arbiter and mux
>>>> cannot guarantee that you are the only owner of the bus when you are
>>>> disconnected.
>>> ARA handling should take the irq register of the pca95x into account and
>>> only send ARAs to indicated mux segments. If more than one segment needs
>>> servicing, then of course one ARA for each segment needs to be sent. If
>>> someone builds hardware like this and then expect no latency, that someone
>>> will hopefully at least learn something.
>>>
>>> I do not know how this fits with the existing ARA handling (it probably
>>> doesn't), but what needs to happen is fairly easy to picture.
>>>
>>>>    You would also need to handle the cases where segments
>>>> lock up and must be released using the bus reset mechanism, this
>>>> resets the IRQs as well. The added cost of a I2C read to coincide the
>>>> write operation to the mux when the irq pin is asserted is
>>>> unacceptable for low latency applications.
>>> I bet there are a lot of corner cases, and yes, this added cost has to be
>>> optional. Perhaps with each mux child segment opting in for ARA support?

Yes my plan was to opt in.
I was things an optional dt entry on the bus segment.
something like this

i2c-switch@74 {
	compatible = "nxp,pca9548";
	#address-cells = <1>;
	#size-cells = <0>;
	reg = <0x74>;

	interrupt-parent = <&ipic>;
	interrupts = <17 IRQ_TYPE_LEVEL_LOW>;
	interrupt-controller;
	#interrupt-cells = <2>;

	i2c@2 {
		#address-cells = <1>;
		#size-cells = <0>;
		reg = <2>;
		smb-alert = <&i2c-switch 1 IRQ_TYPE_LEVEL_HIGH>;
		eeprom@54 {...};
	};
	i2c@4 {
		#address-cells = <1>;
		#size-cells = <0>;
		reg = <2>;
		eeprom@54 {...};
	};
};


>>> If a mux then has a way to determine that an ARA isn't needed for some of
>>> its child segments (by reading some status register), then linux is free
>>> to not send ARAs there. And more importantly, if no mux child segment opts
>>> in for ARA support, it should be possible to preserve current behavior...
>> I agree with making ARA handling for child bus segments optional based
>> on the need of ARA or other SMBus features. We have needed IRQ support
>> for muxes for a while now, I was always concerned about dead locks
>> between setting up the bus and servicing the IRQ. The current patch v6
>> 0/3 appears to handle child IRQs without selecting the bus associated
>> with it. Is the plan to have the irq handling of the child require an
>> i2c transaction on its registered bus, which will in turn select that
>> bus?
>
> You are correct in that the current proposed patches do not support
> having a device being master on a mux child segment and expect that
> any traffic hits the root adapter. A device has no way to lock the
> mux in the correct position short of having its driver issue an
> ordinary transaction (i.e. ->i2c_transfer or ->smbus_xfer) on the
> adapter it sits on. And after the transaction, there is no guarantee
> that the mux will stay in the desired position.

There are SMBus mechanisms for alert the host.
SMBAlert and "Host Notify Protocol".
I just want to make sure where talking about the same thing.

SMBAlert, device asserts ALERT line and then the host polls the i2c bus using an ARA request.
With this method the host can ensure the correct segment is selected before sending the ARA.
The device is always a slave.

"Host Notify Protocol": the devices becomes an i2c master and sends data unsolicited.
No irq are necessary from what I can see, I don't have any devices using this method.
Muxes will get in the way of this one unless they are aware of the protocol.

My work only address SMBAlert and ARA.

>
>>      I also thought i2c transactions were not allowed inside an IRQ
>> handler?
>
> They are allowed in a threaded handler, which is very similar to a
> delayed work. If it is not in fact exactly that?
>
I think that's how they work. There are other devices doing basically the same thing
in an threaded IRQ handler. i2c gpio's with irq support for example, eg: mcp23s08

>>          Could a delayed work struct could be used to fire off the read
>> transaction that would then trigger the nested IRQs (based on the set
>> bits)? Also the PCA954x has an internal OR of the INT signals coming
>> into it, not an AND like the patch says.
>
> All INT signals on the pca954x are active-low. The only sensible way for
> the mux to combine its active-low inputs into an active-low output is
> with an AND gate. If any of the inputs fire, you want the output to
> also fire. So, I'd say that commit message is fine.
Yes.

>
>>                                          Before allowing IRQs to be
>> handled by the assertion of the active low INT out signal from the
>> PCA954x all child interrupts must be de-asserted else the IRQ will
>> always be set. You could read the value of the interrupt register to see
>> if all bits are clear before allowing unmasking of the devices interrupts.
>
> This is not possible with the pca954x. There is no way to prevent them
> from asserting their output if one of their inputs is asserted. Which
> means that if the irq is shared, and if something else have activated
> it, there will be an irq flood. This is exactly the kind of trouble
> Phil is trying to work around since his chargers continuously fire
> interrupts when there is no driver to quiet them down.
>
> I don't know if going with ARAs can help him, but I think it should be
> possible. I also don't know how difficult it is going to be to make
> this work and also be compatible with whatever ARA support there is
> already. I'm not familiar with how the current ARA support works...
>

Hopefully I'll get to looking at this next week to see how feasible it is.



-- 
Regards
Phil Reid