Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\))
Subject: Re: [PATCH v7 1/4] Bluetooth: btusb: Add *setup* function for new
 generation Intel controllers
From:   Marcel Holtmann <marcel@holtmann.org>
In-Reply-To: <1602678517-9912-1-git-send-email-kiran.k@intel.com>
Date:   Wed, 11 Nov 2020 12:01:23 +0100
Cc:     linux-bluetooth <linux-bluetooth@vger.kernel.org>,
        Sathish Narsimman <sathish.narasimman@intel.com>,
        Chethan T N <chethan.tumkur.narayan@intel.com>,
        "Srivatsa, Ravishankar" <ravishankar.srivatsa@intel.com>,
        Kiran K <kiran.k@intel.com>, Amit K Bag <amit.k.bag@intel.com>,
        Raghuram Hegde <raghuram.hegde@intel.com>
Content-Transfer-Encoding: 8BIT
Message-Id: <84956E68-8DAC-4170-BE38-498D8C92604A@holtmann.org>
References: <1602678517-9912-1-git-send-email-kiran.k@intel.com>
To:     Kiran K <kiraank@gmail.com>
Precedence: bulk

Hi Kiran,

> Signed-off-by: Kiran K <kiran.k@intel.com>
> Signed-off-by: Amit K Bag <amit.k.bag@intel.com>
> Signed-off-by: Raghuram Hegde <raghuram.hegde@intel.com>
> Reviewed-by: Sathish Narasimman <Sathish.Narasimman@intel.com>
> Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com>
> Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com>
> ---
> Changes in v7:
> * split code in to multiple patches
> 
> Changes in v6: 
> * Revert to v4
> * Move TyphoonPeak controller mapping to BTUSB_INTEL_NEWGEN to  
>  a separte patch
> 
> Changes in v5: 
> * Remove BTUSB_INTEL_NEWGEN and use usb vid/pid combination to
>  identify controller type
> 
> Changes in v4: 
> * Rebase patchset
> * Fix indentation issues
> * make btusb_setup_intel_new_get_fw_name to return void as return value is  
>  not getting used
> 
> Changes in v3: 
> * Combine the two patches in v2 series to one to avoid compiler warnings
>   reported by kernel bot (lkp)
> 
> Changed in v2: 
> * Fix typo in commit message
> 
> drivers/bluetooth/btusb.c | 144 +++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 143 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 1005b6e..2e40885 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -60,6 +60,7 @@ static struct usb_driver btusb_driver;
> #define BTUSB_WIDEBAND_SPEECH	0x400000
> #define BTUSB_VALID_LE_STATES   0x800000
> #define BTUSB_QCA_WCN6855	0x1000000
> +#define BTUSB_INTEL_NEWGEN	0x2000000
> 
> static const struct usb_device_id btusb_table[] = {
> 	/* Generic Bluetooth USB device */
> @@ -2693,6 +2694,132 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
> 	return 0;
> }
> 
> +static int btusb_setup_intel_newgen(struct hci_dev *hdev)
> +{
> +	struct btusb_data *data = hci_get_drvdata(hdev);
> +	u32 boot_param;
> +	char ddcname[64];
> +	ktime_t calltime, delta, rettime;
> +	unsigned long long duration;
> +	int err;
> +	struct intel_debug_features features;
> +	struct intel_version_tlv version;
> +
> +	BT_DBG("%s", hdev->name);

lets use bt_dev_dbg here.

> +
> +	/* Set the default boot parameter to 0x0 and it is updated to
> +	 * SKU specific boot parameter after reading Intel_Write_Boot_Params
> +	 * command while downloading the firmware.
> +	 */
> +	boot_param = 0x00000000;
> +
> +	calltime = ktime_get();
> +
> +	/* Read the Intel version information to determine if the device
> +	 * is in bootloader mode or if it already has operational firmware
> +	 * loaded.
> +	 */
> +	err = btintel_read_version_tlv(hdev, &version);
> +	if (err) {
> +		bt_dev_err(hdev, "Intel Read version failed (%d)", err);
> +		btintel_reset_to_bootloader(hdev);
> +		return err;
> +	}
> +
> +	btintel_version_info_tlv(hdev, &version);
> +
> +	/* TODO: Plugin in code here to download firmware */
> +
> +	/* check if controller is already having an operational firmware */
> +	if (version.img_type == 0x03)
> +		goto finish;
> +
> +	rettime = ktime_get();
> +	delta = ktime_sub(rettime, calltime);
> +	duration = (unsigned long long)ktime_to_ns(delta) >> 10;
> +
> +	bt_dev_info(hdev, "Firmware loaded in %llu usecs", duration);
> +
> +	calltime = ktime_get();
> +
> +	set_bit(BTUSB_BOOTING, &data->flags);
> +
> +	err = btintel_send_intel_reset(hdev, boot_param);
> +	if (err) {
> +		bt_dev_err(hdev, "Intel Soft Reset failed (%d)", err);
> +		btintel_reset_to_bootloader(hdev);
> +		return err;
> +	}
> +
> +	/* The bootloader will not indicate when the device is ready. This
> +	 * is done by the operational firmware sending bootup notification.
> +	 *
> +	 * Booting into operational firmware should not take longer than
> +	 * 1 second. However if that happens, then just fail the setup
> +	 * since something went wrong.
> +	 */
> +	bt_dev_info(hdev, "Waiting for device to boot");
> +
> +	err = wait_on_bit_timeout(&data->flags, BTUSB_BOOTING,
> +				  TASK_INTERRUPTIBLE,
> +				  msecs_to_jiffies(1000));
> +
> +	if (err == -EINTR) {
> +		bt_dev_err(hdev, "Device boot interrupted");
> +		return -EINTR;
> +	}
> +
> +	if (err) {
> +		bt_dev_err(hdev, "Device boot timeout");
> +		btintel_reset_to_bootloader(hdev);
> +		return -ETIMEDOUT;
> +	}
> +
> +	rettime = ktime_get();
> +	delta = ktime_sub(rettime, calltime);
> +	duration = (unsigned long long)ktime_to_ns(delta) >> 10;
> +
> +	bt_dev_info(hdev, "Device booted in %llu usecs", duration);
> +
> +	clear_bit(BTUSB_BOOTLOADER, &data->flags);
> +
> +	/* TODO: Insert function call here to get the ddc file name */

Don’t add TODO. Just provide the patches in the right order.

> +
> +	/* Once the device is running in operational mode, it needs to
> +	 * apply the device configuration (DDC) parameters.
> +	 *
> +	 * The device can work without DDC parameters, so even if it
> +	 * fails to load the file, no need to fail the setup.
> +	 */
> +	btintel_load_ddc_config(hdev, ddcname);
> +
> +	/* Read the Intel supported features and if new exception formats
> +	 * supported, need to load the additional DDC config to enable.
> +	 */
> +	btintel_read_debug_features(hdev, &features);
> +
> +	/* Set DDC mask for available debug features */
> +	btintel_set_debug_features(hdev, &features);
> +
> +	/* Read the Intel version information after loading the FW  */
> +	err = btintel_read_version_tlv(hdev, &version);
> +	if (err)
> +		return err;
> +
> +	btintel_version_info_tlv(hdev, &version);
> +
> +finish:
> +	/* Set the event mask for Intel specific vendor events. This enables
> +	 * a few extra events that are useful during general operation. It
> +	 * does not enable any debugging related events.
> +	 *
> +	 * The device will function correctly without these events enabled
> +	 * and thus no need to fail the setup.
> +	 */
> +	btintel_set_event_mask(hdev, false);
> +
> +	return 0;
> +}
> static int btusb_shutdown_intel(struct hci_dev *hdev)
> {
> 	struct sk_buff *skb;
> @@ -3969,7 +4096,8 @@ static int btusb_probe(struct usb_interface *intf,
> 	init_usb_anchor(&data->ctrl_anchor);
> 	spin_lock_init(&data->rxlock);
> 
> -	if (id->driver_info & BTUSB_INTEL_NEW) {
> +	if ((id->driver_info & BTUSB_INTEL_NEW) ||
> +	    (id->driver_info & BTUSB_INTEL_NEWGEN)) {

This seems to be a leftover. Put all in the BTUSB_INTEL_NEWGEN section.

> 		data->recv_event = btusb_recv_event_intel;
> 		data->recv_bulk = btusb_recv_bulk_intel;
> 		set_bit(BTUSB_BOOTLOADER, &data->flags);
> @@ -4078,6 +4206,20 @@ static int btusb_probe(struct usb_interface *intf,
> 		set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
> 	}
> 
> +	if (id->driver_info & BTUSB_INTEL_NEWGEN) {
> +		hdev->manufacturer = 2;
> +		hdev->send = btusb_send_frame_intel;
> +		hdev->setup = btusb_setup_intel_newgen;
> +		hdev->shutdown = btusb_shutdown_intel_new;
> +		hdev->hw_error = btintel_hw_error;
> +		hdev->set_diag = btintel_set_diag;
> +		hdev->set_bdaddr = btintel_set_bdaddr;
> +		hdev->cmd_timeout = btusb_intel_cmd_timeout;
> +		set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
> +		set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
> +		set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
> +	}
> +
> 	if (id->driver_info & BTUSB_MARVELL)
> 		hdev->set_bdaddr = btusb_set_bdaddr_marvell;

Regards

Marcel