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 Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C10A6C43334 for ; Thu, 14 Jul 2022 18:16:00 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 7EFF8839C3; Thu, 14 Jul 2022 20:15:58 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=gerhold.net Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gerhold.net header.i=@gerhold.net header.b="BpcruegH"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 7B5A68403D; Thu, 14 Jul 2022 20:15:56 +0200 (CEST) Received: from mo4-p01-ob.smtp.rzone.de (mo4-p01-ob.smtp.rzone.de [81.169.146.165]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 0F723810E9 for ; Thu, 14 Jul 2022 20:15:53 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=none (p=none dis=none) header.from=gerhold.net Authentication-Results: phobos.denx.de; spf=none smtp.mailfrom=stephan@gerhold.net DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; t=1657822546; s=strato-dkim-0002; d=gerhold.net; h=In-Reply-To:References:Message-ID:Subject:Cc:To:From:Date:Cc:Date: From:Subject:Sender; bh=bHOxEIJKd19p4Tu/1qWly27jWPZS3PR90diebPwr7A0=; b=BpcruegHtw8Cci/5mxIISgG/yZHIzbDe4sw7aA11kGNRkfhsgd7Xy87Y3H/PbmkFQ0 zHDcR82KeWSms3v91S32ZQBHVHQ9N49XxPJaXeMuALd5eFbOzaCL9l6V1Vanvi2w83fY fHZB7Yrg7j2weflFAPYRWSMTeaJ3czDmyOuY2unA8ZgMLp0g8EsCKua+CM+3Ko2TGKfx WWESv9FWv6ZdrP5ZWtXNQWXm4FNOFSlnLzv3SUaljnjfVT94FJO8KMKjQ/isLKjxbLnU 3FdKK44hfUM3iBJz9qeA2UHXRRsnE2+FqB8iFp7pj8y4Mhs7o1r7pLUj3CoTszuKNpo/ U+ug== Authentication-Results: strato.com; dkim=none X-RZG-AUTH: ":P3gBZUipdd93FF5ZZvYFPugejmSTVR2nRPhVOQ/OcYgojyw4j34+u267FZF9PwpcNKLVrKw8+6Y=" X-RZG-CLASS-ID: mo00 Received: from gerhold.net by smtp.strato.de (RZmta 47.47.0 AUTH) with ESMTPSA id he04d0y6EIFk88R (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256 bits)) (Client did not present a certificate); Thu, 14 Jul 2022 20:15:46 +0200 (CEST) Date: Thu, 14 Jul 2022 20:15:39 +0200 From: Stephan Gerhold To: Sumit Garg Cc: u-boot@lists.denx.de, robert.marko@sartura.hr, luka.kovacic@sartura.hr, luka.perkov@sartura.hr, rfried.dev@gmail.com, dsankouski@gmail.com, sjg@chromium.org, trini@konsulko.com, vinod.koul@linaro.org, nicolas.dechesne@linaro.org, mworsfold@impinj.com, daniel.thompson@linaro.org, pbrobinson@gmail.com Subject: Re: [PATCH] arm: dts: qcom: Sync pinctrl DT nodes with Linux bindings Message-ID: References: <20220714073337.2298978-1-sumit.garg@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220714073337.2298978-1-sumit.garg@linaro.org> X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.6 at phobos.denx.de X-Virus-Status: Clean On Thu, Jul 14, 2022 at 01:03:37PM +0530, Sumit Garg wrote: > Currently for all Qcom SoCs/boards there are separate compatibles for > GPIO and pinctrl. But this is inconsistent with official (upstream) Linux > bindings which requires only a single compatible "qcom,-pinctrl" > and there is no such compatible property as "qcom,tlmm-". > > So fix this inconsistency for Qcom SoCs in order to comply with upstream > DT bindings. This is done via removing compatibles from "msm_gpio" driver > and via binding to "msm_gpio" driver from pinctrl driver in case > "gpio-controller" property is specified for pinctrl node. > > Suggested-by: Stephan Gerhold > Signed-off-by: Sumit Garg This is a great start, thank you! > --- > > This is based on top of mine other patch-set [1]. Although, I have > tested it on db845c and qcs404-evb but I would like other board > maintainers to test it as well. > > [1] https://patchwork.ozlabs.org/project/uboot/list/?series=309135 If possible I would prefer to introduce the QCS404 platform with a clean DT (i.e. qcs404.dtsi imported from the Linux kernel, instead of adding the custom qcs404-evb.dts and then cleaning it up). This patch would need to come before the QCS404 addition then. > > arch/arm/dts/dragonboard410c-uboot.dtsi | 2 +- > arch/arm/dts/dragonboard410c.dts | 17 +++----- > arch/arm/dts/dragonboard820c-uboot.dtsi | 2 +- > arch/arm/dts/dragonboard820c.dts | 4 +- > arch/arm/dts/qcom-ipq4019.dtsi | 18 +++------ > arch/arm/dts/qcs404-evb.dts | 2 +- > arch/arm/dts/sdm845.dtsi | 2 +- > arch/arm/mach-ipq40xx/pinctrl-snapdragon.c | 31 ++++++++++++++- > arch/arm/mach-snapdragon/Makefile | 2 +- > arch/arm/mach-snapdragon/pinctrl-snapdragon.c | 39 ++++++++++++++++--- > drivers/gpio/msm_gpio.c | 10 +---- > 11 files changed, 83 insertions(+), 46 deletions(-) > > [...] > diff --git a/arch/arm/dts/dragonboard410c.dts b/arch/arm/dts/dragonboard410c.dts > index 7e56140df2..e0452b270c 100644 > --- a/arch/arm/dts/dragonboard410c.dts > +++ b/arch/arm/dts/dragonboard410c.dts > @@ -60,9 +60,13 @@ > reg = <0x60000 0x8000>; > }; > > - pinctrl: qcom,tlmm@1000000 { > - compatible = "qcom,tlmm-apq8016"; > + soc_gpios: pinctrl@1000000 { > + compatible = "qcom,apq8016-pinctrl"; This should be "qcom,msm8916-pinctrl" (see msm8916.dtsi in Linux) > reg = <0x1000000 0x400000>; > + gpio-controller; > + gpio-count = <122>; > + gpio-bank-name="soc"; > + #gpio-cells = <2>; > > blsp1_uart: uart { > function = "blsp1_uart"; > @@ -86,15 +90,6 @@ > pinctrl-0 = <&blsp1_uart>; > }; > > - soc_gpios: pinctrl@1000000 { > - compatible = "qcom,apq8016-pinctrl"; > - reg = <0x1000000 0x300000>; > - gpio-controller; > - gpio-count = <122>; > - gpio-bank-name="soc"; > - #gpio-cells = <2>; > - }; > - > ehci@78d9000 { > compatible = "qcom,ehci-host"; > reg = <0x78d9000 0x400>; > [...] > diff --git a/arch/arm/dts/dragonboard820c.dts b/arch/arm/dts/dragonboard820c.dts > index 1114ddd7d3..1a6e37887f 100644 > --- a/arch/arm/dts/dragonboard820c.dts > +++ b/arch/arm/dts/dragonboard820c.dts > @@ -64,8 +64,8 @@ > reg = <0x300000 0x90000>; > }; > > - pinctrl: qcom,tlmm@1010000 { > - compatible = "qcom,tlmm-apq8096"; > + pinctrl: pinctrl@1010000 { > + compatible = "qcom,msm8916-pinctrl"; Huh, this should be "qcom,msm8996-pinctrl" :) > reg = <0x1010000 0x400000>; > > blsp8_uart: uart { > [...] > diff --git a/arch/arm/dts/qcs404-evb.dts b/arch/arm/dts/qcs404-evb.dts > index 4f0ae20bdb..09687e1fd3 100644 > --- a/arch/arm/dts/qcs404-evb.dts > +++ b/arch/arm/dts/qcs404-evb.dts > @@ -38,7 +38,7 @@ > compatible = "simple-bus"; > > pinctrl_north@1300000 { > - compatible = "qcom,tlmm-qcs404"; > + compatible = "qcom,qcs404-pinctrl"; > reg = <0x1300000 0x200000>; The Linux node still looks a bit different (from qcs404.dtsi): tlmm: pinctrl@1000000 { compatible = "qcom,qcs404-pinctrl"; reg = <0x01000000 0x200000>, <0x01300000 0x200000>, <0x07b00000 0x200000>; reg-names = "south", "north", "east"; I guess we'll need to fetch the "north" region from it (if that's what you need)? > > blsp1_uart2: uart { > diff --git a/arch/arm/dts/sdm845.dtsi b/arch/arm/dts/sdm845.dtsi > index df5b6dfcfc..607af277f8 100644 > --- a/arch/arm/dts/sdm845.dtsi > +++ b/arch/arm/dts/sdm845.dtsi > @@ -37,7 +37,7 @@ > }; > > tlmm_north: pinctrl_north@3900000 { > - compatible = "qcom,tlmm-sdm845"; > + compatible = "qcom,sdm845-pinctrl"; > reg = <0x3900000 0x400000>; Hmm this is reg = <0 0x03400000 0 0xc00000>; in Linux. Some offset applied internally in the driver? Feels much like my SPMI problem. :) It's probably similar to the south/north thing of QCS404. > [...] > diff --git a/arch/arm/mach-snapdragon/Makefile b/arch/arm/mach-snapdragon/Makefile > index 0d31f10f68..cbaaf23f6b 100644 > --- a/arch/arm/mach-snapdragon/Makefile > +++ b/arch/arm/mach-snapdragon/Makefile > @@ -16,6 +16,6 @@ obj-y += pinctrl-snapdragon.o > obj-y += pinctrl-apq8016.o > obj-y += pinctrl-apq8096.o > obj-y += pinctrl-qcs404.o > -obj-$(CONFIG_SDM845) += pinctrl-sdm845.o > +obj-y += pinctrl-sdm845.o > obj-$(CONFIG_TARGET_QCS404EVB) += clock-qcs404.o > obj-$(CONFIG_TARGET_QCS404EVB) += sysmap-qcs404.o Was this change intended here? > diff --git a/arch/arm/mach-snapdragon/pinctrl-snapdragon.c b/arch/arm/mach-snapdragon/pinctrl-snapdragon.c > index c2148a5d0a..bba1f642ae 100644 > --- a/arch/arm/mach-snapdragon/pinctrl-snapdragon.c > +++ b/arch/arm/mach-snapdragon/pinctrl-snapdragon.c > @@ -10,6 +10,8 @@ > #include > #include > #include > +#include > +#include > #include > #include > #include "pinctrl-snapdragon.h" > @@ -113,13 +115,37 @@ static struct pinctrl_ops msm_pinctrl_ops = { > .get_function_name = msm_get_function_name, > }; > > +static int msm_pinctrl_bind(struct udevice *dev) > +{ > + ofnode node = dev_ofnode(dev); > + const char *name; > + int ret; > + > + ofnode_get_property(node, "gpio-controller", &ret); > + if (ret < 0) > + return 0; > + > + /* Get the name of gpio node */ > + name = ofnode_get_name(node); > + if (!name) > + return -EINVAL; > + > + /* Bind gpio node */ > + ret = device_bind_driver_to_node(dev, "gpio_msm", > + name, node, NULL); > + if (ret) > + return ret; > + > + dev_dbg(dev, "bind %s\n", name); > + > + return 0; > +} > + > static const struct udevice_id msm_pinctrl_ids[] = { > - { .compatible = "qcom,tlmm-apq8016", .data = (ulong)&apq8016_data }, > - { .compatible = "qcom,tlmm-apq8096", .data = (ulong)&apq8096_data }, > -#ifdef CONFIG_SDM845 > - { .compatible = "qcom,tlmm-sdm845", .data = (ulong)&sdm845_data }, > -#endif Ah, the Makefile change was to avoid the #ifdefs here I guess. Can you put this into an extra patch for less confusion? > - { .compatible = "qcom,tlmm-qcs404", .data = (ulong)&qcs404_data }, > + { .compatible = "qcom,apq8016-pinctrl", .data = (ulong)&apq8016_data }, > + { .compatible = "qcom,msm8916-pinctrl", .data = (ulong)&apq8096_data }, Same comment as in DT: "qcom,msm8916-pinctrl" = apq8016_data "qcom,msm8996-pinctrl" = apq8096_data Thanks! Stephan