Hi Sandor,
Am Mittwoch, 9. November 2022, 14:26:14 CET schrieb Sandor Yu:
> Thanks for your comments.
>
>
> > -----Original Message-----
> > From: Alexander Stein <alexander.stein at ew.tq-group.com>
> > Sent: 2022年11月8日 21:17
> > To: jonas at kwiboo.se; Sandor Yu <sandor.yu at nxp.com>
> > Cc: dri-devel at lists.freedesktop.org; devicetree at vger.kernel.org;
> > linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org;
> > linux-phy at lists.infradead.org; andrzej.hajda at intel.com;
> > neil.armstrong at linaro.org; robert.foss at linaro.org;
> > Laurent.pinchart at ideasonboard.com; jernej.skrabec at gmail.com;
> > kishon at ti.com; vkoul at kernel.org; Oliver Brown <oliver.brown at nxp.com>;
> > krzysztof.kozlowski+dt at linaro.org; sam at ravnborg.org;
> > jani.nikula at intel.com;
tzimmermann at suse.de; s.hauer at pengutronix.de;
> > javierm at redhat.com;
> > penguin-kernel at i-love.sakura.ne.jp; robh+dt at kernel.org; dl-linux-imx
> > <linux-imx at nxp.com>; kernel at pengutronix.de; Sandor Yu
> > <sandor.yu at nxp.com>; shawnguo at kernel.org; p.yadav at ti.com;
> > maxime at cerno.tech
> > Subject: [EXT] Re: [v2 06/10] drm: bridge: cadence: Add MHDP HDMI driver
> > for i.MX8MQ
> >
> > Caution: EXT Email
> >
> > Hello,
> >
> > thanks for working on this and the updated version.
> >
> > Am Freitag, 4. November 2022, 07:44:56 CET schrieb Sandor Yu:
> >
> > > Add a new DRM HDMI bridge driver for Candence MHDP used in i.MX8MQ
> > > SOC. MHDP IP could support HDMI or DisplayPort standards according
> > > embedded Firmware running in the uCPU.
> > >
> > >
> > >
> > > For iMX8MQ SOC, the HDMI FW was loaded and activated by SOC ROM
> >
> > code.
> >
> > > Bootload binary included HDMI FW was required for the driver.
> > >
> > >
> > >
> > > Signed-off-by: Sandor Yu <Sandor.yu at nxp.com>
> > > ---
> > >
> > > drivers/gpu/drm/bridge/cadence/Kconfig | 12 +
> > > .../gpu/drm/bridge/cadence/cdns-hdmi-core.c | 1038
> >
> > +++++++++++++++++
> >
> > > 2 files changed, 1050 insertions(+)
> > > create mode 100644 drivers/gpu/drm/bridge/cadence/cdns-hdmi-core.c
> > >
> > >
> > >
> > > diff --git a/drivers/gpu/drm/bridge/cadence/Kconfig
> > > b/drivers/gpu/drm/bridge/cadence/Kconfig index
> > > e79ae1af3765..377452d09992
> > > 100644
> > > --- a/drivers/gpu/drm/bridge/cadence/Kconfig
> > > +++ b/drivers/gpu/drm/bridge/cadence/Kconfig
[snip]
> > > +static int cdns_hdmi_get_edid_block(void *data, u8 *edid,
> > > + u32 block, size_t length) {
> > > + struct cdns_mhdp_device *mhdp = data;
> > > + u8 msg[2], reg[5], i;
> > > + int ret;
> > > +
> > > + mutex_lock(&mhdp->mbox_mutex);
> > > +
> > > + for (i = 0; i < 4; i++) {
> >
> >
> > What is i? It is not used inside the loop.
>
> EDID data read by HDMI firmware are not guarantee 100% successful.
> Base on experiments, try 4 times if EDID read failed.
Mh, 4 times sounds a bit too arbitrary to me. How about using a timeout in ms,
like 50ms, for retrying to read the EDID?
[snip]
> > > +static int cdns_mhdp_imx_probe(struct platform_device *pdev) {
> > > + struct device *dev = &pdev->dev;
> > > + struct cdns_mhdp_device *mhdp;
> > > + struct platform_device_info pdevinfo;
> > > + struct resource *res;
> > > + u32 reg;
> > > + int ret;
> > > +
> > > + mhdp = devm_kzalloc(dev, sizeof(*mhdp), GFP_KERNEL);
> > > + if (!mhdp)
> > > + return -ENOMEM;
> > > +
> > > + mutex_init(&mhdp->lock);
> > > + mutex_init(&mhdp->mbox_mutex);
> > > +
> > > + INIT_DELAYED_WORK(&mhdp->hotplug_work, hotplug_work_func);
> > > +
> > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > + if (!res)
> > > + return -ENODEV;
> > > + mhdp->regs = devm_ioremap(dev, res->start, resource_size(res));
> > > + if (IS_ERR(mhdp->regs))
> > > + return PTR_ERR(mhdp->regs);
> >
> >
> > Please use devm_platform_get_and_ioremap_resource.
>
> Both HDMI PHY driver and mhdp HDMI driver should access same APB base
> register offset for mailbox.
devm_ioremap_resource could not support such
> feature.
Oh I see, both remap the same range. To be honest I do not like this. Is there
a need to map overlapping ranges in both drivers? How can you avoid race
conditions due to simultaneous accesses?
> > > + mhdp->phy = devm_of_phy_get_by_index(dev, pdev->dev.of_node,
> >
> > 0);
> >
> > > + if (IS_ERR(mhdp->phy)) {
> > > + dev_err(dev, "no PHY configured\n");
> > > + return PTR_ERR(mhdp->phy);
> > > + }
> >
> >
> > Please use dev_err_probe().
>
> OK.
>
> >
> >
> > > + mhdp->irq[IRQ_IN] = platform_get_irq_byname(pdev, "plug_in");
> > > + if (mhdp->irq[IRQ_IN] < 0) {
> > > + dev_info(dev, "No plug_in irq number\n");
> > > + return -EPROBE_DEFER;
> > > + }
> >
> >
> > Please use dev_err_probe().
>
> OK.
>
> >
> >
> > > + mhdp->irq[IRQ_OUT] = platform_get_irq_byname(pdev,
> >
> > "plug_out");
> >
> > > + if (mhdp->irq[IRQ_OUT] < 0) {
> > > + dev_info(dev, "No plug_out irq number\n");
> > > + return -EPROBE_DEFER;
> > > + }
> >
> >
> > Please use dev_err_probe().
>
> OK.
>
> >
> >
> > > + /*
> > > + * Wait for the KEEP_ALIVE "message" on the first 8 bits.
> > > + * Updated each sched "tick" (~2ms)
> > > + */
> > > + ret = readl_poll_timeout(mhdp->regs + KEEP_ALIVE, reg,
> > > + reg & CDNS_KEEP_ALIVE_MASK,
> >
> > 500,
> >
> > > + CDNS_KEEP_ALIVE_TIMEOUT);
> >
> >
> > This freezes my board TQMa8MQ
> > (arch/arm64/boot/dts/freescale/imx8mq-tqma8mq-
> > mba8mx.dts) completly if this and the PHY driver are compiled in. I have
> > "pd_ignore_unused clk_ignore_unused" passed to kernel command line, so I
> > have no idea what's wrong here.
>
> Here is the first time in the driver to access hdmi register when driver
> probe.
For imx8mq hdmi/dp, mdhp hdmi apb clock and core clock are managed
> by ROM code, they are always on when device bootup. Could you dump the
> clock tree without "pd_ignore_unused clk_ignore_unused" ?
I noticed too this is the 1st access, so I have no idea what's wrong here.
Here is my /sys/kernel/debug/clk/clk_summary from using the regular DT without
enabling 'dcss', 'hdmi_phy' and 'mhdp_hdmi':
enable prepare protect
duty hardware
clock count count count rate
accuracy phase cycle enable
-------------------------------------------------------------------------------------------------------
sys2_pll_out 7 7 0 1000000000
0 0 50000 Y
sys_pll2_out_monitor 0 0 0 1000000000
0 0 50000 Y
sys2_pll_1000m 0 0 0 1000000000
0 0 50000 Y
sys2_pll_500m 1 2 0 500000000
0 0 50000 Y
audio_ahb 0 1 0 500000000
0 0 50000 N
ipg_audio_root 0 1 0 250000000
0 0 50000 Y
sdma2_clk 0 2 0 250000000
0 0 50000 N
sai6_ipg_clk 0 0 0 250000000
0 0 50000 N
sai5_ipg_clk 0 0 0 250000000
0 0 50000 N
sai4_ipg_clk 0 0 0 250000000
0 0 50000 N
sai1_ipg_clk 0 0 0 250000000
0 0 50000 N
nand 0 0 0 500000000
0 0 50000 N
nand_root_clk 0 0 0 500000000
0 0 50000 N
usb_bus 2 2 0 500000000
0 0 50000 Y
usb2_ctrl_root_clk 1 1 0 500000000
0 0 50000 Y
usb1_ctrl_root_clk 1 1 0 500000000
0 0 50000 Y
sys2_pll_333m 1 1 0 333333333
0 0 50000 Y
main_axi 1 1 0 333333333
0 0 50000 Y
sys2_pll_250m 2 2 0 250000000
0 0 50000 Y
pcie1_ctrl 1 1 0 250000000
0 0 50000 Y
pcie1_root_clk 1 1 0 250000000
0 0 50000 Y
pcie2_ctrl 1 1 0 250000000
0 0 50000 Y
pcie2_root_clk 1 1 0 250000000
0 0 50000 Y
sys2_pll_200m 3 3 0 200000000
0 0 50000 Y
ecspi3 0 0 0 200000000
0 0 50000 N
ecspi3_root_clk 0 0 0 200000000
0 0 50000 N
ecspi2 1 1 0 200000000
0 0 50000 Y
ecspi2_root_clk 2 2 0 200000000
0 0 50000 Y
ecspi1 1 1 0 200000000
0 0 50000 Y
ecspi1_root_clk 2 2 0 200000000
0 0 50000 Y
gic 1 1 0 200000000
0 0 50000 Y
arm_m4_core 0 0 0 200000000
0 0 50000 N
sys2_pll_166m 0 0 0 166666666
0 0 50000 Y
sys2_pll_125m 1 1 0 125000000
0 0 50000 Y
enet_ref 1 1 0 125000000
0 0 50000 Y
sys2_pll_100m 3 3 0 100000000
0 0 50000 Y
pcie1_phy 1 1 0 100000000
0 0 50000 Y
pcie2_phy 1 1 0 100000000
0 0 50000 Y
enet_timer 1 1 0 100000000
0 0 50000 Y
sys2_pll_50m 1 1 0 50000000
0 0 50000 Y
enet_phy 1 1 0 50000000
0 0 50000 Y
sys1_pll_out 5 5 0 800000000
0 0 50000 Y
sys_pll1_out_monitor 0 0 0 800000000
0 0 50000 Y
sys1_pll_800m 2 2 0 800000000
0 0 50000 Y
vpu_bus 0 0 0 800000000
0 0 50000 N
vpu_dec_root_clk 0 0 0 800000000
0 0 50000 N
arm_a53_div 0 0 0 800000000
0 0 50000 N
dram_apb 1 1 0 160000000
0 0 50000 Y
noc 1 1 0 800000000
0 0 50000 Y
disp_rtrm 0 0 0 400000000
0 0 50000 N
disp_rtrm_root_clk 0 0 0 400000000
0 0 50000 N
disp_apb 0 0 0 133333334
0 0 50000 N
disp_apb_root_clk 0 0 0 133333334
0 0 50000 N
disp_axi 0 0 0 800000000
0 0 50000 N
disp_axi_root_clk 0 0 0 800000000
0 0 50000 N
sys1_pll_400m 0 0 0 400000000
0 0 50000 Y
usdhc2 0 0 0 400000000
0 0 50000 N
usdhc2_root_clk 0 0 0 400000000
0 0 50000 N
usdhc1 0 0 0 400000000
0 0 50000 N
usdhc1_root_clk 0 0 0 400000000
0 0 50000 N
sys1_pll_266m 1 1 0 266666666
0 0 50000 Y
nand_usdhc_bus 0 0 0 266666666
0 0 50000 N
nand_usdhc_rawnand_clk 0 0 0 266666666
0 0 50000 N
enet_axi 1 1 0 266666666
0 0 50000 Y
enet1_root_clk 2 2 0 266666666
0 0 50000 Y
sys1_pll_200m 0 0 0 200000000
0 0 50000 Y
sys1_pll_160m 0 0 0 160000000
0 0 50000 Y
sys1_pll_133m 1 1 0 133333333
0 0 50000 Y
ahb 9 4 0 133333333
0 0 50000 Y
ipg_root 8 8 0 66666667
0 0 50000 Y
sdma1_clk 6 1 0 66666667
0 0 50000 Y
tmu_root_clk 1 1 0 66666667
0 0 50000 Y
sai3_ipg_clk 0 0 0 66666667
0 0 50000 N
sai2_ipg_clk 0 0 0 66666667
0 0 50000 N
ocotp_root_clk 0 0 0 66666667
0 0 50000 N
mu_root_clk 0 0 0 66666667
0 0 50000 N
gpio5_root_clk 1 1 0 66666667
0 0 50000 Y
gpio4_root_clk 1 1 0 66666667
0 0 50000 Y
gpio3_root_clk 1 1 0 66666667
0 0 50000 Y
gpio2_root_clk 1 1 0 66666667
0 0 50000 Y
gpio1_root_clk 1 1 0 66666667
0 0 50000 Y
sys1_pll_100m 2 2 0 100000000
0 0 50000 Y
usb_phy_ref 2 2 0 100000000
0 0 50000 Y
usb2_phy_root_clk 1 1 0 100000000
0 0 50000 Y
usb1_phy_root_clk 1 1 0 100000000
0 0 50000 Y
usb_core_ref 2 2 0 100000000
0 0 50000 Y
qspi 0 0 0 100000000
0 0 50000 N
qspi_root_clk 0 0 0 100000000
0 0 50000 N
dram_alt 0 0 0 100000000
0 0 50000 N
dram_alt_root 0 0 0 25000000
0 0 50000 Y
sys1_pll_80m 2 2 0 80000000
0 0 50000 Y
pcie1_aux 1 1 0 10000000
0 0 50000 Y
pcie2_aux 1 1 0 10000000
0 0 50000 Y
uart2 0 0 0 80000000
0 0 50000 N
uart2_root_clk 0 0 0 80000000
0 0 50000 N
uart1 0 0 0 80000000
0 0 50000 N
uart1_root_clk 0 0 0 80000000
0 0 50000 N
sys1_pll_40m 0 0 0 40000000
0 0 50000 Y
wrclk 0 0 0 40000000
0 0 50000 N
dummy 0 0 0 0
0 0 50000 Y
clk-xtal25 2 2 0 25000000
0 0 50000 Y
DIF3 0 0 0 100000000
0 0 50000 Y
DIF2 1 1 0 100000000
0 0 50000 Y
DIF1 0 0 0 100000000
0 0 50000 Y
DIF0 1 1 0 100000000
0 0 50000 Y
clock 0 0 0 32768
0 0 50000 Y
clk_ext4 0 0 0 133000000
0 0 50000 Y
clk_ext3 0 0 0 133000000
0 0 50000 Y
clk_ext2 0 0 0 133000000
0 0 50000 Y
clk_ext1 0 0 0 133000000
0 0 50000 Y
hdmi_phy_27m 0 0 0 27000000
0 0 50000 Y
osc_27m 0 0 0 27000000
0 0 50000 Y
osc_25m 7 11 0 25000000
0 0 50000 Y
gpt_3m 0 0 0 3125000
0 0 50000 Y
csi2_esc 0 0 0 25000000
0 0 50000 N
csi2_phy_ref 0 0 0 25000000
0 0 50000 N
csi2_core 0 0 0 25000000
0 0 50000 N
csi2_root_clk 0 0 0 25000000
0 0 50000 N
csi1_esc 0 0 0 25000000
0 0 50000 N
csi1_phy_ref 0 0 0 25000000
0 0 50000 N
csi1_core 0 0 0 25000000
0 0 50000 N
csi1_root_clk 0 0 0 25000000
0 0 50000 N
dsi_ahb 0 0 0 25000000
0 0 50000 N
dsi_ipg_div 0 0 0 12500000
0 0 50000 Y
dsi_esc 0 0 0 25000000
0 0 50000 N
dsi_dbi 0 0 0 25000000
0 0 50000 N
dsi_phy_ref 0 0 0 25000000
0 0 50000 N
dsi_core 0 0 0 25000000
0 0 50000 N
clko2 0 0 0 25000000
0 0 50000 N
clko1 0 0 0 25000000
0 0 50000 N
wdog 1 1 0 25000000
0 0 50000 Y
wdog3_root_clk 0 0 0 25000000
0 0 50000 N
wdog2_root_clk 0 0 0 25000000
0 0 50000 N
wdog1_root_clk 1 1 0 25000000
0 0 50000 Y
gpt1 0 0 0 25000000
0 0 50000 N
gpt1_root_clk 0 0 0 25000000
0 0 50000 N
pwm4 0 0 0 25000000
0 0 50000 N
pwm4_root_clk 0 0 0 25000000
0 0 50000 N
pwm3 0 0 0 25000000
0 0 50000 N
pwm3_root_clk 0 0 0 25000000
0 0 50000 N
pwm2 0 0 0 25000000
0 0 50000 N
pwm2_root_clk 0 0 0 25000000
0 0 50000 N
pwm1 0 0 0 25000000
0 0 50000 N
pwm1_root_clk 0 0 0 25000000
0 0 50000 N
uart4 0 0 0 25000000
0 0 50000 N
uart4_root_clk 0 0 0 25000000
0 0 50000 N
uart3 1 1 0 25000000
0 0 50000 Y
uart3_root_clk 4 4 0 25000000
0 0 50000 Y
i2c4 0 0 0 25000000
0 0 50000 N
i2c4_root_clk 0 0 0 25000000
0 0 50000 N
i2c3 0 1 0 25000000
0 0 50000 N
i2c3_root_clk 0 1 0 25000000
0 0 50000 N
i2c2 0 1 0 25000000
0 0 50000 N
i2c2_root_clk 0 1 0 25000000
0 0 50000 N
i2c1 0 1 0 25000000
0 0 50000 N
i2c1_root_clk 0 1 0 25000000
0 0 50000 N
spdif2 0 0 0 25000000
0 0 50000 N
spdif1 0 0 0 25000000
0 0 50000 N
sai6 0 0 0 25000000
0 0 50000 N
sai6_root_clk 0 0 0 25000000
0 0 50000 N
sai5 0 0 0 25000000
0 0 50000 N
sai5_root_clk 0 0 0 25000000
0 0 50000 N
sai4 0 0 0 25000000
0 0 50000 N
sai4_root_clk 0 0 0 25000000
0 0 50000 N
sai2 0 0 0 25000000
0 0 50000 N
sai2_root_clk 0 0 0 25000000
0 0 50000 N
sai1 0 0 0 25000000
0 0 50000 N
sai1_root_clk 0 0 0 25000000
0 0 50000 N
lcdif_pixel 0 0 0 25000000
0 0 50000 N
disp_dc8000 0 0 0 25000000
0 0 50000 N
disp_root_clk 0 0 0 25000000
0 0 50000 N
disp_dtrc 0 0 0 25000000
0 0 50000 N
noc_apb 1 1 0 25000000
0 0 50000 Y
video2_pll1_ref_sel 0 0 0 25000000
0 0 50000 Y
video2_pll_out 0 0 0 25000000
0 0 50000 Y
video_pll2_out_monitor 0 0 0 25000000
0 0 50000 Y
dram_pll1_ref_sel 1 1 0 25000000
0 0 50000 Y
dram_pll_out 2 2 0 800000000
0 0 50000 Y
dram_core_clk 1 1 0 800000000
0 0 50000 Y
dram_pll_out_monitor 0 0 0 800000000
0 0 50000 Y
sys3_pll1_ref_sel 1 1 0 25000000
0 0 50000 Y
sys3_pll_out 1 1 0 25000000
0 0 50000 Y
sys_pll3_out_monitor 0 0 0 25000000
0 0 50000 Y
video_pll1_ref_sel 0 0 0 25000000
0 0 50000 Y
video_pll1_bypass 0 0 0 25000000
0 0 50000 Y
video_pll1_out_monitor 0 0 0 25000000
0 0 50000 Y
video_pll1_out 0 0 0 25000000
0 0 50000 N
dc_pixel 0 0 0 5000000
0 0 50000 N
video_pll1_ref_div 0 0 0 5000000
0 0 50000 Y
video_pll1 0 0 0 650000000
0 0 50000 Y
audio_pll2_ref_sel 0 0 0 25000000
0 0 50000 Y
audio_pll2_ref_div 0 0 0 5000000
0 0 50000 Y
audio_pll2 0 0 0 722534397
0 0 50000 Y
audio_pll2_bypass 0 0 0 722534397
0 0 50000 Y
audio_pll2_out_monitor 0 0 0 722534397
0 0 50000 Y
audio_pll2_out 0 0 0 722534397
0 0 50000 N
audio_pll1_ref_sel 0 0 0 25000000
0 0 50000 Y
audio_pll1_ref_div 0 0 0 5000000
0 0 50000 Y
audio_pll1 0 0 0 786431998
0 0 50000 Y
audio_pll1_bypass 0 0 0 786431998
0 0 50000 Y
audio_pll1_out_monitor 0 0 0 786431998
0 0 50000 Y
audio_pll1_out 0 0 0 786431998
0 0 50000 N
sai3 0 0 0 49152000
0 0 50000 N
sai3_root_clk 0 0 0 49152000
0 0 50000 N
pll 0 0 0 196608000
0 0 50000 Y
codec_clkin 0 0 0 196608000
0 0 50000 Y
nadc 0 0 0 196608000
0 0 50000 Y
madc 0 0 0 196608000
0 0 50000 Y
ndac 0 0 0 196608000
0 0 50000 Y
mdac 0 0 0 196608000
0 0 50000 Y
bdiv 0 0 0
196608000 0 0 50000 Y
vpu_pll_ref_sel 0 1 0 25000000
0 0 50000 Y
vpu_pll_ref_div 0 1 0 5000000
0 0 50000 Y
vpu_pll 0 1 0 600000000
0 0 50000 Y
vpu_pll_bypass 0 1 0 600000000
0 0 50000 Y
vpu_pll_out_monitor 0 0 0 600000000
0 0 50000 Y
vpu_pll_out 0 2 0 600000000
0 0 50000 N
vpu_g2 0 1 0 600000000
0 0 50000 N
vpu_g2_root_clk 0 1 0 600000000
0 0 50000 N
vpu_g1 0 1 0 600000000
0 0 50000 N
vpu_g1_root_clk 0 1 0 600000000
0 0 50000 N
gpu_pll_ref_sel 0 0 0 25000000
0 0 50000 Y
gpu_pll_ref_div 0 0 0 5000000
0 0 50000 Y
gpu_pll 0 0 0 800000000
0 0 50000 Y
gpu_pll_bypass 0 0 0 800000000
0 0 50000 Y
gpu_pll_out_monitor 0 0 0 800000000
0 0 50000 Y
pllout_monitor_sel 0 0 0 800000000
0 0 50000 Y
pllout_monitor_clk2 0 0 0
800000000 0 0 50000 N
gpu_pll_out 0 0 0 800000000
0 0 50000 N
gpu_ahb 0 0 0 800000000
0 0 50000 N
gpu_axi 0 0 0 800000000
0 0 50000 N
gpu_shader 0 0 0 800000000
0 0 50000 N
gpu_core 0 0 0 800000000
0 0 50000 N
gpu_root_clk 0 0 0 800000000
0 0 50000 N
arm_pll_ref_sel 1 1 0 25000000
0 0 50000 Y
arm_pll_ref_div 1 1 0 5000000
0 0 50000 Y
arm_pll 1 1 0 800000000
0 0 50000 Y
arm_pll_bypass 1 1 0 800000000
0 0 50000 Y
arm_pll_out_monitor 0 0 0 800000000
0 0 50000 Y
arm_pll_out 1 1 0 800000000
0 0 50000 Y
arm_a53_core 1 1 0 800000000
0 0 50000 Y
arm 1 1 0 800000000
0 0 50000 Y
vpu_core 0 0 0 800000000
0 0 50000 N
ckil 2 2 0 32768
0 0 50000 Y
Thanks and best regards
Alexander