[RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Franklin S Cooper Jr]

During test transmitting using CAN-FD at high bitrates (4 Mbps) only
resulted in errors. Scoping the signals I noticed that only a single bit
was being transmitted and with a bit more investigation realized the actual
MCAN IP would go back to initialization mode automatically.

It appears this issue is due to the MCAN needing to use the Transmitter
Delay Compensation Mode as defined in the MCAN User's Guide. When this
mode is used the User's Guide indicates that the Transmitter Delay
Compensation Offset register should be set. The document mentions that this
register should be set to (1/dbitrate)/2*(Func Clk Freq).

Additional CAN-CIA's "Bit Time Requirements for CAN FD" document indicates
that this TDC mode is only needed for data bit rates above 2.5 Mbps.
Therefore, only enable this mode and only set TDCO when the data bit rate
is above 2.5 Mbps.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
I'm pretty surprised that this hasn't been implemented already since
the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
supports up to 10 Mbps.

So it will be nice to get comments from users of this driver to understand
if they have been able to use CAN-FD beyond 2.5 Mbps without this patch.
If they haven't what did they do to get around it if they needed higher
speeds.

Meanwhile I plan on testing this using a more "realistic" CAN bus to insure
everything still works at 5 Mbps which is the max speed of my CAN
transceiver.

 drivers/net/can/m_can/m_can.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index f4947a7..720e073 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -126,6 +126,12 @@ enum m_can_mram_cfg {
 #define DBTP_DSJW_SHIFT		0
 #define DBTP_DSJW_MASK		(0xf << DBTP_DSJW_SHIFT)
 
+/* Transmitter Delay Compensation Register (TDCR) */
+#define TDCR_TDCO_SHIFT		8
+#define TDCR_TDCO_MASK		(0x7F << TDCR_TDCO_SHIFT)
+#define TDCR_TDCF_SHIFT		0
+#define TDCR_TDCF_MASK		(0x7F << TDCR_TDCO_SHIFT)
+
 /* Test Register (TEST) */
 #define TEST_LBCK		BIT(4)
 
@@ -977,6 +983,8 @@ static int m_can_set_bittiming(struct net_device *dev)
 	const struct can_bittiming *dbt = &priv->can.data_bittiming;
 	u16 brp, sjw, tseg1, tseg2;
 	u32 reg_btp;
+	u32 enable_tdc = 0;
+	u32 tdco;
 
 	brp = bt->brp - 1;
 	sjw = bt->sjw - 1;
@@ -991,9 +999,23 @@ static int m_can_set_bittiming(struct net_device *dev)
 		sjw = dbt->sjw - 1;
 		tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
 		tseg2 = dbt->phase_seg2 - 1;
+
+		/* TDC is only needed for bitrates beyond 2.5 MBit/s
+		 * Specified in the "Bit Time Requirements for CAN FD" document
+		 */
+		if (dbt->bitrate > 2500000) {
+			enable_tdc = DBTP_TDC;
+			/* Equation based on Bosch's M_CAN User Manual's
+			 * Transmitter Delay Compensation Section
+			 */
+			tdco = priv->can.clock.freq / (dbt->bitrate * 2);
+			m_can_write(priv, M_CAN_TDCR, tdco << TDCR_TDCO_SHIFT);
+		}
+
 		reg_btp = (brp << DBTP_DBRP_SHIFT) | (sjw << DBTP_DSJW_SHIFT) |
 			(tseg1 << DBTP_DTSEG1_SHIFT) |
-			(tseg2 << DBTP_DTSEG2_SHIFT);
+			(tseg2 << DBTP_DTSEG2_SHIFT) | enable_tdc;
+
 		m_can_write(priv, M_CAN_DBTP, reg_btp);
 	}
 
-- 
2.9.4.dirty

[RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Franklin S Cooper Jr]

During test transmitting using CAN-FD at high bitrates (4 Mbps) only
resulted in errors. Scoping the signals I noticed that only a single bit
was being transmitted and with a bit more investigation realized the actual
MCAN IP would go back to initialization mode automatically.

It appears this issue is due to the MCAN needing to use the Transmitter
Delay Compensation Mode as defined in the MCAN User's Guide. When this
mode is used the User's Guide indicates that the Transmitter Delay
Compensation Offset register should be set. The document mentions that this
register should be set to (1/dbitrate)/2*(Func Clk Freq).

Additional CAN-CIA's "Bit Time Requirements for CAN FD" document indicates
that this TDC mode is only needed for data bit rates above 2.5 Mbps.
Therefore, only enable this mode and only set TDCO when the data bit rate
is above 2.5 Mbps.

Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
---
I'm pretty surprised that this hasn't been implemented already since
the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
supports up to 10 Mbps.

So it will be nice to get comments from users of this driver to understand
if they have been able to use CAN-FD beyond 2.5 Mbps without this patch.
If they haven't what did they do to get around it if they needed higher
speeds.

Meanwhile I plan on testing this using a more "realistic" CAN bus to insure
everything still works at 5 Mbps which is the max speed of my CAN
transceiver.

 drivers/net/can/m_can/m_can.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
index f4947a7..720e073 100644
--- a/drivers/net/can/m_can/m_can.c
+++ b/drivers/net/can/m_can/m_can.c
@@ -126,6 +126,12 @@ enum m_can_mram_cfg {
 #define DBTP_DSJW_SHIFT		0
 #define DBTP_DSJW_MASK		(0xf << DBTP_DSJW_SHIFT)
 
+/* Transmitter Delay Compensation Register (TDCR) */
+#define TDCR_TDCO_SHIFT		8
+#define TDCR_TDCO_MASK		(0x7F << TDCR_TDCO_SHIFT)
+#define TDCR_TDCF_SHIFT		0
+#define TDCR_TDCF_MASK		(0x7F << TDCR_TDCO_SHIFT)
+
 /* Test Register (TEST) */
 #define TEST_LBCK		BIT(4)
 
@@ -977,6 +983,8 @@ static int m_can_set_bittiming(struct net_device *dev)
 	const struct can_bittiming *dbt = &priv->can.data_bittiming;
 	u16 brp, sjw, tseg1, tseg2;
 	u32 reg_btp;
+	u32 enable_tdc = 0;
+	u32 tdco;
 
 	brp = bt->brp - 1;
 	sjw = bt->sjw - 1;
@@ -991,9 +999,23 @@ static int m_can_set_bittiming(struct net_device *dev)
 		sjw = dbt->sjw - 1;
 		tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
 		tseg2 = dbt->phase_seg2 - 1;
+
+		/* TDC is only needed for bitrates beyond 2.5 MBit/s
+		 * Specified in the "Bit Time Requirements for CAN FD" document
+		 */
+		if (dbt->bitrate > 2500000) {
+			enable_tdc = DBTP_TDC;
+			/* Equation based on Bosch's M_CAN User Manual's
+			 * Transmitter Delay Compensation Section
+			 */
+			tdco = priv->can.clock.freq / (dbt->bitrate * 2);
+			m_can_write(priv, M_CAN_TDCR, tdco << TDCR_TDCO_SHIFT);
+		}
+
 		reg_btp = (brp << DBTP_DBRP_SHIFT) | (sjw << DBTP_DSJW_SHIFT) |
 			(tseg1 << DBTP_DTSEG1_SHIFT) |
-			(tseg2 << DBTP_DTSEG2_SHIFT);
+			(tseg2 << DBTP_DTSEG2_SHIFT) | enable_tdc;
+
 		m_can_write(priv, M_CAN_DBTP, reg_btp);
 	}
 
-- 
2.9.4.dirty

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Franklin S Cooper Jr]

On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
> During test transmitting using CAN-FD at high bitrates (4 Mbps) only
> resulted in errors. Scoping the signals I noticed that only a single bit
> was being transmitted and with a bit more investigation realized the actual
> MCAN IP would go back to initialization mode automatically.
> 
> It appears this issue is due to the MCAN needing to use the Transmitter
> Delay Compensation Mode as defined in the MCAN User's Guide. When this
> mode is used the User's Guide indicates that the Transmitter Delay
> Compensation Offset register should be set. The document mentions that this
> register should be set to (1/dbitrate)/2*(Func Clk Freq).
> 
> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document indicates
> that this TDC mode is only needed for data bit rates above 2.5 Mbps.
> Therefore, only enable this mode and only set TDCO when the data bit rate
> is above 2.5 Mbps.
> 
> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> ---
> I'm pretty surprised that this hasn't been implemented already since
> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
> supports up to 10 Mbps.
> 
> So it will be nice to get comments from users of this driver to understand
> if they have been able to use CAN-FD beyond 2.5 Mbps without this patch.
> If they haven't what did they do to get around it if they needed higher
> speeds.
> 
> Meanwhile I plan on testing this using a more "realistic" CAN bus to insure
> everything still works at 5 Mbps which is the max speed of my CAN
> transceiver.

ping. Anyone has any thoughts on this?
> 
>  drivers/net/can/m_can/m_can.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index f4947a7..720e073 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -126,6 +126,12 @@ enum m_can_mram_cfg {
>  #define DBTP_DSJW_SHIFT		0
>  #define DBTP_DSJW_MASK		(0xf << DBTP_DSJW_SHIFT)
>  
> +/* Transmitter Delay Compensation Register (TDCR) */
> +#define TDCR_TDCO_SHIFT		8
> +#define TDCR_TDCO_MASK		(0x7F << TDCR_TDCO_SHIFT)
> +#define TDCR_TDCF_SHIFT		0
> +#define TDCR_TDCF_MASK		(0x7F << TDCR_TDCO_SHIFT)
> +
>  /* Test Register (TEST) */
>  #define TEST_LBCK		BIT(4)
>  
> @@ -977,6 +983,8 @@ static int m_can_set_bittiming(struct net_device *dev)
>  	const struct can_bittiming *dbt = &priv->can.data_bittiming;
>  	u16 brp, sjw, tseg1, tseg2;
>  	u32 reg_btp;
> +	u32 enable_tdc = 0;
> +	u32 tdco;
>  
>  	brp = bt->brp - 1;
>  	sjw = bt->sjw - 1;
> @@ -991,9 +999,23 @@ static int m_can_set_bittiming(struct net_device *dev)
>  		sjw = dbt->sjw - 1;
>  		tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
>  		tseg2 = dbt->phase_seg2 - 1;
> +
> +		/* TDC is only needed for bitrates beyond 2.5 MBit/s
> +		 * Specified in the "Bit Time Requirements for CAN FD" document
> +		 */
> +		if (dbt->bitrate > 2500000) {
> +			enable_tdc = DBTP_TDC;
> +			/* Equation based on Bosch's M_CAN User Manual's
> +			 * Transmitter Delay Compensation Section
> +			 */
> +			tdco = priv->can.clock.freq / (dbt->bitrate * 2);
> +			m_can_write(priv, M_CAN_TDCR, tdco << TDCR_TDCO_SHIFT);
> +		}
> +
>  		reg_btp = (brp << DBTP_DBRP_SHIFT) | (sjw << DBTP_DSJW_SHIFT) |
>  			(tseg1 << DBTP_DTSEG1_SHIFT) |
> -			(tseg2 << DBTP_DTSEG2_SHIFT);
> +			(tseg2 << DBTP_DTSEG2_SHIFT) | enable_tdc;
> +
>  		m_can_write(priv, M_CAN_DBTP, reg_btp);
>  	}
>  
> 

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Franklin S Cooper Jr]

On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
> During test transmitting using CAN-FD at high bitrates (4 Mbps) only
> resulted in errors. Scoping the signals I noticed that only a single bit
> was being transmitted and with a bit more investigation realized the actual
> MCAN IP would go back to initialization mode automatically.
> 
> It appears this issue is due to the MCAN needing to use the Transmitter
> Delay Compensation Mode as defined in the MCAN User's Guide. When this
> mode is used the User's Guide indicates that the Transmitter Delay
> Compensation Offset register should be set. The document mentions that this
> register should be set to (1/dbitrate)/2*(Func Clk Freq).
> 
> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document indicates
> that this TDC mode is only needed for data bit rates above 2.5 Mbps.
> Therefore, only enable this mode and only set TDCO when the data bit rate
> is above 2.5 Mbps.
> 
> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> ---
> I'm pretty surprised that this hasn't been implemented already since
> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
> supports up to 10 Mbps.
> 
> So it will be nice to get comments from users of this driver to understand
> if they have been able to use CAN-FD beyond 2.5 Mbps without this patch.
> If they haven't what did they do to get around it if they needed higher
> speeds.
> 
> Meanwhile I plan on testing this using a more "realistic" CAN bus to insure
> everything still works at 5 Mbps which is the max speed of my CAN
> transceiver.

ping. Anyone has any thoughts on this?
> 
>  drivers/net/can/m_can/m_can.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index f4947a7..720e073 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -126,6 +126,12 @@ enum m_can_mram_cfg {
>  #define DBTP_DSJW_SHIFT		0
>  #define DBTP_DSJW_MASK		(0xf << DBTP_DSJW_SHIFT)
>  
> +/* Transmitter Delay Compensation Register (TDCR) */
> +#define TDCR_TDCO_SHIFT		8
> +#define TDCR_TDCO_MASK		(0x7F << TDCR_TDCO_SHIFT)
> +#define TDCR_TDCF_SHIFT		0
> +#define TDCR_TDCF_MASK		(0x7F << TDCR_TDCO_SHIFT)
> +
>  /* Test Register (TEST) */
>  #define TEST_LBCK		BIT(4)
>  
> @@ -977,6 +983,8 @@ static int m_can_set_bittiming(struct net_device *dev)
>  	const struct can_bittiming *dbt = &priv->can.data_bittiming;
>  	u16 brp, sjw, tseg1, tseg2;
>  	u32 reg_btp;
> +	u32 enable_tdc = 0;
> +	u32 tdco;
>  
>  	brp = bt->brp - 1;
>  	sjw = bt->sjw - 1;
> @@ -991,9 +999,23 @@ static int m_can_set_bittiming(struct net_device *dev)
>  		sjw = dbt->sjw - 1;
>  		tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
>  		tseg2 = dbt->phase_seg2 - 1;
> +
> +		/* TDC is only needed for bitrates beyond 2.5 MBit/s
> +		 * Specified in the "Bit Time Requirements for CAN FD" document
> +		 */
> +		if (dbt->bitrate > 2500000) {
> +			enable_tdc = DBTP_TDC;
> +			/* Equation based on Bosch's M_CAN User Manual's
> +			 * Transmitter Delay Compensation Section
> +			 */
> +			tdco = priv->can.clock.freq / (dbt->bitrate * 2);
> +			m_can_write(priv, M_CAN_TDCR, tdco << TDCR_TDCO_SHIFT);
> +		}
> +
>  		reg_btp = (brp << DBTP_DBRP_SHIFT) | (sjw << DBTP_DSJW_SHIFT) |
>  			(tseg1 << DBTP_DTSEG1_SHIFT) |
> -			(tseg2 << DBTP_DTSEG2_SHIFT);
> +			(tseg2 << DBTP_DTSEG2_SHIFT) | enable_tdc;
> +
>  		m_can_write(priv, M_CAN_DBTP, reg_btp);
>  	}
>  
> 

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Sekhar Nori]

On Thursday 14 September 2017 03:28 AM, Franklin S Cooper Jr wrote:
> 
> 
> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
>> During test transmitting using CAN-FD at high bitrates (4 Mbps) only
>> resulted in errors. Scoping the signals I noticed that only a single bit
>> was being transmitted and with a bit more investigation realized the actual
>> MCAN IP would go back to initialization mode automatically.
>>
>> It appears this issue is due to the MCAN needing to use the Transmitter
>> Delay Compensation Mode as defined in the MCAN User's Guide. When this
>> mode is used the User's Guide indicates that the Transmitter Delay
>> Compensation Offset register should be set. The document mentions that this
>> register should be set to (1/dbitrate)/2*(Func Clk Freq).
>>
>> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document indicates
>> that this TDC mode is only needed for data bit rates above 2.5 Mbps.
>> Therefore, only enable this mode and only set TDCO when the data bit rate
>> is above 2.5 Mbps.
>>
>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>> ---
>> I'm pretty surprised that this hasn't been implemented already since
>> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
>> supports up to 10 Mbps.
>>
>> So it will be nice to get comments from users of this driver to understand
>> if they have been able to use CAN-FD beyond 2.5 Mbps without this patch.
>> If they haven't what did they do to get around it if they needed higher
>> speeds.
>>
>> Meanwhile I plan on testing this using a more "realistic" CAN bus to insure
>> everything still works at 5 Mbps which is the max speed of my CAN
>> transceiver.
> 
> ping. Anyone has any thoughts on this?

I added Dong who authored the m_can driver and Wenyou who added the only
in-kernel user of the driver for any help.

Thanks,
Sekhar

>>
>>  drivers/net/can/m_can/m_can.c | 24 +++++++++++++++++++++++-
>>  1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index f4947a7..720e073 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -126,6 +126,12 @@ enum m_can_mram_cfg {
>>  #define DBTP_DSJW_SHIFT		0
>>  #define DBTP_DSJW_MASK		(0xf << DBTP_DSJW_SHIFT)
>>  
>> +/* Transmitter Delay Compensation Register (TDCR) */
>> +#define TDCR_TDCO_SHIFT		8
>> +#define TDCR_TDCO_MASK		(0x7F << TDCR_TDCO_SHIFT)
>> +#define TDCR_TDCF_SHIFT		0
>> +#define TDCR_TDCF_MASK		(0x7F << TDCR_TDCO_SHIFT)
>> +
>>  /* Test Register (TEST) */
>>  #define TEST_LBCK		BIT(4)
>>  
>> @@ -977,6 +983,8 @@ static int m_can_set_bittiming(struct net_device *dev)
>>  	const struct can_bittiming *dbt = &priv->can.data_bittiming;
>>  	u16 brp, sjw, tseg1, tseg2;
>>  	u32 reg_btp;
>> +	u32 enable_tdc = 0;
>> +	u32 tdco;
>>  
>>  	brp = bt->brp - 1;
>>  	sjw = bt->sjw - 1;
>> @@ -991,9 +999,23 @@ static int m_can_set_bittiming(struct net_device *dev)
>>  		sjw = dbt->sjw - 1;
>>  		tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
>>  		tseg2 = dbt->phase_seg2 - 1;
>> +
>> +		/* TDC is only needed for bitrates beyond 2.5 MBit/s
>> +		 * Specified in the "Bit Time Requirements for CAN FD" document
>> +		 */
>> +		if (dbt->bitrate > 2500000) {
>> +			enable_tdc = DBTP_TDC;
>> +			/* Equation based on Bosch's M_CAN User Manual's
>> +			 * Transmitter Delay Compensation Section
>> +			 */
>> +			tdco = priv->can.clock.freq / (dbt->bitrate * 2);
>> +			m_can_write(priv, M_CAN_TDCR, tdco << TDCR_TDCO_SHIFT);
>> +		}
>> +
>>  		reg_btp = (brp << DBTP_DBRP_SHIFT) | (sjw << DBTP_DSJW_SHIFT) |
>>  			(tseg1 << DBTP_DTSEG1_SHIFT) |
>> -			(tseg2 << DBTP_DTSEG2_SHIFT);
>> +			(tseg2 << DBTP_DTSEG2_SHIFT) | enable_tdc;
>> +
>>  		m_can_write(priv, M_CAN_DBTP, reg_btp);
>>  	}
>>  
>>

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Sekhar Nori]

On Thursday 14 September 2017 03:28 AM, Franklin S Cooper Jr wrote:
> 
> 
> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
>> During test transmitting using CAN-FD at high bitrates (4 Mbps) only
>> resulted in errors. Scoping the signals I noticed that only a single bit
>> was being transmitted and with a bit more investigation realized the actual
>> MCAN IP would go back to initialization mode automatically.
>>
>> It appears this issue is due to the MCAN needing to use the Transmitter
>> Delay Compensation Mode as defined in the MCAN User's Guide. When this
>> mode is used the User's Guide indicates that the Transmitter Delay
>> Compensation Offset register should be set. The document mentions that this
>> register should be set to (1/dbitrate)/2*(Func Clk Freq).
>>
>> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document indicates
>> that this TDC mode is only needed for data bit rates above 2.5 Mbps.
>> Therefore, only enable this mode and only set TDCO when the data bit rate
>> is above 2.5 Mbps.
>>
>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>> ---
>> I'm pretty surprised that this hasn't been implemented already since
>> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
>> supports up to 10 Mbps.
>>
>> So it will be nice to get comments from users of this driver to understand
>> if they have been able to use CAN-FD beyond 2.5 Mbps without this patch.
>> If they haven't what did they do to get around it if they needed higher
>> speeds.
>>
>> Meanwhile I plan on testing this using a more "realistic" CAN bus to insure
>> everything still works at 5 Mbps which is the max speed of my CAN
>> transceiver.
> 
> ping. Anyone has any thoughts on this?

I added Dong who authored the m_can driver and Wenyou who added the only
in-kernel user of the driver for any help.

Thanks,
Sekhar

>>
>>  drivers/net/can/m_can/m_can.c | 24 +++++++++++++++++++++++-
>>  1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>> index f4947a7..720e073 100644
>> --- a/drivers/net/can/m_can/m_can.c
>> +++ b/drivers/net/can/m_can/m_can.c
>> @@ -126,6 +126,12 @@ enum m_can_mram_cfg {
>>  #define DBTP_DSJW_SHIFT		0
>>  #define DBTP_DSJW_MASK		(0xf << DBTP_DSJW_SHIFT)
>>  
>> +/* Transmitter Delay Compensation Register (TDCR) */
>> +#define TDCR_TDCO_SHIFT		8
>> +#define TDCR_TDCO_MASK		(0x7F << TDCR_TDCO_SHIFT)
>> +#define TDCR_TDCF_SHIFT		0
>> +#define TDCR_TDCF_MASK		(0x7F << TDCR_TDCO_SHIFT)
>> +
>>  /* Test Register (TEST) */
>>  #define TEST_LBCK		BIT(4)
>>  
>> @@ -977,6 +983,8 @@ static int m_can_set_bittiming(struct net_device *dev)
>>  	const struct can_bittiming *dbt = &priv->can.data_bittiming;
>>  	u16 brp, sjw, tseg1, tseg2;
>>  	u32 reg_btp;
>> +	u32 enable_tdc = 0;
>> +	u32 tdco;
>>  
>>  	brp = bt->brp - 1;
>>  	sjw = bt->sjw - 1;
>> @@ -991,9 +999,23 @@ static int m_can_set_bittiming(struct net_device *dev)
>>  		sjw = dbt->sjw - 1;
>>  		tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
>>  		tseg2 = dbt->phase_seg2 - 1;
>> +
>> +		/* TDC is only needed for bitrates beyond 2.5 MBit/s
>> +		 * Specified in the "Bit Time Requirements for CAN FD" document
>> +		 */
>> +		if (dbt->bitrate > 2500000) {
>> +			enable_tdc = DBTP_TDC;
>> +			/* Equation based on Bosch's M_CAN User Manual's
>> +			 * Transmitter Delay Compensation Section
>> +			 */
>> +			tdco = priv->can.clock.freq / (dbt->bitrate * 2);
>> +			m_can_write(priv, M_CAN_TDCR, tdco << TDCR_TDCO_SHIFT);
>> +		}
>> +
>>  		reg_btp = (brp << DBTP_DBRP_SHIFT) | (sjw << DBTP_DSJW_SHIFT) |
>>  			(tseg1 << DBTP_DTSEG1_SHIFT) |
>> -			(tseg2 << DBTP_DTSEG2_SHIFT);
>> +			(tseg2 << DBTP_DTSEG2_SHIFT) | enable_tdc;
>> +
>>  		m_can_write(priv, M_CAN_DBTP, reg_btp);
>>  	}
>>  
>>

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Yang, Wenyou]

On 2017/9/14 13:06, Sekhar Nori wrote:
> On Thursday 14 September 2017 03:28 AM, Franklin S Cooper Jr wrote:
>>
>> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
>>> During test transmitting using CAN-FD at high bitrates (4 Mbps) only
>>> resulted in errors. Scoping the signals I noticed that only a single bit
>>> was being transmitted and with a bit more investigation realized the actual
>>> MCAN IP would go back to initialization mode automatically.
>>>
>>> It appears this issue is due to the MCAN needing to use the Transmitter
>>> Delay Compensation Mode as defined in the MCAN User's Guide. When this
>>> mode is used the User's Guide indicates that the Transmitter Delay
>>> Compensation Offset register should be set. The document mentions that this
>>> register should be set to (1/dbitrate)/2*(Func Clk Freq).
>>>
>>> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document indicates
>>> that this TDC mode is only needed for data bit rates above 2.5 Mbps.
>>> Therefore, only enable this mode and only set TDCO when the data bit rate
>>> is above 2.5 Mbps.
>>>
>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>>> ---
>>> I'm pretty surprised that this hasn't been implemented already since
>>> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
>>> supports up to 10 Mbps.
>>>
>>> So it will be nice to get comments from users of this driver to understand
>>> if they have been able to use CAN-FD beyond 2.5 Mbps without this patch.
>>> If they haven't what did they do to get around it if they needed higher
>>> speeds.
>>>
>>> Meanwhile I plan on testing this using a more "realistic" CAN bus to insure
>>> everything still works at 5 Mbps which is the max speed of my CAN
>>> transceiver.
>> ping. Anyone has any thoughts on this?
> I added Dong who authored the m_can driver and Wenyou who added the only
> in-kernel user of the driver for any help.
I tested it on SAMA5D2 Xplained board both with and without this patch,  
both work with the 4M bps data bit rate.

>
> Thanks,
> Sekhar
>
>>>   drivers/net/can/m_can/m_can.c | 24 +++++++++++++++++++++++-
>>>   1 file changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>>> index f4947a7..720e073 100644
>>> --- a/drivers/net/can/m_can/m_can.c
>>> +++ b/drivers/net/can/m_can/m_can.c
>>> @@ -126,6 +126,12 @@ enum m_can_mram_cfg {
>>>   #define DBTP_DSJW_SHIFT		0
>>>   #define DBTP_DSJW_MASK		(0xf << DBTP_DSJW_SHIFT)
>>>   
>>> +/* Transmitter Delay Compensation Register (TDCR) */
>>> +#define TDCR_TDCO_SHIFT		8
>>> +#define TDCR_TDCO_MASK		(0x7F << TDCR_TDCO_SHIFT)
>>> +#define TDCR_TDCF_SHIFT		0
>>> +#define TDCR_TDCF_MASK		(0x7F << TDCR_TDCO_SHIFT)
>>> +
>>>   /* Test Register (TEST) */
>>>   #define TEST_LBCK		BIT(4)
>>>   
>>> @@ -977,6 +983,8 @@ static int m_can_set_bittiming(struct net_device *dev)
>>>   	const struct can_bittiming *dbt = &priv->can.data_bittiming;
>>>   	u16 brp, sjw, tseg1, tseg2;
>>>   	u32 reg_btp;
>>> +	u32 enable_tdc = 0;
>>> +	u32 tdco;
>>>   
>>>   	brp = bt->brp - 1;
>>>   	sjw = bt->sjw - 1;
>>> @@ -991,9 +999,23 @@ static int m_can_set_bittiming(struct net_device *dev)
>>>   		sjw = dbt->sjw - 1;
>>>   		tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
>>>   		tseg2 = dbt->phase_seg2 - 1;
>>> +
>>> +		/* TDC is only needed for bitrates beyond 2.5 MBit/s
>>> +		 * Specified in the "Bit Time Requirements for CAN FD" document
>>> +		 */
>>> +		if (dbt->bitrate > 2500000) {
>>> +			enable_tdc = DBTP_TDC;
>>> +			/* Equation based on Bosch's M_CAN User Manual's
>>> +			 * Transmitter Delay Compensation Section
>>> +			 */
>>> +			tdco = priv->can.clock.freq / (dbt->bitrate * 2);
>>> +			m_can_write(priv, M_CAN_TDCR, tdco << TDCR_TDCO_SHIFT);
>>> +		}
>>> +
>>>   		reg_btp = (brp << DBTP_DBRP_SHIFT) | (sjw << DBTP_DSJW_SHIFT) |
>>>   			(tseg1 << DBTP_DTSEG1_SHIFT) |
>>> -			(tseg2 << DBTP_DTSEG2_SHIFT);
>>> +			(tseg2 << DBTP_DTSEG2_SHIFT) | enable_tdc;
>>> +
>>>   		m_can_write(priv, M_CAN_DBTP, reg_btp);
>>>   	}
>>>   
>>>

Regards,
Wenyou Yang

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Yang, Wenyou]

On 2017/9/14 13:06, Sekhar Nori wrote:
> On Thursday 14 September 2017 03:28 AM, Franklin S Cooper Jr wrote:
>>
>> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
>>> During test transmitting using CAN-FD at high bitrates (4 Mbps) only
>>> resulted in errors. Scoping the signals I noticed that only a single bit
>>> was being transmitted and with a bit more investigation realized the actual
>>> MCAN IP would go back to initialization mode automatically.
>>>
>>> It appears this issue is due to the MCAN needing to use the Transmitter
>>> Delay Compensation Mode as defined in the MCAN User's Guide. When this
>>> mode is used the User's Guide indicates that the Transmitter Delay
>>> Compensation Offset register should be set. The document mentions that this
>>> register should be set to (1/dbitrate)/2*(Func Clk Freq).
>>>
>>> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document indicates
>>> that this TDC mode is only needed for data bit rates above 2.5 Mbps.
>>> Therefore, only enable this mode and only set TDCO when the data bit rate
>>> is above 2.5 Mbps.
>>>
>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>>> ---
>>> I'm pretty surprised that this hasn't been implemented already since
>>> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
>>> supports up to 10 Mbps.
>>>
>>> So it will be nice to get comments from users of this driver to understand
>>> if they have been able to use CAN-FD beyond 2.5 Mbps without this patch.
>>> If they haven't what did they do to get around it if they needed higher
>>> speeds.
>>>
>>> Meanwhile I plan on testing this using a more "realistic" CAN bus to insure
>>> everything still works at 5 Mbps which is the max speed of my CAN
>>> transceiver.
>> ping. Anyone has any thoughts on this?
> I added Dong who authored the m_can driver and Wenyou who added the only
> in-kernel user of the driver for any help.
I tested it on SAMA5D2 Xplained board both with and without this patch,  
both work with the 4M bps data bit rate.

>
> Thanks,
> Sekhar
>
>>>   drivers/net/can/m_can/m_can.c | 24 +++++++++++++++++++++++-
>>>   1 file changed, 23 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
>>> index f4947a7..720e073 100644
>>> --- a/drivers/net/can/m_can/m_can.c
>>> +++ b/drivers/net/can/m_can/m_can.c
>>> @@ -126,6 +126,12 @@ enum m_can_mram_cfg {
>>>   #define DBTP_DSJW_SHIFT		0
>>>   #define DBTP_DSJW_MASK		(0xf << DBTP_DSJW_SHIFT)
>>>   
>>> +/* Transmitter Delay Compensation Register (TDCR) */
>>> +#define TDCR_TDCO_SHIFT		8
>>> +#define TDCR_TDCO_MASK		(0x7F << TDCR_TDCO_SHIFT)
>>> +#define TDCR_TDCF_SHIFT		0
>>> +#define TDCR_TDCF_MASK		(0x7F << TDCR_TDCO_SHIFT)
>>> +
>>>   /* Test Register (TEST) */
>>>   #define TEST_LBCK		BIT(4)
>>>   
>>> @@ -977,6 +983,8 @@ static int m_can_set_bittiming(struct net_device *dev)
>>>   	const struct can_bittiming *dbt = &priv->can.data_bittiming;
>>>   	u16 brp, sjw, tseg1, tseg2;
>>>   	u32 reg_btp;
>>> +	u32 enable_tdc = 0;
>>> +	u32 tdco;
>>>   
>>>   	brp = bt->brp - 1;
>>>   	sjw = bt->sjw - 1;
>>> @@ -991,9 +999,23 @@ static int m_can_set_bittiming(struct net_device *dev)
>>>   		sjw = dbt->sjw - 1;
>>>   		tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
>>>   		tseg2 = dbt->phase_seg2 - 1;
>>> +
>>> +		/* TDC is only needed for bitrates beyond 2.5 MBit/s
>>> +		 * Specified in the "Bit Time Requirements for CAN FD" document
>>> +		 */
>>> +		if (dbt->bitrate > 2500000) {
>>> +			enable_tdc = DBTP_TDC;
>>> +			/* Equation based on Bosch's M_CAN User Manual's
>>> +			 * Transmitter Delay Compensation Section
>>> +			 */
>>> +			tdco = priv->can.clock.freq / (dbt->bitrate * 2);
>>> +			m_can_write(priv, M_CAN_TDCR, tdco << TDCR_TDCO_SHIFT);
>>> +		}
>>> +
>>>   		reg_btp = (brp << DBTP_DBRP_SHIFT) | (sjw << DBTP_DSJW_SHIFT) |
>>>   			(tseg1 << DBTP_DTSEG1_SHIFT) |
>>> -			(tseg2 << DBTP_DTSEG2_SHIFT);
>>> +			(tseg2 << DBTP_DTSEG2_SHIFT) | enable_tdc;
>>> +
>>>   		m_can_write(priv, M_CAN_DBTP, reg_btp);
>>>   	}
>>>   
>>>

Regards,
Wenyou Yang

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Franklin S Cooper Jr]

Hi Wenyou,

On 09/17/2017 10:47 PM, Yang, Wenyou wrote:
> 
> 
> On 2017/9/14 13:06, Sekhar Nori wrote:
>> On Thursday 14 September 2017 03:28 AM, Franklin S Cooper Jr wrote:
>>>
>>> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
>>>> During test transmitting using CAN-FD at high bitrates (4 Mbps) only
>>>> resulted in errors. Scoping the signals I noticed that only a single
>>>> bit
>>>> was being transmitted and with a bit more investigation realized the
>>>> actual
>>>> MCAN IP would go back to initialization mode automatically.
>>>>
>>>> It appears this issue is due to the MCAN needing to use the Transmitter
>>>> Delay Compensation Mode as defined in the MCAN User's Guide. When this
>>>> mode is used the User's Guide indicates that the Transmitter Delay
>>>> Compensation Offset register should be set. The document mentions
>>>> that this
>>>> register should be set to (1/dbitrate)/2*(Func Clk Freq).
>>>>
>>>> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document
>>>> indicates
>>>> that this TDC mode is only needed for data bit rates above 2.5 Mbps.
>>>> Therefore, only enable this mode and only set TDCO when the data bit
>>>> rate
>>>> is above 2.5 Mbps.
>>>>
>>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>>>> ---
>>>> I'm pretty surprised that this hasn't been implemented already since
>>>> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
>>>> supports up to 10 Mbps.
>>>>
>>>> So it will be nice to get comments from users of this driver to
>>>> understand
>>>> if they have been able to use CAN-FD beyond 2.5 Mbps without this
>>>> patch.
>>>> If they haven't what did they do to get around it if they needed higher
>>>> speeds.
>>>>
>>>> Meanwhile I plan on testing this using a more "realistic" CAN bus to
>>>> insure
>>>> everything still works at 5 Mbps which is the max speed of my CAN
>>>> transceiver.
>>> ping. Anyone has any thoughts on this?
>> I added Dong who authored the m_can driver and Wenyou who added the only
>> in-kernel user of the driver for any help.
> I tested it on SAMA5D2 Xplained board both with and without this patch, 
> both work with the 4M bps data bit rate.

