[PATCH v3 24/35] acpi: Add support for writing a GPIO power sequence

[Bin Meng <bmeng.cn@gmail.com>]

Hi Simon,

On Sun, Jun 14, 2020 at 10:55 AM Simon Glass <sjg@chromium.org> wrote:
>
> Power to some devices is controlled by GPIOs. Add a way to generate ACPI
> code to enable and disable a GPIO so that this can be handled within an
> ACPI method.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> Reviewed-by: Wolfgang Wallner <wolfgang.wallner@br-automation.com>
> ---
>
> (no changes since v1)
>
>  include/acpi/acpigen.h | 12 +++++++
>  lib/acpi/acpigen.c     | 80 ++++++++++++++++++++++++++++++++++++++++++
>  test/dm/acpigen.c      | 54 ++++++++++++++++++++++++++++
>  3 files changed, 146 insertions(+)
>
> diff --git a/include/acpi/acpigen.h b/include/acpi/acpigen.h
> index d7a7f2f538..d68dca5dde 100644
> --- a/include/acpi/acpigen.h
> +++ b/include/acpi/acpigen.h
> @@ -13,6 +13,7 @@
>  #include <linux/types.h>
>
>  struct acpi_ctx;
> +struct acpi_gpio;
>
>  /* Top 4 bits of the value used to indicate a three-byte length value */
>  #define ACPI_PKG_LEN_3_BYTES   0x80
> @@ -340,4 +341,15 @@ void acpigen_write_power_res(struct acpi_ctx *ctx, const char *name, uint level,
>                              uint order, const char *const dev_states[],
>                              size_t dev_states_count);
>
> +/*
> + * Helper functions for enabling/disabling Tx GPIOs based on the GPIO
> + * polarity. These functions end up calling acpigen_soc_{set,clear}_tx_gpio to
> + * make callbacks into SoC acpigen code.
> + *
> + * Returns 0 on success and -1 on error.
> + */
> +int acpigen_set_enable_tx_gpio(struct acpi_ctx *ctx, u32 tx_state_val,
> +                              const char *dw0_name, struct acpi_gpio *gpio,

What is dw0? Is dw0 generic stuff or GPIO specific? Need a comment
block for parameter descriptions.

> +                              bool enable);
> +
>  #endif
> diff --git a/lib/acpi/acpigen.c b/lib/acpi/acpigen.c
> index 5df6ebe334..e0af36f775 100644
> --- a/lib/acpi/acpigen.c
> +++ b/lib/acpi/acpigen.c
> @@ -13,6 +13,7 @@
>  #include <log.h>
>  #include <uuid.h>
>  #include <acpi/acpigen.h>
> +#include <acpi/acpi_device.h>
>  #include <dm/acpi.h>
>
>  u8 *acpigen_get_current(struct acpi_ctx *ctx)
> @@ -392,3 +393,82 @@ void acpigen_write_debug_string(struct acpi_ctx *ctx, const char *str)
>         acpigen_write_string(ctx, str);
>         acpigen_emit_ext_op(ctx, DEBUG_OP);
>  }
> +
> +/**
> + * acpigen_get_dw0_in_local5() - Generate code to put dw0 cfg0 in local5

Could you please explain why we put dw0 to local5? How about other
local objects?

> + *
> + * @ctx: ACPI context pointer
> + * @dw0_name: Name to use (e.g. "\\_SB.GPC0")
> + * @addr: GPIO pin configuration register address
> + *
> + * Store (\_SB.GPC0 (addr), Local5)
> + * \_SB.GPC0 is used to read cfg0 value from dw0. It is typically defined in
> + * the board's gpiolib.asl
> + */
> +static void acpigen_get_dw0_in_local5(struct acpi_ctx *ctx,
> +                                     const char *dw0_name, ulong addr)
> +{
> +       acpigen_write_store(ctx);
> +       acpigen_emit_namestring(ctx, dw0_name);
> +       acpigen_write_integer(ctx, addr);
> +       acpigen_emit_byte(ctx, LOCAL5_OP);
> +}
> +
> +/**
> + * acpigen_set_gpio_val() - Set value of TX GPIO to on/off
> + *
> + * @ctx: ACPI context pointer
> + * @dw0_name: Name to use (e.g. "\\_SB.GPC0")
> + * @gpio_num: GPIO number to adjust
> + * @vaL: true to set on, false to set off
> + */
> +static int acpigen_set_gpio_val(struct acpi_ctx *ctx, u32 tx_state_val,
> +                               const char *dw0_name, struct acpi_gpio *gpio,
> +                               bool val)
> +{
> +       acpigen_get_dw0_in_local5(ctx, dw0_name, gpio->pin0_addr);
> +
> +       if (val) {
> +               /* Or (Local5, PAD_CFG0_TX_STATE, Local5) */
> +               acpigen_write_or(ctx, LOCAL5_OP, tx_state_val, LOCAL5_OP);
> +       } else {
> +               /* Not (PAD_CFG0_TX_STATE, Local6) */
> +               acpigen_write_not(ctx, tx_state_val, LOCAL6_OP);
> +
> +               /* And (Local5, Local6, Local5) */
> +               acpigen_write_and(ctx, LOCAL5_OP, LOCAL6_OP, LOCAL5_OP);
> +       }
> +
> +       /*
> +        * \_SB.SPC0 (addr, Local5)
> +        * \_SB.SPC0 is used to write cfg0 value in dw0. It is defined in
> +        * gpiolib.asl.
> +        */
> +       acpigen_emit_namestring(ctx, dw0_name);
> +       acpigen_write_integer(ctx, gpio->pin0_addr);
> +       acpigen_emit_byte(ctx, LOCAL5_OP);
> +
> +       return 0;
> +}
> +
> +/*
> + * Helper functions for enabling/disabling Tx GPIOs based on the GPIO
> + * polarity. These functions end up calling acpigen_{set,clear}_tx_gpio to
> + * make callbacks into SoC acpigen code.
> + *
> + * Returns 0 on success and -1 on error.
> + */
> +int acpigen_set_enable_tx_gpio(struct acpi_ctx *ctx, u32 tx_state_val,
> +                              const char *dw0_name, struct acpi_gpio *gpio,
> +                              bool enable)
> +{
> +       bool set;
> +       int ret;
> +
> +       set = gpio->polarity == ACPI_GPIO_ACTIVE_HIGH ? enable : !enable;
> +       ret = acpigen_set_gpio_val(ctx, tx_state_val, dw0_name, gpio, set);
> +       if (ret)
> +               return log_msg_ret("call", ret);
> +
> +       return 0;
> +}
> diff --git a/test/dm/acpigen.c b/test/dm/acpigen.c
> index e66508932b..cbd0007365 100644
> --- a/test/dm/acpigen.c
> +++ b/test/dm/acpigen.c
> @@ -741,3 +741,57 @@ static int dm_test_acpi_power_res(struct unit_test_state *uts)
>         return 0;
>  }
>  DM_TEST(dm_test_acpi_power_res, 0);
> +
> +/* Test writing ACPI code to toggle a GPIO */
> +static int dm_test_acpi_gpio_toggle(struct unit_test_state *uts)
> +{
> +       const uint addr = 0x80012;
> +       const int txbit = BIT(2);
> +       struct gpio_desc desc;
> +       struct acpi_gpio gpio;
> +       struct acpi_ctx *ctx;
> +       struct udevice *dev;
> +       u8 *ptr;
> +
> +       ut_assertok(alloc_context(&ctx));
> +
> +       ut_assertok(uclass_get_device(UCLASS_TEST_FDT, 0, &dev));
> +       ut_asserteq_str("a-test", dev->name);
> +       ut_assertok(gpio_request_by_name(dev, "test2-gpios", 2, &desc, 0));
> +       ut_assertok(gpio_get_acpi(&desc, &gpio));
> +
> +       /* Spot-check the results - see sb_gpio_get_acpi() */
> +       ptr = acpigen_get_current(ctx);
> +       acpigen_set_enable_tx_gpio(ctx, txbit, "\\_SB.GPC0", &gpio, true);
> +       acpigen_set_enable_tx_gpio(ctx, txbit, "\\_SB.GPC0", &gpio, false);
> +
> +       /* Since this GPIO is active low, we expect it to be cleared here */
> +       ut_asserteq(STORE_OP, ptr[0]);
> +       ut_asserteq_strn("_SB_GPC0", (char *)ptr + 3);
> +       ut_asserteq(addr + desc.offset, get_unaligned((u32 *)(ptr + 0xc)));
> +       ut_asserteq(LOCAL5_OP, ptr[0x10]);
> +
> +       ut_asserteq(NOT_OP, ptr[0x11]);
> +       ut_asserteq(txbit, ptr[0x12]);
> +       ut_asserteq(AND_OP, ptr[0x14]);
> +       ut_asserteq_strn("_SB_GPC0", (char *)ptr + 0x1a);
> +       ut_asserteq(addr + desc.offset, get_unaligned((u32 *)(ptr + 0x23)));
> +       ut_asserteq(LOCAL5_OP, ptr[0x27]);
> +
> +       /* Now the second one, which should be set */
> +       ut_asserteq_strn("_SB_GPC0", (char *)ptr + 0x2b);
> +       ut_asserteq(addr + desc.offset, get_unaligned((u32 *)(ptr + 0x34)));
> +       ut_asserteq(LOCAL5_OP, ptr[0x38]);
> +
> +       ut_asserteq(OR_OP, ptr[0x39]);
> +       ut_asserteq(txbit, ptr[0x3b]);
> +       ut_asserteq_strn("_SB_GPC0", (char *)ptr + 0x3f);
> +       ut_asserteq(addr + desc.offset, get_unaligned((u32 *)(ptr + 0x48)));
> +       ut_asserteq(LOCAL5_OP, ptr[0x4c]);
> +       ut_asserteq(0x4d, acpigen_get_current(ctx) - ptr);
> +
> +       free_context(&ctx);
> +
> +       return 0;
> +}
> +DM_TEST(dm_test_acpi_gpio_toggle, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);

Regards,
Bin