diff mbox series

[13/15] phy: qcom-qmp-pcie: add support for pipediv2 clock

Message ID 20221017145328.22090-14-johan+linaro@kernel.org
State New
Headers
Series phy: qcom-qmp-pcie: add support for sc8280xp |

Commit Message

Johan Hovold Oct. 17, 2022, 2:53 p.m. UTC 
  Some QMP PHYs have a second fixed-divider pipe clock that needs to be
enabled along with the pipe clock.

Add support for an optional "pipediv2" clock.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 42 ++++++++++++++++++++----
 1 file changed, 36 insertions(+), 6 deletions(-)

Comments

Dmitry Baryshkov Oct. 18, 2022, 1:05 p.m. UTC | #1 
On 17/10/2022 17:53, Johan Hovold wrote:
> Some QMP PHYs have a second fixed-divider pipe clock that needs to be
> enabled along with the pipe clock.
> 
> Add support for an optional "pipediv2" clock.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 42 ++++++++++++++++++++----
>   1 file changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> index 9c8e009033f1..c1d74c06fad1 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> @@ -1379,7 +1379,9 @@ struct qmp_pcie {
>   	void __iomem *rx2;
>   
>   	struct clk *pipe_clk;
> +	struct clk *pipediv2_clk;
>   	struct clk_bulk_data *clks;
> +
>   	struct reset_control_bulk_data *resets;
>   	struct regulator_bulk_data *vregs;
>   
> @@ -1902,6 +1904,36 @@ static int qmp_pcie_exit(struct phy *phy)
>   	return 0;
>   }
>   
> +static int pipe_clk_enable(struct qmp_pcie *qmp)
> +{
> +	int ret;
> +
> +	ret = clk_prepare_enable(qmp->pipe_clk);
> +	if (ret) {
> +		dev_err(qmp->dev, "failed to enable pipe clock: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(qmp->pipediv2_clk);
> +	if (ret) {
> +		dev_err(qmp->dev, "failed to enable pipediv2 clock: %d\n", ret);
> +		goto err_disable_pipe_clk;
> +	}

Do they have to be enabled in sequence? If not, I'd use a bulk clocks 
here for the pipe clocks. While it can look like an overkill, it would 
be a safe net for the possible future changes, which might include 
additional clocks.

> +
> +	return 0;
> +
> +err_disable_pipe_clk:
> +	clk_disable_unprepare(qmp->pipe_clk);
> +
> +	return ret;
> +}
> +
> +static void pipe_clk_disable(struct qmp_pcie *qmp)
> +{
> +	clk_disable_unprepare(qmp->pipediv2_clk);
> +	clk_disable_unprepare(qmp->pipe_clk);
> +}
> +
>   static int qmp_pcie_power_on(struct phy *phy)
>   {
>   	struct qmp_pcie *qmp = phy_get_drvdata(phy);
> @@ -1923,11 +1955,9 @@ static int qmp_pcie_power_on(struct phy *phy)
>   	qmp_pcie_init_registers(qmp, &cfg->tables);
>   	qmp_pcie_init_registers(qmp, mode_tables);
>   
> -	ret = clk_prepare_enable(qmp->pipe_clk);
> -	if (ret) {
> -		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
> +	ret = pipe_clk_enable(qmp);
> +	if (ret)
>   		return ret;
> -	}
>   
>   	/* Pull PHY out of reset state */
>   	qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
> @@ -1950,7 +1980,7 @@ static int qmp_pcie_power_on(struct phy *phy)
>   	return 0;
>   
>   err_disable_pipe_clk:
> -	clk_disable_unprepare(qmp->pipe_clk);
> +	pipe_clk_disable(qmp);
>   
>   	return ret;
>   }
> @@ -1960,7 +1990,7 @@ static int qmp_pcie_power_off(struct phy *phy)
>   	struct qmp_pcie *qmp = phy_get_drvdata(phy);
>   	const struct qmp_phy_cfg *cfg = qmp->cfg;
>   
> -	clk_disable_unprepare(qmp->pipe_clk);
> +	pipe_clk_disable(qmp);
>   
>   	/* PHY reset */
>   	qphy_setbits(qmp->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
Johan Hovold Oct. 18, 2022, 2:53 p.m. UTC | #2 
On Tue, Oct 18, 2022 at 04:05:29PM +0300, Dmitry Baryshkov wrote:
> On 17/10/2022 17:53, Johan Hovold wrote:
> > Some QMP PHYs have a second fixed-divider pipe clock that needs to be
> > enabled along with the pipe clock.
> > 
> > Add support for an optional "pipediv2" clock.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 42 ++++++++++++++++++++----
> >   1 file changed, 36 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > index 9c8e009033f1..c1d74c06fad1 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > @@ -1379,7 +1379,9 @@ struct qmp_pcie {
> >   	void __iomem *rx2;
> >   
> >   	struct clk *pipe_clk;
> > +	struct clk *pipediv2_clk;
> >   	struct clk_bulk_data *clks;
> > +
> >   	struct reset_control_bulk_data *resets;
> >   	struct regulator_bulk_data *vregs;
> >   
> > @@ -1902,6 +1904,36 @@ static int qmp_pcie_exit(struct phy *phy)
> >   	return 0;
> >   }
> >   
> > +static int pipe_clk_enable(struct qmp_pcie *qmp)
> > +{
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(qmp->pipe_clk);
> > +	if (ret) {
> > +		dev_err(qmp->dev, "failed to enable pipe clock: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = clk_prepare_enable(qmp->pipediv2_clk);
> > +	if (ret) {
> > +		dev_err(qmp->dev, "failed to enable pipediv2 clock: %d\n", ret);
> > +		goto err_disable_pipe_clk;
> > +	}
> 
> Do they have to be enabled in sequence? If not, I'd use a bulk clocks 
> here for the pipe clocks. While it can look like an overkill, it would 
> be a safe net for the possible future changes, which might include 
> additional clocks.

I don't believe the bulk API is a good fit here as we need to support
both the new and old bindings, and for the latter the pipe_clk is looked
up by index rather than name (and that's from the child node too which
limits which APIs you can use further).

The code is clear enough as it stands, and I don't think we need to take
height for a hypothetical third pipe clock just yet.

Johan
diff mbox series

Patch

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
index 9c8e009033f1..c1d74c06fad1 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
@@ -1379,7 +1379,9 @@  struct qmp_pcie {
 	void __iomem *rx2;
 
 	struct clk *pipe_clk;
+	struct clk *pipediv2_clk;
 	struct clk_bulk_data *clks;
+
 	struct reset_control_bulk_data *resets;
 	struct regulator_bulk_data *vregs;
 
@@ -1902,6 +1904,36 @@  static int qmp_pcie_exit(struct phy *phy)
 	return 0;
 }
 
+static int pipe_clk_enable(struct qmp_pcie *qmp)
+{
+	int ret;
+
+	ret = clk_prepare_enable(qmp->pipe_clk);
+	if (ret) {
+		dev_err(qmp->dev, "failed to enable pipe clock: %d\n", ret);
+		return ret;
+	}
+
+	ret = clk_prepare_enable(qmp->pipediv2_clk);
+	if (ret) {
+		dev_err(qmp->dev, "failed to enable pipediv2 clock: %d\n", ret);
+		goto err_disable_pipe_clk;
+	}
+
+	return 0;
+
+err_disable_pipe_clk:
+	clk_disable_unprepare(qmp->pipe_clk);
+
+	return ret;
+}
+
+static void pipe_clk_disable(struct qmp_pcie *qmp)
+{
+	clk_disable_unprepare(qmp->pipediv2_clk);
+	clk_disable_unprepare(qmp->pipe_clk);
+}
+
 static int qmp_pcie_power_on(struct phy *phy)
 {
 	struct qmp_pcie *qmp = phy_get_drvdata(phy);
@@ -1923,11 +1955,9 @@  static int qmp_pcie_power_on(struct phy *phy)
 	qmp_pcie_init_registers(qmp, &cfg->tables);
 	qmp_pcie_init_registers(qmp, mode_tables);
 
-	ret = clk_prepare_enable(qmp->pipe_clk);
-	if (ret) {
-		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
+	ret = pipe_clk_enable(qmp);
+	if (ret)
 		return ret;
-	}
 
 	/* Pull PHY out of reset state */
 	qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
@@ -1950,7 +1980,7 @@  static int qmp_pcie_power_on(struct phy *phy)
 	return 0;
 
 err_disable_pipe_clk:
-	clk_disable_unprepare(qmp->pipe_clk);
+	pipe_clk_disable(qmp);
 
 	return ret;
 }
@@ -1960,7 +1990,7 @@  static int qmp_pcie_power_off(struct phy *phy)
 	struct qmp_pcie *qmp = phy_get_drvdata(phy);
 	const struct qmp_phy_cfg *cfg = qmp->cfg;
 
-	clk_disable_unprepare(qmp->pipe_clk);
+	pipe_clk_disable(qmp);
 
 	/* PHY reset */
 	qphy_setbits(qmp->pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);