Re: [PATCH 2/2] drm/i915: GPIO for CHT generic MIPI

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----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

[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux