| Message ID | 1443654180-29416-1-git-send-email-bjorn.andersson@sonymobile.com (mailing list archive) |
|---|---|
| State | Under Review, archived |
| Delegated to: | Andy Gross |
| Headers | show |
Hi Bjorn,
[auto build test results on v4.3-rc3 -- if it's inappropriate base, please ignore]
config: arm-allmodconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout a380238e176a2641aa74bac37f4e735fcec4d854
# save the attached .config to linux build tree
make.cross ARCH=arm
All error/warnings (new ones prefixed by >>):
>> drivers/bluetooth/hci_smd.c:186:33: error: array type has incomplete element type
static const struct qcom_smd_id smd_hci_acl_match[] = {
^
>> drivers/bluetooth/hci_smd.c:187:2: error: field name not in record or union initializer
{ .name = "APPS_RIVA_BT_ACL" },
^
drivers/bluetooth/hci_smd.c:187:2: error: (near initialization for 'smd_hci_acl_match')
drivers/bluetooth/hci_smd.c:191:33: error: array type has incomplete element type
static const struct qcom_smd_id smd_hci_cmd_match[] = {
^
drivers/bluetooth/hci_smd.c:192:2: error: field name not in record or union initializer
{ .name = "APPS_RIVA_BT_CMD" },
^
drivers/bluetooth/hci_smd.c:192:2: error: (near initialization for 'smd_hci_cmd_match')
>> drivers/bluetooth/hci_smd.c:200:2: error: unknown field 'smd_match_table' specified in initializer
.smd_match_table = smd_hci_acl_match,
^
>> drivers/bluetooth/hci_smd.c:200:2: warning: excess elements in struct initializer
>> drivers/bluetooth/hci_smd.c:200:2: warning: (near initialization for 'smd_hci_acl_driver')
drivers/bluetooth/hci_smd.c:211:2: error: unknown field 'smd_match_table' specified in initializer
.smd_match_table = smd_hci_cmd_match,
^
drivers/bluetooth/hci_smd.c:211:2: warning: excess elements in struct initializer
>> drivers/bluetooth/hci_smd.c:211:2: warning: (near initialization for 'smd_hci_cmd_driver')
In file included from drivers/bluetooth/hci_smd.c:14:0:
>> include/linux/module.h:128:27: error: redefinition of '__inittest'
static inline initcall_t __inittest(void) \
^
>> include/linux/device.h:1321:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^
>> include/linux/soc/qcom/smd.h:41:2: note: in expansion of macro 'module_driver'
module_driver(__smd_driver, qcom_smd_driver_register, \
^
>> drivers/bluetooth/hci_smd.c:219:1: note: in expansion of macro 'module_qcom_smd_driver'
module_qcom_smd_driver(smd_hci_cmd_driver);
^
include/linux/module.h:128:27: note: previous definition of '__inittest' was here
static inline initcall_t __inittest(void) \
^
>> include/linux/device.h:1321:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^
>> include/linux/soc/qcom/smd.h:41:2: note: in expansion of macro 'module_driver'
module_driver(__smd_driver, qcom_smd_driver_register, \
^
drivers/bluetooth/hci_smd.c:218:1: note: in expansion of macro 'module_qcom_smd_driver'
module_qcom_smd_driver(smd_hci_acl_driver);
^
>> include/linux/module.h:130:6: error: redefinition of 'init_module'
int init_module(void) __attribute__((alias(#initfn)));
^
>> include/linux/device.h:1321:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^
>> include/linux/soc/qcom/smd.h:41:2: note: in expansion of macro 'module_driver'
module_driver(__smd_driver, qcom_smd_driver_register, \
^
>> drivers/bluetooth/hci_smd.c:219:1: note: in expansion of macro 'module_qcom_smd_driver'
module_qcom_smd_driver(smd_hci_cmd_driver);
^
include/linux/module.h:130:6: note: previous definition of 'init_module' was here
int init_module(void) __attribute__((alias(#initfn)));
^
>> include/linux/device.h:1321:1: note: in expansion of macro 'module_init'
module_init(__driver##_init); \
^
>> include/linux/soc/qcom/smd.h:41:2: note: in expansion of macro 'module_driver'
module_driver(__smd_driver, qcom_smd_driver_register, \
^
drivers/bluetooth/hci_smd.c:218:1: note: in expansion of macro 'module_qcom_smd_driver'
module_qcom_smd_driver(smd_hci_acl_driver);
^
>> include/linux/module.h:134:27: error: redefinition of '__exittest'
static inline exitcall_t __exittest(void) \
^
>> include/linux/device.h:1326:1: note: in expansion of macro 'module_exit'
module_exit(__driver##_exit);
^
vim +186 drivers/bluetooth/hci_smd.c
180 static void smd_hci_cmd_remove(struct qcom_smd_device *sdev)
181 {
182 smd_hci.cmd_channel = NULL;
183 smd_hci_unregister();
184 }
185
> 186 static const struct qcom_smd_id smd_hci_acl_match[] = {
> 187 { .name = "APPS_RIVA_BT_ACL" },
188 {}
189 };
190
> 191 static const struct qcom_smd_id smd_hci_cmd_match[] = {
> 192 { .name = "APPS_RIVA_BT_CMD" },
193 {}
194 };
195
196 static struct qcom_smd_driver smd_hci_acl_driver = {
197 .probe = smd_hci_acl_probe,
198 .remove = smd_hci_acl_remove,
199 .callback = smd_hci_acl_callback,
> 200 .smd_match_table = smd_hci_acl_match,
201 .driver = {
202 .name = "qcom_smd_hci_acl",
203 .owner = THIS_MODULE,
204 },
205 };
206
207 static struct qcom_smd_driver smd_hci_cmd_driver = {
208 .probe = smd_hci_cmd_probe,
209 .remove = smd_hci_cmd_remove,
210 .callback = smd_hci_cmd_callback,
> 211 .smd_match_table = smd_hci_cmd_match,
212 .driver = {
213 .name = "qcom_smd_hci_cmd",
214 .owner = THIS_MODULE,
215 },
216 };
217
> 218 module_qcom_smd_driver(smd_hci_acl_driver);
> 219 module_qcom_smd_driver(smd_hci_cmd_driver);
220
221 MODULE_DESCRIPTION("Qualcomm SMD HCI driver");
222 MODULE_LICENSE("GPL v2");
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Hi Bjorn, > The Qualcomm WCNSS chip provides two SMD channels to the BT core; one > for command and one for event packets. This driver exposes the two > channels as a hci device. > > Signed-off-by: Bjorn Andersson <bjorn.andersson@sonymobile.com> > --- > drivers/bluetooth/Kconfig | 11 +++ > drivers/bluetooth/Makefile | 1 + > drivers/bluetooth/hci_smd.c | 222 ++++++++++++++++++++++++++++++++++++++++++++ > include/net/bluetooth/hci.h | 1 + > 4 files changed, 235 insertions(+) > create mode 100644 drivers/bluetooth/hci_smd.c > > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig > index 3d480d8c6111..1a2658f373b5 100644 > --- a/drivers/bluetooth/Kconfig > +++ b/drivers/bluetooth/Kconfig > @@ -62,6 +62,17 @@ config BT_HCIBTSDIO > Say Y here to compile support for Bluetooth SDIO devices into the > kernel or say M to compile it as module (btsdio). > > +config BT_HCISMD > + tristate "HCI Qualcomm SMD driver" > + depends on QCOM_SMD > + help > + Qualcomm SMD HCI driver. > + This driver is used to bridge HCI data onto the shared memory > + channels to the WCNSS core. > + > + Say Y here to compile support for HCI over Qualcomm SMD into the > + kernelor say M to compile as a module. > + > config BT_HCIUART > tristate "HCI UART driver" > depends on TTY > diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile > index 07c9cf381e5a..43c7dc8641ff 100644 > --- a/drivers/bluetooth/Makefile > +++ b/drivers/bluetooth/Makefile > @@ -14,6 +14,7 @@ obj-$(CONFIG_BT_HCIBTUART) += btuart_cs.o > > obj-$(CONFIG_BT_HCIBTUSB) += btusb.o > obj-$(CONFIG_BT_HCIBTSDIO) += btsdio.o > +obj-$(CONFIG_BT_HCISMD) += hci_smd.o I wonder if choosing a name like btqcomsmd.ko would not be better here. For now I am fine with keeping hci_smd.ko since there are other issues here to be fixed first. Especially the kbuild test robot complained loudly. > obj-$(CONFIG_BT_INTEL) += btintel.o > obj-$(CONFIG_BT_ATH3K) += ath3k.o > diff --git a/drivers/bluetooth/hci_smd.c b/drivers/bluetooth/hci_smd.c > new file mode 100644 > index 000000000000..e5748da2f902 > --- /dev/null > +++ b/drivers/bluetooth/hci_smd.c > @@ -0,0 +1,222 @@ > +/* > + * Copyright (c) 2015, Sony Mobile Communications Inc. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/module.h> > +#include <linux/slab.h> > +#include <linux/soc/qcom/smd.h> > +#include <net/bluetooth/bluetooth.h> > +#include <net/bluetooth/hci_core.h> > +#include <net/bluetooth/hci.h> The hci.h include is not needed. If you do, then you are doing something fishy. > + > +static struct { > + struct qcom_smd_channel *acl_channel; > + struct qcom_smd_channel *cmd_channel; > + > + struct hci_dev *hci; > +} smd_hci; A driver that only supports a single instance of a device and uses a global variable to do so is not a good idea. There should be no reason for this. Why is this done this way. > + > +static int smd_hci_recv(unsigned type, const void *data, size_t count) > +{ > + struct sk_buff *skb; > + void *buf; > + int ret; > + > + skb = bt_skb_alloc(count, GFP_ATOMIC); Is it required that this is GFP_ATOMIC or can this actually be GFP_KERNEL? > + if (!skb) > + return -ENOMEM; > + > + buf = skb_put(skb, count); > + memcpy_fromio(buf, data, count); Why is this a memcpy_fromio here? > + > + skb->dev = (void *)smd_hci.hci; This is no longer needed. > + bt_cb(skb)->pkt_type = type; > + skb_orphan(skb); > + > + ret = hci_recv_frame(smd_hci.hci, skb); > + if (ret < 0) > + kfree_skb(skb); This is a double kfree_skb here. The hci_recv_frame will consume the skb no matter what. > + > + return ret; > +} > + > +static int smd_hci_acl_callback(struct qcom_smd_device *qsdev, > + const void *data, > + size_t count) > +{ > + return smd_hci_recv(HCI_ACLDATA_PKT, data, count); > +} > + > +static int smd_hci_cmd_callback(struct qcom_smd_device *qsdev, > + const void *data, > + size_t count) > +{ > + return smd_hci_recv(HCI_EVENT_PKT, data, count); > +} > + > +static int smd_hci_send(struct hci_dev *hdev, struct sk_buff *skb) > +{ > + int ret; > + > + switch (bt_cb(skb)->pkt_type) { > + case HCI_ACLDATA_PKT: > + case HCI_SCODATA_PKT: > + ret = qcom_smd_send(smd_hci.acl_channel, skb->data, skb->len); > + break; > + case HCI_COMMAND_PKT: > + ret = qcom_smd_send(smd_hci.cmd_channel, skb->data, skb->len); > + break; > + default: > + ret = -ENODEV; > + break; > + } > + > + kfree_skb(skb); If you return an error, then the caller of hdev->send will free the skb. > + > + return ret; > +} > + > +static int smd_hci_open(struct hci_dev *hci) > +{ > + return 0; > +} > + > +static int smd_hci_close(struct hci_dev *hci) > +{ > + return 0; > +} These two need to at least handling the HCI_RUNNING flag setting and clearing like all the other drivers do. > + > +static int smd_hci_set_bdaddr(struct hci_dev *hci, > + const bdaddr_t *bdaddr) > +{ > + u8 buf[12]; > + > + buf[0] = 0x0b; > + buf[1] = 0xfc; > + buf[2] = 0x9; > + buf[3] = 0x1; > + buf[4] = 0x2; Use full 0x00 syntax here. > + buf[5] = sizeof(bdaddr_t); > + memcpy(buf + 6, bdaddr, sizeof(bdaddr_t)); We also need to be sure that this command only changes the BD_ADDR in RAM. It can not be persistent. When resetting / power cycle the controller it will fallback to its default. > + > + return qcom_smd_send(smd_hci.cmd_channel, buf, sizeof(buf)); Use __hci_cmd_sync since that is what is suppose to be used in this callback. See the other drivers on how this is done. I wonder if qca_set_bdaddr_rome is not actually the same command and you could just use that instead. > +} > + > +static int smd_hci_register(void) > +{ > + struct hci_dev *hci; > + int ret; > + > + if (smd_hci.hci) > + return 0; > + > + /* Wait for both channels to probe before registering */ > + if (!smd_hci.acl_channel || !smd_hci.cmd_channel) > + return 0; > + > + hci = hci_alloc_dev(); > + if (!hci) > + return -ENOMEM; > + > + hci->bus = HCI_SMD; > + hci->open = smd_hci_open; > + hci->close = smd_hci_close; > + hci->send = smd_hci_send; > + hci->set_bdaddr = smd_hci_set_bdaddr; > + > + ret = hci_register_dev(hci); > + if (ret < 0) { > + hci_free_dev(hci); > + return ret; > + } > + > + smd_hci.hci = hci; > + > + return 0; > +} > + > +static void smd_hci_unregister(void) > +{ > + /* Only unregister on the first remove call */ > + if (!smd_hci.hci) > + return; > + > + hci_unregister_dev(smd_hci.hci); > + hci_free_dev(smd_hci.hci); > + smd_hci.hci = NULL; > +} > + > +static int smd_hci_acl_probe(struct qcom_smd_device *sdev) > +{ > + smd_hci.acl_channel = sdev->channel; > + smd_hci_register(); > + > + return 0; > +} > + > +static int smd_hci_cmd_probe(struct qcom_smd_device *sdev) > +{ > + smd_hci.cmd_channel = sdev->channel; > + smd_hci_register(); > + > + return 0; > +} > + > +static void smd_hci_acl_remove(struct qcom_smd_device *sdev) > +{ > + smd_hci.acl_channel = NULL; > + smd_hci_unregister(); > +} > + > +static void smd_hci_cmd_remove(struct qcom_smd_device *sdev) > +{ > + smd_hci.cmd_channel = NULL; > + smd_hci_unregister(); > +} > + > +static const struct qcom_smd_id smd_hci_acl_match[] = { > + { .name = "APPS_RIVA_BT_ACL" }, > + {} > +}; > + > +static const struct qcom_smd_id smd_hci_cmd_match[] = { > + { .name = "APPS_RIVA_BT_CMD" }, > + {} > +}; > + > +static struct qcom_smd_driver smd_hci_acl_driver = { > + .probe = smd_hci_acl_probe, > + .remove = smd_hci_acl_remove, > + .callback = smd_hci_acl_callback, > + .smd_match_table = smd_hci_acl_match, > + .driver = { > + .name = "qcom_smd_hci_acl", > + .owner = THIS_MODULE, > + }, > +}; > + > +static struct qcom_smd_driver smd_hci_cmd_driver = { > + .probe = smd_hci_cmd_probe, > + .remove = smd_hci_cmd_remove, > + .callback = smd_hci_cmd_callback, > + .smd_match_table = smd_hci_cmd_match, > + .driver = { > + .name = "qcom_smd_hci_cmd", > + .owner = THIS_MODULE, > + }, > +}; > + > +module_qcom_smd_driver(smd_hci_acl_driver); > +module_qcom_smd_driver(smd_hci_cmd_driver); This is not going to fly. This turns it into two module_{init,exit} calls. That really does not work this way. I mean why do you need two different drivers in the first place. I should be matching via a single qcom_smd_id in the first place. And you handle everything else in the probe() callback. For me this is similar to the USB case where you actually have to claim to interfaces. We just lookup the other interface and claim it. It looks like SMD core functionality needs the same feature. > + > +MODULE_DESCRIPTION("Qualcomm SMD HCI driver"); > +MODULE_LICENSE("GPL v2"); > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > index 7ca6690355ea..ee5b2dd922f6 100644 > --- a/include/net/bluetooth/hci.h > +++ b/include/net/bluetooth/hci.h > @@ -58,6 +58,7 @@ > #define HCI_RS232 4 > #define HCI_PCI 5 > #define HCI_SDIO 6 > +#define HCI_SMD 7 I would prefer if this come in a separate patch from the driver. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 30 Sep 23:54 PDT 2015, Marcel Holtmann wrote: > Hi Bjorn, > > > The Qualcomm WCNSS chip provides two SMD channels to the BT core; one > > for command and one for event packets. This driver exposes the two > > channels as a hci device. > > [..] > > diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile > > index 07c9cf381e5a..43c7dc8641ff 100644 > > --- a/drivers/bluetooth/Makefile > > +++ b/drivers/bluetooth/Makefile > > @@ -14,6 +14,7 @@ obj-$(CONFIG_BT_HCIBTUART) += btuart_cs.o > > > > obj-$(CONFIG_BT_HCIBTUSB) += btusb.o > > obj-$(CONFIG_BT_HCIBTSDIO) += btsdio.o > > +obj-$(CONFIG_BT_HCISMD) += hci_smd.o > > I wonder if choosing a name like btqcomsmd.ko would not be better > here. For now I am fine with keeping hci_smd.ko since there are other > issues here to be fixed first. I had some problems figuring out the naming scheme here, but btqcomsmd does seem reasonable, I'll update it in the next set. > Especially the kbuild test robot complained loudly. > Sorry, I forgot to mention in the patch that the patch depends on the qcom_smd_id patch - that's available in linux-next. I will take another look, but I think that was the cause of all the issues. > > obj-$(CONFIG_BT_INTEL) += btintel.o > > obj-$(CONFIG_BT_ATH3K) += ath3k.o > > diff --git a/drivers/bluetooth/hci_smd.c b/drivers/bluetooth/hci_smd.c [..] > > +#include <net/bluetooth/hci.h> > > The hci.h include is not needed. If you do, then you are doing > something fishy. > Nothing fishy going on here, so I'll drop it. > > + > > +static struct { > > + struct qcom_smd_channel *acl_channel; > > + struct qcom_smd_channel *cmd_channel; > > + > > + struct hci_dev *hci; > > +} smd_hci; > > A driver that only supports a single instance of a device and uses a > global variable to do so is not a good idea. There should be no reason > for this. Why is this done this way. > There's two channels coming up, each probing the associated driver that combines these into 1 hci device. So I have two options; * the original idea was to implement multi-channel-per-device support in SMD, but as this is the only known case where that's needed I really don't want to add all the extra complexity to SMD. * I can accept the fact that this is silicon inside the MSM SoC and should never exist in more than one instance and make this hack. The first version I implemented had this structure allocated on the first probe, but that didn't really add any value to the static struct. I picked the second option due to the fact that the patch to SMD was way bigger then this patch, and full of nasty logic. > > + > > +static int smd_hci_recv(unsigned type, const void *data, size_t count) > > +{ > > + struct sk_buff *skb; > > + void *buf; > > + int ret; > > + > > + skb = bt_skb_alloc(count, GFP_ATOMIC); > > Is it required that this is GFP_ATOMIC or can this actually be > GFP_KERNEL? > This is being called from IRQ context upon receiving data on the channels. > > + if (!skb) > > + return -ENOMEM; > > + > > + buf = skb_put(skb, count); > > + memcpy_fromio(buf, data, count); > > Why is this a memcpy_fromio here? > A memcpy here doesn't seem to work on ARM64, probably because the pointer references ioremapped memory. We are discussing either marking the data __iomem or perhaps using memremap rather then ioremap. > > + > > + skb->dev = (void *)smd_hci.hci; > > This is no longer needed. > Thanks > > + bt_cb(skb)->pkt_type = type; > > + skb_orphan(skb); > > + > > + ret = hci_recv_frame(smd_hci.hci, skb); > > + if (ret < 0) > > + kfree_skb(skb); > > This is a double kfree_skb here. The hci_recv_frame will consume the > skb no matter what. > Thanks > > + > > + return ret; > > +} > > + [..] > > + > > +static int smd_hci_send(struct hci_dev *hdev, struct sk_buff *skb) > > +{ > > + int ret; > > + > > + switch (bt_cb(skb)->pkt_type) { > > + case HCI_ACLDATA_PKT: > > + case HCI_SCODATA_PKT: > > + ret = qcom_smd_send(smd_hci.acl_channel, skb->data, skb->len); > > + break; > > + case HCI_COMMAND_PKT: > > + ret = qcom_smd_send(smd_hci.cmd_channel, skb->data, skb->len); > > + break; > > + default: > > + ret = -ENODEV; > > + break; > > + } > > + > > + kfree_skb(skb); > > If you return an error, then the caller of hdev->send will free the > skb. > Must have looked in the wrong kernel tree :( Thanks. > > + > > + return ret; > > +} > > + > > +static int smd_hci_open(struct hci_dev *hci) > > +{ > > + return 0; > > +} > > + > > +static int smd_hci_close(struct hci_dev *hci) > > +{ > > + return 0; > > +} > > These two need to at least handling the HCI_RUNNING flag setting and > clearing like all the other drivers do. > I'm puzzled to the purpose of this, is this only for indication to user space or am I missing something? I found a couple of others that does nothing but set and clear this bit in their open and close. Would you be open for a framework patch that provides this functionality to drivers that doesn't need anything else? > > + > > +static int smd_hci_set_bdaddr(struct hci_dev *hci, > > + const bdaddr_t *bdaddr) > > +{ > > + u8 buf[12]; > > + > > + buf[0] = 0x0b; > > + buf[1] = 0xfc; > > + buf[2] = 0x9; > > + buf[3] = 0x1; > > + buf[4] = 0x2; > > Use full 0x00 syntax here. > Ok > > + buf[5] = sizeof(bdaddr_t); > > + memcpy(buf + 6, bdaddr, sizeof(bdaddr_t)); > > We also need to be sure that this command only changes the BD_ADDR in > RAM. It can not be persistent. When resetting / power cycle the > controller it will fallback to its default. > There seems to be no persistent storage at all in this chip, the BD_ADDR being used comes from one of the firmware files, so this just adjust the in-memory configuration. > > + > > + return qcom_smd_send(smd_hci.cmd_channel, buf, sizeof(buf)); > > Use __hci_cmd_sync since that is what is suppose to be used in this > callback. See the other drivers on how this is done. > I did see this, and I first implemented it like that. Unfortunately the firmware does not ack the command and __hci_cmd_sync() just times out. I didn't manage to find a way around this. > I wonder if qca_set_bdaddr_rome is not actually the same command and > you could just use that instead. > Looks to be exactly the same thing and gives insight in what those first 5 bytes are. I'll update the magics based on this information. If I didn't have the issue of the timeout it seems like I should be able to just call it. > > +} > > + > > +static int smd_hci_register(void) > > +{ > > + struct hci_dev *hci; > > + int ret; > > + > > + if (smd_hci.hci) > > + return 0; > > + > > + /* Wait for both channels to probe before registering */ > > + if (!smd_hci.acl_channel || !smd_hci.cmd_channel) > > + return 0; > > + > > + hci = hci_alloc_dev(); > > + if (!hci) > > + return -ENOMEM; > > + > > + hci->bus = HCI_SMD; > > + hci->open = smd_hci_open; > > + hci->close = smd_hci_close; > > + hci->send = smd_hci_send; > > + hci->set_bdaddr = smd_hci_set_bdaddr; > > + > > + ret = hci_register_dev(hci); > > + if (ret < 0) { > > + hci_free_dev(hci); > > + return ret; > > + } > > + > > + smd_hci.hci = hci; > > + > > + return 0; > > +} > > + > > +static void smd_hci_unregister(void) > > +{ > > + /* Only unregister on the first remove call */ > > + if (!smd_hci.hci) > > + return; > > + > > + hci_unregister_dev(smd_hci.hci); > > + hci_free_dev(smd_hci.hci); > > + smd_hci.hci = NULL; > > +} > > + > > +static int smd_hci_acl_probe(struct qcom_smd_device *sdev) > > +{ > > + smd_hci.acl_channel = sdev->channel; > > + smd_hci_register(); > > + > > + return 0; > > +} > > + > > +static int smd_hci_cmd_probe(struct qcom_smd_device *sdev) > > +{ > > + smd_hci.cmd_channel = sdev->channel; > > + smd_hci_register(); > > + > > + return 0; > > +} > > + > > +static void smd_hci_acl_remove(struct qcom_smd_device *sdev) > > +{ > > + smd_hci.acl_channel = NULL; > > + smd_hci_unregister(); > > +} > > + > > +static void smd_hci_cmd_remove(struct qcom_smd_device *sdev) > > +{ > > + smd_hci.cmd_channel = NULL; > > + smd_hci_unregister(); > > +} > > + > > +static const struct qcom_smd_id smd_hci_acl_match[] = { > > + { .name = "APPS_RIVA_BT_ACL" }, > > + {} > > +}; > > + > > +static const struct qcom_smd_id smd_hci_cmd_match[] = { > > + { .name = "APPS_RIVA_BT_CMD" }, > > + {} > > +}; > > + > > +static struct qcom_smd_driver smd_hci_acl_driver = { > > + .probe = smd_hci_acl_probe, > > + .remove = smd_hci_acl_remove, > > + .callback = smd_hci_acl_callback, > > + .smd_match_table = smd_hci_acl_match, > > + .driver = { > > + .name = "qcom_smd_hci_acl", > > + .owner = THIS_MODULE, > > + }, > > +}; > > + > > +static struct qcom_smd_driver smd_hci_cmd_driver = { > > + .probe = smd_hci_cmd_probe, > > + .remove = smd_hci_cmd_remove, > > + .callback = smd_hci_cmd_callback, > > + .smd_match_table = smd_hci_cmd_match, > > + .driver = { > > + .name = "qcom_smd_hci_cmd", > > + .owner = THIS_MODULE, > > + }, > > +}; > > + > > +module_qcom_smd_driver(smd_hci_acl_driver); > > +module_qcom_smd_driver(smd_hci_cmd_driver); > > This is not going to fly. This turns it into two module_{init,exit} > calls. That really does not work this way. > Right, I would need to at least fill in my own init and exit functions with the two registers/unregisters calls... > I mean why do you need two different drivers in the first place. I > should be matching via a single qcom_smd_id in the first place. And > you handle everything else in the probe() callback. > The first iteration had 1 probe, 1 remove and 1 callback. I pulled the match-data in probe and made the rest use this information by drvdata. It was the same blocks of code as now, but accessed via the wrapper drvdata object - so although one shouldn't do like this, this is much cleaner. > For me this is similar to the USB case where you actually have to > claim to interfaces. We just lookup the other interface and claim it. > It looks like SMD core functionality needs the same feature. > Indeed, as I said above the plan was to implement something where I either only probe when all channels where up (turned out to be a mess to implement) or have some accessor function that can be called from probe to open a second channel. The latter seems reasonable to do, but it will be way more code than there's in this driver - which seems to be the only case where that feature would be needed. > > + > > +MODULE_DESCRIPTION("Qualcomm SMD HCI driver"); > > +MODULE_LICENSE("GPL v2"); > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > > index 7ca6690355ea..ee5b2dd922f6 100644 > > --- a/include/net/bluetooth/hci.h > > +++ b/include/net/bluetooth/hci.h > > @@ -58,6 +58,7 @@ > > #define HCI_RS232 4 > > #define HCI_PCI 5 > > #define HCI_SDIO 6 > > +#define HCI_SMD 7 > > I would prefer if this come in a separate patch from the driver. > Sure thing. Thanks for your review Marcel. Regards, Born -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, >>> The Qualcomm WCNSS chip provides two SMD channels to the BT core; one >>> for command and one for event packets. This driver exposes the two >>> channels as a hci device. >>> > [..] >>> diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile >>> index 07c9cf381e5a..43c7dc8641ff 100644 >>> --- a/drivers/bluetooth/Makefile >>> +++ b/drivers/bluetooth/Makefile >>> @@ -14,6 +14,7 @@ obj-$(CONFIG_BT_HCIBTUART) += btuart_cs.o >>> >>> obj-$(CONFIG_BT_HCIBTUSB) += btusb.o >>> obj-$(CONFIG_BT_HCIBTSDIO) += btsdio.o >>> +obj-$(CONFIG_BT_HCISMD) += hci_smd.o >> >> I wonder if choosing a name like btqcomsmd.ko would not be better >> here. For now I am fine with keeping hci_smd.ko since there are other >> issues here to be fixed first. > > I had some problems figuring out the naming scheme here, but btqcomsmd > does seem reasonable, I'll update it in the next set. > >> Especially the kbuild test robot complained loudly. >> > > Sorry, I forgot to mention in the patch that the patch depends on the > qcom_smd_id patch - that's available in linux-next. I will take another > look, but I think that was the cause of all the issues. which means, I can only merge this driver when this other patch has hit net-next tree. Unless I take the qcom_smd_id through bluetooth-next tree which doesn't look like a good idea. >>> obj-$(CONFIG_BT_INTEL) += btintel.o >>> obj-$(CONFIG_BT_ATH3K) += ath3k.o >>> diff --git a/drivers/bluetooth/hci_smd.c b/drivers/bluetooth/hci_smd.c > [..] >>> +#include <net/bluetooth/hci.h> >> >> The hci.h include is not needed. If you do, then you are doing >> something fishy. >> > > Nothing fishy going on here, so I'll drop it. > >>> + >>> +static struct { >>> + struct qcom_smd_channel *acl_channel; >>> + struct qcom_smd_channel *cmd_channel; >>> + >>> + struct hci_dev *hci; >>> +} smd_hci; >> >> A driver that only supports a single instance of a device and uses a >> global variable to do so is not a good idea. There should be no reason >> for this. Why is this done this way. >> > > There's two channels coming up, each probing the associated driver that > combines these into 1 hci device. > > So I have two options; > > * the original idea was to implement multi-channel-per-device support in > SMD, but as this is the only known case where that's needed I really > don't want to add all the extra complexity to SMD. > > * I can accept the fact that this is silicon inside the MSM SoC and > should never exist in more than one instance and make this hack. The > first version I implemented had this structure allocated on the first > probe, but that didn't really add any value to the static struct. > > I picked the second option due to the fact that the patch to SMD was way > bigger then this patch, and full of nasty logic. Writing a driver that assume it is a single instance of a given device is never a good idea. While you might find some instances in the kernel if you look hard enough, but that doesn't mean we should keep doing this. So this needs addressing. > >>> + >>> +static int smd_hci_recv(unsigned type, const void *data, size_t count) >>> +{ >>> + struct sk_buff *skb; >>> + void *buf; >>> + int ret; >>> + >>> + skb = bt_skb_alloc(count, GFP_ATOMIC); >> >> Is it required that this is GFP_ATOMIC or can this actually be >> GFP_KERNEL? >> > > This is being called from IRQ context upon receiving data on the > channels. I wonder why that is needed, but seems an issue in the SMD subsystem. So this is fine, might want to add a comment above the bt_skb_alloc to remind us. > >>> + if (!skb) >>> + return -ENOMEM; >>> + >>> + buf = skb_put(skb, count); >>> + memcpy_fromio(buf, data, count); >> >> Why is this a memcpy_fromio here? >> > > A memcpy here doesn't seem to work on ARM64, probably because the > pointer references ioremapped memory. We are discussing either marking > the data __iomem or perhaps using memremap rather then ioremap. I would add a comment here as well to remind everybody why this is used. And marking up the pointers is a good idea as well. >>> + >>> + skb->dev = (void *)smd_hci.hci; >> >> This is no longer needed. >> > > Thanks > >>> + bt_cb(skb)->pkt_type = type; >>> + skb_orphan(skb); >>> + >>> + ret = hci_recv_frame(smd_hci.hci, skb); >>> + if (ret < 0) >>> + kfree_skb(skb); >> >> This is a double kfree_skb here. The hci_recv_frame will consume the >> skb no matter what. >> > > Thanks > >>> + >>> + return ret; >>> +} >>> + > [..] >>> + >>> +static int smd_hci_send(struct hci_dev *hdev, struct sk_buff *skb) >>> +{ >>> + int ret; >>> + >>> + switch (bt_cb(skb)->pkt_type) { >>> + case HCI_ACLDATA_PKT: >>> + case HCI_SCODATA_PKT: >>> + ret = qcom_smd_send(smd_hci.acl_channel, skb->data, skb->len); >>> + break; >>> + case HCI_COMMAND_PKT: >>> + ret = qcom_smd_send(smd_hci.cmd_channel, skb->data, skb->len); >>> + break; >>> + default: >>> + ret = -ENODEV; >>> + break; >>> + } >>> + >>> + kfree_skb(skb); > >> >> If you return an error, then the caller of hdev->send will free the >> skb. >> > > Must have looked in the wrong kernel tree :( > > Thanks. > >>> + >>> + return ret; >>> +} >>> + >>> +static int smd_hci_open(struct hci_dev *hci) >>> +{ >>> + return 0; >>> +} >>> + >>> +static int smd_hci_close(struct hci_dev *hci) >>> +{ >>> + return 0; >>> +} >> >> These two need to at least handling the HCI_RUNNING flag setting and >> clearing like all the other drivers do. >> > > I'm puzzled to the purpose of this, is this only for indication to user > space or am I missing something? > > I found a couple of others that does nothing but set and clear this bit > in their open and close. Would you be open for a framework patch that > provides this functionality to drivers that doesn't need anything else? This has been in the drivers for as long as the Bluetooth subsystem has been merged upstream. So Linux 2.4.6 to be precise. I had plans to move this code into the core, but that has not happened yet. So until then, lets just include the HCI_RUNNING part. If there are drivers not doing this, then that is a bug and needs to be fixed. Please point them out to me. >>> + >>> +static int smd_hci_set_bdaddr(struct hci_dev *hci, >>> + const bdaddr_t *bdaddr) >>> +{ >>> + u8 buf[12]; >>> + >>> + buf[0] = 0x0b; >>> + buf[1] = 0xfc; >>> + buf[2] = 0x9; >>> + buf[3] = 0x1; >>> + buf[4] = 0x2; >> >> Use full 0x00 syntax here. >> > > Ok > >>> + buf[5] = sizeof(bdaddr_t); >>> + memcpy(buf + 6, bdaddr, sizeof(bdaddr_t)); >> >> We also need to be sure that this command only changes the BD_ADDR in >> RAM. It can not be persistent. When resetting / power cycle the >> controller it will fallback to its default. >> > > There seems to be no persistent storage at all in this chip, the BD_ADDR > being used comes from one of the firmware files, so this just adjust the > in-memory configuration. > >>> + >>> + return qcom_smd_send(smd_hci.cmd_channel, buf, sizeof(buf)); >> >> Use __hci_cmd_sync since that is what is suppose to be used in this >> callback. See the other drivers on how this is done. >> > > I did see this, and I first implemented it like that. Unfortunately the > firmware does not ack the command and __hci_cmd_sync() just times out. > > I didn't manage to find a way around this. What to you mean by this? It does not send a Command Complete? Does it send a Vendor Event instead? The real problem is that this callback is on purpose synchronous. The core relies on the fact that you only return when it actually completed. >> I wonder if qca_set_bdaddr_rome is not actually the same command and >> you could just use that instead. >> > > Looks to be exactly the same thing and gives insight in what those first > 5 bytes are. I'll update the magics based on this information. > > If I didn't have the issue of the timeout it seems like I should be able > to just call it. Has something tried to talk to Qualcomm why that timeout happens? They have something like this for the UART speed change which is changed in NVRAM. Maybe this is a more generic way on how they handle things. It would be good if guys from Qualcomm could give some insights here. However in case of their UART driver, this seems to work as it should. Maybe just a newer firmware is needed. >>> +} >>> + >>> +static int smd_hci_register(void) >>> +{ >>> + struct hci_dev *hci; >>> + int ret; >>> + >>> + if (smd_hci.hci) >>> + return 0; >>> + >>> + /* Wait for both channels to probe before registering */ >>> + if (!smd_hci.acl_channel || !smd_hci.cmd_channel) >>> + return 0; >>> + >>> + hci = hci_alloc_dev(); >>> + if (!hci) >>> + return -ENOMEM; >>> + >>> + hci->bus = HCI_SMD; >>> + hci->open = smd_hci_open; >>> + hci->close = smd_hci_close; >>> + hci->send = smd_hci_send; >>> + hci->set_bdaddr = smd_hci_set_bdaddr; >>> + >>> + ret = hci_register_dev(hci); >>> + if (ret < 0) { >>> + hci_free_dev(hci); >>> + return ret; >>> + } >>> + >>> + smd_hci.hci = hci; >>> + >>> + return 0; >>> +} >>> + >>> +static void smd_hci_unregister(void) >>> +{ >>> + /* Only unregister on the first remove call */ >>> + if (!smd_hci.hci) >>> + return; >>> + >>> + hci_unregister_dev(smd_hci.hci); >>> + hci_free_dev(smd_hci.hci); >>> + smd_hci.hci = NULL; >>> +} >>> + >>> +static int smd_hci_acl_probe(struct qcom_smd_device *sdev) >>> +{ >>> + smd_hci.acl_channel = sdev->channel; >>> + smd_hci_register(); >>> + >>> + return 0; >>> +} >>> + >>> +static int smd_hci_cmd_probe(struct qcom_smd_device *sdev) >>> +{ >>> + smd_hci.cmd_channel = sdev->channel; >>> + smd_hci_register(); >>> + >>> + return 0; >>> +} >>> + >>> +static void smd_hci_acl_remove(struct qcom_smd_device *sdev) >>> +{ >>> + smd_hci.acl_channel = NULL; >>> + smd_hci_unregister(); >>> +} >>> + >>> +static void smd_hci_cmd_remove(struct qcom_smd_device *sdev) >>> +{ >>> + smd_hci.cmd_channel = NULL; >>> + smd_hci_unregister(); >>> +} >>> + >>> +static const struct qcom_smd_id smd_hci_acl_match[] = { >>> + { .name = "APPS_RIVA_BT_ACL" }, >>> + {} >>> +}; >>> + >>> +static const struct qcom_smd_id smd_hci_cmd_match[] = { >>> + { .name = "APPS_RIVA_BT_CMD" }, >>> + {} >>> +}; >>> + >>> +static struct qcom_smd_driver smd_hci_acl_driver = { >>> + .probe = smd_hci_acl_probe, >>> + .remove = smd_hci_acl_remove, >>> + .callback = smd_hci_acl_callback, >>> + .smd_match_table = smd_hci_acl_match, >>> + .driver = { >>> + .name = "qcom_smd_hci_acl", >>> + .owner = THIS_MODULE, >>> + }, >>> +}; >>> + >>> +static struct qcom_smd_driver smd_hci_cmd_driver = { >>> + .probe = smd_hci_cmd_probe, >>> + .remove = smd_hci_cmd_remove, >>> + .callback = smd_hci_cmd_callback, >>> + .smd_match_table = smd_hci_cmd_match, >>> + .driver = { >>> + .name = "qcom_smd_hci_cmd", >>> + .owner = THIS_MODULE, >>> + }, >>> +}; >>> + >>> +module_qcom_smd_driver(smd_hci_acl_driver); >>> +module_qcom_smd_driver(smd_hci_cmd_driver); > >> >> This is not going to fly. This turns it into two module_{init,exit} >> calls. That really does not work this way. >> > > Right, I would need to at least fill in my own init and exit functions > with the two registers/unregisters calls... > >> I mean why do you need two different drivers in the first place. I >> should be matching via a single qcom_smd_id in the first place. And >> you handle everything else in the probe() callback. >> > > The first iteration had 1 probe, 1 remove and 1 callback. I pulled the > match-data in probe and made the rest use this information by drvdata. > It was the same blocks of code as now, but accessed via the wrapper > drvdata object - so although one shouldn't do like this, this is much > cleaner. > >> For me this is similar to the USB case where you actually have to >> claim to interfaces. We just lookup the other interface and claim it. >> It looks like SMD core functionality needs the same feature. >> > > Indeed, as I said above the plan was to implement something where I > either only probe when all channels where up (turned out to be a mess to > implement) or have some accessor function that can be called from probe > to open a second channel. > > The latter seems reasonable to do, but it will be way more code than > there's in this driver - which seems to be the only case where that > feature would be needed. That is what USB does when claiming a second interface. I would do exactly that and especially be able to make sure that both channels have the same struct device as parent. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat 03 Oct 09:55 PDT 2015, Marcel Holtmann wrote: > Hi Bjorn, > [..] > >> Especially the kbuild test robot complained loudly. > >> > > > > Sorry, I forgot to mention in the patch that the patch depends on the > > qcom_smd_id patch - that's available in linux-next. I will take another > > look, but I think that was the cause of all the issues. > > which means, I can only merge this driver when this other patch has > hit net-next tree. Unless I take the qcom_smd_id through > bluetooth-next tree which doesn't look like a good idea. > I was going to suggest that we just wait for -rc1, but the new additions to SMD adds a bunch of new items on the dependency list. Perhaps once we agree we can get your Ack and have Andy pull it through the qcom-soc tree together with the SMD patches? > >>> obj-$(CONFIG_BT_INTEL) += btintel.o > >>> obj-$(CONFIG_BT_ATH3K) += ath3k.o > >>> diff --git a/drivers/bluetooth/hci_smd.c b/drivers/bluetooth/hci_smd.c > > [..] > >>> +#include <net/bluetooth/hci.h> > >> > >> The hci.h include is not needed. If you do, then you are doing > >> something fishy. > >> > > > > Nothing fishy going on here, so I'll drop it. > > > >>> + > >>> +static struct { > >>> + struct qcom_smd_channel *acl_channel; > >>> + struct qcom_smd_channel *cmd_channel; > >>> + > >>> + struct hci_dev *hci; > >>> +} smd_hci; > >> > >> A driver that only supports a single instance of a device and uses a > >> global variable to do so is not a good idea. There should be no reason > >> for this. Why is this done this way. > >> > > > > There's two channels coming up, each probing the associated driver that > > combines these into 1 hci device. > > > > So I have two options; > > > > * the original idea was to implement multi-channel-per-device support in > > SMD, but as this is the only known case where that's needed I really > > don't want to add all the extra complexity to SMD. > > > > * I can accept the fact that this is silicon inside the MSM SoC and > > should never exist in more than one instance and make this hack. The > > first version I implemented had this structure allocated on the first > > probe, but that didn't really add any value to the static struct. > > > > I picked the second option due to the fact that the patch to SMD was way > > bigger then this patch, and full of nasty logic. > > Writing a driver that assume it is a single instance of a given device > is never a good idea. While you might find some instances in the > kernel if you look hard enough, but that doesn't mean we should keep > doing this. So this needs addressing. > Fair enough. > > > >>> + > >>> +static int smd_hci_recv(unsigned type, const void *data, size_t count) > >>> +{ > >>> + struct sk_buff *skb; > >>> + void *buf; > >>> + int ret; > >>> + > >>> + skb = bt_skb_alloc(count, GFP_ATOMIC); > >> > >> Is it required that this is GFP_ATOMIC or can this actually be > >> GFP_KERNEL? > >> > > > > This is being called from IRQ context upon receiving data on the > > channels. > > I wonder why that is needed, but seems an issue in the SMD subsystem. > So this is fine, might want to add a comment above the bt_skb_alloc to > remind us. > It's a design choice of mine, to deliver the incoming data directly from the interrupt handler. Sure enough we could have had a worker thread or something do it. I'll add a comment. > > > >>> + if (!skb) > >>> + return -ENOMEM; > >>> + > >>> + buf = skb_put(skb, count); > >>> + memcpy_fromio(buf, data, count); > >> > >> Why is this a memcpy_fromio here? > >> > > > > A memcpy here doesn't seem to work on ARM64, probably because the > > pointer references ioremapped memory. We are discussing either marking > > the data __iomem or perhaps using memremap rather then ioremap. > > I would add a comment here as well to remind everybody why this is > used. And marking up the pointers is a good idea as well. > Yeah, I have this on the todo. Will put a comment here for now. [..] > >>> + > >>> + return ret; > >>> +} > >>> + > >>> +static int smd_hci_open(struct hci_dev *hci) > >>> +{ > >>> + return 0; > >>> +} > >>> + > >>> +static int smd_hci_close(struct hci_dev *hci) > >>> +{ > >>> + return 0; > >>> +} > >> > >> These two need to at least handling the HCI_RUNNING flag setting and > >> clearing like all the other drivers do. > >> > > > > I'm puzzled to the purpose of this, is this only for indication to user > > space or am I missing something? > > > > I found a couple of others that does nothing but set and clear this bit > > in their open and close. Would you be open for a framework patch that > > provides this functionality to drivers that doesn't need anything else? > > This has been in the drivers for as long as the Bluetooth subsystem > has been merged upstream. So Linux 2.4.6 to be precise. I had plans to > move this code into the core, but that has not happened yet. So until > then, lets just include the HCI_RUNNING part. > I'll add the pieces. > If there are drivers not doing this, then that is a bug and needs to > be fixed. Please point them out to me. > What I meant was that I couldn't find anyone consuming this bit, the cases I looked at all fiddled with it in various ways. But I presume it can be seen from userspace. > >>> + > >>> +static int smd_hci_set_bdaddr(struct hci_dev *hci, > >>> + const bdaddr_t *bdaddr) > >>> +{ > >>> + u8 buf[12]; > >>> + > >>> + buf[0] = 0x0b; > >>> + buf[1] = 0xfc; > >>> + buf[2] = 0x9; > >>> + buf[3] = 0x1; > >>> + buf[4] = 0x2; > >> > >> Use full 0x00 syntax here. > >> > > > > Ok > > > >>> + buf[5] = sizeof(bdaddr_t); > >>> + memcpy(buf + 6, bdaddr, sizeof(bdaddr_t)); > >> > >> We also need to be sure that this command only changes the BD_ADDR in > >> RAM. It can not be persistent. When resetting / power cycle the > >> controller it will fallback to its default. > >> > > > > There seems to be no persistent storage at all in this chip, the BD_ADDR > > being used comes from one of the firmware files, so this just adjust the > > in-memory configuration. > > > >>> + > >>> + return qcom_smd_send(smd_hci.cmd_channel, buf, sizeof(buf)); > >> > >> Use __hci_cmd_sync since that is what is suppose to be used in this > >> callback. See the other drivers on how this is done. > >> > > > > I did see this, and I first implemented it like that. Unfortunately the > > firmware does not ack the command and __hci_cmd_sync() just times out. > > > > I didn't manage to find a way around this. > > What to you mean by this? It does not send a Command Complete? Does it > send a Vendor Event instead? > The immediate problem I had seems to be that I specified the wrong event type. With this corrected it passes most of the time - but in some cases I don't get the command complete packet back. I don't know what causes this and I don't have access to the source code of the other side :/ > The real problem is that this callback is on purpose synchronous. The > core relies on the fact that you only return when it actually > completed. > Makes sense. > >> I wonder if qca_set_bdaddr_rome is not actually the same command and > >> you could just use that instead. > >> > > > > Looks to be exactly the same thing and gives insight in what those first > > 5 bytes are. I'll update the magics based on this information. > > > > If I didn't have the issue of the timeout it seems like I should be able > > to just call it. > > Has something tried to talk to Qualcomm why that timeout happens? They > have something like this for the UART speed change which is changed in > NVRAM. Maybe this is a more generic way on how they handle things. It > would be good if guys from Qualcomm could give some insights here. > However in case of their UART driver, this seems to work as it should. > Maybe just a newer firmware is needed. > Unfortunately this part of Qualcomm hasn't joined the dance yet. When booting the chip we load a file with "NV values", so I presume that this command allows us to patch those entries in runtime. [..] > > That is what USB does when claiming a second interface. I would do > exactly that and especially be able to make sure that both channels > have the same struct device as parent. > I was hoping to not have to implement this, as it does complicates the life cycle management of things in the SMD core but agrees with you that it's cleaner. I'll finish up the rewrite and send out a new version based on this. Thanks for the review Marcel! Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Oct 08, 2015 at 10:07:44AM -0700, Bjorn Andersson wrote: > On Sat 03 Oct 09:55 PDT 2015, Marcel Holtmann wrote: > > > Hi Bjorn, > > > [..] > > >> Especially the kbuild test robot complained loudly. > > >> > > > > > > Sorry, I forgot to mention in the patch that the patch depends on the > > > qcom_smd_id patch - that's available in linux-next. I will take another > > > look, but I think that was the cause of all the issues. > > > > which means, I can only merge this driver when this other patch has > > hit net-next tree. Unless I take the qcom_smd_id through > > bluetooth-next tree which doesn't look like a good idea. > > > > I was going to suggest that we just wait for -rc1, but the new additions > to SMD adds a bunch of new items on the dependency list. > > Perhaps once we agree we can get your Ack and have Andy pull it through > the qcom-soc tree together with the SMD patches? This is fine by me. > > >>> obj-$(CONFIG_BT_INTEL) += btintel.o > > >>> obj-$(CONFIG_BT_ATH3K) += ath3k.o > > >>> diff --git a/drivers/bluetooth/hci_smd.c b/drivers/bluetooth/hci_smd.c > > > [..] > > >>> +#include <net/bluetooth/hci.h> > > >> > > >> The hci.h include is not needed. If you do, then you are doing > > >> something fishy. > > >> > > > <snip>
diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig index 3d480d8c6111..1a2658f373b5 100644 --- a/drivers/bluetooth/Kconfig +++ b/drivers/bluetooth/Kconfig @@ -62,6 +62,17 @@ config BT_HCIBTSDIO Say Y here to compile support for Bluetooth SDIO devices into the kernel or say M to compile it as module (btsdio). +config BT_HCISMD + tristate "HCI Qualcomm SMD driver" + depends on QCOM_SMD + help + Qualcomm SMD HCI driver. + This driver is used to bridge HCI data onto the shared memory + channels to the WCNSS core. + + Say Y here to compile support for HCI over Qualcomm SMD into the + kernelor say M to compile as a module. + config BT_HCIUART tristate "HCI UART driver" depends on TTY diff --git a/drivers/bluetooth/Makefile b/drivers/bluetooth/Makefile index 07c9cf381e5a..43c7dc8641ff 100644 --- a/drivers/bluetooth/Makefile +++ b/drivers/bluetooth/Makefile @@ -14,6 +14,7 @@ obj-$(CONFIG_BT_HCIBTUART) += btuart_cs.o obj-$(CONFIG_BT_HCIBTUSB) += btusb.o obj-$(CONFIG_BT_HCIBTSDIO) += btsdio.o +obj-$(CONFIG_BT_HCISMD) += hci_smd.o obj-$(CONFIG_BT_INTEL) += btintel.o obj-$(CONFIG_BT_ATH3K) += ath3k.o diff --git a/drivers/bluetooth/hci_smd.c b/drivers/bluetooth/hci_smd.c new file mode 100644 index 000000000000..e5748da2f902 --- /dev/null +++ b/drivers/bluetooth/hci_smd.c @@ -0,0 +1,222 @@ +/* + * Copyright (c) 2015, Sony Mobile Communications Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/module.h> +#include <linux/slab.h> +#include <linux/soc/qcom/smd.h> +#include <net/bluetooth/bluetooth.h> +#include <net/bluetooth/hci_core.h> +#include <net/bluetooth/hci.h> + +static struct { + struct qcom_smd_channel *acl_channel; + struct qcom_smd_channel *cmd_channel; + + struct hci_dev *hci; +} smd_hci; + +static int smd_hci_recv(unsigned type, const void *data, size_t count) +{ + struct sk_buff *skb; + void *buf; + int ret; + + skb = bt_skb_alloc(count, GFP_ATOMIC); + if (!skb) + return -ENOMEM; + + buf = skb_put(skb, count); + memcpy_fromio(buf, data, count); + + skb->dev = (void *)smd_hci.hci; + bt_cb(skb)->pkt_type = type; + skb_orphan(skb); + + ret = hci_recv_frame(smd_hci.hci, skb); + if (ret < 0) + kfree_skb(skb); + + return ret; +} + +static int smd_hci_acl_callback(struct qcom_smd_device *qsdev, + const void *data, + size_t count) +{ + return smd_hci_recv(HCI_ACLDATA_PKT, data, count); +} + +static int smd_hci_cmd_callback(struct qcom_smd_device *qsdev, + const void *data, + size_t count) +{ + return smd_hci_recv(HCI_EVENT_PKT, data, count); +} + +static int smd_hci_send(struct hci_dev *hdev, struct sk_buff *skb) +{ + int ret; + + switch (bt_cb(skb)->pkt_type) { + case HCI_ACLDATA_PKT: + case HCI_SCODATA_PKT: + ret = qcom_smd_send(smd_hci.acl_channel, skb->data, skb->len); + break; + case HCI_COMMAND_PKT: + ret = qcom_smd_send(smd_hci.cmd_channel, skb->data, skb->len); + break; + default: + ret = -ENODEV; + break; + } + + kfree_skb(skb); + + return ret; +} + +static int smd_hci_open(struct hci_dev *hci) +{ + return 0; +} + +static int smd_hci_close(struct hci_dev *hci) +{ + return 0; +} + +static int smd_hci_set_bdaddr(struct hci_dev *hci, + const bdaddr_t *bdaddr) +{ + u8 buf[12]; + + buf[0] = 0x0b; + buf[1] = 0xfc; + buf[2] = 0x9; + buf[3] = 0x1; + buf[4] = 0x2; + buf[5] = sizeof(bdaddr_t); + memcpy(buf + 6, bdaddr, sizeof(bdaddr_t)); + + return qcom_smd_send(smd_hci.cmd_channel, buf, sizeof(buf)); +} + +static int smd_hci_register(void) +{ + struct hci_dev *hci; + int ret; + + if (smd_hci.hci) + return 0; + + /* Wait for both channels to probe before registering */ + if (!smd_hci.acl_channel || !smd_hci.cmd_channel) + return 0; + + hci = hci_alloc_dev(); + if (!hci) + return -ENOMEM; + + hci->bus = HCI_SMD; + hci->open = smd_hci_open; + hci->close = smd_hci_close; + hci->send = smd_hci_send; + hci->set_bdaddr = smd_hci_set_bdaddr; + + ret = hci_register_dev(hci); + if (ret < 0) { + hci_free_dev(hci); + return ret; + } + + smd_hci.hci = hci; + + return 0; +} + +static void smd_hci_unregister(void) +{ + /* Only unregister on the first remove call */ + if (!smd_hci.hci) + return; + + hci_unregister_dev(smd_hci.hci); + hci_free_dev(smd_hci.hci); + smd_hci.hci = NULL; +} + +static int smd_hci_acl_probe(struct qcom_smd_device *sdev) +{ + smd_hci.acl_channel = sdev->channel; + smd_hci_register(); + + return 0; +} + +static int smd_hci_cmd_probe(struct qcom_smd_device *sdev) +{ + smd_hci.cmd_channel = sdev->channel; + smd_hci_register(); + + return 0; +} + +static void smd_hci_acl_remove(struct qcom_smd_device *sdev) +{ + smd_hci.acl_channel = NULL; + smd_hci_unregister(); +} + +static void smd_hci_cmd_remove(struct qcom_smd_device *sdev) +{ + smd_hci.cmd_channel = NULL; + smd_hci_unregister(); +} + +static const struct qcom_smd_id smd_hci_acl_match[] = { + { .name = "APPS_RIVA_BT_ACL" }, + {} +}; + +static const struct qcom_smd_id smd_hci_cmd_match[] = { + { .name = "APPS_RIVA_BT_CMD" }, + {} +}; + +static struct qcom_smd_driver smd_hci_acl_driver = { + .probe = smd_hci_acl_probe, + .remove = smd_hci_acl_remove, + .callback = smd_hci_acl_callback, + .smd_match_table = smd_hci_acl_match, + .driver = { + .name = "qcom_smd_hci_acl", + .owner = THIS_MODULE, + }, +}; + +static struct qcom_smd_driver smd_hci_cmd_driver = { + .probe = smd_hci_cmd_probe, + .remove = smd_hci_cmd_remove, + .callback = smd_hci_cmd_callback, + .smd_match_table = smd_hci_cmd_match, + .driver = { + .name = "qcom_smd_hci_cmd", + .owner = THIS_MODULE, + }, +}; + +module_qcom_smd_driver(smd_hci_acl_driver); +module_qcom_smd_driver(smd_hci_cmd_driver); + +MODULE_DESCRIPTION("Qualcomm SMD HCI driver"); +MODULE_LICENSE("GPL v2"); diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h index 7ca6690355ea..ee5b2dd922f6 100644 --- a/include/net/bluetooth/hci.h +++ b/include/net/bluetooth/hci.h @@ -58,6 +58,7 @@ #define HCI_RS232 4 #define HCI_PCI 5 #define HCI_SDIO 6 +#define HCI_SMD 7 /* HCI controller types */ #define HCI_BREDR 0x00