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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx)
Public Key: https://mrcn.st/pub



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux PPP]     [Linux FS]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Linmodem]     [Device Mapper]     [Linux Kernel for ARM]

  Powered by Linux