[v5,0/5] LiteX SoC controller and LiteUART serial driver

This patchset introduces support for LiteX SoC Controller
and LiteUART - serial device from LiteX SoC builder
(https://github.com/enjoy-digital/litex).

In the following patchset I will add
a new mor1kx-based (OpenRISC) platform that
uses this device.

Later I plan to extend this platform by
adding support for more devices from LiteX suite.

Changes in v5:
    - added Reviewed-by tag
    - removed custom accessors from SoC Controller's driver
    - fixed error checking in SoC Controller's driver

Changes in v4:
    - fixed copyright headers
    - fixed SoC Controller's yaml 
    - simplified SoC Controller's driver

Changes in v3:
    - added Acked-by and Reviewed-by tags
    - introduced LiteX SoC Controller driver
    - removed endianness detection (handled now by LiteX SoC Controller driver)
    - modified litex.h header
    - DTS aliases for LiteUART made optional
    - renamed SERIAL_LITEUART_NR_PORTS to SERIAL_LITEUART_MAX_PORTS
    - changed PORT_LITEUART from 122 to 123

Changes in v2:
    - binding description rewritten to a yaml schema file
    - added litex.h header with common register access functions

Filip Kokosinski (3):
  dt-bindings: vendor: add vendor prefix for LiteX
  dt-bindings: serial: document LiteUART bindings
  drivers/tty/serial: add LiteUART driver

Pawel Czarnecki (2):
  dt-bindings: soc: document LiteX SoC Controller bindings
  drivers/soc/litex: add LiteX SoC Controller driver

 .../bindings/serial/litex,liteuart.yaml       |  38 ++
 .../soc/litex/litex,soc-controller.yaml       |  39 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   9 +
 drivers/soc/Kconfig                           |   1 +
 drivers/soc/Makefile                          |   1 +
 drivers/soc/litex/Kconfig                     |  14 +
 drivers/soc/litex/Makefile                    |   3 +
 drivers/soc/litex/litex_soc_ctrl.c            | 197 +++++++++
 drivers/tty/serial/Kconfig                    |  31 ++
 drivers/tty/serial/Makefile                   |   1 +
 drivers/tty/serial/liteuart.c                 | 411 ++++++++++++++++++
 include/linux/litex.h                         |  45 ++
 include/uapi/linux/serial_core.h              |   3 +
 14 files changed, 795 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/litex,liteuart.yaml
 create mode 100644 Documentation/devicetree/bindings/soc/litex/litex,soc-controller.yaml
 create mode 100644 drivers/soc/litex/Kconfig
 create mode 100644 drivers/soc/litex/Makefile
 create mode 100644 drivers/soc/litex/litex_soc_ctrl.c
 create mode 100644 drivers/tty/serial/liteuart.c
 create mode 100644 include/linux/litex.h

On Sat, Apr 25, 2020 at 1:42 PM Mateusz Holenko <mholenko@antmicro.com> wrote:
>
> From: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
>
> This commit adds driver for the FPGA-based LiteX SoC
> Controller from LiteX SoC builder.
>
> Co-developed-by: Mateusz Holenko <mholenko@antmicro.com>
> Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> Signed-off-by: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> ---
>
> Notes:
>     Changes in v5:
>     - removed helper accessors and used __raw_readl/__raw_writel instead
>     - fixed checking for errors in litex_soc_ctrl_probe
>
>     Changes in v4:
>     - fixed indent in Kconfig's help section
>     - fixed copyright header
>     - changed compatible to "litex,soc-controller"
>     - simplified litex_soc_ctrl_probe
>     - removed unnecessary litex_soc_ctrl_remove
>
>     This commit has been introduced in v3 of the patchset.
>
>     It includes a simplified version of common 'litex.h'
>     header introduced in v2 of the patchset.
>
>  MAINTAINERS                        |   2 +
>  drivers/soc/Kconfig                |   1 +
>  drivers/soc/Makefile               |   1 +
>  drivers/soc/litex/Kconfig          |  14 ++
>  drivers/soc/litex/Makefile         |   3 +
>  drivers/soc/litex/litex_soc_ctrl.c | 197 +++++++++++++++++++++++++++++
>  include/linux/litex.h              |  45 +++++++
>  7 files changed, 263 insertions(+)
>  create mode 100644 drivers/soc/litex/Kconfig
>  create mode 100644 drivers/soc/litex/Makefile
>  create mode 100644 drivers/soc/litex/litex_soc_ctrl.c
>  create mode 100644 include/linux/litex.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 421ede6c4f71..1afe7348353b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9820,6 +9820,8 @@ M:        Karol Gugala <kgugala@antmicro.com>
>  M:     Mateusz Holenko <mholenko@antmicro.com>
>  S:     Maintained
>  F:     Documentation/devicetree/bindings/*/litex,*.yaml
> +F:     drivers/soc/litex/litex_soc_ctrl.c
> +F:     include/linux/litex.h
>
>  LIVE PATCHING
>  M:     Josh Poimboeuf <jpoimboe@redhat.com>
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index 1778f8c62861..78add2a163be 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -9,6 +9,7 @@ source "drivers/soc/bcm/Kconfig"
>  source "drivers/soc/fsl/Kconfig"
>  source "drivers/soc/imx/Kconfig"
>  source "drivers/soc/ixp4xx/Kconfig"
> +source "drivers/soc/litex/Kconfig"
>  source "drivers/soc/mediatek/Kconfig"
>  source "drivers/soc/qcom/Kconfig"
>  source "drivers/soc/renesas/Kconfig"
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index a39f17cea376..49bbb6ca6d95 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_ARCH_GEMINI)     += gemini/
>  obj-y                          += imx/
>  obj-$(CONFIG_ARCH_IXP4XX)      += ixp4xx/
>  obj-$(CONFIG_SOC_XWAY)         += lantiq/
> +obj-$(CONFIG_LITEX_SOC_CONTROLLER) += litex/
>  obj-y                          += mediatek/
>  obj-y                          += amlogic/
>  obj-y                          += qcom/
> diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig
> new file mode 100644
> index 000000000000..71264c0e1d6c
> --- /dev/null
> +++ b/drivers/soc/litex/Kconfig
> @@ -0,0 +1,14 @@
> +# SPDX-License_Identifier: GPL-2.0
> +
> +menu "Enable LiteX SoC Builder specific drivers"
> +
> +config LITEX_SOC_CONTROLLER
> +       tristate "Enable LiteX SoC Controller driver"
> +       help
> +         This option enables the SoC Controller Driver which verifies
> +         LiteX CSR access and provides common litex_get_reg/litex_set_reg
> +         accessors.
> +         All drivers that use functions from litex.h must depend on
> +         LITEX_SOC_CONTROLLER.
> +
> +endmenu
> diff --git a/drivers/soc/litex/Makefile b/drivers/soc/litex/Makefile
> new file mode 100644
> index 000000000000..98ff7325b1c0
> --- /dev/null
> +++ b/drivers/soc/litex/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License_Identifier: GPL-2.0
> +
> +obj-$(CONFIG_LITEX_SOC_CONTROLLER)     += litex_soc_ctrl.o
> diff --git a/drivers/soc/litex/litex_soc_ctrl.c b/drivers/soc/litex/litex_soc_ctrl.c
> new file mode 100644
> index 000000000000..16f1625836a5
> --- /dev/null
> +++ b/drivers/soc/litex/litex_soc_ctrl.c
> @@ -0,0 +1,197 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LiteX SoC Controller Driver
> + *
> + * Copyright (C) 2020 Antmicro <www.antmicro.com>
> + *
> + */
> +
> +#include <linux/litex.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/printk.h>
> +#include <linux/module.h>
> +#include <linux/errno.h>
> +#include <linux/io.h>
> +
> +/*
> + * The parameters below are true for LiteX SoC
> + * configured for 8-bit CSR Bus, 32-bit aligned.
> + *
> + * Supporting other configurations will require
> + * extending the logic in this header.
> + */
> +#define LITEX_REG_SIZE             0x4
> +#define LITEX_SUBREG_SIZE          0x1
> +#define LITEX_SUBREG_SIZE_BIT      (LITEX_SUBREG_SIZE * 8)
> +
> +static DEFINE_SPINLOCK(csr_lock);
> +
> +/*
> + * LiteX SoC Generator, depending on the configuration,
> + * can split a single logical CSR (Control & Status Register)
> + * into a series of consecutive physical registers.
> + *
> + * For example, in the configuration with 8-bit CSR Bus,
> + * 32-bit aligned (the default one for 32-bit CPUs) a 32-bit
> + * logical CSR will be generated as four 32-bit physical registers,
> + * each one containing one byte of meaningful data.
> + *
> + * For details see: https://github.com/enjoy-digital/litex/issues/314

This could link to the wiki
(https://github.com/enjoy-digital/litex/wiki/CSR-Bus) directly.
(Suggested by Gabriel Somlo <gsomlo@gmail.com>)

> + *
> + * The purpose of `litex_set_reg`/`litex_get_reg` is to implement
> + * the logic of writing to/reading from the LiteX CSR in a single
> + * place that can be then reused by all LiteX drivers.
> + */
> +void litex_set_reg(void __iomem *reg, unsigned long reg_size,
> +                   unsigned long val)
> +{
> +       unsigned long shifted_data, shift, i;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&csr_lock, flags);
> +
> +       for (i = 0; i < reg_size; ++i) {
> +               shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT);
> +               shifted_data = val >> shift;
> +
> +               __raw_writel(shifted_data, reg + (LITEX_REG_SIZE * i));

As Gabriel Somlo <gsomlo@gmail.com> suggested to me, I could still use
readl/writel/ioread/iowrite() standard functions providing memory
barriers *and* have values in CPU native endianness by using the
following constructs:

`le32_to_cpu(readl(addr))`

and

`writel(cpu_to_le32(value), addr)`

as le32_to_cpu/cpu_to_le32():
- does nothing on LE CPUs and
- reorders bytes on BE CPUs which in turn reverts swapping made by
readl() resulting in returning the original value.

> +       }
> +
> +       spin_unlock_irqrestore(&csr_lock, flags);
> +}
> +
> +unsigned long litex_get_reg(void __iomem *reg, unsigned long reg_size)
> +{
> +       unsigned long shifted_data, shift, i;
> +       unsigned long result = 0;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&csr_lock, flags);
> +
> +       for (i = 0; i < reg_size; ++i) {
> +               shifted_data = __raw_readl(reg + (LITEX_REG_SIZE * i));
> +
> +               shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT);
> +               result |= (shifted_data << shift);
> +       }
> +
> +       spin_unlock_irqrestore(&csr_lock, flags);
> +
> +       return result;
> +}
> +
> +static int accessors_ok;
> +
> +/*
> + * Check if accessors are safe to be used by other drivers
> + * returns true if yes - false if not
> + */
> +int litex_check_accessors(void)
> +{
> +       return accessors_ok;
> +}
> +
> +#define SCRATCH_REG_OFF         0x04
> +#define SCRATCH_REG_SIZE        4
> +#define SCRATCH_REG_VALUE       0x12345678
> +#define SCRATCH_TEST_VALUE      0xdeadbeef
> +
> +/*
> + * Check LiteX CSR read/write access
> + *
> + * This function reads and writes a scratch register in order
> + * to verify if CSR access works.
> + *
> + * In case any problems are detected, the driver should panic
> + * and not set `accessors_ok` flag. As a result no other
> + * LiteX driver should access CSR bus.
> + *
> + * Access to the LiteX CSR is, by design, done in CPU native
> + * endianness. The driver should not dynamically configure
> + * access functions when the endianness mismatch is detected.
> + * Such situation indicates problems in the soft SoC design
> + * and should be solved at the LiteX generator level,
> + * not in the software.
> + */
> +static int litex_check_csr_access(void __iomem *reg_addr)
> +{
> +       unsigned long reg;
> +
> +       reg = litex_get_reg(reg_addr + SCRATCH_REG_OFF, SCRATCH_REG_SIZE);
> +
> +       if (reg != SCRATCH_REG_VALUE) {
> +               panic("Scratch register read error! Expected: 0x%x but got: 0x%lx",
> +                       SCRATCH_REG_VALUE, reg);
> +               return -EINVAL;
> +       }
> +
> +       litex_set_reg(reg_addr + SCRATCH_REG_OFF,
> +               SCRATCH_REG_SIZE, SCRATCH_TEST_VALUE);
> +       reg = litex_get_reg(reg_addr + SCRATCH_REG_OFF, SCRATCH_REG_SIZE);
> +
> +       if (reg != SCRATCH_TEST_VALUE) {
> +               panic("Scratch register write error! Expected: 0x%x but got: 0x%lx",
> +                       SCRATCH_TEST_VALUE, reg);
> +               return -EINVAL;
> +       }
> +
> +       /* restore original value of the SCRATCH register */
> +       litex_set_reg(reg_addr + SCRATCH_REG_OFF,
> +               SCRATCH_REG_SIZE, SCRATCH_REG_VALUE);
> +
> +       /* Set flag for other drivers */
> +       accessors_ok = 1;
> +       pr_info("LiteX SoC Controller driver initialized");
> +
> +       return 0;
> +}
> +
> +struct litex_soc_ctrl_device {
> +       void __iomem *base;
> +};
> +
> +static const struct of_device_id litex_soc_ctrl_of_match[] = {
> +       {.compatible = "litex,soc-controller"},
> +       {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, litex_soc_ctrl_of_match);
> +
> +static int litex_soc_ctrl_probe(struct platform_device *pdev)
> +{
> +       struct device *dev;
> +       struct device_node *node;
> +       struct litex_soc_ctrl_device *soc_ctrl_dev;
> +
> +       dev = &pdev->dev;
> +       node = dev->of_node;
> +       if (!node)
> +               return -ENODEV;
> +
> +       soc_ctrl_dev = devm_kzalloc(dev, sizeof(*soc_ctrl_dev), GFP_KERNEL);
> +       if (!soc_ctrl_dev)
> +               return -ENOMEM;
> +
> +       soc_ctrl_dev->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(soc_ctrl_dev->base))
> +               return PTR_ERR(soc_ctrl_dev->base);
> +
> +       return litex_check_csr_access(soc_ctrl_dev->base);
> +}
> +
> +static struct platform_driver litex_soc_ctrl_driver = {
> +       .driver = {
> +               .name = "litex-soc-controller",
> +               .of_match_table = of_match_ptr(litex_soc_ctrl_of_match)
> +       },
> +       .probe = litex_soc_ctrl_probe,
> +};
> +
> +module_platform_driver(litex_soc_ctrl_driver);
> +MODULE_DESCRIPTION("LiteX SoC Controller driver");
> +MODULE_AUTHOR("Antmicro <www.antmicro.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/litex.h b/include/linux/litex.h
> new file mode 100644
> index 000000000000..f31062436273
> --- /dev/null
> +++ b/include/linux/litex.h
> @@ -0,0 +1,45 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Common LiteX header providing
> + * helper functions for accessing CSRs.
> + *
> + * Implementation of the functions is provided by
> + * the LiteX SoC Controller driver.
> + *
> + * Copyright (C) 2019-2020 Antmicro <www.antmicro.com>
> + */
> +
> +#ifndef _LINUX_LITEX_H
> +#define _LINUX_LITEX_H
> +
> +#include <linux/io.h>
> +#include <linux/types.h>
> +#include <linux/compiler_types.h>
> +
> +/*
> + * litex_check_accessors is a function implemented in
> + * drivers/soc/litex/litex_soc_controller.c
> + * checking if the common LiteX CSR accessors
> + * are safe to be used by the drivers;
> + * returns true (1) if yes - false (0) if not
> + *
> + * Important: All drivers that use litex_set_reg/litex_get_reg
> + * functions should make sure that LiteX SoC Controller driver
> + * has verified LiteX CSRs read and write operations before
> + * issuing any read/writes to the LiteX peripherals.
> + *
> + * Exemplary snippet that can be used at the beginning
> + * of the driver's probe() function to ensure that LiteX
> + * SoC Controller driver is properely initialized:
> + *
> + * if (!litex_check_accessors())
> + *     return -EPROBE_DEFER;
> + */
> +int litex_check_accessors(void);
> +
> +void litex_set_reg(void __iomem *reg, unsigned long reg_sz, unsigned long val);
> +
> +unsigned long litex_get_reg(void __iomem *reg, unsigned long reg_sz);
> +
> +
> +#endif /* _LINUX_LITEX_H */
> --
> 2.25.1
>


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

On Sat, Apr 25, 2020 at 2:45 PM Mateusz Holenko <mholenko@antmicro.com> wrote:
>
> From: Filip Kokosinski <fkokosinski@antmicro.com>
>
> 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@antmicro.com>
> Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>

Co-developed-by?

...

> --- 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.

...

> +config SERIAL_LITEUART
> +       tristate "LiteUART serial port support"
> +       depends on HAS_IOMEM

> +       depends on OF

|| COMPILE_TEST ?

> +       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?

...

> +#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_*?

...

> +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.

> +       .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?

...

> +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.

> +       /* 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.

> +       /* 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() ?

> +       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).

> +       },
> +};

...


> +static int __init liteuart_console_init(void)
> +{

Missed spin lock initialization.

> +       register_console(&liteuart_console);
> +
> +       return 0;
> +}

> +

Extra blank line.

> +console_initcall(liteuart_console_init);

...

> +/* LiteUART */
> +#define PORT_LITEUART  123

We have holes in the list, use them.

And again why we need this?

On Sat, 2020-04-25 at 13:42 +0200, Mateusz Holenko wrote:
> From: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> 
> This commit adds driver for the FPGA-based LiteX SoC
> Controller from LiteX SoC builder.

Sorry for jumping in late, Joel only just pointed me to this :)

> + * The purpose of `litex_set_reg`/`litex_get_reg` is to implement
> + * the logic of writing to/reading from the LiteX CSR in a single
> + * place that can be then reused by all LiteX drivers.
> + */
> +void litex_set_reg(void __iomem *reg, unsigned long reg_size,
> +		    unsigned long val)
> +{
> +	unsigned long shifted_data, shift, i;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&csr_lock, flags);
> +
> +	for (i = 0; i < reg_size; ++i) {
> +		shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT);
> +		shifted_data = val >> shift;
> +
> +		__raw_writel(shifted_data, reg + (LITEX_REG_SIZE * i));
> +	}
> +
> +	spin_unlock_irqrestore(&csr_lock, flags);
> +}
> +
> +unsigned long litex_get_reg(void __iomem *reg, unsigned long reg_size)
> +{
> +	unsigned long shifted_data, shift, i;
> +	unsigned long result = 0;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&csr_lock, flags);
> +
> +	for (i = 0; i < reg_size; ++i) {
> +		shifted_data = __raw_readl(reg + (LITEX_REG_SIZE * i));
> +
> +		shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT);
> +		result |= (shifted_data << shift);
> +	}
> +
> +	spin_unlock_irqrestore(&csr_lock, flags);
> +
> +	return result;
> +}

I really don't like the fact that the register sizes & sub sizes are
#defined. As your comment explains, this makes it harder to support
other configurations. This geometry should come from the device-tree
instead.

Also this while thing is rather gross (and the lock will not help
performance). Why can't CSRs be normally memory mapped always instead ?

Even when transporting them on a HW bus that's smaller, the HW bus
conversion should be able to do the break-down into a multi-breat
transfer rather than doing that in SW.

Or at least have a fast-path if the register size is no larger than the
sub size, so you can use a normal ioread32/iowrite32.

Also I wonder ... last I played with LiteX, it would re-generate the
register layout (including the bit layout inside registers potentially)
rather enthousiastically, making it pretty hard to have a fixed
register layout for use by a kernel driver. Was this addressed ?

Cheers,
Ben.

On Mon, 2020-04-27 at 11:13 +0200, Mateusz Holenko wrote:
> As Gabriel Somlo <gsomlo@gmail.com> suggested to me, I could still use
> readl/writel/ioread/iowrite() standard functions providing memory
> barriers *and* have values in CPU native endianness by using the
> following constructs:
> 
> `le32_to_cpu(readl(addr))`
> 
> and
> 
> `writel(cpu_to_le32(value), addr)`
> 
> as le32_to_cpu/cpu_to_le32():
> - does nothing on LE CPUs and
> - reorders bytes on BE CPUs which in turn reverts swapping made by
> readl() resulting in returning the original value.

