[PATCH can-next 0/5] m_can, tcan4x5x: device-specific interrupt handling

[PATCH can-next 0/5] m_can, tcan4x5x: device-specific interrupt handling

[Torin Cooper-Bennun]

Hi Marc and list,

This series is a follow-up to the RFC here:
  https://lore.kernel.org/linux-can/20210514121946.2344901-1-torin@maxiluxsystems.com/

In this series we enable M_CAN-based devices to implement their own
device-specific interrupt handling, and add such handling for tcan4x5x.

In 1, we replace the clear_interrupts() m_can_ops callback with
handle_interrupts(), which returns irqreturn_t and has an extra
parameter, clear_only, which is used if it isn't necessary to handle
device interrupts, only clear them.

In 2, we use the new infrastructure in m_can_isr(). If M_CAN core
interrupts are handled, we still only clear device interrupts.
Otherwise, we try to handle any pending device interrupts.

In 3-5, we clean up interrupt enabling and clearing in tcan4x5x, and
handle device interrupts appropriately. We specifically look for fatal
errors arising from transceiver or power, and SPI errors, which are not
necessarily fatal, but are dangerous!

TCAN4550 shutdown is attempted by setting the device into standby mode.
There is probably a better way, but I understand we are limited by being
in the ISR context.

The patches are based on linux-can-next/testing.

Torin Cooper-Bennun (5):
  can: m_can: ops: clear_interrupts -> handle_dev_interrupts
  can: m_can: m_can_isr(): handle device-specific interrupts
  can: tcan4x5x: tcan4x5x_clear_interrupts(): rm unnecessary reg clears
  can: tcan4x5x: only enable useful device interrupts
  can: tcan4x5x: implement handling of device interrupts

 drivers/net/can/m_can/m_can.c         | 21 +++++++----
 drivers/net/can/m_can/m_can.h         |  4 +-
 drivers/net/can/m_can/tcan4x5x-core.c | 71 ++++++++++++++++++++++++++++-------
 3 files changed, 75 insertions(+), 21 deletions(-)

[PATCH can-next 1/5] can: m_can: ops: clear_interrupts -> handle_dev_interrupts

[Torin Cooper-Bennun]

M_CAN-based devices such as TI TCAN4550 have device-specific interrupts
which are not part of the M_CAN core, but are signaled on the same
interrupt pin. Therefore, replace the clear_interrupts callback with
handle_dev_interrupts, which can handle and clear interrupts, returning
irqreturn_t.

The clear_only argument is added to retain the option of clearing
interrupts without any handling, for the purpose of saving overhead due
to register accesses.

Signed-off-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
---
 drivers/net/can/m_can/m_can.c         | 4 ++--
 drivers/net/can/m_can/m_can.h         | 4 +++-
 drivers/net/can/m_can/tcan4x5x-core.c | 9 ++++++++-
 3 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 34073cd077e4..fa853201d2c4 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1044,8 +1044,8 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
 	if (ir & IR_ALL_INT)
 		m_can_write(cdev, M_CAN_IR, ir);
 
-	if (cdev->ops->clear_interrupts)
-		cdev->ops->clear_interrupts(cdev);
+	if (cdev->ops->handle_dev_interrupts)
+		cdev->ops->handle_dev_interrupts(cdev, true);
 
 	/* schedule NAPI in case of
 	 * - rx IRQ
diff --git a/drivers/net/can/m_can/m_can.h b/drivers/net/can/m_can/m_can.h
index ace071c3e58c..81d201aa5af2 100644
--- a/drivers/net/can/m_can/m_can.h
+++ b/drivers/net/can/m_can/m_can.h
@@ -28,6 +28,7 @@
 #include <linux/iopoll.h>
 #include <linux/can/dev.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/irqreturn.h>
 
 /* m_can lec values */
 enum m_can_lec_type {
@@ -61,7 +62,8 @@ struct mram_cfg {
 struct m_can_classdev;
 struct m_can_ops {
 	/* Device specific call backs */
-	int (*clear_interrupts)(struct m_can_classdev *cdev);
+	irqreturn_t (*handle_dev_interrupts)(struct m_can_classdev *cdev,
+					     bool clear_only);
 	u32 (*read_reg)(struct m_can_classdev *cdev, int reg);
 	int (*write_reg)(struct m_can_classdev *cdev, int reg, int val);
 	u32 (*read_fifo)(struct m_can_classdev *cdev, int addr_offset);
diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index 4147cecfbbd6..b4aeab10d62f 100644
--- a/drivers/net/can/m_can/tcan4x5x-core.c
+++ b/drivers/net/can/m_can/tcan4x5x-core.c
@@ -221,6 +221,13 @@ static int tcan4x5x_clear_interrupts(struct m_can_classdev *cdev)
 				       TCAN4X5X_CLEAR_ALL_INT);
 }
 
+static irqreturn_t tcan4x5x_handle_dev_interrupts(struct m_can_classdev *cdev,
+						  bool clear_only)
+{
+	tcan4x5x_clear_interrupts(cdev);
+	return IRQ_NONE;
+}
+
 static int tcan4x5x_init(struct m_can_classdev *cdev)
 {
 	struct tcan4x5x_priv *tcan4x5x = cdev_to_priv(cdev);
@@ -304,7 +311,7 @@ static struct m_can_ops tcan4x5x_ops = {
 	.write_reg = tcan4x5x_write_reg,
 	.write_fifo = tcan4x5x_write_fifo,
 	.read_fifo = tcan4x5x_read_fifo,
-	.clear_interrupts = tcan4x5x_clear_interrupts,
+	.handle_dev_interrupts = tcan4x5x_handle_dev_interrupts,
 };
 
 static int tcan4x5x_can_probe(struct spi_device *spi)
-- 
2.30.2

[PATCH can-next 2/5] can: m_can: m_can_isr(): handle device-specific interrupts

[Torin Cooper-Bennun]

Device-specific interrupts are handled, if no M_CAN core interrupts were
handled in the ISR call.

The patch also improves the flow at the start of m_can_isr(), removing a
conditional which always evaluates to true, and improves comments.

Signed-off-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
---
 drivers/net/can/m_can/m_can.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index fa853201d2c4..3bc957da06f7 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -1033,17 +1033,24 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
 	struct net_device *dev = (struct net_device *)dev_id;
 	struct m_can_classdev *cdev = netdev_priv(dev);
 	u32 ir;
+	irqreturn_t irq_ret = IRQ_NONE;
 
 	if (pm_runtime_suspended(cdev->dev))
 		return IRQ_NONE;
+
 	ir = m_can_read(cdev, M_CAN_IR);
-	if (!ir)
-		return IRQ_NONE;
 
-	/* ACK all irqs */
-	if (ir & IR_ALL_INT)
-		m_can_write(cdev, M_CAN_IR, ir);
+	if (!ir) {
+		/* Handle device-specific interrupts */
+		if (cdev->ops->handle_dev_interrupts)
+			irq_ret = cdev->ops->handle_dev_interrupts(cdev, false);
+		return irq_ret;
+	}
+
+	/* ACK M_CAN interrupts */
+	m_can_write(cdev, M_CAN_IR, ir);
 
+	/* ACK device-specific interrupts */
 	if (cdev->ops->handle_dev_interrupts)
 		cdev->ops->handle_dev_interrupts(cdev, true);
 
-- 
2.30.2

[PATCH can-next 3/5] can: tcan4x5x: tcan4x5x_clear_interrupts(): rm unnecessary reg clears

[Torin Cooper-Bennun]

Remove clear of reg 0x0824 (TCAN4X5X_MCAN_INT_REG). It has no effect, as
this is a read-only copy of the M_CAN core interrupt register.

Remove clear of reg 0x0010 (TCAN4X5X_ERROR_STATUS). The reg is not
documented in the TCAN4550 datasheet. It is listed in the official TI
demo program headers, but is not used.

Signed-off-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
---
 drivers/net/can/m_can/tcan4x5x-core.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index b4aeab10d62f..a938dbc617e3 100644
--- a/drivers/net/can/m_can/tcan4x5x-core.c
+++ b/drivers/net/can/m_can/tcan4x5x-core.c
@@ -207,17 +207,7 @@ static int tcan4x5x_clear_interrupts(struct m_can_classdev *cdev)
 	if (ret)
 		return ret;
 
-	ret = tcan4x5x_write_tcan_reg(cdev, TCAN4X5X_MCAN_INT_REG,
-				      TCAN4X5X_ENABLE_MCAN_INT);
-	if (ret)
-		return ret;
-
-	ret = tcan4x5x_write_tcan_reg(cdev, TCAN4X5X_INT_FLAGS,
-				      TCAN4X5X_CLEAR_ALL_INT);
-	if (ret)
-		return ret;
-
-	return tcan4x5x_write_tcan_reg(cdev, TCAN4X5X_ERROR_STATUS,
+	return tcan4x5x_write_tcan_reg(cdev, TCAN4X5X_INT_FLAGS,
 				       TCAN4X5X_CLEAR_ALL_INT);
 }
 
-- 
2.30.2

[PATCH can-next 4/5] can: tcan4x5x: only enable useful device interrupts

[Torin Cooper-Bennun]

Aside from TCAN4X5X_CANINT_INT_EN, the masks used to write to the IE
register (0x0830) were all reserved fields. Remove these. Remove CANINT
also: it indicates a CAN wake event, which is not processed by the
driver.

Instead, enable optional interrupts that indicate power and transceiver
failures. We are able to shut down the device if these interrupts are
handled.

Signed-off-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
---
 drivers/net/can/m_can/tcan4x5x-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index a938dbc617e3..a300a14dc5de 100644
--- a/drivers/net/can/m_can/tcan4x5x-core.c
+++ b/drivers/net/can/m_can/tcan4x5x-core.c
@@ -40,8 +40,8 @@
 #define TCAN4X5X_BUS_FAULT BIT(4)
 #define TCAN4X5X_MCAN_INT BIT(1)
 #define TCAN4X5X_ENABLE_TCAN_INT \
-	(TCAN4X5X_MCAN_INT | TCAN4X5X_BUS_FAULT | \
-	 TCAN4X5X_CANBUS_ERR_INT_EN | TCAN4X5X_CANINT_INT_EN)
+	(TCAN4X5X_UVSUP_INT_EN | TCAN4X5X_UVIO_INT_EN | TCAN4X5X_TSD_INT_EN | \
+	 TCAN4X5X_ECCERR_INT_EN | TCAN4X5X_CANDOM_INT_EN)
 
 /* MCAN Interrupt bits */
 #define TCAN4X5X_MCAN_IR_ARA BIT(29)
-- 
2.30.2

[PATCH can-next 5/5] can: tcan4x5x: implement handling of device interrupts

[Torin Cooper-Bennun]

Handle power, transceiver and SPI failures by printing a useful error
message (multiple simultaneous failures are not logged) and disabling
the TCAN4550 by setting it to standby mode.

Additionally, print an error message if any regmap access fails in the
tcan4x5x_handle_dev_interrupts() call.

Signed-off-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
---
 drivers/net/can/m_can/tcan4x5x-core.c | 50 ++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index a300a14dc5de..2016a4b54a44 100644
--- a/drivers/net/can/m_can/tcan4x5x-core.c
+++ b/drivers/net/can/m_can/tcan4x5x-core.c
@@ -38,6 +38,7 @@
 #define TCAN4X5X_CANDOM_INT_EN BIT(8)
 #define TCAN4X5X_CANBUS_ERR_INT_EN BIT(5)
 #define TCAN4X5X_BUS_FAULT BIT(4)
+#define TCAN4X5X_SPIERR_INT BIT(3)
 #define TCAN4X5X_MCAN_INT BIT(1)
 #define TCAN4X5X_ENABLE_TCAN_INT \
 	(TCAN4X5X_UVSUP_INT_EN | TCAN4X5X_UVIO_INT_EN | TCAN4X5X_TSD_INT_EN | \
@@ -214,7 +215,54 @@ static int tcan4x5x_clear_interrupts(struct m_can_classdev *cdev)
 static irqreturn_t tcan4x5x_handle_dev_interrupts(struct m_can_classdev *cdev,
 						  bool clear_only)
 {
-	tcan4x5x_clear_interrupts(cdev);
+	struct tcan4x5x_priv *priv = cdev_to_priv(cdev);
+	int err = 0;
+	irqreturn_t handled = IRQ_NONE;
+
+	if (!clear_only) {
+		u32 ir = 0;
+		const char *fail_str = "";
+
+		err = regmap_read(priv->regmap, TCAN4X5X_INT_FLAGS, &ir);
+		if (err)
+			goto exit_regmap_failure;
+
+		handled = IRQ_HANDLED;
+
+		if (ir & TCAN4X5X_UVSUP_INT_EN)
+			fail_str = "supply under-voltage (UVSUP)";
+		else if (ir & TCAN4X5X_UVIO_INT_EN)
+			fail_str = "I/O under-voltage (UVIO)";
+		else if (ir & TCAN4X5X_TSD_INT_EN)
+			fail_str = "thermal shutdown (TSD)";
+		else if (ir & TCAN4X5X_ECCERR_INT_EN)
+			fail_str = "uncorrectable ECC error (ECCERR)";
+		else if (ir & TCAN4X5X_CANDOM_INT_EN)
+			fail_str = "CAN stuck dominant (CANDOM)";
+		else if (ir & TCAN4X5X_SPIERR_INT)
+			fail_str = "SPI error (SPIERR)";
+		else
+			handled = IRQ_NONE;
+
+		if (handled == IRQ_HANDLED) {
+			netdev_err(cdev->net, "%s: device is disabled.\n",
+				   fail_str);
+			err = regmap_update_bits(priv->regmap, TCAN4X5X_CONFIG,
+						 TCAN4X5X_MODE_SEL_MASK,
+						 TCAN4X5X_MODE_STANDBY);
+			if (err)
+				goto exit_regmap_failure;
+		}
+	}
+
+	err = tcan4x5x_clear_interrupts(cdev);
+	if (err)
+		goto exit_regmap_failure;
+
+	return handled;
+
+exit_regmap_failure:
+	netdev_err(cdev->net, "regmap access failed in IRQ handler.\n");
 	return IRQ_NONE;
 }
 
-- 
2.30.2

Re: [PATCH can-next 2/5] can: m_can: m_can_isr(): handle device-specific interrupts

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2119 bytes --]

On 26.05.2021 13:47:44, Torin Cooper-Bennun wrote:
> Device-specific interrupts are handled, if no M_CAN core interrupts were
> handled in the ISR call.

In case there are both core and device specific interrupts the kernel
IRQ handler will call the ISR a 2nd time - should be OK.

> The patch also improves the flow at the start of m_can_isr(), removing a
> conditional which always evaluates to true, and improves comments.
> 
> Signed-off-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
> ---
>  drivers/net/can/m_can/m_can.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index fa853201d2c4..3bc957da06f7 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -1033,17 +1033,24 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
>  	struct net_device *dev = (struct net_device *)dev_id;
>  	struct m_can_classdev *cdev = netdev_priv(dev);
>  	u32 ir;
> +	irqreturn_t irq_ret = IRQ_NONE;

nitpick: please move before the "u32 ir;"

>  
>  	if (pm_runtime_suspended(cdev->dev))
>  		return IRQ_NONE;
> +
>  	ir = m_can_read(cdev, M_CAN_IR);
> -	if (!ir)
> -		return IRQ_NONE;
>  
> -	/* ACK all irqs */
> -	if (ir & IR_ALL_INT)
> -		m_can_write(cdev, M_CAN_IR, ir);
> +	if (!ir) {
> +		/* Handle device-specific interrupts */
> +		if (cdev->ops->handle_dev_interrupts)
> +			irq_ret = cdev->ops->handle_dev_interrupts(cdev, false);
> +		return irq_ret;
> +	}
> +
> +	/* ACK M_CAN interrupts */
> +	m_can_write(cdev, M_CAN_IR, ir);
>  
> +	/* ACK device-specific interrupts */
>  	if (cdev->ops->handle_dev_interrupts)
>  		cdev->ops->handle_dev_interrupts(cdev, true);

Why do you call a 2nd time the handle_dev_interrupts() callback?

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH can-next 5/5] can: tcan4x5x: implement handling of device interrupts

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3674 bytes --]

On 26.05.2021 13:47:47, Torin Cooper-Bennun wrote:
> Handle power, transceiver and SPI failures by printing a useful error
> message (multiple simultaneous failures are not logged) and disabling
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Is this a limitation of your code or the tcan core?

> the TCAN4550 by setting it to standby mode.
> 
> Additionally, print an error message if any regmap access fails in the
> tcan4x5x_handle_dev_interrupts() call.
> 
> Signed-off-by: Torin Cooper-Bennun <torin@maxiluxsystems.com>
> ---
>  drivers/net/can/m_can/tcan4x5x-core.c | 50 ++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> index a300a14dc5de..2016a4b54a44 100644
> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> @@ -38,6 +38,7 @@
>  #define TCAN4X5X_CANDOM_INT_EN BIT(8)
>  #define TCAN4X5X_CANBUS_ERR_INT_EN BIT(5)
>  #define TCAN4X5X_BUS_FAULT BIT(4)
> +#define TCAN4X5X_SPIERR_INT BIT(3)
>  #define TCAN4X5X_MCAN_INT BIT(1)
>  #define TCAN4X5X_ENABLE_TCAN_INT \
>  	(TCAN4X5X_UVSUP_INT_EN | TCAN4X5X_UVIO_INT_EN | TCAN4X5X_TSD_INT_EN | \
> @@ -214,7 +215,54 @@ static int tcan4x5x_clear_interrupts(struct m_can_classdev *cdev)
>  static irqreturn_t tcan4x5x_handle_dev_interrupts(struct m_can_classdev *cdev,
>  						  bool clear_only)
>  {
> -	tcan4x5x_clear_interrupts(cdev);
> +	struct tcan4x5x_priv *priv = cdev_to_priv(cdev);
> +	int err = 0;
> +	irqreturn_t handled = IRQ_NONE;

nitpick: please make "int err" the last.

> +
> +	if (!clear_only) {
> +		u32 ir = 0;
> +		const char *fail_str = "";

nitpick: please make the u32 the last.

> +
> +		err = regmap_read(priv->regmap, TCAN4X5X_INT_FLAGS, &ir);
> +		if (err)
> +			goto exit_regmap_failure;
> +
> +		handled = IRQ_HANDLED;
> +
> +		if (ir & TCAN4X5X_UVSUP_INT_EN)
> +			fail_str = "supply under-voltage (UVSUP)";
> +		else if (ir & TCAN4X5X_UVIO_INT_EN)
> +			fail_str = "I/O under-voltage (UVIO)";
> +		else if (ir & TCAN4X5X_TSD_INT_EN)
> +			fail_str = "thermal shutdown (TSD)";
> +		else if (ir & TCAN4X5X_ECCERR_INT_EN)
> +			fail_str = "uncorrectable ECC error (ECCERR)";
> +		else if (ir & TCAN4X5X_CANDOM_INT_EN)
> +			fail_str = "CAN stuck dominant (CANDOM)";

The error message suggests, that this error can be triggered by messing
around with the CAN high/low wires. I'm not sure if it's a good idea to
shutdown the driver in this case.

> +		else if (ir & TCAN4X5X_SPIERR_INT)
> +			fail_str = "SPI error (SPIERR)";
> +		else
> +			handled = IRQ_NONE;
> +
> +		if (handled == IRQ_HANDLED) {
> +			netdev_err(cdev->net, "%s: device is disabled.\n",

Better change the error message and say that the driver is disabling the
device due to the error.

> +				   fail_str);
> +			err = regmap_update_bits(priv->regmap, TCAN4X5X_CONFIG,
> +						 TCAN4X5X_MODE_SEL_MASK,
> +						 TCAN4X5X_MODE_STANDBY);
> +			if (err)
> +				goto exit_regmap_failure;
> +		}
> +	}
> +
> +	err = tcan4x5x_clear_interrupts(cdev);
> +	if (err)
> +		goto exit_regmap_failure;
> +
> +	return handled;
> +
> +exit_regmap_failure:
> +	netdev_err(cdev->net, "regmap access failed in IRQ handler.\n");
>  	return IRQ_NONE;
>  }
>  
> -- 
> 2.30.2
> 
>

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH can-next 2/5] can: m_can: m_can_isr(): handle device-specific interrupts

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 976 bytes --]

