[PATCH v1 0/5] PCI: qcom: rework pipe_clk/pipe_clk_src handling

[PATCH v1 0/5] PCI: qcom: rework pipe_clk/pipe_clk_src handling

[Dmitry Baryshkov]

PCIe pipe clk (and some other clocks) must be parked to the "safe"
source (bi_tcxo) when corresponding GDSC is turned off and on again.
Currently this is handcoded in the PCIe driver by reparenting the
gcc_pipe_N_clk_src clock.

Instead of doing it manually, follow the approach used by
clk_rcg2_shared_ops and implement this parking in the enable() and
disable() clock operations for respective pipe clocks.

Changes since RFC:
 - Rework clk-regmap-mux fields. Specify safe parent as P_* value rather
   than specifying the register value directly
 - Expand commit message to the first patch to specially mention that
   it is required only on newer generations of Qualcomm chipsets.

Dmitry Baryshkov (5):
  clk: qcom: regmap-mux: add pipe clk implementation
  clk: qcom: gcc-sm8450: use new clk_regmap_mux_safe_ops for PCIe pipe
    clocks
  clk: qcom: gcc-sc7280: use new clk_regmap_mux_safe_ops for PCIe pipe
    clocks
  PCI: qcom: Remove unnecessary pipe_clk handling
  PCI: qcom: Drop manual pipe_clk_src handling

 drivers/clk/qcom/clk-regmap-mux.c      | 78 +++++++++++++++++++++++
 drivers/clk/qcom/clk-regmap-mux.h      |  3 +
 drivers/clk/qcom/gcc-sc7280.c          |  6 +-
 drivers/clk/qcom/gcc-sm8450.c          |  6 +-
 drivers/pci/controller/dwc/pcie-qcom.c | 87 +-------------------------
 5 files changed, 92 insertions(+), 88 deletions(-)

-- 
2.35.1

[PATCH v1 1/5] clk: qcom: regmap-mux: add pipe clk implementation

[Dmitry Baryshkov]

On recent Qualcomm platforms the QMP PIPE clocks feed into a set of
muxes which must be parked to the "safe" source (bi_tcxo) when
corresponding GDSC is turned off and on again. Currently this is
handcoded in the PCIe driver by reparenting the gcc_pipe_N_clk_src
clock. However the same code sequence should be applied in the
pcie-qcom endpoint, USB3 and UFS drivers.

Rather than copying this sequence over and over again, follow the
example of clk_rcg2_shared_ops and implement this parking in the
enable() and disable() clock operations. As we are changing the parent
behind the back of the clock framework, also implement custom
set_parent() and get_parent() operations behaving accroding to the clock
framework expectations (cache the new parent if the clock is in disabled
state, return cached parent).

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/clk-regmap-mux.c | 78 +++++++++++++++++++++++++++++++
 drivers/clk/qcom/clk-regmap-mux.h |  3 ++
 2 files changed, 81 insertions(+)

diff --git a/drivers/clk/qcom/clk-regmap-mux.c b/drivers/clk/qcom/clk-regmap-mux.c
index 45d9cca28064..c39ee783ee83 100644
--- a/drivers/clk/qcom/clk-regmap-mux.c
+++ b/drivers/clk/qcom/clk-regmap-mux.c
@@ -49,9 +49,87 @@ static int mux_set_parent(struct clk_hw *hw, u8 index)
 	return regmap_update_bits(clkr->regmap, mux->reg, mask, val);
 }
 
+static u8 mux_safe_get_parent(struct clk_hw *hw)
+{
+	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
+	unsigned int val;
+
+	if (clk_hw_is_enabled(hw))
+		return mux_get_parent(hw);
+
+	val = mux->stored_parent_cfg;
+
+	if (mux->parent_map)
+		return qcom_find_cfg_index(hw, mux->parent_map, val);
+
+	return val;
+}
+
+static int mux_safe_set_parent(struct clk_hw *hw, u8 index)
+{
+	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
+
+	if (clk_hw_is_enabled(hw))
+		return mux_set_parent(hw, index);
+
+	if (mux->parent_map)
+		index = mux->parent_map[index].cfg;
+
+	mux->stored_parent_cfg = index;
+
+	return 0;
+}
+
+static void mux_safe_disable(struct clk_hw *hw)
+{
+	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
+	struct clk_regmap *clkr = to_clk_regmap(hw);
+	unsigned int mask = GENMASK(mux->width + mux->shift - 1, mux->shift);
+	unsigned int val;
+
+	regmap_read(clkr->regmap, mux->reg, &val);
+
+	mux->stored_parent_cfg = (val & mask) >> mux->shift;
+
+	val = mux->safe_src_parent;
+	if (mux->parent_map) {
+		int index = qcom_find_src_index(hw, mux->parent_map, val);
+
+		if (WARN_ON(index < 0))
+			return;
+
+		val = mux->parent_map[index].cfg;
+	}
+	val <<= mux->shift;
+
+	regmap_update_bits(clkr->regmap, mux->reg, mask, val);
+}
+
+static int mux_safe_enable(struct clk_hw *hw)
+{
+	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
+	struct clk_regmap *clkr = to_clk_regmap(hw);
+	unsigned int mask = GENMASK(mux->width + mux->shift - 1, mux->shift);
+	unsigned int val;
+
+	val = mux->stored_parent_cfg;
+	val <<= mux->shift;
+
+	return regmap_update_bits(clkr->regmap, mux->reg, mask, val);
+}
+
 const struct clk_ops clk_regmap_mux_closest_ops = {
 	.get_parent = mux_get_parent,
 	.set_parent = mux_set_parent,
 	.determine_rate = __clk_mux_determine_rate_closest,
 };
 EXPORT_SYMBOL_GPL(clk_regmap_mux_closest_ops);
+
+const struct clk_ops clk_regmap_mux_safe_ops = {
+	.enable = mux_safe_enable,
+	.disable = mux_safe_disable,
+	.get_parent = mux_safe_get_parent,
+	.set_parent = mux_safe_set_parent,
+	.determine_rate = __clk_mux_determine_rate_closest,
+};
+EXPORT_SYMBOL_GPL(clk_regmap_mux_safe_ops);
diff --git a/drivers/clk/qcom/clk-regmap-mux.h b/drivers/clk/qcom/clk-regmap-mux.h
index db6f4cdd9586..f86c674ce139 100644
--- a/drivers/clk/qcom/clk-regmap-mux.h
+++ b/drivers/clk/qcom/clk-regmap-mux.h
@@ -14,10 +14,13 @@ struct clk_regmap_mux {
 	u32			reg;
 	u32			shift;
 	u32			width;
+	u8			safe_src_parent;
+	u8			stored_parent_cfg;
 	const struct parent_map	*parent_map;
 	struct clk_regmap	clkr;
 };
 
 extern const struct clk_ops clk_regmap_mux_closest_ops;
+extern const struct clk_ops clk_regmap_mux_safe_ops;
 
 #endif
-- 
2.35.1

Re: [PATCH v1 1/5] clk: qcom: regmap-mux: add pipe clk implementation

[Lorenzo Pieralisi]

On Wed, Mar 23, 2022 at 11:50:06AM +0300, Dmitry Baryshkov wrote:
> On recent Qualcomm platforms the QMP PIPE clocks feed into a set of
> muxes which must be parked to the "safe" source (bi_tcxo) when
> corresponding GDSC is turned off and on again. Currently this is
> handcoded in the PCIe driver by reparenting the gcc_pipe_N_clk_src
> clock. However the same code sequence should be applied in the
> pcie-qcom endpoint, USB3 and UFS drivers.
> 
> Rather than copying this sequence over and over again, follow the
> example of clk_rcg2_shared_ops and implement this parking in the
> enable() and disable() clock operations. As we are changing the parent
> behind the back of the clock framework, also implement custom
> set_parent() and get_parent() operations behaving accroding to the clock
> framework expectations (cache the new parent if the clock is in disabled
> state, return cached parent).
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/clk/qcom/clk-regmap-mux.c | 78 +++++++++++++++++++++++++++++++
>  drivers/clk/qcom/clk-regmap-mux.h |  3 ++
>  2 files changed, 81 insertions(+)

Need BjornA's ACK on this patch and I can pull the series then.

Lorenzo

> diff --git a/drivers/clk/qcom/clk-regmap-mux.c b/drivers/clk/qcom/clk-regmap-mux.c
> index 45d9cca28064..c39ee783ee83 100644
> --- a/drivers/clk/qcom/clk-regmap-mux.c
> +++ b/drivers/clk/qcom/clk-regmap-mux.c
> @@ -49,9 +49,87 @@ static int mux_set_parent(struct clk_hw *hw, u8 index)
>  	return regmap_update_bits(clkr->regmap, mux->reg, mask, val);
>  }
>  
> +static u8 mux_safe_get_parent(struct clk_hw *hw)
> +{
> +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
> +	unsigned int val;
> +
> +	if (clk_hw_is_enabled(hw))
> +		return mux_get_parent(hw);
> +
> +	val = mux->stored_parent_cfg;
> +
> +	if (mux->parent_map)
> +		return qcom_find_cfg_index(hw, mux->parent_map, val);
> +
> +	return val;
> +}
> +
> +static int mux_safe_set_parent(struct clk_hw *hw, u8 index)
> +{
> +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
> +
> +	if (clk_hw_is_enabled(hw))
> +		return mux_set_parent(hw, index);
> +
> +	if (mux->parent_map)
> +		index = mux->parent_map[index].cfg;
> +
> +	mux->stored_parent_cfg = index;
> +
> +	return 0;
> +}
> +
> +static void mux_safe_disable(struct clk_hw *hw)
> +{
> +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
> +	struct clk_regmap *clkr = to_clk_regmap(hw);
> +	unsigned int mask = GENMASK(mux->width + mux->shift - 1, mux->shift);
> +	unsigned int val;
> +
> +	regmap_read(clkr->regmap, mux->reg, &val);
> +
> +	mux->stored_parent_cfg = (val & mask) >> mux->shift;
> +
> +	val = mux->safe_src_parent;
> +	if (mux->parent_map) {
> +		int index = qcom_find_src_index(hw, mux->parent_map, val);
> +
> +		if (WARN_ON(index < 0))
> +			return;
> +
> +		val = mux->parent_map[index].cfg;
> +	}
> +	val <<= mux->shift;
> +
> +	regmap_update_bits(clkr->regmap, mux->reg, mask, val);
> +}
> +
> +static int mux_safe_enable(struct clk_hw *hw)
> +{
> +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
> +	struct clk_regmap *clkr = to_clk_regmap(hw);
> +	unsigned int mask = GENMASK(mux->width + mux->shift - 1, mux->shift);
> +	unsigned int val;
> +
> +	val = mux->stored_parent_cfg;
> +	val <<= mux->shift;
> +
> +	return regmap_update_bits(clkr->regmap, mux->reg, mask, val);
> +}
> +
>  const struct clk_ops clk_regmap_mux_closest_ops = {
>  	.get_parent = mux_get_parent,
>  	.set_parent = mux_set_parent,
>  	.determine_rate = __clk_mux_determine_rate_closest,
>  };
>  EXPORT_SYMBOL_GPL(clk_regmap_mux_closest_ops);
> +
> +const struct clk_ops clk_regmap_mux_safe_ops = {
> +	.enable = mux_safe_enable,
> +	.disable = mux_safe_disable,
> +	.get_parent = mux_safe_get_parent,
> +	.set_parent = mux_safe_set_parent,
> +	.determine_rate = __clk_mux_determine_rate_closest,
> +};
> +EXPORT_SYMBOL_GPL(clk_regmap_mux_safe_ops);
> diff --git a/drivers/clk/qcom/clk-regmap-mux.h b/drivers/clk/qcom/clk-regmap-mux.h
> index db6f4cdd9586..f86c674ce139 100644
> --- a/drivers/clk/qcom/clk-regmap-mux.h
> +++ b/drivers/clk/qcom/clk-regmap-mux.h
> @@ -14,10 +14,13 @@ struct clk_regmap_mux {
>  	u32			reg;
>  	u32			shift;
>  	u32			width;
> +	u8			safe_src_parent;
> +	u8			stored_parent_cfg;
>  	const struct parent_map	*parent_map;
>  	struct clk_regmap	clkr;
>  };
>  
>  extern const struct clk_ops clk_regmap_mux_closest_ops;
> +extern const struct clk_ops clk_regmap_mux_safe_ops;
>  
>  #endif
> -- 
> 2.35.1
> 

Re: [PATCH v1 1/5] clk: qcom: regmap-mux: add pipe clk implementation

[Bjorn Andersson]

On Fri 08 Apr 06:15 CDT 2022, Lorenzo Pieralisi wrote:

> On Wed, Mar 23, 2022 at 11:50:06AM +0300, Dmitry Baryshkov wrote:
> > On recent Qualcomm platforms the QMP PIPE clocks feed into a set of
> > muxes which must be parked to the "safe" source (bi_tcxo) when
> > corresponding GDSC is turned off and on again. Currently this is
> > handcoded in the PCIe driver by reparenting the gcc_pipe_N_clk_src
> > clock. However the same code sequence should be applied in the
> > pcie-qcom endpoint, USB3 and UFS drivers.
> > 
> > Rather than copying this sequence over and over again, follow the
> > example of clk_rcg2_shared_ops and implement this parking in the
> > enable() and disable() clock operations. As we are changing the parent
> > behind the back of the clock framework, also implement custom
> > set_parent() and get_parent() operations behaving accroding to the clock
> > framework expectations (cache the new parent if the clock is in disabled
> > state, return cached parent).
> > 
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  drivers/clk/qcom/clk-regmap-mux.c | 78 +++++++++++++++++++++++++++++++
> >  drivers/clk/qcom/clk-regmap-mux.h |  3 ++
> >  2 files changed, 81 insertions(+)
> 
> Need BjornA's ACK on this patch and I can pull the series then.
> 

It seems I have a few more clock patches in the queue which depends on
top of this, so I picked up the three clock branches and pushed a tag
for you to pick up, Lorenzo.

The following changes since commit a9ed9e2bf7940353d2ffa4faa2ad2b75a24f3ac0:

  clk: qcom: gcc-sc7280: use new clk_regmap_mux_safe_ops for PCIe pipe clocks (2022-04-12 13:32:58 -0500)

are available in the Git repository at:

  https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git tags/20220323085010.1753493-4-dmitry.baryshkov@linaro.org

for you to fetch changes up to a9ed9e2bf7940353d2ffa4faa2ad2b75a24f3ac0:

  clk: qcom: gcc-sc7280: use new clk_regmap_mux_safe_ops for PCIe pipe clocks (2022-04-12 13:32:58 -0500)

----------------------------------------------------------------
v5.18-rc1 +
20220323085010.1753493-2-dmitry.baryshkov@linaro.org +
20220323085010.1753493-3-dmitry.baryshkov@linaro.org +
20220323085010.1753493-4-dmitry.baryshkov@linaro.org

----------------------------------------------------------------

Hope this works for you.

Regards,
Bjorn

> Lorenzo
> 
> > diff --git a/drivers/clk/qcom/clk-regmap-mux.c b/drivers/clk/qcom/clk-regmap-mux.c
> > index 45d9cca28064..c39ee783ee83 100644
> > --- a/drivers/clk/qcom/clk-regmap-mux.c
> > +++ b/drivers/clk/qcom/clk-regmap-mux.c
> > @@ -49,9 +49,87 @@ static int mux_set_parent(struct clk_hw *hw, u8 index)
> >  	return regmap_update_bits(clkr->regmap, mux->reg, mask, val);
> >  }
> >  
> > +static u8 mux_safe_get_parent(struct clk_hw *hw)
> > +{
> > +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
> > +	unsigned int val;
> > +
> > +	if (clk_hw_is_enabled(hw))
> > +		return mux_get_parent(hw);
> > +
> > +	val = mux->stored_parent_cfg;
> > +
> > +	if (mux->parent_map)
> > +		return qcom_find_cfg_index(hw, mux->parent_map, val);
> > +
> > +	return val;
> > +}
> > +
> > +static int mux_safe_set_parent(struct clk_hw *hw, u8 index)
> > +{
> > +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
> > +
> > +	if (clk_hw_is_enabled(hw))
> > +		return mux_set_parent(hw, index);
> > +
> > +	if (mux->parent_map)
> > +		index = mux->parent_map[index].cfg;
> > +
> > +	mux->stored_parent_cfg = index;
> > +
> > +	return 0;
> > +}
> > +
> > +static void mux_safe_disable(struct clk_hw *hw)
> > +{
> > +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
> > +	struct clk_regmap *clkr = to_clk_regmap(hw);
> > +	unsigned int mask = GENMASK(mux->width + mux->shift - 1, mux->shift);
> > +	unsigned int val;
> > +
> > +	regmap_read(clkr->regmap, mux->reg, &val);
> > +
> > +	mux->stored_parent_cfg = (val & mask) >> mux->shift;
> > +
> > +	val = mux->safe_src_parent;
> > +	if (mux->parent_map) {
> > +		int index = qcom_find_src_index(hw, mux->parent_map, val);
> > +
> > +		if (WARN_ON(index < 0))
> > +			return;
> > +
> > +		val = mux->parent_map[index].cfg;
> > +	}
> > +	val <<= mux->shift;
> > +
> > +	regmap_update_bits(clkr->regmap, mux->reg, mask, val);
> > +}
> > +
> > +static int mux_safe_enable(struct clk_hw *hw)
> > +{
> > +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
> > +	struct clk_regmap *clkr = to_clk_regmap(hw);
> > +	unsigned int mask = GENMASK(mux->width + mux->shift - 1, mux->shift);
> > +	unsigned int val;
> > +
> > +	val = mux->stored_parent_cfg;
> > +	val <<= mux->shift;
> > +
> > +	return regmap_update_bits(clkr->regmap, mux->reg, mask, val);
> > +}
> > +
> >  const struct clk_ops clk_regmap_mux_closest_ops = {
> >  	.get_parent = mux_get_parent,
> >  	.set_parent = mux_set_parent,
> >  	.determine_rate = __clk_mux_determine_rate_closest,
> >  };
> >  EXPORT_SYMBOL_GPL(clk_regmap_mux_closest_ops);
> > +
> > +const struct clk_ops clk_regmap_mux_safe_ops = {
> > +	.enable = mux_safe_enable,
> > +	.disable = mux_safe_disable,
> > +	.get_parent = mux_safe_get_parent,
> > +	.set_parent = mux_safe_set_parent,
> > +	.determine_rate = __clk_mux_determine_rate_closest,
> > +};
> > +EXPORT_SYMBOL_GPL(clk_regmap_mux_safe_ops);
> > diff --git a/drivers/clk/qcom/clk-regmap-mux.h b/drivers/clk/qcom/clk-regmap-mux.h
> > index db6f4cdd9586..f86c674ce139 100644
> > --- a/drivers/clk/qcom/clk-regmap-mux.h
> > +++ b/drivers/clk/qcom/clk-regmap-mux.h
> > @@ -14,10 +14,13 @@ struct clk_regmap_mux {
> >  	u32			reg;
> >  	u32			shift;
> >  	u32			width;
> > +	u8			safe_src_parent;
> > +	u8			stored_parent_cfg;
> >  	const struct parent_map	*parent_map;
> >  	struct clk_regmap	clkr;
> >  };
> >  
> >  extern const struct clk_ops clk_regmap_mux_closest_ops;
> > +extern const struct clk_ops clk_regmap_mux_safe_ops;
> >  
> >  #endif
> > -- 
> > 2.35.1
> > 

Re: [PATCH v1 1/5] clk: qcom: regmap-mux: add pipe clk implementation

[Johan Hovold]

On Tue, Apr 12, 2022 at 01:45:37PM -0500, Bjorn Andersson wrote:
> On Fri 08 Apr 06:15 CDT 2022, Lorenzo Pieralisi wrote:
> 
> > On Wed, Mar 23, 2022 at 11:50:06AM +0300, Dmitry Baryshkov wrote:
> > > On recent Qualcomm platforms the QMP PIPE clocks feed into a set of
> > > muxes which must be parked to the "safe" source (bi_tcxo) when
> > > corresponding GDSC is turned off and on again. Currently this is
> > > handcoded in the PCIe driver by reparenting the gcc_pipe_N_clk_src
> > > clock. However the same code sequence should be applied in the
> > > pcie-qcom endpoint, USB3 and UFS drivers.
> > > 
> > > Rather than copying this sequence over and over again, follow the
> > > example of clk_rcg2_shared_ops and implement this parking in the
> > > enable() and disable() clock operations. As we are changing the parent
> > > behind the back of the clock framework, also implement custom
> > > set_parent() and get_parent() operations behaving accroding to the clock
> > > framework expectations (cache the new parent if the clock is in disabled
> > > state, return cached parent).
> > > 
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >  drivers/clk/qcom/clk-regmap-mux.c | 78 +++++++++++++++++++++++++++++++
> > >  drivers/clk/qcom/clk-regmap-mux.h |  3 ++
> > >  2 files changed, 81 insertions(+)
> > 
> > Need BjornA's ACK on this patch and I can pull the series then.
> > 
> 
> It seems I have a few more clock patches in the queue which depends on
> top of this, so I picked up the three clock branches and pushed a tag
> for you to pick up, Lorenzo.

I've found a few issues with these clock patches and I'm starting to
think we should consider handling the muxing in the PHY driver instead.

Dmitry just posted a v2, which I'll comment on:

	https://lore.kernel.org/all/20220412193839.2545814-1-dmitry.baryshkov@linaro.org/

Please take a look at that before merging the clock changes.

Johan

