diff mbox series

[3/4] arm64: dts: qcom: sa8540p-ride: add qup1_i2c15 and qup2_i2c18 nodes

Message ID 20221212182314.1902632-4-bmasney@redhat.com (mailing list archive)
State Superseded
Headers show
Series arm64: dts: qcom: sc8280xp: add i2c and spi nodes | expand

Commit Message

Brian Masney Dec. 12, 2022, 6:23 p.m. UTC
Add the necessary nodes in order to get qup1_i2c15 and qup2_i2c18
functioning on the automotive board and exposed to userspace.

This work was derived from various patches that Qualcomm delivered
to Red Hat in a downstream kernel. This change was validated by using
i2c-tools 4.3.3 on CentOS Stream 9:

[root@localhost ~]# i2cdetect -l
i2c-15  i2c             Geni-I2C                                I2C adapter
i2c-18  i2c             Geni-I2C                                I2C adapter

[root@localhost ~]# i2cdetect -a -y 15
Warning: Can't use SMBus Quick Write command, will skip some addresses
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:
10:
20:
30: -- -- -- -- -- -- -- --
40:
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
60:
70:

Bus 18 has the same output. I validated that we get the same output on
the downstream kernel.

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 46 +++++++++++++++++++++++
 1 file changed, 46 insertions(+)

Comments

