When PCH works under eSPI mode, the PMC (Power Management Controller) in
PCH is waiting for SUS_ACK from BMC after it alerts SUS_WARN. It is in
dead loop if no SUS_ACK assert. This is the basic requirement for the BMC
works as eSPI slave.
Also for the host power on / off actions, from BMC side, the following VW
(Virtual Wire) messages are done in firmware:
1. SLAVE_BOOT_LOAD_DONE / SLAVE_BOOT_LOAD_STATUS
2. SUS_ACK
3. OOB_RESET_ACK
4. HOST_RESET_ACK
Signed-off-by: Haiyue Wang <[email protected]>
---
.../devicetree/bindings/misc/aspeed-espi-slave.txt | 20 ++
Documentation/misc-devices/espi-slave.rst | 114 +++++++++
drivers/misc/Kconfig | 11 +
drivers/misc/Makefile | 1 +
drivers/misc/aspeed-espi-slave.c | 266 +++++++++++++++++++++
5 files changed, 412 insertions(+)
create mode 100644 Documentation/devicetree/bindings/misc/aspeed-espi-slave.txt
create mode 100644 Documentation/misc-devices/espi-slave.rst
create mode 100644 drivers/misc/aspeed-espi-slave.c
diff --git a/Documentation/devicetree/bindings/misc/aspeed-espi-slave.txt b/Documentation/devicetree/bindings/misc/aspeed-espi-slave.txt
new file mode 100644
index 0000000..35da26f
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/aspeed-espi-slave.txt
@@ -0,0 +1,20 @@
+Aspeed eSPI Slave Controller
+
+Required properties:
+ - compatible: must be one of:
+ - "aspeed,ast2500-espi-slave"
+
+ - reg: physical base address of the controller and length of memory mapped
+ region
+
+ - interrupts: interrupt generated by the controller
+
+Example:
+
+ espi: espi@1e6ee000 {
+ compatible = "aspeed,ast2500-espi-slave";
+ reg = <0x1e6ee000 0x100>;
+ interrupts = <23>;
+ status = "disabled";
+};
+
diff --git a/Documentation/misc-devices/espi-slave.rst b/Documentation/misc-devices/espi-slave.rst
new file mode 100644
index 0000000..405b619
--- /dev/null
+++ b/Documentation/misc-devices/espi-slave.rst
@@ -0,0 +1,114 @@
+eSPI Slave
+==========
+
+The PCH (**eSPI master**) provides the eSPI to support connection of a
+BMC (**eSPI slave**) to the platform.
+
+The LPC and eSPI interfaces are mutually exclusive. Both use the same
+pins, but on power-up, a HW strap determines if the eSPI or the LPC bus
+is operational. Once selected, it’s not possible to change to the other
+interface.
+
+``eSPI Channels and Supported Transactions``
+ +------+---------------------+----------------------+--------------------+
+ | CH # | Channel | Posted Cycles | Non-Posted Cycles |
+ +======+=====================+======================+====================+
+ | 0 | Peripheral | Memory Write, | Memory Read, |
+ | | | Completions | I/O Read/Write |
+ +------+---------------------+----------------------+--------------------+
+ | 1 | Virtual Wire | Virtual Wire GET/PUT | N/A |
+ +------+---------------------+----------------------+--------------------+
+ | 2 | Out-of-Band Message | SMBus Packet GET/PUT | N/A |
+ +------+---------------------+----------------------+--------------------+
+ | 3 | Flash Access | N/A | Flash Read, Write, |
+ | | | | Erase |
+ +------+---------------------+----------------------+--------------------+
+ | N/A | General | Register Accesses | N/A |
+ +------+---------------------+----------------------+--------------------+
+
+Virtual Wire Channel (Channel 1) Overview
+-----------------------------------------
+
+The Virtual Wire channel uses a standard message format to communicate
+several types of signals between the components on the platform::
+
+ - Sideband and GPIO Pins: System events and other dedicated signals
+ between the PCH and eSPI slave. These signals are tunneled between the
+ two components over eSPI.
+
+ - Serial IRQ Interrupts: Interrupts are tunneled from the eSPI slave to
+ the PCH. Both edge and triggered interrupts are supported.
+
+When PCH runs on eSPI mode, from BMC side, the following VW messages are
+done in firmware::
+
+ 1. SLAVE_BOOT_LOAD_DONE / SLAVE_BOOT_LOAD_STATUS
+ 2. SUS_ACK
+ 3. OOB_RESET_ACK
+ 4. HOST_RESET_ACK
+
+``eSPI Virtual Wires (VW)``
+ +----------------------+---------+---------------------------------------+
+ |Virtual Wire |PCH Pin |Comments |
+ | |Direction| |
+ +======================+=========+=======================================+
+ |SUS_WARN# |Output |PCH pin is a GPIO when eSPI is enabled.|
+ | | |eSPI controller receives as VW message.|
+ +----------------------+---------+---------------------------------------+
+ |SUS_ACK# |Input |PCH pin is a GPIO when eSPI is enabled.|
+ | | |eSPI controller receives as VW message.|
+ +----------------------+---------+---------------------------------------+
+ |SLAVE_BOOT_LOAD_DONE |Input |Sent when the BMC has completed its |
+ | | |boot process as an indication to |
+ | | |eSPI-MC to continue with the G3 to S0 |
+ | | |exit. |
+ | | |The eSPI Master waits for the assertion|
+ | | |of this virtual wire before proceeding |
+ | | |with the SLP_S5# deassertion. |
+ | | |The intent is that it is never changed |
+ | | |except on a G3 exit - it is reset on a |
+ | | |G3 entry. |
+ +----------------------+---------+---------------------------------------+
+ |SLAVE_BOOT_LOAD_STATUS|Input |Sent upon completion of the Slave Boot |
+ | | |Load from the attached flash. A stat of|
+ | | |1 indicates that the boot code load was|
+ | | |successful and that the integrity of |
+ | | |the image is intact. |
+ +----------------------+---------+---------------------------------------+
+ |HOST_RESET_WARN |Output |Sent from the MC just before the Host |
+ | | |is about to enter reset. Upon receiving|
+ | | |, the BMC must flush and quiesce its |
+ | | |upstream Peripheral Channel request |
+ | | |queues and assert HOST_RESET_ACK VWire.|
+ | | |The MC subsequently completes any |
+ | | |outstanding posted transactions or |
+ | | |completions and then disables the |
+ | | |Peripheral Channel via a write to |
+ | | |the Slave's Configuration Register. |
+ +----------------------+---------+---------------------------------------+
+ |HOST_RESET_ACK |Input |ACK for the HOST_RESET_WARN message |
+ +----------------------+---------+---------------------------------------+
+ |OOB_RESET_WARN |Output |Sent from the MC just before the OOB |
+ | | |processor is about to enter reset. Upon|
+ | | |receiving, the BMC must flush and |
+ | | |quiesce its OOB Channel upstream |
+ | | |request queues and assert OOB_RESET_ACK|
+ | | |VWire. The-MC subsequently completes |
+ | | |any outstanding posted transactions or |
+ | | |completions and then disables the OOB |
+ | | |Channel via a write to the Slave's |
+ | | |Configuration Register. |
+ +----------------------+---------+---------------------------------------+
+ |OOB_RESET_ACK |Input |ACK for OOB_RESET_WARN message |
+ +----------------------+---------+---------------------------------------+
+
+`Intel C620 Series Chipset Platform Controller Hub
+<https://www.intel.com/content/www/us/en/chipsets/c620-series-chipset-datasheet.html>`_
+
+ -- 17. Enhanced Serial Peripheral Interface
+
+
+`Enhanced Serial Peripheral Interface (eSPI)
+- Interface Base Specification (for Client and Server Platforms)
+<https://www.intel.com/content/dam/support/us/en/documents/software/chipset-software/327432-004_espi_base_specification_rev1.0.pdf>`_
+
diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index f1a5c23..fec5783 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -471,6 +471,17 @@ config VEXPRESS_SYSCFG
ARM Ltd. Versatile Express uses specialised platform configuration
bus. System Configuration interface is one of the possible means
of generating transactions on this bus.
+config ASPEED_ESPI_SLAVE
+ depends on ARCH_ASPEED || COMPILE_TEST
+ select REGMAP_MMIO
+ tristate "Aspeed ast2500 eSPI slave device"
+ ---help---
+ This allows host to access Baseboard Management Controller (BMC) over the
+ Enhanced Serial Peripheral Interface (eSPI) bus, which replaces the Low Pin
+ Count (LPC) bus.
+
+ Its interface supports peripheral, virtual wire, out-of-band, and flash
+ sharing channels.
config ASPEED_LPC_CTRL
depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 5ca5f64..a1081f4 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_GENWQE) += genwqe/
obj-$(CONFIG_ECHO) += echo/
obj-$(CONFIG_VEXPRESS_SYSCFG) += vexpress-syscfg.o
obj-$(CONFIG_CXL_BASE) += cxl/
+obj-$(CONFIG_ASPEED_ESPI_SLAVE) += aspeed-espi-slave.o
obj-$(CONFIG_ASPEED_LPC_CTRL) += aspeed-lpc-ctrl.o
obj-$(CONFIG_ASPEED_LPC_SNOOP) += aspeed-lpc-snoop.o
obj-$(CONFIG_PCI_ENDPOINT_TEST) += pci_endpoint_test.o
diff --git a/drivers/misc/aspeed-espi-slave.c b/drivers/misc/aspeed-espi-slave.c
new file mode 100644
index 0000000..a0359cb
--- /dev/null
+++ b/drivers/misc/aspeed-espi-slave.c
@@ -0,0 +1,266 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2012-2020, ASPEED Technology Inc.
+// Copyright (c) 2015-2017, Intel Corporation.
+
+#include <linux/atomic.h>
+#include <linux/errno.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/poll.h>
+#include <linux/regmap.h>
+#include <linux/sched.h>
+#include <linux/slab.h>
+#include <linux/timer.h>
+
+#define DEVICE_NAME "aspeed-espi-slave"
+
+
+#define ESPI_CTRL 0x00
+#define ESPI_CTRL_SW_RESET GENMASK(31, 24)
+#define ESPI_CTRL_OOB_CHRDY BIT(4)
+#define ESPI_ISR 0x08
+#define ESPI_ISR_HW_RESET BIT(31)
+#define ESPI_ISR_VW_SYS_EVT1 BIT(22)
+#define ESPI_ISR_VW_SYS_EVT BIT(8)
+#define ESPI_IER 0x0C
+
+#define ESPI_SYS_IER 0x94
+#define ESPI_SYS_EVENT 0x98
+#define ESPI_SYS_INT_T0 0x110
+#define ESPI_SYS_INT_T1 0x114
+#define ESPI_SYS_INT_T2 0x118
+#define ESPI_SYS_ISR 0x11C
+#define ESPI_SYSEVT_HOST_RST_ACK BIT(27)
+#define ESPI_SYSEVT_SLAVE_BOOT_STATUS BIT(23)
+#define ESPI_SYSEVT_SLAVE_BOOT_DONE BIT(20)
+#define ESPI_SYSEVT_OOB_RST_ACK BIT(16)
+#define ESPI_SYSEVT_HOST_RST_WARN BIT(8)
+#define ESPI_SYSEVT_OOB_RST_WARN BIT(6)
+#define ESPI_SYSEVT_PLT_RST_N BIT(5)
+
+#define ESPI_SYS1_IER 0x100
+#define ESPI_SYS1_EVENT 0x104
+#define ESPI_SYS1_INT_T0 0x120
+#define ESPI_SYS1_INT_T1 0x124
+#define ESPI_SYS1_INT_T2 0x128
+#define ESPI_SYS1_ISR 0x12C
+#define ESPI_SYSEVT1_SUS_ACK BIT(20)
+#define ESPI_SYSEVT1_SUS_WARN BIT(0)
+
+
+struct aspeed_espi_slave_data {
+ struct regmap *map;
+};
+
+static void aspeed_espi_slave_sys_event(struct aspeed_espi_slave_data *priv)
+{
+ u32 sts, evt;
+
+ if (regmap_read(priv->map, ESPI_SYS_ISR, &sts) != 0 ||
+ regmap_read(priv->map, ESPI_SYS_EVENT, &evt) != 0)
+ return;
+
+ if (sts & ESPI_SYSEVT_HOST_RST_WARN)
+ regmap_update_bits_base(priv->map, ESPI_SYS_EVENT,
+ ESPI_SYSEVT_HOST_RST_ACK,
+ evt & ESPI_SYSEVT_HOST_RST_WARN ?
+ ESPI_SYSEVT_HOST_RST_ACK : 0,
+ NULL, false, true);
+
+ if (sts & ESPI_SYSEVT_OOB_RST_WARN)
+ regmap_update_bits_base(priv->map, ESPI_SYS_EVENT,
+ ESPI_SYSEVT_OOB_RST_ACK,
+ evt & ESPI_SYSEVT_OOB_RST_WARN ?
+ ESPI_SYSEVT_OOB_RST_ACK : 0,
+ NULL, false, true);
+
+ regmap_write(priv->map, ESPI_SYS_ISR, sts);
+}
+
+static void aspeed_espi_slave_sys1_event(struct aspeed_espi_slave_data *priv)
+{
+ u32 sts;
+
+ if (regmap_read(priv->map, ESPI_SYS1_ISR, &sts) != 0)
+ return;
+
+ if (sts & ESPI_SYSEVT1_SUS_WARN)
+ regmap_update_bits_base(priv->map, ESPI_SYS1_EVENT,
+ ESPI_SYSEVT1_SUS_ACK, ESPI_SYSEVT1_SUS_ACK,
+ NULL, false, true);
+
+ regmap_write(priv->map, ESPI_SYS1_ISR, sts);
+}
+
+static irqreturn_t aspeed_espi_slave_irq(int irq, void *arg)
+{
+ struct aspeed_espi_slave_data *priv = arg;
+ u32 sts;
+
+ if (regmap_read(priv->map, ESPI_ISR, &sts) != 0)
+ return IRQ_NONE;
+
+ if (sts & ESPI_ISR_HW_RESET) {
+ regmap_update_bits_base(priv->map, ESPI_CTRL,
+ ESPI_CTRL_SW_RESET, 0,
+ NULL, false, true);
+ regmap_update_bits_base(priv->map, ESPI_CTRL,
+ ESPI_CTRL_SW_RESET, ESPI_CTRL_SW_RESET,
+ NULL, false, true);
+
+ regmap_update_bits_base(priv->map, ESPI_SYS_EVENT,
+ ESPI_SYSEVT_SLAVE_BOOT_STATUS |
+ ESPI_SYSEVT_SLAVE_BOOT_DONE,
+ ESPI_SYSEVT_SLAVE_BOOT_STATUS |
+ ESPI_SYSEVT_SLAVE_BOOT_DONE,
+ NULL, false, true);
+ }
+
+ if (sts & ESPI_ISR_VW_SYS_EVT)
+ aspeed_espi_slave_sys_event(priv);
+
+ if (sts & ESPI_ISR_VW_SYS_EVT1)
+ aspeed_espi_slave_sys1_event(priv);
+
+ regmap_write(priv->map, ESPI_ISR, sts);
+
+ return IRQ_HANDLED;
+}
+
+
+/* Setup Interrupt Type/Enable of System Event from Master
+ * T2 T1 T0
+ * 1). HOST_RST_WARN : Dual Edge 1 0 0
+ * 2). OOB_RST_WARN : Dual Edge 1 0 0
+ * 3). PLTRST_N : Dual Edge 1 0 0
+ */
+#define ESPI_SYS_INT_T0_SET 0x00000000
+#define ESPI_SYS_INT_T1_SET 0x00000000
+#define ESPI_SYS_INT_T2_SET \
+(ESPI_SYSEVT_HOST_RST_WARN | ESPI_SYSEVT_OOB_RST_WARN | ESPI_SYSEVT_PLT_RST_N)
+#define ESPI_SYS_INT_SET \
+(ESPI_SYSEVT_HOST_RST_WARN | ESPI_SYSEVT_OOB_RST_WARN | ESPI_SYSEVT_PLT_RST_N)
+
+/* Setup Interrupt Type/Enable of System Event 1 from Master
+ * T2 T1 T0
+ * 1). SUS_WARN : Rising Edge 0 0 1
+ */
+#define ESPI_SYS1_INT_T0_SET ESPI_SYSEVT1_SUS_WARN
+#define ESPI_SYS1_INT_T1_SET 0x00000000
+#define ESPI_SYS1_INT_T2_SET 0x00000000
+#define ESPI_SYS1_INT_SET ESPI_SYSEVT1_SUS_WARN
+
+static int aspeed_espi_slave_config_irq(struct aspeed_espi_slave_data *priv,
+ struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ int irq;
+ int rc;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return irq;
+
+ rc = devm_request_irq(dev, irq, aspeed_espi_slave_irq, IRQF_SHARED,
+ dev_name(dev), priv);
+ if (rc < 0)
+ return rc;
+
+ regmap_update_bits(priv->map, ESPI_CTRL, ESPI_CTRL_OOB_CHRDY,
+ ESPI_CTRL_OOB_CHRDY);
+
+ regmap_write(priv->map, ESPI_SYS_INT_T0, ESPI_SYS_INT_T0_SET);
+ regmap_write(priv->map, ESPI_SYS_INT_T1, ESPI_SYS_INT_T1_SET);
+ regmap_write(priv->map, ESPI_SYS_INT_T2, ESPI_SYS_INT_T2_SET);
+ regmap_write(priv->map, ESPI_SYS_IER, ESPI_SYS_INT_SET);
+
+ regmap_write(priv->map, ESPI_SYS1_INT_T0, ESPI_SYS1_INT_T0_SET);
+ regmap_write(priv->map, ESPI_SYS1_INT_T1, ESPI_SYS1_INT_T1_SET);
+ regmap_write(priv->map, ESPI_SYS1_INT_T2, ESPI_SYS1_INT_T2_SET);
+ regmap_write(priv->map, ESPI_SYS1_IER, ESPI_SYS1_INT_SET);
+
+ regmap_write(priv->map, ESPI_IER, 0xFFFFFFFF);
+
+ return 0;
+}
+
+static void aspeed_espi_slave_boot_ack(struct aspeed_espi_slave_data *priv)
+{
+ u32 evt;
+
+ if (regmap_read(priv->map, ESPI_SYS_EVENT, &evt) == 0 &&
+ (evt & ESPI_SYSEVT_SLAVE_BOOT_STATUS) == 0)
+ regmap_write(priv->map, ESPI_SYS_EVENT, evt |
+ ESPI_SYSEVT_SLAVE_BOOT_STATUS |
+ ESPI_SYSEVT_SLAVE_BOOT_DONE);
+
+ if (regmap_read(priv->map, ESPI_SYS1_EVENT, &evt) == 0 &&
+ (evt & ESPI_SYSEVT1_SUS_WARN) != 0)
+ regmap_write(priv->map, ESPI_SYS1_EVENT, evt |
+ ESPI_SYSEVT1_SUS_ACK);
+}
+
+static const struct regmap_config espi_slave_regmap_cfg = {
+ .reg_bits = 32,
+ .reg_stride = 4,
+ .val_bits = 32,
+ .max_register = ESPI_SYS1_ISR,
+};
+
+static int aspeed_espi_slave_probe(struct platform_device *pdev)
+{
+ struct aspeed_espi_slave_data *priv;
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+ void __iomem *regs;
+ int rc;
+
+ dev_set_name(dev, DEVICE_NAME);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ regs = devm_ioremap_resource(dev, res);
+ if (IS_ERR(regs))
+ return PTR_ERR(regs);
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->map = devm_regmap_init_mmio(dev, regs, &espi_slave_regmap_cfg);
+ if (IS_ERR(priv->map))
+ return PTR_ERR(priv->map);
+
+ rc = aspeed_espi_slave_config_irq(priv, pdev);
+ if (rc)
+ return rc;
+
+ aspeed_espi_slave_boot_ack(priv);
+
+ platform_set_drvdata(pdev, priv);
+
+ return 0;
+}
+
+static const struct of_device_id of_espi_slave_match_table[] = {
+ { .compatible = "aspeed,ast2500-espi-slave" },
+ { }
+};
+
+static struct platform_driver aspeed_espi_slave_driver = {
+ .driver = {
+ .name = DEVICE_NAME,
+ .of_match_table = of_espi_slave_match_table,
+ },
+ .probe = aspeed_espi_slave_probe,
+};
+
+module_platform_driver(aspeed_espi_slave_driver);
+
+MODULE_DEVICE_TABLE(of, of_espi_slave_match_table);
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Haiyue Wang <[email protected]>");
+MODULE_DESCRIPTION("Linux device interface to the eSPI slave");
--
2.7.4
On Fri, Dec 29, 2017 at 2:53 AM, Haiyue Wang
<[email protected]> wrote:
> When PCH works under eSPI mode, the PMC (Power Management Controller) in
> PCH is waiting for SUS_ACK from BMC after it alerts SUS_WARN. It is in
> dead loop if no SUS_ACK assert. This is the basic requirement for the BMC
> works as eSPI slave.
>
> Also for the host power on / off actions, from BMC side, the following VW
> (Virtual Wire) messages are done in firmware:
> 1. SLAVE_BOOT_LOAD_DONE / SLAVE_BOOT_LOAD_STATUS
> 2. SUS_ACK
> 3. OOB_RESET_ACK
> 4. HOST_RESET_ACK
I have not looked at the driver contents yet, but I'm adding the SPI
maintainer and
mailing list to Cc here for further discussion. Can you clarify how
the eSPI slave
mode relates to SPI slaves that we already support? I was under the impression
that the difference between SPI and eSPI is mainly on the master side, but that
any SPI slave can also act as an eSPI slave. Would this driver fit into the SPI
slave framework, possibly with some extensions to the generic abstraction?
It also seems rather inflexible to have a single driver that is responsible both
for the transport (eSPI register level interface for ASPEED) and the high-level
protocol (talking to an Intel PCH), since either half of the work could be
done elsewhere, using either a different eSPI slave implementation, or
a different
host architecture)
Arnd
> On 2017-12-31 07:10, Arnd Bergmann wrote:
> > On Fri, Dec 29, 2017 at 2:53 AM, Haiyue Wang
> > <[email protected]> wrote:
> >> When PCH works under eSPI mode, the PMC (Power Management Controller) in
> >> PCH is waiting for SUS_ACK from BMC after it alerts SUS_WARN. It is in
> >> dead loop if no SUS_ACK assert. This is the basic requirement for the BMC
> >> works as eSPI slave.
> >>
> >> Also for the host power on / off actions, from BMC side, the following VW
> >> (Virtual Wire) messages are done in firmware:
> >> 1. SLAVE_BOOT_LOAD_DONE / SLAVE_BOOT_LOAD_STATUS
> >> 2. SUS_ACK
> >> 3. OOB_RESET_ACK
> >> 4. HOST_RESET_ACK
> > I have not looked at the driver contents yet, but I'm adding the SPI
> > maintainer and
> > mailing list to Cc here for further discussion. Can you clarify how
> > the eSPI slave
> > mode relates to SPI slaves that we already support? I was under the impression
> > that the difference between SPI and eSPI is mainly on the master side, but that
> > any SPI slave can also act as an eSPI slave. Would this driver fit into the SPI
> > slave framework, possibly with some extensions to the generic abstraction?
> In simple word, the eSPI uses the SPI interface pin definition, but it
> will replace Low Pin Count (LPC)
> interface. From its name, sure, it will confuse you! ;-)
I know what eSPI is meant for, and understand the basic idea of the
protocol, but I'm not familiar with the Apeed slave hardware
implementation.
> > It also seems rather inflexible to have a single driver that is responsible both
> > for the transport (eSPI register level interface for ASPEED) and the high-level
> > protocol (talking to an Intel PCH), since either half of the work could be
> > done elsewhere, using either a different eSPI slave implementation, or
> > a different
> > host architecture)
> Yes, eSPI has the architecture such as transaction layer, link Layer;
> all of it is about the **silicon**
> design. That's why I put the driver under /misc directory, not /spi
> directory.
I don't see any requirement in the eSPI spec for the upper layers to
be implemented in hardware. Obviously an x86 host such as Intel's
PCH would implement the host interface using PIO, and MMIO
accesses that are compatible with ISA and LPC, as this is the motivation
behind the specification, but an ARM server that wants to use eSPI
based peripherals could choose to implement it just as well using
a traditional SPI master hardware, some GPIOs (reset and alert)
and a (driver independent) software implementation of the transaction
and link layers.
On the slave side, it seems that aspeed have implemented the
virtual wires partially in hardware and require a driver like the one
you wrote to reply to some of the wires being accessed by the host,
but again it seems plausible that this could be implemented in another
BMC using a generic SPI slave and a transaction layer written
entirely in software.
Your driver does not handle the other channels (smbus, mmio, spinor)
at the moment at all, can you provide some information how they
are implemented in the ast2500? Are those handled completely
in hardware (I assume this is the case for spinor at least), or do they
require help from a driver, either this one or a separate one?
Arnd
On 2018-01-02 23:13, Arnd Bergmann wrote:
>> On 2017-12-31 07:10, Arnd Bergmann wrote:
>>> On Fri, Dec 29, 2017 at 2:53 AM, Haiyue Wang
>>> <[email protected]> wrote:
>>>> When PCH works under eSPI mode, the PMC (Power Management Controller) in
>>>> PCH is waiting for SUS_ACK from BMC after it alerts SUS_WARN. It is in
>>>> dead loop if no SUS_ACK assert. This is the basic requirement for the BMC
>>>> works as eSPI slave.
>>>>
>>>> Also for the host power on / off actions, from BMC side, the following VW
>>>> (Virtual Wire) messages are done in firmware:
>>>> 1. SLAVE_BOOT_LOAD_DONE / SLAVE_BOOT_LOAD_STATUS
>>>> 2. SUS_ACK
>>>> 3. OOB_RESET_ACK
>>>> 4. HOST_RESET_ACK
>>> I have not looked at the driver contents yet, but I'm adding the SPI
>>> maintainer and
>>> mailing list to Cc here for further discussion. Can you clarify how
>>> the eSPI slave
>>> mode relates to SPI slaves that we already support? I was under the impression
>>> that the difference between SPI and eSPI is mainly on the master side, but that
>>> any SPI slave can also act as an eSPI slave. Would this driver fit into the SPI
>>> slave framework, possibly with some extensions to the generic abstraction?
>> In simple word, the eSPI uses the SPI interface pin definition, but it
>> will replace Low Pin Count (LPC)
>> interface. From its name, sure, it will confuse you! ;-)
> I know what eSPI is meant for, and understand the basic idea of the
> protocol, but I'm not familiar with the Apeed slave hardware
> implementation.
I see! ;-)
>>> It also seems rather inflexible to have a single driver that is responsible both
>>> for the transport (eSPI register level interface for ASPEED) and the high-level
>>> protocol (talking to an Intel PCH), since either half of the work could be
>>> done elsewhere, using either a different eSPI slave implementation, or
>>> a different
>>> host architecture)
>> Yes, eSPI has the architecture such as transaction layer, link Layer;
>> all of it is about the **silicon**
>> design. That's why I put the driver under /misc directory, not /spi
>> directory.
> I don't see any requirement in the eSPI spec for the upper layers to
> be implemented in hardware. Obviously an x86 host such as Intel's
> PCH would implement the host interface using PIO, and MMIO
> accesses that are compatible with ISA and LPC, as this is the motivation
> behind the specification, but an ARM server that wants to use eSPI
> based peripherals could choose to implement it just as well using
> a traditional SPI master hardware, some GPIOs (reset and alert)
> and a (driver independent) software implementation of the transaction
> and link layers.
>
> On the slave side, it seems that aspeed have implemented the
> virtual wires partially in hardware and require a driver like the one
> you wrote to reply to some of the wires being accessed by the host,
> but again it seems plausible that this could be implemented in another
> BMC using a generic SPI slave and a transaction layer written
> entirely in software.
Yes, you are right, Aspeed have implemented the virtual wires partially.
Tthat's why I named it
as aspeed-espi-slave driver.
> Your driver does not handle the other channels (smbus, mmio, spinor)
> at the moment at all, can you provide some information how they
> are implemented in the ast2500? Are those handled completely
> in hardware (I assume this is the case for spinor at least), or do they
> require help from a driver, either this one or a separate one?
I can't send the AST2500 datasheet to you directly, but you can contact
Aspeed firstly.
https://www.aspeedtech.com/products.php?fPath=20&rId=440
These functions are handled in hardware, the original SDK just provides
some ioctl API for user
application to access them. The mmio function such as KCS / Port 80,
these controllers will get
data from eSPI IP in silicon, but their drivers do not need to be
changed, run the same as LPC
mode.
You can image bellowing work path:
KCS Mailbox Snoop (Port 80) UART ....
^ ^ ^ ^
| | | |
| | | |
\ | / /
{ LPC IP } <-------------------- { eSPI IP to
decode the mmio address }
And in our first generation eSPI x86 server, we just use the eSPI new
function to decode the VW to
boot the PCH (eSPI master). Other functions such as GPIO SMBus, we
didn't use it. So for making
things clean, we just keep the basic code, which is verified and tested
well.
> Arnd
---
BR,
Haiyue
On Tue, Jan 2, 2018 at 4:36 PM, Wang, Haiyue
<[email protected]> wrote:
> On 2018-01-02 23:13, Arnd Bergmann wrote:
>>> On 2017-12-31 07:10, Arnd Bergmann wrote:
>>>> It also seems rather inflexible to have a single driver that is
>>>> responsible both
>>>> for the transport (eSPI register level interface for ASPEED) and the
>>>> high-level
>>>> protocol (talking to an Intel PCH), since either half of the work could
>>>> be
>>>> done elsewhere, using either a different eSPI slave implementation, or
>>>> a different
>>>> host architecture)
>>>
>>> Yes, eSPI has the architecture such as transaction layer, link Layer;
>>> all of it is about the **silicon**
>>> design. That's why I put the driver under /misc directory, not /spi
>>> directory.
>>
>> I don't see any requirement in the eSPI spec for the upper layers to
>> be implemented in hardware. Obviously an x86 host such as Intel's
>> PCH would implement the host interface using PIO, and MMIO
>> accesses that are compatible with ISA and LPC, as this is the motivation
>> behind the specification, but an ARM server that wants to use eSPI
>> based peripherals could choose to implement it just as well using
>> a traditional SPI master hardware, some GPIOs (reset and alert)
>> and a (driver independent) software implementation of the transaction
>> and link layers.
>>
>> On the slave side, it seems that aspeed have implemented the
>> virtual wires partially in hardware and require a driver like the one
>> you wrote to reply to some of the wires being accessed by the host,
>> but again it seems plausible that this could be implemented in another
>> BMC using a generic SPI slave and a transaction layer written
>> entirely in software.
>
> Yes, you are right, Aspeed have implemented the virtual wires partially.
> Tthat's why I named it
> as aspeed-espi-slave driver.
Maybe the name should be more specific and refer to only virtual-wire
rather than espi-slave?
>> Your driver does not handle the other channels (smbus, mmio, spinor)
>> at the moment at all, can you provide some information how they
>> are implemented in the ast2500? Are those handled completely
>> in hardware (I assume this is the case for spinor at least), or do they
>> require help from a driver, either this one or a separate one?
>
> I can't send the AST2500 datasheet to you directly, but you can contact
> Aspeed firstly.
> https://www.aspeedtech.com/products.php?fPath=20&rId=440
>
> These functions are handled in hardware, the original SDK just provides some
> ioctl API for user
> application to access them. The mmio function such as KCS / Port 80, these
> controllers will get
> data from eSPI IP in silicon, but their drivers do not need to be changed,
> run the same as LPC
> mode.
>
> You can image bellowing work path:
>
> KCS Mailbox Snoop (Port 80) UART ....
> ^ ^ ^ ^
> | | | |
> | | | |
> \ | / /
> { LPC IP } <-------------------- { eSPI IP to decode
> the mmio address }
This is all handled by the drivers/misc/aspeed-lpc-snoop.c driver, right?
> And in our first generation eSPI x86 server, we just use the eSPI new
> function to decode the VW to boot the PCH (eSPI master).
>
> Other functions such as GPIO SMBus, we didn't use it. So for making
> things clean, we just keep the basic code, which is verified and tested
> well.
For the upstream submission, having the code verified and tested
is secondary, it most of all must be maintainable in the future ;-)
Your current driver is very simple, which is good: it shouldn't try to be
overly generic and do things we won't ever need, but I want to be
sure that I understand the bigger picture well enough and ensure
that the code is generic enough to do the things we know we will
need.
I see that your documentation only refers to the generic principle of
eSPI, while the driver deals mostly with the aspeed specifics. If we
get a generic virtual-wire implementation based on the spi-slave
framework, the documentation would be the same, and part
of the driver would also be the same. OTOH, if one were to use
the SMBUS over eSPI, the high-level interaction with the vw
would have to be different, and at that point, we'd probably want
an abstraction that can deal with both the aspeed hardware and
a simple spi-slave based implementation.
Superficially, the virtual wires closely resemble GPIOs both on
the host and the slave side, so I wonder if your driver could be
rewritten into a gpiochip driver that implements the slave side of
the eSPI VW on ast2500: make it export a set of GPIO lines,
I suppose you'd need 64 GPIOs to cover all possible
bits in ESPI_SYS_ISR and ESPI_SYS1_ISR, plus an irqchip
to handle the virtual events (ESPI_SYSEVT_HOST_RST_WARN
etc). That would let you separate the simple logic (ack after
warn, boot-done after boot, ...) into one driver or even
user space, and keep the low-level driver specific to ast2500
but otherwise independent of the host side. Do you think that
makes sense?
Arnd
On 2018-01-03 00:23, Arnd Bergmann wrote:
> On Tue, Jan 2, 2018 at 4:36 PM, Wang, Haiyue
> <[email protected]> wrote:
>> On 2018-01-02 23:13, Arnd Bergmann wrote:
>>>> On 2017-12-31 07:10, Arnd Bergmann wrote:
>>>>> It also seems rather inflexible to have a single driver that is
>>>>> responsible both
>>>>> for the transport (eSPI register level interface for ASPEED) and the
>>>>> high-level
>>>>> protocol (talking to an Intel PCH), since either half of the work could
>>>>> be
>>>>> done elsewhere, using either a different eSPI slave implementation, or
>>>>> a different
>>>>> host architecture)
>>>> Yes, eSPI has the architecture such as transaction layer, link Layer;
>>>> all of it is about the **silicon**
>>>> design. That's why I put the driver under /misc directory, not /spi
>>>> directory.
>>> I don't see any requirement in the eSPI spec for the upper layers to
>>> be implemented in hardware. Obviously an x86 host such as Intel's
>>> PCH would implement the host interface using PIO, and MMIO
>>> accesses that are compatible with ISA and LPC, as this is the motivation
>>> behind the specification, but an ARM server that wants to use eSPI
>>> based peripherals could choose to implement it just as well using
>>> a traditional SPI master hardware, some GPIOs (reset and alert)
>>> and a (driver independent) software implementation of the transaction
>>> and link layers.
>>>
>>> On the slave side, it seems that aspeed have implemented the
>>> virtual wires partially in hardware and require a driver like the one
>>> you wrote to reply to some of the wires being accessed by the host,
>>> but again it seems plausible that this could be implemented in another
>>> BMC using a generic SPI slave and a transaction layer written
>>> entirely in software.
>> Yes, you are right, Aspeed have implemented the virtual wires partially.
>> Tthat's why I named it
>> as aspeed-espi-slave driver.
> Maybe the name should be more specific and refer to only virtual-wire
> rather than espi-slave?
We changed Aspeed's reference code about virtual-wire to production
code, which meets
the upstream code style. If other people used new features in eSPI
slave, they can add into
this place one by one, which is proved to work. This is a eSPI slave
start point for Aspeed,
not just virtual wires.
>>> Your driver does not handle the other channels (smbus, mmio, spinor)
>>> at the moment at all, can you provide some information how they
>>> are implemented in the ast2500? Are those handled completely
>>> in hardware (I assume this is the case for spinor at least), or do they
>>> require help from a driver, either this one or a separate one?
>> I can't send the AST2500 datasheet to you directly, but you can contact
>> Aspeed firstly.
>> https://www.aspeedtech.com/products.php?fPath=20&rId=440
>>
>> These functions are handled in hardware, the original SDK just provides some
>> ioctl API for user
>> application to access them. The mmio function such as KCS / Port 80, these
>> controllers will get
>> data from eSPI IP in silicon, but their drivers do not need to be changed,
>> run the same as LPC
>> mode.
>>
>> You can image bellowing work path:
>>
>> KCS Mailbox Snoop (Port 80) UART ....
>> ^ ^ ^ ^
>> | | | |
>> | | | |
>> \ | / /
>> { LPC IP } <-------------------- { eSPI IP to decode
>> the mmio address }
> This is all handled by the drivers/misc/aspeed-lpc-snoop.c driver, right?
This driver just handle port 80. And later may have kcs-bmc.c upstream
from openbmc
project: https://github.com/openbmc/docs/blob/master/kernel-development.md
>> And in our first generation eSPI x86 server, we just use the eSPI new
>> function to decode the VW to boot the PCH (eSPI master).
>>
>> Other functions such as GPIO SMBus, we didn't use it. So for making
>> things clean, we just keep the basic code, which is verified and tested
>> well.
> For the upstream submission, having the code verified and tested
> is secondary, it most of all must be maintainable in the future ;-)
>
> Your current driver is very simple, which is good: it shouldn't try to be
> overly generic and do things we won't ever need, but I want to be
> sure that I understand the bigger picture well enough and ensure
> that the code is generic enough to do the things we know we will
> need.
Sure, people should easily add new features into this file. They just
need add other interrupt
handling here. Currently, we handle the basic interrupt bits.
> I see that your documentation only refers to the generic principle of
> eSPI, while the driver deals mostly with the aspeed specifics. If we
> get a generic virtual-wire implementation based on the spi-slave
> framework, the documentation would be the same, and part
> of the driver would also be the same. OTOH, if one were to use
> the SMBUS over eSPI, the high-level interaction with the vw
> would have to be different, and at that point, we'd probably want
> an abstraction that can deal with both the aspeed hardware and
> a simple spi-slave based implementation.
>
> Superficially, the virtual wires closely resemble GPIOs both on
> the host and the slave side, so I wonder if your driver could be
> rewritten into a gpiochip driver that implements the slave side of
> the eSPI VW on ast2500: make it export a set of GPIO lines,
> I suppose you'd need 64 GPIOs to cover all possible
> bits in ESPI_SYS_ISR and ESPI_SYS1_ISR, plus an irqchip
> to handle the virtual events (ESPI_SYSEVT_HOST_RST_WARN
> etc). That would let you separate the simple logic (ack after
> warn, boot-done after boot, ...) into one driver or even
> user space, and keep the low-level driver specific to ast2500
> but otherwise independent of the host side. Do you think that
> makes sense?
Currently, these virtual wires side-band signals between PCH and BMC
(AST2500) needs to be
handled in time. So we did it in ISR by reading and writing registers.
When this driver is loaded,
then it can handle just in kernel mode, no need a user application. And
the real GPIO pin signal
if transferred by ePSI VW, Aspeed will map these VW values to the GPIO
contorller, so that the
original GPIO driver still work.
> Arnd
On Sun, Dec 31, 2017 at 12:10:51AM +0100, Arnd Bergmann wrote:
> On Fri, Dec 29, 2017 at 2:53 AM, Haiyue Wang
> <[email protected]> wrote:
> > When PCH works under eSPI mode, the PMC (Power Management Controller) in
> > PCH is waiting for SUS_ACK from BMC after it alerts SUS_WARN. It is in
> > dead loop if no SUS_ACK assert. This is the basic requirement for the BMC
> > works as eSPI slave.
> >
> > Also for the host power on / off actions, from BMC side, the following VW
> > (Virtual Wire) messages are done in firmware:
> > 1. SLAVE_BOOT_LOAD_DONE / SLAVE_BOOT_LOAD_STATUS
> > 2. SUS_ACK
> > 3. OOB_RESET_ACK
> > 4. HOST_RESET_ACK
>
> I have not looked at the driver contents yet, but I'm adding the SPI
> maintainer and
> mailing list to Cc here for further discussion. Can you clarify how
More generally:
As documented in SubmittingPatches please send patches to the
maintainers for the code you would like to change. The normal kernel
workflow is that people apply patches from their inboxes, if they aren't
copied they are likely to not see the patch at all and it is much more
difficult to apply patches.
On 2018-01-03 19:38, Mark Brown wrote:
> On Sun, Dec 31, 2017 at 12:10:51AM +0100, Arnd Bergmann wrote:
>> On Fri, Dec 29, 2017 at 2:53 AM, Haiyue Wang
>> <[email protected]> wrote:
>>> When PCH works under eSPI mode, the PMC (Power Management Controller) in
>>> PCH is waiting for SUS_ACK from BMC after it alerts SUS_WARN. It is in
>>> dead loop if no SUS_ACK assert. This is the basic requirement for the BMC
>>> works as eSPI slave.
>>>
>>> Also for the host power on / off actions, from BMC side, the following VW
>>> (Virtual Wire) messages are done in firmware:
>>> 1. SLAVE_BOOT_LOAD_DONE / SLAVE_BOOT_LOAD_STATUS
>>> 2. SUS_ACK
>>> 3. OOB_RESET_ACK
>>> 4. HOST_RESET_ACK
>> I have not looked at the driver contents yet, but I'm adding the SPI
>> maintainer and
>> mailing list to Cc here for further discussion. Can you clarify how
> More generally:
>
> As documented in SubmittingPatches please send patches to the
> maintainers for the code you would like to change. The normal kernel
> workflow is that people apply patches from their inboxes, if they aren't
> copied they are likely to not see the patch at all and it is much more
> difficult to apply patches.
Should send to like this ? Because I add patch for Aspeed chip:
./scripts/get_maintainer.pl drivers/misc/aspeed-lpc-snoop.c
Joel Stanley <[email protected]> (maintainer:ARM/ASPEED MACHINE SUPPORT)
Arnd Bergmann <[email protected]> (supporter:CHAR and MISC DRIVERS)
Greg Kroah-Hartman <[email protected]> (supporter:CHAR and MISC
DRIVERS)
[email protected] (open list)
On Wed, Jan 03, 2018 at 10:28:22PM +0800, Wang, Haiyue wrote:
> Should send to like this ? Because I add patch for Aspeed chip:
> ./scripts/get_maintainer.pl drivers/misc/aspeed-lpc-snoop.c
> Joel Stanley <[email protected]> (maintainer:ARM/ASPEED MACHINE SUPPORT)
> Arnd Bergmann <[email protected]> (supporter:CHAR and MISC DRIVERS)
> Greg Kroah-Hartman <[email protected]> (supporter:CHAR and MISC
> DRIVERS)
> [email protected] (open list)
So it's not actually doing anything at all with the SPI subsystem? I
lacked any context for the discussion having been copied in part way
through. If it is a SPI controller it ought to have been in
drivers/spi.
On 2018-01-03 22:32, Mark Brown wrote:
> On Wed, Jan 03, 2018 at 10:28:22PM +0800, Wang, Haiyue wrote:
>
>> Should send to like this ? Because I add patch for Aspeed chip:
>> ./scripts/get_maintainer.pl drivers/misc/aspeed-lpc-snoop.c
>> Joel Stanley <[email protected]> (maintainer:ARM/ASPEED MACHINE SUPPORT)
>> Arnd Bergmann <[email protected]> (supporter:CHAR and MISC DRIVERS)
>> Greg Kroah-Hartman <[email protected]> (supporter:CHAR and MISC
>> DRIVERS)
>> [email protected] (open list)
> So it's not actually doing anything at all with the SPI subsystem? I
> lacked any context for the discussion having been copied in part way
> through. If it is a SPI controller it ought to have been in
> drivers/spi.
Yes, eSPI just uses the SPI pin definition, but it replaces LPC interface.
On Wed, Jan 3, 2018 at 3:32 PM, Mark Brown <[email protected]> wrote:
> On Wed, Jan 03, 2018 at 10:28:22PM +0800, Wang, Haiyue wrote:
>
>> Should send to like this ? Because I add patch for Aspeed chip:
>
>> ./scripts/get_maintainer.pl drivers/misc/aspeed-lpc-snoop.c
>> Joel Stanley <[email protected]> (maintainer:ARM/ASPEED MACHINE SUPPORT)
>> Arnd Bergmann <[email protected]> (supporter:CHAR and MISC DRIVERS)
>> Greg Kroah-Hartman <[email protected]> (supporter:CHAR and MISC
>> DRIVERS)
>> [email protected] (open list)
>
> So it's not actually doing anything at all with the SPI subsystem? I
> lacked any context for the discussion having been copied in part way
> through. If it is a SPI controller it ought to have been in
> drivers/spi.
It's not an SPI host controller, but a specialized driver for a specialuzed
SPI slave hardware block.
The SPI slave driver implements the higher-level parts of the eSPI protocol
stack in Linux, and the lower levels in hardware. The question is whether (and
how) there should be a split between the levels. If we are expecting to add
a software implementation of the same eSPI stack in software using the generic
SPI slave code in the future, it may be helpful to have that separate in place
already.
With my later suggestion of splitting out the eSPI "virtual wire" low-level
support into a gpiochip driver, neither half would be in drivers/spi/, but
one could implement a drivers/spi/spi-slave-espi-vw.c slave protocol
driver that exposes the same in-kernel interface on top of a slave-capable
SPI controller.
Arnd
On Wed, Jan 3, 2018 at 3:21 AM, Wang, Haiyue
<[email protected]> wrote:
> On 2018-01-03 00:23, Arnd Bergmann wrote:
>> On Tue, Jan 2, 2018 at 4:36 PM, Wang, Haiyue <[email protected]> wrote:
>>> On 2018-01-02 23:13, Arnd Bergmann wrote:
>>>>>
>>>>> On 2017-12-31 07:10, Arnd Bergmann wrote:
>>>> On the slave side, it seems that aspeed have implemented the
>>>> virtual wires partially in hardware and require a driver like the one
>>>> you wrote to reply to some of the wires being accessed by the host,
>>>> but again it seems plausible that this could be implemented in another
>>>> BMC using a generic SPI slave and a transaction layer written
>>>> entirely in software.
>>>
>>> Yes, you are right, Aspeed have implemented the virtual wires partially.
>>> Tthat's why I named it
>>> as aspeed-espi-slave driver.
>>
>> Maybe the name should be more specific and refer to only virtual-wire
>> rather than espi-slave?
>
> We changed Aspeed's reference code about virtual-wire to production code,
> which meets the upstream code style. If other people used new features in eSPI
> slave, they can add into this place one by one, which is proved to work. This is
> a eSPI slave start point for Aspeed, not just virtual wires.
I fear this could tie the application-level protocol too closely to the aspeed
hardware driver. More on that below.
>>> You can image bellowing work path:
>>>
>>> KCS Mailbox Snoop (Port 80) UART ....
>>> ^ ^ ^ ^
>>> | | | |
>>> | | | |
>>> \ | / /
>>> { LPC IP } <-------------------- { eSPI IP to
>>> decode
>>> the mmio address }
>>
>> This is all handled by the drivers/misc/aspeed-lpc-snoop.c driver, right?
>
> This driver just handle port 80. And later may have kcs-bmc.c upstream from
> openbmc
> project: https://github.com/openbmc/docs/blob/master/kernel-development.md
Ok.
>>> And in our first generation eSPI x86 server, we just use the eSPI new
>>> function to decode the VW to boot the PCH (eSPI master).
>>>
>>> Other functions such as GPIO SMBus, we didn't use it. So for making
>>> things clean, we just keep the basic code, which is verified and tested
>>> well.
>>
>> For the upstream submission, having the code verified and tested
>> is secondary, it most of all must be maintainable in the future ;-)
>>
>> Your current driver is very simple, which is good: it shouldn't try to be
>> overly generic and do things we won't ever need, but I want to be
>> sure that I understand the bigger picture well enough and ensure
>> that the code is generic enough to do the things we know we will
>> need.
>
> Sure, people should easily add new features into this file. They just need
> add other interrupt
> handling here. Currently, we handle the basic interrupt bits.
Can you list what other interrupts there are in this hardware block,
and what they relate to? You already said that the MMIO/PIO support
is a separate hardware block that is shared with the LPC slave,
and I guess there is no block for a flash protocol, so is this
VW and SMBUS combined, or does it do more than those two?
>> I see that your documentation only refers to the generic principle of
>> eSPI, while the driver deals mostly with the aspeed specifics. If we
>> get a generic virtual-wire implementation based on the spi-slave
>> framework, the documentation would be the same, and part
>> of the driver would also be the same. OTOH, if one were to use
>> the SMBUS over eSPI, the high-level interaction with the vw
>> would have to be different, and at that point, we'd probably want
>> an abstraction that can deal with both the aspeed hardware and
>> a simple spi-slave based implementation.
>>
>> Superficially, the virtual wires closely resemble GPIOs both on
>> the host and the slave side, so I wonder if your driver could be
>> rewritten into a gpiochip driver that implements the slave side of
>> the eSPI VW on ast2500: make it export a set of GPIO lines,
>> I suppose you'd need 64 GPIOs to cover all possible
>> bits in ESPI_SYS_ISR and ESPI_SYS1_ISR, plus an irqchip
>> to handle the virtual events (ESPI_SYSEVT_HOST_RST_WARN
>> etc). That would let you separate the simple logic (ack after
>> warn, boot-done after boot, ...) into one driver or even
>> user space, and keep the low-level driver specific to ast2500
>> but otherwise independent of the host side. Do you think that
>> makes sense?
>
> Currently, these virtual wires side-band signals between PCH and BMC
> (AST2500) needs to be
> handled in time. So we did it in ISR by reading and writing registers. When
> this driver is loaded,
> then it can handle just in kernel mode, no need a user application. And the
> real GPIO pin signal
> if transferred by ePSI VW, Aspeed will map these VW values to the GPIO
> contorller, so that the
> original GPIO driver still work.
I meant it can be done either in user space or kernel. Doing the
update of the VW can easily be done on top of a GPIO abstraction
when you register an interrupt handler for each VW that is is an
event source, and then sets the registers through gpiolib. On the
hardware side, the interaction is the same, just with a few cycles
added for the separation between the application layer driver
and the hardware specific driver.
Arnd
On 2018-01-04 01:08, Wang, Haiyue wrote:
>
>
>
> On 2018-01-04 00:43, Wang, Haiyue wrote:
>> On 2018-01-03 23:17, Arnd Bergmann wrote:
>>> On Wed, Jan 3, 2018 at 3:21 AM, Wang, Haiyue
>>> <[email protected]> wrote:
>>>> On 2018-01-03 00:23, Arnd Bergmann wrote:
>>>>> On Tue, Jan 2, 2018 at 4:36 PM, Wang, Haiyue<[email protected]> wrote:
>>>>>> On 2018-01-02 23:13, Arnd Bergmann wrote:
>>>>>>>> On 2017-12-31 07:10, Arnd Bergmann wrote:
>>>>>>> On the slave side, it seems that aspeed have implemented the
>>>>>>> virtual wires partially in hardware and require a driver like the one
>>>>>>> you wrote to reply to some of the wires being accessed by the host,
>>>>>>> but again it seems plausible that this could be implemented in another
>>>>>>> BMC using a generic SPI slave and a transaction layer written
>>>>>>> entirely in software.
>>>>>> Yes, you are right, Aspeed have implemented the virtual wires partially.
>>>>>> Tthat's why I named it
>>>>>> as aspeed-espi-slave driver.
>>>>> Maybe the name should be more specific and refer to only virtual-wire
>>>>> rather than espi-slave?
>>>> We changed Aspeed's reference code about virtual-wire to production code,
>>>> which meets the upstream code style. If other people used new features in eSPI
>>>> slave, they can add into this place one by one, which is proved to work. This is
>>>> a eSPI slave start point for Aspeed, not just virtual wires.
>>> I fear this could tie the application-level protocol too closely to the aspeed
>>> hardware driver. More on that below.
>>
>> Looks like yes, for eSPI is new thing, not sure other BMC chip
>> company how to design the eSPI slave.
>>
>>>>>> You can image bellowing work path:
>>>>>>
>>>>>> KCS Mailbox Snoop (Port 80) UART ....
>>>>>> ^ ^ ^ ^
>>>>>> | | | |
>>>>>> | | | |
>>>>>> \ | / /
>>>>>> { LPC IP } <-------------------- { eSPI IP to
>>>>>> decode
>>>>>> the mmio address }
>>>>> This is all handled by the drivers/misc/aspeed-lpc-snoop.c driver, right?
>>>> This driver just handle port 80. And later may have kcs-bmc.c upstream from
>>>> openbmc
>>>> project:https://github.com/openbmc/docs/blob/master/kernel-development.md
>>> Ok.
>>>
>>>>>> And in our first generation eSPI x86 server, we just use the eSPI new
>>>>>> function to decode the VW to boot the PCH (eSPI master).
>>>>>>
>>>>>> Other functions such as GPIO SMBus, we didn't use it. So for making
>>>>>> things clean, we just keep the basic code, which is verified and tested
>>>>>> well.
>>>>> For the upstream submission, having the code verified and tested
>>>>> is secondary, it most of all must be maintainable in the future ;-)
>>>>>
>>>>> Your current driver is very simple, which is good: it shouldn't try to be
>>>>> overly generic and do things we won't ever need, but I want to be
>>>>> sure that I understand the bigger picture well enough and ensure
>>>>> that the code is generic enough to do the things we know we will
>>>>> need.
>>>> Sure, people should easily add new features into this file. They just need
>>>> add other interrupt
>>>> handling here. Currently, we handle the basic interrupt bits.
>>> Can you list what other interrupts there are in this hardware block,
>>> and what they relate to? You already said that the MMIO/PIO support
>>> is a separate hardware block that is shared with the LPC slave,
>>> and I guess there is no block for a flash protocol, so is this
>>> VW and SMBUS combined, or does it do more than those two?
>>
>> Such as:
>> Flash Channel Tx Error
>> OOB Channel Tx Error
>> Flash Channel Tx Abort
>> OOB Channel Tx Abort
>> Peripheral Channel Non-Posted Tx Abort
>> Peripheral Channel Posted Tx Abort
>> Virtual Wire GPIO Event
>> ...
>>
>>>>> I see that your documentation only refers to the generic principle of
>>>>> eSPI, while the driver deals mostly with the aspeed specifics. If we
>>>>> get a generic virtual-wire implementation based on the spi-slave
>>>>> framework, the documentation would be the same, and part
>>>>> of the driver would also be the same. OTOH, if one were to use
>>>>> the SMBUS over eSPI, the high-level interaction with the vw
>>>>> would have to be different, and at that point, we'd probably want
>>>>> an abstraction that can deal with both the aspeed hardware and
>>>>> a simple spi-slave based implementation.
>>>>>
>>>>> Superficially, the virtual wires closely resemble GPIOs both on
>>>>> the host and the slave side, so I wonder if your driver could be
>>>>> rewritten into a gpiochip driver that implements the slave side of
>>>>> the eSPI VW on ast2500: make it export a set of GPIO lines,
>>>>> I suppose you'd need 64 GPIOs to cover all possible
>>>>> bits in ESPI_SYS_ISR and ESPI_SYS1_ISR, plus an irqchip
>>>>> to handle the virtual events (ESPI_SYSEVT_HOST_RST_WARN
>>>>> etc). That would let you separate the simple logic (ack after
>>>>> warn, boot-done after boot, ...) into one driver or even
>>>>> user space, and keep the low-level driver specific to ast2500
>>>>> but otherwise independent of the host side. Do you think that
>>>>> makes sense?
>>>> Currently, these virtual wires side-band signals between PCH and BMC
>>>> (AST2500) needs to be
>>>> handled in time. So we did it in ISR by reading and writing registers. When
>>>> this driver is loaded,
>>>> then it can handle just in kernel mode, no need a user application. And the
>>>> real GPIO pin signal
>>>> if transferred by ePSI VW, Aspeed will map these VW values to the GPIO
>>>> contorller, so that the
>>>> original GPIO driver still work.
>>> I meant it can be done either in user space or kernel. Doing the
>>> update of the VW can easily be done on top of a GPIO abstraction
>>> when you register an interrupt handler for each VW that is is an
>>> event source, and then sets the registers through gpiolib. On the
>>> hardware side, the interaction is the same, just with a few cycles
>>> added for the separation between the application layer driver
>>> and the hardware specific driver.
>>
>> In practice, we load this driver as soon as possible, so that the
>> eSPI master can make PMC in PCH
>> to exit G3 state, which is said in the patch commit patch. So that
>> other drivers such as KCS, Snoop
>> can work in time for powering on the host. Simple should be better
>> for embedded system ? ;-)
>>
>
> And if we design the VW as gpio, looks like that the developers need
> to design their own application
> to handle the VWs. This makes things worse in my understanding. They
> have to look into the eSPI
> spec in detail, and in fact, this VW handing is not easily to
> understand. For they are about Intel's PCH
> power side-band signal handling. ;-)
>
>>> Arnd
>>
>