Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752657AbdCFC1t (ORCPT ); Sun, 5 Mar 2017 21:27:49 -0500 Received: from smtpout.microchip.com ([198.175.253.82]:44093 "EHLO email.microchip.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752533AbdCFC1r (ORCPT ); Sun, 5 Mar 2017 21:27:47 -0500 From: To: , , CC: , , , , , , Subject: RE: [PATCH] can: m_can: support transmit frame in CAN FD format Thread-Topic: [PATCH] can: m_can: support transmit frame in CAN FD format Thread-Index: AQHSk+CKaxQM/ggW5kuqE5PKh7/t36GHHImA///2wVA= Date: Mon, 6 Mar 2017 02:27:45 +0000 Message-ID: References: <20170303053331.20883-1-wenyou.yang@atmel.com> <12c3ae1d-65f7-8ba2-61e7-eed5291ebd21@hartkopp.net> In-Reply-To: <12c3ae1d-65f7-8ba2-61e7-eed5291ebd21@hartkopp.net> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.10.76.4] Content-Type: text/plain; charset="gb2312" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id v262Rsld011457 Content-Length: 4626 Lines: 152 Hi Oliver, Thank you for your review. > -----Original Message----- > From: Oliver Hartkopp [mailto:socketcan@hartkopp.net] > Sent: 2017??3??6?? 3:34 > To: Wenyou Yang - A41535 ; Wolfgang > Grandegger ; Marc Kleine-Budde > Cc: Alexandre Belloni ; Florian Fainelli > ; Quentin Schulz ; > Wenyou Yang - A41535 ; Nicolas Ferre > ; linux-can@vger.kernel.org; netdev@vger.kernel.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH] can: m_can: support transmit frame in CAN FD format > > The patch has some issues: > > On 03/03/2017 06:33 AM, Wenyou Yang wrote: > > Add support to transmit the frame in the CAN FD format and with the > > bit rate switching. > > This is a misleading comment. > > "can: m_can: support transmit frame in CAN FD format" > is misleading too. You were able to send CAN FD frames before. > > This patch enables the transmission of CAN FD frames on M_CAN IP cores >= > v3.1.x, right? The revision of M_CAN IP on SAMA5D2 is 3.1.0 (CREL = 0x31040730). > > So this should be mentioned properly. > > > Tested on SAMA5D2 Xplained board. > > Tested on which M_CAN IP core revision would be useful here. > > > Signed-off-by: Wenyou Yang > > --- > > The testing is based on > > [RESEND PATCH 1/1] can: m_can: fix bitrate setup on latest silicon > > http://lkml.iu.edu/hypermail/linux/kernel/1702.1/05347.html > > > > drivers/net/can/m_can/m_can.c | 21 +++++++++++++++++---- > > 1 file changed, 17 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/net/can/m_can/m_can.c > > b/drivers/net/can/m_can/m_can.c index 195f15edb32e..9ef9b337d25b > > 100644 > > --- a/drivers/net/can/m_can/m_can.c > > +++ b/drivers/net/can/m_can/m_can.c > > @@ -266,8 +266,12 @@ enum m_can_mram_cfg { > > > > /* Tx Buffer Element */ > > /* R0 */ > > +#define TX_BUF_ESI BIT(31) > > #define TX_BUF_XTD BIT(30) > > #define TX_BUF_RTR BIT(29) > > +#define TX_BUF_EFC BIT(23) > > +#define TX_BUF_EDL BIT(21) > > This bit is named FDF (FD Format bit). > > It has to be: > > #define TX_BUF_FDF BIT(21) > > > +#define TX_BUF_BRS BIT(20) > > > > /* address offset and element number for each FIFO/Buffer in the > > Message RAM */ struct mram_cfg { @@ -884,7 +888,7 @@ static void > > m_can_chip_config(struct net_device *dev) > > } > > > > if (priv->can.ctrlmode & CAN_CTRLMODE_FD) > > - cccr |= CCCR_CME_CANFD_BRS << CCCR_CME_SHIFT; > > + cccr |= (CCCR_CME_CANFD_BRS | CCCR_CME_CANFD) << > CCCR_CME_SHIFT; > > > > m_can_write(priv, M_CAN_CCCR, cccr); > > m_can_write(priv, M_CAN_TEST, test); @@ -1047,6 +1051,7 @@ static > > netdev_tx_t m_can_start_xmit(struct sk_buff *skb, > > struct canfd_frame *cf = (struct canfd_frame *)skb->data; > > u32 id, cccr; > > int i; > > + u32 dlc; > > > > if (can_dropped_invalid_skb(dev, skb)) > > return NETDEV_TX_OK; > > @@ -1065,7 +1070,6 @@ 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, 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), @@ - > 1073,20 > > +1077,29 @@ static netdev_tx_t m_can_start_xmit(struct sk_buff *skb, > > > > can_put_echo_skb(skb, dev, 0); > > > > + dlc = can_len2dlc(cf->len) << 16; > > + > > if (priv->can.ctrlmode & CAN_CTRLMODE_FD) { > > cccr = m_can_read(priv, M_CAN_CCCR); > > cccr &= ~(CCCR_CMR_MASK << CCCR_CMR_SHIFT); > > if (can_is_canfd_skb(skb)) { > > - if (cf->flags & CANFD_BRS) > > + dlc |= TX_BUF_EDL; > > dito FDF > > > + if (cf->flags & CANFD_ESI) > > + dlc |= TX_BUF_ESI; > > + if (cf->flags & CANFD_BRS) { > > + dlc |= TX_BUF_BRS; > > cccr |= CCCR_CMR_CANFD_BRS << > CCCR_CMR_SHIFT; > > - else > > + } else { > > cccr |= CCCR_CMR_CANFD << > CCCR_CMR_SHIFT; > > + } > > } else { > > cccr |= CCCR_CMR_CAN << CCCR_CMR_SHIFT; > > } > > m_can_write(priv, M_CAN_CCCR, cccr); > > } > > > > + m_can_fifo_write(priv, 0, M_CAN_FIFO_DLC, dlc); > > + > > /* enable first TX buffer to start transfer */ > > m_can_write(priv, M_CAN_TXBTIE, 0x1); > > m_can_write(priv, M_CAN_TXBAR, 0x1); > > > > Beside the documentation and the wrong bit naming the patch looks ok. > Please resend. I will send a version 2. Thanks > > Regards, > Oliver Best Regards, Wenyou Yang