diff mbox series

[RFC,3/4] arm64: dts: qcom: sm6350: add uart1 node

Message ID 20230421-fp4-bluetooth-v1-3-0430e3a7e0a2@fairphone.com (mailing list archive)
State Superseded
Headers show
Series Add WCN3988 Bluetooth support for Fairphone 4 | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch warning WARNING: line length of 113 exceeds 100 columns #147: FILE: arch/arm64/boot/dts/qcom/sm6350.dtsi:769: + pinctrl-0 = <&qup_uart1_cts>, <&qup_uart1_rts>, <&qup_uart1_tx>, <&qup_uart1_rx>; WARNING: line length of 109 exceeds 100 columns #151: FILE: arch/arm64/boot/dts/qcom/sm6350.dtsi:773: + interconnects = <&clk_virt MASTER_QUP_CORE_0 0 &clk_virt SLAVE_QUP_CORE_0 0>, WARNING: line length of 103 exceeds 100 columns #152: FILE: arch/arm64/boot/dts/qcom/sm6350.dtsi:774: + <&aggre1_noc MASTER_QUP_0 0 &clk_virt SLAVE_EBI_CH0 0>; total: 0 errors, 3 warnings, 81 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix-inplace. /github/workspace/src/src/13220105.patch has style problems, please review. NOTE: Ignored message types: UNKNOWN_COMMIT_ID NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS.
tedd_an/GitLint success Gitlint PASS
tedd_an/SubjectPrefix fail "Bluetooth: " prefix is not specified in the subject
tedd_an/IncrementalBuild success Incremental Build PASS

Commit Message

Luca Weiss April 21, 2023, 2:11 p.m. UTC
Add the node describing uart1 incl. opp table and pinctrl.

Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
---
 arch/arm64/boot/dts/qcom/sm6350.dtsi | 63 ++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

Comments

