mbox series

[v4,00/18] Apple M1 SoC platform bring-up

Message ID 20210402090542.131194-1-marcan@marcan.st
Headers show
Series Apple M1 SoC platform bring-up | expand

Message

Hector Martin April 2, 2021, 9:05 a.m. UTC
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)

See below for an overview of changes since v3.

== Merge notes ==

This patchset has several dependencies:

* Build dep on the FIQ support series in [1].
* Runtime dep on the Samsung TTY changes in tty-next (modulo DT validation).
* Runtime dep on the nVHE changes being reviewed in [2].

A tree containing this patchset on top of the required dependencies is
available at [3][4], for those who want to test it.

This series is expected to be merged by Arnd via the SoC tree.
Maintainers, please ack if you are happy with the patches. We will
coordinate with Arnd and Mark on the FIQ series to make sure all
that goes smoothly.

[1] https://lore.kernel.org/linux-arm-kernel/20210315115629.57191-1-mark.rutland@arm.com/T/
[2] https://lore.kernel.org/linux-arm-kernel/20210330173947.999859-1-maz@kernel.org/T/
[3] git://github.com/AsahiLinux/linux.git upstream-bringup-v4
[4] https://github.com/AsahiLinux/linux/tree/upstream-bringup-v4

== Testing notes ==

This has been tested on an Apple M1 Mac Mini booting to a framebuffer
and serial console, with SMP and KASLR, with an arm64 defconfig
(+ CONFIG_FB_SIMPLE for the fb). In addition, the AIC driver now supports
running in EL1, tested in UP mode only.

== Patch overview ==

- 01-02 Core platform DT bindings
- 03-04 CPU DT bindings and MIDR defines
- 05-06 Add interrupt-names support to the ARM timer driver
        This is used to cleanly express the lack of a secure timer;
        platforms in the past have used various hacks like dummy
        IRQs here.
- 07-12 ioremap_np() (nGnRnE) support
        The fabric in these SoCs only supports nGnRnE accesses for
        standard MMIO, except for PCI ranges which use nGnRE. Linux
        currently defaults to the latter on ARM64, so this adds a new
        ioremap type and DT properties to automatically select it for
        drivers using OF and devm abstractions, under buses specified
        in DT.
- 13-15 AIC (Apple Interrupt Controller) driver and support defines
        This also embeds FIQ handling for this platform.
- 16    Introduce CONFIG_ARCH_APPLE & add it to defconfig
- 17    simple-framebuffer bindings for Apple (trivial)
- 18    Add the initial M1 Mac Mini (j274) devicetree

== About the hardware ==

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.

We also have WIP/not merged yet support for loading kernels and
interacting via dwc3 usb-gadget, which works with a standard C-C or C-A
cable and any Linux host.

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 and #asahi-dev on freenode to chat with us, or check
our website for more information on the project:

https://asahilinux.org/

== Changes since v3 ==

* Serial patches have already been merged into tty-next and are
  no longer included
* nVHE fixup patches are being reviewed separately and no longer
  part of this series
* Updated arm,arch-timer bindings to be more restrictive, renamed
  phys-secure to sec-phys
* Reordered ioremap() variants list in device-io.rst
* Removed sysreg_apple.h (defines are in drivers now)
* Many cleanups, bug fixes, and reworks to AIC, including some bug
  fixes from Marc's KVM series, kernel as EL1 support, and more.
* Simplified non-posted MMIO DT handling to only apply to direct
  children of a bus, and only use "nonposted-mmio". Removed quirk
  optimization.
* Implemented default pci_remap_cfgspace() in terms of ioremap_np()
  and removed arch-specific pci_remap_cfgspace override for arm64
  (arm32 can come later)
* Replaced license in AIC bindings header with GPL-2.0+ OR MIT
* Other minor typo/style fixes

Arnd Bergmann (1):
  docs: driver-api: device-io: Document I/O access functions

