Messages in this thread | | | Subject | Re: [PATCH v1] eSPI: add Aspeed AST2500 eSPI driver to boot a host with PCH runs on eSPI | From | "Wang, Haiyue" <> | Date | Thu, 4 Jan 2018 08:11:53 +0800 |
| |
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@linux.intel.com> wrote: >>>> On 2018-01-03 00:23, Arnd Bergmann wrote: >>>>> On Tue, Jan 2, 2018 at 4:36 PM, Wang, Haiyue<haiyue.wang@linux.intel.com> 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 >> >
| |