Re: [PATCH v1 1/5] clk: qcom: regmap-mux: add pipe clk implementation

[Lorenzo Pieralisi]

On Tue, Apr 12, 2022 at 01:45:37PM -0500, Bjorn Andersson wrote:
> On Fri 08 Apr 06:15 CDT 2022, Lorenzo Pieralisi wrote:
> 
> > On Wed, Mar 23, 2022 at 11:50:06AM +0300, Dmitry Baryshkov wrote:
> > > On recent Qualcomm platforms the QMP PIPE clocks feed into a set of
> > > muxes which must be parked to the "safe" source (bi_tcxo) when
> > > corresponding GDSC is turned off and on again. Currently this is
> > > handcoded in the PCIe driver by reparenting the gcc_pipe_N_clk_src
> > > clock. However the same code sequence should be applied in the
> > > pcie-qcom endpoint, USB3 and UFS drivers.
> > > 
> > > Rather than copying this sequence over and over again, follow the
> > > example of clk_rcg2_shared_ops and implement this parking in the
> > > enable() and disable() clock operations. As we are changing the parent
> > > behind the back of the clock framework, also implement custom
> > > set_parent() and get_parent() operations behaving accroding to the clock
> > > framework expectations (cache the new parent if the clock is in disabled
> > > state, return cached parent).
> > > 
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >  drivers/clk/qcom/clk-regmap-mux.c | 78 +++++++++++++++++++++++++++++++
> > >  drivers/clk/qcom/clk-regmap-mux.h |  3 ++
> > >  2 files changed, 81 insertions(+)
> > 
> > Need BjornA's ACK on this patch and I can pull the series then.
> > 
> 
> It seems I have a few more clock patches in the queue which depends on
> top of this, so I picked up the three clock branches and pushed a tag
> for you to pick up, Lorenzo.

There are still comments on v2 as of today that require attention.

https://lore.kernel.org/linux-pci/YlaUtCuMZZL4bM2U@hovoldconsulting.com

I will put this on hold waiting for the discussion to complete.

Lorenzo

> The following changes since commit a9ed9e2bf7940353d2ffa4faa2ad2b75a24f3ac0:
> 
>   clk: qcom: gcc-sc7280: use new clk_regmap_mux_safe_ops for PCIe pipe clocks (2022-04-12 13:32:58 -0500)
> 
> are available in the Git repository at:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git tags/20220323085010.1753493-4-dmitry.baryshkov@linaro.org
> 
> for you to fetch changes up to a9ed9e2bf7940353d2ffa4faa2ad2b75a24f3ac0:
> 
>   clk: qcom: gcc-sc7280: use new clk_regmap_mux_safe_ops for PCIe pipe clocks (2022-04-12 13:32:58 -0500)
> 
> ----------------------------------------------------------------
> v5.18-rc1 +
> 20220323085010.1753493-2-dmitry.baryshkov@linaro.org +
> 20220323085010.1753493-3-dmitry.baryshkov@linaro.org +
> 20220323085010.1753493-4-dmitry.baryshkov@linaro.org
> 
> ----------------------------------------------------------------
> 
> Hope this works for you.
> 
> Regards,
> Bjorn
> 
> > Lorenzo
> > 
> > > diff --git a/drivers/clk/qcom/clk-regmap-mux.c b/drivers/clk/qcom/clk-regmap-mux.c
> > > index 45d9cca28064..c39ee783ee83 100644
> > > --- a/drivers/clk/qcom/clk-regmap-mux.c
> > > +++ b/drivers/clk/qcom/clk-regmap-mux.c
> > > @@ -49,9 +49,87 @@ static int mux_set_parent(struct clk_hw *hw, u8 index)
> > >  	return regmap_update_bits(clkr->regmap, mux->reg, mask, val);
> > >  }
> > >  
> > > +static u8 mux_safe_get_parent(struct clk_hw *hw)
> > > +{
> > > +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
> > > +	unsigned int val;
> > > +
> > > +	if (clk_hw_is_enabled(hw))
> > > +		return mux_get_parent(hw);
> > > +
> > > +	val = mux->stored_parent_cfg;
> > > +
> > > +	if (mux->parent_map)
> > > +		return qcom_find_cfg_index(hw, mux->parent_map, val);
> > > +
> > > +	return val;
> > > +}
> > > +
> > > +static int mux_safe_set_parent(struct clk_hw *hw, u8 index)
> > > +{
> > > +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
> > > +
> > > +	if (clk_hw_is_enabled(hw))
> > > +		return mux_set_parent(hw, index);
> > > +
> > > +	if (mux->parent_map)
> > > +		index = mux->parent_map[index].cfg;
> > > +
> > > +	mux->stored_parent_cfg = index;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void mux_safe_disable(struct clk_hw *hw)
> > > +{
> > > +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
> > > +	struct clk_regmap *clkr = to_clk_regmap(hw);
> > > +	unsigned int mask = GENMASK(mux->width + mux->shift - 1, mux->shift);
> > > +	unsigned int val;
> > > +
> > > +	regmap_read(clkr->regmap, mux->reg, &val);
> > > +
> > > +	mux->stored_parent_cfg = (val & mask) >> mux->shift;
> > > +
> > > +	val = mux->safe_src_parent;
> > > +	if (mux->parent_map) {
> > > +		int index = qcom_find_src_index(hw, mux->parent_map, val);
> > > +
> > > +		if (WARN_ON(index < 0))
> > > +			return;
> > > +
> > > +		val = mux->parent_map[index].cfg;
> > > +	}
> > > +	val <<= mux->shift;
> > > +
> > > +	regmap_update_bits(clkr->regmap, mux->reg, mask, val);
> > > +}
> > > +
> > > +static int mux_safe_enable(struct clk_hw *hw)
> > > +{
> > > +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
> > > +	struct clk_regmap *clkr = to_clk_regmap(hw);
> > > +	unsigned int mask = GENMASK(mux->width + mux->shift - 1, mux->shift);
> > > +	unsigned int val;
> > > +
> > > +	val = mux->stored_parent_cfg;
> > > +	val <<= mux->shift;
> > > +
> > > +	return regmap_update_bits(clkr->regmap, mux->reg, mask, val);
> > > +}
> > > +
> > >  const struct clk_ops clk_regmap_mux_closest_ops = {
> > >  	.get_parent = mux_get_parent,
> > >  	.set_parent = mux_set_parent,
> > >  	.determine_rate = __clk_mux_determine_rate_closest,
> > >  };
> > >  EXPORT_SYMBOL_GPL(clk_regmap_mux_closest_ops);
> > > +
> > > +const struct clk_ops clk_regmap_mux_safe_ops = {
> > > +	.enable = mux_safe_enable,
> > > +	.disable = mux_safe_disable,
> > > +	.get_parent = mux_safe_get_parent,
> > > +	.set_parent = mux_safe_set_parent,
> > > +	.determine_rate = __clk_mux_determine_rate_closest,
> > > +};
> > > +EXPORT_SYMBOL_GPL(clk_regmap_mux_safe_ops);
> > > diff --git a/drivers/clk/qcom/clk-regmap-mux.h b/drivers/clk/qcom/clk-regmap-mux.h
> > > index db6f4cdd9586..f86c674ce139 100644
> > > --- a/drivers/clk/qcom/clk-regmap-mux.h
> > > +++ b/drivers/clk/qcom/clk-regmap-mux.h
> > > @@ -14,10 +14,13 @@ struct clk_regmap_mux {
> > >  	u32			reg;
> > >  	u32			shift;
> > >  	u32			width;
> > > +	u8			safe_src_parent;
> > > +	u8			stored_parent_cfg;
> > >  	const struct parent_map	*parent_map;
> > >  	struct clk_regmap	clkr;
> > >  };
> > >  
> > >  extern const struct clk_ops clk_regmap_mux_closest_ops;
> > > +extern const struct clk_ops clk_regmap_mux_safe_ops;
> > >  
> > >  #endif
> > > -- 
> > > 2.35.1
> > > 

Re: [PATCH v1 1/5] clk: qcom: regmap-mux: add pipe clk implementation

[Bjorn Andersson]

On Wed 23 Mar 03:50 CDT 2022, Dmitry Baryshkov wrote:

> On recent Qualcomm platforms the QMP PIPE clocks feed into a set of
> muxes which must be parked to the "safe" source (bi_tcxo) when
> corresponding GDSC is turned off and on again. Currently this is
> handcoded in the PCIe driver by reparenting the gcc_pipe_N_clk_src
> clock. However the same code sequence should be applied in the
> pcie-qcom endpoint, USB3 and UFS drivers.
> 
> Rather than copying this sequence over and over again, follow the
> example of clk_rcg2_shared_ops and implement this parking in the
> enable() and disable() clock operations. As we are changing the parent
> behind the back of the clock framework, also implement custom
> set_parent() and get_parent() operations behaving accroding to the clock
> framework expectations (cache the new parent if the clock is in disabled
> state, return cached parent).
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/clk/qcom/clk-regmap-mux.c | 78 +++++++++++++++++++++++++++++++
>  drivers/clk/qcom/clk-regmap-mux.h |  3 ++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/drivers/clk/qcom/clk-regmap-mux.c b/drivers/clk/qcom/clk-regmap-mux.c
> index 45d9cca28064..c39ee783ee83 100644
> --- a/drivers/clk/qcom/clk-regmap-mux.c
> +++ b/drivers/clk/qcom/clk-regmap-mux.c
> @@ -49,9 +49,87 @@ static int mux_set_parent(struct clk_hw *hw, u8 index)
>  	return regmap_update_bits(clkr->regmap, mux->reg, mask, val);
>  }
>  
> +static u8 mux_safe_get_parent(struct clk_hw *hw)
> +{
> +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
> +	unsigned int val;
> +
> +	if (clk_hw_is_enabled(hw))
> +		return mux_get_parent(hw);
> +
> +	val = mux->stored_parent_cfg;
> +
> +	if (mux->parent_map)
> +		return qcom_find_cfg_index(hw, mux->parent_map, val);
> +
> +	return val;
> +}
> +
> +static int mux_safe_set_parent(struct clk_hw *hw, u8 index)
> +{
> +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
> +
> +	if (clk_hw_is_enabled(hw))
> +		return mux_set_parent(hw, index);
> +
> +	if (mux->parent_map)
> +		index = mux->parent_map[index].cfg;
> +
> +	mux->stored_parent_cfg = index;
> +
> +	return 0;
> +}
> +
> +static void mux_safe_disable(struct clk_hw *hw)
> +{
> +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
> +	struct clk_regmap *clkr = to_clk_regmap(hw);
> +	unsigned int mask = GENMASK(mux->width + mux->shift - 1, mux->shift);
> +	unsigned int val;
> +
> +	regmap_read(clkr->regmap, mux->reg, &val);
> +
> +	mux->stored_parent_cfg = (val & mask) >> mux->shift;
> +
> +	val = mux->safe_src_parent;
> +	if (mux->parent_map) {
> +		int index = qcom_find_src_index(hw, mux->parent_map, val);
> +
> +		if (WARN_ON(index < 0))
> +			return;
> +
> +		val = mux->parent_map[index].cfg;
> +	}
> +	val <<= mux->shift;
> +
> +	regmap_update_bits(clkr->regmap, mux->reg, mask, val);
> +}
> +
> +static int mux_safe_enable(struct clk_hw *hw)
> +{
> +	struct clk_regmap_mux *mux = to_clk_regmap_mux(hw);
> +	struct clk_regmap *clkr = to_clk_regmap(hw);
> +	unsigned int mask = GENMASK(mux->width + mux->shift - 1, mux->shift);
> +	unsigned int val;
> +
> +	val = mux->stored_parent_cfg;
> +	val <<= mux->shift;
> +
> +	return regmap_update_bits(clkr->regmap, mux->reg, mask, val);
> +}
> +
>  const struct clk_ops clk_regmap_mux_closest_ops = {
>  	.get_parent = mux_get_parent,
>  	.set_parent = mux_set_parent,
>  	.determine_rate = __clk_mux_determine_rate_closest,
>  };
>  EXPORT_SYMBOL_GPL(clk_regmap_mux_closest_ops);
> +
> +const struct clk_ops clk_regmap_mux_safe_ops = {
> +	.enable = mux_safe_enable,
> +	.disable = mux_safe_disable,
> +	.get_parent = mux_safe_get_parent,
> +	.set_parent = mux_safe_set_parent,
> +	.determine_rate = __clk_mux_determine_rate_closest,
> +};
> +EXPORT_SYMBOL_GPL(clk_regmap_mux_safe_ops);
> diff --git a/drivers/clk/qcom/clk-regmap-mux.h b/drivers/clk/qcom/clk-regmap-mux.h
> index db6f4cdd9586..f86c674ce139 100644
> --- a/drivers/clk/qcom/clk-regmap-mux.h
> +++ b/drivers/clk/qcom/clk-regmap-mux.h
> @@ -14,10 +14,13 @@ struct clk_regmap_mux {
>  	u32			reg;
>  	u32			shift;
>  	u32			width;
> +	u8			safe_src_parent;
> +	u8			stored_parent_cfg;
>  	const struct parent_map	*parent_map;
>  	struct clk_regmap	clkr;
>  };
>  
>  extern const struct clk_ops clk_regmap_mux_closest_ops;
> +extern const struct clk_ops clk_regmap_mux_safe_ops;
>  
>  #endif
> -- 
> 2.35.1
> 

[PATCH v1 2/5] clk: qcom: gcc-sm8450: use new clk_regmap_mux_safe_ops for PCIe pipe clocks

[Dmitry Baryshkov]

Use newly defined clk_regmap_mux_safe_ops for PCIe pipe clocks to let
the clock framework automatically park the clock when the clock is
switched off and restore the parent when the clock is switched on.

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/gcc-sm8450.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/gcc-sm8450.c b/drivers/clk/qcom/gcc-sm8450.c
index 593a195467ff..fb6decd3df49 100644
--- a/drivers/clk/qcom/gcc-sm8450.c
+++ b/drivers/clk/qcom/gcc-sm8450.c
@@ -243,13 +243,14 @@ static struct clk_regmap_mux gcc_pcie_0_pipe_clk_src = {
 	.reg = 0x7b060,
 	.shift = 0,
 	.width = 2,
+	.safe_src_parent = P_BI_TCXO,
 	.parent_map = gcc_parent_map_4,
 	.clkr = {
 		.hw.init = &(struct clk_init_data){
 			.name = "gcc_pcie_0_pipe_clk_src",
 			.parent_data = gcc_parent_data_4,
 			.num_parents = ARRAY_SIZE(gcc_parent_data_4),
-			.ops = &clk_regmap_mux_closest_ops,
+			.ops = &clk_regmap_mux_safe_ops,
 		},
 	},
 };
@@ -273,13 +274,14 @@ static struct clk_regmap_mux gcc_pcie_1_pipe_clk_src = {
 	.reg = 0x9d064,
 	.shift = 0,
 	.width = 2,
+	.safe_src_parent = P_BI_TCXO,
 	.parent_map = gcc_parent_map_6,
 	.clkr = {
 		.hw.init = &(struct clk_init_data){
 			.name = "gcc_pcie_1_pipe_clk_src",
 			.parent_data = gcc_parent_data_6,
 			.num_parents = ARRAY_SIZE(gcc_parent_data_6),
-			.ops = &clk_regmap_mux_closest_ops,
+			.ops = &clk_regmap_mux_safe_ops,
 		},
 	},
 };
-- 
2.35.1

[PATCH v1 3/5] clk: qcom: gcc-sc7280: use new clk_regmap_mux_safe_ops for PCIe pipe clocks

[Dmitry Baryshkov]

Use newly defined clk_regmap_mux_safe_ops for PCIe pipe clocks to let
the clock framework automatically park the clock when the clock is
switched off and restore the parent when the clock is switched on.

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/gcc-sc7280.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/qcom/gcc-sc7280.c b/drivers/clk/qcom/gcc-sc7280.c
index 423627d49719..dafbbc8f3bf4 100644
--- a/drivers/clk/qcom/gcc-sc7280.c
+++ b/drivers/clk/qcom/gcc-sc7280.c
@@ -373,13 +373,14 @@ static struct clk_regmap_mux gcc_pcie_0_pipe_clk_src = {
 	.reg = 0x6b054,
 	.shift = 0,
 	.width = 2,
+	.safe_src_parent = P_BI_TCXO,
 	.parent_map = gcc_parent_map_6,
 	.clkr = {
 		.hw.init = &(struct clk_init_data){
 			.name = "gcc_pcie_0_pipe_clk_src",
 			.parent_data = gcc_parent_data_6,
 			.num_parents = ARRAY_SIZE(gcc_parent_data_6),
-			.ops = &clk_regmap_mux_closest_ops,
+			.ops = &clk_regmap_mux_safe_ops,
 		},
 	},
 };
@@ -388,13 +389,14 @@ static struct clk_regmap_mux gcc_pcie_1_pipe_clk_src = {
 	.reg = 0x8d054,
 	.shift = 0,
 	.width = 2,
+	.safe_src_parent = P_BI_TCXO,
 	.parent_map = gcc_parent_map_7,
 	.clkr = {
 		.hw.init = &(struct clk_init_data){
 			.name = "gcc_pcie_1_pipe_clk_src",
 			.parent_data = gcc_parent_data_7,
 			.num_parents = ARRAY_SIZE(gcc_parent_data_7),
-			.ops = &clk_regmap_mux_closest_ops,
+			.ops = &clk_regmap_mux_safe_ops,
 		},
 	},
 };
-- 
2.35.1

[PATCH v1 4/5] PCI: qcom: Remove unnecessary pipe_clk handling

[Dmitry Baryshkov]

QMP PHY driver already does clk_prepare_enable()/_disable() pipe_clk.
Remove extra calls to enable/disable this clock from the PCIe driver, so
that the PHY driver can manage the clock on its own.

Fixes: aa9c0df98c29 ("PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280")
Cc: Prasad Malisetty <quic_pmaliset@quicinc.com>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 50 ++------------------------
 1 file changed, 3 insertions(+), 47 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index 6ab90891801d..a6becafb6a77 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -128,7 +128,6 @@ struct qcom_pcie_resources_2_3_2 {
 	struct clk *master_clk;
 	struct clk *slave_clk;
 	struct clk *cfg_clk;
-	struct clk *pipe_clk;
 	struct regulator_bulk_data supplies[QCOM_PCIE_2_3_2_MAX_SUPPLY];
 };
 
@@ -165,7 +164,6 @@ struct qcom_pcie_resources_2_7_0 {
 	int num_clks;
 	struct regulator_bulk_data supplies[2];
 	struct reset_control *pci_reset;
-	struct clk *pipe_clk;
 	struct clk *pipe_clk_src;
 	struct clk *phy_pipe_clk;
 	struct clk *ref_clk_src;
@@ -597,8 +595,7 @@ static int qcom_pcie_get_resources_2_3_2(struct qcom_pcie *pcie)
 	if (IS_ERR(res->slave_clk))
 		return PTR_ERR(res->slave_clk);
 
-	res->pipe_clk = devm_clk_get(dev, "pipe");
-	return PTR_ERR_OR_ZERO(res->pipe_clk);
+	return 0;
 }
 
 static void qcom_pcie_deinit_2_3_2(struct qcom_pcie *pcie)
@@ -613,13 +610,6 @@ static void qcom_pcie_deinit_2_3_2(struct qcom_pcie *pcie)
 	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
 }
 
-static void qcom_pcie_post_deinit_2_3_2(struct qcom_pcie *pcie)
-{
-	struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
-
-	clk_disable_unprepare(res->pipe_clk);
-}
-
 static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
 {
 	struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
@@ -694,22 +684,6 @@ static int qcom_pcie_init_2_3_2(struct qcom_pcie *pcie)
 	return ret;
 }
 
-static int qcom_pcie_post_init_2_3_2(struct qcom_pcie *pcie)
-{
-	struct qcom_pcie_resources_2_3_2 *res = &pcie->res.v2_3_2;
-	struct dw_pcie *pci = pcie->pci;
-	struct device *dev = pci->dev;
-	int ret;
-
-	ret = clk_prepare_enable(res->pipe_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable pipe clock\n");
-		return ret;
-	}
-
-	return 0;
-}
-
 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;
@@ -1198,8 +1172,7 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
 			return PTR_ERR(res->ref_clk_src);
 	}
 
-	res->pipe_clk = devm_clk_get(dev, "pipe");
-	return PTR_ERR_OR_ZERO(res->pipe_clk);
+	return 0;
 }
 
 static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
@@ -1238,12 +1211,6 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
 		goto err_disable_clocks;
 	}
 
-	ret = clk_prepare_enable(res->pipe_clk);
-	if (ret) {
-		dev_err(dev, "cannot prepare/enable pipe clock\n");
-		goto err_disable_clocks;
-	}
-
 	/* Wait for reset to complete, required on SM8450 */
 	usleep_range(1000, 1500);
 
@@ -1298,14 +1265,7 @@ static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
 	if (pcie->cfg->pipe_clk_need_muxing)
 		clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
 
-	return clk_prepare_enable(res->pipe_clk);
-}
-
-static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
-{
-	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
-
-	clk_disable_unprepare(res->pipe_clk);
+	return 0;
 }
 
 static int qcom_pcie_link_up(struct dw_pcie *pci)
