diff mbox series

[v3,14/15] drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back

Message ID 20231102151228.668842-15-andriy.shevchenko@linux.intel.com
State New
Headers
Series drm/i915/dsi: 2nd attempt to get rid of IOSF GPIO |

Commit Message

Andy Shevchenko Nov. 2, 2023, 3:12 p.m. UTC 
  It's a dirty hack in the driver that pokes GPIO registers behind
the driver's back. Moreoever it might be problematic as simultaneous
I/O may hang the system, see the commit 0bd50d719b00 ("pinctrl:
cherryview: prevent concurrent access to GPIO controllers") for
the details. Taking all this into consideration replace the hack
with proper GPIO APIs being used.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 47 +++++---------------
 1 file changed, 10 insertions(+), 37 deletions(-)

Comments

Hans de Goede Nov. 2, 2023, 3:47 p.m. UTC | #1 
Hi,

On 11/2/23 16:12, Andy Shevchenko wrote:
> It's a dirty hack in the driver that pokes GPIO registers behind
> the driver's back. Moreoever it might be problematic as simultaneous
> I/O may hang the system, see the commit 0bd50d719b00 ("pinctrl:
> cherryview: prevent concurrent access to GPIO controllers") for
> the details. Taking all this into consideration replace the hack
> with proper GPIO APIs being used.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 47 +++++---------------
>  1 file changed, 10 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> index b1736c1301ea..ffc65c943b11 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
> @@ -66,19 +66,6 @@ struct i2c_adapter_lookup {
>  #define CHV_GPIO_IDX_START_SW		100
>  #define CHV_GPIO_IDX_START_SE		198
>  
> -#define CHV_VBT_MAX_PINS_PER_FMLY	15
> -
> -#define CHV_GPIO_PAD_CFG0(f, i)		(0x4400 + (f) * 0x400 + (i) * 8)
> -#define  CHV_GPIO_GPIOEN		(1 << 15)
> -#define  CHV_GPIO_GPIOCFG_GPIO		(0 << 8)
> -#define  CHV_GPIO_GPIOCFG_GPO		(1 << 8)
> -#define  CHV_GPIO_GPIOCFG_GPI		(2 << 8)
> -#define  CHV_GPIO_GPIOCFG_HIZ		(3 << 8)
> -#define  CHV_GPIO_GPIOTXSTATE(state)	((!!(state)) << 1)
> -
> -#define CHV_GPIO_PAD_CFG1(f, i)		(0x4400 + (f) * 0x400 + (i) * 8 + 4)
> -#define  CHV_GPIO_CFGLOCK		(1 << 31)
> -
>  /* ICL DSI Display GPIO Pins */
>  #define  ICL_GPIO_DDSP_HPD_A		0
>  #define  ICL_GPIO_L_VDDEN_1		1
> @@ -278,23 +265,21 @@ static void chv_gpio_set_value(struct intel_connector *connector,
>  			       u8 gpio_source, u8 gpio_index, bool value)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> -	u16 cfg0, cfg1;
> -	u16 family_num;
> -	u8 port;
>  
>  	if (connector->panel.vbt.dsi.seq_version >= 3) {
>  		if (gpio_index >= CHV_GPIO_IDX_START_SE) {
>  			/* XXX: it's unclear whether 255->57 is part of SE. */
> -			gpio_index -= CHV_GPIO_IDX_START_SE;
> -			port = CHV_IOSF_PORT_GPIO_SE;
> +			soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:03", "Panel SE",
> +					     gpio_index - CHV_GPIO_IDX_START_SW, value);

The "gpio_index - CHV_GPIO_IDX_START_SW" here needs to be "gpio_index - CHV_GPIO_IDX_START_SE".

Also this patch needs s/soc_exec_opaque_gpio/soc_opaque_gpio_set_value/ to compile ...

Regards,

Hans




>  		} else if (gpio_index >= CHV_GPIO_IDX_START_SW) {
> -			gpio_index -= CHV_GPIO_IDX_START_SW;
> -			port = CHV_IOSF_PORT_GPIO_SW;
> +			soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:00", "Panel SW",
> +					     gpio_index - CHV_GPIO_IDX_START_SW, value);
>  		} else if (gpio_index >= CHV_GPIO_IDX_START_E) {
> -			gpio_index -= CHV_GPIO_IDX_START_E;
> -			port = CHV_IOSF_PORT_GPIO_E;
> +			soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:02", "Panel E",
> +					     gpio_index - CHV_GPIO_IDX_START_E, value);
>  		} else {
> -			port = CHV_IOSF_PORT_GPIO_N;
> +			soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:01", "Panel N",
> +					     gpio_index - CHV_GPIO_IDX_START_N, value);
>  		}
>  	} else {
>  		/* XXX: The spec is unclear about CHV GPIO on seq v2 */
> @@ -311,21 +296,9 @@ static void chv_gpio_set_value(struct intel_connector *connector,
>  			return;
>  		}
>  
> -		port = CHV_IOSF_PORT_GPIO_N;
> +		soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:01", "Panel N",
> +				     gpio_index - CHV_GPIO_IDX_START_N, value);
>  	}
> -
> -	family_num = gpio_index / CHV_VBT_MAX_PINS_PER_FMLY;
> -	gpio_index = gpio_index % CHV_VBT_MAX_PINS_PER_FMLY;
> -
> -	cfg0 = CHV_GPIO_PAD_CFG0(family_num, gpio_index);
> -	cfg1 = CHV_GPIO_PAD_CFG1(family_num, gpio_index);
> -
> -	vlv_iosf_sb_get(dev_priv, BIT(VLV_IOSF_SB_GPIO));
> -	vlv_iosf_sb_write(dev_priv, port, cfg1, 0);
> -	vlv_iosf_sb_write(dev_priv, port, cfg0,
> -			  CHV_GPIO_GPIOEN | CHV_GPIO_GPIOCFG_GPO |
> -			  CHV_GPIO_GPIOTXSTATE(value));
> -	vlv_iosf_sb_put(dev_priv, BIT(VLV_IOSF_SB_GPIO));
>  }
>  
>  static void bxt_gpio_set_value(struct intel_connector *connector,
Andy Shevchenko Nov. 2, 2023, 4:49 p.m. UTC | #2 
On Thu, Nov 02, 2023 at 04:47:41PM +0100, Hans de Goede wrote:
> On 11/2/23 16:12, Andy Shevchenko wrote:

...

> > +			soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:03", "Panel SE",
> > +					     gpio_index - CHV_GPIO_IDX_START_SW, value);
> 
> The "gpio_index - CHV_GPIO_IDX_START_SW" here needs to be "gpio_index - CHV_GPIO_IDX_START_SE".
> 
> Also this patch needs s/soc_exec_opaque_gpio/soc_opaque_gpio_set_value/ to compile ...

Ah, indeed. I looks like I run the test build, but forgot to look into the result. :-(
kernel test robot Nov. 3, 2023, 7:27 p.m. UTC | #3 
Hi Andy,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on drm-intel/for-linux-next-fixes linus/master v6.6 next-20231103]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Andy-Shevchenko/drm-i915-dsi-assume-BXT-gpio-works-for-non-native-GPIO/20231103-064642
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
patch link:    https://lore.kernel.org/r/20231102151228.668842-15-andriy.shevchenko%40linux.intel.com
patch subject: [PATCH v3 14/15] drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20231104/202311040312.Tf6bTkw0-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231104/202311040312.Tf6bTkw0-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311040312.Tf6bTkw0-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/gpu/drm/i915/display/intel_dsi_vbt.c:272:4: error: call to undeclared function 'soc_exec_opaque_gpio'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                           soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:03", "Panel SE",
                           ^
   drivers/gpu/drm/i915/display/intel_dsi_vbt.c:275:4: error: call to undeclared function 'soc_exec_opaque_gpio'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                           soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:00", "Panel SW",
                           ^
   drivers/gpu/drm/i915/display/intel_dsi_vbt.c:278:4: error: call to undeclared function 'soc_exec_opaque_gpio'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                           soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:02", "Panel E",
                           ^
   drivers/gpu/drm/i915/display/intel_dsi_vbt.c:281:4: error: call to undeclared function 'soc_exec_opaque_gpio'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                           soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:01", "Panel N",
                           ^
   drivers/gpu/drm/i915/display/intel_dsi_vbt.c:299:3: error: call to undeclared function 'soc_exec_opaque_gpio'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                   soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:01", "Panel N",
                   ^
   5 errors generated.


