spufs platform dependent code

[Arnd Bergmann]

On Friday 27 January 2006 01:22, Geoff Levand wrote:
> We need to consider how we will arrange the platform
> dependent code in spufs.
> 
> First, we need to consider the run-time selection of
> spu wrappers, etc. You seemed to think that a simple
> comparison, as here, will be sufficient:
> 
>  if (firmware_has_feature(FW_FEATURE_HYPERVISOR))
>         hvcall_spu_get_irq_mask(spu->spu_magical_id, cls, &val);
>  else
>         val = in_be64(&spu->priv1->int_stat_class0_RW + cls);
> 
> I see a problem in that in the general case, support for several
> hypervisors and also 'raw hardware' will be compiled in.  I was
> thinking it might work better to use an indirect call through a
> pointer to a structure of function pointers.

In most cases we have now, the code above should be pretty well
optimized because firmware_has_feature() can be evaluated at compile
time depending on CONFIG_* and no conditional branch gets added here.

I agree that in the long term, we may want to optimize for the case
where we have a single kernel binary that has all possible options
enabled, which would work better with a function pointer.
 
>  struct spu_ops *spu_ops;
>  spu_ops->spu_get_irq_mask(spu->spu_magical_id, cls, &val);
> 
> The instance of the structure could be setup statically in
> the platform code and the pointer set with a runtime check
> or in the platform startup code.

The standard way to do this kind of OO abstraction would pass
'spu' as the first argument to spu_get_irq_mask() instead of
spu->spu_magical_id. Also, you can leave out the 'spu_'
prefix in the struct member.

If you want to prepare a patch for this, I'd prefer you to
write this as

val = spu->ops.get_irq_mask(spu, cls);

or

val = spu_ops->get_irq_mask(spu, cls);

if the latter turns out to be more efficient. I don't particularly
like having a new global function pointer structure, but I don't
really expect to see systems where it makes sense to have different
->ops pointers per spu structure.

> Another point is that there is some platform dependent interrupt
> and spu setup code in spu_base.c.  This needs to be moved
> out and into a platform dependent file. We currently have 
> spu_priv1.c with the platform dependent wrappers.  I
> propose we rename spu_priv1.c to something
> that hints that is the platform code for 'raw' access,
> and move the platform code in spu_base.c to it.  Do you
> have any recommended names, or any other suggestions?

I'm not sure I could follow you. My idea of the cell platform
code is that it ought to be generic enough to work with all
hardware that has some SPU support in it. Right now, a lot of
stuff is hardwired to use the stuff we have on the Cell Blade,
but that only means that so far nobody has submitted a patch
to extend this to any other hardware.

Did I get you right that you instead want to add a separate
platform directory for your hypervisor abstraction?
Since I haven't seen your code, I don't know how much it differs
from what we have so far and can't really tell which would be
better.

For the spu_priv1.c file, it's proably easiest to do it similar
to the abstraction we already have for spu_context_ops, the files
could e.g. be named spu_priv1_mmio.c and spu_priv1_hvcall.c.

	Arnd <><