Re: [Xen-devel] [PATCH v3 7/8] xen/arm: introduce vexpress_syscfg

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Tue, 2012-12-18 at 18:46 +0000, Stefano Stabellini wrote:
> Introduce a Versatile Express specific function to read/write
> motherboard settings.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> ---
>  xen/arch/arm/Makefile                   |    1 +
>  xen/arch/arm/platform_vexpress.c        |   97 
> +++++++++++++++++++++++++++++++
>  xen/include/asm-arm/platform_vexpress.h |   23 +++++++

I wonder if we ought to start a "platforms" subdirs include and arch?
This is presumably the first of many. Going the plat-<foo> route seems
unnecessary at least at this stage.

> +#include <asm/platform_vexpress.h>
> +#include <xen/mm.h>
> +
> +#define DCC_SHIFT      26
> +#define FUNCTION_SHIFT 20
> +#define SITE_SHIFT     16
> +#define POSITION_SHIFT 12
> +#define DEVICE_SHIFT   0
> +
> +int vexpress_syscfg(int write, int function, int device, uint32_t *data)
> +{
> +    uint32_t *syscfg = (uint32_t *) FIXMAP_ADDR(FIXMAP_MISC);
> +    uint32_t stat;
> +    int dcc = 0; /* DCC to access */
> +    int site = 0; /* motherboard */
> +    int position = 0; /* motherboard */

motherboard twice?

> +    set_fixmap(FIXMAP_MISC, V2M_SYS_MMIO_BASE >> PAGE_SHIFT, DEV_SHARED);
> +
> +    if ( syscfg[V2M_SYS_CFGCTRL] & V2M_SYS_CFG_START )
> +        return -1;
> +
> +    /* clear the complete bit in the V2M_SYS_CFGSTAT status register */

Do you mean clear all the bits or specifically the "completion" bit?

> +    syscfg[V2M_SYS_CFGSTAT] = 0;
> +
> +    if ( write )
> +    {
> +        /* write data */
> +        syscfg[V2M_SYS_CFGDATA] = *data;
> +
> +        /* set control register */
> +        syscfg[V2M_SYS_CFGCTRL] = V2M_SYS_CFG_START | V2M_SYS_CFG_WRITE |
> +            (dcc << DCC_SHIFT) | (function << FUNCTION_SHIFT) |
> +            (site << SITE_SHIFT) | (position << POSITION_SHIFT) |
> +            (device << DEVICE_SHIFT);

Most of this shifting is repeated below, perhaps do it once into a local
var to avoid them getting out of sync?

> +
> +        /* wait for complete flag to be set */
> +        do {
> +            stat = syscfg[V2M_SYS_CFGSTAT];
> +            dsb();
> +        } while ( !(stat & V2M_SYS_CFG_COMPLETE) );

I assume there's no wait-for-event or way to relax this spin loop?

Since this is repeated below a helper "wait_for_ocmplet" might be good.

Actually, this while bit "set control register" until the error check is
common to both the read and write cases, isn't it?

> +
> +        /* check error status and return error flag if set */
> +        if ( stat & V2M_SYS_CFG_ERROR )
> +            return -1;
> +    } else {
> +        /* set control register */
> +        syscfg[V2M_SYS_CFGCTRL] = V2M_SYS_CFG_START | (dcc << DCC_SHIFT) |
> +            (function << FUNCTION_SHIFT) | (site << SITE_SHIFT) |
> +            (position << POSITION_SHIFT) | (device << DEVICE_SHIFT);
> +
> +        /* wait for complete flag to be set */
> +        do {
> +            stat = syscfg[V2M_SYS_CFGSTAT];
> +            dsb();
> +        } while ( !(stat & V2M_SYS_CFG_COMPLETE) );
> +
> +        /* check error status flag and return error flag if set */
> +        if ( stat & V2M_SYS_CFG_ERROR )
> +            return -1;
> +        else
> +            /* read data */
> +            *data = syscfg[V2M_SYS_CFGDATA];
> +    }
> +
> +    clear_fixmap(FIXMAP_MISC);
> +
> +    return 0;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-set-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/asm-arm/platform_vexpress.h 
> b/xen/include/asm-arm/platform_vexpress.h
> index 3556af3..407602d 100644
> --- a/xen/include/asm-arm/platform_vexpress.h
> +++ b/xen/include/asm-arm/platform_vexpress.h
> @@ -6,6 +6,29 @@
>  #define V2M_SYS_FLAGSSET      (0x30)
>  #define V2M_SYS_FLAGSCLR      (0x34)
>  
> +#define V2M_SYS_CFGDATA       (0x00A0/4)
> +#define V2M_SYS_CFGCTRL       (0x00A4/4)
> +#define V2M_SYS_CFGSTAT       (0x00A8/4)

It'd be better to either define all registers in bytes (as the existing
FLAGS*) or words  (the new ones) and not to mix the two...

> +
> +#define V2M_SYS_CFG_START     (1<<31)
> +#define V2M_SYS_CFG_WRITE     (1<<30)
> +#define V2M_SYS_CFG_ERROR     (1<<1)
> +#define V2M_SYS_CFG_COMPLETE  (1<<0)
> +
> +#define V2M_SYS_CFG_OSC_FUNC  1
> +#define V2M_SYS_CFG_OSC0      0
> +#define V2M_SYS_CFG_OSC1      1
> +#define V2M_SYS_CFG_OSC2      2
> +#define V2M_SYS_CFG_OSC3      3
> +#define V2M_SYS_CFG_OSC4      4
> +#define V2M_SYS_CFG_OSC5      5
> +
> +#ifndef __ASSEMBLY__
> +#include <xen/inttypes.h>
> +
> +int vexpress_syscfg(int write, int function, int device, uint32_t *data);
> +#endif
> +
>  #endif /* __ASM_ARM_PLATFORM_H */
>  /*
>   * Local variables:



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel