Re: [PATCH net-next v2 06/11] net: ethernet: qualcomm: ppe: Add IPQ5424 BM buffer configuration

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch adds PPE_BM_PORT_GROUP_ID_IPQ5424_ADDR = 0x6001c0, and
  ppe_config_bm_threshold() now writes the IPQ5424 BM port group IDs to…

--

net: ethernet: qualcomm: ppe: Add IPQ5424 BM buffer configuration

This patch adds an IPQ5424 BM buffer configuration (per-port thresholds
and a larger group0 shared pool) and a ppe_type field in struct
ppe_device to select it. It also adds per-SoC address and entry-count
macros for the BM register tables, which have more entries on IPQ5424
and in some cases sit at different addresses.

> diff --git a/drivers/net/ethernet/qualcomm/ppe/ppe_regs.h b/drivers/net/ethernet/qualcomm/ppe/ppe_regs.h
> index 746dfbb5a682..441e8e04a80b 100644
> --- a/drivers/net/ethernet/qualcomm/ppe/ppe_regs.h
> +++ b/drivers/net/ethernet/qualcomm/ppe/ppe_regs.h

[ ... ]

> -#define PPE_BM_PORT_GROUP_ID_ADDR		0x600180
> -#define PPE_BM_PORT_GROUP_ID_ENTRIES		15
> +#define PPE_BM_PORT_GROUP_ID_IPQ9574_ADDR	0x600180
> +#define PPE_BM_PORT_GROUP_ID_IPQ5424_ADDR	0x6001c0

[Severity: Low]
With this change, two tables in this header use the same address on
IPQ5424. Right below this hunk, the counter tables still have one
fixed layout for all SoCs:

  /* Counters for PPE buffers used for packets cached. */
  #define PPE_BM_USED_CNT_TBL_ADDR		0x6001c0
  #define PPE_BM_USED_CNT_TBL_ENTRIES		15
  ...
  #define PPE_BM_REACT_CNT_TBL_ADDR		0x600240
  #define PPE_BM_REACT_CNT_TBL_ENTRIES		15

ppe_bm_counter_get() in ppe_debugfs.c uses these constants on every SoC:

  for (i = 0; i < PPE_BM_USED_CNT_TBL_ENTRIES; i++) {
          reg = PPE_BM_USED_CNT_TBL_ADDR + i * PPE_BM_USED_CNT_TBL_INC;
          ...
          reg = PPE_BM_REACT_CNT_TBL_ADDR + i * PPE_BM_REACT_CNT_TBL_INC;

On IPQ5424, will the "BM USED/REACT" debugfs output show the group ID
registers that ppe_config_bm_threshold() just wrote, instead of buffer
usage counters? The REACT values would also come from the wrong table,
and BM ports 15-18 would never be shown. Ports 16-18 are the three
physical ports.

The configuration write to 0x6001c0 looks correct; only the debugfs read
side is affected. The new PPE_BM_PORT_*_ENTRIES(ppe_dev) macros also have
no callers yet in this patch.

A later patch in this series, "net: ethernet: qualcomm: ppe: Add IPQ5424
counter debugfs support", fixes this. It adds IPQ5424 counter table
addresses (0x600220 and 0x600280, 19 entries each) and makes
ppe_debugfs.c use the per-SoC macros.

Would it make sense to move the counter address split into this patch?
That way the header never has GROUP_ID and USED_CNT at the same IPQ5424
address in between patches.

> +#define PPE_BM_PORT_GROUP_ID_ADDR(ppe_dev)					\
> +	((ppe_dev)->type == IPQ5424_PPE ? PPE_BM_PORT_GROUP_ID_IPQ5424_ADDR :	\
> +					  PPE_BM_PORT_GROUP_ID_IPQ9574_ADDR)
> +#define PPE_BM_PORT_GROUP_ID_ENTRIES(ppe_dev)	PPE_BM_PORT_ENTRIES(ppe_dev)
>  #define PPE_BM_PORT_GROUP_ID_INC		0x4
>  #define PPE_BM_PORT_GROUP_ID_SHARED_GROUP_ID	GENMASK(1, 0)

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261011-qcom_ipq5424_ppe-v2-0-8c7b057e2c43%40oss.qualcomm.com