* [PATCH v2] can: m_can: don't enable transceiver when probing
@ 2024-05-01 12:42 Martin Hundebøll
2024-05-06 10:04 ` Marc Kleine-Budde
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Martin Hundebøll @ 2024-05-01 12:42 UTC (permalink / raw
To: Chandrasekar Ramakrishnan, Marc Kleine-Budde, Vincent Mailhol,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Markus Schneider-Pargmann, Martin Hundebøll, linux-can,
netdev, linux-kernel
The m_can driver sets and clears the CCCR.INIT bit during probe (both
when testing the NON-ISO bit, and when configuring the chip). After
clearing the CCCR.INIT bit, the transceiver enters normal mode, where it
affects the CAN bus (i.e. it ACKs frames). This can cause troubles when
the m_can node is only used for monitoring the bus, as one cannot setup
listen-only mode before the device is probed.
Rework the probe flow, so that the CCCR.INIT bit is only cleared when
upping the device. First, the tcan4x5x driver is changed to stay in
standby mode during/after probe. This in turn requires changes when
setting bits in the CCCR register, as its CSR and CSA bits are always
high in standby mode.
Signed-off-by: Martin Hundebøll <martin@geanix.com>
---
Changes since v1:
* Implement Markus review comments:
- Rename m_can_cccr_wait_bits() to m_can_cccr_update_bits()
- Explicitly set CCCR_INIT bit in m_can_dev_setup()
- Revert to 5 timeouts/tries to 10
- Use m_can_config_{en|dis}able() in m_can_niso_supported()
- Revert move of call to m_can_enable_all_interrupts()
- Return -EBUSY on failure to enter normal mode
- Use tcan4x5x_clear_interrupts() in tcan4x5x_can_probe()
drivers/net/can/m_can/m_can.c | 131 +++++++++++++++-----------
drivers/net/can/m_can/tcan4x5x-core.c | 13 ++-
2 files changed, 85 insertions(+), 59 deletions(-)
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index 14b231c4d7ec..7974aaa5d8cc 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -379,38 +379,60 @@ m_can_txe_fifo_read(struct m_can_classdev *cdev, u32 fgi, u32 offset, u32 *val)
return cdev->ops->read_fifo(cdev, addr_offset, val, 1);
}
-static void m_can_config_endisable(struct m_can_classdev *cdev, bool enable)
+static bool m_can_cccr_update_bits(struct m_can_classdev *cdev, u32 mask, u32 val)
{
- u32 cccr = m_can_read(cdev, M_CAN_CCCR);
- u32 timeout = 10;
- u32 val = 0;
-
- /* Clear the Clock stop request if it was set */
- if (cccr & CCCR_CSR)
- cccr &= ~CCCR_CSR;
-
- if (enable) {
- /* enable m_can configuration */
- m_can_write(cdev, M_CAN_CCCR, cccr | CCCR_INIT);
- udelay(5);
- /* CCCR.CCE can only be set/reset while CCCR.INIT = '1' */
- m_can_write(cdev, M_CAN_CCCR, cccr | CCCR_INIT | CCCR_CCE);
- } else {
- m_can_write(cdev, M_CAN_CCCR, cccr & ~(CCCR_INIT | CCCR_CCE));
- }
+ u32 val_before = m_can_read(cdev, M_CAN_CCCR);
+ u32 val_after = (val_before & ~mask) | val;
+ size_t tries = 10;
+
+ if (!(mask & CCCR_INIT) && !(val_before & CCCR_INIT))
+ dev_warn(cdev->dev,
+ "trying to configure device when in normal mode. Expect failures\n");
+
+ /* The chip should be in standby mode when changing the CCCR register,
+ * and some chips set the CSR and CSA bits when in standby. Furthermore,
+ * the CSR and CSA bits should be written as zeros, even when they read
+ * ones.
+ */
+ val_after &= ~(CCCR_CSR | CCCR_CSA);
+
+ while (tries--) {
+ u32 val_read;
+
+ /* Write the desired value in each try, as setting some bits in
+ * the CCCR register require other bits to be set first. E.g.
+ * setting the NISO bit requires setting the CCE bit first.
+ */
+ m_can_write(cdev, M_CAN_CCCR, val_after);
+
+ val_read = m_can_read(cdev, M_CAN_CCCR) & ~(CCCR_CSR | CCCR_CSA);
- /* there's a delay for module initialization */
- if (enable)
- val = CCCR_INIT | CCCR_CCE;
-
- while ((m_can_read(cdev, M_CAN_CCCR) & (CCCR_INIT | CCCR_CCE)) != val) {
- if (timeout == 0) {
- netdev_warn(cdev->net, "Failed to init module\n");
- return;
- }
- timeout--;
- udelay(1);
+ if (val_read == val_after)
+ return true;
+
+ usleep_range(1, 5);
}
+
+ return false;
+}
+
+static void m_can_config_enable(struct m_can_classdev *cdev)
+{
+ /* CCCR_INIT must be set in order to set CCCR_CCE, but access to
+ * configuration registers should only be enabled when in standby mode,
+ * where CCCR_INIT is always set.
+ */
+ if (!m_can_cccr_update_bits(cdev, CCCR_CCE, CCCR_CCE))
+ netdev_err(cdev->net, "failed to enable configuration mode\n");
+}
+
+static void m_can_config_disable(struct m_can_classdev *cdev)
+{
+ /* Only clear CCCR_CCE, since CCCR_INIT cannot be cleared while in
+ * standby mode
+ */
+ if (!m_can_cccr_update_bits(cdev, CCCR_CCE, 0))
+ netdev_err(cdev->net, "failed to disable configuration registers\n");
}
static void m_can_interrupt_enable(struct m_can_classdev *cdev, u32 interrupts)
@@ -1403,7 +1425,7 @@ static int m_can_chip_config(struct net_device *dev)
interrupts &= ~(IR_ARA | IR_ELO | IR_DRX | IR_TEFF | IR_TFE | IR_TCF |
IR_HPM | IR_RF1F | IR_RF1W | IR_RF1N | IR_RF0F);
- m_can_config_endisable(cdev, true);
+ m_can_config_enable(cdev);
/* RX Buffer/FIFO Element Size 64 bytes data field */
m_can_write(cdev, M_CAN_RXESC,
@@ -1521,7 +1543,7 @@ static int m_can_chip_config(struct net_device *dev)
FIELD_PREP(TSCC_TCP_MASK, 0xf) |
FIELD_PREP(TSCC_TSS_MASK, TSCC_TSS_INTERNAL));
- m_can_config_endisable(cdev, false);
+ m_can_config_disable(cdev);
if (cdev->ops->init)
cdev->ops->init(cdev);
@@ -1550,6 +1572,11 @@ static int m_can_start(struct net_device *dev)
cdev->tx_fifo_putidx = FIELD_GET(TXFQS_TFQPI_MASK,
m_can_read(cdev, M_CAN_TXFQS));
+ if (!m_can_cccr_update_bits(cdev, CCCR_INIT, 0)) {
+ netdev_err(dev, "failed to enter normal mode\n");
+ return -EBUSY;
+ }
+
return 0;
}
@@ -1603,33 +1630,20 @@ static int m_can_check_core_release(struct m_can_classdev *cdev)
*/
static bool m_can_niso_supported(struct m_can_classdev *cdev)
{
- u32 cccr_reg, cccr_poll = 0;
- int niso_timeout = -ETIMEDOUT;
- int i;
+ bool niso_supported;
- m_can_config_endisable(cdev, true);
- cccr_reg = m_can_read(cdev, M_CAN_CCCR);
- cccr_reg |= CCCR_NISO;
- m_can_write(cdev, M_CAN_CCCR, cccr_reg);
+ m_can_config_enable(cdev);
- for (i = 0; i <= 10; i++) {
- cccr_poll = m_can_read(cdev, M_CAN_CCCR);
- if (cccr_poll == cccr_reg) {
- niso_timeout = 0;
- break;
- }
+ /* First try to set the NISO bit. */
+ niso_supported = m_can_cccr_update_bits(cdev, CCCR_NISO, CCCR_NISO);
- usleep_range(1, 5);
- }
+ /* Then clear the it again. */
+ if (!m_can_cccr_update_bits(cdev, CCCR_NISO, 0))
+ dev_err(cdev->dev, "failed to revert the NON-ISO bit in CCCR\n");
- /* Clear NISO */
- cccr_reg &= ~(CCCR_NISO);
- m_can_write(cdev, M_CAN_CCCR, cccr_reg);
+ m_can_config_disable(cdev);
- m_can_config_endisable(cdev, false);
-
- /* return false if time out (-ETIMEDOUT), else return true */
- return !niso_timeout;
+ return niso_supported;
}
static int m_can_dev_setup(struct m_can_classdev *cdev)
@@ -1694,8 +1708,12 @@ static int m_can_dev_setup(struct m_can_classdev *cdev)
return -EINVAL;
}
- if (cdev->ops->init)
- cdev->ops->init(cdev);
+ /* Forcing standby mode should be redunant, as the chip should be in
+ * standby after a reset. Write the INIT bit anyways, should the chip
+ * be configured by previous stage.
+ */
+ if (!m_can_cccr_update_bits(cdev, CCCR_INIT, CCCR_INIT))
+ return -EBUSY;
return 0;
}
@@ -1708,7 +1726,8 @@ static void m_can_stop(struct net_device *dev)
m_can_disable_all_interrupts(cdev);
/* Set init mode to disengage from the network */
- m_can_config_endisable(cdev, true);
+ if (!m_can_cccr_update_bits(cdev, CCCR_INIT, CCCR_INIT))
+ netdev_err(dev, "failed to enter standby mode\n");
/* set the state as STOPPED */
cdev->can.state = CAN_STATE_STOPPED;
diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
index a42600dac70d..d723206ac7c9 100644
--- a/drivers/net/can/m_can/tcan4x5x-core.c
+++ b/drivers/net/can/m_can/tcan4x5x-core.c
@@ -453,10 +453,17 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
goto out_power;
}
- ret = tcan4x5x_init(mcan_class);
+ tcan4x5x_check_wake(priv);
+
+ ret = tcan4x5x_write_tcan_reg(mcan_class, TCAN4X5X_INT_EN, 0);
if (ret) {
- dev_err(&spi->dev, "tcan initialization failed %pe\n",
- ERR_PTR(ret));
+ dev_err(&spi->dev, "Disabling interrupts failed %pe\n", ERR_PTR(ret));
+ goto out_power;
+ }
+
+ ret = tcan4x5x_clear_interrupts(mcan_class);
+ if (ret) {
+ dev_err(&spi->dev, "Clearing interrupts failed %pe\n", ERR_PTR(ret));
goto out_power;
}
--
2.44.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] can: m_can: don't enable transceiver when probing
2024-05-01 12:42 [PATCH v2] can: m_can: don't enable transceiver when probing Martin Hundebøll
@ 2024-05-06 10:04 ` Marc Kleine-Budde
2024-05-07 12:29 ` Markus Schneider-Pargmann
2024-05-08 12:44 ` Simon Horman
2 siblings, 0 replies; 5+ messages in thread
From: Marc Kleine-Budde @ 2024-05-06 10:04 UTC (permalink / raw
To: Martin Hundebøll, Markus Schneider-Pargmann
Cc: Chandrasekar Ramakrishnan, Vincent Mailhol, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-can, netdev,
linux-kernel
[-- Attachment #1: Type: text/plain, Size: 9442 bytes --]
Hello Markus,
do you have time to review this patch? I want to send a PR tomorrow.
regards,
Marc
On 01.05.2024 14:42:03, Martin Hundebøll wrote:
> The m_can driver sets and clears the CCCR.INIT bit during probe (both
> when testing the NON-ISO bit, and when configuring the chip). After
> clearing the CCCR.INIT bit, the transceiver enters normal mode, where it
> affects the CAN bus (i.e. it ACKs frames). This can cause troubles when
> the m_can node is only used for monitoring the bus, as one cannot setup
> listen-only mode before the device is probed.
>
> Rework the probe flow, so that the CCCR.INIT bit is only cleared when
> upping the device. First, the tcan4x5x driver is changed to stay in
> standby mode during/after probe. This in turn requires changes when
> setting bits in the CCCR register, as its CSR and CSA bits are always
> high in standby mode.
>
> Signed-off-by: Martin Hundebøll <martin@geanix.com>
> ---
>
> Changes since v1:
> * Implement Markus review comments:
> - Rename m_can_cccr_wait_bits() to m_can_cccr_update_bits()
> - Explicitly set CCCR_INIT bit in m_can_dev_setup()
> - Revert to 5 timeouts/tries to 10
> - Use m_can_config_{en|dis}able() in m_can_niso_supported()
> - Revert move of call to m_can_enable_all_interrupts()
> - Return -EBUSY on failure to enter normal mode
> - Use tcan4x5x_clear_interrupts() in tcan4x5x_can_probe()
>
> drivers/net/can/m_can/m_can.c | 131 +++++++++++++++-----------
> drivers/net/can/m_can/tcan4x5x-core.c | 13 ++-
> 2 files changed, 85 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 14b231c4d7ec..7974aaa5d8cc 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -379,38 +379,60 @@ m_can_txe_fifo_read(struct m_can_classdev *cdev, u32 fgi, u32 offset, u32 *val)
> return cdev->ops->read_fifo(cdev, addr_offset, val, 1);
> }
>
> -static void m_can_config_endisable(struct m_can_classdev *cdev, bool enable)
> +static bool m_can_cccr_update_bits(struct m_can_classdev *cdev, u32 mask, u32 val)
> {
> - u32 cccr = m_can_read(cdev, M_CAN_CCCR);
> - u32 timeout = 10;
> - u32 val = 0;
> -
> - /* Clear the Clock stop request if it was set */
> - if (cccr & CCCR_CSR)
> - cccr &= ~CCCR_CSR;
> -
> - if (enable) {
> - /* enable m_can configuration */
> - m_can_write(cdev, M_CAN_CCCR, cccr | CCCR_INIT);
> - udelay(5);
> - /* CCCR.CCE can only be set/reset while CCCR.INIT = '1' */
> - m_can_write(cdev, M_CAN_CCCR, cccr | CCCR_INIT | CCCR_CCE);
> - } else {
> - m_can_write(cdev, M_CAN_CCCR, cccr & ~(CCCR_INIT | CCCR_CCE));
> - }
> + u32 val_before = m_can_read(cdev, M_CAN_CCCR);
> + u32 val_after = (val_before & ~mask) | val;
> + size_t tries = 10;
> +
> + if (!(mask & CCCR_INIT) && !(val_before & CCCR_INIT))
> + dev_warn(cdev->dev,
> + "trying to configure device when in normal mode. Expect failures\n");
> +
> + /* The chip should be in standby mode when changing the CCCR register,
> + * and some chips set the CSR and CSA bits when in standby. Furthermore,
> + * the CSR and CSA bits should be written as zeros, even when they read
> + * ones.
> + */
> + val_after &= ~(CCCR_CSR | CCCR_CSA);
> +
> + while (tries--) {
> + u32 val_read;
> +
> + /* Write the desired value in each try, as setting some bits in
> + * the CCCR register require other bits to be set first. E.g.
> + * setting the NISO bit requires setting the CCE bit first.
> + */
> + m_can_write(cdev, M_CAN_CCCR, val_after);
> +
> + val_read = m_can_read(cdev, M_CAN_CCCR) & ~(CCCR_CSR | CCCR_CSA);
>
> - /* there's a delay for module initialization */
> - if (enable)
> - val = CCCR_INIT | CCCR_CCE;
> -
> - while ((m_can_read(cdev, M_CAN_CCCR) & (CCCR_INIT | CCCR_CCE)) != val) {
> - if (timeout == 0) {
> - netdev_warn(cdev->net, "Failed to init module\n");
> - return;
> - }
> - timeout--;
> - udelay(1);
> + if (val_read == val_after)
> + return true;
> +
> + usleep_range(1, 5);
> }
> +
> + return false;
> +}
> +
> +static void m_can_config_enable(struct m_can_classdev *cdev)
> +{
> + /* CCCR_INIT must be set in order to set CCCR_CCE, but access to
> + * configuration registers should only be enabled when in standby mode,
> + * where CCCR_INIT is always set.
> + */
> + if (!m_can_cccr_update_bits(cdev, CCCR_CCE, CCCR_CCE))
> + netdev_err(cdev->net, "failed to enable configuration mode\n");
> +}
> +
> +static void m_can_config_disable(struct m_can_classdev *cdev)
> +{
> + /* Only clear CCCR_CCE, since CCCR_INIT cannot be cleared while in
> + * standby mode
> + */
> + if (!m_can_cccr_update_bits(cdev, CCCR_CCE, 0))
> + netdev_err(cdev->net, "failed to disable configuration registers\n");
> }
>
> static void m_can_interrupt_enable(struct m_can_classdev *cdev, u32 interrupts)
> @@ -1403,7 +1425,7 @@ static int m_can_chip_config(struct net_device *dev)
> interrupts &= ~(IR_ARA | IR_ELO | IR_DRX | IR_TEFF | IR_TFE | IR_TCF |
> IR_HPM | IR_RF1F | IR_RF1W | IR_RF1N | IR_RF0F);
>
> - m_can_config_endisable(cdev, true);
> + m_can_config_enable(cdev);
>
> /* RX Buffer/FIFO Element Size 64 bytes data field */
> m_can_write(cdev, M_CAN_RXESC,
> @@ -1521,7 +1543,7 @@ static int m_can_chip_config(struct net_device *dev)
> FIELD_PREP(TSCC_TCP_MASK, 0xf) |
> FIELD_PREP(TSCC_TSS_MASK, TSCC_TSS_INTERNAL));
>
> - m_can_config_endisable(cdev, false);
> + m_can_config_disable(cdev);
>
> if (cdev->ops->init)
> cdev->ops->init(cdev);
> @@ -1550,6 +1572,11 @@ static int m_can_start(struct net_device *dev)
> cdev->tx_fifo_putidx = FIELD_GET(TXFQS_TFQPI_MASK,
> m_can_read(cdev, M_CAN_TXFQS));
>
> + if (!m_can_cccr_update_bits(cdev, CCCR_INIT, 0)) {
> + netdev_err(dev, "failed to enter normal mode\n");
> + return -EBUSY;
> + }
> +
> return 0;
> }
>
> @@ -1603,33 +1630,20 @@ static int m_can_check_core_release(struct m_can_classdev *cdev)
> */
> static bool m_can_niso_supported(struct m_can_classdev *cdev)
> {
> - u32 cccr_reg, cccr_poll = 0;
> - int niso_timeout = -ETIMEDOUT;
> - int i;
> + bool niso_supported;
>
> - m_can_config_endisable(cdev, true);
> - cccr_reg = m_can_read(cdev, M_CAN_CCCR);
> - cccr_reg |= CCCR_NISO;
> - m_can_write(cdev, M_CAN_CCCR, cccr_reg);
> + m_can_config_enable(cdev);
>
> - for (i = 0; i <= 10; i++) {
> - cccr_poll = m_can_read(cdev, M_CAN_CCCR);
> - if (cccr_poll == cccr_reg) {
> - niso_timeout = 0;
> - break;
> - }
> + /* First try to set the NISO bit. */
> + niso_supported = m_can_cccr_update_bits(cdev, CCCR_NISO, CCCR_NISO);
>
> - usleep_range(1, 5);
> - }
> + /* Then clear the it again. */
> + if (!m_can_cccr_update_bits(cdev, CCCR_NISO, 0))
> + dev_err(cdev->dev, "failed to revert the NON-ISO bit in CCCR\n");
>
> - /* Clear NISO */
> - cccr_reg &= ~(CCCR_NISO);
> - m_can_write(cdev, M_CAN_CCCR, cccr_reg);
> + m_can_config_disable(cdev);
>
> - m_can_config_endisable(cdev, false);
> -
> - /* return false if time out (-ETIMEDOUT), else return true */
> - return !niso_timeout;
> + return niso_supported;
> }
>
> static int m_can_dev_setup(struct m_can_classdev *cdev)
> @@ -1694,8 +1708,12 @@ static int m_can_dev_setup(struct m_can_classdev *cdev)
> return -EINVAL;
> }
>
> - if (cdev->ops->init)
> - cdev->ops->init(cdev);
> + /* Forcing standby mode should be redunant, as the chip should be in
> + * standby after a reset. Write the INIT bit anyways, should the chip
> + * be configured by previous stage.
> + */
> + if (!m_can_cccr_update_bits(cdev, CCCR_INIT, CCCR_INIT))
> + return -EBUSY;
>
> return 0;
> }
> @@ -1708,7 +1726,8 @@ static void m_can_stop(struct net_device *dev)
> m_can_disable_all_interrupts(cdev);
>
> /* Set init mode to disengage from the network */
> - m_can_config_endisable(cdev, true);
> + if (!m_can_cccr_update_bits(cdev, CCCR_INIT, CCCR_INIT))
> + netdev_err(dev, "failed to enter standby mode\n");
>
> /* set the state as STOPPED */
> cdev->can.state = CAN_STATE_STOPPED;
> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> index a42600dac70d..d723206ac7c9 100644
> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> @@ -453,10 +453,17 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
> goto out_power;
> }
>
> - ret = tcan4x5x_init(mcan_class);
> + tcan4x5x_check_wake(priv);
> +
> + ret = tcan4x5x_write_tcan_reg(mcan_class, TCAN4X5X_INT_EN, 0);
> if (ret) {
> - dev_err(&spi->dev, "tcan initialization failed %pe\n",
> - ERR_PTR(ret));
> + dev_err(&spi->dev, "Disabling interrupts failed %pe\n", ERR_PTR(ret));
> + goto out_power;
> + }
> +
> + ret = tcan4x5x_clear_interrupts(mcan_class);
> + if (ret) {
> + dev_err(&spi->dev, "Clearing interrupts failed %pe\n", ERR_PTR(ret));
> goto out_power;
> }
>
> --
> 2.44.0
>
>
>
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] can: m_can: don't enable transceiver when probing
2024-05-01 12:42 [PATCH v2] can: m_can: don't enable transceiver when probing Martin Hundebøll
2024-05-06 10:04 ` Marc Kleine-Budde
@ 2024-05-07 12:29 ` Markus Schneider-Pargmann
2024-05-28 13:43 ` Marc Kleine-Budde
2024-05-08 12:44 ` Simon Horman
2 siblings, 1 reply; 5+ messages in thread
From: Markus Schneider-Pargmann @ 2024-05-07 12:29 UTC (permalink / raw
To: Martin Hundebøll, Marc Kleine-Budde
Cc: Chandrasekar Ramakrishnan, Marc Kleine-Budde, Vincent Mailhol,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-can, netdev, linux-kernel
Hi Martin,
On Wed, May 01, 2024 at 02:42:03PM +0200, Martin Hundebøll wrote:
> The m_can driver sets and clears the CCCR.INIT bit during probe (both
> when testing the NON-ISO bit, and when configuring the chip). After
> clearing the CCCR.INIT bit, the transceiver enters normal mode, where it
> affects the CAN bus (i.e. it ACKs frames). This can cause troubles when
> the m_can node is only used for monitoring the bus, as one cannot setup
> listen-only mode before the device is probed.
>
> Rework the probe flow, so that the CCCR.INIT bit is only cleared when
> upping the device. First, the tcan4x5x driver is changed to stay in
> standby mode during/after probe. This in turn requires changes when
> setting bits in the CCCR register, as its CSR and CSA bits are always
> high in standby mode.
>
> Signed-off-by: Martin Hundebøll <martin@geanix.com>
> ---
>
> Changes since v1:
> * Implement Markus review comments:
> - Rename m_can_cccr_wait_bits() to m_can_cccr_update_bits()
> - Explicitly set CCCR_INIT bit in m_can_dev_setup()
> - Revert to 5 timeouts/tries to 10
> - Use m_can_config_{en|dis}able() in m_can_niso_supported()
> - Revert move of call to m_can_enable_all_interrupts()
> - Return -EBUSY on failure to enter normal mode
> - Use tcan4x5x_clear_interrupts() in tcan4x5x_can_probe()
Thanks for addressing these.
In general this looks good:
Reviewed-by: Markus Schneider-Pargmann <msp@baylibre.com>
A few small things commented below, mostly nit-picks.
@Marc: Up to you if you want to merge it or not. I hope the review was
early enough for your PR :)
I don't have time to test it this week, but I can do that next week.
>
> drivers/net/can/m_can/m_can.c | 131 +++++++++++++++-----------
> drivers/net/can/m_can/tcan4x5x-core.c | 13 ++-
> 2 files changed, 85 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 14b231c4d7ec..7974aaa5d8cc 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -379,38 +379,60 @@ m_can_txe_fifo_read(struct m_can_classdev *cdev, u32 fgi, u32 offset, u32 *val)
> return cdev->ops->read_fifo(cdev, addr_offset, val, 1);
> }
>
> -static void m_can_config_endisable(struct m_can_classdev *cdev, bool enable)
> +static bool m_can_cccr_update_bits(struct m_can_classdev *cdev, u32 mask, u32 val)
I personally prefer functions that return error values, in this case
-ETIMEDOUT, as it more clearly indicates what error occured.
> {
> - u32 cccr = m_can_read(cdev, M_CAN_CCCR);
> - u32 timeout = 10;
> - u32 val = 0;
> -
> - /* Clear the Clock stop request if it was set */
> - if (cccr & CCCR_CSR)
> - cccr &= ~CCCR_CSR;
> -
> - if (enable) {
> - /* enable m_can configuration */
> - m_can_write(cdev, M_CAN_CCCR, cccr | CCCR_INIT);
> - udelay(5);
> - /* CCCR.CCE can only be set/reset while CCCR.INIT = '1' */
> - m_can_write(cdev, M_CAN_CCCR, cccr | CCCR_INIT | CCCR_CCE);
> - } else {
> - m_can_write(cdev, M_CAN_CCCR, cccr & ~(CCCR_INIT | CCCR_CCE));
> - }
> + u32 val_before = m_can_read(cdev, M_CAN_CCCR);
> + u32 val_after = (val_before & ~mask) | val;
> + size_t tries = 10;
> +
> + if (!(mask & CCCR_INIT) && !(val_before & CCCR_INIT))
> + dev_warn(cdev->dev,
> + "trying to configure device when in normal mode. Expect failures\n");
> +
> + /* The chip should be in standby mode when changing the CCCR register,
> + * and some chips set the CSR and CSA bits when in standby. Furthermore,
> + * the CSR and CSA bits should be written as zeros, even when they read
> + * ones.
> + */
> + val_after &= ~(CCCR_CSR | CCCR_CSA);
By the way is this a fix that should be fixed for earlier driver/kernel
versions as well? Or is it just required as part of this series?
> +
> + while (tries--) {
> + u32 val_read;
> +
> + /* Write the desired value in each try, as setting some bits in
> + * the CCCR register require other bits to be set first. E.g.
> + * setting the NISO bit requires setting the CCE bit first.
> + */
> + m_can_write(cdev, M_CAN_CCCR, val_after);
> +
> + val_read = m_can_read(cdev, M_CAN_CCCR) & ~(CCCR_CSR | CCCR_CSA);
>
> - /* there's a delay for module initialization */
> - if (enable)
> - val = CCCR_INIT | CCCR_CCE;
> -
> - while ((m_can_read(cdev, M_CAN_CCCR) & (CCCR_INIT | CCCR_CCE)) != val) {
> - if (timeout == 0) {
> - netdev_warn(cdev->net, "Failed to init module\n");
> - return;
> - }
> - timeout--;
> - udelay(1);
> + if (val_read == val_after)
> + return true;
> +
> + usleep_range(1, 5);
> }
> +
> + return false;
> +}
> +
> +static void m_can_config_enable(struct m_can_classdev *cdev)
> +{
> + /* CCCR_INIT must be set in order to set CCCR_CCE, but access to
> + * configuration registers should only be enabled when in standby mode,
> + * where CCCR_INIT is always set.
> + */
> + if (!m_can_cccr_update_bits(cdev, CCCR_CCE, CCCR_CCE))
Another personal preference is the use of this style of error checking
for functions that actually do things:
err = m_can_cccr_update_bits();
if (err)
> + netdev_err(cdev->net, "failed to enable configuration mode\n");
If we detect an error here, should it be propagated and fail probing? I
know it wasn't checked before, so not really necessary to do it now.
Best
Markus
> +}
> +
> +static void m_can_config_disable(struct m_can_classdev *cdev)
> +{
> + /* Only clear CCCR_CCE, since CCCR_INIT cannot be cleared while in
> + * standby mode
> + */
> + if (!m_can_cccr_update_bits(cdev, CCCR_CCE, 0))
> + netdev_err(cdev->net, "failed to disable configuration registers\n");
> }
>
> static void m_can_interrupt_enable(struct m_can_classdev *cdev, u32 interrupts)
> @@ -1403,7 +1425,7 @@ static int m_can_chip_config(struct net_device *dev)
> interrupts &= ~(IR_ARA | IR_ELO | IR_DRX | IR_TEFF | IR_TFE | IR_TCF |
> IR_HPM | IR_RF1F | IR_RF1W | IR_RF1N | IR_RF0F);
>
> - m_can_config_endisable(cdev, true);
> + m_can_config_enable(cdev);
>
> /* RX Buffer/FIFO Element Size 64 bytes data field */
> m_can_write(cdev, M_CAN_RXESC,
> @@ -1521,7 +1543,7 @@ static int m_can_chip_config(struct net_device *dev)
> FIELD_PREP(TSCC_TCP_MASK, 0xf) |
> FIELD_PREP(TSCC_TSS_MASK, TSCC_TSS_INTERNAL));
>
> - m_can_config_endisable(cdev, false);
> + m_can_config_disable(cdev);
>
> if (cdev->ops->init)
> cdev->ops->init(cdev);
> @@ -1550,6 +1572,11 @@ static int m_can_start(struct net_device *dev)
> cdev->tx_fifo_putidx = FIELD_GET(TXFQS_TFQPI_MASK,
> m_can_read(cdev, M_CAN_TXFQS));
>
> + if (!m_can_cccr_update_bits(cdev, CCCR_INIT, 0)) {
> + netdev_err(dev, "failed to enter normal mode\n");
> + return -EBUSY;
> + }
> +
> return 0;
> }
>
> @@ -1603,33 +1630,20 @@ static int m_can_check_core_release(struct m_can_classdev *cdev)
> */
> static bool m_can_niso_supported(struct m_can_classdev *cdev)
> {
> - u32 cccr_reg, cccr_poll = 0;
> - int niso_timeout = -ETIMEDOUT;
> - int i;
> + bool niso_supported;
>
> - m_can_config_endisable(cdev, true);
> - cccr_reg = m_can_read(cdev, M_CAN_CCCR);
> - cccr_reg |= CCCR_NISO;
> - m_can_write(cdev, M_CAN_CCCR, cccr_reg);
> + m_can_config_enable(cdev);
>
> - for (i = 0; i <= 10; i++) {
> - cccr_poll = m_can_read(cdev, M_CAN_CCCR);
> - if (cccr_poll == cccr_reg) {
> - niso_timeout = 0;
> - break;
> - }
> + /* First try to set the NISO bit. */
> + niso_supported = m_can_cccr_update_bits(cdev, CCCR_NISO, CCCR_NISO);
>
> - usleep_range(1, 5);
> - }
> + /* Then clear the it again. */
> + if (!m_can_cccr_update_bits(cdev, CCCR_NISO, 0))
> + dev_err(cdev->dev, "failed to revert the NON-ISO bit in CCCR\n");
>
> - /* Clear NISO */
> - cccr_reg &= ~(CCCR_NISO);
> - m_can_write(cdev, M_CAN_CCCR, cccr_reg);
> + m_can_config_disable(cdev);
>
> - m_can_config_endisable(cdev, false);
> -
> - /* return false if time out (-ETIMEDOUT), else return true */
> - return !niso_timeout;
> + return niso_supported;
> }
>
> static int m_can_dev_setup(struct m_can_classdev *cdev)
> @@ -1694,8 +1708,12 @@ static int m_can_dev_setup(struct m_can_classdev *cdev)
> return -EINVAL;
> }
>
> - if (cdev->ops->init)
> - cdev->ops->init(cdev);
> + /* Forcing standby mode should be redunant, as the chip should be in
> + * standby after a reset. Write the INIT bit anyways, should the chip
> + * be configured by previous stage.
> + */
> + if (!m_can_cccr_update_bits(cdev, CCCR_INIT, CCCR_INIT))
> + return -EBUSY;
>
> return 0;
> }
> @@ -1708,7 +1726,8 @@ static void m_can_stop(struct net_device *dev)
> m_can_disable_all_interrupts(cdev);
>
> /* Set init mode to disengage from the network */
> - m_can_config_endisable(cdev, true);
> + if (!m_can_cccr_update_bits(cdev, CCCR_INIT, CCCR_INIT))
> + netdev_err(dev, "failed to enter standby mode\n");
>
> /* set the state as STOPPED */
> cdev->can.state = CAN_STATE_STOPPED;
> diff --git a/drivers/net/can/m_can/tcan4x5x-core.c b/drivers/net/can/m_can/tcan4x5x-core.c
> index a42600dac70d..d723206ac7c9 100644
> --- a/drivers/net/can/m_can/tcan4x5x-core.c
> +++ b/drivers/net/can/m_can/tcan4x5x-core.c
> @@ -453,10 +453,17 @@ static int tcan4x5x_can_probe(struct spi_device *spi)
> goto out_power;
> }
>
> - ret = tcan4x5x_init(mcan_class);
> + tcan4x5x_check_wake(priv);
> +
> + ret = tcan4x5x_write_tcan_reg(mcan_class, TCAN4X5X_INT_EN, 0);
> if (ret) {
> - dev_err(&spi->dev, "tcan initialization failed %pe\n",
> - ERR_PTR(ret));
> + dev_err(&spi->dev, "Disabling interrupts failed %pe\n", ERR_PTR(ret));
> + goto out_power;
> + }
> +
> + ret = tcan4x5x_clear_interrupts(mcan_class);
> + if (ret) {
> + dev_err(&spi->dev, "Clearing interrupts failed %pe\n", ERR_PTR(ret));
> goto out_power;
> }
>
> --
> 2.44.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] can: m_can: don't enable transceiver when probing
2024-05-01 12:42 [PATCH v2] can: m_can: don't enable transceiver when probing Martin Hundebøll
2024-05-06 10:04 ` Marc Kleine-Budde
2024-05-07 12:29 ` Markus Schneider-Pargmann
@ 2024-05-08 12:44 ` Simon Horman
2 siblings, 0 replies; 5+ messages in thread
From: Simon Horman @ 2024-05-08 12:44 UTC (permalink / raw
To: Martin Hundebøll
Cc: Chandrasekar Ramakrishnan, Marc Kleine-Budde, Vincent Mailhol,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Markus Schneider-Pargmann, linux-can, netdev, linux-kernel
On Wed, May 01, 2024 at 02:42:03PM +0200, Martin Hundebøll wrote:
> The m_can driver sets and clears the CCCR.INIT bit during probe (both
> when testing the NON-ISO bit, and when configuring the chip). After
> clearing the CCCR.INIT bit, the transceiver enters normal mode, where it
> affects the CAN bus (i.e. it ACKs frames). This can cause troubles when
> the m_can node is only used for monitoring the bus, as one cannot setup
> listen-only mode before the device is probed.
>
> Rework the probe flow, so that the CCCR.INIT bit is only cleared when
> upping the device. First, the tcan4x5x driver is changed to stay in
> standby mode during/after probe. This in turn requires changes when
> setting bits in the CCCR register, as its CSR and CSA bits are always
> high in standby mode.
>
> Signed-off-by: Martin Hundebøll <martin@geanix.com>
> ---
>
> Changes since v1:
> * Implement Markus review comments:
> - Rename m_can_cccr_wait_bits() to m_can_cccr_update_bits()
> - Explicitly set CCCR_INIT bit in m_can_dev_setup()
> - Revert to 5 timeouts/tries to 10
> - Use m_can_config_{en|dis}able() in m_can_niso_supported()
> - Revert move of call to m_can_enable_all_interrupts()
> - Return -EBUSY on failure to enter normal mode
> - Use tcan4x5x_clear_interrupts() in tcan4x5x_can_probe()
>
> drivers/net/can/m_can/m_can.c | 131 +++++++++++++++-----------
> drivers/net/can/m_can/tcan4x5x-core.c | 13 ++-
> 2 files changed, 85 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
...
> @@ -1694,8 +1708,12 @@ static int m_can_dev_setup(struct m_can_classdev *cdev)
> return -EINVAL;
> }
>
> - if (cdev->ops->init)
> - cdev->ops->init(cdev);
> + /* Forcing standby mode should be redunant, as the chip should be in
Hi Martin,
A minor nit, to consider fixing if you send a v3 for some other reason:
redunant -> redundant.
> + * standby after a reset. Write the INIT bit anyways, should the chip
> + * be configured by previous stage.
> + */
> + if (!m_can_cccr_update_bits(cdev, CCCR_INIT, CCCR_INIT))
> + return -EBUSY;
>
> return 0;
> }
...
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] can: m_can: don't enable transceiver when probing
2024-05-07 12:29 ` Markus Schneider-Pargmann
@ 2024-05-28 13:43 ` Marc Kleine-Budde
0 siblings, 0 replies; 5+ messages in thread
From: Marc Kleine-Budde @ 2024-05-28 13:43 UTC (permalink / raw
To: Markus Schneider-Pargmann
Cc: Martin Hundebøll, Chandrasekar Ramakrishnan, Vincent Mailhol,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
linux-can, netdev, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2201 bytes --]
On 07.05.2024 14:29:57, Markus Schneider-Pargmann wrote:
> Hi Martin,
>
> On Wed, May 01, 2024 at 02:42:03PM +0200, Martin Hundebøll wrote:
> > The m_can driver sets and clears the CCCR.INIT bit during probe (both
> > when testing the NON-ISO bit, and when configuring the chip). After
> > clearing the CCCR.INIT bit, the transceiver enters normal mode, where it
> > affects the CAN bus (i.e. it ACKs frames). This can cause troubles when
> > the m_can node is only used for monitoring the bus, as one cannot setup
> > listen-only mode before the device is probed.
> >
> > Rework the probe flow, so that the CCCR.INIT bit is only cleared when
> > upping the device. First, the tcan4x5x driver is changed to stay in
> > standby mode during/after probe. This in turn requires changes when
> > setting bits in the CCCR register, as its CSR and CSA bits are always
> > high in standby mode.
> >
> > Signed-off-by: Martin Hundebøll <martin@geanix.com>
> > ---
> >
> > Changes since v1:
> > * Implement Markus review comments:
> > - Rename m_can_cccr_wait_bits() to m_can_cccr_update_bits()
> > - Explicitly set CCCR_INIT bit in m_can_dev_setup()
> > - Revert to 5 timeouts/tries to 10
> > - Use m_can_config_{en|dis}able() in m_can_niso_supported()
> > - Revert move of call to m_can_enable_all_interrupts()
> > - Return -EBUSY on failure to enter normal mode
> > - Use tcan4x5x_clear_interrupts() in tcan4x5x_can_probe()
>
> Thanks for addressing these.
>
> In general this looks good:
> Reviewed-by: Markus Schneider-Pargmann <msp@baylibre.com>
>
> A few small things commented below, mostly nit-picks.
> @Marc: Up to you if you want to merge it or not. I hope the review was
> early enough for your PR :)
> I don't have time to test it this week, but I can do that next week.
Martin, please address the review feedback by Markus and send a v3.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-05-28 13:43 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-01 12:42 [PATCH v2] can: m_can: don't enable transceiver when probing Martin Hundebøll
2024-05-06 10:04 ` Marc Kleine-Budde
2024-05-07 12:29 ` Markus Schneider-Pargmann
2024-05-28 13:43 ` Marc Kleine-Budde
2024-05-08 12:44 ` Simon Horman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).