Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Sean Wang]

On Tue, 2018-05-08 at 13:18 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> > +

[ ... ]

> > 
> > I'm happy to do with btmon. just the environment with buildroot the BT
> > running on seems there's a missing support for btmon. I can start to use
> > btmon once I change the environment to Debian.
> > 
> >> So all the MTK vendor commands respond with a vendor event? Or are there 
> >> some that do the standard command status/complete handling?
> >> 
> > 
> > yes, mtk controller after mt7622 (included), its MTK vendors command
> > (opcode 0xfc6f) always respond with a vendor event id 0xe4. And they
> > don't do any standard status/complete handling.


> then we need to figure out where the __hci_cmd_sync_ev causes a problem. 
> Since normally that should just work for you.
> 

Okay. I will look into more about the issue after I finished the v2
based on btuart driver. By the way, I've ported the btmon to my board,
these vendor commands/events reported via btmon looks like below shown
up

> HCI Event: Unknown (0xe4) plen 5
 [hci0] 11.213593
02 01 01 00 00   .

> HCI Event: Unknown (0xe4) plen 5
 [hci0] 11.214272
02 01 01 00 00   .

< HCI Command: Vendor (0x3f|0x006f) plen 5
 [hci0] 11.214318
01 07 01 00 04   .

> HCI Event: Unknown (0xe4) plen 5
 [hci0] 11.214438
02 07 01 00 00   .

< HCI Command: Vendor (0x3f|0x006f) plen 6
 [hci0] 13.229379
01 06 02 00 00 01..

> HCI Event: Unknown (0xe4) plen 5
 [hci0] 13.307729
02 06 01 00 00   .


> > BTW, mtk controller before mt7622, such as mt7623, its MTK vendor
> > command always go with completely specific format, not with hci format.
> 
> What does that mean? Do you have an example?
> 

what I meant is that these vendor commands and events applied on old
SoCs prior to MT7622 always use completely proprietary format rather
than any BT packet to setup the BT controller.

for example:
- vendor command 
01 06 02 00 00 01 

- vendor event
02 06 01 00 00

> Regards
> 
> Marcel
> 

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Sean Wang]

On Tue, 2018-05-08 at 13:18 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> > +

[ ... ]

> > 
> > I'm happy to do with btmon. just the environment with buildroot the BT
> > running on seems there's a missing support for btmon. I can start to use
> > btmon once I change the environment to Debian.
> > 
> >> So all the MTK vendor commands respond with a vendor event? Or are there 
> >> some that do the standard command status/complete handling?
> >> 
> > 
> > yes, mtk controller after mt7622 (included), its MTK vendors command
> > (opcode 0xfc6f) always respond with a vendor event id 0xe4. And they
> > don't do any standard status/complete handling.


> then we need to figure out where the __hci_cmd_sync_ev causes a problem. 
> Since normally that should just work for you.
> 

Okay. I will look into more about the issue after I finished the v2
based on btuart driver. By the way, I've ported the btmon to my board,
these vendor commands/events reported via btmon looks like below shown
up

> HCI Event: Unknown (0xe4) plen 5
 [hci0] 11.213593
02 01 01 00 00   .

> HCI Event: Unknown (0xe4) plen 5
 [hci0] 11.214272
02 01 01 00 00   .

< HCI Command: Vendor (0x3f|0x006f) plen 5
 [hci0] 11.214318
01 07 01 00 04   .

> HCI Event: Unknown (0xe4) plen 5
 [hci0] 11.214438
02 07 01 00 00   .

< HCI Command: Vendor (0x3f|0x006f) plen 6
 [hci0] 13.229379
01 06 02 00 00 01..

> HCI Event: Unknown (0xe4) plen 5
 [hci0] 13.307729
02 06 01 00 00   .


> > BTW, mtk controller before mt7622, such as mt7623, its MTK vendor
> > command always go with completely specific format, not with hci format.
> 
> What does that mean? Do you have an example?
> 

what I meant is that these vendor commands and events applied on old
SoCs prior to MT7622 always use completely proprietary format rather
than any BT packet to setup the BT controller.

for example:
- vendor command 
01 06 02 00 00 01 

- vendor event
02 06 01 00 00

> Regards
> 
> Marcel
> 

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann]

Hi Sean,

> +
> +static int mtk_wmt_cmd_sync(struct hci_uart *hu, u8 opcode, u8 flag, u16 
> plen,
> + const void *param)
> +{
> + struct mtk_bt_dev *btdev = hu->priv;
> + struct hci_command_hdr *hhdr;
> + struct hci_acl_hdr *ahdr;
> + struct mtk_wmt_hdr *whdr;
> + struct sk_buff *skb;
> + int ret = 0;
> +
> + init_completion(>wmt_cmd);
> +
> + skb = bt_skb_alloc(plen + MTK_WMT_CMD_SIZE, GFP_KERNEL);
> + if (!skb)
> + return -ENOMEM;
> +
> + /*
> +  * WMT data is carried in either ACL or HCI format with op code as
> +  * 0xfc6f and followed by a WMT header and its actual payload.
> +  */
 
 Please use net subsystem comment style.
 
> + switch (opcode) {
> + case MTK_WMT_PATCH_DWNLD:
> + ahdr = skb_put(skb, HCI_ACL_HDR_SIZE);
> + ahdr->handle = cpu_to_le16(0xfc6f);
> + ahdr->dlen   = cpu_to_le16(plen + MTK_WMT_HDR_SIZE);
> + break;
> + default:
> + hhdr = skb_put(skb, HCI_COMMAND_HDR_SIZE);
> + hhdr->opcode = cpu_to_le16(0xfc6f);
> + hhdr->plen = plen + MTK_WMT_HDR_SIZE;
> + break;
> + }
> +
> + hci_skb_pkt_type(skb) = opcode == MTK_WMT_PATCH_DWNLD ?
> + HCI_ACLDATA_PKT : HCI_COMMAND_PKT;
 
 Why not move that into the switch statement above.
 
> +
> + /* Start to build a WMT header and its actual payload. */
> + whdr = skb_put(skb, MTK_WMT_HDR_SIZE);
> + whdr->dir = 1;
> + whdr->op = opcode;
> + whdr->dlen = cpu_to_le16(plen + 1);
> + whdr->flag = flag;
> + skb_put_data(skb, param, plen);
> +
> + mtk_enqueue(hu, skb);
> + hci_uart_tx_wakeup(hu);
> +
> + /*
> +  * Waiting a WMT event response, while we must take care in case of
> +  * failures for the wait.
> +  */
> + ret = wait_for_completion_interruptible_timeout(>wmt_cmd, HZ);
> +
> + return ret > 0 ? 0 : ret < 0 ? ret : -ETIMEDOUT;
> +}
 
 All in all I am not convinced that this is super clean. I get that we need 
 something special for having this in the ACL data packets, but for the 
 standard HCI command I prefer that __hci_cmd_sync is used. I addition, it 
 seems that patch download is the only special case and that happens before 
 at the setup stage. So we could make things special for that. I need to 
 understand this a bit better. Can I get a btmon -w trace.log file from the 
 whole init procedure.
 
>>> 
>>> While i was trying to rewrite the driver based on btuart.c. you posted
>>> on RFC, I used __hci_cmd_sync_ev to replace such kinds of SoC specific
>>> hci command sending which I've done previously with mtk_wmt_cmd_sync.
>>> 
>>> However, eventually, I got a cmd_timer timeout whose message printed
>>> on console as "Bluetooth: hci0: command 0xfc6f tx timeout".
>>> 
>>> The mtk soc specific cmd/event I posted below, I dumped directly in
>>> driver, always uses cmd as opcode 0xfc6f, and its event id as 0xe4.
>>> 
>>> It appears to the event id is not standard and thus it cannot cancel the
>>> cmd timer when the special hci event is being handled. This way can we
>>> can still use __hci_cmd_sync api ?
>>> 
>>> [4.896200] hci tx: : 01 6f fc 05 01 07 01 00 04
>>> [4.904671] hci rx: : e4 05 02 07 01 00
>>> 00 
>>> [4.912859] Bluetooth: hci0 event 0xe4
>>> 
>>> 
>>> buildroot login: [6.914509] Bluetooth: hci0: command 0xfc6f tx
>>> timeout
>>> [6.919831] hci tx: : 01 6f fc 06 01 06 02 00 00
>>> 01.o
>>> [7.006631] hci rx: : e4 05 02 06 01 00
>>> 00 ...
>>> [7.014821] Bluetooth: hci0 event 0xe4
>> 
>> can you just start btmon before loading the module / driver? It makes it a 
>> lot easier since it will actually decode the basics for us. If there is a 
>> bug within __hci_cmd_sync_ev, then we are going to fix it.
>> 
> 
> I'm happy to do with btmon. just the environment with buildroot the BT
> running on seems there's a missing support for btmon. I can start to use
> btmon once I change the environment to Debian.
> 
>> So all the MTK vendor commands respond with a vendor event? Or are there 
>> some that do the standard command status/complete handling?
>> 
> 
> yes, mtk controller after mt7622 (included), its MTK vendors command
> (opcode 0xfc6f) always respond with a vendor event id 0xe4. And they
> don't do any standard status/complete handling.

then we need to figure out where the __hci_cmd_sync_ev causes a problem. Since 
normally that should just work for you.

> BTW, mtk controller before mt7622, such as mt7623, its MTK vendor
> command always go with completely specific format, not with hci format.

What does that mean? Do you have an example?

Regards

Marcel

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann]

Hi Sean,

> +
> +static int mtk_wmt_cmd_sync(struct hci_uart *hu, u8 opcode, u8 flag, u16 
> plen,
> + const void *param)
> +{
> + struct mtk_bt_dev *btdev = hu->priv;
> + struct hci_command_hdr *hhdr;
> + struct hci_acl_hdr *ahdr;
> + struct mtk_wmt_hdr *whdr;
> + struct sk_buff *skb;
> + int ret = 0;
> +
> + init_completion(>wmt_cmd);
> +
> + skb = bt_skb_alloc(plen + MTK_WMT_CMD_SIZE, GFP_KERNEL);
> + if (!skb)
> + return -ENOMEM;
> +
> + /*
> +  * WMT data is carried in either ACL or HCI format with op code as
> +  * 0xfc6f and followed by a WMT header and its actual payload.
> +  */
 
 Please use net subsystem comment style.
 
> + switch (opcode) {
> + case MTK_WMT_PATCH_DWNLD:
> + ahdr = skb_put(skb, HCI_ACL_HDR_SIZE);
> + ahdr->handle = cpu_to_le16(0xfc6f);
> + ahdr->dlen   = cpu_to_le16(plen + MTK_WMT_HDR_SIZE);
> + break;
> + default:
> + hhdr = skb_put(skb, HCI_COMMAND_HDR_SIZE);
> + hhdr->opcode = cpu_to_le16(0xfc6f);
> + hhdr->plen = plen + MTK_WMT_HDR_SIZE;
> + break;
> + }
> +
> + hci_skb_pkt_type(skb) = opcode == MTK_WMT_PATCH_DWNLD ?
> + HCI_ACLDATA_PKT : HCI_COMMAND_PKT;
 
 Why not move that into the switch statement above.
 
> +
> + /* Start to build a WMT header and its actual payload. */
> + whdr = skb_put(skb, MTK_WMT_HDR_SIZE);
> + whdr->dir = 1;
> + whdr->op = opcode;
> + whdr->dlen = cpu_to_le16(plen + 1);
> + whdr->flag = flag;
> + skb_put_data(skb, param, plen);
> +
> + mtk_enqueue(hu, skb);
> + hci_uart_tx_wakeup(hu);
> +
> + /*
> +  * Waiting a WMT event response, while we must take care in case of
> +  * failures for the wait.
> +  */
> + ret = wait_for_completion_interruptible_timeout(>wmt_cmd, HZ);
> +
> + return ret > 0 ? 0 : ret < 0 ? ret : -ETIMEDOUT;
> +}
 
 All in all I am not convinced that this is super clean. I get that we need 
 something special for having this in the ACL data packets, but for the 
 standard HCI command I prefer that __hci_cmd_sync is used. I addition, it 
 seems that patch download is the only special case and that happens before 
 at the setup stage. So we could make things special for that. I need to 
 understand this a bit better. Can I get a btmon -w trace.log file from the 
 whole init procedure.
 
>>> 
>>> While i was trying to rewrite the driver based on btuart.c. you posted
>>> on RFC, I used __hci_cmd_sync_ev to replace such kinds of SoC specific
>>> hci command sending which I've done previously with mtk_wmt_cmd_sync.
>>> 
>>> However, eventually, I got a cmd_timer timeout whose message printed
>>> on console as "Bluetooth: hci0: command 0xfc6f tx timeout".
>>> 
>>> The mtk soc specific cmd/event I posted below, I dumped directly in
>>> driver, always uses cmd as opcode 0xfc6f, and its event id as 0xe4.
>>> 
>>> It appears to the event id is not standard and thus it cannot cancel the
>>> cmd timer when the special hci event is being handled. This way can we
>>> can still use __hci_cmd_sync api ?
>>> 
>>> [4.896200] hci tx: : 01 6f fc 05 01 07 01 00 04
>>> [4.904671] hci rx: : e4 05 02 07 01 00
>>> 00 
>>> [4.912859] Bluetooth: hci0 event 0xe4
>>> 
>>> 
>>> buildroot login: [6.914509] Bluetooth: hci0: command 0xfc6f tx
>>> timeout
>>> [6.919831] hci tx: : 01 6f fc 06 01 06 02 00 00
>>> 01.o
>>> [7.006631] hci rx: : e4 05 02 06 01 00
>>> 00 ...
>>> [7.014821] Bluetooth: hci0 event 0xe4
>> 
>> can you just start btmon before loading the module / driver? It makes it a 
>> lot easier since it will actually decode the basics for us. If there is a 
>> bug within __hci_cmd_sync_ev, then we are going to fix it.
>> 
> 
> I'm happy to do with btmon. just the environment with buildroot the BT
> running on seems there's a missing support for btmon. I can start to use
> btmon once I change the environment to Debian.
> 
>> So all the MTK vendor commands respond with a vendor event? Or are there 
>> some that do the standard command status/complete handling?
>> 
> 
> yes, mtk controller after mt7622 (included), its MTK vendors command
> (opcode 0xfc6f) always respond with a vendor event id 0xe4. And they
> don't do any standard status/complete handling.

then we need to figure out where the __hci_cmd_sync_ev causes a problem. Since 
normally that should just work for you.

> BTW, mtk controller before mt7622, such as mt7623, its MTK vendor
> command always go with completely specific format, not with hci format.

What does that mean? Do you have an example?

Regards

Marcel

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Sean Wang]

Hi, Marcel

