[PATCH 0/7] coordinate cht i2c-pmic and i915-punit accesses

[PATCH 0/7] coordinate cht i2c-pmic and i915-punit accesses

[Hans de Goede]

Hi All,

Here is a non RFC version of my previous RFC series for coordinating
cht i2c-pmic and i915-punit accesses. New in this non RFC version is
that forcewake accesses are coodinated now too.

The problem:
============

This series fixes an issue where the following messages are shown in dmesg:
[drm:fw_domains_get [i915]] *ERROR* render: timed out waiting for forcewake ack request.
[drm:fw_domains_get [i915]] *MEDIA* render: timed out waiting for forcewake ack request.
i2c_designware 808622C1:06: punit semaphore timed out, resetting
i2c_designware 808622C1:06: PUNIT SEM: 2
i2c_designware 808622C1:06: couldn't acquire bus ownership

Usually followed by a gfx or system freeze, this is happening both on my
cherrytrail tablet as well as being seen by an user commenting on:
https://bugzilla.kernel.org/show_bug.cgi?id=155241

This problem is triggered by my patch series to fix the punit i2c bus
semaphore code in drivers/i2c/busses/designware-baytrail.c :

https://patchwork.ozlabs.org/patch/710067/
https://patchwork.ozlabs.org/patch/710068/
https://patchwork.ozlabs.org/patch/710069/
https://patchwork.ozlabs.org/patch/710070/
https://patchwork.ozlabs.org/patch/710071/
https://patchwork.ozlabs.org/patch/710072/

Which actually allows i2c access to pmic-s wich need bus-access arbritration
for the first time on cherrytrail, combined with patches from me to actually
make the axp288_fuel_gauge driver load, which leads to regular pmic i2c
accesses (when userspace checks the battery level).

The fix / workaround
====================

Testing has shown that just taking the punit i2c bus semaphore on
i2c accesses to a shared pmic i2c bus is not enough to avoid problem,
e.g. besides this series to coordinate i915 access, we also need:
https://patchwork.ozlabs.org/patch/710070/

This series introduces 2 mechanisms to coordinate direct pmic accesses
vs punit requests which lead to pmic accesses. The first mechanism is
a simple mutex, which works well for the direct punit accesses the i915
code does. However testing has shown this is not enough, we also need
to avoid doing a forcewake when a driver is directly accessing the pmic
i2c bus, otherwise we get the timeout errors quoted above, the fact
that after the forcewake errors the designware-baytrail.c code also
fails to acquire the lock, suggests that doing forcewakes during the
access window causes the i2c bus to get stuck (or the punit to get
confused).

The deal with the forcewake issue a notifier is introduced which allows
drivers to get notified before any pmic i2c transfer begins (and after
it ends). This is used in the i915 driver to do a
forcewake_get(FORCEWAKE_ALL) before the access begins avoiding needing to
it while the access is in progress.

Testing has shown that this avoids the problem for both me and the user,
where as before it could be reproduced at will.

The implementation of the fix
=============================

I've decided to put the mutex and the notifier in the iosf_mbi code,
as that is somewhat of a common middle ground between the i915 driver and
the designware-baytrail.c code, with the latter already depending on the
iosf_mbi code. I'm open for moving them if someone has a suggestion for a
better place to put them.

If iosf_mbi support is not enabled then all the functions used will become
static inline NOPs and nothing changes on the i915 side.

If the code is enabled and runs on a system which does not need pmic i2c
bus arbritration, then the mutex lock / unlock calls added to the i915 code
will still be made, but there won't be any contention, so other then the
overhead of the lock / unlock there will not be any delays. These calls are
not made in any hot paths.

The biggest downside IMHO is that taking FORCEWAKE_ALL before each bus
access will cause extra GPU wakeups, and thus powerdrain. I plan to make
the impact of this minimal by modifying any drivers (axp288 code, with
which I'm quite familiar by now) to limit their i2c accesses to a bare
minimum. Specifically I plan to e.g. cache the battery level and wait
at least 15 seconds (max 4 times a minute) before getting a fresh reading
even if userspace asks for it more often, combined with some mechanism
to only do 1 access-begin and 1 access-end notification for i2c accesses
in quick succession.

Alternatives
============

It would be tempting to say that this is not worth the trouble and we
should just disallow direct i2c accesses to the pmic bus. Unfortunately
that will cause some major issues on affected devices:
-No battery monitoring
-No "AC" plugged in monitoring
-If booted with a normal USB-A -> micro-USB cable, or no cable, plugged
 in and then the user replaces the cable with an otg USB-host cable /
 adapter, the id-pin shorting will enable a 5v boost convertor, but we
 need to disable the pmic's USB-Vbus path otherwise it will start drawing
 current from the boost convertor, leading to aprox 300mA of extra
 battery drain, this is done by the axp288_charger driver, which needs
 direct i2c access to the pmic bus

Way forward
===========

I realize the workaround this patch-set adds is not the prettiest code
ever seen, but I see no otherway to get things like battery monitoring
and proper OTG support working on affected devices without breaking
drm/i915 support. So I would like to see this merged in some form, I'm
happy to make any kind of requested changes; and/or to try alternative
fixes.

Regards,

Hans

[PATCH 1/7] x86/platform/intel/iosf_mbi: Add a mutex for punit access

[Hans de Goede]

One some systems the punit accesses the pmic to change various voltages
through the same bus as other kernel drivers use for e.g. battery
monitoring.

If a driver sends requests to the punit which require the punit to access
the pmic bus while another driver is also accessing the pmic bus various
bad things happen.

This commit adds a mutex to protect the punit against simultaneous accesses
and 2 functions to lock / unlock this mutex.

Note on these systems the i2c-bus driver will request a sempahore from the
punit for exclusive access to the pmic bus when i2c drivers are accessing
it, but this does not appear to be sufficient, we still need to avoid
making certain punit requests during the access window to avoid problems.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
---
 arch/x86/include/asm/iosf_mbi.h    | 31 +++++++++++++++++++++++++++++++
 arch/x86/platform/intel/iosf_mbi.c | 13 +++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
index b41ee16..91f5d16 100644
--- a/arch/x86/include/asm/iosf_mbi.h
+++ b/arch/x86/include/asm/iosf_mbi.h
@@ -88,6 +88,33 @@ int iosf_mbi_write(u8 port, u8 opcode, u32 offset, u32 mdr);
  */
 int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask);
 
+/**
+ * iosf_mbi_punit_lock() - Lock the punit mutex
+ *
+ * One some systems the punit accesses the pmic to change various voltages
+ * through the same bus as other kernel drivers use for e.g. battery monitoring.
+ *
+ * If a driver sends requests to the punit which require the punit to access the
+ * pmic bus while another driver is also accessing the pmic bus various bad
+ * things happen.
+ *
+ * To avoid these problems this function must be called before accessing the
+ * punit or the pmic, be it through iosf_mbi* functions or through other means.
+ *
+ * Note on these systems the i2c-bus driver will request a sempahore from the
+ * punit for exclusive access to the pmic bus when i2c drivers are accessing it,
+ * but this does not appear to be sufficient, we still need to avoid making
+ * certain punit requests during the access window to avoid problems.
+ *
+ * This function locks a mutex, as such it may sleep.
+ */
+void iosf_mbi_punit_lock(void);
+
+/**
+ * iosf_mbi_punit_unlock() - Unlock the punit mutex
+ */
+void iosf_mbi_punit_unlock(void);
+
 #else /* CONFIG_IOSF_MBI is not enabled */
 static inline
 bool iosf_mbi_available(void)
@@ -115,6 +142,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask)
 	WARN(1, "IOSF_MBI driver not available");
 	return -EPERM;
 }
+
+static inline void iosf_mbi_punit_lock(void) {}
+static inline void iosf_mbi_punit_unlock(void) {}
+
 #endif /* CONFIG_IOSF_MBI */
 
 #endif /* IOSF_MBI_SYMS_H */
diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c
index edf2c54..75d8135 100644
--- a/arch/x86/platform/intel/iosf_mbi.c
+++ b/arch/x86/platform/intel/iosf_mbi.c
@@ -34,6 +34,7 @@
 
 static struct pci_dev *mbi_pdev;
 static DEFINE_SPINLOCK(iosf_mbi_lock);
+static DEFINE_MUTEX(iosf_mbi_punit_mutex);
 
 static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
 {
@@ -190,6 +191,18 @@ bool iosf_mbi_available(void)
 }
 EXPORT_SYMBOL(iosf_mbi_available);
 
+void iosf_mbi_punit_lock(void)
+{
+	mutex_lock(&iosf_mbi_punit_mutex);
+}
+EXPORT_SYMBOL(iosf_mbi_punit_lock);
+
+void iosf_mbi_punit_unlock(void)
+{
+	mutex_unlock(&iosf_mbi_punit_mutex);
+}
+EXPORT_SYMBOL(iosf_mbi_punit_unlock);
+
 #ifdef CONFIG_IOSF_MBI_DEBUG
 static u32	dbg_mdr;
 static u32	dbg_mcr;
-- 
2.9.3

