[PATCH v2 12/35] acpi: Support generation of SPI descriptor
Wolfgang Wallner
wolfgang.wallner at br-automation.com
Wed Jun 10 10:40:34 CEST 2020
Hi Simon,
-----"Simon Glass" <sjg at chromium.org> schrieb: -----
> Betreff: [PATCH v2 12/35] acpi: Support generation of SPI descriptor
>
> Add a function to write a SPI descriptor to the generated ACPI code.
>
> Signed-off-by: Simon Glass <sjg at chromium.org>
> ---
>
> Changes in v2:
> - Fix memset of SPI descriptor
>
> Changes in v1: None
>
> drivers/spi/sandbox_spi.c | 11 ++++
> include/acpi/acpi_device.h | 36 +++++++++++
> include/spi.h | 4 +-
> lib/acpi/acpi_device.c | 121 +++++++++++++++++++++++++++++++++++++
> test/dm/acpigen.c | 36 +++++++++++
> 5 files changed, 206 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/sandbox_spi.c b/drivers/spi/sandbox_spi.c
> index b0a46c8868..4264acc953 100644
> --- a/drivers/spi/sandbox_spi.c
> +++ b/drivers/spi/sandbox_spi.c
> @@ -21,6 +21,7 @@
> #include <linux/errno.h>
> #include <asm/spi.h>
> #include <asm/state.h>
> +#include <dm/acpi.h>
> #include <dm/device-internal.h>
>
> #ifndef CONFIG_SPI_IDLE_VAL
> @@ -133,6 +134,15 @@ static int sandbox_spi_get_mmap(struct udevice *dev, ulong *map_basep,
> return 0;
> }
>
> +static int sandbox_spi_get_name(const struct udevice *dev, char *out_name)
> +{
> + return acpi_copy_name(out_name, "SSPI");
> +}
> +
> +struct acpi_ops sandbox_spi_acpi_ops = {
> + .get_name = sandbox_spi_get_name,
> +};
> +
> static const struct dm_spi_ops sandbox_spi_ops = {
> .xfer = sandbox_spi_xfer,
> .set_speed = sandbox_spi_set_speed,
> @@ -151,4 +161,5 @@ U_BOOT_DRIVER(spi_sandbox) = {
> .id = UCLASS_SPI,
> .of_match = sandbox_spi_ids,
> .ops = &sandbox_spi_ops,
> + ACPI_OPS_PTR(&sandbox_spi_acpi_ops)
> };
> diff --git a/include/acpi/acpi_device.h b/include/acpi/acpi_device.h
> index cf1ac695a7..d43ab4ba36 100644
> --- a/include/acpi/acpi_device.h
> +++ b/include/acpi/acpi_device.h
> @@ -10,6 +10,7 @@
> #define __ACPI_DEVICE_H
>
> #include <i2c.h>
> +#include <spi.h>
> #include <linux/bitops.h>
>
> struct acpi_ctx;
> @@ -207,6 +208,29 @@ struct acpi_i2c {
> const char *resource;
> };
>
> +/**
> + * struct acpi_spi - representation of an ACPI SPI device
> + *
> + * @device_select: Chip select used by this device (typically 0)
> + * @device_select_polarity: Polarity for the device
> + * @wire_mode: Number of wires used for SPI
> + * @speed: Bus speed in Hz
> + * @data_bit_length: Word length for SPI (typically 8)
> + * @clock_phase: Clock phase to capture data
> + * @clock_polarity: Bus polarity
> + * @resource: Resource name for the SPI controller
> + */
> +struct acpi_spi {
> + u16 device_select;
> + enum spi_polarity device_select_polarity;
> + enum spi_wire_mode wire_mode;
> + unsigned int speed;
> + u8 data_bit_length;
> + enum spi_clock_phase clock_phase;
> + enum spi_polarity clock_polarity;
> + const char *resource;
> +};
> +
> /**
> * acpi_device_path() - Get the full path to an ACPI device
> *
> @@ -306,4 +330,16 @@ int acpi_device_write_interrupt_or_gpio(struct acpi_ctx *ctx,
> */
> int acpi_device_write_i2c_dev(struct acpi_ctx *ctx, const struct udevice *dev);
>
> +/**
> + * acpi_device_write_spi_dev() - Write a SPI device to ACPI
Nit: Should it be "an" SPI device instead of "a" SPI device?
(at least as I pronounce it, the abbreviation starts with a vowel)
> + *
> + * This writes a serial bus descriptor for the SPI device so that ACPI can use
> + * it
> + *
> + * @ctx: ACPI context pointer
> + * @dev: SPI device to write
> + * @return 0 if OK, -ve on error
> + */
> +int acpi_device_write_spi_dev(struct acpi_ctx *ctx, const struct udevice *dev);
> +
> #endif
> diff --git a/include/spi.h b/include/spi.h
> index 5cc6d6e008..f34533f54e 100644
> --- a/include/spi.h
> +++ b/include/spi.h
> @@ -13,8 +13,8 @@
> #include <linux/bitops.h>
>
> /* SPI mode flags */
> -#define SPI_CPHA BIT(0) /* clock phase */
> -#define SPI_CPOL BIT(1) /* clock polarity */
> +#define SPI_CPHA BIT(0) /* clock phase (1 = SPI_CLOCK_PHASE_SECOND) */
> +#define SPI_CPOL BIT(1) /* clock polarity (1 = SPI_POLARITY_HIGH) */
Nit: I assume the comments are now unaligned to stay within 80 characters/line.
According to [1], I think it would be fine to keep them aligned, and trade off
slightly longer line length vs readability.
[1] https://lists.denx.de/pipermail/u-boot/2020-June/414419.html
> #define SPI_MODE_0 (0|0) /* (original MicroWire) */
> #define SPI_MODE_1 (0|SPI_CPHA)
> #define SPI_MODE_2 (SPI_CPOL|0)
> diff --git a/lib/acpi/acpi_device.c b/lib/acpi/acpi_device.c
> index 0136a0bdc9..3d5dc746f8 100644
> --- a/lib/acpi/acpi_device.c
> +++ b/lib/acpi/acpi_device.c
> @@ -484,3 +484,124 @@ int acpi_device_write_i2c_dev(struct acpi_ctx *ctx, const struct udevice *dev)
>
> return 0;
> }
> +
> +#ifdef CONFIG_SPI
> +/* ACPI 6.1 section 6.4.3.8.2.2 - SpiSerialBus() */
> +void acpi_device_write_spi(struct acpi_ctx *ctx, const struct acpi_spi *spi)
I think this function should be static.
> +{
> + void *desc_length, *type_length;
> + u16 flags = 0;
> +
> + /* Byte 0: Descriptor Type */
> + acpigen_emit_byte(ctx, ACPI_DESCRIPTOR_SERIAL_BUS);
> +
> + /* Byte 1+2: Length (filled in later) */
> + desc_length = acpi_device_write_zero_len(ctx);
> +
> + /* Byte 3: Revision ID */
> + acpigen_emit_byte(ctx, ACPI_SPI_SERIAL_BUS_REVISION_ID);
> +
> + /* Byte 4: Resource Source Index is Reserved */
> + acpigen_emit_byte(ctx, 0);
> +
> + /* Byte 5: Serial Bus Type is SPI */
> + acpigen_emit_byte(ctx, ACPI_SERIAL_BUS_TYPE_SPI);
> +
> + /*
> + * Byte 6: Flags
> + * [7:2]: 0 => Reserved
> + * [1]: 1 => ResourceConsumer
> + * [0]: 0 => ControllerInitiated
> + */
> + acpigen_emit_byte(ctx, 1 << 1);
Nit: BIT(1) ?
> +
> + /*
> + * Byte 7-8: Type Specific Flags
> + * [15:2]: 0 => Reserveda
> + * [1]: 0 => ActiveLow, 1 => ActiveHigh
> + * [0]: 0 => FourWire, 1 => ThreeWire
> + */
> + if (spi->wire_mode == SPI_3_WIRE_MODE)
> + flags |= 1 << 0;
> + if (spi->device_select_polarity == SPI_POLARITY_HIGH)
> + flags |= 1 << 1;
> + acpigen_emit_word(ctx, flags);
> +
> + /* Byte 9: Type Specific Revision ID */
> + acpigen_emit_byte(ctx, ACPI_SPI_TYPE_SPECIFIC_REVISION_ID);
> +
> + /* Byte 10-11: SPI Type Data Length */
> + type_length = acpi_device_write_zero_len(ctx);
> +
> + /* Byte 12-15: Connection Speed */
> + acpigen_emit_dword(ctx, spi->speed);
> +
> + /* Byte 16: Data Bit Length */
> + acpigen_emit_byte(ctx, spi->data_bit_length);
> +
> + /* Byte 17: Clock Phase */
> + acpigen_emit_byte(ctx, spi->clock_phase);
> +
> + /* Byte 18: Clock Polarity */
> + acpigen_emit_byte(ctx, spi->clock_polarity);
> +
> + /* Byte 19-20: Device Selection */
> + acpigen_emit_word(ctx, spi->device_select);
> +
> + /* Fill in Type Data Length */
> + acpi_device_fill_len(ctx, type_length);
> +
> + /* Byte 21+: ResourceSource String */
> + acpigen_emit_string(ctx, spi->resource);
> +
> + /* Fill in SPI Descriptor Length */
> + acpi_device_fill_len(ctx, desc_length);
> +}
> +
> +/**
> + * acpi_device_set_spi() - Set up an ACPI SPI struct from a device
> + *
> + * @dev: SPI device to convert
> + * @spi: Place to put the new structure
> + * @scope: Scope of the SPI device (this is the controller path)
As with the previous patch (about I2C) I would propose to add an additional
comment to state that scope is only referenced.
> + * @return 0 (always)
> + */
> +static int acpi_device_set_spi(const struct udevice *dev, struct acpi_spi *spi,
> + const char *scope)
> +{
> + struct dm_spi_slave_platdata *plat;
> + struct spi_slave *slave = dev_get_parent_priv(dev);
> +
> + plat = dev_get_parent_platdata(slave->dev);
> + memset(spi, '\0', sizeof(*spi));
> + spi->device_select = plat->cs;
> + spi->device_select_polarity = SPI_POLARITY_LOW;
> + spi->wire_mode = SPI_4_WIRE_MODE;
> + spi->speed = plat->max_hz;
> + spi->data_bit_length = slave->wordlen;
> + spi->clock_phase = plat->mode & SPI_CPHA ?
> + SPI_CLOCK_PHASE_SECOND : SPI_CLOCK_PHASE_FIRST;
> + spi->clock_polarity = plat->mode & SPI_CPOL ?
> + SPI_POLARITY_HIGH : SPI_POLARITY_LOW;
> + spi->resource = scope;
> +
> + return 0;
> +}
> +
> +int acpi_device_write_spi_dev(struct acpi_ctx *ctx, const struct udevice *dev)
> +{
> + char scope[ACPI_PATH_MAX];
> + struct acpi_spi spi;
> + int ret;
> +
> + ret = acpi_device_scope(dev, scope, sizeof(scope));
> + if (ret)
> + return log_msg_ret("scope", ret);
> + ret = acpi_device_set_spi(dev, &spi, scope);
> + if (ret)
> + return log_msg_ret("set", ret);
> + acpi_device_write_spi(ctx, &spi);
> +
> + return 0;
> +}
> +#endif /* CONFIG_SPI */
> diff --git a/test/dm/acpigen.c b/test/dm/acpigen.c
> index de9996ab35..3d580a23a5 100644
> --- a/test/dm/acpigen.c
> +++ b/test/dm/acpigen.c
> @@ -298,3 +298,39 @@ static int dm_test_acpi_i2c(struct unit_test_state *uts)
> return 0;
> }
> DM_TEST(dm_test_acpi_i2c, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
> +
> +/* Test emitting a SPI descriptor */
> +static int dm_test_acpi_spi(struct unit_test_state *uts)
> +{
> + struct acpi_ctx *ctx;
> + struct udevice *dev;
> + u8 *ptr;
> +
> + ut_assertok(alloc_context(&ctx));
> +
> + ptr = acpigen_get_current(ctx);
> +
> + ut_assertok(uclass_first_device_err(UCLASS_SPI_FLASH, &dev));
> + ut_assertok(acpi_device_write_spi_dev(ctx, dev));
> + ut_asserteq(31, acpigen_get_current(ctx) - ptr);
> + ut_asserteq(ACPI_DESCRIPTOR_SERIAL_BUS, ptr[0]);
> + ut_asserteq(28, get_unaligned((u16 *)(ptr + 1)));
> + ut_asserteq(ACPI_SPI_SERIAL_BUS_REVISION_ID, ptr[3]);
> + ut_asserteq(0, ptr[4]);
> + ut_asserteq(ACPI_SERIAL_BUS_TYPE_SPI, ptr[5]);
> + ut_asserteq(2, ptr[6]);
> + ut_asserteq(0, get_unaligned((u16 *)(ptr + 7)));
> + ut_asserteq(ACPI_SPI_TYPE_SPECIFIC_REVISION_ID, ptr[9]);
> + ut_asserteq(9, get_unaligned((u16 *)(ptr + 10)));
> + ut_asserteq(40000000, get_unaligned((u32 *)(ptr + 12)));
> + ut_asserteq(8, ptr[16]);
> + ut_asserteq(0, ptr[17]);
> + ut_asserteq(0, ptr[18]);
> + ut_asserteq(0, get_unaligned((u16 *)(ptr + 19)));
> + ut_asserteq_str("\\_SB.SSPI", (char *)ptr + 21);
> +
> + free_context(&ctx);
> +
> + return 0;
> +}
> +DM_TEST(dm_test_acpi_spi, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);
> --
> 2.26.2.645.ge9eca65c58-goog
>
regards, Wolfgang
More information about the U-Boot
mailing list