Re: [PATCH 3/5] mfd: Add Hexagon EFI driver

Marco Felsch
2026-02-09T11:13:46Z

On 26-02-09, Sascha Hauer wrote:
> On Thu, Feb 05, 2026 at 04:45:05PM +0100, Marco Felsch wrote:
> > This adds the EFI core driver to communicate with the system
> > co-processor.
>
> This EFI doesn't have anything to do with the Extensible Firmware
> Interface, right? Could you add some note what it means to commit
> message, Kconfig and to the description in hgs-efi.c to avoid confusion?

Sure!

> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > drivers/mfd/Kconfig | 8 +
> > drivers/mfd/Makefile | 1 +
> > drivers/mfd/hgs-efi.c | 473 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > include/mfd/hgs-efi.h | 46 +++++
> > 4 files changed, 528 insertions(+)
> >
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index 471f85cd63dad30b39017f694b594b768e463a15..f09e3903bcb50556b1556a84500ae73d91a5a63d 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -158,4 +158,12 @@ config MFD_ATMEL_SMC
> > bool
> > select MFD_SYSCON
> >
> > +config MFD_HGS_EFI
> > + tristate "Hexagon Geosystems EFI core driver"
> > + depends on SERIAL_DEV_BUS
> > + select CRC16
> > + help
> > + Select this to get support for the EFI Co-Processor
> > + device found on several devices in the System1600 platform.
> > +
> > endmenu
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 88480f70640d464e0e12241ec422a63ca860330d..9d27b0bc336a908f1245eb7f1d257a23e271ebd7 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -27,3 +27,4 @@ obj-$(CONFIG_MFD_ATMEL_SMC) += atmel-smc.o
> > obj-$(CONFIG_MFD_ROHM_BD718XX) += rohm-bd718x7.o
> > obj-$(CONFIG_MFD_PCA9450) += pca9450.o
> > obj-$(CONFIG_MFD_TPS65219) += tps65219.o
> > +obj-$(CONFIG_MFD_HGS_EFI) += hgs-efi.o
> > diff --git a/drivers/mfd/hgs-efi.c b/drivers/mfd/hgs-efi.c
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..e548a6b209a6f64cecd8d56edcaa565b0b6d2657
> > --- /dev/null
> > +++ b/drivers/mfd/hgs-efi.c
> > @@ -0,0 +1,473 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +// SPDX-FileCopyrightText: 2025 Pengutronix
> > +
> > +/*
> > + * Multifunction core driver for Hexagon Geosystems EFI MCU that is connected
> > + * via dedicated UART port. The communication protocol between both parties is
> > + * called Sensor Protocol (SEP).
> > + *
> > + * Based on drivers/mfd/rave-sp.c
> > + */
> > +
> > +#include <asm/unaligned.h>
> > +#include <common.h>
> > +#include <init.h>
> > +#include <of_device.h>
> > +#include <mfd/hgs-efi.h>
> > +#include <linux/crc16.h>
> > +#include <linux/ctype.h>
> > +#include <linux/string.h>
> > +#include <mfd/hgs-efi.h>
> > +
> > +#define HGS_EFI_SEP_ASCII_SYNCBYTE 'S'
> > +#define HGS_EFI_SEP_ASCII_MSG_TYPE_CMD 'C'
> > +#define HGS_EFI_SEP_ASCII_MSG_TYPE_EVENT 'E'
> > +#define HGS_EFI_SEP_ASCII_MSG_TYPE_REPLY 'R'
> > +#define HGS_EFI_SEP_ASCII_DELIM ','
> > +#define HGS_EFI_SEP_ASCII_HDR_END ':'
> > +
> > +/* Non addressed ascii header format */
> > +struct hgs_efi_sep_ascii_hdr {
> > + u8 syncbyte;
> > + u8 msg_type;
> > + u8 msg_id[5]; /* u16 dec number */
> > + u8 delim;
> > + u8 crc16[4]; /* u16 hex number */
> > + u8 hdr_end;
> > +} __packed;
> > +
> > +#define HGS_EFI_SEP_RX_BUFFER_SIZE 64
> > +#define HGS_EFI_SEP_FRAME_PREAMBLE_SZ 2
> > +#define HGS_EFI_SEP_FRAME_POSTAMBLE_SZ 2
> > +
> > +enum hgs_efi_sep_deframer_state {
> > + HGS_EFI_SEP_EXPECT_SOF,
> > + HGS_EFI_SEP_EXPECT_DATA,
> > +};
> > +
> > +/**
> > + * struct hgs_efi_deframer - Device protocol deframer
> > + *
> > + * @state: Current state of the deframer
> > + * @data: Buffer used to collect deframed data
> > + * @length: Number of bytes de-framed so far
> > + */
> > +struct hgs_efi_deframer {
> > + enum hgs_efi_sep_deframer_state state;
> > + unsigned char data[HGS_EFI_SEP_RX_BUFFER_SIZE];
> > + size_t length;
> > +};
> > +
> > +/**
> > + * struct hgs_efi_reply - Reply as per SEP
> > + *
> > + * @length: Expected reply length
> > + * @data: Buffer to store reply payload in
> > + * @msg_id: Expected SEP msg-id
> > + * @received: Successful reply reception
> > + */
> > +struct hgs_efi_reply {
> > + size_t length;
> > + void *data;
> > + u16 msg_id;
> > + bool received;
> > +};
> > +
> > +struct hgs_efi_sep_coder {
> > + int (*encode)(struct hgs_efi *efi, struct hgs_sep_cmd *cmd, u8 *buf);
> > + int (*process_frame)(struct hgs_efi *efi, void *buf, size_t size);
> > + unsigned int sep_header_hdrsize;
> > + char sep_sof_char;
> > +};
> > +
> > +struct hgs_efi {
> > + struct device dev;
> > + struct serdev_device *serdev;
> > + const struct hgs_efi_sep_coder *coder;
> > + struct hgs_efi_deframer deframer;
> > + struct hgs_efi_reply *reply;
> > +};
> > +
> > +static void hgs_efi_write(struct hgs_efi *efi, const u8 *data, size_t data_size)
> > +{
> > + print_hex_dump_bytes("hgs-efi tx: ", DUMP_PREFIX_NONE, data, data_size);
> > +
> > + /* timeout is ignored, instead polling_window is used */
> > + serdev_device_write(efi->serdev, data, data_size, SECOND);
> > +}
> > +
> > +int hgs_efi_exec(struct hgs_efi *efi, struct hgs_sep_cmd *cmd)
> > +{
> > + struct device *dev = efi->serdev->dev;
> > + struct hgs_efi_reply reply = {
> > + .msg_id = cmd->msg_id,
> > + .data = cmd->reply_data,
> > + .length = cmd->reply_data_size,
> > + .received = false,
> > + };
> > + unsigned int max_msg_len;
> > + u8 *msg, *p;
> > + int ret;
> > +
> > + switch (cmd->type) {
> > + case HGS_SEP_MSG_TYPE_COMMAND:
> > + case HGS_SEP_MSG_TYPE_EVENT:
> > + break;
> > + case HGS_SEP_MSG_TYPE_REPLY:
> > + dev_warn(dev, "MCU initiated communication is not supported yet!\n");
> > + return -EINVAL;
> > + default:
> > + dev_warn(dev, "Unknown EFI msg-type %#x\n", cmd->type);
> > + return -EINVAL;
> > + }
> > +
> > + max_msg_len = HGS_EFI_SEP_FRAME_PREAMBLE_SZ +
> > + HGS_EFI_SEP_FRAME_POSTAMBLE_SZ +
> > + efi->coder->sep_header_hdrsize + cmd->payload_size;
> > + msg = p = xzalloc(max_msg_len);
> > + if (!msg) {
> > + dev_err(dev, "No memory\n");
> > + return -ENOMEM;
> > + }
> > +
> > + /* MCU serial flush preamble */
> > + *p++ = '\r';
> > + *p++ = '\n';
> > +
> > + ret = efi->coder->encode(efi, cmd, p);
> > + if (ret < 0) {
> > + free(msg);
> > + return ret;
> > + }
> > +
> > + p += ret;
> > +
> > + /* SEP postamble */
> > + *p++ = '\r';
> > + *p++ = '\n';
> > +
> > + efi->reply = &reply;
> > + hgs_efi_write(efi, msg, p - msg);
> > +
> > + free(msg);
> > +
> > + if (cmd->type == HGS_SEP_MSG_TYPE_EVENT) {
> > + efi->reply = NULL;
> > + return 0;
> > + }
> > +
> > + /*
> > + * is_timeout will implicitly poll serdev via poller
> > + * infrastructure
> > + */
> > + ret = wait_on_timeout(SECOND, reply.received);
> > + if (ret)
> > + dev_err(dev, "Command timeout\n");
> > +
> > + efi->reply = NULL;
> > +
> > + return ret;
> > +}
> > +
> > +#define HGS_SEP_DOUBLE_QUOTE_SUB_VAL 0x1a
> > +
> > +char *hgs_efi_extract_str_response(u8 *buf)
> > +{
> > + unsigned char *start;
> > + unsigned char *end;
> > + unsigned char *p;
> > + size_t i;
> > +
> > + if (!buf || buf[0] != '"') {
> > + pr_warn("hgs-efi: No start \" char found in string response\n");
>
> Add above the includes:
>
> #define pr_fmt(fmt) "hgs-efi: " fmt
>
> And drp the prefix here.

Will do.

> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + start = &buf[1];
> > + end = strrchr(start, '"');
> > + if (!end) {
> > + pr_warn("hgs-efi: No end \" char found in string response\n");
> > + return ERR_PTR(-EINVAL);
> > + }
> > + *end = '\0';
> > +
> > + /*
> > + * Last step, check for substition val in string reply
> > + * and re-substitute it.
> > + */
> > + p = start;
> > + for (i = 0; i < strlen(start); i++)
> > + if (*p == HGS_SEP_DOUBLE_QUOTE_SUB_VAL)
> > + *p = '"';
> > +
> > + return start;
> > +}
> > +
> > +static int hgs_sep_ascii_encode(struct hgs_efi *efi, struct hgs_sep_cmd *cmd,
> > + u8 *buf)
> > +{
> > + struct device *dev = efi->serdev->dev;
> > + size_t hdr_len;
> > + char msg_type;
> > + char *hdr;
> > +
> > + switch (cmd->type) {
> > + case HGS_SEP_MSG_TYPE_COMMAND:
> > + msg_type = 'C';
> > + break;
> > + case HGS_SEP_MSG_TYPE_EVENT:
> > + msg_type = 'E';
> > + break;
> > + default:
> > + /* Should never happen */
> > + return -EINVAL;
> > + }
> > +
> > + /*
> > + * The ASCII coder doesn't care about the CRC, also the CRC handling
> > + * has a few flaws. Therefore skip it for now.
> > + */
> > + hdr = xasprintf("S%c%u:", msg_type, cmd->msg_id);
>
> xasprintf also doesn't fail.

Thanks

> > + if (!hdr) {
> > + dev_err(dev, "No memory\n");
> > + return -ENOMEM;
> > + }
> > +
> > + /* Now copy the header and the payload to the buffer */
> > + hdr_len = strlen(hdr);
> > + memcpy(buf, hdr, hdr_len);
>
> You could simplify the above by doing:
>
> hdr_len = sprintf(buf, "S%c%u:", msg_type, cmd->msg_id);

Thats a good point, thanks!

>
> > + memcpy(buf + hdr_len, cmd->payload, cmd->payload_size);
> > +
> > + free(hdr);
> > +
> > + return hdr_len + cmd->payload_size;
> > +}
> > +
> > +static int
> > +hgs_sep_process_ascii_frame(struct hgs_efi *efi, void *_buf, size_t size)
> > +{
> > + struct device *dev = efi->serdev->dev;
> > + unsigned char *payload;
> > + unsigned int copy_bytes;
> > + unsigned int msgid;
> > + size_t payload_len;
> > + u8 *buf = _buf;
> > + size_t hdrlen;
> > + char *p;
> > + int ret;
> > +
> > + /*
> > + * Non addressing ASCII format:
> > + * S[MsgType][MsgID](,[CRC]):[Payload]
> > + */
> > + if (buf[1] != HGS_EFI_SEP_ASCII_MSG_TYPE_REPLY) {
>
> check buf[0] != 0 before looking at buf[1]?

I could do this, but this function is only called if buf[0] has the
correct value (see: HGS_EFI_SEP_EXPECT_SOF).

> > + dev_warn(dev, "Invalid msg-type %c(%#x)\n", *buf, *buf);
> > + return -EINVAL;
> > + }
> > +
> > + /* Split header from payload first for the following str-ops on buf */
> > + payload = strstr(buf, ":");
> > + if (!payload) {
> > + dev_warn(dev, "Failed to find header delim\n");
> > + return -EINVAL;
> > + }
> > +
> > + hdrlen = payload - buf;
> > + if (hdrlen > sizeof(struct hgs_efi_sep_ascii_hdr)) {
> > + dev_warn(dev, "Invalid header len detected\n");
> > + return -EINVAL;
> > + }
> > +
> > + *payload = 0;
> > + payload++;
> > +
> > + /*
> > + * Albeit the CRC is optional and the calc as a few flaws the coder may
>
> s/as/has/

thanks.

>
> > + * has added it. Skip the CRC check but do the msg-id check.
> > + */
> > + p = strstr(buf, ",");
> > + if (p)
> > + *p = 0;
> > +
> > + ret = kstrtouint(&buf[2], 10, &msgid);
> > + if (ret) {
> > + dev_warn(dev, "Failed to parse msgid, ret:%d\n", ret);
> > + return -EINVAL;
> > + }
> > +
> > + if (msgid != efi->reply->msg_id) {
> > + dev_warn(dev, "Wrong msg-id received, ignore frame (%u != %u)\n",
> > + msgid, efi->reply->msg_id);
> > + return -EINVAL;
> > + }
> > +
> > + payload_len = size - hdrlen;
> > + copy_bytes = payload_len;
> > + if (payload_len > efi->reply->length) {
> > + dev_warn(dev, "Reply buffer to small, dropping remaining %zu bytes\n",
> > + payload_len - efi->reply->length);
> > + copy_bytes = efi->reply->length;
> > + }
> > +
> > + memcpy(efi->reply->data, payload, copy_bytes);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct hgs_efi_sep_coder hgs_efi_ascii_coder = {
> > + .encode = hgs_sep_ascii_encode,
> > + .process_frame = hgs_sep_process_ascii_frame,
> > + .sep_header_hdrsize = sizeof(struct hgs_efi_sep_ascii_hdr),
> > + .sep_sof_char = HGS_EFI_SEP_ASCII_SYNCBYTE,
> > +};
> > +
> > +static bool hgs_efi_eof_received(struct hgs_efi_deframer *deframer)
> > +{
> > + const char eof_seq[] = { '\r', '\n' };
> > +
> > + if (deframer->length <= 2)
> > + return false;
> > +
> > + if (memcmp(&deframer->data[deframer->length - 2], eof_seq, 2))
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +static void hgs_efi_receive_frame(struct hgs_efi *efi,
> > + struct hgs_efi_deframer *deframer)
> > +{
> > + int ret;
> > +
> > + if (deframer->length < efi->coder->sep_header_hdrsize) {
> > + dev_warn(efi->serdev->dev, "Bad frame: Too short\n");
> > + return;
> > + }
> > +
> > + print_hex_dump_bytes("hgs-efi rx-frame: ", DUMP_PREFIX_NONE,
> > + deframer->data, deframer->length);
> > +
> > + ret = efi->coder->process_frame(efi, deframer->data,
> > + deframer->length - HGS_EFI_SEP_FRAME_PREAMBLE_SZ);
> > + if (!ret)
> > + efi->reply->received = true;
> > +}
> > +
> > +static int hgs_efi_receive_buf(struct serdev_device *serdev,
> > + const unsigned char *buf, size_t size)
> > +{
> > + struct device *dev = serdev->dev;
> > + struct hgs_efi *efi = dev->priv;
> > + struct hgs_efi_deframer *deframer = &efi->deframer;
> > + const unsigned char *src = buf;
> > + const unsigned char *end = buf + size;
> > +
> > + print_hex_dump_bytes("hgs-efi rx-bytes: ", DUMP_PREFIX_NONE, buf, size);
> > +
> > + while (src < end) {
> > + const unsigned char byte = *src++;
> > +
> > + switch (deframer->state) {
> > + case HGS_EFI_SEP_EXPECT_SOF:
> > + if (byte == efi->coder->sep_sof_char)
> > + deframer->state = HGS_EFI_SEP_EXPECT_DATA;
> > + deframer->data[deframer->length++] = byte;
> > + break;
> > + case HGS_EFI_SEP_EXPECT_DATA:
> > + if (deframer->length >= sizeof(deframer->data)) {
> > + dev_warn(dev, "Bad frame: Too long\n");
> > + goto frame_reset;
> > + }
> > +
> > + deframer->data[deframer->length++] = byte;
> > + if (hgs_efi_eof_received(deframer)) {
> > + hgs_efi_receive_frame(efi, deframer);
> > + goto frame_reset;
> > + }
> > + }
> > + }
> > +
> > + /*
> > + * All bytes processed but no EOF detected yet which because the serdev
>
> s/which//
>
> > + * poller may called us to early. Keep the deframer state to continue
> > + * the work where we finished.
> > + */
> > + return size;
> > +
> > +frame_reset:
> > + memset(deframer->data, 0, deframer->length);
> > + deframer->length = 0;
> > + deframer->state = HGS_EFI_SEP_EXPECT_SOF;
> > +
> > + return src - buf;
> > +}
> > +
> > +static int hgs_efi_register_dev(struct hgs_efi *efi)
> > +{
> > + struct device *dev = &efi->dev;
> > +
> > + dev->parent = efi->serdev->dev;
> > + dev_set_name(dev, "%s", "efi");
>
> dev_set_name(dev, "efi");

Sure.

> > + dev->id = DEVICE_ID_SINGLE;
> > +
> > + return register_device(dev);
> > +}
> > +
> > +static int hgs_efi_probe(struct device *dev)
> > +{
> > + struct serdev_device *serdev = to_serdev_device(dev->parent);
> > + struct hgs_efi *efi;
> > + u32 baud;
> > + int ret;
> > +
> > + if (of_property_read_u32(dev->of_node, "current-speed", &baud)) {
> > + dev_err(dev,
> > + "'current-speed' is not specified in device node\n");
> > + return -EINVAL;
> > + }
> > +
> > + efi = xzalloc(sizeof(*efi));
> > + if (!efi)
> > + return -ENOMEM;
> > +
> > + efi->coder = of_device_get_match_data(dev);
> > + if (!efi->coder) {
> > + free(efi);
> > + return -ENODEV;
> > + }
> > +
> > + efi->serdev = serdev;
> > +
> > + dev->priv = efi;
> > + serdev->dev = dev;
> > + serdev->receive_buf = hgs_efi_receive_buf;
> > + serdev->polling_interval = 200 * MSECOND;
> > + serdev->polling_window = 10 * MSECOND;
> > +
> > + ret = serdev_device_open(serdev);
>
> This registers a poller, so when one of the calls below fails this might
> have some bad consequences. Oh, we do not have a serdev_device_close(),
> I won't look further ;)

Hm.. something we should fix. Let me check if I can add a
serdev_device_close() routine real quick else I would add a comment.

Regards,
Marco

>
> Sascha
>
> > + if (ret)
> > + return ret;
> > +
> > + serdev_device_set_baudrate(serdev, baud);
> > +
> > + ret = hgs_efi_register_dev(efi);
> > + if (ret) {
> > + dev_err(dev, "Failed to register EFI device\n");
> > + return ret;
> > + };
> > +
> > + return of_platform_populate(dev->of_node, NULL, dev);
> > +}
> > +
> > +static const struct of_device_id __maybe_unused hgs_efi_dt_ids[] = {
> > + { .compatible = "hgs,efi-gs05", .data = &hgs_efi_ascii_coder },
> > + { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, lgs_efi_dt_ids);
> > +
> > +static struct driver hgs_efi_drv = {
> > + .name = "hgs-efi",
> > + .probe = hgs_efi_probe,
> > + .of_compatible = DRV_OF_COMPAT(hgs_efi_dt_ids),
> > +};
> > +console_platform_driver(hgs_efi_drv);
>
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
>

--
#gernperDu
#CallMeByMyFirstName

Pengutronix e.K. | |
Steuerwalder Str. 21 | https://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |