On Wed, 2018-08-01 at 09:53 +0200, Marcel Holtmann wrote:
> Hi Sean,
>
> > This adds a driver based on serdev driver for the MediaTek serial protocol
> > based on running H:4, which can enable the built-in Bluetooth device inside
> > MT7622 SoC.
> >
[ ... ]
> > +enum {
> > + MTK_WMT_PATCH_DWNLD = 0x1,
> > + MTK_WMT_FUNC_CTRL = 0x6,
> > + MTK_WMT_RST = 0x7
> > +};
> > +
> > +struct mtk_stp_hdr {
> > + u8 prefix;
> > + u8 dlen1:4;
> > + u8 type:4;
>
> So this is the hard one. I doubt that this is endian safe. It is also some fun way of packing it. Can you find a better variable name and just pack it into an u16 in the function. And then also label this __le16 or __be16 accordingly.
okay, I will do it. here I suppose 'u8 dlen1:4 and u8 type:4' only take up one byte.
> > + u8 dlen2;
> > + u8 cs;
>
> Are you checking the checksum on receive?
>
it is no needs. cs always shows zeros when I dump these received packets.
> > +} __packed;
> > +
> > +struct mtk_wmt_hdr {
> > + u8 dir;
> > + u8 op;
> > + __le16 dlen;
> > + u8 flag;
> > +} __packed;
> > +
> > +struct mtk_hci_wmt_cmd {
> > + struct mtk_wmt_hdr hdr;
> > + u8 data[256];
> > +} __packed;
> > +
> > +struct btmtkuart_dev {
> > + struct hci_dev *hdev;
> > + struct serdev_device *serdev;
> > +
> > + struct work_struct tx_work;
> > + unsigned long tx_state;
> > + struct sk_buff_head txq;
> > +
> > + struct sk_buff *rx_skb;
> > +
> > + struct mtk_stp_splitter *sp;
>
> This should be a leftover and no longer be needed.
>
okay. it's my fault and I should have a removal in the version
> > + struct clk *clk;
>
> Move the struct clk below struct serdev_device.
>
okay, it is a nice arrangement
> > +
> > + u8 stp_pad[6];
> > + u8 stp_cursor;
> > + u16 stp_dlen;
> > +};
> > +
> > +static int mtk_hci_wmt_sync(struct hci_dev *hdev, u8 op, u8 flag, u16 plen,
> > + const void *param)
> > +{
> > + struct mtk_hci_wmt_cmd wc;
> > + struct mtk_wmt_hdr *hdr;
> > + struct sk_buff *skb;
> > + u32 hlen;
> > +
> > + hlen = sizeof(*hdr) + plen;
> > + if (hlen > 255)
> > + return -EINVAL;
> > +
> > + hdr = (struct mtk_wmt_hdr *)&wc;
> > + hdr->dir = 1;
> > + hdr->op = op;
> > + hdr->dlen = cpu_to_le16(plen + 1);
> > + hdr->flag = flag;
> > + memcpy(wc.data, param, plen);
> > +
> > + atomic_inc(&hdev->cmd_cnt);
>
> Why are you doing this one. It will need a comment here if really needed. However I doubt that this is needed. You are only using it from hdev->setup and hdev->shutdown callbacks.
>
An increment on cmd_cnt is really needed because hci_cmd_work would check whether cmd_cnt is positive and then has a decrement on cmd_cnt before a packet is being sent out.
okay will add a comment.
> > +
> > + skb = __hci_cmd_sync_ev(hdev, 0xfc6f, hlen, &wc, HCI_VENDOR_PKT,
> > + HCI_INIT_TIMEOUT);
> > +
> > + if (IS_ERR(skb)) {
> > + int err = PTR_ERR(skb);
> > +
> > + bt_dev_err(hdev, "Failed to send wmt cmd (%d)", err);
> > + return err;
> > + }
> > +
> > + kfree_skb(skb);
> > +
> > + return 0;
> > +}
> > +
> > +static int mtk_setup_fw(struct hci_dev *hdev)
> > +{
> > + const struct firmware *fw;
> > + const char *fwname;
> > + const u8 *fw_ptr;
> > + size_t fw_size;
> > + int err, dlen;
> > + u8 flag;
> > +
> > + fwname = FIRMWARE_MT7622;
>
> Scrap the fwname variable and use it directly. If you later want to support newer/older hardware with other firmware names, we deal with it then.
>
okay
> > +
> > + err = request_firmware(&fw, fwname, &hdev->dev);
> > + if (err < 0) {
> > + bt_dev_err(hdev, "Failed to load firmware file (%d)", err);
> > + return err;
> > + }
> > +
> > + fw_ptr = fw->data;
> > + fw_size = fw->size;
> > +
> > + /* The size of patch header is 30 bytes, should be skip. */
> > + if (fw_size < 30)
> > + return -EINVAL;
> > +
> > + fw_size -= 30;
> > + fw_ptr += 30;
> > + flag = 1;
> > +
> > + while (fw_size > 0) {
> > + dlen = min_t(int, 250, fw_size);
> > +
> > + /* Tell deivice the position in sequence. */
> > + if (fw_size - dlen <= 0)
> > + flag = 3;
> > + else if (fw_size < fw->size - 30)
> > + flag = 2;
> > +
> > + err = mtk_hci_wmt_sync(hdev, MTK_WMT_PATCH_DWNLD, flag, dlen,
> > + fw_ptr);
> > + if (err < 0)
> > + break;
> > +
> > + fw_size -= dlen;
> > + fw_ptr += dlen;
> > + }
> > +
> > + release_firmware(fw);
> > +
> > + return err;
> > +}
> > +
> > +static int btmtkuart_recv_event(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
> > + struct hci_event_hdr *hdr = (void *)skb->data;
> > +
> > + /* Fix up the vendor event id with HCI_VENDOR_PKT instead of
> > + * 0xe4 so that btmon can parse the kind of vendor event properly.
> > + */
> > + if (hdr->evt == 0xe4)
> > + hdr->evt = HCI_VENDOR_PKT;
> > +
> > + /* Each HCI event would go through the core. */
>
> This comment adds really no value here. Just remove it.
>
okay
> > + return hci_recv_frame(hdev, skb);
> > +}
> > +
> > +static const struct h4_recv_pkt mtk_recv_pkts[] = {
> > + { H4_RECV_ACL, .recv = hci_recv_frame },
> > + { H4_RECV_SCO, .recv = hci_recv_frame },
> > + { H4_RECV_EVENT, .recv = btmtkuart_recv_event },
> > +};
> > +
> > +static const unsigned char *
> > +mtk_stp_split(struct btmtkuart_dev *bdev, const unsigned char *data, int count,
> > + int *sz_h4)
> > +{
> > + struct mtk_stp_hdr *shdr;
> > +
> > + /* The cursor is reset when all the data of STP is consumed out. */
> > + if (!bdev->stp_dlen && bdev->stp_cursor >= 6)
> > + bdev->stp_cursor = 0;
> > +
> > + /* Filling pad until all STP info is obtained. */
> > + while (bdev->stp_cursor < 6 && count > 0) {
> > + bdev->stp_pad[bdev->stp_cursor] = *data;
> > + bdev->stp_cursor++;
> > + data++;
> > + count--;
> > + }
> > +
> > + /* Retrieve STP info and have a sanity check. */
> > + if (!bdev->stp_dlen && bdev->stp_cursor >= 6) {
> > + shdr = (struct mtk_stp_hdr *)&bdev->stp_pad[2];
> > + bdev->stp_dlen = shdr->dlen1 << 8 | shdr->dlen2;
> > +
> > + /* Resync STP when unexpected data is being read. */
> > + if (shdr->prefix != 0x80 || bdev->stp_dlen > 2048) {
> > + bt_dev_err(bdev->hdev, "stp format unexpect (%d, %d)",
> > + shdr->prefix, bdev->stp_dlen);
> > + bdev->stp_cursor = 2;
> > + bdev->stp_dlen = 0;
> > + }
> > + }
> > +
> > + /* Directly quit when there's no data found for H4 can process. */
> > + if (count <= 0)
> > + return NULL;
> > +
> > + /* Tranlate to how much the size of data H4 can handle so far. */
> > + *sz_h4 = min_t(int, count, bdev->stp_dlen);
> > +
> > + /* Update the remaining size of STP packet. */
> > + bdev->stp_dlen -= *sz_h4;
> > +
> > + /* Data points to STP payload which can be handled by H4. */
> > + return data;
> > +}
> > +
> > +static int btmtkuart_recv(struct hci_dev *hdev, const u8 *data, size_t count)
> > +{
> > + struct btmtkuart_dev *bdev = hci_get_drvdata(hdev);
> > + const unsigned char *p_left = data, *p_h4;
> > + int sz_left = count, sz_h4, adv;
> > + int err;
> > +
> > + while (sz_left > 0) {
> > + /* The serial data received from MT7622 BT controller is
> > + * at all time padded around with the STP header and tailer.
> > + *
> > + * A full STP packet is looking like
> > + * -----------------------------------
> > + * | STP header | H:4 | STP tailer |
> > + * -----------------------------------
> > + * but it doesn't guarantee to contain a full H:4 packet which
> > + * means that it's possible for multiple STP packets forms a
> > + * full H:4 packet that means extra STP header + length doesn't
> > + * indicate a full H:4 frame, things can fragment. Whose length
> > + * recorded in STP header just shows up the most length the
> > + * H:4 engine can handle currently.
> > + */
> > +
> > + p_h4 = mtk_stp_split(bdev, p_left, sz_left, &sz_h4);
> > + if (!p_h4)
> > + break;
> > +
> > + adv = p_h4 - p_left;
> > + sz_left -= adv;
> > + p_left += adv;
> > +
> > + bdev->rx_skb = h4_recv_buf(bdev->hdev, bdev->rx_skb, p_h4,
> > + sz_h4, mtk_recv_pkts,
> > + sizeof(mtk_recv_pkts));
> > + if (IS_ERR(bdev->rx_skb)) {
> > + err = PTR_ERR(bdev->rx_skb);
> > + bt_dev_err(bdev->hdev,
> > + "Frame reassembly failed (%d)", err);
> > + bdev->rx_skb = NULL;
> > + return err;
> > + }
> > +
> > + sz_left -= sz_h4;
> > + p_left += sz_h4;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void btmtkuart_tx_work(struct work_struct *work)
> > +{
> > + struct btmtkuart_dev *bdev = container_of(work, struct btmtkuart_dev,
> > + tx_work);
> > + struct serdev_device *serdev = bdev->serdev;
> > + struct hci_dev *hdev = bdev->hdev;
> > +
> > + while (1) {
> > + clear_bit(BTMTKUART_TX_STATE_WAKEUP, &bdev->tx_state);
> > +
> > + while (1) {
> > + struct sk_buff *skb = skb_dequeue(&bdev->txq);
> > + int len;
> > +
> > + if (!skb)
> > + break;
> > +
> > + len = serdev_device_write_buf(serdev, skb->data,
> > + skb->len);
> > + hdev->stat.byte_tx += len;
> > +
> > + skb_pull(skb, len);
> > + if (skb->len > 0) {
> > + skb_queue_head(&bdev->txq, skb);
> > + break;
> > + }
> > +
> > + switch (hci_skb_pkt_type(skb)) {
> > + case HCI_COMMAND_PKT:
> > + hdev->stat.cmd_tx++;
> > + break;
> > + case HCI_ACLDATA_PKT:
> > + hdev->stat.acl_tx++;
> > + break;
> > + case HCI_SCODATA_PKT:
> > + hdev->stat.sco_tx++;
> > + break;
> > + }
> > +
> > + kfree_skb(skb);
> > + }
> > +
> > + if (!test_bit(BTMTKUART_TX_STATE_WAKEUP, &bdev->tx_state))
> > + break;
> > + }
> > +
> > + clear_bit(BTMTKUART_TX_STATE_ACTIVE, &bdev->tx_state);
> > +}
> > +
> > +static void btmtkuart_tx_wakeup(struct btmtkuart_dev *bdev)
> > +{
> > + if (test_and_set_bit(BTMTKUART_TX_STATE_ACTIVE, &bdev->tx_state))
> > + set_bit(BTMTKUART_TX_STATE_WAKEUP, &bdev->tx_state);
> > +
> > + schedule_work(&bdev->tx_work);
> > +}
> > +
>
> Move btmtkuart_recv and mtk_stp_split above this function to keep them close where they are used.
>
okay
> > +static int btmtkuart_receive_buf(struct serdev_device *serdev, const u8 *data,
> > + size_t count)
> > +{
> > + struct btmtkuart_dev *bdev = serdev_device_get_drvdata(serdev);
> > + int err;
> > +
> > + err = btmtkuart_recv(bdev->hdev, data, count);
> > + if (err < 0)
> > + return err;
> > +
> > + bdev->hdev->stat.byte_rx += count;
> > +
> > + return count;
> > +}
> > +
> > +static void btmtkuart_write_wakeup(struct serdev_device *serdev)
> > +{
> > + struct btmtkuart_dev *bdev = serdev_device_get_drvdata(serdev);
> > +
> > + btmtkuart_tx_wakeup(bdev);
> > +}
> > +
> > +static const struct serdev_device_ops btmtkuart_client_ops = {
> > + .receive_buf = btmtkuart_receive_buf,
> > + .write_wakeup = btmtkuart_write_wakeup,
> > +};
> > +
> > +static int btmtkuart_open(struct hci_dev *hdev)
> > +{
> > + struct btmtkuart_dev *bdev = hci_get_drvdata(hdev);
> > + struct device *dev;
> > + int err;
> > +
> > + err = serdev_device_open(bdev->serdev);
> > + if (err) {
> > + bt_dev_err(hdev, "Unable to open UART device %s",
> > + dev_name(&bdev->serdev->dev));
> > + goto err_open;
> > + }
> > +
> > + dev = &bdev->serdev->dev;
> > +
> > + bdev->stp_cursor = 2;
> > + bdev->stp_dlen = 0;
> > +
> > + /* Enable the power domain and clock the device requires. */
> > + pm_runtime_enable(dev);
> > + err = pm_runtime_get_sync(dev);
> > + if (err < 0) {
> > + pm_runtime_put_noidle(dev);
> > + goto err_disable_rpm;
> > + }
> > +
> > + err = clk_prepare_enable(bdev->clk);
> > + if (err < 0)
> > + goto err_put_rpm;
>
> Add an extra empty line here.
>
okay
> > + return 0;
> > +
> > +err_put_rpm:
> > + pm_runtime_put_sync(dev);
> > +err_disable_rpm:
> > + pm_runtime_disable(dev);
> > +err_open:
> > + return err;
> > +}
> > +
> > +static int btmtkuart_close(struct hci_dev *hdev)
> > +{
> > + struct btmtkuart_dev *bdev = hci_get_drvdata(hdev);
> > + struct device *dev = &bdev->serdev->dev;
> > +
> > + /* Shutdown the clock and power domain the device requires. */
> > + clk_disable_unprepare(bdev->clk);
> > + pm_runtime_put_sync(dev);
> > + pm_runtime_disable(dev);
> > +
> > + serdev_device_close(bdev->serdev);
> > +
> > + return 0;
> > +}
> > +
> > +static int btmtkuart_flush(struct hci_dev *hdev)
> > +{
> > + struct btmtkuart_dev *bdev = hci_get_drvdata(hdev);
> > +
> > + /* Flush any pending characters */
> > + serdev_device_write_flush(bdev->serdev);
> > + skb_queue_purge(&bdev->txq);
> > +
> > + cancel_work_sync(&bdev->tx_work);
> > +
> > + kfree_skb(bdev->rx_skb);
> > + bdev->rx_skb = NULL;
>
> I would assume you want to reset the stp_cursor here as well.
>
yes, it can be and is better
> > +
> > + return 0;
> > +}
> > +
> > +static int btmtkuart_setup(struct hci_dev *hdev)
> > +{
> > + u8 param = 0x1;
> > + int err = 0;
> > +
> > + /* Setup a firmware which the device definitely requires. */
> > + err = mtk_setup_fw(hdev);
> > + if (err < 0)
> > + return err;
> > +
> > + /* Activate funciton the firmware providing to. */
> > + err = mtk_hci_wmt_sync(hdev, MTK_WMT_RST, 0x4, 0, 0);
> > + if (err < 0)
> > + return err;
> > +
> > + /* Enable Bluetooth protocol. */
> > + err = mtk_hci_wmt_sync(hdev, MTK_WMT_FUNC_CTRL, 0x0, sizeof(param),
> > + ¶m);
> > + if (err < 0)
> > + return err;
> > +
> > + set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
>
> Since you have your own driver. Just move this after the hdev->manufacturer setting in probe(). There is no need to keep setting this over and over again.
>
okay
> > +
> > + return 0;
> > +}
> > +
> > +static int btmtkuart_shutdown(struct hci_dev *hdev)
> > +{
> > + u8 param = 0x0;
> > + int err;
> > +
> > + /* Disable the device. */
> > + err = mtk_hci_wmt_sync(hdev, MTK_WMT_FUNC_CTRL, 0x0, sizeof(param),
> > + ¶m);
> > +
> > + return err;
> > +}
> > +
> > +static int btmtkuart_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
> > +{
> > + struct btmtkuart_dev *bdev = hci_get_drvdata(hdev);
> > + struct mtk_stp_hdr *shdr;
> > + struct sk_buff *new_skb;
> > + int dlen;
> > + u8 *p;
> > +
> > + /* Prepend skb with frame type */
> > + memcpy(skb_push(skb, 1), &hci_skb_pkt_type(skb), 1);
> > + dlen = skb->len;
> > +
> > + /* Make sure of STP header at least has 4-bytes free space to fill. */
> > + if (unlikely(skb_headroom(skb) < sizeof(*shdr))) {
> > + new_skb = skb_realloc_headroom(skb, sizeof(*shdr));
> > + kfree_skb(skb);
> > + skb = new_skb;
> > + }
> > +
> > + /* Build for STP packet format. */
> > + shdr = skb_push(skb, sizeof(*shdr));
> > + p = (u8 *)shdr;
> > + shdr->prefix = 0x80;
> > + shdr->dlen1 = (dlen & 0xf00) >> 8;
> > + shdr->type = 0;
> > + shdr->dlen2 = dlen & 0xff;
> > + shdr->cs = p[0] + p[1] + p[2];
>
as above discussion about shr->cs , it can be filled with zero to have less computing
> I would add another comment here that this added the STP trailer. And change the above to mention it adds the STP header.
>
sure
> And you might want to check if there is space for the trailer as well. Otherwise skb_put tends to call BUG() if I remember correctly. I know this is super unlikely since our bt_skb_alloc is pretty large.
>
sure, I will add the handling for that. it should be better to make sure all rooms are enough for header and trailer before adding content to them
> > + skb_put_zero(skb, MTK_STP_TLR_SIZE);
>
> Extra empty line here please.
>
okay
> > + skb_queue_tail(&bdev->txq, skb);
> > +
> > + btmtkuart_tx_wakeup(bdev);
> > + return 0;
> > +}
> > +
> > +static int btmtkuart_probe(struct serdev_device *serdev)
> > +{
> > + struct btmtkuart_dev *bdev;
> > + struct hci_dev *hdev;
> > +
> > + bdev = devm_kzalloc(&serdev->dev, sizeof(*bdev), GFP_KERNEL);
> > + if (!bdev)
> > + return -ENOMEM;
> > +
> > + bdev->clk = devm_clk_get(&serdev->dev, "ref");
> > + if (IS_ERR(bdev->clk))
> > + return PTR_ERR(bdev->clk);
> > +
> > + bdev->serdev = serdev;
> > + serdev_device_set_drvdata(serdev, bdev);
> > +
> > + serdev_device_set_client_ops(serdev, &btmtkuart_client_ops);
> > +
> > + INIT_WORK(&bdev->tx_work, btmtkuart_tx_work);
> > + skb_queue_head_init(&bdev->txq);
> > +
> > + /* Initialize and register HCI device */
> > + hdev = hci_alloc_dev();
> > + if (!hdev) {
> > + dev_err(&serdev->dev, "Can't allocate HCI device\n");
> > + return -ENOMEM;
> > + }
> > +
> > + bdev->hdev = hdev;
> > +
> > + hdev->bus = HCI_UART;
> > + hci_set_drvdata(hdev, bdev);
> > +
> > + hdev->open = btmtkuart_open;
> > + hdev->close = btmtkuart_close;
> > + hdev->flush = btmtkuart_flush;
> > + hdev->setup = btmtkuart_setup;
> > + hdev->shutdown = btmtkuart_shutdown;
> > + hdev->send = btmtkuart_send_frame;
> > + SET_HCIDEV_DEV(hdev, &serdev->dev);
> > +
> > + hdev->manufacturer = 70;
> > +
> > + if (hci_register_dev(hdev) < 0) {
> > + dev_err(&serdev->dev, "Can't register HCI device\n");
> > + hci_free_dev(hdev);
> > + return -ENODEV;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void btmtkuart_remove(struct serdev_device *serdev)
> > +{
> > + struct btmtkuart_dev *bdev = serdev_device_get_drvdata(serdev);
> > + struct hci_dev *hdev = bdev->hdev;
> > +
> > + hci_unregister_dev(hdev);
> > + hci_free_dev(hdev);
> > +}
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id mtk_of_match_table[] = {
> > + { .compatible = "mediatek,mt7622-bluetooth"},
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_of_match_table);
> > +#endif
> > +
> > +static struct serdev_device_driver btmtkuart_driver = {
> > + .probe = btmtkuart_probe,
> > + .remove = btmtkuart_remove,
> > + .driver = {
> > + .name = "btmtkuart",
> > + .of_match_table = of_match_ptr(mtk_of_match_table),
> > + },
> > +};
> > +
> > +module_serdev_device_driver(btmtkuart_driver);
> > +
> > +MODULE_AUTHOR("Sean Wang <sean.wang@xxxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("MediaTek Bluetooth Serial driver" VERSION);
>
> You are missing a “ ver “ at the end of your string here. Check with modinfo that it looks correct.
>
okay
> > +MODULE_VERSION(VERSION);
> > +MODULE_LICENSE("GPL”);
>
> You want to add a MODULE_FIRMWARE here as well.
>
okay
> Regards
>
> Marcel
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
