Re: x86-power-control

[Vijay Khemka <vijaykhemka@fb.com>]

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.
    > 
    >      
    >      >
    >      >
    >      
    >