| Message ID | 20220114110438.58452-7-kettenis@openbsd.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | Apple M1 NVMe storage support | expand |
Hi Mark, On Fri, 14 Jan 2022 at 04:05, Mark Kettenis <kettenis@openbsd.org> 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> > --- > arch/arm/Kconfig | 1 + > drivers/power/domain/apple-pmgr.c | 73 ++++++++++++++++++++++++++++++- > 2 files changed, 73 insertions(+), 1 deletion(-) This should use devicetree instead of device_bind() and be a reset driver in drivers/reset Regards, Simon
> From: Simon Glass <sjg@chromium.org> > Date: Fri, 21 Jan 2022 18:40:23 -0700 > > Hi Mark, > > On Fri, 14 Jan 2022 at 04:05, Mark Kettenis <kettenis@openbsd.org> 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> > > --- > > arch/arm/Kconfig | 1 + > > drivers/power/domain/apple-pmgr.c | 73 ++++++++++++++++++++++++++++++- > > 2 files changed, 73 insertions(+), 1 deletion(-) > > This should use devicetree instead of device_bind() and be a reset > driver in drivers/reset Not sure what you mean with "this should use devicetree". The reset and power domain functionality is integrated in the same hardware register and there is a single node that decsribes the device. The Linux driver implements the power domain and reset functionality in a single driver as well. I suppose I could move the reset code into a file of its own in drivers/reset, but I don't think it would make the code easier to understand. And I'd still need to bind the reset driver explicitly in apple_pmgr_probe() as it isn't possible to automatically bind two drivers to a single device tree node as far as I can tell.
Hi Mark, On Sat, 22 Jan 2022 at 07:12, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > From: Simon Glass <sjg@chromium.org> > > Date: Fri, 21 Jan 2022 18:40:23 -0700 > > > > Hi Mark, > > > > On Fri, 14 Jan 2022 at 04:05, Mark Kettenis <kettenis@openbsd.org> 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> > > > --- > > > arch/arm/Kconfig | 1 + > > > drivers/power/domain/apple-pmgr.c | 73 ++++++++++++++++++++++++++++++- > > > 2 files changed, 73 insertions(+), 1 deletion(-) > > > > This should use devicetree instead of device_bind() and be a reset > > driver in drivers/reset > > Not sure what you mean with "this should use devicetree". The reset > and power domain functionality is integrated in the same hardware > register and there is a single node that decsribes the device. > > The Linux driver implements the power domain and reset functionality > in a single driver as well. I suppose I could move the reset code > into a file of its own in drivers/reset, but I don't think it would > make the code easier to understand. And I'd still need to bind the > reset driver explicitly in apple_pmgr_probe() as it isn't possible to > automatically bind two drivers to a single device tree node as far as > I can tell. It seems odd that Linux does this sort of thing. The normal U-Boot approach would be to create a parent MFD driver with children for each uclass. Does linux do that these days? Driver model cannot create two drivers from one device tree node, although you can create one manually later as you have done. It seems OK to follow along with Linux, if it makes it easier to maintain. Otherwise, drivers/reset would be better for the reset driver. Either way: Reviewed-by: Simon Glass <sjg@chromium.org> Tested on: Macbook Air M1 Tested-by: Simon Glass <sjg@chromium.org> Regards, Simon
> From: Simon Glass <sjg@chromium.org> > Date: Sat, 22 Jan 2022 10:17:10 -0700 Hi Simon, > Hi Mark, > > On Sat, 22 Jan 2022 at 07:12, Mark Kettenis <mark.kettenis@xs4all.nl> wrote: > > > > > From: Simon Glass <sjg@chromium.org> > > > Date: Fri, 21 Jan 2022 18:40:23 -0700 > > > > > > Hi Mark, > > > > > > On Fri, 14 Jan 2022 at 04:05, Mark Kettenis <kettenis@openbsd.org> 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> > > > > --- > > > > arch/arm/Kconfig | 1 + > > > > drivers/power/domain/apple-pmgr.c | 73 ++++++++++++++++++++++++++++++- > > > > 2 files changed, 73 insertions(+), 1 deletion(-) > > > > > > This should use devicetree instead of device_bind() and be a reset > > > driver in drivers/reset > > > > Not sure what you mean with "this should use devicetree". The reset > > and power domain functionality is integrated in the same hardware > > register and there is a single node that decsribes the device. > > > > The Linux driver implements the power domain and reset functionality > > in a single driver as well. I suppose I could move the reset code > > into a file of its own in drivers/reset, but I don't think it would > > make the code easier to understand. And I'd still need to bind the > > reset driver explicitly in apple_pmgr_probe() as it isn't possible to > > automatically bind two drivers to a single device tree node as far as > > I can tell. > > It seems odd that Linux does this sort of thing. The normal U-Boot > approach would be to create a parent MFD driver with children for each > uclass. Does linux do that these days? It seems to be common for pinctrl/gpio and clk/reset drivers to be combined in Linux. Not sure if there was a pd/reset combo before the M1 support was accepted, but it is there. MFD is typically used for higher-level functions, i.e. PMIC IC's that include both voltage regulators and an RTC for example. > Driver model cannot create two drivers from one device tree node, > although you can create one manually later as you have done. > > It seems OK to follow along with Linux, if it makes it easier to > maintain. Otherwise, drivers/reset would be better for the reset > driver. > > Either way: > > Reviewed-by: Simon Glass <sjg@chromium.org> > Tested on: Macbook Air M1 > Tested-by: Simon Glass <sjg@chromium.org> I'll keep it as-is then. Thanks, Mark
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 79eec3aa06..04b4a20211 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) #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; }
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> --- arch/arm/Kconfig | 1 + drivers/power/domain/apple-pmgr.c | 73 ++++++++++++++++++++++++++++++- 2 files changed, 73 insertions(+), 1 deletion(-)