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

[Manivannan Sadhasivam]

On Mon, Jul 27, 2026 at 10:59:05AM -0500, Alex Elder wrote:
> On 7/25/26 3:59 AM, Manivannan Sadhasivam via B4 Relay wrote:
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> > 
> > 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?
> 

Yes.

> 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?
> 

Exactly. DT can define both functions, but since there is no per-function
tuning, these properties cannot exist in both nodes, but only in function 0,
which also corresponds to the Endpoint.

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

It is being enforced in Patch 1.

- Mani

> Reviewed-by: Alex Elder <elder@xxxxxxxxxxxx>
> 
> > 
> > Fixes: 4c9c7be47310 ("PCI: pwrctrl: Add power control driver for TC9563")
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> > ---
> >   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;
> >   			}
> > 
> 

-- 
மணிவண்ணன் சதாசிவம்