| Message ID | 20241004113557.2851060-2-catalin.popescu@leica-geosystems.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] dt-bindings: net: bluetooth: nxp: add support for supply and reset | expand |
| Context | Check | Description |
|---|---|---|
| tedd_an/pre-ci_am | success | Success |
| tedd_an/CheckPatch | success | CheckPatch PASS |
| tedd_an/GitLint | fail | WARNING: I3 - ignore-body-lines: gitlint will be switching from using Python regex 'match' (match beginning) to 'search' (match anywhere) semantics. Please review your ignore-body-lines.regex option accordingly. To remove this warning, set general.regex-style-search=True. More details: https://jorisroovers.github.io/gitlint/configuration/#regex-style-search 1: T3 Title has trailing punctuation (.): "[2/2] Bluetooth: btnxpuart: implement powerup sequence." |
| tedd_an/SubjectPrefix | success | Gitlint PASS |
> -----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. > > 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. 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 >
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 >>
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
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; + } + /* 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); }
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 Signed-off-by: Catalin Popescu <catalin.popescu@leica-geosystems.com> --- drivers/bluetooth/btnxpuart.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)