[Intel-gfx] [PATCH 00/10] drm/i915/spi: spi access for discrete graphics

[Intel-gfx] [PATCH 00/10] drm/i915/spi: spi access for discrete graphics

[Alexander Usyskin]

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

-- 
2.34.1

[Intel-gfx] [PATCH 01/10] drm/i915/spi: add spi device for discrete graphics

[Alexander Usyskin]

From: Jani Nikula <jani.nikula@intel.com>

Enable access to internal spi on DGFX devices via a child device.
The spi child device is exposed via auxiliary bus.

CC: Rodrigo Vivi <rodrigo.vivi@intel.com>
CC: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
---
 drivers/gpu/drm/i915/Makefile        |  3 ++
 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 | 68 ++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/spi/intel_spi.h | 26 +++++++++++
 6 files changed, 109 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/spi/intel_spi.c
 create mode 100644 drivers/gpu/drm/i915/spi/intel_spi.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 79f65eff6bb2..f16870ad2615 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -222,6 +222,9 @@ i915-y += \
 # graphics system controller (GSC) support
 i915-y += gt/intel_gsc.o
 
+# graphics spi device (DGFX) support
+i915-y += spi/intel_spi.o
+
 # graphics hardware monitoring (HWMON) support
 i915-$(CONFIG_HWMON) += i915_hwmon.o
 
diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index f8dbee7a5af7..aeeb34a8dde2 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -80,6 +80,8 @@
 #include "soc/intel_dram.h"
 #include "soc/intel_gmch.h"
 
+#include "spi/intel_spi.h"
+
 #include "i915_debugfs.h"
 #include "i915_driver.h"
 #include "i915_drm_client.h"
@@ -666,6 +668,8 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
 
 	i915_hwmon_unregister(dev_priv);
 
+	intel_spi_fini(&dev_priv->spi);
+
 	i915_perf_unregister(dev_priv);
 	i915_pmu_unregister(dev_priv);
 
@@ -1133,6 +1137,9 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
 
 	i915_gem_suspend_late(dev_priv);
 
+
+	intel_spi_init(&dev_priv->spi, dev_priv);
+
 	for_each_gt(gt, dev_priv, i)
 		intel_uncore_suspend(gt->uncore);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 87ffc477c3b1..abc601200cb4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -51,6 +51,8 @@
 
 #include "soc/intel_pch.h"
 
+#include "spi/intel_spi.h"
+
 #include "i915_drm_client.h"
 #include "i915_gem.h"
 #include "i915_gpu_error.h"
@@ -315,6 +317,8 @@ struct drm_i915_private {
 
 	struct i915_perf perf;
 
+	struct intel_spi spi;
+
 	struct i915_hwmon *hwmon;
 
 	/* Abstract the submission mechanism (legacy ringbuffer or execlists) away */
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index e00e4d569ba9..0f8b01495b77 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -930,6 +930,7 @@
 #define DG2_GSC_HECI2_BASE	0x00374000
 #define MTL_GSC_HECI1_BASE	0x00116000
 #define MTL_GSC_HECI2_BASE	0x00117000
+#define GEN12_GUNIT_SPI_BASE	0x00102040
 
 #define HECI_H_CSR(base)	_MMIO((base) + 0x4)
 #define   HECI_H_CSR_IE		REG_BIT(0)
diff --git a/drivers/gpu/drm/i915/spi/intel_spi.c b/drivers/gpu/drm/i915/spi/intel_spi.c
new file mode 100644
index 000000000000..9eb5ab6bc4b9
--- /dev/null
+++ b/drivers/gpu/drm/i915/spi/intel_spi.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright(c) 2019-2022, Intel Corporation. All rights reserved.
+ */
+
+#include <linux/irq.h>
+#include "i915_reg.h"
+#include "i915_drv.h"
+#include "spi/intel_spi.h"
+
+#define GEN12_GUNIT_SPI_SIZE 0x80
+
+static void i915_spi_release_dev(struct device *dev)
+{
+}
+
+void intel_spi_init(struct intel_spi *spi, struct drm_i915_private *dev_priv)
+{
+	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
+	struct auxiliary_device *aux_dev = &spi->aux_dev;
+	int ret;
+
+	/* Only the DGFX devices have internal SPI */
+	if (!IS_DGFX(dev_priv))
+		return;
+
+	spi->bar.parent = &pdev->resource[0];
+	spi->bar.start = GEN12_GUNIT_SPI_BASE + pdev->resource[0].start;
+	spi->bar.end = spi->bar.start + GEN12_GUNIT_SPI_SIZE - 1;
+	spi->bar.flags = IORESOURCE_MEM;
+	spi->bar.desc = IORES_DESC_NONE;
+
+	aux_dev->name = "spi";
+	aux_dev->id = (pci_domain_nr(pdev->bus) << 16) |
+		       PCI_DEVID(pdev->bus->number, pdev->devfn);
+	aux_dev->dev.parent = &pdev->dev;
+	aux_dev->dev.release = i915_spi_release_dev;
+
+	ret = auxiliary_device_init(aux_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "i915-spi aux init failed %d\n", ret);
+		return;
+	}
+
+	ret = auxiliary_device_add(aux_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "i915-spi aux add failed %d\n", ret);
+		auxiliary_device_uninit(aux_dev);
+		return;
+	}
+
+	spi->i915 = dev_priv;
+}
+
+void intel_spi_fini(struct intel_spi *spi)
+{
+	struct pci_dev *pdev;
+
+	if (!spi->i915)
+		return;
+
+	pdev = to_pci_dev(spi->i915->drm.dev);
+
+	dev_dbg(&pdev->dev, "removing i915-spi cell\n");
+
+	auxiliary_device_delete(&spi->aux_dev);
+	auxiliary_device_uninit(&spi->aux_dev);
+}
diff --git a/drivers/gpu/drm/i915/spi/intel_spi.h b/drivers/gpu/drm/i915/spi/intel_spi.h
new file mode 100644
index 000000000000..a58bf79dcbc9
--- /dev/null
+++ b/drivers/gpu/drm/i915/spi/intel_spi.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright(c) 2019-2022, Intel Corporation. All rights reserved.
+ */
+
+#ifndef __INTEL_SPI_DEV_H__
+#define __INTEL_SPI_DEV_H__
+
+#include <linux/auxiliary_bus.h>
+
+struct drm_i915_private;
+
+struct intel_spi {
+	struct auxiliary_device aux_dev;
+	struct drm_i915_private *i915;
+	struct resource bar;
+};
+
+#define auxiliary_dev_to_intel_spi_dev(auxiliary_dev) \
+	container_of(auxiliary_dev, struct intel_spi, aux_dev)
+
+void intel_spi_init(struct intel_spi *spi, struct drm_i915_private *i915);
+
+void intel_spi_fini(struct intel_spi *spi);
+
+#endif /* __INTEL_SPI_DEV_H__ */
-- 
2.34.1

[Intel-gfx] [PATCH 02/10] drm/i915/spi: add intel_spi_region map

[Alexander Usyskin]

From: Tomas Winkler <tomas.winkler@intel.com>

Add the dGFX spi region map and convey it via auxiliary device
to the spi child device.

CC: Rodrigo Vivi <rodrigo.vivi@intel.com>
CC: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/gpu/drm/i915/spi/intel_spi.c | 8 ++++++++
 drivers/gpu/drm/i915/spi/intel_spi.h | 6 ++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/spi/intel_spi.c b/drivers/gpu/drm/i915/spi/intel_spi.c
index 9eb5ab6bc4b9..c697ca226e34 100644
--- a/drivers/gpu/drm/i915/spi/intel_spi.c
+++ b/drivers/gpu/drm/i915/spi/intel_spi.c
@@ -10,6 +10,13 @@
 
 #define GEN12_GUNIT_SPI_SIZE 0x80
 
