On Wed, Feb 17, 2016 at 8:40 AM, Raghav Dogra <raghav.do...@nxp.com> wrote: > > >> -----Original Message----- >> From: Scott Wood [mailto:o...@buserror.net] >> Sent: Tuesday, February 16, 2016 2:05 PM >> To: Raghav Dogra <raghav.do...@nxp.com>; linuxppc-dev@lists.ozlabs.org >> Cc: Prabhakar Kushwaha <prabhakar.kushw...@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.kushw...@nxp.com> >> > Signed-off-by: Raghav Dogra <raghav.do...@nxp.com>
Similar comment as last time, that we should involve the MTD guys. >> > --- >> > 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. At the resume() stage, interrupts are already enabled. But the problem of using interrupt based wait here is that we cannot give a correct return value at this point. And it can also defeat the ordering of resume() callbacks for dependent devices. Regards, Leo _______________________________________________ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev