Re: [RFC 1/3] system-power: Add system power and restart framework
On Tue 2017-01-31 18:46:58, Thierry Reding wrote:
> On Mon, Jan 30, 2017 at 10:53:01PM +0100, Pavel Machek wrote:
> > Hi!
> >
> >
> > > +struct system_power_chip;
> > > +
> > > +struct system_power_ops {
> > > + int (*restart)(struct system_power_chip *chip, enum reboot_mode mode,
> > > +char *cmd);
> > > + int (*power_off_prepare)(struct system_power_chip *chip);
> > > + int (*power_off)(struct system_power_chip *chip);
> > > +};
> > > +
> > > +struct system_power_chip {
> > > + const struct system_power_ops *ops;
> > > + struct list_head list;
> > > + struct device *dev;
> > > +};
> >
> > Is it useful to have two structures? AFAICT one would do.
>
> Yeah, one structure works fine. I was drawing inspiration from other
> subsystems that have a separate structure for these. I've merged the
> operations into the struct system_power_chip now because that gives
> us some more flexiblity, for example in cases where a chip can be a
> power controller and a reset controller, but sometimes we may want
> it to be only one of them.
>
> > Do we always have struct device * to work with? IMO we have nothing
> > suitable for example in the ACPI case. Would void * be more suitable?
>
> The struct device * was meant to be purely optional, but working with
> the code some more today and doing some more conversions, I've resorted
> to adding a separate field (const char *name) that takes precedence. So
> if a chip specifies both a .dev and .name field, then .name will be the
> user visible string, otherwise dev_name(.dev) will be used in
> messages.
Thanks!
> > Could you convert someting (acpi?) to the new framework as
> > demonstration?
>
> I had originally only converted architecture code to call into system
> power instead of the notifier chain and added a driver for a chip that
> I want to get this to work on. I've now converted a couple of other
> drivers from drivers/power/reset as well as ACPI. I've also added a
> very rudimentary prioritization mechanism that I've validated on the
> specific setup that I'm working on.
>
> On the Jetson TX1 that I'm testing this on, the SoC has a way of
> resetting itself. This has the advantage that some of the registers are
> kept intact over the reset, and this in turn is used to control early
> boot, so that specific recovery modes can be used. However, the board
> has to be powered off using the PMIC (via I2C). The patches achieve this
> by splitting up restart and power off into two steps, prepare and
> restart/power-off, as well as levels to prioritize. On Jetson TX1 the
> PMIC will be higher priority than the SoC (determined by the level) and
> therefore be able to override the SoC restart mechanism if we want to.
> If we don't we simply instruct the MAX77620 driver not to register the
> restart callback, in which case the SoC implementation will be used.
>
> I've uploaded all of it to a branch on github:
>
> https://github.com/thierryreding/linux/tree/system-power
>
> It's rather lengthy, so I'm not sure if it makes sense to send to the
> lists right away.
It is easier to review on lists, but no reasons to do it now.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [RFC 1/3] system-power: Add system power and restart framework
On Tue 2017-01-31 18:46:58, Thierry Reding wrote:
> On Mon, Jan 30, 2017 at 10:53:01PM +0100, Pavel Machek wrote:
> > Hi!
> >
> >
> > > +struct system_power_chip;
> > > +
> > > +struct system_power_ops {
> > > + int (*restart)(struct system_power_chip *chip, enum reboot_mode mode,
> > > +char *cmd);
> > > + int (*power_off_prepare)(struct system_power_chip *chip);
> > > + int (*power_off)(struct system_power_chip *chip);
> > > +};
> > > +
> > > +struct system_power_chip {
> > > + const struct system_power_ops *ops;
> > > + struct list_head list;
> > > + struct device *dev;
> > > +};
> >
> > Is it useful to have two structures? AFAICT one would do.
>
> Yeah, one structure works fine. I was drawing inspiration from other
> subsystems that have a separate structure for these. I've merged the
> operations into the struct system_power_chip now because that gives
> us some more flexiblity, for example in cases where a chip can be a
> power controller and a reset controller, but sometimes we may want
> it to be only one of them.
>
> > Do we always have struct device * to work with? IMO we have nothing
> > suitable for example in the ACPI case. Would void * be more suitable?
>
> The struct device * was meant to be purely optional, but working with
> the code some more today and doing some more conversions, I've resorted
> to adding a separate field (const char *name) that takes precedence. So
> if a chip specifies both a .dev and .name field, then .name will be the
> user visible string, otherwise dev_name(.dev) will be used in
> messages.
Thanks!
> > Could you convert someting (acpi?) to the new framework as
> > demonstration?
>
> I had originally only converted architecture code to call into system
> power instead of the notifier chain and added a driver for a chip that
> I want to get this to work on. I've now converted a couple of other
> drivers from drivers/power/reset as well as ACPI. I've also added a
> very rudimentary prioritization mechanism that I've validated on the
> specific setup that I'm working on.
>
> On the Jetson TX1 that I'm testing this on, the SoC has a way of
> resetting itself. This has the advantage that some of the registers are
> kept intact over the reset, and this in turn is used to control early
> boot, so that specific recovery modes can be used. However, the board
> has to be powered off using the PMIC (via I2C). The patches achieve this
> by splitting up restart and power off into two steps, prepare and
> restart/power-off, as well as levels to prioritize. On Jetson TX1 the
> PMIC will be higher priority than the SoC (determined by the level) and
> therefore be able to override the SoC restart mechanism if we want to.
> If we don't we simply instruct the MAX77620 driver not to register the
> restart callback, in which case the SoC implementation will be used.
>
> I've uploaded all of it to a branch on github:
>
> https://github.com/thierryreding/linux/tree/system-power
>
> It's rather lengthy, so I'm not sure if it makes sense to send to the
> lists right away.
It is easier to review on lists, but no reasons to do it now.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
Re: [RFC 1/3] system-power: Add system power and restart framework
On Mon, Jan 30, 2017 at 10:53:01PM +0100, Pavel Machek wrote:
> Hi!
>
>
> > +struct system_power_chip;
> > +
> > +struct system_power_ops {
> > + int (*restart)(struct system_power_chip *chip, enum reboot_mode mode,
> > + char *cmd);
> > + int (*power_off_prepare)(struct system_power_chip *chip);
> > + int (*power_off)(struct system_power_chip *chip);
> > +};
> > +
> > +struct system_power_chip {
> > + const struct system_power_ops *ops;
> > + struct list_head list;
> > + struct device *dev;
> > +};
>
> Is it useful to have two structures? AFAICT one would do.
Yeah, one structure works fine. I was drawing inspiration from other
subsystems that have a separate structure for these. I've merged the
operations into the struct system_power_chip now because that gives
us some more flexiblity, for example in cases where a chip can be a
power controller and a reset controller, but sometimes we may want
it to be only one of them.
> Do we always have struct device * to work with? IMO we have nothing
> suitable for example in the ACPI case. Would void * be more suitable?
The struct device * was meant to be purely optional, but working with
the code some more today and doing some more conversions, I've resorted
to adding a separate field (const char *name) that takes precedence. So
if a chip specifies both a .dev and .name field, then .name will be the
user visible string, otherwise dev_name(.dev) will be used in messages.
> Could you convert someting (acpi?) to the new framework as
> demonstration?
I had originally only converted architecture code to call into system
power instead of the notifier chain and added a driver for a chip that
I want to get this to work on. I've now converted a couple of other
drivers from drivers/power/reset as well as ACPI. I've also added a
very rudimentary prioritization mechanism that I've validated on the
specific setup that I'm working on.
On the Jetson TX1 that I'm testing this on, the SoC has a way of
resetting itself. This has the advantage that some of the registers are
kept intact over the reset, and this in turn is used to control early
boot, so that specific recovery modes can be used. However, the board
has to be powered off using the PMIC (via I2C). The patches achieve this
by splitting up restart and power off into two steps, prepare and
restart/power-off, as well as levels to prioritize. On Jetson TX1 the
PMIC will be higher priority than the SoC (determined by the level) and
therefore be able to override the SoC restart mechanism if we want to.
If we don't we simply instruct the MAX77620 driver not to register the
restart callback, in which case the SoC implementation will be used.
I've uploaded all of it to a branch on github:
https://github.com/thierryreding/linux/tree/system-power
It's rather lengthy, so I'm not sure if it makes sense to send to the
lists right away.
Thierry
signature.asc
Description: PGP signature
Re: [RFC 1/3] system-power: Add system power and restart framework
On Mon, Jan 30, 2017 at 10:53:01PM +0100, Pavel Machek wrote:
> Hi!
>
>
> > +struct system_power_chip;
> > +
> > +struct system_power_ops {
> > + int (*restart)(struct system_power_chip *chip, enum reboot_mode mode,
> > + char *cmd);
> > + int (*power_off_prepare)(struct system_power_chip *chip);
> > + int (*power_off)(struct system_power_chip *chip);
> > +};
> > +
> > +struct system_power_chip {
> > + const struct system_power_ops *ops;
> > + struct list_head list;
> > + struct device *dev;
> > +};
>
> Is it useful to have two structures? AFAICT one would do.
Yeah, one structure works fine. I was drawing inspiration from other
subsystems that have a separate structure for these. I've merged the
operations into the struct system_power_chip now because that gives
us some more flexiblity, for example in cases where a chip can be a
power controller and a reset controller, but sometimes we may want
it to be only one of them.
> Do we always have struct device * to work with? IMO we have nothing
> suitable for example in the ACPI case. Would void * be more suitable?
The struct device * was meant to be purely optional, but working with
the code some more today and doing some more conversions, I've resorted
to adding a separate field (const char *name) that takes precedence. So
if a chip specifies both a .dev and .name field, then .name will be the
user visible string, otherwise dev_name(.dev) will be used in messages.
> Could you convert someting (acpi?) to the new framework as
> demonstration?
I had originally only converted architecture code to call into system
power instead of the notifier chain and added a driver for a chip that
I want to get this to work on. I've now converted a couple of other
drivers from drivers/power/reset as well as ACPI. I've also added a
very rudimentary prioritization mechanism that I've validated on the
specific setup that I'm working on.
On the Jetson TX1 that I'm testing this on, the SoC has a way of
resetting itself. This has the advantage that some of the registers are
kept intact over the reset, and this in turn is used to control early
boot, so that specific recovery modes can be used. However, the board
has to be powered off using the PMIC (via I2C). The patches achieve this
by splitting up restart and power off into two steps, prepare and
restart/power-off, as well as levels to prioritize. On Jetson TX1 the
PMIC will be higher priority than the SoC (determined by the level) and
therefore be able to override the SoC restart mechanism if we want to.
If we don't we simply instruct the MAX77620 driver not to register the
restart callback, in which case the SoC implementation will be used.
I've uploaded all of it to a branch on github:
https://github.com/thierryreding/linux/tree/system-power
It's rather lengthy, so I'm not sure if it makes sense to send to the
lists right away.
Thierry
signature.asc
Description: PGP signature
Re: [RFC 1/3] system-power: Add system power and restart framework
Hi!
> +struct system_power_chip;
> +
> +struct system_power_ops {
> + int (*restart)(struct system_power_chip *chip, enum reboot_mode mode,
> +char *cmd);
> + int (*power_off_prepare)(struct system_power_chip *chip);
> + int (*power_off)(struct system_power_chip *chip);
> +};
> +
> +struct system_power_chip {
> + const struct system_power_ops *ops;
> + struct list_head list;
> + struct device *dev;
> +};
Is it useful to have two structures? AFAICT one would do.
Do we always have struct device * to work with? IMO we have nothing
suitable for example in the ACPI case. Would void * be more suitable?
Could you convert someting (acpi?) to the new framework as
demonstration?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [RFC 1/3] system-power: Add system power and restart framework
Hi!
> +struct system_power_chip;
> +
> +struct system_power_ops {
> + int (*restart)(struct system_power_chip *chip, enum reboot_mode mode,
> +char *cmd);
> + int (*power_off_prepare)(struct system_power_chip *chip);
> + int (*power_off)(struct system_power_chip *chip);
> +};
> +
> +struct system_power_chip {
> + const struct system_power_ops *ops;
> + struct list_head list;
> + struct device *dev;
> +};
Is it useful to have two structures? AFAICT one would do.
Do we always have struct device * to work with? IMO we have nothing
suitable for example in the ACPI case. Would void * be more suitable?
Could you convert someting (acpi?) to the new framework as
demonstration?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
