Re: [PATCH net-next v4 4/4] net: ocelot: add FDMA support

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Fri, Dec 03, 2021 at 06:19:16PM +0100, Clément Léger wrote:
> Ethernet frames can be extracted or injected autonomously to or from
> the device’s DDR3/DDR3L memory and/or PCIe memory space. Linked list
> data structures in memory are used for injecting or extracting Ethernet
> frames. The FDMA generates interrupts when frame extraction or
> injection is done and when the linked lists need updating.
> 
> The FDMA is shared between all the ethernet ports of the switch and
> uses a linked list of descriptors (DCB) to inject and extract packets.
> Before adding descriptors, the FDMA channels must be stopped. It would
> be inefficient to do that each time a descriptor would be added so the
> channels are restarted only once they stopped.
> 
> Both channels uses ring-like structure to feed the DCBs to the FDMA.
> head and tail are never touched by hardware and are completely handled
> by the driver. On top of that, page recycling has been added and is
> mostly taken from gianfar driver.
> 
> Co-developed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> Signed-off-by: Clément Léger <clement.leger@bootlin.com>
> ---

Doesn't look too bad. Did the page reuse make any difference to the
throughput, or is the interaction with the FDMA extraction channel where
the bottleneck is?

> +static bool ocelot_fdma_add_rx_frag(struct ocelot_fdma_rx_buf *rxb, u32 stat,
> +				    struct sk_buff *skb, bool first)
> +{
> +	int size = MSCC_FDMA_DCB_STAT_BLOCKL(stat);
> +	struct page *page = rxb->page;
> +
> +	if (likely(first)) {
> +		skb_put(skb, size);
> +	} else {
> +		skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, page,
> +				rxb->page_offset, size, OCELOT_FDMA_RX_SIZE);
> +	}
> +
> +	/* Try to reuse page */
> +	if (unlikely(page_count(page) != 1 || page_is_pfmemalloc(page)))

I don't think you need to go through a folio, you can use
page_ref_count() directly.

> +		return false;
> +
> +	/* Change offset to the other half */
> +	rxb->page_offset ^= OCELOT_FDMA_RX_SIZE;
> +
> +	page_ref_inc(page);
> +
> +	return true;
> +}
> +
> +static void ocelot_fdma_send_skb(struct ocelot *ocelot,
> +				 struct ocelot_fdma *fdma, struct sk_buff *skb)
> +{
> +	struct ocelot_fdma_tx_ring *tx_ring = &fdma->tx_ring;
> +	struct ocelot_fdma_tx_buf *tx_buf;
> +	struct ocelot_fdma_dcb *dcb;
> +	dma_addr_t dma;
> +	u16 next_idx;
> +
> +	dcb = &tx_ring->dcbs[tx_ring->next_to_use];
> +	tx_buf = &tx_ring->bufs[tx_ring->next_to_use];
> +	if (!ocelot_fdma_tx_dcb_set_skb(ocelot, tx_buf, dcb, skb)) {
> +		dev_kfree_skb_any(skb);
> +		return;
> +	}
> +
> +	next_idx = ocelot_fdma_idx_next(tx_ring->next_to_use,
> +					OCELOT_FDMA_TX_RING_SIZE);
> +	/* If the FDMA TX chan is empty, then enqueue the DCB directly */
> +	if (ocelot_fdma_tx_ring_empty(fdma)) {
> +		dma = ocelot_fdma_idx_dma(tx_ring->dcbs_dma, tx_ring->next_to_use);
> +		ocelot_fdma_activate_chan(fdma, dma, MSCC_FDMA_INJ_CHAN);
> +	} else {
> +		/* Chain the DCBs */
> +		dcb->llp = ocelot_fdma_idx_dma(tx_ring->dcbs_dma, next_idx);
> +	}
> +
> +	tx_ring->next_to_use = next_idx;
> +
> +	skb_tx_timestamp(skb);

Isn't it problematic to update tx_ring->next_to_use and skb_tx_timestamp
after you've actually called ocelot_fdma_activate_chan()? This will race
with ocelot_fdma_tx_cleanup().

> +}
> +void ocelot_fdma_init(struct platform_device *pdev, struct ocelot *ocelot)
> +{
> +	struct device *dev = ocelot->dev;
> +	struct ocelot_fdma *fdma;
> +	void __iomem *regs;
> +	int ret;
> +
> +	regs = devm_platform_ioremap_resource_byname(pdev, "fdma");
> +	if (IS_ERR_OR_NULL(regs))
> +		return;

Shouldn't this be an optional io_target inside mscc_ocelot_probe, like
all the others are?

> +
> +	fdma = devm_kzalloc(dev, sizeof(*fdma), GFP_KERNEL);
> +	if (!fdma)
> +		goto err_release_resource;
> +
> +	ocelot->fdma = fdma;
> +	ocelot->dev->coherent_dma_mask = DMA_BIT_MASK(32);
> +	fdma->regs = regs;

This doesn't need to be kept inside struct ocelot_fdma, it isn't used
anywhere else.

> +	fdma->regmap = regmap_init_mmio(ocelot->dev, regs,
> +					&ocelot_fdma_regmap_config);
> +	if (IS_ERR(fdma->regmap))
> +		goto err_free_fdma;
> +
> +	ocelot_fdma_writel(fdma, MSCC_FDMA_INTR_ENA, 0);
> +
> +	fdma->irq = platform_get_irq_byname(pdev, "fdma");
> +	ret = devm_request_irq(dev, fdma->irq, ocelot_fdma_interrupt, 0,
> +			       dev_name(dev), ocelot);
> +	if (ret)
> +		goto err_regmap_exit;
> +
> +	ret = ocelot_fdma_rings_alloc(ocelot);
> +	if (ret)
> +		goto err_free_irq;
> +
> +	static_branch_enable(&ocelot_fdma_enabled);
> +
> +	return;
> +
> +err_free_irq:
> +	devm_free_irq(dev, fdma->irq, fdma);
> +err_regmap_exit:
> +	regmap_exit(fdma->regmap);
> +err_free_fdma:
> +	devm_kfree(dev, fdma);
> +err_release_resource:
> +	devm_iounmap(dev, regs);
> +
> +	ocelot->fdma = NULL;
> +}
> diff --git a/drivers/net/ethernet/mscc/ocelot_vsc7514.c b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
> index 38103b0255b0..d737c680b424 100644
> --- a/drivers/net/ethernet/mscc/ocelot_vsc7514.c
> +++ b/drivers/net/ethernet/mscc/ocelot_vsc7514.c
> @@ -18,6 +18,7 @@
>  
>  #include <soc/mscc/ocelot_vcap.h>
>  #include <soc/mscc/ocelot_hsio.h>
> +#include "ocelot_fdma.h"
>  #include "ocelot.h"
>  

