Re: [PATCH net-next 05/11] net: ethernet: qualcomm: ppe: Add platform support for IPQ5424

[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] In commit "net: ethernet: qualcomm: ppe: Add platform support for
  IPQ5424", the qcom,ipq5424-ppe compatible is enabled before any per-…
- [Low] At commit "net: ethernet: qualcomm: ppe: Add platform support for
  IPQ5424", ppe_port_resource_get() in…

--

net: ethernet: qualcomm: ppe: Add platform support for IPQ5424

This commit adds struct ppe_of_data to the PPE driver. It holds the
per-SoC clock rate, port count, regmap access tables and interconnect
paths. The commit also adds the qcom,ipq5424-ppe compatible.

> diff --git a/drivers/net/ethernet/qualcomm/ppe/ppe.c b/drivers/net/ethernet/qualcomm/ppe/ppe.c
> index 3c301e609d3e3..f71b673b6f978 100644
> --- a/drivers/net/ethernet/qualcomm/ppe/ppe.c
> +++ b/drivers/net/ethernet/qualcomm/ppe/ppe.c

[ ... ]

> @@ -218,8 +301,25 @@ static void qcom_ppe_remove(struct platform_device *pdev)
>  	ppe_debugfs_teardown(ppe_dev);
>  }
>  

[ ... ]

> +static const struct ppe_of_data ipq5424_data = {
> +	.ppe_rate = IPQ5424_PPE_CLK_RATE,
> +	.num_ports = IPQ5424_PPE_PORT_MAX,

[Severity: Low]
At this commit num_ports is 4. With that value, does ppe_port_resource_get()
in ppe_config.c still return the reserved resource when asked for it?

The function still indexes the single IPQ9574 table, where the reserved
entry sits at fixed index 8:

drivers/net/ethernet/qualcomm/ppe/ppe_config.c:ppe_port_resource_get() {
    ...
	if (port > ppe_dev->num_ports)
		return -EINVAL;

	res = ppe_scheduler_res[port];
    ...
}

On IPQ5424, a request with port == num_ports (4) would get the queue and
node ranges of IPQ9574 physical port 4 instead of the reserved entry.
Index 8 can no longer be reached.

No in-tree caller passes port == num_ports at this commit.
ppe_queue_dest_init() iterates port_id < num_ports, and
ppe_queues_to_ring_init() uses port 0.

A later patch in this series, "net: ethernet: qualcomm: ppe: Add IPQ5424
scheduler resource configuration", fixes this. It adds
ipq5424_ppe_scheduler_res[] with the reserved entry last, at index 4. It
also makes ppe_port_resource_get() pick the table per SoC and check
port >= scheduler_res_len.

Could that change come before this one in the series?

> +	.regmap_config = &regmap_config_ipq5424,
> +	.icc_paths = ipq5424_ppe_icc_data,
> +	.num_icc_paths = ARRAY_SIZE(ipq5424_ppe_icc_data),
> +};
> +
>  static const struct of_device_id qcom_ppe_of_match[] = {
> -	{ .compatible = "qcom,ipq9574-ppe" },
> +	{ .compatible = "qcom,ipq9574-ppe", .data = &ipq9574_data },
> +	{ .compatible = "qcom,ipq5424-ppe", .data = &ipq5424_data },

[Severity: Low]
Is it safe to enable the qcom,ipq5424-ppe compatible before ppe_config.c
can tell the two SoCs apart?

At this commit neither struct ppe_of_data nor struct ppe_device has a type
field. So on IPQ5424, ppe_hw_config() still programs the IPQ9574-only
tables:

ppe_config_bm():
	port_cfg = ipq9574_ppe_bm_port_config;

ppe_config_qm():
	queue_cfg = ipq9574_ppe_qm_queue_config;

ppe_config_scheduler():
	bm_cfg = ipq9574_ppe_sch_bm_config;
	...
	qm_cfg = ipq9574_ppe_sch_qm_config;

The scheduler tables give BM/QM arbitration slots to PPE ports 4-7. Those
ports don't exist on IPQ5424, and none of the slots has a backup port.

The BM port thresholds also cover the missing MAC3-5 and EIP ports. The
queue config includes multicast queues 284-299, which IPQ5424 doesn't
have.

Three later patches in this series fix this by adding
switch (ppe_dev->type) with IPQ5424_PPE cases:

  "net: ethernet: qualcomm: ppe: Add IPQ5424 BM buffer configuration"
  "net: ethernet: qualcomm: ppe: Add IPQ5424 QM configuration"
  "net: ethernet: qualcomm: ppe: Add IPQ5424 scheduler configuration"

Could the IPQ5424 compatible be added after those patches? Then a bisect
that lands on this commit wouldn't probe IPQ5424 with the IPQ9574
configuration.

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