diff mbox series

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

Message ID 20210204203951.52105-19-marcan@marcan.st (mailing list archive)
State Changes Requested
Headers show
Series Apple M1 SoC platform bring-up | expand

Commit Message

Hector Martin Feb. 4, 2021, 8:39 p.m. UTC
This currently supports:

* SMP (via spin-tables)
* AIC IRQs
* Serial (with earlycon)
* Framebuffer

A number of properties are dynamic, and based on system firmware
decisions that vary from version to version. These are expected
to be filled in by the loader.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 MAINTAINERS                              |   1 +
 arch/arm64/boot/dts/Makefile             |   1 +
 arch/arm64/boot/dts/apple/Makefile       |   2 +
 arch/arm64/boot/dts/apple/apple-j274.dts | 143 +++++++++++++++++++++++
 4 files changed, 147 insertions(+)
 create mode 100644 arch/arm64/boot/dts/apple/Makefile
 create mode 100644 arch/arm64/boot/dts/apple/apple-j274.dts

Comments

Arnd Bergmann Feb. 4, 2021, 9:29 p.m. UTC | #1
On Thu, Feb 4, 2021 at 9:39 PM Hector Martin <marcan@marcan.st> wrote:

> +/ {
> +       model = "Apple Mac Mini M1 2020";
> +       compatible = "AAPL,j274", "AAPL,m1", "AAPL,arm-platform";
> +       #address-cells = <2>;
> +       #size-cells = <2>;
> +
> +       chosen {
> +               #address-cells = <2>;
> +               #size-cells = <2>;
> +               ranges;
> +
> +               bootargs = "earlycon";
> +               stdout-path = "serial0:1500000";
> +
> +               framebuffer0: framebuffer@0 {
> +                       compatible = "AAPL,simple-framebuffer", "simple-framebuffer";
> +                       reg = <0 0 0 0>; // To be filled by loader
> +                       // Format properties will be added by loader
> +                       status = "disabled";
> +               };
> +       };
> +
> +       memory@800000000 {
> +               device_type = "memory";
> +               reg = <0 0 0 0>; // To be filled by loader
> +       };
> +
> +       aliases {
> +               serial0 = &serial0;
> +       };

We tend to split the dts file into one file per SoC and one for the
specific board. I guess in this case the split can be slightly different,
but it does feel better to be prepared for sharing a lot of the contents
between the different products.

In most cases, you'd want the 'aliases' and 'chosen' nodes to be
in the board specific file.

> +       cpus {
> +               #address-cells = <2>;
> +               #size-cells = <0>;
> +
> +               cpu0: cpu@0 {
> +                       compatible = "AAPL,icestorm";
> +                       device_type = "cpu";
> +                       reg = <0x0 0x0>;
> +                       enable-method = "spin-table";
> +                       cpu-release-addr = <0 0>; // To be filled by loader
> +               };

Did you see the discussion on the #armlinux channel about the possibility
of moving the cpu-enable method to PSCI based on a UEFI runtime
interface?

There are a few open questions about what that would look like in the
end, but Ard has come up with a prototype for the kernel side of it
(obviously untested), which would interface either into the UEFI side
of u-boot, or a simple already-instantiated version that could be
kept inside of m1n1 and stay resident in memory.

I would like to see that model get adopted here eventually. If
we manage to get the other patches ready for an initial merge in
v5.12, we can probably start out with spin-table and move to that
in a following release though.

        Arnd
Hector Martin Feb. 4, 2021, 9:44 p.m. UTC | #2
On 05/02/2021 06.29, Arnd Bergmann wrote:
> On Thu, Feb 4, 2021 at 9:39 PM Hector Martin <marcan@marcan.st> wrote:
> 
>> +/ {
>> +       model = "Apple Mac Mini M1 2020";
>> +       compatible = "AAPL,j274", "AAPL,m1", "AAPL,arm-platform";
>> +       #address-cells = <2>;
>> +       #size-cells = <2>;
>> +
>> +       chosen {
>> +               #address-cells = <2>;
>> +               #size-cells = <2>;
>> +               ranges;
>> +
>> +               bootargs = "earlycon";
>> +               stdout-path = "serial0:1500000";
>> +
>> +               framebuffer0: framebuffer@0 {
>> +                       compatible = "AAPL,simple-framebuffer", "simple-framebuffer";
>> +                       reg = <0 0 0 0>; // To be filled by loader
>> +                       // Format properties will be added by loader
>> +                       status = "disabled";
>> +               };
>> +       };
>> +
>> +       memory@800000000 {
>> +               device_type = "memory";
>> +               reg = <0 0 0 0>; // To be filled by loader
>> +       };
>> +
>> +       aliases {
>> +               serial0 = &serial0;
>> +       };
> 
> We tend to split the dts file into one file per SoC and one for the
> specific board. I guess in this case the split can be slightly different,
> but it does feel better to be prepared for sharing a lot of the contents
> between the different products.
> 
> In most cases, you'd want the 'aliases' and 'chosen' nodes to be
> in the board specific file.

I thought about that, but wasn't sure if splitting it up at this early 
stage made much sense since I'm not sure what the split should be, given 
all supported hardware is the same for all 3 released devices.

I'm happy to throw the aliases/chosen nodes into board specific files if 
you think that's a good starting point. Perhaps /memory too? Those 
properties are filled in/patched by the bootloader anyway...

There are also DT overlays; I was wondering if we could use those to 
keep the hierarchy and avoid having many duplicate trees in a 
hypothetical bootloader that embeds support for a large set of hardware, 
having it construct the final devicetree on the fly from SoC + a board 
overlay (and possibly further levels); but I'm not sure how that ties in 
with the device trees that live in the Linux tree. Do you have any 
pointers about this?

For reference, this is our current DT patching code in m1n1:

https://github.com/AsahiLinux/m1n1/blob/main/src/kboot.c

Eventually we're going to build some kind of tooling to automate diffing 
Apple device trees and importing changes/new devices into our own, 
though it will probably be quite a while until that is relevant; at this 
stage hand-maintaining them is perfectly fine (in any case this wouldn't 
be fully automated, so in the end our trees will still be organized 
however we want).

>> +       cpus {
>> +               #address-cells = <2>;
>> +               #size-cells = <0>;
>> +
>> +               cpu0: cpu@0 {
>> +                       compatible = "AAPL,icestorm";
>> +                       device_type = "cpu";
>> +                       reg = <0x0 0x0>;
>> +                       enable-method = "spin-table";
>> +                       cpu-release-addr = <0 0>; // To be filled by loader
>> +               };
> 
> Did you see the discussion on the #armlinux channel about the possibility
> of moving the cpu-enable method to PSCI based on a UEFI runtime
> interface?
> 
> There are a few open questions about what that would look like in the
> end, but Ard has come up with a prototype for the kernel side of it
> (obviously untested), which would interface either into the UEFI side
> of u-boot, or a simple already-instantiated version that could be
> kept inside of m1n1 and stay resident in memory.
> 
> I would like to see that model get adopted here eventually. If
> we manage to get the other patches ready for an initial merge in
> v5.12, we can probably start out with spin-table and move to that
> in a following release though.

I saw it go by but need to review it again; I've been missing too much 
sleep this week :) thanks for the reminder.

I think we might want to start with spin-table for now, given that there 
are no kernel changes needed anyway, but I'm happy to take the protoype 
for a spin (:)) and try implementing it in m1n1.

I do think it's valuable for whatever we do, at this stage, to not 
require u-boot; having that be an integral part of the boot chain is 
perfectly fine in the future but right now it helps to have a simple 
boot chain while we work out the early bring-up, and while u-boot grows 
the required support.
Arnd Bergmann Feb. 4, 2021, 11:08 p.m. UTC | #3
On Thu, Feb 4, 2021 at 10:44 PM Hector Martin 'marcan' <marcan@marcan.st> wrote:
> On 05/02/2021 06.29, Arnd Bergmann wrote:
> > On Thu, Feb 4, 2021 at 9:39 PM Hector Martin <marcan@marcan.st> wrote:
> >
> > We tend to split the dts file into one file per SoC and one for the
> > specific board. I guess in this case the split can be slightly different,
> > but it does feel better to be prepared for sharing a lot of the contents
> > between the different products.
> >
> > In most cases, you'd want the 'aliases' and 'chosen' nodes to be
> > in the board specific file.
>
> I thought about that, but wasn't sure if splitting it up at this early
> stage made much sense since I'm not sure what the split should be, given
> all supported hardware is the same for all 3 released devices.
>
> I'm happy to throw the aliases/chosen nodes into board specific files if
> you think that's a good starting point. Perhaps /memory too? Those
> properties are filled in/patched by the bootloader anyway...

Yes, I think that would help make it more consistent with other
platforms even if we don't care too much here.

> There are also DT overlays; I was wondering if we could use those to
> keep the hierarchy and avoid having many duplicate trees in a
> hypothetical bootloader that embeds support for a large set of hardware,
> having it construct the final devicetree on the fly from SoC + a board
> overlay (and possibly further levels); but I'm not sure how that ties in
> with the device trees that live in the Linux tree. Do you have any
> pointers about this?

We don't really have overlays in the kernel sources (yet), though it
is something that keeps coming up. For the moment, I'd just
assume you can have one .dts file for each thing you want to
support and keep the shared bits in .dtsi files.

> >
> > Did you see the discussion on the #armlinux channel about the possibility
> > of moving the cpu-enable method to PSCI based on a UEFI runtime
> > interface?
...
>
> I saw it go by but need to review it again; I've been missing too much
> sleep this week :) thanks for the reminder.
>
> I think we might want to start with spin-table for now, given that there
> are no kernel changes needed anyway, but I'm happy to take the protoype
> for a spin (:)) and try implementing it in m1n1.
>
> I do think it's valuable for whatever we do, at this stage, to not
> require u-boot; having that be an integral part of the boot chain is
> perfectly fine in the future but right now it helps to have a simple
> boot chain while we work out the early bring-up, and while u-boot grows
> the required support.

Agreed.

       Arnd
Hector Martin Feb. 5, 2021, 7:11 a.m. UTC | #4
On 05/02/2021 08.08, Arnd Bergmann wrote:
> On Thu, Feb 4, 2021 at 10:44 PM Hector Martin 'marcan' <marcan@marcan.st> wrote:
>> On 05/02/2021 06.29, Arnd Bergmann wrote:
>>> On Thu, Feb 4, 2021 at 9:39 PM Hector Martin <marcan@marcan.st> wrote:
>>>
>>> We tend to split the dts file into one file per SoC and one for the
>>> specific board. I guess in this case the split can be slightly different,
>>> but it does feel better to be prepared for sharing a lot of the contents
>>> between the different products.
>>>
>>> In most cases, you'd want the 'aliases' and 'chosen' nodes to be
>>> in the board specific file.
>>
>> I thought about that, but wasn't sure if splitting it up at this early
>> stage made much sense since I'm not sure what the split should be, given
>> all supported hardware is the same for all 3 released devices.
>>
>> I'm happy to throw the aliases/chosen nodes into board specific files if
>> you think that's a good starting point. Perhaps /memory too? Those
>> properties are filled in/patched by the bootloader anyway...
> 
> Yes, I think that would help make it more consistent with other
> platforms even if we don't care too much here.

Ack, I'll split it up for v2.

> We don't really have overlays in the kernel sources (yet), though it
> is something that keeps coming up. For the moment, I'd just
> assume you can have one .dts file for each thing you want to
> support and keep the shared bits in .dtsi files.

No problem. We'll experiment with overlays in m1n1 and see how that goes.

One thing I wanted to ask: is there some kind of "experimental" policy 
for DT bindings? At early platform bring-up stages it seems like it 
could be valuable to allow for breaking DT changes while we flesh out 
the details (this is especially true of a reverse engineered platform 
like this, where we don't have knowledge of all the hardware details a 
priori). The dozen or so users we might have at this stage obviously 
won't complain too much :)
Arnd Bergmann Feb. 5, 2021, 12:43 p.m. UTC | #5
On Fri, Feb 5, 2021 at 8:11 AM Hector Martin 'marcan' <marcan@marcan.st> wrote:
>
> One thing I wanted to ask: is there some kind of "experimental" policy
> for DT bindings? At early platform bring-up stages it seems like it
> could be valuable to allow for breaking DT changes while we flesh out
> the details (this is especially true of a reverse engineered platform
> like this, where we don't have knowledge of all the hardware details a
> priori). The dozen or so users we might have at this stage obviously
> won't complain too much :)

We don't have a hard policy here, other than the obvious "never break
setups that users rely on". As you expected, this usually means that
I wouldn't complain if you change something in the initial versions
and there is a good reason for that change, it shouldn't be a problem.

An extreme example here is the omap platform that keeps making
incompatible dt changes as there is still ongoing work for removing
the remaining platform specific code in arch/arm/ and replacing it
with DT descriptions.

Once your port is fairly complete and there are users that rely on stability
of the bindings, I would expect all further changes to be compatible in
both ways, allowing old kernels with new dtbs and the other way round.

       Arnd
Krzysztof Kozlowski Feb. 8, 2021, 11:04 a.m. UTC | #6
On Fri, Feb 05, 2021 at 05:39:51AM +0900, Hector Martin wrote:
> This currently supports:
> 
> * SMP (via spin-tables)
> * AIC IRQs
> * Serial (with earlycon)
> * Framebuffer
> 
> A number of properties are dynamic, and based on system firmware
> decisions that vary from version to version. These are expected
> to be filled in by the loader.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  MAINTAINERS                              |   1 +
>  arch/arm64/boot/dts/Makefile             |   1 +
>  arch/arm64/boot/dts/apple/Makefile       |   2 +
>  arch/arm64/boot/dts/apple/apple-j274.dts | 143 +++++++++++++++++++++++
>  4 files changed, 147 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/apple/Makefile
>  create mode 100644 arch/arm64/boot/dts/apple/apple-j274.dts
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3a54ee5747d3..5481b5bc2ef7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1635,6 +1635,7 @@ C:	irc://chat.freenode.net/asahi-dev
>  T:	git https://github.com/AsahiLinux/linux.git
>  F:	Documentation/devicetree/bindings/arm/AAPL.yaml
>  F:	Documentation/devicetree/bindings/interrupt-controller/AAPL,aic.yaml
> +F:	arch/arm64/boot/dts/AAPL/

apple

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

>  F:	drivers/irqchip/irq-apple-aic.c
>  F:	include/dt-bindings/interrupt-controller/apple-aic.h
>  
> diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
> index 9b1170658d60..64f055d94948 100644
> --- a/arch/arm64/boot/dts/Makefile
> +++ b/arch/arm64/boot/dts/Makefile
> @@ -6,6 +6,7 @@ subdir-y += amazon
>  subdir-y += amd
>  subdir-y += amlogic
>  subdir-y += apm
> +subdir-y += apple
>  subdir-y += arm
>  subdir-y += bitmain
>  subdir-y += broadcom
> diff --git a/arch/arm64/boot/dts/apple/Makefile b/arch/arm64/boot/dts/apple/Makefile
> new file mode 100644
> index 000000000000..ec03c474efd4
> --- /dev/null
> +++ b/arch/arm64/boot/dts/apple/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +dtb-$(CONFIG_ARCH_APPLE) += apple-j274.dtb
> diff --git a/arch/arm64/boot/dts/apple/apple-j274.dts b/arch/arm64/boot/dts/apple/apple-j274.dts
> new file mode 100644
> index 000000000000..238a1bcee066
> --- /dev/null
> +++ b/arch/arm64/boot/dts/apple/apple-j274.dts
> @@ -0,0 +1,143 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2021 Hector Martin <marcan@marcan.st>

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)?

> + */
> +
> +/dts-v1/;
> +#include <dt-bindings/interrupt-controller/apple-aic.h>
> +#include <dt-bindings/interrupt-controller/irq.h>
> +
> +/ {
> +	model = "Apple Mac Mini M1 2020";
> +	compatible = "AAPL,j274", "AAPL,m1", "AAPL,arm-platform";

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.

> +	#address-cells = <2>;
> +	#size-cells = <2>;
> +
> +	chosen {
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		bootargs = "earlycon";

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

> +		stdout-path = "serial0:1500000";

Use aliases.

> +
> +		framebuffer0: framebuffer@0 {
> +			compatible = "AAPL,simple-framebuffer", "simple-framebuffer";
> +			reg = <0 0 0 0>; // To be filled by loader
> +			// Format properties will be added by loader

Use /* style of comments

> +			status = "disabled";
> +		};
> +	};
> +
> +	memory@800000000 {
> +		device_type = "memory";
> +		reg = <0 0 0 0>; // To be filled by loader
> +	};
> +
> +	aliases {
> +		serial0 = &serial0;
> +	};
> +
> +	cpus {
> +		#address-cells = <2>;
> +		#size-cells = <0>;
> +
> +		cpu0: cpu@0 {
> +			compatible = "AAPL,icestorm";
> +			device_type = "cpu";
> +			reg = <0x0 0x0>;
> +			enable-method = "spin-table";
> +			cpu-release-addr = <0 0>; // To be filled by loader
> +		};
> +		cpu1: cpu@1 {
> +			compatible = "AAPL,icestorm";
> +			device_type = "cpu";
> +			reg = <0x0 0x1>;
> +			enable-method = "spin-table";
> +			cpu-release-addr = <0 0>; // To be filled by loader
> +		};
> +		cpu2: cpu@2 {
> +			compatible = "AAPL,icestorm";
> +			device_type = "cpu";
> +			reg = <0x0 0x2>;
> +			enable-method = "spin-table";
> +			cpu-release-addr = <0 0>; // To be filled by loader
> +		};
> +		cpu3: cpu@3 {
> +			compatible = "AAPL,icestorm";
> +			device_type = "cpu";
> +			reg = <0x0 0x3>;
> +			enable-method = "spin-table";
> +			cpu-release-addr = <0 0>; // To be filled by loader
> +		};
> +		cpu4: cpu@10100 {
> +			compatible = "AAPL,firestorm";
> +			device_type = "cpu";
> +			reg = <0x0 0x10100>;
> +			enable-method = "spin-table";
> +			cpu-release-addr = <0 0>; // To be filled by loader
> +		};
> +		cpu5: cpu@10101 {
> +			compatible = "AAPL,firestorm";
> +			device_type = "cpu";
> +			reg = <0x0 0x10101>;
> +			enable-method = "spin-table";
> +			cpu-release-addr = <0 0>; // To be filled by loader
> +		};
> +		cpu6: cpu@10102 {
> +			compatible = "AAPL,firestorm";
> +			device_type = "cpu";
> +			reg = <0x0 0x10102>;
> +			enable-method = "spin-table";
> +			cpu-release-addr = <0 0>; // To be filled by loader
> +		};
> +		cpu7: cpu@10103 {
> +			compatible = "AAPL,firestorm";
> +			device_type = "cpu";
> +			reg = <0x0 0x10103>;
> +			enable-method = "spin-table";
> +			cpu-release-addr = <0 0>; // To be filled by loader
> +		};
> +	};
> +
> +	timer {
> +		compatible = "arm,armv8-timer";
> +		interrupt-parent = <&aic>;
> +		interrupts = <AIC_FIQ 0 IRQ_TYPE_LEVEL_HIGH>,
> +				<AIC_FIQ 0 IRQ_TYPE_LEVEL_HIGH>,
> +				<AIC_FIQ 1 IRQ_TYPE_LEVEL_HIGH>,
> +				<AIC_FIQ 0 IRQ_TYPE_LEVEL_HIGH>;
> +	};
> +
> +	clk24: clk24 {

Just "clock". Node names should be generic.

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

> +	};
> +
> +	soc {
> +		compatible = "simple-bus";
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +
> +		aic: interrupt-controller@23b100000 {
> +			compatible = "AAPL,m1-aic", "AAPL,aic";
> +			#interrupt-cells = <3>;
> +			interrupt-controller;
> +			reg = <0x2 0x3b100000 0x0 0x8000>;
> +		};
> +
> +		serial0: serial@235200000 {
> +			compatible = "AAPL,s5l-uart";
> +			reg = <0x2 0x35200000 0x0 0x1000>;
> +			reg-io-width = <4>;
> +			interrupt-parent = <&aic>;
> +			interrupts = <AIC_IRQ 605 IRQ_TYPE_LEVEL_HIGH>;
> +			clocks = <&clk24>, <&clk24>;
> +			clock-names = "uart", "clk_uart_baud0";
> +		};
> +

No blank lines at end of blocks.

Best regards,
Krzysztof
Hector Martin Feb. 8, 2021, 11:56 a.m. UTC | #7
On 08/02/2021 20.04, Krzysztof Kozlowski wrote:
> 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 :)

>> + * Copyright 2021 Hector Martin <marcan@marcan.st>
> 
> 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)

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

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

>> +	clk24: clk24 {
> 
> Just "clock". Node names should be generic.

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

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

Ack on all the other comments, will fix for v2.

Thanks for the review!
Krzysztof Kozlowski Feb. 8, 2021, 12:13 p.m. UTC | #8
On Mon, Feb 08, 2021 at 08:56:53PM +0900, Hector Martin 'marcan' wrote:
> On 08/02/2021 20.04, Krzysztof Kozlowski wrote:
> > 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 :)

Thanks for explanation. I propose to choose just "apple". Sticking to
old vendor name is not a requirement - we have few vendor prefixes which
were marked as deprecated because we switched to a better one.

> 
> > > + * Copyright 2021 Hector Martin <marcan@marcan.st>
> > 
> > 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.

Makes sense. In such case it's indeed your work. Since you introduce it,
the DTSes are usually licensed with (GPL-2.0+ OR MIT).

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

The copyrights matter more in case the need to relicense the work or
some copyright-infringement cases. If you use generic alias - The Asahi
contributors - how it would be possible to find people with actual
copyrights? For example to ask them about relicense permission?  Unless
it's an official body (e.g. foundation or company).  Therefore I propose
to stick to real names and include other contributors once they
contribute.

However this are just my thoughts, not a really professional opinion
about copyright aspects.

> 
> > 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.
> 
> > > +		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.
> 
> > > +	clk24: clk24 {
> > 
> > Just "clock". Node names should be generic.
> 
> Really? Almost every other device device tree uses unique clock node names.

Yes, really, devicetree/ePAPR spec:
"The name of a node should be somewhat generic, reflecting the function
of the device and not its precise programming model. If appropriate, the
name should be one of the following choices:"

Multiple other boards and people (including myself...) made the same
mistake of adding specific names. Even some platform maintainers still
don't get it or never cared to look at DT spec. :)

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

Such workaround is ok, but maybe add a comment that it's a workaround so
far.

Best regards,
Krzysztof
Marc Zyngier Feb. 8, 2021, 12:27 p.m. UTC | #9
On 2021-02-04 20:39, Hector Martin wrote:
> This currently supports:
> 
> * SMP (via spin-tables)
> * AIC IRQs
> * Serial (with earlycon)
> * Framebuffer
> 
> A number of properties are dynamic, and based on system firmware
> decisions that vary from version to version. These are expected
> to be filled in by the loader.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---

[...]

> +	timer {
> +		compatible = "arm,armv8-timer";
> +		interrupt-parent = <&aic>;
> +		interrupts = <AIC_FIQ 0 IRQ_TYPE_LEVEL_HIGH>,
> +				<AIC_FIQ 0 IRQ_TYPE_LEVEL_HIGH>,
> +				<AIC_FIQ 1 IRQ_TYPE_LEVEL_HIGH>,
> +				<AIC_FIQ 0 IRQ_TYPE_LEVEL_HIGH>;

This unfortunately doesn't match the binding, which doesn't cater
for systems without a secure physical timer, nor allows the description
of the EL2 virtual timer.

You should also have *different* interrupts for EL1 and EL2 timers,
although this is all a lie...

Looking at the only similar case, XGene lies about the secure timer
(it doesn't have any), and of course doesn't have an EL2 virtual
timer (ARMv8.0 only).

A sensible course of action could be to update the binding to at least:

- tell the kernel that there is no secure physical timer (and that
   the interrupt should be ignored)
- introduce a 5th possible interrupt for the EL2 virtual timer.

Thanks,

         M.
Arnd Bergmann Feb. 8, 2021, 12:40 p.m. UTC | #10
On Mon, Feb 8, 2021 at 1:13 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On Mon, Feb 08, 2021 at 08:56:53PM +0900, Hector Martin 'marcan' wrote:
> > On 08/02/2021 20.04, Krzysztof Kozlowski wrote:
> > > 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 :)
>
> Thanks for explanation. I propose to choose just "apple". Sticking to
> old vendor name is not a requirement - we have few vendor prefixes which
> were marked as deprecated because we switched to a better one.

We've gone back and forth on this a few times already. My current
preference would also be to go with "apple", not because it's somehow
nicer or clearer but because it avoids the namespace conflict with
what the Apple firmware uses:

The point of the vendor prefix is to prevent two people from introducing
the same identifier with slightly different meanings. This used to be
the stock ticker symbol ("ibm", "SUNW", "AAPL", ...) back when the
only people making devices with firmware were large corporations.

In FDT, it's usually random contributors introducing the names to make
something work on Linux, with unique strings coming from code review.

The identifiers that Apple use are highly unlikely to cause clashes
with the ones we use in Linux, since you probably won't find much
software that has to deal with both formats, but the fact that Apple
still prefixes those strings with their stock ticker symbol tells me that
someone there still considers the namespace relevant, so it's more
polite to stay out of their lawn.

> Makes sense. In such case it's indeed your work. Since you introduce it,
> the DTSes are usually licensed with (GPL-2.0+ OR MIT).

Indeed, we do want other OSs to use our dts files, so the general
preference is to have a permissive license, unless you have a strong
reason yourself to require GPL-only.

       Arnd
Hector Martin Feb. 8, 2021, 2:12 p.m. UTC | #11
On 08/02/2021 21.40, Arnd Bergmann wrote:
> On Mon, Feb 8, 2021 at 1:13 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>>
>> On Mon, Feb 08, 2021 at 08:56:53PM +0900, Hector Martin 'marcan' wrote:
>>> On 08/02/2021 20.04, Krzysztof Kozlowski wrote:
>>>> 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 :)
>>
>> Thanks for explanation. I propose to choose just "apple". Sticking to
>> old vendor name is not a requirement - we have few vendor prefixes which
>> were marked as deprecated because we switched to a better one.
> 
> We've gone back and forth on this a few times already. My current
> preference would also be to go with "apple", not because it's somehow
> nicer or clearer but because it avoids the namespace conflict with
> what the Apple firmware uses:

Ack, I'll use 'apple' for v2.

Amusingly, Apple actually use 'apple,firestorm' and 'apple,icestorm' for 
the CPUs in their devicetrees for these machines, so those will end up 
identical :) (they don't use apple-related prefixes for any other 
compatible strings at all, it's a mess). But we don't care about what 
their ADTs (Apple DTs) do in Linux anyway, the bootloader abstracts all 
that out and we'll be dealing with mantaining proper DTs ourselves.

>> Makes sense. In such case it's indeed your work. Since you introduce it,
>> the DTSes are usually licensed with (GPL-2.0+ OR MIT).
> 
> Indeed, we do want other OSs to use our dts files, so the general
> preference is to have a permissive license, unless you have a strong
> reason yourself to require GPL-only.

Thanks for pointing this out; this was actually unintentional. I based 
it off of an old dts I'd written ages ago and forgot to revisit the 
license. I even have it marked GPL-2.0+ in the copy in our bootloader 
repo, which is otherwise supposed to be MIT for original code...
Hector Martin Feb. 8, 2021, 2:53 p.m. UTC | #12
On 08/02/2021 21.27, Marc Zyngier wrote:
>> +	timer {
>> +		compatible = "arm,armv8-timer";
>> +		interrupt-parent = <&aic>;
>> +		interrupts = <AIC_FIQ 0 IRQ_TYPE_LEVEL_HIGH>,
>> +				<AIC_FIQ 0 IRQ_TYPE_LEVEL_HIGH>,
>> +				<AIC_FIQ 1 IRQ_TYPE_LEVEL_HIGH>,
>> +				<AIC_FIQ 0 IRQ_TYPE_LEVEL_HIGH>;
> 
> This unfortunately doesn't match the binding, which doesn't cater
> for systems without a secure physical timer, nor allows the description
> of the EL2 virtual timer.
> 
> You should also have *different* interrupts for EL1 and EL2 timers,
> although this is all a lie...

Well, we do - now that I confirmed all 4 timers work properly, the AIC 
driver should provide all 4. And ideally I find those EL1 timer mask 
bits and implement them in the aic driver too (for only the virt timers 
that have them and of course need them).

I just found the code in arm_arch_timer that forwards all this stuff to 
the kvm code, so it all makes sense now; if I can wire that up properly, 
heck, KVM might even just work here.

> 
> Looking at the only similar case, XGene lies about the secure timer
> (it doesn't have any), and of course doesn't have an EL2 virtual
> timer (ARMv8.0 only).
> 
> A sensible course of action could be to update the binding to at least:
> 
> - tell the kernel that there is no secure physical timer (and that
>     the interrupt should be ignored)
> - introduce a 5th possible interrupt for the EL2 virtual timer.

Sounds like I should be introducing interrupt-names support into this 
driver and using that, so we can just not specify IRQs that don't exist, 
instead of the hack with dummies. Falling back to indexes of course, to 
keep DT compat. i.e.

const char *names = {"phys-secure", "phys", "virt", "hyp-phys", "hyp-virt"};

bool has_names = of_property_read_bool(..., "interrupt-names");

for (each irq)
	if (has_names) foo = of_irq_get_byname(..., names[i])
	else foo = of_irq_get(..., i)

That said, is there a use case for the EL2 virtual timer? The driver 
always uses the EL2 physical timer with VHE right now. I guess it's 
worth describing it in the binding and dts, even if the driver never 
selects it...?
Marc Zyngier Feb. 8, 2021, 3:36 p.m. UTC | #13
On 2021-02-08 14:53, Hector Martin wrote:
> On 08/02/2021 21.27, Marc Zyngier wrote:
>>> +	timer {
>>> +		compatible = "arm,armv8-timer";
>>> +		interrupt-parent = <&aic>;
>>> +		interrupts = <AIC_FIQ 0 IRQ_TYPE_LEVEL_HIGH>,
>>> +				<AIC_FIQ 0 IRQ_TYPE_LEVEL_HIGH>,
>>> +				<AIC_FIQ 1 IRQ_TYPE_LEVEL_HIGH>,
>>> +				<AIC_FIQ 0 IRQ_TYPE_LEVEL_HIGH>;
>> 
>> This unfortunately doesn't match the binding, which doesn't cater
>> for systems without a secure physical timer, nor allows the 
>> description
>> of the EL2 virtual timer.
>> 
>> You should also have *different* interrupts for EL1 and EL2 timers,
>> although this is all a lie...
> 
> Well, we do - now that I confirmed all 4 timers work properly, the AIC
> driver should provide all 4. And ideally I find those EL1 timer mask
> bits and implement them in the aic driver too (for only the virt
> timers that have them and of course need them).
> 
> I just found the code in arm_arch_timer that forwards all this stuff
> to the kvm code, so it all makes sense now; if I can wire that up
> properly, heck, KVM might even just work here.

There is a bunch of other things to do to enable KVM, specially the
GICv3 emulation, but I've now started refactoring that part of the
code not to rely on a full blown CPU interface. Hopefully I'll have
something for the 5.13 time frame.

> 
>> 
>> Looking at the only similar case, XGene lies about the secure timer
>> (it doesn't have any), and of course doesn't have an EL2 virtual
>> timer (ARMv8.0 only).
>> 
>> A sensible course of action could be to update the binding to at 
>> least:
>> 
>> - tell the kernel that there is no secure physical timer (and that
>>     the interrupt should be ignored)
>> - introduce a 5th possible interrupt for the EL2 virtual timer.
> 
> Sounds like I should be introducing interrupt-names support into this
> driver and using that, so we can just not specify IRQs that don't
> exist, instead of the hack with dummies. Falling back to indexes of
> course, to keep DT compat. i.e.
> 
> const char *names = {"phys-secure", "phys", "virt", "hyp-phys", 
> "hyp-virt"};
> 
> bool has_names = of_property_read_bool(..., "interrupt-names");
> 
> for (each irq)
> 	if (has_names) foo = of_irq_get_byname(..., names[i])
> 	else foo = of_irq_get(..., i)

Yup, that definitely looks like a good thing to introduce.

> That said, is there a use case for the EL2 virtual timer? The driver
> always uses the EL2 physical timer with VHE right now. I guess it's
> worth describing it in the binding and dts, even if the driver never
> selects it...?

Linux doesn't have a use for the EL2 virtual timer yet. It was only
introduced for symmetry with EL1 (except for CNTVOFF of course).
But it definitely is worth describing it. Who knows, we may find a use
for it at some point, and other OSes are using the same DT binding 
anway.

Thanks,

         M.
Rob Herring (Arm) Feb. 8, 2021, 5:58 p.m. UTC | #14
On Mon, Feb 08, 2021 at 11:12:52PM +0900, Hector Martin wrote:
> On 08/02/2021 21.40, Arnd Bergmann wrote:
> > On Mon, Feb 8, 2021 at 1:13 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > 
> > > On Mon, Feb 08, 2021 at 08:56:53PM +0900, Hector Martin 'marcan' wrote:
> > > > On 08/02/2021 20.04, Krzysztof Kozlowski wrote:
> > > > > 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 :)
> > > 
> > > Thanks for explanation. I propose to choose just "apple". Sticking to
> > > old vendor name is not a requirement - we have few vendor prefixes which
> > > were marked as deprecated because we switched to a better one.
> > 
> > We've gone back and forth on this a few times already. My current
> > preference would also be to go with "apple", not because it's somehow
> > nicer or clearer but because it avoids the namespace conflict with
> > what the Apple firmware uses:

It's only AAPL,phandle and AAPL,unit-string (equivalent to unit-address) 
AFAICT which are really internal format details. So it's really 'apple' 
that could conflct, but I can't see that mattering.

> Ack, I'll use 'apple' for v2.

3 votes for 'apple'. You all get to pick the color of this shed.

> Amusingly, Apple actually use 'apple,firestorm' and 'apple,icestorm' for the
> CPUs in their devicetrees for these machines, so those will end up identical
> :) (they don't use apple-related prefixes for any other compatible strings
> at all, it's a mess). But we don't care about what their ADTs (Apple DTs) do
> in Linux anyway, the bootloader abstracts all that out and we'll be dealing
> with mantaining proper DTs ourselves.
> 
> > > Makes sense. In such case it's indeed your work. Since you introduce it,
> > > the DTSes are usually licensed with (GPL-2.0+ OR MIT).
> > 
> > Indeed, we do want other OSs to use our dts files, so the general
> > preference is to have a permissive license, unless you have a strong
> > reason yourself to require GPL-only.
> 
> Thanks for pointing this out; this was actually unintentional. I based it
> off of an old dts I'd written ages ago and forgot to revisit the license. I
> even have it marked GPL-2.0+ in the copy in our bootloader repo, which is
> otherwise supposed to be MIT for original code...

I'll also highlight there's a DT only tree[1] available to import DT 
related parts to other projects. It's generated from the kernel tree. 
Probably an overkill to copying at this point though.

Rob

[1] https://git.kernel.org/pub/scm/linux/kernel/git/devicetree/devicetree-rebasing.git/
Rob Herring (Arm) Feb. 8, 2021, 7:14 p.m. UTC | #15
On Mon, Feb 08, 2021 at 08:56:53PM +0900, Hector Martin 'marcan' wrote:
> On 08/02/2021 20.04, Krzysztof Kozlowski wrote:
> > 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 :)
> 
> > > + * Copyright 2021 Hector Martin <marcan@marcan.st>
> > 
> > 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?

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

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

> > > +		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
Hector Martin Feb. 9, 2021, 12:32 a.m. UTC | #16
On 09/02/2021 02.58, Rob Herring wrote:
> I'll also highlight there's a DT only tree[1] available to import DT
> related parts to other projects. It's generated from the kernel tree.
> Probably an overkill to copying at this point though.
> 
> Rob
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/devicetree/devicetree-rebasing.git/

This actually brings up something else: do we want (eventually) *all* 
Apple boards using these SoCs to have up-to-date devicetrees in the 
Linux kernel tree? Obviously not every device supported by mainline has 
proper ones in, but I don't know if that's expected or something to be 
avoided.

If this is intended to be kept in sync and be fully comprehensive, I 
might as well start planning out our longer term DT maintenance strategy 
around that (which might involve using that tree in our bootloader).
Hector Martin Feb. 9, 2021, 12:49 a.m. UTC | #17
On 09/02/2021 04.14, Rob Herring wrote:
> Does there need to be a legal entity behind 'The Asahi Linux
> Contributors' to be valid?

I don't think so, this seems to be common practice in other open source 
projects, and recommended these days.

Some recent discussion on the subject from the Linux Foundation:

https://www.linuxfoundation.org/en/blog/copyright-notices-in-open-source-software-projects/

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

I thought that's what Git history was for; certainly we aren't keeping 
file headers up to date every time someone touches a file (which for 
anything other than trivial changes gives them a copyright interest in a 
portion of the file).

Asahi Linux's policy for bespoke projects is to use "The Asahi Linux 
Contributors" for this reason, acknowledging that the copyright headers 
aren't up to date anyway (also the years...), and implicitly directing 
people to the orignal project (which is where Git history is kept and 
contains the true record of copyright owneship).

I'm not trying to shake up how we handle copyright lines in the kernel 
here, of course; if you prefer some nominal copyright line from "whoever 
first wrote the file or most of it" I can do that. But it certainly 
won't be the only person you have to ask if you want to relicense, if 
anyone else touched the file in a nontrivial way :)

There are a few examples of this style in the tree, mostly pulled from 
other projects:

arch/arm/oprofile/common.c
drivers/gpu/drm/vgem/vgem_drv.[ch]
drivers/md/dm-verity-target.c
drivers/md/dm-verity.h

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

The pattern here was board, soc, "arm-platform"; the first two seem to 
be a common (and useful) pattern, and I hope I can get rid of the third 
once we solve #11/#12 in a saner way.

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

What do you suggest for this series?

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

Ack. I hope we can basically acknowledge breaking DT changes without too 
much fuss at this early stage of bring-up, until things calm down a bit 
and we have real users who would complain :) (not that I won't try to 
avoid it).
Rob Herring (Arm) Feb. 9, 2021, 2:05 a.m. UTC | #18
On Mon, Feb 8, 2021 at 6:49 PM Hector Martin <marcan@marcan.st> wrote:
>
> On 09/02/2021 04.14, Rob Herring wrote:
> > Does there need to be a legal entity behind 'The Asahi Linux
> > Contributors' to be valid?
>
> I don't think so, this seems to be common practice in other open source
> projects, and recommended these days.
>
> Some recent discussion on the subject from the Linux Foundation:
>
> https://www.linuxfoundation.org/en/blog/copyright-notices-in-open-source-software-projects/

Okay, that's good to know. I'd primarily seen this for Chromium which
obviously has a company (and lawyers) behind it.

> >  From a more practical standpoint, if we want to relicense something in
> > say 5 years from now, who do we ask for an okay?
>
> I thought that's what Git history was for; certainly we aren't keeping
> file headers up to date every time someone touches a file (which for
> anything other than trivial changes gives them a copyright interest in a
> portion of the file).

Yes, for sure. Especially since copyrights tend to be stale as the
blog talks about.

> Asahi Linux's policy for bespoke projects is to use "The Asahi Linux
> Contributors" for this reason, acknowledging that the copyright headers
> aren't up to date anyway (also the years...), and implicitly directing
> people to the orignal project (which is where Git history is kept and
> contains the true record of copyright owneship).
>
> I'm not trying to shake up how we handle copyright lines in the kernel
> here, of course; if you prefer some nominal copyright line from "whoever
> first wrote the file or most of it" I can do that. But it certainly
> won't be the only person you have to ask if you want to relicense, if
> anyone else touched the file in a nontrivial way :)

No, it's fine as is.

> There are a few examples of this style in the tree, mostly pulled from
> other projects:
>
> arch/arm/oprofile/common.c
> drivers/gpu/drm/vgem/vgem_drv.[ch]
> drivers/md/dm-verity-target.c
> drivers/md/dm-verity.h
>
> >>> 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.
>
> The pattern here was board, soc, "arm-platform"; the first two seem to
> be a common (and useful) pattern, and I hope I can get rid of the third
> once we solve #11/#12 in a saner way.
>
> > 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.
>
> What do you suggest for this series?

Fine as-is unless someone wants to define the pattern and add it to
the fixed-clock schema before this is merged.

Rob
Tony Lindgren Feb. 10, 2021, 10:19 a.m. UTC | #19
* Hector Martin 'marcan' <marcan@marcan.st> [210208 12:05]:
> On 08/02/2021 20.04, Krzysztof Kozlowski wrote:
...

> > > +	clk24: clk24 {
> > 
> > Just "clock". Node names should be generic.
> 
> Really? Almost every other device device tree uses unique clock node names.

Yeah please just use generic node name "clock". FYI, we're still hurting
because of this for the TI clock node names years after because the drivers
got a chance to rely on the clock node name..

Using "clock" means your clock driver code won't get a chance to wrongly
use the node name and you avoid similar issues.

Regards,

Tony
Hector Martin Feb. 10, 2021, 11:07 a.m. UTC | #20
On 10/02/2021 19.19, Tony Lindgren wrote:
> * Hector Martin 'marcan' <marcan@marcan.st> [210208 12:05]:
>> On 08/02/2021 20.04, Krzysztof Kozlowski wrote:
> ...
> 
>>>> +	clk24: clk24 {
>>>
>>> Just "clock". Node names should be generic.
>>
>> Really? Almost every other device device tree uses unique clock node names.
> 
> Yeah please just use generic node name "clock". FYI, we're still hurting
> because of this for the TI clock node names years after because the drivers
> got a chance to rely on the clock node name..
> 
> Using "clock" means your clock driver code won't get a chance to wrongly
> use the node name and you avoid similar issues.

That means it'll end up like this (so that we can have more than one 
fixed-clock):

clocks {
     #address-cells = <1>;
     #size-cells = <0>;

     clk123: clock@0 {
         ...
         reg = <0>
     }

     clk456: clock@1 {
         ...
         reg = <1>
     }
}

Correct?

Incidentally, there is just one example in the kernel tree of doing this 
right (in arch/arm/boot/dts/imx6qdl-tx6.dtsi). All the others that use 
non-mmio clocks called `clock`, including the various tegra devicetrees, 
violate the DT spec by not including a dummy reg property matching the 
unit-address.
Tony Lindgren Feb. 10, 2021, 11:34 a.m. UTC | #21
* Hector Martin <marcan@marcan.st> [210210 11:14]:
> On 10/02/2021 19.19, Tony Lindgren wrote:
> > * Hector Martin 'marcan' <marcan@marcan.st> [210208 12:05]:
> > > On 08/02/2021 20.04, Krzysztof Kozlowski wrote:
> > ...
> > 
> > > > > +	clk24: clk24 {
> > > > 
> > > > Just "clock". Node names should be generic.
> > > 
> > > Really? Almost every other device device tree uses unique clock node names.
> > 
> > Yeah please just use generic node name "clock". FYI, we're still hurting
> > because of this for the TI clock node names years after because the drivers
> > got a chance to rely on the clock node name..
> > 
> > Using "clock" means your clock driver code won't get a chance to wrongly
> > use the node name and you avoid similar issues.
> 
> That means it'll end up like this (so that we can have more than one
> fixed-clock):
> 
> clocks {
>     #address-cells = <1>;
>     #size-cells = <0>;
> 
>     clk123: clock@0 {
>         ...
>         reg = <0>
>     }
> 
>     clk456: clock@1 {
>         ...
>         reg = <1>
>     }
> }
> 
> Correct?

Yeah, just don't use an imaginary dummy index for the reg. Use a real
register offset from a clock controller instance base, and a register
bit offset too if needed.

That way if you discover a new clock inbetween somewhere, you don't have
renumber any imaginary lists in the driver or device tree. So try to
follow sort of what the standard interrupts binding is doing only
describing the hardware.

> Incidentally, there is just one example in the kernel tree of doing this
> right (in arch/arm/boot/dts/imx6qdl-tx6.dtsi). All the others that use
> non-mmio clocks called `clock`, including the various tegra devicetrees,
> violate the DT spec by not including a dummy reg property matching the
> unit-address.

Doing it right will save you tons of time later on ;)

FYI, for the TI clocks, we ended up redoing most of the clocks as
documented in Documentation/devicetree/bindings/clock/ti-clkctrl.txt.

Regards,

Tony
Hector Martin Feb. 10, 2021, 11:43 a.m. UTC | #22
On 10/02/2021 20.34, Tony Lindgren wrote:
> * Hector Martin <marcan@marcan.st> [210210 11:14]:
>> That means it'll end up like this (so that we can have more than one
>> fixed-clock):
>>
>> clocks {
>>      #address-cells = <1>;
>>      #size-cells = <0>;
>>
>>      clk123: clock@0 {
>>          ...
>>          reg = <0>
>>      }
>>
>>      clk456: clock@1 {
>>          ...
>>          reg = <1>
>>      }
>> }
>>
>> Correct?
> 
> Yeah, just don't use an imaginary dummy index for the reg. Use a real
> register offset from a clock controller instance base, and a register
> bit offset too if needed.

I mean for fixed input clocks without any particular numbering, or for 
temporary fake clocks while we figure out the clock controller. Once a 
real clock controller is involved, if there are hardware indexes 
involved that are consistent then of course I'll use those in some way 
that makes sense.

The purpose of the clock in this particular case is just to make the 
uart driver work, since it wants to know its reference clock; there is 
work to be done here to figure out the real clock tree (e.g. we don't 
even know yet if the uart supports alternate clocks, that's tricky to 
test until we have some form of I/O other than uart!).

> Doing it right will save you tons of time later on ;)

Absolutely, I'm just pointing out that instances of it being done right 
are in short supply right now :-) (which makes it tricky for people like 
me, trying to put this together for a new soc, to guess what the right 
approach is by looking at existing examples)
Daniel Palmer Feb. 10, 2021, 12:24 p.m. UTC | #23
Hi Hector,

On Wed, 10 Feb 2021 at 20:49, Hector Martin <marcan@marcan.st> wrote:

> > Yeah, just don't use an imaginary dummy index for the reg. Use a real
> > register offset from a clock controller instance base, and a register
> > bit offset too if needed.
>
> I mean for fixed input clocks without any particular numbering, or for
> temporary fake clocks while we figure out the clock controller. Once a
> real clock controller is involved, if there are hardware indexes
> involved that are consistent then of course I'll use those in some way
> that makes sense.

This exact problem exists for MStar/SigmaStar too.
As it stands there is no documentation to show what the actual clock
tree looks like so everything is guess and I need to come up with numbers.
I'm interested to see what the solution to this is as it will come up again
when mainlining chips without documentation.


> The purpose of the clock in this particular case is just to make the
> uart driver work, since it wants to know its reference clock; there is
> work to be done here to figure out the real clock tree

FWIW arm/boot/dts/mstar-v7.dtsi has the same issue: Needs uart,
has no uart clock. In that instance the uart clock setup by u-boot
is passed to the uart driver as a property instead of creating a fake
clock.

Cheers,

Daniel
Tony Lindgren Feb. 10, 2021, 12:54 p.m. UTC | #24
* Daniel Palmer <daniel@0x0f.com> [210210 12:24]:
> Hi Hector,
> 
> On Wed, 10 Feb 2021 at 20:49, Hector Martin <marcan@marcan.st> wrote:
> 
> > > Yeah, just don't use an imaginary dummy index for the reg. Use a real
> > > register offset from a clock controller instance base, and a register
> > > bit offset too if needed.
> >
> > I mean for fixed input clocks without any particular numbering, or for
> > temporary fake clocks while we figure out the clock controller. Once a
> > real clock controller is involved, if there are hardware indexes
> > involved that are consistent then of course I'll use those in some way
> > that makes sense.
> 
> This exact problem exists for MStar/SigmaStar too.
> As it stands there is no documentation to show what the actual clock
> tree looks like so everything is guess and I need to come up with numbers.
> I'm interested to see what the solution to this is as it will come up again
> when mainlining chips without documentation.
> 
> 
> > The purpose of the clock in this particular case is just to make the
> > uart driver work, since it wants to know its reference clock; there is
> > work to be done here to figure out the real clock tree
> 
> FWIW arm/boot/dts/mstar-v7.dtsi has the same issue: Needs uart,
> has no uart clock. In that instance the uart clock setup by u-boot
> is passed to the uart driver as a property instead of creating a fake
> clock.

Using more local dts nodes for the fixed clocks might help a bit with
the dummy numbering problem but is still not a nice solution.

How about using node names like "clock-foo" for the fixed clocks?
This would be along what we do for with regulator names.

Rob and Stephen might have some better suggestions here.

Regards,

Tony
Krzysztof Kozlowski Feb. 10, 2021, 12:55 p.m. UTC | #25
On Wed, Feb 10, 2021 at 01:34:50PM +0200, Tony Lindgren wrote:
> * Hector Martin <marcan@marcan.st> [210210 11:14]:
> > On 10/02/2021 19.19, Tony Lindgren wrote:
> > > * Hector Martin 'marcan' <marcan@marcan.st> [210208 12:05]:
> > > > On 08/02/2021 20.04, Krzysztof Kozlowski wrote:
> > > ...
> > > 
> > > > > > +	clk24: clk24 {
> > > > > 
> > > > > Just "clock". Node names should be generic.
> > > > 
> > > > Really? Almost every other device device tree uses unique clock node names.
> > > 
> > > Yeah please just use generic node name "clock". FYI, we're still hurting
> > > because of this for the TI clock node names years after because the drivers
> > > got a chance to rely on the clock node name..
> > > 
> > > Using "clock" means your clock driver code won't get a chance to wrongly
> > > use the node name and you avoid similar issues.
> > 
> > That means it'll end up like this (so that we can have more than one
> > fixed-clock):
> > 
> > clocks {
> >     #address-cells = <1>;
> >     #size-cells = <0>;
> > 
> >     clk123: clock@0 {
> >         ...
> >         reg = <0>
> >     }
> > 
> >     clk456: clock@1 {
> >         ...
> >         reg = <1>
> >     }
> > }
> > 
> > Correct?
> 
> Yeah, just don't use an imaginary dummy index for the reg. Use a real
> register offset from a clock controller instance base, and a register
> bit offset too if needed.

No, there is no need for fake "clocks" node with fake addresses. If you
have multiple clocks, the rules are the same as for other similar cases,
e.g. leds:

{
    clock-0 {
       ...
    };

    clock-1 {
        ..
    };

    soc@0 {
    };
}

This should not generate any dtc W=1 warnings and work with dtschema
(you need to check for both).

Best regards,
Krzysztof
Hector Martin Feb. 10, 2021, 12:56 p.m. UTC | #26
On 10/02/2021 21.24, Daniel Palmer wrote:
> This exact problem exists for MStar/SigmaStar too.
> As it stands there is no documentation to show what the actual clock
> tree looks like so everything is guess and I need to come up with numbers.
> I'm interested to see what the solution to this is as it will come up again
> when mainlining chips without documentation.

So far the answer seems to be to take the best guess available, and then 
we get to fix it until we have real users and breaking DT compatibility 
is no longer an option... :-)

>> The purpose of the clock in this particular case is just to make the
>> uart driver work, since it wants to know its reference clock; there is
>> work to be done here to figure out the real clock tree
> 
> FWIW arm/boot/dts/mstar-v7.dtsi has the same issue: Needs uart,
> has no uart clock. In that instance the uart clock setup by u-boot
> is passed to the uart driver as a property instead of creating a fake
> clock.

In our case it's an existing driver (with patches) that is already 
integrated with the clock infrastructure, so it makes sense to use a 
fixed-clock instead of just an ad-hoc property.
Tony Lindgren Feb. 10, 2021, 1:19 p.m. UTC | #27
* Krzysztof Kozlowski <krzk@kernel.org> [210210 12:56]:
> On Wed, Feb 10, 2021 at 01:34:50PM +0200, Tony Lindgren wrote:
> > * Hector Martin <marcan@marcan.st> [210210 11:14]:
> > > On 10/02/2021 19.19, Tony Lindgren wrote:
> > > > * Hector Martin 'marcan' <marcan@marcan.st> [210208 12:05]:
> > > > > On 08/02/2021 20.04, Krzysztof Kozlowski wrote:
> > > > ...
> > > > 
> > > > > > > +	clk24: clk24 {
> > > > > > 
> > > > > > Just "clock". Node names should be generic.
> > > > > 
> > > > > Really? Almost every other device device tree uses unique clock node names.
> > > > 
> > > > Yeah please just use generic node name "clock". FYI, we're still hurting
> > > > because of this for the TI clock node names years after because the drivers
> > > > got a chance to rely on the clock node name..
> > > > 
> > > > Using "clock" means your clock driver code won't get a chance to wrongly
> > > > use the node name and you avoid similar issues.
> > > 
> > > That means it'll end up like this (so that we can have more than one
> > > fixed-clock):
> > > 
> > > clocks {
> > >     #address-cells = <1>;
> > >     #size-cells = <0>;
> > > 
> > >     clk123: clock@0 {
> > >         ...
> > >         reg = <0>
> > >     }
> > > 
> > >     clk456: clock@1 {
> > >         ...
> > >         reg = <1>
> > >     }
> > > }
> > > 
> > > Correct?
> > 
> > Yeah, just don't use an imaginary dummy index for the reg. Use a real
> > register offset from a clock controller instance base, and a register
> > bit offset too if needed.
> 
> No, there is no need for fake "clocks" node with fake addresses. If you
> have multiple clocks, the rules are the same as for other similar cases,
> e.g. leds:
> 
> {
>     clock-0 {
>        ...
>     };
> 
>     clock-1 {
>         ..
>     };
> 
>     soc@0 {
>     };
> }
> 
> This should not generate any dtc W=1 warnings and work with dtschema
> (you need to check for both).

OK yeah so no need for the node name there after the "clock-" :)
Sounds good to me.

Regards,

Tony
Krzysztof Kozlowski Feb. 10, 2021, 1:25 p.m. UTC | #28
On Wed, Feb 10, 2021 at 03:19:46PM +0200, Tony Lindgren wrote:
> * Krzysztof Kozlowski <krzk@kernel.org> [210210 12:56]:
> > On Wed, Feb 10, 2021 at 01:34:50PM +0200, Tony Lindgren wrote:
> > > * Hector Martin <marcan@marcan.st> [210210 11:14]:
> > > > On 10/02/2021 19.19, Tony Lindgren wrote:
> > > > > * Hector Martin 'marcan' <marcan@marcan.st> [210208 12:05]:
> > > > > > On 08/02/2021 20.04, Krzysztof Kozlowski wrote:
> > > > > ...
> > > > > 
> > > > > > > > +	clk24: clk24 {
> > > > > > > 
> > > > > > > Just "clock". Node names should be generic.
> > > > > > 
> > > > > > Really? Almost every other device device tree uses unique clock node names.
> > > > > 
> > > > > Yeah please just use generic node name "clock". FYI, we're still hurting
> > > > > because of this for the TI clock node names years after because the drivers
> > > > > got a chance to rely on the clock node name..
> > > > > 
> > > > > Using "clock" means your clock driver code won't get a chance to wrongly
> > > > > use the node name and you avoid similar issues.
> > > > 
> > > > That means it'll end up like this (so that we can have more than one
> > > > fixed-clock):
> > > > 
> > > > clocks {
> > > >     #address-cells = <1>;
> > > >     #size-cells = <0>;
> > > > 
> > > >     clk123: clock@0 {
> > > >         ...
> > > >         reg = <0>
> > > >     }
> > > > 
> > > >     clk456: clock@1 {
> > > >         ...
> > > >         reg = <1>
> > > >     }
> > > > }
> > > > 
> > > > Correct?
> > > 
> > > Yeah, just don't use an imaginary dummy index for the reg. Use a real
> > > register offset from a clock controller instance base, and a register
> > > bit offset too if needed.
> > 
> > No, there is no need for fake "clocks" node with fake addresses. If you
> > have multiple clocks, the rules are the same as for other similar cases,
> > e.g. leds:
> > 
> > {
> >     clock-0 {
> >        ...
> >     };
> > 
> >     clock-1 {
> >         ..
> >     };
> > 
> >     soc@0 {
> >     };
> > }
> > 
> > This should not generate any dtc W=1 warnings and work with dtschema
> > (you need to check for both).
> 
> OK yeah so no need for the node name there after the "clock-" :)
> Sounds good to me.

You also could add a suffix or prefix ("fixed-clock-0", "clock-ext").
There is no strict requirement and Rob admitted that as-is was good
enough.

From my point of view, the dt spec mentions "clock" as one of preferred
names, so I would stick to it.

Anyway, it's not that important. :)

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 3a54ee5747d3..5481b5bc2ef7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1635,6 +1635,7 @@  C:	irc://chat.freenode.net/asahi-dev
 T:	git https://github.com/AsahiLinux/linux.git
 F:	Documentation/devicetree/bindings/arm/AAPL.yaml
 F:	Documentation/devicetree/bindings/interrupt-controller/AAPL,aic.yaml
+F:	arch/arm64/boot/dts/AAPL/
 F:	drivers/irqchip/irq-apple-aic.c
 F:	include/dt-bindings/interrupt-controller/apple-aic.h
 
diff --git a/arch/arm64/boot/dts/Makefile b/arch/arm64/boot/dts/Makefile
index 9b1170658d60..64f055d94948 100644
--- a/arch/arm64/boot/dts/Makefile
+++ b/arch/arm64/boot/dts/Makefile
@@ -6,6 +6,7 @@  subdir-y += amazon
 subdir-y += amd
 subdir-y += amlogic
 subdir-y += apm
+subdir-y += apple
 subdir-y += arm
 subdir-y += bitmain
 subdir-y += broadcom
diff --git a/arch/arm64/boot/dts/apple/Makefile b/arch/arm64/boot/dts/apple/Makefile
new file mode 100644
index 000000000000..ec03c474efd4
--- /dev/null
+++ b/arch/arm64/boot/dts/apple/Makefile
@@ -0,0 +1,2 @@ 
+# SPDX-License-Identifier: GPL-2.0
+dtb-$(CONFIG_ARCH_APPLE) += apple-j274.dtb
diff --git a/arch/arm64/boot/dts/apple/apple-j274.dts b/arch/arm64/boot/dts/apple/apple-j274.dts
new file mode 100644
index 000000000000..238a1bcee066
--- /dev/null
+++ b/arch/arm64/boot/dts/apple/apple-j274.dts
@@ -0,0 +1,143 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2021 Hector Martin <marcan@marcan.st>
+ */
+
+/dts-v1/;
+#include <dt-bindings/interrupt-controller/apple-aic.h>
+#include <dt-bindings/interrupt-controller/irq.h>
+
+/ {
+	model = "Apple Mac Mini M1 2020";
+	compatible = "AAPL,j274", "AAPL,m1", "AAPL,arm-platform";
+	#address-cells = <2>;
+	#size-cells = <2>;
+
+	chosen {
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		bootargs = "earlycon";
+		stdout-path = "serial0:1500000";
+
+		framebuffer0: framebuffer@0 {
+			compatible = "AAPL,simple-framebuffer", "simple-framebuffer";
+			reg = <0 0 0 0>; // To be filled by loader
+			// Format properties will be added by loader
+			status = "disabled";
+		};
+	};
+
+	memory@800000000 {
+		device_type = "memory";
+		reg = <0 0 0 0>; // To be filled by loader
+	};
+
+	aliases {
+		serial0 = &serial0;
+	};
+
+	cpus {
+		#address-cells = <2>;
+		#size-cells = <0>;
+
+		cpu0: cpu@0 {
+			compatible = "AAPL,icestorm";
+			device_type = "cpu";
+			reg = <0x0 0x0>;
+			enable-method = "spin-table";
+			cpu-release-addr = <0 0>; // To be filled by loader
+		};
+		cpu1: cpu@1 {
+			compatible = "AAPL,icestorm";
+			device_type = "cpu";
+			reg = <0x0 0x1>;
+			enable-method = "spin-table";
+			cpu-release-addr = <0 0>; // To be filled by loader
+		};
+		cpu2: cpu@2 {
+			compatible = "AAPL,icestorm";
+			device_type = "cpu";
+			reg = <0x0 0x2>;
+			enable-method = "spin-table";
+			cpu-release-addr = <0 0>; // To be filled by loader
+		};
+		cpu3: cpu@3 {
+			compatible = "AAPL,icestorm";
+			device_type = "cpu";
+			reg = <0x0 0x3>;
+			enable-method = "spin-table";
+			cpu-release-addr = <0 0>; // To be filled by loader
+		};
+		cpu4: cpu@10100 {
+			compatible = "AAPL,firestorm";
+			device_type = "cpu";
+			reg = <0x0 0x10100>;
+			enable-method = "spin-table";
+			cpu-release-addr = <0 0>; // To be filled by loader
+		};
+		cpu5: cpu@10101 {
+			compatible = "AAPL,firestorm";
+			device_type = "cpu";
+			reg = <0x0 0x10101>;
+			enable-method = "spin-table";
+			cpu-release-addr = <0 0>; // To be filled by loader
+		};
+		cpu6: cpu@10102 {
+			compatible = "AAPL,firestorm";
+			device_type = "cpu";
+			reg = <0x0 0x10102>;
+			enable-method = "spin-table";
+			cpu-release-addr = <0 0>; // To be filled by loader
+		};
+		cpu7: cpu@10103 {
+			compatible = "AAPL,firestorm";
+			device_type = "cpu";
+			reg = <0x0 0x10103>;
+			enable-method = "spin-table";
+			cpu-release-addr = <0 0>; // To be filled by loader
+		};
+	};
+
+	timer {
+		compatible = "arm,armv8-timer";
+		interrupt-parent = <&aic>;
+		interrupts = <AIC_FIQ 0 IRQ_TYPE_LEVEL_HIGH>,
+				<AIC_FIQ 0 IRQ_TYPE_LEVEL_HIGH>,
+				<AIC_FIQ 1 IRQ_TYPE_LEVEL_HIGH>,
+				<AIC_FIQ 0 IRQ_TYPE_LEVEL_HIGH>;
+	};
+
+	clk24: clk24 {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <24000000>;
+		clock-output-names = "clk24";
+	};
+
+	soc {
+		compatible = "simple-bus";
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+
+		aic: interrupt-controller@23b100000 {
+			compatible = "AAPL,m1-aic", "AAPL,aic";
+			#interrupt-cells = <3>;
+			interrupt-controller;
+			reg = <0x2 0x3b100000 0x0 0x8000>;
+		};
+
+		serial0: serial@235200000 {
+			compatible = "AAPL,s5l-uart";
+			reg = <0x2 0x35200000 0x0 0x1000>;
+			reg-io-width = <4>;
+			interrupt-parent = <&aic>;
+			interrupts = <AIC_IRQ 605 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&clk24>, <&clk24>;
+			clock-names = "uart", "clk_uart_baud0";
+		};
+
+	};
+};