[PATCH 0/4] Small fixes/improvement for hfpll and krait

[PATCH 0/4] Small fixes/improvement for hfpll and krait

[Ansuel Smith]

This series has small fixes/improvement to the hfpll and krait clk
driver.

This comes from another series that got split to better facilitate the
merge since it was grown to 21 patches and was getting hard to review.

For hfpll, a conversion to read_poll macro and introduction
of a timeout to prevent a stall.
For krait, a fix for the mux sel logic, an extra check for
div2_rount_rate and an introduction for 8064 errata.

Ansuel Smith (4):
  clk: qcom: clk-hfpll: use poll_timeout macro
  clk: qcom: clk-krait: unlock spin after mux completion
  clk: qcom: clk-krait: add hw_parent check for div2_round_rate
  clk: qcom: clk-krait: add apq/ipq8064 errata workaround

 drivers/clk/qcom/clk-hfpll.c | 15 +++++++++------
 drivers/clk/qcom/clk-krait.c | 30 ++++++++++++++++++++++++++++--
 drivers/clk/qcom/clk-krait.h |  1 +
 drivers/clk/qcom/krait-cc.c  |  1 +
 4 files changed, 39 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH 1/4] clk: qcom: clk-hfpll: use poll_timeout macro

[Ansuel Smith]

Use regmap_read_poll_timeout macro instead of do-while structure to tidy
things up. Also set a timeout to prevent any sort of system stall.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/clk/qcom/clk-hfpll.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/qcom/clk-hfpll.c b/drivers/clk/qcom/clk-hfpll.c
index e847d586a73a..7dd17c184b69 100644
--- a/drivers/clk/qcom/clk-hfpll.c
+++ b/drivers/clk/qcom/clk-hfpll.c
@@ -72,13 +72,16 @@ static void __clk_hfpll_enable(struct clk_hw *hw)
 	regmap_update_bits(regmap, hd->mode_reg, PLL_RESET_N, PLL_RESET_N);
 
 	/* Wait for PLL to lock. */
-	if (hd->status_reg) {
-		do {
-			regmap_read(regmap, hd->status_reg, &val);
-		} while (!(val & BIT(hd->lock_bit)));
-	} else {
+	if (hd->status_reg)
+		/*
+		 * Busy wait. Should never timeout, we add a timeout to
+		 * prevent any sort of stall.
+		 */
+		regmap_read_poll_timeout(regmap, hd->status_reg, val,
+					 !(val & BIT(hd->lock_bit)), 0,
+					 100 * USEC_PER_MSEC);
+	else
 		udelay(60);
-	}
 
 	/* Enable PLL output. */
 	regmap_update_bits(regmap, hd->mode_reg, PLL_OUTCTRL, PLL_OUTCTRL);
-- 
2.34.1

[PATCH 2/4] clk: qcom: clk-krait: unlock spin after mux completion

[Ansuel Smith]

Unlock spinlock after the mux switch is completed to prevent any corner
case of mux request while the switch still needs to be done.

Fixes: 4d7dc77babfe ("clk: qcom: Add support for Krait clocks")
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/clk/qcom/clk-krait.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/clk-krait.c b/drivers/clk/qcom/clk-krait.c
index 59f1af415b58..90046428693c 100644
--- a/drivers/clk/qcom/clk-krait.c
+++ b/drivers/clk/qcom/clk-krait.c
@@ -32,11 +32,16 @@ static void __krait_mux_set_sel(struct krait_mux_clk *mux, int sel)
 		regval |= (sel & mux->mask) << (mux->shift + LPL_SHIFT);
 	}
 	krait_set_l2_indirect_reg(mux->offset, regval);
-	spin_unlock_irqrestore(&krait_clock_reg_lock, flags);
 
 	/* Wait for switch to complete. */
 	mb();
 	udelay(1);
+
+	/*
+	 * Unlock now to make sure the mux register is not
+	 * modified while switching to the new parent.
+	 */
+	spin_unlock_irqrestore(&krait_clock_reg_lock, flags);
 }
 
 static int krait_mux_set_parent(struct clk_hw *hw, u8 index)
-- 
2.34.1

[PATCH 3/4] clk: qcom: clk-krait: add hw_parent check for div2_round_rate

[Ansuel Smith]

Check if hw_parent is present before calculating the round_rate to
prevent kernel panic. On error -EINVAL is reported.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/clk/qcom/clk-krait.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/qcom/clk-krait.c b/drivers/clk/qcom/clk-krait.c
index 90046428693c..6c367ad6506a 100644
--- a/drivers/clk/qcom/clk-krait.c
+++ b/drivers/clk/qcom/clk-krait.c
@@ -84,7 +84,12 @@ EXPORT_SYMBOL_GPL(krait_mux_clk_ops);
 static long krait_div2_round_rate(struct clk_hw *hw, unsigned long rate,
 				  unsigned long *parent_rate)
 {
-	*parent_rate = clk_hw_round_rate(clk_hw_get_parent(hw), rate * 2);
+	struct clk_hw *hw_parent = clk_hw_get_parent(hw);
+
+	if (!hw_parent)
+		return -EINVAL;
+
+	*parent_rate = clk_hw_round_rate(hw_parent, rate * 2);
 	return DIV_ROUND_UP(*parent_rate, 2);
 }
 
-- 
2.34.1

[PATCH 4/4] clk: qcom: clk-krait: add apq/ipq8064 errata workaround

[Ansuel Smith]

Add apq/ipq8064 errata workaround where the sec_src clock gating needs to
be disabled during switching. To enable this set disable_sec_src_gating
in the mux struct.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/clk/qcom/clk-krait.c | 16 ++++++++++++++++
 drivers/clk/qcom/clk-krait.h |  1 +
 drivers/clk/qcom/krait-cc.c  |  1 +
 3 files changed, 18 insertions(+)

diff --git a/drivers/clk/qcom/clk-krait.c b/drivers/clk/qcom/clk-krait.c
index 6c367ad6506a..4a9b3296c45b 100644
--- a/drivers/clk/qcom/clk-krait.c
+++ b/drivers/clk/qcom/clk-krait.c
@@ -18,13 +18,23 @@
 static DEFINE_SPINLOCK(krait_clock_reg_lock);
 
 #define LPL_SHIFT	8
+#define SECCLKAGD	BIT(4)
+
 static void __krait_mux_set_sel(struct krait_mux_clk *mux, int sel)
 {
 	unsigned long flags;
 	u32 regval;
 
 	spin_lock_irqsave(&krait_clock_reg_lock, flags);
+
 	regval = krait_get_l2_indirect_reg(mux->offset);
+
+	/* apq/ipq8064 Errata: disable sec_src clock gating during switch. */
+	if (mux->disable_sec_src_gating) {
+		regval |= SECCLKAGD;
+		krait_set_l2_indirect_reg(mux->offset, regval);
+	}
+
 	regval &= ~(mux->mask << mux->shift);
 	regval |= (sel & mux->mask) << mux->shift;
 	if (mux->lpl) {
@@ -33,6 +43,12 @@ static void __krait_mux_set_sel(struct krait_mux_clk *mux, int sel)
 	}
 	krait_set_l2_indirect_reg(mux->offset, regval);
 
+	/* apq/ipq8064 Errata: re-enabled sec_src clock gating. */
+	if (mux->disable_sec_src_gating) {
+		regval &= ~SECCLKAGD;
+		krait_set_l2_indirect_reg(mux->offset, regval);
+	}
+
 	/* Wait for switch to complete. */
 	mb();
 	udelay(1);
diff --git a/drivers/clk/qcom/clk-krait.h b/drivers/clk/qcom/clk-krait.h
index 9120bd2f5297..f930538c539e 100644
--- a/drivers/clk/qcom/clk-krait.h
+++ b/drivers/clk/qcom/clk-krait.h
@@ -15,6 +15,7 @@ struct krait_mux_clk {
 	u8		safe_sel;
 	u8		old_index;
 	bool		reparent;
+	bool		disable_sec_src_gating;
 
 	struct clk_hw	hw;
 	struct notifier_block   clk_nb;
diff --git a/drivers/clk/qcom/krait-cc.c b/drivers/clk/qcom/krait-cc.c
index 4d4b657d33c3..0f88bf41ec6e 100644
--- a/drivers/clk/qcom/krait-cc.c
+++ b/drivers/clk/qcom/krait-cc.c
@@ -138,6 +138,7 @@ krait_add_sec_mux(struct device *dev, int id, const char *s,
 	mux->parent_map = sec_mux_map;
 	mux->hw.init = &init;
 	mux->safe_sel = 0;
+	mux->disable_sec_src_gating = true;
 
 	init.name = kasprintf(GFP_KERNEL, "krait%s_sec_mux", s);
 	if (!init.name)
-- 
2.34.1

Re: [PATCH 2/4] clk: qcom: clk-krait: unlock spin after mux completion

[Dmitry Baryshkov]

On 29/04/2022 15:01, Ansuel Smith wrote:
> Unlock spinlock after the mux switch is completed to prevent any corner
> case of mux request while the switch still needs to be done.
> 
> Fixes: 4d7dc77babfe ("clk: qcom: Add support for Krait clocks")
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
>   drivers/clk/qcom/clk-krait.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/clk-krait.c b/drivers/clk/qcom/clk-krait.c
> index 59f1af415b58..90046428693c 100644
> --- a/drivers/clk/qcom/clk-krait.c
> +++ b/drivers/clk/qcom/clk-krait.c
> @@ -32,11 +32,16 @@ static void __krait_mux_set_sel(struct krait_mux_clk *mux, int sel)
>   		regval |= (sel & mux->mask) << (mux->shift + LPL_SHIFT);
>   	}
>   	krait_set_l2_indirect_reg(mux->offset, regval);
> -	spin_unlock_irqrestore(&krait_clock_reg_lock, flags);
>   
>   	/* Wait for switch to complete. */
>   	mb();
>   	udelay(1);
> +
> +	/*
> +	 * Unlock now to make sure the mux register is not
> +	 * modified while switching to the new parent.
> +	 */
> +	spin_unlock_irqrestore(&krait_clock_reg_lock, flags);
>   }
>   
>   static int krait_mux_set_parent(struct clk_hw *hw, u8 index)


-- 
With best wishes
Dmitry

Re: [PATCH 1/4] clk: qcom: clk-hfpll: use poll_timeout macro

[Dmitry Baryshkov]

On 29/04/2022 15:01, Ansuel Smith wrote:
> Use regmap_read_poll_timeout macro instead of do-while structure to tidy
> things up. Also set a timeout to prevent any sort of system stall.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
>   drivers/clk/qcom/clk-hfpll.c | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/qcom/clk-hfpll.c b/drivers/clk/qcom/clk-hfpll.c
> index e847d586a73a..7dd17c184b69 100644
> --- a/drivers/clk/qcom/clk-hfpll.c
> +++ b/drivers/clk/qcom/clk-hfpll.c
> @@ -72,13 +72,16 @@ static void __clk_hfpll_enable(struct clk_hw *hw)
>   	regmap_update_bits(regmap, hd->mode_reg, PLL_RESET_N, PLL_RESET_N);
>   
>   	/* Wait for PLL to lock. */
> -	if (hd->status_reg) {
> -		do {
> -			regmap_read(regmap, hd->status_reg, &val);
> -		} while (!(val & BIT(hd->lock_bit)));
> -	} else {
> +	if (hd->status_reg)
> +		/*
> +		 * Busy wait. Should never timeout, we add a timeout to
> +		 * prevent any sort of stall.
> +		 */
> +		regmap_read_poll_timeout(regmap, hd->status_reg, val,
> +					 !(val & BIT(hd->lock_bit)), 0,
> +					 100 * USEC_PER_MSEC);
> +	else
>   		udelay(60);
> -	}
>   
>   	/* Enable PLL output. */
>   	regmap_update_bits(regmap, hd->mode_reg, PLL_OUTCTRL, PLL_OUTCTRL);


-- 
With best wishes
Dmitry

Re: [PATCH 3/4] clk: qcom: clk-krait: add hw_parent check for div2_round_rate

[Dmitry Baryshkov]

On 29/04/2022 15:01, Ansuel Smith wrote:
> Check if hw_parent is present before calculating the round_rate to
> prevent kernel panic. On error -EINVAL is reported.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>

I see that other clock drivers do not perform this check. Which path 
leads to this oops?

> ---
>   drivers/clk/qcom/clk-krait.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/qcom/clk-krait.c b/drivers/clk/qcom/clk-krait.c
> index 90046428693c..6c367ad6506a 100644
> --- a/drivers/clk/qcom/clk-krait.c
> +++ b/drivers/clk/qcom/clk-krait.c
> @@ -84,7 +84,12 @@ EXPORT_SYMBOL_GPL(krait_mux_clk_ops);
>   static long krait_div2_round_rate(struct clk_hw *hw, unsigned long rate,
>   				  unsigned long *parent_rate)
>   {
> -	*parent_rate = clk_hw_round_rate(clk_hw_get_parent(hw), rate * 2);
> +	struct clk_hw *hw_parent = clk_hw_get_parent(hw);
> +
> +	if (!hw_parent)
> +		return -EINVAL;
> +
> +	*parent_rate = clk_hw_round_rate(hw_parent, rate * 2);
>   	return DIV_ROUND_UP(*parent_rate, 2);
>   }
>   


-- 
With best wishes
Dmitry

Re: [PATCH 4/4] clk: qcom: clk-krait: add apq/ipq8064 errata workaround

[Dmitry Baryshkov]

On 29/04/2022 15:01, Ansuel Smith wrote:
> Add apq/ipq8064 errata workaround where the sec_src clock gating needs to
> be disabled during switching. To enable this set disable_sec_src_gating
> in the mux struct.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>   drivers/clk/qcom/clk-krait.c | 16 ++++++++++++++++
>   drivers/clk/qcom/clk-krait.h |  1 +
>   drivers/clk/qcom/krait-cc.c  |  1 +
>   3 files changed, 18 insertions(+)
> 
> diff --git a/drivers/clk/qcom/clk-krait.c b/drivers/clk/qcom/clk-krait.c
> index 6c367ad6506a..4a9b3296c45b 100644
> --- a/drivers/clk/qcom/clk-krait.c
> +++ b/drivers/clk/qcom/clk-krait.c
> @@ -18,13 +18,23 @@
>   static DEFINE_SPINLOCK(krait_clock_reg_lock);
>   
>   #define LPL_SHIFT	8
> +#define SECCLKAGD	BIT(4)
> +
>   static void __krait_mux_set_sel(struct krait_mux_clk *mux, int sel)
>   {
>   	unsigned long flags;
>   	u32 regval;
>   
>   	spin_lock_irqsave(&krait_clock_reg_lock, flags);
> +
>   	regval = krait_get_l2_indirect_reg(mux->offset);
> +
> +	/* apq/ipq8064 Errata: disable sec_src clock gating during switch. */
> +	if (mux->disable_sec_src_gating) {
> +		regval |= SECCLKAGD;
> +		krait_set_l2_indirect_reg(mux->offset, regval);
> +	}
> +
>   	regval &= ~(mux->mask << mux->shift);
>   	regval |= (sel & mux->mask) << mux->shift;
>   	if (mux->lpl) {
> @@ -33,6 +43,12 @@ static void __krait_mux_set_sel(struct krait_mux_clk *mux, int sel)
>   	}
>   	krait_set_l2_indirect_reg(mux->offset, regval);
>   
> +	/* apq/ipq8064 Errata: re-enabled sec_src clock gating. */
> +	if (mux->disable_sec_src_gating) {
> +		regval &= ~SECCLKAGD;
> +		krait_set_l2_indirect_reg(mux->offset, regval);
> +	}
> +
>   	/* Wait for switch to complete. */
>   	mb();
>   	udelay(1);
> diff --git a/drivers/clk/qcom/clk-krait.h b/drivers/clk/qcom/clk-krait.h
> index 9120bd2f5297..f930538c539e 100644
> --- a/drivers/clk/qcom/clk-krait.h
> +++ b/drivers/clk/qcom/clk-krait.h
> @@ -15,6 +15,7 @@ struct krait_mux_clk {
>   	u8		safe_sel;
>   	u8		old_index;
>   	bool		reparent;
> +	bool		disable_sec_src_gating;
>   
>   	struct clk_hw	hw;
>   	struct notifier_block   clk_nb;
> diff --git a/drivers/clk/qcom/krait-cc.c b/drivers/clk/qcom/krait-cc.c
> index 4d4b657d33c3..0f88bf41ec6e 100644
> --- a/drivers/clk/qcom/krait-cc.c
> +++ b/drivers/clk/qcom/krait-cc.c
> @@ -138,6 +138,7 @@ krait_add_sec_mux(struct device *dev, int id, const char *s,
>   	mux->parent_map = sec_mux_map;
>   	mux->hw.init = &init;
>   	mux->safe_sel = 0;
> +	mux->disable_sec_src_gating = true;

This has to be guarded with the of_compatible checks. Otherwise you'd 
apply this errata to all Krait CPUs, not only apq/ipq8064.

At least this should be limited to krait-cc-v1 with the note that there 
is no way to distinguish between platforms.

>   
>   	init.name = kasprintf(GFP_KERNEL, "krait%s_sec_mux", s);
>   	if (!init.name)


-- 
With best wishes
Dmitry

Re: [PATCH 3/4] clk: qcom: clk-krait: add hw_parent check for div2_round_rate

[Ansuel Smith]

On Fri, Apr 29, 2022 at 05:53:32PM +0300, Dmitry Baryshkov wrote:
> On 29/04/2022 15:01, Ansuel Smith wrote:
> > Check if hw_parent is present before calculating the round_rate to
> > prevent kernel panic. On error -EINVAL is reported.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> 
> I see that other clock drivers do not perform this check. Which path leads
> to this oops?
>

This comes from qsdk patches so I apologize in advance about this.

Anyway I'm checking the code and krait-cc is the only user of
krait_div2_clk_ops. That user have as parent only hfpll_something that
is declared by gcc. Now hfpll can also be declared in dts with a
dedicated driver so I wonder if the problem is there in the case when
hfpll is declared in dts and is probed after krait-cc. This is not the
case for ipq8064 but I wonder if qsdk have other krait based device that
have a configuration with hfpll declared in dts.

In short you are right and in our current code the check is uselss and
I'm positive about dropping this patch but I do wonder if downstream
there is an actual use of this. Don't know how to proceed. Any hint?

> > ---
> >   drivers/clk/qcom/clk-krait.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/qcom/clk-krait.c b/drivers/clk/qcom/clk-krait.c
> > index 90046428693c..6c367ad6506a 100644
> > --- a/drivers/clk/qcom/clk-krait.c
> > +++ b/drivers/clk/qcom/clk-krait.c
> > @@ -84,7 +84,12 @@ EXPORT_SYMBOL_GPL(krait_mux_clk_ops);
> >   static long krait_div2_round_rate(struct clk_hw *hw, unsigned long rate,
> >   				  unsigned long *parent_rate)
> >   {
> > -	*parent_rate = clk_hw_round_rate(clk_hw_get_parent(hw), rate * 2);
> > +	struct clk_hw *hw_parent = clk_hw_get_parent(hw);
> > +
> > +	if (!hw_parent)
> > +		return -EINVAL;
> > +
> > +	*parent_rate = clk_hw_round_rate(hw_parent, rate * 2);
> >   	return DIV_ROUND_UP(*parent_rate, 2);
> >   }
> 
> 
> -- 
> With best wishes
> Dmitry

-- 
	Ansuel

Re: [PATCH 4/4] clk: qcom: clk-krait: add apq/ipq8064 errata workaround

[Ansuel Smith]

On Fri, Apr 29, 2022 at 06:00:45PM +0300, Dmitry Baryshkov wrote:
> On 29/04/2022 15:01, Ansuel Smith wrote:
> > Add apq/ipq8064 errata workaround where the sec_src clock gating needs to
> > be disabled during switching. To enable this set disable_sec_src_gating
> > in the mux struct.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >   drivers/clk/qcom/clk-krait.c | 16 ++++++++++++++++
> >   drivers/clk/qcom/clk-krait.h |  1 +
> >   drivers/clk/qcom/krait-cc.c  |  1 +
> >   3 files changed, 18 insertions(+)
> > 
> > diff --git a/drivers/clk/qcom/clk-krait.c b/drivers/clk/qcom/clk-krait.c
> > index 6c367ad6506a..4a9b3296c45b 100644
> > --- a/drivers/clk/qcom/clk-krait.c
> > +++ b/drivers/clk/qcom/clk-krait.c
> > @@ -18,13 +18,23 @@
> >   static DEFINE_SPINLOCK(krait_clock_reg_lock);
> >   #define LPL_SHIFT	8
> > +#define SECCLKAGD	BIT(4)
> > +
> >   static void __krait_mux_set_sel(struct krait_mux_clk *mux, int sel)
> >   {
> >   	unsigned long flags;
> >   	u32 regval;
> >   	spin_lock_irqsave(&krait_clock_reg_lock, flags);
> > +
> >   	regval = krait_get_l2_indirect_reg(mux->offset);
> > +
> > +	/* apq/ipq8064 Errata: disable sec_src clock gating during switch. */
> > +	if (mux->disable_sec_src_gating) {
> > +		regval |= SECCLKAGD;
> > +		krait_set_l2_indirect_reg(mux->offset, regval);
> > +	}
> > +
> >   	regval &= ~(mux->mask << mux->shift);
> >   	regval |= (sel & mux->mask) << mux->shift;
> >   	if (mux->lpl) {
> > @@ -33,6 +43,12 @@ static void __krait_mux_set_sel(struct krait_mux_clk *mux, int sel)
> >   	}
> >   	krait_set_l2_indirect_reg(mux->offset, regval);
> > +	/* apq/ipq8064 Errata: re-enabled sec_src clock gating. */
> > +	if (mux->disable_sec_src_gating) {
> > +		regval &= ~SECCLKAGD;
> > +		krait_set_l2_indirect_reg(mux->offset, regval);
> > +	}
> > +
> >   	/* Wait for switch to complete. */
> >   	mb();
> >   	udelay(1);
> > diff --git a/drivers/clk/qcom/clk-krait.h b/drivers/clk/qcom/clk-krait.h
> > index 9120bd2f5297..f930538c539e 100644
> > --- a/drivers/clk/qcom/clk-krait.h
> > +++ b/drivers/clk/qcom/clk-krait.h
> > @@ -15,6 +15,7 @@ struct krait_mux_clk {
> >   	u8		safe_sel;
> >   	u8		old_index;
> >   	bool		reparent;
> > +	bool		disable_sec_src_gating;
> >   	struct clk_hw	hw;
> >   	struct notifier_block   clk_nb;
> > diff --git a/drivers/clk/qcom/krait-cc.c b/drivers/clk/qcom/krait-cc.c
> > index 4d4b657d33c3..0f88bf41ec6e 100644
> > --- a/drivers/clk/qcom/krait-cc.c
> > +++ b/drivers/clk/qcom/krait-cc.c
> > @@ -138,6 +138,7 @@ krait_add_sec_mux(struct device *dev, int id, const char *s,
> >   	mux->parent_map = sec_mux_map;
> >   	mux->hw.init = &init;
> >   	mux->safe_sel = 0;
> > +	mux->disable_sec_src_gating = true;
> 
> This has to be guarded with the of_compatible checks. Otherwise you'd apply
> this errata to all Krait CPUs, not only apq/ipq8064.
> 
> At least this should be limited to krait-cc-v1 with the note that there is
> no way to distinguish between platforms.
>

Mhh can't i check the machine compatible directly to limit this to
apq/ipq8064?

> >   	init.name = kasprintf(GFP_KERNEL, "krait%s_sec_mux", s);
> >   	if (!init.name)
> 
> 
> -- 
> With best wishes
> Dmitry

-- 
	Ansuel

Re: [PATCH 4/4] clk: qcom: clk-krait: add apq/ipq8064 errata workaround

[Dmitry Baryshkov]

On Fri, 29 Apr 2022 at 18:11, Ansuel Smith <ansuelsmth@gmail.com> wrote:
>
> On Fri, Apr 29, 2022 at 06:00:45PM +0300, Dmitry Baryshkov wrote:
> > On 29/04/2022 15:01, Ansuel Smith wrote:
> > > Add apq/ipq8064 errata workaround where the sec_src clock gating needs to
> > > be disabled during switching. To enable this set disable_sec_src_gating
> > > in the mux struct.
> > >
> > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > > ---
> > >   drivers/clk/qcom/clk-krait.c | 16 ++++++++++++++++
> > >   drivers/clk/qcom/clk-krait.h |  1 +
> > >   drivers/clk/qcom/krait-cc.c  |  1 +
> > >   3 files changed, 18 insertions(+)
> > >
> > > diff --git a/drivers/clk/qcom/clk-krait.c b/drivers/clk/qcom/clk-krait.c
> > > index 6c367ad6506a..4a9b3296c45b 100644
> > > --- a/drivers/clk/qcom/clk-krait.c
> > > +++ b/drivers/clk/qcom/clk-krait.c
> > > @@ -18,13 +18,23 @@
> > >   static DEFINE_SPINLOCK(krait_clock_reg_lock);
> > >   #define LPL_SHIFT 8
> > > +#define SECCLKAGD  BIT(4)
> > > +
> > >   static void __krait_mux_set_sel(struct krait_mux_clk *mux, int sel)
> > >   {
> > >     unsigned long flags;
> > >     u32 regval;
> > >     spin_lock_irqsave(&krait_clock_reg_lock, flags);
> > > +
> > >     regval = krait_get_l2_indirect_reg(mux->offset);
> > > +
> > > +   /* apq/ipq8064 Errata: disable sec_src clock gating during switch. */
> > > +   if (mux->disable_sec_src_gating) {
> > > +           regval |= SECCLKAGD;
> > > +           krait_set_l2_indirect_reg(mux->offset, regval);
> > > +   }
> > > +
> > >     regval &= ~(mux->mask << mux->shift);
> > >     regval |= (sel & mux->mask) << mux->shift;
> > >     if (mux->lpl) {
> > > @@ -33,6 +43,12 @@ static void __krait_mux_set_sel(struct krait_mux_clk *mux, int sel)
> > >     }
> > >     krait_set_l2_indirect_reg(mux->offset, regval);
> > > +   /* apq/ipq8064 Errata: re-enabled sec_src clock gating. */
> > > +   if (mux->disable_sec_src_gating) {
> > > +           regval &= ~SECCLKAGD;
> > > +           krait_set_l2_indirect_reg(mux->offset, regval);
> > > +   }
> > > +
> > >     /* Wait for switch to complete. */
> > >     mb();
> > >     udelay(1);
> > > diff --git a/drivers/clk/qcom/clk-krait.h b/drivers/clk/qcom/clk-krait.h
> > > index 9120bd2f5297..f930538c539e 100644
> > > --- a/drivers/clk/qcom/clk-krait.h
> > > +++ b/drivers/clk/qcom/clk-krait.h
> > > @@ -15,6 +15,7 @@ struct krait_mux_clk {
> > >     u8              safe_sel;
> > >     u8              old_index;
> > >     bool            reparent;
> > > +   bool            disable_sec_src_gating;
> > >     struct clk_hw   hw;
> > >     struct notifier_block   clk_nb;
> > > diff --git a/drivers/clk/qcom/krait-cc.c b/drivers/clk/qcom/krait-cc.c
> > > index 4d4b657d33c3..0f88bf41ec6e 100644
> > > --- a/drivers/clk/qcom/krait-cc.c
> > > +++ b/drivers/clk/qcom/krait-cc.c
> > > @@ -138,6 +138,7 @@ krait_add_sec_mux(struct device *dev, int id, const char *s,
> > >     mux->parent_map = sec_mux_map;
> > >     mux->hw.init = &init;
> > >     mux->safe_sel = 0;
> > > +   mux->disable_sec_src_gating = true;
> >
> > This has to be guarded with the of_compatible checks. Otherwise you'd apply
> > this errata to all Krait CPUs, not only apq/ipq8064.
> >
> > At least this should be limited to krait-cc-v1 with the note that there is
> > no way to distinguish between platforms.
> >
>
> Mhh can't i check the machine compatible directly to limit this to
> apq/ipq8064?

Yes, use of_machine_is_compatible(). Add a comment that you have to do
this because of the lack of soc-specific device compats.

> > >     init.name = kasprintf(GFP_KERNEL, "krait%s_sec_mux", s);
> > >     if (!init.name)
> >
> >
> > --
> > With best wishes
> > Dmitry
>
> --
>         Ansuel



-- 
With best wishes
Dmitry

Re: [PATCH 3/4] clk: qcom: clk-krait: add hw_parent check for div2_round_rate

[Dmitry Baryshkov]

On Fri, 29 Apr 2022 at 18:08, Ansuel Smith <ansuelsmth@gmail.com> wrote:
>
> On Fri, Apr 29, 2022 at 05:53:32PM +0300, Dmitry Baryshkov wrote:
> > On 29/04/2022 15:01, Ansuel Smith wrote:
> > > Check if hw_parent is present before calculating the round_rate to
> > > prevent kernel panic. On error -EINVAL is reported.
> > >
> > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> >
> > I see that other clock drivers do not perform this check. Which path leads
> > to this oops?
> >
>
> This comes from qsdk patches so I apologize in advance about this.

Ugh. If it comes from the code authored by somebody else, it'd be
better to note this (by using the From or Co-developed-by tags).
At the very least (if the author is unknown) you can mention the
origin of the patch (qsdk) in the commit message.

>
> Anyway I'm checking the code and krait-cc is the only user of
> krait_div2_clk_ops. That user have as parent only hfpll_something that
> is declared by gcc. Now hfpll can also be declared in dts with a
> dedicated driver so I wonder if the problem is there in the case when
> hfpll is declared in dts and is probed after krait-cc. This is not the
> case for ipq8064 but I wonder if qsdk have other krait based device that
> have a configuration with hfpll declared in dts.

On msm8974 (and maybe others) the hfpll should be driven by the
separate hfpll driver.

>
> In short you are right and in our current code the check is uselss and
> I'm positive about dropping this patch but I do wonder if downstream
> there is an actual use of this. Don't know how to proceed. Any hint?

I'd say, let's drop it for now unless Stephen or Bjorn tell us that
it's a valid check.

>
> > > ---
> > >   drivers/clk/qcom/clk-krait.c | 7 ++++++-
> > >   1 file changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/clk/qcom/clk-krait.c b/drivers/clk/qcom/clk-krait.c
> > > index 90046428693c..6c367ad6506a 100644
> > > --- a/drivers/clk/qcom/clk-krait.c
> > > +++ b/drivers/clk/qcom/clk-krait.c
> > > @@ -84,7 +84,12 @@ EXPORT_SYMBOL_GPL(krait_mux_clk_ops);
> > >   static long krait_div2_round_rate(struct clk_hw *hw, unsigned long rate,
> > >                               unsigned long *parent_rate)
> > >   {
> > > -   *parent_rate = clk_hw_round_rate(clk_hw_get_parent(hw), rate * 2);
> > > +   struct clk_hw *hw_parent = clk_hw_get_parent(hw);
> > > +
> > > +   if (!hw_parent)
> > > +           return -EINVAL;
> > > +
> > > +   *parent_rate = clk_hw_round_rate(hw_parent, rate * 2);
> > >     return DIV_ROUND_UP(*parent_rate, 2);
> > >   }
> >
> >
> > --
> > With best wishes
> > Dmitry
>
> --
>         Ansuel



-- 
With best wishes
Dmitry