Re: [PATCH v2 2/2] can: m_can: Batch FIFO writes during CAN transmit - Marc Kleine-Budde

From: Marc Kleine-Budde <mkl@pengutronix.de>
To: Matt Kline <matt@bitbashing.io>
Cc: linux-can@vger.kernel.org
Subject: Re: [PATCH v2 2/2] can: m_can: Batch FIFO writes during CAN transmit
Date: Wed, 11 Aug 2021 08:35:20 +0200	[thread overview]
Message-ID: <20210811063520.aw6hkll2kax22ytr@pengutronix.de> (raw)
In-Reply-To: <YRLl5PavqmjkIkeD@kline-desktop>

[-- Attachment #1: Type: text/plain, Size: 2959 bytes --]

On 10.08.2021 13:47:32, Matt Kline wrote:
> 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.

In the mcp251xfd (another SPI-CAN controller) driver I have the same
problem. I've basically implemented error checking everywhere. If there
is an error in the interrupt handler, I shut down the driver, see:

https://elixir.bootlin.com/linux/v5.13/source/drivers/net/can/spi/mcp251xfd/mcp251xfd-core.c#L2298

> > >  	/* 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!

A struct can easily extended to hold the data, too.

> > >  
> > >  	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.

Yes, but at least for the SPI this is a neglectable overhead.

> I wanted to start with this smaller, simpler patch first. Is that
> alright?

Fine with me!

> I'll try to send a v3 up shortly.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]