| Message ID | 20230910123949.1251964-7-alexander.usyskin@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm/i915/spi: spi access for discrete graphics | expand |
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(&spi->mtd, parts, n); > + > + kfree(parts); > + > + return ret; > +} > + Thanks, Miquèl
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? > > > + spi->mtd.erasesize = SZ_4K; /* 4K bytes granularity */ > > + spi->mtd.size = spi->size; > > +
Hi Alexander, alexander.usyskin@intel.com wrote on Tue, 17 Oct 2023 11:54:41 +0000: > 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
> > > > + 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.
Hi Alexander, alexander.usyskin@intel.com wrote on Tue, 17 Oct 2023 14:20:32 +0000: > > > > > + 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
> > > > > > > + 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?
Hi Alexander, + Michael and Tudor Folks, any interesting thought about the below discussion? alexander.usyskin@intel.com wrote on Tue, 14 Nov 2023 08:47:34 +0000: > > > > > > > > > + 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
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?
Hi Alexander, alexander.usyskin@intel.com wrote on Wed, 14 Feb 2024 12:16:00 +0000: > 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
diff --git a/drivers/gpu/drm/i915/spi/intel_spi_drv.c b/drivers/gpu/drm/i915/spi/intel_spi_drv.c index e3b78128ba76..355f9ad71602 100644 --- a/drivers/gpu/drm/i915/spi/intel_spi_drv.c +++ b/drivers/gpu/drm/i915/spi/intel_spi_drv.c @@ -15,8 +15,13 @@ #include <linux/delay.h> #include "spi/intel_spi.h" +#include <linux/mtd/mtd.h> +#include <linux/mtd/partitions.h> + struct i915_spi { struct kref refcnt; + struct mtd_info mtd; + struct mutex lock; /* region access lock */ void __iomem *base; size_t size; unsigned int nregions; @@ -407,6 +412,29 @@ static int i915_spi_init(struct i915_spi *spi, struct device *device) return n; } +static int i915_spi_erase(struct mtd_info *mtd, struct erase_info *info) +{ + dev_err(&mtd->dev, "erasing %lld %lld\n", info->addr, info->len); + + return 0; +} + +static int i915_spi_read(struct mtd_info *mtd, loff_t from, size_t len, + size_t *retlen, u_char *buf) +{ + dev_err(&mtd->dev, "read %lld %zd\n", from, len); + + return 0; +} + +static int i915_spi_write(struct mtd_info *mtd, loff_t to, size_t len, + size_t *retlen, const u_char *buf) +{ + dev_err(&mtd->dev, "writing %lld %zd\n", to, len); + + return 0; +} + static void i915_spi_release(struct kref *kref) { struct i915_spi *spi = container_of(kref, struct i915_spi, refcnt); @@ -415,9 +443,90 @@ static void i915_spi_release(struct kref *kref) pr_debug("freeing spi memory\n"); for (i = 0; i < spi->nregions; i++) kfree(spi->regions[i].name); + mutex_destroy(&spi->lock); kfree(spi); } +static int i915_spi_get_device(struct mtd_info *mtd) +{ + struct mtd_info *master; + struct i915_spi *spi; + + if (!mtd) + return -ENODEV; + + master = mtd_get_master(mtd); + spi = master->priv; + if (WARN_ON(!spi)) + return -EINVAL; + pr_debug("get spi %s %d\n", mtd->name, kref_read(&spi->refcnt)); + kref_get(&spi->refcnt); + + return 0; +} + +static void i915_spi_put_device(struct mtd_info *mtd) +{ + struct mtd_info *master; + struct i915_spi *spi; + + if (!mtd) + return; + + master = mtd_get_master(mtd); + spi = master->priv; + if (WARN_ON(!spi)) + return; + pr_debug("put spi %s %d\n", mtd->name, kref_read(&spi->refcnt)); + kref_put(&spi->refcnt, i915_spi_release); +} + +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 */ + 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(&spi->mtd, parts, n); + + kfree(parts); + + return ret; +} + static int i915_spi_probe(struct auxiliary_device *aux_dev, const struct auxiliary_device_id *aux_dev_id) { @@ -449,6 +558,7 @@ static int i915_spi_probe(struct auxiliary_device *aux_dev, if (!spi) return -ENOMEM; + mutex_init(&spi->lock); kref_init(&spi->refcnt); spi->nregions = nregions; @@ -481,6 +591,12 @@ static int i915_spi_probe(struct auxiliary_device *aux_dev, goto err; } + ret = i915_spi_init_mtd(spi, device, ret); + if (ret) { + dev_err(device, "i915-spi failed init mtd %d\n", ret); + goto err; + } + dev_set_drvdata(&aux_dev->dev, spi); dev_dbg(device, "i915-spi is bound\n"); @@ -499,6 +615,8 @@ static void i915_spi_remove(struct auxiliary_device *aux_dev) if (!spi) return; + mtd_device_unregister(&spi->mtd); + dev_set_drvdata(&aux_dev->dev, NULL); kref_put(&spi->refcnt, i915_spi_release);