From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id CC79AC54EAA for ; Mon, 30 Jan 2023 08:04:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235533AbjA3IE0 (ORCPT ); Mon, 30 Jan 2023 03:04:26 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58142 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235314AbjA3IE0 (ORCPT ); Mon, 30 Jan 2023 03:04:26 -0500 Received: from mail-ed1-x52b.google.com (mail-ed1-x52b.google.com [IPv6:2a00:1450:4864:20::52b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 29720279A6 for ; Mon, 30 Jan 2023 00:04:23 -0800 (PST) Received: by mail-ed1-x52b.google.com with SMTP id q19so151914edd.2 for ; Mon, 30 Jan 2023 00:04:23 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20210112.gappssmtp.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=jSqYq94IziQ+GwHhxfEtIh9C+sIOzYwnbpL+9Sa+D6g=; b=zhglBq6bfoBAi2lZoEVbraO8PHMnQWwTAHAZCSi7U8Oc5A/AZgMo3p8pR21FZtnjFI BM+mozWKaD4/5047N/CFDup7OEzAFUV+lH7agDR9MaOsux9+X9xeFgUUqMg9CCnWIxvv oWaf6slopNBDYo4PSP5+TlXlhweEAyMfp2CR7Y4UQAXm8M8oqTiPnndTVsxjv/BMlFjN MmH+wnwUp6AFgrwdnarYh2SdfGXve8XPuscQgX5f7UMMnV/owVJ0wtyHA+U27Lfy9L6o ANalzT/5F+ybUAfoLrgj3Oqj7beXlJjB7yNyt6IAwJXBdiFvxVW+baTybtBcCSemCGab xE9w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=jSqYq94IziQ+GwHhxfEtIh9C+sIOzYwnbpL+9Sa+D6g=; b=8DbjvtGQdL2sq57p5SZohi9ttR0Wlz9v5aIJ+mGOJRa7ZQY1DkwnSuEmZgrV5jbxDU XqyLUKBgeD7bcsgAHVk/nluqORm4yRzrMe6ZV9EA/hCHHUADPMCkcKOzuB6CfpcrT0FH 6EjfAZ4mNWw45L+wk/opRvg3EPm4nxxXa2ZdFgTr8vz7FjYJOWF36NKeZSkRyOhLNKO4 +H+3H+d0Ilg7FxnQ16vbYxXU8PYHQfGTkbwQQBPpubZeAHS6y43EY1p5MvwUvaTi8nIN JpFao+M1NJp/wwYOqju5UYv/NmuZ3YWzw28dbS6UcfiBoJyYaZuxZu6dE3zBTRveYWvr HnHA== X-Gm-Message-State: AO0yUKXIhckhCqwTiigxt/GVZrhtNkHMYuDRo2EyRrlOq1hnYBCDwIrk RhbIHFQoAu4JxyKy5MfOYKzIpw== X-Google-Smtp-Source: AK7set+cYVCENCwEm1hET+u41SxqRExvo79clTCj9hVoH4l/i9hXms0u8pq9XPXi1dlZc2QxBHeeAw== X-Received: by 2002:aa7:cf89:0:b0:49c:9999:600e with SMTP id z9-20020aa7cf89000000b0049c9999600emr5570934edx.11.1675065861267; Mon, 30 Jan 2023 00:04:21 -0800 (PST) Received: from blmsp ([2001:4091:a247:815f:d5ca:514b:67d4:fc9f]) by smtp.gmail.com with ESMTPSA id n14-20020a056402060e00b00499703df898sm6359296edv.69.2023.01.30.00.04.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 30 Jan 2023 00:04:20 -0800 (PST) Date: Mon, 30 Jan 2023 09:04:19 +0100 From: Markus Schneider-Pargmann To: Simon Horman Cc: Marc Kleine-Budde , Chandrasekar Ramakrishnan , Wolfgang Grandegger , Vincent MAILHOL , linux-can@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 08/18] can: m_can: Write transmit header and data in one transaction Message-ID: <20230130080419.dzdnmq4vtag7wbpd@blmsp> References: <20230125195059.630377-1-msp@baylibre.com> <20230125195059.630377-9-msp@baylibre.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-can@vger.kernel.org Hi Simon, On Thu, Jan 26, 2023 at 09:04:56AM +0100, Simon Horman wrote: > On Wed, Jan 25, 2023 at 08:50:49PM +0100, Markus Schneider-Pargmann wrote: > > Combine header and data before writing to the transmit fifo to reduce > > the overhead for peripheral chips. > > > > Signed-off-by: Markus Schneider-Pargmann > > --- > > drivers/net/can/m_can/m_can.c | 10 +++++----- > > 1 file changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c > > index 78f6ed744c36..440bc0536951 100644 > > --- a/drivers/net/can/m_can/m_can.c > > +++ b/drivers/net/can/m_can/m_can.c > > @@ -1681,6 +1681,7 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev) > > m_can_write(cdev, M_CAN_TXBAR, 0x1); > > /* End of xmit function for version 3.0.x */ > > } else { > > + char buf[TXB_ELEMENT_SIZE]; > > /* Transmit routine for version >= v3.1.x */ > > > > txfqs = m_can_read(cdev, M_CAN_TXFQS); > > @@ -1720,12 +1721,11 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev) > > fifo_header.dlc = FIELD_PREP(TX_BUF_MM_MASK, putidx) | > > FIELD_PREP(TX_BUF_DLC_MASK, can_fd_len2dlc(cf->len)) | > > fdflags | TX_BUF_EFC; > > - err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID, &fifo_header, 2); > > - if (err) > > - goto out_fail; > > + memcpy(buf, &fifo_header, 8); > > + memcpy(&buf[8], &cf->data, cf->len); > > > > - err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_DATA, > > - cf->data, DIV_ROUND_UP(cf->len, 4)); > > + err = m_can_fifo_write(cdev, putidx, M_CAN_FIFO_ID, > > + buf, 8 + DIV_ROUND_UP(cf->len, 4)); > > Perhaps I am missing something here, but my reading is that: > > - 8 is a length in bytes > - the 5th argument to m_can_fifo_write is the val_count parameter, > whose unit is 4-byte long values. > > By this logic, perhaps the correct value for this argument is: > > DIV_ROUND_UP(8 + cf->len, 4) Thank you for spotting this. You are totally right, I will fix it for the next version. > > Also: > > - If cf->len is not a multiple of 4, is there a possibility > that uninitialised trailing data in buf will be used > indirectly by m_can_fifo_write()? Good point. I think this can only happen for 1, 2, 3, 5, 6, 7 bytes, values above have to be multiple of 4 because of the CAN-FD specification. With 'buf' it should read garbage from the buffer which I think is not a problem as the chip knows how much of the data to use. Also the tx elemnt size is hardcoded to 64 byte in the driver, so we do not overwrite the next element with that. The chip minimum size is 8 bytes for the data field anyways. So I think this is fine. Thanks, Markus > > > if (err) > > goto out_fail; > > > > -- > > 2.39.0 > >