diff mbox series

[RFC,2/9] drm/i915/spi: intel_spi_region map

Message ID 20210216181925.650082-3-tomas.winkler@intel.com
State New
Delegated to: Ambarus Tudor
Headers show
Series drm/i915/spi: discrete graphics internal spi | expand

Commit Message

Winkler, Tomas Feb. 16, 2021, 6:19 p.m. UTC
Add the dGFX spi region map and convey it via mfd cell platform data
to the spi child device.

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

Comments

Jani Nikula Feb. 17, 2021, 10:46 a.m. UTC | #1
On Tue, 16 Feb 2021, Tomas Winkler <tomas.winkler@intel.com> wrote:
> Add the dGFX spi region map and convey it via mfd cell platform data
> to the spi child device.
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  drivers/gpu/drm/i915/spi/intel_spi.c | 9 +++++++++
>  drivers/gpu/drm/i915/spi/intel_spi.h | 5 +++++
>  2 files changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/spi/intel_spi.c b/drivers/gpu/drm/i915/spi/intel_spi.c
> index 07da7197bd5d..6f83f24f7208 100644
> --- a/drivers/gpu/drm/i915/spi/intel_spi.c
> +++ b/drivers/gpu/drm/i915/spi/intel_spi.c
> @@ -14,11 +14,20 @@ static const struct resource spi_resources[] = {
>  	DEFINE_RES_MEM_NAMED(GEN12_GUNIT_SPI_BASE, 0x80, "i915-spi-mmio"),
>  };
>  
> +static const struct i915_spi_region regions[I915_SPI_REGIONS] = {
> +	[0] = { .name = "DESCRIPTOR", },
> +	[2] = { .name = "GSC", },
> +	[11] = { .name = "OptionROM", },
> +	[12] = { .name = "DAM", },
> +};
> +
>  static const struct mfd_cell intel_spi_cell = {
>  	.id = 2,
>  	.name = "i915-spi",
>  	.num_resources = ARRAY_SIZE(spi_resources),
>  	.resources = spi_resources,
> +	.platform_data = (void *)regions,
> +	.pdata_size    = sizeof(regions),
>  };
>  
>  void intel_spi_init(struct intel_spi *spi, struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/spi/intel_spi.h b/drivers/gpu/drm/i915/spi/intel_spi.h
> index 276551fed993..6b5bf053f7d3 100644
> --- a/drivers/gpu/drm/i915/spi/intel_spi.h
> +++ b/drivers/gpu/drm/i915/spi/intel_spi.h
> @@ -8,6 +8,11 @@
>  
>  struct drm_i915_private;
>  
> +#define I915_SPI_REGIONS 13
> +struct i915_spi_region {
> +	const char *name;
> +};

Does this need to be exposed to the rest of i915? If we're trying to
isolate spi/, I'd prefer it if this header was the only header included
from the rest of i915, and contained the minimum required information.

As the driver has grown bigger, we've tried to minimize the
interconnections between the modules, and it's slow going. Let's try to
keep the new parts isolated.

BR,
Jani.

> +
>  struct intel_spi {
>  	struct drm_i915_private *i915;
>  };
Winkler, Tomas Feb. 17, 2021, 8:45 p.m. UTC | #2
> On Tue, 16 Feb 2021, Tomas Winkler <tomas.winkler@intel.com> wrote:
> > Add the dGFX spi region map and convey it via mfd cell platform data
> > to the spi child device.
> >
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > ---
> >  drivers/gpu/drm/i915/spi/intel_spi.c | 9 +++++++++
> > drivers/gpu/drm/i915/spi/intel_spi.h | 5 +++++
> >  2 files changed, 14 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/spi/intel_spi.c
> > b/drivers/gpu/drm/i915/spi/intel_spi.c
> > index 07da7197bd5d..6f83f24f7208 100644
> > --- a/drivers/gpu/drm/i915/spi/intel_spi.c
> > +++ b/drivers/gpu/drm/i915/spi/intel_spi.c
> > @@ -14,11 +14,20 @@ static const struct resource spi_resources[] = {
> >  	DEFINE_RES_MEM_NAMED(GEN12_GUNIT_SPI_BASE, 0x80, "i915-
> spi-mmio"),
> > };
> >
> > +static const struct i915_spi_region regions[I915_SPI_REGIONS] = {
> > +	[0] = { .name = "DESCRIPTOR", },
> > +	[2] = { .name = "GSC", },
> > +	[11] = { .name = "OptionROM", },
> > +	[12] = { .name = "DAM", },
> > +};
> > +
> >  static const struct mfd_cell intel_spi_cell = {
> >  	.id = 2,
> >  	.name = "i915-spi",
> >  	.num_resources = ARRAY_SIZE(spi_resources),
> >  	.resources = spi_resources,
> > +	.platform_data = (void *)regions,
> > +	.pdata_size    = sizeof(regions),
> >  };
> >
> >  void intel_spi_init(struct intel_spi *spi, struct drm_i915_private
> > *dev_priv) diff --git a/drivers/gpu/drm/i915/spi/intel_spi.h
> > b/drivers/gpu/drm/i915/spi/intel_spi.h
> > index 276551fed993..6b5bf053f7d3 100644
> > --- a/drivers/gpu/drm/i915/spi/intel_spi.h
> > +++ b/drivers/gpu/drm/i915/spi/intel_spi.h
> > @@ -8,6 +8,11 @@
> >
> >  struct drm_i915_private;
> >
> > +#define I915_SPI_REGIONS 13
> > +struct i915_spi_region {
> > +	const char *name;
> > +};
> 
> Does this need to be exposed to the rest of i915? 
This part is between the device which is part of i915 and the driver.
>If we're trying to isolate
> spi/, I'd prefer it if this header was the only header included from the rest of
> i915, and contained the minimum required information.

> As the driver has grown bigger, we've tried to minimize the interconnections
> between the modules, and it's slow going. Let's try to keep the new parts
> isolated.
>
So do you prefer we create another header or duplicate the structure definition? 

Thanks
Tomas
Jani Nikula Feb. 22, 2021, 10:17 a.m. UTC | #3
On Wed, 17 Feb 2021, "Winkler, Tomas" <tomas.winkler@intel.com> wrote:
>> On Tue, 16 Feb 2021, Tomas Winkler <tomas.winkler@intel.com> wrote:
>> > Add the dGFX spi region map and convey it via mfd cell platform data
>> > to the spi child device.
>> >
>> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/spi/intel_spi.c | 9 +++++++++
>> > drivers/gpu/drm/i915/spi/intel_spi.h | 5 +++++
>> >  2 files changed, 14 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/i915/spi/intel_spi.c
>> > b/drivers/gpu/drm/i915/spi/intel_spi.c
>> > index 07da7197bd5d..6f83f24f7208 100644
>> > --- a/drivers/gpu/drm/i915/spi/intel_spi.c
>> > +++ b/drivers/gpu/drm/i915/spi/intel_spi.c
>> > @@ -14,11 +14,20 @@ static const struct resource spi_resources[] = {
>> >  	DEFINE_RES_MEM_NAMED(GEN12_GUNIT_SPI_BASE, 0x80, "i915-
>> spi-mmio"),
>> > };
>> >
>> > +static const struct i915_spi_region regions[I915_SPI_REGIONS] = {
>> > +	[0] = { .name = "DESCRIPTOR", },
>> > +	[2] = { .name = "GSC", },
>> > +	[11] = { .name = "OptionROM", },
>> > +	[12] = { .name = "DAM", },
>> > +};
>> > +
>> >  static const struct mfd_cell intel_spi_cell = {
>> >  	.id = 2,
>> >  	.name = "i915-spi",
>> >  	.num_resources = ARRAY_SIZE(spi_resources),
>> >  	.resources = spi_resources,
>> > +	.platform_data = (void *)regions,
>> > +	.pdata_size    = sizeof(regions),
>> >  };
>> >
>> >  void intel_spi_init(struct intel_spi *spi, struct drm_i915_private
>> > *dev_priv) diff --git a/drivers/gpu/drm/i915/spi/intel_spi.h
>> > b/drivers/gpu/drm/i915/spi/intel_spi.h
>> > index 276551fed993..6b5bf053f7d3 100644
>> > --- a/drivers/gpu/drm/i915/spi/intel_spi.h
>> > +++ b/drivers/gpu/drm/i915/spi/intel_spi.h
>> > @@ -8,6 +8,11 @@
>> >
>> >  struct drm_i915_private;
>> >
>> > +#define I915_SPI_REGIONS 13
>> > +struct i915_spi_region {
>> > +	const char *name;
>> > +};
>> 
>> Does this need to be exposed to the rest of i915? 
> This part is between the device which is part of i915 and the driver.
>>If we're trying to isolate
>> spi/, I'd prefer it if this header was the only header included from the rest of
>> i915, and contained the minimum required information.
>
>> As the driver has grown bigger, we've tried to minimize the interconnections
>> between the modules, and it's slow going. Let's try to keep the new parts
>> isolated.
>>
> So do you prefer we create another header or duplicate the structure definition? 

I didn't see the struct being used in i915, or am I missing something?

Have a header that contains the interface exposed to the rest of i915,
and another header with stuff internal to spi/?

BR,
Jani.
Winkler, Tomas Feb. 22, 2021, 11:30 a.m. UTC | #4
> On Wed, 17 Feb 2021, "Winkler, Tomas" <tomas.winkler@intel.com> wrote:
> >> On Tue, 16 Feb 2021, Tomas Winkler <tomas.winkler@intel.com> wrote:
> >> > Add the dGFX spi region map and convey it via mfd cell platform
> >> > data to the spi child device.
> >> >
> >> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> >> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/spi/intel_spi.c | 9 +++++++++
> >> > drivers/gpu/drm/i915/spi/intel_spi.h | 5 +++++
> >> >  2 files changed, 14 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/spi/intel_spi.c
> >> > b/drivers/gpu/drm/i915/spi/intel_spi.c
> >> > index 07da7197bd5d..6f83f24f7208 100644
> >> > --- a/drivers/gpu/drm/i915/spi/intel_spi.c
> >> > +++ b/drivers/gpu/drm/i915/spi/intel_spi.c
> >> > @@ -14,11 +14,20 @@ static const struct resource spi_resources[] = {
> >> >  	DEFINE_RES_MEM_NAMED(GEN12_GUNIT_SPI_BASE, 0x80, "i915-
> >> spi-mmio"),
> >> > };
> >> >
> >> > +static const struct i915_spi_region regions[I915_SPI_REGIONS] = {
> >> > +	[0] = { .name = "DESCRIPTOR", },
> >> > +	[2] = { .name = "GSC", },
> >> > +	[11] = { .name = "OptionROM", },
> >> > +	[12] = { .name = "DAM", },
> >> > +};
> >> > +
> >> >  static const struct mfd_cell intel_spi_cell = {
> >> >  	.id = 2,
> >> >  	.name = "i915-spi",
> >> >  	.num_resources = ARRAY_SIZE(spi_resources),
> >> >  	.resources = spi_resources,
> >> > +	.platform_data = (void *)regions,
> >> > +	.pdata_size    = sizeof(regions),
> >> >  };
> >> >
> >> >  void intel_spi_init(struct intel_spi *spi, struct drm_i915_private
> >> > *dev_priv) diff --git a/drivers/gpu/drm/i915/spi/intel_spi.h
> >> > b/drivers/gpu/drm/i915/spi/intel_spi.h
> >> > index 276551fed993..6b5bf053f7d3 100644
> >> > --- a/drivers/gpu/drm/i915/spi/intel_spi.h
> >> > +++ b/drivers/gpu/drm/i915/spi/intel_spi.h
> >> > @@ -8,6 +8,11 @@
> >> >
> >> >  struct drm_i915_private;
> >> >
> >> > +#define I915_SPI_REGIONS 13
> >> > +struct i915_spi_region {
> >> > +	const char *name;
> >> > +};
> >>
> >> Does this need to be exposed to the rest of i915?
> > This part is between the device which is part of i915 and the driver.
> >>If we're trying to isolate
> >> spi/, I'd prefer it if this header was the only header included from
> >>the rest of  i915, and contained the minimum required information.
> >
> >> As the driver has grown bigger, we've tried to minimize the
> >> interconnections between the modules, and it's slow going. Let's try
> >> to keep the new parts isolated.
> >>
> > So do you prefer we create another header or duplicate the structure
> definition?
> 
> I didn't see the struct being used in i915, or am I missing something?
This file is part of i915
 
> Have a header that contains the interface exposed to the rest of i915, and
> another header with stuff internal to spi/?

The spi `device` is part of i915 (as a mfd cell) and  struct i915_spi_region is  passed to the spi `driver` as platform_data of the device.
In order for driver to be able to understand  it needs to know ' struct i915_spi_region' 

Thanks
Tomas
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/spi/intel_spi.c b/drivers/gpu/drm/i915/spi/intel_spi.c
index 07da7197bd5d..6f83f24f7208 100644
--- a/drivers/gpu/drm/i915/spi/intel_spi.c
+++ b/drivers/gpu/drm/i915/spi/intel_spi.c
@@ -14,11 +14,20 @@  static const struct resource spi_resources[] = {
 	DEFINE_RES_MEM_NAMED(GEN12_GUNIT_SPI_BASE, 0x80, "i915-spi-mmio"),
 };
 
+static const struct i915_spi_region regions[I915_SPI_REGIONS] = {
+	[0] = { .name = "DESCRIPTOR", },
+	[2] = { .name = "GSC", },
+	[11] = { .name = "OptionROM", },
+	[12] = { .name = "DAM", },
+};
+
 static const struct mfd_cell intel_spi_cell = {
 	.id = 2,
 	.name = "i915-spi",
 	.num_resources = ARRAY_SIZE(spi_resources),
 	.resources = spi_resources,
+	.platform_data = (void *)regions,
+	.pdata_size    = sizeof(regions),
 };
 
 void intel_spi_init(struct intel_spi *spi, struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/spi/intel_spi.h b/drivers/gpu/drm/i915/spi/intel_spi.h
index 276551fed993..6b5bf053f7d3 100644
--- a/drivers/gpu/drm/i915/spi/intel_spi.h
+++ b/drivers/gpu/drm/i915/spi/intel_spi.h
@@ -8,6 +8,11 @@ 
 
 struct drm_i915_private;
 
+#define I915_SPI_REGIONS 13
+struct i915_spi_region {
+	const char *name;
+};
+
 struct intel_spi {
 	struct drm_i915_private *i915;
 };