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

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart