[PATCH 00/10] mtd: add driver for Intel discrete graphics

[PATCH 00/10] mtd: add driver for Intel discrete graphics

[Alexander Usyskin]

Add driver for access to Intel discrete graphics card
internal NVM device.
Expose device on auxiliary bus by i915 and Xe drivers and
provide mtd driver to register this device with MTD framework.

This is a rewrite of "drm/i915/spi: spi access for discrete graphics"
and "spi: add driver for Intel discrete graphics"
series with connection to the Xe driver and splitting
the spi driver part to separate module in mtd subsystem.

This series intended to be pushed through drm-xe-next.

Alexander Usyskin (10):
  mtd: add driver for intel graphics non-volatile memory device
  mtd: intel-dg: implement region enumeration
  mtd: intel-dg: implement access functions
  mtd: intel-dg: register with mtd
  mtd: intel-dg: align 64bit read and write
  mtd: intel-dg: wake card on operations
  drm/i915/nvm: add nvm device for discrete graphics
  drm/i915/nvm: add support for access mode
  drm/xe/nvm: add on-die non-volatile memory device
  drm/xe/nvm: add support for access mode

 MAINTAINERS                           |   7 +
 drivers/gpu/drm/i915/Makefile         |   4 +
 drivers/gpu/drm/i915/i915_driver.c    |   6 +
 drivers/gpu/drm/i915/i915_drv.h       |   3 +
 drivers/gpu/drm/i915/i915_reg.h       |   1 +
 drivers/gpu/drm/i915/intel_nvm.c      | 119 ++++
 drivers/gpu/drm/i915/intel_nvm.h      |  15 +
 drivers/gpu/drm/xe/Makefile           |   1 +
 drivers/gpu/drm/xe/regs/xe_gsc_regs.h |   4 +
 drivers/gpu/drm/xe/xe_device.c        |   3 +
 drivers/gpu/drm/xe/xe_device_types.h  |   8 +
 drivers/gpu/drm/xe/xe_heci_gsc.c      |   5 +-
 drivers/gpu/drm/xe/xe_nvm.c           | 131 ++++
 drivers/gpu/drm/xe/xe_nvm.h           |  15 +
 drivers/gpu/drm/xe/xe_pci.c           |   5 +
 drivers/mtd/devices/Kconfig           |  11 +
 drivers/mtd/devices/Makefile          |   1 +
 drivers/mtd/devices/mtd-intel-dg.c    | 843 ++++++++++++++++++++++++++
 include/linux/intel_dg_nvm_aux.h      |  27 +
 19 files changed, 1205 insertions(+), 4 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_nvm.c
 create mode 100644 drivers/gpu/drm/i915/intel_nvm.h
 create mode 100644 drivers/gpu/drm/xe/xe_nvm.c
 create mode 100644 drivers/gpu/drm/xe/xe_nvm.h
 create mode 100644 drivers/mtd/devices/mtd-intel-dg.c
 create mode 100644 include/linux/intel_dg_nvm_aux.h

-- 
2.43.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 01/10] mtd: add driver for intel graphics non-volatile memory device

[Alexander Usyskin]

Add auxiliary driver for intel discrete graphics
non-volatile memory device.

CC: Rodrigo Vivi <rodrigo.vivi@intel.com>
CC: Lucas De Marchi <lucas.demarchi@intel.com>
Co-developed-by: Tomas Winkler <tomasw@gmail.com>
Signed-off-by: Tomas Winkler <tomasw@gmail.com>
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
---
 MAINTAINERS                        |   7 ++
 drivers/mtd/devices/Kconfig        |  11 +++
 drivers/mtd/devices/Makefile       |   1 +
 drivers/mtd/devices/mtd-intel-dg.c | 139 +++++++++++++++++++++++++++++
 include/linux/intel_dg_nvm_aux.h   |  27 ++++++
 5 files changed, 185 insertions(+)
 create mode 100644 drivers/mtd/devices/mtd-intel-dg.c
 create mode 100644 include/linux/intel_dg_nvm_aux.h

diff --git a/MAINTAINERS b/MAINTAINERS
index c27f3190737f..a09c035849ef 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11360,6 +11360,13 @@ L:	linux-kernel@vger.kernel.org
 S:	Supported
 F:	arch/x86/include/asm/intel-family.h
 
+INTEL DISCRETE GRAPHIC NVM MTD DRIVER
+M:	Alexander Usyskin <alexander.usyskin@intel.com>
+L:	linux-mtd@lists.infradead.org
+S:	Supported
+F:	drivers/mtd/devices/mtd-intel-dg.c
+F:	include/linux/intel_dg_nvm_aux.h
+
 INTEL DRM DISPLAY FOR XE AND I915 DRIVERS
 M:	Jani Nikula <jani.nikula@linux.intel.com>
 M:	Rodrigo Vivi <rodrigo.vivi@intel.com>
diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
index ff2f9e55ef28..d93edf45c0bb 100644
--- a/drivers/mtd/devices/Kconfig
+++ b/drivers/mtd/devices/Kconfig
@@ -183,6 +183,17 @@ config MTD_POWERNV_FLASH
 	  platforms from Linux. This device abstracts away the
 	  firmware interface for flash access.
 
+config MTD_INTEL_DG
+	tristate "Intel Discrete Graphic non-volatile memory driver"
+	depends on AUXILIARY_BUS
+	depends on MTD
+	help
+	  This provides MTD device to access Intel Discrete Graphic
+	  non-volatile memory.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called mtd-intel-dg.
+
 comment "Disk-On-Chip Device Drivers"
 
 config MTD_DOCG3
diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile
index d11eb2b8b6f8..77c05d269034 100644
--- a/drivers/mtd/devices/Makefile
+++ b/drivers/mtd/devices/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_MTD_SST25L)	+= sst25l.o
 obj-$(CONFIG_MTD_BCM47XXSFLASH)	+= bcm47xxsflash.o
 obj-$(CONFIG_MTD_ST_SPI_FSM)    += st_spi_fsm.o
 obj-$(CONFIG_MTD_POWERNV_FLASH)	+= powernv_flash.o
+obj-$(CONFIG_MTD_INTEL_DG)	+= mtd-intel-dg.o
 
 
 CFLAGS_docg3.o			+= -I$(src)
diff --git a/drivers/mtd/devices/mtd-intel-dg.c b/drivers/mtd/devices/mtd-intel-dg.c
new file mode 100644
index 000000000000..746c963ea540
--- /dev/null
+++ b/drivers/mtd/devices/mtd-intel-dg.c
@@ -0,0 +1,139 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright(c) 2019-2024, Intel Corporation. All rights reserved.
+ */
+
+#include <linux/device.h>
+#include <linux/intel_dg_nvm_aux.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/string.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+struct intel_dg_nvm {
+	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 intel_dg_nvm_release(struct kref *kref)
+{
+	struct intel_dg_nvm *nvm = container_of(kref, struct intel_dg_nvm, refcnt);
+	int i;
+
+	pr_debug("freeing intel_dg nvm\n");
+	for (i = 0; i < nvm->nregions; i++)
+		kfree(nvm->regions[i].name);
+	kfree(nvm);
+}
+
+static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
+			      const struct auxiliary_device_id *aux_dev_id)
+{
+	struct intel_dg_nvm_dev *invm = auxiliary_dev_to_intel_dg_nvm_dev(aux_dev);
+	struct device *device;
+	struct intel_dg_nvm *nvm;
+	unsigned int nregions;
+	unsigned int i, n;
+	size_t size;
+	char *name;
+	int ret;
+
+	device = &aux_dev->dev;
+
+	/* count available regions */
+	for (nregions = 0, i = 0; i < INTEL_DG_NVM_REGIONS; i++) {
+		if (invm->regions[i].name)
+			nregions++;
+	}
+
+	if (!nregions) {
+		dev_err(device, "no regions defined\n");
+		return -ENODEV;
+	}
+
+	size = sizeof(*nvm) + sizeof(nvm->regions[0]) * nregions;
+	nvm = kzalloc(size, GFP_KERNEL);
+	if (!nvm)
+		return -ENOMEM;
+
+	kref_init(&nvm->refcnt);
+
+	nvm->nregions = nregions;
+	for (n = 0, i = 0; i < INTEL_DG_NVM_REGIONS; i++) {
+		if (!invm->regions[i].name)
+			continue;
+
+		name = kasprintf(GFP_KERNEL, "%s.%s",
+				 dev_name(&aux_dev->dev), invm->regions[i].name);
+		if (!name)
+			continue;
+		nvm->regions[n].name = name;
+		nvm->regions[n].id = i;
+		n++;
+	}
+
+	nvm->base = devm_ioremap_resource(device, &invm->bar);
+	if (IS_ERR(nvm->base)) {
+		dev_err(device, "mmio not mapped\n");
+		ret = PTR_ERR(nvm->base);
+		goto err;
+	}
+
+	dev_set_drvdata(&aux_dev->dev, nvm);
+
+	return 0;
+
+err:
+	kref_put(&nvm->refcnt, intel_dg_nvm_release);
+	return ret;
+}
+
+static void intel_dg_mtd_remove(struct auxiliary_device *aux_dev)
+{
+	struct intel_dg_nvm *nvm = dev_get_drvdata(&aux_dev->dev);
+
+	if (!nvm)
+		return;
+
+	dev_set_drvdata(&aux_dev->dev, NULL);
+
+	kref_put(&nvm->refcnt, intel_dg_nvm_release);
+}
+
+static const struct auxiliary_device_id intel_dg_mtd_id_table[] = {
+	{
+		.name = "i915.nvm",
+	},
+	{
+		.name = "xe.nvm",
+	},
+	{
+		/* sentinel */
+	}
+};
+MODULE_DEVICE_TABLE(auxiliary, intel_dg_mtd_id_table);
+
+static struct auxiliary_driver intel_dg_mtd_driver = {
+	.probe  = intel_dg_mtd_probe,
+	.remove = intel_dg_mtd_remove,
+	.driver = {
+		/* auxiliary_driver_register() sets .name to be the modname */
+	},
+	.id_table = intel_dg_mtd_id_table
+};
+
+module_auxiliary_driver(intel_dg_mtd_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_DESCRIPTION("Intel DGFX MTD driver");
diff --git a/include/linux/intel_dg_nvm_aux.h b/include/linux/intel_dg_nvm_aux.h
new file mode 100644
index 000000000000..2cc4179fbde2
--- /dev/null
+++ b/include/linux/intel_dg_nvm_aux.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright(c) 2019-2024, Intel Corporation. All rights reserved.
+ */
+
+#ifndef __INTEL_DG_NVM_AUX_H__
+#define __INTEL_DG_NVM_AUX_H__
+
+#include <linux/auxiliary_bus.h>
+
+#define INTEL_DG_NVM_REGIONS 13
+
+struct intel_dg_nvm_region {
+	const char *name;
+};
+
+struct intel_dg_nvm_dev {
+	struct auxiliary_device aux_dev;
+	bool writeable_override;
+	struct resource bar;
+	const struct intel_dg_nvm_region *regions;
+};
+
+#define auxiliary_dev_to_intel_dg_nvm_dev(auxiliary_dev) \
+	container_of(auxiliary_dev, struct intel_dg_nvm_dev, aux_dev)
+
+#endif /* __INTEL_DG_NVM_AUX_H__ */
-- 
2.43.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 02/10] mtd: intel-dg: implement region enumeration

[Alexander Usyskin]

In intel-dg, there is no access to the spi controller,
the information is extracted from the descriptor region.

CC: Rodrigo Vivi <rodrigo.vivi@intel.com>
CC: Lucas De Marchi <lucas.demarchi@intel.com>
Co-developed-by: Tomas Winkler <tomasw@gmail.com>
Signed-off-by: Tomas Winkler <tomasw@gmail.com>
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
---
 drivers/mtd/devices/mtd-intel-dg.c | 199 +++++++++++++++++++++++++++++
 1 file changed, 199 insertions(+)

diff --git a/drivers/mtd/devices/mtd-intel-dg.c b/drivers/mtd/devices/mtd-intel-dg.c
index 746c963ea540..05e333771be0 100644
--- a/drivers/mtd/devices/mtd-intel-dg.c
+++ b/drivers/mtd/devices/mtd-intel-dg.c
@@ -3,6 +3,8 @@
  * Copyright(c) 2019-2024, Intel Corporation. All rights reserved.
  */
 
+#include <linux/bitfield.h>
+#include <linux/bits.h>
 #include <linux/device.h>
 #include <linux/intel_dg_nvm_aux.h>
 #include <linux/io.h>
@@ -22,9 +24,199 @@ struct intel_dg_nvm {
 		u8 id;
 		u64 offset;
 		u64 size;
+		unsigned int is_readable:1;
+		unsigned int is_writable:1;
 	} regions[];
 };
 
+#define NVM_TRIGGER_REG       0x00000000
+#define NVM_VALSIG_REG        0x00000010
+#define NVM_ADDRESS_REG       0x00000040
+#define NVM_REGION_ID_REG     0x00000044
+/*
+ * [15:0]-Erase size = 0x0010 4K 0x0080 32K 0x0100 64K
+ * [23:16]-Reserved
+ * [31:24]-Erase MEM RegionID
+ */
+#define NVM_ERASE_REG         0x00000048
+#define NVM_ACCESS_ERROR_REG  0x00000070
+#define NVM_ADDRESS_ERROR_REG 0x00000074
+
+/* Flash Valid Signature */
+#define NVM_FLVALSIG          0x0FF0A55A
+
+#define NVM_MAP_ADDR_MASK     GENMASK(7, 0)
+#define NVM_MAP_ADDR_SHIFT    0x00000004
+
+#define NVM_REGION_ID_DESCRIPTOR  0
+/* Flash Region Base Address */
+#define NVM_FRBA      0x40
+/* Flash Region __n - Flash Descriptor Record */
+#define NVM_FLREG(__n) (NVM_FRBA + ((__n) * 4))
+/*  Flash Map 1 Register */
+#define NVM_FLMAP1_REG  0x18
+#define NVM_FLMSTR4_OFFSET 0x00C
+
+#define NVM_ACCESS_ERROR_PCIE_MASK 0x7
+
+#define NVM_FREG_BASE_MASK GENMASK(15, 0)
+#define NVM_FREG_ADDR_MASK GENMASK(31, 16)
+#define NVM_FREG_ADDR_SHIFT 12
+#define NVM_FREG_MIN_REGION_SIZE 0xFFF
+
+static inline void idg_nvm_set_region_id(struct intel_dg_nvm *nvm, u8 region)
+{
+	iowrite32((u32)region, nvm->base + NVM_REGION_ID_REG);
+}
+
+static inline u32 idg_nvm_error(struct intel_dg_nvm *nvm)
+{
+	void __iomem *base = nvm->base;
+
+	u32 reg = ioread32(base + NVM_ACCESS_ERROR_REG) & NVM_ACCESS_ERROR_PCIE_MASK;
+
+	/* reset error bits */
+	if (reg)
+		iowrite32(reg, base + NVM_ACCESS_ERROR_REG);
+
+	return reg;
+}
+
+static inline u32 idg_nvm_read32(struct intel_dg_nvm *nvm, u32 address)
+{
+	void __iomem *base = nvm->base;
+
+	iowrite32(address, base + NVM_ADDRESS_REG);
+
+	return ioread32(base + NVM_TRIGGER_REG);
+}
+
+static int idg_nvm_get_access_map(struct intel_dg_nvm *nvm, u32 *access_map)
+{
+	u32 flmap1;
+	u32 fmba;
+	u32 fmstr4;
+	u32 fmstr4_addr;
+
+	idg_nvm_set_region_id(nvm, NVM_REGION_ID_DESCRIPTOR);
+
+	flmap1 = idg_nvm_read32(nvm, NVM_FLMAP1_REG);
+	if (idg_nvm_error(nvm))
+		return -EIO;
+	/* Get Flash Master Baser Address (FMBA) */
+	fmba = (FIELD_GET(NVM_MAP_ADDR_MASK, flmap1) << NVM_MAP_ADDR_SHIFT);
+	fmstr4_addr = fmba + NVM_FLMSTR4_OFFSET;
+
+	fmstr4 = idg_nvm_read32(nvm, fmstr4_addr);
+	if (idg_nvm_error(nvm))
+		return -EIO;
+
+	*access_map = fmstr4;
+	return 0;
+}
+
+static bool idg_nvm_region_readable(u32 access_map, u8 region)
+{
+	if (region < 12)
+		return access_map & BIT(region + 8); /* [19:8] */
+	else
+		return access_map & BIT(region - 12); /* [3:0] */
+}
+
+static bool idg_nvm_region_writeable(u32 access_map, u8 region)
+{
+	if (region < 12)
+		return access_map & BIT(region + 20); /* [31:20] */
+	else
+		return access_map & BIT(region - 8); /* [7:4] */
+}
+
+static int idg_nvm_is_valid(struct intel_dg_nvm *nvm)
+{
+	u32 is_valid;
+
+	idg_nvm_set_region_id(nvm, NVM_REGION_ID_DESCRIPTOR);
+
+	is_valid = idg_nvm_read32(nvm, NVM_VALSIG_REG);
+	if (idg_nvm_error(nvm))
+		return -EIO;
+
+	if (is_valid != NVM_FLVALSIG)
+		return -ENODEV;
+
+	return 0;
+}
+
+static int intel_dg_nvm_init(struct intel_dg_nvm *nvm, struct device *device)
+{
+	int ret;
+	unsigned int i, n;
+	u32 access_map = 0;
+
+	/* clean error register, previous errors are ignored */
+	idg_nvm_error(nvm);
+
+	ret = idg_nvm_is_valid(nvm);
+	if (ret) {
+		dev_err(device, "The MEM is not valid %d\n", ret);
+		return ret;
+	}
+
+	if (idg_nvm_get_access_map(nvm, &access_map))
+		return -EIO;
+
+	for (i = 0, n = 0; i < nvm->nregions; i++) {
+		u32 address, base, limit, region;
+		u8 id = nvm->regions[i].id;
+
+		address = NVM_FLREG(id);
+		region = idg_nvm_read32(nvm, address);
+
+		base = FIELD_GET(NVM_FREG_BASE_MASK, region) << NVM_FREG_ADDR_SHIFT;
+		limit = (FIELD_GET(NVM_FREG_ADDR_MASK, region) << NVM_FREG_ADDR_SHIFT) |
+			NVM_FREG_MIN_REGION_SIZE;
+
+		dev_dbg(device, "[%d] %s: region: 0x%08X base: 0x%08x limit: 0x%08x\n",
+			id, nvm->regions[i].name, region, base, limit);
+
+		if (base >= limit || (i > 0 && limit == 0)) {
+			dev_dbg(device, "[%d] %s: disabled\n",
+				id, nvm->regions[i].name);
+			nvm->regions[i].is_readable = 0;
+			continue;
+		}
+
+		if (nvm->size < limit)
+			nvm->size = limit;
+
+		nvm->regions[i].offset = base;
+		nvm->regions[i].size = limit - base + 1;
+		/* No write access to descriptor; mask it out*/
+		nvm->regions[i].is_writable = idg_nvm_region_writeable(access_map, id);
+
+		nvm->regions[i].is_readable = idg_nvm_region_readable(access_map, id);
+		dev_dbg(device, "Registered, %s id=%d offset=%lld size=%lld rd=%d wr=%d\n",
+			nvm->regions[i].name,
+			nvm->regions[i].id,
+			nvm->regions[i].offset,
+			nvm->regions[i].size,
+			nvm->regions[i].is_readable,
+			nvm->regions[i].is_writable);
+
+		if (nvm->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
+	 */
+	nvm->size += 1;
+
+	return n;
+}
+
 static void intel_dg_nvm_release(struct kref *kref)
 {
 	struct intel_dg_nvm *nvm = container_of(kref, struct intel_dg_nvm, refcnt);
@@ -89,6 +281,13 @@ static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
 		goto err;
 	}
 
+	ret = intel_dg_nvm_init(nvm, device);
+	if (ret < 0) {
+		dev_err(device, "cannot initialize nvm\n");
+		ret = -ENODEV;
+		goto err;
+	}
+
 	dev_set_drvdata(&aux_dev->dev, nvm);
 
 	return 0;
-- 
2.43.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 03/10] mtd: intel-dg: implement access functions

[Alexander Usyskin]

Implement read(), erase() and write() functions.

CC: Lucas De Marchi <lucas.demarchi@intel.com>
CC: Rodrigo Vivi <rodrigo.vivi@intel.com>
Co-developed-by: Tomas Winkler <tomasw@gmail.com>
Signed-off-by: Tomas Winkler <tomasw@gmail.com>
Co-developed-by: Vitaly Lubart <lubvital@gmail.com>
Signed-off-by: Vitaly Lubart <lubvital@gmail.com>
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
---
 drivers/mtd/devices/mtd-intel-dg.c | 199 +++++++++++++++++++++++++++++
 1 file changed, 199 insertions(+)

diff --git a/drivers/mtd/devices/mtd-intel-dg.c b/drivers/mtd/devices/mtd-intel-dg.c
index 05e333771be0..8d21947bcc23 100644
--- a/drivers/mtd/devices/mtd-intel-dg.c
+++ b/drivers/mtd/devices/mtd-intel-dg.c
@@ -5,13 +5,16 @@
 
 #include <linux/bitfield.h>
 #include <linux/bits.h>
+#include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/intel_dg_nvm_aux.h>
 #include <linux/io.h>
+#include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/string.h>
 #include <linux/slab.h>
+#include <linux/sizes.h>
 #include <linux/types.h>
 
 struct intel_dg_nvm {
@@ -91,6 +94,33 @@ static inline u32 idg_nvm_read32(struct intel_dg_nvm *nvm, u32 address)
 	return ioread32(base + NVM_TRIGGER_REG);
 }
 
+static inline u64 idg_nvm_read64(struct intel_dg_nvm *nvm, u32 address)
+{
+	void __iomem *base = nvm->base;
+
+	iowrite32(address, base + NVM_ADDRESS_REG);
+
+	return readq(base + NVM_TRIGGER_REG);
+}
+
+static void idg_nvm_write32(struct intel_dg_nvm *nvm, u32 address, u32 data)
+{
+	void __iomem *base = nvm->base;
+
+	iowrite32(address, base + NVM_ADDRESS_REG);
+
+	iowrite32(data, base + NVM_TRIGGER_REG);
+}
+
+static void idg_nvm_write64(struct intel_dg_nvm *nvm, u32 address, u64 data)
+{
+	void __iomem *base = nvm->base;
+
+	iowrite32(address, base + NVM_ADDRESS_REG);
+
+	writeq(data, base + NVM_TRIGGER_REG);
+}
+
 static int idg_nvm_get_access_map(struct intel_dg_nvm *nvm, u32 *access_map)
 {
 	u32 flmap1;
@@ -147,6 +177,175 @@ static int idg_nvm_is_valid(struct intel_dg_nvm *nvm)
 	return 0;
 }
 
+__maybe_unused
+static unsigned int idg_nvm_get_region(const struct intel_dg_nvm *nvm, loff_t from)
+{
+	unsigned int i;
+
+	for (i = 0; i < nvm->nregions; i++) {
+		if ((nvm->regions[i].offset + nvm->regions[i].size - 1) > from &&
+		    nvm->regions[i].offset <= from &&
+		    nvm->regions[i].size != 0)
+			break;
+	}
+
+	return i;
+}
+
+static ssize_t idg_nvm_rewrite_partial(struct intel_dg_nvm *nvm, loff_t to,
+				       loff_t offset, size_t len, const u32 *newdata)
+{
+	u32 data = idg_nvm_read32(nvm, to);
+
+	if (idg_nvm_error(nvm))
+		return -EIO;
+
+	memcpy((u8 *)&data + offset, newdata, len);
+
+	idg_nvm_write32(nvm, to, data);
+	if (idg_nvm_error(nvm))
+		return -EIO;
+
+	return len;
+}
+
+__maybe_unused
+static ssize_t idg_write(struct intel_dg_nvm *nvm, 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;
+
+	idg_nvm_set_region_id(nvm, region);
+
+	to4 = ALIGN_DOWN(to, sizeof(u32));
+	to_shift = min(sizeof(u32) - ((size_t)to - to4), len);
+	if (to - to4) {
+		ret = idg_nvm_rewrite_partial(nvm, 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));
+		idg_nvm_write64(nvm, to + i, data);
+		if (idg_nvm_error(nvm))
+			return -EIO;
+	}
+
+	len4 = len_s - len8;
+	if (len4 >= sizeof(u32)) {
+		u32 data;
+
+		memcpy(&data, &buf[i], sizeof(u32));
+		idg_nvm_write32(nvm, to + i, data);
+		if (idg_nvm_error(nvm))
+			return -EIO;
+		i += sizeof(u32);
+		len4 -= sizeof(u32);
+	}
+
+	if (len4 > 0) {
+		ret = idg_nvm_rewrite_partial(nvm, to + i, 0, len4,
+					  (uint32_t *)&buf[i]);
+		if (ret < 0)
+			return ret;
+	}
+
+	return len;
+}
+
+__maybe_unused
+static ssize_t idg_read(struct intel_dg_nvm *nvm, 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;
+
+	idg_nvm_set_region_id(nvm, region);
+
+	from4 = ALIGN_DOWN(from, sizeof(u32));
+	from_shift = min(sizeof(u32) - ((size_t)from - from4), len);
+
+	if (from - from4) {
+		u32 data = idg_nvm_read32(nvm, from4);
+
+		if (idg_nvm_error(nvm))
+			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 = idg_nvm_read64(nvm, from + i);
+
+		if (idg_nvm_error(nvm))
+			return -EIO;
+
+		memcpy(&buf[i], &data, sizeof(data));
+	}
+
+	len4 = len_s - len8;
+	if (len4 >= sizeof(u32)) {
+		u32 data = idg_nvm_read32(nvm, from + i);
+
+		if (idg_nvm_error(nvm))
+			return -EIO;
+		memcpy(&buf[i], &data, sizeof(data));
+		i += sizeof(u32);
+		len4 -= sizeof(u32);
+	}
+
+	if (len4 > 0) {
+		u32 data = idg_nvm_read32(nvm, from + i);
+
+		if (idg_nvm_error(nvm))
+			return -EIO;
+		memcpy(&buf[i], &data, len4);
+	}
+
+	return len;
+}
+
+__maybe_unused
+static ssize_t
+idg_erase(struct intel_dg_nvm *nvm, u8 region, loff_t from, u64 len, u64 *fail_addr)
+{
+	u64 i;
+	const u32 block = 0x10;
+	void __iomem *base = nvm->base;
+
+	for (i = 0; i < len; i += SZ_4K) {
+		iowrite32(from + i, base + NVM_ADDRESS_REG);
+		iowrite32(region << 24 | block, base + NVM_ERASE_REG);
+		/* Since the writes are via sguint
+		 * we cannot do back to back erases.
+		 */
+		msleep(50);
+	}
+	return len;
+}
+
 static int intel_dg_nvm_init(struct intel_dg_nvm *nvm, struct device *device)
 {
 	int ret;
-- 
2.43.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 04/10] mtd: intel-dg: register with mtd

[Alexander Usyskin]

Register the on-die nvm device with the mtd subsystem.
Refcount nvm object on _get and _put mtd callbacks.
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>
Co-developed-by: Tomas Winkler <tomasw@gmail.com>
Signed-off-by: Tomas Winkler <tomasw@gmail.com>
Co-developed-by: Vitaly Lubart <lubvital@gmail.com>
Signed-off-by: Vitaly Lubart <lubvital@gmail.com>
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
---
 drivers/mtd/devices/mtd-intel-dg.c | 230 ++++++++++++++++++++++++++++-
 1 file changed, 226 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/devices/mtd-intel-dg.c b/drivers/mtd/devices/mtd-intel-dg.c
index 8d21947bcc23..2efcb4d64ebf 100644
--- a/drivers/mtd/devices/mtd-intel-dg.c
+++ b/drivers/mtd/devices/mtd-intel-dg.c
@@ -5,6 +5,7 @@
 
 #include <linux/bitfield.h>
 #include <linux/bits.h>
+#include <linux/cleanup.h>
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/intel_dg_nvm_aux.h>
@@ -12,6 +13,8 @@
 #include <linux/io-64-nonatomic-lo-hi.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
 #include <linux/string.h>
 #include <linux/slab.h>
 #include <linux/sizes.h>
@@ -19,6 +22,8 @@
 
 struct intel_dg_nvm {
 	struct kref refcnt;
+	struct mtd_info mtd;
+	struct mutex lock; /* region access lock */
 	void __iomem *base;
 	size_t size;
 	unsigned int nregions;
@@ -177,7 +182,6 @@ static int idg_nvm_is_valid(struct intel_dg_nvm *nvm)
 	return 0;
 }
 
-__maybe_unused
 static unsigned int idg_nvm_get_region(const struct intel_dg_nvm *nvm, loff_t from)
 {
 	unsigned int i;
@@ -209,7 +213,6 @@ static ssize_t idg_nvm_rewrite_partial(struct intel_dg_nvm *nvm, loff_t to,
 	return len;
 }
 
-__maybe_unused
 static ssize_t idg_write(struct intel_dg_nvm *nvm, u8 region,
 			 loff_t to, size_t len, const unsigned char *buf)
 {
@@ -268,7 +271,6 @@ static ssize_t idg_write(struct intel_dg_nvm *nvm, u8 region,
 	return len;
 }
 
-__maybe_unused
 static ssize_t idg_read(struct intel_dg_nvm *nvm, u8 region,
 			loff_t from, size_t len, unsigned char *buf)
 {
@@ -327,7 +329,6 @@ static ssize_t idg_read(struct intel_dg_nvm *nvm, u8 region,
 	return len;
 }
 
-__maybe_unused
 static ssize_t
 idg_erase(struct intel_dg_nvm *nvm, u8 region, loff_t from, u64 len, u64 *fail_addr)
 {
@@ -416,6 +417,147 @@ static int intel_dg_nvm_init(struct intel_dg_nvm *nvm, struct device *device)
 	return n;
 }
 
+static int intel_dg_mtd_erase(struct mtd_info *mtd, struct erase_info *info)
+{
+	struct intel_dg_nvm *nvm = mtd->priv;
+	unsigned int idx;
+	u8 region;
+	u64 addr;
+	ssize_t bytes;
+	loff_t from;
+	size_t len;
+	size_t total_len;
+
+	if (WARN_ON(!nvm))
+		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;
+
+	guard(mutex)(&nvm->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;
+			return -ERANGE;
+		}
+
+		idx = idg_nvm_get_region(nvm, addr);
+		if (idx >= nvm->nregions) {
+			dev_err(&mtd->dev, "out of range");
+			info->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
+			return -ERANGE;
+		}
+
+		from = addr - nvm->regions[idx].offset;
+		region = nvm->regions[idx].id;
+		len = total_len;
+		if (len > nvm->regions[idx].size - from)
+			len = nvm->regions[idx].size - from;
+
+		dev_dbg(&mtd->dev, "erasing region[%d] %s from %llx len %zx\n",
+			region, nvm->regions[idx].name, from, len);
+
+		bytes = idg_erase(nvm, region, from, len, &info->fail_addr);
+		if (bytes < 0) {
+			dev_dbg(&mtd->dev, "erase failed with %zd\n", bytes);
+			info->fail_addr += nvm->regions[idx].offset;
+			return bytes;
+		}
+
+		addr += len;
+		total_len -= len;
+	}
+
+	return 0;
+}
+
+static int intel_dg_mtd_read(struct mtd_info *mtd, loff_t from, size_t len,
+			     size_t *retlen, u_char *buf)
+{
+	struct intel_dg_nvm *nvm = mtd->priv;
+	ssize_t ret;
+	unsigned int idx;
+	u8 region;
+
+	if (WARN_ON(!nvm))
+		return -EINVAL;
+
+	idx = idg_nvm_get_region(nvm, from);
+
+	dev_dbg(&mtd->dev, "reading region[%d] %s from %lld len %zd\n",
+		nvm->regions[idx].id, nvm->regions[idx].name, from, len);
+
+	if (idx >= nvm->nregions) {
+		dev_err(&mtd->dev, "out of ragnge");
+		return -ERANGE;
+	}
+
+	from -= nvm->regions[idx].offset;
+	region = nvm->regions[idx].id;
+	if (len > nvm->regions[idx].size - from)
+		len = nvm->regions[idx].size - from;
+
+	guard(mutex)(&nvm->lock);
+
+	ret = idg_read(nvm, region, from, len, buf);
+	if (ret < 0) {
+		dev_dbg(&mtd->dev, "read failed with %zd\n", ret);
+		return ret;
+	}
+
+	*retlen = ret;
+
+	return 0;
+}
+
+static int intel_dg_mtd_write(struct mtd_info *mtd, loff_t to, size_t len,
+			      size_t *retlen, const u_char *buf)
+{
+	struct intel_dg_nvm *nvm = mtd->priv;
+	ssize_t ret;
+	unsigned int idx;
+	u8 region;
+
+	if (WARN_ON(!nvm))
+		return -EINVAL;
+
+	idx = idg_nvm_get_region(nvm, to);
+
+	dev_dbg(&mtd->dev, "writing region[%d] %s to %lld len %zd\n",
+		nvm->regions[idx].id, nvm->regions[idx].name, to, len);
+
+	if (idx >= nvm->nregions) {
+		dev_err(&mtd->dev, "out of range");
+		return -ERANGE;
+	}
+
+	to -= nvm->regions[idx].offset;
+	region = nvm->regions[idx].id;
+	if (len > nvm->regions[idx].size - to)
+		len = nvm->regions[idx].size - to;
+
+	guard(mutex)(&nvm->lock);
+
+	ret = idg_write(nvm, region, to, len, buf);
+	if (ret < 0) {
+		dev_dbg(&mtd->dev, "write failed with %zd\n", ret);
+		return ret;
+	}
+
+	*retlen = ret;
+
+	return 0;
+}
+
 static void intel_dg_nvm_release(struct kref *kref)
 {
 	struct intel_dg_nvm *nvm = container_of(kref, struct intel_dg_nvm, refcnt);
@@ -424,9 +566,80 @@ static void intel_dg_nvm_release(struct kref *kref)
 	pr_debug("freeing intel_dg nvm\n");
 	for (i = 0; i < nvm->nregions; i++)
 		kfree(nvm->regions[i].name);
+	mutex_destroy(&nvm->lock);
 	kfree(nvm);
 }
 
+static int intel_dg_mtd_get_device(struct mtd_info *mtd)
+{
+	struct mtd_info *master = mtd_get_master(mtd);
+	struct intel_dg_nvm *nvm = master->priv;
+
+	if (WARN_ON(!nvm))
+		return -EINVAL;
+	pr_debug("get mtd %s %d\n", mtd->name, kref_read(&nvm->refcnt));
+	kref_get(&nvm->refcnt);
+
+	return 0;
+}
+
+static void intel_dg_mtd_put_device(struct mtd_info *mtd)
+{
+	struct mtd_info *master = mtd_get_master(mtd);
+	struct intel_dg_nvm *nvm = master->priv;
+
+	if (WARN_ON(!nvm))
+		return;
+	pr_debug("put mtd %s %d\n", mtd->name, kref_read(&nvm->refcnt));
+	kref_put(&nvm->refcnt, intel_dg_nvm_release);
+}
+
+static int intel_dg_nvm_init_mtd(struct intel_dg_nvm *nvm, struct device *device,
+				 unsigned int nparts, bool writeable_override)
+{
+	unsigned int i;
+	unsigned int n;
+	struct mtd_partition *parts = NULL;
+	int ret;
+
+	dev_dbg(device, "registering with mtd\n");
+
+	nvm->mtd.owner = THIS_MODULE;
+	nvm->mtd.dev.parent = device;
+	nvm->mtd.flags = MTD_CAP_NORFLASH | MTD_WRITEABLE;
+	nvm->mtd.type = MTD_DATAFLASH;
+	nvm->mtd.priv = nvm;
+	nvm->mtd._write = intel_dg_mtd_write;
+	nvm->mtd._read = intel_dg_mtd_read;
+	nvm->mtd._erase = intel_dg_mtd_erase;
+	nvm->mtd._get_device = intel_dg_mtd_get_device;
+	nvm->mtd._put_device = intel_dg_mtd_put_device;
+	nvm->mtd.writesize = SZ_1; /* 1 byte granularity */
+	nvm->mtd.erasesize = SZ_4K; /* 4K bytes granularity */
+	nvm->mtd.size = nvm->size;
+
+	parts = kcalloc(nvm->nregions, sizeof(*parts), GFP_KERNEL);
+	if (!parts)
+		return -ENOMEM;
+
+	for (i = 0, n = 0; i < nvm->nregions && n < nparts; i++) {
+		if (!nvm->regions[i].is_readable)
+			continue;
+		parts[n].name = nvm->regions[i].name;
+		parts[n].offset  = nvm->regions[i].offset;
+		parts[n].size = nvm->regions[i].size;
+		if (!nvm->regions[i].is_writable && !writeable_override)
+			parts[n].mask_flags = MTD_WRITEABLE;
+		n++;
+	}
+
+	ret = mtd_device_register(&nvm->mtd, parts, n);
+
+	kfree(parts);
+
+	return ret;
+}
+
 static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
 			      const struct auxiliary_device_id *aux_dev_id)
 {
@@ -458,6 +671,7 @@ static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
 		return -ENOMEM;
 
 	kref_init(&nvm->refcnt);
+	mutex_init(&nvm->lock);
 
 	nvm->nregions = nregions;
 	for (n = 0, i = 0; i < INTEL_DG_NVM_REGIONS; i++) {
@@ -487,6 +701,12 @@ static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
 		goto err;
 	}
 
+	ret = intel_dg_nvm_init_mtd(nvm, device, ret, invm->writeable_override);
+	if (ret) {
+		dev_err(device, "failed init mtd %d\n", ret);
+		goto err;
+	}
+
 	dev_set_drvdata(&aux_dev->dev, nvm);
 
 	return 0;
@@ -503,6 +723,8 @@ static void intel_dg_mtd_remove(struct auxiliary_device *aux_dev)
 	if (!nvm)
 		return;
 
+	mtd_device_unregister(&nvm->mtd);
+
 	dev_set_drvdata(&aux_dev->dev, NULL);
 
 	kref_put(&nvm->refcnt, intel_dg_nvm_release);
-- 
2.43.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 05/10] mtd: intel-dg: align 64bit read and write

[Alexander Usyskin]

GSC NVM controller 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/mtd/devices/mtd-intel-dg.c | 35 ++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/drivers/mtd/devices/mtd-intel-dg.c b/drivers/mtd/devices/mtd-intel-dg.c
index 2efcb4d64ebf..4951438e8faf 100644
--- a/drivers/mtd/devices/mtd-intel-dg.c
+++ b/drivers/mtd/devices/mtd-intel-dg.c
@@ -239,6 +239,24 @@ static ssize_t idg_write(struct intel_dg_nvm *nvm, u8 region,
 		len_s -= to_shift;
 	}
 
+	if (!IS_ALIGNED(to, sizeof(u64)) &&
+	    ((to ^ (to + len_s)) & 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));
+		idg_nvm_write32(nvm, to, data);
+		if (idg_nvm_error(nvm))
+			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;
@@ -297,6 +315,23 @@ static ssize_t idg_read(struct intel_dg_nvm *nvm, u8 region,
 		from += from_shift;
 	}
 
+	if (!IS_ALIGNED(from, sizeof(u64)) &&
+	    ((from ^ (from + len_s)) & GENMASK(31, 10))) {
+		/*
+		 * Workaround reads/writes across 1k-aligned addresses
+		 * (start u32 before 1k, end u32 after)
+		 * as this fails on hardware.
+		 */
+		u32 data = idg_nvm_read32(nvm, from);
+
+		if (idg_nvm_error(nvm))
+			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 = idg_nvm_read64(nvm, from + i);
-- 
2.43.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 06/10] mtd: intel-dg: wake card on operations

[Alexander Usyskin]

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

CC: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
---
 drivers/mtd/devices/mtd-intel-dg.c | 73 +++++++++++++++++++++++++-----
 1 file changed, 61 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/devices/mtd-intel-dg.c b/drivers/mtd/devices/mtd-intel-dg.c
index 4951438e8faf..3d53f35478e8 100644
--- a/drivers/mtd/devices/mtd-intel-dg.c
+++ b/drivers/mtd/devices/mtd-intel-dg.c
@@ -15,11 +15,14 @@
 #include <linux/module.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
+#include <linux/pm_runtime.h>
 #include <linux/string.h>
 #include <linux/slab.h>
 #include <linux/sizes.h>
 #include <linux/types.h>
 
+#define INTEL_DG_NVM_RPM_TIMEOUT 500
+
 struct intel_dg_nvm {
 	struct kref refcnt;
 	struct mtd_info mtd;
@@ -462,6 +465,7 @@ static int intel_dg_mtd_erase(struct mtd_info *mtd, struct erase_info *info)
 	loff_t from;
 	size_t len;
 	size_t total_len;
+	int ret = 0;
 
 	if (WARN_ON(!nvm))
 		return -EINVAL;
@@ -476,20 +480,28 @@ static int intel_dg_mtd_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;
+	}
+
 	guard(mutex)(&nvm->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;
-			return -ERANGE;
+			ret = -ERANGE;
+			goto out;
 		}
 
 		idx = idg_nvm_get_region(nvm, addr);
 		if (idx >= nvm->nregions) {
 			dev_err(&mtd->dev, "out of range");
 			info->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
-			return -ERANGE;
+			ret = -ERANGE;
+			goto out;
 		}
 
 		from = addr - nvm->regions[idx].offset;
@@ -505,14 +517,18 @@ static int intel_dg_mtd_erase(struct mtd_info *mtd, struct erase_info *info)
 		if (bytes < 0) {
 			dev_dbg(&mtd->dev, "erase failed with %zd\n", bytes);
 			info->fail_addr += nvm->regions[idx].offset;
-			return bytes;
+			ret = bytes;
+			goto out;
 		}
 
 		addr += len;
 		total_len -= len;
 	}
 
-	return 0;
+out:
+	pm_runtime_mark_last_busy(mtd->dev.parent);
+	pm_runtime_put_autosuspend(mtd->dev.parent);
+	return ret;
 }
 
 static int intel_dg_mtd_read(struct mtd_info *mtd, loff_t from, size_t len,
@@ -541,17 +557,25 @@ static int intel_dg_mtd_read(struct mtd_info *mtd, loff_t from, size_t len,
 	if (len > nvm->regions[idx].size - from)
 		len = nvm->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;
+	}
+
 	guard(mutex)(&nvm->lock);
 
 	ret = idg_read(nvm, region, from, len, buf);
 	if (ret < 0) {
 		dev_dbg(&mtd->dev, "read failed with %zd\n", ret);
-		return ret;
+	} else {
+		*retlen = ret;
+		ret = 0;
 	}
 
-	*retlen = ret;
-
-	return 0;
+	pm_runtime_mark_last_busy(mtd->dev.parent);
+	pm_runtime_put_autosuspend(mtd->dev.parent);
+	return ret;
 }
 
 static int intel_dg_mtd_write(struct mtd_info *mtd, loff_t to, size_t len,
@@ -580,17 +604,25 @@ static int intel_dg_mtd_write(struct mtd_info *mtd, loff_t to, size_t len,
 	if (len > nvm->regions[idx].size - to)
 		len = nvm->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;
+	}
+
 	guard(mutex)(&nvm->lock);
 
 	ret = idg_write(nvm, region, to, len, buf);
 	if (ret < 0) {
 		dev_dbg(&mtd->dev, "write failed with %zd\n", ret);
-		return ret;
+	} else {
+		*retlen = ret;
+		ret = 0;
 	}
 
-	*retlen = ret;
-
-	return 0;
+	pm_runtime_mark_last_busy(mtd->dev.parent);
+	pm_runtime_put_autosuspend(mtd->dev.parent);
+	return ret;
 }
 
 static void intel_dg_nvm_release(struct kref *kref)
@@ -722,6 +754,17 @@ static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
 		n++;
 	}
 
+	pm_runtime_enable(device);
+
+	pm_runtime_set_autosuspend_delay(device, INTEL_DG_NVM_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;
+	}
+
 	nvm->base = devm_ioremap_resource(device, &invm->bar);
 	if (IS_ERR(nvm->base)) {
 		dev_err(device, "mmio not mapped\n");
@@ -744,9 +787,13 @@ static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
 
 	dev_set_drvdata(&aux_dev->dev, nvm);
 
+	pm_runtime_put(device);
 	return 0;
 
 err:
+	pm_runtime_put(device);
+err_norpm:
+	pm_runtime_disable(device);
 	kref_put(&nvm->refcnt, intel_dg_nvm_release);
 	return ret;
 }
@@ -758,6 +805,8 @@ static void intel_dg_mtd_remove(struct auxiliary_device *aux_dev)
 	if (!nvm)
 		return;
 
+	pm_runtime_disable(&aux_dev->dev);
+
 	mtd_device_unregister(&nvm->mtd);
 
 	dev_set_drvdata(&aux_dev->dev, NULL);
-- 
2.43.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 07/10] drm/i915/nvm: add nvm device for discrete graphics

[Alexander Usyskin]

Enable access to internal non-volatile memory on
DGFX devices via a child device.
The nvm child device is exposed via auxiliary bus.

CC: Rodrigo Vivi <rodrigo.vivi@intel.com>
CC: Lucas De Marchi <lucas.demarchi@intel.com>
Co-developed-by: Tomas Winkler <tomasw@gmail.com>
Signed-off-by: Tomas Winkler <tomasw@gmail.com>
Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
---
 drivers/gpu/drm/i915/Makefile      |  4 ++
 drivers/gpu/drm/i915/i915_driver.c |  6 ++
 drivers/gpu/drm/i915/i915_drv.h    |  3 +
 drivers/gpu/drm/i915/i915_reg.h    |  1 +
 drivers/gpu/drm/i915/intel_nvm.c   | 95 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_nvm.h   | 15 +++++
 6 files changed, 124 insertions(+)
 create mode 100644 drivers/gpu/drm/i915/intel_nvm.c
 create mode 100644 drivers/gpu/drm/i915/intel_nvm.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index c63fa2133ccb..bcb8ddadde51 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -211,6 +211,10 @@ i915-y += \
 i915-y += \
 	gt/intel_gsc.o
 
+# graphics nvm device (DGFX) support
+i915-y += \
+	intel_nvm.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 a40f05b993da..782c3c7f913d 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 "intel_nvm.h"
+
 #include "i915_debugfs.h"
 #include "i915_driver.h"
 #include "i915_drm_client.h"
@@ -620,6 +622,8 @@ static void i915_driver_register(struct drm_i915_private *dev_priv)
 	/* Depends on sysfs having been initialized */
 	i915_perf_register(dev_priv);
 
+	intel_nvm_init(dev_priv);
+
 	for_each_gt(gt, dev_priv, i)
 		intel_gt_driver_register(gt);
 
@@ -663,6 +667,8 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv)
 
 	i915_hwmon_unregister(dev_priv);
 
+	intel_nvm_fini(dev_priv);
+
 	i915_perf_unregister(dev_priv);
 	i915_pmu_unregister(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 39f6614a0a99..fa7c94430e27 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -67,6 +67,7 @@
 struct drm_i915_clock_gating_funcs;
 struct vlv_s0ix_state;
 struct intel_pxp;
+struct intel_dg_nvm_dev;
 
 #define GEM_QUIRK_PIN_SWIZZLED_PAGES	BIT(0)
 
@@ -315,6 +316,8 @@ struct drm_i915_private {
 
 	struct i915_perf perf;
 
+	struct intel_dg_nvm_dev *nvm;
+
 	struct i915_hwmon *hwmon;
 
 	struct intel_gt *gt[I915_MAX_GT];
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 41f4350a7c6c..91c877c5f932 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -323,6 +323,7 @@
 #define DG2_GSC_HECI2_BASE	0x00374000
 #define MTL_GSC_HECI1_BASE	0x00116000
 #define MTL_GSC_HECI2_BASE	0x00117000
+#define GEN12_GUNIT_NVM_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/intel_nvm.c b/drivers/gpu/drm/i915/intel_nvm.c
new file mode 100644
index 000000000000..003c89cbcb88
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_nvm.c
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright(c) 2019-2024, Intel Corporation. All rights reserved.
+ */
+
+#include <linux/intel_dg_nvm_aux.h>
+#include <linux/irq.h>
+#include "i915_reg.h"
+#include "i915_drv.h"
+#include "intel_nvm.h"
+
+#define GEN12_GUNIT_NVM_SIZE 0x80
+
+static const struct intel_dg_nvm_region regions[INTEL_DG_NVM_REGIONS] = {
+	[0] = { .name = "DESCRIPTOR", },
+	[2] = { .name = "GSC", },
+	[11] = { .name = "OptionROM", },
+	[12] = { .name = "DAM", },
+};
+
+static void i915_nvm_release_dev(struct device *dev)
+{
+}
+
+void intel_nvm_init(struct drm_i915_private *i915)
+{
+	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
+	struct intel_dg_nvm_dev *nvm;
+	struct auxiliary_device *aux_dev;
+	int ret;
+
+	/* Only the DGFX devices have internal NVM */
+	if (!IS_DGFX(i915))
+		return;
+
+	/* Nvm pointer should be NULL here */
+	if (WARN_ON(i915->nvm))
+		return;
+
+	i915->nvm = kzalloc(sizeof(*nvm), GFP_KERNEL);
+	if (!i915->nvm)
+		return;
+
+	nvm = i915->nvm;
+
+	nvm->writeable_override = true;
+	nvm->bar.parent = &pdev->resource[0];
+	nvm->bar.start = GEN12_GUNIT_NVM_BASE + pdev->resource[0].start;
+	nvm->bar.end = nvm->bar.start + GEN12_GUNIT_NVM_SIZE - 1;
+	nvm->bar.flags = IORESOURCE_MEM;
+	nvm->bar.desc = IORES_DESC_NONE;
+	nvm->regions = regions;
+
+	aux_dev = &nvm->aux_dev;
+
+	aux_dev->name = "nvm";
+	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_nvm_release_dev;
+
+	ret = auxiliary_device_init(aux_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "i915-nvm aux init failed %d\n", ret);
+		return;
+	}
+
+	ret = auxiliary_device_add(aux_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "i915-nvm aux add failed %d\n", ret);
+		auxiliary_device_uninit(aux_dev);
+		return;
+	}
+}
+
+void intel_nvm_fini(struct drm_i915_private *i915)
+{
+	struct intel_dg_nvm_dev *nvm = i915->nvm;
+	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
+
+	/* Only the DGFX devices have internal NVM */
+	if (!IS_DGFX(i915))
+		return;
+
+	/* Nvm pointer should not be NULL here */
+	if (WARN_ON(!nvm))
+		return;
+
+	dev_dbg(&pdev->dev, "removing i915-nvm cell\n");
+
+	auxiliary_device_delete(&nvm->aux_dev);
+	auxiliary_device_uninit(&nvm->aux_dev);
+	kfree(nvm);
+	i915->nvm = NULL;
+}
diff --git a/drivers/gpu/drm/i915/intel_nvm.h b/drivers/gpu/drm/i915/intel_nvm.h
new file mode 100644
index 000000000000..7bc3d1114a3f
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_nvm.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright(c) 2019-2024 Intel Corporation. All rights reserved.
+ */
+
+#ifndef __INTEL_NVM_H__
+#define __INTEL_NVM_H__
+
+struct drm_i915_private;
+
+void intel_nvm_init(struct drm_i915_private *i915);
+
+void intel_nvm_fini(struct drm_i915_private *i915);
+
+#endif /* __INTEL_NVM_H__ */
-- 
2.43.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 08/10] drm/i915/nvm: add support for access mode

