[PATCH v2 0/4] Add eSPI device driver (flash channel)

[PATCH v2 0/4] Add eSPI device driver (flash channel)

[Manojkiran Eda]

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/20220516005412.4844-1-chiawei_wang at aspeedtech.com/

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

[PATCH v2 1/4] Add eSPI device driver (flash channel)

[Manojkiran Eda]

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 <manojkiran.eda@gmail.com>
---
 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 <manojkiran.eda@gmail.com>");
+MODULE_AUTHOR("Patrick Rudolph <patrick.rudolph@9elements.com>");
+MODULE_AUTHOR("Chia-Wei Wang <chiawei_wang@aspeedtech.com>");
+MODULE_AUTHOR("Ryan Chen <ryan_chen@aspeedtech.com>");
+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, &reg);
+	*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, &reg);
+	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, &reg);
+	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 = (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

[PATCH v2 1/4] Add eSPI device driver (flash channel)

[Krzysztof Kozlowski]

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 <manojkiran.eda@gmail.com>
> ---
>  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 = (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

[PATCH v2 2/4] mtd: Replace module_init with subsys_initcall

[Manojkiran Eda]

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/20240213-espi_driver-v1-1-92741c812843 at gmail.com

Signed-off-by: Manojkiran Eda <manojkiran.eda@gmail.com>
---
 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

[PATCH v2 2/4] mtd: Replace module_init with subsys_initcall

[Krzysztof Kozlowski]

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

[PATCH v2 2/4] mtd: Replace module_init with subsys_initcall

[Miquel Raynal]

Hi,

krzysztof.kozlowski at linaro.org 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

[PATCH v2 3/4] ARM: dts: aspeed: Add eSPI node

[Manojkiran Eda]

This commit adds eSPI to the device tree for aspeed 5/6th
generation SoCs.

Signed-off-by: Manojkiran Eda <manojkiran.eda@gmail.com>
---
 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 at 200 {
 					status = "disabled";
 				};
 			};
+			espi: espi at 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 at 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 at 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 at 1e700000 {
 				status = "disabled";
 			};
 
+			espi: espi at 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 at 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 at 1e780000 {
 				#gpio-cells = <2>;
 				gpio-controller;
-- 
2.40.1

[PATCH v2 3/4] ARM: dts: aspeed: Add eSPI node

[Krzysztof Kozlowski]

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 <manojkiran.eda@gmail.com>
> ---
>  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 at 200 {
>  					status = "disabled";
>  				};
>  			};
> +			espi: espi at 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 at 0 {

What is this device? If parent is espi, then what is this?

Where is the binding?


Best regards,
Krzysztof

[PATCH v2 4/4] dt-bindings: aspeed: Add eSPI controller

[Manojkiran Eda]

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 <manojkiran.eda@gmail.com>
---
 .../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 <manojkiran.eda@gmail.com>
+  - Patrick Rudolph <patrick.rudolph@9elements.com>
+  - Chia-Wei Wang <chiawei_wang@aspeedtech.com>
+  - Ryan Chen <ryan_chen@aspeedtech.com>
+
+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 at 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 at 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

[PATCH v2 4/4] dt-bindings: aspeed: Add eSPI controller

[Krzysztof Kozlowski]

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 <manojkiran.eda@gmail.com>
> ---
>  .../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 <manojkiran.eda@gmail.com>
> +  - Patrick Rudolph <patrick.rudolph@9elements.com>
> +  - Chia-Wei Wang <chiawei_wang@aspeedtech.com>
> +  - Ryan Chen <ryan_chen@aspeedtech.com>
> +
> +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 at 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 at 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

[PATCH v2 4/4] dt-bindings: aspeed: Add eSPI controller

[Manojkiran Eda]

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<manojkiran.eda@gmail.com>
>> ---
>>   .../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.
>
>> @@ -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<manojkiran.eda@gmail.com>
>> +  - Patrick Rudolph<patrick.rudolph@9elements.com>
>> +  - Chia-Wei Wang<chiawei_wang@aspeedtech.com>
>> +  - Ryan Chen<ryan_chen@aspeedtech.com>
>> +
>> +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 
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.

  * 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.

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 ?

>   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,

Apologies, this was not my intention. I wanted this to be as generic as 
possible. But i don't really have much knowledge on what's the right way 
to model things in kernel at the moment. Still trying to learn and 
understand by looking at various other drivers. Appreciate all the 
feedback.
>   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.
sure, will fix it.
>> +          - 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 at 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 at 0 {
>> +            compatible = "aspeed,ast2600-espi-ctrl";
>> +            reg = <0x0 0x800>,<0x0 0x4000000>;
> Fix your style in DTS. There is always a space after ','.
sure , will fix that. Is there a link that could help me understand 
various styling requirements on the DTS files. Also is there any 
formatting tool available currently ? that could fix the styling in the 
DTS files automatically rather than manual inspection/modification. Did 
i accidentally missed running some tool check ?
>
>> +            reg-names = "espi_ctrl","espi_flash";
>> +            interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>;
>> +            clocks = <&syscon ASPEED_CLK_GATE_ESPICLK>;
>> +        };
>> +    };
> Best regards,
> Krzysztof
>
>
Krzysztof,Thanks for the review comments. I am still figuring out few of the 
review comments (would need a little more time, since its my first 
attempt into kernel development) , but mean while I wanted to make sure 
if the direction of choosing "soc" vs "spi" was correct, so that i could 
re-work on the comments.So i have selectively answered to few of your 
comments. Could you let me know if the reasoning that was provided in 
reply to your comments help ? Thanks, Manoj
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/linux-aspeed/attachments/20240320/f2a0be08/attachment-0001.htm>

[PATCH v2 4/4] dt-bindings: aspeed: Add eSPI controller

[Krzysztof Kozlowski]

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<manojkiran.eda@gmail.com>
>>> ---
>>>   .../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<manojkiran.eda@gmail.com>
>>> +  - Patrick Rudolph<patrick.rudolph@9elements.com>
>>> +  - Chia-Wei Wang<chiawei_wang@aspeedtech.com>
>>> +  - Ryan Chen<ryan_chen@aspeedtech.com>
>>> +
>>> +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

[PATCH v2 4/4] dt-bindings: aspeed: Add eSPI controller

[Manojkiran Eda]

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<manojkiran.eda@gmail.com>
>>>> ---
>>>>    .../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<manojkiran.eda@gmail.com>
>>>> +  - Patrick Rudolph<patrick.rudolph@9elements.com>
>>>> +  - Chia-Wei Wang<chiawei_wang@aspeedtech.com>
>>>> +  - Ryan Chen<ryan_chen@aspeedtech.com>
>>>> +
>>>> +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 at 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

[PATCH v2 4/4] dt-bindings: aspeed: Add eSPI controller

[Krzysztof Kozlowski]

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 at 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

[PATCH v2 4/4] dt-bindings: aspeed: Add eSPI controller

[Rob Herring]

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]<manojkiran.eda@gmail.com>
> ---
>  .../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