Re: [PATCH] dt-bindings: Add bindings for aliases node

Re: [PATCH] dt-bindings: Add bindings for aliases node

[Brian Norris]

+ linux-spi, linux-wireless, netdev
+ others from previous conversations

Hi,

On Tue, Sep 25, 2018 at 02:02:55PM -0700, Matthias Kaehlcke wrote:
> Add a global binding for the 'aliases' node. This includes an initial list
> of standardized alias names for some hardware components that are commonly
> found in 'aliases'.
> 
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
>  Documentation/devicetree/bindings/aliases.txt | 47 +++++++++++++++++++
>  1 file changed, 47 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/aliases.txt
> 
> diff --git a/Documentation/devicetree/bindings/aliases.txt b/Documentation/devicetree/bindings/aliases.txt
> new file mode 100644
> index 000000000000..d64ed1c7eb34
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/aliases.txt
> @@ -0,0 +1,47 @@
> +The aliases node
> +----------------

I like the idea in general, and it might be good to note (e.g., commit
message) that this was inspired by this thread:

https://lore.kernel.org/lkml/20180815221601.GB24830@rob-hp-laptop/

where we were interested in firmware-to-device-tree path stability --
and the answer was basically: don't memorize paths, just use aliases
instead. But then, it was clear that aliases were not documented very
formally at all.

So here we are!

> +
> +The aliases node contains properties that represent aliases to device tree
> +nodes. The name of the property is the alias name, the value is the path of
> +a the device tree node that corresponds to the alias. The path may be
> +specified as a string or a phandle.
> +
> +Alias names are often suffixed with a numeric ID, especially when there may
> +be multiple instances of the same type. The ID typically corresponds to the
> +hardware layout, it may also be used by drivers for a stable mapping of
> +device names and hardware entities.
> +
> +Alias names
> +-----------
> +
> +The devicetree specification doesn't require the use of specific alias
> +names to refer to hardware entities of a given type, however the Linux
> +kernel aims for a certain level of consistency.
> +
> +The following standardized alias names shall be used for their
> +corresponding hardware components:
> +
> +  bluetoothN		Bluetooth controller
> +  ethernetN		Ethernet interface
> +  gpioN			GPIO controller
> +  i2cN			i2c bus
> +  mmcN			MMC bus
> +  rtcN			Real time clock
> +  serialN		UART port
> +  spiN			SPI bus
> +  wifiN			Wireless network interface

For the network-device-related names (bluetooth, ethernet, and wifi), I
think there's a clear documented reason for this (supporting MAC address
plumbing from a DT-aware bootloader). I'm not quite as sure about all
the others, and unfortunately, I'm aware of at least one subsystem owner
that explicitly does NOT like the aliases usage that is currently
supported (spi), and shot down a patch where I tried to use it in a DTS
file (despite its regular usage in many other DTS files).

So I guess I'm saying: perhaps we should get buy-in from various
subsystems before we include them? So maybe it's wiser to start
small(er) and only add once we're sure they are useful? Or perhaps Rob
has other thoughts.

> +
> +The above list is not exhaustive and will be extended over time. Please
> +send patches to devicetree@vger.kernel.org if you think a hardware
> +component and its alias name should be on the list.
> +
> +Example
> +-------
> +
> +aliases {
> +	bluetooth0 = "/soc/serial@fdf01000/bluetooth";
> +	rtc0 = &rtc0;
> +	wifi0 = &wlcore;
> +};
> +
> +(based on arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts)

What is the relevance of this line? This doesn't look anything like that
hikey DTS. Maybe the "based on" line should just be removed? The example
seems fine though.

Anyway, perhaps with a trimmed list of supported alias names:

Reviewed-by: Brian Norris <briannorris@chromium.org>

Re: [PATCH] dt-bindings: Add bindings for aliases node

[Geert Uytterhoeven]

Hi Brian,

On Tue, Oct 9, 2018 at 3:07 AM Brian Norris <briannorris@chromium.org> wrote:
> On Tue, Sep 25, 2018 at 02:02:55PM -0700, Matthias Kaehlcke wrote:
> > Add a global binding for the 'aliases' node. This includes an initial list
> > of standardized alias names for some hardware components that are commonly
> > found in 'aliases'.
> >
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>

> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/aliases.txt
> > @@ -0,0 +1,47 @@
> > +The aliases node
> > +----------------
>
> I like the idea in general, and it might be good to note (e.g., commit
> message) that this was inspired by this thread:
>
> https://lore.kernel.org/lkml/20180815221601.GB24830@rob-hp-laptop/
>
> where we were interested in firmware-to-device-tree path stability --
> and the answer was basically: don't memorize paths, just use aliases
> instead. But then, it was clear that aliases were not documented very
> formally at all.
>
> So here we are!
>
> > +
> > +The aliases node contains properties that represent aliases to device tree
> > +nodes. The name of the property is the alias name, the value is the path of
> > +a the device tree node that corresponds to the alias. The path may be
> > +specified as a string or a phandle.
> > +
> > +Alias names are often suffixed with a numeric ID, especially when there may
> > +be multiple instances of the same type. The ID typically corresponds to the
> > +hardware layout, it may also be used by drivers for a stable mapping of
> > +device names and hardware entities.
> > +
> > +Alias names
> > +-----------
> > +
> > +The devicetree specification doesn't require the use of specific alias
> > +names to refer to hardware entities of a given type, however the Linux
> > +kernel aims for a certain level of consistency.
> > +
> > +The following standardized alias names shall be used for their

s/shall/may/

> > +corresponding hardware components:
> > +
> > +  bluetoothN         Bluetooth controller
> > +  ethernetN          Ethernet interface
> > +  gpioN                      GPIO controller
> > +  i2cN                       i2c bus
> > +  mmcN                       MMC bus
> > +  rtcN                       Real time clock
> > +  serialN            UART port
> > +  spiN                       SPI bus
> > +  wifiN                      Wireless network interface
>
> For the network-device-related names (bluetooth, ethernet, and wifi), I
> think there's a clear documented reason for this (supporting MAC address
> plumbing from a DT-aware bootloader). I'm not quite as sure about all
> the others, and unfortunately, I'm aware of at least one subsystem owner
> that explicitly does NOT like the aliases usage that is currently
> supported (spi), and shot down a patch where I tried to use it in a DTS
> file (despite its regular usage in many other DTS files).
>
> So I guess I'm saying: perhaps we should get buy-in from various
> subsystems before we include them? So maybe it's wiser to start
> small(er) and only add once we're sure they are useful? Or perhaps Rob
> has other thoughts.

Please note these aliases become cumbersome once you start considering
(dynamic) DT overlays.  That's why I made them optional in the sh-sci
serial driver, cfr. commit 7678f4c20fa7670f ("serial: sh-sci: Add support
for dynamic instances").
Relevant parts of the commit description are:

    On DT platforms, the sh-sci driver requires the presence of "serialN"
    aliases in DT, from which instance IDs are derived.  If a DT alias is
    missing, the drivers fails to probe the corresponding serial port.

    This becomes cumbersome when considering DT overlays, as currently
    there is no upstream support for dynamically updating the /aliases node
    in DT.  Furthermore, even in the presence of such support, hardcoded
    instance IDs in independent overlays are prone to conflicts.

    Hence add support for dynamic instance IDs, to be used in the absence of
    a DT alias.  This makes serial ports behave similar to I2C and SPI
    buses, which already support dynamic instances.

To clarify my point: R-Car M2-W has 4 different types of serial ports, for a
total of 18 ports, and the two ports on a board labeled 0 and 1 may not
correspond to the physical first two ports (what's "first" in a collection of
4 different types?).

Aliases may be fine for referring to the main serial console (labeled
port 0 on the device, too), and the primary Ethernet interface (so U-Boot
knows where to add the "local-mac-address" property), but beyond that,
I think they should be avoided.

Just my two^H^H^Hfive €c.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] dt-bindings: Add bindings for aliases node

[Brian Norris]

On Tue, Oct 09, 2018 at 09:22:07AM +0200, Geert Uytterhoeven wrote:
> Please note these aliases become cumbersome once you start considering
> (dynamic) DT overlays.  That's why I made them optional in the sh-sci
> serial driver, cfr. commit 7678f4c20fa7670f ("serial: sh-sci: Add support
> for dynamic instances").

Note that as I understand it, the entire point of documenting this sort
of thing is to help solidify the interface between a DT aware boot
program (e.g., bootloader) and a device tree which is provided
separately, to avoid memorizing node/path hierarchy. It doesn't need to
(and doesn't, as I read it) enforce an OS's device naming policy.

> Relevant parts of the commit description are:
> 
>     On DT platforms, the sh-sci driver requires the presence of "serialN"
>     aliases in DT, from which instance IDs are derived.  If a DT alias is
>     missing, the drivers fails to probe the corresponding serial port.
> 
>     This becomes cumbersome when considering DT overlays, as currently
>     there is no upstream support for dynamically updating the /aliases node
>     in DT.

That part is not a DT spec problem :)

>     Furthermore, even in the presence of such support, hardcoded
>     instance IDs in independent overlays are prone to conflicts.
> 
>     Hence add support for dynamic instance IDs, to be used in the absence of
>     a DT alias.  This makes serial ports behave similar to I2C and SPI
>     buses, which already support dynamic instances.

This seems to be a much different sort of problem. People always love
having predictable IDs given by the OS (myself included), but that's
just plain hard to do and impossible in some cases. I don't think that's
what this document is about though.

IOW, this document seems pretty consistent with the above: it doesn't
require the usage of aliases (and it seems silly to have a driver
*require* an alias) -- it just documents how one should name such an
alias if you expect multiple independent software components to
understand it.

> To clarify my point: R-Car M2-W has 4 different types of serial ports, for a
> total of 18 ports, and the two ports on a board labeled 0 and 1 may not
> correspond to the physical first two ports (what's "first" in a collection of
> 4 different types?).
> 
> Aliases may be fine for referring to the main serial console (labeled
> port 0 on the device, too), and the primary Ethernet interface (so U-Boot
> knows where to add the "local-mac-address" property), but beyond that,
> I think they should be avoided.

That's fair enough. Just because the solution isn't an all-purpose tool
doesn't mean it shouldn't be documented. The general concept is already
in ePAPR, but it's just not very specific about property names.

> Just my two^H^H^Hfive €c.

Thanks,
Brian

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH] dt-bindings: Add bindings for aliases node