On 26.05.2021 17:07:05, Marc Kleine-Budde wrote:
> On 26.05.2021 13:47:44, Torin Cooper-Bennun wrote:
> > +	if (!ir) {
> > +		/* Handle device-specific interrupts */
> > +		if (cdev->ops->handle_dev_interrupts)
> > +			irq_ret = cdev->ops->handle_dev_interrupts(cdev, false);
> > +		return irq_ret;
> > +	}
> > +
> > +	/* ACK M_CAN interrupts */
> > +	m_can_write(cdev, M_CAN_IR, ir);
> >  
> > +	/* ACK device-specific interrupts */
> >  	if (cdev->ops->handle_dev_interrupts)
> >  		cdev->ops->handle_dev_interrupts(cdev, true);
> 
> Why do you call a 2nd time the handle_dev_interrupts() callback?

I see, clear and no clear. Why are these two separate operations?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH can-next 0/5] m_can, tcan4x5x: device-specific interrupt handling

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1697 bytes --]

On 26.05.2021 13:47:42, Torin Cooper-Bennun wrote:
> Hi Marc and list,
> 
> This series is a follow-up to the RFC here:
>   https://lore.kernel.org/linux-can/20210514121946.2344901-1-torin@maxiluxsystems.com/
> 
> In this series we enable M_CAN-based devices to implement their own
> device-specific interrupt handling, and add such handling for tcan4x5x.
> 
> In 1, we replace the clear_interrupts() m_can_ops callback with
> handle_interrupts(), which returns irqreturn_t and has an extra
> parameter, clear_only, which is used if it isn't necessary to handle
> device interrupts, only clear them.
> 
> In 2, we use the new infrastructure in m_can_isr(). If M_CAN core
> interrupts are handled, we still only clear device interrupts.
> Otherwise, we try to handle any pending device interrupts.
> 
> In 3-5, we clean up interrupt enabling and clearing in tcan4x5x, and
> handle device interrupts appropriately. We specifically look for fatal
> errors arising from transceiver or power, and SPI errors, which are not
> necessarily fatal, but are dangerous!
> 
> TCAN4550 shutdown is attempted by setting the device into standby mode.
> There is probably a better way, but I understand we are limited by being
> in the ISR context.

Not exactly. The tcan's ISR runs in a threaded context, so you can
basically do normal SPI or regmap transactions, shut down clocks and
regulators, etc...

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH can-next 2/5] can: m_can: m_can_isr(): handle device-specific interrupts

[Torin Cooper-Bennun]

[-- Attachment #1: Type: text/plain, Size: 663 bytes --]

On Wed, May 26, 2021 at 05:07:05PM +0200, Marc Kleine-Budde wrote:
> > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> > index fa853201d2c4..3bc957da06f7 100644
> > --- a/drivers/net/can/m_can/m_can.c
> > +++ b/drivers/net/can/m_can/m_can.c
> > @@ -1033,17 +1033,24 @@ static irqreturn_t m_can_isr(int irq, void *dev_id)
> >  	struct net_device *dev = (struct net_device *)dev_id;
> >  	struct m_can_classdev *cdev = netdev_priv(dev);
> >  	u32 ir;
> > +	irqreturn_t irq_ret = IRQ_NONE;
> 
> nitpick: please move before the "u32 ir;"

ACK

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH can-next 5/5] can: tcan4x5x: implement handling of device interrupts

[Torin Cooper-Bennun]

[-- Attachment #1: Type: text/plain, Size: 1787 bytes --]

On Wed, May 26, 2021 at 05:15:59PM +0200, Marc Kleine-Budde wrote:
> On 26.05.2021 13:47:47, Torin Cooper-Bennun wrote:
> > Handle power, transceiver and SPI failures by printing a useful error
> > message (multiple simultaneous failures are not logged) and disabling
>            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> Is this a limitation of your code or the tcan core?

My code doesn't print an error message for every handled interrupt, only
the first, because it's very rare to see more than one. Perhaps it's
prudent to print a line for each handled interrupt just in case.

> >  static irqreturn_t tcan4x5x_handle_dev_interrupts(struct m_can_classdev *cdev,
> >  						  bool clear_only)
> >  {
> > -	tcan4x5x_clear_interrupts(cdev);
> > +	struct tcan4x5x_priv *priv = cdev_to_priv(cdev);
> > +	int err = 0;
> > +	irqreturn_t handled = IRQ_NONE;
> 
> nitpick: please make "int err" the last.

ACK

> 
> > +
> > +	if (!clear_only) {
> > +		u32 ir = 0;
> > +		const char *fail_str = "";
> 
> nitpick: please make the u32 the last.

ACK

> > +		else if (ir & TCAN4X5X_CANDOM_INT_EN)
> > +			fail_str = "CAN stuck dominant (CANDOM)";
> 
> The error message suggests, that this error can be triggered by messing
> around with the CAN high/low wires. I'm not sure if it's a good idea to
> shutdown the driver in this case.

ACK, but I need to test whether the device stays functional without CPU
intervention after CANDOM is asserted.

> > +		if (handled == IRQ_HANDLED) {
> > +			netdev_err(cdev->net, "%s: device is disabled.\n",
> 
> Better change the error message and say that the driver is disabling the
> device due to the error.

ACK

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH can-next 5/5] can: tcan4x5x: implement handling of device interrupts

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 2479 bytes --]

On 01.06.2021 08:50:28, Torin Cooper-Bennun wrote:
> On Wed, May 26, 2021 at 05:15:59PM +0200, Marc Kleine-Budde wrote:
> > On 26.05.2021 13:47:47, Torin Cooper-Bennun wrote:
> > > Handle power, transceiver and SPI failures by printing a useful error
> > > message (multiple simultaneous failures are not logged) and disabling
> >            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > Is this a limitation of your code or the tcan core?
> 
> My code doesn't print an error message for every handled interrupt, only
> the first, because it's very rare to see more than one. Perhaps it's
> prudent to print a line for each handled interrupt just in case.

Yes, please make it so.

> > >  static irqreturn_t tcan4x5x_handle_dev_interrupts(struct m_can_classdev *cdev,
> > >  						  bool clear_only)
> > >  {
> > > -	tcan4x5x_clear_interrupts(cdev);
> > > +	struct tcan4x5x_priv *priv = cdev_to_priv(cdev);
> > > +	int err = 0;
> > > +	irqreturn_t handled = IRQ_NONE;
> > 
> > nitpick: please make "int err" the last.
> 
> ACK
> 
> > 
> > > +
> > > +	if (!clear_only) {
> > > +		u32 ir = 0;
> > > +		const char *fail_str = "";
> > 
> > nitpick: please make the u32 the last.
> 
> ACK
> 
> > > +		else if (ir & TCAN4X5X_CANDOM_INT_EN)
> > > +			fail_str = "CAN stuck dominant (CANDOM)";
> > 
> > The error message suggests, that this error can be triggered by messing
> > around with the CAN high/low wires. I'm not sure if it's a good idea to
> > shutdown the driver in this case.
> 
> ACK, but I need to test whether the device stays functional without CPU
> intervention after CANDOM is asserted.

- Does IRQ line stay asserted if the CAN lines are still stuck dominant?

If yes, this would result in an IRQ storm, which we don't want to have.
If you/we want to handle this in a "proper" way, send a CAN error frame [1],
mask this interrupt, setup a timer/workqueue/etc and unmask it after
some 100ms.
  
[1] But I'm not sure if we have proper values for stuck dominant yet.
    https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/can/error.h#L110
  
- What happens if the stuck dominant condition is gone?

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH can-next 0/5] m_can, tcan4x5x: device-specific interrupt handling

[Torin Cooper-Bennun]

[-- Attachment #1: Type: text/plain, Size: 981 bytes --]

On Wed, May 26, 2021 at 05:20:45PM +0200, Marc Kleine-Budde wrote:
> On 26.05.2021 13:47:42, Torin Cooper-Bennun wrote:
> > TCAN4550 shutdown is attempted by setting the device into standby mode.
> > There is probably a better way, but I understand we are limited by being
> > in the ISR context.
> 
> Not exactly. The tcan's ISR runs in a threaded context, so you can
> basically do normal SPI or regmap transactions, shut down clocks and
> regulators, etc...

Got you. I keep forgetting that detail!

Would it be sufficient to change the CAN state as follows?

|	if (handled == IRQ_HANDLED) {
|		netdev_err(cdev->net,
|			   "Device is disabled by driver.\n");
|
|		cdev->can.state = CAN_STATE_STOPPED;
|
|		err = regmap_update_bits(priv->regmap, TCAN4X5X_CONFIG,
|					 TCAN4X5X_MODE_SEL_MASK,
|					 TCAN4X5X_MODE_STANDBY);
|		if (err)
|			goto exit_regmap_failure;
|	}

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH can-next 2/5] can: m_can: m_can_isr(): handle device-specific interrupts

[Torin Cooper-Bennun]

[-- Attachment #1: Type: text/plain, Size: 2027 bytes --]

On Wed, May 26, 2021 at 05:18:33PM +0200, Marc Kleine-Budde wrote:
> On 26.05.2021 17:07:05, Marc Kleine-Budde wrote:
> > On 26.05.2021 13:47:44, Torin Cooper-Bennun wrote:
> > > +	if (!ir) {
> > > +		/* Handle device-specific interrupts */
> > > +		if (cdev->ops->handle_dev_interrupts)
> > > +			irq_ret = cdev->ops->handle_dev_interrupts(cdev, false);
> > > +		return irq_ret;
> > > +	}
> > > +
> > > +	/* ACK M_CAN interrupts */
> > > +	m_can_write(cdev, M_CAN_IR, ir);
> > >  
> > > +	/* ACK device-specific interrupts */
> > >  	if (cdev->ops->handle_dev_interrupts)
> > >  		cdev->ops->handle_dev_interrupts(cdev, true);
> > 
> > Why do you call a 2nd time the handle_dev_interrupts() callback?
> 
> I see, clear and no clear. Why are these two separate operations?

As discussed here,

https://lore.kernel.org/linux-can/20210514141012.3ehw4tosog3lreq4@pengutronix.de/

you previously recommended adding a 2nd parameter the callback to
indicate that we only want to clear the device IR, not read it, in order
to save an SPI transaction when M_CAN core interrupts were already
handled.

However, quoting from your previous mail,

> On 26.05.2021 13:47:44, Torin Cooper-Bennun wrote:
> > Device-specific interrupts are handled, if no M_CAN core interrupts were
> > handled in the ISR call.
> 
> In case there are both core and device specific interrupts the kernel
> IRQ handler will call the ISR a 2nd time - should be OK.

this implies I should leave device IR totally alone unless no M_CAN
interrupts are asserted, and do the following only:

|	ir = m_can_read(cdev, M_CAN_IR);
|
|	if (!ir) {
|		/* Handle device-specific interrupts */
|		if (cdev->ops->handle_dev_interrupts)
|			irq_ret = cdev->ops->handle_dev_interrupts(cdev, false);
|		return irq_ret;
|	}
|
|	/* ACK M_CAN interrupts */
|	m_can_write(cdev, M_CAN_IR, ir);

I can probably also kill off the clear_only parameter.

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH can-next 0/5] m_can, tcan4x5x: device-specific interrupt handling

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1544 bytes --]

On 01.06.2021 09:21:07, Torin Cooper-Bennun wrote:
> On Wed, May 26, 2021 at 05:20:45PM +0200, Marc Kleine-Budde wrote:
> > On 26.05.2021 13:47:42, Torin Cooper-Bennun wrote:
> > > TCAN4550 shutdown is attempted by setting the device into standby mode.
> > > There is probably a better way, but I understand we are limited by being
> > > in the ISR context.
> > 
> > Not exactly. The tcan's ISR runs in a threaded context, so you can
> > basically do normal SPI or regmap transactions, shut down clocks and
> > regulators, etc...
> 
> Got you. I keep forgetting that detail!
> 
> Would it be sufficient to change the CAN state as follows?
> 
> |	if (handled == IRQ_HANDLED) {
> |		netdev_err(cdev->net,
> |			   "Device is disabled by driver.\n");
> |
> |		cdev->can.state = CAN_STATE_STOPPED;
> |
> |		err = regmap_update_bits(priv->regmap, TCAN4X5X_CONFIG,
> |					 TCAN4X5X_MODE_SEL_MASK,
> |					 TCAN4X5X_MODE_STANDBY);
> |		if (err)
> |			goto exit_regmap_failure;
> |	}

I think there already is a function to stop the m-can core:

| m_can_stop(struct net_device *dev)

You have remove the static to use it from the tcan4x5x, though. If this
function doesn't stop the tcan properly, you might have to add another
callback.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH can-next 2/5] can: m_can: m_can_isr(): handle device-specific interrupts

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3427 bytes --]

On 01.06.2021 09:23:38, Torin Cooper-Bennun wrote:
> On Wed, May 26, 2021 at 05:18:33PM +0200, Marc Kleine-Budde wrote:
> > On 26.05.2021 17:07:05, Marc Kleine-Budde wrote:
> > > On 26.05.2021 13:47:44, Torin Cooper-Bennun wrote:
> > > > +	if (!ir) {
> > > > +		/* Handle device-specific interrupts */
> > > > +		if (cdev->ops->handle_dev_interrupts)
> > > > +			irq_ret = cdev->ops->handle_dev_interrupts(cdev, false);
> > > > +		return irq_ret;
> > > > +	}
> > > > +
> > > > +	/* ACK M_CAN interrupts */
> > > > +	m_can_write(cdev, M_CAN_IR, ir);
> > > >  
> > > > +	/* ACK device-specific interrupts */
> > > >  	if (cdev->ops->handle_dev_interrupts)
> > > >  		cdev->ops->handle_dev_interrupts(cdev, true);
> > > 
> > > Why do you call a 2nd time the handle_dev_interrupts() callback?
> > 
> > I see, clear and no clear. Why are these two separate operations?
> 
> As discussed here,
> 
> https://lore.kernel.org/linux-can/20210514141012.3ehw4tosog3lreq4@pengutronix.de/
> 
> you previously recommended adding a 2nd parameter the callback to
> indicate that we only want to clear the device IR, not read it, in order
> to save an SPI transaction when M_CAN core interrupts were already
> handled.

ACK.

I had in mind a single call to handle_dev_interrupts().

In tcan4x5x_clear_interrupts() the original code does 4 individual reg
writes. With:

| tcan4x5x: tcan4x5x_clear_interrupts(): rm unnecessary reg clears

two of them are removed \o/. In Linux we want to avoid individual SPI
transfers at all cost, as they are quite expensive. So the SPI accesses
should be optimized for the no error use case.

If the tcan4x5x interrupts only need clearing/handling in the error
case, it might be an option to only call handle_dev_interrupts() if the
M_CAN_IR register shows no interrupts.

If both m-can and tcan interrupts are pending, in the first run only the
m-can IRQs are handled. If the IRQ line is still asserted the Linux IRQ
code should call the ISR again, no m-can IRQ, then the tcan IRQs will be
handled.

> However, quoting from your previous mail,
> 
> > On 26.05.2021 13:47:44, Torin Cooper-Bennun wrote:
> > > Device-specific interrupts are handled, if no M_CAN core interrupts were
> > > handled in the ISR call.
> > 
> > In case there are both core and device specific interrupts the kernel
> > IRQ handler will call the ISR a 2nd time - should be OK.
> 
> this implies I should leave device IR totally alone unless no M_CAN
> interrupts are asserted, and do the following only:

ACK

> |	ir = m_can_read(cdev, M_CAN_IR);
> |
> |	if (!ir) {
> |		/* Handle device-specific interrupts */
> |		if (cdev->ops->handle_dev_interrupts)
> |			irq_ret = cdev->ops->handle_dev_interrupts(cdev, false);

ACK - and the handle_dev_interrupts handler should do all necessary
things :)

> |		return irq_ret;
> |	}
> |
> |	/* ACK M_CAN interrupts */
> |	m_can_write(cdev, M_CAN_IR, ir);
> 
> I can probably also kill off the clear_only parameter.

