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

[Marek Vasut <marek.vasut@mailbox.org>]

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