svn commit: r250956 - projects/pmac_pmu/sys/kern

[Justin Hibbits]

Hi John,

I know it's an ancient thread I'm replying to, but it's the same thing.


On Fri, May 24, 2013 at 5:49 AM, John Baldwin <jhb at freebsd.org> wrote:

> On Thursday, May 23, 2013 11:58:31 pm Justin Hibbits wrote:
> > Author: jhibbits
> > Date: Fri May 24 03:58:31 2013
> > New Revision: 250956
> > URL: http://svnweb.freebsd.org/changeset/base/250956
> >
> > Log:
> >   Add multipass for suspend/resume.  This allows some devices to be
> resumed before
> >   others, even their peers, as required by PowerPC Mac hardware.
>
> I think this is a good start.  One question I have is why not reuse the
> pass number
> from the driver instead of stuffing it in the devclass?  (It is
> conceivable that
> different drivers with different passes might share a devclass even,
> though unlikely).
>
> That is, can't you use child->driver->dl_pass directly?  (If a child
> doesn't have a
> driver it can't suspend or resume anyway).
>
> Also, can you explain the EAGAIN logic?  It's not obvious.  Is this how
> you are
> enforcing that already resumed drivers keep traversing their tree in
> subsequent
> passes (if not you need to deal with that in some fashion).
>
> I think we might want to be more explicit about forcing suspend and resume
> to
> traverse the tree for each pass.  The boot-time probe has a BUS_NEW_PASS
> callback
> it uses for this, and bus_generic_new_pass() is what it uses for that.  I
> think
> you might want to create BUS_SUSPEND_PASS and BUS_RESUME_PASS with generic
> versions similar to bus_generic_new_pass.  You would check the DF_SUSPENDED
> flag there to decide whether or not you wanted to call
> BUS_SUSPEND/RESUME_PASS
> or DEVICE_SUSPEND().
>
> The other thing that makes this more complicated is that unlike
> probe/attach,
> we might need to actually ask the bus to suspend and resume the device so
> that
> the bus can do power management.  Right now this works because the bus
> devices
> suspend everything in one pass so they can order things correctly (e.g.
> pci_suspend()).  If we want to shut down some devices early we might
> consider
> having a new bus_if method which takes a child and handles suspending that
> specific child (it would call DEVICE_SUSPEND).  For PCI you might get
> something
> like this:
>
> int
> pci_suspend_child(device_t bus, device_t child)
> {
>         struct pci_devinfo *dinfo;
>         int error;
>
>         dinfo = device_get_ivars(child);
>         pci_cfg_save(child, dinfo, 0);
>         error = DEVICE_SUSPEND(child);
>         if (error)
>                 return (error);
>         if (pci_do_power_suspend)
>                 /* set power state for just this child to D3 */
>         return (error);
> }
>
> I need to think a bit more, but I think this is more of a correct model to
> handle
> passes, and will also be cleaner for suspend/resume in general.
>
> --
> John Baldwin
>

Have you had any time to put more thought into imrpoving this?  I've fixed
I think the last few bugs with the PowerPC-side of things, and want to push
this into HEAD after the thaw.  Warner had mentioned in a peer thread that
a second interface, to pass the bus pass along, with a default of calling
the existing bus_resume().

Thoughts?

- Justin