Re: [PATCH net-next v2 3/9] dt-bindings: net: qcom,ethqos: add qcom,shikra-ethqos compatible

[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: 0 · Low: 2

- [Low] Documentation/devicetree/bindings/net/qcom,ethqos.yaml: the new
  `if: compatible contains qcom,shikra-ethqos` block makes `operating-…
- [Low] Concern (verified as resolved within this series): the binding
  adds `qcom,shikra-ethqos` with six mandatory clocks (including `axi-…

--

dt-bindings: net: qcom,ethqos: add qcom,shikra-ethqos compatible

This adds the qcom,shikra-ethqos compatible to the qcom,ethqos and
snps,dwmac schemas, and introduces an if/else block that constrains Shikra
to six clocks (adding axi-noc and pcie-tile-axi-noc) while keeping the
existing compatibles at four. The Shikra branch also makes
operating-points-v2 mandatory.

A couple of questions below.

> diff --git a/Documentation/devicetree/bindings/net/qcom,ethqos.yaml b/Documentation/devicetree/bindings/net/qcom,ethqos.yaml
> index 423959cb928d9..ad2cc706fa740 100644
> --- a/Documentation/devicetree/bindings/net/qcom,ethqos.yaml
> +++ b/Documentation/devicetree/bindings/net/qcom,ethqos.yaml

[ ... ]

> @@ -32,6 +29,7 @@ properties:
>            - qcom,qcs404-ethqos
>            - qcom,sa8775p-ethqos
>            - qcom,sc8280xp-ethqos
> +          - qcom,shikra-ethqos
>            - qcom,sm8150-ethqos

[Severity: Low]
At this commit there is no driver side for this compatible:
qcom_ethqos_match[] in drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
has no qcom,shikra-ethqos entry, and nothing acquires axi-noc or
pcie-tile-axi-noc or registers an OPP table.

Checking the rest of the series shows this is closed later by "net: stmmac:
qcom-ethqos: add per-platform NOC clock voting" and "net: stmmac:
qcom-ethqos: add Shikra EMAC support", which add:

  { .compatible = "qcom,shikra-ethqos", .data = &shikra_data},

and

  static const struct ethqos_noc_clk_cfg shikra_noc_clks[] = {
	{ "axi-noc", 120000000 },
	{ "pcie-tile-axi-noc", 120000000 },
	{ "stmmaceth", 120000000 },
  };

along with qcom_ethqos_init_noc_clks() doing devm_clk_bulk_get() plus
devm_pm_opp_of_add_table() and dev_pm_opp_set_rate(). The clock-names order
here (rgmii as the fourth name) also matches shikra_data, which does not
override link_clk_name.

So this looks consistent with the series as a whole; noting it only because
the binding lands ahead of its consumer and the intermediate bisect point
has no driver support.

[ ... ]

> @@ -92,6 +99,41 @@ required:
>    - clock-names
>    - reg-names
>  
> +allOf:
> +  - $ref: snps,dwmac.yaml#
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: qcom,shikra-ethqos
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 6
> +        clock-names:
> +          items:
> +            - const: stmmaceth
> +            - const: pclk
> +            - const: ptp_ref
> +            - const: rgmii
> +            - const: axi-noc
> +            - const: pcie-tile-axi-noc
> +      required:
> +        - operating-points-v2

[Severity: Low]
The commit message says the OPP table is there so its required-opps can
"vote VDD_CX to SVS when the NOC clocks are enabled". Should power-domains
be in this required list as well?

The required-opps vote only reaches a genpd if the consumer device has one
attached. In drivers/opp/core.c, _set_required_opps() starts with:

	struct device **devs = opp_table->required_devs;
	...
	if (!devs)
		return 0;

and required_devs[] is only populated via _opp_set_required_dev(), reached
from genpd_set_required_opp_dev() in drivers/pmdomain/core.c during genpd
attach, i.e. only when the node has a power-domains phandle. Even past that
point, dev_pm_domain_set_performance_state() in drivers/base/power/common.c
does:

	if (dev->pm_domain && dev->pm_domain->set_performance_state)
		return dev->pm_domain->set_performance_state(dev, state);

	return 0;

power-domains is already permitted through the snps,dwmac.yaml $ref (it is
declared there as power-domains: maxItems: 1), so unevaluatedProperties:
false is not in the way; it is simply not required.

As written, a Shikra node with operating-points-v2 and no power-domains
passes dt_binding_check, and at runtime the CX performance-state request is
silently skipped while the NOC clocks still run at 120 MHz. No in-tree DTS
is affected today since there is no Shikra DTS yet.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260908-shikra_ethernet-v2-0-bbe3389d0652%40oss.qualcomm.com