[PATCH v4 0/3] greybus: Add BeaglePlay Greybus Driver

[PATCH v4 0/3] greybus: Add BeaglePlay Greybus Driver

[Ayush Singh]

BeaglePlay is a board by BeagleBoard.org. It contains a main AM62
processor with a CC1352 co-processor. They are connected over UART.

Greybus is a hardware protocol that was designed to provide Unipro with a
sane application layer. It can be used in IOT and IIOT applications
keeping the intelligence on the host.

This driver has been tested on BeaglePlay by BeagleBoard.org. It serves
as Greybus Host device and communicates with BeaglePlay CC1352
co-processor which serves as Greybus SVC. This replaces the old setup with
bcfserial, wpanusb and GBridge. This driver also contains async HDLC code
since communication with SVC take place over UART using HDLC.

This driver has been created as a part of my Google Summer of Code 2023.
For more information, take a look at my blog.

This patchset has been tested over `next-20230825`.

My GSoC23 Blog: https://programmershideaway.xyz/tags/gsoc23/
Zephyr App: https://git.beagleboard.org/gsoc/greybus/cc1352-firmware
GitHub Branch: https://github.com/Ayush1325/linux/tree/gb-beagleplay
Video Demo: https://youtu.be/GVuIB7i5pjk

This the v4 of this patch
v3 -> v4:
- Add DT Bindings
- Reorder commits
- Improve commit messages

v2 -> v3:
- Move gb-beagleplay out of staging

v1 -> v2:
- Combine the driver into a single file
- Remove redundant code
- Fix Checkpatch complaints
- Other suggested changes

Ayush Singh (3):
  dt-bindings: Add beaglecc1352
  greybus: Add BeaglePlay Linux Driver
  dts: ti: k3-am625-beagleplay: Add beaglecc1352

 .../bindings/serial/beaglecc1352.yaml         |  25 +
 MAINTAINERS                                   |   7 +
 .../arm64/boot/dts/ti/k3-am625-beagleplay.dts |   4 +
 drivers/greybus/Kconfig                       |  10 +
 drivers/greybus/Makefile                      |   3 +-
 drivers/greybus/gb-beagleplay.c               | 494 ++++++++++++++++++
 6 files changed, 542 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/serial/beaglecc1352.yaml
 create mode 100644 drivers/greybus/gb-beagleplay.c

-- 
2.41.0

[PATCH v4 1/3] dt-bindings: Add beaglecc1352

[Ayush Singh]

Add DT bindings for BeagleCC1352 co-processor UART.

The BeaglePlay has a CC1352 co-processor. This co-processor is connected
to the main AM62 (running Linux) over UART. The CC1352 can run Zephyr
and other embedded OS. This commit adds DT bindings for the BeagleCC1352
UART, which will allow Linux platform drivers to identify and access this
device.

This commit adds serial/beaglecc1352 for identifying this UART. It is
used by an upcoming gb-beagleplay greybus driver.

Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com>
---
 .../bindings/serial/beaglecc1352.yaml         | 25 +++++++++++++++++++
 MAINTAINERS                                   |  6 +++++
 2 files changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/beaglecc1352.yaml

diff --git a/Documentation/devicetree/bindings/serial/beaglecc1352.yaml b/Documentation/devicetree/bindings/serial/beaglecc1352.yaml
new file mode 100644
index 000000000000..54db630a2a50
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/beaglecc1352.yaml
@@ -0,0 +1,25 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/serial/beaglecc1352.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: BeaglePlay CC1352 serial UART
+
+maintainers:
+  - Ayush Singh <ayushdevel1325@gmail.com>
+
+properties:
+  compatible:
+    const: beagle,cc1352
+
+required:
+  - compatible
+
+additionalProperties: false
+
+examples:
+  - |
+    beaglecc1352 {
+      compatible = "beagle,cc1352";
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 37b9626ee654..9d1b49a6dfad 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8969,6 +8969,12 @@ F:	drivers/staging/greybus/sdio.c
 F:	drivers/staging/greybus/spi.c
 F:	drivers/staging/greybus/spilib.c
 
+GREYBUS BEAGLEPLAY DRIVERS
+M:	Ayush Singh <ayushdevel1325@gmail.com>
+L:	greybus-dev@lists.linaro.org (moderated for non-subscribers)
+S:	Maintained
+F:	Documentation/devicetree/bindings/serial/beaglecc1352.yaml
+
 GREYBUS SUBSYSTEM
 M:	Johan Hovold <johan@kernel.org>
 M:	Alex Elder <elder@kernel.org>
-- 
2.41.0

Re: [PATCH v4 1/3] dt-bindings: Add beaglecc1352

[Krzysztof Kozlowski]

On 02/09/2023 20:28, Ayush Singh wrote:
> Add DT bindings for BeagleCC1352 co-processor UART.

This does not look like UART controller.

> 
> The BeaglePlay has a CC1352 co-processor. This co-processor is connected
> to the main AM62 (running Linux) over UART. The CC1352 can run Zephyr
> and other embedded OS. This commit adds DT bindings for the BeagleCC1352

Please do not use "This commit/patch", but imperative mood. See longer
explanation here:
https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95

> UART, which will allow Linux platform drivers to identify and access this
> device.
> 
> This commit adds serial/beaglecc1352 for identifying this UART. It is
> used by an upcoming gb-beagleplay greybus driver.

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC (and consider --no-git-fallback argument). It might
happen, that command when run on an older kernel, gives you outdated
entries. Therefore please be sure you base your patches on recent Linux
kernel.

> 
> Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com>
> ---
>  .../bindings/serial/beaglecc1352.yaml         | 25 +++++++++++++++++++

It's not a serial driver. Don't put it in unrelated directory.

>  MAINTAINERS                                   |  6 +++++
>  2 files changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/serial/beaglecc1352.yaml
> 
> diff --git a/Documentation/devicetree/bindings/serial/beaglecc1352.yaml b/Documentation/devicetree/bindings/serial/beaglecc1352.yaml
> new file mode 100644
> index 000000000000..54db630a2a50
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/beaglecc1352.yaml

Missing vendor prefix. Filename should match compatible. Compatible is
not "beaglecc1352"


> @@ -0,0 +1,25 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/serial/beaglecc1352.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: BeaglePlay CC1352 serial UART

How is this serial UART? Of what? The SoC? Do not describe interface but
the device.

> +
> +maintainers:
> +  - Ayush Singh <ayushdevel1325@gmail.com>
> +
> +properties:
> +  compatible:
> +    const: beagle,cc1352

No resources? This does not seem useful... Put it then only in trivial
devices if your hardware - hardware, not driver - does not have any
pins, interrupts or other resources.

> +
> +required:
> +  - compatible
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    beaglecc1352 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

Best regards,
Krzysztof

Re: [PATCH v4 1/3] dt-bindings: Add beaglecc1352

[Ayush Singh]

On 9/4/23 12:44, Krzysztof Kozlowski wrote:
> On 02/09/2023 20:28, Ayush Singh wrote:
>> Add DT bindings for BeagleCC1352 co-processor UART.
> This does not look like UART controller.
>
>
>> Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com>
>> ---
>>   .../bindings/serial/beaglecc1352.yaml         | 25 +++++++++++++++++++
> It's not a serial driver. Don't put it in unrelated directory.
>
>> @@ -0,0 +1,25 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/serial/beaglecc1352.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: BeaglePlay CC1352 serial UART
> How is this serial UART? Of what? The SoC? Do not describe interface but
> the device.
>
>> +
>> +maintainers:
>> +  - Ayush Singh <ayushdevel1325@gmail.com>
>> +
>> +properties:
>> +  compatible:
>> +    const: beagle,cc1352
> No resources? This does not seem useful... Put it then only in trivial
> devices if your hardware - hardware, not driver - does not have any
> pins, interrupts or other resources.
>
>> +
>> +required:
>> +  - compatible
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    beaglecc1352 {
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>
> Best regards,
> Krzysztof

I would like to get some help on how to tackle this particular device 
since I cannot seem to find anything similar to this setup. First let me 
go over the setup.

The BeaglePlay board has 2 processors. AM625 processor which is the main 
processor. This runs the main Linux system. This processor does not have 
direct access to SubG.

It also contains a CC1352P7 MCU with it's own flash program memory, ROM 
and SRAM. This processor has SubG antenna. It runs it's own OS (Zephyr 
by default).

The only way for CC1352P7 and AM625 to communicate is by using that 
particular UART which is just a standard 8250 UART. The setup pretty 
much looks like 2 boards connected over UART except the UART will always 
be static.

I guess it would make most sense to write a CC1352P7 binding in this 
case? However, I am not sure how accurate that is since anything from 
the Linux side will be interacting with the Zephyr application, and not 
the processor. So in all actuality, the processor itself does not matter 
at all.


Ayush Singh

Re: [PATCH v4 1/3] dt-bindings: Add beaglecc1352

[Krzysztof Kozlowski]

On 06/09/2023 18:47, Ayush Singh wrote:
> On 9/4/23 12:44, Krzysztof Kozlowski wrote:
>> On 02/09/2023 20:28, Ayush Singh wrote:
>>> Add DT bindings for BeagleCC1352 co-processor UART.
>> This does not look like UART controller.
>>
>>
>>> Signed-off-by: Ayush Singh <ayushdevel1325@gmail.com>
>>> ---
>>>   .../bindings/serial/beaglecc1352.yaml         | 25 +++++++++++++++++++
>> It's not a serial driver. Don't put it in unrelated directory.
>>
>>> @@ -0,0 +1,25 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/serial/beaglecc1352.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: BeaglePlay CC1352 serial UART
>> How is this serial UART? Of what? The SoC? Do not describe interface but
>> the device.
>>
>>> +
>>> +maintainers:
>>> +  - Ayush Singh <ayushdevel1325@gmail.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: beagle,cc1352
>> No resources? This does not seem useful... Put it then only in trivial
>> devices if your hardware - hardware, not driver - does not have any
>> pins, interrupts or other resources.
>>
>>> +
>>> +required:
>>> +  - compatible
>>> +
>>> +additionalProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    beaglecc1352 {
>> Node names should be generic. See also an explanation and list of
>> examples (not exhaustive) in DT specification:
>> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
>>
>> Best regards,
>> Krzysztof
> 
> I would like to get some help on how to tackle this particular device 
> since I cannot seem to find anything similar to this setup. First let me 
> go over the setup.
> 
> The BeaglePlay board has 2 processors. AM625 processor which is the main 
> processor. This runs the main Linux system. This processor does not have 
> direct access to SubG.
> 
> It also contains a CC1352P7 MCU with it's own flash program memory, ROM 
> and SRAM. This processor has SubG antenna. It runs it's own OS (Zephyr 
> by default).
> 
> The only way for CC1352P7 and AM625 to communicate is by using that 
> particular UART which is just a standard 8250 UART. The setup pretty 
> much looks like 2 boards connected over UART except the UART will always 
> be static.
> 
> I guess it would make most sense to write a CC1352P7 binding in this 
> case? However, I am not sure how accurate that is since anything from 
> the Linux side will be interacting with the Zephyr application, and not 
> the processor. So in all actuality, the processor itself does not matter 
> at all.

I think the bindings require specific name and give you proper hint what
should it be. If you still wonder, it means you still did not test your
DTS. Such testing is a requirement.

Best regards,
Krzysztof

[PATCH v4 2/3] greybus: Add BeaglePlay Linux Driver

[Ayush Singh]

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@gmail.com>
---
 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@gmail.com>
 L:	greybus-dev@lists.linaro.org (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@kernel.org>
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
+	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
+obj-$(CONFIG_GREYBUS_BEAGLEPLAY)	+= gb-beagleplay.o
diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c
new file mode 100644
index 000000000000..dccf30324203
--- /dev/null
+++ b/drivers/greybus/gb-beagleplay.c
@@ -0,0 +1,494 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Beagleplay Linux Driver for Greybus
+ *
+ * Copyright (c) 2023 Ayush Singh <ayushdevel1325@gmail.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 1024
+#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 0x04
+
+#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 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);
+
+/*
+ * BeaglePlay Greybus driver
+ *
+ * @serdev: Serdev device
+ *
+ * @gb_host_device: greybud host device
+ *
+ * @tx_work: transmit work
+ * @tx_producer_lock: transmit producer lock
+ * @tx_consumer_lock: transmit consumer lock
+ * @tx_circ_buf: transmit circular buffer
+ * @tx_crc: HDCL CRC
+ * @tx_ack_seq: current TX ACK sequence number
+ *
+ * @rx_buffer_len: Rx buffer length
+ * @rx_in_esc: Rx Flag to indicate if ESC
+ * @rx_buffer: Rx buffer
+ */
+struct gb_beagleplay {
+	struct serdev_device *serdev;
+
+	struct gb_host_device *gb_host_device;
+
+	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[MAX_RX_HDLC];
+};
+
+struct hdlc_payload {
+	u16 length;
+	void *payload;
+};
+
+static void hdlc_handle_greybus_frame(struct gb_beagleplay *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(cport_id));
+
+	dev_dbg(&bg->serdev->dev,
+		"Greybus Operation %u of Type %X on CPort %u with Status %u",
+		hdr->operation_id, hdr->type, cport_id, hdr->result);
+
+	greybus_data_rcvd(bg->gb_host_device, cport_id, buffer, buffer_len);
+}
+
+static void hdlc_handle_dbg_frame(struct gb_beagleplay *bg, const char *buffer,
+				  size_t buffer_len)
+{
+	dev_dbg(&bg->serdev->dev, "CC1352 Debug: %.*s", (int)buffer_len,
+		buffer);
+}
+
+/*
+ * Consume HDLC Buffer. This function assumes that consumer lock has been acquired.
+ */
+static void hdlc_write(struct gb_beagleplay *bg)
+{
+	/* Start consuning 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];
+	int written;
+
+	if (count > 0) {
+		written = serdev_device_write_buf(bg->serdev, buf, count);
+
+		/* Finish consuming HDLC data */
+		smp_store_release(&bg->tx_circ_buf.tail,
+				  (tail + written) & (TX_CIRC_BUF_SIZE - 1));
+	}
+}
+
+/*
+ * Queue HDLC data for sending. This function 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->serdev->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_buffer(struct gb_beagleplay *bg, const u8 *buffer,
+				  size_t len)
+{
+	size_t i;
+
+	for (i = 0; i < len; i++)
+		hdlc_append_tx_u8(bg, buffer[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_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);
+	}
+}
+
+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
+	 * 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");
+		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 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;
+		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");
+
+	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");
+
+	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),
+	    },
+};
+
+module_serdev_device_driver(gb_beagleplay_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Ayush Singh <ayushdevel1325@gmail.com>");
+MODULE_DESCRIPTION("A Greybus driver for BeaglePlay");
-- 
2.41.0

Re: [PATCH v4 2/3] greybus: Add BeaglePlay Linux Driver

[Krzysztof Kozlowski]

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@gmail.com>
> ---
>  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@gmail.com>
>  L:	greybus-dev@lists.linaro.org (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@kernel.org>
> 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

Re: [PATCH v4 2/3] greybus: Add BeaglePlay Linux Driver

[Ayush Singh]

>> +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.

I don't think `hdlc_handle_rx_frame` is the correct place since it only 
processes a single completed HDLC frame.  The more appropriate place 
would be `hdlc_rx` if we want to limit based on the number of HDLC 
frames or `gb_beagleplay_tty_receive` to limit based on the number of bytes.

I would like to ask, though, why is rate limiting required here? Won't 
`serdev_device_ops->receive_buf` already rate limit the number of bytes 
somewhat? Or is it related to blocking in the 
`serdev_device_ops->receive_buf` callback? In the case of latter, it 
would probably make sense to ratelimit based on number of frames, I think.


Ayush Singh

Re: [PATCH v4 2/3] greybus: Add BeaglePlay Linux Driver

[Krzysztof Kozlowski]

On 05/09/2023 18:27, Ayush Singh wrote:
>>> +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.
> 
> I don't think `hdlc_handle_rx_frame` is the correct place since it only 
> processes a single completed HDLC frame.  The more appropriate place 
> would be `hdlc_rx` if we want to limit based on the number of HDLC 
> frames or `gb_beagleplay_tty_receive` to limit based on the number of bytes.
> 
> I would like to ask, though, why is rate limiting required here? Won't 
> `serdev_device_ops->receive_buf` already rate limit the number of bytes 
> somewhat? Or is it related to blocking in the 
> `serdev_device_ops->receive_buf` callback? In the case of latter, it 
> would probably make sense to ratelimit based on number of frames, I think.

My comment might not be accurate, so I do not insist. The name of the
function suggested something being called very often (on every frame),
thus you would print warning also very often.

Best regards,
Krzysztof

Re: [PATCH v4 2/3] greybus: Add BeaglePlay Linux Driver

[Ayush Singh]

On 9/6/23 15:29, Krzysztof Kozlowski wrote:

> On 05/09/2023 18:27, Ayush Singh wrote:
>>>> +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.
>> I don't think `hdlc_handle_rx_frame` is the correct place since it only
>> processes a single completed HDLC frame.  The more appropriate place
>> would be `hdlc_rx` if we want to limit based on the number of HDLC
>> frames or `gb_beagleplay_tty_receive` to limit based on the number of bytes.
>>
>> I would like to ask, though, why is rate limiting required here? Won't
>> `serdev_device_ops->receive_buf` already rate limit the number of bytes
>> somewhat? Or is it related to blocking in the
>> `serdev_device_ops->receive_buf` callback? In the case of latter, it
>> would probably make sense to ratelimit based on number of frames, I think.
> My comment might not be accurate, so I do not insist. The name of the
> function suggested something being called very often (on every frame),
> thus you would print warning also very often.
>
> Best regards,
> Krzysztof
>
Rate limiting the logs is not a bad idea. Initially I was not aware you 
meant about logging, hence the question. With proper firmware in CC1352, 
the warning will never be printed. But maybe it can cause problem with 
improper firmware.


Ayush Singh

Re: [PATCH v4 2/3] greybus: Add BeaglePlay Linux Driver

[kernel test robot]

Hi Ayush,

kernel test robot noticed the following build errors:

[auto build test ERROR on tty/tty-testing]
[also build test ERROR on tty/tty-next tty/tty-linus linus/master v6.5 next-20230906]
[cannot apply to robh/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Ayush-Singh/dt-bindings-Add-beaglecc1352/20230903-022958
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git tty-testing
patch link:    https://lore.kernel.org/r/20230902182845.1840620-3-ayushdevel1325%40gmail.com
patch subject: [PATCH v4 2/3] greybus: Add BeaglePlay Linux Driver
config: microblaze-randconfig-r022-20230907 (https://download.01.org/0day-ci/archive/20230907/202309070423.35kLcJgy-lkp@intel.com/config)
compiler: microblaze-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230907/202309070423.35kLcJgy-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202309070423.35kLcJgy-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

>> drivers/greybus/gb-beagleplay.c:490:1: warning: data definition has no type or storage class
     490 | module_serdev_device_driver(gb_beagleplay_driver);
         | ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/greybus/gb-beagleplay.c:490:1: error: type defaults to 'int' in declaration of 'module_serdev_device_driver' [-Werror=implicit-int]
>> drivers/greybus/gb-beagleplay.c:490:1: warning: parameter names (without types) in function declaration
>> drivers/greybus/gb-beagleplay.c:481:36: warning: 'gb_beagleplay_driver' defined but not used [-Wunused-variable]
     481 | static struct serdev_device_driver gb_beagleplay_driver = {
         |                                    ^~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors


vim +490 drivers/greybus/gb-beagleplay.c

   480	
 > 481	static struct serdev_device_driver gb_beagleplay_driver = {
   482		.probe = gb_beagleplay_probe,
   483		.remove = gb_beagleplay_remove,
   484		.driver = {
   485		      .name = "gb_beagleplay",
   486		      .of_match_table = of_match_ptr(gb_beagleplay_of_match),
   487		    },
   488	};
   489	
 > 490	module_serdev_device_driver(gb_beagleplay_driver);
   491	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v4 3/3] dts: ti: k3-am625-beagleplay: Add beaglecc1352

[Ayush Singh]

The BeaglePlay board by BeagleBoard.org has a CC1352 co-processor. This
co-processor is connected to the main AM62 (running Linux) over UART. The
CC1352 can run Zephyr and other embedded OS. This commit adds support for
the CC1352 in the Linux kernel DTS. This allows Linux platform drivers to
identify the device and communicate with it.

This UART is used by gb-beagleplay, an upcoming Greybus driver for
BeaglePlay.

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 7cfdf562b53b..10abbb8feda5 100644
--- a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
+++ b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
@@ -870,6 +870,10 @@ &main_uart6 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&wifi_debug_uart_pins_default>;
 	status = "okay";
+
+	beaglecc1352 {
+		compatible = "beagle,cc1352";
+	};
 };
 
 &dss {
-- 
2.41.0

Re: [PATCH v4 3/3] dts: ti: k3-am625-beagleplay: Add beaglecc1352

[Krzysztof Kozlowski]

On 02/09/2023 20:28, Ayush Singh wrote:
> The BeaglePlay board by BeagleBoard.org has a CC1352 co-processor. This
> co-processor is connected to the main AM62 (running Linux) over UART. The
> CC1352 can run Zephyr and other embedded OS. This commit adds support for
> the CC1352 in the Linux kernel DTS. This allows Linux platform drivers to
> identify the device and communicate with it.
> 
> This UART is used by gb-beagleplay, an upcoming Greybus driver for
> BeaglePlay.

upcoming? So not present in this patchset?

> 
> 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 7cfdf562b53b..10abbb8feda5 100644
> --- a/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
> +++ b/arch/arm64/boot/dts/ti/k3-am625-beagleplay.dts
> @@ -870,6 +870,10 @@ &main_uart6 {
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&wifi_debug_uart_pins_default>;
>  	status = "okay";
> +
> +	beaglecc1352 {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation

It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).

Best regards,
Krzysztof

Re: [PATCH v4 0/3] greybus: Add BeaglePlay Greybus Driver

[Jason Kridner]

On Sat, Sep 2, 2023 at 2:28 PM Ayush Singh <ayushdevel1325@gmail.com> wrote:
>
> BeaglePlay is a board by BeagleBoard.org. It contains a main AM62
> processor with a CC1352 co-processor. They are connected over UART.
>
> Greybus is a hardware protocol that was designed to provide Unipro with a
> sane application layer. It can be used in IOT and IIOT applications
> keeping the intelligence on the host.
>
> This driver has been tested on BeaglePlay by BeagleBoard.org. It serves
> as Greybus Host device and communicates with BeaglePlay CC1352
> co-processor which serves as Greybus SVC. This replaces the old setup with
> bcfserial, wpanusb and GBridge. This driver also contains async HDLC code
> since communication with SVC take place over UART using HDLC.

Ayush,

I think part of the problem you are seeing in getting this patch set
accepted is due to a lack of clarity from me on branding. Yes, this is
a Greybus driver that runs on BeaglePlay and talks to specific
firmware running on a CC1352P7 on BeaglePlay, but none of those
individual things explains what this is on its own, it has to be
comprehended a bit more collectively. Together, I've been calling this
the "BeagleConnect" concept and I need to do a bit more to smooth out
the rough edges in the communications, just as you have smoothed out
the rough edges in the implementation by moving GBridge out of
userspace and made a proper Greybus driver.

I have a draft of the concept at
https://docs.beagleboard.org/latest/boards/beagleconnect/index.html,
but it is really in rough form. Let me try to state it here for
clarity and I think you might paraphrase part of it in your next patch
set as I see you've had some comments that likely need to be addressed
such that a v5 will be on the way.

BeagleConnect is both a technology concept and a line of board designs
that implement the technology. Born from Greybus, a mechanism for
dynamically extending a Linux system with embedded peripherals,
BeagleConnect adds two key elements: a 6LoWPAN transport and mikroBUS
manifests. The 6LoWPAN transport provides for arbitrary connections,
including the IEEE802.15.4g long-range wireless transport supported
between BeaglePlay and BeagleConnect Freedom (the first BeagleConnect
board design). The mikroBUS manifests provide for rapid prototyping
and low-node-count production with sensor boards following the
mikroBUS freely-licensable embedded bus standard such that existing
Linux drivers can be loaded upon Greybus discovery of the nodes.

This patch set provides the Linux-side hooks required for the 6LoWPAN
transport for BeagleConnect on BeaglePlay. A different patch set,
currently in RFC, provides the mikroBUS manifest support to complete
the BeagleConnect functionality. (Be sure to use imperative mode if
paraphrasing this in the patch submission).

(If wondering, Beagle hasn't done any patent applications and
considers all public record of describing BeagleConnect technology as
evidence of prior art in public that will hopefully prevent anyone
else from trying to patent it. The name BeagleConnect is a trademark
owned by the BeagleBoard.org Foundation, but anyone should naturally
be able to implement Greybus over 6LoWPAN without any sort of
royalty--we just need to call it something so that people can
recognize compatible devices, so don't implement anything that isn't
interoperable and call it BeagleConnect, please.)

With this said, maybe the names can be a bit more clear? If I haven't
defined terms well enough, let me know.

So, when naming the binding, I'd think something like
"beagle,play-cc1352-connecthost". I removed the redundant use of
"beagle" if that seems right. I think it is accurate from a
hierarchical perspective because it runs on BeaglePlay, it talks to
the cc1352 and the cc1352 needs to be running the BeagleConnect host
firmware.

Hope this helps.

--Jason
(Board president at BeagleBoard.org Foundation)

>
> This driver has been created as a part of my Google Summer of Code 2023.
> For more information, take a look at my blog.
>
> This patchset has been tested over `next-20230825`.
>
> My GSoC23 Blog: https://programmershideaway.xyz/tags/gsoc23/
> Zephyr App: https://git.beagleboard.org/gsoc/greybus/cc1352-firmware
> GitHub Branch: https://github.com/Ayush1325/linux/tree/gb-beagleplay
> Video Demo: https://youtu.be/GVuIB7i5pjk
>
> This the v4 of this patch
> v3 -> v4:
> - Add DT Bindings
> - Reorder commits
> - Improve commit messages
>
> v2 -> v3:
> - Move gb-beagleplay out of staging
>
> v1 -> v2:
> - Combine the driver into a single file
> - Remove redundant code
> - Fix Checkpatch complaints
> - Other suggested changes
>
> Ayush Singh (3):
>   dt-bindings: Add beaglecc1352
>   greybus: Add BeaglePlay Linux Driver
>   dts: ti: k3-am625-beagleplay: Add beaglecc1352
>
>  .../bindings/serial/beaglecc1352.yaml         |  25 +
>  MAINTAINERS                                   |   7 +
>  .../arm64/boot/dts/ti/k3-am625-beagleplay.dts |   4 +
>  drivers/greybus/Kconfig                       |  10 +
>  drivers/greybus/Makefile                      |   3 +-
>  drivers/greybus/gb-beagleplay.c               | 494 ++++++++++++++++++
>  6 files changed, 542 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/serial/beaglecc1352.yaml
>  create mode 100644 drivers/greybus/gb-beagleplay.c
>
> --
> 2.41.0
>


-- 
https://beagleboard.org/about/jkridner - a 501c3 non-profit educating
around open hardware computing

[PATCH v4 0/3] greybus: Add BeaglePlay Greybus Driver

[Ayush Singh]

BeaglePlay is a board by BeagleBoard.org. It contains a main AM62
processor with a CC1352 co-processor. They are connected over UART.

Greybus is a hardware protocol that was designed to provide Unipro with a
sane application layer. It can be used in IOT and IIOT applications
keeping the intelligence on the host.

This driver has been tested on BeaglePlay by BeagleBoard.org. It serves
as Greybus Host device and communicates with BeaglePlay CC1352
co-processor which serves as Greybus SVC. This replaces the old setup with
bcfserial, wpanusb and GBridge. This driver also contains async HDLC code
since communication with SVC take place over UART using HDLC.

This driver has been created as a part of my Google Summer of Code 2023.
For more information, take a look at my blog.

This patchset has been tested over `next-20230825`.

My GSoC23 Blog: https://programmershideaway.xyz/tags/gsoc23/
Zephyr App: https://git.beagleboard.org/gsoc/greybus/cc1352-firmware
GitHub Branch: https://github.com/Ayush1325/linux/tree/gb-beagleplay
Video Demo: https://youtu.be/GVuIB7i5pjk

This the v4 of this patch
v3 -> v4:
- Add DT Bindings
- Reorder commits
- Improve commit messages

v2 -> v3:
- Move gb-beagleplay out of staging

v1 -> v2:
- Combine the driver into a single file
- Remove redundant code
- Fix Checkpatch complaints
- Other suggested changes

Ayush Singh (3):
  dt-bindings: Add beaglecc1352
  greybus: Add BeaglePlay Linux Driver
  dts: ti: k3-am625-beagleplay: Add beaglecc1352

 .../bindings/serial/beaglecc1352.yaml         |  25 +
 MAINTAINERS                                   |   7 +
 .../arm64/boot/dts/ti/k3-am625-beagleplay.dts |   4 +
 drivers/greybus/Kconfig                       |  10 +
 drivers/greybus/Makefile                      |   3 +-
 drivers/greybus/gb-beagleplay.c               | 494 ++++++++++++++++++
 6 files changed, 542 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/serial/beaglecc1352.yaml
 create mode 100644 drivers/greybus/gb-beagleplay.c

-- 
2.41.0