Patchwork [RFC,for-1.2] arm: Move some ARM devices into libhw

login
register
mail settings
Submitter Andreas Färber
Date Aug. 2, 2012, 1:16 a.m.
Message ID <1343870169-16049-1-git-send-email-afaerber@suse.de>
Download mbox | patch
Permalink /patch/174630/
State New
Headers show

Comments

Andreas Färber - Aug. 2, 2012, 1:16 a.m.
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 Hello Peter, here's my current draft for subjecting more arm devices to the
 stricter device checks in libhwX. Please review desired granularity (here:
 fine-grained) and naming (e.g., PL310 vs. l2x0).
 Since this is preparing for a future armeb-softmmu, Anthony's CONFIG_ARCH_ARM
 is not going to help here (we would want to turn off many devices for armeb).
 The SoCs with references to CPUs in their header are untouched, i.MX31 was
 not yet reviewed for movability.

 default-configs/arm-softmmu.mak |   19 +++++++++++++++++++
 hw/Makefile.objs                |   21 +++++++++++++++++++++
 hw/arm/Makefile.objs            |   16 ++++------------
 3 files changed, 44 insertions(+), 12 deletions(-)
Peter Maydell - Aug. 2, 2012, 1:48 p.m.
On 2 August 2012 02:16, Andreas Färber <afaerber@suse.de> wrote:
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  Hello Peter, here's my current draft for subjecting more arm devices to the
>  stricter device checks in libhwX. Please review desired granularity (here:
>  fine-grained) and naming (e.g., PL310 vs. l2x0).
>  Since this is preparing for a future armeb-softmmu, Anthony's CONFIG_ARCH_ARM
>  is not going to help here (we would want to turn off many devices for armeb).
>  The SoCs with references to CPUs in their header are untouched, i.MX31 was
>  not yet reviewed for movability.

So what's our long term vision here? Every device has a CONFIG_FOO that
gets turned on in some default-configs/ file?

What are the 'stricter checks' you mention?

> +hw-obj-$(CONFIG_PL310) += arm_l2x0.o

Maybe we should have named the source file pl310.c...

> +hw-obj-$(CONFIG_STELLARIS_ENET) += stellaris_enet.o

Why just this stellaris device and not the others?
> -obj-y += pl041.o lm4549.o
> +obj-y += lm4549.o

If we're moving the pl041 we should move its accompanying codec
(the lm4549) too, especially since the pl041 is definitely ARM
only but the lm4549 could be used on other platforms in theory.

-- PMM
Andreas Färber - Aug. 2, 2012, 2:53 p.m.
Am 02.08.2012 15:48, schrieb Peter Maydell:
> On 2 August 2012 02:16, Andreas Färber <afaerber@suse.de> wrote:
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  Hello Peter, here's my current draft for subjecting more arm devices to the
>>  stricter device checks in libhwX. Please review desired granularity (here:
>>  fine-grained) and naming (e.g., PL310 vs. l2x0).
>>  Since this is preparing for a future armeb-softmmu, Anthony's CONFIG_ARCH_ARM
>>  is not going to help here (we would want to turn off many devices for armeb).
>>  The SoCs with references to CPUs in their header are untouched, i.MX31 was
>>  not yet reviewed for movability.
> 
> So what's our long term vision here? Every device has a CONFIG_FOO that
> gets turned on in some default-configs/ file?

The general idea is to set good examples for authors of new devices and
to prepare for armeb: To me, that calls for disabling all ARM devices
apart from those whitelisted / strictly needed for BE.

And for me personally to reduce build times when changing CPU things:
Currently way too many files are needlessly rebuilt because they happen
to trigger a cpu.h dependency (NEED_CPU_H) due to sitting in the "wrong"
Makefile.

Another, independent long-term vision would be to place arm files into
hw/arm/, to put some more structure into the looong list of hw/ files.
But moving files around would be a task for you to do on your
arm-devs.next queue to not interfere with any ongoing device work. The
difference between devices and machine stuff would then be obj-y vs.
hw-obj-y as Anthony explained to me in the kvm/ context.

> What are the 'stricter checks' you mention?

Poisoning certain identifiers (Blue's initiative, I believe), no
explicit guest-dependent swaps and other limitations incurred by
cpu.h-less headers.

>> +hw-obj-$(CONFIG_PL310) += arm_l2x0.o
> 
> Maybe we should have named the source file pl310.c...

That was one of my RFC points - not sure how to interpret the header: Is
this multiple devices in one? Or different names for the same thing?
I just found it looked nicer this way. ;)

Independent of that, do we need to separate things on that granularity
at all? Or just do CONFIG_PL or something?
In practice, it seemed that usage of these devices is rather fragmented
(not all boards use all PLxxx devices) so that per-device config names
as in master allow for fine-grained control of which devices get built
if only armeb-softmmu were to be built;
on the other hand if that seems too complicated the alternative question
to ask would be, are all PLxxx devices theoretically capable of being
used for armeb as well?

>> +hw-obj-$(CONFIG_STELLARIS_ENET) += stellaris_enet.o
> 
> Why just this stellaris device and not the others?

I sent this out to comply with the rule of having a patch on the list by
Soft Freeze date, I did not find the time to complete/update it. Either
there's CPU/swap dependencies in the other files or I did not try to
device'ify them yet.

>> -obj-y += pl041.o lm4549.o
>> +obj-y += lm4549.o
> 
> If we're moving the pl041 we should move its accompanying codec
> (the lm4549) too, especially since the pl041 is definitely ARM
> only but the lm4549 could be used on other platforms in theory.

There was a merge conflict. The lm4549.o did not seem to exist when I
put together the original patch. And the file names are not exactly
telling, you'll have to admit. So I didn't check on that one yet, it got
late enough...

Andreas
Peter Maydell - Aug. 2, 2012, 3:01 p.m.
On 2 August 2012 15:53, Andreas Färber <afaerber@suse.de> wrote:
> Am 02.08.2012 15:48, schrieb Peter Maydell:
>> On 2 August 2012 02:16, Andreas Färber <afaerber@suse.de> wrote:
>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>> ---
>>>  Hello Peter, here's my current draft for subjecting more arm devices to the
>>>  stricter device checks in libhwX. Please review desired granularity (here:
>>>  fine-grained) and naming (e.g., PL310 vs. l2x0).
>>>  Since this is preparing for a future armeb-softmmu, Anthony's CONFIG_ARCH_ARM
>>>  is not going to help here (we would want to turn off many devices for armeb).
>>>  The SoCs with references to CPUs in their header are untouched, i.MX31 was
>>>  not yet reviewed for movability.
>>
>> So what's our long term vision here? Every device has a CONFIG_FOO that
>> gets turned on in some default-configs/ file?
>
> The general idea is to set good examples for authors of new devices and
> to prepare for armeb: To me, that calls for disabling all ARM devices
> apart from those whitelisted / strictly needed for BE.
>
> And for me personally to reduce build times when changing CPU things:
> Currently way too many files are needlessly rebuilt because they happen
> to trigger a cpu.h dependency (NEED_CPU_H) due to sitting in the "wrong"
> Makefile.
>
> Another, independent long-term vision would be to place arm files into
> hw/arm/, to put some more structure into the looong list of hw/ files.
> But moving files around would be a task for you to do on your
> arm-devs.next queue to not interfere with any ongoing device work. The
> difference between devices and machine stuff would then be obj-y vs.
> hw-obj-y as Anthony explained to me in the kvm/ context.
>
>> What are the 'stricter checks' you mention?
>
> Poisoning certain identifiers (Blue's initiative, I believe), no
> explicit guest-dependent swaps and other limitations incurred by
> cpu.h-less headers.
>
>>> +hw-obj-$(CONFIG_PL310) += arm_l2x0.o
>>
>> Maybe we should have named the source file pl310.c...
>
> That was one of my RFC points - not sure how to interpret the header: Is
> this multiple devices in one? Or different names for the same thing?
> I just found it looked nicer this way. ;)

Basically the (hardware) device was renamed to match the other PL*,
but from an engineering pov the PL310 is just the newer version of
the L220 (which in turn is the newer version of the L210); they're all
very similar from a programmer's view perspective. I think 'l2x0' comes
from what the kernel calls it.

> Independent of that, do we need to separate things on that granularity
> at all? Or just do CONFIG_PL or something?
> In practice, it seemed that usage of these devices is rather fragmented
> (not all boards use all PLxxx devices) so that per-device config names
> as in master allow for fine-grained control of which devices get built
> if only armeb-softmmu were to be built;
> on the other hand if that seems too complicated the alternative question
> to ask would be, are all PLxxx devices theoretically capable of being
> used for armeb as well?

They should in theory be OK on a big-endian system, but then in theory
just about any device should be OK on a big endian system: device/system
endianness and CPU endianness are orthogonal.

-- PMM
Andreas Färber - Aug. 2, 2012, 3:58 p.m.
Am 02.08.2012 17:01, schrieb Peter Maydell:
> On 2 August 2012 15:53, Andreas Färber <afaerber@suse.de> wrote:
>> In practice, it seemed that usage of these devices is rather fragmented
>> (not all boards use all PLxxx devices) so that per-device config names
>> as in master allow for fine-grained control of which devices get built
>> if only armeb-softmmu were to be built;
>> on the other hand if that seems too complicated the alternative question
>> to ask would be, are all PLxxx devices theoretically capable of being
>> used for armeb as well?
> 
> They should in theory be OK on a big-endian system, but then in theory
> just about any device should be OK on a big endian system: device/system
> endianness and CPU endianness are orthogonal.

I was specifically thinking of whether the specified device endianness
is correct, i.e. whether device code assumes Little Endian but might be
marked DEVICE_NATIVE_ENDIAN (thus becoming Big Endian on armeb) or is
marked DEVICE_LITTLE_ENDIAN but should be DEVICE_NATIVE_ENDIAN to adapt
to armeb (e.g., double-swapping issues with IBM8514 card a while ago).

Andreas
Andreas Färber - Aug. 10, 2012, 5:15 p.m.
Am 02.08.2012 15:48, schrieb Peter Maydell:
> On 2 August 2012 02:16, Andreas Färber <afaerber@suse.de> wrote:
>> +hw-obj-$(CONFIG_STELLARIS_ENET) += stellaris_enet.o
> 
> Why just this stellaris device and not the others?

So, turns out there was an actual reason:

  CC    libhw64/hw/stellaris.o
In file included from /home/andreas/QEMU/qemu-arm/hw/stellaris.c:12:0:
/home/andreas/QEMU/qemu-arm/hw/arm-misc.h:19:28: error: unknown type
name ‘ARMCPU’
/home/andreas/QEMU/qemu-arm/hw/arm-misc.h:53:34: error: unknown type
name ‘ARMCPU’
/home/andreas/QEMU/qemu-arm/hw/arm-misc.h:55:38: error: unknown type
name ‘ARMCPU’
/home/andreas/QEMU/qemu-arm/hw/arm-misc.h:62:22: error: unknown type
name ‘ARMCPU’
make[1]: *** [hw/stellaris.o] Fehler 1
make: *** [subdir-libhw64] Fehler 2

Leave this part of the patch as is for now then?

Andreas
Peter Maydell - Aug. 11, 2012, 7:12 p.m.
On 10 August 2012 18:15, Andreas Färber <afaerber@suse.de> wrote:
> Am 02.08.2012 15:48, schrieb Peter Maydell:
>> On 2 August 2012 02:16, Andreas Färber <afaerber@suse.de> wrote:
>>> +hw-obj-$(CONFIG_STELLARIS_ENET) += stellaris_enet.o
>>
>> Why just this stellaris device and not the others?
>
> So, turns out there was an actual reason:
>
>   CC    libhw64/hw/stellaris.o
> In file included from /home/andreas/QEMU/qemu-arm/hw/stellaris.c:12:0:
> /home/andreas/QEMU/qemu-arm/hw/arm-misc.h:19:28: error: unknown type
> name ‘ARMCPU’
> /home/andreas/QEMU/qemu-arm/hw/arm-misc.h:53:34: error: unknown type
> name ‘ARMCPU’
> /home/andreas/QEMU/qemu-arm/hw/arm-misc.h:55:38: error: unknown type
> name ‘ARMCPU’
> /home/andreas/QEMU/qemu-arm/hw/arm-misc.h:62:22: error: unknown type
> name ‘ARMCPU’
> make[1]: *** [hw/stellaris.o] Fehler 1
> make: *** [subdir-libhw64] Fehler 2

Yeah, although in fact this is just the result of arm-misc.h being a bit
of a grab-bag header: all stellaris.c needs is the prototype for armv7m_init()
and the system_clock_scale extern definition, not any of the ARMCPU
using things.

(system_clock_scale incidentally is pretty nasty -- this should be
done as some kind of QOM connection from the board model to the
armv7m and its systick device so the board model can feed the
right value in. This corresponds to hardware, where there is an external
set of signal lines for the board to pass this value in with.)

Also, stellaris.c is the board model, not a pure device, so you could
kind of construct a justification for only moving the other stellaris files
(ideally the devices wedged into stellaris.c at the moment would move
out of it, I guess.)

> Leave this part of the patch as is for now then?

If you don't want to try fixing this mess now I'd leave all the stellaris
bits where they are for the moment.

-- PMM

Patch

diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
index e542b4f..f8f32f6 100644
--- a/default-configs/arm-softmmu.mak
+++ b/default-configs/arm-softmmu.mak
@@ -27,3 +27,22 @@  CONFIG_SMC91C111=y
 CONFIG_DS1338=y
 CONFIG_PFLASH_CFI01=y
 CONFIG_PFLASH_CFI02=y
+
+CONFIG_ARM_TIMER=y
+CONFIG_PL011=y
+CONFIG_PL022=y
+CONFIG_PL031=y
+CONFIG_PL041=y
+CONFIG_PL050=y
+CONFIG_PL061=y
+CONFIG_PL080=y
+CONFIG_PL110=y
+CONFIG_PL181=y
+CONFIG_PL190=y
+CONFIG_PL310=y
+CONFIG_CADENCE=y
+CONFIG_XGMAC=y
+
+CONFIG_VERSATILE_PCI=y
+CONFIG_VERSATILE_I2C=y
+CONFIG_STELLARIS_ENET=y
diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index 8327e55..769fb43 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -66,6 +66,27 @@  hw-obj-$(CONFIG_XILINX) += xilinx_uartlite.o
 hw-obj-$(CONFIG_XILINX_AXI) += xilinx_axidma.o
 hw-obj-$(CONFIG_XILINX_AXI) += xilinx_axienet.o
 
+# ARM devices
+hw-obj-$(CONFIG_ARM_TIMER) += arm_timer.o
+hw-obj-$(CONFIG_PL011) += pl011.o
+hw-obj-$(CONFIG_PL022) += pl022.o
+hw-obj-$(CONFIG_PL031) += pl031.o
+hw-obj-$(CONFIG_PL041) += pl041.o
+hw-obj-$(CONFIG_PL050) += pl050.o
+hw-obj-$(CONFIG_PL061) += pl061.o
+hw-obj-$(CONFIG_PL080) += pl080.o
+hw-obj-$(CONFIG_PL110) += pl110.o
+hw-obj-$(CONFIG_PL181) += pl181.o
+hw-obj-$(CONFIG_PL190) += pl190.o
+hw-obj-$(CONFIG_PL310) += arm_l2x0.o
+hw-obj-$(CONFIG_VERSATILE_PCI) += versatile_pci.o
+hw-obj-$(CONFIG_VERSATILE_I2C) += versatile_i2c.o
+hw-obj-$(CONFIG_CADENCE) += cadence_uart.o
+hw-obj-$(CONFIG_CADENCE) += cadence_ttc.o
+hw-obj-$(CONFIG_CADENCE) += cadence_gem.o
+hw-obj-$(CONFIG_XGMAC) += xgmac.o
+hw-obj-$(CONFIG_STELLARIS_ENET) += stellaris_enet.o
+
 # PCI watchdog devices
 hw-obj-$(CONFIG_PCI) += wdt_i6300esb.o
 
diff --git a/hw/arm/Makefile.objs b/hw/arm/Makefile.objs
index c413780..b1fc7f5 100644
--- a/hw/arm/Makefile.objs
+++ b/hw/arm/Makefile.objs
@@ -1,10 +1,5 @@ 
-obj-y = integratorcp.o versatilepb.o arm_pic.o arm_timer.o
-obj-y += arm_boot.o pl011.o pl031.o pl050.o pl080.o pl110.o pl181.o pl190.o
-obj-y += versatile_pci.o
-obj-y += versatile_i2c.o
-obj-y += cadence_uart.o
-obj-y += cadence_ttc.o
-obj-y += cadence_gem.o
+obj-y = integratorcp.o versatilepb.o arm_pic.o
+obj-y += arm_boot.o
 obj-y += xilinx_zynq.o zynq_slcr.o
 obj-y += arm_gic.o arm_gic_common.o
 obj-y += realview_gic.o realview.o arm_sysctl.o arm11mpcore.o a9mpcore.o
@@ -12,12 +7,9 @@  obj-y += exynos4210_gic.o exynos4210_combiner.o exynos4210.o
 obj-y += exynos4_boards.o exynos4210_uart.o exynos4210_pwm.o
 obj-y += exynos4210_pmu.o exynos4210_mct.o exynos4210_fimd.o
 obj-y += exynos4210_rtc.o exynos4210_i2c.o
-obj-y += arm_l2x0.o
 obj-y += arm_mptimer.o a15mpcore.o
-obj-y += armv7m.o armv7m_nvic.o stellaris.o pl022.o stellaris_enet.o
+obj-y += armv7m.o armv7m_nvic.o stellaris.o
 obj-y += highbank.o
-obj-y += pl061.o
-obj-y += xgmac.o
 obj-y += pxa2xx.o pxa2xx_pic.o pxa2xx_gpio.o pxa2xx_timer.o pxa2xx_dma.o
 obj-y += pxa2xx_lcd.o pxa2xx_mmci.o pxa2xx_pcmcia.o pxa2xx_keypad.o
 obj-y += gumstix.o
@@ -37,7 +29,7 @@  obj-y += strongarm.o
 obj-y += collie.o
 obj-y += imx_serial.o imx_ccm.o imx_timer.o imx_avic.o
 obj-y += kzm.o
-obj-y += pl041.o lm4549.o
+obj-y += lm4549.o
 obj-$(CONFIG_FDT) += ../device_tree.o
 
 obj-y := $(addprefix ../,$(obj-y))