| Message ID | 20230910123949.1251964-1-alexander.usyskin@intel.com |
|---|---|
| Headers | show |
| Series | drm/i915/spi: spi access for discrete graphics | expand |
Hi Alexander, + Mark Brown + spi list + spi-nor maintainers alexander.usyskin@intel.com wrote on Sun, 10 Sep 2023 15:39:39 +0300: > Add driver for access to the discrete graphics card > internal SPI device. > Expose device on auxiliary bus and provide driver to register > this device with MTD framework. Maybe you can explain why you think auxiliary bus is relevant here? The cover letter might maybe be a bit more verbose to give us more context? I've looked at the series, it looks like you try to expose a spi memory connected to a spi controller on your hardware. We usually expect the spi controller driver to register in the spi core and provide spi-mem operations for that. I don't know if this memory is supposed to be used as general purpose, but if it's not I would advise to use some kind of firmware mechanism instead. Also, what is the purpose of exposing this content in this case? Well, I'm partially convinced here, I would like to hear from the other maintainers, maybe your choices are legitimate and I'm off topic. Thanks, Miquèl > This series is intended to be upstreamed through drm tree. > > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com> > > > Alexander Usyskin (3): > drm/i915/spi: align 64bit read and write > drm/i915/spi: wake card on operations > drm/i915/spi: add support for access mode > > Jani Nikula (1): > drm/i915/spi: add spi device for discrete graphics > > Tomas Winkler (6): > drm/i915/spi: add intel_spi_region map > drm/i915/spi: add driver for on-die spi device > drm/i915/spi: implement region enumeration > drm/i915/spi: implement spi access functions > drm/i915/spi: spi register with mtd > drm/i915/spi: mtd: implement access handlers > > drivers/gpu/drm/i915/Kconfig | 1 + > drivers/gpu/drm/i915/Makefile | 6 + > drivers/gpu/drm/i915/i915_driver.c | 7 + > drivers/gpu/drm/i915/i915_drv.h | 4 + > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/spi/intel_spi.c | 101 +++ > drivers/gpu/drm/i915/spi/intel_spi.h | 33 + > drivers/gpu/drm/i915/spi/intel_spi_drv.c | 865 +++++++++++++++++++++++ > 8 files changed, 1018 insertions(+) > create mode 100644 drivers/gpu/drm/i915/spi/intel_spi.c > create mode 100644 drivers/gpu/drm/i915/spi/intel_spi.h > create mode 100644 drivers/gpu/drm/i915/spi/intel_spi_drv.c Thanks, Miquèl
> > > Add driver for access to the discrete graphics card > > internal SPI device. > > Expose device on auxiliary bus and provide driver to register > > this device with MTD framework. > > Maybe you can explain why you think auxiliary bus is relevant here? The > cover letter might maybe be a bit more verbose to give us more context? > > I've looked at the series, it looks like you try to expose a spi > memory connected to a spi controller on your hardware. We usually > expect the spi controller driver to register in the spi core and > provide spi-mem operations for that. > > I don't know if this memory is supposed to be used as general purpose, > but if it's not I would advise to use some kind of firmware mechanism > instead. Also, what is the purpose of exposing this content in this > case? > > Well, I'm partially convinced here, I would like to hear from the other > maintainers, maybe your choices are legitimate and I'm off topic. > > Thanks, > Miquèl > The main purpose of creating this driver is to allow device manufacturing and recovery. In these flows the firmware may be unavailable and direct spi access is the only way. There is a use-case that require another kernel driver to access the spi partitions directly. This use-case is not upstreamed yet. The spi controller on discreet graphics card is not visible to user-space. Spi access flows are supported by another hardware module and relevant registers are available on graphics device memory bar. Auxiliary bus device is created to separate spi code and flows from already overloaded i915 driver.
On Tue, Sep 12, 2023 at 10:50:22AM +0000, Usyskin, Alexander wrote: > The spi controller on discreet graphics card is not visible to user-space. > Spi access flows are supported by another hardware module and relevant registers are > available on graphics device memory bar. No SPI controllers are directly visible to userspace, some SPI devices are selectively exposed but that needs to be explicitly requested and is generally discouraged.
> > > The spi controller on discreet graphics card is not visible to user-space. > > Spi access flows are supported by another hardware module and relevant > registers are > > available on graphics device memory bar. > > No SPI controllers are directly visible to userspace, some SPI devices > are selectively exposed but that needs to be explicitly requested and is > generally discouraged. What are the options here? Explicitly request exception is the one. Any other way to add access to flash memory connected in such way?
Hi, alexander.usyskin@intel.com wrote on Tue, 12 Sep 2023 13:15:58 +0000: > > > > > The spi controller on discreet graphics card is not visible to user-space. > > > Spi access flows are supported by another hardware module and relevant > > registers are > > > available on graphics device memory bar. > > > > No SPI controllers are directly visible to userspace, some SPI devices > > are selectively exposed but that needs to be explicitly requested and is > > generally discouraged. > > What are the options here? Explicitly request exception is the one. > Any other way to add access to flash memory connected in such way? Register a spi controller with at least spi-mem ops, as suggested previously, is the standard way I guess. If you're not willing to do so, it must be justified, I guess? Thanks, Miquèl
On Tue, Sep 12, 2023 at 03:21:02PM +0200, Miquel Raynal wrote: > alexander.usyskin@intel.com wrote on Tue, 12 Sep 2023 13:15:58 +0000: > > > No SPI controllers are directly visible to userspace, some SPI devices > > > are selectively exposed but that needs to be explicitly requested and is > > > generally discouraged. > > What are the options here? Explicitly request exception is the one. > > Any other way to add access to flash memory connected in such way? > Register a spi controller with at least spi-mem ops, as suggested > previously, is the standard way I guess. If you're not willing to do > so, it must be justified, I guess? Right, we already have a way of describing flash chips so that should be used to describe any flash chips.
> > > > No SPI controllers are directly visible to userspace, some SPI devices > > > > are selectively exposed but that needs to be explicitly requested and is > > > > generally discouraged. > > > > What are the options here? Explicitly request exception is the one. > > > Any other way to add access to flash memory connected in such way? > > > Register a spi controller with at least spi-mem ops, as suggested > > previously, is the standard way I guess. If you're not willing to do > > so, it must be justified, I guess? > > Right, we already have a way of describing flash chips so that should be > used to describe any flash chips. Hi I've tried to register spi controller with a spi-mem ops, but I can't find a way to connect to mtd subsystem. I took spi-intel as example, which connects to spi-nor but it relies on JDEC ID of flash to configure itself. We have predefined set of operations unrelated to flash behind the controller. What is the right way to simulate the general operations? Should I add dummy flash device? Or there is another option to connect spi-mem-only controller to mtd? Or this is too much and warrant the exception to have direct MTD device?
On Wed, Sep 20, 2023 at 01:52:07PM +0000, Usyskin, Alexander wrote: > I've tried to register spi controller with a spi-mem ops, but I can't find a way to connect to mtd subsystem. > I took spi-intel as example, which connects to spi-nor but it relies on JDEC ID of flash to configure itself. You should use the normal SPI device registration interfaces to register whatever devices are connected to the SPI controller. What in concrete terms have you tried to do here and in what way did it not work? > We have predefined set of operations unrelated to flash behind the controller. This sounds like there's some sort of MFD rather than or as well as a flash chip, or possibly multiple SPI devices?
> > On Wed, Sep 20, 2023 at 01:52:07PM +0000, Usyskin, Alexander wrote: > > > I've tried to register spi controller with a spi-mem ops, but I can't find a way > to connect to mtd subsystem. > > I took spi-intel as example, which connects to spi-nor but it relies on JDEC ID > of flash to configure itself. > > You should use the normal SPI device registration interfaces to register > whatever devices are connected to the SPI controller. What in concrete terms > have you tried to do here and in what way did it not work? > > > We have predefined set of operations unrelated to flash behind the > controller. > > This sounds like there's some sort of MFD rather than or as well as a flash > chip, or possibly multiple SPI devices? Yes, the driver doesn't talk to SPI controller directly it goes via another layer, so all SPI standard HW is not accessible, but we wish to expose the MTD interface. Thanks Tomas
On Wed, Sep 20, 2023 at 09:00:08PM +0000, Winkler, Tomas wrote: > > This sounds like there's some sort of MFD rather than or as well as a flash > > chip, or possibly multiple SPI devices? > Yes, the driver doesn't talk to SPI controller directly it goes via > another layer, so all SPI standard HW is not accessible, but we wish > to expose the MTD interface. Perhaps if you described clearly and directly the actual system you are trying to model then it would be easier to tell how it fits into the kernel? What is the actual interface here - how does the host interact with the flash? Also to repeat: please fix your mail client to word wrap within paragraphs at something substantially less than 80 columns. Doing this makes your messages much easier to read and reply to.
> > > > This sounds like there's some sort of MFD rather than or as well as a flash > > > chip, or possibly multiple SPI devices? > > > Yes, the driver doesn't talk to SPI controller directly it goes via > > another layer, so all SPI standard HW is not accessible, but we wish > > to expose the MTD interface. > > Perhaps if you described clearly and directly the actual system you are > trying to model then it would be easier to tell how it fits into the > kernel? What is the actual interface here - how does the host interact > with the flash? > > Also to repeat: please fix your mail client to word wrap within > paragraphs at something substantially less than 80 columns. Doing this > makes your messages much easier to read and reply to. Sorry for that, I'm fairly new in SPI and MTD subsystems. Will try to describe as best as I could. There is a Discreet Graphic device with embedded SPI (controller & flash). The embedded SPI is not visible to OS. There is another HW in the chip that gates access to the controller and exposes registers for: region select; read and write (4 and 8 bytes); erase (4K); error register; There are two main usages - user-space manufacturing and repair application that requires unrestricted read/write/erase over flash chip and kernel module that requires read access to one of flash regions for configuration. -- Alexander (Sasha) Usyskin CSE FW Dev - Host SW Intel Israel (74) Limited
On Wed, Sep 27, 2023 at 02:11:47PM +0000, Usyskin, Alexander wrote: > There is a Discreet Graphic device with embedded SPI (controller & flash). > The embedded SPI is not visible to OS. > There is another HW in the chip that gates access to the controller and > exposes registers for: > region select; read and write (4 and 8 bytes); erase (4K); error register; So assuming that's flash region select it sounds like this is a MTD controller and the fact that there's SPI isn't really relevant at all from a programming model point of view and it should probably be described as a MTD controller of some kind. Does that sound about right?
Hi Mark, broonie@kernel.org wrote on Wed, 27 Sep 2023 16:37:35 +0200: > On Wed, Sep 27, 2023 at 02:11:47PM +0000, Usyskin, Alexander wrote: > > > There is a Discreet Graphic device with embedded SPI (controller & flash). > > The embedded SPI is not visible to OS. > > There is another HW in the chip that gates access to the controller and > > exposes registers for: > > region select; read and write (4 and 8 bytes); erase (4K); error register; > > So assuming that's flash region select it sounds like this is a MTD > controller and the fact that there's SPI isn't really relevant at all > from a programming model point of view and it should probably be > described as a MTD controller of some kind. Does that sound about > right? Yeah in this case it seems the best option if the OS only has access to a very small subset of what the spi controller can do. Thanks, Miquèl
> > > > > There is a Discreet Graphic device with embedded SPI (controller & flash). > > > The embedded SPI is not visible to OS. > > > There is another HW in the chip that gates access to the controller and > > > exposes registers for: > > > region select; read and write (4 and 8 bytes); erase (4K); error register; > > > > So assuming that's flash region select it sounds like this is a MTD > > controller and the fact that there's SPI isn't really relevant at all > > from a programming model point of view and it should probably be > > described as a MTD controller of some kind. Does that sound about > > right? > > Yeah in this case it seems the best option if the OS only has access to > a very small subset of what the spi controller can do. > > Thanks, > Miquèl So, the approach of patch series that started the whole thread is right in general? Is the series submitted to the right mailing lists to review? If so, can you please review the series and evaluate it readiness to be merged?
Add driver for access to the discrete graphics card internal SPI device. Expose device on auxiliary bus and provide driver to register this device with MTD framework. This series is intended to be upstreamed through drm tree. Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com> Alexander Usyskin (3): drm/i915/spi: align 64bit read and write drm/i915/spi: wake card on operations drm/i915/spi: add support for access mode Jani Nikula (1): drm/i915/spi: add spi device for discrete graphics Tomas Winkler (6): drm/i915/spi: add intel_spi_region map drm/i915/spi: add driver for on-die spi device drm/i915/spi: implement region enumeration drm/i915/spi: implement spi access functions drm/i915/spi: spi register with mtd drm/i915/spi: mtd: implement access handlers drivers/gpu/drm/i915/Kconfig | 1 + drivers/gpu/drm/i915/Makefile | 6 + drivers/gpu/drm/i915/i915_driver.c | 7 + drivers/gpu/drm/i915/i915_drv.h | 4 + drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/spi/intel_spi.c | 101 +++ drivers/gpu/drm/i915/spi/intel_spi.h | 33 + drivers/gpu/drm/i915/spi/intel_spi_drv.c | 865 +++++++++++++++++++++++ 8 files changed, 1018 insertions(+) create mode 100644 drivers/gpu/drm/i915/spi/intel_spi.c create mode 100644 drivers/gpu/drm/i915/spi/intel_spi.h create mode 100644 drivers/gpu/drm/i915/spi/intel_spi_drv.c