| Message ID | 20220714073337.2298978-1-sumit.garg@linaro.org |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | arm: dts: qcom: Sync pinctrl DT nodes with Linux bindings | expand |
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
On Thu, 14 Jul 2022 at 23:45, Stephan Gerhold <stephan@gerhold.net> wrote: > > 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! > You are welcome. > > --- > > > > 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. > Sorry but it's unfair to block new SoCs/boards support until all the existing mess around DT is cleaned up in Qcom specific u-boot drivers. This patch is a good example to demonstrate that all corresponding boards DT need to be fixed as well which requires testing. And I don't even have access to starqltechn, ipq4019 based board and db820c. AFAIK, it's not a requirement yet but a recommendation at this stage to import DT directly from Linux kernel and work with it. But I would be very happy to work in this direction to make Qcom SoCs/boards DT compliant. So I would request the merge of new boards support and then we can follow up with patches like this one. > > > > 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) > Ack. > > 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" :) > Correct, I mixed it up. > > 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)? This is another example of a shortcut already used by the u-boot pinctrl driver (arch/arm/mach-snapdragon/pinctrl-snapdragon.c). You can find another user here (arch/arm/dts/sdm845.dtsi). So the pinctrl drivers need to be converted to a format supported by Linux kernel. Also, the pinctrl drivers need to be moved to "drivers/pinctrl/qcom/" dir instead. > > > > > 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. That's true, it requires a u-boot driver update as above. > > > [...] > > 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? See my reply below. > > > 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? > Yeah it was intended to make sdm845 consistent with others. I will move it to a separate patch. > > - { .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 > Ack. -Sumit > Thanks! > Stephan
On Fri, Jul 15, 2022 at 03:21:45PM +0530, Sumit Garg wrote: > On Thu, 14 Jul 2022 at 23:45, Stephan Gerhold <stephan@gerhold.net> wrote: > > On Thu, Jul 14, 2022 at 01:03:37PM +0530, Sumit Garg wrote: > > > 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. > > > > Sorry but it's unfair to block new SoCs/boards support until all the > existing mess around DT is cleaned up in Qcom specific u-boot drivers. > This patch is a good example to demonstrate that all corresponding > boards DT need to be fixed as well which requires testing. And I don't > even have access to starqltechn, ipq4019 based board and db820c. > Sorry, I thought this is the only patch you need to use the Linux QCS404 DT as-is (maybe I misunderstood). I'm not expecting that you clean up all existing boards first of course. :) I just thought it would be nice to start clean for QCS404 immediately if this is the only patch you need. This patch looks simple enough for me if we test it on a couple of boards, the pinctrl setup is fairly similar across all of them. However, I wrote this before the comments with the additional "reg"s below. If we need to add handling for that as well the patch will need to become a bit larger of course, maybe too large to prepend it to your QCS404 series. > AFAIK, it's not a requirement yet but a recommendation at this stage > to import DT directly from Linux kernel and work with it. But I would > be very happy to work in this direction to make Qcom SoCs/boards DT > compliant. So I would request the merge of new boards support and then > we can follow up with patches like this one. > Thanks! I'm not familiar with the requirements so I'll leave this up to Tom to decide. :) > [...] > > > 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)? > > This is another example of a shortcut already used by the u-boot > pinctrl driver (arch/arm/mach-snapdragon/pinctrl-snapdragon.c). You > can find another user here (arch/arm/dts/sdm845.dtsi). So the pinctrl > drivers need to be converted to a format supported by Linux kernel. > Also, the pinctrl drivers need to be moved to "drivers/pinctrl/qcom/" > dir instead. > Right. FYI, there is started work on one possible solution for this here: https://github.com/minlexx/u-boot-2nd/commits/660 Basically Alexey (now in Cc) and Michael ported parts of the Linux pinctrl-msm driver to U-Boot, in a way that you can mostly just copy the platform specific definitions as-is. The additional memory regions are handled correctly there AFAICT. The code size overall is quite a bit higher of course, but I think this is not a problem for any of the Qualcomm boards in U-Boot. The ease of porting and flexibility should outweigh the cost here, I think. Thanks, Stephan
On Sat, 16 Jul 2022 at 18:54, Stephan Gerhold <stephan@gerhold.net> wrote: > > On Fri, Jul 15, 2022 at 03:21:45PM +0530, Sumit Garg wrote: > > On Thu, 14 Jul 2022 at 23:45, Stephan Gerhold <stephan@gerhold.net> wrote: > > > On Thu, Jul 14, 2022 at 01:03:37PM +0530, Sumit Garg wrote: > > > > 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. > > > > > > > Sorry but it's unfair to block new SoCs/boards support until all the > > existing mess around DT is cleaned up in Qcom specific u-boot drivers. > > This patch is a good example to demonstrate that all corresponding > > boards DT need to be fixed as well which requires testing. And I don't > > even have access to starqltechn, ipq4019 based board and db820c. > > > > Sorry, I thought this is the only patch you need to use the Linux QCS404 > DT as-is (maybe I misunderstood). I'm not expecting that you clean up > all existing boards first of course. :) I just thought it would be nice > to start clean for QCS404 immediately if this is the only patch you need. > > This patch looks simple enough for me if we test it on a couple of > boards, the pinctrl setup is fairly similar across all of them. However, > I wrote this before the comments with the additional "reg"s below. If we > need to add handling for that as well the patch will need to become a > bit larger of course, maybe too large to prepend it to your QCS404 > series. Yeah especially the test dependency makes it cumbersome to prepend it to QCS404 series. > > > AFAIK, it's not a requirement yet but a recommendation at this stage > > to import DT directly from Linux kernel and work with it. But I would > > be very happy to work in this direction to make Qcom SoCs/boards DT > > compliant. So I would request the merge of new boards support and then > > we can follow up with patches like this one. > > > > Thanks! I'm not familiar with the requirements so I'll leave this up to > Tom to decide. :) > > > [...] > > > > 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)? > > > > This is another example of a shortcut already used by the u-boot > > pinctrl driver (arch/arm/mach-snapdragon/pinctrl-snapdragon.c). You > > can find another user here (arch/arm/dts/sdm845.dtsi). So the pinctrl > > drivers need to be converted to a format supported by Linux kernel. > > Also, the pinctrl drivers need to be moved to "drivers/pinctrl/qcom/" > > dir instead. > > > > Right. FYI, there is started work on one possible solution for this > here: https://github.com/minlexx/u-boot-2nd/commits/660 > > Basically Alexey (now in Cc) and Michael ported parts of the Linux > pinctrl-msm driver to U-Boot, in a way that you can mostly just copy the > platform specific definitions as-is. The additional memory regions are > handled correctly there AFAICT. > > The code size overall is quite a bit higher of course, but I think this > is not a problem for any of the Qualcomm boards in U-Boot. The ease of > porting and flexibility should outweigh the cost here, I think. I had a brief look at this WIP patch-set but in general this should be the direction we should pursue longer term. Although I think the platform specific definitions could be limited to what's actually required for u-boot to function properly rather than adding redundant code. I think in a similar manner we need to generalize Qcom clock drivers as well (move from arch/arm/mach-snapdragon/clock-* -> drivers/clk/qcom/). I would be happy to review this patch-set once it's posted upstream and would be able to port platforms which I have access to using a generic pinctrl and clk framework. -Sumit > > Thanks, > Stephan
On Thu, Jul 14, 2022 at 10:33 AM Sumit Garg <sumit.garg@linaro.org> 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 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 > > 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-uboot.dtsi b/arch/arm/dts/dragonboard410c-uboot.dtsi > index 9c1be2566f..e4fecaa19e 100644 > --- a/arch/arm/dts/dragonboard410c-uboot.dtsi > +++ b/arch/arm/dts/dragonboard410c-uboot.dtsi > @@ -14,7 +14,7 @@ > soc { > u-boot,dm-pre-reloc; > > - qcom,tlmm@1000000 { > + pinctrl@1000000 { > u-boot,dm-pre-reloc; > > uart { > 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"; > 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-uboot.dtsi b/arch/arm/dts/dragonboard820c-uboot.dtsi > index 8610d7ec37..2270ac73bf 100644 > --- a/arch/arm/dts/dragonboard820c-uboot.dtsi > +++ b/arch/arm/dts/dragonboard820c-uboot.dtsi > @@ -13,7 +13,7 @@ > soc { > u-boot,dm-pre-reloc; > > - qcom,tlmm@1010000 { > + pinctrl@1010000 { > u-boot,dm-pre-reloc; > > uart { > 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"; > reg = <0x1010000 0x400000>; > > blsp8_uart: uart { > diff --git a/arch/arm/dts/qcom-ipq4019.dtsi b/arch/arm/dts/qcom-ipq4019.dtsi > index 7a52ea2c4e..181732d262 100644 > --- a/arch/arm/dts/qcom-ipq4019.dtsi > +++ b/arch/arm/dts/qcom-ipq4019.dtsi > @@ -75,9 +75,13 @@ > u-boot,dm-pre-reloc; > }; > > - pinctrl: qcom,tlmm@1000000 { > - compatible = "qcom,tlmm-ipq4019"; > + soc_gpios: pinctrl@1000000 { > + compatible = "qcom,ipq4019-pinctrl"; > reg = <0x1000000 0x300000>; > + gpio-controller; > + gpio-count = <100>; > + gpio-bank-name="soc"; > + #gpio-cells = <2>; > u-boot,dm-pre-reloc; > }; > > @@ -90,16 +94,6 @@ > u-boot,dm-pre-reloc; > }; > > - soc_gpios: pinctrl@1000000 { > - compatible = "qcom,ipq4019-pinctrl"; > - reg = <0x1000000 0x300000>; > - gpio-controller; > - gpio-count = <100>; > - gpio-bank-name="soc"; > - #gpio-cells = <2>; > - u-boot,dm-pre-reloc; > - }; > - > blsp1_spi1: spi@78b5000 { > compatible = "qcom,spi-qup-v2.2.1"; > reg = <0x78b5000 0x600>; > 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>; > > 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>; > gpio-count = <150>; > gpio-controller; > diff --git a/arch/arm/mach-ipq40xx/pinctrl-snapdragon.c b/arch/arm/mach-ipq40xx/pinctrl-snapdragon.c > index c51a75ee94..036fec93d7 100644 > --- a/arch/arm/mach-ipq40xx/pinctrl-snapdragon.c > +++ b/arch/arm/mach-ipq40xx/pinctrl-snapdragon.c > @@ -14,6 +14,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" > @@ -110,6 +112,32 @@ static int msm_pinconf_set(struct udevice *dev, unsigned int pin_selector, > return 0; > } > > +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 struct pinctrl_ops msm_pinctrl_ops = { > .get_pins_count = msm_get_pins_count, > .get_pin_name = msm_get_pin_name, > @@ -123,7 +151,7 @@ static struct pinctrl_ops msm_pinctrl_ops = { > }; > > static const struct udevice_id msm_pinctrl_ids[] = { > - { .compatible = "qcom,tlmm-ipq4019", .data = (ulong)&ipq4019_data }, > + { .compatible = "qcom,ipq4019-pinctrl", .data = (ulong)&ipq4019_data }, > { } > }; > > @@ -134,4 +162,5 @@ U_BOOT_DRIVER(pinctrl_snapdraon) = { > .priv_auto = sizeof(struct msm_pinctrl_priv), > .ops = &msm_pinctrl_ops, > .probe = msm_pinctrl_probe, > + .bind = msm_pinctrl_bind, > }; > 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 > 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 > - { .compatible = "qcom,tlmm-qcs404", .data = (ulong)&qcs404_data }, > + { .compatible = "qcom,apq8016-pinctrl", .data = (ulong)&apq8016_data }, > + { .compatible = "qcom,msm8916-pinctrl", .data = (ulong)&apq8096_data }, > + { .compatible = "qcom,sdm845-pinctrl", .data = (ulong)&sdm845_data }, > + { .compatible = "qcom,qcs404-pinctrl", .data = (ulong)&qcs404_data }, > { } > }; > > @@ -130,4 +156,5 @@ U_BOOT_DRIVER(pinctrl_snapdraon) = { > .priv_auto = sizeof(struct msm_pinctrl_priv), > .ops = &msm_pinctrl_ops, > .probe = msm_pinctrl_probe, > + .bind = msm_pinctrl_bind, > }; > diff --git a/drivers/gpio/msm_gpio.c b/drivers/gpio/msm_gpio.c > index a3c3cd7824..51670f2637 100644 > --- a/drivers/gpio/msm_gpio.c > +++ b/drivers/gpio/msm_gpio.c > @@ -116,20 +116,12 @@ static int msm_gpio_of_to_plat(struct udevice *dev) > return 0; > } > > -static const struct udevice_id msm_gpio_ids[] = { > - { .compatible = "qcom,msm8916-pinctrl" }, > - { .compatible = "qcom,apq8016-pinctrl" }, > - { .compatible = "qcom,ipq4019-pinctrl" }, > - { .compatible = "qcom,sdm845-pinctrl" }, > - { } > -}; > - > U_BOOT_DRIVER(gpio_msm) = { > .name = "gpio_msm", > .id = UCLASS_GPIO, > - .of_match = msm_gpio_ids, > .of_to_plat = msm_gpio_of_to_plat, > .probe = msm_gpio_probe, > .ops = &gpio_msm_ops, > + .flags = DM_UC_FLAG_SEQ_ALIAS, > .priv_auto = sizeof(struct msm_gpio_bank), > }; > -- > 2.25.1 > I will test it on db410c, the db820c I currently have is dead.
On Sun, 7 Aug 2022 at 05:50, Ramon Fried <rfried.dev@gmail.com> wrote: > > On Thu, Jul 14, 2022 at 10:33 AM Sumit Garg <sumit.garg@linaro.org> 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 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 > > > > 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-uboot.dtsi b/arch/arm/dts/dragonboard410c-uboot.dtsi > > index 9c1be2566f..e4fecaa19e 100644 > > --- a/arch/arm/dts/dragonboard410c-uboot.dtsi > > +++ b/arch/arm/dts/dragonboard410c-uboot.dtsi > > @@ -14,7 +14,7 @@ > > soc { > > u-boot,dm-pre-reloc; > > > > - qcom,tlmm@1000000 { > > + pinctrl@1000000 { > > u-boot,dm-pre-reloc; > > > > uart { > > 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"; > > 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-uboot.dtsi b/arch/arm/dts/dragonboard820c-uboot.dtsi > > index 8610d7ec37..2270ac73bf 100644 > > --- a/arch/arm/dts/dragonboard820c-uboot.dtsi > > +++ b/arch/arm/dts/dragonboard820c-uboot.dtsi > > @@ -13,7 +13,7 @@ > > soc { > > u-boot,dm-pre-reloc; > > > > - qcom,tlmm@1010000 { > > + pinctrl@1010000 { > > u-boot,dm-pre-reloc; > > > > uart { > > 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"; > > reg = <0x1010000 0x400000>; > > > > blsp8_uart: uart { > > diff --git a/arch/arm/dts/qcom-ipq4019.dtsi b/arch/arm/dts/qcom-ipq4019.dtsi > > index 7a52ea2c4e..181732d262 100644 > > --- a/arch/arm/dts/qcom-ipq4019.dtsi > > +++ b/arch/arm/dts/qcom-ipq4019.dtsi > > @@ -75,9 +75,13 @@ > > u-boot,dm-pre-reloc; > > }; > > > > - pinctrl: qcom,tlmm@1000000 { > > - compatible = "qcom,tlmm-ipq4019"; > > + soc_gpios: pinctrl@1000000 { > > + compatible = "qcom,ipq4019-pinctrl"; > > reg = <0x1000000 0x300000>; > > + gpio-controller; > > + gpio-count = <100>; > > + gpio-bank-name="soc"; > > + #gpio-cells = <2>; > > u-boot,dm-pre-reloc; > > }; > > > > @@ -90,16 +94,6 @@ > > u-boot,dm-pre-reloc; > > }; > > > > - soc_gpios: pinctrl@1000000 { > > - compatible = "qcom,ipq4019-pinctrl"; > > - reg = <0x1000000 0x300000>; > > - gpio-controller; > > - gpio-count = <100>; > > - gpio-bank-name="soc"; > > - #gpio-cells = <2>; > > - u-boot,dm-pre-reloc; > > - }; > > - > > blsp1_spi1: spi@78b5000 { > > compatible = "qcom,spi-qup-v2.2.1"; > > reg = <0x78b5000 0x600>; > > 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>; > > > > 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>; > > gpio-count = <150>; > > gpio-controller; > > diff --git a/arch/arm/mach-ipq40xx/pinctrl-snapdragon.c b/arch/arm/mach-ipq40xx/pinctrl-snapdragon.c > > index c51a75ee94..036fec93d7 100644 > > --- a/arch/arm/mach-ipq40xx/pinctrl-snapdragon.c > > +++ b/arch/arm/mach-ipq40xx/pinctrl-snapdragon.c > > @@ -14,6 +14,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" > > @@ -110,6 +112,32 @@ static int msm_pinconf_set(struct udevice *dev, unsigned int pin_selector, > > return 0; > > } > > > > +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 struct pinctrl_ops msm_pinctrl_ops = { > > .get_pins_count = msm_get_pins_count, > > .get_pin_name = msm_get_pin_name, > > @@ -123,7 +151,7 @@ static struct pinctrl_ops msm_pinctrl_ops = { > > }; > > > > static const struct udevice_id msm_pinctrl_ids[] = { > > - { .compatible = "qcom,tlmm-ipq4019", .data = (ulong)&ipq4019_data }, > > + { .compatible = "qcom,ipq4019-pinctrl", .data = (ulong)&ipq4019_data }, > > { } > > }; > > > > @@ -134,4 +162,5 @@ U_BOOT_DRIVER(pinctrl_snapdraon) = { > > .priv_auto = sizeof(struct msm_pinctrl_priv), > > .ops = &msm_pinctrl_ops, > > .probe = msm_pinctrl_probe, > > + .bind = msm_pinctrl_bind, > > }; > > 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 > > 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 > > - { .compatible = "qcom,tlmm-qcs404", .data = (ulong)&qcs404_data }, > > + { .compatible = "qcom,apq8016-pinctrl", .data = (ulong)&apq8016_data }, > > + { .compatible = "qcom,msm8916-pinctrl", .data = (ulong)&apq8096_data }, > > + { .compatible = "qcom,sdm845-pinctrl", .data = (ulong)&sdm845_data }, > > + { .compatible = "qcom,qcs404-pinctrl", .data = (ulong)&qcs404_data }, > > { } > > }; > > > > @@ -130,4 +156,5 @@ U_BOOT_DRIVER(pinctrl_snapdraon) = { > > .priv_auto = sizeof(struct msm_pinctrl_priv), > > .ops = &msm_pinctrl_ops, > > .probe = msm_pinctrl_probe, > > + .bind = msm_pinctrl_bind, > > }; > > diff --git a/drivers/gpio/msm_gpio.c b/drivers/gpio/msm_gpio.c > > index a3c3cd7824..51670f2637 100644 > > --- a/drivers/gpio/msm_gpio.c > > +++ b/drivers/gpio/msm_gpio.c > > @@ -116,20 +116,12 @@ static int msm_gpio_of_to_plat(struct udevice *dev) > > return 0; > > } > > > > -static const struct udevice_id msm_gpio_ids[] = { > > - { .compatible = "qcom,msm8916-pinctrl" }, > > - { .compatible = "qcom,apq8016-pinctrl" }, > > - { .compatible = "qcom,ipq4019-pinctrl" }, > > - { .compatible = "qcom,sdm845-pinctrl" }, > > - { } > > -}; > > - > > U_BOOT_DRIVER(gpio_msm) = { > > .name = "gpio_msm", > > .id = UCLASS_GPIO, > > - .of_match = msm_gpio_ids, > > .of_to_plat = msm_gpio_of_to_plat, > > .probe = msm_gpio_probe, > > .ops = &gpio_msm_ops, > > + .flags = DM_UC_FLAG_SEQ_ALIAS, > > .priv_auto = sizeof(struct msm_gpio_bank), > > }; > > -- > > 2.25.1 > > > I will test it on db410c, the db820c I currently have is dead. Thanks, looking forward to your tested-by tag. -Sumit
diff --git a/arch/arm/dts/dragonboard410c-uboot.dtsi b/arch/arm/dts/dragonboard410c-uboot.dtsi index 9c1be2566f..e4fecaa19e 100644 --- a/arch/arm/dts/dragonboard410c-uboot.dtsi +++ b/arch/arm/dts/dragonboard410c-uboot.dtsi @@ -14,7 +14,7 @@ soc { u-boot,dm-pre-reloc; - qcom,tlmm@1000000 { + pinctrl@1000000 { u-boot,dm-pre-reloc; uart { 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"; 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-uboot.dtsi b/arch/arm/dts/dragonboard820c-uboot.dtsi index 8610d7ec37..2270ac73bf 100644 --- a/arch/arm/dts/dragonboard820c-uboot.dtsi +++ b/arch/arm/dts/dragonboard820c-uboot.dtsi @@ -13,7 +13,7 @@ soc { u-boot,dm-pre-reloc; - qcom,tlmm@1010000 { + pinctrl@1010000 { u-boot,dm-pre-reloc; uart { 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"; reg = <0x1010000 0x400000>; blsp8_uart: uart { diff --git a/arch/arm/dts/qcom-ipq4019.dtsi b/arch/arm/dts/qcom-ipq4019.dtsi index 7a52ea2c4e..181732d262 100644 --- a/arch/arm/dts/qcom-ipq4019.dtsi +++ b/arch/arm/dts/qcom-ipq4019.dtsi @@ -75,9 +75,13 @@ u-boot,dm-pre-reloc; }; - pinctrl: qcom,tlmm@1000000 { - compatible = "qcom,tlmm-ipq4019"; + soc_gpios: pinctrl@1000000 { + compatible = "qcom,ipq4019-pinctrl"; reg = <0x1000000 0x300000>; + gpio-controller; + gpio-count = <100>; + gpio-bank-name="soc"; + #gpio-cells = <2>; u-boot,dm-pre-reloc; }; @@ -90,16 +94,6 @@ u-boot,dm-pre-reloc; }; - soc_gpios: pinctrl@1000000 { - compatible = "qcom,ipq4019-pinctrl"; - reg = <0x1000000 0x300000>; - gpio-controller; - gpio-count = <100>; - gpio-bank-name="soc"; - #gpio-cells = <2>; - u-boot,dm-pre-reloc; - }; - blsp1_spi1: spi@78b5000 { compatible = "qcom,spi-qup-v2.2.1"; reg = <0x78b5000 0x600>; 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>; 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>; gpio-count = <150>; gpio-controller; diff --git a/arch/arm/mach-ipq40xx/pinctrl-snapdragon.c b/arch/arm/mach-ipq40xx/pinctrl-snapdragon.c index c51a75ee94..036fec93d7 100644 --- a/arch/arm/mach-ipq40xx/pinctrl-snapdragon.c +++ b/arch/arm/mach-ipq40xx/pinctrl-snapdragon.c @@ -14,6 +14,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" @@ -110,6 +112,32 @@ static int msm_pinconf_set(struct udevice *dev, unsigned int pin_selector, return 0; } +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 struct pinctrl_ops msm_pinctrl_ops = { .get_pins_count = msm_get_pins_count, .get_pin_name = msm_get_pin_name, @@ -123,7 +151,7 @@ static struct pinctrl_ops msm_pinctrl_ops = { }; static const struct udevice_id msm_pinctrl_ids[] = { - { .compatible = "qcom,tlmm-ipq4019", .data = (ulong)&ipq4019_data }, + { .compatible = "qcom,ipq4019-pinctrl", .data = (ulong)&ipq4019_data }, { } }; @@ -134,4 +162,5 @@ U_BOOT_DRIVER(pinctrl_snapdraon) = { .priv_auto = sizeof(struct msm_pinctrl_priv), .ops = &msm_pinctrl_ops, .probe = msm_pinctrl_probe, + .bind = msm_pinctrl_bind, }; 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 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 - { .compatible = "qcom,tlmm-qcs404", .data = (ulong)&qcs404_data }, + { .compatible = "qcom,apq8016-pinctrl", .data = (ulong)&apq8016_data }, + { .compatible = "qcom,msm8916-pinctrl", .data = (ulong)&apq8096_data }, + { .compatible = "qcom,sdm845-pinctrl", .data = (ulong)&sdm845_data }, + { .compatible = "qcom,qcs404-pinctrl", .data = (ulong)&qcs404_data }, { } }; @@ -130,4 +156,5 @@ U_BOOT_DRIVER(pinctrl_snapdraon) = { .priv_auto = sizeof(struct msm_pinctrl_priv), .ops = &msm_pinctrl_ops, .probe = msm_pinctrl_probe, + .bind = msm_pinctrl_bind, }; diff --git a/drivers/gpio/msm_gpio.c b/drivers/gpio/msm_gpio.c index a3c3cd7824..51670f2637 100644 --- a/drivers/gpio/msm_gpio.c +++ b/drivers/gpio/msm_gpio.c @@ -116,20 +116,12 @@ static int msm_gpio_of_to_plat(struct udevice *dev) return 0; } -static const struct udevice_id msm_gpio_ids[] = { - { .compatible = "qcom,msm8916-pinctrl" }, - { .compatible = "qcom,apq8016-pinctrl" }, - { .compatible = "qcom,ipq4019-pinctrl" }, - { .compatible = "qcom,sdm845-pinctrl" }, - { } -}; - U_BOOT_DRIVER(gpio_msm) = { .name = "gpio_msm", .id = UCLASS_GPIO, - .of_match = msm_gpio_ids, .of_to_plat = msm_gpio_of_to_plat, .probe = msm_gpio_probe, .ops = &gpio_msm_ops, + .flags = DM_UC_FLAG_SEQ_ALIAS, .priv_auto = sizeof(struct msm_gpio_bank), };
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 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 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(-)