[U-Boot] [PATCH v1 1/2] misc: Add SCU IPC driver for Intel MID platforms
From: Felipe Balbi felipe.balbi@linux.intel.com
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 vincent.tinelli@intel.com Signed-off-by: Felipe Balbi felipe.balbi@linux.intel.com Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- 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
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index dfdd7564ea..6a747c332e 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -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 diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 1aae4bcd07..8d81f9c51c 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -167,4 +167,10 @@ config I2C_EEPROM depends on MISC help Enable a generic driver for EEPROMs attached via I2C. + +config INTEL_SCU + bool "Enable support for SCU on Intel MID platforms" + depends on INTEL_MID + default y + endmenu diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index e3151ea097..47551e44d6 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -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 diff --git a/drivers/misc/intel_scu_ipc.c b/drivers/misc/intel_scu_ipc.c new file mode 100644 index 0000000000..19a99b0b68 --- /dev/null +++ b/drivers/misc/intel_scu_ipc.c @@ -0,0 +1,199 @@ +/* + * Copyright (c) 2017 Intel Corporation + * + * SPDX-License-Identifier: GPL-2.0+ + */ +#include <common.h> +#include <dm.h> +#include <dm/device-internal.h> +#include <dm/device.h> +#include <intel_scu_ipc.h> +#include <linux/errno.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/sizes.h> + +#define IPC_STATUS_ADDR 0x04 +#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) + +struct intel_scu { + void __iomem *base; +}; + +/* Only one for now */ +static struct intel_scu *the_scu; + +static void scu_writel(void __iomem *base, unsigned int offset, unsigned int val) +{ + writel(val, base + offset); +} + +static unsigned int scu_readl(void __iomem *base, unsigned int offset) +{ + return readl(base + offset); +} + +/* + * 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)| + */ +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: + * |rfu3(8)|error code(8)|initiator id(8)|cmd id(4)|rfu1(2)|error(1)|busy(1)| + */ +static u32 ipc_read_status(struct intel_scu *scu) +{ + return scu_readl(scu->base, IPC_STATUS_ADDR); +} + +static u32 ipc_data_readl(struct intel_scu *scu, u32 offset) +{ + return scu_readl(scu->base, IPC_READ_BUFFER + offset); +} + +static int intel_scu_ipc_check_status(struct intel_scu *scu) +{ + int loop_count = 3000000; + int status; + + do { + status = ipc_read_status(scu); + udelay(1); + } while ((status & BIT(0)) && --loop_count); + if (!loop_count) + return -ETIMEDOUT; + + if (status & BIT(1)) { + printf("%s() status=0x%08x\n", __func__, status); + return -EIO; + } + + return 0; +} + +static int intel_scu_ipc_raw_cmd(struct intel_scu *scu, u32 cmd, u32 sub, + u8 *in, u8 inlen, u32 *out, u32 outlen, + u32 dptr, u32 sptr) +{ + 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; +} + +/** + * 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. + */ +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; +} + +static const struct udevice_id intel_scu_match[] = { + { .compatible = "intel,scu-ipc" }, + { /* sentinel */ } +}; + +U_BOOT_DRIVER(intel_scu_ipc) = { + .name = "intel_scu_ipc", + .id = UCLASS_MISC, + .of_match = intel_scu_match, + .bind = intel_scu_bind, + .probe = intel_scu_probe, + .priv_auto_alloc_size = sizeof(struct intel_scu), +}; diff --git a/include/intel_scu_ipc.h b/include/intel_scu_ipc.h new file mode 100644 index 0000000000..ded6352e10 --- /dev/null +++ b/include/intel_scu_ipc.h @@ -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 +#define IPCMSG_GET_FW_REVISION 0xF4 +#define IPCMSG_WATCHDOG_TIMER 0xF8 /* Set Kernel Watchdog Threshold */ + +#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 + +/* 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 */ + +union ipc_ifwi_version { + u8 raw[16]; + struct { + u16 ifwi_minor; + u8 ifwi_major; + u8 hardware_id; + u32 reserved[3]; + } fw __attribute__((packed)); +} __attribute__((packed)); + +/* 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); + +enum intel_mid_cpu_type { + INTEL_CPU_CHIP_NOTMID = 0, + INTEL_MID_CPU_CHIP_LINCROFT, + INTEL_MID_CPU_CHIP_PENWELL, + INTEL_MID_CPU_CHIP_CLOVERVIEW, + INTEL_MID_CPU_CHIP_TANGIER, +}; + +static inline enum intel_mid_cpu_type intel_mid_identify_cpu(void) +{ + return INTEL_MID_CPU_CHIP_TANGIER; +} + +#endif /* _INTEL_SCU_IPC_H_ */
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 felipe.balbi@linux.intel.com Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com --- arch/x86/Kconfig | 2 + drivers/misc/Kconfig | 6 +++ drivers/misc/Makefile | 1 + drivers/misc/intel_mid_pmu.c | 124 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 133 insertions(+) create mode 100644 drivers/misc/intel_mid_pmu.c
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 6a747c332e..04310653bb 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -83,7 +83,9 @@ endchoice # subarchitectures-specific options below config INTEL_MID bool "Intel MID platform support" + select MISC select INTEL_SCU + select INTEL_MID_PMU help Select to build a U-Boot capable of supporting Intel MID (Mobile Internet Device) platform systems which do not have diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 8d81f9c51c..0222a8beed 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -168,6 +168,12 @@ config I2C_EEPROM help Enable a generic driver for EEPROMs attached via I2C.
+config INTEL_MID_PMU + bool "Support for PMU on Intel MID platforms" + depends on MISC + depends on INTEL_MID + default y + config INTEL_SCU bool "Enable support for SCU on Intel MID platforms" depends on INTEL_MID diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index 47551e44d6..98a58f241c 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -51,4 +51,5 @@ 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_MID_PMU) += intel_mid_pmu.o obj-$(CONFIG_INTEL_SCU) += intel_scu_ipc.o diff --git a/drivers/misc/intel_mid_pmu.c b/drivers/misc/intel_mid_pmu.c new file mode 100644 index 0000000000..0eda3b4349 --- /dev/null +++ b/drivers/misc/intel_mid_pmu.c @@ -0,0 +1,124 @@ +/* + * Copyright (c) 2017 Intel Corporation + * + * SPDX-License-Identifier: GPL-2.0+ + */ +#include <common.h> +#include <dm.h> +#include <dm/device-internal.h> +#include <dm/device.h> +#include <linux/io.h> +#include <linux/sizes.h> +#include <malloc.h> + +/* Registers */ +#define PM_STS 0x00 +#define PM_CMD 0x04 +#define PM_ICS 0x08 +#define PM_WKC(x) (0x10 + (x) * 4) +#define PM_WKS(x) (0x18 + (x) * 4) +#define PM_SSC(x) (0x20 + (x) * 4) +#define PM_SSS(x) (0x30 + (x) * 4) + +/* Bits in PM_STS */ +#define PM_STS_BUSY (1 << 8) + +/* List of Intel Tangier LSSs */ +#define PMU_LSS_TANGIER_SDIO0_01 1 + +struct pmu_mid { + void __iomem *ioaddr; +}; + +static unsigned int pmu_readl(void __iomem *base, unsigned int offset) +{ + return readl(base + offset); +} + +static void pmu_writel(void __iomem *base, unsigned int offset, + unsigned int value) +{ + writel(value, base + offset); +} + +static int pmu_read_status(struct pmu_mid *pmu) +{ + int retry = 500000; + u32 reg; + + do { + reg = pmu_readl(pmu->ioaddr, PM_STS); + if (!(reg & PM_STS_BUSY)) + return 0; + + udelay(1); + } while (--retry); + + printf("WARNING: PMU still busy\n"); + return -EBUSY; +} + +static int pmu_enable_mmc(struct pmu_mid *pmu) +{ + u32 pm_ssc0; + int ret; + + /* Check PMU status */ + ret = pmu_read_status(pmu); + if (ret) + return ret; + + /* Read PMU values */ + pm_ssc0 = pmu_readl(pmu->ioaddr, PM_SSS(0)); + + /* Modify PMU values */ + + /* Enable SDIO0, sdhci for SD card */ + pm_ssc0 &= ~(0x3 << (PMU_LSS_TANGIER_SDIO0_01 * 2)); + + /* Write modified PMU values */ + pmu_writel(pmu->ioaddr, PM_SSC(0), pm_ssc0); + + /* Update modified PMU values */ + pmu_writel(pmu->ioaddr, PM_CMD, 0x00002201); + + /* Check PMU status */ + return pmu_read_status(pmu); +} + +static int pmu_mid_bind(struct udevice *dev) +{ + /* This device is needed straight away */ + return device_probe(dev); +} + +static int pmu_mid_probe(struct udevice *dev) +{ + struct pmu_mid *pmu = dev_get_uclass_priv(dev); + fdt_addr_t base; + + base = dev_get_addr(dev); + if (base == FDT_ADDR_T_NONE) + return -EINVAL; + + pmu->ioaddr = devm_ioremap(dev, base, SZ_1K); + if (!pmu->ioaddr) + return -ENOMEM; + + return pmu_enable_mmc(pmu); +} + +static const struct udevice_id pmu_mid_match[] = { + { .compatible = "intel,pmu-mid" }, + { .compatible = "intel,pmu-tangier" }, + { /* sentinel */ } +}; + +U_BOOT_DRIVER(intel_mid_pmu) = { + .name = "intel_mid_pmu", + .id = UCLASS_MISC, + .of_match = pmu_mid_match, + .bind = pmu_mid_bind, + .probe = pmu_mid_probe, + .priv_auto_alloc_size = sizeof(struct pmu_mid), +};
Hi Andy,
On 5 March 2017 at 12:17, Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
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 felipe.balbi@linux.intel.com Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
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.
Regards,
On Sun, Mar 12, 2017 at 10:21 PM, Simon Glass sjg@chromium.org wrote:
Hi Andy,
On 5 March 2017 at 12:17, Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
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 felipe.balbi@linux.intel.com Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
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.
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?
On Wed, Mar 15, 2017 at 8:01 PM, Andy Shevchenko andy.shevchenko@gmail.com wrote:
On Sun, Mar 12, 2017 at 10:21 PM, Simon Glass sjg@chromium.org wrote:
Hi Andy,
On 5 March 2017 at 12:17, Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
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 felipe.balbi@linux.intel.com Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
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.
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?
Whoof, found three mistakes - two typos in my code and an issue in sdhci generic driver.
Works now.
.Hi Andy,
On 5 March 2017 at 12:17, Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
From: Felipe Balbi felipe.balbi@linux.intel.com
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 vincent.tinelli@intel.com Signed-off-by: Felipe Balbi felipe.balbi@linux.intel.com Signed-off-by: Andy Shevchenko andriy.shevchenko@linux.intel.com
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
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index dfdd7564ea..6a747c332e 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig @@ -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 havediff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index 1aae4bcd07..8d81f9c51c 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -167,4 +167,10 @@ config I2C_EEPROM depends on MISC help Enable a generic driver for EEPROMs attached via I2C.
+config INTEL_SCU
bool "Enable support for SCU on Intel MID platforms"depends on INTEL_MIDdefault y
Please add help explaining what SCU is and stands for, and perhaps MID also.
endmenu diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index e3151ea097..47551e44d6 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -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 diff --git a/drivers/misc/intel_scu_ipc.c b/drivers/misc/intel_scu_ipc.c new file mode 100644 index 0000000000..19a99b0b68 --- /dev/null +++ b/drivers/misc/intel_scu_ipc.c @@ -0,0 +1,199 @@ +/*
- Copyright (c) 2017 Intel Corporation
- SPDX-License-Identifier: GPL-2.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 )
+#include <linux/errno.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/sizes.h>
+#define IPC_STATUS_ADDR 0x04 +#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?
+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?
+static void scu_writel(void __iomem *base, unsigned int offset, unsigned int val) +{
writel(val, base + offset);+}
+static unsigned int scu_readl(void __iomem *base, unsigned int offset) +{
return readl(base + offset);+}
+/*
- 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
- */
+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:
- |rfu3(8)|error code(8)|initiator id(8)|cmd id(4)|rfu1(2)|error(1)|busy(1)|
- */
+static u32 ipc_read_status(struct intel_scu *scu) +{
return scu_readl(scu->base, IPC_STATUS_ADDR);+}
+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.
+}
+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?
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; }
if (status & BIT(1)) {printf("%s() status=0x%08x\n", __func__, status);return -EIO;}return 0;+}
+static int intel_scu_ipc_raw_cmd(struct intel_scu *scu, u32 cmd, 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.
+{
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;+}
+/**
- 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?
- */
+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;+}
+static const struct udevice_id intel_scu_match[] = {
{ .compatible = "intel,scu-ipc" },{ /* sentinel */ }+};
+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
.of_match = intel_scu_match,.bind = intel_scu_bind,.probe = intel_scu_probe,.priv_auto_alloc_size = sizeof(struct intel_scu),+}; diff --git a/include/intel_scu_ipc.h b/include/intel_scu_ipc.h new file mode 100644 index 0000000000..ded6352e10 --- /dev/null +++ b/include/intel_scu_ipc.h @@ -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 +#define IPCMSG_GET_FW_REVISION 0xF4 +#define IPCMSG_WATCHDOG_TIMER 0xF8 /* Set Kernel Watchdog Threshold */
+#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
+/* 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 */
+union ipc_ifwi_version {
u8 raw[16];struct {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?
+/* 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.
+enum intel_mid_cpu_type {
INTEL_CPU_CHIP_NOTMID = 0,INTEL_MID_CPU_CHIP_LINCROFT,INTEL_MID_CPU_CHIP_PENWELL,INTEL_MID_CPU_CHIP_CLOVERVIEW,INTEL_MID_CPU_CHIP_TANGIER,+};
+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?
+#endif /* _INTEL_SCU_IPC_H_ */
2.11.0
Regards, Simon
On Sun, 2017-03-12 at 14:21 -0600, Simon Glass wrote:
# 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.
+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.
and perhaps MID also.
MID is explained in INTEL_MID help, though I can decode the abbreviation in the explanation of SCU.
@@ -0,0 +1,199 @@ +/*
- Copyright (c) 2017 Intel Corporation
- SPDX-License-Identifier: GPL-2.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.
+#include <linux/errno.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/sizes.h>
+#define IPC_STATUS_ADDR 0x04 +#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?
+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)?
+/*
- 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
OK.
+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.
+}
+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.
+ 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();
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() ?
while (1) {
Btw, I don't like at all "while (true)" style of loops.
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,
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.
They are register width / hardware related, so, I would rather not change them.
+/**
- 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?
What exactly? Locking?
This might be a copy'n'paste from kernel and might need an adjustment.
+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.
+ .of_match = intel_scu_match, + .bind = intel_scu_bind, + .probe = intel_scu_probe, + .priv_auto_alloc_size = sizeof(struct intel_scu), +};
+#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.
+/* 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 */
+union ipc_ifwi_version { + u8 raw[16]; + struct { + u16 ifwi_minor; + u8 ifwi_major; + u8 hardware_id; + u32 reserved[3]; + } fw __attribute__((packed)); +} __attribute__((packed));
__packed.
OK.
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.
+/* 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?
+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].
Hi Andy,
On 13 March 2017 at 08:05, Andy Shevchenko andriy.shevchenko@linux.intel.com wrote:
On Sun, 2017-03-12 at 14:21 -0600, Simon Glass wrote:
# 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 nothave
Thanks for review. My comments / answers below.
+config INTEL_SCU
bool "Enable support for SCU on Intel MID platforms"depends on INTEL_MIDdefault yPlease add help explaining what SCU is and stands for,
OK.
and perhaps MID also.
MID is explained in INTEL_MID help, though I can decode the abbreviation in the explanation of SCU.
@@ -0,0 +1,199 @@ +/*
- Copyright (c) 2017 Intel Corporation
- SPDX-License-Identifier: GPL-2.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.
+#include <linux/errno.h> +#include <linux/io.h> +#include <linux/kernel.h> +#include <linux/sizes.h>
+#define IPC_STATUS_ADDR 0x04 +#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?
Perhaps something like this?
struct ipc_regs { u32 reserved0; u32 sptr; u32 dptr; u8 reserved1[0x80 - 0x10]; u32 read[4]; u32 write[4]; };
+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)?
With your header file you can provide an enum:
enum { ROCKCHIP_SYSCON_NOC, ROCKCHIP_SYSCON_GRF, ROCKCHIP_SYSCON_SGRF, ROCKCHIP_SYSCON_PMU, ROCKCHIP_SYSCON_PMUGRF, ROCKCHIP_SYSCON_PMUSGRF, ROCKCHIP_SYSCON_CIC, };
In your driver:
static const struct udevice_id rk3288_syscon_ids[] = { { .compatible = "rockchip,rk3288-noc", .data = ROCKCHIP_SYSCON_NOC }, { .compatible = "rockchip,rk3288-grf", .data = ROCKCHIP_SYSCON_GRF }, { .compatible = "rockchip,rk3288-sgrf", .data = ROCKCHIP_SYSCON_SGRF }, { .compatible = "rockchip,rk3288-pmu", .data = ROCKCHIP_SYSCON_PMU }, { } };
U_BOOT_DRIVER(syscon_rk3288) = { .name = "rk3288_syscon", .id = UCLASS_SYSCON, .of_match = rk3288_syscon_ids, };
The device tree has nodes with the above compatible strings, each of which becomes a syscon device.
Then in the code somewhere you can call syscon_get_by_driver_data(ROCKCHIP_SYSCON_GRF..) to get access to that particular instance of the syscon_rk3288 driver.
+/*
- 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
OK.
+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.
+}
+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.
So that's at least half a second. Hopefully most systems would be fully booted by then! :-)
Anyway I guess there is nothing you can do about this.
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();
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() ?
while (1) {
Btw, I don't like at all "while (true)" style of loops.
OK, well you can perhaps adjust it to check the timeout instead the loop. But really you are running forever until a timeout or condition change.
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,
u32 sub,
u8 *in, u8 inlen, u32 *out, u32outlen,
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.
They are register width / hardware related, so, I would rather not change them.
OK well we can always deal with any issue later.
+/**
- 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?
What exactly? Locking?
Yes.
This might be a copy'n'paste from kernel and might need an adjustment.
+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.
.of_match = intel_scu_match,.bind = intel_scu_bind,.probe = intel_scu_probe,.priv_auto_alloc_size = sizeof(struct intel_scu),+};
+#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.
+/* 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 */
+union ipc_ifwi_version {
u8 raw[16];struct {u16 ifwi_minor;u8 ifwi_major;u8 hardware_id;u32 reserved[3];} fw __attribute__((packed));+} __attribute__((packed));
__packed.
OK.
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.
Well how about testing it without? It probably works fine. Having said that, on Intel CPUs there is not really much of a cost for unaligned things.
+/* 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?
+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].
Well I don't really understand what it is for, so I'll leave it to you.
Regards, Simon
participants (3)
-
Andy Shevchenko -
Andy Shevchenko -
Simon Glass