Re: [PATCH v3 2/3] can: m_can: Batch FIFO reads during CAN receive

[Aswath Govindraju <a-govindraju@ti.com>]

Hi Matt, Marc,

On 17/08/21 10:38 am, Matt Kline wrote:
> On peripherals communicating over a relatively slow SPI line
> (e.g. tcan4x5x), individual transfers have high fixed costs.
> This causes the driver to spend most of its time waiting between
> transfers and severely limits throughput.
> 
> Reduce these overheads by reading more than one word at a time.
> Writing could get a similar treatment in follow-on commits.
> 
> Signed-off-by: Matt Kline <matt@bitbashing.io>
> ---
>  drivers/net/can/m_can/m_can.c | 51 +++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/net/can/m_can/m_can.c b/drivers/net/can/m_can/m_can.c
> index 83eb5cd51de5..85d6cd03bff1 100644
> --- a/drivers/net/can/m_can/m_can.c
> +++ b/drivers/net/can/m_can/m_can.c
> @@ -308,6 +308,15 @@ enum m_can_reg {
>  #define TX_EVENT_MM_MASK	GENMASK(31, 24)
>  #define TX_EVENT_TXTS_MASK	GENMASK(15, 0)
>  
> +/* The ID and DLC registers are adjacent in M_CAN FIFO memory,
> + * and we can save a (potentially slow) bus round trip by combining
> + * reads and writes to them.
> + */
> +struct __packed id_and_dlc {
> +	u32 id;
> +	u32 dlc;
> +};
> +
>  static inline u32 m_can_read(struct m_can_classdev *cdev, enum m_can_reg reg)
>  {
>  	return cdev->ops->read_reg(cdev, reg);
> @@ -460,17 +469,18 @@ static int m_can_read_fifo(struct net_device *dev, u32 rxfs)
>  	struct m_can_classdev *cdev = netdev_priv(dev);
>  	struct canfd_frame *cf;
>  	struct sk_buff *skb;
> -	u32 id, fgi, dlc;
> +	struct id_and_dlc fifo_header;
> +	u32 fgi;
>  	u32 timestamp = 0;
> -	int i, err;
> +	int err;
>  
>  	/* calculate the fifo get index for where to read data */
>  	fgi = FIELD_GET(RXFS_FGI_MASK, rxfs);
> -	err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DLC, &dlc, 1);
> +	err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_ID, &fifo_header, 2);

While reading multiple register fields and calling iomap_read_fifo() in
m_can_platform.c is causing an issue.

In iomap_read_fifo(), ioread32_rep() is being used for reading.
ioread32_rep reads() from the same source address for val_count times.
This is not the intended behavior here. The source address also needs to
be shifted along with the destination address.

Is a fix required in iomap_read_fifo() ?

Thanks,
Aswath

>  	if (err)
>  		goto out_fail;
>  
> -	if (dlc & RX_BUF_FDF)
> +	if (fifo_header.dlc & RX_BUF_FDF)
>  		skb = alloc_canfd_skb(dev, &cf);
>  	else
>  		skb = alloc_can_skb(dev, (struct can_frame **)&cf);
> @@ -479,36 +489,31 @@ static int m_can_read_fifo(struct net_device *dev, u32 rxfs)
>  		return 0;
>  	}
>  
> -	if (dlc & RX_BUF_FDF)
> -		cf->len = can_fd_dlc2len((dlc >> 16) & 0x0F);
> +	if (fifo_header.dlc & RX_BUF_FDF)
> +		cf->len = can_fd_dlc2len((fifo_header.dlc >> 16) & 0x0F);
>  	else
> -		cf->len = can_cc_dlc2len((dlc >> 16) & 0x0F);
> +		cf->len = can_cc_dlc2len((fifo_header.dlc >> 16) & 0x0F);
>  
> -	err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_ID, &id, 1);
> -	if (err)
> -		goto out_fail;
> -
> -	if (id & RX_BUF_XTD)
> -		cf->can_id = (id & CAN_EFF_MASK) | CAN_EFF_FLAG;
> +	if (fifo_header.id & RX_BUF_XTD)
> +		cf->can_id = (fifo_header.id & CAN_EFF_MASK) | CAN_EFF_FLAG;
>  	else
> -		cf->can_id = (id >> 18) & CAN_SFF_MASK;
> +		cf->can_id = (fifo_header.id >> 18) & CAN_SFF_MASK;
>  
> -	if (id & RX_BUF_ESI) {
> +	if (fifo_header.id & RX_BUF_ESI) {
>  		cf->flags |= CANFD_ESI;
>  		netdev_dbg(dev, "ESI Error\n");
>  	}
>  
> -	if (!(dlc & RX_BUF_FDF) && (id & RX_BUF_RTR)) {
> +	if (!(fifo_header.dlc & RX_BUF_FDF) && (fifo_header.id & RX_BUF_RTR)) {
>  		cf->can_id |= CAN_RTR_FLAG;
>  	} else {
> -		if (dlc & RX_BUF_BRS)
> +		if (fifo_header.dlc & RX_BUF_BRS)
>  			cf->flags |= CANFD_BRS;
>  
> -		for (i = 0; i < cf->len; i += 4) {
> -			err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DATA(i / 4), cf->data + i, 1);
> -			if (err)
> -				goto out_fail;
> -		}
> +		err = m_can_fifo_read(cdev, fgi, M_CAN_FIFO_DATA(0),
> +				      cf->data, DIV_ROUND_UP(cf->len, 4));
> +		if (err)
> +			goto out_fail;
>  	}
>  
>  	/* acknowledge rx fifo 0 */
> @@ -517,7 +522,7 @@ static int m_can_read_fifo(struct net_device *dev, u32 rxfs)
>  	stats->rx_packets++;
>  	stats->rx_bytes += cf->len;
>  
> -	timestamp = FIELD_GET(RX_BUF_RXTS_MASK, dlc);
> +	timestamp = FIELD_GET(RX_BUF_RXTS_MASK, fifo_header.dlc);
>  
>  	m_can_receive_skb(cdev, skb, timestamp);
>  
>