Re: x86-power-control

[Vijay Khemka <vijaykhemka@fb.com>]

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.

    
    >      
    >