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@xxxxxxxxxxxxxxx> > > Sent: 2022年11月8日 21:17 > > To: jonas@xxxxxxxxx; Sandor Yu <sandor.yu@xxxxxxx> > > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; > > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > > linux-phy@xxxxxxxxxxxxxxxxxxx; andrzej.hajda@xxxxxxxxx; > > neil.armstrong@xxxxxxxxxx; robert.foss@xxxxxxxxxx; > > Laurent.pinchart@xxxxxxxxxxxxxxxx; jernej.skrabec@xxxxxxxxx; > > kishon@xxxxxx; vkoul@xxxxxxxxxx; Oliver Brown <oliver.brown@xxxxxxx>; > > krzysztof.kozlowski+dt@xxxxxxxxxx; sam@xxxxxxxxxxxx; > > jani.nikula@xxxxxxxxx; tzimmermann@xxxxxxx; s.hauer@xxxxxxxxxxxxxx; > > javierm@xxxxxxxxxx; > > penguin-kernel@xxxxxxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; dl-linux-imx > > <linux-imx@xxxxxxx>; kernel@xxxxxxxxxxxxxx; Sandor Yu > > <sandor.yu@xxxxxxx>; shawnguo@xxxxxxxxxx; p.yadav@xxxxxx; > > maxime@xxxxxxxxxx > > 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@xxxxxxx> > > > --- > > > > > > 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