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

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

 



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?

> +
> +#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
_______________________________________________
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