Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <c23fe6e7-879a-fab6-71ba-fcebdaf7ae37@baylibre.com>
Date:   Tue, 1 Mar 2022 10:27:50 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH V7 0/6] Use CCF to describe the UART baud rate clock
Content-Language: en-US
To:     Yu Tu <yu.tu@amlogic.com>, Jerome Brunet <jbrunet@baylibre.com>,
        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 <gregkh@linuxfoundation.org>,
        Jiri Slaby <jirislaby@kernel.org>,
        Kevin Hilman <khilman@baylibre.com>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>
References: <20220225073922.3947-1-yu.tu@amlogic.com>
 <1j8rtvxnkv.fsf@starbuckisacylon.baylibre.com>
 <d0da38f1-72c1-d111-2d0d-2bfa2faf1a1d@amlogic.com>
 <1jilsyvyz9.fsf@starbuckisacylon.baylibre.com>
 <0054cf2f-f09d-5b01-d6ca-0dc877a757c3@amlogic.com>
From:   Neil Armstrong <narmstrong@baylibre.com>
Organization: Baylibre
In-Reply-To: <0054cf2f-f09d-5b01-d6ca-0dc877a757c3@amlogic.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

On 01/03/2022 10:01, Yu Tu wrote:
> Hi Jerome,
> 
> On 2022/3/1 16:36, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>>
>> On Tue 01 Mar 2022 at 13:54, Yu Tu <yu.tu@amlogic.com> wrote:
>>
>>> Hi Jerome,
>>>
>>> On 2022/2/28 18:59, Jerome Brunet wrote:
>>>> [ EXTERNAL EMAIL ]
>>>>
>>>> On Fri 25 Feb 2022 at 15:39, Yu Tu <yu.tu@amlogic.com> 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. Add Meson S4 SoC compatible.
>>>>>
>>>>> The test method:
>>>>> Start the console and run the following commands in turn:
>>>>> stty -F /dev/ttyAML0 115200 and stty -F /dev/ttyAML0 921600.
>>>>>
>>>>> Since most SoCs are too old, I was able to find all the platforms myself
>>>>> such as Meson6, Meson8, Meson8b, GXL and so on. I only tested it with
>>>>> G12A and S4.
>>>> GXL based board are still very common an easy to come by.
>>>> I'm quite surprised that you are unable to test on this SoC family
>>> The fact of the matter is that the S4 is our end-2020 chip, the G12A is
>>> five years old, and the GXL is seven years old. If you must ask for a
>>> test, I will report this problem to the leadership to coordinate resources.
>>
>> The age of the SoC is irrelevant. SoCs don't get deprecated based on age
>> in mainline. It is not just GXL, same goes for meson8.
>>
>> These SoCs are actively used. Boards with these SoCs are still sold and
>> easily available. See the VIM1 or the Libretech boards.
>>
>> Breaking things for the the users of these SoCs is not acceptable.
>> So yes, looking at your series, I strongly recommend you do more tests.
>>
> You have a point there. Let's go back to the root of the problem. I aim to increase S4. The S4 uses 12MHZ to calculate baud. That's all.
> Change it to CCF as you suggested. The changes are so large that you ask to test all the chips.
> I also mentioned last time that using CCF would lead to a longer drive probe time and affect the board startup time. If this problem is not solved, can we reject the way you suggest using CCF?

I have a much simpler proposal (non-tested):

============><===================================================
diff --git a/drivers/tty/serial/meson_uart.c b/drivers/tty/serial/meson_uart.c
index 45e00d928253..eda3fdad60d1 100644
--- a/drivers/tty/serial/meson_uart.c
+++ b/drivers/tty/serial/meson_uart.c
@@ -76,6 +76,12 @@
  #define AML_UART_POLL_USEC		5
  #define AML_UART_TIMEOUT_USEC		10000

+struct meson_uart_data {
+	struct clk	*clk_pclk;
+	struct clk	*clk_xtal;
+	struct clk	*clk_baud;
+};
+
  static struct uart_driver meson_uart_driver;

  static struct uart_port *meson_ports[AML_UART_PORT_NUM];
