Re: [RFT PATCH v3 27/27] arm64: apple: Add initial Apple Mac mini (M1, 2020) devicetree

[Arnd Bergmann <arnd@kernel.org>]

On Mon, Apr 5, 2021 at 7:50 AM Hector Martin <marcan@marcan.st> wrote:
> On 14/03/2021 05.22, Konrad Dybcio wrote:
>
> [1]
> https://lore.kernel.org/linux-arm-kernel/20210216073120.qmlaky43t6uelqc4@kozik-lap/
>
> >> +/*
> >> + * Apple Mac mini (M1, 2020)
> >
> > Not sure if it's necessary, you already state it in model=""
>
> I find it helpful to have identifying info in the top comment, since
> it's easier to locate at a glance than visually searching for the model
> property. Since I also add identifiers that Apple uses to be able to
> cross reference things here, even if it's somewhat duplicative, I think
> it makes sense to keep all of it in the top comment.

Agreed

> >> +
> >> +/dts-v1/;
> >> +
> >> +#include "t8103.dtsi"
> >> +
> >> +/ {
> >> +    compatible = "apple,j274", "apple,t8103", "apple,arm-platform";
> >> +    model = "Apple Mac mini (M1, 2020)";
> >> +
> >> +    aliases {
> >> +            serial0 = &serial0;
> >
> > Isn't this M1-common?
>
> It's common to all existing M1 devices, but in principle there is
> nothing that says that the stdout path has to be the first UART on any
> given device (there is more than one UART, we just aren't declaring the
> others yet in this series). This is a common pattern, see e.g.
> nvidia/tegra210-smaug.dts.

Also, I normally ask for the aliases, chosen and memory nodes to be part of
the .dts file since these are settings that may be set by the bootloader and
are not specific to the SoCs at all.

> >> +    compatible = "apple,t8103", "apple,arm-platform";
> >
> > Please remove this line, it's getting overwritten anyway.
>
> I think it's helpful to document the expected compatible subset to be
> inherited by board files. The Tegra example I linked above does this too.

I'm fine with it either way here, your reasoning makes sense, but I
personally don't ask for these to be added or removed.

> > Other than this, node order seems to be entirely random. Please sort them
> > by address (where applicable) and by name where not, so that this doesn't
> > become a huge mess as we go forward.
>
> Outside of the soc bus I think the order we have makes sense: CPUs
> first, timer (which is part of the CPU complex) next, then fixed clocks.
> There are only a few things that are going to go in here, so I think an
> ad-hoc order like this is okay.
>
> Inside the bus we only have two nodes so far, and they are indeed in the
> wrong order :-). I'll sort them by address. Almost everything else is
> going to go in here in the future, so that should keep things sane.

Sounds good. Ordering by address is a good default, in particular for the
SoC node, but other orders can make sense, too. We used to have some
SoCs that sorted everything alphabetically, but that is less common now.

The main reason for having a fixed sort order is to help with merge conflicts
if we ever get nodes added through different git trees. If both sides add the
same node in the same place, that lets git cleanly resolve the merge, while
adding the same node in different places duplicates it, and adding different
versions in the same place should cause a merge conflict.

        Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel