Re: [PATCH v2 7/9] power: domain: apple: Add reset support

[Mark Kettenis <mark.kettenis@xs4all.nl>]

> Date: Thu, 27 Jan 2022 08:54:29 +0900
> From: Jaehoon Chung <jh80.chung@samsung.com>
> 
> Hi,
> 
> On 1/23/22 04:38, Mark Kettenis wrote:
> > The power management controller found on Apple SoCs als provides
> > a way to reset all devices within a power domain. This is needed
> > to cleanly shutdown the NVMe controller before we hand over
> > control to the OS.
> > 
> > Signed-off-by: Mark Kettenis <kettenis@openbsd.org>
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> > Tested on: Macbook Air M1
> > Tested-by: Simon Glass <sjg@chromium.org>
> 
> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
> 
> Add minor comment.

Hi Jaehoon,

> > ---
> >  arch/arm/Kconfig                  |  1 +
> >  drivers/power/domain/apple-pmgr.c | 73 ++++++++++++++++++++++++++++++-
> >  2 files changed, 73 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > index ecacd6860b..14c83ea19e 100644
> > --- a/arch/arm/Kconfig
> > +++ b/arch/arm/Kconfig
> > @@ -935,6 +935,7 @@ config ARCH_APPLE
> >  	select DM_GPIO
> >  	select DM_KEYBOARD
> >  	select DM_MAILBOX
> > +	select DM_RESET
> >  	select DM_SERIAL
> >  	select DM_USB
> >  	select DM_VIDEO
> > diff --git a/drivers/power/domain/apple-pmgr.c b/drivers/power/domain/apple-pmgr.c
> > index d25f136b9d..4d06e76ff5 100644
> > --- a/drivers/power/domain/apple-pmgr.c
> > +++ b/drivers/power/domain/apple-pmgr.c
> > @@ -6,14 +6,22 @@
> >  #include <common.h>
> >  #include <asm/io.h>
> >  #include <dm.h>
> > +#include <dm/device-internal.h>
> >  #include <linux/err.h>
> >  #include <linux/bitfield.h>
> >  #include <power-domain-uclass.h>
> > +#include <reset-uclass.h>
> >  #include <regmap.h>
> >  #include <syscon.h>
> >  
> > -#define APPLE_PMGR_PS_TARGET	GENMASK(3, 0)
> > +#define APPLE_PMGR_RESET	BIT(31)
> > +#define APPLE_PMGR_DEV_DISABLE	BIT(10)
> > +#define APPLE_PMGR_WAS_CLKGATED	BIT(9)
> > +#define APPLE_PMGR_WAS_PWRGATED BIT(8)
> 
> Bit description is specified "WAS_CLKGATED"?
> I think it can be removed "WAS". CLKGATED has already similar meaning.

The names are taken from the Linux driver and I would prefer to keep
them the same to make it easier to compare the two drivers since
nobody outside of Apple has access to documentation for this block.

> >  #define APPLE_PMGR_PS_ACTUAL	GENMASK(7, 4)
> > +#define APPLE_PMGR_PS_TARGET	GENMASK(3, 0)
> > +
> > +#define APPLE_PMGR_FLAGS	(APPLE_PMGR_WAS_CLKGATED | APPLE_PMGR_WAS_PWRGATED)
> >  
> >  #define APPLE_PMGR_PS_ACTIVE	0xf
> >  #define APPLE_PMGR_PS_PWRGATE	0x0
> > @@ -25,6 +33,65 @@ struct apple_pmgr_priv {
> >  	u32 offset;		/* offset within regmap for this domain */
> >  };
> >  
> > +static int apple_reset_of_xlate(struct reset_ctl *reset_ctl,
> > +				struct ofnode_phandle_args *args)
> > +{
> > +	if (args->args_count != 0)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int apple_reset_request(struct reset_ctl *reset_ctl)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int apple_reset_free(struct reset_ctl *reset_ctl)
> > +{
> > +	return 0;
> > +}
> > +
> > +static int apple_reset_assert(struct reset_ctl *reset_ctl)
> > +{
> > +	struct apple_pmgr_priv *priv = dev_get_priv(reset_ctl->dev->parent);
> > +
> > +	regmap_update_bits(priv->regmap, priv->offset,
> > +			   APPLE_PMGR_FLAGS | APPLE_PMGR_DEV_DISABLE,
> > +			   APPLE_PMGR_DEV_DISABLE);
> > +	regmap_update_bits(priv->regmap, priv->offset,
> > +			   APPLE_PMGR_FLAGS | APPLE_PMGR_RESET,
> > +			   APPLE_PMGR_RESET);
> > +
> > +	return 0;
> > +}
> > +
> > +static int apple_reset_deassert(struct reset_ctl *reset_ctl)
> > +{
> > +	struct apple_pmgr_priv *priv = dev_get_priv(reset_ctl->dev->parent);
> > +
> > +	regmap_update_bits(priv->regmap, priv->offset,
> > +			   APPLE_PMGR_FLAGS | APPLE_PMGR_RESET, 0);
> > +	regmap_update_bits(priv->regmap, priv->offset,
> > +			   APPLE_PMGR_FLAGS | APPLE_PMGR_DEV_DISABLE, 0);
> > +
> > +	return 0;
> > +}
> > +
> > +struct reset_ops apple_reset_ops = {
> > +	.of_xlate = apple_reset_of_xlate,
> > +	.request = apple_reset_request,
> > +	.rfree = apple_reset_free,
> > +	.rst_assert = apple_reset_assert,
> > +	.rst_deassert = apple_reset_deassert,
> > +};
> > +
> > +static struct driver apple_reset_driver = {
> > +	.name = "apple_reset",
> > +	.id = UCLASS_RESET,
> > +	.ops = &apple_reset_ops,
> > +};
> > +
> >  static int apple_pmgr_request(struct power_domain *power_domain)
> >  {
> >  	return 0;
> > @@ -78,6 +145,7 @@ static const struct udevice_id apple_pmgr_ids[] = {
> >  static int apple_pmgr_probe(struct udevice *dev)
> >  {
> >  	struct apple_pmgr_priv *priv = dev_get_priv(dev);
> > +	struct udevice *child;
> >  	int ret;
> >  
> >  	ret = dev_power_domain_on(dev);
> > @@ -92,6 +160,9 @@ static int apple_pmgr_probe(struct udevice *dev)
> >  	if (ret < 0)
> >  		return ret;
> >  
> > +	device_bind(dev, &apple_reset_driver, "apple_reset", NULL,
> > +		    dev_ofnode(dev), &child);
> > +
> >  	return 0;
> >  }
> >  
> 
>