diff mbox series

[v5,2/4] Bluetooth: hci_qca: Add support for QTI Bluetooth chip wcn6855

Message ID 20230209020916.6475-3-steev@kali.org
State New
Headers show
Series Add WCN6855 Bluetooth support | expand

Commit Message

Steev Klimaszewski Feb. 9, 2023, 2:09 a.m. UTC
Added regulators,GPIOs and changes required to power on/off wcn6855.
Added support for firmware download for wcn6855.

Signed-off-by: Steev Klimaszewski <steev@kali.org>
---
Changes since v4:
 * Remove unused firmware check because we don't have mbn firmware.
 * Set qcadev->init_speed if it hasn't been set.

Changes since v3:
 * drop unused regulators

Changes since v2:
 * drop unnecessary commit info

Changes since v1:
 * None

 drivers/bluetooth/btqca.c   |  9 ++++++-
 drivers/bluetooth/btqca.h   | 10 ++++++++
 drivers/bluetooth/hci_qca.c | 50 ++++++++++++++++++++++++++++---------
 3 files changed, 56 insertions(+), 13 deletions(-)

Comments

Bjorn Andersson Feb. 10, 2023, 5:05 p.m. UTC | #1
On Wed, Feb 08, 2023 at 08:09:14PM -0600, Steev Klimaszewski wrote:
> Added regulators,GPIOs and changes required to power on/off wcn6855.
> Added support for firmware download for wcn6855.
> 
> Signed-off-by: Steev Klimaszewski <steev@kali.org>

Reviewed-by: Bjorn Andersson <andersson@kernel.org>
Tested-by: Bjorn Andersson <andersson@kernel.org>

Regards,
Bjorn

> ---
> Changes since v4:
>  * Remove unused firmware check because we don't have mbn firmware.
>  * Set qcadev->init_speed if it hasn't been set.
> 
> Changes since v3:
>  * drop unused regulators
> 
> Changes since v2:
>  * drop unnecessary commit info
> 
> Changes since v1:
>  * None
> 
>  drivers/bluetooth/btqca.c   |  9 ++++++-
>  drivers/bluetooth/btqca.h   | 10 ++++++++
>  drivers/bluetooth/hci_qca.c | 50 ++++++++++++++++++++++++++++---------
>  3 files changed, 56 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index c9064d34d830..2f9d8bd27c38 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -614,6 +614,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  		config.type = ELF_TYPE_PATCH;
>  		snprintf(config.fwname, sizeof(config.fwname),
>  			 "qca/msbtfw%02x.mbn", rom_ver);
> +	} else if (soc_type == QCA_WCN6855) {
> +		snprintf(config.fwname, sizeof(config.fwname),
> +			 "qca/hpbtfw%02x.tlv", rom_ver);
>  	} else {
>  		snprintf(config.fwname, sizeof(config.fwname),
>  			 "qca/rampatch_%08x.bin", soc_ver);
> @@ -648,6 +651,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  	else if (soc_type == QCA_WCN6750)
>  		snprintf(config.fwname, sizeof(config.fwname),
>  			 "qca/msnv%02x.bin", rom_ver);
> +	else if (soc_type == QCA_WCN6855)
> +		snprintf(config.fwname, sizeof(config.fwname),
> +			 "qca/hpnv%02x.bin", rom_ver);
>  	else
>  		snprintf(config.fwname, sizeof(config.fwname),
>  			 "qca/nvm_%08x.bin", soc_ver);
> @@ -672,6 +678,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  	case QCA_WCN3991:
>  	case QCA_WCN3998:
>  	case QCA_WCN6750:
> +	case QCA_WCN6855:
>  		hci_set_msft_opcode(hdev, 0xFD70);
>  		break;
>  	default:
> @@ -685,7 +692,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  		return err;
>  	}
>  
> -	if (soc_type == QCA_WCN3991 || soc_type == QCA_WCN6750) {
> +	if (soc_type == QCA_WCN3991 || soc_type == QCA_WCN6750 || soc_type == QCA_WCN6855) {
>  		/* get fw build info */
>  		err = qca_read_fw_build_info(hdev);
>  		if (err < 0)
> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> index 61e9a50e66ae..b884095bcd9d 100644
> --- a/drivers/bluetooth/btqca.h
> +++ b/drivers/bluetooth/btqca.h
> @@ -147,6 +147,7 @@ enum qca_btsoc_type {
>  	QCA_WCN3991,
>  	QCA_QCA6390,
>  	QCA_WCN6750,
> +	QCA_WCN6855,
>  };
>  
>  #if IS_ENABLED(CONFIG_BT_QCA)
> @@ -168,6 +169,10 @@ static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type)
>  {
>  	return soc_type == QCA_WCN6750;
>  }
> +static inline bool qca_is_wcn6855(enum qca_btsoc_type soc_type)
> +{
> +	return soc_type == QCA_WCN6855;
> +}
>  
>  #else
>  
> @@ -206,6 +211,11 @@ static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type)
>  	return false;
>  }
>  
> +static inline bool qca_is_wcn6855(enum qca_btsoc_type soc_type)
> +{
> +	return false;
> +}
> +
>  static inline int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
>  {
>  	return -EOPNOTSUPP;
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 3df8c3606e93..efc1c0306b4e 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -605,8 +605,7 @@ static int qca_open(struct hci_uart *hu)
>  	if (hu->serdev) {
>  		qcadev = serdev_device_get_drvdata(hu->serdev);
>  
> -		if (qca_is_wcn399x(qcadev->btsoc_type) ||
> -		    qca_is_wcn6750(qcadev->btsoc_type))
> +		if (!(qcadev->init_speed))
>  			hu->init_speed = qcadev->init_speed;
>  
>  		if (qcadev->oper_speed)
> @@ -1317,7 +1316,8 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
>  
>  	/* Give the controller time to process the request */
>  	if (qca_is_wcn399x(qca_soc_type(hu)) ||
> -	    qca_is_wcn6750(qca_soc_type(hu)))
> +	    qca_is_wcn6750(qca_soc_type(hu)) ||
> +	    qca_is_wcn6855(qca_soc_type(hu)))
>  		usleep_range(1000, 10000);
>  	else
>  		msleep(300);
> @@ -1394,7 +1394,8 @@ static unsigned int qca_get_speed(struct hci_uart *hu,
>  static int qca_check_speeds(struct hci_uart *hu)
>  {
>  	if (qca_is_wcn399x(qca_soc_type(hu)) ||
> -	    qca_is_wcn6750(qca_soc_type(hu))) {
> +	    qca_is_wcn6750(qca_soc_type(hu)) ||
> +	    qca_is_wcn6855(qca_soc_type(hu))) {
>  		if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
>  		    !qca_get_speed(hu, QCA_OPER_SPEED))
>  			return -EINVAL;
> @@ -1682,7 +1683,8 @@ static int qca_power_on(struct hci_dev *hdev)
>  		return 0;
>  
>  	if (qca_is_wcn399x(soc_type) ||
> -	    qca_is_wcn6750(soc_type)) {
> +	    qca_is_wcn6750(soc_type) ||
> +	    qca_is_wcn6855(soc_type)) {
>  		ret = qca_regulator_init(hu);
>  	} else {
>  		qcadev = serdev_device_get_drvdata(hu->serdev);
> @@ -1723,7 +1725,8 @@ static int qca_setup(struct hci_uart *hu)
>  
>  	bt_dev_info(hdev, "setting up %s",
>  		qca_is_wcn399x(soc_type) ? "wcn399x" :
> -		(soc_type == QCA_WCN6750) ? "wcn6750" : "ROME/QCA6390");
> +		(soc_type == QCA_WCN6750) ? "wcn6750" :
> +		(soc_type == QCA_WCN6855) ? "wcn6855" : "ROME/QCA6390");
>  
>  	qca->memdump_state = QCA_MEMDUMP_IDLE;
>  
> @@ -1735,7 +1738,8 @@ static int qca_setup(struct hci_uart *hu)
>  	clear_bit(QCA_SSR_TRIGGERED, &qca->flags);
>  
>  	if (qca_is_wcn399x(soc_type) ||
> -	    qca_is_wcn6750(soc_type)) {
> +	    qca_is_wcn6750(soc_type) ||
> +	    qca_is_wcn6855(soc_type)) {
>  		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
>  		hci_set_aosp_capable(hdev);
>  
> @@ -1757,7 +1761,8 @@ static int qca_setup(struct hci_uart *hu)
>  	}
>  
>  	if (!(qca_is_wcn399x(soc_type) ||
> -	     qca_is_wcn6750(soc_type))) {
> +	     qca_is_wcn6750(soc_type) ||
> +	     qca_is_wcn6855(soc_type))) {
>  		/* Get QCA version information */
>  		ret = qca_read_soc_version(hdev, &ver, soc_type);
>  		if (ret)
> @@ -1883,6 +1888,20 @@ static const struct qca_device_data qca_soc_data_wcn6750 = {
>  	.capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
>  };
>  
> +static const struct qca_device_data qca_soc_data_wcn6855 = {
> +	.soc_type = QCA_WCN6855,
> +	.vregs = (struct qca_vreg []) {
> +		{ "vddio", 5000 },
> +		{ "vddbtcxmx", 126000 },
> +		{ "vddrfacmn", 12500 },
> +		{ "vddrfa0p8", 102000 },
> +		{ "vddrfa1p7", 302000 },
> +		{ "vddrfa1p2", 257000 },
> +	},
> +	.num_vregs = 6,
> +	.capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
> +};
> +
>  static void qca_power_shutdown(struct hci_uart *hu)
>  {
>  	struct qca_serdev *qcadev;
> @@ -2047,7 +2066,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  
>  	if (data &&
>  	    (qca_is_wcn399x(data->soc_type) ||
> -	    qca_is_wcn6750(data->soc_type))) {
> +	    qca_is_wcn6750(data->soc_type) ||
> +	    qca_is_wcn6855(data->soc_type))) {
>  		qcadev->btsoc_type = data->soc_type;
>  		qcadev->bt_power = devm_kzalloc(&serdev->dev,
>  						sizeof(struct qca_power),
> @@ -2067,14 +2087,18 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  
>  		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>  					       GPIOD_OUT_LOW);
> -		if (IS_ERR_OR_NULL(qcadev->bt_en) && data->soc_type == QCA_WCN6750) {
> +		if (IS_ERR_OR_NULL(qcadev->bt_en)
> +		    && (data->soc_type == QCA_WCN6750 ||
> +			data->soc_type == QCA_WCN6855)) {
>  			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
>  			power_ctrl_enabled = false;
>  		}
>  
>  		qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
>  					       GPIOD_IN);
> -		if (IS_ERR_OR_NULL(qcadev->sw_ctrl) && data->soc_type == QCA_WCN6750)
> +		if (IS_ERR_OR_NULL(qcadev->sw_ctrl)
> +		    && (data->soc_type == QCA_WCN6750 ||
> +			data->soc_type == QCA_WCN6855))
>  			dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
>  
>  		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
> @@ -2150,7 +2174,8 @@ static void qca_serdev_remove(struct serdev_device *serdev)
>  	struct qca_power *power = qcadev->bt_power;
>  
>  	if ((qca_is_wcn399x(qcadev->btsoc_type) ||
> -	     qca_is_wcn6750(qcadev->btsoc_type)) &&
> +	     qca_is_wcn6750(qcadev->btsoc_type) ||
> +	     qca_is_wcn6855(qcadev->btsoc_type)) &&
>  	     power->vregs_on)
>  		qca_power_shutdown(&qcadev->serdev_hu);
>  	else if (qcadev->susclk)
> @@ -2335,6 +2360,7 @@ static const struct of_device_id qca_bluetooth_of_match[] = {
>  	{ .compatible = "qcom,wcn3991-bt", .data = &qca_soc_data_wcn3991},
>  	{ .compatible = "qcom,wcn3998-bt", .data = &qca_soc_data_wcn3998},
>  	{ .compatible = "qcom,wcn6750-bt", .data = &qca_soc_data_wcn6750},
> +	{ .compatible = "qcom,wcn6855-bt", .data = &qca_soc_data_wcn6855},
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, qca_bluetooth_of_match);
> -- 
> 2.39.1
>
Johan Hovold March 9, 2023, 5:09 p.m. UTC | #2
On Wed, Feb 08, 2023 at 08:09:14PM -0600, Steev Klimaszewski wrote:
> Added regulators,GPIOs and changes required to power on/off wcn6855.
> Added support for firmware download for wcn6855.
> 
> Signed-off-by: Steev Klimaszewski <steev@kali.org>
> ---
> Changes since v4:
>  * Remove unused firmware check because we don't have mbn firmware.
>  * Set qcadev->init_speed if it hasn't been set.
> 
> Changes since v3:
>  * drop unused regulators
> 
> Changes since v2:
>  * drop unnecessary commit info
> 
> Changes since v1:
>  * None
> 
>  drivers/bluetooth/btqca.c   |  9 ++++++-
>  drivers/bluetooth/btqca.h   | 10 ++++++++
>  drivers/bluetooth/hci_qca.c | 50 ++++++++++++++++++++++++++++---------
>  3 files changed, 56 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> index c9064d34d830..2f9d8bd27c38 100644
> --- a/drivers/bluetooth/btqca.c
> +++ b/drivers/bluetooth/btqca.c
> @@ -614,6 +614,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  		config.type = ELF_TYPE_PATCH;
>  		snprintf(config.fwname, sizeof(config.fwname),
>  			 "qca/msbtfw%02x.mbn", rom_ver);
> +	} else if (soc_type == QCA_WCN6855) {
> +		snprintf(config.fwname, sizeof(config.fwname),
> +			 "qca/hpbtfw%02x.tlv", rom_ver);
>  	} else {
>  		snprintf(config.fwname, sizeof(config.fwname),
>  			 "qca/rampatch_%08x.bin", soc_ver);
> @@ -648,6 +651,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  	else if (soc_type == QCA_WCN6750)
>  		snprintf(config.fwname, sizeof(config.fwname),
>  			 "qca/msnv%02x.bin", rom_ver);
> +	else if (soc_type == QCA_WCN6855)
> +		snprintf(config.fwname, sizeof(config.fwname),
> +			 "qca/hpnv%02x.bin", rom_ver);
>  	else
>  		snprintf(config.fwname, sizeof(config.fwname),
>  			 "qca/nvm_%08x.bin", soc_ver);
> @@ -672,6 +678,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  	case QCA_WCN3991:
>  	case QCA_WCN3998:
>  	case QCA_WCN6750:
> +	case QCA_WCN6855:

Did you actually verify the microsoft extensions need this, or you are
assuming it works as 6750?

>  		hci_set_msft_opcode(hdev, 0xFD70);
>  		break;
>  	default:
> @@ -685,7 +692,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
>  		return err;
>  	}
>  
> -	if (soc_type == QCA_WCN3991 || soc_type == QCA_WCN6750) {
> +	if (soc_type == QCA_WCN3991 || soc_type == QCA_WCN6750 || soc_type == QCA_WCN6855) {

Line is now over 80 columns which is still the preferred limit.

Perhaps this should now be a switch statement instead?

>  		/* get fw build info */
>  		err = qca_read_fw_build_info(hdev);
>  		if (err < 0)
> diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> index 61e9a50e66ae..b884095bcd9d 100644
> --- a/drivers/bluetooth/btqca.h
> +++ b/drivers/bluetooth/btqca.h
> @@ -147,6 +147,7 @@ enum qca_btsoc_type {
>  	QCA_WCN3991,
>  	QCA_QCA6390,
>  	QCA_WCN6750,
> +	QCA_WCN6855,
>  };
>  
>  #if IS_ENABLED(CONFIG_BT_QCA)
> @@ -168,6 +169,10 @@ static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type)
>  {
>  	return soc_type == QCA_WCN6750;
>  }
> +static inline bool qca_is_wcn6855(enum qca_btsoc_type soc_type)
> +{
> +	return soc_type == QCA_WCN6855;
> +}
>  
>  #else
>  
> @@ -206,6 +211,11 @@ static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type)
>  	return false;
>  }
>  
> +static inline bool qca_is_wcn6855(enum qca_btsoc_type soc_type)
> +{
> +	return false;
> +}
> +
>  static inline int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
>  {
>  	return -EOPNOTSUPP;
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 3df8c3606e93..efc1c0306b4e 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -605,8 +605,7 @@ static int qca_open(struct hci_uart *hu)
>  	if (hu->serdev) {
>  		qcadev = serdev_device_get_drvdata(hu->serdev);
>  
> -		if (qca_is_wcn399x(qcadev->btsoc_type) ||
> -		    qca_is_wcn6750(qcadev->btsoc_type))
> +		if (!(qcadev->init_speed))
>  			hu->init_speed = qcadev->init_speed;

This change makes no sense. 

In fact, it seems the driver never sets init_speed anywhere.

Either way, it should not be needed for wcn6855.

>  
>  		if (qcadev->oper_speed)
> @@ -1317,7 +1316,8 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
>  
>  	/* Give the controller time to process the request */
>  	if (qca_is_wcn399x(qca_soc_type(hu)) ||
> -	    qca_is_wcn6750(qca_soc_type(hu)))
> +	    qca_is_wcn6750(qca_soc_type(hu)) ||
> +	    qca_is_wcn6855(qca_soc_type(hu)))
>  		usleep_range(1000, 10000);
>  	else
>  		msleep(300);
> @@ -1394,7 +1394,8 @@ static unsigned int qca_get_speed(struct hci_uart *hu,
>  static int qca_check_speeds(struct hci_uart *hu)
>  {
>  	if (qca_is_wcn399x(qca_soc_type(hu)) ||
> -	    qca_is_wcn6750(qca_soc_type(hu))) {
> +	    qca_is_wcn6750(qca_soc_type(hu)) ||
> +	    qca_is_wcn6855(qca_soc_type(hu))) {
>  		if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
>  		    !qca_get_speed(hu, QCA_OPER_SPEED))
>  			return -EINVAL;
> @@ -1682,7 +1683,8 @@ static int qca_power_on(struct hci_dev *hdev)
>  		return 0;
>  
>  	if (qca_is_wcn399x(soc_type) ||
> -	    qca_is_wcn6750(soc_type)) {
> +	    qca_is_wcn6750(soc_type) ||
> +	    qca_is_wcn6855(soc_type)) {
>  		ret = qca_regulator_init(hu);
>  	} else {
>  		qcadev = serdev_device_get_drvdata(hu->serdev);
> @@ -1723,7 +1725,8 @@ static int qca_setup(struct hci_uart *hu)
>  
>  	bt_dev_info(hdev, "setting up %s",
>  		qca_is_wcn399x(soc_type) ? "wcn399x" :
> -		(soc_type == QCA_WCN6750) ? "wcn6750" : "ROME/QCA6390");
> +		(soc_type == QCA_WCN6750) ? "wcn6750" :
> +		(soc_type == QCA_WCN6855) ? "wcn6855" : "ROME/QCA6390");

This is hideous, but not your fault...

>  
>  	qca->memdump_state = QCA_MEMDUMP_IDLE;
>  
> @@ -1735,7 +1738,8 @@ static int qca_setup(struct hci_uart *hu)
>  	clear_bit(QCA_SSR_TRIGGERED, &qca->flags);
>  
>  	if (qca_is_wcn399x(soc_type) ||
> -	    qca_is_wcn6750(soc_type)) {
> +	    qca_is_wcn6750(soc_type) ||
> +	    qca_is_wcn6855(soc_type)) {
>  		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
>  		hci_set_aosp_capable(hdev);
>  
> @@ -1757,7 +1761,8 @@ static int qca_setup(struct hci_uart *hu)
>  	}
>  
>  	if (!(qca_is_wcn399x(soc_type) ||
> -	     qca_is_wcn6750(soc_type))) {
> +	     qca_is_wcn6750(soc_type) ||
> +	     qca_is_wcn6855(soc_type))) {

Perhaps you can add a leading space while changing this so that the
open-parenthesis alignment makes sense.

>  		/* Get QCA version information */
>  		ret = qca_read_soc_version(hdev, &ver, soc_type);
>  		if (ret)
> @@ -1883,6 +1888,20 @@ static const struct qca_device_data qca_soc_data_wcn6750 = {
>  	.capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
>  };
>  
> +static const struct qca_device_data qca_soc_data_wcn6855 = {
> +	.soc_type = QCA_WCN6855,
> +	.vregs = (struct qca_vreg []) {
> +		{ "vddio", 5000 },
> +		{ "vddbtcxmx", 126000 },
> +		{ "vddrfacmn", 12500 },
> +		{ "vddrfa0p8", 102000 },
> +		{ "vddrfa1p7", 302000 },
> +		{ "vddrfa1p2", 257000 },

Hmm. More random regulator load values. I really think we should get rid
of this but that's a separate discussion.

> +	},
> +	.num_vregs = 6,
> +	.capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
> +};
> +
>  static void qca_power_shutdown(struct hci_uart *hu)
>  {
>  	struct qca_serdev *qcadev;

As I mentioned elsewhere, you need to update also this function so that
wcn6855 can be powered down.

> @@ -2047,7 +2066,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  
>  	if (data &&
>  	    (qca_is_wcn399x(data->soc_type) ||
> -	    qca_is_wcn6750(data->soc_type))) {
> +	    qca_is_wcn6750(data->soc_type) ||
> +	    qca_is_wcn6855(data->soc_type))) {

Perhaps you fix the alignment here too.

>  		qcadev->btsoc_type = data->soc_type;
>  		qcadev->bt_power = devm_kzalloc(&serdev->dev,
>  						sizeof(struct qca_power),
> @@ -2067,14 +2087,18 @@ static int qca_serdev_probe(struct serdev_device *serdev)
>  
>  		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
>  					       GPIOD_OUT_LOW);
> -		if (IS_ERR_OR_NULL(qcadev->bt_en) && data->soc_type == QCA_WCN6750) {
> +		if (IS_ERR_OR_NULL(qcadev->bt_en)
> +		    && (data->soc_type == QCA_WCN6750 ||

&& operator should go on the previous line before the line break.

> +			data->soc_type == QCA_WCN6855)) {
>  			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
>  			power_ctrl_enabled = false;
>  		}
>  
>  		qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
>  					       GPIOD_IN);
> -		if (IS_ERR_OR_NULL(qcadev->sw_ctrl) && data->soc_type == QCA_WCN6750)
> +		if (IS_ERR_OR_NULL(qcadev->sw_ctrl)
> +		    && (data->soc_type == QCA_WCN6750 ||

Same here.

> +			data->soc_type == QCA_WCN6855))
>  			dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
>  
>  		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
> @@ -2150,7 +2174,8 @@ static void qca_serdev_remove(struct serdev_device *serdev)
>  	struct qca_power *power = qcadev->bt_power;
>  
>  	if ((qca_is_wcn399x(qcadev->btsoc_type) ||
> -	     qca_is_wcn6750(qcadev->btsoc_type)) &&
> +	     qca_is_wcn6750(qcadev->btsoc_type) ||
> +	     qca_is_wcn6855(qcadev->btsoc_type)) &&
>  	     power->vregs_on)
>  		qca_power_shutdown(&qcadev->serdev_hu);
>  	else if (qcadev->susclk)
> @@ -2335,6 +2360,7 @@ static const struct of_device_id qca_bluetooth_of_match[] = {
>  	{ .compatible = "qcom,wcn3991-bt", .data = &qca_soc_data_wcn3991},
>  	{ .compatible = "qcom,wcn3998-bt", .data = &qca_soc_data_wcn3998},
>  	{ .compatible = "qcom,wcn6750-bt", .data = &qca_soc_data_wcn6750},
> +	{ .compatible = "qcom,wcn6855-bt", .data = &qca_soc_data_wcn6855},
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(of, qca_bluetooth_of_match);

With power-off handling fixed, this seems to work as quite well on my
X13s with 6.3-rc1. Nice job!

Btw, apart from the frame reassembly error, I'm also seeing:

	Bluetooth: Received HCI_IBS_WAKE_ACK in tx state 0

during probe.

Johan
Steev Klimaszewski March 9, 2023, 8:24 p.m. UTC | #3
On Thu, Mar 9, 2023 at 11:08 AM Johan Hovold <johan@kernel.org> wrote:
>
> On Wed, Feb 08, 2023 at 08:09:14PM -0600, Steev Klimaszewski wrote:
> > Added regulators,GPIOs and changes required to power on/off wcn6855.
> > Added support for firmware download for wcn6855.
> >
> > Signed-off-by: Steev Klimaszewski <steev@kali.org>
> > ---
> > Changes since v4:
> >  * Remove unused firmware check because we don't have mbn firmware.
> >  * Set qcadev->init_speed if it hasn't been set.
> >
> > Changes since v3:
> >  * drop unused regulators
> >
> > Changes since v2:
> >  * drop unnecessary commit info
> >
> > Changes since v1:
> >  * None
> >
> >  drivers/bluetooth/btqca.c   |  9 ++++++-
> >  drivers/bluetooth/btqca.h   | 10 ++++++++
> >  drivers/bluetooth/hci_qca.c | 50 ++++++++++++++++++++++++++++---------
> >  3 files changed, 56 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
> > index c9064d34d830..2f9d8bd27c38 100644
> > --- a/drivers/bluetooth/btqca.c
> > +++ b/drivers/bluetooth/btqca.c
> > @@ -614,6 +614,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> >               config.type = ELF_TYPE_PATCH;
> >               snprintf(config.fwname, sizeof(config.fwname),
> >                        "qca/msbtfw%02x.mbn", rom_ver);
> > +     } else if (soc_type == QCA_WCN6855) {
> > +             snprintf(config.fwname, sizeof(config.fwname),
> > +                      "qca/hpbtfw%02x.tlv", rom_ver);
> >       } else {
> >               snprintf(config.fwname, sizeof(config.fwname),
> >                        "qca/rampatch_%08x.bin", soc_ver);
> > @@ -648,6 +651,9 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> >       else if (soc_type == QCA_WCN6750)
> >               snprintf(config.fwname, sizeof(config.fwname),
> >                        "qca/msnv%02x.bin", rom_ver);
> > +     else if (soc_type == QCA_WCN6855)
> > +             snprintf(config.fwname, sizeof(config.fwname),
> > +                      "qca/hpnv%02x.bin", rom_ver);
> >       else
> >               snprintf(config.fwname, sizeof(config.fwname),
> >                        "qca/nvm_%08x.bin", soc_ver);
> > @@ -672,6 +678,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> >       case QCA_WCN3991:
> >       case QCA_WCN3998:
> >       case QCA_WCN6750:
> > +     case QCA_WCN6855:
>
> Did you actually verify the microsoft extensions need this, or you are
> assuming it works as 6750?
>
It was 100% an assumption that since the 6750 does it, the 6855 does
too.  I should know better than to assume since I used to work at a
device manufacturer but high hopes things have changed a bit in the
past 12 years ;)

> >               hci_set_msft_opcode(hdev, 0xFD70);
> >               break;
> >       default:
> > @@ -685,7 +692,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> >               return err;
> >       }
> >
> > -     if (soc_type == QCA_WCN3991 || soc_type == QCA_WCN6750) {
> > +     if (soc_type == QCA_WCN3991 || soc_type == QCA_WCN6750 || soc_type == QCA_WCN6855) {
>
> Line is now over 80 columns which is still the preferred limit.
>
> Perhaps this should now be a switch statement instead?
>
switch statement might work, I'll give it a shot here.

> >               /* get fw build info */
> >               err = qca_read_fw_build_info(hdev);
> >               if (err < 0)
> > diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
> > index 61e9a50e66ae..b884095bcd9d 100644
> > --- a/drivers/bluetooth/btqca.h
> > +++ b/drivers/bluetooth/btqca.h
> > @@ -147,6 +147,7 @@ enum qca_btsoc_type {
> >       QCA_WCN3991,
> >       QCA_QCA6390,
> >       QCA_WCN6750,
> > +     QCA_WCN6855,
> >  };
> >
> >  #if IS_ENABLED(CONFIG_BT_QCA)
> > @@ -168,6 +169,10 @@ static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type)
> >  {
> >       return soc_type == QCA_WCN6750;
> >  }
> > +static inline bool qca_is_wcn6855(enum qca_btsoc_type soc_type)
> > +{
> > +     return soc_type == QCA_WCN6855;
> > +}
> >
> >  #else
> >
> > @@ -206,6 +211,11 @@ static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type)
> >       return false;
> >  }
> >
> > +static inline bool qca_is_wcn6855(enum qca_btsoc_type soc_type)
> > +{
> > +     return false;
> > +}
> > +
> >  static inline int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
> >  {
> >       return -EOPNOTSUPP;
> > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > index 3df8c3606e93..efc1c0306b4e 100644
> > --- a/drivers/bluetooth/hci_qca.c
> > +++ b/drivers/bluetooth/hci_qca.c
> > @@ -605,8 +605,7 @@ static int qca_open(struct hci_uart *hu)
> >       if (hu->serdev) {
> >               qcadev = serdev_device_get_drvdata(hu->serdev);
> >
> > -             if (qca_is_wcn399x(qcadev->btsoc_type) ||
> > -                 qca_is_wcn6750(qcadev->btsoc_type))
> > +             if (!(qcadev->init_speed))
> >                       hu->init_speed = qcadev->init_speed;
>
> This change makes no sense.
>
> In fact, it seems the driver never sets init_speed anywhere.
>
> Either way, it should not be needed for wcn6855.
>

So, that was a request from an earlier review, but if it's not needed
for 6855, I'll just drop it, and then I don't need to do any of those
changes :D

> >
> >               if (qcadev->oper_speed)
> > @@ -1317,7 +1316,8 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
> >
> >       /* Give the controller time to process the request */
> >       if (qca_is_wcn399x(qca_soc_type(hu)) ||
> > -         qca_is_wcn6750(qca_soc_type(hu)))
> > +         qca_is_wcn6750(qca_soc_type(hu)) ||
> > +         qca_is_wcn6855(qca_soc_type(hu)))
> >               usleep_range(1000, 10000);
> >       else
> >               msleep(300);
> > @@ -1394,7 +1394,8 @@ static unsigned int qca_get_speed(struct hci_uart *hu,
> >  static int qca_check_speeds(struct hci_uart *hu)
> >  {
> >       if (qca_is_wcn399x(qca_soc_type(hu)) ||
> > -         qca_is_wcn6750(qca_soc_type(hu))) {
> > +         qca_is_wcn6750(qca_soc_type(hu)) ||
> > +         qca_is_wcn6855(qca_soc_type(hu))) {
> >               if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
> >                   !qca_get_speed(hu, QCA_OPER_SPEED))
> >                       return -EINVAL;
> > @@ -1682,7 +1683,8 @@ static int qca_power_on(struct hci_dev *hdev)
> >               return 0;
> >
> >       if (qca_is_wcn399x(soc_type) ||
> > -         qca_is_wcn6750(soc_type)) {
> > +         qca_is_wcn6750(soc_type) ||
> > +         qca_is_wcn6855(soc_type)) {
> >               ret = qca_regulator_init(hu);
> >       } else {
> >               qcadev = serdev_device_get_drvdata(hu->serdev);
> > @@ -1723,7 +1725,8 @@ static int qca_setup(struct hci_uart *hu)
> >
> >       bt_dev_info(hdev, "setting up %s",
> >               qca_is_wcn399x(soc_type) ? "wcn399x" :
> > -             (soc_type == QCA_WCN6750) ? "wcn6750" : "ROME/QCA6390");
> > +             (soc_type == QCA_WCN6750) ? "wcn6750" :
> > +             (soc_type == QCA_WCN6855) ? "wcn6855" : "ROME/QCA6390");
>
> This is hideous, but not your fault...
>
It is, and, I'm not entirely sure we need it? I mean, it's nice to
show that it's now starting to set up, but it isn't particularly
helpful for end users or making sure things are working?

> >
> >       qca->memdump_state = QCA_MEMDUMP_IDLE;
> >
> > @@ -1735,7 +1738,8 @@ static int qca_setup(struct hci_uart *hu)
> >       clear_bit(QCA_SSR_TRIGGERED, &qca->flags);
> >
> >       if (qca_is_wcn399x(soc_type) ||
> > -         qca_is_wcn6750(soc_type)) {
> > +         qca_is_wcn6750(soc_type) ||
> > +         qca_is_wcn6855(soc_type)) {
> >               set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
> >               hci_set_aosp_capable(hdev);
> >
> > @@ -1757,7 +1761,8 @@ static int qca_setup(struct hci_uart *hu)
> >       }
> >
> >       if (!(qca_is_wcn399x(soc_type) ||
> > -          qca_is_wcn6750(soc_type))) {
> > +          qca_is_wcn6750(soc_type) ||
> > +          qca_is_wcn6855(soc_type))) {
>
> Perhaps you can add a leading space while changing this so that the
> open-parenthesis alignment makes sense.
>
> >               /* Get QCA version information */
> >               ret = qca_read_soc_version(hdev, &ver, soc_type);
> >               if (ret)
> > @@ -1883,6 +1888,20 @@ static const struct qca_device_data qca_soc_data_wcn6750 = {
> >       .capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
> >  };
> >
> > +static const struct qca_device_data qca_soc_data_wcn6855 = {
> > +     .soc_type = QCA_WCN6855,
> > +     .vregs = (struct qca_vreg []) {
> > +             { "vddio", 5000 },
> > +             { "vddbtcxmx", 126000 },
> > +             { "vddrfacmn", 12500 },
> > +             { "vddrfa0p8", 102000 },
> > +             { "vddrfa1p7", 302000 },
> > +             { "vddrfa1p2", 257000 },
>
> Hmm. More random regulator load values. I really think we should get rid
> of this but that's a separate discussion.
>
Bjorn specifically requested that he wanted me to leave them in.  I'm
not married to them, and don't care one way or the other, I just
wanted working bluetooth since audio wasn't quite ready yet :)

> > +     },
> > +     .num_vregs = 6,
> > +     .capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
> > +};
> > +
> >  static void qca_power_shutdown(struct hci_uart *hu)
> >  {
> >       struct qca_serdev *qcadev;
>
> As I mentioned elsewhere, you need to update also this function so that
> wcn6855 can be powered down.

Sorry, I do have that locally, I just haven't pushed a v6 as I was
looking at Tim's v2 of the qca2066 and was wondering if I should or
shouldn't continue working on my version of the driver?

>
> > @@ -2047,7 +2066,8 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> >
> >       if (data &&
> >           (qca_is_wcn399x(data->soc_type) ||
> > -         qca_is_wcn6750(data->soc_type))) {
> > +         qca_is_wcn6750(data->soc_type) ||
> > +         qca_is_wcn6855(data->soc_type))) {
>
> Perhaps you fix the alignment here too.
>
> >               qcadev->btsoc_type = data->soc_type;
> >               qcadev->bt_power = devm_kzalloc(&serdev->dev,
> >                                               sizeof(struct qca_power),
> > @@ -2067,14 +2087,18 @@ static int qca_serdev_probe(struct serdev_device *serdev)
> >
> >               qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
> >                                              GPIOD_OUT_LOW);
> > -             if (IS_ERR_OR_NULL(qcadev->bt_en) && data->soc_type == QCA_WCN6750) {
> > +             if (IS_ERR_OR_NULL(qcadev->bt_en)
> > +                 && (data->soc_type == QCA_WCN6750 ||
>
> && operator should go on the previous line before the line break.
>
> > +                     data->soc_type == QCA_WCN6855)) {
> >                       dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
> >                       power_ctrl_enabled = false;
> >               }
> >
> >               qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
> >                                              GPIOD_IN);
> > -             if (IS_ERR_OR_NULL(qcadev->sw_ctrl) && data->soc_type == QCA_WCN6750)
> > +             if (IS_ERR_OR_NULL(qcadev->sw_ctrl)
> > +                 && (data->soc_type == QCA_WCN6750 ||
>
> Same here.
>
> > +                     data->soc_type == QCA_WCN6855))
> >                       dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
> >
> >               qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
> > @@ -2150,7 +2174,8 @@ static void qca_serdev_remove(struct serdev_device *serdev)
> >       struct qca_power *power = qcadev->bt_power;
> >
> >       if ((qca_is_wcn399x(qcadev->btsoc_type) ||
> > -          qca_is_wcn6750(qcadev->btsoc_type)) &&
> > +          qca_is_wcn6750(qcadev->btsoc_type) ||
> > +          qca_is_wcn6855(qcadev->btsoc_type)) &&
> >            power->vregs_on)
> >               qca_power_shutdown(&qcadev->serdev_hu);
> >       else if (qcadev->susclk)
> > @@ -2335,6 +2360,7 @@ static const struct of_device_id qca_bluetooth_of_match[] = {
> >       { .compatible = "qcom,wcn3991-bt", .data = &qca_soc_data_wcn3991},
> >       { .compatible = "qcom,wcn3998-bt", .data = &qca_soc_data_wcn3998},
> >       { .compatible = "qcom,wcn6750-bt", .data = &qca_soc_data_wcn6750},
> > +     { .compatible = "qcom,wcn6855-bt", .data = &qca_soc_data_wcn6855},
> >       { /* sentinel */ }
> >  };
> >  MODULE_DEVICE_TABLE(of, qca_bluetooth_of_match);
>
> With power-off handling fixed, this seems to work as quite well on my
> X13s with 6.3-rc1. Nice job!
>
> Btw, apart from the frame reassembly error, I'm also seeing:
>
>         Bluetooth: Received HCI_IBS_WAKE_ACK in tx state 0
>
> during probe.
>
I'm still not sure where the frame reassembly error comes from, and I
don't know how to get more info to figure it out either, if anyone
happens to have any guidance for that, I would love some.
Additionally, it doesn't always happen.  It seems to happen on the
first load of the module, however, running modprobe -r && modprobe in
a loop (with the powerdown properly modified so the log isn't full of
splats),  it doesn't seem to occur every time. Likewise for the
WAKE_ACK.

> Johan
Johan Hovold March 10, 2023, 7:27 a.m. UTC | #4
On Thu, Mar 09, 2023 at 02:24:57PM -0600, Steev Klimaszewski wrote:
> On Thu, Mar 9, 2023 at 11:08 AM Johan Hovold <johan@kernel.org> wrote:
> > On Wed, Feb 08, 2023 at 08:09:14PM -0600, Steev Klimaszewski wrote:

> > > @@ -672,6 +678,7 @@ int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
> > >       case QCA_WCN3991:
> > >       case QCA_WCN3998:
> > >       case QCA_WCN6750:
> > > +     case QCA_WCN6855:
> >
> > Did you actually verify the microsoft extensions need this, or you are
> > assuming it works as 6750?
> >
> It was 100% an assumption that since the 6750 does it, the 6855 does
> too.  I should know better than to assume since I used to work at a
> device manufacturer but high hopes things have changed a bit in the
> past 12 years ;)

Heh. Thanks for confirming. :)

> > >               hci_set_msft_opcode(hdev, 0xFD70);
> > >               break;
> > >       default:

> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > > index 3df8c3606e93..efc1c0306b4e 100644
> > > --- a/drivers/bluetooth/hci_qca.c
> > > +++ b/drivers/bluetooth/hci_qca.c
> > > @@ -605,8 +605,7 @@ static int qca_open(struct hci_uart *hu)
> > >       if (hu->serdev) {
> > >               qcadev = serdev_device_get_drvdata(hu->serdev);
> > >
> > > -             if (qca_is_wcn399x(qcadev->btsoc_type) ||
> > > -                 qca_is_wcn6750(qcadev->btsoc_type))
> > > +             if (!(qcadev->init_speed))
> > >                       hu->init_speed = qcadev->init_speed;
> >
> > This change makes no sense.
> >
> > In fact, it seems the driver never sets init_speed anywhere.
> >
> > Either way, it should not be needed for wcn6855.
> 
> So, that was a request from an earlier review, but if it's not needed
> for 6855, I'll just drop it, and then I don't need to do any of those
> changes :D

Note that with the above, init_speed is only is set if qcadev->init_speed
is *not* set, so if this was needed the test would need to be negated.

But as I mentioned above, this also looks broken as qcadev->init_speed
is never set anywhere.

You could investigate and clean up this code before or after adding
support for wcn6855, but the above really doesn't look right and would
at least need to go in a separate patch with a proper explanation.

> > >
> > >               if (qcadev->oper_speed)

> > > @@ -1723,7 +1725,8 @@ static int qca_setup(struct hci_uart *hu)
> > >
> > >       bt_dev_info(hdev, "setting up %s",
> > >               qca_is_wcn399x(soc_type) ? "wcn399x" :
> > > -             (soc_type == QCA_WCN6750) ? "wcn6750" : "ROME/QCA6390");
> > > +             (soc_type == QCA_WCN6750) ? "wcn6750" :
> > > +             (soc_type == QCA_WCN6855) ? "wcn6855" : "ROME/QCA6390");
> >
> > This is hideous, but not your fault...
> >
> It is, and, I'm not entirely sure we need it? I mean, it's nice to
> show that it's now starting to set up, but it isn't particularly
> helpful for end users or making sure things are working?

This is driver is already spamming the logs, while generally we should
only be logging when things go wrong. Those info messages can probably
be changed to dev_dbg(), but that's a separate issue and not something
that's needed to add support for wcn6855.

And same here, you can keep the above as is, but it seems like someone
will soon need to clean up the type handling as all these conditionals
are getting a bit unwieldy.

> > > @@ -1883,6 +1888,20 @@ static const struct qca_device_data qca_soc_data_wcn6750 = {
> > >       .capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
> > >  };
> > >
> > > +static const struct qca_device_data qca_soc_data_wcn6855 = {
> > > +     .soc_type = QCA_WCN6855,
> > > +     .vregs = (struct qca_vreg []) {
> > > +             { "vddio", 5000 },
> > > +             { "vddbtcxmx", 126000 },
> > > +             { "vddrfacmn", 12500 },
> > > +             { "vddrfa0p8", 102000 },
> > > +             { "vddrfa1p7", 302000 },
> > > +             { "vddrfa1p2", 257000 },
> >
> > Hmm. More random regulator load values. I really think we should get rid
> > of this but that's a separate discussion.
> >
> Bjorn specifically requested that he wanted me to leave them in.  I'm
> not married to them, and don't care one way or the other, I just
> wanted working bluetooth since audio wasn't quite ready yet :)

You can them in for now, but we had a discussion last fall about whether
these made up numbers really belong in the kernel.

> > > +     },
> > > +     .num_vregs = 6,
> > > +     .capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
> > > +};
> > > +
> > >  static void qca_power_shutdown(struct hci_uart *hu)
> > >  {
> > >       struct qca_serdev *qcadev;
> >
> > As I mentioned elsewhere, you need to update also this function so that
> > wcn6855 can be powered down.
> 
> Sorry, I do have that locally, I just haven't pushed a v6 as I was
> looking at Tim's v2 of the qca2066 and was wondering if I should or
> shouldn't continue working on my version of the driver?

I only skimmed that patch a while ago, but that ones not strictly needed
for wcn6855, right? Things seems to work well here with just this series
applied.

> > With power-off handling fixed, this seems to work as quite well on my
> > X13s with 6.3-rc1. Nice job!
> >
> > Btw, apart from the frame reassembly error, I'm also seeing:
> >
> >         Bluetooth: Received HCI_IBS_WAKE_ACK in tx state 0
> >
> > during probe.
> >
> I'm still not sure where the frame reassembly error comes from, and I
> don't know how to get more info to figure it out either, if anyone
> happens to have any guidance for that, I would love some.
> Additionally, it doesn't always happen.  It seems to happen on the
> first load of the module, however, running modprobe -r && modprobe in
> a loop (with the powerdown properly modified so the log isn't full of
> splats),  it doesn't seem to occur every time. Likewise for the
> WAKE_ACK.

Ok. Looks like the Chromium team tried to suppress these errors when
switching line speed by toggling rts, but the frame-assembly error I get
appears to happen before that.

Johan
diff mbox series

Patch

diff --git a/drivers/bluetooth/btqca.c b/drivers/bluetooth/btqca.c
index c9064d34d830..2f9d8bd27c38 100644
--- a/drivers/bluetooth/btqca.c
+++ b/drivers/bluetooth/btqca.c
@@ -614,6 +614,9 @@  int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 		config.type = ELF_TYPE_PATCH;
 		snprintf(config.fwname, sizeof(config.fwname),
 			 "qca/msbtfw%02x.mbn", rom_ver);
+	} else if (soc_type == QCA_WCN6855) {
+		snprintf(config.fwname, sizeof(config.fwname),
+			 "qca/hpbtfw%02x.tlv", rom_ver);
 	} else {
 		snprintf(config.fwname, sizeof(config.fwname),
 			 "qca/rampatch_%08x.bin", soc_ver);
@@ -648,6 +651,9 @@  int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 	else if (soc_type == QCA_WCN6750)
 		snprintf(config.fwname, sizeof(config.fwname),
 			 "qca/msnv%02x.bin", rom_ver);
+	else if (soc_type == QCA_WCN6855)
+		snprintf(config.fwname, sizeof(config.fwname),
+			 "qca/hpnv%02x.bin", rom_ver);
 	else
 		snprintf(config.fwname, sizeof(config.fwname),
 			 "qca/nvm_%08x.bin", soc_ver);
@@ -672,6 +678,7 @@  int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 	case QCA_WCN3991:
 	case QCA_WCN3998:
 	case QCA_WCN6750:
+	case QCA_WCN6855:
 		hci_set_msft_opcode(hdev, 0xFD70);
 		break;
 	default:
@@ -685,7 +692,7 @@  int qca_uart_setup(struct hci_dev *hdev, uint8_t baudrate,
 		return err;
 	}
 
-	if (soc_type == QCA_WCN3991 || soc_type == QCA_WCN6750) {
+	if (soc_type == QCA_WCN3991 || soc_type == QCA_WCN6750 || soc_type == QCA_WCN6855) {
 		/* get fw build info */
 		err = qca_read_fw_build_info(hdev);
 		if (err < 0)
diff --git a/drivers/bluetooth/btqca.h b/drivers/bluetooth/btqca.h
index 61e9a50e66ae..b884095bcd9d 100644
--- a/drivers/bluetooth/btqca.h
+++ b/drivers/bluetooth/btqca.h
@@ -147,6 +147,7 @@  enum qca_btsoc_type {
 	QCA_WCN3991,
 	QCA_QCA6390,
 	QCA_WCN6750,
+	QCA_WCN6855,
 };
 
 #if IS_ENABLED(CONFIG_BT_QCA)
@@ -168,6 +169,10 @@  static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type)
 {
 	return soc_type == QCA_WCN6750;
 }
+static inline bool qca_is_wcn6855(enum qca_btsoc_type soc_type)
+{
+	return soc_type == QCA_WCN6855;
+}
 
 #else
 
@@ -206,6 +211,11 @@  static inline bool qca_is_wcn6750(enum qca_btsoc_type soc_type)
 	return false;
 }
 
+static inline bool qca_is_wcn6855(enum qca_btsoc_type soc_type)
+{
+	return false;
+}
+
 static inline int qca_send_pre_shutdown_cmd(struct hci_dev *hdev)
 {
 	return -EOPNOTSUPP;
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 3df8c3606e93..efc1c0306b4e 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -605,8 +605,7 @@  static int qca_open(struct hci_uart *hu)
 	if (hu->serdev) {
 		qcadev = serdev_device_get_drvdata(hu->serdev);
 
-		if (qca_is_wcn399x(qcadev->btsoc_type) ||
-		    qca_is_wcn6750(qcadev->btsoc_type))
+		if (!(qcadev->init_speed))
 			hu->init_speed = qcadev->init_speed;
 
 		if (qcadev->oper_speed)
@@ -1317,7 +1316,8 @@  static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate)
 
 	/* Give the controller time to process the request */
 	if (qca_is_wcn399x(qca_soc_type(hu)) ||
-	    qca_is_wcn6750(qca_soc_type(hu)))
+	    qca_is_wcn6750(qca_soc_type(hu)) ||
+	    qca_is_wcn6855(qca_soc_type(hu)))
 		usleep_range(1000, 10000);
 	else
 		msleep(300);
@@ -1394,7 +1394,8 @@  static unsigned int qca_get_speed(struct hci_uart *hu,
 static int qca_check_speeds(struct hci_uart *hu)
 {
 	if (qca_is_wcn399x(qca_soc_type(hu)) ||
-	    qca_is_wcn6750(qca_soc_type(hu))) {
+	    qca_is_wcn6750(qca_soc_type(hu)) ||
+	    qca_is_wcn6855(qca_soc_type(hu))) {
 		if (!qca_get_speed(hu, QCA_INIT_SPEED) &&
 		    !qca_get_speed(hu, QCA_OPER_SPEED))
 			return -EINVAL;
@@ -1682,7 +1683,8 @@  static int qca_power_on(struct hci_dev *hdev)
 		return 0;
 
 	if (qca_is_wcn399x(soc_type) ||
-	    qca_is_wcn6750(soc_type)) {
+	    qca_is_wcn6750(soc_type) ||
+	    qca_is_wcn6855(soc_type)) {
 		ret = qca_regulator_init(hu);
 	} else {
 		qcadev = serdev_device_get_drvdata(hu->serdev);
@@ -1723,7 +1725,8 @@  static int qca_setup(struct hci_uart *hu)
 
 	bt_dev_info(hdev, "setting up %s",
 		qca_is_wcn399x(soc_type) ? "wcn399x" :
-		(soc_type == QCA_WCN6750) ? "wcn6750" : "ROME/QCA6390");
+		(soc_type == QCA_WCN6750) ? "wcn6750" :
+		(soc_type == QCA_WCN6855) ? "wcn6855" : "ROME/QCA6390");
 
 	qca->memdump_state = QCA_MEMDUMP_IDLE;
 
@@ -1735,7 +1738,8 @@  static int qca_setup(struct hci_uart *hu)
 	clear_bit(QCA_SSR_TRIGGERED, &qca->flags);
 
 	if (qca_is_wcn399x(soc_type) ||
-	    qca_is_wcn6750(soc_type)) {
+	    qca_is_wcn6750(soc_type) ||
+	    qca_is_wcn6855(soc_type)) {
 		set_bit(HCI_QUIRK_USE_BDADDR_PROPERTY, &hdev->quirks);
 		hci_set_aosp_capable(hdev);
 
@@ -1757,7 +1761,8 @@  static int qca_setup(struct hci_uart *hu)
 	}
 
 	if (!(qca_is_wcn399x(soc_type) ||
-	     qca_is_wcn6750(soc_type))) {
+	     qca_is_wcn6750(soc_type) ||
+	     qca_is_wcn6855(soc_type))) {
 		/* Get QCA version information */
 		ret = qca_read_soc_version(hdev, &ver, soc_type);
 		if (ret)
@@ -1883,6 +1888,20 @@  static const struct qca_device_data qca_soc_data_wcn6750 = {
 	.capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
 };
 
+static const struct qca_device_data qca_soc_data_wcn6855 = {
+	.soc_type = QCA_WCN6855,
+	.vregs = (struct qca_vreg []) {
+		{ "vddio", 5000 },
+		{ "vddbtcxmx", 126000 },
+		{ "vddrfacmn", 12500 },
+		{ "vddrfa0p8", 102000 },
+		{ "vddrfa1p7", 302000 },
+		{ "vddrfa1p2", 257000 },
+	},
+	.num_vregs = 6,
+	.capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
+};
+
 static void qca_power_shutdown(struct hci_uart *hu)
 {
 	struct qca_serdev *qcadev;
@@ -2047,7 +2066,8 @@  static int qca_serdev_probe(struct serdev_device *serdev)
 
 	if (data &&
 	    (qca_is_wcn399x(data->soc_type) ||
-	    qca_is_wcn6750(data->soc_type))) {
+	    qca_is_wcn6750(data->soc_type) ||
+	    qca_is_wcn6855(data->soc_type))) {
 		qcadev->btsoc_type = data->soc_type;
 		qcadev->bt_power = devm_kzalloc(&serdev->dev,
 						sizeof(struct qca_power),
@@ -2067,14 +2087,18 @@  static int qca_serdev_probe(struct serdev_device *serdev)
 
 		qcadev->bt_en = devm_gpiod_get_optional(&serdev->dev, "enable",
 					       GPIOD_OUT_LOW);
-		if (IS_ERR_OR_NULL(qcadev->bt_en) && data->soc_type == QCA_WCN6750) {
+		if (IS_ERR_OR_NULL(qcadev->bt_en)
+		    && (data->soc_type == QCA_WCN6750 ||
+			data->soc_type == QCA_WCN6855)) {
 			dev_err(&serdev->dev, "failed to acquire BT_EN gpio\n");
 			power_ctrl_enabled = false;
 		}
 
 		qcadev->sw_ctrl = devm_gpiod_get_optional(&serdev->dev, "swctrl",
 					       GPIOD_IN);
-		if (IS_ERR_OR_NULL(qcadev->sw_ctrl) && data->soc_type == QCA_WCN6750)
+		if (IS_ERR_OR_NULL(qcadev->sw_ctrl)
+		    && (data->soc_type == QCA_WCN6750 ||
+			data->soc_type == QCA_WCN6855))
 			dev_warn(&serdev->dev, "failed to acquire SW_CTRL gpio\n");
 
 		qcadev->susclk = devm_clk_get_optional(&serdev->dev, NULL);
@@ -2150,7 +2174,8 @@  static void qca_serdev_remove(struct serdev_device *serdev)
 	struct qca_power *power = qcadev->bt_power;
 
 	if ((qca_is_wcn399x(qcadev->btsoc_type) ||
-	     qca_is_wcn6750(qcadev->btsoc_type)) &&
+	     qca_is_wcn6750(qcadev->btsoc_type) ||
+	     qca_is_wcn6855(qcadev->btsoc_type)) &&
 	     power->vregs_on)
 		qca_power_shutdown(&qcadev->serdev_hu);
 	else if (qcadev->susclk)
@@ -2335,6 +2360,7 @@  static const struct of_device_id qca_bluetooth_of_match[] = {
 	{ .compatible = "qcom,wcn3991-bt", .data = &qca_soc_data_wcn3991},
 	{ .compatible = "qcom,wcn3998-bt", .data = &qca_soc_data_wcn3998},
 	{ .compatible = "qcom,wcn6750-bt", .data = &qca_soc_data_wcn6750},
+	{ .compatible = "qcom,wcn6855-bt", .data = &qca_soc_data_wcn6855},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, qca_bluetooth_of_match);