RE: [PATCH 5/6] soc: aspeed: Add eSPI driver
> -Original Message- > From: Joel Stanley > Sent: Friday, January 8, 2021 10:59 AM > To: ChiaWei Wang ; Jeremy Kerr > > Cc: Rob Herring ; and...@aj.id.au; t...@linutronix.de; > m...@kernel.org; p.za...@pengutronix.de; linux-asp...@lists.ozlabs.org; > open...@lists.ozlabs.org; devicet...@vger.kernel.org; > linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org; BMC-SW > > Subject: Re: [PATCH 5/6] soc: aspeed: Add eSPI driver > > On Thu, 7 Jan 2021 at 02:39, ChiaWei Wang > wrote: > > > > Hi Rob, > > > > > -Original Message- > > > From: Rob Herring > > > Sent: Wednesday, January 6, 2021 11:32 PM > > > To: ChiaWei Wang > > > Subject: Re: [PATCH 5/6] soc: aspeed: Add eSPI driver > > > > > > On Wed, Jan 06, 2021 at 01:59:38PM +0800, Chia-Wei, Wang wrote: > > > > 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 > > > > --- > > > > 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 > > > > > > > drivers/soc/aspeed/aspeed-espi-peripheral.c | 613 > + > > > > 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. > > > > > > > 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. > > > > > eSPI resues the timing and electrical specification of SPI but runs > > completely > different protocol. > > Only the flash channel is related to SPI and the other 3 channels are for > EC/BMC/SIO. > > 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
Re: [PATCH 5/6] soc: aspeed: Add eSPI driver
On Thu, 7 Jan 2021 at 02:39, ChiaWei Wang wrote: > > Hi Rob, > > > -Original Message- > > From: Rob Herring > > Sent: Wednesday, January 6, 2021 11:32 PM > > To: ChiaWei Wang > > Subject: Re: [PATCH 5/6] soc: aspeed: Add eSPI driver > > > > On Wed, Jan 06, 2021 at 01:59:38PM +0800, Chia-Wei, Wang wrote: > > > 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 > > > --- > > > 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 > > > > > drivers/soc/aspeed/aspeed-espi-peripheral.c | 613 + > > > 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. > > > > > 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. > > > eSPI resues the timing and electrical specification of SPI but runs > completely different protocol. > Only the flash channel is related to SPI and the other 3 channels are for > EC/BMC/SIO. > 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. Cheers, Joel
RE: [PATCH 5/6] soc: aspeed: Add eSPI driver
Hi Rob, > -Original Message- > From: Rob Herring > Sent: Wednesday, January 6, 2021 11:32 PM > To: ChiaWei Wang > Subject: Re: [PATCH 5/6] soc: aspeed: Add eSPI driver > > On Wed, Jan 06, 2021 at 01:59:38PM +0800, Chia-Wei, Wang wrote: > > 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 > > --- > > 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 > > > drivers/soc/aspeed/aspeed-espi-peripheral.c | 613 + > > 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. > > > 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. > eSPI resues the timing and electrical specification of SPI but runs completely different protocol. Only the flash channel is related to SPI and the other 3 channels are for EC/BMC/SIO. Therefore, an eSPI driver might not fit into the SPI model. Chiawei
Re: [PATCH 5/6] soc: aspeed: Add eSPI driver
On Wed, Jan 06, 2021 at 01:59:38PM +0800, Chia-Wei, Wang wrote: > 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 > --- > 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 > drivers/soc/aspeed/aspeed-espi-peripheral.c | 613 + > 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. > 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. Rob
