Received: by 2002:a25:c593:0:0:0:0:0 with SMTP id v141csp985219ybe; Thu, 19 Sep 2019 07:01:12 -0700 (PDT) X-Google-Smtp-Source: APXvYqwQ0lyfZEKIri7Aj/9OWQqUSLVF4nkUgVsdmqRcGjd2tgLSBmB8qhydTHd30qkd6uDo9cy3 X-Received: by 2002:a17:906:5813:: with SMTP id m19mr14004454ejq.246.1568901671999; Thu, 19 Sep 2019 07:01:11 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1568901671; cv=none; d=google.com; s=arc-20160816; b=e0mTTjrHH6qOu7uC6VtJ/SawA3gYFYfEWa3gPr7IpSNi6LhbnVfHWh7NCH6mQ+vMcM Ti/x5j8Xqpwc9MEgMaEhndC0BPCVXT5l36nXSnlX+DdWSJaasswD6c/hmgM6LJY2sMOT e0hGz/4IbyjHDdeSx3x80qOrVKtPQz+wFWN8RFDIXBQW1BF2IvSLDcx3b9M6IyLD/+ko +F+ATTXz0oBdAbECRABRsbH5e2ksu2mRWfRFWfau+6zgX1QH80ZlkCIzgKsqTya0Fv/i ODS3dL8N4aMQ/VVkEL1JaHRfG/7fQSPMZcGH5blMZu3M8aqk9rU7rph4NFmUqYJI/pYD xr3Q== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:references:cms-type:content-language :content-transfer-encoding:in-reply-to:mime-version:user-agent:date :message-id:from:cc:to:subject:dkim-signature:dkim-filter; bh=wu/Z2SptIyvGmw5fHBKgX11aKpziKA66m7IhdhmfOkA=; b=IyTwkcc4qthiFIPaZ+JLH81OzjyVc7dTNaPadEcQSCLMEO/pwp9uHdF3RTSw8N/If1 MvUPeQ4bZ6UcQcpeWoHlBm7XRYlAFGa1ofKR8iHolQulrwM11RGz1ZD7+k4xW7AOLHlV STBzRmSHDvMHgWOyHbEOzI4cNM993+l6O+HS3G12QUTq1Zx4aI78U3xje+bHfxFNQH0L iSJTzs3Ge9HHl6GBgYM3g4vCFuPjHrn9PgtnxpiKYO+3o2WT20mqjk5uT5Rbuo80fLWl WUgiUVx6enmrn/GmHLw076imOFG0fk9b96HqexonWo3KAecqCEL+glk1B66AF1sjeO2I 7s4g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@samsung.com header.s=mail20170921 header.b=BAraJC6B; 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; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=samsung.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id d17si4864577edv.76.2019.09.19.07.00.46; Thu, 19 Sep 2019 07:01:11 -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; dkim=pass header.i=@samsung.com header.s=mail20170921 header.b=BAraJC6B; 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; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=samsung.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1732199AbfISN4x (ORCPT + 99 others); Thu, 19 Sep 2019 09:56:53 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:43575 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730838AbfISN4x (ORCPT ); Thu, 19 Sep 2019 09:56:53 -0400 Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20190919135650euoutp0274af244e150df315713f068bb7f52e11~F24UnD3L10214702147euoutp02D for ; Thu, 19 Sep 2019 13:56:50 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20190919135650euoutp0274af244e150df315713f068bb7f52e11~F24UnD3L10214702147euoutp02D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1568901410; bh=wu/Z2SptIyvGmw5fHBKgX11aKpziKA66m7IhdhmfOkA=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=BAraJC6BLiwYzDZWwvoFb5Sn+U0DzYUzFYqhgObULlMzaSx6mMOyPXn801np5by17 aZNxCNpkvw9XIg+dZjjxs86BXRk5vNceTNtxp8en/zMmiTUMQjQJ6Xi4oRiZ1xg6nc PRTHRb81cegx1F1RSZCYetZuCDisNN9GBG3591wg= Received: from eusmges3new.samsung.com (unknown [203.254.199.245]) by eucas1p1.samsung.com (KnoxPortal) with ESMTP id 20190919135649eucas1p13c580aa815eb13fd9d4101b140dc6572~F24TrpDY_0586905869eucas1p1u; Thu, 19 Sep 2019 13:56:49 +0000 (GMT) Received: from eucas1p1.samsung.com ( [182.198.249.206]) by eusmges3new.samsung.com (EUCPMTA) with SMTP id AF.55.04374.029838D5; Thu, 19 Sep 2019 14:56:48 +0100 (BST) Received: from eusmtrp2.samsung.com (unknown [182.198.249.139]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20190919135648eucas1p1e13d3bd33881ee39c6d0dbc0fa76cab9~F24Sykbjq0712107121eucas1p1U; Thu, 19 Sep 2019 13:56:48 +0000 (GMT) Received: from eusmgms1.samsung.com (unknown [182.198.249.179]) by eusmtrp2.samsung.com (KnoxPortal) with ESMTP id 20190919135647eusmtrp24f9c95ce7b85752e9ea2782a1f1e079b~F24SkMzNO1464114641eusmtrp2y; Thu, 19 Sep 2019 13:56:47 +0000 (GMT) X-AuditID: cbfec7f5-92d689c000001116-88-5d838920ae5c Received: from eusmtip2.samsung.com ( [203.254.199.222]) by eusmgms1.samsung.com (EUCPMTA) with SMTP id 82.7B.04166.F19838D5; Thu, 19 Sep 2019 14:56:47 +0100 (BST) Received: from [106.120.51.74] (unknown [106.120.51.74]) by eusmtip2.samsung.com (KnoxPortal) with ESMTPA id 20190919135646eusmtip2b21435328c7b5ba35fe5cea8b014edbe~F24RlLJcH1139311393eusmtip2b; Thu, 19 Sep 2019 13:56:46 +0000 (GMT) Subject: Re: [PATCH v5 2/2] drm/bridge: Add NWL MIPI DSI host controller support To: =?UTF-8?Q?Guido_G=c3=bcnther?= 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 From: Andrzej Hajda Message-ID: Date: Thu, 19 Sep 2019 15:56:45 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: <20190914161155.GA2933@bogon.m.sigxcpu.org> Content-Transfer-Encoding: 8bit Content-Language: en-US X-Brightmail-Tracker: H4sIAAAAAAAAA01SWUwTURT1dZZOidWhIH2iCdpAjCYCiibPaIgmfkz8EPWLQIhWHZGw2mER l4gWFGpUrEGhEopIw2ITtCy1xmooSK1aEKVsYgqKRkTABXDBim0HI3/3nHPPfffcPAqT3CID qYSUdFaRIk+SkT54U9vP9rUrCpRx4TU94ejL6SECXWi3CZBL3SZEs02XMaRtbSdQ19QEiYYc UcjxfQRDtk8OHNUWmXBUcLlSiJxTjwAyvO0mkOpXDYZe3islka6nU4AsF2NRs9ZCoDxzqxC5 jAYcVf9sBOhDg3RrAKMv0wNm5pcaMBO9eULGPF2OMybNayFzPb+EYAy1BSTzuPCFgBnovk8y xulBgnGetwqY+spTzJ3xuwLmkiucqbzqIBnN+SawSxLjs+Ugm5SQySrCIvf5HB4qLSLTWjXY UaPrD5kD3j0TqICIgvQGeL+og1QBH0pCVwNoM1/DeDAJYMVvFcGDbwBar1QL/1nO9tkFvFAF YF1V3Zx/DMDumRvA0+VH74EThee8Dn+3Q50/4B2F0UUkLLRMEx6BpFdDV30f6anFdCQsLtZ7 DTgdAvUDo7inXkJHw6+DLQTf4wttJcNeXkQjmPew2MtjdBBUNl7H+FoK+4e13vUgPUxBw/d+ N6DcYDvs6orgI/jBj9aGuTjL4axJO3eNU9BZnYvx3nwAG2+bMF7YDFusnYRnDuZeuu5eGE9v g4aSZ4Afvwj2jvnyKyyC6ibPHT20GOaflfDdK6HT3jg3UAp1z6fIQiDTzAummRdGMy+M5v+7 5QCvBVI2g0uOZ7mIFDYrlJMncxkp8aEHUpMNwP2Fn/6xTt0FD37vtwCaArKF4hVZyjgJIc/k spMtAFKYzF9cuvFMnER8UJ59jFWk7lVkJLGcBSyjcJlUfHzBYKyEjpens4ksm8Yq/qkCShSY A65usq5Z3LL75Bervf1SWeJkjNn+Y/RJ8buEyL6dYft0h9QTXJ/GGR2kS336JqtTb9VFvdeN +4mSFqTvWB9180SQqRnvkCm1C7V2rbQ+OGDSuLimHlUGn0szbRkdqRhZlSt+NXPauOxrxwFl iC017WL27Oojop6lObmv975RBX5+IMO5w/J1azAFJ/8LKtsqqb4DAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA02Sf0yMcRzH931+3VNz9niU++oP8him5ur6oW/Jrdls3z9YJjNTLbd6lNXd cc+F2BQtKkNlUed04VjamSlXF2KdSJNMXKKSJj9D80cZOdwPtv577f35vN7bZ/uwJH+bDmF3 6IyiQafJE5hA6uHvrlcrFpYVp0c6z0ajbwdHaXSst5tA7qr7MvSnpZJEls5eGj2bnGDQqCsZ ub5/JFH3ZxeFGqvbKFRWaZWhkcl7ADW96adR+c/LJHp6w8ygi8+fEMh5PBV1WJw0KmnvlCF3 axOFGn7YAfpwXZE0D9vqbABP/6wCeGKgRIbbp+op3GYaluEzpbU0bmosY/CDij4CD/XfYnDr 1GsajxztInCztRBf++og8Al3JLaecjHYdLQFbOC3KhMN+nyjGJqjl4yrhVQVilKq4pEyKiZe qYqOS0+IihUi1IlZYt6O3aIhQr1NmTNqrmZ2dprIva3u30wReNtDlIMAFnIx8PCLRx4OZHnu IoAD1nOMf6CANy1fSD/Phb/6y305z40DWDG81stzuY1wouKIzMtBnqKq0iHaW0RypxnYYbaT /lY7AS98uuSzGW45dDe/8LGcU8OaGpvPprgl0DY0Tnk5mNsC7zpMwL8zB3bXjvnyAA7Bkjs1 tJdJbhn8VddH+nkhLLaf+ccK+HLMQlQA3jRDN81QTDMU0wylHlCNIEjMl7TZWkmllDRaKV+X rczUa5uA53da7v9odoC+aylOwLFAmCUP3VOcztOa3VKB1gkgSwpBcnPsoXRenqUp2Cca9BmG /DxRcoJYz3GVZEhwpt7ziTpjhipWFYfiVXHRcdErkaCQl3IdaTyXrTGKuaK4UzT89wg2IKQI 4KTl4ebMZXUrc+tTacG4Zn1hwfaM6UU5+3tTjA3v+kaSHe2sZdXmkwcq69yDut6rXbpwXVLW IDarAwam7LYNCvmdTVe+zk5IxGGp7+NjhM5d2daexxHJ8xumyu4tTpsU9AsuBxPHHKFKFx/I qt3UuqV8JB9+/umwemna4LkjAiXlaFRhpEHS/AUbP3JqUQMAAA== X-CMS-MailID: 20190919135648eucas1p1e13d3bd33881ee39c6d0dbc0fa76cab9 X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20190909022600epcas3p19955ec815e4ceb54097ede895bc57b52 X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20190909022600epcas3p19955ec815e4ceb54097ede895bc57b52 References: <3ce1891ea41249bf4a9985e2cee8640fb36de42e.1567995854.git.agx@sigxcpu.org> <20190914161155.GA2933@bogon.m.sigxcpu.org> Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > >>> 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. Btw. naming different SoCs 8QM an 8MQ suggests i.mx engineers are quite nasty :) > > 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? 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. > >>> + >>> +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. 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