Re: [PATCH v2 4/5] PCI/pwrctrl: tc9563: switch per-port reset to GPIO descriptor API

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 6268 bytes --]

> On Thu, Sep 03, 2026 at 09:53:00AM +0200, Lorenzo Bianconi wrote:
> > Remove the local TC9563_GPIO_MASK and TC9563_GPIO_DEASSERT_BITS
> > definitions, which are no longer used after switching to the GPIO
> > descriptor API.
> > 
> > Replace the direct regmap-based per-port reset logic in
> > assert_deassert_reset() with gpiod_direction_output() calls, falling
> > back to the legacy regmap approach only when no reset-gpios DT
> > property is present for a given port.
> > 
> > Add the reset GPIO pointer to struct tc9563_pwrctrl_cfg and introduce
> > tc9563_pwrctrl_parse_reset_line() to look up reset-gpios from each
> > PCI downstream port child node. The lookup is done lazily at the
> > beginning of power_on(), returning -EPROBE_DEFER until the GPIO chip
> > is registered.
> > 
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> 
> Capitalize "Switch per-port ..." in subject.

ack, I will fix it in v2.

> 
> Question below.
> 
> > ---
> >  drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c | 85 +++++++++++++++++++++++++++-----
> >  1 file changed, 73 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> > index 6df512d78b54..09ab4718db8a 100644
> > --- a/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> > +++ b/drivers/pci/pwrctrl/pci-pwrctrl-tc9563.c
> > @@ -57,9 +57,6 @@
> >  #define TC9563_POWER_CONTROL		0x82b09c
> >  #define TC9563_POWER_CONTROL_OVREN	0x82b2c8
> >  
> > -#define TC9563_GPIO_MASK		0xfffffff3
> > -#define TC9563_GPIO_DEASSERT_BITS	0xc  /* Clear to deassert GPIO */
> > -
> >  #define TC9563_TX_MARGIN_MIN_UA		400000
> >  
> >  /*
> > @@ -85,6 +82,7 @@ struct tc9563_pwrctrl_cfg {
> >  	u8 nfts[2]; /* GEN1 & GEN2 */
> >  	bool disable_dfe;
> >  	bool disable_port;
> > +	struct gpio_desc *reset;
> >  };
> >  
> >  #define TC9563_PWRCTL_MAX_SUPPLY	6
> > @@ -349,16 +347,40 @@ static int tc9563_pwrctrl_set_nfts(struct tc9563_pwrctrl *tc9563,
> >  static int tc9563_pwrctrl_assert_deassert_reset(struct tc9563_pwrctrl *tc9563,
> >  						bool deassert)
> >  {
> > -	int ret, val;
> > -
> > -	ret = regmap_write(tc9563->regmap, TC9563_GPIO_CONFIG,
> > -			   TC9563_GPIO_MASK);
> > -	if (ret)
> > -		return ret;
> > -
> > -	val = deassert ? TC9563_GPIO_DEASSERT_BITS : 0;
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(tc9563->cfg); i++) {
> > +		int err;
> > +
> > +		if (tc9563->cfg[i].reset) {
> > +			err = gpiod_direction_output(tc9563->cfg[i].reset,
> > +						     !deassert);
> > +			if (err)
> > +				return err;
> > +		} else {
> > +			/* Fallback: legacy DTS without reset-gpios */
> > +			switch (i) {
> > +			case TC9563_DSP1:
> > +			case TC9563_DSP2:
> > +				err = regmap_clear_bits(tc9563->regmap,
> > +							TC9563_GPIO_CONFIG,
> > +							BIT(i + 1));
> > +				if (err)
> > +					return err;
> > +
> > +				err = regmap_assign_bits(tc9563->regmap,
> > +							 TC9563_RESET_GPIO,
> > +							 BIT(i + 1), deassert);
> > +				if (err)
> > +					return err;
> > +				break;
> > +			default:
> > +				break;
> > +			}
> > +		}
> > +	}
> >  
> > -	return regmap_write(tc9563->regmap, TC9563_RESET_GPIO, val);
> > +	return 0;
> >  }
> >  
> >  static int tc9563_pwrctrl_parse_device_dt(struct device_node *node,
> > @@ -393,6 +415,41 @@ static int tc9563_pwrctrl_parse_device_dt(struct device_node *node,
> >  	return 0;
> >  }
> >  
> > +static int tc9563_pwrctrl_parse_reset_line(struct tc9563_pwrctrl *tc9563)
> > +{
> > +	enum tc9563_pwrctrl_ports port = TC9563_USP;
> > +	struct device *dev = tc9563->pwrctrl.dev;
> > +	struct device_node *node = dev->of_node;
> > +
> > +	for_each_child_of_node_scoped(node, child) {
> > +		struct tc9563_pwrctrl_cfg *cfg;
> > +
> > +		if (!of_node_is_type(child, "pci"))
> > +			continue;
> > +
> > +		if (++port >= TC9563_MAX)
> > +			break;
> > +
> > +		cfg = &tc9563->cfg[port];
> > +		if (cfg->reset) /* Already discovered */
> > +			continue;
> > +
> > +		cfg->reset = devm_fwnode_gpiod_get(dev, of_fwnode_handle(child),
> > +						   "reset", GPIOD_ASIS,
> > +						   NULL);
> > +		if (IS_ERR(cfg->reset)) {
> > +			int err = PTR_ERR(cfg->reset);
> > +
> > +			cfg->reset = NULL;
> > +			if (err != -ENOENT)
> > +				return dev_err_probe(dev, err,
> > +						     "failed to get reset\n");
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static void tc9563_pwrctrl_adev_release(struct device *dev)
> >  {
> >  	struct auxiliary_device *adev = to_auxiliary_dev(dev);
> > @@ -480,6 +537,10 @@ static int tc9563_pwrctrl_power_on(struct pci_pwrctrl *pwrctrl)
> >  	struct tc9563_pwrctrl_cfg *cfg;
> >  	int ret, i;
> >  
> > +	ret = tc9563_pwrctrl_parse_reset_line(tc9563);
> > +	if (ret)
> > +		return ret;
> 
> Seems like this call would fit better in
> tc9563_pwrctrl_parse_device_dt() since it already handles similar
> properties and nothing in DT is changing.
> 
> But I guess this is because the GPIO chip may not be registered at
> tc9563_pwrctrl_probe() time.  Could tc9563_pwrctrl_probe() itself
> return -EPROBE_DEFER?  Returning it from tc9563_pwrctrl_power_on()
> seems a little weird.

Correct, I am not running tc9563_pwrctrl_parse_reset_line() in
tc9563_pwrctrl_parse_device_dt() since GPIO chip may not be registered at
moment (we need to run tc9563_pwrctrl_add_gpio_adev() first).
Moreover, pwrctrl power_on() callback is running during PCI host bridge
driver probe:

	qcom_pcie_probe()
	-> dw_pcie_host_init()
	   -> qcom_pcie_host_init()
	      -> pci_pwrctrl_power_on_devices()
	         -> tc9563_pwrctrl_power_on()

But this approach will allow us to avoid running pci_pwrctrl_destroy_devices()
in qcom_pcie_host_init() if tc9563_pwrctrl_power_on() returns -EPROBE_DEFER
(e.g. if the GPIO controller has not fully probed yet).
If we return -EPROBE_DEFER in tc9563_pwrctrl_probe() this kernel will destroy
even the GPIO aux device and we need to create it again during the next
attempt.

Regards,
Lorenzo

> 
> >  	ret = regulator_bulk_enable(ARRAY_SIZE(tc9563->supplies),
> >  				    tc9563->supplies);
> >  	if (ret < 0)
> > 
> > -- 
> > 2.55.0
> > 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]