diff mbox series

[RFC,2/4] Bluetooth: btqca: Add WCN3988 support

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

Checks

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

Commit Message

Luca Weiss April 21, 2023, 2:11 p.m. UTC
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(-)

Comments

Simon Horman May 1, 2023, 1:11 p.m. UTC | #1
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),

...
Luca Weiss May 12, 2023, 11:14 a.m. UTC | #2
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),
>
> ...
Simon Horman May 15, 2023, 11:30 a.m. UTC | #3
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 mbox series

Patch

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},