On Fri, Jun 9, 2023 at 1:49 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> [+cc Rafael, linux-pm]
>
> On Fri, Jun 09, 2023 at 10:30:38AM +0800, Zhiren Chen wrote:
> > From: Zhiren Chen <Zhiren.Chen@xxxxxxxxxxxx>
> >
> > The platform_pci_choose_state function and other low-level platform
> > interfaces used by PCI power management processing did not take into
> > account non-ACPI-supported platforms. This shortcoming can result in
> > limitations and issues.
> >
> > For example, in embedded systems like smartphones, a PCI device can be
> > shared by multiple processors for different purposes. The PCI device and
> > some of the processors are controlled by Linux, while the rest of the
> > processors runs its own operating system.
> > When Linux initiates system-level sleep, if it does not consider the
> > working state of the shared PCI device and forcefully sets the PCI device
> > state to D3, it will affect the functionality of other processors that
> > are currently using the PCI device.
> >
> > To address this problem, an interface should be created for PCI devices
> > that don't support ACPI to enable accurate reporting of the power state
> > during the PCI PM handling process.
> >
> > Signed-off-by: Zhiren Chen <Zhiren.Chen@xxxxxxxxxxxx>
Something like the pci_platform_pm_ops introduced here had been there
for several years and the only users of it known to me were ACPI and
Intel MID, which is why it was dropped.
I would like to see the platform code using these new callbacks in the
first place.
> > ---
> > drivers/pci/pci.c | 24 ++++++++++++++++++++++++
> > drivers/pci/pci.h | 40 ++++++++++++++++++++++++++++++++++++++++
> > include/linux/pci.h | 1 +
> > 3 files changed, 65 insertions(+)
> >
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 5ede93222bc1..9f03406f3081 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -1014,6 +1014,9 @@ static void pci_restore_bars(struct pci_dev *dev)
> >
> > static inline bool platform_pci_power_manageable(struct pci_dev *dev)
> > {
> > + if (dev->platform_pm_ops && dev->platform_pm_ops->is_manageable)
> > + return dev->platform_pm_ops->is_manageable(dev);
> > +
> > if (pci_use_mid_pm())
> > return true;
> >
> > @@ -1023,6 +1026,9 @@ static inline bool platform_pci_power_manageable(struct pci_dev *dev)
> > static inline int platform_pci_set_power_state(struct pci_dev *dev,
> > pci_power_t t)
> > {
> > + if (dev->platform_pm_ops && dev->platform_pm_ops->set_state)
> > + return dev->platform_pm_ops->set_state(dev, t);
> > +
> > if (pci_use_mid_pm())
> > return mid_pci_set_power_state(dev, t);
> >
> > @@ -1031,6 +1037,9 @@ static inline int platform_pci_set_power_state(struct pci_dev *dev,
> >
> > static inline pci_power_t platform_pci_get_power_state(struct pci_dev *dev)
> > {
> > + if (dev->platform_pm_ops && dev->platform_pm_ops->get_state)
> > + return dev->platform_pm_ops->get_state(dev);
> > +
> > if (pci_use_mid_pm())
> > return mid_pci_get_power_state(dev);
> >
> > @@ -1039,12 +1048,18 @@ static inline pci_power_t platform_pci_get_power_state(struct pci_dev *dev)
> >
> > static inline void platform_pci_refresh_power_state(struct pci_dev *dev)
> > {
> > + if (dev->platform_pm_ops && dev->platform_pm_ops->refresh_state)
> > + dev->platform_pm_ops->refresh_state(dev);
> > +
> > if (!pci_use_mid_pm())
> > acpi_pci_refresh_power_state(dev);
> > }
> >
> > static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev)
> > {
> > + if (dev->platform_pm_ops && dev->platform_pm_ops->choose_state)
> > + return dev->platform_pm_ops->choose_state(dev);
> > +
> > if (pci_use_mid_pm())
> > return PCI_POWER_ERROR;
> >
> > @@ -1053,6 +1068,9 @@ static inline pci_power_t platform_pci_choose_state(struct pci_dev *dev)
> >
> > static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable)
> > {
> > + if (dev->platform_pm_ops && dev->platform_pm_ops->set_wakeup)
> > + return dev->platform_pm_ops->set_wakeup(dev, enable);
> > +
> > if (pci_use_mid_pm())
> > return PCI_POWER_ERROR;
> >
> > @@ -1061,6 +1079,9 @@ static inline int platform_pci_set_wakeup(struct pci_dev *dev, bool enable)
> >
> > static inline bool platform_pci_need_resume(struct pci_dev *dev)
> > {
> > + if (dev->platform_pm_ops && dev->platform_pm_ops->need_resume)
> > + return dev->platform_pm_ops->need_resume(dev);
> > +
> > if (pci_use_mid_pm())
> > return false;
> >
> > @@ -1069,6 +1090,9 @@ static inline bool platform_pci_need_resume(struct pci_dev *dev)
> >
> > static inline bool platform_pci_bridge_d3(struct pci_dev *dev)
> > {
> > + if (dev->platform_pm_ops && dev->platform_pm_ops->bridge_d3)
> > + return dev->platform_pm_ops->bridge_d3(dev);
> > +
> > if (pci_use_mid_pm())
> > return false;
> >
> > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> > index 2475098f6518..85154470c083 100644
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -71,6 +71,42 @@ struct pci_cap_saved_state *pci_find_saved_ext_cap(struct pci_dev *dev,
> > */
> > #define PCI_RESET_WAIT 1000 /* msec */
> >
> > +/**
> > + * struct pci_platform_pm_ops - Firmware PM callbacks
> > + *
> > + * @is_manageable: returns 'true' if given device is power manageable by the
> > + * platform firmware
> > + *
> > + * @set_state: invokes the platform firmware to set the device's power state
> > + *
> > + * @get_state: queries the platform firmware for a device's current power state
> > + *
> > + * @choose_state: returns PCI power state of given device preferred by the
> > + * platform; to be used during system-wide transitions from a
> > + * sleeping state to the working state and vice versa
> > + *
> > + * @set_wakeup: enables/disables wakeup capability for the device
> > + *
> > + * @need_resume: returns 'true' if the given device (which is currently
> > + * suspended) needs to be resumed to be configured for system
> > + * wakeup.
> > + *
> > + * @bridge_d3: return 'true' if given device supoorts D3 when it is a bridge
> > + *
> > + * @refresh_state: refresh the given device's power state
> > + *
> > + */
> > +struct pci_platform_pm_ops {
> > + bool (*is_manageable)(struct pci_dev *dev);
> > + int (*set_state)(struct pci_dev *dev, pci_power_t state);
> > + pci_power_t (*get_state)(struct pci_dev *dev);
> > + pci_power_t (*choose_state)(struct pci_dev *dev);
> > + int (*set_wakeup)(struct pci_dev *dev, bool enable);
> > + bool (*need_resume)(struct pci_dev *dev);
> > + bool (*bridge_d3)(struct pci_dev *dev);
> > + void (*refresh_state)(struct pci_dev *dev);
> > +};
> > +
> > void pci_update_current_state(struct pci_dev *dev, pci_power_t state);
> > void pci_refresh_power_state(struct pci_dev *dev);
> > int pci_power_up(struct pci_dev *dev);
> > @@ -96,6 +132,10 @@ void pci_bridge_d3_update(struct pci_dev *dev);
> > void pci_bridge_reconfigure_ltr(struct pci_dev *dev);
> > int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type);
> >
> > +static inline void pci_set_platform_pm(struct pci_dev *dev, struct pci_platform_pm_ops *ops)
> > +{
> > + dev->platform_pm_ops = ops;
> > +}
> > static inline void pci_wakeup_event(struct pci_dev *dev)
> > {
> > /* Wait 100 ms before the system can be put into a sleep state. */
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 60b8772b5bd4..a0171f1abf2f 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -327,6 +327,7 @@ struct pci_dev {
> > void *sysdata; /* Hook for sys-specific extension */
> > struct proc_dir_entry *procent; /* Device entry in /proc/bus/pci */
> > struct pci_slot *slot; /* Physical slot this device is in */
> > + struct pci_platform_pm_ops *platform_pm_ops;
> >
> > unsigned int devfn; /* Encoded device & function index */
> > unsigned short vendor;
> > --
> > 2.17.0
> >
