[PATCH v2 1/2] can: mcp251xfd: Increase poll timeout

[PATCH v2 1/2] can: mcp251xfd: Increase poll timeout

[Marek Vasut]

From: Fedor Ross <fedor.ross@ifm.com>

Make `MCP251XFD_POLL_TIMEOUT_US` timeout calculation dynamic. Use
maximum of 1ms (arbitrarily chosen during driver development) and
bit time of one full CANFD frame including bit stuffing and bus idle
condition sample cycles, at the current bitrate. This seems to be
necessary when configuring low bit rates like 10 Kbit/s for example.
Otherwise during polling for the CAN controller to enter
'Normal CAN 2.0 mode' the timeout limit is exceeded and the
configuration fails with:

$ ip link set dev can1 up type can bitrate 10000
[  731.911072] mcp251xfd spi2.1 can1: Controller failed to enter mode CAN 2.0 Mode (6) and stays in Configuration Mode (4) (con=0x068b0760, osc=0x00000468).
[  731.927192] mcp251xfd spi2.1 can1: CRC read error at address 0x0e0c (length=4, data=00 00 00 00, CRC=0x0000) retrying.
[  731.938101] A link change request failed with some changes committed already. Interface can1 may have been left with an inconsistent configuration, please check.
RTNETLINK answers: Connection timed out

Fixes: 55e5b97f003e ("can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN")
Signed-off-by: Fedor Ross <fedor.ross@ifm.com>
Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Manivannan Sadhasivam <mani@kernel.org>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: Marek Vasut <marex@denx.de>
Cc: Mark Brown <broonie@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Thomas Kopp <thomas.kopp@microchip.com>
Cc: Wolfgang Grandegger <wg@grandegger.com>
Cc: linux-can@vger.kernel.org
---
V2: - Add macros for CAN_BIT_STUFFING_OVERHEAD and CAN_IDLE_CONDITION_SAMPLES
      (thanks Thomas, but please double check the comments)
    - Update commit message
---
 drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 8 +++++++-
 drivers/net/can/spi/mcp251xfd/mcp251xfd.h      | 9 +++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
index 68df6d4641b5c..207bcd5bf795b 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
@@ -227,6 +227,7 @@ static int
 __mcp251xfd_chip_set_mode(const struct mcp251xfd_priv *priv,
 			  const u8 mode_req, bool nowait)
 {
+	const struct can_bittiming *bt = &priv->can.bittiming;
 	u32 con = 0, con_reqop, osc = 0;
 	u8 mode;
 	int err;
@@ -251,7 +252,12 @@ __mcp251xfd_chip_set_mode(const struct mcp251xfd_priv *priv,
 				       FIELD_GET(MCP251XFD_REG_CON_OPMOD_MASK,
 						 con) == mode_req,
 				       MCP251XFD_POLL_SLEEP_US,
-				       MCP251XFD_POLL_TIMEOUT_US);
+				       max(MCP251XFD_POLL_TIMEOUT_US,
+					   (unsigned int)(CANFD_FRAME_LEN_MAX *
+					    BITS_PER_BYTE *
+					    CAN_BIT_STUFFING_OVERHEAD +
+					    CAN_IDLE_CONDITION_SAMPLES) *
+					   USEC_PER_SEC / bt->bitrate));
 	if (err != -ETIMEDOUT && err != -EBADMSG)
 		return err;
 
diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
index 7024ff0cc2c0c..412d58d84fb63 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
@@ -432,6 +432,15 @@ static_assert(MCP251XFD_FIFO_RX_NUM <= 4U);
 /* Use Half Duplex SPI transfers */
 #define MCP251XFD_QUIRK_HALF_DUPLEX BIT(5)
 