[Alexander Usyskin]

Check NVM 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/intel_nvm.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_nvm.c b/drivers/gpu/drm/i915/intel_nvm.c
index 003c89cbcb88..98c11e027b76 100644
--- a/drivers/gpu/drm/i915/intel_nvm.c
+++ b/drivers/gpu/drm/i915/intel_nvm.c
@@ -10,6 +10,7 @@
 #include "intel_nvm.h"
 
 #define GEN12_GUNIT_NVM_SIZE 0x80
+#define HECI_FW_STATUS_2_NVM_ACCESS_MODE BIT(3)
 
 static const struct intel_dg_nvm_region regions[INTEL_DG_NVM_REGIONS] = {
 	[0] = { .name = "DESCRIPTOR", },
@@ -22,6 +23,29 @@ static void i915_nvm_release_dev(struct device *dev)
 {
 }
 
+static bool i915_nvm_writeable_override(struct drm_i915_private *i915)
+{
+	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
+	resource_size_t base;
+	bool writeable_override;
+
+	if (IS_DG1(i915)) {
+		base = DG1_GSC_HECI2_BASE;
+	} else if (IS_DG2(i915)) {
+		base = DG2_GSC_HECI2_BASE;
+	} else {
+		dev_err(&pdev->dev, "Unknown platform\n");
+		return true;
+	}
+
+	writeable_override =
+		!(intel_uncore_read(&i915->uncore, HECI_FWSTS(base, 2)) &
+		  HECI_FW_STATUS_2_NVM_ACCESS_MODE);
+	if (writeable_override)
+		dev_info(&pdev->dev, "NVM access overridden by jumper\n");
+	return writeable_override;
+}
+
 void intel_nvm_init(struct drm_i915_private *i915)
 {
 	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
@@ -43,7 +67,7 @@ void intel_nvm_init(struct drm_i915_private *i915)
 
 	nvm = i915->nvm;
 
-	nvm->writeable_override = true;
+	nvm->writeable_override = i915_nvm_writeable_override(i915);
 	nvm->bar.parent = &pdev->resource[0];
 	nvm->bar.start = GEN12_GUNIT_NVM_BASE + pdev->resource[0].start;
 	nvm->bar.end = nvm->bar.start + GEN12_GUNIT_NVM_SIZE - 1;
-- 
2.43.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 09/10] drm/xe/nvm: add on-die non-volatile memory device

[Alexander Usyskin]

Enable access to internal non-volatile memory on DGFX
with GSC/CSC devices via a child device.
The nvm child device is exposed via auxiliary bus.

Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
---
 drivers/gpu/drm/xe/Makefile          |   1 +
 drivers/gpu/drm/xe/xe_device.c       |   3 +
 drivers/gpu/drm/xe/xe_device_types.h |   8 +++
 drivers/gpu/drm/xe/xe_nvm.c          | 100 +++++++++++++++++++++++++++
 drivers/gpu/drm/xe/xe_nvm.h          |  15 ++++
 drivers/gpu/drm/xe/xe_pci.c          |   5 ++
 6 files changed, 132 insertions(+)
 create mode 100644 drivers/gpu/drm/xe/xe_nvm.c
 create mode 100644 drivers/gpu/drm/xe/xe_nvm.h

diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
index cb6c625bdef0..4225a654a937 100644
--- a/drivers/gpu/drm/xe/Makefile
+++ b/drivers/gpu/drm/xe/Makefile
@@ -94,6 +94,7 @@ xe-y += xe_bb.o \
 	xe_ring_ops.o \
 	xe_sa.o \
 	xe_sched_job.o \
+	xe_nvm.o \
 	xe_step.o \
 	xe_sync.o \
 	xe_tile.o \
diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
index 962751c966d1..844697f79eee 100644
--- a/drivers/gpu/drm/xe/xe_device.c
+++ b/drivers/gpu/drm/xe/xe_device.c
@@ -49,6 +49,7 @@
 #include "xe_pcode.h"
 #include "xe_pm.h"
 #include "xe_query.h"
+#include "xe_nvm.h"
 #include "xe_sriov.h"
 #include "xe_tile.h"
 #include "xe_ttm_stolen_mgr.h"
@@ -743,6 +744,7 @@ int xe_device_probe(struct xe_device *xe)
 			goto err_fini_gt;
 	}
 
+	xe_nvm_init(xe);
 	xe_heci_gsc_init(xe);
 
 	err = xe_oa_init(xe);
@@ -811,6 +813,7 @@ void xe_device_remove(struct xe_device *xe)
 	xe_oa_fini(xe);
 
 	xe_heci_gsc_fini(xe);
+	xe_nvm_fini(xe);
 
 	for_each_gt(gt, xe, id)
 		xe_gt_remove(gt);
diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
index 85bede4dd646..ec3d82f05519 100644
--- a/drivers/gpu/drm/xe/xe_device_types.h
+++ b/drivers/gpu/drm/xe/xe_device_types.h
@@ -35,6 +35,8 @@
 struct xe_ggtt;
 struct xe_pat_ops;
 
+struct intel_dg_nvm_dev;
+
 #define XE_BO_INVALID_OFFSET	LONG_MAX
 
 #define GRAPHICS_VER(xe) ((xe)->info.graphics_verx100 / 100)
@@ -44,6 +46,7 @@ struct xe_pat_ops;
 #define IS_DGFX(xe) ((xe)->info.is_dgfx)
 #define HAS_HECI_GSCFI(xe) ((xe)->info.has_heci_gscfi)
 #define HAS_HECI_CSCFI(xe) ((xe)->info.has_heci_cscfi)
+#define HAS_GSC_NVM(xe) ((xe)->info.has_gsc_nvm)
 
 #define XE_VRAM_FLAGS_NEED64K		BIT(0)
 
@@ -331,6 +334,8 @@ struct xe_device {
 		u8 has_heci_gscfi:1;
 		/** @info.has_heci_cscfi: device has heci cscfi */
 		u8 has_heci_cscfi:1;
+		/** @info.has_gsc_nvm: device has gsc non-volatile memory */
+		u8 has_gsc_nvm:1;
 		/** @info.skip_guc_pc: Skip GuC based PM feature init */
 		u8 skip_guc_pc:1;
 		/** @info.has_atomic_enable_pte_bit: Device has atomic enable PTE bit */
@@ -502,6 +507,9 @@ struct xe_device {
 	/** @heci_gsc: graphics security controller */
 	struct xe_heci_gsc heci_gsc;
 
+	/** @nvm: discrete graphics non-volatile memory */
+	struct intel_dg_nvm_dev *nvm;
+
 	/** @oa: oa observation subsystem */
 	struct xe_oa oa;
 
diff --git a/drivers/gpu/drm/xe/xe_nvm.c b/drivers/gpu/drm/xe/xe_nvm.c
new file mode 100644
index 000000000000..ce56bff1268b
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_nvm.c
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright(c) 2019-2024, Intel Corporation. All rights reserved.
+ */
+
+#include <linux/intel_dg_nvm_aux.h>
+#include <linux/pci.h>
+#include "xe_device_types.h"
+#include "xe_nvm.h"
+#include "xe_sriov.h"
+
+#define GEN12_GUNIT_NVM_BASE 0x00102040
+#define GEN12_GUNIT_NVM_SIZE 0x80
+#define HECI_FW_STATUS_2_NVM_ACCESS_MODE BIT(3)
+
+static const struct intel_dg_nvm_region regions[INTEL_DG_NVM_REGIONS] = {
+	[0] = { .name = "DESCRIPTOR", },
+	[2] = { .name = "GSC", },
+	[11] = { .name = "OptionROM", },
+	[12] = { .name = "DAM", },
+};
+
+static void xe_nvm_release_dev(struct device *dev)
+{
+}
+
+void xe_nvm_init(struct xe_device *xe)
+{
+	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
+	struct intel_dg_nvm_dev *nvm;
+	struct auxiliary_device *aux_dev;
+	int ret;
+
+	if (!HAS_GSC_NVM(xe))
+		return;
+
+	/* No access to internal NVM from VFs */
+	if (IS_SRIOV_VF(xe))
+		return;
+
+	/* Nvm pointer should be NULL here */
+	if (WARN_ON(xe->nvm))
+		return;
+
+	xe->nvm = kzalloc(sizeof(*nvm), GFP_KERNEL);
+	if (!xe->nvm)
+		return;
+
+	nvm = xe->nvm;
+
+	nvm->writeable_override = false;
+	nvm->bar.parent = &pdev->resource[0];
+	nvm->bar.start = GEN12_GUNIT_NVM_BASE + pdev->resource[0].start;
+	nvm->bar.end = nvm->bar.start + GEN12_GUNIT_NVM_SIZE - 1;
+	nvm->bar.flags = IORESOURCE_MEM;
+	nvm->bar.desc = IORES_DESC_NONE;
+	nvm->regions = regions;
+
+	aux_dev = &nvm->aux_dev;
+
+	aux_dev->name = "nvm";
+	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 = xe_nvm_release_dev;
+
+	ret = auxiliary_device_init(aux_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "xe-nvm aux init failed %d\n", ret);
+		return;
+	}
+
+	ret = auxiliary_device_add(aux_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "xe-nvm aux add failed %d\n", ret);
+		auxiliary_device_uninit(aux_dev);
+		return;
+	}
+}
+
+void xe_nvm_fini(struct xe_device *xe)
+{
+	struct intel_dg_nvm_dev *nvm = xe->nvm;
+
+	if (!HAS_GSC_NVM(xe))
+		return;
+
+	/* No access to internal NVM from VFs */
+	if (IS_SRIOV_VF(xe))
+		return;
+
+	/* Nvm pointer should not be NULL here */
+	if (WARN_ON(!nvm))
+		return;
+
+	auxiliary_device_delete(&nvm->aux_dev);
+	auxiliary_device_uninit(&nvm->aux_dev);
+	kfree(nvm);
+	xe->nvm = NULL;
+}
diff --git a/drivers/gpu/drm/xe/xe_nvm.h b/drivers/gpu/drm/xe/xe_nvm.h
new file mode 100644
index 000000000000..068695447913
--- /dev/null
+++ b/drivers/gpu/drm/xe/xe_nvm.h
@@ -0,0 +1,15 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright(c) 2019-2024 Intel Corporation. All rights reserved.
+ */
+
+#ifndef __XE_NVM_H__
+#define __XE_NVM_H__
+
+struct xe_device;
+
+void xe_nvm_init(struct xe_device *xe);
+
+void xe_nvm_fini(struct xe_device *xe);
+
+#endif /* __XE_NVM_H__ */
diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
index 64a8336ca437..85c419eea710 100644
--- a/drivers/gpu/drm/xe/xe_pci.c
+++ b/drivers/gpu/drm/xe/xe_pci.c
@@ -60,6 +60,7 @@ struct xe_device_desc {
 	u8 has_display:1;
 	u8 has_heci_gscfi:1;
 	u8 has_heci_cscfi:1;
+	u8 has_gsc_nvm:1;
 	u8 has_llc:1;
 	u8 has_mmio_ext:1;
 	u8 has_sriov:1;
@@ -282,6 +283,7 @@ static const struct xe_device_desc dg1_desc = {
 	PLATFORM(DG1),
 	.has_display = true,
 	.has_heci_gscfi = 1,
+	.has_gsc_nvm = 1,
 	.require_force_probe = true,
 };
 
@@ -293,6 +295,7 @@ static const u16 dg2_g12_ids[] = { XE_DG2_G12_IDS(NOP), 0 };
 	DGFX_FEATURES, \
 	PLATFORM(DG2), \
 	.has_heci_gscfi = 1, \
+	.has_gsc_nvm = 1, \
 	.subplatforms = (const struct xe_subplatform_desc[]) { \
 		{ XE_SUBPLATFORM_DG2_G10, "G10", dg2_g10_ids }, \
 		{ XE_SUBPLATFORM_DG2_G11, "G11", dg2_g11_ids }, \
@@ -324,6 +327,7 @@ static const __maybe_unused struct xe_device_desc pvc_desc = {
 	PLATFORM(PVC),
 	.has_display = false,
 	.has_heci_gscfi = 1,
+	.has_gsc_nvm = 1,
 	.require_force_probe = true,
 };
 
@@ -623,6 +627,7 @@ static int xe_info_init_early(struct xe_device *xe,
 	xe->info.is_dgfx = desc->is_dgfx;
 	xe->info.has_heci_gscfi = desc->has_heci_gscfi;
 	xe->info.has_heci_cscfi = desc->has_heci_cscfi;
+	xe->info.has_gsc_nvm = desc->has_gsc_nvm;
 	xe->info.has_llc = desc->has_llc;
 	xe->info.has_mmio_ext = desc->has_mmio_ext;
 	xe->info.has_sriov = desc->has_sriov;
-- 
2.43.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 10/10] drm/xe/nvm: add support for access mode

[Alexander Usyskin]

Check NVM 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/xe/regs/xe_gsc_regs.h |  4 ++++
 drivers/gpu/drm/xe/xe_heci_gsc.c      |  5 +---
 drivers/gpu/drm/xe/xe_nvm.c           | 33 ++++++++++++++++++++++++++-
 3 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/xe/regs/xe_gsc_regs.h b/drivers/gpu/drm/xe/regs/xe_gsc_regs.h
index 7702364b65f1..9b66cc972a63 100644
--- a/drivers/gpu/drm/xe/regs/xe_gsc_regs.h
+++ b/drivers/gpu/drm/xe/regs/xe_gsc_regs.h
@@ -16,6 +16,10 @@
 #define MTL_GSC_HECI1_BASE	0x00116000
 #define MTL_GSC_HECI2_BASE	0x00117000
 
+#define DG1_GSC_HECI2_BASE	0x00259000
+#define PVC_GSC_HECI2_BASE	0x00285000
+#define DG2_GSC_HECI2_BASE	0x00374000
+
 #define HECI_H_CSR(base)	XE_REG((base) + 0x4)
 #define   HECI_H_CSR_IE		REG_BIT(0)
 #define   HECI_H_CSR_IS		REG_BIT(1)
diff --git a/drivers/gpu/drm/xe/xe_heci_gsc.c b/drivers/gpu/drm/xe/xe_heci_gsc.c
index 65b2e147c4b9..27734085164e 100644
--- a/drivers/gpu/drm/xe/xe_heci_gsc.c
+++ b/drivers/gpu/drm/xe/xe_heci_gsc.c
@@ -11,14 +11,11 @@
 #include "xe_device_types.h"
 #include "xe_drv.h"
 #include "xe_heci_gsc.h"
+#include "regs/xe_gsc_regs.h"
 #include "xe_platform_types.h"
 
 #define GSC_BAR_LENGTH  0x00000FFC
 
-#define DG1_GSC_HECI2_BASE			0x259000
-#define PVC_GSC_HECI2_BASE			0x285000
-#define DG2_GSC_HECI2_BASE			0x374000
-
 static void heci_gsc_irq_mask(struct irq_data *d)
 {
 	/* generic irq handling */
diff --git a/drivers/gpu/drm/xe/xe_nvm.c b/drivers/gpu/drm/xe/xe_nvm.c
index ce56bff1268b..fbebe02522c0 100644
--- a/drivers/gpu/drm/xe/xe_nvm.c
+++ b/drivers/gpu/drm/xe/xe_nvm.c
@@ -5,8 +5,11 @@
 
 #include <linux/intel_dg_nvm_aux.h>
 #include <linux/pci.h>
+#include "xe_device.h"
 #include "xe_device_types.h"
+#include "xe_mmio.h"
 #include "xe_nvm.h"
+#include "regs/xe_gsc_regs.h"
 #include "xe_sriov.h"
 
 #define GEN12_GUNIT_NVM_BASE 0x00102040
@@ -24,6 +27,34 @@ static void xe_nvm_release_dev(struct device *dev)
 {
 }
 
+static bool xe_nvm_writeable_override(struct xe_device *xe)
+{
+	struct xe_gt *gt = xe_root_mmio_gt(xe);
+	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
+	resource_size_t base;
+	bool writeable_override;
+
+	if (xe->info.platform == XE_BATTLEMAGE) {
+		base = DG2_GSC_HECI2_BASE;
+	} else if (xe->info.platform == XE_PVC) {
+		base = PVC_GSC_HECI2_BASE;
+	} else if (xe->info.platform == XE_DG2) {
+		base = DG2_GSC_HECI2_BASE;
+	} else if (xe->info.platform == XE_DG1) {
+		base = DG1_GSC_HECI2_BASE;
+	} else {
+		dev_err(&pdev->dev, "Unknown platform\n");
+		return true;
+	}
+
+	writeable_override =
+		!(xe_mmio_read32(&gt->mmio, HECI_H_GS1(base)) &
+		  HECI_FW_STATUS_2_NVM_ACCESS_MODE);
+	if (writeable_override)
+		dev_info(&pdev->dev, "NVM access overridden by jumper\n");
+	return writeable_override;
+}
+
 void xe_nvm_init(struct xe_device *xe)
 {
 	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
@@ -48,7 +79,7 @@ void xe_nvm_init(struct xe_device *xe)
 
 	nvm = xe->nvm;
 
-	nvm->writeable_override = false;
+	nvm->writeable_override = xe_nvm_writeable_override(xe);
 	nvm->bar.parent = &pdev->resource[0];
 	nvm->bar.start = GEN12_GUNIT_NVM_BASE + pdev->resource[0].start;
 	nvm->bar.end = nvm->bar.start + GEN12_GUNIT_NVM_SIZE - 1;
-- 
2.43.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 09/10] drm/xe/nvm: add on-die non-volatile memory device

[Rodrigo Vivi]

On Tue, Oct 22, 2024 at 01:41:18PM +0300, Alexander Usyskin wrote:
> Enable access to internal non-volatile memory on DGFX
> with GSC/CSC devices via a child device.
> The nvm child device is exposed via auxiliary bus.

I looked at all of the i915 and xe patches here and everything looks right.
Just a few common doubts before I put my rv-b here below...


Starting with the one from the other patch. Could you please share some doc
where I could confirm
HECI_FW_STATUS_2_NVM_ACCESS_MODE bit?

more below...

> 
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> ---
>  drivers/gpu/drm/xe/Makefile          |   1 +
>  drivers/gpu/drm/xe/xe_device.c       |   3 +
>  drivers/gpu/drm/xe/xe_device_types.h |   8 +++
>  drivers/gpu/drm/xe/xe_nvm.c          | 100 +++++++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_nvm.h          |  15 ++++
>  drivers/gpu/drm/xe/xe_pci.c          |   5 ++
>  6 files changed, 132 insertions(+)
>  create mode 100644 drivers/gpu/drm/xe/xe_nvm.c
>  create mode 100644 drivers/gpu/drm/xe/xe_nvm.h
> 
> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> index cb6c625bdef0..4225a654a937 100644
> --- a/drivers/gpu/drm/xe/Makefile
> +++ b/drivers/gpu/drm/xe/Makefile
> @@ -94,6 +94,7 @@ xe-y += xe_bb.o \
>  	xe_ring_ops.o \
>  	xe_sa.o \
>  	xe_sched_job.o \
> +	xe_nvm.o \
>  	xe_step.o \
>  	xe_sync.o \
>  	xe_tile.o \
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 962751c966d1..844697f79eee 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -49,6 +49,7 @@
>  #include "xe_pcode.h"
>  #include "xe_pm.h"
>  #include "xe_query.h"
> +#include "xe_nvm.h"
>  #include "xe_sriov.h"
>  #include "xe_tile.h"
>  #include "xe_ttm_stolen_mgr.h"
> @@ -743,6 +744,7 @@ int xe_device_probe(struct xe_device *xe)
>  			goto err_fini_gt;
>  	}
>  
> +	xe_nvm_init(xe);
>  	xe_heci_gsc_init(xe);
>  
>  	err = xe_oa_init(xe);
> @@ -811,6 +813,7 @@ void xe_device_remove(struct xe_device *xe)
>  	xe_oa_fini(xe);
>  
>  	xe_heci_gsc_fini(xe);
> +	xe_nvm_fini(xe);
>  
>  	for_each_gt(gt, xe, id)
>  		xe_gt_remove(gt);
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 85bede4dd646..ec3d82f05519 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -35,6 +35,8 @@
>  struct xe_ggtt;
>  struct xe_pat_ops;
>  
> +struct intel_dg_nvm_dev;
> +
>  #define XE_BO_INVALID_OFFSET	LONG_MAX
>  
>  #define GRAPHICS_VER(xe) ((xe)->info.graphics_verx100 / 100)
> @@ -44,6 +46,7 @@ struct xe_pat_ops;
>  #define IS_DGFX(xe) ((xe)->info.is_dgfx)
>  #define HAS_HECI_GSCFI(xe) ((xe)->info.has_heci_gscfi)
>  #define HAS_HECI_CSCFI(xe) ((xe)->info.has_heci_cscfi)
> +#define HAS_GSC_NVM(xe) ((xe)->info.has_gsc_nvm)
>  
>  #define XE_VRAM_FLAGS_NEED64K		BIT(0)
>  
> @@ -331,6 +334,8 @@ struct xe_device {
>  		u8 has_heci_gscfi:1;
>  		/** @info.has_heci_cscfi: device has heci cscfi */
>  		u8 has_heci_cscfi:1;
> +		/** @info.has_gsc_nvm: device has gsc non-volatile memory */
> +		u8 has_gsc_nvm:1;
>  		/** @info.skip_guc_pc: Skip GuC based PM feature init */
>  		u8 skip_guc_pc:1;
>  		/** @info.has_atomic_enable_pte_bit: Device has atomic enable PTE bit */
> @@ -502,6 +507,9 @@ struct xe_device {
>  	/** @heci_gsc: graphics security controller */
>  	struct xe_heci_gsc heci_gsc;
>  
> +	/** @nvm: discrete graphics non-volatile memory */
> +	struct intel_dg_nvm_dev *nvm;
> +
>  	/** @oa: oa observation subsystem */
>  	struct xe_oa oa;
>  
> diff --git a/drivers/gpu/drm/xe/xe_nvm.c b/drivers/gpu/drm/xe/xe_nvm.c
> new file mode 100644
> index 000000000000..ce56bff1268b
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_nvm.c
> @@ -0,0 +1,100 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright(c) 2019-2024, Intel Corporation. All rights reserved.
> + */
> +
> +#include <linux/intel_dg_nvm_aux.h>
> +#include <linux/pci.h>
> +#include "xe_device_types.h"
> +#include "xe_nvm.h"
> +#include "xe_sriov.h"
> +
> +#define GEN12_GUNIT_NVM_BASE 0x00102040
> +#define GEN12_GUNIT_NVM_SIZE 0x80
> +#define HECI_FW_STATUS_2_NVM_ACCESS_MODE BIT(3)
> +
> +static const struct intel_dg_nvm_region regions[INTEL_DG_NVM_REGIONS] = {
> +	[0] = { .name = "DESCRIPTOR", },
> +	[2] = { .name = "GSC", },
> +	[11] = { .name = "OptionROM", },
> +	[12] = { .name = "DAM", },

Could you please give some pointers to confirm this base and these regions?

> +};
> +
> +static void xe_nvm_release_dev(struct device *dev)
> +{
> +}
> +
> +void xe_nvm_init(struct xe_device *xe)
> +{
> +	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> +	struct intel_dg_nvm_dev *nvm;
> +	struct auxiliary_device *aux_dev;
> +	int ret;
> +
> +	if (!HAS_GSC_NVM(xe))
> +		return;
> +
> +	/* No access to internal NVM from VFs */
> +	if (IS_SRIOV_VF(xe))
> +		return;
> +
> +	/* Nvm pointer should be NULL here */
> +	if (WARN_ON(xe->nvm))
> +		return;
> +
> +	xe->nvm = kzalloc(sizeof(*nvm), GFP_KERNEL);
> +	if (!xe->nvm)
> +		return;
> +
> +	nvm = xe->nvm;
> +
> +	nvm->writeable_override = false;
> +	nvm->bar.parent = &pdev->resource[0];
> +	nvm->bar.start = GEN12_GUNIT_NVM_BASE + pdev->resource[0].start;
> +	nvm->bar.end = nvm->bar.start + GEN12_GUNIT_NVM_SIZE - 1;
> +	nvm->bar.flags = IORESOURCE_MEM;
> +	nvm->bar.desc = IORES_DESC_NONE;
> +	nvm->regions = regions;
> +
> +	aux_dev = &nvm->aux_dev;
> +
> +	aux_dev->name = "nvm";
> +	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 = xe_nvm_release_dev;
> +
> +	ret = auxiliary_device_init(aux_dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "xe-nvm aux init failed %d\n", ret);

Since these are inside the i915 and xe and you have our drm private device,
I believe it would be better if we would use the drm_err and other drm debug
variants here, below and also in the i915 patch.

Thank you so much,
Rodrigo.

> +		return;
> +	}
> +
> +	ret = auxiliary_device_add(aux_dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "xe-nvm aux add failed %d\n", ret);
> +		auxiliary_device_uninit(aux_dev);
> +		return;
> +	}
> +}
> +
> +void xe_nvm_fini(struct xe_device *xe)
> +{
> +	struct intel_dg_nvm_dev *nvm = xe->nvm;
> +
> +	if (!HAS_GSC_NVM(xe))
> +		return;
> +
> +	/* No access to internal NVM from VFs */
> +	if (IS_SRIOV_VF(xe))
> +		return;
> +
> +	/* Nvm pointer should not be NULL here */
> +	if (WARN_ON(!nvm))
> +		return;
> +
> +	auxiliary_device_delete(&nvm->aux_dev);
> +	auxiliary_device_uninit(&nvm->aux_dev);
> +	kfree(nvm);
> +	xe->nvm = NULL;
> +}
> diff --git a/drivers/gpu/drm/xe/xe_nvm.h b/drivers/gpu/drm/xe/xe_nvm.h
> new file mode 100644
> index 000000000000..068695447913
> --- /dev/null
> +++ b/drivers/gpu/drm/xe/xe_nvm.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright(c) 2019-2024 Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef __XE_NVM_H__
> +#define __XE_NVM_H__
> +
> +struct xe_device;
> +
> +void xe_nvm_init(struct xe_device *xe);
> +
> +void xe_nvm_fini(struct xe_device *xe);
> +
> +#endif /* __XE_NVM_H__ */
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index 64a8336ca437..85c419eea710 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -60,6 +60,7 @@ struct xe_device_desc {
>  	u8 has_display:1;
>  	u8 has_heci_gscfi:1;
>  	u8 has_heci_cscfi:1;
> +	u8 has_gsc_nvm:1;
>  	u8 has_llc:1;
>  	u8 has_mmio_ext:1;
>  	u8 has_sriov:1;
> @@ -282,6 +283,7 @@ static const struct xe_device_desc dg1_desc = {
>  	PLATFORM(DG1),
>  	.has_display = true,
>  	.has_heci_gscfi = 1,
> +	.has_gsc_nvm = 1,
>  	.require_force_probe = true,
>  };
>  
> @@ -293,6 +295,7 @@ static const u16 dg2_g12_ids[] = { XE_DG2_G12_IDS(NOP), 0 };
>  	DGFX_FEATURES, \
>  	PLATFORM(DG2), \
>  	.has_heci_gscfi = 1, \
> +	.has_gsc_nvm = 1, \
>  	.subplatforms = (const struct xe_subplatform_desc[]) { \
>  		{ XE_SUBPLATFORM_DG2_G10, "G10", dg2_g10_ids }, \
>  		{ XE_SUBPLATFORM_DG2_G11, "G11", dg2_g11_ids }, \
> @@ -324,6 +327,7 @@ static const __maybe_unused struct xe_device_desc pvc_desc = {
>  	PLATFORM(PVC),
>  	.has_display = false,
>  	.has_heci_gscfi = 1,
> +	.has_gsc_nvm = 1,
>  	.require_force_probe = true,
>  };
>  
> @@ -623,6 +627,7 @@ static int xe_info_init_early(struct xe_device *xe,
>  	xe->info.is_dgfx = desc->is_dgfx;
>  	xe->info.has_heci_gscfi = desc->has_heci_gscfi;
>  	xe->info.has_heci_cscfi = desc->has_heci_cscfi;
> +	xe->info.has_gsc_nvm = desc->has_gsc_nvm;
>  	xe->info.has_llc = desc->has_llc;
>  	xe->info.has_mmio_ext = desc->has_mmio_ext;
>  	xe->info.has_sriov = desc->has_sriov;
> -- 
> 2.43.0
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 06/10] mtd: intel-dg: wake card on operations

[Rodrigo Vivi]

On Tue, Oct 22, 2024 at 01:41:15PM +0300, Alexander Usyskin wrote:
> Enable runtime PM in mtd driver to notify graphics driver that
> whole card should be kept awake while nvm operations are
> performed through this driver.
> 
> CC: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> ---
>  drivers/mtd/devices/mtd-intel-dg.c | 73 +++++++++++++++++++++++++-----
>  1 file changed, 61 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/devices/mtd-intel-dg.c b/drivers/mtd/devices/mtd-intel-dg.c
> index 4951438e8faf..3d53f35478e8 100644
> --- a/drivers/mtd/devices/mtd-intel-dg.c
> +++ b/drivers/mtd/devices/mtd-intel-dg.c
> @@ -15,11 +15,14 @@
>  #include <linux/module.h>
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/partitions.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/string.h>
>  #include <linux/slab.h>
>  #include <linux/sizes.h>
>  #include <linux/types.h>
>  
> +#define INTEL_DG_NVM_RPM_TIMEOUT 500
> +
>  struct intel_dg_nvm {
>  	struct kref refcnt;
>  	struct mtd_info mtd;
> @@ -462,6 +465,7 @@ static int intel_dg_mtd_erase(struct mtd_info *mtd, struct erase_info *info)
>  	loff_t from;
>  	size_t len;
>  	size_t total_len;
> +	int ret = 0;
>  
>  	if (WARN_ON(!nvm))
>  		return -EINVAL;
> @@ -476,20 +480,28 @@ static int intel_dg_mtd_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;
> +	}
> +
>  	guard(mutex)(&nvm->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;
> -			return -ERANGE;
> +			ret = -ERANGE;
> +			goto out;
>  		}
>  
>  		idx = idg_nvm_get_region(nvm, addr);
>  		if (idx >= nvm->nregions) {
>  			dev_err(&mtd->dev, "out of range");
>  			info->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
> -			return -ERANGE;
> +			ret = -ERANGE;
> +			goto out;
>  		}
>  
>  		from = addr - nvm->regions[idx].offset;
> @@ -505,14 +517,18 @@ static int intel_dg_mtd_erase(struct mtd_info *mtd, struct erase_info *info)
>  		if (bytes < 0) {
>  			dev_dbg(&mtd->dev, "erase failed with %zd\n", bytes);
>  			info->fail_addr += nvm->regions[idx].offset;
> -			return bytes;
> +			ret = bytes;
> +			goto out;
>  		}
>  
>  		addr += len;
>  		total_len -= len;
>  	}
>  
> -	return 0;
> +out:
> +	pm_runtime_mark_last_busy(mtd->dev.parent);
> +	pm_runtime_put_autosuspend(mtd->dev.parent);
> +	return ret;
>  }
>  
>  static int intel_dg_mtd_read(struct mtd_info *mtd, loff_t from, size_t len,
> @@ -541,17 +557,25 @@ static int intel_dg_mtd_read(struct mtd_info *mtd, loff_t from, size_t len,
>  	if (len > nvm->regions[idx].size - from)
>  		len = nvm->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;
> +	}
> +
>  	guard(mutex)(&nvm->lock);
>  
>  	ret = idg_read(nvm, region, from, len, buf);
>  	if (ret < 0) {
>  		dev_dbg(&mtd->dev, "read failed with %zd\n", ret);
> -		return ret;
> +	} else {
> +		*retlen = ret;
> +		ret = 0;
>  	}
>  
> -	*retlen = ret;
> -
> -	return 0;
> +	pm_runtime_mark_last_busy(mtd->dev.parent);
> +	pm_runtime_put_autosuspend(mtd->dev.parent);
> +	return ret;
>  }
>  
>  static int intel_dg_mtd_write(struct mtd_info *mtd, loff_t to, size_t len,
> @@ -580,17 +604,25 @@ static int intel_dg_mtd_write(struct mtd_info *mtd, loff_t to, size_t len,
>  	if (len > nvm->regions[idx].size - to)
>  		len = nvm->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;
> +	}
> +
>  	guard(mutex)(&nvm->lock);
>  
>  	ret = idg_write(nvm, region, to, len, buf);
>  	if (ret < 0) {
>  		dev_dbg(&mtd->dev, "write failed with %zd\n", ret);
> -		return ret;
> +	} else {
> +		*retlen = ret;
> +		ret = 0;
>  	}
>  
> -	*retlen = ret;
> -
> -	return 0;
> +	pm_runtime_mark_last_busy(mtd->dev.parent);
> +	pm_runtime_put_autosuspend(mtd->dev.parent);
> +	return ret;
>  }
>  
>  static void intel_dg_nvm_release(struct kref *kref)
> @@ -722,6 +754,17 @@ static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
>  		n++;
>  	}
>  
> +	pm_runtime_enable(device);
> +
> +	pm_runtime_set_autosuspend_delay(device, INTEL_DG_NVM_RPM_TIMEOUT);
> +	pm_runtime_use_autosuspend(device);

something looks strange here...
on the functions above you get and put references for the parent device directly.
But here you enable the rpm for this device.

If I remember correctly from some old experiments, the rpm is smart enough
and wake up the parent before waking up the child. So I'm wondering if we should
only do the child without the parent.

Cc: Imre Deak <imre.deak@intel.com>
Cc: Anshuman Gupta <anshuman.gupta@intel.com>

> +
> +	ret = pm_runtime_resume_and_get(device);
> +	if (ret < 0) {
> +		dev_err(device, "rpm: get failed %d\n", ret);
> +		goto err_norpm;
> +	}
> +
>  	nvm->base = devm_ioremap_resource(device, &invm->bar);
>  	if (IS_ERR(nvm->base)) {
>  		dev_err(device, "mmio not mapped\n");
> @@ -744,9 +787,13 @@ static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
>  
>  	dev_set_drvdata(&aux_dev->dev, nvm);
>  
> +	pm_runtime_put(device);
>  	return 0;
>  
>  err:
> +	pm_runtime_put(device);
> +err_norpm:
> +	pm_runtime_disable(device);
>  	kref_put(&nvm->refcnt, intel_dg_nvm_release);
>  	return ret;
>  }
> @@ -758,6 +805,8 @@ static void intel_dg_mtd_remove(struct auxiliary_device *aux_dev)
>  	if (!nvm)
>  		return;
>  
> +	pm_runtime_disable(&aux_dev->dev);
> +
>  	mtd_device_unregister(&nvm->mtd);
>  
>  	dev_set_drvdata(&aux_dev->dev, NULL);
> -- 
> 2.43.0
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [PATCH 06/10] mtd: intel-dg: wake card on operations

[Gupta, Anshuman]

> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Sent: Monday, October 28, 2024 8:27 PM
> To: Usyskin, Alexander <alexander.usyskin@intel.com>; Gupta, Anshuman
> <anshuman.gupta@intel.com>; Deak, Imre <imre.deak@intel.com>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Richard Weinberger
> <richard@nod.at>; Vignesh Raghavendra <vigneshr@ti.com>; De Marchi,
> Lucas <lucas.demarchi@intel.com>; Thomas Hellström
> <thomas.hellstrom@linux.intel.com>; Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>;
> Thomas Zimmermann <tzimmermann@suse.de>; David Airlie
> <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> <jani.nikula@linux.intel.com>; Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin <tursulin@ursulin.net>;
> Weil, Oren jer <oren.jer.weil@intel.com>; linux-mtd@lists.infradead.org; dri-
> devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 06/10] mtd: intel-dg: wake card on operations
> 
> On Tue, Oct 22, 2024 at 01:41:15PM +0300, Alexander Usyskin wrote:
> > Enable runtime PM in mtd driver to notify graphics driver that whole
> > card should be kept awake while nvm operations are performed through
> > this driver.
> >
> > CC: Lucas De Marchi <lucas.demarchi@intel.com>
> > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> > ---
> >  drivers/mtd/devices/mtd-intel-dg.c | 73
> > +++++++++++++++++++++++++-----
> >  1 file changed, 61 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/mtd/devices/mtd-intel-dg.c
> > b/drivers/mtd/devices/mtd-intel-dg.c
> > index 4951438e8faf..3d53f35478e8 100644
> > --- a/drivers/mtd/devices/mtd-intel-dg.c
> > +++ b/drivers/mtd/devices/mtd-intel-dg.c
> > @@ -15,11 +15,14 @@
> >  #include <linux/module.h>
> >  #include <linux/mtd/mtd.h>
> >  #include <linux/mtd/partitions.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/string.h>
> >  #include <linux/slab.h>
> >  #include <linux/sizes.h>
> >  #include <linux/types.h>
> >
> > +#define INTEL_DG_NVM_RPM_TIMEOUT 500
> > +
> >  struct intel_dg_nvm {
> >  	struct kref refcnt;
> >  	struct mtd_info mtd;
> > @@ -462,6 +465,7 @@ static int intel_dg_mtd_erase(struct mtd_info *mtd,
> struct erase_info *info)
> >  	loff_t from;
> >  	size_t len;
> >  	size_t total_len;
> > +	int ret = 0;
> >
> >  	if (WARN_ON(!nvm))
> >  		return -EINVAL;
> > @@ -476,20 +480,28 @@ static int intel_dg_mtd_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;
> > +	}
> > +
> >  	guard(mutex)(&nvm->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;
> > -			return -ERANGE;
> > +			ret = -ERANGE;
> > +			goto out;
> >  		}
> >
> >  		idx = idg_nvm_get_region(nvm, addr);
> >  		if (idx >= nvm->nregions) {
> >  			dev_err(&mtd->dev, "out of range");
> >  			info->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
> > -			return -ERANGE;
> > +			ret = -ERANGE;
> > +			goto out;
> >  		}
> >
> >  		from = addr - nvm->regions[idx].offset; @@ -505,14 +517,18
> @@
> > static int intel_dg_mtd_erase(struct mtd_info *mtd, struct erase_info *info)
> >  		if (bytes < 0) {
> >  			dev_dbg(&mtd->dev, "erase failed with %zd\n",
> bytes);
> >  			info->fail_addr += nvm->regions[idx].offset;
> > -			return bytes;
> > +			ret = bytes;
> > +			goto out;
> >  		}
> >
> >  		addr += len;
> >  		total_len -= len;
> >  	}
> >
> > -	return 0;
> > +out:
> > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > +	return ret;
> >  }
> >
> >  static int intel_dg_mtd_read(struct mtd_info *mtd, loff_t from,
> > size_t len, @@ -541,17 +557,25 @@ static int intel_dg_mtd_read(struct
> mtd_info *mtd, loff_t from, size_t len,
> >  	if (len > nvm->regions[idx].size - from)
> >  		len = nvm->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;
> > +	}
> > +
> >  	guard(mutex)(&nvm->lock);
> >
> >  	ret = idg_read(nvm, region, from, len, buf);
> >  	if (ret < 0) {
> >  		dev_dbg(&mtd->dev, "read failed with %zd\n", ret);
> > -		return ret;
> > +	} else {
> > +		*retlen = ret;
> > +		ret = 0;
> >  	}
> >
> > -	*retlen = ret;
> > -
> > -	return 0;
> > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > +	return ret;
> >  }
> >
> >  static int intel_dg_mtd_write(struct mtd_info *mtd, loff_t to, size_t
> > len, @@ -580,17 +604,25 @@ static int intel_dg_mtd_write(struct mtd_info
> *mtd, loff_t to, size_t len,
> >  	if (len > nvm->regions[idx].size - to)
> >  		len = nvm->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;
> > +	}
> > +
> >  	guard(mutex)(&nvm->lock);
> >
> >  	ret = idg_write(nvm, region, to, len, buf);
> >  	if (ret < 0) {
> >  		dev_dbg(&mtd->dev, "write failed with %zd\n", ret);
> > -		return ret;
> > +	} else {
> > +		*retlen = ret;
> > +		ret = 0;
> >  	}
> >
> > -	*retlen = ret;
> > -
> > -	return 0;
> > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > +	return ret;
> >  }
> >
> >  static void intel_dg_nvm_release(struct kref *kref) @@ -722,6 +754,17
> > @@ static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
> >  		n++;
> >  	}
> >
> > +	pm_runtime_enable(device);
> > +
> > +	pm_runtime_set_autosuspend_delay(device,
> INTEL_DG_NVM_RPM_TIMEOUT);
> > +	pm_runtime_use_autosuspend(device);
> 
> something looks strange here...
> on the functions above you get and put references for the parent device
> directly.
> But here you enable the rpm for this device.
> 
> If I remember correctly from some old experiments, the rpm is smart enough
> and wake up the parent before waking up the child. So I'm wondering if we
> should only do the child without the parent.
Agree parent can't runtime suspend ind if it has active child.
Let this driver have it's own get/put routine instead of parent.
Thanks,
Anshuman Gupta.
> 
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> 
> > +
> > +	ret = pm_runtime_resume_and_get(device);
> > +	if (ret < 0) {
> > +		dev_err(device, "rpm: get failed %d\n", ret);
> > +		goto err_norpm;
> > +	}
> > +
> >  	nvm->base = devm_ioremap_resource(device, &invm->bar);
> >  	if (IS_ERR(nvm->base)) {
> >  		dev_err(device, "mmio not mapped\n"); @@ -744,9 +787,13
> @@ static
> > int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
> >
> >  	dev_set_drvdata(&aux_dev->dev, nvm);
> >
> > +	pm_runtime_put(device);
> >  	return 0;
> >
> >  err:
> > +	pm_runtime_put(device);
> > +err_norpm:
> > +	pm_runtime_disable(device);
> >  	kref_put(&nvm->refcnt, intel_dg_nvm_release);
> >  	return ret;
> >  }
> > @@ -758,6 +805,8 @@ static void intel_dg_mtd_remove(struct
> auxiliary_device *aux_dev)
> >  	if (!nvm)
> >  		return;
> >
> > +	pm_runtime_disable(&aux_dev->dev);
> > +
> >  	mtd_device_unregister(&nvm->mtd);
> >
> >  	dev_set_drvdata(&aux_dev->dev, NULL);
> > --
> > 2.43.0
> >

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [PATCH 06/10] mtd: intel-dg: wake card on operations

[Usyskin, Alexander]

> -----Original Message-----
> From: Gupta, Anshuman <anshuman.gupta@intel.com>
> Sent: Monday, October 28, 2024 5:02 PM
> To: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Usyskin, Alexander
> <alexander.usyskin@intel.com>; Deak, Imre <imre.deak@intel.com>
> Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Richard Weinberger
> <richard@nod.at>; Vignesh Raghavendra <vigneshr@ti.com>; De Marchi,
> Lucas <lucas.demarchi@intel.com>; Thomas Hellström
> <thomas.hellstrom@linux.intel.com>; Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>;
> Thomas Zimmermann <tzimmermann@suse.de>; David Airlie
> <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> <jani.nikula@linux.intel.com>; Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin <tursulin@ursulin.net>;
> Weil, Oren jer <oren.jer.weil@intel.com>; linux-mtd@lists.infradead.org; dri-
> devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; linux-
> kernel@vger.kernel.org
> Subject: RE: [PATCH 06/10] mtd: intel-dg: wake card on operations
> 
> 
> 
> > -----Original Message-----
> > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Sent: Monday, October 28, 2024 8:27 PM
> > To: Usyskin, Alexander <alexander.usyskin@intel.com>; Gupta, Anshuman
> > <anshuman.gupta@intel.com>; Deak, Imre <imre.deak@intel.com>
> > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Richard Weinberger
> > <richard@nod.at>; Vignesh Raghavendra <vigneshr@ti.com>; De Marchi,
> > Lucas <lucas.demarchi@intel.com>; Thomas Hellström
> > <thomas.hellstrom@linux.intel.com>; Maarten Lankhorst
> > <maarten.lankhorst@linux.intel.com>; Maxime Ripard
> <mripard@kernel.org>;
> > Thomas Zimmermann <tzimmermann@suse.de>; David Airlie
> > <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> > <jani.nikula@linux.intel.com>; Joonas Lahtinen
> > <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin <tursulin@ursulin.net>;
> > Weil, Oren jer <oren.jer.weil@intel.com>; linux-mtd@lists.infradead.org; dri-
> > devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH 06/10] mtd: intel-dg: wake card on operations
> >
> > On Tue, Oct 22, 2024 at 01:41:15PM +0300, Alexander Usyskin wrote:
> > > Enable runtime PM in mtd driver to notify graphics driver that whole
> > > card should be kept awake while nvm operations are performed through
> > > this driver.
> > >
> > > CC: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> > > ---
> > >  drivers/mtd/devices/mtd-intel-dg.c | 73
> > > +++++++++++++++++++++++++-----
> > >  1 file changed, 61 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/mtd/devices/mtd-intel-dg.c
> > > b/drivers/mtd/devices/mtd-intel-dg.c
> > > index 4951438e8faf..3d53f35478e8 100644
> > > --- a/drivers/mtd/devices/mtd-intel-dg.c
> > > +++ b/drivers/mtd/devices/mtd-intel-dg.c
> > > @@ -15,11 +15,14 @@
> > >  #include <linux/module.h>
> > >  #include <linux/mtd/mtd.h>
> > >  #include <linux/mtd/partitions.h>
> > > +#include <linux/pm_runtime.h>
> > >  #include <linux/string.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/sizes.h>
> > >  #include <linux/types.h>
> > >
> > > +#define INTEL_DG_NVM_RPM_TIMEOUT 500
> > > +
> > >  struct intel_dg_nvm {
> > >  	struct kref refcnt;
> > >  	struct mtd_info mtd;
> > > @@ -462,6 +465,7 @@ static int intel_dg_mtd_erase(struct mtd_info
> *mtd,
> > struct erase_info *info)
> > >  	loff_t from;
> > >  	size_t len;
> > >  	size_t total_len;
> > > +	int ret = 0;
> > >
> > >  	if (WARN_ON(!nvm))
> > >  		return -EINVAL;
> > > @@ -476,20 +480,28 @@ static int intel_dg_mtd_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;
> > > +	}
> > > +
> > >  	guard(mutex)(&nvm->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;
> > > -			return -ERANGE;
> > > +			ret = -ERANGE;
> > > +			goto out;
> > >  		}
> > >
> > >  		idx = idg_nvm_get_region(nvm, addr);
> > >  		if (idx >= nvm->nregions) {
> > >  			dev_err(&mtd->dev, "out of range");
> > >  			info->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
> > > -			return -ERANGE;
> > > +			ret = -ERANGE;
> > > +			goto out;
> > >  		}
> > >
> > >  		from = addr - nvm->regions[idx].offset; @@ -505,14 +517,18
> > @@
> > > static int intel_dg_mtd_erase(struct mtd_info *mtd, struct erase_info
> *info)
> > >  		if (bytes < 0) {
> > >  			dev_dbg(&mtd->dev, "erase failed with %zd\n",
> > bytes);
> > >  			info->fail_addr += nvm->regions[idx].offset;
> > > -			return bytes;
> > > +			ret = bytes;
> > > +			goto out;
> > >  		}
> > >
> > >  		addr += len;
> > >  		total_len -= len;
> > >  	}
> > >
> > > -	return 0;
> > > +out:
> > > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > > +	return ret;
> > >  }
> > >
> > >  static int intel_dg_mtd_read(struct mtd_info *mtd, loff_t from,
> > > size_t len, @@ -541,17 +557,25 @@ static int intel_dg_mtd_read(struct
> > mtd_info *mtd, loff_t from, size_t len,
> > >  	if (len > nvm->regions[idx].size - from)
> > >  		len = nvm->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;
> > > +	}
> > > +
> > >  	guard(mutex)(&nvm->lock);
> > >
> > >  	ret = idg_read(nvm, region, from, len, buf);
> > >  	if (ret < 0) {
> > >  		dev_dbg(&mtd->dev, "read failed with %zd\n", ret);
> > > -		return ret;
> > > +	} else {
> > > +		*retlen = ret;
> > > +		ret = 0;
> > >  	}
> > >
> > > -	*retlen = ret;
> > > -
> > > -	return 0;
> > > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > > +	return ret;
> > >  }
> > >
> > >  static int intel_dg_mtd_write(struct mtd_info *mtd, loff_t to, size_t
> > > len, @@ -580,17 +604,25 @@ static int intel_dg_mtd_write(struct
> mtd_info
> > *mtd, loff_t to, size_t len,
> > >  	if (len > nvm->regions[idx].size - to)
> > >  		len = nvm->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;
> > > +	}
> > > +
> > >  	guard(mutex)(&nvm->lock);
> > >
> > >  	ret = idg_write(nvm, region, to, len, buf);
> > >  	if (ret < 0) {
> > >  		dev_dbg(&mtd->dev, "write failed with %zd\n", ret);
> > > -		return ret;
> > > +	} else {
> > > +		*retlen = ret;
> > > +		ret = 0;
> > >  	}
> > >
> > > -	*retlen = ret;
> > > -
> > > -	return 0;
> > > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > > +	return ret;
> > >  }
> > >
> > >  static void intel_dg_nvm_release(struct kref *kref) @@ -722,6 +754,17
> > > @@ static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
> > >  		n++;
> > >  	}
> > >
> > > +	pm_runtime_enable(device);
> > > +
> > > +	pm_runtime_set_autosuspend_delay(device,
> > INTEL_DG_NVM_RPM_TIMEOUT);
> > > +	pm_runtime_use_autosuspend(device);
> >
> > something looks strange here...
> > on the functions above you get and put references for the parent device
> > directly.
> > But here you enable the rpm for this device.
> >
> > If I remember correctly from some old experiments, the rpm is smart enough
> > and wake up the parent before waking up the child. So I'm wondering if we
> > should only do the child without the parent.
> Agree parent can't runtime suspend ind if it has active child.
> Let this driver have it's own get/put routine instead of parent.
> Thanks,
> Anshuman Gupta.