+static const struct i915_spi_region regions[I915_SPI_REGIONS] = {
+	[0] = { .name = "DESCRIPTOR", },
+	[2] = { .name = "GSC", },
+	[11] = { .name = "OptionROM", },
+	[12] = { .name = "DAM", },
+};
+
 static void i915_spi_release_dev(struct device *dev)
 {
 }
@@ -29,6 +36,7 @@ void intel_spi_init(struct intel_spi *spi, struct drm_i915_private *dev_priv)
 	spi->bar.end = spi->bar.start + GEN12_GUNIT_SPI_SIZE - 1;
 	spi->bar.flags = IORESOURCE_MEM;
 	spi->bar.desc = IORES_DESC_NONE;
+	spi->regions = regions;
 
 	aux_dev->name = "spi";
 	aux_dev->id = (pci_domain_nr(pdev->bus) << 16) |
diff --git a/drivers/gpu/drm/i915/spi/intel_spi.h b/drivers/gpu/drm/i915/spi/intel_spi.h
index a58bf79dcbc9..1ecf1a8581b4 100644
--- a/drivers/gpu/drm/i915/spi/intel_spi.h
+++ b/drivers/gpu/drm/i915/spi/intel_spi.h
@@ -10,10 +10,16 @@
 
 struct drm_i915_private;
 
+#define I915_SPI_REGIONS 13
+struct i915_spi_region {
+	const char *name;
+};
+
 struct intel_spi {
 	struct auxiliary_device aux_dev;
 	struct drm_i915_private *i915;
 	struct resource bar;
+	const struct i915_spi_region *regions;
 };
 
 #define auxiliary_dev_to_intel_spi_dev(auxiliary_dev) \
-- 
2.34.1

[Intel-gfx] [PATCH 03/10] drm/i915/spi: add driver for on-die spi device

[Alexander Usyskin]

From: Tomas Winkler <tomas.winkler@intel.com>

Add auxiliary driver for i915 on-die spi device.

CC: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
---
 drivers/gpu/drm/i915/Kconfig             |   1 +
 drivers/gpu/drm/i915/Makefile            |   3 +
 drivers/gpu/drm/i915/spi/intel_spi_drv.c | 141 +++++++++++++++++++++++
 3 files changed, 145 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/spi/intel_spi_drv.c

diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index ce397a8797f7..c13d25658d87 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -39,6 +39,7 @@ config DRM_I915
 	select DRM_TTM
 	select DRM_BUDDY
 	select AUXILIARY_BUS
+	select MTD
 	help
 	  Choose this option if you have a system that has "Intel Graphics
 	  Media Accelerator" or "HD Graphics" integrated graphics,
diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index f16870ad2615..544e39448c3c 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -393,6 +393,9 @@ ifdef CONFIG_DRM_I915_WERROR
     cmd_checkdoc = $(srctree)/scripts/kernel-doc -none -Werror $<
 endif
 
+obj-m += i915_spi.o
+i915_spi-y := spi/intel_spi_drv.o
+
 # header test
 
 # exclude some broken headers from the test coverage
diff --git a/drivers/gpu/drm/i915/spi/intel_spi_drv.c b/drivers/gpu/drm/i915/spi/intel_spi_drv.c
new file mode 100644
index 000000000000..15c77b4b38bb
--- /dev/null
+++ b/drivers/gpu/drm/i915/spi/intel_spi_drv.c
@@ -0,0 +1,141 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright(c) 2019-2022, Intel Corporation. All rights reserved.
+ */
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/string.h>
+#include <linux/ioport.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include "spi/intel_spi.h"
+
+struct i915_spi {
+	struct kref refcnt;
+	void __iomem *base;
+	size_t size;
+	unsigned int nregions;
+	struct {
+		const char *name;
+		u8 id;
+		u64 offset;
+		u64 size;
+	} regions[];
+};
+
+static void i915_spi_release(struct kref *kref)
+{
+	struct i915_spi *spi = container_of(kref, struct i915_spi, refcnt);
+	int i;
+
+	pr_debug("freeing spi memory\n");
+	for (i = 0; i < spi->nregions; i++)
+		kfree(spi->regions[i].name);
+	kfree(spi);
+}
+
+static int i915_spi_probe(struct auxiliary_device *aux_dev,
+			  const struct auxiliary_device_id *aux_dev_id)
+{
+	struct intel_spi *ispi = auxiliary_dev_to_intel_spi_dev(aux_dev);
+	struct device *device;
+	struct i915_spi *spi;
+	unsigned int nregions;
+	unsigned int i, n;
+	size_t size;
+	char *name;
+	size_t name_size;
+	int ret;
+
+	device = &aux_dev->dev;
+
+	/* count available regions */
+	for (nregions = 0, i = 0; i < I915_SPI_REGIONS; i++) {
+		if (ispi->regions[i].name)
+			nregions++;
+	}
+
+	if (!nregions) {
+		dev_err(device, "no regions defined\n");
+		return -ENODEV;
+	}
+
+	size = sizeof(*spi) + sizeof(spi->regions[0]) * nregions;
+	spi = kzalloc(size, GFP_KERNEL);
+	if (!spi)
+		return -ENOMEM;
+
+	kref_init(&spi->refcnt);
+
+	spi->nregions = nregions;
+	for (n = 0, i = 0; i < I915_SPI_REGIONS; i++) {
+		if (ispi->regions[i].name) {
+			name_size = strlen(dev_name(&aux_dev->dev)) +
+				    strlen(ispi->regions[i].name) + 2; /* for point */
+			name = kzalloc(name_size, GFP_KERNEL);
+			if (!name)
+				continue;
+			snprintf(name, name_size, "%s.%s",
+				 dev_name(&aux_dev->dev), ispi->regions[i].name);
+			spi->regions[n].name = name;
+			spi->regions[n].id = i;
+			n++;
+		}
+	}
+
+	spi->base = devm_ioremap_resource(device, &ispi->bar);
+	if (IS_ERR(spi->base)) {
+		dev_err(device, "mmio not mapped\n");
+		ret = PTR_ERR(spi->base);
+		goto err;
+	}
+
+	dev_set_drvdata(&aux_dev->dev, spi);
+
+	dev_dbg(device, "i915-spi is bound\n");
+
+	return 0;
+
+err:
+	kref_put(&spi->refcnt, i915_spi_release);
+	return ret;
+}
+
+static void i915_spi_remove(struct auxiliary_device *aux_dev)
+{
+	struct i915_spi *spi = dev_get_drvdata(&aux_dev->dev);
+
+	if (!spi)
+		return;
+
+	dev_set_drvdata(&aux_dev->dev, NULL);
+
+	kref_put(&spi->refcnt, i915_spi_release);
+}
+
+static const struct auxiliary_device_id i915_spi_id_table[] = {
+	{
+		.name = "i915.spi",
+	},
+	{
+		/* sentinel */
+	}
+};
+MODULE_DEVICE_TABLE(auxiliary, i915_spi_id_table);
+
+static struct auxiliary_driver i915_spi_driver = {
+	.probe  = i915_spi_probe,
+	.remove = i915_spi_remove,
+	.driver = {
+		/* auxiliary_driver_register() sets .name to be the modname */
+	},
+	.id_table = i915_spi_id_table
+};
+
+module_auxiliary_driver(i915_spi_driver);
+
+MODULE_ALIAS("auxiliary:i915.spi");
+MODULE_LICENSE("Dual MIT/GPL");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_DESCRIPTION("Intel DGFX SPI driver");
-- 
2.34.1

[Intel-gfx] [PATCH 04/10] drm/i915/spi: implement region enumeration

[Alexander Usyskin]

From: Tomas Winkler <tomas.winkler@intel.com>

In i915-spi, there is no access to the spi controller,
the information is extracted form the descriptor region.

CC: Rodrigo Vivi <rodrigo.vivi@intel.com>
CC: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
---
 drivers/gpu/drm/i915/spi/intel_spi_drv.c | 193 ++++++++++++++++++++++-
 1 file changed, 192 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/spi/intel_spi_drv.c b/drivers/gpu/drm/i915/spi/intel_spi_drv.c
index 15c77b4b38bb..f32ea05f4f64 100644
--- a/drivers/gpu/drm/i915/spi/intel_spi_drv.c
+++ b/drivers/gpu/drm/i915/spi/intel_spi_drv.c
@@ -2,11 +2,12 @@
 /*
  * Copyright(c) 2019-2022, Intel Corporation. All rights reserved.
  */
+
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
 #include <linux/string.h>
-#include <linux/ioport.h>
+#include <linux/io.h>
 #include <linux/device.h>
 #include <linux/slab.h>
 #include "spi/intel_spi.h"
@@ -16,14 +17,197 @@ struct i915_spi {
 	void __iomem *base;
 	size_t size;
 	unsigned int nregions;
+	u32 access_map;
 	struct {
 		const char *name;
 		u8 id;
 		u64 offset;
 		u64 size;
+		unsigned int is_readable:1;
+		unsigned int is_writable:1;
 	} regions[];
 };
 
+#define SPI_TRIGGER_REG       0x00000000
+#define SPI_VALSIG_REG        0x00000010
+#define SPI_ADDRESS_REG       0x00000040
+#define SPI_REGION_ID_REG     0x00000044
+/*
+ * [15:0]-Erase size = 0x0010 4K 0x0080 32K 0x0100 64K
+ * [23:16]-Reserved
+ * [31:24]-Erase SPI RegionID
+ */
+#define SPI_ERASE_REG         0x00000048
+#define SPI_ACCESS_ERROR_REG  0x00000070
+#define SPI_ADDRESS_ERROR_REG 0x00000074
+
+/* Flash Valid Signature */
+#define SPI_FLVALSIG          0x0FF0A55A
+
+#define SPI_MAP_ADDR_MASK     0x000000FF
+#define SPI_MAP_ADDR_SHIFT    0x00000004
+
+#define REGION_ID_DESCRIPTOR  0
+/* Flash Region Base Address */
+#define FRBA      0x40
+/* Flash Region __n - Flash Descriptor Record */
+#define FLREG(__n)  (FRBA + ((__n) * 4))
+/*  Flash Map 1 Register */
+#define FLMAP1_REG  0x18
+#define FLMSTR4_OFFSET 0x00C
+
+#define SPI_ACCESS_ERROR_PCIE_MASK 0x7
+
+static inline void spi_set_region_id(struct i915_spi *spi, u8 region)
+{
+	iowrite32((u32)region, spi->base + SPI_REGION_ID_REG);
+}
+
+static inline u32 spi_error(struct i915_spi *spi)
+{
+	u32 reg = ioread32(spi->base + SPI_ACCESS_ERROR_REG) &
+		  SPI_ACCESS_ERROR_PCIE_MASK;
+
+	/* reset error bits */
+	if (reg)
+		iowrite32(reg, spi->base + SPI_ACCESS_ERROR_REG);
+
+	return reg;
+}
+
+static inline u32 spi_read32(struct i915_spi *spi, u32 address)
+{
+	void __iomem *base = spi->base;
+
+	iowrite32(address, base + SPI_ADDRESS_REG);
+
+	return ioread32(base + SPI_TRIGGER_REG);
+}
+
+static int spi_get_access_map(struct i915_spi *spi)
+{
+	u32 flmap1;
+	u32 fmba;
+	u32 fmstr4;
+	u32 fmstr4_addr;
+
+	spi_set_region_id(spi, REGION_ID_DESCRIPTOR);
+
+	flmap1 = spi_read32(spi, FLMAP1_REG);
+	if (spi_error(spi))
+		return -EIO;
+	/* Get Flash Master Baser Address (FMBA) */
+	fmba = ((flmap1 & SPI_MAP_ADDR_MASK) << SPI_MAP_ADDR_SHIFT);
+	fmstr4_addr = fmba + FLMSTR4_OFFSET;
+
+	fmstr4 = spi_read32(spi, fmstr4_addr);
+	if (spi_error(spi))
+		return -EIO;
+
+	spi->access_map = fmstr4;
+	return 0;
+}
+
+static bool spi_region_readable(struct i915_spi *spi, u8 region)
+{
+	if (region < 12)
+		return spi->access_map & (1 << (region + 8)); /* [19:8] */
+	else
+		return spi->access_map & (1 << (region - 12)); /* [3:0] */
+}
+
+static bool spi_region_writeable(struct i915_spi *spi, u8 region)
+{
+	if (region < 12)
+		return spi->access_map & (1 << (region + 20)); /* [31:20] */
+	else
+		return spi->access_map & (1 << (region - 8)); /* [7:4] */
+}
+
+static int i915_spi_is_valid(struct i915_spi *spi)
+{
+	u32 is_valid;
+
+	spi_set_region_id(spi, REGION_ID_DESCRIPTOR);
+
+	is_valid = spi_read32(spi, SPI_VALSIG_REG);
+	if (spi_error(spi))
+		return -EIO;
+
+	if (is_valid != SPI_FLVALSIG)
+		return -ENODEV;
+
+	return 0;
+}
+
+static int i915_spi_init(struct i915_spi *spi, struct device *device)
+{
+	int ret;
+	unsigned int i, n;
+
+	/* clean error register, previous errors are ignored */
+	spi_error(spi);
+
+	ret = i915_spi_is_valid(spi);
+	if (ret) {
+		dev_err(device, "The SPI is not valid %d\n", ret);
+		return ret;
+	}
+
+	if (spi_get_access_map(spi))
+		return -EIO;
+
+	for (i = 0, n = 0; i < spi->nregions; i++) {
+		u32 address, base, limit, region;
+		u8 id = spi->regions[i].id;
+
+		address = FLREG(id);
+		region = spi_read32(spi, address);
+
+		base = (region & 0x0000FFFF) << 12;
+		limit = (((region & 0xFFFF0000) >> 16) << 12) | 0xFFF;
+
+		dev_dbg(device, "[%d] %s: region: 0x%08X base: 0x%08x limit: 0x%08x\n",
+			id, spi->regions[i].name, region, base, limit);
+
+		if (base >= limit || (i > 0 && limit == 0)) {
+			dev_dbg(device, "[%d] %s: disabled\n",
+				id, spi->regions[i].name);
+			spi->regions[i].is_readable = 0;
+			continue;
+		}
+
+		if (spi->size < limit)
+			spi->size = limit;
+
+		spi->regions[i].offset = base;
+		spi->regions[i].size = limit - base + 1;
+		/* No write access to descriptor; mask it out*/
+		spi->regions[i].is_writable = spi_region_writeable(spi, id);
+
+		spi->regions[i].is_readable = spi_region_readable(spi, id);
+		dev_dbg(device, "Registered, %s id=%d offset=%lld size=%lld rd=%d wr=%d\n",
+			spi->regions[i].name,
+			spi->regions[i].id,
+			spi->regions[i].offset,
+			spi->regions[i].size,
+			spi->regions[i].is_readable,
+			spi->regions[i].is_writable);
+
+		if (spi->regions[i].is_readable)
+			n++;
+	}
+
+	dev_dbg(device, "Registered %d regions\n", n);
+
+	/* Need to add 1 to the amount of memory
+	 * so it is reported as an even block
+	 */
+	spi->size += 1;
+
+	return n;
+}
+
 static void i915_spi_release(struct kref *kref)
 {
 	struct i915_spi *spi = container_of(kref, struct i915_spi, refcnt);
@@ -91,6 +275,13 @@ static int i915_spi_probe(struct auxiliary_device *aux_dev,
 		goto err;
 	}
 
+	ret = i915_spi_init(spi, device);
+	if (ret < 0) {
+		dev_err(device, "cannot initialize spi\n");
+		ret = -ENODEV;
+		goto err;
+	}
+
 	dev_set_drvdata(&aux_dev->dev, spi);
 
 	dev_dbg(device, "i915-spi is bound\n");
-- 
2.34.1

[Intel-gfx] [PATCH 05/10] drm/i915/spi: implement spi access functions

[Alexander Usyskin]

From: Tomas Winkler <tomas.winkler@intel.com>

Implement spi_read() spi_erase() spi_write() functions.

CC: Lucas De Marchi <lucas.demarchi@intel.com>
CC: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Vitaly Lubart <vitaly.lubart@intel.com>
---
 drivers/gpu/drm/i915/spi/intel_spi_drv.c | 199 +++++++++++++++++++++++
 1 file changed, 199 insertions(+)

diff --git a/drivers/gpu/drm/i915/spi/intel_spi_drv.c b/drivers/gpu/drm/i915/spi/intel_spi_drv.c
index f32ea05f4f64..e3b78128ba76 100644
--- a/drivers/gpu/drm/i915/spi/intel_spi_drv.c
+++ b/drivers/gpu/drm/i915/spi/intel_spi_drv.c
@@ -10,6 +10,9 @@
 #include <linux/io.h>
 #include <linux/device.h>
 #include <linux/slab.h>
+#include <linux/sizes.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
+#include <linux/delay.h>
 #include "spi/intel_spi.h"
 
 struct i915_spi {
@@ -84,6 +87,33 @@ static inline u32 spi_read32(struct i915_spi *spi, u32 address)
 	return ioread32(base + SPI_TRIGGER_REG);
 }
 
+static inline u64 spi_read64(struct i915_spi *spi, u32 address)
+{
+	void __iomem *base = spi->base;
+
+	iowrite32(address, base + SPI_ADDRESS_REG);
+
+	return readq(base + SPI_TRIGGER_REG);
+}
+
+static void spi_write32(struct i915_spi *spi, u32 address, u32 data)
+{
+	void __iomem *base = spi->base;
+
+	iowrite32(address, base + SPI_ADDRESS_REG);
+
+	iowrite32(data, base + SPI_TRIGGER_REG);
+}
+
+static void spi_write64(struct i915_spi *spi, u32 address, u64 data)
+{
+	void __iomem *base = spi->base;
+
+	iowrite32(address, base + SPI_ADDRESS_REG);
+
+	writeq(data, base + SPI_TRIGGER_REG);
+}
+
 static int spi_get_access_map(struct i915_spi *spi)
 {
 	u32 flmap1;
@@ -140,6 +170,175 @@ static int i915_spi_is_valid(struct i915_spi *spi)
 	return 0;
 }
 
+__maybe_unused
+static unsigned int spi_get_region(const struct i915_spi *spi, loff_t from)
+{
+	unsigned int i;
+
+	for (i = 0; i < spi->nregions; i++) {
+		if ((spi->regions[i].offset + spi->regions[i].size - 1) > from &&
+		    spi->regions[i].offset <= from &&
+		    spi->regions[i].size != 0)
+			break;
+	}
+
+	return i;
+}
+
+static ssize_t spi_rewrite_partial(struct i915_spi *spi, loff_t to,
+			       loff_t offset, size_t len, const u32 *newdata)
+{
+	u32 data = spi_read32(spi, to);
+
+	if (spi_error(spi))
+		return -EIO;
+
+	memcpy((u8 *)&data + offset, newdata, len);
+
+	spi_write32(spi, to, data);
+	if (spi_error(spi))
+		return -EIO;
+
+	return len;
+}
+
+__maybe_unused
+static ssize_t spi_write(struct i915_spi *spi, u8 region,
+			 loff_t to, size_t len, const unsigned char *buf)
+{
+	size_t i;
+	size_t len8;
+	size_t len4;
+	size_t to4;
+	size_t to_shift;
+	size_t len_s = len;
+	ssize_t ret;
+
+	spi_set_region_id(spi, region);
+
+	to4 = ALIGN_DOWN(to, sizeof(u32));
+	to_shift = min(sizeof(u32) - ((size_t)to - to4), len);
+	if (to - to4) {
+		ret = spi_rewrite_partial(spi, to4, to - to4, to_shift,
+					  (uint32_t *)&buf[0]);
+		if (ret < 0)
+			return ret;
+
+		buf += to_shift;
+		to += to_shift;
+		len_s -= to_shift;
+	}
+
+	len8 = ALIGN_DOWN(len_s, sizeof(u64));
+	for (i = 0; i < len8; i += sizeof(u64)) {
+		u64 data;
+
+		memcpy(&data, &buf[i], sizeof(u64));
+		spi_write64(spi, to + i, data);
+		if (spi_error(spi))
+			return -EIO;
+	}
+
+	len4 = len_s - len8;
+	if (len4 >= sizeof(u32)) {
+		u32 data;
+
+		memcpy(&data, &buf[i], sizeof(u32));
+		spi_write32(spi, to + i, data);
+		if (spi_error(spi))
+			return -EIO;
+		i += sizeof(u32);
+		len4 -= sizeof(u32);
+	}
+
+	if (len4 > 0) {
+		ret = spi_rewrite_partial(spi, to + i, 0, len4,
+					  (uint32_t *)&buf[i]);
+		if (ret < 0)
+			return ret;
+	}
+
+	return len;
+}
+
+__maybe_unused
+static ssize_t spi_read(struct i915_spi *spi, u8 region,
+			loff_t from, size_t len, unsigned char *buf)
+{
+	size_t i;
+	size_t len8;
+	size_t len4;
+	size_t from4;
+	size_t from_shift;
+	size_t len_s = len;
+
+	spi_set_region_id(spi, region);
+
+	from4 = ALIGN_DOWN(from, sizeof(u32));
+	from_shift = min(sizeof(u32) - ((size_t)from - from4), len);
+
+	if (from - from4) {
+		u32 data = spi_read32(spi, from4);
+
+		if (spi_error(spi))
+			return -EIO;
+		memcpy(&buf[0], (u8 *)&data + (from - from4), from_shift);
+		len_s -= from_shift;
+		buf += from_shift;
+		from += from_shift;
+	}
+
+	len8 = ALIGN_DOWN(len_s, sizeof(u64));
+	for (i = 0; i < len8; i += sizeof(u64)) {
+		u64 data = spi_read64(spi, from + i);
+
+		if (spi_error(spi))
+			return -EIO;
+
+		memcpy(&buf[i], &data, sizeof(data));
+	}
+
+	len4 = len_s - len8;
+	if (len4 >= sizeof(u32)) {
+		u32 data = spi_read32(spi, from + i);
+
+		if (spi_error(spi))
+			return -EIO;
+		memcpy(&buf[i], &data, sizeof(data));
+		i += sizeof(u32);
+		len4 -= sizeof(u32);
+	}
+
+	if (len4 > 0) {
+		u32 data = spi_read32(spi, from + i);
+
+		if (spi_error(spi))
+			return -EIO;
+		memcpy(&buf[i], &data, len4);
+	}
+
+	return len;
+}
+
+__maybe_unused
+static ssize_t
+spi_erase(struct i915_spi *spi, u8 region, loff_t from, u64 len, u64 *fail_addr)
+{
+	u64 i;
+	const u32 block = 0x10;
+	void __iomem *base = spi->base;
+
+	for (i = 0; i < len; i += SZ_4K) {
+		iowrite32(from + i, base + SPI_ADDRESS_REG);
+		iowrite32(region << 24 | block, base + SPI_ERASE_REG);
+		/* Since the writes are via sguint
+		 * we cannot do back to back erases.
+		 */
+		msleep(50);
+	}
+	return len;
+}
+
 static int i915_spi_init(struct i915_spi *spi, struct device *device)
 {
 	int ret;
-- 
2.34.1

[Intel-gfx] [PATCH 06/10] drm/i915/spi: spi register with mtd

[Alexander Usyskin]

From: Tomas Winkler <tomas.winkler@intel.com>

Register the on-die spi device with the mtd subsystem.
Refcount spi object on _get and _put mtd callbacks.

CC: Rodrigo Vivi <rodrigo.vivi@intel.com>
CC: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
---
 drivers/gpu/drm/i915/spi/intel_spi_drv.c | 118 +++++++++++++++++++++++
 1 file changed, 118 insertions(+)

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);
-- 
2.34.1

[Intel-gfx] [PATCH 07/10] drm/i915/spi: mtd: implement access handlers

[Alexander Usyskin]

From: Tomas Winkler <tomas.winkler@intel.com>

Implement mtd read, erase, and write handlers.
For erase operation address and size should be 4K aligned.
For write operation address and size has to be 4bytes aligned.

CC: Rodrigo Vivi <rodrigo.vivi@intel.com>
CC: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
Signed-off-by: Vitaly Lubart <vitaly.lubart@intel.com>
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
---
 drivers/gpu/drm/i915/spi/intel_spi_drv.c | 152 +++++++++++++++++++++--
 1 file changed, 144 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/spi/intel_spi_drv.c b/drivers/gpu/drm/i915/spi/intel_spi_drv.c
index 355f9ad71602..39369a0c64a0 100644
--- a/drivers/gpu/drm/i915/spi/intel_spi_drv.c
+++ b/drivers/gpu/drm/i915/spi/intel_spi_drv.c
@@ -175,7 +175,6 @@ static int i915_spi_is_valid(struct i915_spi *spi)
 	return 0;
 }
 