Steev Klimaszewski April 21, 2023, 4:59 p.m. UTC | #1
On Fri, Apr 21, 2023 at 9:12 AM Luca Weiss <luca.weiss@fairphone.com> wrote:
>
> Add the node describing uart1 incl. opp table and pinctrl.
>
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>  arch/arm64/boot/dts/qcom/sm6350.dtsi | 63 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> index 18c4616848ce..16c5e9a6c98a 100644
> --- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
> @@ -378,6 +378,25 @@ opp-2073600000 {
>                 };
>         };
>
> +       qup_opp_table: opp-table-qup {
> +               compatible = "operating-points-v2";
> +
> +               opp-75000000 {
> +                       opp-hz = /bits/ 64 <75000000>;
> +                       required-opps = <&rpmhpd_opp_low_svs>;
> +               };
> +
> +               opp-100000000 {
> +                       opp-hz = /bits/ 64 <100000000>;
> +                       required-opps = <&rpmhpd_opp_svs>;
> +               };
> +
> +               opp-128000000 {
> +                       opp-hz = /bits/ 64 <128000000>;
> +                       required-opps = <&rpmhpd_opp_nom>;
> +               };
> +       };
> +
>         pmu {
>                 compatible = "arm,armv8-pmuv3";
>                 interrupts = <GIC_PPI 5 IRQ_TYPE_LEVEL_LOW>;
> @@ -741,6 +760,22 @@ i2c0: i2c@880000 {
>                                 status = "disabled";
>                         };
>
> +                       uart1: serial@884000 {
> +                               compatible = "qcom,geni-uart";
> +                               reg = <0 0x00884000 0 0x4000>;
> +                               clock-names = "se";
> +                               clocks = <&gcc GCC_QUPV3_WRAP0_S1_CLK>;
> +                               pinctrl-names = "default";
> +                               pinctrl-0 = <&qup_uart1_cts>, <&qup_uart1_rts>, <&qup_uart1_tx>, <&qup_uart1_rx>;
> +                               interrupts = <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>;
> +                               power-domains = <&rpmhpd SM6350_CX>;
> +                               operating-points-v2 = <&qup_opp_table>;
> +                               interconnects = <&clk_virt MASTER_QUP_CORE_0 0 &clk_virt SLAVE_QUP_CORE_0 0>,
> +                                               <&aggre1_noc MASTER_QUP_0 0 &clk_virt SLAVE_EBI_CH0 0>;
> +                               interconnect-names = "qup-core", "qup-config";
> +                               status = "disabled";
> +                       };
> +
>                         i2c2: i2c@888000 {
>                                 compatible = "qcom,geni-i2c";
>                                 reg = <0 0x00888000 0 0x4000>;
> @@ -1726,6 +1761,34 @@ qup_i2c10_default: qup-i2c10-default-state {
>                                 drive-strength = <2>;
>                                 bias-pull-up;
>                         };
> +
> +                       qup_uart1_cts: qup-uart1-cts-default-state {
> +                               pins = "gpio61";
> +                               function = "qup01";
> +                               drive-strength = <2>;
> +                               bias-disable;
> +                       };
> +
> +                       qup_uart1_rts: qup-uart1-rts-default-state {
> +                               pins = "gpio62";
> +                               function = "qup01";
> +                               drive-strength = <2>;
> +                               bias-pull-down;
> +                       };
> +
> +                       qup_uart1_tx: qup-uart1-tx-default-state {
> +                               pins = "gpio63";
> +                               function = "qup01";
> +                               drive-strength = <2>;
> +                               bias-pull-up;
> +                       };
> +
tx should come after the rx, this caught me too when I was doing my
bluetooth driver, it goes by name, not gpio#.

> +                       qup_uart1_rx: qup-uart1-rx-default-state {
> +                               pins = "gpio64";
> +                               function = "qup01";
> +                               drive-strength = <2>;
> +                               bias-disable;
> +                       };
>                 };
>
>                 apps_smmu: iommu@15000000 {
>
> --
> 2.40.0
>
Krzysztof Kozlowski April 23, 2023, 10:51 a.m. UTC | #2
On 21/04/2023 16:11, Luca Weiss wrote:
> Add the node describing uart1 incl. opp table and pinctrl.
> 
> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> ---
>  arch/arm64/boot/dts/qcom/sm6350.dtsi | 63 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)

Please do not send DTS patches for net-next. DTS must go via Qualcomm
SoC. Split the series and mention where is the bindings change in DTS
patchset.


Best regards,
Krzysztof
Luca Weiss May 12, 2023, 2:30 p.m. UTC | #3
On Sun Apr 23, 2023 at 12:51 PM CEST, Krzysztof Kozlowski wrote:
> On 21/04/2023 16:11, Luca Weiss wrote:
> > Add the node describing uart1 incl. opp table and pinctrl.
> > 
> > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
> > ---
> >  arch/arm64/boot/dts/qcom/sm6350.dtsi | 63 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 63 insertions(+)
>
> Please do not send DTS patches for net-next. DTS must go via Qualcomm
> SoC. Split the series and mention where is the bindings change in DTS
> patchset.

Sorry, just saw now after already sending v2.

Is this a special rule for linux-bluetooth@ / netdev@? Isn't it easier
to keep it together so the status of series can be assessed easier? I've
always submitted patches by topic, like input patches + dts patches and
it was never mentioned.

Regards
Luca

>
>
> Best regards,
> Krzysztof
Krzysztof Kozlowski May 12, 2023, 3:04 p.m. UTC | #4
On 12/05/2023 16:30, Luca Weiss wrote:
> On Sun Apr 23, 2023 at 12:51 PM CEST, Krzysztof Kozlowski wrote:
>> On 21/04/2023 16:11, Luca Weiss wrote:
>>> Add the node describing uart1 incl. opp table and pinctrl.
>>>
>>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com>
>>> ---
>>>  arch/arm64/boot/dts/qcom/sm6350.dtsi | 63 ++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 63 insertions(+)
>>
>> Please do not send DTS patches for net-next. DTS must go via Qualcomm
>> SoC. Split the series and mention where is the bindings change in DTS
>> patchset.
> 
> Sorry, just saw now after already sending v2.
> 
> Is this a special rule for linux-bluetooth@ / netdev@? Isn't it easier
> to keep it together so the status of series can be assessed easier? I've
> always submitted patches by topic, like input patches + dts patches and
> it was never mentioned.

The rule that DTS must go via Qualcomm SoC (arm-soc) was there always,
but other maintainers often do not pay attention to this. I don't blame
them, don't get me wrong. I am just stating the observed actions.
Usually netdev folks and Greg will take everything you throw at them, so
for these subsystems it is recommended to split DTS to different patchset.

For other maintainers it is usually also more useful to split, because
then they can apply entire patchset with one command, instead of picking
up specific patches (omitting DTS).

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sm6350.dtsi b/arch/arm64/boot/dts/qcom/sm6350.dtsi
index 18c4616848ce..16c5e9a6c98a 100644
--- a/arch/arm64/boot/dts/qcom/sm6350.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm6350.dtsi
@@ -378,6 +378,25 @@  opp-2073600000 {
 		};
 	};
 
