Re: [PATCH v4 0/6] drm: lcdif: Add i.MX93 LCDIF support

From: Alexander Stein
Date: Mon Feb 20 2023 - 03:55:32 EST

Hi Liu,

Am Freitag, 17. Februar 2023, 09:59:14 CET schrieb Liu Ying:
> On Fri, 2023-02-17 at 09:18 +0100, Alexander Stein wrote:
> > Hi Liu,
>
> Hi Alexander,
>
> > Am Freitag, 17. Februar 2023, 07:54:01 CET schrieb Liu Ying:
> > > Hi,
> > >
> > > This patch set aims to add i.MX93 LCDIF display controller support
> > > in the existing LCDIF DRM driver. The LCDIF embedded in i.MX93 SoC
> > > is essentially the same to those embedded in i.MX8mp SoC. Through
> > > internal bridges, i.MX93 LCDIF may drive a MIPI DSI display or a LVDS
> > > display or a parallel display.
> > >
> > > Patch 1/6 adds device tree binding support for i.MX93 LCDIF in the
> > > existing fsl,lcdif.yaml.
> > >
> > > Patch 2/6 drops lcdif->bridge NULL pointer check as a cleanup patch.
> > >
> > > Patch 3/6~5/6 prepare for adding i.MX93 LCDIF support step by step.
> > >
> > > Patch 6/6 adds i.MX93 LCDIF compatible string as the last step of
> > > adding i.MX93 LCDIF support.
> >
> > Thanks for the series. I could test this on my TQMa93xxLA/MBa93xxCA with a
> > single LVDS display attached, so no DSI or parallel display. Hence I could
> > not test the bus format and flags checks, but they look okay.
> > So you can add
> > Tested-by: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
> > to the whole series as well.
>
> Thanks for your test.
>
> > One thing I noticed is that, sometimes it seems that before probing lcdif
> > my system completely freezes. Adding some debug output it seems that's
> > during powering up the IMX93_MEDIABLK_PD_LCDIF power domain there is some
> > race condition. But adding more more detailed output made the problem go
> > away. Did you notice something similar? It might be a red hering though.
> I don't see system freezing with my i.MX93 11x11 EVK when probing
> lcdif. I did try to boot the system several times. All look ok. This is
> a snippet of dmesg when lcdif probes:
>
> --------------------------8<------------------------------------------
> [ 0.753083] Serial: 8250/16550 driver, 4 ports, IRQ sharing enabled
> [ 0.761669] SuperH (H)SCI(F) driver initialized
> [ 0.766523] msm_serial: driver initialized
> [ 0.780523] printk: console [ttyLP0] enabled0x44380010 (irq = 16,
> base_baud = 1500000) is a FSL_LPUART
> [ 0.780523] printk: console [ttyLP0] enabled
> [ 0.788928] printk: bootconsole [lpuart32] disabled
> [ 0.788928] printk: bootconsole [lpuart32] disabled
> [ 0.804632] panel-simple lvds_panel: supply power not found, using
> dummy regulator
> [ 0.814741] [drm] Initialized imx-lcdif 1.0.0 20220417 for
> 4ae30000.lcd-controller on minor 0
> [ 1.195930] Console: switching to colour frame buffer device 160x50
> [ 1.218385] imx-lcdif 4ae30000.lcd-controller: [drm] fb0: imx-
> lcdifdrmfb frame buffer device
> [ 1.227099] cacheinfo: Unable to detect cache hierarchy for CPU 0
> [ 1.236725] loop: module loaded
> --------------------------8<------------------------------------------
>
> ~300 milliseconds are consumed by the enablement delay required by the
> "boe,ev121wxm-n10-1850" LVDS panel I use.

It seems you have the drivers compiled in. I use modules in my case and
simple-panel as well. But this is unrelated, because lcdif_probe() is yet to
be called. Using the debug diff from below I get the following output:

[ 16.111197] imx93-blk-ctrl 4ac10000.system-controller:
imx93_blk_ctrl_power_on: 1
[ 16.122491] imx93-blk-ctrl 4ac10000.system-controller:
imx93_blk_ctrl_power_on: 2
[ 16.137766] imx93-blk-ctrl 4ac10000.system-controller:
imx93_blk_ctrl_power_on: 3
[ 16.154905] imx93-blk-ctrl 4ac10000.system-controller:
imx93_blk_ctrl_power_on: 4

It seems setting BLK_CLK_EN blocks the whole system, even reading is not
possible. I don't have any details on the hardware, but it seems that either
some clock or power domain is not enabled. This can also happen if I'm loading
the lcdif module manually after boot. But I can't detect any differences in /
sys/kernel/debug/clk/clk_summary.

