diff mbox series

[v2] arm64: dts: qcom: sc7280: Add bluetooth node on SC7280

Message ID 1634043698-20256-1-git-send-email-bgodavar@codeaurora.org (mailing list archive)
State Handled Elsewhere
Headers show
Series [v2] arm64: dts: qcom: sc7280: Add bluetooth node on SC7280 | expand

Commit Message

Balakrishna Godavarthi Oct. 12, 2021, 1:01 p.m. UTC
Add bluetooth SoC WCN6750 node for SC7280 IDP boards.

Signed-off-by: Balakrishna Godavarthi <bgodavar@codeaurora.org>
---
v2: 
  * merged two patches into one
  * Removed unused comments
  * Removed pinmux & pin conf.
  * Addressed reviewers comments 

v1: initial patch
---
 arch/arm64/boot/dts/qcom/sc7280-idp.dts  |  6 ++++++
 arch/arm64/boot/dts/qcom/sc7280-idp.dtsi | 25 +++++++++++++++++++++++++
 arch/arm64/boot/dts/qcom/sc7280-idp2.dts |  6 ++++++
 3 files changed, 37 insertions(+)

Comments

Stephen Boyd Oct. 12, 2021, 5:24 p.m. UTC | #1
Quoting Balakrishna Godavarthi (2021-10-12 06:01:38)
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> index 272d5ca..09adc802 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> @@ -393,6 +393,23 @@
>                                 <&tlmm 31 IRQ_TYPE_EDGE_FALLING>;
>         pinctrl-names = "default", "sleep";
>         pinctrl-1 = <&qup_uart7_sleep_cts>, <&qup_uart7_sleep_rts>, <&qup_uart7_sleep_tx>, <&qup_uart7_sleep_rx>;
> +
> +       bluetooth: wcn6750-bt {

bluetooth: bluetooth {

Node names should be generic.

> +               compatible = "qcom,wcn6750-bt";
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&bt_en_default>;
> +               enable-gpios = <&tlmm 85 GPIO_ACTIVE_HIGH>;
> +               swctrl-gpios = <&tlmm 86 GPIO_ACTIVE_HIGH>;

Is there any pinctrl config for gpio 86?

> +               vddaon-supply = <&vreg_s7b_0p9>;
> +               vddbtcxmx-supply = <&vreg_s7b_0p9>;
> +               vddrfacmn-supply = <&vreg_s7b_0p9>;
> +               vddrfa0p8-supply = <&vreg_s7b_0p9>;
> +               vddrfa1p7-supply = <&vreg_s1b_1p8>;
> +               vddrfa1p2-supply = <&vreg_s8b_1p2>;
> +               vddrfa2p2-supply = <&vreg_s1c_2p2>;
> +               vddasd-supply = <&vreg_l11c_2p8>;
> +               max-speed = <3200000>;
> +       };
>  };
>
>  /* PINCTRL - additions to nodes defined in sc7280.dtsi */
> @@ -504,6 +521,14 @@
>                  */
>                 bias-pull-up;
>         };
> +
> +       bt_en_default: bt_en_default {

bt_en: bt-en {

Node names shouldn't have underscores and 'default' is redundant.

> +               pins = "gpio85";
> +               function = "gpio";
> +               drive-strength = <2>;
> +               output-low;
> +               bias-pull-down;

Why is there a pull down on an output gpio? Shouldn't this be
bias-disable?

> +       };
>  };
>
>  &sdc1_on {
Balakrishna Godavarthi Oct. 13, 2021, 5:30 a.m. UTC | #2
Hi Stephen Boyd,

On 2021-10-12 22:54, Stephen Boyd wrote:
> Quoting Balakrishna Godavarthi (2021-10-12 06:01:38)
>> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi 
>> b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> index 272d5ca..09adc802 100644
>> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
>> @@ -393,6 +393,23 @@
>>                                 <&tlmm 31 IRQ_TYPE_EDGE_FALLING>;
>>         pinctrl-names = "default", "sleep";
>>         pinctrl-1 = <&qup_uart7_sleep_cts>, <&qup_uart7_sleep_rts>, 
>> <&qup_uart7_sleep_tx>, <&qup_uart7_sleep_rx>;
>> +
>> +       bluetooth: wcn6750-bt {
> 
> bluetooth: bluetooth {
> 
> Node names should be generic.
> 

[Bala]: will update in next patch.

>> +               compatible = "qcom,wcn6750-bt";
>> +               pinctrl-names = "default";
>> +               pinctrl-0 = <&bt_en_default>;
>> +               enable-gpios = <&tlmm 85 GPIO_ACTIVE_HIGH>;
>> +               swctrl-gpios = <&tlmm 86 GPIO_ACTIVE_HIGH>;
> 
> Is there any pinctrl config for gpio 86?
> 
[Bala]: This is input GPIO to apps, BT SOC will handle configurations.

>> +               vddaon-supply = <&vreg_s7b_0p9>;
>> +               vddbtcxmx-supply = <&vreg_s7b_0p9>;
>> +               vddrfacmn-supply = <&vreg_s7b_0p9>;
>> +               vddrfa0p8-supply = <&vreg_s7b_0p9>;
>> +               vddrfa1p7-supply = <&vreg_s1b_1p8>;
>> +               vddrfa1p2-supply = <&vreg_s8b_1p2>;
>> +               vddrfa2p2-supply = <&vreg_s1c_2p2>;
>> +               vddasd-supply = <&vreg_l11c_2p8>;
>> +               max-speed = <3200000>;
>> +       };
>>  };
>> 
>>  /* PINCTRL - additions to nodes defined in sc7280.dtsi */
>> @@ -504,6 +521,14 @@
>>                  */
>>                 bias-pull-up;
>>         };
>> +
>> +       bt_en_default: bt_en_default {
> 
> bt_en: bt-en {
> 
> Node names shouldn't have underscores and 'default' is redundant.
> 
[Bala]: will update in next patch.

>> +               pins = "gpio85";
>> +               function = "gpio";
>> +               drive-strength = <2>;
>> +               output-low;
>> +               bias-pull-down;
> 
> Why is there a pull down on an output gpio? Shouldn't this be
> bias-disable?
> 

[Bala]: BT_EN pin is OP of apps and input to BT SoC.
by default we want the state of BT_EN to be low. so used pull down 
instead of bias-disable
as AFAIK bias-disable may trigger a tristate on BT_EN pin, which may 
trigger BT SoC enable
if it is not actually triggered.

>> +       };
>>  };
>> 
>>  &sdc1_on {
Stephen Boyd Oct. 13, 2021, 8:14 p.m. UTC | #3
Quoting bgodavar@codeaurora.org (2021-10-12 22:30:50)
> On 2021-10-12 22:54, Stephen Boyd wrote:
> > Quoting Balakrishna Godavarthi (2021-10-12 06:01:38)
> >> diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> >> b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> >> index 272d5ca..09adc802 100644
> >> --- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> >> +++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
> >> +               compatible = "qcom,wcn6750-bt";
> >> +               pinctrl-names = "default";
> >> +               pinctrl-0 = <&bt_en_default>;
> >> +               enable-gpios = <&tlmm 85 GPIO_ACTIVE_HIGH>;
> >> +               swctrl-gpios = <&tlmm 86 GPIO_ACTIVE_HIGH>;
> >
> > Is there any pinctrl config for gpio 86?
> >
> [Bala]: This is input GPIO to apps, BT SOC will handle configurations.

Ok. So there should be some pinctrl stating that the function is "gpio"
and the biasing is probably bias-disable. The BT SOC will handle setting
any pull, either up or down?

>
> >> +               vddaon-supply = <&vreg_s7b_0p9>;
> >> +               vddbtcxmx-supply = <&vreg_s7b_0p9>;
> >> +               vddrfacmn-supply = <&vreg_s7b_0p9>;
> >> +               vddrfa0p8-supply = <&vreg_s7b_0p9>;
> >> +               vddrfa1p7-supply = <&vreg_s1b_1p8>;
> >> +               vddrfa1p2-supply = <&vreg_s8b_1p2>;
> >> +               vddrfa2p2-supply = <&vreg_s1c_2p2>;
> >> +               vddasd-supply = <&vreg_l11c_2p8>;
> >> +               max-speed = <3200000>;
> >> +       };
> >>  };
> >>
> >>  /* PINCTRL - additions to nodes defined in sc7280.dtsi */
> >> @@ -504,6 +521,14 @@
> >>                  */
> >>                 bias-pull-up;
> >>         };
> >> +
> >> +       bt_en_default: bt_en_default {
> >
> > bt_en: bt-en {
> >
> > Node names shouldn't have underscores and 'default' is redundant.
> >
> [Bala]: will update in next patch.
>
> >> +               pins = "gpio85";
> >> +               function = "gpio";
> >> +               drive-strength = <2>;
> >> +               output-low;
> >> +               bias-pull-down;
> >
> > Why is there a pull down on an output gpio? Shouldn't this be
> > bias-disable?
> >
>
> [Bala]: BT_EN pin is OP of apps and input to BT SoC.
> by default we want the state of BT_EN to be low. so used pull down
> instead of bias-disable

The pin state will be low because of the 'output-low' property.

> as AFAIK bias-disable may trigger a tristate on BT_EN pin, which may
> trigger BT SoC enable
> if it is not actually triggered.

Is the pin ever "turned around" and made an input? If not then it will
be output and be driving low until the enable-gpios is set to output
active. The pull down is probably wasting power when the pin is being
driven either high or low.
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dts b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
index 64fc22a..d8b9262 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dts
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dts
@@ -16,7 +16,9 @@ 
 	compatible = "qcom,sc7280-idp", "google,senor", "qcom,sc7280";
 
 	aliases {
+		bluetooth0 = &bluetooth;
 		serial0 = &uart5;
+		serial1 = &uart7;
 	};
 
 	chosen {
@@ -68,3 +70,7 @@ 
 		qcom,pre-scaling = <1 1>;
 	};
 };
+
+&bluetooth {
+	vddio-supply = <&vreg_l19b_1p8>;
+};
diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
index 272d5ca..09adc802 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp.dtsi
@@ -393,6 +393,23 @@ 
 				<&tlmm 31 IRQ_TYPE_EDGE_FALLING>;
 	pinctrl-names = "default", "sleep";
 	pinctrl-1 = <&qup_uart7_sleep_cts>, <&qup_uart7_sleep_rts>, <&qup_uart7_sleep_tx>, <&qup_uart7_sleep_rx>;
+
+	bluetooth: wcn6750-bt {
+		compatible = "qcom,wcn6750-bt";
+		pinctrl-names = "default";
+		pinctrl-0 = <&bt_en_default>;
+		enable-gpios = <&tlmm 85 GPIO_ACTIVE_HIGH>;
+		swctrl-gpios = <&tlmm 86 GPIO_ACTIVE_HIGH>;
+		vddaon-supply = <&vreg_s7b_0p9>;
+		vddbtcxmx-supply = <&vreg_s7b_0p9>;
+		vddrfacmn-supply = <&vreg_s7b_0p9>;
+		vddrfa0p8-supply = <&vreg_s7b_0p9>;
+		vddrfa1p7-supply = <&vreg_s1b_1p8>;
+		vddrfa1p2-supply = <&vreg_s8b_1p2>;
+		vddrfa2p2-supply = <&vreg_s1c_2p2>;
+		vddasd-supply = <&vreg_l11c_2p8>;
+		max-speed = <3200000>;
+	};
 };
 
 /* PINCTRL - additions to nodes defined in sc7280.dtsi */
@@ -504,6 +521,14 @@ 
 		 */
 		bias-pull-up;
 	};
+
+	bt_en_default: bt_en_default {
+		pins = "gpio85";
+		function = "gpio";
+		drive-strength = <2>;
+		output-low;
+		bias-pull-down;
+	};
 };
 
 &sdc1_on {
diff --git a/arch/arm64/boot/dts/qcom/sc7280-idp2.dts b/arch/arm64/boot/dts/qcom/sc7280-idp2.dts
index 1fc2add..e60fa88 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-idp2.dts
+++ b/arch/arm64/boot/dts/qcom/sc7280-idp2.dts
@@ -14,10 +14,16 @@ 
 	compatible = "qcom,sc7280-idp2", "google,piglin", "qcom,sc7280";
 
 	aliases {
+		bluetooth0 = &bluetooth;
 		serial0 = &uart5;
+		serial1 = &uart7;
 	};
 
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
 };
+
+&bluetooth {
+	vddio-supply = <&vreg_l18b_1p8>;
+};