Message ID | 20181026144333.12276-1-boris.brezillon@bootlin.com |
---|---|
Headers | show |
Series | Add the I3C subsystem | expand |
Hi Boris, On 26/10/18 15:43, Boris Brezillon wrote: > Hi Greg, > > I think we've reached a point where we can eventually consider the I3C > framework for inclusion in 4.20 (5.0?). A few more issues were reported > on v9 and fixed in v10. I can't guarantee that the implementation is > free of bugs but I still think it's worth merging it in v4.20: it's a > new subsystem, so we don't risk regressions, and the only way we can > detect other issues is by having other people experiment with this > implementation. > > The only remaining concern raised by Arnd is the fact that both hosts > and slaves share the same bus type and are differentiated thanks to > their device_type, which IMHO is fine since this is what other > subsystems do (plus I don't see other solutions to have both I3C > devices and I3C buses represented under /sys/bus/i3c/). > > I'd really like to get this series merged in 4.20 so that other > contributors can work on top of it to add > > 1/ new host drivers > 2/ support for advanced features > 3/ new device drivers > > So, unless you have strong reasons to reject this request, and, > assuming I get Rob's ack soon enough, I plan to send a PR for this > framework next week. > > Here is the usual description copy&pasted from the previous versions: > > This patch series is adding a new subsystem to support I3C devices. > > This is just adding support for basic features. Extra features will > be added afterwards. > > There are a few design choices that are worth mentioning because they > impact the way I3C device drivers can interact with their devices: > > - all functions used to send I3C/I2C frames must be called in > non-atomic context. Mainly done this way to ease implementation, but > this is still open to discussion. Please let me know if you think it's > worth considering an asynchronous model here > - the I3C bus and I3C master controller are now tightly coupled even > though they're still allocated separately. There's now a 1:1 > relationship between these objects, and the I3C master is no longer > represented under the I3C bus object. > Arnd, let me know if you had something different in mind, and I'll > rework the implementation accordingly. > > - I2C backward compatibility has been designed to be transparent to I2C > drivers and the I2C subsystem. The I3C master just registers an I2C > adapter which creates a new I2C bus. I'd say that, from a > representation PoV it's not ideal because what should appear as a > single I3C bus exposing I3C and I2C devices here appears as 2 > different busses connected to each other through the parenting (the > I3C master is the parent of the I2C and I3C busses). > On the other hand, I don't see a better solution if we want something > that is not invasive. > > Missing features (will be added in separate patch series after initial > support has been accepted/merged): > - support for HDR modes (has been removed because of lack of real users) > - no support for multi-master and the associated concepts (mastership > handover, support for secondary masters, ...) > - I2C devices can only be described using DT because this is the only > use case I have. However, the framework can easily be extended with > ACPI and board info support > - I3C slave framework. This has been completely omitted, but shouldn't > have a huge impact on the I3C framework because I3C slaves don't see > the whole bus, it's only about handling master requests and generating > IBIs. Some of the struct, constant and enum definitions could be > shared, but most of the I3C slave framework logic will be different > > Note that this patchset is available on the linux-i3c repo[1]. > > Main change between v8 and v9: > - The DT bindings have been simplified to get rid of the I3C/I3C_DEV() > macros > > Main change between v7 and v8: > - The bus object is now embedded in the master_controller object (as > suggested by Arnd) > > Main changes between v6 and v7: > - I3C bus/master representations have been reworked to match what other > subsystems are doing (master implicitly represented by the bus > object) > - I3C dev registration has been fixed > - I3C bus mode selection has been fixed > - Calls to readsl/writesl() in the Cadence I3C master driver have been > fixed > > Main changes between v5 and v6: > - Introduce {i3c,i2c}_dev_desc structures to better match how I3C > master controllers (reservation of one HW slot for each device > attached to the bus). With this solution, the resource migration > that happens when a device lose its dynamic address and is > re-assigned a different address is simplified on the driver side, > because most of it is now handled in the core (reserve a new dev > slot, reserve IBI resources and free all resources attached to the > old slot) > - Add I3C error codes (M0 to M2) so that the core and device drivers > can have fine grained information on what caused an EIO error. > > Only minor things happened between v3 and v5 (you can go check the > changelog in each patch for more details). > > Main changes between v2 and v3 are: > - Reworked the DT bindings as suggested by Rob > - Reworked the bus initialization step as suggested by Vitor > - Added a driver for an I3C GPIO expander > > Main changes between the initial RFC and this v2 are: > - Add a generic infrastructure to support IBIs. It's worth mentioning > that I tried exposing IBIs as a regular IRQs, but after several > attempts and a discussion with Mark Zyngier, it appeared that it was > not really fitting in the Linux IRQ model (the fact that you have > payload attached to IBIs, the fact that most of the time an IBI will > generate a transfer on the bus which has to be done in an atomic > context, ...) > The counterpart of this decision is the latency induced by the > workqueue approach, but since I don't have real use cases, I don't > know if this can be a problem or not. > - Add helpers to support Hot Join > - Add support for IBIs and Hot Join in Cadence I3C master driver > - Address several issues in how I was using the device model > > Thanks, > > Boris > > [1]https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_i3c_linux.git_log_-3Fh-3Di3c_next&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=TkhdwjEerP4IBiC_WzI_vEUyZD2Dcxl03UeycbwNI2Q&s=KK31_5lEEgNN2iyURFULH0HmjkOPWoarNiT1hehvIsY&e= > > *** BLURB HERE *** > > Boris Brezillon (9): > i3c: Add core I3C infrastructure > docs: driver-api: Add I3C documentation > i3c: Add sysfs ABI spec > dt-bindings: i3c: Document core bindings > MAINTAINERS: Add myself as the I3C subsystem maintainer > i3c: master: Add driver for Cadence IP > dt-bindings: i3c: Document Cadence I3C master bindings > gpio: Add a driver for Cadence I3C GPIO expander > dt-bindings: gpio: Add bindings for Cadence I3C gpio expander > > Documentation/ABI/testing/sysfs-bus-i3c | 146 + > .../bindings/gpio/gpio-cdns-i3c.txt | 39 + > .../bindings/i3c/cdns,i3c-master.txt | 44 + > Documentation/devicetree/bindings/i3c/i3c.txt | 139 + > .../driver-api/i3c/device-driver-api.rst | 9 + > Documentation/driver-api/i3c/index.rst | 11 + > .../driver-api/i3c/master-driver-api.rst | 10 + > Documentation/driver-api/i3c/protocol.rst | 203 ++ > Documentation/driver-api/index.rst | 1 + > MAINTAINERS | 12 + > drivers/Kconfig | 2 + > drivers/Makefile | 2 +- > drivers/gpio/Kconfig | 11 + > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-cdns-i3c.c | 411 +++ > drivers/i3c/Kconfig | 24 + > drivers/i3c/Makefile | 4 + > drivers/i3c/device.c | 233 ++ > drivers/i3c/internals.h | 26 + > drivers/i3c/master.c | 2661 +++++++++++++++++ > drivers/i3c/master/Kconfig | 6 + > drivers/i3c/master/Makefile | 1 + > drivers/i3c/master/i3c-master-cdns.c | 1671 +++++++++++ > include/linux/i3c/ccc.h | 385 +++ > include/linux/i3c/device.h | 331 ++ > include/linux/i3c/master.h | 648 ++++ > include/linux/mod_devicetable.h | 17 + > 27 files changed, 7047 insertions(+), 1 deletion(-) > create mode 100644 Documentation/ABI/testing/sysfs-bus-i3c > create mode 100644 Documentation/devicetree/bindings/gpio/gpio-cdns-i3c.txt > create mode 100644 Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt > create mode 100644 Documentation/devicetree/bindings/i3c/i3c.txt > create mode 100644 Documentation/driver-api/i3c/device-driver-api.rst > create mode 100644 Documentation/driver-api/i3c/index.rst > create mode 100644 Documentation/driver-api/i3c/master-driver-api.rst > create mode 100644 Documentation/driver-api/i3c/protocol.rst > create mode 100644 drivers/gpio/gpio-cdns-i3c.c > create mode 100644 drivers/i3c/Kconfig > create mode 100644 drivers/i3c/Makefile > create mode 100644 drivers/i3c/device.c > create mode 100644 drivers/i3c/internals.h > create mode 100644 drivers/i3c/master.c > create mode 100644 drivers/i3c/master/Kconfig > create mode 100644 drivers/i3c/master/Makefile > create mode 100644 drivers/i3c/master/i3c-master-cdns.c > create mode 100644 include/linux/i3c/ccc.h > create mode 100644 include/linux/i3c/device.h > create mode 100644 include/linux/i3c/master.h > Can you update the i3c/next tree? Best regards, Vitor Soares
On Fri, 26 Oct 2018 16:22:06 +0100
vitor <vitor.soares@synopsys.com> wrote:
> Can you update the i3c/next tree?
Done.
On Fri, 26 Oct 2018 16:43:24 +0200 Boris Brezillon <boris.brezillon@bootlin.com> wrote: > Hi Greg, > > I think we've reached a point where we can eventually consider the I3C > framework for inclusion in 4.20 (5.0?). A few more issues were reported > on v9 and fixed in v10. I can't guarantee that the implementation is > free of bugs but I still think it's worth merging it in v4.20: it's a > new subsystem, so we don't risk regressions, and the only way we can > detect other issues is by having other people experiment with this > implementation. > > The only remaining concern raised by Arnd is the fact that both hosts > and slaves share the same bus type and are differentiated thanks to > their device_type, which IMHO is fine since this is what other > subsystems do (plus I don't see other solutions to have both I3C > devices and I3C buses represented under /sys/bus/i3c/). > > I'd really like to get this series merged in 4.20 so that other > contributors can work on top of it to add > > 1/ new host drivers > 2/ support for advanced features > 3/ new device drivers > > So, unless you have strong reasons to reject this request, and, > assuming I get Rob's ack soon enough, I plan to send a PR for this > framework next week. > > Here is the usual description copy&pasted from the previous versions: > > This patch series is adding a new subsystem to support I3C devices. > > This is just adding support for basic features. Extra features will > be added afterwards. > > There are a few design choices that are worth mentioning because they > impact the way I3C device drivers can interact with their devices: > > - all functions used to send I3C/I2C frames must be called in > non-atomic context. Mainly done this way to ease implementation, but > this is still open to discussion. Please let me know if you think it's > worth considering an asynchronous model here > - the I3C bus and I3C master controller are now tightly coupled even > though they're still allocated separately. There's now a 1:1 > relationship between these objects, and the I3C master is no longer > represented under the I3C bus object. > Arnd, let me know if you had something different in mind, and I'll > rework the implementation accordingly. > > - I2C backward compatibility has been designed to be transparent to I2C > drivers and the I2C subsystem. The I3C master just registers an I2C > adapter which creates a new I2C bus. I'd say that, from a > representation PoV it's not ideal because what should appear as a > single I3C bus exposing I3C and I2C devices here appears as 2 > different busses connected to each other through the parenting (the > I3C master is the parent of the I2C and I3C busses). > On the other hand, I don't see a better solution if we want something > that is not invasive. > > Missing features (will be added in separate patch series after initial > support has been accepted/merged): > - support for HDR modes (has been removed because of lack of real users) > - no support for multi-master and the associated concepts (mastership > handover, support for secondary masters, ...) > - I2C devices can only be described using DT because this is the only > use case I have. However, the framework can easily be extended with > ACPI and board info support > - I3C slave framework. This has been completely omitted, but shouldn't > have a huge impact on the I3C framework because I3C slaves don't see > the whole bus, it's only about handling master requests and generating > IBIs. Some of the struct, constant and enum definitions could be > shared, but most of the I3C slave framework logic will be different > > Note that this patchset is available on the linux-i3c repo[1]. > Forgot to add: Main changes between v9 and v10: - Fix the example in the DT bindings file - Dynamically allocate CCC payloads to make them DMA-safe - Fix a deadlock - Mention that i2c bufs are not guaranteed to be DMA-safe > Main change between v8 and v9: > - The DT bindings have been simplified to get rid of the I3C/I3C_DEV() > macros > > Main change between v7 and v8: > - The bus object is now embedded in the master_controller object (as > suggested by Arnd) > > Main changes between v6 and v7: > - I3C bus/master representations have been reworked to match what other > subsystems are doing (master implicitly represented by the bus > object) > - I3C dev registration has been fixed > - I3C bus mode selection has been fixed > - Calls to readsl/writesl() in the Cadence I3C master driver have been > fixed > > Main changes between v5 and v6: > - Introduce {i3c,i2c}_dev_desc structures to better match how I3C > master controllers (reservation of one HW slot for each device > attached to the bus). With this solution, the resource migration > that happens when a device lose its dynamic address and is > re-assigned a different address is simplified on the driver side, > because most of it is now handled in the core (reserve a new dev > slot, reserve IBI resources and free all resources attached to the > old slot) > - Add I3C error codes (M0 to M2) so that the core and device drivers > can have fine grained information on what caused an EIO error. > > Only minor things happened between v3 and v5 (you can go check the > changelog in each patch for more details). > > Main changes between v2 and v3 are: > - Reworked the DT bindings as suggested by Rob > - Reworked the bus initialization step as suggested by Vitor > - Added a driver for an I3C GPIO expander > > Main changes between the initial RFC and this v2 are: > - Add a generic infrastructure to support IBIs. It's worth mentioning > that I tried exposing IBIs as a regular IRQs, but after several > attempts and a discussion with Mark Zyngier, it appeared that it was > not really fitting in the Linux IRQ model (the fact that you have > payload attached to IBIs, the fact that most of the time an IBI will > generate a transfer on the bus which has to be done in an atomic > context, ...) > The counterpart of this decision is the latency induced by the > workqueue approach, but since I don't have real use cases, I don't > know if this can be a problem or not. > - Add helpers to support Hot Join > - Add support for IBIs and Hot Join in Cadence I3C master driver > - Address several issues in how I was using the device model > > Thanks, > > Boris > > [1]https://git.kernel.org/pub/scm/linux/kernel/git/i3c/linux.git/log/?h=i3c/next > > *** BLURB HERE *** > > Boris Brezillon (9): > i3c: Add core I3C infrastructure > docs: driver-api: Add I3C documentation > i3c: Add sysfs ABI spec > dt-bindings: i3c: Document core bindings > MAINTAINERS: Add myself as the I3C subsystem maintainer > i3c: master: Add driver for Cadence IP > dt-bindings: i3c: Document Cadence I3C master bindings > gpio: Add a driver for Cadence I3C GPIO expander > dt-bindings: gpio: Add bindings for Cadence I3C gpio expander > > Documentation/ABI/testing/sysfs-bus-i3c | 146 + > .../bindings/gpio/gpio-cdns-i3c.txt | 39 + > .../bindings/i3c/cdns,i3c-master.txt | 44 + > Documentation/devicetree/bindings/i3c/i3c.txt | 139 + > .../driver-api/i3c/device-driver-api.rst | 9 + > Documentation/driver-api/i3c/index.rst | 11 + > .../driver-api/i3c/master-driver-api.rst | 10 + > Documentation/driver-api/i3c/protocol.rst | 203 ++ > Documentation/driver-api/index.rst | 1 + > MAINTAINERS | 12 + > drivers/Kconfig | 2 + > drivers/Makefile | 2 +- > drivers/gpio/Kconfig | 11 + > drivers/gpio/Makefile | 1 + > drivers/gpio/gpio-cdns-i3c.c | 411 +++ > drivers/i3c/Kconfig | 24 + > drivers/i3c/Makefile | 4 + > drivers/i3c/device.c | 233 ++ > drivers/i3c/internals.h | 26 + > drivers/i3c/master.c | 2661 +++++++++++++++++ > drivers/i3c/master/Kconfig | 6 + > drivers/i3c/master/Makefile | 1 + > drivers/i3c/master/i3c-master-cdns.c | 1671 +++++++++++ > include/linux/i3c/ccc.h | 385 +++ > include/linux/i3c/device.h | 331 ++ > include/linux/i3c/master.h | 648 ++++ > include/linux/mod_devicetable.h | 17 + > 27 files changed, 7047 insertions(+), 1 deletion(-) > create mode 100644 Documentation/ABI/testing/sysfs-bus-i3c > create mode 100644 Documentation/devicetree/bindings/gpio/gpio-cdns-i3c.txt > create mode 100644 Documentation/devicetree/bindings/i3c/cdns,i3c-master.txt > create mode 100644 Documentation/devicetree/bindings/i3c/i3c.txt > create mode 100644 Documentation/driver-api/i3c/device-driver-api.rst > create mode 100644 Documentation/driver-api/i3c/index.rst > create mode 100644 Documentation/driver-api/i3c/master-driver-api.rst > create mode 100644 Documentation/driver-api/i3c/protocol.rst > create mode 100644 drivers/gpio/gpio-cdns-i3c.c > create mode 100644 drivers/i3c/Kconfig > create mode 100644 drivers/i3c/Makefile > create mode 100644 drivers/i3c/device.c > create mode 100644 drivers/i3c/internals.h > create mode 100644 drivers/i3c/master.c > create mode 100644 drivers/i3c/master/Kconfig > create mode 100644 drivers/i3c/master/Makefile > create mode 100644 drivers/i3c/master/i3c-master-cdns.c > create mode 100644 include/linux/i3c/ccc.h > create mode 100644 include/linux/i3c/device.h > create mode 100644 include/linux/i3c/master.h >
On 26/10/18 17:15, Boris Brezillon wrote: > On Fri, 26 Oct 2018 16:22:06 +0100 > vitor <vitor.soares@synopsys.com> wrote: > >> Can you update the i3c/next tree? > Done. Thanks. I will apply the driver and them I give you feedback.
On Fri, 26 Oct 2018 17:20:42 +0100 vitor <vitor.soares@synopsys.com> wrote: > On 26/10/18 17:15, Boris Brezillon wrote: > > On Fri, 26 Oct 2018 16:22:06 +0100 > > vitor <vitor.soares@synopsys.com> wrote: > > > >> Can you update the i3c/next tree? > > Done. > > Thanks. > > I will apply the driver and them I give you feedback. Great! Note that the bug in the ->send_ccc_cmd() path should be fixed, but you might have issues with i2c transfers as msg->buf is not guaranteed to be aligned on 32-bit. Don't know if you've followed the discussion with Arnd, but it seems some (most?) archs are making sure writesl()/readsl() work for unaligned bufs. Maybe we should fix that for ARC. The other solution is to use i2c_get_dma_safe_msg_buf() to get something aligned on a cache-line and by extension aligned on 32bits.
On Fri, Oct 26, 2018 at 04:43:24PM +0200, Boris Brezillon wrote: > Hi Greg, > > I think we've reached a point where we can eventually consider the I3C > framework for inclusion in 4.20 (5.0?). A few more issues were reported > on v9 and fixed in v10. I can't guarantee that the implementation is > free of bugs but I still think it's worth merging it in v4.20: it's a > new subsystem, so we don't risk regressions, and the only way we can > detect other issues is by having other people experiment with this > implementation. > > The only remaining concern raised by Arnd is the fact that both hosts > and slaves share the same bus type and are differentiated thanks to > their device_type, which IMHO is fine since this is what other > subsystems do (plus I don't see other solutions to have both I3C > devices and I3C buses represented under /sys/bus/i3c/). Yeah, it's not the nicest, but it will work, we did it also for USB and greybus and it solves the issue. This all looks good to me, so I've queued it up. Let's see if linux-next has any problems with it. Thanks for sticking with it, nice work! greg k-h
Hi Greg, On Sun, 11 Nov 2018 09:39:32 -0800 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Fri, Oct 26, 2018 at 04:43:24PM +0200, Boris Brezillon wrote: > > Hi Greg, > > > > I think we've reached a point where we can eventually consider the I3C > > framework for inclusion in 4.20 (5.0?). A few more issues were reported > > on v9 and fixed in v10. I can't guarantee that the implementation is > > free of bugs but I still think it's worth merging it in v4.20: it's a > > new subsystem, so we don't risk regressions, and the only way we can > > detect other issues is by having other people experiment with this > > implementation. > > > > The only remaining concern raised by Arnd is the fact that both hosts > > and slaves share the same bus type and are differentiated thanks to > > their device_type, which IMHO is fine since this is what other > > subsystems do (plus I don't see other solutions to have both I3C > > devices and I3C buses represented under /sys/bus/i3c/). > > Yeah, it's not the nicest, but it will work, we did it also for USB and > greybus and it solves the issue. > > This all looks good to me, so I've queued it up. Let's see if > linux-next has any problems with it. I recently asked Stephen to add the linux-i3c tree to linux-next, so I'm expecting conflicts :-/. Sorry, I didn't know you were planning to take these patches through your tree. BTW, I also fixed a couple of things when rebasing on top of 4.20-rc1: - KernelVersion in the sysfs ABI doc has been updated to 5.0 - Fixed i3c_master_getmxds_locked() (bug reported/fixed by Colin here https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1799850.html) - Removed a blank line at the end of master-driver-api.rst For the record, the i3c/next branch pulled by Stephen is available here [1]. > Thanks for sticking with it, nice work! Thanks for reviewing it! Greg, Stephen, let me know if you want me to reset i3c/next to v4.20-rc1 to avoid conflicts in linux-next. Regards, Boris [1]https://git.kernel.org/pub/scm/linux/kernel/git/i3c/linux.git/log/?h=i3c/next
On Sun, Nov 11, 2018 at 07:10:17PM +0100, Boris Brezillon wrote: > Hi Greg, > > On Sun, 11 Nov 2018 09:39:32 -0800 > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > On Fri, Oct 26, 2018 at 04:43:24PM +0200, Boris Brezillon wrote: > > > Hi Greg, > > > > > > I think we've reached a point where we can eventually consider the I3C > > > framework for inclusion in 4.20 (5.0?). A few more issues were reported > > > on v9 and fixed in v10. I can't guarantee that the implementation is > > > free of bugs but I still think it's worth merging it in v4.20: it's a > > > new subsystem, so we don't risk regressions, and the only way we can > > > detect other issues is by having other people experiment with this > > > implementation. > > > > > > The only remaining concern raised by Arnd is the fact that both hosts > > > and slaves share the same bus type and are differentiated thanks to > > > their device_type, which IMHO is fine since this is what other > > > subsystems do (plus I don't see other solutions to have both I3C > > > devices and I3C buses represented under /sys/bus/i3c/). > > > > Yeah, it's not the nicest, but it will work, we did it also for USB and > > greybus and it solves the issue. > > > > This all looks good to me, so I've queued it up. Let's see if > > linux-next has any problems with it. > > I recently asked Stephen to add the linux-i3c tree to linux-next, so > I'm expecting conflicts :-/. Sorry, I didn't know you were planning to > take these patches through your tree. > > BTW, I also fixed a couple of things when rebasing on top of 4.20-rc1: > > - KernelVersion in the sysfs ABI doc has been updated to 5.0 There is no 5.0 yet :) > - Fixed i3c_master_getmxds_locked() (bug reported/fixed by Colin here > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1799850.html) > - Removed a blank line at the end of master-driver-api.rst > > For the record, the i3c/next branch pulled by Stephen is available here > [1]. > > > Thanks for sticking with it, nice work! > > Thanks for reviewing it! Greg, Stephen, let me know if you want me to > reset i3c/next to v4.20-rc1 to avoid conflicts in linux-next. So do you want me to just drop these patches from my tree? If so, I can, but i have no problem sending these to Linus for the next -rc1 merge window through my tree if that is easier. It's up to you. greg k-h
On Sun, 11 Nov 2018 11:10:20 -0800 Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > On Sun, Nov 11, 2018 at 07:10:17PM +0100, Boris Brezillon wrote: > > Hi Greg, > > > > On Sun, 11 Nov 2018 09:39:32 -0800 > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > > > On Fri, Oct 26, 2018 at 04:43:24PM +0200, Boris Brezillon wrote: > > > > Hi Greg, > > > > > > > > I think we've reached a point where we can eventually consider the I3C > > > > framework for inclusion in 4.20 (5.0?). A few more issues were reported > > > > on v9 and fixed in v10. I can't guarantee that the implementation is > > > > free of bugs but I still think it's worth merging it in v4.20: it's a > > > > new subsystem, so we don't risk regressions, and the only way we can > > > > detect other issues is by having other people experiment with this > > > > implementation. > > > > > > > > The only remaining concern raised by Arnd is the fact that both hosts > > > > and slaves share the same bus type and are differentiated thanks to > > > > their device_type, which IMHO is fine since this is what other > > > > subsystems do (plus I don't see other solutions to have both I3C > > > > devices and I3C buses represented under /sys/bus/i3c/). > > > > > > Yeah, it's not the nicest, but it will work, we did it also for USB and > > > greybus and it solves the issue. > > > > > > This all looks good to me, so I've queued it up. Let's see if > > > linux-next has any problems with it. > > > > I recently asked Stephen to add the linux-i3c tree to linux-next, so > > I'm expecting conflicts :-/. Sorry, I didn't know you were planning to > > take these patches through your tree. > > > > BTW, I also fixed a couple of things when rebasing on top of 4.20-rc1: > > > > - KernelVersion in the sysfs ABI doc has been updated to 5.0 > > There is no 5.0 yet :) Actually I had a hard time choosing between 4.21 and 5.0, and then I saw Linus' email announcing 4.20-rc1 ;-). But I can change it back to 4.21 if you prefer. > > > - Fixed i3c_master_getmxds_locked() (bug reported/fixed by Colin here > > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1799850.html) > > - Removed a blank line at the end of master-driver-api.rst One extra thing I didn't mention (and didn't fix yet) is the I3C mailing list. I asked Dave Miller if he'd be okay to create the linux-i3c ML on vger.kernel.org, and he suggested that we use the linux-i2c ML instead which I know Wolfram is not fond of. I might decide to just put linux-kernel@vger.kernel.org as the ML to Cc for I3C patches until we settle on something. > > > > For the record, the i3c/next branch pulled by Stephen is available here > > [1]. > > > > > Thanks for sticking with it, nice work! > > > > Thanks for reviewing it! Greg, Stephen, let me know if you want me to > > reset i3c/next to v4.20-rc1 to avoid conflicts in linux-next. > > So do you want me to just drop these patches from my tree? If so, I > can, but i have no problem sending these to Linus for the next -rc1 > merge window through my tree if that is easier. > > It's up to you. I think that's easier for me and for you if I take them in the i3c tree and then send a PR to Linus myself. This way I won't bother you if fixes are needed or if I decide to apply patches adding support for other I3C controllers (I see you commented on the Synopsys driver already, and I might indeed decide to queue this patchset for this release). Doing that also allows me to get everything in place since I'll anyway have to send PRs to Linus at some point. Regards, Boris PS: If you're fine with me taking the I3C patches, I'll need your Acked-by.
On Sun, Nov 11, 2018 at 09:08:18PM +0100, Boris Brezillon wrote: > On Sun, 11 Nov 2018 11:10:20 -0800 > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > On Sun, Nov 11, 2018 at 07:10:17PM +0100, Boris Brezillon wrote: > > > Hi Greg, > > > > > > On Sun, 11 Nov 2018 09:39:32 -0800 > > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > > > > > On Fri, Oct 26, 2018 at 04:43:24PM +0200, Boris Brezillon wrote: > > > > > Hi Greg, > > > > > > > > > > I think we've reached a point where we can eventually consider the I3C > > > > > framework for inclusion in 4.20 (5.0?). A few more issues were reported > > > > > on v9 and fixed in v10. I can't guarantee that the implementation is > > > > > free of bugs but I still think it's worth merging it in v4.20: it's a > > > > > new subsystem, so we don't risk regressions, and the only way we can > > > > > detect other issues is by having other people experiment with this > > > > > implementation. > > > > > > > > > > The only remaining concern raised by Arnd is the fact that both hosts > > > > > and slaves share the same bus type and are differentiated thanks to > > > > > their device_type, which IMHO is fine since this is what other > > > > > subsystems do (plus I don't see other solutions to have both I3C > > > > > devices and I3C buses represented under /sys/bus/i3c/). > > > > > > > > Yeah, it's not the nicest, but it will work, we did it also for USB and > > > > greybus and it solves the issue. > > > > > > > > This all looks good to me, so I've queued it up. Let's see if > > > > linux-next has any problems with it. > > > > > > I recently asked Stephen to add the linux-i3c tree to linux-next, so > > > I'm expecting conflicts :-/. Sorry, I didn't know you were planning to > > > take these patches through your tree. > > > > > > BTW, I also fixed a couple of things when rebasing on top of 4.20-rc1: > > > > > > - KernelVersion in the sysfs ABI doc has been updated to 5.0 > > > > There is no 5.0 yet :) > > Actually I had a hard time choosing between 4.21 and 5.0, and then I > saw Linus' email announcing 4.20-rc1 ;-). But I can change it back to > 4.21 if you prefer. > > > > > > - Fixed i3c_master_getmxds_locked() (bug reported/fixed by Colin here > > > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1799850.html) > > > - Removed a blank line at the end of master-driver-api.rst > > One extra thing I didn't mention (and didn't fix yet) is the I3C > mailing list. I asked Dave Miller if he'd be okay to create the > linux-i3c ML on vger.kernel.org, and he suggested that we use the > linux-i2c ML instead which I know Wolfram is not fond of. I might > decide to just put linux-kernel@vger.kernel.org as the ML to Cc for > I3C patches until we settle on something. > > > > > > > For the record, the i3c/next branch pulled by Stephen is available here > > > [1]. > > > > > > > Thanks for sticking with it, nice work! > > > > > > Thanks for reviewing it! Greg, Stephen, let me know if you want me to > > > reset i3c/next to v4.20-rc1 to avoid conflicts in linux-next. > > > > So do you want me to just drop these patches from my tree? If so, I > > can, but i have no problem sending these to Linus for the next -rc1 > > merge window through my tree if that is easier. > > > > It's up to you. > > I think that's easier for me and for you if I take them in the i3c tree > and then send a PR to Linus myself. This way I won't bother you if > fixes are needed or if I decide to apply patches adding support for > other I3C controllers (I see you commented on the Synopsys driver > already, and I might indeed decide to queue this patchset for this > release). > > Doing that also allows me to get everything in place since I'll anyway > have to send PRs to Linus at some point. > > Regards, > > Boris > > PS: If you're fine with me taking the I3C patches, I'll need your > Acked-by. Sure, feel free to do it all yourself, I do not object to that! :) Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
On Sun, Nov 11, 2018 at 12:57:18PM -0800, Greg Kroah-Hartman wrote: > On Sun, Nov 11, 2018 at 09:08:18PM +0100, Boris Brezillon wrote: > > On Sun, 11 Nov 2018 11:10:20 -0800 > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > > > On Sun, Nov 11, 2018 at 07:10:17PM +0100, Boris Brezillon wrote: > > > > Hi Greg, > > > > > > > > On Sun, 11 Nov 2018 09:39:32 -0800 > > > > Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > > > > > > > > On Fri, Oct 26, 2018 at 04:43:24PM +0200, Boris Brezillon wrote: > > > > > > Hi Greg, > > > > > > > > > > > > I think we've reached a point where we can eventually consider the I3C > > > > > > framework for inclusion in 4.20 (5.0?). A few more issues were reported > > > > > > on v9 and fixed in v10. I can't guarantee that the implementation is > > > > > > free of bugs but I still think it's worth merging it in v4.20: it's a > > > > > > new subsystem, so we don't risk regressions, and the only way we can > > > > > > detect other issues is by having other people experiment with this > > > > > > implementation. > > > > > > > > > > > > The only remaining concern raised by Arnd is the fact that both hosts > > > > > > and slaves share the same bus type and are differentiated thanks to > > > > > > their device_type, which IMHO is fine since this is what other > > > > > > subsystems do (plus I don't see other solutions to have both I3C > > > > > > devices and I3C buses represented under /sys/bus/i3c/). > > > > > > > > > > Yeah, it's not the nicest, but it will work, we did it also for USB and > > > > > greybus and it solves the issue. > > > > > > > > > > This all looks good to me, so I've queued it up. Let's see if > > > > > linux-next has any problems with it. > > > > > > > > I recently asked Stephen to add the linux-i3c tree to linux-next, so > > > > I'm expecting conflicts :-/. Sorry, I didn't know you were planning to > > > > take these patches through your tree. > > > > > > > > BTW, I also fixed a couple of things when rebasing on top of 4.20-rc1: > > > > > > > > - KernelVersion in the sysfs ABI doc has been updated to 5.0 > > > > > > There is no 5.0 yet :) > > > > Actually I had a hard time choosing between 4.21 and 5.0, and then I > > saw Linus' email announcing 4.20-rc1 ;-). But I can change it back to > > 4.21 if you prefer. > > > > > > > > > - Fixed i3c_master_getmxds_locked() (bug reported/fixed by Colin here > > > > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1799850.html) > > > > - Removed a blank line at the end of master-driver-api.rst > > > > One extra thing I didn't mention (and didn't fix yet) is the I3C > > mailing list. I asked Dave Miller if he'd be okay to create the > > linux-i3c ML on vger.kernel.org, and he suggested that we use the > > linux-i2c ML instead which I know Wolfram is not fond of. I might > > decide to just put linux-kernel@vger.kernel.org as the ML to Cc for > > I3C patches until we settle on something. > > > > > > > > > > For the record, the i3c/next branch pulled by Stephen is available here > > > > [1]. > > > > > > > > > Thanks for sticking with it, nice work! > > > > > > > > Thanks for reviewing it! Greg, Stephen, let me know if you want me to > > > > reset i3c/next to v4.20-rc1 to avoid conflicts in linux-next. > > > > > > So do you want me to just drop these patches from my tree? If so, I > > > can, but i have no problem sending these to Linus for the next -rc1 > > > merge window through my tree if that is easier. > > > > > > It's up to you. > > > > I think that's easier for me and for you if I take them in the i3c tree > > and then send a PR to Linus myself. This way I won't bother you if > > fixes are needed or if I decide to apply patches adding support for > > other I3C controllers (I see you commented on the Synopsys driver > > already, and I might indeed decide to queue this patchset for this > > release). > > > > Doing that also allows me to get everything in place since I'll anyway > > have to send PRs to Linus at some point. > > > > Regards, > > > > Boris > > > > PS: If you're fine with me taking the I3C patches, I'll need your > > Acked-by. > > Sure, feel free to do it all yourself, I do not object to that! :) > > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> And I've now dropped all of these patches from my tree, so no need to worry about any conflicts. thanks, greg k-h
Hi Boris, Given the current state of the subsystem I think it might worth start to think how to expose the devices under /dev. My initial thoughts are to do the same think as for i2c, expose the buses or the i3c_devices and use ioctl for private transfers. Some direct CCC commands can be sent through the /sys as you plan for SETNEWDA . What do you think about this? Best regards, Vitor Soares
+Mark Brown for the question about /dev/spidev Hi Vitor, On Thu, 15 Nov 2018 12:14:37 +0000 vitor <vitor.soares@synopsys.com> wrote: > Hi Boris, > > Given the current state of the subsystem I think it might worth start to > think how to expose the devices under /dev. Thanks for starting this discussion. I'm not against the idea in general, we just need to be careful when doing that. > My initial thoughts are to do the same think as for i2c, expose the > buses or the i3c_devices and use ioctl for private transfers. Exposing the bus is dangerous IMO, because an I3C bus is not like an I2C bus: * I3C device needs to be discovered through DAA * I2C devices need to be declared ahead of time, and LVR is used to determine the limitations on the bus at runtime So you'd anyway be able to interact only with devices that have previously been discovered. Note that the virtual I2C bus is already exposed, but any command targeting an address that is not attached to a registered I2C dev will get a -ENOENT error. What we could do though, is expose I3C devices that do not have a driver in kernel space, like spidev does. > Some > direct CCC commands can be sent through the /sys as you plan for SETNEWDA . Yes, CCC commands that need to be exposed to userspace should be exposed through sysfs, or, if we decide to create a /dev/i3cX device per bus, through ioctls. > > What do you think about this? I think this request is perfectly valid, we just need to decide how it should be done, and before we take this decision, I'd like to get inputs from other maintainers. Mark, Wolfram, Arnd, Greg, any opinion? Regards, Boris
On Thu, Nov 15, 2018 at 4:58 AM Boris Brezillon <boris.brezillon@bootlin.com> wrote: > +Mark Brown for the question about /dev/spidev > On Thu, 15 Nov 2018 12:14:37 +0000 > vitor <vitor.soares@synopsys.com> wrote: > > My initial thoughts are to do the same think as for i2c, expose the > > buses or the i3c_devices and use ioctl for private transfers. > > Exposing the bus is dangerous IMO, because an I3C bus is not like an > I2C bus: > > * I3C device needs to be discovered through DAA > * I2C devices need to be declared ahead of time, and LVR is used to > determine the limitations on the bus at runtime > > So you'd anyway be able to interact only with devices that have > previously been discovered. > > Note that the virtual I2C bus is already exposed, but any command > targeting an address that is not attached to a registered I2C dev will > get a -ENOENT error. > > What we could do though, is expose I3C devices that do not have a > driver in kernel space, like spidev does. > > > Some > > direct CCC commands can be sent through the /sys as you plan for SETNEWDA . > > Yes, CCC commands that need to be exposed to userspace should be > exposed through sysfs, or, if we decide to create a /dev/i3cX device > per bus, through ioctls. > > > > > What do you think about this? > > I think this request is perfectly valid, we just need to decide how it > should be done, and before we take this decision, I'd like to get > inputs from other maintainers. > > Mark, Wolfram, Arnd, Greg, any opinion? I think for a new user space interface, it makes sense to explore a number of different options before making the final decision. I agree about better not exposing the bus as a /dev/i3c* node, and that we probably do need to expose individual devices in some form to allow writing complete user space drivers that can do everything a kernel driver can do. Can you describe what a low-level interface to the device looks like in the kernel? Can this be abstracted as simply pread()/pwrite() plus an interrupt mechanism, or do we need a set of ioctl() operations as well? If it can be purely based on a regmap abstraction, a sysfs inteface might be sufficient, though that has some downsides with permission management compared to a /dev/* node. Another option might be the use of a socket interface, which also has some issues in terms of permission management, but might be a good fit if we could abstract bus transactions as packets that can be queued. Arnd
Hi Boris, > What we could do though, is expose I3C devices that do not have a > driver in kernel space, like spidev does. ... > Mark, Wolfram, Arnd, Greg, any opinion? Is there a benefit for having drivers in userspace? My gut feeling is to encourage people to write kernel drivers. If this is, for some reason, not possible for some driver, then we have a use case at hand to test the then-to-be-developed userspace interface against. Until then, I personally wouldn't waste effort on designing it without a user in sight. Dunno if you have that, but a debug interface (exchanging data with clients) on the other hand would be super useful most probably. Maybe you can start having that in debugfs and already learn from it if you ever want to move some interface outside of debugfs? Kind regards, Wolfram
On Thu, Nov 15, 2018 at 7:01 AM Wolfram Sang <wsa@the-dreams.de> wrote: > > What we could do though, is expose I3C devices that do not have a > > driver in kernel space, like spidev does. > > ... > > > Mark, Wolfram, Arnd, Greg, any opinion? > > Is there a benefit for having drivers in userspace? My gut feeling is to > encourage people to write kernel drivers. If this is, for some reason, > not possible for some driver, then we have a use case at hand to test > the then-to-be-developed userspace interface against. Until then, I > personally wouldn't waste effort on designing it without a user in > sight. > > Dunno if you have that, but a debug interface (exchanging data with > clients) on the other hand would be super useful most probably. Maybe > you can start having that in debugfs and already learn from it if you > ever want to move some interface outside of debugfs? I think it may depend a little bit on the complexity we require for a user interface. If it's basically just pread/pwrite, then the debugfs would not look any different from a stable interface, and there is little risk of getting it wrong. The more complex the interface turns out to be, the more cautious we may want to be about declaring it stable. Other than that, I agree we should encourage users to write kernel drivers, but given the precedent of uio, libusb, spidev, i2c-dev, and vfio, it does seem extremely likely that users will have requirements for it, and I think it's a good idea to start the design discussion before users start building their own interfaces to do the same thing badly. Arnd
Hi, On 15/11/18 12:57, Boris Brezillon wrote: > +Mark Brown for the question about /dev/spidev > > Hi Vitor, > > On Thu, 15 Nov 2018 12:14:37 +0000 > vitor <vitor.soares@synopsys.com> wrote: > >> Hi Boris, >> >> Given the current state of the subsystem I think it might worth start to >> think how to expose the devices under /dev. > Thanks for starting this discussion. I'm not against the idea in > general, we just need to be careful when doing that. > >> My initial thoughts are to do the same think as for i2c, expose the >> buses or the i3c_devices and use ioctl for private transfers. > Exposing the bus is dangerous IMO, because an I3C bus is not like an > I2C bus: > > * I3C device needs to be discovered through DAA > * I2C devices need to be declared ahead of time, and LVR is used to > determine the limitations on the bus at runtime > > So you'd anyway be able to interact only with devices that have > previously been discovered. > > Note that the virtual I2C bus is already exposed, but any command > targeting an address that is not attached to a registered I2C dev will > get a -ENOENT error. I initially thought to do the same thing for the i3c devices adding a routine get_i3c_dev_by_addr()... > > What we could do though, is expose I3C devices that do not have a > driver in kernel space, like spidev does. ...but I like more this approach. >> Some >> direct CCC commands can be sent through the /sys as you plan for SETNEWDA . > Yes, CCC commands that need to be exposed to userspace should be > exposed through sysfs, or, if we decide to create a /dev/i3cX device > per bus, through ioctls. There already some attributes exposed, just need to add the possibility to write to them and off course add some that are missing like GETSTATUS. > >> What do you think about this? > I think this request is perfectly valid, we just need to decide how it > should be done, and before we take this decision, I'd like to get > inputs from other maintainers. > > Mark, Wolfram, Arnd, Greg, any opinion? > > Regards, > > Boris Best regards, Vitor Soares
Hi Arnd, On 15/11/18 14:25, Arnd Bergmann wrote: > I agree about better not exposing the bus as a /dev/i3c* node, and that we > probably do need to expose individual devices in some form to allow > writing complete user space drivers that can do everything a kernel driver > can do. > > Can you describe what a low-level interface to the device looks like > in the kernel? Can this be abstracted as simply pread()/pwrite() plus > an interrupt mechanism, or do we need a set of ioctl() operations as > well? Like in i2c is likely to need the ioctl() too. Best regards, Vitor Soares
On Thu, 15 Nov 2018 16:01:37 +0100 Wolfram Sang <wsa@the-dreams.de> wrote: > Hi Boris, > > > What we could do though, is expose I3C devices that do not have a > > driver in kernel space, like spidev does. > > ... > > > Mark, Wolfram, Arnd, Greg, any opinion? > > Is there a benefit for having drivers in userspace? My gut feeling is to > encourage people to write kernel drivers. If this is, for some reason, > not possible for some driver, then we have a use case at hand to test > the then-to-be-developed userspace interface against. Until then, I > personally wouldn't waste effort on designing it without a user in > sight. I kind of agree with that. Vitor, do you have a use case in mind for such userspace drivers? I don't think it's worth designing an API for something we don't need (yet).
Hi Boris, On 15/11/18 15:28, Boris Brezillon wrote: > On Thu, 15 Nov 2018 16:01:37 +0100 > Wolfram Sang <wsa@the-dreams.de> wrote: > >> Hi Boris, >> >>> What we could do though, is expose I3C devices that do not have a >>> driver in kernel space, like spidev does. >> ... >> >>> Mark, Wolfram, Arnd, Greg, any opinion? >> Is there a benefit for having drivers in userspace? My gut feeling is to >> encourage people to write kernel drivers. If this is, for some reason, >> not possible for some driver, then we have a use case at hand to test >> the then-to-be-developed userspace interface against. Until then, I >> personally wouldn't waste effort on designing it without a user in >> sight. > I kind of agree with that. Vitor, do you have a use case in mind for > such userspace drivers? I don't think it's worth designing an API for > something we don't need (yet). My use case is a tool for tests, lets say like the i2c tools. There is other subsystems, some of them mentioned on this thread, that have and ioctl system call or other method to change parameters or send data. I rise this topic because I really think it worth to define now how this should be design (and for me how to do the things right) to avoid future issues. Best regards, Vitor Soares
On Thu, 15 Nov 2018 18:03:47 +0000 vitor <vitor.soares@synopsys.com> wrote: > Hi Boris, > > > On 15/11/18 15:28, Boris Brezillon wrote: > > On Thu, 15 Nov 2018 16:01:37 +0100 > > Wolfram Sang <wsa@the-dreams.de> wrote: > > > >> Hi Boris, > >> > >>> What we could do though, is expose I3C devices that do not have a > >>> driver in kernel space, like spidev does. > >> ... > >> > >>> Mark, Wolfram, Arnd, Greg, any opinion? > >> Is there a benefit for having drivers in userspace? My gut feeling is to > >> encourage people to write kernel drivers. If this is, for some reason, > >> not possible for some driver, then we have a use case at hand to test > >> the then-to-be-developed userspace interface against. Until then, I > >> personally wouldn't waste effort on designing it without a user in > >> sight. > > I kind of agree with that. Vitor, do you have a use case in mind for > > such userspace drivers? I don't think it's worth designing an API for > > something we don't need (yet). > > My use case is a tool for tests, lets say like the i2c tools. What would you like to test exactly? > There is > other subsystems, some of them mentioned on this thread, that have and > ioctl system call or other method to change parameters or send data. I don't think they added the /dev interface before having a real use case for it. > > > I rise this topic because I really think it worth to define now how this > should be design (and for me how to do the things right) to avoid future > issues. Actually it should be done the other way around: you should have a real need and the /dev interface should be designed to fulfill this need. Based on this real use case we can discuss other potential usage that might appear in the future and try to design something more future-proof, but clearly, this userspace interface should be driven by a real/well-defined use case. Also, exposing things to userspace is way more risky than adding a new in-kernel subsystem/framework, because it then becomes part of the stable ABI. To make things clearer, I'm not against the idea of exposing I3C devices (or I3C buses) to userspace, but I'd like to understand what you plan to do with that. If this is about testing, what kind of tests you'd like to run. If this is about developing drivers in userspace, why can't these be done in kernel space (license issues?), and what would those drivers be allowed to do?
On Thu, 15 Nov 2018 15:25:11 +0000 vitor <vitor.soares@synopsys.com> wrote: > Hi Arnd, > > On 15/11/18 14:25, Arnd Bergmann wrote: > > I agree about better not exposing the bus as a /dev/i3c* node, and that we > > probably do need to expose individual devices in some form to allow > > writing complete user space drivers that can do everything a kernel driver > > can do. > > > > Can you describe what a low-level interface to the device looks like > > in the kernel? Can this be abstracted as simply pread()/pwrite() plus > > an interrupt mechanism, or do we need a set of ioctl() operations as > > well? > > Like in i2c is likely to need the ioctl() too. Yep, I think we'll need an ioctl given the various type of transfers one case use to interact with a device.
On Thu, Nov 15, 2018 at 01:57:31PM +0100, Boris Brezillon wrote: > What we could do though, is expose I3C devices that do not have a > driver in kernel space, like spidev does. Yes, this is much safer and more robust especially if the bus has enumeration requirements like you're saying it does. It's much easier and more sensible to do this if the bus can be enumerated than it is with SPI, USB would be a good example here I guess (though I've never looked at the details of how USB does this on the implementation side so this may be me speaking in blissful ignorance). I do agree that there will be use cases that turn up for this.
Hi Boris, On 15/11/18 19:00, Boris Brezillon wrote: > On Thu, 15 Nov 2018 18:03:47 +0000 > vitor <vitor.soares@synopsys.com> wrote: > >> Hi Boris, >> >> >> On 15/11/18 15:28, Boris Brezillon wrote: >>> On Thu, 15 Nov 2018 16:01:37 +0100 >>> Wolfram Sang <wsa@the-dreams.de> wrote: >>> >>>> Hi Boris, >>>> >>>>> What we could do though, is expose I3C devices that do not have a >>>>> driver in kernel space, like spidev does. >>>> ... >>>> >>>>> Mark, Wolfram, Arnd, Greg, any opinion? >>>> Is there a benefit for having drivers in userspace? My gut feeling is to >>>> encourage people to write kernel drivers. If this is, for some reason, >>>> not possible for some driver, then we have a use case at hand to test >>>> the then-to-be-developed userspace interface against. Until then, I >>>> personally wouldn't waste effort on designing it without a user in >>>> sight. >>> I kind of agree with that. Vitor, do you have a use case in mind for >>> such userspace drivers? I don't think it's worth designing an API for >>> something we don't need (yet). >> My use case is a tool for tests, lets say like the i2c tools. > What would you like to test exactly? > >> There is >> other subsystems, some of them mentioned on this thread, that have and >> ioctl system call or other method to change parameters or send data. > I don't think they added the /dev interface before having a real use > case for it. > >> >> I rise this topic because I really think it worth to define now how this >> should be design (and for me how to do the things right) to avoid future >> issues. > Actually it should be done the other way around: you should have a real > need and the /dev interface should be designed to fulfill this need. > Based on this real use case we can discuss other potential usage that > might appear in the future and try to design something more > future-proof, but clearly, this userspace interface should be driven by > a real/well-defined use case. > > Also, exposing things to userspace is way more risky than adding a new > in-kernel subsystem/framework, because it then becomes part of the > stable ABI. > > To make things clearer, I'm not against the idea of exposing I3C > devices (or I3C buses) to userspace, but I'd like to understand what you > plan to do with that. If this is about testing, what kind of tests > you'd like to run. If this is about developing drivers in userspace, > why can't these be done in kernel space (license issues?), and what > would those drivers be allowed to do? Basically I need a tool that help me during the development and to avoid me to write a dummy driver for each device that I test. For instances do some read/write, get/set ccc commands, if something goes wrong during the bus initialization have a to debug etc... For me this is a valid use case and I imagine when people start to develop in i3c this interface will help everyone. Best regards, Vitor Soares
Hi Vitor, On 11/16/18, 1:32 PM, "vitor" <vitor.soares@synopsys.com> wrote: EXTERNAL MAIL Hi Boris, On 15/11/18 19:00, Boris Brezillon wrote: > On Thu, 15 Nov 2018 18:03:47 +0000 > vitor <vitor.soares@synopsys.com> wrote: > >> Hi Boris, >> >> >> On 15/11/18 15:28, Boris Brezillon wrote: >>> On Thu, 15 Nov 2018 16:01:37 +0100 >>> Wolfram Sang <wsa@the-dreams.de> wrote: >>> >>>> Hi Boris, >>>> >>>>> What we could do though, is expose I3C devices that do not have a >>>>> driver in kernel space, like spidev does. >>>> ... >>>> >>>>> Mark, Wolfram, Arnd, Greg, any opinion? >>>> Is there a benefit for having drivers in userspace? My gut feeling is to >>>> encourage people to write kernel drivers. If this is, for some reason, >>>> not possible for some driver, then we have a use case at hand to test >>>> the then-to-be-developed userspace interface against. Until then, I >>>> personally wouldn't waste effort on designing it without a user in >>>> sight. >>> I kind of agree with that. Vitor, do you have a use case in mind for >>> such userspace drivers? I don't think it's worth designing an API for >>> something we don't need (yet). >> My use case is a tool for tests, lets say like the i2c tools. > What would you like to test exactly? > >> There is >> other subsystems, some of them mentioned on this thread, that have and >> ioctl system call or other method to change parameters or send data. > I don't think they added the /dev interface before having a real use > case for it. > >> >> I rise this topic because I really think it worth to define now how this >> should be design (and for me how to do the things right) to avoid future >> issues. > Actually it should be done the other way around: you should have a real > need and the /dev interface should be designed to fulfill this need. > Based on this real use case we can discuss other potential usage that > might appear in the future and try to design something more > future-proof, but clearly, this userspace interface should be driven by > a real/well-defined use case. > > Also, exposing things to userspace is way more risky than adding a new > in-kernel subsystem/framework, because it then becomes part of the > stable ABI. > > To make things clearer, I'm not against the idea of exposing I3C > devices (or I3C buses) to userspace, but I'd like to understand what you > plan to do with that. If this is about testing, what kind of tests > you'd like to run. If this is about developing drivers in userspace, > why can't these be done in kernel space (license issues?), and what > would those drivers be allowed to do? Basically I need a tool that help me during the development and to avoid me to write a dummy driver for each device that I test. For now we are doing it that way. Separate dummy driver for each device. For instances do some read/write, get/set ccc commands, if something goes wrong during the bus initialization have a to debug etc... That sounds good to have possibility to make simple reads/writes and send basic ccc commands. But of course keeping in mind that I3C bus is more complicated than I2C, as Boris mentioned before. For me this is a valid use case and I imagine when people start to develop in i3c this interface will help everyone. Best regards, Vitor Soares Regards, Przemek
On Fri, 16 Nov 2018 12:31:42 +0000 vitor <vitor.soares@synopsys.com> wrote: > Hi Boris, > > > On 15/11/18 19:00, Boris Brezillon wrote: > > On Thu, 15 Nov 2018 18:03:47 +0000 > > vitor <vitor.soares@synopsys.com> wrote: > > > >> Hi Boris, > >> > >> > >> On 15/11/18 15:28, Boris Brezillon wrote: > >>> On Thu, 15 Nov 2018 16:01:37 +0100 > >>> Wolfram Sang <wsa@the-dreams.de> wrote: > >>> > >>>> Hi Boris, > >>>> > >>>>> What we could do though, is expose I3C devices that do not have a > >>>>> driver in kernel space, like spidev does. > >>>> ... > >>>> > >>>>> Mark, Wolfram, Arnd, Greg, any opinion? > >>>> Is there a benefit for having drivers in userspace? My gut feeling is to > >>>> encourage people to write kernel drivers. If this is, for some reason, > >>>> not possible for some driver, then we have a use case at hand to test > >>>> the then-to-be-developed userspace interface against. Until then, I > >>>> personally wouldn't waste effort on designing it without a user in > >>>> sight. > >>> I kind of agree with that. Vitor, do you have a use case in mind for > >>> such userspace drivers? I don't think it's worth designing an API for > >>> something we don't need (yet). > >> My use case is a tool for tests, lets say like the i2c tools. > > What would you like to test exactly? > > > >> There is > >> other subsystems, some of them mentioned on this thread, that have and > >> ioctl system call or other method to change parameters or send data. > > I don't think they added the /dev interface before having a real use > > case for it. > > > >> > >> I rise this topic because I really think it worth to define now how this > >> should be design (and for me how to do the things right) to avoid future > >> issues. > > Actually it should be done the other way around: you should have a real > > need and the /dev interface should be designed to fulfill this need. > > Based on this real use case we can discuss other potential usage that > > might appear in the future and try to design something more > > future-proof, but clearly, this userspace interface should be driven by > > a real/well-defined use case. > > > > Also, exposing things to userspace is way more risky than adding a new > > in-kernel subsystem/framework, because it then becomes part of the > > stable ABI. > > > > To make things clearer, I'm not against the idea of exposing I3C > > devices (or I3C buses) to userspace, but I'd like to understand what you > > plan to do with that. If this is about testing, what kind of tests > > you'd like to run. If this is about developing drivers in userspace, > > why can't these be done in kernel space (license issues?), and what > > would those drivers be allowed to do? > > > Basically I need a tool that help me during the development and to avoid > me to write a dummy driver for each device that I test. But we want I3C device drivers to be upstreamed, so why not developing a real driver everytime you test a new device and submitting it upstream? > > For instances do some read/write, Doing SDR/DDR transfers is probably acceptable, but I still think we should push hard to have kernel drivers when that's possible. > get/set ccc commands, Exposing CCC commands is definitely not a good idea, since they're not even exposed to kernel drivers. > if something > goes wrong during the bus initialization have a to debug etc... Can't we add such a debug infrastructure in the kernel. Maybe we can expose debugfs files too if that helps, though if those debugfs files are actually used by userspace libs/tools, it's not any better than ioctls or sysfs files, since they will anyway become a stable ABI. > > > For me this is a valid use case and I imagine when people start to > develop in i3c this interface will help everyone. How about you propose an i3cdev driver that allow users to do SDR transfers throuh an ioctl?
Hi Boris, On 16/11/18 13:16, Boris Brezillon wrote: > On Fri, 16 Nov 2018 12:31:42 +0000 > vitor <vitor.soares@synopsys.com> wrote: > >> Hi Boris, >> >> >> On 15/11/18 19:00, Boris Brezillon wrote: >>> On Thu, 15 Nov 2018 18:03:47 +0000 >>> vitor <vitor.soares@synopsys.com> wrote: >>> >>>> Hi Boris, >>>> >>>> >>>> On 15/11/18 15:28, Boris Brezillon wrote: >>>>> On Thu, 15 Nov 2018 16:01:37 +0100 >>>>> Wolfram Sang <wsa@the-dreams.de> wrote: >>>>> >>>>>> Hi Boris, >>>>>> >>>>>>> What we could do though, is expose I3C devices that do not have a >>>>>>> driver in kernel space, like spidev does. >>>>>> ... >>>>>> >>>>>>> Mark, Wolfram, Arnd, Greg, any opinion? >>>>>> Is there a benefit for having drivers in userspace? My gut feeling is to >>>>>> encourage people to write kernel drivers. If this is, for some reason, >>>>>> not possible for some driver, then we have a use case at hand to test >>>>>> the then-to-be-developed userspace interface against. Until then, I >>>>>> personally wouldn't waste effort on designing it without a user in >>>>>> sight. >>>>> I kind of agree with that. Vitor, do you have a use case in mind for >>>>> such userspace drivers? I don't think it's worth designing an API for >>>>> something we don't need (yet). >>>> My use case is a tool for tests, lets say like the i2c tools. >>> What would you like to test exactly? >>> >>>> There is >>>> other subsystems, some of them mentioned on this thread, that have and >>>> ioctl system call or other method to change parameters or send data. >>> I don't think they added the /dev interface before having a real use >>> case for it. >>> >>>> I rise this topic because I really think it worth to define now how this >>>> should be design (and for me how to do the things right) to avoid future >>>> issues. >>> Actually it should be done the other way around: you should have a real >>> need and the /dev interface should be designed to fulfill this need. >>> Based on this real use case we can discuss other potential usage that >>> might appear in the future and try to design something more >>> future-proof, but clearly, this userspace interface should be driven by >>> a real/well-defined use case. >>> >>> Also, exposing things to userspace is way more risky than adding a new >>> in-kernel subsystem/framework, because it then becomes part of the >>> stable ABI. >>> >>> To make things clearer, I'm not against the idea of exposing I3C >>> devices (or I3C buses) to userspace, but I'd like to understand what you >>> plan to do with that. If this is about testing, what kind of tests >>> you'd like to run. If this is about developing drivers in userspace, >>> why can't these be done in kernel space (license issues?), and what >>> would those drivers be allowed to do? >> >> Basically I need a tool that help me during the development and to avoid >> me to write a dummy driver for each device that I test. > But we want I3C device drivers to be upstreamed, so why not developing a > real driver everytime you test a new device and submitting it upstream? Usually the devices that I test aren't the final product so it isn't easy to do the upstream. But when possible I plan to do that. > >> For instances do some read/write, > Doing SDR/DDR transfers is probably acceptable, but I still think we > should push hard to have kernel drivers when that's possible. > >> get/set ccc commands, > Exposing CCC commands is definitely not a good idea, since they're not > even exposed to kernel drivers. > >> if something >> goes wrong during the bus initialization have a to debug etc... > Can't we add such a debug infrastructure in the kernel. Maybe we can > expose debugfs files too if that helps, though if those debugfs files > are actually used by userspace libs/tools, it's not any better than > ioctls or sysfs files, since they will anyway become a stable ABI. > >> >> For me this is a valid use case and I imagine when people start to >> develop in i3c this interface will help everyone. > How about you propose an i3cdev driver that allow users to do SDR > transfers throuh an ioctl? I think that was for v6 I started to something to expose the bus like in i2c-dev, but I liked the idea of expose only the device doesn't have a driver. Do you know if there is already something in the kernel doing the same? Best regards, Vitor Soares
On Mon, 19 Nov 2018 12:35:42 +0000 vitor <vitor.soares@synopsys.com> wrote: > Hi Boris, > > On 16/11/18 13:16, Boris Brezillon wrote: > > On Fri, 16 Nov 2018 12:31:42 +0000 > > vitor <vitor.soares@synopsys.com> wrote: > > > >> Hi Boris, > >> > >> > >> On 15/11/18 19:00, Boris Brezillon wrote: > >>> On Thu, 15 Nov 2018 18:03:47 +0000 > >>> vitor <vitor.soares@synopsys.com> wrote: > >>> > >>>> Hi Boris, > >>>> > >>>> > >>>> On 15/11/18 15:28, Boris Brezillon wrote: > >>>>> On Thu, 15 Nov 2018 16:01:37 +0100 > >>>>> Wolfram Sang <wsa@the-dreams.de> wrote: > >>>>> > >>>>>> Hi Boris, > >>>>>> > >>>>>>> What we could do though, is expose I3C devices that do not have a > >>>>>>> driver in kernel space, like spidev does. > >>>>>> ... > >>>>>> > >>>>>>> Mark, Wolfram, Arnd, Greg, any opinion? > >>>>>> Is there a benefit for having drivers in userspace? My gut feeling is to > >>>>>> encourage people to write kernel drivers. If this is, for some reason, > >>>>>> not possible for some driver, then we have a use case at hand to test > >>>>>> the then-to-be-developed userspace interface against. Until then, I > >>>>>> personally wouldn't waste effort on designing it without a user in > >>>>>> sight. > >>>>> I kind of agree with that. Vitor, do you have a use case in mind for > >>>>> such userspace drivers? I don't think it's worth designing an API for > >>>>> something we don't need (yet). > >>>> My use case is a tool for tests, lets say like the i2c tools. > >>> What would you like to test exactly? > >>> > >>>> There is > >>>> other subsystems, some of them mentioned on this thread, that have and > >>>> ioctl system call or other method to change parameters or send data. > >>> I don't think they added the /dev interface before having a real use > >>> case for it. > >>> > >>>> I rise this topic because I really think it worth to define now how this > >>>> should be design (and for me how to do the things right) to avoid future > >>>> issues. > >>> Actually it should be done the other way around: you should have a real > >>> need and the /dev interface should be designed to fulfill this need. > >>> Based on this real use case we can discuss other potential usage that > >>> might appear in the future and try to design something more > >>> future-proof, but clearly, this userspace interface should be driven by > >>> a real/well-defined use case. > >>> > >>> Also, exposing things to userspace is way more risky than adding a new > >>> in-kernel subsystem/framework, because it then becomes part of the > >>> stable ABI. > >>> > >>> To make things clearer, I'm not against the idea of exposing I3C > >>> devices (or I3C buses) to userspace, but I'd like to understand what you > >>> plan to do with that. If this is about testing, what kind of tests > >>> you'd like to run. If this is about developing drivers in userspace, > >>> why can't these be done in kernel space (license issues?), and what > >>> would those drivers be allowed to do? > >> > >> Basically I need a tool that help me during the development and to avoid > >> me to write a dummy driver for each device that I test. > > But we want I3C device drivers to be upstreamed, so why not developing a > > real driver everytime you test a new device and submitting it upstream? > > > Usually the devices that I test aren't the final product so it isn't > easy to do the upstream. > > But when possible I plan to do that. > > > > > >> For instances do some read/write, > > Doing SDR/DDR transfers is probably acceptable, but I still think we > > should push hard to have kernel drivers when that's possible. > > > >> get/set ccc commands, > > Exposing CCC commands is definitely not a good idea, since they're not > > even exposed to kernel drivers. > > > >> if something > >> goes wrong during the bus initialization have a to debug etc... > > Can't we add such a debug infrastructure in the kernel. Maybe we can > > expose debugfs files too if that helps, though if those debugfs files > > are actually used by userspace libs/tools, it's not any better than > > ioctls or sysfs files, since they will anyway become a stable ABI. > > > >> > >> For me this is a valid use case and I imagine when people start to > >> develop in i3c this interface will help everyone. > > How about you propose an i3cdev driver that allow users to do SDR > > transfers throuh an ioctl? > > I think that was for v6 I started to something to expose the bus like in > i2c-dev, but I liked the idea of expose only the device doesn't have a > driver. Do you know if there is already something in the kernel doing > the same? I know [1], but there might be other subsystems doing the same thing. [1]https://elixir.bootlin.com/linux/v4.20-rc3/source/drivers/spi/spidev.c
On 19/11/18 12:43, Boris Brezillon wrote: > I know [1], but there might be other subsystems doing the same thing. > > [1]https://urldefense.proofpoint.com/v2/url?u=https-3A__elixir.bootlin.com_linux_v4.20-2Drc3_source_drivers_spi_spidev.c&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=qVuU64u9x77Y0Kd0PhDK_lpxFgg6PK9PateHwjb_DY0&m=S_YF5FjfGFFGFa7x6nHrT6npsIFjUbYRorSANaPpaiI&s=xXnwGBSGrVxandwp6biURmXq-85iMMLd8-n2McDsIWY&e= Thanks.