Re: [PATCH v1] eSPI: add Aspeed AST2500 eSPI driver to boot a host with PCH runs on eSPI

["Wang, Haiyue" <haiyue.wang@xxxxxxxxxxxxxxx>]

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

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html