Thank you for testing this out. Its interesting that you have been able
to use higher speeds without this patch. What is the CAN transceiver
being used on the SAMA5D2 Xplained board? I tried looking at the
schematic but it seems the CAN signals are used on an extension board
which I can't find the schematic for. Also do you mind sharing your test
setup? Were you doing a short point to point test?

Thank You,
Franklin

> 
>>
>> Thanks,
>> Sekhar
>>
>>>>   drivers/net/can/m_can/m_can.c | 24 +++++++++++++++++++++++-
>>>>   1 file changed, 23 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/can/m_can/m_can.c
>>>> b/drivers/net/can/m_can/m_can.c
>>>> index f4947a7..720e073 100644
>>>> --- a/drivers/net/can/m_can/m_can.c
>>>> +++ b/drivers/net/can/m_can/m_can.c
>>>> @@ -126,6 +126,12 @@ enum m_can_mram_cfg {
>>>>   #define DBTP_DSJW_SHIFT        0
>>>>   #define DBTP_DSJW_MASK        (0xf << DBTP_DSJW_SHIFT)
>>>>   +/* Transmitter Delay Compensation Register (TDCR) */
>>>> +#define TDCR_TDCO_SHIFT        8
>>>> +#define TDCR_TDCO_MASK        (0x7F << TDCR_TDCO_SHIFT)
>>>> +#define TDCR_TDCF_SHIFT        0
>>>> +#define TDCR_TDCF_MASK        (0x7F << TDCR_TDCO_SHIFT)
>>>> +
>>>>   /* Test Register (TEST) */
>>>>   #define TEST_LBCK        BIT(4)
>>>>   @@ -977,6 +983,8 @@ static int m_can_set_bittiming(struct
>>>> net_device *dev)
>>>>       const struct can_bittiming *dbt = &priv->can.data_bittiming;
>>>>       u16 brp, sjw, tseg1, tseg2;
>>>>       u32 reg_btp;
>>>> +    u32 enable_tdc = 0;
>>>> +    u32 tdco;
>>>>         brp = bt->brp - 1;
>>>>       sjw = bt->sjw - 1;
>>>> @@ -991,9 +999,23 @@ static int m_can_set_bittiming(struct
>>>> net_device *dev)
>>>>           sjw = dbt->sjw - 1;
>>>>           tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
>>>>           tseg2 = dbt->phase_seg2 - 1;
>>>> +
>>>> +        /* TDC is only needed for bitrates beyond 2.5 MBit/s
>>>> +         * Specified in the "Bit Time Requirements for CAN FD"
>>>> document
>>>> +         */
>>>> +        if (dbt->bitrate > 2500000) {
>>>> +            enable_tdc = DBTP_TDC;
>>>> +            /* Equation based on Bosch's M_CAN User Manual's
>>>> +             * Transmitter Delay Compensation Section
>>>> +             */
>>>> +            tdco = priv->can.clock.freq / (dbt->bitrate * 2);
>>>> +            m_can_write(priv, M_CAN_TDCR, tdco << TDCR_TDCO_SHIFT);
>>>> +        }
>>>> +
>>>>           reg_btp = (brp << DBTP_DBRP_SHIFT) | (sjw <<
>>>> DBTP_DSJW_SHIFT) |
>>>>               (tseg1 << DBTP_DTSEG1_SHIFT) |
>>>> -            (tseg2 << DBTP_DTSEG2_SHIFT);
>>>> +            (tseg2 << DBTP_DTSEG2_SHIFT) | enable_tdc;
>>>> +
>>>>           m_can_write(priv, M_CAN_DBTP, reg_btp);
>>>>       }
>>>>  
> 
> Regards,
> Wenyou Yang
> 

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Franklin S Cooper Jr]

Hi Wenyou,

On 09/17/2017 10:47 PM, Yang, Wenyou wrote:
> 
> 
> On 2017/9/14 13:06, Sekhar Nori wrote:
>> On Thursday 14 September 2017 03:28 AM, Franklin S Cooper Jr wrote:
>>>
>>> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
>>>> During test transmitting using CAN-FD at high bitrates (4 Mbps) only
>>>> resulted in errors. Scoping the signals I noticed that only a single
>>>> bit
>>>> was being transmitted and with a bit more investigation realized the
>>>> actual
>>>> MCAN IP would go back to initialization mode automatically.
>>>>
>>>> It appears this issue is due to the MCAN needing to use the Transmitter
>>>> Delay Compensation Mode as defined in the MCAN User's Guide. When this
>>>> mode is used the User's Guide indicates that the Transmitter Delay
>>>> Compensation Offset register should be set. The document mentions
>>>> that this
>>>> register should be set to (1/dbitrate)/2*(Func Clk Freq).
>>>>
>>>> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document
>>>> indicates
>>>> that this TDC mode is only needed for data bit rates above 2.5 Mbps.
>>>> Therefore, only enable this mode and only set TDCO when the data bit
>>>> rate
>>>> is above 2.5 Mbps.
>>>>
>>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>>>> ---
>>>> I'm pretty surprised that this hasn't been implemented already since
>>>> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
>>>> supports up to 10 Mbps.
>>>>
>>>> So it will be nice to get comments from users of this driver to
>>>> understand
>>>> if they have been able to use CAN-FD beyond 2.5 Mbps without this
>>>> patch.
>>>> If they haven't what did they do to get around it if they needed higher
>>>> speeds.
>>>>
>>>> Meanwhile I plan on testing this using a more "realistic" CAN bus to
>>>> insure
>>>> everything still works at 5 Mbps which is the max speed of my CAN
>>>> transceiver.
>>> ping. Anyone has any thoughts on this?
>> I added Dong who authored the m_can driver and Wenyou who added the only
>> in-kernel user of the driver for any help.
> I tested it on SAMA5D2 Xplained board both with and without this patch, 
> both work with the 4M bps data bit rate.

Thank you for testing this out. Its interesting that you have been able
to use higher speeds without this patch. What is the CAN transceiver
being used on the SAMA5D2 Xplained board? I tried looking at the
schematic but it seems the CAN signals are used on an extension board
which I can't find the schematic for. Also do you mind sharing your test
setup? Were you doing a short point to point test?

Thank You,
Franklin

> 
>>
>> Thanks,
>> Sekhar
>>
>>>>   drivers/net/can/m_can/m_can.c | 24 +++++++++++++++++++++++-
>>>>   1 file changed, 23 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/can/m_can/m_can.c
>>>> b/drivers/net/can/m_can/m_can.c
>>>> index f4947a7..720e073 100644
>>>> --- a/drivers/net/can/m_can/m_can.c
>>>> +++ b/drivers/net/can/m_can/m_can.c
>>>> @@ -126,6 +126,12 @@ enum m_can_mram_cfg {
>>>>   #define DBTP_DSJW_SHIFT        0
>>>>   #define DBTP_DSJW_MASK        (0xf << DBTP_DSJW_SHIFT)
>>>>   +/* Transmitter Delay Compensation Register (TDCR) */
>>>> +#define TDCR_TDCO_SHIFT        8
>>>> +#define TDCR_TDCO_MASK        (0x7F << TDCR_TDCO_SHIFT)
>>>> +#define TDCR_TDCF_SHIFT        0
>>>> +#define TDCR_TDCF_MASK        (0x7F << TDCR_TDCO_SHIFT)
>>>> +
>>>>   /* Test Register (TEST) */
>>>>   #define TEST_LBCK        BIT(4)
>>>>   @@ -977,6 +983,8 @@ static int m_can_set_bittiming(struct
>>>> net_device *dev)
>>>>       const struct can_bittiming *dbt = &priv->can.data_bittiming;
>>>>       u16 brp, sjw, tseg1, tseg2;
>>>>       u32 reg_btp;
>>>> +    u32 enable_tdc = 0;
>>>> +    u32 tdco;
>>>>         brp = bt->brp - 1;
>>>>       sjw = bt->sjw - 1;
>>>> @@ -991,9 +999,23 @@ static int m_can_set_bittiming(struct
>>>> net_device *dev)
>>>>           sjw = dbt->sjw - 1;
>>>>           tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1;
>>>>           tseg2 = dbt->phase_seg2 - 1;
>>>> +
>>>> +        /* TDC is only needed for bitrates beyond 2.5 MBit/s
>>>> +         * Specified in the "Bit Time Requirements for CAN FD"
>>>> document
>>>> +         */
>>>> +        if (dbt->bitrate > 2500000) {
>>>> +            enable_tdc = DBTP_TDC;
>>>> +            /* Equation based on Bosch's M_CAN User Manual's
>>>> +             * Transmitter Delay Compensation Section
>>>> +             */
>>>> +            tdco = priv->can.clock.freq / (dbt->bitrate * 2);
>>>> +            m_can_write(priv, M_CAN_TDCR, tdco << TDCR_TDCO_SHIFT);
>>>> +        }
>>>> +
>>>>           reg_btp = (brp << DBTP_DBRP_SHIFT) | (sjw <<
>>>> DBTP_DSJW_SHIFT) |
>>>>               (tseg1 << DBTP_DTSEG1_SHIFT) |
>>>> -            (tseg2 << DBTP_DTSEG2_SHIFT);
>>>> +            (tseg2 << DBTP_DTSEG2_SHIFT) | enable_tdc;
>>>> +
>>>>           m_can_write(priv, M_CAN_DBTP, reg_btp);
>>>>       }
>>>>  
> 
> Regards,
> Wenyou Yang
> 

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Mario Hüttel]

[-- Attachment #1.1: Type: text/plain, Size: 3666 bytes --]



On 09/20/2017 10:19 PM, Franklin S Cooper Jr wrote:
> Hi Wenyou,
>
> On 09/17/2017 10:47 PM, Yang, Wenyou wrote:
>>
>> On 2017/9/14 13:06, Sekhar Nori wrote:
>>> On Thursday 14 September 2017 03:28 AM, Franklin S Cooper Jr wrote:
>>>> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
>>>>> During test transmitting using CAN-FD at high bitrates (4 Mbps) only
>>>>> resulted in errors. Scoping the signals I noticed that only a single
>>>>> bit
>>>>> was being transmitted and with a bit more investigation realized the
>>>>> actual
>>>>> MCAN IP would go back to initialization mode automatically.
>>>>>
>>>>> It appears this issue is due to the MCAN needing to use the Transmitter
>>>>> Delay Compensation Mode as defined in the MCAN User's Guide. When this
>>>>> mode is used the User's Guide indicates that the Transmitter Delay
>>>>> Compensation Offset register should be set. The document mentions
>>>>> that this
>>>>> register should be set to (1/dbitrate)/2*(Func Clk Freq).
>>>>>
>>>>> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document
>>>>> indicates
>>>>> that this TDC mode is only needed for data bit rates above 2.5 Mbps.
>>>>> Therefore, only enable this mode and only set TDCO when the data bit
>>>>> rate
>>>>> is above 2.5 Mbps.
>>>>>
>>>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>>>>> ---
>>>>> I'm pretty surprised that this hasn't been implemented already since
>>>>> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
>>>>> supports up to 10 Mbps.
>>>>>
>>>>> So it will be nice to get comments from users of this driver to
>>>>> understand
>>>>> if they have been able to use CAN-FD beyond 2.5 Mbps without this
>>>>> patch.
>>>>> If they haven't what did they do to get around it if they needed higher
>>>>> speeds.
>>>>>
>>>>> Meanwhile I plan on testing this using a more "realistic" CAN bus to
>>>>> insure
>>>>> everything still works at 5 Mbps which is the max speed of my CAN
>>>>> transceiver.
>>>> ping. Anyone has any thoughts on this?
>>> I added Dong who authored the m_can driver and Wenyou who added the only
>>> in-kernel user of the driver for any help.
>> I tested it on SAMA5D2 Xplained board both with and without this patch, 
>> both work with the 4M bps data bit rate.
> Thank you for testing this out. Its interesting that you have been able
> to use higher speeds without this patch. What is the CAN transceiver
> being used on the SAMA5D2 Xplained board? I tried looking at the
> schematic but it seems the CAN signals are used on an extension board
> which I can't find the schematic for. Also do you mind sharing your test
> setup? Were you doing a short point to point test?
>
> Thank You,
> Franklin
Hello Franklin,

your patch definitely makes sense.

I forgot the TDC in my patches because it was not present in the
previous driver versions and because I didn't encounter any
problems when testing it myself.

The error is highly dependent on the hardware (transceiver) setup.
So it is definitely possible that some people don't encounter errors
without your patch.

Could you clarify what you meant with
> Scoping the signals I noticed that only a single bit was being transmitted 

Do you mean one data bit (high bit rate)  or did the core already fail
in the arbitration phase?

There is also another aspect that can lead to errors:

If the CAN clock 'cclk' is above the frequency of the interface/logic
clock 'hclk', the clock domain crossing of the CAN messages can't
work properly. However, I will throw this topic as an extra e-mail into
the round.

Mario

 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Franklin S Cooper Jr]

On 09/20/2017 04:37 PM, Mario Hüttel wrote:
> 
> 
> On 09/20/2017 10:19 PM, Franklin S Cooper Jr wrote:
>> Hi Wenyou,
>>
>> On 09/17/2017 10:47 PM, Yang, Wenyou wrote:
>>>
>>> On 2017/9/14 13:06, Sekhar Nori wrote:
>>>> On Thursday 14 September 2017 03:28 AM, Franklin S Cooper Jr wrote:
>>>>> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
>>>>>> During test transmitting using CAN-FD at high bitrates (4 Mbps) only
>>>>>> resulted in errors. Scoping the signals I noticed that only a single
>>>>>> bit
>>>>>> was being transmitted and with a bit more investigation realized the
>>>>>> actual
>>>>>> MCAN IP would go back to initialization mode automatically.
>>>>>>
>>>>>> It appears this issue is due to the MCAN needing to use the Transmitter
>>>>>> Delay Compensation Mode as defined in the MCAN User's Guide. When this
>>>>>> mode is used the User's Guide indicates that the Transmitter Delay
>>>>>> Compensation Offset register should be set. The document mentions
>>>>>> that this
>>>>>> register should be set to (1/dbitrate)/2*(Func Clk Freq).
>>>>>>
>>>>>> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document
>>>>>> indicates
>>>>>> that this TDC mode is only needed for data bit rates above 2.5 Mbps.
>>>>>> Therefore, only enable this mode and only set TDCO when the data bit
>>>>>> rate
>>>>>> is above 2.5 Mbps.
>>>>>>
>>>>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>>>>>> ---
>>>>>> I'm pretty surprised that this hasn't been implemented already since
>>>>>> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
>>>>>> supports up to 10 Mbps.
>>>>>>
>>>>>> So it will be nice to get comments from users of this driver to
>>>>>> understand
>>>>>> if they have been able to use CAN-FD beyond 2.5 Mbps without this
>>>>>> patch.
>>>>>> If they haven't what did they do to get around it if they needed higher
>>>>>> speeds.
>>>>>>
>>>>>> Meanwhile I plan on testing this using a more "realistic" CAN bus to
>>>>>> insure
>>>>>> everything still works at 5 Mbps which is the max speed of my CAN
>>>>>> transceiver.
>>>>> ping. Anyone has any thoughts on this?
>>>> I added Dong who authored the m_can driver and Wenyou who added the only
>>>> in-kernel user of the driver for any help.
>>> I tested it on SAMA5D2 Xplained board both with and without this patch, 
>>> both work with the 4M bps data bit rate.
>> Thank you for testing this out. Its interesting that you have been able
>> to use higher speeds without this patch. What is the CAN transceiver
>> being used on the SAMA5D2 Xplained board? I tried looking at the
>> schematic but it seems the CAN signals are used on an extension board
>> which I can't find the schematic for. Also do you mind sharing your test
>> setup? Were you doing a short point to point test?
>>
>> Thank You,
>> Franklin
> Hello Franklin,
> 
> your patch definitely makes sense.
> 
> I forgot the TDC in my patches because it was not present in the
> previous driver versions and because I didn't encounter any
> problems when testing it myself.
> 
> The error is highly dependent on the hardware (transceiver) setup.
> So it is definitely possible that some people don't encounter errors
> without your patch.

So the Transmission Delay Compensation feature Value register is suppose
to take into consideration the transceiver delay automatically and add
the value of TDCO on top of that. So why would TDCO be dependent on the
transceiver? I've heard conflicting things regarding TDC so any
clarification on what actually impacts it would be appreciated.

Also part of the issue I'm having is how can we properly configure TDCO?
Configuring TDCO is essentially figuring out what Secondary Sample Point
to use. However, it is unclear what value to set SSP to and which use
cases a given SSP will work or doesn't work. I've seen various
recommendations from Bosch on choosing SSP but ultimately it seems they
suggestion "real world testing" to come up with a proper value. Not
setting TDCO causes problems for my device and improperly setting TDCO
causes problems for my device. So its likely any value I use could end
up breaking something for someone else.

