From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Google-Smtp-Source: ACJfBov6uowY3PEa+jYaQdorgte12V1y36LCv+pWUk3266RBJ8kuPpuDyKwF8CryyY7OAGLHb3yI ARC-Seal: i=1; a=rsa-sha256; t=1514997798; cv=none; d=google.com; s=arc-20160816; b=hlnLVytAt9vQrWEfaVHIyhY9/4EGNIsI5Q8WRg47Pvqa2udHL8WxbG3k8v7CcMPxrG EvnOqLkiOH83PafUMhgzYhnF2LVkM3DEOvg+Go0ty8rlYR1juqRCNOI3vvD7gGIzNHNX URXT6alSvlg5M2JXey6d0eVP/bq+FMplAmn2+Ob1SHmskcQguG1riC3elWyAXONP9kI+ z0EOn4LXbkWAukIz7Xg9GoRi1n2uMgPYvIqGAsS0NS1r2GUP7OeJ6DG36p5rzzSoUtPV ZryrhNUGfV9WtSv++4CoQAuLKmTlzG5WITgh4dvV/K5hFC2XtYI8tTAPgUpN/o2/o3VD /jNA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject:arc-authentication-results; bh=bdjASe6v2qYVgmvXep0lPsESYVbBwi1hLTE3sxuRwWE=; b=iOUnfjkdKdbJn7DXAavesODEh6PYDZH1XZveP0/FODsVEwcraYNEqkBXK5U7ljMRcA 0cHunlmN/WCaRWdPlONkPYqcHsiaS2W2EsOlM5oYozU3/dvutu5TMh4tdG2NuWkcdN0G IsLvvOVLa9Ojuwe445l3I+2LcOdXs9rLPhMHR7qzUFhp9Mc+rDFaY+G7xnpJwULONCHO 9f2zP0EG8e/YqyiYcLcFXzkht2APMYBTVePUOVbXdFk3dpLtYCfehrKW3oXl268+dGh9 QFBBH3qI+3kvAyAYQRhtrHk7Wfuf0q0S5hxNzfRuyPRhyKMDtooBluZeNjng9hZixuPA opRw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of haiyue.wang@linux.intel.com designates 134.134.136.65 as permitted sender) smtp.mailfrom=haiyue.wang@linux.intel.com Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of haiyue.wang@linux.intel.com designates 134.134.136.65 as permitted sender) smtp.mailfrom=haiyue.wang@linux.intel.com X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,501,1508828400"; d="scan'208,217";a="6965866" Subject: Re: [PATCH v1] eSPI: add Aspeed AST2500 eSPI driver to boot a host with PCH runs on eSPI 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> From: "Wang, Haiyue" Message-ID: Date: Thu, 4 Jan 2018 00:43:14 +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: multipart/alternative; boundary="------------262CFE4FDF9C4A852DD3A1C0" Content-Language: en-US X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: =?utf-8?q?1588081316766464367?= X-GMAIL-MSGID: =?utf-8?q?1588590330975098673?= X-Mailing-List: linux-kernel@vger.kernel.org List-ID: This is a multi-part message in MIME format. --------------262CFE4FDF9C4A852DD3A1C0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit 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 ? ;-) > Arnd --------------262CFE4FDF9C4A852DD3A1C0 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 8bit
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 ? ;-)

    

    

      
        Arnd


    

    

  


--------------262CFE4FDF9C4A852DD3A1C0--