Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
From:   Sandor Yu <sandor.yu@nxp.com>
To:     Alexander Stein <alexander.stein@ew.tq-group.com>,
        "dmitry.baryshkov@linaro.org" <dmitry.baryshkov@linaro.org>,
        "andrzej.hajda@intel.com" <andrzej.hajda@intel.com>,
        "neil.armstrong@linaro.org" <neil.armstrong@linaro.org>,
        "Laurent.pinchart@ideasonboard.com" 
        <Laurent.pinchart@ideasonboard.com>,
        "jonas@kwiboo.se" <jonas@kwiboo.se>,
        "jernej.skrabec@gmail.com" <jernej.skrabec@gmail.com>,
        "airlied@gmail.com" <airlied@gmail.com>,
        "daniel@ffwll.ch" <daniel@ffwll.ch>,
        "robh+dt@kernel.org" <robh+dt@kernel.org>,
        "krzysztof.kozlowski+dt@linaro.org" 
        <krzysztof.kozlowski+dt@linaro.org>,
        "shawnguo@kernel.org" <shawnguo@kernel.org>,
        "s.hauer@pengutronix.de" <s.hauer@pengutronix.de>,
        "festevam@gmail.com" <festevam@gmail.com>,
        "vkoul@kernel.org" <vkoul@kernel.org>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-phy@lists.infradead.org" <linux-phy@lists.infradead.org>
CC:     "kernel@pengutronix.de" <kernel@pengutronix.de>,
        dl-linux-imx <linux-imx@nxp.com>,
        Oliver Brown <oliver.brown@nxp.com>,
        "sam@ravnborg.org" <sam@ravnborg.org>
Subject: RE: [EXT] Re: [PATCH v10 4/7] drm: bridge: Cadence: Add MHDP8501
 DP/HDMI driver
Thread-Topic: [EXT] Re: [PATCH v10 4/7] drm: bridge: Cadence: Add MHDP8501
 DP/HDMI driver
Thread-Index: AQHZ/YCkvljU4iEKsk25DqYpOSNnO7BHUqEAgARjNaA=
Date:   Mon, 16 Oct 2023 03:05:57 +0000
Message-ID: <PAXPR04MB9448C5DD9A5D3CBFBB73735CF4D7A@PAXPR04MB9448.eurprd04.prod.outlook.com>
References: <cover.1697162990.git.Sandor.yu@nxp.com>
 <037c61d582df78a3309a5672ac66b9e74b396ddd.1697162990.git.Sandor.yu@nxp.com>
 <5605026.mvXUDI8C0e@steina-w>
In-Reply-To: <5605026.mvXUDI8C0e@steina-w>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?iso-8859-1?Q?ag7BTWaQ7+1aSXVf0Jwn8VhryNJEz4hxQDl7EjnD6NjybPVJRiXCn5m3vi?=
 =?iso-8859-1?Q?LkO+M/3xf/OdsRVhLQFWxBSQFLZ4ZeZGLWHEppSN3nABMc8XsTAtuSs+0K?=
 =?iso-8859-1?Q?qWxJt0g5xtUegmBcT3uqELYNFENgTDWlZP23zef3rEUydcW9qABsvQCNuM?=
 =?iso-8859-1?Q?DLYQYwIZvXHoiCWzVnDLkyMEErjB70fvyXU4yeco/efrriozMe+mgQHhfn?=
 =?iso-8859-1?Q?32tHcSq+5gdKFuh1cpd5qOkmbB2YUMKqC0PwBa51GyIXtTLAUyVMHMKH/d?=
 =?iso-8859-1?Q?RqWq7tHCLSkETxixlbWMBmeVsjUfyTKUiv0xusmzYRmMvTn1V+9MP8hBNO?=
 =?iso-8859-1?Q?2e/byIBygFl5alt/LoNaUZcq2V88jdjyBVZun5eBk88Tj409BOQyhUiM+B?=
 =?iso-8859-1?Q?ztENm9VgL/YWabQaUVL8wxW7K89ZHi3AZreBapNyKTF7Rl9c+0WRNVDvzY?=
 =?iso-8859-1?Q?DrNLbZNoZ74gf2dnTx/4q1AB8hG4PD/Cxhpea9azgsUW7Qj1z9TM74ptps?=
 =?iso-8859-1?Q?7z/vZgcebT7F0IBjEPQrqQ3HulEJSWBMkRhl52+0+roVpqr++EpudwfNrr?=
 =?iso-8859-1?Q?A6VRSd/VtLj65h80dsgUp9fGQKqDZiYBscKANy/nz3bWI+OvxlumXzTDAY?=
 =?iso-8859-1?Q?nSC90TFm58Xda/ZZN0B3/Og+gHvFVrLkN/yz7bx+CVR6HhsDxyA9HlaXGz?=
 =?iso-8859-1?Q?Wd04bVIMtp/m/8e8jTaJ4Mhqx4V/8kG4QBiaSxEZMMeQ1lAQ5sU3MtbQ1i?=
 =?iso-8859-1?Q?tizwGcNfRaRc5vnjcyHo5Jyzi4ISyBBnJ32vR/Ynu5XUjOLAQRkTUl4DmO?=
 =?iso-8859-1?Q?gEl2wcDIBYajspcF3kDScNICt3gbdLkmdbOCW2VHMpOEoQYtHEN1BLEgF5?=
 =?iso-8859-1?Q?Uvc+MOxiG7wJonRsgRat4VyPQhFq+sIWXQr/kyl4JzOqaYsQ1eA2P/ZYvX?=
 =?iso-8859-1?Q?xJgEM6xf+bNGUzhNZzTychKHVMlSzYhUN7uEeCa24JPg2S4gfjS7wQjsJR?=
 =?iso-8859-1?Q?HVapK2zMAyl3TzdUJ5KDWDvH51vvOB7KT+S02rRUdYR9u+szEAQOx5xWjp?=
 =?iso-8859-1?Q?NCZ5RESmvYYouSiXCU6Pf+Zcfn8ZDQ6cSWtRUmVnGjTiJpF2zB5Nv9/+MQ?=
 =?iso-8859-1?Q?CNOD6Ew1LrervEd4S9AowIDMOT1Qh/QXWX98YdT7imBaZmcapYuhDNoTPt?=
 =?iso-8859-1?Q?KlcTeW8abaWV5FDmtHE4lc6s6YO4vbmfctWidsIL598vZ5MmjbVuMtleZc?=
 =?iso-8859-1?Q?c518Akf46hxD3I2E4nlxlUPYKT3Hr1cIxIm0qCFYTvSFz0I6Qhzy39RccO?=
 =?iso-8859-1?Q?Cp6sF/usfkoGTqZ9uRA1ACHtlXPMB0vEtQFoWh2dWA1mGQHbznKn7OT7Id?=
 =?iso-8859-1?Q?Bqe1VVMyj/DDBg/LrDLWoMGRBiWyfeUjziTDWtrrFm5l0pxv+icRMCRcHP?=
 =?iso-8859-1?Q?fjV+6S1c4hYSyVMWIeXKsFPzaV6+yH5PRhhx4Y30ucfbl+1tkbhLk4Woh/?=
 =?iso-8859-1?Q?PMkFVDWjHQNkCvDXobCWmOOrTeK//zYZ1e72g2Zd+v0zCLCNBCVYWJaekK?=
 =?iso-8859-1?Q?Ef5jO6pLbYtG8ENoc60Xbhk5l+mpOnUChQkhT5CMAPuYTk0reoiYXPDYux?=
 =?iso-8859-1?Q?VU6592GAvdo0I=3D?=
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: PAXPR04MB9448.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e0bb3af6-dbdf-48a6-0b23-08dbcdf4d1e4
X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Oct 2023 03:05:57.9523
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: EZASTgTfGZcnbIY6eYYhFdaOKGSorV+DzP+oykp+U9qkNzSlTdpVhe0qdEX0cZk+xu5Nhl+HoAg0E0M/E+kxHA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM8PR04MB7298
X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on howler.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Sun, 15 Oct 2023 20:07:03 -0700 (PDT)

Hi Alexander,

Thanks your comments,

>=20
>=20
> Hi Sandor,
>=20
> thanks for the updated series.
>=20
> 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@nxp.com>
> > Tested-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > 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] =3D 0
> > + *          packet[1-3] =3D HB[0-2]  InfoFrame Packet Header
> > + *          packet[4-31 =3D 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 !=3D 32)
> > +             return;
> > +
> > +     /* invalidate entry */
> > +     val =3D 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 =3D packet_len / 4;
> > +     for (i =3D 0; i < len32; i++) {
> > +             packet32 =3D 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 =3D 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 =3D data;
> > +     u8 msg[2], reg[5], i;
> > +     int ret;
> > +
> > +     mutex_lock(&mhdp->mbox_mutex);
> > +
> > +     for (i =3D 0; i < 4; i++) {
> > +             msg[0] =3D block / 2;
> > +             msg[1] =3D block % 2;
> > +
> > +             ret =3D cdns_mhdp_mailbox_send(&mhdp->base,
> MB_MODULE_ID_HDMI_TX,
> > HDMI_TX_EDID, +
> sizeof(msg),
> msg);
> > +             if (ret)
> > +                     continue;
> > +
> > +             ret =3D cdns_mhdp_mailbox_recv_header(&mhdp->base,
> MB_MODULE_ID_HDMI_TX,
> > +
> HDMI_TX_EDID,
> sizeof(reg) + length);
> > +             if (ret)
> > +                     continue;
> > +
> > +             ret =3D cdns_mhdp_mailbox_recv_data(&mhdp->base, reg,
> sizeof(reg));
> > +             if (ret)
> > +                     continue;
> > +
> > +             ret =3D cdns_mhdp_mailbox_recv_data(&mhdp->base, edid,
> length);
> > +             if (ret)
> > +                     continue;
> > +
> > +             if ((reg[3] << 8 | reg[4]) =3D=3D 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] =3D 0x54;
> > +     msg[1] =3D addr;
> > +     msg[2] =3D 0;
> > +     msg[3] =3D 1;
> > +     msg[4] =3D value;
> > +
> > +     mutex_lock(&mhdp->mbox_mutex);
>=20
> 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=FChlstra=DFe 2, Gut Delling | 82229 Seefeld,
> Germany Amtsgericht M=FCnchen, HRB 105018
> Gesch=E4ftsf=FChrer: Detlef Schneider, R=FCdiger Stahl, Stefan Schneider
>=20

There are two types of command carried over the mailbox channel, with respo=
nd 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 ca=
lled by HDMI driver,=20
so I keep the function in MHDP8501 HDMI driver,=20
if the other mhdp want to reused it later, it should be move to helper driv=
er.

B.R
Sandor