It's a bit sad... I don't understand why you need this. The HW has a
fied endian has you have mentioned earlier (and that is a good design).

The fact that you are trying to shove things into a "smaller pipe" than
the actual register shouldn't affect at what address the MSB and LSB
reside. And readl/writel (or ioread32/iowrite32) will always be LE as
well, so will match the HW layout. Thus I don't see why you need to
play swapping games here.

This however would be avoided completely if the HW was a tiny bit
smarter and would do the multi-beat access for you which shouldn't be
terribly hard to implement.

That said, it would be even clearer if you just open coded the 2 or 3
useful cases: 32/8, 32/16 and 32/32. The loop with calculated shifts
(and no masks) makes the code hard to understand.

Cheers,
Ben.

Hi Ben,

On Wed, Apr 29, 2020 at 01:21:11PM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2020-04-27 at 11:13 +0200, Mateusz Holenko wrote:
> > As Gabriel Somlo <gsomlo@gmail.com> suggested to me, I could still use
> > readl/writel/ioread/iowrite() standard functions providing memory
> > barriers *and* have values in CPU native endianness by using the
> > following constructs:
> > 
> > `le32_to_cpu(readl(addr))`
> > 
> > and
> > 
> > `writel(cpu_to_le32(value), addr)`
> > 
> > as le32_to_cpu/cpu_to_le32():
> > - does nothing on LE CPUs and
> > - reorders bytes on BE CPUs which in turn reverts swapping made by
> > readl() resulting in returning the original value.
> 
> It's a bit sad... I don't understand why you need this. The HW has a
> fied endian has you have mentioned earlier (and that is a good design).
> 
> The fact that you are trying to shove things into a "smaller pipe" than
> the actual register shouldn't affect at what address the MSB and LSB
> reside. And readl/writel (or ioread32/iowrite32) will always be LE as
> well, so will match the HW layout. Thus I don't see why you need to
> play swapping games here.
> 
> This however would be avoided completely if the HW was a tiny bit
> smarter and would do the multi-beat access for you which shouldn't be
> terribly hard to implement.
> 
> That said, it would be even clearer if you just open coded the 2 or 3
> useful cases: 32/8, 32/16 and 32/32. The loop with calculated shifts
> (and no masks) makes the code hard to understand.

A "compound" LiteX MMIO register of 32 bits total, starting at address
0x80000004, containing value 0x12345678, is spread across 4 8-bit
subregisters aligned at ulong in the MMIO space like this on LE:

0x82000000  00 00 00 00 12 00 00 00 34 00 00 00 56 00 00 00  ........4...V...
                        ^^^^^^^^^^^ ^^^^^^^^^^^ ^^^^^^^^^^^
0x82000010  78 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  x...............
            ^^^^^^^^^^^

and like this on BE:

0x82000000  00 00 00 00 00 00 00 12 00 00 00 34 00 00 00 56  ...........4...V
                        ^^^^^^^^^^^ ^^^^^^^^^^^ ^^^^^^^^^^^
0x82000010  00 00 00 78 00 00 00 00 00 00 00 00 00 00 00 00  ...x............
            ^^^^^^^^^^^

LiteX can be optionally built to use larger than 8-bit subregisters,
here's an example with 16-bit subregisters (also aligned at ulong),
for the same "compound" register:

on LE:
0x82000000  00 00 00 00 34 12 00 00 78 56 00 00 00 00 00 00  ....4...xV......
                        ^^^^^^^^^^^ ^^^^^^^^^^^

and on BE:
0x82000000  00 00 00 00 00 00 12 34 00 00 56 78 00 00 00 00  .......4..Vx....
                        ^^^^^^^^^^^ ^^^^^^^^^^^