On Tue, 2018-05-08 at 09:27 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> >>> +
> >>> +static int mtk_wmt_cmd_sync(struct hci_uart *hu, u8 opcode, u8 flag, u16 
> >>> plen,
> >>> + const void *param)
> >>> +{
> >>> + struct mtk_bt_dev *btdev = hu->priv;
> >>> + struct hci_command_hdr *hhdr;
> >>> + struct hci_acl_hdr *ahdr;
> >>> + struct mtk_wmt_hdr *whdr;
> >>> + struct sk_buff *skb;
> >>> + int ret = 0;
> >>> +
> >>> + init_completion(>wmt_cmd);
> >>> +
> >>> + skb = bt_skb_alloc(plen + MTK_WMT_CMD_SIZE, GFP_KERNEL);
> >>> + if (!skb)
> >>> + return -ENOMEM;
> >>> +
> >>> + /*
> >>> +  * WMT data is carried in either ACL or HCI format with op code as
> >>> +  * 0xfc6f and followed by a WMT header and its actual payload.
> >>> +  */
> >> 
> >> Please use net subsystem comment style.
> >> 
> >>> + switch (opcode) {
> >>> + case MTK_WMT_PATCH_DWNLD:
> >>> + ahdr = skb_put(skb, HCI_ACL_HDR_SIZE);
> >>> + ahdr->handle = cpu_to_le16(0xfc6f);
> >>> + ahdr->dlen   = cpu_to_le16(plen + MTK_WMT_HDR_SIZE);
> >>> + break;
> >>> + default:
> >>> + hhdr = skb_put(skb, HCI_COMMAND_HDR_SIZE);
> >>> + hhdr->opcode = cpu_to_le16(0xfc6f);
> >>> + hhdr->plen = plen + MTK_WMT_HDR_SIZE;
> >>> + break;
> >>> + }
> >>> +
> >>> + hci_skb_pkt_type(skb) = opcode == MTK_WMT_PATCH_DWNLD ?
> >>> + HCI_ACLDATA_PKT : HCI_COMMAND_PKT;
> >> 
> >> Why not move that into the switch statement above.
> >> 
> >>> +
> >>> + /* Start to build a WMT header and its actual payload. */
> >>> + whdr = skb_put(skb, MTK_WMT_HDR_SIZE);
> >>> + whdr->dir = 1;
> >>> + whdr->op = opcode;
> >>> + whdr->dlen = cpu_to_le16(plen + 1);
> >>> + whdr->flag = flag;
> >>> + skb_put_data(skb, param, plen);
> >>> +
> >>> + mtk_enqueue(hu, skb);
> >>> + hci_uart_tx_wakeup(hu);
> >>> +
> >>> + /*
> >>> +  * Waiting a WMT event response, while we must take care in case of
> >>> +  * failures for the wait.
> >>> +  */
> >>> + ret = wait_for_completion_interruptible_timeout(>wmt_cmd, HZ);
> >>> +
> >>> + return ret > 0 ? 0 : ret < 0 ? ret : -ETIMEDOUT;
> >>> +}
> >> 
> >> All in all I am not convinced that this is super clean. I get that we need 
> >> something special for having this in the ACL data packets, but for the 
> >> standard HCI command I prefer that __hci_cmd_sync is used. I addition, it 
> >> seems that patch download is the only special case and that happens before 
> >> at the setup stage. So we could make things special for that. I need to 
> >> understand this a bit better. Can I get a btmon -w trace.log file from the 
> >> whole init procedure.
> >> 
> > 
> > While i was trying to rewrite the driver based on btuart.c. you posted
> > on RFC, I used __hci_cmd_sync_ev to replace such kinds of SoC specific
> > hci command sending which I've done previously with mtk_wmt_cmd_sync.
> > 
> > However, eventually, I got a cmd_timer timeout whose message printed
> > on console as "Bluetooth: hci0: command 0xfc6f tx timeout".
> > 
> > The mtk soc specific cmd/event I posted below, I dumped directly in
> > driver, always uses cmd as opcode 0xfc6f, and its event id as 0xe4.
> > 
> > It appears to the event id is not standard and thus it cannot cancel the
> > cmd timer when the special hci event is being handled. This way can we
> > can still use __hci_cmd_sync api ?
> > 
> > [4.896200] hci tx: : 01 6f fc 05 01 07 01 00 04
> > [4.904671] hci rx: : e4 05 02 07 01 00
> > 00 
> > [4.912859] Bluetooth: hci0 event 0xe4
> > 
> > 
> > buildroot login: [6.914509] Bluetooth: hci0: command 0xfc6f tx
> > timeout
> > [6.919831] hci tx: : 01 6f fc 06 01 06 02 00 00
> > 01.o
> > [7.006631] hci rx: : e4 05 02 06 01 00
> > 00 ...
> > [7.014821] Bluetooth: hci0 event 0xe4
> 
> can you just start btmon before loading the module / driver? It makes it a 
> lot easier since it will actually decode the basics for us. If there is a bug 
> within __hci_cmd_sync_ev, then we are going to fix it.
> 

I'm happy to do with btmon. just the environment with buildroot the BT
running on seems there's a missing support for btmon. I can start to use
btmon once I change the environment to Debian.

> So all the MTK vendor commands respond with a vendor event? Or are there some 
> that do the standard command status/complete handling?
> 

yes, mtk controller after mt7622 (included), its MTK vendors command
(opcode 0xfc6f) always respond with a vendor event id 0xe4. And they
don't do any standard status/complete handling.

BTW, mtk controller before mt7622, such as mt7623, its MTK vendor
command always go with completely specific format, not with hci format.

> Regards
> 
> Marcel
> 

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Sean Wang]

Hi, Marcel

On Tue, 2018-05-08 at 09:27 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> >>> +
> >>> +static int mtk_wmt_cmd_sync(struct hci_uart *hu, u8 opcode, u8 flag, u16 
> >>> plen,
> >>> + const void *param)
> >>> +{
> >>> + struct mtk_bt_dev *btdev = hu->priv;
> >>> + struct hci_command_hdr *hhdr;
> >>> + struct hci_acl_hdr *ahdr;
> >>> + struct mtk_wmt_hdr *whdr;
> >>> + struct sk_buff *skb;
> >>> + int ret = 0;
> >>> +
> >>> + init_completion(>wmt_cmd);
> >>> +
> >>> + skb = bt_skb_alloc(plen + MTK_WMT_CMD_SIZE, GFP_KERNEL);
> >>> + if (!skb)
> >>> + return -ENOMEM;
> >>> +
> >>> + /*
> >>> +  * WMT data is carried in either ACL or HCI format with op code as
> >>> +  * 0xfc6f and followed by a WMT header and its actual payload.
> >>> +  */
> >> 
> >> Please use net subsystem comment style.
> >> 
> >>> + switch (opcode) {
> >>> + case MTK_WMT_PATCH_DWNLD:
> >>> + ahdr = skb_put(skb, HCI_ACL_HDR_SIZE);
> >>> + ahdr->handle = cpu_to_le16(0xfc6f);
> >>> + ahdr->dlen   = cpu_to_le16(plen + MTK_WMT_HDR_SIZE);
> >>> + break;
> >>> + default:
> >>> + hhdr = skb_put(skb, HCI_COMMAND_HDR_SIZE);
> >>> + hhdr->opcode = cpu_to_le16(0xfc6f);
> >>> + hhdr->plen = plen + MTK_WMT_HDR_SIZE;
> >>> + break;
> >>> + }
> >>> +
> >>> + hci_skb_pkt_type(skb) = opcode == MTK_WMT_PATCH_DWNLD ?
> >>> + HCI_ACLDATA_PKT : HCI_COMMAND_PKT;
> >> 
> >> Why not move that into the switch statement above.
> >> 
> >>> +
> >>> + /* Start to build a WMT header and its actual payload. */
> >>> + whdr = skb_put(skb, MTK_WMT_HDR_SIZE);
> >>> + whdr->dir = 1;
> >>> + whdr->op = opcode;
> >>> + whdr->dlen = cpu_to_le16(plen + 1);
> >>> + whdr->flag = flag;
> >>> + skb_put_data(skb, param, plen);
> >>> +
> >>> + mtk_enqueue(hu, skb);
> >>> + hci_uart_tx_wakeup(hu);
> >>> +
> >>> + /*
> >>> +  * Waiting a WMT event response, while we must take care in case of
> >>> +  * failures for the wait.
> >>> +  */
> >>> + ret = wait_for_completion_interruptible_timeout(>wmt_cmd, HZ);
> >>> +
> >>> + return ret > 0 ? 0 : ret < 0 ? ret : -ETIMEDOUT;
> >>> +}
> >> 
> >> All in all I am not convinced that this is super clean. I get that we need 
> >> something special for having this in the ACL data packets, but for the 
> >> standard HCI command I prefer that __hci_cmd_sync is used. I addition, it 
> >> seems that patch download is the only special case and that happens before 
> >> at the setup stage. So we could make things special for that. I need to 
> >> understand this a bit better. Can I get a btmon -w trace.log file from the 
> >> whole init procedure.
> >> 
> > 
> > While i was trying to rewrite the driver based on btuart.c. you posted
> > on RFC, I used __hci_cmd_sync_ev to replace such kinds of SoC specific
> > hci command sending which I've done previously with mtk_wmt_cmd_sync.
> > 
> > However, eventually, I got a cmd_timer timeout whose message printed
> > on console as "Bluetooth: hci0: command 0xfc6f tx timeout".
> > 
> > The mtk soc specific cmd/event I posted below, I dumped directly in
> > driver, always uses cmd as opcode 0xfc6f, and its event id as 0xe4.
> > 
> > It appears to the event id is not standard and thus it cannot cancel the
> > cmd timer when the special hci event is being handled. This way can we
> > can still use __hci_cmd_sync api ?
> > 
> > [4.896200] hci tx: : 01 6f fc 05 01 07 01 00 04
> > [4.904671] hci rx: : e4 05 02 07 01 00
> > 00 
> > [4.912859] Bluetooth: hci0 event 0xe4
> > 
> > 
> > buildroot login: [6.914509] Bluetooth: hci0: command 0xfc6f tx
> > timeout
> > [6.919831] hci tx: : 01 6f fc 06 01 06 02 00 00
> > 01.o
> > [7.006631] hci rx: : e4 05 02 06 01 00
> > 00 ...
> > [7.014821] Bluetooth: hci0 event 0xe4
> 
> can you just start btmon before loading the module / driver? It makes it a 
> lot easier since it will actually decode the basics for us. If there is a bug 
> within __hci_cmd_sync_ev, then we are going to fix it.
> 

I'm happy to do with btmon. just the environment with buildroot the BT
running on seems there's a missing support for btmon. I can start to use
btmon once I change the environment to Debian.

> So all the MTK vendor commands respond with a vendor event? Or are there some 
> that do the standard command status/complete handling?
> 

yes, mtk controller after mt7622 (included), its MTK vendors command
(opcode 0xfc6f) always respond with a vendor event id 0xe4. And they
don't do any standard status/complete handling.

BTW, mtk controller before mt7622, such as mt7623, its MTK vendor
command always go with completely specific format, not with hci format.

> Regards
> 
> Marcel
> 

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann]

Hi Sean,

>>> +
>>> +static int mtk_wmt_cmd_sync(struct hci_uart *hu, u8 opcode, u8 flag, u16 
>>> plen,
>>> +   const void *param)
>>> +{
>>> +   struct mtk_bt_dev *btdev = hu->priv;
>>> +   struct hci_command_hdr *hhdr;
>>> +   struct hci_acl_hdr *ahdr;
>>> +   struct mtk_wmt_hdr *whdr;
>>> +   struct sk_buff *skb;
>>> +   int ret = 0;
>>> +
>>> +   init_completion(>wmt_cmd);
>>> +
>>> +   skb = bt_skb_alloc(plen + MTK_WMT_CMD_SIZE, GFP_KERNEL);
>>> +   if (!skb)
>>> +   return -ENOMEM;
>>> +
>>> +   /*
>>> +* WMT data is carried in either ACL or HCI format with op code as
>>> +* 0xfc6f and followed by a WMT header and its actual payload.
>>> +*/
>> 
>> Please use net subsystem comment style.
>> 
>>> +   switch (opcode) {
>>> +   case MTK_WMT_PATCH_DWNLD:
>>> +   ahdr = skb_put(skb, HCI_ACL_HDR_SIZE);
>>> +   ahdr->handle = cpu_to_le16(0xfc6f);
>>> +   ahdr->dlen   = cpu_to_le16(plen + MTK_WMT_HDR_SIZE);
>>> +   break;
>>> +   default:
>>> +   hhdr = skb_put(skb, HCI_COMMAND_HDR_SIZE);
>>> +   hhdr->opcode = cpu_to_le16(0xfc6f);
>>> +   hhdr->plen = plen + MTK_WMT_HDR_SIZE;
>>> +   break;
>>> +   }
>>> +
>>> +   hci_skb_pkt_type(skb) = opcode == MTK_WMT_PATCH_DWNLD ?
>>> +   HCI_ACLDATA_PKT : HCI_COMMAND_PKT;
>> 
>> Why not move that into the switch statement above.
>> 
>>> +
>>> +   /* Start to build a WMT header and its actual payload. */
>>> +   whdr = skb_put(skb, MTK_WMT_HDR_SIZE);
>>> +   whdr->dir = 1;
>>> +   whdr->op = opcode;
>>> +   whdr->dlen = cpu_to_le16(plen + 1);
>>> +   whdr->flag = flag;
>>> +   skb_put_data(skb, param, plen);
>>> +
>>> +   mtk_enqueue(hu, skb);
>>> +   hci_uart_tx_wakeup(hu);
>>> +
>>> +   /*
>>> +* Waiting a WMT event response, while we must take care in case of
>>> +* failures for the wait.
>>> +*/
>>> +   ret = wait_for_completion_interruptible_timeout(>wmt_cmd, HZ);
>>> +
>>> +   return ret > 0 ? 0 : ret < 0 ? ret : -ETIMEDOUT;
>>> +}
>> 
>> All in all I am not convinced that this is super clean. I get that we need 
>> something special for having this in the ACL data packets, but for the 
>> standard HCI command I prefer that __hci_cmd_sync is used. I addition, it 
>> seems that patch download is the only special case and that happens before 
>> at the setup stage. So we could make things special for that. I need to 
>> understand this a bit better. Can I get a btmon -w trace.log file from the 
>> whole init procedure.
>> 
> 
> While i was trying to rewrite the driver based on btuart.c. you posted
> on RFC, I used __hci_cmd_sync_ev to replace such kinds of SoC specific
> hci command sending which I've done previously with mtk_wmt_cmd_sync.
> 
> However, eventually, I got a cmd_timer timeout whose message printed
> on console as "Bluetooth: hci0: command 0xfc6f tx timeout".
> 
> The mtk soc specific cmd/event I posted below, I dumped directly in
> driver, always uses cmd as opcode 0xfc6f, and its event id as 0xe4.
> 
> It appears to the event id is not standard and thus it cannot cancel the
> cmd timer when the special hci event is being handled. This way can we
> can still use __hci_cmd_sync api ?
> 
> [4.896200] hci tx: : 01 6f fc 05 01 07 01 00 04
> [4.904671] hci rx: : e4 05 02 07 01 00
> 00 
> [4.912859] Bluetooth: hci0 event 0xe4
> 
> 
> buildroot login: [6.914509] Bluetooth: hci0: command 0xfc6f tx
> timeout
> [6.919831] hci tx: : 01 6f fc 06 01 06 02 00 00
> 01.o
> [7.006631] hci rx: : e4 05 02 06 01 00
> 00 ...
> [7.014821] Bluetooth: hci0 event 0xe4

can you just start btmon before loading the module / driver? It makes it a lot 
easier since it will actually decode the basics for us. If there is a bug 
within __hci_cmd_sync_ev, then we are going to fix it.

So all the MTK vendor commands respond with a vendor event? Or are there some 
that do the standard command status/complete handling?

Regards

Marcel

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann]

Hi Sean,

