| Message ID | 20230421-fp4-bluetooth-v1-2-0430e3a7e0a2@fairphone.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add WCN3988 Bluetooth support for Fairphone 4 | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | success | Gitlint PASS |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
| tedd_an/IncrementalBuild | success | Incremental Build PASS |
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. > > 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? > snprintf(config.fwname, sizeof(config.fwname), ...
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. > > > > > 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. :) Regards Luca > > > snprintf(config.fwname, sizeof(config.fwname), > > ...
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!
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); 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) { snprintf(config.fwname, sizeof(config.fwname), diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h index b884095bcd9d..fc6cf314eb0e 100644 --- a/drivers/bluetooth/btqca.h +++ b/drivers/bluetooth/btqca.h @@ -142,6 +142,7 @@ enum qca_btsoc_type { QCA_INVALID = -1, QCA_AR3002, QCA_ROME, + QCA_WCN3988, QCA_WCN3990, QCA_WCN3998, QCA_WCN3991, @@ -162,8 +163,15 @@ int qca_set_bdaddr(struct hci_dev *hdev, const bdaddr_t *bdaddr); int qca_send_pre_shutdown_cmd(struct hci_dev *hdev); static inline bool qca_is_wcn399x(enum qca_btsoc_type soc_type) { - return soc_type == QCA_WCN3990 || soc_type == QCA_WCN3991 || - soc_type == QCA_WCN3998; + switch (soc_type) { + case QCA_WCN3988: + case QCA_WCN3990: + case QCA_WCN3991: + case QCA_WCN3998: + return true; + default: + return false; + } } static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type) { diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c index 1597797ff169..96b837410a6b 100644 --- a/drivers/bluetooth/hci_qca.c +++ b/drivers/bluetooth/hci_qca.c @@ -1835,6 +1835,17 @@ static const struct hci_uart_proto qca_proto = { .dequeue = qca_dequeue, }; +static const struct qca_device_data qca_soc_data_wcn3988 __maybe_unused = { + .soc_type = QCA_WCN3988, + .vregs = (struct qca_vreg []) { + { "vddio", 15000 }, + { "vddxo", 80000 }, + { "vddrf", 300000 }, + { "vddch0", 450000 }, + }, + .num_vregs = 4, +}; + static const struct qca_device_data qca_soc_data_wcn3990 __maybe_unused = { .soc_type = QCA_WCN3990, .vregs = (struct qca_vreg []) { @@ -2359,6 +2370,7 @@ static const struct of_device_id qca_bluetooth_of_match[] = { { .compatible = "qcom,qca6174-bt" }, { .compatible = "qcom,qca6390-bt", .data = &qca_soc_data_qca6390}, { .compatible = "qcom,qca9377-bt" }, + { .compatible = "qcom,wcn3988-bt", .data = &qca_soc_data_wcn3988}, { .compatible = "qcom,wcn3990-bt", .data = &qca_soc_data_wcn3990}, { .compatible = "qcom,wcn3991-bt", .data = &qca_soc_data_wcn3991}, { .compatible = "qcom,wcn3998-bt", .data = &qca_soc_data_wcn3998},
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(-)