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

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
>

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