From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 34C62C3A5A6 for ; Thu, 19 Sep 2019 17:43:49 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E9DD8214AF for ; Thu, 19 Sep 2019 17:43:48 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="qWt7gDAa" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E9DD8214AF Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=sigxcpu.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=XGIPlxx4MIXimzjb9PhNJ+Z/l7fhjlJLydezRXE8HVU=; b=qWt7gDAaAHGf2+ PpHVIzd8XhTVPci9L9xJxZmuP25laFTplcQ5hhrd+hSWYGc125RbcTxeogAnsIOSWw2IecFen109A UiB4U1e7GmMlfJDap8jq1cdpM1nJlwad6fOGEBsxw5dEn6xAc1W2uDOgUcvfhU157bfVTA0CkT0j5 PjRCdM25+jiXBVDmgAc+cXpKlz5BKT/7Ymm3qipENA6xK4TO5K2GBCrtFrkFOY1awskLiTyC+nkjj betlTI4AfP4KFkh+Vt8miDysFVAYIL6E+325h1EHY6IWIhiSOXdmk3maVYG/nX+1WwxAnA3g+lnTN K9dsCIqrjoPId+KkixqQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.2 #3 (Red Hat Linux)) id 1iB0St-0006II-Em; Thu, 19 Sep 2019 17:43:35 +0000 Received: from honk.sigxcpu.org ([24.134.29.49]) by bombadil.infradead.org with esmtps (Exim 4.92.2 #3 (Red Hat Linux)) id 1iB0Sm-0006HU-Jf for linux-arm-kernel@lists.infradead.org; Thu, 19 Sep 2019 17:43:33 +0000 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 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-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190919_104329_219575_4037E71F X-CRM114-Status: GOOD ( 42.75 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Mark Rutland , devicetree@vger.kernel.org, Jernej Skrabec , Pengutronix Kernel Team , Sam Ravnborg , Neil Armstrong , David Airlie , Fabio Estevam , Sascha Hauer , Jonas Karlman , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Rob Herring , Arnd Bergmann , NXP Linux Team , Daniel Vetter , Robert Chiras , Lee Jones , Shawn Guo , linux-arm-kernel@lists.infradead.org, Laurent Pinchart Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.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=FCnther 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=FCnther 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=FCnther > >>> 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) +=3D analogix/ > >>> obj-$(CONFIG_DRM_I2C_ADV7511) +=3D adv7511/ > >>> obj-$(CONFIG_DRM_TI_SN65DSI86) +=3D ti-sn65dsi86.o > >>> obj-$(CONFIG_DRM_TI_TFP410) +=3D ti-tfp410.o > >>> +obj-$(CONFIG_DRM_NWL_MIPI_DSI) +=3D nwl-dsi/ > >>> obj-y +=3D 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/dr= m/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 :=3D nwl-drv.o nwl-dsi.o > >>> +obj-$(CONFIG_DRM_NWL_MIPI_DSI) +=3D nwl-mipi-dsi.o > >>> +header-test-y +=3D nwl-drv.h nwl-dsi.h > >>> diff --git a/drivers/gpu/drm/bridge/nwl-dsi/nwl-drv.c b/drivers/gpu/d= rm/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 =3D { > >>> + .reg_bits =3D 16, > >>> + .val_bits =3D 32, > >>> + .reg_stride =3D 4, > >>> + .max_register =3D NWL_DSI_IRQ_MASK2, > >>> + .name =3D 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_CLOC= KS]; > >>> +}; > >> > >> 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=3Dimx_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 *bridg= e) > >>> +{ > >>> + return container_of(bridge, struct nwl_dsi, bridge); > >>> +} > >>> + > >>> +static int nwl_dsi_set_platform_clocks(struct nwl_dsi *dsi, bool ena= ble) > >>> +{ > >>> + struct device *dev =3D dsi->dev; > >>> + const char *id; > >>> + struct clk *clk; > >>> + size_t i; > >>> + unsigned long rate; > >>> + int ret, result =3D 0; > >>> + > >>> + DRM_DEV_DEBUG_DRIVER(dev, "%s platform clocks\n", > >>> + enable ? "enabling" : "disabling"); > >>> + for (i =3D 0; i < ARRAY_SIZE(dsi->pdata->clk_config); i++) { > >>> + if (!dsi->clk_config[i].present) > >>> + continue; > >>> + id =3D dsi->clk_config[i].id; > >>> + clk =3D dsi->clk_config[i].clk; > >>> + > >>> + if (enable) { > >>> + ret =3D clk_prepare_enable(clk); > >>> + if (ret < 0) { > >>> + DRM_DEV_ERROR(dev, > >>> + "Failed to enable %s clk: %d\n", > >>> + id, ret); > >>> + result =3D result ?: ret; > >>> + } > >>> + rate =3D 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 =3D dsi->dev; > >>> + int ret; > >>> + > >>> + if (dsi->pdata->select_input) > >>> + dsi->pdata->select_input(dsi); > >>> + > >>> + ret =3D nwl_dsi_set_platform_clocks(dsi, true); > >>> + if (ret < 0) > >>> + return ret; > >>> + > >>> + ret =3D 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 =3D 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 =3D 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 =3D 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 =3D 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 |=3D (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVS= YNC); > >>> + adjusted_mode->flags &=3D ~(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NV= SYNC); > >>> + > >>> + 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 =3D bridge_to_dsi(bridge); > >>> + int bpp =3D 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 =3D bridge_to_dsi(bridge); > >>> + struct device *dev =3D dsi->dev; > >>> + union phy_configure_opts new_cfg; > >>> + unsigned long phy_ref_rate; > >>> + int ret; > >>> + > >>> + ret =3D 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 =3D=3D dsi->phy_cfg.mipi_dphy.hs_= clk_rate) > >>> + return; > >>> + > >>> + phy_ref_rate =3D 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 =3D 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 =3D bridge->driver_private; > >>> + > >>> + return drm_bridge_attach(bridge->encoder, dsi->panel_bridge, bridge= ); > >>> +} > >>> + > >>> +static const struct drm_bridge_funcs nwl_dsi_bridge_funcs =3D { > >>> + .pre_enable =3D nwl_dsi_bridge_pre_enable, > >>> + .disable =3D nwl_dsi_bridge_disable, > >>> + .mode_fixup =3D nwl_dsi_bridge_mode_fixup, > >>> + .mode_set =3D nwl_dsi_bridge_mode_set, > >>> + .mode_valid =3D nwl_dsi_bridge_mode_valid, > >>> + .attach =3D nwl_dsi_bridge_attach, > >>> +}; > >>> + > >>> +static int nwl_dsi_parse_dt(struct nwl_dsi *dsi) > >>> +{ > >>> + struct platform_device *pdev =3D to_platform_device(dsi->dev); > >>> + struct clk *clk; > >>> + const char *clk_id; > >>> + void __iomem *base; > >>> + int i, ret; > >>> + > >>> + dsi->phy =3D devm_phy_get(dsi->dev, "dphy"); > >>> + if (IS_ERR(dsi->phy)) { > >>> + ret =3D PTR_ERR(dsi->phy); > >>> + if (ret !=3D -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 =3D 0; i < ARRAY_SIZE(dsi->pdata->clk_config); i++) { > >>> + if (!dsi->clk_config[i].present) > >>> + continue; > >>> + > >>> + clk_id =3D dsi->clk_config[i].id; > >>> + clk =3D devm_clk_get(dsi->dev, clk_id); > >>> + if (IS_ERR(clk)) { > >>> + ret =3D 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 =3D clk; > >>> + } > >>> + > >>> + /* DSI clocks */ > >>> + clk =3D devm_clk_get(dsi->dev, "phy_ref"); > >>> + if (IS_ERR(clk)) { > >>> + ret =3D PTR_ERR(clk); > >>> + DRM_DEV_ERROR(dsi->dev, "Failed to get phy_ref clock: %d\n", > >>> + ret); > >>> + return ret; > >>> + } > >>> + dsi->phy_ref_clk =3D clk; > >>> + > >>> + clk =3D devm_clk_get(dsi->dev, "rx_esc"); > >>> + if (IS_ERR(clk)) { > >>> + ret =3D PTR_ERR(clk); > >>> + DRM_DEV_ERROR(dsi->dev, "Failed to get rx_esc clock: %d\n", > >>> + ret); > >>> + return ret; > >>> + } > >>> + dsi->rx_esc_clk =3D clk; > >>> + > >>> + clk =3D devm_clk_get(dsi->dev, "tx_esc"); > >>> + if (IS_ERR(clk)) { > >>> + ret =3D PTR_ERR(clk); > >>> + DRM_DEV_ERROR(dsi->dev, "Failed to get tx_esc clock: %d\n", > >>> + ret); > >>> + return ret; > >>> + } > >>> + dsi->tx_esc_clk =3D clk; > >>> + > >>> + dsi->mux =3D devm_mux_control_get(dsi->dev, NULL); > >>> + if (IS_ERR(dsi->mux)) { > >>> + ret =3D PTR_ERR(dsi->mux); > >>> + if (ret !=3D -EPROBE_DEFER) > >>> + DRM_DEV_ERROR(dsi->dev, "Failed to get mux: %d\n", ret); > >>> + return ret; > >>> + } > >>> + > >>> + base =3D devm_platform_ioremap_resource(pdev, 0); > >>> + if (IS_ERR(base)) > >>> + return PTR_ERR(base); > >>> + > >>> + dsi->regmap =3D > >>> + devm_regmap_init_mmio(dsi->dev, base, &nwl_dsi_regmap_config); > >>> + if (IS_ERR(dsi->regmap)) { > >>> + ret =3D PTR_ERR(dsi->regmap); > >>> + DRM_DEV_ERROR(dsi->dev, "Failed to create NWL DSI regmap: %d\n", > >>> + ret); > >>> + return ret; > >>> + } > >>> + > >>> + dsi->irq =3D 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 =3D 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 =3D 1; > >>> + int ret; > >>> + > >>> + remote =3D of_graph_get_remote_node(dsi->dev->of_node, 0, 0); > >>> + if (strcmp(remote->name, "lcdif") =3D=3D 0) > >>> + use_dcss =3D 0; > >> > >> Relying on node name seems to me wrong. I am not sure if whole logic f= or > >> 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 =3D 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 =3D 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 =3D 0; > >>> + > >>> + /* otherwise the display stays blank */ > >>> + usleep_range(200, 300); > >>> + > >>> + if (dsi->rstc) > >>> + ret =3D reset_control_deassert(dsi->rstc); > >>> + > >>> + return ret; > >>> +} > >>> + > >>> +static int imx8mq_dsi_poweroff(struct nwl_dsi *dsi) > >>> +{ > >>> + int ret =3D 0; > >>> + > >>> + if (dsi->quirks & SRC_RESET_QUIRK) > >>> + return 0; > >>> + > >>> + if (dsi->rstc) > >>> + ret =3D reset_control_assert(dsi->rstc); > >>> + return ret; > >>> +} > >>> + > >>> +static const struct drm_bridge_timings nwl_dsi_timings =3D { > >>> + .input_bus_flags =3D DRM_BUS_FLAG_DE_LOW, > >>> +}; > >>> + > >>> +static const struct nwl_dsi_platform_data imx8mq_dev =3D { > >>> + .poweron =3D &imx8mq_dsi_poweron, > >>> + .poweroff =3D &imx8mq_dsi_poweroff, > >>> + .select_input =3D &imx8mq_dsi_select_input, > >>> + .deselect_input =3D &imx8mq_dsi_deselect_input, > >>> + .clk_config =3D { > >>> + { .id =3D NWL_DSI_CLK_CORE, .present =3D true }, > >>> + }, > >>> +}; > >>> + > >>> +static const struct of_device_id nwl_dsi_dt_ids[] =3D { > >>> + { .compatible =3D "fsl,imx8mq-nwl-dsi", .data =3D &imx8mq_dev, }, > >>> + { /* sentinel */ } > >>> +}; > >>> +MODULE_DEVICE_TABLE(of, nwl_dsi_dt_ids); > >>> + > >>> +static const struct soc_device_attribute nwl_dsi_quirks_match[] =3D { > >>> + { .soc_id =3D "i.MX8MQ", .revision =3D "2.0", > >>> + .data =3D (void *)(E11418_HS_MODE_QUIRK | SRC_RESET_QUIRK) }, > >>> + { /* sentinel. */ }, > >>> +}; > >>> + > >>> +static int nwl_dsi_probe(struct platform_device *pdev) > >>> +{ > >>> + struct device *dev =3D &pdev->dev; > >>> + const struct of_device_id *of_id =3D of_match_device(nwl_dsi_dt_ids= , dev); > >>> + const struct nwl_dsi_platform_data *pdata =3D of_id->data; > >>> + const struct soc_device_attribute *attr; > >>> + struct nwl_dsi *dsi; > >>> + int ret; > >>> + > >>> + dsi =3D devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL); > >>> + if (!dsi) > >>> + return -ENOMEM; > >>> + > >>> + dsi->dev =3D dev; > >>> + dsi->pdata =3D pdata; > >>> + > >>> + ret =3D nwl_dsi_parse_dt(dsi); > >>> + if (ret) > >>> + return ret; > >>> + > >>> + ret =3D 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 =3D &nwl_dsi_host_ops; > >>> + dsi->dsi_host.dev =3D dev; > >>> + ret =3D mipi_dsi_host_register(&dsi->dsi_host); > >>> + if (ret) { > >>> + DRM_DEV_ERROR(dev, "Failed to register MIPI host: %d\n", ret); > >>> + return ret; > >>> + } > >>> + > >>> + attr =3D soc_device_match(nwl_dsi_quirks_match); > >>> + if (attr) > >>> + dsi->quirks =3D (uintptr_t)attr->data; > >>> + > >>> + dsi->bridge.driver_private =3D dsi; > >>> + dsi->bridge.funcs =3D &nwl_dsi_bridge_funcs; > >>> + dsi->bridge.of_node =3D dev->of_node; > >>> + dsi->bridge.timings =3D &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 =3D 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 =3D { > >>> + .probe =3D nwl_dsi_probe, > >>> + .remove =3D nwl_dsi_remove, > >>> + .driver =3D { > >>> + .of_match_table =3D nwl_dsi_dt_ids, > >>> + .name =3D 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/d= rm/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_CLOC= KS]; > >>> + > >>> + /* 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/d= rm/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