On 12/11/23 12:54 AM, Ayush Singh wrote:

Make gb-beagleplay greybus spec compliant by moving cport information to transport layer instead of using `header->pad` bytes.

Greybus HDLC frame now has the following payload:

1. le16 cport
2. gb_operation_msg_hdr msg_header
3. u8 *msg_payload

Fixes: ec558bbfea67 ("greybus: Add BeaglePlay Linux Driver") Signed-off-by: Ayush Singh ayushdevel1325@gmail.com

I would say that this is an improvement, but I wish I had a better picture in mind of how this works. The initial commit provided some explanation, but even there it talks about the "CC1352 (running SVC Zephyr application)" and that leads me to wonder even how the hardware is structured. (I'm not really asking you for this right now, but you have a reference to something that provides some background, you should provide it for context.)

Another general comment is that the use of HDLC seems like it could be a more clearly separated layer that could be used by other Greybus protocols or applications. Maybe that's overkill, but it is a distinct layer, right?

I had a comment or two about using (void *) instead of (u8 *), to reduce the need for explicit type casts. But I found that (u8 *) is used elsewhere in the Greybus code.

One comment I *will* share is that the serdev RX callback has a const receive buffer. I recommend you preserve that "constness" in your code.

-Alex

---

drivers/greybus/gb-beagleplay.c | 55 ++++++++++++++++++++++++--------- 1 file changed, 41 insertions(+), 14 deletions(-)

diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c index 1e70ff7e3da4..82dc8a25e6b9 100644 --- a/drivers/greybus/gb-beagleplay.c +++ b/drivers/greybus/gb-beagleplay.c @@ -85,17 +85,31 @@ struct hdlc_payload { void *buf; }; +/**

• • struct hdlc_greybus_frame - Structure to represent greybus HDLC frame payload
• • @cport: cport id
• • @hdr: greybus operation header
• • @payload: greybus message payload
• • The HDLC payload sent over UART for greybus address has cport preappended to greybus message
• */

+struct hdlc_greybus_frame {

• __le16 cport;
• struct gb_operation_msg_hdr hdr;
• u8 payload[];

+} __packed;

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

• struct hdlc_greybus_frame *gb_frame = (struct hdlc_greybus_frame *)buf;
• u16 cport_id = le16_to_cpu(gb_frame->cport);
• u16 gb_msg_len = le16_to_cpu(gb_frame->hdr.size);

dev_dbg(&bg->sd->dev, "Greybus Operation %u type %X cport %u status %u received",

• hdr->operation_id, hdr->type, le16_to_cpu(cport_id), hdr->result);
  

• gb_frame->hdr.operation_id, gb_frame->hdr.type, cport_id, gb_frame->hdr.result);
  

• greybus_data_rcvd(bg->gb_hd, le16_to_cpu(cport_id), buf, len);

• greybus_data_rcvd(bg->gb_hd, cport_id, (u8 *)&gb_frame->hdr, gb_msg_len); }

static void hdlc_rx_dbg_frame(const struct gb_beagleplay *bg, const char *buf, u16 len) @@ -336,10 +350,23 @@ static struct serdev_device_ops gb_beagleplay_ops = { .write_wakeup = gb_tty_wakeup, }; +/**

• • gb_message_send() - Send greybus message using HDLC over UART
• • @hd: pointer to greybus host device
• • @cport: AP cport where message originates
• • @msg: greybus message to send
• • @mask: gfp mask
• • Greybus HDLC frame has the following format:
• • 1. le16 cport
• • 2. gb_operation_msg_hdr msg_header
• • 3. u8 *msg_payload
• */ 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];

• struct hdlc_payload payloads[3]; __le16 cport_id = le16_to_cpu(cport);

dev_dbg(&hd->dev, "Sending greybus message with Operation %u, Type: %X on Cport %u", @@ -348,14 +375,14 @@ static int gb_message_send(struct gb_host_device *hd, u16 cport, struct gb_messa if (le16_to_cpu(msg->header->size) > RX_HDLC_PAYLOAD) return dev_err_probe(&hd->dev, -E2BIG, "Greybus message too big");

• memcpy(msg->header->pad, &cport_id, sizeof(cport_id));
• payloads[0].buf = msg->header;
• payloads[0].len = sizeof(*msg->header);
• payloads[1].buf = msg->payload;
• payloads[1].len = msg->payload_size;

• payloads[0].buf = &cport_id;
• payloads[0].len = sizeof(cport_id);
• payloads[1].buf = msg->header;
• payloads[1].len = sizeof(*msg->header);
• payloads[2].buf = msg->payload;
• payloads[2].len = msg->payload_size;

• hdlc_tx_frames(bg, ADDRESS_GREYBUS, 0x03, payloads, 2);

• hdlc_tx_frames(bg, ADDRESS_GREYBUS, 0x03, payloads, 3); greybus_message_sent(bg->gb_hd, msg, 0);

return 0;