I need to wake DG card before the MTD device is established to scan the partition table on flash.
Thus, if I move rpm down to MTD device I should enable and take parent (auxiliary device) rpm anyway.
Considering above, is this move is justified?
Also, MTD drivers tend to enable parent rpm, not its own one.

- - 
Thanks,
Sasha



> >
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> >
> > > +
> > > +	ret = pm_runtime_resume_and_get(device);
> > > +	if (ret < 0) {
> > > +		dev_err(device, "rpm: get failed %d\n", ret);
> > > +		goto err_norpm;
> > > +	}
> > > +
> > >  	nvm->base = devm_ioremap_resource(device, &invm->bar);
> > >  	if (IS_ERR(nvm->base)) {
> > >  		dev_err(device, "mmio not mapped\n"); @@ -744,9 +787,13
> > @@ static
> > > int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
> > >
> > >  	dev_set_drvdata(&aux_dev->dev, nvm);
> > >
> > > +	pm_runtime_put(device);
> > >  	return 0;
> > >
> > >  err:
> > > +	pm_runtime_put(device);
> > > +err_norpm:
> > > +	pm_runtime_disable(device);
> > >  	kref_put(&nvm->refcnt, intel_dg_nvm_release);
> > >  	return ret;
> > >  }
> > > @@ -758,6 +805,8 @@ static void intel_dg_mtd_remove(struct
> > auxiliary_device *aux_dev)
> > >  	if (!nvm)
> > >  		return;
> > >
> > > +	pm_runtime_disable(&aux_dev->dev);
> > > +
> > >  	mtd_device_unregister(&nvm->mtd);
> > >
> > >  	dev_set_drvdata(&aux_dev->dev, NULL);
> > > --
> > > 2.43.0
> > >
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 06/10] mtd: intel-dg: wake card on operations

[Rodrigo Vivi]

On Tue, Oct 29, 2024 at 11:24:36AM +0000, Usyskin, Alexander wrote:
> > -----Original Message-----
> > From: Gupta, Anshuman <anshuman.gupta@intel.com>
> > Sent: Monday, October 28, 2024 5:02 PM
> > To: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Usyskin, Alexander
> > <alexander.usyskin@intel.com>; Deak, Imre <imre.deak@intel.com>
> > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Richard Weinberger
> > <richard@nod.at>; Vignesh Raghavendra <vigneshr@ti.com>; De Marchi,
> > Lucas <lucas.demarchi@intel.com>; Thomas Hellström
> > <thomas.hellstrom@linux.intel.com>; Maarten Lankhorst
> > <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>;
> > Thomas Zimmermann <tzimmermann@suse.de>; David Airlie
> > <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> > <jani.nikula@linux.intel.com>; Joonas Lahtinen
> > <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin <tursulin@ursulin.net>;
> > Weil, Oren jer <oren.jer.weil@intel.com>; linux-mtd@lists.infradead.org; dri-
> > devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; linux-
> > kernel@vger.kernel.org
> > Subject: RE: [PATCH 06/10] mtd: intel-dg: wake card on operations
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > > Sent: Monday, October 28, 2024 8:27 PM
> > > To: Usyskin, Alexander <alexander.usyskin@intel.com>; Gupta, Anshuman
> > > <anshuman.gupta@intel.com>; Deak, Imre <imre.deak@intel.com>
> > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Richard Weinberger
> > > <richard@nod.at>; Vignesh Raghavendra <vigneshr@ti.com>; De Marchi,
> > > Lucas <lucas.demarchi@intel.com>; Thomas Hellström
> > > <thomas.hellstrom@linux.intel.com>; Maarten Lankhorst
> > > <maarten.lankhorst@linux.intel.com>; Maxime Ripard
> > <mripard@kernel.org>;
> > > Thomas Zimmermann <tzimmermann@suse.de>; David Airlie
> > > <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> > > <jani.nikula@linux.intel.com>; Joonas Lahtinen
> > > <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin <tursulin@ursulin.net>;
> > > Weil, Oren jer <oren.jer.weil@intel.com>; linux-mtd@lists.infradead.org; dri-
> > > devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; linux-
> > > kernel@vger.kernel.org
> > > Subject: Re: [PATCH 06/10] mtd: intel-dg: wake card on operations
> > >
> > > On Tue, Oct 22, 2024 at 01:41:15PM +0300, Alexander Usyskin wrote:
> > > > Enable runtime PM in mtd driver to notify graphics driver that whole
> > > > card should be kept awake while nvm operations are performed through
> > > > this driver.
> > > >
> > > > CC: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> > > > ---
> > > >  drivers/mtd/devices/mtd-intel-dg.c | 73
> > > > +++++++++++++++++++++++++-----
> > > >  1 file changed, 61 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/mtd/devices/mtd-intel-dg.c
> > > > b/drivers/mtd/devices/mtd-intel-dg.c
> > > > index 4951438e8faf..3d53f35478e8 100644
> > > > --- a/drivers/mtd/devices/mtd-intel-dg.c
> > > > +++ b/drivers/mtd/devices/mtd-intel-dg.c
> > > > @@ -15,11 +15,14 @@
> > > >  #include <linux/module.h>
> > > >  #include <linux/mtd/mtd.h>
> > > >  #include <linux/mtd/partitions.h>
> > > > +#include <linux/pm_runtime.h>
> > > >  #include <linux/string.h>
> > > >  #include <linux/slab.h>
> > > >  #include <linux/sizes.h>
> > > >  #include <linux/types.h>
> > > >
> > > > +#define INTEL_DG_NVM_RPM_TIMEOUT 500
> > > > +
> > > >  struct intel_dg_nvm {
> > > >  	struct kref refcnt;
> > > >  	struct mtd_info mtd;
> > > > @@ -462,6 +465,7 @@ static int intel_dg_mtd_erase(struct mtd_info
> > *mtd,
> > > struct erase_info *info)
> > > >  	loff_t from;
> > > >  	size_t len;
> > > >  	size_t total_len;
> > > > +	int ret = 0;
> > > >
> > > >  	if (WARN_ON(!nvm))
> > > >  		return -EINVAL;
> > > > @@ -476,20 +480,28 @@ static int intel_dg_mtd_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;
> > > > +	}
> > > > +
> > > >  	guard(mutex)(&nvm->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;
> > > > -			return -ERANGE;
> > > > +			ret = -ERANGE;
> > > > +			goto out;
> > > >  		}
> > > >
> > > >  		idx = idg_nvm_get_region(nvm, addr);
> > > >  		if (idx >= nvm->nregions) {
> > > >  			dev_err(&mtd->dev, "out of range");
> > > >  			info->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
> > > > -			return -ERANGE;
> > > > +			ret = -ERANGE;
> > > > +			goto out;
> > > >  		}
> > > >
> > > >  		from = addr - nvm->regions[idx].offset; @@ -505,14 +517,18
> > > @@
> > > > static int intel_dg_mtd_erase(struct mtd_info *mtd, struct erase_info
> > *info)
> > > >  		if (bytes < 0) {
> > > >  			dev_dbg(&mtd->dev, "erase failed with %zd\n",
> > > bytes);
> > > >  			info->fail_addr += nvm->regions[idx].offset;
> > > > -			return bytes;
> > > > +			ret = bytes;
> > > > +			goto out;
> > > >  		}
> > > >
> > > >  		addr += len;
> > > >  		total_len -= len;
> > > >  	}
> > > >
> > > > -	return 0;
> > > > +out:
> > > > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > > > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > > > +	return ret;
> > > >  }
> > > >
> > > >  static int intel_dg_mtd_read(struct mtd_info *mtd, loff_t from,
> > > > size_t len, @@ -541,17 +557,25 @@ static int intel_dg_mtd_read(struct
> > > mtd_info *mtd, loff_t from, size_t len,
> > > >  	if (len > nvm->regions[idx].size - from)
> > > >  		len = nvm->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;
> > > > +	}
> > > > +
> > > >  	guard(mutex)(&nvm->lock);
> > > >
> > > >  	ret = idg_read(nvm, region, from, len, buf);
> > > >  	if (ret < 0) {
> > > >  		dev_dbg(&mtd->dev, "read failed with %zd\n", ret);
> > > > -		return ret;
> > > > +	} else {
> > > > +		*retlen = ret;
> > > > +		ret = 0;
> > > >  	}
> > > >
> > > > -	*retlen = ret;
> > > > -
> > > > -	return 0;
> > > > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > > > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > > > +	return ret;
> > > >  }
> > > >
> > > >  static int intel_dg_mtd_write(struct mtd_info *mtd, loff_t to, size_t
> > > > len, @@ -580,17 +604,25 @@ static int intel_dg_mtd_write(struct
> > mtd_info
> > > *mtd, loff_t to, size_t len,
> > > >  	if (len > nvm->regions[idx].size - to)
> > > >  		len = nvm->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;
> > > > +	}
> > > > +
> > > >  	guard(mutex)(&nvm->lock);
> > > >
> > > >  	ret = idg_write(nvm, region, to, len, buf);
> > > >  	if (ret < 0) {
> > > >  		dev_dbg(&mtd->dev, "write failed with %zd\n", ret);
> > > > -		return ret;
> > > > +	} else {
> > > > +		*retlen = ret;
> > > > +		ret = 0;
> > > >  	}
> > > >
> > > > -	*retlen = ret;
> > > > -
> > > > -	return 0;
> > > > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > > > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > > > +	return ret;
> > > >  }
> > > >
> > > >  static void intel_dg_nvm_release(struct kref *kref) @@ -722,6 +754,17
> > > > @@ static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
> > > >  		n++;
> > > >  	}
> > > >
> > > > +	pm_runtime_enable(device);
> > > > +
> > > > +	pm_runtime_set_autosuspend_delay(device,
> > > INTEL_DG_NVM_RPM_TIMEOUT);
> > > > +	pm_runtime_use_autosuspend(device);
> > >
> > > something looks strange here...
> > > on the functions above you get and put references for the parent device
> > > directly.
> > > But here you enable the rpm for this device.
> > >
> > > If I remember correctly from some old experiments, the rpm is smart enough
> > > and wake up the parent before waking up the child. So I'm wondering if we
> > > should only do the child without the parent.
> > Agree parent can't runtime suspend ind if it has active child.
> > Let this driver have it's own get/put routine instead of parent.
> > Thanks,
> > Anshuman Gupta.
> 
> I need to wake DG card before the MTD device is established to scan the partition table on flash.
> Thus, if I move rpm down to MTD device I should enable and take parent (auxiliary device) rpm anyway.

That's the part that I'm not sure if I agree. if I remember from some experiments in the past,
when you call to wake up the child, the parent will wakeup first anyway.

> Considering above, is this move is justified?
> Also, MTD drivers tend to enable parent rpm, not its own one.

What other drivers are you looking for reference?

> 
> - - 
> Thanks,
> Sasha
> 
> 
> 
> > >
> > > Cc: Imre Deak <imre.deak@intel.com>
> > > Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> > >
> > > > +
> > > > +	ret = pm_runtime_resume_and_get(device);
> > > > +	if (ret < 0) {
> > > > +		dev_err(device, "rpm: get failed %d\n", ret);
> > > > +		goto err_norpm;
> > > > +	}
> > > > +
> > > >  	nvm->base = devm_ioremap_resource(device, &invm->bar);
> > > >  	if (IS_ERR(nvm->base)) {
> > > >  		dev_err(device, "mmio not mapped\n"); @@ -744,9 +787,13
> > > @@ static
> > > > int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
> > > >
> > > >  	dev_set_drvdata(&aux_dev->dev, nvm);
> > > >
> > > > +	pm_runtime_put(device);
> > > >  	return 0;
> > > >
> > > >  err:
> > > > +	pm_runtime_put(device);
> > > > +err_norpm:
> > > > +	pm_runtime_disable(device);
> > > >  	kref_put(&nvm->refcnt, intel_dg_nvm_release);
> > > >  	return ret;
> > > >  }
> > > > @@ -758,6 +805,8 @@ static void intel_dg_mtd_remove(struct
> > > auxiliary_device *aux_dev)
> > > >  	if (!nvm)
> > > >  		return;
> > > >
> > > > +	pm_runtime_disable(&aux_dev->dev);
> > > > +
> > > >  	mtd_device_unregister(&nvm->mtd);
> > > >
> > > >  	dev_set_drvdata(&aux_dev->dev, NULL);
> > > > --
> > > > 2.43.0
> > > >

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 09/10] drm/xe/nvm: add on-die non-volatile memory device

[Rodrigo Vivi]

On Mon, Oct 28, 2024 at 10:48:48AM -0400, Rodrigo Vivi wrote:
> On Tue, Oct 22, 2024 at 01:41:18PM +0300, Alexander Usyskin wrote:
> > Enable access to internal non-volatile memory on DGFX
> > with GSC/CSC devices via a child device.
> > The nvm child device is exposed via auxiliary bus.
> 
> I looked at all of the i915 and xe patches here and everything looks right.
> Just a few common doubts before I put my rv-b here below...
> 
> 
> Starting with the one from the other patch. Could you please share some doc
> where I could confirm
> HECI_FW_STATUS_2_NVM_ACCESS_MODE bit?
> 
> more below...
> 
> > 
> > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> > ---
> >  drivers/gpu/drm/xe/Makefile          |   1 +
> >  drivers/gpu/drm/xe/xe_device.c       |   3 +
> >  drivers/gpu/drm/xe/xe_device_types.h |   8 +++
> >  drivers/gpu/drm/xe/xe_nvm.c          | 100 +++++++++++++++++++++++++++
> >  drivers/gpu/drm/xe/xe_nvm.h          |  15 ++++
> >  drivers/gpu/drm/xe/xe_pci.c          |   5 ++
> >  6 files changed, 132 insertions(+)
> >  create mode 100644 drivers/gpu/drm/xe/xe_nvm.c
> >  create mode 100644 drivers/gpu/drm/xe/xe_nvm.h
> > 
> > diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
> > index cb6c625bdef0..4225a654a937 100644
> > --- a/drivers/gpu/drm/xe/Makefile
> > +++ b/drivers/gpu/drm/xe/Makefile
> > @@ -94,6 +94,7 @@ xe-y += xe_bb.o \
> >  	xe_ring_ops.o \
> >  	xe_sa.o \
> >  	xe_sched_job.o \
> > +	xe_nvm.o \
> >  	xe_step.o \
> >  	xe_sync.o \
> >  	xe_tile.o \
> > diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> > index 962751c966d1..844697f79eee 100644
> > --- a/drivers/gpu/drm/xe/xe_device.c
> > +++ b/drivers/gpu/drm/xe/xe_device.c
> > @@ -49,6 +49,7 @@
> >  #include "xe_pcode.h"
> >  #include "xe_pm.h"
> >  #include "xe_query.h"
> > +#include "xe_nvm.h"
> >  #include "xe_sriov.h"
> >  #include "xe_tile.h"
> >  #include "xe_ttm_stolen_mgr.h"
> > @@ -743,6 +744,7 @@ int xe_device_probe(struct xe_device *xe)
> >  			goto err_fini_gt;
> >  	}
> >  
> > +	xe_nvm_init(xe);
> >  	xe_heci_gsc_init(xe);
> >  
> >  	err = xe_oa_init(xe);
> > @@ -811,6 +813,7 @@ void xe_device_remove(struct xe_device *xe)
> >  	xe_oa_fini(xe);
> >  
> >  	xe_heci_gsc_fini(xe);
> > +	xe_nvm_fini(xe);
> >  
> >  	for_each_gt(gt, xe, id)
> >  		xe_gt_remove(gt);
> > diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> > index 85bede4dd646..ec3d82f05519 100644
> > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > @@ -35,6 +35,8 @@
> >  struct xe_ggtt;
> >  struct xe_pat_ops;
> >  
> > +struct intel_dg_nvm_dev;
> > +
> >  #define XE_BO_INVALID_OFFSET	LONG_MAX
> >  
> >  #define GRAPHICS_VER(xe) ((xe)->info.graphics_verx100 / 100)
> > @@ -44,6 +46,7 @@ struct xe_pat_ops;
> >  #define IS_DGFX(xe) ((xe)->info.is_dgfx)
> >  #define HAS_HECI_GSCFI(xe) ((xe)->info.has_heci_gscfi)
> >  #define HAS_HECI_CSCFI(xe) ((xe)->info.has_heci_cscfi)
> > +#define HAS_GSC_NVM(xe) ((xe)->info.has_gsc_nvm)
> >  
> >  #define XE_VRAM_FLAGS_NEED64K		BIT(0)
> >  
> > @@ -331,6 +334,8 @@ struct xe_device {
> >  		u8 has_heci_gscfi:1;
> >  		/** @info.has_heci_cscfi: device has heci cscfi */
> >  		u8 has_heci_cscfi:1;
> > +		/** @info.has_gsc_nvm: device has gsc non-volatile memory */
> > +		u8 has_gsc_nvm:1;
> >  		/** @info.skip_guc_pc: Skip GuC based PM feature init */
> >  		u8 skip_guc_pc:1;
> >  		/** @info.has_atomic_enable_pte_bit: Device has atomic enable PTE bit */
> > @@ -502,6 +507,9 @@ struct xe_device {
> >  	/** @heci_gsc: graphics security controller */
> >  	struct xe_heci_gsc heci_gsc;
> >  
> > +	/** @nvm: discrete graphics non-volatile memory */
> > +	struct intel_dg_nvm_dev *nvm;
> > +
> >  	/** @oa: oa observation subsystem */
> >  	struct xe_oa oa;
> >  
> > diff --git a/drivers/gpu/drm/xe/xe_nvm.c b/drivers/gpu/drm/xe/xe_nvm.c
> > new file mode 100644
> > index 000000000000..ce56bff1268b
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xe/xe_nvm.c
> > @@ -0,0 +1,100 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright(c) 2019-2024, Intel Corporation. All rights reserved.
> > + */
> > +
> > +#include <linux/intel_dg_nvm_aux.h>
> > +#include <linux/pci.h>
> > +#include "xe_device_types.h"
> > +#include "xe_nvm.h"
> > +#include "xe_sriov.h"
> > +
> > +#define GEN12_GUNIT_NVM_BASE 0x00102040
> > +#define GEN12_GUNIT_NVM_SIZE 0x80
> > +#define HECI_FW_STATUS_2_NVM_ACCESS_MODE BIT(3)
> > +
> > +static const struct intel_dg_nvm_region regions[INTEL_DG_NVM_REGIONS] = {
> > +	[0] = { .name = "DESCRIPTOR", },
> > +	[2] = { .name = "GSC", },
> > +	[11] = { .name = "OptionROM", },
> > +	[12] = { .name = "DAM", },
> 
> Could you please give some pointers to confirm this base and these regions?
> 
> > +};
> > +
> > +static void xe_nvm_release_dev(struct device *dev)
> > +{
> > +}
> > +
> > +void xe_nvm_init(struct xe_device *xe)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
> > +	struct intel_dg_nvm_dev *nvm;
> > +	struct auxiliary_device *aux_dev;
> > +	int ret;
> > +
> > +	if (!HAS_GSC_NVM(xe))
> > +		return;
> > +
> > +	/* No access to internal NVM from VFs */
> > +	if (IS_SRIOV_VF(xe))
> > +		return;
> > +
> > +	/* Nvm pointer should be NULL here */
> > +	if (WARN_ON(xe->nvm))
> > +		return;
> > +
> > +	xe->nvm = kzalloc(sizeof(*nvm), GFP_KERNEL);
> > +	if (!xe->nvm)
> > +		return;
> > +
> > +	nvm = xe->nvm;
> > +
> > +	nvm->writeable_override = false;
> > +	nvm->bar.parent = &pdev->resource[0];
> > +	nvm->bar.start = GEN12_GUNIT_NVM_BASE + pdev->resource[0].start;
> > +	nvm->bar.end = nvm->bar.start + GEN12_GUNIT_NVM_SIZE - 1;
> > +	nvm->bar.flags = IORESOURCE_MEM;
> > +	nvm->bar.desc = IORES_DESC_NONE;
> > +	nvm->regions = regions;
> > +
> > +	aux_dev = &nvm->aux_dev;
> > +
> > +	aux_dev->name = "nvm";
> > +	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 = xe_nvm_release_dev;
> > +
> > +	ret = auxiliary_device_init(aux_dev);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "xe-nvm aux init failed %d\n", ret);
> 
> Since these are inside the i915 and xe and you have our drm private device,
> I believe it would be better if we would use the drm_err and other drm debug
> variants here, below and also in the i915 patch.

Thanks for the confirmation of the offsets and regions.
With these dev_err changed towards the drm_err, feel free to use

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> Thank you so much,
> Rodrigo.
> 
> > +		return;
> > +	}
> > +
> > +	ret = auxiliary_device_add(aux_dev);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "xe-nvm aux add failed %d\n", ret);
> > +		auxiliary_device_uninit(aux_dev);
> > +		return;
> > +	}
> > +}
> > +
> > +void xe_nvm_fini(struct xe_device *xe)
> > +{
> > +	struct intel_dg_nvm_dev *nvm = xe->nvm;
> > +
> > +	if (!HAS_GSC_NVM(xe))
> > +		return;
> > +
> > +	/* No access to internal NVM from VFs */
> > +	if (IS_SRIOV_VF(xe))
> > +		return;
> > +
> > +	/* Nvm pointer should not be NULL here */
> > +	if (WARN_ON(!nvm))
> > +		return;
> > +
> > +	auxiliary_device_delete(&nvm->aux_dev);
> > +	auxiliary_device_uninit(&nvm->aux_dev);
> > +	kfree(nvm);
> > +	xe->nvm = NULL;
> > +}
> > diff --git a/drivers/gpu/drm/xe/xe_nvm.h b/drivers/gpu/drm/xe/xe_nvm.h
> > new file mode 100644
> > index 000000000000..068695447913
> > --- /dev/null
> > +++ b/drivers/gpu/drm/xe/xe_nvm.h
> > @@ -0,0 +1,15 @@
> > +/* SPDX-License-Identifier: MIT */
> > +/*
> > + * Copyright(c) 2019-2024 Intel Corporation. All rights reserved.
> > + */
> > +
> > +#ifndef __XE_NVM_H__
> > +#define __XE_NVM_H__
> > +
> > +struct xe_device;
> > +
> > +void xe_nvm_init(struct xe_device *xe);
> > +
> > +void xe_nvm_fini(struct xe_device *xe);
> > +
> > +#endif /* __XE_NVM_H__ */
> > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> > index 64a8336ca437..85c419eea710 100644
> > --- a/drivers/gpu/drm/xe/xe_pci.c
> > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > @@ -60,6 +60,7 @@ struct xe_device_desc {
> >  	u8 has_display:1;
> >  	u8 has_heci_gscfi:1;
> >  	u8 has_heci_cscfi:1;
> > +	u8 has_gsc_nvm:1;
> >  	u8 has_llc:1;
> >  	u8 has_mmio_ext:1;
> >  	u8 has_sriov:1;
> > @@ -282,6 +283,7 @@ static const struct xe_device_desc dg1_desc = {
> >  	PLATFORM(DG1),
> >  	.has_display = true,
> >  	.has_heci_gscfi = 1,
> > +	.has_gsc_nvm = 1,
> >  	.require_force_probe = true,
> >  };
> >  
> > @@ -293,6 +295,7 @@ static const u16 dg2_g12_ids[] = { XE_DG2_G12_IDS(NOP), 0 };
> >  	DGFX_FEATURES, \
> >  	PLATFORM(DG2), \
> >  	.has_heci_gscfi = 1, \
> > +	.has_gsc_nvm = 1, \
> >  	.subplatforms = (const struct xe_subplatform_desc[]) { \
> >  		{ XE_SUBPLATFORM_DG2_G10, "G10", dg2_g10_ids }, \
> >  		{ XE_SUBPLATFORM_DG2_G11, "G11", dg2_g11_ids }, \
> > @@ -324,6 +327,7 @@ static const __maybe_unused struct xe_device_desc pvc_desc = {
> >  	PLATFORM(PVC),
> >  	.has_display = false,
> >  	.has_heci_gscfi = 1,
> > +	.has_gsc_nvm = 1,
> >  	.require_force_probe = true,
> >  };
> >  
> > @@ -623,6 +627,7 @@ static int xe_info_init_early(struct xe_device *xe,
> >  	xe->info.is_dgfx = desc->is_dgfx;
> >  	xe->info.has_heci_gscfi = desc->has_heci_gscfi;
> >  	xe->info.has_heci_cscfi = desc->has_heci_cscfi;
> > +	xe->info.has_gsc_nvm = desc->has_gsc_nvm;
> >  	xe->info.has_llc = desc->has_llc;
> >  	xe->info.has_mmio_ext = desc->has_mmio_ext;
> >  	xe->info.has_sriov = desc->has_sriov;
> > -- 
> > 2.43.0
> > 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 01/10] mtd: add driver for intel graphics non-volatile memory device

[Rodrigo Vivi]

On Tue, Oct 22, 2024 at 01:41:10PM +0300, Alexander Usyskin wrote:
> Add auxiliary driver for intel discrete graphics
> non-volatile memory device.
> 
> CC: Rodrigo Vivi <rodrigo.vivi@intel.com>
> CC: Lucas De Marchi <lucas.demarchi@intel.com>
> Co-developed-by: Tomas Winkler <tomasw@gmail.com>
> Signed-off-by: Tomas Winkler <tomasw@gmail.com>
> Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  MAINTAINERS                        |   7 ++
>  drivers/mtd/devices/Kconfig        |  11 +++
>  drivers/mtd/devices/Makefile       |   1 +
>  drivers/mtd/devices/mtd-intel-dg.c | 139 +++++++++++++++++++++++++++++
>  include/linux/intel_dg_nvm_aux.h   |  27 ++++++
>  5 files changed, 185 insertions(+)
>  create mode 100644 drivers/mtd/devices/mtd-intel-dg.c
>  create mode 100644 include/linux/intel_dg_nvm_aux.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c27f3190737f..a09c035849ef 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11360,6 +11360,13 @@ L:	linux-kernel@vger.kernel.org
>  S:	Supported
>  F:	arch/x86/include/asm/intel-family.h
>  
> +INTEL DISCRETE GRAPHIC NVM MTD DRIVER
> +M:	Alexander Usyskin <alexander.usyskin@intel.com>
> +L:	linux-mtd@lists.infradead.org
> +S:	Supported
> +F:	drivers/mtd/devices/mtd-intel-dg.c
> +F:	include/linux/intel_dg_nvm_aux.h
> +
>  INTEL DRM DISPLAY FOR XE AND I915 DRIVERS
>  M:	Jani Nikula <jani.nikula@linux.intel.com>
>  M:	Rodrigo Vivi <rodrigo.vivi@intel.com>
> diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
> index ff2f9e55ef28..d93edf45c0bb 100644
> --- a/drivers/mtd/devices/Kconfig
> +++ b/drivers/mtd/devices/Kconfig
> @@ -183,6 +183,17 @@ config MTD_POWERNV_FLASH
>  	  platforms from Linux. This device abstracts away the
>  	  firmware interface for flash access.
>  
> +config MTD_INTEL_DG
> +	tristate "Intel Discrete Graphic non-volatile memory driver"
> +	depends on AUXILIARY_BUS
> +	depends on MTD
> +	help
> +	  This provides MTD device to access Intel Discrete Graphic
> +	  non-volatile memory.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called mtd-intel-dg.
> +
>  comment "Disk-On-Chip Device Drivers"
>  
>  config MTD_DOCG3
> diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile
> index d11eb2b8b6f8..77c05d269034 100644
> --- a/drivers/mtd/devices/Makefile
> +++ b/drivers/mtd/devices/Makefile
> @@ -18,6 +18,7 @@ obj-$(CONFIG_MTD_SST25L)	+= sst25l.o
>  obj-$(CONFIG_MTD_BCM47XXSFLASH)	+= bcm47xxsflash.o
>  obj-$(CONFIG_MTD_ST_SPI_FSM)    += st_spi_fsm.o
>  obj-$(CONFIG_MTD_POWERNV_FLASH)	+= powernv_flash.o
> +obj-$(CONFIG_MTD_INTEL_DG)	+= mtd-intel-dg.o
>  
>  
>  CFLAGS_docg3.o			+= -I$(src)
> diff --git a/drivers/mtd/devices/mtd-intel-dg.c b/drivers/mtd/devices/mtd-intel-dg.c
> new file mode 100644
> index 000000000000..746c963ea540
> --- /dev/null
> +++ b/drivers/mtd/devices/mtd-intel-dg.c
> @@ -0,0 +1,139 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright(c) 2019-2024, Intel Corporation. All rights reserved.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/intel_dg_nvm_aux.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/string.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +struct intel_dg_nvm {
> +	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 intel_dg_nvm_release(struct kref *kref)
> +{
> +	struct intel_dg_nvm *nvm = container_of(kref, struct intel_dg_nvm, refcnt);
> +	int i;
> +
> +	pr_debug("freeing intel_dg nvm\n");
> +	for (i = 0; i < nvm->nregions; i++)
> +		kfree(nvm->regions[i].name);
> +	kfree(nvm);
> +}
> +
> +static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
> +			      const struct auxiliary_device_id *aux_dev_id)
> +{
> +	struct intel_dg_nvm_dev *invm = auxiliary_dev_to_intel_dg_nvm_dev(aux_dev);
> +	struct device *device;
> +	struct intel_dg_nvm *nvm;
> +	unsigned int nregions;
> +	unsigned int i, n;
> +	size_t size;
> +	char *name;
> +	int ret;
> +
> +	device = &aux_dev->dev;
> +
> +	/* count available regions */
> +	for (nregions = 0, i = 0; i < INTEL_DG_NVM_REGIONS; i++) {
> +		if (invm->regions[i].name)
> +			nregions++;
> +	}
> +
> +	if (!nregions) {
> +		dev_err(device, "no regions defined\n");
> +		return -ENODEV;
> +	}
> +
> +	size = sizeof(*nvm) + sizeof(nvm->regions[0]) * nregions;
> +	nvm = kzalloc(size, GFP_KERNEL);
> +	if (!nvm)
> +		return -ENOMEM;
> +
> +	kref_init(&nvm->refcnt);
> +
> +	nvm->nregions = nregions;
> +	for (n = 0, i = 0; i < INTEL_DG_NVM_REGIONS; i++) {
> +		if (!invm->regions[i].name)
> +			continue;
> +
> +		name = kasprintf(GFP_KERNEL, "%s.%s",
> +				 dev_name(&aux_dev->dev), invm->regions[i].name);
> +		if (!name)
> +			continue;
> +		nvm->regions[n].name = name;
> +		nvm->regions[n].id = i;
> +		n++;
> +	}
> +
> +	nvm->base = devm_ioremap_resource(device, &invm->bar);
> +	if (IS_ERR(nvm->base)) {
> +		dev_err(device, "mmio not mapped\n");
> +		ret = PTR_ERR(nvm->base);
> +		goto err;
> +	}
> +
> +	dev_set_drvdata(&aux_dev->dev, nvm);
> +
> +	return 0;
> +
> +err:
> +	kref_put(&nvm->refcnt, intel_dg_nvm_release);
> +	return ret;
> +}
> +
> +static void intel_dg_mtd_remove(struct auxiliary_device *aux_dev)
> +{
> +	struct intel_dg_nvm *nvm = dev_get_drvdata(&aux_dev->dev);
> +
> +	if (!nvm)
> +		return;
> +
> +	dev_set_drvdata(&aux_dev->dev, NULL);
> +
> +	kref_put(&nvm->refcnt, intel_dg_nvm_release);
> +}
> +
> +static const struct auxiliary_device_id intel_dg_mtd_id_table[] = {
> +	{
> +		.name = "i915.nvm",
> +	},
> +	{
> +		.name = "xe.nvm",
> +	},
> +	{
> +		/* sentinel */
> +	}
> +};
> +MODULE_DEVICE_TABLE(auxiliary, intel_dg_mtd_id_table);
> +
> +static struct auxiliary_driver intel_dg_mtd_driver = {
> +	.probe  = intel_dg_mtd_probe,
> +	.remove = intel_dg_mtd_remove,
> +	.driver = {
> +		/* auxiliary_driver_register() sets .name to be the modname */
> +	},
> +	.id_table = intel_dg_mtd_id_table
> +};
> +
> +module_auxiliary_driver(intel_dg_mtd_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_DESCRIPTION("Intel DGFX MTD driver");
> diff --git a/include/linux/intel_dg_nvm_aux.h b/include/linux/intel_dg_nvm_aux.h
> new file mode 100644
> index 000000000000..2cc4179fbde2
> --- /dev/null
> +++ b/include/linux/intel_dg_nvm_aux.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright(c) 2019-2024, Intel Corporation. All rights reserved.
> + */
> +
> +#ifndef __INTEL_DG_NVM_AUX_H__
> +#define __INTEL_DG_NVM_AUX_H__
> +
> +#include <linux/auxiliary_bus.h>
> +
> +#define INTEL_DG_NVM_REGIONS 13
> +
> +struct intel_dg_nvm_region {
> +	const char *name;
> +};
> +
> +struct intel_dg_nvm_dev {
> +	struct auxiliary_device aux_dev;
> +	bool writeable_override;
> +	struct resource bar;
> +	const struct intel_dg_nvm_region *regions;
> +};
> +
> +#define auxiliary_dev_to_intel_dg_nvm_dev(auxiliary_dev) \
> +	container_of(auxiliary_dev, struct intel_dg_nvm_dev, aux_dev)
> +
> +#endif /* __INTEL_DG_NVM_AUX_H__ */
> -- 
> 2.43.0
> 

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [PATCH 06/10] mtd: intel-dg: wake card on operations

[Usyskin, Alexander]

> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Sent: Monday, November 4, 2024 11:16 PM
> To: Usyskin, Alexander <alexander.usyskin@intel.com>
> Cc: Gupta, Anshuman <anshuman.gupta@intel.com>; Deak, Imre
> <imre.deak@intel.com>; Miquel Raynal <miquel.raynal@bootlin.com>;
> Richard Weinberger <richard@nod.at>; Vignesh Raghavendra
> <vigneshr@ti.com>; De Marchi, Lucas <lucas.demarchi@intel.com>; Thomas
> Hellström <thomas.hellstrom@linux.intel.com>; Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>;
> Thomas Zimmermann <tzimmermann@suse.de>; David Airlie
> <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> <jani.nikula@linux.intel.com>; Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin <tursulin@ursulin.net>;
> Weil, Oren jer <oren.jer.weil@intel.com>; linux-mtd@lists.infradead.org; dri-
> devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 06/10] mtd: intel-dg: wake card on operations
> 
> On Tue, Oct 29, 2024 at 11:24:36AM +0000, Usyskin, Alexander wrote:
> > > -----Original Message-----
> > > From: Gupta, Anshuman <anshuman.gupta@intel.com>
> > > Sent: Monday, October 28, 2024 5:02 PM
> > > To: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Usyskin, Alexander
> > > <alexander.usyskin@intel.com>; Deak, Imre <imre.deak@intel.com>
> > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Richard Weinberger
> > > <richard@nod.at>; Vignesh Raghavendra <vigneshr@ti.com>; De Marchi,
> > > Lucas <lucas.demarchi@intel.com>; Thomas Hellström
> > > <thomas.hellstrom@linux.intel.com>; Maarten Lankhorst
> > > <maarten.lankhorst@linux.intel.com>; Maxime Ripard
> <mripard@kernel.org>;
> > > Thomas Zimmermann <tzimmermann@suse.de>; David Airlie
> > > <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> > > <jani.nikula@linux.intel.com>; Joonas Lahtinen
> > > <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin <tursulin@ursulin.net>;
> > > Weil, Oren jer <oren.jer.weil@intel.com>; linux-mtd@lists.infradead.org;
> dri-
> > > devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; linux-
> > > kernel@vger.kernel.org
> > > Subject: RE: [PATCH 06/10] mtd: intel-dg: wake card on operations
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > > > Sent: Monday, October 28, 2024 8:27 PM
> > > > To: Usyskin, Alexander <alexander.usyskin@intel.com>; Gupta,
> Anshuman
> > > > <anshuman.gupta@intel.com>; Deak, Imre <imre.deak@intel.com>
> > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Richard Weinberger
> > > > <richard@nod.at>; Vignesh Raghavendra <vigneshr@ti.com>; De Marchi,
> > > > Lucas <lucas.demarchi@intel.com>; Thomas Hellström
> > > > <thomas.hellstrom@linux.intel.com>; Maarten Lankhorst
> > > > <maarten.lankhorst@linux.intel.com>; Maxime Ripard
> > > <mripard@kernel.org>;
> > > > Thomas Zimmermann <tzimmermann@suse.de>; David Airlie
> > > > <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> > > > <jani.nikula@linux.intel.com>; Joonas Lahtinen
> > > > <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin
> <tursulin@ursulin.net>;
> > > > Weil, Oren jer <oren.jer.weil@intel.com>; linux-mtd@lists.infradead.org;
> dri-
> > > > devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; linux-
> > > > kernel@vger.kernel.org
> > > > Subject: Re: [PATCH 06/10] mtd: intel-dg: wake card on operations
> > > >
> > > > On Tue, Oct 22, 2024 at 01:41:15PM +0300, Alexander Usyskin wrote:
> > > > > Enable runtime PM in mtd driver to notify graphics driver that whole
> > > > > card should be kept awake while nvm operations are performed
> through
> > > > > this driver.
> > > > >
> > > > > CC: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> > > > > ---
> > > > >  drivers/mtd/devices/mtd-intel-dg.c | 73
> > > > > +++++++++++++++++++++++++-----
> > > > >  1 file changed, 61 insertions(+), 12 deletions(-)
> > > > >
> > > > > diff --git a/drivers/mtd/devices/mtd-intel-dg.c
> > > > > b/drivers/mtd/devices/mtd-intel-dg.c
> > > > > index 4951438e8faf..3d53f35478e8 100644
> > > > > --- a/drivers/mtd/devices/mtd-intel-dg.c
> > > > > +++ b/drivers/mtd/devices/mtd-intel-dg.c
> > > > > @@ -15,11 +15,14 @@
> > > > >  #include <linux/module.h>
> > > > >  #include <linux/mtd/mtd.h>
> > > > >  #include <linux/mtd/partitions.h>
> > > > > +#include <linux/pm_runtime.h>
> > > > >  #include <linux/string.h>
> > > > >  #include <linux/slab.h>
> > > > >  #include <linux/sizes.h>
> > > > >  #include <linux/types.h>
> > > > >
> > > > > +#define INTEL_DG_NVM_RPM_TIMEOUT 500
> > > > > +
> > > > >  struct intel_dg_nvm {
> > > > >  	struct kref refcnt;
> > > > >  	struct mtd_info mtd;
> > > > > @@ -462,6 +465,7 @@ static int intel_dg_mtd_erase(struct mtd_info
> > > *mtd,
> > > > struct erase_info *info)
> > > > >  	loff_t from;
> > > > >  	size_t len;
> > > > >  	size_t total_len;
> > > > > +	int ret = 0;
> > > > >
> > > > >  	if (WARN_ON(!nvm))
> > > > >  		return -EINVAL;
> > > > > @@ -476,20 +480,28 @@ static int intel_dg_mtd_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;
> > > > > +	}
> > > > > +
> > > > >  	guard(mutex)(&nvm->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;
> > > > > -			return -ERANGE;
> > > > > +			ret = -ERANGE;
> > > > > +			goto out;
> > > > >  		}
> > > > >
> > > > >  		idx = idg_nvm_get_region(nvm, addr);
> > > > >  		if (idx >= nvm->nregions) {
> > > > >  			dev_err(&mtd->dev, "out of range");
> > > > >  			info->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
> > > > > -			return -ERANGE;
> > > > > +			ret = -ERANGE;
> > > > > +			goto out;
> > > > >  		}
> > > > >
> > > > >  		from = addr - nvm->regions[idx].offset; @@ -505,14 +517,18
> > > > @@
> > > > > static int intel_dg_mtd_erase(struct mtd_info *mtd, struct erase_info
> > > *info)
> > > > >  		if (bytes < 0) {
> > > > >  			dev_dbg(&mtd->dev, "erase failed with %zd\n",
> > > > bytes);
> > > > >  			info->fail_addr += nvm->regions[idx].offset;
> > > > > -			return bytes;
> > > > > +			ret = bytes;
> > > > > +			goto out;
> > > > >  		}
> > > > >
> > > > >  		addr += len;
> > > > >  		total_len -= len;
> > > > >  	}
> > > > >
> > > > > -	return 0;
> > > > > +out:
> > > > > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > > > > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > > > > +	return ret;
> > > > >  }
> > > > >
> > > > >  static int intel_dg_mtd_read(struct mtd_info *mtd, loff_t from,
> > > > > size_t len, @@ -541,17 +557,25 @@ static int
> intel_dg_mtd_read(struct
> > > > mtd_info *mtd, loff_t from, size_t len,
> > > > >  	if (len > nvm->regions[idx].size - from)
> > > > >  		len = nvm->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;
> > > > > +	}
> > > > > +
> > > > >  	guard(mutex)(&nvm->lock);
> > > > >
> > > > >  	ret = idg_read(nvm, region, from, len, buf);
> > > > >  	if (ret < 0) {
> > > > >  		dev_dbg(&mtd->dev, "read failed with %zd\n", ret);
> > > > > -		return ret;
> > > > > +	} else {
> > > > > +		*retlen = ret;
> > > > > +		ret = 0;
> > > > >  	}
> > > > >
> > > > > -	*retlen = ret;
> > > > > -
> > > > > -	return 0;
> > > > > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > > > > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > > > > +	return ret;
> > > > >  }
> > > > >
> > > > >  static int intel_dg_mtd_write(struct mtd_info *mtd, loff_t to, size_t
> > > > > len, @@ -580,17 +604,25 @@ static int intel_dg_mtd_write(struct
> > > mtd_info
> > > > *mtd, loff_t to, size_t len,
> > > > >  	if (len > nvm->regions[idx].size - to)
> > > > >  		len = nvm->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;
> > > > > +	}
> > > > > +
> > > > >  	guard(mutex)(&nvm->lock);
> > > > >
> > > > >  	ret = idg_write(nvm, region, to, len, buf);
> > > > >  	if (ret < 0) {
> > > > >  		dev_dbg(&mtd->dev, "write failed with %zd\n", ret);
> > > > > -		return ret;
> > > > > +	} else {
> > > > > +		*retlen = ret;
> > > > > +		ret = 0;
> > > > >  	}
> > > > >
> > > > > -	*retlen = ret;
> > > > > -
> > > > > -	return 0;
> > > > > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > > > > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > > > > +	return ret;
> > > > >  }
> > > > >
> > > > >  static void intel_dg_nvm_release(struct kref *kref) @@ -722,6
> +754,17
> > > > > @@ static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
> > > > >  		n++;
> > > > >  	}
> > > > >
> > > > > +	pm_runtime_enable(device);
> > > > > +
> > > > > +	pm_runtime_set_autosuspend_delay(device,
> > > > INTEL_DG_NVM_RPM_TIMEOUT);
> > > > > +	pm_runtime_use_autosuspend(device);
> > > >
> > > > something looks strange here...
> > > > on the functions above you get and put references for the parent device
> > > > directly.
> > > > But here you enable the rpm for this device.
> > > >
> > > > If I remember correctly from some old experiments, the rpm is smart
> enough
> > > > and wake up the parent before waking up the child. So I'm wondering if
> we
> > > > should only do the child without the parent.
> > > Agree parent can't runtime suspend ind if it has active child.
> > > Let this driver have it's own get/put routine instead of parent.
> > > Thanks,
> > > Anshuman Gupta.
> >
> > I need to wake DG card before the MTD device is established to scan the
> partition table on flash.
> > Thus, if I move rpm down to MTD device I should enable and take parent
> (auxiliary device) rpm anyway.
> 
> That's the part that I'm not sure if I agree. if I remember from some
> experiments in the past,
> when you call to wake up the child, the parent will wakeup first anyway.
> 
The child (mtd device) does not exist at this point of time.
To create MTD device, the partition table should be provided
and it read directly from flash that should be powered to do read.

> > Considering above, is this move is justified?
> > Also, MTD drivers tend to enable parent rpm, not its own one.
> 
> What other drivers are you looking for reference?

drivers/mtd/nand/raw/tegra_nand.c
drivers/mtd/nand/raw/renesas-nand-controller.c
drivers/mtd/maps/physmap-core.c

> 
> >
> > - -
> > Thanks,
> > Sasha
> >
> >
> >
> > > >
> > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> > > >
> > > > > +
> > > > > +	ret = pm_runtime_resume_and_get(device);
> > > > > +	if (ret < 0) {
> > > > > +		dev_err(device, "rpm: get failed %d\n", ret);
> > > > > +		goto err_norpm;
> > > > > +	}
> > > > > +
> > > > >  	nvm->base = devm_ioremap_resource(device, &invm->bar);
> > > > >  	if (IS_ERR(nvm->base)) {
> > > > >  		dev_err(device, "mmio not mapped\n"); @@ -744,9 +787,13
> > > > @@ static
> > > > > int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
> > > > >
> > > > >  	dev_set_drvdata(&aux_dev->dev, nvm);
> > > > >
> > > > > +	pm_runtime_put(device);
> > > > >  	return 0;
> > > > >
> > > > >  err:
> > > > > +	pm_runtime_put(device);
> > > > > +err_norpm:
> > > > > +	pm_runtime_disable(device);
> > > > >  	kref_put(&nvm->refcnt, intel_dg_nvm_release);
> > > > >  	return ret;
> > > > >  }
> > > > > @@ -758,6 +805,8 @@ static void intel_dg_mtd_remove(struct
> > > > auxiliary_device *aux_dev)
> > > > >  	if (!nvm)
> > > > >  		return;
> > > > >
> > > > > +	pm_runtime_disable(&aux_dev->dev);
> > > > > +
> > > > >  	mtd_device_unregister(&nvm->mtd);
> > > > >
> > > > >  	dev_set_drvdata(&aux_dev->dev, NULL);
> > > > > --
> > > > > 2.43.0
> > > > >
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 06/10] mtd: intel-dg: wake card on operations

[Rodrigo Vivi]

