On 1/27/22 20:48, Mark Kettenis wrote:
>> Date: Thu, 27 Jan 2022 08:54:29 +0900
>> From: Jaehoon Chung <jh80.ch...@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 <kette...@openbsd.org>
>>> Reviewed-by: Simon Glass <s...@chromium.org>
>>> Tested on: Macbook Air M1
>>> Tested-by: Simon Glass <s...@chromium.org>
>>
>> Reviewed-by: Jaehoon Chung <jh80.ch...@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.
Thanks for explanation.
Best Regards,
Jaehoon Chung
>
>>> #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;
>>> }
>>>
>>
>>
>
