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

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();

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

> +}
> +
> +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()

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

- Mani


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