Re: [PATCH V3 4/6] tty: serial: meson: The UART baud rate calculation is described using the common clock code. Also added S4 chip uart Compatible.

[Yu Tu <yu.tu@amlogic.com>]

  Hi Jerome,

On 2022/1/4 22:35, Yu Tu wrote:
> Hi Jerome,
>      Thank you very much for your patient reply. I have learned a lot 
> from it.
> 
> On 2022/1/4 18:36, Jerome Brunet wrote:
>> [ EXTERNAL EMAIL ]
>>
>>
>> On Tue 04 Jan 2022 at 17:57, Yu Tu <yu.tu@amlogic.com> wrote:
>>
>>> Hi Jerome,
>>>     Thank you very much for your reply.
>>>
>>> On 2022/1/3 20:40, Jerome Brunet wrote:
>>>> [ EXTERNAL EMAIL ]
>>>>
>>>> On Thu 30 Dec 2021 at 18:21, 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
>>>>>
>>>>> Signed-off-by: Yu Tu <yu.tu@amlogic.com>
>>>>> ---
>>>>>    drivers/tty/serial/Kconfig      |   1 +
>>>>>    drivers/tty/serial/meson_uart.c | 311 
>>>>> ++++++++++++++++++++++++++------
>>>>>    2 files changed, 257 insertions(+), 55 deletions(-)
>>>>>
>>>>> diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
>>>>> index 780908d43557..32e238173036 100644
>>>>> --- a/drivers/tty/serial/Kconfig
>>>>> +++ b/drivers/tty/serial/Kconfig
>>>>> @@ -198,6 +198,7 @@ config SERIAL_KGDB_NMI
>>>>>    config SERIAL_MESON
>>>>>        tristate "Meson serial port support"
>>>>>        depends on ARCH_MESON
>>>>> +    depends on COMMON_CLK
>>>>>        select SERIAL_CORE
>>>>>        help
>>>>>          This enables the driver for the on-chip UARTs of the Amlogic
>>>>> diff --git a/drivers/tty/serial/meson_uart.c 
>>>>> b/drivers/tty/serial/meson_uart.c
>>>>> index 99efe62a1507..07eb1f40aaaa 100644
>>>>> --- a/drivers/tty/serial/meson_uart.c
>>>>> +++ b/drivers/tty/serial/meson_uart.c
>>>>> @@ -6,6 +6,7 @@
>>>>>     */
>>>>>      #include <linux/clk.h>
>>>>> +#include <linux/clk-provider.h>
>>>>>    #include <linux/console.h>
>>>>>    #include <linux/delay.h>
>>>>>    #include <linux/init.h>
>>>>> @@ -65,9 +66,7 @@
>>>>>    #define AML_UART_RECV_IRQ(c)        ((c) & 0xff)
>>>>>      /* AML_UART_REG5 bits */
>>>>> -#define AML_UART_BAUD_MASK        0x7fffff
>>>>>    #define AML_UART_BAUD_USE        BIT(23)
>>>>> -#define AML_UART_BAUD_XTAL        BIT(24)
>>>>>      #define AML_UART_PORT_NUM        12
>>>>>    #define AML_UART_PORT_OFFSET        6
>>>>> @@ -76,6 +75,21 @@
>>>>>    #define AML_UART_POLL_USEC        5
>>>>>    #define AML_UART_TIMEOUT_USEC        10000
>>>>>    +struct meson_uart_data {
>>>>> +    struct uart_port    port;
>>>>> +    struct clk        *pclk;
>>>>> +    struct clk        *baud_clk;
>>>>> +    struct clk_divider    baud_div;
>>>>> +    struct clk_mux        use_xtal_mux;
>>>>> +    struct clk_mux        xtal_clk_sel_mux;
>>>>> +    struct clk_mux        xtal2_clk_sel_mux;
>>>>> +    struct clk_fixed_factor    xtal_div2;
>>>>> +    struct clk_fixed_factor    xtal_div3;
>>>>> +    struct clk_fixed_factor    clk81_div4;
>>>> Keeping all these internal elements around is not useful since they are
>>>> registered using devm_
>>>>
>>> I'm sorry. I don't know what you mean. That's exactly what you said, but
>>> what's wrong, I don't understand. Do you have any better suggestions,
>>> please specify specific points, preferably give examples.
>>
>> I'm saying that you don't need to keep reference to the internal
>> elements of the clock tree you have registered since devm_ will take
>> care of the removal later on. IOW, Once they are registered, the pointer
>> is never used again so you don't need it in the private data.
>>
> I understand you now. What you say is right. I will correct it.
>>
>>>>> +    bool            no_clk81_input;
>>>> What is this ?
>>>>
>>> To distinguish between clK81 and XTAL.
>>
>> ... Yet, it is not used
>>
>>>>> +    bool            has_xtal_clk_sel;
>>>>> +};
>>>>> +
>>>>>    static struct uart_driver meson_uart_driver;
>>>>>      static struct uart_port *meson_ports[AML_UART_PORT_NUM];
>>>>> @@ -270,14 +284,11 @@ static void meson_uart_reset(struct uart_port 
>>>>> *port)
>>>>>    static int meson_uart_startup(struct uart_port *port)
>>>>>    {
>>>>>        u32 val;
>>>>> -    int ret = 0;
>>>>> +    int ret;
>>>>>    -    val = readl(port->membase + AML_UART_CONTROL);
>>>>> -    val |= AML_UART_CLEAR_ERR;
>>>>> -    writel(val, port->membase + AML_UART_CONTROL);
>>>>> -    val &= ~AML_UART_CLEAR_ERR;
>>>>> -    writel(val, port->membase + AML_UART_CONTROL);
>>>>> +    meson_uart_reset(port);
>>>>>    +    val = readl(port->membase + AML_UART_CONTROL);
>>>>>        val |= (AML_UART_RX_EN | AML_UART_TX_EN);
>>>>>        writel(val, port->membase + AML_UART_CONTROL);
>>>>>    @@ -295,19 +306,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) {
>>>> This check shows that previous code assumed bit 24 was left untouched
>>>> Below you can see that GXBB and newer used the XTAL path while older
>>>> used the other
>>>> Your change makes this dynamic which is another "unexpected" change.
>>>> Please make the bit 24 mux RO to start with so the behavior remains 
>>>> unchanged
>>>> for older SoCs.
>>> I agree with what you say, and I will correct it.
>>>>
>>>>> -        val = ((port->uartclk / 3) / baud) - 1;
>>>>> -        val |= AML_UART_BAUD_XTAL;
>>>>> -    } else {
>>>>> -        val = ((port->uartclk * 10 / (baud * 4) + 5) / 10) - 1;
>>>>> -    }
>>>>> +    val = readl(port->membase + AML_UART_REG5);
>>>>>        val |= AML_UART_BAUD_USE;
>>>>>        writel(val, port->membase + AML_UART_REG5);
>>>>> +
>>>>> +    clk_set_rate(private_data->baud_clk, baud);
>>>>>    }
>>>>>      static void meson_uart_set_termios(struct uart_port *port,
>>>>> @@ -397,11 +406,27 @@ static int meson_uart_verify_port(struct 
>>>>> uart_port *port,
>>>>>      static void meson_uart_release_port(struct uart_port *port)
>>>>>    {
>>>>> -    /* nothing to do */
>>>>> +    struct meson_uart_data *private_data = port->private_data;
>>>>> +
>>>>> +    clk_disable_unprepare(private_data->baud_clk);
>>>>> +    clk_disable_unprepare(private_data->pclk);
>>>>>    }
>>>>>      static int meson_uart_request_port(struct uart_port *port)
>>>>>    {
>>>>> +    struct meson_uart_data *private_data = port->private_data;
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = clk_prepare_enable(private_data->pclk);
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>> +
>>>>> +    ret = clk_prepare_enable(private_data->baud_clk);
>>>>> +    if (ret) {
>>>>> +        clk_disable_unprepare(private_data->pclk);
>>>>> +        return ret;
>>>>> +    }
>>>>> +
>>>>>        return 0;
>>>>>    }
>>>>>    @@ -629,56 +654,175 @@ static struct uart_driver 
>>>>> meson_uart_driver = {
>>>>>        .cons        = MESON_SERIAL_CONSOLE,
>>>>>    };
>>>>>    -static inline struct clk *meson_uart_probe_clock(struct device 
>>>>> *dev,
>>>>> -                         const char *id)
>>>>> +static int meson_uart_register_clk(struct uart_port *port,
>>>>> +                   const char *name_suffix,
>>>>> +                   const struct clk_parent_data *parent_data,
>>>>> +                   unsigned int num_parents,
>>>>> +                   const struct clk_ops *ops,
>>>>> +                   struct clk_hw *hw)
>>>>>    {
>>>>> -    struct clk *clk = NULL;
>>>>> +    struct clk_init_data init = { };
>>>>> +    char clk_name[32];
>>>>>        int ret;
>>>>>    -    clk = devm_clk_get(dev, id);
>>>>> -    if (IS_ERR(clk))
>>>>> -        return clk;
>>>>> +    snprintf(clk_name, sizeof(clk_name), "%s#%s", 
>>>>> dev_name(port->dev),
>>>>> +         name_suffix);
>>>>>    -    ret = clk_prepare_enable(clk);
>>>>> -    if (ret) {
>>>>> -        dev_err(dev, "couldn't enable clk\n");
>>>>> -        return ERR_PTR(ret);
>>>>> -    }
>>>>> +    init.name = clk_name;
>>>>> +    init.ops = ops;
>>>>> +    init.flags = CLK_SET_RATE_PARENT;
>>>>> +    init.parent_data = parent_data;
>>>>> +    init.num_parents = num_parents;
>>>>> +
>>>>> +    hw->init = &init;
>>>>>    -    devm_add_action_or_reset(dev,
>>>>> -            (void(*)(void *))clk_disable_unprepare,
>>>>> -            clk);
>>>>> +    ret = devm_clk_hw_register(port->dev, hw);
>>>>> +    if (ret)
>>>>> +        return dev_err_probe(port->dev, ret,
>>>>> +                     "Failed to register the '%s' clock\n",
>>>>> +                     clk_name);
>>>>>    -    return clk;
>>>>> +    return ret;
>>>>>    }
>>>>>    -static int meson_uart_probe_clocks(struct platform_device *pdev,
>>>>> -                   struct uart_port *port)
>>>>> -{
>>>>> -    struct clk *clk_xtal = NULL;
>>>>> -    struct clk *clk_pclk = NULL;
>>>>> -    struct clk *clk_baud = NULL;
>>>>> +static int meson_uart_probe_clocks(struct uart_port *port,
>>>>> +                   bool register_clk81_div4)
>>>>> +{
>>>>> +    struct meson_uart_data *private_data = port->private_data;
>>>>> +    struct clk_parent_data use_xtal_mux_parents[2] = {
>>>>> +        { .index = -1, },
>>>>> +        { .index = -1, },
>>>>> +    };
>>>>> +    struct clk_parent_data xtal_clk_sel_mux_parents[2] = { };
>>>>> +    struct clk_parent_data xtal2_clk_sel_mux_parents[2] = { };
>>>>> +    struct clk_parent_data xtal_div_parent = { .fw_name = "xtal", };
>>>>> +    struct clk_parent_data clk81_div_parent = { .fw_name = "baud", };
>>>>> +    struct clk_parent_data baud_div_parent = { };
>>>>> +    struct clk *clk_baud, *clk_xtal;
>>>>> +    int ret;
>>>>>    -    clk_pclk = meson_uart_probe_clock(&pdev->dev, "pclk");
>>>>> -    if (IS_ERR(clk_pclk))
>>>>> -        return PTR_ERR(clk_pclk);
>>>>> +    private_data->pclk = devm_clk_get(port->dev, "pclk");
>>>>> +    if (IS_ERR(private_data->pclk))
>>>>> +        return dev_err_probe(port->dev, PTR_ERR(private_data->pclk),
>>>>> +                     "Failed to get the 'pclk' clock\n");
>>>>> +
>>>>> +    clk_baud = devm_clk_get(port->dev, "baud");
>>>>> +    if (IS_ERR(clk_baud))
>>>>> +        return dev_err_probe(port->dev, PTR_ERR(clk_baud),
>>>>> +                     "Failed to get the 'baud' clock\n");
>>>>>    -    clk_xtal = meson_uart_probe_clock(&pdev->dev, "xtal");
>>>>> +    clk_xtal = devm_clk_get(port->dev, "xtal");
>>>>>        if (IS_ERR(clk_xtal))
>>>>> -        return PTR_ERR(clk_xtal);
>>>>> +        return dev_err_probe(port->dev, PTR_ERR(clk_xtal),
>>>>> +                     "Failed to get the 'xtal' clock\n");
>>>>> +
>>>>> +    private_data->xtal_div3.mult = 1;
>>>>> +    private_data->xtal_div3.div = 3;
>>>>> +    ret = meson_uart_register_clk(port, "xtal_div3", 
>>>>> &xtal_div_parent,
>>>>> +                      1, &clk_fixed_factor_ops,
>>>>> +                      &private_data->xtal_div3.hw);
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>>    -    clk_baud = meson_uart_probe_clock(&pdev->dev, "baud");
>>>>> -    if (IS_ERR(clk_baud))
>>>>> -        return PTR_ERR(clk_baud);
>>>>> +    if (register_clk81_div4) {
>>>> Clock dividers represent HW elements. The presence of an HW element
>>>> cannot
>>>> dependent on the current of the input clocks. There is no way this 
>>>> is right
>>>>
>>>>> +        private_data->clk81_div4.mult = 1;
>>>>> +        private_data->clk81_div4.div = 4;
>>>>> +        ret = meson_uart_register_clk(port, "clk81_div4",
>>>>> +                          &clk81_div_parent, 1,
>>>>> +                          &clk_fixed_factor_ops,
>>>>> +                          &private_data->clk81_div4.hw);
>>>>> +        if (ret)
>>>>> +            return ret;
>>>>> +
>>>>> +        use_xtal_mux_parents[0].hw = &private_data->clk81_div4.hw;
>>>> This is only going to be used on meson8 and older. Worst case it should
>>>> depend
>>>> on this compatible but I don't think this would be right here.
>>>> IMO, this shows that the UART "baud" input was actually fed by a
>>>> derivation of "fclk_div4" instead of "clk_81" on these older SoCs.
>>>> So instead of registering what I suspect to be a fake element, the
>>>> meson8 clock controller driver and DT should be fixed a this should 
>>>> go away.
>>>>
>>> Virtually all UART controllers are supported, but they are not used 
>>> after
>>> Meson8. So the description is more reasonable in UART driver.
>>
>> No it is not. It's not virtual, the support for meson8 is there in
>> mainline.
>>
>> Unless you tell me there is an actual /4 divider in the UART block of
>> the meson8 and older and that is was removed from the newer SoC,
>> the bit above should be removed and the meson8 clock tree fixed.
>>
>> I think it is more probable that the /4 was never there to begin with,
>> and that clk81/4 aka "fclk_div4", which is already available from the
>> clock controller, was actually routed on these older SoCs.
> I will check with the chip design department if it is divided by 4. I'll 
> get back to you.
I confirmed with the chip design department that there is a 4 divider 
inside. So I'm going to use the divider 4 for the description, but only 
the older meson8 is in use, so I'm going to pass the parameter through 
compatible. Do you have any better suggestions?

>>
>>>>> +    }
>>>>>    -    port->uartclk = clk_get_rate(clk_baud);
>>>>> +    if (private_data->has_xtal_clk_sel) {
>>>>> +        private_data->xtal_div2.mult = 1;
>>>>> +        private_data->xtal_div2.div = 2;
>>>>> +        ret = meson_uart_register_clk(port, "xtal_div2",
>>>>> +                          &xtal_div_parent, 1,
>>>>> +                          &clk_fixed_factor_ops,
>>>>> +                          &private_data->xtal_div2.hw);
>>>>> +        if (ret)
>>>>> +            return ret;
>>>>> +
>>>>> +        xtal_clk_sel_mux_parents[0].hw = &private_data->xtal_div3.hw;
>>>>> +        xtal_clk_sel_mux_parents[1].fw_name = "xtal";
>>>>> +
>>>>> +        private_data->xtal_clk_sel_mux.reg = port->membase + 
>>>>> AML_UART_REG5;
>>>>> +        private_data->xtal_clk_sel_mux.mask = 0x1;
>>>>> +        private_data->xtal_clk_sel_mux.shift = 26;
>>>>> +        private_data->xtal_clk_sel_mux.flags = CLK_MUX_ROUND_CLOSEST;
>>>>> +        ret = meson_uart_register_clk(port, "xtal_clk_sel",
>>>>> +                          xtal_clk_sel_mux_parents,
>>>>> +                          ARRAY_SIZE(xtal_clk_sel_mux_parents),
>>>>> +                          &clk_mux_ops,
>>>>> +                          &private_data->xtal_clk_sel_mux.hw);
>>>>> +        if (ret)
>>>>> +            return ret;
>>>>> +
>>>>> +        xtal2_clk_sel_mux_parents[0].hw = 
>>>>> &private_data->xtal_clk_sel_mux.hw;
>>>>> +        xtal2_clk_sel_mux_parents[1].hw = 
>>>>> &private_data->xtal_div2.hw;
>>>>> +
>>>>> +        private_data->xtal2_clk_sel_mux.reg = port->membase + 
>>>>> AML_UART_REG5;
>>>>> +        private_data->xtal2_clk_sel_mux.mask = 0x1;
>>>>> +        private_data->xtal2_clk_sel_mux.shift = 27;
>>>>> +        private_data->xtal2_clk_sel_mux.flags = 
>>>>> CLK_MUX_ROUND_CLOSEST;
>>>>> +        ret = meson_uart_register_clk(port, "xtal2_clk_sel",
>>>>> +                          xtal2_clk_sel_mux_parents,
>>>>> +                          ARRAY_SIZE(xtal2_clk_sel_mux_parents),
>>>>> +                          &clk_mux_ops,
>>>>> +                          &private_data->xtal2_clk_sel_mux.hw);
>>>>> +        if (ret)
>>>>> +            return ret;
>>>>> +
>>>>> +        use_xtal_mux_parents[1].hw = 
>>>>> &private_data->xtal2_clk_sel_mux.hw;
>>>>> +    } else {
>>>>> +        use_xtal_mux_parents[1].hw = &private_data->xtal_div3.hw;
>>>> Well the above is a bit over-complicated. If I summarize:
>>>> GXBB and older used a fixed divider of 3. Bits 26 and 27 read
>>>> 0 according to the documentation.
>>>> Chips after GXBB have 2 configurable divider of 2 and 3. bits 26 and 27
>>>> selects which of these dividers is used.
>>>> So the above could be replaced with a single divider covering bits 
>>>> 26 and
>>>> 27
>>>> with the following divider table { 2, 2, 1, 3 }. The divider should use
>>>> RO ops and not have CLK_SET_RATE_PARENT. It can be used for all chips
>>>> variant like this, including the older ones.
>>>>
>>> As you say, the way you say it is complicated. I think you're 
>>> complicating
>>> things.I don't understand what you're trying to achieve.
>>> Can you be more specific?
>>
>> I'm proposing to replace the 4 elements above, paths, parent_table and
>> all with a single divider with a specific table. It accomplishes the 
>> same thing.
>>
>> |--------+--------+-----------|
>> | Bit 27 | Bit 26 | Div Value |
>> |--------+--------+-----------|
>> |      0 |      0 |         3 |
>> |--------+--------+-----------|
>> |      0 |      1 |         1 |
>> |--------+--------+-----------|
>> |      1 |      0 |         2 |
>> |--------+--------+-----------|
>> |      1 |      1 |         2 |
>> |--------+--------+-----------|
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/clk-provider.h?h=v5.16-rc8#n602 
>>
>>
>> something like:
>>
>> static struct clk_div_table reg5_table[] = {
>>     { 0, 3 },
>>          { 1, 1 },
>>          { 2, 2 },
>>          { 3, 2 },
>> };
>>
>> I'm also pointing out that, since bit 26 and 27 reads 0 on the older
>> SoCs, the divider would work for these too, as long as the Ops is
>> read-only. So you don't even need to register different element
>> depending on the SoC which simplify things a bit more.
>>
>> If you prefer to keep a fixed divider for the older ones, It's fine by
>> me. It gives a bit more work but it is closer to reality. The newer SoC
>> could still use the custom divider regardless.
>>
>> Last, and as explained on other bits, that part of the clock path should
>> remain RO at first, to keep behavior stable with this change. You may
>> change that later on, in a dedicated patch describing the change.
>>
> I understand what you mean, and i will try to realize it. I hope you can 
> help review and give me your suggestions.
>>>
>>>> The only information you need to carry if whether or not you want to
>>>> make this divider modifiable. This means using the dt data to store
>>>> clk_div_ops or clk_div_ro_ops pointer.
>>>> To avoid changing the behavior of the older platforms in this patch, I
>>>> would
>>>> suggest to make everything use clk_div_ro_ops first, and make another
>>>> patch to use clk_div_ops if necessary.
>>>>
>>>>> +    }
>>>>> +
>>>>> +    private_data->use_xtal_mux.reg = port->membase + AML_UART_REG5;
>>>>> +    private_data->use_xtal_mux.mask = 0x1;
>>>>> +    private_data->use_xtal_mux.shift = 24;
>>>>> +    private_data->use_xtal_mux.flags = CLK_MUX_ROUND_CLOSEST;
>>>>> +    ret = meson_uart_register_clk(port, "use_xtal", 
>>>>> use_xtal_mux_parents,
>>>>> +                      ARRAY_SIZE(use_xtal_mux_parents),
>>>>> +                      &clk_mux_ops,
>>>> Use RO ops here to start with.
>>>> You can make this writable with another patch explicitly describing 
>>>> the change.
>>> I agree with what you say, and I will correct it.
>>>>
>>>>> +                      &private_data->use_xtal_mux.hw);
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>> +
>>>>> +    baud_div_parent.hw = &private_data->use_xtal_mux.hw;
>>>>> +
>>>>> +    private_data->baud_div.reg = port->membase + AML_UART_REG5;
>>>>> +    private_data->baud_div.shift = 0;
>>>>> +    private_data->baud_div.width = 23;
>>>>> +    private_data->baud_div.flags = CLK_DIVIDER_ROUND_CLOSEST;
>>>>> +    ret = meson_uart_register_clk(port, "baud_div",
>>>>> +                      &baud_div_parent, 1,
>>>>> +                      &clk_divider_ops,
>>>>> +                      &private_data->baud_div.hw);
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>> +
>>>>> +    private_data->baud_clk = devm_clk_hw_get_clk(port->dev,
>>>>> +                             &private_data->baud_div.hw,
>>>>> +                             "baud_rate");
>>>> There is a problem with this function in CCF. It will pin the driver to
>>>> itself, making it unremovable. It is an ongoing topic. For now, just 
>>>> use
>>>> "hw->clk" to get the struct *clk.
>>>>
>>>>> +    if (IS_ERR(private_data->baud_clk))
>>>>> +        return dev_err_probe(port->dev,
>>>> I don't think anything can defer here, so dev_err_probe() is not
>>>> necessary I think
>>>>
>>> I agree with what you say, and I will correct it.
>>>>> +                     PTR_ERR(private_data->baud_clk),
>>>>> +                     "Failed to request the 'baud_rate' clock\n");
>>>>>          return 0;
>>>>>    }
>>>>>      static int meson_uart_probe(struct platform_device *pdev)
>>>>>    {
>>>>> +    struct meson_uart_data *private_data;
>>>>>        struct resource *res_mem, *res_irq;
>>>>> +    struct clk *clk_baud, *clk_xtal;
>>>>> +    bool register_clk81_div4;
>>>>>        struct uart_port *port;
>>>>>        int ret = 0;
>>>>>        int id = -1;
>>>>> @@ -711,18 +855,37 @@ static int meson_uart_probe(struct 
>>>>> platform_device *pdev)
>>>>>            return -EBUSY;
>>>>>        }
>>>>>    -    port = devm_kzalloc(&pdev->dev, sizeof(struct uart_port),
>>>>> GFP_KERNEL);
>>>>> -    if (!port)
>>>>> +    private_data = devm_kzalloc(&pdev->dev, sizeof(*private_data),
>>>>> +                    GFP_KERNEL);
>>>>> +    if (!private_data)
>>>>>            return -ENOMEM;
>>>>>    +    if (device_get_match_data(&pdev->dev))
>>>>> +        private_data->has_xtal_clk_sel = true;
>>>>> +
>>>>> +    private_data->pclk = devm_clk_get(&pdev->dev, "pclk");
>>>>> +    if (IS_ERR(private_data->pclk))
>>>>> +        return dev_err_probe(&pdev->dev, PTR_ERR(private_data->pclk),
>>>>> +                     "Failed to get the 'pclk' clock\n");
>>>>> +
>>>>> +    clk_baud = devm_clk_get(&pdev->dev, "baud");
>>>>> +    if (IS_ERR(clk_baud))
>>>>> +        return dev_err_probe(&pdev->dev, PTR_ERR(clk_baud),
>>>>> +                     "Failed to get the 'baud' clock\n");
>>>>> +
>>>>> +    clk_xtal = devm_clk_get(&pdev->dev, "xtal");
>>>>> +    if (IS_ERR(clk_xtal))
>>>>> +        return dev_err_probe(&pdev->dev, PTR_ERR(clk_xtal),
>>>>> +                     "Failed to get the 'xtal' clock\n");
>>>>> +
>>>> This is second time you call devm_clk_get() on these clocks. One
>>>> instance has to go away
>>>>
>>> I agree with what you say, and I will correct it.
>>>>> +    register_clk81_div4 = clk_get_rate(clk_xtal) != 
>>>>> clk_get_rate(clk_baud);
>>>>> +
>>>> The above is a ugly way to distinguish the meson8 (32bit) SoC family
>>>> from the rest. This definitely not the way to achieve it.
>>>> The right is the compatible data but here I think it is not
>>>> necessary. The clock input should be fixed.
>>>>
>>>>> +    port = &private_data->port;
>>>>> +
>>>>>        port->membase = devm_ioremap_resource(&pdev->dev, res_mem);
>>>>>        if (IS_ERR(port->membase))
>>>>>            return PTR_ERR(port->membase);
>>>>>    -    ret = meson_uart_probe_clocks(pdev, port);
>>>>> -    if (ret)
>>>>> -        return ret;
>>>>> -
>>>>>        port->iotype = UPIO_MEM;
>>>>>        port->mapbase = res_mem->start;
>>>>>        port->mapsize = resource_size(res_mem);
>>>>> @@ -735,6 +898,12 @@ static int meson_uart_probe(struct 
>>>>> platform_device *pdev)
>>>>>        port->x_char = 0;
>>>>>        port->ops = &meson_uart_ops;
>>>>>        port->fifosize = 64;
>>>>> +    port->uartclk = clk_get_rate(clk_baud);
>>>>> +    port->private_data = private_data;
>>>>> +
>>>>> +    ret = meson_uart_probe_clocks(port, register_clk81_div4);
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>>          meson_ports[pdev->id] = port;
>>>>>        platform_set_drvdata(pdev, port);
>>>>> @@ -761,10 +930,42 @@ static int meson_uart_remove(struct 
>>>>> platform_device *pdev)
>>>>>    }
>>>>>      static const struct of_device_id meson_uart_dt_match[] = {
>>>>> -    { .compatible = "amlogic,meson6-uart" },
>>>>> -    { .compatible = "amlogic,meson8-uart" },
>>>>> -    { .compatible = "amlogic,meson8b-uart" },
>>>>> -    { .compatible = "amlogic,meson-gx-uart" },
>>>>> +    {
>>>>> +        .compatible = "amlogic,meson6-uart",
>>>>> +        .data = (void *)false,
>>>>> +    },
>>>>> +    {
>>>>> +        .compatible = "amlogic,meson8-uart",
>>>>> +        .data = (void *)false,
>>>>> +    },
>>>>> +    {
>>>>> +        .compatible = "amlogic,meson8b-uart",
>>>>> +        .data = (void *)false,
>>>>> +    },
>>>>> +    {
>>>>> +        .compatible = "amlogic,meson-gxbb-uart",
>>>>> +        .data = (void *)false,
>>>>> +    },
>>>>> +    {
>>>>> +        .compatible = "amlogic,meson-gxl-uart",
>>>>> +        .data = (void *)true,
>>>>> +    },
>>>>> +    {
>>>>> +        .compatible = "amlogic,meson-g12a-uart",
>>>>> +        .data = (void *)true,
>>>>> +    },
>>>>> +    {
>>>>> +        .compatible = "amlogic,meson-s4-uart",
>>>>> +        .data = (void *)true,
>>>>> +    },
>>>>> +    /*
>>>>> +     * deprecated, don't use anymore because it doesn't differentiate
>>>>> +     * between GXBB and GXL which have different revisions of the 
>>>>> UART IP.
>>>>> +     */
>>>>> +    {
>>>>> +        .compatible = "amlogic,meson-gx-uart",
>>>>> +        .data = (void *)false,
>>>>> +    },
>>>>>        { /* sentinel */ },
>>>>>    };
>>>>>    MODULE_DEVICE_TABLE(of, meson_uart_dt_match);
>>>>
>>