2021-09-19 04:10:04

by Rafael J. Wysocki

[permalink] [raw]
Subject: [PATCH v1 1/5] PCI: PM: x86: Drop Intel MID PCI PM support

From: Rafael J. Wysocki <[email protected]>

Support for Intel MID platforms has mostly gone away with the SFI
support removal in commit 4590d98f5a4f ("sfi: Remove framework for
deprecated firmware"), but there are some pieces of it still in the
tree. One of them is the MID PCI PM support code which gets in the
way of subsequent PCI PM simplifications and trying to update it is
rather pointless, so get rid of it completely along with the arch
code pieces that are only used by it.

Signed-off-by: Rafael J. Wysocki <[email protected]>
---

I am going to post patches removing the rest of MID support from arch/x86/
and elsewhere, but that is still quite a bit of stuff and I don't want this
simple PCI PM series to depend on that work.

---
arch/x86/include/asm/intel-mid.h | 3
arch/x86/platform/intel-mid/pwr.c | 150 --------------------------------------
drivers/pci/Makefile | 1
drivers/pci/pci-mid.c | 77 -------------------
4 files changed, 231 deletions(-)

Index: linux-pm/drivers/pci/Makefile
===================================================================
--- linux-pm.orig/drivers/pci/Makefile
+++ linux-pm/drivers/pci/Makefile
@@ -22,7 +22,6 @@ obj-$(CONFIG_PCI_ATS) += ats.o
obj-$(CONFIG_PCI_IOV) += iov.o
obj-$(CONFIG_PCI_BRIDGE_EMUL) += pci-bridge-emul.o
obj-$(CONFIG_PCI_LABEL) += pci-label.o
-obj-$(CONFIG_X86_INTEL_MID) += pci-mid.o
obj-$(CONFIG_PCI_SYSCALL) += syscall.o
obj-$(CONFIG_PCI_STUB) += pci-stub.o
obj-$(CONFIG_PCI_PF_STUB) += pci-pf-stub.o
Index: linux-pm/drivers/pci/pci-mid.c
===================================================================
--- linux-pm.orig/drivers/pci/pci-mid.c
+++ /dev/null
@@ -1,77 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Intel MID platform PM support
- *
- * Copyright (C) 2016, Intel Corporation
- *
- * Author: Andy Shevchenko <[email protected]>
- */
-
-#include <linux/init.h>
-#include <linux/pci.h>
-
-#include <asm/cpu_device_id.h>
-#include <asm/intel-family.h>
-#include <asm/intel-mid.h>
-
-#include "pci.h"
-
-static bool mid_pci_power_manageable(struct pci_dev *dev)
-{
- return true;
-}
-
-static int mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t state)
-{
- return intel_mid_pci_set_power_state(pdev, state);
-}
-
-static pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
-{
- return intel_mid_pci_get_power_state(pdev);
-}
-
-static pci_power_t mid_pci_choose_state(struct pci_dev *pdev)
-{
- return PCI_D3hot;
-}
-
-static int mid_pci_wakeup(struct pci_dev *dev, bool enable)
-{
- return 0;
-}
-
-static bool mid_pci_need_resume(struct pci_dev *dev)
-{
- return false;
-}
-
-static const struct pci_platform_pm_ops mid_pci_platform_pm = {
- .is_manageable = mid_pci_power_manageable,
- .set_state = mid_pci_set_power_state,
- .get_state = mid_pci_get_power_state,
- .choose_state = mid_pci_choose_state,
- .set_wakeup = mid_pci_wakeup,
- .need_resume = mid_pci_need_resume,
-};
-
-/*
- * This table should be in sync with the one in
- * arch/x86/platform/intel-mid/pwr.c.
- */
-static const struct x86_cpu_id lpss_cpu_ids[] = {
- X86_MATCH_INTEL_FAM6_MODEL(ATOM_SALTWELL_MID, NULL),
- X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_MID, NULL),
- {}
-};
-
-static int __init mid_pci_init(void)
-{
- const struct x86_cpu_id *id;
-
- id = x86_match_cpu(lpss_cpu_ids);
- if (id)
- pci_set_platform_pm(&mid_pci_platform_pm);
- return 0;
-}
-arch_initcall(mid_pci_init);
Index: linux-pm/arch/x86/platform/intel-mid/pwr.c
===================================================================
--- linux-pm.orig/arch/x86/platform/intel-mid/pwr.c
+++ linux-pm/arch/x86/platform/intel-mid/pwr.c
@@ -5,13 +5,6 @@
* Copyright (C) 2016, Intel Corporation
*
* Author: Andy Shevchenko <[email protected]>
- *
- * Intel MID Power Management Unit device driver handles the South Complex PCI
- * devices such as GPDMA, SPI, I2C, PWM, and so on. By default PCI core
- * modifies bits in PMCSR register in the PCI configuration space. This is not
- * enough on some SoCs like Intel Tangier. In such case PCI core sets a new
- * power state of the device in question through a PM hook registered in struct
- * pci_platform_pm_ops (see drivers/pci/pci-mid.c).
*/

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -103,11 +96,6 @@ struct mid_pwr {

static struct mid_pwr *midpwr;

-static u32 mid_pwr_get_state(struct mid_pwr *pwr, int reg)
-{
- return readl(pwr->regs + PM_SSS(reg));
-}
-
static void mid_pwr_set_state(struct mid_pwr *pwr, int reg, u32 value)
{
writel(value, pwr->regs + PM_SSC(reg));
@@ -150,143 +138,6 @@ static int mid_pwr_wait_for_cmd(struct m
return mid_pwr_wait(pwr);
}

-static int __update_power_state(struct mid_pwr *pwr, int reg, int bit, int new)
-{
- int curstate;
- u32 power;
- int ret;
-
- /* Check if the device is already in desired state */
- power = mid_pwr_get_state(pwr, reg);
- curstate = (power >> bit) & 3;
- if (curstate == new)
- return 0;
-
- /* Update the power state */
- mid_pwr_set_state(pwr, reg, (power & ~(3 << bit)) | (new << bit));
-
- /* Send command to SCU */
- ret = mid_pwr_wait_for_cmd(pwr, CMD_SET_CFG);
- if (ret)
- return ret;
-
- /* Check if the device is already in desired state */
- power = mid_pwr_get_state(pwr, reg);
- curstate = (power >> bit) & 3;
- if (curstate != new)
- return -EAGAIN;
-
- return 0;
-}
-
-static pci_power_t __find_weakest_power_state(struct mid_pwr_dev *lss,
- struct pci_dev *pdev,
- pci_power_t state)
-{
- pci_power_t weakest = PCI_D3hot;
- unsigned int j;
-
- /* Find device in cache or first free cell */
- for (j = 0; j < LSS_MAX_SHARED_DEVS; j++) {
- if (lss[j].pdev == pdev || !lss[j].pdev)
- break;
- }
-
- /* Store the desired state in cache */
- if (j < LSS_MAX_SHARED_DEVS) {
- lss[j].pdev = pdev;
- lss[j].state = state;
- } else {
- dev_WARN(&pdev->dev, "No room for device in PWRMU LSS cache\n");
- weakest = state;
- }
-
- /* Find the power state we may use */
- for (j = 0; j < LSS_MAX_SHARED_DEVS; j++) {
- if (lss[j].state < weakest)
- weakest = lss[j].state;
- }
-
- return weakest;
-}
-
-static int __set_power_state(struct mid_pwr *pwr, struct pci_dev *pdev,
- pci_power_t state, int id, int reg, int bit)
-{
- const char *name;
- int ret;
-
- state = __find_weakest_power_state(pwr->lss[id], pdev, state);
- name = pci_power_name(state);
-
- ret = __update_power_state(pwr, reg, bit, (__force int)state);
- if (ret) {
- dev_warn(&pdev->dev, "Can't set power state %s: %d\n", name, ret);
- return ret;
- }
-
- dev_vdbg(&pdev->dev, "Set power state %s\n", name);
- return 0;
-}
-
-static int mid_pwr_set_power_state(struct mid_pwr *pwr, struct pci_dev *pdev,
- pci_power_t state)
-{
- int id, reg, bit;
- int ret;
-
- id = intel_mid_pwr_get_lss_id(pdev);
- if (id < 0)
- return id;
-
- reg = (id * LSS_PWS_BITS) / 32;
- bit = (id * LSS_PWS_BITS) % 32;
-
- /* We support states between PCI_D0 and PCI_D3hot */
- if (state < PCI_D0)
- state = PCI_D0;
- if (state > PCI_D3hot)
- state = PCI_D3hot;
-
- mutex_lock(&pwr->lock);
- ret = __set_power_state(pwr, pdev, state, id, reg, bit);
- mutex_unlock(&pwr->lock);
- return ret;
-}
-
-int intel_mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t state)
-{
- struct mid_pwr *pwr = midpwr;
- int ret = 0;
-
- might_sleep();
-
- if (pwr && pwr->available)
- ret = mid_pwr_set_power_state(pwr, pdev, state);
- dev_vdbg(&pdev->dev, "set_power_state() returns %d\n", ret);
-
- return 0;
-}
-
-pci_power_t intel_mid_pci_get_power_state(struct pci_dev *pdev)
-{
- struct mid_pwr *pwr = midpwr;
- int id, reg, bit;
- u32 power;
-
- if (!pwr || !pwr->available)
- return PCI_UNKNOWN;
-
- id = intel_mid_pwr_get_lss_id(pdev);
- if (id < 0)
- return PCI_UNKNOWN;
-
- reg = (id * LSS_PWS_BITS) / 32;
- bit = (id * LSS_PWS_BITS) % 32;
- power = mid_pwr_get_state(pwr, reg);
- return (__force pci_power_t)((power >> bit) & 3);
-}
-
void intel_mid_pwr_power_off(void)
{
struct mid_pwr *pwr = midpwr;
@@ -469,7 +320,6 @@ static const struct mid_pwr_device_info
.set_initial_state = tng_set_initial_state,
};

-/* This table should be in sync with the one in drivers/pci/pci-mid.c */
static const struct pci_device_id mid_pwr_pci_ids[] = {
{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PENWELL), (kernel_ulong_t)&pnw_info },
{ PCI_VDEVICE(INTEL, PCI_DEVICE_ID_TANGIER), (kernel_ulong_t)&tng_info },
Index: linux-pm/arch/x86/include/asm/intel-mid.h
===================================================================
--- linux-pm.orig/arch/x86/include/asm/intel-mid.h
+++ linux-pm/arch/x86/include/asm/intel-mid.h
@@ -10,9 +10,6 @@
#include <linux/pci.h>

extern int intel_mid_pci_init(void);
-extern int intel_mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t state);
-extern pci_power_t intel_mid_pci_get_power_state(struct pci_dev *pdev);
-
extern void intel_mid_pwr_power_off(void);

#define INTEL_MID_PWR_LSS_OFFSET 4




2021-09-20 03:11:24

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v1 1/5] PCI: PM: x86: Drop Intel MID PCI PM support

On Sun, Sep 19, 2021 at 9:01 AM Rafael J. Wysocki <[email protected]> wrote:
>
> From: Rafael J. Wysocki <[email protected]>
>
> Support for Intel MID platforms has mostly gone away with the SFI
> support removal in commit 4590d98f5a4f ("sfi: Remove framework for
> deprecated firmware"), but there are some pieces of it still in the
> tree. One of them is the MID PCI PM support code which gets in the
> way of subsequent PCI PM simplifications and trying to update it is
> rather pointless, so get rid of it completely along with the arch
> code pieces that are only used by it.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
>
> I am going to post patches removing the rest of MID support from arch/x86/
> and elsewhere, but that is still quite a bit of stuff and I don't want this
> simple PCI PM series to depend on that work.

This is still being used by MID with ACPI assisted (*) support.
Hence, not ack.

*) ACPI layer is provided by U-Boot and can't fulfill all possible
features that ACPI may use in the Linux kernel.

--
With Best Regards,
Andy Shevchenko

2021-09-20 05:16:03

by Ferry Toth

[permalink] [raw]
Subject: Re: [PATCH v1 1/5] PCI: PM: x86: Drop Intel MID PCI PM support

Hi,

Op 18-09-2021 om 15:21 schreef Rafael J. Wysocki:
> From: Rafael J. Wysocki <[email protected]>
>
> Support for Intel MID platforms has mostly gone away with the SFI
> support removal in commit 4590d98f5a4f ("sfi: Remove framework for
> deprecated firmware"), but there are some pieces of it still in the
> tree. One of them is the MID PCI PM support code which gets in the
> way of subsequent PCI PM simplifications and trying to update it is
> rather pointless, so get rid of it completely along with the arch
> code pieces that are only used by it.

Removing PM support for MID will break (among others) Intel Edison,
which is currently in use and running up to date vanilla kernel (v5.14)
and user space.

I would happily test updates PM when they appear.

> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
>
> I am going to post patches removing the rest of MID support from arch/x86/
> and elsewhere, but that is still quite a bit of stuff and I don't want this
> simple PCI PM series to depend on that work.
>
> ---
> arch/x86/include/asm/intel-mid.h | 3
> arch/x86/platform/intel-mid/pwr.c | 150 --------------------------------------
> drivers/pci/Makefile | 1
> drivers/pci/pci-mid.c | 77 -------------------
> 4 files changed, 231 deletions(-)
>
> Index: linux-pm/drivers/pci/Makefile
> ===================================================================
> --- linux-pm.orig/drivers/pci/Makefile
> +++ linux-pm/drivers/pci/Makefile
> @@ -22,7 +22,6 @@ obj-$(CONFIG_PCI_ATS) += ats.o
> obj-$(CONFIG_PCI_IOV) += iov.o
> obj-$(CONFIG_PCI_BRIDGE_EMUL) += pci-bridge-emul.o
> obj-$(CONFIG_PCI_LABEL) += pci-label.o
> -obj-$(CONFIG_X86_INTEL_MID) += pci-mid.o
> obj-$(CONFIG_PCI_SYSCALL) += syscall.o
> obj-$(CONFIG_PCI_STUB) += pci-stub.o
> obj-$(CONFIG_PCI_PF_STUB) += pci-pf-stub.o
> Index: linux-pm/drivers/pci/pci-mid.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci-mid.c
> +++ /dev/null
> @@ -1,77 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * Intel MID platform PM support
> - *
> - * Copyright (C) 2016, Intel Corporation
> - *
> - * Author: Andy Shevchenko <[email protected]>
> - */
> -
> -#include <linux/init.h>
> -#include <linux/pci.h>
> -
> -#include <asm/cpu_device_id.h>
> -#include <asm/intel-family.h>
> -#include <asm/intel-mid.h>
> -
> -#include "pci.h"
> -
> -static bool mid_pci_power_manageable(struct pci_dev *dev)
> -{
> - return true;
> -}
> -
> -static int mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t state)
> -{
> - return intel_mid_pci_set_power_state(pdev, state);
> -}
> -
> -static pci_power_t mid_pci_get_power_state(struct pci_dev *pdev)
> -{
> - return intel_mid_pci_get_power_state(pdev);
> -}
> -
> -static pci_power_t mid_pci_choose_state(struct pci_dev *pdev)
> -{
> - return PCI_D3hot;
> -}
> -
> -static int mid_pci_wakeup(struct pci_dev *dev, bool enable)
> -{
> - return 0;
> -}
> -
> -static bool mid_pci_need_resume(struct pci_dev *dev)
> -{
> - return false;
> -}
> -
> -static const struct pci_platform_pm_ops mid_pci_platform_pm = {
> - .is_manageable = mid_pci_power_manageable,
> - .set_state = mid_pci_set_power_state,
> - .get_state = mid_pci_get_power_state,
> - .choose_state = mid_pci_choose_state,
> - .set_wakeup = mid_pci_wakeup,
> - .need_resume = mid_pci_need_resume,
> -};
> -
> -/*
> - * This table should be in sync with the one in
> - * arch/x86/platform/intel-mid/pwr.c.
> - */
> -static const struct x86_cpu_id lpss_cpu_ids[] = {
> - X86_MATCH_INTEL_FAM6_MODEL(ATOM_SALTWELL_MID, NULL),
> - X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT_MID, NULL),
> - {}
> -};
> -
> -static int __init mid_pci_init(void)
> -{
> - const struct x86_cpu_id *id;
> -
> - id = x86_match_cpu(lpss_cpu_ids);
> - if (id)
> - pci_set_platform_pm(&mid_pci_platform_pm);
> - return 0;
> -}
> -arch_initcall(mid_pci_init);
> Index: linux-pm/arch/x86/platform/intel-mid/pwr.c
> ===================================================================
> --- linux-pm.orig/arch/x86/platform/intel-mid/pwr.c
> +++ linux-pm/arch/x86/platform/intel-mid/pwr.c
> @@ -5,13 +5,6 @@
> * Copyright (C) 2016, Intel Corporation
> *
> * Author: Andy Shevchenko <[email protected]>
> - *
> - * Intel MID Power Management Unit device driver handles the South Complex PCI
> - * devices such as GPDMA, SPI, I2C, PWM, and so on. By default PCI core
> - * modifies bits in PMCSR register in the PCI configuration space. This is not
> - * enough on some SoCs like Intel Tangier. In such case PCI core sets a new
> - * power state of the device in question through a PM hook registered in struct
> - * pci_platform_pm_ops (see drivers/pci/pci-mid.c).
> */
>
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> @@ -103,11 +96,6 @@ struct mid_pwr {
>
> static struct mid_pwr *midpwr;
>
> -static u32 mid_pwr_get_state(struct mid_pwr *pwr, int reg)
> -{
> - return readl(pwr->regs + PM_SSS(reg));
> -}
> -
> static void mid_pwr_set_state(struct mid_pwr *pwr, int reg, u32 value)
> {
> writel(value, pwr->regs + PM_SSC(reg));
> @@ -150,143 +138,6 @@ static int mid_pwr_wait_for_cmd(struct m
> return mid_pwr_wait(pwr);
> }
>
> -static int __update_power_state(struct mid_pwr *pwr, int reg, int bit, int new)
> -{
> - int curstate;
> - u32 power;
> - int ret;
> -
> - /* Check if the device is already in desired state */
> - power = mid_pwr_get_state(pwr, reg);
> - curstate = (power >> bit) & 3;
> - if (curstate == new)
> - return 0;
> -
> - /* Update the power state */
> - mid_pwr_set_state(pwr, reg, (power & ~(3 << bit)) | (new << bit));
> -
> - /* Send command to SCU */
> - ret = mid_pwr_wait_for_cmd(pwr, CMD_SET_CFG);
> - if (ret)
> - return ret;
> -
> - /* Check if the device is already in desired state */
> - power = mid_pwr_get_state(pwr, reg);
> - curstate = (power >> bit) & 3;
> - if (curstate != new)
> - return -EAGAIN;
> -
> - return 0;
> -}
> -
> -static pci_power_t __find_weakest_power_state(struct mid_pwr_dev *lss,
> - struct pci_dev *pdev,
> - pci_power_t state)
> -{
> - pci_power_t weakest = PCI_D3hot;
> - unsigned int j;
> -
> - /* Find device in cache or first free cell */
> - for (j = 0; j < LSS_MAX_SHARED_DEVS; j++) {
> - if (lss[j].pdev == pdev || !lss[j].pdev)
> - break;
> - }
> -
> - /* Store the desired state in cache */
> - if (j < LSS_MAX_SHARED_DEVS) {
> - lss[j].pdev = pdev;
> - lss[j].state = state;
> - } else {
> - dev_WARN(&pdev->dev, "No room for device in PWRMU LSS cache\n");
> - weakest = state;
> - }
> -
> - /* Find the power state we may use */
> - for (j = 0; j < LSS_MAX_SHARED_DEVS; j++) {
> - if (lss[j].state < weakest)
> - weakest = lss[j].state;
> - }
> -
> - return weakest;
> -}
> -
> -static int __set_power_state(struct mid_pwr *pwr, struct pci_dev *pdev,
> - pci_power_t state, int id, int reg, int bit)
> -{
> - const char *name;
> - int ret;
> -
> - state = __find_weakest_power_state(pwr->lss[id], pdev, state);
> - name = pci_power_name(state);
> -
> - ret = __update_power_state(pwr, reg, bit, (__force int)state);
> - if (ret) {
> - dev_warn(&pdev->dev, "Can't set power state %s: %d\n", name, ret);
> - return ret;
> - }
> -
> - dev_vdbg(&pdev->dev, "Set power state %s\n", name);
> - return 0;
> -}
> -
> -static int mid_pwr_set_power_state(struct mid_pwr *pwr, struct pci_dev *pdev,
> - pci_power_t state)
> -{
> - int id, reg, bit;
> - int ret;
> -
> - id = intel_mid_pwr_get_lss_id(pdev);
> - if (id < 0)
> - return id;
> -
> - reg = (id * LSS_PWS_BITS) / 32;
> - bit = (id * LSS_PWS_BITS) % 32;
> -
> - /* We support states between PCI_D0 and PCI_D3hot */
> - if (state < PCI_D0)
> - state = PCI_D0;
> - if (state > PCI_D3hot)
> - state = PCI_D3hot;
> -
> - mutex_lock(&pwr->lock);
> - ret = __set_power_state(pwr, pdev, state, id, reg, bit);
> - mutex_unlock(&pwr->lock);
> - return ret;
> -}
> -
> -int intel_mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t state)
> -{
> - struct mid_pwr *pwr = midpwr;
> - int ret = 0;
> -
> - might_sleep();
> -
> - if (pwr && pwr->available)
> - ret = mid_pwr_set_power_state(pwr, pdev, state);
> - dev_vdbg(&pdev->dev, "set_power_state() returns %d\n", ret);
> -
> - return 0;
> -}
> -
> -pci_power_t intel_mid_pci_get_power_state(struct pci_dev *pdev)
> -{
> - struct mid_pwr *pwr = midpwr;
> - int id, reg, bit;
> - u32 power;
> -
> - if (!pwr || !pwr->available)
> - return PCI_UNKNOWN;
> -
> - id = intel_mid_pwr_get_lss_id(pdev);
> - if (id < 0)
> - return PCI_UNKNOWN;
> -
> - reg = (id * LSS_PWS_BITS) / 32;
> - bit = (id * LSS_PWS_BITS) % 32;
> - power = mid_pwr_get_state(pwr, reg);
> - return (__force pci_power_t)((power >> bit) & 3);
> -}
> -
> void intel_mid_pwr_power_off(void)
> {
> struct mid_pwr *pwr = midpwr;
> @@ -469,7 +320,6 @@ static const struct mid_pwr_device_info
> .set_initial_state = tng_set_initial_state,
> };
>
> -/* This table should be in sync with the one in drivers/pci/pci-mid.c */
> static const struct pci_device_id mid_pwr_pci_ids[] = {
> { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_PENWELL), (kernel_ulong_t)&pnw_info },
> { PCI_VDEVICE(INTEL, PCI_DEVICE_ID_TANGIER), (kernel_ulong_t)&tng_info },
> Index: linux-pm/arch/x86/include/asm/intel-mid.h
> ===================================================================
> --- linux-pm.orig/arch/x86/include/asm/intel-mid.h
> +++ linux-pm/arch/x86/include/asm/intel-mid.h
> @@ -10,9 +10,6 @@
> #include <linux/pci.h>
>
> extern int intel_mid_pci_init(void);
> -extern int intel_mid_pci_set_power_state(struct pci_dev *pdev, pci_power_t state);
> -extern pci_power_t intel_mid_pci_get_power_state(struct pci_dev *pdev);
> -
> extern void intel_mid_pwr_power_off(void);
>
> #define INTEL_MID_PWR_LSS_OFFSET 4
>
>
>

2021-09-20 14:20:34

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH v1 1/5] PCI: PM: x86: Drop Intel MID PCI PM support

