This driver is for BeaglePlay by BeagleBoard.org. It communicates with BeaglePlay CC1352 co-processor which serves as Greybus SVC [1]. This replaces the old setup with bcfserial, wpanusb and GBridge.
This driver also contains async HDLC code since communication with CC1352 take place over UART using HDLC.
Since this is my first driver, I expect there to be some mistakes, so feel free to give feedback and point out things I might have missed or might not be aware of.
This driver has been created as a part of my Google Summer of Code 2023. For more information, take a look at my blog [2]
[1]: https://git.beagleboard.org/gsoc/greybus/cc1352-firmware [2]: https://programmershideaway.xyz/tags/gsoc23/
Ayush Singh (4): Add beagleplaygreybus to devicetree Add beagleplay greybus driver Add HDLC helper for beagleplay driver Allow building beagleplay greybus driver
.../arm64/boot/dts/ti/k3-am625-beagleplay.dts | 4 + drivers/staging/greybus/Kconfig | 9 + drivers/staging/greybus/Makefile | 5 + .../greybus/beagleplay-greybus-driver.c | 264 ++++++++++++++++++ .../greybus/beagleplay-greybus-driver.h | 28 ++ drivers/staging/greybus/hdlc.c | 229 +++++++++++++++ drivers/staging/greybus/hdlc.h | 137 +++++++++ 7 files changed, 676 insertions(+) create mode 100644 drivers/staging/greybus/beagleplay-greybus-driver.c create mode 100644 drivers/staging/greybus/beagleplay-greybus-driver.h create mode 100644 drivers/staging/greybus/hdlc.c create mode 100644 drivers/staging/greybus/hdlc.h
This UART is used for communication with beagleplay cc1352
Signed-off-by: Ayush Singh ayushdevel1325@gmail.com --- arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts index 7c1402b0fa2d..feead1396718 100644 --- a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts +++ b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts @@ -755,4 +755,8 @@ &main_uart6 { pinctrl-names = "default"; pinctrl-0 = <&wifi_debug_uart_pins_default>; status = "okay"; + + beagleplaygreybus { + compatible = "beagle,beagleplaygreybus"; + }; };
Signed-off-by: Ayush Singh ayushdevel1325@gmail.com --- .../greybus/beagleplay-greybus-driver.c | 264 ++++++++++++++++++ .../greybus/beagleplay-greybus-driver.h | 28 ++ 2 files changed, 292 insertions(+) create mode 100644 drivers/staging/greybus/beagleplay-greybus-driver.c create mode 100644 drivers/staging/greybus/beagleplay-greybus-driver.h
diff --git a/drivers/staging/greybus/beagleplay-greybus-driver.c b/drivers/staging/greybus/beagleplay-greybus-driver.c new file mode 100644 index 000000000000..1b4225a9fc8d --- /dev/null +++ b/drivers/staging/greybus/beagleplay-greybus-driver.c @@ -0,0 +1,264 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Beagleplay Linux Driver for Greybus + * + * Copyright (c) 2023 Ayush Singh ayushdevel1325@gmail.com + */ + +#include "beagleplay-greybus-driver.h" +#include "hdlc.h" +#include <linux/crc-ccitt.h> +#include <linux/device.h> +#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> + +#define BEAGLEPLAY_GREYBUS_DRV_VERSION "0.1.0" +#define BEAGLEPLAY_GREYBUS_DRV_NAME "bcfgreybus" + +#define BEAGLEPLAY_GB_MAX_CPORTS 32 + +static const struct of_device_id beagleplay_greybus_of_match[] = { + { + .compatible = "beagle,beagleplaygreybus", + }, + {}, +}; +MODULE_DEVICE_TABLE(of, beagleplay_greybus_of_match); + +static int beagleplay_greybus_serdev_write_locked(void *drvdata, + const unsigned char *buf, + size_t buf_len) +{ + struct beagleplay_greybus *beagleplay_greybus; + + beagleplay_greybus = drvdata; + return serdev_device_write_buf(beagleplay_greybus->serdev, buf, + buf_len); +} + +static void +beagleplay_greybus_handle_greybus_frame(struct beagleplay_greybus *bg, + u8 *buffer, size_t buffer_len) +{ + u16 cport_id; + struct gb_operation_msg_hdr *hdr = + (struct gb_operation_msg_hdr *)buffer; + memcpy(&cport_id, hdr->pad, sizeof(u16)); + if (hdr->result) { + dev_warn( + &bg->serdev->dev, + "Failed Greybus Operation %u of Type %X on CPort %u with Status %u", + hdr->operation_id, hdr->type, cport_id, hdr->result); + } else { + dev_dbg(&bg->serdev->dev, + "Successful Greybus Operation %u of Type %X on CPort %u", + hdr->operation_id, hdr->type, cport_id); + } + greybus_data_rcvd(bg->gb_host_device, cport_id, buffer, buffer_len); +} + +static void beagleplay_greybus_handle_dbg_frame(struct beagleplay_greybus *bg, + const char *buffer, + size_t buffer_len) +{ + char buf[RX_HDLC_PAYLOAD]; + + memcpy(buf, buffer, buffer_len); + buf[buffer_len] = 0; + dev_dbg(&bg->serdev->dev, "CC1352 Debug: %s", buf); +} + +static void beagleplay_greybus_handle_hdlc_rx_frame(void *drv_data, u8 *buffer, + size_t buffer_len, + uint8_t address) +{ + struct beagleplay_greybus *beagleplay_greybus; + + beagleplay_greybus = drv_data; + + switch (address) { + case ADDRESS_DBG: + beagleplay_greybus_handle_dbg_frame(beagleplay_greybus, buffer, + buffer_len); + break; + case ADDRESS_GREYBUS: + beagleplay_greybus_handle_greybus_frame(beagleplay_greybus, + buffer, buffer_len); + break; + default: + dev_warn(&beagleplay_greybus->serdev->dev, + "Got Unknown Frame %u", address); + } +} + +static int beagleplay_greybus_tty_receive(struct serdev_device *serdev, + const unsigned char *data, + size_t count) +{ + struct beagleplay_greybus *beagleplay_greybus; + + beagleplay_greybus = serdev_device_get_drvdata(serdev); + return hdlc_rx(beagleplay_greybus->hdlc_drv, data, count); +} + +static void beagleplay_greybus_tty_wakeup(struct serdev_device *serdev) +{ + struct beagleplay_greybus *beagleplay_greybus; + + beagleplay_greybus = serdev_device_get_drvdata(serdev); + hdlc_tx_wakeup(beagleplay_greybus->hdlc_drv); +} + +static struct serdev_device_ops beagleplay_greybus_ops = { + .receive_buf = beagleplay_greybus_tty_receive, + .write_wakeup = beagleplay_greybus_tty_wakeup, +}; + +static int gb_message_send(struct gb_host_device *hd, u16 cport_id, + struct gb_message *message, gfp_t gfp_mask) +{ + struct beagleplay_greybus *beagleplay_greybus; + char block_payload[HDLC_MAX_BLOCK_SIZE]; + + dev_dbg(&hd->dev, + "Sending Greybus message with Operation %u, Type: %X on Cport %u", + message->header->operation_id, message->header->type, cport_id); + + if (message->header->size > HDLC_MAX_BLOCK_SIZE) { + dev_err(&hd->dev, "Greybus message too big"); + return -1; + } + + beagleplay_greybus = dev_get_drvdata(&hd->dev); + memcpy(message->header->pad, &cport_id, sizeof(u16)); + memcpy(&block_payload, message->header, + sizeof(struct gb_operation_msg_hdr)); + memcpy(&block_payload[sizeof(struct gb_operation_msg_hdr)], + message->payload, message->payload_size); + hdlc_send_async(beagleplay_greybus->hdlc_drv, message->header->size, + &block_payload, ADDRESS_GREYBUS, 0x03); + + greybus_message_sent(beagleplay_greybus->gb_host_device, message, 0); + return 0; +} + +static void gb_message_cancel(struct gb_message *message) +{ +} + +static struct gb_hd_driver gb_hdlc_driver = { .message_send = gb_message_send, + .message_cancel = + gb_message_cancel }; + +static void beagleplay_greybus_start_svc(struct beagleplay_greybus *bg) +{ + const u8 command[1] = { CONTROL_SVC_START }; + + hdlc_send_async(bg->hdlc_drv, sizeof(command), command, ADDRESS_CONTROL, + 0x03); +} + +static void beagleplay_greybus_stop_svc(struct beagleplay_greybus *bg) +{ + const u8 command = CONTROL_SVC_STOP; + + hdlc_send_async(bg->hdlc_drv, 1, &command, ADDRESS_CONTROL, 0x03); +} + +static int beagleplay_greybus_probe(struct serdev_device *serdev) +{ + struct beagleplay_greybus *bg; + u32 speed = 115200; + int ret = 0; + + bg = devm_kmalloc(&serdev->dev, sizeof(struct beagleplay_greybus), + GFP_KERNEL); + + bg->serdev = serdev; + + serdev_device_set_drvdata(serdev, bg); + serdev_device_set_client_ops(serdev, &beagleplay_greybus_ops); + + ret = serdev_device_open(serdev); + if (ret) { + dev_err(&bg->serdev->dev, "Unable to Open Device"); + return ret; + } + + speed = serdev_device_set_baudrate(serdev, speed); + dev_info(&bg->serdev->dev, "Using baudrate %u\n", speed); + + serdev_device_set_flow_control(serdev, false); + + // Init HDLC + bg->hdlc_drv = + hdlc_init(&serdev->dev, beagleplay_greybus_serdev_write_locked, + beagleplay_greybus_handle_hdlc_rx_frame); + if (!bg->hdlc_drv) { + dev_err(&serdev->dev, "Failed to initialize HDLC"); + return -ENOMEM; + } + + // Greybus setup + bg->gb_host_device = + gb_hd_create(&gb_hdlc_driver, &serdev->dev, TX_CIRC_BUF_SIZE, + BEAGLEPLAY_GB_MAX_CPORTS); + if (IS_ERR(bg->gb_host_device)) { + dev_err(&bg->serdev->dev, + "Unable to create Greybus Host Device"); + return -1; + } + ret = gb_hd_add(bg->gb_host_device); + if (ret) { + dev_err(&serdev->dev, "Failed to add Greybus Host Device"); + return ret; + } + dev_set_drvdata(&bg->gb_host_device->dev, bg); + + beagleplay_greybus_start_svc(bg); + + dev_info(&bg->serdev->dev, "Successful Beagleplay Greybus Probe"); + + return 0; +} + +static void beagleplay_greybus_remove(struct serdev_device *serdev) +{ + struct beagleplay_greybus *beagleplay_greybus = + serdev_device_get_drvdata(serdev); + + dev_info(&beagleplay_greybus->serdev->dev, + "Remove BeaglePlay Greybus Driver"); + + // Free greybus stuff + gb_hd_del(beagleplay_greybus->gb_host_device); + gb_hd_put(beagleplay_greybus->gb_host_device); + + beagleplay_greybus_stop_svc(beagleplay_greybus); + + hdlc_deinit(beagleplay_greybus->hdlc_drv); + + serdev_device_close(serdev); +} + +static struct serdev_device_driver beagleplay_greybus_driver = { + .probe = beagleplay_greybus_probe, + .remove = beagleplay_greybus_remove, + .driver = { + .name = BEAGLEPLAY_GREYBUS_DRV_NAME, + .of_match_table = of_match_ptr(beagleplay_greybus_of_match), + }, +}; + +module_serdev_device_driver(beagleplay_greybus_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_AUTHOR("Ayush Singh ayushdevel1325@gmail.com"); +MODULE_DESCRIPTION("A Greybus driver for BeaglePlay"); +MODULE_VERSION(BEAGLEPLAY_GREYBUS_DRV_VERSION); diff --git a/drivers/staging/greybus/beagleplay-greybus-driver.h b/drivers/staging/greybus/beagleplay-greybus-driver.h new file mode 100644 index 000000000000..47b601b7fff4 --- /dev/null +++ b/drivers/staging/greybus/beagleplay-greybus-driver.h @@ -0,0 +1,28 @@ +/* SPDX-License-Identifier: GPL-2.0 + * + * Copyright (c) 2023 Ayush Singh ayushdevel1325@gmail.com + */ + +#ifndef BEAGLEPLAY_GREBUBS_DRIVER_H +#define BEAGLEPLAY_GREBUBS_DRIVER_H + +#include "hdlc.h" +#include <linux/greybus/hd.h> +#include <linux/init.h> + +#define ADDRESS_GREYBUS 0x01 +#define ADDRESS_DBG 0x02 +#define ADDRESS_CONTROL 0x04 + +#define CONTROL_SVC_START 0x01 +#define CONTROL_SVC_STOP 0x02 + +struct beagleplay_greybus { + struct serdev_device *serdev; + + struct hdlc_driver *hdlc_drv; + + struct gb_host_device *gb_host_device; +}; + +#endif
This file provides functions that allow sending and recieving async HDLC frames over any transport. Currently only tested with UART.
I am not quite sure where these files should go so I have put them close to Beagleplay greybus driver for now.
Signed-off-by: Ayush Singh ayushdevel1325@gmail.com --- drivers/staging/greybus/hdlc.c | 229 +++++++++++++++++++++++++++++++++ drivers/staging/greybus/hdlc.h | 137 ++++++++++++++++++++ 2 files changed, 366 insertions(+) create mode 100644 drivers/staging/greybus/hdlc.c create mode 100644 drivers/staging/greybus/hdlc.h
diff --git a/drivers/staging/greybus/hdlc.c b/drivers/staging/greybus/hdlc.c new file mode 100644 index 000000000000..079d4c10e476 --- /dev/null +++ b/drivers/staging/greybus/hdlc.c @@ -0,0 +1,229 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2023 Ayush Singh ayushdevel1325@gmail.com + */ + +#include "hdlc.h" +#include <linux/device.h> +#include <linux/crc-ccitt.h> +#include <linux/serdev.h> + +static void hdlc_write_locked(struct hdlc_driver *drv) +{ + // must be locked already + int head = smp_load_acquire(&drv->tx_circ_buf.head); + int tail = drv->tx_circ_buf.tail; + int count = CIRC_CNT_TO_END(head, tail, TX_CIRC_BUF_SIZE); + int written; + + if (count >= 1) { + written = drv->hdlc_send_frame_cb(dev_get_drvdata(drv->parent), + &drv->tx_circ_buf.buf[tail], + count); + + /* Finish consuming HDLC data */ + smp_store_release(&drv->tx_circ_buf.tail, + (tail + written) & (TX_CIRC_BUF_SIZE - 1)); + } +} + +static void hdlc_append(struct hdlc_driver *drv, u8 value) +{ + // must be locked already + int head, tail; + + head = drv->tx_circ_buf.head; + + while (true) { + tail = READ_ONCE(drv->tx_circ_buf.tail); + + if (CIRC_SPACE(head, tail, TX_CIRC_BUF_SIZE) >= 1) { + drv->tx_circ_buf.buf[head] = value; + + /* Finish producing HDLC byte */ + smp_store_release(&drv->tx_circ_buf.head, + (head + 1) & (TX_CIRC_BUF_SIZE - 1)); + return; + } + dev_warn(drv->parent, "Tx circ buf full\n"); + usleep_range(3000, 5000); + } +} + +static void hdlc_append_escaped(struct hdlc_driver *drv, u8 value) +{ + if (value == HDLC_FRAME || value == HDLC_ESC) { + hdlc_append(drv, HDLC_ESC); + value ^= HDLC_XOR; + } + hdlc_append(drv, value); +} + +static void hdlc_append_tx_frame(struct hdlc_driver *drv) +{ + drv->tx_crc = 0xFFFF; + hdlc_append(drv, HDLC_FRAME); +} + +static void hdlc_append_tx_u8(struct hdlc_driver *drv, u8 value) +{ + drv->tx_crc = crc_ccitt(drv->tx_crc, &value, 1); + hdlc_append_escaped(drv, value); +} + +static void hdlc_append_tx_buffer(struct hdlc_driver *drv, const void *buffer, + size_t len) +{ + size_t i; + + for (i = 0; i < len; i++) + hdlc_append_tx_u8(drv, ((u8 *)buffer)[i]); +} + +static void hdlc_append_tx_crc(struct hdlc_driver *drv) +{ + drv->tx_crc ^= 0xffff; + hdlc_append_escaped(drv, drv->tx_crc & 0xff); + hdlc_append_escaped(drv, (drv->tx_crc >> 8) & 0xff); +} + +static void hdlc_handle_rx_frame(struct hdlc_driver *drv) +{ + u8 address = drv->rx_buffer[0]; + char *buf = &drv->rx_buffer[2]; + size_t buf_len = drv->rx_buffer_len - 4; + + drv->hdlc_process_frame_cb(dev_get_drvdata(drv->parent), buf, buf_len, + address); +} + +static void hdlc_transmit(struct work_struct *work) +{ + struct hdlc_driver *drv = + container_of(work, struct hdlc_driver, tx_work); + + spin_lock_bh(&drv->tx_consumer_lock); + hdlc_write_locked(drv); + spin_unlock_bh(&drv->tx_consumer_lock); +} + +static void hdlc_send_s_frame_ack(struct hdlc_driver *drv) +{ + u8 address = drv->rx_buffer[0]; + + hdlc_send_async(drv, 0, NULL, address, (drv->rx_buffer[1] >> 1) & 0x7); +} + +int hdlc_rx(struct hdlc_driver *drv, const unsigned char *data, size_t count) +{ + u16 crc_check; + size_t i; + u8 c, ctrl; + + for (i = 0; i < count; ++i) { + c = data[i]; + + switch (c) { + case HDLC_FRAME: { + if (drv->rx_buffer_len) { + crc_check = crc_ccitt(0xffff, drv->rx_buffer, + drv->rx_buffer_len); + + if (crc_check == 0xf0b8) { + ctrl = drv->rx_buffer[1]; + if ((ctrl & 1) == 0) { + // I-Frame, send S-Frame ACK + hdlc_send_s_frame_ack(drv); + } + + hdlc_handle_rx_frame(drv); + } else { + dev_err(drv->parent, + "CRC Failed from %02x: 0x%04x\n", + drv->rx_buffer[0], crc_check); + } + } + drv->rx_buffer_len = 0; + break; + } + case HDLC_ESC: + drv->rx_in_esc = 1; + break; + default: + if (drv->rx_in_esc) { + c ^= 0x20; + drv->rx_in_esc = 0; + } + + if (drv->rx_buffer_len < MAX_RX_HDLC) { + drv->rx_buffer[drv->rx_buffer_len] = c; + drv->rx_buffer_len++; + } else { + // buffer overflow + dev_err(drv->parent, "RX Buffer Overflow\n"); + drv->rx_buffer_len = 0; + } + } + } + + return count; +} + +struct hdlc_driver *hdlc_init(struct device *parent, + hdlc_send_frame_callback hdlc_send_frame_cb, + hdlc_process_frame_callback hdlc_process_frame_cb) +{ + struct hdlc_driver *drv; + + drv = devm_kmalloc(parent, sizeof(*drv), GFP_KERNEL); + if (!drv) + goto early_exit; + + drv->parent = parent; + INIT_WORK(&drv->tx_work, hdlc_transmit); + drv->hdlc_send_frame_cb = hdlc_send_frame_cb; + spin_lock_init(&drv->tx_producer_lock); + spin_lock_init(&drv->tx_consumer_lock); + drv->tx_circ_buf.head = 0; + drv->tx_circ_buf.tail = 0; + drv->tx_circ_buf.buf = + devm_kmalloc(parent, TX_CIRC_BUF_SIZE, GFP_KERNEL); + drv->rx_buffer_len = 0; + drv->rx_in_esc = 0; + drv->hdlc_process_frame_cb = hdlc_process_frame_cb; + + return drv; + +early_exit: + return NULL; +} + +void hdlc_deinit(struct hdlc_driver *drv) +{ + flush_work(&drv->tx_work); +} + +void hdlc_send_async(struct hdlc_driver *drv, u16 length, const void *buffer, + u8 address, u8 control) +{ + // HDLC_FRAME + // 0 address : 0x01 + // 1 control : 0x03 + // contents + // x/y crc + // HDLC_FRAME + + spin_lock(&drv->tx_producer_lock); + + hdlc_append_tx_frame(drv); + hdlc_append_tx_u8(drv, + address); // address + hdlc_append_tx_u8(drv, control); // control + hdlc_append_tx_buffer(drv, buffer, length); + hdlc_append_tx_crc(drv); + hdlc_append_tx_frame(drv); + + spin_unlock(&drv->tx_producer_lock); + + schedule_work(&drv->tx_work); +} diff --git a/drivers/staging/greybus/hdlc.h b/drivers/staging/greybus/hdlc.h new file mode 100644 index 000000000000..7eae10871f88 --- /dev/null +++ b/drivers/staging/greybus/hdlc.h @@ -0,0 +1,137 @@ +/* SPDX-License-Identifier: GPL-2.0 + * + * Copyright (c) 2023 Ayush Singh ayushdevel1325@gmail.com + */ + +#ifndef _HDLC_H +#define _HDLC_H + +#include <linux/circ_buf.h> +#include <linux/types.h> +#include <linux/workqueue.h> + +#define MAX_RX_HDLC (1 + RX_HDLC_PAYLOAD + CRC_LEN) +#define RX_HDLC_PAYLOAD 1024 +#define CRC_LEN 2 +#define TX_CIRC_BUF_SIZE 1024 + +#define HDLC_FRAME 0x7E +#define HDLC_ESC 0x7D +#define HDLC_XOR 0x20 + +#define HDLC_MAX_BLOCK_SIZE 512 + +/* + * Callback to process a complete HDLC frame + * + * @param drvdata of hdlc device registered + * @param buffer of hdlc block + * @param length of buffer + * @param HDLC address + */ +typedef void (*hdlc_process_frame_callback)(void *, u8 *, size_t, uint8_t); + +/* + * Callback to send HDLC frame + * + * @param drvdata of hdlc device registered + * @param buffer of hdlc block + * @param length of buffer + */ +typedef int (*hdlc_send_frame_callback)(void *, const unsigned char *, size_t); + +/* + * HDLC driver + * + * @param device HDLC driver is using + * + * @param callback called when hdlc block is received + * @param callback called to send hdlc block + * + * @param transmit work + * @param transmit producer lock + * @param transmit consumer lock + * @param transmit circular buffer + * @param HDCL CRC + * @param current TX ACK sequence number + * + * @param Rx buffer length + * @param Rx HDLC block address + * @param Rx Flag to indicate if ESC + * @parma Rx buffer + */ +struct hdlc_driver { + struct device *parent; + + hdlc_process_frame_callback hdlc_process_frame_cb; + hdlc_send_frame_callback hdlc_send_frame_cb; + + struct work_struct tx_work; + /* tx_producer_lock: HDLC producer lock */ + spinlock_t tx_producer_lock; + /* tx_consumer_lock: HDLC consumer lock */ + spinlock_t tx_consumer_lock; + struct circ_buf tx_circ_buf; + u16 tx_crc; + u8 tx_ack_seq; + + u16 rx_buffer_len; + u8 rx_in_esc; + u8 rx_buffer[HDLC_MAX_BLOCK_SIZE]; +}; + +/* + * Queue data to be sent as an HDLC block + * + * @param hdlc driver + * @param buffer length + * @param buffer + * @param address + * @param control + */ +void hdlc_send_async(struct hdlc_driver *drv, u16 buffer_length, + const void *buffer, u8 address, u8 control); + +/* + * Add HDCL data for processing + * + * @param hdlc driver + * @param buffer + * @param buffer length + * + * @return number of bytes processed + */ +int hdlc_rx(struct hdlc_driver *drv, const unsigned char *buffer, + size_t buffer_length); + +/* + * Initialize HDLC + * + * @param device to use + * @param callback to send HDLC block + * @param callback to process hdlc frame + * + * @return hdlc driver allocated on heap + */ +struct hdlc_driver *hdlc_init(struct device *drv, + hdlc_send_frame_callback send_frame_cb, + hdlc_process_frame_callback process_frame_cb); + +/* + * De-initialize HDLC + * + * @param hdlc driver + */ +void hdlc_deinit(struct hdlc_driver *drv); + +/* + * Wakeup Tx + * + * @param hdlc_driver + */ +static inline void hdlc_tx_wakeup(struct hdlc_driver *drv) +{ + schedule_work(&drv->tx_work); +} + +#endif
Add KConfig and Makefile stuff
Signed-off-by: Ayush Singh ayushdevel1325@gmail.com --- drivers/staging/greybus/Kconfig | 9 +++++++++ drivers/staging/greybus/Makefile | 5 +++++ 2 files changed, 14 insertions(+)
diff --git a/drivers/staging/greybus/Kconfig b/drivers/staging/greybus/Kconfig index 927cfa4bc989..5306d773c3ce 100644 --- a/drivers/staging/greybus/Kconfig +++ b/drivers/staging/greybus/Kconfig @@ -213,4 +213,13 @@ config GREYBUS_ARCHE To compile this code as a module, chose M here: the module will be called gb-arche.ko
+config GREYBUS_BEAGLEPLAY + tristate "Greybus BeaglePlay driver" + depends on TTY + help + Select this option if you have a BeaglePlay device. + + To compile this code as a module, chose M here: the module + will be called gb-beagleplay.ko + endif # GREYBUS diff --git a/drivers/staging/greybus/Makefile b/drivers/staging/greybus/Makefile index 7c5e89622334..a94a88bfad01 100644 --- a/drivers/staging/greybus/Makefile +++ b/drivers/staging/greybus/Makefile @@ -71,3 +71,8 @@ obj-$(CONFIG_GREYBUS_USB) += gb-usb.o gb-arche-y := arche-platform.o arche-apb-ctrl.o
obj-$(CONFIG_GREYBUS_ARCHE) += gb-arche.o + +# Beagleplay Greybus driver +gb-beagleplay-y := beagleplay-greybus-driver.o hdlc.o + +obj-$(CONFIG_GREYBUS_BEAGLEPLAY) += gb-beagleplay.o
On Wed, Aug 23, 2023 at 10:25:17PM +0530, Ayush Singh wrote:
This UART is used for communication with beagleplay cc1352
Signed-off-by: Ayush Singh ayushdevel1325@gmail.com
arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts index 7c1402b0fa2d..feead1396718 100644 --- a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts +++ b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts @@ -755,4 +755,8 @@ &main_uart6 { pinctrl-names = "default"; pinctrl-0 = <&wifi_debug_uart_pins_default>; status = "okay";
- beagleplaygreybus {
compatible = "beagle,beagleplaygreybus";- };
};
2.41.0
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him a patch that has triggered this response. He used to manually respond to these common problems, but in order to save his sanity (he kept writing the same thing over and over, yet to different people), I was created. Hopefully you will not take offence and will fix the problem in your patch and resubmit it so that it can be accepted into the Linux kernel tree.
You are receiving this message because of the following common error(s) as indicated below:
- You did not specify a description of why the patch is needed, or possibly, any description at all, in the email body. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/process/submitting-patches.rst for what is needed in order to properly describe the change.
- You did not write a descriptive Subject: for the patch, allowing Greg, and everyone else, to know what this patch is all about. Please read the section entitled "The canonical patch format" in the kernel file, Documentation/process/submitting-patches.rst for what a proper Subject: line should look like.
If you wish to discuss this problem further, or you have questions about how to resolve this issue, please feel free to respond to this email and Greg will reply once he has dug out from the pending patches received from other developers.
thanks,
greg k-h's patch email bot
On Wed, Aug 23, 2023 at 10:25:19PM +0530, Ayush Singh wrote:
This file provides functions that allow sending and recieving async HDLC frames over any transport. Currently only tested with UART.
I am not quite sure where these files should go so I have put them close to Beagleplay greybus driver for now.
This shouldn't be in a changelog text :)
You can put comments below the --- line.
And to answer this question, no, it probably shouldn't be here in drivers/staging/ it should be in the "real" part of the kernel as it is a real driver. drivers/staging/ is for stuff that still needs work to do to get it out of that part of the kernel, do the work ahead of time and then you don't have to mess with that at all.
Signed-off-by: Ayush Singh ayushdevel1325@gmail.com
drivers/staging/greybus/hdlc.c | 229 +++++++++++++++++++++++++++++++++ drivers/staging/greybus/hdlc.h | 137 ++++++++++++++++++++
No need for a .h file for a simple .c file, just put it all together into one file please.
2 files changed, 366 insertions(+) create mode 100644 drivers/staging/greybus/hdlc.c create mode 100644 drivers/staging/greybus/hdlc.h
diff --git a/drivers/staging/greybus/hdlc.c b/drivers/staging/greybus/hdlc.c new file mode 100644 index 000000000000..079d4c10e476 --- /dev/null +++ b/drivers/staging/greybus/hdlc.c @@ -0,0 +1,229 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Copyright (c) 2023 Ayush Singh ayushdevel1325@gmail.com
- */
+#include "hdlc.h" +#include <linux/device.h> +#include <linux/crc-ccitt.h> +#include <linux/serdev.h>
+static void hdlc_write_locked(struct hdlc_driver *drv) +{
- // must be locked already
What must be locked? I don't understand this comment, sorry.
- int head = smp_load_acquire(&drv->tx_circ_buf.head);
- int tail = drv->tx_circ_buf.tail;
- int count = CIRC_CNT_TO_END(head, tail, TX_CIRC_BUF_SIZE);
- int written;
- if (count >= 1) {
written = drv->hdlc_send_frame_cb(dev_get_drvdata(drv->parent),&drv->tx_circ_buf.buf[tail],count);/* Finish consuming HDLC data */smp_store_release(&drv->tx_circ_buf.tail,(tail + written) & (TX_CIRC_BUF_SIZE - 1));- }
+}
+static void hdlc_append(struct hdlc_driver *drv, u8 value) +{
- // must be locked already
Again, I don't understand.
- int head, tail;
- head = drv->tx_circ_buf.head;
- while (true) {
tail = READ_ONCE(drv->tx_circ_buf.tail);if (CIRC_SPACE(head, tail, TX_CIRC_BUF_SIZE) >= 1) {drv->tx_circ_buf.buf[head] = value;/* Finish producing HDLC byte */smp_store_release(&drv->tx_circ_buf.head,(head + 1) & (TX_CIRC_BUF_SIZE - 1));return;}dev_warn(drv->parent, "Tx circ buf full\n");usleep_range(3000, 5000);- }
+}
+static void hdlc_append_escaped(struct hdlc_driver *drv, u8 value) +{
- if (value == HDLC_FRAME || value == HDLC_ESC) {
hdlc_append(drv, HDLC_ESC);value ^= HDLC_XOR;- }
- hdlc_append(drv, value);
+}
+static void hdlc_append_tx_frame(struct hdlc_driver *drv) +{
- drv->tx_crc = 0xFFFF;
- hdlc_append(drv, HDLC_FRAME);
+}
+static void hdlc_append_tx_u8(struct hdlc_driver *drv, u8 value) +{
- drv->tx_crc = crc_ccitt(drv->tx_crc, &value, 1);
- hdlc_append_escaped(drv, value);
+}
+static void hdlc_append_tx_buffer(struct hdlc_driver *drv, const void *buffer,
size_t len)+{
- size_t i;
- for (i = 0; i < len; i++)
hdlc_append_tx_u8(drv, ((u8 *)buffer)[i]);+}
+static void hdlc_append_tx_crc(struct hdlc_driver *drv) +{
- drv->tx_crc ^= 0xffff;
- hdlc_append_escaped(drv, drv->tx_crc & 0xff);
- hdlc_append_escaped(drv, (drv->tx_crc >> 8) & 0xff);
+}
+static void hdlc_handle_rx_frame(struct hdlc_driver *drv) +{
- u8 address = drv->rx_buffer[0];
- char *buf = &drv->rx_buffer[2];
- size_t buf_len = drv->rx_buffer_len - 4;
- drv->hdlc_process_frame_cb(dev_get_drvdata(drv->parent), buf, buf_len,
address);+}
+static void hdlc_transmit(struct work_struct *work) +{
- struct hdlc_driver *drv =
container_of(work, struct hdlc_driver, tx_work);- spin_lock_bh(&drv->tx_consumer_lock);
- hdlc_write_locked(drv);
- spin_unlock_bh(&drv->tx_consumer_lock);
+}
+static void hdlc_send_s_frame_ack(struct hdlc_driver *drv) +{
- u8 address = drv->rx_buffer[0];
- hdlc_send_async(drv, 0, NULL, address, (drv->rx_buffer[1] >> 1) & 0x7);
+}
+int hdlc_rx(struct hdlc_driver *drv, const unsigned char *data, size_t count)
Why is this a global function?
+void hdlc_send_async(struct hdlc_driver *drv, u16 length, const void *buffer,
u8 address, u8 control)+{
Same here, why is this global?
Who calls these new functions? There seems to be something missing here as if this is never called, there's no need for it?
confused,
greg k-h
On Wed, Aug 23, 2023 at 10:25:18PM +0530, Ayush Singh wrote:
Signed-off-by: Ayush Singh ayushdevel1325@gmail.com
I can't take patches without any changelog text at all (neither do you want me to.)
Writing the changelog is usually the hardest part. Take a look at the link my patch bot sent you and read that to see what to do.
.../greybus/beagleplay-greybus-driver.c | 264 ++++++++++++++++++
This is a really really really long name for a driver.
Why not beagle_gb.c?
.../greybus/beagleplay-greybus-driver.h | 28 ++
Again, no need for .h files for a simple .c file.
+#define BEAGLEPLAY_GREYBUS_DRV_VERSION "0.1.0"
driver versions make no sense in the kernel tree, just drop this.
+#define BEAGLEPLAY_GREYBUS_DRV_NAME "bcfgreybus"
KBUILD_MODNAME?
+#define BEAGLEPLAY_GB_MAX_CPORTS 32
Where does this number come from?
+static const struct of_device_id beagleplay_greybus_of_match[] = {
- {
.compatible = "beagle,beagleplaygreybus",
Are you sure this will work? You need to get your dt changes properly reviewed first.
- },
- {},
+}; +MODULE_DEVICE_TABLE(of, beagleplay_greybus_of_match);
+static int beagleplay_greybus_serdev_write_locked(void *drvdata,
const unsigned char *buf,size_t buf_len)+{
- struct beagleplay_greybus *beagleplay_greybus;
- beagleplay_greybus = drvdata;
- return serdev_device_write_buf(beagleplay_greybus->serdev, buf,
buf_len);+}
+static void +beagleplay_greybus_handle_greybus_frame(struct beagleplay_greybus *bg,
u8 *buffer, size_t buffer_len)+{
- u16 cport_id;
- struct gb_operation_msg_hdr *hdr =
(struct gb_operation_msg_hdr *)buffer;- memcpy(&cport_id, hdr->pad, sizeof(u16));
Did you run checkpatch on the code before sending it? Please do so.
- if (hdr->result) {
dev_warn(&bg->serdev->dev,"Failed Greybus Operation %u of Type %X on CPort %u with Status %u",hdr->operation_id, hdr->type, cport_id, hdr->result);- } else {
dev_dbg(&bg->serdev->dev,"Successful Greybus Operation %u of Type %X on CPort %u",hdr->operation_id, hdr->type, cport_id);- }
- greybus_data_rcvd(bg->gb_host_device, cport_id, buffer, buffer_len);
+}
+static void beagleplay_greybus_handle_dbg_frame(struct beagleplay_greybus *bg,
const char *buffer,size_t buffer_len)+{
- char buf[RX_HDLC_PAYLOAD];
Are you sure you can do all of that on the stack?
- memcpy(buf, buffer, buffer_len);
- buf[buffer_len] = 0;
- dev_dbg(&bg->serdev->dev, "CC1352 Debug: %s", buf);
Why are you using a stack buffer for a debug log?
And no need for prefixes on a debug message, that comes for free from the dynamic debug infrastructure.
and are you sure this buffer is a string?
+}
+static void beagleplay_greybus_handle_hdlc_rx_frame(void *drv_data, u8 *buffer,
size_t buffer_len,uint8_t address)+{
- struct beagleplay_greybus *beagleplay_greybus;
- beagleplay_greybus = drv_data;
This is usually just one line: struct beagleplay_greybus *beagleplay_greybus = drv_data;
but really, use less characters, how about: struct beagle_gb *beagle_gb = data;
or something like that.
- switch (address) {
- case ADDRESS_DBG:
beagleplay_greybus_handle_dbg_frame(beagleplay_greybus, buffer,buffer_len);
very long function names, you can make them smaller :)
break;- case ADDRESS_GREYBUS:
beagleplay_greybus_handle_greybus_frame(beagleplay_greybus,buffer, buffer_len);break;- default:
dev_warn(&beagleplay_greybus->serdev->dev,"Got Unknown Frame %u", address);- }
+}
+static int beagleplay_greybus_tty_receive(struct serdev_device *serdev,
const unsigned char *data,size_t count)+{
- struct beagleplay_greybus *beagleplay_greybus;
- beagleplay_greybus = serdev_device_get_drvdata(serdev);
- return hdlc_rx(beagleplay_greybus->hdlc_drv, data, count);
+}
+static void beagleplay_greybus_tty_wakeup(struct serdev_device *serdev) +{
- struct beagleplay_greybus *beagleplay_greybus;
- beagleplay_greybus = serdev_device_get_drvdata(serdev);
- hdlc_tx_wakeup(beagleplay_greybus->hdlc_drv);
+}
+static struct serdev_device_ops beagleplay_greybus_ops = {
- .receive_buf = beagleplay_greybus_tty_receive,
- .write_wakeup = beagleplay_greybus_tty_wakeup,
+};
+static int gb_message_send(struct gb_host_device *hd, u16 cport_id,
struct gb_message *message, gfp_t gfp_mask)+{
- struct beagleplay_greybus *beagleplay_greybus;
- char block_payload[HDLC_MAX_BLOCK_SIZE];
- dev_dbg(&hd->dev,
"Sending Greybus message with Operation %u, Type: %X on Cport %u",message->header->operation_id, message->header->type, cport_id);- if (message->header->size > HDLC_MAX_BLOCK_SIZE) {
dev_err(&hd->dev, "Greybus message too big");return -1;
Please always use real error values, not made up negative numbers. -ETOBIG?
- }
- beagleplay_greybus = dev_get_drvdata(&hd->dev);
- memcpy(message->header->pad, &cport_id, sizeof(u16));
- memcpy(&block_payload, message->header,
sizeof(struct gb_operation_msg_hdr));- memcpy(&block_payload[sizeof(struct gb_operation_msg_hdr)],
message->payload, message->payload_size);- hdlc_send_async(beagleplay_greybus->hdlc_drv, message->header->size,
&block_payload, ADDRESS_GREYBUS, 0x03);- greybus_message_sent(beagleplay_greybus->gb_host_device, message, 0);
- return 0;
+}
+static void gb_message_cancel(struct gb_message *message) +{ +}
Why is an empty function needed? That feels wrong.
+static struct gb_hd_driver gb_hdlc_driver = { .message_send = gb_message_send,
.message_cancel =gb_message_cancel };
Formatting can be fixed up here.
+static void beagleplay_greybus_start_svc(struct beagleplay_greybus *bg) +{
- const u8 command[1] = { CONTROL_SVC_START };
Are you sure this is allowed on the stack?
- hdlc_send_async(bg->hdlc_drv, sizeof(command), command, ADDRESS_CONTROL,
0x03);+}
+static void beagleplay_greybus_stop_svc(struct beagleplay_greybus *bg) +{
- const u8 command = CONTROL_SVC_STOP;
- hdlc_send_async(bg->hdlc_drv, 1, &command, ADDRESS_CONTROL, 0x03);
+}
+static int beagleplay_greybus_probe(struct serdev_device *serdev) +{
- struct beagleplay_greybus *bg;
- u32 speed = 115200;
- int ret = 0;
- bg = devm_kmalloc(&serdev->dev, sizeof(struct beagleplay_greybus),
GFP_KERNEL);- bg->serdev = serdev;
- serdev_device_set_drvdata(serdev, bg);
- serdev_device_set_client_ops(serdev, &beagleplay_greybus_ops);
- ret = serdev_device_open(serdev);
- if (ret) {
dev_err(&bg->serdev->dev, "Unable to Open Device");return ret;- }
- speed = serdev_device_set_baudrate(serdev, speed);
- dev_info(&bg->serdev->dev, "Using baudrate %u\n", speed);
- serdev_device_set_flow_control(serdev, false);
- // Init HDLC
- bg->hdlc_drv =
hdlc_init(&serdev->dev, beagleplay_greybus_serdev_write_locked,beagleplay_greybus_handle_hdlc_rx_frame);- if (!bg->hdlc_drv) {
dev_err(&serdev->dev, "Failed to initialize HDLC");return -ENOMEM;- }
- // Greybus setup
- bg->gb_host_device =
gb_hd_create(&gb_hdlc_driver, &serdev->dev, TX_CIRC_BUF_SIZE,BEAGLEPLAY_GB_MAX_CPORTS);- if (IS_ERR(bg->gb_host_device)) {
dev_err(&bg->serdev->dev,"Unable to create Greybus Host Device");return -1;
How about returning the error given to you?
- }
- ret = gb_hd_add(bg->gb_host_device);
- if (ret) {
dev_err(&serdev->dev, "Failed to add Greybus Host Device");return ret;
Did you just leak memory?
Anyway, these are all tiny things, it's great to see this work happening, it's getting close!
greg k-h
And to answer this question, no, it probably shouldn't be here in drivers/staging/ it should be in the "real" part of the kernel as it is a real driver. drivers/staging/ is for stuff that still needs work to do to get it out of that part of the kernel, do the work ahead of time and then you don't have to mess with that at all.
What do you mean by "real" part of kernel? You mean non-staging? The HDLC module/code initially started out as a part of beagleplay greybus driver (which started from wpanusb [1]). I separated it out since it should be possible to use it from other drivers which need async HDLC framing, but I am not sure how suitable it is to be used outside of UART. Thus, I do not feel it should be outside staging for now.
No need for a .h file for a simple .c file, just put it all together into one file please.
Well, it is not really a standalone driver. It is supposed to be used by some other driver (like serdev) to stack HDLC on top of that. So I think it needs .h file?
+int hdlc_rx(struct hdlc_driver *drv, const unsigned char *data, size_t count)
Why is this a global function?
These functions are called by any driver that wants to stack HDLC on top of the underlying transport. The HDLC files themselves can only read an HDLC frame or create an HDLC frame. It does not really care much about the underlying transport
I absolutely wish to make it clear that all the HDLC code can be put in beagleplay greybus driver (that's how it began). I just thought it might be better to separate it out for clarity and possibly allowing future drivers to use it for async HDLC framing.
Ayush Singh
[1]:https://git.beagleboard.org/beagleconnect/zephyr/wpanusb_bc
On Thu, Aug 24, 2023 at 11:21:26AM +0530, Ayush Singh wrote:
And to answer this question, no, it probably shouldn't be here in drivers/staging/ it should be in the "real" part of the kernel as it is a real driver. drivers/staging/ is for stuff that still needs work to do to get it out of that part of the kernel, do the work ahead of time and then you don't have to mess with that at all.
What do you mean by "real" part of kernel? You mean non-staging? The HDLC module/code initially started out as a part of beagleplay greybus driver (which started from wpanusb [1]). I separated it out since it should be possible to use it from other drivers which need async HDLC framing, but I am not sure how suitable it is to be used outside of UART. Thus, I do not feel it should be outside staging for now.
No need for a .h file for a simple .c file, just put it all together into one file please.
Well, it is not really a standalone driver. It is supposed to be used by some other driver (like serdev) to stack HDLC on top of that. So I think it needs .h file?
+int hdlc_rx(struct hdlc_driver *drv, const unsigned char *data, size_t count)
Why is this a global function?
These functions are called by any driver that wants to stack HDLC on top of the underlying transport. The HDLC files themselves can only read an HDLC frame or create an HDLC frame. It does not really care much about the underlying transport
I absolutely wish to make it clear that all the HDLC code can be put in beagleplay greybus driver (that's how it began). I just thought it might be better to separate it out for clarity and possibly allowing future drivers to use it for async HDLC framing.
Worry about future drivers then, in the future. We write kernel code for today, for what is needed today, and if you want to reuse anything later, wonderful, the code can be changed then to do so. But never make anything more complex today than it has to be.
thanks,
greg k-h
On 8/23/23 23:06, Greg KH wrote:
+#define BEAGLEPLAY_GB_MAX_CPORTS 32
Where does this number come from?
Well, it is the max number of Cports our APBridge supports. Since it is a KConfig variable on Zephyr application side, maybe it should be a config variable here as well?
- if (hdr->result) {
dev_warn(&bg->serdev->dev,"Failed Greybus Operation %u of Type %X on CPort %u with Status %u",hdr->operation_id, hdr->type, cport_id, hdr->result);- } else {
dev_dbg(&bg->serdev->dev,"Successful Greybus Operation %u of Type %X on CPort %u",hdr->operation_id, hdr->type, cport_id);- }
- greybus_data_rcvd(bg->gb_host_device, cport_id, buffer, buffer_len);
+}
+static void beagleplay_greybus_handle_dbg_frame(struct beagleplay_greybus *bg,
const char *buffer,size_t buffer_len)+{
- char buf[RX_HDLC_PAYLOAD];
Are you sure you can do all of that on the stack?
I think it should be fine. Zephyr application itself places a limit on the maximum size of an HDLC frame and compile time, and we will only process a single frame in this function. I do need to clean up some of these constants. And the zephyr application only supports a max frame of 256 bytes, so the current buffer is way too big.
- memcpy(buf, buffer, buffer_len);
- buf[buffer_len] = 0;
- dev_dbg(&bg->serdev->dev, "CC1352 Debug: %s", buf);
Why are you using a stack buffer for a debug log?
And no need for prefixes on a debug message, that comes for free from the dynamic debug infrastructure.
and are you sure this buffer is a string?
This is printing logs from BeaglePlay CC1352, which are transported over a specific HDLC address. This is because unless you have a JTAG, you cannot view anything happening in CC1352 without disabling the driver using dt overlay. This is incontinent for development and debugging.
It should always be a string since I am routing the zephyr log outputs over hdlc: https://git.beagleboard.org/gsoc/greybus/cc1352-firmware/-/blob/develop/src/...
- }
- beagleplay_greybus = dev_get_drvdata(&hd->dev);
- memcpy(message->header->pad, &cport_id, sizeof(u16));
- memcpy(&block_payload, message->header,
sizeof(struct gb_operation_msg_hdr));- memcpy(&block_payload[sizeof(struct gb_operation_msg_hdr)],
message->payload, message->payload_size);- hdlc_send_async(beagleplay_greybus->hdlc_drv, message->header->size,
&block_payload, ADDRESS_GREYBUS, 0x03);- greybus_message_sent(beagleplay_greybus->gb_host_device, message, 0);
- return 0;
+}
+static void gb_message_cancel(struct gb_message *message) +{ +}
Why is an empty function needed? That feels wrong.
Well, this is a required function to define: https://elixir.bootlin.com/linux/v6.5-rc7/source/drivers/greybus/hd.c#L136
+static void beagleplay_greybus_start_svc(struct beagleplay_greybus *bg) +{
- const u8 command[1] = { CONTROL_SVC_START };
Are you sure this is allowed on the stack?
Sorry, that should just be an u8 instead of u8 array. Will fix it.
- }
- ret = gb_hd_add(bg->gb_host_device);
- if (ret) {
dev_err(&serdev->dev, "Failed to add Greybus Host Device");return ret;Did you just leak memory?
Good catch
Thanks for the feedback. I will send a new patch once everything mentioned is fixed and tested.
Ayush Singh
On Fri, Aug 25, 2023 at 04:53:01PM +0530, Ayush Singh wrote:
On 8/23/23 23:06, Greg KH wrote:
+#define BEAGLEPLAY_GB_MAX_CPORTS 32
Where does this number come from?
Well, it is the max number of Cports our APBridge supports. Since it is a KConfig variable on Zephyr application side, maybe it should be a config variable here as well?
Just document it please. But if this gets out of sync with the device (as Linux has no idea what the device has), perhaps you should be able to detect it automatically?
- if (hdr->result) {
dev_warn(&bg->serdev->dev,"Failed Greybus Operation %u of Type %X on CPort %u with Status %u",hdr->operation_id, hdr->type, cport_id, hdr->result);- } else {
dev_dbg(&bg->serdev->dev,"Successful Greybus Operation %u of Type %X on CPort %u",hdr->operation_id, hdr->type, cport_id);- }
- greybus_data_rcvd(bg->gb_host_device, cport_id, buffer, buffer_len);
+}
+static void beagleplay_greybus_handle_dbg_frame(struct beagleplay_greybus *bg,
const char *buffer,size_t buffer_len)+{
- char buf[RX_HDLC_PAYLOAD];
Are you sure you can do all of that on the stack?
I think it should be fine. Zephyr application itself places a limit on the maximum size of an HDLC frame and compile time, and we will only process a single frame in this function. I do need to clean up some of these constants. And the zephyr application only supports a max frame of 256 bytes, so the current buffer is way too big.
Again, plutting that much data on the stack is generally a bad idea. Also, are you sure the hardware can copy from the stack properly? Many bus types can not handle this at all (i.e. USB), so can you just make it dynamic? Or is it needed at all?
- memcpy(buf, buffer, buffer_len);
- buf[buffer_len] = 0;
- dev_dbg(&bg->serdev->dev, "CC1352 Debug: %s", buf);
Why are you using a stack buffer for a debug log?
And no need for prefixes on a debug message, that comes for free from the dynamic debug infrastructure.
and are you sure this buffer is a string?
This is printing logs from BeaglePlay CC1352, which are transported over a specific HDLC address. This is because unless you have a JTAG, you cannot view anything happening in CC1352 without disabling the driver using dt overlay. This is incontinent for development and debugging.
It should always be a string since I am routing the zephyr log outputs over hdlc: https://git.beagleboard.org/gsoc/greybus/cc1352-firmware/-/blob/develop/src/...
Ok, but perhaps do something that doesn't need so much stack space just for a debug message?
- }
- beagleplay_greybus = dev_get_drvdata(&hd->dev);
- memcpy(message->header->pad, &cport_id, sizeof(u16));
- memcpy(&block_payload, message->header,
sizeof(struct gb_operation_msg_hdr));- memcpy(&block_payload[sizeof(struct gb_operation_msg_hdr)],
message->payload, message->payload_size);- hdlc_send_async(beagleplay_greybus->hdlc_drv, message->header->size,
&block_payload, ADDRESS_GREYBUS, 0x03);- greybus_message_sent(beagleplay_greybus->gb_host_device, message, 0);
- return 0;
+}
+static void gb_message_cancel(struct gb_message *message) +{ +}
Why is an empty function needed? That feels wrong.
Well, this is a required function to define: https://elixir.bootlin.com/linux/v6.5-rc7/source/drivers/greybus/hd.c#L136
If it's required, an empty function is not allowed, right? Otherwise the core would allow an empty function :)
thanks,
greg k-h
On 8/25/23 17:08, Greg KH wrote:
On Fri, Aug 25, 2023 at 04:53:01PM +0530, Ayush Singh wrote:
On 8/23/23 23:06, Greg KH wrote:
+#define BEAGLEPLAY_GB_MAX_CPORTS 32
Where does this number come from?
Well, it is the max number of Cports our APBridge supports. Since it is a KConfig variable on Zephyr application side, maybe it should be a config variable here as well?
Just document it please. But if this gets out of sync with the device (as Linux has no idea what the device has), perhaps you should be able to detect it automatically?
Let me see what I can do about the detection. There is no mechanism in greybus to do that, so it will need to be something bespoke I guess.
- if (hdr->result) {
dev_warn(&bg->serdev->dev,"Failed Greybus Operation %u of Type %X on CPort %u with Status %u",hdr->operation_id, hdr->type, cport_id, hdr->result);- } else {
dev_dbg(&bg->serdev->dev,"Successful Greybus Operation %u of Type %X on CPort %u",hdr->operation_id, hdr->type, cport_id);- }
- greybus_data_rcvd(bg->gb_host_device, cport_id, buffer, buffer_len);
+}
+static void beagleplay_greybus_handle_dbg_frame(struct beagleplay_greybus *bg,
const char *buffer,size_t buffer_len)+{
- char buf[RX_HDLC_PAYLOAD];
Are you sure you can do all of that on the stack?
I think it should be fine. Zephyr application itself places a limit on the maximum size of an HDLC frame and compile time, and we will only process a single frame in this function. I do need to clean up some of these constants. And the zephyr application only supports a max frame of 256 bytes, so the current buffer is way too big.
Again, plutting that much data on the stack is generally a bad idea. Also, are you sure the hardware can copy from the stack properly? Many bus types can not handle this at all (i.e. USB), so can you just make it dynamic? Or is it needed at all?
I can shift it to heap. The reason I am copying it here is that the HDLC frame payload is not null terminated. If it was possible to supply the length of the string buffer without null terminating it, we could remove the whole copying.
I think I can actually just NULL terminate the supplied buffer without creating any problem in the current setup (it will just override CRC which is not going to be checked again). Let me try that.
Also, the data is being copied from a heap array (`beagleplay_greybus->rx_buffer`), not any hardware directly.
- memcpy(buf, buffer, buffer_len);
- buf[buffer_len] = 0;
- dev_dbg(&bg->serdev->dev, "CC1352 Debug: %s", buf);
Why are you using a stack buffer for a debug log?
And no need for prefixes on a debug message, that comes for free from the dynamic debug infrastructure.
and are you sure this buffer is a string?
This is printing logs from BeaglePlay CC1352, which are transported over a specific HDLC address. This is because unless you have a JTAG, you cannot view anything happening in CC1352 without disabling the driver using dt overlay. This is incontinent for development and debugging.
It should always be a string since I am routing the zephyr log outputs over hdlc: https://git.beagleboard.org/gsoc/greybus/cc1352-firmware/-/blob/develop/src/...
Ok, but perhaps do something that doesn't need so much stack space just for a debug message?
Well, a debug frame will contain min(HDLC_FRAME_SIZE, DEBUG_MSG_SIZE). And I only know HDLC_FRAME_SIZE at compile time. I can allocate a heap array for every debug message (to make it the size of the actual message), but well I think it would make more sense to just allocate once and reuse it.
- }
- beagleplay_greybus = dev_get_drvdata(&hd->dev);
- memcpy(message->header->pad, &cport_id, sizeof(u16));
- memcpy(&block_payload, message->header,
sizeof(struct gb_operation_msg_hdr));- memcpy(&block_payload[sizeof(struct gb_operation_msg_hdr)],
message->payload, message->payload_size);- hdlc_send_async(beagleplay_greybus->hdlc_drv, message->header->size,
&block_payload, ADDRESS_GREYBUS, 0x03);- greybus_message_sent(beagleplay_greybus->gb_host_device, message, 0);
- return 0;
+}
+static void gb_message_cancel(struct gb_message *message) +{ +}
Why is an empty function needed? That feels wrong.
Well, this is a required function to define: https://elixir.bootlin.com/linux/v6.5-rc7/source/drivers/greybus/hd.c#L136
If it's required, an empty function is not allowed, right? Otherwise the core would allow an empty function :)
Well, the empty message was taken from `gb_netlink`. Anyway, I am not quite sure what the expected behavior of this function. A greybus message can be in one of the following stages:
1. Queued for sending over UART but still not left linux driver.
2. Sent to CC1352, but not sent to Greybus node yet.
3. Sent to greybus node,
If you add the state of response message as well, then you get 3 more for that.
Anyway, does cancel just mean that it does not want to receive a response for this message, or does it actively want to stop the message from being sent. In the former case, I can just maintain a list of cancelled messages and check each incoming message against it. This would mean the message will always be sent to node anyway and just not submitted back to `gb_host_device`.
The latter is much more difficult after stage 1. The APBridge on CC1352 is dumb by design and does not keep track of messages it send or receives. Making it keep track of every message from every direction will add a lot more complexity and cause performance problems (since it is just 1 core doing APBridge, SVC and node discovery stuff).
Ayush Singh
-
Ayush Singh -
Greg KH