How to handle GPIO differences between P8 and P9

How to handle GPIO differences between P8 and P9

[Mine]

Hi All,

This mail is to discuss how to handle GPIO differences (e.g.
functionality, IN/OUT) between different HWs.

I went into a case the P8 uses two GPIOs, but P9 does not use them: in
skeleton/op-hostctl/control_host_obj.c:

GPIO Throttle = (GPIO){ "BMC_THROTTLE" };
GPIO idbtn = (GPIO){ "IDBTN" };
...
rc |= gpio_write(&Throttle,1);
rc |= gpio_write(&idbtn,0);

In P9, BMC_THROTTLE is renamed to N_MODE_N, and changed to an IN GPIO;
IDBTN is changed to IN GPIO as well.
If we keep the above code and overriding the GPIOs, writing
BMC_THROTTLE takes no effect, but writing to IDBTN may break the
function of its related front panel button.

So I think we need to distinguish such cases to avoid issues.

The question is, how to change the code to support different HWs that
have unique GPIOs?

Thanks!

—
BRs,
Lei YU

Re: How to handle GPIO differences between P8 and P9

[Patrick Williams]

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

On Fri, Nov 04, 2016 at 11:45:02AM -0500, Mine wrote:
> Hi All,
> 
> This mail is to discuss how to handle GPIO differences (e.g.
> functionality, IN/OUT) between different HWs.
> 
> I went into a case the P8 uses two GPIOs, but P9 does not use them: in
> skeleton/op-hostctl/control_host_obj.c:
> 
> GPIO Throttle = (GPIO){ "BMC_THROTTLE" };
> GPIO idbtn = (GPIO){ "IDBTN" };
> ...
> rc |= gpio_write(&Throttle,1);
> rc |= gpio_write(&idbtn,0);
> 
> In P9, BMC_THROTTLE is renamed to N_MODE_N, and changed to an IN GPIO;
> IDBTN is changed to IN GPIO as well.
> If we keep the above code and overriding the GPIOs, writing
> BMC_THROTTLE takes no effect, but writing to IDBTN may break the
> function of its related front panel button.
> 
> So I think we need to distinguish such cases to avoid issues.
> 
> The question is, how to change the code to support different HWs that
> have unique GPIOs?

The current GPIO names are "Barreleye-Centric" in that they are named
based on names that were on the Barreleye board.  We need to move the
GPIO structure to be "Function-Centric" instead so that it is more
generally applicable.

That is something that will probably happen over the next few months.
Joel is working on a proposal and we'll schedule some of the refactoring
work afterwards in the userspace code.

We might go the route of having dbus objects for GPIOs instead of having
direct access, similar to what I believe the current proposal is for
LEDs.  We'll have to see what Joel proposes first and then discuss it.

I would suggest for now we simply do the minimum we need to to "make P9
work".  If this means using GPIO names that continue to match Barreleye
naming schemes, rather than Witherspoon / Romulus, I think that makes
sense.  You can always keep a comment in the System.py file that names
the "real board" name.

In cases where they have different functionality, doesn't using a
different name solve that?  Having a different name means the lookup to
"BMC_THROTTLE" or "IDBTN" will fail on Witherspoon and thus this code
will no longer function, right?

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: How to handle GPIO differences between P8 and P9

[Yi TZ Li]

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

I committed a patch: https://gerrit.openbmc-project.xyz/#/c/1019/
to add a getSystemName() dbus method in org.obmc.managers.System interface.
There are several places in skeleton requires fix for a specific system.

Other way be add a compile flag in yocto when building Skeleton.

> From: Patrick Williams <patrick@stwcx.xyz>
> To: Mine <mine260309@gmail.com>
> Cc: OpenBMC Maillist <openbmc@lists.ozlabs.org>
> Date: 11/07/2016 12:07 PM
> Subject: Re: How to handle GPIO differences between P8 and P9
> Sent by: "openbmc" <openbmc-bounces+shliyi=cn.ibm.com@lists.ozlabs.org>
>
> On Fri, Nov 04, 2016 at 11:45:02AM -0500, Mine wrote:
> > Hi All,
> >
> > This mail is to discuss how to handle GPIO differences (e.g.
> > functionality, IN/OUT) between different HWs.
> >
> > I went into a case the P8 uses two GPIOs, but P9 does not use them: in
> > skeleton/op-hostctl/control_host_obj.c:
> >
> > GPIO Throttle = (GPIO){ "BMC_THROTTLE" };
> > GPIO idbtn = (GPIO){ "IDBTN" };
> > ...
> > rc |= gpio_write(&Throttle,1);
> > rc |= gpio_write(&idbtn,0);
> >
> > In P9, BMC_THROTTLE is renamed to N_MODE_N, and changed to an IN GPIO;
> > IDBTN is changed to IN GPIO as well.
> > If we keep the above code and overriding the GPIOs, writing
> > BMC_THROTTLE takes no effect, but writing to IDBTN may break the
> > function of its related front panel button.
> >
> > So I think we need to distinguish such cases to avoid issues.
> >
> > The question is, how to change the code to support different HWs that
> > have unique GPIOs?
>
> The current GPIO names are "Barreleye-Centric" in that they are named
> based on names that were on the Barreleye board.  We need to move the
> GPIO structure to be "Function-Centric" instead so that it is more
> generally applicable.
>
> That is something that will probably happen over the next few months.
> Joel is working on a proposal and we'll schedule some of the refactoring
> work afterwards in the userspace code.
>
> We might go the route of having dbus objects for GPIOs instead of having
> direct access, similar to what I believe the current proposal is for
> LEDs.  We'll have to see what Joel proposes first and then discuss it.
>
> I would suggest for now we simply do the minimum we need to to "make P9
> work".  If this means using GPIO names that continue to match Barreleye
> naming schemes, rather than Witherspoon / Romulus, I think that makes
> sense.  You can always keep a comment in the System.py file that names
> the "real board" name.
>
> In cases where they have different functionality, doesn't using a
> different name solve that?  Having a different name means the lookup to
> "BMC_THROTTLE" or "IDBTN" will fail on Witherspoon and thus this code
> will no longer function, right?

For a temp fix, the simplest way is not to define IDBTN in Romulus.py.
This will cause an error message reported - but does not hurt.

Thanks,
-Yi

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

Re: How to handle GPIO differences between P8 and P9

[Patrick Williams]

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

On Mon, Nov 07, 2016 at 05:10:59PM +0800, Yi TZ Li wrote:
> I committed a patch: https://gerrit.openbmc-project.xyz/#/c/1019/
> to add a getSystemName() dbus method in org.obmc.managers.System interface.
> There are several places in skeleton requires fix for a specific system.
> 
> Other way be add a compile flag in yocto when building Skeleton.

We are really trying to avoid "if (machine_type == ...)" type code.  This
becomes a gigantic problem when we are talking about supporting 3 dozen
machines.

-- 
Patrick Williams

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: How to handle GPIO differences between P8 and P9

[Mine]

On Mon, Nov 7, 2016 at 10:19 AM, Patrick Williams <patrick@stwcx.xyz> wrote:
> On Mon, Nov 07, 2016 at 05:10:59PM +0800, Yi TZ Li wrote:
>> I committed a patch: https://gerrit.openbmc-project.xyz/#/c/1019/
>> to add a getSystemName() dbus method in org.obmc.managers.System interface.
>> There are several places in skeleton requires fix for a specific system.
>>
>> Other way be add a compile flag in yocto when building Skeleton.
>
> We are really trying to avoid "if (machine_type == ...)" type code.  This
> becomes a gigantic problem when we are talking about supporting 3 dozen
> machines.
>
> --
> Patrick Williams

Yup, I prefer not to write `if (machine_type == ...)` type code, nor
to have a compile flag (it just becomes `#if xxx` or `#ifdef xxx`).

The temp fix by not defining `IDBTN` will not only cause error log,
but also prevent the code in `op-hostctl/control_host_obj.c` to work,
because it will get error during opening the GPIO, and jump out,
resulting in error: "GPIO sequence failed".

If we are targeting for booting P9 up, I would suggest a temp fix by
defining `IDBTN` for both Witherspoon and Romulus.
It will not block power on, but may break a function related to LED.

--
BRs,
Lei YU

Re: How to handle GPIO differences between P8 and P9

[Xo Wang]

On Mon, Nov 7, 2016 at 9:01 AM, Mine <mine260309@gmail.com> wrote:
> On Mon, Nov 7, 2016 at 10:19 AM, Patrick Williams <patrick@stwcx.xyz> wrote:
>> On Mon, Nov 07, 2016 at 05:10:59PM +0800, Yi TZ Li wrote:
>>> I committed a patch: https://gerrit.openbmc-project.xyz/#/c/1019/
>>> to add a getSystemName() dbus method in org.obmc.managers.System interface.
>>> There are several places in skeleton requires fix for a specific system.
>>>
>>> Other way be add a compile flag in yocto when building Skeleton.
>>
>> We are really trying to avoid "if (machine_type == ...)" type code.  This
>> becomes a gigantic problem when we are talking about supporting 3 dozen
>> machines.
>>
>> --
>> Patrick Williams
>
> Yup, I prefer not to write `if (machine_type == ...)` type code, nor
> to have a compile flag (it just becomes `#if xxx` or `#ifdef xxx`).
>
> The temp fix by not defining `IDBTN` will not only cause error log,
> but also prevent the code in `op-hostctl/control_host_obj.c` to work,
> because it will get error during opening the GPIO, and jump out,
> resulting in error: "GPIO sequence failed".
>
> If we are targeting for booting P9 up, I would suggest a temp fix by
> defining `IDBTN` for both Witherspoon and Romulus.
> It will not block power on, but may break a function related to LED.
>
> --
> BRs,
> Lei YU
> _______________________________________________
> openbmc mailing list
> openbmc@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/openbmc

We had a similar issue in the op-pwrctl where the source hard-coded
the system net names. There I put in a short-term solution of adding a
function-specific abstraction over the net names into the System.py
config, removing the per-machine patches for name and polarity:
https://gerrit.openbmc-project.xyz/#/c/606/
https://gerrit.openbmc-project.xyz/#/c/689/

It's not difficult to extend POWER_CONFIG for GPIO functions that only
exist on one machine, like so:
https://gerrit.openbmc-project.xyz/#/c/608/3

With the abstraction taking place in the Python machine config, we
don't need per-machine dispatch in the user interface (op-pwrctl,
op-hostctl, op-pwr-button, etc).

Does that work for you in the short term?

cheers
xo

Re: How to handle GPIO differences between P8 and P9

[Mine]

On Mon, Nov 7, 2016 at 3:53 PM, Xo Wang <xow@google.com> wrote:
> On Mon, Nov 7, 2016 at 9:01 AM, Mine <mine260309@gmail.com> wrote:
>> On Mon, Nov 7, 2016 at 10:19 AM, Patrick Williams <patrick@stwcx.xyz> wrote:
>>> On Mon, Nov 07, 2016 at 05:10:59PM +0800, Yi TZ Li wrote:
>>>> I committed a patch: https://gerrit.openbmc-project.xyz/#/c/1019/
>>>> to add a getSystemName() dbus method in org.obmc.managers.System interface.
>>>> There are several places in skeleton requires fix for a specific system.
>>>>
>>>> Other way be add a compile flag in yocto when building Skeleton.
>>>
>>> We are really trying to avoid "if (machine_type == ...)" type code.  This
>>> becomes a gigantic problem when we are talking about supporting 3 dozen
>>> machines.
>>>
>>> --
>>> Patrick Williams
>>
>> Yup, I prefer not to write `if (machine_type == ...)` type code, nor
>> to have a compile flag (it just becomes `#if xxx` or `#ifdef xxx`).
>>
>> The temp fix by not defining `IDBTN` will not only cause error log,
>> but also prevent the code in `op-hostctl/control_host_obj.c` to work,
>> because it will get error during opening the GPIO, and jump out,
>> resulting in error: "GPIO sequence failed".
>>
>> If we are targeting for booting P9 up, I would suggest a temp fix by
>> defining `IDBTN` for both Witherspoon and Romulus.
>> It will not block power on, but may break a function related to LED.
>>
>> --
>> BRs,
>> Lei YU
>> _______________________________________________
>> openbmc mailing list
>> openbmc@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/openbmc
>
> We had a similar issue in the op-pwrctl where the source hard-coded
> the system net names. There I put in a short-term solution of adding a
> function-specific abstraction over the net names into the System.py
> config, removing the per-machine patches for name and polarity:
> https://gerrit.openbmc-project.xyz/#/c/606/
> https://gerrit.openbmc-project.xyz/#/c/689/
>
> It's not difficult to extend POWER_CONFIG for GPIO functions that only
> exist on one machine, like so:
> https://gerrit.openbmc-project.xyz/#/c/608/3
>
> With the abstraction taking place in the Python machine config, we
> don't need per-machine dispatch in the user interface (op-pwrctl,
> op-hostctl, op-pwr-button, etc).
>
> Does that work for you in the short term?
>
> cheers
> xo

Yup, that could be a solution. It's just the GPIOs are not for power control,
so following the solution would probably introduce similar code to
hostctl_gpio.h/c.

Maybe I can combine the power_gpio and hostctl_gpio, so they share the same
code?

Something like:

GPIO_CONFIG = {
  'power_gpios': {
    'latch_out' : 'BMC_UCD_LATCH_LE',
    'power_good_in' : 'SYS_PWROK_BUFF',
    'power_up_outs' : [
        ('SOFTWARE_PGOOD', True),
        ('BMC_POWER_UP', True),
    ],
    'reset_outs' : [
    ],
  },
  'hostctl_gpios': {
    'fsi_data' : 'FSI_DATA',
    ...
    'optionals' : [
      ('BMC_THROTTLE', True),
      ('IDBTN', False),
    ],
  }
}

What do you think?

--
BRs,
Lei YU

Re: How to handle GPIO differences between P8 and P9

[Xo Wang]

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

On Tue, Nov 8, 2016 at 1:13 PM, Mine <mine260309@gmail.com> wrote:

> On Mon, Nov 7, 2016 at 3:53 PM, Xo Wang <xow@google.com> wrote:
> > On Mon, Nov 7, 2016 at 9:01 AM, Mine <mine260309@gmail.com> wrote:
> >> On Mon, Nov 7, 2016 at 10:19 AM, Patrick Williams <patrick@stwcx.xyz>
> wrote:
> >>> On Mon, Nov 07, 2016 at 05:10:59PM +0800, Yi TZ Li wrote:
> >>>> I committed a patch: https://gerrit.openbmc-project.xyz/#/c/1019/
> >>>> to add a getSystemName() dbus method in org.obmc.managers.System
> interface.
> >>>> There are several places in skeleton requires fix for a specific
> system.
> >>>>
> >>>> Other way be add a compile flag in yocto when building Skeleton.
> >>>
> >>> We are really trying to avoid "if (machine_type == ...)" type code.
> This
> >>> becomes a gigantic problem when we are talking about supporting 3 dozen
> >>> machines.
> >>>
> >>> --
> >>> Patrick Williams
> >>
> >> Yup, I prefer not to write `if (machine_type == ...)` type code, nor
> >> to have a compile flag (it just becomes `#if xxx` or `#ifdef xxx`).
> >>
> >> The temp fix by not defining `IDBTN` will not only cause error log,
> >> but also prevent the code in `op-hostctl/control_host_obj.c` to work,
> >> because it will get error during opening the GPIO, and jump out,
> >> resulting in error: "GPIO sequence failed".
> >>
> >> If we are targeting for booting P9 up, I would suggest a temp fix by
> >> defining `IDBTN` for both Witherspoon and Romulus.
> >> It will not block power on, but may break a function related to LED.
> >>
> >> --
> >> BRs,
> >> Lei YU
> >> _______________________________________________
> >> openbmc mailing list
> >> openbmc@lists.ozlabs.org
> >> https://lists.ozlabs.org/listinfo/openbmc
> >
> > We had a similar issue in the op-pwrctl where the source hard-coded
> > the system net names. There I put in a short-term solution of adding a
> > function-specific abstraction over the net names into the System.py
> > config, removing the per-machine patches for name and polarity:
> > https://gerrit.openbmc-project.xyz/#/c/606/
> > https://gerrit.openbmc-project.xyz/#/c/689/
> >
> > It's not difficult to extend POWER_CONFIG for GPIO functions that only
> > exist on one machine, like so:
> > https://gerrit.openbmc-project.xyz/#/c/608/3
> >
> > With the abstraction taking place in the Python machine config, we
> > don't need per-machine dispatch in the user interface (op-pwrctl,
> > op-hostctl, op-pwr-button, etc).
> >
> > Does that work for you in the short term?
> >
> > cheers
> > xo
>
> Yup, that could be a solution. It's just the GPIOs are not for power
> control,
> so following the solution would probably introduce similar code to
> hostctl_gpio.h/c.
>
> Maybe I can combine the power_gpio and hostctl_gpio, so they share the same
> code?
>
> Something like:
>
> GPIO_CONFIG = {
>   'power_gpios': {
>     'latch_out' : 'BMC_UCD_LATCH_LE',
>     'power_good_in' : 'SYS_PWROK_BUFF',
>     'power_up_outs' : [
>         ('SOFTWARE_PGOOD', True),
>         ('BMC_POWER_UP', True),
>     ],
>     'reset_outs' : [
>     ],
>   },
>   'hostctl_gpios': {
>     'fsi_data' : 'FSI_DATA',
>     ...
>     'optionals' : [
>       ('BMC_THROTTLE', True),
>       ('IDBTN', False),
>     ],
>   }
> }
>
> What do you think?
>
> --
> BRs,
> Lei YU
>

Yes, that sounds reasonable to me. Maybe change the name of the PowerGpio
stuff to reflect your more general approach.

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.

cheers
xo

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

Re: How to handle GPIO differences between P8 and P9

[Andrew Jeffery]

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

On Tue, 2016-11-08 at 15:38 -0800, Xo Wang wrote:
> 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.

Ultimately this should be handled by the kernel via devicetree and
pinconf. I'd hold off on fixing the application because of that, but
there's also no timeline for implementing pinconf support in pinctrl.

Andrew

> 
> cheers
> xo

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

How to handle GPIO differences between P8 and P9

[Lei KM Yu]

[-- Attachment #1: Type: text/html, Size: 7885 bytes --]