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:
- DP device enters suspend state <- call zynqmp_gpd_power_off
- zynqmp_disp_crtc_setup_clock <- configurate register VPLL_FRAC_CFG
- pm_runtime_get_sync <- call zynqmp_gpd_power_on and clear previous VPLL_FRAC_CFG configuration
- 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");