[Cbe-oss-dev] [PATCH 3/6] cell: add hardcoded spu vicinity information for CBPW

[André Detsch]

Arnd Bergmann wrote:
> The name of the platform is not CPBW, but QS20, we should really
> call it that, both in the comments and in the code.

Ok.

>> +static void init_aff_CPBW_harcoded(void)
>> +{
>> +       int node, i;
>> +       struct spu *last_spu, *spu;
>> +       u32 reg;
>> +
>> +       for (node = 0; node < MAX_NUMNODES; node++) {
>> +               last_spu = NULL;
>> +               for (i = 0; i < SPES_PER_BE; i++) {
>> +                       spu = spu_lookup(node, "reg", CPBW_reg_idxs[i]);
>> +                       if (!spu)
>> +                               continue;
>> +                       reg = *(u32 *)get_property(spu->devnode, "reg", NULL);
>> +                       spu->has_mem_affinity = CPBW_reg_memory[reg];
>> +                       if (last_spu)
>> +                               list_add_tail(&spu->aff_list,
>> +                                               &last_spu->aff_list);
>> +                       last_spu = spu;
>> +               }
>> +       }
>> +}
> 
> Hmm, not sure if this is safe. The meaning of the "reg" property has
> changed in current firmware. While this is currently not shipping
> to customers, there is an internal version of the QS20 firmware that
> puts the register addresses into the "reg" properties, instead of the
> spu numbers. If the above code ever runs on such a system, it will get
> overrun the CPBW_reg_idxs array and bad things happen.
> 
> You can probably assume that the new firmware would have proper
> vicinity information though.

The code should not be called on newer platforms, but I'll add range 
checking there anyway. It's certainly not a good idea to blindly rely on 
FW data as array index.

>> @@ -696,6 +738,11 @@ static int __init init_spu_base(void)
>>  
>>         xmon_register_spus(&spu_full_list);
>>  
>> +       root = of_get_flat_dt_root();
>> +       if (of_flat_dt_is_compatible(root, "IBM,CPBW-1.0") ||
>> +           of_flat_dt_is_compatible(root, "IBM,CPBW-SystemSim"))
>> +               init_aff_CPBW_harcoded();
>> +
> 
> I'd suggest that you first check if the properties are there,
> before you search for the hack to do on IBM,CPBW-1.0.

Ok.

> I'm not really sure how to handle SystemSim. Does it even simulate
> different access patterns based on the location of the SPU? If not,
> we should not try to fake anything here. If anything, it should
> be up to the simulation to create the vicinity properties for
> the simulated hardware, based on user decisions.

I was using SystemSim for testing, but I don't think it simulates 
different access patterns based on SPU location. I don't see much 
problem on having affinity. Should the 'of_flat_dt_is_compatible(root, 
"IBM,CPBW-SystemSim")' condition be removed from the code then?