On Mon, Dec 30, 2024 at 08:05:33AM +0000, Sandor Yu wrote:
>
> > On Wed, Dec 25, 2024 at 07:57:01AM +0000, Sandor Yu wrote:
> > > >
> > > > On Tue, Dec 17, 2024 at 02:51:47PM +0800, Sandor Yu wrote:
> > > > > 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>
> > > > > ---
> > > > > v19->v20:
> > > > > - Dump mhdp FW version by debugfs
> > > > > - Combine HDMI and DP cable detect functions into one function
> > > > > - Combine HDMI and DP cable bridge_mode_valid() functions into one
> > > > function
> > > > > - Rename cdns_hdmi_reset_link() to cdns_hdmi_handle_hotplug()
> > > > > - Add comments for EDID in cdns_hdmi_handle_hotplug() and
> > > > cdns_dp_check_link_state()
> > > > > - Add atomic_get_input_bus_fmts() and bridge_atomic_check() for DP
> > > > > driver
> > > > > - Remove bpc and color_fmt init in atomic_enable() function.
> > > > > - More detail comments for DDC adapter only support
> > > > SCDC_I2C_SLAVE_ADDRESS
> > > > > read and write in HDMI driver.
> > > > >
> > > > >
> > > > > v18->v19:
> > > > > - Get endpoint for data-lanes as it had move to endpoint of port@1
> > > > > - Update clock management as devm_clk_get_enabled() introduced.
> > > > > - Fix clear_infoframe() function is not work issue.
> > > > > - Manage PHY power state via phy_power_on() and phy_power_off().
> > > > >
> > > > > v17->v18:
> > > > > - MHDP8501 HDMI and DP commands that need access mailbox are
> > > > rewrited
> > > > > with new API functions created in patch #1.
> > > > > - replace lane-mapping with data-lanes, use the value from data-lanes
> > > > > to reorder HDMI and DP lane mapping.
> > > > > - create I2C adapter for HDMI SCDC, remove cdns_hdmi_scdc_write()
> > > > function.
> > > > > - Rewrite cdns_hdmi_sink_config() function, use HDMI SCDC helper
> > function
> > > > > drm_scdc_set_high_tmds_clock_ratio() and
> > drm_scdc_set_scrambling()
> > > > > to config HDMI sink TMDS.
> > > > > - Remove struct video_info from HDMI driver.
> > > > > - Remove tmds_char_rate_valid() be called in bridge_mode_valid(),
> > > > > community had patch in reviewing to implement the function.
> > > > > - Remove warning message print when get unknown HPD cable status.
> > > > > - Add more detail comments for HDP plugin and plugout interrupt.
> > > > > - use dev_dbg to repleace DRM_INFO when cable HPD status changed.
> > > > > - Remove t-b tag as above code change.
> > > > >
> > > > > drivers/gpu/drm/bridge/cadence/Kconfig | 16 +
> > > > > drivers/gpu/drm/bridge/cadence/Makefile | 2 +
> > > > > .../drm/bridge/cadence/cdns-mhdp8501-core.c | 379 +++++++++
> > > > > .../drm/bridge/cadence/cdns-mhdp8501-core.h | 380 +++++++++
> > > > > .../gpu/drm/bridge/cadence/cdns-mhdp8501-dp.c | 694
> > > > ++++++++++++++++
> > > > > .../drm/bridge/cadence/cdns-mhdp8501-hdmi.c | 745
> > > > ++++++++++++++++++
> > > > > 6 files changed, 2216 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 dbb06533ccab2..bd979f3e6df48 100644
> > > > > --- a/drivers/gpu/drm/bridge/cadence/Kconfig
> > > > > +++ b/drivers/gpu/drm/bridge/cadence/Kconfig
> > > > > @@ -48,3 +48,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 DRM_CDNS_AUDIO
> > > > > + select CDNS_MHDP_HELPER
> > > > > + 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 c95fd5b81d137..ea327287d1c14 100644
> > > > > --- a/drivers/gpu/drm/bridge/cadence/Makefile
> > > > > +++ b/drivers/gpu/drm/bridge/cadence/Makefile
> > > > > @@ -5,3 +5,5 @@ cdns-dsi-$(CONFIG_DRM_CDNS_DSI_J721E) +=
> > > > cdns-dsi-j721e.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..98116ef012fa3
> > > > > --- /dev/null
> > > > > +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8501-core.c
> > > > > @@ -0,0 +1,379 @@
> > > > > +// 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>
> > > > > +#include <linux/platform_device.h> #include <linux/phy/phy.h>
> > > > > +
> > > > > +#include "cdns-mhdp8501-core.h"
> > > > > +
> > > > > +static ssize_t firmware_version_show(struct device *dev,
> > > > > + struct device_attribute *attr,
> > > > char *buf);
> > > > > +static struct device_attribute firmware_version =
> > > > __ATTR_RO(firmware_version);
> > > > > +
> > > > > +ssize_t firmware_version_show(struct device *dev,
> > > > > + struct device_attribute *attr, char
> > > > > +*buf) {
> > > > > + struct cdns_mhdp8501_device *mhdp = dev_get_drvdata(dev);
> > > > > +
> > > > > + u32 version = readl(mhdp->base.regs + VER_L) |
> > > > readl(mhdp->base.regs + VER_H) << 8;
> > > > > + u32 lib_version = readl(mhdp->base.regs + VER_LIB_L_ADDR) |
> > > > > + readl(mhdp->base.regs +
> > VER_LIB_H_ADDR)
> > > > << 8;
> > > > > +
> > > > > + return sprintf(buf, "FW version %d, Lib version %d\n",
> > > > > + version,
> > > > lib_version);
> > > > > +}
> > > > > +
> > > > > +static void cdns_mhdp8501_create_device_files(struct
> > > > cdns_mhdp8501_device *mhdp)
> > > > > +{
> > > > > + if (device_create_file(mhdp->dev, &firmware_version)) {
> > > > > + DRM_ERROR("Unable to create firmware_version
> > > > sysfs\n");
> > > > > + device_remove_file(mhdp->dev, &firmware_version);
> > > > > + }
> > > > > +}
> > > > > +
> > > > > +static void cdns_mhdp8501_remove_device_files(struct
> > > > cdns_mhdp8501_device *mhdp)
> > > > > +{
> > > > > + device_remove_file(mhdp->dev, &firmware_version); }
> > > > > +
> > > > > +static int cdns_mhdp8501_read_hpd(struct cdns_mhdp8501_device
> > > > *mhdp)
> > > > > +{
> > > > > + u8 status;
> > > > > + int ret;
> > > > > +
> > > > > + ret = cdns_mhdp_mailbox_send_recv(&mhdp->base,
> > > > MB_MODULE_ID_GENERAL,
> > > > > +
> > > > GENERAL_GET_HPD_STATE,
> > > > > + 0, NULL,
> > sizeof(status),
> > > > &status);
> > > > > + if (ret) {
> > > > > + dev_err(mhdp->dev, "read hpd failed: %d\n", ret);
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > + return status;
> > > > > +}
> > > > > +
> > > > > +enum drm_connector_status cdns_mhdp8501_detect(struct
> > drm_bridge
> > > > *bridge)
> > > > > +{
> > > > > + struct cdns_mhdp8501_device *mhdp = bridge->driver_private;
> > > > > +
> > > > > + u8 hpd = 0xf;
> > > > > +
> > > > > + hpd = cdns_mhdp8501_read_hpd(mhdp);
> > > > > + if (hpd == 1)
> > > > > + return connector_status_connected;
> > > > > + else if (hpd == 0)
> > > > > + return connector_status_disconnected;
> > > > > +
> > > > > + return connector_status_unknown; }
> > > > > +
> > > > > +enum drm_mode_status
> > > > > +cdns_mhdp8501_mode_valid(struct drm_bridge *bridge,
> > > > > + const struct drm_display_info *info,
> > > > > + const struct drm_display_mode *mode) {
> > > > > + /* We don't support double-clocked */
> > > > > + if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> > > > > + return MODE_BAD;
> > > > > +
> > > > > + /* MAX support pixel clock rate 594MHz */
> > > > > + if (mode->clock > 594000)
> > > > > + return MODE_CLOCK_HIGH;
> > > > > +
> > > > > + if (mode->hdisplay > 3840)
> > > > > + return MODE_BAD_HVALUE;
> > > > > +
> > > > > + if (mode->vdisplay > 2160)
> > > > > + return MODE_BAD_VVALUE;
> > > > > +
> > > > > + return MODE_OK;
> > > > > +}
> > > > > +
> > > > > +static void hotplug_work_func(struct work_struct *work) {
> > > > > + struct cdns_mhdp8501_device *mhdp = container_of(work,
> > > > > + struct
> > > > cdns_mhdp8501_device,
> > > > > +
> > > > hotplug_work.work);
> > > > > + enum drm_connector_status status =
> > > > cdns_mhdp8501_detect(&mhdp->bridge);
> > > > > +
> > > > > + drm_bridge_hpd_notify(&mhdp->bridge, status);
> > > > > +
> > > > > + /*
> > > > > + * iMX8MQ has two HPD interrupts: one for plugout and one
> > > > > + for
> > > > plugin.
> > > > > + * These interrupts cannot be masked and cleaned, so we must
> > > > enable one
> > > > > + * and disable the other to avoid continuous interrupt
> > generation.
> > > > > + */
> > > > > + if (status == connector_status_connected) {
> > > > > + /* Cable connected */
> > > > > + dev_dbg(mhdp->dev, "HDMI/DP Cable Plug In\n");
> > > > > + enable_irq(mhdp->irq[IRQ_OUT]);
> > > > > +
> > > > > + /* Reset HDMI/DP link with sink */
> > > > > + if (mhdp->connector_type ==
> > > > DRM_MODE_CONNECTOR_HDMIA)
> > > > > + cdns_hdmi_handle_hotplug(mhdp);
> > > > > + else
> > > > > + cdns_dp_check_link_state(mhdp);
> > > > > +
> > > > > + } else if (status == connector_status_disconnected) {
> > > > > + /* Cable Disconnected */
> > > > > + dev_dbg(mhdp->dev, "HDMI/DP Cable Plug Out\n");
> > > > > + enable_irq(mhdp->irq[IRQ_IN]);
> > > > > + }
> > > > > +}
> > > > > +
> > > > > +static irqreturn_t cdns_mhdp8501_irq_thread(int irq, void *data)
> > > > > +{
> > > > > + struct cdns_mhdp8501_device *mhdp = data;
> > > > > +
> > > > > + disable_irq_nosync(irq);
> > > > > +
> > > > > + mod_delayed_work(system_wq, &mhdp->hotplug_work,
> > > > > +
> > msecs_to_jiffies(HOTPLUG_DEBOUNCE_MS));
> > > > > +
> > > > > + return IRQ_HANDLED;
> > > > > +}
> > > > > +
> > > > > +#define DATA_LANES_COUNT 4
> > > > > +static int cdns_mhdp8501_dt_parse(struct cdns_mhdp8501_device
> > > > *mhdp,
> > > > > + struct platform_device *pdev) {
> > > > > + struct device *dev = &pdev->dev;
> > > > > + struct device_node *np = dev->of_node;
> > > > > + struct device_node *remote, *endpoint;
> > > > > + u32 data_lanes[DATA_LANES_COUNT];
> > > > > + u32 lane_value;
> > > > > + int ret, i;
> > > > > +
> > > > > + remote = of_graph_get_remote_node(np, 1, 0);
> > > > > + if (!remote) {
> > > > > + dev_err(dev, "fail to get remote node\n");
> > > > > + of_node_put(remote);
> > > > > + return -EINVAL;
> > > > > + }
> > > > > +
> > > > > + /* get connector type */
> > > > > + if (of_device_is_compatible(remote, "hdmi-connector")) {
> > > > > + mhdp->connector_type =
> > > > DRM_MODE_CONNECTOR_HDMIA;
> > > > > +
> > > > > + } else if (of_device_is_compatible(remote, "dp-connector")) {
> > > > > + mhdp->connector_type =
> > > > DRM_MODE_CONNECTOR_DisplayPort;
> > > >
> > > > Interesting hack. What if somebody wraps DP signals with the USB-C
> > > > controller in order to provide DP over USB-C?
> > >
> > > There is no such requirement now, and the supported types of
> > display-connectors do not include Type-C DP connectors.
> > > If type-C DP connectors is added in the future, I think it would be acceptable
> > to modify the code here.
> >
> > It would be acceptable, but it most likely will also require chaning the DT
> > bindings as there is no longer an easy way to identify the next bridge. Also you
> > might not have that now, but it is pretty common to have DP retimers on the
> > board in order to improve the sinal integrity.
> > Thus I think it is not a proper solution to check the next node's compatible. I
> > think we need a way to specify HDMI vs DP firmware / mode in the device
> > tree.
> >
>
> Since there already have the connector type for HDMI or DP.
> adding another parameter to the DT bindings to specify the PHY type seems redundant.
>
> If anyone add DP retimer to the board, the similar code as followed may add to the driver.
> last_remote = remote;
> while (of_graph_get_remote_node(last_remote, 1, 0))
> last_remote = of_graph_get_remote_node(last_remote, 1, 0);
Definite NAK for such a code piece. It's not a host driver's business to
traverse DT for other, independent devices. They might or might not
follow the usb-switch.yaml
>
> Before new requirements arise, I hope to keep the current implementation.
I'd say, no. Please make it good from the beginning.
--
With best wishes
Dmitry
