[PATCH][v3] drivers/memory: Add deep sleep support for IFC
Raghav Dogra
raghav.dogra at nxp.com
Thu Feb 18 01:40:03 AEDT 2016
> -----Original Message-----
> From: Scott Wood [mailto:oss at buserror.net]
> Sent: Tuesday, February 16, 2016 2:05 PM
> To: Raghav Dogra <raghav.dogra at nxp.com>; linuxppc-dev at lists.ozlabs.org
> Cc: Prabhakar Kushwaha <prabhakar.kushwaha at nxp.com>
> Subject: Re: [PATCH][v3] drivers/memory: Add deep sleep support for IFC
>
> On Mon, 2016-02-15 at 11:44 +0530, Raghav Dogra wrote:
> > Add support of suspend, resume function to support deep sleep.
> > Also make sure of SRAM initialization during resume.
> >
> > Signed-off-by: Prabhakar Kushwaha <prabhakar.kushwaha at nxp.com>
> > Signed-off-by: Raghav Dogra <raghav.dogra at nxp.com>
> > ---
> > Changes for v3: Replace spin_event_timeout() with arch independent
> > macro
> >
> > Based on
> > git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
> > branch "master"
> >
> > drivers/memory/fsl_ifc.c | 165
> > +++++++++++++++++++++++++++++++++++++++++++++++
> > include/linux/fsl_ifc.h | 6 ++
> > 2 files changed, 171 insertions(+)
> >
> > diff --git a/drivers/memory/fsl_ifc.c b/drivers/memory/fsl_ifc.c index
> > acd1460..fa028bd 100644
> > --- a/drivers/memory/fsl_ifc.c
> > +++ b/drivers/memory/fsl_ifc.c
> > @@ -24,6 +24,7 @@
> > #include <linux/compiler.h>
> > #include <linux/sched.h>
> > #include <linux/spinlock.h>
> > +#include <linux/delay.h>
> > #include <linux/types.h>
> > #include <linux/slab.h>
> > #include <linux/io.h>
> > @@ -35,6 +36,8 @@
> >
> > struct fsl_ifc_ctrl *fsl_ifc_ctrl_dev;
> > EXPORT_SYMBOL(fsl_ifc_ctrl_dev);
> > +#define FSL_IFC_V1_3_0 0x01030000
> > +#define IFC_TIMEOUT_MSECS 100000 /* 100ms */
>
> What does the "MSECS" mean in IFC_TIMEOUT_MSECS? It's a unit without a
> quantity.
Yes, I agree. I will rename it to IFC_WAIT_ITR.
>
> >
> > /*
> > * convert_ifc_address - convert the base address @@ -309,6 +312,163
> > @@ err:
> > return ret;
> > }
> >
> > +#ifdef CONFIG_PM_SLEEP
> > +/* save ifc registers */
> > +static int fsl_ifc_suspend(struct device *dev) {
> > + struct fsl_ifc_ctrl *ctrl = dev_get_drvdata(dev);
> > + struct fsl_ifc_regs __iomem *ifc = ctrl->regs;
> > + __be32 nand_evter_intr_en, cm_evter_intr_en, nor_evter_intr_en,
> > + gpcm_evter_intr_en
> > ;
>
> s/__be32/u32/ as they've already been converted to host endianness.
>
> Also please repeat the type on a new line rather than use continuation lines
> to declare more variables (and don't indent continuation lines so far).
>
Okay, will take care of this in the next version.
> > +
> > + ctrl->saved_regs = kzalloc(sizeof(struct fsl_ifc_regs),
> > GFP_KERNEL);
> > + if (!ctrl->saved_regs)
> > + return -ENOMEM;
>
> Allocate memory at probe time, not here.
>
But, why allocate memory at the probe when it is not known at that time whether
deep sleep state would be required or not? Is that because we want to save time
while going to deep sleep?
> > + cm_evter_intr_en = ifc_in32(&ifc->cm_evter_intr_en);
> > + nand_evter_intr_en = ifc_in32(&ifc->ifc_nand.nand_evter_intr_en);
> > + nor_evter_intr_en = ifc_in32(&ifc->ifc_nor.nor_evter_intr_en);
> > + gpcm_evter_intr_en = ifc_in32(&ifc-
> >ifc_gpcm.gpcm_evter_intr_en);
> > +
> > +/* IFC interrupts disabled */
> > +
> > + ifc_out32(0x0, &ifc->cm_evter_intr_en);
>
> Indent the comments the same as the code.
>
Okay.
> > + ifc_out32(0x0, &ifc->ifc_nand.nand_evter_intr_en);
> > + ifc_out32(0x0, &ifc->ifc_nor.nor_evter_intr_en);
> > + ifc_out32(0x0, &ifc->ifc_gpcm.gpcm_evter_intr_en);
> > +
> > + memcpy_fromio(ctrl->saved_regs, ifc, sizeof(struct fsl_ifc_regs));
> > +
> > +/* save the interrupt values */
> > + ctrl->saved_regs->cm_evter_intr_en = cm_evter_intr_en;
> > + ctrl->saved_regs->ifc_nand.nand_evter_intr_en =
> nand_evter_intr_en;
> > + ctrl->saved_regs->ifc_nor.nor_evter_intr_en = nor_evter_intr_en;
> > + ctrl->saved_regs->ifc_gpcm.gpcm_evter_intr_en =
> gpcm_evter_intr_en;
>
> Why didn't you use the memcpy_fromio() to save these, and clear intr_en
> later?
>
I used it whenever I did a write/read on iomem. In this case, both memories
are non iomem.
> That said, I still don't like this approach. I'd rather see the nand driver save
> the registers it cares about, and this driver wouldn't have to do much other
> than quiesce the rest of the interrupts.
>
Okay, we will analyze the required changes and include them.
> > +
> > + return 0;
> > +}
> > +
> > +/* restore ifc registers */
> > +static int fsl_ifc_resume(struct device *dev) {
> > + struct fsl_ifc_ctrl *ctrl = dev_get_drvdata(dev);
> > + struct fsl_ifc_regs __iomem *ifc = ctrl->regs;
> > + struct fsl_ifc_regs *savd_regs = ctrl->saved_regs;
> > + uint32_t ver = 0, ncfgr, timeout, ifc_bank, i;
>
> s/savd/saved/
>
Okay.
> > +
> > +/*
> > + * IFC interrupts disabled
> > + */
> > + ifc_out32(0x0, &ifc->cm_evter_intr_en);
> > + ifc_out32(0x0, &ifc->ifc_nand.nand_evter_intr_en);
> > + ifc_out32(0x0, &ifc->ifc_nor.nor_evter_intr_en);
> > + ifc_out32(0x0, &ifc->ifc_gpcm.gpcm_evter_intr_en);
> > +
> > +
> > + if (ctrl->saved_regs) {
> > + for (ifc_bank = 0; ifc_bank < FSL_IFC_BANK_COUNT;
> > ifc_bank++) {
> > + ifc_out32(savd_regs->cspr_cs[ifc_bank].cspr_ext,
> > + &ifc->cspr_cs[ifc_bank].cspr_ext);
> > + ifc_out32(savd_regs->cspr_cs[ifc_bank].cspr,
> > + &ifc->cspr_cs[ifc_bank].cspr);
> > + ifc_out32(savd_regs->amask_cs[ifc_bank].amask,
> > + &ifc->amask_cs[ifc_bank].amask);
> > + ifc_out32(savd_regs->csor_cs[ifc_bank].csor_ext,
> > + &ifc->csor_cs[ifc_bank].csor_ext);
> > + ifc_out32(savd_regs->csor_cs[ifc_bank].csor,
> > + &ifc->csor_cs[ifc_bank].csor);
>
> Align continuation lines the way patchwork suggests ("&ifc" aligned with
> "savd").
Okay, I will take care of this in the next patch.
>
> Does resume from deep sleep go via U-Boot (which would initialize these
> registers) on these chips?
Yes, deep sleep resume goes via u-boot and these registers should be initialized
By u-boot.
>
> > + for (i = 0; i < 4; i++) {
> > + ifc_out32(savd_regs
> > ->ftim_cs[ifc_bank].ftim[i],
> > + &ifc->ftim_cs[ifc_bank].ftim[i]);
> > + }
> > + }
> > + ifc_out32(savd_regs->ifc_gcr, &ifc->ifc_gcr);
> > + ifc_out32(savd_regs->cm_evter_en, &ifc->cm_evter_en);
> > +
> > +/*
> > +* IFC controller NAND machine registers */
> > + ifc_out32(savd_regs->ifc_nand.ncfgr, &ifc->ifc_nand.ncfgr);
> > + ifc_out32(savd_regs->ifc_nand.nand_fcr0,
> > + &ifc->ifc_nand.nand_fcr0);
> > + ifc_out32(savd_regs->ifc_nand.nand_fcr1,
> > + &ifc->ifc_nand.nand_fcr1);
> > + ifc_out32(savd_regs->ifc_nand.row0, &ifc->ifc_nand.row0);
> > + ifc_out32(savd_regs->ifc_nand.row1, &ifc->ifc_nand.row1);
> > + ifc_out32(savd_regs->ifc_nand.col0, &ifc->ifc_nand.col0);
> > + ifc_out32(savd_regs->ifc_nand.col1, &ifc->ifc_nand.col1);
> > + ifc_out32(savd_regs->ifc_nand.row2, &ifc->ifc_nand.row2);
> > + ifc_out32(savd_regs->ifc_nand.col2, &ifc->ifc_nand.col2);
> > + ifc_out32(savd_regs->ifc_nand.row3, &ifc->ifc_nand.row3);
> > + ifc_out32(savd_regs->ifc_nand.col3, &ifc->ifc_nand.col3);
> > + ifc_out32(savd_regs->ifc_nand.nand_fbcr,
> > + &ifc->ifc_nand.nand_fbcr);
> > + ifc_out32(savd_regs->ifc_nand.nand_fir0,
> > + &ifc->ifc_nand.nand_fir0);
> > + ifc_out32(savd_regs->ifc_nand.nand_fir1,
> > + &ifc->ifc_nand.nand_fir1);
> > + ifc_out32(savd_regs->ifc_nand.nand_fir2,
> > + &ifc->ifc_nand.nand_fir2);
> > + ifc_out32(savd_regs->ifc_nand.nand_csel,
> > + &ifc->ifc_nand.nand_csel);
> > + ifc_out32(savd_regs->ifc_nand.nandseq_strt,
> > + &ifc
> > ->ifc_nand.nandseq_strt);
> > + ifc_out32(savd_regs->ifc_nand.nand_evter_en,
> > + &ifc
> > ->ifc_nand.nand_evter_en);
> > + ifc_out32(savd_regs->ifc_nand.nanndcr, &ifc
> > ->ifc_nand.nanndcr);
>
> Many of these are either initialized by the NAND driver for each transaction,
> or are not used at all.
>
Yes, will analyze the registers used and take care of them.
> > +
> > +/*
> > +* IFC controller NOR machine registers */
> > + ifc_out32(savd_regs->ifc_nor.nor_evter_en,
> > + &ifc->ifc_nor.nor_evter_en);
> > + ifc_out32(savd_regs->ifc_nor.norcr, &ifc->ifc_nor.norcr);
>
> What uses these?
>
These registers are not used as such, but we would like to retain their value as they
can be of help in case of error conditions.
> > +
> > +/*
> > + * IFC controller GPCM Machine registers */
> > + ifc_out32(savd_regs->ifc_gpcm.gpcm_evter_en,
> > + &ifc->ifc_gpcm.gpcm_evter_en);
> > +
> > +
> > +
> > +/*
> > + * IFC interrupts enabled
> > + */
> > + ifc_out32(ctrl->saved_regs->cm_evter_intr_en, &ifc
> > ->cm_evter_intr_en);
> > + ifc_out32(ctrl->saved_regs->ifc_nand.nand_evter_intr_en,
> > + &ifc->ifc_nand.nand_evter_intr_en);
> > + ifc_out32(ctrl->saved_regs->ifc_nor.nor_evter_intr_en,
> > + &ifc->ifc_nor.nor_evter_intr_en);
> > + ifc_out32(ctrl->saved_regs->ifc_gpcm.gpcm_evter_intr_en,
> > + &ifc-
> >ifc_gpcm.gpcm_evter_intr_en);
> > +
> > + kfree(ctrl->saved_regs);
> > + ctrl->saved_regs = NULL;
> > + }
>
> Bad indentation
>
Will take care.
> > +
> > + ver = ifc_in32(&ctrl->regs->ifc_rev);
> > + ncfgr = ifc_in32(&ifc->ifc_nand.ncfgr);
> > + if (ver >= FSL_IFC_V1_3_0) {
> > +
> > + ifc_out32(ncfgr | IFC_NAND_SRAM_INIT_EN,
> > + &ifc->ifc_nand.ncfgr);
> > + /* wait for SRAM_INIT bit to be clear or timeout */
> > + timeout = IFC_TIMEOUT_MSECS;
> > + while ((ifc_in32(&ifc->ifc_nand.ncfgr) &
> > + IFC_NAND_SRAM_INIT_EN) &&
> timeout)
> > {
> > + cpu_relax();
> > + timeout--;
> > + }
>
> How can this timeout be in milliseconds or any other real unit of time, if it's
> actually measuring loop iterations with no udelay() or similar?
>
Yes, it's not in millisecond any longer. Will change the name to IFC_WAIT_ITR
> Is it really necessary to spin here rather than waiting for an interrupt like
> normal?
>
Aren't the global interrupts disabled at this stage? Can we use the interrupt based
waits in the deep sleep code? We used it based on the assumption that interrupts
cannot be used here.
> > +
> > + if (!timeout)
> > + dev_err(ctrl->dev, "Timeout waiting for IFC SRAM
> > INIT");
> > + }
>
> U-boot does this when the version is > 1.1.0 -- why >= 1.3.0 here? Are there
> any versions in between 1.1.0 and 1.3.0?
Because only B4 and T4 are based on 1.1.0 which do not support deep sleep.
The first board which supports deep sleep is T1040 which has version 1.3.0.
>
> Also, how did Linux and U-Boot end up having opposite argument ordering
> for ifc_out32()? :-(
>
> -Scott
I guess it happened in the patch:
https://patchwork.ozlabs.org/patch/474719/
Let's keep it that way now? Changing it would require significant work.
-Raghav
More information about the Linuxppc-dev
mailing list