Re: [PATCH v4 16/28] tty: serial: owl: Implement console driver

[Andreas FÃrber]

Am 07.06.2017 um 16:37 schrieb Andy Shevchenko:
> On Tue, Jun 6, 2017 at 3:54 AM, Andreas FÃrber <afaerber@xxxxxxx> wrote:
>> Implement serial console driver to complement earlycon.
>>
>> Based on LeMaker linux-actions tree.
> 
>> +#define OWL_UART_CTL_DWLS_MASK         (0x3 << 0)
> 
> GENMASK() ?
> 
>> +#define OWL_UART_CTL_PRS_MASK          (0x7 << 4)
> 
> Ditto.
> 
>>  #define OWL_UART_STAT_TRFL_MASK                (0x1f << 11)
> 
> Ditto.

Changed.

>> +static struct owl_uart_port *owl_uart_ports[OWL_UART_PORT_NUM];
> 
> And this is needed for...?

That's what both the downstream driver and several in-tree drivers are
doing. See `git grep '_ports\[' -- drivers/tty/serial/`.

If you feel this is wrong, --verbose explanation please.

>> +static void owl_uart_set_mctrl(struct uart_port *port, unsigned int mctrl)
>> +{
>> +}
> 
> Do we need an empty stub?

The only flag I have found in the CTL register is AFE for automatic
RTS/CTS flow-control - RTS and CTS can only be read in STAT, not set.

And yes, if I drop this empty callback function, I get no serial output.

