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 <[email protected]>
---
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);
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},
--
2.40.0
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 <[email protected]>
> ---
> 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 <[email protected]>
> > ---
> > 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),
>
> ...