lkml.org 
[lkml]   [2020]   [Jul]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH V2 1/1] irqchip: imx-intmux: implement intmux PM
Date

> -----Original Message-----
> From: Marc Zyngier <maz@kernel.org>
> Sent: 2020年7月25日 21:05
> To: Joakim Zhang <qiangqing.zhang@nxp.com>
> Cc: tglx@linutronix.de; jason@lakedaemon.net; shawnguo@kernel.org;
> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
> dl-linux-imx <linux-imx@nxp.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH V2 1/1] irqchip: imx-intmux: implement intmux PM
>
> On Mon, 20 Jul 2020 11:42:37 +0100,
> Joakim Zhang <qiangqing.zhang@nxp.com> wrote:
> >
> > When system suspended, we could explicitly disable clock to save power.
> > And we need save registers' state since it could be lost after power
> > off.
> >
> > Implement PM which will:
> > 1) Without CONFIG_PM, clock is always on after probe stage.
> > 2) With CONFIG_PM, clock is off after probe stage.
> > 3) Disable clock and save registers' state when do system suspend and
> > enable clock and restore registers' state while system resume.
> > 4) Make Power Domain framework be able to shutdown the corresponding
> > power domain of this device.
> >
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> > ---
> > drivers/irqchip/irq-imx-intmux.c | 70
> > +++++++++++++++++++++++++++++++-
> > 1 file changed, 68 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/irqchip/irq-imx-intmux.c
> > b/drivers/irqchip/irq-imx-intmux.c
> > index c27577c81126..5971603cc607 100644
> > --- a/drivers/irqchip/irq-imx-intmux.c
> > +++ b/drivers/irqchip/irq-imx-intmux.c
> > @@ -53,6 +53,7 @@
> > #include <linux/of_irq.h>
> > #include <linux/of_platform.h>
> > #include <linux/spinlock.h>
> > +#include <linux/pm_runtime.h>
> >
> > #define CHANIER(n) (0x10 + (0x40 * n))
> > #define CHANIPR(n) (0x20 + (0x40 * n))
> > @@ -60,6 +61,7 @@
> > #define CHAN_MAX_NUM 0x8
> >
> > struct intmux_irqchip_data {
> > + struct irq_chip chip;
> > int chanidx;
> > int irq;
> > struct irq_domain *domain;
> > @@ -70,6 +72,7 @@ struct intmux_data {
> > void __iomem *regs;
> > struct clk *ipg_clk;
> > int channum;
> > + u32 *saved_reg;
> > struct intmux_irqchip_data irqchip_data[];
> > };
> >
> > @@ -120,8 +123,10 @@ static struct irq_chip imx_intmux_irq_chip = {
> > static int imx_intmux_irq_map(struct irq_domain *h, unsigned int irq,
> > irq_hw_number_t hwirq)
> > {
> > - irq_set_chip_data(irq, h->host_data);
> > - irq_set_chip_and_handler(irq, &imx_intmux_irq_chip, handle_level_irq);
> > + struct intmux_irqchip_data *data = h->host_data;
> > +
> > + irq_set_chip_data(irq, data);
> > + irq_set_chip_and_handler(irq, &data->chip, handle_level_irq);
> >
> > return 0;
> > }
> > @@ -232,6 +237,19 @@ static int imx_intmux_probe(struct platform_device
> *pdev)
> > data->channum = channum;
> > raw_spin_lock_init(&data->lock);
> >
> > + if (IS_ENABLED(CONFIG_PM)) {
> > + /* save CHANIER register */
> > + data->saved_reg = devm_kzalloc(&pdev->dev,
> > + sizeof(unsigned int) * channum,
>
> This isn't consistent with the type of data->saved_reg. Consider using
> sizeof(*data->saved_reg), which is guaranteed to be the right type.

Sorry for my mistake, I forgot to modify this unit.

> It also begs the question: since this saved_reg array is allocated on a per
> channel basis, why don't you have a per-channel additional u32 in the
> intmux_irqchip_data structure instead? This would sidestep this extra
> allocation altogether.

Yes, this make sense. Thanks.

Best Regards,
Joakim Zhang
> Thanks,
>
> M.
>
> --
> Without deviation from the norm, progress is not possible.
\
 
 \ /
  Last update: 2020-07-27 04:46    [W:0.324 / U:0.300 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site