[PATCH 2/7] x86/platform/intel/iosf_mbi: Add a pmic bus access notifier

[Hans de Goede]

Some drivers may need to acquire punit managed resources from interrupt
context, where they cannot call iosf_mbi_punit_lock().

This commit adds a notifier chain which allows a driver to get notified
(in a process context) before other drivers start accessing the pmic bus,
so that the driver can acquire any resources, which it may need during
the window the other driver is accessing the pmic, before hand.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
---
 arch/x86/include/asm/iosf_mbi.h    | 54 ++++++++++++++++++++++++++++++++++++++
 arch/x86/platform/intel/iosf_mbi.c | 36 +++++++++++++++++++++++++
 2 files changed, 90 insertions(+)

diff --git a/arch/x86/include/asm/iosf_mbi.h b/arch/x86/include/asm/iosf_mbi.h
index 91f5d16..b8733bb 100644
--- a/arch/x86/include/asm/iosf_mbi.h
+++ b/arch/x86/include/asm/iosf_mbi.h
@@ -47,6 +47,10 @@
 #define QRK_MBI_UNIT_MM		0x05
 #define QRK_MBI_UNIT_SOC	0x31
 
+/* Action values for the pmic_bus_access_notifier functions */
+#define MBI_PMIC_BUS_ACCESS_BEGIN	1
+#define MBI_PMIC_BUS_ACCESS_END		2
+
 #if IS_ENABLED(CONFIG_IOSF_MBI)
 
 bool iosf_mbi_available(void);
@@ -115,6 +119,38 @@ void iosf_mbi_punit_lock(void);
  */
 void iosf_mbi_punit_unlock(void);
 
+/**
+ * iosf_mbi_register_pmic_bus_access_notifier - Register pmic bus notifier
+ *
+ * This function can be used by drivers which may need to acquire punit
+ * managed resources from interrupt context, where iosf_mbi_punit_lock()
+ * can not be used.
+ *
+ * This function allows a driver to register a notifier to get notified (in a
+ * process context) before other drivers start accessing the pmic bus.
+ *
+ * This allows the driver to acquire any resources, which it may need during
+ * the window the other driver is accessing the pmic, before hand.
+ *
+ * @nb: notifier_block to register
+ */
+int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb);
+
+/**
+ * iosf_mbi_register_pmic_bus_access_notifier - Unregister pmic bus notifier
+ *
+ * @nb: notifier_block to unregister
+ */
+int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb);
+
+/**
+ * iosf_mbi_call_pmic_bus_access_notifier_chain - Call pmic bus notifier chain
+ *
+ * @val: action to pass into listener's notifier_call function
+ * @v: data pointer to pass into listener's notifier_call function
+ */
+int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v);
+
 #else /* CONFIG_IOSF_MBI is not enabled */
 static inline
 bool iosf_mbi_available(void)
@@ -146,6 +182,24 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32 mask)
 static inline void iosf_mbi_punit_lock(void) {}
 static inline void iosf_mbi_punit_unlock(void) {}
 
+static inline
+int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb)
+{
+	return 0;
+}
+
+static inline
+int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb)
+{
+	return 0;
+}
+
+static inline
+int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v)
+{
+	return 0;
+}
+
 #endif /* CONFIG_IOSF_MBI */
 
 #endif /* IOSF_MBI_SYMS_H */
diff --git a/arch/x86/platform/intel/iosf_mbi.c b/arch/x86/platform/intel/iosf_mbi.c
index 75d8135..a995789 100644
--- a/arch/x86/platform/intel/iosf_mbi.c
+++ b/arch/x86/platform/intel/iosf_mbi.c
@@ -35,6 +35,7 @@
 static struct pci_dev *mbi_pdev;
 static DEFINE_SPINLOCK(iosf_mbi_lock);
 static DEFINE_MUTEX(iosf_mbi_punit_mutex);
+static BLOCKING_NOTIFIER_HEAD(iosf_mbi_pmic_bus_access_notifier);
 
 static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
 {
@@ -203,6 +204,41 @@ void iosf_mbi_punit_unlock(void)
 }
 EXPORT_SYMBOL(iosf_mbi_punit_unlock);
 
+int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block *nb)
+{
+	int ret;
+
+	/* Wait for the bus to go inactive before registering */
+	mutex_lock(&iosf_mbi_punit_mutex);
+	ret = blocking_notifier_chain_register(
+				&iosf_mbi_pmic_bus_access_notifier, nb);
+	mutex_unlock(&iosf_mbi_punit_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(iosf_mbi_register_pmic_bus_access_notifier);
+
+int iosf_mbi_unregister_pmic_bus_access_notifier(struct notifier_block *nb)
+{
+	int ret;
+
+	/* Wait for the bus to go inactive before unregistering */
+	mutex_lock(&iosf_mbi_punit_mutex);
+	ret = blocking_notifier_chain_unregister(
+				&iosf_mbi_pmic_bus_access_notifier, nb);
+	mutex_unlock(&iosf_mbi_punit_mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier);
+
+int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val, void *v)
+{
+	return blocking_notifier_call_chain(
+				&iosf_mbi_pmic_bus_access_notifier, val, v);
+}
+EXPORT_SYMBOL(iosf_mbi_call_pmic_bus_access_notifier_chain);
+
 #ifdef CONFIG_IOSF_MBI_DEBUG
 static u32	dbg_mdr;
 static u32	dbg_mcr;
-- 
2.9.3

[PATCH 3/7] i2c: designware-baytrail: Take punit lock on bus acquire

[Hans de Goede]

Take the punit lock to stop others from accessing the punit while the
pmic i2c bus is in use. This is necessary because accessing the punit
from the kernel may result in the punit trying to access the pmic i2c
bus, which results in a hang when it happens while we own the pmic i2c
bus semaphore.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 3effc9a..cf7a2a0 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -62,6 +62,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
 		dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
 
 	pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
+
+	iosf_mbi_punit_unlock();
 }
 
 static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
@@ -79,6 +81,8 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 	if (!dev->release_lock)
 		return 0;
 
+	iosf_mbi_punit_lock();
+
 	/*
 	 * Disallow the CPU to enter C6 or C7 state, entering these states
 	 * requires the punit to talk to the pmic and if this happens while
-- 
2.9.3

[PATCH 4/7] i2c: designware-baytrail: Call pmic_bus_access_notifier_chain

[Hans de Goede]

Call the iosf_mbi pmic_bus_access_notifier_chain on bus acquire / release.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index cf7a2a0..e8cf10a 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -63,6 +63,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
 
 	pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
 
+	iosf_mbi_call_pmic_bus_access_notifier_chain(MBI_PMIC_BUS_ACCESS_END,
+						     NULL);
 	iosf_mbi_punit_unlock();
 }
 
@@ -82,6 +84,8 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev *dev)
 		return 0;
 
 	iosf_mbi_punit_lock();
+	iosf_mbi_call_pmic_bus_access_notifier_chain(MBI_PMIC_BUS_ACCESS_BEGIN,
+						     NULL);
 
 	/*
 	 * Disallow the CPU to enter C6 or C7 state, entering these states
-- 
2.9.3

[PATCH 5/7] drm/i915: Add intel_uncore_suspend / resume functions

[Hans de Goede]

Rename intel_uncore_early_sanitize to intel_uncore_resume, dropping the
(always true) restore_forcewake argument and add a new intel_uncore_resume
function to replace the intel_uncore_forcewake_reset(dev_priv, false)
calls done from the suspend / runtime_suspend functions and make
intel_uncore_forcewake_reset private.

This is a preparation patch for adding pmic bus access notifier support.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.c     |  6 +++---
 drivers/gpu/drm/i915/i915_drv.h     |  6 ++----
 drivers/gpu/drm/i915/intel_uncore.c | 18 +++++++++++-------
 3 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 2c020ea..ec61e6e 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1445,7 +1445,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 	opregion_target_state = suspend_to_idle(dev_priv) ? PCI_D1 : PCI_D3cold;
 	intel_opregion_notify_adapter(dev_priv, opregion_target_state);
 
-	intel_uncore_forcewake_reset(dev_priv, false);
+	intel_uncore_suspend(dev_priv);
 	intel_opregion_unregister(dev_priv);
 
 	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED, true);
@@ -1690,7 +1690,7 @@ static int i915_drm_resume_early(struct drm_device *dev)
 		DRM_ERROR("Resume prepare failed: %d, continuing anyway\n",
 			  ret);
 
-	intel_uncore_early_sanitize(dev_priv, true);
+	intel_uncore_resume(dev_priv);
 
 	if (IS_BROXTON(dev_priv)) {
 		if (!dev_priv->suspended_to_idle)
@@ -2343,7 +2343,7 @@ static int intel_runtime_suspend(struct device *kdev)
 		return ret;
 	}
 
-	intel_uncore_forcewake_reset(dev_priv, false);
+	intel_uncore_suspend(dev_priv);
 
 	enable_rpm_wakeref_asserts(dev_priv);
 	WARN_ON_ONCE(atomic_read(&dev_priv->pm.wakeref_count));
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5194686..7cd0363 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3052,14 +3052,12 @@ int intel_irq_install(struct drm_i915_private *dev_priv);
 void intel_irq_uninstall(struct drm_i915_private *dev_priv);
 
 extern void intel_uncore_sanitize(struct drm_i915_private *dev_priv);
-extern void intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
-					bool restore_forcewake);
 extern void intel_uncore_init(struct drm_i915_private *dev_priv);
 extern bool intel_uncore_unclaimed_mmio(struct drm_i915_private *dev_priv);
 extern bool intel_uncore_arm_unclaimed_mmio_detection(struct drm_i915_private *dev_priv);
 extern void intel_uncore_fini(struct drm_i915_private *dev_priv);
-extern void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
-					 bool restore);
+extern void intel_uncore_suspend(struct drm_i915_private *dev_priv);
+extern void intel_uncore_resume(struct drm_i915_private *dev_priv);
 const char *intel_uncore_forcewake_domain_to_str(const enum forcewake_domain_id id);
 void intel_uncore_forcewake_get(struct drm_i915_private *dev_priv,
 				enum forcewake_domains domains);
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index 8fc5f29..c7fa222 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -250,7 +250,7 @@ intel_uncore_fw_release_timer(struct hrtimer *timer)
 	return HRTIMER_NORESTART;
 }
 
-void intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
+static void __intel_uncore_forcewake_reset(struct drm_i915_private *dev_priv,
 				  bool restore)
 {
 	unsigned long irqflags;
@@ -424,13 +424,17 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
 	if (IS_BXT_REVID(dev_priv, 0, BXT_REVID_B_LAST))
 		info->has_decoupled_mmio = false;
 
-	intel_uncore_forcewake_reset(dev_priv, restore_forcewake);
+	__intel_uncore_forcewake_reset(dev_priv, restore_forcewake);
 }
 
-void intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
-				 bool restore_forcewake)
+void intel_uncore_suspend(struct drm_i915_private *dev_priv)
 {
-	__intel_uncore_early_sanitize(dev_priv, restore_forcewake);
+	__intel_uncore_forcewake_reset(dev_priv, false);
+}
+
+void intel_uncore_resume(struct drm_i915_private *dev_priv)
+{
+	__intel_uncore_early_sanitize(dev_priv, true);
 	i915_check_and_clear_faults(dev_priv);
 }
 
@@ -1463,7 +1467,7 @@ void intel_uncore_fini(struct drm_i915_private *dev_priv)
 {
 	/* Paranoia: make sure we have disabled everything before we exit. */
 	intel_uncore_sanitize(dev_priv);
-	intel_uncore_forcewake_reset(dev_priv, false);
+	__intel_uncore_forcewake_reset(dev_priv, false);
 }
 
 #define GEN_RANGE(l, h) GENMASK((h) - 1, (l) - 1)
@@ -1679,7 +1683,7 @@ static int gen6_reset_engines(struct drm_i915_private *dev_priv,
 
 	ret = gen6_hw_domain_reset(dev_priv, hw_mask);
 
-	intel_uncore_forcewake_reset(dev_priv, true);
+	__intel_uncore_forcewake_reset(dev_priv, true);
 
 	return ret;
 }
-- 
2.9.3

[PATCH 6/7] drm/i915: Listen for pmic bus access notifications

[Hans de Goede]

Listen for pmic bus access notifications and get FORCEWAKE_ALL while
the bus is accessed to avoid needing to do any forcewakes, which need
pmic bus access, while the pmic bus is busy:

This fixes errors like these showing up in dmesg, usually followed
by a gfx or system freeze:

[drm:fw_domains_get [i915]] *ERROR* render: timed out waiting for forcewake ack request.
[drm:fw_domains_get [i915]] *MEDIA* render: timed out waiting for forcewake ack request.
i2c_designware 808622C1:06: punit semaphore timed out, resetting
i2c_designware 808622C1:06: PUNIT SEM: 2
i2c_designware 808622C1:06: couldn't acquire bus ownership

Downside of this approach is that it causes wakeups whenever the pmic
bus is accessed. Unfortunately we cannot simply wait for the pmic bus
to go idle when we hit a race, as forcewakes may be done from interrupt
handlers where we cannot sleep to wait for the i2c pmic bus access to
finish.

Note that the notifications and thus the wakeups will only happen on
baytrail / cherrytrail devices using pmic-s with a shared i2c bus for
punit and host pmic access (i2c busses with a _SEM method in their
APCI node), e.g. an axp288 pmic.

I plan to write some patches for drivers accessing the pmic bus to
limit their bus accesses to a bare minimum (e.g. cache registers, do not
update battery level more often then 4 times a minute), to limit the
amount of wakeups.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_uncore.c | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7cd0363..60297de 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -731,6 +731,7 @@ struct intel_uncore {
 	const struct intel_forcewake_range *fw_domains_table;
 	unsigned int fw_domains_table_entries;
 
+	struct notifier_block pmic_bus_access_nb;
 	struct intel_uncore_funcs funcs;
 
 	unsigned fifo_count;
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index c7fa222..1668de4 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -25,6 +25,7 @@
 #include "intel_drv.h"
 #include "i915_vgpu.h"
 
+#include <asm/iosf_mbi.h>
 #include <linux/pm_runtime.h>
 
 #define FORCEWAKE_ACK_TIMEOUT_MS 50
@@ -429,12 +430,16 @@ static void __intel_uncore_early_sanitize(struct drm_i915_private *dev_priv,
 
 void intel_uncore_suspend(struct drm_i915_private *dev_priv)
 {
+	iosf_mbi_unregister_pmic_bus_access_notifier(
+		&dev_priv->uncore.pmic_bus_access_nb);
 	__intel_uncore_forcewake_reset(dev_priv, false);
 }
 
 void intel_uncore_resume(struct drm_i915_private *dev_priv)
 {
 	__intel_uncore_early_sanitize(dev_priv, true);
+	iosf_mbi_register_pmic_bus_access_notifier(
+		&dev_priv->uncore.pmic_bus_access_nb);
 	i915_check_and_clear_faults(dev_priv);
 }
 
@@ -1390,6 +1395,28 @@ static void intel_uncore_fw_domains_init(struct drm_i915_private *dev_priv)
 	dev_priv->uncore.fw_domains_table_entries = ARRAY_SIZE((d)); \
 }
 
+static int i915_pmic_bus_access_notifier(struct notifier_block *nb,
+					 unsigned long action, void *data)
+{
+	struct drm_i915_private *dev_priv = container_of(nb,
+			struct drm_i915_private, uncore.pmic_bus_access_nb);
+
+	switch (action) {
+	case MBI_PMIC_BUS_ACCESS_BEGIN:
+		/*
+		 * forcewake all to make sure that we don't need to forcewake
+		 * any power-planes while the pmic bus is busy.
+		 */
+		intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
+		break;
+	case MBI_PMIC_BUS_ACCESS_END:
+		intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
 void intel_uncore_init(struct drm_i915_private *dev_priv)
 {
 	i915_check_vgpu(dev_priv);
@@ -1399,6 +1426,8 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
 	__intel_uncore_early_sanitize(dev_priv, false);
 
 	dev_priv->uncore.unclaimed_mmio_check = 1;
+	dev_priv->uncore.pmic_bus_access_nb.notifier_call =
+		i915_pmic_bus_access_notifier;
 
 	switch (INTEL_INFO(dev_priv)->gen) {
 	default:
@@ -1458,6 +1487,9 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
 		ASSIGN_READ_MMIO_VFUNCS(vgpu);
 	}
 
+	iosf_mbi_register_pmic_bus_access_notifier(
+		&dev_priv->uncore.pmic_bus_access_nb);
+
 	i915_check_and_clear_faults(dev_priv);
 }
 #undef ASSIGN_WRITE_MMIO_VFUNCS
@@ -1465,6 +1497,9 @@ void intel_uncore_init(struct drm_i915_private *dev_priv)
 
 void intel_uncore_fini(struct drm_i915_private *dev_priv)
 {
+	iosf_mbi_unregister_pmic_bus_access_notifier(
+		&dev_priv->uncore.pmic_bus_access_nb);
+
 	/* Paranoia: make sure we have disabled everything before we exit. */
 	intel_uncore_sanitize(dev_priv);
 	__intel_uncore_forcewake_reset(dev_priv, false);
-- 
2.9.3

[PATCH 7/7] drm/i915: Take punit lock when modifying punit settings

[Hans de Goede]

Make sure the punit i2c bus is not in use when we send a request to
the punit by calling iosf_mbi_punit_lock() / iosf_mbi_punit_unlock()
around punit write accesses.

BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: tagorereddy <tagore.chandan@gmail.com>
---
 drivers/gpu/drm/i915/intel_display.c    | 6 ++++++
 drivers/gpu/drm/i915/intel_pm.c         | 9 +++++++++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
 3 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fec8eb3..b8be6ea 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -47,6 +47,7 @@
 #include <drm/drm_rect.h>
 #include <linux/dma_remapping.h>
 #include <linux/reservation.h>
+#include <asm/iosf_mbi.h>
 
 static bool is_mmio_work(struct intel_flip_work *work)
 {
@@ -6423,6 +6424,8 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
 		cmd = 0;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
+	iosf_mbi_punit_lock();
+
 	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
 	val &= ~DSPFREQGUAR_MASK;
 	val |= (cmd << DSPFREQGUAR_SHIFT);
@@ -6432,6 +6435,7 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
 		     50)) {
 		DRM_ERROR("timed out waiting for CDclk change\n");
 	}
+	iosf_mbi_punit_unlock();
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
 	mutex_lock(&dev_priv->sb_lock);
@@ -6499,6 +6503,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
 	cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
+	iosf_mbi_punit_lock();
 	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
 	val &= ~DSPFREQGUAR_MASK_CHV;
 	val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
@@ -6508,6 +6513,7 @@ static void cherryview_set_cdclk(struct drm_device *dev, int cdclk)
 		     50)) {
 		DRM_ERROR("timed out waiting for CDclk change\n");
 	}
+	iosf_mbi_punit_unlock();
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
 	intel_update_cdclk(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4b12637..0d55b61 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -32,6 +32,7 @@
 #include "../../../platform/x86/intel_ips.h"
 #include <linux/module.h>
 #include <drm/drm_atomic_helper.h>
+#include <asm/iosf_mbi.h>
 
 /**
  * DOC: RC6
@@ -276,6 +277,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
 	u32 val;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
+	iosf_mbi_punit_lock();
 
 	val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
 	if (enable)
@@ -290,6 +292,7 @@ static void chv_set_memory_dvfs(struct drm_i915_private *dev_priv, bool enable)
 		      FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
 		DRM_ERROR("timed out waiting for Punit DDR DVFS request\n");
 
+	iosf_mbi_punit_unlock();
 	mutex_unlock(&dev_priv->rps.hw_lock);
 }
 
@@ -298,6 +301,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
 	u32 val;
 
 	mutex_lock(&dev_priv->rps.hw_lock);
+	iosf_mbi_punit_lock();
 
 	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
 	if (enable)
@@ -306,6 +310,7 @@ static void chv_set_memory_pm5(struct drm_i915_private *dev_priv, bool enable)
 		val &= ~DSP_MAXFIFO_PM5_ENABLE;
 	vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
 
+	iosf_mbi_punit_unlock();
 	mutex_unlock(&dev_priv->rps.hw_lock);
 }
 
@@ -4546,6 +4551,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
 
 	if (IS_CHERRYVIEW(dev_priv)) {
 		mutex_lock(&dev_priv->rps.hw_lock);
+		iosf_mbi_punit_lock();
 
 		val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
 		if (val & DSP_MAXFIFO_PM5_ENABLE)
@@ -4575,6 +4581,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
 				wm->level = VLV_WM_LEVEL_DDR_DVFS;
 		}
 
+		iosf_mbi_punit_unlock();
 		mutex_unlock(&dev_priv->rps.hw_lock);
 	}
 
@@ -4981,7 +4988,9 @@ static void valleyview_set_rps(struct drm_i915_private *dev_priv, u8 val)
 	I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
 
 	if (val != dev_priv->rps.cur_freq) {
+		iosf_mbi_punit_lock();
 		vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
+		iosf_mbi_punit_unlock();
 		if (!IS_CHERRYVIEW(dev_priv))
 			gen6_set_rps_thresholds(dev_priv, val);
 	}
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index c0b7e95..17922ae 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -28,6 +28,7 @@
 
 #include <linux/pm_runtime.h>
 #include <linux/vgaarb.h>
+#include <asm/iosf_mbi.h>
 
 #include "i915_drv.h"
 #include "intel_drv.h"
@@ -1027,6 +1028,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
 	if (COND)
 		goto out;
 
+	iosf_mbi_punit_lock();
+
 	ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL);
 	ctrl &= ~mask;
 	ctrl |= state;
@@ -1037,6 +1040,8 @@ static void vlv_set_power_well(struct drm_i915_private *dev_priv,
 			  state,
 			  vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL));
 
+	iosf_mbi_punit_unlock();
+
 #undef COND
 
 out:
@@ -1643,6 +1648,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
 	if (COND)
 		goto out;
 
+	iosf_mbi_punit_lock();
+
 	ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
 	ctrl &= ~DP_SSC_MASK(pipe);
 	ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe);
@@ -1653,6 +1660,8 @@ static void chv_set_pipe_power_well(struct drm_i915_private *dev_priv,
 			  state,
 			  vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ));
 
+	iosf_mbi_punit_unlock();
+
 #undef COND
 
 out:
-- 
2.9.3

Re: [PATCH 1/7] x86/platform/intel/iosf_mbi: Add a mutex for punit access

[Andy Shevchenko]

On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
> One some systems the punit accesses the pmic to change various
> voltages
> through the same bus as other kernel drivers use for e.g. battery
> monitoring.
> 
> If a driver sends requests to the punit which require the punit to
> access
> the pmic bus while another driver is also accessing the pmic bus
> various
> bad things happen.
> 
> This commit adds a mutex to protect the punit against simultaneous
> accesses
> and 2 functions to lock / unlock this mutex.
> 
> Note on these systems the i2c-bus driver will request a sempahore from
> the
> punit for exclusive access to the pmic bus when i2c drivers are
> accessing
> it, but this does not appear to be sufficient, we still need to avoid
> making certain punit requests during the access window to avoid
> problems.

I'm fine with the patch, but please spell
P-Unit
PMIC

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.com>
> ---
>  arch/x86/include/asm/iosf_mbi.h    | 31
> +++++++++++++++++++++++++++++++
>  arch/x86/platform/intel/iosf_mbi.c | 13 +++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/arch/x86/include/asm/iosf_mbi.h
> b/arch/x86/include/asm/iosf_mbi.h
> index b41ee16..91f5d16 100644
> --- a/arch/x86/include/asm/iosf_mbi.h
> +++ b/arch/x86/include/asm/iosf_mbi.h
> @@ -88,6 +88,33 @@ int iosf_mbi_write(u8 port, u8 opcode, u32 offset,
> u32 mdr);
>   */
>  int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32
> mask);
>  
> +/**
> + * iosf_mbi_punit_lock() - Lock the punit mutex
> + *
> + * One some systems the punit accesses the pmic to change various
> voltages
> + * through the same bus as other kernel drivers use for e.g. battery
> monitoring.
> + *
> + * If a driver sends requests to the punit which require the punit to
> access the
> + * pmic bus while another driver is also accessing the pmic bus
> various bad
> + * things happen.
> + *
> + * To avoid these problems this function must be called before
> accessing the
> + * punit or the pmic, be it through iosf_mbi* functions or through
> other means.
> + *
> + * Note on these systems the i2c-bus driver will request a sempahore
> from the
> + * punit for exclusive access to the pmic bus when i2c drivers are
> accessing it,
> + * but this does not appear to be sufficient, we still need to avoid
> making
> + * certain punit requests during the access window to avoid problems.
> + *
> + * This function locks a mutex, as such it may sleep.
> + */
> +void iosf_mbi_punit_lock(void);
> +
> +/**
> + * iosf_mbi_punit_unlock() - Unlock the punit mutex
> + */
> +void iosf_mbi_punit_unlock(void);
> +
>  #else /* CONFIG_IOSF_MBI is not enabled */
>  static inline
>  bool iosf_mbi_available(void)
> @@ -115,6 +142,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32
> offset, u32 mdr, u32 mask)
>  	WARN(1, "IOSF_MBI driver not available");
>  	return -EPERM;
>  }
> +
> +static inline void iosf_mbi_punit_lock(void) {}
> +static inline void iosf_mbi_punit_unlock(void) {}
> +
>  #endif /* CONFIG_IOSF_MBI */
>  
>  #endif /* IOSF_MBI_SYMS_H */
> diff --git a/arch/x86/platform/intel/iosf_mbi.c
> b/arch/x86/platform/intel/iosf_mbi.c
> index edf2c54..75d8135 100644
> --- a/arch/x86/platform/intel/iosf_mbi.c
> +++ b/arch/x86/platform/intel/iosf_mbi.c
> @@ -34,6 +34,7 @@
>  
>  static struct pci_dev *mbi_pdev;
>  static DEFINE_SPINLOCK(iosf_mbi_lock);
> +static DEFINE_MUTEX(iosf_mbi_punit_mutex);
>  
>  static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
>  {
> @@ -190,6 +191,18 @@ bool iosf_mbi_available(void)
>  }
>  EXPORT_SYMBOL(iosf_mbi_available);
>  
> +void iosf_mbi_punit_lock(void)
> +{
> +	mutex_lock(&iosf_mbi_punit_mutex);
> +}
> +EXPORT_SYMBOL(iosf_mbi_punit_lock);
> +
> +void iosf_mbi_punit_unlock(void)
> +{
> +	mutex_unlock(&iosf_mbi_punit_mutex);
> +}
> +EXPORT_SYMBOL(iosf_mbi_punit_unlock);
> +
>  #ifdef CONFIG_IOSF_MBI_DEBUG
>  static u32	dbg_mdr;
>  static u32	dbg_mcr;

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 2/7] x86/platform/intel/iosf_mbi: Add a pmic bus access notifier

