-----Original Message-----
From: Joel Stanley <joel@jms.id.au>
Sent: Friday, January 8, 2021 10:59 AM
To: ChiaWei Wang <redacted>; Jeremy Kerr
[off-list ref]
Cc: Rob Herring <robh@kernel.org>; andrew@aj.id.au; tglx@linutronix.de;
maz@kernel.org; p.zabel@pengutronix.de; linux-aspeed@lists.ozlabs.org;
openbmc@lists.ozlabs.org; devicetree@vger.kernel.org;
linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; BMC-SW
[off-list ref]
Subject: Re: [PATCH 5/6] soc: aspeed: Add eSPI driver

On Thu, 7 Jan 2021 at 02:39, ChiaWei Wang
[off-list ref] wrote:

quoted

Hi Rob,

quoted

-----Original Message-----
From: Rob Herring <robh@kernel.org>
Sent: Wednesday, January 6, 2021 11:32 PM
To: ChiaWei Wang <redacted>
Subject: Re: [PATCH 5/6] soc: aspeed: Add eSPI driver

On Wed, Jan 06, 2021 at 01:59:38PM +0800, Chia-Wei, Wang wrote:

quoted

The Aspeed eSPI controller is slave device to communicate with the
master through the Enhanced Serial Peripheral Interface (eSPI).
All of the four eSPI channels, namely peripheral, virtual wire,
out-of-band, and flash are supported.

Signed-off-by: Chia-Wei, Wang <redacted>
---
 drivers/soc/aspeed/Kconfig                  |  49 ++
 drivers/soc/aspeed/Makefile                 |   5 +
 drivers/soc/aspeed/aspeed-espi-ctrl.c       | 197 ++++++
 drivers/soc/aspeed/aspeed-espi-flash.c      | 490 ++++++++++++++
 drivers/soc/aspeed/aspeed-espi-oob.c        | 706

++++++++++++++++++++

quoted

 drivers/soc/aspeed/aspeed-espi-peripheral.c | 613

+++++++++++++++++

quoted

quoted

quoted

 drivers/soc/aspeed/aspeed-espi-vw.c         | 211 ++++++
 include/uapi/linux/aspeed-espi.h            | 160 +++++
 8 files changed, 2431 insertions(+)  create mode 100644
drivers/soc/aspeed/aspeed-espi-ctrl.c
 create mode 100644 drivers/soc/aspeed/aspeed-espi-flash.c
 create mode 100644 drivers/soc/aspeed/aspeed-espi-oob.c
 create mode 100644 drivers/soc/aspeed/aspeed-espi-peripheral.c
 create mode 100644 drivers/soc/aspeed/aspeed-espi-vw.c

drivers/spi/ is the correct location for a SPI controller.

quoted

 create mode 100644 include/uapi/linux/aspeed-espi.h

This userspace interface is not going to be accepted upstream.

I'd suggest you look at similar SPI flash capable SPI controller
drivers upstream and model your driver after them. This looks like it needs

major reworking.

quoted

quoted

eSPI resues the timing and electrical specification of SPI but runs completely

different protocol.

quoted

Only the flash channel is related to SPI and the other 3 channels are for

EC/BMC/SIO.

quoted

Therefore, an eSPI driver might not fit into the SPI model.

I agree, the naming is confusing but eSPI doesn't belong in drivers/spi.

As it is a bus that is common to more than just the Aspeed BMC, we may want
to implement it as a new bus type that has devices hanging off it, similar to
FSI.

The ASPEED eSPI controller driver is slave side device, not master side. I think it will be stay soc/aspeed first. 
Because is most SoC Chip related. 

Cheers,

Ryan