| Message ID | 20230217065407.2259731-1-victor.liu@nxp.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm: lcdif: Add i.MX93 LCDIF support | expand |
Hi Liu, 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@ew.tq-group.com> to the whole series as well. 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. 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(-)
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@ew.tq-group.com> > 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. 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(-) > >
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@ew.tq-group.com> > > 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(-)
Hi Liu, Am Montag, 20. Februar 2023, 09:55:19 CET schrieb Alexander Stein: > 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@ew.tq-group.com> > > > 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. I think I found the cause. It's the maximum clock frequency for media_axi and media_apb. These clocks were not explicitly configured, most probably exceeding the maximum frequency allowed. Best regards, Alexander > ---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(-)
On 17/02/2023 09.18, Alexander Stein wrote: > Hi Liu, > > 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@ew.tq-group.com> > to the whole series as well. > > 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. Interesting. Sounds similar to what I encountered on several imx8mp-based boards, both the NXP EVK and our custom design, running a mainline U-Boot and downstream NXP kernel: https://lore.kernel.org/u-boot/20220823133645.4046432-1-rasmus.villemoes@prevas.dk/ I never really found a real solution, but as the hack I ended up applying in U-Boot does involve some clock settings, and you apparently now figured out some connection to "overclocking", I do think these issues are related. Rasmus
On Mon, 2023-02-20 at 11:16 +0100, Alexander Stein wrote: > Hi Liu, Hi Alexander, > > Am Montag, 20. Februar 2023, 09:55:19 CET schrieb Alexander Stein: > > 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@ew.tq-group.com> > > > > 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. > > I think I found the cause. It's the maximum clock frequency for > media_axi and > media_apb. These clocks were not explicitly configured, most > probably > exceeding the maximum frequency allowed. Thanks for sharing the cause. I use 400MHz media_axi and 133MHz media_apb. Regards, Liu Ying > > Best regards, > Alexander > > > ---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(-) > >
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. 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(-)