[PATCH][v3] drivers/memory: Add deep sleep support for IFC
Leo Li
pku.leo at gmail.com
Fri Apr 15 09:07:30 AEST 2016
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 <oss at buserror.net> wrote:
> On Wed, 2016-02-17 at 14:40 +0000, Raghav Dogra wrote:
>>
>> > -----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:
>> > >
>> > > +
>> > > + 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?
>
> 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.
>
>> > >
>> > > + 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.
>
> Huh? &ifc->ifc_nand.nand_evter_intr_en and such are iomem.
>
>> > > +
>> > > +/*
>> > > + * 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.
>
> So then we don't need to save/restore 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.
>
> 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.
>
>> > >
>> > > +
>> > > + 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
>
> 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.
>
>
>> > > +
>> > > + 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.
>
>> > 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.
>
> 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 at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
--
- Leo
More information about the Linuxppc-dev
mailing list