Message ID | 20221212182314.1902632-1-bmasney@redhat.com |
---|---|
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. 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 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 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 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 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: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