Currently I leaning to a DT property that can be used for setting SSP.
Perhaps use a generic default value and allow individuals to override it
via DT?
> 
> Could you clarify what you meant with
>> Scoping the signals I noticed that only a single bit was being transmitted 
> 
> Do you mean one data bit (high bit rate)  or did the core already fail
> in the arbitration phase?

A single bit during the arbitration phase. Essentially the Start of
Frame bit is sent and then nothing else and the IP resets.
> 
> There is also another aspect that can lead to errors:
> 
> If the CAN clock 'cclk' is above the frequency of the interface/logic
> clock 'hclk', the clock domain crossing of the CAN messages can't
> work properly. However, I will throw this topic as an extra e-mail into
> the round.
> 
> Mario
> 
>  
> 

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 4910 bytes --]

On 09/21/2017 02:48 AM, Franklin S Cooper Jr wrote:
> 
> 
> On 09/20/2017 04:37 PM, Mario Hüttel wrote:
>>
>>
>> On 09/20/2017 10:19 PM, Franklin S Cooper Jr wrote:
>>> Hi Wenyou,
>>>
>>> On 09/17/2017 10:47 PM, Yang, Wenyou wrote:
>>>>
>>>> On 2017/9/14 13:06, Sekhar Nori wrote:
>>>>> On Thursday 14 September 2017 03:28 AM, Franklin S Cooper Jr wrote:
>>>>>> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
>>>>>>> During test transmitting using CAN-FD at high bitrates (4 Mbps) only
>>>>>>> resulted in errors. Scoping the signals I noticed that only a single
>>>>>>> bit
>>>>>>> was being transmitted and with a bit more investigation realized the
>>>>>>> actual
>>>>>>> MCAN IP would go back to initialization mode automatically.
>>>>>>>
>>>>>>> It appears this issue is due to the MCAN needing to use the Transmitter
>>>>>>> Delay Compensation Mode as defined in the MCAN User's Guide. When this
>>>>>>> mode is used the User's Guide indicates that the Transmitter Delay
>>>>>>> Compensation Offset register should be set. The document mentions
>>>>>>> that this
>>>>>>> register should be set to (1/dbitrate)/2*(Func Clk Freq).
>>>>>>>
>>>>>>> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document
>>>>>>> indicates
>>>>>>> that this TDC mode is only needed for data bit rates above 2.5 Mbps.
>>>>>>> Therefore, only enable this mode and only set TDCO when the data bit
>>>>>>> rate
>>>>>>> is above 2.5 Mbps.
>>>>>>>
>>>>>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>>>>>>> ---
>>>>>>> I'm pretty surprised that this hasn't been implemented already since
>>>>>>> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
>>>>>>> supports up to 10 Mbps.
>>>>>>>
>>>>>>> So it will be nice to get comments from users of this driver to
>>>>>>> understand
>>>>>>> if they have been able to use CAN-FD beyond 2.5 Mbps without this
>>>>>>> patch.
>>>>>>> If they haven't what did they do to get around it if they needed higher
>>>>>>> speeds.
>>>>>>>
>>>>>>> Meanwhile I plan on testing this using a more "realistic" CAN bus to
>>>>>>> insure
>>>>>>> everything still works at 5 Mbps which is the max speed of my CAN
>>>>>>> transceiver.
>>>>>> ping. Anyone has any thoughts on this?
>>>>> I added Dong who authored the m_can driver and Wenyou who added the only
>>>>> in-kernel user of the driver for any help.
>>>> I tested it on SAMA5D2 Xplained board both with and without this patch, 
>>>> both work with the 4M bps data bit rate.
>>> Thank you for testing this out. Its interesting that you have been able
>>> to use higher speeds without this patch. What is the CAN transceiver
>>> being used on the SAMA5D2 Xplained board? I tried looking at the
>>> schematic but it seems the CAN signals are used on an extension board
>>> which I can't find the schematic for. Also do you mind sharing your test
>>> setup? Were you doing a short point to point test?
>>>
>>> Thank You,
>>> Franklin
>> Hello Franklin,
>>
>> your patch definitely makes sense.
>>
>> I forgot the TDC in my patches because it was not present in the
>> previous driver versions and because I didn't encounter any
>> problems when testing it myself.
>>
>> The error is highly dependent on the hardware (transceiver) setup.
>> So it is definitely possible that some people don't encounter errors
>> without your patch.
> 
> So the Transmission Delay Compensation feature Value register is suppose
> to take into consideration the transceiver delay automatically and add
> the value of TDCO on top of that. So why would TDCO be dependent on the
> transceiver? I've heard conflicting things regarding TDC so any
> clarification on what actually impacts it would be appreciated.
> 
> Also part of the issue I'm having is how can we properly configure TDCO?
> Configuring TDCO is essentially figuring out what Secondary Sample Point
> to use. However, it is unclear what value to set SSP to and which use
> cases a given SSP will work or doesn't work. I've seen various
> recommendations from Bosch on choosing SSP but ultimately it seems they
> suggestion "real world testing" to come up with a proper value. Not
> setting TDCO causes problems for my device and improperly setting TDCO
> causes problems for my device. So its likely any value I use could end
> up breaking something for someone else.
> 
> Currently I leaning to a DT property that can be used for setting SSP.
> Perhaps use a generic default value and allow individuals to override it
> via DT?

Sounds reasonable. What's the status of this series?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Sekhar Nori]

Hi Marc,

On Wednesday 18 October 2017 06:14 PM, Marc Kleine-Budde wrote:
> On 09/21/2017 02:48 AM, Franklin S Cooper Jr wrote:
>>
>>
>> On 09/20/2017 04:37 PM, Mario Hüttel wrote:
>>>
>>>
>>> On 09/20/2017 10:19 PM, Franklin S Cooper Jr wrote:
>>>> Hi Wenyou,
>>>>
>>>> On 09/17/2017 10:47 PM, Yang, Wenyou wrote:
>>>>>
>>>>> On 2017/9/14 13:06, Sekhar Nori wrote:
>>>>>> On Thursday 14 September 2017 03:28 AM, Franklin S Cooper Jr wrote:
>>>>>>> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
>>>>>>>> During test transmitting using CAN-FD at high bitrates (4 Mbps) only
>>>>>>>> resulted in errors. Scoping the signals I noticed that only a single
>>>>>>>> bit
>>>>>>>> was being transmitted and with a bit more investigation realized the
>>>>>>>> actual
>>>>>>>> MCAN IP would go back to initialization mode automatically.
>>>>>>>>
>>>>>>>> It appears this issue is due to the MCAN needing to use the Transmitter
>>>>>>>> Delay Compensation Mode as defined in the MCAN User's Guide. When this
>>>>>>>> mode is used the User's Guide indicates that the Transmitter Delay
>>>>>>>> Compensation Offset register should be set. The document mentions
>>>>>>>> that this
>>>>>>>> register should be set to (1/dbitrate)/2*(Func Clk Freq).
>>>>>>>>
>>>>>>>> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document
>>>>>>>> indicates
>>>>>>>> that this TDC mode is only needed for data bit rates above 2.5 Mbps.
>>>>>>>> Therefore, only enable this mode and only set TDCO when the data bit
>>>>>>>> rate
>>>>>>>> is above 2.5 Mbps.
>>>>>>>>
>>>>>>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>>>>>>>> ---
>>>>>>>> I'm pretty surprised that this hasn't been implemented already since
>>>>>>>> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
>>>>>>>> supports up to 10 Mbps.
>>>>>>>>
>>>>>>>> So it will be nice to get comments from users of this driver to
>>>>>>>> understand
>>>>>>>> if they have been able to use CAN-FD beyond 2.5 Mbps without this
>>>>>>>> patch.
>>>>>>>> If they haven't what did they do to get around it if they needed higher
>>>>>>>> speeds.
>>>>>>>>
>>>>>>>> Meanwhile I plan on testing this using a more "realistic" CAN bus to
>>>>>>>> insure
>>>>>>>> everything still works at 5 Mbps which is the max speed of my CAN
>>>>>>>> transceiver.
>>>>>>> ping. Anyone has any thoughts on this?
>>>>>> I added Dong who authored the m_can driver and Wenyou who added the only
>>>>>> in-kernel user of the driver for any help.
>>>>> I tested it on SAMA5D2 Xplained board both with and without this patch, 
>>>>> both work with the 4M bps data bit rate.
>>>> Thank you for testing this out. Its interesting that you have been able
>>>> to use higher speeds without this patch. What is the CAN transceiver
>>>> being used on the SAMA5D2 Xplained board? I tried looking at the
>>>> schematic but it seems the CAN signals are used on an extension board
>>>> which I can't find the schematic for. Also do you mind sharing your test
>>>> setup? Were you doing a short point to point test?
>>>>
>>>> Thank You,
>>>> Franklin
>>> Hello Franklin,
>>>
>>> your patch definitely makes sense.
>>>
>>> I forgot the TDC in my patches because it was not present in the
>>> previous driver versions and because I didn't encounter any
>>> problems when testing it myself.
>>>
>>> The error is highly dependent on the hardware (transceiver) setup.
>>> So it is definitely possible that some people don't encounter errors
>>> without your patch.
>>
>> So the Transmission Delay Compensation feature Value register is suppose
>> to take into consideration the transceiver delay automatically and add
>> the value of TDCO on top of that. So why would TDCO be dependent on the
>> transceiver? I've heard conflicting things regarding TDC so any
>> clarification on what actually impacts it would be appreciated.
>>
>> Also part of the issue I'm having is how can we properly configure TDCO?
>> Configuring TDCO is essentially figuring out what Secondary Sample Point
>> to use. However, it is unclear what value to set SSP to and which use
>> cases a given SSP will work or doesn't work. I've seen various
>> recommendations from Bosch on choosing SSP but ultimately it seems they
>> suggestion "real world testing" to come up with a proper value. Not
>> setting TDCO causes problems for my device and improperly setting TDCO
>> causes problems for my device. So its likely any value I use could end
>> up breaking something for someone else.
>>
>> Currently I leaning to a DT property that can be used for setting SSP.
>> Perhaps use a generic default value and allow individuals to override it
>> via DT?
> 
> Sounds reasonable. What's the status of this series?

I have had some offline discussions with Franklin on this, and I am not
fully convinced that DT is the way to go here (although I don't have the
agreement with Franklin there).

There are two components in configuring the secondary sample point. It
is the transceiver loopback delay and an offset (example half of the bit
time in data phase).

While the transceiver loopback delay is pretty board dependent (and thus
amenable to DT encoding), I am not quite sure the offset can be
configured in DT because its not really board dependent.

Unfortunately, offset calculation does not seem to be an exact science.
There are recommendations ranging from using 50% of bit time to making
it same as the sample point configured. This means users who need to
change the SSP due to offset variations need to change  their DT even
without anything changing on their board.

Since we have a netlink socket interface to configure sample point, I
wonder if that should be extended to configure SSP too (or at least the
offset part of SSP)?

Thanks,
Sekhar

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Franklin S Cooper Jr]

On 10/18/2017 08:24 AM, Sekhar Nori wrote:
> Hi Marc,
> 
> On Wednesday 18 October 2017 06:14 PM, Marc Kleine-Budde wrote:
>> On 09/21/2017 02:48 AM, Franklin S Cooper Jr wrote:
>>>
>>>
>>> On 09/20/2017 04:37 PM, Mario Hüttel wrote:
>>>>
>>>>
>>>> On 09/20/2017 10:19 PM, Franklin S Cooper Jr wrote:
>>>>> Hi Wenyou,
>>>>>
>>>>> On 09/17/2017 10:47 PM, Yang, Wenyou wrote:
>>>>>>
>>>>>> On 2017/9/14 13:06, Sekhar Nori wrote:
>>>>>>> On Thursday 14 September 2017 03:28 AM, Franklin S Cooper Jr wrote:
>>>>>>>> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
>>>>>>>>> During test transmitting using CAN-FD at high bitrates (4 Mbps) only
>>>>>>>>> resulted in errors. Scoping the signals I noticed that only a single
>>>>>>>>> bit
>>>>>>>>> was being transmitted and with a bit more investigation realized the
>>>>>>>>> actual
>>>>>>>>> MCAN IP would go back to initialization mode automatically.
>>>>>>>>>
>>>>>>>>> It appears this issue is due to the MCAN needing to use the Transmitter
>>>>>>>>> Delay Compensation Mode as defined in the MCAN User's Guide. When this
>>>>>>>>> mode is used the User's Guide indicates that the Transmitter Delay
>>>>>>>>> Compensation Offset register should be set. The document mentions
>>>>>>>>> that this
>>>>>>>>> register should be set to (1/dbitrate)/2*(Func Clk Freq).
>>>>>>>>>
>>>>>>>>> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document
>>>>>>>>> indicates
>>>>>>>>> that this TDC mode is only needed for data bit rates above 2.5 Mbps.
>>>>>>>>> Therefore, only enable this mode and only set TDCO when the data bit
>>>>>>>>> rate
>>>>>>>>> is above 2.5 Mbps.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>>>>>>>>> ---
>>>>>>>>> I'm pretty surprised that this hasn't been implemented already since
>>>>>>>>> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
>>>>>>>>> supports up to 10 Mbps.
>>>>>>>>>
>>>>>>>>> So it will be nice to get comments from users of this driver to
>>>>>>>>> understand
>>>>>>>>> if they have been able to use CAN-FD beyond 2.5 Mbps without this
>>>>>>>>> patch.
>>>>>>>>> If they haven't what did they do to get around it if they needed higher
>>>>>>>>> speeds.
>>>>>>>>>
>>>>>>>>> Meanwhile I plan on testing this using a more "realistic" CAN bus to
>>>>>>>>> insure
>>>>>>>>> everything still works at 5 Mbps which is the max speed of my CAN
>>>>>>>>> transceiver.
>>>>>>>> ping. Anyone has any thoughts on this?
>>>>>>> I added Dong who authored the m_can driver and Wenyou who added the only
>>>>>>> in-kernel user of the driver for any help.
>>>>>> I tested it on SAMA5D2 Xplained board both with and without this patch, 
>>>>>> both work with the 4M bps data bit rate.
>>>>> Thank you for testing this out. Its interesting that you have been able
>>>>> to use higher speeds without this patch. What is the CAN transceiver
>>>>> being used on the SAMA5D2 Xplained board? I tried looking at the
>>>>> schematic but it seems the CAN signals are used on an extension board
>>>>> which I can't find the schematic for. Also do you mind sharing your test
>>>>> setup? Were you doing a short point to point test?
>>>>>
>>>>> Thank You,
>>>>> Franklin
>>>> Hello Franklin,
>>>>
>>>> your patch definitely makes sense.
>>>>
>>>> I forgot the TDC in my patches because it was not present in the
>>>> previous driver versions and because I didn't encounter any
>>>> problems when testing it myself.
>>>>
>>>> The error is highly dependent on the hardware (transceiver) setup.
>>>> So it is definitely possible that some people don't encounter errors
>>>> without your patch.
>>>
>>> So the Transmission Delay Compensation feature Value register is suppose
>>> to take into consideration the transceiver delay automatically and add
>>> the value of TDCO on top of that. So why would TDCO be dependent on the
>>> transceiver? I've heard conflicting things regarding TDC so any
>>> clarification on what actually impacts it would be appreciated.
>>>
>>> Also part of the issue I'm having is how can we properly configure TDCO?
>>> Configuring TDCO is essentially figuring out what Secondary Sample Point
>>> to use. However, it is unclear what value to set SSP to and which use
>>> cases a given SSP will work or doesn't work. I've seen various
>>> recommendations from Bosch on choosing SSP but ultimately it seems they
>>> suggestion "real world testing" to come up with a proper value. Not
>>> setting TDCO causes problems for my device and improperly setting TDCO
>>> causes problems for my device. So its likely any value I use could end
>>> up breaking something for someone else.
>>>
>>> Currently I leaning to a DT property that can be used for setting SSP.
>>> Perhaps use a generic default value and allow individuals to override it
>>> via DT?
>>
>> Sounds reasonable. What's the status of this series?
> 
> I have had some offline discussions with Franklin on this, and I am not
> fully convinced that DT is the way to go here (although I don't have the
> agreement with Franklin there).

Probably the fundamental area where we disagree is what "default" SSP
value should be used. Based on a short (< 1 ft) point to point test
using a SSP of 50% worked fine. However, I'm not convinced that this
default value of 50% will work in a more "traditional" CAN bus at higher
speeds. Nor am I convinced that a SSP of 50% will work on every MCAN
board in even the simplest test cases.

I believe that this default SSP should be a DT property that allows any
board to determine what default value works best in general.
> 
> There are two components in configuring the secondary sample point. It
> is the transceiver loopback delay and an offset (example half of the bit
> time in data phase).
> 
> While the transceiver loopback delay is pretty board dependent (and thus
> amenable to DT encoding), I am not quite sure the offset can be
> configured in DT because its not really board dependent.
> 
> Unfortunately, offset calculation does not seem to be an exact science.
> There are recommendations ranging from using 50% of bit time to making
> it same as the sample point configured. This means users who need to
> change the SSP due to offset variations need to change  their DT even
> without anything changing on their board.
> 
> Since we have a netlink socket interface to configure sample point, I
> wonder if that should be extended to configure SSP too (or at least the
> offset part of SSP)?

Sekhar is right that ideally the user should be able to set the SSP at
runtime. However, my issue is that based on my experience CAN users
expect the driver to just work the majority of times. For unique use
cases where the driver calculated values don't work then the user should
be able to override it. This should only be done for a very small
percentage of CAN users. Unless you allow DT to provide a default SSP
many users of MCAN may find that the default SSP doesn't work and must
always use runtime overrides to get anything to work. I don't think that
is a good user experience which is why I don't like the idea.


> 
> Thanks,
> Sekhar
> 

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Sekhar Nori]

On Wednesday 18 October 2017 07:47 PM, Franklin S Cooper Jr wrote:
> 
> 
> On 10/18/2017 08:24 AM, Sekhar Nori wrote:
>> Hi Marc,
>>
>> On Wednesday 18 October 2017 06:14 PM, Marc Kleine-Budde wrote:
>>> On 09/21/2017 02:48 AM, Franklin S Cooper Jr wrote:
>>>>
>>>>
>>>> On 09/20/2017 04:37 PM, Mario Hüttel wrote:
>>>>>
>>>>>
>>>>> On 09/20/2017 10:19 PM, Franklin S Cooper Jr wrote:
>>>>>> Hi Wenyou,
>>>>>>
>>>>>> On 09/17/2017 10:47 PM, Yang, Wenyou wrote:
>>>>>>>
>>>>>>> On 2017/9/14 13:06, Sekhar Nori wrote:
>>>>>>>> On Thursday 14 September 2017 03:28 AM, Franklin S Cooper Jr wrote:
>>>>>>>>> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
>>>>>>>>>> During test transmitting using CAN-FD at high bitrates (4 Mbps) only
>>>>>>>>>> resulted in errors. Scoping the signals I noticed that only a single
>>>>>>>>>> bit
>>>>>>>>>> was being transmitted and with a bit more investigation realized the
>>>>>>>>>> actual
>>>>>>>>>> MCAN IP would go back to initialization mode automatically.
>>>>>>>>>>
>>>>>>>>>> It appears this issue is due to the MCAN needing to use the Transmitter
>>>>>>>>>> Delay Compensation Mode as defined in the MCAN User's Guide. When this
>>>>>>>>>> mode is used the User's Guide indicates that the Transmitter Delay
>>>>>>>>>> Compensation Offset register should be set. The document mentions
>>>>>>>>>> that this
>>>>>>>>>> register should be set to (1/dbitrate)/2*(Func Clk Freq).
>>>>>>>>>>
>>>>>>>>>> Additional CAN-CIA's "Bit Time Requirements for CAN FD" document
>>>>>>>>>> indicates
>>>>>>>>>> that this TDC mode is only needed for data bit rates above 2.5 Mbps.
>>>>>>>>>> Therefore, only enable this mode and only set TDCO when the data bit
>>>>>>>>>> rate
>>>>>>>>>> is above 2.5 Mbps.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
>>>>>>>>>> ---
>>>>>>>>>> I'm pretty surprised that this hasn't been implemented already since
>>>>>>>>>> the primary purpose of CAN-FD is to go beyond 1 Mbps and the MCAN IP
>>>>>>>>>> supports up to 10 Mbps.
>>>>>>>>>>
>>>>>>>>>> So it will be nice to get comments from users of this driver to
>>>>>>>>>> understand
>>>>>>>>>> if they have been able to use CAN-FD beyond 2.5 Mbps without this
>>>>>>>>>> patch.
>>>>>>>>>> If they haven't what did they do to get around it if they needed higher
>>>>>>>>>> speeds.
>>>>>>>>>>
>>>>>>>>>> Meanwhile I plan on testing this using a more "realistic" CAN bus to
>>>>>>>>>> insure
>>>>>>>>>> everything still works at 5 Mbps which is the max speed of my CAN
>>>>>>>>>> transceiver.
>>>>>>>>> ping. Anyone has any thoughts on this?
>>>>>>>> I added Dong who authored the m_can driver and Wenyou who added the only
>>>>>>>> in-kernel user of the driver for any help.
>>>>>>> I tested it on SAMA5D2 Xplained board both with and without this patch, 
>>>>>>> both work with the 4M bps data bit rate.
>>>>>> Thank you for testing this out. Its interesting that you have been able
>>>>>> to use higher speeds without this patch. What is the CAN transceiver
>>>>>> being used on the SAMA5D2 Xplained board? I tried looking at the
>>>>>> schematic but it seems the CAN signals are used on an extension board
>>>>>> which I can't find the schematic for. Also do you mind sharing your test
>>>>>> setup? Were you doing a short point to point test?
>>>>>>
>>>>>> Thank You,
>>>>>> Franklin
>>>>> Hello Franklin,
>>>>>
>>>>> your patch definitely makes sense.
>>>>>
>>>>> I forgot the TDC in my patches because it was not present in the
>>>>> previous driver versions and because I didn't encounter any
>>>>> problems when testing it myself.
>>>>>
>>>>> The error is highly dependent on the hardware (transceiver) setup.
>>>>> So it is definitely possible that some people don't encounter errors
>>>>> without your patch.
>>>>
>>>> So the Transmission Delay Compensation feature Value register is suppose
>>>> to take into consideration the transceiver delay automatically and add
>>>> the value of TDCO on top of that. So why would TDCO be dependent on the
>>>> transceiver? I've heard conflicting things regarding TDC so any
>>>> clarification on what actually impacts it would be appreciated.
>>>>
>>>> Also part of the issue I'm having is how can we properly configure TDCO?
>>>> Configuring TDCO is essentially figuring out what Secondary Sample Point
>>>> to use. However, it is unclear what value to set SSP to and which use
>>>> cases a given SSP will work or doesn't work. I've seen various
>>>> recommendations from Bosch on choosing SSP but ultimately it seems they
>>>> suggestion "real world testing" to come up with a proper value. Not
>>>> setting TDCO causes problems for my device and improperly setting TDCO
>>>> causes problems for my device. So its likely any value I use could end
>>>> up breaking something for someone else.
>>>>
>>>> Currently I leaning to a DT property that can be used for setting SSP.
>>>> Perhaps use a generic default value and allow individuals to override it
>>>> via DT?
>>>
>>> Sounds reasonable. What's the status of this series?
>>
>> I have had some offline discussions with Franklin on this, and I am not
>> fully convinced that DT is the way to go here (although I don't have the
>> agreement with Franklin there).
> 
> Probably the fundamental area where we disagree is what "default" SSP
> value should be used. Based on a short (< 1 ft) point to point test
> using a SSP of 50% worked fine. However, I'm not convinced that this
> default value of 50% will work in a more "traditional" CAN bus at higher
> speeds. Nor am I convinced that a SSP of 50% will work on every MCAN
> board in even the simplest test cases.
> 
> I believe that this default SSP should be a DT property that allows any
> board to determine what default value works best in general.

With that, I think, we are taking DT from describing board/hardware
characteristics to providing default values that software should use.

In any case, if Marc and/or Wolfgang are okay with it, binding
documentation for such a property should be sent to DT maintainers for
review.

>>
>> There are two components in configuring the secondary sample point. It
>> is the transceiver loopback delay and an offset (example half of the bit
>> time in data phase).
>>
>> While the transceiver loopback delay is pretty board dependent (and thus
>> amenable to DT encoding), I am not quite sure the offset can be
>> configured in DT because its not really board dependent.
>>
>> Unfortunately, offset calculation does not seem to be an exact science.
>> There are recommendations ranging from using 50% of bit time to making
>> it same as the sample point configured. This means users who need to
>> change the SSP due to offset variations need to change  their DT even
>> without anything changing on their board.
>>
>> Since we have a netlink socket interface to configure sample point, I
>> wonder if that should be extended to configure SSP too (or at least the
>> offset part of SSP)?
> 
> Sekhar is right that ideally the user should be able to set the SSP at
> runtime. However, my issue is that based on my experience CAN users
> expect the driver to just work the majority of times. For unique use
> cases where the driver calculated values don't work then the user should
> be able to override it. This should only be done for a very small
> percentage of CAN users. Unless you allow DT to provide a default SSP
> many users of MCAN may find that the default SSP doesn't work and must
> always use runtime overrides to get anything to work. I don't think that
> is a good user experience which is why I don't like the idea.

Fair enough. But not quite sure if CAN users expect CAN-FD to "just
work" without doing any bittiming related setup.

Thanks,
Sekhar

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 3942 bytes --]

On 10/19/2017 07:07 AM, Sekhar Nori wrote:
>>>> Sounds reasonable. What's the status of this series?
>>>
>>> I have had some offline discussions with Franklin on this, and I am not
>>> fully convinced that DT is the way to go here (although I don't have the
>>> agreement with Franklin there).
>>
>> Probably the fundamental area where we disagree is what "default" SSP
>> value should be used. Based on a short (< 1 ft) point to point test
>> using a SSP of 50% worked fine. However, I'm not convinced that this
>> default value of 50% will work in a more "traditional" CAN bus at higher
>> speeds. Nor am I convinced that a SSP of 50% will work on every MCAN
>> board in even the simplest test cases.
>>
>> I believe that this default SSP should be a DT property that allows any
>> board to determine what default value works best in general.
> 
> With that, I think, we are taking DT from describing board/hardware
> characteristics to providing default values that software should use.

If the default value is board specific and cannot be calculated in
general or from other values present in the DT, then it's from my point
of view describing the hardware.

> In any case, if Marc and/or Wolfgang are okay with it, binding
> documentation for such a property should be sent to DT maintainers for
> review.
> 
>>>
>>> There are two components in configuring the secondary sample point. It
>>> is the transceiver loopback delay and an offset (example half of the bit
>>> time in data phase).
>>>
>>> While the transceiver loopback delay is pretty board dependent (and thus
>>> amenable to DT encoding), I am not quite sure the offset can be
>>> configured in DT because its not really board dependent.
>>>
>>> Unfortunately, offset calculation does not seem to be an exact science.
>>> There are recommendations ranging from using 50% of bit time to making
>>> it same as the sample point configured. This means users who need to
>>> change the SSP due to offset variations need to change  their DT even
>>> without anything changing on their board.
>>>
>>> Since we have a netlink socket interface to configure sample point, I
>>> wonder if that should be extended to configure SSP too (or at least the
>>> offset part of SSP)?
>>
>> Sekhar is right that ideally the user should be able to set the SSP at
>> runtime. However, my issue is that based on my experience CAN users
>> expect the driver to just work the majority of times. For unique use
>> cases where the driver calculated values don't work then the user should
>> be able to override it. This should only be done for a very small
>> percentage of CAN users. Unless you allow DT to provide a default SSP
>> many users of MCAN may find that the default SSP doesn't work and must
>> always use runtime overrides to get anything to work. I don't think that
>> is a good user experience which is why I don't like the idea.
> 
> Fair enough. But not quite sure if CAN users expect CAN-FD to "just
> work" without doing any bittiming related setup.

From my point of view I'd rather buy a board from a HW vendor where
CAN-FD works, rather than a board where I have to tweak the bit-timing
for a simple CAN-FD test setup.

As far as I see for the m_can driver it's a single tuple: "bitrate > 2.5
MBit/s -> 50%". Do we need an array of tuples in general?

If good default values are transceiver and board specific, they can go
into the DT. We need a generic (this means driver agnostic) binding for
this. If this table needs to be tweaked for special purpose, then we can
add a netlink interface for this as well.

Comments?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Sekhar Nori]

On Thursday 19 October 2017 02:43 PM, Marc Kleine-Budde wrote:
> On 10/19/2017 07:07 AM, Sekhar Nori wrote:
>>>>> Sounds reasonable. What's the status of this series?
>>>>
>>>> I have had some offline discussions with Franklin on this, and I am not
>>>> fully convinced that DT is the way to go here (although I don't have the
>>>> agreement with Franklin there).
>>>
>>> Probably the fundamental area where we disagree is what "default" SSP
>>> value should be used. Based on a short (< 1 ft) point to point test
>>> using a SSP of 50% worked fine. However, I'm not convinced that this
>>> default value of 50% will work in a more "traditional" CAN bus at higher
>>> speeds. Nor am I convinced that a SSP of 50% will work on every MCAN
>>> board in even the simplest test cases.
>>>
>>> I believe that this default SSP should be a DT property that allows any
>>> board to determine what default value works best in general.
>>
>> With that, I think, we are taking DT from describing board/hardware
>> characteristics to providing default values that software should use.
> 
> If the default value is board specific and cannot be calculated in
> general or from other values present in the DT, then it's from my point
> of view describing the hardware.
> 
>> In any case, if Marc and/or Wolfgang are okay with it, binding
>> documentation for such a property should be sent to DT maintainers for
>> review.
>>
>>>>
>>>> There are two components in configuring the secondary sample point. It
>>>> is the transceiver loopback delay and an offset (example half of the bit
>>>> time in data phase).
>>>>
>>>> While the transceiver loopback delay is pretty board dependent (and thus
>>>> amenable to DT encoding), I am not quite sure the offset can be
>>>> configured in DT because its not really board dependent.
>>>>
>>>> Unfortunately, offset calculation does not seem to be an exact science.
>>>> There are recommendations ranging from using 50% of bit time to making
>>>> it same as the sample point configured. This means users who need to
>>>> change the SSP due to offset variations need to change  their DT even
>>>> without anything changing on their board.
>>>>
>>>> Since we have a netlink socket interface to configure sample point, I
>>>> wonder if that should be extended to configure SSP too (or at least the
>>>> offset part of SSP)?
>>>
>>> Sekhar is right that ideally the user should be able to set the SSP at
>>> runtime. However, my issue is that based on my experience CAN users
>>> expect the driver to just work the majority of times. For unique use
>>> cases where the driver calculated values don't work then the user should
>>> be able to override it. This should only be done for a very small
>>> percentage of CAN users. Unless you allow DT to provide a default SSP
>>> many users of MCAN may find that the default SSP doesn't work and must
>>> always use runtime overrides to get anything to work. I don't think that
>>> is a good user experience which is why I don't like the idea.
>>
>> Fair enough. But not quite sure if CAN users expect CAN-FD to "just
>> work" without doing any bittiming related setup.
> 
> From my point of view I'd rather buy a board from a HW vendor where
> CAN-FD works, rather than a board where I have to tweak the bit-timing
> for a simple CAN-FD test setup.
> 
> As far as I see for the m_can driver it's a single tuple: "bitrate > 2.5
> MBit/s -> 50%". Do we need an array of tuples in general?
> 
> If good default values are transceiver and board specific, they can go
> into the DT. We need a generic (this means driver agnostic) binding for
> this. If this table needs to be tweaked for special purpose, then we can
> add a netlink interface for this as well.
> 
> Comments?

I dont know how a good default (other than 50% as the starting point)
can be arrived at without doing any actual measurements on the actual
network. Since we do know that the value has to be tweaked, agree that
netlink interface has to be provided.

I wonder whether even if a DT binding for default is provided, everyone
will end up setting it to 50% (since there is no way for them to predict
any better). In effect, I am suggesting using a hardcoded value of 50%
instead of introducing a binding without a clear need for it.

Note that I am only talking about there "offset" part of SSP here. The
transceiver loopback delay is calculated automatically by Bosch's MCAN
IP. But if there are other IPs which don't do that, then yes, that
should be a DT property IMO.

Thanks,
Sekhar

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 5205 bytes --]

On 10/19/2017 01:09 PM, Sekhar Nori wrote:
> On Thursday 19 October 2017 02:43 PM, Marc Kleine-Budde wrote:
>> On 10/19/2017 07:07 AM, Sekhar Nori wrote:
>>>>>> Sounds reasonable. What's the status of this series?
>>>>>
>>>>> I have had some offline discussions with Franklin on this, and I am not
>>>>> fully convinced that DT is the way to go here (although I don't have the
>>>>> agreement with Franklin there).
>>>>
>>>> Probably the fundamental area where we disagree is what "default" SSP
>>>> value should be used. Based on a short (< 1 ft) point to point test
>>>> using a SSP of 50% worked fine. However, I'm not convinced that this
>>>> default value of 50% will work in a more "traditional" CAN bus at higher
>>>> speeds. Nor am I convinced that a SSP of 50% will work on every MCAN
>>>> board in even the simplest test cases.
>>>>
>>>> I believe that this default SSP should be a DT property that allows any
>>>> board to determine what default value works best in general.
>>>
>>> With that, I think, we are taking DT from describing board/hardware
>>> characteristics to providing default values that software should use.
>>
>> If the default value is board specific and cannot be calculated in
>> general or from other values present in the DT, then it's from my point
>> of view describing the hardware.
>>
>>> In any case, if Marc and/or Wolfgang are okay with it, binding
>>> documentation for such a property should be sent to DT maintainers for
>>> review.
>>>
>>>>>
>>>>> There are two components in configuring the secondary sample point. It
>>>>> is the transceiver loopback delay and an offset (example half of the bit
>>>>> time in data phase).
>>>>>
>>>>> While the transceiver loopback delay is pretty board dependent (and thus
>>>>> amenable to DT encoding), I am not quite sure the offset can be
>>>>> configured in DT because its not really board dependent.
>>>>>
>>>>> Unfortunately, offset calculation does not seem to be an exact science.
>>>>> There are recommendations ranging from using 50% of bit time to making
>>>>> it same as the sample point configured. This means users who need to
>>>>> change the SSP due to offset variations need to change  their DT even
>>>>> without anything changing on their board.
>>>>>
>>>>> Since we have a netlink socket interface to configure sample point, I
>>>>> wonder if that should be extended to configure SSP too (or at least the
>>>>> offset part of SSP)?
>>>>
>>>> Sekhar is right that ideally the user should be able to set the SSP at
>>>> runtime. However, my issue is that based on my experience CAN users
>>>> expect the driver to just work the majority of times. For unique use
>>>> cases where the driver calculated values don't work then the user should
>>>> be able to override it. This should only be done for a very small
>>>> percentage of CAN users. Unless you allow DT to provide a default SSP
>>>> many users of MCAN may find that the default SSP doesn't work and must
>>>> always use runtime overrides to get anything to work. I don't think that
>>>> is a good user experience which is why I don't like the idea.
>>>
>>> Fair enough. But not quite sure if CAN users expect CAN-FD to "just
>>> work" without doing any bittiming related setup.
>>
>> From my point of view I'd rather buy a board from a HW vendor where
>> CAN-FD works, rather than a board where I have to tweak the bit-timing
>> for a simple CAN-FD test setup.
>>
>> As far as I see for the m_can driver it's a single tuple: "bitrate > 2.5
>> MBit/s -> 50%". Do we need an array of tuples in general?

