arm: aspeed: Add eSPI support

The Aspeed eSPI controller is slave device to communicate with the master through the Enhanced Serial Peripheral Interface (eSPI). All of the four eSPI channels, namely peripheral, virtual wire, out-of-band, and flash are supported.

From: Jeremy Kerr <[email protected]>
Sent: Thursday, September 2, 2021 11:30 AM

Hi Chiawei,

Quoted 4 lines

The Aspeed eSPI controller is slave device to communicate with the master through the Enhanced Serial Peripheral Interface (eSPI). All of the four eSPI channels, namely peripheral, virtual wire, out-of-band, and flash are supported.

I'm still not confinced this raw packet user-ABI is the right approach for this. The four channels that you're exposing would be much more useful to use existing kernel subsystems.

Specifically:

1) The VW channel is essentially a GPIO interface, and we have a kernel subsystem to expose GPIOs. We might want to add additional support for in-kernel system event handlers, but that's a minor addition that can be done separately if we don't want that handled by a gpio consumer in userspace.

2) The out-of-band (OOB) channel provides a way to issue SMBus transactions, so could well provide an i2c controller interface. Additionally, the eSPI specs imply that this is intended to support MCTP - in which case, you'll *need* a kernel i2c controller device to be able to use the new kernel MCTP stack.

3) The flash channel exposes read/write/erase operations, which would be much more useful as an actual flash-type device, perhaps using the existing mtd interface? Or is there additional functionality expected for this?

4) The peripheral channel is the only one that would seem to require arbitrary cycle access, but we'll still need a proper uapi definition to support that. At the minimum, your ioctl definitions should go under include/uapi/ - you shouldn't need to duplicate the header into each userspace repo, as you've done for the test examples.

The eSPI on BMC side is a slave device. Most of time it listen to the Host requests and then react.
This makes it not quite fit to interfaces that act as a master role.

Quoted 5 lines

1) The VW channel is essentially a GPIO interface, and we have a kernel ?? subsystem to expose GPIOs. We might want to add additional support ?? for in-kernel system event handlers, but that's a minor addition that ?? can be done separately if we don't want that handled by a gpio ?? consumer in userspace.

eSPI VW channel can be used to forward a byte value to/from GPIO.

Quoted 8 lines

2) The out-of-band (OOB) channel provides a way to issue SMBus ?? transactions, so could well provide an i2c controller interface. ?? Additionally, the eSPI specs imply that this is intended to support
?? MCTP - in which case, you'll *need* a kernel i2c controller device to
?? be able to use the new kernel MCTP stack.

Could you share us more information about the i2c need of kernel MCTP kernel stack?

Quoted 6 lines

3) The flash channel exposes read/write/erase operations, which would be
?? much more useful as an actual flash-type device, perhaps using the
?? existing mtd interface? Or is there additional functionality ?? expected for this?

The flash channel works in either the Master Attached Flash Sharing (MAFS) or Slave Attached Flash Sharing (SAFS) mode.

For MAFS, BMC issues eSPI flash R/W/E packets to the Host. In this case, it may fit into the MTD interface.

For SAFS (mostly used), BMC needs to listen, parse and filter eSPI flash R/W/E packets from the Host.
And then send replies in the eSPI success/unsuccess completion packet format.
This behaves differently from MTD.

To support both the two scenario, the MTD interface might not be suitable.

Quoted 6 lines

4) The peripheral channel is the only one that would seem to require arbitrary cycle access, but we'll still need a proper uapi definition to support that. At the minimum, your ioctl definitions should go under include/uapi/ - you shouldn't need to duplicate the header into each userspace repo, as you've done for the test examples.

In the first submission, I was told that the include/uapi patch is not going to be accepted.

Quoted 1 line

 create mode 100644 include/uapi/linux/aspeed-espi.h

This userspace interface is not going to be accepted upstream.

In summary, considering the various applications that might be constructed upon eSPI transaction,
we thought that the raw packet paradigm might be the first step to start with.

From: Jeremy Kerr <[email protected]>
Sent: Thursday, September 2, 2021 3:05 PM

Hi Chiawei,

Quoted 3 lines

The eSPI on BMC side is a slave device. Most of time it listen to the Host requests and then react.
This makes it not quite fit to interfaces that act as a master role.

That's a good point, but I don't think it precludes using existing interfaces.

Quoted 10 lines

Quoted 6 lines

1) The VW channel is essentially a GPIO interface, and we have a kernel ?? subsystem to expose GPIOs. We might want to add additional support ?? for in-kernel system event handlers, but that's a minor addition that ?? can be done separately if we don't want that handled by a gpio ?? consumer in userspace.

eSPI VW channel can be used to forward a byte value to/from GPIO. However, the targeted GPIO group and its direction are determined by the Host. This is different from usual GPIO devices as it supports very limited operations.

I'm not referring to the hardware GPIOs that can be connected here, rather the logic values represented over the VW channel. The eSPI master is transferring IO states over the channel, and we could represnt those on the BMC side as "virtual" GPIOs.

If that model doesn't fit though, that's OK, but I think we need some rationale there.

Quoted 10 lines

Quoted 9 lines

2) The out-of-band (OOB) channel provides a way to issue SMBus ?? transactions, so could well provide an i2c controller interface. ?? Additionally, the eSPI specs imply that this is intended to support
?? MCTP - in which case, you'll *need* a kernel i2c controller device to
?? be able to use the new kernel MCTP stack.

Quoted 3 lines

Could you share us more information about the i2c need of kernel MCTP kernel stack? To our understanding, the MCTP is a bus agnostic protocol.
A generic raw packet interface makes it more flexible to adapt to different interfaces.

The currently proposed mctp-i2c interface driver binds to a kernel i2c device. If you expose a kernel i2c device here, we can connect that as an MCTP interface.

With the packet interface proposed here, we can't do that, and would need a whole new driver for this, implemented in userspace. The same would apply to any other usage of the i2c bus (EEPROM access, etc).

Quoted 14 lines

Quoted 7 lines

3) The flash channel exposes read/write/erase operations, which would be
?? much more useful as an actual flash-type device, perhaps using the
?? existing mtd interface? Or is there additional functionality ?? expected for this?

The flash channel works in either the Master Attached Flash Sharing (MAFS) or Slave Attached Flash Sharing (SAFS) mode.

For MAFS, BMC issues eSPI flash R/W/E packets to the Host. In this case, it may fit into the MTD interface.

Quoted 8 lines

For SAFS (mostly used), BMC needs to listen, parse and filter eSPI flash R/W/E packets from the Host.
And then send replies in the eSPI success/unsuccess completion packet format.
This behaves differently from MTD.

To support both the two scenario, the MTD interface might not be suitable.

OK, that makes sense. In this case the BMC is acting as the virtual flash device, right?

Quoted 9 lines

Quoted 6 lines

4) The peripheral channel is the only one that would seem to require arbitrary cycle access, but we'll still need a proper uapi definition to support that. At the minimum, your ioctl definitions should go under include/uapi/ - you shouldn't need to duplicate the header into each userspace repo, as you've done for the test examples.

In the first submission, I was told that the include/uapi patch is not going to be accepted.

This is what you were told in the v1 submission:

Quoted 3 lines

Quoted 1 line

 create mode 100644 include/uapi/linux/aspeed-espi.h

This userspace interface is not going to be accepted upstream.

