Messages in this thread Patch in this message | | | Subject | Re: [PATCH v3 2/2] serial: 8250: Add new 8250-core based Broadcom STB driver | From | Jiri Slaby <> | Date | Fri, 19 Feb 2021 10:43:21 +0100 |
| |
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
| |