spufs platform dependent code

[Geoff Levand]

Arnd Bergmann wrote:
> 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.


I started some work on this.   I'll post a patch for comment, maybe
next week.


>>�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.


Yes, you are of course correct, I just cut and pasted
that merely for illustration.


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


I can't see a system having different ops either.  I'll see what
code is generated by each.


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


I found you have some stuff in there specific to your platform.
It assumes there is a certain south bridge, and that spu
interrupts are serviced by that south bridge.  It also makes
assumptions of how the spu's are represented in the device tree.

I've got a first cut patch to break those things out.  I'll post
it soon for comment.


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


Sorry for the confusion, what I meant was that if we move the
interrupt code, etc. to spu_priv1.c, it becomes more than just
priv1 wrappers, so I thought it might be better to name it
something else.

I was also thinking to put support for all platforms into
arch/powerpc/platforms/cell, each in a different file.  But
on consideration it might work better to put them into a
separate directory.  Let's not decide that now though, and
just focus on splitting out the platform code from spu_base.c
and doing the wrappers.

-Geoff