misc: Add SCU IPC driver for Intel MID platforms

This simple PMU driver enables access to MMC controllers during probe so tangier_sdhci can probe and be useful.

In the future it might be expanded to cover other Intel MID platforms, that's why it's called intel_mid_pmu.c.

Signed-off-by Felipe Balbi <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

Hi Andy,

Quoted 8 lines · Andy Shevchenko

This simple PMU driver enables access to MMC controllers during probe so tangier_sdhci can probe and be useful.

In the future it might be expanded to cover other Intel MID platforms, that's why it's called intel_mid_pmu.c.

Signed-off-by Felipe Balbi <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

Please consider my comments on the previous driver to apply to this one also.

One more thing - you cannot call device_probe() from the bind function. That is breaking the rules.

Instead, it should be found by the first user, perhaps using syscon or perhaps with a device_get_...() function, and that will automatically probe it.

Quoted 18 lines · Simon Glass

Hi Andy,

Quoted 8 lines · Andy Shevchenko

This simple PMU driver enables access to MMC controllers during probe so tangier_sdhci can probe and be useful.

In the future it might be expanded to cover other Intel MID platforms, that's why it's called intel_mid_pmu.c.

Signed-off-by Felipe Balbi <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

Please consider my comments on the previous driver to apply to this one also.

One more thing - you cannot call device_probe() from the bind function. That is breaking the rules.

Quoted 3 lines

Instead, it should be found by the first user, perhaps using syscon or perhaps with a device_get_...() function, and that will automatically probe it.

While SCU works fine with this model, PMU doesn't.
MMC (sdhci) requires to power on controllers, it looks like device can't be probed at that time (too early). What do you suggest to do?

From: Felipe Balbi <[email protected]>

Intel MID platforms have few microcontrollers inside SoC, one of them is so called System Controller Unit (SCU).

Here is the driver to communicate with microcontroller.

Signed-off-by Vincent Tinelli <[email protected]>

Signed-off-by Felipe Balbi <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

---
 arch/x86/Kconfig             |   1 +
 drivers/misc/Kconfig         |   6 ++
 drivers/misc/Makefile        |   1 +
 drivers/misc/intel_scu_ipc.c | 199 +++++++++++++++++++++++++++++++++++++++++++
 include/intel_scu_ipc.h      |  57 +++++++++++++
 5 files changed, 264 insertions(+)
 create mode 100644 drivers/misc/intel_scu_ipc.c
 create mode 100644 include/intel_scu_ipc.h

arch/x86/Kconfig +1 -0

index dfdd7564ea..6a747c332e 100644
@@ -83,6 +83,7 @@ endchoice
 # subarchitectures-specific options below
 config INTEL_MID
        bool "Intel MID platform support"
+       select INTEL_SCU
        help
          Select to build a U-Boot capable of supporting Intel MID
          (Mobile Internet Device) platform systems which do not have

drivers/misc/Kconfig +0 -0

index 1aae4bcd07..8d81f9c51c 100644

drivers/misc/Kconfig +1 -0

+
 endmenu

drivers/misc/Makefile +1 -0

index e3151ea097..47551e44d6 100644
@@ -51,3 +51,4 @@ obj-$(CONFIG_PCA9551_LED) += pca9551_led.o
 obj-$(CONFIG_FSL_DEVICE_DISABLE) += fsl_devdis.o
 obj-$(CONFIG_WINBOND_W83627) += winbond_w83627.o
 obj-$(CONFIG_QFW) += qfw.o
+obj-$(CONFIG_INTEL_SCU) += intel_scu_ipc.o

drivers/misc/intel_scu_ipc.c new file +0 -0

new file mode 100644
index 0000000000..19a99b0b68

drivers/misc/intel_scu_ipc.c new file +6 -0

+
+static void scu_writel(void __iomem *base, unsigned int offset, unsigned int val)
+{
+       writel(val, base + offset);
+}
+

drivers/misc/intel_scu_ipc.c new file +21 -0

