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@xxxxxxx> wrote:
> From: Fedor Ross <fedor.ross@xxxxxxx>
>
> 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@xxxxxxx>
> Signed-off-by: Marek Vasut <marex@xxxxxxx>
> ---
> Cc: "David S. Miller" <davem@xxxxxxxxxxxxx>
> Cc: Eric Dumazet <edumazet@xxxxxxxxxx>
> Cc: Jakub Kicinski <kuba@xxxxxxxxxx>
> Cc: Liam Girdwood <lgirdwood@xxxxxxxxx>
> Cc: Manivannan Sadhasivam <mani@xxxxxxxxxx>
> Cc: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx>
> Cc: Marek Vasut <marex@xxxxxxx>
> Cc: Mark Brown <broonie@xxxxxxxxxx>
> Cc: Paolo Abeni <pabeni@xxxxxxxxxx>
> Cc: Thomas Kopp <thomas.kopp@xxxxxxxxxxxxx>
> Cc: Wolfgang Grandegger <wg@xxxxxxxxxxxxxx>
> Cc: linux-can@xxxxxxxxxxxxxxx
> ---
> 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@xxxxxxxxxx/
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
>
