| Message ID | 1415174326-6623-1-git-send-email-b29396@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05.11.2014 08:58, Dong Aisheng wrote: > @@ -327,41 +357,65 @@ static inline void m_can_disable_all_interrupts(const struct m_can_priv *priv) > m_can_write(priv, M_CAN_ILE, 0x0); > } > > -static void m_can_read_fifo(const struct net_device *dev, struct can_frame *cf, > - u32 rxfs) > +static void m_can_read_fifo(struct net_device *dev, u32 rxfs) > { > + struct net_device_stats *stats = &dev->stats; > struct m_can_priv *priv = netdev_priv(dev); > - u32 id, fgi; > + struct canfd_frame *cf; > + struct sk_buff *skb; > + u32 id, fgi, dlc; > + int i; > > /* calculate the fifo get index for where to read data */ > fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF; > + dlc = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC); > + if (dlc & RX_BUF_EDL) > + skb = alloc_canfd_skb(dev, &cf); > + else > + skb = alloc_can_skb(dev, (struct can_frame **)&cf); Yes. That's the way it should look like ;-) > + if (!skb) { > + stats->rx_dropped++; > + return; > + } > + > id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_ID); > if (id & RX_BUF_XTD) > cf->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG; > else > cf->can_id = (id >> 18) & CAN_SFF_MASK; > > - if (id & RX_BUF_RTR) { > + if (id & RX_BUF_ESI) { > + cf->flags |= CANFD_ESI; > + netdev_dbg(dev, "ESI Error\n"); > + } > + > + if (!(dlc & RX_BUF_EDL) && (id & RX_BUF_RTR)) { > cf->can_id |= CAN_RTR_FLAG; > } else { > id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC); > - cf->can_dlc = get_can_dlc((id >> 16) & 0x0F); > - *(u32 *)(cf->data + 0) = m_can_fifo_read(priv, fgi, > - M_CAN_FIFO_DATA(0)); > - *(u32 *)(cf->data + 4) = m_can_fifo_read(priv, fgi, > - M_CAN_FIFO_DATA(1)); > + cf->len = can_dlc2len(get_canfd_dlc((id >> 16) & 0x0F)); if (dlc & RX_BUF_EDL) cf->len = can_dlc2len((id >> 16) & 0x0F); else cf->len = get_can_dlc((id >> 16) & 0x0F); (..) > @@ -804,7 +870,8 @@ static void m_can_chip_config(struct net_device *dev) > RXFC_FWM_1 | priv->mcfg[MRAM_RXF1].off); > > cccr = m_can_read(priv, M_CAN_CCCR); > - cccr &= ~(CCCR_TEST | CCCR_MON); > + cccr &= ~(CCCR_TEST | CCCR_MON | (CCCR_CMR_MASK << CCCR_CMR_SHIFT) | > + (CCCR_CME_MASK << CCCR_CME_SHIFT)); > test = m_can_read(priv, M_CAN_TEST); > test &= ~TEST_LBCK; > > @@ -816,6 +883,9 @@ static void m_can_chip_config(struct net_device *dev) > test |= TEST_LBCK; > } > > + if (priv->can.ctrlmode & CAN_CTRLMODE_FD) > + cccr |= CCCR_CME_CANFD_BRS << CCCR_CME_SHIFT; > + > m_can_write(priv, M_CAN_CCCR, cccr); > m_can_write(priv, M_CAN_TEST, test); > (..) > > + if (priv->can.ctrlmode & CAN_CTRLMODE_FD) { > + cccr = m_can_read(priv, M_CAN_CCCR); > + cccr &= ~(CCCR_CMR_MASK << CCCR_CMR_SHIFT); > + if (cf->flags & CANFD_BRS) > + cccr |= CCCR_CMR_CANFD_BRS << CCCR_CMR_SHIFT; > + else > + cccr |= CCCR_CMR_CANFD << CCCR_CMR_SHIFT; > + m_can_write(priv, M_CAN_CCCR, cccr); > + } Unfortunately No. Here it becomes complicated due to the fact that the revision 3.0.x does not support per-frame switching for FD/BRS ... When (priv->can.ctrlmode & CAN_CTRLMODE_FD) is true this *only* tells us, that the controller is _capable_ to send either CAN or CAN FD frames. It does not configure the controller into one of these specific settings! Additionally: AFAIK when writing to the CCCR you have to make sure that there's currently no ongoing transfer. I would suggest the following approach to make the revision 3.0.x act like a correct CAN FD capable controller: - create a helper function to switch FD and BRS while taking care of ongoing transmissions - create a variable that knows the current tx_mode for FD and BRS When we need to send a CAN frame which doesn't fit the settings in the tx_mode we need to switch the CCCR and update the tx_mode variable. This at least reduces the needed CCCR operations. And it also addresses the intention of your patch [PATCH V1 4/4] can: m_can: allow to send std frame on CAN FD mode Regards, Oliver
On Wed, Nov 05, 2014 at 11:12:24AM +0100, Oliver Hartkopp wrote: > On 05.11.2014 08:58, Dong Aisheng wrote: > > >@@ -327,41 +357,65 @@ static inline void m_can_disable_all_interrupts(const struct m_can_priv *priv) > > m_can_write(priv, M_CAN_ILE, 0x0); > > } > > > >-static void m_can_read_fifo(const struct net_device *dev, struct can_frame *cf, > >- u32 rxfs) > >+static void m_can_read_fifo(struct net_device *dev, u32 rxfs) > > { > >+ struct net_device_stats *stats = &dev->stats; > > struct m_can_priv *priv = netdev_priv(dev); > >- u32 id, fgi; > >+ struct canfd_frame *cf; > >+ struct sk_buff *skb; > >+ u32 id, fgi, dlc; > >+ int i; > > > > /* calculate the fifo get index for where to read data */ > > fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF; > >+ dlc = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC); > >+ if (dlc & RX_BUF_EDL) > >+ skb = alloc_canfd_skb(dev, &cf); > >+ else > >+ skb = alloc_can_skb(dev, (struct can_frame **)&cf); > > Yes. That's the way it should look like ;-) > > >+ if (!skb) { > >+ stats->rx_dropped++; > >+ return; > >+ } > >+ > > id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_ID); > > if (id & RX_BUF_XTD) > > cf->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG; > > else > > cf->can_id = (id >> 18) & CAN_SFF_MASK; > > > >- if (id & RX_BUF_RTR) { > >+ if (id & RX_BUF_ESI) { > >+ cf->flags |= CANFD_ESI; > >+ netdev_dbg(dev, "ESI Error\n"); > >+ } > >+ > >+ if (!(dlc & RX_BUF_EDL) && (id & RX_BUF_RTR)) { > > cf->can_id |= CAN_RTR_FLAG; > > } else { > > id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC); > >- cf->can_dlc = get_can_dlc((id >> 16) & 0x0F); > >- *(u32 *)(cf->data + 0) = m_can_fifo_read(priv, fgi, > >- M_CAN_FIFO_DATA(0)); > >- *(u32 *)(cf->data + 4) = m_can_fifo_read(priv, fgi, > >- M_CAN_FIFO_DATA(1)); > >+ cf->len = can_dlc2len(get_canfd_dlc((id >> 16) & 0x0F)); > > if (dlc & RX_BUF_EDL) > cf->len = can_dlc2len((id >> 16) & 0x0F); > else > cf->len = get_can_dlc((id >> 16) & 0x0F); > > (..) > Correct. Will change it. > >@@ -804,7 +870,8 @@ static void m_can_chip_config(struct net_device *dev) > > RXFC_FWM_1 | priv->mcfg[MRAM_RXF1].off); > > > > cccr = m_can_read(priv, M_CAN_CCCR); > >- cccr &= ~(CCCR_TEST | CCCR_MON); > >+ cccr &= ~(CCCR_TEST | CCCR_MON | (CCCR_CMR_MASK << CCCR_CMR_SHIFT) | > >+ (CCCR_CME_MASK << CCCR_CME_SHIFT)); > > test = m_can_read(priv, M_CAN_TEST); > > test &= ~TEST_LBCK; > > > >@@ -816,6 +883,9 @@ static void m_can_chip_config(struct net_device *dev) > > test |= TEST_LBCK; > > } > > > >+ if (priv->can.ctrlmode & CAN_CTRLMODE_FD) > >+ cccr |= CCCR_CME_CANFD_BRS << CCCR_CME_SHIFT; > >+ > > m_can_write(priv, M_CAN_CCCR, cccr); > > m_can_write(priv, M_CAN_TEST, test); > > > > (..) > > > > >+ if (priv->can.ctrlmode & CAN_CTRLMODE_FD) { > >+ cccr = m_can_read(priv, M_CAN_CCCR); > >+ cccr &= ~(CCCR_CMR_MASK << CCCR_CMR_SHIFT); > >+ if (cf->flags & CANFD_BRS) > >+ cccr |= CCCR_CMR_CANFD_BRS << CCCR_CMR_SHIFT; > >+ else > >+ cccr |= CCCR_CMR_CANFD << CCCR_CMR_SHIFT; > >+ m_can_write(priv, M_CAN_CCCR, cccr); > >+ } > > Unfortunately No. Here it becomes complicated due to the fact that > the revision 3.0.x does not support per-frame switching for FD/BRS > ... I'm not sure i got your point. From m_can spec, it allows switch CAN mode by setting CMR bit. Bits 11:10 CMR[1:0]: CAN Mode Request A change of the CAN operation mode is requested by writing to this bit field. After change to the requested operation mode the bit field is reset to “00” and the status flags FDBS and FDO are set accordingly. In case the requested CAN operation mode is not enabled, the value written to CMR is retained until it is overwritten by the next mode change request. In case CME = “01”/”10”/”11” a change to CAN operation according to ISO 11898-1 is always possible. Default is CAN operation according to ISO11898-1. 00 unchanged 01 Request CAN FD operation 10 Request CAN FD operation with bit rate switching 11 Request CAN operation according ISO11898-1 So what's the difference between this function and the per-frame switching you mentioned? > > When (priv->can.ctrlmode & CAN_CTRLMODE_FD) is true this *only* > tells us, that the controller is _capable_ to send either CAN or CAN > FD frames. > > It does not configure the controller into one of these specific settings! > > Additionally: AFAIK when writing to the CCCR you have to make sure > that there's currently no ongoing transfer. > I don't know about it before. By searching m_can user manual v302 again, i still did not find this limitation. Can you point me if you know where it is? BTW, since we only use one Tx Buffer for transmission currently, so we should not meet that case that CAN mode is switched during transfer. So the issue you concern may not happen. Additionally it looks to me like m_can does allow set CMR bit at runtime since the mode change will take affect when controller becomes idle. See below: "A mode change requested by writing to CCCR.CMR will be executed next time the CAN protocol controller FSM reaches idle phase between CAN frames. Upon this event CCCR.CMR is reset to “00” and the status flags CCCR.FDBS and CCCR.FDO are set accordingly. In case the requested CAN operation mode is not enabled, the value written to CCCR.CMR is retained until it is overwritten by the next mode change request. Default is CAN operation according to ISO11898-1." > I would suggest the following approach to make the revision 3.0.x > act like a correct CAN FD capable controller: > > - create a helper function to switch FD and BRS while taking care of > ongoing transmissions > > - create a variable that knows the current tx_mode for FD and BRS > > When we need to send a CAN frame which doesn't fit the settings in > the tx_mode we need to switch the CCCR and update the tx_mode > variable. > > This at least reduces the needed CCCR operations. > Yes, i can do like that. But what i see by doing that is only reduces the needed CCCR operations? Any other benefits i missed? And the test for current code showed it seemed work well on the Mode switch among std frame/fd frame/brs frame. Regards Dong Aisheng > And it also addresses the intention of your patch > [PATCH V1 4/4] can: m_can: allow to send std frame on CAN FD mode > > Regards, > Oliver >
Hi Dong Aisheng, On 11/05/2014 01:28 PM, Dong Aisheng wrote: > Bosch M_CAN is CAN FD capable device. This patch implements the CAN > FD features include up to 64 bytes payload and bitrate switch function. > 1) Change the Rx FIFO and Tx Buffer to 64 bytes for support CAN FD > up to 64 bytes payload. It's backward compatible with old 8 bytes > normal CAN frame. > 2) Allocate can frame or canfd frame based on EDL bit > 3) Bitrate Switch function is disabled by default and will be enabled > according to CANFD_BRS bit in cf->flags. > > Signed-off-by: Dong Aisheng <b29396@freescale.com> In these four patches two of them are V1 and other two are V2. Is there any reason..? How will you be able to generate like this by git format-patch..?
On Wed, Nov 05, 2014 at 05:05:11PM +0530, Varka Bhadram wrote: > Hi Dong Aisheng, > > On 11/05/2014 01:28 PM, Dong Aisheng wrote: > > >Bosch M_CAN is CAN FD capable device. This patch implements the CAN > >FD features include up to 64 bytes payload and bitrate switch function. > >1) Change the Rx FIFO and Tx Buffer to 64 bytes for support CAN FD > > up to 64 bytes payload. It's backward compatible with old 8 bytes > > normal CAN frame. > >2) Allocate can frame or canfd frame based on EDL bit > >3) Bitrate Switch function is disabled by default and will be enabled > > according to CANFD_BRS bit in cf->flags. > > > >Signed-off-by: Dong Aisheng <b29396@freescale.com> > > In these four patches two of them are V1 and other two are V2. > > Is there any reason..? > Sorry for confusion. Because some patches are already picked by Macr on the first round. See below: http://www.spinics.net/lists/netdev/msg302133.html > How will you be able to generate like this by git format-patch..? > Generated as v2 and i manually changed the later two to v1 since they are new. Not sure if it's suitable to do like that. :-) Regards Dong Aisheng > -- > Thanks and Regards, > Varka Bhadram. >
On Wed, Nov 05, 2014 at 02:10:05PM +0100, Oliver Hartkopp wrote: > On 05.11.2014 12:26, Dong Aisheng wrote: > >On Wed, Nov 05, 2014 at 11:12:24AM +0100, Oliver Hartkopp wrote: > >>On 05.11.2014 08:58, Dong Aisheng wrote: > > > >>Unfortunately No. Here it becomes complicated due to the fact that > >>the revision 3.0.x does not support per-frame switching for FD/BRS > >>... > > > >I'm not sure i got your point. > > From m_can spec, it allows switch CAN mode by setting CMR bit. > > > >Bits 11:10 CMR[1:0]: CAN Mode Request > >A change of the CAN operation mode is requested by writing to this bit field. After change to the > >requested operation mode the bit field is reset to “00” and the status flags FDBS and FDO are set > >accordingly. In case the requested CAN operation mode is not enabled, the value written to CMR is > >retained until it is overwritten by the next mode change request. In case CME = “01”/”10”/”11” a > >change to CAN operation according to ISO 11898-1 is always possible. Default is CAN operation > >according to ISO11898-1. > >00 unchanged > >01 Request CAN FD operation > >10 Request CAN FD operation with bit rate switching > >11 Request CAN operation according ISO11898-1 > > > >So what's the difference between this function and the per-frame switching > >you mentioned? > > > >> > >>When (priv->can.ctrlmode & CAN_CTRLMODE_FD) is true this *only* > >>tells us, that the controller is _capable_ to send either CAN or CAN > >>FD frames. > >> > >>It does not configure the controller into one of these specific settings! > >> > >>Additionally: AFAIK when writing to the CCCR you have to make sure > >>that there's currently no ongoing transfer. > >> > > > >I don't know about it before. > >By searching m_can user manual v302 again, i still did not find this > >limitation. Can you point me if you know where it is? > > > >BTW, since we only use one Tx Buffer for transmission currently, so we > >should not meet that case that CAN mode is switched during transfer. > >So the issue you concern may not happen. > > Yes. You are right. Having a FIFO with a size of 1 will help here :-) > > > > >Additionally it looks to me like m_can does allow set CMR bit at runtime > >since the mode change will take affect when controller becomes idle. > >See below: > > > >"A mode change requested by writing to CCCR.CMR will be executed next > >time the CAN protocol controller FSM reaches idle phase between CAN frames. > >Upon this event CCCR.CMR is reset to “00” and the status flags CCCR.FDBS > >and CCCR.FDO are set accordingly. In case the requested > >CAN operation mode is not enabled, the value written to CCCR.CMR is > >retained until it is overwritten by the next mode change request. > >Default is CAN operation according to ISO11898-1." > > Ah. I assumed we have to take care to set these bits in the idle state. > > So when thinking about the one and only tx buffer your current > approach should work indeed. Sorry for my confusion. > > > > >>I would suggest the following approach to make the revision 3.0.x > >>act like a correct CAN FD capable controller: > >> > >>- create a helper function to switch FD and BRS while taking care of > >>ongoing transmissions > >> > >>- create a variable that knows the current tx_mode for FD and BRS > >> > >>When we need to send a CAN frame which doesn't fit the settings in > >>the tx_mode we need to switch the CCCR and update the tx_mode > >>variable. > >> > >>This at least reduces the needed CCCR operations. > >> > > > >Yes, i can do like that. > >But what i see by doing that is only reduces the needed CCCR operations? > >Any other benefits i missed? > > No. I just thought about a function that also takes care of the idle > phase to switch the bits. But now you made it clear that this is not > needed - so you can stay on your current implementation: Setting the > CCCR each time before sending the frame. > Okay > With the 3.1.x or 3.2.x this code will become obsolete then as the > EDL and BRS seeting will get extra bits in the Tx Buffer. > But that's a future point. > Got it. Will update it in the future when the new IP doc is available. > >And the test for current code showed it seemed work well on the Mode > >switch among std frame/fd frame/brs frame. > > Fine. > > Btw. I would suggest to integrate patch [4/4] into [3/4]. > Yes, will do it. Thanks Regards Dong Aisheng > Best regards, > Oliver >
On Wed, Nov 05, 2014 at 02:15:27PM +0100, Oliver Hartkopp wrote: > Typo > > On 05.11.2014 14:10, Oliver Hartkopp wrote: > > >Btw. I would suggest to integrate patch [4/4] into [3/4]. > > into [1/4] of course Understand! :-) Regards Dong Aisheng
On 05.11.2014 12:26, Dong Aisheng wrote: > On Wed, Nov 05, 2014 at 11:12:24AM +0100, Oliver Hartkopp wrote: >> On 05.11.2014 08:58, Dong Aisheng wrote: >> Unfortunately No. Here it becomes complicated due to the fact that >> the revision 3.0.x does not support per-frame switching for FD/BRS >> ... > > I'm not sure i got your point. > From m_can spec, it allows switch CAN mode by setting CMR bit. > > Bits 11:10 CMR[1:0]: CAN Mode Request > A change of the CAN operation mode is requested by writing to this bit field. After change to the > requested operation mode the bit field is reset to “00” and the status flags FDBS and FDO are set > accordingly. In case the requested CAN operation mode is not enabled, the value written to CMR is > retained until it is overwritten by the next mode change request. In case CME = “01”/”10”/”11” a > change to CAN operation according to ISO 11898-1 is always possible. Default is CAN operation > according to ISO11898-1. > 00 unchanged > 01 Request CAN FD operation > 10 Request CAN FD operation with bit rate switching > 11 Request CAN operation according ISO11898-1 > > So what's the difference between this function and the per-frame switching > you mentioned? > >> >> When (priv->can.ctrlmode & CAN_CTRLMODE_FD) is true this *only* >> tells us, that the controller is _capable_ to send either CAN or CAN >> FD frames. >> >> It does not configure the controller into one of these specific settings! >> >> Additionally: AFAIK when writing to the CCCR you have to make sure >> that there's currently no ongoing transfer. >> > > I don't know about it before. > By searching m_can user manual v302 again, i still did not find this > limitation. Can you point me if you know where it is? > > BTW, since we only use one Tx Buffer for transmission currently, so we > should not meet that case that CAN mode is switched during transfer. > So the issue you concern may not happen. Yes. You are right. Having a FIFO with a size of 1 will help here :-) > > Additionally it looks to me like m_can does allow set CMR bit at runtime > since the mode change will take affect when controller becomes idle. > See below: > > "A mode change requested by writing to CCCR.CMR will be executed next > time the CAN protocol controller FSM reaches idle phase between CAN frames. > Upon this event CCCR.CMR is reset to “00” and the status flags CCCR.FDBS > and CCCR.FDO are set accordingly. In case the requested > CAN operation mode is not enabled, the value written to CCCR.CMR is > retained until it is overwritten by the next mode change request. > Default is CAN operation according to ISO11898-1." Ah. I assumed we have to take care to set these bits in the idle state. So when thinking about the one and only tx buffer your current approach should work indeed. Sorry for my confusion. > >> I would suggest the following approach to make the revision 3.0.x >> act like a correct CAN FD capable controller: >> >> - create a helper function to switch FD and BRS while taking care of >> ongoing transmissions >> >> - create a variable that knows the current tx_mode for FD and BRS >> >> When we need to send a CAN frame which doesn't fit the settings in >> the tx_mode we need to switch the CCCR and update the tx_mode >> variable. >> >> This at least reduces the needed CCCR operations. >> > > Yes, i can do like that. > But what i see by doing that is only reduces the needed CCCR operations? > Any other benefits i missed? No. I just thought about a function that also takes care of the idle phase to switch the bits. But now you made it clear that this is not needed - so you can stay on your current implementation: Setting the CCCR each time before sending the frame. With the 3.1.x or 3.2.x this code will become obsolete then as the EDL and BRS seeting will get extra bits in the Tx Buffer. But that's a future point. > And the test for current code showed it seemed work well on the Mode > switch among std frame/fd frame/brs frame. Fine. Btw. I would suggest to integrate patch [4/4] into [3/4]. Best regards, Oliver
Typo
On 05.11.2014 14:10, Oliver Hartkopp wrote:
> Btw. I would suggest to integrate patch [4/4] into [3/4].
into [1/4] of course
On 11/05/2014 02:10 PM, Oliver Hartkopp wrote: > On 05.11.2014 12:26, Dong Aisheng wrote: >> On Wed, Nov 05, 2014 at 11:12:24AM +0100, Oliver Hartkopp wrote: >>> On 05.11.2014 08:58, Dong Aisheng wrote: > > >>> Unfortunately No. Here it becomes complicated due to the fact that >>> the revision 3.0.x does not support per-frame switching for FD/BRS >>> ... >> >> I'm not sure i got your point. >> From m_can spec, it allows switch CAN mode by setting CMR bit. >> >> Bits 11:10 CMR[1:0]: CAN Mode Request >> A change of the CAN operation mode is requested by writing to this bit >> field. After change to the >> requested operation mode the bit field is reset to “00” and the status >> flags FDBS and FDO are set >> accordingly. In case the requested CAN operation mode is not enabled, >> the value written to CMR is >> retained until it is overwritten by the next mode change request. In >> case CME = “01”/”10”/”11” a >> change to CAN operation according to ISO 11898-1 is always possible. >> Default is CAN operation >> according to ISO11898-1. >> 00 unchanged >> 01 Request CAN FD operation >> 10 Request CAN FD operation with bit rate switching >> 11 Request CAN operation according ISO11898-1 >> >> So what's the difference between this function and the per-frame >> switching >> you mentioned? >> >>> >>> When (priv->can.ctrlmode & CAN_CTRLMODE_FD) is true this *only* >>> tells us, that the controller is _capable_ to send either CAN or CAN >>> FD frames. >>> >>> It does not configure the controller into one of these specific >>> settings! >>> >>> Additionally: AFAIK when writing to the CCCR you have to make sure >>> that there's currently no ongoing transfer. >>> >> >> I don't know about it before. >> By searching m_can user manual v302 again, i still did not find this >> limitation. Can you point me if you know where it is? >> >> BTW, since we only use one Tx Buffer for transmission currently, so we >> should not meet that case that CAN mode is switched during transfer. >> So the issue you concern may not happen. > > Yes. You are right. Having a FIFO with a size of 1 will help here :-) Errrr....sorry...no. Taking an easy route here but making it x times harder to extend the driver to make use of the FIFO is not an option. Marc
On Wed, Nov 05, 2014 at 02:19:22PM +0100, Marc Kleine-Budde wrote: > On 11/05/2014 02:10 PM, Oliver Hartkopp wrote: > > On 05.11.2014 12:26, Dong Aisheng wrote: > >> On Wed, Nov 05, 2014 at 11:12:24AM +0100, Oliver Hartkopp wrote: > >>> On 05.11.2014 08:58, Dong Aisheng wrote: > > > > > >>> Unfortunately No. Here it becomes complicated due to the fact that > >>> the revision 3.0.x does not support per-frame switching for FD/BRS > >>> ... > >> > >> I'm not sure i got your point. > >> From m_can spec, it allows switch CAN mode by setting CMR bit. > >> > >> Bits 11:10 CMR[1:0]: CAN Mode Request > >> A change of the CAN operation mode is requested by writing to this bit > >> field. After change to the > >> requested operation mode the bit field is reset to “00” and the status > >> flags FDBS and FDO are set > >> accordingly. In case the requested CAN operation mode is not enabled, > >> the value written to CMR is > >> retained until it is overwritten by the next mode change request. In > >> case CME = “01”/”10”/”11” a > >> change to CAN operation according to ISO 11898-1 is always possible. > >> Default is CAN operation > >> according to ISO11898-1. > >> 00 unchanged > >> 01 Request CAN FD operation > >> 10 Request CAN FD operation with bit rate switching > >> 11 Request CAN operation according ISO11898-1 > >> > >> So what's the difference between this function and the per-frame > >> switching > >> you mentioned? > >> > >>> > >>> When (priv->can.ctrlmode & CAN_CTRLMODE_FD) is true this *only* > >>> tells us, that the controller is _capable_ to send either CAN or CAN > >>> FD frames. > >>> > >>> It does not configure the controller into one of these specific > >>> settings! > >>> > >>> Additionally: AFAIK when writing to the CCCR you have to make sure > >>> that there's currently no ongoing transfer. > >>> > >> > >> I don't know about it before. > >> By searching m_can user manual v302 again, i still did not find this > >> limitation. Can you point me if you know where it is? > >> > >> BTW, since we only use one Tx Buffer for transmission currently, so we > >> should not meet that case that CAN mode is switched during transfer. > >> So the issue you concern may not happen. > > > > Yes. You are right. Having a FIFO with a size of 1 will help here :-) > > Errrr....sorry...no. > > Taking an easy route here but making it x times harder to extend the > driver to make use of the FIFO is not an option. > Hmm, this way is just following the original approach the current driver used. It's initial work and won't make things complicated. Extend the driver to use FIFO for TX is another story and based on my understanding it may be a bit complicated to do CAN FD mode switch on this case due to hw limitation that the revision 3.0.x does not support per-frame switching for FD/BRS as Oliver pointed out. (e.g. how to switch FD MODE for each frame on Tx FIFO?) Probably that's why the 3.1.x version will add the FD/BRS bit controller in Tx Buffer to fix this issue. Anyway, that's future work and we can discuss it when adding FIFO support for Tx function. Regards Dong Aisheng > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Industrial Linux Solutions | Phone: +49-231-2826-924 | > Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | > Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | >
On 05.11.2014 14:46, Dong Aisheng wrote: > On Wed, Nov 05, 2014 at 02:19:22PM +0100, Marc Kleine-Budde wrote: >> On 11/05/2014 02:10 PM, Oliver Hartkopp wrote: >>> On 05.11.2014 12:26, Dong Aisheng wrote: >>>> On Wed, Nov 05, 2014 at 11:12:24AM +0100, Oliver Hartkopp wrote: >>>>> On 05.11.2014 08:58, Dong Aisheng wrote: >>> >>> >>>>> Unfortunately No. Here it becomes complicated due to the fact that >>>>> the revision 3.0.x does not support per-frame switching for FD/BRS >>>>> ... >>>> >>>> I'm not sure i got your point. >>>> From m_can spec, it allows switch CAN mode by setting CMR bit. >>>> >>>> Bits 11:10 CMR[1:0]: CAN Mode Request >>>> A change of the CAN operation mode is requested by writing to this bit >>>> field. After change to the >>>> requested operation mode the bit field is reset to “00” and the status >>>> flags FDBS and FDO are set >>>> accordingly. In case the requested CAN operation mode is not enabled, >>>> the value written to CMR is >>>> retained until it is overwritten by the next mode change request. In >>>> case CME = “01”/”10”/”11” a >>>> change to CAN operation according to ISO 11898-1 is always possible. >>>> Default is CAN operation >>>> according to ISO11898-1. >>>> 00 unchanged >>>> 01 Request CAN FD operation >>>> 10 Request CAN FD operation with bit rate switching >>>> 11 Request CAN operation according ISO11898-1 >>>> >>>> So what's the difference between this function and the per-frame >>>> switching >>>> you mentioned? >>>> >>>>> >>>>> When (priv->can.ctrlmode & CAN_CTRLMODE_FD) is true this *only* >>>>> tells us, that the controller is _capable_ to send either CAN or CAN >>>>> FD frames. >>>>> >>>>> It does not configure the controller into one of these specific >>>>> settings! >>>>> >>>>> Additionally: AFAIK when writing to the CCCR you have to make sure >>>>> that there's currently no ongoing transfer. >>>>> >>>> >>>> I don't know about it before. >>>> By searching m_can user manual v302 again, i still did not find this >>>> limitation. Can you point me if you know where it is? >>>> >>>> BTW, since we only use one Tx Buffer for transmission currently, so we >>>> should not meet th > Regards > Dong Aisheng > >> Marc >> >> -- >> Pengutronix e.K. | Marc Kleine-Budde | >> Industrial Linux Solutions | Phone: +49-231-2826-924 | >> Vertretung West/Dortmund | Fax: +49-5121-206917-5555 | >> Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de | >> > > at case that CAN mode is switched during transfer. >>>> So the issue you concern may not happen. >>> >>> Yes. You are right. Having a FIFO with a size of 1 will help here :-) >> >> Errrr....sorry...no. >> >> Taking an easy route here but making it x times harder to extend the >> driver to make use of the FIFO is not an option. >> > > Hmm, this way is just following the original approach the current driver > used. It's initial work and won't make things complicated. > > Extend the driver to use FIFO for TX is another story and based on > my understanding it may be a bit complicated to do CAN FD mode switch on this > case due to hw limitation that the revision 3.0.x does not support per-frame > switching for FD/BRS as Oliver pointed out. > (e.g. how to switch FD MODE for each frame on Tx FIFO?) > Probably that's why the 3.1.x version will add the FD/BRS bit controller > in Tx Buffer to fix this issue. > > Anyway, that's future work and we can discuss it when adding FIFO support > for Tx function. > Yes. I have to second this opinion. I also would like to have a TX FIFO. But due to the limitations of the 3.0.x M_CAN I would suggest to prefer a 'correct" CAN FD driver implementation in favor of having a TX FIFO which is unusable for mixed CAN frame types. Let's try the FIFO stuff with the next M_CAN revision. It's a bit of a SJA1000 for CAN FD :-) Regards, Oliver
diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c index 6160b9c..664fe30 100644 --- a/drivers/net/can/m_can/m_can.c +++ b/drivers/net/can/m_can/m_can.c @@ -105,14 +105,36 @@ enum m_can_mram_cfg { MRAM_CFG_NUM, }; +/* Fast Bit Timing & Prescaler Register (FBTP) */ +#define FBTR_FBRP_MASK 0x1f +#define FBTR_FBRP_SHIFT 16 +#define FBTR_FTSEG1_SHIFT 8 +#define FBTR_FTSEG1_MASK (0xf << FBTR_FTSEG1_SHIFT) +#define FBTR_FTSEG2_SHIFT 4 +#define FBTR_FTSEG2_MASK (0x7 << FBTR_FTSEG2_SHIFT) +#define FBTR_FSJW_SHIFT 0 +#define FBTR_FSJW_MASK 0x3 + /* Test Register (TEST) */ #define TEST_LBCK BIT(4) /* CC Control Register(CCCR) */ -#define CCCR_TEST BIT(7) -#define CCCR_MON BIT(5) -#define CCCR_CCE BIT(1) -#define CCCR_INIT BIT(0) +#define CCCR_TEST BIT(7) +#define CCCR_CMR_MASK 0x3 +#define CCCR_CMR_SHIFT 10 +#define CCCR_CMR_CANFD 0x1 +#define CCCR_CMR_CANFD_BRS 0x2 +#define CCCR_CMR_CAN 0x3 +#define CCCR_CME_MASK 0x3 +#define CCCR_CME_SHIFT 8 +#define CCCR_CME_CAN 0 +#define CCCR_CME_CANFD 0x1 +#define CCCR_CME_CANFD_BRS 0x2 +#define CCCR_TEST BIT(7) +#define CCCR_MON BIT(5) +#define CCCR_CCE BIT(1) +#define CCCR_INIT BIT(0) +#define CCCR_CANFD 0x10 /* Bit Timing & Prescaler Register (BTP) */ #define BTR_BRP_MASK 0x3ff @@ -204,6 +226,7 @@ enum m_can_mram_cfg { /* Rx Buffer / FIFO Element Size Configuration (RXESC) */ #define M_CAN_RXESC_8BYTES 0x0 +#define M_CAN_RXESC_64BYTES 0x777 /* Tx Buffer Configuration(TXBC) */ #define TXBC_NDTB_OFF 16 @@ -211,6 +234,7 @@ enum m_can_mram_cfg { /* Tx Buffer Element Size Configuration(TXESC) */ #define TXESC_TBDS_8BYTES 0x0 +#define TXESC_TBDS_64BYTES 0x7 /* Tx Event FIFO Con.guration (TXEFC) */ #define TXEFC_EFS_OFF 16 @@ -219,11 +243,11 @@ enum m_can_mram_cfg { /* Message RAM Configuration (in bytes) */ #define SIDF_ELEMENT_SIZE 4 #define XIDF_ELEMENT_SIZE 8 -#define RXF0_ELEMENT_SIZE 16 -#define RXF1_ELEMENT_SIZE 16 +#define RXF0_ELEMENT_SIZE 72 +#define RXF1_ELEMENT_SIZE 72 #define RXB_ELEMENT_SIZE 16 #define TXE_ELEMENT_SIZE 8 -#define TXB_ELEMENT_SIZE 16 +#define TXB_ELEMENT_SIZE 72 /* Message RAM Elements */ #define M_CAN_FIFO_ID 0x0 @@ -231,11 +255,17 @@ enum m_can_mram_cfg { #define M_CAN_FIFO_DATA(n) (0x8 + ((n) << 2)) /* Rx Buffer Element */ +/* R0 */ #define RX_BUF_ESI BIT(31) #define RX_BUF_XTD BIT(30) #define RX_BUF_RTR BIT(29) +/* R1 */ +#define RX_BUF_ANMF BIT(31) +#define RX_BUF_EDL BIT(21) +#define RX_BUF_BRS BIT(20) /* Tx Buffer Element */ +/* R0 */ #define TX_BUF_XTD BIT(30) #define TX_BUF_RTR BIT(29) @@ -327,41 +357,65 @@ static inline void m_can_disable_all_interrupts(const struct m_can_priv *priv) m_can_write(priv, M_CAN_ILE, 0x0); } -static void m_can_read_fifo(const struct net_device *dev, struct can_frame *cf, - u32 rxfs) +static void m_can_read_fifo(struct net_device *dev, u32 rxfs) { + struct net_device_stats *stats = &dev->stats; struct m_can_priv *priv = netdev_priv(dev); - u32 id, fgi; + struct canfd_frame *cf; + struct sk_buff *skb; + u32 id, fgi, dlc; + int i; /* calculate the fifo get index for where to read data */ fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF; + dlc = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC); + if (dlc & RX_BUF_EDL) + skb = alloc_canfd_skb(dev, &cf); + else + skb = alloc_can_skb(dev, (struct can_frame **)&cf); + if (!skb) { + stats->rx_dropped++; + return; + } + id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_ID); if (id & RX_BUF_XTD) cf->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG; else cf->can_id = (id >> 18) & CAN_SFF_MASK; - if (id & RX_BUF_RTR) { + if (id & RX_BUF_ESI) { + cf->flags |= CANFD_ESI; + netdev_dbg(dev, "ESI Error\n"); + } + + if (!(dlc & RX_BUF_EDL) && (id & RX_BUF_RTR)) { cf->can_id |= CAN_RTR_FLAG; } else { id = m_can_fifo_read(priv, fgi, M_CAN_FIFO_DLC); - cf->can_dlc = get_can_dlc((id >> 16) & 0x0F); - *(u32 *)(cf->data + 0) = m_can_fifo_read(priv, fgi, - M_CAN_FIFO_DATA(0)); - *(u32 *)(cf->data + 4) = m_can_fifo_read(priv, fgi, - M_CAN_FIFO_DATA(1)); + cf->len = can_dlc2len(get_canfd_dlc((id >> 16) & 0x0F)); + + if (id & RX_BUF_BRS) + cf->flags |= CANFD_BRS; + + for (i = 0; i < cf->len; i += 4) + *(u32 *)(cf->data + i) = + m_can_fifo_read(priv, fgi, + M_CAN_FIFO_DATA(i / 4)); } /* acknowledge rx fifo 0 */ m_can_write(priv, M_CAN_RXF0A, fgi); + + stats->rx_packets++; + stats->rx_bytes += cf->len; + + netif_receive_skb(skb); } static int m_can_do_rx_poll(struct net_device *dev, int quota) { struct m_can_priv *priv = netdev_priv(dev); - struct net_device_stats *stats = &dev->stats; - struct sk_buff *skb; - struct can_frame *frame; u32 pkts = 0; u32 rxfs; @@ -375,18 +429,7 @@ static int m_can_do_rx_poll(struct net_device *dev, int quota) if (rxfs & RXFS_RFL) netdev_warn(dev, "Rx FIFO 0 Message Lost\n"); - skb = alloc_can_skb(dev, &frame); - if (!skb) { - stats->rx_dropped++; - return pkts; - } - - m_can_read_fifo(dev, frame, rxfs); - - stats->rx_packets++; - stats->rx_bytes += frame->can_dlc; - - netif_receive_skb(skb); + m_can_read_fifo(dev, rxfs); quota--; pkts++; @@ -744,10 +787,23 @@ static const struct can_bittiming_const m_can_bittiming_const = { .brp_inc = 1, }; +static const struct can_bittiming_const m_can_data_bittiming_const = { + .name = KBUILD_MODNAME, + .tseg1_min = 2, /* Time segment 1 = prop_seg + phase_seg1 */ + .tseg1_max = 16, + .tseg2_min = 1, /* Time segment 2 = phase_seg2 */ + .tseg2_max = 8, + .sjw_max = 4, + .brp_min = 1, + .brp_max = 32, + .brp_inc = 1, +}; + static int m_can_set_bittiming(struct net_device *dev) { struct m_can_priv *priv = netdev_priv(dev); const struct can_bittiming *bt = &priv->can.bittiming; + const struct can_bittiming *dbt = &priv->can.data_bittiming; u16 brp, sjw, tseg1, tseg2; u32 reg_btp; @@ -758,7 +814,17 @@ static int m_can_set_bittiming(struct net_device *dev) reg_btp = (brp << BTR_BRP_SHIFT) | (sjw << BTR_SJW_SHIFT) | (tseg1 << BTR_TSEG1_SHIFT) | (tseg2 << BTR_TSEG2_SHIFT); m_can_write(priv, M_CAN_BTP, reg_btp); - netdev_dbg(dev, "setting BTP 0x%x\n", reg_btp); + + if (priv->can.ctrlmode & CAN_CTRLMODE_FD) { + brp = dbt->brp - 1; + sjw = dbt->sjw - 1; + tseg1 = dbt->prop_seg + dbt->phase_seg1 - 1; + tseg2 = dbt->phase_seg2 - 1; + reg_btp = (brp << FBTR_FBRP_SHIFT) | (sjw << FBTR_FSJW_SHIFT) | + (tseg1 << FBTR_FTSEG1_SHIFT) | + (tseg2 << FBTR_FTSEG2_SHIFT); + m_can_write(priv, M_CAN_FBTP, reg_btp); + } return 0; } @@ -778,8 +844,8 @@ static void m_can_chip_config(struct net_device *dev) m_can_config_endisable(priv, true); - /* RX Buffer/FIFO Element Size 8 bytes data field */ - m_can_write(priv, M_CAN_RXESC, M_CAN_RXESC_8BYTES); + /* RX Buffer/FIFO Element Size 64 bytes data field */ + m_can_write(priv, M_CAN_RXESC, M_CAN_RXESC_64BYTES); /* Accept Non-matching Frames Into FIFO 0 */ m_can_write(priv, M_CAN_GFC, 0x0); @@ -788,8 +854,8 @@ static void m_can_chip_config(struct net_device *dev) m_can_write(priv, M_CAN_TXBC, (1 << TXBC_NDTB_OFF) | priv->mcfg[MRAM_TXB].off); - /* only support 8 bytes firstly */ - m_can_write(priv, M_CAN_TXESC, TXESC_TBDS_8BYTES); + /* support 64 bytes payload */ + m_can_write(priv, M_CAN_TXESC, TXESC_TBDS_64BYTES); m_can_write(priv, M_CAN_TXEFC, (1 << TXEFC_EFS_OFF) | priv->mcfg[MRAM_TXE].off); @@ -804,7 +870,8 @@ static void m_can_chip_config(struct net_device *dev) RXFC_FWM_1 | priv->mcfg[MRAM_RXF1].off); cccr = m_can_read(priv, M_CAN_CCCR); - cccr &= ~(CCCR_TEST | CCCR_MON); + cccr &= ~(CCCR_TEST | CCCR_MON | (CCCR_CMR_MASK << CCCR_CMR_SHIFT) | + (CCCR_CME_MASK << CCCR_CME_SHIFT)); test = m_can_read(priv, M_CAN_TEST); test &= ~TEST_LBCK; @@ -816,6 +883,9 @@ static void m_can_chip_config(struct net_device *dev) test |= TEST_LBCK; } + if (priv->can.ctrlmode & CAN_CTRLMODE_FD) + cccr |= CCCR_CME_CANFD_BRS << CCCR_CME_SHIFT; + m_can_write(priv, M_CAN_CCCR, cccr); m_can_write(priv, M_CAN_TEST, test); @@ -880,11 +950,13 @@ static struct net_device *alloc_m_can_dev(void) priv->dev = dev; priv->can.bittiming_const = &m_can_bittiming_const; + priv->can.data_bittiming_const = &m_can_data_bittiming_const; priv->can.do_set_mode = m_can_set_mode; priv->can.do_get_berr_counter = m_can_get_berr_counter; priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_LISTENONLY | - CAN_CTRLMODE_BERR_REPORTING; + CAN_CTRLMODE_BERR_REPORTING | + CAN_CTRLMODE_FD; return dev; } @@ -967,8 +1039,9 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb, struct net_device *dev) { struct m_can_priv *priv = netdev_priv(dev); - struct can_frame *cf = (struct can_frame *)skb->data; - u32 id; + struct canfd_frame *cf = (struct canfd_frame *)skb->data; + u32 id, cccr; + int i; if (can_dropped_invalid_skb(dev, skb)) return NETDEV_TX_OK; @@ -987,11 +1060,24 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb, /* message ram configuration */ m_can_fifo_write(priv, 0, M_CAN_FIFO_ID, id); - m_can_fifo_write(priv, 0, M_CAN_FIFO_DLC, cf->can_dlc << 16); - m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(0), *(u32 *)(cf->data + 0)); - m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(1), *(u32 *)(cf->data + 4)); + m_can_fifo_write(priv, 0, M_CAN_FIFO_DLC, can_len2dlc(cf->len) << 16); + + for (i = 0; i < cf->len; i += 4) + m_can_fifo_write(priv, 0, M_CAN_FIFO_DATA(i / 4), + *(u32 *)(cf->data + i)); + can_put_echo_skb(skb, dev, 0); + if (priv->can.ctrlmode & CAN_CTRLMODE_FD) { + cccr = m_can_read(priv, M_CAN_CCCR); + cccr &= ~(CCCR_CMR_MASK << CCCR_CMR_SHIFT); + if (cf->flags & CANFD_BRS) + cccr |= CCCR_CMR_CANFD_BRS << CCCR_CMR_SHIFT; + else + cccr |= CCCR_CMR_CANFD << CCCR_CMR_SHIFT; + m_can_write(priv, M_CAN_CCCR, cccr); + } + /* enable first TX buffer to start transfer */ m_can_write(priv, M_CAN_TXBTIE, 0x1); m_can_write(priv, M_CAN_TXBAR, 0x1);
Bosch M_CAN is CAN FD capable device. This patch implements the CAN FD features include up to 64 bytes payload and bitrate switch function. 1) Change the Rx FIFO and Tx Buffer to 64 bytes for support CAN FD up to 64 bytes payload. It's backward compatible with old 8 bytes normal CAN frame. 2) Allocate can frame or canfd frame based on EDL bit 3) Bitrate Switch function is disabled by default and will be enabled according to CANFD_BRS bit in cf->flags. Signed-off-by: Dong Aisheng <b29396@freescale.com> --- Changes Since V1: * Allocate can frame or canfd frame based on EDL bit * Only check and set RTR bit for normal frame (no EDL bit set) --- drivers/net/can/m_can/m_can.c | 172 +++++++++++++++++++++++++++++++----------- 1 file changed, 129 insertions(+), 43 deletions(-)