RE: x86-power-control for ARM CPU based system.

RE: x86-power-control for ARM CPU based system.

[Mohaimen Alsamarai]

Adding openbmc mail list

-----Original Message-----
From: Bills, Jason M <jason.m.bills@linux.intel.com> 
Sent: Tuesday, March 23, 2021 4:08 PM
To: Brandon Ong <Brandon.Ong@fii-na.com>
Cc: Lancelot Kao <lancelot.cy.kao@fii-na.com>; Mohaimen Alsamarai <Mohaimen.Alsamarai@fii-na.com>
Subject: Re: x86-power-control for ARM CPU based system.

Hi Brandon,
On 3/22/2021 3:43 PM, Brandon Ong wrote:
> Hi Jason,
> 
> I am currently working on the implementation of the x86-power-control 
> for an ARM CPU based system.
> 
> 
> Is there a way to add a compile option to x86-power-control in order 
> to change the behavior to support the ARM power control logic if it 
> were to be integrated into x86-power-control?
x86-power-control was created to solve specific timing issues with our platforms.  It wasn't designed to be a flexible solution for the community to use.

phosphor-state-manager
(https://github.com/openbmc/phosphor-state-manager) is the OpenBMC community power state manager.  It is designed for flexibility in how different systems change power state.

Rather than add build modifications to x86-power-control for your needs, I'd recommend that you look at phosphor-state-manager which was designed to be customizable for different systems.

Thanks,
-Jason

> 
> Thanks,
> Brandon
> 

Re: x86-power-control for ARM CPU based system.

[Ed Tanous]

On Thu, Apr 29, 2021 at 8:47 AM Mohaimen Alsamarai
<Mohaimen.Alsamarai@fii-na.com> wrote:
>
> Adding openbmc mail list
>
> -----Original Message-----
> From: Bills, Jason M <jason.m.bills@linux.intel.com>
> Sent: Tuesday, March 23, 2021 4:08 PM
> To: Brandon Ong <Brandon.Ong@fii-na.com>
> Cc: Lancelot Kao <lancelot.cy.kao@fii-na.com>; Mohaimen Alsamarai <Mohaimen.Alsamarai@fii-na.com>
> Subject: Re: x86-power-control for ARM CPU based system.
>
> Hi Brandon,
> On 3/22/2021 3:43 PM, Brandon Ong wrote:
> > Hi Jason,
> >
> > I am currently working on the implementation of the x86-power-control
> > for an ARM CPU based system.
> >
> >
> > Is there a way to add a compile option to x86-power-control in order
> > to change the behavior to support the ARM power control logic if it
> > were to be integrated into x86-power-control?
> x86-power-control was created to solve specific timing issues with our platforms.  It wasn't designed to be a flexible solution for the community to use.

And OpenBMC was initially designed for POWER platforms.  Things change :)

Clearly x86-power-control seems to solve more problems, as a lot of
new platforms seem to be preferring it.  If the code being changed is
messy, unmaintainable, or isn't well abstracted, that's a different
discussion, but outright saying nobody else can make use of
x86-power-control seems problematic, and would lead to a power control
daemon per-platform, which seems hard to maintain, and in looking at
the amd patches, an amd specific daemon would largely just copy-paste
95% of x86-power-control today into something like amd-power-control
if we took this to the logical conclusion.

>
> phosphor-state-manager
> (https://github.com/openbmc/phosphor-state-manager) is the OpenBMC community power state manager.  It is designed for flexibility in how different systems change power state.
>
> Rather than add build modifications to x86-power-control for your needs, I'd recommend that you look at phosphor-state-manager which was designed to be customizable for different systems.

phosphor-state-manager has all the problems that you found when you
went to use it, and found it lacking.  Clearly Brandon has found the
same and is looking to make some (hopefully minor) mods to make
x86-power-control more useful in more contexts.  If it's a matter of
code cleanliness or separation, there's certainly a discussion to be
had here, but effectively saying that everyone should go build their
own version of x86-power-control seems wasteful, as a lot of platforms
share similar properties to what x86-power-control does.

The things I see in the patch are:
1. The ability to invert polarities of the inputs.
2. The ability to disable at compile time some of the watchdogs that
don't make sense on certain platforms.
3. disabling the beeper (which I'm not sure is needed so long as you
handle errors silently).
4. A couple of platform-name-specific hardcodes, that I suspect aren't
needed or can be abstracted.

Is there a way we can avoid the duplication of code in this case?

>
> Thanks,
> -Jason
>
> >
> > Thanks,
> > Brandon
> >

Re: x86-power-control for ARM CPU based system.

[Bills, Jason M]

On 5/13/2021 1:55 PM, Ed Tanous wrote:
> On Thu, Apr 29, 2021 at 8:47 AM Mohaimen Alsamarai
> <Mohaimen.Alsamarai@fii-na.com> wrote:
>>
>> Adding openbmc mail list
>>
>> -----Original Message-----
>> From: Bills, Jason M <jason.m.bills@linux.intel.com>
>> Sent: Tuesday, March 23, 2021 4:08 PM
>> To: Brandon Ong <Brandon.Ong@fii-na.com>
>> Cc: Lancelot Kao <lancelot.cy.kao@fii-na.com>; Mohaimen Alsamarai <Mohaimen.Alsamarai@fii-na.com>
>> Subject: Re: x86-power-control for ARM CPU based system.
>>
>> Hi Brandon,
>> On 3/22/2021 3:43 PM, Brandon Ong wrote:
>>> Hi Jason,
>>>
>>> I am currently working on the implementation of the x86-power-control
>>> for an ARM CPU based system.
>>>
>>>
>>> Is there a way to add a compile option to x86-power-control in order
>>> to change the behavior to support the ARM power control logic if it
>>> were to be integrated into x86-power-control?
>> x86-power-control was created to solve specific timing issues with our platforms.  It wasn't designed to be a flexible solution for the community to use.
> 
> And OpenBMC was initially designed for POWER platforms.  Things change :)
> 
> Clearly x86-power-control seems to solve more problems, as a lot of
> new platforms seem to be preferring it.
I'm glad it is working well. :)

