| Message ID | 1632399378-12229-9-git-send-email-rajpat@codeaurora.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Add QSPI and QUPv3 DT nodes for SC7280 SoC | expand |
On Thu, Sep 23, 2021 at 2:18 PM Rajesh Patil <rajpat@codeaurora.org> wrote: > > Add aliases for i2c and spi for sc7280 soc. > > Signed-off-by: Rajesh Patil <rajpat@codeaurora.org> > Reviewed-by: Stephen Boyd <swboyd@chromium.org> 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 <rajpat@codeaurora.org> wrote: > > > > Add aliases for i2c and spi for sc7280 soc. > > > > Signed-off-by: Rajesh Patil <rajpat@codeaurora.org> > > Reviewed-by: Stephen Boyd <swboyd@chromium.org> > > 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 <bjorn.andersson@linaro.org> wrote: > > On Tue 19 Oct 13:43 PDT 2021, Arnd Bergmann wrote: > > > On Thu, Sep 23, 2021 at 2:18 PM Rajesh Patil <rajpat@codeaurora.org> wrote: > > > > > > Add aliases for i2c and spi for sc7280 soc. > > > > > > Signed-off-by: Rajesh Patil <rajpat@codeaurora.org> > > > Reviewed-by: Stephen Boyd <swboyd@chromium.org> > > > > 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 <dianders@chromium.org> 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 <arnd@kernel.org> wrote: > > On Tue, Oct 19, 2021 at 11:11 PM Doug Anderson <dianders@chromium.org> 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
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 {