Re: [PATCH net-next 01/13] net: enetc: add initial netc-lib driver to support NTMP

[Andrew Lunn <andrew@lunn.ch>]

> +#define NTMP_FILL_CRD(crd, tblv, qa, ua) \
> +({ \
> +	typeof(crd) _crd = (crd); \
> +	(_crd)->update_act = cpu_to_le16(ua); \
> +	(_crd)->tblv_qact = NTMP_TBLV_QACT(tblv, qa); \
> +})
> +
> +#define NTMP_FILL_CRD_EID(req, tblv, qa, ua, eid) \
> +({ \
> +	typeof(req) _req = (req); \
> +	NTMP_FILL_CRD(&(_req)->crd, tblv, qa, ua); \
> +	(_req)->entry_id = cpu_to_le32(eid); \
> +})


These are pretty complex for #defines. Can they be made into
functions? That will get you type checking, finding bugs where
parameters are swapped.

> +int netc_setup_cbdr(struct device *dev, int cbd_num,
> +		    struct netc_cbdr_regs *regs,
> +		    struct netc_cbdr *cbdr)
> +{
> +	int size;
> +
> +	size = cbd_num * sizeof(union netc_cbd) + NTMP_BASE_ADDR_ALIGN;
> +
> +	cbdr->addr_base = dma_alloc_coherent(dev, size, &cbdr->dma_base,
> +					     GFP_KERNEL);
> +	if (!cbdr->addr_base)
> +		return -ENOMEM;
> +
> +	cbdr->dma_size = size;
> +	cbdr->bd_num = cbd_num;
> +	cbdr->regs = *regs;
> +
> +	/* The base address of the Control BD Ring must be 128 bytes aligned */
> +	cbdr->dma_base_align =  ALIGN(cbdr->dma_base,  NTMP_BASE_ADDR_ALIGN);
> +	cbdr->addr_base_align = PTR_ALIGN(cbdr->addr_base,
> +					  NTMP_BASE_ADDR_ALIGN);
> +
> +	cbdr->next_to_clean = 0;
> +	cbdr->next_to_use = 0;
> +	spin_lock_init(&cbdr->ring_lock);
> +
> +	/* Step 1: Configure the base address of the Control BD Ring */
> +	netc_write(cbdr->regs.bar0, lower_32_bits(cbdr->dma_base_align));
> +	netc_write(cbdr->regs.bar1, upper_32_bits(cbdr->dma_base_align));
> +
> +	/* Step 2: Configure the producer index register */
> +	netc_write(cbdr->regs.pir, cbdr->next_to_clean);
> +
> +	/* Step 3: Configure the consumer index register */
> +	netc_write(cbdr->regs.cir, cbdr->next_to_use);
> +
> +	/* Step4: Configure the number of BDs of the Control BD Ring */
> +	netc_write(cbdr->regs.lenr, cbdr->bd_num);
> +
> +	/* Step 5: Enable the Control BD Ring */
> +	netc_write(cbdr->regs.mr, NETC_CBDR_MR_EN);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(netc_setup_cbdr);

I assume there is a version 3 in development, which will need a
different library, or at least different symbols. Maybe you should
think about the naming issues now?

> diff --git a/include/linux/fsl/ntmp.h b/include/linux/fsl/ntmp.h
> new file mode 100644
> index 000000000000..7cf322a1c8e3
> --- /dev/null
> +++ b/include/linux/fsl/ntmp.h
> @@ -0,0 +1,178 @@
> +/* SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause) */
> +/* Copyright 2025 NXP */
> +#ifndef __NETC_NTMP_H
> +#define __NETC_NTMP_H

Does this header need to be global? What else will use it outside of
drivers/net/ethernet/freescale/enetc?

	Andrew