On Sun, Sep 19, 2021 at 10:32 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Sun, Sep 19, 2021 at 9:01 AM Rafael J. Wysocki <[email protected]> wrote:
> >
> > From: Rafael J. Wysocki <[email protected]>
> >
> > Support for Intel MID platforms has mostly gone away with the SFI
> > support removal in commit 4590d98f5a4f ("sfi: Remove framework for
> > deprecated firmware"), but there are some pieces of it still in the
> > tree. One of them is the MID PCI PM support code which gets in the
> > way of subsequent PCI PM simplifications and trying to update it is
> > rather pointless, so get rid of it completely along with the arch
> > code pieces that are only used by it.
> >
> > Signed-off-by: Rafael J. Wysocki <[email protected]>
> > ---
> >
> > I am going to post patches removing the rest of MID support from arch/x86/
> > and elsewhere, but that is still quite a bit of stuff and I don't want this
> > simple PCI PM series to depend on that work.
>
> This is still being used by MID with ACPI assisted (*) support.
> Hence, not ack.
>
> *) ACPI layer is provided by U-Boot and can't fulfill all possible
> features that ACPI may use in the Linux kernel.

OK, good to know.

I'm not sure how this PCI PM stuff works with ACPI. It looks like
this relies on a specific ordering of arch_initcall() calls for
correctness which is sort of fragile.

