Re: [PATCH 2/3] tty: serial: meson: Added 12Mhz as the clock source for calculating baud rate

[Yu Tu]

Hi Neil,
	Thank you for your reply.

On 2022/4/7 22:03, Neil Armstrong wrote:
> [ EXTERNAL EMAIL ]
>
> On 07/04/2022 10:13, Yu Tu wrote:
> > Starting with the g12A chip, add a 12Mhz clock to calculate the baud
> > rate, since the BT module uses 3Mhz baud rate. 8Mhz calculations can
> > lead to baud rate bias, causing some problems.
>
> The commit message isn't clear enough, 12MHz is a new intermediate clock
> rate, not a new clock source of the UART module.
>
> Please explain a /2 divider over XTAL was introduced since G12A, and
> is preferred to be used over the still present /3 divider since it
> provides much closer frequencies vs the request baudrate.
I will prepare the next version of the code as you requested.
> > Signed-off-by: Yu Tu <yu.tu@xxxxxxxxxxx>
> > ---
> >   drivers/tty/serial/meson_uart.c | 25 +++++++++++++++++++++++--
> >   1 file changed, 23 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/tty/serial/meson_uart.c 
> > b/drivers/tty/serial/meson_uart.c
> > index 8e59624935af..7e77693a1318 100644
> > --- a/drivers/tty/serial/meson_uart.c
> > +++ b/drivers/tty/serial/meson_uart.c
> > @@ -68,6 +68,7 @@
> >   #define AML_UART_BAUD_MASK        0x7fffff
> >   #define AML_UART_BAUD_USE        BIT(23)
> >   #define AML_UART_BAUD_XTAL        BIT(24)
> > +#define AML_UART_BAUD_XTAL_DIV2        BIT(27)
> >   #define AML_UART_PORT_NUM        12
> >   #define AML_UART_PORT_OFFSET        6
> > @@ -80,6 +81,10 @@ static struct uart_driver meson_uart_driver;
> >   static struct uart_port *meson_ports[AML_UART_PORT_NUM];
> > +struct meson_uart_data {
> > +    bool has_xtal_div2;
> > +};
> > +
> >   static void meson_uart_set_mctrl(struct uart_port *port, unsigned 
> > int mctrl)
> >   {
> >   }
> > @@ -293,13 +298,20 @@ static int meson_uart_startup(struct uart_port 
> > *port)
> >   static void meson_uart_change_speed(struct uart_port *port, unsigned 
> > long baud)
> >   {
> > -    u32 val;
> > +    struct meson_uart_data *private_data = port->private_data;
> > +    u32 val = 0;
> >       while (!meson_uart_tx_empty(port))
> >           cpu_relax();
> >       if (port->uartclk == 24000000) {
> > -        val = DIV_ROUND_CLOSEST(port->uartclk / 3, baud) - 1;
> > +        unsigned int xtal_div = 3;
> > +
> > +        if (private_data->has_xtal_div2) {
> > +            xtal_div = 2;
> > +            val |= AML_UART_BAUD_XTAL_DIV2;
> > +        }
> > +        val |= DIV_ROUND_CLOSEST(port->uartclk / xtal_div, baud) - 1;
> >           val |= AML_UART_BAUD_XTAL;
> >       } else {
> >           val =  DIV_ROUND_CLOSEST(port->uartclk / 4, baud) - 1;
> > @@ -691,6 +703,7 @@ static int meson_uart_probe_clocks(struct 
> > platform_device *pdev,
> >   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 */
> > @@ -733,6 +746,13 @@ static int meson_uart_probe(struct 
> > platform_device *pdev)
> >       if (!port)
> >           return -ENOMEM;
> > +    private_data = devm_kzalloc(&pdev->dev, sizeof(struct 
> > meson_uart_data), GFP_KERNEL);
> > +    if (!private_data)
> > +        return -ENOMEM;
> > +
> > +    if ((bool)device_get_match_data(&pdev->dev))
> > +        private_data->has_xtal_div2 = true;
>
> It should be much cleaner to pass meson_uart_data to meson_uart_dt_match 
> .data,
> and then you can retrieve device_get_match_data() and put the result into
> port->private_data.
> This will avoid a devm_kzalloc().
>
> Then in meson_uart_change_speed() change the test to check for NULL 
> private_data:
> if (private_data && private_data->has_xtal_div2)
>
> > +
> >       ret = meson_uart_probe_clocks(pdev, port);
> >       if (ret)
> >           return ret;
> > @@ -749,6 +769,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
I will prepare the next version of the code as you requested.

> .