[Qemu-devel] [PATCH v3 00/11] hw: arm: exynos: Bring up secondary CPU, QOM-ify Soc, other improvements

Krzysztof Kozlowski posted 11 patches 6 years, 10 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170521152949.15338-1-krzk@kernel.org
Test checkpatch passed
Test docker passed
Test s390x passed
hw/arm/exynos4210.c         | 43 ++++++++++++++++--------------------
hw/arm/exynos4_boards.c     | 53 +++++++++++++++++++++++++++++++++++++++------
hw/intc/exynos4210_gic.c    | 35 ++++++++++++++++++------------
hw/misc/exynos4210_pmu.c    | 20 ++++++++++++++++-
hw/timer/exynos4210_mct.c   | 50 ++++++++++++++++++------------------------
include/hw/arm/exynos4210.h | 11 +++++-----
6 files changed, 132 insertions(+), 80 deletions(-)
[Qemu-devel] [PATCH v3 00/11] hw: arm: exynos: Bring up secondary CPU, QOM-ify Soc, other improvements
Posted by Krzysztof Kozlowski 6 years, 10 months ago
Hi,

This is a collection of three already sent patchsets [1] [2] [3].


Changes since previous versions (v2):
1. Patch 11/11: set BIT(8) in PS_HOLD to fix power off on Linux v4.12-rc1.
2. Add reviewed-by tags.


The first patch in set is a bugfix which also exposes problem of cpuidle.
Kernel compiled with disabled CPU_IDLE works fine (two CPUs, coming SGIs,
poweroff working) but with CPU_IDLE it behaves bad due to low-power
mode (AFTR). This was discussed at [1].

There are no external dependencies.

Patchset is also available here:
https://github.com/krzk/qemu/tree/for-next/exynos4210-gic-qom-soc-poweroff-v3


Best regards,
Krzysztof

[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg436902.html
[2] https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01562.html
[3] https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01553.html


Krzysztof Kozlowski (11):
  hw/intc/exynos4210_gic: Fix GIC memory mappings for secondary CPU
  hw/intc/exynos4210_gic: Use more meaningful name for local variable
  hw/timer/exynos4210_mct: Fix checkpatch style errors
  hw/timer/exynos4210_mct: Cleanup indentation and empty new lines
  hw/timer/exynos4210_mct: Remove unused defines
  hw/arm/exynos: Move DRAM initialization next boards
  hw/arm/exynos: Declare local variables in some order
  hw/arm/exynos: QOM-ify the SoC
  hw/arm/exynos: Use type define instead of hard-coded a9mpcore_priv
    string
  hw/intc/exynos4210_gic: Constify array of combiner interrupts
  hw/misc/exynos4210_pmu: Add support for system poweroff

 hw/arm/exynos4210.c         | 43 ++++++++++++++++--------------------
 hw/arm/exynos4_boards.c     | 53 +++++++++++++++++++++++++++++++++++++++------
 hw/intc/exynos4210_gic.c    | 35 ++++++++++++++++++------------
 hw/misc/exynos4210_pmu.c    | 20 ++++++++++++++++-
 hw/timer/exynos4210_mct.c   | 50 ++++++++++++++++++------------------------
 include/hw/arm/exynos4210.h | 11 +++++-----
 6 files changed, 132 insertions(+), 80 deletions(-)

-- 
2.9.3


Re: [Qemu-devel] [PATCH v3 00/11] hw: arm: exynos: Bring up secondary CPU, QOM-ify Soc, other improvements
Posted by Peter Maydell 6 years, 10 months ago
On 21 May 2017 at 16:29, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> This is a collection of three already sent patchsets [1] [2] [3].
>
>
> Changes since previous versions (v2):
> 1. Patch 11/11: set BIT(8) in PS_HOLD to fix power off on Linux v4.12-rc1.
> 2. Add reviewed-by tags.
>
>
> The first patch in set is a bugfix which also exposes problem of cpuidle.
> Kernel compiled with disabled CPU_IDLE works fine (two CPUs, coming SGIs,
> poweroff working) but with CPU_IDLE it behaves bad due to low-power
> mode (AFTR). This was discussed at [1].

So is this a regression compared to our current model? I was
under the impression from the previous thread that it was,
which is why I didn't apply that patchset.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 00/11] hw: arm: exynos: Bring up secondary CPU, QOM-ify Soc, other improvements
Posted by Krzysztof Kozlowski 6 years, 10 months ago
On Tue, May 30, 2017 at 2:04 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 21 May 2017 at 16:29, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>> This is a collection of three already sent patchsets [1] [2] [3].
>>
>>
>> Changes since previous versions (v2):
>> 1. Patch 11/11: set BIT(8) in PS_HOLD to fix power off on Linux v4.12-rc1.
>> 2. Add reviewed-by tags.
>>
>>
>> The first patch in set is a bugfix which also exposes problem of cpuidle.
>> Kernel compiled with disabled CPU_IDLE works fine (two CPUs, coming SGIs,
>> poweroff working) but with CPU_IDLE it behaves bad due to low-power
>> mode (AFTR). This was discussed at [1].
>
> So is this a regression compared to our current model? I was
> under the impression from the previous thread that it was,
> which is why I didn't apply that patchset.

It depends how wide understanding of regression you have. :)
1. Before this patch, second CPU could not boot. System was usable but
worked on only one CPU.
2. Before this patch, kernel's IRQ work is broken. None of IRQ work
are executed which is mostly visible during poweroff - system hangs on
syncing IRQ works.
3. With this patch, second CPU boots and IRQ works properly (so
poweroff is possible).
4. However with this patch, Linux kernel with cpuidle enabled (which
is also included in many kernel defconfigs), the system is very
unresponsive.

So overall... a lot of things were broken already. This patches fixes
them... but breaks different thing.

Best regards,
Krzysztof

Re: [Qemu-devel] [PATCH v3 00/11] hw: arm: exynos: Bring up secondary CPU, QOM-ify Soc, other improvements
Posted by Peter Maydell 6 years, 10 months ago
On 31 May 2017 at 09:58, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Tue, May 30, 2017 at 2:04 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> So is this a regression compared to our current model? I was
>> under the impression from the previous thread that it was,
>> which is why I didn't apply that patchset.
>
> It depends how wide understanding of regression you have. :)
> 1. Before this patch, second CPU could not boot. System was usable but
> worked on only one CPU.
> 2. Before this patch, kernel's IRQ work is broken. None of IRQ work
> are executed which is mostly visible during poweroff - system hangs on
> syncing IRQ works.
> 3. With this patch, second CPU boots and IRQ works properly (so
> poweroff is possible).
> 4. However with this patch, Linux kernel with cpuidle enabled (which
> is also included in many kernel defconfigs), the system is very
> unresponsive.
>
> So overall... a lot of things were broken already. This patches fixes
> them... but breaks different thing.

It sounds like it breaks a key thing, ie "boot a single cpu kernel
built from a defconfig", though. That's a regression I'd rather
not have. If there's a subset of these patches which don't break
that I'm happy to take those. Otherwise we need to investigate
and fix whatever the problem is that causes that unresponsiveness
before we can apply them.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 00/11] hw: arm: exynos: Bring up secondary CPU, QOM-ify Soc, other improvements
Posted by Krzysztof Kozlowski 6 years, 10 months ago
On Thu, Jun 01, 2017 at 05:31:41PM +0100, Peter Maydell wrote:
> On 31 May 2017 at 09:58, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > On Tue, May 30, 2017 at 2:04 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> >> So is this a regression compared to our current model? I was
> >> under the impression from the previous thread that it was,
> >> which is why I didn't apply that patchset.
> >
> > It depends how wide understanding of regression you have. :)
> > 1. Before this patch, second CPU could not boot. System was usable but
> > worked on only one CPU.
> > 2. Before this patch, kernel's IRQ work is broken. None of IRQ work
> > are executed which is mostly visible during poweroff - system hangs on
> > syncing IRQ works.
> > 3. With this patch, second CPU boots and IRQ works properly (so
> > poweroff is possible).
> > 4. However with this patch, Linux kernel with cpuidle enabled (which
> > is also included in many kernel defconfigs), the system is very
> > unresponsive.
> >
> > So overall... a lot of things were broken already. This patches fixes
> > them... but breaks different thing.
> 
> It sounds like it breaks a key thing, ie "boot a single cpu kernel
> built from a defconfig", though. That's a regression I'd rather
> not have.

I am not sure if I understand you correctly... the system previously
booted into one CPU mode because second CPU just could not boot. Now by
default, second CPU succeeds and this means that *default* settings are
indeed more broken than before.

On the other hand, after removing a line from hw/arm/exynos4210.c:
	qdev_prop_set_uint32(dev, "num-cpu", EXYNOS4210_NCPUS);
and running qemu with "-accel tcg -smp cpus=1,maxcpus=1,cores=1"
everything is okay. Booting of one CPU is unaffected.

> If there's a subset of these patches which don't break
> that I'm happy to take those. Otherwise we need to investigate
> and fix whatever the problem is that causes that unresponsiveness
> before we can apply them.

Yes, you can take everything except first patch
("hw/intc/exynos4210_gic: Fix GIC memory mappings for secondary CPU")
and it should be fine. Rest of patches are just independent.

Best regards,
Krzysztof


Re: [Qemu-devel] [PATCH v3 00/11] hw: arm: exynos: Bring up secondary CPU, QOM-ify Soc, other improvements
Posted by Krzysztof Kozlowski 6 years, 10 months ago
On Thu, Jun 01, 2017 at 07:10:19PM +0200, Krzysztof Kozlowski wrote:
> On Thu, Jun 01, 2017 at 05:31:41PM +0100, Peter Maydell wrote:
> > On 31 May 2017 at 09:58, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> > > On Tue, May 30, 2017 at 2:04 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> > >> So is this a regression compared to our current model? I was
> > >> under the impression from the previous thread that it was,
> > >> which is why I didn't apply that patchset.
> > >
> > > It depends how wide understanding of regression you have. :)
> > > 1. Before this patch, second CPU could not boot. System was usable but
> > > worked on only one CPU.
> > > 2. Before this patch, kernel's IRQ work is broken. None of IRQ work
> > > are executed which is mostly visible during poweroff - system hangs on
> > > syncing IRQ works.
> > > 3. With this patch, second CPU boots and IRQ works properly (so
> > > poweroff is possible).
> > > 4. However with this patch, Linux kernel with cpuidle enabled (which
> > > is also included in many kernel defconfigs), the system is very
> > > unresponsive.
> > >
> > > So overall... a lot of things were broken already. This patches fixes
> > > them... but breaks different thing.
> > 
> > It sounds like it breaks a key thing, ie "boot a single cpu kernel
> > built from a defconfig", though. That's a regression I'd rather
> > not have.
> 
> I am not sure if I understand you correctly... the system previously
> booted into one CPU mode because second CPU just could not boot. Now by
> default, second CPU succeeds and this means that *default* settings are
> indeed more broken than before.
> 
> On the other hand, after removing a line from hw/arm/exynos4210.c:
> 	qdev_prop_set_uint32(dev, "num-cpu", EXYNOS4210_NCPUS);
> and running qemu with "-accel tcg -smp cpus=1,maxcpus=1,cores=1"
> everything is okay. Booting of one CPU is unaffected.

Ahh, and one more thing. For forced single CPU mode (mentioned above),
now it actually behaves better. Fixed GIC mappings means that SGIs are
working (execution of IRQ work is fixed) thus the system can properly
power off.

This means we could hard-code one-cpu for now and get better behavior
than previously (one CPU but with SGIs).

Best regards,
Krzysztof

> 
> > If there's a subset of these patches which don't break
> > that I'm happy to take those. Otherwise we need to investigate
> > and fix whatever the problem is that causes that unresponsiveness
> > before we can apply them.
> 
> Yes, you can take everything except first patch
> ("hw/intc/exynos4210_gic: Fix GIC memory mappings for secondary CPU")
> and it should be fine. Rest of patches are just independent.


Re: [Qemu-devel] [PATCH v3 00/11] hw: arm: exynos: Bring up secondary CPU, QOM-ify Soc, other improvements
Posted by Peter Maydell 6 years, 10 months ago
On 1 June 2017 at 18:10, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> On Thu, Jun 01, 2017 at 05:31:41PM +0100, Peter Maydell wrote:
>> It sounds like it breaks a key thing, ie "boot a single cpu kernel
>> built from a defconfig", though. That's a regression I'd rather
>> not have.
>
> I am not sure if I understand you correctly... the system previously
> booted into one CPU mode because second CPU just could not boot. Now by
> default, second CPU succeeds and this means that *default* settings are
> indeed more broken than before.
>
> On the other hand, after removing a line from hw/arm/exynos4210.c:
>         qdev_prop_set_uint32(dev, "num-cpu", EXYNOS4210_NCPUS);
> and running qemu with "-accel tcg -smp cpus=1,maxcpus=1,cores=1"
> everything is okay. Booting of one CPU is unaffected.

Ah, I see now.

>> If there's a subset of these patches which don't break
>> that I'm happy to take those. Otherwise we need to investigate
>> and fix whatever the problem is that causes that unresponsiveness
>> before we can apply them.
>
> Yes, you can take everything except first patch
> ("hw/intc/exynos4210_gic: Fix GIC memory mappings for secondary CPU")
> and it should be fine. Rest of patches are just independent.

Can you send a series which has just the patches that don't break,
please? I think this series needs a respin anyway because of the
make check problems (noted on the other thread).

thanks
-- PMM