> If the code being changed is
> messy, unmaintainable, or isn't well abstracted, that's a different
> discussion,
This is definitely a concern.  Since it wasn't designed as a flexible 
solution, I'm worried it will become fragile with a lot of changes.

> but outright saying nobody else can make use of
> x86-power-control seems problematic, and would lead to a power control
> daemon per-platform, which seems hard to maintain,
This was not my intention, so I apologize if it came out that way. 
Making changes to x86-power-control is definitely better than everyone 
forking their own.

However, I have tried to think of good ways to make x86-power-control 
more flexible and generic and have not come up with anything much 
different from phosphor-state-manager.  So, I worry that we could spend 
a lot of time and effort making x86-power-control flexible only to end 
up with something that is essentially the same as what we already have. 
But I don't want to squash any efforts here, so I'm open to ideas and 
proposals.

> and in looking at
> the amd patches, an amd specific daemon would largely just copy-paste
> 95% of x86-power-control today into something like amd-power-control
> if we took this to the logical conclusion.
95% the same sounds like a minor effort to include in x86-power-control. 
  This particular thread was mentioning a build switch for ARM CPU 
support which made it sound like there would be significant differences, 
so I wanted to make sure that phosphor-state-manager had been evaluated.

> 
>>
>> phosphor-state-manager
>> (https://github.com/openbmc/phosphor-state-manager) is the OpenBMC community power state manager.  It is designed for flexibility in how different systems change power state.
>>
>> Rather than add build modifications to x86-power-control for your needs, I'd recommend that you look at phosphor-state-manager which was designed to be customizable for different systems.
> 
> phosphor-state-manager has all the problems that you found when you
> went to use it, and found it lacking.  Clearly Brandon has found the
> same and is looking to make some (hopefully minor) mods to make
> x86-power-control more useful in more contexts.  If it's a matter of
> code cleanliness or separation, there's certainly a discussion to be
> had here, but effectively saying that everyone should go build their
> own version of x86-power-control seems wasteful, as a lot of platforms
> share similar properties to what x86-power-control does.
I have some vague high-level ideas of trying to figure out how to make 
the power states and event handlers more generic, so that the various 
events, timeouts, and state changes could be customized.  But I haven't 
gone any further than "maybe a class or something". :)  Maybe that level 
of flexibility isn't needed, though...

> 
> The things I see in the patch are:
> 1. The ability to invert polarities of the inputs.
> 2. The ability to disable at compile time some of the watchdogs that
> don't make sense on certain platforms.
> 3. disabling the beeper (which I'm not sure is needed so long as you
> handle errors silently).
> 4. A couple of platform-name-specific hardcodes, that I suspect aren't
> needed or can be abstracted.
> 
> Is there a way we can avoid the duplication of code in this case?
I agree.  These changes sound minor and able to be integrated into 
x86-power-control.  Is a patch already available to look at?

> 
>>
>> Thanks,
>> -Jason
>>
>>>
>>> Thanks,
>>> Brandon
>>>

Re: x86-power-control for ARM CPU based system.

[Ed Tanous]

On Thu, May 13, 2021 at 6:42 PM Bills, Jason M
<jason.m.bills@linux.intel.com> wrote:
>
>
>
> On 5/13/2021 1:55 PM, Ed Tanous wrote:
> > On Thu, Apr 29, 2021 at 8:47 AM Mohaimen Alsamarai
> > <Mohaimen.Alsamarai@fii-na.com> wrote:
> >>
> >> Adding openbmc mail list
> >>
> >> -----Original Message-----
> >> From: Bills, Jason M <jason.m.bills@linux.intel.com>
> >> Sent: Tuesday, March 23, 2021 4:08 PM
> >> To: Brandon Ong <Brandon.Ong@fii-na.com>
> >> Cc: Lancelot Kao <lancelot.cy.kao@fii-na.com>; Mohaimen Alsamarai <Mohaimen.Alsamarai@fii-na.com>
> >> Subject: Re: x86-power-control for ARM CPU based system.
> >>
> >> Hi Brandon,
> >> On 3/22/2021 3:43 PM, Brandon Ong wrote:
> >>> Hi Jason,
> >>>
> >>> I am currently working on the implementation of the x86-power-control
> >>> for an ARM CPU based system.
> >>>
> >>>
> >>> Is there a way to add a compile option to x86-power-control in order
> >>> to change the behavior to support the ARM power control logic if it
> >>> were to be integrated into x86-power-control?
> >> x86-power-control was created to solve specific timing issues with our platforms.  It wasn't designed to be a flexible solution for the community to use.
> >
> > And OpenBMC was initially designed for POWER platforms.  Things change :)
> >
> > Clearly x86-power-control seems to solve more problems, as a lot of
> > new platforms seem to be preferring it.
> I'm glad it is working well. :)
>
> > If the code being changed is
> > messy, unmaintainable, or isn't well abstracted, that's a different
> > discussion,
> This is definitely a concern.  Since it wasn't designed as a flexible
> solution, I'm worried it will become fragile with a lot of changes.
>
> > but outright saying nobody else can make use of
> > x86-power-control seems problematic, and would lead to a power control
> > daemon per-platform, which seems hard to maintain,
> This was not my intention, so I apologize if it came out that way.
> Making changes to x86-power-control is definitely better than everyone
> forking their own.
>
> However, I have tried to think of good ways to make x86-power-control
> more flexible and generic and have not come up with anything much
> different from phosphor-state-manager.  So, I worry that we could spend
> a lot of time and effort making x86-power-control flexible only to end
> up with something that is essentially the same as what we already have.
> But I don't want to squash any efforts here, so I'm open to ideas and
> proposals.

Brandon, it sounds like this is your time to propose something either
here or via patchsets.

>
> > and in looking at
> > the amd patches, an amd specific daemon would largely just copy-paste
> > 95% of x86-power-control today into something like amd-power-control
> > if we took this to the logical conclusion.
> 95% the same sounds like a minor effort to include in x86-power-control.
>   This particular thread was mentioning a build switch for ARM CPU
> support which made it sound like there would be significant differences,
> so I wanted to make sure that phosphor-state-manager had been evaluated.
>
> >
> >>
> >> phosphor-state-manager
> >> (https://github.com/openbmc/phosphor-state-manager) is the OpenBMC community power state manager.  It is designed for flexibility in how different systems change power state.
> >>
> >> Rather than add build modifications to x86-power-control for your needs, I'd recommend that you look at phosphor-state-manager which was designed to be customizable for different systems.
> >
> > phosphor-state-manager has all the problems that you found when you
> > went to use it, and found it lacking.  Clearly Brandon has found the
> > same and is looking to make some (hopefully minor) mods to make
> > x86-power-control more useful in more contexts.  If it's a matter of
> > code cleanliness or separation, there's certainly a discussion to be
> > had here, but effectively saying that everyone should go build their
> > own version of x86-power-control seems wasteful, as a lot of platforms
> > share similar properties to what x86-power-control does.
> I have some vague high-level ideas of trying to figure out how to make
> the power states and event handlers more generic, so that the various
> events, timeouts, and state changes could be customized.  But I haven't
> gone any further than "maybe a class or something". :)  Maybe that level
> of flexibility isn't needed, though...

When I looked at the various forks, I don't really see them changing
any of the state handlers;  The state machine seems largely unchanged,
aside from doing things like removing watch dogs, which are just state
transitions and probably should be configurable anyway for debug, and
even then, I've only seen one get removed (although admittedly I
haven't looked through every line of every fork of x86-power-control
yet).

>
> >
> > The things I see in the patch are:
> > 1. The ability to invert polarities of the inputs.
> > 2. The ability to disable at compile time some of the watchdogs that
> > don't make sense on certain platforms.
> > 3. disabling the beeper (which I'm not sure is needed so long as you
> > handle errors silently).
> > 4. A couple of platform-name-specific hardcodes, that I suspect aren't
> > needed or can be abstracted.
> >
> > Is there a way we can avoid the duplication of code in this case?
> I agree.  These changes sound minor and able to be integrated into
> x86-power-control.  Is a patch already available to look at?

Not that I'm aware of, but glad to hear you're open to things.

Brandon, sounds like getting a couple patches put up is your next step.

-Ed

>
> >
> >>
> >> Thanks,
> >> -Jason
> >>
> >>>
> >>> Thanks,
> >>> Brandon
> >>>