Re: [PATCH v4 5/6] PCI: dwc: rcar-gen4: Handle PERST via reset subsystem

[Marek Vasut]

On 9/21/26 5:46 PM, Manivannan Sadhasivam wrote:

Hello Manivannan,

> > +++ b/drivers/pci/controller/dwc/pcie-rcar-gen4.c
> > @@ -99,6 +99,7 @@ struct rcar_gen4_pcie {
> >   	void __iomem *base;
> >   	void __iomem *phy_base;
> >   	struct platform_device *pdev;
> > +	struct reset_control *perst;
> >   	const struct rcar_gen4_pcie_drvdata *drvdata;
> >   };
> >   #define to_rcar_gen4_pcie(_dw)	container_of(_dw, struct rcar_gen4_pcie, dw)
> > @@ -317,12 +318,27 @@ static void rcar_gen4_pcie_unprepare(struct rcar_gen4_pcie *rcar)
> >   
> >   static int rcar_gen4_pcie_get_resources(struct rcar_gen4_pcie *rcar)
> >   {
> > +	struct device *dev = rcar->dw.dev;
> > +	struct device_node *root_port;
> > +
> >   	rcar->phy_base = devm_platform_ioremap_resource_byname(rcar->pdev, "phy");
> >   	if (IS_ERR(rcar->phy_base))
> >   		return PTR_ERR(rcar->phy_base);
> >   
> > +	root_port = of_get_next_available_child(dev->of_node, NULL);
> > +	if (root_port) {
> > +		rcar->perst = of_reset_control_get_optional_exclusive(root_port, "perst");
> > +		of_node_put(root_port);
> > +		if (IS_ERR(rcar->perst))
> > +			return dev_err_probe(dev, PTR_ERR(rcar->perst), "Failed to get PERST#\n");
> > +	} else {
> > +		rcar->perst = NULL;
>
> 'rcar->perst' is NULL by default.

Indeed.

> > +	}
> > +
> >   	/* Renesas-specific registers */
> >   	rcar->base = devm_platform_ioremap_resource_byname(rcar->pdev, "app");
> > +	if (IS_ERR(rcar->base))
> > +		reset_control_put(rcar->perst);
> >   
> >   	return PTR_ERR_OR_ZERO(rcar->base);
> >   }
> > @@ -494,6 +510,22 @@ static int rcar_gen4_pcie_enable_device(struct pci_host_bridge *bridge,
> >   	return 0;
> >   }
> >   
> > +static void rcar_gen4_pcie_host_perst_assert(struct dw_pcie_rp *pp, bool assert)
> > +{
> > +	struct dw_pcie *dw = to_dw_pcie_from_pp(pp);
> > +	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> > +
> > +	gpiod_set_value_cansleep(dw->pe_rst, assert);
> > +
> > +	if (!rcar->perst)
> > +		return;
> > +
> > +	if (assert)
> > +		reset_control_assert(rcar->perst);
> > +	else
> > +		reset_control_deassert(rcar->perst);
>
> So the controller will only have one form of PERST# implemented at a time. Even
> though this code is technically correct, it also gives an impression that both
> form could co-exist.
>
> So I'd recommend using:
>
>          if (dw->pe_rst) {
>                  gpiod_set_value_cansleep(dw->pe_rst, assert);
>          } else {
>                  if (assert)
>                          reset_control_assert(rcar->perst);
>                  else
>                          reset_control_deassert(rcar->perst);
>          }
>
> to makes it clear that only one form of PERST# is supported.
>
> I'll implement both of these changes while applying.

Please do. Thank you !

--
Best regards,
Marek Vasut