Essentially (back to the more common 8-bit subregister size), a compound
register foo = 0x12345678 is stored as

	ulong foo[4] = {0x12, 0x34, 0x56, 0x78};

in the CPU's native endianness, aligned at the CPU's native word width
(hence "ulong").

With 16-bit subregisters that would then be:

	ulong foo[2] = {0x1234, 0x5678};

Trouble with readl() and writel() is that they convert everything to LE
internally, which on BE would get us something different *within* each
subregister (i.e., 0x12000000 instead of 0x12, or 0x34120000 instead of
0x1234).

The cleanest way (IMHO) to accomplish an endian-agnostic readl() (that
preserves both barriers AND native endianness) is to undo the internal
__le32_to_cpu() using:

	cpu_to_le32(readl(addr))

This keeps us away from using any '__' internals directly (e.g.,
__raw_readl()), or open-coding our own `litex_readl()`, e.g.:

	static inline u32 litex_readl(const volatile void __iomem *addr)
	{
		u32 val;
		__io_br();
		val = __raw_readl(addr)); /* No le32 byteswap here! */
		__io_ar(val);
		return val;
	}

... which is something that was strongly advised against in earlier
revisions of this series.

Cheers,
--Gabriel

On Tue, Apr 28, 2020 at 5:50 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sat, Apr 25, 2020 at 2:45 PM Mateusz Holenko <mholenko@antmicro.com> wrote:
> >
> > From: Filip Kokosinski <fkokosinski@antmicro.com>
> >
> > 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@antmicro.com>
> > Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
>
> 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

