[2/2] drm/i915: GPIO for CHT generic MIPI
diff mbox

Message ID 1456321426-28074-2-git-send-email-m.deepak@intel.com
State New
Headers show

Commit Message

Deepak M Feb. 24, 2016, 1:43 p.m. UTC
From: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>

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@linux.intel.com>
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
Signed-off-by: Deepak M <m.deepak@intel.com>
---
 drivers/gpu/drm/i915/intel_dsi_panel_vbt.c | 123 +++++++++++++++++++++++------
 1 file changed, 98 insertions(+), 25 deletions(-)

Comments

Ville Syrjälä Feb. 25, 2016, 3:37 p.m. UTC | #1
On Wed, Feb 24, 2016 at 07:13:46PM +0530, Deepak M wrote:
> From: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
> 
> 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@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu@intel.com>
> Signed-off-by: Deepak M <m.deepak@intel.com>
> ---
>  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?

> +
> +#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
Deepak M Feb. 29, 2016, 11 a.m. UTC | #2
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Thursday, February 25, 2016 9:07 PM
> To: Deepak, M <m.deepak@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Mohan Marimuthu, Yogesh
> <yogesh.mohan.marimuthu@intel.com>; Nikula, Jani
> <jani.nikula@intel.com>
> 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@intel.com>
> >
> > 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@linux.intel.com>
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Yogesh Mohan Marimuthu
> > <yogesh.mohan.marimuthu@intel.com>
> > Signed-off-by: Deepak M <m.deepak@intel.com>
> > ---
> >  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
Ville Syrjälä Feb. 29, 2016, 1:50 p.m. UTC | #3
On Mon, Feb 29, 2016 at 11:00:34AM +0000, Deepak, M wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Thursday, February 25, 2016 9:07 PM
> > To: Deepak, M <m.deepak@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; Mohan Marimuthu, Yogesh
> > <yogesh.mohan.marimuthu@intel.com>; Nikula, Jani
> > <jani.nikula@intel.com>
> > 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@intel.com>
> > >
> > > 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@linux.intel.com>
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Signed-off-by: Yogesh Mohan Marimuthu
> > > <yogesh.mohan.marimuthu@intel.com>
> > > Signed-off-by: Deepak M <m.deepak@intel.com>
> > > ---
> > >  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, 

Any chance someone could try to get this table included in the spec, or
at least have a link to it? Having the information spread around this
way is not productive.
Deepak M Feb. 29, 2016, 3:07 p.m. UTC | #4
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Monday, February 29, 2016 7:20 PM
> To: Deepak, M <m.deepak@intel.com>
> Cc: intel-gfx@lists.freedesktop.org; Mohan Marimuthu, Yogesh
> <yogesh.mohan.marimuthu@intel.com>; Nikula, Jani
> <jani.nikula@intel.com>
> Subject: Re: [PATCH 2/2] drm/i915: GPIO for CHT generic MIPI
> 
> On Mon, Feb 29, 2016 at 11:00:34AM +0000, Deepak, M wrote:
> >
> >
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > > Sent: Thursday, February 25, 2016 9:07 PM
> > > To: Deepak, M <m.deepak@intel.com>
> > > Cc: intel-gfx@lists.freedesktop.org; Mohan Marimuthu, Yogesh
> > > <yogesh.mohan.marimuthu@intel.com>; Nikula, Jani
> > > <jani.nikula@intel.com>
> > > 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@intel.com>
> > > >
> > > > 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@linux.intel.com>
> > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > Signed-off-by: Yogesh Mohan Marimuthu
> > > > <yogesh.mohan.marimuthu@intel.com>
> > > > Signed-off-by: Deepak M <m.deepak@intel.com>
> > > > ---
> > > >  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,
> 
> Any chance someone could try to get this table included in the spec, or at
> least have a link to it? Having the information spread around this way is not
> productive.
[Deepak, M] Sure, will try to put this doc in sharepoint. 
> 
> --
> Ville Syrjälä
> Intel OTC

Patch
diff mbox

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
+#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
+
+#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
+
+#define CHV_GPIO_CFG_UNLOCK                    0x00000000
+#define CHV_GPIO_CFG_HIZ                       0x00008100
+#define CHV_GPIO_CFG_TX_STATE_SHIFT            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);
+	pad = CHV_PAD_FMLY_BASE + (family_num * CHV_PAD_FMLY_SIZE) +
+		(((u16)gpio) * CHV_PAD_CFG_0_1_REG_SIZE);
+	function = pad + CHV_PAD_CFG_REG_SIZE;
+
+	mutex_lock(&dev_priv->sb_lock);
+	vlv_iosf_sb_write(dev_priv, block, function,
+			CHV_GPIO_CFG_UNLOCK);
+	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;