[Intel-gfx] [PATCH 2/2] drm/i915: GPIO for CHT generic MIPI
Deepak, M
m.deepak at intel.com
Mon Feb 29 11:00:34 UTC 2016
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> Sent: Thursday, February 25, 2016 9:07 PM
> To: Deepak, M <m.deepak at intel.com>
> Cc: intel-gfx at lists.freedesktop.org; Mohan Marimuthu, Yogesh
> <yogesh.mohan.marimuthu at intel.com>; Nikula, Jani
> <jani.nikula at 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 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?
>
[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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: BYT GPIO Mapping Table To be Used In IntelSequenceTool.xlsx
Type: application/vnd.openxmlformats-officedocument.spreadsheetml.sheet
Size: 29439 bytes
Desc: BYT GPIO Mapping Table To be Used In IntelSequenceTool.xlsx
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20160229/49e6f8ec/attachment-0001.xlsx>
More information about the Intel-gfx
mailing list