Re: [Qemu-devel] [PATCH v2] realview: fix reset bit depending on platform

[Peter Maydell]

On 5 November 2011 11:23, Jean-Christophe DUBOIS <address@hidden> wrote:
> Depending on the considered baseboard the bit used to
> reset the platform is different.
>
> Here is the list of considered Realview/Versatile platforms:
>
> Realview/Versatile AB for ARM926EJ-S: BOARD_ID = 0x100 = BOARD_ID_PB926
> http://infocenter.arm.com/help/topic/com.arm.doc.dui0225d/CACCIFGI.html
>
> RealView Emulation Baseboard: BOARD_ID = 0x140 = BOARD_ID_EB
> No reset register
>
> RealView PB for ARM1176JZF-S: BOARD_ID = 0x147 = BOARD_ID_PB1176
> http://infocenter.arm.com/help/topic/com.arm.doc.dui0425f/Caccifgi.html
>
> RealView PB for ARM11 MPCore: BOARD_ID = 0x159 = BOARD_ID_PB11MP
> http://infocenter.arm.com/help/topic/com.arm.doc.dui0351e/CACCHBFB.html
>
> RealView PB for Cortex-A8: BOARD_ID = 0x178 = BOARD_ID_PBA8
> http://infocenter.arm.com/help/topic/com.arm.doc.dui0417d/BBACIGAD.html
>
> RealView PB for Cortex-A9: BOARD_ID = 0x182 = BOARD_ID_PBX
> http://infocenter.arm.com/help/topic/com.arm.doc.dui0440b/CACCHBFB.html
>
> Motherboard Express µATX: BOARD_ID = 0x190 = BOARD_ID_VEXPRESS
> No reset register
>
> v2:
> - Add multiple boards support
> - fix coding style
> - Added a BOARD_ID descriptor for unsupported baseboards.
>
> Signed-off-by: Jean-Christophe DUBOIS <address@hidden>
> ---
>  hw/arm_sysctl.c |   43 ++++++++++++++++++++++++++++++++-----------
>  1 files changed, 32 insertions(+), 11 deletions(-)

I don't think you need to quote the URLs for every devboard TRM.

The 'v2/v3/etc' changes bit is better below the '---', ie not in
the final git commit message (people looking at git care only about
what the final patch does, not about what earlier versions failed
to do).

The Subject is kind of vague. Try:
"hw/arm_sysctl: Fix RESETCTL for realview-pb-a8 and -pbx-a9"

> diff --git a/hw/arm_sysctl.c b/hw/arm_sysctl.c
> index 17cf6f7..8f07fd5 100644
> --- a/hw/arm_sysctl.c
> +++ b/hw/arm_sysctl.c
> @@ -63,6 +63,8 @@ static const VMStateDescription vmstate_arm_sysctl = {
>  */
>  #define BOARD_ID_PB926 0x100
>  #define BOARD_ID_EB 0x140
> +#define BOARD_ID_PB1176 0x147
> +#define BOARD_ID_PB11MP 0x159

We don't support these boards (and I see no reason why we ever should)
so there is no point adding these constants or any code to deal with them.

> @@ -143,7 +145,8 @@ static uint64_t arm_sysctl_read(void *opaque, 
> target_phys_addr_t offset,
>     case 0x58: /* BOOTCS */
>         return 0;
>     case 0x5c: /* 24MHz */
> -        return muldiv64(qemu_get_clock_ns(vm_clock), 24000000, 
> get_ticks_per_sec());
> +        return muldiv64(qemu_get_clock_ns(vm_clock), 24000000,
> +                        get_ticks_per_sec());
>     case 0x60: /* MISC */
>         return 0;
>     case 0x84: /* PROCID0 */

Please don't fix coding style problems in bits of the code the patch is
not touching anyway. (Also applies to a few other bits of the patch.)

The actual meat of the patch looks generally OK.

thanks
-- PMM