Add aliases for i2c and spi for sc7280 soc.
Signed-off-by: Rajesh Patil <[email protected]>
Reviewed-by: Stephen Boyd <[email protected]>
---
Change in V10:
- No changes
Changes in V9:
- No changes
Changes in V8:
- No changes
Changes in V7:
- As per Stephen's comments, Sorted alias names for i2c and spi as per alphabet order
Changes in V6:
- As per Doug's comments, added aliases for i2c and spi
arch/arm64/boot/dts/qcom/sc7280.dtsi | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
index c26647a..e5fefd1 100644
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -26,8 +26,40 @@
chosen { };
aliases {
+ i2c0 = &i2c0;
+ i2c1 = &i2c1;
+ i2c2 = &i2c2;
+ i2c3 = &i2c3;
+ i2c4 = &i2c4;
+ i2c5 = &i2c5;
+ i2c6 = &i2c6;
+ i2c7 = &i2c7;
+ i2c8 = &i2c8;
+ i2c9 = &i2c9;
+ i2c10 = &i2c10;
+ i2c11 = &i2c11;
+ i2c12 = &i2c12;
+ i2c13 = &i2c13;
+ i2c14 = &i2c14;
+ i2c15 = &i2c15;
mmc1 = &sdhc_1;
mmc2 = &sdhc_2;
+ spi0 = &spi0;
+ spi1 = &spi1;
+ spi2 = &spi2;
+ spi3 = &spi3;
+ spi4 = &spi4;
+ spi5 = &spi5;
+ spi6 = &spi6;
+ spi7 = &spi7;
+ spi8 = &spi8;
+ spi9 = &spi9;
+ spi10 = &spi10;
+ spi11 = &spi11;
+ spi12 = &spi12;
+ spi13 = &spi13;
+ spi14 = &spi14;
+ spi15 = &spi15;
};
clocks {
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
On Thu, Sep 23, 2021 at 2:18 PM Rajesh Patil <[email protected]> wrote:
>
> Add aliases for i2c and spi for sc7280 soc.
>
> Signed-off-by: Rajesh Patil <[email protected]>
> Reviewed-by: Stephen Boyd <[email protected]>
I saw this in the pull request, can this please be reverted?
Putting the aliases into the .dtsi file is really silly, as there are
likely boards that
don't connect every single one of those, and then will have to
override and renumber
them.
Please only list the aliases that are actually connected on a particular
board.
Arnd
> diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> index c26647a..e5fefd1 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> @@ -26,8 +26,40 @@
> chosen { };
>
> aliases {
> + i2c0 = &i2c0;
> + i2c1 = &i2c1;
> + i2c2 = &i2c2;
> + i2c3 = &i2c3;
> + i2c4 = &i2c4;
> + i2c5 = &i2c5;
> + i2c6 = &i2c6;
> + i2c7 = &i2c7;
> + i2c8 = &i2c8;
> + i2c9 = &i2c9;
> + i2c10 = &i2c10;
> + i2c11 = &i2c11;
> + i2c12 = &i2c12;
> + i2c13 = &i2c13;
> + i2c14 = &i2c14;
> + i2c15 = &i2c15;
> mmc1 = &sdhc_1;
> mmc2 = &sdhc_2;
The mmc ones should probably go away as well.
> + spi0 = &spi0;
> + spi1 = &spi1;
> + spi2 = &spi2;
> + spi3 = &spi3;
> + spi4 = &spi4;
> + spi5 = &spi5;
> + spi6 = &spi6;
> + spi7 = &spi7;
> + spi8 = &spi8;
> + spi9 = &spi9;
> + spi10 = &spi10;
> + spi11 = &spi11;
> + spi12 = &spi12;
> + spi13 = &spi13;
> + spi14 = &spi14;
> + spi15 = &spi15;
> };
>
On Tue 19 Oct 13:43 PDT 2021, Arnd Bergmann wrote:
> On Thu, Sep 23, 2021 at 2:18 PM Rajesh Patil <[email protected]> wrote:
> >
> > Add aliases for i2c and spi for sc7280 soc.
> >
> > Signed-off-by: Rajesh Patil <[email protected]>
> > Reviewed-by: Stephen Boyd <[email protected]>
>
> I saw this in the pull request, can this please be reverted?
>
Yes, this can certainly be corrected.
> Putting the aliases into the .dtsi file is really silly, as there are
> likely boards that
> don't connect every single one of those, and then will have to
> override and renumber
> them.
>
> Please only list the aliases that are actually connected on a particular
> board.
>
> Arnd
>
> > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > index c26647a..e5fefd1 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
> > @@ -26,8 +26,40 @@
> > chosen { };
> >
> > aliases {
> > + i2c0 = &i2c0;
> > + i2c1 = &i2c1;
> > + i2c2 = &i2c2;
> > + i2c3 = &i2c3;
> > + i2c4 = &i2c4;
> > + i2c5 = &i2c5;
> > + i2c6 = &i2c6;
> > + i2c7 = &i2c7;
> > + i2c8 = &i2c8;
> > + i2c9 = &i2c9;
> > + i2c10 = &i2c10;
> > + i2c11 = &i2c11;
> > + i2c12 = &i2c12;
> > + i2c13 = &i2c13;
> > + i2c14 = &i2c14;
> > + i2c15 = &i2c15;
> > mmc1 = &sdhc_1;
> > mmc2 = &sdhc_2;
>
> The mmc ones should probably go away as well.
>
I should have paid more attention when applying this patch, because the
commit message should have stated why any of these were introduced.
@Rajesh, can you please help me understand the need for any of these and
prepare a patch that introduce the specific ones needed in the
individual board dts(i) files - with reasoning for the aliases in the
commit message.
Thanks,
Bjorn
> > + spi0 = &spi0;
> > + spi1 = &spi1;
> > + spi2 = &spi2;
> > + spi3 = &spi3;
> > + spi4 = &spi4;
> > + spi5 = &spi5;
> > + spi6 = &spi6;
> > + spi7 = &spi7;
> > + spi8 = &spi8;
> > + spi9 = &spi9;
> > + spi10 = &spi10;
> > + spi11 = &spi11;
> > + spi12 = &spi12;
> > + spi13 = &spi13;
> > + spi14 = &spi14;
> > + spi15 = &spi15;
> > };
> >
Hi,
On Tue, Oct 19, 2021 at 1:57 PM Bjorn Andersson
<[email protected]> wrote:
>
> On Tue 19 Oct 13:43 PDT 2021, Arnd Bergmann wrote:
>
> > On Thu, Sep 23, 2021 at 2:18 PM Rajesh Patil <[email protected]> wrote:
> > >
> > > Add aliases for i2c and spi for sc7280 soc.
> > >
> > > Signed-off-by: Rajesh Patil <[email protected]>
> > > Reviewed-by: Stephen Boyd <[email protected]>
> >
> > I saw this in the pull request, can this please be reverted?
> >
>
> Yes, this can certainly be corrected.
>
> > Putting the aliases into the .dtsi file is really silly, as there are
> > likely boards that
> > don't connect every single one of those, and then will have to
> > override and renumber
> > them.
> >
> > Please only list the aliases that are actually connected on a particular
> > board.
Hrm. I know this gets into slightly controversial topics, but I'm a
little curious what the downside of having these in the dtsi is. In
the case where these i2c/spi/mmc devices _don't_ have "well defined"
numbers in the hardware manual of the SoC then I can agree that it
doesn't make sense to list these in the dtsi file. However, in the
case of sc7280 these numbers are well defined at the SoC level for i2c
and SPI.
Said another way: if you have a board that's got peripherals connected
on the pins labelled "i2c2" and "i2c6" on the SoC then it's a really
nice thing if these show up on /dev/i2c-2 and /dev/i2c-6.
...so I'm not sure what board exactly would be overriding and
re-numbering? Unless a board really has a strong use case where they
need the device connected to the pins for "i2c2" to show up on
"/dev/i2c-0"?
-Doug
On Tue, Oct 19, 2021 at 11:11 PM Doug Anderson <[email protected]> wrote:
> On Tue, Oct 19, 2021 at 1:57 PM Bjorn Andersson
>
> Hrm. I know this gets into slightly controversial topics, but I'm a
> little curious what the downside of having these in the dtsi is. In
> the case where these i2c/spi/mmc devices _don't_ have "well defined"
> numbers in the hardware manual of the SoC then I can agree that it
> doesn't make sense to list these in the dtsi file. However, in the
> case of sc7280 these numbers are well defined at the SoC level for i2c
> and SPI.
>
> Said another way: if you have a board that's got peripherals connected
> on the pins labelled "i2c2" and "i2c6" on the SoC then it's a really
> nice thing if these show up on /dev/i2c-2 and /dev/i2c-6.
>
> ...so I'm not sure what board exactly would be overriding and
> re-numbering? Unless a board really has a strong use case where they
> need the device connected to the pins for "i2c2" to show up on
> "/dev/i2c-0"?
There are multiple things going on here:
- The aliases are traditionally managed by the bootloader, same way
as the /chosen nodes including the kernel command line, so the
numbers are local policy, and the per-board defaults are just
for convenience.
- IMHO there should not be any aliases for status="disabled"
nodes, and the status is usually set in the board files.
- The labels on the board don't always match what the SoC calls
them, or there might not be any labels at all. This is more
important for things like serial ports that are often bare
connectors rather than already wired up. The aliases should
normally match how the board numbers the connectors, not
how they are attached internally.
- For i2c, it's not uncommon to have i2c devices attached behind
expanders on i2c/spi/gpio/usb/pci devices
Arnd
Hi,
On Tue, Oct 19, 2021 at 2:27 PM Arnd Bergmann <[email protected]> wrote:
>
> On Tue, Oct 19, 2021 at 11:11 PM Doug Anderson <[email protected]> wrote:
> > On Tue, Oct 19, 2021 at 1:57 PM Bjorn Andersson
> >
> > Hrm. I know this gets into slightly controversial topics, but I'm a
> > little curious what the downside of having these in the dtsi is. In
> > the case where these i2c/spi/mmc devices _don't_ have "well defined"
> > numbers in the hardware manual of the SoC then I can agree that it
> > doesn't make sense to list these in the dtsi file. However, in the
> > case of sc7280 these numbers are well defined at the SoC level for i2c
> > and SPI.
> >
> > Said another way: if you have a board that's got peripherals connected
> > on the pins labelled "i2c2" and "i2c6" on the SoC then it's a really
> > nice thing if these show up on /dev/i2c-2 and /dev/i2c-6.
> >
> > ...so I'm not sure what board exactly would be overriding and
> > re-numbering? Unless a board really has a strong use case where they
> > need the device connected to the pins for "i2c2" to show up on
> > "/dev/i2c-0"?
>
> There are multiple things going on here:
>
> - The aliases are traditionally managed by the bootloader, same way
> as the /chosen nodes including the kernel command line, so the
> numbers are local policy, and the per-board defaults are just
> for convenience.
The bootloader creates aliases? I've never seen this for I2C or SPI or
MMC, but I will admit I've been off in Chrome OS land for a really
long time and coreboot/depthcharge don't do this. I guess I could
believe that u-boot or some other bootloader does? Do you happen to
have a pointer to code that does this?
> - IMHO there should not be any aliases for status="disabled"
> nodes, and the status is usually set in the board files.
...but there is no harm in having an alias for status="disabled",
right? Below I have a use case where it's helpful to have aliases even
for status="disabled", and if it doesn't hurt...
> - The labels on the board don't always match what the SoC calls
> them, or there might not be any labels at all.
Are you saying that someone would draw up schematics and write on the
schematics "i2c0" and then connect it up to the pins on the SoC
labeled "i2c2"? I mean, I guess they could. I would really not like
working with the EE who did that, but people can do all sorts of crazy
things.
...or maybe you're saying that someone would take these I2C and SPI
pins and expose them to the end user with a little label over them
that said "i2c-0", "i2c-1", and "i2c-2"? ...and that end user would be
confused because the "/dev/i2c" and "/dev/spi" numbers wouldn't match?
I guess I could see that being a problem, though it feels unlikely to
come up in many cases except maybe in dev boards? This is also a new
SoC not designed onto any existing boards, so I'm not convinced that
someone would actually go and do this...
> This is more
> important for things like serial ports that are often bare
> connectors rather than already wired up. The aliases should
> normally match how the board numbers the connectors, not
> how they are attached internally.
So for UARTs I agree with you and that's one reason why this patch
doesn't include serial aliases. There seems to be a lot of history
around UART and requirements built into userspace / other places that
require UARTs be numbered starting at 0. Also UARTs _are_ historically
exposed to end users and they want sane numbers. Luckily this doesn't
cause _too_ much confusion since usually there is only one or two
UARTs in use and mostly they just hook up to console and bluetooth.
I think of UARTs as really the exception here, not the norm.
> - For i2c, it's not uncommon to have i2c devices attached behind
> expanders on i2c/spi/gpio/usb/pci devices
If there are extra i2c devices, that's OK. The i2c subsystem handles
will pick a number that's above the highest defined alias.
...and, in my mind, that actually gives a really good reason for
including all the aliases, even for status="disabled" nodes. Here's an
example output of `i2cdetect -l` on a sc7180-based device which has
aliases for all i2c adapters:
# i2cdetect -l
i2c-13 i2c dpu_dp_aux I2C adapter
i2c-4 i2c Geni-I2C I2C adapter
i2c-2 i2c Geni-I2C I2C adapter
i2c-9 i2c Geni-I2C I2C adapter
i2c-7 i2c Geni-I2C I2C adapter
i2c-14 i2c ti-sn65dsi86-aux I2C adapter
i2c-12 i2c cros-ec-i2c-tunnel I2C adapter
You can see that we've got peripherals hooked up to i2c ports 2, 4, 7, and 9.
On the sc7180 SoC, there are 12 i2c ports built-in to the SoC with
well-defined numbers. These are i2c-0 through i2c-11. On this system,
there are 3 additional extra i2c adapters. You can see that the i2c
subsystem starts numbering the extra adapters at 12. This is
specifically because i2c-10 and i2c-11 aliases were defined. This is
_good_ IMO.
If I saw a log message about "i2c-10" in the logs or in /dev/, I would
first look in the sc7180 devicetree file and assume that the
peripherals must be connected to the i2c10 pins on the SoC. If I see
"i2c-12" in the logs I would quickly realize that it couldn't be one
of the SoC i2c ports and I'd go look at the dynamically numbered ones.
Yes, yes. I'm smart enough to look things up and deal with any random
/ arbitrary numbers. You could also come up with an arbitrary Chinese
character for each i2c bus and I'm smart enough to look it up and map
it to find the right port. ...but there is no reason to make people go
through this work. This is the primary SoC on the system and it has
well-defined numbers. It just makes everyone's lives a little easier
if the numbers match the reference manual.
-Doug