On Mon, Apr 5, 2021 at 7:50 AM Hector Martin [off-list ref] wrote:

On 14/03/2021 05.22, Konrad Dybcio wrote:

[1]
https://lore.kernel.org/linux-arm-kernel/20210216073120.qmlaky43t6uelqc4@kozik-lap/ (local)

quoted

quoted

+/*
+ * 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

quoted

quoted

+
+/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.

quoted

quoted

+    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.

quoted

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