On Mon, May 4, 2020 at 4:44 PM Mateusz Holenko <mholenko@antmicro.com> wrote:
> On Tue, Apr 28, 2020 at 5:50 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sat, Apr 25, 2020 at 2:45 PM Mateusz Holenko <mholenko@antmicro.com> wrote:

...

> > > Signed-off-by: Filip Kokosinski <fkokosinski@antmicro.com>
> > > Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> >
> > 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 :)

I see.

...

> > > +#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.

So, can't you simple use them as is? I still didn't get how SIZE helps here.

...

> > > +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.

The problem here that .driver_name is a part of user visible
interface, so, when you rename it it will affect the module alias.
How DEV_NAME is shared? It should not be, otherwise it will collide
with other drivers.

> > > +       .major = DRIVER_MAJOR,
> > > +       .minor = DRIVER_MINOR,
> >
> > Ditto.

Ditto.

> > > +};

...

> > > +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.

Legacy drivers are not the best example to take. So, if you can
survive without UART type, please go with it. Otherwise commit message
should point out why it's needed so eagerly.

...

> > > +       /* 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.

Rather consider to use xArray API.

...

> > > +               .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.

Yes.

...

> > > +static int __init liteuart_console_init(void)
> > > +{

> > > +}
> >
> > > +
> >
> > 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.

Less LOCs is good (but keep common sense applied).

...

> > > +/* 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.

See above. This ID is a part of UAPI which is kinda redundant nowadays.
You need to provide a good argument for that. Otherwise, get rid of it.

On Wed, Apr 29, 2020 at 5:12 AM Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> On Sat, 2020-04-25 at 13:42 +0200, Mateusz Holenko wrote:
> > From: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> >
> > This commit adds driver for the FPGA-based LiteX SoC
> > Controller from LiteX SoC builder.
>
> Sorry for jumping in late, Joel only just pointed me to this :)
>
> > + * The purpose of `litex_set_reg`/`litex_get_reg` is to implement
> > + * the logic of writing to/reading from the LiteX CSR in a single
> > + * place that can be then reused by all LiteX drivers.
> > + */
> > +void litex_set_reg(void __iomem *reg, unsigned long reg_size,
> > +                 unsigned long val)
> > +{
> > +     unsigned long shifted_data, shift, i;
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&csr_lock, flags);
> > +
> > +     for (i = 0; i < reg_size; ++i) {
> > +             shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT);
> > +             shifted_data = val >> shift;
> > +
> > +             __raw_writel(shifted_data, reg + (LITEX_REG_SIZE * i));
> > +     }
> > +
> > +     spin_unlock_irqrestore(&csr_lock, flags);
> > +}
> > +
> > +unsigned long litex_get_reg(void __iomem *reg, unsigned long reg_size)
> > +{
> > +     unsigned long shifted_data, shift, i;
> > +     unsigned long result = 0;
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&csr_lock, flags);
> > +
> > +     for (i = 0; i < reg_size; ++i) {
> > +             shifted_data = __raw_readl(reg + (LITEX_REG_SIZE * i));
> > +
> > +             shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT);
> > +             result |= (shifted_data << shift);
> > +     }
> > +
> > +     spin_unlock_irqrestore(&csr_lock, flags);
> > +
> > +     return result;
> > +}
>
> I really don't like the fact that the register sizes & sub sizes are
> #defined. As your comment explains, this makes it harder to support
> other configurations. This geometry should come from the device-tree
> instead.

This is a valid point - putting those parameters into DT would indeed allow
for more flexibility. Currently we are focusing on supporting a single LiteX
configuration (32-bit/8-bit bus) and that's why those parameters got fixed.

Adding support for other configurations however is not just changing those
two parameters (which should be fairly easy by itself), but also
handling different
registers layout for a single peripheral (in different configurations
CSRs offsets and sizes may differ).

Since this adds another layer of complexity we want to start with a simpler
version that can be extended in the future.

> Also this while thing is rather gross (and the lock will not help
> performance). Why can't CSRs be normally memory mapped always instead ?

Using a different LiteX configuration 32-bit/32-bit bus would solve
the problem -
a single LiteX CSR would map nicely to a single 32-bit memory pointer and no
loop/locks would be needed.

In the default configuration (32-bit/8-bit bus) there are gaps between bytes
(as Gabriel Somlo already explained in his mail) which need to be handled
"manually".

> Even when transporting them on a HW bus that's smaller, the HW bus
> conversion should be able to do the break-down into a multi-breat
> transfer rather than doing that in SW.
>
> Or at least have a fast-path if the register size is no larger than the
> sub size, so you can use a normal ioread32/iowrite32.

Again - this is possible, but using a non-default 32-bit/32-bit bus LiteX
configuration.

> Also I wonder ... last I played with LiteX, it would re-generate the
> register layout (including the bit layout inside registers potentially)
> rather enthousiastically, making it pretty hard to have a fixed
> register layout for use by a kernel driver. Was this addressed ?

TBH I never experienced bit layout inside a register to change by itself,
but I agree that using different bus width configurations causes CSRs
to be splitted into 4/2/1 32-bit registers (changing de facto the layout
from the SW perspective) - that's why we provide helper functions
in this file.

It is possible to have different configurations of a peripheral
in LiteX that e.g, turns features on/off - this might cause some CSRs
to shift and result in incompatibilities. There are ways in LiteX
to avoid such problems if the model is properly designed, though.

Another aspect of LiteX is that the order in which separate peripherals
(modules) are created results in a different memory map of the whole SoC.
This, however, is easily addressed by using a dynamically generated DT
and do not require the code of drivers to be altered in any way.

> Cheers,
> Ben.
>
>

Thanks for your comments!

On Tue, May 5, 2020 at 4:02 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, May 4, 2020 at 4:44 PM Mateusz Holenko <mholenko@antmicro.com> wrote:
> > On Tue, Apr 28, 2020 at 5:50 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Sat, Apr 25, 2020 at 2:45 PM Mateusz Holenko <mholenko@antmicro.com> wrote:
>
> ...
>
> > > > Signed-off-by: Filip Kokosinski <fkokosinski@antmicro.com>
> > > > Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> > >
> > > 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 :)
>
> I see.
>
> ...
>
> > > > +#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.
>
> So, can't you simple use them as is? I still didn't get how SIZE helps here.
>
> ...

Do you mean to call litex_get_reg() with 1 directly as a second argument instead
of using a defined value (which in the context of uart driver is 1 for
every register anyway)?

Sure, this is doable. We just thought a named define will explain
better what's going on.

In general case, register's offsets and sizes might differ between
LiteX configurations
(LiteX is an SoC generator capable of creating different setups) so
having a dynamic
size in litex_get_reg() is useful.
With this patchset we are targeting a single (default) configuration
(so it's enough for
OFF_ and SIZE_ to be fixed), but this could be extended to be more
dynamic in the future.

> > > > +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.
>
> The problem here that .driver_name is a part of user visible
> interface, so, when you rename it it will affect the module alias.
> How DEV_NAME is shared? It should not be, otherwise it will collide
> with other drivers.

I meant that DRIVER_NAME define is used in the file in many places:
* liteuart_driver.driver_name
* liteuart_type()
* liteuart_platfrom_driver.driver.name
* liteuart_console.name
* MODULE_ALIAS
It's not shared with other drivers, but used multiple times in the
code of this one.

DEV_NAME/DRIVER_MAJOR/DRIVER_MINOR are indeed referenced only once so
it's no problem to write those values directly and get rid of their
defines.

> > > > +       .major = DRIVER_MAJOR,
> > > > +       .minor = DRIVER_MINOR,
> > >
> > > Ditto.
>
> Ditto.
>
> > > > +};
>
> ...

What do you mean by '...' ?

>
> > > > +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.
>
> Legacy drivers are not the best example to take. So, if you can
> survive without UART type, please go with it. Otherwise commit message
> should point out why it's needed so eagerly.
>
> ...

I guess we'll be good without the UART type.

> > > > +       /* 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.
>
> Rather consider to use xArray API.
>
> ...

We'll check that up.

> > > > +               .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.
>
> Yes.
>
> ...

OK.

> > > > +static int __init liteuart_console_init(void)
> > > > +{
>
> > > > +}
> > >
> > > > +
> > >
> > > 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.
>
> Less LOCs is good (but keep common sense applied).
>
> ...

Sure.

> > > > +/* 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.
>
> See above. This ID is a part of UAPI which is kinda redundant nowadays.
> You need to provide a good argument for that. Otherwise, get rid of it.

We'll remove this and see if everything works fine.

> --
> With Best Regards,
> Andy Shevchenko

Thanks for the discussion!


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