Re: [PATCH v4 03/11] PCI/ACPI: Add PERST# Assertion Delay _DSM method

["Nilawar, Badal" <badal.nilawar@intel.com>]

On 02-07-2025 16:55, Rafael J. Wysocki wrote:
> On Thu, May 29, 2025 at 11:57 PM Sathyanarayanan Kuppuswamy
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>
>> On 5/29/25 4:16 AM, Badal Nilawar wrote:
>>> From: Anshuman Gupta <anshuman.gupta@intel.com>
>>>
>>> Implement _DSM Method 0Bh as per PCI firmware specs
>>> section 4.6.11 Rev 3.3.
>>>
>>> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
>>> Signed-off-by: Badal Nilawar <badal.nilawar@intel.com>
>>> ---
>>>    drivers/pci/pci-acpi.c   | 57 ++++++++++++++++++++++++++++++++++++++++
>>>    include/linux/pci-acpi.h |  8 +++++-
>>>    2 files changed, 64 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>>> index d33efba4ca94..88044491feaa 100644
>>> --- a/drivers/pci/pci-acpi.c
>>> +++ b/drivers/pci/pci-acpi.c
>>> @@ -1531,6 +1531,63 @@ int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>>    }
>>>    EXPORT_SYMBOL_GPL(pci_acpi_request_d3cold_aux_power);
>>>
>>> +/**
>>> + * pci_acpi_add_perst_assertion_delay - Request PERST# delay via ACPI DSM
>>> + * @dev: PCI device instance
>>> + * @delay_us: Requested delay_us
>>> + *
>>> + * This function sends a request to the host BIOS via ACPI _DSM to grant the
>>> + * required PERST# delay for the specified PCI device. It evaluates the _DSM
>>> + * to request the PERST# delay and handles the response accordingly.
>>> + *
>>> + * Return: returns 0 on success and errno on failure.
>>> + */
>>> +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us)
>>> +{
>>> +     union acpi_object in_obj = {
>>> +             .integer.type = ACPI_TYPE_INTEGER,
>>> +             .integer.value = delay_us,
>>> +     };
>>> +
>>> +     union acpi_object *out_obj;
>>> +     acpi_handle handle;
>>> +     int result, ret = -EINVAL;
>>> +
>>> +     if (!dev)
>>> +             return -EINVAL;
>>> +
>>> +     handle = ACPI_HANDLE(&dev->dev);
>>> +     if (!handle)
>>> +             return -EINVAL;
>>> +
>>> +     if (!acpi_check_dsm(handle, &pci_acpi_dsm_guid, 4, 1 << DSM_PCI_PERST_ASSERTION_DELAY)) {
>>> +             pci_dbg(dev, "ACPI _DSM 0%Xh not supported\n", DSM_PCI_PERST_ASSERTION_DELAY);
>>> +             return -ENODEV;
>>> +     }
>>> +
>>> +     out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
>>> +                                       DSM_PCI_PERST_ASSERTION_DELAY,
>>> +                                       &in_obj, ACPI_TYPE_INTEGER);
>>> +     if (!out_obj)
>>> +             return -EINVAL;
>>> +
>>> +     result = out_obj->integer.value;
>>> +
>>> +     if (result == delay_us) {
>>> +             pci_info(dev, "PERST# Assertion Delay set to %u microseconds\n", delay_us);
>>> +             ret = 0;
>> I think above is a debug message. If it is set properly, why would you want to know
>> the details?
>>
>>> +     } else if (result == 0) {
>>> +             pci_warn(dev, "PERST# Assertion Delay request failed, no previous valid request\n");
>>> +     } else {
>>> +             pci_warn(dev, "PERST# Assertion Delay request failed, Previous valid delay: %u microseconds\n",
>>> +                      result);
>>> +     }
>> May be you don't need to elaborate the error details. Will following work?
>>
>> pci_warn(dev, "PERST# Assertion Delay request failed, result:%u micro seconds\n", result);
> Or even
>
> pci_info(dev, "PERST# Assertion Delay request failed, using %u ms
> delay\n", result);
>
> And it doesn't really need to be pci_warn().

Make sense. I will incorporate above suggestion in next rev.

Thanks,
Badal

>
>>> +
>>> +     ACPI_FREE(out_obj);
>>> +     return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(pci_acpi_add_perst_assertion_delay);
>>> +
>>>    static void pci_acpi_set_external_facing(struct pci_dev *dev)
>>>    {
>>>        u8 val;
>>> diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h
>>> index 6079306ad754..e53d4893cf56 100644
>>> --- a/include/linux/pci-acpi.h
>>> +++ b/include/linux/pci-acpi.h
>>> @@ -122,6 +122,7 @@ extern const guid_t pci_acpi_dsm_guid;
>>>    #define DSM_PCI_POWER_ON_RESET_DELAY                0x08
>>>    #define DSM_PCI_DEVICE_READINESS_DURATIONS  0x09
>>>    #define DSM_PCI_D3COLD_AUX_POWER_LIMIT              0x0A
>>> +#define DSM_PCI_PERST_ASSERTION_DELAY                0x0B
>>>
>>>    #ifdef CONFIG_PCIE_EDR
>>>    void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
>>> @@ -135,7 +136,7 @@ int pci_acpi_set_companion_lookup_hook(struct acpi_device *(*func)(struct pci_de
>>>    void pci_acpi_clear_companion_lookup_hook(void);
>>>    int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 requested_power,
>>>                                      u32 *retry_interval);
>>> -
>>> +int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us);
>>>    #else       /* CONFIG_ACPI */
>>>    static inline void acpi_pci_add_bus(struct pci_bus *bus) { }
>>>    static inline void acpi_pci_remove_bus(struct pci_bus *bus) { }
>>> @@ -144,6 +145,11 @@ static inline int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 req
>>>    {
>>>        return -EOPNOTSUPP;
>>>    }
>>> +
>>> +static inline int pci_acpi_add_perst_assertion_delay(struct pci_dev *dev, u32 delay_us)
>>> +{
>>> +     return -EOPNOTSUPP;
>>> +}
>>>    #endif      /* CONFIG_ACPI */
>>>
>>>    #endif      /* _PCI_ACPI_H_ */
>> --
>> Sathyanarayanan Kuppuswamy
>> Linux Kernel Developer
>>