On Tue, Jan 2, 2018 at 4:36 PM, Wang, Haiyue <haiyue.w...@linux.intel.com> 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