+ */
+static void intel_scu_ipc_send_command(struct intel_scu *scu, u32 cmd)
+{
+       scu_writel(scu->base, 0x00, cmd | IPC_IOC);
+}
+
+/*
+ * IPC Write Buffer (Write Only):
+ * 16-byte buffer for sending data associated with IPC command to
+ * SCU. Size of the data is specified in the IPC_COMMAND_REG register
+ */
+static void ipc_data_writel(struct intel_scu *scu, u32 data, u32 offset)
+{
+       scu_writel(scu->base, IPC_WRITE_BUFFER + offset, data);
+}
+
+/*
+ * Status Register (Read Only):
+ * Driver will read this register to get the ready/busy status of the IPC
+ * block and error status of the IPC command that was just processed by SCU
+ * Format:

drivers/misc/intel_scu_ipc.c new file +39 -0

+{
+       int i, err;
+       u32 wbuf[4];
+
+       if (inlen > 16)
+               return -EINVAL;
+
+       memcpy(wbuf, in, inlen);
+
+       scu_writel(scu->base, IPC_DPTR_ADDR, dptr);
+       scu_writel(scu->base, IPC_SPTR_ADDR, sptr);
+
+       /**
+        * SRAM controller don't support 8bit write, it only supports
+        * 32bit write, so we have to write into the WBUF in 32bit,
+        * and SCU FW will use the inlen to determine the actual input
+        * data length in the WBUF.
+        */
+       for (i = 0; i < DIV_ROUND_UP(inlen, 4); i++)
+               ipc_data_writel(scu, wbuf[i], 4 * i);
+
+       /**
+        * Watchdog IPC command is an exception here using double word
+        * as the unit of input data size because of some historical
+        * reasons and SCU FW is doing so.
+        */
+       if ((cmd & 0xff) == IPCMSG_WATCHDOG_TIMER)
+               inlen = DIV_ROUND_UP(inlen, 4);
+
+       intel_scu_ipc_send_command(scu, (inlen << 16) | (sub << 12) | cmd);
+       err = intel_scu_ipc_check_status(scu);
+
+       for (i = 0; i < outlen; i++)
+               *out++ = ipc_data_readl(scu, 4 * i);
+
+       return err;
+}
+
+/**

drivers/misc/intel_scu_ipc.c new file +35 -0

+ */
+int intel_scu_ipc_simple_command(int cmd, int sub)
+{
+       intel_scu_ipc_send_command(the_scu, sub << 12 | cmd);
+
+       return intel_scu_ipc_check_status(the_scu);
+}
+
+int intel_scu_ipc_command(u32 cmd, u32 sub, u8 *in, u8 inlen, u32 *out, u32 outlen)
+{
+       return intel_scu_ipc_raw_cmd(the_scu, cmd, sub, in, inlen, out, outlen, 0, 0);
+}
+
+static int intel_scu_bind(struct udevice *dev)
+{
+       /* This device is needed straight away */
+       return device_probe(dev);
+}
+
+static int intel_scu_probe(struct udevice *dev)
+{
+       struct intel_scu        *scu = dev_get_uclass_priv(dev);
+       fdt_addr_t              base;
+
+       base = dev_get_addr(dev);
+       if (base == FDT_ADDR_T_NONE)
+               return -EINVAL;
+
+       scu->base = devm_ioremap(dev, base, SZ_1K);
+       if (!scu->base)
+               return -ENOMEM;
+
+       the_scu = scu;
+
+       return 0;

drivers/misc/intel_scu_ipc.c new file +5 -0

+       .of_match       = intel_scu_match,
+       .bind           = intel_scu_bind,
+       .probe          = intel_scu_probe,
+       .priv_auto_alloc_size = sizeof(struct intel_scu),
+};

include/intel_scu_ipc.h new file +13 -0

new file mode 100644
index 0000000000..ded6352e10
@@ -0,0 +1,57 @@
+/*
+ * Copyright (c) 2017 Intel Corporation
+ *
+ * SPDX-License-Identifier:    GPL-2.0+
+ */
+#ifndef _INTEL_SCU_IPC_H_
+#define _INTEL_SCU_IPC_H_
+
+/* IPC defines the following message types */
+#define IPCMSG_WARM_RESET              0xF0
+#define IPCMSG_COLD_RESET              0xF1
+#define IPCMSG_SOFT_RESET              0xF2
+#define IPCMSG_COLD_BOOT               0xF3

include/intel_scu_ipc.h new file +5 -0

+
+/* Command id associated with message IPCMSG_VRTC */
+#define IPC_CMD_VRTC_SETTIME      1    /* Set time */
+#define IPC_CMD_VRTC_SETALARM     2    /* Set alarm */
+#define IPC_CMD_VRTC_SYNC_RTC     3    /* Sync MSIC/PMIC RTC to VRTC */

include/intel_scu_ipc.h new file +2 -1

