[Intel-gfx] [PATCH 2/2] drm/i915: GPIO for CHT generic MIPI
Ville Syrjälä
ville.syrjala at linux.intel.com
Thu Feb 25 15:37:22 UTC 2016
On Wed, Feb 24, 2016 at 07:13:46PM +0530, Deepak M wrote:
> From: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu at 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 at linux.intel.com>
> Cc: Jani Nikula <jani.nikula at intel.com>
> Signed-off-by: Yogesh Mohan Marimuthu <yogesh.mohan.marimuthu at intel.com>
> Signed-off-by: Deepak M <m.deepak at 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
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list