Messages in this thread | | | Date | Wed, 5 Jan 2022 13:53:56 +0800 | Subject | 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. | From | Yu Tu <> |
| |
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); >>>> >>
| |