[v2,8/8] Add Swift platform

Signed-off-by: Reza Arbab <arbab@linux.ibm.com>
---
 platforms/astbmc/Makefile.inc |   2 +-
 platforms/astbmc/swift.c      | 143 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 144 insertions(+), 1 deletion(-)
 create mode 100644 platforms/astbmc/swift.c

On 09/07/2019 07:07, Reza Arbab wrote:
> Signed-off-by: Reza Arbab <arbab@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.


> ---
>   platforms/astbmc/Makefile.inc |   2 +-
>   platforms/astbmc/swift.c      | 143 ++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 144 insertions(+), 1 deletion(-)
>   create mode 100644 platforms/astbmc/swift.c
> 
> diff --git a/platforms/astbmc/Makefile.inc b/platforms/astbmc/Makefile.inc
> index 5130275debce..65a8b46eae8e 100644
> --- a/platforms/astbmc/Makefile.inc
> +++ b/platforms/astbmc/Makefile.inc
> @@ -6,7 +6,7 @@ ASTBMC_OBJS = pnor.o common.o slots.o \
>   	      garrison.o barreleye.o \
>   	      witherspoon.o zaius.o romulus.o p9dsu.o \
>   	      vesnin.o nicole.o \
> -	      talos.o
> +	      talos.o swift.o
>   
>   ASTBMC = $(PLATDIR)/astbmc/built-in.a
>   $(ASTBMC): $(ASTBMC_OBJS:%=$(PLATDIR)/astbmc/%)
> diff --git a/platforms/astbmc/swift.c b/platforms/astbmc/swift.c
> new file mode 100644
> index 000000000000..78aa4e33cc76
> --- /dev/null
> +++ b/platforms/astbmc/swift.c
> @@ -0,0 +1,143 @@
> +/* Copyright 2019 IBM Corp.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at
> + *
> + *	http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
> + * implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <skiboot.h>
> +#include <ipmi.h>
> +#include <npu3.h>
> +#include "astbmc.h"
> +
> +static void swift_npu3_device_detect(struct npu3 *npu)
> +{
> +	struct npu3_dev *dev;
> +	uint32_t node, gpu_index;
> +	char slot[6];
> +
> +	node = P9_GCID2NODEID(npu->chip_id);
> +
> +	switch (npu->index) {
> +	case 0:
> +		gpu_index = node * 2 + 1;
> +		break;
> +	case 2:
> +		gpu_index = node * 2;
> +		break;
> +	default:
> +		return;
> +	}
> +
> +	snprintf(slot, sizeof(slot), "GPU%d", gpu_index);
> +
> +	npu3_for_each_dev(dev, npu) {
> +		dev->type = NPU3_DEV_TYPE_NVLINK;
> +		dt_add_property_string(dev->dn, "ibm,slot-label", slot);
> +		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,


> +	}
> +}
> +
> +#define SWIFT_POSSIBLE_GPUS 4
> +
> +#define DN(g)	devs[g]->nvlink.gpu->dn

This one does not seem very useful, can be open coded.

> +#define G(g)	(devs[g] ? devs[g]->nvlink.gpu->dn->phandle : 0)
> +#define N(g)	(devs[g] ? devs[g]->npu->nvlink.phb.dt_node->phandle : 0)
> +
> +#define add_peers_prop(g, p...)						\
> +	if (devs[g])  							\
> +		dt_add_property_cells(DN(g), "ibm,nvlink-peers", ##p)
> +
> +/* Add GPU interconnect properties to the dt */
> +static void swift_npu3_fixup(void)
> +{
> +	struct npu3 *npu;
> +	struct npu3_dev *dev;
> +	struct npu3_dev *devs[SWIFT_POSSIBLE_GPUS] = {};
> +	uint32_t index;
> +
> +	if (npu3_chip_possible_gpus() != 2) {
> +		prlog(PR_ERR, "NPU: Unknown link topology detected\n");
> +		return;
> +	}
> +
> +	/* Collect the first link we find for each GPU */
> +	npu3_for_each_nvlink_npu(npu) {
> +		npu3_for_each_nvlink_dev(dev, npu) {
> +			index = npu3_dev_gpu_index(dev);
> +			if (index == -1 || index >= ARRAY_SIZE(devs))
> +				continue;
> +
> +			if (dev->nvlink.gpu && !devs[index])
> +				devs[index] = dev;
> +		}
> +	}
> +
> +	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 :) Also, this looks like some GPUs 
only have 2 links between them and some have 3, is that correct?


> +
> +	add_peers_prop(1, G(2),
> +			  G(3), G(3), G(3),
> +			  G(0), G(0), G(0),
> +			  G(2),
> +			  N(1), N(1), N(1), N(1));
> +
> +	add_peers_prop(2, G(1),
> +			  G(3), G(3), G(3),
> +			  G(0), G(0), G(0),
> +			  G(1),
> +			  N(2), N(2), N(2), N(2));
> +
> +	add_peers_prop(3, G(2), G(2), G(2),
> +			  G(1), G(1), G(1),
> +			  G(0), G(0),
> +			  N(3), N(3), N(3), N(3));
> +}
> +
> +static void swift_exit(void)
> +{
> +	swift_npu3_fixup();
> +	astbmc_exit();
> +}
> +
> +static bool swift_probe(void)
> +{
> +	if (!dt_node_is_compatible(dt_root, "ibm,swift"))
> +		return false;
> +
> +	/* Lot of common early inits here */
> +	astbmc_early_init();
> +
> +	/* Setup UART for use by OPAL (Linux hvc) */
> +	uart_set_console_policy(UART_CONSOLE_OPAL);
> +
> +	return true;
> +}
> +
> +DECLARE_PLATFORM(swift) = {
> +	.bmc			= &bmc_plat_ast2500_openbmc,
> +	.cec_power_down		= astbmc_ipmi_power_down,
> +	.cec_reboot		= astbmc_ipmi_reboot,
> +	.elog_commit		= ipmi_elog_commit,
> +	.exit			= swift_exit,
> +	.init			= astbmc_init,
> +	.name			= "Swift",
> +	.npu3_device_detect	= swift_npu3_device_detect,
> +	.pci_get_slot_info	= dt_slot_get_slot_info,
> +	.probe			= swift_probe,
> +	.resource_loaded	= flash_resource_loaded,
> +	.start_preload_resource	= flash_start_preload_resource,
> +	.terminate		= ipmi_terminate,
> +};
>

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@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. :)

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

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

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

That is correct.

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