Re: [PATCH v2 12/35] acpi: Support generation of SPI descriptor

Hi Simon,

-----"Simon Glass" sjg@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@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