+/* CAN bit stuffing overhead multiplication factor */
+#define CAN_BIT_STUFFING_OVERHEAD	1.2
+
+/* Number of samples after which an idle condition is present on the bus
+ * as specified in the ISO. This is 11 consecutive sampled recessive bits
+ * after a full frame (if one is currently in transmission).
+ */
+#define CAN_IDLE_CONDITION_SAMPLES	11
+
 struct mcp251xfd_hw_tef_obj {
 	u32 id;
 	u32 flags;
-- 
2.39.2

[PATCH v2 2/2] can: mcp251xfd: Move generic macros into length.h

[Marek Vasut]

Move CAN_BIT_STUFFING_OVERHEAD and CAN_IDLE_CONDITION_SAMPLES generic
CAN macros into linux/can/length.h . No functional change.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Manivannan Sadhasivam <mani@kernel.org>
Cc: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: Marek Vasut <marex@denx.de>
Cc: Mark Brown <broonie@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: Thomas Kopp <thomas.kopp@microchip.com>
Cc: Wolfgang Grandegger <wg@grandegger.com>
Cc: linux-can@vger.kernel.org
---
V2: - New separate patch, separate from the mcp251xfd addition to ease backporting
---
 drivers/net/can/spi/mcp251xfd/mcp251xfd.h | 9 ---------
 include/linux/can/length.h                | 9 +++++++++
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
index 412d58d84fb63..7024ff0cc2c0c 100644
--- a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
+++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
@@ -432,15 +432,6 @@ static_assert(MCP251XFD_FIFO_RX_NUM <= 4U);
 /* Use Half Duplex SPI transfers */
 #define MCP251XFD_QUIRK_HALF_DUPLEX BIT(5)
 
-/* CAN bit stuffing overhead multiplication factor */
-#define CAN_BIT_STUFFING_OVERHEAD	1.2
-
-/* Number of samples after which an idle condition is present on the bus
- * as specified in the ISO. This is 11 consecutive sampled recessive bits
- * after a full frame (if one is currently in transmission).
- */
-#define CAN_IDLE_CONDITION_SAMPLES	11
-
 struct mcp251xfd_hw_tef_obj {
 	u32 id;
 	u32 flags;
diff --git a/include/linux/can/length.h b/include/linux/can/length.h
index 6995092b774ec..0e20e153955d4 100644
--- a/include/linux/can/length.h
+++ b/include/linux/can/length.h
@@ -122,6 +122,15 @@
  */
 #define CANFD_FRAME_LEN_MAX (CANFD_FRAME_OVERHEAD_EFF + CANFD_MAX_DLEN)
 
+/* CAN bit stuffing overhead multiplication factor */
+#define CAN_BIT_STUFFING_OVERHEAD	1.2
+
+/* Number of samples after which an idle condition is present on the bus
+ * as specified in the ISO. This is 11 consecutive sampled recessive bits
+ * after a full frame (if one is currently in transmission).
+ */
+#define CAN_IDLE_CONDITION_SAMPLES	11
+
 /*
  * can_cc_dlc2len(value) - convert a given data length code (dlc) of a
  * Classical CAN frame into a valid data length of max. 8 bytes.
-- 
2.39.2

Re: [PATCH v2 1/2] can: mcp251xfd: Increase poll timeout

[Vincent Mailhol]

Hi Marek,

The patches should have been in reverse order:

  1st: can: mcp251xfd: Move generic macros into length.h
  2nd: can: mcp251xfd: Increase poll timeout

so that you do not have to remove the lines just added in the previous patch.

On Tue. 6 May 2023 at 07:36, Marek Vasut <marex@denx.de> wrote:
> From: Fedor Ross <fedor.ross@ifm.com>
>
> Make `MCP251XFD_POLL_TIMEOUT_US` timeout calculation dynamic. Use
> maximum of 1ms (arbitrarily chosen during driver development) and
> bit time of one full CANFD frame including bit stuffing and bus idle
> condition sample cycles, at the current bitrate. This seems to be
> necessary when configuring low bit rates like 10 Kbit/s for example.
> Otherwise during polling for the CAN controller to enter
> 'Normal CAN 2.0 mode' the timeout limit is exceeded and the
> configuration fails with:
>
> $ ip link set dev can1 up type can bitrate 10000
> [  731.911072] mcp251xfd spi2.1 can1: Controller failed to enter mode CAN 2.0 Mode (6) and stays in Configuration Mode (4) (con=0x068b0760, osc=0x00000468).
> [  731.927192] mcp251xfd spi2.1 can1: CRC read error at address 0x0e0c (length=4, data=00 00 00 00, CRC=0x0000) retrying.
> [  731.938101] A link change request failed with some changes committed already. Interface can1 may have been left with an inconsistent configuration, please check.
> RTNETLINK answers: Connection timed out
>
> Fixes: 55e5b97f003e ("can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN")
> Signed-off-by: Fedor Ross <fedor.ross@ifm.com>
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Manivannan Sadhasivam <mani@kernel.org>
> Cc: Marc Kleine-Budde <mkl@pengutronix.de>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Thomas Kopp <thomas.kopp@microchip.com>
> Cc: Wolfgang Grandegger <wg@grandegger.com>
> Cc: linux-can@vger.kernel.org
> ---
> V2: - Add macros for CAN_BIT_STUFFING_OVERHEAD and CAN_IDLE_CONDITION_SAMPLES
>       (thanks Thomas, but please double check the comments)
>     - Update commit message
> ---
>  drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 8 +++++++-
>  drivers/net/can/spi/mcp251xfd/mcp251xfd.h      | 9 +++++++++
>  2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> index 68df6d4641b5c..207bcd5bf795b 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> @@ -227,6 +227,7 @@ static int
>  __mcp251xfd_chip_set_mode(const struct mcp251xfd_priv *priv,
>                           const u8 mode_req, bool nowait)
>  {
> +       const struct can_bittiming *bt = &priv->can.bittiming;
>         u32 con = 0, con_reqop, osc = 0;
>         u8 mode;
>         int err;
> @@ -251,7 +252,12 @@ __mcp251xfd_chip_set_mode(const struct mcp251xfd_priv *priv,
>                                        FIELD_GET(MCP251XFD_REG_CON_OPMOD_MASK,
>                                                  con) == mode_req,
>                                        MCP251XFD_POLL_SLEEP_US,
> -                                      MCP251XFD_POLL_TIMEOUT_US);
> +                                      max(MCP251XFD_POLL_TIMEOUT_US,
> +                                          (unsigned int)(CANFD_FRAME_LEN_MAX *
> +                                           BITS_PER_BYTE *
> +                                           CAN_BIT_STUFFING_OVERHEAD +

The goal is to have the exact number of bits, right?

It seems odd to me to use a rounded value and then try to recalculate
the exact length in bits.
I understand that because CANFD_FRAME_OVERHEAD_EFF is a multiple of
BITS_PER_BYTE, CANFD_FRAME_LEN_MAX happened to be the exact value.
Regardless, that is a fluke.

I think that we should have another set of definitions for the frame
lengths in bits. I sent a proposal here:

 https://lore.kernel.org/linux-can/20230507155506.3179711-1-mailhol.vincent@wanadoo.fr/

If you like it, you can rebase this patch on top of mine and use the
newly defined CANFD_FRAME_LEN_MAX_BITS_STUFFING.


> +                                           CAN_IDLE_CONDITION_SAMPLES) *
> +                                          USEC_PER_SEC / bt->bitrate));
>         if (err != -ETIMEDOUT && err != -EBADMSG)
>                 return err;
>
> diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
> index 7024ff0cc2c0c..412d58d84fb63 100644
> --- a/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
> +++ b/drivers/net/can/spi/mcp251xfd/mcp251xfd.h
> @@ -432,6 +432,15 @@ static_assert(MCP251XFD_FIFO_RX_NUM <= 4U);
>  /* Use Half Duplex SPI transfers */
>  #define MCP251XFD_QUIRK_HALF_DUPLEX BIT(5)
>
> +/* CAN bit stuffing overhead multiplication factor */
> +#define CAN_BIT_STUFFING_OVERHEAD      1.2
> +
> +/* Number of samples after which an idle condition is present on the bus
> + * as specified in the ISO. This is 11 consecutive sampled recessive bits
> + * after a full frame (if one is currently in transmission).
> + */
> +#define CAN_IDLE_CONDITION_SAMPLES     11
> +
>  struct mcp251xfd_hw_tef_obj {
>         u32 id;
>         u32 flags;
> --
> 2.39.2
>

Re: [PATCH v2 1/2] can: mcp251xfd: Increase poll timeout

[Marek Vasut]

On 5/7/23 17:58, Vincent Mailhol wrote:
> Hi Marek,

Hi,

> The patches should have been in reverse order:
> 
>    1st: can: mcp251xfd: Move generic macros into length.h
>    2nd: can: mcp251xfd: Increase poll timeout
> 
> so that you do not have to remove the lines just added in the previous patch.

The current order is actually deliberate to make it easy to backport 
this one patch via stable queue, since it Fixes: a bug. The 2/2 is just 
a generalization.

> On Tue. 6 May 2023 at 07:36, Marek Vasut <marex@denx.de> wrote:
>> From: Fedor Ross <fedor.ross@ifm.com>
>>
>> Make `MCP251XFD_POLL_TIMEOUT_US` timeout calculation dynamic. Use
>> maximum of 1ms (arbitrarily chosen during driver development) and
>> bit time of one full CANFD frame including bit stuffing and bus idle
>> condition sample cycles, at the current bitrate. This seems to be
>> necessary when configuring low bit rates like 10 Kbit/s for example.
>> Otherwise during polling for the CAN controller to enter
>> 'Normal CAN 2.0 mode' the timeout limit is exceeded and the
>> configuration fails with:
>>
>> $ ip link set dev can1 up type can bitrate 10000
>> [  731.911072] mcp251xfd spi2.1 can1: Controller failed to enter mode CAN 2.0 Mode (6) and stays in Configuration Mode (4) (con=0x068b0760, osc=0x00000468).
>> [  731.927192] mcp251xfd spi2.1 can1: CRC read error at address 0x0e0c (length=4, data=00 00 00 00, CRC=0x0000) retrying.
>> [  731.938101] A link change request failed with some changes committed already. Interface can1 may have been left with an inconsistent configuration, please check.
>> RTNETLINK answers: Connection timed out
>>
>> Fixes: 55e5b97f003e ("can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN")
>> Signed-off-by: Fedor Ross <fedor.ross@ifm.com>
>> Signed-off-by: Marek Vasut <marex@denx.de>

[...]

>> @@ -251,7 +252,12 @@ __mcp251xfd_chip_set_mode(const struct mcp251xfd_priv *priv,
>>                                         FIELD_GET(MCP251XFD_REG_CON_OPMOD_MASK,
>>                                                   con) == mode_req,
>>                                         MCP251XFD_POLL_SLEEP_US,
>> -                                      MCP251XFD_POLL_TIMEOUT_US);
>> +                                      max(MCP251XFD_POLL_TIMEOUT_US,
>> +                                          (unsigned int)(CANFD_FRAME_LEN_MAX *
>> +                                           BITS_PER_BYTE *
>> +                                           CAN_BIT_STUFFING_OVERHEAD +
> 
> The goal is to have the exact number of bits, right?

Not really, the goal is to calculate a suitable delay, for which the 
kernel has to wait for this SPI CAN controller to switch mode . That 
delay is dependent on the bit timing though.

> It seems odd to me to use a rounded value and then try to recalculate
> the exact length in bits.
> I understand that because CANFD_FRAME_OVERHEAD_EFF is a multiple of
> BITS_PER_BYTE, CANFD_FRAME_LEN_MAX happened to be the exact value.
> Regardless, that is a fluke.
> 
> I think that we should have another set of definitions for the frame
> lengths in bits. I sent a proposal here:
> 
>   https://lore.kernel.org/linux-can/20230507155506.3179711-1-mailhol.vincent@wanadoo.fr/
> 
> If you like it, you can rebase this patch on top of mine and use the
> newly defined CANFD_FRAME_LEN_MAX_BITS_STUFFING.

I think I don't have enough experience to decide one way or the other, 
so I will wait for the reviewers to suggest the course of action.

Re: [PATCH v2 1/2] can: mcp251xfd: Increase poll timeout

[Vincent Mailhol]

Le lun. 8 mai 2023 à 04:20, Marek Vasut <marex@denx.de> a écrit :
>
> On 5/7/23 17:58, Vincent Mailhol wrote:
> > Hi Marek,
>
> Hi,
>
> > The patches should have been in reverse order:
> >
> >    1st: can: mcp251xfd: Move generic macros into length.h
> >    2nd: can: mcp251xfd: Increase poll timeout
> >
> > so that you do not have to remove the lines just added in the previous patch.
>
> The current order is actually deliberate to make it easy to backport
> this one patch via stable queue, since it Fixes: a bug. The 2/2 is just
> a generalization.

I see your point. However, your patch uses CANFD_FRAME_LEN_MAX which
was introduced in Linux 5.12 but mcp251xfd was introduced in 5.10. So
you will need a separate patch for the 5.10 LTS branch.

As for the other stable branches (5.15 and greater), patches toward
linux/can/length.h can easily be backported. To conclude, in that
particular instance, I think that the correct approach is:

 - One separate mcp251xfd patch for 5.10.
 - A pair of patches targeting mcp251xfd and length.c for 5.15+

> > On Tue. 6 May 2023 at 07:36, Marek Vasut <marex@denx.de> wrote:
> >> From: Fedor Ross <fedor.ross@ifm.com>
> >>
> >> Make `MCP251XFD_POLL_TIMEOUT_US` timeout calculation dynamic. Use
> >> maximum of 1ms (arbitrarily chosen during driver development) and
> >> bit time of one full CANFD frame including bit stuffing and bus idle
> >> condition sample cycles, at the current bitrate. This seems to be
> >> necessary when configuring low bit rates like 10 Kbit/s for example.
> >> Otherwise during polling for the CAN controller to enter
> >> 'Normal CAN 2.0 mode' the timeout limit is exceeded and the
> >> configuration fails with:
> >>
> >> $ ip link set dev can1 up type can bitrate 10000
> >> [  731.911072] mcp251xfd spi2.1 can1: Controller failed to enter mode CAN 2.0 Mode (6) and stays in Configuration Mode (4) (con=0x068b0760, osc=0x00000468).
> >> [  731.927192] mcp251xfd spi2.1 can1: CRC read error at address 0x0e0c (length=4, data=00 00 00 00, CRC=0x0000) retrying.
> >> [  731.938101] A link change request failed with some changes committed already. Interface can1 may have been left with an inconsistent configuration, please check.
> >> RTNETLINK answers: Connection timed out
> >>
> >> Fixes: 55e5b97f003e ("can: mcp25xxfd: add driver for Microchip MCP25xxFD SPI CAN")
> >> Signed-off-by: Fedor Ross <fedor.ross@ifm.com>
> >> Signed-off-by: Marek Vasut <marex@denx.de>
>
> [...]
>
> >> @@ -251,7 +252,12 @@ __mcp251xfd_chip_set_mode(const struct mcp251xfd_priv *priv,
> >>                                         FIELD_GET(MCP251XFD_REG_CON_OPMOD_MASK,
> >>                                                   con) == mode_req,
> >>                                         MCP251XFD_POLL_SLEEP_US,
> >> -                                      MCP251XFD_POLL_TIMEOUT_US);
> >> +                                      max(MCP251XFD_POLL_TIMEOUT_US,
> >> +                                          (unsigned int)(CANFD_FRAME_LEN_MAX *
> >> +                                           BITS_PER_BYTE *
> >> +                                           CAN_BIT_STUFFING_OVERHEAD +
> >
> > The goal is to have the exact number of bits, right?
>
> Not really, the goal is to calculate a suitable delay, for which the
> kernel has to wait for this SPI CAN controller to switch mode . That
> delay is dependent on the bit timing though.

Ack. In that sense, the above formula is indeed suitable. Regardless,

  CANFD_FRAME_LEN_MAX * BITS_PER_BYTE

expands to:

  (DIV_ROUND_UP(80, 8) + 64) * 8

and that confuses me a lot to divide and then just multiply again by
the same value.

> > It seems odd to me to use a rounded value and then try to recalculate
> > the exact length in bits.
> > I understand that because CANFD_FRAME_OVERHEAD_EFF is a multiple of
> > BITS_PER_BYTE, CANFD_FRAME_LEN_MAX happened to be the exact value.
> > Regardless, that is a fluke.
> >
> > I think that we should have another set of definitions for the frame
> > lengths in bits. I sent a proposal here:
> >
> >   https://lore.kernel.org/linux-can/20230507155506.3179711-1-mailhol.vincent@wanadoo.fr/
> >
> > If you like it, you can rebase this patch on top of mine and use the
> > newly defined CANFD_FRAME_LEN_MAX_BITS_STUFFING.
>
> I think I don't have enough experience to decide one way or the other,
> so I will wait for the reviewers to suggest the course of action.

ACK. I am fine with delegating the final decision to others.