| Message ID | 20220228172528.3489-4-srinivas.kandagatla@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soundwire: qcom: add pm runtime support | expand |
> @@ -1424,6 +1464,11 @@ static int swrm_runtime_resume(struct device *dev) > struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev); > int ret; > > + if (ctrl->wake_irq > 0) { > + if (!irqd_irq_disabled(irq_get_irq_data(ctrl->wake_irq))) > + disable_irq_nosync(ctrl->wake_irq); > + } > + > clk_prepare_enable(ctrl->hclk); This one is quite interesting. If you disable the IRQ mechanism but haven't yet resumed the clock, that leaves a time window where the peripheral could attempt to drive the line high. what happens in that case? > > if (ctrl->clock_stop_not_supported) { > @@ -1491,6 +1536,11 @@ static int __maybe_unused swrm_runtime_suspend(struct device *dev) > > usleep_range(300, 305); > > + if (ctrl->wake_irq > 0) { > + if (irqd_irq_disabled(irq_get_irq_data(ctrl->wake_irq))) > + enable_irq(ctrl->wake_irq); > + } > + and this one is similar, you could have a case where the peripheral signals a wake immediately after the ClockStopNow frame, but you may not yet have enabled the wake detection interrupt. Would that imply that the wake is missed? > return 0; > } >
On 28/02/2022 18:01, Pierre-Louis Bossart wrote: > >> @@ -1424,6 +1464,11 @@ static int swrm_runtime_resume(struct device *dev) >> struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev); >> int ret; >> >> + if (ctrl->wake_irq > 0) { >> + if (!irqd_irq_disabled(irq_get_irq_data(ctrl->wake_irq))) >> + disable_irq_nosync(ctrl->wake_irq); >> + } >> + >> clk_prepare_enable(ctrl->hclk); > > This one is quite interesting. If you disable the IRQ mechanism but > haven't yet resumed the clock, that leaves a time window where the > peripheral could attempt to drive the line high. what happens in that case? We did call pm_runtime_get_sync() from Wake IRQ handler, which means that resume should be finished as part of Wake IRQ handler. Any new Interrupt conditions/status generated by slave in the meantime will be cleared while handling SLAVE PEND interrupt. > >> >> if (ctrl->clock_stop_not_supported) { >> @@ -1491,6 +1536,11 @@ static int __maybe_unused swrm_runtime_suspend(struct device *dev) >> >> usleep_range(300, 305); >> >> + if (ctrl->wake_irq > 0) { >> + if (irqd_irq_disabled(irq_get_irq_data(ctrl->wake_irq))) >> + enable_irq(ctrl->wake_irq); >> + } >> + > > and this one is similar, you could have a case where the peripheral > signals a wake immediately after the ClockStopNow frame, but you may not > yet have enabled the wake detection interrupt. > > Would that imply that the wake is missed? Its Possible it might be missed at that instance, however as the Slave interrupt source condition/status (Ex: button Press) is still not cleared it should generate a Wake interrupt as soon as its enabled. --srini > > > >> return 0; >> } >>
On 3/1/22 05:13, Srinivas Kandagatla wrote: > > > On 28/02/2022 18:01, Pierre-Louis Bossart wrote: >> >>> @@ -1424,6 +1464,11 @@ static int swrm_runtime_resume(struct device >>> *dev) >>> struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev); >>> int ret; >>> + if (ctrl->wake_irq > 0) { >>> + if (!irqd_irq_disabled(irq_get_irq_data(ctrl->wake_irq))) >>> + disable_irq_nosync(ctrl->wake_irq); >>> + } >>> + >>> clk_prepare_enable(ctrl->hclk); >> >> This one is quite interesting. If you disable the IRQ mechanism but >> haven't yet resumed the clock, that leaves a time window where the >> peripheral could attempt to drive the line high. what happens in that >> case? > > > We did call pm_runtime_get_sync() from Wake IRQ handler, which means > that resume should be finished as part of Wake IRQ handler. Any new > Interrupt conditions/status generated by slave in the meantime will be > cleared while handling SLAVE PEND interrupt. > >> >>> if (ctrl->clock_stop_not_supported) { >>> @@ -1491,6 +1536,11 @@ static int __maybe_unused >>> swrm_runtime_suspend(struct device *dev) >>> usleep_range(300, 305); >>> + if (ctrl->wake_irq > 0) { >>> + if (irqd_irq_disabled(irq_get_irq_data(ctrl->wake_irq))) >>> + enable_irq(ctrl->wake_irq); >>> + } >>> + >> >> and this one is similar, you could have a case where the peripheral >> signals a wake immediately after the ClockStopNow frame, but you may not >> yet have enabled the wake detection interrupt. >> >> Would that imply that the wake is missed? > Its Possible it might be missed at that instance, however as the Slave > interrupt source condition/status (Ex: button Press) is still not > cleared it should generate a Wake interrupt as soon as its enabled. ok, thanks for the answers - both make sense.
On 2/28/22 11:25, Srinivas Kandagatla wrote: > Some of the Qualcomm SoundWire Controller instances like the ones that are > connected to RX path along with Headset connections support Waking up > Controller from Low power clock stop state using SoundWire In-band interrupt. > SoundWire Slave on the bus would initiate this by pulling the data line high, > while the clock is stopped. > > Add support to this wake up interrupt. > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/soundwire/qcom.c | 50 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c > index 810232686196..e893aee1b057 100644 > --- a/drivers/soundwire/qcom.c > +++ b/drivers/soundwire/qcom.c > @@ -14,6 +14,7 @@ > #include <linux/pm_runtime.h> > #include <linux/regmap.h> > #include <linux/slab.h> > +#include <linux/pm_wakeirq.h> > #include <linux/slimbus.h> > #include <linux/soundwire/sdw.h> > #include <linux/soundwire/sdw_registers.h> > @@ -154,6 +155,7 @@ struct qcom_swrm_ctrl { > u8 rd_cmd_id; > int irq; > unsigned int version; > + int wake_irq; > int num_din_ports; > int num_dout_ports; > int cols_index; > @@ -503,6 +505,31 @@ static int qcom_swrm_enumerate(struct sdw_bus *bus) > return 0; > } > > +static irqreturn_t qcom_swrm_wake_irq_handler(int irq, void *dev_id) > +{ > + struct qcom_swrm_ctrl *swrm = dev_id; > + int ret; > + > + ret = pm_runtime_get_sync(swrm->dev); > + if (ret < 0 && ret != -EACCES) { > + dev_err_ratelimited(swrm->dev, > + "pm_runtime_get_sync failed in %s, ret %d\n", > + __func__, ret); > + pm_runtime_put_noidle(swrm->dev); > + } > + > + if (swrm->wake_irq > 0) { > + if (!irqd_irq_disabled(irq_get_irq_data(swrm->wake_irq))) > + disable_irq_nosync(swrm->wake_irq); > + } > + > + pm_runtime_mark_last_busy(swrm->dev); > + pm_runtime_put_autosuspend(swrm->dev); > + > + return IRQ_HANDLED; > +} > + > + > static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id) > { > struct qcom_swrm_ctrl *swrm = dev_id; > @@ -1340,6 +1367,19 @@ static int qcom_swrm_probe(struct platform_device *pdev) > goto err_clk; > } > > + ctrl->wake_irq = of_irq_get(dev->of_node, 1); > + if (ctrl->wake_irq > 0) { > + ret = devm_request_threaded_irq(dev, ctrl->wake_irq, NULL, > + qcom_swrm_wake_irq_handler, > + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > + "swr_wake_irq", ctrl); > + if (ret) { > + dev_err(dev, "Failed to request soundwire wake irq\n"); > + goto err_init; > + } > + } > + > + > ret = sdw_bus_master_add(&ctrl->bus, dev, dev->fwnode); > if (ret) { > dev_err(dev, "Failed to register Soundwire controller (%d)\n", > @@ -1424,6 +1464,11 @@ static int swrm_runtime_resume(struct device *dev) > struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev); > int ret; > > + if (ctrl->wake_irq > 0) { > + if (!irqd_irq_disabled(irq_get_irq_data(ctrl->wake_irq))) > + disable_irq_nosync(ctrl->wake_irq); > + } > + > clk_prepare_enable(ctrl->hclk); > > if (ctrl->clock_stop_not_supported) { > @@ -1491,6 +1536,11 @@ static int __maybe_unused swrm_runtime_suspend(struct device *dev) > > usleep_range(300, 305); > > + if (ctrl->wake_irq > 0) { > + if (irqd_irq_disabled(irq_get_irq_data(ctrl->wake_irq))) > + enable_irq(ctrl->wake_irq); > + } > + > return 0; > } >
>> +static irqreturn_t qcom_swrm_wake_irq_handler(int irq, void *dev_id) >> +{ >> + struct qcom_swrm_ctrl *swrm = dev_id; >> + int ret; >> + >> + ret = pm_runtime_get_sync(swrm->dev); >> + if (ret < 0 && ret != -EACCES) { >> + dev_err_ratelimited(swrm->dev, >> + "pm_runtime_get_sync failed in %s, ret %d\n", >> + __func__, ret); >> + pm_runtime_put_noidle(swrm->dev); missing 'return ret' here as well, is this intentional? Fix at https://github.com/thesofproject/linux/pull/3602/commits/6353eec8dc971c5f0fda0166ae1777f71784ea32 ready to go, but not sure what the intent was. >> + } >> + >> + if (swrm->wake_irq > 0) { >> + if (!irqd_irq_disabled(irq_get_irq_data(swrm->wake_irq))) >> + disable_irq_nosync(swrm->wake_irq); >> + } >> + >> + pm_runtime_mark_last_busy(swrm->dev); >> + pm_runtime_put_autosuspend(swrm->dev); >> + >> + return IRQ_HANDLED; >> +}
diff --git a/drivers/soundwire/qcom.c b/drivers/soundwire/qcom.c index 810232686196..e893aee1b057 100644 --- a/drivers/soundwire/qcom.c +++ b/drivers/soundwire/qcom.c @@ -14,6 +14,7 @@ #include <linux/pm_runtime.h> #include <linux/regmap.h> #include <linux/slab.h> +#include <linux/pm_wakeirq.h> #include <linux/slimbus.h> #include <linux/soundwire/sdw.h> #include <linux/soundwire/sdw_registers.h> @@ -154,6 +155,7 @@ struct qcom_swrm_ctrl { u8 rd_cmd_id; int irq; unsigned int version; + int wake_irq; int num_din_ports; int num_dout_ports; int cols_index; @@ -503,6 +505,31 @@ static int qcom_swrm_enumerate(struct sdw_bus *bus) return 0; } +static irqreturn_t qcom_swrm_wake_irq_handler(int irq, void *dev_id) +{ + struct qcom_swrm_ctrl *swrm = dev_id; + int ret; + + ret = pm_runtime_get_sync(swrm->dev); + if (ret < 0 && ret != -EACCES) { + dev_err_ratelimited(swrm->dev, + "pm_runtime_get_sync failed in %s, ret %d\n", + __func__, ret); + pm_runtime_put_noidle(swrm->dev); + } + + if (swrm->wake_irq > 0) { + if (!irqd_irq_disabled(irq_get_irq_data(swrm->wake_irq))) + disable_irq_nosync(swrm->wake_irq); + } + + pm_runtime_mark_last_busy(swrm->dev); + pm_runtime_put_autosuspend(swrm->dev); + + return IRQ_HANDLED; +} + + static irqreturn_t qcom_swrm_irq_handler(int irq, void *dev_id) { struct qcom_swrm_ctrl *swrm = dev_id; @@ -1340,6 +1367,19 @@ static int qcom_swrm_probe(struct platform_device *pdev) goto err_clk; } + ctrl->wake_irq = of_irq_get(dev->of_node, 1); + if (ctrl->wake_irq > 0) { + ret = devm_request_threaded_irq(dev, ctrl->wake_irq, NULL, + qcom_swrm_wake_irq_handler, + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, + "swr_wake_irq", ctrl); + if (ret) { + dev_err(dev, "Failed to request soundwire wake irq\n"); + goto err_init; + } + } + + ret = sdw_bus_master_add(&ctrl->bus, dev, dev->fwnode); if (ret) { dev_err(dev, "Failed to register Soundwire controller (%d)\n", @@ -1424,6 +1464,11 @@ static int swrm_runtime_resume(struct device *dev) struct qcom_swrm_ctrl *ctrl = dev_get_drvdata(dev); int ret; + if (ctrl->wake_irq > 0) { + if (!irqd_irq_disabled(irq_get_irq_data(ctrl->wake_irq))) + disable_irq_nosync(ctrl->wake_irq); + } + clk_prepare_enable(ctrl->hclk); if (ctrl->clock_stop_not_supported) { @@ -1491,6 +1536,11 @@ static int __maybe_unused swrm_runtime_suspend(struct device *dev) usleep_range(300, 305); + if (ctrl->wake_irq > 0) { + if (irqd_irq_disabled(irq_get_irq_data(ctrl->wake_irq))) + enable_irq(ctrl->wake_irq); + } + return 0; }
Some of the Qualcomm SoundWire Controller instances like the ones that are connected to RX path along with Headset connections support Waking up Controller from Low power clock stop state using SoundWire In-band interrupt. SoundWire Slave on the bus would initiate this by pulling the data line high, while the clock is stopped. Add support to this wake up interrupt. Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> --- drivers/soundwire/qcom.c | 50 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)