From: Matt Kline <matt@bitbashing.io>
To: Marc Kleine-Budde <mkl@pengutronix.de>
Cc: linux-can@vger.kernel.org
Subject: Re: [PATCH v2 2/2] can: m_can: Batch FIFO writes during CAN transmit
Date: Tue, 10 Aug 2021 13:47:32 -0700 [thread overview]
Message-ID: <YRLl5PavqmjkIkeD@kline-desktop> (raw)
In-Reply-To: <20210804091858.vvvrzrmnmi76mg3c@pengutronix.de>
On Wed, Aug 04, 2021 at 11:18:58AM +0200, Marc Kleine-Budde wrote:
> >
> > - cdev->ops->write_fifo(cdev, addr_offset, val);
> > + result = cdev->ops->write_fifo(cdev, addr_offset, val, val_count);
> > + WARN_ON(result != 0);
>
> What about converting all read/write functions to return an error, and
> handle the error in the caller?
Yeah, that would be cleaner.
> > /* acknowledge rx fifo 0 */
> > @@ -1546,8 +1548,8 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
> > struct net_device *dev = cdev->net;
> > struct sk_buff *skb = cdev->tx_skb;
> > u32 id, cccr, fdflags;
> > - int i;
> > int putidx;
> > + u32 id_and_dlc[2];
>
> Can you create a struct for this?
Ditto, sure!
> >
> > cdev->tx_skb = NULL;
> >
> > @@ -1563,18 +1565,16 @@ static netdev_tx_t m_can_tx_handler(struct m_can_classdev *cdev)
> > if (cf->can_id & CAN_RTR_FLAG)
> > id |= TX_BUF_RTR;
> >
> > + id_and_dlc[0] = id;
> > +
> > if (cdev->version == 30) {
> > netif_stop_queue(dev);
> >
> > - /* message ram configuration */
> > - m_can_fifo_write(cdev, 0, M_CAN_FIFO_ID, id);
> > - m_can_fifo_write(cdev, 0, M_CAN_FIFO_DLC,
> > - can_fd_len2dlc(cf->len) << 16);
> > + id_and_dlc[1] = can_fd_len2dlc(cf->len) << 16;
> >
> > - for (i = 0; i < cf->len; i += 4)
> > - m_can_fifo_write(cdev, 0,
> > - M_CAN_FIFO_DATA(i / 4),
> > - *(u32 *)(cf->data + i));
> > + /* Write the frame ID, DLC, and payload to the FIFO element. */
> > + m_can_fifo_write(cdev, 0, M_CAN_FIFO_ID, id_and_dlc, ARRAY_SIZE(id_and_dlc));
> > + m_can_fifo_write(cdev, 0, M_CAN_FIFO_DATA, cf->data, DIV_ROUND_UP(cf->len, 4));
>
> Does it make sense to combine these, too? Same for the v3.1 variant.
I think that's the eventual goal, but since the ID, DLC, and frame data would
have to be contiguous for a single m_can_fifo_write(), you'd end up copying
things around. I wanted to start with this smaller, simpler patch first.
Is that alright?
I'll try to send a v3 up shortly.
Best,
Matt
next prev parent reply other threads:[~2021-08-10 20:47 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-27 1:58 [PATCH v2 0/2] can: m_can: Merge FIFO ops to increase throughput Matt Kline
2021-07-27 1:58 ` [PATCH v2 1/2] can: m_can: Batch FIFO reads during CAN receive Matt Kline
2021-07-27 1:58 ` [PATCH v2 2/2] can: m_can: Batch FIFO writes during CAN transmit Matt Kline
2021-08-04 9:18 ` Marc Kleine-Budde
2021-08-10 20:47 ` Matt Kline [this message]
2021-08-11 6:35 ` Marc Kleine-Budde
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YRLl5PavqmjkIkeD@kline-desktop \
--to=matt@bitbashing.io \
--cc=linux-can@vger.kernel.org \
--cc=mkl@pengutronix.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).