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>
---
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(+)
@@ -309,6 +312,163 @@ err:returnret;}+#ifdef CONFIG_PM_SLEEP+/* save ifc registers */+staticintfsl_ifc_suspend(structdevice*dev)+{+structfsl_ifc_ctrl*ctrl=dev_get_drvdata(dev);+structfsl_ifc_regs__iomem*ifc=ctrl->regs;+__be32nand_evter_intr_en,cm_evter_intr_en,nor_evter_intr_en,+gpcm_evter_intr_en;++ctrl->saved_regs=kzalloc(sizeof(structfsl_ifc_regs),GFP_KERNEL);+if(!ctrl->saved_regs)+return-ENOMEM;++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);+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(structfsl_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;++return0;+}++/* restore ifc registers */+staticintfsl_ifc_resume(structdevice*dev)+{+structfsl_ifc_ctrl*ctrl=dev_get_drvdata(dev);+structfsl_ifc_regs__iomem*ifc=ctrl->regs;+structfsl_ifc_regs*savd_regs=ctrl->saved_regs;+uint32_tver=0,ncfgr,timeout,ifc_bank,i;++/*+*IFCinterruptsdisabled+*/+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);+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);++/*+*IFCcontrollerNANDmachineregisters+*/+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);++/*+*IFCcontrollerNORmachineregisters+*/+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);++/*+*IFCcontrollerGPCMMachineregisters+*/+ifc_out32(savd_regs->ifc_gpcm.gpcm_evter_en,+&ifc->ifc_gpcm.gpcm_evter_en);++++/*+*IFCinterruptsenabled+*/+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;+}++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--;+}++if(!timeout)+dev_err(ctrl->dev,"Timeout waiting for IFC SRAM INIT");+}++return0;+}+#endif /* CONFIG_PM_SLEEP */+staticconststructof_device_idfsl_ifc_match[]={{.compatible="fsl,ifc",
@@ -842,6 +844,10 @@ struct fsl_ifc_ctrl {u32nand_stat;wait_queue_head_tnand_wait;boollittle_endian;+#ifdef CONFIG_PM_SLEEP+/* save regs when system go to deep-sleep */+structfsl_ifc_regs*saved_regs;+#endif};externstructfsl_ifc_ctrl*fsl_ifc_ctrl_dev;
From: Scott Wood <oss@buserror.net> Date: 2016-02-16 08:35:00
On Mon, 2016-02-15 at 11:44 +0530, Raghav Dogra wrote:
quoted hunk
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>
---
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(+)
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).
Why didn't you use the memcpy_fromio() to save these, and clear intr_en later?
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.
Align continuation lines the way patchwork suggests ("&ifc" aligned with
"savd").
Does resume from deep sleep go via U-Boot (which would initialize these
registers) on these chips?
+
+ 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?
Is it really necessary to spin here rather than waiting for an interrupt like
normal?
+
+ 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?
Also, how did Linux and U-Boot end up having opposite argument ordering for
ifc_out32()? :-(
-Scott
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
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).
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?
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.
++ 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
From: Scott Wood <oss@buserror.net> Date: 2016-02-18 01:11:35
On Wed, 2016-02-17 at 17:19 -0600, Leo Li wrote:
On Wed, Feb 17, 2016 at 8:40 AM, Raghav Dogra [off-list ref] wrote:
quoted
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.
I didn't say to return from the resume() function before the operation is
done, just to have the resume() function wait for the interrupt. At the very
least it would make it easier to reuse existing code once this is moved to the
NAND driver, if we don't need a special way of waiting for this operation.
-Scott
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?
We also want to avoid potential failures here. We can also keep the code
simpler by embedding this into the ctrl struct itself, and not dynamically
allocating it at all.
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.
I don't follow. Neither of those registers reports errors, and the registers
that *do* report errors are generally w1c and thus you can't save/restore
them.
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
What does ITR mean? And my complaint was not just about naming -- this type
of delay loop is inherently unpredictable. Future chips might go through
100,000 loops a lot faster than current chips. Use a timeout that reflects
actual time.
quoted
quoted
+
+ 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.
That's a lousy excuse for making the code look like only >= 1.3.0 needs SRAM
init. BTW, we should be doing this SRAM init on regular boot as well, since
we shouldn't rely on it having happened in the bootloader.
quoted
Also, how did Linux and U-Boot end up having opposite argument ordering
for ifc_out32()? :-(
-Scott
Hi Raghav,
Are we planning to send a new version of this patch? Btw, I see that
the current patch covers NOR/GPCM related registers, but the driver is
only built when IFC NAND is enabled right now. Do we want to change
the Kconfig to make it not depending on NAND?
Regards,
Leo
On Wed, Feb 17, 2016 at 7:19 PM, Scott Wood [off-list ref] wrote:
On Wed, 2016-02-17 at 14:40 +0000, Raghav Dogra wrote:
quoted
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:
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?
We also want to avoid potential failures here. We can also keep the code
simpler by embedding this into the ctrl struct itself, and not dynamically
allocating it at all.
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.
I don't follow. Neither of those registers reports errors, and the registers
that *do* report errors are generally w1c and thus you can't save/restore
them.
quoted
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
What does ITR mean? And my complaint was not just about naming -- this type
of delay loop is inherently unpredictable. Future chips might go through
100,000 loops a lot faster than current chips. Use a timeout that reflects
actual time.
quoted
quoted
quoted
+
+ 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.
That's a lousy excuse for making the code look like only >= 1.3.0 needs SRAM
init. BTW, we should be doing this SRAM init on regular boot as well, since
we shouldn't rely on it having happened in the bootloader.
quoted
quoted
Also, how did Linux and U-Boot end up having opposite argument ordering
for ifc_out32()? :-(
-Scott
I'm not suggesting that it be changed now -- just grumbling, and hoping that
we're more careful next time.
-Scott
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev