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 83997C433EF for ; Sun, 3 Oct 2021 02:01:28 +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 857A96126A for ; Sun, 3 Oct 2021 02:01:27 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 857A96126A Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=chromium.org 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 AB60D81D1F; Sun, 3 Oct 2021 04:01:24 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.b="ampP6yux"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id B8E2581D1F; Sun, 3 Oct 2021 04:01:21 +0200 (CEST) Received: from mail-vs1-xe30.google.com (mail-vs1-xe30.google.com [IPv6:2607:f8b0:4864:20::e30]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id B5558812B6 for ; Sun, 3 Oct 2021 04:01:16 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=sjg@google.com Received: by mail-vs1-xe30.google.com with SMTP id y141so23257vsy.5 for ; Sat, 02 Oct 2021 19:01:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=p8xuFy/zxW18MHT84X7+2VLOuvp+sO0wv0z7ZJ5wVVE=; b=ampP6yux6cMTeMIBDR+YZ3u1+9NRnGzEMVaWHFkOLXkrrzRxzvDXGQise9AaFEG1Me 97EvtoAylyDn78JkwlbgEvy0XMN/E9IO6q1y9IsmVUEDRaCGS6YcMsJ0QYWmXXUYATLP nSKehb0TMt9Fqw3I/pp1N+s0WmOX4GzVAy5Jg= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=p8xuFy/zxW18MHT84X7+2VLOuvp+sO0wv0z7ZJ5wVVE=; b=F7YP+kPqcEy/pFwckVsTjJTly0ER9HYlzRBSOM4RRfd/vjqtYMWbcbQS0s3ySGvDL+ PXjSVdkNvEHL/CTTsyaczo7AVhA7pJcfZ9uNRc9fbIwU8vziGhL6mUPEBB7MsCtGHSYJ CkpgJ2qdQZbmjuQBH0IDbw9TcvcLl/dFLGG3SZ8bhGO7EAjnmL38Y7jp5XWzPqdPQMgh eM4ydwIIPAOVdWEX0MpdUg6B3N7aooCcLvvSepUmYtU5n84+cFn4yaip3zjYCXdySt2O AZ6ZGTrbo9HHYR5mRVrOfSodMOKOqrdQZezPvkD5RmYqi/u/29K9hTuNrS62D3wrfikY LO1A== X-Gm-Message-State: AOAM533ZQtHZUNcKC2wAvxNG9prNTo8hU/Ir8ObkCmUO5eurwgu316SH eZylrhf9hqr5kvzhLyqlg0YxrsfM8h035/yIik2UOw== X-Google-Smtp-Source: ABdhPJxzsIDdSCD0Llf30Gr8sJeSFeMbXugOiXU34XBDpFr9OOiaLebCJtbzyjdYJ6HBUGpy4oiqkZl6oZoBmgeHt6s= X-Received: by 2002:a05:6102:457:: with SMTP id e23mr9285634vsq.8.1633226474848; Sat, 02 Oct 2021 19:01:14 -0700 (PDT) MIME-Version: 1.0 References: <20210918135437.36667-1-kettenis@openbsd.org> <20210918135437.36667-3-kettenis@openbsd.org> In-Reply-To: From: Simon Glass Date: Sat, 2 Oct 2021 20:01:03 -0600 Message-ID: Subject: Re: [PATCH 2/5] serial: s5p: Add Apple M1 support To: Mark Kettenis Cc: Mark Kettenis , U-Boot Mailing List , Oliver Graute , Leo Liang , Kishon Vijay Abraham I , Priyanka Jain , Stephan Gerhold , Padmarao Begari , Tianrui Wei , Michael Walle , Heinrich Schuchardt , Asherah Connor , Bin Meng , Michal Simek , Wasim Khan , Ye Li , Igor Opaniuk , Stefan Roese , AKASHI Takahiro , Vabhav Sharma , Weijie Gao , Pratyush Yadav , Andy Shevchenko Content-Type: text/plain; charset="UTF-8" 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 Hi Mark, On Sat, 2 Oct 2021 at 16:16, Mark Kettenis wrote: > > > 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. 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