Re: [PATCH v3] bluetooth: hci_qca: Fix qca6390 enable failure after warm reboot

[Zijun Hu <zijuhu@codeaurora.org>]

On 5/28/2020 11:44 PM, Matthias Kaehlcke wrote:
> On Thu, May 28, 2020 at 01:04:25PM +0800, Zijun Hu wrote:
>>
>>
>> On 5/28/2020 12:48 AM, Matthias Kaehlcke wrote:
>>> Hi Zijun,
>>>
>>> On Wed, May 27, 2020 at 10:32:39AM +0800, Zijun Hu wrote:
>>>> Warm reboot can not restore qca6390 controller baudrate
>>>> to default due to lack of controllable BT_EN pin or power
>>>> supply, so fails to download firmware after warm reboot.
>>>>
>>>> Fixed by sending EDL_SOC_RESET VSC to reset controller
>>>> within added device shutdown implementation.
>>>>
>>>> Signed-off-by: Zijun Hu <zijuhu@codeaurora.org>
>>>> ---
>>>>  drivers/bluetooth/hci_qca.c | 29 +++++++++++++++++++++++++++++
>>>>  1 file changed, 29 insertions(+)
>>>>
>>>> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>>>> index e4a6823..4b6f8b6 100644
>>>> --- a/drivers/bluetooth/hci_qca.c
>>>> +++ b/drivers/bluetooth/hci_qca.c
>>>> @@ -1975,6 +1975,34 @@ static void qca_serdev_remove(struct serdev_device *serdev)
>>>>  	hci_uart_unregister_device(&qcadev->serdev_hu);
>>>>  }
>>>>  
>>>> +static void qca_serdev_shutdown(struct device *dev)
>>>> +{
>>>> +	int ret;
>>>> +	int timeout = msecs_to_jiffies(CMD_TRANS_TIMEOUT_MS);
>>>> +	struct serdev_device *serdev = to_serdev_device(dev);
>>>> +	struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev);
>>>> +	const u8 ibs_wake_cmd[] = { 0xFD };
>>>> +	const u8 edl_reset_soc_cmd[] = { 0x01, 0x00, 0xFC, 0x01, 0x05 };
>>>> +
>>>> +	if (qcadev->btsoc_type == QCA_QCA6390) {
>>>> +		serdev_device_write_flush(serdev);
>>>> +		serdev_device_write_buf(serdev,
>>>> +				ibs_wake_cmd, sizeof(ibs_wake_cmd));
>>>> +		serdev_device_wait_until_sent(serdev, timeout);
>>>
>>> Why no check of the return value of serdev_device_write_buf() here,
>>> does it make sense to continue if sending the wakeup command failed?
>>>
>> i will correct it at v4 patch
>>> Couldn't serdev_device_write() be used instead of the _write_buf() +
>>> _wait_until_sent() combo?
>>>
>> i don't think so, serdev_device_write() is not appropriate at here.
>> serdev_device_write_wakeup() should be used to release completion hold
>> by serdev_device_write(), however @hci_serdev_client_ops doesn't use
>> serdev_device_write_wakeup() to implement its write_wakeup operation.
>> we don't want to touch common hci_serdev.c code.
> 
> Thanks for the clarification!
> 
>>>> +		usleep_range(8000, 10000);
>>>> +
>>>> +		serdev_device_write_flush(serdev);
>>>
>>> I suppose the flush is done because _wait_until_sent() could have timed out.
>>> Another reason to use _device_write() (if suitable), since it returns
>>> -ETIMEDOUT in that case?
>>>
>> flush is prefixed at write operation to speed up
>> shutdown procedure in case of unexpected data injected
>> during waiting for controller wakeup.
> 
> hm, wouldn't it be a bug if unexpected data is injected during shutdown? It
> seems it would be better to detect such a problem and fix the root cause,
> rather than papering over it.
> 
> Also, a flush doesn't really guarantee that there is no unexpected data when
> serdev_device_write_buf() is called, it could be injected just after returning
> from _flush().
> 
actually, we never see these unexpected data injection scenario and it is impossible
to happen theoretically. the main purpose of prefixing flush before the secondary
writing is to make code look more perfect and harmonious visually.

BTW, i have updated this patch to v5 version in order to fix these issue pointed.

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project