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

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

[Arnd Bergmann]

On Fri, Dec 29, 2017 at 2:53 AM, Haiyue Wang
<haiyue.wang-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> When PCH works under eSPI mode, the PMC (Power Management Controller) in
> PCH is waiting for SUS_ACK from BMC after it alerts SUS_WARN. It is in
> dead loop if no SUS_ACK assert. This is the basic requirement for the BMC
> works as eSPI slave.
>
> Also for the host power on / off actions, from BMC side, the following VW
> (Virtual Wire) messages are done in firmware:
> 1. SLAVE_BOOT_LOAD_DONE / SLAVE_BOOT_LOAD_STATUS
> 2. SUS_ACK
> 3. OOB_RESET_ACK
> 4. HOST_RESET_ACK

I have not looked at the driver contents yet, but I'm adding the SPI
maintainer and
mailing list to Cc here for further discussion. Can you clarify how
the eSPI slave
mode relates to SPI slaves that we already support? I was under the impression
that the difference between SPI and eSPI is mainly on the master side, but that
any SPI slave can also act as an eSPI slave. Would this driver fit into the SPI
slave framework, possibly with some extensions to the generic abstraction?

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)

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

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

[Arnd Bergmann]

> On 2017-12-31 07:10, Arnd Bergmann wrote:
> > On Fri, Dec 29, 2017 at 2:53 AM, Haiyue Wang
> > <haiyue.wang@linux.intel.com> wrote:
> >> When PCH works under eSPI mode, the PMC (Power Management Controller) in
> >> PCH is waiting for SUS_ACK from BMC after it alerts SUS_WARN. It is in
> >> dead loop if no SUS_ACK assert. This is the basic requirement for the BMC
> >> works as eSPI slave.
> >>
> >> Also for the host power on / off actions, from BMC side, the following VW
> >> (Virtual Wire) messages are done in firmware:
> >> 1. SLAVE_BOOT_LOAD_DONE / SLAVE_BOOT_LOAD_STATUS
> >> 2. SUS_ACK
> >> 3. OOB_RESET_ACK
> >> 4. HOST_RESET_ACK
> > I have not looked at the driver contents yet, but I'm adding the SPI
> > maintainer and
> > mailing list to Cc here for further discussion. Can you clarify how
> > the eSPI slave
> > mode relates to SPI slaves that we already support? I was under the impression
> > that the difference between SPI and eSPI is mainly on the master side, but that
> > any SPI slave can also act as an eSPI slave. Would this driver fit into the SPI
> > slave framework, possibly with some extensions to the generic abstraction?
> In simple word, the eSPI uses the SPI interface pin definition, but it
> will replace Low Pin Count (LPC)
> interface. From its name, sure, it will confuse you! ;-)

I know what eSPI is meant for, and understand the basic idea of the
protocol, but I'm not familiar with the Apeed slave hardware
implementation.

> > 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.

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?

       Arnd

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

[Wang, Haiyue]

On 2018-01-02 23:13, Arnd Bergmann wrote:
>> On 2017-12-31 07:10, Arnd Bergmann wrote:
>>> On Fri, Dec 29, 2017 at 2:53 AM, Haiyue Wang
>>> <haiyue.wang@linux.intel.com> wrote:
>>>> When PCH works under eSPI mode, the PMC (Power Management Controller) in
>>>> PCH is waiting for SUS_ACK from BMC after it alerts SUS_WARN. It is in
>>>> dead loop if no SUS_ACK assert. This is the basic requirement for the BMC
>>>> works as eSPI slave.
>>>>
>>>> Also for the host power on / off actions, from BMC side, the following VW
>>>> (Virtual Wire) messages are done in firmware:
>>>> 1. SLAVE_BOOT_LOAD_DONE / SLAVE_BOOT_LOAD_STATUS
>>>> 2. SUS_ACK
>>>> 3. OOB_RESET_ACK
>>>> 4. HOST_RESET_ACK
>>> I have not looked at the driver contents yet, but I'm adding the SPI
>>> maintainer and
>>> mailing list to Cc here for further discussion. Can you clarify how
>>> the eSPI slave
>>> mode relates to SPI slaves that we already support? I was under the impression
>>> that the difference between SPI and eSPI is mainly on the master side, but that
>>> any SPI slave can also act as an eSPI slave. Would this driver fit into the SPI
>>> slave framework, possibly with some extensions to the generic abstraction?
>> In simple word, the eSPI uses the SPI interface pin definition, but it
>> will replace Low Pin Count (LPC)
>> interface. From its name, sure, it will confuse you! ;-)
> I know what eSPI is meant for, and understand the basic idea of the
> protocol, but I'm not familiar with the Apeed slave hardware
> implementation.
I see! ;-)
>>> 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.
> 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 }

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.
>         Arnd
---
BR,
Haiyue

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

[Arnd Bergmann]

On Tue, Jan 2, 2018 at 4:36 PM, Wang, Haiyue
<haiyue.wang-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> 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?

>> 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?

> 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.

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?

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

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

[Wang, Haiyue]

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

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

On Sun, Dec 31, 2017 at 12:10:51AM +0100, Arnd Bergmann wrote:
> On Fri, Dec 29, 2017 at 2:53 AM, Haiyue Wang
> <haiyue.wang-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> > When PCH works under eSPI mode, the PMC (Power Management Controller) in
> > PCH is waiting for SUS_ACK from BMC after it alerts SUS_WARN. It is in
> > dead loop if no SUS_ACK assert. This is the basic requirement for the BMC
> > works as eSPI slave.
> >
> > Also for the host power on / off actions, from BMC side, the following VW
> > (Virtual Wire) messages are done in firmware:
> > 1. SLAVE_BOOT_LOAD_DONE / SLAVE_BOOT_LOAD_STATUS
> > 2. SUS_ACK
> > 3. OOB_RESET_ACK
> > 4. HOST_RESET_ACK
> 
> I have not looked at the driver contents yet, but I'm adding the SPI
> maintainer and
> mailing list to Cc here for further discussion. Can you clarify how

More generally:

As documented in SubmittingPatches please send patches to the 
maintainers for the code you would like to change.  The normal kernel
workflow is that people apply patches from their inboxes, if they aren't
copied they are likely to not see the patch at all and it is much more
difficult to apply patches.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Wang, Haiyue]

On 2018-01-03 19:38, Mark Brown wrote:
> On Sun, Dec 31, 2017 at 12:10:51AM +0100, Arnd Bergmann wrote:
>> On Fri, Dec 29, 2017 at 2:53 AM, Haiyue Wang
>> <haiyue.wang@linux.intel.com> wrote:
>>> When PCH works under eSPI mode, the PMC (Power Management Controller) in
>>> PCH is waiting for SUS_ACK from BMC after it alerts SUS_WARN. It is in
>>> dead loop if no SUS_ACK assert. This is the basic requirement for the BMC
>>> works as eSPI slave.
>>>
>>> Also for the host power on / off actions, from BMC side, the following VW
>>> (Virtual Wire) messages are done in firmware:
>>> 1. SLAVE_BOOT_LOAD_DONE / SLAVE_BOOT_LOAD_STATUS
>>> 2. SUS_ACK
>>> 3. OOB_RESET_ACK
>>> 4. HOST_RESET_ACK
>> I have not looked at the driver contents yet, but I'm adding the SPI
>> maintainer and
>> mailing list to Cc here for further discussion. Can you clarify how
> More generally:
>
> As documented in SubmittingPatches please send patches to the
> maintainers for the code you would like to change.  The normal kernel
> workflow is that people apply patches from their inboxes, if they aren't
> copied they are likely to not see the patch at all and it is much more
> difficult to apply patches.
Should send to like this ? Because I add patch for Aspeed chip:

./scripts/get_maintainer.pl drivers/misc/aspeed-lpc-snoop.c
Joel Stanley <joel@jms.id.au> (maintainer:ARM/ASPEED MACHINE SUPPORT)
Arnd Bergmann <arnd@arndb.de> (supporter:CHAR and MISC DRIVERS)
Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:CHAR and MISC 
DRIVERS)
linux-kernel@vger.kernel.org (open list)

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 673 bytes --]

On Wed, Jan 03, 2018 at 10:28:22PM +0800, Wang, Haiyue wrote:

> Should send to like this ? Because I add patch for Aspeed chip:

> ./scripts/get_maintainer.pl drivers/misc/aspeed-lpc-snoop.c
> Joel Stanley <joel@jms.id.au> (maintainer:ARM/ASPEED MACHINE SUPPORT)
> Arnd Bergmann <arnd@arndb.de> (supporter:CHAR and MISC DRIVERS)
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:CHAR and MISC
> DRIVERS)
> linux-kernel@vger.kernel.org (open list)

So it's not actually doing anything at all with the SPI subsystem?  I
lacked any context for the discussion having been copied in part way
through.  If it is a SPI controller it ought to have been in
drivers/spi.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Wang, Haiyue]

On 2018-01-03 22:32, Mark Brown wrote:
> On Wed, Jan 03, 2018 at 10:28:22PM +0800, Wang, Haiyue wrote:
>
>> Should send to like this ? Because I add patch for Aspeed chip:
>> ./scripts/get_maintainer.pl drivers/misc/aspeed-lpc-snoop.c
>> Joel Stanley <joel@jms.id.au> (maintainer:ARM/ASPEED MACHINE SUPPORT)
>> Arnd Bergmann <arnd@arndb.de> (supporter:CHAR and MISC DRIVERS)
>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> (supporter:CHAR and MISC
>> DRIVERS)
>> linux-kernel@vger.kernel.org (open list)
> So it's not actually doing anything at all with the SPI subsystem?  I
> lacked any context for the discussion having been copied in part way
> through.  If it is a SPI controller it ought to have been in
> drivers/spi.
Yes, eSPI just uses the SPI pin definition, but it replaces LPC interface.

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

[Arnd Bergmann]

On Wed, Jan 3, 2018 at 3:32 PM, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Jan 03, 2018 at 10:28:22PM +0800, Wang, Haiyue wrote:
>
>> Should send to like this ? Because I add patch for Aspeed chip:
>
>> ./scripts/get_maintainer.pl drivers/misc/aspeed-lpc-snoop.c
>> Joel Stanley <joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org> (maintainer:ARM/ASPEED MACHINE SUPPORT)
>> Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> (supporter:CHAR and MISC DRIVERS)
>> Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org> (supporter:CHAR and MISC
>> DRIVERS)
>> linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org (open list)
>
> So it's not actually doing anything at all with the SPI subsystem?  I
> lacked any context for the discussion having been copied in part way
> through.  If it is a SPI controller it ought to have been in
> drivers/spi.

It's not an SPI host controller, but a specialized driver for a specialuzed
SPI slave hardware block.

The SPI slave driver implements the higher-level parts of the eSPI protocol
stack in Linux, and the lower levels in hardware. The question is whether (and
how) there should be a split between the levels. If we are expecting to add
a software implementation of the same eSPI stack in software using the generic
SPI slave code in the future, it may be helpful to have that separate in place
already.

With my later suggestion of splitting out the eSPI "virtual wire" low-level
support into a gpiochip driver, neither half would be in drivers/spi/, but
one could implement a drivers/spi/spi-slave-espi-vw.c slave protocol
driver that exposes the same in-kernel interface on top of a slave-capable
SPI controller.

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

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

[Arnd Bergmann]

On Wed, Jan 3, 2018 at 3:21 AM, Wang, Haiyue
<haiyue.wang-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> On 2018-01-03 00:23, Arnd Bergmann wrote:
>> On Tue, Jan 2, 2018 at 4:36 PM, Wang, Haiyue <haiyue.wang-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> 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.

>>> 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?

>> 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.

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

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

[Wang, Haiyue]

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-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>  wrote:
>>>> On 2018-01-03 00:23, Arnd Bergmann wrote:
>>>>> On Tue, Jan 2, 2018 at 4:36 PM, Wang, Haiyue<haiyue.wang-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>  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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html