On Wed, Feb 17, 2016 at 8:40 AM, Raghav Dogra [off-list ref] wrote:

quoted

-----Original Message-----
From: Scott Wood [mailto:oss@buserror.net]
Sent: Tuesday, February 16, 2016 2:05 PM
To: Raghav Dogra <redacted>; linuxppc-dev@lists.ozlabs.org
Cc: Prabhakar Kushwaha <redacted>
Subject: Re: [PATCH][v3] drivers/memory: Add deep sleep support for IFC

On Mon, 2016-02-15 at 11:44 +0530, Raghav Dogra wrote:

quoted

Add support of suspend, resume function to support deep sleep.
Also make sure of SRAM initialization  during resume.

Signed-off-by: Prabhakar Kushwaha <redacted>
Signed-off-by: Raghav Dogra <redacted>

Similar comment as last time, that we should involve the MTD guys.

quoted

quoted

---
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.

quoted

quoted

 /*
  * 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.

quoted

quoted

+
+   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?

quoted

quoted

+   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.

quoted

quoted

+   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;

quoted

+   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.

quoted

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.

quoted

quoted

+
+   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.

quoted

quoted

+
+/*
+ * 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.

quoted

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.

quoted

quoted

+                   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.

quoted

quoted

+
+/*
+* 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.

quoted

quoted

+
+/*
+ * 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.

quoted

quoted

+
+   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)

quoted

{
+                   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

quoted

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