Hector Martin (17):
  dt-bindings: vendor-prefixes: Add apple prefix
  dt-bindings: arm: apple: Add bindings for Apple ARM platforms
  dt-bindings: arm: cpus: Add apple,firestorm & icestorm compatibles
  arm64: cputype: Add CPU implementor & types for the Apple M1 cores
  dt-bindings: timer: arm,arch_timer: Add interrupt-names support
  arm64: arch_timer: Implement support for interrupt-names
  asm-generic/io.h:  Add a non-posted variant of ioremap()
  docs: driver-api: device-io: Document ioremap() variants & access
    funcs
  arm64: Implement ioremap_np() to map MMIO as nGnRnE
  asm-generic/io.h: implement pci_remap_cfgspace using ioremap_np
  of/address: Add infrastructure to declare MMIO as non-posted
  arm64: Move ICH_ sysreg bits from arm-gic-v3.h to sysreg.h
  dt-bindings: interrupt-controller: Add DT bindings for apple-aic
  irqchip/apple-aic: Add support for the Apple Interrupt Controller
  arm64: Kconfig: Introduce CONFIG_ARCH_APPLE
  dt-bindings: display: Add apple,simple-framebuffer
  arm64: apple: Add initial Apple Mac mini (M1, 2020) devicetree

 .../devicetree/bindings/arm/apple.yaml        |  64 ++
 .../devicetree/bindings/arm/cpus.yaml         |   2 +
 .../bindings/display/simple-framebuffer.yaml  |   5 +
 .../interrupt-controller/apple,aic.yaml       |  88 ++
 .../bindings/timer/arm,arch_timer.yaml        |  19 +
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 Documentation/driver-api/device-io.rst        | 356 ++++++++
 .../driver-api/driver-model/devres.rst        |   1 +
 MAINTAINERS                                   |  14 +
 arch/arm64/Kconfig.platforms                  |   7 +
 arch/arm64/boot/dts/Makefile                  |   1 +
 arch/arm64/boot/dts/apple/Makefile            |   2 +
 arch/arm64/boot/dts/apple/t8103-j274.dts      |  45 +
 arch/arm64/boot/dts/apple/t8103.dtsi          | 135 +++
 arch/arm64/configs/defconfig                  |   1 +
 arch/arm64/include/asm/cputype.h              |   6 +
 arch/arm64/include/asm/io.h                   |  11 +-
 arch/arm64/include/asm/sysreg.h               |  60 ++
 arch/sparc/include/asm/io_64.h                |   4 +
 drivers/clocksource/arm_arch_timer.c          |  24 +-
 drivers/irqchip/Kconfig                       |   8 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-apple-aic.c               | 837 ++++++++++++++++++
 drivers/of/address.c                          |  43 +-
 include/asm-generic/io.h                      |  22 +-
 include/asm-generic/iomap.h                   |   9 +
 include/clocksource/arm_arch_timer.h          |   1 +
 .../interrupt-controller/apple-aic.h          |  15 +
 include/linux/cpuhotplug.h                    |   1 +
 include/linux/io.h                            |  23 +-
 include/linux/ioport.h                        |   1 +
 include/linux/irqchip/arm-gic-v3.h            |  56 --
 include/linux/of_address.h                    |   1 +
 lib/devres.c                                  |  22 +
 34 files changed, 1807 insertions(+), 80 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/apple.yaml
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/apple,aic.yaml
 create mode 100644 arch/arm64/boot/dts/apple/Makefile
 create mode 100644 arch/arm64/boot/dts/apple/t8103-j274.dts
 create mode 100644 arch/arm64/boot/dts/apple/t8103.dtsi
 create mode 100644 drivers/irqchip/irq-apple-aic.c
 create mode 100644 include/dt-bindings/interrupt-controller/apple-aic.h

--
2.30.0

Comments

Rob Herring (Arm) April 6, 2021, 4:44 p.m. UTC | #1
On Fri, 02 Apr 2021 18:05:29 +0900, Hector Martin wrote:
> Not all platforms provide the same set of timers/interrupts, and Linux

> only needs one (plus kvm/guest ones); some platforms are working around

> this by using dummy fake interrupts. Implementing interrupt-names allows

> the devicetree to specify an arbitrary set of available interrupts, so

> the timer code can pick the right one.

> 

