| Message ID | 20221212182314.1902632-2-bmasney@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm64: dts: qcom: sc8280xp: add i2c and spi nodes | expand |
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";
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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 --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";
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(-)