Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <ed3d6c68-a575-eb3e-e564-0c70acf29c71@molgen.mpg.de>
Date:   Sun, 11 Jun 2023 20:32:26 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [RESEND v3] Bluetooth: btintel: Add support to reset bluetooth
 via ACPI DSM
From:   Paul Menzel <pmenzel@molgen.mpg.de>
To:     Kiran K <kiran.k@intel.com>
Cc:     linux-bluetooth@vger.kernel.org, ravishankar.srivatsa@intel.com
References: <20230611064342.1587278-1-kiran.k@intel.com>
 <fa2131b8-0186-f9cf-fb79-1975868934ff@molgen.mpg.de>
Content-Language: en-US
In-Reply-To: <fa2131b8-0186-f9cf-fb79-1975868934ff@molgen.mpg.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

[Cc: Remove chethan.tumkur.nayaran@intel.com (550 #5.1.0 Address 
rejected. (in reply to RCPT TO command))]

Am 11.06.23 um 11:42 schrieb Paul Menzel:
> Dear Kiran,
> 
> 
> Thank you for your patch. Some minor nits.
> 
> Am 11.06.23 um 08:43 schrieb Kiran K:
>> New Intel platforms supports reset of Bluetooth device  via ACPI DSM
> 
> 1.  support
> 2.  one space after device
> 
>> methods. The legacy reset mechanism via GPIO will be deprecated in
> 
> Can you please name the new platform it started with.
> 
>> future. This patch checks the platform support for reset methods and if
>> supported uses the same instead of legacy GPIO toggling method.
> 
> Could you please document the datasheet name, version and section this 
> is documented in?
> 
>> ACPI firmware supports two types of reset method based on NIC card.
>> (Discrete or Integrated).
> 
> I’d remove the dot/period before (.
> 
>> 1. VSEC Type - Vendor Specific Extended Capability. Here  BT_EN and
> 
> Only one space after Here.
> 
>>     BT_IF_SELECT lines are driven by a register in PCH cluster. This
>>     interface is supported on discrete BT solution.
>>
>> 2. WDISABLE2 - In this soluton, W_DISABLE2 pin in M.2 is connected to
> 
> solution
> 
>>     physical GPIO from PCH. The DSM interface shall toggle this to recover
>>     from  error.
> 
> Only one space after from.
> 
> How did you test this? (Maybe also paste the new log messages?)
> 
> I’d also appreciated one paragraph about the implentation.
> 
>> Signed-off-by: Kiran K <kiran.k@intel.com>
>> ---
>>   drivers/bluetooth/btintel.c | 121 ++++++++++++++++++++++++++++++++++++
>>   drivers/bluetooth/btintel.h |   2 +
>>   drivers/bluetooth/btusb.c   |  16 +++++
>>   3 files changed, 139 insertions(+)
>>
>> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
>> index d9349ba48281..dd1e48808ee2 100644
>> --- a/drivers/bluetooth/btintel.c
>> +++ b/drivers/bluetooth/btintel.c
>> @@ -10,6 +10,7 @@
>>   #include <linux/firmware.h>
>>   #include <linux/regmap.h>
>>   #include <linux/acpi.h>
>> +#include <acpi/acpi_bus.h>
>>   #include <asm/unaligned.h>
>>   #include <net/bluetooth/bluetooth.h>
>> @@ -27,6 +28,11 @@
>>   #define BTINTEL_PPAG_NAME   "PPAG"
>> +enum {
>> +    DSM_SET_WDISABLE2_DELAY = 1,
>> +    DSM_SET_RESET_METHOD = 3,
>> +};
>> +
>>   /* structure to store the PPAG data read from ACPI table */
>>   struct btintel_ppag {
>>       u32    domain;
>> @@ -49,6 +55,10 @@ static struct {
>>       u32        fw_build_num;
>>   } coredump_info;
>> +static const guid_t btintel_guid_dsm =
>> +    GUID_INIT(0xaa10f4e0, 0x81ac, 0x4233,
>> +          0xab, 0xf6, 0x3b, 0x2a, 0xc5, 0x0e, 0x28, 0xd9);
>> +
>>   int btintel_check_bdaddr(struct hci_dev *hdev)
>>   {
>>       struct hci_rp_read_bd_addr *bda;
>> @@ -2444,6 +2454,116 @@ static void btintel_set_ppag(struct hci_dev 
>> *hdev, struct intel_version_tlv *ver
>>       kfree_skb(skb);
>>   }
>> +static int btintel_acpi_reset_method(struct hci_dev *hdev)
>> +{
>> +    int ret = 0;
>> +    acpi_status status;
>> +    union acpi_object *p, *ref;
>> +    struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
>> +
>> +    status = acpi_evaluate_object(ACPI_HANDLE(GET_HCIDEV_DEV(hdev)), "_PRR", NULL, &buffer);
>> +    if (ACPI_FAILURE(status)) {
>> +        bt_dev_err(hdev, "Failed to run _PRR method");
> 
> Will the failure code(?) be printed?
> 
>> +        ret = -ENODEV;
>> +        return ret;
>> +    }
>> +    p = buffer.pointer;
>> +
>> +    if (p->package.count != 1 || p->type != ACPI_TYPE_PACKAGE) {
>> +        bt_dev_err(hdev, "Invalid arguments");
> 
> Output the values?
> 
>> +        ret = -EINVAL;
>> +        goto exit_on_error;
>> +    }
>> +
>> +    ref = &p->package.elements[0];
>> +    if (ref->type != ACPI_TYPE_LOCAL_REFERENCE) {
>> +        bt_dev_err(hdev, "Invalid object type: 0x%x", ref->type);
> 
> … should be ACPI_TYPE_LOCAL_REFERENCE.
> 
>> +        ret = -EINVAL;
>> +        goto exit_on_error;
>> +    }
>> +
>> +    status = acpi_evaluate_object(ref->reference.handle, "_RST", NULL, NULL);
>> +    if (ACPI_FAILURE(status)) {
>> +        bt_dev_err(hdev, "Failed to run_RST method");
> 
> Will the failure code(?) be printed?
> 
>> +        ret = -ENODEV;
>> +        goto exit_on_error;
>> +    }
>> +
>> +exit_on_error:
>> +    kfree(buffer.pointer);
>> +    return ret;
>> +}
>> +
>> +static void btintel_set_dsm_reset_method(struct hci_dev *hdev,
>> +                     struct intel_version_tlv *ver_tlv)
>> +{
>> +    struct btintel_data *data = hci_get_priv(hdev);
>> +    acpi_handle handle = ACPI_HANDLE(GET_HCIDEV_DEV(hdev));
>> +    u8 reset_payload[4] = {0x01, 0x00, 0x01, 0x00};
> 
> In other parts you add spaces after { and before }.
> 
>> +    union acpi_object *obj, argv4;
>> +    enum {
>> +        RESET_TYPE_WDISABLE2,
>> +        RESET_TYPE_VSEC
>> +    };
>> +
>> +    handle = ACPI_HANDLE(GET_HCIDEV_DEV(hdev));
>> +
>> +    if (!handle) {
>> +        bt_dev_dbg(hdev, "No support for bluetooth device in ACPI firmware");
>> +        return;
>> +    }
>> +
>> +    if (!acpi_has_method(handle, "_PRR")) {
>> +        bt_dev_err(hdev, "No support for _PRR ACPI method");
>> +        return;
>> +    }
>> +
>> +    switch (ver_tlv->cnvi_top & 0xfff) {
>> +    case 0x910: /* GalePeak2 */
>> +        reset_payload[2] = RESET_TYPE_VSEC;
>> +        break;
>> +    default:
>> +        /* WDISABLE2 is the default reset method */
>> +        reset_payload[2] = RESET_TYPE_WDISABLE2;
>> +
>> +        if (!acpi_check_dsm(handle, &btintel_guid_dsm, 0,
>> +                    BIT(DSM_SET_WDISABLE2_DELAY))) {
>> +            bt_dev_err(hdev, "No dsm support to set reset delay");
>> +            return;
>> +        }
>> +        argv4.integer.type = ACPI_TYPE_INTEGER;
>> +        /* delay required to toggle BT power */
>> +        argv4.integer.value = 160;
> 
> Where does that 160 come from?
> 
>> +        obj = acpi_evaluate_dsm(handle, &btintel_guid_dsm, 0,
>> +                    DSM_SET_WDISABLE2_DELAY, &argv4);
>> +        if (!obj) {
>> +            bt_dev_err(hdev, "Failed to call dsm to set reset delay");
>> +            return;
>> +        }
>> +        ACPI_FREE(obj);
>> +    }
>> +
>> +    bt_dev_info(hdev, "DSM reset method type: 0x%02x", reset_payload[2]);
>> +
>> +    if (!acpi_check_dsm(handle, &btintel_guid_dsm, 0,
>> +                DSM_SET_RESET_METHOD)) {
> 
> Does this fit in one line?
> 
>> +        bt_dev_warn(hdev, "No support for dsm to set reset method");
>> +        return;
>> +    }
>> +    argv4.buffer.type = ACPI_TYPE_BUFFER;
>> +    argv4.buffer.length = sizeof(reset_payload);
>> +    argv4.buffer.pointer = reset_payload;
>> +
>> +    obj = acpi_evaluate_dsm(handle, &btintel_guid_dsm, 0,
>> +                DSM_SET_RESET_METHOD, &argv4);
>> +    if (!obj) {
>> +        bt_dev_err(hdev, "Failed to call dsm to set reset method");
>> +        return;
>> +    }
>> +    ACPI_FREE(obj);
>> +    data->acpi_reset_method = btintel_acpi_reset_method;
>> +}
>> +
>>   static int btintel_bootloader_setup_tlv(struct hci_dev *hdev,
>>                       struct intel_version_tlv *ver)
>>   {
>> @@ -2757,6 +2877,7 @@ static int btintel_setup_combined(struct hci_dev 
>> *hdev)
>>           /* Setup MSFT Extension support */
>>           btintel_set_msft_opcode(hdev,
>>                       INTEL_HW_VARIANT(ver_tlv.cnvi_bt));
>> +        btintel_set_dsm_reset_method(hdev, &ver_tlv);
>>           err = btintel_bootloader_setup_tlv(hdev, &ver_tlv);
>>           btintel_register_devcoredump_support(hdev);
>> diff --git a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h
>> index d6a1dc8d8a82..7fd29ef038bd 100644
>> --- a/drivers/bluetooth/btintel.h
>> +++ b/drivers/bluetooth/btintel.h
>> @@ -166,12 +166,14 @@ enum {
>>       INTEL_BROKEN_SHUTDOWN_LED,
>>       INTEL_ROM_LEGACY,
>>       INTEL_ROM_LEGACY_NO_WBS_SUPPORT,
>> +    INTEL_ACPI_RESET_ACTIVE,
>>       __INTEL_NUM_FLAGS,
>>   };
>>   struct btintel_data {
>>       DECLARE_BITMAP(flags, __INTEL_NUM_FLAGS);
>> +    int (*acpi_reset_method)(struct hci_dev *hdev);
>>   };
>>   #define btintel_set_flag(hdev, nr)                    \
>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>> index 8776e0f93c73..c6ac63fdecfa 100644
>> --- a/drivers/bluetooth/btusb.c
>> +++ b/drivers/bluetooth/btusb.c
>> @@ -857,10 +857,26 @@ static void btusb_intel_cmd_timeout(struct 
>> hci_dev *hdev)
>>   {
>>       struct btusb_data *data = hci_get_drvdata(hdev);
>>       struct gpio_desc *reset_gpio = data->reset_gpio;
>> +    struct btintel_data *intel_data = hci_get_priv(hdev);
>>       if (++data->cmd_timeout_cnt < 5)
>>           return;
>> +    if (intel_data->acpi_reset_method) {
>> +        if (test_and_set_bit(INTEL_ACPI_RESET_ACTIVE, intel_data->flags)) {
>> +            bt_dev_err(hdev, "acpi: last reset failed ? Not resetting again");
> 
> Why the question mark? (No space before it.)
> 
>> +            return;
>> +        }
>> +
>> +        bt_dev_err(hdev, "Initiating acpi reset method");
>> +        /* If ACPI reset method fails, lets try with legacy GPIO
>> +         * toggling
>> +         */
>> +        if (!intel_data->acpi_reset_method(hdev)) {
>> +            return;
>> +        }
>> +    }
>> +
>>       if (!reset_gpio) {
>>           btusb_reset(hdev);
>>           return;
> 
> 
> Kind regards,
> 
> Paul