| Message ID | 20181213112136.7790-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/3] ACPI / PMIC: Add support for executing PMIC MIPI sequence elements | expand |
On Thu, Dec 13, 2018 at 12:21:35PM +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> > --- > Changes in v2: > -Interpret data passed to the PMIC MIPI elements according to the docs > instead of my own reverse engineered interpretation > Changes in v3: > -Use hex values for out of range checks > -Make intel_cht_wc_exec_mipi_pmic_seq_element return errors > --- > drivers/acpi/pmic/intel_pmic_chtwc.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/acpi/pmic/intel_pmic_chtwc.c b/drivers/acpi/pmic/intel_pmic_chtwc.c > index 078b0448f30a..8ede74e9b89f 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,29 @@ static int intel_cht_wc_pmic_update_power(struct regmap *regmap, int reg, > return regmap_update_bits(regmap, reg, bitmask, on ? 1 : 0); > } > > +static int intel_cht_wc_exec_mipi_pmic_seq_element(struct regmap *regmap, > + const u8 *data) > +{ > + u32 value, mask, reg_address, address; > + u16 i2c_client_address; > + > + /* byte 0 aka PMIC Flag is reserved */ > + i2c_client_address = get_unaligned_le16(data + 1); > + reg_address = get_unaligned_le32(data + 3); > + value = get_unaligned_le32(data + 7); > + mask = get_unaligned_le32(data + 11); Upon further reflection maybe it would better to do this decoding in the i915 code and just pass each parameter to this hook separately? That way we wouldn't be spreading the vbt details all over the place. > + > + if (i2c_client_address > 0xff || reg_address > 0xff) { > + pr_warn("%s warning addresses too big client 0x%x reg 0x%x\n", > + __func__, i2c_client_address, reg_address); > + return -ERANGE; > + } > + > + address = (i2c_client_address << 8) | reg_address; > + > + return 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 +262,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
Hi, On 13-12-18 13:14, Ville Syrjälä wrote: > On Thu, Dec 13, 2018 at 12:21:35PM +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> >> --- >> Changes in v2: >> -Interpret data passed to the PMIC MIPI elements according to the docs >> instead of my own reverse engineered interpretation >> Changes in v3: >> -Use hex values for out of range checks >> -Make intel_cht_wc_exec_mipi_pmic_seq_element return errors >> --- >> drivers/acpi/pmic/intel_pmic_chtwc.c | 25 +++++++++++++++++++++++++ >> 1 file changed, 25 insertions(+) >> >> diff --git a/drivers/acpi/pmic/intel_pmic_chtwc.c b/drivers/acpi/pmic/intel_pmic_chtwc.c >> index 078b0448f30a..8ede74e9b89f 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,29 @@ static int intel_cht_wc_pmic_update_power(struct regmap *regmap, int reg, >> return regmap_update_bits(regmap, reg, bitmask, on ? 1 : 0); >> } >> >> +static int intel_cht_wc_exec_mipi_pmic_seq_element(struct regmap *regmap, >> + const u8 *data) >> +{ >> + u32 value, mask, reg_address, address; >> + u16 i2c_client_address; >> + >> + /* byte 0 aka PMIC Flag is reserved */ >> + i2c_client_address = get_unaligned_le16(data + 1); >> + reg_address = get_unaligned_le32(data + 3); >> + value = get_unaligned_le32(data + 7); >> + mask = get_unaligned_le32(data + 11); > > Upon further reflection maybe it would better to do this decoding in > the i915 code and just pass each parameter to this hook separately? > That way we wouldn't be spreading the vbt details all over the place. Interesting point, if the VBT spec says that this encoding is PMIC independent, then yes we should probably fo the decoding in the VBT code and change the intel_soc_pmic_exec_mipi_pmic_seq_element prototype to: int intel_soc_pmic_exec_mipi_pmic_seq_element(u16 i2c_address, u32 reg_address, u32 value, u32 mask); If you agree please let me know and I will do a v4 of the patchset. I've also been thinking about trying to make the implementation under drivers/acpi/pmic pmic independent, but not all pmic drivers use the regmap the same way. The CHT Whiskey Cove PMIC mfd driver uses a regmap with 16 bit addresses where the upper byte is the i2c client address and the lower byte is the register address (this PMIC listens on multiple addresses, with different registers behind each i2c address). Where as most PMIC mfd drivers simply use the standard devm_regmap_init_i2c() method of creating a regmap. For these others we could do a standard implementation where we check the passed in i2c_address is what we expect (for that type PMIC) and then pass the other 3 parameters to regmap_update_bits. But I think it would be best to wait with such a generic implementation until we encounter a device using the PMIC MIPI sequence element with another type of PMIC. Since we still need the special implementation for the CHT WC case, we still need an operation pointer for this in intel_pmic_opregion_data anyways, so we can easily plug in the generic implementation for others later. Regards, Hans > >> + >> + if (i2c_client_address > 0xff || reg_address > 0xff) { >> + pr_warn("%s warning addresses too big client 0x%x reg 0x%x\n", >> + __func__, i2c_client_address, reg_address); >> + return -ERANGE; >> + } >> + >> + address = (i2c_client_address << 8) | reg_address; >> + >> + return 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 +262,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 >
On Thu, Dec 13, 2018 at 01:40:27PM +0100, Hans de Goede wrote: > Hi, > > On 13-12-18 13:14, Ville Syrjälä wrote: > > On Thu, Dec 13, 2018 at 12:21:35PM +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> > >> --- > >> Changes in v2: > >> -Interpret data passed to the PMIC MIPI elements according to the docs > >> instead of my own reverse engineered interpretation > >> Changes in v3: > >> -Use hex values for out of range checks > >> -Make intel_cht_wc_exec_mipi_pmic_seq_element return errors > >> --- > >> drivers/acpi/pmic/intel_pmic_chtwc.c | 25 +++++++++++++++++++++++++ > >> 1 file changed, 25 insertions(+) > >> > >> diff --git a/drivers/acpi/pmic/intel_pmic_chtwc.c b/drivers/acpi/pmic/intel_pmic_chtwc.c > >> index 078b0448f30a..8ede74e9b89f 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,29 @@ static int intel_cht_wc_pmic_update_power(struct regmap *regmap, int reg, > >> return regmap_update_bits(regmap, reg, bitmask, on ? 1 : 0); > >> } > >> > >> +static int intel_cht_wc_exec_mipi_pmic_seq_element(struct regmap *regmap, > >> + const u8 *data) > >> +{ > >> + u32 value, mask, reg_address, address; > >> + u16 i2c_client_address; > >> + > >> + /* byte 0 aka PMIC Flag is reserved */ > >> + i2c_client_address = get_unaligned_le16(data + 1); > >> + reg_address = get_unaligned_le32(data + 3); > >> + value = get_unaligned_le32(data + 7); > >> + mask = get_unaligned_le32(data + 11); > > > > Upon further reflection maybe it would better to do this decoding in > > the i915 code and just pass each parameter to this hook separately? > > That way we wouldn't be spreading the vbt details all over the place. > > Interesting point, if the VBT spec says that this encoding is PMIC > independent, then yes we should probably fo the decoding in the VBT > code and change the intel_soc_pmic_exec_mipi_pmic_seq_element > prototype to: > > int intel_soc_pmic_exec_mipi_pmic_seq_element(u16 i2c_address, u32 reg_address, > u32 value, u32 mask); > > If you agree please let me know and I will do a v4 of the patchset. Yeah, I think that's probably better. The spec has just the one interpretation for the sequence. > > I've also been thinking about trying to make the implementation > under drivers/acpi/pmic pmic independent, but not all pmic > drivers use the regmap the same way. The CHT Whiskey Cove PMIC > mfd driver uses a regmap with 16 bit addresses where the upper > byte is the i2c client address and the lower byte is the register > address (this PMIC listens on multiple addresses, with different > registers behind each i2c address). > > Where as most PMIC mfd drivers simply use the standard > devm_regmap_init_i2c() method of creating a regmap. For these > others we could do a standard implementation where we check the > passed in i2c_address is what we expect (for that type PMIC) and > then pass the other 3 parameters to regmap_update_bits. > > But I think it would be best to wait with such a generic implementation > until we encounter a device using the PMIC MIPI sequence element > with another type of PMIC. Since we still need the special > implementation for the CHT WC case, we still need an operation > pointer for this in intel_pmic_opregion_data anyways, so we can > easily plug in the generic implementation for others later. Yeah, probably not worth worrying about this until we encounter a machine that needs it. Oh, and we should probably change the DRM_DEBUG_KMS() for the PMIC_OPREGION=n case to a DRM_ERROR() which tells people to enable PMIC_OPREGION=y. Not sure why all these random knobs are even user configurable. No one can really be expected to know how to configure them properly. There was a recent problem with someone having set I2C_DESIGNWARE_BAYTRAIL=n as well because they had a CHT/BSW instead of a BYT :(
On Thu, 13 Dec 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, Dec 13, 2018 at 01:40:27PM +0100, Hans de Goede wrote: >> Hi, >> >> On 13-12-18 13:14, Ville Syrjälä wrote: >> > On Thu, Dec 13, 2018 at 12:21:35PM +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> >> >> --- >> >> Changes in v2: >> >> -Interpret data passed to the PMIC MIPI elements according to the docs >> >> instead of my own reverse engineered interpretation >> >> Changes in v3: >> >> -Use hex values for out of range checks >> >> -Make intel_cht_wc_exec_mipi_pmic_seq_element return errors >> >> --- >> >> drivers/acpi/pmic/intel_pmic_chtwc.c | 25 +++++++++++++++++++++++++ >> >> 1 file changed, 25 insertions(+) >> >> >> >> diff --git a/drivers/acpi/pmic/intel_pmic_chtwc.c b/drivers/acpi/pmic/intel_pmic_chtwc.c >> >> index 078b0448f30a..8ede74e9b89f 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,29 @@ static int intel_cht_wc_pmic_update_power(struct regmap *regmap, int reg, >> >> return regmap_update_bits(regmap, reg, bitmask, on ? 1 : 0); >> >> } >> >> >> >> +static int intel_cht_wc_exec_mipi_pmic_seq_element(struct regmap *regmap, >> >> + const u8 *data) >> >> +{ >> >> + u32 value, mask, reg_address, address; >> >> + u16 i2c_client_address; >> >> + >> >> + /* byte 0 aka PMIC Flag is reserved */ >> >> + i2c_client_address = get_unaligned_le16(data + 1); >> >> + reg_address = get_unaligned_le32(data + 3); >> >> + value = get_unaligned_le32(data + 7); >> >> + mask = get_unaligned_le32(data + 11); >> > >> > Upon further reflection maybe it would better to do this decoding in >> > the i915 code and just pass each parameter to this hook separately? >> > That way we wouldn't be spreading the vbt details all over the place. >> >> Interesting point, if the VBT spec says that this encoding is PMIC >> independent, then yes we should probably fo the decoding in the VBT >> code and change the intel_soc_pmic_exec_mipi_pmic_seq_element >> prototype to: >> >> int intel_soc_pmic_exec_mipi_pmic_seq_element(u16 i2c_address, u32 reg_address, >> u32 value, u32 mask); >> >> If you agree please let me know and I will do a v4 of the patchset. > > Yeah, I think that's probably better. The spec has just the one > interpretation for the sequence. Agreed. BR, Jani. > >> >> I've also been thinking about trying to make the implementation >> under drivers/acpi/pmic pmic independent, but not all pmic >> drivers use the regmap the same way. The CHT Whiskey Cove PMIC >> mfd driver uses a regmap with 16 bit addresses where the upper >> byte is the i2c client address and the lower byte is the register >> address (this PMIC listens on multiple addresses, with different >> registers behind each i2c address). >> >> Where as most PMIC mfd drivers simply use the standard >> devm_regmap_init_i2c() method of creating a regmap. For these >> others we could do a standard implementation where we check the >> passed in i2c_address is what we expect (for that type PMIC) and >> then pass the other 3 parameters to regmap_update_bits. >> >> But I think it would be best to wait with such a generic implementation >> until we encounter a device using the PMIC MIPI sequence element >> with another type of PMIC. Since we still need the special >> implementation for the CHT WC case, we still need an operation >> pointer for this in intel_pmic_opregion_data anyways, so we can >> easily plug in the generic implementation for others later. > > Yeah, probably not worth worrying about this until we > encounter a machine that needs it. > > Oh, and we should probably change the DRM_DEBUG_KMS() for the > PMIC_OPREGION=n case to a DRM_ERROR() which tells people to > enable PMIC_OPREGION=y. Not sure why all these random knobs are > even user configurable. No one can really be expected to know > how to configure them properly. There was a recent problem with > someone having set I2C_DESIGNWARE_BAYTRAIL=n as well because > they had a CHT/BSW instead of a BYT :(
Hi, On 13-12-18 14:08, Ville Syrjälä wrote: > On Thu, Dec 13, 2018 at 01:40:27PM +0100, Hans de Goede wrote: >> Hi, >> >> On 13-12-18 13:14, Ville Syrjälä wrote: <snip> >>>> +static int intel_cht_wc_exec_mipi_pmic_seq_element(struct regmap *regmap, >>>> + const u8 *data) >>>> +{ >>>> + u32 value, mask, reg_address, address; >>>> + u16 i2c_client_address; >>>> + >>>> + /* byte 0 aka PMIC Flag is reserved */ >>>> + i2c_client_address = get_unaligned_le16(data + 1); >>>> + reg_address = get_unaligned_le32(data + 3); >>>> + value = get_unaligned_le32(data + 7); >>>> + mask = get_unaligned_le32(data + 11); >>> >>> Upon further reflection maybe it would better to do this decoding in >>> the i915 code and just pass each parameter to this hook separately? >>> That way we wouldn't be spreading the vbt details all over the place. >> >> Interesting point, if the VBT spec says that this encoding is PMIC >> independent, then yes we should probably fo the decoding in the VBT >> code and change the intel_soc_pmic_exec_mipi_pmic_seq_element >> prototype to: >> >> int intel_soc_pmic_exec_mipi_pmic_seq_element(u16 i2c_address, u32 reg_address, >> u32 value, u32 mask); >> >> If you agree please let me know and I will do a v4 of the patchset. > > Yeah, I think that's probably better. The spec has just the one > interpretation for the sequence. Ok, v4. coming up. > Oh, and we should probably change the DRM_DEBUG_KMS() for the > PMIC_OPREGION=n case to a DRM_ERROR() which tells people to > enable PMIC_OPREGION=y. Will do. Regards, Hans p.s. Have you also seen these 2 DSI related series (I've not see any feedback on these 2 series yet) ? : https://patchwork.kernel.org/patch/10707629/ (patch 1/4) https://patchwork.kernel.org/patch/10707647/ (patch 1/4)
diff --git a/drivers/acpi/pmic/intel_pmic_chtwc.c b/drivers/acpi/pmic/intel_pmic_chtwc.c index 078b0448f30a..8ede74e9b89f 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,29 @@ static int intel_cht_wc_pmic_update_power(struct regmap *regmap, int reg, return regmap_update_bits(regmap, reg, bitmask, on ? 1 : 0); } +static int intel_cht_wc_exec_mipi_pmic_seq_element(struct regmap *regmap, + const u8 *data) +{ + u32 value, mask, reg_address, address; + u16 i2c_client_address; + + /* byte 0 aka PMIC Flag is reserved */ + i2c_client_address = get_unaligned_le16(data + 1); + reg_address = get_unaligned_le32(data + 3); + value = get_unaligned_le32(data + 7); + mask = get_unaligned_le32(data + 11); + + if (i2c_client_address > 0xff || reg_address > 0xff) { + pr_warn("%s warning addresses too big client 0x%x reg 0x%x\n", + __func__, i2c_client_address, reg_address); + return -ERANGE; + } + + address = (i2c_client_address << 8) | reg_address; + + return 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 +262,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), };
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> --- Changes in v2: -Interpret data passed to the PMIC MIPI elements according to the docs instead of my own reverse engineered interpretation Changes in v3: -Use hex values for out of range checks -Make intel_cht_wc_exec_mipi_pmic_seq_element return errors --- drivers/acpi/pmic/intel_pmic_chtwc.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)