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

On 2026-07-30 10:00 +0200, Manivannan Sadhasivam wrote:
> 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 <[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?
> > 
> 
> 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.
>

Agreed. I will use devm_clk_bulk_get_all() and rely on the DT binding to
validate the required clocks in v4. Thanks!

> > > > +	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
> 
> -- 
> மணிவண்ணன் சதாசிவம்
> 

Best regards,
Jia Wang