[Andy Shevchenko]

On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
> Some drivers may need to acquire punit managed resources from
> interrupt
> context, where they cannot call iosf_mbi_punit_lock().
> 
> This commit adds a notifier chain which allows a driver to get
> notified
> (in a process context) before other drivers start accessing the pmic
> bus,
> so that the driver can acquire any resources, which it may need during
> the window the other driver is accessing the pmic, before hand.
> 

Same comments as per patch 1.

I'm okay with the patch, but I would hear from other stakeholders that's
_the_ way we are going.

So, FWIW:
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.com>
> ---
>  arch/x86/include/asm/iosf_mbi.h    | 54
> ++++++++++++++++++++++++++++++++++++++
>  arch/x86/platform/intel/iosf_mbi.c | 36 +++++++++++++++++++++++++
>  2 files changed, 90 insertions(+)
> 
> diff --git a/arch/x86/include/asm/iosf_mbi.h
> b/arch/x86/include/asm/iosf_mbi.h
> index 91f5d16..b8733bb 100644
> --- a/arch/x86/include/asm/iosf_mbi.h
> +++ b/arch/x86/include/asm/iosf_mbi.h
> @@ -47,6 +47,10 @@
>  #define QRK_MBI_UNIT_MM		0x05
>  #define QRK_MBI_UNIT_SOC	0x31
>  
> +/* Action values for the pmic_bus_access_notifier functions */
> +#define MBI_PMIC_BUS_ACCESS_BEGIN	1
> +#define MBI_PMIC_BUS_ACCESS_END		2
> +
>  #if IS_ENABLED(CONFIG_IOSF_MBI)
>  
>  bool iosf_mbi_available(void);
> @@ -115,6 +119,38 @@ void iosf_mbi_punit_lock(void);
>   */
>  void iosf_mbi_punit_unlock(void);
>  
> +/**
> + * iosf_mbi_register_pmic_bus_access_notifier - Register pmic bus
> notifier
> + *
> + * This function can be used by drivers which may need to acquire
> punit
> + * managed resources from interrupt context, where
> iosf_mbi_punit_lock()
> + * can not be used.
> + *
> + * This function allows a driver to register a notifier to get
> notified (in a
> + * process context) before other drivers start accessing the pmic
> bus.
> + *
> + * This allows the driver to acquire any resources, which it may need
> during
> + * the window the other driver is accessing the pmic, before hand.
> + *
> + * @nb: notifier_block to register
> + */
> +int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block
> *nb);
> +
> +/**
> + * iosf_mbi_register_pmic_bus_access_notifier - Unregister pmic bus
> notifier
> + *
> + * @nb: notifier_block to unregister
> + */
> +int iosf_mbi_unregister_pmic_bus_access_notifier(struct
> notifier_block *nb);
> +
> +/**
> + * iosf_mbi_call_pmic_bus_access_notifier_chain - Call pmic bus
> notifier chain
> + *
> + * @val: action to pass into listener's notifier_call function
> + * @v: data pointer to pass into listener's notifier_call function
> + */
> +int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val,
> void *v);
> +
>  #else /* CONFIG_IOSF_MBI is not enabled */
>  static inline
>  bool iosf_mbi_available(void)
> @@ -146,6 +182,24 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32
> offset, u32 mdr, u32 mask)
>  static inline void iosf_mbi_punit_lock(void) {}
>  static inline void iosf_mbi_punit_unlock(void) {}
>  
> +static inline
> +int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block
> *nb)
> +{
> +	return 0;
> +}
> +
> +static inline
> +int iosf_mbi_unregister_pmic_bus_access_notifier(struct
> notifier_block *nb)
> +{
> +	return 0;
> +}
> +
> +static inline
> +int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val,
> void *v)
> +{
> +	return 0;
> +}
> +
>  #endif /* CONFIG_IOSF_MBI */
>  
>  #endif /* IOSF_MBI_SYMS_H */
> diff --git a/arch/x86/platform/intel/iosf_mbi.c
> b/arch/x86/platform/intel/iosf_mbi.c
> index 75d8135..a995789 100644
> --- a/arch/x86/platform/intel/iosf_mbi.c
> +++ b/arch/x86/platform/intel/iosf_mbi.c
> @@ -35,6 +35,7 @@
>  static struct pci_dev *mbi_pdev;
>  static DEFINE_SPINLOCK(iosf_mbi_lock);
>  static DEFINE_MUTEX(iosf_mbi_punit_mutex);
> +static BLOCKING_NOTIFIER_HEAD(iosf_mbi_pmic_bus_access_notifier);
>  
>  static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
>  {
> @@ -203,6 +204,41 @@ void iosf_mbi_punit_unlock(void)
>  }
>  EXPORT_SYMBOL(iosf_mbi_punit_unlock);
>  
> +int iosf_mbi_register_pmic_bus_access_notifier(struct notifier_block
> *nb)
> +{
> +	int ret;
> +
> +	/* Wait for the bus to go inactive before registering */
> +	mutex_lock(&iosf_mbi_punit_mutex);
> +	ret = blocking_notifier_chain_register(
> +				&iosf_mbi_pmic_bus_access_notifier,
> nb);
> +	mutex_unlock(&iosf_mbi_punit_mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(iosf_mbi_register_pmic_bus_access_notifier);
> +
> +int iosf_mbi_unregister_pmic_bus_access_notifier(struct
> notifier_block *nb)
> +{
> +	int ret;
> +
> +	/* Wait for the bus to go inactive before unregistering */
> +	mutex_lock(&iosf_mbi_punit_mutex);
> +	ret = blocking_notifier_chain_unregister(
> +				&iosf_mbi_pmic_bus_access_notifier,
> nb);
> +	mutex_unlock(&iosf_mbi_punit_mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(iosf_mbi_unregister_pmic_bus_access_notifier);
> +
> +int iosf_mbi_call_pmic_bus_access_notifier_chain(unsigned long val,
> void *v)
> +{
> +	return blocking_notifier_call_chain(
> +				&iosf_mbi_pmic_bus_access_notifier,
> val, v);
> +}
> +EXPORT_SYMBOL(iosf_mbi_call_pmic_bus_access_notifier_chain);
> +
>  #ifdef CONFIG_IOSF_MBI_DEBUG
>  static u32	dbg_mdr;
>  static u32	dbg_mcr;

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 4/7] i2c: designware-baytrail: Call pmic_bus_access_notifier_chain

[Andy Shevchenko]

On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
> Call the iosf_mbi pmic_bus_access_notifier_chain on bus acquire /
> release.
> 

FWIW:
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.com>
> ---
>  drivers/i2c/busses/i2c-designware-baytrail.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c
> b/drivers/i2c/busses/i2c-designware-baytrail.c
> index cf7a2a0..e8cf10a 100644
> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> @@ -63,6 +63,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
>  
>  	pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
>  
> +	iosf_mbi_call_pmic_bus_access_notifier_chain(MBI_PMIC_BUS_ACC
> ESS_END,
> +						     NULL);
>  	iosf_mbi_punit_unlock();
>  }
>  
> @@ -82,6 +84,8 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev
> *dev)
>  		return 0;
>  
>  	iosf_mbi_punit_lock();
> +	iosf_mbi_call_pmic_bus_access_notifier_chain(MBI_PMIC_BUS_ACC
> ESS_BEGIN,
> +						     NULL);
>  
>  	/*
>  	 * Disallow the CPU to enter C6 or C7 state, entering these
> states

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 3/7] i2c: designware-baytrail: Take punit lock on bus acquire

[Andy Shevchenko]

On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
> Take the punit lock to stop others from accessing the punit while the
> pmic i2c bus is in use. This is necessary because accessing the punit
> from the kernel may result in the punit trying to access the pmic i2c
> bus, which results in a hang when it happens while we own the pmic i2c
> bus semaphore.
> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.com>
> ---
>  drivers/i2c/busses/i2c-designware-baytrail.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c
> b/drivers/i2c/busses/i2c-designware-baytrail.c
> index 3effc9a..cf7a2a0 100644
> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> 

> @@ -62,6 +62,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev)

> +	iosf_mbi_punit_unlock();

> @@ -79,6 +81,8 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev
> +	iosf_mbi_punit_lock();

For me it looks a bit asymmetrical.

Can we use acquire()/release()?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 1/7] x86/platform/intel/iosf_mbi: Add a mutex for punit access

[Hans de Goede]

Hi,

On 08-01-17 16:16, Andy Shevchenko wrote:
> On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
>> One some systems the punit accesses the pmic to change various
>> voltages
>> through the same bus as other kernel drivers use for e.g. battery
>> monitoring.
>>
>> If a driver sends requests to the punit which require the punit to
>> access
>> the pmic bus while another driver is also accessing the pmic bus
>> various
>> bad things happen.
>>
>> This commit adds a mutex to protect the punit against simultaneous
>> accesses
>> and 2 functions to lock / unlock this mutex.
>>
>> Note on these systems the i2c-bus driver will request a sempahore from
>> the
>> punit for exclusive access to the pmic bus when i2c drivers are
>> accessing
>> it, but this does not appear to be sufficient, we still need to avoid
>> making certain punit requests during the access window to avoid
>> problems.
>
> I'm fine with the patch, but please spell
> P-Unit
> PMIC

In the commit msg and comments, not in code you mean I assume ?

Regards,

Hans



>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
>> ---
>>  arch/x86/include/asm/iosf_mbi.h    | 31
>> +++++++++++++++++++++++++++++++
>>  arch/x86/platform/intel/iosf_mbi.c | 13 +++++++++++++
>>  2 files changed, 44 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/iosf_mbi.h
>> b/arch/x86/include/asm/iosf_mbi.h
>> index b41ee16..91f5d16 100644
>> --- a/arch/x86/include/asm/iosf_mbi.h
>> +++ b/arch/x86/include/asm/iosf_mbi.h
>> @@ -88,6 +88,33 @@ int iosf_mbi_write(u8 port, u8 opcode, u32 offset,
>> u32 mdr);
>>   */
>>  int iosf_mbi_modify(u8 port, u8 opcode, u32 offset, u32 mdr, u32
>> mask);
>>
>> +/**
>> + * iosf_mbi_punit_lock() - Lock the punit mutex
>> + *
>> + * One some systems the punit accesses the pmic to change various
>> voltages
>> + * through the same bus as other kernel drivers use for e.g. battery
>> monitoring.
>> + *
>> + * If a driver sends requests to the punit which require the punit to
>> access the
>> + * pmic bus while another driver is also accessing the pmic bus
>> various bad
>> + * things happen.
>> + *
>> + * To avoid these problems this function must be called before
>> accessing the
>> + * punit or the pmic, be it through iosf_mbi* functions or through
>> other means.
>> + *
>> + * Note on these systems the i2c-bus driver will request a sempahore
>> from the
>> + * punit for exclusive access to the pmic bus when i2c drivers are
>> accessing it,
>> + * but this does not appear to be sufficient, we still need to avoid
>> making
>> + * certain punit requests during the access window to avoid problems.
>> + *
>> + * This function locks a mutex, as such it may sleep.
>> + */
>> +void iosf_mbi_punit_lock(void);
>> +
>> +/**
>> + * iosf_mbi_punit_unlock() - Unlock the punit mutex
>> + */
>> +void iosf_mbi_punit_unlock(void);
>> +
>>  #else /* CONFIG_IOSF_MBI is not enabled */
>>  static inline
>>  bool iosf_mbi_available(void)
>> @@ -115,6 +142,10 @@ int iosf_mbi_modify(u8 port, u8 opcode, u32
>> offset, u32 mdr, u32 mask)
>>  	WARN(1, "IOSF_MBI driver not available");
>>  	return -EPERM;
>>  }
>> +
>> +static inline void iosf_mbi_punit_lock(void) {}
>> +static inline void iosf_mbi_punit_unlock(void) {}
>> +
>>  #endif /* CONFIG_IOSF_MBI */
>>
>>  #endif /* IOSF_MBI_SYMS_H */
>> diff --git a/arch/x86/platform/intel/iosf_mbi.c
>> b/arch/x86/platform/intel/iosf_mbi.c
>> index edf2c54..75d8135 100644
>> --- a/arch/x86/platform/intel/iosf_mbi.c
>> +++ b/arch/x86/platform/intel/iosf_mbi.c
>> @@ -34,6 +34,7 @@
>>
>>  static struct pci_dev *mbi_pdev;
>>  static DEFINE_SPINLOCK(iosf_mbi_lock);
>> +static DEFINE_MUTEX(iosf_mbi_punit_mutex);
>>
>>  static inline u32 iosf_mbi_form_mcr(u8 op, u8 port, u8 offset)
>>  {
>> @@ -190,6 +191,18 @@ bool iosf_mbi_available(void)
>>  }
>>  EXPORT_SYMBOL(iosf_mbi_available);
>>
>> +void iosf_mbi_punit_lock(void)
>> +{
>> +	mutex_lock(&iosf_mbi_punit_mutex);
>> +}
>> +EXPORT_SYMBOL(iosf_mbi_punit_lock);
>> +
>> +void iosf_mbi_punit_unlock(void)
>> +{
>> +	mutex_unlock(&iosf_mbi_punit_mutex);
>> +}
>> +EXPORT_SYMBOL(iosf_mbi_punit_unlock);
>> +
>>  #ifdef CONFIG_IOSF_MBI_DEBUG
>>  static u32	dbg_mdr;
>>  static u32	dbg_mcr;
>

