Re: [PATCH] drm: xlnx: call pm_runtime_get_sync before setting pixel clock

[Michal Simek <michal.simek@xilinx.com>]

+Rohit

On 3/17/21 4:00 AM, quanyang.wang wrote:
> Hi Laurent,
> 
> On 3/17/21 4:32 AM, Laurent Pinchart wrote:
>> Hi Quanyang,
>>
>> Thank you for the patch.
>>
>> On Wed, Mar 10, 2021 at 12:59:45PM +0800, quanyang.wang@windriver.com
>> wrote:
>>> From: Quanyang Wang <quanyang.wang@windriver.com>
>>>
>>> The Runtime PM subsystem will force the device "fd4a0000.zynqmp-display"
>>> to enter suspend state while booting if the following conditions are
>>> met:
>>> - the usage counter is zero (pm_runtime_get_sync hasn't been called yet)
>>> - no 'active' children (no zynqmp-dp-snd-xx node under dpsub node)
>>> - no other device in the same power domain (dpdma node has no
>>>         "power-domains = <&zynqmp_firmware PD_DP>" property)
>>>
>>> So there is a scenario as below:
>>> 1) DP device enters suspend state   <- call zynqmp_gpd_power_off
>>> 2) zynqmp_disp_crtc_setup_clock        <- configurate register
>>> VPLL_FRAC_CFG
>>> 3) pm_runtime_get_sync            <- call zynqmp_gpd_power_on and
>>> clear previous
>>>                        VPLL_FRAC_CFG configuration
>>> 4) clk_prepare_enable(disp->pclk)   <- enable failed since VPLL_FRAC_CFG
>>>                        configuration is corrupted
>>>
>>>  From above, we can see that pm_runtime_get_sync may clear register
>>> VPLL_FRAC_CFG configuration and result the failure of clk enabling.
>>> Putting pm_runtime_get_sync at the very beginning of the function
>>> zynqmp_disp_crtc_atomic_enable can resolve this issue.
>> Isn't this an issue in the firmware though, which shouldn't clear the
>> previous VPLLF_FRAC_CFG ?
> 
> Thank you for your review.  I used to look into the atf and PWU code and
> it seems (I didn't add debug code
> 
> to PMU to make sure if PMU really does this, I only in kernel call
> zynqmp_pm_get_pll_frac_data to make sure that
> 
> the value in data field of VPLL_FRAC_CFG register is changed from 0x4000
> to 0x0 after run pm_runtime_get_sync)
> 
> that PMU intends to reset VPLL when there is an  Off and On in DP
> Powerdomain.
> 
> 
> Linux ATF                                 PWU
> 
> zynqmp_gpd_power_on
> ->zynqmp_pm_set_requirement
> -->send PM_SET_REQUIREMENT to ATF  ==>        ATF send ipi to PWU  
> ==>   PmSetRequirement
>                                     ->PmRequirementUpdate
>                                     -->PmUpdateSlave(masterReq->slave)
>                                     --->PmSlaveChangeState
>                                      ---->PmSlaveChangeState
>                                      ----->PmSlaveClearAfterState
>                                      ------>PmClockRelease
>                                     
> ------->PmClockReleaseInt(&ch->clock->base)
>                                      -------->clk->class->release(clk)
>                                      --------->PmPllBypassAndReset
> //Here reset the VPLL then VPLL_FRAC_CFG is cleared.
>     
>>
>>> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
>> Nonetheless, this change looks good to me, I actually had the same patch
>> in my tree while investigation issues related to the clock rate, so
>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>
>> I was hoping it would solve the issue I'm experiencing with the DP
>> clock, but that's not the case :-( In a nutshell, when the DP is first
>> started, the clock frequency is incorrect. The following quick & dirty
>> patch fixes the problem:
>>
>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> index 74ac0a064eb5..fdbe1b0640aa 100644
>> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>> @@ -1439,6 +1439,10 @@ zynqmp_disp_crtc_atomic_enable(struct drm_crtc
>> *crtc,
>>
>>       pm_runtime_get_sync(disp->dev);
>>
>> +    ret = clk_prepare_enable(disp->pclk);
>> +    if (!ret)
>> +        clk_disable_unprepare(disp->pclk);
>> +
>>       zynqmp_disp_crtc_setup_clock(crtc, adjusted_mode);
>>
>>       ret = clk_prepare_enable(disp->pclk);
>>
>> The problem doesn't seem to be in the kernel, but on the TF-A or PMU
>> firmware side. Have you experienced this by any chance ?
> 
> Yes,  I bumped into the same issue and I used to make a patch (Patch 1)
> as below.
> 
> I didn't send it to mainline because it seems not to be a driver issue.
> The mode of VPLL
> 
> is not set correctly because:
> 
> 1) VPLL is enabled before linux
> 
> 2) linux calling pm_clock_set_pll_mode can't really set register because
> in ATF
> 
> it only store the mode value to a structure and wait a clk-enable
> request to do
> 
> the register-set operation.
> 
> 3) linux call clk_enable will not send a clk-enable request since it
> checks that
> 
> the VPLL is already hardware-enabled because of 1).
> 
> So the firmware should disable VPLL when it exits and also in linux
> zynqmp clk driver
> 
> there should be a check list to reset some clks to a predefined state.
> 
> 
> By the way, there is a tiny patch (Patch 2) to fix the black screen
> issue in DP. I think you may
> 
> be preparing a big patch which add drm properties to this driver and it
> may contain this modification,
> 
> so I didn't send it.
> 
> 
> Thanks,
> 
> Quanyang
> 
> Patch 1:
> 
> From 93311de2c61e87f2426b89259d81cded71aee673 Mon Sep 17 00:00:00 2001
> From: Quanyang Wang <quanyang.wang@windriver.com>
> Date: Thu, 3 Dec 2020 19:19:50 +0800
> Subject: [PATCH 1/3] drm: xlnx: set PLL_MODE_FRAC mode to VPLL_FRAC_CFG by
>  re-enabling disp->pclk
> 
> When the function clk_set_rate configures the rate of disp->pclk,
> zynqmp_pm_set_pll_frac_mode will be called to set VPLL's mode to
> be PLL_MODE_FRAC or PLL_MODE_INT by invoking an SMC call to ATF.
> But in ATF, the service pm_clock_set_pll_mode doesn't really set
> the VPLL_FRAC_CFG register but only stores the mode value to
> struct pm_pll *pll. The operation that sets the register must be
> triggered by zynqmp_pm_clock_enable.
> 
> Since disp->pclk is enabled in hardware before linux booting,
> clk_prepare_enable will skip over zynqmp_pm_clock_enable. So
> we have to enable then disable disp->pclk, and re-enable it to
> make sure that zynqmp_pm_clock_enable is triggered and the mode is
> set to VPLL_FRAC_CFG. Or else VPLL will work in an incorrect mode.
> 
> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
> ---
>  drivers/gpu/drm/xlnx/zynqmp_disp.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index 98bd48f13fd1..19753ffc424e 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -1668,6 +1668,11 @@ int zynqmp_disp_probe(struct zynqmp_dpsub *dpsub,
> struct drm_device *drm)
>             dev_err(disp->dev, "failed to init any video clock\n");
>             return PTR_ERR(disp->pclk);
>         }
> +
> +       /* Make sure that disp->pclk is disabled in hardware */
> +       ret = clk_prepare_enable(disp->pclk);
> +       clk_disable_unprepare(disp->pclk);
> +
>         disp->pclk_from_ps = true;
>     }
> 
> Patch 2:
> 
> From 8c6d36bcb4e79e0e5f8e157446cd994b4a2126f0 Mon Sep 17 00:00:00 2001
> From: Quanyang Wang <quanyang.wang@windriver.com>
> Date: Thu, 3 Dec 2020 19:32:56 +0800
> Subject: [PATCH 2/3] drm: xlnx: configure alpha value to make graphic layer
>  opaque
> 
> Since graphics layer is primary, and video layer is overaly,
> we need to configure the V_BLEND_SET_GLOBAL_ALPHA_REG register
> to make graphic layer opaque by default, or else graphic layer
> will be transparent and invisible.
> 
> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
> ---
>  drivers/gpu/drm/xlnx/zynqmp_disp.c      | 3 ++-
>  drivers/gpu/drm/xlnx/zynqmp_disp_regs.h | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> index 19753ffc424e..5c84589e1899 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
> @@ -1468,7 +1468,8 @@ zynqmp_disp_crtc_atomic_enable(struct drm_crtc *crtc,
>     zynqmp_disp_blend_set_output_format(&disp->blend,
>                         ZYNQMP_DPSUB_FORMAT_RGB);
>     zynqmp_disp_blend_set_bg_color(&disp->blend, 0, 0, 0);
> -   zynqmp_disp_blend_set_global_alpha(&disp->blend, false, 0);
> +   zynqmp_disp_blend_set_global_alpha(&disp->blend, true,
> +                  ZYNQMP_DISP_V_BLEND_SET_GLOBAL_ALPHA_MAX);
> 
>     zynqmp_disp_enable(disp);
> 
> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> index f92a006d5070..ef409aca11ad 100644
> --- a/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp_regs.h
> @@ -22,6 +22,7 @@
>  #define ZYNQMP_DISP_V_BLEND_SET_GLOBAL_ALPHA       0xc
>  #define ZYNQMP_DISP_V_BLEND_SET_GLOBAL_ALPHA_VALUE(n)  ((n) << 1)
>  #define ZYNQMP_DISP_V_BLEND_SET_GLOBAL_ALPHA_EN        BIT(0)
> +#define ZYNQMP_DISP_V_BLEND_SET_GLOBAL_ALPHA_MAX   0xff
>  #define ZYNQMP_DISP_V_BLEND_OUTPUT_VID_FMT     0x14
>  #define ZYNQMP_DISP_V_BLEND_OUTPUT_VID_FMT_RGB     0x0
>  #define ZYNQMP_DISP_V_BLEND_OUTPUT_VID_FMT_YCBCR444    0x1
> 
>>> ---
>>>   drivers/gpu/drm/xlnx/zynqmp_disp.c | 3 ++-
>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c
>>> b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>>> index 148add0ca1d6..909e6c265406 100644
>>> --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c
>>> +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c
>>> @@ -1445,9 +1445,10 @@ zynqmp_disp_crtc_atomic_enable(struct drm_crtc
>>> *crtc,
>>>       struct drm_display_mode *adjusted_mode =
>>> &crtc->state->adjusted_mode;
>>>       int ret, vrefresh;
>>>   +    pm_runtime_get_sync(disp->dev);
>>> +
>>>       zynqmp_disp_crtc_setup_clock(crtc, adjusted_mode);
>>>   -    pm_runtime_get_sync(disp->dev);
>>>       ret = clk_prepare_enable(disp->pclk);
>>>       if (ret) {
>>>           dev_err(disp->dev, "failed to enable a pixel clock\n");

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