From mboxrd@z Thu Jan 1 00:00:00 1970 From: Archit Taneja Subject: Re: [PATCH v2 1/2] drm/bridge: Add Cadence DSI driver Date: Wed, 14 Jun 2017 09:32:24 +0530 Message-ID: <37e95be1-1085-b71b-8532-69fbd4981673@codeaurora.org> References: <1496405096-18275-1-git-send-email-boris.brezillon@free-electrons.com> <29776d56-b6ae-9f92-0b94-8c55b46169fd@codeaurora.org> <20170606122053.604f9582@bbrezillon> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20170606122053.604f9582@bbrezillon> Content-Language: en-US Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Boris Brezillon Cc: dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org, David Airlie , Daniel Vetter , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Neil Webb , Richard Sproul , Simon Hatliff , Maxime Ripard , Thomas Petazzoni , Cyprian Wronka , Alan Douglas List-Id: devicetree@vger.kernel.org On 06/06/2017 03:50 PM, Boris Brezillon wrote: > Hi Archit, > > On Sun, 4 Jun 2017 00:20:06 +0530 > Archit Taneja wrote: > > >>> + >>> +#define DPHY_CFG0 0x1b0 >>> +#define DPHY_C_RSTB BIT(20) >>> +#define DPHY_D_RSTB(x) ((x) << 16) >>> +#define DPHY_TIF_FORCE_WRITE BIT(12) >>> +#define DPHY_PLL_PDN BIT(10) >>> +#define DPHY_CMN_PDN BIT(9) >>> +#define DPHY_C_PDN BIT(8) >>> +#define DPHY_D_PDN(x) ((x) << 4) >>> +#define DPHY_PLL_PSO BIT(1) >>> +#define DPHY_CMN_PSO BIT(0) >>> + >>> +#define DPHY_CFG1 0x1b4 >>> +#define PDHY_PLL_OPDIV(x) ((x) << 20) >>> +#define PDHY_PLL_IPDIV(x) ((x) << 12) >>> +#define PDHY_PLL_FBDIV(x) (x) >>> + >>> +#define DPHY_PLL_TM_LO 0x1b8 >>> +#define DPHY_PLL_TM_MID 0x1bc >>> +#define DPHY_PLL_TM_HI 0x1c0 >>> + >>> +#define DPHY_STATUS 0x1c4 >>> +#define PPI_D_RX_ULPS_ESC(x) ((x) >> 12) >>> +#define PPI_C_TX_READY_HS BIT(8) >>> +#define PPI_PLL_LOCK BIT(7) >>> +#define PPI_PLL_COARSE BIT(6) >>> +#define PPI_PLL_COARSE_CODE(x) ((x) & GENMASK(5, 0)) >>> + >>> +#define DPHY_BIST 0x1c8 >>> +#define PSO_BYPASS_CTX_EN BIT(12) >>> +#define PSO_BYPASS_TX_EN(l) BIT(8 + (l)) >>> +#define BIST_CTX_EN BIT(4) >>> +#define BIST_TX_EN(l) BIT(l) >>> + >> >> Do the above DPHY registers actually configure the PHY used with the >> controller, or do we need to configure any additional register outside >> of this block to get things working? > > The DPHY has a separate I/O mem range with its own interface. I didn't > provide support for this part yet because the interface is not stable > yet. > >> >> I ask because they aren't used in the code below, and the DT binding >> for this device specifies the PHY as a separate device. What's the >> plan in the future for PHY? > > The short-term plan is to add support for this DPHY in the cdns-dsi > driver. The driver will just retrieve the I/O mem range and interrupt > attached to the cdns DPHY block by following the phandle and using > of helpers to do the conversion, and then use these resources > internally. > > The mid/long-term plan is to add a dphy framework (on top of the PHY > framework) to let dphy providers expose their features in a generic > manner. > There are 2 pros to this generic DPHY framework/layer: > - you could possibly use DPHY and DSI blocks provided by 2 different > vendors (not sure how feasible this is in practice) > - CSI can also use DPHY as its PHY layer, so DPHY drivers could be > shared between V4L and DRM drivers > > Note that I can't promise the mid/long-term goals are achievable, > because my knowledge on DPHY is quite limited, but designing the DT > bindings to handle this use case is really important, because of the DT > stable ABI thing. Okay. There are patches to support the synopsis DW DSI bridge driver in the list, and the consensus there was that it isn't necessary to have a separate DPHY node for blocks which are tightly integrated with a SoC: https://patchwork.kernel.org/patch/9764395/ I was wondering if we should adopt to a standard way of dealing with the phys, i.e, let the SoC driver provide phy ops as platform data versus having a separate PHY driver altogether. In any case, I don't think there isn't harm having a PHY node in the bindings even if we go the other way. > > >>> + >>> +static int cdns_dsi_bridge_attach(struct drm_bridge *bridge) >>> +{ >>> + struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge); >>> + struct cdns_dsi_output *output = input->dsi->output; >>> + int ret; >>> + >>> + if (!drm_core_check_feature(bridge->dev, DRIVER_ATOMIC)) { >>> + dev_err(input->dsi->base.dev, >>> + "cdns-dsi driver is only compatible with DRM devices supporting atomic updates"); >>> + return -ENOTSUPP; >>> + } >>> + >>> + switch (output->type) { >>> + case CDNS_DSI_PANEL: >>> + ret = cdns_dsi_output_attach_panel(output); >>> + break; >>> + >>> + case CDNS_DSI_BRIDGE: >>> + ret = drm_bridge_attach(bridge->encoder, output->bridge, >>> + bridge); >>> + break; >> >> Could you have a look at Eric's dsi-panel-bridge patch-set? I think it >> should simplify things for this driver too. > > Yep, that was planned. Was just waiting for this feature to reach > drm-misc-next (which seems to be the case ;-)). > >> >>> + >>> + default: >>> + ret = -ENOTSUPP; >>> + } >>> + >>> + return ret; >>> +} >>> + >>> +static bool cdns_dsi_bridge_mode_fixup(struct drm_bridge *bridge, >>> + const struct drm_display_mode *mode, >>> + struct drm_display_mode *adj) >>> +{ >>> + struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge); >>> + int bpp; >>> + >>> + /* >>> + * VFP_DSI should be less than VFP_DPI and VFP_DSI should be at >>> + * least 1. >>> + */ >>> + if (adj->crtc_vtotal - adj->crtc_vsync_end < 2) >>> + return false; >>> + >>> + /* VSA_DSI = VSA_DPI and must be at least 2. */ >>> + if (adj->crtc_vsync_end - adj->crtc_vsync_start < 2) >>> + return false; >>> + >>> + /* HACT must be a 32-bits aligned. */ >>> + bpp = mipi_dsi_pixel_format_to_bpp(input->dsi->output->dev->format); >>> + if ((adj->hdisplay * bpp) % 32) >>> + return false; >>> + >>> + return true; >> >> We could consider using the new mode_valid helpers that Jose recently >> added. This might be better as bridge mode_valid() op. > > Ditto. It was on my TODO list. I'll address that in my v3. > >> >> >>> +} > > [...] > >>> + >>> +static int cdns_dsi_attach(struct mipi_dsi_host *host, >>> + struct mipi_dsi_device *dev) >>> +{ >>> + struct cdns_dsi *dsi = to_cdns_dsi(host); >>> + struct cdns_dsi_output *output; >>> + int ret; >>> + >>> + /* TODO: support multi-devices setup? */ >>> + if (dsi->output) >>> + return -EBUSY; >>> + >>> + output = devm_kzalloc(host->dev, sizeof(*output), GFP_KERNEL); >>> + if (!output) >>> + return -ENOMEM; >>> + >>> + output->dev = dev; >>> + >>> + output->panel = of_drm_find_panel(dev->dev.of_node); >>> + if (output->panel) { >>> + output->type = CDNS_DSI_PANEL; >>> + } else { >>> + output->bridge = of_drm_find_bridge(dev->dev.of_node); >>> + if (!output->bridge) { >>> + dev_err(host->dev, >>> + "%s is neither a panel nor a bridge", >>> + dev->name); >>> + return -ENOTSUPP; >>> + } >>> + >>> + output->type = CDNS_DSI_BRIDGE; >>> + dsi->input->bridge.next = output->bridge; >>> + } >>> + >>> + dsi->output = output; >>> + >>> + ret = cdns_dsi_init_link(dsi); >>> + if (ret) >>> + return ret; >>> + >>> + /* FIXME: should be moved somewhere else. */ >>> + return drm_bridge_add(&dsi->input->bridge); >> >> In the driver probe? > > I assumed that not everything was in place at probe time, but maybe I > was wrong. This should be good, as long as all DSI devices have been > instantiated and attached after calling mipi_dsi_host_register(). I > guess this is something we can ensure by using the proposed bindings > (with all DSI output ports described in the DT) and returning > EPROBE_DEFER if at least one of the DSI device is missing at probe time. > >> >>> +} >>> + > > [...] > >>> + >>> +static int cdns_dsi_drm_probe(struct platform_device *pdev) >> >> I wanted to bring up the point I made in the DT patch too: Is it >> more suitable to implement this is a library with bind/unbind, and >> probe/remove helpers like it's done for dw-hdmi. Could there be SoCs >> that integrate this IP but require some additional wrapper >> configurations to make things work? > > As answered in my previous reply, yes it can be done this way, but is it > worth introducing this infrastructure right now? Note that we can still > overload the compatible to support SoC specific integration of this IP > directly in this driver. > For the rest, it should be pretty transparent to connect a DPI encoder > to this DSI bridge. Okay. Sounds good to me. Thanks, Archit > > Right now, we have an easy way to connect a DPI encoder to this DSI > bridge. The only thing that could be missing is the pixel format > negotiation (the format transiting on the DPI bus), and this is a > runtime thing, so maybe we can extend the drm_bridge interface to allow > such negotiation. > > Note that this is a need I had on atmel platforms as well, because the > DPI bus can be connected to several devices (panels or external > bridges), and, depending on the device we are enabling in the pipeline, > we have to reconfigure the DPI encoder to send the pixel in the > appropriate format. > >> >> Looks good otherwise. > > Thanks for your review. > > Boris > -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html