| Message ID | 20220201140412.467233-3-elder@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: ipa: enable register retention | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 4 of 4 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 130 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Tue, 1 Feb 2022 08:04:12 -0600 Alex Elder wrote:
> Fixes: 2775cbc5afeb6 ("net: ipa: rename "ipa_clock.c"")
The Fixes tag should point at the place the code was introduced,
even if it moved or otherwise the patch won't apply as far back.
On 2/2/22 11:02 PM, Jakub Kicinski wrote: > On Tue, 1 Feb 2022 08:04:12 -0600 Alex Elder wrote: >> Fixes: 2775cbc5afeb6 ("net: ipa: rename "ipa_clock.c"") > > The Fixes tag should point at the place the code was introduced, > even if it moved or otherwise the patch won't apply as far back. The problem was not "activated" until this commit: 1aac309d32075 net: ipa: use autosuspend And that commit was merged together in a series that included the one I mentioned above: 2775cbc5afeb6 net: ipa: rename "ipa_clock.c" The rename commit is two commits after "use autosuspend". The merge commit was: 863434886497d Merge branch 'ipa-autosuspend' Until autosuspend is enabled, this new code is completely unnecessary, so back-porting it beyond that is pointless. I supplied the commit in the "Fixes" tag because I thought it would be close to equivalent and would avoid some trouble back-porting. Perhaps the "use autosuspend" commit is the one that should be in the "Fixes" tag, but I don't believe it should be back-ported any further than that. Re-spinning the series to fix the tag is trivial, but before I do that, can you tell me which commit you recommend I use in the "Fixes" tag? The original commit that introduced the microcontroller code (and also included the clock/power code) is: a646d6ec90983 soc: qcom: ipa: modem and microcontroller Thanks. -Alex
On Thu, 3 Feb 2022 05:22:23 -0600 Alex Elder wrote: > On 2/2/22 11:02 PM, Jakub Kicinski wrote: > > On Tue, 1 Feb 2022 08:04:12 -0600 Alex Elder wrote: > >> Fixes: 2775cbc5afeb6 ("net: ipa: rename "ipa_clock.c"") > > > > The Fixes tag should point at the place the code was introduced, > > even if it moved or otherwise the patch won't apply as far back. > > The problem was not "activated" until this commit: > 1aac309d32075 net: ipa: use autosuspend > > > And that commit was merged together in a series that > included the one I mentioned above: > 2775cbc5afeb6 net: ipa: rename "ipa_clock.c" > > The rename commit is two commits after "use autosuspend". > > The merge commit was: > 863434886497d Merge branch 'ipa-autosuspend' > > > Until autosuspend is enabled, this new code is > completely unnecessary, so back-porting it beyond > that is pointless. I supplied the commit in the > "Fixes" tag because I thought it would be close > to equivalent and would avoid some trouble back-porting. > > Perhaps the "use autosuspend" commit is the one that > should be in the "Fixes" tag, but I don't believe it > should be back-ported any further than that. > > Re-spinning the series to fix the tag is trivial, but > before I do that, can you tell me which commit you > recommend I use in the "Fixes" tag? > > The original commit that introduced the microcontroller > code (and also included the clock/power code) is: > a646d6ec90983 soc: qcom: ipa: modem and microcontroller Thanks for the explanation 1aac309d32075 sounds like the right choice. Let me just swap it for you and apply the v2.
diff --git a/drivers/net/ipa/ipa_power.c b/drivers/net/ipa/ipa_power.c index b1c6c0fcb654f..f2989aac47a62 100644 --- a/drivers/net/ipa/ipa_power.c +++ b/drivers/net/ipa/ipa_power.c @@ -11,6 +11,8 @@ #include <linux/pm_runtime.h> #include <linux/bitops.h> +#include "linux/soc/qcom/qcom_aoss.h" + #include "ipa.h" #include "ipa_power.h" #include "ipa_endpoint.h" @@ -64,6 +66,7 @@ enum ipa_power_flag { * struct ipa_power - IPA power management information * @dev: IPA device pointer * @core: IPA core clock + * @qmp: QMP handle for AOSS communication * @spinlock: Protects modem TX queue enable/disable * @flags: Boolean state flags * @interconnect_count: Number of elements in interconnect[] @@ -72,6 +75,7 @@ enum ipa_power_flag { struct ipa_power { struct device *dev; struct clk *core; + struct qmp *qmp; spinlock_t spinlock; /* used with STOPPED/STARTED power flags */ DECLARE_BITMAP(flags, IPA_POWER_FLAG_COUNT); u32 interconnect_count; @@ -382,6 +386,47 @@ void ipa_power_modem_queue_active(struct ipa *ipa) clear_bit(IPA_POWER_FLAG_STARTED, ipa->power->flags); } +static int ipa_power_retention_init(struct ipa_power *power) +{ + struct qmp *qmp = qmp_get(power->dev); + + if (IS_ERR(qmp)) { + if (PTR_ERR(qmp) == -EPROBE_DEFER) + return -EPROBE_DEFER; + + /* We assume any other error means it's not defined/needed */ + qmp = NULL; + } + power->qmp = qmp; + + return 0; +} + +static void ipa_power_retention_exit(struct ipa_power *power) +{ + qmp_put(power->qmp); + power->qmp = NULL; +} + +/* Control register retention on power collapse */ +void ipa_power_retention(struct ipa *ipa, bool enable) +{ + static const char fmt[] = "{ class: bcm, res: ipa_pc, val: %c }"; + struct ipa_power *power = ipa->power; + char buf[36]; /* Exactly enough for fmt[]; size a multiple of 4 */ + int ret; + + if (!power->qmp) + return; /* Not needed on this platform */ + + (void)snprintf(buf, sizeof(buf), fmt, enable ? '1' : '0'); + + ret = qmp_send(power->qmp, buf, sizeof(buf)); + if (ret) + dev_err(power->dev, "error %d sending QMP %sable request\n", + ret, enable ? "en" : "dis"); +} + int ipa_power_setup(struct ipa *ipa) { int ret; @@ -438,12 +483,18 @@ ipa_power_init(struct device *dev, const struct ipa_power_data *data) if (ret) goto err_kfree; + ret = ipa_power_retention_init(power); + if (ret) + goto err_interconnect_exit; + pm_runtime_set_autosuspend_delay(dev, IPA_AUTOSUSPEND_DELAY); pm_runtime_use_autosuspend(dev); pm_runtime_enable(dev); return power; +err_interconnect_exit: + ipa_interconnect_exit(power); err_kfree: kfree(power); err_clk_put: @@ -460,6 +511,7 @@ void ipa_power_exit(struct ipa_power *power) pm_runtime_disable(dev); pm_runtime_dont_use_autosuspend(dev); + ipa_power_retention_exit(power); ipa_interconnect_exit(power); kfree(power); clk_put(clk); diff --git a/drivers/net/ipa/ipa_power.h b/drivers/net/ipa/ipa_power.h index 2151805d7fbb0..6f84f057a2095 100644 --- a/drivers/net/ipa/ipa_power.h +++ b/drivers/net/ipa/ipa_power.h @@ -40,6 +40,13 @@ void ipa_power_modem_queue_wake(struct ipa *ipa); */ void ipa_power_modem_queue_active(struct ipa *ipa); +/** + * ipa_power_retention() - Control register retention on power collapse + * @ipa: IPA pointer + * @enable: Whether retention should be enabled or disabled + */ +void ipa_power_retention(struct ipa *ipa, bool enable); + /** * ipa_power_setup() - Set up IPA power management * @ipa: IPA pointer diff --git a/drivers/net/ipa/ipa_uc.c b/drivers/net/ipa/ipa_uc.c index 856e55a080a7f..fe11910518d95 100644 --- a/drivers/net/ipa/ipa_uc.c +++ b/drivers/net/ipa/ipa_uc.c @@ -11,6 +11,7 @@ #include "ipa.h" #include "ipa_uc.h" +#include "ipa_power.h" /** * DOC: The IPA embedded microcontroller @@ -154,6 +155,7 @@ static void ipa_uc_response_hdlr(struct ipa *ipa, enum ipa_irq_id irq_id) case IPA_UC_RESPONSE_INIT_COMPLETED: if (ipa->uc_powered) { ipa->uc_loaded = true; + ipa_power_retention(ipa, true); pm_runtime_mark_last_busy(dev); (void)pm_runtime_put_autosuspend(dev); ipa->uc_powered = false; @@ -184,6 +186,9 @@ void ipa_uc_deconfig(struct ipa *ipa) ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_UC_1); ipa_interrupt_remove(ipa->interrupt, IPA_IRQ_UC_0); + if (ipa->uc_loaded) + ipa_power_retention(ipa, false); + if (!ipa->uc_powered) return;
In some cases, the IPA hardware needs to request the always-on subsystem (AOSS) to coordinate with the IPA microcontroller to retain IPA register values at power collapse. This is done by issuing a QMP request to the AOSS microcontroller. A similar request ondoes that request. We must get and hold the "QMP" handle early, because we might get back EPROBE_DEFER for that. But the actual request should be sent while we know the IPA clock is active, and when we know the microcontroller is operational. Fixes: 2775cbc5afeb6 ("net: ipa: rename "ipa_clock.c"") Signed-off-by: Alex Elder <elder@linaro.org> --- drivers/net/ipa/ipa_power.c | 52 +++++++++++++++++++++++++++++++++++++ drivers/net/ipa/ipa_power.h | 7 +++++ drivers/net/ipa/ipa_uc.c | 5 ++++ 3 files changed, 64 insertions(+)