Re: [PATCH 7/7] drm/i915: Take punit lock when modifying punit settings

[Andy Shevchenko]

On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
> Make sure the punit i2c bus is not in use when we send a request to
> the punit by calling iosf_mbi_punit_lock() / iosf_mbi_punit_unlock()
> around punit write accesses.
> 

But should not i915 drm eventually share the same iosf_mbi driver?
Currently what you are doing you create notifier and all that not-best-
ever stuff due to having two accessors to IOSF MB. If we have only one
driver which i915 will not ignore you don't need to create such things.

That's what I was trying to imply when commenting one of the patch in
previous series.


> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c    | 6 ++++++
>  drivers/gpu/drm/i915/intel_pm.c         | 9 +++++++++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index fec8eb3..b8be6ea 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -47,6 +47,7 @@
>  #include <drm/drm_rect.h>
>  #include <linux/dma_remapping.h>
>  #include <linux/reservation.h>
> +#include <asm/iosf_mbi.h>
>  
>  static bool is_mmio_work(struct intel_flip_work *work)
>  {
> @@ -6423,6 +6424,8 @@ static void valleyview_set_cdclk(struct
> drm_device *dev, int cdclk)
>  		cmd = 0;
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
> +	iosf_mbi_punit_lock();
> +
>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>  	val &= ~DSPFREQGUAR_MASK;
>  	val |= (cmd << DSPFREQGUAR_SHIFT);
> @@ -6432,6 +6435,7 @@ static void valleyview_set_cdclk(struct
> drm_device *dev, int cdclk)
>  		     50)) {
>  		DRM_ERROR("timed out waiting for CDclk change\n");
>  	}
> +	iosf_mbi_punit_unlock();
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  
>  	mutex_lock(&dev_priv->sb_lock);
> @@ -6499,6 +6503,7 @@ static void cherryview_set_cdclk(struct
> drm_device *dev, int cdclk)
>  	cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
> +	iosf_mbi_punit_lock();
>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>  	val &= ~DSPFREQGUAR_MASK_CHV;
>  	val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
> @@ -6508,6 +6513,7 @@ static void cherryview_set_cdclk(struct
> drm_device *dev, int cdclk)
>  		     50)) {
>  		DRM_ERROR("timed out waiting for CDclk change\n");
>  	}
> +	iosf_mbi_punit_unlock();
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  
>  	intel_update_cdclk(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 4b12637..0d55b61 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -32,6 +32,7 @@
>  #include "../../../platform/x86/intel_ips.h"
>  #include <linux/module.h>
>  #include <drm/drm_atomic_helper.h>
> +#include <asm/iosf_mbi.h>
>  
>  /**
>   * DOC: RC6
> @@ -276,6 +277,7 @@ static void chv_set_memory_dvfs(struct
> drm_i915_private *dev_priv, bool enable)
>  	u32 val;
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
> +	iosf_mbi_punit_lock();
>  
>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
>  	if (enable)
> @@ -290,6 +292,7 @@ static void chv_set_memory_dvfs(struct
> drm_i915_private *dev_priv, bool enable)
>  		      FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
>  		DRM_ERROR("timed out waiting for Punit DDR DVFS
> request\n");
>  
> +	iosf_mbi_punit_unlock();
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  }
>  
> @@ -298,6 +301,7 @@ static void chv_set_memory_pm5(struct
> drm_i915_private *dev_priv, bool enable)
>  	u32 val;
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
> +	iosf_mbi_punit_lock();
>  
>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>  	if (enable)
> @@ -306,6 +310,7 @@ static void chv_set_memory_pm5(struct
> drm_i915_private *dev_priv, bool enable)
>  		val &= ~DSP_MAXFIFO_PM5_ENABLE;
>  	vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>  
> +	iosf_mbi_punit_unlock();
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  }
>  
> @@ -4546,6 +4551,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>  
>  	if (IS_CHERRYVIEW(dev_priv)) {
>  		mutex_lock(&dev_priv->rps.hw_lock);
> +		iosf_mbi_punit_lock();
>  
>  		val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>  		if (val & DSP_MAXFIFO_PM5_ENABLE)
> @@ -4575,6 +4581,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>  				wm->level = VLV_WM_LEVEL_DDR_DVFS;
>  		}
>  
> +		iosf_mbi_punit_unlock();
>  		mutex_unlock(&dev_priv->rps.hw_lock);
>  	}
>  
> @@ -4981,7 +4988,9 @@ static void valleyview_set_rps(struct
> drm_i915_private *dev_priv, u8 val)
>  	I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
>  
>  	if (val != dev_priv->rps.cur_freq) {
> +		iosf_mbi_punit_lock();
>  		vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
> val);
> +		iosf_mbi_punit_unlock();
>  		if (!IS_CHERRYVIEW(dev_priv))
>  			gen6_set_rps_thresholds(dev_priv, val);
>  	}
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index c0b7e95..17922ae 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -28,6 +28,7 @@
>  
>  #include <linux/pm_runtime.h>
>  #include <linux/vgaarb.h>
> +#include <asm/iosf_mbi.h>
>  
>  #include "i915_drv.h"
>  #include "intel_drv.h"
> @@ -1027,6 +1028,8 @@ static void vlv_set_power_well(struct
> drm_i915_private *dev_priv,
>  	if (COND)
>  		goto out;
>  
> +	iosf_mbi_punit_lock();
> +
>  	ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL);
>  	ctrl &= ~mask;
>  	ctrl |= state;
> @@ -1037,6 +1040,8 @@ static void vlv_set_power_well(struct
> drm_i915_private *dev_priv,
>  			  state,
>  			  vlv_punit_read(dev_priv,
> PUNIT_REG_PWRGT_CTRL));
>  
> +	iosf_mbi_punit_unlock();
> +
>  #undef COND
>  
>  out:
> @@ -1643,6 +1648,8 @@ static void chv_set_pipe_power_well(struct
> drm_i915_private *dev_priv,
>  	if (COND)
>  		goto out;
>  
> +	iosf_mbi_punit_lock();
> +
>  	ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>  	ctrl &= ~DP_SSC_MASK(pipe);
>  	ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe);
> @@ -1653,6 +1660,8 @@ static void chv_set_pipe_power_well(struct
> drm_i915_private *dev_priv,
>  			  state,
>  			  vlv_punit_read(dev_priv,
> PUNIT_REG_DSPFREQ));
>  
> +	iosf_mbi_punit_unlock();
> +
>  #undef COND
>  
>  out:

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] x86/platform/intel/iosf_mbi: Add a mutex for punit access

[Andy Shevchenko]

On Sun, 2017-01-08 at 16:30 +0100, Hans de Goede wrote:
> Hi,
> 
> On 08-01-17 16:16, Andy Shevchenko wrote:
> > On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
> > > One some systems the punit accesses the pmic to change various
> > > voltages
> > > through the same bus as other kernel drivers use for e.g. battery
> > > monitoring.
> > > 
> > > If a driver sends requests to the punit which require the punit to
> > > access
> > > the pmic bus while another driver is also accessing the pmic bus
> > > various
> > > bad things happen.
> > > 
> > > This commit adds a mutex to protect the punit against simultaneous
> > > accesses
> > > and 2 functions to lock / unlock this mutex.
> > > 
> > > Note on these systems the i2c-bus driver will request a sempahore
> > > from
> > > the
> > > punit for exclusive access to the pmic bus when i2c drivers are
> > > accessing
> > > it, but this does not appear to be sufficient, we still need to
> > > avoid
> > > making certain punit requests during the access window to avoid
> > > problems.
> > 
> > I'm fine with the patch, but please spell
> > P-Unit
> > PMIC
> 
> In the commit msg and comments, not in code you mean I assume ?

Correct.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 3/7] i2c: designware-baytrail: Take punit lock on bus acquire

[Hans de Goede]

Hi,

On 08-01-17 16:27, Andy Shevchenko wrote:
> On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
>> Take the punit lock to stop others from accessing the punit while the
>> pmic i2c bus is in use. This is necessary because accessing the punit
>> from the kernel may result in the punit trying to access the pmic i2c
>> bus, which results in a hang when it happens while we own the pmic i2c
>> bus semaphore.
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-designware-baytrail.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c
>> b/drivers/i2c/busses/i2c-designware-baytrail.c
>> index 3effc9a..cf7a2a0 100644
>> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
>> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
>>
>
>> @@ -62,6 +62,8 @@ static void reset_semaphore(struct dw_i2c_dev *dev)
>
>> +	iosf_mbi_punit_unlock();
>
>> @@ -79,6 +81,8 @@ static int baytrail_i2c_acquire(struct dw_i2c_dev
>> +	iosf_mbi_punit_lock();
>
> For me it looks a bit asymmetrical.
>
> Can we use acquire()/release()?

Sure I can change things to use acquire()/release() instead. I will
change this for v2.

Regards,

Hans

Re: [PATCH 7/7] drm/i915: Take punit lock when modifying punit settings

[Hans de Goede]

Hi,

On 08-01-17 16:34, Andy Shevchenko wrote:
> On Sun, 2017-01-08 at 14:44 +0100, Hans de Goede wrote:
>> Make sure the punit i2c bus is not in use when we send a request to
>> the punit by calling iosf_mbi_punit_lock() / iosf_mbi_punit_unlock()
>> around punit write accesses.
>>
>
> But should not i915 drm eventually share the same iosf_mbi driver?
> Currently what you are doing you create notifier and all that not-best-
> ever stuff due to having two accessors to IOSF MB. If we have only one
> driver which i915 will not ignore you don't need to create such things.
>
> That's what I was trying to imply when commenting one of the patch in
> previous series.

Ah, yes that as an interesting point. So what do the i915 devs think
of changing the i915 code to use the iosf_mbi functions for at least
punit accesses, instead of doing those through the i915 pci config space?

Note that we will still need to have some higher level locking, or
a new iosf_mbi function which does this under a lock, for code paths
where the i915 code does a write followed by multiple reads to wait
for the punit to have changed the value to the requested value.

Regards,

Hans




>
>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c    | 6 ++++++
>>  drivers/gpu/drm/i915/intel_pm.c         | 9 +++++++++
>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 9 +++++++++
>>  3 files changed, 24 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index fec8eb3..b8be6ea 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -47,6 +47,7 @@
>>  #include <drm/drm_rect.h>
>>  #include <linux/dma_remapping.h>
>>  #include <linux/reservation.h>
>> +#include <asm/iosf_mbi.h>
>>
>>  static bool is_mmio_work(struct intel_flip_work *work)
>>  {
>> @@ -6423,6 +6424,8 @@ static void valleyview_set_cdclk(struct
>> drm_device *dev, int cdclk)
>>  		cmd = 0;
>>
>>  	mutex_lock(&dev_priv->rps.hw_lock);
>> +	iosf_mbi_punit_lock();
>> +
>>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>  	val &= ~DSPFREQGUAR_MASK;
>>  	val |= (cmd << DSPFREQGUAR_SHIFT);
>> @@ -6432,6 +6435,7 @@ static void valleyview_set_cdclk(struct
>> drm_device *dev, int cdclk)
>>  		     50)) {
>>  		DRM_ERROR("timed out waiting for CDclk change\n");
>>  	}
>> +	iosf_mbi_punit_unlock();
>>  	mutex_unlock(&dev_priv->rps.hw_lock);
>>
>>  	mutex_lock(&dev_priv->sb_lock);
>> @@ -6499,6 +6503,7 @@ static void cherryview_set_cdclk(struct
>> drm_device *dev, int cdclk)
>>  	cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
>>
>>  	mutex_lock(&dev_priv->rps.hw_lock);
>> +	iosf_mbi_punit_lock();
>>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>  	val &= ~DSPFREQGUAR_MASK_CHV;
>>  	val |= (cmd << DSPFREQGUAR_SHIFT_CHV);
>> @@ -6508,6 +6513,7 @@ static void cherryview_set_cdclk(struct
>> drm_device *dev, int cdclk)
>>  		     50)) {
>>  		DRM_ERROR("timed out waiting for CDclk change\n");
>>  	}
>> +	iosf_mbi_punit_unlock();
>>  	mutex_unlock(&dev_priv->rps.hw_lock);
>>
>>  	intel_update_cdclk(dev_priv);
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index 4b12637..0d55b61 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -32,6 +32,7 @@
>>  #include "../../../platform/x86/intel_ips.h"
>>  #include <linux/module.h>
>>  #include <drm/drm_atomic_helper.h>
>> +#include <asm/iosf_mbi.h>
>>
>>  /**
>>   * DOC: RC6
>> @@ -276,6 +277,7 @@ static void chv_set_memory_dvfs(struct
>> drm_i915_private *dev_priv, bool enable)
>>  	u32 val;
>>
>>  	mutex_lock(&dev_priv->rps.hw_lock);
>> +	iosf_mbi_punit_lock();
>>
>>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DDR_SETUP2);
>>  	if (enable)
>> @@ -290,6 +292,7 @@ static void chv_set_memory_dvfs(struct
>> drm_i915_private *dev_priv, bool enable)
>>  		      FORCE_DDR_FREQ_REQ_ACK) == 0, 3))
>>  		DRM_ERROR("timed out waiting for Punit DDR DVFS
>> request\n");
>>
>> +	iosf_mbi_punit_unlock();
>>  	mutex_unlock(&dev_priv->rps.hw_lock);
>>  }
>>
>> @@ -298,6 +301,7 @@ static void chv_set_memory_pm5(struct
>> drm_i915_private *dev_priv, bool enable)
>>  	u32 val;
>>
>>  	mutex_lock(&dev_priv->rps.hw_lock);
>> +	iosf_mbi_punit_lock();
>>
>>  	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>  	if (enable)
>> @@ -306,6 +310,7 @@ static void chv_set_memory_pm5(struct
>> drm_i915_private *dev_priv, bool enable)
>>  		val &= ~DSP_MAXFIFO_PM5_ENABLE;
>>  	vlv_punit_write(dev_priv, PUNIT_REG_DSPFREQ, val);
>>
>> +	iosf_mbi_punit_unlock();
>>  	mutex_unlock(&dev_priv->rps.hw_lock);
>>  }
>>
>> @@ -4546,6 +4551,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>>
>>  	if (IS_CHERRYVIEW(dev_priv)) {
>>  		mutex_lock(&dev_priv->rps.hw_lock);
>> +		iosf_mbi_punit_lock();
>>
>>  		val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>  		if (val & DSP_MAXFIFO_PM5_ENABLE)
>> @@ -4575,6 +4581,7 @@ void vlv_wm_get_hw_state(struct drm_device *dev)
>>  				wm->level = VLV_WM_LEVEL_DDR_DVFS;
>>  		}
>>
>> +		iosf_mbi_punit_unlock();
>>  		mutex_unlock(&dev_priv->rps.hw_lock);
>>  	}
>>
>> @@ -4981,7 +4988,9 @@ static void valleyview_set_rps(struct
>> drm_i915_private *dev_priv, u8 val)
>>  	I915_WRITE(GEN6_PMINTRMSK, gen6_rps_pm_mask(dev_priv, val));
>>
>>  	if (val != dev_priv->rps.cur_freq) {
>> +		iosf_mbi_punit_lock();
>>  		vlv_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ,
>> val);
>> +		iosf_mbi_punit_unlock();
>>  		if (!IS_CHERRYVIEW(dev_priv))
>>  			gen6_set_rps_thresholds(dev_priv, val);
>>  	}
>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> index c0b7e95..17922ae 100644
>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> @@ -28,6 +28,7 @@
>>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/vgaarb.h>
>> +#include <asm/iosf_mbi.h>
>>
>>  #include "i915_drv.h"
>>  #include "intel_drv.h"
>> @@ -1027,6 +1028,8 @@ static void vlv_set_power_well(struct
>> drm_i915_private *dev_priv,
>>  	if (COND)
>>  		goto out;
>>
>> +	iosf_mbi_punit_lock();
>> +
>>  	ctrl = vlv_punit_read(dev_priv, PUNIT_REG_PWRGT_CTRL);
>>  	ctrl &= ~mask;
>>  	ctrl |= state;
>> @@ -1037,6 +1040,8 @@ static void vlv_set_power_well(struct
>> drm_i915_private *dev_priv,
>>  			  state,
>>  			  vlv_punit_read(dev_priv,
>> PUNIT_REG_PWRGT_CTRL));
>>
>> +	iosf_mbi_punit_unlock();
>> +
>>  #undef COND
>>
>>  out:
>> @@ -1643,6 +1648,8 @@ static void chv_set_pipe_power_well(struct
>> drm_i915_private *dev_priv,
>>  	if (COND)
>>  		goto out;
>>
>> +	iosf_mbi_punit_lock();
>> +
>>  	ctrl = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
>>  	ctrl &= ~DP_SSC_MASK(pipe);
>>  	ctrl |= enable ? DP_SSC_PWR_ON(pipe) : DP_SSC_PWR_GATE(pipe);
>> @@ -1653,6 +1660,8 @@ static void chv_set_pipe_power_well(struct
>> drm_i915_private *dev_priv,
>>  			  state,
>>  			  vlv_punit_read(dev_priv,
>> PUNIT_REG_DSPFREQ));
>>
>> +	iosf_mbi_punit_unlock();
>> +
>>  #undef COND
>>
>>  out:
>

Re: [PATCH 3/7] i2c: designware-baytrail: Take punit lock on bus acquire

[Wolfram Sang]

On Sun, Jan 08, 2017 at 02:44:23PM +0100, Hans de Goede wrote:
> Take the punit lock to stop others from accessing the punit while the
> pmic i2c bus is in use. This is necessary because accessing the punit
> from the kernel may result in the punit trying to access the pmic i2c
> bus, which results in a hang when it happens while we own the pmic i2c
> bus semaphore.
> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.com>

I don't think the I2C patches need to go via I2C tree, so:

Acked-by: Wolfram Sang <wsa@the-dreams.de>

Re: [PATCH 4/7] i2c: designware-baytrail: Call pmic_bus_access_notifier_chain

[Wolfram Sang]

On Sun, Jan 08, 2017 at 02:44:24PM +0100, Hans de Goede wrote:
> Call the iosf_mbi pmic_bus_access_notifier_chain on bus acquire / release.
> 
> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: tagorereddy <tagore.chandan@gmail.com>

Acked-by: Wolfram Sang <wsa@the-dreams.de>

Re: [PATCH 3/7] i2c: designware-baytrail: Take punit lock on bus acquire

[Hans de Goede]

Hi,

On 12-01-17 19:45, Wolfram Sang wrote:
> On Sun, Jan 08, 2017 at 02:44:23PM +0100, Hans de Goede wrote:
>> Take the punit lock to stop others from accessing the punit while the
>> pmic i2c bus is in use. This is necessary because accessing the punit
>> from the kernel may result in the punit trying to access the pmic i2c
>> bus, which results in a hang when it happens while we own the pmic i2c
>> bus semaphore.
>>
>> BugLink: https://bugzilla.kernel.org/show_bug.cgi?id=155241
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Tested-by: tagorereddy <tagore.chandan@gmail.com>
>
> I don't think the I2C patches need to go via I2C tree, so:
>
> Acked-by: Wolfram Sang <wsa@the-dreams.de>

Note that as mentioned in the cover-letter, these 2 i2c patches depend
on these:

https://patchwork.ozlabs.org/patch/710067/
https://patchwork.ozlabs.org/patch/710068/
https://patchwork.ozlabs.org/patch/710069/
https://patchwork.ozlabs.org/patch/710070/
https://patchwork.ozlabs.org/patch/710071/
https://patchwork.ozlabs.org/patch/710072/

Which all have many reviews / acks. Given the race between i915
and designware-i2c opened up by the last patch in this series
these not having been merged yet is a good thing, but patch 1-5
are ready for merging.

So Wolfram, what is the plan with these ? As said they are necessary
for the 2 i2c patches in this patch-set, so do you want the
entire set of 8 i2c patches to go through an other tree to avoid
inter tree dependencies ?

And if so, can we then have you're Acked-by for the 6 above too ?

Regards,

Hans

Re: [PATCH 3/7] i2c: designware-baytrail: Take punit lock on bus acquire

[Wolfram Sang]

Hi Hans,

> So Wolfram, what is the plan with these ? As said they are necessary
> for the 2 i2c patches in this patch-set, so do you want the
> entire set of 8 i2c patches to go through an other tree to avoid
> inter tree dependencies ?

Thanks for the heads up. So, my plan was that I send a pull request for
4.10 any minute now, and start pulling in patches for 4.11 based on
shiny new rc4 from tomorrow on. And your series would have been one of
the fist to get merged because I think it is ready.

Reading this though, my feeling is now that it should be merged via some
other tree so you can get these nasty issues fixed without too many
dependencies. An immutable branch for me to pull in would be great,
though.

Makes sense?

Regards,

   Wolfram

Re: [PATCH 3/7] i2c: designware-baytrail: Take punit lock on bus acquire

[Hans de Goede]

Hi,

On 15-01-17 12:45, Wolfram Sang wrote:
> Hi Hans,
>
>> So Wolfram, what is the plan with these ? As said they are necessary
>> for the 2 i2c patches in this patch-set, so do you want the
>> entire set of 8 i2c patches to go through an other tree to avoid
>> inter tree dependencies ?
>
> Thanks for the heads up. So, my plan was that I send a pull request for
> 4.10 any minute now, and start pulling in patches for 4.11 based on
> shiny new rc4 from tomorrow on. And your series would have been one of
> the fist to get merged because I think it is ready.
>
> Reading this though, my feeling is now that it should be merged via some
> other tree so you can get these nasty issues fixed without too many
> dependencies. An immutable branch for me to pull in would be great,
> though.
>
> Makes sense?

Sounds fine to me, step one is to get consensus on how to deal with
coordinating the kernel directly accessing to the pmic-i2c-bus vs punit
accesses which also end up needing to use the same i2c-bus from the
punit side.

Once I've a patch-set everyone likes I will start talking to people
to coordinate the merging, I believe it is probably best for all this
to be merged through the drm-intel tree. But we'll see about that
when the patch-set is ready.

Regards,

Hans

Re: [PATCH 3/7] i2c: designware-baytrail: Take punit lock on bus acquire

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 252 bytes --]


> Once I've a patch-set everyone likes I will start talking to people
> to coordinate the merging, I believe it is probably best for all this
> to be merged through the drm-intel tree. But we'll see about that
> when the patch-set is ready.

Agreed.


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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