<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Nov 8, 2016 at 1:13 PM, Mine <span dir="ltr"><<a href="mailto:mine260309@gmail.com" target="_blank">mine260309@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">On Mon, Nov 7, 2016 at 3:53 PM, Xo Wang <<a href="mailto:xow@google.com">xow@google.com</a>> wrote:<br>
> On Mon, Nov 7, 2016 at 9:01 AM, Mine <<a href="mailto:mine260309@gmail.com">mine260309@gmail.com</a>> wrote:<br>
>> On Mon, Nov 7, 2016 at 10:19 AM, Patrick Williams <<a href="mailto:patrick@stwcx.xyz">patrick@stwcx.xyz</a>> wrote:<br>
>>> On Mon, Nov 07, 2016 at 05:10:59PM +0800, Yi TZ Li wrote:<br>
>>>> I committed a patch: <a href="https://gerrit.openbmc-project.xyz/#/c/1019/" rel="noreferrer" target="_blank">https://gerrit.openbmc-<wbr>project.xyz/#/c/1019/</a><br>
>>>> to add a getSystemName() dbus method in org.obmc.managers.System interface.<br>
>>>> There are several places in skeleton requires fix for a specific system.<br>
>>>><br>
>>>> Other way be add a compile flag in yocto when building Skeleton.<br>
>>><br>
>>> We are really trying to avoid "if (machine_type == ...)" type code.  This<br>
>>> becomes a gigantic problem when we are talking about supporting 3 dozen<br>
>>> machines.<br>
>>><br>
>>> --<br>
>>> Patrick Williams<br>
>><br>
>> Yup, I prefer not to write `if (machine_type == ...)` type code, nor<br>
>> to have a compile flag (it just becomes `#if xxx` or `#ifdef xxx`).<br>
>><br>
>> The temp fix by not defining `IDBTN` will not only cause error log,<br>
>> but also prevent the code in `op-hostctl/control_host_obj.<wbr>c` to work,<br>
>> because it will get error during opening the GPIO, and jump out,<br>
>> resulting in error: "GPIO sequence failed".<br>
>><br>
>> If we are targeting for booting P9 up, I would suggest a temp fix by<br>
>> defining `IDBTN` for both Witherspoon and Romulus.<br>
>> It will not block power on, but may break a function related to LED.<br>
>><br>
>> --<br>
>> BRs,<br>
>> Lei YU<br>
>> ______________________________<wbr>_________________<br>
>> openbmc mailing list<br>
>> <a href="mailto:openbmc@lists.ozlabs.org">openbmc@lists.ozlabs.org</a><br>
>> <a href="https://lists.ozlabs.org/listinfo/openbmc" rel="noreferrer" target="_blank">https://lists.ozlabs.org/<wbr>listinfo/openbmc</a><br>
><br>
> We had a similar issue in the op-pwrctl where the source hard-coded<br>
> the system net names. There I put in a short-term solution of adding a<br>
> function-specific abstraction over the net names into the System.py<br>
> config, removing the per-machine patches for name and polarity:<br>
> <a href="https://gerrit.openbmc-project.xyz/#/c/606/" rel="noreferrer" target="_blank">https://gerrit.openbmc-<wbr>project.xyz/#/c/606/</a><br>
> <a href="https://gerrit.openbmc-project.xyz/#/c/689/" rel="noreferrer" target="_blank">https://gerrit.openbmc-<wbr>project.xyz/#/c/689/</a><br>
><br>
> It's not difficult to extend POWER_CONFIG for GPIO functions that only<br>
> exist on one machine, like so:<br>
> <a href="https://gerrit.openbmc-project.xyz/#/c/608/3" rel="noreferrer" target="_blank">https://gerrit.openbmc-<wbr>project.xyz/#/c/608/3</a><br>
><br>
> With the abstraction taking place in the Python machine config, we<br>
> don't need per-machine dispatch in the user interface (op-pwrctl,<br>
> op-hostctl, op-pwr-button, etc).<br>
><br>
> Does that work for you in the short term?<br>
><br>
> cheers<br>
> xo<br>
<br>
</div></div>Yup, that could be a solution. It's just the GPIOs are not for power control,<br>
so following the solution would probably introduce similar code to<br>
hostctl_gpio.h/c.<br>
<br>
Maybe I can combine the power_gpio and hostctl_gpio, so they share the same<br>
code?<br>
<br>
Something like:<br>
<br>
GPIO_CONFIG = {<br>
  'power_gpios': {<br>
    'latch_out' : 'BMC_UCD_LATCH_LE',<br>
    'power_good_in' : 'SYS_PWROK_BUFF',<br>
    'power_up_outs' : [<br>
        ('SOFTWARE_PGOOD', True),<br>
        ('BMC_POWER_UP', True),<br>
    ],<br>
    'reset_outs' : [<br>
    ],<br>
  },<br>
  'hostctl_gpios': {<br>
    'fsi_data' : 'FSI_DATA',<br>
    ...<br>
    'optionals' : [<br>
      ('BMC_THROTTLE', True),<br>
      ('IDBTN', False),<br>
    ],<br>
  }<br>
}<br>
<br>
What do you think?<br>
<br>
--<br>
BRs,<br>
Lei YU<br></blockquote><div><br></div><div>Yes, that sounds reasonable to me. Maybe change the name of the PowerGpio stuff to reflect your more general approach.</div><div><br></div><div>FYI something that I didn't implement was adding GPIO polarity support into libopenbmc_intf/gpio. So, all the polarity (active high/low) logic needs to be handled by the application, which is less than ideal.</div><div><br></div><div>cheers</div><div>xo</div></div><br></div></div>