Hi Laurent,

On 3/21/21 8:01 AM, Laurent Pinchart wrote:

Hi Quanyang,

Thank you for the patch.

On Fri, Mar 19, 2021 at 06:07:17PM +0800, quanyang.wang@windriver.com wrote:

quoted

From: Quanyang Wang <redacted>

If there is a IOCTL_SET_PLL_FRAC_MODE request sent to ATF ever,
we shouldn't skip invoking PM_CLOCK_ENABLE fn even though this
pll has been enabled. In ATF implementation, it will only assign
the mode to the variable (struct pm_pll *)pll->mode when handling
IOCTL_SET_PLL_FRAC_MODE call. Invoking PM_CLOCK_ENABLE can force
ATF send request to PWU to set the pll mode to PLL's register.

There is a scenario that happens in enabling VPLL_INT(clk_id:96):
1) VPLL_INT has been enabled during booting.
2) A driver calls clk_set_rate and according to the rate, the VPLL_INT
    should be set to FRAC mode. Then zynqmp_pll_set_mode is called
    to pass IOCTL_SET_PLL_FRAC_MODE to ATF. Note that at this point
    ATF just stores the mode to a variable.
3) This driver calls clk_prepare_enable and zynqmp_pll_enable is
    called to try to enable VPLL_INT pll. Because of 1), the function
    zynqmp_pll_enable just returns without doing anything after checking
    that this pll has been enabled.

In the scenario above, the pll mode of VPLL_INT will never be set
successfully. So adding set_pll_mode to chec condition to fix it.

s/chec/check/

quoted

Signed-off-by: Quanyang Wang <redacted>
---
  drivers/clk/zynqmp/pll.c | 11 ++++++++++-
  1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/zynqmp/pll.c b/drivers/clk/zynqmp/pll.c
index 92f449ed38e5..f1e8f37d7f52 100644
--- a/drivers/clk/zynqmp/pll.c
+++ b/drivers/clk/zynqmp/pll.c

@@ -14,10 +14,12 @@
   * struct zynqmp_pll - PLL clock
   * @hw:		Handle between common and hardware-specific interfaces
   * @clk_id:	PLL clock ID
+ * @set_pll_mode:	Whether an IOCTL_SET_PLL_FRAC_MODE request be sent to ATF
   */
  struct zynqmp_pll {
  	struct clk_hw hw;
  	u32 clk_id;
+	bool set_pll_mode;
  };
  
  #define to_zynqmp_pll(_hw)	container_of(_hw, struct zynqmp_pll, hw)

@@ -81,6 +83,8 @@ static inline void zynqmp_pll_set_mode(struct clk_hw *hw, bool on)
  	if (ret)
  		pr_warn_once("%s() PLL set frac mode failed for %s, ret = %d\n",
  			     __func__, clk_name, ret);
+	else
+		clk->set_pll_mode = true;
  }
  
  /**

@@ -240,9 +244,14 @@ static int zynqmp_pll_enable(struct clk_hw *hw)
  	u32 clk_id = clk->clk_id;
  	int ret;
  
-	if (zynqmp_pll_is_enabled(hw))
+	/* Don't skip enabling clock if there is an IOCTL_SET_PLL_FRAC_MODE request
+	 * that has been sent to ATF.
+	 */

Very small issue, multiline kerneldoc comments are supposed to start
with a '/*' on its own line:

	/*
	 * Don't skip enabling clock if there is an IOCTL_SET_PLL_FRAC_MODE
	 * request that has been sent to ATF.
	 */

quoted

+	if (zynqmp_pll_is_enabled(hw) && (!clk->set_pll_mode))
  		return 0;
  
+	clk->set_pll_mode = false;
+
  	ret = zynqmp_pm_clock_enable(clk_id);
  	if (ret)
  		pr_warn_once("%s() clock enable failed for %s, ret = %d\n",

This fixes the DPSUB clock issue, so

Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I however wonder if this is the best solution. Shouldn't we instead fix
it on the ATF side, to program the hardware when zynqmp_pll_set_mode()
is called if the clock is already enabled ?

Just reading the code, I can immediately see another potential issue in
zynqmp_pll_set_mode(). The function is called from
zynqmp_pll_round_rate(), which seems completely wrong, as
zynqmp_pll_round_rate() is supposed to only perform rate calculation,
not program the hardware. Am I missing something, or does the PLL
implementation need to be reworked more extensively than this ?

I will send another patch to fix this issue.

Thanks,

Quanyang

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel