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 X-Spam-Level: X-Spam-Status: No, score=-0.7 required=3.0 tests=BAYES_00,DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED,DKIM_VALID,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D960AC433DB for ; Fri, 5 Mar 2021 15:30:58 +0000 (UTC) Received: from desiato.infradead.org (desiato.infradead.org [90.155.92.199]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 4F9F165015 for ; Fri, 5 Mar 2021 15:30:58 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4F9F165015 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=desiato.20200630; h=Sender:Content-Transfer-Encoding :Content-Type:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From:In-Reply-To: References:MIME-Version:Reply-To:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=qDbXnY3FzqKfYgNKaXF4nnqmiIAzplN4b1yv/ZahAzU=; b=Lf3mz7KG1XeyOosONE2T2cIXu /VjIGfGOV89ZNF+mPAZbntGWXJHAWgYmPa2P2gj4dSGUeJmwdxqoL7+/UgqxWpB1Hp93M2Zo8u7+Z l931iotFuWsHW7FV3uu/fb5Z/tKqhHvLJW2c7o03/NCy1ONfhqx/SugJxHKA6jn9pon3hpoGgAfsn vWCh/kf53e3qgXNHxDO3TUfGRFn+TSu3lxsrfoPkrjDYpzbqTfBGrX4PULhHOkEd77BRvYXaIyRqB mJ197zXsNnnlJcgpOHVvnKx8V7IlLuBfIjviVKPqpChmLzfRuz6i0fAxg6wHaSDq6S/v8V54sMUVj P5DtTphgA==; Received: from localhost ([::1] helo=desiato.infradead.org) by desiato.infradead.org with esmtp (Exim 4.94 #2 (Red Hat Linux)) id 1lICNk-00FRww-ET; Fri, 05 Mar 2021 15:28:48 +0000 Received: from mail-pf1-x436.google.com ([2607:f8b0:4864:20::436]) by desiato.infradead.org with esmtps (Exim 4.94 #2 (Red Hat Linux)) id 1lICNZ-00FRtB-NZ for linux-arm-kernel@lists.infradead.org; Fri, 05 Mar 2021 15:28:40 +0000 Received: by mail-pf1-x436.google.com with SMTP id l7so2405896pfd.3 for ; Fri, 05 Mar 2021 07:28:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=uLYLsu/Tg277ivzbocTilYLO6uuOWC+beELR4myHiWQ=; b=PKQndGo6w0Ga2p/+wx7fUyr32VdMqomj8GnE5drMTXxNKulXVHiQgWgQN/5s1N5Ugj cg/zNGtkrfTYp5YxtSV9FnoE2ygs6l5OAsiH5DM5PHYha/wQMB4slK1/R0e7H7K3U4xu oPMNqnqn5ftxZnsS4rbjRvo/rLhyfgfXglFf0qTilnf0gAhEeeNyKGCnMRTByc6+61sa M77lUUQAhjOdlV1FbDtgcIYW7lfMQ3p/tScT13i0sU/nALgX5UR9qP5f9zdmdoikk89f DHsVHMIhk84iNaoqgPvW9EXZaoF6XthoOXV6KT9e9xekx/nuGQkBfyLPGkXTeTohXhkJ 09Fw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=uLYLsu/Tg277ivzbocTilYLO6uuOWC+beELR4myHiWQ=; b=BTubYHnRH8MI5OvWPrS0oTeq/XuHax+SUId8ZomXT4VYDCfUKx2N7v3VC2TnMkpAZd QbogYYSNET/cDzPWCLnuKGaJAgt3J8AFm+UTAmR4eU9F5Zer5PdeRZyDpqiU3+MrB2Lj xMtHJhf9Ga+bNwuuE8RuzEWYdSO+hMfiLZ4Q+Vj+cmhSojAeUq4+tYbd8GwokpQoR71W OVTdnUcQE03O5ERy4sIfv7SPQz4NJw1nhTkIIZmhypX4HZU89877GvcqyI2pfKDfwYdq ZYDzbSFU2YMNoVpLzB7tm7U01oRbZbK45n6wWjOeEkGvkWEelpr417AcwotGCwNlaKRx wK0w== X-Gm-Message-State: AOAM531cjUiiA4jTkEjubQlWO8aJNpPfeaJdilmL+beHVTaIUQudsdlb UTAkDhOEsc6GRXjuvl+qLxQt7PrfzOKsidlZRNg= X-Google-Smtp-Source: ABdhPJz3vL0uJ4Zj/ECoBh9S5qEinAHiBZJDk5ytq25zAnnCL2NyXtBzVn+oUmpLroD9lE3sdevKw1cwYGm8/Kw7Rbc= X-Received: by 2002:a63:ce15:: with SMTP id y21mr9256094pgf.4.1614958116086; Fri, 05 Mar 2021 07:28:36 -0800 (PST) MIME-Version: 1.0 References: <20210304213902.83903-1-marcan@marcan.st> <20210304213902.83903-25-marcan@marcan.st> In-Reply-To: <20210304213902.83903-25-marcan@marcan.st> From: Andy Shevchenko Date: Fri, 5 Mar 2021 17:28:19 +0200 Message-ID: Subject: Re: [RFT PATCH v3 24/27] tty: serial: samsung_tty: Add support for Apple UARTs To: Hector Martin Cc: linux-arm Mailing List , Marc Zyngier , Rob Herring , Arnd Bergmann , Olof Johansson , Krzysztof Kozlowski , Mark Kettenis , Tony Lindgren , Mohamed Mediouni , Stan Skowronek , Alexander Graf , Will Deacon , Linus Walleij , Mark Rutland , Greg Kroah-Hartman , Jonathan Corbet , Catalin Marinas , Christoph Hellwig , "David S. Miller" , devicetree , "open list:SERIAL DRIVERS" , Linux Documentation List , Linux Samsung SOC , Linux-Arch , Linux Kernel Mailing List X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210305_152838_373746_7448A2DB X-CRM114-Status: GOOD ( 27.32 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, Mar 4, 2021 at 11:42 PM Hector Martin wrote: > > Apple SoCs are a distant descendant of Samsung designs and use yet > another variant of their UART style, with different interrupt handling. > > In particular, this variant has the following differences with existing > ones: > > * It includes a built-in interrupt controller with different registers, > using only a single platform IRQ > > * Internal interrupt sources are treated as edge-triggered, even though > the IRQ output is level-triggered. This chiefly affects the TX IRQ > path: the driver can no longer rely on the TX buffer empty IRQ > immediately firing after TX is enabled, but instead must prime the > FIFO with data directly. ... > + case TYPE_APPLE_S5L: > + WARN_ON(1); // No DMA Oh, no, please use the ONCE variant. ... > + /* Apple types use these bits for IRQ masks */ > + if (ourport->info->type != TYPE_APPLE_S5L) { > + ucon &= ~(S3C64XX_UCON_TIMEOUT_MASK | > + S3C64XX_UCON_EMPTYINT_EN | > + S3C64XX_UCON_DMASUS_EN | > + S3C64XX_UCON_TIMEOUT_EN); > + ucon |= 0xf << S3C64XX_UCON_TIMEOUT_SHIFT | Can you spell 0xf with named constant(s), please? In case they are repetitive via the code, introduce either a temporary variable (in case it scoped to one function only), or define it as a constant. > + S3C64XX_UCON_TIMEOUT_EN; > + } ... > +/* interrupt handler for Apple SoC's.*/ > +static irqreturn_t apple_serial_handle_irq(int irq, void *id) > +{ > + struct s3c24xx_uart_port *ourport = id; > + struct uart_port *port = &ourport->port; > + unsigned int pend = rd_regl(port, S3C2410_UTRSTAT); > + irqreturn_t ret = IRQ_NONE; Redundant. You may return directly. > + > + if (pend & (APPLE_S5L_UTRSTAT_RXTHRESH | APPLE_S5L_UTRSTAT_RXTO)) { > + wr_regl(port, S3C2410_UTRSTAT, > + APPLE_S5L_UTRSTAT_RXTHRESH | APPLE_S5L_UTRSTAT_RXTO); > + ret = s3c24xx_serial_rx_irq(irq, id); > + } > + if (pend & APPLE_S5L_UTRSTAT_TXTHRESH) { > + wr_regl(port, S3C2410_UTRSTAT, APPLE_S5L_UTRSTAT_TXTHRESH); > + ret = s3c24xx_serial_tx_irq(irq, id); > + } No IO serialization? > + return ret; > +} ... > +static void apple_s5l_serial_shutdown(struct uart_port *port) > +{ > + struct s3c24xx_uart_port *ourport = to_ourport(port); > + Extra blank line (check your entire series for a such) > + unsigned int ucon; > + ourport->tx_in_progress = 0; > +} ... > + ourport->rx_enabled = 1; > + ourport->tx_enabled = 0; How are these protected against race? ... > + case TYPE_APPLE_S5L: { > + unsigned int ucon; > + int ret; > + > + ret = clk_prepare_enable(ourport->clk); > + if (ret) { > + dev_err(dev, "clk_enable clk failed: %d\n", ret); > + return ret; > + } > + if (!IS_ERR(ourport->baudclk)) { > + ret = clk_prepare_enable(ourport->baudclk); > + if (ret) { > + dev_err(dev, "clk_enable baudclk failed: %d\n", ret); > + clk_disable_unprepare(ourport->clk); > + return ret; > + } > + } Wouldn't it be better to use CLK bulk API? > + ucon = rd_regl(port, S3C2410_UCON); > + > + ucon &= ~(APPLE_S5L_UCON_TXTHRESH_ENA_MSK | > + APPLE_S5L_UCON_RXTHRESH_ENA_MSK | > + APPLE_S5L_UCON_RXTO_ENA_MSK); > + > + if (ourport->tx_enabled) > + ucon |= APPLE_S5L_UCON_TXTHRESH_ENA_MSK; > + if (ourport->rx_enabled) > + ucon |= APPLE_S5L_UCON_RXTHRESH_ENA_MSK | > + APPLE_S5L_UCON_RXTO_ENA_MSK; > + > + wr_regl(port, S3C2410_UCON, ucon); > + > + if (!IS_ERR(ourport->baudclk)) > + clk_disable_unprepare(ourport->baudclk); > + clk_disable_unprepare(ourport->clk); > + break; > + } ... > +#ifdef CONFIG_ARCH_APPLE Why? Wouldn't you like the one kernel to work on many SoCs? > +static struct s3c24xx_serial_drv_data s5l_serial_drv_data = { > + .info = &(struct s3c24xx_uart_info) { > + .name = "Apple S5L UART", > + .type = TYPE_APPLE_S5L, > + .port_type = PORT_8250, > + .fifosize = 16, > + .rx_fifomask = S3C2410_UFSTAT_RXMASK, > + .rx_fifoshift = S3C2410_UFSTAT_RXSHIFT, > + .rx_fifofull = S3C2410_UFSTAT_RXFULL, > + .tx_fifofull = S3C2410_UFSTAT_TXFULL, > + .tx_fifomask = S3C2410_UFSTAT_TXMASK, > + .tx_fifoshift = S3C2410_UFSTAT_TXSHIFT, > + .def_clk_sel = S3C2410_UCON_CLKSEL0, > + .num_clks = 1, > + .clksel_mask = 0, > + .clksel_shift = 0, > + }, > + .def_cfg = &(struct s3c2410_uartcfg) { > + .ucon = APPLE_S5L_UCON_DEFAULT, > + .ufcon = S3C2410_UFCON_DEFAULT, > + }, > +}; > +#define S5L_SERIAL_DRV_DATA ((kernel_ulong_t)&s5l_serial_drv_data) > +#else > +#define S5L_SERIAL_DRV_DATA ((kernel_ulong_t)NULL) > +#endif ... > +#define APPLE_S5L_UCON_RXTO_ENA_MSK (1 << APPLE_S5L_UCON_RXTO_ENA) > +#define APPLE_S5L_UCON_RXTHRESH_ENA_MSK (1 << APPLE_S5L_UCON_RXTHRESH_ENA) > +#define APPLE_S5L_UCON_TXTHRESH_ENA_MSK (1 << APPLE_S5L_UCON_TXTHRESH_ENA) BIT() ? ... > +#define APPLE_S5L_UCON_DEFAULT (S3C2410_UCON_TXIRQMODE | \ > + S3C2410_UCON_RXIRQMODE | \ > + S3C2410_UCON_RXFIFO_TOI) Indentation level is too high. Hint: start a value of the definition on the new line. ... > +#define APPLE_S5L_UTRSTAT_RXTHRESH (1<<4) > +#define APPLE_S5L_UTRSTAT_TXTHRESH (1<<5) > +#define APPLE_S5L_UTRSTAT_RXTO (1<<9) > +#define APPLE_S5L_UTRSTAT_ALL_FLAGS (0x3f0) BIT() ? -- With Best Regards, Andy Shevchenko _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel