[+cc Paul]
On Wed, Oct 23, 2024 at 02:46:47PM +0300, ChandraShekar wrote:
> This patch contains the changes in driver to support the suspend and
> resume i.e move the controller to D3 state when the platform is entering
> into suspend and move the controller to D0 on resume.
>
> Signed-off-by: Kiran K <kiran.k@xxxxxxxxx>
> Signed-off-by: ChandraShekar <chandrashekar.devegowda@xxxxxxxxx>
> ---
> drivers/bluetooth/btintel_pcie.c | 52 ++++++++++++++++++++++++++++++++
> drivers/bluetooth/btintel_pcie.h | 4 +++
> 2 files changed, 56 insertions(+)
>
> diff --git a/drivers/bluetooth/btintel_pcie.c b/drivers/bluetooth/btintel_pcie.c
> index fd4a8bd056fa..f2c44b9d7328 100644
> --- a/drivers/bluetooth/btintel_pcie.c
> +++ b/drivers/bluetooth/btintel_pcie.c
> @@ -273,6 +273,12 @@ static int btintel_pcie_reset_bt(struct btintel_pcie_data *data)
> return reg == 0 ? 0 : -ENODEV;
> }
>
> +static void btintel_pcie_set_persistence_mode(struct btintel_pcie_data *data)
> +{
> + btintel_pcie_set_reg_bits(data, BTINTEL_PCIE_CSR_HW_BOOT_CONFIG,
> + BTINTEL_PCIE_CSR_HW_BOOT_CONFIG_KEEP_ON);
> +}
> +
> /* This function enables BT function by setting BTINTEL_PCIE_CSR_FUNC_CTRL_MAC_INIT bit in
> * BTINTEL_PCIE_CSR_FUNC_CTRL_REG register and wait for MSI-X with
> * BTINTEL_PCIE_MSIX_HW_INT_CAUSES_GP0.
> @@ -297,6 +303,8 @@ static int btintel_pcie_enable_bt(struct btintel_pcie_data *data)
> */
> data->boot_stage_cache = 0x0;
>
> + btintel_pcie_set_persistence_mode(data);
> +
> /* Set MAC_INIT bit to start primary bootloader */
> reg = btintel_pcie_rd_reg32(data, BTINTEL_PCIE_CSR_FUNC_CTRL_REG);
> reg &= ~(BTINTEL_PCIE_CSR_FUNC_CTRL_FUNC_INIT |
> @@ -1653,11 +1661,55 @@ static void btintel_pcie_remove(struct pci_dev *pdev)
> pci_set_drvdata(pdev, NULL);
> }
>
> +static int btintel_pcie_suspend(struct device *dev)
> +{
> + struct btintel_pcie_data *data;
> + int err;
> + struct pci_dev *pdev = to_pci_dev(dev);
> +
> + data = pci_get_drvdata(pdev);
> + btintel_pcie_wr_sleep_cntrl(data, BTINTEL_PCIE_STATE_D3_HOT);
> + data->gp0_received = false;
> + err = wait_event_timeout(data->gp0_wait_q, data->gp0_received,
> + msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
The generic power management code already knows how to put standard
PCI devices in D3 at suspend. So if you have to use device-specific
code like this, I guess the implication is that this device is not
spec-compliant? That would surprise me a little bit since Intel is
pretty good about making their devices work correctly.
Some detail about exactly how this device is non-compliant would be
helpful here and in the commit log.
It looks wrong to me that you call btintel_pcie_wr_sleep_cntrl()
(which I assume starts something that will result in an interrupt that
causes gp0_received to be set to "true") *before* you set gp0_received
to "false".
That looks like a race because the interrupt could happen before
"data->gp0_received = false", and you would return -EBUSY when you
shouldn't. You could test this by inserting a delay before setting
"data->gp0_received = false". Adding a delay should never break this
functionality.
It looks to me like f4c8e876ef6b ("Bluetooth: btintel_pcie: Add
handshake between driver and firmware") (currently in linux-next) has
the same race, where btintel_pcie_send_frame() starts something that
will result in an interrupt, then sets "data->gp0_received = false".
But I don't understand the workings of this hardware or the driver.
> + if (!err) {
> + bt_dev_err(data->hdev, "failed to receive gp0 interrupt for suspend");
> + goto fail;
> + }
> + return 0;
> +fail:
> + return -EBUSY;
Since there's no cleanup, you could just return -EBUSY directly above
and there would be no need for the goto or the "fail" label.
> +}
> +
> +static int btintel_pcie_resume(struct device *dev)
> +{
> + struct btintel_pcie_data *data;
> + struct pci_dev *pdev = to_pci_dev(dev);
> + int err;
> +
> + data = pci_get_drvdata(pdev);
> + btintel_pcie_wr_sleep_cntrl(data, BTINTEL_PCIE_STATE_D0);
> + data->gp0_received = false;
> + err = wait_event_timeout(data->gp0_wait_q, data->gp0_received,
> + msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
Same potential race here. And of course I'm still dubious about the
need for this device-specific code in the first place.
> + if (!err) {
> + bt_dev_err(data->hdev, "failed to receive gp0 interrupt for resume");
> + goto fail;
> + }
> + return 0;
> +fail:
> + return -EBUSY;
> +}
> +
> +static SIMPLE_DEV_PM_OPS(btintel_pcie_pm_ops, btintel_pcie_suspend,
> + btintel_pcie_resume);
> +
> static struct pci_driver btintel_pcie_driver = {
> .name = KBUILD_MODNAME,
> .id_table = btintel_pcie_table,
> .probe = btintel_pcie_probe,
> .remove = btintel_pcie_remove,
> + .driver.pm = &btintel_pcie_pm_ops,
> };
> module_pci_driver(btintel_pcie_driver);
>
> diff --git a/drivers/bluetooth/btintel_pcie.h b/drivers/bluetooth/btintel_pcie.h
> index f9aada0543c4..38d0c8ea2b6f 100644
> --- a/drivers/bluetooth/btintel_pcie.h
> +++ b/drivers/bluetooth/btintel_pcie.h
> @@ -8,6 +8,7 @@
>
> /* Control and Status Register(BTINTEL_PCIE_CSR) */
> #define BTINTEL_PCIE_CSR_BASE (0x000)
> +#define BTINTEL_PCIE_CSR_HW_BOOT_CONFIG (BTINTEL_PCIE_CSR_BASE + 0x000)
> #define BTINTEL_PCIE_CSR_FUNC_CTRL_REG (BTINTEL_PCIE_CSR_BASE + 0x024)
> #define BTINTEL_PCIE_CSR_HW_REV_REG (BTINTEL_PCIE_CSR_BASE + 0x028)
> #define BTINTEL_PCIE_CSR_RF_ID_REG (BTINTEL_PCIE_CSR_BASE + 0x09C)
> @@ -48,6 +49,9 @@
> #define BTINTEL_PCIE_CSR_MSIX_IVAR_BASE (BTINTEL_PCIE_CSR_MSIX_BASE + 0x0880)
> #define BTINTEL_PCIE_CSR_MSIX_IVAR(cause) (BTINTEL_PCIE_CSR_MSIX_IVAR_BASE + (cause))
>
> +/* CSR HW BOOT CONFIG Register */
> +#define BTINTEL_PCIE_CSR_HW_BOOT_CONFIG_KEEP_ON (BIT(31))
> +
> /* Causes for the FH register interrupts */
> enum msix_fh_int_causes {
> BTINTEL_PCIE_MSIX_FH_INT_CAUSES_0 = BIT(0), /* cause 0 */
> --
> 2.34.1
>
