From:   Alexander Stein <alexander.stein@ew.tq-group.com>
To:     Liu Ying <victor.liu@nxp.com>
Cc:     dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        marex@denx.de, stefan@agner.ch, airlied@gmail.com, daniel@ffwll.ch,
        robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
        shawnguo@kernel.org, s.hauer@pengutronix.de, kernel@pengutronix.de,
        festevam@gmail.com, linux-imx@nxp.com,
        krzysztof.kozlowski@linaro.org, LW@karo-electronics.de
Subject: Re: [PATCH v4 0/6] drm: lcdif: Add i.MX93 LCDIF support
Date:   Mon, 20 Feb 2023 09:55:19 +0100
Message-ID: <2135575.irdbgypaU6@steina-w>
Organization: TQ-Systems GmbH
In-Reply-To: <52b8025ee9b71dfb147127bd1cb2c532d222df3c.camel@nxp.com>
References: <20230217065407.2259731-1-victor.liu@nxp.com> <13207614.uLZWGnKmhe@steina-w> <52b8025ee9b71dfb147127bd1cb2c532d222df3c.camel@nxp.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
Precedence: bulk

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,
>=20
> Hi Alexander,
>=20
> > Am Freitag, 17. Februar 2023, 07:54:01 CET schrieb Liu Ying:
> > > Hi,
> > >=20
> > > 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.
> > >=20
> > > Patch 1/6 adds device tree binding support for i.MX93 LCDIF in the
> > > existing fsl,lcdif.yaml.
> > >=20
> > > Patch 2/6 drops lcdif->bridge NULL pointer check as a cleanup patch.
> > >=20
> > > Patch 3/6~5/6 prepare for adding i.MX93 LCDIF support step by step.
> > >=20
> > > Patch 6/6 adds i.MX93 LCDIF compatible string as the last step of
> > > adding i.MX93 LCDIF support.
> >=20
> > Thanks for the series. I could test this on my TQMa93xxLA/MBa93xxCA wit=
h a
> > single LVDS display attached, so no DSI or parallel display. Hence I co=
uld
> > not test the bus format and flags checks, but they look okay.
> > So you can add
> > Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > to the whole series as well.
>=20
> Thanks for your test.
>=20
> > One thing I noticed is that, sometimes it seems that before probing lcd=
if
> > my system completely freezes. Adding some debug output it seems that's
> > during powering up the IMX93_MEDIABLK_PD_LCDIF power domain there is so=
me
> > 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:
>=20
> --------------------------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 =3D 16,
> base_baud =3D 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<------------------------------------------
>=20
> ~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=20
simple-panel as well. But this is unrelated, because lcdif_probe() is yet t=
o=20
be called. Using the debug diff from below I get the following output:

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

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

=2D--8<---
diff --git a/drivers/soc/imx/imx93-blk-ctrl.c b/drivers/soc/imx/imx93-blk-
ctrl.c
index 2c600329436cf..50aeb20ce90dc 100644
=2D-- 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=20
generic_pm_domain *genpd)
 	struct imx93_blk_ctrl *bc =3D domain->bc;
 	int ret;
=20
+	dev_info(bc->dev, "%s: 1\n", __func__);
 	ret =3D clk_bulk_prepare_enable(bc->num_clks, bc->clks);
 	if (ret) {
 		dev_err(bc->dev, "failed to enable bus clocks\n");
 		return ret;
 	}
=20
+	dev_info(bc->dev, "%s: 2\n", __func__);
 	ret =3D 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=20
generic_pm_domain *genpd)
 		return ret;
 	}
=20
+	dev_info(bc->dev, "%s: 3\n", __func__);
 	ret =3D 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=20
generic_pm_domain *genpd)
 		goto disable_clk;
 	}
=20
+	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__);
=20
 	/* release reset */
 	regmap_set_bits(bc->regmap, BLK_SFT_RSTN, data->rst_mask);
+	dev_info(bc->dev, "%s: 6\n", __func__);
=20
 	dev_dbg(bc->dev, "pd_on: name: %s\n", genpd->name);
=20

=2D--8<---

Best regards,
Alexander

> Regards,
> Liu Ying
>=20
> > Best regards,
> > Alexander
> >=20
> > > v3->v4:
> > > * Improve warning message when ignoring invalid LCDIF OF endpoint ids=
 in
> > >=20
> > >   patch 5/6. (Alexander)
> > >=20
> > > * Use 'new_{c,p}state' instead of 'new_{crtc,plane}_state' in patch 3=
/6.
> > >=20
> > >   (Alexander)
> > >=20
> > > * Simplify lcdif_crtc_reset() by calling
> > > lcdif_crtc_atomic_destroy_state()
> > >=20
> > >   in patch 3/6. (Alexander)
> > >=20
> > > * 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.
> > >=20
> > > v2->v3:
> > > * Fix a trivial typo in patch 6/6's commit message.
> > >=20
> > > 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
> > >=20
> > >   note it should always be the first member. (Lothar)
> > >=20
> > > * Drop unneeded 'bridges' member from lcdif_drm_private structure.
> > > * Drop a comment about bridge input bus format from
> > > lcdif_crtc_atomic_check().
> > >=20
> > > 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 brid=
ges
> > >   drm: lcdif: Add multiple encoders and first bridges support
> > >   drm: lcdif: Add i.MX93 LCDIF compatible string
> > > =20
> > >  .../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 ++++++++++++----=
=2D-
> > >  4 files changed, 206 insertions(+), 75 deletions(-)


=2D-=20
TQ-Systems GmbH | M=FChlstra=DFe 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht M=FCnchen, HRB 105018
Gesch=E4ftsf=FChrer: Detlef Schneider, R=FCdiger Stahl, Stefan Schneider
http://www.tq-group.com/