Re: [PATCH v3 2/3] PCI: ultrarisc: get and enable DP1000 PCIe clocks

On 2026-07-29 15:48 +0200, Manivannan Sadhasivam wrote:
> On Tue, Jul 14, 2026 at 09:11:03AM +0800, Jia Wang via B4 Relay wrote:
> > From: Jia Wang <[email protected]>
> > 
> > Add the required core, dbi, and aux clocks for the DP1000 PCIe
> > controller and enable them before initializing the DesignWare host.
> > 
> > Also manage the clocks across system suspend and resume.
> > 
> > Fixes: 5fc35740c3b3 ("PCI: ultrarisc: Add UltraRISC DP1000 PCIe Root Complex driver")
> > Signed-off-by: Jia Wang <[email protected]>
> > ---
> >  drivers/pci/controller/dwc/pcie-ultrarisc.c | 102 ++++++++++++++++++++++++++--
> >  1 file changed, 95 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-ultrarisc.c b/drivers/pci/controller/dwc/pcie-ultrarisc.c
> > index 6ee661ceff67..72ba5840b62d 100644
> > --- a/drivers/pci/controller/dwc/pcie-ultrarisc.c
> > +++ b/drivers/pci/controller/dwc/pcie-ultrarisc.c
> > @@ -5,6 +5,7 @@
> >   * Copyright (C) 2026 UltraRISC Technology (Shanghai) Co., Ltd.
> >   */
> >  
> > +#include <linux/clk.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/of_device.h>
> > @@ -23,6 +24,12 @@
> >  
> >  #define ULTRARISC_PCIE_COMP_TIMEOUT_65_210MS	0x6
> >  
> > +struct ultrarisc_pcie {
> > +	struct dw_pcie pci;
> > +	struct clk_bulk_data clks[3];
> > +	bool clks_enabled;
> > +};
> > +
> >  static struct pci_ops ultrarisc_pci_ops = {
> >  	.map_bus = dw_pcie_own_conf_map_bus,
> >  	.read = pci_generic_config_read32,
> > @@ -98,17 +105,66 @@ static const struct dw_pcie_ops dw_pcie_ops = {
> >  	.start_link = ultrarisc_pcie_start_link,
> >  };
> >  
> > +static int ultrarisc_pcie_enable_clks(struct ultrarisc_pcie *ultra)
> > +{
> > +	int ret;
> > +
> > +	if (ultra->clks_enabled)
> > +		return 0;
> > +
> 
> This check looks redundant and I don't see a need for the 'clks_enabled' flag.
> Just do:
> 
> 	return clk_bulk_prepare_enable();
>

Thanks for the review.

Agreed. I will remove clks_enabled and have ultrarisc_pcie_enable_clks()
return clk_bulk_prepare_enable() directly in the next version.

> > +	ret = clk_bulk_prepare_enable(ARRAY_SIZE(ultra->clks), ultra->clks);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ultra->clks_enabled = true;
> > +
> > +	return 0;
> > +}
> > +
> > +static void ultrarisc_pcie_disable_clks(void *data)
> > +{
> > +	struct ultrarisc_pcie *ultra = data;
> > +
> > +	if (!ultra->clks_enabled)
> > +		return;
> > +
> > +	clk_bulk_disable_unprepare(ARRAY_SIZE(ultra->clks), ultra->clks);
> > +	ultra->clks_enabled = false;
> 
> Same here.
>

Agreed. I will make ultrarisc_pcie_disable_clks() call
clk_bulk_disable_unprepare() unconditionally in the next version.

> > +}
> > +
> > +static int ultrarisc_pcie_init_clks(struct ultrarisc_pcie *ultra)
> > +{
> > +	struct device *dev = ultra->pci.dev;
> > +	int ret;
> > +
> > +	ultra->clks[0].id = "core";
> > +	ultra->clks[1].id = "dbi";
> > +	ultra->clks[2].id = "aux";
> > +
> > +	ret = devm_clk_bulk_get(dev, ARRAY_SIZE(ultra->clks), ultra->clks);
> 
> Use devm_clk_bulk_get_all()
>

I used `devm_clk_bulk_get()` to make the driver require the "core", "dbi",
and "aux" clock names. Using `devm_clk_bulk_get_all()` would instead accept
the clocks without requiring those names. Is that intended, or is there a
requirement or convention I am missing?

> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to get clocks\n");
> > +
> > +	ret = ultrarisc_pcie_enable_clks(ultra);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to enable clocks\n");
> > +
> > +	return devm_add_action_or_reset(dev, ultrarisc_pcie_disable_clks, ultra);
> > +}
> > +
> >  static int ultrarisc_pcie_probe(struct platform_device *pdev)
> >  {
> > +	struct ultrarisc_pcie *ultra;
> >  	struct device *dev = &pdev->dev;
> >  	struct dw_pcie_rp *pp;
> >  	struct dw_pcie *pci;
> >  	int ret;
> >  
> > -	pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> > -	if (!pci)
> > +	ultra = devm_kzalloc(dev, sizeof(*ultra), GFP_KERNEL);
> > +	if (!ultra)
> >  		return -ENOMEM;
> >  
> > +	pci = &ultra->pci;
> >  	pci->dev = dev;
> >  	pci->ops = &dw_pcie_ops;
> >  
> > @@ -117,7 +173,11 @@ static int ultrarisc_pcie_probe(struct platform_device *pdev)
> >  
> >  	pp = &pci->pp;
> >  
> > -	platform_set_drvdata(pdev, pci);
> > +	platform_set_drvdata(pdev, ultra);
> > +
> > +	ret = ultrarisc_pcie_init_clks(ultra);
> > +	if (ret)
> > +		return ret;
> >  
> >  	pp->num_vectors = MAX_MSI_IRQS;
> >  	/* No L2/L3 Ready indication is available on this platform */
> > @@ -135,16 +195,44 @@ static int ultrarisc_pcie_probe(struct platform_device *pdev)
> >  
> >  static int ultrarisc_pcie_suspend_noirq(struct device *dev)
> >  {
> > -	struct dw_pcie *pci = dev_get_drvdata(dev);
> > +	struct ultrarisc_pcie *ultra = dev_get_drvdata(dev);
> > +	struct dw_pcie *pci = &ultra->pci;
> > +	int ret;
> > +
> > +	if (pci->suspended) {
> > +		ultrarisc_pcie_disable_clks(ultra);
> > +		return 0;
> > +	}
> 
> Same as above. This looks redundant. Actually, it is wrong too as there cannot
> be more than one suspend callback invocations.
>

Agreed that suspend_noirq() is invoked once per transition. However, a
failed resume leaves pci->suspended set and the clocks disabled. The PM
core records the resume error and continues the transition, so a later,
independent system suspend can invoke this callback again. In the next
version, I will keep the entry check as an early return for that case,
without disabling the clocks again.

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

Best regards,
Jia Wang