vim +/soc_exec_opaque_gpio +272 drivers/gpu/drm/i915/display/intel_dsi_vbt.c

   263	
   264	static void chv_gpio_set_value(struct intel_connector *connector,
   265				       u8 gpio_source, u8 gpio_index, bool value)
   266	{
   267		struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
   268	
   269		if (connector->panel.vbt.dsi.seq_version >= 3) {
   270			if (gpio_index >= CHV_GPIO_IDX_START_SE) {
   271				/* XXX: it's unclear whether 255->57 is part of SE. */
 > 272				soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:03", "Panel SE",
   273						     gpio_index - CHV_GPIO_IDX_START_SW, value);
   274			} else if (gpio_index >= CHV_GPIO_IDX_START_SW) {
   275				soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:00", "Panel SW",
   276						     gpio_index - CHV_GPIO_IDX_START_SW, value);
   277			} else if (gpio_index >= CHV_GPIO_IDX_START_E) {
   278				soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:02", "Panel E",
   279						     gpio_index - CHV_GPIO_IDX_START_E, value);
   280			} else {
   281				soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:01", "Panel N",
   282						     gpio_index - CHV_GPIO_IDX_START_N, value);
   283			}
   284		} else {
   285			/* XXX: The spec is unclear about CHV GPIO on seq v2 */
   286			if (gpio_source != 0) {
   287				drm_dbg_kms(&dev_priv->drm,
   288					    "unknown gpio source %u\n", gpio_source);
   289				return;
   290			}
   291	
   292			if (gpio_index >= CHV_GPIO_IDX_START_E) {
   293				drm_dbg_kms(&dev_priv->drm,
   294					    "invalid gpio index %u for GPIO N\n",
   295					    gpio_index);
   296				return;
   297			}
   298	
   299			soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:01", "Panel N",
   300					     gpio_index - CHV_GPIO_IDX_START_N, value);
   301		}
   302	}
   303
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index b1736c1301ea..ffc65c943b11 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -66,19 +66,6 @@  struct i2c_adapter_lookup {
 #define CHV_GPIO_IDX_START_SW		100
 #define CHV_GPIO_IDX_START_SE		198
 
-#define CHV_VBT_MAX_PINS_PER_FMLY	15
-
-#define CHV_GPIO_PAD_CFG0(f, i)		(0x4400 + (f) * 0x400 + (i) * 8)
-#define  CHV_GPIO_GPIOEN		(1 << 15)
-#define  CHV_GPIO_GPIOCFG_GPIO		(0 << 8)
-#define  CHV_GPIO_GPIOCFG_GPO		(1 << 8)
-#define  CHV_GPIO_GPIOCFG_GPI		(2 << 8)
-#define  CHV_GPIO_GPIOCFG_HIZ		(3 << 8)
-#define  CHV_GPIO_GPIOTXSTATE(state)	((!!(state)) << 1)
-
-#define CHV_GPIO_PAD_CFG1(f, i)		(0x4400 + (f) * 0x400 + (i) * 8 + 4)
-#define  CHV_GPIO_CFGLOCK		(1 << 31)
-
 /* ICL DSI Display GPIO Pins */
 #define  ICL_GPIO_DDSP_HPD_A		0
 #define  ICL_GPIO_L_VDDEN_1		1
@@ -278,23 +265,21 @@  static void chv_gpio_set_value(struct intel_connector *connector,
 			       u8 gpio_source, u8 gpio_index, bool value)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-	u16 cfg0, cfg1;
-	u16 family_num;
-	u8 port;
 
 	if (connector->panel.vbt.dsi.seq_version >= 3) {
 		if (gpio_index >= CHV_GPIO_IDX_START_SE) {
 			/* XXX: it's unclear whether 255->57 is part of SE. */
-			gpio_index -= CHV_GPIO_IDX_START_SE;
-			port = CHV_IOSF_PORT_GPIO_SE;
+			soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:03", "Panel SE",
+					     gpio_index - CHV_GPIO_IDX_START_SW, value);
 		} else if (gpio_index >= CHV_GPIO_IDX_START_SW) {
-			gpio_index -= CHV_GPIO_IDX_START_SW;
-			port = CHV_IOSF_PORT_GPIO_SW;
+			soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:00", "Panel SW",
+					     gpio_index - CHV_GPIO_IDX_START_SW, value);
 		} else if (gpio_index >= CHV_GPIO_IDX_START_E) {
-			gpio_index -= CHV_GPIO_IDX_START_E;
-			port = CHV_IOSF_PORT_GPIO_E;
+			soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:02", "Panel E",
+					     gpio_index - CHV_GPIO_IDX_START_E, value);
 		} else {
-			port = CHV_IOSF_PORT_GPIO_N;
+			soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:01", "Panel N",
+					     gpio_index - CHV_GPIO_IDX_START_N, value);
 		}
 	} else {
 		/* XXX: The spec is unclear about CHV GPIO on seq v2 */
@@ -311,21 +296,9 @@  static void chv_gpio_set_value(struct intel_connector *connector,
 			return;
 		}
 
-		port = CHV_IOSF_PORT_GPIO_N;
+		soc_exec_opaque_gpio(connector, gpio_index, "INT33FF:01", "Panel N",
+				     gpio_index - CHV_GPIO_IDX_START_N, value);
 	}
-
-	family_num = gpio_index / CHV_VBT_MAX_PINS_PER_FMLY;
-	gpio_index = gpio_index % CHV_VBT_MAX_PINS_PER_FMLY;
-
-	cfg0 = CHV_GPIO_PAD_CFG0(family_num, gpio_index);
-	cfg1 = CHV_GPIO_PAD_CFG1(family_num, gpio_index);
-
-	vlv_iosf_sb_get(dev_priv, BIT(VLV_IOSF_SB_GPIO));
-	vlv_iosf_sb_write(dev_priv, port, cfg1, 0);
-	vlv_iosf_sb_write(dev_priv, port, cfg0,
-			  CHV_GPIO_GPIOEN | CHV_GPIO_GPIOCFG_GPO |
-			  CHV_GPIO_GPIOTXSTATE(value));
-	vlv_iosf_sb_put(dev_priv, BIT(VLV_IOSF_SB_GPIO));
 }
 
 static void bxt_gpio_set_value(struct intel_connector *connector,