2021-09-20 15:21:15

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH v1 1/5] PCI: PM: x86: Drop Intel MID PCI PM support

On Sun, Sep 19, 2021 at 11:11 PM Ferry Toth <[email protected]> wrote:
>
> Hi,
>
> Op 18-09-2021 om 15:21 schreef Rafael J. Wysocki:
> > From: Rafael J. Wysocki <[email protected]>
> >
> > Support for Intel MID platforms has mostly gone away with the SFI
> > support removal in commit 4590d98f5a4f ("sfi: Remove framework for
> > deprecated firmware"), but there are some pieces of it still in the
> > tree. One of them is the MID PCI PM support code which gets in the
> > way of subsequent PCI PM simplifications and trying to update it is
> > rather pointless, so get rid of it completely along with the arch
> > code pieces that are only used by it.
>
> Removing PM support for MID will break (among others) Intel Edison,
> which is currently in use and running up to date vanilla kernel (v5.14)
> and user space.
>
> I would happily test updates PM when they appear.

Thanks for the information, I'll use a different approach then.

2021-09-21 12:18:32

by Andy Shevchenko

[permalink] [raw]
Subject: Re: [PATCH v1 1/5] PCI: PM: x86: Drop Intel MID PCI PM support

On Mon, Sep 20, 2021 at 1:57 PM Rafael J. Wysocki <[email protected]> wrote:
> On Sun, Sep 19, 2021 at 10:32 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Sun, Sep 19, 2021 at 9:01 AM Rafael J. Wysocki <[email protected]> wrote:

...

> > > I am going to post patches removing the rest of MID support from arch/x86/
> > > and elsewhere, but that is still quite a bit of stuff and I don't want this
> > > simple PCI PM series to depend on that work.
> >
> > This is still being used by MID with ACPI assisted (*) support.
> > Hence, not ack.
> >
> > *) ACPI layer is provided by U-Boot and can't fulfill all possible
> > features that ACPI may use in the Linux kernel.
>
> OK, good to know.
>
> I'm not sure how this PCI PM stuff works with ACPI.

