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. Also adds required devicetree additions.
Tested over `next-20230825`.
Link: https://programmershideaway.xyz/tags/gsoc23/ GSoC23 Blog Link: https://git.beagleboard.org/gsoc/greybus/cc1352-firmware Zephyr App Link: https://github.com/Ayush1325/linux/tree/gb-beagleplay GitHub Branch Link: https://docs.beagleboard.org/latest/boards/beagleconnect/index.html BeagleConnect Link: https://docs.beagleboard.org/latest/boards/beagleplay/index.html BeaglePlay Link: https://github.com/Ayush1325/linux/tree/gb-beagleplay Github Repo Link: https://lists.linaro.org/archives/list/greybus-dev@lists.linaro.org/thread/B... Patch v5
Changes in Patch v6 v5 -> v6: - Rename compatible to `ti,cc1352p7` - Fix formatting - Use kerneldoc - Add clocks, power-gpios, reset-gpios to dt bindings
v4 -> v5: - Move DT Bindings to net - Rename compatible to `beagle,play-cc1352` - Expose CC1352 as MCU - Remove redundant tracing messages - Fix memory leaks
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
.../devicetree/bindings/net/ti,cc1352p7.yaml | 48 ++ MAINTAINERS | 7 + .../arm64/boot/dts/ti/k3-am625-beagleplay.dts | 4 + drivers/greybus/Kconfig | 10 + drivers/greybus/Makefile | 2 + drivers/greybus/gb-beagleplay.c | 495 ++++++++++++++++++ 6 files changed, 566 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/ti,cc1352p7.yaml create mode 100644 drivers/greybus/gb-beagleplay.c
base-commit: 6269320850097903b30be8f07a5c61d9f7592393
Add DT bindings for BeaglePlay CC1352 co-processor.
The BeaglePlay has a CC1352 co-processor. This co-processor is connected to the main AM62 (running Linux) over UART. In the BeagleConnect Technology, CC1352 is responsible for handling 6LoWPAN communication with beagleconnect freedom nodes as well as their discovery
This commit adds net/ti,cc1352p7. It is used by gb-beagleplay greybus driver.
Signed-off-by: Ayush Singh ayushdevel1325@gmail.com --- .../devicetree/bindings/net/ti,cc1352p7.yaml | 48 +++++++++++++++++++ MAINTAINERS | 6 +++ 2 files changed, 54 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
diff --git a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml new file mode 100644 index 000000000000..57bc2c43e5b1 --- /dev/null +++ b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml @@ -0,0 +1,48 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/ti,cc1352p7.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Texas Instruments Simplelink CC1352P7 wireless MCU + +description: + The cc1352p7 mcu can be connected via SPI or UART. + +maintainers: + - Ayush Singh ayushdevel1325@gmail.com + +properties: + compatible: + const: ti,cc1352p7 + + clocks: + maxItems: 2 + + reset-gpios: + maxItems: 1 + + power-gpios: + maxItems: 3 + description: + The device has three power rails that are exposed on external pins VDDS, + VDDR and DCOUPL. + + +required: + - compatible + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + serial { + mcu { + compatible = "ti,cc1352p7"; + clocks = <&sclk_hf 0>, <&sclk_lf 25>; + reset-gpios = <&pio 35 GPIO_ACTIVE_LOW>; + power-gpios = <&pio 1 GPIO_ACTIVE_HIGH>, <&pio 2 GPIO_ACTIVE_HIGH>, <&pio 3 GPIO_ACTIVE_HIGH>; + }; + }; diff --git a/MAINTAINERS b/MAINTAINERS index 37b9626ee654..5467669d7963 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/net/ti,cc1352p7.yaml + GREYBUS SUBSYSTEM M: Johan Hovold johan@kernel.org M: Alex Elder elder@kernel.org
On 02/10/2023 20:24, Ayush Singh wrote:
Add DT bindings for BeaglePlay CC1352 co-processor.
Thank you for your patch. There is something to discuss/improve.
Please use subject prefixes matching the subsystem. You can get them for example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory your patch is touching. For example: dt-bindings: net:
The BeaglePlay has a CC1352 co-processor. This co-processor is connected to the main AM62 (running Linux) over UART. In the BeagleConnect Technology, CC1352 is responsible for handling 6LoWPAN communication with beagleconnect freedom nodes as well as their discovery
This commit adds net/ti,cc1352p7. It is used by gb-beagleplay greybus
A nit: I pointed you to the documentation explaining not to use "This commit adds". It's v6 and the wording is back. Instead drop both sentences - they are pointless in this context. First one repeats previous text, second describes driver, but we do not talk here about drivers.
driver.
Signed-off-by: Ayush Singh ayushdevel1325@gmail.com
.../devicetree/bindings/net/ti,cc1352p7.yaml | 48 +++++++++++++++++++ MAINTAINERS | 6 +++ 2 files changed, 54 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
diff --git a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml new file mode 100644 index 000000000000..57bc2c43e5b1 --- /dev/null +++ b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml @@ -0,0 +1,48 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/ti,cc1352p7.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#
+title: Texas Instruments Simplelink CC1352P7 wireless MCU
+description:
- The cc1352p7 mcu can be connected via SPI or UART.
If over SPI, then the binding is incomplete. This is fine for now, I guess.
+maintainers:
- Ayush Singh ayushdevel1325@gmail.com
+properties:
- compatible:
- const: ti,cc1352p7
- clocks:
- maxItems: 2
Instead please list the items like:
clocks: items: - description: High-Foo-bar - description: Low-Foo-bar
- reset-gpios:
- maxItems: 1
- power-gpios:
- maxItems: 3
- description:
The device has three power rails that are exposed on external pins VDDS,
VDDR and DCOUPL.
No, power rails are not GPIOs. You need supplies, so: vdds-supply: true vddr-supply: true dcoupl-supply: true
Look also at: Documentation/devicetree/bindings/gpio/gpio-consumer-common.yaml which explicitly allows only one powerdown GPIO.
Best regards, Krzysztof
driver.
Signed-off-by: Ayush Singh ayushdevel1325@gmail.com
.../devicetree/bindings/net/ti,cc1352p7.yaml | 48 +++++++++++++++++++ MAINTAINERS | 6 +++ 2 files changed, 54 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
diff --git a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml new file mode 100644 index 000000000000..57bc2c43e5b1 --- /dev/null +++ b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml @@ -0,0 +1,48 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/ti,cc1352p7.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#
+title: Texas Instruments Simplelink CC1352P7 wireless MCU
+description:
- The cc1352p7 mcu can be connected via SPI or UART.
If over SPI, then the binding is incomplete. This is fine for now, I guess.
Best regards, Krzysztof
Well, I added the line about SPI because the data sheet states that CC1352P7 can be connected over SPI or UART when used as wireless MCU. But yes, I do not have much knowledge about SPI itself, so the bindings might be incomplete for SPI usage. Should I remove it or leave it be?
Sincerely,
Ayush Singh
On 17:39-20231003, Ayush Singh wrote:
driver.
Signed-off-by: Ayush Singh ayushdevel1325@gmail.com
.../devicetree/bindings/net/ti,cc1352p7.yaml | 48 +++++++++++++++++++ MAINTAINERS | 6 +++ 2 files changed, 54 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
diff --git a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml new file mode 100644 index 000000000000..57bc2c43e5b1 --- /dev/null +++ b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml @@ -0,0 +1,48 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/ti,cc1352p7.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#
+title: Texas Instruments Simplelink CC1352P7 wireless MCU
+description:
- The cc1352p7 mcu can be connected via SPI or UART.
If over SPI, then the binding is incomplete. This is fine for now, I guess.
Best regards, Krzysztof
Well, I added the line about SPI because the data sheet states that CC1352P7 can be connected over SPI or UART when used as wireless MCU. But yes, I do not have much knowledge about SPI itself, so the bindings might be incomplete for SPI usage. Should I remove it or leave it be?
I'd suggest to leave it for now, we can expand as there is a need.
On 23:54-20231002, Ayush Singh wrote:
Add DT bindings for BeaglePlay CC1352 co-processor.
The BeaglePlay has a CC1352 co-processor. This co-processor is connected to the main AM62 (running Linux) over UART. In the BeagleConnect Technology, CC1352 is responsible for handling 6LoWPAN communication with beagleconnect freedom nodes as well as their discovery
This commit adds net/ti,cc1352p7. It is used by gb-beagleplay greybus driver.
Signed-off-by: Ayush Singh ayushdevel1325@gmail.com
.../devicetree/bindings/net/ti,cc1352p7.yaml | 48 +++++++++++++++++++ MAINTAINERS | 6 +++ 2 files changed, 54 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/ti,cc1352p7.yaml
diff --git a/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml new file mode 100644 index 000000000000..57bc2c43e5b1 --- /dev/null +++ b/Documentation/devicetree/bindings/net/ti,cc1352p7.yaml @@ -0,0 +1,48 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/ti,cc1352p7.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml#
+title: Texas Instruments Simplelink CC1352P7 wireless MCU
+description:
- The cc1352p7 mcu can be connected via SPI or UART.
+maintainers:
- Ayush Singh ayushdevel1325@gmail.com
+properties:
- compatible:
- const: ti,cc1352p7
- clocks:
- maxItems: 2
I would suggest clock-names and description for it.
- reset-gpios:
- maxItems: 1
- power-gpios:
- maxItems: 3
- description:
The device has three power rails that are exposed on external pins VDDS,
VDDR and DCOUPL.
Shouldn't these be regulators? The power rails are input to the MCU, correct? The properties should be something like: vdds-supply vddr-supply dcoupl-supply ? (not sure what dcoupl is, but description should provide that info).
the gpio controls for those can be modelled by regulator-gpio ?
+required:
- compatible
+additionalProperties: false
+examples:
- |
- #include <dt-bindings/gpio/gpio.h>
- serial {
mcu {
compatible = "ti,cc1352p7";
clocks = <&sclk_hf 0>, <&sclk_lf 25>;
reset-gpios = <&pio 35 GPIO_ACTIVE_LOW>;
power-gpios = <&pio 1 GPIO_ACTIVE_HIGH>, <&pio 2 GPIO_ACTIVE_HIGH>, <&pio 3 GPIO_ACTIVE_HIGH>;
};
- };
diff --git a/MAINTAINERS b/MAINTAINERS index 37b9626ee654..5467669d7963 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/net/ti,cc1352p7.yaml
GREYBUS SUBSYSTEM M: Johan Hovold johan@kernel.org M: Alex Elder elder@kernel.org -- 2.41.0
- reset-gpios:
- maxItems: 1
- power-gpios:
- maxItems: 3
- description:
The device has three power rails that are exposed on external pins VDDS,
VDDR and DCOUPL.
Shouldn't these be regulators? The power rails are input to the MCU, correct? The properties should be something like: vdds-supply vddr-supply dcoupl-supply ? (not sure what dcoupl is, but description should provide that info).
the gpio controls for those can be modelled by regulator-gpio ?
I picked up power lines from "CC13xx/CC26xx Hardware Configuration and PCB Design Considerations Application Report" present under "8.14 Network Processor" of CC1352P7 data sheet.
But now looking closer, it doesn't seem like DCOUPL can be supplied externally for CC1352P7 and thus should probably be removed.
Also, it seems like for CC1352P7, VDDR must always be supplied internally (The data sheet states: "Internal supply, must be powered from the internal DC/DC converter or the internal LDO"). Thus, it should be safe to remove VDDR as well.
That means only VDDS needs to be present for power line.
CC13xx/CC26xx Hardware Configuration and PCB Design Considerations Application Report: https://www.ti.com/lit/pdf/swra640
CC1352P7 Data sheet: https://www.ti.com/lit/gpn/CC1352P7
Sincerely,
Ayush Singh
On 18:17-20231003, Ayush Singh wrote:
- reset-gpios:
- maxItems: 1
- power-gpios:
- maxItems: 3
- description:
The device has three power rails that are exposed on external pins VDDS,
VDDR and DCOUPL.
Shouldn't these be regulators? The power rails are input to the MCU, correct? The properties should be something like: vdds-supply vddr-supply dcoupl-supply ? (not sure what dcoupl is, but description should provide that info).
the gpio controls for those can be modelled by regulator-gpio ?
I picked up power lines from "CC13xx/CC26xx Hardware Configuration and PCB Design Considerations Application Report" present under "8.14 Network Processor" of CC1352P7 data sheet.
But now looking closer, it doesn't seem like DCOUPL can be supplied externally for CC1352P7 and thus should probably be removed.
Also, it seems like for CC1352P7, VDDR must always be supplied internally (The data sheet states: "Internal supply, must be powered from the internal DC/DC converter or the internal LDO"). Thus, it should be safe to remove VDDR as well.
From Figure 3-1. CC1312R 7x7 RF Part Schematic Overview (app report you point out below) Typical usage is vdds-supply supplying: VDDS (pin 44) VDDS2 (pin 13) VDDS3 (pin 22) VDDS_DCDC (pin 34)
And DCDC_SW (pin 33) supplies vddr supplying: VDDR(pin 45) VDDR_RF (Pin 48)
That means only VDDS needs to be present for power line.
I agree that that would be the typical supply model. So, just vdds-supply
CC13xx/CC26xx Hardware Configuration and PCB Design Considerations Application Report: https://www.ti.com/lit/pdf/swra640
CC1352P7 Data sheet: https://www.ti.com/lit/gpn/CC1352P7
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 | 2 + drivers/greybus/gb-beagleplay.c | 495 ++++++++++++++++++++++++++++++++ 4 files changed, 508 insertions(+) create mode 100644 drivers/greybus/gb-beagleplay.c
diff --git a/MAINTAINERS b/MAINTAINERS index 5467669d7963..d87e30626a6a 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/net/ti,cc1352p7.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..fd4f26d09c53 100644 --- a/drivers/greybus/Kconfig +++ b/drivers/greybus/Kconfig @@ -17,6 +17,16 @@ menuconfig GREYBUS
if GREYBUS
+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 + config GREYBUS_ES2 tristate "Greybus ES3 USB host controller" depends on USB diff --git a/drivers/greybus/Makefile b/drivers/greybus/Makefile index 9bccdd229aa2..d986e94f8897 100644 --- a/drivers/greybus/Makefile +++ b/drivers/greybus/Makefile @@ -18,6 +18,8 @@ obj-$(CONFIG_GREYBUS) += greybus.o # needed for trace events ccflags-y += -I$(src)
+obj-$(CONFIG_GREYBUS_BEAGLEPLAY) += gb-beagleplay.o + # Greybus Host controller drivers gb-es2-y := es2.o
diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c new file mode 100644 index 000000000000..831e3244661d --- /dev/null +++ b/drivers/greybus/gb-beagleplay.c @@ -0,0 +1,495 @@ +// 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 256 +#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 0x03 + +#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 GB_MAX_CPORTS 32 + +/** + * BeaglePlay Greybus driver + * + * @sd: underlying serdev device + * + * @gb_hd: greybus host device of this driver + * + * @tx_work: hdlc transmit work + * @tx_producer_lock: hdlc transmit data producer lock. acquired when appending data to buffer. + * @tx_consumer_lock: hdlc transmit data consumer lock. acquired when sending data over uart. + * @tx_circ_buf: hdlc transmit circular buffer. + * @tx_crc: hdlc transmit crc-ccitt fcs + * + * @rx_buffer_len: length of receive buffer filled. + * @rx_buffer: hdlc frame receive buffer + * @rx_in_esc: hdlc rx flag to indicate ESC frame + */ +struct gb_beagleplay { + struct serdev_device *sd; + + struct gb_host_device *gb_hd; + + struct work_struct tx_work; + spinlock_t tx_producer_lock; + spinlock_t tx_consumer_lock; + struct circ_buf tx_circ_buf; + u16 tx_crc; + + u16 rx_buffer_len; + bool rx_in_esc; + u8 rx_buffer[MAX_RX_HDLC]; +}; + +/** + * Structure to represent part of HDCL frame payload data. + * + * @len: buffer length in bytes + * @buf: payload buffer + */ +struct hdlc_payload { + u16 len; + void *buf; +}; + +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)); + + dev_dbg(&bg->sd->dev, "Greybus Operation %u type %X cport %u status %u received", + hdr->operation_id, hdr->type, cport_id, hdr->result); + + greybus_data_rcvd(bg->gb_hd, cport_id, buf, len); +} + +static void hdlc_rx_dbg_frame(const struct gb_beagleplay *bg, const char *buf, u16 len) +{ + dev_dbg(&bg->sd->dev, "CC1352 Log: %.*s", (int)len, buf); +} + +/** + * Consume HDLC Buffer. This function assumes that consumer lock has been acquired. + */ +static void hdlc_write(struct gb_beagleplay *bg) +{ + int written; + /* Start consuming 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]; + + if (count > 0) { + written = serdev_device_write_buf(bg->sd, 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->sd->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_buf(struct gb_beagleplay *bg, const u8 *buf, u16 len) +{ + size_t i; + + for (i = 0; i < len; i++) + hdlc_append_tx_u8(bg, buf[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_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_tx_frames(struct gb_beagleplay *bg, u8 address, u8 control, + const struct hdlc_payload payloads[], size_t count) +{ + size_t i; + + 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_buf(bg, payloads[i].buf, payloads[i].len); + + hdlc_append_tx_crc(bg); + hdlc_append_tx_frame(bg); + + spin_unlock(&bg->tx_producer_lock); + + schedule_work(&bg->tx_work); +} + +static void hdlc_tx_s_frame_ack(struct gb_beagleplay *bg) +{ + hdlc_tx_frames(bg, bg->rx_buffer[0], (bg->rx_buffer[1] >> 1) & 0x7, NULL, 0); +} + +static void hdlc_rx_frame(struct gb_beagleplay *bg) +{ + u16 crc, len; + u8 ctrl, *buf; + u8 address = bg->rx_buffer[0]; + + crc = crc_ccitt(0xffff, bg->rx_buffer, bg->rx_buffer_len); + if (crc != 0xf0b8) { + dev_warn_ratelimited(&bg->sd->dev, "CRC failed from %02x: 0x%04x", address, crc); + return; + } + + ctrl = bg->rx_buffer[1]; + buf = &bg->rx_buffer[2]; + len = bg->rx_buffer_len - 4; + + /* I-Frame, send S-Frame ACK */ + if ((ctrl & 1) == 0) + hdlc_tx_s_frame_ack(bg); + + switch (address) { + case ADDRESS_DBG: + hdlc_rx_dbg_frame(bg, buf, len); + break; + case ADDRESS_GREYBUS: + hdlc_rx_greybus_frame(bg, buf, len); + break; + default: + dev_warn_ratelimited(&bg->sd->dev, "unknown frame %u", address); + } +} + +static int hdlc_rx(struct gb_beagleplay *bg, const u8 *data, size_t count) +{ + size_t i; + u8 c; + + for (i = 0; i < count; ++i) { + c = data[i]; + + switch (c) { + case HDLC_FRAME: + if (bg->rx_buffer_len) + hdlc_rx_frame(bg); + + bg->rx_buffer_len = 0; + break; + case HDLC_ESC: + bg->rx_in_esc = true; + break; + default: + if (bg->rx_in_esc) { + c ^= 0x20; + bg->rx_in_esc = false; + } + + if (bg->rx_buffer_len < MAX_RX_HDLC) { + bg->rx_buffer[bg->rx_buffer_len] = c; + bg->rx_buffer_len++; + } else { + dev_err_ratelimited(&bg->sd->dev, "RX Buffer Overflow"); + bg->rx_buffer_len = 0; + } + } + } + + return count; +} + +static int 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->sd->dev, TX_CIRC_BUF_SIZE, GFP_KERNEL); + if (!bg->tx_circ_buf.buf) + return -ENOMEM; + + bg->rx_buffer_len = 0; + bg->rx_in_esc = false; + + return 0; +} + +static void hdlc_deinit(struct gb_beagleplay *bg) +{ + flush_work(&bg->tx_work); +} + +static int gb_tty_receive(struct serdev_device *sd, const unsigned char *data, size_t count) +{ + struct gb_beagleplay *bg = serdev_device_get_drvdata(sd); + + return hdlc_rx(bg, data, count); +} + +static void gb_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_tty_receive, + .write_wakeup = gb_tty_wakeup, +}; + +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]; + + 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) + 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; + + hdlc_tx_frames(bg, ADDRESS_GREYBUS, 0x03, payloads, 2); + greybus_message_sent(bg->gb_hd, 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 = { .len = 1, .buf = (void *)&command }; + + hdlc_tx_frames(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 = { .len = 1, .buf = (void *)&command }; + + hdlc_tx_frames(bg, ADDRESS_CONTROL, 0x03, &payload, 1); +} + +static void gb_greybus_deinit(struct gb_beagleplay *bg) +{ + gb_hd_del(bg->gb_hd); + gb_hd_put(bg->gb_hd); +} + +static int gb_greybus_init(struct gb_beagleplay *bg) +{ + int ret; + + bg->gb_hd = gb_hd_create(&gb_hdlc_driver, &bg->sd->dev, TX_CIRC_BUF_SIZE, GB_MAX_CPORTS); + if (IS_ERR(bg->gb_hd)) { + dev_err(&bg->sd->dev, "Failed to create greybus host device"); + return PTR_ERR(bg->gb_hd); + } + + ret = gb_hd_add(bg->gb_hd); + if (ret) { + dev_err(&bg->sd->dev, "Failed to add greybus host device"); + goto free_gb_hd; + } + dev_set_drvdata(&bg->gb_hd->dev, bg); + + return 0; + +free_gb_hd: + gb_greybus_deinit(bg); + return ret; +} + +static void gb_serdev_deinit(struct gb_beagleplay *bg) +{ + serdev_device_close(bg->sd); +} + +static int gb_serdev_init(struct gb_beagleplay *bg) +{ + u32 speed = 115200; + int ret; + + serdev_device_set_drvdata(bg->sd, bg); + serdev_device_set_client_ops(bg->sd, &gb_beagleplay_ops); + ret = serdev_device_open(bg->sd); + if (ret) + return dev_err_probe(&bg->sd->dev, ret, "Unable to open serial device"); + + speed = serdev_device_set_baudrate(bg->sd, speed); + serdev_device_set_flow_control(bg->sd, false); + + return 0; +} + +static int gb_beagleplay_probe(struct serdev_device *serdev) +{ + int ret = 0; + struct gb_beagleplay *bg; + + bg = devm_kmalloc(&serdev->dev, sizeof(*bg), GFP_KERNEL); + if (!bg) + return -ENOMEM; + + bg->sd = serdev; + ret = gb_serdev_init(bg); + if (ret) + return ret; + + ret = hdlc_init(bg); + if (ret) + goto free_serdev; + + ret = gb_greybus_init(bg); + if (ret) + goto free_hdlc; + + gb_beagleplay_start_svc(bg); + + return 0; + +free_hdlc: + hdlc_deinit(bg); +free_serdev: + gb_serdev_deinit(bg); + return ret; +} + +static void gb_beagleplay_remove(struct serdev_device *serdev) +{ + struct gb_beagleplay *bg = serdev_device_get_drvdata(serdev); + + gb_greybus_deinit(bg); + gb_beagleplay_stop_svc(bg); + hdlc_deinit(bg); + gb_serdev_deinit(bg); +} + +static const struct of_device_id gb_beagleplay_of_match[] = { + { + .compatible = "ti,cc1352p7", + }, + {}, +}; +MODULE_DEVICE_TABLE(of, gb_beagleplay_of_match); + +static struct serdev_device_driver gb_beagleplay_driver = { + .probe = gb_beagleplay_probe, + .remove = gb_beagleplay_remove, + .driver = { + .name = "gb_beagleplay", + .of_match_table = 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");
Hi Ayush,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 6269320850097903b30be8f07a5c61d9f7592393]
url: https://github.com/intel-lab-lkp/linux/commits/Ayush-Singh/dt-bindings-Add-b... base: 6269320850097903b30be8f07a5c61d9f7592393 patch link: https://lore.kernel.org/r/20231002182454.211165-3-ayushdevel1325%40gmail.com patch subject: [PATCH v6 2/3] greybus: Add BeaglePlay Linux Driver config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20231003/202310031521.Iq3S1RE9-lkp@i...) compiler: sh4-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231003/202310031521.Iq3S1RE9-lkp@i...)
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/202310031521.Iq3S1RE9-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/greybus/gb-beagleplay.c:45: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst
* BeaglePlay Greybus driver drivers/greybus/gb-beagleplay.c:78: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst * Structure to represent part of HDCL frame payload data. drivers/greybus/gb-beagleplay.c:107: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst * Consume HDLC Buffer. This function assumes that consumer lock has been acquired. drivers/greybus/gb-beagleplay.c:127: warning: This comment starts with '/**', but isn't a kernel-doc comment. Refer Documentation/doc-guide/kernel-doc.rst * Queue HDLC data for sending. This function assumes that producer lock as been acquired.
vim +45 drivers/greybus/gb-beagleplay.c
43 44 /**
45 * BeaglePlay Greybus driver
46 * 47 * @sd: underlying serdev device 48 * 49 * @gb_hd: greybus host device of this driver 50 * 51 * @tx_work: hdlc transmit work 52 * @tx_producer_lock: hdlc transmit data producer lock. acquired when appending data to buffer. 53 * @tx_consumer_lock: hdlc transmit data consumer lock. acquired when sending data over uart. 54 * @tx_circ_buf: hdlc transmit circular buffer. 55 * @tx_crc: hdlc transmit crc-ccitt fcs 56 * 57 * @rx_buffer_len: length of receive buffer filled. 58 * @rx_buffer: hdlc frame receive buffer 59 * @rx_in_esc: hdlc rx flag to indicate ESC frame 60 */ 61 struct gb_beagleplay { 62 struct serdev_device *sd; 63 64 struct gb_host_device *gb_hd; 65 66 struct work_struct tx_work; 67 spinlock_t tx_producer_lock; 68 spinlock_t tx_consumer_lock; 69 struct circ_buf tx_circ_buf; 70 u16 tx_crc; 71 72 u16 rx_buffer_len; 73 bool rx_in_esc; 74 u8 rx_buffer[MAX_RX_HDLC]; 75 }; 76
On 02/10/2023 20:24, 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.
...
Thank you for your patch. There is something to discuss/improve.
+static int gb_serdev_init(struct gb_beagleplay *bg) +{
- u32 speed = 115200;
- int ret;
- serdev_device_set_drvdata(bg->sd, bg);
- serdev_device_set_client_ops(bg->sd, &gb_beagleplay_ops);
- ret = serdev_device_open(bg->sd);
- if (ret)
return dev_err_probe(&bg->sd->dev, ret, "Unable to open serial device");
- speed = serdev_device_set_baudrate(bg->sd, speed);
Unused value speed. Probably this results in warnings. Plus it is a confusing code. Check the return value if it is relevant. If it can be ignored, skip assignment and drop "speed" variable.
Rest looks okay to me after glance (I did not perform full review).
- serdev_device_set_flow_control(bg->sd, false);
- return 0;
+}
+static int gb_beagleplay_probe(struct serdev_device *serdev) +{
- int ret = 0;
- struct gb_beagleplay *bg;
- bg = devm_kmalloc(&serdev->dev, sizeof(*bg), GFP_KERNEL);
- if (!bg)
return -ENOMEM;
Best regards, Krzysztof
The BeaglePlay board by BeagleBoard.org has a CC1352 co-processor. This co-processor is connected to the main AM62 (running Linux) over UART. In the BeagleConnect Technology, CC1352 is responsible for handling 6LoWPAN communication with beagleconnect freedom nodes as well as their discovery.
This mcu is used by gb-beagleplay, a 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..5160923b4dc2 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"; + + mcu { + compatible = "ti,cc1352p7"; + }; };
&dss {