[Skiboot] [PATCH] npu2-opencapi: Add opencapi support on ZZ

Frederic Barrat fbarrat at linux.ibm.com
Tue Jul 9 19:10:09 AEST 2019


Thanks for the review, I'll send a new version soon.
And thanks for openening my eyes on some of those dt APIs.

   Fred


Le 09/07/2019 à 06:03, Oliver O'Halloran a écrit :
> On Mon, Jul 8, 2019 at 8:30 PM Frederic Barrat <fbarrat at linux.ibm.com> wrote:
>>
>> This patch adds opencapi support on ZZ. It hard-codes the required
>> device tree entries for the NPU and links. The alternative was to use
>> HDAT, but it somehow proved to painful to do.
> 
> I know that feeling...
> 
>> The new device tree entries activate the npu2 init code on ZZ. On
>> systems with no opencapi adapters, it should go unnoticed, as presence
>> detection will skip link training.
>>
>> Signed-off-by: Frederic Barrat <fbarrat at linux.ibm.com>
>> ---
>>
>> Stewart, Oliver, Vasant: it's hard to see clearly in the ever changing
>> plan, but opencapi support may be desired for FW940. So hopefully this
>> patch can be merged in whatever skiboot branch that fw will use (is
>> that decided yet?).
>>
>>
>>   hdata/spira.c          |  8 +++-
>>   platforms/ibm-fsp/zz.c | 96 +++++++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 102 insertions(+), 2 deletions(-)
>>
>> diff --git a/hdata/spira.c b/hdata/spira.c
>> index 6891a9c7..c330a1fa 100644
>> --- a/hdata/spira.c
>> +++ b/hdata/spira.c
>> @@ -1369,7 +1369,13 @@ static void add_npu(struct dt_node *xscom, const struct HDIF_array_hdr *links,
>>                  uint64_t speed = 0, nvlink_speed = 0;
>>                  struct dt_node *node;
>>
>> -               /* only add a link node if this link is targeted at at device */
>> +               /*
>> +                * only add a link node if this link is targeted at a
>> +                * (GPU) device.
>> +                * If we ever activate it for an opencapi device,
>> +                * we'll need to revisit the link definitions
>> +                * hard-coded on ZZ
>> +                */
>>                  if (be32_to_cpu(link->usage) != SMP_LINK_USE_DEVICE)
>>                          continue;
>>
>> diff --git a/platforms/ibm-fsp/zz.c b/platforms/ibm-fsp/zz.c
>> index f44c618c..55078003 100644
>> --- a/platforms/ibm-fsp/zz.c
>> +++ b/platforms/ibm-fsp/zz.c
>> @@ -45,6 +45,97 @@ static const struct platform_ocapi zz_ocapi = {
>>          .odl_phy_swap        = true,
>>   };
>>
>> +#define NPU_BASE 0x5011000
>> +#define NPU_SIZE 0x2c
>> +#define NPU_INDIRECT0  0x8000000009010c3f /* OB0 - no OB3 on ZZ */
>> +
>> +static void create_link(struct dt_node *npu, int group, int index)
>> +{
>> +       struct dt_node *link;
>> +       uint32_t lane_mask;
>> +       char namebuf[32];
>> +
>> +       snprintf(namebuf, sizeof(namebuf), "link@%x", index);
>> +       link = dt_new(npu, namebuf);
>> +
>> +       dt_add_property_string(link, "compatible", "ibm,npu-link");
>> +       dt_add_property_cells(link, "ibm,npu-link-index", index);
>> +
>> +       switch (index) {
>> +       case 2:
>> +               lane_mask = 0xf1e000; /* 0-3, 7-10 */
>> +               break;
>> +       case 3:
>> +               lane_mask = 0x00078f; /* 13-16, 20-23 */
>> +               break;
>> +       default:
>> +               assert(0);
>> +       }
>> +
>> +       dt_add_property_u64s(link, "ibm,npu-phy", NPU_INDIRECT0);
>> +       dt_add_property_cells(link, "ibm,npu-lane-mask", lane_mask);
>> +       dt_add_property_cells(link, "ibm,npu-group-id", group);
>> +       dt_add_property_u64s(link, "ibm,link-speed", 25000000000ul);
>> +}
>> +
> 
>> +static void zz_opencapi_setup(void)
> 
> Call it add_opencapi_dt_nodes() or something. this isn't setting up anything.
> 
>> +{
>> +       struct dt_node *npu, *xscom;
>> +       int npu_index = 0;
>> +       int phb_index = 7;
>> +       struct dt_property *prop;
>> +       char namebuf[32];
> Reverse
> christmas
> tree
> declarations
> 
> wait, sorry:
> 
> declarations
> christmas
> Reverse
> tree
> 
>> +
>> +       /*
>> +        * In an ideal world, we should get all the NPU links
>> +        * information from HDAT. But after some effort, HDAT is still
>> +        * giving surprising information for opencapi.
> 
> Can you be a little more specific about what "surprising" means? At
> some point we'll want to remove this and without more context than
> "it's broken" it's hard to judge when that's safe to do so.
> 
>> +        * As a consequence:
>> +        * 1. the hdat parsing code in skiboot remains disabled (for
>> +        *    opencapi)
>> +        * 2. we hard-code the NPU and links entries in the device
>> +        *    tree.
>> +        *
>> +        * Getting the data from HDAT would have the advantage of
>> +        * providing the real link speed (20.0 vs. 25.78125 gbps),
>> +        * which is useful as there's one speed-dependent setting we
>> +        * need to do when initializing the NPU. Our hard coded
>> +        * definition assumes the higher speed and may need tuning in
>> +        * debug scenario using a lower link speed.
>> +        */
>> +       snprintf(namebuf, sizeof(namebuf), "npu@%x", NPU_BASE);
>> +       dt_for_each_compatible(dt_root, xscom, "ibm,xscom") {
>> +               npu = dt_find_by_name(xscom, namebuf);
> 
> use dt_find_by_name_addr() and you can drop namebuf.
> 
>> +               if (npu) {
>> +                       /*
>> +                        * our hdat parsing code may create NPU nodes
>> +                        * with no links, so we just refresh some
>> +                        * properties if needed
>> +                        */
>> +                       prop = __dt_find_property(npu, "ibm,npu-index");
>> +                       if (prop)
>> +                               dt_del_property(npu, prop);
>> +                       prop = __dt_find_property(npu, "ibm,phb-index");
>> +                       if (prop)
>> +                               dt_del_property(npu, prop);
>> +                       prop = __dt_find_property(npu, "ibm,npu-links");
>> +                       if (prop)
>> +                               dt_del_property(npu, prop);
> 
> Is there anything in the npu node you keep? Why not just nuke the
> whole thing with dt_free(npu) and re-create it from scratch? If HDAT
> is "fixed" enough to start creating link nodes we might run into
> problems since what gets created based on HDAT may might clash with
> what you're adding here which will probably break booting.
> 
> Also, dt_check_del_prop() does this
> find-and-delete-property-if-it-exists. It has a stupid name because I
> couldn't think of a better one.
> 
>> +               } else {
>> +                       npu = dt_new(xscom, namebuf);
> dt_new_addr()
> 
>> +                       dt_add_property_cells(npu, "reg", NPU_BASE, NPU_SIZE);
>> +                       dt_add_property_strings(npu, "compatible",
>> +                                               "ibm,power9-npu");
>> +               }
>> +               dt_add_property_cells(npu, "ibm,npu-index", npu_index++);
>> +               dt_add_property_cells(npu, "ibm,phb-index", phb_index++);
>> +               dt_add_property_cells(npu, "ibm,npu-links", 2);
>> +
>> +               create_link(npu, 1, 2);
>> +               create_link(npu, 2, 3);
>> +       }
>> +}
>> +
>>   static bool zz_probe(void)
>>   {
>>          /* FIXME: make this neater when the dust settles */
>> @@ -54,8 +145,11 @@ static bool zz_probe(void)
>>              dt_node_is_compatible(dt_root, "ibm,zz-2s4u") ||
>>              dt_node_is_compatible(dt_root, "ibm,zz-1s4u+gen4") ||
>>              dt_node_is_compatible(dt_root, "ibm,zz-2s2u+gen4") ||
>> -           dt_node_is_compatible(dt_root, "ibm,zz-2s4u+gen4"))
>> +           dt_node_is_compatible(dt_root, "ibm,zz-2s4u+gen4")) {
>> +
>> +               zz_opencapi_setup();
>>                  return true;
>> +       }
>>
>>          return false;
>>   }
>> --
>> 2.21.0
>>
> 



More information about the Skiboot mailing list