[PATCH 0/9] ARM: vf610: Suspend/resume support

On Wed, Sep 24, 2014 at 12:33:00PM -0400, Bill Pringlemeir wrote:

quoted

quoted

I think that Shawn Guo already did a patchset to remove stuff from
the vf610.dtsi to the machine/configuration DT files.

Do you mean the pin configuration?  That's really more board-specific.
But IP block/device is not really the case.

In this patchset, I suggested that we could include the notation in the
headers which are included in the 'DT' files.  So instead of 'dtsi', we
could use,

   #define VF610_GPC_SUPPORT \
           gpc: gpc at 4006c000 {                \
                   compatible = "fsl,vf610-gpc"; \
                   reg = <0x4006c000 0x1000>;    \
           };

Or even,

   #define VF610_SUSPEND_DT_SUPPORT \ ...

I don't think this will be accepted by DT maintainers.  We have already
got some objections when we define pin function ID as multiple integers.
They expect the DT macro is used for single integer case.

Anyways, the DT issue doesn't matter so much.  People will have to make
custom versions for their own boards.  Although, I don't think it is not
that hard to support it at the machine level.

I agree that so far there are some peripherals that make it difficult to
run the mainline Linux with an MQX.  However, I think this patch set
seems to bring it closer to making it impossible.  I am only suggesting
that we make it a configuration option and not include it for all Vybrid
devices.

Is it clear that the desired way is to use,

   &gpc { status = "disabled"; };
   &src { status = "disabled"; };

We have,

arch/arm/mach-imx/gpc.c

      void __init vf610_gpc_init(void)
      {
              struct device_node *np;

              np = of_find_compatible_node(NULL, NULL, "fsl,vf610-gpc");
              gpc_base = of_iomap(np, 0);
              gpc_imr_base = gpc_base + VF610_GPC_IMR1;
   ...

arch/arm/mach-imx/mach-vf610.c

   static void __init vf610_init_irq(void)
   {
           vf610_gpc_init();
           irqchip_init();
   }

   ...

   +	.init_irq	= vf610_init_irq,

I don't think this will work.

Yes, you're right.  But I guess this can be fixed by an additional
of_device_is_available() check after of_find_compatible_node() call.

Not to mention that 'mach-vf610.c' will
not build if HAVE_IMX_GPC is not defined.

HAVE_IMX_GPC cannot be configured out, because it's selected by 
SOC_VF610.

Also, I don't really see a
use of the GPC module unless suspend/resume is active?   Even some wall
powered designs may wish to exclude this functionality?

I think that the SRC maybe needed for secure parts.  I think that some
designs might wish to restrict Linux's access to these registers as
well.  I don't actually see why we need this module?  I think the imx6
needs it due to multi-CPU bring-up, but in the Vybrid case, this does
not exist.  Can you check to see why we need the SRC?  I don't see where
we actually use it?  In patch9/9, we record it but do we actually access
the registers?  Is it just for the vf610_src_init() code?  Even that
seems the whole 'src.c' file is only needed for the 'src_base'
reference, which we don't use?

+1.  I'm also wondering how SRC is used by suspend routine in this
series.

Shawn