Re: [PATCH v2 1/4] uart-routing: Add common UART routing framework

[Changhuang Liang]

Hi, Julian

Thanks for the review

> On 9/5/26 11:28, Changhuang Liang wrote:
> > Several SoCs contain a serial crossbar, usually called UART routing,
> > that lets the RX line of any on-chip UART controller or physical
> > serial port be fed from any other endpoint. The Aspeed
> > AST2400/2500/2600 and the StarFive JHB100 both have one, and both
> > expose it through the same user space interface: one sysfs file per
> > endpoint, listing the routing targets with the current one in square brackets.
> >
> > The two drivers implementing that interface duplicate the whole sysfs
> > plumbing and only really differ in the description of their register
> > layout, so factor the common part out into a framework.
> >
> > An SoC driver now only describes each mux with a struct
> > uart_routing_selector - register offset, bit position, field mask and
> > the array of routing target names indexed by the raw field value -
> > gathers them in an attribute group and hands the group and a regmap to
> > devm_uart_routing_register(). The framework validates the description,
> > creates the files and implements the show()/store() handlers.
> >
> > The framework keeps its state in a devres node rather than in the
> > device drvdata, so drivers stay free to use dev_set_drvdata() for
> > their own purposes, and the sysfs files are removed by devres, so
> > drivers do not need a remove() callback for them.
> >
> > Signed-off-by: Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> > ---
> >  Documentation/driver-api/index.rst        |   1 +
> >  Documentation/driver-api/uart-routing.rst | 145 ++++++++++++++++
> >  MAINTAINERS                               |   7 +
> >  drivers/Kconfig                           |   2 +
> >  drivers/Makefile                          |   1 +
> >  drivers/uart-routing/Kconfig              |  16 ++
> >  drivers/uart-routing/Makefile             |   2 +
> >  drivers/uart-routing/uart-routing.c       | 200
> ++++++++++++++++++++++
> >  drivers/uart-routing/uart-routing.h       |  84 +++++++++
> >  9 files changed, 458 insertions(+)
> >  create mode 100644 Documentation/driver-api/uart-routing.rst
> >  create mode 100644 drivers/uart-routing/Kconfig  create mode 100644
> > drivers/uart-routing/Makefile  create mode 100644
> > drivers/uart-routing/uart-routing.c
> >  create mode 100644 drivers/uart-routing/uart-routing.h
> >
> > diff --git a/Documentation/driver-api/index.rst
> > b/Documentation/driver-api/index.rst
> > index 6601a258690f..2a0f375cd206 100644
> > --- a/Documentation/driver-api/index.rst
> > +++ b/Documentation/driver-api/index.rst
> > @@ -146,6 +146,7 @@ Subsystem-specific APIs
> >     tee
> >     thermal/index
> >     tty/index
> > +   uart-routing
> >     wbrf
> >     wmi
> >     xilinx/index
> > diff --git a/Documentation/driver-api/uart-routing.rst
> > b/Documentation/driver-api/uart-routing.rst
> > new file mode 100644
> > index 000000000000..67bb84958977
> > --- /dev/null
> > +++ b/Documentation/driver-api/uart-routing.rst
> > @@ -0,0 +1,145 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +======================
> > +UART routing framework
> > +======================
> > +
> > +:Author: Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> > +
> > +Overview
> > +========
> > +
> > +Several SoCs contain a serial crossbar, usually called *UART
> > +routing*, that sits between the on-chip UART controllers and the
> > +physical serial ports exposed on the package pins. The crossbar lets
> > +the RX line of any endpoint be fed from any other endpoint, which
> > +makes it possible to, for example, snoop the traffic of a host serial
> > +console, or to loop two on-chip UARTs back into each other without any
> external wiring.
> > +
> > +Two endpoint families are involved:
> > +
> > +``uartN``
> > +	the RX line of the on-chip UART controller number N.
> > +
> > +``ioN``
> > +	the RX line of the physical serial port number N.
> > +
> > +The crossbar is programmed through bit fields, one per endpoint,
> > +spread over one or more registers. The value written into a field
> > +picks the endpoint the RX line is connected to; the meaning of a
> > +given value differs from field to field and from SoC to SoC.
> > +
> > +The framework in ``drivers/uart-routing/`` takes a static description
> > +of those fields and turns it into a set of sysfs files, one per
> > +endpoint, so that SoC drivers only have to describe their hardware.
> > +
> > +User space interface
> > +====================
> > +
> > +Every endpoint gets one read/write file in the device directory of
> > +the platform driver, named after the endpoint. Reading the file lists
> > +all the routing targets the endpoint can be connected to, with the
> > +current one enclosed in square brackets::
> > +
> > +	# cat
> /sys/bus/platform/drivers/aspeed-uart-routing/*.uart_routing/uart1
> > +	[io1] io2 io3 io4 uart2 uart3 uart4 io6
> > +
> > +Writing one of the listed names to the file changes the routing::
> > +
> > +	# echo uart2 >
> > +/sys/bus/platform/drivers/aspeed-uart-routing/*.uart_routing/uart1
> > +
> > +Writing a name that is not part of the list fails with ``-EINVAL``.
> > +
> > +The list is not necessarily the same for every file: it is ordered by
> > +the raw value programmed into the hardware, so the first entry is the
> > +target selected when the field reads back as 0. Some SoCs define
> > +fields that are wider than the number of documented targets. When
> > +such a field holds a value with no name attached, the read appends
> > +``[unknown(N)]`` to the list instead of bracketing one of the names.
> > +
> > +Fields whose name is ``reserved`` are placeholders for values the
> > +hardware does not implement. They are listed so that the position of
> > +the following names stays correct, and writing ``reserved`` programs
> > +a value that has no defined behaviour, so do not do that.
> > +
> > +The exact set of files of a given SoC, together with the routing
> > +targets each of them accepts, is described in the corresponding
> > +``Documentation/ABI/testing/sysfs-driver-*-uart-routing`` file.
> > +
> > +Writing a driver
> > +================
> > +
> > +An SoC driver describes each mux with a ``struct
> > +uart_routing_selector``, defined with the ``UART_ROUTING_SELECTOR()``
> helper::
> > +
> > +	static const char *const foo_uart1_options[] = {
> > +		"io1", "io2", "io3", "io4", "uart2", "uart3", NULL,
> > +	};
> > +	UART_ROUTING_SELECTOR(foo_uart1_sel, uart1, FOO_MUX_REG, 16,
> 0x7,
> > +			      foo_uart1_options);
> > +
> > +The arguments are, in order, the name of the variable to define, the
> > +name of the sysfs file, the offset of the register holding the field,
> > +the position of the least significant bit of the field, the field
> > +mask and the array of routing targets.
> > +
> > +The mask is given in field coordinates, that is, it is *not* shifted
> > +by the bit position: a three bit field is always described as
> > +``0x7``, whatever its position in the register is.
> > +
> > +The array of routing targets is indexed by the raw field value, so
> > +``options[n]`` is the name of the target selected when the field
> > +holds n. It has to be NULL terminated, and it may be shared between
> > +several selectors that happen to have the same target order.
> > +
> > +The selectors are then gathered in an attribute group::
> > +
> > +	static struct attribute *foo_uart_routing_attrs[] = {
> > +		UART_ROUTING_SELECTOR_ATTR(foo_uart1_sel),
> > +		/* ... */
> > +		NULL,
> > +	};
> > +
> > +	static const struct attribute_group foo_uart_routing_attr_group = {
> > +		.attrs = foo_uart_routing_attrs,
> > +	};
> > +
> > +and handed over, along with a regmap covering the selector registers,
> > +in
> > +probe()::
> > +
> > +	static int foo_uart_routing_probe(struct platform_device *pdev)
> > +	{
> > +		struct device *dev = &pdev->dev;
> > +		struct regmap *regmap;
> > +
> > +		regmap = /* ... */;
> > +
> > +		return devm_uart_routing_register(dev, regmap,
> > +						  &foo_uart_routing_attr_group);
> > +	}
> > +
> > +The framework validates the description at registration time and
> > +rejects selectors whose target list cannot fit in the field, or whose
> > +field does not fit in a 32 bit register.
> > +
> > +The sysfs files are created and removed by devres, so a driver does
> > +not need a remove() callback for them. The framework keeps its own
> > +state in a devres node rather than in the device drvdata, so drivers
> > +are free to use ``dev_set_drvdata()`` for their own purposes.
> > +
> > +Locking
> > +=======
> > +
> > +The framework does not serialise accesses itself. The
> > +read-modify-write of a selector field is done with
> > +``regmap_update_bits()``, so concurrent writes to two endpoints
> > +sharing a register are made safe by the regmap lock. Reading a file
> > +always reports the current hardware state, which may have been changed
> by another writer in between.
> > +
> > +API reference
> > +=============
> > +
> > +.. kernel-doc:: drivers/uart-routing/uart-routing.h
> > +
> > +.. kernel-doc:: drivers/uart-routing/uart-routing.c
> > +   :export:
> > diff --git a/MAINTAINERS b/MAINTAINERS index
> > 90919c672d4d..b7fa722755c6 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -27818,6 +27818,13 @@ F:	drivers/misc/uacce/
> >  F:	include/linux/uacce.h
> >  F:	include/uapi/misc/uacce/
> >
> > +UART ROUTING FRAMEWORK
> > +M:	Changhuang Liang <changhuang.liang@xxxxxxxxxxxxxxxx>
> > +L:	openbmc@xxxxxxxxxxxxxxxx (moderated for non-subscribers)
> > +S:	Maintained
> > +F:	Documentation/driver-api/uart-routing.rst
> > +F:	drivers/uart-routing/
> > +
> >  UBI FILE SYSTEM (UBIFS)
> >  M:	Richard Weinberger <richard@xxxxxx>
> >  R:	Zhihao Cheng <chengzhihao1@xxxxxxxxxx>
> > diff --git a/drivers/Kconfig b/drivers/Kconfig index
> > f2bed2ddeb66..cb2b5c3b22b0 100644
> > --- a/drivers/Kconfig
> > +++ b/drivers/Kconfig
> > @@ -251,6 +251,8 @@ source "drivers/hte/Kconfig"
> >
> >  source "drivers/cdx/Kconfig"
> >
> > +source "drivers/uart-routing/Kconfig"
> > +
> >  source "drivers/resctrl/Kconfig"
> >
> >  endmenu
> > diff --git a/drivers/Makefile b/drivers/Makefile index
> > 0841ea851847..fe4b11419496 100644
> > --- a/drivers/Makefile
> > +++ b/drivers/Makefile
> > @@ -195,6 +195,7 @@ obj-$(CONFIG_DRM_ACCEL)		+= accel/
> >  obj-$(CONFIG_CDX_BUS)		+= cdx/
> >  obj-$(CONFIG_DPLL)		+= dpll/
> >  obj-y				+= resctrl/
> > +obj-$(CONFIG_UART_ROUTING)	+= uart-routing/
> >
> >  obj-$(CONFIG_DIBS)		+= dibs/
> >  obj-$(CONFIG_S390)		+= s390/
> > diff --git a/drivers/uart-routing/Kconfig
> > b/drivers/uart-routing/Kconfig new file mode 100644 index
> > 000000000000..a0c45a7bba47
> > --- /dev/null
> > +++ b/drivers/uart-routing/Kconfig
> > @@ -0,0 +1,16 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +
> > +menu "UART routing drivers"
> > +
> 
> Configuring the kernel can be an annoying process, and on platforms that can't
> use these options, this menu just appears empty, further complicating
> menuconfig...
> 
> Maybe it makes sense to add a dependency to the menu like this?
> 'depends on ARCH_ASPEED || ARCH_STARFIVE || COMPILE_TEST'
> 


Excellent advice. Thank you.

Best Regards,
Changhuang