diff mbox series

[1/4] arm64: dts: qcom: sc8280xp: rename i2c5 to i2c21

Message ID 20221212182314.1902632-2-bmasney@redhat.com
State New
Headers
Series arm64: dts: qcom: sc8280xp: add i2c and spi nodes |

Commit Message

Brian Masney Dec. 12, 2022, 6:23 p.m. UTC 
  According to the downstream 5.4 kernel sources for the sa8540p,
i2c@894000 is labeled i2c bus 21, not 5. The interrupts and clocks
also match. Let's go ahead and correct the name that's used in the
three files where this is listed.

Signed-off-by: Brian Masney <bmasney@redhat.com>
Fixes: 152d1faf1e2f3 ("arm64: dts: qcom: add SC8280XP platform")
Fixes: ccd3517faf183 ("arm64: dts: qcom: sc8280xp: Add reference device")
Fixes: 32c231385ed43 ("arm64: dts: qcom: sc8280xp: add Lenovo Thinkpad X13s devicetree")
---
 arch/arm64/boot/dts/qcom/sc8280xp-crd.dts                  | 6 +++---
 arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 6 +++---
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi                     | 2 +-
 3 files changed, 7 insertions(+), 7 deletions(-)

Comments

Konrad Dybcio Dec. 12, 2022, 6:48 p.m. UTC | #1 
On 12.12.2022 19:23, Brian Masney wrote:
> According to the downstream 5.4 kernel sources for the sa8540p,
> i2c@894000 is labeled i2c bus 21, not 5. The interrupts and clocks
> also match. Let's go ahead and correct the name that's used in the
> three files where this is listed.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> Fixes: 152d1faf1e2f3 ("arm64: dts: qcom: add SC8280XP platform")
> Fixes: ccd3517faf183 ("arm64: dts: qcom: sc8280xp: Add reference device")
> Fixes: 32c231385ed43 ("arm64: dts: qcom: sc8280xp: add Lenovo Thinkpad X13s devicetree")
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>

Konrad
>  arch/arm64/boot/dts/qcom/sc8280xp-crd.dts                  | 6 +++---
>  arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts | 6 +++---
>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi                     | 2 +-
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> index 551768f97729..1ab76724144d 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
> @@ -326,11 +326,11 @@ &qup2 {
>  	status = "okay";
>  };
>  
> -&qup2_i2c5 {
> +&qup2_i2c21 {
>  	clock-frequency = <400000>;
>  
>  	pinctrl-names = "default";
> -	pinctrl-0 = <&qup2_i2c5_default>;
> +	pinctrl-0 = <&qup2_i2c21_default>;
>  
>  	status = "okay";
>  
> @@ -598,7 +598,7 @@ qup0_i2c4_default: qup0-i2c4-default-state {
>  		drive-strength = <16>;
>  	};
>  
> -	qup2_i2c5_default: qup2-i2c5-default-state {
> +	qup2_i2c21_default: qup2-i2c21-default-state {
>  		pins = "gpio81", "gpio82";
>  		function = "qup21";
>  
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> index 568c6be1ceaa..284adf60386a 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
> @@ -531,11 +531,11 @@ &qup2 {
>  	status = "okay";
>  };
>  
> -&qup2_i2c5 {
> +&qup2_i2c21 {
>  	clock-frequency = <400000>;
>  
>  	pinctrl-names = "default";
> -	pinctrl-0 = <&qup2_i2c5_default>;
> +	pinctrl-0 = <&qup2_i2c21_default>;
>  
>  	status = "okay";
>  
> @@ -801,7 +801,7 @@ qup0_i2c4_default: qup0-i2c4-default-state {
>  		drive-strength = <16>;
>  	};
>  
> -	qup2_i2c5_default: qup2-i2c5-default-state {
> +	qup2_i2c21_default: qup2-i2c21-default-state {
>  		pins = "gpio81", "gpio82";
>  		function = "qup21";
>  		bias-disable;
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index 109c9d2b684d..875cc91324ce 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -827,7 +827,7 @@ qup2_uart17: serial@884000 {
>  				status = "disabled";
>  			};
>  
> -			qup2_i2c5: i2c@894000 {
> +			qup2_i2c21: i2c@894000 {
>  				compatible = "qcom,geni-i2c";
>  				reg = <0 0x00894000 0 0x4000>;
>  				clock-names = "se";
Johan Hovold Dec. 13, 2022, 2:54 p.m. UTC | #2 
On Mon, Dec 12, 2022 at 01:23:11PM -0500, Brian Masney wrote:
> According to the downstream 5.4 kernel sources for the sa8540p,
> i2c@894000 is labeled i2c bus 21, not 5. The interrupts and clocks
> also match. Let's go ahead and correct the name that's used in the
> three files where this is listed.
> 
> Signed-off-by: Brian Masney <bmasney@redhat.com>
> Fixes: 152d1faf1e2f3 ("arm64: dts: qcom: add SC8280XP platform")
> Fixes: ccd3517faf183 ("arm64: dts: qcom: sc8280xp: Add reference device")
> Fixes: 32c231385ed43 ("arm64: dts: qcom: sc8280xp: add Lenovo Thinkpad X13s devicetree")

> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index 109c9d2b684d..875cc91324ce 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -827,7 +827,7 @@ qup2_uart17: serial@884000 {
>  				status = "disabled";
>  			};
>  
> -			qup2_i2c5: i2c@894000 {
> +			qup2_i2c21: i2c@894000 {

Note that the node is labelled qup2_i2c5 and not qup_i2c5.

That is, the QUP nodes are labelled using two indices, and specifically

	qup2_i2c5

would be another name for

	qup_i2c21

if we'd been using such a flat naming scheme (there are 8 engines per
QUP).

So there's nothing wrong with how these nodes are currently named, but
mixing the two scheme as you are suggesting would not be correct.

Johan
Shazad Hussain Dec. 13, 2022, 3:04 p.m. UTC | #3 
On 12/13/2022 8:24 PM, Johan Hovold wrote:
> On Mon, Dec 12, 2022 at 01:23:11PM -0500, Brian Masney wrote:
>> According to the downstream 5.4 kernel sources for the sa8540p,
>> i2c@894000 is labeled i2c bus 21, not 5. The interrupts and clocks
>> also match. Let's go ahead and correct the name that's used in the
>> three files where this is listed.
>>
>> Signed-off-by: Brian Masney <bmasney@redhat.com>
>> Fixes: 152d1faf1e2f3 ("arm64: dts: qcom: add SC8280XP platform")
>> Fixes: ccd3517faf183 ("arm64: dts: qcom: sc8280xp: Add reference device")
>> Fixes: 32c231385ed43 ("arm64: dts: qcom: sc8280xp: add Lenovo Thinkpad X13s devicetree")
> 
>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> index 109c9d2b684d..875cc91324ce 100644
>> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> @@ -827,7 +827,7 @@ qup2_uart17: serial@884000 {
>>   				status = "disabled";
>>   			};
>>   
>> -			qup2_i2c5: i2c@894000 {
>> +			qup2_i2c21: i2c@894000 {
> 
> Note that the node is labelled qup2_i2c5 and not qup_i2c5.
> 
> That is, the QUP nodes are labelled using two indices, and specifically
> 
> 	qup2_i2c5
> 
> would be another name for
> 
> 	qup_i2c21
> 
> if we'd been using such a flat naming scheme (there are 8 engines per
> QUP).
> 
> So there's nothing wrong with how these nodes are currently named, but
> mixing the two scheme as you are suggesting would not be correct.

Wondering we might need to change qup2_uart17 to qup2_uart1 then ?

Shazad

> 
> Johan
Brian Masney Dec. 13, 2022, 3:05 p.m. UTC | #4 
On Tue, Dec 13, 2022 at 03:54:05PM +0100, Johan Hovold wrote:
> Note that the node is labelled qup2_i2c5 and not qup_i2c5.
> 
> That is, the QUP nodes are labelled using two indices, and specifically
> 
> 	qup2_i2c5
> 
> would be another name for
> 
> 	qup_i2c21
> 
> if we'd been using such a flat naming scheme (there are 8 engines per
> QUP).
> 
> So there's nothing wrong with how these nodes are currently named, but
> mixing the two scheme as you are suggesting would not be correct.

OK, I see; that makes sense. I'll drop this patch in v2 and fix up the
new nodes accordingly. Thank you for the explanation!

Brian
Brian Masney Dec. 13, 2022, 3:12 p.m. UTC | #5 
On Tue, Dec 13, 2022 at 03:54:05PM +0100, Johan Hovold wrote:
> Note that the node is labelled qup2_i2c5 and not qup_i2c5.
> 
> That is, the QUP nodes are labelled using two indices, and specifically
> 
> 	qup2_i2c5
> 
> would be another name for
> 
> 	qup_i2c21
> 
> if we'd been using such a flat naming scheme (there are 8 engines per
> QUP).
> 
> So there's nothing wrong with how these nodes are currently named, but
> mixing the two scheme as you are suggesting would not be correct.

Hi Johan,

What would I use for the name in the aliases section? Right now I have:

    aliases {
        i2c18 = &qup2_i2c18;
    }

So qup2_i2c18 becomes qup2_i2c2. Would I use the flat naming scheme for
the alias like so?

    aliases {
        i2c18 = &qup2_i2c2;
    }

Brian
Johan Hovold Dec. 13, 2022, 3:17 p.m. UTC | #6 
On Tue, Dec 13, 2022 at 03:54:05PM +0100, Johan Hovold wrote:
> On Mon, Dec 12, 2022 at 01:23:11PM -0500, Brian Masney wrote:
> > According to the downstream 5.4 kernel sources for the sa8540p,
> > i2c@894000 is labeled i2c bus 21, not 5. The interrupts and clocks
> > also match. Let's go ahead and correct the name that's used in the
> > three files where this is listed.
> > 
> > Signed-off-by: Brian Masney <bmasney@redhat.com>
> > Fixes: 152d1faf1e2f3 ("arm64: dts: qcom: add SC8280XP platform")
> > Fixes: ccd3517faf183 ("arm64: dts: qcom: sc8280xp: Add reference device")
> > Fixes: 32c231385ed43 ("arm64: dts: qcom: sc8280xp: add Lenovo Thinkpad X13s devicetree")
> 
> > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > index 109c9d2b684d..875cc91324ce 100644
> > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > @@ -827,7 +827,7 @@ qup2_uart17: serial@884000 {
> >  				status = "disabled";
> >  			};
> >  
> > -			qup2_i2c5: i2c@894000 {
> > +			qup2_i2c21: i2c@894000 {
> 
> Note that the node is labelled qup2_i2c5 and not qup_i2c5.
> 
> That is, the QUP nodes are labelled using two indices, and specifically
> 
> 	qup2_i2c5
> 
> would be another name for
> 
> 	qup_i2c21
> 
> if we'd been using such a flat naming scheme (there are 8 engines per
> QUP).
> 
> So there's nothing wrong with how these nodes are currently named, but
> mixing the two scheme as you are suggesting would not be correct.

It appears sc8280xp is the only qcom platform using a qup prefix (even
if some older platform use a blsp equivalent), and we're not even using
it consistently as we, for example, have both

	qup2_uart17, and
	qup2_i2c5

(where the former should have been qup2_uart1).

So either we fix up the current labels or just drop the qup prefixes and
use a flat naming scheme (e.g. uart17 and i2c21).

Either way, there's no need for any Fixes tags as this isn't a bug.

Johan
Johan Hovold Dec. 13, 2022, 3:19 p.m. UTC | #7 
On Tue, Dec 13, 2022 at 08:34:56PM +0530, Shazad Hussain wrote:
> 
> 
> On 12/13/2022 8:24 PM, Johan Hovold wrote:
> > On Mon, Dec 12, 2022 at 01:23:11PM -0500, Brian Masney wrote:
> >> According to the downstream 5.4 kernel sources for the sa8540p,
> >> i2c@894000 is labeled i2c bus 21, not 5. The interrupts and clocks
> >> also match. Let's go ahead and correct the name that's used in the
> >> three files where this is listed.
> >>
> >> Signed-off-by: Brian Masney <bmasney@redhat.com>
> >> Fixes: 152d1faf1e2f3 ("arm64: dts: qcom: add SC8280XP platform")
> >> Fixes: ccd3517faf183 ("arm64: dts: qcom: sc8280xp: Add reference device")
> >> Fixes: 32c231385ed43 ("arm64: dts: qcom: sc8280xp: add Lenovo Thinkpad X13s devicetree")
> > 
> >> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> >> index 109c9d2b684d..875cc91324ce 100644
> >> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> >> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> >> @@ -827,7 +827,7 @@ qup2_uart17: serial@884000 {
> >>   				status = "disabled";
> >>   			};
> >>   
> >> -			qup2_i2c5: i2c@894000 {
> >> +			qup2_i2c21: i2c@894000 {
> > 
> > Note that the node is labelled qup2_i2c5 and not qup_i2c5.
> > 
> > That is, the QUP nodes are labelled using two indices, and specifically
> > 
> > 	qup2_i2c5
> > 
> > would be another name for
> > 
> > 	qup_i2c21
> > 
> > if we'd been using such a flat naming scheme (there are 8 engines per
> > QUP).
> > 
> > So there's nothing wrong with how these nodes are currently named, but
> > mixing the two scheme as you are suggesting would not be correct.
> 
> Wondering we might need to change qup2_uart17 to qup2_uart1 then ?

Right, I just noticed that too.

Johan
Johan Hovold Dec. 13, 2022, 3:28 p.m. UTC | #8 
On Tue, Dec 13, 2022 at 10:12:57AM -0500, Brian Masney wrote:
> On Tue, Dec 13, 2022 at 03:54:05PM +0100, Johan Hovold wrote:
> > Note that the node is labelled qup2_i2c5 and not qup_i2c5.
> > 
> > That is, the QUP nodes are labelled using two indices, and specifically
> > 
> > 	qup2_i2c5
> > 
> > would be another name for
> > 
> > 	qup_i2c21
> > 
> > if we'd been using such a flat naming scheme (there are 8 engines per
> > QUP).
> > 
> > So there's nothing wrong with how these nodes are currently named, but
> > mixing the two scheme as you are suggesting would not be correct.
> 
> Hi Johan,
> 
> What would I use for the name in the aliases section? Right now I have:
> 
>     aliases {
>         i2c18 = &qup2_i2c18;
>     }
> 
> So qup2_i2c18 becomes qup2_i2c2. Would I use the flat naming scheme for
> the alias like so?
> 
>     aliases {
>         i2c18 = &qup2_i2c2;
>     }

Or perhaps the i2c controllers should use a zero-based index instead of
being named after the serial engines (e.g. as we do for the console
uart).

How are they named in the schematics?

Johan
Konrad Dybcio Dec. 13, 2022, 3:29 p.m. UTC | #9 
On 13.12.2022 16:17, Johan Hovold wrote:
> On Tue, Dec 13, 2022 at 03:54:05PM +0100, Johan Hovold wrote:
>> On Mon, Dec 12, 2022 at 01:23:11PM -0500, Brian Masney wrote:
>>> According to the downstream 5.4 kernel sources for the sa8540p,
>>> i2c@894000 is labeled i2c bus 21, not 5. The interrupts and clocks
>>> also match. Let's go ahead and correct the name that's used in the
>>> three files where this is listed.
>>>
>>> Signed-off-by: Brian Masney <bmasney@redhat.com>
>>> Fixes: 152d1faf1e2f3 ("arm64: dts: qcom: add SC8280XP platform")
>>> Fixes: ccd3517faf183 ("arm64: dts: qcom: sc8280xp: Add reference device")
>>> Fixes: 32c231385ed43 ("arm64: dts: qcom: sc8280xp: add Lenovo Thinkpad X13s devicetree")
>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>> index 109c9d2b684d..875cc91324ce 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>> @@ -827,7 +827,7 @@ qup2_uart17: serial@884000 {
>>>  				status = "disabled";
>>>  			};
>>>  
>>> -			qup2_i2c5: i2c@894000 {
>>> +			qup2_i2c21: i2c@894000 {
>>
>> Note that the node is labelled qup2_i2c5 and not qup_i2c5.
>>
>> That is, the QUP nodes are labelled using two indices, and specifically
>>
>> 	qup2_i2c5
>>
>> would be another name for
>>
>> 	qup_i2c21
>>
>> if we'd been using such a flat naming scheme (there are 8 engines per
>> QUP).
>>
>> So there's nothing wrong with how these nodes are currently named, but
>> mixing the two scheme as you are suggesting would not be correct.
> 
> It appears sc8280xp is the only qcom platform using a qup prefix (even
> if some older platform use a blsp equivalent), and we're not even using
> it consistently as we, for example, have both
> 
> 	qup2_uart17, and
> 	qup2_i2c5
> 
> (where the former should have been qup2_uart1).
> 
> So either we fix up the current labels or just drop the qup prefixes and
> use a flat naming scheme (e.g. uart17 and i2c21).
Oh, I didn't notice that! I suppose sticking with i2cN as we've been
doing ever since i2c-geni was introduced sounds like the best option..

Konrad
> 
> Either way, there's no need for any Fixes tags as this isn't a bug.
> 
> Johan
Johan Hovold Dec. 13, 2022, 3:32 p.m. UTC | #10 
On Tue, Dec 13, 2022 at 04:29:04PM +0100, Konrad Dybcio wrote:
> 
> 
> On 13.12.2022 16:17, Johan Hovold wrote:
> > On Tue, Dec 13, 2022 at 03:54:05PM +0100, Johan Hovold wrote:
> >> On Mon, Dec 12, 2022 at 01:23:11PM -0500, Brian Masney wrote:
> >>> According to the downstream 5.4 kernel sources for the sa8540p,
> >>> i2c@894000 is labeled i2c bus 21, not 5. The interrupts and clocks
> >>> also match. Let's go ahead and correct the name that's used in the
> >>> three files where this is listed.
> >>>
> >>> Signed-off-by: Brian Masney <bmasney@redhat.com>
> >>> Fixes: 152d1faf1e2f3 ("arm64: dts: qcom: add SC8280XP platform")
> >>> Fixes: ccd3517faf183 ("arm64: dts: qcom: sc8280xp: Add reference device")
> >>> Fixes: 32c231385ed43 ("arm64: dts: qcom: sc8280xp: add Lenovo Thinkpad X13s devicetree")
> >>
> >>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> >>> index 109c9d2b684d..875cc91324ce 100644
> >>> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> >>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> >>> @@ -827,7 +827,7 @@ qup2_uart17: serial@884000 {
> >>>  				status = "disabled";
> >>>  			};
> >>>  
> >>> -			qup2_i2c5: i2c@894000 {
> >>> +			qup2_i2c21: i2c@894000 {
> >>
> >> Note that the node is labelled qup2_i2c5 and not qup_i2c5.
> >>
> >> That is, the QUP nodes are labelled using two indices, and specifically
> >>
> >> 	qup2_i2c5
> >>
> >> would be another name for
> >>
> >> 	qup_i2c21
> >>
> >> if we'd been using such a flat naming scheme (there are 8 engines per
> >> QUP).
> >>
> >> So there's nothing wrong with how these nodes are currently named, but
> >> mixing the two scheme as you are suggesting would not be correct.
> > 
> > It appears sc8280xp is the only qcom platform using a qup prefix (even
> > if some older platform use a blsp equivalent), and we're not even using
> > it consistently as we, for example, have both
> > 
> > 	qup2_uart17, and
> > 	qup2_i2c5
> > 
> > (where the former should have been qup2_uart1).
> > 
> > So either we fix up the current labels or just drop the qup prefixes and
> > use a flat naming scheme (e.g. uart17 and i2c21).

> Oh, I didn't notice that! I suppose sticking with i2cN as we've been
> doing ever since i2c-geni was introduced sounds like the best option..

Yeah, sounds good to me.

Johan
Shazad Hussain Dec. 13, 2022, 3:34 p.m. UTC | #11 
On 12/13/2022 8:58 PM, Johan Hovold wrote:
> On Tue, Dec 13, 2022 at 10:12:57AM -0500, Brian Masney wrote:
>> On Tue, Dec 13, 2022 at 03:54:05PM +0100, Johan Hovold wrote:
>>> Note that the node is labelled qup2_i2c5 and not qup_i2c5.
>>>
>>> That is, the QUP nodes are labelled using two indices, and specifically
>>>
>>> 	qup2_i2c5
>>>
>>> would be another name for
>>>
>>> 	qup_i2c21
>>>
>>> if we'd been using such a flat naming scheme (there are 8 engines per
>>> QUP).
>>>
>>> So there's nothing wrong with how these nodes are currently named, but
>>> mixing the two scheme as you are suggesting would not be correct.
>>
>> Hi Johan,
>>
>> What would I use for the name in the aliases section? Right now I have:
>>
>>      aliases {
>>          i2c18 = &qup2_i2c18;
>>      }
>>
>> So qup2_i2c18 becomes qup2_i2c2. Would I use the flat naming scheme for
>> the alias like so?
>>
>>      aliases {
>>          i2c18 = &qup2_i2c2;
>>      }
> 
> Or perhaps the i2c controllers should use a zero-based index instead of
> being named after the serial engines (e.g. as we do for the console
> uart).
> 
> How are they named in the schematics?
> 

We should use from 0 to N.

Shazad

> Johan
Johan Hovold Dec. 13, 2022, 3:39 p.m. UTC | #12 
On Tue, Dec 13, 2022 at 09:04:39PM +0530, Shazad Hussain wrote:
> 
> 
> On 12/13/2022 8:58 PM, Johan Hovold wrote:
> > On Tue, Dec 13, 2022 at 10:12:57AM -0500, Brian Masney wrote:
> >> On Tue, Dec 13, 2022 at 03:54:05PM +0100, Johan Hovold wrote:
> >>> Note that the node is labelled qup2_i2c5 and not qup_i2c5.
> >>>
> >>> That is, the QUP nodes are labelled using two indices, and specifically
> >>>
> >>> 	qup2_i2c5
> >>>
> >>> would be another name for
> >>>
> >>> 	qup_i2c21
> >>>
> >>> if we'd been using such a flat naming scheme (there are 8 engines per
> >>> QUP).
> >>>
> >>> So there's nothing wrong with how these nodes are currently named, but
> >>> mixing the two scheme as you are suggesting would not be correct.
> >>
> >> Hi Johan,
> >>
> >> What would I use for the name in the aliases section? Right now I have:
> >>
> >>      aliases {
> >>          i2c18 = &qup2_i2c18;
> >>      }
> >>
> >> So qup2_i2c18 becomes qup2_i2c2. Would I use the flat naming scheme for
> >> the alias like so?
> >>
> >>      aliases {
> >>          i2c18 = &qup2_i2c2;
> >>      }
> > 
> > Or perhaps the i2c controllers should use a zero-based index instead of
> > being named after the serial engines (e.g. as we do for the console
> > uart).
> > 
> > How are they named in the schematics?
> 
> We should use from 0 to N.

With N being 23 after the number of serial engines, or the number of
available i2c buses on a particular board minus one?

Johan
Johan Hovold Dec. 13, 2022, 3:42 p.m. UTC | #13 
On Tue, Dec 13, 2022 at 04:39:54PM +0100, Johan Hovold wrote:
> On Tue, Dec 13, 2022 at 09:04:39PM +0530, Shazad Hussain wrote:
> > On 12/13/2022 8:58 PM, Johan Hovold wrote:

> > >> So qup2_i2c18 becomes qup2_i2c2. Would I use the flat naming scheme for
> > >> the alias like so?
> > >>
> > >>      aliases {
> > >>          i2c18 = &qup2_i2c2;
> > >>      }
> > > 
> > > Or perhaps the i2c controllers should use a zero-based index instead of
> > > being named after the serial engines (e.g. as we do for the console
> > > uart).
> > > 
> > > How are they named in the schematics?
> > 
> > We should use from 0 to N.
> 
> With N being 23 after the number of serial engines, or the number of
> available i2c buses on a particular board minus one?

Looks like the more recent Qualcomm platforms use aliases that reflect
the engine number (i.e. 0 to 23) for i2c and spi.

Johan
Konrad Dybcio Dec. 13, 2022, 3:44 p.m. UTC | #14 
On 13.12.2022 16:42, Johan Hovold wrote:
> On Tue, Dec 13, 2022 at 04:39:54PM +0100, Johan Hovold wrote:
>> On Tue, Dec 13, 2022 at 09:04:39PM +0530, Shazad Hussain wrote:
>>> On 12/13/2022 8:58 PM, Johan Hovold wrote:
> 
>>>>> So qup2_i2c18 becomes qup2_i2c2. Would I use the flat naming scheme for
>>>>> the alias like so?
>>>>>
>>>>>      aliases {
>>>>>          i2c18 = &qup2_i2c2;
>>>>>      }
>>>>
>>>> Or perhaps the i2c controllers should use a zero-based index instead of
>>>> being named after the serial engines (e.g. as we do for the console
>>>> uart).
>>>>
>>>> How are they named in the schematics?
>>>
>>> We should use from 0 to N.
>>
>> With N being 23 after the number of serial engines, or the number of
>> available i2c buses on a particular board minus one?
> 
> Looks like the more recent Qualcomm platforms use aliases that reflect
> the engine number (i.e. 0 to 23) for i2c and spi.
IMO it makes the most sense, as it tells the userspace "hello, this
device is connected to the physical I2Cn on the SoC" as opposed to
"hello, this device is connected to the nth enabled bus on this
particular board".

Konrad
> 
> Johan
Shazad Hussain Dec. 13, 2022, 3:45 p.m. UTC | #15 
On 12/13/2022 9:09 PM, Johan Hovold wrote:
> On Tue, Dec 13, 2022 at 09:04:39PM +0530, Shazad Hussain wrote:
>>
>>
>> On 12/13/2022 8:58 PM, Johan Hovold wrote:
>>> On Tue, Dec 13, 2022 at 10:12:57AM -0500, Brian Masney wrote:
>>>> On Tue, Dec 13, 2022 at 03:54:05PM +0100, Johan Hovold wrote:
>>>>> Note that the node is labelled qup2_i2c5 and not qup_i2c5.
>>>>>
>>>>> That is, the QUP nodes are labelled using two indices, and specifically
>>>>>
>>>>> 	qup2_i2c5
>>>>>
>>>>> would be another name for
>>>>>
>>>>> 	qup_i2c21
>>>>>
>>>>> if we'd been using such a flat naming scheme (there are 8 engines per
>>>>> QUP).
>>>>>
>>>>> So there's nothing wrong with how these nodes are currently named, but
>>>>> mixing the two scheme as you are suggesting would not be correct.
>>>>
>>>> Hi Johan,
>>>>
>>>> What would I use for the name in the aliases section? Right now I have:
>>>>
>>>>       aliases {
>>>>           i2c18 = &qup2_i2c18;
>>>>       }
>>>>
>>>> So qup2_i2c18 becomes qup2_i2c2. Would I use the flat naming scheme for
>>>> the alias like so?
>>>>
>>>>       aliases {
>>>>           i2c18 = &qup2_i2c2;
>>>>       }
>>>
>>> Or perhaps the i2c controllers should use a zero-based index instead of
>>> being named after the serial engines (e.g. as we do for the console
>>> uart).
>>>
>>> How are they named in the schematics?
>>
>> We should use from 0 to N.
> 
> With N being 23 after the number of serial engines, or the number of
> available i2c buses on a particular board minus one?
> 

wrt to serial engine number starting from 0.

Shazad

> Johan
Brian Masney Dec. 13, 2022, 3:59 p.m. UTC | #16 
On Tue, Dec 13, 2022 at 04:32:43PM +0100, Johan Hovold wrote:
> On Tue, Dec 13, 2022 at 04:29:04PM +0100, Konrad Dybcio wrote:
> > On 13.12.2022 16:17, Johan Hovold wrote:
> > > It appears sc8280xp is the only qcom platform using a qup prefix (even
> > > if some older platform use a blsp equivalent), and we're not even using
> > > it consistently as we, for example, have both
> > > 
> > > 	qup2_uart17, and
> > > 	qup2_i2c5
> > > 
> > > (where the former should have been qup2_uart1).
> > > 
> > > So either we fix up the current labels or just drop the qup prefixes and
> > > use a flat naming scheme (e.g. uart17 and i2c21).
> 
> > Oh, I didn't notice that! I suppose sticking with i2cN as we've been
> > doing ever since i2c-geni was introduced sounds like the best option..
> 
> Yeah, sounds good to me.

This makes sense and I'll fix up the existing geni nodes and my new
nodes in v2.

I noticed another inconsistency with sc8280xp.dtsi compared to other
platforms. I left off all of the pin mappings in sc8280xp.dtsi and
added them to the sa8540-ride.dts file since the existing sc8280xp.dtsi
file contains no pin mappings. Other platforms such as sm8450.dtsi,
sm8350.dtsi, and sm8250.dtsi contain the geni pin mappings. My
understanding is that these geni pins are fixed within the SoC and
don't change with the different boards. Should I also add the geni
pin mappings to sc8280xp.dtsi?

Brian
Johan Hovold Dec. 13, 2022, 4:15 p.m. UTC | #17 
On Tue, Dec 13, 2022 at 04:44:15PM +0100, Konrad Dybcio wrote:
> 
> 
> On 13.12.2022 16:42, Johan Hovold wrote:
> > On Tue, Dec 13, 2022 at 04:39:54PM +0100, Johan Hovold wrote:
> >> On Tue, Dec 13, 2022 at 09:04:39PM +0530, Shazad Hussain wrote:
> >>> On 12/13/2022 8:58 PM, Johan Hovold wrote:
> > 
> >>>>> So qup2_i2c18 becomes qup2_i2c2. Would I use the flat naming scheme for
> >>>>> the alias like so?
> >>>>>
> >>>>>      aliases {
> >>>>>          i2c18 = &qup2_i2c2;
> >>>>>      }
> >>>>
> >>>> Or perhaps the i2c controllers should use a zero-based index instead of
> >>>> being named after the serial engines (e.g. as we do for the console
> >>>> uart).
> >>>>
> >>>> How are they named in the schematics?
> >>>
> >>> We should use from 0 to N.
> >>
> >> With N being 23 after the number of serial engines, or the number of
> >> available i2c buses on a particular board minus one?
> > 
> > Looks like the more recent Qualcomm platforms use aliases that reflect
> > the engine number (i.e. 0 to 23) for i2c and spi.
> IMO it makes the most sense, as it tells the userspace "hello, this
> device is connected to the physical I2Cn on the SoC" as opposed to
> "hello, this device is connected to the nth enabled bus on this
> particular board".

But I guess it still depends on the board. I wouldn't expect a product
with four serial ports to use the engine numbers on labels for the
connectors for example.

Johan
Johan Hovold Dec. 13, 2022, 4:22 p.m. UTC | #18 
On Tue, Dec 13, 2022 at 10:59:47AM -0500, Brian Masney wrote:

> I noticed another inconsistency with sc8280xp.dtsi compared to other
> platforms. I left off all of the pin mappings in sc8280xp.dtsi and
> added them to the sa8540-ride.dts file since the existing sc8280xp.dtsi
> file contains no pin mappings. Other platforms such as sm8450.dtsi,
> sm8350.dtsi, and sm8250.dtsi contain the geni pin mappings. My
> understanding is that these geni pins are fixed within the SoC and
> don't change with the different boards. Should I also add the geni
> pin mappings to sc8280xp.dtsi?

The pins are fixed but the pin configuration is still board specific.

This came up earlier and we decided that keeping all pin configuration
in the board dts was the way to go (e.g. for consistency and as it
allows the integrator to easily review the actual configuration).

Johan
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
index 551768f97729..1ab76724144d 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-crd.dts
@@ -326,11 +326,11 @@  &qup2 {
 	status = "okay";
 };
 
-&qup2_i2c5 {
+&qup2_i2c21 {
 	clock-frequency = <400000>;
 
 	pinctrl-names = "default";
-	pinctrl-0 = <&qup2_i2c5_default>;
+	pinctrl-0 = <&qup2_i2c21_default>;
 
 	status = "okay";
 
@@ -598,7 +598,7 @@  qup0_i2c4_default: qup0-i2c4-default-state {
 		drive-strength = <16>;
 	};
 
-	qup2_i2c5_default: qup2-i2c5-default-state {
+	qup2_i2c21_default: qup2-i2c21-default-state {
 		pins = "gpio81", "gpio82";
 		function = "qup21";
 
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
index 568c6be1ceaa..284adf60386a 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
+++ b/arch/arm64/boot/dts/qcom/sc8280xp-lenovo-thinkpad-x13s.dts
@@ -531,11 +531,11 @@  &qup2 {
 	status = "okay";
 };
 
-&qup2_i2c5 {
+&qup2_i2c21 {
 	clock-frequency = <400000>;
 
 	pinctrl-names = "default";
-	pinctrl-0 = <&qup2_i2c5_default>;
+	pinctrl-0 = <&qup2_i2c21_default>;
 
 	status = "okay";
 
@@ -801,7 +801,7 @@  qup0_i2c4_default: qup0-i2c4-default-state {
 		drive-strength = <16>;
 	};
 
-	qup2_i2c5_default: qup2-i2c5-default-state {
+	qup2_i2c21_default: qup2-i2c21-default-state {
 		pins = "gpio81", "gpio82";
 		function = "qup21";
 		bias-disable;
diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index 109c9d2b684d..875cc91324ce 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -827,7 +827,7 @@  qup2_uart17: serial@884000 {
 				status = "disabled";
 			};
 
-			qup2_i2c5: i2c@894000 {
+			qup2_i2c21: i2c@894000 {
 				compatible = "qcom,geni-i2c";
 				reg = <0 0x00894000 0 0x4000>;
 				clock-names = "se";