From: Hector Martin <hidden> Date: 2021-02-04 20:41:37
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Hector Martin <hidden> Date: 2021-02-04 20:41:58
Amusingly, this wasn't yet documented, even though this vendor prefix
has been used since time immemorial on PPC.
Signed-off-by: Hector Martin <redacted>
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
1 file changed, 2 insertions(+)
From: Hector Martin <hidden> Date: 2021-02-04 20:42:30
This adds a Kconfig option to toggle support for Apple ARM SoCs.
At this time this targets the M1 and later "Apple Silicon" Mac SoCs.
Signed-off-by: Hector Martin <redacted>
---
arch/arm64/Kconfig.platforms | 7 +++++++
1 file changed, 7 insertions(+)
From: Hector Martin <hidden> Date: 2021-02-04 20:42:30
Apple SoCs are a distant descendant of Samsung designs and use yet
another variant of their UART style, with different interrupt handling.
In particular, this variant has the following differences with existing
ones:
* It includes a built-in interrupt controller with different registers,
using only a single platform IRQ
* Internal interrupt sources are treated as edge-triggered, even though
the IRQ output is level-triggered. This chiefly affects the TX IRQ
path: the driver can no longer rely on the TX buffer empty IRQ
immediately firing after TX is enabled, but instead must prime the
FIFO with data directly.
Signed-off-by: Hector Martin <redacted>
---
drivers/tty/serial/samsung_tty.c | 297 +++++++++++++++++++++++++++----
include/linux/serial_s3c.h | 16 ++
include/uapi/linux/serial_core.h | 3 +
3 files changed, 280 insertions(+), 36 deletions(-)
@@ -56,6 +56,9 @@/* flag to ignore all characters coming in */#define RXSTAT_DUMMY_READ (0x10000000)+/* IRQ number used when the handler is called in non-IRQ context */+#define NO_IRQ -1+structs3c24xx_uart_info{char*name;unsignedinttype;
@@ -651,14 +706,18 @@ static void enable_rx_pio(struct s3c24xx_uart_port *ourport)/* set Rx mode to DMA mode */ucon=rd_regl(port,S3C2410_UCON);-ucon&=~(S3C64XX_UCON_TIMEOUT_MASK|-S3C64XX_UCON_EMPTYINT_EN|-S3C64XX_UCON_DMASUS_EN|-S3C64XX_UCON_TIMEOUT_EN|-S3C64XX_UCON_RXMODE_MASK);-ucon|=0xf<<S3C64XX_UCON_TIMEOUT_SHIFT|-S3C64XX_UCON_TIMEOUT_EN|-S3C64XX_UCON_RXMODE_CPU;+ucon&=~S3C64XX_UCON_RXMODE_MASK;+ucon|=S3C64XX_UCON_RXMODE_CPU;++/* Apple types use these bits for IRQ masks */+if(s3c24xx_irq_type(port)!=IRQ_APPLE){+ucon&=~(S3C64XX_UCON_TIMEOUT_MASK|+S3C64XX_UCON_EMPTYINT_EN|+S3C64XX_UCON_DMASUS_EN|+S3C64XX_UCON_TIMEOUT_EN);+ucon|=0xf<<S3C64XX_UCON_TIMEOUT_SHIFT|+S3C64XX_UCON_TIMEOUT_EN;+}wr_regl(port,S3C2410_UCON,ucon);ourport->rx_mode=S3C24XX_RX_PIO;
@@ -831,7 +890,9 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id)unsignedlongflags;intcount,dma_count=0;-spin_lock_irqsave(&port->lock,flags);+/* Only lock if called from IRQ context */+if(irq!=NO_IRQ)+spin_lock_irqsave(&port->lock,flags);count=CIRC_CNT_TO_END(xmit->head,xmit->tail,UART_XMIT_SIZE);
@@ -1106,18 +1188,34 @@ static void s3c24xx_serial_shutdown(struct uart_port *port)}if(ourport->rx_claimed){-if(!s3c24xx_serial_has_interrupt_mask(port))+if(s3c24xx_irq_type(port)==IRQ_DISCRETE)free_irq(ourport->rx_irq,ourport);ourport->rx_claimed=0;ourport->rx_enabled=0;}/* Clear pending interrupts and mask all interrupts */-if(s3c24xx_serial_has_interrupt_mask(port)){+switch(s3c24xx_irq_type(port)){+caseIRQ_APPLE:{+unsignedintucon;++ucon=rd_regl(port,S3C2410_UCON);+ucon&=~(APPLE_UCON_TXTHRESH_ENA_MSK|+APPLE_UCON_RXTHRESH_ENA_MSK|+APPLE_UCON_RXTO_ENA_MSK);+wr_regl(port,S3C2410_UCON,ucon);++wr_regl(port,S3C2410_UTRSTAT,APPLE_UTRSTAT_ALL_FLAGS);++free_irq(port->irq,ourport);+break;+}+caseIRQ_S3C6400:free_irq(port->irq,ourport);wr_regl(port,S3C64XX_UINTP,0xf);wr_regl(port,S3C64XX_UINTM,0xf);+break;}if(ourport->dma)
@@ -1215,6 +1313,47 @@ static int s3c64xx_serial_startup(struct uart_port *port)returnret;}+staticintapple_serial_startup(structuart_port*port)+{+structs3c24xx_uart_port*ourport=to_ourport(port);+unsignedlongflags;+unsignedintufcon;+intret;++wr_regl(port,S3C2410_UTRSTAT,APPLE_UTRSTAT_ALL_FLAGS);++ret=request_irq(port->irq,apple_serial_handle_irq,IRQF_SHARED,+s3c24xx_serial_portname(port),ourport);+if(ret){+dev_err(port->dev,"cannot get irq %d\n",port->irq);+returnret;+}++/* For compatibility with s3c24xx Soc's */+ourport->rx_enabled=1;+ourport->rx_claimed=1;+ourport->tx_enabled=0;+ourport->tx_claimed=1;++spin_lock_irqsave(&port->lock,flags);++ufcon=rd_regl(port,S3C2410_UFCON);+ufcon|=S3C2410_UFCON_RESETRX|S5PV210_UFCON_RXTRIG8;+if(!uart_console(port))+ufcon|=S3C2410_UFCON_RESETTX;+wr_regl(port,S3C2410_UFCON,ufcon);++enable_rx_pio(ourport);++spin_unlock_irqrestore(&port->lock,flags);++/* Enable Rx Interrupt */+s3c24xx_set_bit(port,APPLE_UCON_RXTHRESH_ENA,S3C2410_UCON);+s3c24xx_set_bit(port,APPLE_UCON_RXTO_ENA,S3C2410_UCON);++returnret;+}+/* power power management control */staticvoids3c24xx_serial_pm(structuart_port*port,unsignedintlevel,
@@ -1868,9 +2009,16 @@ static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,/* setup info for port */port->dev=&platdev->dev;+switch(s3c24xx_irq_type(port)){+/* Startup sequence is different for Apple SoC's */+caseIRQ_APPLE:+s3c24xx_serial_ops.startup=apple_serial_startup;+break;/* Startup sequence is different for s3c64xx and higher SoC's */-if(s3c24xx_serial_has_interrupt_mask(port))+caseIRQ_S3C6400:s3c24xx_serial_ops.startup=s3c64xx_serial_startup;+break;+}port->uartclk=1;
@@ -1905,7 +2053,7 @@ static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,ourport->tx_irq=ret+1;}-if(!s3c24xx_serial_has_interrupt_mask(port)){+if(s3c24xx_irq_type(port)==IRQ_DISCRETE){ret=platform_get_irq(platdev,1);if(ret>0)ourport->tx_irq=ret;
@@ -1945,10 +2093,24 @@ static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport,pr_warn("uart: failed to enable baudclk\n");/* Keep all interrupts masked and cleared */-if(s3c24xx_serial_has_interrupt_mask(port)){+switch(s3c24xx_irq_type(port)){+caseIRQ_APPLE:{+unsignedintucon;++ucon=rd_regl(port,S3C2410_UCON);+ucon&=~(APPLE_UCON_TXTHRESH_ENA_MSK|+APPLE_UCON_RXTHRESH_ENA_MSK|+APPLE_UCON_RXTO_ENA_MSK);+wr_regl(port,S3C2410_UCON,ucon);++wr_regl(port,S3C2410_UTRSTAT,APPLE_UTRSTAT_ALL_FLAGS);+break;+}+caseIRQ_S3C6400:wr_regl(port,S3C64XX_UINTM,0xf);wr_regl(port,S3C64XX_UINTP,0xf);wr_regl(port,S3C64XX_UINTSP,0xf);+break;}dev_dbg(port->dev,"port: map=%pa, mem=%p, irq=%d (%d,%d), clock=%u\n",
@@ -2694,6 +2916,9 @@ static int __init s3c2410_early_console_setup(struct earlycon_device *device,OF_EARLYCON_DECLARE(s3c2410,"samsung,s3c2410-uart",s3c2410_early_console_setup);+/* Apple SoCs are close enough to s3c2410 for earlycon */+OF_EARLYCON_DECLARE(s5l,"AAPL,s5l-uart",+s3c2410_early_console_setup);/* S3C2412, S3C2440, S3C64xx */staticstructsamsung_early_console_datas3c2440_early_console_data={
From: Hector Martin <hidden> Date: 2021-02-04 20:42:48
Apple mobile devices originally used Samsung SoCs (starting with the
S5L8900), and their current in-house SoCs continue to use compatible
UART peripherals. We'll call this UART variant AAPL,s5l-uart.
Signed-off-by: Hector Martin <redacted>
---
Documentation/devicetree/bindings/serial/samsung_uart.yaml | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Hector Martin <hidden> Date: 2021-02-04 20:43:08
Apple M1 SoCs are distant descendants of Samsung SoCs and use similar
UART blocks.
Signed-off-by: Hector Martin <redacted>
---
drivers/tty/serial/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Hector Martin <hidden> Date: 2021-02-04 20:43:31
These are not used at the moment, but let's add them for documentation
purposes.
Signed-off-by: Hector Martin <redacted>
---
arch/arm64/include/asm/cputype.h | 5 +++++
1 file changed, 5 insertions(+)
From: Hector Martin <hidden> Date: 2021-02-04 20:43:47
Apple ARM SoCs (A11 and newer) have some interrupt sources hard-wired to
the FIQ line. Introduce a cpufeature that can be used to enable FIQ
unmasking and handling via alternatives.
This is currently enabled for all Apple CPUs. If/when support is
implemented for older (pre-A11) iPhone/iPad SoCs which do not need FIQs,
or if newer SoCs are released without the FIQ requirement, we can
revisit the condition.
Signed-off-by: Hector Martin <redacted>
---
arch/arm64/Kconfig | 10 +++++++++
arch/arm64/include/asm/cpucaps.h | 3 ++-
arch/arm64/include/asm/cpufeature.h | 6 ++++++
arch/arm64/include/asm/cputype.h | 1 +
arch/arm64/kernel/cpufeature.c | 32 +++++++++++++++++++++++++++++
5 files changed, 51 insertions(+), 1 deletion(-)
@@ -1237,6 +1237,29 @@ static bool has_cache_idc(const struct arm64_cpu_capabilities *entry,returnctr&BIT(CTR_IDC_SHIFT);}+staticvoidcpu_sync_irq_to_fiq(structarm64_cpu_capabilitiesconst*cap)+{+u64daif=read_sysreg(daif);++/*+*BythispointinthebootprocessIRQsarelikelymaskedandFIOs+*aren't,soweneedtosyncthingstoavoidspuriousearlyFIQs.+*/++if(daif&PSR_I_BIT)+daif|=PSR_F_BIT;+else+daif&=~PSR_F_BIT;++write_sysreg(daif,daif);+}++staticboolneeds_fiq(conststructarm64_cpu_capabilities*entry,int__unused)+{+/* All supported Apple cores need this */+returnread_cpuid_implementor()==ARM_CPU_IMP_APPLE;+}+staticvoidcpu_emulate_effective_ctr(conststructarm64_cpu_capabilities*__unused){/*
From: Hector Martin <hidden> Date: 2021-02-04 20:44:23
This follows from the fixmap patch, but relates to the general case.
This is a hack, and incomplete. Read on for discussion.
The problem: on Apple ARM platforms, SoC MMIO needs to use nGnRnE
mappings: writes using nGnRE are blackholed. This seems to be by design,
and there doesn't seem to be any fabric configuration or other bit we
can flip to make the problem go away.
As an additional confounding factor, reportedly PCIe MMIO BAR mappings
conversely *do* need to use nGnRE to work properly. So we can't even get
away with a single ioremap setting, but need to discriminate based on
what bus the device is in. Since these devices have Thunderbolt, all PCI
devices in the tree are potentially in scope. Ugh.
Ideas:
(1) Set up some devicetree property to default to nGnRnE at the platform
level, and then make PCI drivers use nGnRE.
This will require changing the PCI code to make pci_ioremap_bar do
something other than a plain ioremap().
Unfortunately, of the ~630 PCI drivers in the tree, only ~90 use
pci_ioremap_bar(). This would require a tree-wide cleanup to
introduce something akin to pci_ioremap(), and make all PCI
drivers use it instead of naked ioremap().
Currently there are three ioremap variants:
ioremap()
ioremap_wc()
ioremap_uc() (not normally used on arm64)
None of these really capture the nGnRE vs nGnRnE distinction. If
a new variant is introduced in common code, we'd have to provide
a default implementation that falls back to regular ioremap() on
other arches. Something like ioremap() vs. ioremap_np() (nonposted)?
(2) The converse of (1): keep the nGnRE default, but introduce special
casing to the OF binding code to use nGnRnE when instructed to do so
on these platforms. This means of_iomap() needs changing.
The advantage of this approach is that the set of possible non-PCI
drivers that are useful on these SoCs is bounded, so not all drivers
that don't go through that path need to be fixed.
Additionally, this could take advantage of the OF address
translation stuff to be smarter about deciding to use nGnRnE, e.g.
doing it based on a property of the parent bus node.
Of note, some devices (like samsung_tty) go through the platform
device framework, which eventually goes into devm code. So
of_address_to_resource would need to set some flag on the struct
resource, that can then be used by both of_iomap() and
devm_ioremap_resource() and friends to eventually call the right
ioremap variant.
The ioremap considerations from (1) apply here too.
(3) Do it at a lower level, in ioremap() itself. This requires that
ioremap() somehow discriminates based on address range to pick what
kind of mapping to make.
Declaring these address ranges would be an issue. Options:
a) An out of band list in a DT node, a la /reserved-memory
b) Something based on the existing DT hierarchy, where we can scan
bus ranges and locate buses with a property that says "nGnRnE" or
"nGnRE" and dynamically build the list based on that.
The advantage of this option is that it doesn't touch non-arch code.
The disadvantage is that it adds a complete new bespoke mechanism to
the DT, and that it does not let device drivers actually select the
IO mode, which might be desirable in the future anyway for some
devices.
All discussion and additional ideas welcome.
Signed-off-by: Hector Martin <redacted>
---
arch/arm64/include/asm/io.h | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
From: Hector Martin <hidden> Date: 2021-02-04 20:44:49
AIC is the Apple Interrupt Controller found on Apple ARM SoCs, such as
the M1.
Signed-off-by: Hector Martin <redacted>
---
.../interrupt-controller/AAPL,aic.yaml | 88 +++++++++++++++++++
MAINTAINERS | 2 +
.../interrupt-controller/apple-aic.h | 14 +++
3 files changed, 104 insertions(+)
create mode 100644 Documentation/devicetree/bindings/interrupt-controller/AAPL,aic.yaml
create mode 100644 include/dt-bindings/interrupt-controller/apple-aic.h
@@ -0,0 +1,88 @@+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)+%YAML1.2+---+$id:http://devicetree.org/schemas/interrupt-controller/AAPL,aic.yaml#+$schema:http://devicetree.org/meta-schemas/core.yaml#++title:Apple Interrupt Controller++maintainers:+-Hector Martin <marcan@marcan.st>++description:|+The Apple Interrupt Controller is a simple interrupt controller present on+Apple ARM SoC platforms, including various iPhone and iPad devices and the+"AppleSilicon"M1 Macs.++It provides the following features:++-Level-triggered hardware IRQs wired to SoC blocks+-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++allOf:+-$ref:/schemas/interrupt-controller.yaml#++properties:+compatible:+contains:+enum:+-AAPL,aic+-AAPL,m1-aic++interrupt-controller:true++'#interrupt-cells':+const:3+description:|+The 1st cell contains the interrupt type:+-0:Hardware IRQ+-1:FIQ+-2:IPI++The 2nd cell contains the interrupt number.+-HW IRQs:interrupt number+-FIQs:+-0:physical timer+-1:virtual timer+-IPIs:+-0:normal/"other" IPI (used interanlly for virtual IPIs)+-1:self IPI (normally unused)++The 3rd cell contains the interrupt flags. This is normally+IRQ_TYPE_LEVEL_HIGH (4).++reg:+description:|+Specifies base physical address and size of the AIC registers.+maxItems:1++required:+-compatible+-'#interrupt-cells'+-interrupt-controller+-reg++unevaluatedProperties:false++examples:+-|+soc {+#address-cells = <2>;+#size-cells = <2>;++aic:interrupt-controller@23b100000 {+compatible = "AAPL,m1-aic", "AAPL,aic";+#interrupt-cells = <3>;+interrupt-controller;+reg = <0x2 0x3b100000 0x0 0x8000>;+};+};
From: Hector Martin <hidden> Date: 2021-02-04 20:44:50
Apple SoCs run firmware that sets up a simplefb-compatible framebuffer
for us. Add a compatible for it, and two missing supported formats.
Signed-off-by: Hector Martin <redacted>
---
.../devicetree/bindings/display/simple-framebuffer.yaml | 5 +++++
1 file changed, 5 insertions(+)
From: Hector Martin <hidden> Date: 2021-02-04 20:45:04
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 <redacted>
---
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
@@ -0,0 +1,143 @@+// SPDX-License-Identifier: GPL-2.0++/*+*Copyright2021HectorMartin<marcan@marcan.st>+*/++/dts-v1/;+#include<dt-bindings/interrupt-controller/apple-aic.h>+#include<dt-bindings/interrupt-controller/irq.h>++/{+model="Apple Mac Mini M1 2020";+compatible="AAPL,j274","AAPL,m1","AAPL,arm-platform";+#address-cells=<2>;+#size-cells=<2>;++chosen{+#address-cells=<2>;+#size-cells=<2>;+ranges;++bootargs="earlycon";+stdout-path="serial0:1500000";++framebuffer0:framebuffer@0{+compatible="AAPL,simple-framebuffer","simple-framebuffer";+reg=<0000>;// To be filled by loader+// Format properties will be added by loader+status="disabled";+};+};++memory@800000000{+device_type="memory";+reg=<0000>;// 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=<0x00x0>;+enable-method="spin-table";+cpu-release-addr=<00>;// To be filled by loader+};+cpu1:cpu@1{+compatible="AAPL,icestorm";+device_type="cpu";+reg=<0x00x1>;+enable-method="spin-table";+cpu-release-addr=<00>;// To be filled by loader+};+cpu2:cpu@2{+compatible="AAPL,icestorm";+device_type="cpu";+reg=<0x00x2>;+enable-method="spin-table";+cpu-release-addr=<00>;// To be filled by loader+};+cpu3:cpu@3{+compatible="AAPL,icestorm";+device_type="cpu";+reg=<0x00x3>;+enable-method="spin-table";+cpu-release-addr=<00>;// To be filled by loader+};+cpu4:cpu@10100{+compatible="AAPL,firestorm";+device_type="cpu";+reg=<0x00x10100>;+enable-method="spin-table";+cpu-release-addr=<00>;// To be filled by loader+};+cpu5:cpu@10101{+compatible="AAPL,firestorm";+device_type="cpu";+reg=<0x00x10101>;+enable-method="spin-table";+cpu-release-addr=<00>;// To be filled by loader+};+cpu6:cpu@10102{+compatible="AAPL,firestorm";+device_type="cpu";+reg=<0x00x10102>;+enable-method="spin-table";+cpu-release-addr=<00>;// To be filled by loader+};+cpu7:cpu@10103{+compatible="AAPL,firestorm";+device_type="cpu";+reg=<0x00x10103>;+enable-method="spin-table";+cpu-release-addr=<00>;// To be filled by loader+};+};++timer{+compatible="arm,armv8-timer";+interrupt-parent=<&aic>;+interrupts=<AIC_FIQ0IRQ_TYPE_LEVEL_HIGH>,+<AIC_FIQ0IRQ_TYPE_LEVEL_HIGH>,+<AIC_FIQ1IRQ_TYPE_LEVEL_HIGH>,+<AIC_FIQ0IRQ_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=<0x20x3b1000000x00x8000>;+};++serial0:serial@235200000{+compatible="AAPL,s5l-uart";+reg=<0x20x352000000x00x1000>;+reg-io-width=<4>;+interrupt-parent=<&aic>;+interrupts=<AIC_IRQ605IRQ_TYPE_LEVEL_HIGH>;+clocks=<&clk24>,<&clk24>;+clock-names="uart","clk_uart_baud0";+};++};+};
--
2.30.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Hector Martin <hidden> Date: 2021-02-04 20:45:42
Since the hardware IRQ controller only supports two IPIs per CPU and
Linux needs more, we implement 32 virtual IPIs using software and funnel
them through a single hardware IPI.
Signed-off-by: Hector Martin <redacted>
---
drivers/irqchip/irq-apple-aic.c | 195 +++++++++++++++++++++++++++++++-
1 file changed, 190 insertions(+), 5 deletions(-)
@@ -264,6 +299,149 @@ static const struct irq_domain_ops aic_irq_domain_ops = {.xlate=aic_irq_domain_xlate,};+staticvoidaic_ipi_mask(structirq_data*d)+{+structaic_irq_chip*ic=irq_data_get_irq_chip_data(d);+u32irq_bit=BIT(irqd_to_hwirq(d));+intthis_cpu=smp_processor_id();++atomic_and(~irq_bit,&aic_vipi_mask[this_cpu]);++if(!atomic_read(&aic_vipi_mask[this_cpu]))+aic_ic_write(ic,AIC_IPI_MASK_SET,AIC_IPI_OTHER);+}++staticvoidaic_ipi_unmask(structirq_data*d)+{+structaic_irq_chip*ic=irq_data_get_irq_chip_data(d);+u32irq_bit=BIT(irqd_to_hwirq(d));+intthis_cpu=smp_processor_id();++/* Make sure the kernel's idea of logical CPU order is the same as AIC's */+WARN_ON(aic_ic_read(ic,AIC_WHOAMI)!=this_cpu);++atomic_or(irq_bit,&aic_vipi_mask[this_cpu]);++aic_ic_write(ic,AIC_IPI_MASK_CLR,AIC_IPI_OTHER);+}++staticvoidaic_ipi_send_mask(structirq_data*d,conststructcpumask*mask)+{+structaic_irq_chip*ic=irq_data_get_irq_chip_data(d);+u32irq_bit=BIT(irqd_to_hwirq(d));+u32send=0;+intcpu;++for_each_cpu(cpu,mask){+if(atomic_read(&aic_vipi_mask[cpu])&irq_bit){+atomic_or(irq_bit,&aic_vipi_flag[cpu]);+send|=AIC_IPI_SEND_CPU(cpu);+}+}++if(send){+/*+*EnsurethatstorestoNormalmemoryarevisibletothe+*otherCPUsbeforeissuingtheIPI.+*/+wmb();+aic_ic_write(ic,AIC_IPI_SEND,send);+}+}++staticstructirq_chipipi_chip={+.name="AIC-IPI",+.irq_mask=aic_ipi_mask,+.irq_unmask=aic_ipi_unmask,+.ipi_send_mask=aic_ipi_send_mask,+};++staticvoidaic_handle_ipi(structirq_desc*desc)+{+structirq_chip*chip=irq_desc_get_chip(desc);+intthis_cpu=smp_processor_id();+inti;+unsignedlongfiring;++chained_irq_enter(chip,desc);+aic_ic_write(aic_irqc,AIC_IPI_ACK,AIC_IPI_OTHER);++firing=atomic_xchg(&aic_vipi_flag[this_cpu],0);++for_each_set_bit(i,&firing,AIC_NR_SWIPI){+generic_handle_irq(irq_find_mapping(aic_irqc->ipi_domain,i));+}++aic_ic_write(aic_irqc,AIC_IPI_MASK_CLR,AIC_IPI_OTHER);+chained_irq_exit(chip,desc);+}++staticintaic_ipi_alloc(structirq_domain*d,unsignedintvirq,+unsignedintnr_irqs,void*args)+{+inti;++for(i=0;i<nr_irqs;i++){+irq_set_percpu_devid(virq+i);+irq_domain_set_info(d,virq+i,i,&ipi_chip,d->host_data,+handle_percpu_devid_irq,NULL,NULL);+}++return0;+}++staticvoidaic_ipi_free(structirq_domain*d,unsignedintvirq,+unsignedintnr_irqs)+{+/* Not freeing IPIs */+}++staticconststructirq_domain_opsaic_ipi_domain_ops={+.alloc=aic_ipi_alloc,+.free=aic_ipi_free,+};++staticintaic_init_smp(structaic_irq_chip*irqc,structdevice_node*node)+{+structirq_fwspecipi_fwspec={+.fwnode=irqc->hw_domain->fwnode,+.param_count=3,+.param={+[0]=AIC_IPI,+[1]=0,+[2]=0,+},+};+intbase_ipi,mux_irq;++mux_irq=irq_create_fwspec_mapping(&ipi_fwspec);+if(WARN_ON(mux_irq<=0))+return-ENODEV;++irqc->ipi_domain=+irq_domain_create_linear(irqc->hw_domain->fwnode,AIC_NR_SWIPI,+&aic_ipi_domain_ops,irqc);+if(WARN_ON(!irqc->ipi_domain))+return-ENODEV;++irqc->ipi_domain->flags|=IRQ_DOMAIN_FLAG_IPI_SINGLE;+irq_domain_update_bus_token(irqc->ipi_domain,DOMAIN_BUS_IPI);++base_ipi=__irq_domain_alloc_irqs(irqc->ipi_domain,-1,AIC_NR_SWIPI,+NUMA_NO_NODE,NULL,false,NULL);++if(WARN_ON(!base_ipi)){+irq_domain_remove(irqc->ipi_domain);+return-ENODEV;+}++set_smp_ipi_range(base_ipi,AIC_NR_SWIPI);++irq_set_chained_handler_and_data(mux_irq,aic_handle_ipi,NULL);++return0;+}+staticint__initaic_of_ic_init(structdevice_node*node,structdevice_node*parent){
@@ -298,6 +476,13 @@ static int __init aic_of_ic_init(struct device_node *node,irq_domain_update_bus_token(irqc->hw_domain,DOMAIN_BUS_WIRED);+if(aic_init_smp(irqc,node)){+irq_domain_remove(irqc->hw_domain);+iounmap(irqc->base);+kfree(irqc);+return-ENODEV;+}+set_handle_irq(aic_handle_irq_or_fiq);for(i=0;i<BITS_TO_LONGS(irqc->nr_hw);i++)
@@ -307,8 +492,8 @@ static int __init aic_of_ic_init(struct device_node *node,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);+pr_info("AIC: initialized with %d IRQs, %d FIQs, %d IPIs, %d vIPIs\n",+irqc->nr_hw,AIC_NR_FIQ,AIC_NR_IPI,AIC_NR_SWIPI);return0;}
--
2.30.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Hector Martin <hidden> Date: 2021-02-04 20:46:00
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 <redacted>
---
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
From: Hector Martin <hidden> Date: 2021-02-04 20:46:01
These are the CPU cores in the "Apple Silicon" M1 SoC.
Signed-off-by: Hector Martin <redacted>
---
Documentation/devicetree/bindings/arm/cpus.yaml | 2 ++
1 file changed, 2 insertions(+)
@@ -0,0 +1,36 @@+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause+%YAML1.2+---+$id:http://devicetree.org/schemas/arm/AAPL.yaml#+$schema:http://devicetree.org/meta-schemas/core.yaml#++title:Apple ARM Machine Device Tree Bindings++maintainers:+-Hector Martin <marcan@marcan.st>++description:|+Apple ARM ("Apple Silicon") platforms should contain compatible strings+in the following format:++-AAPL,j274 (board/device ID)+-AAPL,m1 (SoC name)+-AAPL,arm-platform (used for all Apple ARM devices)++properties:+$nodename:+const:"/"+compatible:+oneOf:+-description:Apple M1 SoC based platforms+items:+-enum:+-AAPL,j274# Mac mini (M1, 2020)+-AAPL,j293# MacBook Pro (13-inch, M1, 2020)+-AAPL,j313# MacBook Air (M1, 2020)+-const:AAPL,m1+-const:AAPL,arm-platform++additionalProperties:true++...
From: Hector Martin <hidden> Date: 2021-02-04 20:46:50
This is a hack. I do not expect this to be merged as-is.
The problem: on Apple ARM platforms, SoC MMIO needs to use nGnRnE
mappings: writes using nGnRE are blackholed. This seems to be by design,
and there doesn't seem to be any fabric configuration or other bit we
can flip to make the problem go away.
Particularly tricky is that this affects earlycon, which uses fixmap,
which all gets initialized before any of the usual cpufeatures /
alternatives stuff. So we need to take care of fixmap very early.
Options I can think of:
(1) Unconditionally use nGnRnE on all platforms for fixmap IO. Maybe
this is actually fine? I suspect it might break some PCI-based
earlycons?
(2) Deal with this special case in the earlycon code, since that seems
to be the only user that matters on these platforms. Since the
IO mapping is done in earlycon.c, this will require some cooperation
with samsung_tty.c so earlycon knows when it needs to do this. Note
that doing it with DT properties will break cmdline-only earlycon
config (which otherwise works fine on this driver).
(3) This patch, but do something saner, like use a specific DT flag to
trigger this mode instead of a platform match.
Any other ideas?
Signed-off-by: Hector Martin <redacted>
---
arch/arm64/include/asm/fixmap.h | 10 +++++++++-
arch/arm64/kernel/setup.c | 12 ++++++++++++
2 files changed, 21 insertions(+), 1 deletion(-)
From: Hector Martin <hidden> Date: 2021-02-04 20:47:18
All currently supported Apple ARM SoCs (and possibly all future ones
too) require FIQs.
Signed-off-by: Hector Martin <redacted>
---
arch/arm64/Kconfig.platforms | 1 +
1 file changed, 1 insertion(+)
From: Hector Martin <hidden> Date: 2021-02-04 20:47:18
Apple SoCs (A11 and newer) have some interrupt sources hardwired to the
FIQ line. Implement support for this by simply treating IRQs and FIQs
the same way in the interrupt vectors. This is conditional on the
ARM64_NEEDS_FIQ CPU feature flag, and thus will not affect other
systems.
Root irqchip drivers can discriminate between IRQs and FIQs by checking
the ISR_EL1 system register.
Signed-off-by: Hector Martin <redacted>
---
arch/arm64/include/asm/assembler.h | 4 ++++
arch/arm64/include/asm/daifflags.h | 7 +++++++
arch/arm64/include/asm/irqflags.h | 17 +++++++++++++----
arch/arm64/kernel/entry.S | 27 +++++++++++++++++++++++----
4 files changed, 47 insertions(+), 8 deletions(-)
@@ -42,7 +42,11 @@/* IRQ is the lowest priority flag, unconditionally unmask the rest. */.macroenable_da_f+alternative_ifARM64_NEEDS_FIQ+msrdaifclr,#(8|4)+alternative_elsemsrdaifclr,#(8|4|1)+alternative_endif.endm/*
On Thu, Feb 4, 2021 at 9:39 PM Hector Martin [off-list ref] wrote:
config SERIAL_SAMSUNG
tristate "Samsung SoC serial support"
- depends on PLAT_SAMSUNG || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST
+ depends on PLAT_SAMSUNG || ARCH_S5PV210 || ARCH_EXYNOS || COMPILE_TEST || ARCH_APPLE
By convention, please keep "|| COMPILE_TEST" last in the list.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Feb 4, 2021 at 9:39 PM Hector Martin [off-list ref] 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.
Did you see the discussion on the #armlinux channel about the possibility
of moving the cpu-enable method to PSCI based on a UEFI runtime
interface?
There are a few open questions about what that would look like in the
end, but Ard has come up with a prototype for the kernel side of it
(obviously untested), which would interface either into the UEFI side
of u-boot, or a simple already-instantiated version that could be
kept inside of m1n1 and stay resident in memory.
I would like to see that model get adopted here eventually. If
we manage to get the other patches ready for an initial merge in
v5.12, we can probably start out with spin-table and move to that
in a following release though.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Feb 4, 2021 at 9:39 PM Hector Martin [off-list ref] wrote:
+/*
+ * 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
I think we should discuss the binding a bit here. My initial thinking was that
it would be better to separate the AIC from the FIQ handling, as they don't
seem to have any relation in hardware, and representing them as two
separate nodes seems like a cleaner abstraction.
+#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);
+ }
+}
This seems to be a minor layering violation to me.
One idea I had was to just keep all the fiq handling in the timer driver
itself, jumping there directly from the top-level fiq entry whenever
we are on an Apple platform. At least as long as nothing else ever
uses fiq.
When we discussed the earlier submission for the aic, I understood
that FIQ is used for both timer and IPI, but the IPI actually has another
method based on normal AIC interrupts that can be used as an
alternative.
Having the shared entry point here looks reasonable to me though, it
does seem to make a few things easier.
I wonder if there is a possible race here: if we are ever in a situation
where one of the two -- fiq or irq -- is disabled while the other one
is enabled, we could get into a state where a handler is run while
it should be masked.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Hector Martin 'marcan' <hidden> Date: 2021-02-04 21:48:23
On 05/02/2021 06.29, Arnd Bergmann wrote:
On Thu, Feb 4, 2021 at 9:39 PM Hector Martin [off-list ref] wrote:
quoted
+/ {
+ 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).
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" (marcan@marcan.st)
Public Key: https://mrcn.st/pub
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Hector Martin 'marcan' <hidden> Date: 2021-02-04 22:05:30
On 05/02/2021 06.37, Arnd Bergmann wrote:
On Thu, Feb 4, 2021 at 9:39 PM Hector Martin [off-list ref] wrote:
quoted
+ * - 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
I think we should discuss the binding a bit here. My initial thinking was that
it would be better to separate the AIC from the FIQ handling, as they don't
seem to have any relation in hardware, and representing them as two
separate nodes seems like a cleaner abstraction.
This was actually my original approach (I still have the FIQ irqchip
patch lying around), but that idea somewhat broke when we decided to
merge the vectors.
If we split it up again, one of the two still needs to be the root,
decide whether what fired is an IRQ or FIQ, and dispatch accordingly. Or
we could have three nodes and have one root handler dispatch to IRQ and
FIQ nodes, but that sounds like overkill... (?)
Are you proposing just having different drivers/nodes in the same file,
or implementing these as separate drivers in separate files?
quoted
+#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);
+ }
+}
This seems to be a minor layering violation to me.
Absolutely. Under the assumption that these IRQ lines are ORed together
into FIQ with no top-level dispatch though, there isn't a great solution
here...
I think there is a chance FIQ interrupt child bits exist *somewhere*, so
I actually plan on brute-forcing the list of implemented/valid CPU
registers and trying to see if I can find some bits that do what I want.
If it turns out they exist, this could alleviate some of the ugliness of
the current approach.
One idea I had was to just keep all the fiq handling in the timer driver
itself, jumping there directly from the top-level fiq entry whenever
we are on an Apple platform. At least as long as nothing else ever
uses fiq.
In principle, as long as the timer handler only ever uses one IRQ (which
I think is the case here, it just picks one of the 4, usually the
physical timer, and it should only enable that one) it would work. But
we still need *some* IRQ chip driver to deliver that, unless we want to
throw a bunch of special-case code into the timer driver to hook
directly into FIQs without an interrupt parent which... seems like it
could get quite messy.
When we discussed the earlier submission for the aic, I understood
that FIQ is used for both timer and IPI, but the IPI actually has another
method based on normal AIC interrupts that can be used as an
alternative.
Correct, there are two parallel IPI implementations. It is my
understanding that the CPU register based one, which ties into FIQ, is
faster / more featureful (it has deferred IPIs, not sure if the plain
AIC does those), as it is built into the core complexes instead of being
part of the external AIC block. I could try benchmarking it within m1n1
and see if I can find out how much faster it is.
I think it's worth thinking about supporting that IPI mechanism, which
would necessitate dispatching FIQs too, so hard-coding it to route
straight to the timer doesn't sound like a very future-proof plan...
consider that Apple might put out a SoC in the future that rips out the
AIC IPIs and leaves only the FIQ ones too.
Having the shared entry point here looks reasonable to me though, it
does seem to make a few things easier.
I wonder if there is a possible race here: if we are ever in a situation
where one of the two -- fiq or irq -- is disabled while the other one
is enabled, we could get into a state where a handler is run while
it should be masked.
That's a good point. We could filter with the SPSR_ELx mask bits here.
Though the FIQ support patch tries pretty hard to keep the mask bits in
sync after early boot, so this concern might be somewhat academic. I'm
happy to implement it if you think it might help though.
--
Hector Martin "marcan" (marcan@marcan.st)
Public Key: https://mrcn.st/pub
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Feb 4, 2021 at 9:39 PM Hector Martin [off-list ref] wrote:
Currently there are three ioremap variants:
ioremap()
ioremap_wc()
ioremap_uc() (not normally used on arm64)
None of these really capture the nGnRE vs nGnRnE distinction. If
a new variant is introduced in common code, we'd have to provide
a default implementation that falls back to regular ioremap() on
other arches. Something like ioremap() vs. ioremap_np() (nonposted)?
The ioport_map() function could be considered a variant of nonposted
I/O, as being nonposted is a requirement for PCI I/O space. It's
a bit weird to overload it here, as I/O space has a number of other
special cases, including a limit for the total size of the address space,
and the assumption that all I/O ports are always mapped into virtual
addresses at boot time.
Are the registers that need nGnRnE all part of a well-defined
physical address range that we could pretend to be I/O space?
Also, is the actual PCI I/O space within this region?
The main advantage here would be that we could reuse the
IORESOURCE_IO bit to signify a register in this area.
Note: I don't actually think this is going to be a good solution, just
throwing it out as another alternative in case everything else ends
up being worse.
(2) The converse of (1): keep the nGnRE default, but introduce special
casing to the OF binding code to use nGnRnE when instructed to do so
on these platforms. This means of_iomap() needs changing.
It probably also means changing of_address_to_resource(),
and devm_ioremap_resource().
(3) Do it at a lower level, in ioremap() itself. This requires that
ioremap() somehow discriminates based on address range to pick what
kind of mapping to make.
Declaring these address ranges would be an issue. Options:
a) An out of band list in a DT node, a la /reserved-memory
b) Something based on the existing DT hierarchy, where we can scan
bus ranges and locate buses with a property that says "nGnRnE" or
"nGnRE" and dynamically build the list based on that.
The advantage of this option is that it doesn't touch non-arch code.
The disadvantage is that it adds a complete new bespoke mechanism to
the DT, and that it does not let device drivers actually select the
IO mode, which might be desirable in the future anyway for some
devices.
All discussion and additional ideas welcome.
A very simple but ugly hack would be to take one of the high address
bits in phys_addr_t to encode the type, and then pass that through
all the way into the ioremap implementation.
This does have some precedent with the upper bits of the (96-bit)
PCI addresses encoding the type of resource, but it also conflates
the DT representation with the arm64 kernel implementation and
requires extending both in a fairly generic way to do something that
is highly platform specific.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Feb 4, 2021 at 9:39 PM Hector Martin [off-list ref] wrote:
This is a hack. I do not expect this to be merged as-is.
The problem: on Apple ARM platforms, SoC MMIO needs to use nGnRnE
mappings: writes using nGnRE are blackholed. This seems to be by design,
and there doesn't seem to be any fabric configuration or other bit we
can flip to make the problem go away.
Particularly tricky is that this affects earlycon, which uses fixmap,
which all gets initialized before any of the usual cpufeatures /
alternatives stuff. So we need to take care of fixmap very early.
Options I can think of:
(1) Unconditionally use nGnRnE on all platforms for fixmap IO. Maybe
this is actually fine? I suspect it might break some PCI-based
earlycons?
(2) Deal with this special case in the earlycon code, since that seems
to be the only user that matters on these platforms. Since the
IO mapping is done in earlycon.c, this will require some cooperation
with samsung_tty.c so earlycon knows when it needs to do this. Note
that doing it with DT properties will break cmdline-only earlycon
config (which otherwise works fine on this driver).
(3) This patch, but do something saner, like use a specific DT flag to
trigger this mode instead of a platform match.
Any other ideas?
I think we should decide the approach for the normal ioremap()
path first. Once we have solved that, there is likely an obvious
answer for this one as well.
My feeling is that we'll end up with (2).
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Feb 4, 2021 at 9:39 PM Hector Martin [off-list ref] wrote:
This series brings up initial support for the Apple M1 SoC, used in the
2020 Mac Mini, MacBook Pro, and MacBook Air models.
I like this a lot overall, great work!
As you probably know, the timing would be really tight for a merge
in 5.12, but if we can work out a solution for the FIQ and MMIO
problems quickly and you get Ack's from the relevant maintainers
(arch/arm64, serial, irqchip), I'd be happy to merge all of it through
the soc tree even fairly late this cycle.
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
Side note about simplefb: Ard mentioned on IRC that there is
earlycon support in efifb but not in simplefb. Once we have
a u-boot based boot flow, it would be a great help for boot-time
debugging to use earlycon on the framebuffer for those that don't
have a special uart cable.
An alternative would be to rework the simplefb driver in some
form to allow earlycon without efifb. The earlycon support there
is actually fairly simple, but doing this would raise the question
of whether it can be shared between the two and possibly the
drm/kms based drivers.
- 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.
I would add here the cpu enable-method as a third point that needs to
be decided on before we can consider merging it. As I commented
in patch 18, staying with spin-table would be the easiest way to start,
but that means the risk of an incompatible DT change later on.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Feb 4, 2021 at 11:06 PM Hector Martin 'marcan' [off-list ref] wrote:
On 05/02/2021 06.37, Arnd Bergmann wrote:
quoted
On Thu, Feb 4, 2021 at 9:39 PM Hector Martin [off-list ref] wrote:
quoted
+ * - 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
I think we should discuss the binding a bit here. My initial thinking was that
it would be better to separate the AIC from the FIQ handling, as they don't
seem to have any relation in hardware, and representing them as two
separate nodes seems like a cleaner abstraction.
This was actually my original approach (I still have the FIQ irqchip
patch lying around), but that idea somewhat broke when we decided to
merge the vectors.
If we split it up again, one of the two still needs to be the root,
decide whether what fired is an IRQ or FIQ, and dispatch accordingly. Or
we could have three nodes and have one root handler dispatch to IRQ and
FIQ nodes, but that sounds like overkill... (?)
Maybe I'm misreading the low-level entry code, but my impression
was that the fiq and irq exception vectors could just be pointed to
two different root drivers from the code in kernel_ventry
Are you proposing just having different drivers/nodes in the same file,
or implementing these as separate drivers in separate files?
I was thinking of separate driver files.
quoted
quoted
+#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);
+ }
+}
This seems to be a minor layering violation to me.
Absolutely. Under the assumption that these IRQ lines are ORed together
into FIQ with no top-level dispatch though, there isn't a great solution
here...
I think there is a chance FIQ interrupt child bits exist *somewhere*, so
I actually plan on brute-forcing the list of implemented/valid CPU
registers and trying to see if I can find some bits that do what I want.
If it turns out they exist, this could alleviate some of the ugliness of
the current approach.
Right, that would of course be ideal.
quoted
One idea I had was to just keep all the fiq handling in the timer driver
itself, jumping there directly from the top-level fiq entry whenever
we are on an Apple platform. At least as long as nothing else ever
uses fiq.
In principle, as long as the timer handler only ever uses one IRQ (which
I think is the case here, it just picks one of the 4, usually the
physical timer, and it should only enable that one) it would work. But
we still need *some* IRQ chip driver to deliver that, unless we want to
throw a bunch of special-case code into the timer driver to hook
directly into FIQs without an interrupt parent which... seems like it
could get quite messy.
I looked at other architectures, and found that at least powerpc
and sparc64 have a really minimal timer tick, with their timer_interrupt()
function getting called directly from the exception vector, and
doing a minimum of accounting (irq_enter(), statistics, ...) manually.
It's a different question if we want to do that, or if there should always
be an irqchip for consistency.
quoted
When we discussed the earlier submission for the aic, I understood
that FIQ is used for both timer and IPI, but the IPI actually has another
method based on normal AIC interrupts that can be used as an
alternative.
Correct, there are two parallel IPI implementations. It is my
understanding that the CPU register based one, which ties into FIQ, is
faster / more featureful (it has deferred IPIs, not sure if the plain
AIC does those), as it is built into the core complexes instead of being
part of the external AIC block. I could try benchmarking it within m1n1
and see if I can find out how much faster it is.
Benchmarking would at least help understand why there are two.
My best guess was that this is mostly an artifact of the XNU kernel
design, where it makes sense to split the timer and IPI that
you want to be handled by Mach kernel from the device irqs
that (I guess) would be handled by the BSD kernel.
I think it's worth thinking about supporting that IPI mechanism, which
would necessitate dispatching FIQs too, so hard-coding it to route
straight to the timer doesn't sound like a very future-proof plan...
consider that Apple might put out a SoC in the future that rips out the
AIC IPIs and leaves only the FIQ ones too.
I don't think we have to pay too much attention to preparing the
code design for it, we can always change it when needed. However,
anything that impacts the DT binding here would have to be designed
to not get in the way of adding it later.
Having the shared entry point here looks reasonable to me though, it
does seem to make a few things easier.
I wonder if there is a possible race here: if we are ever in a situation
where one of the two -- fiq or irq -- is disabled while the other one
is enabled, we could get into a state where a handler is run while
it should be masked.
That's a good point. We could filter with the SPSR_ELx mask bits here.
Though the FIQ support patch tries pretty hard to keep the mask bits in
sync after early boot, so this concern might be somewhat academic. I'm
happy to implement it if you think it might help though.
It's probably fine as it is, just wanted to make sure you were not missing
something here.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Thu, Feb 4, 2021 at 10:44 PM Hector Martin 'marcan' [off-list ref] wrote:
On 05/02/2021 06.29, Arnd Bergmann wrote:
quoted
On Thu, Feb 4, 2021 at 9:39 PM Hector Martin [off-list ref] wrote:
We tend to split the dts file into one file per SoC and one for the
specific board. I guess in this case the split can be slightly different,
but it does feel better to be prepared for sharing a lot of the contents
between the different products.
In most cases, you'd want the 'aliases' and 'chosen' nodes to be
in the board specific file.
I thought about that, but wasn't sure if splitting it up at this early
stage made much sense since I'm not sure what the split should be, given
all supported hardware is the same for all 3 released devices.
I'm happy to throw the aliases/chosen nodes into board specific files if
you think that's a good starting point. Perhaps /memory too? Those
properties are filled in/patched by the bootloader anyway...
Yes, I think that would help make it more consistent with other
platforms even if we don't care too much here.
There are also DT overlays; I was wondering if we could use those to
keep the hierarchy and avoid having many duplicate trees in a
hypothetical bootloader that embeds support for a large set of hardware,
having it construct the final devicetree on the fly from SoC + a board
overlay (and possibly further levels); but I'm not sure how that ties in
with the device trees that live in the Linux tree. Do you have any
pointers about this?
We don't really have overlays in the kernel sources (yet), though it
is something that keeps coming up. For the moment, I'd just
assume you can have one .dts file for each thing you want to
support and keep the shared bits in .dtsi files.
quoted
Did you see the discussion on the #armlinux channel about the possibility
of moving the cpu-enable method to PSCI based on a UEFI runtime
interface?
...
I saw it go by but need to review it again; I've been missing too much
sleep this week :) thanks for the reminder.
I think we might want to start with spin-table for now, given that there
are no kernel changes needed anyway, but I'm happy to take the protoype
for a spin (:)) and try implementing it in m1n1.
I do think it's valuable for whatever we do, at this stage, to not
require u-boot; having that be an integral part of the boot chain is
perfectly fine in the future but right now it helps to have a simple
boot chain while we work out the early bring-up, and while u-boot grows
the required support.
60 | #define NO_IRQ -1
|
In file included from arch/arm/include/asm/hardirq.h:5,
from include/linux/hardirq.h:10,
from include/linux/interrupt.h:11,
from include/linux/serial_core.h:13,
from drivers/tty/serial/samsung_tty.c:36:
arch/arm/include/asm/irq.h:22: note: this is the location of the previous definition
22 | #define NO_IRQ ((unsigned int)(-1))
|
drivers/tty/serial/samsung_tty.c: In function 's3c24xx_serial_resume_noirq':
drivers/tty/serial/samsung_tty.c:2309:4: error: a label can only be part of a statement and a declaration is not a statement
2309 | unsigned int ucon;
| ^~~~~~~~
drivers/tty/serial/samsung_tty.c:2334:4: error: a label can only be part of a statement and a declaration is not a statement
2334 | unsigned int uintm = 0xf;
| ^~~~~~~~
vim +/NO_IRQ +60 drivers/tty/serial/samsung_tty.c
58
59 /* IRQ number used when the handler is called in non-IRQ context */
> 60 #define NO_IRQ -1
61
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
From: kernel test robot <hidden> Date: 2021-02-05 02:21:26
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: arm-defconfig (attached as .config)
compiler: arm-linux-gnueabi-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/c3bf64138f141e20577083d30e0542004c194a20
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 c3bf64138f141e20577083d30e0542004c194a20
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <redacted>
All errors (new ones prefixed by >>):
drivers/tty/serial/samsung_tty.c:60: warning: "NO_IRQ" redefined
60 | #define NO_IRQ -1
|
In file included from arch/arm/include/asm/hardirq.h:5,
from include/linux/hardirq.h:10,
from include/linux/interrupt.h:11,
from include/linux/serial_core.h:13,
from drivers/tty/serial/samsung_tty.c:36:
arch/arm/include/asm/irq.h:22: note: this is the location of the previous definition
22 | #define NO_IRQ ((unsigned int)(-1))
|
drivers/tty/serial/samsung_tty.c: In function 's3c24xx_serial_resume_noirq':
quoted
drivers/tty/serial/samsung_tty.c:2309:4: error: a label can only be part of a statement and a declaration is not a statement
2309 | unsigned int ucon;
| ^~~~~~~~
drivers/tty/serial/samsung_tty.c:2334:4: error: a label can only be part of a statement and a declaration is not a statement
2334 | unsigned int uintm = 0xf;
| ^~~~~~~~
vim +2309 drivers/tty/serial/samsung_tty.c
2299
2300 static int s3c24xx_serial_resume_noirq(struct device *dev)
2301 {
2302 struct uart_port *port = s3c24xx_dev_to_port(dev);
2303 struct s3c24xx_uart_port *ourport = to_ourport(port);
2304
2305 if (port) {
2306 /* restore IRQ mask */
2307 switch (s3c24xx_irq_type(port)) {
2308 case IRQ_APPLE:
From: Hector Martin 'marcan' <hidden> Date: 2021-02-05 07:14:49
On 05/02/2021 08.08, Arnd Bergmann wrote:
On Thu, Feb 4, 2021 at 10:44 PM Hector Martin 'marcan' [off-list ref] wrote:
quoted
On 05/02/2021 06.29, Arnd Bergmann wrote:
quoted
On Thu, Feb 4, 2021 at 9:39 PM Hector Martin [off-list ref] 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" (marcan@marcan.st)
Public Key: https://mrcn.st/pub
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Hector Martin 'marcan' <hidden> Date: 2021-02-05 07:43:14
On 05/02/2021 08.04, Arnd Bergmann wrote:
On Thu, Feb 4, 2021 at 11:06 PM Hector Martin 'marcan' [off-list ref] wrote:
quoted
If we split it up again, one of the two still needs to be the root,
decide whether what fired is an IRQ or FIQ, and dispatch accordingly. Or
we could have three nodes and have one root handler dispatch to IRQ and
FIQ nodes, but that sounds like overkill... (?)
Maybe I'm misreading the low-level entry code, but my impression
was that the fiq and irq exception vectors could just be pointed to
two different root drivers from the code in kernel_ventry
Certainly, but we'd have to introduce a fiq handler global and duplicate
the handler code; this is what was done in the previous submission, but
I seem to recall someone (Marc?) mentioned it would be cleaner to just
merge them into the single IRQ path and discriminate in the irqchip,
which is what I did here.
I can certainly go with either solution; I don't have a strong
preference here.
Advantages of split path:
* More orthogonal
Advantages of merged path:
* Minimizes common vector changes needed for a single platform
* Keeps FIQ/IRQ code common, so FIQs are less likely to be accidentally
broken by people not testing on Apple platforms.
Unclear:
* Performance. Split path runs less code, merged path has lower icache
pressure.
quoted
Are you proposing just having different drivers/nodes in the same file,
or implementing these as separate drivers in separate files?
I was thinking of separate driver files.
That's what I previously had then :)
If this is the way to go I can certainly go back to that.
I looked at other architectures, and found that at least powerpc
and sparc64 have a really minimal timer tick, with their timer_interrupt()
function getting called directly from the exception vector, and
doing a minimum of accounting (irq_enter(), statistics, ...) manually.
It's a different question if we want to do that, or if there should always
be an irqchip for consistency.
I think the issue here is that those platforms presumably have *one*
timer hard wired to a specific exception vector (e.g. on PowerPC that's
the decrementer). So, that setup is shared by all implementations in
that platform.
But on ARM64, the architectural timer is supposed to go through an
irqchip (GIC in normal platforms), it's just that here it ended up
hard-wired to FIQ - though not alone, since fast IPIs are also there, so
we can't treat it as a strict "timer vector" either.
So even if we could do this for Apple SoCs, it would be a non-standard
setup, since every other ARM64 platform puts the timer behind an
irqchip. Therefore, I think it makes sense to always go through an
irqchip, rather than introduce a bypass for these SoCs.
Also worth noting that we have at least two functional hardware timers
here (not sure if there are more, we run with HCR_EL2.E2H=1 in m1n1
which maps the EL2 timer to be the EL1 timer; I'm not yet sure if
setting that to 0 will expose extra HV timers or not) wired to the same
FIQ. I confirmed that both the virtual and physical timers function
independently in m1n1.
I did confirm there are no secure timers, which is expected given that
there is no EL3 on these chips.
Benchmarking would at least help understand why there are two.
Well, they call them "Fast IPIs" so *presumably* they are faster, but
we'll see :)
I don't think we have to pay too much attention to preparing the
code design for it, we can always change it when needed. However,
anything that impacts the DT binding here would have to be designed
to not get in the way of adding it later.
I think this shouldn't pose much of a problem, since IPIs aren't exposed
in the DT anyway. As long as we decide how we're handling IRQs vs FIQs
(one or two nodes/drivers), then either of them could take
responsibility for handling IPIs depending on the platform. We should
probably just add a "fast-ipi" property to both nodes on platforms that
support that, so that the drivers can make the decision based on it. Or
perhaps that should be done with different compatibles?
--
Hector Martin "marcan" (marcan@marcan.st)
Public Key: https://mrcn.st/pub
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Turns out arm (32) defines NO_IRQ. Replaced with NOT_IN_IRQ for v2.
--
Hector Martin "marcan" (marcan@marcan.st)
Public Key: https://mrcn.st/pub
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Hector Martin 'marcan' <hidden> Date: 2021-02-05 09:55:22
On 05/02/2021 11.27, kernel test robot wrote:
config: arc-allyesconfig (attached as .config)
This is never going to build on !ARM64 since it uses ARM64 registers, so
removing COMPILE_TEST for v2.
--
Hector Martin "marcan" (marcan@marcan.st)
Public Key: https://mrcn.st/pub
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Feb 5, 2021 at 8:41 AM Hector Martin 'marcan' [off-list ref] wrote:
On 05/02/2021 08.04, Arnd Bergmann wrote:
quoted
On Thu, Feb 4, 2021 at 11:06 PM Hector Martin 'marcan' [off-list ref] wrote:
quoted
If we split it up again, one of the two still needs to be the root,
decide whether what fired is an IRQ or FIQ, and dispatch accordingly. Or
we could have three nodes and have one root handler dispatch to IRQ and
FIQ nodes, but that sounds like overkill... (?)
Maybe I'm misreading the low-level entry code, but my impression
was that the fiq and irq exception vectors could just be pointed to
two different root drivers from the code in kernel_ventry
Certainly, but we'd have to introduce a fiq handler global and duplicate
the handler code; this is what was done in the previous submission, but
I seem to recall someone (Marc?) mentioned it would be cleaner to just
merge them into the single IRQ path and discriminate in the irqchip,
which is what I did here.
I can certainly go with either solution; I don't have a strong
preference here.
Advantages of split path:
* More orthogonal
Advantages of merged path:
* Minimizes common vector changes needed for a single platform
* Keeps FIQ/IRQ code common, so FIQs are less likely to be accidentally
broken by people not testing on Apple platforms.
Unclear:
* Performance. Split path runs less code, merged path has lower icache
pressure.
quoted
quoted
Are you proposing just having different drivers/nodes in the same file,
or implementing these as separate drivers in separate files?
I was thinking of separate driver files.
That's what I previously had then :)
If this is the way to go I can certainly go back to that.
Marc is the authority on this one. I would prefer the split code
path, in particular if we end up using the FIQ just for the timer,
but if Marc (or Thomas, or the arm64 maintainers) has another
preference, do whatever they say.
quoted
I looked at other architectures, and found that at least powerpc
and sparc64 have a really minimal timer tick, with their timer_interrupt()
function getting called directly from the exception vector, and
doing a minimum of accounting (irq_enter(), statistics, ...) manually.
It's a different question if we want to do that, or if there should always
be an irqchip for consistency.
I think the issue here is that those platforms presumably have *one*
timer hard wired to a specific exception vector (e.g. on PowerPC that's
the decrementer). So, that setup is shared by all implementations in
that platform.
But on ARM64, the architectural timer is supposed to go through an
irqchip (GIC in normal platforms), it's just that here it ended up
hard-wired to FIQ - though not alone, since fast IPIs are also there, so
we can't treat it as a strict "timer vector" either.
So even if we could do this for Apple SoCs, it would be a non-standard
setup, since every other ARM64 platform puts the timer behind an
irqchip. Therefore, I think it makes sense to always go through an
irqchip, rather than introduce a bypass for these SoCs.
Also worth noting that we have at least two functional hardware timers
here (not sure if there are more, we run with HCR_EL2.E2H=1 in m1n1
which maps the EL2 timer to be the EL1 timer; I'm not yet sure if
setting that to 0 will expose extra HV timers or not) wired to the same
FIQ. I confirmed that both the virtual and physical timers function
independently in m1n1.
I did confirm there are no secure timers, which is expected given that
there is no EL3 on these chips.
I think the simplest implementation would be for the top-level fiq
handler to default to whatever it does today (complain, I suppose),
but have the arch timer call set_handle_fiq() in order to redirect
it to a custom entry point that does the irq_enter() etc bits and
then calls arch_timer_handler_phys.
Whatever solution we pick will end up weird and ugly, so why not
pick the one that takes the least amount of code ;-)
I think this shouldn't pose much of a problem, since IPIs aren't exposed
in the DT anyway. As long as we decide how we're handling IRQs vs FIQs
(one or two nodes/drivers), then either of them could take
responsibility for handling IPIs depending on the platform. We should
probably just add a "fast-ipi" property to both nodes on platforms that
support that, so that the drivers can make the decision based on it. Or
perhaps that should be done with different compatibles?
If it gets modeled as a separate irqchip with a separate driver, I would
also use a different compatible string for the fiq. If it gets integrated
straight into the timer code, that would be a custom compatible string
for that timer.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Hector Martin 'marcan' <hidden> Date: 2021-02-05 11:38:26
On 05/02/2021 05.39, Hector Martin wrote:
This series brings up initial support for the Apple M1 SoC, used in the
2020 Mac Mini, MacBook Pro, and MacBook Air models.
Forgot to CC: a few folks involved in the previous related thread,
sorry! Adding them here, hope everyone got the series via the MLs.
v2 will be CCed to everyone else too.
--
Hector Martin "marcan" (marcan@marcan.st)
Public Key: https://mrcn.st/pub
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Fri, Feb 5, 2021 at 8:11 AM Hector Martin 'marcan' [off-list ref] wrote:
One thing I wanted to ask: is there some kind of "experimental" policy
for DT bindings? At early platform bring-up stages it seems like it
could be valuable to allow for breaking DT changes while we flesh out
the details (this is especially true of a reverse engineered platform
like this, where we don't have knowledge of all the hardware details a
priori). The dozen or so users we might have at this stage obviously
won't complain too much :)
We don't have a hard policy here, other than the obvious "never break
setups that users rely on". As you expected, this usually means that
I wouldn't complain if you change something in the initial versions
and there is a good reason for that change, it shouldn't be a problem.
An extreme example here is the omap platform that keeps making
incompatible dt changes as there is still ongoing work for removing
the remaining platform specific code in arch/arm/ and replacing it
with DT descriptions.
Once your port is fairly complete and there are users that rely on stability
of the bindings, I would expect all further changes to be compatible in
both ways, allowing old kernels with new dtbs and the other way round.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-02-06 13:17:00
Hi Hector,
On Thu, 04 Feb 2021 20:39:38 +0000,
Hector Martin [off-list ref] wrote:
quoted hunk
Apple SoCs are a distant descendant of Samsung designs and use yet
another variant of their UART style, with different interrupt handling.
In particular, this variant has the following differences with existing
ones:
* It includes a built-in interrupt controller with different registers,
using only a single platform IRQ
* Internal interrupt sources are treated as edge-triggered, even though
the IRQ output is level-triggered. This chiefly affects the TX IRQ
path: the driver can no longer rely on the TX buffer empty IRQ
immediately firing after TX is enabled, but instead must prime the
FIFO with data directly.
Signed-off-by: Hector Martin <redacted>
---
drivers/tty/serial/samsung_tty.c | 297 +++++++++++++++++++++++++++----
include/linux/serial_s3c.h | 16 ++
include/uapi/linux/serial_core.h | 3 +
3 files changed, 280 insertions(+), 36 deletions(-)
@@ -56,6 +56,9 @@/* flag to ignore all characters coming in */#define RXSTAT_DUMMY_READ (0x10000000)+/* IRQ number used when the handler is called in non-IRQ context */+#define NO_IRQ -1+structs3c24xx_uart_info{char*name;unsignedinttype;
@@ -231,11 +242,20 @@ static int s3c24xx_serial_txempty_nofifo(struct uart_port *port)/**s3c64xxandlaterSoC'sincludetheinterruptmaskandstatusregistersin*thecontrolleritself,unlikethes3c24xxSoC'swhichhavetheseregisters-*intheinterruptcontroller.Checkiftheporttypeiss3c64xxorhigher.+*intheinterruptcontroller.AppleSoCsuseadifferentflavorofmask+*andstatusregisters.ThisfunctionreturnstheIRQstyletouse.*/-staticints3c24xx_serial_has_interrupt_mask(structuart_port*port)+staticints3c24xx_irq_type(structuart_port*port){-returnto_ourport(port)->info->type==PORT_S3C6400;+switch(to_ourport(port)->info->type){+casePORT_S3C6400:+returnIRQ_S3C6400;+casePORT_APPLE:+returnIRQ_APPLE;+default:+returnIRQ_DISCRETE;+}+
nit: For ease of reviewing, it'd be good if you could split this patch
with introducing the S3C6400 and "discrete" support initially, and
only then add the new stuff.
@@ -383,16 +417,30 @@ static void enable_tx_pio(struct s3c24xx_uart_port *ourport) ucon = rd_regl(port, S3C2410_UCON); ucon &= ~(S3C64XX_UCON_TXMODE_MASK); ucon |= S3C64XX_UCON_TXMODE_CPU;- wr_regl(port, S3C2410_UCON, ucon); /* Unmask Tx interrupt */- if (s3c24xx_serial_has_interrupt_mask(port))- s3c24xx_clear_bit(port, S3C64XX_UINTM_TXD,- S3C64XX_UINTM);- else+ switch (s3c24xx_irq_type(port)) {+ case IRQ_APPLE:+ ucon |= APPLE_UCON_TXTHRESH_ENA_MSK;+ break;+ case IRQ_S3C6400:+ s3c24xx_clear_bit(port, S3C64XX_UINTM_TXD, S3C64XX_UINTM);+ break;+ default: enable_irq(ourport->tx_irq);+ break;+ }++ wr_regl(port, S3C2410_UCON, ucon); ourport->tx_mode = S3C24XX_TX_PIO;++ /*+ * The Apple version only has edge triggered TX IRQs, so we need+ * to kick off the process by sending some characters here.+ */+ if (s3c24xx_irq_type(port) == IRQ_APPLE)+ s3c24xx_serial_tx_chars(NO_IRQ, ourport);
Instead of directly calling into the handler (which has its own
problems, see below), could you just tickle the interrupt status
register to make an interrupt pending and trigger an actual interrupt?
I have no idea whether the HW supports this kind of trick though.
@@ -831,7 +890,9 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id) unsigned long flags; int count, dma_count = 0;- spin_lock_irqsave(&port->lock, flags);+ /* Only lock if called from IRQ context */+ if (irq != NO_IRQ)+ spin_lock_irqsave(&port->lock, flags);
Isn't that actually dangerous? What prevents the interrupt from firing
right in the middle of this sequence and create havoc when called from
enable_tx_pio()? I fail to see what you gain with sidestepping the
locking.
@@ -1868,9 +2009,16 @@ static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport, /* setup info for port */ port->dev = &platdev->dev;+ switch (s3c24xx_irq_type(port)) {+ /* Startup sequence is different for Apple SoC's */+ case IRQ_APPLE:+ s3c24xx_serial_ops.startup = apple_serial_startup;+ break; /* Startup sequence is different for s3c64xx and higher SoC's */- if (s3c24xx_serial_has_interrupt_mask(port))+ case IRQ_S3C6400: s3c24xx_serial_ops.startup = s3c64xx_serial_startup;+ break;+ } port->uartclk = 1;
@@ -1905,7 +2053,7 @@ static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport, ourport->tx_irq = ret + 1; }- if (!s3c24xx_serial_has_interrupt_mask(port)) {+ if (s3c24xx_irq_type(port) == IRQ_DISCRETE) { ret = platform_get_irq(platdev, 1); if (ret > 0) ourport->tx_irq = ret;
@@ -1945,10 +2093,24 @@ static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport, pr_warn("uart: failed to enable baudclk\n"); /* Keep all interrupts masked and cleared */- if (s3c24xx_serial_has_interrupt_mask(port)) {+ switch (s3c24xx_irq_type(port)) {+ case IRQ_APPLE: {+ unsigned int ucon;++ ucon = rd_regl(port, S3C2410_UCON);+ ucon &= ~(APPLE_UCON_TXTHRESH_ENA_MSK |+ APPLE_UCON_RXTHRESH_ENA_MSK |+ APPLE_UCON_RXTO_ENA_MSK);+ wr_regl(port, S3C2410_UCON, ucon);++ wr_regl(port, S3C2410_UTRSTAT, APPLE_UTRSTAT_ALL_FLAGS);+ break;+ }+ case IRQ_S3C6400: wr_regl(port, S3C64XX_UINTM, 0xf); wr_regl(port, S3C64XX_UINTP, 0xf); wr_regl(port, S3C64XX_UINTSP, 0xf);+ break; } dev_dbg(port->dev, "port: map=%pa, mem=%p, irq=%d (%d,%d), clock=%u\n",
@@ -2142,7 +2304,33 @@ static int s3c24xx_serial_resume_noirq(struct device *dev) if (port) { /* restore IRQ mask */- if (s3c24xx_serial_has_interrupt_mask(port)) {+ switch (s3c24xx_irq_type(port)) {+ case IRQ_APPLE:+ unsigned int ucon;++ clk_prepare_enable(ourport->clk);+ if (!IS_ERR(ourport->baudclk))+ clk_prepare_enable(ourport->baudclk);++ ucon = rd_regl(port, S3C2410_UCON);++ ucon &= ~(APPLE_UCON_TXTHRESH_ENA_MSK |+ APPLE_UCON_RXTHRESH_ENA_MSK |+ APPLE_UCON_RXTO_ENA_MSK);++ if (ourport->tx_enabled)+ ucon |= APPLE_UCON_TXTHRESH_ENA_MSK;+ if (ourport->rx_enabled)+ ucon |= APPLE_UCON_RXTHRESH_ENA_MSK |+ APPLE_UCON_RXTO_ENA_MSK;++ wr_regl(port, S3C2410_UCON, ucon);++ if (!IS_ERR(ourport->baudclk))+ clk_disable_unprepare(ourport->baudclk);+ clk_disable_unprepare(ourport->clk);+ break;+ case IRQ_S3C6400: unsigned int uintm = 0xf; if (ourport->tx_enabled)
@@ -2156,6 +2344,7 @@ static int s3c24xx_serial_resume_noirq(struct device *dev) if (!IS_ERR(ourport->baudclk)) clk_disable_unprepare(ourport->baudclk); clk_disable_unprepare(ourport->clk);+ break; } }
Do you actually need a new port type here? Looking at the driver
itself, it is mainly used to work out the IRQ model. Maybe introducing
a new irq_type field in the port structure would be better than
exposing this to userspace (which should see something that is exactly
the same as a S3C UART).
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-02-06 13:18:46
Hi Hector,
On Thu, 04 Feb 2021 20:39:37 +0000,
Hector Martin [off-list ref] wrote:
quoted hunk
This adds a Kconfig option to toggle support for Apple ARM SoCs.
At this time this targets the M1 and later "Apple Silicon" Mac SoCs.
Signed-off-by: Hector Martin <redacted>
---
arch/arm64/Kconfig.platforms | 7 +++++++
1 file changed, 7 insertions(+)
nit: This is better selected by the interrupt controller that relies
on the generic irqchip infrastructure.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-02-06 14:00:04
Hi Hector,
On Thu, 04 Feb 2021 20:39:41 +0000,
Hector Martin [off-list ref] wrote:
quoted hunk
Apple ARM SoCs (A11 and newer) have some interrupt sources hard-wired to
the FIQ line. Introduce a cpufeature that can be used to enable FIQ
unmasking and handling via alternatives.
This is currently enabled for all Apple CPUs. If/when support is
implemented for older (pre-A11) iPhone/iPad SoCs which do not need FIQs,
or if newer SoCs are released without the FIQ requirement, we can
revisit the condition.
Signed-off-by: Hector Martin <redacted>
---
arch/arm64/Kconfig | 10 +++++++++
arch/arm64/include/asm/cpucaps.h | 3 ++-
arch/arm64/include/asm/cpufeature.h | 6 ++++++
arch/arm64/include/asm/cputype.h | 1 +
arch/arm64/kernel/cpufeature.c | 32 +++++++++++++++++++++++++++++
5 files changed, 51 insertions(+), 1 deletion(-)
Could this happen too late? If, as explained above, we can get a FIQ
until we mask it here, what prevents something (a timer?) from kicking
and creating havoc just before the sync?
If the answer is "nothing", then it probably means that the default
behaviour should be to treat PSTATE.I and PSTATE.F as containing the
same value at all times, and not just as an afterthought when we
detect that we're on a CPU type or another.
This could expand into enabling Group-0 interrupts with GICv3 on
systems that have a single security state (such as virtual machines),
though I don't really see a good use case for it.
quoted hunk
+}
+
+static bool needs_fiq(const struct arm64_cpu_capabilities *entry, int __unused)
+{
+ /* All supported Apple cores need this */
+ return read_cpuid_implementor() == ARM_CPU_IMP_APPLE;
+}
+
static void cpu_emulate_effective_ctr(const struct arm64_cpu_capabilities *__unused)
{
/*
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-02-06 15:39:50
On Thu, 04 Feb 2021 20:39:43 +0000,
Hector Martin [off-list ref] wrote:
quoted hunk
Apple SoCs (A11 and newer) have some interrupt sources hardwired to the
FIQ line. Implement support for this by simply treating IRQs and FIQs
the same way in the interrupt vectors. This is conditional on the
ARM64_NEEDS_FIQ CPU feature flag, and thus will not affect other
systems.
Root irqchip drivers can discriminate between IRQs and FIQs by checking
the ISR_EL1 system register.
Signed-off-by: Hector Martin <redacted>
---
arch/arm64/include/asm/assembler.h | 4 ++++
arch/arm64/include/asm/daifflags.h | 7 +++++++
arch/arm64/include/asm/irqflags.h | 17 +++++++++++++----
arch/arm64/kernel/entry.S | 27 +++++++++++++++++++++++----
4 files changed, 47 insertions(+), 8 deletions(-)
@@ -42,7 +42,11 @@/* IRQ is the lowest priority flag, unconditionally unmask the rest. */.macroenable_da_f+alternative_ifARM64_NEEDS_FIQ+msrdaifclr,#(8|4)+alternative_elsemsrdaifclr,#(8|4|1)+alternative_endif
See my digression in patch 8. I really wonder what the benefit is to
treat FIQ independently of IRQ, and we might as well generalise
this. We could always panic on getting a FIQ on platforms that don't
expect one.
It'd be good to rope in the other interested parties (Mark for the
early entry code, James for RAS and SError handling).
Err... no. Please. It may be a cool hack, but that's an unmaintainable
one in the long run. If you *really* have to have a special case here,
consider using a callback instead, and generate the right instruction
directly.
It could be better to create a set of first class FIQ handlers rather
than this alternative target macro. I quickly hacked this instead,
which I find more readable.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-02-06 15:47:22
On Thu, 04 Feb 2021 20:39:44 +0000,
Hector Martin [off-list ref] wrote:
quoted hunk
All currently supported Apple ARM SoCs (and possibly all future ones
too) require FIQs.
Signed-off-by: Hector Martin <redacted>
---
arch/arm64/Kconfig.platforms | 1 +
1 file changed, 1 insertion(+)
Ah, this is what I was expecting in the previous patch. I guess the
initial ARCH_APPLE patch could be moved down the line and add all the
dependencies in one go.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
@@ -42,7 +42,11 @@/* IRQ is the lowest priority flag, unconditionally unmask the rest. */.macroenable_da_f+alternative_ifARM64_NEEDS_FIQ+msrdaifclr,#(8|4)+alternative_elsemsrdaifclr,#(8|4|1)+alternative_endif
See my digression in patch 8. I really wonder what the benefit is to
treat FIQ independently of IRQ, and we might as well generalise
this. We could always panic on getting a FIQ on platforms that don't
expect one.
It'd be good to rope in the other interested parties (Mark for the
early entry code, James for RAS and SError handling).
There might be another slightly hacky but less intrusive option
that could be done (almost) purely within the aic code:
* In the fiq handler code, check if normal interrupts were enabled
when the fiq hit. Normally they are enabled, so just proceed to
handle the timer and ipi directly
* if irq was disabled, defer the handling by doing a self-ipi
through the aic's ipi method, and handle it from there
when dealing with the next interrupt once interrupts get
enabled.
This would be similar to the soft-disable feature on powerpc, which
never actually turns off interrupts from regular kernel code but
just checks a flag in local_irq_enable that gets set when a
hardirq happened.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Hector Martin 'marcan' <hidden> Date: 2021-02-07 08:33:38
On 06/02/2021 22.58, Marc Zyngier wrote:
Hector Martin [off-list ref] wrote:
quoted
+static void cpu_sync_irq_to_fiq(struct arm64_cpu_capabilities const *cap)
+{
+ u64 daif = read_sysreg(daif);
+
+ /*
+ * By this point in the boot process IRQs are likely masked and FIOs
+ * aren't, so we need to sync things to avoid spurious early FIQs.
+ */
+
+ if (daif & PSR_I_BIT)
+ daif |= PSR_F_BIT;
+ else
+ daif &= ~PSR_F_BIT;
+
+ write_sysreg(daif, daif);
Could this happen too late? If, as explained above, we can get a FIQ
until we mask it here, what prevents something (a timer?) from kicking
and creating havoc just before the sync?
Nothing, other than timers not being enabled this early (hopefully the
bootloader doesn't leave a rogue timer running for us...)
If the answer is "nothing", then it probably means that the default
behaviour should be to treat PSTATE.I and PSTATE.F as containing the
same value at all times, and not just as an afterthought when we
detect that we're on a CPU type or another.
I thought of this too. Originally I thought PSTATE.F was always set on
other systems, and thus unmasking FIQs could cause problems if there is
a pending rogue FIQ for some reason. However, while writing this patch I
realized that as part of normal process state changes we already unmask
FIQs anyway (see DAIF_PROCCTX).
Thus, in fact, this patch actually changes things (when the cpufeature
is set) to mask FIQs in some cases where they currently aren't, and
conversely to unmask them in some cases where they currently are. But
the fact that FIQ masking is somewhat inconsistent to begin with
suggests that we should be able to mess with it without causing breakage
for other systems.
So I guess in this case it would be legitimate to just make I==F on
every system, and if something breaks it should be fixed by making
whatever is causing a rogue FIQ not do that, right?
That would leave the vector switcheroo as the only thing the cpufeature
does, which would certainly simplify a lot of the patch.
This could expand into enabling Group-0 interrupts with GICv3 on
systems that have a single security state (such as virtual machines),
though I don't really see a good use case for it.
I could see having a separate vector path opening up the door for
performance hacks for very specific use cases that want really low
latency for *one* thing (e.g. the mess the Raspberry Pi folks do to work
around that braindead USB controller's performance issues), though I
imagine there would have to be very compelling reasons to develop a
framework to do this sanely upstream.
Incidentally, I have a personal interest in real-time performance
(especially audio); once the dust settles and we have a workable kernel
for normal use I do hope to spend some time taking a deep dive into
latencies and finding RT-unfriendly code, but that's pretty far off
right now. Maybe PREEMPT_RT will even be merged by then :-) (I hope that
without SMM to screw things up on these machines they might make very
nice RT-capable boxes...)
--
Hector Martin "marcan" (marcan@marcan.st)
Public Key: https://mrcn.st/pub
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Hector Martin 'marcan' <hidden> Date: 2021-02-07 08:38:27
On 07/02/2021 01.22, Arnd Bergmann wrote:
* In the fiq handler code, check if normal interrupts were enabled
when the fiq hit. Normally they are enabled, so just proceed to
handle the timer and ipi directly
* if irq was disabled, defer the handling by doing a self-ipi
through the aic's ipi method, and handle it from there
when dealing with the next interrupt once interrupts get
enabled.
This would be similar to the soft-disable feature on powerpc, which
never actually turns off interrupts from regular kernel code but
just checks a flag in local_irq_enable that gets set when a
hardirq happened.
Case #2 seems messy. In AIC, we'd have to either:
* Disable FIQs, and hope that doesn't confuse any save/restore code
going on, then set a flag and check it in *both* the IRQ and FIQ path
since either might trigger depending on what happens next, or
* Mask the relevant timer, which we'd then need to make sure does not
confuse the timer code (Unmask it again when we fire the interrupt? But
what if the timer code intended to mask it in the interim?)
Neither sounds particularly clean to me... if we had FIQ status masking
registers this would be more reasonable, but I'm not sure I'd want the
AIC driver to mess with neither DAIF nor the timer registers. It's bad
enough that it has to read the latter already (and I hope I can find a
better way of doing that...).
Plus I don't know if the vector entry code and other scaffolding between
the vector and the AIC driver would be happy with, effectively,
recursive interrupts. This could work with a carefully controlled path
to make sure it doesn't break things, but I'm not so sure about the
current "just point FIQ and IRQ to the same place" approach here.
--
Hector Martin "marcan" (marcan@marcan.st)
Public Key: https://mrcn.st/pub
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Hector Martin 'marcan' <hidden> Date: 2021-02-07 08:48:40
On 07/02/2021 00.37, Marc Zyngier wrote:
See my digression in patch 8. I really wonder what the benefit is to
treat FIQ independently of IRQ, and we might as well generalise
this. We could always panic on getting a FIQ on platforms that don't
expect one.
It'd be good to rope in the other interested parties (Mark for the
early entry code, James for RAS and SError handling).
CCing Mark and James: TL;DR what do you think about unconditionally
keeping DAIF.I == DAIF.F, would this break other platforms with spurious
FIQs or conversely mask FIQs when we don't want to in some cases? The
FIQ vector would remain a panic except on platforms that require using
it, via an alternatives patch.
It could be better to create a set of first class FIQ handlers rather
than this alternative target macro. I quickly hacked this instead,
which I find more readable.
I think I ended up with the macro change to keep it 1:1 with IRQ, vs a
separate branch... but I didn't think of the fallthrough-with-nop trick,
neat. It is definitely is more readable. Are you OK with me pulling this
patch in for v2, with your name on it?
From: Hector Martin 'marcan' <hidden> Date: 2021-02-07 09:13:18
On 06/02/2021 22.15, Marc Zyngier wrote:
quoted
-static int s3c24xx_serial_has_interrupt_mask(struct uart_port *port)
+static int s3c24xx_irq_type(struct uart_port *port)
{
- return to_ourport(port)->info->type == PORT_S3C6400;
+ switch (to_ourport(port)->info->type) {
+ case PORT_S3C6400:
+ return IRQ_S3C6400;
+ case PORT_APPLE:
+ return IRQ_APPLE;
+ default:
+ return IRQ_DISCRETE;
+ }
+
nit: For ease of reviewing, it'd be good if you could split this patch
with introducing the S3C6400 and "discrete" support initially, and
only then add the new stuff.
Good idea, will do for v2.
quoted
+ if (s3c24xx_irq_type(port) == IRQ_APPLE)
+ s3c24xx_serial_tx_chars(NO_IRQ, ourport);
Instead of directly calling into the handler (which has its own
problems, see below), could you just tickle the interrupt status
register to make an interrupt pending and trigger an actual interrupt?
I have no idea whether the HW supports this kind of trick though.
I thought of that, but I tried really hard to find such a feature with
no success. The best I can do is unmask and trigger the *RX* timeout
interrupt which will eventually fire but... this doesn't work so well in
practice. There is no way to trigger IRQ flags directly (as those bits
are write-1-to-clear).
quoted
- spin_lock_irqsave(&port->lock, flags);
+ /* Only lock if called from IRQ context */
+ if (irq != NO_IRQ)
+ spin_lock_irqsave(&port->lock, flags);
Isn't that actually dangerous? What prevents the interrupt from firing
right in the middle of this sequence and create havoc when called from
enable_tx_pio()? I fail to see what you gain with sidestepping the
locking.
The callpath here is:
uart_start -> __uart_start -> (uart_ops.start_tx)
s3c24xx_serial_start_tx -> s3c24xx_serial_start_tx_pio -> enable_tx_pio
-> s3c24xx_serial_tx_chars
And uart_start takes the uart_port lock. None of the serial functions
take the lock because the serial core already does, but obviously the
IRQ handler needs to, *if* it's called as an IRQ handler only.
The default should be IRQ_NONE, otherwise the kernel cannot detect a
screaming spurious interrupt.
Good point, and this needs fixing in s3c64xx_serial_handle_irq too then
(which is what I based mine off of).
quoted
+ ret = request_irq(port->irq, apple_serial_handle_irq, IRQF_SHARED,
+ s3c24xx_serial_portname(port), ourport);
Why IRQF_SHARED? Do you expect any other device sharing the same line
with this UART?
This also came from s3c64xx_serial_startup and... now I wonder why that
one needs it. Maybe on some SoCs it does get shared? Certainly not for
discrete rx/tx irq chips (and indeed those don't set the flag)...
CCing Thomas, who added the S3C64xx support (and should probably review
this patch); is there a reason for IRQF_SHARED there? NB: v1 breaks the
build on arm or with CONFIG_PM_SLEEP, those will be fixed for v2.
Either way, certainly not for Apple SoCs; I'll get rid of IRQF_SHARED
for v2.
Do you actually need a new port type here? Looking at the driver
itself, it is mainly used to work out the IRQ model. Maybe introducing
a new irq_type field in the port structure would be better than
exposing this to userspace (which should see something that is exactly
the same as a S3C UART).
Well... every S3C variant already has its own port type here.
#define PORT_S3C2410 55
#define PORT_S3C2440 61
#define PORT_S3C2400 67
#define PORT_S3C2412 73
#define PORT_S3C6400 84
If we don't introduce a new one, which one should we pretend to be? :)
I agree that it might make sense to merge all of these into one, though;
I don't know what the original reason for splitting them out is. But now
that they're part of the userspace API, this might not be a good idea.
Though, unsurprisingly, some googling suggests there are zero users of
these defines in userspace.
--
Hector Martin "marcan" (marcan@marcan.st)
Public Key: https://mrcn.st/pub
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Ah, this is what I was expecting in the previous patch. I guess the
initial ARCH_APPLE patch could be moved down the line and add all the
dependencies in one go.
I was trying to introduce the Kconfig before the code that depends on
it; is it kosher to have it in the other order, looking for CONFIG_
defines that don't exist yet?
Though in this case the only user earlier in the series is the Samsung
stuff, which doesn't care about FIQs, so I can just sort things as
FIQ->ARCH_APPLE->samsung->AIC...
I'm not sure about AIC vs. ARCH_APPLE though. Right now the pattern is
that AIC depends on ARCH_APPLE and also defaults to that. But then you
can build with ARCH_APPLE and AIC disabled if you so choose, which does
result in a broken system on these machines. AIC should build without
ARCH_APPLE (as long as we're on ARM64), so we could reverse that.
--
Hector Martin "marcan" (marcan@marcan.st)
Public Key: https://mrcn.st/pub
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Hector Martin 'marcan' <hidden> Date: 2021-02-07 09:28:03
On 07/02/2021 18.12, Hector Martin 'marcan' wrote:
On 06/02/2021 22.15, Marc Zyngier wrote:
quoted
The default should be IRQ_NONE, otherwise the kernel cannot detect a
screaming spurious interrupt.
Good point, and this needs fixing in s3c64xx_serial_handle_irq too then
(which is what I based mine off of).
quoted
quoted
+ ret = request_irq(port->irq, apple_serial_handle_irq, IRQF_SHARED,
+ s3c24xx_serial_portname(port), ourport);
Why IRQF_SHARED? Do you expect any other device sharing the same line
with this UART?
This also came from s3c64xx_serial_startup and... now I wonder why that
one needs it. Maybe on some SoCs it does get shared? Certainly not for
discrete rx/tx irq chips (and indeed those don't set the flag)...
CCing Thomas, who added the S3C64xx support (and should probably review
this patch); is there a reason for IRQF_SHARED there? NB: v1 breaks the
build on arm or with CONFIG_PM_SLEEP, those will be fixed for v2.
Seems Thomas does not work for Linaro any more :)
CCing Krzysztof instead, who is the Samsung arch maintainer.
Either way, certainly not for Apple SoCs; I'll get rid of IRQF_SHARED
for v2.
On Sun, Feb 7, 2021 at 9:36 AM Hector Martin 'marcan' [off-list ref] wrote:
On 07/02/2021 01.22, Arnd Bergmann wrote:
quoted
* In the fiq handler code, check if normal interrupts were enabled
when the fiq hit. Normally they are enabled, so just proceed to
handle the timer and ipi directly
* if irq was disabled, defer the handling by doing a self-ipi
through the aic's ipi method, and handle it from there
when dealing with the next interrupt once interrupts get
enabled.
This would be similar to the soft-disable feature on powerpc, which
never actually turns off interrupts from regular kernel code but
just checks a flag in local_irq_enable that gets set when a
hardirq happened.
Case #2 seems messy. In AIC, we'd have to either:
* Disable FIQs, and hope that doesn't confuse any save/restore code
going on, then set a flag and check it in *both* the IRQ and FIQ path
since either might trigger depending on what happens next, or
* Mask the relevant timer, which we'd then need to make sure does not
confuse the timer code (Unmask it again when we fire the interrupt? But
what if the timer code intended to mask it in the interim?)
I'm not quite following here. The IRQ should be disabled the entire time
while handling that self-IPI and the timer top half code, so if we get
another FIQ while handling the timer from the IRQ path, it will lead
either yet another self-IPI or it will be ignored in case the previous timer
event has not been Acked yet. I would expect that both cases are
race-free here, the only time that the FIQ needs to be disabled is
while actually handling the FIQ. Did I miss something?
Plus I don't know if the vector entry code and other scaffolding between
the vector and the AIC driver would be happy with, effectively,
recursive interrupts. This could work with a carefully controlled path
to make sure it doesn't break things, but I'm not so sure about the
current "just point FIQ and IRQ to the same place" approach here.
If we do what I described above, the FIQ and IRQ entry would have
to be separate and only arrive in the same code path when calling
into drivers/clocksource/arm_arch_timer.c. It's not recursive there
because that part is only called when IRQ is disabled, and no IRQ
is being executed while the FIQ hits.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Hector Martin 'marcan' <hidden> Date: 2021-02-07 15:39:42
On 07/02/2021 21.25, Arnd Bergmann wrote:
On Sun, Feb 7, 2021 at 9:36 AM Hector Martin 'marcan' [off-list ref] wrote:
quoted
On 07/02/2021 01.22, Arnd Bergmann wrote:
quoted
* In the fiq handler code, check if normal interrupts were enabled
when the fiq hit. Normally they are enabled, so just proceed to
handle the timer and ipi directly
* if irq was disabled, defer the handling by doing a self-ipi
through the aic's ipi method, and handle it from there
when dealing with the next interrupt once interrupts get
enabled.
This would be similar to the soft-disable feature on powerpc, which
never actually turns off interrupts from regular kernel code but
just checks a flag in local_irq_enable that gets set when a
hardirq happened.
Case #2 seems messy. In AIC, we'd have to either:
* Disable FIQs, and hope that doesn't confuse any save/restore code
going on, then set a flag and check it in *both* the IRQ and FIQ path
since either might trigger depending on what happens next, or
* Mask the relevant timer, which we'd then need to make sure does not
confuse the timer code (Unmask it again when we fire the interrupt? But
what if the timer code intended to mask it in the interim?)
I'm not quite following here. The IRQ should be disabled the entire time
while handling that self-IPI and the timer top half code, so if we get
another FIQ while handling the timer from the IRQ path, it will lead
either yet another self-IPI or it will be ignored in case the previous timer
event has not been Acked yet. I would expect that both cases are
race-free here, the only time that the FIQ needs to be disabled is
while actually handling the FIQ. Did I miss something?
FIQs are level-triggered, and there are only two* ways of masking them
(that we know of): in the timer, or DAIF. That means that if we get a
FIQ, we *must* do one of two things: either mask it in the timer
register, or mask FIQs entirely. If we do neither of these, we get a FIQ
storm.
So if a timer FIQ fires while IRQs are disabled, and we can't call into
the timer code (because IRQs were disabled, so we need to defer handling
via the IPI), the only other options are to either poke the timer mask
bit directly, or to mask FIQs. Neither seems particularly correct.
* An exception seems to be non-HV timer interrupts firing while we have
a VM guest running (HCR_EL2.TGE=0). This causes a single FIQ, and no
more, which suggests there is a mask bit for guest timer FIQs somewhere
that gets automatically set when the FIQ is delivered to the CPU core.
I've yet to find where this bit lives, I'll be doing a brute force sweep
of system register space soon to see if I can find it, and if there is
anything else useful near it.
quoted
Plus I don't know if the vector entry code and other scaffolding between
the vector and the AIC driver would be happy with, effectively,
recursive interrupts. This could work with a carefully controlled path
to make sure it doesn't break things, but I'm not so sure about the
current "just point FIQ and IRQ to the same place" approach here.
If we do what I described above, the FIQ and IRQ entry would have
to be separate and only arrive in the same code path when calling
into drivers/clocksource/arm_arch_timer.c. It's not recursive there
because that part is only called when IRQ is disabled, and no IRQ
is being executed while the FIQ hits.
Right, that's what i'm saying; we can't re-use the IRQ handler like Marc
proposed, because I don't think that expects to be called reentrantly;
we'd have to have a separate FIQ entry, but since it can be called with
IRQs enabled and handle the FIQ in-line, it also needs to be able to
*conditionally* behave like a normal IRQ handler. This level of
complexity seems somewhat dubious, just to not maintain the FIQ mask bit
synced. That's not just AIC code any more, it needs a bespoke FIQ vector
and logic to decide whether IRQs are masked (call AIC to self-IPI
without doing the usual IRQ processing) or unmasked (go through regular
IRQ accounting and behave like an IRQ).
Perhaps I'm misunderstanding what you're proposing here or how this
would work :)
--
Hector Martin "marcan" (marcan@marcan.st)
Public Key: https://mrcn.st/pub
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Sun, Feb 7, 2021 at 4:40 PM Hector Martin 'marcan' [off-list ref] wrote:
On 07/02/2021 21.25, Arnd Bergmann wrote:
quoted
On Sun, Feb 7, 2021 at 9:36 AM Hector Martin 'marcan' [off-list ref] wrote:
quoted
On 07/02/2021 01.22, Arnd Bergmann wrote:
quoted
* In the fiq handler code, check if normal interrupts were enabled
when the fiq hit. Normally they are enabled, so just proceed to
handle the timer and ipi directly
* if irq was disabled, defer the handling by doing a self-ipi
through the aic's ipi method, and handle it from there
when dealing with the next interrupt once interrupts get
enabled.
This would be similar to the soft-disable feature on powerpc, which
never actually turns off interrupts from regular kernel code but
just checks a flag in local_irq_enable that gets set when a
hardirq happened.
Case #2 seems messy. In AIC, we'd have to either:
* Disable FIQs, and hope that doesn't confuse any save/restore code
going on, then set a flag and check it in *both* the IRQ and FIQ path
since either might trigger depending on what happens next, or
* Mask the relevant timer, which we'd then need to make sure does not
confuse the timer code (Unmask it again when we fire the interrupt? But
what if the timer code intended to mask it in the interim?)
I'm not quite following here. The IRQ should be disabled the entire time
while handling that self-IPI and the timer top half code, so if we get
another FIQ while handling the timer from the IRQ path, it will lead
either yet another self-IPI or it will be ignored in case the previous timer
event has not been Acked yet. I would expect that both cases are
race-free here, the only time that the FIQ needs to be disabled is
while actually handling the FIQ. Did I miss something?
FIQs are level-triggered, and there are only two* ways of masking them
(that we know of): in the timer, or DAIF. That means that if we get a
FIQ, we *must* do one of two things: either mask it in the timer
register, or mask FIQs entirely. If we do neither of these, we get a FIQ
storm.
So if a timer FIQ fires while IRQs are disabled, and we can't call into
the timer code (because IRQs were disabled, so we need to defer handling
via the IPI), the only other options are to either poke the timer mask
bit directly, or to mask FIQs. Neither seems particularly correct.
Ok, I had not realized the timer was level triggered. In case of the
timer, I suppose it could be either masked or acknowledged from the
fiq top-half handler when deferring to irq, but I agree that it means a
layering violation in either case.
What might still work is an approach where FIQ is normally enabled,
and local_irq_disable() leaves it on, while local_irq_enable() turns
it on regardless of the current state.
In this case, the fiq handler could run the timer function if interrupts
are enabled but just turn off fiqs when they are turned off, waiting
for the next local_irq_enable() to get us back in the state where
the handler can run. Not sure if that would buy us anything though,
or if that still requires platform specific conditionals in common code.
* An exception seems to be non-HV timer interrupts firing while we have
a VM guest running (HCR_EL2.TGE=0). This causes a single FIQ, and no
more, which suggests there is a mask bit for guest timer FIQs somewhere
that gets automatically set when the FIQ is delivered to the CPU core.
I've yet to find where this bit lives, I'll be doing a brute force sweep
of system register space soon to see if I can find it, and if there is
anything else useful near it.
Right. Maybe you can even find a bit that switches between FIQ and
IRQ mode for the timer, as that would solve the problem completely.
I think it's not that rare for irqchips to be configurable to either route
an interrupt one way or the other.
quoted
quoted
Plus I don't know if the vector entry code and other scaffolding between
the vector and the AIC driver would be happy with, effectively,
recursive interrupts. This could work with a carefully controlled path
to make sure it doesn't break things, but I'm not so sure about the
current "just point FIQ and IRQ to the same place" approach here.
If we do what I described above, the FIQ and IRQ entry would have
to be separate and only arrive in the same code path when calling
into drivers/clocksource/arm_arch_timer.c. It's not recursive there
because that part is only called when IRQ is disabled, and no IRQ
is being executed while the FIQ hits.
Right, that's what i'm saying; we can't re-use the IRQ handler like Marc
proposed, because I don't think that expects to be called reentrantly;
we'd have to have a separate FIQ entry, but since it can be called with
IRQs enabled and handle the FIQ in-line, it also needs to be able to
*conditionally* behave like a normal IRQ handler. This level of
complexity seems somewhat dubious, just to not maintain the FIQ mask bit
synced. That's not just AIC code any more, it needs a bespoke FIQ vector
and logic to decide whether IRQs are masked (call AIC to self-IPI
without doing the usual IRQ processing) or unmasked (go through regular
IRQ accounting and behave like an IRQ).
Perhaps I'm misunderstanding what you're proposing here or how this
would work :)
The way I had imagined it was to have a parallel set_handle_irq()
and set_handle_fiq() in the aic driver, which end up using the same
logic in the entry code to call into the driver. The code leading up
to that is all in assembler but isn't all that complex in the end, and
is already abstracted with macros to a large degree. For existing
machines that don't call set_handle_fiq() it could just end up in
either panic() or in WARN_ONCE() if an FIQ does happen
unexpectedly.
The aic_handle_fiq() function itself would be straightforward,
doing not much more than
if (interrupts_enabled(ptregs))
/* safe to call timer interrupt here, as interrupts are on */
handle_domain_irq(aic->domain, AIC_TIMER_IRQ, regs);
else
/* need to defer until interrupts get re-enabled */
aic_send_ipi(smp_processor_id(), TIMER_SELF_IPI);
Anyway, it's probably not worth pursuing this further if the timer
interrupt is level-triggered, as you explained above.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-02-08 09:27:23
On Thu, 04 Feb 2021 20:39:48 +0000,
Hector Martin [off-list ref] wrote:
quoted hunk
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 <redacted>
---
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
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?
Nit: please consider introducing masks and using the bitfield macros
to extract the various fields.
+
+ /* 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.
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.
Spurious interrupts aren't an error, in general. If you really want to
keep this, at the very least make it rate-limited.
+ }
+
+ event = aic_ic_read(ic, AIC_EVENT);
Consider turning the whole thing into a do{}while() so that there is
only a single read of AIC_EVENT in the function.
+ }
+}
+
+#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...
+ */
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.
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?
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.
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.
+ 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++)
long is 64bit on arm64, so this loop is unlikely to do what you
want. Consider using BITS_TO_U32.
+ 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);
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2021-02-08 09:45:12
On Sun, Feb 07, 2021 at 06:26:43PM +0900, Hector Martin 'marcan' wrote:
On 07/02/2021 18.12, Hector Martin 'marcan' wrote:
quoted
On 06/02/2021 22.15, Marc Zyngier wrote:
quoted
The default should be IRQ_NONE, otherwise the kernel cannot detect a
screaming spurious interrupt.
Good point, and this needs fixing in s3c64xx_serial_handle_irq too then
(which is what I based mine off of).
quoted
quoted
+ ret = request_irq(port->irq, apple_serial_handle_irq, IRQF_SHARED,
+ s3c24xx_serial_portname(port), ourport);
Why IRQF_SHARED? Do you expect any other device sharing the same line
with this UART?
This also came from s3c64xx_serial_startup and... now I wonder why that
one needs it. Maybe on some SoCs it does get shared? Certainly not for
discrete rx/tx irq chips (and indeed those don't set the flag)...
CCing Thomas, who added the S3C64xx support (and should probably review
this patch); is there a reason for IRQF_SHARED there? NB: v1 breaks the
build on arm or with CONFIG_PM_SLEEP, those will be fixed for v2.
Seems Thomas does not work for Linaro any more :)
CCing Krzysztof instead, who is the Samsung arch maintainer.
Please use the scripts/get_maintainers.pl to get the list of people to
Cc. The script would point necessary folks.
A different issue is that all your emails from this thread were marked
by Google as spam. I don't see any particular warning signs in the
header so it looks more of content-based match for spam.
quoted
Either way, certainly not for Apple SoCs; I'll get rid of IRQF_SHARED
for v2.
Please send a v2 after fixing issues pointed out by kbuild.
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Feb 8, 2021 at 10:25 AM Marc Zyngier [off-list ref] wrote:
On Thu, 04 Feb 2021 20:39:48 +0000, Hector Martin [off-list ref] wrote:
quoted
+{
+ return readl(ic->base + reg);
Please consider using the _relaxed accessors, as I don't think any of
these interacts with memory (apart from IPIs, of course).
MSI interrupts require serializing with DMA, so at the minimum I think there
needs to be something that ensures that DMA from device into memory
has completed before delivering the completion interrupt to a driver. This
may already be implied when the AIC is entered, but this is hard to know
without actual hardware specs.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-02-08 10:35:17
On 2021-02-07 09:12, Hector Martin 'marcan' wrote:
On 06/02/2021 22.15, Marc Zyngier wrote:
[...]
quoted
quoted
- spin_lock_irqsave(&port->lock, flags);
+ /* Only lock if called from IRQ context */
+ if (irq != NO_IRQ)
+ spin_lock_irqsave(&port->lock, flags);
Isn't that actually dangerous? What prevents the interrupt from firing
right in the middle of this sequence and create havoc when called from
enable_tx_pio()? I fail to see what you gain with sidestepping the
locking.
The callpath here is:
uart_start -> __uart_start -> (uart_ops.start_tx)
s3c24xx_serial_start_tx -> s3c24xx_serial_start_tx_pio ->
enable_tx_pio -> s3c24xx_serial_tx_chars
And uart_start takes the uart_port lock. None of the serial functions
take the lock because the serial core already does, but obviously the
IRQ handler needs to, *if* it's called as an IRQ handler only.
Right, that's the part I missed, thanks for pointing this out.
It'd probably be cleaner to move the whole s3c24xx_serial_tx_chars()
into a helper and keep the locking in the interrupt handler proper.
[...]
quoted
quoted
diff --git a/include/uapi/linux/serial_core.h
b/include/uapi/linux/serial_core.h
index 62c22045fe65..59d102b674db 100644
Do you actually need a new port type here? Looking at the driver
itself, it is mainly used to work out the IRQ model. Maybe introducing
a new irq_type field in the port structure would be better than
exposing this to userspace (which should see something that is exactly
the same as a S3C UART).
Well... every S3C variant already has its own port type here.
#define PORT_S3C2410 55
#define PORT_S3C2440 61
#define PORT_S3C2400 67
#define PORT_S3C2412 73
#define PORT_S3C6400 84
If we don't introduce a new one, which one should we pretend to be? :)
Pick one! :D
I agree that it might make sense to merge all of these into one,
though; I don't know what the original reason for splitting them out
is. But now that they're part of the userspace API, this might not be
a good idea. Though, unsurprisingly, some googling suggests there are
zero users of these defines in userspace.
I don't think we can do that, but I don't think we should keep adding
to this unless there is a very good reason. Greg would know, I expect.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2021-02-08 10:40:19
On Fri, Feb 05, 2021 at 05:39:34AM +0900, Hector Martin wrote:
quoted hunk
Amusingly, this wasn't yet documented, even though this vendor prefix
has been used since time immemorial on PPC.
Signed-off-by: Hector Martin <redacted>
---
Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
1 file changed, 2 insertions(+)
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2021-02-08 11:04:54
On Fri, Feb 05, 2021 at 05:39:38AM +0900, Hector Martin wrote:
quoted hunk
Apple SoCs are a distant descendant of Samsung designs and use yet
another variant of their UART style, with different interrupt handling.
In particular, this variant has the following differences with existing
ones:
* It includes a built-in interrupt controller with different registers,
using only a single platform IRQ
* Internal interrupt sources are treated as edge-triggered, even though
the IRQ output is level-triggered. This chiefly affects the TX IRQ
path: the driver can no longer rely on the TX buffer empty IRQ
immediately firing after TX is enabled, but instead must prime the
FIFO with data directly.
Signed-off-by: Hector Martin <redacted>
---
drivers/tty/serial/samsung_tty.c | 297 +++++++++++++++++++++++++++----
include/linux/serial_s3c.h | 16 ++
include/uapi/linux/serial_core.h | 3 +
3 files changed, 280 insertions(+), 36 deletions(-)
@@ -56,6 +56,9 @@/* flag to ignore all characters coming in */#define RXSTAT_DUMMY_READ (0x10000000)+/* IRQ number used when the handler is called in non-IRQ context */+#define NO_IRQ -1+structs3c24xx_uart_info{char*name;unsignedinttype;
It seems you add the third structure to differentiate type of UART.
There is already port type and s3c24xx_serial_drv_data, no need for
third structure (kind of similar to tries of Tamseel Shams in recent
patches). It's too much. Instead, differentiate by port type or prepare
own set of uart_ops if it's really different (like you did with startup
op).
@@ -231,11 +242,20 @@ static int s3c24xx_serial_txempty_nofifo(struct uart_port *port) /* * s3c64xx and later SoC's include the interrupt mask and status registers in * the controller itself, unlike the s3c24xx SoC's which have these registers- * in the interrupt controller. Check if the port type is s3c64xx or higher.+ * in the interrupt controller. Apple SoCs use a different flavor of mask+ * and status registers. This function returns the IRQ style to use. */-static int s3c24xx_serial_has_interrupt_mask(struct uart_port *port)+static int s3c24xx_irq_type(struct uart_port *port) {- return to_ourport(port)->info->type == PORT_S3C6400;+ switch (to_ourport(port)->info->type) {+ case PORT_S3C6400:+ return IRQ_S3C6400;+ case PORT_APPLE:+ return IRQ_APPLE;+ default:+ return IRQ_DISCRETE;+ }+
@@ -831,7 +890,9 @@ static irqreturn_t s3c24xx_serial_tx_chars(int irq, void *id) unsigned long flags; int count, dma_count = 0;- spin_lock_irqsave(&port->lock, flags);+ /* Only lock if called from IRQ context */+ if (irq != NO_IRQ)+ spin_lock_irqsave(&port->lock, flags);
I would prefer to have two functions - unlocked (doing actual stuff) and
a locking wrapper. Something like is done for regulator_is_enabled().
However the s3c24xx_serial_tx_chars() also unlocks-locks inside, so it
might be not easy to split common part Anyway hacking interrupt handler
to NO_IRQ is confusing and not readable.
@@ -1106,18 +1188,34 @@ static void s3c24xx_serial_shutdown(struct uart_port *port) } if (ourport->rx_claimed) {- if (!s3c24xx_serial_has_interrupt_mask(port))+ if (s3c24xx_irq_type(port) == IRQ_DISCRETE) free_irq(ourport->rx_irq, ourport); ourport->rx_claimed = 0; ourport->rx_enabled = 0; } /* Clear pending interrupts and mask all interrupts */- if (s3c24xx_serial_has_interrupt_mask(port)) {+ switch (s3c24xx_irq_type(port)) {+ case IRQ_APPLE: {+ unsigned int ucon;++ ucon = rd_regl(port, S3C2410_UCON);+ ucon &= ~(APPLE_UCON_TXTHRESH_ENA_MSK |+ APPLE_UCON_RXTHRESH_ENA_MSK |+ APPLE_UCON_RXTO_ENA_MSK);+ wr_regl(port, S3C2410_UCON, ucon);++ wr_regl(port, S3C2410_UTRSTAT, APPLE_UTRSTAT_ALL_FLAGS);++ free_irq(port->irq, ourport);+ break;+ }+ case IRQ_S3C6400: free_irq(port->irq, ourport); wr_regl(port, S3C64XX_UINTP, 0xf); wr_regl(port, S3C64XX_UINTM, 0xf);+ break; } if (ourport->dma)
@@ -1215,6 +1313,47 @@ static int s3c64xx_serial_startup(struct uart_port *port) return ret; }+static int apple_serial_startup(struct uart_port *port)+{+ struct s3c24xx_uart_port *ourport = to_ourport(port);+ unsigned long flags;+ unsigned int ufcon;+ int ret;++ wr_regl(port, S3C2410_UTRSTAT, APPLE_UTRSTAT_ALL_FLAGS);++ ret = request_irq(port->irq, apple_serial_handle_irq, IRQF_SHARED,+ s3c24xx_serial_portname(port), ourport);+ if (ret) {+ dev_err(port->dev, "cannot get irq %d\n", port->irq);+ return ret;+ }++ /* For compatibility with s3c24xx Soc's */+ ourport->rx_enabled = 1;+ ourport->rx_claimed = 1;+ ourport->tx_enabled = 0;+ ourport->tx_claimed = 1;++ spin_lock_irqsave(&port->lock, flags);++ ufcon = rd_regl(port, S3C2410_UFCON);+ ufcon |= S3C2410_UFCON_RESETRX | S5PV210_UFCON_RXTRIG8;+ if (!uart_console(port))+ ufcon |= S3C2410_UFCON_RESETTX;+ wr_regl(port, S3C2410_UFCON, ufcon);++ enable_rx_pio(ourport);++ spin_unlock_irqrestore(&port->lock, flags);++ /* Enable Rx Interrupt */+ s3c24xx_set_bit(port, APPLE_UCON_RXTHRESH_ENA, S3C2410_UCON);+ s3c24xx_set_bit(port, APPLE_UCON_RXTO_ENA, S3C2410_UCON);++ return ret;+}+ /* power power management control */ static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
@@ -1544,6 +1683,8 @@ static const char *s3c24xx_serial_type(struct uart_port *port) return "S3C2412"; case PORT_S3C6400: return "S3C6400/10";+ case PORT_APPLE:+ return "APPLE";
"Apple S5L"?
quoted hunk
default:
return NULL;
}
@@ -1868,9 +2009,16 @@ static int s3c24xx_serial_init_port(struct s3c24xx_uart_port *ourport, /* setup info for port */ port->dev = &platdev->dev;+ switch (s3c24xx_irq_type(port)) {+ /* Startup sequence is different for Apple SoC's */+ case IRQ_APPLE:+ s3c24xx_serial_ops.startup = apple_serial_startup;+ break; /* Startup sequence is different for s3c64xx and higher SoC's */- if (s3c24xx_serial_has_interrupt_mask(port))+ case IRQ_S3C6400: s3c24xx_serial_ops.startup = s3c64xx_serial_startup;
Don't overwrite specific ops. It's difficult to see then which ops are
being used. Instead create a new set of uart_ops matching the needs.
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2021-02-08 11:15:05
On Fri, Feb 05, 2021 at 05:39:51AM +0900, Hector Martin wrote:
quoted hunk
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 <redacted>
---
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
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.
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
On Mon, Feb 8, 2021 at 12:13 PM Hector Martin 'marcan' [off-list ref] wrote:
On 08/02/2021 19.29, Arnd Bergmann wrote:
quoted
On Mon, Feb 8, 2021 at 10:25 AM Marc Zyngier [off-list ref] wrote:
quoted
On Thu, 04 Feb 2021 20:39:48 +0000, Hector Martin [off-list ref] wrote:
quoted
quoted
+{
+ return readl(ic->base + reg);
Please consider using the _relaxed accessors, as I don't think any of
these interacts with memory (apart from IPIs, of course).
MSI interrupts require serializing with DMA, so at the minimum I think there
needs to be something that ensures that DMA from device into memory
has completed before delivering the completion interrupt to a driver. This
may already be implied when the AIC is entered, but this is hard to know
without actual hardware specs.
I don't think this can be implied in any case, because if IRQ A fires
and then the CPU speculates its way through AIC into the IRQ B handler,
which reads DMA'd memory, then IRQ B fires and it has higher priority
and *that* is what ends up getting returned from the event register
first, the execution will commit with an ordering violation.
I'm pretty sure we need *some* level of explicit synchronization between
reading the event register and actually delivering IRQs downstream.
Using _relaxed might be okay, but we'd still need something where the
isb() currently is in aic_handle_irq (though I admit I don't have a
perfect picture of the memory ordering subtleties involved here yet).
The idea of the barriers in readl/writel is to avoid having to guess these
subtleties, so maybe it's easier to start with the normal readl() and no
other barrier for an initial merge, as that should be sufficient, and then
do any optimizations in a later patch after everyone agrees what the
specific requirements are and much much time can be saved this way.
Incidentally, just from the races and problems I've run into with
trivial tests in m1n1, these CPUs seem to be *very* eager to speculate
and I suspect they will help uncover race conditions in Linux...
From: Hector Martin 'marcan' <hidden> Date: 2021-02-08 11:30:10
On 08/02/2021 19.29, Arnd Bergmann wrote:
On Mon, Feb 8, 2021 at 10:25 AM Marc Zyngier [off-list ref] wrote:
quoted
On Thu, 04 Feb 2021 20:39:48 +0000, Hector Martin [off-list ref] wrote:
quoted
quoted
+{
+ return readl(ic->base + reg);
Please consider using the _relaxed accessors, as I don't think any of
these interacts with memory (apart from IPIs, of course).
MSI interrupts require serializing with DMA, so at the minimum I think there
needs to be something that ensures that DMA from device into memory
has completed before delivering the completion interrupt to a driver. This
may already be implied when the AIC is entered, but this is hard to know
without actual hardware specs.
I don't think this can be implied in any case, because if IRQ A fires
and then the CPU speculates its way through AIC into the IRQ B handler,
which reads DMA'd memory, then IRQ B fires and it has higher priority
and *that* is what ends up getting returned from the event register
first, the execution will commit with an ordering violation.
I'm pretty sure we need *some* level of explicit synchronization between
reading the event register and actually delivering IRQs downstream.
Using _relaxed might be okay, but we'd still need something where the
isb() currently is in aic_handle_irq (though I admit I don't have a
perfect picture of the memory ordering subtleties involved here yet).
Incidentally, just from the races and problems I've run into with
trivial tests in m1n1, these CPUs seem to be *very* eager to speculate
and I suspect they will help uncover race conditions in Linux...
--
Hector Martin "marcan" (marcan@marcan.st)
Public Key: https://mrcn.st/pub
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-02-08 11:30:18
On Sun, 07 Feb 2021 08:28:35 +0000,
Hector Martin 'marcan' [off-list ref] wrote:
On 06/02/2021 22.58, Marc Zyngier wrote:
quoted
Hector Martin [off-list ref] wrote:
quoted
+static void cpu_sync_irq_to_fiq(struct arm64_cpu_capabilities const *cap)
+{
+ u64 daif = read_sysreg(daif);
+
+ /*
+ * By this point in the boot process IRQs are likely masked and FIOs
+ * aren't, so we need to sync things to avoid spurious early FIQs.
+ */
+
+ if (daif & PSR_I_BIT)
+ daif |= PSR_F_BIT;
+ else
+ daif &= ~PSR_F_BIT;
+
+ write_sysreg(daif, daif);
Could this happen too late? If, as explained above, we can get a FIQ
until we mask it here, what prevents something (a timer?) from kicking
and creating havoc just before the sync?
Nothing, other than timers not being enabled this early (hopefully the
bootloader doesn't leave a rogue timer running for us...)
I'm not sure we want to trust the FW on that particular front (no
offence intended...;-).
quoted
If the answer is "nothing", then it probably means that the default
behaviour should be to treat PSTATE.I and PSTATE.F as containing the
same value at all times, and not just as an afterthought when we
detect that we're on a CPU type or another.
I thought of this too. Originally I thought PSTATE.F was always set on
other systems, and thus unmasking FIQs could cause problems if there
is a pending rogue FIQ for some reason. However, while writing this
patch I realized that as part of normal process state changes we
already unmask FIQs anyway (see DAIF_PROCCTX).
Thus, in fact, this patch actually changes things (when the cpufeature
is set) to mask FIQs in some cases where they currently aren't, and
conversely to unmask them in some cases where they currently are. But
the fact that FIQ masking is somewhat inconsistent to begin with
suggests that we should be able to mess with it without causing
breakage for other systems.
So I guess in this case it would be legitimate to just make I==F on
every system, and if something breaks it should be fixed by making
whatever is causing a rogue FIQ not do that, right?
That is my current take on this patch. Nothing in the arm64 kernel
expects a FIQ today, so *when* a FIQ fires is pretty much irrelevant,
as long as we handle it properly (panic). Keeping the two bits in sync
is trivial, and shouldn't carry material overhead.
That would leave the vector switcheroo as the only thing the
cpufeature does, which would certainly simplify a lot of the patch.
quoted
This could expand into enabling Group-0 interrupts with GICv3 on
systems that have a single security state (such as virtual machines),
though I don't really see a good use case for it.
I could see having a separate vector path opening up the door for
performance hacks for very specific use cases that want really low
latency for *one* thing (e.g. the mess the Raspberry Pi folks do to
work around that braindead USB controller's performance issues),
though I imagine there would have to be very compelling reasons to
develop a framework to do this sanely upstream.
In general, this only works for single security state systems (systems
without EL3 and VMs), as FIQs are usually routed to the secure side
otherwise. If tied to a GIC, Group-0 interrupts aren't configurable
from NS either.
Incidentally, I have a personal interest in real-time performance
(especially audio); once the dust settles and we have a workable
kernel for normal use I do hope to spend some time taking a deep dive
into latencies and finding RT-unfriendly code, but that's pretty far
off right now. Maybe PREEMPT_RT will even be merged by then :-) (I
hope that without SMM to screw things up on these machines they might
make very nice RT-capable boxes...)
Aside from the lack of programmable priority, the lack of convenient
masking for per-CPU interrupts is a bit of an issue...
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-02-08 11:32:05
On Sun, 07 Feb 2021 08:47:23 +0000,
Hector Martin 'marcan' [off-list ref] wrote:
On 07/02/2021 00.37, Marc Zyngier wrote:
quoted
See my digression in patch 8. I really wonder what the benefit is to
treat FIQ independently of IRQ, and we might as well generalise
this. We could always panic on getting a FIQ on platforms that don't
expect one.
It'd be good to rope in the other interested parties (Mark for the
early entry code, James for RAS and SError handling).
CCing Mark and James: TL;DR what do you think about unconditionally
keeping DAIF.I == DAIF.F, would this break other platforms with
spurious FIQs or conversely mask FIQs when we don't want to in some
cases? The FIQ vector would remain a panic except on platforms that
require using it, via an alternatives patch.
It could be better to create a set of first class FIQ handlers rather
than this alternative target macro. I quickly hacked this instead,
which I find more readable.
I think I ended up with the macro change to keep it 1:1 with IRQ, vs a
separate branch... but I didn't think of the fallthrough-with-nop
trick, neat. It is definitely is more readable. Are you OK with me
pulling this patch in for v2, with your name on it?
Up to you, I don't mind either way. This is just code! :D
Of course.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-02-08 11:37:21
On Mon, 08 Feb 2021 10:29:23 +0000,
Arnd Bergmann [off-list ref] wrote:
On Mon, Feb 8, 2021 at 10:25 AM Marc Zyngier [off-list ref] wrote:
quoted
On Thu, 04 Feb 2021 20:39:48 +0000, Hector Martin [off-list ref] wrote:
quoted
quoted
+{
+ return readl(ic->base + reg);
Please consider using the _relaxed accessors, as I don't think any of
these interacts with memory (apart from IPIs, of course).
MSI interrupts require serializing with DMA, so at the minimum I think there
needs to be something that ensures that DMA from device into memory
has completed before delivering the completion interrupt to a driver. This
may already be implied when the AIC is entered, but this is hard to know
without actual hardware specs.
If there is a sync with memory required, it should happen at the point
where it is Acked, not when masked/unmasked or anything else. And
given that you want to sync with an external agent (the DMA producer),
the DMB generated by readl won't save you, as it only orders CPU
accesses AFAICT.
So if MSIs require some additional synchronisation we probably need a
DSB somewhere if the HW doesn't guarantee ordering of DMA vs
MSIs. Whether it does or not is anyone's bet.
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Ah, this is what I was expecting in the previous patch. I guess the
initial ARCH_APPLE patch could be moved down the line and add all the
dependencies in one go.
I was trying to introduce the Kconfig before the code that depends on
it; is it kosher to have it in the other order, looking for CONFIG_
defines that don't exist yet?
Absolutely. The only requirement is to make sure that nothing breaks in
the middle of a series.
Though in this case the only user earlier in the series is the Samsung
stuff, which doesn't care about FIQs, so I can just sort things as
FIQ->ARCH_APPLE->samsung->AIC...
Seems fine to me. Sorting out the infrastructure first (FIQ, memory
attributes) first is a requirement anyway, so the ordering of the
series could reflect that priority.
I'm not sure about AIC vs. ARCH_APPLE though. Right now the pattern is
that AIC depends on ARCH_APPLE and also defaults to that. But then you
can build with ARCH_APPLE and AIC disabled if you so choose, which
does result in a broken system on these machines. AIC should build
without ARCH_APPLE (as long as we're on ARM64), so we could reverse
that.
As long as ARCH_APPLE selects AIC, you can make AIC selectable on
its own. What I'm trying to avoid is people ending up with an unbootable
system, and not having interrupts is one thing that makes it really hard
to debug...
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Hector Martin 'marcan' <hidden> Date: 2021-02-08 12:17:07
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 :)
quoted
+ * Copyright 2021 Hector Martin [off-list ref]
A lot here might be difficult to reverse-egineer or figure out by
ourself, so usually people rely on vendor sources (the open source
compliance package). Didn't you receive such for the iOS (or whatever
was on your Mac)?
Apple source drops are sparse (they don't even include things like the
irqchip driver, only the very core OS code) and APSL licensed, which is
a license incompatible with the GPL. Worse, they've moved to a
partial-blob model with the M1; M1-compatible XNU source code drops now
include a .a blob with startup and CPU-specific code, for which no
source code is provided. (to be clear: Apple does not ship Linux for
these machines)
Honestly, beyond what's in this patchset and a few more details about
CPU registers like performance monitoring stuff that exist in public XNU
drops but I haven't looked into yet, Apple's source code drops are going
to be practically useless to us from here on out. It's all binaries
after this.
Apple device trees are not open source at all; those are provided by
iBoot and ship with device firmware, which is not openly licensed. Those
device trees are OF-inspired, but otherwise in a different format and
structure to Linux device trees.
Since there is zero Apple-published code or data with a license
compatible with the Linux kernel to be used here, there can be zero
copyright lines claiming any submissions are copyright Apple from us,
because that would imply a license violation has occurred. I am treating
this as I would any other no-source reverse engineering project, that
is, ensuring that I only look at Apple code (binaries, source,
devicetrees, whatever) to understand how the hardware functions, build
documentation for it (at least in my head, but I am also trying to
document things on our wiki as I go), and then write original code to
drive it from Linux, unrelated to whatever Apple was doing.
We're also trying to avoid looking at any Apple stuff in general as much
as possible, preferring black-box approaches where feasible, to minimize
exposure. For example, I only looked at an (outdated, arm32 era) AIC
register name list in XNU to write the AIC driver; there is no actual
AIC driver code in the source, and instead of decompiling Apple's binary
blob AIC driver module, I figured out how the hardware actually worked
via probing and experimentation. The entire userspace GPU stack is being
reverse engineered via a black-box approach, without any decompilation.
I'm going to see what I can do about the kernel driver in the future,
and prefer some kind of mmio tracing solution if I can get it all to
work on macOS.
As for this file specifically: while I am obviously looking at Apple's
DTs to figure out things like register offsets and what hardware exists,
those are facts, and facts are not copyrightable, and thus Apple does
not hold any copyright interest over this code as I submitted it. Short
of verbatim copying and pasting of entire nodes with bespoke property
names (which would never fly here anyway because Apple does things very
differently from Linux DTs when you get down into the details), it would
be extremely hard to argue that translating hardware information from
decompiled Apple DTs to Linux DTs would constitute a copyright
violation, since the entire purpose of DTs is to describe hardware facts.
You can read more about our reverse engineering and copyright policy at
https://alx.sh/re - if you have any suggestions or spot anything
problematic, please let me know.
(I'm actually probably going to change that copyright line to "The Asahi
Linux Contributors" for v2, if that's okay with the kernel folks, to be
in line with our other projects; I defaulted to my name since so far I'm
the only contributor to these files, but I expect other people to throw
PRs at me in the future and the history to end up with more names here)
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.
quoted
+ bootargs = "earlycon";
This should not be hard-coded in DTS. Pass it from bootloader.
My apologies, this was garbage left over from before I had bootargs
support in the bootloader. Will be gone for v2.
quoted
+ clk24: clk24 {
Just "clock". Node names should be generic.
Really? Almost every other device device tree uses unique clock node names.
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" (marcan@marcan.st)
Public Key: https://mrcn.st/pub
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Feb 8, 2021 at 12:36 PM Marc Zyngier [off-list ref] wrote:
On Mon, 08 Feb 2021 10:29:23 +0000, Arnd Bergmann [off-list ref] wrote:
quoted
On Mon, Feb 8, 2021 at 10:25 AM Marc Zyngier [off-list ref] wrote:
quoted
On Thu, 04 Feb 2021 20:39:48 +0000, Hector Martin [off-list ref] wrote:
quoted
quoted
+{
+ return readl(ic->base + reg);
Please consider using the _relaxed accessors, as I don't think any of
these interacts with memory (apart from IPIs, of course).
MSI interrupts require serializing with DMA, so at the minimum I think there
needs to be something that ensures that DMA from device into memory
has completed before delivering the completion interrupt to a driver. This
may already be implied when the AIC is entered, but this is hard to know
without actual hardware specs.
If there is a sync with memory required, it should happen at the point
where it is Acked, not when masked/unmasked or anything else. And
given that you want to sync with an external agent (the DMA producer),
the DMB generated by readl won't save you, as it only orders CPU
accesses AFAICT.
If the dmb in readl() is not sufficient for preventing the speculative read
on DMA data from a device that sent the MSI, why is it sufficient for
the case in which any device driver relies on readl() from a device
register to ensure that DMA is complete?
I had expected those two cases to be equivalent.
So if MSIs require some additional synchronisation we probably need a
DSB somewhere if the HW doesn't guarantee ordering of DMA vs
MSIs. Whether it does or not is anyone's bet.
Right, for all we know it could require a much more heavyweight barrier
as it does on certain broken SoCs, or nothing at all.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Marc Zyngier <maz@kernel.org> Date: 2021-02-08 12:28:57
On 2021-02-04 20:39, Hector Martin wrote:
This currently supports:
* SMP (via spin-tables)
* AIC IRQs
* Serial (with earlycon)
* Framebuffer
A number of properties are dynamic, and based on system firmware
decisions that vary from version to version. These are expected
to be filled in by the loader.
Signed-off-by: Hector Martin <redacted>
---
This unfortunately doesn't match the binding, which doesn't cater
for systems without a secure physical timer, nor allows the description
of the EL2 virtual timer.
You should also have *different* interrupts for EL1 and EL2 timers,
although this is all a lie...
Looking at the only similar case, XGene lies about the secure timer
(it doesn't have any), and of course doesn't have an EL2 virtual
timer (ARMv8.0 only).
A sensible course of action could be to update the binding to at least:
- tell the kernel that there is no secure physical timer (and that
the interrupt should be ignored)
- introduce a 5th possible interrupt for the EL2 virtual timer.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2021-02-08 12:29:40
On Mon, Feb 08, 2021 at 08:56:53PM +0900, Hector Martin 'marcan' wrote:
On 08/02/2021 20.04, Krzysztof Kozlowski wrote:
quoted
apple
Don't make things different for this one platform (comparing to all
other platforms). Apple is not that special. :)
AAPL is the old vendor prefix used in the PowerPC era. I'm happy to use
`apple`, as long as we're OK with having two different prefixes for the same
vendor, one for PPC and one for ARM64. I've seen opinions go both ways on
this one :)
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.
quoted
quoted
+ * Copyright 2021 Hector Martin [off-list ref]
A lot here might be difficult to reverse-egineer or figure out by
ourself, so usually people rely on vendor sources (the open source
compliance package). Didn't you receive such for the iOS (or whatever
was on your Mac)?
Apple source drops are sparse (they don't even include things like the
irqchip driver, only the very core OS code) and APSL licensed, which is a
license incompatible with the GPL. Worse, they've moved to a partial-blob
model with the M1; M1-compatible XNU source code drops now include a .a blob
with startup and CPU-specific code, for which no source code is provided.
(to be clear: Apple does not ship Linux for these machines)
Honestly, beyond what's in this patchset and a few more details about CPU
registers like performance monitoring stuff that exist in public XNU drops
but I haven't looked into yet, Apple's source code drops are going to be
practically useless to us from here on out. It's all binaries after this.
Apple device trees are not open source at all; those are provided by iBoot
and ship with device firmware, which is not openly licensed. Those device
trees are OF-inspired, but otherwise in a different format and structure to
Linux device trees.
Since there is zero Apple-published code or data with a license compatible
with the Linux kernel to be used here, there can be zero copyright lines
claiming any submissions are copyright Apple from us, because that would
imply a license violation has occurred. I am treating this as I would any
other no-source reverse engineering project, that is, ensuring that I only
look at Apple code (binaries, source, devicetrees, whatever) to understand
how the hardware functions, build documentation for it (at least in my head,
but I am also trying to document things on our wiki as I go), and then write
original code to drive it from Linux, unrelated to whatever Apple was doing.
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.
quoted
I guess Rob will comment on the dt-bindings more... but for me a generic
"arm-platform" is too generic. What's the point of it? I didn't see any
of such generic compatibles in other platforms.
This is a hack for patches #11/#12 to use, and I expect it will go away once
we figure out how to properly handle that problem (which needs further
discussion). Sorry for the noise, this should not be there in the final
version.
quoted
quoted
+ bootargs = "earlycon";
This should not be hard-coded in DTS. Pass it from bootloader.
My apologies, this was garbage left over from before I had bootargs support
in the bootloader. Will be gone for v2.
quoted
quoted
+ clk24: clk24 {
Just "clock". Node names should be generic.
Really? Almost every other device device tree uses unique clock node names.
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. :)
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Mon, Feb 8, 2021 at 1:13 PM Krzysztof Kozlowski [off-list ref] wrote:
On Mon, Feb 08, 2021 at 08:56:53PM +0900, Hector Martin 'marcan' wrote:
quoted
On 08/02/2021 20.04, Krzysztof Kozlowski wrote:
quoted
apple
Don't make things different for this one platform (comparing to all
other platforms). Apple is not that special. :)
AAPL is the old vendor prefix used in the PowerPC era. I'm happy to use
`apple`, as long as we're OK with having two different prefixes for the same
vendor, one for PPC and one for ARM64. I've seen opinions go both ways on
this one :)
Thanks for explanation. I propose to choose just "apple". Sticking to
old vendor name is not a requirement - we have few vendor prefixes which
were marked as deprecated because we switched to a better one.
We've gone back and forth on this a few times already. My current
preference would also be to go with "apple", not because it's somehow
nicer or clearer but because it avoids the namespace conflict with
what the Apple firmware uses:
The point of the vendor prefix is to prevent two people from introducing
the same identifier with slightly different meanings. This used to be
the stock ticker symbol ("ibm", "SUNW", "AAPL", ...) back when the
only people making devices with firmware were large corporations.
In FDT, it's usually random contributors introducing the names to make
something work on Linux, with unique strings coming from code review.
The identifiers that Apple use are highly unlikely to cause clashes
with the ones we use in Linux, since you probably won't find much
software that has to deal with both formats, but the fact that Apple
still prefixes those strings with their stock ticker symbol tells me that
someone there still considers the namespace relevant, so it's more
polite to stay out of their lawn.
Makes sense. In such case it's indeed your work. Since you introduce it,
the DTSes are usually licensed with (GPL-2.0+ OR MIT).
Indeed, we do want other OSs to use our dts files, so the general
preference is to have a permissive license, unless you have a strong
reason yourself to require GPL-only.
Arnd
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Hector Martin <hidden> Date: 2021-02-08 14:39:57
On 08/02/2021 21.40, Arnd Bergmann wrote:
On Mon, Feb 8, 2021 at 1:13 PM Krzysztof Kozlowski [off-list ref] wrote:
quoted
On Mon, Feb 08, 2021 at 08:56:53PM +0900, Hector Martin 'marcan' wrote:
quoted
On 08/02/2021 20.04, Krzysztof Kozlowski wrote:
quoted
apple
Don't make things different for this one platform (comparing to all
other platforms). Apple is not that special. :)
AAPL is the old vendor prefix used in the PowerPC era. I'm happy to use
`apple`, as long as we're OK with having two different prefixes for the same
vendor, one for PPC and one for ARM64. I've seen opinions go both ways on
this one :)
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.
quoted
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 (marcan@marcan.st)
Public Key: https://mrcn.st/pub
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel