Hi Simon,
On Sun, Jun 14, 2020 at 10:55 AM Simon Glass <s...@chromium.org> wrote:
>
> When generating ACPI tables we need to convert GPIOs in U-Boot to the ACPI
> structures required by ACPI. This is a SoC-specific conversion and cannot
> be handled by generic code, so add a new GPIO method to do the conversion.
>
> Signed-off-by: Simon Glass <s...@chromium.org>
> Reviewed-by: Wolfgang Wallner <wolfgang.wall...@br-automation.com>
> ---
>
> (no changes since v1)
>
> Changes in v1:
> - Update sandbox driver slightly for testing
>
> drivers/gpio/gpio-uclass.c | 21 +++++++++
> drivers/gpio/sandbox.c | 86 ++++++++++++++++++++++++++++++++++++
> include/acpi/acpi_device.h | 90 ++++++++++++++++++++++++++++++++++++++
> include/asm-generic/gpio.h | 27 ++++++++++++
> test/dm/gpio.c | 62 ++++++++++++++++++++++++++
> test/dm/irq.c | 1 +
> 6 files changed, 287 insertions(+)
>
> diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
> index 9eeab22eef..e2f4d7e02c 100644
> --- a/drivers/gpio/gpio-uclass.c
> +++ b/drivers/gpio/gpio-uclass.c
> @@ -13,6 +13,7 @@
> #include <errno.h>
> #include <fdtdec.h>
> #include <malloc.h>
> +#include <acpi/acpi_device.h>
> #include <asm/gpio.h>
> #include <dm/device_compat.h>
> #include <linux/bug.h>
> @@ -809,6 +810,26 @@ int gpio_get_status(struct udevice *dev, int offset,
> char *buf, int buffsize)
> return 0;
> }
>
> +#if CONFIG_IS_ENABLED(ACPIGEN)
> +int gpio_get_acpi(const struct gpio_desc *desc, struct acpi_gpio *gpio)
> +{
> + struct dm_gpio_ops *ops;
> +
> + if (!dm_gpio_is_valid(desc)) {
> + /* Indicate that the GPIO is not valid */
> + gpio->pin_count = 0;
> + gpio->pins[0] = 0;
> + return -EINVAL;
> + }
> +
Let's do memset() to gpio so that GPIO driver doesn't need to clear by
themselves, to avoid potential issues.
> + ops = gpio_get_ops(desc->dev);
> + if (!ops->get_acpi)
> + return -ENOSYS;
> +
> + return ops->get_acpi(desc, gpio);
> +}
> +#endif
> +
> int gpio_claim_vector(const int *gpio_num_array, const char *fmt)
> {
> int i, ret;
> diff --git a/drivers/gpio/sandbox.c b/drivers/gpio/sandbox.c
> index 98b7fa4bb3..6cd2a1455a 100644
> --- a/drivers/gpio/sandbox.c
> +++ b/drivers/gpio/sandbox.c
> @@ -8,7 +8,9 @@
> #include <fdtdec.h>
> #include <log.h>
> #include <malloc.h>
> +#include <acpi/acpi_device.h>
> #include <asm/gpio.h>
> +#include <dm/acpi.h>
> #include <dm/device_compat.h>
> #include <dm/lists.h>
> #include <dm/of.h>
> @@ -197,6 +199,72 @@ static int sb_gpio_get_dir_flags(struct udevice *dev,
> unsigned int offset,
> return 0;
> }
>
> +#if CONFIG_IS_ENABLED(ACPIGEN)
> +static int sb_gpio_get_acpi(const struct gpio_desc *desc,
> + struct acpi_gpio *gpio)
> +{
> + int ret;
> +
> + /* All of these values are just used for testing */
> + if (desc->flags & GPIOD_ACTIVE_LOW) {
> + gpio->pin_count = 1;
> + gpio->pins[0] = desc->offset;
> + gpio->pin0_addr = 0x80012 + desc->offset;
> + gpio->type = ACPI_GPIO_TYPE_INTERRUPT;
> + gpio->pull = ACPI_GPIO_PULL_DOWN;
> + ret = acpi_device_scope(desc->dev, gpio->resource,
> + sizeof(gpio->resource));
> + if (ret)
> + return log_ret(ret);
> + gpio->interrupt_debounce_timeout = 4321;
> + memset(&gpio->irq, '\0', sizeof(gpio->irq));
> +
> + /* We use the GpioInt part */
> + gpio->irq.pin = desc->offset;
> + gpio->irq.polarity = ACPI_IRQ_ACTIVE_BOTH;
> + gpio->irq.shared = ACPI_IRQ_SHARED;
> + gpio->irq.wake = ACPI_IRQ_WAKE;
> +
> + /* The GpioIo part is not used */
> + gpio->output_drive_strength = 0;
> + gpio->io_shared = false;
> + gpio->io_restrict = 0;
> + gpio->polarity = ACPI_GPIO_ACTIVE_LOW;
> + } else {
> + gpio->pin_count = 1;
> + gpio->pins[0] = desc->offset;
> + gpio->pin0_addr = 0xc00dc + desc->offset;
> + gpio->type = ACPI_GPIO_TYPE_IO;
> + gpio->pull = ACPI_GPIO_PULL_UP;
> + ret = acpi_device_scope(desc->dev, gpio->resource,
> + sizeof(gpio->resource));
> + if (ret)
> + return log_ret(ret);
> + gpio->interrupt_debounce_timeout = 0;
> +
> + /* The GpioInt part is not used */
> + memset(&gpio->irq, '\0', sizeof(gpio->irq));
> +
> + /* We use the GpioIo part */
> + gpio->output_drive_strength = 1234;
> + gpio->io_shared = true;
> + gpio->io_restrict = ACPI_GPIO_IO_RESTRICT_INPUT;
> + gpio->polarity = 0;
> + }
Could we refactor the logic a little bit, such that the common part is
put outside the if () else () logic, and only different part is put
into the if () or else () part.
> +
> + return 0;
> +}
> +
> +static int sb_gpio_get_name(const struct udevice *dev, char *out_name)
> +{
> + return acpi_copy_name(out_name, "GPIO");
> +}
> +
> +struct acpi_ops gpio_sandbox_acpi_ops = {
> + .get_name = sb_gpio_get_name,
> +};
> +#endif /* ACPIGEN */
> +
> static const struct dm_gpio_ops gpio_sandbox_ops = {
> .direction_input = sb_gpio_direction_input,
> .direction_output = sb_gpio_direction_output,
> @@ -206,6 +274,9 @@ static const struct dm_gpio_ops gpio_sandbox_ops = {
> .xlate = sb_gpio_xlate,
> .set_dir_flags = sb_gpio_set_dir_flags,
> .get_dir_flags = sb_gpio_get_dir_flags,
> +#if CONFIG_IS_ENABLED(ACPIGEN)
> + .get_acpi = sb_gpio_get_acpi,
> +#endif
> };
>
> static int sandbox_gpio_ofdata_to_platdata(struct udevice *dev)
> @@ -252,6 +323,7 @@ U_BOOT_DRIVER(gpio_sandbox) = {
> .probe = gpio_sandbox_probe,
> .remove = gpio_sandbox_remove,
> .ops = &gpio_sandbox_ops,
> + ACPI_OPS_PTR(&gpio_sandbox_acpi_ops)
> };
>
> /* pincontrol: used only to check GPIO pin configuration (pinmux command) */
> @@ -419,6 +491,13 @@ static int sb_pinctrl_get_pin_muxing(struct udevice *dev,
> return 0;
> }
>
> +#if CONFIG_IS_ENABLED(ACPIGEN)
> +static int sb_pinctrl_get_name(const struct udevice *dev, char *out_name)
> +{
> + return acpi_copy_name(out_name, "PINC");
> +}
> +#endif
> +
> static int sandbox_pinctrl_probe(struct udevice *dev)
> {
> struct sb_pinctrl_priv *priv = dev_get_priv(dev);
> @@ -434,6 +513,12 @@ static struct pinctrl_ops sandbox_pinctrl_gpio_ops = {
> .get_pin_muxing = sb_pinctrl_get_pin_muxing,
> };
>
> +#if CONFIG_IS_ENABLED(ACPIGEN)
> +struct acpi_ops pinctrl_sandbox_acpi_ops = {
> + .get_name = sb_pinctrl_get_name,
> +};
> +#endif
> +
> static const struct udevice_id sandbox_pinctrl_gpio_match[] = {
> { .compatible = "sandbox,pinctrl-gpio" },
> { /* sentinel */ }
> @@ -447,4 +532,5 @@ U_BOOT_DRIVER(sandbox_pinctrl_gpio) = {
> .bind = dm_scan_fdt_dev,
> .probe = sandbox_pinctrl_probe,
> .priv_auto_alloc_size = sizeof(struct sb_pinctrl_priv),
> + ACPI_OPS_PTR(&pinctrl_sandbox_acpi_ops)
> };
> diff --git a/include/acpi/acpi_device.h b/include/acpi/acpi_device.h
> index 4f87cd003a..cb9166aeae 100644
> --- a/include/acpi/acpi_device.h
> +++ b/include/acpi/acpi_device.h
> @@ -92,6 +92,96 @@ struct acpi_irq {
> enum acpi_irq_wake wake;
> };
>
> +/**
> + * enum acpi_gpio_type - type of the descriptor
> + *
> + * @ACPI_GPIO_TYPE_INTERRUPT: GpioInterrupt
> + * @ACPI_GPIO_TYPE_IO: GpioIo
> + */
> +enum acpi_gpio_type {
> + ACPI_GPIO_TYPE_INTERRUPT,
> + ACPI_GPIO_TYPE_IO,
> +};
> +
> +/**
> + * enum acpi_gpio_pull - pull direction
> + *
> + * @ACPI_GPIO_PULL_DEFAULT: Use default value for pin
> + * @ACPI_GPIO_PULL_UP: Pull up
> + * @ACPI_GPIO_PULL_DOWN: Pull down
> + * @ACPI_GPIO_PULL_NONE: No pullup/pulldown
> + */
> +enum acpi_gpio_pull {
> + ACPI_GPIO_PULL_DEFAULT,
> + ACPI_GPIO_PULL_UP,
> + ACPI_GPIO_PULL_DOWN,
> + ACPI_GPIO_PULL_NONE,
> +};
> +
> +/**
> + * enum acpi_gpio_io_restrict - controls input/output of pin
> + *
> + * @ACPI_GPIO_IO_RESTRICT_NONE: no restrictions
> + * @ACPI_GPIO_IO_RESTRICT_INPUT: input only (no output)
> + * @ACPI_GPIO_IO_RESTRICT_OUTPUT: output only (no input)
> + * @ACPI_GPIO_IO_RESTRICT_PRESERVE: preserve settings when driver not active
> + */
> +enum acpi_gpio_io_restrict {
> + ACPI_GPIO_IO_RESTRICT_NONE,
> + ACPI_GPIO_IO_RESTRICT_INPUT,
> + ACPI_GPIO_IO_RESTRICT_OUTPUT,
> + ACPI_GPIO_IO_RESTRICT_PRESERVE,
> +};
> +
> +/** enum acpi_gpio_polarity - controls the GPIO polarity */
> +enum acpi_gpio_polarity {
> + ACPI_GPIO_ACTIVE_HIGH = 0,
> + ACPI_GPIO_ACTIVE_LOW = 1,
> +};
> +
> +#define ACPI_GPIO_REVISION_ID 1
> +#define ACPI_GPIO_MAX_PINS 2
> +
> +/**
> + * struct acpi_gpio - representation of an ACPI GPIO
> + *
> + * @pin_count: Number of pins represented
> + * @pins: List of pins
> + * @pin0_addr: Address in memory of the control registers for pin 0. This is
> + * used when generating ACPI tables
> + * @type: GPIO type
> + * @pull: Pullup/pulldown setting
> + * @resource: Resource name for this GPIO controller
> + * For GpioInt:
> + * @interrupt_debounce_timeout: Debounce timeout in units of 10us
> + * @irq: Interrupt
> + *
> + * For GpioIo:
> + * @output_drive_strength: Drive strength in units of 10uA
> + * @io_shared; true if GPIO is shared
> + * @io_restrict: I/O restriction setting
> + * @polarity: GPIO polarity
> + */
> +struct acpi_gpio {
> + int pin_count;
> + u16 pins[ACPI_GPIO_MAX_PINS];
> + ulong pin0_addr;
> +
> + enum acpi_gpio_type type;
> + enum acpi_gpio_pull pull;
> + char resource[ACPI_PATH_MAX];
> +
> + /* GpioInt */
> + u16 interrupt_debounce_timeout;
> + struct acpi_irq irq;
> +
> + /* GpioIo */
> + u16 output_drive_strength;
> + bool io_shared;
> + enum acpi_gpio_io_restrict io_restrict;
> + enum acpi_gpio_polarity polarity;
> +};
> +
> /**
> * acpi_device_path() - Get the full path to an ACPI device
> *
> diff --git a/include/asm-generic/gpio.h b/include/asm-generic/gpio.h
> index e16c2f31d9..a57dd2665c 100644
> --- a/include/asm-generic/gpio.h
> +++ b/include/asm-generic/gpio.h
> @@ -10,6 +10,7 @@
> #include <dm/ofnode.h>
> #include <linux/bitops.h>
>
> +struct acpi_gpio;
> struct ofnode_phandle_args;
>
> /*
> @@ -329,6 +330,20 @@ struct dm_gpio_ops {
> */
> int (*get_dir_flags)(struct udevice *dev, unsigned int offset,
> ulong *flags);
> +
> +#if CONFIG_IS_ENABLED(ACPIGEN)
> + /**
> + * get_acpi() - Get the ACPI info for a GPIO
> + *
> + * This converts a GPIO to an ACPI structure for adding to the ACPI
> + * tables.
> + *
> + * @desc: GPIO description to convert
> + * @gpio: Output ACPI GPIO information
> + * @return ACPI pin number or -ve on error
> + */
> + int (*get_acpi)(const struct gpio_desc *desc, struct acpi_gpio *gpio);
> +#endif
> };
>
> /**
> @@ -674,4 +689,16 @@ int dm_gpio_get_dir_flags(struct gpio_desc *desc, ulong
> *flags);
> */
> int gpio_get_number(const struct gpio_desc *desc);
>
> +/**
> + * gpio_get_acpi() - Get the ACPI pin for a GPIO
> + *
> + * This converts a GPIO to an ACPI pin number for adding to the ACPI
> + * tables. If the GPIO is invalid, the pin_count and pins[0] are set to 0
> + *
> + * @desc: GPIO description to convert
> + * @gpio: Output ACPI GPIO information
> + * @return ACPI pin number or -ve on error
> + */
> +int gpio_get_acpi(const struct gpio_desc *desc, struct acpi_gpio *gpio);
> +
> #endif /* _ASM_GENERIC_GPIO_H_ */
> diff --git a/test/dm/gpio.c b/test/dm/gpio.c
> index b5ee4e4f87..843ba2c894 100644
> --- a/test/dm/gpio.c
> +++ b/test/dm/gpio.c
> @@ -8,6 +8,7 @@
> #include <dm.h>
> #include <log.h>
> #include <malloc.h>
> +#include <acpi/acpi_device.h>
> #include <dm/root.h>
> #include <dm/test.h>
> #include <dm/util.h>
> @@ -385,3 +386,64 @@ static int dm_test_gpio_get_dir_flags(struct
> unit_test_state *uts)
> return 0;
> }
> DM_TEST(dm_test_gpio_get_dir_flags, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
> +
> +/* Test of gpio_get_acpi() */
> +static int dm_test_gpio_get_acpi(struct unit_test_state *uts)
> +{
> + struct acpi_gpio agpio;
> + struct udevice *dev;
> + struct gpio_desc desc;
> +
> + ut_assertok(uclass_get_device(UCLASS_TEST_FDT, 0, &dev));
> + ut_asserteq_str("a-test", dev->name);
> + ut_assertok(gpio_request_by_name(dev, "test-gpios", 1, &desc, 0));
> +
> + /* See sb_gpio_get_acpi() */
> + ut_assertok(gpio_get_acpi(&desc, &agpio));
> + ut_asserteq(1, agpio.pin_count);
> + ut_asserteq(4, agpio.pins[0]);
> + ut_asserteq(ACPI_GPIO_TYPE_IO, agpio.type);
> + ut_asserteq(ACPI_GPIO_PULL_UP, agpio.pull);
> + ut_asserteq_str("\\_SB.PINC", agpio.resource);
> + ut_asserteq(0, agpio.interrupt_debounce_timeout);
> + ut_asserteq(0, agpio.irq.pin);
> + ut_asserteq(1234, agpio.output_drive_strength);
> + ut_asserteq(true, agpio.io_shared);
> + ut_asserteq(ACPI_GPIO_IO_RESTRICT_INPUT, agpio.io_restrict);
> + ut_asserteq(ACPI_GPIO_ACTIVE_HIGH, agpio.polarity);
> +
> + return 0;
> +}
> +DM_TEST(dm_test_gpio_get_acpi, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
> +
> +/* Test of gpio_get_acpi() with an interrupt GPIO */
> +static int dm_test_gpio_get_acpi_irq(struct unit_test_state *uts)
> +{
> + struct acpi_gpio agpio;
> + struct udevice *dev;
> + struct gpio_desc desc;
> +
> + 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));
> +
> + /* See sb_gpio_get_acpi() */
> + ut_assertok(gpio_get_acpi(&desc, &agpio));
> + ut_asserteq(1, agpio.pin_count);
> + ut_asserteq(6, agpio.pins[0]);
> + ut_asserteq(ACPI_GPIO_TYPE_INTERRUPT, agpio.type);
> + ut_asserteq(ACPI_GPIO_PULL_DOWN, agpio.pull);
> + ut_asserteq_str("\\_SB.PINC", agpio.resource);
> + ut_asserteq(4321, agpio.interrupt_debounce_timeout);
> + ut_asserteq(6, agpio.irq.pin);
> + ut_asserteq(ACPI_IRQ_ACTIVE_BOTH, agpio.irq.polarity);
> + ut_asserteq(ACPI_IRQ_SHARED, agpio.irq.shared);
> + ut_asserteq(true, agpio.irq.wake);
> + ut_asserteq(0, agpio.output_drive_strength);
> + ut_asserteq(false, agpio.io_shared);
> + ut_asserteq(0, agpio.io_restrict);
> + ut_asserteq(ACPI_GPIO_ACTIVE_LOW, agpio.polarity);
> +
> + return 0;
> +}
> +DM_TEST(dm_test_gpio_get_acpi_irq, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
> diff --git a/test/dm/irq.c b/test/dm/irq.c
> index 50e505e657..51bae31b0f 100644
> --- a/test/dm/irq.c
> +++ b/test/dm/irq.c
> @@ -87,6 +87,7 @@ static int dm_test_irq_get_acpi(struct unit_test_state *uts)
> ut_assertok(uclass_first_device_err(UCLASS_TEST_FDT, &dev));
> ut_assertok(irq_get_by_index(dev, 0, &irq));
>
> + /* see sandbox_get_acpi() */
> ut_assertok(irq_get_acpi(&irq, &airq));
> ut_asserteq(3, airq.pin);
> ut_asserteq(ACPI_IRQ_LEVEL_TRIGGERED, airq.mode);
> --
Regards,
Bin