+	qup_opp_table: opp-table-qup {
+		compatible = "operating-points-v2";
+
+		opp-75000000 {
+			opp-hz = /bits/ 64 <75000000>;
+			required-opps = <&rpmhpd_opp_low_svs>;
+		};
+
+		opp-100000000 {
+			opp-hz = /bits/ 64 <100000000>;
+			required-opps = <&rpmhpd_opp_svs>;
+		};
+
+		opp-128000000 {
+			opp-hz = /bits/ 64 <128000000>;
+			required-opps = <&rpmhpd_opp_nom>;
+		};
+	};
+
 	pmu {
 		compatible = "arm,armv8-pmuv3";
 		interrupts = <GIC_PPI 5 IRQ_TYPE_LEVEL_LOW>;
@@ -741,6 +760,22 @@  i2c0: i2c@880000 {
 				status = "disabled";
 			};
 
+			uart1: serial@884000 {
+				compatible = "qcom,geni-uart";
+				reg = <0 0x00884000 0 0x4000>;
+				clock-names = "se";
+				clocks = <&gcc GCC_QUPV3_WRAP0_S1_CLK>;
+				pinctrl-names = "default";
+				pinctrl-0 = <&qup_uart1_cts>, <&qup_uart1_rts>, <&qup_uart1_tx>, <&qup_uart1_rx>;
+				interrupts = <GIC_SPI 602 IRQ_TYPE_LEVEL_HIGH>;
+				power-domains = <&rpmhpd SM6350_CX>;
+				operating-points-v2 = <&qup_opp_table>;
+				interconnects = <&clk_virt MASTER_QUP_CORE_0 0 &clk_virt SLAVE_QUP_CORE_0 0>,
+						<&aggre1_noc MASTER_QUP_0 0 &clk_virt SLAVE_EBI_CH0 0>;
+				interconnect-names = "qup-core", "qup-config";
+				status = "disabled";
+			};
+
 			i2c2: i2c@888000 {
 				compatible = "qcom,geni-i2c";
 				reg = <0 0x00888000 0 0x4000>;
@@ -1726,6 +1761,34 @@  qup_i2c10_default: qup-i2c10-default-state {
 				drive-strength = <2>;
 				bias-pull-up;
 			};
+
+			qup_uart1_cts: qup-uart1-cts-default-state {
+				pins = "gpio61";
+				function = "qup01";
+				drive-strength = <2>;
+				bias-disable;
+			};
+
+			qup_uart1_rts: qup-uart1-rts-default-state {
+				pins = "gpio62";
+				function = "qup01";
+				drive-strength = <2>;
+				bias-pull-down;
+			};
+
+			qup_uart1_tx: qup-uart1-tx-default-state {
+				pins = "gpio63";
+				function = "qup01";
+				drive-strength = <2>;
+				bias-pull-up;
+			};
+
+			qup_uart1_rx: qup-uart1-rx-default-state {
+				pins = "gpio64";
+				function = "qup01";
+				drive-strength = <2>;
+				bias-disable;
+			};
 		};
 
 		apps_smmu: iommu@15000000 {