On Tue, Nov 05, 2024 at 07:17:53AM -0500, Usyskin, Alexander wrote:
> > -----Original Message-----
> > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Sent: Monday, November 4, 2024 11:16 PM
> > To: Usyskin, Alexander <alexander.usyskin@intel.com>
> > Cc: Gupta, Anshuman <anshuman.gupta@intel.com>; Deak, Imre
> > <imre.deak@intel.com>; Miquel Raynal <miquel.raynal@bootlin.com>;
> > Richard Weinberger <richard@nod.at>; Vignesh Raghavendra
> > <vigneshr@ti.com>; De Marchi, Lucas <lucas.demarchi@intel.com>; Thomas
> > Hellström <thomas.hellstrom@linux.intel.com>; Maarten Lankhorst
> > <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>;
> > Thomas Zimmermann <tzimmermann@suse.de>; David Airlie
> > <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> > <jani.nikula@linux.intel.com>; Joonas Lahtinen
> > <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin <tursulin@ursulin.net>;
> > Weil, Oren jer <oren.jer.weil@intel.com>; linux-mtd@lists.infradead.org; dri-
> > devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH 06/10] mtd: intel-dg: wake card on operations
> > 
> > On Tue, Oct 29, 2024 at 11:24:36AM +0000, Usyskin, Alexander wrote:
> > > > -----Original Message-----
> > > > From: Gupta, Anshuman <anshuman.gupta@intel.com>
> > > > Sent: Monday, October 28, 2024 5:02 PM
> > > > To: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Usyskin, Alexander
> > > > <alexander.usyskin@intel.com>; Deak, Imre <imre.deak@intel.com>
> > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Richard Weinberger
> > > > <richard@nod.at>; Vignesh Raghavendra <vigneshr@ti.com>; De Marchi,
> > > > Lucas <lucas.demarchi@intel.com>; Thomas Hellström
> > > > <thomas.hellstrom@linux.intel.com>; Maarten Lankhorst
> > > > <maarten.lankhorst@linux.intel.com>; Maxime Ripard
> > <mripard@kernel.org>;
> > > > Thomas Zimmermann <tzimmermann@suse.de>; David Airlie
> > > > <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> > > > <jani.nikula@linux.intel.com>; Joonas Lahtinen
> > > > <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin <tursulin@ursulin.net>;
> > > > Weil, Oren jer <oren.jer.weil@intel.com>; linux-mtd@lists.infradead.org;
> > dri-
> > > > devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; linux-
> > > > kernel@vger.kernel.org
> > > > Subject: RE: [PATCH 06/10] mtd: intel-dg: wake card on operations
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > > > > Sent: Monday, October 28, 2024 8:27 PM
> > > > > To: Usyskin, Alexander <alexander.usyskin@intel.com>; Gupta,
> > Anshuman
> > > > > <anshuman.gupta@intel.com>; Deak, Imre <imre.deak@intel.com>
> > > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Richard Weinberger
> > > > > <richard@nod.at>; Vignesh Raghavendra <vigneshr@ti.com>; De Marchi,
> > > > > Lucas <lucas.demarchi@intel.com>; Thomas Hellström
> > > > > <thomas.hellstrom@linux.intel.com>; Maarten Lankhorst
> > > > > <maarten.lankhorst@linux.intel.com>; Maxime Ripard
> > > > <mripard@kernel.org>;
> > > > > Thomas Zimmermann <tzimmermann@suse.de>; David Airlie
> > > > > <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> > > > > <jani.nikula@linux.intel.com>; Joonas Lahtinen
> > > > > <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin
> > <tursulin@ursulin.net>;
> > > > > Weil, Oren jer <oren.jer.weil@intel.com>; linux-mtd@lists.infradead.org;
> > dri-
> > > > > devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; linux-
> > > > > kernel@vger.kernel.org
> > > > > Subject: Re: [PATCH 06/10] mtd: intel-dg: wake card on operations
> > > > >
> > > > > On Tue, Oct 22, 2024 at 01:41:15PM +0300, Alexander Usyskin wrote:
> > > > > > Enable runtime PM in mtd driver to notify graphics driver that whole
> > > > > > card should be kept awake while nvm operations are performed
> > through
> > > > > > this driver.
> > > > > >
> > > > > > CC: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > > > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> > > > > > ---
> > > > > >  drivers/mtd/devices/mtd-intel-dg.c | 73
> > > > > > +++++++++++++++++++++++++-----
> > > > > >  1 file changed, 61 insertions(+), 12 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/mtd/devices/mtd-intel-dg.c
> > > > > > b/drivers/mtd/devices/mtd-intel-dg.c
> > > > > > index 4951438e8faf..3d53f35478e8 100644
> > > > > > --- a/drivers/mtd/devices/mtd-intel-dg.c
> > > > > > +++ b/drivers/mtd/devices/mtd-intel-dg.c
> > > > > > @@ -15,11 +15,14 @@
> > > > > >  #include <linux/module.h>
> > > > > >  #include <linux/mtd/mtd.h>
> > > > > >  #include <linux/mtd/partitions.h>
> > > > > > +#include <linux/pm_runtime.h>
> > > > > >  #include <linux/string.h>
> > > > > >  #include <linux/slab.h>
> > > > > >  #include <linux/sizes.h>
> > > > > >  #include <linux/types.h>
> > > > > >
> > > > > > +#define INTEL_DG_NVM_RPM_TIMEOUT 500
> > > > > > +
> > > > > >  struct intel_dg_nvm {
> > > > > >  	struct kref refcnt;
> > > > > >  	struct mtd_info mtd;
> > > > > > @@ -462,6 +465,7 @@ static int intel_dg_mtd_erase(struct mtd_info
> > > > *mtd,
> > > > > struct erase_info *info)
> > > > > >  	loff_t from;
> > > > > >  	size_t len;
> > > > > >  	size_t total_len;
> > > > > > +	int ret = 0;
> > > > > >
> > > > > >  	if (WARN_ON(!nvm))
> > > > > >  		return -EINVAL;
> > > > > > @@ -476,20 +480,28 @@ static int intel_dg_mtd_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;
> > > > > > +	}
> > > > > > +
> > > > > >  	guard(mutex)(&nvm->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;
> > > > > > -			return -ERANGE;
> > > > > > +			ret = -ERANGE;
> > > > > > +			goto out;
> > > > > >  		}
> > > > > >
> > > > > >  		idx = idg_nvm_get_region(nvm, addr);
> > > > > >  		if (idx >= nvm->nregions) {
> > > > > >  			dev_err(&mtd->dev, "out of range");
> > > > > >  			info->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
> > > > > > -			return -ERANGE;
> > > > > > +			ret = -ERANGE;
> > > > > > +			goto out;
> > > > > >  		}
> > > > > >
> > > > > >  		from = addr - nvm->regions[idx].offset; @@ -505,14 +517,18
> > > > > @@
> > > > > > static int intel_dg_mtd_erase(struct mtd_info *mtd, struct erase_info
> > > > *info)
> > > > > >  		if (bytes < 0) {
> > > > > >  			dev_dbg(&mtd->dev, "erase failed with %zd\n",
> > > > > bytes);
> > > > > >  			info->fail_addr += nvm->regions[idx].offset;
> > > > > > -			return bytes;
> > > > > > +			ret = bytes;
> > > > > > +			goto out;
> > > > > >  		}
> > > > > >
> > > > > >  		addr += len;
> > > > > >  		total_len -= len;
> > > > > >  	}
> > > > > >
> > > > > > -	return 0;
> > > > > > +out:
> > > > > > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > > > > > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > > > > > +	return ret;
> > > > > >  }
> > > > > >
> > > > > >  static int intel_dg_mtd_read(struct mtd_info *mtd, loff_t from,
> > > > > > size_t len, @@ -541,17 +557,25 @@ static int
> > intel_dg_mtd_read(struct
> > > > > mtd_info *mtd, loff_t from, size_t len,
> > > > > >  	if (len > nvm->regions[idx].size - from)
> > > > > >  		len = nvm->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;
> > > > > > +	}
> > > > > > +
> > > > > >  	guard(mutex)(&nvm->lock);
> > > > > >
> > > > > >  	ret = idg_read(nvm, region, from, len, buf);
> > > > > >  	if (ret < 0) {
> > > > > >  		dev_dbg(&mtd->dev, "read failed with %zd\n", ret);
> > > > > > -		return ret;
> > > > > > +	} else {
> > > > > > +		*retlen = ret;
> > > > > > +		ret = 0;
> > > > > >  	}
> > > > > >
> > > > > > -	*retlen = ret;
> > > > > > -
> > > > > > -	return 0;
> > > > > > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > > > > > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > > > > > +	return ret;
> > > > > >  }
> > > > > >
> > > > > >  static int intel_dg_mtd_write(struct mtd_info *mtd, loff_t to, size_t
> > > > > > len, @@ -580,17 +604,25 @@ static int intel_dg_mtd_write(struct
> > > > mtd_info
> > > > > *mtd, loff_t to, size_t len,
> > > > > >  	if (len > nvm->regions[idx].size - to)
> > > > > >  		len = nvm->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;
> > > > > > +	}
> > > > > > +
> > > > > >  	guard(mutex)(&nvm->lock);
> > > > > >
> > > > > >  	ret = idg_write(nvm, region, to, len, buf);
> > > > > >  	if (ret < 0) {
> > > > > >  		dev_dbg(&mtd->dev, "write failed with %zd\n", ret);
> > > > > > -		return ret;
> > > > > > +	} else {
> > > > > > +		*retlen = ret;
> > > > > > +		ret = 0;
> > > > > >  	}
> > > > > >
> > > > > > -	*retlen = ret;
> > > > > > -
> > > > > > -	return 0;
> > > > > > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > > > > > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > > > > > +	return ret;
> > > > > >  }
> > > > > >
> > > > > >  static void intel_dg_nvm_release(struct kref *kref) @@ -722,6
> > +754,17
> > > > > > @@ static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
> > > > > >  		n++;
> > > > > >  	}
> > > > > >
> > > > > > +	pm_runtime_enable(device);
> > > > > > +
> > > > > > +	pm_runtime_set_autosuspend_delay(device,
> > > > > INTEL_DG_NVM_RPM_TIMEOUT);
> > > > > > +	pm_runtime_use_autosuspend(device);
> > > > >
> > > > > something looks strange here...
> > > > > on the functions above you get and put references for the parent device
> > > > > directly.
> > > > > But here you enable the rpm for this device.
> > > > >
> > > > > If I remember correctly from some old experiments, the rpm is smart
> > enough
> > > > > and wake up the parent before waking up the child. So I'm wondering if
> > we
> > > > > should only do the child without the parent.
> > > > Agree parent can't runtime suspend ind if it has active child.
> > > > Let this driver have it's own get/put routine instead of parent.
> > > > Thanks,
> > > > Anshuman Gupta.
> > >
> > > I need to wake DG card before the MTD device is established to scan the
> > partition table on flash.
> > > Thus, if I move rpm down to MTD device I should enable and take parent
> > (auxiliary device) rpm anyway.
> > 
> > That's the part that I'm not sure if I agree. if I remember from some
> > experiments in the past,
> > when you call to wake up the child, the parent will wakeup first anyway.
> > 
> The child (mtd device) does not exist at this point of time.
> To create MTD device, the partition table should be provided
> and it read directly from flash that should be powered to do read.

I don't understand... you have the mtd->dev at this point... this is
the one you should be touching, not the mtd->dev.parent... even at the
probe, but moreover on everywhere else as well.

> 
> > > Considering above, is this move is justified?
> > > Also, MTD drivers tend to enable parent rpm, not its own one.
> > 
> > What other drivers are you looking for reference?
> 
> drivers/mtd/nand/raw/tegra_nand.c
> drivers/mtd/nand/raw/renesas-nand-controller.c
> drivers/mtd/maps/physmap-core.c

I see they getting pdev->dev... not the parent...

> 
> > 
> > >
> > > - -
> > > Thanks,
> > > Sasha
> > >
> > >
> > >
> > > > >
> > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> > > > >
> > > > > > +
> > > > > > +	ret = pm_runtime_resume_and_get(device);
> > > > > > +	if (ret < 0) {
> > > > > > +		dev_err(device, "rpm: get failed %d\n", ret);
> > > > > > +		goto err_norpm;
> > > > > > +	}
> > > > > > +
> > > > > >  	nvm->base = devm_ioremap_resource(device, &invm->bar);
> > > > > >  	if (IS_ERR(nvm->base)) {
> > > > > >  		dev_err(device, "mmio not mapped\n"); @@ -744,9 +787,13
> > > > > @@ static
> > > > > > int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
> > > > > >
> > > > > >  	dev_set_drvdata(&aux_dev->dev, nvm);
> > > > > >
> > > > > > +	pm_runtime_put(device);
> > > > > >  	return 0;
> > > > > >
> > > > > >  err:
> > > > > > +	pm_runtime_put(device);
> > > > > > +err_norpm:
> > > > > > +	pm_runtime_disable(device);
> > > > > >  	kref_put(&nvm->refcnt, intel_dg_nvm_release);
> > > > > >  	return ret;
> > > > > >  }
> > > > > > @@ -758,6 +805,8 @@ static void intel_dg_mtd_remove(struct
> > > > > auxiliary_device *aux_dev)
> > > > > >  	if (!nvm)
> > > > > >  		return;
> > > > > >
> > > > > > +	pm_runtime_disable(&aux_dev->dev);
> > > > > > +
> > > > > >  	mtd_device_unregister(&nvm->mtd);
> > > > > >
> > > > > >  	dev_set_drvdata(&aux_dev->dev, NULL);
> > > > > > --
> > > > > > 2.43.0
> > > > > >

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 06/10] mtd: intel-dg: wake card on operations

[Miquel Raynal]

>> > That's the part that I'm not sure if I agree. if I remember from some
>> > experiments in the past,
>> > when you call to wake up the child, the parent will wakeup first anyway.
>> > 
>> The child (mtd device) does not exist at this point of time.
>> To create MTD device, the partition table should be provided
>> and it read directly from flash that should be powered to do read.
>
> I don't understand... you have the mtd->dev at this point... this is
> the one you should be touching, not the mtd->dev.parent... even at the
> probe, but moreover on everywhere else as well.
>
>> 
>> > > Considering above, is this move is justified?
>> > > Also, MTD drivers tend to enable parent rpm, not its own one.
>> > 
>> > What other drivers are you looking for reference?
>> 
>> drivers/mtd/nand/raw/tegra_nand.c
>> drivers/mtd/nand/raw/renesas-nand-controller.c
>> drivers/mtd/maps/physmap-core.c
>
> I see they getting pdev->dev... not the parent...

That's three drivers where there is probably room for improvement.

These differences are subtle and likely un-catch during review. Feel
free to correct the drivers if you think they are wrong and more
importantly, do the correct thing in your own.

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [PATCH 06/10] mtd: intel-dg: wake card on operations

[Usyskin, Alexander]

> -----Original Message-----
> From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Sent: Friday, November 8, 2024 12:50 AM
> To: Usyskin, Alexander <alexander.usyskin@intel.com>
> Cc: Gupta, Anshuman <anshuman.gupta@intel.com>; Deak, Imre
> <imre.deak@intel.com>; Miquel Raynal <miquel.raynal@bootlin.com>;
> Richard Weinberger <richard@nod.at>; Vignesh Raghavendra
> <vigneshr@ti.com>; De Marchi, Lucas <lucas.demarchi@intel.com>; Thomas
> Hellström <thomas.hellstrom@linux.intel.com>; Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>;
> Thomas Zimmermann <tzimmermann@suse.de>; David Airlie
> <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> <jani.nikula@linux.intel.com>; Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin <tursulin@ursulin.net>;
> Weil, Oren jer <oren.jer.weil@intel.com>; linux-mtd@lists.infradead.org; dri-
> devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 06/10] mtd: intel-dg: wake card on operations
> 
> On Tue, Nov 05, 2024 at 07:17:53AM -0500, Usyskin, Alexander wrote:
> > > -----Original Message-----
> > > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > > Sent: Monday, November 4, 2024 11:16 PM
> > > To: Usyskin, Alexander <alexander.usyskin@intel.com>
> > > Cc: Gupta, Anshuman <anshuman.gupta@intel.com>; Deak, Imre
> > > <imre.deak@intel.com>; Miquel Raynal <miquel.raynal@bootlin.com>;
> > > Richard Weinberger <richard@nod.at>; Vignesh Raghavendra
> > > <vigneshr@ti.com>; De Marchi, Lucas <lucas.demarchi@intel.com>;
> Thomas
> > > Hellström <thomas.hellstrom@linux.intel.com>; Maarten Lankhorst
> > > <maarten.lankhorst@linux.intel.com>; Maxime Ripard
> <mripard@kernel.org>;
> > > Thomas Zimmermann <tzimmermann@suse.de>; David Airlie
> > > <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> > > <jani.nikula@linux.intel.com>; Joonas Lahtinen
> > > <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin <tursulin@ursulin.net>;
> > > Weil, Oren jer <oren.jer.weil@intel.com>; linux-mtd@lists.infradead.org;
> dri-
> > > devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; linux-
> > > kernel@vger.kernel.org
> > > Subject: Re: [PATCH 06/10] mtd: intel-dg: wake card on operations
> > >
> > > On Tue, Oct 29, 2024 at 11:24:36AM +0000, Usyskin, Alexander wrote:
> > > > > -----Original Message-----
> > > > > From: Gupta, Anshuman <anshuman.gupta@intel.com>
> > > > > Sent: Monday, October 28, 2024 5:02 PM
> > > > > To: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Usyskin, Alexander
> > > > > <alexander.usyskin@intel.com>; Deak, Imre <imre.deak@intel.com>
> > > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Richard Weinberger
> > > > > <richard@nod.at>; Vignesh Raghavendra <vigneshr@ti.com>; De
> Marchi,
> > > > > Lucas <lucas.demarchi@intel.com>; Thomas Hellström
> > > > > <thomas.hellstrom@linux.intel.com>; Maarten Lankhorst
> > > > > <maarten.lankhorst@linux.intel.com>; Maxime Ripard
> > > <mripard@kernel.org>;
> > > > > Thomas Zimmermann <tzimmermann@suse.de>; David Airlie
> > > > > <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> > > > > <jani.nikula@linux.intel.com>; Joonas Lahtinen
> > > > > <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin
> <tursulin@ursulin.net>;
> > > > > Weil, Oren jer <oren.jer.weil@intel.com>; linux-
> mtd@lists.infradead.org;
> > > dri-
> > > > > devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; linux-
> > > > > kernel@vger.kernel.org
> > > > > Subject: RE: [PATCH 06/10] mtd: intel-dg: wake card on operations
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > > > > > Sent: Monday, October 28, 2024 8:27 PM
> > > > > > To: Usyskin, Alexander <alexander.usyskin@intel.com>; Gupta,
> > > Anshuman
> > > > > > <anshuman.gupta@intel.com>; Deak, Imre <imre.deak@intel.com>
> > > > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Richard Weinberger
> > > > > > <richard@nod.at>; Vignesh Raghavendra <vigneshr@ti.com>; De
> Marchi,
> > > > > > Lucas <lucas.demarchi@intel.com>; Thomas Hellström
> > > > > > <thomas.hellstrom@linux.intel.com>; Maarten Lankhorst
> > > > > > <maarten.lankhorst@linux.intel.com>; Maxime Ripard
> > > > > <mripard@kernel.org>;
> > > > > > Thomas Zimmermann <tzimmermann@suse.de>; David Airlie
> > > > > > <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> > > > > > <jani.nikula@linux.intel.com>; Joonas Lahtinen
> > > > > > <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin
> > > <tursulin@ursulin.net>;
> > > > > > Weil, Oren jer <oren.jer.weil@intel.com>; linux-
> mtd@lists.infradead.org;
> > > dri-
> > > > > > devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; linux-
> > > > > > kernel@vger.kernel.org
> > > > > > Subject: Re: [PATCH 06/10] mtd: intel-dg: wake card on operations
> > > > > >
> > > > > > On Tue, Oct 22, 2024 at 01:41:15PM +0300, Alexander Usyskin
> wrote:
> > > > > > > Enable runtime PM in mtd driver to notify graphics driver that whole
> > > > > > > card should be kept awake while nvm operations are performed
> > > through
> > > > > > > this driver.
> > > > > > >
> > > > > > > CC: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > > > > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> > > > > > > ---
> > > > > > >  drivers/mtd/devices/mtd-intel-dg.c | 73
> > > > > > > +++++++++++++++++++++++++-----
> > > > > > >  1 file changed, 61 insertions(+), 12 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/mtd/devices/mtd-intel-dg.c
> > > > > > > b/drivers/mtd/devices/mtd-intel-dg.c
> > > > > > > index 4951438e8faf..3d53f35478e8 100644
> > > > > > > --- a/drivers/mtd/devices/mtd-intel-dg.c
> > > > > > > +++ b/drivers/mtd/devices/mtd-intel-dg.c
> > > > > > > @@ -15,11 +15,14 @@
> > > > > > >  #include <linux/module.h>
> > > > > > >  #include <linux/mtd/mtd.h>
> > > > > > >  #include <linux/mtd/partitions.h>
> > > > > > > +#include <linux/pm_runtime.h>
> > > > > > >  #include <linux/string.h>
> > > > > > >  #include <linux/slab.h>
> > > > > > >  #include <linux/sizes.h>
> > > > > > >  #include <linux/types.h>
> > > > > > >
> > > > > > > +#define INTEL_DG_NVM_RPM_TIMEOUT 500
> > > > > > > +
> > > > > > >  struct intel_dg_nvm {
> > > > > > >  	struct kref refcnt;
> > > > > > >  	struct mtd_info mtd;
> > > > > > > @@ -462,6 +465,7 @@ static int intel_dg_mtd_erase(struct
> mtd_info
> > > > > *mtd,
> > > > > > struct erase_info *info)
> > > > > > >  	loff_t from;
> > > > > > >  	size_t len;
> > > > > > >  	size_t total_len;
> > > > > > > +	int ret = 0;
> > > > > > >
> > > > > > >  	if (WARN_ON(!nvm))
> > > > > > >  		return -EINVAL;
> > > > > > > @@ -476,20 +480,28 @@ static int intel_dg_mtd_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;
> > > > > > > +	}
> > > > > > > +
> > > > > > >  	guard(mutex)(&nvm->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;
> > > > > > > -			return -ERANGE;
> > > > > > > +			ret = -ERANGE;
> > > > > > > +			goto out;
> > > > > > >  		}
> > > > > > >
> > > > > > >  		idx = idg_nvm_get_region(nvm, addr);
> > > > > > >  		if (idx >= nvm->nregions) {
> > > > > > >  			dev_err(&mtd->dev, "out of range");
> > > > > > >  			info->fail_addr =
> MTD_FAIL_ADDR_UNKNOWN;
> > > > > > > -			return -ERANGE;
> > > > > > > +			ret = -ERANGE;
> > > > > > > +			goto out;
> > > > > > >  		}
> > > > > > >
> > > > > > >  		from = addr - nvm->regions[idx].offset; @@ -505,14
> +517,18
> > > > > > @@
> > > > > > > static int intel_dg_mtd_erase(struct mtd_info *mtd, struct
> erase_info
> > > > > *info)
> > > > > > >  		if (bytes < 0) {
> > > > > > >  			dev_dbg(&mtd->dev, "erase failed with
> %zd\n",
> > > > > > bytes);
> > > > > > >  			info->fail_addr += nvm->regions[idx].offset;
> > > > > > > -			return bytes;
> > > > > > > +			ret = bytes;
> > > > > > > +			goto out;
> > > > > > >  		}
> > > > > > >
> > > > > > >  		addr += len;
> > > > > > >  		total_len -= len;
> > > > > > >  	}
> > > > > > >
> > > > > > > -	return 0;
> > > > > > > +out:
> > > > > > > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > > > > > > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > > > > > > +	return ret;
> > > > > > >  }
> > > > > > >
> > > > > > >  static int intel_dg_mtd_read(struct mtd_info *mtd, loff_t from,
> > > > > > > size_t len, @@ -541,17 +557,25 @@ static int
> > > intel_dg_mtd_read(struct
> > > > > > mtd_info *mtd, loff_t from, size_t len,
> > > > > > >  	if (len > nvm->regions[idx].size - from)
> > > > > > >  		len = nvm->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;
> > > > > > > +	}
> > > > > > > +
> > > > > > >  	guard(mutex)(&nvm->lock);
> > > > > > >
> > > > > > >  	ret = idg_read(nvm, region, from, len, buf);
> > > > > > >  	if (ret < 0) {
> > > > > > >  		dev_dbg(&mtd->dev, "read failed with %zd\n", ret);
> > > > > > > -		return ret;
> > > > > > > +	} else {
> > > > > > > +		*retlen = ret;
> > > > > > > +		ret = 0;
> > > > > > >  	}
> > > > > > >
> > > > > > > -	*retlen = ret;
> > > > > > > -
> > > > > > > -	return 0;
> > > > > > > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > > > > > > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > > > > > > +	return ret;
> > > > > > >  }
> > > > > > >
> > > > > > >  static int intel_dg_mtd_write(struct mtd_info *mtd, loff_t to, size_t
> > > > > > > len, @@ -580,17 +604,25 @@ static int intel_dg_mtd_write(struct
> > > > > mtd_info
> > > > > > *mtd, loff_t to, size_t len,
> > > > > > >  	if (len > nvm->regions[idx].size - to)
> > > > > > >  		len = nvm->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;
> > > > > > > +	}
> > > > > > > +
> > > > > > >  	guard(mutex)(&nvm->lock);
> > > > > > >
> > > > > > >  	ret = idg_write(nvm, region, to, len, buf);
> > > > > > >  	if (ret < 0) {
> > > > > > >  		dev_dbg(&mtd->dev, "write failed with %zd\n", ret);
> > > > > > > -		return ret;
> > > > > > > +	} else {
> > > > > > > +		*retlen = ret;
> > > > > > > +		ret = 0;
> > > > > > >  	}
> > > > > > >
> > > > > > > -	*retlen = ret;
> > > > > > > -
> > > > > > > -	return 0;
> > > > > > > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > > > > > > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > > > > > > +	return ret;
> > > > > > >  }
> > > > > > >
> > > > > > >  static void intel_dg_nvm_release(struct kref *kref) @@ -722,6
> > > +754,17
> > > > > > > @@ static int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
> > > > > > >  		n++;
> > > > > > >  	}
> > > > > > >
> > > > > > > +	pm_runtime_enable(device);
> > > > > > > +
> > > > > > > +	pm_runtime_set_autosuspend_delay(device,
> > > > > > INTEL_DG_NVM_RPM_TIMEOUT);
> > > > > > > +	pm_runtime_use_autosuspend(device);
> > > > > >
> > > > > > something looks strange here...
> > > > > > on the functions above you get and put references for the parent
> device
> > > > > > directly.
> > > > > > But here you enable the rpm for this device.
> > > > > >
> > > > > > If I remember correctly from some old experiments, the rpm is smart
> > > enough
> > > > > > and wake up the parent before waking up the child. So I'm wondering
> if
> > > we
> > > > > > should only do the child without the parent.
> > > > > Agree parent can't runtime suspend ind if it has active child.
> > > > > Let this driver have it's own get/put routine instead of parent.
> > > > > Thanks,
> > > > > Anshuman Gupta.
> > > >
> > > > I need to wake DG card before the MTD device is established to scan the
> > > partition table on flash.
> > > > Thus, if I move rpm down to MTD device I should enable and take parent
> > > (auxiliary device) rpm anyway.
> > >
> > > That's the part that I'm not sure if I agree. if I remember from some
> > > experiments in the past,
> > > when you call to wake up the child, the parent will wakeup first anyway.
> > >
> > The child (mtd device) does not exist at this point of time.
> > To create MTD device, the partition table should be provided
> > and it read directly from flash that should be powered to do read.
> 
> I don't understand... you have the mtd->dev at this point... this is
> the one you should be touching, not the mtd->dev.parent... even at the
> probe, but moreover on everywhere else as well.
> 

At the probe time I do not have dev->mtd, but now I see you point here.
I'll separate power management:
- probe before dev->mtd creation will use aux_dev->dev (that will be mtd->dev.parent later)
- mtd functions will use mtd->dev

Is this that you have in mind?

> >
> > > > Considering above, is this move is justified?
> > > > Also, MTD drivers tend to enable parent rpm, not its own one.
> > >
> > > What other drivers are you looking for reference?
> >
> > drivers/mtd/nand/raw/tegra_nand.c
> > drivers/mtd/nand/raw/renesas-nand-controller.c
> > drivers/mtd/maps/physmap-core.c
> 
> I see they getting pdev->dev... not the parent...
> 
> >
> > >
> > > >
> > > > - -
> > > > Thanks,
> > > > Sasha
> > > >
> > > >
> > > >
> > > > > >
> > > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > > Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> > > > > >
> > > > > > > +
> > > > > > > +	ret = pm_runtime_resume_and_get(device);
> > > > > > > +	if (ret < 0) {
> > > > > > > +		dev_err(device, "rpm: get failed %d\n", ret);
> > > > > > > +		goto err_norpm;
> > > > > > > +	}
> > > > > > > +
> > > > > > >  	nvm->base = devm_ioremap_resource(device, &invm->bar);
> > > > > > >  	if (IS_ERR(nvm->base)) {
> > > > > > >  		dev_err(device, "mmio not mapped\n"); @@ -744,9
> +787,13
> > > > > > @@ static
> > > > > > > int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
> > > > > > >
> > > > > > >  	dev_set_drvdata(&aux_dev->dev, nvm);
> > > > > > >
> > > > > > > +	pm_runtime_put(device);
> > > > > > >  	return 0;
> > > > > > >
> > > > > > >  err:
> > > > > > > +	pm_runtime_put(device);
> > > > > > > +err_norpm:
> > > > > > > +	pm_runtime_disable(device);
> > > > > > >  	kref_put(&nvm->refcnt, intel_dg_nvm_release);
> > > > > > >  	return ret;
> > > > > > >  }
> > > > > > > @@ -758,6 +805,8 @@ static void intel_dg_mtd_remove(struct
> > > > > > auxiliary_device *aux_dev)
> > > > > > >  	if (!nvm)
> > > > > > >  		return;
> > > > > > >
> > > > > > > +	pm_runtime_disable(&aux_dev->dev);
> > > > > > > +
> > > > > > >  	mtd_device_unregister(&nvm->mtd);
> > > > > > >
> > > > > > >  	dev_set_drvdata(&aux_dev->dev, NULL);
> > > > > > > --
> > > > > > > 2.43.0
> > > > > > >
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [PATCH 06/10] mtd: intel-dg: wake card on operations

[Usyskin, Alexander]

> -----Original Message-----
> From: Usyskin, Alexander
> Sent: Sunday, November 10, 2024 3:17 PM
> To: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Cc: Gupta, Anshuman <Anshuman.Gupta@intel.com>; Deak, Imre
> <imre.deak@intel.com>; Miquel Raynal <miquel.raynal@bootlin.com>;
> Richard Weinberger <richard@nod.at>; Vignesh Raghavendra
> <vigneshr@ti.com>; De Marchi, Lucas <lucas.demarchi@intel.com>; Thomas
> Hellström <thomas.hellstrom@linux.intel.com>; Maarten Lankhorst
> <maarten.lankhorst@linux.intel.com>; Maxime Ripard <mripard@kernel.org>;
> Thomas Zimmermann <tzimmermann@suse.de>; David Airlie
> <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> <jani.nikula@linux.intel.com>; Joonas Lahtinen
> <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin <tursulin@ursulin.net>;
> Weil, Oren jer <oren.jer.weil@intel.com>; linux-mtd@lists.infradead.org; dri-
> devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; linux-
> kernel@vger.kernel.org
> Subject: RE: [PATCH 06/10] mtd: intel-dg: wake card on operations
> 
> > -----Original Message-----
> > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Sent: Friday, November 8, 2024 12:50 AM
> > To: Usyskin, Alexander <alexander.usyskin@intel.com>
> > Cc: Gupta, Anshuman <anshuman.gupta@intel.com>; Deak, Imre
> > <imre.deak@intel.com>; Miquel Raynal <miquel.raynal@bootlin.com>;
> > Richard Weinberger <richard@nod.at>; Vignesh Raghavendra
> > <vigneshr@ti.com>; De Marchi, Lucas <lucas.demarchi@intel.com>; Thomas
> > Hellström <thomas.hellstrom@linux.intel.com>; Maarten Lankhorst
> > <maarten.lankhorst@linux.intel.com>; Maxime Ripard
> <mripard@kernel.org>;
> > Thomas Zimmermann <tzimmermann@suse.de>; David Airlie
> > <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> > <jani.nikula@linux.intel.com>; Joonas Lahtinen
> > <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin <tursulin@ursulin.net>;
> > Weil, Oren jer <oren.jer.weil@intel.com>; linux-mtd@lists.infradead.org; dri-
> > devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH 06/10] mtd: intel-dg: wake card on operations
> >
> > On Tue, Nov 05, 2024 at 07:17:53AM -0500, Usyskin, Alexander wrote:
> > > > -----Original Message-----
> > > > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > > > Sent: Monday, November 4, 2024 11:16 PM
> > > > To: Usyskin, Alexander <alexander.usyskin@intel.com>
> > > > Cc: Gupta, Anshuman <anshuman.gupta@intel.com>; Deak, Imre
> > > > <imre.deak@intel.com>; Miquel Raynal <miquel.raynal@bootlin.com>;
> > > > Richard Weinberger <richard@nod.at>; Vignesh Raghavendra
> > > > <vigneshr@ti.com>; De Marchi, Lucas <lucas.demarchi@intel.com>;
> > Thomas
> > > > Hellström <thomas.hellstrom@linux.intel.com>; Maarten Lankhorst
> > > > <maarten.lankhorst@linux.intel.com>; Maxime Ripard
> > <mripard@kernel.org>;
> > > > Thomas Zimmermann <tzimmermann@suse.de>; David Airlie
> > > > <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> > > > <jani.nikula@linux.intel.com>; Joonas Lahtinen
> > > > <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin
> <tursulin@ursulin.net>;
> > > > Weil, Oren jer <oren.jer.weil@intel.com>; linux-mtd@lists.infradead.org;
> > dri-
> > > > devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; linux-
> > > > kernel@vger.kernel.org
> > > > Subject: Re: [PATCH 06/10] mtd: intel-dg: wake card on operations
> > > >
> > > > On Tue, Oct 29, 2024 at 11:24:36AM +0000, Usyskin, Alexander wrote:
> > > > > > -----Original Message-----
> > > > > > From: Gupta, Anshuman <anshuman.gupta@intel.com>
> > > > > > Sent: Monday, October 28, 2024 5:02 PM
> > > > > > To: Vivi, Rodrigo <rodrigo.vivi@intel.com>; Usyskin, Alexander
> > > > > > <alexander.usyskin@intel.com>; Deak, Imre <imre.deak@intel.com>
> > > > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Richard Weinberger
> > > > > > <richard@nod.at>; Vignesh Raghavendra <vigneshr@ti.com>; De
> > Marchi,
> > > > > > Lucas <lucas.demarchi@intel.com>; Thomas Hellström
> > > > > > <thomas.hellstrom@linux.intel.com>; Maarten Lankhorst
> > > > > > <maarten.lankhorst@linux.intel.com>; Maxime Ripard
> > > > <mripard@kernel.org>;
> > > > > > Thomas Zimmermann <tzimmermann@suse.de>; David Airlie
> > > > > > <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> > > > > > <jani.nikula@linux.intel.com>; Joonas Lahtinen
> > > > > > <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin
> > <tursulin@ursulin.net>;
> > > > > > Weil, Oren jer <oren.jer.weil@intel.com>; linux-
> > mtd@lists.infradead.org;
> > > > dri-
> > > > > > devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; linux-
> > > > > > kernel@vger.kernel.org
> > > > > > Subject: RE: [PATCH 06/10] mtd: intel-dg: wake card on operations
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > > > > > > Sent: Monday, October 28, 2024 8:27 PM
> > > > > > > To: Usyskin, Alexander <alexander.usyskin@intel.com>; Gupta,
> > > > Anshuman
> > > > > > > <anshuman.gupta@intel.com>; Deak, Imre <imre.deak@intel.com>
> > > > > > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Richard
> Weinberger
> > > > > > > <richard@nod.at>; Vignesh Raghavendra <vigneshr@ti.com>; De
> > Marchi,
> > > > > > > Lucas <lucas.demarchi@intel.com>; Thomas Hellström
> > > > > > > <thomas.hellstrom@linux.intel.com>; Maarten Lankhorst
> > > > > > > <maarten.lankhorst@linux.intel.com>; Maxime Ripard
> > > > > > <mripard@kernel.org>;
> > > > > > > Thomas Zimmermann <tzimmermann@suse.de>; David Airlie
> > > > > > > <airlied@gmail.com>; Simona Vetter <simona@ffwll.ch>; Jani Nikula
> > > > > > > <jani.nikula@linux.intel.com>; Joonas Lahtinen
> > > > > > > <joonas.lahtinen@linux.intel.com>; Tvrtko Ursulin
> > > > <tursulin@ursulin.net>;
> > > > > > > Weil, Oren jer <oren.jer.weil@intel.com>; linux-
> > mtd@lists.infradead.org;
> > > > dri-
> > > > > > > devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org; linux-
> > > > > > > kernel@vger.kernel.org
> > > > > > > Subject: Re: [PATCH 06/10] mtd: intel-dg: wake card on operations
> > > > > > >
> > > > > > > On Tue, Oct 22, 2024 at 01:41:15PM +0300, Alexander Usyskin
> > wrote:
> > > > > > > > Enable runtime PM in mtd driver to notify graphics driver that
> whole
> > > > > > > > card should be kept awake while nvm operations are performed
> > > > through
> > > > > > > > this driver.
> > > > > > > >
> > > > > > > > CC: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > > > > > Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
> > > > > > > > ---
> > > > > > > >  drivers/mtd/devices/mtd-intel-dg.c | 73
> > > > > > > > +++++++++++++++++++++++++-----
> > > > > > > >  1 file changed, 61 insertions(+), 12 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/mtd/devices/mtd-intel-dg.c
> > > > > > > > b/drivers/mtd/devices/mtd-intel-dg.c
> > > > > > > > index 4951438e8faf..3d53f35478e8 100644
> > > > > > > > --- a/drivers/mtd/devices/mtd-intel-dg.c
> > > > > > > > +++ b/drivers/mtd/devices/mtd-intel-dg.c
> > > > > > > > @@ -15,11 +15,14 @@
> > > > > > > >  #include <linux/module.h>
> > > > > > > >  #include <linux/mtd/mtd.h>
> > > > > > > >  #include <linux/mtd/partitions.h>
> > > > > > > > +#include <linux/pm_runtime.h>
> > > > > > > >  #include <linux/string.h>
> > > > > > > >  #include <linux/slab.h>
> > > > > > > >  #include <linux/sizes.h>
> > > > > > > >  #include <linux/types.h>
> > > > > > > >
> > > > > > > > +#define INTEL_DG_NVM_RPM_TIMEOUT 500
> > > > > > > > +
> > > > > > > >  struct intel_dg_nvm {
> > > > > > > >  	struct kref refcnt;
> > > > > > > >  	struct mtd_info mtd;
> > > > > > > > @@ -462,6 +465,7 @@ static int intel_dg_mtd_erase(struct
> > mtd_info
> > > > > > *mtd,
> > > > > > > struct erase_info *info)
> > > > > > > >  	loff_t from;
> > > > > > > >  	size_t len;
> > > > > > > >  	size_t total_len;
> > > > > > > > +	int ret = 0;
> > > > > > > >
> > > > > > > >  	if (WARN_ON(!nvm))
> > > > > > > >  		return -EINVAL;
> > > > > > > > @@ -476,20 +480,28 @@ static int intel_dg_mtd_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;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > >  	guard(mutex)(&nvm->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;
> > > > > > > > -			return -ERANGE;
> > > > > > > > +			ret = -ERANGE;
> > > > > > > > +			goto out;
> > > > > > > >  		}
> > > > > > > >
> > > > > > > >  		idx = idg_nvm_get_region(nvm, addr);
> > > > > > > >  		if (idx >= nvm->nregions) {
> > > > > > > >  			dev_err(&mtd->dev, "out of range");
> > > > > > > >  			info->fail_addr =
> > MTD_FAIL_ADDR_UNKNOWN;
> > > > > > > > -			return -ERANGE;
> > > > > > > > +			ret = -ERANGE;
> > > > > > > > +			goto out;
> > > > > > > >  		}
> > > > > > > >
> > > > > > > >  		from = addr - nvm->regions[idx].offset; @@ -505,14
> > +517,18
> > > > > > > @@
> > > > > > > > static int intel_dg_mtd_erase(struct mtd_info *mtd, struct
> > erase_info
> > > > > > *info)
> > > > > > > >  		if (bytes < 0) {
> > > > > > > >  			dev_dbg(&mtd->dev, "erase failed with
> > %zd\n",
> > > > > > > bytes);
> > > > > > > >  			info->fail_addr += nvm->regions[idx].offset;
> > > > > > > > -			return bytes;
> > > > > > > > +			ret = bytes;
> > > > > > > > +			goto out;
> > > > > > > >  		}
> > > > > > > >
> > > > > > > >  		addr += len;
> > > > > > > >  		total_len -= len;
> > > > > > > >  	}
> > > > > > > >
> > > > > > > > -	return 0;
> > > > > > > > +out:
> > > > > > > > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > > > > > > > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > > > > > > > +	return ret;
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  static int intel_dg_mtd_read(struct mtd_info *mtd, loff_t from,
> > > > > > > > size_t len, @@ -541,17 +557,25 @@ static int
> > > > intel_dg_mtd_read(struct
> > > > > > > mtd_info *mtd, loff_t from, size_t len,
> > > > > > > >  	if (len > nvm->regions[idx].size - from)
> > > > > > > >  		len = nvm->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;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > >  	guard(mutex)(&nvm->lock);
> > > > > > > >
> > > > > > > >  	ret = idg_read(nvm, region, from, len, buf);
> > > > > > > >  	if (ret < 0) {
> > > > > > > >  		dev_dbg(&mtd->dev, "read failed with %zd\n", ret);
> > > > > > > > -		return ret;
> > > > > > > > +	} else {
> > > > > > > > +		*retlen = ret;
> > > > > > > > +		ret = 0;
> > > > > > > >  	}
> > > > > > > >
> > > > > > > > -	*retlen = ret;
> > > > > > > > -
> > > > > > > > -	return 0;
> > > > > > > > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > > > > > > > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > > > > > > > +	return ret;
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  static int intel_dg_mtd_write(struct mtd_info *mtd, loff_t to,
> size_t
> > > > > > > > len, @@ -580,17 +604,25 @@ static int
> intel_dg_mtd_write(struct
> > > > > > mtd_info
> > > > > > > *mtd, loff_t to, size_t len,
> > > > > > > >  	if (len > nvm->regions[idx].size - to)
> > > > > > > >  		len = nvm->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;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > >  	guard(mutex)(&nvm->lock);
> > > > > > > >
> > > > > > > >  	ret = idg_write(nvm, region, to, len, buf);
> > > > > > > >  	if (ret < 0) {
> > > > > > > >  		dev_dbg(&mtd->dev, "write failed with %zd\n", ret);
> > > > > > > > -		return ret;
> > > > > > > > +	} else {
> > > > > > > > +		*retlen = ret;
> > > > > > > > +		ret = 0;
> > > > > > > >  	}
> > > > > > > >
> > > > > > > > -	*retlen = ret;
> > > > > > > > -
> > > > > > > > -	return 0;
> > > > > > > > +	pm_runtime_mark_last_busy(mtd->dev.parent);
> > > > > > > > +	pm_runtime_put_autosuspend(mtd->dev.parent);
> > > > > > > > +	return ret;
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  static void intel_dg_nvm_release(struct kref *kref) @@ -722,6
> > > > +754,17
> > > > > > > > @@ static int intel_dg_mtd_probe(struct auxiliary_device
> *aux_dev,
> > > > > > > >  		n++;
> > > > > > > >  	}
> > > > > > > >
> > > > > > > > +	pm_runtime_enable(device);
> > > > > > > > +
> > > > > > > > +	pm_runtime_set_autosuspend_delay(device,
> > > > > > > INTEL_DG_NVM_RPM_TIMEOUT);
> > > > > > > > +	pm_runtime_use_autosuspend(device);
> > > > > > >
> > > > > > > something looks strange here...
> > > > > > > on the functions above you get and put references for the parent
> > device
> > > > > > > directly.
> > > > > > > But here you enable the rpm for this device.
> > > > > > >
> > > > > > > If I remember correctly from some old experiments, the rpm is smart
> > > > enough
> > > > > > > and wake up the parent before waking up the child. So I'm
> wondering
> > if
> > > > we
> > > > > > > should only do the child without the parent.
> > > > > > Agree parent can't runtime suspend ind if it has active child.
> > > > > > Let this driver have it's own get/put routine instead of parent.
> > > > > > Thanks,
> > > > > > Anshuman Gupta.
> > > > >
> > > > > I need to wake DG card before the MTD device is established to scan the
> > > > partition table on flash.
> > > > > Thus, if I move rpm down to MTD device I should enable and take
> parent
> > > > (auxiliary device) rpm anyway.
> > > >
> > > > That's the part that I'm not sure if I agree. if I remember from some
> > > > experiments in the past,
> > > > when you call to wake up the child, the parent will wakeup first anyway.
> > > >
> > > The child (mtd device) does not exist at this point of time.
> > > To create MTD device, the partition table should be provided
> > > and it read directly from flash that should be powered to do read.
> >
> > I don't understand... you have the mtd->dev at this point... this is
> > the one you should be touching, not the mtd->dev.parent... even at the
> > probe, but moreover on everywhere else as well.
> >
> 
> At the probe time I do not have dev->mtd, but now I see you point here.
> I'll separate power management:
> - probe before dev->mtd creation will use aux_dev->dev (that will be mtd-
> >dev.parent later)
> - mtd functions will use mtd->dev
> 
> Is this that you have in mind?

I've tried it and found out that mtd->dev is not initialized if partitions are present [1].
Miquel - this may be the reason why other mtd drivers use pci or platform
devices to manage runtime pm.
Or I have missed something?

[1] https://elixir.bootlin.com/linux/v6.12-rc6/source/drivers/mtd/mtdcore.c#L1078
> 
> > >
> > > > > Considering above, is this move is justified?
> > > > > Also, MTD drivers tend to enable parent rpm, not its own one.
> > > >
> > > > What other drivers are you looking for reference?
> > >
> > > drivers/mtd/nand/raw/tegra_nand.c
> > > drivers/mtd/nand/raw/renesas-nand-controller.c
> > > drivers/mtd/maps/physmap-core.c
> >
> > I see they getting pdev->dev... not the parent...
> >
> > >
> > > >
> > > > >
> > > > > - -
> > > > > Thanks,
> > > > > Sasha
> > > > >
> > > > >
> > > > >
> > > > > > >
> > > > > > > Cc: Imre Deak <imre.deak@intel.com>
> > > > > > > Cc: Anshuman Gupta <anshuman.gupta@intel.com>
> > > > > > >
> > > > > > > > +
> > > > > > > > +	ret = pm_runtime_resume_and_get(device);
> > > > > > > > +	if (ret < 0) {
> > > > > > > > +		dev_err(device, "rpm: get failed %d\n", ret);
> > > > > > > > +		goto err_norpm;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > >  	nvm->base = devm_ioremap_resource(device, &invm->bar);
> > > > > > > >  	if (IS_ERR(nvm->base)) {
> > > > > > > >  		dev_err(device, "mmio not mapped\n"); @@ -744,9
> > +787,13
> > > > > > > @@ static
> > > > > > > > int intel_dg_mtd_probe(struct auxiliary_device *aux_dev,
> > > > > > > >
> > > > > > > >  	dev_set_drvdata(&aux_dev->dev, nvm);
> > > > > > > >
> > > > > > > > +	pm_runtime_put(device);
> > > > > > > >  	return 0;
> > > > > > > >
> > > > > > > >  err:
> > > > > > > > +	pm_runtime_put(device);
> > > > > > > > +err_norpm:
> > > > > > > > +	pm_runtime_disable(device);
> > > > > > > >  	kref_put(&nvm->refcnt, intel_dg_nvm_release);
> > > > > > > >  	return ret;
> > > > > > > >  }
> > > > > > > > @@ -758,6 +805,8 @@ static void intel_dg_mtd_remove(struct
> > > > > > > auxiliary_device *aux_dev)
> > > > > > > >  	if (!nvm)
> > > > > > > >  		return;
> > > > > > > >
> > > > > > > > +	pm_runtime_disable(&aux_dev->dev);
> > > > > > > > +
> > > > > > > >  	mtd_device_unregister(&nvm->mtd);
> > > > > > > >
> > > > > > > >  	dev_set_drvdata(&aux_dev->dev, NULL);
> > > > > > > > --
> > > > > > > > 2.43.0
> > > > > > > >
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 06/10] mtd: intel-dg: wake card on operations

[Miquel Raynal]

Hi Alexander,

Please reduce the context when answering, otherwise it's hard to find
all places where you commented.

>> > > > That's the part that I'm not sure if I agree. if I remember from some
>> > > > experiments in the past,
>> > > > when you call to wake up the child, the parent will wakeup first anyway.
>> > > >
>> > > The child (mtd device) does not exist at this point of time.
>> > > To create MTD device, the partition table should be provided
>> > > and it read directly from flash that should be powered to do read.
>> >
>> > I don't understand... you have the mtd->dev at this point... this is
>> > the one you should be touching, not the mtd->dev.parent... even at the
>> > probe, but moreover on everywhere else as well.
>> >
>> 
>> At the probe time I do not have dev->mtd, but now I see you point here.
>> I'll separate power management:
>> - probe before dev->mtd creation will use aux_dev->dev (that will be mtd-
>> >dev.parent later)
>> - mtd functions will use mtd->dev
>> 
>> Is this that you have in mind?
>
> I've tried it and found out that mtd->dev is not initialized if partitions are present [1].
> Miquel - this may be the reason why other mtd drivers use pci or platform
> devices to manage runtime pm.
> Or I have missed something?

Please keep in mind there is _a lot_ of history behind mtd, and
sometimes choices from the past cannot be simply "fixed" without
breaking userspace. The problem with mtd is that the "mtd" structure
defines nothing with precision. It may be a controller, a chip, a
partition, or whatever mix of those. In this particular case, I believe
you are mentioning the KEEP_PARTITIONED_MASTER configuration, which by
default is unset, which means you'll loose the "top level" mtd device?

However in general I believe the "framework" struct device is maybe less
relevant than the "bus" struct device when it comes to runtime PM, so
actually I would eventually expect this device to be used?

> [1] https://elixir.bootlin.com/linux/v6.12-rc6/source/drivers/mtd/mtdcore.c#L1078

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [PATCH 06/10] mtd: intel-dg: wake card on operations

[Usyskin, Alexander]

Adding Kartihik

> Subject: Re: [PATCH 06/10] mtd: intel-dg: wake card on operations
> 
> Hi Alexander,
> 
> Please reduce the context when answering, otherwise it's hard to find
> all places where you commented.
> 
> >> > > > That's the part that I'm not sure if I agree. if I remember from some
> >> > > > experiments in the past,
> >> > > > when you call to wake up the child, the parent will wakeup first
> anyway.
> >> > > >
> >> > > The child (mtd device) does not exist at this point of time.
> >> > > To create MTD device, the partition table should be provided
> >> > > and it read directly from flash that should be powered to do read.
> >> >
> >> > I don't understand... you have the mtd->dev at this point... this is
> >> > the one you should be touching, not the mtd->dev.parent... even at the
> >> > probe, but moreover on everywhere else as well.
> >> >
> >>
> >> At the probe time I do not have dev->mtd, but now I see you point here.
> >> I'll separate power management:
> >> - probe before dev->mtd creation will use aux_dev->dev (that will be mtd-
> >> >dev.parent later)
> >> - mtd functions will use mtd->dev
> >>
> >> Is this that you have in mind?
> >
> > I've tried it and found out that mtd->dev is not initialized if partitions are
> present [1].
> > Miquel - this may be the reason why other mtd drivers use pci or platform
> > devices to manage runtime pm.
> > Or I have missed something?
> 
> Please keep in mind there is _a lot_ of history behind mtd, and
> sometimes choices from the past cannot be simply "fixed" without
> breaking userspace. The problem with mtd is that the "mtd" structure
> defines nothing with precision. It may be a controller, a chip, a
> partition, or whatever mix of those. In this particular case, I believe
> you are mentioning the KEEP_PARTITIONED_MASTER configuration, which by
> default is unset, which means you'll loose the "top level" mtd device?
> 
> However in general I believe the "framework" struct device is maybe less
> relevant than the "bus" struct device when it comes to runtime PM, so
> actually I would eventually expect this device to be used?
> 

So, we can't reliable use the mtd here and should hook power management
on mtd->parent, as done in the patch.
Not sure if there any value in extending mtd to support runtime power management.

- - 
Thanks,
Sasha


> > [1] https://elixir.bootlin.com/linux/v6.12-
> rc6/source/drivers/mtd/mtdcore.c#L1078
> 
> Thanks,
> Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/