It doesn't that is the point. The PCI is very interesting there and
what I meant is that the ACPI implementation I have provided via
U-Boot does not cover these. If you have any hints/ideas how it may be
handled, I am all ears!

> It looks like
> this relies on a specific ordering of arch_initcall() calls for
> correctness which is sort of fragile.

--
With Best Regards,
Andy Shevchenko

2021-09-21 14:27:11

by Rafael J. Wysocki

[permalink] [raw]
Subject: Re: [PATCH v1 1/5] PCI: PM: x86: Drop Intel MID PCI PM support

On Tue, Sep 21, 2021 at 2:17 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Mon, Sep 20, 2021 at 1:57 PM Rafael J. Wysocki <[email protected]> wrote:
> > On Sun, Sep 19, 2021 at 10:32 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Sun, Sep 19, 2021 at 9:01 AM Rafael J. Wysocki <[email protected]> wrote:
>
> ...
>
> > > > I am going to post patches removing the rest of MID support from arch/x86/
> > > > and elsewhere, but that is still quite a bit of stuff and I don't want this
> > > > simple PCI PM series to depend on that work.
> > >
> > > This is still being used by MID with ACPI assisted (*) support.
> > > Hence, not ack.
> > >
> > > *) ACPI layer is provided by U-Boot and can't fulfill all possible
> > > features that ACPI may use in the Linux kernel.
> >
> > OK, good to know.
> >
> > I'm not sure how this PCI PM stuff works with ACPI.
>
> It doesn't that is the point. The PCI is very interesting there and
> what I meant is that the ACPI implementation I have provided via
> U-Boot does not cover these.

That's OK. It just means that these devices are not power-manageable
via ACPI on the platforms in question, but the MID PCI PM code is
present in the kernel, so we don't need analogous code in AML in the
ACPI tables.

My point is that something like the v2 of this patch series
(https://lore.kernel.org/linux-acpi/1800633.tdWV9SEqCh@kreacher/T/#m1ec249724a5ad5ad358b0ed8e149e3926934955d)
is needed to prevent ACPI from overtaking the PM for the PCI devices
on the platform once we've decided to use the MID PM for them.

Now, if it is necessary to use ACPI PM for some devices and the MID PM
for other devices on the same platform, the latter needs to grow a
meaningful "power manageable" function that needs to be used in the
code as appropriate.

> If you have any hints/ideas how it may be handled, I am all ears!