Re: [PATCH v5 5/5] drivers/tty/serial: add LiteUART driver

[Mateusz Holenko]

On Tue, Apr 28, 2020 at 5:50 PM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
>
> On Sat, Apr 25, 2020 at 2:45 PM Mateusz Holenko <mholenko@xxxxxxxxxxxx> wrote:
> >
> > From: Filip Kokosinski <fkokosinski@xxxxxxxxxxxx>
> >
> > This commit adds driver for the FPGA-based LiteUART serial controller
> > from LiteX SoC builder.
> >
> > The current implementation supports LiteUART configured
> > for 32 bit data width and 8 bit CSR bus width.
> >
> > It does not support IRQ.
> >
> > Signed-off-by: Filip Kokosinski <fkokosinski@xxxxxxxxxxxx>
> > Signed-off-by: Mateusz Holenko <mholenko@xxxxxxxxxxxx>
>
> Co-developed-by?

Most of the coding here is done by Filip Kokosinski - I'm responsible
for managing the patches and sending to LKML so I don't think I
qualify as a co-developer :)

> ...
>
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9731,6 +9731,7 @@ S:        Maintained
> >  F:     Documentation/devicetree/bindings/*/litex,*.yaml
> >  F:     drivers/soc/litex/litex_soc_ctrl.c
> >  F:     include/linux/litex.h
> > +F:     drivers/tty/serial/liteuart.c
>
> Ordering issue, run latest checkpatch.pl and parse-maintaners.pl to fix.

We'll check that.

> ...
>
> > +config SERIAL_LITEUART
> > +       tristate "LiteUART serial port support"
> > +       depends on HAS_IOMEM
>
> > +       depends on OF
>
> || COMPILE_TEST ?

Sure, we'll add that.

> > +       depends on LITEX_SOC_CONTROLLER
> > +       select SERIAL_CORE
>
> ...
>
> > +/*
> > + * CSRs definitions
> > + * (base address offsets + width)
> > + *
> > + * The definitions below are true for
> > + * LiteX SoC configured for
> > + * 8-bit CSR Bus, 32-bit aligned.
> > + *
> > + * Supporting other configurations
> > + * might require new definitions
> > + * or a more generic way of indexing
> > + * the LiteX CSRs.
> > + *
> > + * For more details on how CSRs
> > + * are defined and handled in LiteX,
> > + * see comments in the LiteX SoC Driver:
> > + * drivers/soc/litex/litex_soc_ctrl.c
> > + */
>
> Can you use some like 76 characters per line?
>

We'll reformat the code to match 76 chars.

> ...
>
> > +#define OFF_RXTX       0x00
> > +#define SIZE_RXTX      1
> > +#define OFF_TXFULL     0x04
> > +#define SIZE_TXFULL    1
> > +#define OFF_RXEMPTY    0x08
> > +#define SIZE_RXEMPTY   1
> > +#define OFF_EV_STATUS  0x0c
> > +#define SIZE_EV_STATUS 1
> > +#define OFF_EV_PENDING 0x10
> > +#define SIZE_EV_PENDING        1
> > +#define OFF_EV_ENABLE  0x14
> > +#define SIZE_EV_ENABLE 1
>
> Why do you need all those SIZE_*?
>
> ...

This is related to how LiteX peripherals (LiteUART being one of them)
handle register access.
The LiteX HW splits a classic 32-bit register into 4 32-bit registers,
each one containing only 8-bit part of it.

SIZE in this context means how many of those "subregisters" (still
32-bit wide, but with only 8-bit of meaningful data) to read/write.
The "litex.h" header (patch 3 of this patchset) provides common
functions for doing it, but it must know the size for each register.

