| Message ID | 20200723074314.3304-3-horia.geanta@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | input: i.MX snvs powerkey updates | expand |
Hi, On 20-07-23 10:43, Horia Geantă wrote: > From: André Draszik <git@andred.net> > > At the moment, enabling this driver without the SNVS RTC driver > being active will hang the kernel as soon as the power button > is pressed. > > The reason is that in that case the SNVS isn't enabled, and > any attempt to read the SNVS registers will simply hang forever. > > Ensure the clock is enabled (during the interrupt handler) to > make this driver work. > > Also see commit 7f8993995410 ("drivers/rtc/rtc-snvs: add clock support") > and commit edb190cb1734 > ("rtc: snvs: make sure clock is enabled for interrupt handle") > for similar updates to the snvs rtc driver. > > Signed-off-by: André Draszik <git@andred.net> > Reviewed-by: Horia Geantă <horia.geanta@nxp.com> > Signed-off-by: Horia Geantă <horia.geanta@nxp.com> > --- > drivers/input/keyboard/snvs_pwrkey.c | 28 +++++++++++++++++++++++++++- > 1 file changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c > index 2f5e3ab5ed63..382d2ae82c9b 100644 > --- a/drivers/input/keyboard/snvs_pwrkey.c > +++ b/drivers/input/keyboard/snvs_pwrkey.c > @@ -16,6 +16,7 @@ > #include <linux/of_address.h> > #include <linux/platform_device.h> > #include <linux/pm_wakeirq.h> > +#include <linux/clk.h> > #include <linux/mfd/syscon.h> > #include <linux/regmap.h> > > @@ -38,6 +39,7 @@ struct pwrkey_drv_data { > int wakeup; > struct timer_list check_timer; > struct input_dev *input; > + struct clk *clk; > u8 minor_rev; > }; > > @@ -47,7 +49,10 @@ static void imx_imx_snvs_check_for_events(struct timer_list *t) > struct input_dev *input = pdata->input; > u32 state; > > + clk_enable(pdata->clk); > regmap_read(pdata->snvs, SNVS_HPSR_REG, &state); > + clk_disable(pdata->clk); > + > state = state & SNVS_HPSR_BTN ? 1 : 0; > > /* only report new event if status changed */ > @@ -74,11 +79,13 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id) > > pm_wakeup_event(input->dev.parent, 0); > > + clk_enable(pdata->clk); > + > regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status); > if (lp_status & SNVS_LPSR_SPO) { > if (pdata->minor_rev == 0) { > /* > - * The first generation i.MX6 SoCs only sends an > + * The first generation i.MX[6|7] SoCs only send an > * interrupt on button release. To mimic power-key > * usage, we'll prepend a press event. > */ > @@ -96,6 +103,8 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id) > /* clear SPO status */ > regmap_write(pdata->snvs, SNVS_LPSR_REG, SNVS_LPSR_SPO); > > + clk_disable(pdata->clk); I'm not so happy about clk_enable/disable() during the interrupt routine since those routines should be handled fast. Since we assume that the clock is always on I would rather call clk_prepare_enable() during probe() and keep the clock on. > + > return IRQ_HANDLED; > } > > @@ -140,6 +149,23 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev) > if (pdata->irq < 0) > return -EINVAL; > > + pdata->clk = devm_clk_get_optional(&pdev->dev, "snvs-pwrkey"); > + if (IS_ERR(pdata->clk)) > + return PTR_ERR(pdata->clk); > + > + error = clk_prepare(pdata->clk); > + if (error) { > + dev_err(&pdev->dev, "failed to prepare the snvs clock\n"); > + return error; > + } > + error = devm_add_action_or_reset(&pdev->dev, > + (void(*)(void *))clk_unprepare, > + pdata->clk); I'm not a fan about those casts. However, the intentation should be fixed. Regards, Marco > + if (error) { > + dev_err(&pdev->dev, "failed to add reset action on 'snvs-pwrkey'"); > + return error; > + } > + > regmap_read(pdata->snvs, SNVS_HPVIDR1_REG, &vid); > pdata->minor_rev = vid & 0xff;
On Wed, Jul 29, 2020 at 09:33:23AM +0200, Marco Felsch wrote: > Hi, > > On 20-07-23 10:43, Horia Geantă wrote: > > From: André Draszik <git@andred.net> > > > > At the moment, enabling this driver without the SNVS RTC driver > > being active will hang the kernel as soon as the power button > > is pressed. > > > > The reason is that in that case the SNVS isn't enabled, and > > any attempt to read the SNVS registers will simply hang forever. > > > > Ensure the clock is enabled (during the interrupt handler) to > > make this driver work. > > > > Also see commit 7f8993995410 ("drivers/rtc/rtc-snvs: add clock support") > > and commit edb190cb1734 > > ("rtc: snvs: make sure clock is enabled for interrupt handle") > > for similar updates to the snvs rtc driver. > > > > Signed-off-by: André Draszik <git@andred.net> > > Reviewed-by: Horia Geantă <horia.geanta@nxp.com> > > Signed-off-by: Horia Geantă <horia.geanta@nxp.com> > > --- > > drivers/input/keyboard/snvs_pwrkey.c | 28 +++++++++++++++++++++++++++- > > 1 file changed, 27 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c > > index 2f5e3ab5ed63..382d2ae82c9b 100644 > > --- a/drivers/input/keyboard/snvs_pwrkey.c > > +++ b/drivers/input/keyboard/snvs_pwrkey.c > > @@ -16,6 +16,7 @@ > > #include <linux/of_address.h> > > #include <linux/platform_device.h> > > #include <linux/pm_wakeirq.h> > > +#include <linux/clk.h> > > #include <linux/mfd/syscon.h> > > #include <linux/regmap.h> > > > > @@ -38,6 +39,7 @@ struct pwrkey_drv_data { > > int wakeup; > > struct timer_list check_timer; > > struct input_dev *input; > > + struct clk *clk; > > u8 minor_rev; > > }; > > > > @@ -47,7 +49,10 @@ static void imx_imx_snvs_check_for_events(struct timer_list *t) > > struct input_dev *input = pdata->input; > > u32 state; > > > > + clk_enable(pdata->clk); > > regmap_read(pdata->snvs, SNVS_HPSR_REG, &state); > > + clk_disable(pdata->clk); > > + > > state = state & SNVS_HPSR_BTN ? 1 : 0; > > > > /* only report new event if status changed */ > > @@ -74,11 +79,13 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id) > > > > pm_wakeup_event(input->dev.parent, 0); > > > > + clk_enable(pdata->clk); > > + > > regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status); > > if (lp_status & SNVS_LPSR_SPO) { > > if (pdata->minor_rev == 0) { > > /* > > - * The first generation i.MX6 SoCs only sends an > > + * The first generation i.MX[6|7] SoCs only send an > > * interrupt on button release. To mimic power-key > > * usage, we'll prepend a press event. > > */ > > @@ -96,6 +103,8 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id) > > /* clear SPO status */ > > regmap_write(pdata->snvs, SNVS_LPSR_REG, SNVS_LPSR_SPO); > > > > + clk_disable(pdata->clk); > > I'm not so happy about clk_enable/disable() during the interrupt > routine since those routines should be handled fast. Since we assume > that the clock is always oni We do? I thought that the fact that clock might be off is the root of the problem. > I would rather call clk_prepare_enable() > during probe() and keep the clock on. clk_enable() must not sleep and is supposed to be lightweight. This interrupt I believe is supposed to be infrequent, so having it here should be fine and might save us some power. > > > + > > return IRQ_HANDLED; > > } > > > > @@ -140,6 +149,23 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev) > > if (pdata->irq < 0) > > return -EINVAL; > > > > + pdata->clk = devm_clk_get_optional(&pdev->dev, "snvs-pwrkey"); > > + if (IS_ERR(pdata->clk)) > > + return PTR_ERR(pdata->clk); > > + > > + error = clk_prepare(pdata->clk); > > + if (error) { > > + dev_err(&pdev->dev, "failed to prepare the snvs clock\n"); > > + return error; > > + } > > + error = devm_add_action_or_reset(&pdev->dev, > > + (void(*)(void *))clk_unprepare, > > + pdata->clk); > > I'm not a fan about those casts. However, the intentation should be > fixed. Yes, can we please create a proper function for this (or maybe we will finally get devm clk API?). Thanks.
Hi Dmitry, On 20-07-29 23:04, Dmitry Torokhov wrote: > On Wed, Jul 29, 2020 at 09:33:23AM +0200, Marco Felsch wrote: > > Hi, > > > > On 20-07-23 10:43, Horia Geantă wrote: > > > From: André Draszik <git@andred.net> > > > > > > At the moment, enabling this driver without the SNVS RTC driver > > > being active will hang the kernel as soon as the power button > > > is pressed. > > > > > > The reason is that in that case the SNVS isn't enabled, and > > > any attempt to read the SNVS registers will simply hang forever. > > > > > > Ensure the clock is enabled (during the interrupt handler) to > > > make this driver work. > > > > > > Also see commit 7f8993995410 ("drivers/rtc/rtc-snvs: add clock support") > > > and commit edb190cb1734 > > > ("rtc: snvs: make sure clock is enabled for interrupt handle") > > > for similar updates to the snvs rtc driver. > > > > > > Signed-off-by: André Draszik <git@andred.net> > > > Reviewed-by: Horia Geantă <horia.geanta@nxp.com> > > > Signed-off-by: Horia Geantă <horia.geanta@nxp.com> > > > --- > > > drivers/input/keyboard/snvs_pwrkey.c | 28 +++++++++++++++++++++++++++- > > > 1 file changed, 27 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c > > > index 2f5e3ab5ed63..382d2ae82c9b 100644 > > > --- a/drivers/input/keyboard/snvs_pwrkey.c > > > +++ b/drivers/input/keyboard/snvs_pwrkey.c > > > @@ -16,6 +16,7 @@ > > > #include <linux/of_address.h> > > > #include <linux/platform_device.h> > > > #include <linux/pm_wakeirq.h> > > > +#include <linux/clk.h> > > > #include <linux/mfd/syscon.h> > > > #include <linux/regmap.h> > > > > > > @@ -38,6 +39,7 @@ struct pwrkey_drv_data { > > > int wakeup; > > > struct timer_list check_timer; > > > struct input_dev *input; > > > + struct clk *clk; > > > u8 minor_rev; > > > }; > > > > > > @@ -47,7 +49,10 @@ static void imx_imx_snvs_check_for_events(struct timer_list *t) > > > struct input_dev *input = pdata->input; > > > u32 state; > > > > > > + clk_enable(pdata->clk); > > > regmap_read(pdata->snvs, SNVS_HPSR_REG, &state); > > > + clk_disable(pdata->clk); > > > + > > > state = state & SNVS_HPSR_BTN ? 1 : 0; > > > > > > /* only report new event if status changed */ > > > @@ -74,11 +79,13 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id) > > > > > > pm_wakeup_event(input->dev.parent, 0); > > > > > > + clk_enable(pdata->clk); > > > + > > > regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status); > > > if (lp_status & SNVS_LPSR_SPO) { > > > if (pdata->minor_rev == 0) { > > > /* > > > - * The first generation i.MX6 SoCs only sends an > > > + * The first generation i.MX[6|7] SoCs only send an > > > * interrupt on button release. To mimic power-key > > > * usage, we'll prepend a press event. > > > */ > > > @@ -96,6 +103,8 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id) > > > /* clear SPO status */ > > > regmap_write(pdata->snvs, SNVS_LPSR_REG, SNVS_LPSR_SPO); > > > > > > + clk_disable(pdata->clk); > > > > I'm not so happy about clk_enable/disable() during the interrupt > > routine since those routines should be handled fast. Since we assume > > that the clock is always oni > > We do? I thought that the fact that clock might be off is the root of > the problem. Currently yes else the driver would never work. I thought so at least after reading the commit message. But after checking the imx6qdl I found that the imx6qdl has no clock-gate for the SNVS module. I think Horia had Problems on a imx7 device since he adapted the comment to cover the imx7. Which makes more sense since the imx7 has a clock gate but the commit message is wrong. After checking the device-tree's in mainline all imx7 and imx8 devices specify the "snvs-pwrkey" clock so this is a new feature and they need the clock. So IMHO for those devices a clock should be required and not optional. > > I would rather call clk_prepare_enable() > > during probe() and keep the clock on. > > clk_enable() must not sleep and is supposed to be lightweight. This > interrupt I believe is supposed to be infrequent, so having it here > should be fine and might save us some power. Didn't know that clk_enable()/clk_disable() never sleeps and yes it should be infrequent triggered. I don't think that we can save a measureable amount of power since this is Low-Power IP clocked by a low-power clock. However, this was just my point of view. > > > + > > > return IRQ_HANDLED; > > > } > > > > > > @@ -140,6 +149,23 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev) > > > if (pdata->irq < 0) > > > return -EINVAL; > > > > > > + pdata->clk = devm_clk_get_optional(&pdev->dev, "snvs-pwrkey"); > > > + if (IS_ERR(pdata->clk)) > > > + return PTR_ERR(pdata->clk); > > > + > > > + error = clk_prepare(pdata->clk); > > > + if (error) { > > > + dev_err(&pdev->dev, "failed to prepare the snvs clock\n"); > > > + return error; > > > + } > > > + error = devm_add_action_or_reset(&pdev->dev, > > > + (void(*)(void *))clk_unprepare, > > > + pdata->clk); > > > > I'm not a fan about those casts. However, the intentation should be > > fixed. > > Yes, can we please create a proper function for this (or maybe we will > finally get devm clk API?). devm_clk API +1 :) Regards, Marco
diff --git a/drivers/input/keyboard/snvs_pwrkey.c b/drivers/input/keyboard/snvs_pwrkey.c index 2f5e3ab5ed63..382d2ae82c9b 100644 --- a/drivers/input/keyboard/snvs_pwrkey.c +++ b/drivers/input/keyboard/snvs_pwrkey.c @@ -16,6 +16,7 @@ #include <linux/of_address.h> #include <linux/platform_device.h> #include <linux/pm_wakeirq.h> +#include <linux/clk.h> #include <linux/mfd/syscon.h> #include <linux/regmap.h> @@ -38,6 +39,7 @@ struct pwrkey_drv_data { int wakeup; struct timer_list check_timer; struct input_dev *input; + struct clk *clk; u8 minor_rev; }; @@ -47,7 +49,10 @@ static void imx_imx_snvs_check_for_events(struct timer_list *t) struct input_dev *input = pdata->input; u32 state; + clk_enable(pdata->clk); regmap_read(pdata->snvs, SNVS_HPSR_REG, &state); + clk_disable(pdata->clk); + state = state & SNVS_HPSR_BTN ? 1 : 0; /* only report new event if status changed */ @@ -74,11 +79,13 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id) pm_wakeup_event(input->dev.parent, 0); + clk_enable(pdata->clk); + regmap_read(pdata->snvs, SNVS_LPSR_REG, &lp_status); if (lp_status & SNVS_LPSR_SPO) { if (pdata->minor_rev == 0) { /* - * The first generation i.MX6 SoCs only sends an + * The first generation i.MX[6|7] SoCs only send an * interrupt on button release. To mimic power-key * usage, we'll prepend a press event. */ @@ -96,6 +103,8 @@ static irqreturn_t imx_snvs_pwrkey_interrupt(int irq, void *dev_id) /* clear SPO status */ regmap_write(pdata->snvs, SNVS_LPSR_REG, SNVS_LPSR_SPO); + clk_disable(pdata->clk); + return IRQ_HANDLED; } @@ -140,6 +149,23 @@ static int imx_snvs_pwrkey_probe(struct platform_device *pdev) if (pdata->irq < 0) return -EINVAL; + pdata->clk = devm_clk_get_optional(&pdev->dev, "snvs-pwrkey"); + if (IS_ERR(pdata->clk)) + return PTR_ERR(pdata->clk); + + error = clk_prepare(pdata->clk); + if (error) { + dev_err(&pdev->dev, "failed to prepare the snvs clock\n"); + return error; + } + error = devm_add_action_or_reset(&pdev->dev, + (void(*)(void *))clk_unprepare, + pdata->clk); + if (error) { + dev_err(&pdev->dev, "failed to add reset action on 'snvs-pwrkey'"); + return error; + } + regmap_read(pdata->snvs, SNVS_HPVIDR1_REG, &vid); pdata->minor_rev = vid & 0xff;