Re: [PATCH 06/10] drm/i915/spi: spi register with mtd
Hi Alexander, alexander.usys...@intel.com wrote on Wed, 14 Feb 2024 12:16:00 +: > Hi Miquel > > Intel Gfx in infinite wisdom decided to create another driver - Xe and > the spi driver part of this series should be moved to some common location. > Should it be drivers/mtd or drivers/spi, or some other place? It probably depends on the framework they decided to register into. I'm sorry but I interacted in this thread more than 3 months ago, I no longer remember most of the details. If this is a driver for a spi controller (even a limited one) then it should be drivers/spi/ I guess. Thanks, Miquèl
Re: [Intel-gfx] [PATCH 06/10] drm/i915/spi: spi register with mtd
Hi Alexander, + Michael and Tudor Folks, any interesting thought about the below discussion? alexander.usys...@intel.com wrote on Tue, 14 Nov 2023 08:47:34 +: > > > > > > > > > + spi->mtd.writesize = SZ_1; /* 1 byte granularity */ > > > > > > > > > > > > You say writesize should be aligned with 4 in your next patch? > > > > > > > > > > We support unaligned write by reading aligned 4bytes, > > > > > replacing changed bytes there and writing whole 4bytes back. > > > > > Is there any problem with this approach? > > > > > > > > Is there a reason to do that manually rather than letting the core > > > > handle the complexity? > > > > > > > I was not aware that core can do this. The core implements above logic > > > if I put SZ_4 here and caller try to write, say, one byte? > > > And sync multiple writers? > > > If so, I can remove manual work, I think, and make the patches smaller. > > > > I haven't checked in detail but I would expect this yes. Please have a > > round of tests and if it works, please simplify this part. > > > > Thanks, > > Miquèl > > When I put SZ_4 here the "mtd_debug info /dev/mtd0" prints "mtd.writesize = > 4", > but "mtd_debug write /dev/mtd0 128 1 c" passes one byte to > i915_spi_write (mtd._write callback). > I suppose that mtd subsystem do not support this. > Or I did something wrong? > Thanks, Miquèl
Re: [Intel-gfx] [PATCH 06/10] drm/i915/spi: spi register with mtd
Hi Alexander, alexander.usys...@intel.com wrote on Tue, 17 Oct 2023 14:20:32 +: > > > > > + spi->mtd.writesize = SZ_1; /* 1 byte granularity */ > > > > > > > > You say writesize should be aligned with 4 in your next patch? > > > > > > We support unaligned write by reading aligned 4bytes, > > > replacing changed bytes there and writing whole 4bytes back. > > > Is there any problem with this approach? > > > > Is there a reason to do that manually rather than letting the core > > handle the complexity? > > > I was not aware that core can do this. The core implements above logic > if I put SZ_4 here and caller try to write, say, one byte? > And sync multiple writers? > If so, I can remove manual work, I think, and make the patches smaller. I haven't checked in detail but I would expect this yes. Please have a round of tests and if it works, please simplify this part. Thanks, Miquèl
Re: [Intel-gfx] [PATCH 06/10] drm/i915/spi: spi register with mtd
Hi Alexander,
alexander.usys...@intel.com wrote on Tue, 17 Oct 2023 11:54:41 +:
> Hi Miquel,
>
> > > +static int i915_spi_init_mtd(struct i915_spi *spi, struct device *device,
> > > + unsigned int nparts)
> > > +{
> > > + unsigned int i;
> > > + unsigned int n;
> > > + struct mtd_partition *parts = NULL;
> > > + int ret;
> > > +
> > > + dev_dbg(device, "registering with mtd\n");
> > > +
> > > + spi->mtd.owner = THIS_MODULE;
> > > + spi->mtd.dev.parent = device;
> > > + spi->mtd.flags = MTD_CAP_NORFLASH | MTD_WRITEABLE;
> > > + spi->mtd.type = MTD_DATAFLASH;
> > > + spi->mtd.priv = spi;
> > > + spi->mtd._write = i915_spi_write;
> > > + spi->mtd._read = i915_spi_read;
> > > + spi->mtd._erase = i915_spi_erase;
> > > + spi->mtd._get_device = i915_spi_get_device;
> > > + spi->mtd._put_device = i915_spi_put_device;
> > > + spi->mtd.writesize = SZ_1; /* 1 byte granularity */
> >
> > You say writesize should be aligned with 4 in your next patch?
>
> We support unaligned write by reading aligned 4bytes,
> replacing changed bytes there and writing whole 4bytes back.
> Is there any problem with this approach?
Is there a reason to do that manually rather than letting the core
handle the complexity?
>
> >
> > > + spi->mtd.erasesize = SZ_4K; /* 4K bytes granularity */
> > > + spi->mtd.size = spi->size;
> > > +
>
Thanks,
Miquèl
Re: [Intel-gfx] [PATCH 06/10] drm/i915/spi: spi register with mtd
Hi Alexander,
> +static int i915_spi_init_mtd(struct i915_spi *spi, struct device *device,
> + unsigned int nparts)
> +{
> + unsigned int i;
> + unsigned int n;
> + struct mtd_partition *parts = NULL;
> + int ret;
> +
> + dev_dbg(device, "registering with mtd\n");
> +
> + spi->mtd.owner = THIS_MODULE;
> + spi->mtd.dev.parent = device;
> + spi->mtd.flags = MTD_CAP_NORFLASH | MTD_WRITEABLE;
> + spi->mtd.type = MTD_DATAFLASH;
> + spi->mtd.priv = spi;
> + spi->mtd._write = i915_spi_write;
> + spi->mtd._read = i915_spi_read;
> + spi->mtd._erase = i915_spi_erase;
> + spi->mtd._get_device = i915_spi_get_device;
> + spi->mtd._put_device = i915_spi_put_device;
> + spi->mtd.writesize = SZ_1; /* 1 byte granularity */
You say writesize should be aligned with 4 in your next patch?
> + spi->mtd.erasesize = SZ_4K; /* 4K bytes granularity */
> + spi->mtd.size = spi->size;
> +
> + parts = kcalloc(spi->nregions, sizeof(*parts), GFP_KERNEL);
> + if (!parts)
> + return -ENOMEM;
> +
> + for (i = 0, n = 0; i < spi->nregions && n < nparts; i++) {
> + if (!spi->regions[i].is_readable)
> + continue;
> + parts[n].name = spi->regions[i].name;
> + parts[n].offset = spi->regions[i].offset;
> + parts[n].size = spi->regions[i].size;
> + if (!spi->regions[i].is_writable)
> + parts[n].mask_flags = MTD_WRITEABLE;
> + n++;
> + }
> +
> + ret = mtd_device_register(>mtd, parts, n);
> +
> + kfree(parts);
> +
> + return ret;
> +}
> +
Thanks,
Miquèl
Re: [Intel-gfx] [PATCH 00/10] drm/i915/spi: spi access for discrete graphics
Hi Mark, broo...@kernel.org wrote on Wed, 27 Sep 2023 16:37:35 +0200: > On Wed, Sep 27, 2023 at 02:11:47PM +, 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: [Intel-gfx] [PATCH 00/10] drm/i915/spi: spi access for discrete graphics
Hi, alexander.usys...@intel.com wrote on Tue, 12 Sep 2023 13:15:58 +: > > > > > 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: [Intel-gfx] [PATCH 00/10] drm/i915/spi: spi access for discrete graphics
Hi Alexander, + Mark Brown + spi list + spi-nor maintainers alexander.usys...@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 (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: [Intel-gfx] [trivial PATCH] treewide: Convert switch/case fallthrough; to break;
Hi Joe, For MTD: > drivers/mtd/nand/raw/nandsim.c| 2 +- Reviewed-by: Miquel Raynal Thanks, Miquèl ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
