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 at fii-na.com> wrote:
>>
>> Adding openbmc mail list
>>
>> -----Original Message-----
>> From: Bills, Jason M <jason.m.bills at linux.intel.com>
>> Sent: Tuesday, March 23, 2021 4:08 PM
>> To: Brandon Ong <Brandon.Ong at fii-na.com>
>> Cc: Lancelot Kao <lancelot.cy.kao at fii-na.com>; Mohaimen Alsamarai <Mohaimen.Alsamarai at 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
>>>