[Intel-gfx] [PATCH v3 1/2] drm/i915: remove confusing GPIO vs PCH_GPIO

[Zhenyu Wang]

On 2018.07.19 17:20:15 +0000, De Marchi, Lucas wrote:
> CC'ing gvt maintainers (and fixing Jani's address in CC).
> 
> See below
> 
> On Wed, 2018-07-18 at 13:01 +0300, Ville Syrjälä wrote:
> > On Tue, Jul 17, 2018 at 03:16:53PM -0700, Lucas De Marchi wrote:
> > > On Fri, Jul 13, 2018 at 9:10 AM Ville Syrjälä
> > > <ville.syrjala at linux.intel.com> wrote:
> > > > 
> > > > On Fri, Jul 13, 2018 at 08:42:11AM -0700, Lucas De Marchi wrote:
> > > > > Instead of defining all registers twice, define just a PCH_GPIO_BASE
> > > > > that has the same address as PCH_GPIO_A and use that to calculate all
> > > > > the others. This also brings VLV and !HAS_GMCH_DISPLAY in line, doing
> > > > > the same thing.
> > > > > 
> > > > > v2: Fix GMBUS registers to be relative to gpio base; create GPIO()
> > > > >     macro to return a particular gpio address and move the enum out of
> > > > >     i915_reg.h (suggested by Jani)
> > > > > 
> > > > > v3: Move base offset inside the GPIO() macro so the GMBUS defines
> > > > > don't
> > > > >     actually need to be changed (suggested by Daniel/Ville)
> > > > > 
> > > > > Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/gvt/handlers.c |  2 +-
> > > > >  drivers/gpu/drm/i915/i915_drv.h     |  3 ++-
> > > > >  drivers/gpu/drm/i915/i915_reg.h     | 24 +++++-------------------
> > > > >  drivers/gpu/drm/i915/intel_drv.h    | 16 ++++++++++++++++
> > > > >  drivers/gpu/drm/i915/intel_i2c.c    | 12 ++++--------
> > > > >  5 files changed, 28 insertions(+), 29 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/gvt/handlers.c
> > > > > b/drivers/gpu/drm/i915/gvt/handlers.c
> > > > > index 7a58ca555197..ecff866bbbf1 100644
> > > > > --- a/drivers/gpu/drm/i915/gvt/handlers.c
> > > > > +++ b/drivers/gpu/drm/i915/gvt/handlers.c
> > > > > @@ -2118,7 +2118,7 @@ static int init_generic_mmio_info(struct
> > > > > intel_gvt *gvt)
> > > > > 
> > > > >       MMIO_F(PCH_GMBUS0, 4 * 4, 0, 0, 0, D_ALL, gmbus_mmio_read,
> > > > >               gmbus_mmio_write);
> > > > > -     MMIO_F(PCH_GPIOA, 6 * 4, F_UNALIGN, 0, 0, D_ALL, NULL, NULL);
> > > > > +     MMIO_F(GPIO(GPIOA), 6 * 4, F_UNALIGN, 0, 0, D_ALL, NULL, NULL);
> > > > 
> > > > I have no idea of gpio_mmio_base is populated correctly at this point
> > > > for gvt, and I'm too lazy to find out.
> > > 
> > > humn, unfortunately it is not
> > > 
> > > i915_driver_load() -> i915_load_modeset_init() -> intel_setup_gmbus()
> > > -> dev_priv->gpio_mmio_base = ...
> > > i915_driver_load() -> i915_driver_init_hw() -> intel_gvt_init() ->
> > > intel_gvt_init_device() -> intel_gvt_setup_mmio_info() ->
> > > init_generic_mmio_info()
> > > 
> > > Is adding a single PCH_GPIO_BASE that doesn't depend on dev_priv being
> > > populated for use on gvt an option?
> > 
> > IIRC gvt already has some local register defines (possibly due to this
> > same reason?). Could add a few more I suppose. +cc the gvt folks to get
> > their input.
> 
> Summary to gvt maintainers: I want to get rid of the multiple GPIO vs PCH_GPIO
> defines we have today. For that I created the GPIO() macro, but it depends on
> dev_priv being populated with the gpio's block offset already which is not
> true while initializing gvt.
>

This also applys to GMBUS change, right?

> I can define a PCH_GPIO_BASE as above in i915_reg.h that is the same as
> PCH_GPIOA today. Or define it in a private gvt header. Do you have another
> option/suggestion?
>

yeah, adding a base define for gvt private seems good to me, as gvt handles
MMIO trap so may need to use them in different way.

thanks

-- 
Open Source Technology Center, Intel ltd.

$gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20180723/4485f4e9/attachment.sig>