+
+#endif /* _INTEL_SCU_IPC_H_ */
--

2.11.0

Quoted 8 lines

 # subarchitectures-specific options below
 config INTEL_MID
        bool "Intel MID platform support"
+       select INTEL_SCU
        help
          Select to build a U-Boot capable of supporting Intel MID
          (Mobile Internet Device) platform systems which do not 
have

Quoted 4 lines

+config INTEL_SCU
+       bool "Enable support for SCU on Intel MID platforms" +       depends on INTEL_MID
+       default y

Please add help explaining what SCU is and stands for,

and perhaps MID also.

Quoted 6 lines

drivers/misc/intel_scu_ipc.c new file +5 -0

@@ -0,0 +1,199 @@
+/*
+ * Copyright (c) 2017 Intel Corporation
+ *
+ * SPDX-License-Identifier:    GPL-2.0+
+ */

drivers/misc/intel_scu_ipc.c line 10

Quoted 5 lines

drivers/misc/intel_scu_ipc.c new file +5 -0

+#include <common.h>
+#include <dm.h>
+#include <dm/device-internal.h>
+#include <dm/device.h>
+#include <intel_scu_ipc.h>

This should go after dm.h. (see http://www.denx.de/wiki/U-
Boot/CodingStyle )

Quoted 6 lines

drivers/misc/intel_scu_ipc.c new file +6 -0

+#include <linux/errno.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/sizes.h>
+
+#define IPC_STATUS_ADDR         0x04

drivers/misc/intel_scu_ipc.c line 21

Quoted 5 lines

drivers/misc/intel_scu_ipc.c new file +5 -0

+#define IPC_SPTR_ADDR           0x08
+#define IPC_DPTR_ADDR           0x0C
+#define IPC_READ_BUFFER         0x90
+#define IPC_WRITE_BUFFER        0x80
+#define IPC_IOC                        BIT(8)

If these offsets don't change can we use a C struct to access the registers?

drivers/misc/intel_scu_ipc.c line 28

Quoted 7 lines

drivers/misc/intel_scu_ipc.c new file +7 -0

+
+struct intel_scu {
+       void __iomem *base;
+};
+
+/* Only one for now */
+static struct intel_scu *the_scu;

OK, but we cannot do this with driver model. It can be found using syscon_get_regmap_by_driver_data() perhaps?

Quoted 7 lines

drivers/misc/intel_scu_ipc.c new file +3 -0

+/*
+ * Command Register (Write Only):
+ * A write to this register results in an interrupt to the SCU core

processor
+ * Format:
+ * |rfu2(8) | size(8) | command id(4) | rfu1(3) | ioc(1) | command(8)|

Please can you use the standard function header format, something like:

/*
 * intel_scu_ipc_send_command() - send a command to the SCU *
 * @scu: Pointer to SCU
 * @cmd: Command to send (defined by ..?)
 */

If there is a return value, the last line would be something like:

@return torque propounder coefficient in mm

drivers/misc/intel_scu_ipc.c line 75

Quoted 3 lines

drivers/misc/intel_scu_ipc.c new file +3 -0

