From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id CAD94C433EF for ; Thu, 30 Dec 2021 06:03:35 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id 520B581EB2; Thu, 30 Dec 2021 07:03:26 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.b="bTfg8lXG"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 0A76181429; Thu, 30 Dec 2021 07:03:21 +0100 (CET) Received: from mail-ua1-x934.google.com (mail-ua1-x934.google.com [IPv6:2607:f8b0:4864:20::934]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 8F31A816A2 for ; Thu, 30 Dec 2021 07:03:17 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=sjg@google.com Received: by mail-ua1-x934.google.com with SMTP id r15so40959654uao.3 for ; Wed, 29 Dec 2021 22:03:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=mnTEmZXlEF1c14N6RwnLZkyZZ5JI/ZrQggEg8OA3qGk=; b=bTfg8lXGkXcPH+bV+9YEmCYwvGudFQf7cXfuHFZpJLhCLw3BKc6XOcB818X5bmVyL9 hGWaNtDcNipkcrq5W/RABIFOYhDWoJk+7651y/VxGhEoRGKj1M+4+Euci1eHenOInDxw 5PipsgPSqsdKOpgqzXIhPvCI6HihBdOxnzK+8= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=mnTEmZXlEF1c14N6RwnLZkyZZ5JI/ZrQggEg8OA3qGk=; b=PB49dFnGmoH7s0JJCjJszIIw5zVSdY85i9Sk16Gy4gpJ3hEJ+flhte0grJL9SIpPfu DgXJIq45bw6JMHV+E7hQa1HHRlM0r2GEHodlrBxRAo9bNbOb2xj8she+Pc0aFKTokBTl TMQyj6HSbqwWXzpsnOQOiKVDjPGVbEYxF6OpSzJ5gQAttJbdW8g0EWMluMLD+5+8JDRl Ka00Ly86WiOoBeA1zuBnBa3zoERWSlVNd8/5IVG9Ss+5U/fib18KtXMvvKuVoteWhgzi 2IHNNnZh4yed5w/0P+b0bRrjxgkF8g3w7Z1bGGj8SFPtxITkELhwxbNsAXnYPRmc5gKS 0LEw== X-Gm-Message-State: AOAM531sB45MziNr934kKaRVQwAA5EpGUIq9XLShUtHntE1fNRl1ondX pdyQC+QCV53KybA1BkUOtV/BfWMrx431f9I8eJaYfA== X-Google-Smtp-Source: ABdhPJwqQNIPb29vXl1uFoDxUZgsOMvcIB7BkWsrnbiIzNingAYxeVg18lwtafwku/x2KKODcCJmGMd+6O/z0RnvYDc= X-Received: by 2002:ab0:66d9:: with SMTP id d25mr9127636uaq.35.1640844196201; Wed, 29 Dec 2021 22:03:16 -0800 (PST) MIME-Version: 1.0 References: <20211223213418.68994-1-kettenis@openbsd.org> <20211223213418.68994-4-kettenis@openbsd.org> In-Reply-To: From: Simon Glass Date: Wed, 29 Dec 2021 23:03:04 -0700 Message-ID: Subject: Re: [PATCH v2 3/3] power: domain: Add Apple pmgr driver To: Mark Kettenis Cc: Mark Kettenis , U-Boot Mailing List , Jaehoon Chung Content-Type: text/plain; charset="UTF-8" X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.38 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean Hi Mark, On Wed, 29 Dec 2021 at 08:00, Mark Kettenis wrote: > > > From: Simon Glass > > Date: Wed, 29 Dec 2021 06:36:24 -0700 > > > > Hi Mark, > > > > On Tue, 28 Dec 2021 at 07:27, Mark Kettenis wrote: > > > > > > > From: Simon Glass > > > > Date: Tue, 28 Dec 2021 01:34:16 -0700 > > > > > > > > Hi Mark, > > > > > > > > On Thu, 23 Dec 2021 at 14:35, Mark Kettenis wrote: > > > > > > > > > > This driver supports power domains for the power management > > > > > controller found on Apple SoCs. > > > > > > > > > > Signed-off-by: Mark Kettenis > > > > > --- > > > > > 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 > > > > > > > > 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_DOMAIN > > > > > + select REGMAP > > > > > + select SYSCON > > > > > select SYSRESET > > > > > select SYSRESET_WATCHDOG > > > > > select SYSRESET_WATCHDOG_AUTO > > > > > diff --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_DOMAIN > > > > > + default y if ARCH_APPLE > > > > > + help > > > > > + Enable support for manipulating the Apple M1 power domains via > > > > > + MMIO 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 > > > > > + */ > > > > > + > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > + > > > > > +#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. Yes please. Re the other things, just do your best. We often have docs problems, so Apple is not unique here. Regards, Simon