Re: [PATCH 1/2] clk: zynqmp: pll: add set_pll_mode to check condition in zynqmp_pll_enable

[quanyang.wang]

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@xxxxxxxxxxxxx wrote:
> > From: Quanyang Wang <quanyang.wang@xxxxxxxxxxxxx>
> >
> > 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/
Thanks for pointing it out. Will fix it in the next version.
> > Signed-off-by: Quanyang Wang <quanyang.wang@xxxxxxxxxxxxx>
> > ---
> >   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.
> 	 */
OK. Will fix it in the next version.
> > +	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@xxxxxxxxxxxxxxxx>
>
> 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 ?

I found the commit message which records the reason of this change. And 
in this commit,

it shows that the code is changing from programming the hardware to 
buffering the mode.


https://github.com/Xilinx/arm-trusted-firmware.git

commit 8975f317e7608c832192b71531901602dc625484
Author: Jolly Shah <jollys@xxxxxxxxxx>
Date:   Wed Jan 2 12:46:46 2019 -0800

    zynqmp: pm: Buffer the PLL mode that is set using IOCTL API

    When linux calls pm_ioctl_set_pll_frac_mode() it doesn't expect the
    fractional mode to be changed in hardware. Furthermore, even before 
this
    patch setting the mode which is done by writing into register takes
    no effect until the PLL reset is deasserted, i.e. until linux "enables"
    the PLL. To adjust the code to system-level PLL EEMI API and avoid
    unnecessary IPIs that would otherwise be issued, we buffer the mode
    value set via IOCTL until the PLL mode really needs to be set.

> 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.

I agree.  Moving zynqmp_pll_set_mode out of zynqmp_pll_round_rate

and putting it in zynqmp_pll_set_rate may be better.

Thanks,

Quanyang

>   Am I missing something, or does the PLL
> implementation need to be reworked more extensively than this ?