RE: [EXT] Re: [PATCH v10 4/7] drm: bridge: Cadence: Add MHDP8501 DP/HDMI driver

[Sandor Yu]

Hi Alexander,

Thanks your comments,

> 
> 
> Hi Sandor,
> 
> thanks for the updated series.
> 
> Am Freitag, 13. Oktober 2023, 05:24:23 CEST schrieb Sandor Yu:
> > Add a new DRM DisplayPort and HDMI bridge driver for Candence
> MHDP8501
> > used in i.MX8MQ SOC. MHDP8501 could support HDMI or DisplayPort
> > standards according embedded Firmware running in the uCPU.
> >
> > For iMX8MQ SOC, the DisplayPort/HDMI FW was loaded and activated by
> > SOC's ROM code. Bootload binary included respective specific firmware
> > is required.
> >
> > Driver will check display connector type and then load the
> > corresponding driver.
> >
> > Signed-off-by: Sandor Yu <Sandor.yu@xxxxxxx>
> > Tested-by: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
> > ---
> > v9->v10:
> >  - struct cdns_mhdp_device is renamed to cdns_mhdp8501_device.
> >  - update for mhdp helper driver is introduced.
> > Remove head file cdns-mhdp-mailbox.h and add cdns-mhdp-helper.h Add
> > struct cdns_mhdp_base base to struct cdns_mhdp8501_device.
> > Init struct cdns_mhdp_base base when driver probe.
> >
> >  drivers/gpu/drm/bridge/cadence/Kconfig        |  16 +
> >  drivers/gpu/drm/bridge/cadence/Makefile       |   2 +
> >  .../drm/bridge/cadence/cdns-mhdp8501-core.c   | 316 ++++++++
> >  .../drm/bridge/cadence/cdns-mhdp8501-core.h   | 365 +++++++++
> >  .../gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c | 708
> ++++++++++++++++++
> >  .../drm/bridge/cadence/cdns-mhdp8501-hdmi.c   | 673
> +++++++++++++++++
> >  6 files changed, 2080 insertions(+)
> >  create mode 100644
> > drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c
> >  create mode 100644
> > drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.h
> >  create mode 100644
> drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c
> >  create mode 100644
> > drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c
> >
> > [...]
> > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c
> > b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c new file mode
> > 100644 index 0000000000000..73d1c35a74599
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c
> > @@ -0,0 +1,673 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Cadence MHDP8501 HDMI bridge driver
> > + *
> > + * Copyright (C) 2019-2023 NXP Semiconductor, Inc.
> > + *
> > + */
> > +#include <drm/display/drm_hdmi_helper.h> #include
> > +<drm/display/drm_scdc_helper.h> #include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_edid.h> #include <drm/drm_print.h> #include
> > +<linux/phy/phy.h> #include <linux/phy/phy-hdmi.h>
> > +
> > +#include "cdns-mhdp8501-core.h"
> > +
> > +/**
> > + * cdns_hdmi_infoframe_set() - fill the HDMI AVI infoframe
> > + * @mhdp: phandle to mhdp device.
> > + * @entry_id: The packet memory address in which the data is written.
> > + * @packet_len: 32, only 32 bytes now.
> > + * @packet: point to InfoFrame Packet.
> > + *          packet[0] = 0
> > + *          packet[1-3] = HB[0-2]  InfoFrame Packet Header
> > + *          packet[4-31 = PB[0-27] InfoFrame Packet Contents
> > + * @packet_type: Packet Type of InfoFrame in HDMI Specification.
> > + *
> > + */
> > +static void cdns_hdmi_infoframe_set(struct cdns_mhdp8501_device
> *mhdp,
> > +                                 u8 entry_id, u8 packet_len,
> > +                                 u8 *packet, u8 packet_type) {
> > +     u32 packet32, len32;
> > +     u32 val, i;
> > +
> > +     /* only support 32 bytes now */
> > +     if (packet_len != 32)
> > +             return;
> > +
> > +     /* 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 */
> > +     len32 = packet_len / 4;
> > +     for (i = 0; i < len32; i++) {
> > +             packet32 = get_unaligned_le32(packet + 4 * i);
> > +             writel(F_DATA_WR(packet32), 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 *edid,
> > +                                 u32 block, size_t length) {
> > +     struct cdns_mhdp8501_device *mhdp = data;
> > +     u8 msg[2], reg[5], i;
> > +     int ret;
> > +
> > +     mutex_lock(&mhdp->mbox_mutex);
> > +
> > +     for (i = 0; i < 4; i++) {
> > +             msg[0] = block / 2;
> > +             msg[1] = block % 2;
> > +
> > +             ret = cdns_mhdp_mailbox_send(&mhdp->base,
> MB_MODULE_ID_HDMI_TX,
> > HDMI_TX_EDID, +
> sizeof(msg),
> msg);
> > +             if (ret)
> > +                     continue;
> > +
> > +             ret = cdns_mhdp_mailbox_recv_header(&mhdp->base,
> MB_MODULE_ID_HDMI_TX,
> > +
> HDMI_TX_EDID,
> sizeof(reg) + length);
> > +             if (ret)
> > +                     continue;
> > +
> > +             ret = cdns_mhdp_mailbox_recv_data(&mhdp->base, reg,
> sizeof(reg));
> > +             if (ret)
> > +                     continue;
> > +
> > +             ret = cdns_mhdp_mailbox_recv_data(&mhdp->base, edid,
> length);
> > +             if (ret)
> > +                     continue;
> > +
> > +             if ((reg[3] << 8 | reg[4]) == length)
> > +                     break;
> > +     }
> > +
> > +     mutex_unlock(&mhdp->mbox_mutex);
> > +
> > +     if (ret)
> > +             DRM_ERROR("get block[%d] edid failed: %d\n", block,
> ret);
> > +     return ret;
> > +}
> > +
> > +static int cdns_hdmi_scdc_write(struct cdns_mhdp8501_device *mhdp, u8
> > +addr,
> > u8 value) +{
> > +     u8 msg[5], reg[5];
> > +     int ret;
> > +
> > +     msg[0] = 0x54;
> > +     msg[1] = addr;
> > +     msg[2] = 0;
> > +     msg[3] = 1;
> > +     msg[4] = value;
> > +
> > +     mutex_lock(&mhdp->mbox_mutex);
> 
> I don't like that locking. Sometimes the mutex is locked by HDMI driver,
> sometimes within the helper.
> What is this mutex actually protecting? Concurrent access to the mailbox or a
> programming sequence which must not be interrupted aka critical section?
> When TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld,
> Germany Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> 

There are two types of command carried over the mailbox channel, with respond and w/o respond.
mutex is used to protect a full cycle command over the mailbox channel.

In helper driver, those commands could be share by different mhdp drivers.
In HDMI driver, for example the cdns_hdmi_scdc_write function is only be called by HDMI driver, 
so I keep the function in MHDP8501 HDMI driver, 
if the other mhdp want to reused it later, it should be move to helper driver.

B.R
Sandor