diff mbox series

[v1] arm64: dts: qcom: msm8998: Add i2c5 pins

Message ID ed5b1b55-285a-1c6d-c562-a965119000a5@free.fr (mailing list archive)
State New, archived
Headers show
Series [v1] arm64: dts: qcom: msm8998: Add i2c5 pins | expand

Commit Message

Marc Gonzalez April 25, 2019, 4:06 p.m. UTC
Downstream source:
https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8998-pinctrl.dtsi?h=LE.UM.1.3.r3.25#n165

Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
---
 arch/arm64/boot/dts/qcom/msm8998-pins.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Bjorn Andersson April 27, 2019, 4:51 a.m. UTC | #1
On Thu 25 Apr 09:06 PDT 2019, Marc Gonzalez wrote:

> Downstream source:
> https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8998-pinctrl.dtsi?h=LE.UM.1.3.r3.25#n165
> 
> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
> ---
>  arch/arm64/boot/dts/qcom/msm8998-pins.dtsi | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
> index 6db70acd38ee..d0a95c70d1e7 100644
> --- a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
> +++ b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
> @@ -2,6 +2,13 @@
>  /* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
>  
>  &tlmm {
> +	i2c5_default: i2c5_default {

You need to reference this node for it to make a difference. Also the
drive-strength and bias are board specific, so please move this to your
board dts (and reference the node).

Regards,
Bjorn

> +		pins = "gpio87", "gpio88";
> +		function = "blsp_i2c5";
> +		drive-strength = <2>;
> +		bias-disable;
> +	};
> +
>  	sdc2_clk_on: sdc2_clk_on {
>  		config {
>  			pins = "sdc2_clk";
> -- 
> 2.17.1
Marc Gonzalez April 29, 2019, 8:38 a.m. UTC | #2
On 27/04/2019 06:51, Bjorn Andersson wrote:

> On Thu 25 Apr 09:06 PDT 2019, Marc Gonzalez wrote:
> 
>> Downstream source:
>> https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8998-pinctrl.dtsi?h=LE.UM.1.3.r3.25#n165
>>
>> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
>> ---
>>  arch/arm64/boot/dts/qcom/msm8998-pins.dtsi | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
>> index 6db70acd38ee..d0a95c70d1e7 100644
>> --- a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
>> @@ -2,6 +2,13 @@
>>  /* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
>>  
>>  &tlmm {
>> +	i2c5_default: i2c5_default {
>> +		pins = "gpio87", "gpio88";
>> +		function = "blsp_i2c5";
>> +		drive-strength = <2>;
>> +		bias-disable;
>> +	};
> 
> You need to reference this node for it to make a difference.

Right. I do have a local board file referencing i2c5_default, which I plan
to submit at some point. It contains:

&blsp1_i2c5 {
	status = "ok";
	clock-frequency = <100000>;
	pinctrl-names = "default";
	pinctrl-0 = <&i2c5_default>;
};

> Also the drive-strength and bias are board specific, so please move this
> to your board dts (and reference the node).

Wait... Are you saying there should be no drive-strength nor bias definitions
inside msm8998-pins.dtsi?

$ grep -c 'strength\|bias' arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
18

Why are the SDHC pins different than the I2C pins?

i2c5 is "tied" to gpio87 and gpio88. Could my board designer "reassign"
these pins to a different HW block? Or is that immutable?

Regards.
Bjorn Andersson May 2, 2019, 3:12 p.m. UTC | #3
On Mon 29 Apr 01:38 PDT 2019, Marc Gonzalez wrote:

> On 27/04/2019 06:51, Bjorn Andersson wrote:
> 
> > On Thu 25 Apr 09:06 PDT 2019, Marc Gonzalez wrote:
> > 
> >> Downstream source:
> >> https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8998-pinctrl.dtsi?h=LE.UM.1.3.r3.25#n165
> >>
> >> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
> >> ---
> >>  arch/arm64/boot/dts/qcom/msm8998-pins.dtsi | 7 +++++++
> >>  1 file changed, 7 insertions(+)
> >>
> >> diff --git a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
> >> index 6db70acd38ee..d0a95c70d1e7 100644
> >> --- a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
> >> +++ b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
> >> @@ -2,6 +2,13 @@
> >>  /* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
> >>  
> >>  &tlmm {
> >> +	i2c5_default: i2c5_default {
> >> +		pins = "gpio87", "gpio88";
> >> +		function = "blsp_i2c5";
> >> +		drive-strength = <2>;
> >> +		bias-disable;
> >> +	};
> > 
> > You need to reference this node for it to make a difference.
> 
> Right. I do have a local board file referencing i2c5_default, which I plan
> to submit at some point. It contains:
> 
> &blsp1_i2c5 {
> 	status = "ok";
> 	clock-frequency = <100000>;
> 	pinctrl-names = "default";
> 	pinctrl-0 = <&i2c5_default>;
> };
> 
> > Also the drive-strength and bias are board specific, so please move this
> > to your board dts (and reference the node).
> 
> Wait... Are you saying there should be no drive-strength nor bias definitions
> inside msm8998-pins.dtsi?
> 
> $ grep -c 'strength\|bias' arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
> 18
> 
> Why are the SDHC pins different than the I2C pins?
> 
> i2c5 is "tied" to gpio87 and gpio88. Could my board designer "reassign"
> these pins to a different HW block? Or is that immutable?
> 

Right, so it makes a lot of sense to have a node in msm8998.dtsi that
says that if i2c5 is probed then the associated pinmux should be set up.

But the pinconf (drive-strenght, internal vs external bias) are board
specific, so this part better go in the board.dts.


On sdm845 we put a node with pinmux in the platform.dtsi and then in the
board we extend this node with the electrical properties of the board.
This works out pretty well, but we haven't gone back and updated the
older platforms/boards yet.

Regards,
Bjorn
Marc Gonzalez June 26, 2019, 4:20 p.m. UTC | #4
On 02/05/2019 17:12, Bjorn Andersson wrote:

> On Mon 29 Apr 01:38 PDT 2019, Marc Gonzalez wrote:
> 
>> On 27/04/2019 06:51, Bjorn Andersson wrote:
>>
>>> On Thu 25 Apr 09:06 PDT 2019, Marc Gonzalez wrote:
>>>
>>>> Downstream source:
>>>> https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8998-pinctrl.dtsi?h=LE.UM.1.3.r3.25#n165
>>>>
>>>> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr>
>>>> ---
>>>>  arch/arm64/boot/dts/qcom/msm8998-pins.dtsi | 7 +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
>>>> index 6db70acd38ee..d0a95c70d1e7 100644
>>>> --- a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
>>>> @@ -2,6 +2,13 @@
>>>>  /* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
>>>>  
>>>>  &tlmm {
>>>> +	i2c5_default: i2c5_default {
>>>> +		pins = "gpio87", "gpio88";
>>>> +		function = "blsp_i2c5";
>>>> +		drive-strength = <2>;
>>>> +		bias-disable;
>>>> +	};
>>>
>>> You need to reference this node for it to make a difference.
>>
>> Right. I do have a local board file referencing i2c5_default, which I plan
>> to submit at some point. It contains:
>>
>> &blsp1_i2c5 {
>> 	status = "ok";
>> 	clock-frequency = <100000>;
>> 	pinctrl-names = "default";
>> 	pinctrl-0 = <&i2c5_default>;
>> };
>>
>>> Also the drive-strength and bias are board specific, so please move this
>>> to your board dts (and reference the node).
>>
>> Wait... Are you saying there should be no drive-strength nor bias definitions
>> inside msm8998-pins.dtsi?
>>
>> $ grep -c 'strength\|bias' arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
>> 18
>>
>> Why are the SDHC pins different than the I2C pins?
>>
>> i2c5 is "tied" to gpio87 and gpio88. Could my board designer "reassign"
>> these pins to a different HW block? Or is that immutable?
>>
> 
> Right, so it makes a lot of sense to have a node in msm8998.dtsi that
> says that if i2c5 is probed then the associated pinmux should be set up.
> 
> But the pinconf (drive-strenght, internal vs external bias) are board
> specific, so this part better go in the board.dts.
> 
> 
> On sdm845 we put a node with pinmux in the platform.dtsi and then in the
> board we extend this node with the electrical properties of the board.
> This works out pretty well, but we haven't gone back and updated the
> older platforms/boards yet.

Wow, I had completely lost track of this thread...

OK, I think what you had in mind is the following:
(Please confirm before I spin a v2)

diff --git a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
index f09f3e03f708..9cd1f96dc3c8 100644
--- a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
@@ -27,6 +27,18 @@
 	status = "okay";
 };
 
+&blsp1_i2c5 {
+	status = "ok";
+	clock-frequency = <100000>; /*** NOT SURE... This depends on which devices are on the I2C bus? ***/
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2c5_default>;
+};
+
+&i2c5_default {
+	drive-strength = <2>;
+	bias-disable;
+};
+
 &qusb2phy {
 	status = "okay";
 
diff --git a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
index 6db70acd38ee..dad175a52d03 100644
--- a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
@@ -2,6 +2,11 @@
 /* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
 
 &tlmm {
+	i2c5_default: i2c5-default {
+		pins = "gpio87", "gpio88";
+		function = "blsp_i2c5";
+	};
+
 	sdc2_clk_on: sdc2_clk_on {
 		config {
 			pins = "sdc2_clk";




Well, except that there don't seem to be any devices on the i2c5 bus
on the mediabox...

# i2cdetect -r 0
i2cdetect: WARNING! This program can confuse your I2C bus
Continue? [y/N] y
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:          -- -- -- -- -- -- -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
70: -- -- -- -- -- -- -- --

But there are on several on my batfish board:

# i2cdetect -r 0
i2cdetect: WARNING! This program can confuse your I2C bus
Continue? [y/N] y
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:          -- -- -- -- -- -- -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
40: -- -- -- -- 44 -- -- 47 -- -- -- -- -- -- -- --
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
60: -- -- -- -- -- -- -- -- 68 -- -- -- -- -- -- --
70: -- -- -- -- -- -- -- --


Can I submit the arch/arm64/boot/dts/qcom/msm8998-pins.dtsi alone?

Regards.
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
index 6db70acd38ee..d0a95c70d1e7 100644
--- a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
@@ -2,6 +2,13 @@ 
 /* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
 
 &tlmm {
+	i2c5_default: i2c5_default {
+		pins = "gpio87", "gpio88";
+		function = "blsp_i2c5";
+		drive-strength = <2>;
+		bias-disable;
+	};
+
 	sdc2_clk_on: sdc2_clk_on {
 		config {
 			pins = "sdc2_clk";