| Message ID | 1596713672-8146-1-git-send-email-sjakhade@cadence.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm: Add support for Cadence MHDP DPI/DP bridge and J721E wrapper. | expand |
Hi, On Thu, Aug 06, 2020 at 01:34:29PM +0200, Swapnil Jakhade wrote: > This patch series adds new DRM bridge driver for Cadence MHDP DPI/DP > bridge. The Cadence Display Port IP is also referred as MHDP (Mobile High > Definition Link, High-Definition Multimedia Interface, Display Port). > Cadence Display Port complies with VESA DisplayPort (DP) and embedded > Display Port (eDP) standards. Is there any relation to the cadence mhdp ip core used inthe imx8mq: https://lore.kernel.org/dri-devel/cover.1590982881.git.Sandor.yu@nxp.com/ It looks very similar in several places so should that use the same driver? Cheers, -- Guido > > The MHDP bridge driver currently implements Single Stream Transport (SST) > mode. It also adds Texas Instruments j721e SoC specific wrapper and adds > the device tree bindings in YAML format. > > Some of the features that will be added later on include (but are not > limited to): > - Power Management (PM) support: We will implement the PM functions in > next stage once there will be a stable driver in upstream > - Audio and MST support > > The patch series has three patches in the below sequence: > 1. 0001-dt-bindings-drm-bridge-Document-Cadence-MHDP-brid.patch > Documents the bindings in yaml format. > 2. 0002-drm-bridge-Add-support-for-Cadence-MHDP-DPI-DP-br.patch > This patch adds new DRM bridge driver for Cadence MHDP Display Port. > The patch implements support for single stream transport mode. > 3. 0003-drm-bridge-cdns-mhdp-Add-j721e-wrapper.patch > Adds Texas Instruments (TI) j721e wrapper for MHDP. The wrapper configures > MHDP clocks and muxes as required by SoC. > > This patch series is dependent on PHY patch series [1] to add new PHY APIs > to get/set PHY attributes which is under review and not merged yet. > > [1] https://lkml.org/lkml/2020/7/17/158 > > Version History: > > v8: > > In 1/3 > - Fix error reported by dt_binding_check > - Fix indent in the example > - Fix other comments given for v7 patches. > > In 2/3: > - Implement bridge connector operations .get_edid() and .detect(). > - Make connector creation optional based on DRM_BRIDGE_ATTACH_NO_CONNECTOR > flag. > - Fix other comments given for v7 patches. > > In 3/3 > - Fix comments given for v7 patches. > > v7: > > In 1/3 > - No change > > In 2/3 > - Switch to atomic versions of bridge operations > - Implement atomic_check() handler to perform all validation checks > - Add struct cdns_mhdp_bridge_state with subclassed bridge state > - Use PHY API[1] to get PHY attributes instead of reading from PHY DT node > - Updated HPD handling and link configuration in IRQ handler > - Add "link_mutex" protecting the access to all the link parameters > - Add support to check and print FW version information > - Add separate function to initialize host parameters to simplify probe > - Use waitqueue instead of manual loop in cdns_mhdp_remove > - Add forward declarations and header files in cdns-mhdp-core.h file > - Use bool instead of single bit values in struct cdns_mhdp_device > - Fix for other minor comments given for v6 patches > > In 3/3 > - Use of_device_is_compatible() to set compatible string specific values > - Move mhdp_ti_j721e_ops structure to cdns-mhdp-j721e.c > - Remove duplicate Copyright message > - Remove CONFIG_DRM_CDNS_MHDP_J721E check > - Add Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > v6: > - Added minor fixes in YAML file. > - Added Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > to the YAML patch. > - Removed all the FIXME comments which are invalid in drm driver. > - Reduced the mailbox timeout from 5s to 2s. > - Added Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > to the 003-drm-mhdp-add-j721e-wrapper patch. > - Added Signed-off all the module authors. > - Fixed the compiler error Reported-by: kbuild test robot <lkp@intel.com>. > > v5: > - Added Signed-off-by: Jyri Sarha <jsarha@ti.com> tag to > the code patches. > > v4: > - Added SPDX dual license tag to YAML bindings. > - Corrected indentation of the child node properties. > - Removed the maxItems in the conditional statement. > - Add Reviewed-by: Rob Herring <robh@kernel.org> tag to the > Document Cadence MHDP bridge bindings patch. > - Renamed the DRM driver executable name from mhdp8546 to cdns-mhdp in > Makefile. > - Renamed the DRM driver and header file from cdns-mhdp to cdns-mhdp-core. > > v3: > - Added if / then clause to validate that the reg length is proper > based on the value of the compatible property. > - Updated phy property description in YAML to a generic one. > - Renamed num_lanes and max_bit_rate property strings to cdns,num-lanes > and cdns,max-bit-rate. > > v2: > - Use enum in compatible property of YAML file. > - Add reg-names property to YAML file > - Add minItems and maxItems to reg property in YAML. > - Remove cdns_mhdp_link_probe function to remove > duplication of reading dpcd capabilities. > > Swapnil Jakhade (2): > drm: bridge: Add support for Cadence MHDP DPI/DP bridge > drm: bridge: cdns-mhdp: Add j721e wrapper > > Yuti Amonkar (1): > dt-bindings: drm/bridge: Document Cadence MHDP bridge bindings > > .../bindings/display/bridge/cdns,mhdp.yaml | 139 + > drivers/gpu/drm/bridge/Kconfig | 24 + > drivers/gpu/drm/bridge/Makefile | 4 + > drivers/gpu/drm/bridge/cdns-mhdp-core.c | 2562 +++++++++++++++++ > drivers/gpu/drm/bridge/cdns-mhdp-core.h | 397 +++ > drivers/gpu/drm/bridge/cdns-mhdp-j721e.c | 72 + > drivers/gpu/drm/bridge/cdns-mhdp-j721e.h | 19 + > 7 files changed, 3217 insertions(+) > create mode 100644 Documentation/devicetree/bindings/display/bridge/cdns,mhdp.yaml > create mode 100644 drivers/gpu/drm/bridge/cdns-mhdp-core.c > create mode 100644 drivers/gpu/drm/bridge/cdns-mhdp-core.h > create mode 100644 drivers/gpu/drm/bridge/cdns-mhdp-j721e.c > create mode 100644 drivers/gpu/drm/bridge/cdns-mhdp-j721e.h > > -- > 2.26.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
Hi Guido, On 12/08/2020 11:39, Guido Günther wrote: > Hi, > On Thu, Aug 06, 2020 at 01:34:29PM +0200, Swapnil Jakhade wrote: >> This patch series adds new DRM bridge driver for Cadence MHDP DPI/DP >> bridge. The Cadence Display Port IP is also referred as MHDP (Mobile High >> Definition Link, High-Definition Multimedia Interface, Display Port). >> Cadence Display Port complies with VESA DisplayPort (DP) and embedded >> Display Port (eDP) standards. > > Is there any relation to the cadence mhdp ip core used inthe imx8mq: > > https://lore.kernel.org/dri-devel/cover.1590982881.git.Sandor.yu@nxp.com/ > > It looks very similar in several places so should that use the same driver? > Cheers, > -- Guido Interesting. So the original Cadence DP patches for TI SoCs did create a common driver with Rockchip's older mhdp driver. And looks like the IMX series points to an early version of that patch ("drm/rockchip: prepare common code for cdns and rk dpi/dp driver"). We gave up on that as the IPs did have differences and the firmwares used were apparently quite different. The end result was very difficult to maintain, especially as (afaik) none of the people involved had relevant Rockchip HW. The idea was to get a stable DP driver for TI SoCs ready and upstream, and then carefully try to create common parts with Rockchip's driver in small pieces. If the Rockchip and IMX mhdp have the same IP and same firmware, then they obviously should share code as done in the series you point to. Perhaps Cadence can clarify the differences between IMX, TI and Rockchip IPs and FWs? I'm worried that if there are IP differences, even if not great ones, and if the FWs are different and developed separately, it'll be a constant "fix X for SoC A, and accidentally break Y for SoC B and C", especially if too much code is shared. In the long run I'm all for a single driver (or large shared parts), but I'm not sure if we should start with that approach. Tomi
Hi, On Wed, Aug 12, 2020 at 01:47:42PM +0300, Tomi Valkeinen wrote: > Hi Guido, > > On 12/08/2020 11:39, Guido Günther wrote: > > Hi, > > On Thu, Aug 06, 2020 at 01:34:29PM +0200, Swapnil Jakhade wrote: > >> This patch series adds new DRM bridge driver for Cadence MHDP DPI/DP > >> bridge. The Cadence Display Port IP is also referred as MHDP (Mobile High > >> Definition Link, High-Definition Multimedia Interface, Display Port). > >> Cadence Display Port complies with VESA DisplayPort (DP) and embedded > >> Display Port (eDP) standards. > > > > Is there any relation to the cadence mhdp ip core used inthe imx8mq: > > > > https://lore.kernel.org/dri-devel/cover.1590982881.git.Sandor.yu@nxp.com/ > > > > It looks very similar in several places so should that use the same driver? > > Cheers, > > -- Guido > > Interesting. > > So the original Cadence DP patches for TI SoCs did create a common driver with Rockchip's older mhdp > driver. And looks like the IMX series points to an early version of that patch ("drm/rockchip: > prepare common code for cdns and rk dpi/dp driver"). > > We gave up on that as the IPs did have differences and the firmwares used were apparently quite > different. The end result was very difficult to maintain, especially as (afaik) none of the people > involved had relevant Rockchip HW. Is the `struct mhdp_platform_ops` a leftover from that? Can that be dropped? > The idea was to get a stable DP driver for TI SoCs ready and upstream, and then carefully try to > create common parts with Rockchip's driver in small pieces. I wonder how imx8 would best blend into this? First thing will likely be to upstream the phy code in driveres/phy/ so a modified version of this bridge driver could call into that, then go and look for common patterns. > If the Rockchip and IMX mhdp have the same IP and same firmware, then they obviously should share > code as done in the series you point to. I'm pretty sure they use different firmware though - the imx8mq additionally supports HDMI with a different firmware on the same IP core (13.4 and 13.5 in the imx8mq ref manual). > Perhaps Cadence can clarify the differences between IMX, TI and > Rockchip IPs and FWs? That would be great! -- Guido > I'm worried that if there are IP differences, even if not great ones, and if the FWs are different > and developed separately, it'll be a constant "fix X for SoC A, and accidentally break Y for SoC B > and C", especially if too much code is shared. > > In the long run I'm all for a single driver (or large shared parts), but I'm not sure if we should > start with that approach. > > Tomi > > -- > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki >
Hi Guido, > -----Original Message----- > From: Guido Günther <agx@sigxcpu.org> > Sent: Wednesday, August 12, 2020 7:27 PM > To: Tomi Valkeinen <tomi.valkeinen@ti.com> > Cc: Swapnil Kashinath Jakhade <sjakhade@cadence.com>; airlied@linux.ie; > daniel@ffwll.ch; Laurent.pinchart@ideasonboard.com; robh+dt@kernel.org; > a.hajda@samsung.com; narmstrong@baylibre.com; jonas@kwiboo.se; > jernej.skrabec@siol.net; dri-devel@lists.freedesktop.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Milind Parab > <mparab@cadence.com>; Yuti Suresh Amonkar <yamonkar@cadence.com>; > praneeth@ti.com; nsekhar@ti.com; jsarha@ti.com; sandor.yu@nxp.com > Subject: Re: [PATCH v8 0/3] drm: Add support for Cadence MHDP DPI/DP > bridge and J721E wrapper. > > EXTERNAL MAIL > > > Hi, > On Wed, Aug 12, 2020 at 01:47:42PM +0300, Tomi Valkeinen wrote: > > Hi Guido, > > > > On 12/08/2020 11:39, Guido Günther wrote: > > > Hi, > > > On Thu, Aug 06, 2020 at 01:34:29PM +0200, Swapnil Jakhade wrote: > > >> This patch series adds new DRM bridge driver for Cadence MHDP > > >> DPI/DP bridge. The Cadence Display Port IP is also referred as MHDP > > >> (Mobile High Definition Link, High-Definition Multimedia Interface, > Display Port). > > >> Cadence Display Port complies with VESA DisplayPort (DP) and > > >> embedded Display Port (eDP) standards. > > > > > > Is there any relation to the cadence mhdp ip core used inthe imx8mq: > > > > > > > > > https://urldefense.com/v3/__https://lore.kernel.org/dri-devel/cover. > > > > 1590982881.git.Sandor.yu@nxp.com/__;!!EHscmS1ygiU1lA!QIVUQ0JEY1Wz4 > gM > > > qV3HYGyyp5m4r_Fje6dL5ptUdhSzeqzzqBBR0Jo-BC9arK-g$ > > > > > > It looks very similar in several places so should that use the same driver? > > > Cheers, > > > -- Guido > > > > Interesting. > > > > So the original Cadence DP patches for TI SoCs did create a common > > driver with Rockchip's older mhdp driver. And looks like the IMX series > points to an early version of that patch ("drm/rockchip: > > prepare common code for cdns and rk dpi/dp driver"). > > > > We gave up on that as the IPs did have differences and the firmwares > > used were apparently quite different. The end result was very > > difficult to maintain, especially as (afaik) none of the people involved had > relevant Rockchip HW. > > Is the `struct mhdp_platform_ops` a leftover from that? Can that be > dropped? > > > The idea was to get a stable DP driver for TI SoCs ready and upstream, > > and then carefully try to create common parts with Rockchip's driver in > small pieces. > > I wonder how imx8 would best blend into this? First thing will likely be to > upstream the phy code in driveres/phy/ so a modified version of this bridge > driver could call into that, then go and look for common patterns. > > > If the Rockchip and IMX mhdp have the same IP and same firmware, then > > they obviously should share code as done in the series you point to. > > I'm pretty sure they use different firmware though - the imx8mq additionally > supports HDMI with a different firmware on the same IP core > (13.4 and 13.5 in the imx8mq ref manual). > > > Perhaps Cadence can clarify the differences between IMX, TI and > > Rockchip IPs and FWs? > > That would be great! > -- Guido > Following are the differences between MHDP IPs from Cadence for Rockchip, TI and NxP: The Rockchip and NXP MHDP Core shares the same part (IP8501) which is DP v1.3 SST Controller with HDCP 2.2/1.x. NXP's version additionally supports HDMI. TI uses a different part (IP8546A), which is DP v1.4 with HDCP 2.2/1.x. TI DP Controller adds support for additional features such as Multi Stream Support (MST), Forward Error Correction (FEC) and Compression (DSC). Also, FW used for TI has significant differences than FW used for Rockchip or NXP. NxP and TI firmware are developed and maintained separately by Cadence and are in active support. From the Linux driver perspective, given the differences, it would make sense to have TI driver maintained separately. Thanks, Swapnil > > > I'm worried that if there are IP differences, even if not great ones, > > and if the FWs are different and developed separately, it'll be a > > constant "fix X for SoC A, and accidentally break Y for SoC B and C", > especially if too much code is shared. > > > > In the long run I'm all for a single driver (or large shared parts), > > but I'm not sure if we should start with that approach. > > > > > > > > Tomi > > > > -- > > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > >
Hi Swapnil, On Mon, Aug 24, 2020 at 07:16:31AM +0000, Swapnil Kashinath Jakhade wrote: [..snip..] > Following are the differences between MHDP IPs from Cadence for Rockchip, TI and NxP: > > The Rockchip and NXP MHDP Core shares the same part (IP8501) which is DP v1.3 SST > Controller with HDCP 2.2/1.x. NXP's version additionally supports HDMI. > TI uses a different part (IP8546A), which is DP v1.4 with HDCP 2.2/1.x. > TI DP Controller adds support for additional features such as Multi Stream Support (MST), > Forward Error Correction (FEC) and Compression (DSC). > > Also, FW used for TI has significant differences than FW used for Rockchip or NXP. > NxP and TI firmware are developed and maintained separately by Cadence and are in > active support. > > From the Linux driver perspective, given the differences, it would make sense to have > TI driver maintained separately. Thanks for the clarification, that indeed helps a lot. So the rockchip and nxp drivers can be merged while the ti one should stay separate. Cheers, -- Guido > > Thanks, > Swapnil > > > > > > I'm worried that if there are IP differences, even if not great ones, > > > and if the FWs are different and developed separately, it'll be a > > > constant "fix X for SoC A, and accidentally break Y for SoC B and C", > > especially if too much code is shared. > > > > > > In the long run I'm all for a single driver (or large shared parts), > > > but I'm not sure if we should start with that approach. > > > > > > > > > > > > > > Tomi > > > > > > -- > > > Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. > > > Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki > > > >