This series brings up initial support for the Apple M1 SoC, used in the
2020 Mac Mini, MacBook Pro, and MacBook Air models.
The following features are supported in this initial port:
- UART (samsung-style) with earlycon support
- Interrupts, including affinity and IPIs (Apple Interrupt Controller)
- SMP (through standard spin-table support)
- simplefb-based framebuffer
- Devicetree for the Mac Mini (should work for the others too at this
stage)
The primary pain points of this port are:
- Apple SoCs require FIQs, as the timers and "fast" IPIs are hardwired
to the FIQ interrupt line. This patchset introduces FIQ support through
the alternatives mechanism, so as to not affect other platforms,
implemented as simply merging IRQs and FIQs. The AIC driver takes care
of discriminating and routing IRQs to the right users.
- These SoCs blackhole nGnRE writes to internal MMIO ranges, and require
nGnRnE. There is no obvious right solution to solve this. I do not
expect the two patches for that in this series to be merged as-is, but
the commit messages describe the problem and potential solutions. I
hope we can have a discussion and converge on the right approach to
solve this problem in the sanest way.
These machines officially support booting unsigned/user-provided
XNU-like kernels, with a very different boot protocol and devicetree
format. We are developing an initial bootloader, m1n1 [1], to take care
of as many hardware peculiarities as possible and present a standard
Linux arm64 boot protocol and device tree. In the future, I expect that
production setups will add U-Boot and perhaps GRUB into the boot chain,
to make the boot process similar to other ARM64 platforms.
The machines expose their debug UART over USB Type C, triggered with
vendor-specific USB-PD commands. Currently, the easiest way to get a
serial console on these machines is to use a second M1 box and a simple
USB C cable [2]. You can also build a DIY interface using an Arduino, a
FUSB302 chip or board, and a 1.2V UART-TTL adapter [3]. In the coming
weeks we will be designing an open hardware project to provide
serial/debug connectivity to these machines (and, hopefully, also
support other UART-over-Type C setups from other vendors). Please
contact me privately if you are interested in getting an early prototype
version of one of these devices.
A quickstart guide to booting Linux kernels on these machines is
available at [4], and we are documenting the hardware at [5].
[1] https://github.com/AsahiLinux/m1n1/
[2] https://github.com/AsahiLinux/macvdmtool/
[3] https://github.com/AsahiLinux/vdmtool/
[4] https://github.com/AsahiLinux/docs/wiki/Developer-Quickstart
[5] https://github.com/AsahiLinux/docs/wiki
== Project Blurb ==
Asahi Linux is an open community project dedicated to developing and
maintaining mainline support for Apple Silicon on Linux. Feel free to
drop by #asahi on freenode to chat with us, or check our website for
more information on the project:
https://asahilinux.org/
Hector Martin (18):
dt-bindings: vendor-prefixes: add AAPL prefix
dt-bindings: arm: cpus: Add AAPL,firestorm & icestorm compatibles
dt-bindings: arm: AAPL: Add bindings for Apple ARM platforms
arm64: Kconfig: Introduce CONFIG_ARCH_APPLE
tty: serial: samsung_tty: add support for Apple UARTs
dt-bindings: serial: samsung: Add AAPL,s5l-uart compatible
tty: serial: samsung_tty: enable for ARCH_APPLE
arm64: cpufeature: Add a feature for FIQ support
arm64: cputype: Add CPU types for the Apple M1 big/little cores
arm64: Introduce FIQ support
arm64: Kconfig: Require FIQ support for ARCH_APPLE
arm64: setup: Use nGnRnE IO mappings for fixmap on Apple platforms
arm64: ioremap: use nGnRnE mappings on platforms that require it
dt-bindings: interrupt-controller: Add DT bindings for apple-aic
irqchip/apple-aic: Add support for the Apple Interrupt Controller
irqchip/apple-aic: Add SMP / IPI support
dt-bindings: display: add AAPL,simple-framebuffer
arm64: apple: Add initial Mac Mini 2020 (M1) devicetree
.../devicetree/bindings/arm/AAPL.yaml | 36 ++
.../devicetree/bindings/arm/cpus.yaml | 2 +
.../bindings/display/simple-framebuffer.yaml | 5 +
.../interrupt-controller/AAPL,aic.yaml | 88 +++
.../bindings/serial/samsung_uart.yaml | 4 +-
.../devicetree/bindings/vendor-prefixes.yaml | 2 +
MAINTAINERS | 14 +
arch/arm64/Kconfig | 10 +
arch/arm64/Kconfig.platforms | 8 +
arch/arm64/boot/dts/Makefile | 1 +
arch/arm64/boot/dts/apple/Makefile | 2 +
arch/arm64/boot/dts/apple/apple-j274.dts | 143 +++++
arch/arm64/include/asm/assembler.h | 4 +
arch/arm64/include/asm/cpucaps.h | 3 +-
arch/arm64/include/asm/cpufeature.h | 6 +
arch/arm64/include/asm/cputype.h | 6 +
arch/arm64/include/asm/daifflags.h | 7 +
arch/arm64/include/asm/fixmap.h | 10 +-
arch/arm64/include/asm/io.h | 9 +-
arch/arm64/include/asm/irqflags.h | 17 +-
arch/arm64/kernel/cpufeature.c | 32 ++
arch/arm64/kernel/entry.S | 27 +-
arch/arm64/kernel/setup.c | 12 +
drivers/irqchip/Kconfig | 10 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-apple-aic.c | 501 ++++++++++++++++++
drivers/tty/serial/Kconfig | 2 +-
drivers/tty/serial/samsung_tty.c | 297 +++++++++--
.../interrupt-controller/apple-aic.h | 14 +
include/linux/serial_s3c.h | 16 +
include/uapi/linux/serial_core.h | 3 +
31 files changed, 1243 insertions(+), 49 deletions(-)
create mode 100644 Documentation/devicetree/bindings/arm/AAPL.yaml
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/AAPL,aic.yaml
create mode 100644 arch/arm64/boot/dts/apple/Makefile
create mode 100644 arch/arm64/boot/dts/apple/apple-j274.dts
create mode 100644 drivers/irqchip/irq-apple-aic.c
create mode 100644 include/dt-bindings/interrupt-controller/apple-aic.h
--
2.30.0
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 <[email protected]>
---
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/
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 <[email protected]>
+ */
+
+/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";
+ };
+
+ };
+};
--
2.30.0
This is the root interrupt controller used on Apple ARM SoCs such as the
M1. This irqchip driver performs multiple functions:
* Discriminates between IRQs and FIQs
* Drives the AIC peripheral itself (which handles IRQs)
* Dispatches FIQs to downstream hard-wired clients (currently the ARM
timer).
This patch introduces basic UP irqchip support, without SMP/IPI support.
Signed-off-by: Hector Martin <[email protected]>
---
MAINTAINERS | 1 +
drivers/irqchip/Kconfig | 10 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-apple-aic.c | 316 ++++++++++++++++++++++++++++++++
4 files changed, 328 insertions(+)
create mode 100644 drivers/irqchip/irq-apple-aic.c
diff --git a/MAINTAINERS b/MAINTAINERS
index f3d4661731c8..3a54ee5747d3 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: drivers/irqchip/irq-apple-aic.c
F: include/dt-bindings/interrupt-controller/apple-aic.h
ARM/ARTPEC MACHINE SUPPORT
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index b147f22a78f4..288c01a9abd4 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -590,4 +590,14 @@ config MST_IRQ
help
Support MStar Interrupt Controller.
+config APPLE_AIC
+ bool "Apple Interrupt Controller (AIC)"
+ depends on ARCH_APPLE || COMPILE_TEST
+ default ARCH_APPLE
+ select IRQ_DOMAIN
+ select IRQ_DOMAIN_HIERARCHY
+ help
+ Support for the Apple Interrupt Controller found on Apple Silicon SoCs,
+ such as the M1.
+
endmenu
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 0ac93bfaec61..0e2ba7c2dce7 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -113,3 +113,4 @@ obj-$(CONFIG_LOONGSON_PCH_PIC) += irq-loongson-pch-pic.o
obj-$(CONFIG_LOONGSON_PCH_MSI) += irq-loongson-pch-msi.o
obj-$(CONFIG_MST_IRQ) += irq-mst-intc.o
obj-$(CONFIG_SL28CPLD_INTC) += irq-sl28cpld.o
+obj-$(CONFIG_APPLE_AIC) += irq-apple-aic.o
diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple-aic.c
new file mode 100644
index 000000000000..533e3ce9f432
--- /dev/null
+++ b/drivers/irqchip/irq-apple-aic.c
@@ -0,0 +1,316 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright 2021 Hector Martin <[email protected]>
+ *
+ * Based on irq-lpc32xx:
+ * Copyright 2015-2016 Vladimir Zapolskiy <[email protected]>
+ * Based on irq-bcm2836:
+ * Copyright 2015 Broadcom
+ */
+
+/*
+ * AIC is a fairly simple interrupt controller with the following features:
+ *
+ * - 896 level-triggered hardware IRQs
+ * - Single mask bit per IRQ
+ * - Per-IRQ affinity setting
+ * - Automatic masking on event delivery (auto-ack)
+ * - Software triggering (ORed with hw line)
+ * - 2 per-CPU IPIs (meant as "self" and "other", but they are interchangeable if not symmetric)
+ * - Automatic prioritization (single event/ack register per CPU, lower IRQs = higher priority)
+ * - Automatic masking on ack
+ * - Default "this CPU" register view and explicit per-CPU views
+ *
+ * In addition, this driver also handles FIQs, as these are routed to the same IRQ vector. These
+ * are used for Fast IPIs (TODO) and the ARMv8 timer IRQs.
+ *
+ * Implementation notes:
+ *
+ * - This driver creates one IRQ domain for HW IRQs and the timer FIQs
+ * - FIQ hwirq numbers are assigned after true hwirqs, and are per-cpu
+ * - DT bindings use 3-cell form (like GIC):
+ * - <0 nr flags> - hwirq #nr
+ * - <1 nr flags> - FIQ #nr
+ * - nr=0 physical timer
+ * - nr=1 virtual timer
+ * - <2 nr flags> - IPI #nr
+ * - nr=0 other IPI
+ * - nr=1 self IPI
+ *
+ */
+
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
+#include <linux/io.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+#include <asm/exception.h>
+
+#include <dt-bindings/interrupt-controller/apple-aic.h>
+
+#define AIC_INFO 0x0004
+#define AIC_INFO_NR_HW(i) ((i) & 0x0000ffff)
+
+#define AIC_CONFIG 0x0010
+
+#define AIC_WHOAMI 0x2000
+#define AIC_EVENT 0x2004
+
+#define AIC_EVENT_TYPE_HW 1
+#define AIC_EVENT_TYPE_IPI 4
+#define AIC_EVENT_IPI_OTHER 1
+#define AIC_EVENT_IPI_SELF 2
+
+#define AIC_IPI_SEND 0x2008
+#define AIC_IPI_ACK 0x200c
+#define AIC_IPI_MASK_SET 0x2024
+#define AIC_IPI_MASK_CLR 0x2028
+
+#define AIC_IPI_SEND_CPU(cpu) BIT(cpu)
+
+#define AIC_IPI_OTHER BIT(0)
+#define AIC_IPI_SELF BIT(31)
+
+#define AIC_TARGET_CPU 0x3000
+#define AIC_SW_SET 0x4000
+#define AIC_SW_CLR 0x4080
+#define AIC_MASK_SET 0x4100
+#define AIC_MASK_CLR 0x4180
+
+#define AIC_CPU_IPI_SET(cpu) (0x5008 + (cpu << 7))
+#define AIC_CPU_IPI_CLR(cpu) (0x500c + (cpu << 7))
+#define AIC_CPU_IPI_MASK_SET(cpu) (0x5024 + (cpu << 7))
+#define AIC_CPU_IPI_MASK_CLR(cpu) (0x5028 + (cpu << 7))
+
+#define MASK_REG(x) (4 * ((x) >> 5))
+#define MASK_BIT(x) BIT((x) & 0x1f)
+
+#define AIC_NR_FIQ 2
+#define AIC_NR_IPI 2
+
+/*
+ * Max 31 bits in IPI SEND register (top bit is self).
+ * >=32-core chips will need code changes anyway.
+ */
+#define AIC_MAX_CPUS 31
+
+struct aic_irq_chip {
+ void __iomem *base;
+ struct irq_domain *hw_domain;
+ int nr_hw;
+};
+
+static struct aic_irq_chip *aic_irqc;
+
+static inline u32 aic_ic_read(struct aic_irq_chip *ic, u32 reg)
+{
+ return readl(ic->base + reg);
+}
+
+static inline void aic_ic_write(struct aic_irq_chip *ic, u32 reg, u32 val)
+{
+ writel(val, ic->base + reg);
+}
+
+/* These functions do nothing for FIQs, because they have no masks */
+static void aic_irq_mask(struct irq_data *d)
+{
+ struct aic_irq_chip *ic = irq_data_get_irq_chip_data(d);
+
+ if (d->hwirq < ic->nr_hw)
+ aic_ic_write(ic, AIC_MASK_SET + MASK_REG(d->hwirq),
+ MASK_BIT(d->hwirq));
+}
+
+static void aic_irq_unmask(struct irq_data *d)
+{
+ struct aic_irq_chip *ic = irq_data_get_irq_chip_data(d);
+
+ if (d->hwirq < ic->nr_hw)
+ aic_ic_write(ic, AIC_MASK_CLR + MASK_REG(d->hwirq),
+ MASK_BIT(d->hwirq));
+}
+
+static void aic_irq_eoi(struct irq_data *d)
+{
+ /*
+ * Reading the interrupt reason automatically acknowledges and masks
+ * the IRQ, so we just unmask it here if needed.
+ */
+ if (!irqd_irq_disabled(d) && !irqd_irq_masked(d))
+ aic_irq_unmask(d);
+}
+
+static void aic_handle_irq(struct pt_regs *regs)
+{
+ struct aic_irq_chip *ic = aic_irqc;
+ u32 event = aic_ic_read(ic, AIC_EVENT);
+
+ while (event) {
+ u32 type = event >> 16, irq = event & 0xffff;
+
+ /* AIC_EVENT is read-sensitive, ensure it happens before we proceed */
+ isb();
+
+ if (type == AIC_EVENT_TYPE_HW) {
+ handle_domain_irq(aic_irqc->hw_domain, irq, regs);
+ } else if (type == AIC_EVENT_TYPE_IPI) {
+ handle_domain_irq(aic_irqc->hw_domain,
+ ic->nr_hw + AIC_NR_FIQ + irq - 1, regs);
+ } else {
+ pr_err("spurious IRQ event %d, %d\n", type, irq);
+ }
+
+ event = aic_ic_read(ic, AIC_EVENT);
+ }
+}
+
+#define TIMER_FIRING(x) \
+ (((x) & (ARCH_TIMER_CTRL_ENABLE | ARCH_TIMER_CTRL_IT_MASK | \
+ ARCH_TIMER_CTRL_IT_STAT)) == \
+ (ARCH_TIMER_CTRL_ENABLE | ARCH_TIMER_CTRL_IT_STAT))
+
+static void aic_handle_fiq(struct pt_regs *regs)
+{
+ /*
+ * It would be really nice to find a system register that lets us get the FIQ source
+ * state without having to peek down into clients...
+ */
+ if (TIMER_FIRING(read_sysreg(cntp_ctl_el0))) {
+ handle_domain_irq(aic_irqc->hw_domain,
+ aic_irqc->nr_hw + AIC_TMR_PHYS, regs);
+ }
+
+ if (TIMER_FIRING(read_sysreg(cntv_ctl_el0))) {
+ handle_domain_irq(aic_irqc->hw_domain,
+ aic_irqc->nr_hw + AIC_TMR_VIRT, regs);
+ }
+}
+
+static void __exception_irq_entry aic_handle_irq_or_fiq(struct pt_regs *regs)
+{
+ u64 isr = read_sysreg(isr_el1);
+
+ if (isr & PSR_F_BIT)
+ aic_handle_fiq(regs);
+
+ if (isr & PSR_I_BIT)
+ aic_handle_irq(regs);
+}
+
+static struct irq_chip aic_chip = {
+ .name = "AIC",
+ .irq_mask = aic_irq_mask,
+ .irq_unmask = aic_irq_unmask,
+ .irq_eoi = aic_irq_eoi,
+};
+
+static int aic_irq_domain_map(struct irq_domain *id, unsigned int irq,
+ irq_hw_number_t hw)
+{
+ struct aic_irq_chip *ic = id->host_data;
+
+ irq_set_chip_data(irq, ic);
+ if (hw < ic->nr_hw) {
+ irq_set_chip_and_handler(irq, &aic_chip, handle_fasteoi_irq);
+ } else {
+ irq_set_percpu_devid(irq);
+ irq_set_chip_and_handler(irq, &aic_chip,
+ handle_percpu_devid_irq);
+ }
+ irq_set_status_flags(irq, IRQ_LEVEL);
+ irq_set_noprobe(irq);
+
+ return 0;
+}
+
+static void aic_irq_domain_unmap(struct irq_domain *id, unsigned int irq)
+{
+ irq_set_chip_and_handler(irq, NULL, NULL);
+}
+
+static int aic_irq_domain_xlate(struct irq_domain *id,
+ struct device_node *ctrlr, const u32 *intspec,
+ unsigned int intsize,
+ irq_hw_number_t *out_hwirq,
+ unsigned int *out_type)
+{
+ struct aic_irq_chip *ic = id->host_data;
+
+ if (intsize != 3)
+ return -EINVAL;
+
+ if (intspec[0] == AIC_IRQ && intspec[1] < ic->nr_hw)
+ *out_hwirq = intspec[1];
+ else if (intspec[0] == AIC_FIQ && intspec[1] < AIC_NR_FIQ)
+ *out_hwirq = ic->nr_hw + intspec[1];
+ else if (intspec[0] == AIC_IPI && intspec[1] < AIC_NR_IPI)
+ *out_hwirq = ic->nr_hw + AIC_NR_FIQ + intspec[1];
+ else
+ return -EINVAL;
+
+ *out_type = intspec[2] & IRQ_TYPE_SENSE_MASK;
+
+ return 0;
+}
+
+static const struct irq_domain_ops aic_irq_domain_ops = {
+ .map = aic_irq_domain_map,
+ .unmap = aic_irq_domain_unmap,
+ .xlate = aic_irq_domain_xlate,
+};
+
+static int __init aic_of_ic_init(struct device_node *node,
+ struct device_node *parent)
+{
+ int i;
+ void __iomem *regs;
+ u32 info;
+ struct aic_irq_chip *irqc;
+
+ regs = of_iomap(node, 0);
+ if (WARN_ON(!regs))
+ return -EIO;
+
+ irqc = kzalloc(sizeof(*irqc), GFP_KERNEL);
+ if (!irqc)
+ return -ENOMEM;
+
+ aic_irqc = irqc;
+ irqc->base = regs;
+
+ info = aic_ic_read(irqc, AIC_INFO);
+ irqc->nr_hw = AIC_INFO_NR_HW(info);
+
+ irqc->hw_domain =
+ irq_domain_add_linear(node,
+ irqc->nr_hw + AIC_NR_FIQ + AIC_NR_IPI,
+ &aic_irq_domain_ops, irqc);
+ if (WARN_ON(!irqc->hw_domain)) {
+ iounmap(irqc->base);
+ kfree(irqc);
+ return -ENODEV;
+ }
+
+ irq_domain_update_bus_token(irqc->hw_domain, DOMAIN_BUS_WIRED);
+
+ set_handle_irq(aic_handle_irq_or_fiq);
+
+ for (i = 0; i < BITS_TO_LONGS(irqc->nr_hw); i++)
+ aic_ic_write(irqc, AIC_MASK_SET + i * 4, ~0);
+ for (i = 0; i < BITS_TO_LONGS(irqc->nr_hw); i++)
+ aic_ic_write(irqc, AIC_SW_CLR + i * 4, ~0);
+ for (i = 0; i < irqc->nr_hw; i++)
+ aic_ic_write(irqc, AIC_TARGET_CPU + i * 4, 1);
+
+ pr_info("AIC: initialized with %d IRQs, %d FIQs, %d IPIs\n",
+ irqc->nr_hw, AIC_NR_FIQ, AIC_NR_IPI);
+
+ return 0;
+}
+
+IRQCHIP_DECLARE(apple_m1_aic, "AAPL,aic", aic_of_ic_init);
--
2.30.0
On 05/02/2021 06.29, Arnd Bergmann wrote:
> On Thu, Feb 4, 2021 at 9:39 PM Hector Martin <[email protected]> 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.
--
Hector Martin "marcan" ([email protected])
Public Key: https://mrcn.st/pub
Hi Hector,
I love your patch! Yet something to improve:
[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on robh/for-next linus/master v5.11-rc6 next-20210125]
[cannot apply to tip/irq/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Hector-Martin/Apple-M1-SoC-platform-bring-up/20210205-045228
base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: arc-allyesconfig (attached as .config)
compiler: arceb-elf-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/01ec72727a358a6f4208bf56808f1ce57a251413
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Hector-Martin/Apple-M1-SoC-platform-bring-up/20210205-045228
git checkout 01ec72727a358a6f4208bf56808f1ce57a251413
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
>> drivers/irqchip/irq-apple-aic.c:51:10: fatal error: asm/exception.h: No such file or directory
51 | #include <asm/exception.h>
| ^~~~~~~~~~~~~~~~~
compilation terminated.
vim +51 drivers/irqchip/irq-apple-aic.c
43
44 #include <linux/io.h>
45 #include <linux/irqchip.h>
46 #include <linux/irqchip/chained_irq.h>
47 #include <linux/of_address.h>
48 #include <linux/of_irq.h>
49 #include <linux/of_platform.h>
50 #include <linux/slab.h>
> 51 #include <asm/exception.h>
52
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
On 05/02/2021 08.08, Arnd Bergmann wrote:
> On Thu, Feb 4, 2021 at 10:44 PM Hector Martin 'marcan' <[email protected]> wrote:
>> On 05/02/2021 06.29, Arnd Bergmann wrote:
>>> On Thu, Feb 4, 2021 at 9:39 PM Hector Martin <[email protected]> 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 :)
--
Hector Martin "marcan" ([email protected])
Public Key: https://mrcn.st/pub
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 <[email protected]>
> ---
> 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 <[email protected]>
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
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 <[email protected]>
>
> 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!
--
Hector Martin "marcan" ([email protected])
Public Key: https://mrcn.st/pub
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 <[email protected]>
> >
> > 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
On 08/02/2021 21.40, Arnd Bergmann wrote:
> On Mon, Feb 8, 2021 at 1:13 PM Krzysztof Kozlowski <[email protected]> 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 ([email protected])
Public Key: https://mrcn.st/pub
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...?
--
Hector Martin ([email protected])
Public Key: https://mrcn.st/pub
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 <[email protected]> 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/
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 <[email protected]>
> >
> > 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
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 ([email protected])
Public Key: https://mrcn.st/pub
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).
--
Hector Martin ([email protected])
Public Key: https://mrcn.st/pub
On 08/02/2021 18.25, Marc Zyngier wrote:
> I really do not want to expose IPIs in the DT. The OS defines what
> IPIs are used for, not the firmware/HW. No other platform requires
> this either, so is there any reason to do so?
This is used internally by the chained IPI driver (patch #16), but it
does not need to be ever used in the DT. I guess it would be appropriate
to just not document it in the bindings, and also use a higher type than
2 (e.g. 0xff), so that if we ever have to add another type to the
binding (e.g. the timer on older SoCs) it doesn't have to skip the
number 2 to avoid breaking compat between newer DTs and older drivers.
See irq-bcm2836.c for the same approach: a chained IPI controller using
an otherwise undocumented IRQ binding.
Another approach is to do what irq-armada-370-xp.c does: just ditch the
chained irqchip and call handle_domain_irq into the IPI domain directly
from the main IRQ handler function.
>> +#include <linux/irqchip/chained_irq.h>
>
> There isn't any chained interrupt controller here, AFAICT.
This goes with patch #16, I'll move it there.
> If these functions have no impact on the per-CPU interrupts, then
> maybe these interrupts should be given a different irqchip.
Same IRQ domain, different irqchip? That sounds reasonable and gets rid
of the bounds check on the mask/unmask calls, I'll do it for v2. This
chip would apply for both IPIs (where a different register set in AIC
for masking/unmasking applies, but that is handled at the chained
irqchip level in #16) and FIQs (which have no masking).
>> +static void aic_irq_eoi(struct irq_data *d)
>> +{
>> + /*
>> + * Reading the interrupt reason automatically acknowledges and masks
>> + * the IRQ, so we just unmask it here if needed.
>> + */
>> + if (!irqd_irq_disabled(d) && !irqd_irq_masked(d))
>> + aic_irq_unmask(d);
>
> This doesn't apply to per-CPU interrupts, right? Or does it?
The auto-masking does apply to IPIs, but this code doesn't do the
unmasking. That is handled in the chained IPI domain in #16
*Strictly speaking* if we separate the responsibility at AIC for the
root handler and say the chained handler should purely be a multiplexer
for IPIs that doesn't touch the hardware at all, then the
masking/unmasking should move here (into another irqchip) and the IPI
domain code should just call into the root domain to mask/unmask the
sole used hardware IPI; the current approach is a minor layering
violation but... I'm not sure how useful it is to keep the layering
pristine when both drivers live in the same file and are instantiated
together anyway.
If I switch to the irq-armada-370-xp.c model where there is no logical
chaining, then this would be fine as-is as both domains would logically
represent driving different parts of AIC in parallel, with no nesting
relationship.
>> + u32 type = event >> 16, irq = event & 0xffff;
>
> Nit: please consider introducing masks and using the bitfield macros
> to extract the various fields.
Ack, will do.
>> + /* AIC_EVENT is read-sensitive, ensure it happens before we proceed */
>> + isb();
>
> You seem to have a data dependency after this, so I can't see how the
> ISB influences the read from AIC_EVENT. However you need to order it
> with the read from the timer registers, and I believe it'd be better
> to move the barrier there.
(Keeping the barrier story in the other thread)
>> + if (type == AIC_EVENT_TYPE_HW) {
>> + handle_domain_irq(aic_irqc->hw_domain, irq, regs);
>> + } else if (type == AIC_EVENT_TYPE_IPI) {
>> + handle_domain_irq(aic_irqc->hw_domain,
>> + ic->nr_hw + AIC_NR_FIQ + irq - 1, regs);
>
> nit: it would be slightly less cumbersome to compute the hwirq in a
> switch, and have a single call to handle_domain_irq().
>
> I also wonder whether using two top-level domains would be better. Not
> a big deal though.
Exactly :) I can certainly switch to that if you have no objection. It
should have lower overhead for IPIs anyway, and removes the fwspec step
to glue it all together.
>> + } else {
>> + pr_err("spurious IRQ event %d, %d\n", type, irq);
>
> Spurious interrupts aren't an error, in general. If you really want to
> keep this, at the very least make it rate-limited.
In this case it's more like "unknown IRQ event", which better be an
error because it means the chip is doing something we don't know about -
*except* the zero case, that's just a spurious IRQ which indeed isn't an
error (peripheral asserted and deasserted IRQ before we could handle it;
I need to check but I believe AIC would just withdraw the event in that
case).
So the zero case should be ignored and the unknown case should be fine
without rate limiting, because it really shouldn't happen, ever. I'll
fix the zero case for v2.
> Consider turning the whole thing into a do{}while() so that there is
> only a single read of AIC_EVENT in the function.
Ack.
>> + /*
>> + * It would be really nice to find a system register that lets us get the FIQ source
>> + * state without having to peek down into clients...
>> + */
>
> nit: please try to keep comments within the 80 cols limit. I don't
> mind code being wider, but comments benefit from being more rigorously
> structured.
Ack, I'll keep it in mind. For this one I just used clang-format as a
first-pass and made some minor changes, so please do point out any other
style nits I missed so I can keep it in check. I know it doesn't enforce
nor fully represent kernel style.
> And yes, having to poll each end-point IP is really a drag. How does
> the PMU work on this system? Is there any other per-CPU source?
PMU also ends up in FIQ, and it's nonstandard (not the ARM Performance
Monitors extension). That means one more FIQ source is going to have to
end up here, and one more downstream register to poll (a proprietary one
in this case: SYS_PMCR0 is s3_1_c15_c0_0, not to be confused with the
standard SYS_PMCR_EL0 which is completely different).
So the FIQ sources to be polled are the following:
1. HV timers
2. Guest timers (auto-masked, mask register TBD, I still have no idea
how XNU routes these to the hypervisor framework... haven't found the
code yet, or the mask register)
3. Fast IPIs (not currently implemented)
4 Apple PMU
We have #1, I'll look for the mask bits to properly implement #2, #3 can
wait until we actually implement those, and #4 can wait until we
implement that PMU. Does that sound OK?
If you think it's worth it, I could at least check the status registers
for #3 and #4 and yell loudly, so that if we somehow end up with a FIQ
storm because those paths went off unchecked, at least we have logs. Or
just make sure to mask them in the AIC init code, or both.
Since these are per-CPU, setting the masks is a per-cpu call, so that
needs to go via something like cpuhp_setup_state_nocalls to run on CPU
bring-up.
That said: PMC seems to have IRQ settings to go via AIC instead of FIQ,
but I have no idea if that works on these CPUs; XNU only uses it on
older ones. That should probably be investigated later.
> This system runs VHE, so there is also CNT{P,V}_CTL_EL02 to consider.
> But I really wonder how the whole thing works once these two timers
> are assigned to a guest. Somehow, something must control the masking,
> otherwise you wouldn't be able to enter a guest with a timer firing.
Yeah, as I mentioned on IRC, there is auto-masking for the guest timers,
somehow. I'll find that mask bit.
> It also means that there is no way to have threaded per-CPU
> interrupts, which means no Preempt-RT. You could wire the mask/unmask
> callbacks to mess with the IMASK bit in individual timers, but that
> doesn't solve the problem for guests.
For HV timers, we're probably have to mess with IMASK here if there is
no other way... I need to read through the timer code and make sure that
wouldn't confuse it, as that creates a bit of an implicit contract here.
It'd be great if I can find a true status/mask register for FIQs, but
I'm not holding my breath that it exists...
> Are all interrupts level? How are MSIs implemented?
Seems a fixed set of MSIs are routed into AIC, presumably transformed
into level lines? I need to look into this part in more detail.
>> + irqc->hw_domain =
>> + irq_domain_add_linear(node,
>> + irqc->nr_hw + AIC_NR_FIQ + AIC_NR_IPI,
>> + &aic_irq_domain_ops, irqc);
>
> Please keep assignments on a single line.
I think this one was one of those clang-format things :-).
I'll fix it and watch out for similar things.
>> + for (i = 0; i < BITS_TO_LONGS(irqc->nr_hw); i++)
>
> long is 64bit on arm64, so this loop is unlikely to do what you
> want. Consider using BITS_TO_U32.
Ha, nice catch. Thanks!
--
Hector Martin ([email protected])
Public Key: https://mrcn.st/pub
* Hector Martin 'marcan' <[email protected]> [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
On 10/02/2021 19.19, Tony Lindgren wrote:
> * Hector Martin 'marcan' <[email protected]> [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.
--
Hector Martin ([email protected])
Public Key: https://mrcn.st/pub
* Hector Martin <[email protected]> [210210 11:14]:
> On 10/02/2021 19.19, Tony Lindgren wrote:
> > * Hector Martin 'marcan' <[email protected]> [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
On 10/02/2021 20.34, Tony Lindgren wrote:
> * Hector Martin <[email protected]> [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)
--
Hector Martin ([email protected])
Public Key: https://mrcn.st/pub
Hi Hector,
On Wed, 10 Feb 2021 at 20:49, Hector Martin <[email protected]> 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
* Daniel Palmer <[email protected]> [210210 12:24]:
> Hi Hector,
>
> On Wed, 10 Feb 2021 at 20:49, Hector Martin <[email protected]> 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
On Wed, Feb 10, 2021 at 01:34:50PM +0200, Tony Lindgren wrote:
> * Hector Martin <[email protected]> [210210 11:14]:
> > On 10/02/2021 19.19, Tony Lindgren wrote:
> > > * Hector Martin 'marcan' <[email protected]> [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
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.
--
Hector Martin ([email protected])
Public Key: https://mrcn.st/pub
* Krzysztof Kozlowski <[email protected]> [210210 12:56]:
> On Wed, Feb 10, 2021 at 01:34:50PM +0200, Tony Lindgren wrote:
> > * Hector Martin <[email protected]> [210210 11:14]:
> > > On 10/02/2021 19.19, Tony Lindgren wrote:
> > > > * Hector Martin 'marcan' <[email protected]> [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
On Wed, Feb 10, 2021 at 03:19:46PM +0200, Tony Lindgren wrote:
> * Krzysztof Kozlowski <[email protected]> [210210 12:56]:
> > On Wed, Feb 10, 2021 at 01:34:50PM +0200, Tony Lindgren wrote:
> > > * Hector Martin <[email protected]> [210210 11:14]:
> > > > On 10/02/2021 19.19, Tony Lindgren wrote:
> > > > > * Hector Martin 'marcan' <[email protected]> [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