Re: [PATCH 3/7] soc: apple: Add driver for Apple PMGR power state controls

[Hector Martin <marcan@marcan.st>]

On 06/10/2021 16.28, Krzysztof Kozlowski wrote:
>> +static int apple_pmgr_ps_set(struct generic_pm_domain *genpd, u32 pstate)
>> +{
>> +	int ret;
>> +	struct apple_pmgr_ps *ps = genpd_to_apple_pmgr_ps(genpd);
>> +	u32 reg;
>> +
>> +	regmap_read(ps->regmap, ps->offset, &reg);
> 
> MMIO accesses should not fail, but regmap API could fail if for example
> clk_enable() fails. In such case you will write below value based on
> random stack init. Please check the return value here.

Ack, will fix for v2 (as well as the related ones below).

>> +static int apple_pmgr_reset_deassert(struct reset_controller_dev *rcdev, unsigned long id)
>> +{
>> +	struct apple_pmgr_ps *ps = rcdev_to_apple_pmgr_ps(rcdev);
>> +
>> +	mutex_lock(&ps->genpd.mlock);
> 
> This looks wrong: it can be a spin-lock, not mutex, so you should use
> genpd_lock.

genpd_lock() is not part of the public API, which is why I did it like 
this. This gets decided by whether the GENPD_FLAG_IRQ_SAFE flag is set, 
so it should be a mutex in this case, as that is not set.

> However now I wonder if there could be a case when a reset-controller
> consumer calls it from it's GENPD_NOTIFY_ON notifier? In such case you
> would have this lock taken.

Hm, yeah, I wonder if we'll hit that use case. Probably not, though. I 
mostly expect our drivers to only reset devices on initial probe or in 
some kind of panic recovery scenario, not while doing PM stuff.

>> +static const struct of_device_id apple_pmgr_ps_of_match[] = {
>> +	{ .compatible = "apple,t8103-pmgr-pwrstate" },
>> +	{ .compatible = "apple,pmgr-pwrstate" },
> 
> You call the device/driver "pwrstate", which it seems is "power state".
> These are not power states. These are power controllers or power
> domains. Power state is rather a state of power domain - e.g. on or
> gated. How about renaming it to power domain or pd?

It's a bit confusing. Apple calls these registers "ps" registers, which 
presumably stands for "power state". They can both clockgate and 
powergate devices (where supported), as well as enable auto-PM and also 
handle reset. So they're a bit more complex and higher level than a 
simple power domain, which is why I called the driver "pwrstate", since 
it controls the power state of a specific SoC domain or block. In fact, 
the device PM is controlled via a 4-bit power state index, though right 
now only 0, 4, 15 are used (power gated, clock gated, active). Many 
devices will not support individual power gating and would just 
clockgate at 0, and right now the driver never uses 4, but might in the 
future. If that needs to be exposed to consumers, then it'd have to be 
via genpd idle states.

-- 
Hector Martin (marcan@marcan.st)
Public Key: https://mrcn.st/pub