[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9affdae9-9e95-1f6c-5f18-845d5ffcbd71@gmail.com>
Date: Fri, 13 Oct 2023 17:11:23 +0530
From: Ayush Singh <ayushdevel1325@...il.com>
To: greybus-dev@...ts.linaro.org
Cc: devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
gregkh@...uxfoundation.org, vaishnav@...gleboard.org,
jkridner@...gleboard.org, nm@...com,
krzysztof.kozlowski+dt@...aro.org, johan@...nel.org,
elder@...nel.org
Subject: Re: [PATCH v8 2/3] greybus: Add BeaglePlay Linux Driver
Hello everyone, I would like to get some feedback on the driver patch
before submitting the new version of this patch series.
> Add the Greybus host driver for BeaglePlay board by BeagleBoard.org.
>
> The current greybus setup involves running SVC in a user-space
> application (GBridge) and using netlink to communicate with kernel
> space. GBridge itself uses wpanusb kernel driver, so the greybus messages
> travel from kernel space (gb_netlink) to user-space (GBridge) and then
> back to kernel space (wpanusb) before reaching CC1352.
>
> This driver directly communicates with CC1352 (running SVC Zephyr
> application). Thus, it simplifies the complete greybus setup eliminating
> user-space GBridge.
>
> This driver is responsible for the following:
> - Start SVC (CC1352) on driver load.
> - Send/Receive Greybus messages to/from CC1352 using HDLC over UART.
> - Print Logs from CC1352.
> - Stop SVC (CC1352) on driver load.
>
> Signed-off-by: Ayush Singh <ayushdevel1325@...il.com>
> ---
> MAINTAINERS | 1 +
> drivers/greybus/Kconfig | 10 +
> drivers/greybus/Makefile | 2 +
> drivers/greybus/gb-beagleplay.c | 501 ++++++++++++++++++++++++++++++++
> 4 files changed, 514 insertions(+)
> create mode 100644 drivers/greybus/gb-beagleplay.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5467669d7963..d87e30626a6a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8974,6 +8974,7 @@ M: Ayush Singh <ayushdevel1325@...il.com>
> L: greybus-dev@...ts.linaro.org (moderated for non-subscribers)
> S: Maintained
> F: Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
> +F: drivers/greybus/gb-beagleplay.c
>
> GREYBUS SUBSYSTEM
> M: Johan Hovold <johan@...nel.org>
> diff --git a/drivers/greybus/Kconfig b/drivers/greybus/Kconfig
> index 78ba3c3083d5..033d31dbf3b8 100644
> --- a/drivers/greybus/Kconfig
> +++ b/drivers/greybus/Kconfig
> @@ -17,6 +17,16 @@ menuconfig GREYBUS
>
> if GREYBUS
>
> +config GREYBUS_BEAGLEPLAY
> + tristate "Greybus BeaglePlay driver"
> + depends on SERIAL_DEV_BUS
> + help
> + Select this option if you have a BeaglePlay where CC1352
> + co-processor acts as Greybus SVC.
> +
> + To compile this code as a module, chose M here: the module
> + will be called gb-beagleplay.ko
> +
> config GREYBUS_ES2
> tristate "Greybus ES3 USB host controller"
> depends on USB
> diff --git a/drivers/greybus/Makefile b/drivers/greybus/Makefile
> index 9bccdd229aa2..d986e94f8897 100644
> --- a/drivers/greybus/Makefile
> +++ b/drivers/greybus/Makefile
> @@ -18,6 +18,8 @@ obj-$(CONFIG_GREYBUS) += greybus.o
> # needed for trace events
> ccflags-y += -I$(src)
>
> +obj-$(CONFIG_GREYBUS_BEAGLEPLAY) += gb-beagleplay.o
> +
> # Greybus Host controller drivers
> gb-es2-y := es2.o
>
> diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c
> new file mode 100644
> index 000000000000..43318c1993ba
> --- /dev/null
> +++ b/drivers/greybus/gb-beagleplay.c
> @@ -0,0 +1,501 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Beagleplay Linux Driver for Greybus
> + *
> + * Copyright (c) 2023 Ayush Singh <ayushdevel1325@...il.com>
> + * Copyright (c) 2023 BeagleBoard.org Foundation
> + */
> +
> +#include <linux/gfp.h>
> +#include <linux/greybus.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/printk.h>
> +#include <linux/serdev.h>
> +#include <linux/tty.h>
> +#include <linux/tty_driver.h>
> +#include <linux/greybus/hd.h>
> +#include <linux/init.h>
> +#include <linux/device.h>
> +#include <linux/crc-ccitt.h>
> +#include <linux/circ_buf.h>
> +#include <linux/types.h>
> +#include <linux/workqueue.h>
> +
> +#define RX_HDLC_PAYLOAD 256
> +#define CRC_LEN 2
> +#define MAX_RX_HDLC (1 + RX_HDLC_PAYLOAD + CRC_LEN)
> +#define TX_CIRC_BUF_SIZE 1024
> +
> +#define ADDRESS_GREYBUS 0x01
> +#define ADDRESS_DBG 0x02
> +#define ADDRESS_CONTROL 0x03
> +
> +#define HDLC_FRAME 0x7E
> +#define HDLC_ESC 0x7D
> +#define HDLC_XOR 0x20
> +
> +#define CONTROL_SVC_START 0x01
> +#define CONTROL_SVC_STOP 0x02
> +
> +/* The maximum number of CPorts supported by Greybus Host Device */
> +#define GB_MAX_CPORTS 32
> +
> +/**
> + * struct gb_beagleplay - BeaglePlay Greybus driver
> + *
> + * @sd: underlying serdev device
> + *
> + * @gb_hd: greybus host device
> + *
> + * @tx_work: hdlc transmit work
> + * @tx_producer_lock: hdlc transmit data producer lock. acquired when appending data to buffer.
> + * @tx_consumer_lock: hdlc transmit data consumer lock. acquired when sending data over uart.
> + * @tx_circ_buf: hdlc transmit circular buffer.
> + * @tx_crc: hdlc transmit crc-ccitt fcs
> + *
> + * @rx_buffer_len: length of receive buffer filled.
> + * @rx_buffer: hdlc frame receive buffer
> + * @rx_in_esc: hdlc rx flag to indicate ESC frame
> + */
> +struct gb_beagleplay {
> + struct serdev_device *sd;
> +
> + struct gb_host_device *gb_hd;
> +
> + struct work_struct tx_work;
> + spinlock_t tx_producer_lock;
> + spinlock_t tx_consumer_lock;
> + struct circ_buf tx_circ_buf;
> + u16 tx_crc;
> +
> + u16 rx_buffer_len;
> + bool rx_in_esc;
> + u8 rx_buffer[MAX_RX_HDLC];
> +};
> +
> +/**
> + * struct hdlc_payload - Structure to represent part of HDCL frame payload data.
> + *
> + * @len: buffer length in bytes
> + * @buf: payload buffer
> + */
> +struct hdlc_payload {
> + u16 len;
> + void *buf;
> +};
> +
> +static void hdlc_rx_greybus_frame(struct gb_beagleplay *bg, u8 *buf, u16 len)
> +{
> + u16 cport_id;
> + struct gb_operation_msg_hdr *hdr = (struct gb_operation_msg_hdr *)buf;
> +
> + memcpy(&cport_id, hdr->pad, sizeof(cport_id));
> +
> + dev_dbg(&bg->sd->dev, "Greybus Operation %u type %X cport %u status %u received",
> + hdr->operation_id, hdr->type, cport_id, hdr->result);
> +
> + greybus_data_rcvd(bg->gb_hd, cport_id, buf, len);
> +}
> +
> +static void hdlc_rx_dbg_frame(const struct gb_beagleplay *bg, const char *buf, u16 len)
> +{
> + dev_dbg(&bg->sd->dev, "CC1352 Log: %.*s", (int)len, buf);
> +}
> +
> +/**
> + * hdlc_write() - Consume HDLC Buffer.
> + * @bg: beagleplay greybus driver
> + *
> + * Assumes that consumer lock has been acquired.
> + */
> +static void hdlc_write(struct gb_beagleplay *bg)
> +{
> + int written;
> + /* Start consuming HDLC data */
> + int head = smp_load_acquire(&bg->tx_circ_buf.head);
> + int tail = bg->tx_circ_buf.tail;
> + int count = CIRC_CNT_TO_END(head, tail, TX_CIRC_BUF_SIZE);
> + const unsigned char *buf = &bg->tx_circ_buf.buf[tail];
> +
> + if (count > 0) {
> + written = serdev_device_write_buf(bg->sd, buf, count);
> +
> + /* Finish consuming HDLC data */
> + smp_store_release(&bg->tx_circ_buf.tail, (tail + written) & (TX_CIRC_BUF_SIZE - 1));
> + }
> +}
> +
> +/**
> + * hdlc_append() - Queue HDLC data for sending.
> + * @bg: beagleplay greybus driver
> + * @value: hdlc byte to transmit
> + *
> + * Assumes that producer lock as been acquired.
> + */
> +static void hdlc_append(struct gb_beagleplay *bg, u8 value)
> +{
> + int tail, head = bg->tx_circ_buf.head;
> +
> + while (true) {
> + tail = READ_ONCE(bg->tx_circ_buf.tail);
> +
> + if (CIRC_SPACE(head, tail, TX_CIRC_BUF_SIZE) >= 1) {
> + bg->tx_circ_buf.buf[head] = value;
> +
> + /* Finish producing HDLC byte */
> + smp_store_release(&bg->tx_circ_buf.head,
> + (head + 1) & (TX_CIRC_BUF_SIZE - 1));
> + return;
> + }
> + dev_warn(&bg->sd->dev, "Tx circ buf full");
> + usleep_range(3000, 5000);
> + }
> +}
> +
> +static void hdlc_append_escaped(struct gb_beagleplay *bg, u8 value)
> +{
> + if (value == HDLC_FRAME || value == HDLC_ESC) {
> + hdlc_append(bg, HDLC_ESC);
> + value ^= HDLC_XOR;
> + }
> + hdlc_append(bg, value);
> +}
> +
> +static void hdlc_append_tx_frame(struct gb_beagleplay *bg)
> +{
> + bg->tx_crc = 0xFFFF;
> + hdlc_append(bg, HDLC_FRAME);
> +}
> +
> +static void hdlc_append_tx_u8(struct gb_beagleplay *bg, u8 value)
> +{
> + bg->tx_crc = crc_ccitt(bg->tx_crc, &value, 1);
> + hdlc_append_escaped(bg, value);
> +}
> +
> +static void hdlc_append_tx_buf(struct gb_beagleplay *bg, const u8 *buf, u16 len)
> +{
> + size_t i;
> +
> + for (i = 0; i < len; i++)
> + hdlc_append_tx_u8(bg, buf[i]);
> +}
> +
> +static void hdlc_append_tx_crc(struct gb_beagleplay *bg)
> +{
> + bg->tx_crc ^= 0xffff;
> + hdlc_append_escaped(bg, bg->tx_crc & 0xff);
> + hdlc_append_escaped(bg, (bg->tx_crc >> 8) & 0xff);
> +}
> +
> +static void hdlc_transmit(struct work_struct *work)
> +{
> + struct gb_beagleplay *bg = container_of(work, struct gb_beagleplay, tx_work);
> +
> + spin_lock_bh(&bg->tx_consumer_lock);
> + hdlc_write(bg);
> + spin_unlock_bh(&bg->tx_consumer_lock);
> +}
> +
> +static void hdlc_tx_frames(struct gb_beagleplay *bg, u8 address, u8 control,
> + const struct hdlc_payload payloads[], size_t count)
> +{
> + size_t i;
> +
> + spin_lock(&bg->tx_producer_lock);
> +
> + hdlc_append_tx_frame(bg);
> + hdlc_append_tx_u8(bg, address);
> + hdlc_append_tx_u8(bg, control);
> +
> + for (i = 0; i < count; ++i)
> + hdlc_append_tx_buf(bg, payloads[i].buf, payloads[i].len);
> +
> + hdlc_append_tx_crc(bg);
> + hdlc_append_tx_frame(bg);
> +
> + spin_unlock(&bg->tx_producer_lock);
> +
> + schedule_work(&bg->tx_work);
> +}
> +
> +static void hdlc_tx_s_frame_ack(struct gb_beagleplay *bg)
> +{
> + hdlc_tx_frames(bg, bg->rx_buffer[0], (bg->rx_buffer[1] >> 1) & 0x7, NULL, 0);
> +}
> +
> +static void hdlc_rx_frame(struct gb_beagleplay *bg)
> +{
> + u16 crc, len;
> + u8 ctrl, *buf;
> + u8 address = bg->rx_buffer[0];
> +
> + crc = crc_ccitt(0xffff, bg->rx_buffer, bg->rx_buffer_len);
> + if (crc != 0xf0b8) {
> + dev_warn_ratelimited(&bg->sd->dev, "CRC failed from %02x: 0x%04x", address, crc);
> + return;
> + }
> +
> + ctrl = bg->rx_buffer[1];
> + buf = &bg->rx_buffer[2];
> + len = bg->rx_buffer_len - 4;
> +
> + /* I-Frame, send S-Frame ACK */
> + if ((ctrl & 1) == 0)
> + hdlc_tx_s_frame_ack(bg);
> +
> + switch (address) {
> + case ADDRESS_DBG:
> + hdlc_rx_dbg_frame(bg, buf, len);
> + break;
> + case ADDRESS_GREYBUS:
> + hdlc_rx_greybus_frame(bg, buf, len);
> + break;
> + default:
> + dev_warn_ratelimited(&bg->sd->dev, "unknown frame %u", address);
> + }
> +}
> +
> +static int hdlc_rx(struct gb_beagleplay *bg, const u8 *data, size_t count)
> +{
> + size_t i;
> + u8 c;
> +
> + for (i = 0; i < count; ++i) {
> + c = data[i];
> +
> + switch (c) {
> + case HDLC_FRAME:
> + if (bg->rx_buffer_len)
> + hdlc_rx_frame(bg);
> +
> + bg->rx_buffer_len = 0;
> + break;
> + case HDLC_ESC:
> + bg->rx_in_esc = true;
> + break;
> + default:
> + if (bg->rx_in_esc) {
> + c ^= 0x20;
> + bg->rx_in_esc = false;
> + }
> +
> + if (bg->rx_buffer_len < MAX_RX_HDLC) {
> + bg->rx_buffer[bg->rx_buffer_len] = c;
> + bg->rx_buffer_len++;
> + } else {
> + dev_err_ratelimited(&bg->sd->dev, "RX Buffer Overflow");
> + bg->rx_buffer_len = 0;
> + }
> + }
> + }
> +
> + return count;
> +}
> +
> +static int hdlc_init(struct gb_beagleplay *bg)
> +{
> + INIT_WORK(&bg->tx_work, hdlc_transmit);
> + spin_lock_init(&bg->tx_producer_lock);
> + spin_lock_init(&bg->tx_consumer_lock);
> + bg->tx_circ_buf.head = 0;
> + bg->tx_circ_buf.tail = 0;
> +
> + bg->tx_circ_buf.buf = devm_kmalloc(&bg->sd->dev, TX_CIRC_BUF_SIZE, GFP_KERNEL);
> + if (!bg->tx_circ_buf.buf)
> + return -ENOMEM;
> +
> + bg->rx_buffer_len = 0;
> + bg->rx_in_esc = false;
> +
> + return 0;
> +}
> +
> +static void hdlc_deinit(struct gb_beagleplay *bg)
> +{
> + flush_work(&bg->tx_work);
> +}
> +
> +static int gb_tty_receive(struct serdev_device *sd, const unsigned char *data, size_t count)
> +{
> + struct gb_beagleplay *bg = serdev_device_get_drvdata(sd);
> +
> + return hdlc_rx(bg, data, count);
> +}
> +
> +static void gb_tty_wakeup(struct serdev_device *serdev)
> +{
> + struct gb_beagleplay *bg = serdev_device_get_drvdata(serdev);
> +
> + schedule_work(&bg->tx_work);
> +}
> +
> +static struct serdev_device_ops gb_beagleplay_ops = {
> + .receive_buf = gb_tty_receive,
> + .write_wakeup = gb_tty_wakeup,
> +};
> +
> +static int gb_message_send(struct gb_host_device *hd, u16 cport, struct gb_message *msg, gfp_t mask)
> +{
> + struct gb_beagleplay *bg = dev_get_drvdata(&hd->dev);
> + struct hdlc_payload payloads[2];
> +
> + dev_dbg(&hd->dev, "Sending greybus message with Operation %u, Type: %X on Cport %u",
> + msg->header->operation_id, msg->header->type, cport);
> +
> + if (msg->header->size > RX_HDLC_PAYLOAD)
> + return dev_err_probe(&hd->dev, -E2BIG, "Greybus message too big");
> +
> + memcpy(msg->header->pad, &cport, sizeof(cport));
> +
> + payloads[0].buf = msg->header;
> + payloads[0].len = sizeof(*msg->header);
> + payloads[1].buf = msg->payload;
> + payloads[1].len = msg->payload_size;
> +
> + hdlc_tx_frames(bg, ADDRESS_GREYBUS, 0x03, payloads, 2);
> + greybus_message_sent(bg->gb_hd, msg, 0);
> +
> + return 0;
> +}
> +
> +static void gb_message_cancel(struct gb_message *message)
> +{
> +}
> +
I am not quite sure how to deal with `message_cancel`. The current
greybus message follow the following trajectory: Linux driver --UART-->
CC1352 Firmware --6lowpan--> greybus node.
The main method I can currently think to implement this in the Linux
driver. I can maintain an array containing a canceled greybus message ID
and a timestamp of when it was added. There are two ways to have a
message removed from this array:
1. A canceled message is received: Just remove the message from array
and drop the received message.
2. We have a GC kind of thing which cleans up array members if a set
timeout has passed since timestamp.
Or maybe I am wrong and the expectation of `message_callback` is
different from what I tried to solve above?
Sincerely
Ayush Singh
Powered by blists - more mailing lists