Message-ID: <20250527-benevolent-rainbow-sturgeon-55c33b-mkl@pengutronix.de>
Date: Tue, 27 May 2025 13:00:10 +0200
From: Marc Kleine-Budde <mkl@...gutronix.de>
To: Elaine Zhang <zhangqing@...k-chips.com>
Cc: kernel@...gutronix.de, mailhol.vincent@...adoo.fr, robh@...nel.org, 
	krzk+dt@...nel.org, conor+dt@...nel.org, heiko@...ech.de, cl@...k-chips.com, 
	kever.yang@...k-chips.com, linux-can@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v6 4/4] net: can: rockchip: support dma for rk3576 rx

On 26.05.2025 14:25:59, Elaine Zhang wrote:
> The new can controller of rk3576 supports rx dma.

I'm missing dma_sync_single_for_cpu() and dma_sync_single_for_device().

What does the overall picture look like?

The CAN controller receives a CAN frame, it triggers an interrupt, the
driver starts a RX-DMA, the RX-DMA finishes triggers the callback, the
driver allocates an skb and copies the data from the DMA dest memory to
the skb. Finally the skb is passed to the networking stack.

Have you done any measurements if using DMA brings any benefits here? I
doubt that.

If your hardware supports, a better setup would be to allocate a bunch
of skbs and setup the a DMA request per skb and push them all to the DMA
engine during open(). If the CAN controller receives a CAN frame, the
DMA automatically starts and raises an IRQ after finish of the DMA
transfer. Transform the header and then pass the skb to the networking
stack - no need for memcpy.

This _might_ lower the total CPU cost of RX, but a lot of additional
code is used for that.

Never the less, review inline.

> Signed-off-by: Elaine Zhang <zhangqing@...k-chips.com>
> ---
>  .../net/can/rockchip/rockchip_canfd-core.c    | 39 +++++++++
>  drivers/net/can/rockchip/rockchip_canfd-rx.c  | 87 +++++++++++++++++++
>  drivers/net/can/rockchip/rockchip_canfd.h     | 11 +++
>  3 files changed, 137 insertions(+)
> 
> diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
> index 92e260cb2527..9ef4a9ae19d8 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-core.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
> @@ -433,6 +433,9 @@ static void rk3576canfd_chip_start(struct rkcanfd_priv *priv)
>  		      RK3576CANFD_REG_BRS_CFG_BRS_NEGSYNC_EN |
>  		      RK3576CANFD_REG_BRS_CFG_BRS_POSSYNC_EN);
>  
> +	if (priv->use_dma)
> +		rkcanfd_write(priv, RK3576CANFD_REG_DMA_CTRL,
> +			      RK3576CANFD_REG_DMA_CTRL_DMA_RX_EN | priv->dma_thr);
>  	rkcanfd_set_bittiming(priv);
>  
>  	priv->devtype_data.interrupts_disable(priv);
> @@ -1324,10 +1327,31 @@ static const struct of_device_id rkcanfd_of_match[] = {
>  };
>  MODULE_DEVICE_TABLE(of, rkcanfd_of_match);
>  
> +static void rk3576_canfd_dma_init(struct rkcanfd_priv *priv)
> +{
> +	struct dma_slave_config rxconf = {
> +		.direction = DMA_DEV_TO_MEM,
> +		.src_addr = priv->rx_dma_src_addr,
> +		.src_addr_width = 4,
> +		.dst_addr_width = 4,
> +		.src_maxburst = 9,
> +	};
> +
> +	priv->dma_thr = rxconf.src_maxburst - 1;
> +	priv->rxbuf = dma_alloc_coherent(priv->dev, priv->dma_size * 14,

Where does the 14 come from?

> +					 &priv->rx_dma_dst_addr, GFP_KERNEL);

I'm missing the cleanup for priv->rxbuf.

> +	if (!priv->rxbuf) {
> +		priv->use_dma = 0;
> +		return;
> +	}
> +	dmaengine_slave_config(priv->rxchan, &rxconf);

Please add error handling.

> +}
> +
>  static int rkcanfd_probe(struct platform_device *pdev)
>  {
>  	struct rkcanfd_priv *priv;
>  	struct net_device *ndev;
> +	struct resource *res;
>  	const void *match;
>  	int err;
>  
> @@ -1349,6 +1373,7 @@ static int rkcanfd_probe(struct platform_device *pdev)
>  		goto out_free_candev;
>  	}
>  
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	priv->regs = devm_platform_ioremap_resource(pdev, 0);

Please use devm_platform_get_and_ioremap_resource();

