[PATCH 0/3] ACPI-PMIC + i915: Add support for PMIC MIPI sequences

[PATCH 0/3] ACPI-PMIC + i915: Add support for PMIC MIPI sequences

[Hans de Goede]

Hi All,

This series is the result of me debugging and fixing the LCD panel not
lighting up on some CHT devices when they are booted with an external
monitor connected and the GOP only initializes the external monitor,
leaving the LCD uninitialized.

This is caused by the lack of support for executing PMIC MIPI sequences
in drivers/gpu/drm/i915/intel_dsi_vbt.c. Specifically the VBT on these
devices uses a PMIC MIPI sequence to toggle a GPIO on the PMIC.

Besides fixing the LCD panel not working at all when an external monitor
is used, this should also save some power when the panel is off.

This series touches some files under drivers/acpi/pmic as well as some
i915 code. Since the code under drivers/acpi/pmic does not change that
often, I believe it would be best to merge this entire series through
intel-drm-next-queued, with acks from the ACPI maintainers.

Regards,

Hans

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/3] ACPI / PMIC: Add support for executing PMIC MIPI sequence elements

[Hans de Goede]

DSI LCD panels describe an initialization sequence in the Video BIOS
Tables using so called MIPI sequences. One possible element in these
sequences is a PMIC specific element of 15 bytes.

Although this is not really an ACPI opregion, the ACPI opregion code is the
closest thing we have. We need to have support for these PMIC specific MIPI
sequence elements somwhere. Since we already instantiate a special platform
device for Intel PMICs for the ACPI PMIC OpRegion handler to bind to,
with PMIC specific implementations of the OpRegion, the handling of MIPI
sequence PMIC elements fits very well in the ACPI PMIC OpRegion code.

This commit adds a new intel_soc_pmic_exec_mipi_pmic_seq_element()
function, which is to be backed by a PMIC specific
exec_mipi_pmic_seq_element callback. This function will be called by the
i915 code to execture MIPI sequence PMIC elements.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/pmic/intel_pmic.c     | 25 +++++++++++++++++++++++++
 drivers/acpi/pmic/intel_pmic.h     |  1 +
 include/linux/mfd/intel_soc_pmic.h |  2 ++
 3 files changed, 28 insertions(+)

diff --git a/drivers/acpi/pmic/intel_pmic.c b/drivers/acpi/pmic/intel_pmic.c
index ca18e0d23df9..0d96ca08bb79 100644
--- a/drivers/acpi/pmic/intel_pmic.c
+++ b/drivers/acpi/pmic/intel_pmic.c
@@ -15,6 +15,7 @@
 
 #include <linux/export.h>
 #include <linux/acpi.h>
+#include <linux/mfd/intel_soc_pmic.h>
 #include <linux/regmap.h>
 #include <acpi/acpi_lpat.h>
 #include "intel_pmic.h"
@@ -36,6 +37,8 @@ struct intel_pmic_opregion {
 	struct intel_pmic_regs_handler_ctx ctx;
 };
 
+static struct intel_pmic_opregion *intel_pmic_opregion;
+
 static int pmic_get_reg_bit(int address, struct pmic_table *table,
 			    int count, int *reg, int *bit)
 {
@@ -304,6 +307,7 @@ int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle,
 	}
 
 	opregion->data = d;
+	intel_pmic_opregion = opregion;
 	return 0;
 
 out_remove_thermal_handler:
@@ -319,3 +323,24 @@ int intel_pmic_install_opregion_handler(struct device *dev, acpi_handle handle,
 	return ret;
 }
 EXPORT_SYMBOL_GPL(intel_pmic_install_opregion_handler);