>>> +
>>> +static int mtk_wmt_cmd_sync(struct hci_uart *hu, u8 opcode, u8 flag, u16 
>>> plen,
>>> +   const void *param)
>>> +{
>>> +   struct mtk_bt_dev *btdev = hu->priv;
>>> +   struct hci_command_hdr *hhdr;
>>> +   struct hci_acl_hdr *ahdr;
>>> +   struct mtk_wmt_hdr *whdr;
>>> +   struct sk_buff *skb;
>>> +   int ret = 0;
>>> +
>>> +   init_completion(>wmt_cmd);
>>> +
>>> +   skb = bt_skb_alloc(plen + MTK_WMT_CMD_SIZE, GFP_KERNEL);
>>> +   if (!skb)
>>> +   return -ENOMEM;
>>> +
>>> +   /*
>>> +* WMT data is carried in either ACL or HCI format with op code as
>>> +* 0xfc6f and followed by a WMT header and its actual payload.
>>> +*/
>> 
>> Please use net subsystem comment style.
>> 
>>> +   switch (opcode) {
>>> +   case MTK_WMT_PATCH_DWNLD:
>>> +   ahdr = skb_put(skb, HCI_ACL_HDR_SIZE);
>>> +   ahdr->handle = cpu_to_le16(0xfc6f);
>>> +   ahdr->dlen   = cpu_to_le16(plen + MTK_WMT_HDR_SIZE);
>>> +   break;
>>> +   default:
>>> +   hhdr = skb_put(skb, HCI_COMMAND_HDR_SIZE);
>>> +   hhdr->opcode = cpu_to_le16(0xfc6f);
>>> +   hhdr->plen = plen + MTK_WMT_HDR_SIZE;
>>> +   break;
>>> +   }
>>> +
>>> +   hci_skb_pkt_type(skb) = opcode == MTK_WMT_PATCH_DWNLD ?
>>> +   HCI_ACLDATA_PKT : HCI_COMMAND_PKT;
>> 
>> Why not move that into the switch statement above.
>> 
>>> +
>>> +   /* Start to build a WMT header and its actual payload. */
>>> +   whdr = skb_put(skb, MTK_WMT_HDR_SIZE);
>>> +   whdr->dir = 1;
>>> +   whdr->op = opcode;
>>> +   whdr->dlen = cpu_to_le16(plen + 1);
>>> +   whdr->flag = flag;
>>> +   skb_put_data(skb, param, plen);
>>> +
>>> +   mtk_enqueue(hu, skb);
>>> +   hci_uart_tx_wakeup(hu);
>>> +
>>> +   /*
>>> +* Waiting a WMT event response, while we must take care in case of
>>> +* failures for the wait.
>>> +*/
>>> +   ret = wait_for_completion_interruptible_timeout(>wmt_cmd, HZ);
>>> +
>>> +   return ret > 0 ? 0 : ret < 0 ? ret : -ETIMEDOUT;
>>> +}
>> 
>> All in all I am not convinced that this is super clean. I get that we need 
>> something special for having this in the ACL data packets, but for the 
>> standard HCI command I prefer that __hci_cmd_sync is used. I addition, it 
>> seems that patch download is the only special case and that happens before 
>> at the setup stage. So we could make things special for that. I need to 
>> understand this a bit better. Can I get a btmon -w trace.log file from the 
>> whole init procedure.
>> 
> 
> While i was trying to rewrite the driver based on btuart.c. you posted
> on RFC, I used __hci_cmd_sync_ev to replace such kinds of SoC specific
> hci command sending which I've done previously with mtk_wmt_cmd_sync.
> 
> However, eventually, I got a cmd_timer timeout whose message printed
> on console as "Bluetooth: hci0: command 0xfc6f tx timeout".
> 
> The mtk soc specific cmd/event I posted below, I dumped directly in
> driver, always uses cmd as opcode 0xfc6f, and its event id as 0xe4.
> 
> It appears to the event id is not standard and thus it cannot cancel the
> cmd timer when the special hci event is being handled. This way can we
> can still use __hci_cmd_sync api ?
> 
> [4.896200] hci tx: : 01 6f fc 05 01 07 01 00 04
> [4.904671] hci rx: : e4 05 02 07 01 00
> 00 
> [4.912859] Bluetooth: hci0 event 0xe4
> 
> 
> buildroot login: [6.914509] Bluetooth: hci0: command 0xfc6f tx
> timeout
> [6.919831] hci tx: : 01 6f fc 06 01 06 02 00 00
> 01.o
> [7.006631] hci rx: : e4 05 02 06 01 00
> 00 ...
> [7.014821] Bluetooth: hci0 event 0xe4

can you just start btmon before loading the module / driver? It makes it a lot 
easier since it will actually decode the basics for us. If there is a bug 
within __hci_cmd_sync_ev, then we are going to fix it.

So all the MTK vendor commands respond with a vendor event? Or are there some 
that do the standard command status/complete handling?

Regards

Marcel

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Sean Wang]

Hi, Marcel

On Tue, 2018-04-03 at 12:27 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 

[ ... ]

> > +
> > +static int mtk_wmt_cmd_sync(struct hci_uart *hu, u8 opcode, u8 flag, u16 
> > plen,
> > +   const void *param)
> > +{
> > +   struct mtk_bt_dev *btdev = hu->priv;
> > +   struct hci_command_hdr *hhdr;
> > +   struct hci_acl_hdr *ahdr;
> > +   struct mtk_wmt_hdr *whdr;
> > +   struct sk_buff *skb;
> > +   int ret = 0;
> > +
> > +   init_completion(>wmt_cmd);
> > +
> > +   skb = bt_skb_alloc(plen + MTK_WMT_CMD_SIZE, GFP_KERNEL);
> > +   if (!skb)
> > +   return -ENOMEM;
> > +
> > +   /*
> > +* WMT data is carried in either ACL or HCI format with op code as
> > +* 0xfc6f and followed by a WMT header and its actual payload.
> > +*/
> 
> Please use net subsystem comment style.
> 
> > +   switch (opcode) {
> > +   case MTK_WMT_PATCH_DWNLD:
> > +   ahdr = skb_put(skb, HCI_ACL_HDR_SIZE);
> > +   ahdr->handle = cpu_to_le16(0xfc6f);
> > +   ahdr->dlen   = cpu_to_le16(plen + MTK_WMT_HDR_SIZE);
> > +   break;
> > +   default:
> > +   hhdr = skb_put(skb, HCI_COMMAND_HDR_SIZE);
> > +   hhdr->opcode = cpu_to_le16(0xfc6f);
> > +   hhdr->plen = plen + MTK_WMT_HDR_SIZE;
> > +   break;
> > +   }
> > +
> > +   hci_skb_pkt_type(skb) = opcode == MTK_WMT_PATCH_DWNLD ?
> > +   HCI_ACLDATA_PKT : HCI_COMMAND_PKT;
> 
> Why not move that into the switch statement above.
> 
> > +
> > +   /* Start to build a WMT header and its actual payload. */
> > +   whdr = skb_put(skb, MTK_WMT_HDR_SIZE);
> > +   whdr->dir = 1;
> > +   whdr->op = opcode;
> > +   whdr->dlen = cpu_to_le16(plen + 1);
> > +   whdr->flag = flag;
> > +   skb_put_data(skb, param, plen);
> > +
> > +   mtk_enqueue(hu, skb);
> > +   hci_uart_tx_wakeup(hu);
> > +
> > +   /*
> > +* Waiting a WMT event response, while we must take care in case of
> > +* failures for the wait.
> > +*/
> > +   ret = wait_for_completion_interruptible_timeout(>wmt_cmd, HZ);
> > +
> > +   return ret > 0 ? 0 : ret < 0 ? ret : -ETIMEDOUT;
> > +}
> 
> All in all I am not convinced that this is super clean. I get that we need 
> something special for having this in the ACL data packets, but for the 
> standard HCI command I prefer that __hci_cmd_sync is used. I addition, it 
> seems that patch download is the only special case and that happens before at 
> the setup stage. So we could make things special for that. I need to 
> understand this a bit better. Can I get a btmon -w trace.log file from the 
> whole init procedure.
> 

While i was trying to rewrite the driver based on btuart.c. you posted
on RFC, I used __hci_cmd_sync_ev to replace such kinds of SoC specific
hci command sending which I've done previously with mtk_wmt_cmd_sync.

However, eventually, I got a cmd_timer timeout whose message printed
on console as "Bluetooth: hci0: command 0xfc6f tx timeout".

The mtk soc specific cmd/event I posted below, I dumped directly in
driver, always uses cmd as opcode 0xfc6f, and its event id as 0xe4.

It appears to the event id is not standard and thus it cannot cancel the
cmd timer when the special hci event is being handled. This way can we
can still use __hci_cmd_sync api ?
 
[4.896200] hci tx: : 01 6f fc 05 01 07 01 00 04
[4.904671] hci rx: : e4 05 02 07 01 00
00 
[4.912859] Bluetooth: hci0 event 0xe4


buildroot login: [6.914509] Bluetooth: hci0: command 0xfc6f tx
timeout
[6.919831] hci tx: : 01 6f fc 06 01 06 02 00 00
01.o
[7.006631] hci rx: : e4 05 02 06 01 00
00 ...
[7.014821] Bluetooth: hci0 event 0xe4

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Sean Wang]

Hi, Marcel

On Tue, 2018-04-03 at 12:27 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 

[ ... ]

> > +
> > +static int mtk_wmt_cmd_sync(struct hci_uart *hu, u8 opcode, u8 flag, u16 
> > plen,
> > +   const void *param)
> > +{
> > +   struct mtk_bt_dev *btdev = hu->priv;
> > +   struct hci_command_hdr *hhdr;
> > +   struct hci_acl_hdr *ahdr;
> > +   struct mtk_wmt_hdr *whdr;
> > +   struct sk_buff *skb;
> > +   int ret = 0;
> > +
> > +   init_completion(>wmt_cmd);
> > +
> > +   skb = bt_skb_alloc(plen + MTK_WMT_CMD_SIZE, GFP_KERNEL);
> > +   if (!skb)
> > +   return -ENOMEM;
> > +
> > +   /*
> > +* WMT data is carried in either ACL or HCI format with op code as
> > +* 0xfc6f and followed by a WMT header and its actual payload.
> > +*/
> 
> Please use net subsystem comment style.
> 
> > +   switch (opcode) {
> > +   case MTK_WMT_PATCH_DWNLD:
> > +   ahdr = skb_put(skb, HCI_ACL_HDR_SIZE);
> > +   ahdr->handle = cpu_to_le16(0xfc6f);
> > +   ahdr->dlen   = cpu_to_le16(plen + MTK_WMT_HDR_SIZE);
> > +   break;
> > +   default:
> > +   hhdr = skb_put(skb, HCI_COMMAND_HDR_SIZE);
> > +   hhdr->opcode = cpu_to_le16(0xfc6f);
> > +   hhdr->plen = plen + MTK_WMT_HDR_SIZE;
> > +   break;
> > +   }
> > +
> > +   hci_skb_pkt_type(skb) = opcode == MTK_WMT_PATCH_DWNLD ?
> > +   HCI_ACLDATA_PKT : HCI_COMMAND_PKT;
> 
> Why not move that into the switch statement above.
> 
> > +
> > +   /* Start to build a WMT header and its actual payload. */
> > +   whdr = skb_put(skb, MTK_WMT_HDR_SIZE);
> > +   whdr->dir = 1;
> > +   whdr->op = opcode;
> > +   whdr->dlen = cpu_to_le16(plen + 1);
> > +   whdr->flag = flag;
> > +   skb_put_data(skb, param, plen);
> > +
> > +   mtk_enqueue(hu, skb);
> > +   hci_uart_tx_wakeup(hu);
> > +
> > +   /*
> > +* Waiting a WMT event response, while we must take care in case of
> > +* failures for the wait.
> > +*/
> > +   ret = wait_for_completion_interruptible_timeout(>wmt_cmd, HZ);
> > +
> > +   return ret > 0 ? 0 : ret < 0 ? ret : -ETIMEDOUT;
> > +}
> 
> All in all I am not convinced that this is super clean. I get that we need 
> something special for having this in the ACL data packets, but for the 
> standard HCI command I prefer that __hci_cmd_sync is used. I addition, it 
> seems that patch download is the only special case and that happens before at 
> the setup stage. So we could make things special for that. I need to 
> understand this a bit better. Can I get a btmon -w trace.log file from the 
> whole init procedure.
> 

While i was trying to rewrite the driver based on btuart.c. you posted
on RFC, I used __hci_cmd_sync_ev to replace such kinds of SoC specific
hci command sending which I've done previously with mtk_wmt_cmd_sync.

However, eventually, I got a cmd_timer timeout whose message printed
on console as "Bluetooth: hci0: command 0xfc6f tx timeout".

The mtk soc specific cmd/event I posted below, I dumped directly in
driver, always uses cmd as opcode 0xfc6f, and its event id as 0xe4.

It appears to the event id is not standard and thus it cannot cancel the
cmd timer when the special hci event is being handled. This way can we
can still use __hci_cmd_sync api ?
 
[4.896200] hci tx: : 01 6f fc 05 01 07 01 00 04
[4.904671] hci rx: : e4 05 02 07 01 00
00 
[4.912859] Bluetooth: hci0 event 0xe4


buildroot login: [6.914509] Bluetooth: hci0: command 0xfc6f tx
timeout
[6.919831] hci tx: : 01 6f fc 06 01 06 02 00 00
01.o
[7.006631] hci rx: : e4 05 02 06 01 00
00 ...
[7.014821] Bluetooth: hci0 event 0xe4

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann]

Hi Sean,

>>> This adds a driver for the MediaTek serial protocol based on H4 
>>> protocol,
>>> which can enable the built-in Bluetooth device inside MT7622 SoC.
>>> 
>>> Signed-off-by: Sean Wang 
>>> ---
>>> 
> 
> [... snip ...]
> 
>>> 
>>> where could I find the newest btuart.c (which seems cannot be found in
>>> 4.17 rc2)? It seems a time to rewrite the driver based on btuart.c
>> 
>> It is not merged yet. I posted RFC patches to the mailing list.
>> 
> 
> got it.
> 
>>> 
>>> the Bluetooth device can't survive in a power/down cycle and
>>> 
>>> * power on should be including
>>> - enable clk and power domain
>>> - download firmware through specific ACL command
>>> - send specific commands to configure bluetooth (Required to note that
>>> the steps should be after downloading firmware because the behavior for
>>> the command might be changed by the firmware)
>> 
>> Then this sounds like you need a quirk that runs setup() after every open() 
>> and not just after the first open(). You would be the first hardware that 
>> looses their firmware, but that is fine, I almost expect that at some point 
>> someone comes along and requires this. So just create a new 
>> HCI_QUIRK_NON_PERSISTENT_SETUP.
>> 
> 
> Yes, it should be good to have an option HCI_QUIRK_NON_PERSISTENT_SETUP
> to allow us to run setup() for every open(). 
> 
> When users are feeling unexpected thing happening on its device, they
> always have a habit trying to restart device from UI.
> 
> If close() -> open() can completely power reset a bluetooth device and
> then it can recover from any fatal error to the initial state as at
> boot. It's good for these problems specially hard to be reproduced and
> required to reboot the whole machine to save.
> 
> However, it would take a little longer time on open() since it takes
> extra time to make firmware download and reconfiguration.

if setup() is smart enough to skip the firmware download if it is already 
active, you can avoid these kind of things. It is up to your hardware. All the 
devices I have encountered so far only needed setup() once. Maybe some can be 
more power aggressive via GPIOs to full reset the hardware, but that could also 
be achieved via RFKILL. We need to see how this shapes up with your hardware. 
An alternative could be HCI_QUIRK_SETUP_AFTER_RFKILL and that means only the 
rfkill switch that brings down hciX interfaces will cause the full cycle.

My recommendation is to not worry too much here. You want to get the base logic 
done and then we deal with the rest. Maybe doing ->post_open() and 
->pre_close() is more useful since you want HCI transport to send a HCI command 
to complete transport activation and shutdown.

> 
>>> * power off should be including
>>> - send specific commands, such as to disable bluetooth
>> 
>> So try to put these in shutdown()
>> 
> 
> got it.
> 
>>> - disable power domain and clk
>> 
>> And do this in close().
>> 
> 
> got it.
> 
> 
>>> 
> 
>>> +
>>> +   return err;
> 
> 
> [ ... snip ]
> 
>>  .open   = mtk_open,
 
>> 
>> Go with a brand new btmtkuart.c driver. I really sounds you don’t want the 
>> hci_ldisc.c framework in your way. You want direct control over the core 
>> callbacks.
>> 
> 
> 1)
> 
> In fact. the device is working via a internal serial bus, rather than
> via uart for mt7622. so could i call it btmtkserial.c ?
> 
> Becasue mediatek indeed also have bluetooth over uart, if i called it
> btmtkserialc, the same code logic I think can be fit to all other
> devices using either uart or internal serial bus.

