Re: [RFC PATCH 2/2] drm: xlnx: driver for Xilinx DSI TX Subsystem

[Hyun Kwon]

Hi Laurent,

On Wed, 2020-05-27 at 15:45:24 -0700, Laurent Pinchart wrote:
> Hi Hyun,
> 
> On Wed, May 27, 2020 at 10:54:35AM -0700, Hyun Kwon wrote:
> > On Sat, 2020-05-23 at 20:08:13 -0700, Laurent Pinchart wrote:
> > > On Mon, May 04, 2020 at 11:43:48AM -0700, Hyun Kwon wrote:
> > >> On Mon, 2020-04-20 at 14:20:56 -0700, Venkateshwar Rao Gannavarapu wrote:
> > >>> The Xilinx MIPI DSI Tx Subsystem soft IP is used to display video
> > >>> data from AXI-4 stream interface.
> > >>> 
> > >>> It supports upto 4 lanes, optional register interface for the DPHY,
> > >> 
> > >> I don't see the register interface for dphy support.
> > > 
> > > I think the D-PHY should be supported through a PHY driver, as it seems
> > > to be shared between different subsystems.
> > 
> > Right, if the logic is shared across subsystems. I can't tell if that's
> > the case as the IP comes as a single block. Maybe GVRao can confirm.
> 
> I believe the CSI2-RX subsystem uses the same D-PHY IP core, but a
> confirmation would be nice.
> 
> > >>> multiple RGB color formats, command mode and video mode.
> > >>> This is a MIPI-DSI host driver and provides DSI bus for panels.
> > >>> This driver also helps to communicate with its panel using panel
> > >>> framework.
> > >>> 
> > >>> Signed-off-by: Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xxxxxxxxxx>
> > >>> ---
> > >>>  drivers/gpu/drm/xlnx/Kconfig    |  11 +
> > >>>  drivers/gpu/drm/xlnx/Makefile   |   2 +
> > >>>  drivers/gpu/drm/xlnx/xlnx_dsi.c | 755 ++++++++++++++++++++++++++++++++++++++++
> > > 
> > > Daniel Vetter has recently expressed his opiion that bridge drivers
> > > should go to drivers/gpu/drm/bridge/. It would then be
> > > drivers/gpu/drm/bridge/xlnx/. I don't have a strong opinion myself.
> > > 
> > >>>  3 files changed, 768 insertions(+)
> > >>>  create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c
> > >>> 
> > >>> diff --git a/drivers/gpu/drm/xlnx/Kconfig b/drivers/gpu/drm/xlnx/Kconfig
> > >>> index aa6cd88..73873cf 100644
> > >>> --- a/drivers/gpu/drm/xlnx/Kconfig
> > >>> +++ b/drivers/gpu/drm/xlnx/Kconfig
> > >>> @@ -11,3 +11,14 @@ config DRM_ZYNQMP_DPSUB
> > >>>  	  This is a DRM/KMS driver for ZynqMP DisplayPort controller. Choose
> > >>>  	  this option if you have a Xilinx ZynqMP SoC with DisplayPort
> > >>>  	  subsystem.
> > >>> +
> > >>> +config DRM_XLNX_DSI
> > >>> +        tristate "Xilinx DRM DSI Subsystem Driver"
> > >>> +        select DRM_MIPI_DSI
> > >>> +        select DRM_PANEL
> > >>> +        select DRM_PANEL_SIMPLE
> > >>> +        help
> > >>> +	  This enables support for Xilinx MIPI-DSI.
> > >> 
> > >> This sentence is not needed with below. Could you please rephrase the whole?
> > >> 
> > >>> +	  This is a DRM/KMS driver for Xilinx programmable DSI controller.
> > >>> +	  Choose this option if you have a Xilinx MIPI DSI-TX controller
> > >>> +	  subsytem.
> > >> 
> > >> These seem incorrectly indented.
> > >> 
> > >>> diff --git a/drivers/gpu/drm/xlnx/Makefile b/drivers/gpu/drm/xlnx/Makefile
> > >>> index 2b844c6..b7ee6ef 100644
> > >>> --- a/drivers/gpu/drm/xlnx/Makefile
> > >>> +++ b/drivers/gpu/drm/xlnx/Makefile
> > >>> @@ -1,2 +1,4 @@
> > >>>  zynqmp-dpsub-objs += zynqmp_disp.o zynqmp_dpsub.o zynqmp_dp.o
> > >>>  obj-$(CONFIG_DRM_ZYNQMP_DPSUB) += zynqmp-dpsub.o
> > >>> +
> > >>> +obj-$(CONFIG_DRM_XLNX_DSI) += xlnx_dsi.o
> > >>> diff --git a/drivers/gpu/drm/xlnx/xlnx_dsi.c b/drivers/gpu/drm/xlnx/xlnx_dsi.c
> > >>> new file mode 100644
> > >>> index 0000000..b8cae59
> > >>> --- /dev/null
> > >>> +++ b/drivers/gpu/drm/xlnx/xlnx_dsi.c
> > >>> @@ -0,0 +1,755 @@
> > >>> +// SPDX-License-Identifier: GPL-2.0
> > >>> +/*
> > >>> + * Xilinx FPGA MIPI DSI Tx Controller driver
> > >>> + *
> > >>> + * Copyright (C) 2017 - 2019 Xilinx, Inc.
> > >>> + *
> > >>> + * Authors:
> > >>> + * - Saurabh Sengar <saurabhs@xxxxxxxxxx>
> > >>> + * - Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xxxxxxxxxx>
> > >>> + */
> > >>> +
> > >>> +#include <drm/drm_atomic_helper.h>
> > >>> +#include <drm/drm_connector.h>
> > >>> +#include <drm/drm_crtc.h>
> > >>> +#include <drm/drm_crtc_helper.h>
> > >>> +#include <drm/drm_device.h>
> > >>> +#include <drm/drm_encoder.h>
> > >>> +#include <drm/drm_fourcc.h>
> > >>> +#include <drm/drm_gem_cma_helper.h>
> > >>> +#include <drm/drm_mipi_dsi.h>
> > >>> +#include <drm/drm_panel.h>
> > >>> +#include <drm/drm_probe_helper.h>
> > >>> +
> > >>> +#include <linux/clk.h>
> > >>> +#include <linux/component.h>
> > >>> +#include <linux/device.h>
> > >>> +#include <linux/iopoll.h>
> > >>> +#include <linux/of_device.h>
> > >>> +#include <linux/of_graph.h>
> > >>> +#include <linux/phy/phy.h>
> > >>> +
> > >>> +#include <video/mipi_display.h>
> > >>> +#include <video/videomode.h>
> > >>> +
> > >>> +/* DSI Tx IP registers */
> > >>> +#define XDSI_CCR			0x00
> > >>> +#define XDSI_CCR_COREENB		BIT(0)
> > >>> +#define XDSI_CCR_SOFTRST		BIT(1)
> > >>> +#define XDSI_CCR_CRREADY		BIT(2)
> > >>> +#define XDSI_CCR_CMDMODE		BIT(3)
> > >>> +#define XDSI_CCR_DFIFORST		BIT(4)
> > >>> +#define XDSI_CCR_CMDFIFORST		BIT(5)
> > >>> +#define XDSI_PCR			0x04
> > 
> > [snip]
> > 
> > >>> +	}
> > >>> +
> > >>> +	ret = clk_prepare_enable(dsi->video_aclk);
> > >>> +	if (ret) {
> > >>> +		dev_err(dev, "failed to enable video clk %d\n", ret);
> > >>> +		goto err_disable_dphy_clk;
> > >>> +	}
> > >>> +
> > >>> +	ret = component_add(dev, &xlnx_dsi_component_ops);
> > > 
> > > The driver should expose the DSI-TX as a drm_bridge instead of using the
> > > component framework. You shouldn't register a drm_encoder, and I don't
> > > think you should register a drm_connector either. Only bridge operations
> > > should be exposed, and the drm_bridge .attach() operation should return
> > > an error when DRM_BRIDGE_ATTACH_NO_CONNECTOR is not set. The top-level
> > > driver using this bridge should create the drm_encoder and
> > > drm_connector, most likely using drm_bridge_connector_init() to create
> > > the connector.
> > 
> > Not clear to me if this has to be a bridge, and then what it will be attached
> > to. The IP block itself pretty much self-contains all functionalities already,
> > just like any other drm encoder / connector, so it doesn't have to be wrapped
> > around by any other layer. Please let me know your thought, so I can understand
> > better. :-)
> 
> The DSI output will likely often be connected to a DSI panel, but it
> could also be connected to another bridge, for instance to an ADV7533
> DSI-to-HDMI bridge. In that case an HDMI connector needs to be created,
> not a DSI connector. This is why we are moving towards a model where
> bridge drivers only handle the bridge device, and the drm_encoder and
> drm_connector is created externally, but the display controller driver.
> The drm_bridge_connector_init() helper can automate connector creation
> for a chain of bridges.

Ah I see. Thanks for explanation. It seems relatively new changes and more
scalable. The above case is less likely with FPGA pipeline. But right, it can
still happen in certain cases, ex with 96 boards that have DSI interface only,
and that will help. So I believe encoder / connector drivers (there are more
in Xilinx tree) need to be converted to bridges, with a new top level driver
handling the bridges.

Thanks,
-hyun