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

[Hector Martin <marcan@marcan.st>]

Hi,

Sorry, I did not see this e-mail originally as it was sent only to the 
mailing list, not to me nor CCed to anyone else.

I am subscribed to the lists, but I generally rely on my inbox for 
review feedback. I have added a sieve rule to catch emails sent to the 
list that are replies to me and do not have me in To: or Cc: and copy 
them to my inbox in the future.

That said, I think it's best to reply-all, to keep everyone else in the 
loop. CCing a few relevant folks.

On 14/03/2021 05.22, Konrad Dybcio wrote:
>> +// SPDX-License-Identifier: GPL-2.0+ OR MIT
> 
> It seems to be
> 
> "// SPDX-License-Identifier: (GPL-2.0+ OR MIT)"
> 
> in other DTs (notice the brackets).

This was already discussed in [1].

The conclusion was that no braces is the preferred form according to the 
SPDX spec.

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

>> + *
>> + * target-type: J174
> 
> Isn't that a typo? Compatible states J*2*74, but I'm curious if that's intentional.

That's a typo, it's already fixed in v4 :)

>> + *
>> + * Copyright The Asahi Linux Contributors
> 
> * Copyright (C) 2021 The Asahi Linux Contributors

This was also discussed, see [2] and [3]. Copyright dates in headers are 
essentially useless because they are always out of date in practice.

[2] 
https://www.linuxfoundation.org/en/blog/copyright-notices-in-open-source-software-projects/
[3] 
https://lore.kernel.org/linux-arm-kernel/d8454963-3242-699b-4c20-e85d26b19796@marcan.st/

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

>> +	};
>> +
>> +	chosen {
>> +		#address-cells = <2>;
>> +		#size-cells = <2>;
>> +		ranges;
>> +
>> +		stdout-path = "serial0";
> 
> Doesn't serial work without this?

This is for earlycon to work. See e.g. nvidia/tegra210-smaug.dts for 
another example. Note that this still requires the earlycon kernel 
parameter, so if earlycon is not enabled then this does nothing; if the 
user also isn't setting the serial TTY device as a console, then the 
serial port is free for use by the user.

> Not sure if we want the @0 (unless it's also overwritten by the loader).

We need a unit address for the linter, but yes, this is in fact 
overwritten by the loader with the correct one.

>> +			compatible = "apple,simple-framebuffer", "simple-framebuffer";
>> +			reg = <0 0 0 0>; /* To be filled by loader */
>> +			/* Format properties will be added by loader */
>> +			status = "disabled";
> 
> Does it get enabled by the loader?

Yup, assuming everything works. The loader leaves it disabled if 
something goes wrong converting the information from the XNU bootargs 
(e.g. unsupported CLUT format, which I don't think any M1 mac ever uses).

> Also, isn't the framebuffer common for all M1 devices?

The framebuffer is allocated by iBoot dynamically, and is at a variable 
address. The format is also variable, as is the resolution, depending on 
the device. This is why the loader fills in the info.

In principle, the idea that a framebuffer exists is common to all 
existing devices, but nothing says this has to be the case for future 
devices: it's not a SoC property, it's a property of devices built using 
the SoC (and the way they are configured by iBoot).

>> +		};
>> +	};
>> +
>> +	memory@800000000 {
>> +		device_type = "memory";
>> +		reg = <0x8 0 0x2 0>; /* To be filled by loader */
>> +	};
> 
> Shouldn't this be in the SoC DTSI?

It could. The pattern I'm trying to follow here is that stuff filled in 
by the loader or which varies from device to device goes in the board 
file; technically here it doesn't matter since of course every SoC has 
"some" memory and the loader overwrites it anyway, but this still makes 
logical sense to me. If you look around, you'll see memory is usually 
defined in board or sub-platform files, not the SoC root (exception: 
qcom stuff, apparently).

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

>> +	};
>> +
>> +	soc {
>> +		compatible = "simple-bus";
>> +		#address-cells = <2>;
>> +		#size-cells = <2>;
>> +
>> +		ranges;
>> +		nonposted-mmio;
>> +
> 
> I think
> 
> "
> 
> compatible = "simple-bus";
> 
> #address-cells = <2>;
> 
> #size-cells = <2>;
> 
> nonposted-mmio;
> 
> ranges;
> 
> "
> 
> would look more coherent to the human eye, but that's rather a nit.

The reason for putting nonposted-mmio last is that it is a flag for a 
bus, so to me it makes logical sense to put it after ranges, which is 
what marks this as a translatable bus.

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

Thanks!
-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub

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