It is just a name so far. Start with btmtkuart.c for now. We will sort that 
naming out later. Mind you that as long as this is abstracted via serdev, it 
doesn’t really matter what it is in detail. It is just that Bluetooth called 
H:4 UART and H:5 3-Wire UART. These are the two UART protocol we are running. 
That you have extra serial bus framing is just some other detail. But picking 
the final driver name is something we worry about after detailed review.

> 
> 2)
> 
> Yes, i don't want hci_ldisc. so far i thought serdev version is enough,
> and i preferred that bluetotoh device don't depend on any utility on
> user space to launch.

serdev is the way to go. I will not accept any now drivers that require 
btattach.

> 
> 3) 
> 
> last question
> 
> if i have bluetooth over usb, the usb version bluetooth can reuse
> btmtkserial.c code?

We would most likely create a btmtk.c for shared code. Similar to btintel.c, 
btbcm.c etc. However lets do that in step two since it is harder to review.

Regards

Marcel

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann]

Hi Sean,

>>> This adds a driver for the MediaTek serial protocol based on H4 
>>> protocol,
>>> which can enable the built-in Bluetooth device inside MT7622 SoC.
>>> 
>>> Signed-off-by: Sean Wang 
>>> ---
>>> 
> 
> [... snip ...]
> 
>>> 
>>> where could I find the newest btuart.c (which seems cannot be found in
>>> 4.17 rc2)? It seems a time to rewrite the driver based on btuart.c
>> 
>> It is not merged yet. I posted RFC patches to the mailing list.
>> 
> 
> got it.
> 
>>> 
>>> the Bluetooth device can't survive in a power/down cycle and
>>> 
>>> * power on should be including
>>> - enable clk and power domain
>>> - download firmware through specific ACL command
>>> - send specific commands to configure bluetooth (Required to note that
>>> the steps should be after downloading firmware because the behavior for
>>> the command might be changed by the firmware)
>> 
>> Then this sounds like you need a quirk that runs setup() after every open() 
>> and not just after the first open(). You would be the first hardware that 
>> looses their firmware, but that is fine, I almost expect that at some point 
>> someone comes along and requires this. So just create a new 
>> HCI_QUIRK_NON_PERSISTENT_SETUP.
>> 
> 
> Yes, it should be good to have an option HCI_QUIRK_NON_PERSISTENT_SETUP
> to allow us to run setup() for every open(). 
> 
> When users are feeling unexpected thing happening on its device, they
> always have a habit trying to restart device from UI.
> 
> If close() -> open() can completely power reset a bluetooth device and
> then it can recover from any fatal error to the initial state as at
> boot. It's good for these problems specially hard to be reproduced and
> required to reboot the whole machine to save.
> 
> However, it would take a little longer time on open() since it takes
> extra time to make firmware download and reconfiguration.

if setup() is smart enough to skip the firmware download if it is already 
active, you can avoid these kind of things. It is up to your hardware. All the 
devices I have encountered so far only needed setup() once. Maybe some can be 
more power aggressive via GPIOs to full reset the hardware, but that could also 
be achieved via RFKILL. We need to see how this shapes up with your hardware. 
An alternative could be HCI_QUIRK_SETUP_AFTER_RFKILL and that means only the 
rfkill switch that brings down hciX interfaces will cause the full cycle.

My recommendation is to not worry too much here. You want to get the base logic 
done and then we deal with the rest. Maybe doing ->post_open() and 
->pre_close() is more useful since you want HCI transport to send a HCI command 
to complete transport activation and shutdown.

> 
>>> * power off should be including
>>> - send specific commands, such as to disable bluetooth
>> 
>> So try to put these in shutdown()
>> 
> 
> got it.
> 
>>> - disable power domain and clk
>> 
>> And do this in close().
>> 
> 
> got it.
> 
> 
>>> 
> 
>>> +
>>> +   return err;
> 
> 
> [ ... snip ]
> 
>>  .open   = mtk_open,
 
>> 
>> Go with a brand new btmtkuart.c driver. I really sounds you don’t want the 
>> hci_ldisc.c framework in your way. You want direct control over the core 
>> callbacks.
>> 
> 
> 1)
> 
> In fact. the device is working via a internal serial bus, rather than
> via uart for mt7622. so could i call it btmtkserial.c ?
> 
> Becasue mediatek indeed also have bluetooth over uart, if i called it
> btmtkserialc, the same code logic I think can be fit to all other
> devices using either uart or internal serial bus.

It is just a name so far. Start with btmtkuart.c for now. We will sort that 
naming out later. Mind you that as long as this is abstracted via serdev, it 
doesn’t really matter what it is in detail. It is just that Bluetooth called 
H:4 UART and H:5 3-Wire UART. These are the two UART protocol we are running. 
That you have extra serial bus framing is just some other detail. But picking 
the final driver name is something we worry about after detailed review.

> 
> 2)
> 
> Yes, i don't want hci_ldisc. so far i thought serdev version is enough,
> and i preferred that bluetotoh device don't depend on any utility on
> user space to launch.

serdev is the way to go. I will not accept any now drivers that require 
btattach.

> 
> 3) 
> 
> last question
> 
> if i have bluetooth over usb, the usb version bluetooth can reuse
> btmtkserial.c code?

We would most likely create a btmtk.c for shared code. Similar to btintel.c, 
btbcm.c etc. However lets do that in step two since it is harder to review.

Regards

Marcel

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Sean Wang]

On Fri, 2018-04-27 at 07:25 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> > This adds a driver for the MediaTek serial protocol based on H4 
> > protocol,
> > which can enable the built-in Bluetooth device inside MT7622 SoC.
> > 
> > Signed-off-by: Sean Wang 
> > ---
> > 

 [... snip ...]

> > 
> > where could I find the newest btuart.c (which seems cannot be found in
> > 4.17 rc2)? It seems a time to rewrite the driver based on btuart.c
> 
> It is not merged yet. I posted RFC patches to the mailing list.
> 

got it.

> > 
> > the Bluetooth device can't survive in a power/down cycle and
> > 
> > * power on should be including
> >  - enable clk and power domain
> >  - download firmware through specific ACL command
> >  - send specific commands to configure bluetooth (Required to note that
> > the steps should be after downloading firmware because the behavior for
> > the command might be changed by the firmware)
> 
> Then this sounds like you need a quirk that runs setup() after every open() 
> and not just after the first open(). You would be the first hardware that 
> looses their firmware, but that is fine, I almost expect that at some point 
> someone comes along and requires this. So just create a new 
> HCI_QUIRK_NON_PERSISTENT_SETUP.
> 

Yes, it should be good to have an option HCI_QUIRK_NON_PERSISTENT_SETUP
to allow us to run setup() for every open(). 

When users are feeling unexpected thing happening on its device, they
always have a habit trying to restart device from UI.

If close() -> open() can completely power reset a bluetooth device and
then it can recover from any fatal error to the initial state as at
boot. It's good for these problems specially hard to be reproduced and
required to reboot the whole machine to save.

However, it would take a little longer time on open() since it takes
extra time to make firmware download and reconfiguration.

> > * power off should be including
> >  - send specific commands, such as to disable bluetooth
> 
> So try to put these in shutdown()
> 

got it.

> >  - disable power domain and clk
> 
> And do this in close().
> 

got it.


> > 
> >>> 
> > +
> > +   return err;


[ ... snip ]

>   .open   = mtk_open,
> >> 
> 
> Go with a brand new btmtkuart.c driver. I really sounds you don’t want the 
> hci_ldisc.c framework in your way. You want direct control over the core 
> callbacks.
> 

1)

In fact. the device is working via a internal serial bus, rather than
via uart for mt7622. so could i call it btmtkserial.c ?

Becasue mediatek indeed also have bluetooth over uart, if i called it
btmtkserialc, the same code logic I think can be fit to all other
devices using either uart or internal serial bus.

2)

Yes, i don't want hci_ldisc. so far i thought serdev version is enough,
and i preferred that bluetotoh device don't depend on any utility on
user space to launch.


3) 

last question

if i have bluetooth over usb, the usb version bluetooth can reuse
btmtkserial.c code?


> Regards
> 
> Marcel
> 

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Sean Wang]

On Fri, 2018-04-27 at 07:25 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> > This adds a driver for the MediaTek serial protocol based on H4 
> > protocol,
> > which can enable the built-in Bluetooth device inside MT7622 SoC.
> > 
> > Signed-off-by: Sean Wang 
> > ---
> > 

 [... snip ...]

> > 
> > where could I find the newest btuart.c (which seems cannot be found in
> > 4.17 rc2)? It seems a time to rewrite the driver based on btuart.c
> 
> It is not merged yet. I posted RFC patches to the mailing list.
> 

got it.

> > 
> > the Bluetooth device can't survive in a power/down cycle and
> > 
> > * power on should be including
> >  - enable clk and power domain
> >  - download firmware through specific ACL command
> >  - send specific commands to configure bluetooth (Required to note that
> > the steps should be after downloading firmware because the behavior for
> > the command might be changed by the firmware)
> 
> Then this sounds like you need a quirk that runs setup() after every open() 
> and not just after the first open(). You would be the first hardware that 
> looses their firmware, but that is fine, I almost expect that at some point 
> someone comes along and requires this. So just create a new 
> HCI_QUIRK_NON_PERSISTENT_SETUP.
> 

Yes, it should be good to have an option HCI_QUIRK_NON_PERSISTENT_SETUP
to allow us to run setup() for every open(). 

When users are feeling unexpected thing happening on its device, they
always have a habit trying to restart device from UI.

If close() -> open() can completely power reset a bluetooth device and
then it can recover from any fatal error to the initial state as at
boot. It's good for these problems specially hard to be reproduced and
required to reboot the whole machine to save.

However, it would take a little longer time on open() since it takes
extra time to make firmware download and reconfiguration.

> > * power off should be including
> >  - send specific commands, such as to disable bluetooth
> 
> So try to put these in shutdown()
> 

got it.

> >  - disable power domain and clk
> 
> And do this in close().
> 

got it.


> > 
> >>> 
> > +
> > +   return err;


[ ... snip ]

>   .open   = mtk_open,
> >> 
> 
> Go with a brand new btmtkuart.c driver. I really sounds you don’t want the 
> hci_ldisc.c framework in your way. You want direct control over the core 
> callbacks.
> 

1)

In fact. the device is working via a internal serial bus, rather than
via uart for mt7622. so could i call it btmtkserial.c ?

Becasue mediatek indeed also have bluetooth over uart, if i called it
btmtkserialc, the same code logic I think can be fit to all other
devices using either uart or internal serial bus.

2)

Yes, i don't want hci_ldisc. so far i thought serdev version is enough,
and i preferred that bluetotoh device don't depend on any utility on
user space to launch.


3) 

last question

if i have bluetooth over usb, the usb version bluetooth can reuse
btmtkserial.c code?


> Regards
> 
> Marcel
> 

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann]

Hi Sean,

> This adds a driver for the MediaTek serial protocol based on H4 protocol,
> which can enable the built-in Bluetooth device inside MT7622 SoC.
> 
> Signed-off-by: Sean Wang 
> ---
> 
> [... snip ...]
> 
> +
> + /* Start to build a WMT header and its actual payload. */
> + whdr = skb_put(skb, MTK_WMT_HDR_SIZE);
> + whdr->dir = 1;
> + whdr->op = opcode;
> + whdr->dlen = cpu_to_le16(plen + 1);
> + whdr->flag = flag;
> + skb_put_data(skb, param, plen);
> +
> + mtk_enqueue(hu, skb);
> + hci_uart_tx_wakeup(hu);
> +
> + /*
> +  * Waiting a WMT event response, while we must take care in case of
> +  * failures for the wait.
> +  */
> + ret = wait_for_completion_interruptible_timeout(>wmt_cmd, HZ);
> +
> + return ret > 0 ? 0 : ret < 0 ? ret : -ETIMEDOUT;
> +}
 
 All in all I am not convinced that this is super clean. I get that we need 
 something special for having this in the ACL data packets, but for the 
 standard HCI command I prefer that __hci_cmd_sync is used. I addition, it 
 seems that patch download is the only special case and that happens before 
 at the setup stage. So we could make things special for that. I need to 
 understand this a bit better. Can I get a btmon -w trace.log file from the 
 whole init procedure.
 
 