>  	if (IS_ERR(priv->regs)) {
>  		err = PTR_ERR(priv->regs);
> @@ -1376,6 +1401,7 @@ static int rkcanfd_probe(struct platform_device *pdev)
>  	priv->can.do_set_mode = rkcanfd_set_mode;
>  	priv->can.do_get_berr_counter = rkcanfd_get_berr_counter;
>  	priv->ndev = ndev;
> +	priv->dev = &pdev->dev;

Please remove, priv->dev is not used in the hot path, use
priv->ndev->dev.parent instead.

>  
>  	match = device_get_match_data(&pdev->dev);
>  	if (match) {
> @@ -1384,6 +1410,19 @@ static int rkcanfd_probe(struct platform_device *pdev)
>  			priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD;
>  	}
>  
> +	priv->rxchan = dma_request_chan(&pdev->dev, "rx");

I'm missing the cleanup for priv->rxchan.

> +	if (IS_ERR(priv->rxchan)) {

Please handle -EPROBE_DEFER properly.

> +		dev_warn(&pdev->dev, "Failed to request rxchan\n");

Please print the error value and state that you are continuing w/o DMA,
e.g.:

    netdev_warn("Failed to request RX-DMA channel: %pe, continuing without DMA", priv->rxchan);

> +		priv->rxchan = NULL;
> +		priv->use_dma = 0;
> +	} else {
> +		priv->rx_dma_src_addr = res->start + RK3576CANFD_REG_RXFRD;
> +		priv->dma_size = RK3576CANFD_REG_STR_STATE_INTM_LEFT_CNT_UNIT * 4;

Why do you need priv->dma_size, it doesn't change during runtime, does it?

> +		priv->use_dma = 1;
> +	}
> +	if (priv->use_dma)
> +		rk3576_canfd_dma_init(priv);
> +

Can you move all DMA related functionality into rk3576_canfd_dma_init(),
pass res as a parameter, remove rx_dma_src_addr.

I think you don't need use_dma, checking rxchan != NULL should be
sufficient.

>  	err = can_rx_offload_add_manual(ndev, &priv->offload,
>  					RKCANFD_NAPI_WEIGHT);
>  	if (err)
> diff --git a/drivers/net/can/rockchip/rockchip_canfd-rx.c b/drivers/net/can/rockchip/rockchip_canfd-rx.c
> index 8a383cabd9d2..ac06e876552e 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-rx.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-rx.c
> @@ -285,6 +285,52 @@ static int rk3576canfd_handle_rx_int_one(struct rkcanfd_priv *priv)
>  	return 0;
>  }
>  
> +static int rk3576canfd_handle_rx_dma(struct rkcanfd_priv *priv, u32 addr)
> +{
> +	struct net_device_stats *stats = &priv->ndev->stats;
> +	struct canfd_frame cfd[1] = { }, *skb_cfd;
> +	struct rk3576canfd_fifo_header header[1] = { };
> +	struct sk_buff *skb;
> +	u32 __iomem *rxbuf = (u32 __iomem *)priv->rxbuf;

Why do you cast rx_buf to __iomem? It's normal memory, isn't it?

> +	unsigned int len;
> +	int i;
> +
> +	header->frameinfo = readl(rxbuf +
> +				  addr * RK3576CANFD_REG_STR_STATE_INTM_LEFT_CNT_UNIT);
> +	header->id = readl(rxbuf + 1 +
> +			   addr * RK3576CANFD_REG_STR_STATE_INTM_LEFT_CNT_UNIT);
> +	for (i = 0; i < (RK3576CANFD_REG_STR_STATE_INTM_LEFT_CNT_UNIT - 2); i++)
> +		cfd->data[i] = readl(rxbuf + 2 + i +
> +				     addr * RK3576CANFD_REG_STR_STATE_INTM_LEFT_CNT_UNIT);
> +

Remove cfd and header and directly access the rxbuf.

> +	len = rk3576canfd_fifo_header_to_cfd_header(priv, header, cfd);
> +
> +	/* Manual handling of CAN Bus Error counters. See
> +	 * rkcanfd_get_corrected_berr_counter() for detailed
> +	 * explanation.
> +	 */
> +	if (priv->bec.rxerr)
> +		priv->bec.rxerr = min(CAN_ERROR_PASSIVE_THRESHOLD,
> +				      priv->bec.rxerr) - 1;
> +
> +	if (header->frameinfo & RK3576CANFD_REG_RXFRD_FRAMEINFO_FDF)
> +		skb = alloc_canfd_skb(priv->ndev, &skb_cfd);
> +	else
> +		skb = alloc_can_skb(priv->ndev, (struct can_frame **)&skb_cfd);

copy the data to the allocated skb directly.

> +
> +	if (!skb) {
> +		stats->rx_dropped++;
> +
> +		return 0;
> +	}
> +
> +	memcpy(skb_cfd, cfd, len);
> +	stats->rx_packets++;
> +	stats->rx_bytes += cfd->len;
> +	netif_rx(skb);

Use the rx_offload_helper here too.

> +	return 0;
> +}
> +
>  static int rkcanfd_handle_rx_int_one(struct rkcanfd_priv *priv)
>  {
>  	struct net_device_stats *stats = &priv->ndev->stats;
> @@ -380,6 +426,43 @@ rk3576canfd_rx_fifo_get_len(const struct rkcanfd_priv *priv)
>  	return DIV_ROUND_UP(val, RK3576CANFD_REG_STR_STATE_INTM_LEFT_CNT_UNIT);
>  }
>  
> +static void rk3576_canfd_rx_dma_callback(void *data)
> +{
> +	struct rkcanfd_priv *priv = data;
> +	int i;
> +
> +	for (i = 0; i < priv->quota; i++)
> +		rk3576canfd_handle_rx_dma(priv, i);
> +
> +	rkcanfd_write(priv, RK3576CANFD_REG_INT_MASK, priv->reg_int_mask_default);
> +}
> +
> +static int rk3576_canfd_rx_dma(struct rkcanfd_priv *priv)
> +{
> +	struct dma_async_tx_descriptor *rxdesc = NULL;
> +	const u32 reg = rkcanfd_read(priv, RK3576CANFD_REG_STR_STATE);
> +	int quota = FIELD_GET(RK3576CANFD_REG_STR_STATE_INTM_LEFT_CNT, reg);
> +
> +	quota = DIV_ROUND_UP(quota, RK3576CANFD_REG_STR_STATE_INTM_LEFT_CNT_UNIT);
> +	priv->quota = quota;
> +	if (priv->quota == 0) {
> +		rkcanfd_write(priv, RK3576CANFD_REG_INT_MASK, priv->reg_int_mask_default);
> +		return 0;
> +	}
> +
> +	rxdesc = dmaengine_prep_slave_single(priv->rxchan, priv->rx_dma_dst_addr,
> +					     priv->dma_size * priv->quota, DMA_DEV_TO_MEM, 0);
> +	if (!rxdesc)
> +		return -ENOMSG;
> +
> +	rxdesc->callback = rk3576_canfd_rx_dma_callback;
> +	rxdesc->callback_param = priv;
> +
> +	dmaengine_submit(rxdesc);

Please add error handling.

> +	dma_async_issue_pending(priv->rxchan);
> +	return 0;
> +}
> +
>  int rkcanfd_handle_rx_int(struct rkcanfd_priv *priv)
>  {
>  	unsigned int len;
> @@ -399,6 +482,10 @@ int rkcanfd_handle_rk3576_rx_int(struct rkcanfd_priv *priv)
>  	unsigned int len;
>  	int err;
>  
> +	if (priv->use_dma) {
> +		rk3576_canfd_rx_dma(priv);
> +		return 0;

Please add error handling.

> +	}
>  	while ((len = rk3576canfd_rx_fifo_get_len(priv))) {
>  		err = rk3576canfd_handle_rx_int_one(priv);
>  		if (err)
> diff --git a/drivers/net/can/rockchip/rockchip_canfd.h b/drivers/net/can/rockchip/rockchip_canfd.h
> index 9b91d757d054..c7d6845c6d95 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd.h
> +++ b/drivers/net/can/rockchip/rockchip_canfd.h
> @@ -11,6 +11,8 @@
>  #include <linux/can/dev.h>
>  #include <linux/can/rx-offload.h>
>  #include <linux/clk.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dmaengine.h>
>  #include <linux/io.h>
>  #include <linux/netdevice.h>
>  #include <linux/reset.h>
> @@ -737,6 +739,7 @@ struct rkcanfd_priv {
>  	struct can_priv can;
>  	struct can_rx_offload offload;
>  	struct net_device *ndev;
> +	struct device *dev;

It's not used in the hot path, please remove.

>  
>  	void __iomem *regs;
>  	unsigned int tx_head;
> @@ -758,6 +761,14 @@ struct rkcanfd_priv {
>  	struct reset_control *reset;
>  	struct clk_bulk_data *clks;
>  	int clks_num;
> +	bool use_dma;
> +	u32 dma_size;
> +	u32 dma_thr;
> +	int quota;
> +	struct dma_chan *rxchan;
> +	u32 *rxbuf;
> +	dma_addr_t rx_dma_src_addr;
> +	dma_addr_t rx_dma_dst_addr;
>  };
>  
>  static inline u32

I'll look at patch 3/4 later this week or next week.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists