[v3,14/15] drm/i915/dsi: Replace poking of CHV GPIOs behind the driver's back
Commit Message
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
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,
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. :-(
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
@@ -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,