+static u32 ipc_data_readl(struct intel_scu *scu, u32 offset)
+{
+       return scu_readl(scu->base, IPC_READ_BUFFER + offset);

All of these functions take scu as a parameter. I wonder if they could take scu->regs, if you use a struct for the registers.

drivers/misc/intel_scu_ipc.c line 80

Quoted 5 lines

drivers/misc/intel_scu_ipc.c new file +5 -0

+}
+
+static int intel_scu_ipc_check_status(struct intel_scu *scu)
+{
+       int loop_count = 3000000;

3 million loops? Does that indicate some sort of problem?

Quoted 8 lines

+       int status;
+
+       do {
+               status = ipc_read_status(scu); +               udelay(1);
+       } while ((status & BIT(0)) && --loop_count); +       if (!loop_count)
+               return -ETIMEDOUT;

Given the long timeout, how about:

start = timer_get_us();

while (1) {

   status = ...
   if (!(status & BUSY))
      break;
  if ((int)(timer_get_us()  - start) > 3000000)
     return -ETIMEDOUT;
}

+static int intel_scu_ipc_raw_cmd(struct intel_scu *scu, u32 cmd,

Quoted 4 lines

u32 sub,
+                                u8 *in, u8 inlen, u32 *out, u32 outlen,
+                                u32 dptr, u32 sptr)

Function comment. Also try to avoid u32 for integer parameters. Use uint or ulong to avoid problems with 64-bit machines.

Quoted 11 lines

+/**
+ * intel_scu_ipc_simple_command - send a simple command
+ * @cmd: command
+ * @sub: sub type
+ *
+ * Issue a simple command to the SCU. Do not use this interface if
+ * you must then access data as any data values may be overwritten
+ * by another SCU access by the time this function returns.
+ *
+ * This function may sleep. Locking for SCU accesses is handled for
+ * the caller.

It that true?

Quoted 3 lines

+U_BOOT_DRIVER(intel_scu_ipc) = {
+       .name           = "intel_scu_ipc",
+       .id             = UCLASS_MISC,

Consider UCLASS_SYSCON since it allows you to find the device using an enum. See 'intel_me_syscon' for example where the device can be found using X86_SYSCON_ME

Quoted 6 lines

+       .of_match       = intel_scu_match,
+       .bind           = intel_scu_bind,
+       .probe          = intel_scu_probe,
+       .priv_auto_alloc_size = sizeof(struct intel_scu), +};

include/intel_scu_ipc.h line 23

Quoted 7 lines

include/intel_scu_ipc.h new file +7 -0

+#define IPC_ERR_NONE                   0
+#define IPC_ERR_CMD_NOT_SUPPORTED      1
+#define IPC_ERR_CMD_NOT_SERVICED       2
+#define IPC_ERR_UNABLE_TO_SERVICE      3
+#define IPC_ERR_CMD_INVALID            4
+#define IPC_ERR_CMD_FAILED             5
+#define IPC_ERR_EMSECURITY             6

Could be an enum

Quoted 10 lines

include/intel_scu_ipc.h new file +5 -0

+
+/* Command id associated with message IPCMSG_VRTC */
+#define IPC_CMD_VRTC_SETTIME      1    /* Set time */
+#define IPC_CMD_VRTC_SETALARM     2    /* Set alarm */
+#define IPC_CMD_VRTC_SYNC_RTC     3    /* Sync MSIC/PMIC RTC to

VRTC */

include/intel_scu_ipc.h new file +4 -0

+
+union ipc_ifwi_version {
+       u8 raw[16];
+       struct {

Quoted 6 lines

+               u16 ifwi_minor;
+               u8 ifwi_major;
+               u8 hardware_id;
+               u32 reserved[3];
+       } fw __attribute__((packed));
+} __attribute__((packed));

__packed.

The inner packed does not seem to be useful. For the outer
one, do you have multiple structs packed one after the other? If not, perhaps drop it?

Quoted 6 lines

include/intel_scu_ipc.h new file +4 -0

+
+/* Issue commands to the SCU with or without data */
+int intel_scu_ipc_simple_command(int cmd, int sub);
+int intel_scu_ipc_command(u32 cmd, u32 sub, u8 *in, u8 inlen, u32

*out, u32 outlen);

Full function comments. These functions should be passed a struct udevice for the device they use.

include/intel_scu_ipc.h line 55

Quoted 4 lines

include/intel_scu_ipc.h new file +4 -0

+static inline enum intel_mid_cpu_type intel_mid_identify_cpu(void)
+{
+       return INTEL_MID_CPU_CHIP_TANGIER;
+}

Is this identification a function of the SCU?

Quoted 8 lines · Simon Glass

Quoted 8 lines

 # subarchitectures-specific options below
 config INTEL_MID
        bool "Intel MID platform support"
+       select INTEL_SCU
        help
          Select to build a U-Boot capable of supporting Intel MID
          (Mobile Internet Device) platform systems which do not
have

Thanks for review. My comments / answers below.

Quoted 7 lines

drivers/misc/Kconfig config I2C_EEPROM · line 174

Quoted 4 lines

drivers/misc/Kconfig +4 -0

+config INTEL_SCU
+       bool "Enable support for SCU on Intel MID platforms"
+       depends on INTEL_MID
+       default y

Please add help explaining what SCU is and stands for,

OK.

Quoted 1 line

and perhaps MID also.

MID is explained in INTEL_MID help, though I can decode the abbreviation in the explanation of SCU.

Quoted 6 lines

Quoted 6 lines

drivers/misc/intel_scu_ipc.c new file +5 -0

@@ -0,0 +1,199 @@
+/*
+ * Copyright (c) 2017 Intel Corporation
+ *
+ * SPDX-License-Identifier:    GPL-2.0+
+ */

Quoted 8 lines

drivers/misc/intel_scu_ipc.c line 10

Quoted 5 lines

drivers/misc/intel_scu_ipc.c new file +5 -0

+#include <common.h>
+#include <dm.h>
+#include <dm/device-internal.h>
+#include <dm/device.h>
+#include <intel_scu_ipc.h>

This should go after dm.h. (see http://www.denx.de/wiki/U-
Boot/CodingStyle )

Got it.

Quoted 6 lines

Quoted 6 lines

drivers/misc/intel_scu_ipc.c new file +6 -0

+#include <linux/errno.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/sizes.h>
+
+#define IPC_STATUS_ADDR         0x04

Quoted 8 lines

drivers/misc/intel_scu_ipc.c line 21

Quoted 5 lines

drivers/misc/intel_scu_ipc.c new file +5 -0

+#define IPC_SPTR_ADDR           0x08
+#define IPC_DPTR_ADDR           0x0C
+#define IPC_READ_BUFFER         0x90
+#define IPC_WRITE_BUFFER        0x80
+#define IPC_IOC                        BIT(8)

If these offsets don't change can we use a C struct to access the registers?

Hmm... How would you think it will look like?

Quoted 11 lines

drivers/misc/intel_scu_ipc.c line 28

Quoted 8 lines

drivers/misc/intel_scu_ipc.c new file +7 -0

+
+struct intel_scu {
+       void __iomem *base;
+};
+
+/* Only one for now */
+static struct intel_scu *the_scu;

OK, but we cannot do this with driver model. It can be found using syscon_get_regmap_by_driver_data() perhaps?

I'm not familiar with such technique, can you elaborate a bit (perhaps link to some documentation)?

Quoted 19 lines

Quoted 8 lines

drivers/misc/intel_scu_ipc.c new file +3 -0

+/*
+ * Command Register (Write Only):
+ * A write to this register results in an interrupt to the SCU core

processor
+ * Format:
+ * |rfu2(8) | size(8) | command id(4) | rfu1(3) | ioc(1) | command(8)|

Please can you use the standard function header format, something like:

/*
* intel_scu_ipc_send_command() - send a command to the SCU *
* @scu: Pointer to SCU
* @cmd: Command to send (defined by ..?)
*/

Quoted 3 lines

If there is a return value, the last line would be something like:

@return torque propounder coefficient in mm

OK.

Quoted 6 lines

drivers/misc/intel_scu_ipc.c line 75

Quoted 3 lines

drivers/misc/intel_scu_ipc.c new file +3 -0

+static u32 ipc_data_readl(struct intel_scu *scu, u32 offset)
+{
+       return scu_readl(scu->base, IPC_READ_BUFFER + offset);

All of these functions take scu as a parameter. I wonder if they could take scu->regs, if you use a struct for the registers.

Maybe. I dunno.

Quoted 7 lines

drivers/misc/intel_scu_ipc.c line 80

Quoted 5 lines

drivers/misc/intel_scu_ipc.c new file +5 -0

+}
+
+static int intel_scu_ipc_check_status(struct intel_scu *scu)
+{
+       int loop_count = 3000000;

3 million loops? Does that indicate some sort of problem?

SCU is a separate microcontroller inside SoC. It might take time for it to handle the request.

3m might be be big, but 500k is a least we would wait.

Quoted 10 lines

drivers/misc/intel_scu_ipc.c line 88

Quoted 9 lines

drivers/misc/intel_scu_ipc.c new file +8 -0

+       int status;
+
+       do {
+               status = ipc_read_status(scu);
+               udelay(1);
+       } while ((status & BIT(0)) && --loop_count);
+       if (!loop_count)
+               return -ETIMEDOUT;

Quoted 3 lines

Given the long timeout, how about:

start = timer_get_us();

It makes it sleepable or what? I'm afraid of the case when it would be possible to send two sequential requests before getting an ACK from SCU.

Is it possible case when we switch to time_get_us() ?

Quoted 1 line

while (1) {

Btw, I don't like at all "while (true)" style of loops.

Quoted 14 lines

   status = ...
   if (!(status & BUSY))
      break;
  if ((int)(timer_get_us()  - start) > 3000000)
     return -ETIMEDOUT;
}

+static int intel_scu_ipc_raw_cmd(struct intel_scu *scu, u32 cmd,

Quoted 4 lines

u32 sub,
+ u8 *in, u8 inlen, u32 *out, u32
outlen,
+ u32 dptr, u32 sptr)

Quoted 2 lines

Function comment. Also try to avoid u32 for integer parameters. Use uint or ulong to avoid problems with 64-bit machines.

They are register width / hardware related, so, I would rather not change them.

Quoted 13 lines

Quoted 12 lines

+/**
+ * intel_scu_ipc_simple_command - send a simple command
+ * @cmd: command
+ * @sub: sub type
+ *
+ * Issue a simple command to the SCU. Do not use this interface if
+ * you must then access data as any data values may be overwritten
+ * by another SCU access by the time this function returns.
+ *
+ * This function may sleep. Locking for SCU accesses is handled for
+ * the caller.

Quoted 1 line

It that true?

What exactly? Locking?

This might be a copy'n'paste from kernel and might need an adjustment.

Quoted 7 lines

drivers/misc/intel_scu_ipc.c line 194

Quoted 3 lines

drivers/misc/intel_scu_ipc.c new file +3 -0

+U_BOOT_DRIVER(intel_scu_ipc) = {
+       .name           = "intel_scu_ipc",
+       .id             = UCLASS_MISC,

Consider UCLASS_SYSCON since it allows you to find the device using an enum. See 'intel_me_syscon' for example where the device can be found using X86_SYSCON_ME

OK.

Quoted 7 lines

Quoted 6 lines

drivers/misc/intel_scu_ipc.c new file +5 -0

+       .of_match       = intel_scu_match,
+       .bind           = intel_scu_bind,
+       .probe          = intel_scu_probe,
+       .priv_auto_alloc_size = sizeof(struct intel_scu),
+};

Quoted 9 lines

include/intel_scu_ipc.h line 23

Quoted 7 lines

include/intel_scu_ipc.h new file +7 -0

+#define IPC_ERR_NONE                   0
+#define IPC_ERR_CMD_NOT_SUPPORTED      1
+#define IPC_ERR_CMD_NOT_SERVICED       2
+#define IPC_ERR_UNABLE_TO_SERVICE      3
+#define IPC_ERR_CMD_INVALID            4
+#define IPC_ERR_CMD_FAILED             5
+#define IPC_ERR_EMSECURITY             6

Could be an enum

OK.

Quoted 10 lines

Quoted 10 lines

include/intel_scu_ipc.h new file +5 -0

+
+/* Command id associated with message IPCMSG_VRTC */
+#define IPC_CMD_VRTC_SETTIME      1    /* Set time */
+#define IPC_CMD_VRTC_SETALARM     2    /* Set alarm */
+#define IPC_CMD_VRTC_SYNC_RTC     3    /* Sync MSIC/PMIC RTC to

VRTC */

include/intel_scu_ipc.h new file +4 -0

+
+union ipc_ifwi_version {
+       u8 raw[16];
+       struct {

Quoted 8 lines

include/intel_scu_ipc.h line 38

Quoted 6 lines

include/intel_scu_ipc.h new file +6 -0

+               u16 ifwi_minor;
+               u8 ifwi_major;
+               u8 hardware_id;
+               u32 reserved[3];
+       } fw __attribute__((packed));
+} __attribute__((packed));

__packed.

OK.

Quoted 3 lines

The inner packed does not seem to be useful. For the outer
one, do you have multiple structs packed one after the other? If not, perhaps drop it?

I dunno. There is very little documentation available and this code as you may notice is originally written not by me or Felipe, so, let's assume the worse case.

Quoted 10 lines

Quoted 7 lines

include/intel_scu_ipc.h new file +4 -0

+
+/* Issue commands to the SCU with or without data */
+int intel_scu_ipc_simple_command(int cmd, int sub);
+int intel_scu_ipc_command(u32 cmd, u32 sub, u8 *in, u8 inlen, u32

*out, u32 outlen);

Full function comments. These functions should be passed a struct udevice for the device they use.

I have no idea how they find the right device. Can you elaborate a bit?

Quoted 6 lines

include/intel_scu_ipc.h line 55

Quoted 4 lines

include/intel_scu_ipc.h new file +4 -0

+static inline enum intel_mid_cpu_type intel_mid_identify_cpu(void)
+{
+       return INTEL_MID_CPU_CHIP_TANGIER;
+}

Is this identification a function of the SCU?

Nope. It was previously done in a completely separate header. If you think it makes sense to leave it there, we can create a file [with, for my opinion, 20+ useless lines and few lines over the code].