Re: [PATCH net-next v5 3/3] net: macb: distribute evenly Tx SRAM segments

["Théo Lebrun" <theo.lebrun@bootlin.com>]

Hello Nicolai,

On Mon Oct 19, 2026 at 2:58 PM CEST, Nicolai Buchwitz wrote:
> Hi Théo
>
> On 16.10.2026 18:58, Théo Lebrun wrote:
>> GEM has registers to configure the Tx SRAM segments distribution across
>> queues. The reset value is apprioriate (even spread) but we need to
>> care if/when number of active queues is modified (or if we inherited
>> unevenly initialised hardware from bootloader).
>> 
>> To distribute segments, we take as input the number of queues
>> (bp->num_queues) and the number of segments (found inside DCFG6).
>> Its output is a number of segments for each queue, formatted as
>> powers-of-two (eg 2 for queue 0 means it has 2^2=4 segments).
>> 
>> As the distribution logic is quite complex (at least its initial
>> versions had bugs), it is kunit-tested in macb_kunit.c and the
>> implementation lives in macb_utils.c. To test:
>> 
>> ⟩ env --unset=CROSS_COMPILE make ARCH=um mrproper
>> ⟩ env --unset=CROSS_COMPILE ./tools/testing/kunit/kunit.py run \
>>         --kconfig_add CONFIG_NET=y \
>>         --kconfig_add CONFIG_COMMON_CLK=y macb
>> 
>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>> ---
>>  drivers/net/ethernet/cadence/Kconfig      | 12 +++++
>>  drivers/net/ethernet/cadence/Makefile     |  5 +-
>>  drivers/net/ethernet/cadence/macb.h       | 17 +++++++
>>  drivers/net/ethernet/cadence/macb_kunit.c | 78 
>> +++++++++++++++++++++++++++++++
>>  drivers/net/ethernet/cadence/macb_main.c  | 19 +++++++-
>>  drivers/net/ethernet/cadence/macb_utils.c | 60 
>> ++++++++++++++++++++++++
>>  6 files changed, 188 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/cadence/Kconfig 
>> b/drivers/net/ethernet/cadence/Kconfig
>> index 5b2a461dfd28..a901f74fd4ab 100644
>> --- a/drivers/net/ethernet/cadence/Kconfig
>> +++ b/drivers/net/ethernet/cadence/Kconfig
>> @@ -51,4 +51,16 @@ config MACB_PCI
>>  	  To compile this driver as a module, choose M here: the module
>>  	  will be called macb_pci.
>> 
>> +config MACB_KUNIT_TEST
>> +	tristate "KUnit test for MACB" if !KUNIT_ALL_TESTS
>> +	depends on KUNIT
>> +	default KUNIT_ALL_TESTS
>> +	help
>> +	  Build KUnit tests for the MACB driver.
>> +
>> +	  For more information on KUnit and unit tests in general,
>> +	  please refer to the KUnit documentation.
>> +
>> +	  If unsure, say N.
>> +
>>  endif # NET_VENDOR_CADENCE
>> diff --git a/drivers/net/ethernet/cadence/Makefile 
>> b/drivers/net/ethernet/cadence/Makefile
>> index 1f33cdca9a3c..a0740da68649 100644
>> --- a/drivers/net/ethernet/cadence/Makefile
>> +++ b/drivers/net/ethernet/cadence/Makefile
>> @@ -2,7 +2,7 @@
>>  #
>>  # Makefile for the Atmel network device drivers.
>>  #
>> -macb-y	:= macb_main.o
>> +macb-y	:= macb_main.o macb_utils.o
>> 
>>  ifeq ($(CONFIG_MACB_USE_HWSTAMP),y)
>>  macb-y	+= macb_ptp.o
>> @@ -10,3 +10,6 @@ endif
>> 
>>  obj-$(CONFIG_MACB) += macb.o
>>  obj-$(CONFIG_MACB_PCI) += macb_pci.o
>> +
>> +obj-$(CONFIG_MACB_KUNIT_TEST) += macb_test.o
>> +macb_test-y := macb_kunit.o macb_utils.o
>
> With MACB=m and MACB_KUNIT_TEST=m (so allmodconfig) kbuild complains:
>
>    drivers/net/ethernet/cadence/Makefile: macb_utils.o is added to 
> multiple modules: macb macb_test
>
> Maybe build the test into macb.ko instead, or use 
> EXPORT_SYMBOL_IF_KUNIT()?

This is all dropped, following recommendation from Jakub. LLM will
generate that in the future if needed (and the kunits are available on
the mailing lists anyway).

>
>> diff --git a/drivers/net/ethernet/cadence/macb.h 
>> b/drivers/net/ethernet/cadence/macb.h
>> index 51835de0f2a1..60d61efbf3e4 100644
>> --- a/drivers/net/ethernet/cadence/macb.h
>> +++ b/drivers/net/ethernet/cadence/macb.h
>> @@ -194,6 +194,10 @@
>>  /* Screener Type 2 match registers */
>>  #define GEM_SCRT2		0x540
>> 
>> +/* Tx queue segment allocation */
>> +#define GEM_TXQSEGALLOC_LOWER	0x05A0
>> +#define GEM_TXQSEGALLOC_UPPER	0x05A4
>
> nit: unused now?

Yes. It is unused and I kept it on purpose. I want the offset to appear
so that if there is a register offset conflict we see it straight away.

From the datasheets I aggregated, I see no conflict. But that's the kind
of offsets that could be reused if the feature is not available on a
slightly different IP.

I even thought about adding many more #defines for unused (at the
moment) offsets, to act as "documentation". We could even improve the
offset comments to export more docs from datasheets to code. Long term,
it could be useful. Anyway, this was a long rambling to say that this
offset was kept on purpose.

>> diff --git a/drivers/net/ethernet/cadence/macb_kunit.c 
>> b/drivers/net/ethernet/cadence/macb_kunit.c
>
>> [...]
>
>> +static void macb_sram_segments_test(struct kunit *test)
>> +{
>> +	const struct macb_sram_segments_case *p = test->param_value;
>> +	u32 val = gem_sram_distribute_segments(p->num_queues, 
>> p->num_segments);
>> +	unsigned int i, sum_segments = 0, max_assigned_segments;
>> +	unsigned int num_queues = min(p->num_queues, p->num_segments);
>> +
>> +	for (i = 0; i < num_queues; i++) {
>> +		unsigned int q_segments = field_get(GEM_TXQSEGALLOC_MASK(i),
>> +						    val);
>> +
>> +		q_segments = 1U << q_segments;
>> +		sum_segments += q_segments;
>> +		KUNIT_ASSERT_GT_MSG(test, q_segments, 0, "queue %d, val %#x",
>> +				    i, val);
>
> This can't fail, 1U << x is always at least 1.

Code has been dropped!

>
>> +	}
>> +
>> +	for (i = num_queues; i < MACB_MAX_QUEUES; i++) {
>> +		unsigned int pow = field_get(GEM_TXQSEGALLOC_MASK(i), val);
>> +
>> +		KUNIT_ASSERT_EQ_MSG(test, pow, 0, "queue %d, val %#x", i, val);
>> +	}
>> +
>> +	max_assigned_segments = min(p->num_segments,
>> +				    MACB_MAX_QUEUES * num_queues);
>> +	KUNIT_ASSERT_EQ_MSG(test, sum_segments, max_assigned_segments,
>> +			    "val %#x", val);
>> +}
>> +
>> +struct macb_sram_segments_case macb_sram_segments_cases[] = {
>
> static const ... ?

Yes! I think some proper compiler or static analyser tooling flags could
even have warned about that.

>
>> [...]
>
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c 
>> b/drivers/net/ethernet/cadence/macb_main.c
>> index 2d069454795b..b70b81f51161 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -3163,6 +3163,20 @@ static void macb_init_hw(struct macb *bp)
>>  	if (bp->caps & MACB_CAPS_JUMBO)
>>  		bp->rx_frm_len_mask = MACB_RX_JFRMLEN_MASK;
>> 
>> +	/* Distribute Tx SRAM segments evenly based on active number of 
>> queues.
>> +	 * TXQSEGALLOC is only documented for some GEM integrations.
>> +	 */
>> +	if (bp->caps & MACB_CAPS_TXQSEGALLOC) {
>> +		unsigned int num_segments;
>> +		u32 val;
>> +
>> +		num_segments = 1U << GEM_BFEXT(SEGMENTS_BIT_SIZE,
>> +					       gem_readl(bp, DCFG6));
>> +		val = gem_sram_distribute_segments(bp->num_queues,
>> +						   num_segments);
>> +		gem_writel(bp, TXQSEGALLOC_LOWER, val);
>> +	}
>> +
>>  	macb_configure_dma(bp);
>> 
>>  	/* Enable RX partial store and forward and set watermark */
>> @@ -5854,6 +5868,7 @@ static const struct macb_config zynqmp_config = {
>>  	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE |
>>  		MACB_CAPS_JUMBO |
>>  		MACB_CAPS_GEM_HAS_PTP | MACB_CAPS_BD_RD_PREFETCH |
>> +		MACB_CAPS_TXQSEGALLOC |
>>  		MACB_CAPS_USRIO_HAS_MII,
>
> AFAIU zynqmp can't change the queue count,so this only rewrites the 
> reset value there?

Yes. But their datasheets indicate they support segment alloc registers
so I prefer putting the caps flag anyway.

> Also, where does the limit of 8 segments per queue come from? With a 
> single queue
> and 16 segments half of the Tx SRAM stays unused.

Argh, I was sure I had a good reason for that so I sent V6 and wanted to
reply to you afterwards. I thought the number of bits per queue was 2,
but no it is 3 and we should have:

   num_queues=1 num_segments=16 -> TXQSEGALLOC=0x00000007

But currently we have:

   num_queues=1 num_segments=16 -> TXQSEGALLOC=0x00000003

I'm not sure if it matters; it requires HW that has much more segments
than queues. For example EyeQ5 has 4 queues (the most I am aware of,
equal to mpfs) and 16 segments. To reproduce the bug on a 4 queues
platform, we'd need 64 segments.

From the datasheets: zu+ and versal have 1 segment, EyeQ5 has 16 and
mpfs is unknown.

This will wait a bit more. I'll send myself a reply on V6 to clarify I
have pending changes.

Thanks Nicolai!

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com