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

From: Sandor Yu
Date: Sun Feb 04 2024 - 09:29:33 EST

Next message: Ricardo B. Marliere: "[PATCH] x86: mce: make mce_subsys const"
Previous message: Sandor Yu: "RE: [EXT] Re: [PATCH v12 7/7] phy: freescale: Add HDMI PHY driver for i.MX8MQ"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Alexander,

Thanks your comments,

>
>
> Hi Sandor,
>
> thanks for the update.
>
> Am Mittwoch, 10. Januar 2024, 02:08:45 CET 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>
> > ---
> > v11->v12:
> > - Replace DRM_INFO with dev_info or dev_warn.
> > - Replace DRM_ERROR with dev_err.
> > - Return ret when cdns_mhdp_dpcd_read failed in function
> > cdns_dp_aux_transferi(). - Remove unused parmeter in function
> > cdns_dp_get_msa_misc
> > and use two separate variables for color space and bpc.
> > - Add year 2024 to copyright.
> >
> > drivers/gpu/drm/bridge/cadence/Kconfig | 16 +
> > drivers/gpu/drm/bridge/cadence/Makefile | 2 +
> > .../drm/bridge/cadence/cdns-mhdp8501-core.c | 315 ++++++++
> > .../drm/bridge/cadence/cdns-mhdp8501-core.h | 365 +++++++++
> > .../gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c | 699
> ++++++++++++++++++
> > .../drm/bridge/cadence/cdns-mhdp8501-hdmi.c | 678
> +++++++++++++++++
> > 6 files changed, 2075 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/Kconfig
> > b/drivers/gpu/drm/bridge/cadence/Kconfig index
> > e0973339e9e33..45848e741f5f4
> > 100644
> > --- a/drivers/gpu/drm/bridge/cadence/Kconfig
> > +++ b/drivers/gpu/drm/bridge/cadence/Kconfig
> > @@ -51,3 +51,19 @@ config DRM_CDNS_MHDP8546_J721E
> > initializes the J721E Display Port and sets up the
> > clock and data muxes.
> > endif
> > +
> > +config DRM_CDNS_MHDP8501
> > + tristate "Cadence MHDP8501 DP/HDMI bridge"
> > + select DRM_KMS_HELPER
> > + select DRM_PANEL_BRIDGE
> > + select DRM_DISPLAY_DP_HELPER
> > + select DRM_DISPLAY_HELPER
> > + select CDNS_MHDP_HELPER
> > + select DRM_CDNS_AUDIO
> > + depends on OF
> > + help
> > + Support Cadence MHDP8501 DisplayPort/HDMI bridge.
> > + Cadence MHDP8501 support one or more protocols,
> > + including DisplayPort and HDMI.
> > + To use the DP and HDMI drivers, their respective
> > + specific firmware is required.
> > diff --git a/drivers/gpu/drm/bridge/cadence/Makefile
> > b/drivers/gpu/drm/bridge/cadence/Makefile index
> > 087dc074820d7..02c1a9f3cf6fc 100644
> > --- a/drivers/gpu/drm/bridge/cadence/Makefile
> > +++ b/drivers/gpu/drm/bridge/cadence/Makefile
> > @@ -6,3 +6,5 @@ obj-$(CONFIG_CDNS_MHDP_HELPER) +=
> cdns-mhdp-helper.o
> > obj-$(CONFIG_DRM_CDNS_MHDP8546) += cdns-mhdp8546.o
> cdns-mhdp8546-y
> > := cdns-mhdp8546-core.o cdns-mhdp8546-hdcp.o
> > cdns-mhdp8546-$(CONFIG_DRM_CDNS_MHDP8546_J721E) +=
> > cdns-mhdp8546-j721e.o
> > +obj-$(CONFIG_DRM_CDNS_MHDP8501) += cdns-mhdp8501.o
> cdns-mhdp8501-y :=
> > +cdns-mhdp8501-core.o cdns-mhdp8501-dp.o
> > cdns-mhdp8501-hdmi.o diff --git
> > a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c
> > b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c new file mode
> > 100644 index 0000000000000..3080c7507a012
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c
> > @@ -0,0 +1,315 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Cadence Display Port Interface (DP) driver
> > + *
> > + * Copyright (C) 2023, 2024 NXP Semiconductor, Inc.
> > + *
> > + */
> > +#include <drm/drm_of.h>
> > +#include <drm/drm_print.h>
> > +#include <linux/clk.h>
> > +#include <linux/irq.h>
> > +#include <linux/mutex.h>
> > +#include <linux/of_device.h>
>
> Since commit d57d584ef69de ("of: Stop circularly including of_device.h and
> of_platform.h") you to explicitly include linux/platform_device.h here. Please
> compile against next tree.

OK, I will check it on next tree.

>
> > +#include <linux/phy/phy.h>
> > +
> > +#include "cdns-mhdp8501-core.h"
> > +
> > [snip]
> > diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c
> > b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c new file mode
> > 100644 index 0000000000000..6963c7143a3b0
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c
> > @@ -0,0 +1,699 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Cadence MHDP8501 DisplayPort(DP) bridge driver
> > + *
> > + * Copyright (C) 2019-2024 NXP Semiconductor, Inc.
> > + *
> > + */
> > +#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-dp.h>
> > +
> > +#include "cdns-mhdp8501-core.h"
> > +
> > +#define LINK_TRAINING_TIMEOUT_MS 500
> > +#define LINK_TRAINING_RETRY_MS 20
> > +
> > +ssize_t cdns_dp_aux_transfer(struct drm_dp_aux *aux,
> > + struct drm_dp_aux_msg *msg) {
> > + struct cdns_mhdp8501_device *mhdp = dev_get_drvdata(aux->dev);
> > + bool native = msg->request & (DP_AUX_NATIVE_WRITE &
> DP_AUX_NATIVE_READ);
> > + int ret;
> > +
> > + /* Ignore address only message */
> > + if (!msg->size || !msg->buffer) {
> > + msg->reply = native ?
> > + DP_AUX_NATIVE_REPLY_ACK :
> DP_AUX_I2C_REPLY_ACK;
> > + return msg->size;
> > + }
> > +
> > + if (!native) {
> > + dev_err(mhdp->dev, "%s: only native messages
> > + supported\n",
> __func__);
> > + return -EINVAL;
> > + }
> > +
> > + /* msg sanity check */
> > + if (msg->size > DP_AUX_MAX_PAYLOAD_BYTES) {
> > + dev_err(mhdp->dev, "%s: invalid msg: size(%zu),
> request(%x)\n",
> > + __func__, msg->size, (unsigned int)msg-
> >request);
> > + return -EINVAL;
> > + }
> > +
> > + if (msg->request == DP_AUX_NATIVE_WRITE) {
> > + const u8 *buf = msg->buffer;
> > + int i;
> > +
> > + for (i = 0; i < msg->size; ++i) {
> > + ret = cdns_mhdp_dpcd_write(&mhdp->base,
> > + msg->address +
> i, buf[i]);
> > + if (ret < 0) {
> > + dev_err(mhdp->dev, "Failed to write
> DPCD\n");
> > + return ret;
> > + }
> > + }
> > + msg->reply = DP_AUX_NATIVE_REPLY_ACK;
> > + return msg->size;
> > + }
> > +
> > + if (msg->request == DP_AUX_NATIVE_READ) {
> > + ret = cdns_mhdp_dpcd_read(&mhdp->base, msg->address,
> > + msg->buffer, msg->size);
> > + if (ret < 0)
> > + return ret;
> > + msg->reply = DP_AUX_NATIVE_REPLY_ACK;
> > + return msg->size;
> > + }
> > + return 0;
> > +}
> > +
> > +int cdns_dp_aux_destroy(struct cdns_mhdp8501_device *mhdp) {
> > + drm_dp_aux_unregister(&mhdp->dp.aux);
> > +
> > + return 0;
> > +}
> > +
> > +static int cdns_dp_get_msa_misc(struct video_info *video) {
> > + u32 msa_misc;
> > + u8 color_space = 0;
> > + u8 bpc = 0;
> > +
> > + switch (video->color_fmt) {
> > + /* set YUV default color space conversion to BT601 */
> > + case DRM_COLOR_FORMAT_YCBCR444:
> > + color_space = 6 + BT_601 * 8;
> > + break;
> > + case DRM_COLOR_FORMAT_YCBCR422:
> > + color_space = 5 + BT_601 * 8;
> > + break;
> > + case DRM_COLOR_FORMAT_YCBCR420:
> > + color_space = 5;
> > + break;
> > + case DRM_COLOR_FORMAT_RGB444:
> > + default:
> > + color_space = 0;
> > + break;
> > + };
> > +
> > + switch (video->bpc) {
> > + case 6:
> > + bpc = 0;
> > + break;
> > + case 10:
> > + bpc = 2;
> > + break;
> > + case 12:
> > + bpc = 3;
> > + break;
> > + case 16:
> > + bpc = 4;
> > + break;
> > + case 8:
> > + default:
> > + bpc = 1;
> > + break;
> > + };
> > +
> > + msa_misc = (color_space << 1) | (bpc << 5);
>
> This looks much nicer, thanks. But please order them in descending shift
> width: bpc first then color_space.
>

OK.

> > +
> > + return msa_misc;
> > +}
> > +
> > [snip]
> > +int cdns_dp_set_host_cap(struct cdns_mhdp8501_device *mhdp)
>
> This can be made static.

OK.

>
> > +{
> > + u8 msg[8];
> > + int ret;
> > +
> > + msg[0] = drm_dp_link_rate_to_bw_code(mhdp->dp.rate);
> > + msg[1] = mhdp->dp.num_lanes | SCRAMBLER_EN;
> > + msg[2] = VOLTAGE_LEVEL_2;
> > + msg[3] = PRE_EMPHASIS_LEVEL_3;
> > + msg[4] = PTS1 | PTS2 | PTS3 | PTS4;
> > + msg[5] = FAST_LT_NOT_SUPPORT;
> > + msg[6] = mhdp->lane_mapping;
> > + msg[7] = ENHANCED;
> > +
> > + mutex_lock(&mhdp->mbox_mutex);
> > +
> > + ret = cdns_mhdp_mailbox_send(&mhdp->base,
> MB_MODULE_ID_DP_TX,
> > + DPTX_SET_HOST_CAPABILITIES,
> > + sizeof(msg), msg);
> > +
> > + mutex_unlock(&mhdp->mbox_mutex);
> > +
> > + if (ret)
> > + dev_err(mhdp->dev, "set host cap failed: %d\n", ret);
> > +
> > + return ret;
> > +}
> > [snip]
> > 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..ae21f7dfe5e94
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-hdmi.c
> > @@ -0,0 +1,678 @@
> > [snip]
> > +bool cdns_hdmi_bridge_mode_fixup(struct drm_bridge *bridge,
> > + const struct drm_display_mode
> *mode,
> > + struct drm_display_mode
> *adjusted_mode)
>
> This can be made static.

OK, thanks!

Sandor

>
> Thanks and best regards,
> Alexander
>
> > +{
> > + struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
> > + struct video_info *video = &mhdp->video_info;
> > +
> > + /* The only currently supported format */
> > + video->bpc = 8;
> > + video->color_fmt = DRM_COLOR_FORMAT_RGB444;
> > +
> > + return true;
> > +}
> > +
> > +static enum drm_connector_status
> > +cdns_hdmi_bridge_detect(struct drm_bridge *bridge) {
> > + struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
> > +
> > + return cdns_mhdp8501_detect(mhdp); }
> > +
> > +static struct edid *cdns_hdmi_bridge_get_edid(struct drm_bridge *bridge,
> > + struct drm_connector
> *connector)
> > +{
> > + struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
> > +
> > + return drm_do_get_edid(connector, cdns_hdmi_get_edid_block,
> > +mhdp); }
> > +
> > +static void cdns_hdmi_bridge_atomic_disable(struct drm_bridge *bridge,
> > + struct drm_bridge_state
> *old_state)
> > +{
> > + struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
> > +
> > + mhdp->curr_conn = NULL;
> > +
> > + /* Mailbox protect for HDMI PHY access */
> > + mutex_lock(&mhdp->mbox_mutex);
> > + phy_power_off(mhdp->phy);
> > + mutex_unlock(&mhdp->mbox_mutex); }
> > +
> > +static void cdns_hdmi_bridge_atomic_enable(struct drm_bridge *bridge,
> > + struct drm_bridge_state
> *old_state)
> > +{
> > + struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
> > + struct drm_atomic_state *state = old_state->base.state;
> > + struct drm_connector *connector;
> > + struct drm_crtc_state *crtc_state;
> > + struct drm_connector_state *conn_state;
> > + const struct drm_display_mode *mode;
> > +
> > + connector = drm_atomic_get_new_connector_for_encoder(state,
> > +
> bridge->encoder);
> > + if (WARN_ON(!connector))
> > + return;
> > +
> > + mhdp->curr_conn = connector;
> > +
> > + conn_state = drm_atomic_get_new_connector_state(state,
> connector);
> > + if (WARN_ON(!conn_state))
> > + return;
> > +
> > + crtc_state = drm_atomic_get_new_crtc_state(state,
> conn_state->crtc);
> > + if (WARN_ON(!crtc_state))
> > + return;
> > +
> > + mode = &crtc_state->adjusted_mode;
> > + dev_dbg(mhdp->dev, "Mode: %dx%dp%d\n",
> > + mode->hdisplay, mode->vdisplay, mode->clock);
> > + memcpy(&mhdp->mode, mode, sizeof(struct drm_display_mode));
> > +
> > + cdns_hdmi_mode_set(mhdp);
> > +}
> > +
> > +const struct drm_bridge_funcs cdns_hdmi_bridge_funcs = {
> > + .attach = cdns_hdmi_bridge_attach,
> > + .detect = cdns_hdmi_bridge_detect,
> > + .get_edid = cdns_hdmi_bridge_get_edid,
> > + .mode_valid = cdns_hdmi_bridge_mode_valid,
> > + .mode_fixup = cdns_hdmi_bridge_mode_fixup,
> > + .atomic_enable = cdns_hdmi_bridge_atomic_enable,
> > + .atomic_disable = cdns_hdmi_bridge_atomic_disable,
> > + .atomic_duplicate_state =
> drm_atomic_helper_bridge_duplicate_state,
> > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> > + .atomic_reset = drm_atomic_helper_bridge_reset, };
>
>
> --
> 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
> http://www.tq-/
> group.com%2F&data=05%7C02%7CSandor.yu%40nxp.com%7Cb23e376f27494
> 9cdb46c08dc17413d46%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C638410816178929125%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7
> C%7C&sdata=tYc81WajNJAOmUvSD06cto0i%2FhVe%2BuLFxIeYm0uyMDM%3
> D&reserved=0
>

Next message: Ricardo B. Marliere: "[PATCH] x86: mce: make mce_subsys const"
Previous message: Sandor Yu: "RE: [EXT] Re: [PATCH v12 7/7] phy: freescale: Add HDMI PHY driver for i.MX8MQ"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]