Please rebase all your submissions to the current net-next/master, the
following has been introduced here in the meantime, making this patch
fail to apply:

#define VSC7514_VCAP_POLICER_BASE			128
#define VSC7514_VCAP_POLICER_MAX			191

>  static const u32 ocelot_ana_regmap[] = {
> @@ -1080,6 +1081,8 @@ static int mscc_ocelot_probe(struct platform_device *pdev)
>  		ocelot->targets[io_target[i].id] = target;
>  	}
>  
> +	ocelot_fdma_init(pdev, ocelot);
> +
>  	hsio = syscon_regmap_lookup_by_compatible("mscc,ocelot-hsio");
>  	if (IS_ERR(hsio)) {
>  		dev_err(&pdev->dev, "missing hsio syscon\n");
> @@ -1139,6 +1142,9 @@ static int mscc_ocelot_probe(struct platform_device *pdev)
>  	if (err)
>  		goto out_ocelot_devlink_unregister;
>  
> +	if (ocelot->fdma)
> +		ocelot_fdma_start(ocelot);
> +
>  	err = ocelot_devlink_sb_register(ocelot);
>  	if (err)
>  		goto out_ocelot_release_ports;
> @@ -1179,6 +1185,8 @@ static int mscc_ocelot_remove(struct platform_device *pdev)
>  {
>  	struct ocelot *ocelot = platform_get_drvdata(pdev);
>  
> +	if (ocelot->fdma)
> +		ocelot_fdma_deinit(ocelot);
>  	devlink_unregister(ocelot->devlink);
>  	ocelot_deinit_timestamp(ocelot);
>  	ocelot_devlink_sb_unregister(ocelot);
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index 11c99fcfd341..2667a203e10f 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -692,6 +692,12 @@ struct ocelot {
>  	/* Protects the PTP clock */
>  	spinlock_t			ptp_clock_lock;
>  	struct ptp_pin_desc		ptp_pins[OCELOT_PTP_PINS_NUM];
> +
> +	struct ocelot_fdma		*fdma;
> +	/* Napi context used by FDMA. Needs to be in ocelot to avoid using a
> +	 * backpointer in ocelot_fdma
> +	 */
> +	struct napi_struct		napi;

Can it at least be dynamically allocated, and kept as a pointer here?

>  };
>  
>  struct ocelot_policer {
> -- 
> 2.34.1
>