[Matthias Kaehlcke]

On Tue, Oct 09, 2018 at 11:31:42AM -0700, Brian Norris wrote:
> On Tue, Oct 09, 2018 at 09:22:07AM +0200, Geert Uytterhoeven wrote:
> > Please note these aliases become cumbersome once you start considering
> > (dynamic) DT overlays.  That's why I made them optional in the sh-sci
> > serial driver, cfr. commit 7678f4c20fa7670f ("serial: sh-sci: Add support
> > for dynamic instances").
> 
> Note that as I understand it, the entire point of documenting this sort
> of thing is to help solidify the interface between a DT aware boot
> program (e.g., bootloader) and a device tree which is provided
> separately, to avoid memorizing node/path hierarchy. It doesn't need to
> (and doesn't, as I read it) enforce an OS's device naming policy.
> 
> > Relevant parts of the commit description are:
> > 
> >     On DT platforms, the sh-sci driver requires the presence of "serialN"
> >     aliases in DT, from which instance IDs are derived.  If a DT alias is
> >     missing, the drivers fails to probe the corresponding serial port.
> > 
> >     This becomes cumbersome when considering DT overlays, as currently
> >     there is no upstream support for dynamically updating the /aliases node
> >     in DT.
> 
> That part is not a DT spec problem :)
> 
> >     Furthermore, even in the presence of such support, hardcoded
> >     instance IDs in independent overlays are prone to conflicts.
> > 
> >     Hence add support for dynamic instance IDs, to be used in the absence of
> >     a DT alias.  This makes serial ports behave similar to I2C and SPI
> >     buses, which already support dynamic instances.
> 
> This seems to be a much different sort of problem. People always love
> having predictable IDs given by the OS (myself included), but that's
> just plain hard to do and impossible in some cases. I don't think that's
> what this document is about though.
> 
> IOW, this document seems pretty consistent with the above: it doesn't
> require the usage of aliases (and it seems silly to have a driver
> *require* an alias) -- it just documents how one should name such an
> alias if you expect multiple independent software components to
> understand it.
> 
> > To clarify my point: R-Car M2-W has 4 different types of serial ports, for a
> > total of 18 ports, and the two ports on a board labeled 0 and 1 may not
> > correspond to the physical first two ports (what's "first" in a collection of
> > 4 different types?).
> > 
> > Aliases may be fine for referring to the main serial console (labeled
> > port 0 on the device, too), and the primary Ethernet interface (so U-Boot
> > knows where to add the "local-mac-address" property), but beyond that,
> > I think they should be avoided.
> 
> That's fair enough. Just because the solution isn't an all-purpose tool
> doesn't mean it shouldn't be documented. The general concept is already
> in ePAPR, but it's just not very specific about property names.

Basically what Brian said, this doc doesn't encourage the use of
aliases, it just intends to establish a consistent naming for cases
where aliases are needed/more useful than harmful. The misuse of
aliases needs to be addressed in the reviews of the patches that
introduce them.

Maybe the doc should include a recommendation to use aliases
sparingly? I'm open to input on that from folks who have a better
understanding of the potential pitfalls ;-)

Cheers

Matthias

Re: [PATCH] dt-bindings: Add bindings for aliases node

[Christian Lamparter]

On Friday, October 12, 2018 2:08:37 AM CEST Matthias Kaehlcke wrote:
> Maybe the doc should include a recommendation to use aliases
> sparingly? I'm open to input on that from folks who have a better
> understanding of the potential pitfalls 

