[rft, PATCH v1 0/2] drm/i915/dsi: An attempt to get rid of IOSF GPIO on VLV

[rft, PATCH v1 0/2] drm/i915/dsi: An attempt to get rid of IOSF GPIO on VLV

[Andy Shevchenko]

DSI code for VBT has a set of ugly GPIO hacks, one of which is direct
talking to GPIO IP behind the actual driver's back. An attempt to fix
that is here.

If I understood correctly, my approach should work in the similar way as
the current IOSF GPIO. 

Hans, I believe you have some devices that use this piece of code,
is it possible to give a test run on (one of) them?

Andy Shevchenko (2):
  drm/i915/dsi: Extract common soc_gpio_exec() helper
  drm/i915/dsi: Replace poking of VLV GPIOs behind the driver's back

 drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 150 +++++++------------
 1 file changed, 58 insertions(+), 92 deletions(-)

-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 1/2] drm/i915/dsi: Extract common soc_gpio_exec() helper

[Andy Shevchenko]

Extract a common soc_gpio_exec() helper that may be used by a few SoCs.

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

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index 24b2cbcfc1ef..c3c3f4df9ac4 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -243,6 +243,32 @@ static const u8 *mipi_exec_delay(struct intel_dsi *intel_dsi, const u8 *data)
 	return data;
 }
 
+static void soc_exec_gpio(struct intel_connector *connector, const char *con_id,
+			  u8 gpio_index, bool value)
+{
+	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
+	/* XXX: this table is a quick ugly hack. */
+	static struct gpio_desc *soc_gpio_table[U8_MAX + 1];
+	struct gpio_desc *gpio_desc = soc_gpio_table[gpio_index];
+
+	if (gpio_desc) {
+		gpiod_set_value(gpio_desc, value);
+	} else {
+		gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
+						 con_id, gpio_index,
+						 value ? GPIOD_OUT_LOW :
+						 GPIOD_OUT_HIGH);
+		if (IS_ERR(gpio_desc)) {
+			drm_err(&dev_priv->drm,
+				"GPIO index %u request failed (%pe)\n",
+				gpio_index, gpio_desc);
+			return;
+		}
+
+		soc_gpio_table[gpio_index] = gpio_desc;
+	}
+}
+
 static void vlv_exec_gpio(struct intel_connector *connector,
 			  u8 gpio_source, u8 gpio_index, bool value)
 {
@@ -348,28 +374,7 @@ static void chv_exec_gpio(struct intel_connector *connector,
 static void bxt_exec_gpio(struct intel_connector *connector,
 			  u8 gpio_source, u8 gpio_index, bool value)
 {
-	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-	/* XXX: this table is a quick ugly hack. */
-	static struct gpio_desc *bxt_gpio_table[U8_MAX + 1];
-	struct gpio_desc *gpio_desc = bxt_gpio_table[gpio_index];
-
-	if (!gpio_desc) {
-		gpio_desc = devm_gpiod_get_index(dev_priv->drm.dev,
-						 NULL, gpio_index,
-						 value ? GPIOD_OUT_LOW :
-						 GPIOD_OUT_HIGH);
-
-		if (IS_ERR_OR_NULL(gpio_desc)) {
-			drm_err(&dev_priv->drm,
-				"GPIO index %u request failed (%ld)\n",
-				gpio_index, PTR_ERR(gpio_desc));
-			return;
-		}
-
-		bxt_gpio_table[gpio_index] = gpio_desc;
-	}
-
-	gpiod_set_value(gpio_desc, value);
+	soc_exec_gpio(connector, NULL, gpio_index, value);
 }
 
 static void icl_exec_gpio(struct intel_connector *connector,
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 2/2] drm/i915/dsi: Replace poking of VLV GPIOs behind the driver's back

[Andy Shevchenko]

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 40ecab551232 ("pinctrl:
baytrail: Really serialize all register accesses") 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 | 101 ++++++-------------
 1 file changed, 31 insertions(+), 70 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
index c3c3f4df9ac4..35ab4048029d 100644
--- a/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/display/intel_dsi_vbt.c
@@ -55,43 +55,6 @@
 #define MIPI_VIRTUAL_CHANNEL_SHIFT	1
 #define MIPI_PORT_SHIFT			3
 
-/* base offsets for gpio pads */
-#define VLV_GPIO_NC_0_HV_DDI0_HPD	0x4130
-#define VLV_GPIO_NC_1_HV_DDI0_DDC_SDA	0x4120
-#define VLV_GPIO_NC_2_HV_DDI0_DDC_SCL	0x4110
-#define VLV_GPIO_NC_3_PANEL0_VDDEN	0x4140
-#define VLV_GPIO_NC_4_PANEL0_BKLTEN	0x4150
-#define VLV_GPIO_NC_5_PANEL0_BKLTCTL	0x4160
-#define VLV_GPIO_NC_6_HV_DDI1_HPD	0x4180
-#define VLV_GPIO_NC_7_HV_DDI1_DDC_SDA	0x4190
-#define VLV_GPIO_NC_8_HV_DDI1_DDC_SCL	0x4170
-#define VLV_GPIO_NC_9_PANEL1_VDDEN	0x4100
-#define VLV_GPIO_NC_10_PANEL1_BKLTEN	0x40E0
-#define VLV_GPIO_NC_11_PANEL1_BKLTCTL	0x40F0
-
-#define VLV_GPIO_PCONF0(base_offset)	(base_offset)
-#define VLV_GPIO_PAD_VAL(base_offset)	((base_offset) + 8)
-
-struct gpio_map {
-	u16 base_offset;
-	bool init;
-};
-
-static struct gpio_map vlv_gpio_table[] = {
-	{ VLV_GPIO_NC_0_HV_DDI0_HPD },
-	{ VLV_GPIO_NC_1_HV_DDI0_DDC_SDA },
-	{ VLV_GPIO_NC_2_HV_DDI0_DDC_SCL },
-	{ VLV_GPIO_NC_3_PANEL0_VDDEN },
-	{ VLV_GPIO_NC_4_PANEL0_BKLTEN },
-	{ VLV_GPIO_NC_5_PANEL0_BKLTCTL },
-	{ VLV_GPIO_NC_6_HV_DDI1_HPD },
-	{ VLV_GPIO_NC_7_HV_DDI1_DDC_SDA },
-	{ VLV_GPIO_NC_8_HV_DDI1_DDC_SCL },
-	{ VLV_GPIO_NC_9_PANEL1_VDDEN },
-	{ VLV_GPIO_NC_10_PANEL1_BKLTEN },
-	{ VLV_GPIO_NC_11_PANEL1_BKLTCTL },
-};
-
 struct i2c_adapter_lookup {
 	u16 slave_addr;
 	struct intel_dsi *intel_dsi;
@@ -269,52 +232,44 @@ static void soc_exec_gpio(struct intel_connector *connector, const char *con_id,
 	}
 }
 
+static struct gpiod_lookup_table vlv_gpio_table = {
+	.dev_id = "0000:00:02.0",
+	.table = {
+		GPIO_LOOKUP_IDX("INT33FC:01", 0, "Panel NC", 0, GPIO_ACTIVE_HIGH),
+		GPIO_LOOKUP_IDX("INT33FC:01", 1, "Panel NC", 1, GPIO_ACTIVE_HIGH),
+		GPIO_LOOKUP_IDX("INT33FC:01", 2, "Panel NC", 2, GPIO_ACTIVE_HIGH),
+		GPIO_LOOKUP_IDX("INT33FC:01", 3, "Panel NC", 3, GPIO_ACTIVE_HIGH),
+		GPIO_LOOKUP_IDX("INT33FC:01", 4, "Panel NC", 4, GPIO_ACTIVE_HIGH),
+		GPIO_LOOKUP_IDX("INT33FC:01", 5, "Panel NC", 5, GPIO_ACTIVE_HIGH),
+		GPIO_LOOKUP_IDX("INT33FC:01", 6, "Panel NC", 6, GPIO_ACTIVE_HIGH),
+		GPIO_LOOKUP_IDX("INT33FC:01", 7, "Panel NC", 7, GPIO_ACTIVE_HIGH),
+		GPIO_LOOKUP_IDX("INT33FC:01", 8, "Panel NC", 8, GPIO_ACTIVE_HIGH),
+		GPIO_LOOKUP_IDX("INT33FC:01", 9, "Panel NC", 9, GPIO_ACTIVE_HIGH),
+		GPIO_LOOKUP_IDX("INT33FC:01", 10, "Panel NC", 10, GPIO_ACTIVE_HIGH),
+		GPIO_LOOKUP_IDX("INT33FC:01", 11, "Panel NC", 11, GPIO_ACTIVE_HIGH),
+		{ }
+	},
+};
+
 static void vlv_exec_gpio(struct intel_connector *connector,
 			  u8 gpio_source, u8 gpio_index, bool value)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
-	struct gpio_map *map;
-	u16 pconf0, padval;
-	u32 tmp;
-	u8 port;
 
-	if (gpio_index >= ARRAY_SIZE(vlv_gpio_table)) {
-		drm_dbg_kms(&dev_priv->drm, "unknown gpio index %u\n",
-			    gpio_index);
-		return;
-	}
-
-	map = &vlv_gpio_table[gpio_index];
-
-	if (connector->panel.vbt.dsi.seq_version >= 3) {
-		/* XXX: this assumes vlv_gpio_table only has NC GPIOs. */
-		port = IOSF_PORT_GPIO_NC;
-	} else {
-		if (gpio_source == 0) {
-			port = IOSF_PORT_GPIO_NC;
-		} else if (gpio_source == 1) {
+	/* XXX: this assumes vlv_gpio_table only has NC GPIOs. */
+	if (connector->panel.vbt.dsi.seq_version < 3) {
+		if (gpio_source == 1) {
 			drm_dbg_kms(&dev_priv->drm, "SC gpio not supported\n");
 			return;
-		} else {
+		}
+		if (gpio_source > 1) {
 			drm_dbg_kms(&dev_priv->drm,
 				    "unknown gpio source %u\n", gpio_source);
 			return;
 		}
 	}
 
-	pconf0 = VLV_GPIO_PCONF0(map->base_offset);
-	padval = VLV_GPIO_PAD_VAL(map->base_offset);
-
-	vlv_iosf_sb_get(dev_priv, BIT(VLV_IOSF_SB_GPIO));
-	if (!map->init) {
-		/* FIXME: remove constant below */
-		vlv_iosf_sb_write(dev_priv, port, pconf0, 0x2000CC00);
-		map->init = true;
-	}
-
-	tmp = 0x4 | value;
-	vlv_iosf_sb_write(dev_priv, port, padval, tmp);
-	vlv_iosf_sb_put(dev_priv, BIT(VLV_IOSF_SB_GPIO));
+	soc_exec_gpio(connector, "Panel NC", gpio_index, value);
 }
 
 static void chv_exec_gpio(struct intel_connector *connector,
@@ -974,6 +929,9 @@ void intel_dsi_vbt_gpio_init(struct intel_dsi *intel_dsi, bool panel_is_on)
 	struct pinctrl *pinctrl;
 	int ret;
 
+	if (IS_VALLEYVIEW(dev_priv))
+		gpiod_add_lookup_table(&vlv_gpio_table);
+
 	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
 	    mipi_config->pwm_blc == PPS_BLC_PMIC) {
 		gpiod_add_lookup_table(&pmic_panel_gpio_table);
@@ -1043,4 +1001,7 @@ void intel_dsi_vbt_gpio_cleanup(struct intel_dsi *intel_dsi)
 		pinctrl_unregister_mappings(soc_pwm_pinctrl_map);
 		gpiod_remove_lookup_table(&soc_panel_gpio_table);
 	}
+
+	if (IS_VALLEYVIEW(dev_priv))
+		gpiod_remove_lookup_table(&vlv_gpio_table);
 }
-- 
2.40.0.1.gaa8946217a0b

Re: [rft, PATCH v1 0/2] drm/i915/dsi: An attempt to get rid of IOSF GPIO on VLV

[Hans de Goede]

Hi Andy,

On 10/18/23 07:10, Andy Shevchenko wrote:
> DSI code for VBT has a set of ugly GPIO hacks, one of which is direct
> talking to GPIO IP behind the actual driver's back. An attempt to fix
> that is here.
> 
> If I understood correctly, my approach should work in the similar way as
> the current IOSF GPIO. 
> 
> Hans, I believe you have some devices that use this piece of code,
> is it possible to give a test run on (one of) them?

Yes I should be able to find a device or 2 which poke GPIOs from the
VBT MIPI sequences. Unfortunately I don't know from the top of my head
which devices actually use this, so I may need to try quite a few devices
before finding one which actually uses this.

I'll try to get this series tested sometime the coming weeks,
depending on when I can schedule some time for this.

Regards,

Hans




> 
> Andy Shevchenko (2):
>   drm/i915/dsi: Extract common soc_gpio_exec() helper
>   drm/i915/dsi: Replace poking of VLV GPIOs behind the driver's back
> 
>  drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 150 +++++++------------
>  1 file changed, 58 insertions(+), 92 deletions(-)
> 

Re: [rft, PATCH v1 0/2] drm/i915/dsi: An attempt to get rid of IOSF GPIO on VLV

[Andy Shevchenko]

On Wed, Oct 18, 2023 at 11:09:35AM +0200, Hans de Goede wrote:
> On 10/18/23 07:10, Andy Shevchenko wrote:
> > DSI code for VBT has a set of ugly GPIO hacks, one of which is direct
> > talking to GPIO IP behind the actual driver's back. An attempt to fix
> > that is here.
> > 
> > If I understood correctly, my approach should work in the similar way as
> > the current IOSF GPIO. 
> > 
> > Hans, I believe you have some devices that use this piece of code,
> > is it possible to give a test run on (one of) them?
> 
> Yes I should be able to find a device or 2 which poke GPIOs from the
> VBT MIPI sequences. Unfortunately I don't know from the top of my head
> which devices actually use this, so I may need to try quite a few devices
> before finding one which actually uses this.
> 
> I'll try to get this series tested sometime the coming weeks,
> depending on when I can schedule some time for this.

No hurry. maybe you simply can add into your usual tree you run on your
devices?

-- 
With Best Regards,
Andy Shevchenko

Re: [rft, PATCH v1 0/2] drm/i915/dsi: An attempt to get rid of IOSF GPIO on VLV

[Andy Shevchenko]

On Wed, Oct 18, 2023 at 03:52:36PM +0300, Andy Shevchenko wrote:
> On Wed, Oct 18, 2023 at 11:09:35AM +0200, Hans de Goede wrote:
> > On 10/18/23 07:10, Andy Shevchenko wrote:

...

> > Yes I should be able to find a device or 2 which poke GPIOs from the
> > VBT MIPI sequences. Unfortunately I don't know from the top of my head
> > which devices actually use this, so I may need to try quite a few devices
> > before finding one which actually uses this.
> > 
> > I'll try to get this series tested sometime the coming weeks,
> > depending on when I can schedule some time for this.
> 
> No hurry. maybe you simply can add into your usual tree you run on your
> devices?

FYI, I have just sent a v2, which includes CHV conversion.

-- 
With Best Regards,
Andy Shevchenko