+
+void intel_soc_pmic_exec_mipi_pmic_seq_element(const u8 *data)
+{
+	struct intel_pmic_opregion_data *d;
+
+	if (!intel_pmic_opregion) {
+		pr_warn("%s: No PMIC registered\n", __func__);
+		return;
+	}
+
+	d = intel_pmic_opregion->data;
+	if (!d->exec_mipi_pmic_seq_element) {
+		pr_warn("%s: Not implemented\n", __func__);
+		return;
+	}
+
+	mutex_lock(&intel_pmic_opregion->lock);
+	d->exec_mipi_pmic_seq_element(intel_pmic_opregion->regmap, data);
+	mutex_unlock(&intel_pmic_opregion->lock);
+}
+EXPORT_SYMBOL_GPL(intel_soc_pmic_exec_mipi_pmic_seq_element);
diff --git a/drivers/acpi/pmic/intel_pmic.h b/drivers/acpi/pmic/intel_pmic.h
index 095afc96952e..5a7bb33d046a 100644
--- a/drivers/acpi/pmic/intel_pmic.h
+++ b/drivers/acpi/pmic/intel_pmic.h
@@ -15,6 +15,7 @@ struct intel_pmic_opregion_data {
 	int (*update_aux)(struct regmap *r, int reg, int raw_temp);
 	int (*get_policy)(struct regmap *r, int reg, int bit, u64 *value);
 	int (*update_policy)(struct regmap *r, int reg, int bit, int enable);
+	void (*exec_mipi_pmic_seq_element)(struct regmap *r, const u8 *data);
 	struct pmic_table *power_table;
 	int power_table_count;
 	struct pmic_table *thermal_table;
diff --git a/include/linux/mfd/intel_soc_pmic.h b/include/linux/mfd/intel_soc_pmic.h
index ed1dfba5e5f9..ce04ad7d4b6c 100644
--- a/include/linux/mfd/intel_soc_pmic.h
+++ b/include/linux/mfd/intel_soc_pmic.h
@@ -26,4 +26,6 @@ struct intel_soc_pmic {
 	struct device *dev;
 };
 
+void intel_soc_pmic_exec_mipi_pmic_seq_element(const u8 *data);
+
 #endif	/* __INTEL_SOC_PMIC_H__ */
-- 
2.19.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] ACPI / PMIC: Implement exec_mipi_pmic_seq_element for CHT Whiskey Cove PMIC

[Hans de Goede]

Implement the exec_mipi_pmic_seq_element callback for the CHT Whiskey Cove
PMIC.

On some CHT devices this fixes the LCD panel not lighting up when it was
not initialized by the GOP, because an external monitor was plugged in and
the GOP initialized only the external monitor.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/acpi/pmic/intel_pmic_chtwc.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/acpi/pmic/intel_pmic_chtwc.c b/drivers/acpi/pmic/intel_pmic_chtwc.c
index 078b0448f30a..d035541f0ed2 100644
--- a/drivers/acpi/pmic/intel_pmic_chtwc.c
+++ b/drivers/acpi/pmic/intel_pmic_chtwc.c
@@ -12,6 +12,7 @@
 #include <linux/mfd/intel_soc_pmic.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <asm/unaligned.h>
 #include "intel_pmic.h"
 
 #define CHT_WC_V1P05A_CTRL		0x6e3b
@@ -231,6 +232,27 @@ static int intel_cht_wc_pmic_update_power(struct regmap *regmap, int reg,
 	return regmap_update_bits(regmap, reg, bitmask, on ? 1 : 0);
 }
 
+static void intel_cht_wc_exec_mipi_pmic_seq_element(struct regmap *regmap,
+						    const u8 *data)
+{
+	u16 i2c_client_address, reg_address, address;
+	u32 value, mask;
+
+	i2c_client_address	= get_unaligned_be16(data);
+	reg_address		= get_unaligned_be16(data + 2);
+	value			= get_unaligned_be32(data + 4);
+	mask			= get_unaligned_be32(data + 8);
+
+	if ((i2c_client_address & 0xff00) || (reg_address & 0xff00)) {
+		pr_warn("%s warning addresses too big client 0x%x reg 0x%x\n",
+			__func__, i2c_client_address, reg_address);
+		return;
+	}
+
+	address = (i2c_client_address << 8) | reg_address;
+	regmap_update_bits(regmap, address, mask, value);
+}
+
 /*
  * The thermal table and ops are empty, we do not support the Thermal opregion
  * (DPTF) due to lacking documentation.
@@ -238,6 +260,7 @@ static int intel_cht_wc_pmic_update_power(struct regmap *regmap, int reg,
 static struct intel_pmic_opregion_data intel_cht_wc_pmic_opregion_data = {
 	.get_power		= intel_cht_wc_pmic_get_power,
 	.update_power		= intel_cht_wc_pmic_update_power,
+	.exec_mipi_pmic_seq_element = intel_cht_wc_exec_mipi_pmic_seq_element,
 	.power_table		= power_table,
 	.power_table_count	= ARRAY_SIZE(power_table),
 };
-- 
2.19.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] ACPI / PMIC: Implement exec_mipi_pmic_seq_element for CHT Whiskey Cove PMIC

[Ville Syrjälä]

On Wed, Dec 05, 2018 at 10:03:09PM +0100, Hans de Goede wrote:
> Implement the exec_mipi_pmic_seq_element callback for the CHT Whiskey Cove
> PMIC.
> 
> On some CHT devices this fixes the LCD panel not lighting up when it was
> not initialized by the GOP, because an external monitor was plugged in and
> the GOP initialized only the external monitor.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/acpi/pmic/intel_pmic_chtwc.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/acpi/pmic/intel_pmic_chtwc.c b/drivers/acpi/pmic/intel_pmic_chtwc.c
> index 078b0448f30a..d035541f0ed2 100644
> --- a/drivers/acpi/pmic/intel_pmic_chtwc.c
> +++ b/drivers/acpi/pmic/intel_pmic_chtwc.c
> @@ -12,6 +12,7 @@
>  #include <linux/mfd/intel_soc_pmic.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
> +#include <asm/unaligned.h>
>  #include "intel_pmic.h"
>  
>  #define CHT_WC_V1P05A_CTRL		0x6e3b
> @@ -231,6 +232,27 @@ static int intel_cht_wc_pmic_update_power(struct regmap *regmap, int reg,
>  	return regmap_update_bits(regmap, reg, bitmask, on ? 1 : 0);
>  }
>  
> +static void intel_cht_wc_exec_mipi_pmic_seq_element(struct regmap *regmap,
> +						    const u8 *data)
> +{
> +	u16 i2c_client_address, reg_address, address;
> +	u32 value, mask;
> +
> +	i2c_client_address	= get_unaligned_be16(data);
> +	reg_address		= get_unaligned_be16(data + 2);
> +	value			= get_unaligned_be32(data + 4);
> +	mask			= get_unaligned_be32(data + 8);

This doesn't match the docs:

"Byte0 – PMIC Flag
 Bits 7:0 = Reserved for future use

 Byte2,1 – PMIC Slave Address
 Bits 15:0 = Slave address for PMIC access
 Each slave can address 256byte register space in general

 Bytes 6,5,4,3 – DWORD 0
 Bits 31:0 = PMIC Register Address

 Bytes 10,9,8,7 – DWORD 1
 Bits 31:0 = PMIC Register Data

 Bytes 14,13,12,11 – DWORD 2
 Bits 31:0 = PMIC Register DataMask"

Though I wouldn't be entirely surprised if the docs are simply
wrong. There is at least one bug in the docs where it claims
the size to be 14 bytes. Another place in the docs says 15 bytes.
I'll need to file a bug for that at least.

> +
> +	if ((i2c_client_address & 0xff00) || (reg_address & 0xff00)) {
> +		pr_warn("%s warning addresses too big client 0x%x reg 0x%x\n",
> +			__func__, i2c_client_address, reg_address);
> +		return;
> +	}
> +
> +	address = (i2c_client_address << 8) | reg_address;
> +	regmap_update_bits(regmap, address, mask, value);
> +}
> +
>  /*
>   * The thermal table and ops are empty, we do not support the Thermal opregion
>   * (DPTF) due to lacking documentation.
> @@ -238,6 +260,7 @@ static int intel_cht_wc_pmic_update_power(struct regmap *regmap, int reg,
>  static struct intel_pmic_opregion_data intel_cht_wc_pmic_opregion_data = {
>  	.get_power		= intel_cht_wc_pmic_get_power,
>  	.update_power		= intel_cht_wc_pmic_update_power,
> +	.exec_mipi_pmic_seq_element = intel_cht_wc_exec_mipi_pmic_seq_element,
>  	.power_table		= power_table,
>  	.power_table_count	= ARRAY_SIZE(power_table),
>  };
> -- 
> 2.19.2

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] ACPI / PMIC: Implement exec_mipi_pmic_seq_element for CHT Whiskey Cove PMIC

[Hans de Goede]

Hi Ville,

Thank you for the review.

On 05-12-18 22:25, Ville Syrjälä wrote:
> On Wed, Dec 05, 2018 at 10:03:09PM +0100, Hans de Goede wrote:
>> Implement the exec_mipi_pmic_seq_element callback for the CHT Whiskey Cove
>> PMIC.
>>
>> On some CHT devices this fixes the LCD panel not lighting up when it was
>> not initialized by the GOP, because an external monitor was plugged in and
>> the GOP initialized only the external monitor.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/acpi/pmic/intel_pmic_chtwc.c | 23 +++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/acpi/pmic/intel_pmic_chtwc.c b/drivers/acpi/pmic/intel_pmic_chtwc.c
>> index 078b0448f30a..d035541f0ed2 100644
>> --- a/drivers/acpi/pmic/intel_pmic_chtwc.c
>> +++ b/drivers/acpi/pmic/intel_pmic_chtwc.c
>> @@ -12,6 +12,7 @@
>>   #include <linux/mfd/intel_soc_pmic.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/regmap.h>
>> +#include <asm/unaligned.h>
>>   #include "intel_pmic.h"
>>   
>>   #define CHT_WC_V1P05A_CTRL		0x6e3b
>> @@ -231,6 +232,27 @@ static int intel_cht_wc_pmic_update_power(struct regmap *regmap, int reg,
>>   	return regmap_update_bits(regmap, reg, bitmask, on ? 1 : 0);
>>   }
>>   
>> +static void intel_cht_wc_exec_mipi_pmic_seq_element(struct regmap *regmap,
>> +						    const u8 *data)
>> +{
>> +	u16 i2c_client_address, reg_address, address;
>> +	u32 value, mask;
>> +
>> +	i2c_client_address	= get_unaligned_be16(data);
>> +	reg_address		= get_unaligned_be16(data + 2);
>> +	value			= get_unaligned_be32(data + 4);
>> +	mask			= get_unaligned_be32(data + 8);
> 
> This doesn't match the docs:

Well I didn't have access to the docs, I just looked at the
data and interpreted that, the first byte being reserved
for future use tripped me up.

> "Byte0 – PMIC Flag
>   Bits 7:0 = Reserved for future use
> 
>   Byte2,1 – PMIC Slave Address
>   Bits 15:0 = Slave address for PMIC access
>   Each slave can address 256byte register space in general
> 
>   Bytes 6,5,4,3 – DWORD 0
>   Bits 31:0 = PMIC Register Address
> 
>   Bytes 10,9,8,7 – DWORD 1
>   Bits 31:0 = PMIC Register Data
> 
>   Bytes 14,13,12,11 – DWORD 2
>   Bits 31:0 = PMIC Register DataMask"
> 
> Though I wouldn't be entirely surprised if the docs are simply
> wrong.

I think the docs are probably right, of the 2 sequences I'm seeing
the OFF sequence has more 00 bytes, so the ON sequence is the most
interesting, this is:

00 6e 00 3c 00 00 00 01 00 00 00 01 00 00 00

With my (big endian) interpretation that gives:

PMIC Slave Address       00 6e
PMIC Register Address    00 3c
PMIC Register Data       00 00 00 01
PMIC Register Mask       00 00 00 01

With the docs slightly shifted (little endian) interpretation that gives:

Reserverd                00
PMIC Slave Address       00 6e
PMIC Register Address    00 00 00 3c
PMIC Register Data       00 00 00 01
PMIC Register Mask       00 00 00 01

So the 2 interpretation agree for the data which I'm actually
seeing used and the docs interpretation actually consumes all bytes.

I'll prepare a v2 of this patch-set using the docs interpretation
of the bytes.

Regards,

Hans





> 
>> +
>> +	if ((i2c_client_address & 0xff00) || (reg_address & 0xff00)) {
>> +		pr_warn("%s warning addresses too big client 0x%x reg 0x%x\n",
>> +			__func__, i2c_client_address, reg_address);
>> +		return;
>> +	}
>> +
>> +	address = (i2c_client_address << 8) | reg_address;
>> +	regmap_update_bits(regmap, address, mask, value);
>> +}
>> +
>>   /*
>>    * The thermal table and ops are empty, we do not support the Thermal opregion
>>    * (DPTF) due to lacking documentation.
>> @@ -238,6 +260,7 @@ static int intel_cht_wc_pmic_update_power(struct regmap *regmap, int reg,
>>   static struct intel_pmic_opregion_data intel_cht_wc_pmic_opregion_data = {
>>   	.get_power		= intel_cht_wc_pmic_get_power,
>>   	.update_power		= intel_cht_wc_pmic_update_power,
>> +	.exec_mipi_pmic_seq_element = intel_cht_wc_exec_mipi_pmic_seq_element,
>>   	.power_table		= power_table,
>>   	.power_table_count	= ARRAY_SIZE(power_table),
>>   };
>> -- 
>> 2.19.2
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/i915/intel_dsi_vbt: Add support for PMIC mipi sequences

[Hans de Goede]

Add support for PMIC mipi sequences using the new
intel_soc_pmic_exec_mipi_pmic_seq_element function.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpu/drm/i915/intel_dsi_vbt.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dsi_vbt.c b/drivers/gpu/drm/i915/intel_dsi_vbt.c
index f27af47c6e49..6a2ed1ca72e0 100644
--- a/drivers/gpu/drm/i915/intel_dsi_vbt.c
+++ b/drivers/gpu/drm/i915/intel_dsi_vbt.c
@@ -29,6 +29,7 @@
 #include <drm/drm_edid.h>
 #include <drm/i915_drm.h>
 #include <linux/gpio/consumer.h>
+#include <linux/mfd/intel_soc_pmic.h>
 #include <linux/slab.h>
 #include <video/mipi_display.h>
 #include <asm/intel-mid.h>
@@ -371,7 +372,11 @@ static const u8 *mipi_exec_spi(struct intel_dsi *intel_dsi, const u8 *data)
 
 static const u8 *mipi_exec_pmic(struct intel_dsi *intel_dsi, const u8 *data)
 {
+#ifdef CONFIG_PMIC_OPREGION
+	intel_soc_pmic_exec_mipi_pmic_seq_element(data);
+#else
 	DRM_DEBUG_KMS("Skipping PMIC element execution\n");
+#endif
 
 	return data + 15;
 }
-- 
2.19.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel