| Message ID | 20230421-fp4-bluetooth-v1-0-0430e3a7e0a2@fairphone.com |
|---|---|
| Headers | show |
| Series | Add WCN3988 Bluetooth support for Fairphone 4 | expand |
On 21.04.2023 16:11, Luca Weiss wrote: > Just to start with the important part why this is an RFC: > > While Bluetooth chip init works totally fine and bluez seems to be > fairly happy with it, there's a (major) problem with scanning, as shown > with this bluetoothctl snippet and dmesg snippet: > > [bluetooth]# scan on > Failed to start discovery: org.bluez.Error.InProgress > > [ 202.371374] Bluetooth: hci0: Opcode 0x200b failed: -16 > > This opcode should be the following: > > include/net/bluetooth/hci.h:#define HCI_OP_LE_SET_SCAN_PARAM 0x200b Not a bluetooth expert or anything, but does that thing support bluetooth LE? Konrad > > Unfortunately trying various existing code branches in the Bluetooth > driver doesn't show any sign of making this work and I don't really know > where to look to debug this further. > > On the other hand "discoverable on" makes the device show up on other > devices during scanning , so the RF parts of the Bluetooth chip are > generally functional for sure. > > Any ideas are welcome. > > @Bjorn: Patch "arm64: dts: qcom: sm6350: add uart1 node" should be fine > to take regardless the RFC status, I don't think the problem is caused > there. > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > --- > Luca Weiss (4): > dt-bindings: net: qualcomm: Add WCN3988 > Bluetooth: btqca: Add WCN3988 support > arm64: dts: qcom: sm6350: add uart1 node > arm64: dts: qcom: sm7225-fairphone-fp4: Add Bluetooth > > .../bindings/net/bluetooth/qualcomm-bluetooth.yaml | 2 + > arch/arm64/boot/dts/qcom/sm6350.dtsi | 63 ++++++++++++++++++++++ > arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts | 17 ++++++ > drivers/bluetooth/btqca.c | 13 ++++- > drivers/bluetooth/btqca.h | 12 ++++- > drivers/bluetooth/hci_qca.c | 12 +++++ > 6 files changed, 115 insertions(+), 4 deletions(-) > --- > base-commit: cf4c0112a0350cfe8a63b5eb3377e2366f57545b > change-id: 20230421-fp4-bluetooth-b36a0e87b9c8 > > Best regards,
On 21/04/2023 16:11, Luca Weiss wrote: > Add the node describing uart1 incl. opp table and pinctrl. > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > --- > arch/arm64/boot/dts/qcom/sm6350.dtsi | 63 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 63 insertions(+) Please do not send DTS patches for net-next. DTS must go via Qualcomm SoC. Split the series and mention where is the bindings change in DTS patchset. Best regards, Krzysztof
On Sat Apr 22, 2023 at 2:03 PM CEST, Konrad Dybcio wrote: > > > On 21.04.2023 16:11, Luca Weiss wrote: > > Just to start with the important part why this is an RFC: > > > > While Bluetooth chip init works totally fine and bluez seems to be > > fairly happy with it, there's a (major) problem with scanning, as shown > > with this bluetoothctl snippet and dmesg snippet: > > > > [bluetooth]# scan on > > Failed to start discovery: org.bluez.Error.InProgress > > > > [ 202.371374] Bluetooth: hci0: Opcode 0x200b failed: -16 > > > > This opcode should be the following: > > > > include/net/bluetooth/hci.h:#define HCI_OP_LE_SET_SCAN_PARAM 0x200b > Not a bluetooth expert or anything, but does that thing support > bluetooth LE? I don't know too much about Bluetooth details either, but hasn't Bluetooth LE been a consistently supported thing since like 10 years? All the info I can easily find just states SM7225 SoC supports "Bluetooth 5.1". Regards Luca > > Konrad > > > > Unfortunately trying various existing code branches in the Bluetooth > > driver doesn't show any sign of making this work and I don't really know > > where to look to debug this further. > > > > On the other hand "discoverable on" makes the device show up on other > > devices during scanning , so the RF parts of the Bluetooth chip are > > generally functional for sure. > > > > Any ideas are welcome. > > > > @Bjorn: Patch "arm64: dts: qcom: sm6350: add uart1 node" should be fine > > to take regardless the RFC status, I don't think the problem is caused > > there. > > > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > > --- > > Luca Weiss (4): > > dt-bindings: net: qualcomm: Add WCN3988 > > Bluetooth: btqca: Add WCN3988 support > > arm64: dts: qcom: sm6350: add uart1 node > > arm64: dts: qcom: sm7225-fairphone-fp4: Add Bluetooth > > > > .../bindings/net/bluetooth/qualcomm-bluetooth.yaml | 2 + > > arch/arm64/boot/dts/qcom/sm6350.dtsi | 63 ++++++++++++++++++++++ > > arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts | 17 ++++++ > > drivers/bluetooth/btqca.c | 13 ++++- > > drivers/bluetooth/btqca.h | 12 ++++- > > drivers/bluetooth/hci_qca.c | 12 +++++ > > 6 files changed, 115 insertions(+), 4 deletions(-) > > --- > > base-commit: cf4c0112a0350cfe8a63b5eb3377e2366f57545b > > change-id: 20230421-fp4-bluetooth-b36a0e87b9c8 > > > > Best regards,
On 12/05/2023 16:30, Luca Weiss wrote: > On Sun Apr 23, 2023 at 12:51 PM CEST, Krzysztof Kozlowski wrote: >> On 21/04/2023 16:11, Luca Weiss wrote: >>> Add the node describing uart1 incl. opp table and pinctrl. >>> >>> Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> >>> --- >>> arch/arm64/boot/dts/qcom/sm6350.dtsi | 63 ++++++++++++++++++++++++++++++++++++ >>> 1 file changed, 63 insertions(+) >> >> Please do not send DTS patches for net-next. DTS must go via Qualcomm >> SoC. Split the series and mention where is the bindings change in DTS >> patchset. > > Sorry, just saw now after already sending v2. > > Is this a special rule for linux-bluetooth@ / netdev@? Isn't it easier > to keep it together so the status of series can be assessed easier? I've > always submitted patches by topic, like input patches + dts patches and > it was never mentioned. The rule that DTS must go via Qualcomm SoC (arm-soc) was there always, but other maintainers often do not pay attention to this. I don't blame them, don't get me wrong. I am just stating the observed actions. Usually netdev folks and Greg will take everything you throw at them, so for these subsystems it is recommended to split DTS to different patchset. For other maintainers it is usually also more useful to split, because then they can apply entire patchset with one command, instead of picking up specific patches (omitting DTS). Best regards, Krzysztof
On Fri, May 12, 2023 at 01:14:18PM +0200, Luca Weiss wrote: > Hi Simon, > > On Mon May 1, 2023 at 3:11 PM CEST, Simon Horman wrote: > > On Fri, Apr 21, 2023 at 04:11:39PM +0200, Luca Weiss wrote: > > > Add support for the Bluetooth chip codenamed APACHE which is part of > > > WCN3988. > > > > > > The firmware for this chip has a slightly different naming scheme > > > compared to most others. For ROM Version 0x0200 we need to use > > > apbtfw10.tlv + apnv10.bin and for ROM version 0x201 apbtfw11.tlv + > > > apnv11.bin > > > > > > Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> > > > --- > > > drivers/bluetooth/btqca.c | 13 +++++++++++-- > > > drivers/bluetooth/btqca.h | 12 ++++++++++-- > > > drivers/bluetooth/hci_qca.c | 12 ++++++++++++ > > > 3 files changed, 33 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c > > > index fd0941fe8608..3ee1ef88a640 100644 > > > --- a/drivers/bluetooth/btqca.c > > > +++ b/drivers/bluetooth/btqca.c > > > @@ -594,14 +594,20 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, > > > /* Firmware files to download are based on ROM version. > > > * ROM version is derived from last two bytes of soc_ver. > > > */ > > > - rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f); > > > + if (soc_type == QCA_WCN3988) > > > + rom_ver = ((soc_ver & 0x00000f00) >> 0x05) | (soc_ver & 0x0000000f); > > > + else > > > + rom_ver = ((soc_ver & 0x00000f00) >> 0x04) | (soc_ver & 0x0000000f); > > > > Hi Luca, > > > > perhaps it's just me. But I was wondering if this can be improved on a little. > > > > * Move the common portion outside of the conditional > > * And also, I think it's normal to use decimal for shift values. > > > > e.g. > > unsigned shift; > > ... > > > > shift = soc_type == QCA_WCN3988 ? 5 : 4; > > rom_ver = ((soc_ver & 0x00000f00) >> shift) | (soc_ver & 0x0000000f); > > > > Using some helpers such as GENMASK and FIELD_PREP might also be nice. > > While I'm not opposed to the idea, I'm not sure it's worth making > beautiful macros for this since - to my eyes - how the mapping of > soc_ver to firmware name works is rather obscure since the sources from > Qualcomm just have a static lookup table of soc_ver to firmware name so > doing this dynamically like here is different. > > And I haven't looked at other chips that are covered there to see if > there's a pattern to this, for the most part it seems the original > formula works for most chips and the one I added works for WCN3988 (and > the other "APACHE" chips, whatever they are). > > If a third way is added then I would say for sure this line should be > made nicer but for now I think it's easier to keep this as I sent it > because we don't know what the future will hold. Thanks. My feeling is that my suggestion mainly makes sense if it lease to improved readability and maintainability. It sounds like that might not be the case here. > > > if (soc_type == QCA_WCN6750) > > > qca_send_patch_config_cmd(hdev); > > > > > > /* Download rampatch file */ > > > config.type = TLV_TYPE_PATCH; > > > - if (qca_is_wcn399x(soc_type)) { > > > + if (soc_type == QCA_WCN3988) { > > > + snprintf(config.fwname, sizeof(config.fwname), > > > + "qca/apbtfw%02x.tlv", rom_ver); > > > + } else if (qca_is_wcn399x(soc_type)) { > > > snprintf(config.fwname, sizeof(config.fwname), > > > "qca/crbtfw%02x.tlv", rom_ver); > > > } else if (soc_type == QCA_QCA6390) { > > > @@ -636,6 +642,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate, > > > if (firmware_name) > > > snprintf(config.fwname, sizeof(config.fwname), > > > "qca/%s", firmware_name); > > > + else if (soc_type == QCA_WCN3988) > > > + snprintf(config.fwname, sizeof(config.fwname), > > > + "qca/apnv%02x.bin", rom_ver); > > > else if (qca_is_wcn399x(soc_type)) { > > > if (ver.soc_id == QCA_WCN3991_SOC_ID) { > > > > Not strictly related to this patch, but while reviewing this I noticed that > > ver.soc_id is __le32 but QCA_WCN3991_SOC_ID is in host byteorder. > > > > Perhaps a cpu_to_le32() or le32_to_cpu() call is in order here? > > Good catch, as you've seen I sent a patch separately to fix that. :) Thanks!
Just to start with the important part why this is an RFC: While Bluetooth chip init works totally fine and bluez seems to be fairly happy with it, there's a (major) problem with scanning, as shown with this bluetoothctl snippet and dmesg snippet: [bluetooth]# scan on Failed to start discovery: org.bluez.Error.InProgress [ 202.371374] Bluetooth: hci0: Opcode 0x200b failed: -16 This opcode should be the following: include/net/bluetooth/hci.h:#define HCI_OP_LE_SET_SCAN_PARAM 0x200b Unfortunately trying various existing code branches in the Bluetooth driver doesn't show any sign of making this work and I don't really know where to look to debug this further. On the other hand "discoverable on" makes the device show up on other devices during scanning , so the RF parts of the Bluetooth chip are generally functional for sure. Any ideas are welcome. @Bjorn: Patch "arm64: dts: qcom: sm6350: add uart1 node" should be fine to take regardless the RFC status, I don't think the problem is caused there. Signed-off-by: Luca Weiss <luca.weiss@fairphone.com> --- Luca Weiss (4): dt-bindings: net: qualcomm: Add WCN3988 Bluetooth: btqca: Add WCN3988 support arm64: dts: qcom: sm6350: add uart1 node arm64: dts: qcom: sm7225-fairphone-fp4: Add Bluetooth .../bindings/net/bluetooth/qualcomm-bluetooth.yaml | 2 + arch/arm64/boot/dts/qcom/sm6350.dtsi | 63 ++++++++++++++++++++++ arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts | 17 ++++++ drivers/bluetooth/btqca.c | 13 ++++- drivers/bluetooth/btqca.h | 12 ++++- drivers/bluetooth/hci_qca.c | 12 +++++ 6 files changed, 115 insertions(+), 4 deletions(-) --- base-commit: cf4c0112a0350cfe8a63b5eb3377e2366f57545b change-id: 20230421-fp4-bluetooth-b36a0e87b9c8 Best regards,