@@ -1455,9 +1415,7 @@ static const struct qcom_pcie_ops ops_1_0_0 = {
 static const struct qcom_pcie_ops ops_2_3_2 = {
 	.get_resources = qcom_pcie_get_resources_2_3_2,
 	.init = qcom_pcie_init_2_3_2,
-	.post_init = qcom_pcie_post_init_2_3_2,
 	.deinit = qcom_pcie_deinit_2_3_2,
-	.post_deinit = qcom_pcie_post_deinit_2_3_2,
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
 };
 
@@ -1484,7 +1442,6 @@ static const struct qcom_pcie_ops ops_2_7_0 = {
 	.deinit = qcom_pcie_deinit_2_7_0,
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
 	.post_init = qcom_pcie_post_init_2_7_0,
-	.post_deinit = qcom_pcie_post_deinit_2_7_0,
 };
 
 /* Qcom IP rev.: 1.9.0 */
@@ -1494,7 +1451,6 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
 	.deinit = qcom_pcie_deinit_2_7_0,
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
 	.post_init = qcom_pcie_post_init_2_7_0,
-	.post_deinit = qcom_pcie_post_deinit_2_7_0,
 	.config_sid = qcom_pcie_config_sid_sm8250,
 };
 
-- 
2.35.1

Re: [PATCH v1 4/5] PCI: qcom: Remove unnecessary pipe_clk handling

[Johan Hovold]

On Wed, Mar 23, 2022 at 11:50:09AM +0300, Dmitry Baryshkov wrote:
> QMP PHY driver already does clk_prepare_enable()/_disable() pipe_clk.
> Remove extra calls to enable/disable this clock from the PCIe driver, so
> that the PHY driver can manage the clock on its own.
> 
> Fixes: aa9c0df98c29 ("PCI: qcom: Switch pcie_1_pipe_clk_src after PHY init in SC7280")
> Cc: Prasad Malisetty <quic_pmaliset@quicinc.com>

Looks like you're blaming the wrong author and commit, but a Fixes tag
probably isn't warranted at all here.

The first commit to introduce pipe clock handling to this driver was

	d0491fc39bdd ("PCI: qcom: Add support for MSM8996 PCIe controller")

back in 2016.

This was followed up in 2019 with

	ed8cc3b1fc84 ("PCI: qcom: Add support for SDM845 PCIe controller")

which also introduced a clock imbalance by enabling the pipe clock both
in init() and post_init() but only disabling in post_deinit().

Note that this commit also started enabling the pipe clock before
powering up the PHY (i.e. the call in init()) which looks like another
bug.

This bit should be fixed separately and you can just drop the Fixes tag
from this patch. I've sent a fix here:

	https://lore.kernel.org/r/20220401101325.16983-1-johan+linaro@kernel.org

> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/pci/controller/dwc/pcie-qcom.c | 50 ++------------------------
>  1 file changed, 3 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c

>  static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
> @@ -1238,12 +1211,6 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
>  		goto err_disable_clocks;
>  	}
>  
> -	ret = clk_prepare_enable(res->pipe_clk);
> -	if (ret) {
> -		dev_err(dev, "cannot prepare/enable pipe clock\n");
> -		goto err_disable_clocks;
> -	}
> -

Here's the imbalance.

>  	/* Wait for reset to complete, required on SM8450 */
>  	usleep_range(1000, 1500);
>  
> @@ -1298,14 +1265,7 @@ static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
>  	if (pcie->cfg->pipe_clk_need_muxing)
>  		clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
>  
> -	return clk_prepare_enable(res->pipe_clk);
> -}
> -
> -static void qcom_pcie_post_deinit_2_7_0(struct qcom_pcie *pcie)
> -{
> -	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
> -
> -	clk_disable_unprepare(res->pipe_clk);
> +	return 0;
>  }

Johan

[PATCH v1 5/5] PCI: qcom: Drop manual pipe_clk_src handling

[Dmitry Baryshkov]

Manual reparenting of pipe_clk_src is being replaced with the parking of
the clock with clk_disable()/clk_enable(). Drop redundant code letting
the pipe clock driver park the clock to the safe bi_tcxo parent
automatically.

Cc: Prasad Malisetty <quic_pmaliset@quicinc.com>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/pci/controller/dwc/pcie-qcom.c | 39 +-------------------------
 1 file changed, 1 insertion(+), 38 deletions(-)

diff --git a/drivers/pci/controller/dwc/pcie-qcom.c b/drivers/pci/controller/dwc/pcie-qcom.c
index a6becafb6a77..b48c899bcc97 100644
--- a/drivers/pci/controller/dwc/pcie-qcom.c
+++ b/drivers/pci/controller/dwc/pcie-qcom.c
@@ -164,9 +164,6 @@ struct qcom_pcie_resources_2_7_0 {
 	int num_clks;
 	struct regulator_bulk_data supplies[2];
 	struct reset_control *pci_reset;
-	struct clk *pipe_clk_src;
-	struct clk *phy_pipe_clk;
-	struct clk *ref_clk_src;
 };
 
 union qcom_pcie_resources {
@@ -192,7 +189,6 @@ struct qcom_pcie_ops {
 
 struct qcom_pcie_cfg {
 	const struct qcom_pcie_ops *ops;
-	unsigned int pipe_clk_need_muxing:1;
 	unsigned int has_tbu_clk:1;
 	unsigned int has_ddrss_sf_tbu_clk:1;
 	unsigned int has_aggre0_clk:1;
@@ -1158,20 +1154,6 @@ static int qcom_pcie_get_resources_2_7_0(struct qcom_pcie *pcie)
 	if (ret < 0)
 		return ret;
 
-	if (pcie->cfg->pipe_clk_need_muxing) {
-		res->pipe_clk_src = devm_clk_get(dev, "pipe_mux");
-		if (IS_ERR(res->pipe_clk_src))
-			return PTR_ERR(res->pipe_clk_src);
-
-		res->phy_pipe_clk = devm_clk_get(dev, "phy_pipe");
-		if (IS_ERR(res->phy_pipe_clk))
-			return PTR_ERR(res->phy_pipe_clk);
-
-		res->ref_clk_src = devm_clk_get(dev, "ref");
-		if (IS_ERR(res->ref_clk_src))
-			return PTR_ERR(res->ref_clk_src);
-	}
-
 	return 0;
 }
 
@@ -1189,10 +1171,6 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
 		return ret;
 	}
 
-	/* Set TCXO as clock source for pcie_pipe_clk_src */
-	if (pcie->cfg->pipe_clk_need_muxing)
-		clk_set_parent(res->pipe_clk_src, res->ref_clk_src);
-
 	ret = clk_bulk_prepare_enable(res->num_clks, res->clks);
 	if (ret < 0)
 		goto err_disable_regulators;
@@ -1254,18 +1232,8 @@ static void qcom_pcie_deinit_2_7_0(struct qcom_pcie *pcie)
 	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
 
 	clk_bulk_disable_unprepare(res->num_clks, res->clks);
-	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
-}
 
-static int qcom_pcie_post_init_2_7_0(struct qcom_pcie *pcie)
-{
-	struct qcom_pcie_resources_2_7_0 *res = &pcie->res.v2_7_0;
-
-	/* Set pipe clock as clock source for pcie_pipe_clk_src */
-	if (pcie->cfg->pipe_clk_need_muxing)
-		clk_set_parent(res->pipe_clk_src, res->phy_pipe_clk);
-
-	return 0;
+	regulator_bulk_disable(ARRAY_SIZE(res->supplies), res->supplies);
 }
 
 static int qcom_pcie_link_up(struct dw_pcie *pci)
