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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8B915C433F5 for ; Tue, 1 Mar 2022 08:32:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233363AbiCAIdM (ORCPT ); Tue, 1 Mar 2022 03:33:12 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59588 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231576AbiCAIdI (ORCPT ); Tue, 1 Mar 2022 03:33:08 -0500 Received: from mail-ed1-x534.google.com (mail-ed1-x534.google.com [IPv6:2a00:1450:4864:20::534]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5B668396 for ; Tue, 1 Mar 2022 00:32:25 -0800 (PST) Received: by mail-ed1-x534.google.com with SMTP id f8so8811377edf.10 for ; Tue, 01 Mar 2022 00:32:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20210112.gappssmtp.com; s=20210112; h=references:user-agent:from:to:cc:subject:date:in-reply-to :message-id:mime-version; bh=nsUATk/2y14ChiOIn5CE+LTlgsRPR3DrNuBrS7JUbrQ=; b=bYnSfzfZPfCOIKN8Krthey4XMcGw2QKgC/LcWSj5nf7IJYtupnKl4dFnvn/oZJ0DJB pXJYJHK79APoUJTtn6e9ypwUH8KZQ1uBSg4I6Qp6maILe6xIPOnv1hxpWR7LfXYmV8JS LlSv/aFSvjleKM9FcPa7oojCWgSDjptlEsyPmwOlavHAOFVzg5zhqAk/O3ggkYOrMVVj awTMEOSlRt3NSZgshmKVQDjt2n45GY8H7cbf9BQFufFWl4TqVsFVMKJLRoGJQ2zVi9IN uA6UjX0tbaNAELz45jUapuk5dekpctdos/KxrIGfHjBh9ol1/vlqbQ2sDMKwsUoLV95t CUnA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:references:user-agent:from:to:cc:subject:date :in-reply-to:message-id:mime-version; bh=nsUATk/2y14ChiOIn5CE+LTlgsRPR3DrNuBrS7JUbrQ=; b=F2d0zW+33ygc+RIUmDngqi5tRjwKdcwT6aeqhzGXqElahIpm0Gi6GmDyAQ/n0k+OP7 /huXMNqNcmCnOzvj+lgMaIkjA5Wr5+G5dX7wd8ZBYVa+89T5myCNYRYLKgDCxQQ/ZFeP mke73neyvTWblcmRb9JOqkbJPRvFPg0qh+LDGEFQoFHoKGRdr4KqmxXzPQcjzDBXQiWa P2+Lc3PD98sehjr5cAePyP4z4HGKtL0e9BPRUxE12sEH5rOjowP+GHfXl/OGjNebiZNm omR6h8A/fpcr52/lgcq4yiSRuhik+DKLYPTtNCMzMVy4ldCsKtp6BXzfCVnq0jczkC1y YxYQ== X-Gm-Message-State: AOAM532Gwb8ffHCZMWG2j1vL4Bd6Jbm79U+VNhnmZKSkj43N/h+PBvnV kXXfESLWoeaIyrffz0HdQjfbT5c+CIblmg== X-Google-Smtp-Source: ABdhPJzrUHBJAZzS80YmptY0nPErjjPOM4jEHxZBwzUH8W3hc3yhJhzX4ceWFL2nT3TUbkIPc2/GWA== X-Received: by 2002:a50:aa8c:0:b0:410:801c:4e2f with SMTP id q12-20020a50aa8c000000b00410801c4e2fmr22932513edc.179.1646123543706; Tue, 01 Mar 2022 00:32:23 -0800 (PST) Received: from localhost (82-65-169-74.subs.proxad.net. [82.65.169.74]) by smtp.gmail.com with ESMTPSA id q9-20020a17090609a900b006cd30a3c4f0sm5167184eje.147.2022.03.01.00.32.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 01 Mar 2022 00:32:23 -0800 (PST) References: <20220225073922.3947-1-yu.tu@amlogic.com> <20220225073922.3947-4-yu.tu@amlogic.com> <1j4k4jxmji.fsf@starbuckisacylon.baylibre.com> User-agent: mu4e 1.6.10; emacs 27.1 From: Jerome Brunet To: Yu Tu , linux-serial@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org Cc: Greg Kroah-Hartman , Jiri Slaby , Neil Armstrong , Kevin Hilman , Martin Blumenstingl Subject: Re: [PATCH V7 3/6] tty: serial: meson: Describes the calculation of the UART baud rate clock using a clock frame Date: Tue, 01 Mar 2022 09:26:38 +0100 In-reply-to: Message-ID: <1jr17mvzse.fsf@starbuckisacylon.baylibre.com> MIME-Version: 1.0 Content-Type: text/plain Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue 01 Mar 2022 at 14:49, Yu Tu wrote: > Hi Jerome, > > On 2022/2/28 19:10, Jerome Brunet wrote: >> [ EXTERNAL EMAIL ] >> >> On Fri 25 Feb 2022 at 15:39, Yu Tu wrote: >> >>> Using the common Clock code to describe the UART baud rate clock >>> makes it easier for the UART driver to be compatible with the >>> baud rate requirements of the UART IP on different meson chips. >>> >>> Signed-off-by: Yu Tu >>> --- >>> drivers/tty/serial/meson_uart.c | 194 +++++++++++++++++++++++--------- >>> 1 file changed, 142 insertions(+), 52 deletions(-) >>> >>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c >>> index 7570958d010c..4768d51fac70 100644 >>> --- a/drivers/tty/serial/meson_uart.c >>> +++ b/drivers/tty/serial/meson_uart.c >>> @@ -6,6 +6,7 @@ >>> */ >>> #include >>> +#include >>> #include >>> #include >>> #include >>> @@ -65,9 +66,7 @@ >>> #define AML_UART_RECV_IRQ(c) ((c) & 0xff) >>> /* AML_UART_REG5 bits */ >>> -#define AML_UART_BAUD_MASK 0x7fffff >>> #define AML_UART_BAUD_USE BIT(23) >>> -#define AML_UART_BAUD_XTAL BIT(24) >>> #define AML_UART_PORT_NUM 12 >>> #define AML_UART_PORT_OFFSET 6 >>> @@ -76,6 +75,11 @@ >>> #define AML_UART_POLL_USEC 5 >>> #define AML_UART_TIMEOUT_USEC 10000 >>> +struct meson_uart_data { >>> + struct clk *baud_clk; >>> + bool use_xtal_clk; >>> +}; >>> + >>> static struct uart_driver meson_uart_driver; >>> static struct uart_port *meson_ports[AML_UART_PORT_NUM]; >>> @@ -293,19 +297,17 @@ static int meson_uart_startup(struct uart_port *port) >>> static void meson_uart_change_speed(struct uart_port *port, unsigned >>> long baud) >>> { >>> + struct meson_uart_data *private_data = port->private_data; >>> u32 val; >>> while (!meson_uart_tx_empty(port)) >>> cpu_relax(); >>> - if (port->uartclk == 24000000) { >>> - val = ((port->uartclk / 3) / baud) - 1; >>> - val |= AML_UART_BAUD_XTAL; >>> - } else { >>> - val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1; >>> - } >>> + val = readl(port->membase + AML_UART_REG5); >>> val |= AML_UART_BAUD_USE; >>> writel(val, port->membase + AML_UART_REG5); >>> + >>> + clk_set_rate(private_data->baud_clk, baud); >>> } >>> static void meson_uart_set_termios(struct uart_port *port, >>> @@ -395,11 +397,20 @@ static int meson_uart_verify_port(struct uart_port *port, >>> static void meson_uart_release_port(struct uart_port *port) >>> { >>> - /* nothing to do */ >>> + struct meson_uart_data *private_data = port->private_data; >>> + >>> + clk_disable_unprepare(private_data->baud_clk); >>> } >>> static int meson_uart_request_port(struct uart_port *port) >>> { >>> + struct meson_uart_data *private_data = port->private_data; >>> + int ret; >>> + >>> + ret = clk_prepare_enable(private_data->baud_clk); >>> + if (ret) >>> + return ret; >>> + >> I think we already discussed that. This is yet another behavior change >> Previously, enabling the clock was done at probe time. >> It's fine to change it, if there is a justification, but not in the same >> change as the rework of the divider >> >>> return 0; >>> } >>> @@ -629,57 +640,106 @@ static struct uart_driver meson_uart_driver = { >>> .cons = MESON_SERIAL_CONSOLE, >>> }; >>> -static inline struct clk *meson_uart_probe_clock(struct device *dev, >>> - const char *id) >>> -{ >>> - struct clk *clk = NULL; >>> - int ret; >>> - >>> - clk = devm_clk_get(dev, id); >>> - if (IS_ERR(clk)) >>> - return clk; >>> - >>> - ret = clk_prepare_enable(clk); >>> - if (ret) { >>> - dev_err(dev, "couldn't enable clk\n"); >>> - return ERR_PTR(ret); >>> - } >>> +static const struct clk_div_table xtal_div_table[] = { >>> + { 0, 3 }, >>> + { 1, 1 }, >>> + { 2, 2 }, >>> + { 3, 2 }, >>> +}; >>> - devm_add_action_or_reset(dev, >>> - (void(*)(void *))clk_disable_unprepare, >>> - clk); >>> +static u32 use_xtal_mux_table; >>> - return clk; >>> -} >>> - >>> -static int meson_uart_probe_clocks(struct platform_device *pdev, >>> - struct uart_port *port) >>> +static int meson_uart_probe_clocks(struct uart_port *port) >>> { >>> - struct clk *clk_xtal = NULL; >>> - struct clk *clk_pclk = NULL; >>> - struct clk *clk_baud = NULL; >>> + struct meson_uart_data *private_data = port->private_data; >>> + struct clk *clk_baud, *clk_xtal; >>> + struct clk_hw *hw, *clk81_div4_hw; >>> + char clk_name[32]; >>> + struct clk_parent_data use_xtal_mux_parents; >>> - clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk"); >>> - if (IS_ERR(clk_pclk)) >>> - return PTR_ERR(clk_pclk); >>> + clk_baud = devm_clk_get(port->dev, "baud"); >>> + if (IS_ERR(clk_baud)) { >>> + dev_err(port->dev, "Failed to get the 'baud' clock\n"); >>> + return PTR_ERR(clk_baud); >>> + } >> Calling devm_clk_get() would not be necessary if you used "fw_name" in >> the parent table. Same bellow > Personally, I think it is good that you can understand your meaning, but as > you are an expert in CCF, it is nice to write code in that way, but for > people who are not unfamiliar with CCF, they may only care about the use of > CCF.It's not pretty to use but it's easy to understand. There is no magic in CCF. Stephen and the other contributor took time to add the fw_name mechanism espcially for this. I'm suggesting and you are expected to actually look at the code and considerer it. Lack of "expertize" is not a valid argument. >> >>> - clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal"); >>> + clk_xtal = devm_clk_get(port->dev, "xtal"); >>> if (IS_ERR(clk_xtal)) >>> - return PTR_ERR(clk_xtal); >>> - >>> - clk_baud = meson_uart_probe_clock(&pdev->dev, "baud"); >>> - if (IS_ERR(clk_baud)) >>> - return PTR_ERR(clk_baud); >>> + return dev_err_probe(port->dev, PTR_ERR(clk_xtal), >>> + "Failed to get the 'xtal' clock\n"); >>> + >>> + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), >>> + "clk81_div4"); >>> + clk81_div4_hw = devm_clk_hw_register_fixed_factor(port->dev, >>> + clk_name, >>> + __clk_get_name(clk_baud), >>> + CLK_SET_RATE_NO_REPARENT, >>> + 1, 4); >>> + if (IS_ERR(clk81_div4_hw)) >>> + return PTR_ERR(clk81_div4_hw); >> So, whatever the chip type - you register a fixed 4 divider .... > As you suggested last time, this CLK has been stored, but some chips are > not used. If you have better suggestions, please let me know and I can > make corrections later. No, never suggested that. I suspected that 4 divider design was the same on all SoC version. You reported it was not, So I don't get this >> >>> + >>> + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), >>> + "xtal_div"); >>> + hw = devm_clk_hw_register_divider_table(port->dev, >>> + clk_name, >>> + __clk_get_name(clk_baud), >>> + CLK_SET_RATE_NO_REPARENT, >>> + port->membase + AML_UART_REG5, >>> + 26, 2, >>> + CLK_DIVIDER_READ_ONLY, >>> + xtal_div_table, NULL); >>> + if (IS_ERR(hw)) >>> + return PTR_ERR(hw); >>> + >>> + if (private_data->use_xtal_clk) { >>> + use_xtal_mux_table = 1; >>> + use_xtal_mux_parents.hw = hw; >>> + } else { >>> + use_xtal_mux_parents.hw = clk81_div4_hw; >> ... which you may end up not using in the end >> This is bad. > If you have better suggestions, please let me know and I can make > corrections later. >> >>> + } >>> - port->uartclk = clk_get_rate(clk_baud); >>> + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), >>> + "use_xtal"); >>> + hw = __devm_clk_hw_register_mux(port->dev, NULL, >>> + clk_name, >>> + 1, >>> + NULL, NULL, >>> + &use_xtal_mux_parents, >>> + CLK_SET_RATE_PARENT, >>> + port->membase + AML_UART_REG5, >>> + 24, 0x1, >>> + CLK_MUX_READ_ONLY, >>> + &use_xtal_mux_table, NULL); >>> + >>> + if (IS_ERR(hw)) >>> + return PTR_ERR(hw); >>> + >>> + port->uartclk = clk_hw_get_rate(hw); >>> + >>> + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), >>> + "baud_div"); >>> + hw = devm_clk_hw_register_divider(port->dev, >>> + clk_name, >>> + clk_hw_get_name(hw), >>> + CLK_SET_RATE_PARENT, >>> + port->membase + AML_UART_REG5, >>> + 0, 23, >>> + CLK_DIVIDER_ROUND_CLOSEST, >>> + NULL); >>> + if (IS_ERR(hw)) >>> + return PTR_ERR(hw); >>> + >>> + private_data->baud_clk = hw->clk; >>> return 0; >>> } >>> static int meson_uart_probe(struct platform_device *pdev) >>> { >>> + struct meson_uart_data *private_data; >>> struct resource *res_mem; >>> struct uart_port *port; >>> + struct clk *pclk; >>> u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */ >>> int ret = 0; >>> int irq; >>> @@ -705,6 +765,15 @@ static int meson_uart_probe(struct platform_device *pdev) >>> if (!res_mem) >>> return -ENODEV; >>> + pclk = devm_clk_get(&pdev->dev, "pclk"); >>> + if (IS_ERR(pclk)) >>> + return dev_err_probe(&pdev->dev, PTR_ERR(pclk), >>> + "Failed to get the 'pclk' clock\n"); >>> + >>> + ret = clk_prepare_enable(pclk); >>> + if (ret) >>> + return ret; >>> + >>> irq = platform_get_irq(pdev, 0); >>> if (irq < 0) >>> return irq; >>> @@ -724,9 +793,13 @@ static int meson_uart_probe(struct platform_device *pdev) >>> if (IS_ERR(port->membase)) >>> return PTR_ERR(port->membase); >>> - ret = meson_uart_probe_clocks(pdev, port); >>> - if (ret) >>> - return ret; >>> + private_data = devm_kzalloc(&pdev->dev, sizeof(*private_data), >>> + GFP_KERNEL); >>> + if (!private_data) >>> + return -ENOMEM; >>> + >>> + if (device_get_match_data(&pdev->dev)) >>> + private_data->use_xtal_clk = true; >> As long as the device matches a compatible below, the flag will end up >> 'true', regardless of values in the the dt_match table. >> I don't think this is the intended behavior. >> It highlights that proper testing of this series is important. >> Being at Amlogic, I'm sure you can test on more than just g12a and s4 >> > I believe with your experience this may be a real problem. I heard that > your company has our boards. If so, can you help verify it? >>> port->iotype = UPIO_MEM; >>> port->mapbase = res_mem->start; >>> @@ -740,6 +813,11 @@ static int meson_uart_probe(struct platform_device *pdev) >>> port->x_char = 0; >>> port->ops = &meson_uart_ops; >>> port->fifosize = fifosize; >>> + port->private_data = private_data; >>> + >>> + ret = meson_uart_probe_clocks(port); >>> + if (ret) >>> + return ret; >>> meson_ports[pdev->id] = port; >>> platform_set_drvdata(pdev, port); >>> @@ -766,10 +844,22 @@ static int meson_uart_remove(struct platform_device *pdev) >>> } >>> static const struct of_device_id meson_uart_dt_match[] = { >>> - { .compatible = "amlogic,meson6-uart" }, >>> - { .compatible = "amlogic,meson8-uart" }, >>> - { .compatible = "amlogic,meson8b-uart" }, >>> - { .compatible = "amlogic,meson-gx-uart" }, >>> + { >>> + .compatible = "amlogic,meson6-uart", >>> + .data = (void *)false, >>> + }, >>> + { >>> + .compatible = "amlogic,meson8-uart", >>> + .data = (void *)false, >>> + }, >>> + { >>> + .compatible = "amlogic,meson8b-uart", >>> + .data = (void *)false, >>> + }, >>> + { >>> + .compatible = "amlogic,meson-gx-uart", >>> + .data = (void *)true, >>> + }, >>> { /* sentinel */ }, >>> }; >>> MODULE_DEVICE_TABLE(of, meson_uart_dt_match); >> 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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 5E837C433EF for ; Tue, 1 Mar 2022 08:32:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:Message-ID:In-reply-to: Date:Subject:Cc:To:From:References:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=a0XzZ980LrnMrTyZGzLTqMgrKe0u1rV29xRn4FRv6Jo=; b=VgqabeX4SOL/r1 3sKNgresbsDKNDqT23l6ppnvDGaL04Et53/eSmxHNXM9E/aN7DRFKZYLSdW/HSWvDSd6XK/Tj/55t /IaBUvoy6320N/D+ygxFY5s7ETvG+LkoxfAlV3Of9FvSyA79Wkveat7rNV1INGD0EZjF1twZKdL4e Wlq9BzXelZLVJoO5bNhLaicqfwZWoUe5yMv6gvW6JgM+yxDGWYB3B/3U2OgeGUJgHh+gXia98jxbw LkTaa36bOUCcAy4omPv5o16+WMy0S6A5v5nsyFT5+WfJ6/4ZO7/rOxeT8DGc/GBWAM+gyptmlHYSj GfUqdDhQUzFWbT+gqRrA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nOxvp-00Fb0W-Sc; Tue, 01 Mar 2022 08:32:29 +0000 Received: from mail-ed1-x531.google.com ([2a00:1450:4864:20::531]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nOxvl-00FazW-Oy for linux-amlogic@lists.infradead.org; Tue, 01 Mar 2022 08:32:28 +0000 Received: by mail-ed1-x531.google.com with SMTP id w3so20933150edu.8 for ; Tue, 01 Mar 2022 00:32:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20210112.gappssmtp.com; s=20210112; h=references:user-agent:from:to:cc:subject:date:in-reply-to :message-id:mime-version; bh=nsUATk/2y14ChiOIn5CE+LTlgsRPR3DrNuBrS7JUbrQ=; b=bYnSfzfZPfCOIKN8Krthey4XMcGw2QKgC/LcWSj5nf7IJYtupnKl4dFnvn/oZJ0DJB pXJYJHK79APoUJTtn6e9ypwUH8KZQ1uBSg4I6Qp6maILe6xIPOnv1hxpWR7LfXYmV8JS LlSv/aFSvjleKM9FcPa7oojCWgSDjptlEsyPmwOlavHAOFVzg5zhqAk/O3ggkYOrMVVj awTMEOSlRt3NSZgshmKVQDjt2n45GY8H7cbf9BQFufFWl4TqVsFVMKJLRoGJQ2zVi9IN uA6UjX0tbaNAELz45jUapuk5dekpctdos/KxrIGfHjBh9ol1/vlqbQ2sDMKwsUoLV95t CUnA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:references:user-agent:from:to:cc:subject:date :in-reply-to:message-id:mime-version; bh=nsUATk/2y14ChiOIn5CE+LTlgsRPR3DrNuBrS7JUbrQ=; b=CENquIMk5J2cgZNp+u8R2+GXrNcGFmp3TvIexrp5926f1hTswCFjuYL0ZqrkAJGTPq WzFS6O33keeMxwx+GlSWmjc0zzWPFbKIYe+T6E77bn2zuhDBIMUT/nmD9kViD/D+PG+N OjAz/rzO6vY8IJyrkHaa9Boe17i2yk9pyZ0t4Ut1nAYzKY9moXTQ3cRWxjGxL3VXvlNK JpgGqE6RJuXmOCt4CxxNj6lGOw74mWQ5BHWBMJsiiLhqMsL75FZexTepEHM60r8mGpmB whbW3Dff90qfEOhQtpGG2v0eq2Wbj94Yfigpj3nQYxOpBpktG0Pxe1zqKmjk0g7pj1v5 3zFA== X-Gm-Message-State: AOAM530QNMmbIhruqMNJzQNmvTSZfjlY6HPcPu6kp+OEqZnqsCoxvzd0 0t5aOt2IPrSL1M+IZjNGsinCyQ== X-Google-Smtp-Source: ABdhPJzrUHBJAZzS80YmptY0nPErjjPOM4jEHxZBwzUH8W3hc3yhJhzX4ceWFL2nT3TUbkIPc2/GWA== X-Received: by 2002:a50:aa8c:0:b0:410:801c:4e2f with SMTP id q12-20020a50aa8c000000b00410801c4e2fmr22932513edc.179.1646123543706; Tue, 01 Mar 2022 00:32:23 -0800 (PST) Received: from localhost (82-65-169-74.subs.proxad.net. [82.65.169.74]) by smtp.gmail.com with ESMTPSA id q9-20020a17090609a900b006cd30a3c4f0sm5167184eje.147.2022.03.01.00.32.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 01 Mar 2022 00:32:23 -0800 (PST) References: <20220225073922.3947-1-yu.tu@amlogic.com> <20220225073922.3947-4-yu.tu@amlogic.com> <1j4k4jxmji.fsf@starbuckisacylon.baylibre.com> User-agent: mu4e 1.6.10; emacs 27.1 From: Jerome Brunet To: Yu Tu , linux-serial@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org Cc: Greg Kroah-Hartman , Jiri Slaby , Neil Armstrong , Kevin Hilman , Martin Blumenstingl Subject: Re: [PATCH V7 3/6] tty: serial: meson: Describes the calculation of the UART baud rate clock using a clock frame Date: Tue, 01 Mar 2022 09:26:38 +0100 In-reply-to: Message-ID: <1jr17mvzse.fsf@starbuckisacylon.baylibre.com> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220301_003225_944952_FE4EFD79 X-CRM114-Status: GOOD ( 36.24 ) X-BeenThere: linux-amlogic@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-amlogic" Errors-To: linux-amlogic-bounces+linux-amlogic=archiver.kernel.org@lists.infradead.org On Tue 01 Mar 2022 at 14:49, Yu Tu wrote: > Hi Jerome, > > On 2022/2/28 19:10, Jerome Brunet wrote: >> [ EXTERNAL EMAIL ] >> >> On Fri 25 Feb 2022 at 15:39, Yu Tu wrote: >> >>> Using the common Clock code to describe the UART baud rate clock >>> makes it easier for the UART driver to be compatible with the >>> baud rate requirements of the UART IP on different meson chips. >>> >>> Signed-off-by: Yu Tu >>> --- >>> drivers/tty/serial/meson_uart.c | 194 +++++++++++++++++++++++--------- >>> 1 file changed, 142 insertions(+), 52 deletions(-) >>> >>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c >>> index 7570958d010c..4768d51fac70 100644 >>> --- a/drivers/tty/serial/meson_uart.c >>> +++ b/drivers/tty/serial/meson_uart.c >>> @@ -6,6 +6,7 @@ >>> */ >>> #include >>> +#include >>> #include >>> #include >>> #include >>> @@ -65,9 +66,7 @@ >>> #define AML_UART_RECV_IRQ(c) ((c) & 0xff) >>> /* AML_UART_REG5 bits */ >>> -#define AML_UART_BAUD_MASK 0x7fffff >>> #define AML_UART_BAUD_USE BIT(23) >>> -#define AML_UART_BAUD_XTAL BIT(24) >>> #define AML_UART_PORT_NUM 12 >>> #define AML_UART_PORT_OFFSET 6 >>> @@ -76,6 +75,11 @@ >>> #define AML_UART_POLL_USEC 5 >>> #define AML_UART_TIMEOUT_USEC 10000 >>> +struct meson_uart_data { >>> + struct clk *baud_clk; >>> + bool use_xtal_clk; >>> +}; >>> + >>> static struct uart_driver meson_uart_driver; >>> static struct uart_port *meson_ports[AML_UART_PORT_NUM]; >>> @@ -293,19 +297,17 @@ static int meson_uart_startup(struct uart_port *port) >>> static void meson_uart_change_speed(struct uart_port *port, unsigned >>> long baud) >>> { >>> + struct meson_uart_data *private_data = port->private_data; >>> u32 val; >>> while (!meson_uart_tx_empty(port)) >>> cpu_relax(); >>> - if (port->uartclk == 24000000) { >>> - val = ((port->uartclk / 3) / baud) - 1; >>> - val |= AML_UART_BAUD_XTAL; >>> - } else { >>> - val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1; >>> - } >>> + val = readl(port->membase + AML_UART_REG5); >>> val |= AML_UART_BAUD_USE; >>> writel(val, port->membase + AML_UART_REG5); >>> + >>> + clk_set_rate(private_data->baud_clk, baud); >>> } >>> static void meson_uart_set_termios(struct uart_port *port, >>> @@ -395,11 +397,20 @@ static int meson_uart_verify_port(struct uart_port *port, >>> static void meson_uart_release_port(struct uart_port *port) >>> { >>> - /* nothing to do */ >>> + struct meson_uart_data *private_data = port->private_data; >>> + >>> + clk_disable_unprepare(private_data->baud_clk); >>> } >>> static int meson_uart_request_port(struct uart_port *port) >>> { >>> + struct meson_uart_data *private_data = port->private_data; >>> + int ret; >>> + >>> + ret = clk_prepare_enable(private_data->baud_clk); >>> + if (ret) >>> + return ret; >>> + >> I think we already discussed that. This is yet another behavior change >> Previously, enabling the clock was done at probe time. >> It's fine to change it, if there is a justification, but not in the same >> change as the rework of the divider >> >>> return 0; >>> } >>> @@ -629,57 +640,106 @@ static struct uart_driver meson_uart_driver = { >>> .cons = MESON_SERIAL_CONSOLE, >>> }; >>> -static inline struct clk *meson_uart_probe_clock(struct device *dev, >>> - const char *id) >>> -{ >>> - struct clk *clk = NULL; >>> - int ret; >>> - >>> - clk = devm_clk_get(dev, id); >>> - if (IS_ERR(clk)) >>> - return clk; >>> - >>> - ret = clk_prepare_enable(clk); >>> - if (ret) { >>> - dev_err(dev, "couldn't enable clk\n"); >>> - return ERR_PTR(ret); >>> - } >>> +static const struct clk_div_table xtal_div_table[] = { >>> + { 0, 3 }, >>> + { 1, 1 }, >>> + { 2, 2 }, >>> + { 3, 2 }, >>> +}; >>> - devm_add_action_or_reset(dev, >>> - (void(*)(void *))clk_disable_unprepare, >>> - clk); >>> +static u32 use_xtal_mux_table; >>> - return clk; >>> -} >>> - >>> -static int meson_uart_probe_clocks(struct platform_device *pdev, >>> - struct uart_port *port) >>> +static int meson_uart_probe_clocks(struct uart_port *port) >>> { >>> - struct clk *clk_xtal = NULL; >>> - struct clk *clk_pclk = NULL; >>> - struct clk *clk_baud = NULL; >>> + struct meson_uart_data *private_data = port->private_data; >>> + struct clk *clk_baud, *clk_xtal; >>> + struct clk_hw *hw, *clk81_div4_hw; >>> + char clk_name[32]; >>> + struct clk_parent_data use_xtal_mux_parents; >>> - clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk"); >>> - if (IS_ERR(clk_pclk)) >>> - return PTR_ERR(clk_pclk); >>> + clk_baud = devm_clk_get(port->dev, "baud"); >>> + if (IS_ERR(clk_baud)) { >>> + dev_err(port->dev, "Failed to get the 'baud' clock\n"); >>> + return PTR_ERR(clk_baud); >>> + } >> Calling devm_clk_get() would not be necessary if you used "fw_name" in >> the parent table. Same bellow > Personally, I think it is good that you can understand your meaning, but as > you are an expert in CCF, it is nice to write code in that way, but for > people who are not unfamiliar with CCF, they may only care about the use of > CCF.It's not pretty to use but it's easy to understand. There is no magic in CCF. Stephen and the other contributor took time to add the fw_name mechanism espcially for this. I'm suggesting and you are expected to actually look at the code and considerer it. Lack of "expertize" is not a valid argument. >> >>> - clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal"); >>> + clk_xtal = devm_clk_get(port->dev, "xtal"); >>> if (IS_ERR(clk_xtal)) >>> - return PTR_ERR(clk_xtal); >>> - >>> - clk_baud = meson_uart_probe_clock(&pdev->dev, "baud"); >>> - if (IS_ERR(clk_baud)) >>> - return PTR_ERR(clk_baud); >>> + return dev_err_probe(port->dev, PTR_ERR(clk_xtal), >>> + "Failed to get the 'xtal' clock\n"); >>> + >>> + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), >>> + "clk81_div4"); >>> + clk81_div4_hw = devm_clk_hw_register_fixed_factor(port->dev, >>> + clk_name, >>> + __clk_get_name(clk_baud), >>> + CLK_SET_RATE_NO_REPARENT, >>> + 1, 4); >>> + if (IS_ERR(clk81_div4_hw)) >>> + return PTR_ERR(clk81_div4_hw); >> So, whatever the chip type - you register a fixed 4 divider .... > As you suggested last time, this CLK has been stored, but some chips are > not used. If you have better suggestions, please let me know and I can > make corrections later. No, never suggested that. I suspected that 4 divider design was the same on all SoC version. You reported it was not, So I don't get this >> >>> + >>> + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), >>> + "xtal_div"); >>> + hw = devm_clk_hw_register_divider_table(port->dev, >>> + clk_name, >>> + __clk_get_name(clk_baud), >>> + CLK_SET_RATE_NO_REPARENT, >>> + port->membase + AML_UART_REG5, >>> + 26, 2, >>> + CLK_DIVIDER_READ_ONLY, >>> + xtal_div_table, NULL); >>> + if (IS_ERR(hw)) >>> + return PTR_ERR(hw); >>> + >>> + if (private_data->use_xtal_clk) { >>> + use_xtal_mux_table = 1; >>> + use_xtal_mux_parents.hw = hw; >>> + } else { >>> + use_xtal_mux_parents.hw = clk81_div4_hw; >> ... which you may end up not using in the end >> This is bad. > If you have better suggestions, please let me know and I can make > corrections later. >> >>> + } >>> - port->uartclk = clk_get_rate(clk_baud); >>> + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), >>> + "use_xtal"); >>> + hw = __devm_clk_hw_register_mux(port->dev, NULL, >>> + clk_name, >>> + 1, >>> + NULL, NULL, >>> + &use_xtal_mux_parents, >>> + CLK_SET_RATE_PARENT, >>> + port->membase + AML_UART_REG5, >>> + 24, 0x1, >>> + CLK_MUX_READ_ONLY, >>> + &use_xtal_mux_table, NULL); >>> + >>> + if (IS_ERR(hw)) >>> + return PTR_ERR(hw); >>> + >>> + port->uartclk = clk_hw_get_rate(hw); >>> + >>> + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), >>> + "baud_div"); >>> + hw = devm_clk_hw_register_divider(port->dev, >>> + clk_name, >>> + clk_hw_get_name(hw), >>> + CLK_SET_RATE_PARENT, >>> + port->membase + AML_UART_REG5, >>> + 0, 23, >>> + CLK_DIVIDER_ROUND_CLOSEST, >>> + NULL); >>> + if (IS_ERR(hw)) >>> + return PTR_ERR(hw); >>> + >>> + private_data->baud_clk = hw->clk; >>> return 0; >>> } >>> static int meson_uart_probe(struct platform_device *pdev) >>> { >>> + struct meson_uart_data *private_data; >>> struct resource *res_mem; >>> struct uart_port *port; >>> + struct clk *pclk; >>> u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */ >>> int ret = 0; >>> int irq; >>> @@ -705,6 +765,15 @@ static int meson_uart_probe(struct platform_device *pdev) >>> if (!res_mem) >>> return -ENODEV; >>> + pclk = devm_clk_get(&pdev->dev, "pclk"); >>> + if (IS_ERR(pclk)) >>> + return dev_err_probe(&pdev->dev, PTR_ERR(pclk), >>> + "Failed to get the 'pclk' clock\n"); >>> + >>> + ret = clk_prepare_enable(pclk); >>> + if (ret) >>> + return ret; >>> + >>> irq = platform_get_irq(pdev, 0); >>> if (irq < 0) >>> return irq; >>> @@ -724,9 +793,13 @@ static int meson_uart_probe(struct platform_device *pdev) >>> if (IS_ERR(port->membase)) >>> return PTR_ERR(port->membase); >>> - ret = meson_uart_probe_clocks(pdev, port); >>> - if (ret) >>> - return ret; >>> + private_data = devm_kzalloc(&pdev->dev, sizeof(*private_data), >>> + GFP_KERNEL); >>> + if (!private_data) >>> + return -ENOMEM; >>> + >>> + if (device_get_match_data(&pdev->dev)) >>> + private_data->use_xtal_clk = true; >> As long as the device matches a compatible below, the flag will end up >> 'true', regardless of values in the the dt_match table. >> I don't think this is the intended behavior. >> It highlights that proper testing of this series is important. >> Being at Amlogic, I'm sure you can test on more than just g12a and s4 >> > I believe with your experience this may be a real problem. I heard that > your company has our boards. If so, can you help verify it? >>> port->iotype = UPIO_MEM; >>> port->mapbase = res_mem->start; >>> @@ -740,6 +813,11 @@ static int meson_uart_probe(struct platform_device *pdev) >>> port->x_char = 0; >>> port->ops = &meson_uart_ops; >>> port->fifosize = fifosize; >>> + port->private_data = private_data; >>> + >>> + ret = meson_uart_probe_clocks(port); >>> + if (ret) >>> + return ret; >>> meson_ports[pdev->id] = port; >>> platform_set_drvdata(pdev, port); >>> @@ -766,10 +844,22 @@ static int meson_uart_remove(struct platform_device *pdev) >>> } >>> static const struct of_device_id meson_uart_dt_match[] = { >>> - { .compatible = "amlogic,meson6-uart" }, >>> - { .compatible = "amlogic,meson8-uart" }, >>> - { .compatible = "amlogic,meson8b-uart" }, >>> - { .compatible = "amlogic,meson-gx-uart" }, >>> + { >>> + .compatible = "amlogic,meson6-uart", >>> + .data = (void *)false, >>> + }, >>> + { >>> + .compatible = "amlogic,meson8-uart", >>> + .data = (void *)false, >>> + }, >>> + { >>> + .compatible = "amlogic,meson8b-uart", >>> + .data = (void *)false, >>> + }, >>> + { >>> + .compatible = "amlogic,meson-gx-uart", >>> + .data = (void *)true, >>> + }, >>> { /* sentinel */ }, >>> }; >>> MODULE_DEVICE_TABLE(of, meson_uart_dt_match); >> _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic 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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 17126C433FE for ; Tue, 1 Mar 2022 08:33:44 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:Message-ID:In-reply-to: Date:Subject:Cc:To:From:References:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=UOL/09vbd3UOwgVjR4ZSBs5wGio0CK5lI94biYaD8s0=; b=cb24l3SNK/LDyQ GEcH6tyjbH0NKZMHP4YK1EEFzUCQ2/vRBD98SlZFoWYeAvQrdJm4KBTd/TZLKM2VT84XzqDQ7XMd8 +JZKCkZDWec1e+x4+nJFMXrmEOW6K7+UR2iNkzGFqhDHrRu9wbdhAPGXbb46ovHEkE3r/Er+Pfz68 FdAoGSGbKsLScA9HsAlMGxryzL/ZCxhSUYmnHiED0LdPEPQgYFqTebV2rd+UDEOQ7WLCaAQveahDJ 17RBkKQdn131guXwnjAr4Ig/l9dvvBRMStFuN/X2kbUCXr4OKCrpBPLN5HyGnWjcYi3b97ySmtuGF KyxmMTXEz/TjUrqfYVNg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nOxvr-00Fb0f-9i; Tue, 01 Mar 2022 08:32:31 +0000 Received: from mail-ed1-x531.google.com ([2a00:1450:4864:20::531]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nOxvl-00FazV-Ot for linux-arm-kernel@lists.infradead.org; Tue, 01 Mar 2022 08:32:28 +0000 Received: by mail-ed1-x531.google.com with SMTP id h15so20958959edv.7 for ; Tue, 01 Mar 2022 00:32:25 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20210112.gappssmtp.com; s=20210112; h=references:user-agent:from:to:cc:subject:date:in-reply-to :message-id:mime-version; bh=nsUATk/2y14ChiOIn5CE+LTlgsRPR3DrNuBrS7JUbrQ=; b=bYnSfzfZPfCOIKN8Krthey4XMcGw2QKgC/LcWSj5nf7IJYtupnKl4dFnvn/oZJ0DJB pXJYJHK79APoUJTtn6e9ypwUH8KZQ1uBSg4I6Qp6maILe6xIPOnv1hxpWR7LfXYmV8JS LlSv/aFSvjleKM9FcPa7oojCWgSDjptlEsyPmwOlavHAOFVzg5zhqAk/O3ggkYOrMVVj awTMEOSlRt3NSZgshmKVQDjt2n45GY8H7cbf9BQFufFWl4TqVsFVMKJLRoGJQ2zVi9IN uA6UjX0tbaNAELz45jUapuk5dekpctdos/KxrIGfHjBh9ol1/vlqbQ2sDMKwsUoLV95t CUnA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:references:user-agent:from:to:cc:subject:date :in-reply-to:message-id:mime-version; bh=nsUATk/2y14ChiOIn5CE+LTlgsRPR3DrNuBrS7JUbrQ=; b=7q09ZJV0e5NJrbk3pBDZOqgx1C3IzNgQDHIpWhHnIoj8lK4BKILERXeUiOrU5GK3SH SwrZvdZnV6zDnTtEcgfRlz8aLOc/OBMb9Z1yUAIPTmjZMpO+EYrtIik0pc4ZHC7Cn3Yr UKsuDWmXRSXG1AYc2KCkB+M5lsb1CedwBwXCK6UsSoLcnAnKMwJHNpJGnaSxwMk9+wYs 62CtiEZqRI61gOuVAn80I8nOO9T7bSCSmSYld702run4bVbVGiEs2h4uw5F8whJ1In42 rYzTXhQhNHFOE4uQKVx6wI4uZT3qFRZq++Vuu5xavtDpnk3Hus9WbnlJGGC7l6G4yzJG 84GA== X-Gm-Message-State: AOAM532e8Ql4fQ/X9U8OPGja4Bi3cCct6RCR0uDde8o+PZBi2quS8CkG oe6v0pmx8C+K3LXgpnJuuT3+SA== X-Google-Smtp-Source: ABdhPJzrUHBJAZzS80YmptY0nPErjjPOM4jEHxZBwzUH8W3hc3yhJhzX4ceWFL2nT3TUbkIPc2/GWA== X-Received: by 2002:a50:aa8c:0:b0:410:801c:4e2f with SMTP id q12-20020a50aa8c000000b00410801c4e2fmr22932513edc.179.1646123543706; Tue, 01 Mar 2022 00:32:23 -0800 (PST) Received: from localhost (82-65-169-74.subs.proxad.net. [82.65.169.74]) by smtp.gmail.com with ESMTPSA id q9-20020a17090609a900b006cd30a3c4f0sm5167184eje.147.2022.03.01.00.32.23 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 01 Mar 2022 00:32:23 -0800 (PST) References: <20220225073922.3947-1-yu.tu@amlogic.com> <20220225073922.3947-4-yu.tu@amlogic.com> <1j4k4jxmji.fsf@starbuckisacylon.baylibre.com> User-agent: mu4e 1.6.10; emacs 27.1 From: Jerome Brunet To: Yu Tu , linux-serial@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org Cc: Greg Kroah-Hartman , Jiri Slaby , Neil Armstrong , Kevin Hilman , Martin Blumenstingl Subject: Re: [PATCH V7 3/6] tty: serial: meson: Describes the calculation of the UART baud rate clock using a clock frame Date: Tue, 01 Mar 2022 09:26:38 +0100 In-reply-to: Message-ID: <1jr17mvzse.fsf@starbuckisacylon.baylibre.com> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220301_003225_944394_60F9DBAE X-CRM114-Status: GOOD ( 37.87 ) 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 Tue 01 Mar 2022 at 14:49, Yu Tu wrote: > Hi Jerome, > > On 2022/2/28 19:10, Jerome Brunet wrote: >> [ EXTERNAL EMAIL ] >> >> On Fri 25 Feb 2022 at 15:39, Yu Tu wrote: >> >>> Using the common Clock code to describe the UART baud rate clock >>> makes it easier for the UART driver to be compatible with the >>> baud rate requirements of the UART IP on different meson chips. >>> >>> Signed-off-by: Yu Tu >>> --- >>> drivers/tty/serial/meson_uart.c | 194 +++++++++++++++++++++++--------- >>> 1 file changed, 142 insertions(+), 52 deletions(-) >>> >>> diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c >>> index 7570958d010c..4768d51fac70 100644 >>> --- a/drivers/tty/serial/meson_uart.c >>> +++ b/drivers/tty/serial/meson_uart.c >>> @@ -6,6 +6,7 @@ >>> */ >>> #include >>> +#include >>> #include >>> #include >>> #include >>> @@ -65,9 +66,7 @@ >>> #define AML_UART_RECV_IRQ(c) ((c) & 0xff) >>> /* AML_UART_REG5 bits */ >>> -#define AML_UART_BAUD_MASK 0x7fffff >>> #define AML_UART_BAUD_USE BIT(23) >>> -#define AML_UART_BAUD_XTAL BIT(24) >>> #define AML_UART_PORT_NUM 12 >>> #define AML_UART_PORT_OFFSET 6 >>> @@ -76,6 +75,11 @@ >>> #define AML_UART_POLL_USEC 5 >>> #define AML_UART_TIMEOUT_USEC 10000 >>> +struct meson_uart_data { >>> + struct clk *baud_clk; >>> + bool use_xtal_clk; >>> +}; >>> + >>> static struct uart_driver meson_uart_driver; >>> static struct uart_port *meson_ports[AML_UART_PORT_NUM]; >>> @@ -293,19 +297,17 @@ static int meson_uart_startup(struct uart_port *port) >>> static void meson_uart_change_speed(struct uart_port *port, unsigned >>> long baud) >>> { >>> + struct meson_uart_data *private_data = port->private_data; >>> u32 val; >>> while (!meson_uart_tx_empty(port)) >>> cpu_relax(); >>> - if (port->uartclk == 24000000) { >>> - val = ((port->uartclk / 3) / baud) - 1; >>> - val |= AML_UART_BAUD_XTAL; >>> - } else { >>> - val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1; >>> - } >>> + val = readl(port->membase + AML_UART_REG5); >>> val |= AML_UART_BAUD_USE; >>> writel(val, port->membase + AML_UART_REG5); >>> + >>> + clk_set_rate(private_data->baud_clk, baud); >>> } >>> static void meson_uart_set_termios(struct uart_port *port, >>> @@ -395,11 +397,20 @@ static int meson_uart_verify_port(struct uart_port *port, >>> static void meson_uart_release_port(struct uart_port *port) >>> { >>> - /* nothing to do */ >>> + struct meson_uart_data *private_data = port->private_data; >>> + >>> + clk_disable_unprepare(private_data->baud_clk); >>> } >>> static int meson_uart_request_port(struct uart_port *port) >>> { >>> + struct meson_uart_data *private_data = port->private_data; >>> + int ret; >>> + >>> + ret = clk_prepare_enable(private_data->baud_clk); >>> + if (ret) >>> + return ret; >>> + >> I think we already discussed that. This is yet another behavior change >> Previously, enabling the clock was done at probe time. >> It's fine to change it, if there is a justification, but not in the same >> change as the rework of the divider >> >>> return 0; >>> } >>> @@ -629,57 +640,106 @@ static struct uart_driver meson_uart_driver = { >>> .cons = MESON_SERIAL_CONSOLE, >>> }; >>> -static inline struct clk *meson_uart_probe_clock(struct device *dev, >>> - const char *id) >>> -{ >>> - struct clk *clk = NULL; >>> - int ret; >>> - >>> - clk = devm_clk_get(dev, id); >>> - if (IS_ERR(clk)) >>> - return clk; >>> - >>> - ret = clk_prepare_enable(clk); >>> - if (ret) { >>> - dev_err(dev, "couldn't enable clk\n"); >>> - return ERR_PTR(ret); >>> - } >>> +static const struct clk_div_table xtal_div_table[] = { >>> + { 0, 3 }, >>> + { 1, 1 }, >>> + { 2, 2 }, >>> + { 3, 2 }, >>> +}; >>> - devm_add_action_or_reset(dev, >>> - (void(*)(void *))clk_disable_unprepare, >>> - clk); >>> +static u32 use_xtal_mux_table; >>> - return clk; >>> -} >>> - >>> -static int meson_uart_probe_clocks(struct platform_device *pdev, >>> - struct uart_port *port) >>> +static int meson_uart_probe_clocks(struct uart_port *port) >>> { >>> - struct clk *clk_xtal = NULL; >>> - struct clk *clk_pclk = NULL; >>> - struct clk *clk_baud = NULL; >>> + struct meson_uart_data *private_data = port->private_data; >>> + struct clk *clk_baud, *clk_xtal; >>> + struct clk_hw *hw, *clk81_div4_hw; >>> + char clk_name[32]; >>> + struct clk_parent_data use_xtal_mux_parents; >>> - clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk"); >>> - if (IS_ERR(clk_pclk)) >>> - return PTR_ERR(clk_pclk); >>> + clk_baud = devm_clk_get(port->dev, "baud"); >>> + if (IS_ERR(clk_baud)) { >>> + dev_err(port->dev, "Failed to get the 'baud' clock\n"); >>> + return PTR_ERR(clk_baud); >>> + } >> Calling devm_clk_get() would not be necessary if you used "fw_name" in >> the parent table. Same bellow > Personally, I think it is good that you can understand your meaning, but as > you are an expert in CCF, it is nice to write code in that way, but for > people who are not unfamiliar with CCF, they may only care about the use of > CCF.It's not pretty to use but it's easy to understand. There is no magic in CCF. Stephen and the other contributor took time to add the fw_name mechanism espcially for this. I'm suggesting and you are expected to actually look at the code and considerer it. Lack of "expertize" is not a valid argument. >> >>> - clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal"); >>> + clk_xtal = devm_clk_get(port->dev, "xtal"); >>> if (IS_ERR(clk_xtal)) >>> - return PTR_ERR(clk_xtal); >>> - >>> - clk_baud = meson_uart_probe_clock(&pdev->dev, "baud"); >>> - if (IS_ERR(clk_baud)) >>> - return PTR_ERR(clk_baud); >>> + return dev_err_probe(port->dev, PTR_ERR(clk_xtal), >>> + "Failed to get the 'xtal' clock\n"); >>> + >>> + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), >>> + "clk81_div4"); >>> + clk81_div4_hw = devm_clk_hw_register_fixed_factor(port->dev, >>> + clk_name, >>> + __clk_get_name(clk_baud), >>> + CLK_SET_RATE_NO_REPARENT, >>> + 1, 4); >>> + if (IS_ERR(clk81_div4_hw)) >>> + return PTR_ERR(clk81_div4_hw); >> So, whatever the chip type - you register a fixed 4 divider .... > As you suggested last time, this CLK has been stored, but some chips are > not used. If you have better suggestions, please let me know and I can > make corrections later. No, never suggested that. I suspected that 4 divider design was the same on all SoC version. You reported it was not, So I don't get this >> >>> + >>> + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), >>> + "xtal_div"); >>> + hw = devm_clk_hw_register_divider_table(port->dev, >>> + clk_name, >>> + __clk_get_name(clk_baud), >>> + CLK_SET_RATE_NO_REPARENT, >>> + port->membase + AML_UART_REG5, >>> + 26, 2, >>> + CLK_DIVIDER_READ_ONLY, >>> + xtal_div_table, NULL); >>> + if (IS_ERR(hw)) >>> + return PTR_ERR(hw); >>> + >>> + if (private_data->use_xtal_clk) { >>> + use_xtal_mux_table = 1; >>> + use_xtal_mux_parents.hw = hw; >>> + } else { >>> + use_xtal_mux_parents.hw = clk81_div4_hw; >> ... which you may end up not using in the end >> This is bad. > If you have better suggestions, please let me know and I can make > corrections later. >> >>> + } >>> - port->uartclk = clk_get_rate(clk_baud); >>> + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), >>> + "use_xtal"); >>> + hw = __devm_clk_hw_register_mux(port->dev, NULL, >>> + clk_name, >>> + 1, >>> + NULL, NULL, >>> + &use_xtal_mux_parents, >>> + CLK_SET_RATE_PARENT, >>> + port->membase + AML_UART_REG5, >>> + 24, 0x1, >>> + CLK_MUX_READ_ONLY, >>> + &use_xtal_mux_table, NULL); >>> + >>> + if (IS_ERR(hw)) >>> + return PTR_ERR(hw); >>> + >>> + port->uartclk = clk_hw_get_rate(hw); >>> + >>> + snprintf(clk_name, sizeof(clk_name), "%s#%s", dev_name(port->dev), >>> + "baud_div"); >>> + hw = devm_clk_hw_register_divider(port->dev, >>> + clk_name, >>> + clk_hw_get_name(hw), >>> + CLK_SET_RATE_PARENT, >>> + port->membase + AML_UART_REG5, >>> + 0, 23, >>> + CLK_DIVIDER_ROUND_CLOSEST, >>> + NULL); >>> + if (IS_ERR(hw)) >>> + return PTR_ERR(hw); >>> + >>> + private_data->baud_clk = hw->clk; >>> return 0; >>> } >>> static int meson_uart_probe(struct platform_device *pdev) >>> { >>> + struct meson_uart_data *private_data; >>> struct resource *res_mem; >>> struct uart_port *port; >>> + struct clk *pclk; >>> u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */ >>> int ret = 0; >>> int irq; >>> @@ -705,6 +765,15 @@ static int meson_uart_probe(struct platform_device *pdev) >>> if (!res_mem) >>> return -ENODEV; >>> + pclk = devm_clk_get(&pdev->dev, "pclk"); >>> + if (IS_ERR(pclk)) >>> + return dev_err_probe(&pdev->dev, PTR_ERR(pclk), >>> + "Failed to get the 'pclk' clock\n"); >>> + >>> + ret = clk_prepare_enable(pclk); >>> + if (ret) >>> + return ret; >>> + >>> irq = platform_get_irq(pdev, 0); >>> if (irq < 0) >>> return irq; >>> @@ -724,9 +793,13 @@ static int meson_uart_probe(struct platform_device *pdev) >>> if (IS_ERR(port->membase)) >>> return PTR_ERR(port->membase); >>> - ret = meson_uart_probe_clocks(pdev, port); >>> - if (ret) >>> - return ret; >>> + private_data = devm_kzalloc(&pdev->dev, sizeof(*private_data), >>> + GFP_KERNEL); >>> + if (!private_data) >>> + return -ENOMEM; >>> + >>> + if (device_get_match_data(&pdev->dev)) >>> + private_data->use_xtal_clk = true; >> As long as the device matches a compatible below, the flag will end up >> 'true', regardless of values in the the dt_match table. >> I don't think this is the intended behavior. >> It highlights that proper testing of this series is important. >> Being at Amlogic, I'm sure you can test on more than just g12a and s4 >> > I believe with your experience this may be a real problem. I heard that > your company has our boards. If so, can you help verify it? >>> port->iotype = UPIO_MEM; >>> port->mapbase = res_mem->start; >>> @@ -740,6 +813,11 @@ static int meson_uart_probe(struct platform_device *pdev) >>> port->x_char = 0; >>> port->ops = &meson_uart_ops; >>> port->fifosize = fifosize; >>> + port->private_data = private_data; >>> + >>> + ret = meson_uart_probe_clocks(port); >>> + if (ret) >>> + return ret; >>> meson_ports[pdev->id] = port; >>> platform_set_drvdata(pdev, port); >>> @@ -766,10 +844,22 @@ static int meson_uart_remove(struct platform_device *pdev) >>> } >>> static const struct of_device_id meson_uart_dt_match[] = { >>> - { .compatible = "amlogic,meson6-uart" }, >>> - { .compatible = "amlogic,meson8-uart" }, >>> - { .compatible = "amlogic,meson8b-uart" }, >>> - { .compatible = "amlogic,meson-gx-uart" }, >>> + { >>> + .compatible = "amlogic,meson6-uart", >>> + .data = (void *)false, >>> + }, >>> + { >>> + .compatible = "amlogic,meson8-uart", >>> + .data = (void *)false, >>> + }, >>> + { >>> + .compatible = "amlogic,meson8b-uart", >>> + .data = (void *)false, >>> + }, >>> + { >>> + .compatible = "amlogic,meson-gx-uart", >>> + .data = (void *)true, >>> + }, >>> { /* sentinel */ }, >>> }; >>> MODULE_DEVICE_TABLE(of, meson_uart_dt_match); >> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel