| Message ID | 20210826061623.6352-1-chiawei_wang@aspeedtech.com |
|---|---|
| Headers | show |
| Series | arm: aspeed: Add eSPI support | expand |
Hi Chia-Wei, [apologies for the re-send, dropping HTML part...] > 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. Great to have this added submitted upstream! A few comments though: > --- > drivers/soc/aspeed/Kconfig | 11 + > drivers/soc/aspeed/Makefile | 1 + > drivers/soc/aspeed/aspeed-espi-ctrl.c | 205 +++++++++ > drivers/soc/aspeed/aspeed-espi-ctrl.h | 304 ++++++++++++ > drivers/soc/aspeed/aspeed-espi-flash.h | 380 +++++++++++++++ > drivers/soc/aspeed/aspeed-espi-ioc.h | 153 +++++++ > drivers/soc/aspeed/aspeed-espi-oob.h | 611 +++++++++++++++++++++++++ > drivers/soc/aspeed/aspeed-espi-perif.h | 539 ++++++++++++++++++++++ > drivers/soc/aspeed/aspeed-espi-vw.h | 142 ++++++ This structure is a bit odd - you have the one -crtl.c file, which defines the actual driver, but then a bunch of headers that contain more code than header-type definitions. Is there any reason that -flash, -ioc, -oob, -perif and -vw components can't be standard .c files? Then, for the userspace ABI: it looks like you're exposing everything through new device-specific ioctls. Would it not make more sense to use existing interfaces? For example, the virtual wire bits could be regular GPIOs; the flash interface could be a mtd or block device. I understand that we'll likely still need some level of custom device control, but the more we can use generic interfaces for, the less custom code (and interfaces) we'll need on the userspace side. Cheers, Jeremy
Hi Jeremy, Thanks for reviewing the patch. > From: Jeremy Kerr <jk@codeconstruct.com.au> > Sent: Friday, August 27, 2021 11:20 AM > > Hi Chia-Wei, > > [apologies for the re-send, dropping HTML part...] > > > 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. > > Great to have this added submitted upstream! A few comments though: > > > --- > > drivers/soc/aspeed/Kconfig | 11 + > > drivers/soc/aspeed/Makefile | 1 + > > drivers/soc/aspeed/aspeed-espi-ctrl.c | 205 +++++++++ > > drivers/soc/aspeed/aspeed-espi-ctrl.h | 304 ++++++++++++ > > drivers/soc/aspeed/aspeed-espi-flash.h | 380 +++++++++++++++ > > drivers/soc/aspeed/aspeed-espi-ioc.h | 153 +++++++ > > drivers/soc/aspeed/aspeed-espi-oob.h | 611 > > +++++++++++++++++++++++++ > > drivers/soc/aspeed/aspeed-espi-perif.h | 539 ++++++++++++++++++++++ > > drivers/soc/aspeed/aspeed-espi-vw.h | 142 ++++++ > > This structure is a bit odd - you have the one -crtl.c file, which defines the > actual driver, but then a bunch of headers that contain more code than > header-type definitions. > > Is there any reason that -flash, -ioc, -oob, -perif and -vw components can't be > standard .c files? The eSPI slave device comprises four channels, where each of them has individual functionality. Putting the four channels driver code into a single file makes it hard to maintain and trace. We did consider to make them standard .c files. But it requires to export channel functions into kernel space although they are dedicated only to this eSPI driver. As espi-ctrl needs to invoke corresponding channel functions when it is interrupted by eSPI events. To avoid polluting kernel space, we decided to put driver code in header files and make the channel functions 'static'. BTW, I once encountered .c file inclusion in other projects. Is it proper for Linux driver development? > > Then, for the userspace ABI: it looks like you're exposing everything through > new device-specific ioctls. Would it not make more sense to use existing > interfaces? For example, the virtual wire bits could be regular GPIOs; the flash > interface could be a mtd or block device. > > I understand that we'll likely still need some level of custom device control, but > the more we can use generic interfaces for, the less custom code (and > interfaces) we'll need on the userspace side. > eSPI communication is based on the its cycle packet format. We intended to let userspace decided how to interpret and compose TX/RX packets including header, tag, length (encoded), and data. IOCTL comes to our first mind as it also works in the 'packet' like paradigm. Chiawei
Hi Chiawei, > The eSPI slave device comprises four channels, where each of them has > individual functionality. Putting the four channels driver code into a > single file makes it hard to maintain and trace. Yep, understood. > We did consider to make them standard .c files. > But it requires to export channel functions into kernel space > although they are dedicated only to this eSPI driver. What do you mean by "export into kernel space" here? The function prototypes need to be available to your main (-ctrl.c) file, regardless of whether you're putting the entire functions in a header file, or just the prototype. There's doesn't need to be any difference in visibility outside of your own module if you were to do this the usual way. > As espi-ctrl needs to invoke corresponding channel functions when it > is interrupted by eSPI events. > > To avoid polluting kernel space, we decided to put driver code in > header files and make the channel functions 'static'. > > BTW, I once encountered .c file inclusion in other projects. Is it > proper for Linux driver development? It can be, just that in this case it's a bit unusual, and I can't see a good reason for doing so. This could just be a standard multiple-source-file module. > eSPI communication is based on the its cycle packet format. > We intended to let userspace decided how to interpret and compose > TX/RX packets including header, tag, length (encoded), and data. > IOCTL comes to our first mind as it also works in the 'packet' like > paradigm. But you're not always exposing a packet-like interface for this. For example, your virtual-wire interface just has a get/set interface for bits in a register (plus some PCH event handling, which may not be applicable to all platforms...). The other channels do look like more of a packet interface though, but in that case I'm not convinced that an ioctl interface is the best way to go for that. You're essentially sending a (length, pointer) pair over the ioctls there, which sounds more like a write() than an ioctl(). Regardless of the choice of interface though, this will definitely need some documentation or description of the API, and the ioc header to be somewhere useful for userspace to consume. With that documented, we'd have a better idea of how the new ABI is supposed to work. Cheers, Jeremy
Hi Jeremy > From: Jeremy Kerr <jk@codeconstruct.com.au> > Sent: Friday, August 27, 2021 12:37 PM > > Hi Chiawei, > > > The eSPI slave device comprises four channels, where each of them has > > individual functionality. Putting the four channels driver code into > > a single file makes it hard to maintain and trace. > > Yep, understood. > > > We did consider to make them standard .c files. > > But it requires to export channel functions into kernel space although > > they are dedicated only to this eSPI driver. > > What do you mean by "export into kernel space" here? The function prototypes The channel functions will be visible to all kernel driver files. > need to be available to your main (-ctrl.c) file, regardless of whether you're > putting the entire functions in a header file, or just the prototype. There's > doesn't need to be any difference in visibility outside of your own module if > you were to do this the usual way. Maybe I was trying to make channels function visible only to espi-ctrl.c too far. I will revise the driver to present in the usual .c way. > > > As espi-ctrl needs to invoke corresponding channel functions when it > > is interrupted by eSPI events. > > > > To avoid polluting kernel space, we decided to put driver code in > > header files and make the channel functions 'static'. > > > > BTW, I once encountered .c file inclusion in other projects. Is it > > proper for Linux driver development? > > It can be, just that in this case it's a bit unusual, and I can't see a good reason > for doing so. This could just be a standard multiple-source-file module. > > > eSPI communication is based on the its cycle packet format. > > We intended to let userspace decided how to interpret and compose > > TX/RX packets including header, tag, length (encoded), and data. > > IOCTL comes to our first mind as it also works in the 'packet' like > > paradigm. > > But you're not always exposing a packet-like interface for this. For example, > your virtual-wire interface just has a get/set interface for bits in a register > (plus some PCH event handling, which may not be applicable to all > platforms...). > > The other channels do look like more of a packet interface though, but in that > case I'm not convinced that an ioctl interface is the best way to go for that. > You're essentially sending a (length, pointer) pair over the ioctls there, which > sounds more like a write() than an ioctl(). In most cases, yes. Currently only the peripheral channel has more than the 2 (put tx/get rx) IOCTL code. We think it might be a good idea to make the user interfaces of all channels consistent using IOCTL. > > Regardless of the choice of interface though, this will definitely need some > documentation or description of the API, and the ioc header to be somewhere > useful for userspace to consume. > > With that documented, we'd have a better idea of how the new ABI is > supposed to work. Sure. more comments will be added in aspeed-espi-ioc.h to describe the usage and the purpose. Thanks for your feedback. Chiawei