@@ -293,16 +299,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;
+	if (clk_is_match(private_data->clk_baud, private_data->clk_xtal)) {
+		val = ((clk_get_rate(private_data->clk_baud) / 3) / baud) - 1;
  		val |= AML_UART_BAUD_XTAL;
  	} else {
-		val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
+		val = ((clk_get_rate(private_data->clk_baud) * 10 / (baud * 4) + 5) / 10) - 1;
  	}
  	val |= AML_UART_BAUD_USE;
  	writel(val, port->membase + AML_UART_REG5);
@@ -666,31 +673,26 @@ static inline struct clk *meson_uart_probe_clock(struct device *dev,
  }

  static int meson_uart_probe_clocks(struct platform_device *pdev,
-				   struct uart_port *port)
+				   struct meson_uart_data *private_data)
  {
-	struct clk *clk_xtal = NULL;
-	struct clk *clk_pclk = NULL;
-	struct clk *clk_baud = NULL;
-
-	clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
-	if (IS_ERR(clk_pclk))
-		return PTR_ERR(clk_pclk);
+	private_data->clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
+	if (IS_ERR(private_data->clk_pclk))
+		return PTR_ERR(private_data->clk_pclk);

-	clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
-	if (IS_ERR(clk_xtal))
-		return PTR_ERR(clk_xtal);
+	private_data->clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
+	if (IS_ERR(private_data->clk_xtal))
+		return PTR_ERR(private_data->clk_xtal);

-	clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
-	if (IS_ERR(clk_baud))
-		return PTR_ERR(clk_baud);
-
-	port->uartclk = clk_get_rate(clk_baud);
+	private_data->clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
+	if (IS_ERR(private_data->clk_baud))
+		return PTR_ERR(private_data->clk_baud);

  	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;
  	u32 fifosize = 64; /* Default is 64, 128 for EE UART_0 */
@@ -714,6 +716,11 @@ static int meson_uart_probe(struct platform_device *pdev)
  	if (pdev->id < 0 || pdev->id >= AML_UART_PORT_NUM)
  		return -EINVAL;

+	private_data = devm_kzalloc(&pdev->dev, sizeof(*private_data),
+				    GFP_KERNEL);
+	if (!private_data)
+		return -ENOMEM;
+
  	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  	if (!res_mem)
  		return -ENODEV;
@@ -733,7 +740,7 @@ static int meson_uart_probe(struct platform_device *pdev)
  	if (!port)
  		return -ENOMEM;

-	ret = meson_uart_probe_clocks(pdev, port);
+	ret = meson_uart_probe_clocks(pdev, private_data);
  	if (ret)
  		return ret;

@@ -749,6 +756,7 @@ 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;

  	meson_ports[pdev->id] = port;
  	platform_set_drvdata(pdev, port);
============><===================================================

Neil

>>>>
>>>>>
>>>>> Yu Tu (6):
>>>>>     tty: serial: meson: Move request the register region to probe
>>>>>     tty: serial: meson: Use devm_ioremap_resource to get register mapped
>>>>>       memory
>>>>>     tty: serial: meson: Describes the calculation of the UART baud rate
>>>>>       clock using a clock frame
>>>>>     tty: serial: meson: Make some bit of the REG5 register writable
>>>>>     tty: serial: meson: The system stuck when you run the stty command on
>>>>>       the console to change the baud rate
>>>>>     tty: serial: meson: Added S4 SOC compatibility
>>>>>
>>>>> V6 -> V7: To solve the system stuck when you run the stty command on
>>>>> the console to change the baud rate.
>>>>> V5 -> V6: Change error format as discussed in the email.
>>>>> V4 -> V5: Change error format.
>>>>> V3 -> V4: Change CCF to describe the UART baud rate clock as discussed
>>>>> in the email.
>>>>> V2 -> V3: add compatible = "amlogic,meson-gx-uart". Because it must change
>>>>> the DTS before it can be deleted
>>>>> V1 -> V2: Use CCF to describe the UART baud rate clock.Make some changes as
>>>>> discussed in the email
>>>>>
>>>>> Link:https://lore.kernel.org/linux-amlogic/20220118030911.12815-4-yu.tu@amlogic.com/
>>>>>
>>>>>    drivers/tty/serial/meson_uart.c | 221 ++++++++++++++++++++++----------
>>>>>    1 file changed, 154 insertions(+), 67 deletions(-)
>>>>>
>>>>>
>>>>> base-commit: a603ca60cebff8589882427a67f870ed946b3fc8
>>>>
>>