* [PATCH] can: mcp251xfd: Increase poll timeout
@ 2023-05-04 19:50 Marek Vasut
2023-05-05 6:50 ` Marc Kleine-Budde
0 siblings, 1 reply; 14+ messages in thread
From: Marek Vasut @ 2023-05-04 19:50 UTC (permalink / raw
To: linux-can
Cc: Fedor Ross, Marek Vasut, David S. Miller, Eric Dumazet,
Jakub Kicinski, Liam Girdwood, Manivannan Sadhasivam,
Marc Kleine-Budde, Mark Brown, Paolo Abeni, Thomas Kopp,
Wolfgang Grandegger
From: Fedor Ross <fedor.ross@ifm.com>
Make `MCP251XFD_POLL_TIMEOUT_US` timeout calculation dynamic. Use
maximum of 1ms and bit time of one full 72 bytes CANFD frame 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: 55e5b97f003e8 ("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
---
drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 4 +++-
1 file changed, 3 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..9908843798cef 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,8 @@ __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,
+ 576 * USEC_PER_SEC / bt->bitrate));
if (err != -ETIMEDOUT && err != -EBADMSG)
return err;
--
2.39.2
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] can: mcp251xfd: Increase poll timeout
2023-05-04 19:50 [PATCH] can: mcp251xfd: Increase poll timeout Marek Vasut
@ 2023-05-05 6:50 ` Marc Kleine-Budde
2023-05-05 7:07 ` Thomas.Kopp
0 siblings, 1 reply; 14+ messages in thread
From: Marc Kleine-Budde @ 2023-05-05 6:50 UTC (permalink / raw
To: Marek Vasut
Cc: linux-can, Fedor Ross, David S. Miller, Eric Dumazet,
Jakub Kicinski, Liam Girdwood, Manivannan Sadhasivam, Mark Brown,
Paolo Abeni, Thomas Kopp, Wolfgang Grandegger
[-- Attachment #1: Type: text/plain, Size: 3458 bytes --]
On 04.05.2023 21:50:59, Marek Vasut wrote:
> From: Fedor Ross <fedor.ross@ifm.com>
>
> Make `MCP251XFD_POLL_TIMEOUT_US` timeout calculation dynamic. Use
> maximum of 1ms and bit time of one full 72 bytes CANFD frame at the
> current bitrate.
The 1ms was arbitrary chosen by me during development. The previous
driver by Martin Sperl uses a hardcoded loop of 256 retries and no
explicit sleep.
The datasheet "MCP25xxFD Family Reference Manual" says it needs an idle
bus.
> 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: 55e5b97f003e8 ("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
> ---
> drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 4 +++-
> 1 file changed, 3 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..9908843798cef 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,8 @@ __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,
> + 576 * USEC_PER_SEC / bt->bitrate));
Let's use CANFD_FRAME_LEN_MAX * BITS_PER_BYTE instead of 576. I can fix
this while applying.
> if (err != -ETIMEDOUT && err != -EBADMSG)
> return err;
>
> --
> 2.39.2
>
>
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] 14+ messages in thread
* RE: [PATCH] can: mcp251xfd: Increase poll timeout
2023-05-05 6:50 ` Marc Kleine-Budde
@ 2023-05-05 7:07 ` Thomas.Kopp
2023-05-05 7:58 ` Marc Kleine-Budde
0 siblings, 1 reply; 14+ messages in thread
From: Thomas.Kopp @ 2023-05-05 7:07 UTC (permalink / raw
To: mkl, marex
Cc: linux-can, fedor.ross, davem, edumazet, kuba, lgirdwood, mani,
broonie, pabeni, wg
> The datasheet "MCP25xxFD Family Reference Manual" says it needs an idle
> bus.
Technically what's needed is an idle condition on the bus as specified in the ISO. i.e. 11 consecutive sampled recessive bits after a full frame (if one is currently in transmission).
> > 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: 55e5b97f003e8 ("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
> > ---
> > drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c | 4 +++-
> > 1 file changed, 3 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..9908843798cef 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,8 @@ __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,
> > + 576 * USEC_PER_SEC / bt->bitrate));
>
> Let's use CANFD_FRAME_LEN_MAX * BITS_PER_BYTE instead of 576. I can fix
> this while applying.
>
So, I'd suggest CANFD_FRAME_LEN_MAX * BITS_PER_BYTE + 11 + stuffbits, as far as I can tell the CANFD_FRAME_LEN_MAX ignores stuffbits, so as an overapproximation something like (CANFD_FRAME_LEN_MAX * BITS_PER_BYTE) * 1.2 + 11. It's unlikely that it will actually be necessary but it makes it clear where the numbers are coming from.
Best Regards,
Thomas
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: RE: [PATCH] can: mcp251xfd: Increase poll timeout
2023-05-05 7:07 ` Thomas.Kopp
@ 2023-05-05 7:58 ` Marc Kleine-Budde
2023-05-05 8:21 ` Thomas.Kopp
0 siblings, 1 reply; 14+ messages in thread
From: Marc Kleine-Budde @ 2023-05-05 7:58 UTC (permalink / raw
To: Thomas.Kopp
Cc: marex, linux-can, fedor.ross, davem, edumazet, kuba, lgirdwood,
mani, broonie, pabeni, wg
[-- Attachment #1: Type: text/plain, Size: 2244 bytes --]
On 05.05.2023 07:07:03, Thomas.Kopp@microchip.com wrote:
> > The datasheet "MCP25xxFD Family Reference Manual" says it needs an idle
> > bus.
>
> Technically what's needed is an idle condition on the bus as specified
> in the ISO. i.e. 11 consecutive sampled recessive bits after a full
> frame (if one is currently in transmission).
Right. What happens if another CAN frames comes before there are 11
consecutive sampled recessive bits? The IFS for CAN is 3 bits?
> > > diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> > b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> > > index 68df6d4641b5c..9908843798cef 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,8 @@ __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,
> > > + 576 * USEC_PER_SEC / bt->bitrate));
> >
> > Let's use CANFD_FRAME_LEN_MAX * BITS_PER_BYTE instead of 576. I can fix
> > this while applying.
> >
> So, I'd suggest CANFD_FRAME_LEN_MAX * BITS_PER_BYTE + 11 + stuffbits,
> as far as I can tell the CANFD_FRAME_LEN_MAX ignores stuffbits, so as
> an overapproximation something like (CANFD_FRAME_LEN_MAX *
> BITS_PER_BYTE) * 1.2 + 11.
..and a define for the 11 and a comment for the * 1.2
> It's unlikely that it will actually be
> necessary but it makes it clear where the numbers are coming from.
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] 14+ messages in thread
* RE: RE: [PATCH] can: mcp251xfd: Increase poll timeout
2023-05-05 7:58 ` Marc Kleine-Budde
@ 2023-05-05 8:21 ` Thomas.Kopp
2023-05-05 8:39 ` Marc Kleine-Budde
2023-05-10 6:26 ` Thomas.Kopp
0 siblings, 2 replies; 14+ messages in thread
From: Thomas.Kopp @ 2023-05-05 8:21 UTC (permalink / raw
To: mkl
Cc: marex, linux-can, fedor.ross, davem, edumazet, kuba, lgirdwood,
mani, broonie, pabeni, wg
> On 05.05.2023 07:07:03, Thomas.Kopp@microchip.com wrote:
> > > The datasheet "MCP25xxFD Family Reference Manual" says it needs an idle
> > > bus.
> >
> > Technically what's needed is an idle condition on the bus as specified
> > in the ISO. i.e. 11 consecutive sampled recessive bits after a full
> > frame (if one is currently in transmission).
>
> Right. What happens if another CAN frames comes before there are 11
> consecutive sampled recessive bits? The IFS for CAN is 3 bits?
Not quite. Between the Frames is an IFS that's correct but the IFS consists of an Intermission which is 3 bits long + a bus idle condition of 11 bits. Regular frames have to wait for the IFS to elapse BUT there's an exception for error frames and overload frames. EF may be up to 12 bit, OF are 8 dominant + 8 recessive bits, browsing through the spec I think only 2 OFs can happen consecutively. Adding another 32 bits to the formula should cover this.
> > > > diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> > > b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> > > > index 68df6d4641b5c..9908843798cef 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,8 @@ __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,
> > > > + 576 * USEC_PER_SEC / bt->bitrate));
> > >
> > > Let's use CANFD_FRAME_LEN_MAX * BITS_PER_BYTE instead of 576. I can
> fix
> > > this while applying.
> > >
> > So, I'd suggest CANFD_FRAME_LEN_MAX * BITS_PER_BYTE + 11 + stuffbits,
> > as far as I can tell the CANFD_FRAME_LEN_MAX ignores stuffbits, so as
> > an overapproximation something like (CANFD_FRAME_LEN_MAX *
> > BITS_PER_BYTE) * 1.2 + 11.
>
> ..and a define for the 11 and a comment for the * 1.2
>
Right, I think definitions for the 11 and the 1.2 make sense in the include/linux/can/length.h anyway.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: RE: RE: [PATCH] can: mcp251xfd: Increase poll timeout
2023-05-05 8:21 ` Thomas.Kopp
@ 2023-05-05 8:39 ` Marc Kleine-Budde
2023-05-10 6:26 ` Thomas.Kopp
1 sibling, 0 replies; 14+ messages in thread
From: Marc Kleine-Budde @ 2023-05-05 8:39 UTC (permalink / raw
To: Thomas.Kopp
Cc: marex, linux-can, fedor.ross, davem, edumazet, kuba, lgirdwood,
mani, broonie, pabeni, wg
[-- Attachment #1: Type: text/plain, Size: 2998 bytes --]
On 05.05.2023 08:21:36, Thomas.Kopp@microchip.com wrote:
> > On 05.05.2023 07:07:03, Thomas.Kopp@microchip.com wrote:
> > > > The datasheet "MCP25xxFD Family Reference Manual" says it needs an idle
> > > > bus.
> > >
> > > Technically what's needed is an idle condition on the bus as specified
> > > in the ISO. i.e. 11 consecutive sampled recessive bits after a full
> > > frame (if one is currently in transmission).
> >
> > Right. What happens if another CAN frames comes before there are 11
> > consecutive sampled recessive bits? The IFS for CAN is 3 bits?
>
> Not quite. Between the Frames is an IFS that's correct but the IFS
> consists of an Intermission which is 3 bits long + a bus idle
> condition of 11 bits. Regular frames have to wait for the IFS to
> elapse BUT there's an exception for error frames and overload frames.
> EF may be up to 12 bit, OF are 8 dominant + 8 recessive bits, browsing
> through the spec I think only 2 OFs can happen consecutively. Adding
> another 32 bits to the formula should cover this.
>
> > > > > diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> > > > b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> > > > > index 68df6d4641b5c..9908843798cef 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,8 @@ __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,
> > > > > + 576 * USEC_PER_SEC / bt->bitrate));
> > > >
> > > > Let's use CANFD_FRAME_LEN_MAX * BITS_PER_BYTE instead of 576. I can
> > fix
> > > > this while applying.
> > > >
> > > So, I'd suggest CANFD_FRAME_LEN_MAX * BITS_PER_BYTE + 11 + stuffbits,
> > > as far as I can tell the CANFD_FRAME_LEN_MAX ignores stuffbits, so as
> > > an overapproximation something like (CANFD_FRAME_LEN_MAX *
> > > BITS_PER_BYTE) * 1.2 + 11.
> >
> > ..and a define for the 11 and a comment for the * 1.2
> >
> Right, I think definitions for the 11 and the 1.2 make sense in the
> include/linux/can/length.h anyway.
Sounds good, make this a separate patch.
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] 14+ messages in thread
* RE: RE: [PATCH] can: mcp251xfd: Increase poll timeout
2023-05-05 8:21 ` Thomas.Kopp
2023-05-05 8:39 ` Marc Kleine-Budde
@ 2023-05-10 6:26 ` Thomas.Kopp
2023-05-10 7:50 ` Vincent Mailhol
1 sibling, 1 reply; 14+ messages in thread
From: Thomas.Kopp @ 2023-05-10 6:26 UTC (permalink / raw
To: mkl, marex
Cc: linux-can, fedor.ross, davem, edumazet, kuba, lgirdwood, mani,
broonie, pabeni, wg
> > On 05.05.2023 07:07:03, Thomas.Kopp@microchip.com wrote:
> > > > The datasheet "MCP25xxFD Family Reference Manual" says it needs an
> idle
> > > > bus.
> > >
> > > Technically what's needed is an idle condition on the bus as specified
> > > in the ISO. i.e. 11 consecutive sampled recessive bits after a full
> > > frame (if one is currently in transmission).
> >
> > Right. What happens if another CAN frames comes before there are 11
> > consecutive sampled recessive bits? The IFS for CAN is 3 bits?
>
> Not quite. Between the Frames is an IFS that's correct but the IFS consists of
> an Intermission which is 3 bits long + a bus idle condition of 11 bits. Regular
> frames have to wait for the IFS to elapse BUT there's an exception for error
> frames and overload frames. EF may be up to 12 bit, OF are 8 dominant + 8
> recessive bits, browsing through the spec I think only 2 OFs can happen
> consecutively. Adding another 32 bits to the formula should cover this.
Re-reading the spec again I noticed that the part where I wrote that there's an "idle condition" after the intermission is wrong. What follows the intermission is just "bus idle", defined two paragraphs later as "The period of bus idle may be of arbitrary length." The 11 recessive bits can be removed from the formula again. The longest period (under the assumption there aren't multiple/continuous errors on the bus) will be Frame + Error Frame (12 bit) + 2 x Overload Frame.
> > > > > diff --git a/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> > > > b/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c
> > > > > index 68df6d4641b5c..9908843798cef 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,8 @@ __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,
> > > > > + 576 * USEC_PER_SEC / bt-
> >bitrate));
> > > >
> > > > Let's use CANFD_FRAME_LEN_MAX * BITS_PER_BYTE instead of 576. I
> can
> > fix
> > > > this while applying.
> > > >
> > > So, I'd suggest CANFD_FRAME_LEN_MAX * BITS_PER_BYTE + 11 +
> stuffbits,
> > > as far as I can tell the CANFD_FRAME_LEN_MAX ignores stuffbits, so as
> > > an overapproximation something like (CANFD_FRAME_LEN_MAX *
> > > BITS_PER_BYTE) * 1.2 + 11.
> >
> > ..and a define for the 11 and a comment for the * 1.2
> >
> Right, I think definitions for the 11 and the 1.2 make sense in the
> include/linux/can/length.h anyway.
Best Regards,
Thomas
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: RE: [PATCH] can: mcp251xfd: Increase poll timeout
2023-05-10 6:26 ` Thomas.Kopp
@ 2023-05-10 7:50 ` Vincent Mailhol
2023-05-10 9:17 ` Thomas.Kopp
0 siblings, 1 reply; 14+ messages in thread
From: Vincent Mailhol @ 2023-05-10 7:50 UTC (permalink / raw
To: Thomas.Kopp
Cc: mkl, marex, linux-can, fedor.ross, davem, edumazet, kuba,
lgirdwood, mani, broonie, pabeni, wg
On Wed. 10 May 2023 at 15:30, <Thomas.Kopp@microchip.com> wrote:
>
> > > On 05.05.2023 07:07:03, Thomas.Kopp@microchip.com wrote:
> > > > > The datasheet "MCP25xxFD Family Reference Manual" says it needs an
> > idle
> > > > > bus.
> > > >
> > > > Technically what's needed is an idle condition on the bus as specified
> > > > in the ISO. i.e. 11 consecutive sampled recessive bits after a full
> > > > frame (if one is currently in transmission).
> > >
> > > Right. What happens if another CAN frames comes before there are 11
> > > consecutive sampled recessive bits? The IFS for CAN is 3 bits?
> >
> > Not quite. Between the Frames is an IFS that's correct but the IFS consists of
> > an Intermission which is 3 bits long + a bus idle condition of 11 bits. Regular
> > frames have to wait for the IFS to elapse BUT there's an exception for error
> > frames and overload frames. EF may be up to 12 bit, OF are 8 dominant + 8
> > recessive bits, browsing through the spec I think only 2 OFs can happen
> > consecutively. Adding another 32 bits to the formula should cover this.
>
> Re-reading the spec again I noticed that the part where I wrote
> that there's an "idle condition" after the intermission is
> wrong. What follows the intermission is just "bus idle",
> defined two paragraphs later as "The period of bus idle may be
> of arbitrary length." The 11 recessive bits can be removed from
> the formula again. The longest period (under the assumption
> there aren't multiple/continuous errors on the bus) will be
> Frame + Error Frame (12 bit) + 2 x Overload Frame.
^^^^^^^^^^^^^^^^^^^^
How did you find that a error frame is 12 bits? From section 10.4.4
"Specification of EF", I can read:
The EF shall consist of two different fields. The first field
shall be given by the superposition of error flags contributed
from different nodes. The second field shall be the error
delimiter.
Then:
Two forms of error flag may be used, the active error flag and
the passive error flag, where
- the active error flag shall consist of 6 consecutive
dominant bits, and
- the passive error flag shall consist of 6 consecutive
recessive bits unless it is overwritten by dominant bits
from other nodes.
Finally:
The error delimiter shall consist of 8 recessive bits.
So the error frame should be 14 bits (6 + 8), not 12, right?
For the great total, if you want the absolute worst case, you should
consider that:
- The error frame may start at any point. For example, you may
observe the first two bits of the intermission and have it
broken by an error frame. It may also appear in the middle of
a data frame. So you may consider cases such as: partial
frame + error frame + intermission + frame + ...
- The overload frame is required to "destroy the fixed form of
the intermission field". So even if not explicitly specified,
I think that overload frame may start after the second
recessive bits of the intermission, e.g. frame + 2 bits of
intermission + overload frame + 2 bits of intermission +
overload frame + full 3 bits intermission
- An error frame or an overload frame may trigger another error
frame if a node does not correctly receive one of the bits in
the fixed form delimiter. The only exception is the last bit
of that delimiter (c.f. section 10.11 "Error detection"
paragraph d) "Form error"). In other words, you can have
the two overload frames, then an error frame, then two
overload frames again.
This is to say that the worst case scenario is just not something
worth consideration.
The assumption that only one error frame occurs is pretty arbitrary. I
would suggest making it simpler and simply ignore error and overload
frames. As long as frame + intermission works well in empirical tests,
I would suggest to stay with that and call it a day.
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: RE: [PATCH] can: mcp251xfd: Increase poll timeout
2023-05-10 7:50 ` Vincent Mailhol
@ 2023-05-10 9:17 ` Thomas.Kopp
2023-05-10 10:02 ` Vincent Mailhol
0 siblings, 1 reply; 14+ messages in thread
From: Thomas.Kopp @ 2023-05-10 9:17 UTC (permalink / raw
To: vincent.mailhol
Cc: mkl, marex, linux-can, fedor.ross, davem, edumazet, kuba,
lgirdwood, mani, broonie, pabeni, wg
> On Wed. 10 May 2023 at 15:30, <Thomas.Kopp@microchip.com> wrote:
> >
> > > > On 05.05.2023 07:07:03, Thomas.Kopp@microchip.com wrote:
> > > > > > The datasheet "MCP25xxFD Family Reference Manual" says it needs
> an
> > > idle
> > > > > > bus.
> > > > >
> > > > > Technically what's needed is an idle condition on the bus as specified
> > > > > in the ISO. i.e. 11 consecutive sampled recessive bits after a full
> > > > > frame (if one is currently in transmission).
> > > >
> > > > Right. What happens if another CAN frames comes before there are 11
> > > > consecutive sampled recessive bits? The IFS for CAN is 3 bits?
> > >
> > > Not quite. Between the Frames is an IFS that's correct but the IFS consists
> of
> > > an Intermission which is 3 bits long + a bus idle condition of 11 bits. Regular
> > > frames have to wait for the IFS to elapse BUT there's an exception for error
> > > frames and overload frames. EF may be up to 12 bit, OF are 8 dominant + 8
> > > recessive bits, browsing through the spec I think only 2 OFs can happen
> > > consecutively. Adding another 32 bits to the formula should cover this.
> >
> > Re-reading the spec again I noticed that the part where I wrote
> > that there's an "idle condition" after the intermission is
> > wrong. What follows the intermission is just "bus idle",
> > defined two paragraphs later as "The period of bus idle may be
> > of arbitrary length." The 11 recessive bits can be removed from
> > the formula again. The longest period (under the assumption
> > there aren't multiple/continuous errors on the bus) will be
> > Frame + Error Frame (12 bit) + 2 x Overload Frame.
> ^^^^^^^^^^^^^^^^^^^^
>
> How did you find that a error frame is 12 bits? From section 10.4.4
> "Specification of EF", I can read:
>
> The EF shall consist of two different fields. The first field
> shall be given by the superposition of error flags contributed
> from different nodes. The second field shall be the error
> delimiter.
>
> Then:
>
> Two forms of error flag may be used, the active error flag and
> the passive error flag, where
>
> - the active error flag shall consist of 6 consecutive
> dominant bits, and
>
> - the passive error flag shall consist of 6 consecutive
> recessive bits unless it is overwritten by dominant bits
> from other nodes.
>
> Finally:
>
> The error delimiter shall consist of 8 recessive bits.
>
> So the error frame should be 14 bits (6 + 8), not 12, right?
That was imprecise, you're right - I meant an Error Flag, not Frame and hence the 8 recessive bits were missing. There's an active error flag + passive error flag though which can be 6 bits long each. In section 10.4.4.2 The total length of this sequence may vary between a minimum of 6 bit and a maximum of 12 bit.
> For the great total, if you want the absolute worst case, you should
> consider that:
>
> - The error frame may start at any point. For example, you may
> observe the first two bits of the intermission and have it
> broken by an error frame. It may also appear in the middle of
> a data frame. So you may consider cases such as: partial
> frame + error frame + intermission + frame + ...
>
> - The overload frame is required to "destroy the fixed form of
> the intermission field". So even if not explicitly specified,
> I think that overload frame may start after the second
> recessive bits of the intermission, e.g. frame + 2 bits of
> intermission + overload frame + 2 bits of intermission +
> overload frame + full 3 bits intermission
>
> - An error frame or an overload frame may trigger another error
> frame if a node does not correctly receive one of the bits in
> the fixed form delimiter. The only exception is the last bit
> of that delimiter (c.f. section 10.11 "Error detection"
> paragraph d) "Form error"). In other words, you can have
> the two overload frames, then an error frame, then two
> overload frames again.
>
> This is to say that the worst case scenario is just not something
> worth consideration.
ACK
> The assumption that only one error frame occurs is pretty arbitrary. I
> would suggest making it simpler and simply ignore error and overload
> frames. As long as frame + intermission works well in empirical tests,
> I would suggest to stay with that and call it a day.
Correct, that's why I wrote: (under the assumption there aren't multiple/continuous errors on the bus) but I agree that's a pretty arbitrary limit. So I'm fine with changing it to one or two full-sized frames + intermission)
Best Regards,
Thomas
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: RE: [PATCH] can: mcp251xfd: Increase poll timeout
2023-05-10 9:17 ` Thomas.Kopp
@ 2023-05-10 10:02 ` Vincent Mailhol
2023-05-10 10:39 ` Thomas.Kopp
0 siblings, 1 reply; 14+ messages in thread
From: Vincent Mailhol @ 2023-05-10 10:02 UTC (permalink / raw
To: Thomas.Kopp
Cc: mkl, marex, linux-can, fedor.ross, davem, edumazet, kuba,
lgirdwood, mani, broonie, pabeni, wg
Le mer. 10 mai 2023 à 18:18, <Thomas.Kopp@microchip.com> a écrit :
>
> > On Wed. 10 May 2023 at 15:30, <Thomas.Kopp@microchip.com> wrote:
> > >
> > > > > On 05.05.2023 07:07:03, Thomas.Kopp@microchip.com wrote:
> > > > > > > The datasheet "MCP25xxFD Family Reference Manual" says it needs
> > an
> > > > idle
> > > > > > > bus.
> > > > > >
> > > > > > Technically what's needed is an idle condition on the bus as specified
> > > > > > in the ISO. i.e. 11 consecutive sampled recessive bits after a full
> > > > > > frame (if one is currently in transmission).
> > > > >
> > > > > Right. What happens if another CAN frames comes before there are 11
> > > > > consecutive sampled recessive bits? The IFS for CAN is 3 bits?
> > > >
> > > > Not quite. Between the Frames is an IFS that's correct but the IFS consists
> > of
> > > > an Intermission which is 3 bits long + a bus idle condition of 11 bits. Regular
> > > > frames have to wait for the IFS to elapse BUT there's an exception for error
> > > > frames and overload frames. EF may be up to 12 bit, OF are 8 dominant + 8
> > > > recessive bits, browsing through the spec I think only 2 OFs can happen
> > > > consecutively. Adding another 32 bits to the formula should cover this.
> > >
> > > Re-reading the spec again I noticed that the part where I wrote
> > > that there's an "idle condition" after the intermission is
> > > wrong. What follows the intermission is just "bus idle",
> > > defined two paragraphs later as "The period of bus idle may be
> > > of arbitrary length." The 11 recessive bits can be removed from
> > > the formula again. The longest period (under the assumption
> > > there aren't multiple/continuous errors on the bus) will be
> > > Frame + Error Frame (12 bit) + 2 x Overload Frame.
> > ^^^^^^^^^^^^^^^^^^^^
> >
> > How did you find that a error frame is 12 bits? From section 10.4.4
> > "Specification of EF", I can read:
> >
> > The EF shall consist of two different fields. The first field
> > shall be given by the superposition of error flags contributed
> > from different nodes. The second field shall be the error
> > delimiter.
> >
> > Then:
> >
> > Two forms of error flag may be used, the active error flag and
> > the passive error flag, where
> >
> > - the active error flag shall consist of 6 consecutive
> > dominant bits, and
> >
> > - the passive error flag shall consist of 6 consecutive
> > recessive bits unless it is overwritten by dominant bits
> > from other nodes.
> >
> > Finally:
> >
> > The error delimiter shall consist of 8 recessive bits.
> >
> > So the error frame should be 14 bits (6 + 8), not 12, right?
> That was imprecise, you're right - I meant an Error Flag, not Frame and hence the 8 recessive bits were missing. There's an active error flag + passive error flag though which can be 6 bits long each. In section 10.4.4.2 The total length of this sequence may vary between a minimum of 6 bit and a maximum of 12 bit.
The active error flag and the passive error flag may both occur, but
in that case, they occur as a superposition (c.f. above quotation).
This also means that the passive error flag is seen if and only if all
the nodes send a passive error flag. As long as one node sends an
active error flag, the superposition will hide any other passive error
flag.
So, I think that the error flag is always 6.
> > For the great total, if you want the absolute worst case, you should
> > consider that:
> >
> > - The error frame may start at any point. For example, you may
> > observe the first two bits of the intermission and have it
> > broken by an error frame. It may also appear in the middle of
> > a data frame. So you may consider cases such as: partial
> > frame + error frame + intermission + frame + ...
> >
> > - The overload frame is required to "destroy the fixed form of
> > the intermission field". So even if not explicitly specified,
> > I think that overload frame may start after the second
> > recessive bits of the intermission, e.g. frame + 2 bits of
> > intermission + overload frame + 2 bits of intermission +
> > overload frame + full 3 bits intermission
> >
> > - An error frame or an overload frame may trigger another error
> > frame if a node does not correctly receive one of the bits in
> > the fixed form delimiter. The only exception is the last bit
> > of that delimiter (c.f. section 10.11 "Error detection"
> > paragraph d) "Form error"). In other words, you can have
> > the two overload frames, then an error frame, then two
> > overload frames again.
Correcting myself: that last example was invalid. There are at maximum
two overload frames before the next data or remote frame. So the
presence of an error frame should not reset the overload frame count.
> > This is to say that the worst case scenario is just not something
> > worth consideration.
>
> ACK
> > The assumption that only one error frame occurs is pretty arbitrary. I
> > would suggest making it simpler and simply ignore error and overload
> > frames. As long as frame + intermission works well in empirical tests,
> > I would suggest to stay with that and call it a day.
>
> Correct, that's why I wrote: (under the assumption there aren't multiple/continuous errors on the bus) but I agree that's a pretty arbitrary limit. So I'm fine with changing it to one or two full-sized frames + intermission)
ACK. At the end, you have the hardware, so you should decide what is
the best delay to fix the issue. I just hope that my message was
helpful to simplify the formula.
> Best Regards,
> Thomas
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: RE: [PATCH] can: mcp251xfd: Increase poll timeout
2023-05-10 10:02 ` Vincent Mailhol
@ 2023-05-10 10:39 ` Thomas.Kopp
2023-05-10 11:19 ` Vincent Mailhol
0 siblings, 1 reply; 14+ messages in thread
From: Thomas.Kopp @ 2023-05-10 10:39 UTC (permalink / raw
To: vincent.mailhol
Cc: mkl, marex, linux-can, fedor.ross, davem, edumazet, kuba,
lgirdwood, mani, broonie, pabeni, wg
> > > an
> > > > > idle
> > > > > > > > bus.
> > > > > > >
> > > > > > > Technically what's needed is an idle condition on the bus as specified
> > > > > > > in the ISO. i.e. 11 consecutive sampled recessive bits after a full
> > > > > > > frame (if one is currently in transmission).
> > > > > >
> > > > > > Right. What happens if another CAN frames comes before there are
> 11
> > > > > > consecutive sampled recessive bits? The IFS for CAN is 3 bits?
> > > > >
> > > > > Not quite. Between the Frames is an IFS that's correct but the IFS
> consists
> > > of
> > > > > an Intermission which is 3 bits long + a bus idle condition of 11 bits.
> Regular
> > > > > frames have to wait for the IFS to elapse BUT there's an exception for
> error
> > > > > frames and overload frames. EF may be up to 12 bit, OF are 8 dominant
> + 8
> > > > > recessive bits, browsing through the spec I think only 2 OFs can happen
> > > > > consecutively. Adding another 32 bits to the formula should cover this.
> > > >
> > > > Re-reading the spec again I noticed that the part where I wrote
> > > > that there's an "idle condition" after the intermission is
> > > > wrong. What follows the intermission is just "bus idle",
> > > > defined two paragraphs later as "The period of bus idle may be
> > > > of arbitrary length." The 11 recessive bits can be removed from
> > > > the formula again. The longest period (under the assumption
> > > > there aren't multiple/continuous errors on the bus) will be
> > > > Frame + Error Frame (12 bit) + 2 x Overload Frame.
> > > ^^^^^^^^^^^^^^^^^^^^
> > >
> > > How did you find that a error frame is 12 bits? From section 10.4.4
> > > "Specification of EF", I can read:
> > >
> > > The EF shall consist of two different fields. The first field
> > > shall be given by the superposition of error flags contributed
> > > from different nodes. The second field shall be the error
> > > delimiter.
> > >
> > > Then:
> > >
> > > Two forms of error flag may be used, the active error flag and
> > > the passive error flag, where
> > >
> > > - the active error flag shall consist of 6 consecutive
> > > dominant bits, and
> > >
> > > - the passive error flag shall consist of 6 consecutive
> > > recessive bits unless it is overwritten by dominant bits
> > > from other nodes.
> > >
> > > Finally:
> > >
> > > The error delimiter shall consist of 8 recessive bits.
> > >
> > > So the error frame should be 14 bits (6 + 8), not 12, right?
> > That was imprecise, you're right - I meant an Error Flag, not Frame and hence
> the 8 recessive bits were missing. There's an active error flag + passive error
> flag though which can be 6 bits long each. In section 10.4.4.2 The total length
> of this sequence may vary between a minimum of 6 bit and a maximum of 12
> bit.
>
> The active error flag and the passive error flag may both occur, but
> in that case, they occur as a superposition (c.f. above quotation).
> This also means that the passive error flag is seen if and only if all
> the nodes send a passive error flag. As long as one node sends an
> active error flag, the superposition will hide any other passive error
> flag.
>
> So, I think that the error flag is always 6.
I think if one node detects a bit error (that's not on the bus but inside the receiving node) on the second bit of the intermission it will start sending the 6 dominant bits, starting on the third bit of the intermission. Now, according to 10.4.2.2 " A node sampling a dominant bit during its suspend transmission time or at the third bit of intermission shall accept it as SOF.", treating the next 5 dominant bits as part of the ID of a new DF and only then sending its own error flag, giving a total of 12 dominant bits followed by 8 recessive bits EF delimiter etc. Am I missing something here? For this (I believe worst case when only considering 1 EF you could deduct one bittime - the third bit in the intermission)
> > > For the great total, if you want the absolute worst case, you should
> > > consider that:
> > >
> > > - The error frame may start at any point. For example, you may
> > > observe the first two bits of the intermission and have it
> > > broken by an error frame. It may also appear in the middle of
> > > a data frame. So you may consider cases such as: partial
> > > frame + error frame + intermission + frame + ...
> > >
> > > - The overload frame is required to "destroy the fixed form of
> > > the intermission field". So even if not explicitly specified,
> > > I think that overload frame may start after the second
> > > recessive bits of the intermission, e.g. frame + 2 bits of
> > > intermission + overload frame + 2 bits of intermission +
> > > overload frame + full 3 bits intermission
> > >
> > > - An error frame or an overload frame may trigger another error
> > > frame if a node does not correctly receive one of the bits in
> > > the fixed form delimiter. The only exception is the last bit
> > > of that delimiter (c.f. section 10.11 "Error detection"
> > > paragraph d) "Form error"). In other words, you can have
> > > the two overload frames, then an error frame, then two
> > > overload frames again.
>
> Correcting myself: that last example was invalid. There are at maximum
> two overload frames before the next data or remote frame. So the
> presence of an error frame should not reset the overload frame count.
>
> > > This is to say that the worst case scenario is just not something
> > > worth consideration.
> >
> > ACK
> > > The assumption that only one error frame occurs is pretty arbitrary. I
> > > would suggest making it simpler and simply ignore error and overload
> > > frames. As long as frame + intermission works well in empirical tests,
> > > I would suggest to stay with that and call it a day.
> >
> > Correct, that's why I wrote: (under the assumption there aren't
> multiple/continuous errors on the bus) but I agree that's a pretty arbitrary
> limit. So I'm fine with changing it to one or two full-sized frames +
> intermission)
>
> ACK. At the end, you have the hardware, so you should decide what is
> the best delay to fix the issue. I just hope that my message was
> helpful to simplify the formula.
I agree a simple formula will be the best way forward as the rest is academic only anyway.
Best Regards,
Thomas
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: RE: [PATCH] can: mcp251xfd: Increase poll timeout
2023-05-10 10:39 ` Thomas.Kopp
@ 2023-05-10 11:19 ` Vincent Mailhol
2023-05-17 7:15 ` Marc Kleine-Budde
0 siblings, 1 reply; 14+ messages in thread
From: Vincent Mailhol @ 2023-05-10 11:19 UTC (permalink / raw
To: Thomas.Kopp
Cc: mkl, marex, linux-can, fedor.ross, davem, edumazet, kuba,
lgirdwood, mani, broonie, pabeni, wg
On Wed. 10 May 2023 at 19:39, <Thomas.Kopp@microchip.com> wrote:
> > > > > Re-reading the spec again I noticed that the part where I wrote
> > > > > that there's an "idle condition" after the intermission is
> > > > > wrong. What follows the intermission is just "bus idle",
> > > > > defined two paragraphs later as "The period of bus idle may be
> > > > > of arbitrary length." The 11 recessive bits can be removed from
> > > > > the formula again. The longest period (under the assumption
> > > > > there aren't multiple/continuous errors on the bus) will be
> > > > > Frame + Error Frame (12 bit) + 2 x Overload Frame.
> > > > ^^^^^^^^^^^^^^^^^^^^
> > > >
> > > > How did you find that a error frame is 12 bits? From section 10.4.4
> > > > "Specification of EF", I can read:
> > > >
> > > > The EF shall consist of two different fields. The first field
> > > > shall be given by the superposition of error flags contributed
> > > > from different nodes. The second field shall be the error
> > > > delimiter.
> > > >
> > > > Then:
> > > >
> > > > Two forms of error flag may be used, the active error flag and
> > > > the passive error flag, where
> > > >
> > > > - the active error flag shall consist of 6 consecutive
> > > > dominant bits, and
> > > >
> > > > - the passive error flag shall consist of 6 consecutive
> > > > recessive bits unless it is overwritten by dominant bits
> > > > from other nodes.
> > > >
> > > > Finally:
> > > >
> > > > The error delimiter shall consist of 8 recessive bits.
> > > >
> > > > So the error frame should be 14 bits (6 + 8), not 12, right?
> > > That was imprecise, you're right - I meant an Error Flag, not Frame and hence
> > the 8 recessive bits were missing. There's an active error flag + passive error
> > flag though which can be 6 bits long each. In section 10.4.4.2 The total length
> > of this sequence may vary between a minimum of 6 bit and a maximum of 12
> > bit.
> >
> > The active error flag and the passive error flag may both occur, but
> > in that case, they occur as a superposition (c.f. above quotation).
> > This also means that the passive error flag is seen if and only if all
> > the nodes send a passive error flag. As long as one node sends an
> > active error flag, the superposition will hide any other passive error
> > flag.
> >
> > So, I think that the error flag is always 6.
Actually, you were right on the total but not on the reasoning (active
error flag + passive error flag 6 bits long each). The error flags are
actually between 6 and 12 bits but not because of consecutive active
and passive error flags. Even if the field is a superposition of error
flags, the nodes do not necessarily start to transmit their error flag
simultaneously on the same bit. The worst case is when node A sends an
error flag, then node B detects the error condition on the last bit of
A's error flag (bit #6). In that case, B starts to transmit his error
flag after A's error flag is completed (bit #7) and B finishes sending
its error frame on bit #12, thus the total of 6 + 6 = 12. But these
may very well be all dominant bits!
> I think if one node detects a bit error (that's not on the bus but inside the receiving node) on the second bit of the intermission it will start sending the 6 dominant bits, starting on the third bit of the intermission. Now, according to 10.4.2.2 " A node sampling a dominant bit during its suspend transmission time or at the third bit of intermission shall accept it as SOF.", treating the next 5 dominant bits as part of the ID of a new DF and only then sending its own error flag, giving a total of 12 dominant bits followed by 8 recessive bits EF delimiter etc. Am I missing something here? For this (I believe worst case when only considering 1 EF you could deduct one bittime - the third bit in the intermission)
If a node detects a bit error on the second bit of the intermission,
it means that it was expecting a recessive bit and measured a dominant
one. Thus, it should interpret it as the beginning of an overload
flag. Consequently, on the third bit of the intermission, that faulty
node starts to emit an overload flag and according to paragraph
10.4.5.2, all the other nodes shall also start sending an overload
flag.
In fact, we could nearly say that an overload frame is an error frame
sent during intermission. 10.4.6.2 "Intermission" clarifies this
saying that:
During the intermission [...] only signalling of overload condition
is allowed.
The disambiguation between error and overload can be done only because
both can not coexist at the same time!
ISO 11898-1 is such a rabbit hole... sights...
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: RE: [PATCH] can: mcp251xfd: Increase poll timeout
2023-05-10 11:19 ` Vincent Mailhol
@ 2023-05-17 7:15 ` Marc Kleine-Budde
2023-06-11 12:15 ` Marek Vasut
0 siblings, 1 reply; 14+ messages in thread
From: Marc Kleine-Budde @ 2023-05-17 7:15 UTC (permalink / raw
To: Vincent Mailhol
Cc: Thomas.Kopp, marex, linux-can, fedor.ross, davem, edumazet, kuba,
lgirdwood, mani, broonie, pabeni, wg
[-- Attachment #1: Type: text/plain, Size: 641 bytes --]
On 10.05.2023 20:19:38, Vincent Mailhol wrote:
[...]
> ISO 11898-1 is such a rabbit hole... sights...
How do we proceed?
I like Marek's approach of defining the delay first in the driver and
then moving it to a common header in the second patch to facilitate
backporting.
Do we agree on the maximum number of bits to wait in the worst case?
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] 14+ messages in thread
* Re: [PATCH] can: mcp251xfd: Increase poll timeout
2023-05-17 7:15 ` Marc Kleine-Budde
@ 2023-06-11 12:15 ` Marek Vasut
0 siblings, 0 replies; 14+ messages in thread
From: Marek Vasut @ 2023-06-11 12:15 UTC (permalink / raw
To: Marc Kleine-Budde, Vincent Mailhol
Cc: Thomas.Kopp, linux-can, fedor.ross, davem, edumazet, kuba,
lgirdwood, mani, broonie, pabeni, wg
On 5/17/23 09:15, Marc Kleine-Budde wrote:
> On 10.05.2023 20:19:38, Vincent Mailhol wrote:
> [...]
>> ISO 11898-1 is such a rabbit hole... sights...
>
> How do we proceed?
>
> I like Marek's approach of defining the delay first in the driver and
> then moving it to a common header in the second patch to facilitate
> backporting.
>
> Do we agree on the maximum number of bits to wait in the worst case?
Is any further change expected from me to this patch ?
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-06-11 12:15 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-04 19:50 [PATCH] can: mcp251xfd: Increase poll timeout Marek Vasut
2023-05-05 6:50 ` Marc Kleine-Budde
2023-05-05 7:07 ` Thomas.Kopp
2023-05-05 7:58 ` Marc Kleine-Budde
2023-05-05 8:21 ` Thomas.Kopp
2023-05-05 8:39 ` Marc Kleine-Budde
2023-05-10 6:26 ` Thomas.Kopp
2023-05-10 7:50 ` Vincent Mailhol
2023-05-10 9:17 ` Thomas.Kopp
2023-05-10 10:02 ` Vincent Mailhol
2023-05-10 10:39 ` Thomas.Kopp
2023-05-10 11:19 ` Vincent Mailhol
2023-05-17 7:15 ` Marc Kleine-Budde
2023-06-11 12:15 ` Marek Vasut
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.