* Re: [PATCH 00/08] ARM: shmobile: Suspend on non-SMP update for r8a7790
2014-06-05 5:15 [PATCH 00/08] ARM: shmobile: Suspend on non-SMP update for r8a7790 Magnus Damm
@ 2014-06-05 8:01 ` Geert Uytterhoeven
2014-06-05 21:34 ` Magnus Damm
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Geert Uytterhoeven @ 2014-06-05 8:01 UTC (permalink / raw)
To: linux-sh
Hi Magnus,
On Thu, Jun 5, 2014 at 7:15 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> ARM: shmobile: Suspend on non-SMP update for r8a7790
>
> [PATCH 01/08] ARM: shmobile: Allow use of boot code for non-SMP case
> [PATCH 02/08] ARM: shmobile: Adjust APMU code to build for non-SMP
> [PATCH 03/08] ARM: shmobile: Move r8a7790 reset code to pm-r8a7790.c
> [PATCH 04/08] ARM: shmobile: Allow r8a7790 to build non-SMP APMU code
> [PATCH 05/08] ARM: shmobile: Use __init for APMU suspend init function
> [PATCH 06/08] ARM: shmobile: Setup APMU for suspend in non-SMP case
> [PATCH 07/08] ARM: shmobile: Hook up r8a7790 PM code for Multiplatform
> [PATCH 08/08] ARM: shmobile: Use shmobile_init_late on r8a7790 DT-only
>
> These patches update mach-shmobile and r8a7790 to allow build of
> early boot code and APMU power management code in case of CONFIG_SMP=n.
Thanks!
> The reason for this change is to allow use of Suspend-to-RAM for recent
> SoCs like r8a7790 with simplified kernel configuration such as when
> SMP is disabled. Power management code like CPUFreq, CPUIdle and
> Suspend-to-RAM should work regardless of what kind of CONFIG_SMP
> setting the user decides.
Perhaps the non-SMP-specific code can be factored out of headsmp.c and
platsmp.c into one or more separate files, and the "smp" dropped from the
function names, or is it too entangled with the SMP-specific code?
Things like (in [6/8]):
void __init shmobile_smp_apmu_suspend_init(void)
{
+#ifndef CONFIG_SMP
+ /* initialize APMU for non-SMP case */
+ shmobile_smp_apmu_prepare_cpus(1);
+#endif
shmobile_suspend_ops.enter = shmobile_smp_apmu_enter_suspend;
}
#else
can look really confusing: shmobile_*SMP*_apmu_suspend_init() has a
!CONFIG_SMP section calling into shmobile_*SMP*_apmu_prepare_cpus()
(the latter is OK, due to the "1" parameter indicating there's only one CPU).
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 00/08] ARM: shmobile: Suspend on non-SMP update for r8a7790
2014-06-05 5:15 [PATCH 00/08] ARM: shmobile: Suspend on non-SMP update for r8a7790 Magnus Damm
2014-06-05 8:01 ` Geert Uytterhoeven
@ 2014-06-05 21:34 ` Magnus Damm
2014-06-06 6:49 ` Magnus Damm
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Magnus Damm @ 2014-06-05 21:34 UTC (permalink / raw)
To: linux-sh
Hi Geert,
On Thu, Jun 5, 2014 at 5:01 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Magnus,
>
> On Thu, Jun 5, 2014 at 7:15 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>> ARM: shmobile: Suspend on non-SMP update for r8a7790
>>
>> [PATCH 01/08] ARM: shmobile: Allow use of boot code for non-SMP case
>> [PATCH 02/08] ARM: shmobile: Adjust APMU code to build for non-SMP
>> [PATCH 03/08] ARM: shmobile: Move r8a7790 reset code to pm-r8a7790.c
>> [PATCH 04/08] ARM: shmobile: Allow r8a7790 to build non-SMP APMU code
>> [PATCH 05/08] ARM: shmobile: Use __init for APMU suspend init function
>> [PATCH 06/08] ARM: shmobile: Setup APMU for suspend in non-SMP case
>> [PATCH 07/08] ARM: shmobile: Hook up r8a7790 PM code for Multiplatform
>> [PATCH 08/08] ARM: shmobile: Use shmobile_init_late on r8a7790 DT-only
>>
>> These patches update mach-shmobile and r8a7790 to allow build of
>> early boot code and APMU power management code in case of CONFIG_SMP=n.
>
> Thanks!
>
>> The reason for this change is to allow use of Suspend-to-RAM for recent
>> SoCs like r8a7790 with simplified kernel configuration such as when
>> SMP is disabled. Power management code like CPUFreq, CPUIdle and
>> Suspend-to-RAM should work regardless of what kind of CONFIG_SMP
>> setting the user decides.
>
> Perhaps the non-SMP-specific code can be factored out of headsmp.c and
> platsmp.c into one or more separate files, and the "smp" dropped from the
> function names, or is it too entangled with the SMP-specific code?
Yes, eventually I'd like to do this. You are right that both file and
function names should probably be updated.
At this point I'm mainly focused on being able to compile the code in
UP mode. I want to make sure that the UP case of Core Standby
Suspend-to-RAM is working. I know that SMP at least used to work at
some point, but Suspend-to-RAM with Core Standby on UP may need more
attention.
> Things like (in [6/8]):
>
> void __init shmobile_smp_apmu_suspend_init(void)
> {
> +#ifndef CONFIG_SMP
> + /* initialize APMU for non-SMP case */
> + shmobile_smp_apmu_prepare_cpus(1);
> +#endif
> shmobile_suspend_ops.enter = shmobile_smp_apmu_enter_suspend;
> }
> #else
>
> can look really confusing: shmobile_*SMP*_apmu_suspend_init() has a
> !CONFIG_SMP section calling into shmobile_*SMP*_apmu_prepare_cpus()
> (the latter is OK, due to the "1" parameter indicating there's only one CPU).
I agree that this portion is far from pretty. Ideally in my opinion
the ARM SMP callbacks should be updated to allow executing some SMP
setup code even though UP is used. Right now we need to call setup
code from various directions depending on mode which results in
italian pasta dish coding. =)
Cheers,
/ magnus
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 00/08] ARM: shmobile: Suspend on non-SMP update for r8a7790
2014-06-05 5:15 [PATCH 00/08] ARM: shmobile: Suspend on non-SMP update for r8a7790 Magnus Damm
2014-06-05 8:01 ` Geert Uytterhoeven
2014-06-05 21:34 ` Magnus Damm
@ 2014-06-06 6:49 ` Magnus Damm
2014-06-06 7:08 ` Geert Uytterhoeven
2014-06-06 7:36 ` Magnus Damm
4 siblings, 0 replies; 6+ messages in thread
From: Magnus Damm @ 2014-06-06 6:49 UTC (permalink / raw)
To: linux-sh
Hi again Geert,
On Fri, Jun 6, 2014 at 6:34 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> On Thu, Jun 5, 2014 at 5:01 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>> Things like (in [6/8]):
>>
>> void __init shmobile_smp_apmu_suspend_init(void)
>> {
>> +#ifndef CONFIG_SMP
>> + /* initialize APMU for non-SMP case */
>> + shmobile_smp_apmu_prepare_cpus(1);
>> +#endif
>> shmobile_suspend_ops.enter = shmobile_smp_apmu_enter_suspend;
>> }
>> #else
>>
>> can look really confusing: shmobile_*SMP*_apmu_suspend_init() has a
>> !CONFIG_SMP section calling into shmobile_*SMP*_apmu_prepare_cpus()
>> (the latter is OK, due to the "1" parameter indicating there's only one CPU).
>
> I agree that this portion is far from pretty. Ideally in my opinion
> the ARM SMP callbacks should be updated to allow executing some SMP
> setup code even though UP is used. Right now we need to call setup
> code from various directions depending on mode which results in
> italian pasta dish coding. =)
To try to improve the situation I'm considering adding a single setup
function for mach-shmobile. I was playing around with a similar
approach earlier but I could at that time not get any positive
comments about setting up the smp_ops dynamically. In general I think
it makes sense to setup a lot of similar callbacks from C instead of
explicitly adding them to each and every SoC and board mach vector.
Perhaps we could also deal with UP vs SMP at the same time?
The idea with the code below would be to allow passing two pointers to
the setup code:
1) function pointer for UP case
2) smp_ops pointer for SMP setup case
The setup function (shmobile_init_cpus() below) would be responsible
for the following:
- if running as smp then register the smp_ops
- if running as up then call the function pointer for setup
- execute shmobile_init_delay()
- install shmobile_init_late in the machine vector
For the common case every machine vector we would only need dt_compat
and init_early.
What do you think?
Cheers,
/ magnus
--- 0007/arch/arm/mach-shmobile/setup-emev2.c
+++ work/arch/arm/mach-shmobile/setup-emev2.c 2014-06-06
15:34:37.000000000 +0900
@@ -41,17 +41,20 @@ static void __init emev2_map_io(void)
iotable_init(emev2_io_desc, ARRAY_SIZE(emev2_io_desc));
}
+extern struct smp_operations emev2_smp_ops;
+
+static void __init emev2_init(void)
+{
+ shmobile_init_cpus(NULL, smp_ops(emev2_smp_ops));
+}
+
static const char *emev2_boards_compat_dt[] __initconst = {
"renesas,emev2",
NULL,
};
-extern struct smp_operations emev2_smp_ops;
-
DT_MACHINE_START(EMEV2_DT, "Generic Emma Mobile EV2 (Flattened Device Tree)")
- .smp = smp_ops(emev2_smp_ops),
.map_io = emev2_map_io,
- .init_early = shmobile_init_delay,
- .init_late = shmobile_init_late,
+ .init_early = emev2_init,
.dt_compat = emev2_boards_compat_dt,
MACHINE_END
../foo.patch lines 14-36/36 (END)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 00/08] ARM: shmobile: Suspend on non-SMP update for r8a7790
2014-06-05 5:15 [PATCH 00/08] ARM: shmobile: Suspend on non-SMP update for r8a7790 Magnus Damm
` (2 preceding siblings ...)
2014-06-06 6:49 ` Magnus Damm
@ 2014-06-06 7:08 ` Geert Uytterhoeven
2014-06-06 7:36 ` Magnus Damm
4 siblings, 0 replies; 6+ messages in thread
From: Geert Uytterhoeven @ 2014-06-06 7:08 UTC (permalink / raw)
To: linux-sh
Hi Magnus,
On Fri, Jun 6, 2014 at 8:49 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> To try to improve the situation I'm considering adding a single setup
> function for mach-shmobile. I was playing around with a similar
> approach earlier but I could at that time not get any positive
> comments about setting up the smp_ops dynamically. In general I think
> it makes sense to setup a lot of similar callbacks from C instead of
> explicitly adding them to each and every SoC and board mach vector.
> Perhaps we could also deal with UP vs SMP at the same time?
>
> The idea with the code below would be to allow passing two pointers to
> the setup code:
> 1) function pointer for UP case
> 2) smp_ops pointer for SMP setup case
>
> The setup function (shmobile_init_cpus() below) would be responsible
> for the following:
> - if running as smp then register the smp_ops
> - if running as up then call the function pointer for setup
> - execute shmobile_init_delay()
> - install shmobile_init_late in the machine vector
>
> For the common case every machine vector we would only need dt_compat
> and init_early.
>
> What do you think?
Sounds reasonable.
However, I had a quick look at the code, and am no longer that positive
about it:
- .init_late() is too late to register the SMP ops, that's why there's a
.smp pointer, which is used much earlier (see setup_arch()),
- The machine vector is const, so you cannot install a handler later,
- In the end, you would basically replace the current machine vector
callback scheme (which is data) by setup functions (which are code),
which is moving away from what the machine vector was originally
intended for?
Anyway, if you want to go this way, it needs discussion on the ARM kernel
mailing list.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 00/08] ARM: shmobile: Suspend on non-SMP update for r8a7790
2014-06-05 5:15 [PATCH 00/08] ARM: shmobile: Suspend on non-SMP update for r8a7790 Magnus Damm
` (3 preceding siblings ...)
2014-06-06 7:08 ` Geert Uytterhoeven
@ 2014-06-06 7:36 ` Magnus Damm
4 siblings, 0 replies; 6+ messages in thread
From: Magnus Damm @ 2014-06-06 7:36 UTC (permalink / raw)
To: linux-sh
Hi Geert,
On Fri, Jun 6, 2014 at 4:08 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Magnus,
>
> On Fri, Jun 6, 2014 at 8:49 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>> To try to improve the situation I'm considering adding a single setup
>> function for mach-shmobile. I was playing around with a similar
>> approach earlier but I could at that time not get any positive
>> comments about setting up the smp_ops dynamically. In general I think
>> it makes sense to setup a lot of similar callbacks from C instead of
>> explicitly adding them to each and every SoC and board mach vector.
>> Perhaps we could also deal with UP vs SMP at the same time?
>>
>> The idea with the code below would be to allow passing two pointers to
>> the setup code:
>> 1) function pointer for UP case
>> 2) smp_ops pointer for SMP setup case
>>
>> The setup function (shmobile_init_cpus() below) would be responsible
>> for the following:
>> - if running as smp then register the smp_ops
>> - if running as up then call the function pointer for setup
>> - execute shmobile_init_delay()
>> - install shmobile_init_late in the machine vector
>>
>> For the common case every machine vector we would only need dt_compat
>> and init_early.
>>
>> What do you think?
>
> Sounds reasonable.
>
> However, I had a quick look at the code, and am no longer that positive
> about it:
> - .init_late() is too late to register the SMP ops, that's why there's a
> .smp pointer, which is used much earlier (see setup_arch()),
Yes, ->smp_init() and smp_ops->smp_init_cpus() may get called early
on. We do however not rely on those for any of our SMP code.
> - The machine vector is const, so you cannot install a handler later,
Ah, right, the smp_ops portion is handled as a special case. So
installing other callbacks seems unlikely.
> - In the end, you would basically replace the current machine vector
> callback scheme (which is data) by setup functions (which are code),
> which is moving away from what the machine vector was originally
> intended for?
Well, I've noticed a trend which is removing stuff from the machine
vector and replacing with run time installation. Good or bad.
smp_set_ops() is one such case. set_handle_irq() is another.
> Anyway, if you want to go this way, it needs discussion on the ARM kernel
> mailing list.
I agree, but I was hoping for something more short term... Just simply
trying to fix things that are broken take ages unfortunately. This
means we have to hold off for a couple of months at least
dependency-wise.
Cheers,
/ magnus
^ permalink raw reply [flat|nested] 6+ messages in thread