I had a similar discussion with the OpenWrt devs over the
use of "led-$function" aliases in a DTS. I did a bit of digging and
found this wonderful emails from Mark Rutland regarding the general
use and abuse of aliases in a reply to a patch by Christer Weinigel 
"devicetree - document using aliases to set spi bus number."

<https://patchwork.kernel.org/patch/9133903/#19207021>

|"If those ports are physically organised and labelled the same, then
|using aliases could make sense, to describe the well-defined physical
|labels. If you've assigned the numbers artificially, or if the physical
|organisation differs across boards, then aliases are not the right tool
|for the job.
|
|In the latter cases we're altering the hardware description to suit an
|application, rather than providing the necessary abstraction, which is
|the kind of (ab)use of aliases which we want to avoid."

And he followed it up with a summary:
<https://patchwork.kernel.org/comment/19207071/>

|Typically, serial ports are much more user-accessible (physically), and
|much more directly useful to a user in a generic fashion. They're often
|labelled (physically or in a manual) with a number, and we use aliases
|to describe those labels to the kernel. The fact that the kernel may use
|that to drive its own internal numbering is immaterial to the binding.

So the gist of this is that aliases are meant for user-accessible /
physically devices/ports/etc... that are labeled as such. And this of
course works perfectly for power/status LEDs and such because they
usually have little "power" symbols/pictograms/lables near them. 

Re: [PATCH] dt-bindings: Add bindings for aliases node

[Rob Herring]

On Tue, Oct 09, 2018 at 11:31:42AM -0700, Brian Norris wrote:
> On Tue, Oct 09, 2018 at 09:22:07AM +0200, Geert Uytterhoeven wrote:
> > Please note these aliases become cumbersome once you start considering
> > (dynamic) DT overlays.  That's why I made them optional in the sh-sci
> > serial driver, cfr. commit 7678f4c20fa7670f ("serial: sh-sci: Add support
> > for dynamic instances").
> 
> Note that as I understand it, the entire point of documenting this sort
> of thing is to help solidify the interface between a DT aware boot
> program (e.g., bootloader) and a device tree which is provided
> separately, to avoid memorizing node/path hierarchy. It doesn't need to
> (and doesn't, as I read it) enforce an OS's device naming policy.

I'm all for documenting this primarily to prevent folks from just adding 
whatever they wish in /aliases. Some platforms seem to want to have 
aliases for everything.

> > Relevant parts of the commit description are:
> > 
> >     On DT platforms, the sh-sci driver requires the presence of "serialN"
> >     aliases in DT, from which instance IDs are derived.  If a DT alias is
> >     missing, the drivers fails to probe the corresponding serial port.
> > 
> >     This becomes cumbersome when considering DT overlays, as currently
> >     there is no upstream support for dynamically updating the /aliases node
> >     in DT.
> 
> That part is not a DT spec problem :)
> 
> >     Furthermore, even in the presence of such support, hardcoded
> >     instance IDs in independent overlays are prone to conflicts.
> > 
> >     Hence add support for dynamic instance IDs, to be used in the absence of
> >     a DT alias.  This makes serial ports behave similar to I2C and SPI
> >     buses, which already support dynamic instances.
> 
> This seems to be a much different sort of problem. People always love
> having predictable IDs given by the OS (myself included), but that's
> just plain hard to do and impossible in some cases. I don't think that's
> what this document is about though.
> 
> IOW, this document seems pretty consistent with the above: it doesn't
> require the usage of aliases (and it seems silly to have a driver
> *require* an alias) -- it just documents how one should name such an
> alias if you expect multiple independent software components to
> understand it.
> 
> > To clarify my point: R-Car M2-W has 4 different types of serial ports, for a
> > total of 18 ports, and the two ports on a board labeled 0 and 1 may not
> > correspond to the physical first two ports (what's "first" in a collection of
> > 4 different types?).
> > 
> > Aliases may be fine for referring to the main serial console (labeled
> > port 0 on the device, too), and the primary Ethernet interface (so U-Boot
> > knows where to add the "local-mac-address" property), but beyond that,
> > I think they should be avoided.

This basically matches my opinion on aliases.
 
I'd decouple it from board labels a bit. Sometimes the numbering may 
match, but others not. What if a board serial port is labeled "DBG" for 
example? I think 'label' is the right way to handle human identifible 
ports (and then we should have something like /dev/serial/by-label/...).

> That's fair enough. Just because the solution isn't an all-purpose tool
> doesn't mean it shouldn't be documented. The general concept is already
> in ePAPR, but it's just not very specific about property names.

Agreed. I guess the question is what to do on used, but not recommended 
aliases. I would put SPI and I2C into that category BTW.

Rob