| Message ID | 20170818193947.27862-1-fcooper@ti.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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); > } > >
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); >> } >> >>
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
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 >
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
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 > > >
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
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
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 >
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
> >>>>>>> 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
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
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
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
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
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
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
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 >
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
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 >
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
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
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
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)
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); }
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(-)