Date:   Wed, 3 Jan 2018 10:21:03 +0800
From:   "Wang, Haiyue" <haiyue.wang@...ux.intel.com>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Joel Stanley <joel@....id.au>, gregkh <gregkh@...uxfoundation.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Mark Brown <broonie@...nel.org>,
        linux-spi <linux-spi@...r.kernel.org>
Subject: Re: [PATCH v1] eSPI: add Aspeed AST2500 eSPI driver to boot a host
 with PCH runs on eSPI



On 2018-01-03 00:23, Arnd Bergmann wrote:
> On Tue, Jan 2, 2018 at 4:36 PM, Wang, Haiyue
> <haiyue.wang@...ux.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?
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

Powered by blists - more mailing lists