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

[Marek Vasut <marex@denx.de>]

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.