Do we need more than one tuple here?

>> If good default values are transceiver and board specific, they can go
>> into the DT. We need a generic (this means driver agnostic) binding for
>> this. If this table needs to be tweaked for special purpose, then we can
>> add a netlink interface for this as well.
>>
>> Comments?
> 
> I dont know how a good default (other than 50% as the starting point)
> can be arrived at without doing any actual measurements on the actual
> network. Since we do know that the value has to be tweaked, agree that
> netlink interface has to be provided.
> 
> I wonder whether even if a DT binding for default is provided, everyone
> will end up setting it to 50% (since there is no way for them to predict
> any better). In effect, I am suggesting using a hardcoded value of 50%
> instead of introducing a binding without a clear need for it.

Ok, if the value is network and not board specific it doesn't belong
into the DT.

> Note that I am only talking about there "offset" part of SSP here. The
> transceiver loopback delay is calculated automatically by Bosch's MCAN
> IP. But if there are other IPs which don't do that, then yes, that
> should be a DT property IMO.

Ok, but let's wait for such an IP core to show up here :)

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Franklin S Cooper Jr]

On 10/19/2017 06:32 AM, Marc Kleine-Budde wrote:
> On 10/19/2017 01:09 PM, Sekhar Nori wrote:
>> On Thursday 19 October 2017 02:43 PM, Marc Kleine-Budde wrote:
>>> On 10/19/2017 07:07 AM, Sekhar Nori wrote:
>>>>>>> Sounds reasonable. What's the status of this series?
>>>>>>
>>>>>> I have had some offline discussions with Franklin on this, and I am not
>>>>>> fully convinced that DT is the way to go here (although I don't have the
>>>>>> agreement with Franklin there).
>>>>>
>>>>> Probably the fundamental area where we disagree is what "default" SSP
>>>>> value should be used. Based on a short (< 1 ft) point to point test
>>>>> using a SSP of 50% worked fine. However, I'm not convinced that this
>>>>> default value of 50% will work in a more "traditional" CAN bus at higher
>>>>> speeds. Nor am I convinced that a SSP of 50% will work on every MCAN
>>>>> board in even the simplest test cases.
>>>>>
>>>>> I believe that this default SSP should be a DT property that allows any
>>>>> board to determine what default value works best in general.
>>>>
>>>> With that, I think, we are taking DT from describing board/hardware
>>>> characteristics to providing default values that software should use.
>>>
>>> If the default value is board specific and cannot be calculated in
>>> general or from other values present in the DT, then it's from my point
>>> of view describing the hardware.
>>>
>>>> In any case, if Marc and/or Wolfgang are okay with it, binding
>>>> documentation for such a property should be sent to DT maintainers for
>>>> review.
>>>>
>>>>>>
>>>>>> There are two components in configuring the secondary sample point. It
>>>>>> is the transceiver loopback delay and an offset (example half of the bit
>>>>>> time in data phase).
>>>>>>
>>>>>> While the transceiver loopback delay is pretty board dependent (and thus
>>>>>> amenable to DT encoding), I am not quite sure the offset can be
>>>>>> configured in DT because its not really board dependent.
>>>>>>
>>>>>> Unfortunately, offset calculation does not seem to be an exact science.
>>>>>> There are recommendations ranging from using 50% of bit time to making
>>>>>> it same as the sample point configured. This means users who need to
>>>>>> change the SSP due to offset variations need to change  their DT even
>>>>>> without anything changing on their board.
>>>>>>
>>>>>> Since we have a netlink socket interface to configure sample point, I
>>>>>> wonder if that should be extended to configure SSP too (or at least the
>>>>>> offset part of SSP)?
>>>>>
>>>>> Sekhar is right that ideally the user should be able to set the SSP at
>>>>> runtime. However, my issue is that based on my experience CAN users
>>>>> expect the driver to just work the majority of times. For unique use
>>>>> cases where the driver calculated values don't work then the user should
>>>>> be able to override it. This should only be done for a very small
>>>>> percentage of CAN users. Unless you allow DT to provide a default SSP
>>>>> many users of MCAN may find that the default SSP doesn't work and must
>>>>> always use runtime overrides to get anything to work. I don't think that
>>>>> is a good user experience which is why I don't like the idea.
>>>>
>>>> Fair enough. But not quite sure if CAN users expect CAN-FD to "just
>>>> work" without doing any bittiming related setup.
>>>
>>> From my point of view I'd rather buy a board from a HW vendor where
>>> CAN-FD works, rather than a board where I have to tweak the bit-timing
>>> for a simple CAN-FD test setup.
>>>
>>> As far as I see for the m_can driver it's a single tuple: "bitrate > 2.5
>>> MBit/s -> 50%". Do we need an array of tuples in general?

Internally what I proposed was a binding that allowed you to pass in an
array of a range of baud rates and then a SSP for that baud rate range.
Therefore, if the baud rate being used impacted what SSP worked then it
allows someone to provide a range of defaults. Of course a person also
has the ability to use a single large range thus implementing a single
default SSP value.

> 
> Do we need more than one tuple here?
> 
>>> If good default values are transceiver and board specific, they can go
>>> into the DT. We need a generic (this means driver agnostic) binding for
>>> this. If this table needs to be tweaked for special purpose, then we can
>>> add a netlink interface for this as well.
>>>
>>> Comments?
>>
>> I dont know how a good default (other than 50% as the starting point)
>> can be arrived at without doing any actual measurements on the actual
>> network. Since we do know that the value has to be tweaked, agree that
>> netlink interface has to be provided.

Now I have seen in non public documentations that setting SP to SSP also
works. This makes a bit more sense to me and I'm alot more comfortable
going with this. However, since its based on non public information I
can't justify it beyond that "it works for me". But I'm alot more
comfortable going with then saying "hey this default value works for
TI's dra76 evm. Therefore, every MCAN board will be stuck by default for
a value that works for us". So if there is no push back with going with
SSP = db SP with no documentation to back up why that is being used then
I will try that out and send patches.

>>
>> I wonder whether even if a DT binding for default is provided, everyone
>> will end up setting it to 50% (since there is no way for them to predict
>> any better). In effect, I am suggesting using a hardcoded value of 50%
>> instead of introducing a binding without a clear need for it.

The big assumption here is that 50% works for everyone. Its not about
predicting its about atleast based on your testing do you have a value
that works. Our evm is used for evaluation purposes we expect people to
generally do a bit more simplified testing and thus 50% may be fine.
Another board that is meant for an automobile that wants the fastest
rate possible may feel like a default value of 70% is better.
> 
> Ok, if the value is network and not board specific it doesn't belong
> into the DT.

I believe its a bit of both but honestly its not clear. Wenyou whose
board also includes the MCAN ip performed a similar point to point test
and found that it worked without the TDC feature. When I performed the
test without TDC the ip threw an error and went into initialization
mode. So likely the network has an impact (not exactly sure how much),
baud rate may have an impact (again not sure how much it does) but for
sure the board does as well.


> 
>> Note that I am only talking about there "offset" part of SSP here. The
>> transceiver loopback delay is calculated automatically by Bosch's MCAN
>> IP. But if there are other IPs which don't do that, then yes, that
>> should be a DT property IMO.
> 
> Ok, but let's wait for such an IP core to show up here :)
> 
> Marc
> 

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 6602 bytes --]

On 10/19/2017 03:54 PM, Franklin S Cooper Jr wrote:
> On 10/19/2017 06:32 AM, Marc Kleine-Budde wrote:
>> On 10/19/2017 01:09 PM, Sekhar Nori wrote:
>>> On Thursday 19 October 2017 02:43 PM, Marc Kleine-Budde wrote:
>>>> On 10/19/2017 07:07 AM, Sekhar Nori wrote:
>>>>>>>> Sounds reasonable. What's the status of this series?
>>>>>>>
>>>>>>> I have had some offline discussions with Franklin on this, and I am not
>>>>>>> fully convinced that DT is the way to go here (although I don't have the
>>>>>>> agreement with Franklin there).
>>>>>>
>>>>>> Probably the fundamental area where we disagree is what "default" SSP
>>>>>> value should be used. Based on a short (< 1 ft) point to point test
>>>>>> using a SSP of 50% worked fine. However, I'm not convinced that this
>>>>>> default value of 50% will work in a more "traditional" CAN bus at higher
>>>>>> speeds. Nor am I convinced that a SSP of 50% will work on every MCAN
>>>>>> board in even the simplest test cases.
>>>>>>
>>>>>> I believe that this default SSP should be a DT property that allows any
>>>>>> board to determine what default value works best in general.
>>>>>
>>>>> With that, I think, we are taking DT from describing board/hardware
>>>>> characteristics to providing default values that software should use.
>>>>
>>>> If the default value is board specific and cannot be calculated in
>>>> general or from other values present in the DT, then it's from my point
>>>> of view describing the hardware.
>>>>
>>>>> In any case, if Marc and/or Wolfgang are okay with it, binding
>>>>> documentation for such a property should be sent to DT maintainers for
>>>>> review.
>>>>>
>>>>>>>
>>>>>>> There are two components in configuring the secondary sample point. It
>>>>>>> is the transceiver loopback delay and an offset (example half of the bit
>>>>>>> time in data phase).
>>>>>>>
>>>>>>> While the transceiver loopback delay is pretty board dependent (and thus
>>>>>>> amenable to DT encoding), I am not quite sure the offset can be
>>>>>>> configured in DT because its not really board dependent.
>>>>>>>
>>>>>>> Unfortunately, offset calculation does not seem to be an exact science.
>>>>>>> There are recommendations ranging from using 50% of bit time to making
>>>>>>> it same as the sample point configured. This means users who need to
>>>>>>> change the SSP due to offset variations need to change  their DT even
>>>>>>> without anything changing on their board.
>>>>>>>
>>>>>>> Since we have a netlink socket interface to configure sample point, I
>>>>>>> wonder if that should be extended to configure SSP too (or at least the
>>>>>>> offset part of SSP)?
>>>>>>
>>>>>> Sekhar is right that ideally the user should be able to set the SSP at
>>>>>> runtime. However, my issue is that based on my experience CAN users
>>>>>> expect the driver to just work the majority of times. For unique use
>>>>>> cases where the driver calculated values don't work then the user should
>>>>>> be able to override it. This should only be done for a very small
>>>>>> percentage of CAN users. Unless you allow DT to provide a default SSP
>>>>>> many users of MCAN may find that the default SSP doesn't work and must
>>>>>> always use runtime overrides to get anything to work. I don't think that
>>>>>> is a good user experience which is why I don't like the idea.
>>>>>
>>>>> Fair enough. But not quite sure if CAN users expect CAN-FD to "just
>>>>> work" without doing any bittiming related setup.
>>>>
>>>> From my point of view I'd rather buy a board from a HW vendor where
>>>> CAN-FD works, rather than a board where I have to tweak the bit-timing
>>>> for a simple CAN-FD test setup.
>>>>
>>>> As far as I see for the m_can driver it's a single tuple: "bitrate > 2.5
>>>> MBit/s -> 50%". Do we need an array of tuples in general?
> 
> Internally what I proposed was a binding that allowed you to pass in an
> array of a range of baud rates and then a SSP for that baud rate range.
> Therefore, if the baud rate being used impacted what SSP worked then it
> allows someone to provide a range of defaults. Of course a person also
> has the ability to use a single large range thus implementing a single
> default SSP value.

A single tuple is just a special case of a list of tuples :)

I was thinking of something like this:

First we need a struct defining the bitrate spp relationship.

The driver provides default values by a sorted array of these structs
together with array length.

During device registration we assign these default values to the actual
driver instance.

The netlink code can read and overwrite the current values. Maybe it can
read the default values.

The bitrate calculation code calculates the to be used spp value.

The driver sets the value during the open callback.

>> Do we need more than one tuple here?
>>
>>>> If good default values are transceiver and board specific, they can go
>>>> into the DT. We need a generic (this means driver agnostic) binding for
>>>> this. If this table needs to be tweaked for special purpose, then we can
>>>> add a netlink interface for this as well.
>>>>
>>>> Comments?
>>>
>>> I dont know how a good default (other than 50% as the starting point)
>>> can be arrived at without doing any actual measurements on the actual
>>> network. Since we do know that the value has to be tweaked, agree that
>>> netlink interface has to be provided.
> 
> Now I have seen in non public documentations that setting SP to SSP also
> works.

This can already by done now, without the need for new interfaces.

> This makes a bit more sense to me and I'm alot more comfortable going
> with this. However, since its based on non public information I can't
> justify it beyond that "it works for me". But I'm alot more 
> comfortable going with then saying "hey this default value works for 
> TI's dra76 evm. Therefore, every MCAN board will be stuck by default
> for a value that works for us". So if there is no push back with
> going with SSP = db SP with no documentation to back up why that is
> being used then I will try that out and send patches.

This means we postpone the whole add-new-interface-dance until the
SPP=SP approach doesn't work for some usecase?

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Franklin S Cooper Jr]

On 10/19/2017 09:55 AM, Marc Kleine-Budde wrote:
> On 10/19/2017 03:54 PM, Franklin S Cooper Jr wrote:
>> On 10/19/2017 06:32 AM, Marc Kleine-Budde wrote:
>>> On 10/19/2017 01:09 PM, Sekhar Nori wrote:
>>>> On Thursday 19 October 2017 02:43 PM, Marc Kleine-Budde wrote:
>>>>> On 10/19/2017 07:07 AM, Sekhar Nori wrote:
>>>>>>>>> Sounds reasonable. What's the status of this series?
>>>>>>>>
>>>>>>>> I have had some offline discussions with Franklin on this, and I am not
>>>>>>>> fully convinced that DT is the way to go here (although I don't have the
>>>>>>>> agreement with Franklin there).
>>>>>>>
>>>>>>> Probably the fundamental area where we disagree is what "default" SSP
>>>>>>> value should be used. Based on a short (< 1 ft) point to point test
>>>>>>> using a SSP of 50% worked fine. However, I'm not convinced that this
>>>>>>> default value of 50% will work in a more "traditional" CAN bus at higher
>>>>>>> speeds. Nor am I convinced that a SSP of 50% will work on every MCAN
>>>>>>> board in even the simplest test cases.
>>>>>>>
>>>>>>> I believe that this default SSP should be a DT property that allows any
>>>>>>> board to determine what default value works best in general.
>>>>>>
>>>>>> With that, I think, we are taking DT from describing board/hardware
>>>>>> characteristics to providing default values that software should use.
>>>>>
>>>>> If the default value is board specific and cannot be calculated in
>>>>> general or from other values present in the DT, then it's from my point
>>>>> of view describing the hardware.
>>>>>
>>>>>> In any case, if Marc and/or Wolfgang are okay with it, binding
>>>>>> documentation for such a property should be sent to DT maintainers for
>>>>>> review.
>>>>>>
>>>>>>>>
>>>>>>>> There are two components in configuring the secondary sample point. It
>>>>>>>> is the transceiver loopback delay and an offset (example half of the bit
>>>>>>>> time in data phase).
>>>>>>>>
>>>>>>>> While the transceiver loopback delay is pretty board dependent (and thus
>>>>>>>> amenable to DT encoding), I am not quite sure the offset can be
>>>>>>>> configured in DT because its not really board dependent.
>>>>>>>>
>>>>>>>> Unfortunately, offset calculation does not seem to be an exact science.
>>>>>>>> There are recommendations ranging from using 50% of bit time to making
>>>>>>>> it same as the sample point configured. This means users who need to
>>>>>>>> change the SSP due to offset variations need to change  their DT even
>>>>>>>> without anything changing on their board.
>>>>>>>>
>>>>>>>> Since we have a netlink socket interface to configure sample point, I
>>>>>>>> wonder if that should be extended to configure SSP too (or at least the
>>>>>>>> offset part of SSP)?
>>>>>>>
>>>>>>> Sekhar is right that ideally the user should be able to set the SSP at
>>>>>>> runtime. However, my issue is that based on my experience CAN users
>>>>>>> expect the driver to just work the majority of times. For unique use
>>>>>>> cases where the driver calculated values don't work then the user should
>>>>>>> be able to override it. This should only be done for a very small
>>>>>>> percentage of CAN users. Unless you allow DT to provide a default SSP
>>>>>>> many users of MCAN may find that the default SSP doesn't work and must
>>>>>>> always use runtime overrides to get anything to work. I don't think that
>>>>>>> is a good user experience which is why I don't like the idea.
>>>>>>
>>>>>> Fair enough. But not quite sure if CAN users expect CAN-FD to "just
>>>>>> work" without doing any bittiming related setup.
>>>>>
>>>>> From my point of view I'd rather buy a board from a HW vendor where
>>>>> CAN-FD works, rather than a board where I have to tweak the bit-timing
>>>>> for a simple CAN-FD test setup.
>>>>>
>>>>> As far as I see for the m_can driver it's a single tuple: "bitrate > 2.5
>>>>> MBit/s -> 50%". Do we need an array of tuples in general?
>>
>> Internally what I proposed was a binding that allowed you to pass in an
>> array of a range of baud rates and then a SSP for that baud rate range.
>> Therefore, if the baud rate being used impacted what SSP worked then it
>> allows someone to provide a range of defaults. Of course a person also
>> has the ability to use a single large range thus implementing a single
>> default SSP value.
> 
> A single tuple is just a special case of a list of tuples :)
> 
> I was thinking of something like this:
> 
> First we need a struct defining the bitrate spp relationship.
> 
> The driver provides default values by a sorted array of these structs
> together with array length.
> 
> During device registration we assign these default values to the actual
> driver instance.
> 
> The netlink code can read and overwrite the current values. Maybe it can
> read the default values.
> 
> The bitrate calculation code calculates the to be used spp value.
> 
> The driver sets the value during the open callback.
> 
>>> Do we need more than one tuple here?
>>>
>>>>> If good default values are transceiver and board specific, they can go
>>>>> into the DT. We need a generic (this means driver agnostic) binding for
>>>>> this. If this table needs to be tweaked for special purpose, then we can
>>>>> add a netlink interface for this as well.
>>>>>
>>>>> Comments?
>>>>
>>>> I dont know how a good default (other than 50% as the starting point)
>>>> can be arrived at without doing any actual measurements on the actual
>>>> network. Since we do know that the value has to be tweaked, agree that
>>>> netlink interface has to be provided.
>>
>> Now I have seen in non public documentations that setting SP to SSP also
>> works.
> 
> This can already by done now, without the need for new interfaces.
> 
>> This makes a bit more sense to me and I'm alot more comfortable going
>> with this. However, since its based on non public information I can't
>> justify it beyond that "it works for me". But I'm alot more 
>> comfortable going with then saying "hey this default value works for 
>> TI's dra76 evm. Therefore, every MCAN board will be stuck by default
>> for a value that works for us". So if there is no push back with
>> going with SSP = db SP with no documentation to back up why that is
>> being used then I will try that out and send patches.
> 
> This means we postpone the whole add-new-interface-dance until the
> SPP=SP approach doesn't work for some usecase?

My justification behind using SSP = SP will be "it works during my
test". If that is fine then I am ok with postponing any new bindings.
> 
> Marc
> 

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Sekhar Nori]

On Thursday 19 October 2017 08:25 PM, Marc Kleine-Budde wrote:
> On 10/19/2017 03:54 PM, Franklin S Cooper Jr wrote:
>> On 10/19/2017 06:32 AM, Marc Kleine-Budde wrote:
>>> On 10/19/2017 01:09 PM, Sekhar Nori wrote:
>>>> On Thursday 19 October 2017 02:43 PM, Marc Kleine-Budde wrote:
>>>>> On 10/19/2017 07:07 AM, Sekhar Nori wrote:
>>>>>>>>> Sounds reasonable. What's the status of this series?
>>>>>>>>
>>>>>>>> I have had some offline discussions with Franklin on this, and I am not
>>>>>>>> fully convinced that DT is the way to go here (although I don't have the
>>>>>>>> agreement with Franklin there).
>>>>>>>
>>>>>>> Probably the fundamental area where we disagree is what "default" SSP
>>>>>>> value should be used. Based on a short (< 1 ft) point to point test
>>>>>>> using a SSP of 50% worked fine. However, I'm not convinced that this
>>>>>>> default value of 50% will work in a more "traditional" CAN bus at higher
>>>>>>> speeds. Nor am I convinced that a SSP of 50% will work on every MCAN
>>>>>>> board in even the simplest test cases.
>>>>>>>
>>>>>>> I believe that this default SSP should be a DT property that allows any
>>>>>>> board to determine what default value works best in general.
>>>>>>
>>>>>> With that, I think, we are taking DT from describing board/hardware
>>>>>> characteristics to providing default values that software should use.
>>>>>
>>>>> If the default value is board specific and cannot be calculated in
>>>>> general or from other values present in the DT, then it's from my point
>>>>> of view describing the hardware.
>>>>>
>>>>>> In any case, if Marc and/or Wolfgang are okay with it, binding
>>>>>> documentation for such a property should be sent to DT maintainers for
>>>>>> review.
>>>>>>
>>>>>>>>
>>>>>>>> There are two components in configuring the secondary sample point. It
>>>>>>>> is the transceiver loopback delay and an offset (example half of the bit
>>>>>>>> time in data phase).
>>>>>>>>
>>>>>>>> While the transceiver loopback delay is pretty board dependent (and thus
>>>>>>>> amenable to DT encoding), I am not quite sure the offset can be
>>>>>>>> configured in DT because its not really board dependent.
>>>>>>>>
>>>>>>>> Unfortunately, offset calculation does not seem to be an exact science.
>>>>>>>> There are recommendations ranging from using 50% of bit time to making
>>>>>>>> it same as the sample point configured. This means users who need to
>>>>>>>> change the SSP due to offset variations need to change  their DT even
>>>>>>>> without anything changing on their board.
>>>>>>>>
>>>>>>>> Since we have a netlink socket interface to configure sample point, I
>>>>>>>> wonder if that should be extended to configure SSP too (or at least the
>>>>>>>> offset part of SSP)?
>>>>>>>
>>>>>>> Sekhar is right that ideally the user should be able to set the SSP at
>>>>>>> runtime. However, my issue is that based on my experience CAN users
>>>>>>> expect the driver to just work the majority of times. For unique use
>>>>>>> cases where the driver calculated values don't work then the user should
>>>>>>> be able to override it. This should only be done for a very small
>>>>>>> percentage of CAN users. Unless you allow DT to provide a default SSP
>>>>>>> many users of MCAN may find that the default SSP doesn't work and must
>>>>>>> always use runtime overrides to get anything to work. I don't think that
>>>>>>> is a good user experience which is why I don't like the idea.
>>>>>>
>>>>>> Fair enough. But not quite sure if CAN users expect CAN-FD to "just
>>>>>> work" without doing any bittiming related setup.
>>>>>
>>>>> From my point of view I'd rather buy a board from a HW vendor where
>>>>> CAN-FD works, rather than a board where I have to tweak the bit-timing
>>>>> for a simple CAN-FD test setup.
>>>>>
>>>>> As far as I see for the m_can driver it's a single tuple: "bitrate > 2.5
>>>>> MBit/s -> 50%". Do we need an array of tuples in general?
>>
>> Internally what I proposed was a binding that allowed you to pass in an
>> array of a range of baud rates and then a SSP for that baud rate range.
>> Therefore, if the baud rate being used impacted what SSP worked then it
>> allows someone to provide a range of defaults. Of course a person also
>> has the ability to use a single large range thus implementing a single
>> default SSP value.
> 
> A single tuple is just a special case of a list of tuples :)
> 
> I was thinking of something like this:
> 
> First we need a struct defining the bitrate spp relationship.

I dont know if there is such a relation between bitrate and SSP. We do
know that some papers[1] suggest that below 2.5MBits/sec TDC (and hence
SSP) feature is not required. And we also know that hardly anyone has
tested CAN-FD above 5MBits/sec.

Between these two values, we do not have enough data today to suggest
that SSP (as a percentage of bittiming) needs to move as bitrate varies.

> The driver provides default values by a sorted array of these structs
> together with array length.
> 
> During device registration we assign these default values to the actual
> driver instance.
> 
> The netlink code can read and overwrite the current values. Maybe it can
> read the default values.
> 
> The bitrate calculation code calculates the to be used spp value.
> 
> The driver sets the value during the open callback.
> 
>>> Do we need more than one tuple here?
>>>
>>>>> If good default values are transceiver and board specific, they can go
>>>>> into the DT. We need a generic (this means driver agnostic) binding for
>>>>> this. If this table needs to be tweaked for special purpose, then we can
>>>>> add a netlink interface for this as well.
>>>>>
>>>>> Comments?
>>>>
>>>> I dont know how a good default (other than 50% as the starting point)
>>>> can be arrived at without doing any actual measurements on the actual
>>>> network. Since we do know that the value has to be tweaked, agree that
>>>> netlink interface has to be provided.
>>
>> Now I have seen in non public documentations that setting SP to SSP also
>> works.
> 
> This can already by done now, without the need for new interfaces.

If I read correctly, this is what Oliver Hartkopp is saying too.

>> This makes a bit more sense to me and I'm alot more comfortable going
>> with this. However, since its based on non public information I can't
>> justify it beyond that "it works for me". But I'm alot more 
>> comfortable going with then saying "hey this default value works for 
>> TI's dra76 evm. Therefore, every MCAN board will be stuck by default
>> for a value that works for us". So if there is no push back with
>> going with SSP = db SP with no documentation to back up why that is
>> being used then I will try that out and send patches.
> 
> This means we postpone the whole add-new-interface-dance until the
> SPP=SP approach doesn't work for some usecase?

I think thats prudent as any new interface (DT or userspace) needs to be
maintained forever. Over time we should have more data from actual
deployments to see if a new interface is really necessary.

Thanks,
Sekhar

[1]
http://www.bosch-semiconductors.de/media/automotive_electronics/pdf_2/ipmodules_3/can_fd_1/icc14_2013_paper_Hartwich.pdf
(last page)

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Sekhar Nori]

On Thursday 19 October 2017 08:25 PM, Marc Kleine-Budde wrote:
> On 10/19/2017 03:54 PM, Franklin S Cooper Jr wrote:
>> On 10/19/2017 06:32 AM, Marc Kleine-Budde wrote:
>>> On 10/19/2017 01:09 PM, Sekhar Nori wrote:
>>>> On Thursday 19 October 2017 02:43 PM, Marc Kleine-Budde wrote:
>>>>> On 10/19/2017 07:07 AM, Sekhar Nori wrote:
>>>>>>>>> Sounds reasonable. What's the status of this series?
>>>>>>>>
>>>>>>>> I have had some offline discussions with Franklin on this, and I am not
>>>>>>>> fully convinced that DT is the way to go here (although I don't have the
>>>>>>>> agreement with Franklin there).
>>>>>>>
>>>>>>> Probably the fundamental area where we disagree is what "default" SSP
>>>>>>> value should be used. Based on a short (< 1 ft) point to point test
>>>>>>> using a SSP of 50% worked fine. However, I'm not convinced that this
>>>>>>> default value of 50% will work in a more "traditional" CAN bus at higher
>>>>>>> speeds. Nor am I convinced that a SSP of 50% will work on every MCAN
>>>>>>> board in even the simplest test cases.
>>>>>>>
>>>>>>> I believe that this default SSP should be a DT property that allows any
>>>>>>> board to determine what default value works best in general.
>>>>>>
>>>>>> With that, I think, we are taking DT from describing board/hardware
>>>>>> characteristics to providing default values that software should use.
>>>>>
>>>>> If the default value is board specific and cannot be calculated in
>>>>> general or from other values present in the DT, then it's from my point
>>>>> of view describing the hardware.
>>>>>
>>>>>> In any case, if Marc and/or Wolfgang are okay with it, binding
>>>>>> documentation for such a property should be sent to DT maintainers for
>>>>>> review.
>>>>>>
>>>>>>>>
>>>>>>>> There are two components in configuring the secondary sample point. It
>>>>>>>> is the transceiver loopback delay and an offset (example half of the bit
>>>>>>>> time in data phase).
>>>>>>>>
>>>>>>>> While the transceiver loopback delay is pretty board dependent (and thus
>>>>>>>> amenable to DT encoding), I am not quite sure the offset can be
>>>>>>>> configured in DT because its not really board dependent.
>>>>>>>>
>>>>>>>> Unfortunately, offset calculation does not seem to be an exact science.
>>>>>>>> There are recommendations ranging from using 50% of bit time to making
>>>>>>>> it same as the sample point configured. This means users who need to
>>>>>>>> change the SSP due to offset variations need to change  their DT even
>>>>>>>> without anything changing on their board.
>>>>>>>>
>>>>>>>> Since we have a netlink socket interface to configure sample point, I
>>>>>>>> wonder if that should be extended to configure SSP too (or at least the
>>>>>>>> offset part of SSP)?
>>>>>>>
>>>>>>> Sekhar is right that ideally the user should be able to set the SSP at
>>>>>>> runtime. However, my issue is that based on my experience CAN users
>>>>>>> expect the driver to just work the majority of times. For unique use
>>>>>>> cases where the driver calculated values don't work then the user should
>>>>>>> be able to override it. This should only be done for a very small
>>>>>>> percentage of CAN users. Unless you allow DT to provide a default SSP
>>>>>>> many users of MCAN may find that the default SSP doesn't work and must
>>>>>>> always use runtime overrides to get anything to work. I don't think that
>>>>>>> is a good user experience which is why I don't like the idea.
>>>>>>
>>>>>> Fair enough. But not quite sure if CAN users expect CAN-FD to "just
>>>>>> work" without doing any bittiming related setup.
>>>>>
>>>>> From my point of view I'd rather buy a board from a HW vendor where
>>>>> CAN-FD works, rather than a board where I have to tweak the bit-timing
>>>>> for a simple CAN-FD test setup.
>>>>>
>>>>> As far as I see for the m_can driver it's a single tuple: "bitrate > 2.5
>>>>> MBit/s -> 50%". Do we need an array of tuples in general?
>>
>> Internally what I proposed was a binding that allowed you to pass in an
>> array of a range of baud rates and then a SSP for that baud rate range.
>> Therefore, if the baud rate being used impacted what SSP worked then it
>> allows someone to provide a range of defaults. Of course a person also
>> has the ability to use a single large range thus implementing a single
>> default SSP value.
> 
> A single tuple is just a special case of a list of tuples :)
> 
> I was thinking of something like this:
> 
> First we need a struct defining the bitrate spp relationship.

I dont know if there is such a relation between bitrate and SSP. We do
know that some papers[1] suggest that below 2.5MBits/sec TDC (and hence
SSP) feature is not required. And we also know that hardly anyone has
tested CAN-FD above 5MBits/sec.

Between these two values, we do not have enough data today to suggest
that SSP (as a percentage of bittiming) needs to move as bitrate varies.

> The driver provides default values by a sorted array of these structs
> together with array length.
> 
> During device registration we assign these default values to the actual
> driver instance.
> 
> The netlink code can read and overwrite the current values. Maybe it can
> read the default values.
> 
> The bitrate calculation code calculates the to be used spp value.
> 
> The driver sets the value during the open callback.
> 
>>> Do we need more than one tuple here?
>>>
>>>>> If good default values are transceiver and board specific, they can go
>>>>> into the DT. We need a generic (this means driver agnostic) binding for
>>>>> this. If this table needs to be tweaked for special purpose, then we can
>>>>> add a netlink interface for this as well.
>>>>>
>>>>> Comments?
>>>>
>>>> I dont know how a good default (other than 50% as the starting point)
>>>> can be arrived at without doing any actual measurements on the actual
>>>> network. Since we do know that the value has to be tweaked, agree that
>>>> netlink interface has to be provided.
>>
>> Now I have seen in non public documentations that setting SP to SSP also
>> works.
> 
> This can already by done now, without the need for new interfaces.

If I read correctly, this is what Oliver Hartkopp is saying too.

>> This makes a bit more sense to me and I'm alot more comfortable going
>> with this. However, since its based on non public information I can't
>> justify it beyond that "it works for me". But I'm alot more 
>> comfortable going with then saying "hey this default value works for 
>> TI's dra76 evm. Therefore, every MCAN board will be stuck by default
>> for a value that works for us". So if there is no push back with
>> going with SSP = db SP with no documentation to back up why that is
>> being used then I will try that out and send patches.
> 
> This means we postpone the whole add-new-interface-dance until the
> SPP=SP approach doesn't work for some usecase?

I think thats prudent as any new interface (DT or userspace) needs to be
maintained forever. Over time we should have more data from actual
deployments to see if a new interface is really necessary.

Thanks,
Sekhar

[1]
http://www.bosch-semiconductors.de/media/automotive_electronics/pdf_2/ipmodules_3/can_fd_1/icc14_2013_paper_Hartwich.pdf
(last page)

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Oliver Hartkopp]

On 10/19/2017 11:13 AM, Marc Kleine-Budde wrote:
> On 10/19/2017 07:07 AM, Sekhar Nori wrote:


>>>> Since we have a netlink socket interface to configure sample point, I
>>>> wonder if that should be extended to configure SSP too (or at least the
>>>> offset part of SSP)?

+1 too

>>>
>>> Sekhar is right that ideally the user should be able to set the SSP at
>>> runtime. However, my issue is that based on my experience CAN users
>>> expect the driver to just work the majority of times. For unique use
>>> cases where the driver calculated values don't work then the user should
>>> be able to override it. This should only be done for a very small
>>> percentage of CAN users. Unless you allow DT to provide a default SSP
>>> many users of MCAN may find that the default SSP doesn't work and must
>>> always use runtime overrides to get anything to work. I don't think that
>>> is a good user experience which is why I don't like the idea.
>>
>> Fair enough. But not quite sure if CAN users expect CAN-FD to "just
>> work" without doing any bittiming related setup.
> 
>  From my point of view I'd rather buy a board from a HW vendor where
> CAN-FD works, rather than a board where I have to tweak the bit-timing
> for a simple CAN-FD test setup.
> 
> As far as I see for the m_can driver it's a single tuple: "bitrate > 2.5
> MBit/s -> 50%". Do we need an array of tuples in general?
> 
> If good default values are transceiver and board specific, they can go
> into the DT. We need a generic (this means driver agnostic) binding for
> this. If this table needs to be tweaked for special purpose, then we can
> add a netlink interface for this as well. >
> Comments?

By now we calculate reasonable default values (e.g. for SP and SJW), you 
can override by setting alternative values via netlink configuration.

I would tend to stay on this approach and not hide these things in DTs - 
just because of someone wants to initialize his specific interface 'easier'.

One tool, one place is IMHO still the best option.

Regards,
Oliver

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2523 bytes --]

On 10/19/2017 01:14 PM, Oliver Hartkopp wrote:
>>>>> Since we have a netlink socket interface to configure sample point, I
>>>>> wonder if that should be extended to configure SSP too (or at least the
>>>>> offset part of SSP)?
> 
> +1 too

The struct can_bittiming in defined in uapi, so we have to keep ABI
compatibility in mind.

>>>> Sekhar is right that ideally the user should be able to set the SSP at
>>>> runtime. However, my issue is that based on my experience CAN users
>>>> expect the driver to just work the majority of times. For unique use
>>>> cases where the driver calculated values don't work then the user should
>>>> be able to override it. This should only be done for a very small
>>>> percentage of CAN users. Unless you allow DT to provide a default SSP
>>>> many users of MCAN may find that the default SSP doesn't work and must
>>>> always use runtime overrides to get anything to work. I don't think that
>>>> is a good user experience which is why I don't like the idea.
>>>
>>> Fair enough. But not quite sure if CAN users expect CAN-FD to "just
>>> work" without doing any bittiming related setup.
>>
>>  From my point of view I'd rather buy a board from a HW vendor where
>> CAN-FD works, rather than a board where I have to tweak the bit-timing
>> for a simple CAN-FD test setup.
>>
>> As far as I see for the m_can driver it's a single tuple: "bitrate > 2.5
>> MBit/s -> 50%". Do we need an array of tuples in general?
>>
>> If good default values are transceiver and board specific, they can go
>> into the DT. We need a generic (this means driver agnostic) binding for
>> this. If this table needs to be tweaked for special purpose, then we can
>> add a netlink interface for this as well. >
>> Comments?
> 
> By now we calculate reasonable default values (e.g. for SP and SJW), you 
> can override by setting alternative values via netlink configuration.
> 
> I would tend to stay on this approach and not hide these things in DTs - 
> just because of someone wants to initialize his specific interface 'easier'.

If the values are not board specific, then it makes no sense to put them
into the DT.

> One tool, one place is IMHO still the best option.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Oliver Hartkopp]

Hi Marc,

On 10/19/2017 01:26 PM, Marc Kleine-Budde wrote:
> On 10/19/2017 01:14 PM, Oliver Hartkopp wrote:
>>>>>> Since we have a netlink socket interface to configure sample point, I
>>>>>> wonder if that should be extended to configure SSP too (or at least the
>>>>>> offset part of SSP)?
>>
>> +1 too
> 
> The struct can_bittiming in defined in uapi, so we have to keep ABI
> compatibility in mind.
> 

Oh, this is fortunately NO problem ;-)

struct can_bittiming {
         __u32 bitrate;          /* Bit-rate in bits/second */
         __u32 sample_point;     /* Sample point in one-tenth of a 
percent */
         __u32 tq;               /* Time quanta (TQ) in nanoseconds */
         __u32 prop_seg;         /* Propagation segment in TQs */
         __u32 phase_seg1;       /* Phase buffer segment 1 in TQs */
         __u32 phase_seg2;       /* Phase buffer segment 2 in TQs */
         __u32 sjw;              /* Synchronisation jump width in TQs */
         __u32 brp;              /* Bit-rate prescaler */
};

So we have two of these: One for the arbitration bitrate and one 
sample_point for the data bitrate -> the 'secondary' SP -> SSP

:-)

We already have this 'dsample-point' implemented in the ip tool:

$ ip link set vcan0 type can help
Usage: ip link set DEVICE type can
	[ bitrate BITRATE [ sample-point SAMPLE-POINT] ] |
	[ tq TQ prop-seg PROP_SEG phase-seg1 PHASE-SEG1
  	  phase-seg2 PHASE-SEG2 [ sjw SJW ] ]

	[ dbitrate BITRATE [ dsample-point SAMPLE-POINT] ] |  <<-- here!
	[ dtq TQ dprop-seg PROP_SEG dphase-seg1 PHASE-SEG1
  	  dphase-seg2 PHASE-SEG2 [ dsjw SJW ] ]

But AFAIK m_can is not using that value in m_can_set_bittiming().

>>> If good default values are transceiver and board specific, they can go
>>> into the DT. We need a generic (this means driver agnostic) binding for
>>> this. If this table needs to be tweaked for special purpose, then we can
>>> add a netlink interface for this as well. >
>>> Comments?
>>
>> By now we calculate reasonable default values (e.g. for SP and SJW), you
>> can override by setting alternative values via netlink configuration.
>>
>> I would tend to stay on this approach and not hide these things in DTs -
>> just because of someone wants to initialize his specific interface 'easier'.
> 
> If the values are not board specific, then it makes no sense to put them
> into the DT.

When they are NOT(?) board specific?

Thinking about non-SoC CAN adapters with PCI and USB pushing the SSP to 
the DT looks wrong to me.

Best,
Oliver

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Mario Hüttel]

[-- Attachment #1.1: Type: text/plain, Size: 3128 bytes --]


On 10/19/2017 08:35 PM, Oliver Hartkopp wrote:
> Hi Marc,
>
> On 10/19/2017 01:26 PM, Marc Kleine-Budde wrote:
>> On 10/19/2017 01:14 PM, Oliver Hartkopp wrote:
>>>>>>> Since we have a netlink socket interface to configure sample
>>>>>>> point, I
>>>>>>> wonder if that should be extended to configure SSP too (or at
>>>>>>> least the
>>>>>>> offset part of SSP)?
>>>
>>> +1 too
>>
>> The struct can_bittiming in defined in uapi, so we have to keep ABI
>> compatibility in mind.
>>
>
> Oh, this is fortunately NO problem ;-)
>
> struct can_bittiming {
>         __u32 bitrate;          /* Bit-rate in bits/second */
>         __u32 sample_point;     /* Sample point in one-tenth of a
> percent */
>         __u32 tq;               /* Time quanta (TQ) in nanoseconds */
>         __u32 prop_seg;         /* Propagation segment in TQs */
>         __u32 phase_seg1;       /* Phase buffer segment 1 in TQs */
>         __u32 phase_seg2;       /* Phase buffer segment 2 in TQs */
>         __u32 sjw;              /* Synchronisation jump width in TQs */
>         __u32 brp;              /* Bit-rate prescaler */
> };
>
> So we have two of these: One for the arbitration bitrate and one
> sample_point for the data bitrate -> the 'secondary' SP -> SSP
>
> :-)
>
> We already have this 'dsample-point' implemented in the ip tool:
>
> $ ip link set vcan0 type can help
> Usage: ip link set DEVICE type can
>     [ bitrate BITRATE [ sample-point SAMPLE-POINT] ] |
>     [ tq TQ prop-seg PROP_SEG phase-seg1 PHASE-SEG1
>        phase-seg2 PHASE-SEG2 [ sjw SJW ] ]
>
>     [ dbitrate BITRATE [ dsample-point SAMPLE-POINT] ] |  <<-- here!
>     [ dtq TQ dprop-seg PROP_SEG dphase-seg1 PHASE-SEG1
>        dphase-seg2 PHASE-SEG2 [ dsjw SJW ] ]
>
> But AFAIK m_can is not using that value in m_can_set_bittiming().
>
Actually I need some clarification. The sample point of the can core is
between the two time segments.
I always thought that the "sample point" options of the ip tool are used
in the internal
calculation of the two timing segments and is therefore no individual value.

>>>> If good default values are transceiver and board specific, they can go
>>>> into the DT. We need a generic (this means driver agnostic) binding
>>>> for
>>>> this. If this table needs to be tweaked for special purpose, then
>>>> we can
>>>> add a netlink interface for this as well. >
>>>> Comments?
>>>
>>> By now we calculate reasonable default values (e.g. for SP and SJW),
>>> you
>>> can override by setting alternative values via netlink configuration.
>>>
>>> I would tend to stay on this approach and not hide these things in
>>> DTs -
>>> just because of someone wants to initialize his specific interface
>>> 'easier'.
>>
>> If the values are not board specific, then it makes no sense to put them
>> into the DT.
>
> When they are NOT(?) board specific?
>
> Thinking about non-SoC CAN adapters with PCI and USB pushing the SSP
> to the DT looks wrong to me.
>
> Best,
> Oliver


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Oliver Hartkopp]

On 10/19/2017 09:54 PM, Mario Hüttel wrote:
> On 10/19/2017 08:35 PM, Oliver Hartkopp wrote:

>> We already have this 'dsample-point' implemented in the ip tool:
>>
>> $ ip link set vcan0 type can help
>> Usage: ip link set DEVICE type can
>>      [ bitrate BITRATE [ sample-point SAMPLE-POINT] ] |
>>      [ tq TQ prop-seg PROP_SEG phase-seg1 PHASE-SEG1
>>         phase-seg2 PHASE-SEG2 [ sjw SJW ] ]
>>
>>      [ dbitrate BITRATE [ dsample-point SAMPLE-POINT] ] |  <<-- here!
>>      [ dtq TQ dprop-seg PROP_SEG dphase-seg1 PHASE-SEG1
>>         dphase-seg2 PHASE-SEG2 [ dsjw SJW ] ]
>>
>> But AFAIK m_can is not using that value in m_can_set_bittiming().
>>
> Actually I need some clarification. The sample point of the can core is
> between the two time segments.
> I always thought that the "sample point" options of the ip tool are used
> in the internal
> calculation of the two timing segments and is therefore no individual value.

You are right.

See picture at http://www.bittiming.can-wiki.info/

Usually you can give the bitrate and the sample point (which is at 75% 
aka 0.750 by default) and then the kernel-internal bitrate calculating 
algorithm calculates the tq prop-seg phase-seg1 phase-seg2 stuff.

Alternatively you can provide the tq prop-seg phase-seg1 phase-seg2 
stuff on your own which is set to the CAN controller registers then.

For that reason my remark "m_can is not using that value" was wrong as 
m_can just uses the tq prop-seg phase-seg1 phase-seg2 stuff - either 
from the bitrate calculation or provided by the user.

Thanks for the question ;-)

Best,
Oliver

RE: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Ramesh Shanmugasundaram]

> >>>>>>> On 08/18/2017 02:39 PM, Franklin S Cooper Jr wrote:
> >>>>>>>> During test transmitting using CAN-FD at high bitrates (4 Mbps)
> >>>>>>>> only resulted in errors. Scoping the signals I noticed that
> >>>>>>>> only a single bit was being transmitted and with a bit more
> >>>>>>>> investigation realized the actual MCAN IP would go back to
> >>>>>>>> initialization mode automatically.
> >>>>>>>>
> >>>>>>>> It appears this issue is due to the MCAN needing to use the
> >>>>>>>> Transmitter Delay Compensation Mode as defined in the MCAN
> >>>>>>>> User's Guide. When this mode is used the User's Guide indicates
> >>>>>>>> that the Transmitter Delay Compensation Offset register should
> >>>>>>>> be set. The document mentions that this register should be set
> >>>>>>>> to (1/dbitrate)/2*(Func Clk Freq).
> >>>>>>>>
> >>>>>>>> Additional CAN-CIA's "Bit Time Requirements for CAN FD"
> >>>>>>>> document indicates that this TDC mode is only needed for data
> >>>>>>>> bit rates above 2.5 Mbps.
> >>>>>>>> Therefore, only enable this mode and only set TDCO when the
> >>>>>>>> data bit rate is above 2.5 Mbps.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Franklin S Cooper Jr <fcooper@ti.com>
> >>>>>>>> ---
> >>>>>>>> I'm pretty surprised that this hasn't been implemented already
> >>>>>>>> since the primary purpose of CAN-FD is to go beyond 1 Mbps and
> >>>>>>>> the MCAN IP supports up to 10 Mbps.
> >>>>>>>>
> >>>>>>>> So it will be nice to get comments from users of this driver to
> >>>>>>>> understand if they have been able to use CAN-FD beyond 2.5 Mbps
> >>>>>>>> without this patch.
> >>>>>>>> If they haven't what did they do to get around it if they
> >>>>>>>> needed higher speeds.
> >>>>>>>>
> >>>>>>>> Meanwhile I plan on testing this using a more "realistic" CAN
> >>>>>>>> bus to insure everything still works at 5 Mbps which is the max
> >>>>>>>> speed of my CAN transceiver.
> >>>>>>> ping. Anyone has any thoughts on this?
> >>>>>> I added Dong who authored the m_can driver and Wenyou who added
> >>>>>> the only in-kernel user of the driver for any help.
> >>>>> I tested it on SAMA5D2 Xplained board both with and without this
> >>>>> patch, both work with the 4M bps data bit rate.
> >>>> Thank you for testing this out. Its interesting that you have been
> >>>> able to use higher speeds without this patch. What is the CAN
> >>>> transceiver being used on the SAMA5D2 Xplained board? I tried
> >>>> looking at the schematic but it seems the CAN signals are used on
> >>>> an extension board which I can't find the schematic for. Also do
> >>>> you mind sharing your test setup? Were you doing a short point to
> point test?
> >>>>
> >>>> Thank You,
> >>>> Franklin
> >>> Hello Franklin,
> >>>
> >>> your patch definitely makes sense.
> >>>
> >>> I forgot the TDC in my patches because it was not present in the
> >>> previous driver versions and because I didn't encounter any problems
> >>> when testing it myself.
> >>>
> >>> The error is highly dependent on the hardware (transceiver) setup.
> >>> So it is definitely possible that some people don't encounter errors
> >>> without your patch.
> >>
> >> So the Transmission Delay Compensation feature Value register is
> >> suppose to take into consideration the transceiver delay
> >> automatically and add the value of TDCO on top of that. So why would
> >> TDCO be dependent on the transceiver? I've heard conflicting things
> >> regarding TDC so any clarification on what actually impacts it would be
> appreciated.
> >>
> >> Also part of the issue I'm having is how can we properly configure
> TDCO?
> >> Configuring TDCO is essentially figuring out what Secondary Sample
> >> Point to use. However, it is unclear what value to set SSP to and
> >> which use cases a given SSP will work or doesn't work. I've seen
> >> various recommendations from Bosch on choosing SSP but ultimately it
> >> seems they suggestion "real world testing" to come up with a proper
> >> value. Not setting TDCO causes problems for my device and improperly
> >> setting TDCO causes problems for my device. So its likely any value I
> >> use could end up breaking something for someone else.
> >>
> >> Currently I leaning to a DT property that can be used for setting SSP.
> >> Perhaps use a generic default value and allow individuals to override
> >> it via DT?
> >
> > Sounds reasonable. What's the status of this series?
> 
> I have had some offline discussions with Franklin on this, and I am not
> fully convinced that DT is the way to go here (although I don't have the
> agreement with Franklin there).
> 
> There are two components in configuring the secondary sample point. It is
> the transceiver loopback delay and an offset (example half of the bit time
> in data phase).
> 
> While the transceiver loopback delay is pretty board dependent (and thus
> amenable to DT encoding), I am not quite sure the offset can be configured
> in DT because its not really board dependent.
> 
> Unfortunately, offset calculation does not seem to be an exact science.
> There are recommendations ranging from using 50% of bit time to making it
> same as the sample point configured. This means users who need to change
> the SSP due to offset variations need to change  their DT even without
> anything changing on their board.
> 
> Since we have a netlink socket interface to configure sample point, I
> wonder if that should be extended to configure SSP too (or at least the
> offset part of SSP)?

+1

I also wonder how a default TDCO setting in DT would help. Is the driver going to hard code the Tq settings as well for the most commonly used bitrate? A link up start-up script (using netlink) would help setting a default TDCO along with other params if this is the main concern.

Thanks,
Ramesh

Re: [RFC PATCH] can: m_can: Support higher speed CAN-FD bitrates

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 2149 bytes --]

On 10/19/2017 10:04 AM, Ramesh Shanmugasundaram wrote:
>>> Sounds reasonable. What's the status of this series?
>>
>> I have had some offline discussions with Franklin on this, and I am not
>> fully convinced that DT is the way to go here (although I don't have the
>> agreement with Franklin there).
>>
>> There are two components in configuring the secondary sample point. It is
>> the transceiver loopback delay and an offset (example half of the bit time
>> in data phase).
>>
>> While the transceiver loopback delay is pretty board dependent (and thus
>> amenable to DT encoding), I am not quite sure the offset can be configured
>> in DT because its not really board dependent.
>>
>> Unfortunately, offset calculation does not seem to be an exact science.
>> There are recommendations ranging from using 50% of bit time to making it
>> same as the sample point configured. This means users who need to change
>> the SSP due to offset variations need to change  their DT even without
>> anything changing on their board.
>>
>> Since we have a netlink socket interface to configure sample point, I
>> wonder if that should be extended to configure SSP too (or at least the
>> offset part of SSP)?
> 
> +1
> > I also wonder how a default TDCO setting in DT would help.

For a given board with a given transceiver and layout and proper values
in the supplied DT the higher bitrates would work out of the box.

> Is the driver going to hard code the Tq settings as well for the
> most commonly used bitrate?

What's Tq in this context. Time Quanta?

> A link up start-up script (using netlink) would help setting a
> default TDCO along with other params if this is the main concern.

There are various ways to setup these values. Plain old start-up scripts
are one of them, systemd-networkd or network manager are others.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]