> From: Simon Glass <s...@chromium.org> > Date: Sun, 19 Sep 2021 21:15:59 -0600 > > Hi Mark, > > On Sat, 18 Sept 2021 at 07:55, Mark Kettenis <kette...@openbsd.org> wrote: > > > > Apple M1 SoCs include an S5L UART which is a variant of the S5P > > UART. Add support for this variant and enable it by default > > on Apple SoCs. > > > > Signed-off-by: Mark Kettenis <kette...@openbsd.org> > > --- > > arch/arm/include/asm/arch-m1/clk.h | 11 ++++++++ > > arch/arm/include/asm/arch-m1/uart.h | 41 +++++++++++++++++++++++++++++ > > arch/arm/mach-apple/board.c | 5 ++++ > > drivers/serial/Kconfig | 2 +- > > drivers/serial/serial_s5p.c | 22 ++++++++++++++++ > > 5 files changed, 80 insertions(+), 1 deletion(-) > > create mode 100644 arch/arm/include/asm/arch-m1/clk.h > > create mode 100644 arch/arm/include/asm/arch-m1/uart.h > > > > diff --git a/arch/arm/include/asm/arch-m1/clk.h > > b/arch/arm/include/asm/arch-m1/clk.h > > new file mode 100644 > > index 0000000000..f4326d0c0f > > --- /dev/null > > +++ b/arch/arm/include/asm/arch-m1/clk.h > > @@ -0,0 +1,11 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * Copyright (C) 2021 Mark Kettenis <kette...@openbsd.org> > > + */ > > + > > +#ifndef __ASM_ARM_ARCH_CLK_H_ > > +#define __ASM_ARM_ARCH_CLK_H_ > > + > > +unsigned long get_uart_clk(int dev_index); > > comment? But this should be in a clock driver, unless you are trying > to get a debug UART up ASAP with minimal code? > > > + > > +#endif > > diff --git a/arch/arm/include/asm/arch-m1/uart.h > > b/arch/arm/include/asm/arch-m1/uart.h > > new file mode 100644 > > index 0000000000..d2a17a221e > > --- /dev/null > > +++ b/arch/arm/include/asm/arch-m1/uart.h > > @@ -0,0 +1,41 @@ > > +/* SPDX-License-Identifier: GPL-2.0+ */ > > +/* > > + * (C) Copyright 2009 Samsung Electronics > > + * Minkyu Kang <mk7.k...@samsung.com> > > + * Heungjun Kim <riverful....@samsung.com> > > + */ > > + > > +#ifndef __ASM_ARCH_UART_H_ > > +#define __ASM_ARCH_UART_H_ > > + > > +#ifndef __ASSEMBLY__ > > +/* baudrate rest value */ > > +union br_rest { > > + unsigned short slot; /* udivslot */ > > + unsigned char value; /* ufracval */ > > +}; > > + > > +struct s5p_uart { > > + unsigned int ulcon; > > + unsigned int ucon; > > + unsigned int ufcon; > > + unsigned int umcon; > > + unsigned int utrstat; > > + unsigned int uerstat; > > + unsigned int ufstat; > > + unsigned int umstat; > > + unsigned int utxh; > > + unsigned int urxh; > > + unsigned int ubrdiv; > > + union br_rest rest; > > + unsigned char res3[0x3fd0]; > > +}; > > + > > +static inline int s5p_uart_divslot(void) > > +{ > > + return 0; > > +} > > + > > +#endif /* __ASSEMBLY__ */ > > + > > +#endif > > diff --git a/arch/arm/mach-apple/board.c b/arch/arm/mach-apple/board.c > > index 0c8b35292e..8bc5c2f69e 100644 > > --- a/arch/arm/mach-apple/board.c > > +++ b/arch/arm/mach-apple/board.c > > @@ -156,3 +156,8 @@ ulong board_get_usable_ram_top(ulong total_size) > > */ > > return 0x980000000; > > } > > + > > +unsigned long get_uart_clk(int dev_index) > > +{ > > + return 24000000; > > Should add to devicetree for the driver > > > +} > > diff --git a/drivers/serial/Kconfig b/drivers/serial/Kconfig > > index 93348c0929..c3ac773929 100644 > > --- a/drivers/serial/Kconfig > > +++ b/drivers/serial/Kconfig > > @@ -719,7 +719,7 @@ config ROCKCHIP_SERIAL > > > > config S5P_SERIAL > > bool "Support for Samsung S5P UART" > > - depends on ARCH_EXYNOS || ARCH_S5PC1XX > > + depends on ARCH_APPLE || ARCH_EXYNOS || ARCH_S5PC1XX > > default y > > help > > Select this to enable Samsung S5P UART support. > > diff --git a/drivers/serial/serial_s5p.c b/drivers/serial/serial_s5p.c > > index 6d09952a5d..eb770d9b62 100644 > > --- a/drivers/serial/serial_s5p.c > > +++ b/drivers/serial/serial_s5p.c > > @@ -21,12 +21,21 @@ > > > > DECLARE_GLOBAL_DATA_PTR; > > > > +#ifdef CONFIG_ARCH_APPLE > > This should use the compatible string to decide which variant it is, > then the checks happen at runtime. We should never have arch-specific > #ifdefs in drivers. > > > +#define RX_FIFO_COUNT_SHIFT 0 > > +#define RX_FIFO_COUNT_MASK (0xf << RX_FIFO_COUNT_SHIFT) > > +#define RX_FIFO_FULL (1 << 8) > > +#define TX_FIFO_COUNT_SHIFT 4 > > +#define TX_FIFO_COUNT_MASK (0xf << TX_FIFO_COUNT_SHIFT) > > +#define TX_FIFO_FULL (1 << 9) > > +#else > > #define RX_FIFO_COUNT_SHIFT 0 > > #define RX_FIFO_COUNT_MASK (0xff << RX_FIFO_COUNT_SHIFT) > > #define RX_FIFO_FULL (1 << 8) > > #define TX_FIFO_COUNT_SHIFT 16 > > #define TX_FIFO_COUNT_MASK (0xff << TX_FIFO_COUNT_SHIFT) > > #define TX_FIFO_FULL (1 << 24) > > +#endif > > > > /* Information about a serial port */ > > struct s5p_serial_plat { > > @@ -83,7 +92,11 @@ static void __maybe_unused s5p_serial_baud(struct > > s5p_uart *uart, uint uclk, > > if (s5p_uart_divslot()) > > writew(udivslot[val % 16], &uart->rest.slot); > > else > > +#ifdef CONFIG_ARCH_APPLE > > + writel(val % 16, &uart->rest.value); > > +#else > > writeb(val % 16, &uart->rest.value); > > +#endif > > } > > > > #ifndef CONFIG_SPL_BUILD > > @@ -148,7 +161,11 @@ static int s5p_serial_getc(struct udevice *dev) > > return -EAGAIN; > > > > serial_err_check(uart, 0); > > +#ifdef CONFIG_ARCH_APPLE > > + return (int)(readl(&uart->urxh) & 0xff); > > +#else > > return (int)(readb(&uart->urxh) & 0xff); > > +#endif > > } > > > > static int s5p_serial_putc(struct udevice *dev, const char ch) > > @@ -159,7 +176,11 @@ static int s5p_serial_putc(struct udevice *dev, const > > char ch) > > if (readl(&uart->ufstat) & TX_FIFO_FULL) > > return -EAGAIN; > > > > +#ifdef CONFIG_ARCH_APPLE > > + writel(ch, &uart->utxh); > > +#else > > writeb(ch, &uart->utxh); > > +#endif > > serial_err_check(uart, 1); > > > > return 0; > > @@ -201,6 +222,7 @@ static const struct dm_serial_ops s5p_serial_ops = { > > > > static const struct udevice_id s5p_serial_ids[] = { > > { .compatible = "samsung,exynos4210-uart" }, > > + { .compatible = "apple,s5l-uart" }, > > { } > > }; > > > > -- > > 2.33.0 > > > > Can you add debug UART support? Or does this already work?
I don't think it does work. But yes, I want to add it since it will help a lot with debugging. That makes fixing some of the issues raised by you and bing more problematic since the debug UART support code: - Needs to use 32-bit access on Apple M1, and using the "reg-io-width" property there isn't possible. - Relies on the FIFO control bits which are in a different position on the Apple M1. So I am starting to wonder whether it makes sense to simply copy serial_s5p.c to serial_s5l.c and modify that one to do the right thing on the apple SoC. Alternatively I could introduce Kconfig stuff to switch between S5L and S5P support.