Re: [PATCH 6/8] power: domain: apple: Add reset support

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

> 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