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

[Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>]

Hi Catalin,

Thank you for the patch. It looks good to me.

> On 06/10/2024 11:04, Sherry Sun wrote:
> > [收到此邮件的某些人通常不会收到来自 sherry.sun@nxp.com
> 的电子邮件。请访问
> >
> 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@leica-geosystems.com>
> >> Sent: Friday, October 4, 2024 7:36 PM
> >> To: Amitkumar Karwar <amitkumar.karwar@nxp.com>; Neeraj Sanjay Kale
> >> <neeraj.sanjaykale@nxp.com>; marcel@holtmann.org;
> >> luiz.dentz@gmail.com; robh@kernel.org; krzk+dt@kernel.org;
> >> conor+dt@kernel.org; p.zabel@pengutronix.de
> >> Cc: linux-bluetooth@vger.kernel.org; devicetree@vger.kernel.org;
> >> linux- kernel@vger.kernel.org; m.felsch@pengutronix.de; bsp-
> >> development.geo@leica-geosystems.com; Catalin Popescu
> >> <catalin.popescu@leica-geosystems.com>
> >> 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@leica-geosystems.com>
> >> ---
> >>   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
> >>

Reviewed-by: Neeraj Sanjay Kale <neeraj.sanjaykale@nxp.com>

Thanks,
Neeraj