It sees like that was a comment about the API itself, not the location of the header (Rob, correct me if I'm wrong). Simply moving the header out of the uapi directory, while retaining the same API, is not a solution to this.

Quoted 3 lines

In summary, considering the various applications that might be constructed upon eSPI transaction, we thought that the raw packet paradigm might be the first step to start with.

Keep in mind that you're creating a kernel ABI here, which has to stay in place forever - even if it's the first step to something else, we cannot break future compatibility.

Quoted 3 lines

If that model doesn't fit though, that's OK, but I think we need some rationale
there.

After an internal discussion, we found that our eSPI VW device may not fit into existing GPIO model.

The reason is that GPIO direction changes through VW channel has no interrupts triggered.
And the direction is controlled by the Host as aforementioned.

A raw packet, primitive interface should have better flexibility to manage MCTP packets over the OOB channel.

From: Jeremy Kerr <[email protected]>
Sent: Monday, September 6, 2021 11:17 AM

Hi Chiawei,

Quoted 9 lines

Quoted 2 lines

If that model doesn't fit though, that's OK, but I think we need some rationale there.

After an internal discussion, we found that our eSPI VW device may not fit into existing GPIO model.

The reason is that GPIO direction changes through VW channel has no interrupts triggered.
And the direction is controlled by the Host as aforementioned.

This piqued my curiosity, so I had a look through the 2500 datasheet. It appears that the host has full control of both the direction *and* hardware GPIO assignment though the platform-specific eSPI configuration register set.

So, with VW GPIOs in hardware mode (ESPICTRL[9] = 0, the default), the host has arbitrary control of all hardware GPIO lines (except for the GPIOAC bank, I guess?).

There's a huge security implication there - for example, GPIOs that assert physical presence can now be set by the host, possibly remotely - so I'd *strongly* recommend that we always get ESPICTRL[9] to 1, to set software-only mode.

With than in mind, if we're disabling hardware mode - what does the direction control setting effect when we're in software mode (ESPICTRL[9] == 1)? Does it even matter?

For example, what happens when the host goes a GET_VW cycle for a GPIO that is marked as 'master-to-slave' mode? Is the state of the GPIO in ESPI09C still reported?

If that's the case, then we can just ignore the direction settings from ESPICFG800 completely, and have the BMC assign directions to standard gpiodevs as appropriate.

Separate from this: I'm also proposing that we represent the system event VWs as gpiodevs as well.

Quoted 2 lines

A raw packet, primitive interface should have better flexibility to manage MCTP packets over the OOB channel.

OK, let me phrase this differently: can the OOB channel be used for anything other than SMBus messaging? Is it useful to provide an interface that isn't a standard SMBus/i2c device?

If you need custom uapi definitions for this driver, that might be okay, but it's going to be more work for you (to define an interface that can be supported long-term), rather than using standard infrastructure that already exists.

Yes, there is security concern using HW mode.
Our designer is considering to remove the HW mode support in the next generation of Aspeed SoCs.
So far we haven't encountered a scenario demanding HW mode.

Quoted 3 lines

With than in mind, if we're disabling hardware mode - what does the direction control setting effect when we're in software mode (ESPICTRL[9] == 1)? Does it even matter?

Yes, the direction matters even in SW mode.
When the direction is 'master-to-slave' and the GPIO value is updated by the Host through PUT_VW, a VW interrupt is trigger to notify BMC. For the 'slave-to-master' GPIO, a alert is generated to notify the Host to issue GET_VW for the GPIO value updated by the BMC by ESPI09C.

Quoted 8 lines

Separate from this: I'm also proposing that we represent the system event VWs
as gpiodevs as well.

Quoted 3 lines

A raw packet, primitive interface should have better flexibility to
manage MCTP packets over the OOB channel.

Quoted 3 lines

OK, let me phrase this differently: can the OOB channel be used for anything other than SMBus messaging? Is it useful to provide an interface that isn't a standard SMBus/i2c device?

Yes, the PCH spec. also defines two additional packet format for an eSPI slave to retrieve PCH Temperature Data and RTC time. It should be trivial to prepare a byte buffer in that format and send it through the raw packet interface.

Quoted 4 lines

If you need custom uapi definitions for this driver, that might be okay, but it's going to be more work for you (to define an interface that can be supported long-term), rather than using standard infrastructure that already exists.

Thus I suggested that we can refer to the IPMI KCS BMC driver, which supports the selection of different user interfaces, RAW or IPMI.

If IOCTL is considered to be not user friendly or magic code polluting, file-based read/write on the miscdevice node is also an option.

From: Jeremy Kerr <[email protected]>
Sent: Thursday, September 9, 2021 9:53 AM

Quoted 4 lines

Yes, there is security concern using HW mode.
Our designer is considering to remove the HW mode support in the next generation of Aspeed SoCs.
So far we haven't encountered a scenario demanding HW mode.

Great to hear :) can we unconditionally set ESPI000[9] in the driver then?

Quoted 5 lines

Quoted 4 lines

With than in mind, if we're disabling hardware mode - what does the direction control setting effect when we're in software mode (ESPICTRL[9] == 1)? Does it even matter?

Quoted 5 lines

Yes, the direction matters even in SW mode.
When the direction is 'master-to-slave' and the GPIO value is updated by the Host through PUT_VW, a VW interrupt is trigger to notify BMC. For the 'slave-to-master' GPIO, a alert is generated to notify the Host to issue GET_VW for the GPIO value updated by the BMC by ESPI09C.

OK, but the datasheet mentions that ESPICFG804 is only applicable when ESPI000[9] = 0, or is that not the case?

But based on what you've said: yes, it sounds like the generic gpiodev parts won't be useful for this.

Quoted 6 lines

Quoted 6 lines

Separate from this: I'm also proposing that we represent the system event VWs as gpiodevs as well.

Quoted 2 lines

A raw packet, primitive interface should have better flexibility to manage MCTP packets over the OOB channel.

Quoted 8 lines

Quoted 3 lines

OK, let me phrase this differently: can the OOB channel be used for anything other than SMBus messaging? Is it useful to provide an interface that isn't a standard SMBus/i2c device?

Yes, the PCH spec. also defines two additional packet format for an eSPI slave to retrieve PCH Temperature Data and RTC time. It should be trivial to prepare a byte buffer in that format and send it through the raw packet interface.

OK, understood.

Quoted 7 lines

Quoted 4 lines

If you need custom uapi definitions for this driver, that might be okay, but it's going to be more work for you (to define an interface that can be supported long-term), rather than using standard infrastructure that already exists.

Thus I suggested that we can refer to the IPMI KCS BMC driver, which supports the selection of different user interfaces, RAW or IPMI.

Yep, but the KCS "raw" register set is standardised as part of the IPMI spec too, which helps to define a stable user API. We don't have that in this case.

Overall though, if you want to start with the "low-level" API, then introduce "enhanced" functionality - like an actual SMBus driver - alongside that, then that sounds like an OK approach.

Quoted 3 lines

If IOCTL is considered to be not user friendly or magic code polluting, file-based read/write on the miscdevice node is also an option.

It's not really my decision to make, but a read/write event interface would seem to be more consistent to me. Is there an obvious event format that would be common across all channels, perhaps? We'd probably also need a poll too - to make use of incoming events, like master-to-slave VW changes, perhaps?

Add eSPI to the device tree for Aspeed 5/6th generation SoCs.

Signed-off-by Chia-Wei Wang <[email protected]>

---
?arch/arm/boot/dts/aspeed-g6.dtsi | 17 +++++++++++++++++ ?1 file changed, 17 insertions(+)

arch/arm/boot/dts/aspeed-g6.dtsi +0 -0

index f96607b7b4e2..47dc0b3993d1 100644
@@ -364,6 +364,23 @@
 ????????????????????????????????status = "disabled";
 ????????????????????????};

?
+???????????????????????espi: espi at 1e6ee000 { +???????????????????????????????compatible = "aspeed,ast2600-espi", "simple-mfd", "syscon"; +???????????????????????????????reg = <0x1e6ee000 0x1000>;

arch/arm/boot/dts/aspeed-g6.dtsi +7 -0

+
+???????????????????????????????#address-cells = <1>;
+???????????????????????????????#size-cells = <1>;
+???????????????????????????????ranges = <0x0 0x1e6ee000 0x1000>;
+
+???????????????????????????????espi_ctrl: espi-ctrl at 0 {
+???????????????????????????????????????compatible = "aspeed,ast2600-

espi-ctrl"; +???????????????????????????????????????reg = <0x0 0x800>; +???????????????????????????????????????interrupts = <GIC_SPI 42 IRQ_TYPE_LEVEL_HIGH>; +???????????????????????????????????????clocks = <&syscon ASPEED_CLK_GATE_ESPICLK>; +???????????????????????????????????????status = "disabled"; +???????????????????????????????}; +???????????????????????};
+
????????????????????????gpio0: gpio at 1e780000 { ????????????????????????????????#gpio-cells = <2>; ????????????????????????????????gpio-controller;

From: Andrei Kartashev <[email protected]>
Sent: Wednesday, November 10, 2021 7:21 PM

Hi Chia-Wei,

How is it about g5? Why did you add definition only for g6 here?

Quoted 13 lines · Chia-Wei Wang

Add eSPI to the device tree for Aspeed 5/6th generation SoCs.

Signed-off-by Chia-Wei Wang <[email protected]>

---
?arch/arm/boot/dts/aspeed-g6.dtsi | 17 +++++++++++++++++ ?1 file changed, 17 insertions(+)

arch/arm/boot/dts/aspeed-g6.dtsi +0 -0

index f96607b7b4e2..47dc0b3993d1 100644

arch/arm/boot/dts/aspeed-g6.dtsi line 368

Quoted 5 lines

arch/arm/boot/dts/aspeed-g6.dtsi +2 -0

@@ -364,6 +364,23 @@
 ????????????????????????????????status = "disabled";
 ????????????????????????};
 
+???????????????????????espi: espi at 1e6ee000 {
+???????????????????????????????compatible =

"aspeed,ast2600-espi",

arch/arm/boot/dts/aspeed-g6.dtsi line 370

Quoted 2 lines

arch/arm/boot/dts/aspeed-g6.dtsi +1 -0

 "simple-mfd", "syscon";
+???????????????????????????????reg = <0x1e6ee000

0x1000>;

arch/arm/boot/dts/aspeed-g6.dtsi line 374

Quoted 4 lines

arch/arm/boot/dts/aspeed-g6.dtsi +4 -0

+
+???????????????????????????????#address-cells = <1>;
+???????????????????????????????#size-cells = <1>;
+???????????????????????????????ranges = <0x0 0x1e6ee000

0x1000>;

arch/arm/boot/dts/aspeed-g6.dtsi line 377

Quoted 3 lines

arch/arm/boot/dts/aspeed-g6.dtsi +3 -0

+
+???????????????????????????????espi_ctrl: espi-ctrl at 0 {
+???????????????????????????????????????compatible =

"aspeed,ast2600-

arch/arm/boot/dts/aspeed-g6.dtsi line 379

Quoted 2 lines

arch/arm/boot/dts/aspeed-g6.dtsi +1 -0

 espi-ctrl";
+???????????????????????????????????????reg = <0x0

0x800>;

arch/arm/boot/dts/aspeed-g6.dtsi line 380

Quoted 1 line

arch/arm/boot/dts/aspeed-g6.dtsi +1 -0

+???????????????????????????????????????interrupts =

<GIC_SPI 42

arch/arm/boot/dts/aspeed-g6.dtsi line 382

Quoted 2 lines

arch/arm/boot/dts/aspeed-g6.dtsi +1 -0

 IRQ_TYPE_LEVEL_HIGH>;
+???????????????????????????????????????clocks =

<&syscon

Quoted 2 lines

ASPEED_CLK_GATE_ESPICLK>;
+???????????????????????????????????????status =

"disabled";

Quoted 6 lines

+???????????????????????????????}; +???????????????????????};
+
????????????????????????gpio0: gpio at 1e780000 { ????????????????????????????????#gpio-cells = <2>; ????????????????????????????????gpio-controller;

--
Best regards,
Andrei Kartashev