On Sun, 2026-09-06 at 20:00 +0100, Gary Guo wrote: > On Sun Sep 6, 2026 at 6:39 PM BST, Markus Probst wrote: > > On Fri, 2026-09-04 at 10:48 +0530, Ayush Singh wrote: > > > On 9/4/26 2:19 AM, Markus Probst wrote: > > > > > > > On Thu, 2026-08-27 at 13:24 +0530, Ayush Singh wrote: > > > > > Add a driver for Greybus nodes attached over a plain serial port. The > > > > > node is registered with the software SVC (gb-softsvc), which handles the > > > > > SVC protocol on behalf of the AP, so no dedicated coprocessor running > > > > > SVC firmware is needed. > > > > > > > > > > Greybus messages are carried over HDLC framing on the wire. Each frame > > > > > carries a one-byte address (0x01 for Greybus) and control byte, followed > > > > > by the 16-bit CPort ID and the Greybus message itself. > > > > > > > > > > Port parameters are taken from the firmware node: "baudrate" if > > > > > present, otherwise 115200, with flow control and parity disabled. > > > > > > > > > > Since gb-uart-node imports types from gb-softsvc, Rust to Rust calling > > > > > setup from nova-core [0] is being used. > > > > > > > > > > [0]: https://lore.kernel.org/all/20260622-nova-exports-v5-0-6191773fc977@nvidia.com/ > > > > > > > > > > Signed-off-by: Ayush Singh > > > > > --- > > > > > MAINTAINERS | 1 + > > > > > drivers/greybus/.gitignore | 1 + > > > > > drivers/greybus/Kconfig | 15 +++ > > > > > drivers/greybus/Makefile | 48 ++++++++ > > > > > drivers/greybus/gb_uart_node.rs | 245 ++++++++++++++++++++++++++++++++++++++++ > > > > > 5 files changed, 310 insertions(+) > > > > > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > > > index d047090be5f4..49c6dac72748 100644 > > > > > --- a/MAINTAINERS > > > > > +++ b/MAINTAINERS > > > > > @@ -11339,6 +11339,7 @@ M: Ayush Singh > > > > > L: greybus-dev@lists.linaro.org (moderated for non-subscribers) > > > > > S: Maintained > > > > > F: Documentation/devicetree/bindings/beagle/beagle,beagleconnect-freedom.yaml > > > > > +F: drivers/greybus/gb_uart_node.rs > > > > > > > > > > GREYBUS SUBSYSTEM > > > > > M: Johan Hovold > > > > > diff --git a/drivers/greybus/.gitignore b/drivers/greybus/.gitignore > > > > > new file mode 100644 > > > > > index 000000000000..ff9c4a3539b4 > > > > > --- /dev/null > > > > > +++ b/drivers/greybus/.gitignore > > > > > @@ -0,0 +1 @@ > > > > > +exports_gb_softsvc_generated.h > > > > > diff --git a/drivers/greybus/Kconfig b/drivers/greybus/Kconfig > > > > > index 381d1a6ee135..34de913af287 100644 > > > > > --- a/drivers/greybus/Kconfig > > > > > +++ b/drivers/greybus/Kconfig > > > > > @@ -60,5 +60,20 @@ config GREYBUS_SOFTSVC > > > > > To compile this code as a module, choose M here: the module > > > > > will be called gb-softsvc.ko > > > > > > > > > > +config GREYBUS_UART_NODE > > > > > + tristate "Greybus UART node transport" > > > > > + depends on RUST > > > > > + depends on GREYBUS_SOFTSVC > > > > > + depends on RUST_SERIAL_DEV_BUS_ABSTRACTIONS > > > > > + select RUST_CRC_CCITT_ABSTRACTIONS > > > > > + help > > > > > + Select this option if you have a Greybus node connected over a > > > > > + serial port. The node is registered with the software SVC, which > > > > > + handles the SVC protocol on behalf of the AP, so no dedicated > > > > > + coprocessor running SVC firmware is required. > > > > > + > > > > > + To compile this code as a module, choose M here: the module > > > > > + will be called gb-uart-node.ko > > > > > + > > > > > endif # GREYBUS > > > > > > > > > > diff --git a/drivers/greybus/Makefile b/drivers/greybus/Makefile > > > > > index e6f594128802..81151963c01e 100644 > > > > > --- a/drivers/greybus/Makefile > > > > > +++ b/drivers/greybus/Makefile > > > > > @@ -28,3 +28,51 @@ obj-$(CONFIG_GREYBUS_ES2) += gb-es2.o > > > > > obj-$(CONFIG_GREYBUS_SOFTSVC) += gb-softsvc.o > > > > > gb-softsvc-y += gb_softsvc.o gb_softsvc_exports.o > > > > > > > > > > +obj-$(CONFIG_GREYBUS_UART_NODE) += gb-uart-node.o > > > > > +gb-uart-node-y += gb_uart_node.o > > > > > + > > > > > +# Export Rust symbols from gb-softsvc only if gb-uart-node actually references them. > > > > > +gb-softsvc-export-deps := $(if $(CONFIG_GREYBUS_UART_NODE),$(obj)/gb_uart_node.o) > > > > > + > > > > > +rust_needed_exports = \ > > > > > + { $(if $(strip $(2)),$(NM) -u $(2);,) echo "__DEFINED_RUST_SYMBOLS__"; \ > > > > > + $(NM) -p --defined-only $(1); } | \ > > > > > + awk -v fmt='$(3)' ' \ > > > > > + /^__DEFINED_RUST_SYMBOLS__$$/ { defs = 1; next } \ > > > > > + !defs { if ($$NF ~ /^_R/) needed[$$NF] = 1; next } \ > > > > > + defs && $$2 ~ /(T|R|D|B)/ && $$3 ~ /^_R/ && \ > > > > > + $$3 !~ /_(init|cleanup)_module$$/ && \ > > > > > + $$3 !~ /__(pfx|cfi|odr_asan)/ && \ > > > > > + $$3 in needed { printf fmt, $$3 } \ > > > > > + ' > > > > > + > > > > > +quiet_cmd_exports = EXPORTS $@ > > > > > + cmd_exports = \ > > > > > + $(call rust_needed_exports,$<,$(gb-softsvc-export-deps),EXPORT_SYMBOL_RUST_GPL(%s);\n) > $@ > > > > > + > > > > > +$(obj)/exports_gb_softsvc_generated.h: $(obj)/gb_softsvc.o $(gb-softsvc-export-deps) FORCE > > > > > + $(call if_changed,exports) > > > > > + > > > > > +targets += exports_gb_softsvc_generated.h > > > > > + > > > > > +$(obj)/gb_softsvc_exports.o: $(obj)/exports_gb_softsvc_generated.h > > > > > +CFLAGS_gb_softsvc_exports.o := -I $(objtree)/$(obj) > > > > > + > > > > > +ifdef CONFIG_MODVERSIONS > > > > > +# The C export shim declares Rust symbols as `extern int`, so reuse its export > > > > > +# list but generate symbol CRCs from the Rust object instead of the shim's DWARF. > > > > > +$(obj)/gb_softsvc_exports.o: private cmd_gensymtypes_c = \ > > > > > + $(call getexportsymbols,\1) | \ > > > > > + $(objtree)/scripts/gendwarfksyms/gendwarfksyms \ > > > > > + $(if $(KBUILD_GENDWARFKSYMS_STABLE), --stable) \ > > > > > + $(if $(KBUILD_SYMTYPES), --symtypes $(@:.o=.symtypes),) \ > > > > > + $(obj)/gb_softsvc.o > > > > > +endif > > > > > + > > > > > +# Output nova-core's crate metadata for use by nova-drm at compile time. > > > > > +RUSTFLAGS_gb_softsvc.o += \ > > > > > + --emit=metadata=$(objtree)/$(obj)/libgb_softsvc.rmeta > > > > > + > > > > > +# Allow nova-drm to import nova-core's types. > > > > > +$(obj)/gb_uart_node.o: $(obj)/gb_softsvc.o > > > > > +RUSTFLAGS_gb_uart_node.o := -L $(objtree)/$(obj) --extern gb_softsvc > > > > > diff --git a/drivers/greybus/gb_uart_node.rs b/drivers/greybus/gb_uart_node.rs > > > > > new file mode 100644 > > > > > index 000000000000..3eb4f8ab3655 > > > > > --- /dev/null > > > > > +++ b/drivers/greybus/gb_uart_node.rs > > > > > @@ -0,0 +1,245 @@ > > > > > +// SPDX-License-Identifier: GPL-2.0 > > > > > + > > > > > +//! Greybus UART Node driver > > > > > + > > > > > +use kernel::{ > > > > > + alloc::Flags, > > > > > + crc_ccitt::crc_ccitt, > > > > > + device::{ > > > > > + AsBusDevice, > > > > > + Bound, > > > > > + Core, // > > > > > + }, > > > > > + error::code, > > > > > + new_spinlock, of, > > > > > + prelude::*, > > > > > + serdev, > > > > > + sync::{ > > > > > + aref::ARef, > > > > > + Arc, > > > > > + SpinLock, // > > > > > + }, > > > > > +}; > > > > > + > > > > > +use zerocopy::little_endian; > > > > > +use zerocopy_derive::{FromBytes, Immutable, KnownLayout}; > > > > > + > > > > > +const HDLC_MAX_FRAME_LEN: usize = 256; > > > > > + > > > > > +const HDLC_FRAME: u8 = 0x7E; > > > > > +const HDLC_ESC: u8 = 0x7D; > > > > > +const HDLC_XOR: u8 = 0x20; > > > > > +const HDLC_EXPECTED_CRC: u16 = 0xf0b8; > > > > > + > > > > > +const ADDRESS_GREYBUS: u8 = 0x01; > > > > > + > > > > > +#[repr(C, packed)] > > > > > +#[derive(FromBytes, Immutable, KnownLayout)] > > > > > +struct GreybusFrame { > > > > > + cport: little_endian::U16, > > > > > + msg: [u8], > > > > > +} > > > > > + > > > > > +struct HdlcRx { > > > > > + rx_buf: KVec, > > > > > + rx_in_esc: bool, > > > > > + sdev: ARef, > > > > > + node: gb_softsvc::Module, > > > > > +} > > > > > + > > > > > +impl HdlcRx { > > > > > + fn new(sdev: ARef, node: gb_softsvc::Module) -> Result { > > > > > + Ok(Self { > > > > > + node, > > > > > + sdev, > > > > > + rx_buf: KVec::with_capacity(HDLC_MAX_FRAME_LEN, GFP_KERNEL)?, > > > > > + rx_in_esc: false, > > > > > + }) > > > > > + } > > > > > + > > > > > + fn frame_finish(&self) -> Result<()> { > > > > > + if self.rx_buf.len() < 4 { > > > > > + return Err(code::EFAULT); > > > > > + } > > > > > + > > > > > + let crc = crc_ccitt(0xffff, &self.rx_buf); > > > > > + if crc != HDLC_EXPECTED_CRC { > > > > > + dev_warn!(self.sdev.as_ref(), "CRC failed {}", crc); > > > > > + return Ok(()); > > > > > + } > > > > > + > > > > > + let addr = self.rx_buf[0]; > > > > > + let _ctrl = self.rx_buf[1]; > > > > > + let payload = &self.rx_buf[2..self.rx_buf.len() - size_of::()]; > > > > > + > > > > > + match addr { > > > > > + ADDRESS_GREYBUS => { > > > > > + let frame = GreybusFrame::ref_from_bytes(payload).map_err(|_| code::EINVAL)?; > > > > > + self.node.submit_message(0, frame.cport.into(), &frame.msg) > > > > > + } > > > > > + _ => Err(code::EINVAL), > > > > > + } > > > > > + } > > > > > + > > > > > + fn rx(&mut self, data: &[u8]) -> usize { > > > > > + for i in data.iter() { > > > > > + match *i { > > > > > + HDLC_FRAME => { > > > > > + if !self.rx_buf.is_empty() { > > > > > + if let Err(e) = self.frame_finish() { > > > > > + dev_warn!(self.sdev.as_ref(), "bad frame: {e:?}\n"); > > > > > + } > > > > > + } > > > > > + > > > > > + self.rx_buf.clear(); > > > > > + self.rx_in_esc = false; > > > > > + } > > > > > + HDLC_ESC => self.rx_in_esc = true, > > > > > + _ => { > > > > > + let c = if self.rx_in_esc { *i ^ HDLC_XOR } else { *i }; > > > > > + self.rx_in_esc = false; > > > > > + > > > > > + if self.rx_buf.push_within_capacity(c).is_err() { > > > > > + dev_warn!(self.sdev.as_ref(), "buffer overflow. Dropping frame"); > > > > > + > > > > > + self.rx_buf.clear(); > > > > > + self.rx_in_esc = false; > > > > > + } > > > > > + } > > > > > + } > > > > > + } > > > > > + > > > > > + data.len() > > > > > + } > > > > > +} > > > > > + > > > > > +struct GbNode { > > > > > + sdev: ARef, > > > > > +} > > > > > + > > > > > +impl GbNode { > > > > > + const fn new(sdev: ARef) -> Self { > > > > > + Self { sdev } > > > > > + } > > > > > + > > > > > + fn fill_buf(mut crc: u16, data: &[u8], buf: &mut KVec) -> Result { > > > > > + for i in data { > > > > > + crc = crc_ccitt(crc, &[*i]); > > > > > + if *i == HDLC_ESC || *i == HDLC_FRAME { > > > > > + buf.push_within_capacity(HDLC_ESC)?; > > > > > + buf.push_within_capacity(i ^ HDLC_XOR)?; > > > > > + } else { > > > > > + buf.push_within_capacity(*i)?; > > > > > + } > > > > > + } > > > > > + > > > > > + Ok(crc) > > > > > + } > > > > > +} > > > > > + > > > > > +impl gb_softsvc::InterfaceOps for GbNode { > > > > > + fn write(&self, data: &[u8], cport: u16, gfp_mask: Flags) -> Result<()> { > > > > > + // SAFETY: `GbNode` only exists while its serdev driver is bound, so the device is in the > > > > > + // `Bound` state for the duration of this call. > > > > > + let bound: &serdev::Device = > > > > > + unsafe { serdev::Device::from_device(self.sdev.as_ref().as_bound()) }; > > > > > + > > > > > + let mut buf = KVec::with_capacity(HDLC_MAX_FRAME_LEN, gfp_mask)?; > > > > > + > > > > > + let mut crc = 0xffff; > > > > > + > > > > > + buf.push_within_capacity(HDLC_FRAME)?; > > > > > + > > > > > + crc = Self::fill_buf(crc, &[ADDRESS_GREYBUS, 0x03], &mut buf)?; > > > > > + crc = Self::fill_buf(crc, &cport.to_le_bytes(), &mut buf)?; > > > > > + crc = Self::fill_buf(crc, data, &mut buf)?; > > > > > + > > > > > + crc ^= 0xffff; > > > > > + Self::fill_buf(crc, &crc.to_le_bytes(), &mut buf)?; > > > > > + > > > > > + buf.push_within_capacity(HDLC_FRAME)?; > > > > > + > > > > > + bound.write_all(&buf, 0)?; > > > > > + > > > > > + Ok(()) > > > > > + } > > > > > +} > > > > > + > > > > > +#[pin_data] > > > > > +struct GbUartNode { > > > > > + #[pin] > > > > > + rx: SpinLock>, > > > > (add me to CC please) > > > > > > > > Instead of using a lock here, it might be a better idea to > > > > synchronize/stop the receive callback before unbind is called in the > > > > serdev rust abstraction. This would allow the abstraction to provide > > > > mutable references to the driver data in `receive` and `unbind`. It > > > > would also remove the Sync requirement. > > > > > > > > I will send a patch soon. > > > > > > > > Thanks > > > > - Markus Probst > > > > > > That sounds great. The lock here was basically only for getting a mut > > > ref. I will base the next version on top of your patches. > > > > > > I have added your email for the next patch version. > > It appears that "would break the driver core's lifetime design", so I > > have to drop the patch. > > > > You will still need the SpinLock. > > You can still do sychronization to allow this, just not using things from > drvdata. For example, if you move this "active: Mutex" to be part of > `serdev_device`, then you don't need to access `drvdata` to set the active > field. > > That'll be actually more inline with what other buses are doing, where drvdata > is exclusively for the drivers, and for the bus. I am not sure what the active mutex has to do with. In fact, the `active` mutex will be removed anyway [1]. The patch [2] would provide mutable references to callbacks. According to Danilo, "The whole design is based on the guarantee that we never construct a mutable reference of the bus device private data." [3] (which the patch would violate). Thanks - Markus Probst [1] https://lore.kernel.org/rust-for-linux/20260906-rust_serdev_probe_refactor-v1-2-69cdae0074ec@posteo.de/ [2] https://lore.kernel.org/rust-for-linux/20260906-rust_serdev_probe_refactor-v1-5-69cdae0074ec@posteo.de/ [3] https://lore.kernel.org/rust-for-linux/DL8DCNMSENDS.3T14U5W1Y32IX@kernel.org/ > > Best, > Gary