Re: [PATCH 2/2] Bluetooth: btnxpuart: implement powerup sequence.

[POPESCU Catalin]

On 06/10/2024 11:04, Sherry Sun wrote:
> [收到此邮件的某些人通常不会收到来自 sherry.sun@xxxxxxx 的电子邮件。请访问 https://aka.ms/LearnAboutSenderIdentification，以了解为什么这一点很重要。;]
>
> This email is not from Hexagon’s Office 365 instance. Please be careful while clicking links, opening attachments, or replying to this email.
>
>
>> -----Original Message-----
>> From: Catalin Popescu <catalin.popescu@xxxxxxxxxxxxxxxxxxxx>
>> Sent: Friday, October 4, 2024 7:36 PM
>> To: Amitkumar Karwar <amitkumar.karwar@xxxxxxx>; Neeraj Sanjay Kale
>> <neeraj.sanjaykale@xxxxxxx>; marcel@xxxxxxxxxxxx;
>> luiz.dentz@xxxxxxxxx; robh@xxxxxxxxxx; krzk+dt@xxxxxxxxxx;
>> conor+dt@xxxxxxxxxx; p.zabel@xxxxxxxxxxxxxx
>> Cc: linux-bluetooth@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
>> kernel@xxxxxxxxxxxxxxx; m.felsch@xxxxxxxxxxxxxx; bsp-
>> development.geo@xxxxxxxxxxxxxxxxxxxx; Catalin Popescu
>> <catalin.popescu@xxxxxxxxxxxxxxxxxxxx>
>> Subject: [PATCH 2/2] Bluetooth: btnxpuart: implement powerup sequence.
>>
>> NXP bluetooth chip shares power supply and reset gpio with a WLAN chip.
>> Add support for power supply and reset and enforce powerup
>> sequence:
>> 1. apply power supply
>> 2. deassert reset/powerdown
> Hi Catalin,
>
> For NXP WIFI/BT chip, WIFI and BT share the one PDn pin, which means that both wifi and BT controller will be powered on and off at the same time. So you control this pin in the BT driver probe and remove function will directly break the wifi function I think.

Hi Sherry,

I know and that's why I'm using reset control and not gpio... Reset 
control are refcounted, gpios are not. So, basically we can use one 
reset control shared by bluetooth and wlan drivers, each driver can 
operate independently from other as long as they implement the powerup 
sequence (shared supply and reset pin) and download their specific 
firmware (combo firmware not used). As long as one driver is functional, 
the chip supply and reset is not asserted. So, you can look at the 
drivers as users of shared resources (supply and reset).

>
>> Signed-off-by: Catalin Popescu <catalin.popescu@xxxxxxxxxxxxxxxxxxxx>
>> ---
>>   drivers/bluetooth/btnxpuart.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
>>
>> diff --git a/drivers/bluetooth/btnxpuart.c b/drivers/bluetooth/btnxpuart.c
>> index 4f493be763b8..e58e7ac7999f 100644
>> --- a/drivers/bluetooth/btnxpuart.c
>> +++ b/drivers/bluetooth/btnxpuart.c
>> @@ -16,6 +16,8 @@
>>   #include <linux/crc8.h>
>>   #include <linux/crc32.h>
>>   #include <linux/string_helpers.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/reset.h>
>>
>>   #include <net/bluetooth/bluetooth.h>
>>   #include <net/bluetooth/hci_core.h>
>> @@ -188,6 +190,7 @@ struct btnxpuart_dev {
>>
>>        struct ps_data psdata;
>>        struct btnxpuart_data *nxp_data;
>> +     struct reset_control *pdn;
>>   };
>>
>>   #define NXP_V1_FW_REQ_PKT    0xa5
>> @@ -1458,6 +1461,7 @@ static int nxp_serdev_probe(struct serdev_device
>> *serdev)  {
>>        struct hci_dev *hdev;
>>        struct btnxpuart_dev *nxpdev;
>> +     int err;
>>
>>        nxpdev = devm_kzalloc(&serdev->dev, sizeof(*nxpdev), GFP_KERNEL);
>>        if (!nxpdev)
>> @@ -1485,6 +1489,16 @@ static int nxp_serdev_probe(struct serdev_device
>> *serdev)
>>
>>        crc8_populate_msb(crc8_table, POLYNOMIAL8);
>>
>> +     nxpdev->pdn = devm_reset_control_get_optional_shared(&serdev-
>>> dev, NULL);
>> +     if (IS_ERR(nxpdev->pdn))
>> +             return PTR_ERR(nxpdev->pdn);
>> +
>> +     err = devm_regulator_get_enable(&serdev->dev, "vcc");
>> +     if (err) {
>> +             dev_err(&serdev->dev, "Failed to enable vcc regulator\n");
>> +             return err;
>> +     }
>> +
> Now in NXP local repo, the PDn pin has been controlled by the corresponding wifi PCIe/SDIO controller, so we won't add the vcc-supply and reset-gpios properties for the BT driver, seems the code here will return err if no vcc-supply and reset-gpios properties provided, which will break the BT driver probe.
If you look closely, the gpio is optional and the supply will return a 
dummy if none is declared in the DT. So, this change won't break anything.
> Best Regards
> Sherry
>
>>        /* Initialize and register HCI device */
>>        hdev = hci_alloc_dev();
>>        if (!hdev) {
>> @@ -1492,6 +1506,8 @@ static int nxp_serdev_probe(struct serdev_device
>> *serdev)
>>                return -ENOMEM;
>>        }
>>
>> +     reset_control_deassert(nxpdev->pdn);
>> +
>>        nxpdev->hdev = hdev;
>>
>>        hdev->bus = HCI_UART;
>> @@ -1509,6 +1525,7 @@ static int nxp_serdev_probe(struct serdev_device
>> *serdev)
>>
>>        if (hci_register_dev(hdev) < 0) {
>>                dev_err(&serdev->dev, "Can't register HCI device\n");
>> +             reset_control_assert(nxpdev->pdn);
>>                hci_free_dev(hdev);
>>                return -ENODEV;
>>        }
>> @@ -1540,6 +1557,7 @@ static void nxp_serdev_remove(struct
>> serdev_device *serdev)
>>        }
>>        ps_cleanup(nxpdev);
>>        hci_unregister_dev(hdev);
>> +     reset_control_assert(nxpdev->pdn);
>>        hci_free_dev(hdev);
>>   }
>>
>> --
>> 2.34.1
>>