From: bpringlemeir@nbsps•com (Bill Pringlemeir)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH 0/9] ARM: vf610: Suspend/resume support
Date: Wed, 24 Sep 2014 12:33:00 -0400 [thread overview]
Message-ID: <87y4t9kmgz.fsf@nbsps.com> (raw)
In-Reply-To: <4ee0621b0bf5f4d4e10eae7673bdf1cd@agner.ch> (Stefan Agner's message of "Wed, 24 Sep 2014 10:22:39 +0200")
On 24 Sep 2014, stefan at agner.ch wrote:
> Am 2014-09-23 17:36, schrieb Bill Pringlemeir:
>> On 22 Sep 2014, stefan at agner.ch wrote:
[snip]
>>> Power measurement (Colibri VF61, whole module):
>>> - Idle: 540mW
>>> - LP-RUN: 220mW (standby)
>>> - STOP: 200mW (mem)
>>
>>> Stefan Agner (9):
>>> ARM: dts: vf610: Add system reset controller (SRC)
>>> ARM: dts: vf610: add global power controller (GPC)
>>> ARM: dts: vf610: add on-chip SRAM
>>
>> These above three change sets have some implications for dual-chip
>> (Cortex-A5/Cortex-M4) configurations. Epecially those running MQX.
>> There is not harm to define the register space (except DT size).
>> However, if you activate drivers that manipulate the registers for
>> all systems, then there is no choice to have MQX work on the 2nd
>> core.
> On the Timesys BSP, the kernel is fiddling around with this registers
> too, and AFAIK MQX is working with that kernel. Is MQX really using
> the GPC and SRC modules? I thought MQX is just relying on Linux taking
> care of that.
> Also, you have this problem with other registers as well, for instance
> the CCM module. In fact, to get into deeper sleep modes, you need to
> access the GPC (global power controller) as well as the CCM (clock
> controller module, for instance the CCM_CLPCR register). When you look
> at all the entry sequences, they all fiddling around with the GPC and
> the CCM. And I don't think that the kernel can work properly without
> having control over the clock module.
> IMHO, the SRC and GPC are like the CCM, and need to be under control
> of Linux exclusively.
Yes, it is difficult to see the existing Linux infrastructure working
without these. The two cores both depend on the same clocks. You must
use 'clk_ignore_unused' if you want to run both.
> Another case is the SRAM. There are other peripherals which are much
> more important, e.g. both instances of the EDMA modules are currently
> unconditional part of the device tree.
The SRAM is even more of an issue. Often the MQX is running from the
SRAM. I agree that the EDMA are actually a bit of an issue as well.
You need to modify MQX to not use them. However, the MQX use of EDMA
seems stupid, so I prefer Linux to have this feature. I didn't see
the edma patches when they went in and/or I didn't think of this until I
actually encounter the 'emda' conflict issue.
> Besides, afaik you can also use status = "disabled" in a device tree
> including the vf610.dtsi. The device tree is parsed sequential, the
> last settings wins.
This is true, but in some cases the code is depending on the device
being enabled? Also, we have automatically compiled in the drivers if
'SOC_VF610' is active. Ie, there is no way to exclude the code. The
device tree also becomes a little heavier instead of including them in
the machine file.
>> I think that Shawn Guo already did a patchset to remove stuff from
>> the vf610.dtsi to the machine/configuration DT files.
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 \ ...
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. Not to mention that 'mach-vf610.c' will
not build if HAVE_IMX_GPC is not defined. 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?
Thanks,
Bill Pringlemeir.
next prev parent reply other threads:[~2014-09-24 16:33 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-09-22 17:09 [PATCH 0/9] ARM: vf610: Suspend/resume support Stefan Agner
2014-09-22 17:09 ` [PATCH 1/9] ARM: dts: vf610: Add system reset controller (SRC) Stefan Agner
2014-09-24 9:16 ` Linus Walleij
2014-09-24 16:41 ` Stefan Agner
2014-09-25 13:08 ` Philipp Zabel
2014-09-22 17:09 ` [PATCH 2/9] ARM: dts: vf610: add global power controller (GPC) Stefan Agner
2014-09-22 17:09 ` [PATCH 3/9] ARM: dts: vf610: add on-chip SRAM Stefan Agner
2014-09-22 17:09 ` [PATCH 4/9] ARM: dts: vf610-colibri: GPIO power key Stefan Agner
2014-09-22 17:09 ` [PATCH 5/9] gpio: vf610: Extend with wakeup support Stefan Agner
2014-09-24 9:19 ` Linus Walleij
2014-09-24 16:33 ` Stefan Agner
2014-09-24 10:06 ` Lucas Stach
2014-09-24 16:51 ` Stefan Agner
2014-09-22 17:09 ` [PATCH 6/9] ARM: imx: gpc: Support vf610 global power controller Stefan Agner
2014-09-22 17:09 ` [PATCH 7/9] ARM: imx: src: Support vf610 system reset controller Stefan Agner
2014-09-22 17:09 ` [PATCH 8/9] ARM: imx: clk-gate2: allow custom gate configuration Stefan Agner
2014-09-28 2:02 ` Shawn Guo
2014-09-22 17:09 ` [PATCH 9/9] ARM: vf610: initial suspend/resume support Stefan Agner
2014-09-23 15:36 ` [PATCH 0/9] ARM: vf610: Suspend/resume support Bill Pringlemeir
2014-09-24 8:22 ` Stefan Agner
2014-09-24 16:33 ` Bill Pringlemeir [this message]
2014-09-28 3:08 ` Shawn Guo
2014-09-29 12:47 ` Stefan Agner
2014-09-29 15:39 ` Bill Pringlemeir
2014-09-28 3:15 ` Shawn Guo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87y4t9kmgz.fsf@nbsps.com \
--to=bpringlemeir@nbsps$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox