diff mbox series

[v3,1/3] PCI: qcom: Use clk_bulk API for 2.4.0 controllers

Message ID 20190502001955.10575-2-bjorn.andersson@linaro.org (mailing list archive)
State Superseded
Headers show
Series Qualcomm QCS404 PCIe support | expand

Commit Message

Bjorn Andersson May 2, 2019, 12:19 a.m. UTC
Before introducing the QCS404 platform, which uses the same PCIe
controller as IPQ4019, migrate this to use the bulk clock API, in order
to make the error paths slighly cleaner.

Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---

Changes since v2:
- Defined QCOM_PCIE_2_4_0_MAX_CLOCKS

 drivers/pci/controller/dwc/pcie-qcom.c | 49 ++++++++------------------
 1 file changed, 14 insertions(+), 35 deletions(-)

Comments

Vinod Koul May 2, 2019, 11:53 a.m. UTC | #1
On 01-05-19, 17:19, Bjorn Andersson wrote:
> Before introducing the QCS404 platform, which uses the same PCIe
> controller as IPQ4019, migrate this to use the bulk clock API, in order
> to make the error paths slighly cleaner.
> 
> Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v2:
> - Defined QCOM_PCIE_2_4_0_MAX_CLOCKS
> 
>  drivers/pci/controller/dwc/pcie-qcom.c | 49 ++++++++------------------
>  1 file changed, 14 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 0ed235d560e3..d740cbe0e56d 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -112,10 +112,10 @@ struct qcom_pcie_resources_2_3_2 {
>  	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
>  };
>  
> +#define QCOM_PCIE_2_4_0_MAX_CLOCKS	3

empty line after the define please

>  struct qcom_pcie_resources_2_4_0 {
> -	struct clk *aux_clk;
> -	struct clk *master_clk;
> -	struct clk *slave_clk;
> +	struct clk_bulk_data clks[QCOM_PCIE_2_4_0_MAX_CLOCKS];
> +	int num_clks;
>  	struct reset_control *axi_m_reset;
>  	struct reset_control *axi_s_reset;
>  	struct reset_control *pipe_reset;
> @@ -638,18 +638,17 @@ static int qcom_pcie_get_resources_2_4_0(struct qcom_pcie *pcie)
>  	struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
>  	struct dw_pcie *pci = pcie->pci;
>  	struct device *dev = pci->dev;
> +	int ret;
>  
> -	res->aux_clk = devm_clk_get(dev, "aux");
> -	if (IS_ERR(res->aux_clk))
> -		return PTR_ERR(res->aux_clk);
> +	res->clks[0].id = "aux";
> +	res->clks[1].id = "master_bus";
> +	res->clks[2].id = "slave_bus";
>  
> -	res->master_clk = devm_clk_get(dev, "master_bus");
> -	if (IS_ERR(res->master_clk))
> -		return PTR_ERR(res->master_clk);
> +	res->num_clks = 3;
>  
> -	res->slave_clk = devm_clk_get(dev, "slave_bus");
> -	if (IS_ERR(res->slave_clk))
> -		return PTR_ERR(res->slave_clk);
> +	ret = devm_clk_bulk_get(dev, res->num_clks, res->clks);
> +	if (ret < 0)
> +		return ret;
>  
>  	res->axi_m_reset = devm_reset_control_get_exclusive(dev, "axi_m");
>  	if (IS_ERR(res->axi_m_reset))
> @@ -719,9 +718,7 @@ static void qcom_pcie_deinit_2_4_0(struct qcom_pcie *pcie)
>  	reset_control_assert(res->axi_m_sticky_reset);
>  	reset_control_assert(res->pwr_reset);
>  	reset_control_assert(res->ahb_reset);
> -	clk_disable_unprepare(res->aux_clk);
> -	clk_disable_unprepare(res->master_clk);
> -	clk_disable_unprepare(res->slave_clk);
> +	clk_bulk_disable_unprepare(res->num_clks, res->clks);
>  }
>  
>  static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> @@ -850,23 +847,9 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
>  
>  	usleep_range(10000, 12000);
>  
> -	ret = clk_prepare_enable(res->aux_clk);
> -	if (ret) {
> -		dev_err(dev, "cannot prepare/enable iface clock\n");
> +	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
> +	if (ret)
>  		goto err_clk_aux;
> -	}
> -
> -	ret = clk_prepare_enable(res->master_clk);
> -	if (ret) {
> -		dev_err(dev, "cannot prepare/enable core clock\n");
> -		goto err_clk_axi_m;
> -	}
> -
> -	ret = clk_prepare_enable(res->slave_clk);
> -	if (ret) {
> -		dev_err(dev, "cannot prepare/enable phy clock\n");
> -		goto err_clk_axi_s;
> -	}
>  
>  	/* enable PCIe clocks and resets */
>  	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> @@ -891,10 +874,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
>  
>  	return 0;
>  
> -err_clk_axi_s:
> -	clk_disable_unprepare(res->master_clk);
> -err_clk_axi_m:
> -	clk_disable_unprepare(res->aux_clk);
>  err_clk_aux:
>  	reset_control_assert(res->ahb_reset);
>  err_rst_ahb:
> -- 
> 2.18.0


rest lgtm:

Reviewed-by: Vinod Koul <vkoul@kernel.org>
Bjorn Andersson May 2, 2019, 3 p.m. UTC | #2
On Thu 02 May 04:53 PDT 2019, Vinod Koul wrote:
> On 01-05-19, 17:19, Bjorn Andersson wrote:
[..]
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 0ed235d560e3..d740cbe0e56d 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -112,10 +112,10 @@ struct qcom_pcie_resources_2_3_2 {
> >  	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
> >  };
> >  
> > +#define QCOM_PCIE_2_4_0_MAX_CLOCKS	3
> 
> empty line after the define please
> 

This follows the style of QCOM_PCIE_2_3_2_MAX_SUPPLY one block up, so
I think this is the way we want it.

> >  struct qcom_pcie_resources_2_4_0 {
[..]
> 
> 
> rest lgtm:
> 
> Reviewed-by: Vinod Koul <vkoul@kernel.org>
> 

Thanks!

Regards,
Bjorn
Vinod Koul May 3, 2019, 3:10 a.m. UTC | #3
On 02-05-19, 08:00, Bjorn Andersson wrote:
> On Thu 02 May 04:53 PDT 2019, Vinod Koul wrote:
> > On 01-05-19, 17:19, Bjorn Andersson wrote:
> [..]
> > > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > > index 0ed235d560e3..d740cbe0e56d 100644
> > > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > > @@ -112,10 +112,10 @@ struct qcom_pcie_resources_2_3_2 {
> > >  	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
> > >  };
> > >  
> > > +#define QCOM_PCIE_2_4_0_MAX_CLOCKS	3
> > 
> > empty line after the define please
> > 
> 
> This follows the style of QCOM_PCIE_2_3_2_MAX_SUPPLY one block up, so
> I think this is the way we want it.

Okay sounds fine to me

> 
> > >  struct qcom_pcie_resources_2_4_0 {
> [..]
> > 
> > 
> > rest lgtm:
> > 
> > Reviewed-by: Vinod Koul <vkoul@kernel.org>
> > 
> 
> Thanks!
> 
> Regards,
> Bjorn
Stanimir Varbanov May 16, 2019, 9:14 a.m. UTC | #4
Hi Bjorn,

On 5/2/19 3:19 AM, Bjorn Andersson wrote:
> Before introducing the QCS404 platform, which uses the same PCIe
> controller as IPQ4019, migrate this to use the bulk clock API, in order
> to make the error paths slighly cleaner.
> 
> Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> 
> Changes since v2:
> - Defined QCOM_PCIE_2_4_0_MAX_CLOCKS
> 
>  drivers/pci/controller/dwc/pcie-qcom.c | 49 ++++++++------------------
>  1 file changed, 14 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> index 0ed235d560e3..d740cbe0e56d 100644
> --- a/drivers/pci/controller/dwc/pcie-qcom.c
> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> @@ -112,10 +112,10 @@ struct qcom_pcie_resources_2_3_2 {
>  	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
>  };
>  
> +#define QCOM_PCIE_2_4_0_MAX_CLOCKS	3
>  struct qcom_pcie_resources_2_4_0 {
> -	struct clk *aux_clk;
> -	struct clk *master_clk;
> -	struct clk *slave_clk;
> +	struct clk_bulk_data clks[QCOM_PCIE_2_4_0_MAX_CLOCKS];
> +	int num_clks;
>  	struct reset_control *axi_m_reset;
>  	struct reset_control *axi_s_reset;
>  	struct reset_control *pipe_reset;
> @@ -638,18 +638,17 @@ static int qcom_pcie_get_resources_2_4_0(struct qcom_pcie *pcie)
>  	struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
>  	struct dw_pcie *pci = pcie->pci;
>  	struct device *dev = pci->dev;
> +	int ret;
>  
> -	res->aux_clk = devm_clk_get(dev, "aux");
> -	if (IS_ERR(res->aux_clk))
> -		return PTR_ERR(res->aux_clk);
> +	res->clks[0].id = "aux";
> +	res->clks[1].id = "master_bus";
> +	res->clks[2].id = "slave_bus";
>  
> -	res->master_clk = devm_clk_get(dev, "master_bus");
> -	if (IS_ERR(res->master_clk))
> -		return PTR_ERR(res->master_clk);
> +	res->num_clks = 3;

Use the new fresh define QCOM_PCIE_2_4_0_MAX_CLOCKS?

>  
> -	res->slave_clk = devm_clk_get(dev, "slave_bus");
> -	if (IS_ERR(res->slave_clk))
> -		return PTR_ERR(res->slave_clk);
> +	ret = devm_clk_bulk_get(dev, res->num_clks, res->clks);
> +	if (ret < 0)
> +		return ret;
>  
>  	res->axi_m_reset = devm_reset_control_get_exclusive(dev, "axi_m");
>  	if (IS_ERR(res->axi_m_reset))
> @@ -719,9 +718,7 @@ static void qcom_pcie_deinit_2_4_0(struct qcom_pcie *pcie)
>  	reset_control_assert(res->axi_m_sticky_reset);
>  	reset_control_assert(res->pwr_reset);
>  	reset_control_assert(res->ahb_reset);
> -	clk_disable_unprepare(res->aux_clk);
> -	clk_disable_unprepare(res->master_clk);
> -	clk_disable_unprepare(res->slave_clk);
> +	clk_bulk_disable_unprepare(res->num_clks, res->clks);
>  }
>  
>  static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> @@ -850,23 +847,9 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
>  
>  	usleep_range(10000, 12000);
>  
> -	ret = clk_prepare_enable(res->aux_clk);
> -	if (ret) {
> -		dev_err(dev, "cannot prepare/enable iface clock\n");
> +	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
> +	if (ret)
>  		goto err_clk_aux;

Maybe you have to change the name of the label too?

> -	}
> -
> -	ret = clk_prepare_enable(res->master_clk);
> -	if (ret) {
> -		dev_err(dev, "cannot prepare/enable core clock\n");
> -		goto err_clk_axi_m;
> -	}
> -
> -	ret = clk_prepare_enable(res->slave_clk);
> -	if (ret) {
> -		dev_err(dev, "cannot prepare/enable phy clock\n");
> -		goto err_clk_axi_s;
> -	}
>  
>  	/* enable PCIe clocks and resets */
>  	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> @@ -891,10 +874,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
>  
>  	return 0;
>  
> -err_clk_axi_s:
> -	clk_disable_unprepare(res->master_clk);
> -err_clk_axi_m:
> -	clk_disable_unprepare(res->aux_clk);
>  err_clk_aux:
>  	reset_control_assert(res->ahb_reset);
>  err_rst_ahb:
>
Lorenzo Pieralisi May 28, 2019, 3:13 p.m. UTC | #5
On Thu, May 16, 2019 at 12:14:04PM +0300, Stanimir Varbanov wrote:
> Hi Bjorn,
> 
> On 5/2/19 3:19 AM, Bjorn Andersson wrote:
> > Before introducing the QCS404 platform, which uses the same PCIe
> > controller as IPQ4019, migrate this to use the bulk clock API, in order
> > to make the error paths slighly cleaner.
> > 
> > Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> > Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > 
> > Changes since v2:
> > - Defined QCOM_PCIE_2_4_0_MAX_CLOCKS
> > 
> >  drivers/pci/controller/dwc/pcie-qcom.c | 49 ++++++++------------------
> >  1 file changed, 14 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 0ed235d560e3..d740cbe0e56d 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -112,10 +112,10 @@ struct qcom_pcie_resources_2_3_2 {
> >  	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
> >  };
> >  
> > +#define QCOM_PCIE_2_4_0_MAX_CLOCKS	3
> >  struct qcom_pcie_resources_2_4_0 {
> > -	struct clk *aux_clk;
> > -	struct clk *master_clk;
> > -	struct clk *slave_clk;
> > +	struct clk_bulk_data clks[QCOM_PCIE_2_4_0_MAX_CLOCKS];
> > +	int num_clks;
> >  	struct reset_control *axi_m_reset;
> >  	struct reset_control *axi_s_reset;
> >  	struct reset_control *pipe_reset;
> > @@ -638,18 +638,17 @@ static int qcom_pcie_get_resources_2_4_0(struct qcom_pcie *pcie)
> >  	struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
> >  	struct dw_pcie *pci = pcie->pci;
> >  	struct device *dev = pci->dev;
> > +	int ret;
> >  
> > -	res->aux_clk = devm_clk_get(dev, "aux");
> > -	if (IS_ERR(res->aux_clk))
> > -		return PTR_ERR(res->aux_clk);
> > +	res->clks[0].id = "aux";
> > +	res->clks[1].id = "master_bus";
> > +	res->clks[2].id = "slave_bus";
> >  
> > -	res->master_clk = devm_clk_get(dev, "master_bus");
> > -	if (IS_ERR(res->master_clk))
> > -		return PTR_ERR(res->master_clk);
> > +	res->num_clks = 3;
> 
> Use the new fresh define QCOM_PCIE_2_4_0_MAX_CLOCKS?
> 
> >  
> > -	res->slave_clk = devm_clk_get(dev, "slave_bus");
> > -	if (IS_ERR(res->slave_clk))
> > -		return PTR_ERR(res->slave_clk);
> > +	ret = devm_clk_bulk_get(dev, res->num_clks, res->clks);
> > +	if (ret < 0)
> > +		return ret;
> >  
> >  	res->axi_m_reset = devm_reset_control_get_exclusive(dev, "axi_m");
> >  	if (IS_ERR(res->axi_m_reset))
> > @@ -719,9 +718,7 @@ static void qcom_pcie_deinit_2_4_0(struct qcom_pcie *pcie)
> >  	reset_control_assert(res->axi_m_sticky_reset);
> >  	reset_control_assert(res->pwr_reset);
> >  	reset_control_assert(res->ahb_reset);
> > -	clk_disable_unprepare(res->aux_clk);
> > -	clk_disable_unprepare(res->master_clk);
> > -	clk_disable_unprepare(res->slave_clk);
> > +	clk_bulk_disable_unprepare(res->num_clks, res->clks);
> >  }
> >  
> >  static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> > @@ -850,23 +847,9 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> >  
> >  	usleep_range(10000, 12000);
> >  
> > -	ret = clk_prepare_enable(res->aux_clk);
> > -	if (ret) {
> > -		dev_err(dev, "cannot prepare/enable iface clock\n");
> > +	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
> > +	if (ret)
> >  		goto err_clk_aux;
> 
> Maybe you have to change the name of the label too?
> 
> > -	}
> > -
> > -	ret = clk_prepare_enable(res->master_clk);
> > -	if (ret) {
> > -		dev_err(dev, "cannot prepare/enable core clock\n");
> > -		goto err_clk_axi_m;
> > -	}
> > -
> > -	ret = clk_prepare_enable(res->slave_clk);
> > -	if (ret) {
> > -		dev_err(dev, "cannot prepare/enable phy clock\n");
> > -		goto err_clk_axi_s;
> > -	}
> >  
> >  	/* enable PCIe clocks and resets */
> >  	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
> > @@ -891,10 +874,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> >  
> >  	return 0;
> >  
> > -err_clk_axi_s:
> > -	clk_disable_unprepare(res->master_clk);
> > -err_clk_axi_m:
> > -	clk_disable_unprepare(res->aux_clk);
> >  err_clk_aux:
> >  	reset_control_assert(res->ahb_reset);
> >  err_rst_ahb:

Hi Bjorn, Stanimir,

can I merge the series as-is or we need a v4 for the requested
updates ? Please let me know.

Thanks,
Lorenzo
Stanimir Varbanov May 28, 2019, 8:42 p.m. UTC | #6
Hi Lorenzo,

On 28.05.19 г. 18:13 ч., Lorenzo Pieralisi wrote:
> On Thu, May 16, 2019 at 12:14:04PM +0300, Stanimir Varbanov wrote:
>> Hi Bjorn,
>>
>> On 5/2/19 3:19 AM, Bjorn Andersson wrote:
>>> Before introducing the QCS404 platform, which uses the same PCIe
>>> controller as IPQ4019, migrate this to use the bulk clock API, in order
>>> to make the error paths slighly cleaner.
>>>
>>> Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
>>> Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
>>> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>> ---
>>>
>>> Changes since v2:
>>> - Defined QCOM_PCIE_2_4_0_MAX_CLOCKS
>>>
>>>   drivers/pci/controller/dwc/pcie-qcom.c | 49 ++++++++------------------
>>>   1 file changed, 14 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
>>> index 0ed235d560e3..d740cbe0e56d 100644
>>> --- a/drivers/pci/controller/dwc/pcie-qcom.c
>>> +++ b/drivers/pci/controller/dwc/pcie-qcom.c
>>> @@ -112,10 +112,10 @@ struct qcom_pcie_resources_2_3_2 {
>>>   	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
>>>   };
>>>   
>>> +#define QCOM_PCIE_2_4_0_MAX_CLOCKS	3
>>>   struct qcom_pcie_resources_2_4_0 {
>>> -	struct clk *aux_clk;
>>> -	struct clk *master_clk;
>>> -	struct clk *slave_clk;
>>> +	struct clk_bulk_data clks[QCOM_PCIE_2_4_0_MAX_CLOCKS];
>>> +	int num_clks;
>>>   	struct reset_control *axi_m_reset;
>>>   	struct reset_control *axi_s_reset;
>>>   	struct reset_control *pipe_reset;
>>> @@ -638,18 +638,17 @@ static int qcom_pcie_get_resources_2_4_0(struct qcom_pcie *pcie)
>>>   	struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
>>>   	struct dw_pcie *pci = pcie->pci;
>>>   	struct device *dev = pci->dev;
>>> +	int ret;
>>>   
>>> -	res->aux_clk = devm_clk_get(dev, "aux");
>>> -	if (IS_ERR(res->aux_clk))
>>> -		return PTR_ERR(res->aux_clk);
>>> +	res->clks[0].id = "aux";
>>> +	res->clks[1].id = "master_bus";
>>> +	res->clks[2].id = "slave_bus";
>>>   
>>> -	res->master_clk = devm_clk_get(dev, "master_bus");
>>> -	if (IS_ERR(res->master_clk))
>>> -		return PTR_ERR(res->master_clk);
>>> +	res->num_clks = 3;
>>
>> Use the new fresh define QCOM_PCIE_2_4_0_MAX_CLOCKS?
>>
>>>   
>>> -	res->slave_clk = devm_clk_get(dev, "slave_bus");
>>> -	if (IS_ERR(res->slave_clk))
>>> -		return PTR_ERR(res->slave_clk);
>>> +	ret = devm_clk_bulk_get(dev, res->num_clks, res->clks);
>>> +	if (ret < 0)
>>> +		return ret;
>>>   
>>>   	res->axi_m_reset = devm_reset_control_get_exclusive(dev, "axi_m");
>>>   	if (IS_ERR(res->axi_m_reset))
>>> @@ -719,9 +718,7 @@ static void qcom_pcie_deinit_2_4_0(struct qcom_pcie *pcie)
>>>   	reset_control_assert(res->axi_m_sticky_reset);
>>>   	reset_control_assert(res->pwr_reset);
>>>   	reset_control_assert(res->ahb_reset);
>>> -	clk_disable_unprepare(res->aux_clk);
>>> -	clk_disable_unprepare(res->master_clk);
>>> -	clk_disable_unprepare(res->slave_clk);
>>> +	clk_bulk_disable_unprepare(res->num_clks, res->clks);
>>>   }
>>>   
>>>   static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
>>> @@ -850,23 +847,9 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
>>>   
>>>   	usleep_range(10000, 12000);
>>>   
>>> -	ret = clk_prepare_enable(res->aux_clk);
>>> -	if (ret) {
>>> -		dev_err(dev, "cannot prepare/enable iface clock\n");
>>> +	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
>>> +	if (ret)
>>>   		goto err_clk_aux;
>>
>> Maybe you have to change the name of the label too?
>>
>>> -	}
>>> -
>>> -	ret = clk_prepare_enable(res->master_clk);
>>> -	if (ret) {
>>> -		dev_err(dev, "cannot prepare/enable core clock\n");
>>> -		goto err_clk_axi_m;
>>> -	}
>>> -
>>> -	ret = clk_prepare_enable(res->slave_clk);
>>> -	if (ret) {
>>> -		dev_err(dev, "cannot prepare/enable phy clock\n");
>>> -		goto err_clk_axi_s;
>>> -	}
>>>   
>>>   	/* enable PCIe clocks and resets */
>>>   	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
>>> @@ -891,10 +874,6 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
>>>   
>>>   	return 0;
>>>   
>>> -err_clk_axi_s:
>>> -	clk_disable_unprepare(res->master_clk);
>>> -err_clk_axi_m:
>>> -	clk_disable_unprepare(res->aux_clk);
>>>   err_clk_aux:
>>>   	reset_control_assert(res->ahb_reset);
>>>   err_rst_ahb:
> 
> Hi Bjorn, Stanimir,
> 
> can I merge the series as-is or we need a v4 for the requested
> updates ? Please let me know.

I'm fine with either way:

Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>

regards,
Stan
Bjorn Andersson May 29, 2019, 12:58 a.m. UTC | #7
On Thu 16 May 02:14 PDT 2019, Stanimir Varbanov wrote:

> Hi Bjorn,
> 
> On 5/2/19 3:19 AM, Bjorn Andersson wrote:
> > Before introducing the QCS404 platform, which uses the same PCIe
> > controller as IPQ4019, migrate this to use the bulk clock API, in order
> > to make the error paths slighly cleaner.
> > 
> > Acked-by: Stanimir Varbanov <svarbanov@mm-sol.com>
> > Reviewed-by: Niklas Cassel <niklas.cassel@linaro.org>
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > 
> > Changes since v2:
> > - Defined QCOM_PCIE_2_4_0_MAX_CLOCKS
> > 
> >  drivers/pci/controller/dwc/pcie-qcom.c | 49 ++++++++------------------
> >  1 file changed, 14 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
> > index 0ed235d560e3..d740cbe0e56d 100644
> > --- a/drivers/pci/controller/dwc/pcie-qcom.c
> > +++ b/drivers/pci/controller/dwc/pcie-qcom.c
> > @@ -112,10 +112,10 @@ struct qcom_pcie_resources_2_3_2 {
> >  	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
> >  };
> >  
> > +#define QCOM_PCIE_2_4_0_MAX_CLOCKS	3
> >  struct qcom_pcie_resources_2_4_0 {
> > -	struct clk *aux_clk;
> > -	struct clk *master_clk;
> > -	struct clk *slave_clk;
> > +	struct clk_bulk_data clks[QCOM_PCIE_2_4_0_MAX_CLOCKS];
> > +	int num_clks;
> >  	struct reset_control *axi_m_reset;
> >  	struct reset_control *axi_s_reset;
> >  	struct reset_control *pipe_reset;
> > @@ -638,18 +638,17 @@ static int qcom_pcie_get_resources_2_4_0(struct qcom_pcie *pcie)
> >  	struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
> >  	struct dw_pcie *pci = pcie->pci;
> >  	struct device *dev = pci->dev;
> > +	int ret;
> >  
> > -	res->aux_clk = devm_clk_get(dev, "aux");
> > -	if (IS_ERR(res->aux_clk))
> > -		return PTR_ERR(res->aux_clk);
> > +	res->clks[0].id = "aux";
> > +	res->clks[1].id = "master_bus";
> > +	res->clks[2].id = "slave_bus";
> >  
> > -	res->master_clk = devm_clk_get(dev, "master_bus");
> > -	if (IS_ERR(res->master_clk))
> > -		return PTR_ERR(res->master_clk);
> > +	res->num_clks = 3;
> 
> Use the new fresh define QCOM_PCIE_2_4_0_MAX_CLOCKS?
> 

As I replace it in patch 3/3 with a value different from "max clocks", I
don't think it makes sense to use the define here. So I'm leaving this
as is.

> >  
> > -	res->slave_clk = devm_clk_get(dev, "slave_bus");
> > -	if (IS_ERR(res->slave_clk))
> > -		return PTR_ERR(res->slave_clk);
> > +	ret = devm_clk_bulk_get(dev, res->num_clks, res->clks);
> > +	if (ret < 0)
> > +		return ret;
> >  
> >  	res->axi_m_reset = devm_reset_control_get_exclusive(dev, "axi_m");
> >  	if (IS_ERR(res->axi_m_reset))
> > @@ -719,9 +718,7 @@ static void qcom_pcie_deinit_2_4_0(struct qcom_pcie *pcie)
> >  	reset_control_assert(res->axi_m_sticky_reset);
> >  	reset_control_assert(res->pwr_reset);
> >  	reset_control_assert(res->ahb_reset);
> > -	clk_disable_unprepare(res->aux_clk);
> > -	clk_disable_unprepare(res->master_clk);
> > -	clk_disable_unprepare(res->slave_clk);
> > +	clk_bulk_disable_unprepare(res->num_clks, res->clks);
> >  }
> >  
> >  static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> > @@ -850,23 +847,9 @@ static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
> >  
> >  	usleep_range(10000, 12000);
> >  
> > -	ret = clk_prepare_enable(res->aux_clk);
> > -	if (ret) {
> > -		dev_err(dev, "cannot prepare/enable iface clock\n");
> > +	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
> > +	if (ret)
> >  		goto err_clk_aux;
> 
> Maybe you have to change the name of the label too?
> 

Updated this and posted v5. Should be good to be merged now.

Thanks for your reviews!

Regards,
Bjorn
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 0ed235d560e3..d740cbe0e56d 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -112,10 +112,10 @@  struct qcom_pcie_resources_2_3_2 {
 	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
 };
 
+#define QCOM_PCIE_2_4_0_MAX_CLOCKS	3
 struct qcom_pcie_resources_2_4_0 {
-	struct clk *aux_clk;
-	struct clk *master_clk;
-	struct clk *slave_clk;
+	struct clk_bulk_data clks[QCOM_PCIE_2_4_0_MAX_CLOCKS];
+	int num_clks;
 	struct reset_control *axi_m_reset;
 	struct reset_control *axi_s_reset;
 	struct reset_control *pipe_reset;
@@ -638,18 +638,17 @@  static int qcom_pcie_get_resources_2_4_0(struct qcom_pcie *pcie)
 	struct qcom_pcie_resources_2_4_0 *res = &pcie->res.v2_4_0;
 	struct dw_pcie *pci = pcie->pci;
 	struct device *dev = pci->dev;
+	int ret;
 
-	res->aux_clk = devm_clk_get(dev, "aux");
-	if (IS_ERR(res->aux_clk))
-		return PTR_ERR(res->aux_clk);
+	res->clks[0].id = "aux";
+	res->clks[1].id = "master_bus";
+	res->clks[2].id = "slave_bus";
 
-	res->master_clk = devm_clk_get(dev, "master_bus");
-	if (IS_ERR(res->master_clk))
-		return PTR_ERR(res->master_clk);
+	res->num_clks = 3;
 
-	res->slave_clk = devm_clk_get(dev, "slave_bus");
-	if (IS_ERR(res->slave_clk))
-		return PTR_ERR(res->slave_clk);
+	ret = devm_clk_bulk_get(dev, res->num_clks, res->clks);
+	if (ret < 0)
+		return ret;
 
 	res->axi_m_reset = devm_reset_control_get_exclusive(dev, "axi_m");
 	if (IS_ERR(res->axi_m_reset))
@@ -719,9 +718,7 @@  static void qcom_pcie_deinit_2_4_0(struct qcom_pcie *pcie)
 	reset_control_assert(res->axi_m_sticky_reset);
 	reset_control_assert(res->pwr_reset);
 	reset_control_assert(res->ahb_reset);
-	clk_disable_unprepare(res->aux_clk);
-	clk_disable_unprepare(res->master_clk);
-	clk_disable_unprepare(res->slave_clk);
+	clk_bulk_disable_unprepare(res->num_clks, res->clks);
 }
 
 static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
@@ -850,23 +847,9 @@  static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
 
 	usleep_range(10000, 12000);
 
-	ret = clk_prepare_enable(res->aux_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable iface clock\n");
+	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
+	if (ret)
 		goto err_clk_aux;
-	}
-
-	ret = clk_prepare_enable(res->master_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable core clock\n");
-		goto err_clk_axi_m;
-	}
-
-	ret = clk_prepare_enable(res->slave_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable phy clock\n");
-		goto err_clk_axi_s;
-	}
 
 	/* enable PCIe clocks and resets */
 	val = readl(pcie->parf + PCIE20_PARF_PHY_CTRL);
@@ -891,10 +874,6 @@  static int qcom_pcie_init_2_4_0(struct qcom_pcie *pcie)
 
 	return 0;
 
-err_clk_axi_s:
-	clk_disable_unprepare(res->master_clk);
-err_clk_axi_m:
-	clk_disable_unprepare(res->aux_clk);
 err_clk_aux:
 	reset_control_assert(res->ahb_reset);
 err_rst_ahb: