lkml.org 
[lkml]   [2020]   [May]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC PATCH 2/2] drm: xlnx: driver for Xilinx DSI TX Subsystem
    Hi Laurent,

    On Sat, 2020-05-23 at 20:08:13 -0700, Laurent Pinchart wrote:
    > Hi GVRao,
    >
    > Thank you for the patch.
    >
    > 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.

    > > > 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@xilinx.com>
    > > > ---
    > > > 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@xilinx.com>
    > > > + * - Venkateshwar Rao Gannavarapu <venkateshwar.rao.gannavarapu@xilinx.com>
    > > > + */
    > > > +
    > > > +#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. :-)

    Thanks,
    -hyun

    \
     
     \ /
      Last update: 2020-05-27 19:55    [W:5.346 / U:0.140 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site