>> +static void owl_uart_send_chars(struct uart_port *port)
>> +{
> 
>> +               xmit->tail = (xmit->tail + 1) & (SERIAL_XMIT_SIZE - 1);
> 
> % SERIAL_XMIT_SIZE shorter (I hope it's a power of 2), but it's up to you.

crisv10 and meson_uart have it this way, modulo normalized whitespace.
No serial driver uses % for it.

>> +}
> 
>> +static irqreturn_t owl_uart_irq(int irq, void *dev_id)
>> +{
>> +       struct uart_port *port = (struct uart_port *)dev_id;
> 
> Useless casting.

Fixed.

>> +       spin_lock(&port->lock);
> 
> spin_lock_irqsave() ?

Fixed, with matching _irqrestore().

> Consider the kernel command option that switches all IRQ handlers to
> be threaded.
> 
>> +}
> 
>> +static void owl_uart_change_baudrate(struct owl_uart_port *owl_port,
>> +                                    unsigned long baud)
>> +{
>> +       clk_set_rate(owl_port->clk, baud * 8);
>> +}
> 
>> +static void owl_uart_release_port(struct uart_port *port)
>> +{
>> +       struct platform_device *pdev = to_platform_device(port->dev);
>> +       struct resource *res;
>> +
> 
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       if (!res)
>> +               return;
>> +
>> +       if (port->flags & UPF_IOREMAP) {
>> +               devm_release_mem_region(port->dev, port->mapbase,
>> +                       resource_size(res));
>> +               devm_iounmap(port->dev, port->membase);
>> +               port->membase = NULL;
>> +       }
> 
> Above is entirely redundant.

Can you explain what this flag is used for and why some other drivers
implement it? That word alone is not helping.

>> +}
>> +
>> +static int owl_uart_request_port(struct uart_port *port)
>> +{
>> +       struct platform_device *pdev = to_platform_device(port->dev);
>> +       struct resource *res;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       if (!res)
>> +               return -ENXIO;
>> +
>> +       if (!devm_request_mem_region(port->dev, port->mapbase,
>> +                       resource_size(res), dev_name(port->dev)))
>> +               return -EBUSY;
>> +
>> +       if (port->flags & UPF_IOREMAP) {
>> +               port->membase = devm_ioremap_nocache(port->dev, port->mapbase,
>> +                               resource_size(res));
>> +               if (!port->membase)
>> +                       return -EBUSY;
>> +       }
>> +
>> +       return 0;
>> +}
> 
>> +static void owl_uart_config_port(struct uart_port *port, int flags)
>> +{
> 
>> +       if (flags & UART_CONFIG_TYPE) {
> 
> if (!(...))
>  return;
> 
> ?

Not a single serial driver does it that way.
I guess it prepares for handling other flags.

>> +               port->type = PORT_OWL;
>> +               owl_uart_request_port(port);
>> +       }
>> +}
> 
> 
>> +static int owl_uart_probe(struct platform_device *pdev)
>> +{
>> +       struct resource *res_mem, *res_irq;
>> +       struct owl_uart_port *owl_port;
>> +       int ret;
>> +
>> +       if (pdev->dev.of_node)
>> +               pdev->id = of_alias_get_id(pdev->dev.of_node, "serial");
>> +
>> +       if (pdev->id < 0 || pdev->id >= OWL_UART_PORT_NUM) {
>> +               dev_err(&pdev->dev, "id %d out of range\n", pdev->id);
>> +               return -EINVAL;
>> +       }
>> +
> 
> 
>> +       res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       if (!res_mem) {
>> +               dev_err(&pdev->dev, "could not get mem\n");
>> +               return -ENODEV;
>> +       }
> 
> You can use
> 
> struct resource *mem;
> 
> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> x = devm_ioremap_resource();
> if (IS_ERR(x))
>  return PTR_ERR(x);
> 
> and remote IOREMAP flag below.

>> +       res_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
>> +       if (!res_irq) {
>> +               dev_err(&pdev->dev, "could not get irq\n");
>> +               return -ENODEV;
>> +       }
> 
> platform_get_irq()

Adopted.

>> +       if (owl_uart_ports[pdev->id]) {
>> +               dev_err(&pdev->dev, "port %d already allocated\n", pdev->id);
>> +               return -EBUSY;
>> +       }
> 
> I guess it's redundant. It should be conflicting by resource (for example, IRQ).

No, currently not. Memory resources are allocated on request_port, IRQ
is requested on startup.

>> +       owl_port->clk = clk_get(&pdev->dev, NULL);
> 
> devm_ ?

Changed.

>> +       owl_port->port.iotype = UPIO_MEM;
>> +       owl_port->port.mapbase = res_mem->start;
>> +       owl_port->port.irq = res_irq->start;
> 
> 
>> +       owl_port->port.uartclk = clk_get_rate(owl_port->clk);
> 
> You need to check if it's 0 and use device property instead or bail out.

Fixed. Since this is a new driver, I'd rather not fall back to
clock-frequency property here. The binding has clocks property as required.

>> +       owl_port->port.flags = UPF_BOOT_AUTOCONF | UPF_IOREMAP | UPF_LOW_LATENCY;
>> +       owl_port->port.x_char = 0;
>> +       owl_port->port.fifosize = 16;
>> +       owl_port->port.ops = &owl_uart_ops;
>> +
>> +       owl_uart_ports[pdev->id] = owl_port;
> 
>> +       platform_set_drvdata(pdev, owl_port);
> 
> It should be just before return 0, right?..

Does it matter?

>> +
>> +       ret = uart_add_one_port(&owl_uart_driver, &owl_port->port);
>> +       if (ret)
>> +               owl_uart_ports[pdev->id] = NULL;
> 
> ...and thus, taking into consideration redundancy of that global var:
> 
>       ret = uart_add_one_port(&owl_uart_driver, &owl_port->port);
>       if (ret)
>           retrun ret;
> 
>       platform_set_drvdata(pdev, owl_port);
>       return 0;
> 
>> +       return ret;
>> +}
> 
>> +
>> +static int owl_uart_remove(struct platform_device *pdev)
>> +{
> 
>> +       struct owl_uart_port *owl_port;
>> +
>> +       owl_port = platform_get_drvdata(pdev);
> 
> Do above in one line.

Done.

>> +       uart_remove_one_port(&owl_uart_driver, &owl_port->port);
> 
>> +       owl_uart_ports[pdev->id] = NULL;
> 
> Redundant.
> 
>> +
>> +       return 0;
>> +}
> 
>> +/* Actions Semi Owl UART */
>> +#define PORT_OWL       117
> 
> We can use holes for now IIUC.
> 
> See 36 or alike

Okay. 36 works, too.

Please point to a good example of a serial driver that is not
"redundant" in your view. That will help also with other pending serial
drivers of mine. Thanks.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 NÃrnberg, Germany
GF: Felix ImendÃrffer, Jane Smithard, Graham Norton
HRB 21284 (AG NÃrnberg)