Re: [PATCH] arm: dts: qcom: Sync pinctrl DT nodes with Linux bindings

[Stephan Gerhold <stephan@gerhold.net>]

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,<SoC name>-pinctrl"
> and there is no such compatible property as "qcom,tlmm-<SoC name>".
> 
> 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 <stephan@gerhold.net>
> Signed-off-by: Sumit Garg <sumit.garg@linaro.org>

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 <dm.h>
>  #include <errno.h>
>  #include <asm/io.h>
> +#include <dm/device_compat.h>
> +#include <dm/lists.h>
>  #include <dm/pinctrl.h>
>  #include <linux/bitops.h>
>  #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