> -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@xxxxxxxxxxxxxxx] > Sent: Thursday, February 25, 2016 9:07 PM > To: Deepak, M <m.deepak@xxxxxxxxx> > Cc: intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Mohan Marimuthu, Yogesh > <yogesh.mohan.marimuthu@xxxxxxxxx>; Nikula, Jani > <jani.nikula@xxxxxxxxx> > Subject: Re: [PATCH 2/2] drm/i915: GPIO for CHT generic MIPI > > On Wed, Feb 24, 2016 at 07:13:46PM +0530, Deepak M wrote: > > From: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@xxxxxxxxx> > > > > The GPIO configuration and register offsets are different from > > baytrail for cherrytrail. Port the gpio programming accordingly for > > cherrytrail in this patch. > > > > v2: Removing the duplication of parsing > > > > v3: Moved the macro def to panel_vbt.c file > > > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > Cc: Jani Nikula <jani.nikula@xxxxxxxxx> > > Signed-off-by: Yogesh Mohan Marimuthu > > <yogesh.mohan.marimuthu@xxxxxxxxx> > > Signed-off-by: Deepak M <m.deepak@xxxxxxxxx> > > --- > > drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 123 > > +++++++++++++++++++++++------ > > 1 file changed, 98 insertions(+), 25 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c > > b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c > > index 794bd1f..6b9a1f7 100644 > > --- a/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c > > +++ b/drivers/gpu/drm/i915/intel_dsi_panel_vbt.c > > @@ -58,6 +58,28 @@ static inline struct vbt_panel *to_vbt_panel(struct > > drm_panel *panel) > > > > #define NS_KHZ_RATIO 1000000 > > > > +#define CHV_IOSF_PORT_GPIO_N 0x13 > > +#define CHV_IOSF_PORT_GPIO_SE 0x48 > > +#define CHV_IOSF_PORT_GPIO_SW 0xB2 > > +#define CHV_IOSF_PORT_GPIO_E 0xA8 > > These should have remained where the other ports were defined. > > > +#define CHV_MAX_GPIO_NUM_N 72 > > +#define CHV_MAX_GPIO_NUM_SE 99 > > +#define CHV_MAX_GPIO_NUM_SW 197 > > +#define CHV_MIN_GPIO_NUM_SE 73 > > +#define CHV_MIN_GPIO_NUM_SW 100 > > +#define CHV_MIN_GPIO_NUM_E 198 > > I never got any explanation where the block sizes came from on VLV. > IIRC when I checked them against configdb they didn't match the actual > number of pins in the hardware block. And the same story continues here. > Eg. if I check configfb the number of pins in each block is: > N 59, SE 55, SW 56, E 24. > > So I can't review this until someone explains where this stuff comes from. > And there should probably be a comment next to the defines to remind the > next guy who gets totally confused by this. > > Also I don't like the fact that VLV and CHV are now implemented in two > totally different ways. Can you eliminate the massive gpio table from the VLV > code to make it more similar to this? > [Deepak, M] In CHV the GPIO numberings are sequential but in VLV that is not the case, hence the complete table is copied here. I have attached the VLV GPIO mapping table which can clear your doubts. Pfa, > > + > > +#define CHV_PAD_FMLY_BASE 0x4400 > > +#define CHV_PAD_FMLY_SIZE 0x400 > > +#define CHV_PAD_CFG_0_1_REG_SIZE 0x8 > > +#define CHV_PAD_CFG_REG_SIZE 0x4 > > +#define CHV_VBT_MAX_PINS_PER_FMLY 15 > > I take it this magic 15 must be specified in some VBT spec or something? > > > + > > +#define CHV_GPIO_CFG_UNLOCK 0x00000000 > > +#define CHV_GPIO_CFG_HIZ 0x00008100 > > That's not really hi-z is it? It's GPO mode actually w/ txstate=0. > I would suggest adding separate defines for each bit so it's easier to see what > is really set and what isn't. > > > +#define CHV_GPIO_CFG_TX_STATE_SHIFT 1 > > Could be something like > #define CHV_GPIO_CFG0_TX_STATE(state) ((state) << 1) > > > + > > + > > #define VLV_HV_DDI0_HPD_GPIONC_0_PCONF0 0x4130 > > #define VLV_HV_DDI0_HPD_GPIONC_0_PAD 0x4138 > > #define VLV_HV_DDI0_DDC_SDA_GPIONC_1_PCONF0 0x4120 > > @@ -685,34 +707,13 @@ static const u8 *mipi_exec_delay(struct intel_dsi > *intel_dsi, const u8 *data) > > return data; > > } > > > > -static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 > > *data) > > +void vlv_program_gpio(struct intel_dsi *intel_dsi, u8 gpio, u8 > > +action) > > { > > - u8 gpio, action; > > + struct drm_device *dev = intel_dsi->base.base.dev; > > + struct drm_i915_private *dev_priv = dev->dev_private; > > u16 function, pad; > > u32 val; > > u8 port; > > - struct drm_device *dev = intel_dsi->base.base.dev; > > - struct drm_i915_private *dev_priv = dev->dev_private; > > - > > - DRM_DEBUG_DRIVER("MIPI: executing gpio element\n"); > > - > > - if (dev_priv->vbt.dsi.seq_version >= 3) > > - data++; > > - > > - gpio = *data++; > > - > > - /* pull up/down */ > > - action = *data++ & 1; > > - > > - if (gpio >= ARRAY_SIZE(gtable)) { > > - DRM_DEBUG_KMS("unknown gpio %u\n", gpio); > > - goto out; > > - } > > - > > - if (!IS_VALLEYVIEW(dev_priv)) { > > - DRM_DEBUG_KMS("GPIO element not supported on this > platform\n"); > > - goto out; > > - } > > > > if (dev_priv->vbt.dsi.seq_version >= 3) { > > if (gpio <= IOSF_MAX_GPIO_NUM_NC) { @@ -728,7 +729,7 > @@ static > > const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 *data) > > port = IOSF_PORT_GPIO_SUS; > > } else { > > DRM_ERROR("GPIO number is not present in the > table\n"); > > - goto out; > > + return; > > } > > } else { > > port = IOSF_PORT_GPIO_NC; > > @@ -750,6 +751,78 @@ static const u8 *mipi_exec_gpio(struct intel_dsi > *intel_dsi, const u8 *data) > > /* pull up/down */ > > vlv_iosf_sb_write(dev_priv, port, pad, val); > > mutex_unlock(&dev_priv->sb_lock); > > +} > > + > > +void chv_program_gpio(struct intel_dsi *intel_dsi, u8 gpio, u8 > > +action) { > > + struct drm_device *dev = intel_dsi->base.base.dev; > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + u16 function, pad; > > + u16 family_num; > > + u8 block; > > + > > + if (dev_priv->vbt.dsi.seq_version >= 3) { > > + if (gpio <= CHV_MAX_GPIO_NUM_N) { > > + block = CHV_IOSF_PORT_GPIO_N; > > + DRM_DEBUG_DRIVER("GPIO is in the north > Block\n"); > > + } else if (gpio <= CHV_MAX_GPIO_NUM_SE) { > > + block = CHV_IOSF_PORT_GPIO_SE; > > + gpio = gpio - CHV_MIN_GPIO_NUM_SE; > > + DRM_DEBUG_DRIVER("GPIO is in the south east > Block\n"); > > + } else if (gpio <= CHV_MAX_GPIO_NUM_SW) { > > + block = CHV_IOSF_PORT_GPIO_SW; > > + gpio = gpio - CHV_MIN_GPIO_NUM_SW; > > + DRM_DEBUG_DRIVER("GPIO is in the south west > Block\n"); > > + } else { > > + block = CHV_IOSF_PORT_GPIO_E; > > + gpio = gpio - CHV_MIN_GPIO_NUM_E; > > + DRM_DEBUG_DRIVER("GPIO is in the east Block\n"); > > + } > > + } else > > + block = IOSF_PORT_GPIO_NC; > > + > > + family_num = gpio / CHV_VBT_MAX_PINS_PER_FMLY; > > + gpio = gpio - (family_num * CHV_VBT_MAX_PINS_PER_FMLY); > > Writing this second part with % might make it a bit more obvious. > > > + pad = CHV_PAD_FMLY_BASE + (family_num * CHV_PAD_FMLY_SIZE) > + > > + (((u16)gpio) * CHV_PAD_CFG_0_1_REG_SIZE); > > That could be baked into a neat parametrized define eg. > #define CHV_GPIO_PAD_CFG0(family, gpio) (0x4400 + (family) * 0x400 + > (gpio) * 8) > > > + function = pad + CHV_PAD_CFG_REG_SIZE; > > ditto > #define CHV_GPIO_PAD_CFG1(family, gpio) ... > > > + > > + mutex_lock(&dev_priv->sb_lock); > > + vlv_iosf_sb_write(dev_priv, block, function, > > + CHV_GPIO_CFG_UNLOCK); > > Is it OK to clear all the bits that default to 1? parkmode,hysctl etc. > > > + vlv_iosf_sb_write(dev_priv, block, pad, CHV_GPIO_CFG_HIZ | > > + (action << CHV_GPIO_CFG_TX_STATE_SHIFT)); > > + mutex_unlock(&dev_priv->sb_lock); > > + > > +} > > + > > +static const u8 *mipi_exec_gpio(struct intel_dsi *intel_dsi, const u8 > > +*data) { > > + u8 gpio, action; > > + struct drm_device *dev = intel_dsi->base.base.dev; > > + struct drm_i915_private *dev_priv = dev->dev_private; > > + > > + DRM_DEBUG_DRIVER("MIPI: executing gpio element\n"); > > + > > + if (dev_priv->vbt.dsi.seq_version >= 3) > > + data++; > > + > > + gpio = *data++; > > + > > + /* pull up/down */ > > + action = *data++ & 1; > > + > > + if (gpio >= ARRAY_SIZE(gtable)) { > > + DRM_DEBUG_KMS("unknown gpio %u\n", gpio); > > + goto out; > > + } > > + > > + if (IS_VALLEYVIEW(dev)) > > + vlv_program_gpio(intel_dsi, gpio, action); > > + else if (IS_CHERRYVIEW(dev)) > > + chv_program_gpio(intel_dsi, gpio, action); > > + else > > + DRM_ERROR("GPIO programming missing for this > platform.\n"); > > > > out: > > return data; > > -- > > 1.9.1 > > -- > Ville Syrjälä > Intel OTC
Attachment:
BYT GPIO Mapping Table To be Used In IntelSequenceTool.xlsx
Description: BYT GPIO Mapping Table To be Used In IntelSequenceTool.xlsx
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
