| Message ID | 20180929223421.32556-1-andreas@kemnade.info (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] w1: omap: disable iclk autoidle | expand |
* Andreas Kemnade <andreas@kemnade.info> [180929 22:39]: > + * needed to disable autoidle, if system power state is too low > + * hdq transactions will not work correctly, although registers > + * are accessible. > + * According to AM/DM3730 TRM p.2879 the hwmod has to way to > + * keep iclk running during a transfer if autoidle is enabled Sounds like hdq1w is not wake-up capable and the uart is blocking deeper SoC idle states. To me it seems that you should rather just use pm_qos in the hdq1w driver to block SoC idle for the duration of transfers. We had a similar problem with audio playback glitches a while back, see commit 9834ffd1ecc3 ("ASoC: omap-mcbsp: Add PM QoS support for McBSP to prevent glitches"). See how it does pm_qos_add_request(), pm_qos_update_request() and pm_qos_remove_request(). Regards, Tony
Hi Tony, On Mon, 1 Oct 2018 07:47:45 -0700 Tony Lindgren <tony@atomide.com> wrote: > * Andreas Kemnade <andreas@kemnade.info> [180929 22:39]: > > + * needed to disable autoidle, if system power state is too low > > + * hdq transactions will not work correctly, although registers > > + * are accessible. > > + * According to AM/DM3730 TRM p.2879 the hwmod has to way to > > + * keep iclk running during a transfer if autoidle is enabled > > Sounds like hdq1w is not wake-up capable and the uart is blocking > deeper SoC idle states. To me it seems that you should rather just > use pm_qos in the hdq1w driver to block SoC idle for the duration > of transfers. > > We had a similar problem with audio playback glitches a while > back, see commit 9834ffd1ecc3 ("ASoC: omap-mcbsp: Add PM QoS > support for McBSP to prevent glitches"). See how it does > pm_qos_add_request(), pm_qos_update_request() and > pm_qos_remove_request(). > Hmm, that formula for the latency should really be commented. I experimented with that and the results were strange. latency = 100 seems not to be safe. latency = 10 seems to be safe. If there is a qos request with latency 10, power consumption increases even in the case when uarts are active by around 35mA. I do not see any such increase if I keep that autoidle bit clear and disable runtime suspend for hdq. So the qos stuff does keep active more things when needed (of course such a qos request should be removed if not needed anymore, that was just for testing). And also the required latency values are quite strange. We have at least 190µs per bit transferred if I understand things right, and we are getting an interrupt for every byte we transfer. To me it feels like doing random things to make things work. A flag in omap_hwmod_3xxx_data.c to disable iclk autoidle would feel a lot better. I will repeat my tests. BTW: It is enough to idle uart 0 and 1 to have the problem, the others can be active (well, they belong to other domains). Regards, Andreas
Hi Tony, On Mon, 1 Oct 2018 07:47:45 -0700 Tony Lindgren <tony@atomide.com> wrote: > * Andreas Kemnade <andreas@kemnade.info> [180929 22:39]: > > + * needed to disable autoidle, if system power state is too low > > + * hdq transactions will not work correctly, although registers > > + * are accessible. > > + * According to AM/DM3730 TRM p.2879 the hwmod has to way to > > + * keep iclk running during a transfer if autoidle is enabled > > Sounds like hdq1w is not wake-up capable and the uart is blocking > deeper SoC idle states. To me it seems that you should rather just > use pm_qos in the hdq1w driver to block SoC idle for the duration > of transfers. > > We had a similar problem with audio playback glitches a while > back, see commit 9834ffd1ecc3 ("ASoC: omap-mcbsp: Add PM QoS > support for McBSP to prevent glitches"). See how it does > pm_qos_add_request(), pm_qos_update_request() and > pm_qos_remove_request(). I found this interesting function: /** * _setup_iclk_autoidle - configure an IP block's interface clocks * @oh: struct omap_hwmod * * * Set up the module's interface clocks. XXX This function is still mostly * a stub; implementing this properly requires iclk autoidle usecounting in * the clock code. No return value. */ static void __init _setup_iclk_autoidle(struct omap_hwmod *oh) { struct omap_hwmod_ocp_if *os; if (oh->_state != _HWMOD_STATE_INITIALIZED) return; list_for_each_entry(os, &oh->slave_ports, node) { if (!os->_clk) continue; if (os->flags & OCPIF_SWSUP_IDLE) { /* XXX omap_iclk_deny_idle(c); */ } else { /* XXX omap_iclk_allow_idle(c); */ clk_enable(os->_clk); } } return; } So at first glance it looks like we just need to uncomment the first XXX line here. But why the heck we need usecounting for the autoidle stuff? How do I test it (if I would implement some counter)? IMHO the clean solution for the hdq problem would be to fix this function. Regards, Andreas
* Andreas Kemnade <andreas@kemnade.info> [181002 21:42]: > On Mon, 1 Oct 2018 07:47:45 -0700 > Tony Lindgren <tony@atomide.com> wrote: > > Sounds like hdq1w is not wake-up capable and the uart is blocking > > deeper SoC idle states. To me it seems that you should rather just > > use pm_qos in the hdq1w driver to block SoC idle for the duration > > of transfers. > > > > We had a similar problem with audio playback glitches a while > > back, see commit 9834ffd1ecc3 ("ASoC: omap-mcbsp: Add PM QoS > > support for McBSP to prevent glitches"). See how it does > > pm_qos_add_request(), pm_qos_update_request() and > > pm_qos_remove_request(). > > > Hmm, that formula for the latency should really be commented. > > I experimented with that and the results were strange. > latency = 100 seems not to be safe. > latency = 10 seems to be safe. FYI, the value to pick should in theory be something out of the latencies in omap3_idle_driver. But that data is for omap34xx, and 36xx is faster so we really should have separate data for 36xx. Basically you'd want to block anything that cuts off the clocks, so probably anything retention related. > If there is a qos request with latency 10, power consumption > increases even in the case when uarts are active by around 35mA. OK yeah this will block deeper idle states. > I do not see any such increase if I keep that autoidle bit clear and > disable runtime suspend for hdq. OK Regards, Tony
diff --git a/drivers/clk/ti/autoidle.c b/drivers/clk/ti/autoidle.c index 7bb9afbe4058..b8970006efd9 100644 --- a/drivers/clk/ti/autoidle.c +++ b/drivers/clk/ti/autoidle.c @@ -52,6 +52,7 @@ int omap2_clk_deny_idle(struct clk *clk) c->ops->deny_idle(c); return 0; } +EXPORT_SYMBOL(omap2_clk_deny_idle); /** * omap2_clk_allow_idle - enable autoidle on an OMAP clock @@ -68,6 +69,7 @@ int omap2_clk_allow_idle(struct clk *clk) c->ops->allow_idle(c); return 0; } +EXPORT_SYMBOL(omap2_clk_allow_idle); static void _allow_autoidle(struct clk_ti_autoidle *clk) { diff --git a/drivers/w1/masters/omap_hdq.c b/drivers/w1/masters/omap_hdq.c index 3099052e1243..e3aeba8a1155 100644 --- a/drivers/w1/masters/omap_hdq.c +++ b/drivers/w1/masters/omap_hdq.c @@ -18,6 +18,8 @@ #include <linux/sched.h> #include <linux/pm_runtime.h> #include <linux/of.h> +#include <linux/clk.h> +#include <linux/clk/ti.h> #include <linux/w1.h> @@ -59,6 +61,14 @@ MODULE_PARM_DESC(w1_id, "1-wire id for the slave detection in HDQ mode"); struct hdq_data { struct device *dev; + /* + * needed to disable autoidle, if system power state is too low + * hdq transactions will not work correctly, although registers + * are accessible. + * According to AM/DM3730 TRM p.2879 the hwmod has to way to + * keep iclk running during a transfer if autoidle is enabled + */ + struct clk *ick; void __iomem *hdq_base; /* lock status update */ struct mutex hdq_mutex; @@ -414,6 +424,9 @@ static int omap_hdq_get(struct hdq_data *hdq_data) try_module_get(THIS_MODULE); if (1 == hdq_data->hdq_usecount) { + if (!IS_ERR_OR_NULL(hdq_data->ick)) + omap2_clk_deny_idle(hdq_data->ick); + pm_runtime_get_sync(hdq_data->dev); /* make sure HDQ/1W is out of reset */ @@ -460,8 +473,11 @@ static int omap_hdq_put(struct hdq_data *hdq_data) } else { hdq_data->hdq_usecount--; module_put(THIS_MODULE); - if (0 == hdq_data->hdq_usecount) + if (hdq_data->hdq_usecount == 0) { pm_runtime_put_sync(hdq_data->dev); + if (!IS_ERR_OR_NULL(hdq_data->ick)) + omap2_clk_allow_idle(hdq_data->ick); + } } mutex_unlock(&hdq_data->hdq_mutex); @@ -681,8 +697,15 @@ static int omap_hdq_probe(struct platform_device *pdev) hdq_data->hdq_usecount = 0; hdq_data->rrw = 0; + hdq_data->ick = devm_clk_get(dev, "hdq_ick"); + if (IS_ERR_OR_NULL(hdq_data->ick)) + dev_info(dev, "no hdq_ick, lets hope autoidle behaves!"); + mutex_init(&hdq_data->hdq_mutex); + if (!IS_ERR_OR_NULL(hdq_data->ick)) + omap2_clk_deny_idle(hdq_data->ick); + pm_runtime_enable(&pdev->dev); ret = pm_runtime_get_sync(&pdev->dev); if (ret < 0) { @@ -718,6 +741,8 @@ static int omap_hdq_probe(struct platform_device *pdev) omap_hdq_break(hdq_data); pm_runtime_put_sync(&pdev->dev); + if (!IS_ERR_OR_NULL(hdq_data->ick)) + omap2_clk_allow_idle(hdq_data->ick); ret = of_property_read_string(pdev->dev.of_node, "ti,mode", &mode); if (ret < 0 || !strcmp(mode, "hdq")) {
On the gta04 in DM3730, omap_hdq gets stuck whenever autosuspend is enabled for UART1/2. The system will go into a lower power state then. According to the TRM, the module has no way to prevent the ick from being but during a transfer if autoidle is enabled. So disable autoidle. Having omap_hdq working on the gta04 is important for measuring currents through a bq27000. The question is what is the best place to do this. Perhaps better in arch/arm/mach-omap2 somehow, so no additional exported symbols are needed. But there seems to be no simple flag to set there. Maybe we need something like arch/arm/mach-omap2/mcbsp.c? And also the affected platforms need to be checked. Probably omap_hdq_get/put should also be cleaned up and stuff from there should be put into a runtime_suspend/resume handler. Signed-off-by: Andreas Kemnade <andreas@kemnade.info> --- drivers/clk/ti/autoidle.c | 2 ++ drivers/w1/masters/omap_hdq.c | 27 ++++++++++++++++++++++++++- 2 files changed, 28 insertions(+), 1 deletion(-)