---8<---
diff --git a/drivers/soc/imx/imx93-blk-ctrl.c b/drivers/soc/imx/imx93-blk-
ctrl.c
index 2c600329436cf..50aeb20ce90dc 100644
--- a/drivers/soc/imx/imx93-blk-ctrl.c
+++ b/drivers/soc/imx/imx93-blk-ctrl.c
@@ -129,12 +129,14 @@ static int imx93_blk_ctrl_power_on(struct
generic_pm_domain *genpd)
struct imx93_blk_ctrl *bc = domain->bc;
int ret;

+ dev_info(bc->dev, "%s: 1\n", __func__);
ret = clk_bulk_prepare_enable(bc->num_clks, bc->clks);
if (ret) {
dev_err(bc->dev, "failed to enable bus clocks\n");
return ret;
}

+ dev_info(bc->dev, "%s: 2\n", __func__);
ret = clk_bulk_prepare_enable(data->num_clks, domain->clks);
if (ret) {
clk_bulk_disable_unprepare(bc->num_clks, bc->clks);
@@ -142,6 +144,7 @@ static int imx93_blk_ctrl_power_on(struct
generic_pm_domain *genpd)
return ret;
}

+ dev_info(bc->dev, "%s: 3\n", __func__);
ret = pm_runtime_get_sync(bc->dev);
if (ret < 0) {
pm_runtime_put_noidle(bc->dev);
@@ -149,11 +152,15 @@ static int imx93_blk_ctrl_power_on(struct
generic_pm_domain *genpd)
goto disable_clk;
}

+ dev_info(bc->dev, "%s: 4\n", __func__);
+
/* ungate clk */
regmap_clear_bits(bc->regmap, BLK_CLK_EN, data->clk_mask);
+ dev_info(bc->dev, "%s: 5\n", __func__);

/* release reset */
regmap_set_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
+ dev_info(bc->dev, "%s: 6\n", __func__);

dev_dbg(bc->dev, "pd_on: name: %s\n", genpd->name);


---8<---

Best regards,
Alexander

> Regards,
> Liu Ying
>
> > Best regards,
> > Alexander
> >
> > > v3->v4:
> > > * Improve warning message when ignoring invalid LCDIF OF endpoint ids in
> > >
> > > patch 5/6. (Alexander)
> > >
> > > * Use 'new_{c,p}state' instead of 'new_{crtc,plane}_state' in patch 3/6.
> > >
> > > (Alexander)
> > >
> > > * Simplify lcdif_crtc_reset() by calling
> > > lcdif_crtc_atomic_destroy_state()
> > >
> > > in patch 3/6. (Alexander)
> > >
> > > * Add '!crtc->state' check in lcdif_crtc_atomic_duplicate_state() in
> > > patch
> > > 3/6. (Alexander)
> > > * Collect Alexander's R-b tags on patch 1/6, 2/6 and 6/6.
> > >
> > > v2->v3:
> > > * Fix a trivial typo in patch 6/6's commit message.
> > >
> > > v1->v2:
> > > * Add Krzysztof's A-b and Marek's R-b tags on patch 1/6.
> > > * Split patch 2/2 in v1 into patch 2/6~6/6 in v2. (Marek, Alexander)
> > > * Drop '!remote ||' from lcdif_attach_bridge(). (Lothar)
> > > * Add comment on the 'base' member of lcdif_crtc_state structure to
> > >
> > > note it should always be the first member. (Lothar)
> > >
> > > * Drop unneeded 'bridges' member from lcdif_drm_private structure.
> > > * Drop a comment about bridge input bus format from
> > > lcdif_crtc_atomic_check().
> > >
> > > Liu Ying (6):
> > > dt-bindings: lcdif: Add i.MX93 LCDIF support
> > > drm: lcdif: Drop unnecessary NULL pointer check on lcdif->bridge
> > > drm: lcdif: Determine bus format and flags in ->atomic_check()
> > > drm: lcdif: Check consistent bus format and flags across first bridges
> > > drm: lcdif: Add multiple encoders and first bridges support
> > > drm: lcdif: Add i.MX93 LCDIF compatible string
> > >
> > > .../bindings/display/fsl,lcdif.yaml | 7 +-
> > > drivers/gpu/drm/mxsfb/lcdif_drv.c | 71 ++++++-
> > > drivers/gpu/drm/mxsfb/lcdif_drv.h | 5 +-
> > > drivers/gpu/drm/mxsfb/lcdif_kms.c | 198 ++++++++++++------
> > > 4 files changed, 206 insertions(+), 75 deletions(-)


--
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/