Message-ID: <c9493f4d-8056-20a0-6ed2-161fbcc3d34e@gmail.com>
Date:   Tue, 30 May 2023 09:09:16 +0800
From:   Jacky Huang <ychuang570808@...il.com>
To:     Jiri Slaby <jirislaby@...nel.org>, robh+dt@...nel.org,
        krzysztof.kozlowski+dt@...aro.org, lee@...nel.org,
        mturquette@...libre.com, sboyd@...nel.org, p.zabel@...gutronix.de,
        gregkh@...uxfoundation.org, tmaimon77@...il.com,
        catalin.marinas@....com, will@...nel.org
Cc:     devicetree@...r.kernel.org, linux-clk@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-serial@...r.kernel.org, arnd@...db.de, soc@...nel.org,
        schung@...oton.com, mjchen@...oton.com,
        Jacky Huang <ychuang3@...oton.com>
Subject: Re: [PATCH v11 10/10] tty: serial: Add Nuvoton ma35d1 serial driver
 support

Dear Jiri,


On 2023/5/24 下午 04:34, Jacky Huang wrote:
> Dear Jiri,
>
> Thanks for your advice.
>
> On 2023/5/24 下午 03:42, Jiri Slaby wrote:
>> On 16. 05. 23, 9:52, Jacky Huang wrote:
>>> +static void ma35d1serial_config_port(struct uart_port *port, int 
>>> flags)
>>> +{
>>> +    /*
>>> +     * Driver core for serial ports forces a non-zero value for 
>>> port type.
>>> +     * Write an arbitrary value here to accommodate the serial core 
>>> driver,
>>> +     * as ID part of UAPI is redundant.
>>> +     */
>>> +    port->type = 1;
>>
>> So this 1 translates to PORT_8250. Why not to use it directly? Or 
>> something more saner like PORT_16550A?
>>
> It's not actually 8250 or 16550A.
> Can we add the following definition to 
> 'include/uapi/linux/serial_core.h' and use PORT_MA35 instead?
>
> #define PORT_MA35    124
>
>
>>> +}
>>> +
>>> +static int ma35d1serial_verify_port(struct uart_port *port, struct 
>>> serial_struct *ser)
>>> +{
>>> +    if (port->type != PORT_UNKNOWN && ser->type != 1)
>>> +        return -EINVAL;
>>> +
>>> +    return 0;
>>> +}
>> ...
>>> +static int __init ma35d1serial_console_setup(struct console *co, 
>>> char *options)
>>> +{
>>> +    struct device_node *np = ma35d1serial_uart_nodes[co->index];
>>> +    struct uart_ma35d1_port *p = &ma35d1serial_ports[co->index];
>>> +    u32 val32[4];
>>> +    struct uart_port *port;
>>> +    int baud = 115200;
>>> +    int bits = 8;
>>> +    int parity = 'n';
>>> +    int flow = 'n';
>>> +
>>> +    /*
>>> +     * Check whether an invalid uart number has been specified, and
>>
>> You dereferenced ma35d1serial_uart_nodes already. Doesn't 
>> console=ttyNVT1000 (or something like that) crash the system?
>>
>
> I will add the following check before np = 
> "ma35d1serial_uart_nodes[co->index]".
>
> if (co->index < 0 || co->index >= MA35_UART_NR)
>     return -EINVAL;
>
>
>>> +     * if so, search for the first available port that does have
>>> +     * console support.
>>
>> The code below doesn't match this comment.
>
> Yes, I will remove the above comment.
>
>>
>>> +     */
>>> +    if ((co->index < 0) || (co->index >= MA35_UART_NR)) {
>>> +        pr_debug("Console Port%x out of range\n", co->index);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    if (of_property_read_u32_array(np, "reg", val32, 4) != 0)
>>
>> Shouldn't that 4 be ARRAY_SIZE(val32) instead?
>>
>
> Will be fixed.
>
>>> +        return -EINVAL;
>>
>> One \n here please.
>>
>
> Okay, I will add it.
>
>>> +    p->port.iobase = val32[1];
>>> +    p->port.membase = ioremap(p->port.iobase, MA35_UART_REG_SIZE);
>>
>> What if this fails?
>>
>
> I will add a check for the return value.
>
>>> +    p->port.ops = &ma35d1serial_ops;
>>> +    p->port.line = 0;
>>> +    p->port.uartclk = MA35_UART_CONSOLE_CLK;
>>> +
>>> +    port = &ma35d1serial_ports[co->index].port;
>>
>> Isn't this:
>>   port = &p->port;
>> ?
>>
>> Either use port on all above lines or drop the "port" variable 
>> completely and use "p->port" below instead.
>>
>
> I will remove port variable and use p->port only.
>
>>> +
>>> +    if (options)
>>> +        uart_parse_options(options, &baud, &parity, &bits, &flow);
>>> +
>>> +    return uart_set_options(port, co, baud, parity, bits, flow);
>>> +}
>>> +
>>> +static struct console ma35d1serial_console = {
>>> +    .name    = "ttyNVT",
>>> +    .write   = ma35d1serial_console_write,
>>> +    .device  = uart_console_device,
>>> +    .setup   = ma35d1serial_console_setup,
>>> +    .flags   = CON_PRINTBUFFER | CON_ENABLED,
>>> +    .index   = -1,
>>> +    .data    = &ma35d1serial_reg,
>>
>> I don't see console->data used anywhere in the driver?
>>
>
> I will remove it.
>

I removed the " .data    = &ma35d1serial_reg", but kernel crashed in 
'drivers/tty/serial/serial_core.c'.

The variable 'p' of  uart_console_device() refer to a NULL, because 
co->data is NULL.
As a result, kernel crashed at 'p->tty_driver'.

struct tty_driver *uart_console_device(struct console *co, int *index)
{
     struct uart_driver *p = co->data;
     *index = co->index;
     return p->tty_driver;
}

We seem to be unable to remove it.
If there are no other considerations, I will keep it in the next version.

>>> +};
>> ...
>>> +static int ma35d1serial_probe(struct platform_device *pdev)
>>> +{
>>> +    struct resource *res_mem;
>>> +    struct uart_ma35d1_port *up;
>>> +    int ret = 0;
>>> +
>>> +    if (pdev->dev.of_node) {
>>> +        ret = of_alias_get_id(pdev->dev.of_node, "serial");
>>> +        if (ret < 0) {
>>> +            dev_err(&pdev->dev, "failed to get alias/pdev id, errno 
>>> %d\n", ret);
>>> +            return ret;
>>> +        }
>>> +    }
>>> +    up = &ma35d1serial_ports[ret];
>>> +    up->port.line = ret;
>>> +    res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +    if (!res_mem)
>>> +        return -ENODEV;
>>> +
>>> +    up->port.iobase = res_mem->start;
>>> +    up->port.membase = ioremap(up->port.iobase, MA35_UART_REG_SIZE);
>>
>> Check this too.
>>
>
> Okay, sure.
>
>>> +    up->port.ops = &ma35d1serial_ops;
>>> +
>>> +    spin_lock_init(&up->port.lock);
>>> +
>>> +    up->clk = of_clk_get(pdev->dev.of_node, 0);
>>> +    if (IS_ERR(up->clk)) {
>>> +        ret = PTR_ERR(up->clk);
>>> +        dev_err(&pdev->dev, "failed to get core clk: %d\n", ret);
>>> +        goto err_iounmap;
>>> +    }
>>> +
>>> +    ret = clk_prepare_enable(up->clk);
>>> +    if (ret)
>>> +        goto err_iounmap;
>>> +
>>> +    if (up->port.line != 0)
>>> +        up->port.uartclk = clk_get_rate(up->clk);
>>> +
>>> +    ret = platform_get_irq(pdev, 0);
>>> +    if (ret < 0)
>>> +        goto err_clk_disable;
>>> +
>>> +    up->port.irq = ret;
>>> +    up->port.dev = &pdev->dev;
>>> +    up->port.flags = UPF_BOOT_AUTOCONF;
>>> +
>>> +    platform_set_drvdata(pdev, up);
>>> +
>>> +    ret = uart_add_one_port(&ma35d1serial_reg, &up->port);
>>> +    if (ret < 0)
>>> +        goto err_free_irq;
>>> +
>>> +    return 0;
>>> +
>>> +err_free_irq:
>>> +    free_irq(up->port.irq, &up->port);
>>> +
>>> +err_clk_disable:
>>> +    clk_disable_unprepare(up->clk);
>>> +
>>> +err_iounmap:
>>> +    iounmap(up->port.membase);
>>> +    return ret;
>>> +}
>>
>> thanks,
>
> Best regards,
> Jacky Huang
>

Best regards,
Jacky Huang

Powered by blists - more mailing lists