[greybus-dev] Re: [PATCH v3 7/7] greybus: Add Rust UART node driver

Markus Probst markus.probst@posteo.de writes:

On Tue, 2026-10-13 at 17:30 +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.

Signed-off-by: Ayush Singh ayush@beagleboard.org

MAINTAINERS | 1 + drivers/greybus/.gitignore | 1 + drivers/greybus/Kconfig | 15 +++ drivers/greybus/Makefile | 48 ++++++++ drivers/greybus/gb_uart_node.rs | 253 ++++++++++++++++++++++++++++++++++++++++ 5 files changed, 318 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS index 521bc6c67979..5bdd500ef63e 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11527,6 +11527,7 @@ M: Ayush Singh ayush@beagleboard.org 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 johan@kernel.org 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 709563f8c0ad..ce2030161f30 100644 --- a/drivers/greybus/Kconfig +++ b/drivers/greybus/Kconfig @@ -61,5 +61,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 GREYBUS_SOFTSVC
• depends on RUST_SERIAL_DEV_BUS_ABSTRACTIONS
• depends on RUST_GREYBUS_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..bf4d08f728fe 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 gb_softsvc's crate metadata for use by gb_uart_node 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..f719ee35e8f3 --- /dev/null +++ b/drivers/greybus/gb_uart_node.rs @@ -0,0 +1,253 @@ +// 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<u8>,
• rx_in_esc: bool,
• rx_overflow: bool,
• sdev: ARefserdev::Device,
• node: gb_softsvc::Module,

+}

+impl HdlcRx {

• fn new(sdev: ARefserdev::Device, node: gb_softsvc::Module) -> Result<Self> {

•    Ok(Self {
  

•        node,
  

•        sdev,
  

•        rx_buf: KVec::with_capacity(HDLC_MAX_FRAME_LEN, GFP_KERNEL)?,
  

•        rx_in_esc: false,
  

•        rx_overflow: 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::<u16>()];
  

•    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;
  

•                self.rx_overflow = false;
  

•            }
  

•            HDLC_ESC => self.rx_in_esc = true,
  

•            // Skip data if buffer has overflown.
  

•            _ if self.rx_overflow => {}
  

•            _ => {
  

•                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_overflow = true;
  

•                    self.rx_buf.clear();
  

•                }
  

•            }
  

•        }
  

•    }
  

•    data.len()
  

• }

+}

+struct GbNode {

• sdev: ARefserdev::Device,

+}

+impl GbNode {

• const fn new(sdev: ARefserdev::Device) -> Self {

•    Self { sdev }
  

• }
• fn fill_buf(mut crc: u16, data: &[u8], buf: &mut KVec<u8>) -> Result<u16> {

•    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<Bound> =
  

•        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<Option<HdlcRx>>,

+}

+impl GbUartNode {

• fn init(sdev: &serdev::Device<Core<'_>>) -> Result<HdlcRx> {

•    if sdev
  

•        .set_baudrate(
  

•            sdev.as_ref()
  

•                .fwnode()
  

•                .and_then(|fwnode| fwnode.property_read(c"baudrate").optional())
  

•                .unwrap_or(115200),
  

•        )
  

•        .is_err()
  

•    {
  

•        return Err(EINVAL);
  

•    }
  

•    sdev.set_flow_control(false);
  

•    sdev.set_parity(serdev::Parity::None)?;
  

•    let node = gb_softsvc::Module::new(&[Arc::new(GbNode::new(sdev.into()), GFP_KERNEL)?])?;
  

•    HdlcRx::new(sdev.into(), node)
  

• }

+}

+kernel::of_device_table!(

• OF_TABLE,
• <GbUartNode as serdev::Driver>::IdInfo,
• [(of::DeviceId::new(c"beagle,beagleconnect-freedom"), ())]

+);

+#[vtable] +impl serdev::Driver for GbUartNode {

• type IdInfo = ();
• type Data<'bound> = Self;
• const OF_ID_TABLE: Option<of::IdTableSelf::IdInfo> = Some(&OF_TABLE);
• fn probe<'bound>(

•    sdev: &'bound serdev::Device<Core<'_>>,
  

•    _info: Option<&'bound Self::IdInfo>,
  

• ) -> impl PinInit<Self, Error> + 'bound {

•    dev_dbg!(sdev.as_ref(), "Probe gb_uart_node.\n");
  

•    try_pin_init!(Self {
  

•        rx <- new_spinlock!(Some(Self::init(sdev)?), "gb_uart_node::rx"),
  

•    }? Error)
  

• }
• fn receive<'bound>(

•    _sdev: &'bound serdev::Device<Bound>,
  

•    this: Pin<&Self>,
  

•    data: &[u8],
  

• ) -> usize {

•    if let Some(mut guard) = this.rx.try_lock() {
  

•        if let Some(ref mut hdlc_rx) = *guard {
  

•            return hdlc_rx.rx(data);
  

•        }
  

•    }
  

•    0
  

• }
• fn unbind<'bound>(_: &'bound serdev::Device<Core<'_>>, this: Pin<&Self::Data<'bound>>) {

•    // Getting a bound device is not possible after this point. So drop HdlcRx.
  

•    // This also drops HdlcRx::node, which should ensure that GbNode::write will not be called
  

•    // after this point.
  

•    let _ = this.rx.lock().take();
  

Not exactly.

e.g.

struct SerdevDriver<'bound> {
  dev: &'bound serdev::Device<Bound>,
}

impl<'bound> Drop for SerdevDriver<'bound> {

  fn drop(&mut self) {
    // Access to a bound device should still be possible here.
  }

}

So afaik there should be no advantage of manually dropping it here. Drop of `GbUartNode` should automatically drop the spinlock and the data contained. The bound serial device is still valid at this point.

This would likely make the usage of Option<> obsolete too.

Thanks

• Markus Probst

My understanding was that the state of serdev device is unknown between unbind and Drop calls. That would make the unsafe Bound device conversion in GbNode::write unsound.

If I don't need to do the early drop, yes, that would remove the Option which would be great.

...

• }

+}

+kernel::module_serdev_device_driver! {

• type: GbUartNode,
• name: "gb_uart_node",
• authors: ["Ayush Singh ayush@beagleboard.org"],
• description: "Greybus node connected over UART",
• license: "GPL v2",

+}

Best Regards, Ayush Singh