Re: [PATCH 18/18] arm64: apple: Add initial Mac Mini 2020 (M1) devicetree

On Mon, Feb 08, 2021 at 08:56:53PM +0900, Hector Martin 'marcan' wrote:

On 08/02/2021 20.04, Krzysztof Kozlowski wrote:

quoted

apple

Don't make things different for this one platform (comparing to all
other platforms). Apple is not that special. :)

AAPL is the old vendor prefix used in the PowerPC era. I'm happy to use
`apple`, as long as we're OK with having two different prefixes for the same
vendor, one for PPC and one for ARM64. I've seen opinions go both ways on
this one :)

quoted

quoted

+ * Copyright 2021 Hector Martin [off-list ref]

A lot here might be difficult to reverse-egineer or figure out by
ourself, so usually people rely on vendor sources (the open source
compliance package). Didn't you receive such for the iOS (or whatever
was on your Mac)?

Apple source drops are sparse (they don't even include things like the
irqchip driver, only the very core OS code) and APSL licensed, which is a
license incompatible with the GPL. Worse, they've moved to a partial-blob
model with the M1; M1-compatible XNU source code drops now include a .a blob
with startup and CPU-specific code, for which no source code is provided.
(to be clear: Apple does not ship Linux for these machines)

Honestly, beyond what's in this patchset and a few more details about CPU
registers like performance monitoring stuff that exist in public XNU drops
but I haven't looked into yet, Apple's source code drops are going to be
practically useless to us from here on out. It's all binaries after this.

Apple device trees are not open source at all; those are provided by iBoot
and ship with device firmware, which is not openly licensed. Those device
trees are OF-inspired, but otherwise in a different format and structure to
Linux device trees.

Since there is zero Apple-published code or data with a license compatible
with the Linux kernel to be used here, there can be zero copyright lines
claiming any submissions are copyright Apple from us, because that would
imply a license violation has occurred. I am treating this as I would any
other no-source reverse engineering project, that is, ensuring that I only
look at Apple code (binaries, source, devicetrees, whatever) to understand
how the hardware functions, build documentation for it (at least in my head,
but I am also trying to document things on our wiki as I go), and then write
original code to drive it from Linux, unrelated to whatever Apple was doing.

We're also trying to avoid looking at any Apple stuff in general as much as
possible, preferring black-box approaches where feasible, to minimize
exposure. For example, I only looked at an (outdated, arm32 era) AIC
register name list in XNU to write the AIC driver; there is no actual AIC
driver code in the source, and instead of decompiling Apple's binary blob
AIC driver module, I figured out how the hardware actually worked via
probing and experimentation. The entire userspace GPU stack is being reverse
engineered via a black-box approach, without any decompilation. I'm going to
see what I can do about the kernel driver in the future, and prefer some
kind of mmio tracing solution if I can get it all to work on macOS.

As for this file specifically: while I am obviously looking at Apple's DTs
to figure out things like register offsets and what hardware exists, those
are facts, and facts are not copyrightable, and thus Apple does not hold any
copyright interest over this code as I submitted it. Short of verbatim
copying and pasting of entire nodes with bespoke property names (which would
never fly here anyway because Apple does things very differently from Linux
DTs when you get down into the details), it would be extremely hard to argue
that translating hardware information from decompiled Apple DTs to Linux DTs
would constitute a copyright violation, since the entire purpose of DTs is
to describe hardware facts.

You can read more about our reverse engineering and copyright policy at
https://alx.sh/re - if you have any suggestions or spot anything
problematic, please let me know.

(I'm actually probably going to change that copyright line to "The Asahi
Linux Contributors" for v2, if that's okay with the kernel folks, to be in
line with our other projects; I defaulted to my name since so far I'm the
only contributor to these files, but I expect other people to throw PRs at
me in the future and the history to end up with more names here)

Does there need to be a legal entity behind 'The Asahi Linux 
Contributors' to be valid?

From a more practical standpoint, if we want to relicense something in 
say 5 years from now, who do we ask for an okay?

quoted

I guess Rob will comment on the dt-bindings more... but for me a generic
"arm-platform" is too generic. What's the point of it? I didn't see any
of such generic compatibles in other platforms.

This is a hack for patches #11/#12 to use, and I expect it will go away once
we figure out how to properly handle that problem (which needs further
discussion). Sorry for the noise, this should not be there in the final
version.

I was going to ask on this. If you have a user of it, I'm okay with it. 
Generally though, 3 or 4 levels of compatible don't really have users.

quoted

quoted

+		bootargs = "earlycon";

This should not be hard-coded in DTS. Pass it from bootloader.

My apologies, this was garbage left over from before I had bootargs support
in the bootloader. Will be gone for v2.

quoted

quoted

+	clk24: clk24 {

Just "clock". Node names should be generic.

Really? Almost every other device device tree uses unique clock node names.

It's a WIP to be more consistent around node names. For actual
clock controllers we have 'clock-controller(@.*)?'. There's not really 
something established for 'fixed-clock'. We probably should define 
something, but that goes in the schema first.

quoted

quoted

+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <24000000>;
+		clock-output-names = "clk24";

What clock is it? Part of board or SoC? Isn't it a work-around for
missing clock drivers?

The clock topology isn't entirely known yet; I'm submitting this as an
initial bring-up patchset and indeed there should be a clockchip driver in
the future. The UART driver wants a clock to be able to calculate baud
rates. I figured we can get away with a fixed-clock for now while that part
of the SoC gets figured out.

That is normal. It does break compatibility between an old kernel 
and new DT. There's not really a good way to avoid that.

Rob