2023-12-17 12:11:48

by Ayush Singh

[permalink] [raw]
Subject: [PATCH V3 0/1] Make gb-beagleplay driver Greybus compliant

In beagleplay beagleconnect setup, the AP is not directly connected to
greybus nodes. The SVC and APBridge tasks are moved to cc1352
coprocessor. This means that there is a need to send cport information
between linux host and cc1352.

In the initial version of the driver (and the reference implementation
gbridge I was using), the greybus header pad bytes were being used.
However, this was a violation of greybus spec.

The following patchset creates a wrapper around greybus message to send
the cport information without using the pad bytes.

---
V3:
- Rebase on char-misc-next
V2: https://lists.linaro.org/archives/list/[email protected]/thread/7YX5I6ZYZTNTAHBH3M3KAMOWXMTAWQNW/
- Add more comments explaining the new greybus hdlc frame payload
- Remove msg len warnings. These checks are also performed by
`greybus_data_rcvd` and thus no need for it here.

V1: https://lists.linaro.org/archives/list/[email protected]/thread/Y3ELHKLKTP5TQZ5LYKCE6GHWMA3PUOTV/


Ayush Singh (1):
greybus: gb-beagleplay: Remove use of pad bytes

drivers/greybus/gb-beagleplay.c | 58 ++++++++++++++++++++++++---------
1 file changed, 43 insertions(+), 15 deletions(-)


base-commit: e909abe885e2f399be7ac0560a010d7429f951e1
--
2.43.0



2023-12-17 12:12:03

by Ayush Singh

[permalink] [raw]
Subject: [PATCH V3 1/1] greybus: gb-beagleplay: Remove use of pad bytes

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 <[email protected]>
---
drivers/greybus/gb-beagleplay.c | 58 ++++++++++++++++++++++++---------
1 file changed, 43 insertions(+), 15 deletions(-)

diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c
index 43318c1993ba..7d98ae1a8263 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, 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, 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,25 +350,39 @@ 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 payload:
+ * 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 = cpu_to_le16(cport);

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)
+ 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, sizeof(cport));
-
- 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;
--
2.43.0