From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E7C0EC433EF for ; Sat, 2 Oct 2021 22:15:58 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id B913061B1C for ; Sat, 2 Oct 2021 22:15:57 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org B913061B1C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=xs4all.nl Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 7FC4E83238; Sun, 3 Oct 2021 00:15:54 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=xs4all.nl Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: by phobos.denx.de (Postfix, from userid 109) id 12FB383246; Sun, 3 Oct 2021 00:15:53 +0200 (CEST) Received: from sibelius.xs4all.nl (sibelius.xs4all.nl [83.163.83.176]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 3C27A831AE for ; Sun, 3 Oct 2021 00:15:48 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=fail (p=none dis=none) header.from=xs4all.nl Authentication-Results: phobos.denx.de; spf=fail smtp.mailfrom=mark.kettenis@xs4all.nl Received: from localhost (bloch.sibelius.xs4all.nl [local]) by bloch.sibelius.xs4all.nl (OpenSMTPD) with ESMTPA id 0317ef44; Sun, 3 Oct 2021 00:15:46 +0200 (CEST) Date: Sun, 3 Oct 2021 00:15:46 +0200 (CEST) From: Mark Kettenis To: Simon Glass Cc: kettenis@openbsd.org, u-boot@lists.denx.de, oliver.graute@kococonnector.com, ycliang@andestech.com, kishon@ti.com, priyanka.jain@nxp.com, stephan@gerhold.net, padmarao.begari@microchip.com, tianrui-wei@outlook.com, michael@walle.cc, xypron.glpk@gmx.de, ashe@kivikakk.ee, bin.meng@windriver.com, michal.simek@xilinx.com, wasim.khan@nxp.com, ye.li@nxp.com, igor.opaniuk@foundries.io, sr@denx.de, takahiro.akashi@linaro.org, vabhav.sharma@nxp.com, weijie.gao@mediatek.com, p.yadav@ti.com, andriy.shevchenko@linux.intel.com In-Reply-To: (message from Simon Glass on Sun, 19 Sep 2021 21:15:59 -0600) Subject: Re: [PATCH 2/5] serial: s5p: Add Apple M1 support References: <20210918135437.36667-1-kettenis@openbsd.org> <20210918135437.36667-3-kettenis@openbsd.org> Message-ID: X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean > From: Simon Glass > Date: Sun, 19 Sep 2021 21:15:59 -0600 > > Hi Mark, > > On Sat, 18 Sept 2021 at 07:55, Mark Kettenis 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 > > --- > > 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 > > + */ > > + > > +#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 > > + * Heungjun Kim > > + */ > > + > > +#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.