From: Simon Glass sjg@chromium.org Date: Wed, 29 Dec 2021 06:36:24 -0700
Hi Mark,
On Tue, 28 Dec 2021 at 07:27, Mark Kettenis mark.kettenis@xs4all.nl wrote:
From: Simon Glass sjg@chromium.org Date: Tue, 28 Dec 2021 01:34:16 -0700
Hi Mark,
On Thu, 23 Dec 2021 at 14:35, Mark Kettenis kettenis@openbsd.org wrote:
This driver supports power domains for the power management controller found on Apple SoCs.
Signed-off-by: Mark Kettenis kettenis@openbsd.org
arch/arm/Kconfig | 3 + drivers/power/domain/Kconfig | 8 +++ drivers/power/domain/Makefile | 1 + drivers/power/domain/apple-pmgr.c | 113 ++++++++++++++++++++++++++++++ 4 files changed, 125 insertions(+) create mode 100644 drivers/power/domain/apple-pmgr.c
Reviewed-by: Simon Glass sjg@chromium.org
nits below
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 59e031de04..40d3f66acb 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -938,6 +938,9 @@ config ARCH_APPLE select OF_BOARD select PINCTRL select POSITION_INDEPENDENT
select POWER_DOMAINselect REGMAPselect SYSCON select SYSRESET select SYSRESET_WATCHDOG select SYSRESET_WATCHDOG_AUTOdiff --git a/drivers/power/domain/Kconfig b/drivers/power/domain/Kconfig index 99b3f9ae71..6ef7a3b3a7 100644 --- a/drivers/power/domain/Kconfig +++ b/drivers/power/domain/Kconfig @@ -9,6 +9,14 @@ config POWER_DOMAIN domains). This may be used to save power. This API provides the means to control such power management hardware.
+config APPLE_PMGR_POWER_DOMAIN
bool "Enable the Apple PMGR power domain driver"depends on POWER_DOMAINdefault y if ARCH_APPLEhelpEnable support for manipulating the Apple M1 power domains viaMMIO mapped registers.Needs more detail here, perhaps a pointer to docs, or something about what the driver supports.
Struggling to come up with more. The "via MMIO mapped registers" is already a bit silly. There are no docs. And the one line already indicates what the driver supports. Do you want me to add a sentence that says the same thing but in different words?
What is PMGR?
Power ManaGeR? Who knows? It is wat Apple seems to call this thing.
What power domains does the driver support? All of them or just a subset?
Again, we don't know. It supports the power domains we know about.
This is revers-engineered and most of the naming comes from hints dropped by Apple in their version of the device tree and/or the macOS driver names. The more we say here, the more likely it is wrong.
config BCM6328_POWER_DOMAIN bool "Enable the BCM6328 power domain driver" depends on POWER_DOMAIN && ARCH_BMIPS diff --git a/drivers/power/domain/Makefile b/drivers/power/domain/Makefile index 3d1e5f073c..530ae35671 100644 --- a/drivers/power/domain/Makefile +++ b/drivers/power/domain/Makefile @@ -4,6 +4,7 @@ #
obj-$(CONFIG_$(SPL_)POWER_DOMAIN) += power-domain-uclass.o +obj-$(CONFIG_APPLE_PMGR_POWER_DOMAIN) += apple-pmgr.o obj-$(CONFIG_BCM6328_POWER_DOMAIN) += bcm6328-power-domain.o obj-$(CONFIG_IMX8_POWER_DOMAIN) += imx8-power-domain-legacy.o imx8-power-domain.o obj-$(CONFIG_IMX8M_POWER_DOMAIN) += imx8m-power-domain.o diff --git a/drivers/power/domain/apple-pmgr.c b/drivers/power/domain/apple-pmgr.c new file mode 100644 index 0000000000..08a30c8ebf --- /dev/null +++ b/drivers/power/domain/apple-pmgr.c @@ -0,0 +1,113 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*
- Copyright (C) 2021 Mark Kettenis kettenis@openbsd.org
- */
+#include <common.h> +#include <asm/io.h> +#include <dm.h> +#include <linux/err.h> +#include <linux/bitfield.h> +#include <power-domain-uclass.h> +#include <regmap.h> +#include <syscon.h>
+#define APPLE_PMGR_PS_TARGET GENMASK(3, 0) +#define APPLE_PMGR_PS_ACTUAL GENMASK(7, 4)
+#define APPLE_PMGR_PS_ACTIVE 0xf +#define APPLE_PMGR_PS_PWRGATE 0x0
+#define APPLE_PMGR_PS_SET_TIMEOUT 100
TIMEOUT_MS ? _US ?
_US
+struct apple_pmgr_priv {
struct regmap *regmap;u32 offset;Needs comment for struct
Really? I mean, I can add a "Device private data" comment, but that's pretty much implied by the _priv and the vast majority of the drivers don't do such a thing.
What is offset?
The offset into the register map for this particular power domain. Adding a comment feels like stating the obvious to me. But if you feel it needs one, I can add it.
+};
+static int apple_pmgr_request(struct power_domain *power_domain) +{
return 0;+}
+static int apple_pmgr_rfree(struct power_domain *power_domain) +{
return 0;+}
+static int apple_pmgr_ps_set(struct power_domain *power_domain, u32 pstate) +{
struct apple_pmgr_priv *priv = dev_get_priv(power_domain->dev);uint reg;regmap_update_bits(priv->regmap, priv->offset, APPLE_PMGR_PS_TARGET,FIELD_PREP(APPLE_PMGR_PS_TARGET, pstate));return regmap_read_poll_timeout(priv->regmap, priv->offset, reg,(FIELD_GET(APPLE_PMGR_PS_ACTUAL, reg) == pstate), 1,APPLE_PMGR_PS_SET_TIMEOUT);+}
+static int apple_pmgr_on(struct power_domain *power_domain) +{
return apple_pmgr_ps_set(power_domain, APPLE_PMGR_PS_ACTIVE);+}
+static int apple_pmgr_off(struct power_domain *power_domain) +{
return 0;+}
+static int apple_pmgr_of_xlate(struct power_domain *power_domain,
struct ofnode_phandle_args *args)+{
if (args->args_count != 0) {s/ != 0//
!= 0 seems better here since that is the value we're checking for.
It can't be negative though.
[..]
Regards, Simon