Re: [PATCH v3 01/11] PCI/ACPI: Add D3cold Aux Power Limit_DSM method

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

On 29-05-2025 11:48, Nilawar, Badal wrote:
>
> On 24-05-2025 11:30, Sathyanarayanan Kuppuswamy wrote:
>>
>> On 5/23/25 12:01 PM, Badal Nilawar wrote:
>>> From: Anshuman Gupta<anshuman.gupta@intel.com>
>>>
>>> Implement _DSM method 0Ah according to PCI firmware specifications,
>>> section 4.6.10 Rev 3.3., to request auxilary power needed for the
>>> device when in D3Cold.
>>>
>>> Note that this implementation assumes only a single device below the
>>> Downstream Port will request for Aux Power Limit under a given
>>> Root Port because it does not track and aggregate requests
>>> from all child devices below the Downstream Port as required
>>> by Section 4.6.10 Rev 3.3.
>>
>> Add some info about why you are not adding this support?
> Scope is limited to enable the use case of VRAM-SR on BMG GPU. I will 
> mention this in cover letter.
>>
>>> One possible mitigation would be only allowing only first PCIe
>>> Non-Bridge Endpoint Function 0 driver to call_DSM method 0Ah.
>>>
>>> V2(Bjorn/Rafael):
>>>    - Call acpi_dsm_check() to find method 0Ah supported
>>>    - Return retry interval to caller
>>>
>>> Signed-off-by: Varun Gupta<varun.gupta@intel.com>
>>> Signed-off-by: Badal Nilawar<badal.nilawar@intel.com>
>>> Signed-off-by: Anshuman Gupta<anshuman.gupta@intel.com>
>>> ---
>>>   drivers/pci/pci-acpi.c   | 87 
>>> ++++++++++++++++++++++++++++++++++++++++
>>>   include/linux/pci-acpi.h |  8 ++++
>>>   2 files changed, 95 insertions(+)
>>>
>>> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
>>> index af370628e583..76b19525535f 100644
>>> --- a/drivers/pci/pci-acpi.c
>>> +++ b/drivers/pci/pci-acpi.c
>>> @@ -1421,6 +1421,93 @@ static void pci_acpi_optimize_delay(struct 
>>> pci_dev *pdev,
>>>       ACPI_FREE(obj);
>>>   }
>>>   +/**
>>> + * pci_acpi_request_d3cold_aux_power - Request aux power while 
>>> device is in D3Cold
>>> + * @dev: PCI device instance
>>> + * @requested_power: Requested auxiliary power in milliwatts
>>> + * @retry_interval: Retry interval returned by platform to retry 
>>> auxiliary
>>> + *                  power request
>>> + *
>>> + * This function sends a request to the host BIOS via root port 
>>> ACPI _DSM Function 0Ah
>>> + * for the auxiliary power needed by the PCI device when it is in 
>>> D3Cold.
>>> + * It checks and evaluates the _DSM (Device Specific Method) to 
>>> request the auxiliary
>>> + * power and handles the response accordingly.
>>> + *
>>> + * This function shall be only called by 1st non-bridge Endpoint 
>>> driver
>>> + * on Function 0. For a Multi-Function Device, the driver for 
>>> Function 0 is
>>> + * required to report an aggregate power requirement covering all
>>> + * functions contained within the device.
>>> + *
>>> + * Return: Returns 0 on success and errno on failure.
>>> + */
>>> +int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, u32 
>>> requested_power,
>>> +                      u32 *retry_interval)
>>> +{
>>> +    union acpi_object in_obj = {
>>> +        .integer.type = ACPI_TYPE_INTEGER,
>>> +        .integer.value = requested_power,
>>> +    };
>>> +
>>> +    union acpi_object *out_obj;
>>> +    acpi_handle handle;
>>> +    int result, ret = -EINVAL;
>>> +
>>> +    if (!dev)
>>> +        return -EINVAL;
>>
>> Is retry_interval param optional? If not may be a check here for non 
>> NULL condition is needed.
> Yes its optional, its up to caller weather to retry or not.

I will make this param mandatory and add NULL check here. Then its up to 
caller to decide whether to retry.

Thanks,
Badal

>>
>>> +
>>> +    handle = ACPI_HANDLE(&dev->dev);
>>> +    if (!handle)
>>> +        return -EINVAL;
>>> +
>>> +    if (!acpi_check_dsm(handle, &pci_acpi_dsm_guid, 4, 1 << 
>>> DSM_PCI_D3COLD_AUX_POWER_LIMIT)) {
>>> +        pci_dbg(dev, "ACPI _DSM 0%Xh not supported\n", 
>>> DSM_PCI_D3COLD_AUX_POWER_LIMIT);
>>> +        return -ENODEV;
>>> +    }
>>> +
>>> +    out_obj = acpi_evaluate_dsm_typed(handle, &pci_acpi_dsm_guid, 4,
>>> +                      DSM_PCI_D3COLD_AUX_POWER_LIMIT,
>>> +                      &in_obj, ACPI_TYPE_INTEGER);
>>> +    if (!out_obj)
>>> +        return -EINVAL;
>>> +
>>> +    result = out_obj->integer.value;
>>> +    if (retry_interval)
>>> +        *retry_interval = 0;
>>> +
>>> +    switch (result) {
>>> +    case 0x0:
>>> +        pci_dbg(dev, "D3cold Aux Power %u mW request denied\n",
>>> +            requested_power);
>>
>> is this not a error?
>
> No its not error. BIOS Firmware can deny the aux power request.
>
> Regards,
> Badal
>
>>
>>> +        break;
>>> +    case 0x1:
>>> +        pci_info(dev, "D3cold Aux Power request granted: %u mW\n",
>>> +             requested_power);
>>> +        ret = 0;
>>> +        break;
>>> +    case 0x2:
>>> +        pci_info(dev, "D3cold Aux Power: Main power won't be 
>>> removed\n");
>>> +        ret = -EBUSY;
>>> +        break;
>>> +    default:
>>> +        if (result >= 0x11 && result <= 0x1F) {
>>> +            if (retry_interval) {
>>> +                *retry_interval = result & 0xF;
>>> +                pci_warn(dev, "D3cold Aux Power request needs retry 
>>> interval: %u seconds\n",
>>> +                     *retry_interval);
>>> +                ret = -EAGAIN;
>>> +            }
>>> +        } else {
>>> +            pci_err(dev, "D3cold Aux Power: Reserved or unsupported 
>>> response: 0x%x\n",
>>> +                result);
>>> +        }
>>> +        break;
>>> +    }
>>> +
>>> +    ACPI_FREE(out_obj);
>>> +    return ret;
>>> +}
>>> +EXPORT_SYMBOL_GPL(pci_acpi_request_d3cold_aux_power);
>>> +
>>>   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 078225b514d4..1705d03bfe26 100644
>>> --- a/include/linux/pci-acpi.h
>>> +++ b/include/linux/pci-acpi.h
>>> @@ -121,6 +121,7 @@ extern const guid_t pci_acpi_dsm_guid;
>>>   #define DSM_PCI_DEVICE_NAME            0x07
>>>   #define DSM_PCI_POWER_ON_RESET_DELAY        0x08
>>>   #define DSM_PCI_DEVICE_READINESS_DURATIONS    0x09
>>> +#define DSM_PCI_D3COLD_AUX_POWER_LIMIT        0x0A
>>>     #ifdef CONFIG_PCIE_EDR
>>>   void pci_acpi_add_edr_notifier(struct pci_dev *pdev);
>>> @@ -132,10 +133,17 @@ static inline void 
>>> pci_acpi_remove_edr_notifier(struct pci_dev *pdev) { }
>>>     int pci_acpi_set_companion_lookup_hook(struct acpi_device 
>>> *(*func)(struct pci_dev *));
>>>   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);
>>>     #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) { }
>>> +static int pci_acpi_request_d3cold_aux_power(struct pci_dev *dev, 
>>> u32 requested_power,
>>> +                         u32 *retry_interval)
>>> +{
>>> +    return -EOPNOTSUPP;
>>> +}
>>>   #endif    /* CONFIG_ACPI */
>>>     #endif    /* _PCI_ACPI_H_ */
>>