Message ID | 20180525003635.GB2951@ldmartin-desk.amr.corp.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Thu, May 24, 2018 at 05:36:37PM -0700, Lucas De Marchi wrote: > On Thu, May 24, 2018 at 04:42:36PM -0700, Paulo Zanoni wrote: > > From: Mahesh Kumar <mahesh1.kumar@intel.com> > > > > ICP has GPIO pin 1/2 mapped to combo-phy ports & GPIO pins 9/10/11/12 > > mapped to tc ports[1-4]. > > This patch defines GPIOCTL registers for GPIO pins 9-12 & uses them in GPIO > > pin mapping table. > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > > Cc: Madhav Chauhan <madhav.chauhan@intel.com> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > > drivers/gpu/drm/i915/intel_hdmi.c | 2 +- > > drivers/gpu/drm/i915/intel_i2c.c | 12 ++++++------ > > 3 files changed, 11 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 452356a4af07..e48b717769b2 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -3015,6 +3015,10 @@ enum i915_power_well_id { > > #define GPIOF _MMIO(0x5024) > > #define GPIOG _MMIO(0x5028) > > #define GPIOH _MMIO(0x502c) > > +#define GPIOJ _MMIO(0x5034) > > +#define GPIOK _MMIO(0x5038) > > +#define GPIOL _MMIO(0x503C) > > +#define GPIOM _MMIO(0x5040) > > I was reviewing again this and I think again I was puzzled why the spec > has them as 0xc5034, ... > > Probably same conclusion as I had when I first reviewed this. Maybe it > would be nice to add a comment to PCH_GPIO* saying PCH_GPIOA is used > only for calculation the gpio base and remove the rest. I can send this > is a separate patch, what do you think? > > -------8<------- > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 6953419881c4..40b9aa57078b 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -7442,12 +7442,8 @@ enum { > #define PORTE_HOTPLUG_SHORT_DETECT (1 << 0) > #define PORTE_HOTPLUG_LONG_DETECT (2 << 0) > > +/* Used just for calculating the gpio base for PCH */ > #define PCH_GPIOA _MMIO(0xc5010) > -#define PCH_GPIOB _MMIO(0xc5014) > -#define PCH_GPIOC _MMIO(0xc5018) > -#define PCH_GPIOD _MMIO(0xc501c) > -#define PCH_GPIOE _MMIO(0xc5020) > -#define PCH_GPIOF _MMIO(0xc5024) Maybe just have #define PCH_GPIO_BASE ... and throw out the PCH_GPIO/GMBUS defines entirely? > > #define PCH_GMBUS0 _MMIO(0xc5100) > #define PCH_GMBUS1 _MMIO(0xc5104) > -------8<------- > > > Other than that, > > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> > > > # define GPIO_CLOCK_DIR_MASK (1 << 0) > > # define GPIO_CLOCK_DIR_IN (0 << 1) > > # define GPIO_CLOCK_DIR_OUT (1 << 1) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > > index 75f02a0e7d39..3db2459c79b1 100644 > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > @@ -2276,7 +2276,7 @@ static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv, > > ddc_pin = bxt_port_to_ddc_pin(dev_priv, port); > > else if (HAS_PCH_CNP(dev_priv)) > > ddc_pin = cnp_port_to_ddc_pin(dev_priv, port); > > - else if (IS_ICELAKE(dev_priv)) > > + else if (HAS_PCH_ICP(dev_priv)) > > ddc_pin = icl_port_to_ddc_pin(dev_priv, port); > > else > > ddc_pin = g4x_port_to_ddc_pin(dev_priv, port); > > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c > > index e6875509bcd9..b91e418028cb 100644 > > --- a/drivers/gpu/drm/i915/intel_i2c.c > > +++ b/drivers/gpu/drm/i915/intel_i2c.c > > @@ -77,12 +77,12 @@ static const struct gmbus_pin gmbus_pins_cnp[] = { > > }; > > > > static const struct gmbus_pin gmbus_pins_icp[] = { > > - [GMBUS_PIN_1_BXT] = { "dpa", GPIOA }, > > - [GMBUS_PIN_2_BXT] = { "dpb", GPIOB }, > > - [GMBUS_PIN_9_TC1_ICP] = { "tc1", GPIOC }, > > - [GMBUS_PIN_10_TC2_ICP] = { "tc2", GPIOD }, > > - [GMBUS_PIN_11_TC3_ICP] = { "tc3", GPIOE }, > > - [GMBUS_PIN_12_TC4_ICP] = { "tc4", GPIOF }, > > + [GMBUS_PIN_1_BXT] = { "dpa", GPIOB }, > > + [GMBUS_PIN_2_BXT] = { "dpb", GPIOC }, > > + [GMBUS_PIN_9_TC1_ICP] = { "tc1", GPIOJ }, > > + [GMBUS_PIN_10_TC2_ICP] = { "tc2", GPIOK }, > > + [GMBUS_PIN_11_TC3_ICP] = { "tc3", GPIOL }, > > + [GMBUS_PIN_12_TC4_ICP] = { "tc4", GPIOM }, > > }; > > > > /* pin is expected to be valid */ > > -- > > 2.14.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, May 25, 2018 at 07:24:07PM +0300, Ville Syrjälä wrote: > On Thu, May 24, 2018 at 05:36:37PM -0700, Lucas De Marchi wrote: > > On Thu, May 24, 2018 at 04:42:36PM -0700, Paulo Zanoni wrote: > > > From: Mahesh Kumar <mahesh1.kumar@intel.com> > > > > > > ICP has GPIO pin 1/2 mapped to combo-phy ports & GPIO pins 9/10/11/12 > > > mapped to tc ports[1-4]. > > > This patch defines GPIOCTL registers for GPIO pins 9-12 & uses them in GPIO > > > pin mapping table. > > > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > > > Cc: Madhav Chauhan <madhav.chauhan@intel.com> > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > > > drivers/gpu/drm/i915/intel_hdmi.c | 2 +- > > > drivers/gpu/drm/i915/intel_i2c.c | 12 ++++++------ > > > 3 files changed, 11 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > > index 452356a4af07..e48b717769b2 100644 > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > @@ -3015,6 +3015,10 @@ enum i915_power_well_id { > > > #define GPIOF _MMIO(0x5024) > > > #define GPIOG _MMIO(0x5028) > > > #define GPIOH _MMIO(0x502c) > > > +#define GPIOJ _MMIO(0x5034) > > > +#define GPIOK _MMIO(0x5038) > > > +#define GPIOL _MMIO(0x503C) > > > +#define GPIOM _MMIO(0x5040) > > > > I was reviewing again this and I think again I was puzzled why the spec > > has them as 0xc5034, ... > > > > Probably same conclusion as I had when I first reviewed this. Maybe it > > would be nice to add a comment to PCH_GPIO* saying PCH_GPIOA is used > > only for calculation the gpio base and remove the rest. I can send this > > is a separate patch, what do you think? > > > > -------8<------- > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 6953419881c4..40b9aa57078b 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -7442,12 +7442,8 @@ enum { > > #define PORTE_HOTPLUG_SHORT_DETECT (1 << 0) > > #define PORTE_HOTPLUG_LONG_DETECT (2 << 0) > > > > +/* Used just for calculating the gpio base for PCH */ > > #define PCH_GPIOA _MMIO(0xc5010) > > -#define PCH_GPIOB _MMIO(0xc5014) > > -#define PCH_GPIOC _MMIO(0xc5018) > > -#define PCH_GPIOD _MMIO(0xc501c) > > -#define PCH_GPIOE _MMIO(0xc5020) > > -#define PCH_GPIOF _MMIO(0xc5024) > > Maybe just have > > #define PCH_GPIO_BASE ... > > and throw out the PCH_GPIO/GMBUS defines entirely? Yeah just thought about this after sending the email. I will spin a patch with that. thanks Lucas De Marchi > > > > > #define PCH_GMBUS0 _MMIO(0xc5100) > > #define PCH_GMBUS1 _MMIO(0xc5104) > > -------8<------- > > > > > > Other than that, > > > > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> > > > > > # define GPIO_CLOCK_DIR_MASK (1 << 0) > > > # define GPIO_CLOCK_DIR_IN (0 << 1) > > > # define GPIO_CLOCK_DIR_OUT (1 << 1) > > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > > > index 75f02a0e7d39..3db2459c79b1 100644 > > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > > @@ -2276,7 +2276,7 @@ static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv, > > > ddc_pin = bxt_port_to_ddc_pin(dev_priv, port); > > > else if (HAS_PCH_CNP(dev_priv)) > > > ddc_pin = cnp_port_to_ddc_pin(dev_priv, port); > > > - else if (IS_ICELAKE(dev_priv)) > > > + else if (HAS_PCH_ICP(dev_priv)) > > > ddc_pin = icl_port_to_ddc_pin(dev_priv, port); > > > else > > > ddc_pin = g4x_port_to_ddc_pin(dev_priv, port); > > > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c > > > index e6875509bcd9..b91e418028cb 100644 > > > --- a/drivers/gpu/drm/i915/intel_i2c.c > > > +++ b/drivers/gpu/drm/i915/intel_i2c.c > > > @@ -77,12 +77,12 @@ static const struct gmbus_pin gmbus_pins_cnp[] = { > > > }; > > > > > > static const struct gmbus_pin gmbus_pins_icp[] = { > > > - [GMBUS_PIN_1_BXT] = { "dpa", GPIOA }, > > > - [GMBUS_PIN_2_BXT] = { "dpb", GPIOB }, > > > - [GMBUS_PIN_9_TC1_ICP] = { "tc1", GPIOC }, > > > - [GMBUS_PIN_10_TC2_ICP] = { "tc2", GPIOD }, > > > - [GMBUS_PIN_11_TC3_ICP] = { "tc3", GPIOE }, > > > - [GMBUS_PIN_12_TC4_ICP] = { "tc4", GPIOF }, > > > + [GMBUS_PIN_1_BXT] = { "dpa", GPIOB }, > > > + [GMBUS_PIN_2_BXT] = { "dpb", GPIOC }, > > > + [GMBUS_PIN_9_TC1_ICP] = { "tc1", GPIOJ }, > > > + [GMBUS_PIN_10_TC2_ICP] = { "tc2", GPIOK }, > > > + [GMBUS_PIN_11_TC3_ICP] = { "tc3", GPIOL }, > > > + [GMBUS_PIN_12_TC4_ICP] = { "tc4", GPIOM }, > > > }; > > > > > > /* pin is expected to be valid */ > > > -- > > > 2.14.3 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel
On Thu, May 24, 2018 at 05:36:37PM -0700, Lucas De Marchi wrote: > On Thu, May 24, 2018 at 04:42:36PM -0700, Paulo Zanoni wrote: > > From: Mahesh Kumar <mahesh1.kumar@intel.com> > > > > ICP has GPIO pin 1/2 mapped to combo-phy ports & GPIO pins 9/10/11/12 > > mapped to tc ports[1-4]. > > This patch defines GPIOCTL registers for GPIO pins 9-12 & uses them in GPIO > > pin mapping table. > > > > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com> > > Cc: Madhav Chauhan <madhav.chauhan@intel.com> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com> > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > > drivers/gpu/drm/i915/intel_hdmi.c | 2 +- > > drivers/gpu/drm/i915/intel_i2c.c | 12 ++++++------ > > 3 files changed, 11 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index 452356a4af07..e48b717769b2 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -3015,6 +3015,10 @@ enum i915_power_well_id { > > #define GPIOF _MMIO(0x5024) > > #define GPIOG _MMIO(0x5028) > > #define GPIOH _MMIO(0x502c) > > +#define GPIOJ _MMIO(0x5034) > > +#define GPIOK _MMIO(0x5038) > > +#define GPIOL _MMIO(0x503C) > > +#define GPIOM _MMIO(0x5040) > > I was reviewing again this and I think again I was puzzled why the spec > has them as 0xc5034, ... > > Probably same conclusion as I had when I first reviewed this. Maybe it > would be nice to add a comment to PCH_GPIO* saying PCH_GPIOA is used > only for calculation the gpio base and remove the rest. I can send this > is a separate patch, what do you think? yeap... 0xC0000 is magically summed on the gmbus reg access... I spent a while in the past deciphering this... It would be good to have registers and offset mapping to spec to avoid more future readers > > -------8<------- > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 6953419881c4..40b9aa57078b 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -7442,12 +7442,8 @@ enum { > #define PORTE_HOTPLUG_SHORT_DETECT (1 << 0) > #define PORTE_HOTPLUG_LONG_DETECT (2 << 0) > > +/* Used just for calculating the gpio base for PCH */ > #define PCH_GPIOA _MMIO(0xc5010) > -#define PCH_GPIOB _MMIO(0xc5014) > -#define PCH_GPIOC _MMIO(0xc5018) > -#define PCH_GPIOD _MMIO(0xc501c) > -#define PCH_GPIOE _MMIO(0xc5020) > -#define PCH_GPIOF _MMIO(0xc5024) > > #define PCH_GMBUS0 _MMIO(0xc5100) > #define PCH_GMBUS1 _MMIO(0xc5104) > -------8<------- > > > Other than that, > > Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> > > > # define GPIO_CLOCK_DIR_MASK (1 << 0) > > # define GPIO_CLOCK_DIR_IN (0 << 1) > > # define GPIO_CLOCK_DIR_OUT (1 << 1) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > > index 75f02a0e7d39..3db2459c79b1 100644 > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > @@ -2276,7 +2276,7 @@ static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv, > > ddc_pin = bxt_port_to_ddc_pin(dev_priv, port); > > else if (HAS_PCH_CNP(dev_priv)) > > ddc_pin = cnp_port_to_ddc_pin(dev_priv, port); > > - else if (IS_ICELAKE(dev_priv)) > > + else if (HAS_PCH_ICP(dev_priv)) > > ddc_pin = icl_port_to_ddc_pin(dev_priv, port); > > else > > ddc_pin = g4x_port_to_ddc_pin(dev_priv, port); > > diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c > > index e6875509bcd9..b91e418028cb 100644 > > --- a/drivers/gpu/drm/i915/intel_i2c.c > > +++ b/drivers/gpu/drm/i915/intel_i2c.c > > @@ -77,12 +77,12 @@ static const struct gmbus_pin gmbus_pins_cnp[] = { > > }; > > > > static const struct gmbus_pin gmbus_pins_icp[] = { > > - [GMBUS_PIN_1_BXT] = { "dpa", GPIOA }, > > - [GMBUS_PIN_2_BXT] = { "dpb", GPIOB }, > > - [GMBUS_PIN_9_TC1_ICP] = { "tc1", GPIOC }, > > - [GMBUS_PIN_10_TC2_ICP] = { "tc2", GPIOD }, > > - [GMBUS_PIN_11_TC3_ICP] = { "tc3", GPIOE }, > > - [GMBUS_PIN_12_TC4_ICP] = { "tc4", GPIOF }, > > + [GMBUS_PIN_1_BXT] = { "dpa", GPIOB }, > > + [GMBUS_PIN_2_BXT] = { "dpb", GPIOC }, > > + [GMBUS_PIN_9_TC1_ICP] = { "tc1", GPIOJ }, > > + [GMBUS_PIN_10_TC2_ICP] = { "tc2", GPIOK }, > > + [GMBUS_PIN_11_TC3_ICP] = { "tc3", GPIOL }, > > + [GMBUS_PIN_12_TC4_ICP] = { "tc4", GPIOM }, > > }; > > > > /* pin is expected to be valid */ > > -- > > 2.14.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 6953419881c4..40b9aa57078b 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7442,12 +7442,8 @@ enum { #define PORTE_HOTPLUG_SHORT_DETECT (1 << 0) #define PORTE_HOTPLUG_LONG_DETECT (2 << 0) +/* Used just for calculating the gpio base for PCH */ #define PCH_GPIOA _MMIO(0xc5010) -#define PCH_GPIOB _MMIO(0xc5014) -#define PCH_GPIOC _MMIO(0xc5018) -#define PCH_GPIOD _MMIO(0xc501c) -#define PCH_GPIOE _MMIO(0xc5020) -#define PCH_GPIOF _MMIO(0xc5024) #define PCH_GMBUS0 _MMIO(0xc5100) #define PCH_GMBUS1 _MMIO(0xc5104)