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

[Manivannan Sadhasivam]

On Thu, Jul 30, 2026 at 02:48:23PM +0800, Jia Wang wrote:
> 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 <wangjia@xxxxxxxxxxxxx>
> > > 
> > > 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 <wangjia@xxxxxxxxxxxxx>
> > > ---
> > >  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?
> 

Your DT binding should ensure that the DTS supplies the required clocks. So the
driver shouldn't hardcode the clock name and just get whatever clocks DTS
passes. Otherwise, the driver will become a DTS validator.

> > > +	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.
> 

OK!

- Mani

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