> On Tue, Feb 16, 2021 at 08:19:17PM +0200, Tomas Winkler wrote:
> >Enable access to internal spi on descrete devices via a child device.
> >The spi child device is exposed via MFD framework.
> >
> >Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> >Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx> # v3
> >Signed-off-by: Tomas Winkler <tomas.winkler@xxxxxxxxx>
> >---
> > drivers/gpu/drm/i915/Kconfig | 1 +
> > drivers/gpu/drm/i915/Makefile | 3 ++
> > drivers/gpu/drm/i915/i915_drv.c | 9 +++++
> > drivers/gpu/drm/i915/i915_drv.h | 4 +++
> > drivers/gpu/drm/i915/i915_reg.h | 1 +
> > drivers/gpu/drm/i915/spi/intel_spi.c | 53
> ++++++++++++++++++++++++++++
> >drivers/gpu/drm/i915/spi/intel_spi.h | 19 ++++++++++
> > 7 files changed, 90 insertions(+)
> > create mode 100644 drivers/gpu/drm/i915/spi/intel_spi.c
> > create mode 100644 drivers/gpu/drm/i915/spi/intel_spi.h
> >
> >diff --git a/drivers/gpu/drm/i915/Kconfig
> >b/drivers/gpu/drm/i915/Kconfig index 1e1cb245fca7..abcaa8da45ac 100644
> >--- a/drivers/gpu/drm/i915/Kconfig
> >+++ b/drivers/gpu/drm/i915/Kconfig
> >@@ -26,6 +26,7 @@ config DRM_I915
> > select SND_HDA_I915 if SND_HDA_CORE
> > select CEC_CORE if CEC_NOTIFIER
> > select VMAP_PFN
> >+ select MFD_CORE
> > help
> > Choose this option if you have a system that has "Intel Graphics
> > Media Accelerator" or "HD Graphics" integrated graphics, diff --git
> >a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index
> >235679637d1c..7fa9120feb8d 100644
> >--- a/drivers/gpu/drm/i915/Makefile
> >+++ b/drivers/gpu/drm/i915/Makefile
> >@@ -191,6 +191,9 @@ i915-y += gt/uc/intel_uc.o \
> > gt/uc/intel_huc_debugfs.o \
> > gt/uc/intel_huc_fw.o
> >
> >+# graphics spi device (DGFX) support
> >+i915-y += spi/intel_spi.o
> >+
> > # modesetting core code
> > i915-y += \
> > display/intel_atomic.o \
> >diff --git a/drivers/gpu/drm/i915/i915_drv.c
> >b/drivers/gpu/drm/i915/i915_drv.c index 3edd5e47ad68..48c383d37212
> >100644
> >--- a/drivers/gpu/drm/i915/i915_drv.c
> >+++ b/drivers/gpu/drm/i915/i915_drv.c
> >@@ -38,6 +38,7 @@
> > #include <linux/slab.h>
> > #include <linux/vga_switcheroo.h>
> > #include <linux/vt.h>
> >+#include <linux/mfd/core.h>
> >
> > #include <drm/drm_atomic_helper.h>
> > #include <drm/drm_ioctl.h>
> >@@ -67,6 +68,8 @@
> > #include "gt/intel_gt_pm.h"
> > #include "gt/intel_rc6.h"
> >
> >+#include "spi/intel_spi.h"
> >+
> > #include "i915_debugfs.h"
> > #include "i915_drm_client.h"
> > #include "i915_drv.h"
> >@@ -684,6 +687,8 @@ static void i915_driver_register(struct
> >drm_i915_private *dev_priv)
> >
> > intel_gt_driver_register(&dev_priv->gt);
> >
> >+ intel_spi_init(&dev_priv->spi, dev_priv);
> >+
> > intel_display_driver_register(dev_priv);
>
>
> any reason why this is between gt and display init? I think it would be better
> to call this before gt_driver_register. It doesn't need gt and if eventually
> display also needs to register mfd devices, the unregister counter part of this
> wouldn't be so ackward.
It doesn't really matter, actually gt driver will register additional mfd devices I'm not aware of display mfd devices.
>
> Naming-wise, most of the functions called here are "*_register()".
> Shouldn't we follow suit and name it intel_spi_driver_register()?
It's not a driver we are initializing but a device here. If I remember correctly the name was suggested in internal review.
>
> >
> > intel_power_domains_enable(dev_priv);
> >@@ -710,6 +715,10 @@ static void i915_driver_unregister(struct
> >drm_i915_private *dev_priv)
> >
> > intel_display_driver_unregister(dev_priv);
> >
> >+ mfd_remove_devices(&dev_priv->drm.pdev->dev);
> >+
> >+ intel_spi_fini(&dev_priv->spi);
>
> see below, I don't think we actually need/want this. I'd rather have a
> comment here explaining why we are not "unpeeling the onion" wrt to the
> register function:
The is a result of bad rebase, good catch, the above code is correct.
>
> /*
> * mfd drivers are added separately, but removed all at once
> * from dev_priv
> */
> mfd_remove_devices(&dev_priv->drm.pdev->dev);
>
> >+
> > intel_gt_driver_unregister(&dev_priv->gt);
> >
> > i915_perf_unregister(dev_priv);
> >diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >b/drivers/gpu/drm/i915/i915_drv.h index f8413b3b9da8..f12ec7606d75
> >100644
> >--- a/drivers/gpu/drm/i915/i915_drv.h
> >+++ b/drivers/gpu/drm/i915/i915_drv.h
> >@@ -85,6 +85,8 @@
> > #include "gt/intel_workarounds.h"
> > #include "gt/uc/intel_uc.h"
> >
> >+#include "spi/intel_spi.h"
> >+
> > #include "intel_device_info.h"
> > #include "intel_pch.h"
> > #include "intel_runtime_pm.h"
> >@@ -1117,6 +1119,8 @@ struct drm_i915_private {
> >
> > struct i915_perf perf;
> >
> >+ struct intel_spi spi;
> >+
> > /* Abstract the submission mechanism (legacy ringbuffer or
> execlists) away */
> > struct intel_gt gt;
> >
> >diff --git a/drivers/gpu/drm/i915/i915_reg.h
> >b/drivers/gpu/drm/i915/i915_reg.h index 224ad897af34..426c5dd63673
> >100644
> >--- a/drivers/gpu/drm/i915/i915_reg.h
> >+++ b/drivers/gpu/drm/i915/i915_reg.h
> >@@ -2510,6 +2510,7 @@ static inline bool i915_mmio_reg_valid(i915_reg_t
> reg)
> > #define VEBOX_RING_BASE 0x1a000
> > #define GEN11_VEBOX_RING_BASE 0x1c8000
> > #define GEN11_VEBOX2_RING_BASE 0x1d8000
> >+#define GEN12_GUNIT_SPI_BASE 0x00102040
> > #define BLT_RING_BASE 0x22000
> > #define RING_TAIL(base) _MMIO((base) + 0x30)
> > #define RING_HEAD(base) _MMIO((base) + 0x34)
> >diff --git a/drivers/gpu/drm/i915/spi/intel_spi.c
> >b/drivers/gpu/drm/i915/spi/intel_spi.c
> >new file mode 100644
> >index 000000000000..07da7197bd5d
> >--- /dev/null
> >+++ b/drivers/gpu/drm/i915/spi/intel_spi.c
> >@@ -0,0 +1,53 @@
> >+// SPDX-License-Identifier: MIT
> >+/*
> >+ * Copyright(c) 2019-2021, Intel Corporation. All rights reserved.
> >+ */
> >+
> >+#include <linux/mfd/core.h>
> >+#include <linux/irq.h>
> >+#include "i915_reg.h"
> >+#include "i915_drv.h"
> >+#include "gt/intel_gt.h"
> >+#include "spi/intel_spi.h"
> >+
> >+static const struct resource spi_resources[] = {
> >+ DEFINE_RES_MEM_NAMED(GEN12_GUNIT_SPI_BASE, 0x80, "i915-
> spi-mmio"), };
> >+
> >+static const struct mfd_cell intel_spi_cell = {
> >+ .id = 2,
> >+ .name = "i915-spi",
> >+ .num_resources = ARRAY_SIZE(spi_resources),
> >+ .resources = spi_resources,
> >+};
> >+
> >+void intel_spi_init(struct intel_spi *spi, struct drm_i915_private
> >+*dev_priv) {
> >+ struct pci_dev *pdev = dev_priv->drm.pdev;
> >+ int ret;
> >+
> >+ /* Only the DGFX devices have internal SPI */
> >+ if (!IS_DGFX(dev_priv))
> >+ return;
> >+
> >+ ret = mfd_add_devices(&pdev->dev, PLATFORM_DEVID_AUTO,
> >+ &intel_spi_cell, 1,
> >+ &pdev->resource[0], -1, NULL);
> >+
> >+ if (ret)
> >+ dev_err(&pdev->dev, "creating i915-spi cell failed\n");
> >+
> >+ spi->i915 = dev_priv;
> >+}
> >+
> >+void intel_spi_fini(struct intel_spi *spi) {
> >+ struct pci_dev *pdev;
> >+
> >+ if (!spi->i915)
> >+ return;
> >+
> >+ pdev = spi->i915->drm.pdev;
> >+
> >+ dev_dbg(&pdev->dev, "removing i915-spi cell\n");
>
> this is actually a NOP. I don't think we actually need it since mfd devices are
> removed all at once from the driver
There was intention to use that code, but it's a leftover I will drop it.
>
> >+}
> >diff --git a/drivers/gpu/drm/i915/spi/intel_spi.h
> >b/drivers/gpu/drm/i915/spi/intel_spi.h
> >new file mode 100644
> >index 000000000000..276551fed993
> >--- /dev/null
> >+++ b/drivers/gpu/drm/i915/spi/intel_spi.h
> >@@ -0,0 +1,19 @@
> >+/* SPDX-License-Identifier: MIT */
> >+/*
> >+ * Copyright(c) 2019-2021, Intel Corporation. All rights reserved.
> >+ */
> >+
> >+#ifndef __INTEL_SPI_DEV_H__
> >+#define __INTEL_SPI_DEV_H__
> >+
> >+struct drm_i915_private;
> >+
> >+struct intel_spi {
> >+ struct drm_i915_private *i915;
> >+};
> >+
> >+void intel_spi_init(struct intel_spi *spi, struct drm_i915_private
> >+*i915);
> >+
> >+void intel_spi_fini(struct intel_spi *spi);
> >+
> >+#endif /* __INTEL_SPI_DEV_H__ */
> >--
> >2.26.2
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
