[Skiboot] [PATCH v2 8/8] Add Swift platform

[Alexey Kardashevskiy]

On 11/07/2019 02:33, Reza Arbab wrote:
> On Wed, Jul 10, 2019 at 12:26:59PM +1000, Alexey Kardashevskiy wrote:
>> On 09/07/2019 07:07, Reza Arbab wrote:
>>> Signed-off-by: Reza Arbab <arbab at linux.ibm.com>
>>
>> No commit log at all?
>>
>> What is that "swift"? How many/which model/DD cpus, npus, gpus, any 
>> mention of any spec, anything here or in the swift.c header comment 
>> will do.
> 
> My bad. I'll add a description of the system.
> 
> In my defense I'll just offer that the first commit of witherspoon.c has 
> nothing but an ASCII art portrait of Reese Witherspoon. :)

but you did not do even that!!! ;)


> 
>>> +        dt_add_property_u64(dev->dn, "ibm,link-speed", 25000000000ull);
>>> +        dt_add_property_cells(dev->dn, "nvidia,link-speed", 9);
>>
>> I have always been curious - what are these "25000000000ull" and "9" 
>> and where do they come from and can we have them defined? Thanks,
> 
> There are references elsewhere, like hdata/spira.c:
> 
>      /* ibm,link-speed is in bps and nvidia,link-speed is ~magic~ */
> 
> and doc/device-tree/nvlink.rst:
> 
>      ; Denotes the speed the link is running at:
>      ; 0x3 == 20 Gbps, 0x8 = 25.78125 Gbps, 0x9 == 25.00000 Gbps
> 
> I'll add some #defines to make this explicit and use one of them instead 
> of open coding 9.


oh great, please do. I came across systems with 8 and 9 and it took me 
some time to realize that these numbers do matter.


> 
>>> +#define DN(g)    devs[g]->nvlink.gpu->dn
>>
>> This one does not seem very useful, can be open coded.
> 
> Yeah, it only gets used once. I'll drop it.
> 
>>> +    add_peers_prop(0, G(3), G(3),
>>> +              G(2), G(2), G(2),
>>> +              G(1), G(1), G(1),
>>> +              N(0), N(0), N(0), N(0));
>>
>> 2, 3, 3, 4 items per line and the next one 1, 3, 3, 1, 4 - what is the 
>> point in such grouping? There must be something about the code 
>> readability but I just do not see it :)
> 
> I thought it might help to put consecutive links going to the same place 
> on the same line. No worries, I'll just use two lines instead, one line 
> for links to other GPUs, one line for links to the NPU.


I do not mind either way, I was just wondering if it is something like 
"these are grouped into yetanotherbricks" or some other new and weird 
things. Thanks,


>> Also, this looks like some GPUs only have 2 links between them and 
>> some have 3, is that correct?
> 
> That is correct.
> 

-- 
Alexey