>>> 
>>> I can try to use __hci_cmd_sync to send those chip-specific hci commands
>>> which only can be recognized by the chip, but no meaningful to bluez
>>> core.
>>> 
>>> Is btmon able to capture these cmd/evt between driver and device?
>>> 
>>> my question's caused by that mtk_wmt_cmd_sync and its events only happen
>>> between driver and device,  it doesn't go to core.
>> 
>> for Intel, Broadcom, Qualcomm, Realtek etc. we use __hci_cmd_sync() in the 
>> ->setup() callback and it will show up in btmon. For at least Intel and 
>> Broadcom, btmon will decode them as well. You can hint the manufacturer id 
>> out of band it will be told to btmon. And that is also how I want it. If it 
>> is using HCI as transport, I want it to go through the Bluetooth core.
>> 
> 
> understood. I will allow chip-specific HCI cmd/evt to go to bluetooth
> core.
> 
> +
> +static int mtk_setup_fw(struct hci_uart *hu)
> +{
> + struct mtk_bt_dev *btdev = hu->priv;
> + const struct firmware *fw;
> + struct device *dev;
> + const char *fwname;
> + const u8 *fw_ptr;
> + size_t fw_size;
> + int err, dlen;
> + u8 flag;
> +
> + dev = >serdev->dev;
> + fwname = FIRMWARE_MT7622;
> +
> + init_completion(>wmt_cmd);
> +
> + err = request_firmware(, fwname, dev);
> + if (err < 0) {
> + dev_err(dev, "%s: Failed to load firmware file (%d)",
> + hu->hdev->name, err);
> + return err;
> + }
> +
> + fw_ptr = fw->data;
> + fw_size = fw->size;
> +
> + /* The size of a patch header at least has 30 bytes. */
> + if (fw_size < 30)
> + return -EINVAL;
> +
> + while (fw_size > 0) {
> + dlen = min_t(int, 1000, fw_size);
> +
> + /* Tell deivice the position in sequence. */
> + flag = (fw_size - dlen <= 0) ? 3 :
> +(fw_size < fw->size) ? 2 : 1;
> +
> + err = mtk_wmt_cmd_sync(hu, 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 mtk_open(struct hci_uart *hu)
> +{
> + struct mtk_bt_dev *btdev = hu->priv;
> + struct device *dev;
> + int err = 0;
> +
> + dev = >serdev->dev;
> +
> + serdev_device_open(hu->serdev);
> + skb_queue_head_init(>txq);
> +
> + /* Setup the usage of H4. */
> + hu->alignment = 1;
> + hu->padding = 0;
> + mtk_stp_reset(btdev->sp);
> +
> + /* Enable the power domain and clock the device requires */
> + pm_runtime_enable(dev);
> + err = pm_runtime_get_sync(dev);
> + if (err < 0) {
> + pm_runtime_disable(dev);
> + return err;
> + }
> +
> + err = clk_prepare_enable(btdev->clk);
> + if (err < 0) {
> + pm_runtime_put_sync(dev);
> + pm_runtime_disable(dev);
> + }
> +
> + return err;
> +}
> +
> +static int mtk_close(struct hci_uart *hu)
> +{
> + struct mtk_bt_dev *btdev = hu->priv;
> + struct device *dev = >serdev->dev;
> + u8 param = 0x0;
> +
> + skb_queue_purge(>txq);
> + kfree_skb(btdev->rx_skb);
> +
> + /* Disable the device. */
> + mtk_wmt_cmd_sync(hu, MTK_WMT_FUNC_CTRL, 0x0, sizeof(param), );
 
 Wouldn’t this require a enable 

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann]

Hi Sean,

> This adds a driver for the MediaTek serial protocol based on H4 protocol,
> which can enable the built-in Bluetooth device inside MT7622 SoC.
> 
> Signed-off-by: Sean Wang 
> ---
> 
> [... snip ...]
> 
> +
> + /* Start to build a WMT header and its actual payload. */
> + whdr = skb_put(skb, MTK_WMT_HDR_SIZE);
> + whdr->dir = 1;
> + whdr->op = opcode;
> + whdr->dlen = cpu_to_le16(plen + 1);
> + whdr->flag = flag;
> + skb_put_data(skb, param, plen);
> +
> + mtk_enqueue(hu, skb);
> + hci_uart_tx_wakeup(hu);
> +
> + /*
> +  * Waiting a WMT event response, while we must take care in case of
> +  * failures for the wait.
> +  */
> + ret = wait_for_completion_interruptible_timeout(>wmt_cmd, HZ);
> +
> + return ret > 0 ? 0 : ret < 0 ? ret : -ETIMEDOUT;
> +}
 
 All in all I am not convinced that this is super clean. I get that we need 
 something special for having this in the ACL data packets, but for the 
 standard HCI command I prefer that __hci_cmd_sync is used. I addition, it 
 seems that patch download is the only special case and that happens before 
 at the setup stage. So we could make things special for that. I need to 
 understand this a bit better. Can I get a btmon -w trace.log file from the 
 whole init procedure.
 
 
>>> 
>>> I can try to use __hci_cmd_sync to send those chip-specific hci commands
>>> which only can be recognized by the chip, but no meaningful to bluez
>>> core.
>>> 
>>> Is btmon able to capture these cmd/evt between driver and device?
>>> 
>>> my question's caused by that mtk_wmt_cmd_sync and its events only happen
>>> between driver and device,  it doesn't go to core.
>> 
>> for Intel, Broadcom, Qualcomm, Realtek etc. we use __hci_cmd_sync() in the 
>> ->setup() callback and it will show up in btmon. For at least Intel and 
>> Broadcom, btmon will decode them as well. You can hint the manufacturer id 
>> out of band it will be told to btmon. And that is also how I want it. If it 
>> is using HCI as transport, I want it to go through the Bluetooth core.
>> 
> 
> understood. I will allow chip-specific HCI cmd/evt to go to bluetooth
> core.
> 
> +
> +static int mtk_setup_fw(struct hci_uart *hu)
> +{
> + struct mtk_bt_dev *btdev = hu->priv;
> + const struct firmware *fw;
> + struct device *dev;
> + const char *fwname;
> + const u8 *fw_ptr;
> + size_t fw_size;
> + int err, dlen;
> + u8 flag;
> +
> + dev = >serdev->dev;
> + fwname = FIRMWARE_MT7622;
> +
> + init_completion(>wmt_cmd);
> +
> + err = request_firmware(, fwname, dev);
> + if (err < 0) {
> + dev_err(dev, "%s: Failed to load firmware file (%d)",
> + hu->hdev->name, err);
> + return err;
> + }
> +
> + fw_ptr = fw->data;
> + fw_size = fw->size;
> +
> + /* The size of a patch header at least has 30 bytes. */
> + if (fw_size < 30)
> + return -EINVAL;
> +
> + while (fw_size > 0) {
> + dlen = min_t(int, 1000, fw_size);
> +
> + /* Tell deivice the position in sequence. */
> + flag = (fw_size - dlen <= 0) ? 3 :
> +(fw_size < fw->size) ? 2 : 1;
> +
> + err = mtk_wmt_cmd_sync(hu, 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 mtk_open(struct hci_uart *hu)
> +{
> + struct mtk_bt_dev *btdev = hu->priv;
> + struct device *dev;
> + int err = 0;
> +
> + dev = >serdev->dev;
> +
> + serdev_device_open(hu->serdev);
> + skb_queue_head_init(>txq);
> +
> + /* Setup the usage of H4. */
> + hu->alignment = 1;
> + hu->padding = 0;
> + mtk_stp_reset(btdev->sp);
> +
> + /* Enable the power domain and clock the device requires */
> + pm_runtime_enable(dev);
> + err = pm_runtime_get_sync(dev);
> + if (err < 0) {
> + pm_runtime_disable(dev);
> + return err;
> + }
> +
> + err = clk_prepare_enable(btdev->clk);
> + if (err < 0) {
> + pm_runtime_put_sync(dev);
> + pm_runtime_disable(dev);
> + }
> +
> + return err;
> +}
> +
> +static int mtk_close(struct hci_uart *hu)
> +{
> + struct mtk_bt_dev *btdev = hu->priv;
> + struct device *dev = >serdev->dev;
> + u8 param = 0x0;
> +
> + skb_queue_purge(>txq);
> + kfree_skb(btdev->rx_skb);
> +
> + /* Disable the device. */
> + mtk_wmt_cmd_sync(hu, MTK_WMT_FUNC_CTRL, 0x0, sizeof(param), );
 
 Wouldn’t this require a enable device in open callback?

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Sean Wang]

On Thu, 2018-04-26 at 11:47 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> >>> This adds a driver for the MediaTek serial protocol based on H4 protocol,
> >>> which can enable the built-in Bluetooth device inside MT7622 SoC.
> >>> 
> >>> Signed-off-by: Sean Wang 
> >>> ---

[... snip ...]

> >>> +
> >>> + /* Start to build a WMT header and its actual payload. */
> >>> + whdr = skb_put(skb, MTK_WMT_HDR_SIZE);
> >>> + whdr->dir = 1;
> >>> + whdr->op = opcode;
> >>> + whdr->dlen = cpu_to_le16(plen + 1);
> >>> + whdr->flag = flag;
> >>> + skb_put_data(skb, param, plen);
> >>> +
> >>> + mtk_enqueue(hu, skb);
> >>> + hci_uart_tx_wakeup(hu);
> >>> +
> >>> + /*
> >>> +  * Waiting a WMT event response, while we must take care in case of
> >>> +  * failures for the wait.
> >>> +  */
> >>> + ret = wait_for_completion_interruptible_timeout(>wmt_cmd, HZ);
> >>> +
> >>> + return ret > 0 ? 0 : ret < 0 ? ret : -ETIMEDOUT;
> >>> +}
> >> 
> >> All in all I am not convinced that this is super clean. I get that we need 
> >> something special for having this in the ACL data packets, but for the 
> >> standard HCI command I prefer that __hci_cmd_sync is used. I addition, it 
> >> seems that patch download is the only special case and that happens before 
> >> at the setup stage. So we could make things special for that. I need to 
> >> understand this a bit better. Can I get a btmon -w trace.log file from the 
> >> whole init procedure.
> >> 
> >> 
> > 
> > I can try to use __hci_cmd_sync to send those chip-specific hci commands
> > which only can be recognized by the chip, but no meaningful to bluez
> > core.
> > 
> > Is btmon able to capture these cmd/evt between driver and device?
> > 
> > my question's caused by that mtk_wmt_cmd_sync and its events only happen
> > between driver and device,  it doesn't go to core.
> 
> for Intel, Broadcom, Qualcomm, Realtek etc. we use __hci_cmd_sync() in the 
> ->setup() callback and it will show up in btmon. For at least Intel and 
> Broadcom, btmon will decode them as well. You can hint the manufacturer id 
> out of band it will be told to btmon. And that is also how I want it. If it 
> is using HCI as transport, I want it to go through the Bluetooth core.
> 

understood. I will allow chip-specific HCI cmd/evt to go to bluetooth
core.

> >>> +
> >>> +static int mtk_setup_fw(struct hci_uart *hu)
> >>> +{
> >>> + struct mtk_bt_dev *btdev = hu->priv;
> >>> + const struct firmware *fw;
> >>> + struct device *dev;
> >>> + const char *fwname;
> >>> + const u8 *fw_ptr;
> >>> + size_t fw_size;
> >>> + int err, dlen;
> >>> + u8 flag;
> >>> +
> >>> + dev = >serdev->dev;
> >>> + fwname = FIRMWARE_MT7622;
> >>> +
> >>> + init_completion(>wmt_cmd);
> >>> +
> >>> + err = request_firmware(, fwname, dev);
> >>> + if (err < 0) {
> >>> + dev_err(dev, "%s: Failed to load firmware file (%d)",
> >>> + hu->hdev->name, err);
> >>> + return err;
> >>> + }
> >>> +
> >>> + fw_ptr = fw->data;
> >>> + fw_size = fw->size;
> >>> +
> >>> + /* The size of a patch header at least has 30 bytes. */
> >>> + if (fw_size < 30)
> >>> + return -EINVAL;
> >>> +
> >>> + while (fw_size > 0) {
> >>> + dlen = min_t(int, 1000, fw_size);
> >>> +
> >>> + /* Tell deivice the position in sequence. */
> >>> + flag = (fw_size - dlen <= 0) ? 3 :
> >>> +(fw_size < fw->size) ? 2 : 1;
> >>> +
> >>> + err = mtk_wmt_cmd_sync(hu, 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 mtk_open(struct hci_uart *hu)
> >>> +{
> >>> + struct mtk_bt_dev *btdev = hu->priv;
> >>> + struct device *dev;
> >>> + int err = 0;
> >>> +
> >>> + dev = >serdev->dev;
> >>> +
> >>> + serdev_device_open(hu->serdev);
> >>> + skb_queue_head_init(>txq);
> >>> +
> >>> + /* Setup the usage of H4. */
> >>> + hu->alignment = 1;
> >>> + hu->padding = 0;
> >>> + mtk_stp_reset(btdev->sp);
> >>> +
> >>> + /* Enable the power domain and clock the device requires */
> >>> + pm_runtime_enable(dev);
> >>> + err = pm_runtime_get_sync(dev);
> >>> + if (err < 0) {
> >>> + pm_runtime_disable(dev);
> >>> + return err;
> >>> + }
> >>> +
> >>> + err = clk_prepare_enable(btdev->clk);
> >>> + if (err < 0) {
> >>> + pm_runtime_put_sync(dev);
> >>> + pm_runtime_disable(dev);
> >>> + }
> >>> +
> >>> + return err;
> >>> +}
> >>> +
> >>> +static int mtk_close(struct hci_uart *hu)
> >>> +{
> >>> + struct mtk_bt_dev *btdev = hu->priv;
> >>> + struct device *dev = >serdev->dev;
> >>> + u8 param = 0x0;
> >>> +
> >>> + skb_queue_purge(>txq);
> >>> + kfree_skb(btdev->rx_skb);
> >>> +
> >>> + /* Disable the device. */
> >>> + mtk_wmt_cmd_sync(hu, MTK_WMT_FUNC_CTRL, 0x0, sizeof(param), );
> >> 

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Sean Wang]

On Thu, 2018-04-26 at 11:47 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> >>> This adds a driver for the MediaTek serial protocol based on H4 protocol,
> >>> which can enable the built-in Bluetooth device inside MT7622 SoC.
> >>> 
> >>> Signed-off-by: Sean Wang 
> >>> ---

[... snip ...]

> >>> +
> >>> + /* Start to build a WMT header and its actual payload. */
> >>> + whdr = skb_put(skb, MTK_WMT_HDR_SIZE);
> >>> + whdr->dir = 1;
> >>> + whdr->op = opcode;
> >>> + whdr->dlen = cpu_to_le16(plen + 1);
> >>> + whdr->flag = flag;
> >>> + skb_put_data(skb, param, plen);
> >>> +
> >>> + mtk_enqueue(hu, skb);
> >>> + hci_uart_tx_wakeup(hu);
> >>> +
> >>> + /*
> >>> +  * Waiting a WMT event response, while we must take care in case of
> >>> +  * failures for the wait.
> >>> +  */
> >>> + ret = wait_for_completion_interruptible_timeout(>wmt_cmd, HZ);
> >>> +
> >>> + return ret > 0 ? 0 : ret < 0 ? ret : -ETIMEDOUT;
> >>> +}
> >> 
> >> All in all I am not convinced that this is super clean. I get that we need 
> >> something special for having this in the ACL data packets, but for the 
> >> standard HCI command I prefer that __hci_cmd_sync is used. I addition, it 
> >> seems that patch download is the only special case and that happens before 
> >> at the setup stage. So we could make things special for that. I need to 
> >> understand this a bit better. Can I get a btmon -w trace.log file from the 
> >> whole init procedure.
> >> 
> >> 
> > 
> > I can try to use __hci_cmd_sync to send those chip-specific hci commands
> > which only can be recognized by the chip, but no meaningful to bluez
> > core.
> > 
> > Is btmon able to capture these cmd/evt between driver and device?
> > 
> > my question's caused by that mtk_wmt_cmd_sync and its events only happen
> > between driver and device,  it doesn't go to core.
> 
> for Intel, Broadcom, Qualcomm, Realtek etc. we use __hci_cmd_sync() in the 
> ->setup() callback and it will show up in btmon. For at least Intel and 
> Broadcom, btmon will decode them as well. You can hint the manufacturer id 
> out of band it will be told to btmon. And that is also how I want it. If it 
> is using HCI as transport, I want it to go through the Bluetooth core.
> 

understood. I will allow chip-specific HCI cmd/evt to go to bluetooth
core.

> >>> +
> >>> +static int mtk_setup_fw(struct hci_uart *hu)
> >>> +{
> >>> + struct mtk_bt_dev *btdev = hu->priv;
> >>> + const struct firmware *fw;
> >>> + struct device *dev;
> >>> + const char *fwname;
> >>> + const u8 *fw_ptr;
> >>> + size_t fw_size;
> >>> + int err, dlen;
> >>> + u8 flag;
> >>> +
> >>> + dev = >serdev->dev;
> >>> + fwname = FIRMWARE_MT7622;
> >>> +
> >>> + init_completion(>wmt_cmd);
> >>> +
> >>> + err = request_firmware(, fwname, dev);
> >>> + if (err < 0) {
> >>> + dev_err(dev, "%s: Failed to load firmware file (%d)",
> >>> + hu->hdev->name, err);
> >>> + return err;
> >>> + }
> >>> +
> >>> + fw_ptr = fw->data;
> >>> + fw_size = fw->size;
> >>> +
> >>> + /* The size of a patch header at least has 30 bytes. */
> >>> + if (fw_size < 30)
> >>> + return -EINVAL;
> >>> +
> >>> + while (fw_size > 0) {
> >>> + dlen = min_t(int, 1000, fw_size);
> >>> +
> >>> + /* Tell deivice the position in sequence. */
> >>> + flag = (fw_size - dlen <= 0) ? 3 :
> >>> +(fw_size < fw->size) ? 2 : 1;
> >>> +
> >>> + err = mtk_wmt_cmd_sync(hu, 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 mtk_open(struct hci_uart *hu)
> >>> +{
> >>> + struct mtk_bt_dev *btdev = hu->priv;
> >>> + struct device *dev;
> >>> + int err = 0;
> >>> +
> >>> + dev = >serdev->dev;
> >>> +
> >>> + serdev_device_open(hu->serdev);
> >>> + skb_queue_head_init(>txq);
> >>> +
> >>> + /* Setup the usage of H4. */
> >>> + hu->alignment = 1;
> >>> + hu->padding = 0;
> >>> + mtk_stp_reset(btdev->sp);
> >>> +
> >>> + /* Enable the power domain and clock the device requires */
> >>> + pm_runtime_enable(dev);
> >>> + err = pm_runtime_get_sync(dev);
> >>> + if (err < 0) {
> >>> + pm_runtime_disable(dev);
> >>> + return err;
> >>> + }
> >>> +
> >>> + err = clk_prepare_enable(btdev->clk);
> >>> + if (err < 0) {
> >>> + pm_runtime_put_sync(dev);
> >>> + pm_runtime_disable(dev);
> >>> + }
> >>> +
> >>> + return err;
> >>> +}
> >>> +
> >>> +static int mtk_close(struct hci_uart *hu)
> >>> +{
> >>> + struct mtk_bt_dev *btdev = hu->priv;
> >>> + struct device *dev = >serdev->dev;
> >>> + u8 param = 0x0;
> >>> +
> >>> + skb_queue_purge(>txq);
> >>> + kfree_skb(btdev->rx_skb);
> >>> +
> >>> + /* Disable the device. */
> >>> + mtk_wmt_cmd_sync(hu, MTK_WMT_FUNC_CTRL, 0x0, sizeof(param), );
> >> 
> >> Wouldn’t this 

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann]

Hi Sean,

>>> This adds a driver for the MediaTek serial protocol based on H4 protocol,
>>> which can enable the built-in Bluetooth device inside MT7622 SoC.
>>> 
>>> Signed-off-by: Sean Wang 
>>> ---
>>> drivers/bluetooth/Kconfig|  12 +
>>> drivers/bluetooth/Makefile   |   1 +
>>> drivers/bluetooth/hci_mediatek.c | 499 
>>> +++
>>> drivers/bluetooth/hci_uart.h |   3 +-
>>> 4 files changed, 514 insertions(+), 1 deletion(-)
>>> create mode 100644 drivers/bluetooth/hci_mediatek.c
>>> 
>>> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
>>> index 010f5f5..851f430 100644
>>> --- a/drivers/bluetooth/Kconfig
>>> +++ b/drivers/bluetooth/Kconfig
>>> @@ -104,6 +104,18 @@ config BT_HCIUART_H4
>>> 
>>>   Say Y here to compile support for HCI UART (H4) protocol.
>>> 
>>> +config BT_HCIUART_MEDIATEK
>>> +   tristate "MediaTek protocol support"
>>> +   depends on BT_HCIUART_SERDEV
>>> +   select BT_HCIUART_H4
>>> +   help
>>> + The MediaTek protocol based on H4 enables patch firmware download and
>>> + configuration. This protocol is required for MediaTek Bluetooth
>>> + devices with a serial bus such as BTIF, which can be usually found on
>>> + various MediaTek SoCs.
>>> +
>>> + Say Y here to compile support for MediaTek protocol.
>>> +
>>> config BT_HCIUART_NOKIA
>>> tristate "UART Nokia H4+ protocol support"
>>> depends on BT_HCIUART
>>> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
>>> index ec16c55..db93c76 100644
>>> --- a/drivers/bluetooth/Makefile
>>> +++ b/drivers/bluetooth/Makefile
>>> @@ -43,5 +43,6 @@ hci_uart-$(CONFIG_BT_HCIUART_INTEL)   += hci_intel.o
>>> hci_uart-$(CONFIG_BT_HCIUART_BCM)   += hci_bcm.o
>>> hci_uart-$(CONFIG_BT_HCIUART_QCA)   += hci_qca.o
>>> hci_uart-$(CONFIG_BT_HCIUART_AG6XX) += hci_ag6xx.o
>>> +hci_uart-$(CONFIG_BT_HCIUART_MEDIATEK)  += hci_mediatek.o
>> 
>> we used when available the 3 letter short version of the manufacture. So 
>> this would be MTK and hci_mtk.o in this case. Not that I care that much.
>> 
> 
> sure, i can use hci_mtk.o instead of the long one.
> 
>>> hci_uart-$(CONFIG_BT_HCIUART_MRVL)  += hci_mrvl.o
>>> hci_uart-objs   := $(hci_uart-y)
>>> diff --git a/drivers/bluetooth/hci_mediatek.c 
>>> b/drivers/bluetooth/hci_mediatek.c
>>> new file mode 100644
>>> index 000..7ac1e7a
>>> --- /dev/null
>>> +++ b/drivers/bluetooth/hci_mediatek.c
>>> @@ -0,0 +1,499 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +// Copyright (c) 2018 MediaTek Inc.
>>> +
>>> +/*
>>> + * Bluetooth HCI Serial driver for MediaTek SoC
>>> + *
>>> + * Author: Sean Wang 
>>> + *
>>> + */
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#include "hci_uart.h"
>>> +
>>> +#define FIRMWARE_MT7622"mediatek/mt7622_patch_firmware.bin”
>> 
>> Has this firmware already been submitted to linux-firmware tree?
>> 
> 
> I didn't submit this firmware yet into linux-firmware. But I will do it
> eventually.
> 
> Currently, it still takes some time to tweak the firmware before make it
> release, so I let driver go first to see if the driver can get any idea
> for becoming better. 
> 
>>> +
>>> +#define MTK_STP_HDR_SIZE   4
>>> +#define MTK_STP_TLR_SIZE   2
>>> +#define MTK_WMT_HDR_SIZE   5
>>> +#define MTK_WMT_CMD_SIZE   (MTK_WMT_HDR_SIZE + MTK_STP_HDR_SIZE + \
>>> +MTK_STP_TLR_SIZE + HCI_ACL_HDR_SIZE)
>>> +
>>> +enum {
>>> +   MTK_WMT_PATCH_DWNLD = 0x1,
>>> +   MTK_WMT_FUNC_CTRL = 0x6,
>>> +   MTK_WMT_RST = 0x7
>>> +};
>>> +
>>> +struct mtk_stp_splitter {
>>> +   u8  pad[6];
>>> +   u8  cursor;
>>> +   u16 dlen;
>>> +};
>>> +
>>> +struct mtk_bt_dev {
>>> +   struct hci_uart hu;
>>> +   struct clk *clk;
>>> +   struct sk_buff *rx_skb;
>>> +   struct sk_buff_head txq;
>>> +   struct completion wmt_cmd;
>>> +   struct mtk_stp_splitter *sp;
>>> +};
>>> +
>>> +struct mtk_stp_hdr {
>>> +   __u8 prefix;
>>> +   __u8 dlen1:4;
>>> +   __u8 type:4;
>>> +   __u8 dlen2:8;
>>> +   __u8 cs;
>>> +} __packed;
>>> +
>>> +struct mtk_wmt_hdr {
>>> +   __u8dir;
>>> +   __u8op;
>>> +   __le16  dlen;
>>> +   __u8flag;
>>> +} __packed;
>>> +
>>> +static void mtk_stp_reset(struct mtk_stp_splitter *sp)
>>> +{
>>> +   sp->cursor = 2;
>>> +   sp->dlen = 0;
>>> +}
>>> +
>>> +static const unsigned char *
>>> +mtk_stp_split(struct device *dev, struct mtk_stp_splitter *sp,
>>> + 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 being consumed. */
>>> +   if (!sp->dlen && sp->cursor >= 6)
>>> +   sp->cursor = 0;
>>> +
>>> +   /* Filling pad until all STP info is obtained. */
>>> +   while (sp->cursor < 

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann]

Hi Sean,

>>> This adds a driver for the MediaTek serial protocol based on H4 protocol,
>>> which can enable the built-in Bluetooth device inside MT7622 SoC.
>>> 
>>> Signed-off-by: Sean Wang 
>>> ---
>>> drivers/bluetooth/Kconfig|  12 +
>>> drivers/bluetooth/Makefile   |   1 +
>>> drivers/bluetooth/hci_mediatek.c | 499 
>>> +++
>>> drivers/bluetooth/hci_uart.h |   3 +-
>>> 4 files changed, 514 insertions(+), 1 deletion(-)
>>> create mode 100644 drivers/bluetooth/hci_mediatek.c
>>> 
>>> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
>>> index 010f5f5..851f430 100644
>>> --- a/drivers/bluetooth/Kconfig
>>> +++ b/drivers/bluetooth/Kconfig
>>> @@ -104,6 +104,18 @@ config BT_HCIUART_H4
>>> 
>>>   Say Y here to compile support for HCI UART (H4) protocol.
>>> 
>>> +config BT_HCIUART_MEDIATEK
>>> +   tristate "MediaTek protocol support"
>>> +   depends on BT_HCIUART_SERDEV
>>> +   select BT_HCIUART_H4
>>> +   help
>>> + The MediaTek protocol based on H4 enables patch firmware download and
>>> + configuration. This protocol is required for MediaTek Bluetooth
>>> + devices with a serial bus such as BTIF, which can be usually found on
>>> + various MediaTek SoCs.
>>> +
>>> + Say Y here to compile support for MediaTek protocol.
>>> +
>>> config BT_HCIUART_NOKIA
>>> tristate "UART Nokia H4+ protocol support"
>>> depends on BT_HCIUART
>>> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
>>> index ec16c55..db93c76 100644
>>> --- a/drivers/bluetooth/Makefile
>>> +++ b/drivers/bluetooth/Makefile
>>> @@ -43,5 +43,6 @@ hci_uart-$(CONFIG_BT_HCIUART_INTEL)   += hci_intel.o
>>> hci_uart-$(CONFIG_BT_HCIUART_BCM)   += hci_bcm.o
>>> hci_uart-$(CONFIG_BT_HCIUART_QCA)   += hci_qca.o
>>> hci_uart-$(CONFIG_BT_HCIUART_AG6XX) += hci_ag6xx.o
>>> +hci_uart-$(CONFIG_BT_HCIUART_MEDIATEK)  += hci_mediatek.o
>> 
>> we used when available the 3 letter short version of the manufacture. So 
>> this would be MTK and hci_mtk.o in this case. Not that I care that much.
>> 
> 
> sure, i can use hci_mtk.o instead of the long one.
> 
>>> hci_uart-$(CONFIG_BT_HCIUART_MRVL)  += hci_mrvl.o
>>> hci_uart-objs   := $(hci_uart-y)
>>> diff --git a/drivers/bluetooth/hci_mediatek.c 
>>> b/drivers/bluetooth/hci_mediatek.c
>>> new file mode 100644
>>> index 000..7ac1e7a
>>> --- /dev/null
>>> +++ b/drivers/bluetooth/hci_mediatek.c
>>> @@ -0,0 +1,499 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +// Copyright (c) 2018 MediaTek Inc.
>>> +
>>> +/*
>>> + * Bluetooth HCI Serial driver for MediaTek SoC
>>> + *
>>> + * Author: Sean Wang 
>>> + *
>>> + */
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#include "hci_uart.h"
>>> +
>>> +#define FIRMWARE_MT7622"mediatek/mt7622_patch_firmware.bin”
>> 
>> Has this firmware already been submitted to linux-firmware tree?
>> 
> 
> I didn't submit this firmware yet into linux-firmware. But I will do it
> eventually.
> 
> Currently, it still takes some time to tweak the firmware before make it
> release, so I let driver go first to see if the driver can get any idea
> for becoming better. 
> 
>>> +
>>> +#define MTK_STP_HDR_SIZE   4
>>> +#define MTK_STP_TLR_SIZE   2
>>> +#define MTK_WMT_HDR_SIZE   5
>>> +#define MTK_WMT_CMD_SIZE   (MTK_WMT_HDR_SIZE + MTK_STP_HDR_SIZE + \
>>> +MTK_STP_TLR_SIZE + HCI_ACL_HDR_SIZE)
>>> +
>>> +enum {
>>> +   MTK_WMT_PATCH_DWNLD = 0x1,
>>> +   MTK_WMT_FUNC_CTRL = 0x6,
>>> +   MTK_WMT_RST = 0x7
>>> +};
>>> +
>>> +struct mtk_stp_splitter {
>>> +   u8  pad[6];
>>> +   u8  cursor;
>>> +   u16 dlen;
>>> +};
>>> +
>>> +struct mtk_bt_dev {
>>> +   struct hci_uart hu;
>>> +   struct clk *clk;
>>> +   struct sk_buff *rx_skb;
>>> +   struct sk_buff_head txq;
>>> +   struct completion wmt_cmd;
>>> +   struct mtk_stp_splitter *sp;
>>> +};
>>> +
>>> +struct mtk_stp_hdr {
>>> +   __u8 prefix;
>>> +   __u8 dlen1:4;
>>> +   __u8 type:4;
>>> +   __u8 dlen2:8;
>>> +   __u8 cs;
>>> +} __packed;
>>> +
>>> +struct mtk_wmt_hdr {
>>> +   __u8dir;
>>> +   __u8op;
>>> +   __le16  dlen;
>>> +   __u8flag;
>>> +} __packed;
>>> +
>>> +static void mtk_stp_reset(struct mtk_stp_splitter *sp)
>>> +{
>>> +   sp->cursor = 2;
>>> +   sp->dlen = 0;
>>> +}
>>> +
>>> +static const unsigned char *
>>> +mtk_stp_split(struct device *dev, struct mtk_stp_splitter *sp,
>>> + 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 being consumed. */
>>> +   if (!sp->dlen && sp->cursor >= 6)
>>> +   sp->cursor = 0;
>>> +
>>> +   /* Filling pad until all STP info is obtained. */
>>> +   while (sp->cursor < 6 && count > 0) {
>>> +   

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Sean Wang]

On Tue, 2018-04-03 at 12:27 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> > This adds a driver for the MediaTek serial protocol based on H4 protocol,
> > which can enable the built-in Bluetooth device inside MT7622 SoC.
> > 
> > Signed-off-by: Sean Wang 
> > ---
> > drivers/bluetooth/Kconfig|  12 +
> > drivers/bluetooth/Makefile   |   1 +
> > drivers/bluetooth/hci_mediatek.c | 499 
> > +++
> > drivers/bluetooth/hci_uart.h |   3 +-
> > 4 files changed, 514 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/bluetooth/hci_mediatek.c
> > 
> > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > index 010f5f5..851f430 100644
> > --- a/drivers/bluetooth/Kconfig
> > +++ b/drivers/bluetooth/Kconfig
> > @@ -104,6 +104,18 @@ config BT_HCIUART_H4
> > 
> >   Say Y here to compile support for HCI UART (H4) protocol.
> > 
> > +config BT_HCIUART_MEDIATEK
> > +   tristate "MediaTek protocol support"
> > +   depends on BT_HCIUART_SERDEV
> > +   select BT_HCIUART_H4
> > +   help
> > + The MediaTek protocol based on H4 enables patch firmware download and
> > + configuration. This protocol is required for MediaTek Bluetooth
> > + devices with a serial bus such as BTIF, which can be usually found on
> > + various MediaTek SoCs.
> > +
> > + Say Y here to compile support for MediaTek protocol.
> > +
> > config BT_HCIUART_NOKIA
> > tristate "UART Nokia H4+ protocol support"
> > depends on BT_HCIUART
> > diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> > index ec16c55..db93c76 100644
> > --- a/drivers/bluetooth/Makefile
> > +++ b/drivers/bluetooth/Makefile
> > @@ -43,5 +43,6 @@ hci_uart-$(CONFIG_BT_HCIUART_INTEL)   += hci_intel.o
> > hci_uart-$(CONFIG_BT_HCIUART_BCM)   += hci_bcm.o
> > hci_uart-$(CONFIG_BT_HCIUART_QCA)   += hci_qca.o
> > hci_uart-$(CONFIG_BT_HCIUART_AG6XX) += hci_ag6xx.o
> > +hci_uart-$(CONFIG_BT_HCIUART_MEDIATEK)  += hci_mediatek.o
> 
> we used when available the 3 letter short version of the manufacture. So this 
> would be MTK and hci_mtk.o in this case. Not that I care that much.
> 

sure, i can use hci_mtk.o instead of the long one.

> > hci_uart-$(CONFIG_BT_HCIUART_MRVL)  += hci_mrvl.o
> > hci_uart-objs   := $(hci_uart-y)
> > diff --git a/drivers/bluetooth/hci_mediatek.c 
> > b/drivers/bluetooth/hci_mediatek.c
> > new file mode 100644
> > index 000..7ac1e7a
> > --- /dev/null
> > +++ b/drivers/bluetooth/hci_mediatek.c
> > @@ -0,0 +1,499 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2018 MediaTek Inc.
> > +
> > +/*
> > + * Bluetooth HCI Serial driver for MediaTek SoC
> > + *
> > + * Author: Sean Wang 
> > + *
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include "hci_uart.h"
> > +
> > +#define FIRMWARE_MT7622"mediatek/mt7622_patch_firmware.bin”
> 
> Has this firmware already been submitted to linux-firmware tree?
> 

I didn't submit this firmware yet into linux-firmware. But I will do it
eventually.

Currently, it still takes some time to tweak the firmware before make it
release, so I let driver go first to see if the driver can get any idea
for becoming better. 

> > +
> > +#define MTK_STP_HDR_SIZE   4
> > +#define MTK_STP_TLR_SIZE   2
> > +#define MTK_WMT_HDR_SIZE   5
> > +#define MTK_WMT_CMD_SIZE   (MTK_WMT_HDR_SIZE + MTK_STP_HDR_SIZE + \
> > +MTK_STP_TLR_SIZE + HCI_ACL_HDR_SIZE)
> > +
> > +enum {
> > +   MTK_WMT_PATCH_DWNLD = 0x1,
> > +   MTK_WMT_FUNC_CTRL = 0x6,
> > +   MTK_WMT_RST = 0x7
> > +};
> > +
> > +struct mtk_stp_splitter {
> > +   u8  pad[6];
> > +   u8  cursor;
> > +   u16 dlen;
> > +};
> > +
> > +struct mtk_bt_dev {
> > +   struct hci_uart hu;
> > +   struct clk *clk;
> > +   struct sk_buff *rx_skb;
> > +   struct sk_buff_head txq;
> > +   struct completion wmt_cmd;
> > +   struct mtk_stp_splitter *sp;
> > +};
> > +
> > +struct mtk_stp_hdr {
> > +   __u8 prefix;
> > +   __u8 dlen1:4;
> > +   __u8 type:4;
> > +   __u8 dlen2:8;
> > +   __u8 cs;
> > +} __packed;
> > +
> > +struct mtk_wmt_hdr {
> > +   __u8dir;
> > +   __u8op;
> > +   __le16  dlen;
> > +   __u8flag;
> > +} __packed;
> > +
> > +static void mtk_stp_reset(struct mtk_stp_splitter *sp)
> > +{
> > +   sp->cursor = 2;
> > +   sp->dlen = 0;
> > +}
> > +
> > +static const unsigned char *
> > +mtk_stp_split(struct device *dev, struct mtk_stp_splitter *sp,
> > + 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 being consumed. */
> > +   if (!sp->dlen && sp->cursor >= 6)
> > +   sp->cursor = 0;
> > +
> > +   /* Filling pad until all STP info is 

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Sean Wang]

On Tue, 2018-04-03 at 12:27 +0200, Marcel Holtmann wrote:
> Hi Sean,
> 
> > This adds a driver for the MediaTek serial protocol based on H4 protocol,
> > which can enable the built-in Bluetooth device inside MT7622 SoC.
> > 
> > Signed-off-by: Sean Wang 
> > ---
> > drivers/bluetooth/Kconfig|  12 +
> > drivers/bluetooth/Makefile   |   1 +
> > drivers/bluetooth/hci_mediatek.c | 499 
> > +++
> > drivers/bluetooth/hci_uart.h |   3 +-
> > 4 files changed, 514 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/bluetooth/hci_mediatek.c
> > 
> > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> > index 010f5f5..851f430 100644
> > --- a/drivers/bluetooth/Kconfig
> > +++ b/drivers/bluetooth/Kconfig
> > @@ -104,6 +104,18 @@ config BT_HCIUART_H4
> > 
> >   Say Y here to compile support for HCI UART (H4) protocol.
> > 
> > +config BT_HCIUART_MEDIATEK
> > +   tristate "MediaTek protocol support"
> > +   depends on BT_HCIUART_SERDEV
> > +   select BT_HCIUART_H4
> > +   help
> > + The MediaTek protocol based on H4 enables patch firmware download and
> > + configuration. This protocol is required for MediaTek Bluetooth
> > + devices with a serial bus such as BTIF, which can be usually found on
> > + various MediaTek SoCs.
> > +
> > + Say Y here to compile support for MediaTek protocol.
> > +
> > config BT_HCIUART_NOKIA
> > tristate "UART Nokia H4+ protocol support"
> > depends on BT_HCIUART
> > diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> > index ec16c55..db93c76 100644
> > --- a/drivers/bluetooth/Makefile
> > +++ b/drivers/bluetooth/Makefile
> > @@ -43,5 +43,6 @@ hci_uart-$(CONFIG_BT_HCIUART_INTEL)   += hci_intel.o
> > hci_uart-$(CONFIG_BT_HCIUART_BCM)   += hci_bcm.o
> > hci_uart-$(CONFIG_BT_HCIUART_QCA)   += hci_qca.o
> > hci_uart-$(CONFIG_BT_HCIUART_AG6XX) += hci_ag6xx.o
> > +hci_uart-$(CONFIG_BT_HCIUART_MEDIATEK)  += hci_mediatek.o
> 
> we used when available the 3 letter short version of the manufacture. So this 
> would be MTK and hci_mtk.o in this case. Not that I care that much.
> 

sure, i can use hci_mtk.o instead of the long one.

> > hci_uart-$(CONFIG_BT_HCIUART_MRVL)  += hci_mrvl.o
> > hci_uart-objs   := $(hci_uart-y)
> > diff --git a/drivers/bluetooth/hci_mediatek.c 
> > b/drivers/bluetooth/hci_mediatek.c
> > new file mode 100644
> > index 000..7ac1e7a
> > --- /dev/null
> > +++ b/drivers/bluetooth/hci_mediatek.c
> > @@ -0,0 +1,499 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2018 MediaTek Inc.
> > +
> > +/*
> > + * Bluetooth HCI Serial driver for MediaTek SoC
> > + *
> > + * Author: Sean Wang 
> > + *
> > + */
> > +
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +#include 
> > +
> > +#include "hci_uart.h"
> > +
> > +#define FIRMWARE_MT7622"mediatek/mt7622_patch_firmware.bin”
> 
> Has this firmware already been submitted to linux-firmware tree?
> 

I didn't submit this firmware yet into linux-firmware. But I will do it
eventually.

Currently, it still takes some time to tweak the firmware before make it
release, so I let driver go first to see if the driver can get any idea
for becoming better. 

> > +
> > +#define MTK_STP_HDR_SIZE   4
> > +#define MTK_STP_TLR_SIZE   2
> > +#define MTK_WMT_HDR_SIZE   5
> > +#define MTK_WMT_CMD_SIZE   (MTK_WMT_HDR_SIZE + MTK_STP_HDR_SIZE + \
> > +MTK_STP_TLR_SIZE + HCI_ACL_HDR_SIZE)
> > +
> > +enum {
> > +   MTK_WMT_PATCH_DWNLD = 0x1,
> > +   MTK_WMT_FUNC_CTRL = 0x6,
> > +   MTK_WMT_RST = 0x7
> > +};
> > +
> > +struct mtk_stp_splitter {
> > +   u8  pad[6];
> > +   u8  cursor;
> > +   u16 dlen;
> > +};
> > +
> > +struct mtk_bt_dev {
> > +   struct hci_uart hu;
> > +   struct clk *clk;
> > +   struct sk_buff *rx_skb;
> > +   struct sk_buff_head txq;
> > +   struct completion wmt_cmd;
> > +   struct mtk_stp_splitter *sp;
> > +};
> > +
> > +struct mtk_stp_hdr {
> > +   __u8 prefix;
> > +   __u8 dlen1:4;
> > +   __u8 type:4;
> > +   __u8 dlen2:8;
> > +   __u8 cs;
> > +} __packed;
> > +
> > +struct mtk_wmt_hdr {
> > +   __u8dir;
> > +   __u8op;
> > +   __le16  dlen;
> > +   __u8flag;
> > +} __packed;
> > +
> > +static void mtk_stp_reset(struct mtk_stp_splitter *sp)
> > +{
> > +   sp->cursor = 2;
> > +   sp->dlen = 0;
> > +}
> > +
> > +static const unsigned char *
> > +mtk_stp_split(struct device *dev, struct mtk_stp_splitter *sp,
> > + 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 being consumed. */
> > +   if (!sp->dlen && sp->cursor >= 6)
> > +   sp->cursor = 0;
> > +
> > +   /* Filling pad until all STP info is obtained. */
> > +   while (sp->cursor < 6 && count > 0) 

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[kbuild test robot]

Hi Sean,

I love your patch! Perhaps something to improve:

[auto build test WARNING on next-20180329]
[also build test WARNING on v4.16]
[cannot apply to linus/master bluetooth/master bluetooth-next/master v4.16 
v4.16-rc7 v4.16-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/sean-wang-mediatek-com/add-support-for-Bluetooth-on-MT7622-SoC/20180403-160035
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/bluetooth/hci_mediatek.c:322:5: sparse: symbol 'mtk_recv_frame' was 
>> not declared. Should it be static?
>> drivers/bluetooth/hci_mediatek.c:415:57: sparse: Using plain integer as NULL 
>> pointer

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[kbuild test robot]

Hi Sean,

I love your patch! Perhaps something to improve:

[auto build test WARNING on next-20180329]
[also build test WARNING on v4.16]
[cannot apply to linus/master bluetooth/master bluetooth-next/master v4.16 
v4.16-rc7 v4.16-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/sean-wang-mediatek-com/add-support-for-Bluetooth-on-MT7622-SoC/20180403-160035
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/bluetooth/hci_mediatek.c:322:5: sparse: symbol 'mtk_recv_frame' was 
>> not declared. Should it be static?
>> drivers/bluetooth/hci_mediatek.c:415:57: sparse: Using plain integer as NULL 
>> pointer

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann]

Hi Sean,

> This adds a driver for the MediaTek serial protocol based on H4 protocol,
> which can enable the built-in Bluetooth device inside MT7622 SoC.
> 
> Signed-off-by: Sean Wang 
> ---
> drivers/bluetooth/Kconfig|  12 +
> drivers/bluetooth/Makefile   |   1 +
> drivers/bluetooth/hci_mediatek.c | 499 +++
> drivers/bluetooth/hci_uart.h |   3 +-
> 4 files changed, 514 insertions(+), 1 deletion(-)
> create mode 100644 drivers/bluetooth/hci_mediatek.c
> 
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 010f5f5..851f430 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -104,6 +104,18 @@ config BT_HCIUART_H4
> 
> Say Y here to compile support for HCI UART (H4) protocol.
> 
> +config BT_HCIUART_MEDIATEK
> + tristate "MediaTek protocol support"
> + depends on BT_HCIUART_SERDEV
> + select BT_HCIUART_H4
> + help
> +   The MediaTek protocol based on H4 enables patch firmware download and
> +   configuration. This protocol is required for MediaTek Bluetooth
> +   devices with a serial bus such as BTIF, which can be usually found on
> +   various MediaTek SoCs.
> +
> +   Say Y here to compile support for MediaTek protocol.
> +
> config BT_HCIUART_NOKIA
>   tristate "UART Nokia H4+ protocol support"
>   depends on BT_HCIUART
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> index ec16c55..db93c76 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -43,5 +43,6 @@ hci_uart-$(CONFIG_BT_HCIUART_INTEL) += hci_intel.o
> hci_uart-$(CONFIG_BT_HCIUART_BCM) += hci_bcm.o
> hci_uart-$(CONFIG_BT_HCIUART_QCA) += hci_qca.o
> hci_uart-$(CONFIG_BT_HCIUART_AG6XX)   += hci_ag6xx.o
> +hci_uart-$(CONFIG_BT_HCIUART_MEDIATEK)  += hci_mediatek.o

we used when available the 3 letter short version of the manufacture. So this 
would be MTK and hci_mtk.o in this case. Not that I care that much.

> hci_uart-$(CONFIG_BT_HCIUART_MRVL)+= hci_mrvl.o
> hci_uart-objs := $(hci_uart-y)
> diff --git a/drivers/bluetooth/hci_mediatek.c 
> b/drivers/bluetooth/hci_mediatek.c
> new file mode 100644
> index 000..7ac1e7a
> --- /dev/null
> +++ b/drivers/bluetooth/hci_mediatek.c
> @@ -0,0 +1,499 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2018 MediaTek Inc.
> +
> +/*
> + * Bluetooth HCI Serial driver for MediaTek SoC
> + *
> + * Author: Sean Wang 
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "hci_uart.h"
> +
> +#define FIRMWARE_MT7622  "mediatek/mt7622_patch_firmware.bin”

Has this firmware already been submitted to linux-firmware tree?

> +
> +#define MTK_STP_HDR_SIZE 4
> +#define MTK_STP_TLR_SIZE 2
> +#define MTK_WMT_HDR_SIZE 5
> +#define MTK_WMT_CMD_SIZE (MTK_WMT_HDR_SIZE + MTK_STP_HDR_SIZE + \
> +  MTK_STP_TLR_SIZE + HCI_ACL_HDR_SIZE)
> +
> +enum {
> + MTK_WMT_PATCH_DWNLD = 0x1,
> + MTK_WMT_FUNC_CTRL = 0x6,
> + MTK_WMT_RST = 0x7
> +};
> +
> +struct mtk_stp_splitter {
> + u8  pad[6];
> + u8  cursor;
> + u16 dlen;
> +};
> +
> +struct mtk_bt_dev {
> + struct hci_uart hu;
> + struct clk *clk;
> + struct sk_buff *rx_skb;
> + struct sk_buff_head txq;
> + struct completion wmt_cmd;
> + struct mtk_stp_splitter *sp;
> +};
> +
> +struct mtk_stp_hdr {
> + __u8 prefix;
> + __u8 dlen1:4;
> + __u8 type:4;
> + __u8 dlen2:8;
> + __u8 cs;
> +} __packed;
> +
> +struct mtk_wmt_hdr {
> + __u8dir;
> + __u8op;
> + __le16  dlen;
> + __u8flag;
> +} __packed;
> +
> +static void mtk_stp_reset(struct mtk_stp_splitter *sp)
> +{
> + sp->cursor = 2;
> + sp->dlen = 0;
> +}
> +
> +static const unsigned char *
> +mtk_stp_split(struct device *dev, struct mtk_stp_splitter *sp,
> +   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 being consumed. */
> + if (!sp->dlen && sp->cursor >= 6)
> + sp->cursor = 0;
> +
> + /* Filling pad until all STP info is obtained. */
> + while (sp->cursor < 6 && count > 0) {
> + sp->pad[sp->cursor] = *data;
> + sp->cursor++;
> + data++;
> + count--;
> + }
> +
> + /* Retrieve STP info and have a sanity check. */
> + if (!sp->dlen && sp->cursor >= 6) {
> + shdr = (struct mtk_stp_hdr *)>pad[2];
> + sp->dlen = shdr->dlen1 << 8 | shdr->dlen2;
> +
> + /* Resync STP when unexpected data is being read. */
> + if (shdr->prefix != 0x80 || sp->dlen > 2048) {
> + dev_err(dev, 

Re: [PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[Marcel Holtmann]

Hi Sean,

> This adds a driver for the MediaTek serial protocol based on H4 protocol,
> which can enable the built-in Bluetooth device inside MT7622 SoC.
> 
> Signed-off-by: Sean Wang 
> ---
> drivers/bluetooth/Kconfig|  12 +
> drivers/bluetooth/Makefile   |   1 +
> drivers/bluetooth/hci_mediatek.c | 499 +++
> drivers/bluetooth/hci_uart.h |   3 +-
> 4 files changed, 514 insertions(+), 1 deletion(-)
> create mode 100644 drivers/bluetooth/hci_mediatek.c
> 
> diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
> index 010f5f5..851f430 100644
> --- a/drivers/bluetooth/Kconfig
> +++ b/drivers/bluetooth/Kconfig
> @@ -104,6 +104,18 @@ config BT_HCIUART_H4
> 
> Say Y here to compile support for HCI UART (H4) protocol.
> 
> +config BT_HCIUART_MEDIATEK
> + tristate "MediaTek protocol support"
> + depends on BT_HCIUART_SERDEV
> + select BT_HCIUART_H4
> + help
> +   The MediaTek protocol based on H4 enables patch firmware download and
> +   configuration. This protocol is required for MediaTek Bluetooth
> +   devices with a serial bus such as BTIF, which can be usually found on
> +   various MediaTek SoCs.
> +
> +   Say Y here to compile support for MediaTek protocol.
> +
> config BT_HCIUART_NOKIA
>   tristate "UART Nokia H4+ protocol support"
>   depends on BT_HCIUART
> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
> index ec16c55..db93c76 100644
> --- a/drivers/bluetooth/Makefile
> +++ b/drivers/bluetooth/Makefile
> @@ -43,5 +43,6 @@ hci_uart-$(CONFIG_BT_HCIUART_INTEL) += hci_intel.o
> hci_uart-$(CONFIG_BT_HCIUART_BCM) += hci_bcm.o
> hci_uart-$(CONFIG_BT_HCIUART_QCA) += hci_qca.o
> hci_uart-$(CONFIG_BT_HCIUART_AG6XX)   += hci_ag6xx.o
> +hci_uart-$(CONFIG_BT_HCIUART_MEDIATEK)  += hci_mediatek.o

we used when available the 3 letter short version of the manufacture. So this 
would be MTK and hci_mtk.o in this case. Not that I care that much.

> hci_uart-$(CONFIG_BT_HCIUART_MRVL)+= hci_mrvl.o
> hci_uart-objs := $(hci_uart-y)
> diff --git a/drivers/bluetooth/hci_mediatek.c 
> b/drivers/bluetooth/hci_mediatek.c
> new file mode 100644
> index 000..7ac1e7a
> --- /dev/null
> +++ b/drivers/bluetooth/hci_mediatek.c
> @@ -0,0 +1,499 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2018 MediaTek Inc.
> +
> +/*
> + * Bluetooth HCI Serial driver for MediaTek SoC
> + *
> + * Author: Sean Wang 
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include "hci_uart.h"
> +
> +#define FIRMWARE_MT7622  "mediatek/mt7622_patch_firmware.bin”

Has this firmware already been submitted to linux-firmware tree?

> +
> +#define MTK_STP_HDR_SIZE 4
> +#define MTK_STP_TLR_SIZE 2
> +#define MTK_WMT_HDR_SIZE 5
> +#define MTK_WMT_CMD_SIZE (MTK_WMT_HDR_SIZE + MTK_STP_HDR_SIZE + \
> +  MTK_STP_TLR_SIZE + HCI_ACL_HDR_SIZE)
> +
> +enum {
> + MTK_WMT_PATCH_DWNLD = 0x1,
> + MTK_WMT_FUNC_CTRL = 0x6,
> + MTK_WMT_RST = 0x7
> +};
> +
> +struct mtk_stp_splitter {
> + u8  pad[6];
> + u8  cursor;
> + u16 dlen;
> +};
> +
> +struct mtk_bt_dev {
> + struct hci_uart hu;
> + struct clk *clk;
> + struct sk_buff *rx_skb;
> + struct sk_buff_head txq;
> + struct completion wmt_cmd;
> + struct mtk_stp_splitter *sp;
> +};
> +
> +struct mtk_stp_hdr {
> + __u8 prefix;
> + __u8 dlen1:4;
> + __u8 type:4;
> + __u8 dlen2:8;
> + __u8 cs;
> +} __packed;
> +
> +struct mtk_wmt_hdr {
> + __u8dir;
> + __u8op;
> + __le16  dlen;
> + __u8flag;
> +} __packed;
> +
> +static void mtk_stp_reset(struct mtk_stp_splitter *sp)
> +{
> + sp->cursor = 2;
> + sp->dlen = 0;
> +}
> +
> +static const unsigned char *
> +mtk_stp_split(struct device *dev, struct mtk_stp_splitter *sp,
> +   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 being consumed. */
> + if (!sp->dlen && sp->cursor >= 6)
> + sp->cursor = 0;
> +
> + /* Filling pad until all STP info is obtained. */
> + while (sp->cursor < 6 && count > 0) {
> + sp->pad[sp->cursor] = *data;
> + sp->cursor++;
> + data++;
> + count--;
> + }
> +
> + /* Retrieve STP info and have a sanity check. */
> + if (!sp->dlen && sp->cursor >= 6) {
> + shdr = (struct mtk_stp_hdr *)>pad[2];
> + sp->dlen = shdr->dlen1 << 8 | shdr->dlen2;
> +
> + /* Resync STP when unexpected data is being read. */
> + if (shdr->prefix != 0x80 || sp->dlen > 2048) {
> + dev_err(dev, "stp format unexpect (%d, %d)",
> +

[PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[sean.wang]

From: Sean Wang 

This adds a driver for the MediaTek serial protocol based on H4 protocol,
which can enable the built-in Bluetooth device inside MT7622 SoC.

Signed-off-by: Sean Wang 
---
 drivers/bluetooth/Kconfig|  12 +
 drivers/bluetooth/Makefile   |   1 +
 drivers/bluetooth/hci_mediatek.c | 499 +++
 drivers/bluetooth/hci_uart.h |   3 +-
 4 files changed, 514 insertions(+), 1 deletion(-)
 create mode 100644 drivers/bluetooth/hci_mediatek.c

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index 010f5f5..851f430 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -104,6 +104,18 @@ config BT_HCIUART_H4
 
  Say Y here to compile support for HCI UART (H4) protocol.
 
+config BT_HCIUART_MEDIATEK
+   tristate "MediaTek protocol support"
+   depends on BT_HCIUART_SERDEV
+   select BT_HCIUART_H4
+   help
+ The MediaTek protocol based on H4 enables patch firmware download and
+ configuration. This protocol is required for MediaTek Bluetooth
+ devices with a serial bus such as BTIF, which can be usually found on
+ various MediaTek SoCs.
+
+ Say Y here to compile support for MediaTek protocol.
+
 config BT_HCIUART_NOKIA
tristate "UART Nokia H4+ protocol support"
depends on BT_HCIUART
diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
index ec16c55..db93c76 100644
--- a/drivers/bluetooth/Makefile
+++ b/drivers/bluetooth/Makefile
@@ -43,5 +43,6 @@ hci_uart-$(CONFIG_BT_HCIUART_INTEL)   += hci_intel.o
 hci_uart-$(CONFIG_BT_HCIUART_BCM)  += hci_bcm.o
 hci_uart-$(CONFIG_BT_HCIUART_QCA)  += hci_qca.o
 hci_uart-$(CONFIG_BT_HCIUART_AG6XX)+= hci_ag6xx.o
+hci_uart-$(CONFIG_BT_HCIUART_MEDIATEK)  += hci_mediatek.o
 hci_uart-$(CONFIG_BT_HCIUART_MRVL) += hci_mrvl.o
 hci_uart-objs  := $(hci_uart-y)
diff --git a/drivers/bluetooth/hci_mediatek.c b/drivers/bluetooth/hci_mediatek.c
new file mode 100644
index 000..7ac1e7a
--- /dev/null
+++ b/drivers/bluetooth/hci_mediatek.c
@@ -0,0 +1,499 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 MediaTek Inc.
+
+/*
+ * Bluetooth HCI Serial driver for MediaTek SoC
+ *
+ * Author: Sean Wang 
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "hci_uart.h"
+
+#define FIRMWARE_MT7622"mediatek/mt7622_patch_firmware.bin"
+
+#define MTK_STP_HDR_SIZE   4
+#define MTK_STP_TLR_SIZE   2
+#define MTK_WMT_HDR_SIZE   5
+#define MTK_WMT_CMD_SIZE   (MTK_WMT_HDR_SIZE + MTK_STP_HDR_SIZE + \
+MTK_STP_TLR_SIZE + HCI_ACL_HDR_SIZE)
+
+enum {
+   MTK_WMT_PATCH_DWNLD = 0x1,
+   MTK_WMT_FUNC_CTRL = 0x6,
+   MTK_WMT_RST = 0x7
+};
+
+struct mtk_stp_splitter {
+   u8  pad[6];
+   u8  cursor;
+   u16 dlen;
+};
+
+struct mtk_bt_dev {
+   struct hci_uart hu;
+   struct clk *clk;
+   struct sk_buff *rx_skb;
+   struct sk_buff_head txq;
+   struct completion wmt_cmd;
+   struct mtk_stp_splitter *sp;
+};
+
+struct mtk_stp_hdr {
+   __u8 prefix;
+   __u8 dlen1:4;
+   __u8 type:4;
+   __u8 dlen2:8;
+   __u8 cs;
+} __packed;
+
+struct mtk_wmt_hdr {
+   __u8dir;
+   __u8op;
+   __le16  dlen;
+   __u8flag;
+} __packed;
+
+static void mtk_stp_reset(struct mtk_stp_splitter *sp)
+{
+   sp->cursor = 2;
+   sp->dlen = 0;
+}
+
+static const unsigned char *
+mtk_stp_split(struct device *dev, struct mtk_stp_splitter *sp,
+ 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 being consumed. */
+   if (!sp->dlen && sp->cursor >= 6)
+   sp->cursor = 0;
+
+   /* Filling pad until all STP info is obtained. */
+   while (sp->cursor < 6 && count > 0) {
+   sp->pad[sp->cursor] = *data;
+   sp->cursor++;
+   data++;
+   count--;
+   }
+
+   /* Retrieve STP info and have a sanity check. */
+   if (!sp->dlen && sp->cursor >= 6) {
+   shdr = (struct mtk_stp_hdr *)>pad[2];
+   sp->dlen = shdr->dlen1 << 8 | shdr->dlen2;
+
+   /* Resync STP when unexpected data is being read. */
+   if (shdr->prefix != 0x80 || sp->dlen > 2048) {
+   dev_err(dev, "stp format unexpect (%d, %d)",
+   shdr->prefix, sp->dlen);
+   mtk_stp_reset(sp);
+   }
+   }
+
+   /* Directly leave 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 

[PATCH v1 6/7] Bluetooth: hci_mediatek: Add protocol support for MediaTek serial devices

[sean.wang]

From: Sean Wang 

This adds a driver for the MediaTek serial protocol based on H4 protocol,
which can enable the built-in Bluetooth device inside MT7622 SoC.

Signed-off-by: Sean Wang 
---
 drivers/bluetooth/Kconfig|  12 +
 drivers/bluetooth/Makefile   |   1 +
 drivers/bluetooth/hci_mediatek.c | 499 +++
 drivers/bluetooth/hci_uart.h |   3 +-
 4 files changed, 514 insertions(+), 1 deletion(-)
 create mode 100644 drivers/bluetooth/hci_mediatek.c

diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig
index 010f5f5..851f430 100644
--- a/drivers/bluetooth/Kconfig
+++ b/drivers/bluetooth/Kconfig
@@ -104,6 +104,18 @@ config BT_HCIUART_H4
 
  Say Y here to compile support for HCI UART (H4) protocol.
 
+config BT_HCIUART_MEDIATEK
+   tristate "MediaTek protocol support"
+   depends on BT_HCIUART_SERDEV
+   select BT_HCIUART_H4
+   help
+ The MediaTek protocol based on H4 enables patch firmware download and
+ configuration. This protocol is required for MediaTek Bluetooth
+ devices with a serial bus such as BTIF, which can be usually found on
+ various MediaTek SoCs.
+
+ Say Y here to compile support for MediaTek protocol.
+
 config BT_HCIUART_NOKIA
tristate "UART Nokia H4+ protocol support"
depends on BT_HCIUART
diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile
index ec16c55..db93c76 100644
--- a/drivers/bluetooth/Makefile
+++ b/drivers/bluetooth/Makefile
@@ -43,5 +43,6 @@ hci_uart-$(CONFIG_BT_HCIUART_INTEL)   += hci_intel.o
 hci_uart-$(CONFIG_BT_HCIUART_BCM)  += hci_bcm.o
 hci_uart-$(CONFIG_BT_HCIUART_QCA)  += hci_qca.o
 hci_uart-$(CONFIG_BT_HCIUART_AG6XX)+= hci_ag6xx.o
+hci_uart-$(CONFIG_BT_HCIUART_MEDIATEK)  += hci_mediatek.o
 hci_uart-$(CONFIG_BT_HCIUART_MRVL) += hci_mrvl.o
 hci_uart-objs  := $(hci_uart-y)
diff --git a/drivers/bluetooth/hci_mediatek.c b/drivers/bluetooth/hci_mediatek.c
new file mode 100644
index 000..7ac1e7a
--- /dev/null
+++ b/drivers/bluetooth/hci_mediatek.c
@@ -0,0 +1,499 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018 MediaTek Inc.
+
+/*
+ * Bluetooth HCI Serial driver for MediaTek SoC
+ *
+ * Author: Sean Wang 
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include "hci_uart.h"
+
+#define FIRMWARE_MT7622"mediatek/mt7622_patch_firmware.bin"
+
+#define MTK_STP_HDR_SIZE   4
+#define MTK_STP_TLR_SIZE   2
+#define MTK_WMT_HDR_SIZE   5
+#define MTK_WMT_CMD_SIZE   (MTK_WMT_HDR_SIZE + MTK_STP_HDR_SIZE + \
+MTK_STP_TLR_SIZE + HCI_ACL_HDR_SIZE)
+
+enum {
+   MTK_WMT_PATCH_DWNLD = 0x1,
+   MTK_WMT_FUNC_CTRL = 0x6,
+   MTK_WMT_RST = 0x7
+};
+
+struct mtk_stp_splitter {
+   u8  pad[6];
+   u8  cursor;
+   u16 dlen;
+};
+
+struct mtk_bt_dev {
+   struct hci_uart hu;
+   struct clk *clk;
+   struct sk_buff *rx_skb;
+   struct sk_buff_head txq;
+   struct completion wmt_cmd;
+   struct mtk_stp_splitter *sp;
+};
+
+struct mtk_stp_hdr {
+   __u8 prefix;
+   __u8 dlen1:4;
+   __u8 type:4;
+   __u8 dlen2:8;
+   __u8 cs;
+} __packed;
+
+struct mtk_wmt_hdr {
+   __u8dir;
+   __u8op;
+   __le16  dlen;
+   __u8flag;
+} __packed;
+
+static void mtk_stp_reset(struct mtk_stp_splitter *sp)
+{
+   sp->cursor = 2;
+   sp->dlen = 0;
+}
+
+static const unsigned char *
+mtk_stp_split(struct device *dev, struct mtk_stp_splitter *sp,
+ 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 being consumed. */
+   if (!sp->dlen && sp->cursor >= 6)
+   sp->cursor = 0;
+
+   /* Filling pad until all STP info is obtained. */
+   while (sp->cursor < 6 && count > 0) {
+   sp->pad[sp->cursor] = *data;
+   sp->cursor++;
+   data++;
+   count--;
+   }
+
+   /* Retrieve STP info and have a sanity check. */
+   if (!sp->dlen && sp->cursor >= 6) {
+   shdr = (struct mtk_stp_hdr *)>pad[2];
+   sp->dlen = shdr->dlen1 << 8 | shdr->dlen2;
+
+   /* Resync STP when unexpected data is being read. */
+   if (shdr->prefix != 0x80 || sp->dlen > 2048) {
+   dev_err(dev, "stp format unexpect (%d, %d)",
+   shdr->prefix, sp->dlen);
+   mtk_stp_reset(sp);
+   }
+   }
+
+   /* Directly leave 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, sp->dlen);
+   /*