x86-power-control

x86-power-control

[Vijay Khemka]

[-- Attachment #1: Type: text/plain, Size: 2130 bytes --]

Hi Ed,
I am trying to use x86-power-control and see there are lots of hard coded values which needs to be configurable as per platform.

  1.  Name of GPIO line, this should be configurable and should also support GPIO number if user doesn’t want to define line name in DTS.
  2.  All delay time as it varies for us per platform like powerPulseTimeMs is 1 sec instead of 200 ms and powerPulseTimeMs is 6 sec instead of 15 sec and these varies for different FB platforms.
  3.  GPIO lines to be monitored, not everyone needs SIO_S5 monitoring or NMI_OUT etc.
  4.  Enable/disable passthrough

Please suggest what is the best way to make these changes. I am ready to work on this to make required change. We can have these config option defined in entity manager or we can accept a new json file for this configuration.

One more question on code, I see following code requires powerButtonMask to be set before aquiring GPIO line. Please let me know who sets this powerButtonMask to true. I know this is related to GPIO passthrough but still couldn’t understand where in code it gets set until someone call set-property of dbus.


power_control::powerButtonIface->register_property(

        "ButtonMasked", false, [](const bool requested, bool& current) {

            if (requested)

            {

                if (power_control::powerButtonMask)

                {

                    return 1;

                }

                if (!power_control::setGPIOOutput(

                        "POWER_OUT", 1, power_control::powerButtonMask))

                {

                    throw std::runtime_error("Failed to request GPIO");

                    return 0;

                }

                std::cerr << "Power Button Masked.\n";

            }

            else

            {

                if (!power_control::powerButtonMask)

                {

                    return 1;

                }

                std::cerr << "Power Button Un-masked\n";

                power_control::powerButtonMask.reset();

            }

Regards
-Vijay

[-- Attachment #2: Type: text/html, Size: 11453 bytes --]

Re: x86-power-control

[Michael Richardson]

[-- Attachment #1: Type: text/plain, Size: 1429 bytes --]


Vijay Khemka <vijaykhemka@fb.com> wrote:
    > 1.  Name of GPIO line, this should be configurable and should also
    > support GPIO number if user doesn’t want to define line name in DTS.

Why in a target system wouldn't naming it in DTS be the most reliable and
most easily accessible mechanism?  I can see that in development being able to define
things helps, but it is not like the production systems would have wire-wrap
where the GPIO pin might change.

    > 2.  All delay time as it varies for us per platform like
    > powerPulseTimeMs is 1 sec instead of 200 ms and powerPulseTimeMs is 6
    > sec instead of 15 sec and these varies for different FB platforms.

    > 3.  GPIO lines to be monitored, not everyone needs SIO_S5 monitoring or NMI_OUT etc.
    > 4.  Enable/disable passthrough

    > Please suggest what is the best way to make these changes. I am ready
    > to work on this to make required change. We can have these config
    > option defined in entity manager or we can accept a new json file for
    > this configuration.

I take it that this isn't a configuration that should be visible to
operators, just to integrators.

--
]               Never tell me the odds!                 | ipv6 mesh networks [
]   Michael Richardson, Sandelman Software Works        | network architect  [
]     mcr@sandelman.ca  http://www.sandelman.ca/        |   ruby on rails    [


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

Re: x86-power-control

[Oskar Senft]

[-- Attachment #1: Type: text/plain, Size: 854 bytes --]

Hi Michael

Why in a target system wouldn't naming it in DTS be the most reliable and
> most easily accessible mechanism?  I can see that in development being
> able to define
> things helps, but it is not like the production systems would have
> wire-wrap
> where the GPIO pin might change.
>

I totally agree. I was actually experimenting on our platform (TYAN S7106)
to name GPIO pins in the DTS, but I couldn't figure out how to read those
names from userspace.

Here's what I tried in the DTS:

&gpio {
        pin_gpio_d2 {
                gpios = <ASPEED_GPIO(D, 2) GPIO_ACTIVE_HIGH>;
                input;
                line-name = "SYS_PWROK_BUF";
        };
...

However, from what I can tell line-name is actually only relevant when used
together with "gpiohog", which is not what we want.

Did you manage to make this work?

Thanks
Oskar.

[-- Attachment #2: Type: text/html, Size: 1670 bytes --]

Re: x86-power-control

[Bills, Jason M]

Hi Vijay

On 10/16/2019 6:13 PM, Vijay Khemka wrote:
> One more question on code, I see following code requires powerButtonMask 
> to be set before aquiring GPIO line. Please let me know who sets this 
> powerButtonMask to true. I know this is related to GPIO passthrough but 
> still couldn’t understand where in code it gets set until someone call 
> set-property of dbus.

powerButtonMask is a gpiod::line object that returns true if it 
references a GPIO line and false otherwise.

> 
> power_control::powerButtonIface->register_property(
> 
> "ButtonMasked", false, [](constboolrequested, bool& current) {
> 
> if(requested)
> 
> {
> 
> if(power_control::powerButtonMask)
> 
> {
This will return if powerButtonMask already references a GPIO.

> 
> return1;
> 
> }
> 
> if(!power_control::setGPIOOutput(
> 
> "POWER_OUT", 1, power_control::powerButtonMask))
Otherwise, this will request the "POWER_OUT" GPIO and assign it to 
powerButtonMask (which will make it return true).

> 
> {
> 
> throwstd::runtime_error("Failed to request GPIO");
> 
> return0;
> 
> }
> 
> std::cerr << "Power Button Masked.\n";
> 
> }
> 
> else
> 
> {
> 
> if(!power_control::powerButtonMask)
> 
> {
This will return if powerButtonMask does not reference a GPIO line.

> 
> return1;
> 
> }
> 
> std::cerr << "Power Button Un-masked\n";
> 
> power_control::powerButtonMask.reset();
Otherwise this will reset powerButtonMask to release the "POWER_OUT" 
GPIO (which will make it return false).

> 
> }
> 
> Regards
> 
> -Vijay
> 

Re: x86-power-control

[Bills, Jason M]

On 10/17/2019 6:38 AM, Oskar Senft wrote:
> Hi Michael
> 
>     Why in a target system wouldn't naming it in DTS be the most
>     reliable and
>     most easily accessible mechanism?  I can see that in development
>     being able to define
>     things helps, but it is not like the production systems would have
>     wire-wrap
>     where the GPIO pin might change.
> 
> 
> I totally agree. I was actually experimenting on our platform (TYAN 
> S7106) to name GPIO pins in the DTS, but I couldn't figure out how to 
> read those names from userspace.
> 
> Here's what I tried in the DTS:
> 
> &gpio {
>          pin_gpio_d2 {
>                  gpios = <ASPEED_GPIO(D, 2) GPIO_ACTIVE_HIGH>;
>                  input;
>                  line-name = "SYS_PWROK_BUF";
>          };
> ...
> 
> However, from what I can tell line-name is actually only relevant when 
> used together with "gpiohog", which is not what we want.
> 
> Did you manage to make this work?

In our DTS we use gpio-line-names to name all of the GPIOs in one block:

&gpio {
	status = "okay";
	gpio-line-names =
	/*A0-A7*/	"","","","","","","","",
	/*B0-B7*/	"","","","","","","","",
	/*C0-C7*/	"","","","","","","","",
	/*D0-D7*/	"","","","","","","","",
	/*E0-E7*/ 
"RESET_BUTTON","RESET_OUT","POWER_BUTTON","POWER_OUT","","","","",
	/*F0-F7*/	"NMI_OUT","","","","CPU_ERR0","CPU_ERR1","","",
	/*G0-G7*/	"CPU_ERR2","CPU_CATERR","PCH_BMC_THERMTRIP","","","","","",
	/*H0-H7*/	"","","","","","","","",
	/*I0-I7*/	"","","","","","","","",
	/*J0-J7*/	"","","","","","","","",
	/*K0-K7*/	"","","","","","","","",
	/*L0-L7*/	"","","","","","","","",
	/*M0-M7*/	"","","","","","","","",
	/*N0-N7*/	"","","","","","","","",
	/*O0-O7*/	"","","","","","","","",
	/*P0-P7*/	"","","","","","","","",
	/*Q0-Q7*/	"","","","","","","","",
	/*R0-R7*/	"","","","","","","","",
	/*S0-S7*/	"","","","","","","","",
	/*T0-T7*/	"","","","","","","","",
	/*U0-U7*/	"","","","","","","","",
	/*V0-V7*/	"","","","","","","","",
	/*W0-W7*/	"","","","","","","","",
	/*X0-X7*/	"","","","","","","","",
	/*Y0-Y7*/	"SIO_S3","SIO_S5","","SIO_ONCONTROL","","","","",
	/*Z0-Z7*/	"","SIO_POWER_GOOD","","","","","","",
	/*AA0-AA7*/	"P3VBAT_BRIDGE_EN","","","","","","SMI","POST_COMPLETE",
	/*AB0-AB7*/	"","NMI_BUTTON","ID_BUTTON","PS_PWROK","","","","",
	/*AC0-AC7*/	"","","","","","","","";
};

https://github.com/Intel-BMC/openbmc/blob/intel/meta-openbmc-mods/meta-ast2500/recipes-kernel/linux/linux-aspeed/0001-arm-dts-add-DTS-for-Intel-platforms.patch#L144

Then, in user space, libgpiod has a gpiod::find_line() function that 
takes the GPIO name and returns a gpiod::line object that can request 
various functions from the GPIO.

Here is an example of requesting GPIO edge events in x86-power-control:
https://github.com/openbmc/x86-power-control/blob/master/power-control-x86/src/power_control.cpp#L805

> 
> Thanks
> Oskar.

Re: x86-power-control

[Oskar Senft]

[-- Attachment #1: Type: text/plain, Size: 714 bytes --]

Hi Jason


> In our DTS we use gpio-line-names to name all of the GPIOs in one block:
>
> &gpio {
>         status = "okay";
>         gpio-line-names =
> [...]
>         /*D0-D7*/       "","","","","","","","",
>         /*E0-E7*/
>  "RESET_BUTTON","RESET_OUT","POWER_BUTTON","POWER_OUT","","","","",
>         /*F0-F7*/       "NMI_OUT","","","","CPU_ERR0","CPU_ERR1","","",
>         /*G0-G7*/
>  "CPU_ERR2","CPU_CATERR","PCH_BMC_THERMTRIP","","","","","",
>

Ugh, ok - that's a nice trick. But it's also a little messy :-/ Is that the
"officially recommended" way? I guess, since the other option I tried
doesn't work!.

Is that purely used for naming lines, or does it have any semantic impact?

Thanks
Oskar.

[-- Attachment #2: Type: text/html, Size: 1450 bytes --]

Re: x86-power-control

[Bills, Jason M]

Hi Oskar,

On 10/17/2019 10:21 AM, Oskar Senft wrote:
> Hi Jason
> 
> 
>     In our DTS we use gpio-line-names to name all of the GPIOs in one block:
> 
>     &gpio {
>              status = "okay";
>              gpio-line-names =
>     [...]
>              /*D0-D7*/       "","","","","","","","",
>              /*E0-E7*/     
>       "RESET_BUTTON","RESET_OUT","POWER_BUTTON","POWER_OUT","","","","",
>              /*F0-F7*/       "NMI_OUT","","","","CPU_ERR0","CPU_ERR1","","",
>              /*G0-G7*/     
>       "CPU_ERR2","CPU_CATERR","PCH_BMC_THERMTRIP","","","","","",
> 
> 
> Ugh, ok - that's a nice trick. But it's also a little messy :-/ Is that 
> the "officially recommended" way? I guess, since the other option I 
> tried doesn't work!.
I'm not completely sure.  I remember seeing this approach as an example 
when studying up on libgpiod and a quick search showed this approach 
used for other DTS files such as for Raspberry Pi 
(https://github.com/raspberrypi/linux/blob/e33ef2c4cd5dc96aa05a7d328eff61c183c94748/arch/arm/boot/dts/bcm2835-rpi-b-plus.dts#L37). 
  So, if not "officially recommended" it's at least not unheard of. :)

> 
> Is that purely used for naming lines, or does it have any semantic impact?
As far as I know, this is purely used for naming lines for libpiod and 
associated tools such as 'gpioinfo'.  You can still find and access GPIO 
lines by chip and line number.

> 
> Thanks
> Oskar.

Re: x86-power-control

[Vijay Khemka]

On 10/17/19, 2:57 AM, "openbmc on behalf of Michael Richardson" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of mcr@sandelman.ca> wrote:

    
    Vijay Khemka <vijaykhemka@fb.com> wrote:
        > 1.  Name of GPIO line, this should be configurable and should also
        > support GPIO number if user doesn’t want to define line name in DTS.
    
    Why in a target system wouldn't naming it in DTS be the most reliable and
    most easily accessible mechanism?  I can see that in development being able to define
    things helps, but it is not like the production systems would have wire-wrap
    where the GPIO pin might change.

I am not ruling out DTS definition but keeping that as optional. Many platform doesn't 
want to change kernel and want to keep dts minimal as well common across multiple
platform. So providing this option will help developer.
    
        > 2.  All delay time as it varies for us per platform like
        > powerPulseTimeMs is 1 sec instead of 200 ms and powerPulseTimeMs is 6
        > sec instead of 15 sec and these varies for different FB platforms.
    
        > 3.  GPIO lines to be monitored, not everyone needs SIO_S5 monitoring or NMI_OUT etc.
        > 4.  Enable/disable passthrough
    
        > Please suggest what is the best way to make these changes. I am ready
        > to work on this to make required change. We can have these config
        > option defined in entity manager or we can accept a new json file for
        > this configuration.
    
    I take it that this isn't a configuration that should be visible to
    operators, just to integrators.
Yes, you are right.
    
    --
    ]               Never tell me the odds!                 | ipv6 mesh networks [
    ]   Michael Richardson, Sandelman Software Works        | network architect  [
    ]     mcr@sandelman.ca  http://www.sandelman.ca/        |   ruby on rails    [
    
    

Re: x86-power-control

[Vijay Khemka]

[-- Attachment #1: Type: text/plain, Size: 1300 bytes --]



From: openbmc <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org> on behalf of Oskar Senft <osk@google.com>
Date: Thursday, October 17, 2019 at 6:39 AM
To: Michael Richardson <mcr@sandelman.ca>
Cc: OpenBMC Maillist <openbmc@lists.ozlabs.org>
Subject: Re: x86-power-control

Hi Michael

Why in a target system wouldn't naming it in DTS be the most reliable and
most easily accessible mechanism?  I can see that in development being able to define
things helps, but it is not like the production systems would have wire-wrap
where the GPIO pin might change.

I totally agree. I was actually experimenting on our platform (TYAN S7106) to name GPIO pins in the DTS, but I couldn't figure out how to read those names from userspace.

Here's what I tried in the DTS:

&gpio {
        pin_gpio_d2 {
                gpios = <ASPEED_GPIO(D, 2) GPIO_ACTIVE_HIGH>;
                input;
                line-name = "SYS_PWROK_BUF";
        };
...

Another thing here, I want to be able to map actual name defined in DTS to real name used in
Power-control.

However, from what I can tell line-name is actually only relevant when used together with "gpiohog", which is not what we want.

It is mainly used via libgpiod.

Did you manage to make this work?

Thanks
Oskar.

[-- Attachment #2: Type: text/html, Size: 5161 bytes --]

Re: x86-power-control

[Vijay Khemka]

On 10/17/19, 9:03 AM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:

    Hi Vijay
    
    On 10/16/2019 6:13 PM, Vijay Khemka wrote:
    > One more question on code, I see following code requires powerButtonMask 
    > to be set before aquiring GPIO line. Please let me know who sets this 
    > powerButtonMask to true. I know this is related to GPIO passthrough but 
    > still couldn’t understand where in code it gets set until someone call 
    > set-property of dbus.
    
    powerButtonMask is a gpiod::line object that returns true if it 
    references a GPIO line and false otherwise.

I understood code but my point here is there will not be any 
gpiod::line object if powerButtonMask is defined as false. And 
initially it is defined as false means tehre will not be any line 
object created until someone sets it to true. And I don't see it 
any way to set it true other than set-property through dbus.
    
    > 
    > power_control::powerButtonIface->register_property(
    > 
    > "ButtonMasked", false, [](constboolrequested, bool& current) {
    > 
    > if(requested)
    > 
    > {
    > 
    > if(power_control::powerButtonMask)
    > 
    > {
    This will return if powerButtonMask already references a GPIO.
    
    > 
    > return1;
    > 
    > }
    > 
    > if(!power_control::setGPIOOutput(
    > 
    > "POWER_OUT", 1, power_control::powerButtonMask))
    Otherwise, this will request the "POWER_OUT" GPIO and assign it to 
    powerButtonMask (which will make it return true).
    
    > 
    > {
    > 
    > throwstd::runtime_error("Failed to request GPIO");
    > 
    > return0;
    > 
    > }
    > 
    > std::cerr << "Power Button Masked.\n";
    > 
    > }
    > 
    > else
    > 
    > {
    > 
    > if(!power_control::powerButtonMask)
    > 
    > {
    This will return if powerButtonMask does not reference a GPIO line.
    
    > 
    > return1;
    > 
    > }
    > 
    > std::cerr << "Power Button Un-masked\n";
    > 
    > power_control::powerButtonMask.reset();
    Otherwise this will reset powerButtonMask to release the "POWER_OUT" 
    GPIO (which will make it return false).
    
    > 
    > }
    > 
    > Regards
    > 
    > -Vijay
    > 
    

Re: x86-power-control

[Bills, Jason M]

On 10/17/2019 3:32 PM, Vijay Khemka wrote:
> 
> On 10/17/19, 9:03 AM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
> 
>      Hi Vijay
>      
>      On 10/16/2019 6:13 PM, Vijay Khemka wrote:
>      > One more question on code, I see following code requires powerButtonMask
>      > to be set before aquiring GPIO line. Please let me know who sets this
>      > powerButtonMask to true. I know this is related to GPIO passthrough but
>      > still couldn’t understand where in code it gets set until someone call
>      > set-property of dbus.
>      
>      powerButtonMask is a gpiod::line object that returns true if it
>      references a GPIO line and false otherwise.
> 
> I understood code but my point here is there will not be any
> gpiod::line object if powerButtonMask is defined as false. And
> initially it is defined as false means tehre will not be any line
> object created until someone sets it to true. And I don't see it
> any way to set it true other than set-property through dbus.

That's correct.  Masking the power button is something that is done by 
some component outside of power-control.

For example, we currently use it with the Set Front Panel Button Enables 
IPMI command to enable/disable the power button.  So, it is only masked 
when requested.

The actual "POWER_OUT" line for power-control is not permanently 
created, but is asserted using temporary calls like this one:
setGPIOOutputForMs("POWER_OUT", 0, powerPulseTimeMs);

https://github.com/openbmc/x86-power-control/blob/master/power-control-x86/src/power_control.cpp#L946

>      
>      >
>      > power_control::powerButtonIface->register_property(
>      >
>      > "ButtonMasked", false, [](constboolrequested, bool& current) {
>      >
>      > if(requested)
>      >
>      > {
>      >
>      > if(power_control::powerButtonMask)
>      >
>      > {
>      This will return if powerButtonMask already references a GPIO.
>      
>      >
>      > return1;
>      >
>      > }
>      >
>      > if(!power_control::setGPIOOutput(
>      >
>      > "POWER_OUT", 1, power_control::powerButtonMask))
>      Otherwise, this will request the "POWER_OUT" GPIO and assign it to
>      powerButtonMask (which will make it return true).
>      
>      >
>      > {
>      >
>      > throwstd::runtime_error("Failed to request GPIO");
>      >
>      > return0;
>      >
>      > }
>      >
>      > std::cerr << "Power Button Masked.\n";
>      >
>      > }
>      >
>      > else
>      >
>      > {
>      >
>      > if(!power_control::powerButtonMask)
>      >
>      > {
>      This will return if powerButtonMask does not reference a GPIO line.
>      
>      >
>      > return1;
>      >
>      > }
>      >
>      > std::cerr << "Power Button Un-masked\n";
>      >
>      > power_control::powerButtonMask.reset();
>      Otherwise this will reset powerButtonMask to release the "POWER_OUT"
>      GPIO (which will make it return false).
>      
>      >
>      > }
>      >
>      > Regards
>      >
>      > -Vijay
>      >
>      
> 

Re: x86-power-control

[Vijay Khemka]

On 10/17/19, 4:27 PM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:

    
    
    On 10/17/2019 3:32 PM, Vijay Khemka wrote:
    > 
    > On 10/17/19, 9:03 AM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
    > 
    >      Hi Vijay
    >      
    >      On 10/16/2019 6:13 PM, Vijay Khemka wrote:
    >      > One more question on code, I see following code requires powerButtonMask
    >      > to be set before aquiring GPIO line. Please let me know who sets this
    >      > powerButtonMask to true. I know this is related to GPIO passthrough but
    >      > still couldn’t understand where in code it gets set until someone call
    >      > set-property of dbus.
    >      
    >      powerButtonMask is a gpiod::line object that returns true if it
    >      references a GPIO line and false otherwise.
    > 
    > I understood code but my point here is there will not be any
    > gpiod::line object if powerButtonMask is defined as false. And
    > initially it is defined as false means tehre will not be any line
    > object created until someone sets it to true. And I don't see it
    > any way to set it true other than set-property through dbus.
    
    That's correct.  Masking the power button is something that is done by 
    some component outside of power-control.
    
    For example, we currently use it with the Set Front Panel Button Enables 
    IPMI command to enable/disable the power button.  So, it is only masked 
    when requested.
So to use x-86-power-control, either we need to have IPMI command to enable
this or some other daemon has to set this property. Can we have this feature 
optional. I guess this is a prt og GPIO passthrough.
    
    The actual "POWER_OUT" line for power-control is not permanently 
    created, but is asserted using temporary calls like this one:
    setGPIOOutputForMs("POWER_OUT", 0, powerPulseTimeMs);

This function will not run power on/off sequence until button mask is set. It
Will only set GPIO value which is not enough for powering on/off.
    
    https://github.com/openbmc/x86-power-control/blob/master/power-control-x86/src/power_control.cpp#L946
    
    >      
    >      >
    >      > power_control::powerButtonIface->register_property(
    >      >
    >      > "ButtonMasked", false, [](constboolrequested, bool& current) {
    >      >
    >      > if(requested)
    >      >
    >      > {
    >      >
    >      > if(power_control::powerButtonMask)
    >      >
    >      > {
    >      This will return if powerButtonMask already references a GPIO.
    >      
    >      >
    >      > return1;
    >      >
    >      > }
    >      >
    >      > if(!power_control::setGPIOOutput(
    >      >
    >      > "POWER_OUT", 1, power_control::powerButtonMask))
    >      Otherwise, this will request the "POWER_OUT" GPIO and assign it to
    >      powerButtonMask (which will make it return true).
    >      
    >      >
    >      > {
    >      >
    >      > throwstd::runtime_error("Failed to request GPIO");
    >      >
    >      > return0;
    >      >
    >      > }
    >      >
    >      > std::cerr << "Power Button Masked.\n";
    >      >
    >      > }
    >      >
    >      > else
    >      >
    >      > {
    >      >
    >      > if(!power_control::powerButtonMask)
    >      >
    >      > {
    >      This will return if powerButtonMask does not reference a GPIO line.
    >      
    >      >
    >      > return1;
    >      >
    >      > }
    >      >
    >      > std::cerr << "Power Button Un-masked\n";
    >      >
    >      > power_control::powerButtonMask.reset();
    >      Otherwise this will reset powerButtonMask to release the "POWER_OUT"
    >      GPIO (which will make it return false).
    >      
    >      >
    >      > }
    >      >
    >      > Regards
    >      >
    >      > -Vijay
    >      >
    >      
    > 
    

Re: x86-power-control

[Michael Richardson]

[-- Attachment #1: Type: text/plain, Size: 1230 bytes --]


Vijay Khemka <vijaykhemka@fb.com> wrote:
    mcr>     Why in a target system wouldn't naming it in DTS be the most
    mcr> reliable and most easily accessible mechanism?  I can see that in
    mcr> development being able to define things helps, but it is not like the
    mcr> production systems would have wire-wrap where the GPIO pin might
    mcr> change.

    > I am not ruling out DTS definition but keeping that as optional. Many
    > platform doesn't want to change kernel and want to keep dts minimal as
    > well common across multiple platform. So providing this option will
    > help developer.

My experience is that the DTS file is loaded separately from the kernel.
I haven't had a chance to learn how things are booted in practice, since I
haven't gotten real hardware yet.

I would think that the DTS file may be the easiest thing for a VAR to update
compared to editing a configuration file that is embedded in the file system
image. 

-- 
]               Never tell me the odds!                 | ipv6 mesh networks [ 
]   Michael Richardson, Sandelman Software Works        | network architect  [ 
]     mcr@sandelman.ca  http://www.sandelman.ca/        |   ruby on rails    [ 
	

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 487 bytes --]

Re: x86-power-control

[Bills, Jason M]

On 10/17/2019 4:52 PM, Vijay Khemka wrote:
> 
> 
> On 10/17/19, 4:27 PM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
> 
>      
>      
>      On 10/17/2019 3:32 PM, Vijay Khemka wrote:
>      >
>      > On 10/17/19, 9:03 AM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
>      >
>      >      Hi Vijay
>      >
>      >      On 10/16/2019 6:13 PM, Vijay Khemka wrote:
>      >      > One more question on code, I see following code requires powerButtonMask
>      >      > to be set before aquiring GPIO line. Please let me know who sets this
>      >      > powerButtonMask to true. I know this is related to GPIO passthrough but
>      >      > still couldn’t understand where in code it gets set until someone call
>      >      > set-property of dbus.
>      >
>      >      powerButtonMask is a gpiod::line object that returns true if it
>      >      references a GPIO line and false otherwise.
>      >
>      > I understood code but my point here is there will not be any
>      > gpiod::line object if powerButtonMask is defined as false. And
>      > initially it is defined as false means tehre will not be any line
>      > object created until someone sets it to true. And I don't see it
>      > any way to set it true other than set-property through dbus.
>      
>      That's correct.  Masking the power button is something that is done by
>      some component outside of power-control.
>      
>      For example, we currently use it with the Set Front Panel Button Enables
>      IPMI command to enable/disable the power button.  So, it is only masked
>      when requested.
> So to use x-86-power-control, either we need to have IPMI command to enable
> this or some other daemon has to set this property. Can we have this feature
> optional. I guess this is a prt og GPIO passthrough.

Yes, this is part of GPIO passthrough.  When the GPIO is requested, 
passthrough is disabled in the pinctrl driver.  So, to mask the power 
button (disable passthrough), power-control requests and holds the 
"POWER_OUT" GPIO line.

It should behave normally without this property ever getting set.

>      
>      The actual "POWER_OUT" line for power-control is not permanently
>      created, but is asserted using temporary calls like this one:
>      setGPIOOutputForMs("POWER_OUT", 0, powerPulseTimeMs);
> 
> This function will not run power on/off sequence until button mask is set. It
> Will only set GPIO value which is not enough for powering on/off.

Something else is going on, here.  The powerButtonMask is a separate 
feature from driving the "POWER_OUT" pin.  If powerButtonMask is not 
set, then the power on/off should function normally.  There is a special 
case in the setGPIOOutputForMs() code to handle when "POWER_OUT" is 
driven while powerButtonMask is true:

     // If the requested GPIO is masked, use the mask line to set the output
     if (powerButtonMask && name == "POWER_OUT")
     {
         return setMaskedGPIOOutputForMs(powerButtonMask, name, value,
                                         durationMs);
     }
     ...
     // No mask set, so request and set the GPIO normally

So, "POWER_OUT" should work either way, but the default behavior is to 
function without powerButtonMask set.  Are you seeing failures on your 
platform when powerButtonMask is false?

Re: x86-power-control

[Vijay Khemka]

On 10/18/19, 11:02 AM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:

    
    
    On 10/17/2019 4:52 PM, Vijay Khemka wrote:
    > 
    > 
    > On 10/17/19, 4:27 PM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
    > 
    >      
    >      
    >      On 10/17/2019 3:32 PM, Vijay Khemka wrote:
    >      >
    >      > On 10/17/19, 9:03 AM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
    >      >
    >      >      Hi Vijay
    >      >
    >      >      On 10/16/2019 6:13 PM, Vijay Khemka wrote:
    >      >      > One more question on code, I see following code requires powerButtonMask
    >      >      > to be set before aquiring GPIO line. Please let me know who sets this
    >      >      > powerButtonMask to true. I know this is related to GPIO passthrough but
    >      >      > still couldn’t understand where in code it gets set until someone call
    >      >      > set-property of dbus.
    >      >
    >      >      powerButtonMask is a gpiod::line object that returns true if it
    >      >      references a GPIO line and false otherwise.
    >      >
    >      > I understood code but my point here is there will not be any
    >      > gpiod::line object if powerButtonMask is defined as false. And
    >      > initially it is defined as false means tehre will not be any line
    >      > object created until someone sets it to true. And I don't see it
    >      > any way to set it true other than set-property through dbus.
    >      
    >      That's correct.  Masking the power button is something that is done by
    >      some component outside of power-control.
    >      
    >      For example, we currently use it with the Set Front Panel Button Enables
    >      IPMI command to enable/disable the power button.  So, it is only masked
    >      when requested.
    > So to use x-86-power-control, either we need to have IPMI command to enable
    > this or some other daemon has to set this property. Can we have this feature
    > optional. I guess this is a prt og GPIO passthrough.
    
    Yes, this is part of GPIO passthrough.  When the GPIO is requested, 
    passthrough is disabled in the pinctrl driver.  So, to mask the power 
    button (disable passthrough), power-control requests and holds the 
    "POWER_OUT" GPIO line.
    
    It should behave normally without this property ever getting set.
    
    >      
    >      The actual "POWER_OUT" line for power-control is not permanently
    >      created, but is asserted using temporary calls like this one:
    >      setGPIOOutputForMs("POWER_OUT", 0, powerPulseTimeMs);
    > 
    > This function will not run power on/off sequence until button mask is set. It
    > Will only set GPIO value which is not enough for powering on/off.
    
    Something else is going on, here.  The powerButtonMask is a separate 
    feature from driving the "POWER_OUT" pin.  If powerButtonMask is not 
    set, then the power on/off should function normally.  There is a special 
    case in the setGPIOOutputForMs() code to handle when "POWER_OUT" is 
    driven while powerButtonMask is true:
    
         // If the requested GPIO is masked, use the mask line to set the output
         if (powerButtonMask && name == "POWER_OUT")
         {
             return setMaskedGPIOOutputForMs(powerButtonMask, name, value,
                                             durationMs);
         }
         ...
         // No mask set, so request and set the GPIO normally
    
    So, "POWER_OUT" should work either way, but the default behavior is to 
    function without powerButtonMask set.  Are you seeing failures on your 
    platform when powerButtonMask is false?

No, It is not working because it simplpy sets power_out to '0'. But to power on 
it should got down as 0 and come back to 1 after a delay. Which happens only 
in case of powerButtonMask set to true. I guess we may need to fix this.
    

Re: x86-power-control

[Bills, Jason M]

On 10/18/2019 4:04 PM, Vijay Khemka wrote:
> 
> 
> On 10/18/19, 11:02 AM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
> 
>      
>      
>      On 10/17/2019 4:52 PM, Vijay Khemka wrote:
>      >
>      >
>      > On 10/17/19, 4:27 PM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
>      >
>      >
>      >
>      >      On 10/17/2019 3:32 PM, Vijay Khemka wrote:
>      >      >
>      >      > On 10/17/19, 9:03 AM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
>      >      >
>      >      >      Hi Vijay
>      >      >
>      >      >      On 10/16/2019 6:13 PM, Vijay Khemka wrote:
>      >      >      > One more question on code, I see following code requires powerButtonMask
>      >      >      > to be set before aquiring GPIO line. Please let me know who sets this
>      >      >      > powerButtonMask to true. I know this is related to GPIO passthrough but
>      >      >      > still couldn’t understand where in code it gets set until someone call
>      >      >      > set-property of dbus.
>      >      >
>      >      >      powerButtonMask is a gpiod::line object that returns true if it
>      >      >      references a GPIO line and false otherwise.
>      >      >
>      >      > I understood code but my point here is there will not be any
>      >      > gpiod::line object if powerButtonMask is defined as false. And
>      >      > initially it is defined as false means tehre will not be any line
>      >      > object created until someone sets it to true. And I don't see it
>      >      > any way to set it true other than set-property through dbus.
>      >
>      >      That's correct.  Masking the power button is something that is done by
>      >      some component outside of power-control.
>      >
>      >      For example, we currently use it with the Set Front Panel Button Enables
>      >      IPMI command to enable/disable the power button.  So, it is only masked
>      >      when requested.
>      > So to use x-86-power-control, either we need to have IPMI command to enable
>      > this or some other daemon has to set this property. Can we have this feature
>      > optional. I guess this is a prt og GPIO passthrough.
>      
>      Yes, this is part of GPIO passthrough.  When the GPIO is requested,
>      passthrough is disabled in the pinctrl driver.  So, to mask the power
>      button (disable passthrough), power-control requests and holds the
>      "POWER_OUT" GPIO line.
>      
>      It should behave normally without this property ever getting set.
>      
>      >
>      >      The actual "POWER_OUT" line for power-control is not permanently
>      >      created, but is asserted using temporary calls like this one:
>      >      setGPIOOutputForMs("POWER_OUT", 0, powerPulseTimeMs);
>      >
>      > This function will not run power on/off sequence until button mask is set. It
>      > Will only set GPIO value which is not enough for powering on/off.
>      
>      Something else is going on, here.  The powerButtonMask is a separate
>      feature from driving the "POWER_OUT" pin.  If powerButtonMask is not
>      set, then the power on/off should function normally.  There is a special
>      case in the setGPIOOutputForMs() code to handle when "POWER_OUT" is
>      driven while powerButtonMask is true:
>      
>           // If the requested GPIO is masked, use the mask line to set the output
>           if (powerButtonMask && name == "POWER_OUT")
>           {
>               return setMaskedGPIOOutputForMs(powerButtonMask, name, value,
>                                               durationMs);
>           }
>           ...
>           // No mask set, so request and set the GPIO normally
>      
>      So, "POWER_OUT" should work either way, but the default behavior is to
>      function without powerButtonMask set.  Are you seeing failures on your
>      platform when powerButtonMask is false?
> 
> No, It is not working because it simplpy sets power_out to '0'. But to power on
> it should got down as 0 and come back to 1 after a delay. Which happens only
> in case of powerButtonMask set to true. I guess we may need to fix this.

Ahh, okay.  I think I see the issue now.

The issue is that because releasing the GPIO on a system with 
pass-through, sets the power button back to the hardware default 
automatically, the software setting doesn't matter, so it is left at 0.

If you don't need to keep holding the GPIO line for POWER_OUT, I think 
you can just add the code to revert the POWER_OUT value when the timer 
expires.

Take this line:
             // Set the masked GPIO line back to the opposite value
             maskedGPIOLine.set_value(!value);
 From here: 
https://github.com/openbmc/x86-power-control/blob/master/power-control-x86/src/power_control.cpp#L891

And add it here: 
https://github.com/openbmc/x86-power-control/blob/master/power-control-x86/src/power_control.cpp#L932

>      
> 

Re: x86-power-control

[Vijay Khemka]

On 10/21/19, 4:04 PM, "Bills, Jason M" <jason.m.bills@linux.intel.com> wrote:

    
    
    On 10/18/2019 4:04 PM, Vijay Khemka wrote:
    > 
    > 
    > On 10/18/19, 11:02 AM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
    > 
    >      
    >      
    >      On 10/17/2019 4:52 PM, Vijay Khemka wrote:
    >      >
    >      >
    >      > On 10/17/19, 4:27 PM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
    >      >
    >      >
    >      >
    >      >      On 10/17/2019 3:32 PM, Vijay Khemka wrote:
    >      >      >
    >      >      > On 10/17/19, 9:03 AM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
    >      >      >
    >      >      >      Hi Vijay
    >      >      >
    >      >      >      On 10/16/2019 6:13 PM, Vijay Khemka wrote:
    >      >      >      > One more question on code, I see following code requires powerButtonMask
    >      >      >      > to be set before aquiring GPIO line. Please let me know who sets this
    >      >      >      > powerButtonMask to true. I know this is related to GPIO passthrough but
    >      >      >      > still couldn’t understand where in code it gets set until someone call
    >      >      >      > set-property of dbus.
    >      >      >
    >      >      >      powerButtonMask is a gpiod::line object that returns true if it
    >      >      >      references a GPIO line and false otherwise.
    >      >      >
    >      >      > I understood code but my point here is there will not be any
    >      >      > gpiod::line object if powerButtonMask is defined as false. And
    >      >      > initially it is defined as false means tehre will not be any line
    >      >      > object created until someone sets it to true. And I don't see it
    >      >      > any way to set it true other than set-property through dbus.
    >      >
    >      >      That's correct.  Masking the power button is something that is done by
    >      >      some component outside of power-control.
    >      >
    >      >      For example, we currently use it with the Set Front Panel Button Enables
    >      >      IPMI command to enable/disable the power button.  So, it is only masked
    >      >      when requested.
    >      > So to use x-86-power-control, either we need to have IPMI command to enable
    >      > this or some other daemon has to set this property. Can we have this feature
    >      > optional. I guess this is a prt og GPIO passthrough.
    >      
    >      Yes, this is part of GPIO passthrough.  When the GPIO is requested,
    >      passthrough is disabled in the pinctrl driver.  So, to mask the power
    >      button (disable passthrough), power-control requests and holds the
    >      "POWER_OUT" GPIO line.
    >      
    >      It should behave normally without this property ever getting set.
    >      
    >      >
    >      >      The actual "POWER_OUT" line for power-control is not permanently
    >      >      created, but is asserted using temporary calls like this one:
    >      >      setGPIOOutputForMs("POWER_OUT", 0, powerPulseTimeMs);
    >      >
    >      > This function will not run power on/off sequence until button mask is set. It
    >      > Will only set GPIO value which is not enough for powering on/off.
    >      
    >      Something else is going on, here.  The powerButtonMask is a separate
    >      feature from driving the "POWER_OUT" pin.  If powerButtonMask is not
    >      set, then the power on/off should function normally.  There is a special
    >      case in the setGPIOOutputForMs() code to handle when "POWER_OUT" is
    >      driven while powerButtonMask is true:
    >      
    >           // If the requested GPIO is masked, use the mask line to set the output
    >           if (powerButtonMask && name == "POWER_OUT")
    >           {
    >               return setMaskedGPIOOutputForMs(powerButtonMask, name, value,
    >                                               durationMs);
    >           }
    >           ...
    >           // No mask set, so request and set the GPIO normally
    >      
    >      So, "POWER_OUT" should work either way, but the default behavior is to
    >      function without powerButtonMask set.  Are you seeing failures on your
    >      platform when powerButtonMask is false?
    > 
    > No, It is not working because it simplpy sets power_out to '0'. But to power on
    > it should got down as 0 and come back to 1 after a delay. Which happens only
    > in case of powerButtonMask set to true. I guess we may need to fix this.
    
    Ahh, okay.  I think I see the issue now.
    
    The issue is that because releasing the GPIO on a system with 
    pass-through, sets the power button back to the hardware default 
    automatically, the software setting doesn't matter, so it is left at 0.
    
    If you don't need to keep holding the GPIO line for POWER_OUT, I think 
    you can just add the code to revert the POWER_OUT value when the timer 
    expires.
    
    Take this line:
                 // Set the masked GPIO line back to the opposite value
                 maskedGPIOLine.set_value(!value);
     From here: 
    https://github.com/openbmc/x86-power-control/blob/master/power-control-x86/src/power_control.cpp#L891
    
    And add it here: 
    https://github.com/openbmc/x86-power-control/blob/master/power-control-x86/src/power_control.cpp#L932

I already did that as a work around, testing different scenario. Will send patch once I see it working.

I also want to make these timeout values as configurable, either I can add these as a property in dbus interface or 
Entity-manager or have a separate config json file. What would you prefer.

    
    >      
    > 
    

Re: x86-power-control

[Bills, Jason M]

On 10/21/2019 6:15 PM, Vijay Khemka wrote:
> 
> 
> On 10/21/19, 4:04 PM, "Bills, Jason M" <jason.m.bills@linux.intel.com> wrote:
> 
>      
>      
>      On 10/18/2019 4:04 PM, Vijay Khemka wrote:
>      >
>      >
>      > On 10/18/19, 11:02 AM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
>      >
>      >
>      >
>      >      On 10/17/2019 4:52 PM, Vijay Khemka wrote:
>      >      >
>      >      >
>      >      > On 10/17/19, 4:27 PM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
>      >      >
>      >      >
>      >      >
>      >      >      On 10/17/2019 3:32 PM, Vijay Khemka wrote:
>      >      >      >
>      >      >      > On 10/17/19, 9:03 AM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
>      >      >      >
>      >      >      >      Hi Vijay
>      >      >      >
>      >      >      >      On 10/16/2019 6:13 PM, Vijay Khemka wrote:
>      >      >      >      > One more question on code, I see following code requires powerButtonMask
>      >      >      >      > to be set before aquiring GPIO line. Please let me know who sets this
>      >      >      >      > powerButtonMask to true. I know this is related to GPIO passthrough but
>      >      >      >      > still couldn’t understand where in code it gets set until someone call
>      >      >      >      > set-property of dbus.
>      >      >      >
>      >      >      >      powerButtonMask is a gpiod::line object that returns true if it
>      >      >      >      references a GPIO line and false otherwise.
>      >      >      >
>      >      >      > I understood code but my point here is there will not be any
>      >      >      > gpiod::line object if powerButtonMask is defined as false. And
>      >      >      > initially it is defined as false means tehre will not be any line
>      >      >      > object created until someone sets it to true. And I don't see it
>      >      >      > any way to set it true other than set-property through dbus.
>      >      >
>      >      >      That's correct.  Masking the power button is something that is done by
>      >      >      some component outside of power-control.
>      >      >
>      >      >      For example, we currently use it with the Set Front Panel Button Enables
>      >      >      IPMI command to enable/disable the power button.  So, it is only masked
>      >      >      when requested.
>      >      > So to use x-86-power-control, either we need to have IPMI command to enable
>      >      > this or some other daemon has to set this property. Can we have this feature
>      >      > optional. I guess this is a prt og GPIO passthrough.
>      >
>      >      Yes, this is part of GPIO passthrough.  When the GPIO is requested,
>      >      passthrough is disabled in the pinctrl driver.  So, to mask the power
>      >      button (disable passthrough), power-control requests and holds the
>      >      "POWER_OUT" GPIO line.
>      >
>      >      It should behave normally without this property ever getting set.
>      >
>      >      >
>      >      >      The actual "POWER_OUT" line for power-control is not permanently
>      >      >      created, but is asserted using temporary calls like this one:
>      >      >      setGPIOOutputForMs("POWER_OUT", 0, powerPulseTimeMs);
>      >      >
>      >      > This function will not run power on/off sequence until button mask is set. It
>      >      > Will only set GPIO value which is not enough for powering on/off.
>      >
>      >      Something else is going on, here.  The powerButtonMask is a separate
>      >      feature from driving the "POWER_OUT" pin.  If powerButtonMask is not
>      >      set, then the power on/off should function normally.  There is a special
>      >      case in the setGPIOOutputForMs() code to handle when "POWER_OUT" is
>      >      driven while powerButtonMask is true:
>      >
>      >           // If the requested GPIO is masked, use the mask line to set the output
>      >           if (powerButtonMask && name == "POWER_OUT")
>      >           {
>      >               return setMaskedGPIOOutputForMs(powerButtonMask, name, value,
>      >                                               durationMs);
>      >           }
>      >           ...
>      >           // No mask set, so request and set the GPIO normally
>      >
>      >      So, "POWER_OUT" should work either way, but the default behavior is to
>      >      function without powerButtonMask set.  Are you seeing failures on your
>      >      platform when powerButtonMask is false?
>      >
>      > No, It is not working because it simplpy sets power_out to '0'. But to power on
>      > it should got down as 0 and come back to 1 after a delay. Which happens only
>      > in case of powerButtonMask set to true. I guess we may need to fix this.
>      
>      Ahh, okay.  I think I see the issue now.
>      
>      The issue is that because releasing the GPIO on a system with
>      pass-through, sets the power button back to the hardware default
>      automatically, the software setting doesn't matter, so it is left at 0.
>      
>      If you don't need to keep holding the GPIO line for POWER_OUT, I think
>      you can just add the code to revert the POWER_OUT value when the timer
>      expires.
>      
>      Take this line:
>                   // Set the masked GPIO line back to the opposite value
>                   maskedGPIOLine.set_value(!value);
>       From here:
>      https://github.com/openbmc/x86-power-control/blob/master/power-control-x86/src/power_control.cpp#L891
>      
>      And add it here:
>      https://github.com/openbmc/x86-power-control/blob/master/power-control-x86/src/power_control.cpp#L932
> 
> I already did that as a work around, testing different scenario. Will send patch once I see it working.
> 
> I also want to make these timeout values as configurable, either I can add these as a property in dbus interface or
> Entity-manager or have a separate config json file. What would you prefer.
Another option that may be simpler is to move the values that you want 
to configure out to a header file and then override the header in a 
bbappend.  Then you don't need to read or parse any additional 
configuration information at run time.

> 
>      
>      >
>      >
>      
> 

Re: x86-power-control

[Vijay Khemka]

On 10/22/19, 10:16 AM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:

    
    
    On 10/21/2019 6:15 PM, Vijay Khemka wrote:
    > 
    > 
    > On 10/21/19, 4:04 PM, "Bills, Jason M" <jason.m.bills@linux.intel.com> wrote:
    > 
    >      
    >      
    >      On 10/18/2019 4:04 PM, Vijay Khemka wrote:
    >      >
    >      >
    >      > On 10/18/19, 11:02 AM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
    >      >
    >      >
    >      >
    >      >      On 10/17/2019 4:52 PM, Vijay Khemka wrote:
    >      >      >
    >      >      >
    >      >      > On 10/17/19, 4:27 PM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
    >      >      >
    >      >      >
    >      >      >
    >      >      >      On 10/17/2019 3:32 PM, Vijay Khemka wrote:
    >      >      >      >
    >      >      >      > On 10/17/19, 9:03 AM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
    >      >      >      >
    >      >      >      >      Hi Vijay
    >      >      >      >
    >      >      >      >      On 10/16/2019 6:13 PM, Vijay Khemka wrote:
    >      >      >      >      > One more question on code, I see following code requires powerButtonMask
    >      >      >      >      > to be set before aquiring GPIO line. Please let me know who sets this
    >      >      >      >      > powerButtonMask to true. I know this is related to GPIO passthrough but
    >      >      >      >      > still couldn’t understand where in code it gets set until someone call
    >      >      >      >      > set-property of dbus.
    >      >      >      >
    >      >      >      >      powerButtonMask is a gpiod::line object that returns true if it
    >      >      >      >      references a GPIO line and false otherwise.
    >      >      >      >
    >      >      >      > I understood code but my point here is there will not be any
    >      >      >      > gpiod::line object if powerButtonMask is defined as false. And
    >      >      >      > initially it is defined as false means tehre will not be any line
    >      >      >      > object created until someone sets it to true. And I don't see it
    >      >      >      > any way to set it true other than set-property through dbus.
    >      >      >
    >      >      >      That's correct.  Masking the power button is something that is done by
    >      >      >      some component outside of power-control.
    >      >      >
    >      >      >      For example, we currently use it with the Set Front Panel Button Enables
    >      >      >      IPMI command to enable/disable the power button.  So, it is only masked
    >      >      >      when requested.
    >      >      > So to use x-86-power-control, either we need to have IPMI command to enable
    >      >      > this or some other daemon has to set this property. Can we have this feature
    >      >      > optional. I guess this is a prt og GPIO passthrough.
    >      >
    >      >      Yes, this is part of GPIO passthrough.  When the GPIO is requested,
    >      >      passthrough is disabled in the pinctrl driver.  So, to mask the power
    >      >      button (disable passthrough), power-control requests and holds the
    >      >      "POWER_OUT" GPIO line.
    >      >
    >      >      It should behave normally without this property ever getting set.
    >      >
    >      >      >
    >      >      >      The actual "POWER_OUT" line for power-control is not permanently
    >      >      >      created, but is asserted using temporary calls like this one:
    >      >      >      setGPIOOutputForMs("POWER_OUT", 0, powerPulseTimeMs);
    >      >      >
    >      >      > This function will not run power on/off sequence until button mask is set. It
    >      >      > Will only set GPIO value which is not enough for powering on/off.
    >      >
    >      >      Something else is going on, here.  The powerButtonMask is a separate
    >      >      feature from driving the "POWER_OUT" pin.  If powerButtonMask is not
    >      >      set, then the power on/off should function normally.  There is a special
    >      >      case in the setGPIOOutputForMs() code to handle when "POWER_OUT" is
    >      >      driven while powerButtonMask is true:
    >      >
    >      >           // If the requested GPIO is masked, use the mask line to set the output
    >      >           if (powerButtonMask && name == "POWER_OUT")
    >      >           {
    >      >               return setMaskedGPIOOutputForMs(powerButtonMask, name, value,
    >      >                                               durationMs);
    >      >           }
    >      >           ...
    >      >           // No mask set, so request and set the GPIO normally
    >      >
    >      >      So, "POWER_OUT" should work either way, but the default behavior is to
    >      >      function without powerButtonMask set.  Are you seeing failures on your
    >      >      platform when powerButtonMask is false?
    >      >
    >      > No, It is not working because it simplpy sets power_out to '0'. But to power on
    >      > it should got down as 0 and come back to 1 after a delay. Which happens only
    >      > in case of powerButtonMask set to true. I guess we may need to fix this.
    >      
    >      Ahh, okay.  I think I see the issue now.
    >      
    >      The issue is that because releasing the GPIO on a system with
    >      pass-through, sets the power button back to the hardware default
    >      automatically, the software setting doesn't matter, so it is left at 0.
    >      
    >      If you don't need to keep holding the GPIO line for POWER_OUT, I think
    >      you can just add the code to revert the POWER_OUT value when the timer
    >      expires.
    >      
    >      Take this line:
    >                   // Set the masked GPIO line back to the opposite value
    >                   maskedGPIOLine.set_value(!value);
    >       From here:
    >      https://github.com/openbmc/x86-power-control/blob/master/power-control-x86/src/power_control.cpp#L891
    >      
    >      And add it here:
    >      https://github.com/openbmc/x86-power-control/blob/master/power-control-x86/src/power_control.cpp#L932
    > 
    > I already did that as a work around, testing different scenario. Will send patch once I see it working.
    > 
    > I also want to make these timeout values as configurable, either I can add these as a property in dbus interface or
    > Entity-manager or have a separate config json file. What would you prefer.
    Another option that may be simpler is to move the values that you want 
    to configure out to a header file and then override the header in a 
    bbappend.  Then you don't need to read or parse any additional 
    configuration information at run time.
    
I can do that but bbappend for patch is not accepted in any repository.
    > 
    >      
    >      >
    >      >
    >      
    > 
    

Re: x86-power-control

[James Feist]

On 10/22/19 10:46 AM, Vijay Khemka wrote:
> 
> 
> On 10/22/19, 10:16 AM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
> 
>      
>      
>      On 10/21/2019 6:15 PM, Vijay Khemka wrote:
>      >
>      >
>      > On 10/21/19, 4:04 PM, "Bills, Jason M" <jason.m.bills@linux.intel.com> wrote:
>      >
>      >
>      >
>      >      On 10/18/2019 4:04 PM, Vijay Khemka wrote:
>      >      >
>      >      >
>      >      > On 10/18/19, 11:02 AM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
>      >      >
>      >      >
>      >      >
>      >      >      On 10/17/2019 4:52 PM, Vijay Khemka wrote:
>      >      >      >
>      >      >      >
>      >      >      > On 10/17/19, 4:27 PM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
>      >      >      >
>      >      >      >
>      >      >      >
>      >      >      >      On 10/17/2019 3:32 PM, Vijay Khemka wrote:
>      >      >      >      >
>      >      >      >      > On 10/17/19, 9:03 AM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:
>      >      >      >      >
>      >      >      >      >      Hi Vijay
>      >      >      >      >
>      >      >      >      >      On 10/16/2019 6:13 PM, Vijay Khemka wrote:
>      >      >      >      >      > One more question on code, I see following code requires powerButtonMask
>      >      >      >      >      > to be set before aquiring GPIO line. Please let me know who sets this
>      >      >      >      >      > powerButtonMask to true. I know this is related to GPIO passthrough but
>      >      >      >      >      > still couldn’t understand where in code it gets set until someone call
>      >      >      >      >      > set-property of dbus.
>      >      >      >      >
>      >      >      >      >      powerButtonMask is a gpiod::line object that returns true if it
>      >      >      >      >      references a GPIO line and false otherwise.
>      >      >      >      >
>      >      >      >      > I understood code but my point here is there will not be any
>      >      >      >      > gpiod::line object if powerButtonMask is defined as false. And
>      >      >      >      > initially it is defined as false means tehre will not be any line
>      >      >      >      > object created until someone sets it to true. And I don't see it
>      >      >      >      > any way to set it true other than set-property through dbus.
>      >      >      >
>      >      >      >      That's correct.  Masking the power button is something that is done by
>      >      >      >      some component outside of power-control.
>      >      >      >
>      >      >      >      For example, we currently use it with the Set Front Panel Button Enables
>      >      >      >      IPMI command to enable/disable the power button.  So, it is only masked
>      >      >      >      when requested.
>      >      >      > So to use x-86-power-control, either we need to have IPMI command to enable
>      >      >      > this or some other daemon has to set this property. Can we have this feature
>      >      >      > optional. I guess this is a prt og GPIO passthrough.
>      >      >
>      >      >      Yes, this is part of GPIO passthrough.  When the GPIO is requested,
>      >      >      passthrough is disabled in the pinctrl driver.  So, to mask the power
>      >      >      button (disable passthrough), power-control requests and holds the
>      >      >      "POWER_OUT" GPIO line.
>      >      >
>      >      >      It should behave normally without this property ever getting set.
>      >      >
>      >      >      >
>      >      >      >      The actual "POWER_OUT" line for power-control is not permanently
>      >      >      >      created, but is asserted using temporary calls like this one:
>      >      >      >      setGPIOOutputForMs("POWER_OUT", 0, powerPulseTimeMs);
>      >      >      >
>      >      >      > This function will not run power on/off sequence until button mask is set. It
>      >      >      > Will only set GPIO value which is not enough for powering on/off.
>      >      >
>      >      >      Something else is going on, here.  The powerButtonMask is a separate
>      >      >      feature from driving the "POWER_OUT" pin.  If powerButtonMask is not
>      >      >      set, then the power on/off should function normally.  There is a special
>      >      >      case in the setGPIOOutputForMs() code to handle when "POWER_OUT" is
>      >      >      driven while powerButtonMask is true:
>      >      >
>      >      >           // If the requested GPIO is masked, use the mask line to set the output
>      >      >           if (powerButtonMask && name == "POWER_OUT")
>      >      >           {
>      >      >               return setMaskedGPIOOutputForMs(powerButtonMask, name, value,
>      >      >                                               durationMs);
>      >      >           }
>      >      >           ...
>      >      >           // No mask set, so request and set the GPIO normally
>      >      >
>      >      >      So, "POWER_OUT" should work either way, but the default behavior is to
>      >      >      function without powerButtonMask set.  Are you seeing failures on your
>      >      >      platform when powerButtonMask is false?
>      >      >
>      >      > No, It is not working because it simplpy sets power_out to '0'. But to power on
>      >      > it should got down as 0 and come back to 1 after a delay. Which happens only
>      >      > in case of powerButtonMask set to true. I guess we may need to fix this.
>      >
>      >      Ahh, okay.  I think I see the issue now.
>      >
>      >      The issue is that because releasing the GPIO on a system with
>      >      pass-through, sets the power button back to the hardware default
>      >      automatically, the software setting doesn't matter, so it is left at 0.
>      >
>      >      If you don't need to keep holding the GPIO line for POWER_OUT, I think
>      >      you can just add the code to revert the POWER_OUT value when the timer
>      >      expires.
>      >
>      >      Take this line:
>      >                   // Set the masked GPIO line back to the opposite value
>      >                   maskedGPIOLine.set_value(!value);
>      >       From here:
>      >      https://github.com/openbmc/x86-power-control/blob/master/power-control-x86/src/power_control.cpp#L891
>      >
>      >      And add it here:
>      >      https://github.com/openbmc/x86-power-control/blob/master/power-control-x86/src/power_control.cpp#L932
>      >
>      > I already did that as a work around, testing different scenario. Will send patch once I see it working.
>      >
>      > I also want to make these timeout values as configurable, either I can add these as a property in dbus interface or
>      > Entity-manager or have a separate config json file. What would you prefer.
>      Another option that may be simpler is to move the values that you want
>      to configure out to a header file and then override the header in a
>      bbappend.  Then you don't need to read or parse any additional
>      configuration information at run time.
>      
> I can do that but bbappend for patch is not accepted in any repository.

Don't patch, simply copy over your new header into the correct directory 
in a do_configure_prepend (I think that's the right yocto step to 
overwrite, but I might be off).

>      >
>      >
>      >      >
>      >      >
>      >
>      >
>      
> 

Re: x86-power-control

[Johnathan Mantey]

[-- Attachment #1.1.1: Type: text/plain, Size: 597 bytes --]

You may want to delay the copy until the compile step.  For example:

PROJECT_SRC_DIR := "${THISDIR}/${PN}"
do_compile_prepend(){
    cp -f ${PROJECT_SRC_DIR}/your-header.hpp ${S}
}
> Don't patch, simply copy over your new header into the correct
> directory in a do_configure_prepend (I think that's the right yocto
> step to overwrite, but I might be off). --
Johnathan Mantey
Senior Software Engineer
*azad te**chnology partners*
Contributing to Technology Innovation since 1992
Phone: (503) 712-6764
Email: johnathanx.mantey@intel.com <mailto:johnathanx.mantey@intel.com>


[-- Attachment #1.1.2: Type: text/html, Size: 1560 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: x86-power-control

[Johnathan Mantey]

[-- Attachment #1.1.1: Type: text/plain, Size: 1527 bytes --]

One observation about this method.
It may not work for the way CI unit tests are presently behaving.

For phosphorr-networkd, I think, the unit tests insist that everything
being tested be Git committed. The copy of the source file will not be.
This will prevent the unit test from running. I found this requirement
by the unit tests to be an irritant. I would clang-format my source, and
commit.  The unit tests would do an automated reformat, causing my
commit to be useless.  I'd have to recommit the source one more time to
proceed.  It would be nice to eliminate/modify this requirement in the
unit tests.

On 10/22/19 11:04 AM, Johnathan Mantey wrote:
> You may want to delay the copy until the compile step.  For example:
>
> PROJECT_SRC_DIR := "${THISDIR}/${PN}"
> do_compile_prepend(){
>     cp -f ${PROJECT_SRC_DIR}/your-header.hpp ${S}
> }
>> Don't patch, simply copy over your new header into the correct
>> directory in a do_configure_prepend (I think that's the right yocto
>> step to overwrite, but I might be off). --
> Johnathan Mantey
> Senior Software Engineer
> *azad te**chnology partners*
> Contributing to Technology Innovation since 1992
> Phone: (503) 712-6764
> Email: johnathanx.mantey@intel.com <mailto:johnathanx.mantey@intel.com>
>

-- 
Johnathan Mantey
Senior Software Engineer
*azad te**chnology partners*
Contributing to Technology Innovation since 1992
Phone: (503) 712-6764
Email: johnathanx.mantey@intel.com <mailto:johnathanx.mantey@intel.com>


[-- Attachment #1.1.2: Type: text/html, Size: 3510 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: x86-power-control

[James Feist]

On 10/22/19 12:06 PM, Johnathan Mantey wrote:
> One observation about this method.
> It may not work for the way CI unit tests are presently behaving.
> 
> For phosphorr-networkd, I think, the unit tests insist that everything 
> being tested be Git committed. The copy of the source file will not be. 
> This will prevent the unit test from running. I found this requirement 
> by the unit tests to be an irritant. I would clang-format my source, and 
> commit.  The unit tests would do an automated reformat, causing my 
> commit to be useless.  I'd have to recommit the source one more time to 
> proceed.  It would be nice to eliminate/modify this requirement in the 
> unit tests.

It should be fine in this case because CI would only be running against 
the "default" header file, the modified one would not get introduced in 
the CI path.

> 
> On 10/22/19 11:04 AM, Johnathan Mantey wrote:
>> You may want to delay the copy until the compile step.  For example:
>>
>> PROJECT_SRC_DIR := "${THISDIR}/${PN}"
>> do_compile_prepend(){
>>     cp -f ${PROJECT_SRC_DIR}/your-header.hpp ${S}
>> }
>>> Don't patch, simply copy over your new header into the correct 
>>> directory in a do_configure_prepend (I think that's the right yocto 
>>> step to overwrite, but I might be off). --
>> Johnathan Mantey
>> Senior Software Engineer
>> *azad te**chnology partners*
>> Contributing to Technology Innovation since 1992
>> Phone: (503) 712-6764
>> Email: johnathanx.mantey@intel.com <mailto:johnathanx.mantey@intel.com>
>>
> 
> -- 
> Johnathan Mantey
> Senior Software Engineer
> *azad te**chnology partners*
> Contributing to Technology Innovation since 1992
> Phone: (503) 712-6764
> Email: johnathanx.mantey@intel.com <mailto:johnathanx.mantey@intel.com>
> 

Re: x86-power-control

[Vijay Khemka]

[-- Attachment #1: Type: text/plain, Size: 932 bytes --]

Thanks James, Bill and John.

From: Johnathan Mantey <johnathanx.mantey@intel.com>
Date: Tuesday, October 22, 2019 at 11:05 AM
To: James Feist <james.feist@linux.intel.com>, Vijay Khemka <vijaykhemka@fb.com>, "Bills, Jason M" <jason.m.bills@linux.intel.com>, "openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>
Subject: Re: x86-power-control

You may want to delay the copy until the compile step.  For example:

PROJECT_SRC_DIR := "${THISDIR}/${PN}"
do_compile_prepend(){
    cp -f ${PROJECT_SRC_DIR}/your-header.hpp ${S}
}

Don't patch, simply copy over your new header into the correct directory in a do_configure_prepend (I think that's the right yocto step to overwrite, but I might be off). --
Johnathan Mantey
Senior Software Engineer
azad technology partners
Contributing to Technology Innovation since 1992
Phone: (503) 712-6764
Email: johnathanx.mantey@intel.com<mailto:johnathanx.mantey@intel.com>

[-- Attachment #2: Type: text/html, Size: 4181 bytes --]

Re: x86-power-control

[Vijay Khemka]

On 11/4/19, 11:53 AM, "openbmc on behalf of Bills, Jason M" <openbmc-bounces+vijaykhemka=fb.com@lists.ozlabs.org on behalf of jason.m.bills@linux.intel.com> wrote:

    Hi Vijay,
    
    On 11/1/2019 5:33 PM, Vijay Khemka wrote:
    > Hi Jason/James,
    > I see some limitation in current x86-power-control as we don’t have NMI_OUT, NMI_BUTTON and ID_BUTTON usage. And I am not sure why these are being used. So if I don’t define these in DTS then this program fails. So Please how to disable these. These should be optional. I have following options to disable these.
    > 
    > 1. No returning -1 if we don't find line name, simply move on. I have to see impact on rest of code.
    > 2. Make it compile time flag and should be included through bbappend like -DNMI_OUT etc.
    I chatted with James and I think we like this option the best.  If you 
    set a build flag that is enabled by default, it will let you disable the 
    features you don't need in a .bbappend.  This will let you remove the 
    pins you don't use and still allow for easier detection when an expected 
    pin isn't working.

Thanks Jason, I will work on patch and send it for review.
    
    Thanks,
    -Jason
    
    > 3. Have config Jason file and enable or disable gpio line to be used.
    > 
    > 
    > Please let us know your thought and how should we approach. I am fine with changing this code and submit patch.
    > 
    > Regards
    > -Vijay
    > 
    

Re: x86-power-control

[Bills, Jason M]

Hi Vijay,

On 11/1/2019 5:33 PM, Vijay Khemka wrote:
> Hi Jason/James,
> I see some limitation in current x86-power-control as we don’t have NMI_OUT, NMI_BUTTON and ID_BUTTON usage. And I am not sure why these are being used. So if I don’t define these in DTS then this program fails. So Please how to disable these. These should be optional. I have following options to disable these.
> 
> 1. No returning -1 if we don't find line name, simply move on. I have to see impact on rest of code.
> 2. Make it compile time flag and should be included through bbappend like -DNMI_OUT etc.
I chatted with James and I think we like this option the best.  If you 
set a build flag that is enabled by default, it will let you disable the 
features you don't need in a .bbappend.  This will let you remove the 
pins you don't use and still allow for easier detection when an expected 
pin isn't working.

Thanks,
-Jason

> 3. Have config Jason file and enable or disable gpio line to be used.
> 
> 
> Please let us know your thought and how should we approach. I am fine with changing this code and submit patch.
> 
> Regards
> -Vijay
> 

x86-power-control

[Vijay Khemka]

Hi Jason/James,
I see some limitation in current x86-power-control as we don’t have NMI_OUT, NMI_BUTTON and ID_BUTTON usage. And I am not sure why these are being used. So if I don’t define these in DTS then this program fails. So Please how to disable these. These should be optional. I have following options to disable these.

1. No returning -1 if we don't find line name, simply move on. I have to see impact on rest of code.
2. Make it compile time flag and should be included through bbappend like -DNMI_OUT etc.
3. Have config Jason file and enable or disable gpio line to be used.


Please let us know your thought and how should we approach. I am fine with changing this code and submit patch.

Regards
-Vijay