@@ -1441,7 +1409,6 @@ static const struct qcom_pcie_ops ops_2_7_0 = {
 	.init = qcom_pcie_init_2_7_0,
 	.deinit = qcom_pcie_deinit_2_7_0,
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
-	.post_init = qcom_pcie_post_init_2_7_0,
 };
 
 /* Qcom IP rev.: 1.9.0 */
@@ -1450,7 +1417,6 @@ static const struct qcom_pcie_ops ops_1_9_0 = {
 	.init = qcom_pcie_init_2_7_0,
 	.deinit = qcom_pcie_deinit_2_7_0,
 	.ltssm_enable = qcom_pcie_2_3_2_ltssm_enable,
-	.post_init = qcom_pcie_post_init_2_7_0,
 	.config_sid = qcom_pcie_config_sid_sm8250,
 };
 
@@ -1488,7 +1454,6 @@ static const struct qcom_pcie_cfg sm8250_cfg = {
 static const struct qcom_pcie_cfg sm8450_pcie0_cfg = {
 	.ops = &ops_1_9_0,
 	.has_ddrss_sf_tbu_clk = true,
-	.pipe_clk_need_muxing = true,
 	.has_aggre0_clk = true,
 	.has_aggre1_clk = true,
 };
@@ -1496,14 +1461,12 @@ static const struct qcom_pcie_cfg sm8450_pcie0_cfg = {
 static const struct qcom_pcie_cfg sm8450_pcie1_cfg = {
 	.ops = &ops_1_9_0,
 	.has_ddrss_sf_tbu_clk = true,
-	.pipe_clk_need_muxing = true,
 	.has_aggre1_clk = true,
 };
 
 static const struct qcom_pcie_cfg sc7280_cfg = {
 	.ops = &ops_1_9_0,
 	.has_tbu_clk = true,
-	.pipe_clk_need_muxing = true,
 };
 
 static const struct dw_pcie_ops dw_pcie_ops = {
-- 
2.35.1

Re: [PATCH v1 0/5] PCI: qcom: rework pipe_clk/pipe_clk_src handling

[Dmitry Baryshkov]

On 23/03/2022 11:50, Dmitry Baryshkov wrote:
> PCIe pipe clk (and some other clocks) must be parked to the "safe"
> source (bi_tcxo) when corresponding GDSC is turned off and on again.
> Currently this is handcoded in the PCIe driver by reparenting the
> gcc_pipe_N_clk_src clock.
> 
> Instead of doing it manually, follow the approach used by
> clk_rcg2_shared_ops and implement this parking in the enable() and
> disable() clock operations for respective pipe clocks.

Prasad, can we please get your comments on this patchset?
Since you have submitted original patchset for sc7280, it looks like you 
should be interested in testing that this patchset doesn't break your 
devices.

> 
> Changes since RFC:
>   - Rework clk-regmap-mux fields. Specify safe parent as P_* value rather
>     than specifying the register value directly
>   - Expand commit message to the first patch to specially mention that
>     it is required only on newer generations of Qualcomm chipsets.
> 
> Dmitry Baryshkov (5):
>    clk: qcom: regmap-mux: add pipe clk implementation
>    clk: qcom: gcc-sm8450: use new clk_regmap_mux_safe_ops for PCIe pipe
>      clocks
>    clk: qcom: gcc-sc7280: use new clk_regmap_mux_safe_ops for PCIe pipe
>      clocks
>    PCI: qcom: Remove unnecessary pipe_clk handling
>    PCI: qcom: Drop manual pipe_clk_src handling
> 
>   drivers/clk/qcom/clk-regmap-mux.c      | 78 +++++++++++++++++++++++
>   drivers/clk/qcom/clk-regmap-mux.h      |  3 +
>   drivers/clk/qcom/gcc-sc7280.c          |  6 +-
>   drivers/clk/qcom/gcc-sm8450.c          |  6 +-
>   drivers/pci/controller/dwc/pcie-qcom.c | 87 +-------------------------
>   5 files changed, 92 insertions(+), 88 deletions(-)
> 


-- 
With best wishes
Dmitry

RE: [PATCH v1 0/5] PCI: qcom: rework pipe_clk/pipe_clk_src handling

[Prasad Malisetty (Temp) (QUIC)]

Hi Dmitry,

Please find inline comments below.

Thanks
-Prasad

> -----Original Message-----
> From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Sent: Wednesday, March 30, 2022 6:26 PM
> To: Prasad Malisetty (Temp) (QUIC) <quic_pmaliset@quicinc.com>; Andy Gross
> <agross@kernel.org>; bjorn.andersson@linaro.org; Stephen Boyd
> <swboyd@chromium.org>; Michael Turquette <mturquette@baylibre.com>;
> Taniya Das <tdas@codeaurora.org>; Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com>; Krzysztof Wilczyński <kw@linux.com>; Bjorn
> Helgaas <bhelgaas@google.com>
> Cc: linux-arm-msm@vger.kernel.org; linux-clk@vger.kernel.org; linux-
> pci@vger.kernel.org
> Subject: Re: [PATCH v1 0/5] PCI: qcom: rework pipe_clk/pipe_clk_src handling
> 
> On 23/03/2022 11:50, Dmitry Baryshkov wrote:
> > PCIe pipe clk (and some other clocks) must be parked to the "safe"
> > source (bi_tcxo) when corresponding GDSC is turned off and on again.
> > Currently this is handcoded in the PCIe driver by reparenting the
> > gcc_pipe_N_clk_src clock.
> >
> > Instead of doing it manually, follow the approach used by
> > clk_rcg2_shared_ops and implement this parking in the enable() and
> > disable() clock operations for respective pipe clocks.
> 
> Prasad, can we please get your comments on this patchset?
> Since you have submitted original patchset for sc7280, it looks like you should be
> interested in testing that this patchset doesn't break your devices.
> 
Thanks for optimizing pipe clock handling. 
Sure Dmitry, the validation is in progress ( Need to validate power consumption and other stuff).
I will update once done.

Thanks,
-Prasad.
> >
> > Changes since RFC:
> >   - Rework clk-regmap-mux fields. Specify safe parent as P_* value rather
> >     than specifying the register value directly
> >   - Expand commit message to the first patch to specially mention that
> >     it is required only on newer generations of Qualcomm chipsets.
> >
> > Dmitry Baryshkov (5):
> >    clk: qcom: regmap-mux: add pipe clk implementation
> >    clk: qcom: gcc-sm8450: use new clk_regmap_mux_safe_ops for PCIe pipe
> >      clocks
> >    clk: qcom: gcc-sc7280: use new clk_regmap_mux_safe_ops for PCIe pipe
> >      clocks
> >    PCI: qcom: Remove unnecessary pipe_clk handling
> >    PCI: qcom: Drop manual pipe_clk_src handling
> >
> >   drivers/clk/qcom/clk-regmap-mux.c      | 78 +++++++++++++++++++++++
> >   drivers/clk/qcom/clk-regmap-mux.h      |  3 +
> >   drivers/clk/qcom/gcc-sc7280.c          |  6 +-
> >   drivers/clk/qcom/gcc-sm8450.c          |  6 +-
> >   drivers/pci/controller/dwc/pcie-qcom.c | 87 +-------------------------
> >   5 files changed, 92 insertions(+), 88 deletions(-)
> >
> 
> 
> --
> With best wishes
> Dmitry