> This also adds the hyp-virt timer/interrupt, which was previously not

> expressed in the fixed 4-interrupt form.

> 

> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

> Acked-by: Marc Zyngier <maz@kernel.org>

> Reviewed-by: Tony Lindgren <tony@atomide.com>

> Signed-off-by: Hector Martin <marcan@marcan.st>

> ---

>  .../bindings/timer/arm,arch_timer.yaml        | 19 +++++++++++++++++++

>  1 file changed, 19 insertions(+)

> 


Reviewed-by: Rob Herring <robh@kernel.org>
Rob Herring (Arm) April 6, 2021, 4:56 p.m. UTC | #2
On Fri, Apr 02, 2021 at 06:05:42PM +0900, Hector Martin wrote:
> This currently supports:

> 

> * SMP (via spin-tables)

> * AIC IRQs

> * Serial (with earlycon)

> * Framebuffer

> 

> A number of properties are dynamic, and based on system firmware

> decisions that vary from version to version. These are expected

> to be filled in by the loader.

> 

> Signed-off-by: Hector Martin <marcan@marcan.st>

> ---

>  MAINTAINERS                              |   1 +

>  arch/arm64/boot/dts/Makefile             |   1 +

>  arch/arm64/boot/dts/apple/Makefile       |   2 +

>  arch/arm64/boot/dts/apple/t8103-j274.dts |  45 ++++++++

>  arch/arm64/boot/dts/apple/t8103.dtsi     | 135 +++++++++++++++++++++++

>  5 files changed, 184 insertions(+)

>  create mode 100644 arch/arm64/boot/dts/apple/Makefile

>  create mode 100644 arch/arm64/boot/dts/apple/t8103-j274.dts

>  create mode 100644 arch/arm64/boot/dts/apple/t8103.dtsi


Reviewed-by: Rob Herring <robh@kernel.org>
Marc Zyngier April 6, 2021, 6:16 p.m. UTC | #3
Hi Hector,

On Fri, 02 Apr 2021 10:05:39 +0100,
Hector Martin <marcan@marcan.st> wrote:
> 

> This is the root interrupt controller used on Apple ARM SoCs such as the

> M1. This irqchip driver performs multiple functions:

> 

> * Handles both IRQs and FIQs

> 

> * Drives the AIC peripheral itself (which handles IRQs)

> 

> * Dispatches FIQs to downstream hard-wired clients (currently the ARM

>   timer).

> 

> * Implements a virtual IPI multiplexer to funnel multiple Linux IPIs

>   into a single hardware IPI

> 

> Signed-off-by: Hector Martin <marcan@marcan.st>

> ---

>  MAINTAINERS                     |   2 +

>  drivers/irqchip/Kconfig         |   8 +

>  drivers/irqchip/Makefile        |   1 +

>  drivers/irqchip/irq-apple-aic.c | 837 ++++++++++++++++++++++++++++++++

>  include/linux/cpuhotplug.h      |   1 +

>  5 files changed, 849 insertions(+)

>  create mode 100644 drivers/irqchip/irq-apple-aic.c


[...]

> +static int aic_irq_domain_translate(struct irq_domain *id,

> +				    struct irq_fwspec *fwspec,

> +				    unsigned long *hwirq,

> +				    unsigned int *type)

> +{

> +	struct aic_irq_chip *ic = id->host_data;

> +

> +	if (fwspec->param_count != 3 || !is_of_node(fwspec->fwnode))

> +		return -EINVAL;

> +

> +	switch (fwspec->param[0]) {

> +	case AIC_IRQ:

> +		if (fwspec->param[1] >= ic->nr_hw)

> +			return -EINVAL;

> +		*hwirq = fwspec->param[1];

> +		break;

> +	case AIC_FIQ:

> +		if (fwspec->param[1] >= AIC_NR_FIQ)

> +			return -EINVAL;

> +		*hwirq = ic->nr_hw + fwspec->param[1];

> +

> +		/*

> +		 * In EL1 the non-redirected registers are the guest's,

> +		 * not EL2's, so remap the hwirqs to match.

> +		 */

> +		if (!is_kernel_in_hyp_mode()) {

> +			switch (fwspec->param[1]) {

> +			case AIC_TMR_GUEST_PHYS:

> +				*hwirq = ic->nr_hw + AIC_TMR_HV_PHYS;

> +				break;

> +			case AIC_TMR_GUEST_VIRT:

> +				*hwirq = ic->nr_hw + AIC_TMR_HV_VIRT;

> +				break;

> +			case AIC_TMR_HV_PHYS:

> +			case AIC_TMR_HV_VIRT:

> +				return -ENOENT;

> +			default:

> +				break;

> +			}

> +		}


Urgh, this is nasty. You are internally remapping the hwirq from one
timer to another in order to avoid accessing the enable register
which happens to be an EL2 only register?

The way we normally deal with this kind of things is by providing a
different set of irq_chip callbacks. In this particular case, this
would leave mask/unmask as empty stubs. Or you could move the FIQ
handling to use handle_simple_irq(), because there isn't any callback
that is actually applicable.

It isn't a big deal for now, but that's something we should consider
addressing in the future. With that in mind:

Reviewed-by: Marc Zyngier <maz@kernel.org>


Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
Hector Martin April 6, 2021, 7:21 p.m. UTC | #4
On 07/04/2021 03.16, Marc Zyngier wrote:
> Hi Hector,

> 

> On Fri, 02 Apr 2021 10:05:39 +0100,

> Hector Martin <marcan@marcan.st> wrote:

>> +		/*

>> +		 * In EL1 the non-redirected registers are the guest's,

>> +		 * not EL2's, so remap the hwirqs to match.

>> +		 */

>> +		if (!is_kernel_in_hyp_mode()) {

>> +			switch (fwspec->param[1]) {

>> +			case AIC_TMR_GUEST_PHYS:

>> +				*hwirq = ic->nr_hw + AIC_TMR_HV_PHYS;

>> +				break;

>> +			case AIC_TMR_GUEST_VIRT:

>> +				*hwirq = ic->nr_hw + AIC_TMR_HV_VIRT;

>> +				break;

>> +			case AIC_TMR_HV_PHYS:

>> +			case AIC_TMR_HV_VIRT:

>> +				return -ENOENT;

>> +			default:

>> +				break;

>> +			}

>> +		}

> 

> Urgh, this is nasty. You are internally remapping the hwirq from one

> timer to another in order to avoid accessing the enable register

> which happens to be an EL2 only register?


The remapping is to make the IRQs route properly at all.

There are EL2 and EL0 timers, and on GIC each timer goes to its own IRQ. 
But here there are no real IRQs, everything's a FIQ. However, thanks to 
VHE, the EL2 timer shows up as the EL0 timer, and the EL0 timer is 
accessed via EL02 registers, when in EL2. So in EL2/VHE mode, "HV" means 
EL0 and "guest" means EL02, while in EL1, there is no HV and "guest" 
means EL0. And since we figure out which IRQ fired by reading timer 
registers, this is what matters. So I map the guest IRQs to the HV 
hwirqs in EL1 mode, which makes this all work out. Then the timer code 
goes and ends up undoing all this logic again, so we map to separate 
fake "IRQs" only to end up right back at using the same timer registers 
anuway :-)

Really, the ugliness here is that the constant meaning is overloaded. In 
fwspec context they mean what they say on the tin, while in hwirq 
context "HV" means EL0 and "guest" means EL02 (other FIQs would be 
passed through unchanged). Perhaps some additional defines might help 
clarify this? Say, at the top of this file (not in the binding),

/*
  * Pass-through mapping from real timers to the correct registers to
  * access them in EL2/VHE mode. When running in EL1, this gets
  * overridden to access the guest timer using EL0 registers.
  */
#define AIC_TMR_EL0_PHYS AIC_TMR_HV_PHYS
#define AIC_TMR_EL0_VIRT AIC_TMR_HV_VIRT
#define AIC_TMR_EL02_PHYS AIC_TMR_GUEST_PHYS
#define AIC_TMR_EL02_VIRT AIC_TMR_GUEST_VIRT

Then the irqchip/FIQ dispatch side can use the EL* constants, the 
default pass-through mapping is appropriate for VHE/EL2 mode, and 
translation can adjust it for EL1 mode.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub
Andy Shevchenko April 7, 2021, 1:27 p.m. UTC | #5
On Fri, Apr 2, 2021 at 12:07 PM Hector Martin <marcan@marcan.st> wrote:
>

> Now that we have ioremap_np(), we can make pci_remap_cfgspace() default

> to it, falling back to ioremap() on platforms where it is not available.

>

> Remove the arm64 implementation, since that is now redundant. Future

> cleanups should be able to do the same for other arches, and eventually

> make the generic pci_remap_cfgspace() unconditional.


...

> +       void __iomem *ret = ioremap_np(offset, size);

> +

> +       if (!ret)

> +               ret = ioremap(offset, size);

> +

> +       return ret;


Usually negative conditions are worse for cognitive functions of human beings.
(On top of that some patterns are applied)

I would rewrite above as

void __iomem *ret;

ret = ioremap_np(offset, size);
if (ret)
  return ret;

return ioremap(offset, size);

-- 
With Best Regards,
Andy Shevchenko
Will Deacon April 7, 2021, 9:03 p.m. UTC | #6
On Wed, Apr 07, 2021 at 04:27:42PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 2, 2021 at 12:07 PM Hector Martin <marcan@marcan.st> wrote:

> >

> > Now that we have ioremap_np(), we can make pci_remap_cfgspace() default

> > to it, falling back to ioremap() on platforms where it is not available.

> >

> > Remove the arm64 implementation, since that is now redundant. Future

> > cleanups should be able to do the same for other arches, and eventually

> > make the generic pci_remap_cfgspace() unconditional.

> 

> ...

> 

> > +       void __iomem *ret = ioremap_np(offset, size);

> > +

> > +       if (!ret)

> > +               ret = ioremap(offset, size);

> > +

> > +       return ret;

> 

> Usually negative conditions are worse for cognitive functions of human beings.

> (On top of that some patterns are applied)

> 

> I would rewrite above as

> 

> void __iomem *ret;

> 

> ret = ioremap_np(offset, size);

> if (ret)

>   return ret;

> 

> return ioremap(offset, size);


Looks like it might be one of those rare occasions where the GCC ternary if
extension thingy comes in handy:

	return ioremap_np(offset, size) ?: ioremap(offset, size);

but however it's done, the logic looks good to me and thanks Hector for
updating this:

Acked-by: Will Deacon <will@kernel.org>


Will
Will Deacon April 7, 2021, 9:09 p.m. UTC | #7
On Fri, Apr 02, 2021 at 06:05:39PM +0900, Hector Martin wrote:
> This is the root interrupt controller used on Apple ARM SoCs such as the

> M1. This irqchip driver performs multiple functions:

> 

> * Handles both IRQs and FIQs

> 

> * Drives the AIC peripheral itself (which handles IRQs)

> 

> * Dispatches FIQs to downstream hard-wired clients (currently the ARM

>   timer).

> 

> * Implements a virtual IPI multiplexer to funnel multiple Linux IPIs

>   into a single hardware IPI

> 

> Signed-off-by: Hector Martin <marcan@marcan.st>

> ---

>  MAINTAINERS                     |   2 +

>  drivers/irqchip/Kconfig         |   8 +

>  drivers/irqchip/Makefile        |   1 +

>  drivers/irqchip/irq-apple-aic.c | 837 ++++++++++++++++++++++++++++++++

>  include/linux/cpuhotplug.h      |   1 +

>  5 files changed, 849 insertions(+)

>  create mode 100644 drivers/irqchip/irq-apple-aic.c


Couple of stale comment nits:

> +static void aic_ipi_unmask(struct irq_data *d)

> +{

> +	struct aic_irq_chip *ic = irq_data_get_irq_chip_data(d);

> +	u32 irq_bit = BIT(irqd_to_hwirq(d));

> +

> +	atomic_or(irq_bit, this_cpu_ptr(&aic_vipi_enable));

> +

> +	/*

> +	 * The atomic_or() above must complete before the atomic_read_acquire() below to avoid

> +	 * racing aic_ipi_send_mask().

> +	 */


(the atomic_read_acquire() is now an atomic_read())

> +	smp_mb__after_atomic();

> +

> +	/*

> +	 * If a pending vIPI was unmasked, raise a HW IPI to ourselves.

> +	 * No barriers needed here since this is a self-IPI.

> +	 */

> +	if (atomic_read(this_cpu_ptr(&aic_vipi_flag)) & irq_bit)

> +		aic_ic_write(ic, AIC_IPI_SEND, AIC_IPI_SEND_CPU(smp_processor_id()));

> +}

> +

> +static void aic_ipi_send_mask(struct irq_data *d, const struct cpumask *mask)

> +{

> +	struct aic_irq_chip *ic = irq_data_get_irq_chip_data(d);

> +	u32 irq_bit = BIT(irqd_to_hwirq(d));

> +	u32 send = 0;

> +	int cpu;

> +	unsigned long pending;

> +

> +	for_each_cpu(cpu, mask) {

> +		/*

> +		 * This sequence is the mirror of the one in aic_ipi_unmask();

> +		 * see the comment there. Additionally, release semantics

> +		 * ensure that the vIPI flag set is ordered after any shared

> +		 * memory accesses that precede it. This therefore also pairs

> +		 * with the atomic_fetch_andnot in aic_handle_ipi().

> +		 */

> +		pending = atomic_fetch_or_release(irq_bit, per_cpu_ptr(&aic_vipi_flag, cpu));

> +

> +		/*

> +		 * The atomic_fetch_or_release() above must complete before the

> +		 * atomic_read_acquire() below to avoid racing aic_ipi_unmask().

> +		 */


(same here)

> +		smp_mb__after_atomic();

> +

> +		if (!(pending & irq_bit) &&

> +		    (atomic_read(per_cpu_ptr(&aic_vipi_enable, cpu)) & irq_bit))

> +			send |= AIC_IPI_SEND_CPU(cpu);

> +	}


But with that:

Acked-by: Will Deacon <will@kernel.org>


Will
Hector Martin April 8, 2021, 11:01 a.m. UTC | #8
On 08/04/2021 06.03, Will Deacon wrote:
>> I would rewrite above as

>>

>> void __iomem *ret;

>>

>> ret = ioremap_np(offset, size);

>> if (ret)

>>    return ret;

>>

>> return ioremap(offset, size);

> 

> Looks like it might be one of those rare occasions where the GCC ternary if

> extension thingy comes in handy:

> 

> 	return ioremap_np(offset, size) ?: ioremap(offset, size);


Today I learned that this one is kosher in kernel code. Handy! Let's go 
with that.

> Acked-by: Will Deacon <will@kernel.org>


Thanks!

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub
Hector Martin April 8, 2021, 11:02 a.m. UTC | #9
On 08/04/2021 06.09, Will Deacon wrote:
> Couple of stale comment nits:


[...]

> But with that:

> 

> Acked-by: Will Deacon <will@kernel.org>


Fixed those for the PR, thanks!

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub
Andy Shevchenko April 8, 2021, 11:24 a.m. UTC | #10
On Thu, Apr 8, 2021 at 2:01 PM Hector Martin <marcan@marcan.st> wrote:
> On 08/04/2021 06.03, Will Deacon wrote:

> >> I would rewrite above as

> >>

> >> void __iomem *ret;

> >>

> >> ret = ioremap_np(offset, size);

> >> if (ret)

> >>    return ret;

> >>

> >> return ioremap(offset, size);

> >

> > Looks like it might be one of those rare occasions where the GCC ternary if

> > extension thingy comes in handy:

> >

> >       return ioremap_np(offset, size) ?: ioremap(offset, size);

>

> Today I learned that this one is kosher in kernel code. Handy! Let's go

> with that.


It depends on the maintainer. Greg, for example, doesn't  like this. I
have no strong opinion (I use both variants on case-by-case basis),
though I think in headers better to spell out all conditionals
clearly.

-- 
With Best Regards,
Andy Shevchenko