| Message ID | 20180125163216.29018-3-rnayak@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On 01/25, Rajendra Nayak wrote: > diff --git a/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi > new file mode 100644 > index 000000000000..b97f99e6f4b4 > --- /dev/null > +++ b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi > @@ -0,0 +1,32 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (c) 2018, The Linux Foundation. All rights reserved. > + */ > + > +&tlmm { I'm not the maintainer, but I find this approach to the pins really annoying. I have to flip to another file to figure out how a board has configured the pins. And we may bring in a bunch of settings that we don't ever use on some board too. Why can't we put the settings in the board file directly? > + qup_uart2_default: qup_uart2_default { > + pinmux { > + function = "qup9"; > + pins = "gpio4", "gpio5"; > + }; > + > + pinconf { > + pins = "gpio4", "gpio5"; > + drive-strength = <2>; > + bias-disable; > + }; > + }; > + > + qup_uart2_sleep: qup_uart2_sleep { > + pinmux { > + function = "gpio"; > + pins = "gpio4", "gpio5"; > + }; > + > + pinconf { > + pins = "gpio4", "gpio5"; > + drive-strength = <2>; > + bias-disable; > + }; > + }; > +}; > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi > index a21f4912b3e2..529f4ba3a1db 100644 > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi > @@ -4,6 +4,7 @@ > */ > > #include <dt-bindings/interrupt-controller/arm-gic.h> > +#include <dt-bindings/clock/qcom,gcc-sdm845.h> > > / { > model = "Qualcomm Technologies, Inc. SDM845"; I'd expect some sort of serial alias node stuff here too. > @@ -304,5 +305,26 @@ > cell-index = <0>; > }; > > + qup_1: qcom,geni_se@ac0000 { > + compatible = "qcom,geni-se-qup"; > + reg = <0xac0000 0x6000>; > + }; > + > + qup_uart2: serial@a84000 { > + compatible = "qcom,geni-console", "qcom,geni-uart"; > + reg = <0xa84000 0x4000>; > + reg-names = "se_phys"; > + clock-names = "se-clk", "m-ahb", "s-ahb"; > + clocks = <&gcc GCC_QUPV3_WRAP1_S1_CLK>, > + <&gcc GCC_QUPV3_WRAP_1_M_AHB_CLK>, > + <&gcc GCC_QUPV3_WRAP_1_S_AHB_CLK>; > + pinctrl-names = "default", "sleep"; > + pinctrl-0 = <&qup_uart2_default>; > + pinctrl-1 = <&qup_uart2_sleep>; > + interrupts = <GIC_SPI 354 IRQ_TYPE_NONE>; > + qcom,wrapper-core = <&qup_1>; Is this binding still being reviewed? Ugly. > + status = "disabled"; > + }; > }; > }; > +#include "sdm845-pins.dtsi" Why at the bottom?
On 01/27/2018 03:48 AM, Stephen Boyd wrote: > On 01/25, Rajendra Nayak wrote: >> diff --git a/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi >> new file mode 100644 >> index 000000000000..b97f99e6f4b4 >> --- /dev/null >> +++ b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi >> @@ -0,0 +1,32 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2018, The Linux Foundation. All rights reserved. >> + */ >> + >> +&tlmm { > > I'm not the maintainer, but I find this approach to the pins > really annoying. I have to flip to another file to figure out how > a board has configured the pins. And we may bring in a bunch of > settings that we don't ever use on some board too. Why can't we > put the settings in the board file directly? I was just keeping it consistent with how things are for other qualcomm platforms. I can move this to the board dts if no one else sees any issues. > >> + qup_uart2_default: qup_uart2_default { >> + pinmux { >> + function = "qup9"; >> + pins = "gpio4", "gpio5"; >> + }; >> + >> + pinconf { >> + pins = "gpio4", "gpio5"; >> + drive-strength = <2>; >> + bias-disable; >> + }; >> + }; >> + >> + qup_uart2_sleep: qup_uart2_sleep { >> + pinmux { >> + function = "gpio"; >> + pins = "gpio4", "gpio5"; >> + }; >> + >> + pinconf { >> + pins = "gpio4", "gpio5"; >> + drive-strength = <2>; >> + bias-disable; >> + }; >> + }; >> +}; >> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi >> index a21f4912b3e2..529f4ba3a1db 100644 >> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi >> @@ -4,6 +4,7 @@ >> */ >> >> #include <dt-bindings/interrupt-controller/arm-gic.h> >> +#include <dt-bindings/clock/qcom,gcc-sdm845.h> >> >> / { >> model = "Qualcomm Technologies, Inc. SDM845"; > > I'd expect some sort of serial alias node stuff here too. yes, will add. > >> @@ -304,5 +305,26 @@ >> cell-index = <0>; >> }; >> >> + qup_1: qcom,geni_se@ac0000 { >> + compatible = "qcom,geni-se-qup"; >> + reg = <0xac0000 0x6000>; >> + }; >> + >> + qup_uart2: serial@a84000 { >> + compatible = "qcom,geni-console", "qcom,geni-uart"; >> + reg = <0xa84000 0x4000>; >> + reg-names = "se_phys"; >> + clock-names = "se-clk", "m-ahb", "s-ahb"; >> + clocks = <&gcc GCC_QUPV3_WRAP1_S1_CLK>, >> + <&gcc GCC_QUPV3_WRAP_1_M_AHB_CLK>, >> + <&gcc GCC_QUPV3_WRAP_1_S_AHB_CLK>; >> + pinctrl-names = "default", "sleep"; >> + pinctrl-0 = <&qup_uart2_default>; >> + pinctrl-1 = <&qup_uart2_sleep>; >> + interrupts = <GIC_SPI 354 IRQ_TYPE_NONE>; >> + qcom,wrapper-core = <&qup_1>; > > Is this binding still being reviewed? Ugly. yes, its still being reviewed > >> + status = "disabled"; >> + }; >> }; >> }; >> +#include "sdm845-pins.dtsi" > > Why at the bottom? Again keeping it consistent with how things are in msm8916/msm8992/msm8996 dtsi files.
Hi, On Fri, Jan 26, 2018 at 2:18 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 01/25, Rajendra Nayak wrote: >> diff --git a/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi >> new file mode 100644 >> index 000000000000..b97f99e6f4b4 >> --- /dev/null >> +++ b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi >> @@ -0,0 +1,32 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2018, The Linux Foundation. All rights reserved. >> + */ >> + >> +&tlmm { > > I'm not the maintainer, but I find this approach to the pins > really annoying. I have to flip to another file to figure out how > a board has configured the pins. And we may bring in a bunch of > settings that we don't ever use on some board too. Why can't we > put the settings in the board file directly? I'm not so familiar with how things work with Qualcomm, but in general I think putting this in the "board" file is a bad idea. I'd be OK with putting this directly in the SoC file (though it might get unwieldy?), but not moving things to the board file as was done with v2 of this patch. Said another way: nearly board that uses SDM845 that uses UART2 will have the same definitions for these pins so we shouldn't be duplicating it across every board, right? I'll also respond to the v2 patch so it's obvious there is feedback there... -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon 29 Jan 00:18 PST 2018, Rajendra Nayak wrote: > On 01/27/2018 03:48 AM, Stephen Boyd wrote: > > On 01/25, Rajendra Nayak wrote: > >> diff --git a/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi > >> new file mode 100644 > >> index 000000000000..b97f99e6f4b4 > >> --- /dev/null > >> +++ b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi > >> @@ -0,0 +1,32 @@ > >> +// SPDX-License-Identifier: GPL-2.0 > >> +/* > >> + * Copyright (c) 2018, The Linux Foundation. All rights reserved. > >> + */ > >> + > >> +&tlmm { > > > > I'm not the maintainer, but I find this approach to the pins > > really annoying. I have to flip to another file to figure out how > > a board has configured the pins. And we may bring in a bunch of > > settings that we don't ever use on some board too. Why can't we > > put the settings in the board file directly? > > I was just keeping it consistent with how things are for other > qualcomm platforms. I can move this to the board dts if no one else > sees any issues. > What we decided recently for 8916 (at least) was that we define the functional properties in the soc dtsi and add the electrical properties in the board dts(i). I fully agree with Stephen on this one, but as you say we're keeping the pinconf in separate files for the other platforms/boards. I'll prepare and bring some new guidelines to our Thursday meeting and we can decide what to do based on that. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue 06 Feb 10:37 PST 2018, Doug Anderson wrote: > Hi, > > On Fri, Jan 26, 2018 at 2:18 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > > On 01/25, Rajendra Nayak wrote: > >> diff --git a/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi > >> new file mode 100644 > >> index 000000000000..b97f99e6f4b4 > >> --- /dev/null > >> +++ b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi > >> @@ -0,0 +1,32 @@ > >> +// SPDX-License-Identifier: GPL-2.0 > >> +/* > >> + * Copyright (c) 2018, The Linux Foundation. All rights reserved. > >> + */ > >> + > >> +&tlmm { > > > > I'm not the maintainer, but I find this approach to the pins > > really annoying. I have to flip to another file to figure out how > > a board has configured the pins. And we may bring in a bunch of > > settings that we don't ever use on some board too. Why can't we > > put the settings in the board file directly? > > I'm not so familiar with how things work with Qualcomm, but in general > I think putting this in the "board" file is a bad idea. I'd be OK > with putting this directly in the SoC file (though it might get > unwieldy?), but not moving things to the board file as was done with > v2 of this patch. > > Said another way: nearly board that uses SDM845 that uses UART2 will > have the same definitions for these pins so we shouldn't be > duplicating it across every board, right? > We've run into several cases where different boards uses the same function but requires board specific electrical configuration. So what we decided was to keep the pinmux in the soc-file (where e.g. the uart definition is) and then extend it with the board specific electrical properties (the pinconf), in the board files. This does come with the complexity of having the pinctrl nodes split in two places, but the responsibilities of the two parts is clear and we remove the need for all board files to ensure the appropriate pinmux is in place. NB. We did discuss adding "sane defaults" for the pinconf in the soc dtsi, but we end up spending considerable time debugging issues stemming from not having the right pinconf; so better make this explicit and say that the board has to specify it's config. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Feb 6, 2018 at 11:06 AM, Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > On Tue 06 Feb 10:37 PST 2018, Doug Anderson wrote: > >> Hi, >> >> On Fri, Jan 26, 2018 at 2:18 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >> > On 01/25, Rajendra Nayak wrote: >> >> diff --git a/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi >> >> new file mode 100644 >> >> index 000000000000..b97f99e6f4b4 >> >> --- /dev/null >> >> +++ b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi >> >> @@ -0,0 +1,32 @@ >> >> +// SPDX-License-Identifier: GPL-2.0 >> >> +/* >> >> + * Copyright (c) 2018, The Linux Foundation. All rights reserved. >> >> + */ >> >> + >> >> +&tlmm { >> > >> > I'm not the maintainer, but I find this approach to the pins >> > really annoying. I have to flip to another file to figure out how >> > a board has configured the pins. And we may bring in a bunch of >> > settings that we don't ever use on some board too. Why can't we >> > put the settings in the board file directly? >> >> I'm not so familiar with how things work with Qualcomm, but in general >> I think putting this in the "board" file is a bad idea. I'd be OK >> with putting this directly in the SoC file (though it might get >> unwieldy?), but not moving things to the board file as was done with >> v2 of this patch. >> >> Said another way: nearly board that uses SDM845 that uses UART2 will >> have the same definitions for these pins so we shouldn't be >> duplicating it across every board, right? >> > > We've run into several cases where different boards uses the same > function but requires board specific electrical configuration. > > So what we decided was to keep the pinmux in the soc-file (where e.g. > the uart definition is) and then extend it with the board specific > electrical properties (the pinconf), in the board files. > > This does come with the complexity of having the pinctrl nodes split in > two places, but the responsibilities of the two parts is clear and we > remove the need for all board files to ensure the appropriate pinmux is > in place. > > > NB. We did discuss adding "sane defaults" for the pinconf in the soc > dtsi, but we end up spending considerable time debugging issues stemming > from not having the right pinconf; so better make this explicit and say > that the board has to specify it's config. Whoops, saw your responses _after_ I sent my response to v2. In any case this makes sense to me then! On Rockchip boards I've been involved in we often added "sane defaults", but I can see how that could be confusing in different ways. I'm happy with your choice and it seems like a happy medium. The sdm845.dtsi file can have the main definition of the nodes and can thus refer to the nodes. Then you just add the extra bit in the board file. What you propose is not what happened in v2 of the series <https://patchwork.kernel.org/patch/10194201/> though. In v2 _both_ the pinconf and the pinmux moved to the board file. That's wrong. To make it concrete, you'd have something like this (this has the wrong bindings from the UART, but folks get the picture hopefully): In sdm845.dtsi: qup_uart2: serial@a84000 { compatible = "qcom,geni-console", "qcom,geni-uart"; reg = <0xa84000 0x4000>; reg-names = "se_phys"; clock-names = "se-clk", "m-ahb", "s-ahb"; clocks = <&gcc GCC_QUPV3_WRAP1_S1_CLK>, <&gcc GCC_QUPV3_WRAP_1_M_AHB_CLK>, <&gcc GCC_QUPV3_WRAP_1_S_AHB_CLK>; pinctrl-names = "default", "sleep"; pinctrl-0 = <&qup_uart2_default>; pinctrl-1 = <&qup_uart2_sleep>; interrupts = <GIC_SPI 354 IRQ_TYPE_NONE>; qcom,wrapper-core = <&qup_1>; status = "disabled"; }; tlmm: pinctrl@3400000 { compatible = "qcom,sdm845-pinctrl"; reg = <0x03400000 0xc00000>; interrupts = <GIC_SPI 208 IRQ_TYPE_NONE>; gpio-controller; #gpio-cells = <2>; interrupt-controller; #interrupt-cells = <2>; qup_uart2_default: qup_uart2_default { pinmux { function = "qup9"; pins = "gpio4", "gpio5"; }; }; qup_uart2_sleep: qup_uart2_sleep { pinmux { function = "gpio"; pins = "gpio4", "gpio5"; }; }; }; In sdm845-mtp.dts: &qup_uart2_default { pinconf { pins = "gpio4", "gpio5"; drive-strength = <2>; bias-disable; }; }; &qup_uart2_sleep { pinconf { pins = "gpio4", "gpio5"; drive-strength = <2>; bias-disable; }; }; -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue 06 Feb 11:49 PST 2018, Doug Anderson wrote: > Hi, > > On Tue, Feb 6, 2018 at 11:06 AM, Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: > > On Tue 06 Feb 10:37 PST 2018, Doug Anderson wrote: > > > >> Hi, > >> > >> On Fri, Jan 26, 2018 at 2:18 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > >> > On 01/25, Rajendra Nayak wrote: > >> >> diff --git a/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi > >> >> new file mode 100644 > >> >> index 000000000000..b97f99e6f4b4 > >> >> --- /dev/null > >> >> +++ b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi > >> >> @@ -0,0 +1,32 @@ > >> >> +// SPDX-License-Identifier: GPL-2.0 > >> >> +/* > >> >> + * Copyright (c) 2018, The Linux Foundation. All rights reserved. > >> >> + */ > >> >> + > >> >> +&tlmm { > >> > > >> > I'm not the maintainer, but I find this approach to the pins > >> > really annoying. I have to flip to another file to figure out how > >> > a board has configured the pins. And we may bring in a bunch of > >> > settings that we don't ever use on some board too. Why can't we > >> > put the settings in the board file directly? > >> > >> I'm not so familiar with how things work with Qualcomm, but in general > >> I think putting this in the "board" file is a bad idea. I'd be OK > >> with putting this directly in the SoC file (though it might get > >> unwieldy?), but not moving things to the board file as was done with > >> v2 of this patch. > >> > >> Said another way: nearly board that uses SDM845 that uses UART2 will > >> have the same definitions for these pins so we shouldn't be > >> duplicating it across every board, right? > >> > > > > We've run into several cases where different boards uses the same > > function but requires board specific electrical configuration. > > > > So what we decided was to keep the pinmux in the soc-file (where e.g. > > the uart definition is) and then extend it with the board specific > > electrical properties (the pinconf), in the board files. > > > > This does come with the complexity of having the pinctrl nodes split in > > two places, but the responsibilities of the two parts is clear and we > > remove the need for all board files to ensure the appropriate pinmux is > > in place. > > > > > > NB. We did discuss adding "sane defaults" for the pinconf in the soc > > dtsi, but we end up spending considerable time debugging issues stemming > > from not having the right pinconf; so better make this explicit and say > > that the board has to specify it's config. > > Whoops, saw your responses _after_ I sent my response to v2. In any > case this makes sense to me then! On Rockchip boards I've been > involved in we often added "sane defaults", but I can see how that > could be confusing in different ways. I'm happy with your choice and > it seems like a happy medium. The sdm845.dtsi file can have the main > definition of the nodes and can thus refer to the nodes. Then you > just add the extra bit in the board file. > > What you propose is not what happened in v2 of the series > <https://patchwork.kernel.org/patch/10194201/> though. In v2 _both_ > the pinconf and the pinmux moved to the board file. That's wrong. > > > To make it concrete, you'd have something like this (this has the > wrong bindings from the UART, but folks get the picture hopefully): > > > In sdm845.dtsi: > > qup_uart2: serial@a84000 { > compatible = "qcom,geni-console", "qcom,geni-uart"; > reg = <0xa84000 0x4000>; > reg-names = "se_phys"; > clock-names = "se-clk", "m-ahb", "s-ahb"; > clocks = <&gcc GCC_QUPV3_WRAP1_S1_CLK>, > <&gcc GCC_QUPV3_WRAP_1_M_AHB_CLK>, > <&gcc GCC_QUPV3_WRAP_1_S_AHB_CLK>; > pinctrl-names = "default", "sleep"; > pinctrl-0 = <&qup_uart2_default>; > pinctrl-1 = <&qup_uart2_sleep>; > interrupts = <GIC_SPI 354 IRQ_TYPE_NONE>; > qcom,wrapper-core = <&qup_1>; > status = "disabled"; > }; > > tlmm: pinctrl@3400000 { > compatible = "qcom,sdm845-pinctrl"; > reg = <0x03400000 0xc00000>; > interrupts = <GIC_SPI 208 IRQ_TYPE_NONE>; > gpio-controller; > #gpio-cells = <2>; > interrupt-controller; > #interrupt-cells = <2>; > > qup_uart2_default: qup_uart2_default { > pinmux { > function = "qup9"; > pins = "gpio4", "gpio5"; > }; > }; > > qup_uart2_sleep: qup_uart2_sleep { > pinmux { > function = "gpio"; > pins = "gpio4", "gpio5"; > }; > }; > }; > > In sdm845-mtp.dts: > > &qup_uart2_default { > pinconf { > pins = "gpio4", "gpio5"; > drive-strength = <2>; > bias-disable; > }; > }; > > &qup_uart2_sleep { > pinconf { > pins = "gpio4", "gpio5"; > drive-strength = <2>; > bias-disable; > }; > }; Correct. This example does however show another thing that I really do not like; When you have a lot of nodes I find it very useful to maintain some sort of grouping, to know that I can find a node describing properties related to some block close to related blocks - e.g. nodes describing a pmic block is close to other nodes for that pmic. Today we seem to have a mixture of bus-based grouping, arbitrary grouping and no grouping at all in our upstream dtsi files, so I think we should set some guidelines here as well. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 26, 2018 at 4:18 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: > On 01/25, Rajendra Nayak wrote: >> diff --git a/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi >> new file mode 100644 >> index 000000000000..b97f99e6f4b4 >> --- /dev/null >> +++ b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi >> @@ -0,0 +1,32 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2018, The Linux Foundation. All rights reserved. >> + */ >> + >> +&tlmm { > > I'm not the maintainer, but I find this approach to the pins > really annoying. I have to flip to another file to figure out how > a board has configured the pins. And we may bring in a bunch of > settings that we don't ever use on some board too. Why can't we > put the settings in the board file directly? FYI, there's a pending dtc change to allow deleting unreferenced nodes which can solve the bloat problem. Rob -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/07/2018 01:19 AM, Doug Anderson wrote: > Hi, > > On Tue, Feb 6, 2018 at 11:06 AM, Bjorn Andersson > <bjorn.andersson@linaro.org> wrote: >> On Tue 06 Feb 10:37 PST 2018, Doug Anderson wrote: >> >>> Hi, >>> >>> On Fri, Jan 26, 2018 at 2:18 PM, Stephen Boyd <sboyd@codeaurora.org> wrote: >>>> On 01/25, Rajendra Nayak wrote: >>>>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi >>>>> new file mode 100644 >>>>> index 000000000000..b97f99e6f4b4 >>>>> --- /dev/null >>>>> +++ b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi >>>>> @@ -0,0 +1,32 @@ >>>>> +// SPDX-License-Identifier: GPL-2.0 >>>>> +/* >>>>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved. >>>>> + */ >>>>> + >>>>> +&tlmm { >>>> >>>> I'm not the maintainer, but I find this approach to the pins >>>> really annoying. I have to flip to another file to figure out how >>>> a board has configured the pins. And we may bring in a bunch of >>>> settings that we don't ever use on some board too. Why can't we >>>> put the settings in the board file directly? >>> >>> I'm not so familiar with how things work with Qualcomm, but in general >>> I think putting this in the "board" file is a bad idea. I'd be OK >>> with putting this directly in the SoC file (though it might get >>> unwieldy?), but not moving things to the board file as was done with >>> v2 of this patch. >>> >>> Said another way: nearly board that uses SDM845 that uses UART2 will >>> have the same definitions for these pins so we shouldn't be >>> duplicating it across every board, right? >>> >> >> We've run into several cases where different boards uses the same >> function but requires board specific electrical configuration. >> >> So what we decided was to keep the pinmux in the soc-file (where e.g. >> the uart definition is) and then extend it with the board specific >> electrical properties (the pinconf), in the board files. >> >> This does come with the complexity of having the pinctrl nodes split in >> two places, but the responsibilities of the two parts is clear and we >> remove the need for all board files to ensure the appropriate pinmux is >> in place. >> >> >> NB. We did discuss adding "sane defaults" for the pinconf in the soc >> dtsi, but we end up spending considerable time debugging issues stemming >> from not having the right pinconf; so better make this explicit and say >> that the board has to specify it's config. > > Whoops, saw your responses _after_ I sent my response to v2. In any > case this makes sense to me then! On Rockchip boards I've been > involved in we often added "sane defaults", but I can see how that > could be confusing in different ways. I'm happy with your choice and > it seems like a happy medium. The sdm845.dtsi file can have the main > definition of the nodes and can thus refer to the nodes. Then you > just add the extra bit in the board file. > > What you propose is not what happened in v2 of the series > <https://patchwork.kernel.org/patch/10194201/> though. In v2 _both_ > the pinconf and the pinmux moved to the board file. That's wrong. got it. I'll fix this up in my v3. Thanks for the review.
diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi b/arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi index 5b1022c20bad..640a48cd628b 100644 --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi @@ -7,5 +7,8 @@ / { soc { + serial@a84000 { + status = "okay"; + }; }; }; diff --git a/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi new file mode 100644 index 000000000000..b97f99e6f4b4 --- /dev/null +++ b/arch/arm64/boot/dts/qcom/sdm845-pins.dtsi @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2018, The Linux Foundation. All rights reserved. + */ + +&tlmm { + qup_uart2_default: qup_uart2_default { + pinmux { + function = "qup9"; + pins = "gpio4", "gpio5"; + }; + + pinconf { + pins = "gpio4", "gpio5"; + drive-strength = <2>; + bias-disable; + }; + }; + + qup_uart2_sleep: qup_uart2_sleep { + pinmux { + function = "gpio"; + pins = "gpio4", "gpio5"; + }; + + pinconf { + pins = "gpio4", "gpio5"; + drive-strength = <2>; + bias-disable; + }; + }; +}; diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi index a21f4912b3e2..529f4ba3a1db 100644 --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi @@ -4,6 +4,7 @@ */ #include <dt-bindings/interrupt-controller/arm-gic.h> +#include <dt-bindings/clock/qcom,gcc-sdm845.h> / { model = "Qualcomm Technologies, Inc. SDM845"; @@ -304,5 +305,26 @@ cell-index = <0>; }; + qup_1: qcom,geni_se@ac0000 { + compatible = "qcom,geni-se-qup"; + reg = <0xac0000 0x6000>; + }; + + qup_uart2: serial@a84000 { + compatible = "qcom,geni-console", "qcom,geni-uart"; + reg = <0xa84000 0x4000>; + reg-names = "se_phys"; + clock-names = "se-clk", "m-ahb", "s-ahb"; + clocks = <&gcc GCC_QUPV3_WRAP1_S1_CLK>, + <&gcc GCC_QUPV3_WRAP_1_M_AHB_CLK>, + <&gcc GCC_QUPV3_WRAP_1_S_AHB_CLK>; + pinctrl-names = "default", "sleep"; + pinctrl-0 = <&qup_uart2_default>; + pinctrl-1 = <&qup_uart2_sleep>; + interrupts = <GIC_SPI 354 IRQ_TYPE_NONE>; + qcom,wrapper-core = <&qup_1>; + status = "disabled"; + }; }; }; +#include "sdm845-pins.dtsi"
Add the qup uart node and geni se instance needed to support the serial console on the MTP. Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org> --- This patch is based on the current proposed DT bindings for the geni based serial driver [1] and also depends on the GCC driver [2] which adds dt-bindings/clock/qcom,gcc-sdm845.h header. This can only be merged once the dependent patches do. [1] https://patchwork.ozlabs.org/cover/860251/ [2] https://lkml.org/lkml/2018/1/22/78 arch/arm64/boot/dts/qcom/sdm845-mtp.dtsi | 3 +++ arch/arm64/boot/dts/qcom/sdm845-pins.dtsi | 32 +++++++++++++++++++++++++++++++ arch/arm64/boot/dts/qcom/sdm845.dtsi | 22 +++++++++++++++++++++ 3 files changed, 57 insertions(+) create mode 100644 arch/arm64/boot/dts/qcom/sdm845-pins.dtsi