RE: [EXT] Re: [v2 06/10] drm: bridge: cadence: Add MHDP HDMI driver for i.MX8MQ
Hi Alexander,
> -Original Message-
> From: Alexander Stein
> Sent: 2022年11月10日 23:44
> To: Sandor Yu
> Cc: jo...@kwiboo.se; dri-devel@lists.freedesktop.org;
> devicet...@vger.kernel.org; linux-arm-ker...@lists.infradead.org;
> linux-ker...@vger.kernel.org; linux-...@lists.infradead.org;
> andrzej.ha...@intel.com; neil.armstr...@linaro.org; robert.f...@linaro.org;
> laurent.pinch...@ideasonboard.com; jernej.skra...@gmail.com;
> kis...@ti.com; vk...@kernel.org; Oliver Brown ;
> krzysztof.kozlowski...@linaro.org; s...@ravnborg.org;
> tzimmerm...@suse.de; s.ha...@pengutronix.de; javierm@redhat. com
> ; penguin-ker...@i-love.sakura.ne.jp;
> robh...@kernel.org; dl-linux-imx ;
> ker...@pengutronix.de; shawn...@kernel.org; p.ya...@ti.com;
> max...@cerno.tech
> Subject: RE: [EXT] Re: [v2 06/10] drm: bridge: cadence: Add MHDP HDMI
> driver for i.MX8MQ
>
> Caution: EXT Email
>
> Hi Sandor,
>
> Am Mittwoch, 9. November 2022, 14:26:14 CET schrieb Sandor Yu:
> > Thanks for your comments.
> >
> >
> > > -Original Message-
> > > From: Alexander Stein
> > > Sent: 2022年11月8日 21:17
> > > To: jo...@kwiboo.se; Sandor Yu
> > > Cc: dri-devel@lists.freedesktop.org; devicet...@vger.kernel.org;
> > > linux-arm-ker...@lists.infradead.org; linux-ker...@vger.kernel.org;
> > > linux-...@lists.infradead.org; andrzej.ha...@intel.com;
> > > neil.armstr...@linaro.org; robert.f...@linaro.org;
> > > laurent.pinch...@ideasonboard.com; jernej.skra...@gmail.com;
> > > kis...@ti.com; vk...@kernel.org; Oliver Brown
> > > ; krzysztof.kozlowski...@linaro.org;
> > > s...@ravnborg.org; jani.nik...@intel.com;
> tzimmerm...@suse.de; s.ha...@pengutronix.de;
> > > javi...@redhat.com;
> > > penguin-ker...@i-love.sakura.ne.jp; robh...@kernel.org; dl-linux-imx
> > > ; ker...@pengutronix.de; Sandor Yu
> > > ; shawn...@kernel.org; p.ya...@ti.com;
> > > max...@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
> > > > ---
> > > >
> > > > 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?
>
For EDID read failed case, the mailbox read will timeout, additional timeout in
the loop is not necessary.
> [snip]
>
> > > > +static int cdns_
RE: [EXT] Re: [v2 06/10] drm: bridge: cadence: Add MHDP HDMI driver for i.MX8MQ
Hi Sandor,
Am Mittwoch, 9. November 2022, 14:26:14 CET schrieb Sandor Yu:
> Thanks for your comments.
>
>
> > -Original Message-
> > From: Alexander Stein
> > Sent: 2022年11月8日 21:17
> > To: jo...@kwiboo.se; Sandor Yu
> > Cc: dri-devel@lists.freedesktop.org; devicet...@vger.kernel.org;
> > linux-arm-ker...@lists.infradead.org; linux-ker...@vger.kernel.org;
> > linux-...@lists.infradead.org; andrzej.ha...@intel.com;
> > neil.armstr...@linaro.org; robert.f...@linaro.org;
> > laurent.pinch...@ideasonboard.com; jernej.skra...@gmail.com;
> > kis...@ti.com; vk...@kernel.org; Oliver Brown ;
> > krzysztof.kozlowski...@linaro.org; s...@ravnborg.org;
> > jani.nik...@intel.com;
tzimmerm...@suse.de; s.ha...@pengutronix.de;
> > javi...@redhat.com;
> > penguin-ker...@i-love.sakura.ne.jp; robh...@kernel.org; dl-linux-imx
> > ; ker...@pengutronix.de; Sandor Yu
> > ; shawn...@kernel.org; p.ya...@ti.com;
> > max...@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
> > > ---
> > >
> > > 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;
> > > + }
RE: [EXT] Re: [v2 06/10] drm: bridge: cadence: Add MHDP HDMI driver for i.MX8MQ
Thanks for your comments.
> -Original Message-
> From: Alexander Stein
> Sent: 2022年11月8日 21:17
> To: jo...@kwiboo.se; Sandor Yu
> Cc: dri-devel@lists.freedesktop.org; devicet...@vger.kernel.org;
> linux-arm-ker...@lists.infradead.org; linux-ker...@vger.kernel.org;
> linux-...@lists.infradead.org; andrzej.ha...@intel.com;
> neil.armstr...@linaro.org; robert.f...@linaro.org;
> laurent.pinch...@ideasonboard.com; jernej.skra...@gmail.com;
> kis...@ti.com; vk...@kernel.org; Oliver Brown ;
> krzysztof.kozlowski...@linaro.org; s...@ravnborg.org; jani.nik...@intel.com;
> tzimmerm...@suse.de; s.ha...@pengutronix.de; javi...@redhat.com;
> penguin-ker...@i-love.sakura.ne.jp; robh...@kernel.org; dl-linux-imx
> ; ker...@pengutronix.de; Sandor Yu
> ; shawn...@kernel.org; p.ya...@ti.com;
> max...@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
> > ---
> > 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
> > @@ -26,6 +26,18 @@ config DRM_CDNS_MHDP8546_J721E
> > clock and data muxes.
> > endif
> >
> > +config DRM_CDNS_HDMI
> > + tristate "Cadence HDMI DRM driver"
> > + select DRM_KMS_HELPER
> > + select DRM_PANEL_BRIDGE
> > + select DRM_DISPLAY_HELPER
> > + select DRM_CDNS_AUDIO
> > + depends on OF
> > + help
> > + Support Cadence MHDP HDMI driver.
> > + Cadence MHDP Controller support one or more protocols,
> > + HDMI firmware is required for this driver.
> > +
> > config DRM_CDNS_DP
> > tristate "Cadence DP DRM driver"
> > select DRM_KMS_HELPER
> > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-hdmi-core.c
> > b/drivers/gpu/drm/bridge/cadence/cdns-hdmi-core.c new file mode
> 100644
> > index ..b392aac015bd
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/cadence/cdns-hdmi-core.c
> > @@ -0,0 +1,1038 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Cadence High-Definition Multimedia Interface (HDMI) driver
> > + *
> > + * Copyright (C) 2019-2022 NXP Semiconductor, Inc.
> > + *
> > + */
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include #include
> > + #include
> > + #include
> > +#include #include
> #include
> > + #include #include
> > + #include
> > +
> > +#include "cdns-mhdp-common.h"
> > +
> > +void cdns_mhdp_infoframe_set(struct cdns_mhdp_device *mhdp,
> > + u8 entry_id, u8 packet_len,
> u8 *packet, u8 packet_type)
> > +{
> > + u32 *packet32, len32;
> > + u32 val, i;
> > +
> > + /* invalidate entry */
> > + val = F_ACTIVE_IDLE_TYPE(1) | F_PKT_ALLOC_ADDRESS(entry_id);
> > + writel(val, mhdp->regs + SOURCE_PIF_PKT_ALLOC_REG);
> > + writel(F_PKT_ALLOC_WR_EN(1), mhdp->regs +
> SOURCE_PIF_PKT_ALLOC_WR_EN);
> > +
> > + /* flush fifo 1 */
> > + writel(F_FIFO1_FLUSH(1), mhdp->regs +
> SOURCE_PIF_FIFO1_FLUSH);
> > +
> > + /* write packet into memory */
> > + packet32 = (u32 *)packet;
>
> This only works on little-endian machines, no?
Register SOURCE_PIF_DATA_WR require little-endian data,
I will use get_unaligned_le32() to replace it in the next version.
>
> > + len32 = packet_len / 4;
> > + for (i = 0; i < len32; i++)
> > + writel(F_DATA_WR(packet32[i]), mhdp->regs +
> SOURCE_PIF_DATA_WR);
> > +
> > + /* write entry id */
> > + writel(F_WR_ADDR(entry_id), mhdp->regs +
> SOURCE_PIF_WR_ADDR);
> > +
> > + /* write request */
> > + writel(F_HOST_WR(1), mhdp->regs + SOURCE_PIF_WR_REQ);
> > +
> > + /* update entry */
> > + val = F_ACTIVE_IDLE_TYPE(1) | F_TYPE_VALID(1) |
> > + F_PACKET_TYPE(packet_type) |
> F_PKT_ALLOC_ADDRESS(entry_id);
> > + writel(val, mhdp->regs + SOURCE_PIF_PKT_ALLOC_REG);
> > +
> > + writel(F_PKT_ALLOC_WR_EN(1), mhdp->regs +
> SOURCE_PIF_PKT_ALLOC_WR_EN);
> > +}
> > +
> > +static int cdns_hdmi_get_edid_block(void *data, u8 *e
