Re: [PATCH 2/6] PCI/pwrctrl: tc9563: Fix parsing the integrated Ethernet MAC Endpoint node

[Alex Elder <elder@riscstar.com>]

On 7/25/26 3:59 AM, Manivannan Sadhasivam via B4 Relay wrote:
> From: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> 
> DSP3 has an integrated Ethernet MAC Endpoint which has its own set of
> config registers for configuring settings such as ASPM. The Endpoint
> device has 2 physical functions and those 2 functions share the same
> settings.

This was not clear from the code.  So this explanation alone
represents an improvement.

> Hence, parse the Endpoint node under DSP3 instead of parsing both the
> functions. The existing parsing logic also has one OOB issue as parsing
> both functions will result in accessing past the tc9563_pwrctrl->cfg array.

The issue you mention is that the array has exactly one entry
available for the Ethernet MAC endpoints, so parsing more than
one (if defined in devicetree) references a non-existent entry.
Right?

But this constraint only applies to power control.  There can be
multiple MAC nodes (for their PCI functions) defined.  And it
seems the pwrctl settings (if any) *must* be specified for
function 0 and if specified for any others, they are quietly
ignored.

Is that accurate?

If so I think the DT binding could explain and possibly enforce
that, but I won't insist on that...

Reviewed-by: Alex Elder <elder@riscstar.com>

> 
> Fixes: 4c9c7be47310 ("PCI: pwrctrl: Add power control driver for TC9563")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@oss.qualcomm.com>
> ---
>   drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> index 488e1ec34a7f..482e82e433dd 100644
> --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> @@ -596,12 +596,17 @@ static int tc9563_pwrctrl_probe(struct platform_device *pdev)
>   		ret = tc9563_pwrctrl_parse_device_dt(tc9563, child, port);
>   		if (ret)
>   			break;
> -		/* Embedded ethernet device are under DSP3 */
> +
> +		/*
> +		 * The integrated Ethernet MAC Endpoint under DSP3 is a single
> +		 * device whose functions share the same config registers.
> +		 */
>   		if (port == TC9563_DSP3) {
> -			for_each_child_of_node_scoped(child, child1) {
> -				port++;
> +			struct device_node *eth __free(device_node) =
> +					of_get_next_available_child(child, NULL);

I'd add a blank line here.

> +			if (eth) {
>   				ret = tc9563_pwrctrl_parse_device_dt(tc9563,
> -								child1, port);
> +								eth, TC9563_ETHERNET);
>   				if (ret)
>   					break;
>   			}
>