On 02/09/2023 20:28, Ayush Singh wrote: > 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@xxxxxxxxx> > --- > MAINTAINERS | 1 + > drivers/greybus/Kconfig | 10 + > drivers/greybus/Makefile | 3 +- > drivers/greybus/gb-beagleplay.c | 494 ++++++++++++++++++++++++++++++++ > 4 files changed, 507 insertions(+), 1 deletion(-) > create mode 100644 drivers/greybus/gb-beagleplay.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 9d1b49a6dfad..3cbf2c87fb14 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -8974,6 +8974,7 @@ M: Ayush Singh <ayushdevel1325@xxxxxxxxx> > L: greybus-dev@xxxxxxxxxxxxxxxx (moderated for non-subscribers) > S: Maintained > F: Documentation/devicetree/bindings/serial/beaglecc1352.yaml > +F: drivers/greybus/gb-beagleplay.c > > GREYBUS SUBSYSTEM > M: Johan Hovold <johan@xxxxxxxxxx> > diff --git a/drivers/greybus/Kconfig b/drivers/greybus/Kconfig > index 78ba3c3083d5..62f091368272 100644 > --- a/drivers/greybus/Kconfig > +++ b/drivers/greybus/Kconfig > @@ -28,5 +28,15 @@ config GREYBUS_ES2 > To compile this code as a module, choose M here: the module > will be called gb-es2.ko > > +config GREYBUS_BEAGLEPLAY B shouild be before E. Keep things ordered, usually by name, do not add stuff at the end. > + tristate "Greybus BeaglePlay driver" > + depends on TTY > + 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 > + > endif # GREYBUS > > diff --git a/drivers/greybus/Makefile b/drivers/greybus/Makefile > index 9bccdd229aa2..63f9cb3b2df0 100644 > --- a/drivers/greybus/Makefile > +++ b/drivers/greybus/Makefile > @@ -23,4 +23,5 @@ gb-es2-y := es2.o > > obj-$(CONFIG_GREYBUS_ES2) += gb-es2.o > > - > +# Beagleplay Greybus driver Drop useless comment > +obj-$(CONFIG_GREYBUS_BEAGLEPLAY) += gb-beagleplay.o Keep things ordered, usually by name, do not add stuff at the end. ... > +/* The maximum number of CPorts supported by Greybus Host Device */ > +#define BEAGLEPLAY_GB_MAX_CPORTS 32 > + > +static const struct of_device_id gb_beagleplay_of_match[] = { > + { > + .compatible = "beagle,cc1352", > + }, > + {}, > +}; > +MODULE_DEVICE_TABLE(of, gb_beagleplay_of_match); The entire structure should be next to probe, not at beginning of file. > + ... > +static void hdlc_handle_rx_frame(struct gb_beagleplay *bg) > +{ > + u8 address = bg->rx_buffer[0]; > + char *buffer = &bg->rx_buffer[2]; > + size_t buffer_len = bg->rx_buffer_len - 4; > + > + switch (address) { > + case ADDRESS_DBG: > + hdlc_handle_dbg_frame(bg, buffer, buffer_len); > + break; > + case ADDRESS_GREYBUS: > + hdlc_handle_greybus_frame(bg, buffer, buffer_len); > + break; > + default: > + dev_warn(&bg->serdev->dev, "Got Unknown Frame %u", address); ratelimit Probably as well in several places with possible flooding. > + } > +} > + > +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_send_async(struct gb_beagleplay *bg, u8 address, u8 control, > + const struct hdlc_payload payloads[], size_t count) > +{ > + size_t i; > + > + /* HDLC_FRAME Use Linux style of comments: /* * foo bar > + * 0 address : 0x01 > + * 1 control : 0x03 > + * contents > + * x/y crc > + * HDLC_FRAME > + */ > + > + 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_buffer(bg, payloads[i].payload, > + payloads[i].length); > + } > + hdlc_append_tx_crc(bg); > + hdlc_append_tx_frame(bg); > + > + spin_unlock(&bg->tx_producer_lock); > + > + schedule_work(&bg->tx_work); > +} > + > +static void hdlc_send_s_frame_ack(struct gb_beagleplay *bg) > +{ > + hdlc_send_async(bg, bg->rx_buffer[0], (bg->rx_buffer[1] >> 1) & 0x7, > + NULL, 0); > +} > + > +static int hdlc_rx(struct gb_beagleplay *bg, const u8 *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 (bg->rx_buffer_len) { > + crc_check = crc_ccitt(0xffff, bg->rx_buffer, > + bg->rx_buffer_len); > + if (crc_check == 0xf0b8) { > + ctrl = bg->rx_buffer[1]; > + if ((ctrl & 1) == 0) { > + /* I-Frame, send S-Frame ACK */ > + hdlc_send_s_frame_ack(bg); > + } > + hdlc_handle_rx_frame(bg); > + } else { > + dev_err(&bg->serdev->dev, > + "CRC Failed from %02x: 0x%04x", > + bg->rx_buffer[0], crc_check); > + } > + } > + bg->rx_buffer_len = 0; > + break; > + case HDLC_ESC: > + bg->rx_in_esc = 1; > + break; > + default: > + if (bg->rx_in_esc) { > + c ^= 0x20; > + bg->rx_in_esc = 0; > + } > + > + if (bg->rx_buffer_len < MAX_RX_HDLC) { > + bg->rx_buffer[bg->rx_buffer_len] = c; > + bg->rx_buffer_len++; > + } else { > + dev_err(&bg->serdev->dev, "RX Buffer Overflow"); > + bg->rx_buffer_len = 0; > + } > + } > + } > + > + return count; > +} > + > +static void 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->serdev->dev, TX_CIRC_BUF_SIZE, GFP_KERNEL); > + bg->rx_buffer_len = 0; > + bg->rx_in_esc = 0; > +} > + > +static void hdlc_deinit(struct gb_beagleplay *bg) > +{ > + flush_work(&bg->tx_work); > +} > + > +static int gb_beagleplay_tty_receive(struct serdev_device *serdev, > + const unsigned char *data, size_t count) > +{ > + struct gb_beagleplay *bg = serdev_device_get_drvdata(serdev); > + > + return hdlc_rx(bg, data, count); > +} > + > +static void beagleplay_greybus_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_beagleplay_tty_receive, > + .write_wakeup = beagleplay_greybus_tty_wakeup, > +}; > + > +static int gb_message_send(struct gb_host_device *hd, u16 cport_id, > + struct gb_message *msg, gfp_t gfp_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_id); > + > + if (msg->header->size > RX_HDLC_PAYLOAD) { > + dev_err(&hd->dev, "Greybus message too big"); > + return -E2BIG; > + } > + > + memcpy(msg->header->pad, &cport_id, sizeof(cport_id)); > + > + payloads[0].payload = msg->header; > + payloads[0].length = sizeof(*msg->header); > + payloads[1].payload = msg->payload; > + payloads[1].length = msg->payload_size; > + > + hdlc_send_async(bg, ADDRESS_GREYBUS, 0x03, payloads, 2); > + greybus_message_sent(bg->gb_host_device, msg, 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 gb_beagleplay_start_svc(struct gb_beagleplay *bg) > +{ > + const u8 command = CONTROL_SVC_START; > + const struct hdlc_payload payload = { .length = 1, > + .payload = (void *)&command }; > + > + hdlc_send_async(bg, ADDRESS_CONTROL, 0x03, &payload, 1); > +} > + > +static void gb_beagleplay_stop_svc(struct gb_beagleplay *bg) > +{ > + const u8 command = CONTROL_SVC_STOP; > + const struct hdlc_payload payload = { .length = 1, > + .payload = (void *)&command }; > + > + hdlc_send_async(bg, ADDRESS_CONTROL, 0x03, &payload, 1); > +} > + > +static int gb_beagleplay_probe(struct serdev_device *serdev) > +{ > + u32 speed = 115200; > + int ret = 0; > + struct gb_beagleplay *bg = > + devm_kmalloc(&serdev->dev, sizeof(*bg), GFP_KERNEL); > + > + if (!bg) { > + dev_err(&serdev->dev, "Failed to allocate driver"); Nope. Run coccinelle. None of the code in the kernel drivers prints error messages for allocation failures. > + return -ENOMEM; > + } > + > + bg->serdev = serdev; > + serdev_device_set_drvdata(serdev, bg); > + serdev_device_set_client_ops(serdev, &gb_beagleplay_ops); > + ret = serdev_device_open(serdev); > + if (ret) { > + dev_err(&bg->serdev->dev, "Unable to Open Device"); return dev_err_probe > + return ret; > + } > + speed = serdev_device_set_baudrate(serdev, speed); > + dev_info(&bg->serdev->dev, "Using baudrate %u", speed); > + serdev_device_set_flow_control(serdev, false); > + > + hdlc_init(bg); > + > + /* 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"); > + ret = -1; ret = PTR_ERR, not some random number. > + goto free_hdlc; > + } > + ret = gb_hd_add(bg->gb_host_device); > + if (ret) { > + dev_err(&serdev->dev, "Failed to add Greybus Host Device"); > + goto free_gb_hd; > + } > + dev_set_drvdata(&bg->gb_host_device->dev, bg); > + > + gb_beagleplay_start_svc(bg); > + > + dev_info(&bg->serdev->dev, "Successful Beagleplay Greybus Probe"); Drop silly tracing messages. > + > + return 0; > + > +free_gb_hd: > + gb_hd_del(bg->gb_host_device); > + gb_hd_put(bg->gb_host_device); > +free_hdlc: > + hdlc_deinit(bg); > + serdev_device_close(serdev); > + return ret; > +} > + > +static void gb_beagleplay_remove(struct serdev_device *serdev) > +{ > + struct gb_beagleplay *beagleplay_greybus = > + serdev_device_get_drvdata(serdev); > + > + dev_info(&beagleplay_greybus->serdev->dev, > + "Remove BeaglePlay Greybus Driver"); Drop silly tracing messages. > + > + gb_hd_del(beagleplay_greybus->gb_host_device); > + gb_hd_put(beagleplay_greybus->gb_host_device); > + > + gb_beagleplay_stop_svc(beagleplay_greybus); > + > + hdlc_deinit(beagleplay_greybus); > + > + serdev_device_close(serdev); > +} > + > +static struct serdev_device_driver gb_beagleplay_driver = { > + .probe = gb_beagleplay_probe, > + .remove = gb_beagleplay_remove, > + .driver = { > + .name = "gb_beagleplay", > + .of_match_table = of_match_ptr(gb_beagleplay_of_match), Drop of_match_ptr(). You will have warnings. Best regards, Krzysztof _______________________________________________ greybus-dev mailing list -- greybus-dev@xxxxxxxxxxxxxxxx To unsubscribe send an email to greybus-dev-leave@xxxxxxxxxxxxxxxx