| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 19 Feb 2021 15:01:43 -0500
From: Al Cooper <al.cooper@...adcom.com>
To: Jiri Slaby <jirislaby@...nel.org>
Cc: Al Cooper <alcooperx@...il.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
BCM Kernel Feedback <bcm-kernel-feedback-list@...adcom.com>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@...r.kernel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
"open list:USB SUBSYSTEM" <linux-usb@...r.kernel.org>,
Masahiro Yamada <yamada.masahiro@...ionext.com>,
Rob Herring <robh+dt@...nel.org>
Subject: Re: [PATCH v3 2/2] serial: 8250: Add new 8250-core based Broadcom STB driver
On Fri, Feb 19, 2021 at 4:43 AM Jiri Slaby <jirislaby@...nel.org> wrote:
>
> 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?
This was done to make the line fit in 80 columns. Now that the rule
has been relaxed, I'll move it.
>
>
> > +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.
Fixed.
>
>
> > +}
> > +
> > +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?
Fixed.
>
>
> > + 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?
brcmuart_arbitration() can return a failure and I'll print an error
message and return EBUSY if it does.
The other functions always return 0 and I'll change them to return void.
>
> > + 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?
Fixed
>
>
> > config SERIAL_8250_TEGRA
> > tristate "8250 support for Tegra serial ports"
> > default SERIAL_8250
>
> regards,
> --
> js
> suse labs
Jira, thanks for the review.
Al
Download attachment "smime.p7s" of type "application/pkcs7-signature" (4157 bytes)
Powered by blists - more mailing lists