>
> > +static struct uart_driver liteuart_driver = {
> > +       .owner = THIS_MODULE,
> > +       .driver_name = DRIVER_NAME,
> > +       .dev_name = DEV_NAME,
>
> Much easier to see if any name collisions are happen by grepping
> similar struct definitions, but these macros are making life harder.

Do you mean to avoid indirection caused by defines and write e.g.,
`.driver_name = "liteuart"`?

OK, but the reason we have defines in the first place is because we
use the same name in many places and we want to avoid inconsistencies
(typos, partial rename, etc.).
What's more, looking at other serial drivers I see the notation is not
consistent - many of them use defines for name/major/minor as well.

> > +       .major = DRIVER_MAJOR,
> > +       .minor = DRIVER_MINOR,
>
> Ditto.
>
> > +       .nr = CONFIG_SERIAL_LITEUART_MAX_PORTS,
>
> > +#ifdef CONFIG_SERIAL_LITEUART_CONSOLE
> > +       .cons = &liteuart_console,
> > +#endif
>
> > +};
>
> ...
>
> > +static const char *liteuart_type(struct uart_port *port)
> > +{
> > +       return (port->type == PORT_LITEUART) ? DRIVER_NAME : NULL;
> > +}
>
> Do we need this check? Do we need a port type at all?
>
> ...

This is inspired by serial_core.c and other serial drivers.
We don't support any alternative `port->types` values so it's probably
not necessary for us, but it seems that this is how other serial
drivers are written too.

> > +static int liteuart_probe(struct platform_device *pdev)
> > +{
> > +       struct device_node *np = pdev->dev.of_node;
> > +       struct liteuart_port *uart;
> > +       struct uart_port *port;
> > +       int dev_id;
> > +
> > +       if (!litex_check_accessors())
> > +               return -EPROBE_DEFER;
> > +
>
> > +       /* no device tree */
> > +       if (!np)
> > +               return -ENODEV;
>
> I guess it should go first, otherwise potentially you may end up with
> deferred module above.

You are right. We'll reorder the initialization.

> > +       /* look for aliases; auto-enumerate for free index if not found */
> > +       dev_id = of_alias_get_id(np, "serial");
> > +       if (dev_id < 0)
> > +               dev_id = find_first_zero_bit(liteuart_ports_in_use,
> > +                                            CONFIG_SERIAL_LITEUART_MAX_PORTS);
>
> Racy.

We'll protect it with a mutex to avoid race conditions.

> > +       /* get {map,mem}base */
> > +       port->mapbase = platform_get_resource(pdev, IORESOURCE_MEM, 0)->start;
> > +       port->membase = of_iomap(np, 0);
>
> Can't you use devm_platform_get_and_ioremap_resource() ?

This indeed can be simplified.

> > +       if (!port->membase)
> > +               return -ENXIO;
>
> > +}
>
> ...
>
> > +static struct platform_driver liteuart_platform_driver = {
> > +       .probe = liteuart_probe,
> > +       .remove = liteuart_remove,
> > +       .driver = {
> > +               .name = DRIVER_NAME,
>
> > +               .of_match_table = of_match_ptr(liteuart_of_match),
>
> of_match_ptr() makes no sense (you have depends on OF).

You mean that `of_match_ptr(X)` resolves simply to `X` when
`CONFIG_OF` is defined?
In this context it surely can be simplified.

> > +       },
> > +};
>
> ...
>
>
> > +static int __init liteuart_console_init(void)
> > +{
>
> Missed spin lock initialization.

We'll fix this.

> > +       register_console(&liteuart_console);
> > +
> > +       return 0;
> > +}
>
> > +
>
> Extra blank line.

You mean we should remove an empty line between the definition of
liteuart_console_init() and the call to console_initcall()? It seems
to be inconsistent across different drivers, but sure - no problem.

> > +console_initcall(liteuart_console_init);
>
> ...
>
> > +/* LiteUART */
> > +#define PORT_LITEUART  123
>
> We have holes in the list, use them.
>
> And again why we need this?

This is inspired by other serial drivers that also reserves
identifiers in this file and handles them the same way we do. We
simply followed the convention.

> --
> With Best Regards,
> Andy Shevchenko

Thanks for your time and the comments! We'll address them in the next
version of the patchset.

Best regards,
Mateusz HoÅenko

--
Mateusz Holenko
Antmicro Ltd | www.antmicro.com
Roosevelta 22, 60-829 Poznan, Poland