Hi Mark, On Sat, 2 Oct 2021 at 16:16, Mark Kettenis <mark.kette...@xs4all.nl> wrote: > > > 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.
Well, firstly you can use a compatible string as mentioned above which solves the normal case. You just need a flag in plat which indicates that it should use word access for some registers. The ns16550 shows how to call dev_get_driver_data(). You will need to add members to s5p_serial_plat to select the tx/tx FIFO values since these need tobe variables now. BTW s5p_serial_baud() should have a param to indicate which access method to use. For the debug UART I suggest just putting an IS_ENABLED(CONFIG_ARCH_APPLE) in there. You could add a Kconfig for the Apple UART (even though it uses the same file) which might be a little nicer, but it is only the debug UART, after all... Finally, this driver is not perfect as it has CLK_EXYNOS only for some boards. Ideally all boards would move to use the clock driver. Regards, Simon