Re: [PATCH -next] regulator: qcom-rpmh: fix build after QCOM_COMMAND_DB is tristate
On Fri, Dec 25 2020 at 11:50 -0700, Randy Dunlap wrote:
Restrict REGULATOR_QCOM_RPMH to QCOM_COMMAND_DB it the latter is enabled.
Fixes this build error:
microblaze-linux-ld: drivers/regulator/qcom-rpmh-regulator.o: in function
`rpmh_regulator_probe':
(.text+0x354): undefined reference to `cmd_db_read_addr'
Fixes: 778279f4f5e4 ("soc: qcom: cmd-db: allow loading as a module")
Reported-by: kernel test robot
Signed-off-by: Randy Dunlap
Cc: Lina Iyer
Cc: Liam Girdwood
Cc: Mark Brown
Reviewed-by: Lina Iyer
---
drivers/regulator/Kconfig |1 +
1 file changed, 1 insertion(+)
--- linux-next-20201223.orig/drivers/regulator/Kconfig
+++ linux-next-20201223/drivers/regulator/Kconfig
@@ -881,6 +881,7 @@ config REGULATOR_QCOM_RPM
config REGULATOR_QCOM_RPMH
tristate "Qualcomm Technologies, Inc. RPMh regulator driver"
depends on QCOM_RPMH || (QCOM_RPMH=n && COMPILE_TEST)
+ depends on QCOM_COMMAND_DB || !QCOM_COMMAND_DB
help
This driver supports control of PMIC regulators via the RPMh hardware
block found on Qualcomm Technologies Inc. SoCs. RPMh regulator
Re: [PATCH] soc: qcom: QCOM_RPMH fix build with modular QCOM_RPMH
On Wed, Oct 28 2020 at 03:43 -0600, Anders Roxell wrote:
On Tue, 27 Oct 2020 at 22:15, Lina Iyer wrote:
Hi Anders,
On Tue, Oct 27 2020 at 05:14 -0600, Anders Roxell wrote:
>When building allmodconfig leading to the following link error with
>CONFIG_QCOM_RPMH=y and CONFIG_QCOM_COMMAND_DB=m:
>
>aarch64-linux-gnu-ld: drivers/clk/qcom/clk-rpmh.o: in function
`clk_rpmh_probe':
> drivers/clk/qcom/clk-rpmh.c:474: undefined reference to `cmd_db_read_addr'
> drivers/clk/qcom/clk-rpmh.c:474:(.text+0x254): relocation truncated to fit:
R_AARCH64_CALL26 against undefined symbol `cmd_db_read_addr'
>
>Fix this by adding a Kconfig depenency and forcing QCOM_RPMH to be a
>module when QCOM_COMMAND_DB is a module. Also removing the dependency on
>'ARCH_QCOM || COMPILE_TEST' since that is already a dependency for
>QCOM_COMMAND_DB.
>
>Fixes: 778279f4f5e4 ("soc: qcom: cmd-db: allow loading as a module")
>Signed-off-by: Anders Roxell
>---
> drivers/soc/qcom/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>index 9b4ae9c16ba7..3bdd1604f78f 100644
>--- a/drivers/soc/qcom/Kconfig
>+++ b/drivers/soc/qcom/Kconfig
>@@ -109,7 +109,7 @@ config QCOM_RMTFS_MEM
>
> config QCOM_RPMH
> tristate "Qualcomm RPM-Hardened (RPMH) Communication"
>- depends on ARCH_QCOM || COMPILE_TEST
>+ depends on QCOM_COMMAND_DB
A solution was posted in the mailing list alredy -
https://lore.kernel.org/linux-arm-msm/20201008040907.7036-1-il...@codeaurora.org/
I missed that one, thanks.
If you get a chance, please give that a shot to see if that works for
you.
That will work too, but the "depends on ARCH_QCOM || COMPILE_TEST"
isn't needed since that is already the dependency for QCOM_COMMAND_DB.
So that should be met here too or am I missing something?
Sure, if you want to post an update to the patch, that would be fine
too.
Bjorn: Have you picked up this patch yet? If he hasn't please feel free
to update the patch. Or, I can do that as well.
--Lina
Cheers,
Anders
Thanks,
Lina
> help
> Support for communication with the hardened-RPM blocks in
> Qualcomm Technologies Inc (QTI) SoCs. RPMH communication uses an
>--
>2.28.0
>
Re: [PATCH] soc: qcom: QCOM_RPMH fix build with modular QCOM_RPMH
Hi Anders,
On Tue, Oct 27 2020 at 05:14 -0600, Anders Roxell wrote:
When building allmodconfig leading to the following link error with
CONFIG_QCOM_RPMH=y and CONFIG_QCOM_COMMAND_DB=m:
aarch64-linux-gnu-ld: drivers/clk/qcom/clk-rpmh.o: in function `clk_rpmh_probe':
drivers/clk/qcom/clk-rpmh.c:474: undefined reference to `cmd_db_read_addr'
drivers/clk/qcom/clk-rpmh.c:474:(.text+0x254): relocation truncated to fit:
R_AARCH64_CALL26 against undefined symbol `cmd_db_read_addr'
Fix this by adding a Kconfig depenency and forcing QCOM_RPMH to be a
module when QCOM_COMMAND_DB is a module. Also removing the dependency on
'ARCH_QCOM || COMPILE_TEST' since that is already a dependency for
QCOM_COMMAND_DB.
Fixes: 778279f4f5e4 ("soc: qcom: cmd-db: allow loading as a module")
Signed-off-by: Anders Roxell
---
drivers/soc/qcom/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index 9b4ae9c16ba7..3bdd1604f78f 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -109,7 +109,7 @@ config QCOM_RMTFS_MEM
config QCOM_RPMH
tristate "Qualcomm RPM-Hardened (RPMH) Communication"
- depends on ARCH_QCOM || COMPILE_TEST
+ depends on QCOM_COMMAND_DB
A solution was posted in the mailing list alredy -
https://lore.kernel.org/linux-arm-msm/20201008040907.7036-1-il...@codeaurora.org/
If you get a chance, please give that a shot to see if that works for
you.
Thanks,
Lina
help
Support for communication with the hardened-RPM blocks in
Qualcomm Technologies Inc (QTI) SoCs. RPMH communication uses an
--
2.28.0
Re: [PATCH v3] PM: domains: Add support for PM domain on/off notifiers for genpd
On Tue, Oct 13 2020 at 06:23 -0600, Ulf Hansson wrote:
A device may have specific HW constraints that must be obeyed to, before
its corresponding PM domain (genpd) can be powered off - and vice verse at
power on. These constraints can't be managed through the regular runtime PM
based deployment for a device, because the access pattern for it, isn't
always request based. In other words, using the runtime PM callbacks to
deal with the constraints doesn't work for these cases.
For these reasons, let's instead add a PM domain power on/off notification
mechanism to genpd. To add/remove a notifier for a device, the device must
already have been attached to the genpd, which also means that it needs to
be a part of the PM domain topology.
To add/remove a notifier, let's introduce two genpd specific functions:
- dev_pm_genpd_add|remove_notifier()
Note that, to further clarify when genpd power on/off notifiers may be
used, one can compare with the existing CPU_CLUSTER_PM_ENTER|EXIT
notifiers. In the long run, the genpd power on/off notifiers should be able
to replace them, but that requires additional genpd based platform support
for the current users.
Signed-off-by: Ulf Hansson
---
Changes in v3:
- Adopted suggestions from Peng Fan to allow more fine grained levels of
notifications, which is needed on some NXP platforms.
- Move the code that fires the notifications into _genpd_power_on|off(),
as it simply fits better in there.
Note that, I understand that some of us may be occupied with dealing with the
merge window, but I still wanted to get this submitted to allow those that have
some time to review and test.
Kind regards
Ulf Hansson
---
drivers/base/power/domain.c | 161 +---
include/linux/pm_domain.h | 22 +
2 files changed, 171 insertions(+), 12 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 05bb4d4401b2..c2a8821bdb26 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -413,28 +413,47 @@ static int _genpd_power_on(struct generic_pm_domain
*genpd, bool timed)
unsigned int state_idx = genpd->state_idx;
ktime_t time_start;
s64 elapsed_ns;
- int ret;
+ int ret, nr_calls = 0;
+
+ /* Notify consumers that we are about to power on. */
+ ret = __raw_notifier_call_chain(>power_notifiers,
+ GENPD_NOTIFY_PRE_ON, NULL, -1,
+ _calls);
Rafael's bleeding-edge fails to compile because this call is removed
now -
https://lore.kernel.org/lkml/20200818135804.325626...@infradead.org/
which also handles the failure case (_robust) variant.
--Lina
+ ret = notifier_to_errno(ret);
+ if (ret)
+ goto err;
if (!genpd->power_on)
- return 0;
+ goto out;
- if (!timed)
- return genpd->power_on(genpd);
+ if (!timed) {
+ ret = genpd->power_on(genpd);
+ if (ret)
+ goto err;
+
+ goto out;
+ }
time_start = ktime_get();
ret = genpd->power_on(genpd);
if (ret)
- return ret;
+ goto err;
elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
- return ret;
+ goto out;
genpd->states[state_idx].power_on_latency_ns = elapsed_ns;
genpd->max_off_time_changed = true;
pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
genpd->name, "on", elapsed_ns);
+out:
+ raw_notifier_call_chain(>power_notifiers, GENPD_NOTIFY_ON, NULL);
+ return 0;
+err:
+ raw_notifier_call_chain(>power_notifiers, GENPD_NOTIFY_OFF,
+ NULL);
return ret;
}
@@ -443,29 +462,51 @@ static int _genpd_power_off(struct generic_pm_domain
*genpd, bool timed)
unsigned int state_idx = genpd->state_idx;
ktime_t time_start;
s64 elapsed_ns;
- int ret;
+ int ret, nr_calls = 0;
+
+ /* Notify consumers that we are about to power off. */
+ ret = __raw_notifier_call_chain(>power_notifiers,
+ GENPD_NOTIFY_PRE_OFF, NULL, -1,
+ _calls);
+ ret = notifier_to_errno(ret);
+ if (ret)
+ goto busy;
if (!genpd->power_off)
- return 0;
+ goto out;
+
+ if (!timed) {
+ ret = genpd->power_off(genpd);
+ if (ret)
+ goto busy;
- if (!timed)
- return genpd->power_off(genpd);
+ goto out;
+ }
time_start = ktime_get();
ret = genpd->power_off(genpd);
if (ret)
- return ret;
+ goto busy;
Re: [PATCH v3] PM: domains: Add support for PM domain on/off notifiers for genpd
On Tue, Oct 13 2020 at 06:23 -0600, Ulf Hansson wrote:
A device may have specific HW constraints that must be obeyed to, before
its corresponding PM domain (genpd) can be powered off - and vice verse at
power on. These constraints can't be managed through the regular runtime PM
based deployment for a device, because the access pattern for it, isn't
always request based. In other words, using the runtime PM callbacks to
deal with the constraints doesn't work for these cases.
For these reasons, let's instead add a PM domain power on/off notification
mechanism to genpd. To add/remove a notifier for a device, the device must
already have been attached to the genpd, which also means that it needs to
be a part of the PM domain topology.
To add/remove a notifier, let's introduce two genpd specific functions:
- dev_pm_genpd_add|remove_notifier()
Note that, to further clarify when genpd power on/off notifiers may be
used, one can compare with the existing CPU_CLUSTER_PM_ENTER|EXIT
notifiers. In the long run, the genpd power on/off notifiers should be able
to replace them, but that requires additional genpd based platform support
for the current users.
Signed-off-by: Ulf Hansson
---
Changes in v3:
- Adopted suggestions from Peng Fan to allow more fine grained levels of
notifications, which is needed on some NXP platforms.
- Move the code that fires the notifications into _genpd_power_on|off(),
as it simply fits better in there.
Note that, I understand that some of us may be occupied with dealing with the
merge window, but I still wanted to get this submitted to allow those that have
some time to review and test.
Kind regards
Ulf Hansson
---
drivers/base/power/domain.c | 161 +---
include/linux/pm_domain.h | 22 +
2 files changed, 171 insertions(+), 12 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 05bb4d4401b2..c2a8821bdb26 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -413,28 +413,47 @@ static int _genpd_power_on(struct generic_pm_domain
*genpd, bool timed)
unsigned int state_idx = genpd->state_idx;
ktime_t time_start;
s64 elapsed_ns;
- int ret;
+ int ret, nr_calls = 0;
+
+ /* Notify consumers that we are about to power on. */
+ ret = __raw_notifier_call_chain(>power_notifiers,
+ GENPD_NOTIFY_PRE_ON, NULL, -1,
+ _calls);
+ ret = notifier_to_errno(ret);
+ if (ret)
+ goto err;
if (!genpd->power_on)
- return 0;
+ goto out;
- if (!timed)
- return genpd->power_on(genpd);
+ if (!timed) {
+ ret = genpd->power_on(genpd);
+ if (ret)
+ goto err;
+
+ goto out;
+ }
time_start = ktime_get();
ret = genpd->power_on(genpd);
if (ret)
- return ret;
+ goto err;
elapsed_ns = ktime_to_ns(ktime_sub(ktime_get(), time_start));
if (elapsed_ns <= genpd->states[state_idx].power_on_latency_ns)
- return ret;
+ goto out;
genpd->states[state_idx].power_on_latency_ns = elapsed_ns;
genpd->max_off_time_changed = true;
pr_debug("%s: Power-%s latency exceeded, new value %lld ns\n",
genpd->name, "on", elapsed_ns);
+out:
+ raw_notifier_call_chain(>power_notifiers, GENPD_NOTIFY_ON, NULL);
+ return 0;
+err:
+ raw_notifier_call_chain(>power_notifiers, GENPD_NOTIFY_OFF,
+ NULL);
return ret;
}
@@ -443,29 +462,51 @@ static int _genpd_power_off(struct generic_pm_domain
*genpd, bool timed)
unsigned int state_idx = genpd->state_idx;
ktime_t time_start;
s64 elapsed_ns;
- int ret;
+ int ret, nr_calls = 0;
+
+ /* Notify consumers that we are about to power off. */
+ ret = __raw_notifier_call_chain(>power_notifiers,
+ GENPD_NOTIFY_PRE_OFF, NULL, -1,
+ _calls);
+ ret = notifier_to_errno(ret);
+ if (ret)
+ goto busy;
Nit: You could enclose this in a function call to keep these functions
clean.
Otherwise,
Tested-by: Lina Iyer
if (!genpd->power_off)
- return 0;
+ goto out;
+
+ if (!timed) {
+ ret = genpd->power_off(genpd);
+ if (ret)
+ goto busy;
- if (!timed)
- return genpd->power_off(genpd);
+ goto out;
+ }
time_start = ktime_get();
ret = genpd->power_off(genpd);
if (ret)
- return ret;
+ goto busy;
elapsed_ns = kti
Re: [PATCH 3/3] PM / Domains: Add support for PM domain on/off notifiers for genpd
-Original Message-
Date: Wed, 19 Aug 2020 12:40:57 +0200
From: Ulf Hansson
To: "Rafael J . Wysocki" , Kevin Hilman
, linux...@vger.kernel.org
Cc: Sudeep Holla , Lorenzo Pieralisi
, Daniel Lezcano ,
Lina Iyer , Lukasz Luba , Vincent
Guittot , Stephen Boyd , Bjorn
Andersson , Benjamin Gaignard
, Ulf Hansson ,
linux-arm-ker...@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: [PATCH 3/3] PM / Domains: Add support for PM domain on/off notifiers
for genpd
A device may have specific HW constraints that must be obeyed to, before
its corresponding PM domain (genpd) can be powered off - and vice verse at
power on. These constraints can't be managed through the regular runtime PM
based deployment for a device, because the access pattern for it, isn't
always request based. In other words, using the runtime PM callbacks to
deal with the constraints doesn't work for these cases.
For these reasons, let's instead add a PM domain power on/off notification
mechanism to genpd. To add/remove a notifier for a device, the device must
already have been attached to the genpd, which also means that it needs to
be a part of the PM domain topology.
To add/remove a notifier, let's introduce two genpd specific functions:
- dev_pm_genpd_add|remove_notifier()
Note that, to further clarify when genpd power on/off notifiers may be
used, one can compare with the existing CPU_CLUSTER_PM_ENTER|EXIT
notifiers. In the long run, the genpd power on/off notifiers should be able
to replace them, but that requires additional genpd based platform support
for the current users.
Signed-off-by: Ulf Hansson
---
drivers/base/power/domain.c | 130 ++--
include/linux/pm_domain.h | 15 +
2 files changed, 141 insertions(+), 4 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 4b787e1ff188..9cb85a5e8342 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -545,13 +545,21 @@ static int genpd_power_off(struct generic_pm_domain
*genpd, bool one_dev_on,
if (!genpd->gov)
genpd->state_idx = 0;
+ /* Notify consumers that we are about to power off. */
+ ret = raw_notifier_call_chain(>power_notifiers, GENPD_STATE_OFF,
+ NULL);
+ if (ret)
+ return ret;
+
/* Don't power off, if a child domain is waiting to power on. */
- if (atomic_read(>sd_count) > 0)
- return -EBUSY;
+ if (atomic_read(>sd_count) > 0) {
+ ret = -EBUSY;
+ goto busy;
+ }
ret = _genpd_power_off(genpd, true);
if (ret)
- return ret;
+ goto busy;
genpd->status = GENPD_STATE_OFF;
genpd_update_accounting(genpd);
@@ -564,6 +572,9 @@ static int genpd_power_off(struct generic_pm_domain *genpd,
bool one_dev_on,
}
return 0;
+busy:
+ raw_notifier_call_chain(>power_notifiers, GENPD_STATE_ON, NULL);
+ return ret;
}
/**
@@ -606,6 +617,9 @@ static int genpd_power_on(struct generic_pm_domain *genpd,
unsigned int depth)
if (ret)
goto err;
+ /* Inform consumers that we have powered on. */
+ raw_notifier_call_chain(>power_notifiers, GENPD_STATE_ON, NULL);
+
genpd->status = GENPD_STATE_ON;
genpd_update_accounting(genpd);
@@ -948,9 +962,18 @@ static void genpd_sync_power_off(struct generic_pm_domain
*genpd, bool use_lock,
/* Choose the deepest state when suspending */
genpd->state_idx = genpd->state_count - 1;
- if (_genpd_power_off(genpd, false))
+
+ /* Notify consumers that we are about to power off. */
+ if (raw_notifier_call_chain(>power_notifiers,
+ GENPD_STATE_OFF, NULL))
return;
If any of the notified returns an error, we probably should inform that
the genpd failed as well.
-- Lina
+ if (_genpd_power_off(genpd, false)) {
+ raw_notifier_call_chain(>power_notifiers,
+ GENPD_STATE_ON, NULL);
+ return;
+ }
+
genpd->status = GENPD_STATE_OFF;
list_for_each_entry(link, >child_links, child_node) {
@@ -998,6 +1021,9 @@ static void genpd_sync_power_on(struct generic_pm_domain
*genpd, bool use_lock,
_genpd_power_on(genpd, false);
+ /* Inform consumers that we have powered on. */
+ raw_notifier_call_chain(>power_notifiers, GENPD_STATE_ON, NULL);
+
genpd->status = GENPD_STATE_ON;
}
@@ -1593,6 +1619,101 @@ int pm_genpd_remove_device(struct device *dev)
}
EXPORT_SYMBOL_GPL(pm_genpd_remove_device);
+/**
+ * dev_pm_genpd_add_notifier - Add a genpd power on/off notifier for @dev
+ *
+ * @dev: Device that should be associated with the notifier
+ * @nb: The notifier block to register
+ *
+
Re: WARNING: suspicious RCU usage - sdhci-pltfm: SDHCI platform and OF driver helper
Hi Ulf,
On Tue, Sep 01 2020 at 06:41 -0600, Ulf Hansson wrote:
On Tue, 1 Sep 2020 at 14:35, Ulf Hansson wrote:
On Tue, 1 Sep 2020 at 12:42, wrote:
>
> On Tue, Sep 01, 2020 at 08:50:57AM +0200, Ulf Hansson wrote:
> > On Tue, 1 Sep 2020 at 08:46, Ulf Hansson wrote:
> > > On Mon, 31 Aug 2020 at 21:44, Paul E. McKenney wrote:
>
> > > > > [5.308588] =
> > > > > [5.308593] WARNING: suspicious RCU usage
> > > > > [5.316628] sdhci-pltfm: SDHCI platform and OF driver helper
> > > > > [5.320052] 5.9.0-rc3 #1 Not tainted
> > > > > [5.320057] -
> > > > > [5.320063] /usr/src/kernel/include/trace/events/lock.h:37
suspicious rcu_dereference_check() usage!
> > > > > [5.320068]
> > > > > [5.320068] other info that might help us debug this:
> > > > > [5.320068]
> > > > > [5.320074]
> > > > > [5.320074] rcu_scheduler_active = 2, debug_locks = 1
> > > > > [5.320078] RCU used illegally from extended quiescent state!
> > > > > [5.320084] no locks held by swapper/0/0.
> > > > > [5.320089]
> > > > > [5.320089] stack backtrace:
> > > > > [5.320098] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.9.0-rc3 #1
> > > > > [5.346354] sdhci_msm 7864900.sdhci: Got CD GPIO
> > > > > [5.346446] Hardware name: Qualcomm Technologies, Inc. APQ 8016
SBC (DT)
> > > > > [5.346452] Call trace:
> > > > > [5.346463] dump_backtrace+0x0/0x1f8
> > > > > [5.346471] show_stack+0x2c/0x38
> > > > > [5.346480] dump_stack+0xec/0x15c
> > > > > [5.346490] lockdep_rcu_suspicious+0xd4/0xf8
> > > > > [5.346499] lock_acquire+0x3d0/0x440
> > > > > [5.346510] _raw_spin_lock_irqsave+0x80/0xb0
> > > > > [5.413118] __pm_runtime_suspend+0x34/0x1d0
> > > > > [5.417457] psci_enter_domain_idle_state+0x4c/0xb0
> > > > > [5.421795] cpuidle_enter_state+0xc8/0x610
> > > > > [5.426392] cpuidle_enter+0x3c/0x50
> > > > > [5.430561] call_cpuidle+0x44/0x80
> > > > > [5.434378] do_idle+0x240/0x2a0
>
> > > > Note also that Peter Zijlstra (CCed) is working to shrink the portion
> > > > of the idle loop that RCU ignores. Not sure that it covers your
> > > > case, but it is worth checking.
>
> Right, so I think I 'caused' this by making the lock tracepoints
> visible. That is, the error always existed, now we actually warn about
> it.
>
> > > Thanks for letting me know. Let's see what Peter thinks about this then.
> > >
> > > Apologize for my ignorance, but from a cpuidle point of view, what
> > > does it mean using RCU_NONIDLE()? I guess we should avoid RCU_NONIDLE
> > > on bigger code paths?
> > >
> > > I could add RCU_NONIDLE for the calls to pm_runtime_put_sync_suspend()
> > > and pm_runtime_get_sync() in psci_enter_domain_idle_state(). Perhaps
> > > that's the easiest approach, at least to start with.
> > >
I think this would be nice. This should also cover the case, where PM domain
power off notification callbacks call trace function internally. Right?
--Lina
> > > Or do you have any other ideas?
>
> So IMO trace_*_rcuidle() and RCU_NONIDLE() are bugs, they just mean we
> got the ordering wrong and are papering over it. That said, that's been
> the modus operandi for a while now, just make it shut up and don't think
> about it :-/
>
> That said; I pushed the rcu_idle_enter() about as deep as it goes into
> generic code in commit 1098582a0f6c ("sched,idle,rcu: Push rcu_idle
> deeper into the idle path")
Aha, that commit should fix this problem, I think. Looks like that
commit was sent as a fix and included in the recent v5.9-rc3.
Naresh, can you try with the above commit?
Ah, just realized that I misread the patch. It doesn't fix it.
We would still need a RCU_NONIDLE() in psci_enter_domain_idle_state()
- or something along the lines of what you suggest below.
Apologies for the noise.
Kind regards
Uffe
[1]. https://lkml.org/lkml/2020/8/26/81
>
> I suppose the next step is pushing it into individual driver when
> needed, something like the below perhaps. I realize the coupled idle
> state stuff is more complicated that most, but it's also not an area
> I've looked at in detail, so perhaps I've just made a bigger mess, but
> it ought to give you enough to get going I think.
These aren't coupled states. Instead, in cpuidle-psci, we are using PM
domains through genpd and runtime PM to manage "shared idle states"
between CPUs.
Kind regards
Uffe
>
> Rafael?
>
> ---
> diff --git a/drivers/cpuidle/cpuidle-psci.c b/drivers/cpuidle/cpuidle-psci.c
> index 74463841805f..617bbef316e6 100644
> --- a/drivers/cpuidle/cpuidle-psci.c
> +++ b/drivers/cpuidle/cpuidle-psci.c
> @@ -49,6 +49,9 @@ static inline u32 psci_get_domain_state(void)
>
> static inline int psci_enter_state(int idx, u32 state)
> {
> + /*
> +* XXX push rcu_idle_enter into the coupled code
> +*/
> return CPU_PM_CPU_IDLE_ENTER_PARAM(psci_cpu_suspend_enter, idx,
state);
> }
>
> @@ -72,7
Re: [PATCH 3/3] PM / Domains: Add support for PM domain on/off notifiers for genpd
On Wed, Aug 19 2020 at 04:41 -0600, Ulf Hansson wrote:
A device may have specific HW constraints that must be obeyed to, before
its corresponding PM domain (genpd) can be powered off - and vice verse at
power on. These constraints can't be managed through the regular runtime PM
based deployment for a device, because the access pattern for it, isn't
always request based. In other words, using the runtime PM callbacks to
deal with the constraints doesn't work for these cases.
For these reasons, let's instead add a PM domain power on/off notification
mechanism to genpd. To add/remove a notifier for a device, the device must
already have been attached to the genpd, which also means that it needs to
be a part of the PM domain topology.
To add/remove a notifier, let's introduce two genpd specific functions:
- dev_pm_genpd_add|remove_notifier()
Note that, to further clarify when genpd power on/off notifiers may be
used, one can compare with the existing CPU_CLUSTER_PM_ENTER|EXIT
notifiers. In the long run, the genpd power on/off notifiers should be able
to replace them, but that requires additional genpd based platform support
for the current users.
Signed-off-by: Ulf Hansson
---
drivers/base/power/domain.c | 130 ++--
include/linux/pm_domain.h | 15 +
2 files changed, 141 insertions(+), 4 deletions(-)
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 4b787e1ff188..9cb85a5e8342 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -545,13 +545,21 @@ static int genpd_power_off(struct generic_pm_domain
*genpd, bool one_dev_on,
if (!genpd->gov)
genpd->state_idx = 0;
+ /* Notify consumers that we are about to power off. */
+ ret = raw_notifier_call_chain(>power_notifiers, GENPD_STATE_OFF,
+ NULL);
+ if (ret)
+ return ret;
+
/* Don't power off, if a child domain is waiting to power on. */
- if (atomic_read(>sd_count) > 0)
- return -EBUSY;
+ if (atomic_read(>sd_count) > 0) {
+ ret = -EBUSY;
+ goto busy;
+ }
ret = _genpd_power_off(genpd, true);
if (ret)
- return ret;
+ goto busy;
genpd->status = GENPD_STATE_OFF;
genpd_update_accounting(genpd);
@@ -564,6 +572,9 @@ static int genpd_power_off(struct generic_pm_domain *genpd,
bool one_dev_on,
}
return 0;
+busy:
+ raw_notifier_call_chain(>power_notifiers, GENPD_STATE_ON, NULL);
It would be helpful to abstract these notification related calls into
functions of their own. So, for CPU PM domains, it would help to add
RCU_NONIDLE() around the notifiers, allowing the callbacks to add trace
functions.
--Lina
+ return ret;
}
/**
@@ -606,6 +617,9 @@ static int genpd_power_on(struct generic_pm_domain *genpd,
unsigned int depth)
if (ret)
goto err;
+ /* Inform consumers that we have powered on. */
+ raw_notifier_call_chain(>power_notifiers, GENPD_STATE_ON, NULL);
+
genpd->status = GENPD_STATE_ON;
genpd_update_accounting(genpd);
@@ -948,9 +962,18 @@ static void genpd_sync_power_off(struct generic_pm_domain
*genpd, bool use_lock,
/* Choose the deepest state when suspending */
genpd->state_idx = genpd->state_count - 1;
- if (_genpd_power_off(genpd, false))
+
+ /* Notify consumers that we are about to power off. */
+ if (raw_notifier_call_chain(>power_notifiers,
+ GENPD_STATE_OFF, NULL))
return;
+ if (_genpd_power_off(genpd, false)) {
+ raw_notifier_call_chain(>power_notifiers,
+ GENPD_STATE_ON, NULL);
+ return;
+ }
+
genpd->status = GENPD_STATE_OFF;
list_for_each_entry(link, >child_links, child_node) {
@@ -998,6 +1021,9 @@ static void genpd_sync_power_on(struct generic_pm_domain
*genpd, bool use_lock,
_genpd_power_on(genpd, false);
+ /* Inform consumers that we have powered on. */
+ raw_notifier_call_chain(>power_notifiers, GENPD_STATE_ON, NULL);
+
genpd->status = GENPD_STATE_ON;
}
@@ -1593,6 +1619,101 @@ int pm_genpd_remove_device(struct device *dev)
}
EXPORT_SYMBOL_GPL(pm_genpd_remove_device);
+/**
+ * dev_pm_genpd_add_notifier - Add a genpd power on/off notifier for @dev
+ *
+ * @dev: Device that should be associated with the notifier
+ * @nb: The notifier block to register
+ *
+ * Users may call this function to add a genpd power on/off notifier for an
+ * attached @dev. Only one notifier per device is allowed. The notifier is
+ * sent when genpd is powering on/off the PM domain.
+ *
+ * It is assumed that the user guarantee that the genpd wouldn't be detached
+ * while this routine is getting called.
+ *
+ * Returns 0 on success and negative error values on failures.
+
Re: [PATCH v4 4/5] arm64: dts: sdm845: Add OPP tables and power-domains for venus
On Tue, Jul 28 2020 at 13:51 -0600, Stephen Boyd wrote: Quoting Lina Iyer (2020-07-28 09:52:12) On Mon, Jul 27 2020 at 18:45 -0600, Stephen Boyd wrote: >Quoting Lina Iyer (2020-07-24 09:28:25) >> On Fri, Jul 24 2020 at 03:03 -0600, Rajendra Nayak wrote: >> >Hi Maulik/Lina, >> > >> >On 7/23/2020 11:36 PM, Stanimir Varbanov wrote: >> >>Hi Rajendra, >> >> >> >>After applying 2,3 and 4/5 patches on linaro-integration v5.8-rc2 I see >> >>below messages on db845: >> >> >> >>qcom-venus aa0.video-codec: dev_pm_opp_set_rate: failed to find >> >>current OPP for freq 53397 (-34) >> >> >> >>^^^ This one is new. >> >> >> >>qcom_rpmh TCS Busy, retrying RPMH message send: addr=0x3 >> >> >> >>^^^ and this message is annoying, can we make it pr_debug in rpmh? >> > >> How annoyingly often do you see this message? >> Usually, this is an indication of bad system state either on remote >> processors in the SoC or in Linux itself. On a smooth sailing build you >> should not see this 'warning'. >> >> >Would you be fine with moving this message to a pr_debug? Its currently >> >a pr_info_ratelimited() >> I would rather not, moving this out of sight will mask a lot serious >> issues that otherwise bring attention to the developers. >> > >I removed this warning message in my patch posted to the list[1]. If >it's a serious problem then I suppose a timeout is more appropriate, on >the order of several seconds or so and then a pr_warn() and bail out of >the async call with an error. > The warning used to capture issues that happen within a second and it helps capture system related issues. Timing out after many seconds overlooks the system issues that generally tend to resolve itself, but nevertheless need to be investigated. Is it correct to read "system related issues" as performance problems where the thread is spinning forever trying to send a message and it can't? So the problem is mostly that it's an unbounded amount of time before the message is sent to rpmh and this printk helps identify those situations where that is happening? Yes, but mostly a short period of time like when other processors are in the middle of a restart or resource states changes have taken unusual amounts of time. The system will generally recover from this without crashing in this case. User action is investigation of the situation leading to these messages. Otherwise as you say above it's a bad system state where the rpmh processor has gotten into a bad state like a crash? Can we recover from that? Or is the only recovery a reboot of the system? Does the rpmh processor reboot the system if it crashes? We cannot recover from such a state. The remote processor will reboot if it detects a failure at it's end. If the system entered a bad state, it is possible that RPMH requests start timing out in Linux and remote processor may not detect it. Hence, the timeout in rpmh_write() API. The advised course of action is a restart as there is no way to recover from this state. --Lina
Re: [PATCH v4 4/5] arm64: dts: sdm845: Add OPP tables and power-domains for venus
On Mon, Jul 27 2020 at 18:45 -0600, Stephen Boyd wrote: Quoting Lina Iyer (2020-07-24 09:28:25) On Fri, Jul 24 2020 at 03:03 -0600, Rajendra Nayak wrote: >Hi Maulik/Lina, > >On 7/23/2020 11:36 PM, Stanimir Varbanov wrote: >>Hi Rajendra, >> >>After applying 2,3 and 4/5 patches on linaro-integration v5.8-rc2 I see >>below messages on db845: >> >>qcom-venus aa0.video-codec: dev_pm_opp_set_rate: failed to find >>current OPP for freq 53397 (-34) >> >>^^^ This one is new. >> >>qcom_rpmh TCS Busy, retrying RPMH message send: addr=0x3 >> >>^^^ and this message is annoying, can we make it pr_debug in rpmh? > How annoyingly often do you see this message? Usually, this is an indication of bad system state either on remote processors in the SoC or in Linux itself. On a smooth sailing build you should not see this 'warning'. >Would you be fine with moving this message to a pr_debug? Its currently >a pr_info_ratelimited() I would rather not, moving this out of sight will mask a lot serious issues that otherwise bring attention to the developers. I removed this warning message in my patch posted to the list[1]. If it's a serious problem then I suppose a timeout is more appropriate, on the order of several seconds or so and then a pr_warn() and bail out of the async call with an error. The warning used to capture issues that happen within a second and it helps capture system related issues. Timing out after many seconds overlooks the system issues that generally tend to resolve itself, but nevertheless need to be investigated. --Lina [1] https://lore.kernel.org/r/20200724211711.810009-1-sb...@kernel.org
Re: [PATCH] soc: qcom: rpmh-rsc: Sleep waiting for tcs slots to be free
On Fri, Jul 24 2020 at 14:11 -0600, Stephen Boyd wrote: Quoting Lina Iyer (2020-07-24 13:08:41) On Fri, Jul 24 2020 at 14:01 -0600, Stephen Boyd wrote: >Quoting Doug Anderson (2020-07-24 12:49:56) >> Hi, >> >> On Fri, Jul 24, 2020 at 12:44 PM Stephen Boyd wrote: >I think Lina was alluding to this earlier in this >thread. I was thinking more of threaded irq handler than a kthread to post the requests. We went away from post-thread approach a few years ago. Ok, got it. Why was the kthread approach abandoned? Simplification mostly, I think. Also, somebody requested that when the async call returns they would atleast be guaranteed that the request has been posted not necessarily processed at the remote end.
Re: [PATCH] soc: qcom: rpmh-rsc: Sleep waiting for tcs slots to be free
On Fri, Jul 24 2020 at 14:01 -0600, Stephen Boyd wrote: Quoting Doug Anderson (2020-07-24 12:49:56) Hi, On Fri, Jul 24, 2020 at 12:44 PM Stephen Boyd wrote: I think Lina was alluding to this earlier in this thread. I was thinking more of threaded irq handler than a kthread to post the requests. We went away from post-thread approach a few years ago.
Re: [PATCH v4 4/5] arm64: dts: sdm845: Add OPP tables and power-domains for venus
On Fri, Jul 24 2020 at 03:03 -0600, Rajendra Nayak wrote: Hi Maulik/Lina, On 7/23/2020 11:36 PM, Stanimir Varbanov wrote: Hi Rajendra, After applying 2,3 and 4/5 patches on linaro-integration v5.8-rc2 I see below messages on db845: qcom-venus aa0.video-codec: dev_pm_opp_set_rate: failed to find current OPP for freq 53397 (-34) ^^^ This one is new. qcom_rpmh TCS Busy, retrying RPMH message send: addr=0x3 ^^^ and this message is annoying, can we make it pr_debug in rpmh? How annoyingly often do you see this message? Usually, this is an indication of bad system state either on remote processors in the SoC or in Linux itself. On a smooth sailing build you should not see this 'warning'. Would you be fine with moving this message to a pr_debug? Its currently a pr_info_ratelimited() I would rather not, moving this out of sight will mask a lot serious issues that otherwise bring attention to the developers. --Lina
Re: [PATCH] soc: qcom: rpmh-rsc: Sleep waiting for tcs slots to be free
On Wed, Jul 22 2020 at 19:01 -0600, Stephen Boyd wrote:
The busy loop in rpmh_rsc_send_data() is written with the assumption
that the udelay will be preempted by the tcs_tx_done() irq handler when
the TCS slots are all full. This doesn't hold true when the calling
thread is an irqthread and the tcs_tx_done() irq is also an irqthread.
That's because kernel irqthreads are SCHED_FIFO and thus need to
voluntarily give up priority by calling into the scheduler so that other
threads can run.
I see RCU stalls when I boot with irqthreads on the kernel commandline
because the modem remoteproc driver is trying to send an rpmh async
message from an irqthread that needs to give up the CPU for the rpmh
irqthread to run and clear out tcs slots.
Would this be not better, if we we use a threaded IRQ handler or offload
tx_done to another waitqueue instead of handling it in IRQ handler?
-- Lina
rcu: INFO: rcu_preempt self-detected stall on CPU
rcu: 0-: (1 GPs behind) idle=402/1/0x4002 softirq=2108/2109
fqs=4920
(t=21016 jiffies g=2933 q=590)
Task dump for CPU 0:
irq/11-smp2pR running task0 148 2 0x0028
Call trace:
dump_backtrace+0x0/0x154
show_stack+0x20/0x2c
sched_show_task+0xfc/0x108
dump_cpu_task+0x44/0x50
rcu_dump_cpu_stacks+0xa4/0xf8
rcu_sched_clock_irq+0x7dc/0xaa8
update_process_times+0x30/0x54
tick_sched_handle+0x50/0x64
tick_sched_timer+0x4c/0x8c
__hrtimer_run_queues+0x21c/0x36c
hrtimer_interrupt+0xf0/0x22c
arch_timer_handler_phys+0x40/0x50
handle_percpu_devid_irq+0x114/0x25c
__handle_domain_irq+0x84/0xc4
gic_handle_irq+0xd0/0x178
el1_irq+0xbc/0x180
save_return_addr+0x18/0x28
return_address+0x54/0x88
preempt_count_sub+0x40/0x88
_raw_spin_unlock_irqrestore+0x4c/0x6c
___ratelimit+0xd0/0x128
rpmh_rsc_send_data+0x24c/0x378
__rpmh_write+0x1b0/0x208
rpmh_write_async+0x90/0xbc
rpmhpd_send_corner+0x60/0x8c
rpmhpd_aggregate_corner+0x8c/0x124
rpmhpd_set_performance_state+0x8c/0xbc
_genpd_set_performance_state+0xdc/0x1b8
dev_pm_genpd_set_performance_state+0xb8/0xf8
q6v5_pds_disable+0x34/0x60 [qcom_q6v5_mss]
qcom_msa_handover+0x38/0x44 [qcom_q6v5_mss]
q6v5_handover_interrupt+0x24/0x3c [qcom_q6v5]
handle_nested_irq+0xd0/0x138
qcom_smp2p_intr+0x188/0x200
irq_thread_fn+0x2c/0x70
irq_thread+0xfc/0x14c
kthread+0x11c/0x12c
ret_from_fork+0x10/0x18
This busy loop naturally lends itself to using a wait queue so that each
thread that tries to send a message will sleep waiting on the waitqueue
and only be woken up when a free slot is available. This should make
things more predictable too because the scheduler will be able to sleep
tasks that are waiting on a free tcs instead of the busy loop we
currently have today.
Cc: Douglas Anderson
Cc: Maulik Shah
Cc: Lina Iyer
Signed-off-by: Stephen Boyd
---
drivers/soc/qcom/rpmh-internal.h | 2 +
drivers/soc/qcom/rpmh-rsc.c | 101 ---
2 files changed, 41 insertions(+), 62 deletions(-)
diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index ef60e790a750..9a325bac58fe 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -8,6 +8,7 @@
#define __RPM_INTERNAL_H__
#include
+#include
#include
#define TCS_TYPE_NR 4
@@ -118,6 +119,7 @@ struct rsc_drv {
struct tcs_group tcs[TCS_TYPE_NR];
DECLARE_BITMAP(tcs_in_use, MAX_TCS_NR);
spinlock_t lock;
+ wait_queue_head_t tcs_wait;
struct rpmh_ctrlr client;
};
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 076fd27f3081..6c758b052c95 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include
#include
@@ -444,6 +445,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
*/
if (!drv->tcs[ACTIVE_TCS].num_tcs)
enable_tcs_irq(drv, i, false);
+ wake_up(>tcs_wait);
spin_unlock(>lock);
if (req)
rpmh_tx_done(req, err);
@@ -562,44 +564,59 @@ static int find_free_tcs(struct tcs_group *tcs)
return -EBUSY;
}
+static int claim_tcs_for_req(struct rsc_drv *drv, struct tcs_group *tcs,
+const struct tcs_request *msg)
+{
+ int ret;
+
+ /*
+* The h/w does not like if we send a request to the same address,
+* when one is already in-flight or being processed.
+*/
+ ret = check_for_req_inflight(drv, tcs, msg);
+ if (ret)
+ return ret;
+
+ return find_free_tcs(tcs);
+}
+
/**
- * tcs_write() - Store messages into a TCS right now, or return -EBUSY.
+ * rpmh_rsc_send_data() - Write / trigger active-only message.
* @drv: The controller.
* @msg: The data to be sent.
*
- * Grabs a TCS for ACTIVE_ONLY transfers and writes the messages to it.
- *
- * If there are no free TCSes for AC
Re: [PATCH] soc: qcom: rpmh: Update rpmh_invalidate function to return void
On Thu, Jun 18 2020 at 07:06 -0600, Maulik Shah wrote:
Currently rpmh_invalidate() always returns success. Update its
return type to void.
Suggested-by: Stephen Boyd
Signed-off-by: Maulik Shah
Reviewed-by: Lina Iyer
---
drivers/interconnect/qcom/bcm-voter.c | 6 +-
drivers/soc/qcom/rpmh.c | 4 +---
include/soc/qcom/rpmh.h | 7 ---
3 files changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/interconnect/qcom/bcm-voter.c
b/drivers/interconnect/qcom/bcm-voter.c
index 2a11a63..a3d2ef1 100644
--- a/drivers/interconnect/qcom/bcm-voter.c
+++ b/drivers/interconnect/qcom/bcm-voter.c
@@ -266,11 +266,7 @@ int qcom_icc_bcm_voter_commit(struct bcm_voter *voter)
if (!commit_idx[0])
goto out;
- ret = rpmh_invalidate(voter->dev);
- if (ret) {
- pr_err("Error invalidating RPMH client (%d)\n", ret);
- goto out;
- }
+ rpmh_invalidate(voter->dev);
ret = rpmh_write_batch(voter->dev, RPMH_ACTIVE_ONLY_STATE,
cmds, commit_idx);
diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index f2b5b46c..b61e183 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c
@@ -497,7 +497,7 @@ int rpmh_flush(struct rpmh_ctrlr *ctrlr)
*
* Invalidate the sleep and wake values in batch_cache.
*/
-int rpmh_invalidate(const struct device *dev)
+void rpmh_invalidate(const struct device *dev)
{
struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
struct batch_cache_req *req, *tmp;
@@ -509,7 +509,5 @@ int rpmh_invalidate(const struct device *dev)
INIT_LIST_HEAD(>batch_cache);
ctrlr->dirty = true;
spin_unlock_irqrestore(>cache_lock, flags);
-
- return 0;
}
EXPORT_SYMBOL(rpmh_invalidate);
diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
index f9ec353..bdbee1a 100644
--- a/include/soc/qcom/rpmh.h
+++ b/include/soc/qcom/rpmh.h
@@ -20,7 +20,7 @@ int rpmh_write_async(const struct device *dev, enum
rpmh_state state,
int rpmh_write_batch(const struct device *dev, enum rpmh_state state,
const struct tcs_cmd *cmd, u32 *n);
-int rpmh_invalidate(const struct device *dev);
+void rpmh_invalidate(const struct device *dev);
#else
@@ -38,8 +38,9 @@ static inline int rpmh_write_batch(const struct device *dev,
const struct tcs_cmd *cmd, u32 *n)
{ return -ENODEV; }
-static inline int rpmh_invalidate(const struct device *dev)
-{ return -ENODEV; }
+static inline void rpmh_invalidate(const struct device *dev)
+{
+}
#endif /* CONFIG_QCOM_RPMH */
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation
Re: [RFC][PATCH 3/5] irqchip: Allow QCOM_PDC to be loadable as a perment module
On Tue, Jun 16 2020 at 05:30 -0600, Maulik Shah wrote:
Hi,
On 6/16/2020 11:43 AM, John Stultz wrote:
Allows qcom-pdc driver to be loaded as a permenent module
typo: permanent
Also, due to the fact that IRQCHIP_DECLARE becomes a no-op when
building as a module, we have to add the platform driver hooks
explicitly.
Thanks to Saravana for his help on pointing out the
IRQCHIP_DECLARE issue and guidance on a solution.
Cc: Andy Gross
Cc: Bjorn Andersson
Cc: Joerg Roedel
Cc: Thomas Gleixner
Cc: Jason Cooper
Cc: Marc Zyngier
Cc: Linus Walleij
Cc: Lina Iyer
Cc: Saravana Kannan
Cc: Todd Kjos
Cc: Greg Kroah-Hartman
Cc: linux-arm-...@vger.kernel.org
Cc: io...@lists.linux-foundation.org
Cc: linux-g...@vger.kernel.org
Signed-off-by: John Stultz
---
drivers/irqchip/Kconfig| 2 +-
drivers/irqchip/qcom-pdc.c | 30 ++
2 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 29fead208cad..12765bed08f9 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -425,7 +425,7 @@ config GOLDFISH_PIC
for Goldfish based virtual platforms.
config QCOM_PDC
- bool "QCOM PDC"
+ tristate "QCOM PDC"
depends on ARCH_QCOM
select IRQ_DOMAIN_HIERARCHY
help
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index 6ae9e1f0819d..98d74160afcd 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -11,7 +11,9 @@
#include
#include
#include
+#include
#include
+#include
please move this include in order after of_device.h
#include
#include
#include
@@ -430,4 +432,32 @@ static int qcom_pdc_init(struct device_node *node, struct
device_node *parent)
return ret;
}
+#ifdef MODULE
+static int qcom_pdc_probe(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct device_node *parent = of_irq_find_parent(np);
+
+ return qcom_pdc_init(np, parent);
+}
+
+static const struct of_device_id qcom_pdc_match_table[] = {
+ { .compatible = "qcom,pdc" },
+ {}
+};
+MODULE_DEVICE_TABLE(of, qcom_pdc_match_table);
+
+static struct platform_driver qcom_pdc_driver = {
+ .probe = qcom_pdc_probe,
+ .driver = {
+ .name = "qcom-pdc",
+ .of_match_table = qcom_pdc_match_table,
can you please set .suppress_bind_attrs = true,
This is to prevent bind/unbind using sysfs. Once irqchip driver module
is loaded, it shouldn't get unbind at runtime.
That is a good point. We probably should do that to RPMH RSC driver as well.
Thanks,
Maulik
+ },
+};
+module_platform_driver(qcom_pdc_driver);
+#else
IRQCHIP_DECLARE(qcom_pdc, "qcom,pdc", qcom_pdc_init);
+#endif
+
+MODULE_DESCRIPTION("Qualcomm Technologies, Inc. Power Domain Controller");
+MODULE_LICENSE("GPL v2");
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of
Code Aurora Forum, hosted by The Linux Foundation
Re: [PATCH RFC 02/14] drivers: irqchip: pdc: Do not toggle IRQ_ENABLE during mask/unmask
On Fri, Sep 20 2019 at 16:22 -0600, Stephen Boyd wrote: Quoting Lina Iyer (2019-09-11 09:15:57) On Thu, Sep 05 2019 at 18:39 -0600, Stephen Boyd wrote: >Quoting Lina Iyer (2019-08-29 11:11:51) >> When an interrupt is to be serviced, the convention is to mask the >> interrupt at the chip and unmask after servicing the interrupt. Enabling >> and disabling the interrupt at the PDC irqchip causes an interrupt storm >> due to the way dual edge interrupts are handled in hardware. >> >> Skip configuring the PDC when the IRQ is masked and unmasked, instead >> use the irq_enable/irq_disable callbacks to toggle the IRQ_ENABLE >> register at the PDC. The PDC's IRQ_ENABLE register is only used during >> the monitoring mode when the system is asleep and is not needed for >> active mode detection. > >I think this is saying that we want to always let the line be sent >through the PDC to the parent irqchip, in this case GIC, so that we >don't get an interrupt storm for dual edge interrupts? Why does dual >edge interrupts cause a problem? > I am not sure about the hardware details, but the PDC designers did not expect enable and disable to be called whenever the interrupt is handled. This specially becomes a problem for dual edge interrupts which seems to generate a interrupt storm when enabled/disabled while handling the interrupt. Ok. I just wanted to confirm that masking "doesn't matter" to the PDC because it assumes the irqchip closer to the CPU will be able to mask it anyway. Is that right? That is correct.
Re: [PATCH RFC 05/14] dt-bindings/interrupt-controller: pdc: add SPI config register
Adding Sibi
On Fri, Sep 13 2019 at 13:53 -0600, Lina Iyer wrote:
Sorry, I couldn't get to this earlier.
On Thu, Sep 05 2019 at 18:03 -0600, Stephen Boyd wrote:
Quoting Lina Iyer (2019-09-03 10:07:22)
On Mon, Sep 02 2019 at 07:58 -0600, Marc Zyngier wrote:
On 02/09/2019 14:38, Rob Herring wrote:
On Thu, Aug 29, 2019 at 12:11:54PM -0600, Lina Iyer wrote:
These are not GIC registers but located on the PDC interface to the GIC.
They may or may not be secure access controlled, depending on the SoC.
It looks like it falls under this "mailbox" device which is really the
catch all bucket for bits with no home besides they're related to the
apps CPUs/subsystem.
Thanks for pointing to this.
apss_shared: mailbox@1799 {
compatible = "qcom,sdm845-apss-shared";
reg = <0 0x1799 0 0x1000>;
But this doesn't seem correct. The registers in this page are all not
mailbox door bell registers. We should restrict the space allocated to
the mbox to 0xC or something, definitely, not the whole page. They all
cannot be treated as a mailbox registers.
#mbox-cells = <1>;
};
Can you point to this node with a phandle and then parse the reg
property out of it to use in the scm readl/writel APIs? Maybe it can be
a two cell property with <_shared 0xf0> to indicate the offset to
the registers to read/write? In non-secure mode presumably we need to
also write these registers? Good news is that there's a regmap for this
driver already, so maybe that can be acquired from the pdc driver.
The register space collection seems to be mix of different types of
application processor registers that should probably not be grouped up
under one subsystem. A single regmap doesn't seem correct either.
-- Lina
[PATCH RFC v2 12/14] arm64: dts: qcom: add PDC interrupt controller for SDM845
Add PDC interrupt controller device bindings for SDM845.
Signed-off-by: Lina Iyer
---
arch/arm64/boot/dts/qcom/sdm845.dtsi | 10 ++
1 file changed, 10 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi
b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index be0022e..41455b8 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -2375,6 +2375,16 @@
#power-domain-cells = <1>;
};
+ pdc_intc: interrupt-controller@b22 {
+ compatible = "qcom,sdm845-pdc";
+ reg = <0 0x0b22 0 0x3>, <0 0x179900f0 0 0x60>;
+ qcom,pdc-ranges = <0 480 94>, <94 609 15>, <115 630 7>;
+ #interrupt-cells = <2>;
+ interrupt-parent = <>;
+ interrupt-controller;
+ qcom,scm-spi-cfg;
+ };
+
pdc_reset: reset-controller@b2e {
compatible = "qcom,sdm845-pdc-global";
reg = <0 0x0b2e 0 0x2>;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH RFC v2 05/14] of: irq: document properties for wakeup interrupt parent
Some interrupt controllers in a SoC, are always powered on and have a select interrupts routed to them, so that they can wakeup the SoC from suspend. Add wakeup-parent DT property to refer to these interrupt controllers. Cc: devicet...@vger.kernel.org Signed-off-by: Lina Iyer Reviewed-by: Rob Herring --- .../devicetree/bindings/interrupt-controller/interrupts.txt | 13 + 1 file changed, 13 insertions(+) diff --git a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt index 8a3c408..c10e310 100644 --- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt +++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt @@ -108,3 +108,16 @@ commonly used: sensitivity = <7>; }; }; + +3) Interrupt wakeup parent +-- + +Some interrupt controllers in a SoC, are always powered on and have a select +interrupts routed to them, so that they can wakeup the SoC from suspend. These +interrupt controllers do not fall into the category of a parent interrupt +controller and can be specified by the "wakeup-parent" property and contain a +single phandle referring to the wakeup capable interrupt controller. + + Example: + wakeup-parent = <_intc>; + -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH RFC v2 04/14] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs
Introduce a new domain for wakeup capable GPIOs. The domain can be
requested using the bus token DOMAIN_BUS_WAKEUP. In the following
patches, we will specify PDC as the wakeup-parent for the TLMM GPIO
irqchip. Requesting a wakeup GPIO will setup the GPIO and the
corresponding PDC interrupt as its parent.
Co-developed-by: Stephen Boyd
Signed-off-by: Stephen Boyd
Signed-off-by: Lina Iyer
---
Changes in RFC v2:
- Move irq_domain_qcom_handle_wakeup to the patch where it is
used
- Replace #define definitons
- Add Signed-off-by and other minor changes
---
drivers/irqchip/qcom-pdc.c | 104 +++
include/linux/soc/qcom/irq.h | 19
2 files changed, 114 insertions(+), 9 deletions(-)
create mode 100644 include/linux/soc/qcom/irq.h
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index 5eef5ea..4abd775 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -13,12 +13,13 @@
#include
#include
#include
+#include
#include
-#include
#include
#include
#define PDC_MAX_IRQS 168
+#define PDC_MAX_GPIO_IRQS 256
#define CLEAR_INTR(reg, intr) (reg & ~(1 << intr))
#define ENABLE_INTR(reg, intr) (reg | (1 << intr))
@@ -26,6 +27,8 @@
#define IRQ_ENABLE_BANK0x10
#define IRQ_i_CFG 0x110
+#define PDC_NO_PARENT_IRQ ~0UL
+
struct pdc_pin_region {
u32 pin_base;
u32 parent_base;
@@ -65,23 +68,35 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
static void qcom_pdc_gic_disable(struct irq_data *d)
{
+ if (d->hwirq == GPIO_NO_WAKE_IRQ)
+ return;
+
pdc_enable_intr(d, false);
irq_chip_disable_parent(d);
}
static void qcom_pdc_gic_enable(struct irq_data *d)
{
+ if (d->hwirq == GPIO_NO_WAKE_IRQ)
+ return;
+
pdc_enable_intr(d, true);
irq_chip_enable_parent(d);
}
static void qcom_pdc_gic_mask(struct irq_data *d)
{
+ if (d->hwirq == GPIO_NO_WAKE_IRQ)
+ return;
+
irq_chip_mask_parent(d);
}
static void qcom_pdc_gic_unmask(struct irq_data *d)
{
+ if (d->hwirq == GPIO_NO_WAKE_IRQ)
+ return;
+
irq_chip_unmask_parent(d);
}
@@ -124,6 +139,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d,
unsigned int type)
int pin_out = d->hwirq;
enum pdc_irq_config_bits pdc_type;
+ if (pin_out == GPIO_NO_WAKE_IRQ)
+ return 0;
+
switch (type) {
case IRQ_TYPE_EDGE_RISING:
pdc_type = PDC_EDGE_RISING;
@@ -181,8 +199,7 @@ static irq_hw_number_t get_parent_hwirq(int pin)
return (region->parent_base + pin - region->pin_base);
}
- WARN_ON(1);
- return ~0UL;
+ return PDC_NO_PARENT_IRQ;
}
static int qcom_pdc_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
@@ -211,17 +228,17 @@ static int qcom_pdc_alloc(struct irq_domain *domain,
unsigned int virq,
ret = qcom_pdc_translate(domain, fwspec, , );
if (ret)
- return -EINVAL;
-
- parent_hwirq = get_parent_hwirq(hwirq);
- if (parent_hwirq == ~0UL)
- return -EINVAL;
+ return ret;
ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
_pdc_gic_chip, NULL);
if (ret)
return ret;
+ parent_hwirq = get_parent_hwirq(hwirq);
+ if (parent_hwirq == PDC_NO_PARENT_IRQ)
+ return 0;
+
if (type & IRQ_TYPE_EDGE_BOTH)
type = IRQ_TYPE_EDGE_RISING;
@@ -244,6 +261,60 @@ static const struct irq_domain_ops qcom_pdc_ops = {
.free = irq_domain_free_irqs_common,
};
+static int qcom_pdc_gpio_alloc(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs, void *data)
+{
+ struct irq_fwspec *fwspec = data;
+ struct irq_fwspec parent_fwspec;
+ irq_hw_number_t hwirq, parent_hwirq;
+ unsigned int type;
+ int ret;
+
+ ret = qcom_pdc_translate(domain, fwspec, , );
+ if (ret)
+ return ret;
+
+ ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+ _pdc_gic_chip, NULL);
+ if (ret)
+ return ret;
+
+ if (hwirq == GPIO_NO_WAKE_IRQ)
+ return 0;
+
+ parent_hwirq = get_parent_hwirq(hwirq);
+ if (parent_hwirq == PDC_NO_PARENT_IRQ)
+ return 0;
+
+ if (type & IRQ_TYPE_EDGE_BOTH)
+ type = IRQ_TYPE_EDGE_RISING;
+
+ if (type & IRQ_TYPE_LEVEL_MASK)
+ type = IRQ_TYPE_LEVEL_HIGH;
+
+ parent_fwspec.fwnode = domain->parent->fwnode;
+ parent_fwspec.param_count = 3;
+ parent_fwspec.param[0]= 0;
+ parent_fwspec.pa
[PATCH RFC v2 09/14] drivers: irqchip: pdc: Add irqchip set/get state calls
From: Maulik Shah
Add irqchip calls to set/get interrupt state from the parent interrupt
controller. When GPIOs are renabled as interrupt lines, it is desirable
to clear the interrupt state at the GIC. This avoids any unwanted
interrupt as a result of stale pending state recorded when the line was
used as a GPIO.
Signed-off-by: Maulik Shah
[updated commit text]
Signed-off-by: Lina Iyer
---
drivers/irqchip/qcom-pdc.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index affb0bfa..2b49e70 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -5,6 +5,7 @@
#include
#include
+#include
#include
#include
#include
@@ -87,6 +88,24 @@ static void qcom_pdc_gic_disable(struct irq_data *d)
irq_chip_disable_parent(d);
}
+static int qcom_pdc_gic_get_irqchip_state(struct irq_data *d,
+ enum irqchip_irq_state which, bool *state)
+{
+ if (d->hwirq == GPIO_NO_WAKE_IRQ)
+ return 0;
+
+ return irq_chip_get_parent_state(d, which, state);
+}
+
+static int qcom_pdc_gic_set_irqchip_state(struct irq_data *d,
+ enum irqchip_irq_state which, bool value)
+{
+ if (d->hwirq == GPIO_NO_WAKE_IRQ)
+ return 0;
+
+ return irq_chip_set_parent_state(d, which, value);
+}
+
static void qcom_pdc_gic_enable(struct irq_data *d)
{
if (d->hwirq == GPIO_NO_WAKE_IRQ)
@@ -248,6 +267,8 @@ static struct irq_chip qcom_pdc_gic_chip = {
.irq_unmask = qcom_pdc_gic_unmask,
.irq_disable= qcom_pdc_gic_disable,
.irq_enable = qcom_pdc_gic_enable,
+ .irq_get_irqchip_state = qcom_pdc_gic_get_irqchip_state,
+ .irq_set_irqchip_state = qcom_pdc_gic_set_irqchip_state,
.irq_retrigger = irq_chip_retrigger_hierarchy,
.irq_set_type = qcom_pdc_gic_set_type,
.flags = IRQCHIP_MASK_ON_SUSPEND |
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH RFC v2 14/14] arm64: defconfig: enable PDC interrupt controller for Qualcomm SDM845
Enable PDC interrupt controller for SDM845 devices. The interrupt controller can detect wakeup capable interrupts when the SoC is in a low power state. Signed-off-by: Lina Iyer --- arch/arm64/configs/defconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig index 0e58ef0..310b604 100644 --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@ -729,6 +729,7 @@ CONFIG_ARCH_R8A77970=y CONFIG_ARCH_R8A77980=y CONFIG_ARCH_R8A77990=y CONFIG_ARCH_R8A77995=y +CONFIG_QCOM_PDC=y CONFIG_ROCKCHIP_PM_DOMAINS=y CONFIG_ARCH_TEGRA_132_SOC=y CONFIG_ARCH_TEGRA_210_SOC=y -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH RFC v2 13/14] arm64: dts: qcom: setup PDC as the wakeup parent for TLMM on SDM845
PDC always-on interrupt controller can detect certain GPIOs even when
the TLMM interrupt controller is powered off. Link the PDC as TLMM's
wakeup parent.
Signed-off-by: Lina Iyer
---
arch/arm64/boot/dts/qcom/sdm845.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi
b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 41455b8..1b70254 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1358,6 +1358,7 @@
interrupt-controller;
#interrupt-cells = <2>;
gpio-ranges = < 0 0 150>;
+ wakeup-parent = <_intc>;
qspi_clk: qspi-clk {
pinmux {
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH RFC v2 10/14] drivers: pinctrl: msm: setup GPIO chip in hierarchy
Some GPIOs are marked as wakeup capable and are routed to another
interrupt controller that is an always-domain and can detect interrupts
even most of the SoC is powered off. The wakeup interrupt controller
wakes up the GIC and replays the interrupt at the GIC.
Setup the TLMM irqchip in hierarchy with the wakeup interrupt controller
and ensure the wakeup GPIOs are handled correctly.
Signed-off-by: Maulik Shah
Signed-off-by: Lina Iyer
Changes in RFC v2:
- Define irq_domain_qcom_handle_wakeup()
- Rebase on top of GPIO hierarchy support in linux-next
- Set the chained irq handler for summary line
---
drivers/pinctrl/qcom/pinctrl-msm.c | 119 +
drivers/pinctrl/qcom/pinctrl-msm.h | 16 +
include/linux/soc/qcom/irq.h | 13
3 files changed, 148 insertions(+)
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c
b/drivers/pinctrl/qcom/pinctrl-msm.c
index f355ddd..c5ba389 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -23,6 +23,8 @@
#include
#include
+#include
+
#include "../core.h"
#include "../pinconf.h"
#include "pinctrl-msm.h"
@@ -44,6 +46,7 @@
* @enabled_irqs: Bitmap of currently enabled irqs.
* @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
* detection.
+ * @skip_wake_irqs: Skip IRQs that are handled by wakeup interrupt contrroller
* @soc;Reference to soc_data of platform specific data.
* @regs: Base addresses for the TLMM tiles.
*/
@@ -61,6 +64,7 @@ struct msm_pinctrl {
DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
+ DECLARE_BITMAP(skip_wake_irqs, MAX_NR_GPIO);
const struct msm_pinctrl_soc_data *soc;
void __iomem *regs[MAX_NR_TILES];
@@ -709,6 +713,12 @@ static void msm_gpio_irq_mask(struct irq_data *d)
unsigned long flags;
u32 val;
+ if (d->parent_data)
+ irq_chip_mask_parent(d);
+
+ if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+ return;
+
g = >soc->groups[d->hwirq];
raw_spin_lock_irqsave(>lock, flags);
@@ -753,6 +763,12 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d,
bool status_clear)
unsigned long flags;
u32 val;
+ if (d->parent_data)
+ irq_chip_unmask_parent(d);
+
+ if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+ return;
+
g = >soc->groups[d->hwirq];
raw_spin_lock_irqsave(>lock, flags);
@@ -780,10 +796,37 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d,
bool status_clear)
static void msm_gpio_irq_enable(struct irq_data *d)
{
+ /*
+* Clear the interrupt that may be pending before we enable
+* the line.
+* This is especially a problem with the GPIOs routed to the
+* PDC. These GPIOs are direct-connect interrupts to the GIC.
+* Disabling the interrupt line at the PDC does not prevent
+* the interrupt from being latched at the GIC. The state at
+* GIC needs to be cleared before enabling.
+*/
+ if (d->parent_data) {
+ irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0);
+ irq_chip_enable_parent(d);
+ }
msm_gpio_irq_clear_unmask(d, true);
}
+static void msm_gpio_irq_disable(struct irq_data *d)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+ struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+
+ if (d->parent_data)
+ irq_chip_disable_parent(d);
+
+ if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+ return;
+
+ msm_gpio_irq_mask(d);
+}
+
static void msm_gpio_irq_unmask(struct irq_data *d)
{
msm_gpio_irq_clear_unmask(d, false);
@@ -797,6 +840,9 @@ static void msm_gpio_irq_ack(struct irq_data *d)
unsigned long flags;
u32 val;
+ if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+ return;
+
g = >soc->groups[d->hwirq];
raw_spin_lock_irqsave(>lock, flags);
@@ -822,6 +868,12 @@ static int msm_gpio_irq_set_type(struct irq_data *d,
unsigned int type)
unsigned long flags;
u32 val;
+ if (d->parent_data)
+ irq_chip_set_type_parent(d, type);
+
+ if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+ return 0;
+
g = >soc->groups[d->hwirq];
raw_spin_lock_irqsave(>lock, flags);
@@ -914,6 +966,15 @@ static int msm_gpio_irq_set_wake(struct irq_data *d,
unsigned int on)
struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
unsigned long flags;
+ if (d->parent_data)
+ irq_chip_set_wake_parent(d, on);
+
+ /*
+* While they may not wake up when the TLMM is powered off
[PATCH RFC v2 08/14] genirq: Introduce irq_chip_get/set_parent_state calls
From: Maulik Shah
On certain QTI chipsets some GPIOs are direct-connect interrupts to the
GIC to be used as regular interrupt lines. When the GPIOs are not used
for interrupt generation the interrupt line is disabled. But disabling
the interrupt at GIC does not prevent the interrupt to be reported as
pending at GIC_ISPEND. Later, when drivers call enable_irq() on the
interrupt, an unwanted interrupt occurs.
Introduce get and set methods for irqchip's parent to clear it's pending
irq state. This then can be invoked by the GPIO interrupt controller on
the parents in it hierarchy to clear the interrupt before enabling the
interrupt.
Signed-off-by: Maulik Shah
[updated commit text and minor code fixes]
Signed-off-by: Lina Iyer
---
Changes in RFC v2 -
- Rephrase commit text
- Address code review comments
---
include/linux/irq.h | 6 ++
kernel/irq/chip.c | 44
2 files changed, 50 insertions(+)
diff --git a/include/linux/irq.h b/include/linux/irq.h
index fb301cf..7853eb9 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -610,6 +610,12 @@ extern int irq_chip_pm_put(struct irq_data *data);
#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
extern void handle_fasteoi_ack_irq(struct irq_desc *desc);
extern void handle_fasteoi_mask_irq(struct irq_desc *desc);
+extern int irq_chip_set_parent_state(struct irq_data *data,
+enum irqchip_irq_state which,
+bool val);
+extern int irq_chip_get_parent_state(struct irq_data *data,
+enum irqchip_irq_state which,
+bool *state);
extern void irq_chip_enable_parent(struct irq_data *data);
extern void irq_chip_disable_parent(struct irq_data *data);
extern void irq_chip_ack_parent(struct irq_data *data);
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index b76703b..b3fa2d8 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -1298,6 +1298,50 @@ EXPORT_SYMBOL_GPL(handle_fasteoi_mask_irq);
#endif /* CONFIG_IRQ_FASTEOI_HIERARCHY_HANDLERS */
/**
+ * irq_chip_set_parent_state - set the state of a parent interrupt.
+ *
+ * @data: Pointer to interrupt specific data
+ * @which: State to be restored (one of IRQCHIP_STATE_*)
+ * @val: Value corresponding to @which
+ *
+ * Conditional success, if the underlying irqchip does not implement it.
+ */
+int irq_chip_set_parent_state(struct irq_data *data,
+ enum irqchip_irq_state which,
+ bool val)
+{
+ data = data->parent_data;
+
+ if (!data || !data->chip->irq_set_irqchip_state)
+ return 0;
+
+ return data->chip->irq_set_irqchip_state(data, which, val);
+}
+EXPORT_SYMBOL_GPL(irq_chip_set_parent_state);
+
+/**
+ * irq_chip_get_parent_state - get the state of a parent interrupt.
+ *
+ * @data: Pointer to interrupt specific data
+ * @which: one of IRQCHIP_STATE_* the caller wants to know
+ * @state: a pointer to a boolean where the state is to be stored
+ *
+ * Conditional success, if the underlying irqchip does not implement it.
+ */
+int irq_chip_get_parent_state(struct irq_data *data,
+ enum irqchip_irq_state which,
+ bool *state)
+{
+ data = data->parent_data;
+
+ if (!data || !data->chip->irq_get_irqchip_state)
+ return 0;
+
+ return data->chip->irq_get_irqchip_state(data, which, state);
+}
+EXPORT_SYMBOL_GPL(irq_chip_get_parent_state);
+
+/**
* irq_chip_enable_parent - Enable the parent interrupt (defaults to unmask if
* NULL)
* @data: Pointer to interrupt specific data
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH RFC v2 11/14] drivers: pinctrl: sdm845: add PDC wakeup interrupt map for GPIOs
Add interrupt parents for wakeup capable GPIOs for Qualcomm SDM845 SoC.
Signed-off-by: Lina Iyer
---
Changes in RFC v2:
- Rearranged GPIO wakeup parent map
---
drivers/pinctrl/qcom/pinctrl-sdm845.c | 23 ++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c
b/drivers/pinctrl/qcom/pinctrl-sdm845.c
index 39f498c..9cff3a4 100644
--- a/drivers/pinctrl/qcom/pinctrl-sdm845.c
+++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
/*
- * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2016-2019, The Linux Foundation. All rights reserved.
*/
#include
@@ -1282,6 +1282,24 @@ static const int sdm845_acpi_reserved_gpios[] = {
0, 1, 2, 3, 81, 82, 83, 84, -1
};
+static const struct msm_gpio_wakeirq_map sdm845_pdc_map[] = {
+ { 1, 30 }, { 3, 31 }, { 5, 32 }, { 10, 33 }, { 11, 34 },
+ { 20, 35 }, { 22, 36 }, { 24, 37 }, { 26, 38 }, { 30, 39 },
+ { 31, 117 }, { 32, 41 }, { 34, 42 }, { 36, 43 }, { 37, 44 },
+ { 38, 45 }, { 39, 46 }, { 40, 47 }, { 41, 115 }, { 43, 49 },
+ { 44, 50 }, { 46, 51 }, { 48, 52 }, { 49, 118 }, { 52, 54 },
+ { 53, 55 }, { 54, 56 }, { 56, 57 }, { 57, 58 }, { 58, 59 },
+ { 59, 60 }, { 60, 61 }, { 61, 62 }, { 62, 63 }, { 63, 64 },
+ { 64, 65 }, { 66, 66 }, { 68, 67 }, { 71, 68 }, { 73, 69 },
+ { 77, 70 }, { 78, 71 }, { 79, 72 }, { 80, 73 }, { 84, 74 },
+ { 85, 75 }, { 86, 76 }, { 88, 77 }, { 89, 116 }, { 91, 79 },
+ { 92, 80 }, { 95, 81 }, { 96, 82 }, { 97, 83 }, { 101, 84 },
+ { 103, 85 }, { 104, 86 }, { 115, 90 }, { 116, 91 }, { 117, 92 },
+ { 118, 93 }, { 119, 94 }, { 120, 95 }, { 121, 96 }, { 122, 97 },
+ { 123, 98 }, { 124, 99 }, { 125, 100 }, { 127, 102 }, { 128, 103 },
+ { 129, 104 }, { 130, 105 }, { 132, 106 }, { 133, 107 }, { 145, 108 },
+};
+
static const struct msm_pinctrl_soc_data sdm845_pinctrl = {
.pins = sdm845_pins,
.npins = ARRAY_SIZE(sdm845_pins),
@@ -1290,6 +1308,9 @@ static const struct msm_pinctrl_soc_data sdm845_pinctrl =
{
.groups = sdm845_groups,
.ngroups = ARRAY_SIZE(sdm845_groups),
.ngpios = 151,
+ .wakeirq_map = sdm845_pdc_map,
+ .nwakeirq_map = ARRAY_SIZE(sdm845_pdc_map),
+
};
static const struct msm_pinctrl_soc_data sdm845_acpi_pinctrl = {
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH RFC v2 07/14] drivers: irqchip: pdc: additionally set type in SPI config registers
GPIOs that can be configured as wakeup are routed to the PDC wakeup
interrupt controller and from there to the GIC interrupt controller. On
some QCOM SoCs, the interface to the GIC for wakeup capable GPIOs have
additional hardware registers that need to be configured as well to
match the trigger type of the GPIO. This register interfaces the PDC to
the GIC and therefore updated from the PDC driver.
Typically, the firmware intializes the interface registers for the
wakeup capable GPIOs with commonly used GPIO trigger type, but it is
possible that a platform may want to use the GPIO differently. So, in
addition to configuring the PDC, configure the interface registers as
well.
Signed-off-by: Lina Iyer
---
drivers/irqchip/qcom-pdc.c | 93 ++
1 file changed, 93 insertions(+)
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index 4abd775..affb0bfa 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -18,6 +18,8 @@
#include
#include
+#include
+
#define PDC_MAX_IRQS 168
#define PDC_MAX_GPIO_IRQS 256
@@ -35,10 +37,20 @@ struct pdc_pin_region {
u32 cnt;
};
+struct spi_cfg_regs {
+ union {
+ u64 start;
+ void __iomem *base;
+ };
+ resource_size_t size;
+ bool scm_io;
+};
+
static DEFINE_RAW_SPINLOCK(pdc_lock);
static void __iomem *pdc_base;
static struct pdc_pin_region *pdc_region;
static int pdc_region_cnt;
+static struct spi_cfg_regs *spi_cfg;
static void pdc_reg_write(int reg, u32 i, u32 val)
{
@@ -100,6 +112,57 @@ static void qcom_pdc_gic_unmask(struct irq_data *d)
irq_chip_unmask_parent(d);
}
+static u32 __spi_pin_read(unsigned int pin)
+{
+ void __iomem *cfg_reg = spi_cfg->base + pin * 4;
+ u64 scm_cfg_reg = spi_cfg->start + pin * 4;
+
+ if (spi_cfg->scm_io) {
+ unsigned int val;
+
+ qcom_scm_io_readl(scm_cfg_reg, );
+ return val;
+ } else {
+ return readl(cfg_reg);
+ }
+}
+
+static void __spi_pin_write(unsigned int pin, unsigned int val)
+{
+ void __iomem *cfg_reg = spi_cfg->base + pin * 4;
+ u64 scm_cfg_reg = spi_cfg->start + pin * 4;
+
+ if (spi_cfg->scm_io)
+ qcom_scm_io_writel(scm_cfg_reg, val);
+ else
+ writel(val, cfg_reg);
+}
+
+static int spi_configure_type(irq_hw_number_t hwirq, unsigned int type)
+{
+ int spi = hwirq - 32;
+ u32 pin = spi / 32;
+ u32 mask = BIT(spi % 32);
+ u32 val;
+ unsigned long flags;
+
+ if (!spi_cfg)
+ return 0;
+
+ if (pin * 4 > spi_cfg->size)
+ return -EFAULT;
+
+ raw_spin_lock_irqsave(_lock, flags);
+ val = __spi_pin_read(pin);
+ val &= ~mask;
+ if (type & IRQ_TYPE_LEVEL_MASK)
+ val |= mask;
+ __spi_pin_write(pin, val);
+ raw_spin_unlock_irqrestore(_lock, flags);
+
+ return 0;
+}
+
/*
* GIC does not handle falling edge or active low. To allow falling edge and
* active low interrupts to be handled at GIC, PDC has an inverter that inverts
@@ -137,7 +200,9 @@ enum pdc_irq_config_bits {
static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
{
int pin_out = d->hwirq;
+ int parent_hwirq = d->parent_data->hwirq;
enum pdc_irq_config_bits pdc_type;
+ int ret;
if (pin_out == GPIO_NO_WAKE_IRQ)
return 0;
@@ -168,6 +233,11 @@ static int qcom_pdc_gic_set_type(struct irq_data *d,
unsigned int type)
pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
+ /* Additionally, configure (only) the GPIO in the f/w */
+ ret = spi_configure_type(parent_hwirq, type);
+ if (ret)
+ return ret;
+
return irq_chip_set_type_parent(d, type);
}
@@ -354,6 +424,7 @@ static int pdc_setup_pin_mapping(struct device_node *np)
static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
{
struct irq_domain *parent_domain, *pdc_domain, *pdc_gpio_domain;
+ struct resource res;
int ret;
pdc_base = of_iomap(node, 0);
@@ -384,6 +455,27 @@ static int qcom_pdc_init(struct device_node *node, struct
device_node *parent)
goto fail;
}
+ ret = of_address_to_resource(node, 1, );
+ if (!ret) {
+ spi_cfg = kcalloc(1, sizeof(*spi_cfg), GFP_KERNEL);
+ if (!spi_cfg) {
+ ret = -ENOMEM;
+ goto remove;
+ }
+ spi_cfg->scm_io = of_find_property(node,
+ "qcom,scm-spi-cfg", NULL);
+ spi_cfg->size = resource_size();
+ if (spi_cfg->scm_io) {
+ spi_cfg->start = res.start;
+ } else {
+
[PATCH RFC v2 02/14] drivers: irqchip: qcom-pdc: update max PDC interrupts
Newer SoCs have increased the number of interrupts routed to the PDC interrupt controller. Update the definition of max PDC interrupts. Signed-off-by: Lina Iyer --- drivers/irqchip/qcom-pdc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c index faa7d61..b230794 100644 --- a/drivers/irqchip/qcom-pdc.c +++ b/drivers/irqchip/qcom-pdc.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 /* - * Copyright (c) 2017-2018, The Linux Foundation. All rights reserved. + * Copyright (c) 2017-2019, The Linux Foundation. All rights reserved. */ #include @@ -18,7 +18,7 @@ #include #include -#define PDC_MAX_IRQS 126 +#define PDC_MAX_IRQS 168 #define CLEAR_INTR(reg, intr) (reg & ~(1 << intr)) #define ENABLE_INTR(reg, intr) (reg | (1 << intr)) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH RFC v2 00/14] Support wakeup capable GPIOs
Thanks for all the helpful reviews. Here is the next revision addressing the comments. Changes in RFC v2: - Address review comments #3, #4, #6, #7, #8, #9, #10 - Rebased on top of linux-next GPIO latest patches [1],[3],[4] - Increase PDC max irqs in #2 (avoid merge conflicts with downstream) - Add Reviewed-by #5 Note: This revision does not update writing the config registers that are written from the PDC. There needs more discussion in that area. #6, #7 --- This series is another attempt on adding wakeup capable GPIOs for QCOM SoC. This patchset is based on Linus's support for hierarchical GPIOs merged into linux-next [1]. The essense of the idea remains the same as the previous submission [2]. GPIO irqchip TLMM is setup in hierarchy wit the PDC as the wakeup-parent. PDC's interrupt parent is the GIC. GPIOs in QCOM SoC that are wakeup capable (when TLMM is powered off) are routed to the PDC as well and can be detected at the always-on interrupt controller (PDC). The idea is setup the irqchips in hierarchy and if the interrupt is handled at the PDC, then TLMM relinquishes control and configuration of the interrupt to the PDC. There are few new additions in this submission. The first is the additional SPI configuration that needs to be done to setup the GPIO type in a register interface between the PDC and the GIC. This is needed only for GPIOs. This registers in some QCOM SoCs is access restricted and has to be written from the TZ. The DT bindings are also updated for this new requirement. The second change is that with the new hierarchical support in gpiolib, we could remove the .alloc and .translate functions from the pinctrl driver. But to distinguish the case where a wakeup interrupt controller needs the TLMM to configure the GPIO interrupts (in the case of MPM interrupt controller), irqdomain flags have been added. The third change is ensure the interrupt controllers' interrupt pending bits are cleared when the GPIO is enabled as an interrupt. Please consider reviewing these patches. Thanks, Lina [1]. https://lore.kernel.org/linux-gpio/20190808123242.5359-1-linus.wall...@linaro.org/ [2]. https://lkml.org/lkml/2019/5/7/1173 [3]. https://lore.kernel.org/r/20190819084904.30027-1-linus.wall...@linaro.org [4]. https://lore.kernel.org/r/20190724083828.7496-1-linus.wall...@linaro.org Lina Iyer (12): irqdomain: add bus token DOMAIN_BUS_WAKEUP drivers: irqchip: qcom-pdc: update max PDC interrupts drivers: irqchip: pdc: Do not toggle IRQ_ENABLE during mask/unmask drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs of: irq: document properties for wakeup interrupt parent dt-bindings/interrupt-controller: pdc: add SPI config register drivers: irqchip: pdc: additionally set type in SPI config registers drivers: pinctrl: msm: setup GPIO chip in hierarchy drivers: pinctrl: sdm845: add PDC wakeup interrupt map for GPIOs arm64: dts: qcom: add PDC interrupt controller for SDM845 arm64: dts: qcom: setup PDC as the wakeup parent for TLMM on SDM845 arm64: defconfig: enable PDC interrupt controller for Qualcomm SDM845 Maulik Shah (2): genirq: Introduce irq_chip_get/set_parent_state calls drivers: irqchip: pdc: Add irqchip set/get state calls .../bindings/interrupt-controller/interrupts.txt | 13 ++ .../bindings/interrupt-controller/qcom,pdc.txt | 13 +- arch/arm64/boot/dts/qcom/sdm845.dtsi | 11 + arch/arm64/configs/defconfig | 1 + drivers/irqchip/qcom-pdc.c | 238 +++-- drivers/pinctrl/qcom/pinctrl-msm.c | 119 +++ drivers/pinctrl/qcom/pinctrl-msm.h | 16 ++ drivers/pinctrl/qcom/pinctrl-sdm845.c | 23 +- include/linux/irq.h| 6 + include/linux/irqdomain.h | 1 + include/linux/soc/qcom/irq.h | 32 +++ kernel/irq/chip.c | 44 12 files changed, 502 insertions(+), 15 deletions(-) create mode 100644 include/linux/soc/qcom/irq.h -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH RFC v2 01/14] irqdomain: add bus token DOMAIN_BUS_WAKEUP
A single controller can handle normal interrupts and wake-up interrupts
independently, with a different numbering space. It is thus crucial to
allow the driver for such a controller discriminate between the two.
A simple way to do so is to tag the wake-up irqdomain with a "bus token"
that indicates the wake-up domain. This slightly abuses the notion of
bus, but also radically simplifies the design of such a driver. Between
two evils, we choose the least damaging.
Suggested-by: Stephen Boyd
Signed-off-by: Lina Iyer
---
include/linux/irqdomain.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 07ec8b3..cc846ab 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -83,6 +83,7 @@ enum irq_domain_bus_token {
DOMAIN_BUS_IPI,
DOMAIN_BUS_FSL_MC_MSI,
DOMAIN_BUS_TI_SCI_INTA_MSI,
+ DOMAIN_BUS_WAKEUP,
};
/**
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH RFC v2 03/14] drivers: irqchip: pdc: Do not toggle IRQ_ENABLE during mask/unmask
When an interrupt is to be serviced, the convention is to mask the
interrupt at the chip and unmask after servicing the interrupt. Enabling
and disabling the interrupt at the PDC irqchip causes an interrupt storm
due to the way dual edge interrupts are handled in hardware.
Skip configuring the PDC when the IRQ is masked and unmasked, instead
use the irq_enable/irq_disable callbacks to toggle the IRQ_ENABLE
register at the PDC. The PDC's IRQ_ENABLE register is only used during
the monitoring mode when the system is asleep and is not needed for
active mode detection.
Signed-off-by: Lina Iyer
---
drivers/irqchip/qcom-pdc.c | 16 ++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index b230794..5eef5ea 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -63,15 +63,25 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
raw_spin_unlock(_lock);
}
-static void qcom_pdc_gic_mask(struct irq_data *d)
+static void qcom_pdc_gic_disable(struct irq_data *d)
{
pdc_enable_intr(d, false);
+ irq_chip_disable_parent(d);
+}
+
+static void qcom_pdc_gic_enable(struct irq_data *d)
+{
+ pdc_enable_intr(d, true);
+ irq_chip_enable_parent(d);
+}
+
+static void qcom_pdc_gic_mask(struct irq_data *d)
+{
irq_chip_mask_parent(d);
}
static void qcom_pdc_gic_unmask(struct irq_data *d)
{
- pdc_enable_intr(d, true);
irq_chip_unmask_parent(d);
}
@@ -148,6 +158,8 @@ static struct irq_chip qcom_pdc_gic_chip = {
.irq_eoi= irq_chip_eoi_parent,
.irq_mask = qcom_pdc_gic_mask,
.irq_unmask = qcom_pdc_gic_unmask,
+ .irq_disable= qcom_pdc_gic_disable,
+ .irq_enable = qcom_pdc_gic_enable,
.irq_retrigger = irq_chip_retrigger_hierarchy,
.irq_set_type = qcom_pdc_gic_set_type,
.flags = IRQCHIP_MASK_ON_SUSPEND |
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH RFC v2 06/14] dt-bindings/interrupt-controller: pdc: add SPI config register
In addition to configuring the PDC, additional registers that interface
the GIC have to be configured to match the GPIO type. The registers on
some QCOM SoCs are access restricted, while on other SoCs are not. They
SoCs with access restriction to these SPI registers need to be written
from the firmware using the SCM interface. Add a flag to indicate if the
register is to be written using SCM interface.
Cc: devicet...@vger.kernel.org
Signed-off-by: Lina Iyer
---
.../devicetree/bindings/interrupt-controller/qcom,pdc.txt | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git
a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
index 8e0797c..e329f8d 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
@@ -24,6 +24,9 @@ Properties:
Usage: required
Value type:
Definition: Specifies the base physical address for PDC hardware.
+ Optionally, specify the PDC's GIC interface registers that
+ need to be configured for wakeup capable GPIOs routed to
+ the PDC.
- interrupt-cells:
Usage: required
@@ -50,15 +53,23 @@ Properties:
The second element is the GIC hwirq number for the PDC port.
The third element is the number of interrupts in sequence.
+- qcom,scm-spi-cfg:
+ Usage: optional
+ Value type:
+ Definition: Specifies if the SPI configuration registers have to be
+ written from the firmware. Sometimes the PDC interface
+ register to the GIC can only be written from the firmware.
+
Example:
pdc: interrupt-controller@b22 {
compatible = "qcom,sdm845-pdc";
- reg = <0xb22 0x3>;
+ reg = <0 0x0b22 0 0x3>, <0 0x179900f0 0 0x60>;
qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
#interrupt-cells = <2>;
interrupt-parent = <>;
interrupt-controller;
+ qcom,scm-spi-cfg;
};
DT binding of a device that wants to use the GIC SPI 514 as a wakeup
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH RFC 05/14] dt-bindings/interrupt-controller: pdc: add SPI config register
Sorry, I couldn't get to this earlier.
On Thu, Sep 05 2019 at 18:03 -0600, Stephen Boyd wrote:
Quoting Lina Iyer (2019-09-03 10:07:22)
On Mon, Sep 02 2019 at 07:58 -0600, Marc Zyngier wrote:
>On 02/09/2019 14:38, Rob Herring wrote:
>> On Thu, Aug 29, 2019 at 12:11:54PM -0600, Lina Iyer wrote:
These are not GIC registers but located on the PDC interface to the GIC.
They may or may not be secure access controlled, depending on the SoC.
It looks like it falls under this "mailbox" device which is really the
catch all bucket for bits with no home besides they're related to the
apps CPUs/subsystem.
Thanks for pointing to this.
apss_shared: mailbox@1799 {
compatible = "qcom,sdm845-apss-shared";
reg = <0 0x1799 0 0x1000>;
But this doesn't seem correct. The registers in this page are all not
mailbox door bell registers. We should restrict the space allocated to
the mbox to 0xC or something, definitely, not the whole page. They all
cannot be treated as a mailbox registers.
#mbox-cells = <1>;
};
Can you point to this node with a phandle and then parse the reg
property out of it to use in the scm readl/writel APIs? Maybe it can be
a two cell property with <_shared 0xf0> to indicate the offset to
the registers to read/write? In non-secure mode presumably we need to
also write these registers? Good news is that there's a regmap for this
driver already, so maybe that can be acquired from the pdc driver.
The register space collection seems to be mix of different types of
application processor registers that should probably not be grouped up
under one subsystem. A single regmap doesn't seem correct either.
-- Lina
Re: [PATCH RFC 09/14] drivers: pinctrl: msm: fix use of deprecated gpiolib APIs
On Wed, Sep 11 2019 at 04:19 -0600, Linus Walleij wrote: On Thu, Aug 29, 2019 at 7:35 PM Lina Iyer wrote: Replace gpiochip_irqchip_add() and gpiochip_set_chained_irqchip() calls by populating the gpio_irq_chip data structures instead. Signed-off-by: Lina Iyer This is mostly fixed upstream I think, but: + chip->irq.fwnode = pctrl->dev->fwnode; This fwnode assignment is missing though. Sorry for the constant churn and required rebasing... Not a problem. Will rebase.
Re: [PATCH RFC 02/14] drivers: irqchip: pdc: Do not toggle IRQ_ENABLE during mask/unmask
On Thu, Sep 05 2019 at 18:39 -0600, Stephen Boyd wrote: Quoting Lina Iyer (2019-08-29 11:11:51) When an interrupt is to be serviced, the convention is to mask the interrupt at the chip and unmask after servicing the interrupt. Enabling and disabling the interrupt at the PDC irqchip causes an interrupt storm due to the way dual edge interrupts are handled in hardware. Skip configuring the PDC when the IRQ is masked and unmasked, instead use the irq_enable/irq_disable callbacks to toggle the IRQ_ENABLE register at the PDC. The PDC's IRQ_ENABLE register is only used during the monitoring mode when the system is asleep and is not needed for active mode detection. I think this is saying that we want to always let the line be sent through the PDC to the parent irqchip, in this case GIC, so that we don't get an interrupt storm for dual edge interrupts? Why does dual edge interrupts cause a problem? I am not sure about the hardware details, but the PDC designers did not expect enable and disable to be called whenever the interrupt is handled. This specially becomes a problem for dual edge interrupts which seems to generate a interrupt storm when enabled/disabled while handling the interrupt. --Lina
Re: [PATCH RFC 05/14] dt-bindings/interrupt-controller: pdc: add SPI config register
On Wed, Sep 11 2019 at 04:05 -0600, Linus Walleij wrote:
On Thu, Aug 29, 2019 at 8:47 PM Lina Iyer wrote:
+- qcom,scm-spi-cfg:
+ Usage: optional
+ Value type:
+ Definition: Specifies if the SPI configuration registers have to be
+ written from the firmware.
+
Example:
pdc: interrupt-controller@b22 {
compatible = "qcom,sdm845-pdc";
- reg = <0xb22 0x3>;
+ reg = <0xb22 0x3>, <0x179900f0 0x60>;
qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
#interrupt-cells = <2>;
interrupt-parent = <>;
interrupt-controller;
+ qcom,scm-spi-cfg;
You can probably drop this bool if you just give names to the registers.
Like
reg = <0xb22 0x3>, <0x179900f0 0x60>;
reg-names = "gic", "pdc";
Then jus check explicitly for a "pdc" register and in that case
initialize the PDC.
Well the address remains the same. The bool defines how to access that
register address - from linux or from the firmware using SCM calls. But
I get your point, I could have different register namess - pdc-linux or
pdc-scm and request by name. I can then use that to determine the mode
for accessing the register.
Thanks,
Lina
Re: [PATCH RFC 05/14] dt-bindings/interrupt-controller: pdc: add SPI config register
On Mon, Sep 02 2019 at 07:58 -0600, Marc Zyngier wrote:
On 02/09/2019 14:38, Rob Herring wrote:
On Thu, Aug 29, 2019 at 12:11:54PM -0600, Lina Iyer wrote:
In addition to configuring the PDC, additional registers that interface
the GIC have to be configured to match the GPIO type. The registers on
some QCOM SoCs are access restricted, while on other SoCs are not. They
SoCs with access restriction to these SPI registers need to be written
Took me a minute to figure out this is GIC SPI interrupts, not SPI bus.
from the firmware using the SCM interface. Add a flag to indicate if the
register is to be written using SCM interface.
Cc: devicet...@vger.kernel.org
Signed-off-by: Lina Iyer
---
.../bindings/interrupt-controller/qcom,pdc.txt | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git
a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
index 8e0797cb1487..852fcba98ea6 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
@@ -50,15 +50,22 @@ Properties:
The second element is the GIC hwirq number for the PDC port.
The third element is the number of interrupts in sequence.
+- qcom,scm-spi-cfg:
+ Usage: optional
+ Value type:
+ Definition: Specifies if the SPI configuration registers have to be
+ written from the firmware.
+
Example:
pdc: interrupt-controller@b22 {
compatible = "qcom,sdm845-pdc";
- reg = <0xb22 0x3>;
+ reg = <0xb22 0x3>, <0x179900f0 0x60>;
There needs to be a description for reg updated. These aren't GIC
registers are they? Because those go in the GIC node.
They are not GIC registers. I will update this documentation.
This is completely insane. Why are the GIC registers configured as
secure the first place, if they are expected to be in control of the
non-secure?
These are not GIC registers but located on the PDC interface to the GIC.
They may or may not be secure access controlled, depending on the SoC.
Thanks,
Lina
Re: [PATCH RFC 03/14] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs
On Fri, Aug 30 2019 at 08:50 -0600, Marc Zyngier wrote:
[Please use my kernel.org address in the future. The days of this
arm.com address are numbered...]
Sure, will update and repost.
On 29/08/2019 19:11, Lina Iyer wrote:
Introduce a new domain for wakeup capable GPIOs. The domain can be
requested using the bus token DOMAIN_BUS_WAKEUP. In the following
patches, we will specify PDC as the wakeup-parent for the TLMM GPIO
irqchip. Requesting a wakeup GPIO will setup the GPIO and the
corresponding PDC interrupt as its parent.
Co-developed-by: Stephen Boyd
Signed-off-by: Lina Iyer
---
drivers/irqchip/qcom-pdc.c | 104 ---
include/linux/soc/qcom/irq.h | 34
2 files changed, 129 insertions(+), 9 deletions(-)
create mode 100644 include/linux/soc/qcom/irq.h
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index 338fae604af5..ad1faf634bcf 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -13,12 +13,13 @@
#include
#include
#include
+#include
#include
-#include
#include
#include
#define PDC_MAX_IRQS 126
+#define PDC_MAX_GPIO_IRQS 256
#define CLEAR_INTR(reg, intr) (reg & ~(1 << intr))
#define ENABLE_INTR(reg, intr) (reg | (1 << intr))
@@ -26,6 +27,8 @@
#define IRQ_ENABLE_BANK0x10
#define IRQ_i_CFG 0x110
+#define PDC_NO_PARENT_IRQ ~0UL
+
struct pdc_pin_region {
u32 pin_base;
u32 parent_base;
@@ -65,23 +68,35 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
static void qcom_pdc_gic_disable(struct irq_data *d)
{
+ if (d->hwirq == GPIO_NO_WAKE_IRQ)
+ return;
+
pdc_enable_intr(d, false);
irq_chip_disable_parent(d);
}
static void qcom_pdc_gic_enable(struct irq_data *d)
{
+ if (d->hwirq == GPIO_NO_WAKE_IRQ)
+ return;
+
pdc_enable_intr(d, true);
irq_chip_enable_parent(d);
}
static void qcom_pdc_gic_mask(struct irq_data *d)
{
+ if (d->hwirq == GPIO_NO_WAKE_IRQ)
+ return;
+
irq_chip_mask_parent(d);
}
static void qcom_pdc_gic_unmask(struct irq_data *d)
{
+ if (d->hwirq == GPIO_NO_WAKE_IRQ)
+ return;
+
irq_chip_unmask_parent(d);
}
@@ -124,6 +139,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d,
unsigned int type)
int pin_out = d->hwirq;
enum pdc_irq_config_bits pdc_type;
+ if (pin_out == GPIO_NO_WAKE_IRQ)
+ return 0;
+
switch (type) {
case IRQ_TYPE_EDGE_RISING:
pdc_type = PDC_EDGE_RISING;
@@ -181,8 +199,7 @@ static irq_hw_number_t get_parent_hwirq(int pin)
return (region->parent_base + pin - region->pin_base);
}
- WARN_ON(1);
- return ~0UL;
+ return PDC_NO_PARENT_IRQ;
}
static int qcom_pdc_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
@@ -211,17 +228,17 @@ static int qcom_pdc_alloc(struct irq_domain *domain,
unsigned int virq,
ret = qcom_pdc_translate(domain, fwspec, , );
if (ret)
- return -EINVAL;
-
- parent_hwirq = get_parent_hwirq(hwirq);
- if (parent_hwirq == ~0UL)
- return -EINVAL;
+ return ret;
ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
_pdc_gic_chip, NULL);
if (ret)
return ret;
+ parent_hwirq = get_parent_hwirq(hwirq);
+ if (parent_hwirq == PDC_NO_PARENT_IRQ)
+ return 0;
+
if (type & IRQ_TYPE_EDGE_BOTH)
type = IRQ_TYPE_EDGE_RISING;
@@ -244,6 +261,60 @@ static const struct irq_domain_ops qcom_pdc_ops = {
.free = irq_domain_free_irqs_common,
};
+static int qcom_pdc_gpio_alloc(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs, void *data)
+{
+ struct irq_fwspec *fwspec = data;
+ struct irq_fwspec parent_fwspec;
+ irq_hw_number_t hwirq, parent_hwirq;
+ unsigned int type;
+ int ret;
+
+ ret = qcom_pdc_translate(domain, fwspec, , );
+ if (ret)
+ return ret;
+
+ ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+ _pdc_gic_chip, NULL);
+ if (ret)
+ return ret;
+
+ if (hwirq == GPIO_NO_WAKE_IRQ)
+ return 0;
+
+ parent_hwirq = get_parent_hwirq(hwirq);
+ if (parent_hwirq == PDC_NO_PARENT_IRQ)
+ return 0;
+
+ if (type & IRQ_TYPE_EDGE_BOTH)
+ type = IRQ_TYPE_EDGE_RISING;
+
+ if (type & IRQ_TYPE_LEVEL_MASK)
+ type = IRQ_TYPE_LEVEL_HIGH;
+
+ parent_fwspec.fwnode = domain->parent->fwnode;
+ parent_fwspec.param_count = 3;
+ parent_fwspec.param[0]= 0;
+ parent_fwspec.pa
[PATCH RFC 01/14] irqdomain: add bus token DOMAIN_BUS_WAKEUP
A single controller can handle normal interrupts and wake-up interrupts
independently, with a different numbering space. It is thus crucial to
allow the driver for such a controller discriminate between the two.
A simple way to do so is to tag the wake-up irqdomain with a "bus token"
that indicates the wake-up domain. This slightly abuses the notion of
bus, but also radically simplifies the design of such a driver. Between
two evils, we choose the least damaging.
Suggested-by: Stephen Boyd
Signed-off-by: Lina Iyer
---
include/linux/irqdomain.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 07ec8b390161..cc846abeff28 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -83,6 +83,7 @@ enum irq_domain_bus_token {
DOMAIN_BUS_IPI,
DOMAIN_BUS_FSL_MC_MSI,
DOMAIN_BUS_TI_SCI_INTA_MSI,
+ DOMAIN_BUS_WAKEUP,
};
/**
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH RFC 05/14] dt-bindings/interrupt-controller: pdc: add SPI config register
In addition to configuring the PDC, additional registers that interface
the GIC have to be configured to match the GPIO type. The registers on
some QCOM SoCs are access restricted, while on other SoCs are not. They
SoCs with access restriction to these SPI registers need to be written
from the firmware using the SCM interface. Add a flag to indicate if the
register is to be written using SCM interface.
Cc: devicet...@vger.kernel.org
Signed-off-by: Lina Iyer
---
.../bindings/interrupt-controller/qcom,pdc.txt | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git
a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
index 8e0797cb1487..852fcba98ea6 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/qcom,pdc.txt
@@ -50,15 +50,22 @@ Properties:
The second element is the GIC hwirq number for the PDC port.
The third element is the number of interrupts in sequence.
+- qcom,scm-spi-cfg:
+ Usage: optional
+ Value type:
+ Definition: Specifies if the SPI configuration registers have to be
+ written from the firmware.
+
Example:
pdc: interrupt-controller@b22 {
compatible = "qcom,sdm845-pdc";
- reg = <0xb22 0x3>;
+ reg = <0xb22 0x3>, <0x179900f0 0x60>;
qcom,pdc-ranges = <0 512 94>, <94 641 15>, <115 662 7>;
#interrupt-cells = <2>;
interrupt-parent = <>;
interrupt-controller;
+ qcom,scm-spi-cfg;
};
DT binding of a device that wants to use the GIC SPI 514 as a wakeup
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH RFC 10/14] drivers: pinctrl: msm: setup GPIO chip in hierarchy
Some GPIOs are marked as wakeup capable and are routed to another
interrupt controller that is an always-domain and can detect interrupts
even most of the SoC is powered off. The wakeup interrupt controller
wakes up the GIC and replays the interrupt at the GIC.
Setup the TLMM irqchip in hierarchy with the wakeup interrupt controller
and ensure the wakeup GPIOs are handled correctly.
Signed-off-by: Maulik Shah
Signed-off-by: Lina Iyer
---
drivers/pinctrl/qcom/pinctrl-msm.c | 114 +
drivers/pinctrl/qcom/pinctrl-msm.h | 16
2 files changed, 130 insertions(+)
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c
b/drivers/pinctrl/qcom/pinctrl-msm.c
index 76e8528e4d0a..d626264fe678 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -23,6 +23,8 @@
#include
#include
+#include
+
#include "../core.h"
#include "../pinconf.h"
#include "pinctrl-msm.h"
@@ -44,6 +46,7 @@
* @enabled_irqs: Bitmap of currently enabled irqs.
* @dual_edge_irqs: Bitmap of irqs that need sw emulated dual edge
* detection.
+ * @skip_wake_irqs: Skip IRQs that are handled by wakeup interrupt contrroller
* @soc;Reference to soc_data of platform specific data.
* @regs: Base addresses for the TLMM tiles.
*/
@@ -61,6 +64,7 @@ struct msm_pinctrl {
DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
+ DECLARE_BITMAP(skip_wake_irqs, MAX_NR_GPIO);
const struct msm_pinctrl_soc_data *soc;
void __iomem *regs[MAX_NR_TILES];
@@ -708,6 +712,12 @@ static void msm_gpio_irq_mask(struct irq_data *d)
unsigned long flags;
u32 val;
+ if (d->parent_data)
+ irq_chip_mask_parent(d);
+
+ if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+ return;
+
g = >soc->groups[d->hwirq];
raw_spin_lock_irqsave(>lock, flags);
@@ -752,6 +762,12 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d,
bool status_clear)
unsigned long flags;
u32 val;
+ if (d->parent_data)
+ irq_chip_unmask_parent(d);
+
+ if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+ return;
+
g = >soc->groups[d->hwirq];
raw_spin_lock_irqsave(>lock, flags);
@@ -779,10 +795,43 @@ static void msm_gpio_irq_clear_unmask(struct irq_data *d,
bool status_clear)
static void msm_gpio_irq_enable(struct irq_data *d)
{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+ struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+
+ /*
+* Clear the interrupt that may be pending before we enable
+* the line.
+* This is especially a problem with the GPIOs routed to the
+* PDC. These GPIOs are direct-connect interrupts to the GIC.
+* Disabling the interrupt line at the PDC does not prevent
+* the interrupt from being latched at the GIC. The state at
+* GIC needs to be cleared before enabling.
+*/
+ if (d->parent_data) {
+ irq_chip_set_parent_state(d, IRQCHIP_STATE_PENDING, 0);
+ irq_chip_enable_parent(d);
+ }
+
+ if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+ return;
msm_gpio_irq_clear_unmask(d, true);
}
+static void msm_gpio_irq_disable(struct irq_data *d)
+{
+ struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+ struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
+
+ if (d->parent_data)
+ irq_chip_disable_parent(d);
+
+ if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+ return;
+
+ msm_gpio_irq_mask(d);
+}
+
static void msm_gpio_irq_unmask(struct irq_data *d)
{
msm_gpio_irq_clear_unmask(d, false);
@@ -796,6 +845,9 @@ static void msm_gpio_irq_ack(struct irq_data *d)
unsigned long flags;
u32 val;
+ if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+ return;
+
g = >soc->groups[d->hwirq];
raw_spin_lock_irqsave(>lock, flags);
@@ -821,6 +873,12 @@ static int msm_gpio_irq_set_type(struct irq_data *d,
unsigned int type)
unsigned long flags;
u32 val;
+ if (d->parent_data)
+ irq_chip_set_type_parent(d, type);
+
+ if (test_bit(d->hwirq, pctrl->skip_wake_irqs))
+ return 0;
+
g = >soc->groups[d->hwirq];
raw_spin_lock_irqsave(>lock, flags);
@@ -913,6 +971,15 @@ static int msm_gpio_irq_set_wake(struct irq_data *d,
unsigned int on)
struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
unsigned long flags;
+ if (d->parent_data)
+ irq_chip_set_wake_parent(d, on);
+
+ /*
+* While they may not wake up when the TLMM is powered off,
+* some GPIOs wou
[PATCH RFC 06/14] drivers: irqchip: pdc: additionally set type in SPI config registers
GPIOs that can be configured as wakeup are routed to the PDC wakeup
interrupt controller and from there to the GIC interrupt controller. On
some QCOM SoCs, the interface to the GIC for wakeup capable GPIOs have
additional hardware registers that need to be configured as well to
match the trigger type of the GPIO. This register interfaces the PDC to
the GIC and therefore updated from the PDC driver.
Typically, the firmware intializes the interface registers for the
wakeup capable GPIOs with commonly used GPIO trigger type, but it is
possible that a platform may want to use the GPIO differently. So, in
addition to configuring the PDC, configure the interface registers as
well.
Signed-off-by: Lina Iyer
---
drivers/irqchip/qcom-pdc.c | 93 ++
1 file changed, 93 insertions(+)
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index ad1faf634bcf..bf5f98bb4d2b 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -18,6 +18,8 @@
#include
#include
+#include
+
#define PDC_MAX_IRQS 126
#define PDC_MAX_GPIO_IRQS 256
@@ -35,10 +37,20 @@ struct pdc_pin_region {
u32 cnt;
};
+struct spi_cfg_regs {
+ union {
+ u64 start;
+ void __iomem *base;
+ };
+ resource_size_t size;
+ bool scm_io;
+};
+
static DEFINE_RAW_SPINLOCK(pdc_lock);
static void __iomem *pdc_base;
static struct pdc_pin_region *pdc_region;
static int pdc_region_cnt;
+static struct spi_cfg_regs *spi_cfg;
static void pdc_reg_write(int reg, u32 i, u32 val)
{
@@ -100,6 +112,57 @@ static void qcom_pdc_gic_unmask(struct irq_data *d)
irq_chip_unmask_parent(d);
}
+static u32 __spi_pin_read(unsigned int pin)
+{
+ void __iomem *cfg_reg = spi_cfg->base + pin * 4;
+ u64 scm_cfg_reg = spi_cfg->start + pin * 4;
+
+ if (spi_cfg->scm_io) {
+ unsigned int val;
+
+ qcom_scm_io_readl(scm_cfg_reg, );
+ return val;
+ } else {
+ return readl(cfg_reg);
+ }
+}
+
+static void __spi_pin_write(unsigned int pin, unsigned int val)
+{
+ void __iomem *cfg_reg = spi_cfg->base + pin * 4;
+ u64 scm_cfg_reg = spi_cfg->start + pin * 4;
+
+ if (spi_cfg->scm_io)
+ qcom_scm_io_writel(scm_cfg_reg, val);
+ else
+ writel(val, cfg_reg);
+}
+
+static int spi_configure_type(irq_hw_number_t hwirq, unsigned int type)
+{
+ int spi = hwirq - 32;
+ u32 pin = spi / 32;
+ u32 mask = BIT(spi % 32);
+ u32 val;
+ unsigned long flags;
+
+ if (!spi_cfg)
+ return 0;
+
+ if (pin * 4 > spi_cfg->size)
+ return -EFAULT;
+
+ raw_spin_lock_irqsave(_lock, flags);
+ val = __spi_pin_read(pin);
+ val &= ~mask;
+ if (type & IRQ_TYPE_LEVEL_MASK)
+ val |= mask;
+ __spi_pin_write(pin, val);
+ raw_spin_unlock_irqrestore(_lock, flags);
+
+ return 0;
+}
+
/*
* GIC does not handle falling edge or active low. To allow falling edge and
* active low interrupts to be handled at GIC, PDC has an inverter that inverts
@@ -137,7 +200,9 @@ enum pdc_irq_config_bits {
static int qcom_pdc_gic_set_type(struct irq_data *d, unsigned int type)
{
int pin_out = d->hwirq;
+ int parent_hwirq = d->parent_data->hwirq;
enum pdc_irq_config_bits pdc_type;
+ int ret;
if (pin_out == GPIO_NO_WAKE_IRQ)
return 0;
@@ -168,6 +233,11 @@ static int qcom_pdc_gic_set_type(struct irq_data *d,
unsigned int type)
pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
+ /* Additionally, configure (only) the GPIO in the f/w */
+ ret = spi_configure_type(parent_hwirq, type);
+ if (ret)
+ return ret;
+
return irq_chip_set_type_parent(d, type);
}
@@ -354,6 +424,7 @@ static int pdc_setup_pin_mapping(struct device_node *np)
static int qcom_pdc_init(struct device_node *node, struct device_node *parent)
{
struct irq_domain *parent_domain, *pdc_domain, *pdc_gpio_domain;
+ struct resource res;
int ret;
pdc_base = of_iomap(node, 0);
@@ -384,6 +455,27 @@ static int qcom_pdc_init(struct device_node *node, struct
device_node *parent)
goto fail;
}
+ ret = of_address_to_resource(node, 1, );
+ if (!ret) {
+ spi_cfg = kcalloc(1, sizeof(*spi_cfg), GFP_KERNEL);
+ if (!spi_cfg) {
+ ret = -ENOMEM;
+ goto remove;
+ }
+ spi_cfg->scm_io = of_find_property(node,
+ "qcom,scm-spi-cfg", NULL);
+ spi_cfg->size = resource_size();
+ if (spi_cfg->scm_io) {
+ spi_cfg->start = res.start;
+ } else {
+
[PATCH RFC 00/14] qcom: support wakeup capable GPIOs
This series is another attempt on adding wakeup capable GPIOs for QCOM SoC. This patchset is based on Linus's support for hierarchical GPIOs merged into linux-next [1]. The essense of the idea remains the same as the previous submission [2]. GPIO irqchip TLMM is setup in hierarchy with the PDC as the wakeup-parent. PDC's interrupt parent is the GIC. GPIOs in QCOM SoC that are wakeup capable (when TLMM is powered off) are routed to the PDC as well and can be detected at the always-on interrupt controller (PDC). The idea is setup the irqchips in hierarchy and if the interrupt is handled at the PDC, then TLMM relinquishes control and configuration of the interrupt to the PDC. There are few new additions in this submission. The first is the additional SPI configuration that needs to be done to setup the GPIO type in a register interface between the PDC and the GIC. This is needed only for GPIOs. This registers in some QCOM SoCs is access restricted and has to be written from the TZ. The DT bindings are also updated for this new requirement. The second change is that with the new hierarchical support in gpiolib, we could remove the .alloc and .translate functions from the pinctrl driver. But to distinguish the case where a wakeup interrupt controller needs the TLMM to configure the GPIO interrupts (in the case of MPM interrupt controller), irqdomain flags have been added. The third change is ensure the interrupt controllers' interrupt pending bits are cleared when the GPIO is enabled as an interrupt. Please consider reviewing these patches. Thanks, Lina Lina Iyer (12): irqdomain: add bus token DOMAIN_BUS_WAKEUP drivers: irqchip: pdc: Do not toggle IRQ_ENABLE during mask/unmask drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs of: irq: document properties for wakeup interrupt parent dt-bindings/interrupt-controller: pdc: add SPI config register drivers: irqchip: pdc: additionally set type in SPI config registers drivers: pinctrl: msm: fix use of deprecated gpiolib APIs drivers: pinctrl: msm: setup GPIO chip in hierarchy drivers: pinctrl: sdm845: add PDC wakeup interrupt map for GPIOs arm64: dts: qcom: add PDC interrupt controller for SDM845 arm64: dts: qcom: setup PDC as the wakeup parent for TLMM on SDM845 arm64: defconfig: enable PDC interrupt controller for Qualcomm SDM845 Maulik Shah (2): genirq: Introduce irq_chip_get/set_parent_state calls drivers: irqchip: pdc: Add irqchip set/get state calls .../interrupt-controller/interrupts.txt | 13 + .../interrupt-controller/qcom,pdc.txt | 9 +- arch/arm64/boot/dts/qcom/sdm845.dtsi | 11 + arch/arm64/configs/defconfig | 1 + drivers/irqchip/qcom-pdc.c| 234 +- drivers/pinctrl/qcom/pinctrl-msm.c| 142 +-- drivers/pinctrl/qcom/pinctrl-msm.h| 16 ++ drivers/pinctrl/qcom/pinctrl-sdm845.c | 83 ++- include/linux/irq.h | 6 + include/linux/irqdomain.h | 1 + include/linux/soc/qcom/irq.h | 34 +++ kernel/irq/chip.c | 44 12 files changed, 566 insertions(+), 28 deletions(-) create mode 100644 include/linux/soc/qcom/irq.h -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH RFC 13/14] arm64: dts: qcom: setup PDC as the wakeup parent for TLMM on SDM845
PDC always-on interrupt controller can detect certain GPIOs even when
the TLMM interrupt controller is powered off. Link the PDC as TLMM's
wakeup parent.
Signed-off-by: Lina Iyer
---
arch/arm64/boot/dts/qcom/sdm845.dtsi | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi
b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index ffe28b3e41d8..3002793ee688 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1358,6 +1358,7 @@
interrupt-controller;
#interrupt-cells = <2>;
gpio-ranges = < 0 0 150>;
+ wakeup-parent = <_intc>;
qspi_clk: qspi-clk {
pinmux {
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH RFC 04/14] of: irq: document properties for wakeup interrupt parent
Some interrupt controllers in a SoC, are always powered on and have a select interrupts routed to them, so that they can wakeup the SoC from suspend. Add wakeup-parent DT property to refer to these interrupt controllers. Cc: devicet...@vger.kernel.org Signed-off-by: Lina Iyer --- .../bindings/interrupt-controller/interrupts.txt| 13 + 1 file changed, 13 insertions(+) diff --git a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt index 8a3c40829899..c10e31050dd2 100644 --- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt +++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt @@ -108,3 +108,16 @@ commonly used: sensitivity = <7>; }; }; + +3) Interrupt wakeup parent +-- + +Some interrupt controllers in a SoC, are always powered on and have a select +interrupts routed to them, so that they can wakeup the SoC from suspend. These +interrupt controllers do not fall into the category of a parent interrupt +controller and can be specified by the "wakeup-parent" property and contain a +single phandle referring to the wakeup capable interrupt controller. + + Example: + wakeup-parent = <_intc>; + -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH RFC 08/14] drivers: irqchip: pdc: Add irqchip set/get state calls
From: Maulik Shah
Add irqchip calls to set/get interrupt status from the parent interrupt
controller.
Signed-off-by: Maulik Shah
---
drivers/irqchip/qcom-pdc.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index bf5f98bb4d2b..ffd5f83d1023 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -5,6 +5,7 @@
#include
#include
+#include
#include
#include
#include
@@ -87,6 +88,24 @@ static void qcom_pdc_gic_disable(struct irq_data *d)
irq_chip_disable_parent(d);
}
+static int qcom_pdc_gic_get_irqchip_state(struct irq_data *d,
+ enum irqchip_irq_state which, bool *state)
+{
+ if (d->hwirq == GPIO_NO_WAKE_IRQ)
+ return 0;
+
+ return irq_chip_get_parent_state(d, which, state);
+}
+
+static int qcom_pdc_gic_set_irqchip_state(struct irq_data *d,
+ enum irqchip_irq_state which, bool value)
+{
+ if (d->hwirq == GPIO_NO_WAKE_IRQ)
+ return 0;
+
+ return irq_chip_set_parent_state(d, which, value);
+}
+
static void qcom_pdc_gic_enable(struct irq_data *d)
{
if (d->hwirq == GPIO_NO_WAKE_IRQ)
@@ -248,6 +267,8 @@ static struct irq_chip qcom_pdc_gic_chip = {
.irq_unmask = qcom_pdc_gic_unmask,
.irq_disable= qcom_pdc_gic_disable,
.irq_enable = qcom_pdc_gic_enable,
+ .irq_get_irqchip_state = qcom_pdc_gic_get_irqchip_state,
+ .irq_set_irqchip_state = qcom_pdc_gic_set_irqchip_state,
.irq_retrigger = irq_chip_retrigger_hierarchy,
.irq_set_type = qcom_pdc_gic_set_type,
.flags = IRQCHIP_MASK_ON_SUSPEND |
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH RFC 07/14] genirq: Introduce irq_chip_get/set_parent_state calls
From: Maulik Shah
On certain QTI chipsets some GPIOs are direct-connect interrupts
to the GIC.
Even when GPIOs are not used for interrupt generation and interrupt
line is disabled, it does not prevent interrupt to get pending at
GIC_ISPEND. When drivers call enable_irq unwanted interrupt occures.
Introduce irq_chip_get/set_parent_state calls to clear pending irq
which can get called within irq_enable of child irq chip to clear
any pending irq before enabling.
Signed-off-by: Maulik Shah
---
include/linux/irq.h | 6 ++
kernel/irq/chip.c | 44
2 files changed, 50 insertions(+)
diff --git a/include/linux/irq.h b/include/linux/irq.h
index fb301cf29148..7853eb9301f2 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -610,6 +610,12 @@ extern int irq_chip_pm_put(struct irq_data *data);
#ifdef CONFIG_IRQ_DOMAIN_HIERARCHY
extern void handle_fasteoi_ack_irq(struct irq_desc *desc);
extern void handle_fasteoi_mask_irq(struct irq_desc *desc);
+extern int irq_chip_set_parent_state(struct irq_data *data,
+enum irqchip_irq_state which,
+bool val);
+extern int irq_chip_get_parent_state(struct irq_data *data,
+enum irqchip_irq_state which,
+bool *state);
extern void irq_chip_enable_parent(struct irq_data *data);
extern void irq_chip_disable_parent(struct irq_data *data);
extern void irq_chip_ack_parent(struct irq_data *data);
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index b76703b2c0af..6bb5b22bb0a7 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -1297,6 +1297,50 @@ EXPORT_SYMBOL_GPL(handle_fasteoi_mask_irq);
#endif /* CONFIG_IRQ_FASTEOI_HIERARCHY_HANDLERS */
+/**
+ * irq_chip_set_parent_state - set the state of a parent interrupt.
+ * @data: Pointer to interrupt specific data
+ * @which: State to be restored (one of IRQCHIP_STATE_*)
+ * @val: Value corresponding to @which
+ *
+ */
+int irq_chip_set_parent_state(struct irq_data *data,
+ enum irqchip_irq_state which,
+ bool val)
+{
+ data = data->parent_data;
+ if (!data)
+ return 0;
+
+ if (data->chip->irq_set_irqchip_state)
+ return data->chip->irq_set_irqchip_state(data, which, val);
+
+ return 0;
+}
+EXPORT_SYMBOL(irq_chip_set_parent_state);
+
+/**
+ * irq_chip_get_parent_state - get the state of a parent interrupt.
+ * @data: Pointer to interrupt specific data
+ * @which: one of IRQCHIP_STATE_* the caller wants to know
+ * @state: a pointer to a boolean where the state is to be stored
+ *
+ */
+int irq_chip_get_parent_state(struct irq_data *data,
+ enum irqchip_irq_state which,
+ bool *state)
+{
+ data = data->parent_data;
+ if (!data)
+ return 0;
+
+ if (data->chip->irq_get_irqchip_state)
+ return data->chip->irq_get_irqchip_state(data, which, state);
+
+ return 0;
+}
+EXPORT_SYMBOL(irq_chip_get_parent_state);
+
/**
* irq_chip_enable_parent - Enable the parent interrupt (defaults to unmask if
* NULL)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH RFC 09/14] drivers: pinctrl: msm: fix use of deprecated gpiolib APIs
Replace gpiochip_irqchip_add() and gpiochip_set_chained_irqchip() calls
by populating the gpio_irq_chip data structures instead.
Signed-off-by: Lina Iyer
---
drivers/pinctrl/qcom/pinctrl-msm.c | 28 +---
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c
b/drivers/pinctrl/qcom/pinctrl-msm.c
index 7f35c196bb3e..76e8528e4d0a 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -1027,7 +1027,19 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres;
pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres;
- ret = gpiochip_add_data(>chip, pctrl);
+ chip->irq.chip = >irq_chip;
+ chip->irq.default_type = IRQ_TYPE_NONE;
+ chip->irq.handler = handle_bad_irq;
+ chip->irq.fwnode = pctrl->dev->fwnode;
+ chip->irq.parent_handler = msm_gpio_irq_handler;
+ chip->irq.num_parents = 1;
+ chip->irq.parents = devm_kcalloc(pctrl->dev, 1,
+sizeof(*chip->irq.parents),
+GFP_KERNEL);
+ if (!chip->irq.parents)
+ return -ENOMEM;
+
+ ret = devm_gpiochip_add_data(pctrl->dev, chip, pctrl);
if (ret) {
dev_err(pctrl->dev, "Failed register gpiochip\n");
return ret;
@@ -1053,20 +1065,6 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
}
}
- ret = gpiochip_irqchip_add(chip,
- >irq_chip,
- 0,
- handle_edge_irq,
- IRQ_TYPE_NONE);
- if (ret) {
- dev_err(pctrl->dev, "Failed to add irqchip to gpiochip\n");
- gpiochip_remove(>chip);
- return -ENOSYS;
- }
-
- gpiochip_set_chained_irqchip(chip, >irq_chip, pctrl->irq,
-msm_gpio_irq_handler);
-
return 0;
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH RFC 14/14] arm64: defconfig: enable PDC interrupt controller for Qualcomm SDM845
Enable PDC interrupt controller for SDM845 devices. The interrupt controller can detect wakeup capable interrupts when the SoC is in a low power state. Signed-off-by: Lina Iyer --- arch/arm64/configs/defconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig index 0e58ef02880c..310b6048054a 100644 --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@ -729,6 +729,7 @@ CONFIG_ARCH_R8A77970=y CONFIG_ARCH_R8A77980=y CONFIG_ARCH_R8A77990=y CONFIG_ARCH_R8A77995=y +CONFIG_QCOM_PDC=y CONFIG_ROCKCHIP_PM_DOMAINS=y CONFIG_ARCH_TEGRA_132_SOC=y CONFIG_ARCH_TEGRA_210_SOC=y -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH RFC 03/14] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs
Introduce a new domain for wakeup capable GPIOs. The domain can be
requested using the bus token DOMAIN_BUS_WAKEUP. In the following
patches, we will specify PDC as the wakeup-parent for the TLMM GPIO
irqchip. Requesting a wakeup GPIO will setup the GPIO and the
corresponding PDC interrupt as its parent.
Co-developed-by: Stephen Boyd
Signed-off-by: Lina Iyer
---
drivers/irqchip/qcom-pdc.c | 104 ---
include/linux/soc/qcom/irq.h | 34
2 files changed, 129 insertions(+), 9 deletions(-)
create mode 100644 include/linux/soc/qcom/irq.h
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index 338fae604af5..ad1faf634bcf 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -13,12 +13,13 @@
#include
#include
#include
+#include
#include
-#include
#include
#include
#define PDC_MAX_IRQS 126
+#define PDC_MAX_GPIO_IRQS 256
#define CLEAR_INTR(reg, intr) (reg & ~(1 << intr))
#define ENABLE_INTR(reg, intr) (reg | (1 << intr))
@@ -26,6 +27,8 @@
#define IRQ_ENABLE_BANK0x10
#define IRQ_i_CFG 0x110
+#define PDC_NO_PARENT_IRQ ~0UL
+
struct pdc_pin_region {
u32 pin_base;
u32 parent_base;
@@ -65,23 +68,35 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
static void qcom_pdc_gic_disable(struct irq_data *d)
{
+ if (d->hwirq == GPIO_NO_WAKE_IRQ)
+ return;
+
pdc_enable_intr(d, false);
irq_chip_disable_parent(d);
}
static void qcom_pdc_gic_enable(struct irq_data *d)
{
+ if (d->hwirq == GPIO_NO_WAKE_IRQ)
+ return;
+
pdc_enable_intr(d, true);
irq_chip_enable_parent(d);
}
static void qcom_pdc_gic_mask(struct irq_data *d)
{
+ if (d->hwirq == GPIO_NO_WAKE_IRQ)
+ return;
+
irq_chip_mask_parent(d);
}
static void qcom_pdc_gic_unmask(struct irq_data *d)
{
+ if (d->hwirq == GPIO_NO_WAKE_IRQ)
+ return;
+
irq_chip_unmask_parent(d);
}
@@ -124,6 +139,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d,
unsigned int type)
int pin_out = d->hwirq;
enum pdc_irq_config_bits pdc_type;
+ if (pin_out == GPIO_NO_WAKE_IRQ)
+ return 0;
+
switch (type) {
case IRQ_TYPE_EDGE_RISING:
pdc_type = PDC_EDGE_RISING;
@@ -181,8 +199,7 @@ static irq_hw_number_t get_parent_hwirq(int pin)
return (region->parent_base + pin - region->pin_base);
}
- WARN_ON(1);
- return ~0UL;
+ return PDC_NO_PARENT_IRQ;
}
static int qcom_pdc_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
@@ -211,17 +228,17 @@ static int qcom_pdc_alloc(struct irq_domain *domain,
unsigned int virq,
ret = qcom_pdc_translate(domain, fwspec, , );
if (ret)
- return -EINVAL;
-
- parent_hwirq = get_parent_hwirq(hwirq);
- if (parent_hwirq == ~0UL)
- return -EINVAL;
+ return ret;
ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
_pdc_gic_chip, NULL);
if (ret)
return ret;
+ parent_hwirq = get_parent_hwirq(hwirq);
+ if (parent_hwirq == PDC_NO_PARENT_IRQ)
+ return 0;
+
if (type & IRQ_TYPE_EDGE_BOTH)
type = IRQ_TYPE_EDGE_RISING;
@@ -244,6 +261,60 @@ static const struct irq_domain_ops qcom_pdc_ops = {
.free = irq_domain_free_irqs_common,
};
+static int qcom_pdc_gpio_alloc(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs, void *data)
+{
+ struct irq_fwspec *fwspec = data;
+ struct irq_fwspec parent_fwspec;
+ irq_hw_number_t hwirq, parent_hwirq;
+ unsigned int type;
+ int ret;
+
+ ret = qcom_pdc_translate(domain, fwspec, , );
+ if (ret)
+ return ret;
+
+ ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+ _pdc_gic_chip, NULL);
+ if (ret)
+ return ret;
+
+ if (hwirq == GPIO_NO_WAKE_IRQ)
+ return 0;
+
+ parent_hwirq = get_parent_hwirq(hwirq);
+ if (parent_hwirq == PDC_NO_PARENT_IRQ)
+ return 0;
+
+ if (type & IRQ_TYPE_EDGE_BOTH)
+ type = IRQ_TYPE_EDGE_RISING;
+
+ if (type & IRQ_TYPE_LEVEL_MASK)
+ type = IRQ_TYPE_LEVEL_HIGH;
+
+ parent_fwspec.fwnode = domain->parent->fwnode;
+ parent_fwspec.param_count = 3;
+ parent_fwspec.param[0]= 0;
+ parent_fwspec.param[1]= parent_hwirq;
+ parent_fwspec.param[2]= type;
+
+ return irq_domain_alloc_irqs_parent(domain, virq, nr_irqs,
+ _fwspec);
+}
+
+stat
[PATCH RFC 11/14] drivers: pinctrl: sdm845: add PDC wakeup interrupt map for GPIOs
Add interrupt parents for wakeup capable GPIOs for Qualcomm SDM845 SoC.
Signed-off-by: Lina Iyer
---
drivers/pinctrl/qcom/pinctrl-sdm845.c | 83 ++-
1 file changed, 82 insertions(+), 1 deletion(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-sdm845.c
b/drivers/pinctrl/qcom/pinctrl-sdm845.c
index 39f498c09906..5f43dabcd8eb 100644
--- a/drivers/pinctrl/qcom/pinctrl-sdm845.c
+++ b/drivers/pinctrl/qcom/pinctrl-sdm845.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
/*
- * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2016-2019, The Linux Foundation. All rights reserved.
*/
#include
@@ -1282,6 +1282,84 @@ static const int sdm845_acpi_reserved_gpios[] = {
0, 1, 2, 3, 81, 82, 83, 84, -1
};
+static const struct msm_gpio_wakeirq_map sdm845_pdc_map[] = {
+ {1, 30},
+ {3, 31},
+ {5, 32},
+ {10, 33},
+ {11, 34},
+ {20, 35},
+ {22, 36},
+ {24, 37},
+ {26, 38},
+ {30, 39},
+ {31, 117},
+ {32, 41},
+ {34, 42},
+ {36, 43},
+ {37, 44},
+ {38, 45},
+ {39, 46},
+ {40, 47},
+ {41, 115},
+ {43, 49},
+ {44, 50},
+ {46, 51},
+ {48, 52},
+ {49, 118},
+ {52, 54},
+ {53, 55},
+ {54, 56},
+ {56, 57},
+ {57, 58},
+ {58, 59},
+ {59, 60},
+ {60, 61},
+ {61, 62},
+ {62, 63},
+ {63, 64},
+ {64, 65},
+ {66, 66},
+ {68, 67},
+ {71, 68},
+ {73, 69},
+ {77, 70},
+ {78, 71},
+ {79, 72},
+ {80, 73},
+ {84, 74},
+ {85, 75},
+ {86, 76},
+ {88, 77},
+ {89, 116},
+ {91, 79},
+ {92, 80},
+ {95, 81},
+ {96, 82},
+ {97, 83},
+ {101, 84},
+ {103, 85},
+ {104, 86},
+ {115, 90},
+ {116, 91},
+ {117, 92},
+ {118, 93},
+ {119, 94},
+ {120, 95},
+ {121, 96},
+ {122, 97},
+ {123, 98},
+ {124, 99},
+ {125, 100},
+ {127, 102},
+ {128, 103},
+ {129, 104},
+ {130, 105},
+ {132, 106},
+ {133, 107},
+ {145, 108},
+};
+
static const struct msm_pinctrl_soc_data sdm845_pinctrl = {
.pins = sdm845_pins,
.npins = ARRAY_SIZE(sdm845_pins),
@@ -1290,6 +1368,9 @@ static const struct msm_pinctrl_soc_data sdm845_pinctrl =
{
.groups = sdm845_groups,
.ngroups = ARRAY_SIZE(sdm845_groups),
.ngpios = 151,
+ .wakeirq_map = sdm845_pdc_map,
+ .nwakeirq_map = ARRAY_SIZE(sdm845_pdc_map),
+
};
static const struct msm_pinctrl_soc_data sdm845_acpi_pinctrl = {
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH RFC 12/14] arm64: dts: qcom: add PDC interrupt controller for SDM845
Add PDC interrupt controller device bindings for SDM845.
Signed-off-by: Lina Iyer
---
arch/arm64/boot/dts/qcom/sdm845.dtsi | 10 ++
1 file changed, 10 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi
b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index be0022e09465..ffe28b3e41d8 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -2375,6 +2375,16 @@
#power-domain-cells = <1>;
};
+ pdc_intc: interrupt-controller@b22 {
+ compatible = "qcom,sdm845-pdc";
+ reg = <0 0x0b22 0 0x3>, <0x179900f0 0x60>;
+ qcom,pdc-ranges = <0 480 94>, <94 609 15>, <115 630 7>;
+ #interrupt-cells = <2>;
+ interrupt-parent = <>;
+ interrupt-controller;
+ qcom,scm-spi-cfg;
+ };
+
pdc_reset: reset-controller@b2e {
compatible = "qcom,sdm845-pdc-global";
reg = <0 0x0b2e 0 0x2>;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH RFC 02/14] drivers: irqchip: pdc: Do not toggle IRQ_ENABLE during mask/unmask
When an interrupt is to be serviced, the convention is to mask the
interrupt at the chip and unmask after servicing the interrupt. Enabling
and disabling the interrupt at the PDC irqchip causes an interrupt storm
due to the way dual edge interrupts are handled in hardware.
Skip configuring the PDC when the IRQ is masked and unmasked, instead
use the irq_enable/irq_disable callbacks to toggle the IRQ_ENABLE
register at the PDC. The PDC's IRQ_ENABLE register is only used during
the monitoring mode when the system is asleep and is not needed for
active mode detection.
Signed-off-by: Lina Iyer
---
drivers/irqchip/qcom-pdc.c | 16 ++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index faa7d61b9d6c..338fae604af5 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -63,15 +63,25 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
raw_spin_unlock(_lock);
}
-static void qcom_pdc_gic_mask(struct irq_data *d)
+static void qcom_pdc_gic_disable(struct irq_data *d)
{
pdc_enable_intr(d, false);
+ irq_chip_disable_parent(d);
+}
+
+static void qcom_pdc_gic_enable(struct irq_data *d)
+{
+ pdc_enable_intr(d, true);
+ irq_chip_enable_parent(d);
+}
+
+static void qcom_pdc_gic_mask(struct irq_data *d)
+{
irq_chip_mask_parent(d);
}
static void qcom_pdc_gic_unmask(struct irq_data *d)
{
- pdc_enable_intr(d, true);
irq_chip_unmask_parent(d);
}
@@ -148,6 +158,8 @@ static struct irq_chip qcom_pdc_gic_chip = {
.irq_eoi= irq_chip_eoi_parent,
.irq_mask = qcom_pdc_gic_mask,
.irq_unmask = qcom_pdc_gic_unmask,
+ .irq_disable= qcom_pdc_gic_disable,
+ .irq_enable = qcom_pdc_gic_enable,
.irq_retrigger = irq_chip_retrigger_hierarchy,
.irq_set_type = qcom_pdc_gic_set_type,
.flags = IRQCHIP_MASK_ON_SUSPEND |
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH V2 2/4] drivers: qcom: rpmh-rsc: avoid locking in the interrupt handler
On Mon, Jul 29 2019 at 14:56 -0600, Stephen Boyd wrote: Quoting Lina Iyer (2019-07-29 12:01:39) On Thu, Jul 25 2019 at 09:44 -0600, Doug Anderson wrote: >On Thu, Jul 25, 2019 at 8:18 AM Lina Iyer wrote: >> >> On Wed, Jul 24 2019 at 17:28 -0600, Doug Anderson wrote: >> > >> >Jumping in without reading all the context, but I saw this fly by and >> >it seemed odd. If I'm way off base then please ignore... >> > >> >Can you give more details? Why are these drivers in atomic contexts? >> >If they are in atomic contexts because they are running in the context >> >of an interrupt then your next patch in the series isn't so correct. >> > >> >Also: when people submit requests in atomic context are they always >> >submitting an asynchronous request? In that case we could >> >(presumably) just use a spinlock to protect the queue of async >> >requests and a mutex for everything else? >> Yes, drivers only make async requests in interrupt contexts. > >So correct me if I'm off base, but you're saying that drivers make >requests in interrupt contexts even after your whole series and that's >why you're using spinlocks instead of mutexes. ...but then in patch >#3 in your series you say: > >> Switch over from using _irqsave/_irqrestore variants since we no longer >> race with a lock from the interrupt handler. > >Those seem like contradictions. What happens if someone is holding >the lock, then an interrupt fires, then the interrupt routine wants to >do an async request. Boom, right? > The interrupt routine is handled by the driver and only completes the waiting object (for sync requests). No other requests can be made from our interrupt handler. The question is more if an interrupt handler for some consumer driver can call into this code and make an async request. Is that possible? If so, the concern is that the driver's interrupt handler can run and try to grab the lock on a CPU that already holds the lock in a non-irq disabled context. This would lead to a deadlock while the CPU servicing the interrupt waits for the lock held by another task that's been interrupted. Hmm.. this patch will cause that issue, since we remove the irqsave aspects of the locking. Let me give that a thought. >> They cannot >> use the sync variants. The async and sync variants are streamlined into >> the same code path. Hence the use of spinlocks instead of mutexes >> through the critical path. > >I will perhaps defer to Stephen who was the one thinking that a mutex >would be a big win here. ...but if a mutex truly is a big win then it >doesn't seem like it'd be that hard to have a linked list (protected >by a spinlock) and then some type of async worker that: > >1. Grab the spinlock, pops one element off the linked list, release the spinlock >2. Grab the mutex, send the one element, release the mutex This would be a problem when the request is made from an irq handler. We want to keep things simple and quick. Is the problem that you want to use RPMh code from deep within the idle thread? As part of some sort of CPU idle driver for qcom platforms? The way this discussion is going it sounds like nothing is standing in the way of a design that use a kthread to pump messages off a queue of messages that is protected by a spinlock. The kthread would be woken up by the sync or async write to continue to pump messages out until the queue is empty. While it is true that we want to use RPMH in cpuidle driver. Its just that we had threads and all in our downstream 845 and it complicated the whole setup a bit too much to our liking and did not help debug either. I would rather not get all that back in the driver. --Lina
Re: [PATCH V2 2/4] drivers: qcom: rpmh-rsc: avoid locking in the interrupt handler
On Thu, Jul 25 2019 at 09:44 -0600, Doug Anderson wrote: Hi, On Thu, Jul 25, 2019 at 8:18 AM Lina Iyer wrote: On Wed, Jul 24 2019 at 17:28 -0600, Doug Anderson wrote: >Hi, > >On Wed, Jul 24, 2019 at 1:36 PM Lina Iyer wrote: >> >> On Wed, Jul 24 2019 at 13:38 -0600, Stephen Boyd wrote: >> >Quoting Lina Iyer (2019-07-24 07:52:51) >> >> On Tue, Jul 23 2019 at 14:11 -0600, Stephen Boyd wrote: >> >> >Quoting Lina Iyer (2019-07-22 14:53:38) >> >> >> Avoid locking in the interrupt context to improve latency. Since we >> >> >> don't lock in the interrupt context, it is possible that we now could >> >> >> race with the DRV_CONTROL register that writes the enable register and >> >> >> cleared by the interrupt handler. For fire-n-forget requests, the >> >> >> interrupt may be raised as soon as the TCS is triggered and the IRQ >> >> >> handler may clear the enable bit before the DRV_CONTROL is read back. >> >> >> >> >> >> Use the non-sync variant when enabling the TCS register to avoid reading >> >> >> back a value that may been cleared because the interrupt handler ran >> >> >> immediately after triggering the TCS. >> >> >> >> >> >> Signed-off-by: Lina Iyer >> >> >> --- >> >> > >> >> >I have to read this patch carefully. The commit text isn't convincing me >> >> >that it is actually safe to make this change. It mostly talks about the >> >> >performance improvements and how we need to fix __tcs_trigger(), which >> >> >is good, but I was hoping to be convinced that not grabbing the lock >> >> >here is safe. >> >> > >> >> >How do we ensure that drv->tcs_in_use is cleared before we call >> >> >tcs_write() and try to look for a free bit? Isn't it possible that we'll >> >> >get into a situation where the bitmap is all used up but the hardware >> >> >has just received an interrupt and is going to clear out a bit and then >> >> >an rpmh write fails with -EBUSY? >> >> > >> >> If we have a situation where there are no available free bits, we retry >> >> and that is part of the function. Since we have only 2 TCSes avaialble >> >> to write to the hardware and there could be multiple requests coming in, >> >> it is a very common situation. We try and acquire the drv->lock and if >> >> there are free TCS available and if available mark them busy and send >> >> our requests. If there are none available, we keep retrying. >> >> >> > >> >Ok. I wonder if we need some sort of barriers here too, like an >> >smp_mb__after_atomic()? That way we can make sure that the write to >> >clear the bit is seen by another CPU that could be spinning forever >> >waiting for that bit to be cleared? Before this change the spinlock >> >would be guaranteed to make these barriers for us, but now that doesn't >> >seem to be the case. I really hope that this whole thing can be changed >> >to be a mutex though, in which case we can use the bit_wait() API, etc. >> >to put tasks to sleep while RPMh is processing things. >> > >> We have drivers that want to send requests in atomic contexts and >> therefore mutex locks would not work. > >Jumping in without reading all the context, but I saw this fly by and >it seemed odd. If I'm way off base then please ignore... > >Can you give more details? Why are these drivers in atomic contexts? >If they are in atomic contexts because they are running in the context >of an interrupt then your next patch in the series isn't so correct. > >Also: when people submit requests in atomic context are they always >submitting an asynchronous request? In that case we could >(presumably) just use a spinlock to protect the queue of async >requests and a mutex for everything else? Yes, drivers only make async requests in interrupt contexts. So correct me if I'm off base, but you're saying that drivers make requests in interrupt contexts even after your whole series and that's why you're using spinlocks instead of mutexes. ...but then in patch #3 in your series you say: Switch over from using _irqsave/_irqrestore variants since we no longer race with a lock from the interrupt handler. Those seem like contradictions. What happens if someone is holding the lock, then an interrupt fires, then the interrupt routine wants to do an async request. Boom, right? The interrupt routine is handled by the driver and only comple
Re: [PATCH V2 2/4] drivers: qcom: rpmh-rsc: avoid locking in the interrupt handler
On Wed, Jul 24 2019 at 17:28 -0600, Doug Anderson wrote: Hi, On Wed, Jul 24, 2019 at 1:36 PM Lina Iyer wrote: On Wed, Jul 24 2019 at 13:38 -0600, Stephen Boyd wrote: >Quoting Lina Iyer (2019-07-24 07:52:51) >> On Tue, Jul 23 2019 at 14:11 -0600, Stephen Boyd wrote: >> >Quoting Lina Iyer (2019-07-22 14:53:38) >> >> Avoid locking in the interrupt context to improve latency. Since we >> >> don't lock in the interrupt context, it is possible that we now could >> >> race with the DRV_CONTROL register that writes the enable register and >> >> cleared by the interrupt handler. For fire-n-forget requests, the >> >> interrupt may be raised as soon as the TCS is triggered and the IRQ >> >> handler may clear the enable bit before the DRV_CONTROL is read back. >> >> >> >> Use the non-sync variant when enabling the TCS register to avoid reading >> >> back a value that may been cleared because the interrupt handler ran >> >> immediately after triggering the TCS. >> >> >> >> Signed-off-by: Lina Iyer >> >> --- >> > >> >I have to read this patch carefully. The commit text isn't convincing me >> >that it is actually safe to make this change. It mostly talks about the >> >performance improvements and how we need to fix __tcs_trigger(), which >> >is good, but I was hoping to be convinced that not grabbing the lock >> >here is safe. >> > >> >How do we ensure that drv->tcs_in_use is cleared before we call >> >tcs_write() and try to look for a free bit? Isn't it possible that we'll >> >get into a situation where the bitmap is all used up but the hardware >> >has just received an interrupt and is going to clear out a bit and then >> >an rpmh write fails with -EBUSY? >> > >> If we have a situation where there are no available free bits, we retry >> and that is part of the function. Since we have only 2 TCSes avaialble >> to write to the hardware and there could be multiple requests coming in, >> it is a very common situation. We try and acquire the drv->lock and if >> there are free TCS available and if available mark them busy and send >> our requests. If there are none available, we keep retrying. >> > >Ok. I wonder if we need some sort of barriers here too, like an >smp_mb__after_atomic()? That way we can make sure that the write to >clear the bit is seen by another CPU that could be spinning forever >waiting for that bit to be cleared? Before this change the spinlock >would be guaranteed to make these barriers for us, but now that doesn't >seem to be the case. I really hope that this whole thing can be changed >to be a mutex though, in which case we can use the bit_wait() API, etc. >to put tasks to sleep while RPMh is processing things. > We have drivers that want to send requests in atomic contexts and therefore mutex locks would not work. Jumping in without reading all the context, but I saw this fly by and it seemed odd. If I'm way off base then please ignore... Can you give more details? Why are these drivers in atomic contexts? If they are in atomic contexts because they are running in the context of an interrupt then your next patch in the series isn't so correct. Also: when people submit requests in atomic context are they always submitting an asynchronous request? In that case we could (presumably) just use a spinlock to protect the queue of async requests and a mutex for everything else? Yes, drivers only make async requests in interrupt contexts. They cannot use the sync variants. The async and sync variants are streamlined into the same code path. Hence the use of spinlocks instead of mutexes through the critical path. --Lina
Re: [PATCH V2 2/4] drivers: qcom: rpmh-rsc: avoid locking in the interrupt handler
On Wed, Jul 24 2019 at 13:38 -0600, Stephen Boyd wrote: Quoting Lina Iyer (2019-07-24 07:52:51) On Tue, Jul 23 2019 at 14:11 -0600, Stephen Boyd wrote: >Quoting Lina Iyer (2019-07-22 14:53:38) >> Avoid locking in the interrupt context to improve latency. Since we >> don't lock in the interrupt context, it is possible that we now could >> race with the DRV_CONTROL register that writes the enable register and >> cleared by the interrupt handler. For fire-n-forget requests, the >> interrupt may be raised as soon as the TCS is triggered and the IRQ >> handler may clear the enable bit before the DRV_CONTROL is read back. >> >> Use the non-sync variant when enabling the TCS register to avoid reading >> back a value that may been cleared because the interrupt handler ran >> immediately after triggering the TCS. >> >> Signed-off-by: Lina Iyer >> --- > >I have to read this patch carefully. The commit text isn't convincing me >that it is actually safe to make this change. It mostly talks about the >performance improvements and how we need to fix __tcs_trigger(), which >is good, but I was hoping to be convinced that not grabbing the lock >here is safe. > >How do we ensure that drv->tcs_in_use is cleared before we call >tcs_write() and try to look for a free bit? Isn't it possible that we'll >get into a situation where the bitmap is all used up but the hardware >has just received an interrupt and is going to clear out a bit and then >an rpmh write fails with -EBUSY? > If we have a situation where there are no available free bits, we retry and that is part of the function. Since we have only 2 TCSes avaialble to write to the hardware and there could be multiple requests coming in, it is a very common situation. We try and acquire the drv->lock and if there are free TCS available and if available mark them busy and send our requests. If there are none available, we keep retrying. Ok. I wonder if we need some sort of barriers here too, like an smp_mb__after_atomic()? That way we can make sure that the write to clear the bit is seen by another CPU that could be spinning forever waiting for that bit to be cleared? Before this change the spinlock would be guaranteed to make these barriers for us, but now that doesn't seem to be the case. I really hope that this whole thing can be changed to be a mutex though, in which case we can use the bit_wait() API, etc. to put tasks to sleep while RPMh is processing things. We have drivers that want to send requests in atomic contexts and therefore mutex locks would not work. --Lina
Re: [PATCH V2 1/4] drivers: qcom: rpmh-rsc: simplify TCS locking
On Wed, Jul 24 2019 at 12:32 -0600, Stephen Boyd wrote: Quoting Lina Iyer (2019-07-24 07:54:52) On Tue, Jul 23 2019 at 14:19 -0600, Stephen Boyd wrote: >Quoting Lina Iyer (2019-07-23 12:21:59) >> On Tue, Jul 23 2019 at 12:22 -0600, Stephen Boyd wrote: >> >Can you keep irq saving and restoring in this patch and then remove that >> >in the next patch with reasoning? It probably isn't safe if the lock is >> >taken in interrupt context anyway. >> > >> Yes, the drv->lock should have been irqsave/irqrestore, but it hasn't >> been changed by this patch. > >It needs to be changed to maintain the irqsaving/restoring of the code. > May be I should club this with the following patch. Instead of adding irqsave and restore to drv->lock and then remvoing them again in the following patch. I suspect that gets us back to v1 of this patch series? I'd prefer you just keep the save/restore of irqs in this patch and then remove them later. Or if the order can be the other way, where we remove grabbing the lock in irq context comes first and then consolidate the locks into one it might work. Patches 1 and 3 need not be bundled. We can keep them separate to help understand the change better. This patch order - #2, #1, #3, #4 would work. --Lina
Re: [PATCH V2 1/4] drivers: qcom: rpmh-rsc: simplify TCS locking
On Tue, Jul 23 2019 at 14:19 -0600, Stephen Boyd wrote:
Quoting Lina Iyer (2019-07-23 12:21:59)
On Tue, Jul 23 2019 at 12:22 -0600, Stephen Boyd wrote:
>Quoting Lina Iyer (2019-07-22 14:53:37)
>> From: "Raju P.L.S.S.S.N"
>>
>> The tcs->lock was introduced to serialize access with in TCS group. But,
>> drv->lock is still needed to synchronize core aspects of the
>> communication. This puts the drv->lock in the critical and high latency
>> path of sending a request. drv->lock provides the all necessary
>> synchronization. So remove locking around TCS group and simply use the
>> drv->lock instead.
>
>This doesn't talk about removing the irq saving and restoring though.
You mean for drv->lock? It was not an _irqsave/_irqrestore anyways and
we were only removing the tcs->lock.
Yes drv->lock wasn't an irqsave/restore variant because it was a
spinlock inside of an obviously already irqsaved region of code because
the tcs->lock was outside the drv->lock and that was saving the irq
flags.
Oh, right.
>Can you keep irq saving and restoring in this patch and then remove that
>in the next patch with reasoning? It probably isn't safe if the lock is
>taken in interrupt context anyway.
>
Yes, the drv->lock should have been irqsave/irqrestore, but it hasn't
been changed by this patch.
It needs to be changed to maintain the irqsaving/restoring of the code.
May be I should club this with the following patch. Instead of adding
irqsave and restore to drv->lock and then remvoing them again in the
following patch.
>> @@ -349,41 +349,35 @@ static int tcs_write(struct rsc_drv *drv, const struct
tcs_request *msg)
>> {
>> struct tcs_group *tcs;
>> int tcs_id;
>> - unsigned long flags;
>> int ret;
>>
>> tcs = get_tcs_for_msg(drv, msg);
>> if (IS_ERR(tcs))
>> return PTR_ERR(tcs);
>>
>> - spin_lock_irqsave(>lock, flags);
>> spin_lock(>lock);
>> /*
>> * The h/w does not like if we send a request to the same address,
>> * when one is already in-flight or being processed.
>> */
>> ret = check_for_req_inflight(drv, tcs, msg);
>> - if (ret) {
>> - spin_unlock(>lock);
>> + if (ret)
>> goto done_write;
>> - }
>>
>> tcs_id = find_free_tcs(tcs);
>> if (tcs_id < 0) {
>> ret = tcs_id;
>> - spin_unlock(>lock);
>> goto done_write;
>> }
>>
>> tcs->req[tcs_id - tcs->offset] = msg;
>> set_bit(tcs_id, drv->tcs_in_use);
>> - spin_unlock(>lock);
>>
>> __tcs_buffer_write(drv, tcs_id, 0, msg);
>> __tcs_trigger(drv, tcs_id);
>>
>> done_write:
>> - spin_unlock_irqrestore(>lock, flags);
>> + spin_unlock(>lock);
>> return ret;
>> }
>>
>> @@ -481,19 +475,18 @@ static int tcs_ctrl_write(struct rsc_drv *drv, const
struct tcs_request *msg)
>> {
>> struct tcs_group *tcs;
>> int tcs_id = 0, cmd_id = 0;
>> - unsigned long flags;
>> int ret;
>>
>> tcs = get_tcs_for_msg(drv, msg);
>> if (IS_ERR(tcs))
>> return PTR_ERR(tcs);
>>
>> - spin_lock_irqsave(>lock, flags);
>> + spin_lock(>lock);
>> /* find the TCS id and the command in the TCS to write to */
>> ret = find_slots(tcs, msg, _id, _id);
>> if (!ret)
>> __tcs_buffer_write(drv, tcs_id, cmd_id, msg);
>> - spin_unlock_irqrestore(>lock, flags);
>> + spin_unlock(>lock);
>>
>
>These ones, just leave them doing the irq save restore for now?
>
drv->lock ??
Yes, it should have irq save/restore still.
Re: [PATCH V2 2/4] drivers: qcom: rpmh-rsc: avoid locking in the interrupt handler
On Tue, Jul 23 2019 at 14:11 -0600, Stephen Boyd wrote: Quoting Lina Iyer (2019-07-22 14:53:38) Avoid locking in the interrupt context to improve latency. Since we don't lock in the interrupt context, it is possible that we now could race with the DRV_CONTROL register that writes the enable register and cleared by the interrupt handler. For fire-n-forget requests, the interrupt may be raised as soon as the TCS is triggered and the IRQ handler may clear the enable bit before the DRV_CONTROL is read back. Use the non-sync variant when enabling the TCS register to avoid reading back a value that may been cleared because the interrupt handler ran immediately after triggering the TCS. Signed-off-by: Lina Iyer --- I have to read this patch carefully. The commit text isn't convincing me that it is actually safe to make this change. It mostly talks about the performance improvements and how we need to fix __tcs_trigger(), which is good, but I was hoping to be convinced that not grabbing the lock here is safe. How do we ensure that drv->tcs_in_use is cleared before we call tcs_write() and try to look for a free bit? Isn't it possible that we'll get into a situation where the bitmap is all used up but the hardware has just received an interrupt and is going to clear out a bit and then an rpmh write fails with -EBUSY? If we have a situation where there are no available free bits, we retry and that is part of the function. Since we have only 2 TCSes avaialble to write to the hardware and there could be multiple requests coming in, it is a very common situation. We try and acquire the drv->lock and if there are free TCS available and if available mark them busy and send our requests. If there are none available, we keep retrying. drivers/soc/qcom/rpmh-rsc.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c index 5ede8d6de3ad..694ba881624e 100644 --- a/drivers/soc/qcom/rpmh-rsc.c +++ b/drivers/soc/qcom/rpmh-rsc.c @@ -242,9 +242,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p) write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, i, 0); write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, i, 0); write_tcs_reg(drv, RSC_DRV_IRQ_CLEAR, 0, BIT(i)); - spin_lock(>lock); clear_bit(i, drv->tcs_in_use); - spin_unlock(>lock); if (req) rpmh_tx_done(req, err); } @@ -304,7 +302,7 @@ static void __tcs_trigger(struct rsc_drv *drv, int tcs_id) enable = TCS_AMC_MODE_ENABLE; write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable); enable |= TCS_AMC_MODE_TRIGGER; - write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable); + write_tcs_reg(drv, RSC_DRV_CONTROL, tcs_id, enable); } static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
Re: [PATCH V2 1/4] drivers: qcom: rpmh-rsc: simplify TCS locking
On Tue, Jul 23 2019 at 12:22 -0600, Stephen Boyd wrote:
Quoting Lina Iyer (2019-07-22 14:53:37)
From: "Raju P.L.S.S.S.N"
The tcs->lock was introduced to serialize access with in TCS group. But,
drv->lock is still needed to synchronize core aspects of the
communication. This puts the drv->lock in the critical and high latency
path of sending a request. drv->lock provides the all necessary
synchronization. So remove locking around TCS group and simply use the
drv->lock instead.
This doesn't talk about removing the irq saving and restoring though.
You mean for drv->lock? It was not an _irqsave/_irqrestore anyways and
we were only removing the tcs->lock.
Can you keep irq saving and restoring in this patch and then remove that
in the next patch with reasoning? It probably isn't safe if the lock is
taken in interrupt context anyway.
Yes, the drv->lock should have been irqsave/irqrestore, but it hasn't
been changed by this patch.
Signed-off-by: Raju P.L.S.S.S.N
[ilina: split patch into multiple files, update commit text]
Signed-off-by: Lina Iyer
diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index a767991c..969d5030860e 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index e278fc11fe5c..5ede8d6de3ad 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -106,26 +106,26 @@ static int tcs_invalidate(struct rsc_drv *drv, int type)
{
int m;
struct tcs_group *tcs;
+ int ret = 0;
tcs = get_tcs_of_type(drv, type);
- spin_lock(>lock);
- if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
- spin_unlock(>lock);
- return 0;
- }
+ spin_lock(>lock);
+ if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS))
+ goto done_invalidate;
for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
if (!tcs_is_free(drv, m)) {
- spin_unlock(>lock);
- return -EAGAIN;
+ ret = -EAGAIN;
+ goto done_invalidate;
}
write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0);
write_tcs_reg_sync(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0);
}
bitmap_zero(tcs->slots, MAX_TCS_SLOTS);
- spin_unlock(>lock);
+done_invalidate:
+ spin_unlock(>lock);
return 0;
return ret now?
Yes, will do.
}
@@ -349,41 +349,35 @@ static int tcs_write(struct rsc_drv *drv, const struct
tcs_request *msg)
{
struct tcs_group *tcs;
int tcs_id;
- unsigned long flags;
int ret;
tcs = get_tcs_for_msg(drv, msg);
if (IS_ERR(tcs))
return PTR_ERR(tcs);
- spin_lock_irqsave(>lock, flags);
spin_lock(>lock);
/*
* The h/w does not like if we send a request to the same address,
* when one is already in-flight or being processed.
*/
ret = check_for_req_inflight(drv, tcs, msg);
- if (ret) {
- spin_unlock(>lock);
+ if (ret)
goto done_write;
- }
tcs_id = find_free_tcs(tcs);
if (tcs_id < 0) {
ret = tcs_id;
- spin_unlock(>lock);
goto done_write;
}
tcs->req[tcs_id - tcs->offset] = msg;
set_bit(tcs_id, drv->tcs_in_use);
- spin_unlock(>lock);
__tcs_buffer_write(drv, tcs_id, 0, msg);
__tcs_trigger(drv, tcs_id);
done_write:
- spin_unlock_irqrestore(>lock, flags);
+ spin_unlock(>lock);
return ret;
}
@@ -481,19 +475,18 @@ static int tcs_ctrl_write(struct rsc_drv *drv, const
struct tcs_request *msg)
{
struct tcs_group *tcs;
int tcs_id = 0, cmd_id = 0;
- unsigned long flags;
int ret;
tcs = get_tcs_for_msg(drv, msg);
if (IS_ERR(tcs))
return PTR_ERR(tcs);
- spin_lock_irqsave(>lock, flags);
+ spin_lock(>lock);
/* find the TCS id and the command in the TCS to write to */
ret = find_slots(tcs, msg, _id, _id);
if (!ret)
__tcs_buffer_write(drv, tcs_id, cmd_id, msg);
- spin_unlock_irqrestore(>lock, flags);
+ spin_unlock(>lock);
These ones, just leave them doing the irq save restore for now?
drv->lock ??
--Lina
[PATCH V2 3/4] drivers: qcom: rpmh: switch over from spinlock irq variants
From: "Raju P.L.S.S.S.N"
Switch over from using _irqsave/_irqrestore variants since we no longer
race with a lock from the interrupt handler. While we are at it, rename
the cache_lock to just lock to allow use of the lock to synchronize
controller access.
Signed-off-by: Raju P.L.S.S.S.N
Signed-off-by: Lina Iyer
---
drivers/soc/qcom/rpmh-internal.h | 4 ++--
drivers/soc/qcom/rpmh-rsc.c | 2 +-
drivers/soc/qcom/rpmh.c | 21 -
3 files changed, 11 insertions(+), 16 deletions(-)
diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index 969d5030860e..93d59db435bb 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -67,13 +67,13 @@ struct rpmh_request {
* struct rpmh_ctrlr: our representation of the controller
*
* @cache: the list of cached requests
- * @cache_lock: synchronize access to the cache data
+ * @lock: synchronize access to the controller data
* @dirty: was the cache updated since flush
* @batch_cache: Cache sleep and wake requests sent as batch
*/
struct rpmh_ctrlr {
struct list_head cache;
- spinlock_t cache_lock;
+ spinlock_t lock;
bool dirty;
struct list_head batch_cache;
};
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 694ba881624e..add5e84751c9 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -656,7 +656,7 @@ static int rpmh_rsc_probe(struct platform_device *pdev)
/* Enable the active TCS to send requests immediately */
write_tcs_reg(drv, RSC_DRV_IRQ_ENABLE, 0, drv->tcs[ACTIVE_TCS].mask);
- spin_lock_init(>client.cache_lock);
+ spin_lock_init(>client.lock);
INIT_LIST_HEAD(>client.cache);
INIT_LIST_HEAD(>client.batch_cache);
diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index 035091fd44b8..d6fb254a4b57 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c
@@ -118,9 +118,8 @@ static struct cache_req *cache_rpm_request(struct
rpmh_ctrlr *ctrlr,
struct tcs_cmd *cmd)
{
struct cache_req *req;
- unsigned long flags;
- spin_lock_irqsave(>cache_lock, flags);
+ spin_lock(>lock);
req = __find_req(ctrlr, cmd->addr);
if (req)
goto existing;
@@ -154,7 +153,7 @@ static struct cache_req *cache_rpm_request(struct
rpmh_ctrlr *ctrlr,
ctrlr->dirty = true;
unlock:
- spin_unlock_irqrestore(>cache_lock, flags);
+ spin_unlock(>lock);
return req;
}
@@ -283,23 +282,20 @@ EXPORT_SYMBOL(rpmh_write);
static void cache_batch(struct rpmh_ctrlr *ctrlr, struct batch_cache_req *req)
{
- unsigned long flags;
-
- spin_lock_irqsave(>cache_lock, flags);
+ spin_lock(>lock);
list_add_tail(>list, >batch_cache);
- spin_unlock_irqrestore(>cache_lock, flags);
+ spin_unlock(>lock);
}
static int flush_batch(struct rpmh_ctrlr *ctrlr)
{
struct batch_cache_req *req;
const struct rpmh_request *rpm_msg;
- unsigned long flags;
int ret = 0;
int i;
/* Send Sleep/Wake requests to the controller, expect no response */
- spin_lock_irqsave(>cache_lock, flags);
+ spin_lock(>lock);
list_for_each_entry(req, >batch_cache, list) {
for (i = 0; i < req->count; i++) {
rpm_msg = req->rpm_msgs + i;
@@ -309,7 +305,7 @@ static int flush_batch(struct rpmh_ctrlr *ctrlr)
break;
}
}
- spin_unlock_irqrestore(>cache_lock, flags);
+ spin_unlock(>lock);
return ret;
}
@@ -317,13 +313,12 @@ static int flush_batch(struct rpmh_ctrlr *ctrlr)
static void invalidate_batch(struct rpmh_ctrlr *ctrlr)
{
struct batch_cache_req *req, *tmp;
- unsigned long flags;
- spin_lock_irqsave(>cache_lock, flags);
+ spin_lock(>lock);
list_for_each_entry_safe(req, tmp, >batch_cache, list)
kfree(req);
INIT_LIST_HEAD(>batch_cache);
- spin_unlock_irqrestore(>cache_lock, flags);
+ spin_unlock(>lock);
}
/**
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH V2 1/4] drivers: qcom: rpmh-rsc: simplify TCS locking
From: "Raju P.L.S.S.S.N"
The tcs->lock was introduced to serialize access with in TCS group. But,
drv->lock is still needed to synchronize core aspects of the
communication. This puts the drv->lock in the critical and high latency
path of sending a request. drv->lock provides the all necessary
synchronization. So remove locking around TCS group and simply use the
drv->lock instead.
Signed-off-by: Raju P.L.S.S.S.N
[ilina: split patch into multiple files, update commit text]
Signed-off-by: Lina Iyer
---
Changes in v2:
- Split the patches into multiple
- Optimzation to remove reundant TCS access
- Split the rpmh library changes into its own patch
- Remove locks in IRQ handler
- Update commit text
- Remove fixes in commit text
---
drivers/soc/qcom/rpmh-internal.h | 2 --
drivers/soc/qcom/rpmh-rsc.c | 32
2 files changed, 12 insertions(+), 22 deletions(-)
diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index a767991c..969d5030860e 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -28,7 +28,6 @@ struct rsc_drv;
* @offset:start of the TCS group relative to the TCSes in the RSC
* @num_tcs: number of TCSes in this type
* @ncpt: number of commands in each TCS
- * @lock: lock for synchronizing this TCS writes
* @req: requests that are sent from the TCS
* @cmd_cache: flattened cache of cmds in sleep/wake TCS
* @slots: indicates which of @cmd_addr are occupied
@@ -40,7 +39,6 @@ struct tcs_group {
u32 offset;
int num_tcs;
int ncpt;
- spinlock_t lock;
const struct tcs_request *req[MAX_TCS_PER_TYPE];
u32 *cmd_cache;
DECLARE_BITMAP(slots, MAX_TCS_SLOTS);
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index e278fc11fe5c..5ede8d6de3ad 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -106,26 +106,26 @@ static int tcs_invalidate(struct rsc_drv *drv, int type)
{
int m;
struct tcs_group *tcs;
+ int ret = 0;
tcs = get_tcs_of_type(drv, type);
- spin_lock(>lock);
- if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
- spin_unlock(>lock);
- return 0;
- }
+ spin_lock(>lock);
+ if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS))
+ goto done_invalidate;
for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
if (!tcs_is_free(drv, m)) {
- spin_unlock(>lock);
- return -EAGAIN;
+ ret = -EAGAIN;
+ goto done_invalidate;
}
write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0);
write_tcs_reg_sync(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0);
}
bitmap_zero(tcs->slots, MAX_TCS_SLOTS);
- spin_unlock(>lock);
+done_invalidate:
+ spin_unlock(>lock);
return 0;
}
@@ -349,41 +349,35 @@ static int tcs_write(struct rsc_drv *drv, const struct
tcs_request *msg)
{
struct tcs_group *tcs;
int tcs_id;
- unsigned long flags;
int ret;
tcs = get_tcs_for_msg(drv, msg);
if (IS_ERR(tcs))
return PTR_ERR(tcs);
- spin_lock_irqsave(>lock, flags);
spin_lock(>lock);
/*
* The h/w does not like if we send a request to the same address,
* when one is already in-flight or being processed.
*/
ret = check_for_req_inflight(drv, tcs, msg);
- if (ret) {
- spin_unlock(>lock);
+ if (ret)
goto done_write;
- }
tcs_id = find_free_tcs(tcs);
if (tcs_id < 0) {
ret = tcs_id;
- spin_unlock(>lock);
goto done_write;
}
tcs->req[tcs_id - tcs->offset] = msg;
set_bit(tcs_id, drv->tcs_in_use);
- spin_unlock(>lock);
__tcs_buffer_write(drv, tcs_id, 0, msg);
__tcs_trigger(drv, tcs_id);
done_write:
- spin_unlock_irqrestore(>lock, flags);
+ spin_unlock(>lock);
return ret;
}
@@ -481,19 +475,18 @@ static int tcs_ctrl_write(struct rsc_drv *drv, const
struct tcs_request *msg)
{
struct tcs_group *tcs;
int tcs_id = 0, cmd_id = 0;
- unsigned long flags;
int ret;
tcs = get_tcs_for_msg(drv, msg);
if (IS_ERR(tcs))
return PTR_ERR(tcs);
- spin_lock_irqsave(>lock, flags);
+ spin_lock(>lock);
/* find the TCS id and the command in the TCS to write to */
ret = find_slots(tcs, msg, _id, _id);
if (!ret)
__tcs_buffer_write(drv, tcs_id, cmd_id, msg);
- spin_unlock_irqrestore(>lock, flags);
+
[PATCH V2 4/4] drivers: qcom: rpmh-rsc: remove redundant register access
Since drv->tcs_in_use is updated when the DRV_STATUS is updated, we
could simply use the former to determine if the TCS is idle or not.
Therefore, remove redundant TCS register read.
Signed-off-by: Lina Iyer
---
drivers/soc/qcom/rpmh-rsc.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index add5e84751c9..b04cd2d2910c 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -93,8 +93,7 @@ static void write_tcs_reg_sync(struct rsc_drv *drv, int reg,
int tcs_id,
static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
{
- return !test_bit(tcs_id, drv->tcs_in_use) &&
- read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id, 0);
+ return !test_bit(tcs_id, drv->tcs_in_use);
}
static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type)
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH V2 2/4] drivers: qcom: rpmh-rsc: avoid locking in the interrupt handler
Avoid locking in the interrupt context to improve latency. Since we don't lock in the interrupt context, it is possible that we now could race with the DRV_CONTROL register that writes the enable register and cleared by the interrupt handler. For fire-n-forget requests, the interrupt may be raised as soon as the TCS is triggered and the IRQ handler may clear the enable bit before the DRV_CONTROL is read back. Use the non-sync variant when enabling the TCS register to avoid reading back a value that may been cleared because the interrupt handler ran immediately after triggering the TCS. Signed-off-by: Lina Iyer --- drivers/soc/qcom/rpmh-rsc.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c index 5ede8d6de3ad..694ba881624e 100644 --- a/drivers/soc/qcom/rpmh-rsc.c +++ b/drivers/soc/qcom/rpmh-rsc.c @@ -242,9 +242,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p) write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, i, 0); write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, i, 0); write_tcs_reg(drv, RSC_DRV_IRQ_CLEAR, 0, BIT(i)); - spin_lock(>lock); clear_bit(i, drv->tcs_in_use); - spin_unlock(>lock); if (req) rpmh_tx_done(req, err); } @@ -304,7 +302,7 @@ static void __tcs_trigger(struct rsc_drv *drv, int tcs_id) enable = TCS_AMC_MODE_ENABLE; write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable); enable |= TCS_AMC_MODE_TRIGGER; - write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable); + write_tcs_reg(drv, RSC_DRV_CONTROL, tcs_id, enable); } static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs, -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH 1/2] drivers: qcom: rpmh-rsc: simplify TCS locking
On Mon, Jul 22 2019 at 12:18 -0600, Stephen Boyd wrote: Quoting Lina Iyer (2019-07-22 09:20:03) On Fri, Jul 19 2019 at 12:20 -0600, Stephen Boyd wrote: >Quoting Lina Iyer (2019-07-01 08:29:06) >> From: "Raju P.L.S.S.S.N" >> >> tcs->lock was introduced to serialize access with in TCS group. But >> even without tcs->lock, drv->lock is serving the same purpose. So >> use a single drv->lock. > >Isn't the downside now that we're going to be serializing access to the >different TCSes when two are being written in parallel or waited on? I >thought that was the whole point of splitting the lock into a TCS lock >and a general "driver" lock that protects the global driver state vs. >the specific TCS state. > Yes but we were holding the drv->lock as well as tcs->lock for the most critical of the path anyways (writing to TCS). The added complexity doesn't seem to help reduce the latency that it expected to reduce. Ok. That sort of information should be in the commit text to explain why it's not helping with reducing the latency or throughput of the API. Will add. --Lina
Re: [PATCH 1/2] drivers: qcom: rpmh-rsc: simplify TCS locking
On Fri, Jul 19 2019 at 12:20 -0600, Stephen Boyd wrote:
Quoting Lina Iyer (2019-07-01 08:29:06)
From: "Raju P.L.S.S.S.N"
tcs->lock was introduced to serialize access with in TCS group. But
even without tcs->lock, drv->lock is serving the same purpose. So
use a single drv->lock.
Isn't the downside now that we're going to be serializing access to the
different TCSes when two are being written in parallel or waited on? I
thought that was the whole point of splitting the lock into a TCS lock
and a general "driver" lock that protects the global driver state vs.
the specific TCS state.
Yes but we were holding the drv->lock as well as tcs->lock for the most
critical of the path anyways (writing to TCS). The added complexity
doesn't seem to help reduce the latency that it expected to reduce.
Other optimizations include -
- Remove locking around clear_bit() in IRQ handler. clear_bit() is
atomic.
- Remove redundant read of TCS registers.
- Use spin_lock instead of _irq variants as the locks are not held
in interrupt context.
Can you please split this patch up into 3 or 4 different patches? I'm
not sure why any of these patches are marked with Fixes either. It's an
optimization patch, not a fix patch, unless the optimization is really
large somehow?
Okay. I will try that.
Fixes: 658628 ("drivers: qcom: rpmh-rsc: add RPMH controller for QCOM
SoCs")
Signed-off-by: Raju P.L.S.S.S.N
Signed-off-by: Lina Iyer
---
drivers/soc/qcom/rpmh-internal.h | 2 --
drivers/soc/qcom/rpmh-rsc.c | 37 +++-
drivers/soc/qcom/rpmh.c | 20 +++--
3 files changed, 21 insertions(+), 38 deletions(-)
diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index a767991c..969d5030860e 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index e278fc11fe5c..92461311aef3 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -93,8 +93,7 @@ static void write_tcs_reg_sync(struct rsc_drv *drv, int reg,
int tcs_id,
static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
{
- return !test_bit(tcs_id, drv->tcs_in_use) &&
- read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id, 0);
+ return !test_bit(tcs_id, drv->tcs_in_use);
This can be a diffedjusted rent patch. Why is reading the tcs register
redundant? Please put that information in the commit text.
The tcs_in_use, is adjusted along with the DRV_STS and reading the
tcs_in_use should be enough.
Thanks for your review Stephen.
--Lina
Re: [PATCH 2/2] drivers: qcom: rpmh-rsc: fix read back of trigger register
On Fri, Jul 19 2019 at 12:22 -0600, Stephen Boyd wrote: Quoting Lina Iyer (2019-07-01 08:29:07) When triggering a TCS to send its contents, reading back the trigger value may return an incorrect value. That is because, writing the trigger may raise an interrupt which could be handled immediately and the trigger value could be reset in the interrupt handler. By doing a read back we may end up spinning waiting for the value we wrote. Doesn't this need to be squashed into the patch that gets rid of the irqs disabled state of this code? It sounds an awful lot like this problem only happens now because the previous patch removed the irqsave/irqrestore code around this function. True. Could be rolled into that fix. --Lina
Re: [PATCH 09/18] drivers: firmware: psci: Add support for PM domains using genpd
On Thu, Jul 18 2019 at 07:19 -0600, Sudeep Holla wrote:
On Thu, Jul 18, 2019 at 01:04:03PM +0200, Ulf Hansson wrote:
On Tue, 16 Jul 2019 at 17:05, Sudeep Holla wrote:
>
> On Mon, May 13, 2019 at 09:22:51PM +0200, Ulf Hansson wrote:
> > When the hierarchical CPU topology layout is used in DT, we need to setup
> > the corresponding PM domain data structures, as to allow a CPU and a group
> > of CPUs to be power managed accordingly. Let's enable this by deploying
> > support through the genpd interface.
> >
> > Additionally, when the OS initiated mode is supported by the PSCI FW, let's
> > also parse the domain idle states DT bindings as to make genpd responsible
> > for the state selection, when the states are compatible with
> > "domain-idle-state". Otherwise, when only Platform Coordinated mode is
> > supported, we rely solely on the state selection to be managed through the
> > regular cpuidle framework.
> >
> > If the initialization of the PM domain data structures succeeds and the OS
> > initiated mode is supported, we try to switch to it. In case it fails,
> > let's fall back into a degraded mode, rather than bailing out and returning
> > an error code.
> >
> > Due to that the OS initiated mode may become enabled, we need to adjust to
> > maintain backwards compatibility for a kernel started through a kexec call.
> > Do this by explicitly switch to Platform Coordinated mode during boot.
> >
> > Finally, the actual initialization of the PM domain data structures, is
> > done via calling the new shared function, psci_dt_init_pm_domains().
> > However, this is implemented by subsequent changes.
> >
> > Co-developed-by: Lina Iyer
> > Signed-off-by: Lina Iyer
> > Signed-off-by: Ulf Hansson
> > ---
> >
> > Changes:
> > - Simplify code setting domain_state at power off.
> > - Use the genpd ->free_state() callback to manage freeing of states.
> > - Fixup a bogus while loop.
> >
> > ---
> > drivers/firmware/psci/Makefile | 2 +-
> > drivers/firmware/psci/psci.c | 7 +-
> > drivers/firmware/psci/psci.h | 5 +
> > drivers/firmware/psci/psci_pm_domain.c | 268 +
> > 4 files changed, 280 insertions(+), 2 deletions(-)
> > create mode 100644 drivers/firmware/psci/psci_pm_domain.c
> >
>
> [...]
>
> > #endif /* __PSCI_H */
> > diff --git a/drivers/firmware/psci/psci_pm_domain.c
b/drivers/firmware/psci/psci_pm_domain.c
> > new file mode 100644
> > index ..3c6ca846caf4
> > --- /dev/null
> > +++ b/drivers/firmware/psci/psci_pm_domain.c
> > @@ -0,0 +1,268 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * PM domains for CPUs via genpd - managed by PSCI.
> > + *
> > + * Copyright (C) 2019 Linaro Ltd.
> > + * Author: Ulf Hansson
> > + *
> > + */
> > +
>
> [...]
>
> > +static int psci_pd_power_off(struct generic_pm_domain *pd)
> > +{
> > + struct genpd_power_state *state = >states[pd->state_idx];
> > + u32 *pd_state;
> > +
> > + /* If we have failed to enable OSI mode, then abort power off. */
> > + if (psci_has_osi_support() && !osi_mode_enabled)
> > + return -EBUSY;
> > +
> > + if (!state->data)
> > + return 0;
> > +
> > + /* When OSI mode is enabled, set the corresponding domain state. */
> > + pd_state = state->data;
> > + psci_set_domain_state(*pd_state);
>
> I trying to understand how would this scale to level 2(cluster of
> clusters or for simply system). The current code for psci_set_domain_state
> just stores the value @pd_state into per-cpu domain_state. E.g.: Now if
> the system level pd is getting called after cluster PD, it will set the
> domain state to system level PD state. It won't work with original
> format and it may work with extended format if it's carefully crafted.
> In short, the point is just over-writing domain_state is asking for
> troubles IMO.
Thanks for spotting this!
While walking upwards in the PM domain topology, I thought I was ORing
the domain states, but clearly the code isn't doing that.
In principle we need to do the below instead.
pd_state = state->data;
composite_pd_state = *pd_state | psci_get_domain_state();
psci_set_domain_state(composite_pd_state);
Yes 2 different issues here:
1. The direct assignment overwriting the value is problem which you agree.
2. The OR logic on it's own is bit not so clear from the specification.
Since firmware authors need to be aware of this to make all the
Re: [PATCH 14/18] drivers: firmware: psci: Manage runtime PM in the idle path for CPUs
On Thu, Jul 18 2019 at 10:55 -0600, Ulf Hansson wrote: On Thu, 18 Jul 2019 at 15:31, Lorenzo Pieralisi wrote: On Thu, Jul 18, 2019 at 12:35:07PM +0200, Ulf Hansson wrote: > On Tue, 16 Jul 2019 at 17:53, Lorenzo Pieralisi > wrote: > > > > On Mon, May 13, 2019 at 09:22:56PM +0200, Ulf Hansson wrote: > > > When the hierarchical CPU topology layout is used in DT, let's allow the > > > CPU to be power managed through its PM domain, via deploying runtime PM > > > support. > > > > > > To know for which idle states runtime PM reference counting is needed, > > > let's store the index of deepest idle state for the CPU, in a per CPU > > > variable. This allows psci_cpu_suspend_enter() to compare this index with > > > the requested idle state index and then act accordingly. > > > > I do not see why a system with two CPU CPUidle states, say CPU retention > > and CPU shutdown, should not be calling runtime PM on CPU retention > > entry. > > If the CPU idle governor did select the CPU retention for the CPU, it > was probably because the target residency for the CPU shutdown state > could not be met. The kernel does not know what those cpu states represent, so, this is an assumption you are making and it must be made clear that this code works as long as your assumption is valid. If eg a "cluster" retention state has lower target_residency than the deepest CPU idle state this assumption is wrong. Good point, you are right. I try to find a place to document this assumption. And CPUidle and genPD governor decisions are not synced anyway so, again, this is an assumption, not a certainty. > In this case, there is no point in allowing any other deeper idle > states for cluster/package/system, since those have even greater > residencies, hence calling runtime PM doesn't make sense. On the systems you are testing on. So what you are saying typically means, that if all CPUs in the same cluster have entered the CPU retention state, on some system the cluster may also put into a cluster retention state (assuming the target residency is met)? Do you know of any systems that has these characteristics? Many QCOM SoCs can do that. But with the hardware improving, the power-performance benefits skew the results in favor of powering off the cluster than keeping the CPU and cluster in retention. Kevin H and I thought of this problem earlier on. But that is a second level problem to solve and definitely to be thought of after we have the support for the deepest states in the kernel. We left that out for a later date. The idea would have been to setup the allowable state(s) in the DT for CPU and cluster state definitions and have the genpd take that into consideration when deciding the idle state for the domain. Thanks, Lina
Re: [PATCH 17/18] arm64: dts: Convert to the hierarchical CPU topology layout for MSM8916
On Tue, Jul 16 2019 at 08:47 -0600, Sudeep Holla wrote:
On Mon, May 13, 2019 at 09:22:59PM +0200, Ulf Hansson wrote:
From: Lina Iyer
In the hierarchical layout, we are creating power domains around each CPU
and describes the idle states for them inside the power domain provider
node. Note that, the CPU's idle states still needs to be compatible with
"arm,idle-state".
Furthermore, represent the CPU cluster as a separate master power domain,
powering the CPU's power domains. The cluster node, contains the idle
states for the cluster and each idle state needs to be compatible with the
"domain-idle-state".
If the running platform is using a PSCI FW that supports the OS initiated
CPU suspend mode, which likely should be the case unless the PSCI FW is
very old, this change triggers the PSCI driver to enable it.
Cc: Andy Gross
Cc: David Brown
Signed-off-by: Lina Iyer
Co-developed-by: Ulf Hansson
Signed-off-by: Ulf Hansson
---
[...]
@@ -166,12 +170,57 @@
min-residency-us = <2000>;
local-timer-stop;
};
+
+ CLUSTER_RET: cluster-retention {
+ compatible = "domain-idle-state";
+ arm,psci-suspend-param = <0x110>;
+ entry-latency-us = <500>;
+ exit-latency-us = <500>;
+ min-residency-us = <2000>;
+ };
+
+ CLUSTER_PWRDN: cluster-gdhs {
+ compatible = "domain-idle-state";
+ arm,psci-suspend-param = <0x130>;
+ entry-latency-us = <2000>;
+ exit-latency-us = <2000>;
+ min-residency-us = <6000>;
+ };
};
};
I was trying to understand the composition of composite state parameters
in this series and that made me look at these DT examples.
This was meant to depict a hierarchical state format for OSI.
What format does the above platform use ? I tried matching them to
both original as well as extended format and I fail to understand.
Assuming original format:
State power_state PowerLevel StateType StateID
SPC 0x4002 0(core)0(Retention) 0x2 (Res0 b[29]=1?)
CLUSTER_RET 0x110 1(clusters) 0(Retention) 0x10
CLUSTER_PWRDN 0x130 1(clusters) 0(Retention?) 0x30
Now extended format:
State power_state StateType StateID
SPC 0x4002 0(Retention) 0x4002 (Res0 b[29]=1?)
CLUSTER_RET 0x110 0(Retention) 0x110
The composite state would comprise of CPU state and Cluster state.
So for the last CPU entering idle -
(CLUSTER_RET | SPC)
0x4112
CLUSTER_PWRDN 0x130 0(Retention?) 0x130
(CLUSTER_PWRDN | SPC)
0x4132
Hope this helps.
Lina
Re: [PATCH 2/2] drivers: qcom: rpmh-rsc: fix read back of trigger register
Switching Andy's email address.
On Mon, Jul 01 2019 at 09:32 -0600, Lina Iyer wrote:
When triggering a TCS to send its contents, reading back the trigger
value may return an incorrect value. That is because, writing the
trigger may raise an interrupt which could be handled immediately and
the trigger value could be reset in the interrupt handler. By doing a
read back we may end up spinning waiting for the value we wrote.
Fixes: 658628 ("drivers: qcom: rpmh-rsc: add RPMH controller for QCOM
SoCs")
Signed-off-by: Lina Iyer
---
drivers/soc/qcom/rpmh-rsc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 92461311aef3..2fc2fa879480 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -300,7 +300,7 @@ static void __tcs_trigger(struct rsc_drv *drv, int tcs_id)
enable = TCS_AMC_MODE_ENABLE;
write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable);
enable |= TCS_AMC_MODE_TRIGGER;
- write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable);
+ write_tcs_reg(drv, RSC_DRV_CONTROL, tcs_id, enable);
}
static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Re: [PATCH 1/2] drivers: qcom: rpmh-rsc: simplify TCS locking
Switching Andy's email address.
On Mon, Jul 01 2019 at 09:32 -0600, Lina Iyer wrote:
From: "Raju P.L.S.S.S.N"
tcs->lock was introduced to serialize access with in TCS group. But
even without tcs->lock, drv->lock is serving the same purpose. So
use a single drv->lock.
Other optimizations include -
- Remove locking around clear_bit() in IRQ handler. clear_bit() is
atomic.
- Remove redundant read of TCS registers.
- Use spin_lock instead of _irq variants as the locks are not held
in interrupt context.
Fixes: 658628 ("drivers: qcom: rpmh-rsc: add RPMH controller for QCOM
SoCs")
Signed-off-by: Raju P.L.S.S.S.N
Signed-off-by: Lina Iyer
---
drivers/soc/qcom/rpmh-internal.h | 2 --
drivers/soc/qcom/rpmh-rsc.c | 37 +++-
drivers/soc/qcom/rpmh.c | 20 +++--
3 files changed, 21 insertions(+), 38 deletions(-)
diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index a767991c..969d5030860e 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -28,7 +28,6 @@ struct rsc_drv;
* @offset:start of the TCS group relative to the TCSes in the RSC
* @num_tcs: number of TCSes in this type
* @ncpt: number of commands in each TCS
- * @lock: lock for synchronizing this TCS writes
* @req: requests that are sent from the TCS
* @cmd_cache: flattened cache of cmds in sleep/wake TCS
* @slots: indicates which of @cmd_addr are occupied
@@ -40,7 +39,6 @@ struct tcs_group {
u32 offset;
int num_tcs;
int ncpt;
- spinlock_t lock;
const struct tcs_request *req[MAX_TCS_PER_TYPE];
u32 *cmd_cache;
DECLARE_BITMAP(slots, MAX_TCS_SLOTS);
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index e278fc11fe5c..92461311aef3 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -93,8 +93,7 @@ static void write_tcs_reg_sync(struct rsc_drv *drv, int reg,
int tcs_id,
static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
{
- return !test_bit(tcs_id, drv->tcs_in_use) &&
- read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id, 0);
+ return !test_bit(tcs_id, drv->tcs_in_use);
}
static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type)
@@ -104,29 +103,28 @@ static struct tcs_group *get_tcs_of_type(struct rsc_drv
*drv, int type)
static int tcs_invalidate(struct rsc_drv *drv, int type)
{
- int m;
+ int m, ret = 0;
struct tcs_group *tcs;
tcs = get_tcs_of_type(drv, type);
- spin_lock(>lock);
- if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
- spin_unlock(>lock);
- return 0;
- }
+ spin_lock(>lock);
+ if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS))
+ goto done;
for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
if (!tcs_is_free(drv, m)) {
- spin_unlock(>lock);
- return -EAGAIN;
+ ret = -EAGAIN;
+ goto done;
}
write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0);
write_tcs_reg_sync(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0);
}
bitmap_zero(tcs->slots, MAX_TCS_SLOTS);
- spin_unlock(>lock);
- return 0;
+done:
+ spin_unlock(>lock);
+ return ret;
}
/**
@@ -242,9 +240,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, i, 0);
write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, i, 0);
write_tcs_reg(drv, RSC_DRV_IRQ_CLEAR, 0, BIT(i));
- spin_lock(>lock);
clear_bit(i, drv->tcs_in_use);
- spin_unlock(>lock);
if (req)
rpmh_tx_done(req, err);
}
@@ -349,14 +345,12 @@ static int tcs_write(struct rsc_drv *drv, const struct
tcs_request *msg)
{
struct tcs_group *tcs;
int tcs_id;
- unsigned long flags;
int ret;
tcs = get_tcs_for_msg(drv, msg);
if (IS_ERR(tcs))
return PTR_ERR(tcs);
- spin_lock_irqsave(>lock, flags);
spin_lock(>lock);
/*
* The h/w does not like if we send a request to the same address,
@@ -364,26 +358,23 @@ static int tcs_write(struct rsc_drv *drv, const struct
tcs_request *msg)
*/
ret = check_for_req_inflight(drv, tcs, msg);
if (ret) {
- spin_unlock(>lock);
goto done_write;
}
tcs_id = find_free_tcs(tcs);
if (tcs_id < 0) {
ret = tcs_id;
- spin_unlock(>lock);
goto done_write;
}
tcs->req[tcs_id - tcs->offset] = msg;
set_bit(tcs_id, drv->tcs_in
[PATCH 1/2] drivers: qcom: rpmh-rsc: simplify TCS locking
From: "Raju P.L.S.S.S.N"
tcs->lock was introduced to serialize access with in TCS group. But
even without tcs->lock, drv->lock is serving the same purpose. So
use a single drv->lock.
Other optimizations include -
- Remove locking around clear_bit() in IRQ handler. clear_bit() is
atomic.
- Remove redundant read of TCS registers.
- Use spin_lock instead of _irq variants as the locks are not held
in interrupt context.
Fixes: 658628 ("drivers: qcom: rpmh-rsc: add RPMH controller for QCOM
SoCs")
Signed-off-by: Raju P.L.S.S.S.N
Signed-off-by: Lina Iyer
---
drivers/soc/qcom/rpmh-internal.h | 2 --
drivers/soc/qcom/rpmh-rsc.c | 37 +++-
drivers/soc/qcom/rpmh.c | 20 +++--
3 files changed, 21 insertions(+), 38 deletions(-)
diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index a767991c..969d5030860e 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -28,7 +28,6 @@ struct rsc_drv;
* @offset:start of the TCS group relative to the TCSes in the RSC
* @num_tcs: number of TCSes in this type
* @ncpt: number of commands in each TCS
- * @lock: lock for synchronizing this TCS writes
* @req: requests that are sent from the TCS
* @cmd_cache: flattened cache of cmds in sleep/wake TCS
* @slots: indicates which of @cmd_addr are occupied
@@ -40,7 +39,6 @@ struct tcs_group {
u32 offset;
int num_tcs;
int ncpt;
- spinlock_t lock;
const struct tcs_request *req[MAX_TCS_PER_TYPE];
u32 *cmd_cache;
DECLARE_BITMAP(slots, MAX_TCS_SLOTS);
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index e278fc11fe5c..92461311aef3 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -93,8 +93,7 @@ static void write_tcs_reg_sync(struct rsc_drv *drv, int reg,
int tcs_id,
static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
{
- return !test_bit(tcs_id, drv->tcs_in_use) &&
- read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id, 0);
+ return !test_bit(tcs_id, drv->tcs_in_use);
}
static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type)
@@ -104,29 +103,28 @@ static struct tcs_group *get_tcs_of_type(struct rsc_drv
*drv, int type)
static int tcs_invalidate(struct rsc_drv *drv, int type)
{
- int m;
+ int m, ret = 0;
struct tcs_group *tcs;
tcs = get_tcs_of_type(drv, type);
- spin_lock(>lock);
- if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
- spin_unlock(>lock);
- return 0;
- }
+ spin_lock(>lock);
+ if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS))
+ goto done;
for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
if (!tcs_is_free(drv, m)) {
- spin_unlock(>lock);
- return -EAGAIN;
+ ret = -EAGAIN;
+ goto done;
}
write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0);
write_tcs_reg_sync(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0);
}
bitmap_zero(tcs->slots, MAX_TCS_SLOTS);
- spin_unlock(>lock);
- return 0;
+done:
+ spin_unlock(>lock);
+ return ret;
}
/**
@@ -242,9 +240,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, i, 0);
write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, i, 0);
write_tcs_reg(drv, RSC_DRV_IRQ_CLEAR, 0, BIT(i));
- spin_lock(>lock);
clear_bit(i, drv->tcs_in_use);
- spin_unlock(>lock);
if (req)
rpmh_tx_done(req, err);
}
@@ -349,14 +345,12 @@ static int tcs_write(struct rsc_drv *drv, const struct
tcs_request *msg)
{
struct tcs_group *tcs;
int tcs_id;
- unsigned long flags;
int ret;
tcs = get_tcs_for_msg(drv, msg);
if (IS_ERR(tcs))
return PTR_ERR(tcs);
- spin_lock_irqsave(>lock, flags);
spin_lock(>lock);
/*
* The h/w does not like if we send a request to the same address,
@@ -364,26 +358,23 @@ static int tcs_write(struct rsc_drv *drv, const struct
tcs_request *msg)
*/
ret = check_for_req_inflight(drv, tcs, msg);
if (ret) {
- spin_unlock(>lock);
goto done_write;
}
tcs_id = find_free_tcs(tcs);
if (tcs_id < 0) {
ret = tcs_id;
- spin_unlock(>lock);
goto done_write;
}
tcs->req[tcs_id - tcs->offset] = msg;
set_bit(tcs_id, drv->tcs_in_use);
- spin_unlock(>lock);
__tcs_buffer_w
[PATCH 2/2] drivers: qcom: rpmh-rsc: fix read back of trigger register
When triggering a TCS to send its contents, reading back the trigger
value may return an incorrect value. That is because, writing the
trigger may raise an interrupt which could be handled immediately and
the trigger value could be reset in the interrupt handler. By doing a
read back we may end up spinning waiting for the value we wrote.
Fixes: 658628 ("drivers: qcom: rpmh-rsc: add RPMH controller for QCOM
SoCs")
Signed-off-by: Lina Iyer
---
drivers/soc/qcom/rpmh-rsc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 92461311aef3..2fc2fa879480 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -300,7 +300,7 @@ static void __tcs_trigger(struct rsc_drv *drv, int tcs_id)
enable = TCS_AMC_MODE_ENABLE;
write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable);
enable |= TCS_AMC_MODE_TRIGGER;
- write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable);
+ write_tcs_reg(drv, RSC_DRV_CONTROL, tcs_id, enable);
}
static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v5 11/11] arm64: dts: qcom: setup PDC as wakeup parent for GPIOs for SDM845
Setup PDC wakeup parent for TLMM for SDM845 SoC. Signed-off-by: Lina Iyer --- Changes in v3: - Provide irqdomain-map for GPIOs that map to PDC --- arch/arm64/boot/dts/qcom/sdm845.dtsi | 79 1 file changed, 79 insertions(+) diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi index 7d4b11c9314e..59da6944b106 100644 --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi @@ -1105,6 +1105,85 @@ #interrupt-cells = <2>; gpio-ranges = < 0 0 150>; + wakeup-parent = <_intc>; + irqdomain-map = <1 0 _intc 30 0>, + <3 0 _intc 31 0>, + <5 0 _intc 32 0>, + <10 0 _intc 33 0>, + <11 0 _intc 34 0>, + <20 0 _intc 35 0>, + <22 0 _intc 36 0>, + <24 0 _intc 37 0>, + <26 0 _intc 38 0>, + <30 0 _intc 39 0>, + <31 0 _intc 117 0>, + <32 0 _intc 41 0>, + <34 0 _intc 42 0>, + <36 0 _intc 43 0>, + <37 0 _intc 44 0>, + <38 0 _intc 45 0>, + <39 0 _intc 46 0>, + <40 0 _intc 47 0>, + <41 0 _intc 115 0>, + <43 0 _intc 49 0>, + <44 0 _intc 50 0>, + <46 0 _intc 51 0>, + <48 0 _intc 52 0>, + <49 0 _intc 118 0>, + <52 0 _intc 54 0>, + <53 0 _intc 55 0>, + <54 0 _intc 56 0>, + <56 0 _intc 57 0>, + <57 0 _intc 58 0>, + <58 0 _intc 59 0>, + <59 0 _intc 60 0>, + <60 0 _intc 61 0>, + <61 0 _intc 62 0>, + <62 0 _intc 63 0>, + <63 0 _intc 64 0>, + <64 0 _intc 65 0>, + <66 0 _intc 66 0>, + <68 0 _intc 67 0>, + <71 0 _intc 68 0>, + <73 0 _intc 69 0>, + <77 0 _intc 70 0>, + <78 0 _intc 71 0>, + <79 0 _intc 72 0>, + <80 0 _intc 73 0>, + <84 0 _intc 74 0>, + <85 0 _intc 75 0>, + <86 0 _intc 76 0>, + <88 0 _intc 77 0>, + <89 0 _intc 116 0>, + <91 0 _intc 79 0>, + <92 0 _intc 80 0>, + <95 0 _intc 81 0>, + <96 0 _intc 82 0>, + <97 0 _intc 83 0>, + <101 0 _intc 84 0>, + <103 0 _intc 85 0>, + <104 0 _intc 86 0>, + <115 0 _intc 90 0>, + <116 0 _intc 91 0>, + <117 0 _intc 92 0>, + <118 0 _intc 93 0>, + <119 0 _intc 94 0>, + <120 0 _intc 95 0>, + <121 0 _intc 96 0>, + <122 0 _intc 97 0>, + <123 0 _intc 98 0>, +
[PATCH v5 00/11] Support wakeup capable GPIOs
Hi all, This is a re-spin of the wakeup capable GPIO support for QCOM SoCs. The earlier version of the patch revision 4, was published [1] and had some good discussions. The comments from the review have also been addressed and the code rebased on top of 5.1 in this spin. There a few changes in this spin: - Review comments from Stephen, Marc - Bug fixes in irqdomain-map - Fix invalid interrupt case - Update documentation - Attempt generalizing gpiochip_to_irq() for hierarchical domain In patch v4, we were discussing about the IRQ type of GPIO defaulting to IRQ_TYPE_NONE (as is was the custom for older implementation). In the SDM845 SoC select GPIOs are routed to an always-on interrupt controller called the PDC and then to the GIC. Wakeup capabable: GPIO ---> PDC --> GIC Requesting a GPIO as an interrupt through the gpio_to_irq() call would setup an interrupt hierarchy as above and return the linux interrupt number. However, since the trigger type of the GPIO is unknown at this time, gpiolib defaults to IRQ_TYPE_NONE. This triggers a warning at the GIC, which expects a valid trigger type be set correctly in the fwspec. The solution to this problem is still at large and I would like to solicit feedback on this. Appreciate your time. Thanks, Lina [1]. https://patchwork.kernel.org/cover/10851807/ Lina Iyer (9): gpio: allow gpio_to_irq to use OF variants for gpiochips irqdomain: add bus token DOMAIN_BUS_WAKEUP of: irq: document properties for wakeup interrupt parent drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO drivers: pinctrl: msm: setup GPIO irqchip hierarchy arm64: dts: qcom: add PDC interrupt controller for SDM845 arm64: defconfig: enable PDC interrupt controller for Qualcomm SDM845 arm64: dts: qcom: setup PDC as wakeup parent for GPIOs for SDM845 Stephen Boyd (1): of: irq: add helper to remap interrupts to another irqdomain Thierry Reding (1): gpio: Add support for hierarchical IRQ domains .../interrupt-controller/interrupts.txt | 54 +++ .../bindings/pinctrl/qcom,sdm845-pinctrl.txt | 79 +- arch/arm64/boot/dts/qcom/sdm845.dtsi | 88 +++ arch/arm64/configs/defconfig | 1 + drivers/gpio/gpiolib.c| 28 +++- drivers/irqchip/qcom-pdc.c| 98 +++-- drivers/of/irq.c | 129 + drivers/pinctrl/qcom/pinctrl-msm.c| 137 +++--- include/linux/gpio/driver.h | 6 + include/linux/irqdomain.h | 1 + include/linux/of_irq.h| 1 + include/linux/soc/qcom/irq.h | 25 12 files changed, 610 insertions(+), 37 deletions(-) create mode 100644 include/linux/soc/qcom/irq.h -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH v5 10/11] arm64: defconfig: enable PDC interrupt controller for Qualcomm SDM845
Enable PDC interrupt controller for SDM845 devices. The interrupt controller can detect wakeup capable interrupts when the SoC is in a low power state. Signed-off-by: Lina Iyer --- arch/arm64/configs/defconfig | 1 + 1 file changed, 1 insertion(+) diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig index 2d9c39033c1a..4e5e681c4b11 100644 --- a/arch/arm64/configs/defconfig +++ b/arch/arm64/configs/defconfig @@ -685,6 +685,7 @@ CONFIG_QCOM_SMEM=y CONFIG_QCOM_SMD_RPM=y CONFIG_QCOM_SMP2P=y CONFIG_QCOM_SMSM=y +CONFIG_QCOM_PDC=y CONFIG_ROCKCHIP_PM_DOMAINS=y CONFIG_ARCH_TEGRA_132_SOC=y CONFIG_ARCH_TEGRA_210_SOC=y -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH v5 07/11] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO
SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO routed to the PDC as interrupts that can be used to wake the system up from deep low power modes and suspend. Update the sdm845 pinctrl device bindings to reference the PDC wakeup interrupt controller and the GPIO PDC interrupt map. Cc: devicet...@vger.kernel.org Signed-off-by: Lina Iyer --- .../bindings/pinctrl/qcom,sdm845-pinctrl.txt | 79 ++- 1 file changed, 78 insertions(+), 1 deletion(-) diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt index 665aadb5ea28..895832127193 100644 --- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt @@ -53,7 +53,6 @@ pin, a group, or a list of pins or groups. This configuration can include the mux function to select on those pin(s)/group(s), and various pin configuration parameters, such as pull-up, drive strength, etc. - PIN CONFIGURATION NODES: The name of each subnode is not important; all subnodes should be enumerated @@ -160,6 +159,84 @@ Example: #gpio-cells = <2>; interrupt-controller; #interrupt-cells = <2>; + wakeup-parent = <_intc>; + irqdomain-map = <1 0 _intc 30 0>, + <3 0 _intc 31 0>, + <5 0 _intc 32 0>, + <10 0 _intc 33 0>, + <11 0 _intc 34 0>, + <20 0 _intc 35 0>, + <22 0 _intc 36 0>, + <24 0 _intc 37 0>, + <26 0 _intc 38 0>, + <30 0 _intc 39 0>, + <31 0 _intc 117 0>, + <32 0 _intc 41 0>, + <34 0 _intc 42 0>, + <36 0 _intc 43 0>, + <37 0 _intc 44 0>, + <38 0 _intc 45 0>, + <39 0 _intc 46 0>, + <40 0 _intc 47 0>, + <41 0 _intc 115 0>, + <43 0 _intc 49 0>, + <44 0 _intc 50 0>, + <46 0 _intc 51 0>, + <48 0 _intc 52 0>, + <49 0 _intc 118 0>, + <52 0 _intc 54 0>, + <53 0 _intc 55 0>, + <54 0 _intc 56 0>, + <56 0 _intc 57 0>, + <57 0 _intc 58 0>, + <58 0 _intc 59 0>, + <59 0 _intc 60 0>, + <60 0 _intc 61 0>, + <61 0 _intc 62 0>, + <62 0 _intc 63 0>, + <63 0 _intc 64 0>, + <64 0 _intc 65 0>, + <66 0 _intc 66 0>, + <68 0 _intc 67 0>, + <71 0 _intc 68 0>, + <73 0 _intc 69 0>, + <77 0 _intc 70 0>, + <78 0 _intc 71 0>, + <79 0 _intc 72 0>, + <80 0 _intc 73 0>, + <84 0 _intc 74 0>, + <85 0 _intc 75 0>, + <86 0 _intc 76 0>, + <88 0 _intc 77 0>, + <89 0 _intc 116 0>, + <91 0 _intc 79 0>, + <92 0 _intc 80 0>, + <95 0 _intc 81 0>, + <96 0 _intc 82 0>, + <97 0 _intc 83 0>, + <101 0 _intc 84 0>, + <103 0 _intc 85 0>, + <104 0 _intc 86 0>, + <115 0 _intc 90 0>, + <116 0 _intc 91 0>, + <117 0 _intc 92 0>, + <118 0 _intc 93 0>, + <119 0 _intc 94 0>, + <120 0 _intc 95 0>, +
[PATCH v5 04/11] of: irq: document properties for wakeup interrupt parent
Some interrupt controllers in a SoC, are always powered on and have a
select interrupts routed to them, so that they can wakeup the SoC from
suspend. Add wakeup-parent DT property to refer to these interrupt
controllers.
If the interrupts routed to the wakeup parent are not sequential, than a
map needs to exist to associate the same interrupt line on multiple
interrupt controllers. Providing this map in every driver is cumbersome.
Let's add this in the device tree and document the properties to map the
interrupt specifiers
Cc: devicet...@vger.kernel.org
Signed-off-by: Lina Iyer
---
Changes in v5:
- Update documentation to describe masks in the example
Changes in v4:
- Added this documentation
---
.../interrupt-controller/interrupts.txt | 54 +++
1 file changed, 54 insertions(+)
diff --git
a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
index 8a3c40829899..e3e43f5d5566 100644
--- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
@@ -108,3 +108,57 @@ commonly used:
sensitivity = <7>;
};
};
+
+3) Interrupt wakeup parent
+--
+
+Some interrupt controllers in a SoC, are always powered on and have a select
+interrupts routed to them, so that they can wakeup the SoC from suspend. These
+interrupt controllers do not fall into the category of a parent interrupt
+controller and can be specified by the "wakeup-parent" property and contain a
+single phandle referring to the wakeup capable interrupt controller.
+
+ Example:
+ wakeup-parent = <_intc>;
+
+
+4) Interrupt mapping
+
+
+Sometimes interrupts may be detected by more than one interrupt controller
+(depending on which controller is active). The interrupt controllers may not
+be in hierarchy and therefore the interrupt controller driver is required to
+establish the relationship between the same interrupt at different interrupt
+controllers. If these interrupts are not sequential then a map needs to be
+specified to help identify these interrupts.
+
+Mapping the interrupt specifiers in the device tree can be done using the
+"irqdomain-map" property. The property contains interrupt specifier at the
+current interrupt controller followed by the interrupt specifier at the mapped
+interrupt controller.
+
+ irqdomain-map =
+
+The optional properties "irqdomain-map-mask" and "irqdomain-map-pass-thru" may
+be provided to help interpret the valid bits of the incoming and mapped
+interrupt specifiers respectively.
+
+ Example:
+ intc: interrupt-controller@17a0 {
+ #interrupt-cells = <3>;
+ };
+
+ pinctrl@340 {
+ #interrupt-cells = <2>;
+ irqdomain-map = <22 0 36 0>, <24 0 37 0>;
+ irqdomain-map-mask = <0xff 0>;
+ irqdomain-map-pass-thru = <0 0xff>;
+ };
+
+In the above example, the input interrupt specifier map-mask <0xff 0> applied
+on the incoming interrupt specifier of the map <22 0>, <24 0>, returns the
+input interrupt 22, 24 etc. The second argument being irq type is immaterial
+from the map and is used from the incoming request instead. The pass-thru
+specifier parses the output interrupt specifier from the rest of the unparsed
+argments from the map < 36 0>, < 37 0> etc to return the output
+interrupt 36, 37 etc.
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v5 02/11] gpio: allow gpio_to_irq to use OF variants for gpiochips
The chip may define additional information in the DT that may be useful
for translating and allocting a linux interrupt for the GPIO. When
drivers do not specify a .to_irq function, the gpiolib defaults to
gpiochip_to_irq() function. The defalt function uses creates an IRQ
without referencing the OF definition of the gpiochip. Let's add this
OF support to the default gpiochip_to_irq function.
When requesting an interrupt for the GPIO, let's stick to IRQ_TYPE_NONE
for the trigger type, for we don't have the information what trigger
type the driver may set when requesting the IRQ.
Signed-off-by: Lina Iyer
---
drivers/gpio/gpiolib.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 4a9a6d4afe6e..77317435e2b2 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1825,6 +1825,19 @@ EXPORT_SYMBOL_GPL(gpiochip_irq_domain_deactivate);
static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
{
+#ifdef CONFIG_OF_GPIO
+ struct irq_fwspec fwspec;
+
+ if (chip->of_node) {
+ fwspec.fwnode = of_node_to_fwnode(chip->of_node);
+ fwspec.param[0] = offset;
+ fwspec.param[1] = IRQ_TYPE_NONE;
+ fwspec.param_count = 2;
+
+ return irq_create_fwspec_mapping();
+ }
+#endif
+
if (!gpiochip_irqchip_irq_valid(chip, offset))
return -ENXIO;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v5 03/11] irqdomain: add bus token DOMAIN_BUS_WAKEUP
A single controller can handle normal interrupts and wake-up interrupts
independently, with a different numbering space. It is thus crucial to
allow the driver for such a controller discriminate between the two.
A simple way to do so is to tag the wake-up irqdomain with a "bus token"
that indicates the wake-up domain. This slightly abuses the notion of
bus, but also radically simplifies the design of such a driver. Between
two evils, we choose the least damaging.
Suggested-by: Stephen Boyd
Signed-off-by: Lina Iyer
---
Changes in v4:
- Update commit text
---
include/linux/irqdomain.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 61706b430907..4eaf17dfd2f8 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -82,6 +82,7 @@ enum irq_domain_bus_token {
DOMAIN_BUS_NEXUS,
DOMAIN_BUS_IPI,
DOMAIN_BUS_FSL_MC_MSI,
+ DOMAIN_BUS_WAKEUP,
};
/**
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v5 06/11] drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs
Introduce a new domain for wakeup capable GPIOs. The domain can be
requested using the bus token DOMAIN_BUS_WAKEUP. In the following
patches, we will specify PDC as the wakeup-parent for the TLMM GPIO
irqchip. Requesting a wakeup GPIO will setup the GPIO and the
corresponding PDC interrupt as its parent.
Co-developed-by: Stephen Boyd
Signed-off-by: Lina Iyer
---
Changes in v5:
- Define invalid wakeup interrupt
Changes in v4:
- Remove vestigial changes from v2
Changes in v3:
- Remove PDC GPIO map data (moved to DT)
- hwirq passed in .alloc() is a PDC pin now
Changes in v2:
- Remove separate file for PDC GPIO map data
- Error checks and return
- Whitespace fixes
---
drivers/irqchip/qcom-pdc.c | 98
include/linux/soc/qcom/irq.h | 25 +
2 files changed, 114 insertions(+), 9 deletions(-)
create mode 100644 include/linux/soc/qcom/irq.h
diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index faa7d61b9d6c..ef0135fbc41a 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -13,12 +13,13 @@
#include
#include
#include
+#include
#include
-#include
#include
#include
#define PDC_MAX_IRQS 126
+#define PDC_MAX_GPIO_IRQS 256
#define CLEAR_INTR(reg, intr) (reg & ~(1 << intr))
#define ENABLE_INTR(reg, intr) (reg | (1 << intr))
@@ -26,6 +27,8 @@
#define IRQ_ENABLE_BANK0x10
#define IRQ_i_CFG 0x110
+#define PDC_NO_PARENT_IRQ ~0UL
+
struct pdc_pin_region {
u32 pin_base;
u32 parent_base;
@@ -65,12 +68,18 @@ static void pdc_enable_intr(struct irq_data *d, bool on)
static void qcom_pdc_gic_mask(struct irq_data *d)
{
+ if (d->hwirq == GPIO_NO_WAKE_IRQ)
+ return;
+
pdc_enable_intr(d, false);
irq_chip_mask_parent(d);
}
static void qcom_pdc_gic_unmask(struct irq_data *d)
{
+ if (d->hwirq == GPIO_NO_WAKE_IRQ)
+ return;
+
pdc_enable_intr(d, true);
irq_chip_unmask_parent(d);
}
@@ -114,6 +123,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d,
unsigned int type)
int pin_out = d->hwirq;
enum pdc_irq_config_bits pdc_type;
+ if (pin_out == GPIO_NO_WAKE_IRQ)
+ return 0;
+
switch (type) {
case IRQ_TYPE_EDGE_RISING:
pdc_type = PDC_EDGE_RISING;
@@ -169,8 +181,7 @@ static irq_hw_number_t get_parent_hwirq(int pin)
return (region->parent_base + pin - region->pin_base);
}
- WARN_ON(1);
- return ~0UL;
+ return PDC_NO_PARENT_IRQ;
}
static int qcom_pdc_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
@@ -199,17 +210,17 @@ static int qcom_pdc_alloc(struct irq_domain *domain,
unsigned int virq,
ret = qcom_pdc_translate(domain, fwspec, , );
if (ret)
- return -EINVAL;
-
- parent_hwirq = get_parent_hwirq(hwirq);
- if (parent_hwirq == ~0UL)
- return -EINVAL;
+ return ret;
ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
_pdc_gic_chip, NULL);
if (ret)
return ret;
+ parent_hwirq = get_parent_hwirq(hwirq);
+ if (parent_hwirq == PDC_NO_PARENT_IRQ)
+ return 0;
+
if (type & IRQ_TYPE_EDGE_BOTH)
type = IRQ_TYPE_EDGE_RISING;
@@ -232,6 +243,63 @@ static const struct irq_domain_ops qcom_pdc_ops = {
.free = irq_domain_free_irqs_common,
};
+static int qcom_pdc_gpio_alloc(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs, void *data)
+{
+ struct qcom_irq_fwspec *qcom_fwspec = data;
+ struct irq_fwspec *fwspec = _fwspec->fwspec;
+ struct irq_fwspec parent_fwspec;
+ irq_hw_number_t hwirq, parent_hwirq;
+ unsigned int type;
+ int ret;
+
+ ret = qcom_pdc_translate(domain, fwspec, , );
+ if (ret)
+ return ret;
+
+ ret = irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
+ _pdc_gic_chip, NULL);
+ if (ret)
+ return ret;
+
+ if (hwirq == GPIO_NO_WAKE_IRQ)
+ return 0;
+
+ parent_hwirq = get_parent_hwirq(hwirq);
+ if (parent_hwirq == PDC_NO_PARENT_IRQ)
+ return 0;
+
+ qcom_fwspec->mask = true;
+
+ if (type & IRQ_TYPE_EDGE_BOTH)
+ type = IRQ_TYPE_EDGE_RISING;
+
+ if (type & IRQ_TYPE_LEVEL_MASK)
+ type = IRQ_TYPE_LEVEL_HIGH;
+
+ parent_fwspec.fwnode = domain->parent->fwnode;
+ parent_fwspec.param_count = 3;
+ parent_fwspec.param[0]= 0;
+ parent_fwspec.param[1]= parent_hwirq;
+ parent_fwspec.param[2]= type;
+
+ re
[PATCH v5 09/11] arm64: dts: qcom: add PDC interrupt controller for SDM845
Add PDC interrupt controller device bindings for SDM845.
Signed-off-by: Lina Iyer
---
Changes in v1:
- Use updated address specification in reg
- Rename to pdc_intc
- Sort per address in DT
---
arch/arm64/boot/dts/qcom/sdm845.dtsi | 9 +
1 file changed, 9 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi
b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 5308f1671824..7d4b11c9314e 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1934,6 +1934,15 @@
#power-domain-cells = <1>;
};
+ pdc_intc: interrupt-controller@b22 {
+ compatible = "qcom,sdm845-pdc";
+ reg = <0 0x0b22 0 0x3>;
+ qcom,pdc-ranges = <0 480 94>, <94 609 15>, <115 630 7>;
+ #interrupt-cells = <2>;
+ interrupt-parent = <>;
+ interrupt-controller;
+ };
+
pdc_reset: reset-controller@b2e {
compatible = "qcom,sdm845-pdc-global";
reg = <0 0x0b2e 0 0x2>;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v5 08/11] drivers: pinctrl: msm: setup GPIO irqchip hierarchy
To allow GPIOs to wakeup the system from suspend or deep idle, the
wakeup capable GPIOs are setup in hierarchy with interrupts from the
wakeup-parent irqchip.
In older SoC's, the TLMM will handover detection to the parent irqchip
and in newer SoC's, the parent irqchip may also be active as well as the
TLMM and therefore the GPIOs need to be masked at TLMM to avoid
duplicate interrupts. To enable both these configurations to exist,
allow the parent irqchip to dictate the TLMM irqchip's behavior when
masking/unmasking the interrupt.
Co-developed-by: Stephen Boyd
Signed-off-by: Lina Iyer
---
Changes in v5:
- Fix when GPIO does not have a wakeup interrupt parent
Changes in v4:
- Remove irq_set_wake() on summary IRQ interrupt
Changes in v3:
- Use of_irq_domain_map() and pass PDC pin to parent irqdomain
Changes in v2:
- Call parent mask when masking GPIO interrupt
Changes in v1:
- Fix bug when unmasking PDC interrupt
---
drivers/pinctrl/qcom/pinctrl-msm.c | 137 -
1 file changed, 114 insertions(+), 23 deletions(-)
diff --git a/drivers/pinctrl/qcom/pinctrl-msm.c
b/drivers/pinctrl/qcom/pinctrl-msm.c
index ee8119879c4c..e52c1c300bcc 100644
--- a/drivers/pinctrl/qcom/pinctrl-msm.c
+++ b/drivers/pinctrl/qcom/pinctrl-msm.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -27,6 +28,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -69,6 +71,7 @@ struct msm_pinctrl {
DECLARE_BITMAP(dual_edge_irqs, MAX_NR_GPIO);
DECLARE_BITMAP(enabled_irqs, MAX_NR_GPIO);
+ DECLARE_BITMAP(wakeup_masked_irqs, MAX_NR_GPIO);
const struct msm_pinctrl_soc_data *soc;
void __iomem *regs[MAX_NR_TILES];
@@ -703,6 +706,13 @@ static void msm_gpio_irq_mask(struct irq_data *d)
g = >soc->groups[d->hwirq];
+ if (d->parent_data)
+ irq_chip_mask_parent(d);
+
+ /* Monitored by parent wakeup controller?*/
+ if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs))
+ return;
+
raw_spin_lock_irqsave(>lock, flags);
val = msm_readl_intr_cfg(pctrl, g);
@@ -747,6 +757,13 @@ static void msm_gpio_irq_unmask(struct irq_data *d)
g = >soc->groups[d->hwirq];
+ if (d->parent_data)
+ irq_chip_unmask_parent(d);
+
+ /* Monitored by parent wakeup controller? Keep masked */
+ if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs))
+ return;
+
raw_spin_lock_irqsave(>lock, flags);
val = msm_readl_intr_cfg(pctrl, g);
@@ -767,6 +784,10 @@ static void msm_gpio_irq_ack(struct irq_data *d)
unsigned long flags;
u32 val;
+ /* Handled by parent wakeup controller? Do nothing */
+ if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs))
+ return;
+
g = >soc->groups[d->hwirq];
raw_spin_lock_irqsave(>lock, flags);
@@ -794,6 +815,13 @@ static int msm_gpio_irq_set_type(struct irq_data *d,
unsigned int type)
g = >soc->groups[d->hwirq];
+ if (d->parent_data)
+ irq_chip_set_type_parent(d, type);
+
+ /* Monitored by parent wakeup controller? Keep masked */
+ if (test_bit(d->hwirq, pctrl->wakeup_masked_irqs))
+ return 0;
+
raw_spin_lock_irqsave(>lock, flags);
/*
@@ -880,17 +908,10 @@ static int msm_gpio_irq_set_type(struct irq_data *d,
unsigned int type)
static int msm_gpio_irq_set_wake(struct irq_data *d, unsigned int on)
{
- struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
- struct msm_pinctrl *pctrl = gpiochip_get_data(gc);
- unsigned long flags;
-
- raw_spin_lock_irqsave(>lock, flags);
-
- irq_set_irq_wake(pctrl->irq, on);
-
- raw_spin_unlock_irqrestore(>lock, flags);
+ if (d->parent_data)
+ return irq_chip_set_wake_parent(d, on);
- return 0;
+ return -ENODEV;
}
static int msm_gpio_irq_reqres(struct irq_data *d)
@@ -967,11 +988,75 @@ static bool msm_gpio_needs_valid_mask(struct msm_pinctrl
*pctrl)
return device_property_read_u16_array(pctrl->dev, "gpios", NULL, 0) > 0;
}
+static int msm_gpio_domain_translate(struct irq_domain *d,
+struct irq_fwspec *fwspec,
+unsigned long *hwirq, unsigned int *type)
+{
+ if (is_of_node(fwspec->fwnode)) {
+ if (fwspec->param_count < 2)
+ return -EINVAL;
+ *hwirq = fwspec->param[0];
+ *type = fwspec->param[1] & IRQ_TYPE_SENSE_MASK;
+ return 0;
+ }
+
+ return -EINVAL;
+}
+
+static int msm_gpio_domain_alloc(struct irq_domain *domain, unsigned int virq,
+unsigned int nr_irqs
[PATCH v5 01/11] gpio: Add support for hierarchical IRQ domains
From: Thierry Reding
Hierarchical IRQ domains can be used to stack different IRQ controllers
on top of each other. One specific use-case where this can be useful is
if a power management controller has top-level controls for wakeup
interrupts. In such cases, the power management controller can be a
parent to other interrupt controllers and program additional registers
when an IRQ has its wake capability enabled or disabled.
Signed-off-by: Thierry Reding
Signed-off-by: Lina Iyer
---
drivers/gpio/gpiolib.c | 15 +++
include/linux/gpio/driver.h | 6 ++
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index bca3e7740ef6..4a9a6d4afe6e 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1936,7 +1936,9 @@ static int gpiochip_add_irqchip(struct gpio_chip
*gpiochip,
type = IRQ_TYPE_NONE;
}
- gpiochip->to_irq = gpiochip_to_irq;
+ if (!gpiochip->to_irq)
+ gpiochip->to_irq = gpiochip_to_irq;
+
gpiochip->irq.default_type = type;
gpiochip->irq.lock_key = lock_key;
gpiochip->irq.request_key = request_key;
@@ -1946,9 +1948,14 @@ static int gpiochip_add_irqchip(struct gpio_chip
*gpiochip,
else
ops = _domain_ops;
- gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
-gpiochip->irq.first,
-ops, gpiochip);
+ if (gpiochip->irq.parent_domain)
+ gpiochip->irq.domain =
irq_domain_add_hierarchy(gpiochip->irq.parent_domain,
+ 0,
gpiochip->ngpio,
+ np, ops,
gpiochip);
+ else
+ gpiochip->irq.domain = irq_domain_add_simple(np,
gpiochip->ngpio,
+
gpiochip->irq.first,
+ops, gpiochip);
if (!gpiochip->irq.domain)
return -EINVAL;
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 01497910f023..f481862f1bb0 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -48,6 +48,12 @@ struct gpio_irq_chip {
*/
const struct irq_domain_ops *domain_ops;
+ /**
+* @parent_domain:
+*
+*/
+ struct irq_domain *parent_domain;
+
/**
* @handler:
*
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v5 05/11] of: irq: add helper to remap interrupts to another irqdomain
From: Stephen Boyd
Sometimes interrupts are routed from an interrupt controller to another
in no specific order. Having these in the drivers makes it difficult to
maintain when the same drivers supports multiple variants with different
mapping. Also, specifying them in DT makes little sense with a bunch of
numbers like -
<0, 13>, <5, 32>,
It makes more sense when we can have the parent handle along with
interrupt specifiers for the incoming interrupt as well as that of the
outgoing interrupt like -
<22 0 36 0>,
<24 0 37 0>,
<26 0 38 0>,
And the interrupt specifiers can be interpreted using these optional
properties -
irqdomain-map-mask = <0xff 0>;
irqdomain-map-pass-thru = <0 0xff>;
The irqdomain-map-mask reads the input interrupt specifier to parse the
incoming interrupt port. The format of the output port is specified with
the irqdomain-map-pass-thru property.
Let's add a helper function to parse this from DT and match a struct
irq_fwspec using the input interrupt specifier from the irqdomain-map
and the valid bits specified in the irqdomain-map-mask and copy the
output interrupt specifier from the map to irq_fwspec per the mask in
irqdomain-map-pass-thru property for the matched interrupt.
Signed-off-by: Stephen Boyd
Signed-off-by: Lina Iyer
---
Changes in v5:
- Fix returning 0 when no match is found
Changes in v4:
- Fix commit text spelling and verbosity
---
drivers/of/irq.c | 129 +
include/linux/of_irq.h | 1 +
2 files changed, 130 insertions(+)
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index e1f6f392a4c0..6186904b2b6b 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -273,6 +273,135 @@ int of_irq_parse_raw(const __be32 *addr, struct
of_phandle_args *out_irq)
}
EXPORT_SYMBOL_GPL(of_irq_parse_raw);
+int of_irq_domain_map(const struct irq_fwspec *in, struct irq_fwspec *out)
+{
+ char *stem_name;
+ char *cells_name, *map_name = NULL, *mask_name = NULL;
+ char *pass_name = NULL;
+ struct device_node *cur, *new = NULL;
+ const __be32 *map, *mask, *pass;
+ static const __be32 dummy_mask[] = { [0 ... MAX_PHANDLE_ARGS] = ~0 };
+ static const __be32 dummy_pass[] = { [0 ... MAX_PHANDLE_ARGS] = 0 };
+ __be32 initial_match_array[MAX_PHANDLE_ARGS];
+ const __be32 *match_array = initial_match_array;
+ int i, ret, map_len, match;
+ u32 in_size, out_size;
+
+ stem_name = "";
+ cells_name = "#interrupt-cells";
+
+ ret = -ENOMEM;
+ map_name = kasprintf(GFP_KERNEL, "irqdomain%s-map", stem_name);
+ if (!map_name)
+ goto free;
+
+ mask_name = kasprintf(GFP_KERNEL, "irqdomain%s-map-mask", stem_name);
+ if (!mask_name)
+ goto free;
+
+ pass_name = kasprintf(GFP_KERNEL, "irqdomain%s-map-pass-thru",
stem_name);
+ if (!pass_name)
+ goto free;
+
+ /* Get the #interrupt-cells property */
+ cur = to_of_node(in->fwnode);
+ ret = of_property_read_u32(cur, cells_name, _size);
+ if (ret < 0)
+ goto put;
+
+ /* Precalculate the match array - this simplifies match loop */
+ for (i = 0; i < in_size; i++)
+ initial_match_array[i] = cpu_to_be32(in->param[i]);
+
+ ret = -EINVAL;
+ /* Get the irqdomain-map property */
+ map = of_get_property(cur, map_name, _len);
+ if (!map) {
+ ret = 0;
+ goto free;
+ }
+ map_len /= sizeof(u32);
+
+ /* Get the irqdomain-map-mask property (optional) */
+ mask = of_get_property(cur, mask_name, NULL);
+ if (!mask)
+ mask = dummy_mask;
+ /* Iterate through irqdomain-map property */
+ match = 0;
+ while (map_len > (in_size + 1) && !match) {
+ /* Compare specifiers */
+ match = 1;
+ for (i = 0; i < in_size; i++, map_len--)
+ match &= !((match_array[i] ^ *map++) & mask[i]);
+
+ of_node_put(new);
+ new = of_find_node_by_phandle(be32_to_cpup(map));
+ map++;
+ map_len--;
+
+ /* Check if not found */
+ if (!new)
+ goto put;
+
+ if (!of_device_is_available(new))
+ match = 0;
+
+ ret = of_property_read_u32(new, cells_name, _size);
+ if (ret)
+ goto put;
+
+ /* Check for malformed properties */
+ if (WARN_ON(out_size > MAX_PHANDLE_ARGS))
+ goto put;
+ if (map_len < out_size)
+ goto put;
+
+ /* Move forward by new node's #interrupt-cells amount */
+
Re: [PATCH v4 07/10] drivers: pinctrl: msm: setup GPIO irqchip hierarchy
On Wed, Apr 17 2019 at 07:59 -0600, Linus Walleij wrote: On Thu, Mar 21, 2019 at 10:54 PM Stephen Boyd wrote: Quoting Marc Zyngier (2019-03-16 04:39:48)> > On Fri, 15 Mar 2019 09:28:31 -0700 > Stephen Boyd wrote: > > > Quoting Lina Iyer (2019-03-13 14:18:41) > > > @@ -994,6 +1092,22 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) > > > pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres; > > > pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres; > > > > > > + chip->irq.chip = >irq_chip; > > > + chip->irq.domain_ops = _gpio_domain_ops; > > > + chip->irq.handler = handle_edge_irq; > > > + chip->irq.default_type = IRQ_TYPE_EDGE_RISING; > > > > This also changed from v3. It used to be IRQ_TYPE_NONE. Specifying this > > here seems to cause gpiolib to print a WARN. > > > > > > /* > > * Specifying a default trigger is a terrible idea if DT or ACPI is > > * used to configure the interrupts, as you may end up with > > * conflicting triggers. Tell the user, and reset to NONE. > > */ > > if (WARN(np && type != IRQ_TYPE_NONE, > > "%s: Ignoring %u default trigger\n", np->full_name, type)) > > type = IRQ_TYPE_NONE; > > > > > > So I guess this change should be dropped. Or at the least, it should be > > split out to it's own patch and the motivations can be discussed in the > > commit text. > > It is something I requested (although I expected this to be a > different patch, and even a clarification would have been OK). > > One way or another, the default trigger must match the flow handler. If > we set it up with IRQ_TYPE_NONE, what does it mean? The fact that > IRQ_TYPE_NONE acts as a wildcard doesn't mean the handle_edge_irq flow > handler is a good match for all interrupt types (it is rarely OK for > level interrupts). I think this is a question for Thierry or Linus. I'm not sure why this check was put in place in the code. I tried to dig into it really quick but I didn't find anything obvious and then I gave up. Maybe with hierarchical irqdomains we can drop this check? I don't think the gpiolib core ever uses this 'default_type' or 'handler' for anything once we replace the irqdomain that's used for a particular gpiochip with a custom irqdomain. The only user I see, gpiochip_irq_map(), won't ever be called so it really ends up being a thing that the driver specific irqdomains should check for and reject when parsing the DT and it sees IRQ_TYPE_NONE come out. --8<--- diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 144af0733581..fe2f7888c473 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1922,7 +1922,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, * used to configure the interrupts, as you may end up with * conflicting triggers. Tell the user, and reset to NONE. */ - if (WARN(np && type != IRQ_TYPE_NONE, + if (WARN(!gpiochip->irq.domain_ops && np && type != IRQ_TYPE_NONE, "%s: Ignoring %u default trigger\n", np->full_name, type)) type = IRQ_TYPE_NONE; Sorry for taking long time to answer... this got lost in some mail storms. It's a bit of Marc Z question really but I try to answer and he can correct me. We are now getting used to ACPI and DT always specifying the IRQ trigger type on the consumer handle: a device tells the irqchip what kind of edge or level it wants. Things weren't always like that. Some boards in the kernel is still using board files. (Yeah please help in modernizing them, I am doing my part.) Old machines with GPIO irqchip jitted to the SoC irq controller sometimes had a hardcoded behavior such as edge, and the consumers would only issue something really legacy like request_irq(42, myhandler, 0, "myirq", data); and expect it to work, since 0 means use the default flags, it might have a platform device with this irq number passed as a resource, but that is a really dumb platform device still, and it might not have set any irqflags for the irq number it passes. It probably doesn't even know that the irq number is backed by an irq descriptor. Since the code that e.g. DT has inside drivers/of/platform.c irq_of_parse_and_map(), will incidentally create an irq descriptor and set up these flags from the consumer flags in the device tree and call the irqchip to set up the trigger through .set_type() whenever the interrupt is requested, this is no problem for DT. Or ACPI. But on a board file, the .set_type() will eventually be called with IRQ_TYPE_NONE, which will cause a bug, or
Re: [PATCH v4 07/10] drivers: pinctrl: msm: setup GPIO irqchip hierarchy
On Fri, Mar 15 2019 at 10:28 -0600, Stephen Boyd wrote:
Quoting Lina Iyer (2019-03-13 14:18:41)
---
Changes in v4:
- Remove irq_set_wake() on summary IRQ interrupt
Changes in v3:
- Use of_irq_domain_map() and pass PDC pin to parent irqdomain
Changes in v2:
- Call parent mask when masking GPIO interrupt
Changes in v1:
- Fix bug when unmasking PDC interrupt
[...]
+}
+
+/*
+ * TODO: Get rid of this and push it into gpiochip_to_irq()
+ */
Any chance this TODO can be resolved?
I am thinking of something like this. Would there be any issue in
setting the type to IRQ_TYPE_SENSE_MASK instead of any one particular
type?
---8<-
static int gpiochip_to_irq(struct gpio_chip *chip, unsigned offset)
{
#ifdef CONFIG_OF_GPIO
struct irq_fwspec fwspec;
if (chip->of_node) {
fwspec.fwnode = of_node_to_fwnode(chip->of_node);
fwspec.param[0] = offset;
fwspec.param[1] = IRQ_TYPE_SENSE_MASK;
fwspec.param_count = 2;
return irq_create_fwspec_mapping();
}
#endif
if (!gpiochip_irqchip_irq_valid(chip, offset))
return -ENXIO;
return irq_create_mapping(chip->irq.domain, offset);
}
---8<
Thanks,
Lina
Re: [PATCH v4 07/10] drivers: pinctrl: msm: setup GPIO irqchip hierarchy
On Thu, Mar 21 2019 at 15:54 -0600, Stephen Boyd wrote: Quoting Marc Zyngier (2019-03-16 04:39:48) On Fri, 15 Mar 2019 09:28:31 -0700 Stephen Boyd wrote: > Quoting Lina Iyer (2019-03-13 14:18:41) > > @@ -994,6 +1092,22 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl) > > pctrl->irq_chip.irq_request_resources = msm_gpio_irq_reqres; > > pctrl->irq_chip.irq_release_resources = msm_gpio_irq_relres; > > > > + chip->irq.chip = >irq_chip; > > + chip->irq.domain_ops = _gpio_domain_ops; > > + chip->irq.handler = handle_edge_irq; > > + chip->irq.default_type = IRQ_TYPE_EDGE_RISING; > > This also changed from v3. It used to be IRQ_TYPE_NONE. Specifying this > here seems to cause gpiolib to print a WARN. > > > /* > * Specifying a default trigger is a terrible idea if DT or ACPI is > * used to configure the interrupts, as you may end up with > * conflicting triggers. Tell the user, and reset to NONE. > */ > if (WARN(np && type != IRQ_TYPE_NONE, > "%s: Ignoring %u default trigger\n", np->full_name, type)) > type = IRQ_TYPE_NONE; > > > So I guess this change should be dropped. Or at the least, it should be > split out to it's own patch and the motivations can be discussed in the > commit text. It is something I requested (although I expected this to be a different patch, and even a clarification would have been OK). One way or another, the default trigger must match the flow handler. If we set it up with IRQ_TYPE_NONE, what does it mean? The fact that IRQ_TYPE_NONE acts as a wildcard doesn't mean the handle_edge_irq flow handler is a good match for all interrupt types (it is rarely OK for level interrupts). I think this is a question for Thierry or Linus. I'm not sure why this check was put in place in the code. I tried to dig into it really quick but I didn't find anything obvious and then I gave up. Maybe with hierarchical irqdomains we can drop this check? I don't think the gpiolib core ever uses this 'default_type' or 'handler' for anything once we replace the irqdomain that's used for a particular gpiochip with a custom irqdomain. The only user I see, gpiochip_irq_map(), won't ever be called so it really ends up being a thing that the driver specific irqdomains should check for and reject when parsing the DT and it sees IRQ_TYPE_NONE come out. --8<--- diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 144af0733581..fe2f7888c473 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1922,7 +1922,7 @@ static int gpiochip_add_irqchip(struct gpio_chip *gpiochip, * used to configure the interrupts, as you may end up with * conflicting triggers. Tell the user, and reset to NONE. */ - if (WARN(np && type != IRQ_TYPE_NONE, + if (WARN(!gpiochip->irq.domain_ops && np && type != IRQ_TYPE_NONE, "%s: Ignoring %u default trigger\n", np->full_name, type)) type = IRQ_TYPE_NONE; Linus, Any thoughts on this? -- Lina
Re: [PATCH v4 03/10] of/irq: document properties for wakeup interrupt parent
On Tue, Apr 16 2019 at 10:54 -0600, Stephen Boyd wrote: Quoting Lina Iyer (2019-04-04 08:58:38) On Mon, Mar 18 2019 at 11:54 -0600, Marc Zyngier wrote: >On Wed, 13 Mar 2019 15:18:37 -0600 >Lina Iyer wrote: > >Please do Cc Rob when posting DT related patches. > >> Some interrupt controllers in a SoC, are always powered on and have a >> select interrupts routed to them, so that they can wakeup the SoC from >> suspend. Add wakeup-parent DT property to refer to these interrupt >> controllers. >> >> If the interrupts routed to the wakeup parent are not sequential, than a >> map needs to exist to associate the same interrupt line on multiple >> interrupt controllers. Providing this map in every driver is cumbersome. >> Let's add this in the device tree and document the properties to map the >> interrupt specifiers >> >> Signed-off-by: Lina Iyer >> --- >> Changes in v4: >> - Added this documentation >> --- >> .../interrupt-controller/interrupts.txt | 39 +++ >> 1 file changed, 39 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt >> index 8a3c40829899..917b598317f5 100644 >> --- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt >> +++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt >> @@ -108,3 +108,42 @@ commonly used: >> sensitivity = <7>; >> }; >> }; >> + >> +3) Interrupt wakeup parent >> +-- >> + >> +Some interrupt controllers in a SoC, are always powered on and have a select >> +interrupts routed to them, so that they can wakeup the SoC from suspend. These >> +interrupt controllers do not fall into the category of a parent interrupt >> +controller and can be specified by the "wakeup-parent" property and contain a >> +single phandle referring to the wakeup capable interrupt controller. >> + >> + Example: >> +wakeup-parent = <_intc>; >> + >> + >> +4) Interrupt mapping >> + >> + >> +Sometimes interrupts may be detected by more than one interrupt controller >> +(depending on which controller is active). The interrupt controllers may not >> +be in hierarchy and therefore the interrupt controller driver is required to >> +establish the relationship between the same interrupt at different interrupt >> +controllers. If these interrupts are not sequential then a map needs to be >> +specified to help identify these interrupts. >> + >> +Mapping the interrupt specifiers in the device tree can be done using the >> +"irqdomain-map" property. The property contains interrupt specifier at the >> +current interrupt controller followed by the interrupt specifier at the mapped >> +interrupt controller. >> + >> + irqdomain-map = >> + >> +The optional properties "irqdomain-map-mask" and "irqdomain-map-pass-thru" may >> +be provided to help interpret the valid bits of the incoming and mapped >> +interrupt specifiers respectively. >> + >> + Example: >> +irqdomain-map = <22 0 36 0>, <24 0 37 0>; >> +irqdomain-map-mask = <0xff 0>; >> +irqdomain-map-pass-thru = <0 0xff>; > > >This doesn't quite explain how the mask and pass-thru properties are >used. I guess that the mask is used to define the 'useful bits' on the >incoming side, but pass-thru puzzles me. In your example, does it mean >that incoming lines map to outgoing interrupt <0 0>? > Sorry about the late reply. How about this to go with the rest of the documentation - In the above example, the input interrupt specifier map-mask <0xff 0> applied on the incoming interrupt specifier of the map <22 0>, <24 0>, returns the input interrupt 22, 24 etc. The second argument being irq type is immaterial from the map and is used from the incoming request instead. The pass-thru specifier parses the output interrupt specifier from the rest of the unparsed argments from the map < 36 0>, < 37 0> etc to return the output interrupt 36, 37 etc. I see two things going on here. Do both need to happen? #1: Specifying wakeup parent phandle #2: Mapping GPIO interrupts to a parent irqdomain Do we need the method of specifying the wakeup parent if with a dt property if we have a way to map irqdomains from one to another? I think I may have already said on the list that we must have #1 but now I'm not so sure. It looks like we
Re: [PATCH v4 03/10] of/irq: document properties for wakeup interrupt parent
On Mon, Apr 15 2019 at 06:42 -0600, Marc Zyngier wrote: On 04/04/2019 16:58, Lina Iyer wrote: On Mon, Mar 18 2019 at 11:54 -0600, Marc Zyngier wrote: On Wed, 13 Mar 2019 15:18:37 -0600 Lina Iyer wrote: Please do Cc Rob when posting DT related patches. Some interrupt controllers in a SoC, are always powered on and have a select interrupts routed to them, so that they can wakeup the SoC from suspend. Add wakeup-parent DT property to refer to these interrupt controllers. If the interrupts routed to the wakeup parent are not sequential, than a map needs to exist to associate the same interrupt line on multiple interrupt controllers. Providing this map in every driver is cumbersome. Let's add this in the device tree and document the properties to map the interrupt specifiers Signed-off-by: Lina Iyer --- Changes in v4: - Added this documentation --- .../interrupt-controller/interrupts.txt | 39 +++ 1 file changed, 39 insertions(+) diff --git a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt index 8a3c40829899..917b598317f5 100644 --- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt +++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt @@ -108,3 +108,42 @@ commonly used: sensitivity = <7>; }; }; + +3) Interrupt wakeup parent +-- + +Some interrupt controllers in a SoC, are always powered on and have a select +interrupts routed to them, so that they can wakeup the SoC from suspend. These +interrupt controllers do not fall into the category of a parent interrupt +controller and can be specified by the "wakeup-parent" property and contain a +single phandle referring to the wakeup capable interrupt controller. + + Example: + wakeup-parent = <_intc>; + + +4) Interrupt mapping + + +Sometimes interrupts may be detected by more than one interrupt controller +(depending on which controller is active). The interrupt controllers may not +be in hierarchy and therefore the interrupt controller driver is required to +establish the relationship between the same interrupt at different interrupt +controllers. If these interrupts are not sequential then a map needs to be +specified to help identify these interrupts. + +Mapping the interrupt specifiers in the device tree can be done using the +"irqdomain-map" property. The property contains interrupt specifier at the +current interrupt controller followed by the interrupt specifier at the mapped +interrupt controller. + + irqdomain-map = + +The optional properties "irqdomain-map-mask" and "irqdomain-map-pass-thru" may +be provided to help interpret the valid bits of the incoming and mapped +interrupt specifiers respectively. + + Example: + irqdomain-map = <22 0 36 0>, <24 0 37 0>; + irqdomain-map-mask = <0xff 0>; + irqdomain-map-pass-thru = <0 0xff>; This doesn't quite explain how the mask and pass-thru properties are used. I guess that the mask is used to define the 'useful bits' on the incoming side, but pass-thru puzzles me. In your example, does it mean that incoming lines map to outgoing interrupt <0 0>? Sorry about the late reply. How about this to go with the rest of the documentation - In the above example, the input interrupt specifier map-mask <0xff 0> applied on the incoming interrupt specifier of the map <22 0>, <24 0>, returns the input interrupt 22, 24 etc. The second argument being irq type is immaterial from the map and is used from the incoming request instead. The pass-thru specifier parses the output interrupt specifier from the rest of the unparsed argments from the map < 36 0>, < 37 0> etc to return the output interrupt 36, 37 etc. I think you need to add #interrupt-cells in your example, which is otherwise hard to interpret. Ok. Thanks, will add. --Lina
Re: [PATCH v4 00/10] support wakeup capable GPIOs
On Mon, Apr 15 2019 at 06:43 -0600, Marc Zyngier wrote: On 13/03/2019 21:18, Lina Iyer wrote: Hi all, This series adds support for wakeup capable GPIOs. It is based on Thierry's hiearchical GPIO domains. This approach is based on Stephen's idea [1]. The SoC that is used for this development is a QCOM SDM845. The current patchset is rebased on top of 5.0 and adds documentation for the wakeup-parent and irqdomain-map DT properties along with the the optional irqdomain-map-mask and irqdomain-map-pass-thru properties. Also incorporating comments from Marc on the earlier submission [2]. I cleaned up some of the change history in these patches to match the version number with that of the submission. The dtsi patches are based on Bjorn's changes for increased address and cell size [3] and [4]. Kindly review the series. What the status of this? What is the expected merge strategy? Hi Mark, I ran into a couple of issues, most of which have been sorted out. But there is a hardware requirement to write up another register (to set type), for GPIO wakeup that was missed earlier. I am trying to get that tested out. That's the hold up. Sorry about that. --Lina
Re: [PATCH v4 03/10] of/irq: document properties for wakeup interrupt parent
On Mon, Mar 18 2019 at 11:54 -0600, Marc Zyngier wrote: On Wed, 13 Mar 2019 15:18:37 -0600 Lina Iyer wrote: Please do Cc Rob when posting DT related patches. Some interrupt controllers in a SoC, are always powered on and have a select interrupts routed to them, so that they can wakeup the SoC from suspend. Add wakeup-parent DT property to refer to these interrupt controllers. If the interrupts routed to the wakeup parent are not sequential, than a map needs to exist to associate the same interrupt line on multiple interrupt controllers. Providing this map in every driver is cumbersome. Let's add this in the device tree and document the properties to map the interrupt specifiers Signed-off-by: Lina Iyer --- Changes in v4: - Added this documentation --- .../interrupt-controller/interrupts.txt | 39 +++ 1 file changed, 39 insertions(+) diff --git a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt index 8a3c40829899..917b598317f5 100644 --- a/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt +++ b/Documentation/devicetree/bindings/interrupt-controller/interrupts.txt @@ -108,3 +108,42 @@ commonly used: sensitivity = <7>; }; }; + +3) Interrupt wakeup parent +-- + +Some interrupt controllers in a SoC, are always powered on and have a select +interrupts routed to them, so that they can wakeup the SoC from suspend. These +interrupt controllers do not fall into the category of a parent interrupt +controller and can be specified by the "wakeup-parent" property and contain a +single phandle referring to the wakeup capable interrupt controller. + + Example: + wakeup-parent = <_intc>; + + +4) Interrupt mapping + + +Sometimes interrupts may be detected by more than one interrupt controller +(depending on which controller is active). The interrupt controllers may not +be in hierarchy and therefore the interrupt controller driver is required to +establish the relationship between the same interrupt at different interrupt +controllers. If these interrupts are not sequential then a map needs to be +specified to help identify these interrupts. + +Mapping the interrupt specifiers in the device tree can be done using the +"irqdomain-map" property. The property contains interrupt specifier at the +current interrupt controller followed by the interrupt specifier at the mapped +interrupt controller. + + irqdomain-map = + +The optional properties "irqdomain-map-mask" and "irqdomain-map-pass-thru" may +be provided to help interpret the valid bits of the incoming and mapped +interrupt specifiers respectively. + + Example: + irqdomain-map = <22 0 36 0>, <24 0 37 0>; + irqdomain-map-mask = <0xff 0>; + irqdomain-map-pass-thru = <0 0xff>; This doesn't quite explain how the mask and pass-thru properties are used. I guess that the mask is used to define the 'useful bits' on the incoming side, but pass-thru puzzles me. In your example, does it mean that incoming lines map to outgoing interrupt <0 0>? Sorry about the late reply. How about this to go with the rest of the documentation - In the above example, the input interrupt specifier map-mask <0xff 0> applied on the incoming interrupt specifier of the map <22 0>, <24 0>, returns the input interrupt 22, 24 etc. The second argument being irq type is immaterial from the map and is used from the incoming request instead. The pass-thru specifier parses the output interrupt specifier from the rest of the unparsed argments from the map < 36 0>, < 37 0> etc to return the output interrupt 36, 37 etc. --Lina
Re: [PATCH v4 04/10] of: irq: add helper to remap interrupts to another irqdomain
I observed an issue with return value being set to 0, when a match is
not found in the irqdomain-map.
On Wed, Mar 13 2019 at 15:20 -0600, Lina Iyer wrote:
From: Stephen Boyd
Sometimes interrupts are routed from an interrupt controller to another
in no specific order. Having these in the drivers makes it difficult to
maintain when the same drivers supports multiple variants with different
mapping. Also, specifying them in DT makes little sense with a bunch of
numbers like -
<0, 13>, <5, 32>,
It makes more sense when we can have the parent handle along with
interrupt specifiers for the incoming interrupt as well as that of the
outgoing interrupt like -
<22 0 36 0>,
<24 0 37 0>,
<26 0 38 0>,
And the interrupt specifiers can be interpreted using these optional
properties -
irqdomain-map-mask = <0xff 0>;
irqdomain-map-pass-thru = <0 0xff>;
The irqdomain-map-mask reads the input interrupt specifier to parse the
incoming interrupt port. The format of the output port is specified with
the irqdomain-map-pass-thru property.
Let's add a helper function to parse this from DT and match a struct
irq_fwspec using the input interrupt specifier from the irqdomain-map
and the valid bits specified in the irqdomain-map-mask and copy the
output interrupt specifier from the map to irq_fwspec per the mask in
irqdomain-map-pass-thru property for the matched interrupt.
Signed-off-by: Stephen Boyd
Signed-off-by: Lina Iyer
---
Changes in v4:
- Fix commit text spelling and verbosity
---
drivers/of/irq.c | 125 +
include/linux/of_irq.h | 1 +
2 files changed, 126 insertions(+)
diff --git a/drivers/of/irq.c b/drivers/of/irq.c
index e1f6f392a4c0..a1534f947ed4 100644
--- a/drivers/of/irq.c
+++ b/drivers/of/irq.c
@@ -273,6 +273,131 @@ int of_irq_parse_raw(const __be32 *addr, struct
of_phandle_args *out_irq)
}
EXPORT_SYMBOL_GPL(of_irq_parse_raw);
+int of_irq_domain_map(const struct irq_fwspec *in, struct irq_fwspec *out)
+{
+ char *stem_name;
+ char *cells_name, *map_name = NULL, *mask_name = NULL;
+ char *pass_name = NULL;
+ struct device_node *cur, *new = NULL;
+ const __be32 *map, *mask, *pass;
+ static const __be32 dummy_mask[] = { [0 ... MAX_PHANDLE_ARGS] = ~0 };
+ static const __be32 dummy_pass[] = { [0 ... MAX_PHANDLE_ARGS] = 0 };
+ __be32 initial_match_array[MAX_PHANDLE_ARGS];
+ const __be32 *match_array = initial_match_array;
+ int i, ret, map_len, match;
+ u32 in_size, out_size;
+
+ stem_name = "";
+ cells_name = "#interrupt-cells";
+
+ ret = -ENOMEM;
+ map_name = kasprintf(GFP_KERNEL, "irqdomain%s-map", stem_name);
+ if (!map_name)
+ goto free;
+
+ mask_name = kasprintf(GFP_KERNEL, "irqdomain%s-map-mask", stem_name);
+ if (!mask_name)
+ goto free;
+
+ pass_name = kasprintf(GFP_KERNEL, "irqdomain%s-map-pass-thru",
stem_name);
+ if (!pass_name)
+ goto free;
+
+ /* Get the #interrupt-cells property */
+ cur = to_of_node(in->fwnode);
+ ret = of_property_read_u32(cur, cells_name, _size);
+ if (ret < 0)
+ goto put;
+
+ /* Precalculate the match array - this simplifies match loop */
+ for (i = 0; i < in_size; i++)
+ initial_match_array[i] = cpu_to_be32(in->param[i]);
+
+ ret = -EINVAL;
This value...
+ /* Get the irqdomain-map property */
+ map = of_get_property(cur, map_name, _len);
+ if (!map) {
+ ret = 0;
+ goto free;
+ }
+ map_len /= sizeof(u32);
+
+ /* Get the irqdomain-map-mask property (optional) */
+ mask = of_get_property(cur, mask_name, NULL);
+ if (!mask)
+ mask = dummy_mask;
+ /* Iterate through irqdomain-map property */
+ match = 0;
+ while (map_len > (in_size + 1) && !match) {
+ /* Compare specifiers */
+ match = 1;
+ for (i = 0; i < in_size; i++, map_len--)
+ match &= !((match_array[i] ^ *map++) & mask[i]);
+
+ of_node_put(new);
+ new = of_find_node_by_phandle(be32_to_cpup(map));
+ map++;
+ map_len--;
+
+ /* Check if not found */
+ if (!new)
+ goto put;
+
+ if (!of_device_is_available(new))
+ match = 0;
+
+ ret = of_property_read_u32(new, cells_name, _size);
Gets overwritten by this, and set to 0.
+ if (ret)
+ goto put;
+
+ /* Check for malformed properties */
+ if (WARN_ON(out_size > MAX_PHANDLE_ARGS))
+ goto put;
+
Re: [PATCH v4 06/10] dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO
Thanks for the review Rob.
On Fri, Mar 15 2019 at 17:37 -0600, Rob Herring wrote:
On Wed, Mar 13, 2019 at 03:18:40PM -0600, Lina Iyer wrote:
SDM845 SoC has an always-on interrupt controller (PDC) with select GPIO
routed to the PDC as interrupts that can be used to wake the system up
from deep low power modes and suspend.
Cc: devicet...@vger.kernel.org
Signed-off-by: Lina Iyer
---
.../devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt| 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
I still don't love this being specific to wake-up, but don't have a
better suggestion. One nit, otherwise:
I added this to interrupts.txt. I will remove this, if the documentation in
[PATCH v4 03/10] of/irq: document properties for wakeup interrupt parent
of this series works.
-- Lina
Reviewed-by: Rob Herring
diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
index 665aadb5ea28..f0fedbc5d41a 100644
--- a/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/qcom,sdm845-pinctrl.txt
@@ -29,6 +29,11 @@ SDM845 platform.
Definition: must be 2. Specifying the pin number and flags, as defined
in
+- wakeup-parent:
+ Usage: optional
+ Value type:
+ Definition: A phandle to the wakeup interrupt controller for the SoC.
+
- gpio-controller:
Usage: required
Value type:
@@ -53,7 +58,6 @@ pin, a group, or a list of pins or groups. This configuration
can include the
mux function to select on those pin(s)/group(s), and various pin configuration
parameters, such as pull-up, drive strength, etc.
-
PIN CONFIGURATION NODES:
The name of each subnode is not important; all subnodes should be enumerated
@@ -160,6 +164,7 @@ Example:
#gpio-cells = <2>;
interrupt-controller;
#interrupt-cells = <2>;
+ wake-parent = <_intc>;
wakeup-parent
qup9_active: qup9-active {
mux {
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v4 01/10] gpio: Add support for hierarchical IRQ domains
From: Thierry Reding
Hierarchical IRQ domains can be used to stack different IRQ controllers
on top of each other. One specific use-case where this can be useful is
if a power management controller has top-level controls for wakeup
interrupts. In such cases, the power management controller can be a
parent to other interrupt controllers and program additional registers
when an IRQ has its wake capability enabled or disabled.
Signed-off-by: Thierry Reding
Signed-off-by: Lina Iyer
---
drivers/gpio/gpiolib.c | 15 +++
include/linux/gpio/driver.h | 6 ++
2 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index d1adfdf50fb3..46ccb9b174b9 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1895,7 +1895,9 @@ static int gpiochip_add_irqchip(struct gpio_chip
*gpiochip,
type = IRQ_TYPE_NONE;
}
- gpiochip->to_irq = gpiochip_to_irq;
+ if (!gpiochip->to_irq)
+ gpiochip->to_irq = gpiochip_to_irq;
+
gpiochip->irq.default_type = type;
gpiochip->irq.lock_key = lock_key;
gpiochip->irq.request_key = request_key;
@@ -1905,9 +1907,14 @@ static int gpiochip_add_irqchip(struct gpio_chip
*gpiochip,
else
ops = _domain_ops;
- gpiochip->irq.domain = irq_domain_add_simple(np, gpiochip->ngpio,
-gpiochip->irq.first,
-ops, gpiochip);
+ if (gpiochip->irq.parent_domain)
+ gpiochip->irq.domain =
irq_domain_add_hierarchy(gpiochip->irq.parent_domain,
+ 0,
gpiochip->ngpio,
+ np, ops,
gpiochip);
+ else
+ gpiochip->irq.domain = irq_domain_add_simple(np,
gpiochip->ngpio,
+
gpiochip->irq.first,
+ops, gpiochip);
if (!gpiochip->irq.domain)
return -EINVAL;
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index 07cddbf45186..88ef196f96ec 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -48,6 +48,12 @@ struct gpio_irq_chip {
*/
const struct irq_domain_ops *domain_ops;
+ /**
+* @parent_domain:
+*
+*/
+ struct irq_domain *parent_domain;
+
/**
* @handler:
*
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH v4 00/10] support wakeup capable GPIOs
Hi all, This series adds support for wakeup capable GPIOs. It is based on Thierry's hiearchical GPIO domains. This approach is based on Stephen's idea [1]. The SoC that is used for this development is a QCOM SDM845. The current patchset is rebased on top of 5.0 and adds documentation for the wakeup-parent and irqdomain-map DT properties along with the the optional irqdomain-map-mask and irqdomain-map-pass-thru properties. Also incorporating comments from Marc on the earlier submission [2]. I cleaned up some of the change history in these patches to match the version number with that of the submission. The dtsi patches are based on Bjorn's changes for increased address and cell size [3] and [4]. Kindly review the series. Thanks, Lina [1]. https://lkml.org/lkml/2018/12/19/813 [2]. https://lkml.org/lkml/2019/2/22/716 [3]. https://patchwork.kernel.org/patch/10767511/ [4]. https://lkml.kernel.org/r/20190117042940.25487-2-bjorn.anders...@linaro.org Lina Iyer (8): irqdomain: add bus token DOMAIN_BUS_WAKEUP of/irq: document properties for wakeup interrupt parent drivers: irqchip: add PDC irqdomain for wakeup capable GPIOs dt-bindings: sdm845-pinctrl: add wakeup interrupt parent for GPIO drivers: pinctrl: msm: setup GPIO irqchip hierarchy arm64: dts: qcom: add PDC interrupt controller for SDM845 arm64: dts: qcom: setup PDC as wakeup parent for GPIOs for SDM845 arm64: defconfig: enable PDC interrupt controller for Qualcomm SDM845 Stephen Boyd (1): of: irq: add helper to remap interrupts to another irqdomain Thierry Reding (1): gpio: Add support for hierarchical IRQ domains .../interrupt-controller/interrupts.txt | 39 + .../bindings/pinctrl/qcom,sdm845-pinctrl.txt | 7 +- arch/arm64/boot/dts/qcom/sdm845.dtsi | 87 ++ arch/arm64/configs/defconfig | 1 + drivers/gpio/gpiolib.c| 15 +- drivers/irqchip/qcom-pdc.c| 72 - drivers/of/irq.c | 125 +++ drivers/pinctrl/qcom/pinctrl-msm.c| 151 +++--- include/linux/gpio/driver.h | 6 + include/linux/irqdomain.h | 1 + include/linux/of_irq.h| 1 + include/linux/soc/qcom/irq.h | 23 +++ 12 files changed, 497 insertions(+), 31 deletions(-) create mode 100644 include/linux/soc/qcom/irq.h -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
[PATCH v4 02/10] irqdomain: add bus token DOMAIN_BUS_WAKEUP
A single controller can handle normal interrupts and wake-up interrupts
independently, with a different numbering space. It is thus crucial to
allow the driver for such a controller discriminate between the two.
A simple way to do so is to tag the wake-up irqdomain with a "bus token"
that indicates the wake-up domain. This slightly abuses the notion of
bus, but also radically simplifies the design of such a driver. Between
two evils, we choose the least damaging.
Suggested-by: Stephen Boyd
Signed-off-by: Lina Iyer
---
Changes in v4:
- Update commit text
---
include/linux/irqdomain.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/irqdomain.h b/include/linux/irqdomain.h
index 35965f41d7be..05055bc992ab 100644
--- a/include/linux/irqdomain.h
+++ b/include/linux/irqdomain.h
@@ -82,6 +82,7 @@ enum irq_domain_bus_token {
DOMAIN_BUS_NEXUS,
DOMAIN_BUS_IPI,
DOMAIN_BUS_FSL_MC_MSI,
+ DOMAIN_BUS_WAKEUP,
};
/**
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
