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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 19 Dec 2012, Ian Campbell wrote:
> 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.

I can move these two files in a subdirectory, but until we have at
least another platform is going to be difficult to come up with a decent
abstraction.


> > +#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?

the completion bit but there isn't much else on that register, so I
think it is OK to zero it.


> > +    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?

Nope


> 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?

I'll refactor them both into a separate function

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