Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v3 2/2] serial: 8250: Add new 8250-core based Broadcom STB
 driver
To:     Al Cooper <alcooperx@gmail.com>, linux-kernel@vger.kernel.org
Cc:     bcm-kernel-feedback-list@broadcom.com, devicetree@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-serial@vger.kernel.org, linux-usb@vger.kernel.org,
        Masahiro Yamada <yamada.masahiro@socionext.com>,
        Rob Herring <robh+dt@kernel.org>
References: <20210212195736.45328-1-alcooperx@gmail.com>
 <20210212195736.45328-3-alcooperx@gmail.com>
From:   Jiri Slaby <jirislaby@kernel.org>
Message-ID: <d891e5bf-4fd8-fc44-7256-35ffa9d0931a@kernel.org>
Date:   Fri, 19 Feb 2021 10:43:21 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.0
MIME-Version: 1.0
In-Reply-To: <20210212195736.45328-3-alcooperx@gmail.com>
Content-Type: text/plain; charset=iso-8859-2; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 12. 02. 21, 20:57, Al Cooper wrote:
> Add a UART driver for the new Broadcom 8250 based STB UART. The new
> UART is backward compatible with the standard 8250, but has some
> additional features. The new features include a high accuracy baud
> rate clock system and DMA support.
> 
> The driver will use the new optional BAUD MUX clock to select the best
> one of the four master clocks (81MHz, 108MHz, 64MHz and 48MHz) to feed
> the baud rate selection logic for any requested baud rate.  This allows
> for more accurate BAUD rates when high speed baud rates are selected.
...
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_bcm7271.c
...
> +static void brcmuart_rx_isr(struct uart_port *up, u32 rx_isr)
> +{
> +	struct brcmuart_priv *priv = up->private_data;
> +	struct device *dev = up->dev;
> +	u32 rx_done_isr;
> +	u32 check_isr;
> +	char seq_err[] = "RX buffer ready out of sequence, restarting RX DMA\n";

What's the purpose of this on-stack variable?

> +static void init_real_clk_rates(struct device *dev, struct brcmuart_priv *priv)
> +{
> +	int x;
> +	int rc;
> +
> +	priv->default_mux_rate = clk_get_rate(priv->baud_mux_clk);
> +	dev_dbg(dev, "Default BAUD MUX Clock rate is %lu\n",
> +		priv->default_mux_rate);
> +
> +	for (x = 0; x < ARRAY_SIZE(priv->real_rates); x++) {
> +		if (priv->rate_table[x] == 0) {
> +			priv->real_rates[x] = 0;
> +			continue;
> +		}
> +		rc = clk_set_rate(priv->baud_mux_clk, priv->rate_table[x]);
> +		if (rc) {
> +			dev_err(dev, "Error selecting BAUD MUX clock for %u\n",
> +				priv->rate_table[x]);
> +			priv->real_rates[x] = priv->rate_table[x];
> +		} else {
> +			priv->real_rates[x] = clk_get_rate(priv->baud_mux_clk);
> +		}
> +	}
> +	 clk_set_rate(priv->baud_mux_clk, priv->default_mux_rate);

This is only weirdly indented.

> +}
> +
> +static void set_clock_mux(struct uart_port *up, struct brcmuart_priv *priv,
> +			u32 baud)
> +{
> +	u32 percent;
> +	u32 best_percent = UINT_MAX;
> +	u32 quot;
> +	u32 best_quot = 1;
> +	u32 rate;
> +	int best_index = -1;
> +	u64 hires_rate;
> +	u64 hires_baud;
> +	u64 hires_err;
> +	int rc;
> +	int i;
> +	int real_baud;
> +
> +	/* If the Baud Mux Clock was not specified, just return */
> +	if (priv->baud_mux_clk == NULL)
> +		return;
> +
> +	/* Find the closest match for specified baud */
> +	for (i = 0; i < ARRAY_SIZE(priv->real_rates); i++) {
> +		if (priv->real_rates[i] == 0)
> +			continue;
> +		rate = priv->real_rates[i] / 16;
> +		quot = DIV_ROUND_CLOSEST(rate, baud);
> +		if (!quot)
> +			continue;
> +
> +		/* increase resolution to get xx.xx percent */
> +		hires_rate = (u64)rate * 10000;
> +		hires_baud = (u64)baud * 10000;
> +
> +		hires_err = div_u64(hires_rate, (u64)quot);
> +
> +		/* get the delta */
> +		if (hires_err > hires_baud)
> +			hires_err = (hires_err - hires_baud);
> +		else
> +			hires_err = (hires_baud - hires_err);
> +
> +		percent = (unsigned long)DIV_ROUND_CLOSEST_ULL(hires_err, baud);
> +		dev_dbg(up->dev,
> +			"Baud rate: %u, MUX Clk: %u, Error: %u.%u%%\n",
> +			baud, priv->real_rates[i], percent / 100,
> +			percent % 100);
> +		if (percent < best_percent) {
> +			best_percent = percent;
> +			best_index = i;
> +			best_quot = quot;
> +		}
> +	}
> +	if (best_index == -1) {
> +		dev_err(up->dev, "Error, %d BAUD rate is too fast.\n", baud);
> +		return;
> +	}
> +	rate = priv->real_rates[best_index];
> +	rc = clk_set_rate(priv->baud_mux_clk, rate);
> +	if (rc)
> +		dev_err(up->dev, "Error selecting BAUD MUX clock\n");
> +
> +	/* Error over 3 percent will cause data errors */
> +	if (best_percent > 300)
> +		dev_err(up->dev, "Error, baud: %d has %u.%u%% error\n",
> +			baud, percent / 100, percent % 100);
> +
> +	real_baud = rate / 16 / best_quot;
> +	dev_dbg(up->dev, "Selecting BAUD MUX rate: %u\n", rate);
> +	dev_dbg(up->dev, "Requested baud: %u, Actual baud: %u\n",
> +		baud, real_baud);
> +
> +	/* calc nanoseconds for 1.5 characters time at the given baud rate */
> +	i = 1000000000 / real_baud / 10;

NSEC_PER_SEC here?

> +	i += (i / 2);
> +	priv->char_wait = ns_to_ktime(i);
> +
> +	up->uartclk = rate;
> +}

...

> +static int __maybe_unused brcmuart_resume(struct device *dev)
> +{
> +	struct brcmuart_priv *priv = dev_get_drvdata(dev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(priv->baud_mux_clk);
> +	if (ret)
> +		dev_err(dev, "Error enabling BAUD MUX clock\n");
> +
> +	/*
> +	 * The hardware goes back to it's default after suspend
> +	 * so get the "clk" back in sync.
> +	 */
> +	ret = clk_set_rate(priv->baud_mux_clk, priv->default_mux_rate);
> +	if (ret)
> +		dev_err(dev, "Error restoring default BAUD MUX clock\n");
> +	if (priv->dma_enabled) {
> +		brcmuart_arbitration(priv, 1);
> +		brcmuart_init_dma_hardware(priv);
> +		start_rx_dma(serial8250_get_port(priv->line));
> +	}
> +	serial8250_resume_port(priv->line);

All these cannot fail? Or the above can, so does proceeding further 
without an error make sense?

> +	return 0;
> +}
....
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -501,6 +501,7 @@ config SERIAL_8250_PXA
>   	  applicable to both devicetree and legacy boards, and early console is
>   	  part of its support.
>   
> +

Why adding a newline here?

>   config SERIAL_8250_TEGRA
>   	tristate "8250 support for Tegra serial ports"
>   	default SERIAL_8250

regards,
-- 
js
suse labs