Received: by 2002:a25:b323:0:0:0:0:0 with SMTP id l35csp565070ybj; Thu, 19 Sep 2019 19:20:13 -0700 (PDT) X-Google-Smtp-Source: APXvYqz6AErrjhXiz1x3iTyQ3L+cn2ZNr5xobZZZadi/alPS4JzdAIOjW01fbXtP4TIU6mCU+9QF X-Received: by 2002:a05:6402:17eb:: with SMTP id t11mr18949830edy.97.1568946013120; Thu, 19 Sep 2019 19:20:13 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1568946013; cv=none; d=google.com; s=arc-20160816; b=DzsmBGcUCVX0S7+YyxIthQ/RIXtzOc9yM5hw6O9fcS+eyl68kXac2Z1SWBe4y0fEoA NXbvCO67DbHFWvPJC4Nrbz0RzEokZIW3Ct6oMJPJfR3AbqTOrTIrtrYn+9/0xV18IktJ WfoYOLkCk/04f9jr2n+v7OzbTr5PIxCxqi4k6a2JwXsftLRB8kld3OLkTrrtVYTvlxoY hhOYpajopuxK22kGniJACrurdS2i95uejaDviADNbsn8l4JF6NhnmS4A4L132zqcT9Tk VuVGAyhdgfVRzW8iMMCSxB37eHQe+eDFYWYLgeiK6SjtAWZzQzyOxzQEJKyl5x8EwtH0 7yDA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:user-agent:in-reply-to :content-transfer-encoding:content-disposition:mime-version :references:message-id:subject:cc:to:from:date; bh=KPGM9I006o8B+ubEV96/93VVXd5qDVxAfmw2rCOu6rI=; b=jvtz+0f2M6xTEoOctd0jzXIpi+KU7oB+kZSi8XQXB8nR0l0Ukoekgvy1JVcO5w400F Pzb339827eSt5uK7lN6oNApbutJQrIflW3+26H+FI+Uw7eOfISbJrlghg8KNOHAQo1Zo cw1z+yE1JCiBHiAM5jXnPqUpwTiY+e8/cFdCZLf2ITG8+A4KT3Nn0T3//fIs82ImuMlr +z2WIJ7olm5lPUtYkmpEbyLxcxG/mp+byiHYkAREK4a5teYiUy0Qdcd6ryHq4LkbAwYP ArtgWlAGQxKflltS/W7xACberZYPgc7q9gRQV8g4aizfn4KpDYtZT8PmfbaBpgRWt7Fy EVMA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id i9si280367ejy.106.2019.09.19.19.19.40; Thu, 19 Sep 2019 19:20:13 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2403961AbfISRn1 (ORCPT + 99 others); Thu, 19 Sep 2019 13:43:27 -0400 Received: from honk.sigxcpu.org ([24.134.29.49]:55252 "EHLO honk.sigxcpu.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2388143AbfISRn0 (ORCPT ); Thu, 19 Sep 2019 13:43:26 -0400 Received: from localhost (localhost [127.0.0.1]) by honk.sigxcpu.org (Postfix) with ESMTP id 092A7FB03; Thu, 19 Sep 2019 19:43:23 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at honk.sigxcpu.org Received: from honk.sigxcpu.org ([127.0.0.1]) by localhost (honk.sigxcpu.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id bpJj9CkIGJ-E; Thu, 19 Sep 2019 19:43:17 +0200 (CEST) Received: by bogon.sigxcpu.org (Postfix, from userid 1000) id DBCD148764; Thu, 19 Sep 2019 10:43:13 -0700 (PDT) Date: Thu, 19 Sep 2019 10:43:13 -0700 From: Guido =?iso-8859-1?Q?G=FCnther?= To: Andrzej Hajda Cc: David Airlie , Daniel Vetter , Rob Herring , Mark Rutland , Shawn Guo , Sascha Hauer , Pengutronix Kernel Team , Fabio Estevam , NXP Linux Team , Neil Armstrong , Laurent Pinchart , Jonas Karlman , Jernej Skrabec , Lee Jones , dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Robert Chiras , Sam Ravnborg , Arnd Bergmann Subject: Re: [PATCH v5 2/2] drm/bridge: Add NWL MIPI DSI host controller support Message-ID: <20190919174313.GA5388@bogon.m.sigxcpu.org> References: <3ce1891ea41249bf4a9985e2cee8640fb36de42e.1567995854.git.agx@sigxcpu.org> <20190914161155.GA2933@bogon.m.sigxcpu.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Andrzej, thanks for your comments! On Thu, Sep 19, 2019 at 03:56:45PM +0200, Andrzej Hajda wrote: > On 14.09.2019 18:11, Guido G?nther wrote: > > Hi Andrzej, > > thanks for having a look! > > > > On Fri, Sep 13, 2019 at 11:31:43AM +0200, Andrzej Hajda wrote: > >> On 09.09.2019 04:25, Guido G?nther wrote: > >>> This adds initial support for the NWL MIPI DSI Host controller found on > >>> i.MX8 SoCs. > >>> > >>> It adds support for the i.MX8MQ but the same IP can be found on > >>> e.g. the i.MX8QXP. > >>> > >>> It has been tested on the Librem 5 devkit using mxsfb. > >>> > >>> Signed-off-by: Guido G?nther > >>> Co-developed-by: Robert Chiras > >>> Signed-off-by: Robert Chiras > >>> Tested-by: Robert Chiras > >>> --- > >>> drivers/gpu/drm/bridge/Kconfig | 2 + > >>> drivers/gpu/drm/bridge/Makefile | 1 + > >>> drivers/gpu/drm/bridge/nwl-dsi/Kconfig | 16 + > >>> drivers/gpu/drm/bridge/nwl-dsi/Makefile | 4 + > >>> drivers/gpu/drm/bridge/nwl-dsi/nwl-drv.c | 499 ++++++++++++++++ > >>> drivers/gpu/drm/bridge/nwl-dsi/nwl-drv.h | 65 +++ > >>> drivers/gpu/drm/bridge/nwl-dsi/nwl-dsi.c | 696 +++++++++++++++++++++++ > >>> drivers/gpu/drm/bridge/nwl-dsi/nwl-dsi.h | 112 ++++ > >> > >> Why do you need separate files nwl-drv.[ch] and nwl-dsi.[ch] ? I guess > >> you can merge all into one file, maybe with separate file for NWL > >> register definitions. > > Idea is to have driver setup, soc specific hooks and revision specific > > quirks in one file and the dsi specific parts in another. If that > > doesn't fly I can merge into one if that's a requirement. > > > One file looks saner to me :), but more importantly it follows current > practice. O.k., I'll merge things for v6 then. > > > > > >>> 8 files changed, 1395 insertions(+) > >>> create mode 100644 drivers/gpu/drm/bridge/nwl-dsi/Kconfig > >>> create mode 100644 drivers/gpu/drm/bridge/nwl-dsi/Makefile > >>> create mode 100644 drivers/gpu/drm/bridge/nwl-dsi/nwl-drv.c > >>> create mode 100644 drivers/gpu/drm/bridge/nwl-dsi/nwl-drv.h > >>> create mode 100644 drivers/gpu/drm/bridge/nwl-dsi/nwl-dsi.c > >>> create mode 100644 drivers/gpu/drm/bridge/nwl-dsi/nwl-dsi.h > >>> > >>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig > >>> index 1cc9f502c1f2..7980b5c2156f 100644 > >>> --- a/drivers/gpu/drm/bridge/Kconfig > >>> +++ b/drivers/gpu/drm/bridge/Kconfig > >>> @@ -154,6 +154,8 @@ source "drivers/gpu/drm/bridge/analogix/Kconfig" > >>> > >>> source "drivers/gpu/drm/bridge/adv7511/Kconfig" > >>> > >>> +source "drivers/gpu/drm/bridge/nwl-dsi/Kconfig" > >>> + > >>> source "drivers/gpu/drm/bridge/synopsys/Kconfig" > >>> > >>> endmenu > >>> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > >>> index 4934fcf5a6f8..d9f6c0f77592 100644 > >>> --- a/drivers/gpu/drm/bridge/Makefile > >>> +++ b/drivers/gpu/drm/bridge/Makefile > >>> @@ -16,4 +16,5 @@ obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/ > >>> obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/ > >>> obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o > >>> obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o > >>> +obj-$(CONFIG_DRM_NWL_MIPI_DSI) += nwl-dsi/ > >>> obj-y += synopsys/ > >>> diff --git a/drivers/gpu/drm/bridge/nwl-dsi/Kconfig b/drivers/gpu/drm/bridge/nwl-dsi/Kconfig > >>> new file mode 100644 > >>> index 000000000000..7fa678e3b5e2 > >>> --- /dev/null > >>> +++ b/drivers/gpu/drm/bridge/nwl-dsi/Kconfig > >>> @@ -0,0 +1,16 @@ > >>> +config DRM_NWL_MIPI_DSI > >>> + tristate "Northwest Logic MIPI DSI Host controller" > >>> + depends on DRM > >>> + depends on COMMON_CLK > >>> + depends on OF && HAS_IOMEM > >>> + select DRM_KMS_HELPER > >>> + select DRM_MIPI_DSI > >>> + select DRM_PANEL_BRIDGE > >>> + select GENERIC_PHY_MIPI_DPHY > >>> + select MFD_SYSCON > >>> + select MULTIPLEXER > >>> + select REGMAP_MMIO > >>> + help > >>> + This enables the Northwest Logic MIPI DSI Host controller as > >>> + for example found on NXP's i.MX8 Processors. > >>> + > >>> diff --git a/drivers/gpu/drm/bridge/nwl-dsi/Makefile b/drivers/gpu/drm/bridge/nwl-dsi/Makefile > >>> new file mode 100644 > >>> index 000000000000..804baf2f1916 > >>> --- /dev/null > >>> +++ b/drivers/gpu/drm/bridge/nwl-dsi/Makefile > >>> @@ -0,0 +1,4 @@ > >>> +# SPDX-License-Identifier: GPL-2.0 > >>> +nwl-mipi-dsi-y := nwl-drv.o nwl-dsi.o > >>> +obj-$(CONFIG_DRM_NWL_MIPI_DSI) += nwl-mipi-dsi.o > >>> +header-test-y += nwl-drv.h nwl-dsi.h > >>> diff --git a/drivers/gpu/drm/bridge/nwl-dsi/nwl-drv.c b/drivers/gpu/drm/bridge/nwl-dsi/nwl-drv.c > >>> new file mode 100644 > >>> index 000000000000..9ff43d2de127 > >>> --- /dev/null > >>> +++ b/drivers/gpu/drm/bridge/nwl-dsi/nwl-drv.c > >>> @@ -0,0 +1,499 @@ > >>> +// SPDX-License-Identifier: GPL-2.0+ > >>> +/* > >>> + * i.MX8 NWL MIPI DSI host driver > >>> + * > >>> + * Copyright (C) 2017 NXP > >>> + * Copyright (C) 2019 Purism SPC > >>> + */ > >>> + > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >> > >> Alphabetic order > > Fixed for v6. > > > >>> +#include > >>> + > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> + > >>> +#include "nwl-drv.h" > >>> +#include "nwl-dsi.h" > >>> + > >>> +#define DRV_NAME "nwl-dsi" > >>> + > >>> +/* Possible platform specific clocks */ > >>> +#define NWL_DSI_CLK_CORE "core" > >>> + > >>> +static const struct regmap_config nwl_dsi_regmap_config = { > >>> + .reg_bits = 16, > >>> + .val_bits = 32, > >>> + .reg_stride = 4, > >>> + .max_register = NWL_DSI_IRQ_MASK2, > >>> + .name = DRV_NAME, > >>> +}; > >> > >> What is the point in using regmap here, why not simple writel/readl. > > For me > > > > cat /sys/kernel/debug/regmap/30a00000.mipi_dsi-imx-nwl-dsi/registers > > > > justifies it's use to help debugging problems when e.g. having it > > connected to panels I don't own, so I think it's worth keeping if > > possible. > > > It still sounds for me like a sledgehammer to crack a nut, but it seems > for many developers it is OK, so up to you. > > > > > >>> + > >>> +struct nwl_dsi_platform_data { > >>> + int (*poweron)(struct nwl_dsi *dsi); > >>> + int (*poweroff)(struct nwl_dsi *dsi); > >>> + int (*select_input)(struct nwl_dsi *dsi); > >>> + int (*deselect_input)(struct nwl_dsi *dsi); > >>> + struct nwl_dsi_plat_clk_config clk_config[NWL_DSI_MAX_PLATFORM_CLOCKS]; > >>> +}; > >> > >> Another construct which do not have justification, at least for now. > >> Please simplify the driver, remove callbacks/intermediate > >> structs/quirks > >> > >> - for now they are useless. > >> > >> Unless there is a serious reason - in such case please describe it in > >> comments. > > They're needed for i.mx 8QM SoC support (the current driver only > > supports the i.mx 8MQ). It will be relatively easy to add with > > these so I expect these to show up quickly. I'll add a comment. > > > That does not work well, it is impossible to review such code without > looking at it's usage. > > It would be better either to add 8QM driver in the same patchset showing > the value of these callbacks, either remove them and add later together > with 8QM driver. > > Maybe these callbacks are not needed at all, but without 8QM code it is > hard to decide. I agree that it's hard to plan in advance until patches show up but let me try to make a last argument for keeping them: - The dsi select into imx8mq_dsi_select_input was suggested by Laurent since they're imx8 specific - This would only leave poweron/off which is different between imx8mq and imx8qm: https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/gpu/drm/imx/nwl_dsi-imx.c?h=imx_4.14.78_1.0.0_ga#n419 if that's still not worth it, i can drop them making the driver smaller. > Btw. naming different SoCs 8QM an 8MQ suggests i.mx engineers are quite > nasty :) It sure is. > > The quirks on the other hand only apply to some i.mx8MQ mask revisions > > so they need to be conditionalized. (or maybe I misunderstood you). > > > I guess at the moment the driver is tested only on one platform - you > have not tested platforms with different set of quirks, am I correct? I have tested with two silicon mask revisions where one needs the quirks and the other doesn't and it behaved as intended. > If so, the quirk 'infrastructure' is not really tested, driver just > anticipates other 8MQ SoCs/platforms. Practice shows that it does not > work well - adding support for different revision usually require either > more changes either no changes at all, so this pre-mature > "diversification" serves nothing except unnecessary noise. Does the above make the diversification o.k.? > >>> + > >>> +static inline struct nwl_dsi *bridge_to_dsi(struct drm_bridge *bridge) > >>> +{ > >>> + return container_of(bridge, struct nwl_dsi, bridge); > >>> +} > >>> + > >>> +static int nwl_dsi_set_platform_clocks(struct nwl_dsi *dsi, bool enable) > >>> +{ > >>> + struct device *dev = dsi->dev; > >>> + const char *id; > >>> + struct clk *clk; > >>> + size_t i; > >>> + unsigned long rate; > >>> + int ret, result = 0; > >>> + > >>> + DRM_DEV_DEBUG_DRIVER(dev, "%s platform clocks\n", > >>> + enable ? "enabling" : "disabling"); > >>> + for (i = 0; i < ARRAY_SIZE(dsi->pdata->clk_config); i++) { > >>> + if (!dsi->clk_config[i].present) > >>> + continue; > >>> + id = dsi->clk_config[i].id; > >>> + clk = dsi->clk_config[i].clk; > >>> + > >>> + if (enable) { > >>> + ret = clk_prepare_enable(clk); > >>> + if (ret < 0) { > >>> + DRM_DEV_ERROR(dev, > >>> + "Failed to enable %s clk: %d\n", > >>> + id, ret); > >>> + result = result ?: ret; > >>> + } > >>> + rate = clk_get_rate(clk); > >>> + DRM_DEV_DEBUG_DRIVER(dev, "Enabled %s clk @%lu Hz\n", > >>> + id, rate); > >>> + } else { > >>> + clk_disable_unprepare(clk); > >>> + DRM_DEV_DEBUG_DRIVER(dev, "Disabled %s clk\n", id); > >>> + } > >>> + } > >>> + > >>> + return result; > >>> +} > >>> + > >>> +static int nwl_dsi_plat_enable(struct nwl_dsi *dsi) > >>> +{ > >>> + struct device *dev = dsi->dev; > >>> + int ret; > >>> + > >>> + if (dsi->pdata->select_input) > >>> + dsi->pdata->select_input(dsi); > >>> + > >>> + ret = nwl_dsi_set_platform_clocks(dsi, true); > >>> + if (ret < 0) > >>> + return ret; > >>> + > >>> + ret = dsi->pdata->poweron(dsi); > >>> + if (ret < 0) > >>> + DRM_DEV_ERROR(dev, "Failed to power on DSI: %d\n", ret); > >>> + return ret; > >>> +} > >>> + > >>> +static void nwl_dsi_plat_disable(struct nwl_dsi *dsi) > >>> +{ > >>> + dsi->pdata->poweroff(dsi); > >>> + nwl_dsi_set_platform_clocks(dsi, false); > >>> + if (dsi->pdata->deselect_input) > >>> + dsi->pdata->deselect_input(dsi); > >>> +} > >>> + > >>> +static void nwl_dsi_bridge_disable(struct drm_bridge *bridge) > >>> +{ > >>> + struct nwl_dsi *dsi = bridge_to_dsi(bridge); > >>> + > >>> + nwl_dsi_disable(dsi); > >>> + nwl_dsi_plat_disable(dsi); > >>> + pm_runtime_put(dsi->dev); > >>> +} > >>> + > >>> +static int nwl_dsi_get_dphy_params(struct nwl_dsi *dsi, > >>> + const struct drm_display_mode *mode, > >>> + union phy_configure_opts *phy_opts) > >>> +{ > >>> + unsigned long rate; > >>> + int ret; > >>> + > >>> + if (dsi->lanes < 1 || dsi->lanes > 4) > >>> + return -EINVAL; > >>> + > >>> + /* > >>> + * So far the DPHY spec minimal timings work for both mixel > >>> + * dphy and nwl dsi host > >>> + */ > >>> + ret = phy_mipi_dphy_get_default_config( > >>> + mode->crtc_clock * 1000, > >>> + mipi_dsi_pixel_format_to_bpp(dsi->format), dsi->lanes, > >>> + &phy_opts->mipi_dphy); > >>> + if (ret < 0) > >>> + return ret; > >>> + > >>> + rate = clk_get_rate(dsi->tx_esc_clk); > >>> + DRM_DEV_DEBUG_DRIVER(dsi->dev, "LP clk is @%lu Hz\n", rate); > >>> + phy_opts->mipi_dphy.lp_clk_rate = rate; > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static bool nwl_dsi_bridge_mode_fixup(struct drm_bridge *bridge, > >>> + const struct drm_display_mode *mode, > >>> + struct drm_display_mode *adjusted_mode) > >>> +{ > >>> + /* At least LCDIF + NWL needs active high sync */ > >>> + adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC); > >>> + adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC); > >>> + > >>> + return true; > >>> +} > >>> + > >>> +static enum drm_mode_status > >>> +nwl_dsi_bridge_mode_valid(struct drm_bridge *bridge, > >>> + const struct drm_display_mode *mode) > >>> +{ > >>> + struct nwl_dsi *dsi = bridge_to_dsi(bridge); > >>> + int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format); > >>> + > >>> + if (mode->clock * bpp > 15000000 * dsi->lanes) > >>> + return MODE_CLOCK_HIGH; > >>> + > >>> + if (mode->clock * bpp < 80000 * dsi->lanes) > >>> + return MODE_CLOCK_LOW; > >>> + > >>> + return MODE_OK; > >>> +} > >>> + > >>> +static void > >>> +nwl_dsi_bridge_mode_set(struct drm_bridge *bridge, > >>> + const struct drm_display_mode *mode, > >>> + const struct drm_display_mode *adjusted_mode) > >>> +{ > >>> + struct nwl_dsi *dsi = bridge_to_dsi(bridge); > >>> + struct device *dev = dsi->dev; > >>> + union phy_configure_opts new_cfg; > >>> + unsigned long phy_ref_rate; > >>> + int ret; > >>> + > >>> + ret = nwl_dsi_get_dphy_params(dsi, adjusted_mode, &new_cfg); > >>> + if (ret < 0) > >>> + return; > >>> + > >>> + /* > >>> + * If hs clock is unchanged, we're all good - all parameters are > >>> + * derived from it atm. > >>> + */ > >>> + if (new_cfg.mipi_dphy.hs_clk_rate == dsi->phy_cfg.mipi_dphy.hs_clk_rate) > >>> + return; > >>> + > >>> + phy_ref_rate = clk_get_rate(dsi->phy_ref_clk); > >>> + DRM_DEV_DEBUG_DRIVER(dev, "PHY at ref rate: %lu\n", phy_ref_rate); > >>> + /* Save the new desired phy config */ > >>> + memcpy(&dsi->phy_cfg, &new_cfg, sizeof(new_cfg)); > >>> + > >>> + memcpy(&dsi->mode, adjusted_mode, sizeof(dsi->mode)); > >>> + drm_mode_debug_printmodeline(adjusted_mode); > >>> +} > >>> + > >>> +static void nwl_dsi_bridge_pre_enable(struct drm_bridge *bridge) > >>> +{ > >>> + struct nwl_dsi *dsi = bridge_to_dsi(bridge); > >>> + > >>> + pm_runtime_get_sync(dsi->dev); > >>> + nwl_dsi_plat_enable(dsi); > >>> + nwl_dsi_enable(dsi); > >>> +} > >>> + > >>> +static int nwl_dsi_bridge_attach(struct drm_bridge *bridge) > >>> +{ > >>> + struct nwl_dsi *dsi = bridge->driver_private; > >>> + > >>> + return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, bridge); > >>> +} > >>> + > >>> +static const struct drm_bridge_funcs nwl_dsi_bridge_funcs = { > >>> + .pre_enable = nwl_dsi_bridge_pre_enable, > >>> + .disable = nwl_dsi_bridge_disable, > >>> + .mode_fixup = nwl_dsi_bridge_mode_fixup, > >>> + .mode_set = nwl_dsi_bridge_mode_set, > >>> + .mode_valid = nwl_dsi_bridge_mode_valid, > >>> + .attach = nwl_dsi_bridge_attach, > >>> +}; > >>> + > >>> +static int nwl_dsi_parse_dt(struct nwl_dsi *dsi) > >>> +{ > >>> + struct platform_device *pdev = to_platform_device(dsi->dev); > >>> + struct clk *clk; > >>> + const char *clk_id; > >>> + void __iomem *base; > >>> + int i, ret; > >>> + > >>> + dsi->phy = devm_phy_get(dsi->dev, "dphy"); > >>> + if (IS_ERR(dsi->phy)) { > >>> + ret = PTR_ERR(dsi->phy); > >>> + if (ret != -EPROBE_DEFER) > >>> + DRM_DEV_ERROR(dsi->dev, "Could not get PHY: %d\n", ret); > >>> + return ret; > >>> + } > >>> + > >>> + /* Platform dependent clocks */ > >>> + memcpy(dsi->clk_config, dsi->pdata->clk_config, > >>> + sizeof(dsi->pdata->clk_config)); > >>> + > >>> + for (i = 0; i < ARRAY_SIZE(dsi->pdata->clk_config); i++) { > >>> + if (!dsi->clk_config[i].present) > >>> + continue; > >>> + > >>> + clk_id = dsi->clk_config[i].id; > >>> + clk = devm_clk_get(dsi->dev, clk_id); > >>> + if (IS_ERR(clk)) { > >>> + ret = PTR_ERR(clk); > >>> + DRM_DEV_ERROR(dsi->dev, "Failed to get %s clock: %d\n", > >>> + clk_id, ret); > >>> + return ret; > >>> + } > >>> + DRM_DEV_DEBUG_DRIVER(dsi->dev, "Setup clk %s (rate: %lu)\n", > >>> + clk_id, clk_get_rate(clk)); > >>> + dsi->clk_config[i].clk = clk; > >>> + } > >>> + > >>> + /* DSI clocks */ > >>> + clk = devm_clk_get(dsi->dev, "phy_ref"); > >>> + if (IS_ERR(clk)) { > >>> + ret = PTR_ERR(clk); > >>> + DRM_DEV_ERROR(dsi->dev, "Failed to get phy_ref clock: %d\n", > >>> + ret); > >>> + return ret; > >>> + } > >>> + dsi->phy_ref_clk = clk; > >>> + > >>> + clk = devm_clk_get(dsi->dev, "rx_esc"); > >>> + if (IS_ERR(clk)) { > >>> + ret = PTR_ERR(clk); > >>> + DRM_DEV_ERROR(dsi->dev, "Failed to get rx_esc clock: %d\n", > >>> + ret); > >>> + return ret; > >>> + } > >>> + dsi->rx_esc_clk = clk; > >>> + > >>> + clk = devm_clk_get(dsi->dev, "tx_esc"); > >>> + if (IS_ERR(clk)) { > >>> + ret = PTR_ERR(clk); > >>> + DRM_DEV_ERROR(dsi->dev, "Failed to get tx_esc clock: %d\n", > >>> + ret); > >>> + return ret; > >>> + } > >>> + dsi->tx_esc_clk = clk; > >>> + > >>> + dsi->mux = devm_mux_control_get(dsi->dev, NULL); > >>> + if (IS_ERR(dsi->mux)) { > >>> + ret = PTR_ERR(dsi->mux); > >>> + if (ret != -EPROBE_DEFER) > >>> + DRM_DEV_ERROR(dsi->dev, "Failed to get mux: %d\n", ret); > >>> + return ret; > >>> + } > >>> + > >>> + base = devm_platform_ioremap_resource(pdev, 0); > >>> + if (IS_ERR(base)) > >>> + return PTR_ERR(base); > >>> + > >>> + dsi->regmap = > >>> + devm_regmap_init_mmio(dsi->dev, base, &nwl_dsi_regmap_config); > >>> + if (IS_ERR(dsi->regmap)) { > >>> + ret = PTR_ERR(dsi->regmap); > >>> + DRM_DEV_ERROR(dsi->dev, "Failed to create NWL DSI regmap: %d\n", > >>> + ret); > >>> + return ret; > >>> + } > >>> + > >>> + dsi->irq = platform_get_irq(pdev, 0); > >>> + if (dsi->irq < 0) { > >>> + DRM_DEV_ERROR(dsi->dev, "Failed to get device IRQ: %d\n", > >>> + dsi->irq); > >>> + return dsi->irq; > >>> + } > >>> + > >>> + dsi->rstc = devm_reset_control_array_get(dsi->dev, false, true); > >>> + if (IS_ERR(dsi->rstc)) { > >>> + DRM_DEV_ERROR(dsi->dev, "Failed to get resets: %ld\n", > >>> + PTR_ERR(dsi->rstc)); > >>> + return PTR_ERR(dsi->rstc); > >>> + } > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int imx8mq_dsi_select_input(struct nwl_dsi *dsi) > >>> +{ > >>> + struct device_node *remote; > >>> + u32 use_dcss = 1; > >>> + int ret; > >>> + > >>> + remote = of_graph_get_remote_node(dsi->dev->of_node, 0, 0); > >>> + if (strcmp(remote->name, "lcdif") == 0) > >>> + use_dcss = 0; > >> > >> Relying on node name seems to me wrong. I am not sure if whole logic for > >> input select should be here. > >> > >> My 1st impression is that selecting should be done rather in DCSS or > >> LCDIF driver, why do you want to put it here? > > Doing it in here keeps it at a single location where on the other hand > > it would need to be done in mxsfb (which handles other SoCs as well) and > > upcoming dcss. Also we can have in the dsi enable path which e.g. mxsfb > > doesn't even know about at this point. > > > But as I understand mux is not a part of this IP. It is always > problematic what to do with such small pieces of hw. > > Let's keep it here until someone find better solution. > > Btw do you have public tree for testing platforms with this driver, I am > curious about this mux device. https://source.puri.sm/guido.gunther/linux-imx8/blob/forward-upstream/next-20190823/mxsfb+nwl/v4/arch/arm64/boot/dts/freescale/imx8mq.dtsi#L477 it was prompted by https://lists.freedesktop.org/archives/dri-devel/2019-August/230868.html Cheers and thanks again for your comments, -- Guido > > > Regards > > Andrzej > > > > > > Cheers, > > -- Guido > > > >> Regards > >> > >> Andrzej > >> > >> > >>> + > >>> + DRM_DEV_INFO(dsi->dev, "Using %s as input source\n", > >>> + (use_dcss) ? "DCSS" : "LCDIF"); > >>> + > >>> + ret = mux_control_try_select(dsi->mux, use_dcss); > >>> + if (ret < 0) > >>> + DRM_DEV_ERROR(dsi->dev, "Failed to select input: %d\n", ret); > >>> + > >>> + of_node_put(remote); > >>> + return ret; > >>> +} > >>> + > >>> + > >>> +static int imx8mq_dsi_deselect_input(struct nwl_dsi *dsi) > >>> +{ > >>> + int ret; > >>> + > >>> + ret = mux_control_deselect(dsi->mux); > >>> + if (ret < 0) > >>> + DRM_DEV_ERROR(dsi->dev, "Failed to deselect input: %d\n", ret); > >>> + > >>> + return ret; > >>> +} > >>> + > >>> + > >>> +static int imx8mq_dsi_poweron(struct nwl_dsi *dsi) > >>> +{ > >>> + int ret = 0; > >>> + > >>> + /* otherwise the display stays blank */ > >>> + usleep_range(200, 300); > >>> + > >>> + if (dsi->rstc) > >>> + ret = reset_control_deassert(dsi->rstc); > >>> + > >>> + return ret; > >>> +} > >>> + > >>> +static int imx8mq_dsi_poweroff(struct nwl_dsi *dsi) > >>> +{ > >>> + int ret = 0; > >>> + > >>> + if (dsi->quirks & SRC_RESET_QUIRK) > >>> + return 0; > >>> + > >>> + if (dsi->rstc) > >>> + ret = reset_control_assert(dsi->rstc); > >>> + return ret; > >>> +} > >>> + > >>> +static const struct drm_bridge_timings nwl_dsi_timings = { > >>> + .input_bus_flags = DRM_BUS_FLAG_DE_LOW, > >>> +}; > >>> + > >>> +static const struct nwl_dsi_platform_data imx8mq_dev = { > >>> + .poweron = &imx8mq_dsi_poweron, > >>> + .poweroff = &imx8mq_dsi_poweroff, > >>> + .select_input = &imx8mq_dsi_select_input, > >>> + .deselect_input = &imx8mq_dsi_deselect_input, > >>> + .clk_config = { > >>> + { .id = NWL_DSI_CLK_CORE, .present = true }, > >>> + }, > >>> +}; > >>> + > >>> +static const struct of_device_id nwl_dsi_dt_ids[] = { > >>> + { .compatible = "fsl,imx8mq-nwl-dsi", .data = &imx8mq_dev, }, > >>> + { /* sentinel */ } > >>> +}; > >>> +MODULE_DEVICE_TABLE(of, nwl_dsi_dt_ids); > >>> + > >>> +static const struct soc_device_attribute nwl_dsi_quirks_match[] = { > >>> + { .soc_id = "i.MX8MQ", .revision = "2.0", > >>> + .data = (void *)(E11418_HS_MODE_QUIRK | SRC_RESET_QUIRK) }, > >>> + { /* sentinel. */ }, > >>> +}; > >>> + > >>> +static int nwl_dsi_probe(struct platform_device *pdev) > >>> +{ > >>> + struct device *dev = &pdev->dev; > >>> + const struct of_device_id *of_id = of_match_device(nwl_dsi_dt_ids, dev); > >>> + const struct nwl_dsi_platform_data *pdata = of_id->data; > >>> + const struct soc_device_attribute *attr; > >>> + struct nwl_dsi *dsi; > >>> + int ret; > >>> + > >>> + dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL); > >>> + if (!dsi) > >>> + return -ENOMEM; > >>> + > >>> + dsi->dev = dev; > >>> + dsi->pdata = pdata; > >>> + > >>> + ret = nwl_dsi_parse_dt(dsi); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + ret = devm_request_irq(dev, dsi->irq, nwl_dsi_irq_handler, 0, > >>> + dev_name(dev), dsi); > >>> + if (ret < 0) { > >>> + DRM_DEV_ERROR(dev, "Failed to request IRQ %d: %d\n", dsi->irq, > >>> + ret); > >>> + return ret; > >>> + } > >>> + > >>> + dsi->dsi_host.ops = &nwl_dsi_host_ops; > >>> + dsi->dsi_host.dev = dev; > >>> + ret = mipi_dsi_host_register(&dsi->dsi_host); > >>> + if (ret) { > >>> + DRM_DEV_ERROR(dev, "Failed to register MIPI host: %d\n", ret); > >>> + return ret; > >>> + } > >>> + > >>> + attr = soc_device_match(nwl_dsi_quirks_match); > >>> + if (attr) > >>> + dsi->quirks = (uintptr_t)attr->data; > >>> + > >>> + dsi->bridge.driver_private = dsi; > >>> + dsi->bridge.funcs = &nwl_dsi_bridge_funcs; > >>> + dsi->bridge.of_node = dev->of_node; > >>> + dsi->bridge.timings = &nwl_dsi_timings; > >>> + > >>> + dev_set_drvdata(dev, dsi); > >>> + pm_runtime_enable(dev); > >>> + return 0; > >>> +} > >>> + > >>> +static int nwl_dsi_remove(struct platform_device *pdev) > >>> +{ > >>> + struct nwl_dsi *dsi = platform_get_drvdata(pdev); > >>> + > >>> + mipi_dsi_host_unregister(&dsi->dsi_host); > >>> + pm_runtime_disable(&pdev->dev); > >>> + return 0; > >>> +} > >>> + > >>> +static struct platform_driver nwl_dsi_driver = { > >>> + .probe = nwl_dsi_probe, > >>> + .remove = nwl_dsi_remove, > >>> + .driver = { > >>> + .of_match_table = nwl_dsi_dt_ids, > >>> + .name = DRV_NAME, > >>> + }, > >>> +}; > >>> + > >>> +module_platform_driver(nwl_dsi_driver); > >>> + > >>> +MODULE_AUTHOR("NXP Semiconductor"); > >>> +MODULE_AUTHOR("Purism SPC"); > >>> +MODULE_DESCRIPTION("Northwest Logic MIPI-DSI driver"); > >>> +MODULE_LICENSE("GPL"); /* GPLv2 or later */ > >>> diff --git a/drivers/gpu/drm/bridge/nwl-dsi/nwl-drv.h b/drivers/gpu/drm/bridge/nwl-dsi/nwl-drv.h > >>> new file mode 100644 > >>> index 000000000000..1e72a9221401 > >>> --- /dev/null > >>> +++ b/drivers/gpu/drm/bridge/nwl-dsi/nwl-drv.h > >>> @@ -0,0 +1,65 @@ > >>> +/* SPDX-License-Identifier: GPL-2.0+ */ > >>> +/* > >>> + * NWL MIPI DSI host driver > >>> + * > >>> + * Copyright (C) 2017 NXP > >>> + * Copyright (C) 2019 Purism SPC > >>> + */ > >>> + > >>> +#ifndef __NWL_DRV_H__ > >>> +#define __NWL_DRV_H__ > >>> + > >>> +#include > >>> +#include > >>> + > >>> +#include > >>> +#include > >>> + > >>> +struct nwl_dsi_platform_data; > >>> + > >>> +/* i.MX8 NWL quirks */ > >>> +/* i.MX8MQ errata E11418 */ > >>> +#define E11418_HS_MODE_QUIRK BIT(0) > >>> +/* Skip DSI bits in SRC on disable to avoid blank display on enable */ > >>> +#define SRC_RESET_QUIRK BIT(1) > >>> + > >>> +#define NWL_DSI_MAX_PLATFORM_CLOCKS 1 > >>> +struct nwl_dsi_plat_clk_config { > >>> + const char *id; > >>> + struct clk *clk; > >>> + bool present; > >>> +}; > >>> + > >>> +struct nwl_dsi { > >>> + struct drm_bridge bridge; > >>> + struct mipi_dsi_host dsi_host; > >>> + struct drm_bridge *panel_bridge; > >>> + struct device *dev; > >>> + struct phy *phy; > >>> + union phy_configure_opts phy_cfg; > >>> + unsigned int quirks; > >>> + > >>> + struct regmap *regmap; > >>> + int irq; > >>> + struct reset_control *rstc; > >>> + struct mux_control *mux; > >>> + > >>> + /* DSI clocks */ > >>> + struct clk *phy_ref_clk; > >>> + struct clk *rx_esc_clk; > >>> + struct clk *tx_esc_clk; > >>> + /* Platform dependent clocks */ > >>> + struct nwl_dsi_plat_clk_config clk_config[NWL_DSI_MAX_PLATFORM_CLOCKS]; > >>> + > >>> + /* dsi lanes */ > >>> + u32 lanes; > >>> + enum mipi_dsi_pixel_format format; > >>> + struct drm_display_mode mode; > >>> + unsigned long dsi_mode_flags; > >>> + > >>> + struct nwl_dsi_transfer *xfer; > >>> + > >>> + const struct nwl_dsi_platform_data *pdata; > >>> +}; > >>> + > >>> +#endif /* __NWL_DRV_H__ */ > >>> diff --git a/drivers/gpu/drm/bridge/nwl-dsi/nwl-dsi.c b/drivers/gpu/drm/bridge/nwl-dsi/nwl-dsi.c > >>> new file mode 100644 > >>> index 000000000000..e6038cb4e849 > >>> --- /dev/null > >>> +++ b/drivers/gpu/drm/bridge/nwl-dsi/nwl-dsi.c > >>> @@ -0,0 +1,696 @@ > >>> +// SPDX-License-Identifier: GPL-2.0+ > >>> +/* > >>> + * NWL MIPI DSI host driver > >>> + * > >>> + * Copyright (C) 2017 NXP > >>> + * Copyright (C) 2019 Purism SPC > >>> + */ > >>> + > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> +#include > >>> + > >>> +#include