Re: [PATCH net-next 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 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] IPQ5424 probe fails at this one commit and works again after
  the next patch.
- [Low] The IPQ5424 BM USED/REACT debugfs dump reads the wrong registers
  at this one commit and is fixed later in the series.

--

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

This commit adds a PPE type field so the driver can pick settings per SoC.
It also adds the IPQ5424 BM port and shared group buffer configuration. Some
BM register addresses and entry counts become per-device macros, because
IPQ5424 has 19 BM ports and some BM tables sit at different addresses.

> diff --git a/drivers/net/ethernet/qualcomm/ppe/ppe_config.c b/drivers/net/ethernet/qualcomm/ppe/ppe_config.c
> index 8a553902ffad1..3afa571193238 100644
> --- a/drivers/net/ethernet/qualcomm/ppe/ppe_config.c
> +++ b/drivers/net/ethernet/qualcomm/ppe/ppe_config.c

[ ... ]

> @@ -1472,12 +1535,23 @@ static int ppe_config_bm(struct ppe_device *ppe_dev)
>   */
>  static int ppe_config_qm(struct ppe_device *ppe_dev)
>  {
> +	int ret, i, group_buffer_cfg, queue_id, queue_cfg_count;
>  	const struct ppe_qm_queue_config *queue_cfg;
> -	int ret, i, queue_id, queue_cfg_count;
>  	u32 reg, multicast_queue_cfg[5];
>  	u32 unicast_queue_cfg[4];
>  	u32 group_cfg[3];
>  
> +	switch (ppe_dev->type) {
> +	case IPQ9574_PPE:
> +		group_buffer_cfg = ipq9574_ppe_qm_group_config;
> +		queue_cfg = ipq9574_ppe_qm_queue_config;
> +		queue_cfg_count = ARRAY_SIZE(ipq9574_ppe_qm_queue_config);
> +		break;
> +	default:
> +		dev_err(ppe_dev->dev, "Invalid PPE type %d\n", ppe_dev->type);
> +		return -EINVAL;
> +	}

[Severity: Medium]
Does IPQ5424 fail to probe at this commit? This switch has no IPQ5424_PPE
case. However, this patch now sets ppe_dev->type = IPQ5424_PPE for the
qcom,ipq5424-ppe compatible.

ppe_hw_config() always calls ppe_config_qm() right after ppe_config_bm().
On IPQ5424 the path is:

qcom_ppe_probe()
  ppe_hw_config()
    ppe_config_bm()    <- succeeds with the new IPQ5424 tables
    ppe_config_qm()    <- default branch, returns -EINVAL

Probe then fails with "PPE HW config failed".

The next patch in the series, "net: ethernet: qualcomm: ppe: Add IPQ5424
QM configuration", adds the IPQ5424_PPE case, so the series works once it
is fully applied.

Could the QM case be added here, or the ppe_config_qm() refactor be moved
into the next patch? Either way, this commit would stay bisectable on
IPQ5424.

Also, the commit message only describes the BM changes. It doesn't mention
the ppe_config_qm() refactor.

> diff --git a/drivers/net/ethernet/qualcomm/ppe/ppe_regs.h b/drivers/net/ethernet/qualcomm/ppe/ppe_regs.h
> index 746dfbb5a6821..441e8e04a80b1 100644
> --- a/drivers/net/ethernet/qualcomm/ppe/ppe_regs.h
> +++ b/drivers/net/ethernet/qualcomm/ppe/ppe_regs.h
> @@ -386,17 +386,32 @@

[ ... ]

> -#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]
Does the BM USED/REACT dump in debugfs read the wrong registers on IPQ5424
at this commit?

The IPQ5424 group ID table now starts at 0x6001c0. The counter tables
further down this file still use the IPQ9574 addresses and entry counts:

drivers/net/ethernet/qualcomm/ppe/ppe_regs.h:
#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() uses these values whatever ppe_dev->type is:

drivers/net/ethernet/qualcomm/ppe/ppe_debugfs.c:ppe_bm_counter_get() {
    ...
	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 this has three effects:

- The group ID registers are reported as used buffer counts.
- The react counts are read from the wrong address.
- BM ports 15-18 are never read.

The last patch in the series, "net: ethernet: qualcomm: ppe: Add IPQ5424
counter debugfs support", appears to fix this. It adds per-device
PPE_BM_USED_CNT_TBL_ADDR(ppe_dev) (0x600220) and
PPE_BM_REACT_CNT_TBL_ADDR(ppe_dev) (0x600280), both with 19 entries.

Could those definitions move into this patch with the other BM address
changes?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008-qcom_ipq5424_ppe-v1-0-16caf43e44ee%40oss.qualcomm.com