| Message ID | 20210216181925.650082-3-tomas.winkler@intel.com |
|---|---|
| State | Not Applicable |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | drm/i915/spi: discrete graphics internal spi | expand |
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; > };
> 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
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.
> 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 --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; };
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(+)