Re: [PATCH 1/2] drivers/memory: Add deep sleep support for IFC

[Scott Wood]

On Tue, 2015-10-27 at 16:34 +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: Raghav Dogra <raghav@xxxxxxxxxxxxx>
> ---
>  drivers/memory/fsl_ifc.c | 56 
> ++++++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/fsl_ifc.h  |  6 ++++++
>  2 files changed, 62 insertions(+)

Who are you asking to apply this?  If me, please send to 
 linuxppc-dev@xxxxxxxxxxxxxxxxxx it gets on patchwork.

Also keep Prabhakar CCed on IFC patches.

> diff --git a/drivers/memory/fsl_ifc.c b/drivers/memory/fsl_ifc.c
> index e87459f..163ccf2 100644
> --- a/drivers/memory/fsl_ifc.c
> +++ b/drivers/memory/fsl_ifc.c
> @@ -23,6 +23,7 @@
>  #include <linux/kernel.h>
>  #include <linux/compiler.h>
>  #include <linux/spinlock.h>
> +#include <linux/delay.h>
>  #include <linux/types.h>
>  #include <linux/slab.h>
>  #include <linux/io.h>
> @@ -35,6 +36,9 @@
>  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 */
> +
>  /*
>   * convert_ifc_address - convert the base address
>   * @addr_base:       base address of the memory bank
> @@ -308,6 +312,53 @@ 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;
> +
> +     ctrl->saved_regs = kzalloc(sizeof(struct fsl_ifc_regs), GFP_KERNEL);
> +     if (!ctrl->saved_regs)
> +             return -ENOMEM;
> +
> +     _memcpy_fromio(ctrl->saved_regs, ifc, sizeof(struct fsl_ifc_regs));
> +
> +     return 0;
> +}

Why _memcpy_fromio() rather than memcpy_fromio()?

> +/* 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;
> +     uint32_t ver = 0, ncfgr, status;
> +
> +     if (ctrl->saved_regs) {
> +             _memcpy_toio(ifc, ctrl->saved_regs,
> +                             sizeof(struct fsl_ifc_regs));

This is too simplistic, and doesn't account for things like read-to-clear 
bits, or the possibility of reserved fields that expose internal state that 
shouldn't be messed with.  Registers that need state saving or 
reinitialization should be handled individually.

You should also make sure that interrupts are disabled at the device.

> +             kfree(ctrl->saved_regs);
> +             ctrl->saved_regs = NULL;
> +     }
> +
> +     ver = in_be32(&ctrl->regs->ifc_rev);
> +     ncfgr = in_be32(&ifc->ifc_nand.ncfgr);
> +     if (ver >= FSL_IFC_V1_3_0) {
> +             out_be32(&ifc->ifc_nand.ncfgr, ncfgr | IFC_NAND_SRAM_INIT_EN);
> +
> +             /* wait for  SRAM_INIT bit to be clear or timeout */
> +             status = spin_event_timeout(!(in_be32(&ifc->ifc_nand.ncfgr)
> +                                & IFC_NAND_SRAM_INIT_EN),
> +                                IFC_TIMEOUT_MSECS, 0);
> +             if (!status)
> +                     dev_err(ctrl->dev, "Timeout waiting for IFC SRAM INIT");
> +     }
> +
> +     return 0;
> +}
> +#endif /* CONFIG_PM_SLEEP */

Normally we do NAND SRAM init in the NAND driver.  Why are you doing it here? 
 How is SRAM going to be reset on older IFCs?

BTW, I see that Linux is missing SRAM init for IFCs newer than v1.1 -- we're 
relying on U-Boot to do so, which we shouldn't be.

-Scott

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/