Hello everyone,
I'm presenting a revised version of the eSPI device driver patch series found at
the following link:
https://lore.kernel.org/openbmc/[email protected]/
This update addresses the issues identified during the review process.
While the previous patch series attempted to incorporate support for all four
different channels of eSPI using the ioctl interface , this new series focuses
on upstreaming the flash channel (using the mtd interface) initially, ensuring
that all review comments are duly addressed, before progressing further.
Diff between PACTH v1-v2:
1. The commit is split into multiple commits as per the review comments.
2. Explicity renamed the driver to indicate that it only support master attached
flash storage (mafs).
3. Added new kconfig options to enable/disable eSPI mafs support.
Results:
Successfully conducted a flash update via eSPI.
Manojkiran Eda (4):
Add eSPI device driver (flash channel)
mtd: Replace module_init with subsys_initcall
ARM: dts: aspeed: Add eSPI node
dt-bindings: aspeed: Add eSPI controller
.../bindings/soc/aspeed/aspeed,espi.yaml | 94 ++++
arch/arm/boot/dts/aspeed/aspeed-g5.dtsi | 19 +
arch/arm/boot/dts/aspeed/aspeed-g6.dtsi | 20 +
drivers/mtd/mtdcore.c | 2 +-
drivers/soc/aspeed/Kconfig | 38 ++
drivers/soc/aspeed/Makefile | 2 +
drivers/soc/aspeed/aspeed-espi-ctrl.c | 197 ++++++++
drivers/soc/aspeed/aspeed-espi-ctrl.h | 169 +++++++
drivers/soc/aspeed/aspeed-espi-flash-mafs.c | 467 ++++++++++++++++++
drivers/soc/aspeed/aspeed-espi-flash.h | 71 +++
10 files changed, 1078 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml
create mode 100644 drivers/soc/aspeed/aspeed-espi-ctrl.c
create mode 100644 drivers/soc/aspeed/aspeed-espi-ctrl.h
create mode 100644 drivers/soc/aspeed/aspeed-espi-flash-mafs.c
create mode 100644 drivers/soc/aspeed/aspeed-espi-flash.h
--
2.40.1
This patch adds the driver support for the eSPI controller of
Aspeed 5/6th generation SoCs. This controller is a slave device
communicating with a master over Enhanced Serial Peripheral
Interface (eSPI).
eSPI supports 4 channels, namely peripheral, virtual wire,
out-of-band, and flash, and operates at max frequency of 66MHz.
But at the moment, this patch set only supports the mafs mode
(master attached flash sharing mode) in the flash channel.
Signed-off-by: Manojkiran Eda <[email protected]>
---
drivers/soc/aspeed/Kconfig | 38 ++
drivers/soc/aspeed/Makefile | 2 +
drivers/soc/aspeed/aspeed-espi-ctrl.c | 197 +++++++++
drivers/soc/aspeed/aspeed-espi-ctrl.h | 169 +++++++
drivers/soc/aspeed/aspeed-espi-flash-mafs.c | 467 ++++++++++++++++++++
drivers/soc/aspeed/aspeed-espi-flash.h | 71 +++
6 files changed, 944 insertions(+)
create mode 100644 drivers/soc/aspeed/aspeed-espi-ctrl.c
create mode 100644 drivers/soc/aspeed/aspeed-espi-ctrl.h
create mode 100644 drivers/soc/aspeed/aspeed-espi-flash-mafs.c
create mode 100644 drivers/soc/aspeed/aspeed-espi-flash.h
diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
index f579ee0b5afa..c300ee8fe33a 100644
--- a/drivers/soc/aspeed/Kconfig
+++ b/drivers/soc/aspeed/Kconfig
@@ -52,6 +52,44 @@ config ASPEED_SOCINFO
help
Say yes to support decoding of ASPEED BMC information.
+menu "ASPEED eSPI Support"
+
+config ASPEED_ESPI
+ bool "ASPEED eSPI slave driver"
+ select REGMAP
+ select MFD_SYSCON
+ depends on ASPEED_ESPI_FLASH
+ default n
+ help
+ Enable driver support for the Aspeed eSPI engine. The eSPI engine
+ plays as a slave device in BMC to communicate with the Host over
+ the eSPI interface.
+
+menu "ASPEED eSPI Flash channel support"
+
+config ASPEED_ESPI_FLASH
+ bool "ASPEED eSPI flash channel support"
+ default n
+ depends on ASPEED_ESPI_FLASH_MAFS
+ select ASPEED_ESPI
+ help
+ Enable eSPI flash channel support.
+
+menu "ASPEED eSPI flash modes"
+
+config ASPEED_ESPI_FLASH_MAFS
+ bool "Master attached flash sharing (MAFS) support in eSPI"
+ default n
+ select ASPEED_ESPI_FLASH
+ help
+ Select this option if you have a Master attached flash connected to
+ the eSPI controller.
+
+endmenu # eSPI Flash Modes
+endmenu # eSPI Flash Channel support
+endmenu # eSPI Support
+
+
endmenu
endif
diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
index b35d74592964..cecbba700071 100644
--- a/drivers/soc/aspeed/Makefile
+++ b/drivers/soc/aspeed/Makefile
@@ -4,3 +4,5 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP) += aspeed-lpc-snoop.o
obj-$(CONFIG_ASPEED_UART_ROUTING) += aspeed-uart-routing.o
obj-$(CONFIG_ASPEED_P2A_CTRL) += aspeed-p2a-ctrl.o
obj-$(CONFIG_ASPEED_SOCINFO) += aspeed-socinfo.o
+obj-$(CONFIG_ASPEED_ESPI) += aspeed-espi-ctrl.o
+obj-$(CONFIG_ASPEED_ESPI_FLASH_MAFS) += aspeed-espi-flash-mafs.o
diff --git a/drivers/soc/aspeed/aspeed-espi-ctrl.c b/drivers/soc/aspeed/aspeed-espi-ctrl.c
new file mode 100644
index 000000000000..7e2b86849fd0
--- /dev/null
+++ b/drivers/soc/aspeed/aspeed-espi-ctrl.c
@@ -0,0 +1,197 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2024 Aspeed Technology Inc.
+ */
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/miscdevice.h>
+#include <linux/dma-mapping.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+#include <linux/uaccess.h>
+#include <linux/vmalloc.h>
+
+#include "aspeed-espi-ctrl.h"
+#include "aspeed-espi-flash.h"
+
+/**
+ * aspeed_espi_ctrl_isr - function to handle various interrupts
+ * @irq: interrupt line
+ * @arg: pointer to access device registers
+ *
+ * Returns IRQ_HANDLED
+ */
+static irqreturn_t aspeed_espi_ctrl_isr(int irq, void *arg)
+{
+ uint32_t sts;
+ struct aspeed_espi_ctrl *espi_ctrl = (struct aspeed_espi_ctrl *)arg;
+
+ regmap_read(espi_ctrl->map, ESPI_INT_STS, &sts);
+
+ if (sts & ESPI_INT_STS_FLASH_BITS) {
+ aspeed_espi_flash_event(sts, espi_ctrl->flash);
+ regmap_write(espi_ctrl->map, ESPI_INT_STS,
+ sts & ESPI_INT_STS_FLASH_BITS);
+ }
+
+ if (sts & ESPI_INT_STS_HW_RST_DEASSERT) {
+ aspeed_espi_flash_enable(espi_ctrl->flash);
+
+ regmap_write(espi_ctrl->map, ESPI_SYSEVT_INT_T0, 0x0);
+ regmap_write(espi_ctrl->map, ESPI_SYSEVT_INT_T1, 0x0);
+ regmap_write(espi_ctrl->map, ESPI_SYSEVT_INT_EN, 0xffffffff);
+
+ regmap_write(espi_ctrl->map, ESPI_SYSEVT1_INT_T0, 0x1);
+ regmap_write(espi_ctrl->map, ESPI_SYSEVT1_INT_EN, 0x1);
+
+ regmap_update_bits(espi_ctrl->map, ESPI_INT_EN,
+ ESPI_INT_EN_HW_RST_DEASSERT,
+ ESPI_INT_EN_HW_RST_DEASSERT);
+
+ regmap_update_bits(
+ espi_ctrl->map, ESPI_SYSEVT,
+ ESPI_SYSEVT_SLV_BOOT_STS | ESPI_SYSEVT_SLV_BOOT_DONE,
+ ESPI_SYSEVT_SLV_BOOT_STS | ESPI_SYSEVT_SLV_BOOT_DONE);
+
+ regmap_write(espi_ctrl->map, ESPI_INT_STS,
+ ESPI_INT_STS_HW_RST_DEASSERT);
+ }
+
+ return IRQ_HANDLED;
+}
+
+/**
+ * aspeed_espi_ctrl_probe - function to probe the platform driver
+ * @pdev: platform device
+ *
+ * Returns 0 on success, -ENOMEM on error
+ */
+static int aspeed_espi_ctrl_probe(struct platform_device *pdev)
+{
+ int rc = 0;
+ struct aspeed_espi_ctrl *espi_ctrl;
+ struct device *dev = &pdev->dev;
+
+ espi_ctrl = devm_kzalloc(dev, sizeof(*espi_ctrl), GFP_KERNEL);
+ if (!espi_ctrl)
+ return -ENOMEM;
+
+ espi_ctrl->model = of_device_get_match_data(dev);
+
+ espi_ctrl->map = syscon_node_to_regmap(dev->parent->of_node);
+ if (IS_ERR(espi_ctrl->map)) {
+ dev_err(dev, "cannot get remap\n");
+ return PTR_ERR(espi_ctrl->map);
+ }
+
+ espi_ctrl->irq = platform_get_irq(pdev, 0);
+ if (espi_ctrl->irq < 0)
+ return espi_ctrl->irq;
+
+ espi_ctrl->clk = devm_clk_get(dev, NULL);
+ if (IS_ERR(espi_ctrl->clk)) {
+ dev_err(dev, "cannot get clock\n");
+ return PTR_ERR(espi_ctrl->clk);
+ }
+
+ rc = clk_prepare_enable(espi_ctrl->clk);
+ if (rc) {
+ dev_err(dev, "cannot enable clock\n");
+ return rc;
+ }
+
+ /*
+ * This takes care of deferred probe , incase the mtd core
+ * subsystem is not probed yet.
+ */
+ espi_ctrl->flash = aspeed_espi_flash_alloc(dev, espi_ctrl);
+ if (IS_ERR(espi_ctrl->flash)) {
+ dev_err(dev, "failed to allocate flash channel\n");
+ pr_info("flash alloc failed with return code %ld\n",
+ PTR_ERR(espi_ctrl->flash));
+ return PTR_ERR(espi_ctrl->flash);
+ }
+
+ regmap_write(espi_ctrl->map, ESPI_SYSEVT_INT_T0, 0x0);
+ regmap_write(espi_ctrl->map, ESPI_SYSEVT_INT_T1, 0x0);
+ regmap_write(espi_ctrl->map, ESPI_SYSEVT_INT_EN, 0xffffffff);
+
+ regmap_write(espi_ctrl->map, ESPI_SYSEVT1_INT_T0, 0x1);
+ regmap_write(espi_ctrl->map, ESPI_SYSEVT1_INT_EN, 0x1);
+
+ rc = devm_request_irq(dev, espi_ctrl->irq, aspeed_espi_ctrl_isr, 0,
+ DEVICE_NAME, espi_ctrl);
+ if (rc) {
+ dev_err(dev, "failed to request IRQ\n");
+ return rc;
+ }
+
+ // clear the interrupt enable register
+ regmap_write(espi_ctrl->map, ESPI_INT_EN_CLR, 0x7fffffff);
+
+ // Disable the interrupts in all channels except flash channel
+ regmap_update_bits(espi_ctrl->map, ESPI_INT_EN,
+ ESPI_INT_EN_FLASH_BITS | ESPI_INT_EN_HW_RST_DEASSERT,
+ ESPI_INT_EN_FLASH_BITS |
+ ESPI_INT_STS_HW_RST_DEASSERT);
+
+ dev_set_drvdata(dev, espi_ctrl);
+
+ dev_info(dev, "module loaded\n");
+
+ return 0;
+}
+
+/**
+ * aspeed_espi_ctrl_remove - Release the driver
+ * @pdev: the platform device
+ *
+ * Returns 0
+ */
+static int aspeed_espi_ctrl_remove(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct aspeed_espi_ctrl *espi_ctrl = dev_get_drvdata(dev);
+
+ aspeed_espi_flash_free(dev, espi_ctrl->flash);
+
+ return 0;
+}
+
+static const struct aspeed_espi_model ast2500_model = {
+ .version = ESPI_AST2500,
+};
+
+static const struct aspeed_espi_model ast2600_model = {
+ .version = ESPI_AST2600,
+};
+
+static const struct of_device_id aspeed_espi_ctrl_of_matches[] = {
+ { .compatible = "aspeed,ast2500-espi-ctrl", .data = &ast2500_model },
+ { .compatible = "aspeed,ast2600-espi-ctrl", .data = &ast2600_model },
+ {},
+};
+MODULE_DEVICE_TABLE(of, aspeed_espi_ctrl_of_matches);
+
+static struct platform_driver aspeed_espi_ctrl_driver = {
+ .driver = {
+ .name = DEVICE_NAME,
+ .of_match_table = aspeed_espi_ctrl_of_matches,
+ },
+ .probe = aspeed_espi_ctrl_probe,
+ .remove = aspeed_espi_ctrl_remove,
+};
+
+module_platform_driver(aspeed_espi_ctrl_driver);
+
+MODULE_AUTHOR("Manojkiran Eda <[email protected]>");
+MODULE_AUTHOR("Patrick Rudolph <[email protected]>");
+MODULE_AUTHOR("Chia-Wei Wang <[email protected]>");
+MODULE_AUTHOR("Ryan Chen <[email protected]>");
+MODULE_DESCRIPTION("Control of Aspeed eSPI Slave Device");
+MODULE_LICENSE("GPL");
diff --git a/drivers/soc/aspeed/aspeed-espi-ctrl.h b/drivers/soc/aspeed/aspeed-espi-ctrl.h
new file mode 100644
index 000000000000..5c4950c84b01
--- /dev/null
+++ b/drivers/soc/aspeed/aspeed-espi-ctrl.h
@@ -0,0 +1,169 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright 2024 Aspeed Technology Inc.
+ */
+#ifndef _ASPEED_ESPI_CTRL_H_
+#define _ASPEED_ESPI_CTRL_H_
+
+#include <linux/bits.h>
+
+#define DEVICE_NAME "aspeed-espi-ctrl"
+
+/**
+ * enum aspeed_espi_version - Enumeration to capture aspeed versions that this
+ * driver supports
+ * @ESPI_AST2500: eSPI for AST2500 Model
+ * @ESPI_AST2600: eSPI for AST2600 Model
+ *
+ * This enumeration captures various aspeed models that this driver supports
+ */
+
+enum aspeed_espi_version {
+ ESPI_AST2500,
+ ESPI_AST2600,
+};
+
+/**
+ * struct aspeed_espi_model - structure to capture version of espi
+ * @version: espi version
+ *
+ * Structure to capture version of eSPI
+ */
+struct aspeed_espi_model {
+ uint32_t version;
+};
+
+/**
+ * struct aspeed_espi_ctrl - structure to group various channels of the driver
+ * @dev: platform device pointer
+ * @map: register map for the device
+ * @clk: clock for the device
+ * @irq: interrupt request
+ * @flash: pointer to flash channel of eSPI
+ * @model: pointer to version of eSPI
+ *
+ * Structure to capture version of eSPI
+ */
+struct aspeed_espi_ctrl {
+ struct device *dev;
+ struct regmap *map;
+ struct clk *clk;
+ int irq;
+ struct aspeed_espi_flash *flash;
+ const struct aspeed_espi_model *model;
+};
+
+/* eSPI register offset */
+#define ESPI_CTRL 0x000
+#define ESPI_CTRL_FLASH_TX_DMA_EN BIT(23)
+#define ESPI_CTRL_FLASH_RX_DMA_EN BIT(22)
+#define ESPI_CTRL_FLASH_SW_MODE_MASK GENMASK(11, 10)
+#define ESPI_CTRL_FLASH_SW_MODE_SHIFT 10
+#define ESPI_CTRL_FLASH_SW_RDY BIT(7)
+
+#define ESPI_STS 0x004
+#define ESPI_CTRL_FLASH_CHAN_RDY BIT(6)
+
+#define ESPI_INT_STS 0x008
+#define ESPI_INT_STS_HW_RST_DEASSERT BIT(31)
+#define ESPI_INT_STS_FLASH_TX_ERR BIT(21)
+#define ESPI_INT_STS_FLASH_TX_ABT BIT(19)
+#define ESPI_INT_STS_FLASH_RX_ABT BIT(15)
+#define ESPI_INT_STS_FLASH_TX_CMPLT BIT(7)
+#define ESPI_INT_STS_FLASH_RX_CMPLT BIT(6)
+#define ESPI_INT_EN 0x00c
+#define ESPI_INT_EN_HW_RST_DEASSERT BIT(31)
+#define ESPI_INT_EN_FLASH_TX_ERR BIT(21)
+#define ESPI_INT_EN_FLASH_TX_ABT BIT(19)
+#define ESPI_INT_EN_FLASH_RX_ABT BIT(15)
+#define ESPI_INT_EN_FLASH_TX_CMPLT BIT(7)
+#define ESPI_INT_EN_FLASH_RX_CMPLT BIT(6)
+#define ESPI_INT_EN_CLR 0x0fc
+#define ESPI_FLASH_RX_DMA 0x060
+#define ESPI_FLASH_RX_CTRL 0x064
+#define ESPI_FLASH_RX_CTRL_PEND_SERV BIT(31)
+#define ESPI_FLASH_RX_CTRL_LEN_MASK GENMASK(23, 12)
+#define ESPI_FLASH_RX_CTRL_LEN_SHIFT 12
+#define ESPI_FLASH_RX_CTRL_TAG_MASK GENMASK(11, 8)
+#define ESPI_FLASH_RX_CTRL_TAG_SHIFT 8
+#define ESPI_FLASH_RX_CTRL_CYC_MASK GENMASK(7, 0)
+#define ESPI_FLASH_RX_CTRL_CYC_SHIFT 0
+#define ESPI_FLASH_RX_PORT 0x068
+#define ESPI_FLASH_TX_DMA 0x070
+#define ESPI_FLASH_TX_CTRL 0x074
+#define ESPI_FLASH_TX_CTRL_TRIGGER BIT(31)
+#define ESPI_FLASH_TX_CTRL_LEN_MASK GENMASK(23, 12)
+#define ESPI_FLASH_TX_CTRL_LEN_SHIFT 12
+#define ESPI_FLASH_TX_CTRL_TAG_MASK GENMASK(11, 8)
+#define ESPI_FLASH_TX_CTRL_TAG_SHIFT 8
+#define ESPI_FLASH_TX_CTRL_CYC_MASK GENMASK(7, 0)
+#define ESPI_FLASH_TX_CTRL_CYC_SHIFT 0
+#define ESPI_FLASH_TX_PORT 0x078
+#define ESPI_CTRL2 0x080
+#define ESPI_CTRL2_MEMCYC_RD_DIS BIT(6)
+#define ESPI_CTRL2_MEMCYC_WR_DIS BIT(4)
+#define ESPI_SYSEVT_INT_EN 0x094
+#define ESPI_SYSEVT 0x098
+#define ESPI_SYSEVT_HOST_RST_ACK BIT(27)
+#define ESPI_SYSEVT_RST_CPU_INIT BIT(26)
+#define ESPI_SYSEVT_SLV_BOOT_STS BIT(23)
+#define ESPI_SYSEVT_NON_FATAL_ERR BIT(22)
+#define ESPI_SYSEVT_FATAL_ERR BIT(21)
+#define ESPI_SYSEVT_SLV_BOOT_DONE BIT(20)
+#define ESPI_SYSEVT_NMI_OUT BIT(10)
+#define ESPI_SYSEVT_SMI_OUT BIT(9)
+#define ESPI_SYSEVT_HOST_RST_WARN BIT(8)
+#define ESPI_SYSEVT_PLTRSTN BIT(5)
+#define ESPI_SYSEVT_SUSPEND BIT(4)
+#define ESPI_SYSEVT_S5_SLEEP BIT(2)
+#define ESPI_SYSEVT_S4_SLEEP BIT(1)
+#define ESPI_SYSEVT_S3_SLEEP BIT(0)
+#define ESPI_GEN_CAP_N_CONF 0x0a0
+#define ESPI_CH0_CAP_N_CONF 0x0a4
+#define ESPI_CH1_CAP_N_CONF 0x0a8
+#define ESPI_CH2_CAP_N_CONF 0x0ac
+#define ESPI_CH3_CAP_N_CONF 0x0b0
+#define ESPI_CH3_CAP_N_CONF_ERASE_MASK GENMASK(4, 2)
+#define ESPI_CH3_CAP_N_CONF_ERASE_SHIFT 2
+#define ESPI_CH3_CAP_N_CONF_ERASE_SIZE_4KB 1
+#define ESPI_CH3_CAP_N_CONF_ERASE_SIZE_64KB 2
+#define ESPI_CH3_CAP_N_CONF_ERASE_SIZE_4KB_64KB 3
+#define ESPI_CH3_CAP_N_CONF_ERASE_SIZE_128KB 4
+#define ESPI_CH3_CAP_N_CONF_ERASE_SIZE_256KB 5
+#define ESPI_CH3_CAP_N_CONF2 0x0b4
+#define ESPI_SYSEVT1_INT_EN 0x100
+#define ESPI_SYSEVT1 0x104
+#define ESPI_SYSEVT1_SUSPEND_ACK BIT(20)
+#define ESPI_SYSEVT1_SUSPEND_WARN BIT(0)
+#define ESPI_SYSEVT_INT_T0 0x110
+#define ESPI_SYSEVT_INT_T1 0x114
+#define ESPI_SYSEVT_INT_T2 0x118
+#define ESPI_SYSEVT_INT_T2_HOST_RST_WARN ESPI_SYSEVT_HOST_RST_WARN
+#define ESPI_SYSEVT_INT_STS 0x11c
+#define ESPI_SYSEVT_INT_STS_NMI_OUT ESPI_SYSEVT_NMI_OUT
+#define ESPI_SYSEVT_INT_STS_SMI_OUT ESPI_SYSEVT_SMI_OUT
+#define ESPI_SYSEVT_INT_STS_HOST_RST_WARN ESPI_SYSEVT_HOST_RST_WARN
+#define ESPI_SYSEVT_INT_STS_PLTRSTN ESPI_SYSEVT_PLTRSTN
+#define ESPI_SYSEVT_INT_STS_SUSPEND ESPI_SYSEVT_SUSPEND
+#define ESPI_SYSEVT_INT_STS_S5_SLEEP ESPI_SYSEVT_INT_S5_SLEEP
+#define ESPI_SYSEVT_INT_STS_S4_SLEEP ESPI_SYSEVT_INT_S4_SLEEP
+#define ESPI_SYSEVT_INT_STS_S3_SLEEP ESPI_SYSEVT_INT_S3_SLEEP
+#define ESPI_SYSEVT1_INT_T0 0x120
+#define ESPI_SYSEVT1_INT_T1 0x124
+#define ESPI_SYSEVT1_INT_T2 0x128
+#define ESPI_SYSEVT1_INT_STS 0x12c
+#define ESPI_SYSEVT1_INT_STS_SUSPEND_WARN ESPI_SYSEVT1_SUSPEND_WARN
+
+/* collect ESPI_INT_STS bits of eSPI channels for convenience */
+#define ESPI_INT_STS_FLASH_BITS \
+ (ESPI_INT_STS_FLASH_TX_ERR | ESPI_INT_STS_FLASH_TX_ABT | \
+ ESPI_INT_STS_FLASH_RX_ABT | ESPI_INT_STS_FLASH_TX_CMPLT | \
+ ESPI_INT_STS_FLASH_RX_CMPLT)
+
+/* collect ESPI_INT_EN bits of eSPI channels for convenience */
+#define ESPI_INT_EN_FLASH_BITS \
+ (ESPI_INT_EN_FLASH_TX_ERR | ESPI_INT_EN_FLASH_TX_ABT | \
+ ESPI_INT_EN_FLASH_RX_ABT | ESPI_INT_EN_FLASH_TX_CMPLT | \
+ ESPI_INT_EN_FLASH_RX_CMPLT)
+
+#endif
diff --git a/drivers/soc/aspeed/aspeed-espi-flash-mafs.c b/drivers/soc/aspeed/aspeed-espi-flash-mafs.c
new file mode 100644
index 000000000000..ac5d55833109
--- /dev/null
+++ b/drivers/soc/aspeed/aspeed-espi-flash-mafs.c
@@ -0,0 +1,467 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyrigt 2024 Aspeed Technology Inc.
+ */
+#include <linux/fs.h>
+#include <linux/of_device.h>
+#include <linux/of_address.h>
+#include <linux/miscdevice.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+#include <linux/uaccess.h>
+#include <linux/vmalloc.h>
+#include <linux/miscdevice.h>
+#include <linux/dma-mapping.h>
+
+#include "aspeed-espi-ctrl.h"
+#include "aspeed-espi-flash.h"
+
+static long aspeed_espi_flash_rx(uint8_t *cyc, uint16_t *len, uint32_t *addr,
+ uint8_t *pkt, size_t *pkt_len,
+ struct aspeed_espi_flash *espi_flash)
+{
+ unsigned long flags;
+ uint32_t reg, rx_pkt_len, addr_be;
+ struct aspeed_espi_ctrl *espi_ctrl = espi_flash->ctrl;
+ void *rx_virt_ptr;
+ ulong to;
+ int ret;
+
+ spin_lock_irqsave(&espi_flash->spinlock, flags);
+
+ to = msecs_to_jiffies(100);
+ ret = wait_event_interruptible_lock_irq_timeout(
+ espi_flash->wq, espi_flash->rx_sts, espi_flash->spinlock, to);
+
+ spin_unlock_irqrestore(&espi_flash->spinlock, flags);
+
+ if (ret == -ERESTARTSYS)
+ return -EINTR;
+ else if (!ret)
+ return -ETIMEDOUT;
+ else if (espi_flash->rx_sts & ESPI_INT_STS_FLASH_RX_ABT)
+ return -EFAULT;
+ else
+ ret = 0;
+
+ /* common header (i.e. cycle type, tag, and length) is taken by HW */
+ regmap_read(espi_ctrl->map, ESPI_FLASH_RX_CTRL, ®);
+ *cyc = (reg & ESPI_FLASH_RX_CTRL_CYC_MASK) >>
+ ESPI_FLASH_RX_CTRL_CYC_SHIFT;
+ *len = (reg & ESPI_FLASH_RX_CTRL_LEN_MASK) >>
+ ESPI_FLASH_RX_CTRL_LEN_SHIFT;
+
+ /*
+ * calculate the length of the rest part of the
+ * eSPI packet to be read from HW and copied to
+ * user space.
+ */
+ switch (*cyc) {
+ case ESPI_FLASH_READ:
+ case ESPI_FLASH_WRITE:
+ case ESPI_FLASH_ERASE:
+ rx_virt_ptr = espi_flash->dma.rx_virt + sizeof(addr_be);
+ rx_pkt_len = (*len) ? *len : ESPI_PLD_LEN_MAX;
+
+ if (addr) {
+ memcpy(&addr_be, espi_flash->dma.rx_virt,
+ sizeof(addr_be));
+ *addr = ntohl(addr_be);
+ }
+
+ break;
+ case ESPI_FLASH_SUC_CMPLT_D_MIDDLE:
+ case ESPI_FLASH_SUC_CMPLT_D_FIRST:
+ case ESPI_FLASH_SUC_CMPLT_D_LAST:
+ case ESPI_FLASH_SUC_CMPLT_D_ONLY:
+ rx_pkt_len = (*len) ? *len : ESPI_PLD_LEN_MAX;
+ rx_virt_ptr = espi_flash->dma.rx_virt;
+ break;
+ case ESPI_FLASH_SUC_CMPLT:
+ case ESPI_FLASH_UNSUC_CMPLT:
+ case ESPI_FLASH_UNSUC_CMPLT_ONLY:
+ /* No data received */
+ rx_pkt_len = 0;
+ rx_virt_ptr = NULL;
+ break;
+ default:
+ rx_pkt_len = 0;
+ ret = -EFAULT;
+ }
+ if (pkt_len)
+ *pkt_len = rx_pkt_len;
+
+ if (rx_pkt_len && pkt_len && pkt) {
+ if (*pkt_len >= rx_pkt_len)
+ memcpy(pkt, rx_virt_ptr, rx_pkt_len);
+ else
+ ret = -EINVAL;
+ }
+
+ spin_lock_irqsave(&espi_flash->spinlock, flags);
+
+ regmap_write_bits(espi_ctrl->map, ESPI_FLASH_RX_CTRL,
+ ESPI_FLASH_RX_CTRL_PEND_SERV,
+ ESPI_FLASH_RX_CTRL_PEND_SERV);
+
+ espi_flash->rx_sts = 0;
+
+ spin_unlock_irqrestore(&espi_flash->spinlock, flags);
+ return ret;
+}
+
+static long
+aspeed_espi_flash_rx_get_completion(struct aspeed_espi_flash *espi_flash)
+{
+ uint8_t cyc;
+ uint16_t len;
+ int ret;
+
+ ret = aspeed_espi_flash_rx(&cyc, &len, NULL, NULL, 0, espi_flash);
+ if (ret)
+ return ret;
+
+ if (cyc != ESPI_FLASH_SUC_CMPLT) {
+ dev_notice(espi_flash->ctrl->dev, "eSPI Rx response was not successful\n");
+ return -EFAULT;
+ }
+ return 0;
+}
+
+static long aspeed_espi_flash_put_tx(uint8_t cyc, uint16_t len, uint32_t addr,
+ const uint8_t *pkt, size_t pkt_len,
+ struct aspeed_espi_flash *espi_flash)
+{
+ struct aspeed_espi_ctrl *espi_ctrl = espi_flash->ctrl;
+ unsigned long flags;
+ uint32_t reg, addr_be;
+ ulong to;
+ int ret = 0;
+
+ /* Test if flash channel is ready */
+ regmap_read(espi_ctrl->map, ESPI_STS, ®);
+ if (!(reg & ESPI_CTRL_FLASH_CHAN_RDY)) {
+ dev_notice(espi_flash->ctrl->dev, "eSPI flash channel not ready\n");
+ return -EIO;
+ }
+
+ /* Test if TX ready */
+ regmap_read(espi_ctrl->map, ESPI_FLASH_TX_CTRL, ®);
+ if (reg & ESPI_FLASH_TX_CTRL_TRIGGER) {
+ dev_notice(espi_flash->ctrl->dev, "eSPI Tx operation still pending\n");
+ return -EBUSY;
+ }
+
+ /*
+ * common header (i.e. cycle type, tag, and length)
+ * part is written to HW registers
+ */
+ addr_be = htonl(addr);
+ memcpy(espi_flash->dma.tx_virt, &addr_be, sizeof(addr_be));
+ if (pkt && pkt_len)
+ memcpy(espi_flash->dma.tx_virt + sizeof(addr_be), pkt, pkt_len);
+
+ dma_wmb();
+
+ spin_lock_irqsave(&espi_flash->spinlock, flags);
+ espi_flash->tx_sts = 0;
+ espi_flash->rx_sts = 0;
+
+ reg = ((cyc << ESPI_FLASH_TX_CTRL_CYC_SHIFT) &
+ ESPI_FLASH_TX_CTRL_CYC_MASK) |
+ ((len << ESPI_FLASH_TX_CTRL_LEN_SHIFT) &
+ ESPI_FLASH_TX_CTRL_LEN_MASK) |
+ ESPI_FLASH_TX_CTRL_TRIGGER;
+
+ regmap_write(espi_ctrl->map, ESPI_FLASH_TX_CTRL, reg);
+
+ to = msecs_to_jiffies(100);
+ ret = wait_event_interruptible_lock_irq_timeout(
+ espi_flash->wq, espi_flash->tx_sts, espi_flash->spinlock, to);
+ if (ret == -ERESTARTSYS)
+ ret = -EINTR;
+ else if (!ret) {
+ dev_notice(espi_flash->ctrl->dev, "eSPI Tx operation not processed within 100msec\n");
+ ret = -ETIMEDOUT;
+ } else if (espi_flash->tx_sts &
+ (ESPI_INT_STS_FLASH_TX_ERR | ESPI_INT_STS_FLASH_TX_ABT)) {
+ dev_notice(espi_flash->ctrl->dev, "eSPI Tx operation declined by remote\n");
+
+ ret = -EFAULT;
+ } else
+ ret = 0;
+
+ spin_unlock_irqrestore(&espi_flash->spinlock, flags);
+ return ret;
+}
+
+void aspeed_espi_flash_event(uint32_t sts, struct aspeed_espi_flash *espi_flash)
+{
+ unsigned long flags;
+
+ if (!(sts & ESPI_INT_STS_FLASH_BITS))
+ return;
+
+ spin_lock_irqsave(&espi_flash->spinlock, flags);
+ espi_flash->rx_sts |=
+ sts & (ESPI_INT_STS_FLASH_RX_ABT | ESPI_INT_STS_FLASH_RX_CMPLT);
+ espi_flash->tx_sts |=
+ sts & (ESPI_INT_STS_FLASH_TX_ERR | ESPI_INT_STS_FLASH_TX_ABT |
+ ESPI_INT_STS_FLASH_TX_CMPLT);
+ spin_unlock_irqrestore(&espi_flash->spinlock, flags);
+ wake_up_interruptible(&espi_flash->wq);
+}
+
+static int aspeed_espi_flash_erase(struct mtd_info *mtd,
+ struct erase_info *instr)
+{
+ struct aspeed_espi_flash *espi_flash = mtd->priv;
+ int ret = 0;
+
+ /* Sanity checks */
+ if ((uint32_t)instr->len % espi_flash->mtd.erasesize)
+ return -EINVAL;
+
+ if ((uint32_t)instr->addr % espi_flash->mtd.erasesize)
+ return -EINVAL;
+
+ mutex_lock(&espi_flash->lock);
+
+ while (instr->len) {
+ ret = aspeed_espi_flash_put_tx(ESPI_FLASH_ERASE,
+ espi_flash->erase_mask,
+ instr->addr, NULL, 0,
+ espi_flash);
+ if (ret)
+ goto unlock_mtx_n_out;
+
+ ret = aspeed_espi_flash_rx_get_completion(espi_flash);
+ if (ret)
+ goto unlock_mtx_n_out;
+
+ instr->len -= espi_flash->mtd.erasesize;
+ instr->addr += espi_flash->mtd.erasesize;
+ }
+
+unlock_mtx_n_out:
+ instr->fail_addr = instr->addr;
+
+ mutex_unlock(&espi_flash->lock);
+ return ret;
+}
+
+static int aspeed_espi_flash_read(struct mtd_info *mtd, loff_t from, size_t len,
+ size_t *retlen, u_char *buf)
+{
+ struct aspeed_espi_flash *espi_flash = mtd->priv;
+ uint16_t len_pt, len_rx;
+ size_t pkt_len;
+ uint8_t cyc;
+ int ret = 0;
+
+ mutex_lock(&espi_flash->lock);
+
+ while (len) {
+ len_pt = (len > ESPI_PLD_LEN_MIN) ? ESPI_PLD_LEN_MIN : len;
+
+ ret = aspeed_espi_flash_put_tx(ESPI_FLASH_READ, len_pt, from,
+ NULL, 0, espi_flash);
+ if (ret)
+ goto unlock_mtx_n_out;
+
+ pkt_len = len_pt;
+ len_rx = 0;
+ cyc = 0;
+ ret = aspeed_espi_flash_rx(&cyc, &len_rx, NULL, buf, &pkt_len,
+ espi_flash);
+ if (ret)
+ goto unlock_mtx_n_out;
+
+ if (cyc != ESPI_FLASH_SUC_CMPLT_D_ONLY) {
+ dev_notice(espi_flash->ctrl->dev, "eSPI Rx response was not successful\n");
+ ret = -EFAULT;
+ goto unlock_mtx_n_out;
+ }
+ if (pkt_len != len_pt) {
+ dev_notice(espi_flash->ctrl->dev,
+ "eSPI Rx response has unexpected data length\n");
+ ret = -ENODATA;
+ goto unlock_mtx_n_out;
+ }
+
+ len -= len_pt;
+ buf += len_pt;
+ from += len_pt;
+ *retlen += len_pt;
+ }
+
+unlock_mtx_n_out:
+ mutex_unlock(&espi_flash->lock);
+ return ret;
+}
+
+static int aspeed_espi_flash_write(struct mtd_info *mtd, loff_t to, size_t len,
+ size_t *retlen, const u_char *buf)
+{
+ struct aspeed_espi_flash *espi_flash = mtd->priv;
+ uint16_t len_pt;
+ int ret = 0;
+
+ mutex_lock(&espi_flash->lock);
+
+ while (len) {
+ len_pt = (len > ESPI_PLD_LEN_MIN) ? ESPI_PLD_LEN_MIN : len;
+
+ ret = aspeed_espi_flash_put_tx(ESPI_FLASH_WRITE, len_pt, to,
+ buf, len_pt, espi_flash);
+ if (ret)
+ goto unlock_mtx_n_out;
+
+ ret = aspeed_espi_flash_rx_get_completion(espi_flash);
+ if (ret)
+ goto unlock_mtx_n_out;
+
+ len -= len_pt;
+ buf += len_pt;
+ to += len_pt;
+ *retlen += len_pt;
+ }
+
+unlock_mtx_n_out:
+ mutex_unlock(&espi_flash->lock);
+ return ret;
+}
+
+void aspeed_espi_flash_enable(struct aspeed_espi_flash *espi_flash)
+{
+ struct aspeed_espi_flash_dma *dma = &espi_flash->dma;
+ struct aspeed_espi_ctrl *espi_ctrl = espi_flash->ctrl;
+
+ regmap_update_bits(espi_ctrl->map, ESPI_CTRL,
+ ESPI_CTRL_FLASH_SW_MODE_MASK, 0);
+
+ /* Enable DMA transfers */
+ regmap_write(espi_ctrl->map, ESPI_FLASH_TX_DMA, dma->tx_addr);
+ regmap_write(espi_ctrl->map, ESPI_FLASH_RX_DMA, dma->rx_addr);
+ regmap_update_bits(
+ espi_ctrl->map, ESPI_CTRL,
+ ESPI_CTRL_FLASH_TX_DMA_EN | ESPI_CTRL_FLASH_RX_DMA_EN,
+ ESPI_CTRL_FLASH_TX_DMA_EN | ESPI_CTRL_FLASH_RX_DMA_EN);
+
+ /* Enable interrupts */
+ regmap_write(espi_ctrl->map, ESPI_INT_STS, ESPI_INT_STS_FLASH_BITS);
+
+ regmap_update_bits(espi_ctrl->map, ESPI_INT_EN, ESPI_INT_EN_FLASH_BITS,
+ ESPI_INT_EN_FLASH_BITS);
+
+ /* Set Flash Channel Software Ready */
+ regmap_update_bits(espi_ctrl->map, ESPI_CTRL, ESPI_CTRL_FLASH_SW_RDY,
+ ESPI_CTRL_FLASH_SW_RDY);
+}
+
+void *aspeed_espi_flash_alloc(struct device *dev,
+ struct aspeed_espi_ctrl *espi_ctrl)
+{
+ int ret, index;
+ struct aspeed_espi_flash_dma *dma;
+ struct mtd_info *mtd;
+ struct aspeed_espi_flash *espi_flash;
+ struct resource res;
+ u32 reg;
+
+ espi_flash =
+ devm_kzalloc(dev, sizeof(struct aspeed_espi_flash), GFP_KERNEL);
+ if (!espi_flash)
+ return ERR_PTR(-ENOMEM);
+
+ espi_flash->ctrl = espi_ctrl;
+
+ /* Bus lock */
+ mutex_init(&espi_flash->lock);
+
+ init_waitqueue_head(&espi_flash->wq);
+
+ spin_lock_init(&espi_flash->spinlock);
+
+ dma = &espi_flash->dma;
+
+ dma->tx_virt =
+ dma_alloc_coherent(dev, PAGE_SIZE, &dma->tx_addr, GFP_KERNEL);
+ if (!dma->tx_virt) {
+ dev_err(dev, "cannot allocate DMA TX buffer\n");
+ return ERR_PTR(-ENOMEM);
+ }
+
+ dma->rx_virt =
+ dma_alloc_coherent(dev, PAGE_SIZE, &dma->rx_addr, GFP_KERNEL);
+ if (!dma->rx_virt) {
+ dev_err(dev, "cannot allocate DMA RX buffer\n");
+ return ERR_PTR(-ENOMEM);
+ }
+ index = of_property_match_string(dev->of_node, "reg-names",
+ "espi_flash");
+ ret = of_address_to_resource(dev->of_node, index, &res);
+ if (ret < 0) {
+ dev_err(dev,
+ "Could not find espi_flash resource block size in devtree\n");
+ return ERR_PTR(-ENODEV);
+ }
+ reg = resource_size(&res);
+ mtd = &espi_flash->mtd;
+ mtd->dev.parent = dev;
+ mtd->size = reg;
+ mtd->flags = MTD_CAP_NORFLASH;
+ mtd->_erase = aspeed_espi_flash_erase;
+ mtd->_read = aspeed_espi_flash_read;
+ mtd->_write = aspeed_espi_flash_write;
+ mtd->type = MTD_NORFLASH;
+ mtd->name = "espi-flash-mafs";
+
+ regmap_read(espi_ctrl->map, ESPI_CH3_CAP_N_CONF, ®);
+ reg = (reg & ESPI_CH3_CAP_N_CONF_ERASE_MASK) >>
+ ESPI_CH3_CAP_N_CONF_ERASE_SHIFT;
+ espi_flash->erase_mask = reg;
+ switch (reg) {
+ case ESPI_CH3_CAP_N_CONF_ERASE_SIZE_4KB:
+ case ESPI_CH3_CAP_N_CONF_ERASE_SIZE_4KB_64KB:
+ mtd->erasesize = 0x1000;
+ espi_flash->erase_mask = 1;
+ break;
+ case ESPI_CH3_CAP_N_CONF_ERASE_SIZE_64KB:
+ mtd->erasesize = 0x10000;
+ break;
+ case ESPI_CH3_CAP_N_CONF_ERASE_SIZE_128KB:
+ mtd->erasesize = 0x20000;
+ break;
+ case ESPI_CH3_CAP_N_CONF_ERASE_SIZE_256KB:
+ mtd->erasesize = 0x40000;
+ break;
+ default:
+ dev_notice(dev, "Unknown erase size %x\n", reg);
+ return ERR_PTR(-ENODEV);
+ }
+
+ mtd->writesize = 1;
+ mtd->owner = THIS_MODULE;
+ mtd->priv = espi_flash;
+
+ ret = mtd_device_register(mtd, NULL, 0);
+ if (ret) {
+ dev_notice(dev, "aspeed-espi-mtd: Failed to register mtd device\n");
+ return ERR_PTR(ret);
+ }
+
+ aspeed_espi_flash_enable(espi_flash);
+ return espi_flash;
+}
+
+void aspeed_espi_flash_free(struct device *dev,
+ struct aspeed_espi_flash *espi_flash)
+{
+ struct aspeed_espi_flash_dma *dma = &espi_flash->dma;
+
+ dma_free_coherent(dev, PAGE_SIZE, dma->tx_virt, dma->tx_addr);
+ dma_free_coherent(dev, PAGE_SIZE, dma->rx_virt, dma->rx_addr);
+
+ mtd_device_unregister(&espi_flash->mtd);
+}
diff --git a/drivers/soc/aspeed/aspeed-espi-flash.h b/drivers/soc/aspeed/aspeed-espi-flash.h
new file mode 100644
index 000000000000..08d0a79796e2
--- /dev/null
+++ b/drivers/soc/aspeed/aspeed-espi-flash.h
@@ -0,0 +1,71 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright 2024 Aspeed Technology Inc.
+ */
+#ifndef _ASPEED_ESPI_FLASH_H_
+#define _ASPEED_ESPI_FLASH_H_
+
+#include <linux/mtd/mtd.h>
+
+/*
+ * eSPI cycle type encoding
+ *
+ * Section 5.1 Cycle Types and Packet Format,
+ * Intel eSPI Interface Base Specification, Rev 1.0, Jan. 2016.
+ */
+#define ESPI_FLASH_READ 0x00
+#define ESPI_FLASH_WRITE 0x01
+#define ESPI_FLASH_ERASE 0x02
+#define ESPI_FLASH_SUC_CMPLT 0x06
+#define ESPI_FLASH_SUC_CMPLT_D_MIDDLE 0x09
+#define ESPI_FLASH_SUC_CMPLT_D_FIRST 0x0b
+#define ESPI_FLASH_SUC_CMPLT_D_LAST 0x0d
+#define ESPI_FLASH_SUC_CMPLT_D_ONLY 0x0f
+#define ESPI_FLASH_UNSUC_CMPLT 0x0c
+#define ESPI_FLASH_UNSUC_CMPLT_ONLY 0x0e
+
+/*
+ * we choose the longest header and the max payload size
+ * based on the Intel specification to define the maximum
+ * eSPI packet length
+ */
+#define ESPI_PLD_LEN_MIN (1UL << 6)
+#define ESPI_PLD_LEN_MAX (1UL << 12)
+
+
+enum aspeed_espi_flash_safs_mode {
+ SAFS_MODE_MIX,
+ SAFS_MODE_SW,
+ SAFS_MODE_HW,
+ SAFS_MODES,
+};
+
+struct aspeed_espi_flash_dma {
+ void *tx_virt;
+ dma_addr_t tx_addr;
+ void *rx_virt;
+ dma_addr_t rx_addr;
+};
+
+struct aspeed_espi_flash {
+
+ unsigned short page_offset; /* offset in flash address */
+ unsigned int page_size; /* of bytes per page */
+ struct mutex lock;
+ struct aspeed_espi_flash_dma dma;
+ struct mtd_info mtd;
+ struct aspeed_espi_ctrl *ctrl;
+ uint8_t erase_mask;
+ uint32_t tx_sts;
+ uint32_t rx_sts;
+ wait_queue_head_t wq;
+ spinlock_t spinlock;
+};
+
+void aspeed_espi_flash_event(uint32_t sts, struct aspeed_espi_flash *espi_flash);
+void aspeed_espi_flash_enable(struct aspeed_espi_flash *espi_flash);
+void *aspeed_espi_flash_alloc(struct device *dev, struct aspeed_espi_ctrl *espi_ctrl);
+void aspeed_espi_flash_free(struct device *dev, struct aspeed_espi_flash *espi_flash);
+
+#endif
+
--
2.40.1
While engaged in development on the espi kernel device driver[1],
I noticed that the espi flash driver, utilizing the mtd subsystem,
appears to initialize before the mtdcore subsystem registers the
mtd_class. As a result, although the mtd device for espi is created,
it does not populate within the /sys/class/mtd hierarchy.
Given that mtd serves as a subsystem upon which numerous other drivers
rely for infrastructure, it appears logical to adjust the module_init()
call to an alternative priority initcall, subsys_initcall(), thereby
ensuring that the mtd core subsystem is probed prior to the drivers
utilizing its infrastructure.
Although this adjustment alters the initialization ordering, there
exists a slight risk of uncovering implicit initialization ordering
issues. However, I believe it is preferable to prioritize it reasonably
rather than having module_init() in order to maintain the exact old
ordering.
Link : [1] https://lore.kernel.org/openbmc/[email protected]
Signed-off-by: Manojkiran Eda <[email protected]>
---
drivers/mtd/mtdcore.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index e451b28840d5..cc51c9fb2c1e 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -2560,7 +2560,7 @@ static void __exit cleanup_mtd(void)
idr_destroy(&mtd_idr);
}
-module_init(init_mtd);
+subsys_initcall(init_mtd);
module_exit(cleanup_mtd);
MODULE_LICENSE("GPL");
--
2.40.1
This commit adds eSPI to the device tree for aspeed 5/6th
generation SoCs.
Signed-off-by: Manojkiran Eda <[email protected]>
---
arch/arm/boot/dts/aspeed/aspeed-g5.dtsi | 19 +++++++++++++++++++
arch/arm/boot/dts/aspeed/aspeed-g6.dtsi | 20 ++++++++++++++++++++
2 files changed, 39 insertions(+)
diff --git a/arch/arm/boot/dts/aspeed/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g5.dtsi
index 04f98d1dbb97..eaf7d82b6f46 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g5.dtsi
@@ -343,6 +343,25 @@ sdhci1: sdhci@200 {
status = "disabled";
};
};
+ espi: espi@1e6ee000 {
+ compatible = "aspeed,ast2500-espi", "simple-mfd", "syscon";
+ reg = <0x1e6ee000 0x1000>;
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x1e6ee000 0x1000>;
+
+ espi_ctrl: espi-ctrl@0 {
+ compatible = "aspeed,ast2500-espi-ctrl";
+ reg = <0x0 0x800>,<0x0 0x4000000>;
+ reg-names = "espi_ctrl","espi_flash";
+ interrupts = <23>;
+ clocks = <&syscon ASPEED_CLK_GATE_ESPICLK>;
+ status = "disabled";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_espi_default>;
+ };
+ };
gpio: gpio@1e780000 {
#gpio-cells = <2>;
diff --git a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
index c4d1faade8be..094e14442101 100644
--- a/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed/aspeed-g6.dtsi
@@ -454,6 +454,26 @@ video: video@1e700000 {
status = "disabled";
};
+ espi: espi@1e6ee000 {
+ compatible = "aspeed,ast2500-espi", "simple-mfd", "syscon";
+ reg = <0x1e6ee000 0x1000>;
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x1e6ee000 0x1000>;
+
+ espi_ctrl: espi-ctrl@0 {
+ compatible = "aspeed,ast2500-espi-ctrl";
+ reg = <0x0 0x800>,<0x0 0x4000000>;
+ reg-names = "espi_ctrl","espi_flash";
+ interrupts = <23>;
+ clocks = <&syscon ASPEED_CLK_GATE_ESPICLK>;
+ status = "disabled";
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_espi_default>;
+ };
+ };
+
gpio0: gpio@1e780000 {
#gpio-cells = <2>;
gpio-controller;
--
2.40.1
This commit adds the device tree bindings for aspeed eSPI
controller.
Although aspeed eSPI hardware supports 4 different channels,
this commit only adds the support for flash channel, the
bindings for other channels could be upstreamed when the driver
support for those are added.
Signed-off-by: Manojkiran Eda <[email protected]>
---
.../bindings/soc/aspeed/aspeed,espi.yaml | 94 +++++++++++++++++++
1 file changed, 94 insertions(+)
create mode 100644 Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml
diff --git a/Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml b/Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml
new file mode 100644
index 000000000000..3d3ad528e3b3
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml
@@ -0,0 +1,94 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# # Copyright (c) 2024 IBM Corporation.
+# # Copyright (c) 2021 Aspeed Technology Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/soc/aspeed/aspeed,espi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Aspeed eSPI Controller
+
+maintainers:
+ - Manojkiran Eda <[email protected]>
+ - Patrick Rudolph <[email protected]>
+ - Chia-Wei Wang <[email protected]>
+ - Ryan Chen <[email protected]>
+
+description:
+ Aspeed eSPI controller implements a device side eSPI endpoint device
+ supporting the flash channel.
+
+properties:
+ compatible:
+ items:
+ - enum:
+ - aspeed,ast2500-espi
+ - aspeed,ast2600-espi
+ - const: simple-mfd
+ - const: syscon
+
+ reg:
+ maxItems: 1
+
+ "#address-cells":
+ const: 1
+
+ "#size-cells":
+ const: 1
+
+ ranges: true
+
+patternProperties:
+ "^espi-ctrl@[0-9a-f]+$":
+ type: object
+
+ description: Controls the flash channel of eSPI hardware
+
+ properties:
+ compatible:
+ items:
+ - enum:
+ - aspeed,ast2500-espi-ctrl
+ - aspeed,ast2600-espi-ctrl
+
+ interrupts:
+ maxItems: 1
+
+ clocks:
+ maxItems: 1
+
+ required:
+ - compatible
+ - interrupts
+ - clocks
+
+required:
+ - compatible
+ - reg
+ - "#address-cells"
+ - "#size-cells"
+ - ranges
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/clock/ast2600-clock.h>
+
+ espi: espi@1e6ee000 {
+ compatible = "aspeed,ast2600-espi", "simple-mfd", "syscon";
+ reg = <0x1e6ee000 0x1000>;
+
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges = <0x0 0x1e6ee000 0x1000>;
+
+ espi_ctrl: espi-ctrl@0 {
+ compatible = "aspeed,ast2600-espi-ctrl";
+ reg = <0x0 0x800>,<0x0 0x4000000>;
+ reg-names = "espi_ctrl","espi_flash";
+ interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&syscon ASPEED_CLK_GATE_ESPICLK>;
+ };
+ };
--
2.40.1
On 19/03/2024 10:34, Manojkiran Eda wrote:
> This patch adds the driver support for the eSPI controller of
> Aspeed 5/6th generation SoCs. This controller is a slave device
> communicating with a master over Enhanced Serial Peripheral
> Interface (eSPI).
>
> eSPI supports 4 channels, namely peripheral, virtual wire,
> out-of-band, and flash, and operates at max frequency of 66MHz.
>
> But at the moment, this patch set only supports the mafs mode
> (master attached flash sharing mode) in the flash channel.
>
> Signed-off-by: Manojkiran Eda <[email protected]>
> ---
> drivers/soc/aspeed/Kconfig | 38 ++
> drivers/soc/aspeed/Makefile | 2 +
> drivers/soc/aspeed/aspeed-espi-ctrl.c | 197 +++++++++
> drivers/soc/aspeed/aspeed-espi-ctrl.h | 169 +++++++
> drivers/soc/aspeed/aspeed-espi-flash-mafs.c | 467 ++++++++++++++++++++
> drivers/soc/aspeed/aspeed-espi-flash.h | 71 +++
> 6 files changed, 944 insertions(+)
> create mode 100644 drivers/soc/aspeed/aspeed-espi-ctrl.c
> create mode 100644 drivers/soc/aspeed/aspeed-espi-ctrl.h
> create mode 100644 drivers/soc/aspeed/aspeed-espi-flash-mafs.c
> create mode 100644 drivers/soc/aspeed/aspeed-espi-flash.h
>
> diff --git a/drivers/soc/aspeed/Kconfig b/drivers/soc/aspeed/Kconfig
> index f579ee0b5afa..c300ee8fe33a 100644
> --- a/drivers/soc/aspeed/Kconfig
> +++ b/drivers/soc/aspeed/Kconfig
> @@ -52,6 +52,44 @@ config ASPEED_SOCINFO
> help
> Say yes to support decoding of ASPEED BMC information.
>
> +menu "ASPEED eSPI Support"
> +
> +config ASPEED_ESPI
> + bool "ASPEED eSPI slave driver"
Why this is not tristate?
> + select REGMAP
> + select MFD_SYSCON
> + depends on ASPEED_ESPI_FLASH
> + default n
> + help
> + Enable driver support for the Aspeed eSPI engine. The eSPI engine
> + plays as a slave device in BMC to communicate with the Host over
> + the eSPI interface.
> +
> +menu "ASPEED eSPI Flash channel support"
You have way too many menus...
> +
> +config ASPEED_ESPI_FLASH
This is not used, drop.
> + bool "ASPEED eSPI flash channel support"
> + default n
> + depends on ASPEED_ESPI_FLASH_MAFS
> + select ASPEED_ESPI
> + help
> + Enable eSPI flash channel support.
> +
> +menu "ASPEED eSPI flash modes"
> +
> +config ASPEED_ESPI_FLASH_MAFS
> + bool "Master attached flash sharing (MAFS) support in eSPI"
Why this is not tristate?
> + default n
> + select ASPEED_ESPI_FLASH
> + help
> + Select this option if you have a Master attached flash connected to
> + the eSPI controller.
> +
> +endmenu # eSPI Flash Modes
> +endmenu # eSPI Flash Channel support
> +endmenu # eSPI Support
> +
> +
> endmenu
>
> endif
> diff --git a/drivers/soc/aspeed/Makefile b/drivers/soc/aspeed/Makefile
> index b35d74592964..cecbba700071 100644
> --- a/drivers/soc/aspeed/Makefile
> +++ b/drivers/soc/aspeed/Makefile
> @@ -4,3 +4,5 @@ obj-$(CONFIG_ASPEED_LPC_SNOOP) += aspeed-lpc-snoop.o
> obj-$(CONFIG_ASPEED_UART_ROUTING) += aspeed-uart-routing.o
> obj-$(CONFIG_ASPEED_P2A_CTRL) += aspeed-p2a-ctrl.o
> obj-$(CONFIG_ASPEED_SOCINFO) += aspeed-socinfo.o
> +obj-$(CONFIG_ASPEED_ESPI) += aspeed-espi-ctrl.o
> +obj-$(CONFIG_ASPEED_ESPI_FLASH_MAFS) += aspeed-espi-flash-mafs.o
Why did you put spi drivers in soc? SPI drivers usually go to spi, don't
they?
> diff --git a/drivers/soc/aspeed/aspeed-espi-ctrl.c b/drivers/soc/aspeed/aspeed-espi-ctrl.c
> new file mode 100644
> index 000000000000..7e2b86849fd0
> --- /dev/null
> +++ b/drivers/soc/aspeed/aspeed-espi-ctrl.c
> @@ -0,0 +1,197 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2024 Aspeed Technology Inc.
> + */
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/clk.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/platform_device.h>
> +#include <linux/miscdevice.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/regmap.h>
> +#include <linux/uaccess.h>
> +#include <linux/vmalloc.h>
> +
> +#include "aspeed-espi-ctrl.h"
> +#include "aspeed-espi-flash.h"
> +
> +/**
No need for kerneldoc for private functions.
> + * aspeed_espi_ctrl_isr - function to handle various interrupts
> + * @irq: interrupt line
> + * @arg: pointer to access device registers
> + *
> + * Returns IRQ_HANDLED
> + */
> +static irqreturn_t aspeed_espi_ctrl_isr(int irq, void *arg)
> +{
> + uint32_t sts;
> + struct aspeed_espi_ctrl *espi_ctrl = (struct aspeed_espi_ctrl *)arg;
> +
> + regmap_read(espi_ctrl->map, ESPI_INT_STS, &sts);
> +
> + if (sts & ESPI_INT_STS_FLASH_BITS) {
> + aspeed_espi_flash_event(sts, espi_ctrl->flash);
> + regmap_write(espi_ctrl->map, ESPI_INT_STS,
> + sts & ESPI_INT_STS_FLASH_BITS);
> + }
> +
> + if (sts & ESPI_INT_STS_HW_RST_DEASSERT) {
> + aspeed_espi_flash_enable(espi_ctrl->flash);
> +
> + regmap_write(espi_ctrl->map, ESPI_SYSEVT_INT_T0, 0x0);
> + regmap_write(espi_ctrl->map, ESPI_SYSEVT_INT_T1, 0x0);
> + regmap_write(espi_ctrl->map, ESPI_SYSEVT_INT_EN, 0xffffffff);
> +
> + regmap_write(espi_ctrl->map, ESPI_SYSEVT1_INT_T0, 0x1);
> + regmap_write(espi_ctrl->map, ESPI_SYSEVT1_INT_EN, 0x1);
> +
> + regmap_update_bits(espi_ctrl->map, ESPI_INT_EN,
> + ESPI_INT_EN_HW_RST_DEASSERT,
> + ESPI_INT_EN_HW_RST_DEASSERT);
> +
> + regmap_update_bits(
> + espi_ctrl->map, ESPI_SYSEVT,
> + ESPI_SYSEVT_SLV_BOOT_STS | ESPI_SYSEVT_SLV_BOOT_DONE,
> + ESPI_SYSEVT_SLV_BOOT_STS | ESPI_SYSEVT_SLV_BOOT_DONE);
> +
> + regmap_write(espi_ctrl->map, ESPI_INT_STS,
> + ESPI_INT_STS_HW_RST_DEASSERT);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +/**
> + * aspeed_espi_ctrl_probe - function to probe the platform driver
> + * @pdev: platform device
> + *
> + * Returns 0 on success, -ENOMEM on error
Drop useless kerneldoc / entire function comment. That's just probe...
> + */
> +static int aspeed_espi_ctrl_probe(struct platform_device *pdev)
> +{
> + int rc = 0;
> + struct aspeed_espi_ctrl *espi_ctrl;
> + struct device *dev = &pdev->dev;
> +
> + espi_ctrl = devm_kzalloc(dev, sizeof(*espi_ctrl), GFP_KERNEL);
> + if (!espi_ctrl)
> + return -ENOMEM;
> +
> + espi_ctrl->model = of_device_get_match_data(dev);
> +
> + espi_ctrl->map = syscon_node_to_regmap(dev->parent->of_node);
> + if (IS_ERR(espi_ctrl->map)) {
> + dev_err(dev, "cannot get remap\n");
> + return PTR_ERR(espi_ctrl->map);
return dev_err_probe
> + }
> +
> + espi_ctrl->irq = platform_get_irq(pdev, 0);
> + if (espi_ctrl->irq < 0)
> + return espi_ctrl->irq;
> +
> + espi_ctrl->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(espi_ctrl->clk)) {
> + dev_err(dev, "cannot get clock\n");
> + return PTR_ERR(espi_ctrl->clk);
return dev_err_probe
> + }
> +
> + rc = clk_prepare_enable(espi_ctrl->clk);
> + if (rc) {
> + dev_err(dev, "cannot enable clock\n");
> + return rc;
> + }
> +
> + /*
> + * This takes care of deferred probe , incase the mtd core
> + * subsystem is not probed yet.
> + */
> + espi_ctrl->flash = aspeed_espi_flash_alloc(dev, espi_ctrl);
> + if (IS_ERR(espi_ctrl->flash)) {
> + dev_err(dev, "failed to allocate flash channel\n");
> + pr_info("flash alloc failed with return code %ld\n",
> + PTR_ERR(espi_ctrl->flash));
Please clean up the code. Drop.
> + return PTR_ERR(espi_ctrl->flash);
> + }
> +
> + regmap_write(espi_ctrl->map, ESPI_SYSEVT_INT_T0, 0x0);
> + regmap_write(espi_ctrl->map, ESPI_SYSEVT_INT_T1, 0x0);
> + regmap_write(espi_ctrl->map, ESPI_SYSEVT_INT_EN, 0xffffffff);
> +
> + regmap_write(espi_ctrl->map, ESPI_SYSEVT1_INT_T0, 0x1);
> + regmap_write(espi_ctrl->map, ESPI_SYSEVT1_INT_EN, 0x1);
> +
> + rc = devm_request_irq(dev, espi_ctrl->irq, aspeed_espi_ctrl_isr, 0,
> + DEVICE_NAME, espi_ctrl);
> + if (rc) {
> + dev_err(dev, "failed to request IRQ\n");
> + return rc;
> + }
> +
> + // clear the interrupt enable register
> + regmap_write(espi_ctrl->map, ESPI_INT_EN_CLR, 0x7fffffff);
> +
> + // Disable the interrupts in all channels except flash channel
> + regmap_update_bits(espi_ctrl->map, ESPI_INT_EN,
> + ESPI_INT_EN_FLASH_BITS | ESPI_INT_EN_HW_RST_DEASSERT,
> + ESPI_INT_EN_FLASH_BITS |
> + ESPI_INT_STS_HW_RST_DEASSERT);
Why do you first request interrupt and then disable them? What if it
fires earlier?
> +
> + dev_set_drvdata(dev, espi_ctrl);
> +
> + dev_info(dev, "module loaded\n");
No, drop such simple function success statements.
> +
> + return 0;
> +}
> +
> +/**
> + * aspeed_espi_ctrl_remove - Release the driver
> + * @pdev: the platform device
> + *
> + * Returns 0
> + */
Drop entire comment, useless.
..
> +
> +void *aspeed_espi_flash_alloc(struct device *dev,
> + struct aspeed_espi_ctrl *espi_ctrl)
> +{
> + int ret, index;
> + struct aspeed_espi_flash_dma *dma;
> + struct mtd_info *mtd;
> + struct aspeed_espi_flash *espi_flash;
> + struct resource res;
> + u32 reg;
> +
> + espi_flash =
> + devm_kzalloc(dev, sizeof(struct aspeed_espi_flash), GFP_KERNEL);
sizeof(*)
> + if (!espi_flash)
> + return ERR_PTR(-ENOMEM);
> +
> + espi_flash->ctrl = espi_ctrl;
> +
> + /* Bus lock */
> + mutex_init(&espi_flash->lock);
> +
> + init_waitqueue_head(&espi_flash->wq);
> +
> + spin_lock_init(&espi_flash->spinlock);
> +
> + dma = &espi_flash->dma;
> +
> + dma->tx_virt =
> + dma_alloc_coherent(dev, PAGE_SIZE, &dma->tx_addr, GFP_KERNEL);
Wrong wrapping.
> + if (!dma->tx_virt) {
> + dev_err(dev, "cannot allocate DMA TX buffer\n");
Do not print anything on allocation failures.
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + dma->rx_virt =
> + dma_alloc_coherent(dev, PAGE_SIZE, &dma->rx_addr, GFP_KERNEL);
> + if (!dma->rx_virt) {
> + dev_err(dev, "cannot allocate DMA RX buffer\n");
Drop
> + return ERR_PTR(-ENOMEM);
> + }
> + index = of_property_match_string(dev->of_node, "reg-names",
> + "espi_flash");
> + ret = of_address_to_resource(dev->of_node, index, &res);
Why such unusual code? Why you cannot just map the io space?
> + if (ret < 0) {
> + dev_err(dev,
> + "Could not find espi_flash resource block size in devtree\n");
> + return ERR_PTR(-ENODEV);
> + }
> + reg = resource_size(&res);
> + mtd = &espi_flash->mtd;
> + mtd->dev.parent = dev;
> + mtd->size = reg;
> + mtd->flags = MTD_CAP_NORFLASH;
> + mtd->_erase = aspeed_espi_flash_erase;
> + mtd->_read = aspeed_espi_flash_read;
> + mtd->_write = aspeed_espi_flash_write;
> + mtd->type = MTD_NORFLASH;
> + mtd->name = "espi-flash-mafs";
> +
> + regmap_read(espi_ctrl->map, ESPI_CH3_CAP_N_CONF, ®);
> + reg = (reg & ESPI_CH3_CAP_N_CONF_ERASE_MASK) >>
> + ESPI_CH3_CAP_N_CONF_ERASE_SHIFT;
> + espi_flash->erase_mask = reg;
> + switch (reg) {
> + case ESPI_CH3_CAP_N_CONF_ERASE_SIZE_4KB:
> + case ESPI_CH3_CAP_N_CONF_ERASE_SIZE_4KB_64KB:
> + mtd->erasesize = 0x1000;
> + espi_flash->erase_mask = 1;
> + break;
> + case ESPI_CH3_CAP_N_CONF_ERASE_SIZE_64KB:
> + mtd->erasesize = 0x10000;
> + break;
> + case ESPI_CH3_CAP_N_CONF_ERASE_SIZE_128KB:
> + mtd->erasesize = 0x20000;
> + break;
> + case ESPI_CH3_CAP_N_CONF_ERASE_SIZE_256KB:
> + mtd->erasesize = 0x40000;
> + break;
> + default:
> + dev_notice(dev, "Unknown erase size %x\n", reg);
> + return ERR_PTR(-ENODEV);
> + }
> +
> + mtd->writesize = 1;
> + mtd->owner = THIS_MODULE;
> + mtd->priv = espi_flash;
> +
> + ret = mtd_device_register(mtd, NULL, 0);
> + if (ret) {
> + dev_notice(dev, "aspeed-espi-mtd: Failed to register mtd device\n");
> + return ERR_PTR(ret);
> + }
> +
> + aspeed_espi_flash_enable(espi_flash);
> + return espi_flash;
> +}
Missing export as GPL
> +
> +void aspeed_espi_flash_free(struct device *dev,
> + struct aspeed_espi_flash *espi_flash)
> +{
> + struct aspeed_espi_flash_dma *dma = &espi_flash->dma;
> +
> + dma_free_coherent(dev, PAGE_SIZE, dma->tx_virt, dma->tx_addr);
> + dma_free_coherent(dev, PAGE_SIZE, dma->rx_virt, dma->rx_addr);
> +
> + mtd_device_unregister(&espi_flash->mtd);
> +}
Missing export as GPL
Best regards,
Krzysztof
On 19/03/2024 10:34, Manojkiran Eda wrote:
> This commit adds eSPI to the device tree for aspeed 5/6th
> generation SoCs.
>
> Signed-off-by: Manojkiran Eda <[email protected]>
> ---
> arch/arm/boot/dts/aspeed/aspeed-g5.dtsi | 19 +++++++++++++++++++
> arch/arm/boot/dts/aspeed/aspeed-g6.dtsi | 20 ++++++++++++++++++++
> 2 files changed, 39 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed/aspeed-g5.dtsi
> index 04f98d1dbb97..eaf7d82b6f46 100644
> --- a/arch/arm/boot/dts/aspeed/aspeed-g5.dtsi
> +++ b/arch/arm/boot/dts/aspeed/aspeed-g5.dtsi
> @@ -343,6 +343,25 @@ sdhci1: sdhci@200 {
> status = "disabled";
> };
> };
> + espi: espi@1e6ee000 {
spi or syscon
Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> + compatible = "aspeed,ast2500-espi", "simple-mfd", "syscon";
> + reg = <0x1e6ee000 0x1000>;
> +
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0x0 0x1e6ee000 0x1000>;
> +
> + espi_ctrl: espi-ctrl@0 {
What is this device? If parent is espi, then what is this?
Where is the binding?
Best regards,
Krzysztof
On 19/03/2024 10:34, Manojkiran Eda wrote:
> While engaged in development on the espi kernel device driver[1],
> I noticed that the espi flash driver, utilizing the mtd subsystem,
> appears to initialize before the mtdcore subsystem registers the
NAK
You incorrectly ordered your call, so now to fix this you incorrectly
re-order rest of kernel. No. Fix your code to handle modules, probe
deferrals and device links.
Best regards,
Krzysztof
Hi,
[email protected] wrote on Tue, 19 Mar 2024 10:51:00 +0100:
> On 19/03/2024 10:34, Manojkiran Eda wrote:
> > While engaged in development on the espi kernel device driver[1],
> > I noticed that the espi flash driver, utilizing the mtd subsystem,
> > appears to initialize before the mtdcore subsystem registers the
>
> NAK
>
> You incorrectly ordered your call, so now to fix this you incorrectly
> re-order rest of kernel. No. Fix your code to handle modules, probe
> deferrals and device links.
Agreed. You shall not need this. Maybe just moving the driver to the
right location (spi) might fix it.
Thanks,
Miquèl
On 19/03/2024 10:34, Manojkiran Eda wrote:
> This commit adds the device tree bindings for aspeed eSPI
> controller.
>
> Although aspeed eSPI hardware supports 4 different channels,
> this commit only adds the support for flash channel, the
> bindings for other channels could be upstreamed when the driver
> support for those are added.
>
> Signed-off-by: Manojkiran Eda <[email protected]>
> ---
> .../bindings/soc/aspeed/aspeed,espi.yaml | 94 +++++++++++++++++++
> 1 file changed, 94 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml
>
> diff --git a/Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml b/Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml
> new file mode 100644
> index 000000000000..3d3ad528e3b3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml
Why Rob's comments got ignored?
This is not a soc component.
> @@ -0,0 +1,94 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# # Copyright (c) 2024 IBM Corporation.
> +# # Copyright (c) 2021 Aspeed Technology Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/soc/aspeed/aspeed,espi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Aspeed eSPI Controller
> +
> +maintainers:
> + - Manojkiran Eda <[email protected]>
> + - Patrick Rudolph <[email protected]>
> + - Chia-Wei Wang <[email protected]>
> + - Ryan Chen <[email protected]>
> +
> +description:
> + Aspeed eSPI controller implements a device side eSPI endpoint device
> + supporting the flash channel.
Explain what is eSPI.
> +
> +properties:
> + compatible:
> + items:
> + - enum:
> + - aspeed,ast2500-espi
> + - aspeed,ast2600-espi
> + - const: simple-mfd
That's not simple-mfd. You have driver for this. Drop.
> + - const: syscon
That's not syscon. Why do you have ranges then? Where is any explanation
of hardware which would justify such combination?
> +
> + reg:
> + maxItems: 1
> +
> + "#address-cells":
> + const: 1
> +
> + "#size-cells":
> + const: 1
> +
> + ranges: true
> +
> +patternProperties:
> + "^espi-ctrl@[0-9a-f]+$":
> + type: object
> +
> + description: Controls the flash channel of eSPI hardware
That explains nothing. Unless you wanted to use here MTD bindings.
This binding did not improve much. I don't understand why this is not
SPI (nothing in commit msg, nothing in description), what is eSPI, why
do you need child device, what are other children (commit msg is quite
vague here). Why there is no MTD bindings here?
All this looks like crafted for your driver, instead of using existing
DT bindings like SPI or MTD/NAND. This is a strong no-go.
> +
> + properties:
> + compatible:
> + items:
No items, just use enum.
> + - enum:
> + - aspeed,ast2500-espi-ctrl
> + - aspeed,ast2600-espi-ctrl
> +
> + interrupts:
> + maxItems: 1
> +
> + clocks:
> + maxItems: 1
> +
> + required:
> + - compatible
> + - interrupts
> + - clocks
> +
> +required:
> + - compatible
> + - reg
> + - "#address-cells"
> + - "#size-cells"
> + - ranges
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/arm-gic.h>
> + #include <dt-bindings/clock/ast2600-clock.h>
> +
> + espi: espi@1e6ee000 {
> + compatible = "aspeed,ast2600-espi", "simple-mfd", "syscon";
> + reg = <0x1e6ee000 0x1000>;
> +
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0x0 0x1e6ee000 0x1000>;
> +
> + espi_ctrl: espi-ctrl@0 {
> + compatible = "aspeed,ast2600-espi-ctrl";
> + reg = <0x0 0x800>,<0x0 0x4000000>;
Fix your style in DTS. There is always a space after ','.
> + reg-names = "espi_ctrl","espi_flash";
> + interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&syscon ASPEED_CLK_GATE_ESPICLK>;
> + };
> + };
Best regards,
Krzysztof
On 20/03/2024 10:59, Manojkiran Eda wrote:
>
> On 19/03/24 3:26 pm, Krzysztof Kozlowski wrote:
>> On 19/03/2024 10:34, Manojkiran Eda wrote:
>>> This commit adds the device tree bindings for aspeed eSPI
>>> controller.
>>>
>>> Although aspeed eSPI hardware supports 4 different channels,
>>> this commit only adds the support for flash channel, the
>>> bindings for other channels could be upstreamed when the driver
>>> support for those are added.
>>>
>>> Signed-off-by: Manojkiran Eda<[email protected]>
>>> ---
>>> .../bindings/soc/aspeed/aspeed,espi.yaml | 94 +++++++++++++++++++
>>> 1 file changed, 94 insertions(+)
>>> create mode 100644 Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml b/Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml
>>> new file mode 100644
>>> index 000000000000..3d3ad528e3b3
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml
>> Why Rob's comments got ignored?
>>
>> This is not a soc component.
> I did not mean to ignore, i have few reasons listed below that provides
> information on why i felt this belongs into soc.
soc is dumping ground of things which are purely SoC specific, not
covered by existing hardware structure in bindings. Maybe indeed this
does not have any other place, but did you actually look?
Anyway, please CC SPI maintainers on future submission.
>>
>>> @@ -0,0 +1,94 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +# # Copyright (c) 2024 IBM Corporation.
>>> +# # Copyright (c) 2021 Aspeed Technology Inc.
>>> +%YAML 1.2
>>> +---
>>> +$id:http://devicetree.org/schemas/soc/aspeed/aspeed,espi.yaml#
>>> +$schema:http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Aspeed eSPI Controller
>>> +
>>> +maintainers:
>>> + - Manojkiran Eda<[email protected]>
>>> + - Patrick Rudolph<[email protected]>
>>> + - Chia-Wei Wang<[email protected]>
>>> + - Ryan Chen<[email protected]>
>>> +
>>> +description:
>>> + Aspeed eSPI controller implements a device side eSPI endpoint device
>>> + supporting the flash channel.
>> Explain what is eSPI.
> eSPI is a serial bus interface for client and server platforms that is
Explain in description of the hardware.
> based on SPI, using the same master and slave topology but operates
> with a different protocol to meet new requirements. For instance, eSPI
> uses I/O, or input/output, communication, instead of MOSI/MISO used in
> SPI. It also includes a transaction layer on top of the SPI protocol,
> defining packets such as command and response packets that allow both
> the master and slave to initiate alert and reset signals. eSPI supports
> communication between Embedded Controller (EC), Baseboard Management
> Controller (BMC), Super-I/O (SIO) and Port-80 debug cards. I could add
> this to the commit message as well in the next patchset.
>>
>>> +
>>> +properties:
>>> + compatible:
>>> + items:
>>> + - enum:
>>> + - aspeed,ast2500-espi
>>> + - aspeed,ast2600-espi
>>> + - const: simple-mfd
>>
>> That's not simple-mfd. You have driver for this. Drop.
>>
>>> + - const: syscon
>> That's not syscon. Why do you have ranges then? Where is any explanation
>> of hardware which would justify such combination?
>>
>>> +
>>> + reg:
>>> + maxItems: 1
>>> +
>>> + "#address-cells":
>>> + const: 1
>>> +
>>> + "#size-cells":
>>> + const: 1
>>> +
>>> + ranges: true
>>> +
>>> +patternProperties:
>>> + "^espi-ctrl@[0-9a-f]+$":
>>> + type: object
>>> +
>>> + description: Controls the flash channel of eSPI hardware
>> That explains nothing. Unless you wanted to use here MTD bindings.
>>
>> This binding did not improve much. I don't understand why this is not
>> SPI (nothing in commit msg, nothing in description), what is eSPI,
>
> eSPI uses Peripheral, Virtual Wire, Out of Band, and Flash Access
> channels to communicate different sets of data.
And what are these channels? What does it mean a "channel"? Is it just
how you organize transfers and classes of devices? Or some sort of
addressable instance on the bus?
The channels feel like some sort of software or logical concept, not
physical. Physical would be endpoint with peripheral. Or flash memory.
How do they fit here?
>
> * The *Peripheral* Channel is used for communication between eSPI host
> bridge located on the master side and eSPI endpoints located on the
> slave side. LPC Host and LPC Peripherals are an example of eSPI host
> bridge and eSPI endpoints respectively.
> * *Virtual Wire* Channel: The Virtual Wire channel is used to
> communicate the state of sideband pins or GPIO tunneled through eSPI
> as in-band messages. Serial IRQ interrupts are communicated through
> this channel as in-band messages.
> * *OOB* Channel: The SMBus packets are tunneled through eSPI as
> Out-Of-Band (OOB) messages. The whole SMBus packet is embedded
> inside the eSPI OOB message as data.
> * *Flash Access* Channel: The Flash Access channel provides a path
> allowing the flash components to be shared run-time between chipset
> and the eSPI slaves that require flash accesses such as EC (Embedded
> Controller) and BMC.
Please make binding complete, so define all of the channels.
>
> Although , eSPI reuses the timing and electrical specification of Serial
> Peripheral Interface (SPI) but it runs an entirely different protocol to
> meet a set of different requirements. Which is why i felt probably
> placing this in soc was a better choice rather than spi. Do you think
> otherwise ?
soc is dumping ground for things do not fit other places. Are there any
other buses / IP blocks similar to this one?
Best regards,
Krzysztof
On Wed, Mar 20, 2024 at 03:29:15PM +0530, Manojkiran Eda wrote:
> On 19/03/24 3:26 pm, Krzysztof Kozlowski wrote:
>
> On 19/03/2024 10:34, Manojkiran Eda wrote:
>
> This commit adds the device tree bindings for aspeed eSPI
> controller.
>
> Although aspeed eSPI hardware supports 4 different channels,
> this commit only adds the support for flash channel, the
> bindings for other channels could be upstreamed when the driver
> support for those are added.
>
> Signed-off-by: Manojkiran Eda [1]<[email protected]>
> ---
> .../bindings/soc/aspeed/aspeed,espi.yaml | 94 +++++++++++++++++++
> 1 file changed, 94 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yam
> l
>
> diff --git a/Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml b/Doc
> umentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml
> new file mode 100644
> index 000000000000..3d3ad528e3b3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml
>
> Why Rob's comments got ignored?
>
> This is not a soc component.
>
> I did not mean to ignore, i have few reasons listed below that provides
> information on why i felt this belongs into soc.
Fix you email program to not send multi-part (txt plus html) emails.
Plain text only on maillists.
Rob
On 20/03/24 8:14 pm, Krzysztof Kozlowski wrote:
> On 20/03/2024 10:59, Manojkiran Eda wrote:
>>
>> On 19/03/24 3:26 pm, Krzysztof Kozlowski wrote:
>>> On 19/03/2024 10:34, Manojkiran Eda wrote:
>>>> This commit adds the device tree bindings for aspeed eSPI
>>>> controller.
>>>>
>>>> Although aspeed eSPI hardware supports 4 different channels,
>>>> this commit only adds the support for flash channel, the
>>>> bindings for other channels could be upstreamed when the driver
>>>> support for those are added.
>>>>
>>>> Signed-off-by: Manojkiran Eda<[email protected]>
>>>> ---
>>>> .../bindings/soc/aspeed/aspeed,espi.yaml | 94 +++++++++++++++++++
>>>> 1 file changed, 94 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml b/Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml
>>>> new file mode 100644
>>>> index 000000000000..3d3ad528e3b3
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/soc/aspeed/aspeed,espi.yaml
>>> Why Rob's comments got ignored?
>>>
>>> This is not a soc component.
>> I did not mean to ignore, i have few reasons listed below that provides
>> information on why i felt this belongs into soc.
>
> soc is dumping ground of things which are purely SoC specific, not
> covered by existing hardware structure in bindings. Maybe indeed this
> does not have any other place, but did you actually look?
>
Yes, i did look at existing hardware bindings, and cannot seem to find
out any other suitable place. I can definitely look again.
> Anyway, please CC SPI maintainers on future submission.
Sure, will add them.
>
>>>
>>>> @@ -0,0 +1,94 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +# # Copyright (c) 2024 IBM Corporation.
>>>> +# # Copyright (c) 2021 Aspeed Technology Inc.
>>>> +%YAML 1.2
>>>> +---
>>>> +$id:http://devicetree.org/schemas/soc/aspeed/aspeed,espi.yaml#
>>>> +$schema:http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Aspeed eSPI Controller
>>>> +
>>>> +maintainers:
>>>> + - Manojkiran Eda<[email protected]>
>>>> + - Patrick Rudolph<[email protected]>
>>>> + - Chia-Wei Wang<[email protected]>
>>>> + - Ryan Chen<[email protected]>
>>>> +
>>>> +description:
>>>> + Aspeed eSPI controller implements a device side eSPI endpoint device
>>>> + supporting the flash channel.
>>> Explain what is eSPI.
>> eSPI is a serial bus interface for client and server platforms that is
>
> Explain in description of the hardware.
Sure, i will add this description in the binding document in the future
submission.
>
>> based on SPI, using the same master and slave topology but operates
>> with a different protocol to meet new requirements. For instance, eSPI
>> uses I/O, or input/output, communication, instead of MOSI/MISO used in
>> SPI. It also includes a transaction layer on top of the SPI protocol,
>> defining packets such as command and response packets that allow both
>> the master and slave to initiate alert and reset signals. eSPI supports
>> communication between Embedded Controller (EC), Baseboard Management
>> Controller (BMC), Super-I/O (SIO) and Port-80 debug cards. I could add
>> this to the commit message as well in the next patchset.
>>>
>>>> +
>>>> +properties:
>>>> + compatible:
>>>> + items:
>>>> + - enum:
>>>> + - aspeed,ast2500-espi
>>>> + - aspeed,ast2600-espi
>>>> + - const: simple-mfd
>>>
>>> That's not simple-mfd. You have driver for this. Drop.
>>>
>>>> + - const: syscon
>>> That's not syscon. Why do you have ranges then? Where is any explanation
>>> of hardware which would justify such combination?
>>>
>>>> +
>>>> + reg:
>>>> + maxItems: 1
>>>> +
>>>> + "#address-cells":
>>>> + const: 1
>>>> +
>>>> + "#size-cells":
>>>> + const: 1
>>>> +
>>>> + ranges: true
>>>> +
>>>> +patternProperties:
>>>> + "^espi-ctrl@[0-9a-f]+$":
>>>> + type: object
>>>> +
>>>> + description: Controls the flash channel of eSPI hardware
>>> That explains nothing. Unless you wanted to use here MTD bindings.
>>>
>>> This binding did not improve much. I don't understand why this is not
>>> SPI (nothing in commit msg, nothing in description), what is eSPI,
>>
>> eSPI uses Peripheral, Virtual Wire, Out of Band, and Flash Access
>> channels to communicate different sets of data.
>
> And what are these channels? What does it mean a "channel"? Is it just
> how you organize transfers and classes of devices? Or some sort of
> addressable instance on the bus?
>
Yes, an espi channel provides a means to allow multiple independent
flows of traffic to share the same physical bus. Each of the channels
has its own dedicated resources such as queue and flow control.
> The channels feel like some sort of software or logical concept, not
> physical. Physical would be endpoint with peripheral. Or flash memory.
A channel is a logical communication pathway or interface between the
chipset and peripheral devices. The concept of channels in the ESPI
protocol helps organize and manage different types of communication
between the chipset and peripherals. Each channel may have its own set
of protocols, data transfer rates, and supported features, tailored to
the requirements of the devices it serves.
> How do they fit here?
I am not sure I understand, can you please elaborate ?
>>
>> * The *Peripheral* Channel is used for communication between eSPI host
>> bridge located on the master side and eSPI endpoints located on the
>> slave side. LPC Host and LPC Peripherals are an example of eSPI host
>> bridge and eSPI endpoints respectively.
>> * *Virtual Wire* Channel: The Virtual Wire channel is used to
>> communicate the state of sideband pins or GPIO tunneled through eSPI
>> as in-band messages. Serial IRQ interrupts are communicated through
>> this channel as in-band messages.
>> * *OOB* Channel: The SMBus packets are tunneled through eSPI as
>> Out-Of-Band (OOB) messages. The whole SMBus packet is embedded
>> inside the eSPI OOB message as data.
>> * *Flash Access* Channel: The Flash Access channel provides a path
>> allowing the flash components to be shared run-time between chipset
>> and the eSPI slaves that require flash accesses such as EC (Embedded
>> Controller) and BMC.
>
> Please make binding complete, so define all of the channels.
I would like to inquire about the rationale behind this request. Based
on previous feedback received from the upstream efforts
[https://lore.kernel.org/openbmc/HK0PR06MB37798462D17443C697433D7191D09@HK0PR06MB3779.apcprd06.prod.outlook.com/],
suggestions were made to model the flash channel by utilizing the mtd
subsystem, the virtual wire channel by utilizing the GPIO subsystem, and
to consider the OOB channel as a type of i2c device, thereby allowing it
to be utilized by the existing in-kernel MCTP subsystem, among others.
My intention was to prioritize upstreaming the flash channel binding,
along with its driver code, before proceeding to address other channels.
I am curious to understand if it is a strict requirement to have the
complete binding upstreamed before addressing the device drivers code.
>
>>
>> Although , eSPI reuses the timing and electrical specification of Serial
>> Peripheral Interface (SPI) but it runs an entirely different protocol to
>> meet a set of different requirements. Which is why i felt probably
>> placing this in soc was a better choice rather than spi. Do you think
>> otherwise ?
>
> soc is dumping ground for things do not fit other places. Are there any
> other buses / IP blocks similar to this one?
>
>
> Best regards,
> Krzysztof
>
Thanks,
Manoj
On 28/03/2024 12:33, Manojkiran Eda wrote:
>>>>> + description: Controls the flash channel of eSPI hardware
>>>> That explains nothing. Unless you wanted to use here MTD bindings.
>>>>
>>>> This binding did not improve much. I don't understand why this is not
>>>> SPI (nothing in commit msg, nothing in description), what is eSPI,
>>>
>>> eSPI uses Peripheral, Virtual Wire, Out of Band, and Flash Access
>>> channels to communicate different sets of data.
>>
>> And what are these channels? What does it mean a "channel"? Is it just
>> how you organize transfers and classes of devices? Or some sort of
>> addressable instance on the bus?
>>
>
> Yes, an espi channel provides a means to allow multiple independent
> flows of traffic to share the same physical bus. Each of the channels
> has its own dedicated resources such as queue and flow control.
Resources as queue and flow-control? Where are they defined in
Devicetree? Which binding?
>
>> The channels feel like some sort of software or logical concept, not
>> physical. Physical would be endpoint with peripheral. Or flash memory.
>
> A channel is a logical communication pathway or interface between the
> chipset and peripheral devices. The concept of channels in the ESPI
> protocol helps organize and manage different types of communication
> between the chipset and peripherals. Each channel may have its own set
> of protocols, data transfer rates, and supported features, tailored to
> the requirements of the devices it serves.
>
>> How do they fit here?
>
> I am not sure I understand, can you please elaborate ?
All this suggests channel is programming aspect of your device thus does
not have to be represented in DT. I don't know, come with any DT
property to back up your case...
So far I see only interrupts and clocks, but then I would claim these
could be part of parent node.
Rob said it last time: your split of nodes looks artificial and it all
looks like one node.
Your DTS reg like:
reg = <0x0 0x800>,<0x0 0x4000000>;
proves it. I don't know if this is just bug in your code or some silly
DTS just to create fake children. :/
>
>>>
>>> * The *Peripheral* Channel is used for communication between eSPI host
>>> bridge located on the master side and eSPI endpoints located on the
>>> slave side. LPC Host and LPC Peripherals are an example of eSPI host
>>> bridge and eSPI endpoints respectively.
>>> * *Virtual Wire* Channel: The Virtual Wire channel is used to
>>> communicate the state of sideband pins or GPIO tunneled through eSPI
>>> as in-band messages. Serial IRQ interrupts are communicated through
>>> this channel as in-band messages.
>>> * *OOB* Channel: The SMBus packets are tunneled through eSPI as
>>> Out-Of-Band (OOB) messages. The whole SMBus packet is embedded
>>> inside the eSPI OOB message as data.
>>> * *Flash Access* Channel: The Flash Access channel provides a path
>>> allowing the flash components to be shared run-time between chipset
>>> and the eSPI slaves that require flash accesses such as EC (Embedded
>>> Controller) and BMC.
>>
>> Please make binding complete, so define all of the channels.
>
>
> I would like to inquire about the rationale behind this request. Based
Rationale - writing bindings document.
https://elixir.bootlin.com/linux/v6.9-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L17
> on previous feedback received from the upstream efforts
> [https://lore.kernel.org/openbmc/HK0PR06MB37798462D17443C697433D7191D09@HK0PR06MB3779.apcprd06.prod.outlook.com/],
> suggestions were made to model the flash channel by utilizing the mtd
> subsystem, the virtual wire channel by utilizing the GPIO subsystem, and
> to consider the OOB channel as a type of i2c device, thereby allowing it
> to be utilized by the existing in-kernel MCTP subsystem, among others.
> My intention was to prioritize upstreaming the flash channel binding,
> along with its driver code, before proceeding to address other channels.
Just to clarify: I don't care about drivers and we do not talk about
them here.
> I am curious to understand if it is a strict requirement to have the
> complete binding upstreamed before addressing the device drivers code.
What if your other "devices" or "channels" are entirely different and
binding would just not work? Or how can we understand the design if you
upstream only part of it?
Best regards,
Krzysztof