Shazad Hussain Dec. 13, 2022, 7:18 a.m. UTC | #1
On 12/12/2022 11:53 PM, Brian Masney wrote:
> Add the necessary nodes in order to get qup1_i2c15 and qup2_i2c18
> functioning on the automotive board and exposed to userspace.
> 
> This work was derived from various patches that Qualcomm delivered
> to Red Hat in a downstream kernel. This change was validated by using
> i2c-tools 4.3.3 on CentOS Stream 9:
> 
> [root@localhost ~]# i2cdetect -l
> i2c-15  i2c             Geni-I2C                                I2C adapter
> i2c-18  i2c             Geni-I2C                                I2C adapter
> 
> [root@localhost ~]# i2cdetect -a -y 15
> Warning: Can't use SMBus Quick Write command, will skip some addresses
>       0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> 00:
> 10:
> 20:
> 30: -- -- -- -- -- -- -- --
> 40:
> 50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 60:
> 70:
> 
> Bus 18 has the same output. I validated that we get the same output on
> the downstream kernel.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>   arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 46 +++++++++++++++++++++++
>   1 file changed, 46 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> index d70859803fbd..6dc3f3ff8ece 100644
> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> @@ -17,6 +17,8 @@ / {
>   	compatible = "qcom,sa8540p-ride", "qcom,sa8540p";
>   
>   	aliases {
> +		i2c15 = &qup1_i2c15;
> +		i2c18 = &qup2_i2c18;

I was listing out all the i2c devices to be enabled apart from above and 
below are applicable for Qdrive3 as well:
i2c0  980000
i2c1  984000
i2c12 0x00A90000

-Shazad

>   		serial0 = &qup2_uart17;
>   	};
>   
> @@ -188,10 +190,28 @@ &pcie3a_phy {
>   	status = "okay";
>   };
>   
> +&qup1 {
> +	status = "okay";
> +};
> +
> +&qup1_i2c15 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&qup1_i2c15_default>;
> +
> +	status = "okay";
> +};
> +
>   &qup2 {
>   	status = "okay";
>   };
>   
> +&qup2_i2c18 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&qup2_i2c18_default>;
> +
> +	status = "okay";
> +};
> +
>   &qup2_uart17 {
>   	compatible = "qcom,geni-debug-uart";
>   	status = "okay";
> @@ -313,4 +333,30 @@ wake-pins {
>   			bias-pull-up;
>   		};
>   	};
> +
> +	qup1_i2c15_default: qup1-i2c15-state {
> +		mux-pins {
> +			pins = "gpio36", "gpio37";
> +			function = "qup15";
> +		};
> +
> +		config-pins {
> +			pins = "gpio36", "gpio37";
> +			drive-strength = <0x02>;
> +			bias-pull-up;
> +		};
> +	};
> +
> +	qup2_i2c18_default: qup2-i2c18-state {
> +		mux-pins {
> +			pins = "gpio66", "gpio67";
> +			function = "qup18";
> +		};
> +
> +		config-pins {
> +			pins = "gpio66", "gpio67";
> +			drive-strength = <0x02>;
> +			bias-pull-up;
> +		};
> +	};
>   };
Konrad Dybcio Dec. 13, 2022, 2:48 p.m. UTC | #2
On 12.12.2022 19:23, Brian Masney wrote:
> Add the necessary nodes in order to get qup1_i2c15 and qup2_i2c18
> functioning on the automotive board and exposed to userspace.
> 
> This work was derived from various patches that Qualcomm delivered
> to Red Hat in a downstream kernel. This change was validated by using
> i2c-tools 4.3.3 on CentOS Stream 9:
> 
> [root@localhost ~]# i2cdetect -l
> i2c-15  i2c             Geni-I2C                                I2C adapter
> i2c-18  i2c             Geni-I2C                                I2C adapter
> 
> [root@localhost ~]# i2cdetect -a -y 15
> Warning: Can't use SMBus Quick Write command, will skip some addresses
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> 00:
> 10:
> 20:
> 30: -- -- -- -- -- -- -- --
> 40:
> 50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 60:
> 70:
> 
> Bus 18 has the same output. I validated that we get the same output on
> the downstream kernel.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 46 +++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> index d70859803fbd..6dc3f3ff8ece 100644
> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> @@ -17,6 +17,8 @@ / {
>  	compatible = "qcom,sa8540p-ride", "qcom,sa8540p";
>  
>  	aliases {
> +		i2c15 = &qup1_i2c15;
> +		i2c18 = &qup2_i2c18;
>  		serial0 = &qup2_uart17;
>  	};
>  
> @@ -188,10 +190,28 @@ &pcie3a_phy {
>  	status = "okay";
>  };
>  
> +&qup1 {
> +	status = "okay";
> +};
> +
> +&qup1_i2c15 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&qup1_i2c15_default>;
> +
> +	status = "okay";
> +};
> +
>  &qup2 {
>  	status = "okay";
>  };
>  
> +&qup2_i2c18 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&qup2_i2c18_default>;
> +
> +	status = "okay";
> +};
> +
>  &qup2_uart17 {
>  	compatible = "qcom,geni-debug-uart";
>  	status = "okay";
> @@ -313,4 +333,30 @@ wake-pins {
>  			bias-pull-up;
>  		};
>  	};
> +
> +	qup1_i2c15_default: qup1-i2c15-state {
You can drop mux/config-pins and have the pin properties live directly
under the qup1-i2cN-state node.

Konrad
> +		mux-pins {
> +			pins = "gpio36", "gpio37";
> +			function = "qup15";
> +		};
> +
> +		config-pins {
> +			pins = "gpio36", "gpio37";
> +			drive-strength = <0x02>;
> +			bias-pull-up;
> +		};
> +	};
> +
> +	qup2_i2c18_default: qup2-i2c18-state {
> +		mux-pins {
> +			pins = "gpio66", "gpio67";
> +			function = "qup18";
> +		};
> +
> +		config-pins {
> +			pins = "gpio66", "gpio67";
> +			drive-strength = <0x02>;
> +			bias-pull-up;
> +		};
> +	};
>  };
Johan Hovold Dec. 13, 2022, 2:59 p.m. UTC | #3
On Mon, Dec 12, 2022 at 01:23:13PM -0500, Brian Masney wrote:
> Add the necessary nodes in order to get qup1_i2c15 and qup2_i2c18
> functioning on the automotive board and exposed to userspace.
> 
> This work was derived from various patches that Qualcomm delivered
> to Red Hat in a downstream kernel. This change was validated by using
> i2c-tools 4.3.3 on CentOS Stream 9:
> 
> [root@localhost ~]# i2cdetect -l
> i2c-15  i2c             Geni-I2C                                I2C adapter
> i2c-18  i2c             Geni-I2C                                I2C adapter
> 
> [root@localhost ~]# i2cdetect -a -y 15
> Warning: Can't use SMBus Quick Write command, will skip some addresses
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> 00:
> 10:
> 20:
> 30: -- -- -- -- -- -- -- --
> 40:
> 50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 60:
> 70:
> 
> Bus 18 has the same output. I validated that we get the same output on
> the downstream kernel.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---

> +	qup1_i2c15_default: qup1-i2c15-state {
> +		mux-pins {
> +			pins = "gpio36", "gpio37";
> +			function = "qup15";
> +		};
> +
> +		config-pins {
> +			pins = "gpio36", "gpio37";
> +			drive-strength = <0x02>;

Use decimal notation.

> +			bias-pull-up;
> +		};
> +	};

Johan
Brian Masney Dec. 14, 2022, 12:30 p.m. UTC | #4
On Tue, Dec 13, 2022 at 03:48:27PM +0100, Konrad Dybcio wrote:
> > +	qup1_i2c15_default: qup1-i2c15-state {
> > +		mux-pins {
> > +			pins = "gpio36", "gpio37";
> > +			function = "qup15";
> > +		};
> > +
> > +		config-pins {
> > +			pins = "gpio36", "gpio37";
> > +			drive-strength = <0x02>;
> > +			bias-pull-up;
> > +		};
> > +	};
>
> You can drop mux/config-pins and have the pin properties live directly
> under the qup1-i2cN-state node.

Hi Konrad (and Shazad below), 

I need to enable 5 i2c buses (0, 1, 12, 15, 18) on this board. I tried
the following combinations with the pin mapping configuration and the
only one that seems to work reliably for me is what I originally had.

With the following, only 2 out of the 5 buses are detected. There's no
i2c mesages in dmesg.

    i2c0_default: i2c0-default-state {
        pins = "gpio135", "gpio136";
        function = "qup15";
    };

Next, I added a drive-strength and bias-pull-up. All 5 buses are
detected. One bus throws read errors when I probe it with i2cdetect, two
others 'i2cdetect -a -y $BUSNUM' takes ~5 seconds to run, and the
remaining two are fast.

    i2c0_default: i2c0-default-state {
        pins = "gpio135", "gpio136";
        function = "qup15";
        drive-strength = <2>;
        bias-pull-up;
    };

This is the style where i2cdetect seems to be happy for all 5 buses and
is fast:

    i2c0_default: i2c0-default-state {
        mux-pins {
            pins = "gpio135", "gpio136";
            function = "qup0";
        };

        config-pins {
            pins = "gpio135", "gpio136";
            drive-strength = <2>;
            bias-pull-up;
        };
    };


Shazad: 'i2cdetect -a -y $BUSNUM) shows that all 5 buses have the same
addresses listening. Is that expected? That seems a bit odd to me.

[root@localhost ~]# i2cdetect -a -y 0
Warning: Can't use SMBus Quick Write command, will skip some addresses
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:                                                 
10:                                                 
20:                                                 
30: -- -- -- -- -- -- -- --                         
40:                                                 
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
60:                                                 
70:                                                 

I triple checked that I have the QUP pins defined correctly for the 5
buses. I checked them against what's in the downstream kernel and I also
checked them against what's in upstream's
drivers/pinctrl/qcom/pinctrl-sc8280xp.c. This is the pin mapping that I
have:

    i2c0: gpio135, gpio136
    i2c1: gpio158, gpio159
    i2c12: gpio0, gpio1
    i2c15: gpio36, gpio37
    i2c18: gpio66, gpio67

Brian
Krzysztof Kozlowski Dec. 14, 2022, 12:51 p.m. UTC | #5
On 12/12/2022 19:23, Brian Masney wrote:
> Add the necessary nodes in order to get qup1_i2c15 and qup2_i2c18
> functioning on the automotive board and exposed to userspace.
> 
> This work was derived from various patches that Qualcomm delivered
> to Red Hat in a downstream kernel. This change was validated by using
> i2c-tools 4.3.3 on CentOS Stream 9:
> 
> [root@localhost ~]# i2cdetect -l
> i2c-15  i2c             Geni-I2C                                I2C adapter
> i2c-18  i2c             Geni-I2C                                I2C adapter
> 
> [root@localhost ~]# i2cdetect -a -y 15
> Warning: Can't use SMBus Quick Write command, will skip some addresses
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> 00:
> 10:
> 20:
> 30: -- -- -- -- -- -- -- --
> 40:
> 50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 60:
> 70:
> 
> Bus 18 has the same output. I validated that we get the same output on
> the downstream kernel.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> ---
>  arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 46 +++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> index d70859803fbd..6dc3f3ff8ece 100644
> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> @@ -17,6 +17,8 @@ / {
>  	compatible = "qcom,sa8540p-ride", "qcom,sa8540p";
>  
>  	aliases {
> +		i2c15 = &qup1_i2c15;
> +		i2c18 = &qup2_i2c18;
>  		serial0 = &qup2_uart17;
>  	};
>  
> @@ -188,10 +190,28 @@ &pcie3a_phy {
>  	status = "okay";
>  };
>  
> +&qup1 {
> +	status = "okay";
> +};
> +
> +&qup1_i2c15 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&qup1_i2c15_default>;
> +
> +	status = "okay";
> +};
> +
>  &qup2 {
>  	status = "okay";
>  };
>  
> +&qup2_i2c18 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&qup2_i2c18_default>;
> +
> +	status = "okay";
> +};
> +
>  &qup2_uart17 {
>  	compatible = "qcom,geni-debug-uart";
>  	status = "okay";
> @@ -313,4 +333,30 @@ wake-pins {
>  			bias-pull-up;
>  		};
>  	};
> +
> +	qup1_i2c15_default: qup1-i2c15-state {
> +		mux-pins {
> +			pins = "gpio36", "gpio37";
> +			function = "qup15";
> +		};
> +
> +		config-pins {
> +			pins = "gpio36", "gpio37";
> +			drive-strength = <0x02>;

Except the problem pointed out by Konrad (we do not have separate mux
and config pins anymore), this is not a hex, it's mA.




Best regards,
Krzysztof
Krzysztof Kozlowski Dec. 14, 2022, 12:52 p.m. UTC | #6
On 14/12/2022 13:30, Brian Masney wrote:
> On Tue, Dec 13, 2022 at 03:48:27PM +0100, Konrad Dybcio wrote:
>>> +	qup1_i2c15_default: qup1-i2c15-state {
>>> +		mux-pins {
>>> +			pins = "gpio36", "gpio37";
>>> +			function = "qup15";
>>> +		};
>>> +
>>> +		config-pins {
>>> +			pins = "gpio36", "gpio37";
>>> +			drive-strength = <0x02>;
>>> +			bias-pull-up;
>>> +		};
>>> +	};
>>
>> You can drop mux/config-pins and have the pin properties live directly
>> under the qup1-i2cN-state node.
> 
> Hi Konrad (and Shazad below), 
> 
> I need to enable 5 i2c buses (0, 1, 12, 15, 18) on this board. I tried
> the following combinations with the pin mapping configuration and the
> only one that seems to work reliably for me is what I originally had.
> 
> With the following, only 2 out of the 5 buses are detected. There's no
> i2c mesages in dmesg.
> 
>     i2c0_default: i2c0-default-state {
>         pins = "gpio135", "gpio136";
>         function = "qup15";
>     };
> 
> Next, I added a drive-strength and bias-pull-up. All 5 buses are
> detected. One bus throws read errors when I probe it with i2cdetect, two
> others 'i2cdetect -a -y $BUSNUM' takes ~5 seconds to run, and the
> remaining two are fast.
> 
>     i2c0_default: i2c0-default-state {
>         pins = "gpio135", "gpio136";
>         function = "qup15";
>         drive-strength = <2>;
>         bias-pull-up;
>     };
> 
> This is the style where i2cdetect seems to be happy for all 5 buses and
> is fast:
> 
>     i2c0_default: i2c0-default-state {
>         mux-pins {
>             pins = "gpio135", "gpio136";
>             function = "qup0";
>         };
> 
>         config-pins {
>             pins = "gpio135", "gpio136";
>             drive-strength = <2>;
>             bias-pull-up;
>         };
>     };
> 
> 
> Shazad: 'i2cdetect -a -y $BUSNUM) shows that all 5 buses have the same
> addresses listening. Is that expected? That seems a bit odd to me.
> 
> [root@localhost ~]# i2cdetect -a -y 0
> Warning: Can't use SMBus Quick Write command, will skip some addresses
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> 00:                                                 
> 10:                                                 
> 20:                                                 
> 30: -- -- -- -- -- -- -- --                         
> 40:                                                 
> 50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
> 60:                                                 
> 70:                                                 
> 
> I triple checked that I have the QUP pins defined correctly for the 5
> buses. I checked them against what's in the downstream kernel and I also
> checked them against what's in upstream's
> drivers/pinctrl/qcom/pinctrl-sc8280xp.c. This is the pin mapping that I

What's the base of this kernel? Are you sure you have d21f4b7ffc22?

Best regards,
Krzysztof
Konrad Dybcio Dec. 14, 2022, 12:53 p.m. UTC | #7
On 14.12.2022 13:30, Brian Masney wrote:
> On Tue, Dec 13, 2022 at 03:48:27PM +0100, Konrad Dybcio wrote:
>>> +	qup1_i2c15_default: qup1-i2c15-state {
>>> +		mux-pins {
>>> +			pins = "gpio36", "gpio37";
>>> +			function = "qup15";
>>> +		};
>>> +
>>> +		config-pins {
>>> +			pins = "gpio36", "gpio37";
>>> +			drive-strength = <0x02>;
>>> +			bias-pull-up;
>>> +		};
>>> +	};
>>
>> You can drop mux/config-pins and have the pin properties live directly
>> under the qup1-i2cN-state node.
> 
> Hi Konrad (and Shazad below), 
> 
> I need to enable 5 i2c buses (0, 1, 12, 15, 18) on this board. I tried
> the following combinations with the pin mapping configuration and the
> only one that seems to work reliably for me is what I originally had.
> 
> With the following, only 2 out of the 5 buses are detected. There's no
> i2c mesages in dmesg.
> 
>     i2c0_default: i2c0-default-state {
>         pins = "gpio135", "gpio136";
>         function = "qup15";
>     };
> 
> Next, I added a drive-strength and bias-pull-up. All 5 buses are
> detected. One bus throws read errors when I probe it with i2cdetect, two
> others 'i2cdetect -a -y $BUSNUM' takes ~5 seconds to run, and the
> remaining two are fast.
> 
>     i2c0_default: i2c0-default-state {
>         pins = "gpio135", "gpio136";
>         function = "qup15";
>         drive-strength = <2>;
>         bias-pull-up;
>     };
> 
> This is the style where i2cdetect seems to be happy for all 5 buses and
> is fast:
> 
>     i2c0_default: i2c0-default-state {
>         mux-pins {
>             pins = "gpio135", "gpio136";
>             function = "qup0";
>         };
> 
>         config-pins {
>             pins = "gpio135", "gpio136";
>             drive-strength = <2>;
>             bias-pull-up;
>         };
>     };
Unless you made a typo somewhere, I genuinely have no explanation for this..

Konrad
> 
> 
> Shazad: 'i2cdetect -a -y $BUSNUM) shows that all 5 buses have the same
> addresses listening. Is that expected? That seems a bit odd to me.
> 
> [root@localhost ~]# i2cdetect -a -y 0
> Warning: Can't use SMBus Quick Write command, will skip some addresses
>      0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> 00:                                                 
> 10:                                                 
> 20:                                                 
> 30: -- -- -- -- -- -- -- --                         
> 40:                                                 
> 50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- 
> 60:                                                 
> 70:                                                 
> 
> I triple checked that I have the QUP pins defined correctly for the 5
> buses. I checked them against what's in the downstream kernel and I also
> checked them against what's in upstream's
> drivers/pinctrl/qcom/pinctrl-sc8280xp.c. This is the pin mapping that I
> have:
> 
>     i2c0: gpio135, gpio136
>     i2c1: gpio158, gpio159
>     i2c12: gpio0, gpio1
>     i2c15: gpio36, gpio37
>     i2c18: gpio66, gpio67
> 
> Brian
>
Brian Masney Dec. 14, 2022, 2:19 p.m. UTC | #8
On Wed, Dec 14, 2022 at 01:52:17PM +0100, Krzysztof Kozlowski wrote:
> On 14/12/2022 13:30, Brian Masney wrote:
> > I triple checked that I have the QUP pins defined correctly for the 5
> > buses. I checked them against what's in the downstream kernel and I also
> > checked them against what's in upstream's
> > drivers/pinctrl/qcom/pinctrl-sc8280xp.c. This is the pin mapping that I
> 
> What's the base of this kernel? Are you sure you have d21f4b7ffc22?

I'm based on top of linux-next-20221208 with no other changes. I have
that commit.

   commit d21f4b7ffc22c009da925046b69b15af08de9d75
   Author: Douglas Anderson <dianders@chromium.org>
   Date:   Fri Oct 14 10:33:18 2022 -0700

      pinctrl: qcom: Avoid glitching lines when we first mux to output


On Wed, Dec 14, 2022 at 01:53:38PM +0100, Konrad Dybcio wrote:
> > This is the style where i2cdetect seems to be happy for all 5 buses and
> > is fast:
> > 
> >     i2c0_default: i2c0-default-state {
> >         mux-pins {
> >             pins = "gpio135", "gpio136";
> >             function = "qup0";
> >         };
> > 
> >         config-pins {
> >             pins = "gpio135", "gpio136";
> >             drive-strength = <2>;
> >             bias-pull-up;
> >         };
> >     };
> Unless you made a typo somewhere, I genuinely have no explanation for this..

I have my unpublished v2 patch set committed to my tree and a clean tree
according to git. I started with the state that I have quoted above. As I
did the various tests I described in my last email, I would do a
'git diff' just to be sure that I didn't have any typos.

I'll wait to hear from Shazad about whether or not the output that I got
from i2cdetect is supposed to be the same for those 5 buses.

Brian
Shazad Hussain Dec. 14, 2022, 3:36 p.m. UTC | #9
On 12/14/2022 6:00 PM, Brian Masney wrote:
> On Tue, Dec 13, 2022 at 03:48:27PM +0100, Konrad Dybcio wrote:
>>> +	qup1_i2c15_default: qup1-i2c15-state {
>>> +		mux-pins {
>>> +			pins = "gpio36", "gpio37";
>>> +			function = "qup15";
>>> +		};
>>> +
>>> +		config-pins {
>>> +			pins = "gpio36", "gpio37";
>>> +			drive-strength = <0x02>;
>>> +			bias-pull-up;
>>> +		};
>>> +	};
>>
>> You can drop mux/config-pins and have the pin properties live directly
>> under the qup1-i2cN-state node.
> 
> Hi Konrad (and Shazad below),
> 
> I need to enable 5 i2c buses (0, 1, 12, 15, 18) on this board. I tried
> the following combinations with the pin mapping configuration and the
> only one that seems to work reliably for me is what I originally had.
> 
> With the following, only 2 out of the 5 buses are detected. There's no
> i2c mesages in dmesg.
> 
>      i2c0_default: i2c0-default-state {
>          pins = "gpio135", "gpio136";
>          function = "qup15";
>      };
> 
> Next, I added a drive-strength and bias-pull-up. All 5 buses are
> detected. One bus throws read errors when I probe it with i2cdetect, two
> others 'i2cdetect -a -y $BUSNUM' takes ~5 seconds to run, and the

This I have also observed on downstream as well, where scanning all 
addresses takes some amount of time near to 5-6 seconds.

> remaining two are fast.
> 
>      i2c0_default: i2c0-default-state {
>          pins = "gpio135", "gpio136";
>          function = "qup15";
>          drive-strength = <2>;
>          bias-pull-up;
>      };
> 

This is the default config we should use.

> This is the style where i2cdetect seems to be happy for all 5 buses and
> is fast:
> 
>      i2c0_default: i2c0-default-state {
>          mux-pins {
>              pins = "gpio135", "gpio136";
>              function = "qup0";
>          };
> 
>          config-pins {
>              pins = "gpio135", "gpio136";
>              drive-strength = <2>;
>              bias-pull-up;
>          };
>      };
> 
> 
> Shazad: 'i2cdetect -a -y $BUSNUM) shows that all 5 buses have the same
> addresses listening. Is that expected? That seems a bit odd to me.
> 

Brian, even I haven't checked with all enabled, let me check this on 
other projects and with downstream as well and get back to you.

-Shazad

> [root@localhost ~]# i2cdetect -a -y 0
> Warning: Can't use SMBus Quick Write command, will skip some addresses
>       0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
> 00:
> 10:
> 20:
> 30: -- -- -- -- -- -- -- --
> 40:
> 50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
> 60:
> 70:
> 
> I triple checked that I have the QUP pins defined correctly for the 5
> buses. I checked them against what's in the downstream kernel and I also
> checked them against what's in upstream's
> drivers/pinctrl/qcom/pinctrl-sc8280xp.c. This is the pin mapping that I
> have:
> 
>      i2c0: gpio135, gpio136
>      i2c1: gpio158, gpio159
>      i2c12: gpio0, gpio1
>      i2c15: gpio36, gpio37
>      i2c18: gpio66, gpio67
> 
> Brian
>
Brian Masney Dec. 14, 2022, 4:24 p.m. UTC | #10
On Wed, Dec 14, 2022 at 09:06:48PM +0530, Shazad Hussain wrote:
> >      i2c0_default: i2c0-default-state {
> >          pins = "gpio135", "gpio136";
> >          function = "qup15";
> >          drive-strength = <2>;
> >          bias-pull-up;
> >      };
> > 
> 
> This is the default config we should use.

OK, I'll stick with this config.

> > Shazad: 'i2cdetect -a -y $BUSNUM) shows that all 5 buses have the same
> > addresses listening. Is that expected? That seems a bit odd to me.
> > 
> 
> Brian, even I haven't checked with all enabled, let me check this on other
> projects and with downstream as well and get back to you.

I'll post my v2 in a little bit so that you can test it.

Brian
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
index d70859803fbd..6dc3f3ff8ece 100644
--- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
+++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
@@ -17,6 +17,8 @@  / {
 	compatible = "qcom,sa8540p-ride", "qcom,sa8540p";
 
 	aliases {
+		i2c15 = &qup1_i2c15;
+		i2c18 = &qup2_i2c18;
 		serial0 = &qup2_uart17;
 	};
 
@@ -188,10 +190,28 @@  &pcie3a_phy {
 	status = "okay";
 };
 
+&qup1 {
+	status = "okay";
+};
+
+&qup1_i2c15 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&qup1_i2c15_default>;
+
+	status = "okay";
+};
+
 &qup2 {
 	status = "okay";
 };
 
+&qup2_i2c18 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&qup2_i2c18_default>;
+
+	status = "okay";
+};
+
 &qup2_uart17 {
 	compatible = "qcom,geni-debug-uart";
 	status = "okay";
@@ -313,4 +333,30 @@  wake-pins {
 			bias-pull-up;
 		};
 	};
+
+	qup1_i2c15_default: qup1-i2c15-state {
+		mux-pins {
+			pins = "gpio36", "gpio37";
+			function = "qup15";
+		};
+
+		config-pins {
+			pins = "gpio36", "gpio37";
+			drive-strength = <0x02>;
+			bias-pull-up;
+		};
+	};
+
+	qup2_i2c18_default: qup2-i2c18-state {
+		mux-pins {
+			pins = "gpio66", "gpio67";
+			function = "qup18";
+		};
+
+		config-pins {
+			pins = "gpio66", "gpio67";
+			drive-strength = <0x02>;
+			bias-pull-up;
+		};
+	};
 };