Re: [PATCH 00/10] drm/i915/spi: spi access for discrete graphics

Re: [PATCH 00/10] drm/i915/spi: spi access for discrete graphics

[Miquel Raynal]

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

RE: [PATCH 00/10] drm/i915/spi: spi access for discrete graphics

[Usyskin, Alexander]

> 
> > 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.

-- 
Thanks,
Sasha


> > 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

Re: [PATCH 00/10] drm/i915/spi: spi access for discrete graphics

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 444 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH 00/10] drm/i915/spi: spi access for discrete graphics

[Usyskin, Alexander]

> 
> > 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?

-- 
Thanks,
Sasha

Re: [PATCH 00/10] drm/i915/spi: spi access for discrete graphics

[Miquel Raynal]

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

Re: [PATCH 00/10] drm/i915/spi: spi access for discrete graphics

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 768 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH 00/10] drm/i915/spi: spi access for discrete graphics

[Usyskin, Alexander]

> > > > 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?

-- 
Thanks,
Sasha

Re: [PATCH 00/10] drm/i915/spi: spi access for discrete graphics

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 703 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH 00/10] drm/i915/spi: spi access for discrete graphics

[Winkler, Tomas]

> 
> 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

Re: [PATCH 00/10] drm/i915/spi: spi access for discrete graphics

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 783 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH 00/10] drm/i915/spi: spi access for discrete graphics

[Usyskin, Alexander]

> 
> > > 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

Re: [PATCH 00/10] drm/i915/spi: spi access for discrete graphics

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 655 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 00/10] drm/i915/spi: spi access for discrete graphics

[Miquel Raynal]

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

RE: [PATCH 00/10] drm/i915/spi: spi access for discrete graphics

[Usyskin, Alexander]

> >
> > > 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?

-- 
Thanks,
Sasha