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

[Alexey Kardashevskiy]

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.


> ---
>   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,
> +};
> 

-- 
Alexey