Re: [PATCH v2 2/3] soc: apple: Add driver for Apple PMGR misc controls

[Sven Peter <sven@kernel.org>]

Hi,

Just a few nits in case you need another version, this otherwise looks 
good to me


On 7/3/26 14:44, Sasha Finkelstein wrote:
> From: Hector Martin <marcan@marcan.st>
>
> Apple SoCs have PMGR blocks that control a bunch of power-related
> features. Besides the existing device power state controls (which are
> very uniform and handled by apple-pmgr-pwrstate), we also need to manage
> more random registers such as SoC-wide fabric and memory controller
> power states, which have a different interface.
>
> Add a driver for these kitchen sink controls. Right now it implements
> fabric and memory controller power state switching on system
> standby/s2idle, which saves about 1W of power or so on t60xx platforms.
>
> Signed-off-by: Hector Martin <marcan@marcan.st>
> Co-developed-by: Sasha Finkelstein <k@chaosmail.tech>
> Signed-off-by: Sasha Finkelstein <k@chaosmail.tech>
> ---

[...]

> @@ -0,0 +1,179 @@
> +// SPDX-License-Identifier: GPL-2.0-only OR MIT
> +/*
> + * Apple SoC PMGR device power state driver
> + *
> + * Copyright The Asahi Linux Contributors
> + */
> +
> +#include <linux/bitops.h>
> +#include <linux/bitfield.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
this should be a few lines up to keep the includes in alphabetical order

> +
> +#define APPLE_CLKGEN_PSTATE 0
> +#define APPLE_CLKGEN_PSTATE_DESIRED GENMASK(3, 0)
> +
[...]
> +
> +static void apple_pmgr_sys_dev_set_pstate(struct apple_pmgr_misc *misc,
> +					  enum sys_device dev, bool active)
> +{
> +	u32 pstate;
> +	u32 val;
> +
> +	if (!misc->devices[dev].base)
> +		return;
> +
> +	if (active)
> +		pstate = misc->devices[dev].active_state;
> +	else
> +		pstate = misc->devices[dev].suspend_state;
> +
> +	dev_dbg(misc->dev, "set %d ps to pstate %d\n", dev, pstate);
> +
> +	val = readl_relaxed(misc->devices[dev].base + APPLE_CLKGEN_PSTATE);
> +	val &= ~APPLE_CLKGEN_PSTATE_DESIRED;
> +	val |= FIELD_PREP(APPLE_CLKGEN_PSTATE_DESIRED, pstate);
> +	writel_relaxed(val, misc->devices[dev].base + APPLE_CLKGEN_PSTATE);

There's also FIELD_MODIFY now.

> +}
> +
[...]
> +
> +static const struct apple_pmgr_misc_hw apple_pmgr_misc_hw_t600x = {
> +	.dev_min_ps = {
> +		[DEV_FABRIC] = SYS_DEV_PSTATE_SUSPEND,
> +		[DEV_DCS] = 7,

Do we know what 7 means here? would be nice to also have that #defined

Either way,

Reviewed-by: Sven Peter <sven@kernel.org>


Best,


Sven