Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751706AbeADAML (ORCPT + 1 other); Wed, 3 Jan 2018 19:12:11 -0500 Received: from mga11.intel.com ([192.55.52.93]:31795 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751397AbeADAMJ (ORCPT ); Wed, 3 Jan 2018 19:12:09 -0500 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,504,1508828400"; d="scan'208";a="7062636" Subject: Re: [PATCH v1] eSPI: add Aspeed AST2500 eSPI driver to boot a host with PCH runs on eSPI From: "Wang, Haiyue" To: Arnd Bergmann Cc: Joel Stanley , gregkh , Linux Kernel Mailing List , Mark Brown , linux-spi , "Shevchenko, Andriy" References: <1514512387-27113-1-git-send-email-haiyue.wang@linux.intel.com> Message-ID: Date: Thu, 4 Jan 2018 08:11:53 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: 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 >>> wrote: >>>> On 2018-01-03 00:23, Arnd Bergmann wrote: >>>>> On Tue, Jan 2, 2018 at 4:36 PM, Wang, Haiyue 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 >> >