If the tcan only needs handling/clearing in the error case, then there's
no need for a 2nd parameter.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH can-next 0/5] m_can, tcan4x5x: device-specific interrupt handling

[Torin Cooper-Bennun]

[-- Attachment #1: Type: text/plain, Size: 516 bytes --]

On Tue, Jun 01, 2021 at 11:18:47AM +0200, Marc Kleine-Budde wrote:
> I think there already is a function to stop the m-can core:
> 
> | m_can_stop(struct net_device *dev)
> 
> You have remove the static to use it from the tcan4x5x, though. If this
> function doesn't stop the tcan properly, you might have to add another
> callback.

ACK, that looks like it'll work. When I get the chance I'll do some
testing to make sure.

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH can-next 0/5] m_can, tcan4x5x: device-specific interrupt handling

[Torin Cooper-Bennun]

[-- Attachment #1: Type: text/plain, Size: 552 bytes --]

On Wed, May 26, 2021 at 01:47:42PM +0100, Torin Cooper-Bennun wrote:
> Hi Marc and list,

It's been a month since I was last able to work on this series - I don't
think I'll ever get round to finishing it. The semiconductor shortage
has hit stocks of the TCAN4550 really hard, so the project I'm working
on has shifted direction. (Good thing the MCP2518FD has such a good
kernel driver, eh Marc?)

So if anyone comes across this and wants to finish off this work, feel
free. :)

--
Regards,

Torin Cooper-Bennun
Software Engineer | maxiluxsystems.com


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]