Re: [U-Boot] [PATCH 070/126] x86: power: Add a PMC uclass

Hi Bin,

On Wed, 9 Oct 2019 at 21:11, Bin Meng bmeng.cn@gmail.com wrote:

Hi Simon,

On Wed, Sep 25, 2019 at 10:58 PM Simon Glass sjg@chromium.org wrote:

...

Intel x86 SoCs have a power manager/controller which handles several power-related aspects of the platform. Add a uclass for this, with a few useful operations.

I don't like to create another x86 specific uclass for this power management controller.

I see two options:

• Create a standard acpi-pmc uclass instead. Almost all registers in

the PMC module follow the ACPI spec, except a few

• Do not abstract this to a uclass level. Use platform codes to do

such. x86 SoC varies from one to another, and it's very hard to do a one-size-fit-all job.

I like the first option, at least for now. I suppose as we see more SoCs added it might get tricky, but quite a few seem to use this common approach.

...

Signed-off-by: Simon Glass sjg@chromium.org

drivers/power/Kconfig | 2 + drivers/power/power_mgr/Kconfig | 25 +++ drivers/power/power_mgr/Makefile | 6 + drivers/power/power_mgr/power-mgr-uclass.c | 191 +++++++++++++++++++++ include/dm/uclass-id.h | 1 + include/power/power_mgr.h | 177 +++++++++++++++++++ 6 files changed, 402 insertions(+) create mode 100644 drivers/power/power_mgr/Kconfig create mode 100644 drivers/power/power_mgr/Makefile create mode 100644 drivers/power/power_mgr/power-mgr-uclass.c create mode 100644 include/power/power_mgr.h

diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig index 9495dca33b9..1cb3f6d5e25 100644 --- a/drivers/power/Kconfig +++ b/drivers/power/Kconfig @@ -4,6 +4,8 @@ source "drivers/power/domain/Kconfig"

source "drivers/power/pmic/Kconfig"

+source "drivers/power/power_mgr/Kconfig"

source "drivers/power/regulator/Kconfig"

choice diff --git a/drivers/power/power_mgr/Kconfig b/drivers/power/power_mgr/Kconfig new file mode 100644 index 00000000000..2731518462f --- /dev/null +++ b/drivers/power/power_mgr/Kconfig @@ -0,0 +1,25 @@ +config POWER_MGR

•   bool "Power Manager (x86 PMC) support"
  

•   help
  

•     Enable support for an x86-style power-management controller which
  

•     provides features including checking whether the system started from
  

•     resume, powering off the system and enabling/disabling the reset
  

•     mechanism.
  

+config SPL_POWER_MGR

•   bool "Power Manager (x86 PMC) support in SPL"
  

•   default y if POWER_MGR
  

•   help
  

•     Enable support for an x86-style power-management controller which
  

•     provides features including checking whether the system started from
  

•     resume, powering off the system and enabling/disabling the reset
  

•     mechanism.
  

+config TPL_POWER_MGR

•   bool "Power Manager (x86 PMC) support in TPL"
  

Why would we need access PMC in both SPL and TPL?

The code used in TPL is in arch_cpu_init_tpm():

- enable the fixed BAR - clear global reset promotion - disable TCO

...

•   default y if POWER_MGR
  

•   help
  

•     Enable support for an x86-style power-management controller which
  

•     provides features including checking whether the system started from
  

•     resume, powering off the system and enabling/disabling the reset
  

•     mechanism.
  

diff --git a/drivers/power/power_mgr/Makefile b/drivers/power/power_mgr/Makefile new file mode 100644 index 00000000000..87542f5248a --- /dev/null +++ b/drivers/power/power_mgr/Makefile @@ -0,0 +1,6 @@ +# SPDX-License-Identifier: GPL-2.0+ +# +# Copyright 2019 Google LLC

+obj-$(CONFIG_$(SPL_TPL_)POWER_MGR) += power-mgr-uclass.o

diff --git a/drivers/power/power_mgr/power-mgr-uclass.c b/drivers/power/power_mgr/power-mgr-uclass.c new file mode 100644 index 00000000000..0d73caf8cf6 --- /dev/null +++ b/drivers/power/power_mgr/power-mgr-uclass.c @@ -0,0 +1,191 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright 2019 Google LLC
• */

+#define LOG_CATEGORY UCLASS_POWER_MGR

+#include <common.h> +#include <acpi_s3.h> +#include <dm.h> +#include <log.h> +#include <asm/io.h> +#ifdef CONFIG_CREATE_ARCH_SYMLINK +#include <asm/arch/gpio.h> +#endif +#include <power/power_mgr.h>

+enum {

•   PM1_STS         = 0x00,
  

•   PM1_EN          = 0x02,
  

•   PM1_CNT         = 0x04,
  

•   GPE0_STS        = 0x20,
  

•   GPE0_EN         = 0x30,
  

+};

+struct tco_regs {

•   u32 tco_rld;
  

•   u32 tco_sts;
  

•   u32 tco1_cnt;
  

•   u32 tco_tmr;
  

+};

+enum {

•   TCO_STS_TIMEOUT                 = 1 << 3,
  

•   TCO_STS_SECOND_TO_STS           = 1 << 17,
  

•   TCO1_CNT_HLT                    = 1 << 11,
  

+};

+static void pmc_fill_pm_reg_info(struct udevice *dev) +{

•   struct power_mgr_upriv *upriv = dev_get_uclass_priv(dev);
  

•   int i;
  

•   upriv->pm1_sts = inw(upriv->acpi_base + PM1_STS);
  

•   upriv->pm1_en = inw(upriv->acpi_base + PM1_EN);
  

•   upriv->pm1_cnt = inw(upriv->acpi_base + PM1_CNT);
  

•   log_debug("pm1_sts: %04x pm1_en: %04x pm1_cnt: %08x\n",
  

•             upriv->pm1_sts, upriv->pm1_en, upriv->pm1_cnt);
  

•   for (i = 0; i < GPE0_REG_MAX; i++) {
  

•           upriv->gpe0_sts[i] = inl(upriv->acpi_base + GPE0_STS + i * 4);
  

•           upriv->gpe0_en[i] = inl(upriv->acpi_base + GPE0_EN + i * 4);
  

•           log_debug("gpe0_sts[%d]: %08x gpe0_en[%d]: %08x\n", i,
  

•                     upriv->gpe0_sts[i], i, upriv->gpe0_en[i]);
  

•   }
  

+}

+int pmc_disable_tco_base(ulong tco_base) +{

TCO is not defined in the ACPI spec. This is APL specific. It looks this has something to do with the hardware watchdog?

Anyway, if we want to put such in an acpi-pmc uclass driver, we should mark such op as optional.

OK.

Regards, SImon