-__maybe_unused
 static unsigned int spi_get_region(const struct i915_spi *spi, loff_t from)
 {
 	unsigned int i;
@@ -207,7 +206,6 @@ static ssize_t spi_rewrite_partial(struct i915_spi *spi, loff_t to,
 	return len;
 }
 
-__maybe_unused
 static ssize_t spi_write(struct i915_spi *spi, u8 region,
 			 loff_t to, size_t len, const unsigned char *buf)
 {
@@ -266,7 +264,6 @@ static ssize_t spi_write(struct i915_spi *spi, u8 region,
 	return len;
 }
 
-__maybe_unused
 static ssize_t spi_read(struct i915_spi *spi, u8 region,
 			loff_t from, size_t len, unsigned char *buf)
 {
@@ -325,7 +322,6 @@ static ssize_t spi_read(struct i915_spi *spi, u8 region,
 	return len;
 }
 
-__maybe_unused
 static ssize_t
 spi_erase(struct i915_spi *spi, u8 region, loff_t from, u64 len, u64 *fail_addr)
 {
@@ -414,24 +410,164 @@ static int i915_spi_init(struct i915_spi *spi, struct device *device)
 
 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);
+	struct i915_spi *spi;
+	unsigned int idx;
+	u8 region;
+	u64 addr;
+	ssize_t bytes;
+	loff_t from;
+	size_t len;
+	size_t total_len;
+	int ret = 0;
+
+	if (!mtd || !info)
+		return -EINVAL;
 
-	return 0;
+	spi = mtd->priv;
+	if (WARN_ON(!spi))
+		return -EINVAL;
+
+	if (!IS_ALIGNED(info->addr, SZ_4K) || !IS_ALIGNED(info->len, SZ_4K)) {
+		dev_err(&mtd->dev, "unaligned erase %llx %llx\n",
+			info->addr, info->len);
+		info->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
+		return -EINVAL;
+	}
+
+	total_len = info->len;
+	addr = info->addr;
+
+	mutex_lock(&spi->lock);
+
+	while (total_len > 0) {
+		if (!IS_ALIGNED(addr, SZ_4K) || !IS_ALIGNED(total_len, SZ_4K)) {
+			dev_err(&mtd->dev, "unaligned erase %llx %zx\n", addr, total_len);
+			info->fail_addr = addr;
+			ret = -ERANGE;
+			goto out;
+		}
+
+		idx = spi_get_region(spi, addr);
+		if (idx >= spi->nregions) {
+			dev_err(&mtd->dev, "out of range");
+			info->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
+			ret = -ERANGE;
+			goto out;
+		}
+
+		from = addr - spi->regions[idx].offset;
+		region = spi->regions[idx].id;
+		len = total_len;
+		if (len > spi->regions[idx].size - from)
+			len = spi->regions[idx].size - from;
+
+		dev_dbg(&mtd->dev, "erasing region[%d] %s from %llx len %zx\n",
+			region, spi->regions[idx].name, from, len);
+
+		bytes = spi_erase(spi, region, from, len, &info->fail_addr);
+		if (bytes < 0) {
+			dev_dbg(&mtd->dev, "erase failed with %zd\n", bytes);
+			info->fail_addr += spi->regions[idx].offset;
+			ret = bytes;
+			goto out;
+		}
+
+		addr += len;
+		total_len -= len;
+	}
+
+out:
+	mutex_unlock(&spi->lock);
+	return ret;
 }
 
 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);
+	struct i915_spi *spi;
+	ssize_t ret;
+	unsigned int idx;
+	u8 region;
+
+	if (!mtd)
+		return -EINVAL;
+
+	spi = mtd->priv;
+	if (WARN_ON(!spi))
+		return -EINVAL;
+
+	idx = spi_get_region(spi, from);
 
+	dev_dbg(&mtd->dev, "reading region[%d] %s from %lld len %zd\n",
+		spi->regions[idx].id, spi->regions[idx].name, from, len);
+
+	if (idx >= spi->nregions) {
+		dev_err(&mtd->dev, "out of ragnge");
+		return -ERANGE;
+	}
+
+	from -= spi->regions[idx].offset;
+	region = spi->regions[idx].id;
+	if (len > spi->regions[idx].size - from)
+		len = spi->regions[idx].size - from;
+
+	mutex_lock(&spi->lock);
+
+	ret = spi_read(spi, region, from, len, buf);
+	if (ret < 0) {
+		dev_dbg(&mtd->dev, "read failed with %zd\n", ret);
+		mutex_unlock(&spi->lock);
+		return ret;
+	}
+
+	*retlen = ret;
+
+	mutex_unlock(&spi->lock);
 	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);
+	struct i915_spi *spi;
+	ssize_t ret;
+	unsigned int idx;
+	u8 region;
+
+	if (!mtd)
+		return -EINVAL;
+
+	spi = mtd->priv;
+	if (WARN_ON(!spi))
+		return -EINVAL;
+
+	idx = spi_get_region(spi, to);
+
+	dev_dbg(&mtd->dev, "writing region[%d] %s to %lld len %zd\n",
+		spi->regions[idx].id, spi->regions[idx].name, to, len);
+
+	if (idx >= spi->nregions) {
+		dev_err(&mtd->dev, "out of range");
+		return -ERANGE;
+	}
+
+	to -= spi->regions[idx].offset;
+	region = spi->regions[idx].id;
+	if (len > spi->regions[idx].size - to)
+		len = spi->regions[idx].size - to;
+
+	mutex_lock(&spi->lock);
+
+	ret = spi_write(spi, region, to, len, buf);
+	if (ret < 0) {
+		dev_dbg(&mtd->dev, "write failed with %zd\n", ret);
+		mutex_unlock(&spi->lock);
+		return ret;
+	}
+
+	*retlen = ret;
 
+	mutex_unlock(&spi->lock);
 	return 0;
 }
 
-- 
2.34.1

[Intel-gfx] [PATCH 08/10] drm/i915/spi: align 64bit read and write

[Alexander Usyskin]

GSC SPI HW errors on quad access overlapping 1K border.
Align 64bit read and write to avoid readq/writeq over 1K border.

Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
---
 drivers/gpu/drm/i915/spi/intel_spi_drv.c | 36 ++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/drivers/gpu/drm/i915/spi/intel_spi_drv.c b/drivers/gpu/drm/i915/spi/intel_spi_drv.c
index 39369a0c64a0..22b804ebadc0 100644
--- a/drivers/gpu/drm/i915/spi/intel_spi_drv.c
+++ b/drivers/gpu/drm/i915/spi/intel_spi_drv.c
@@ -14,6 +14,7 @@
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/delay.h>
 #include "spi/intel_spi.h"
+#include "i915_reg_defs.h"
 
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
@@ -232,6 +233,24 @@ static ssize_t spi_write(struct i915_spi *spi, u8 region,
 		len_s -= to_shift;
 	}
 
+	if (!IS_ALIGNED(to, sizeof(u64)) &&
+	    ((to ^ (to + len_s)) & REG_GENMASK(31, 10))) {
+		/*
+		 * Workaround reads/writes across 1k-aligned addresses
+		 * (start u32 before 1k, end u32 after)
+		 * as this fails on hardware.
+		 */
+		u32 data;
+
+		memcpy(&data, &buf[0], sizeof(u32));
+		spi_write32(spi, to, data);
+		if (spi_error(spi))
+			return -EIO;
+		buf += sizeof(u32);
+		to += sizeof(u32);
+		len_s -= sizeof(u32);
+	}
+
 	len8 = ALIGN_DOWN(len_s, sizeof(u64));
 	for (i = 0; i < len8; i += sizeof(u64)) {
 		u64 data;
@@ -290,6 +309,23 @@ static ssize_t spi_read(struct i915_spi *spi, u8 region,
 		from += from_shift;
 	}
 
+	if (!IS_ALIGNED(from, sizeof(u64)) &&
+	    ((from ^ (from + len_s)) & REG_GENMASK(31, 10))) {
+		/*
+		 * Workaround reads/writes across 1k-aligned addresses
+		 * (start u32 before 1k, end u32 after)
+		 * as this fails on hardware.
+		 */
+		u32 data = spi_read32(spi, from);
+
+		if (spi_error(spi))
+			return -EIO;
+		memcpy(&buf[0], &data, sizeof(data));
+		len_s -= sizeof(u32);
+		buf += sizeof(u32);
+		from += sizeof(u32);
+	}
+
 	len8 = ALIGN_DOWN(len_s, sizeof(u64));
 	for (i = 0; i < len8; i += sizeof(u64)) {
 		u64 data = spi_read64(spi, from + i);
-- 
2.34.1

[Intel-gfx] [PATCH 09/10] drm/i915/spi: wake card on operations

[Alexander Usyskin]

Enable runtime PM in spi driver to notify i915 that
whole card should be kept awake while spi operations are
performed through this driver.

CC: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
---
 drivers/gpu/drm/i915/spi/intel_spi_drv.c | 44 ++++++++++++++++++++++++
 1 file changed, 44 insertions(+)

diff --git a/drivers/gpu/drm/i915/spi/intel_spi_drv.c b/drivers/gpu/drm/i915/spi/intel_spi_drv.c
index 22b804ebadc0..6b514b137fd0 100644
--- a/drivers/gpu/drm/i915/spi/intel_spi_drv.c
+++ b/drivers/gpu/drm/i915/spi/intel_spi_drv.c
@@ -13,12 +13,15 @@
 #include <linux/sizes.h>
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/delay.h>
+#include <linux/pm_runtime.h>
 #include "spi/intel_spi.h"
 #include "i915_reg_defs.h"
 
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
 
+#define I915_SPI_RPM_TIMEOUT 500
+
 struct i915_spi {
 	struct kref refcnt;
 	struct mtd_info mtd;
@@ -473,6 +476,12 @@ static int i915_spi_erase(struct mtd_info *mtd, struct erase_info *info)
 	total_len = info->len;
 	addr = info->addr;
 
+	ret = pm_runtime_resume_and_get(mtd->dev.parent);
+	if (ret < 0) {
+		dev_err(&mtd->dev, "rpm: get failed %d\n", ret);
+		return ret;
+	}
+
 	mutex_lock(&spi->lock);
 
 	while (total_len > 0) {
@@ -514,6 +523,8 @@ static int i915_spi_erase(struct mtd_info *mtd, struct erase_info *info)
 
 out:
 	mutex_unlock(&spi->lock);
+	pm_runtime_mark_last_busy(mtd->dev.parent);
+	pm_runtime_put_autosuspend(mtd->dev.parent);
 	return ret;
 }
 
@@ -547,6 +558,12 @@ static int i915_spi_read(struct mtd_info *mtd, loff_t from, size_t len,
 	if (len > spi->regions[idx].size - from)
 		len = spi->regions[idx].size - from;
 
+	ret = pm_runtime_resume_and_get(mtd->dev.parent);
+	if (ret < 0) {
+		dev_err(&mtd->dev, "rpm: get failed %zd\n", ret);
+		return ret;
+	}
+
 	mutex_lock(&spi->lock);
 
 	ret = spi_read(spi, region, from, len, buf);
@@ -559,6 +576,8 @@ static int i915_spi_read(struct mtd_info *mtd, loff_t from, size_t len,
 	*retlen = ret;
 
 	mutex_unlock(&spi->lock);
+	pm_runtime_mark_last_busy(mtd->dev.parent);
+	pm_runtime_put_autosuspend(mtd->dev.parent);
 	return 0;
 }
 
@@ -592,6 +611,12 @@ static int i915_spi_write(struct mtd_info *mtd, loff_t to, size_t len,
 	if (len > spi->regions[idx].size - to)
 		len = spi->regions[idx].size - to;
 
+	ret = pm_runtime_resume_and_get(mtd->dev.parent);
+	if (ret < 0) {
+		dev_err(&mtd->dev, "rpm: get failed %zd\n", ret);
+		return ret;
+	}
+
 	mutex_lock(&spi->lock);
 
 	ret = spi_write(spi, region, to, len, buf);
@@ -604,6 +629,8 @@ static int i915_spi_write(struct mtd_info *mtd, loff_t to, size_t len,
 	*retlen = ret;
 
 	mutex_unlock(&spi->lock);
+	pm_runtime_mark_last_busy(mtd->dev.parent);
+	pm_runtime_put_autosuspend(mtd->dev.parent);
 	return 0;
 }
 
@@ -749,6 +776,17 @@ static int i915_spi_probe(struct auxiliary_device *aux_dev,
 		}
 	}
 
+	pm_runtime_enable(device);
+
+	pm_runtime_set_autosuspend_delay(device, I915_SPI_RPM_TIMEOUT);
+	pm_runtime_use_autosuspend(device);
+
+	ret = pm_runtime_resume_and_get(device);
+	if (ret < 0) {
+		dev_err(device, "rpm: get failed %d\n", ret);
+		goto err_norpm;
+	}
+
 	spi->base = devm_ioremap_resource(device, &ispi->bar);
 	if (IS_ERR(spi->base)) {
 		dev_err(device, "mmio not mapped\n");
@@ -773,9 +811,13 @@ static int i915_spi_probe(struct auxiliary_device *aux_dev,
 
 	dev_dbg(device, "i915-spi is bound\n");
 
+	pm_runtime_put(device);
 	return 0;
 
 err:
+	pm_runtime_put(device);
+err_norpm:
+	pm_runtime_disable(device);
 	kref_put(&spi->refcnt, i915_spi_release);
 	return ret;
 }
@@ -787,6 +829,8 @@ static void i915_spi_remove(struct auxiliary_device *aux_dev)
 	if (!spi)
 		return;
 
+	pm_runtime_disable(&aux_dev->dev);
+
 	mtd_device_unregister(&spi->mtd);
 
 	dev_set_drvdata(&aux_dev->dev, NULL);
-- 
2.34.1

[Intel-gfx] [PATCH 10/10] drm/i915/spi: add support for access mode

[Alexander Usyskin]

Check SPI access mode from GSC FW status registers
and overwrite access status read from SPI descriptor, if needed.

Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
---
 drivers/gpu/drm/i915/spi/intel_spi.c     | 25 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/spi/intel_spi.h     |  1 +
 drivers/gpu/drm/i915/spi/intel_spi_drv.c |  6 +++---
 3 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/spi/intel_spi.c b/drivers/gpu/drm/i915/spi/intel_spi.c
index c697ca226e34..aac01898169f 100644
--- a/drivers/gpu/drm/i915/spi/intel_spi.c
+++ b/drivers/gpu/drm/i915/spi/intel_spi.c
@@ -9,6 +9,7 @@
 #include "spi/intel_spi.h"
 
 #define GEN12_GUNIT_SPI_SIZE 0x80
+#define HECI_FW_STATUS_2_SPI_ACCESS_MODE BIT(3)
 
 static const struct i915_spi_region regions[I915_SPI_REGIONS] = {
 	[0] = { .name = "DESCRIPTOR", },
@@ -21,6 +22,29 @@ static void i915_spi_release_dev(struct device *dev)
 {
 }
 
+static bool i915_spi_writeable_override(struct drm_i915_private *dev_priv)
+{
+	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
+	resource_size_t base;
+	bool writeable_override;
+
+	if (IS_DG1(dev_priv)) {
+		base = DG1_GSC_HECI2_BASE;
+	} else if (IS_DG2(dev_priv)) {
+		base = DG2_GSC_HECI2_BASE;
+	} else {
+		dev_err(&pdev->dev, "Unknown platform\n");
+		return true;
+	}
+
+	writeable_override =
+		!(intel_uncore_read(&dev_priv->uncore, HECI_FWSTS(base, 2)) &
+		  HECI_FW_STATUS_2_SPI_ACCESS_MODE);
+	if (writeable_override)
+		dev_info(&pdev->dev, "SPI access overridden by jumper\n");
+	return writeable_override;
+}
+
 void intel_spi_init(struct intel_spi *spi, struct drm_i915_private *dev_priv)
 {
 	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
@@ -31,6 +55,7 @@ void intel_spi_init(struct intel_spi *spi, struct drm_i915_private *dev_priv)
 	if (!IS_DGFX(dev_priv))
 		return;
 
+	spi->writeable_override = i915_spi_writeable_override(dev_priv);
 	spi->bar.parent = &pdev->resource[0];
 	spi->bar.start = GEN12_GUNIT_SPI_BASE + pdev->resource[0].start;
 	spi->bar.end = spi->bar.start + GEN12_GUNIT_SPI_SIZE - 1;
diff --git a/drivers/gpu/drm/i915/spi/intel_spi.h b/drivers/gpu/drm/i915/spi/intel_spi.h
index 1ecf1a8581b4..83588fae8c5e 100644
--- a/drivers/gpu/drm/i915/spi/intel_spi.h
+++ b/drivers/gpu/drm/i915/spi/intel_spi.h
@@ -18,6 +18,7 @@ struct i915_spi_region {
 struct intel_spi {
 	struct auxiliary_device aux_dev;
 	struct drm_i915_private *i915;
+	bool writeable_override;
 	struct resource bar;
 	const struct i915_spi_region *regions;
 };
diff --git a/drivers/gpu/drm/i915/spi/intel_spi_drv.c b/drivers/gpu/drm/i915/spi/intel_spi_drv.c
index 6b514b137fd0..75b6939ea93d 100644
--- a/drivers/gpu/drm/i915/spi/intel_spi_drv.c
+++ b/drivers/gpu/drm/i915/spi/intel_spi_drv.c
@@ -681,7 +681,7 @@ static void i915_spi_put_device(struct mtd_info *mtd)
 }
 
 static int i915_spi_init_mtd(struct i915_spi *spi, struct device *device,
-			     unsigned int nparts)
+			     unsigned int nparts, bool writeable_override)
 {
 	unsigned int i;
 	unsigned int n;
@@ -714,7 +714,7 @@ static int i915_spi_init_mtd(struct i915_spi *spi, struct device *device,
 		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)
+		if (!spi->regions[i].is_writable && !writeable_override)
 			parts[n].mask_flags = MTD_WRITEABLE;
 		n++;
 	}
@@ -801,7 +801,7 @@ static int i915_spi_probe(struct auxiliary_device *aux_dev,
 		goto err;
 	}
 
-	ret = i915_spi_init_mtd(spi, device, ret);
+	ret = i915_spi_init_mtd(spi, device, ret, ispi->writeable_override);
 	if (ret) {
 		dev_err(device, "i915-spi failed init mtd %d\n", ret);
 		goto err;
-- 
2.34.1

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/spi: spi access for discrete graphics

[Patchwork]

== Series Details ==

Series: drm/i915/spi: spi access for discrete graphics
URL   : https://patchwork.freedesktop.org/series/123510/
State : warning

== Summary ==

Error: dim checkpatch failed
eb746b900d2f drm/i915/spi: add spi device for discrete graphics
Traceback (most recent call last):
  File "scripts/spdxcheck.py", line 6, in <module>
    from ply import lex, yacc
ModuleNotFoundError: No module named 'ply'
Traceback (most recent call last):
  File "scripts/spdxcheck.py", line 6, in <module>
    from ply import lex, yacc
ModuleNotFoundError: No module named 'ply'
-:55: CHECK:LINE_SPACING: Please don't use multiple blank lines
#55: FILE: drivers/gpu/drm/i915/i915_driver.c:1140:
 
+

-:96: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#96: 
new file mode 100644

total: 0 errors, 1 warnings, 1 checks, 151 lines checked
26fb76820474 drm/i915/spi: add intel_spi_region map
e0ad98aa386a drm/i915/spi: add driver for on-die spi device
Traceback (most recent call last):
  File "scripts/spdxcheck.py", line 6, in <module>
    from ply import lex, yacc
ModuleNotFoundError: No module named 'ply'
-:40: WARNING:FILE_PATH_CHANGES: added, moved or deleted file(s), does MAINTAINERS need updating?
#40: 
new file mode 100644

total: 0 errors, 1 warnings, 0 checks, 157 lines checked
5541c7485c3f drm/i915/spi: implement region enumeration
216eef562149 drm/i915/spi: implement spi access functions
-:82: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#82: FILE: drivers/gpu/drm/i915/spi/intel_spi_drv.c:189:
+static ssize_t spi_rewrite_partial(struct i915_spi *spi, loff_t to,
+			       loff_t offset, size_t len, const u32 *newdata)

total: 0 errors, 0 warnings, 1 checks, 217 lines checked
00bee55e853d drm/i915/spi: spi register with mtd
861dc64f2591 drm/i915/spi: mtd: implement access handlers
dac47dcd273d drm/i915/spi: align 64bit read and write
9cf0cd890835 drm/i915/spi: wake card on operations
c5fd84376d6e drm/i915/spi: add support for access mode

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for drm/i915/spi: spi access for discrete graphics

[Patchwork]

== Series Details ==

Series: drm/i915/spi: spi access for discrete graphics
URL   : https://patchwork.freedesktop.org/series/123510/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

Re: [Intel-gfx] [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: [Intel-gfx] [PATCH 01/10] drm/i915/spi: add spi device for discrete graphics

[Jani Nikula]

On Sun, 10 Sep 2023, Alexander Usyskin <alexander.usyskin@intel.com> wrote:
> From: Jani Nikula <jani.nikula@intel.com>

I'm almost certain I did not write this patch originally. The authorship
may have been changed accidentally along the way, but it's not mine.

BR,
Jani.


>
> Enable access to internal spi on DGFX devices via a child device.
> The spi child device is exposed via auxiliary bus.
>
> CC: Rodrigo Vivi <rodrigo.vivi@intel.com>
> CC: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile        |  3 ++
>  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 | 68 ++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/spi/intel_spi.h | 26 +++++++++++
>  6 files changed, 109 insertions(+)
>  create mode 100644 drivers/gpu/drm/i915/spi/intel_spi.c
>  create mode 100644 drivers/gpu/drm/i915/spi/intel_spi.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 79f65eff6bb2..f16870ad2615 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -222,6 +222,9 @@ i915-y += \
>  # graphics system controller (GSC) support
>  i915-y += gt/intel_gsc.o
>  
> +# graphics spi device (DGFX) support
> +i915-y += spi/intel_spi.o
> +
>  # graphics hardware monitoring (HWMON) support
>  i915-$(CONFIG_HWMON) += i915_hwmon.o
>  
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index f8dbee7a5af7..aeeb34a8dde2 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -80,6 +80,8 @@
>  #include "soc/intel_dram.h"
>  #include "soc/intel_gmch.h"
>  
> +#include "spi/intel_spi.h"
> +
>  #include "i915_debugfs.h"
>  #include "i915_driver.h"
>  #include "i915_drm_client.h"
> @@ -666,6 +668,8 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
>  
>  	i915_hwmon_unregister(dev_priv);
>  
> +	intel_spi_fini(&dev_priv->spi);
> +
>  	i915_perf_unregister(dev_priv);
>  	i915_pmu_unregister(dev_priv);
>  
> @@ -1133,6 +1137,9 @@ static int i915_drm_suspend_late(struct drm_device *dev, bool hibernation)
>  
>  	i915_gem_suspend_late(dev_priv);
>  
> +
> +	intel_spi_init(&dev_priv->spi, dev_priv);
> +
>  	for_each_gt(gt, dev_priv, i)
>  		intel_uncore_suspend(gt->uncore);
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 87ffc477c3b1..abc601200cb4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -51,6 +51,8 @@
>  
>  #include "soc/intel_pch.h"
>  
> +#include "spi/intel_spi.h"
> +
>  #include "i915_drm_client.h"
>  #include "i915_gem.h"
>  #include "i915_gpu_error.h"
> @@ -315,6 +317,8 @@ struct drm_i915_private {
>  
>  	struct i915_perf perf;
>  
> +	struct intel_spi spi;
> +
>  	struct i915_hwmon *hwmon;
>  
>  	/* Abstract the submission mechanism (legacy ringbuffer or execlists) away */
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index e00e4d569ba9..0f8b01495b77 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -930,6 +930,7 @@
>  #define DG2_GSC_HECI2_BASE	0x00374000
>  #define MTL_GSC_HECI1_BASE	0x00116000
>  #define MTL_GSC_HECI2_BASE	0x00117000
> +#define GEN12_GUNIT_SPI_BASE	0x00102040
>  
>  #define HECI_H_CSR(base)	_MMIO((base) + 0x4)
>  #define   HECI_H_CSR_IE		REG_BIT(0)
> diff --git a/drivers/gpu/drm/i915/spi/intel_spi.c b/drivers/gpu/drm/i915/spi/intel_spi.c
> new file mode 100644
> index 000000000000..9eb5ab6bc4b9
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/spi/intel_spi.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright(c) 2019-2022, Intel Corporation. All rights reserved.
> + */
> +
> +#include <linux/irq.h>
> +#include "i915_reg.h"
> +#include "i915_drv.h"
> +#include "spi/intel_spi.h"
> +
> +#define GEN12_GUNIT_SPI_SIZE 0x80
> +
> +static void i915_spi_release_dev(struct device *dev)
> +{
> +}
> +
> +void intel_spi_init(struct intel_spi *spi, struct drm_i915_private *dev_priv)
> +{
> +	struct pci_dev *pdev = to_pci_dev(dev_priv->drm.dev);
> +	struct auxiliary_device *aux_dev = &spi->aux_dev;
> +	int ret;
> +
> +	/* Only the DGFX devices have internal SPI */
> +	if (!IS_DGFX(dev_priv))
> +		return;
> +
> +	spi->bar.parent = &pdev->resource[0];
> +	spi->bar.start = GEN12_GUNIT_SPI_BASE + pdev->resource[0].start;
> +	spi->bar.end = spi->bar.start + GEN12_GUNIT_SPI_SIZE - 1;
> +	spi->bar.flags = IORESOURCE_MEM;
> +	spi->bar.desc = IORES_DESC_NONE;
> +
> +	aux_dev->name = "spi";
> +	aux_dev->id = (pci_domain_nr(pdev->bus) << 16) |
> +		       PCI_DEVID(pdev->bus->number, pdev->devfn);
> +	aux_dev->dev.parent = &pdev->dev;
> +	aux_dev->dev.release = i915_spi_release_dev;
> +
> +	ret = auxiliary_device_init(aux_dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "i915-spi aux init failed %d\n", ret);
> +		return;
> +	}
> +
> +	ret = auxiliary_device_add(aux_dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "i915-spi aux add failed %d\n", ret);
> +		auxiliary_device_uninit(aux_dev);
> +		return;
> +	}
> +
> +	spi->i915 = dev_priv;
> +}
> +
> +void intel_spi_fini(struct intel_spi *spi)
> +{
> +	struct pci_dev *pdev;
> +
> +	if (!spi->i915)
> +		return;
> +
> +	pdev = to_pci_dev(spi->i915->drm.dev);
> +
> +	dev_dbg(&pdev->dev, "removing i915-spi cell\n");
> +
> +	auxiliary_device_delete(&spi->aux_dev);
> +	auxiliary_device_uninit(&spi->aux_dev);
> +}
> diff --git a/drivers/gpu/drm/i915/spi/intel_spi.h b/drivers/gpu/drm/i915/spi/intel_spi.h
> new file mode 100644
> index 000000000000..a58bf79dcbc9
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/spi/intel_spi.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright(c) 2019-2022, Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef __INTEL_SPI_DEV_H__
> +#define __INTEL_SPI_DEV_H__
> +
> +#include <linux/auxiliary_bus.h>
> +
> +struct drm_i915_private;
> +
> +struct intel_spi {
> +	struct auxiliary_device aux_dev;
> +	struct drm_i915_private *i915;
> +	struct resource bar;
> +};
> +
> +#define auxiliary_dev_to_intel_spi_dev(auxiliary_dev) \
> +	container_of(auxiliary_dev, struct intel_spi, aux_dev)
> +
> +void intel_spi_init(struct intel_spi *spi, struct drm_i915_private *i915);
> +
> +void intel_spi_fini(struct intel_spi *spi);
> +
> +#endif /* __INTEL_SPI_DEV_H__ */

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCH 01/10] drm/i915/spi: add spi device for discrete graphics

[Usyskin, Alexander]

> On Sun, 10 Sep 2023, Alexander Usyskin <alexander.usyskin@intel.com>
> wrote:
> > From: Jani Nikula <jani.nikula@intel.com>
> 
> I'm almost certain I did not write this patch originally. The authorship
> may have been changed accidentally along the way, but it's not mine.
> 
> BR,
> Jani.
> 

Sorry for that, seems like some rebase flux in the internal tree.
Will move author to Tomas, as it seem his patch originally.

-- 
Thanks,
Sasha

Re: [Intel-gfx] [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: [Intel-gfx] [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: [Intel-gfx] [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: [Intel-gfx] [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: [Intel-gfx] [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: [Intel-gfx] [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: [Intel-gfx] [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: [Intel-gfx] [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: [Intel-gfx] [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: [Intel-gfx] [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: [Intel-gfx] [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: [Intel-gfx] [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: [Intel-gfx] [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

Re: [Intel-gfx] [PATCH 06/10] drm/i915/spi: spi register with mtd

[Miquel Raynal]

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

Re: [Intel-gfx] [PATCH 06/10] drm/i915/spi: spi register with mtd

[Usyskin, Alexander]

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

-- 
Thanks,
Sasha

Re: [Intel-gfx] [PATCH 06/10] drm/i915/spi: spi register with mtd

[Miquel Raynal]

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

Re: [Intel-gfx] [PATCH 06/10] drm/i915/spi: spi register with mtd

[Usyskin, Alexander]

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

-- 
Thanks,
Sasha

Re: [Intel-gfx] [PATCH 06/10] drm/i915/spi: spi register with mtd

[Miquel Raynal]

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

Re: [Intel-gfx] [PATCH 06/10] drm/i915/spi: spi register with mtd

[Usyskin, Alexander]

> 
> > > > > > +	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,
Sasha

Re: [Intel-gfx] [PATCH 06/10] drm/i915/spi: spi register with mtd

[Miquel Raynal]

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

RE: [PATCH 06/10] drm/i915/spi: spi register with mtd

[Usyskin, Alexander]

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?

-- 
Thanks,
Sasha



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

Re: [PATCH 06/10] drm/i915/spi: spi register with mtd

[Miquel Raynal]

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