[Skiboot] [PATCH v3] PCI: create optional loc-code platform callback
Oliver O'Halloran
oohall at gmail.com
Tue Feb 18 15:16:49 AEDT 2020
On Sat, Feb 15, 2020 at 1:18 AM Klaus Heinrich Kiwi
<klaus at linux.vnet.ibm.com> wrote:
>
> Some platforms (mostly OpenPower-based) will favor a short,
> slot-label-based string for the "ibm,loc-code" DT property. Other
> platforms such as ZZ/FSP-based platforms will prefer the fully-qualified
> slot-location-code for it.
>
> This patch creates a new optional callbacl on the platform struct,
> allowing platforms to create the "ibm,loc-code" property in their
> specific way. If the callback is not defined, default to the cleaned-up
> pci_add_loc_code() that was in use so far.
>
> Signed-off-by: Klaus Heinrich Kiwi <klaus at linux.vnet.ibm.com>
> ---
> core/pci.c | 70 +++++++++++----------------------
> include/platform.h | 8 ++++
> platforms/ibm-fsp/firenze-pci.c | 68 ++++++++++++++++++++++++++++++++
> platforms/ibm-fsp/firenze.c | 1 +
> platforms/ibm-fsp/ibm-fsp.h | 2 +
> platforms/ibm-fsp/zz.c | 1 +
> 6 files changed, 103 insertions(+), 47 deletions(-)
>
> diff --git a/core/pci.c b/core/pci.c
> index 8b52fc10..5170d30b 100644
> --- a/core/pci.c
> +++ b/core/pci.c
> @@ -1383,75 +1383,45 @@ void pci_std_swizzle_irq_map(struct dt_node *np,
> free(map);
> }
>
> -static void pci_add_loc_code(struct dt_node *np, struct pci_device *pd)
> +static void pci_add_loc_code(struct dt_node *np, struct pci_device *pd __unused)
Just remove the unused parameter. If someone needs it they can add it back.
> {
> struct dt_node *p = np->parent;
> - const char *blcode = NULL;
> - char *lcode;
> - uint32_t class_code;
> - uint8_t class, sub;
> - uint8_t pos, len;
> + const char *lcode = NULL;
>
> while (p) {
> - /* if we have a slot label (i.e. openpower) use that */
> - blcode = dt_prop_get_def(p, "ibm,slot-label", NULL);
> - if (blcode)
> + /* prefer slot-label by default */
> + lcode = dt_prop_get_def(p, "ibm,slot-label", NULL);
> + if (lcode)
> break;
>
> /* otherwise use the fully qualified location code */
> - blcode = dt_prop_get_def(p, "ibm,slot-location-code", NULL);
> - if (blcode)
> + lcode = dt_prop_get_def(p, "ibm,slot-location-code", NULL);
> + if (lcode)
> break;
>
> p = p->parent;
> }
>
> - if (!blcode)
> - blcode = dt_prop_get_def(np, "ibm,slot-location-code", NULL);
> + /* try the node itself if none is found */
> + if (!lcode)
> + lcode = dt_prop_get_def(np, "ibm,slot-location-code", NULL);
>
> - if (!blcode) {
> + if (!lcode) {
> /* Fall back to finding a ibm,loc-code */
> p = np->parent;
>
> while (p) {
> - blcode = dt_prop_get_def(p, "ibm,loc-code", NULL);
> - if (blcode)
> + lcode = dt_prop_get_def(p, "ibm,loc-code", NULL);
> + if (lcode)
> break;
> p = p->parent;
> }
> }
For future reference cleanups such as renaming variables should
probably go into a separate patch. I'm not that bothered if minor
cleanups are included in small patches, but this is one pretty large
already and the noise in the diff makes it harder to see what's going
on.
> - if (!blcode)
> + if (!lcode)
> return;
>
> - /* ethernet devices get port codes */
> - class_code = dt_prop_get_u32(np, "class-code");
> - class = class_code >> 16;
> - sub = (class_code >> 8) & 0xff;
> -
> - /* XXX Don't do that on openpower for now, we will need to sort things
> - * out later, otherwise the mezzanine slot on Habanero gets weird results
> - */
> - if (class == 0x02 && sub == 0x00 && !platform.bmc) {
> - /* There's usually several spaces at the end of the property.
> - Test for, but don't rely on, that being the case */
> - len = strlen(blcode);
> - for (pos = 0; pos < len; pos++)
> - if (blcode[pos] == ' ') break;
> - if (pos + 3 < len)
> - lcode = strdup(blcode);
> - else {
> - lcode = malloc(pos + 3);
> - memcpy(lcode, blcode, len);
> - }
> - lcode[pos++] = '-';
> - lcode[pos++] = 'T';
> - lcode[pos++] = (char)PCI_FUNC(pd->bdfn) + '1';
> - lcode[pos++] = '\0';
> - dt_add_property_string(np, "ibm,loc-code", lcode);
> - free(lcode);
> - } else
> - dt_add_property_string(np, "ibm,loc-code", blcode);
> + dt_add_property_string(np, "ibm,loc-code", lcode);
> }
>
> static void pci_print_summary_line(struct phb *phb, struct pci_device *pd,
> @@ -1594,8 +1564,14 @@ static void __noinline pci_add_one_device_node(struct phb *phb,
> dt_add_property_string(np, "ibm,slot-location-code",
> phb->base_loc_code);
>
> - /* Make up location code */
> - pci_add_loc_code(np, pd);
> + /*
> + * Use platform-specific callback (if there is one) to make up
> + * location code
> + */
Don't write comments like this. I know that it uses a platform
specific callback if one exists, because the code is right there.
Comments are for putting things in context and explaining why
something is happening. Repeating what the code says in prose can be
justifiable if it's something tricky is going on, but that's not the
case here.
> + if (platform.pci_get_loc_code)
> + platform.pci_get_loc_code(np, pd);
> + else
> + pci_add_loc_code(np, pd);
Why'd you change the name from _add_ to _get_? Add_loc_code makes more
sense to me since that's what the function actually does.
> /* XXX FIXME: We don't look for BARs, we only put the config space
> * entry in the "reg" property. That's enough for Linux and we might
> diff --git a/include/platform.h b/include/platform.h
> index 6909a6a4..22b5f6d2 100644
> --- a/include/platform.h
> +++ b/include/platform.h
> @@ -185,6 +185,14 @@ struct platform {
> void (*pci_get_slot_info)(struct phb *phb,
> struct pci_device *pd);
>
> + /*
> + * Called for each device during pci_add_device_nodes() descend
> + * to create the device tree, in order to get the correct per-platform
> + * preference for the ibm,loc-code property
> + */
> + void (*pci_get_loc_code)(struct dt_node *np,
> + struct pci_device *pd);
> +
> /*
> * Called after PCI probe is complete and before inventory is
> * displayed in console. This can either run platform fixups or
> diff --git a/platforms/ibm-fsp/firenze-pci.c b/platforms/ibm-fsp/firenze-pci.c
> index 6d5aab1e..bef69052 100644
> --- a/platforms/ibm-fsp/firenze-pci.c
> +++ b/platforms/ibm-fsp/firenze-pci.c
> @@ -972,3 +972,71 @@ void firenze_pci_get_slot_info(struct phb *phb, struct pci_device *pd)
> firenze_pci_slot_init(slot);
> }
> }
> +
> +void firenze_pci_get_loc_code(struct dt_node *np, struct pci_device *pd)
> +{
> + struct dt_node *p = np->parent;
> + const char *blcode = NULL;
> + char *lcode;
> + uint32_t class_code;
> + uint8_t class,sub;
> + uint8_t pos, len;
> +
> +
> + /*
> + * prefer fully-qualified slot-location-code, walk-up parent tree
> + * to find one
> + */
> + while (p) {
> + blcode = dt_prop_get_def(p, "ibm,slot-location-code", NULL);
> + if (blcode)
> + break;
> +
> + p = p-> parent;
stray space between the -> and parent.
Making this use a for loop instead wouldn't hurt either:
for (p = np->parent; p; p = np->parent) {
}
> + }
> +
> + /* try the node itself if none is found */
> + if (!blcode)
> + blcode = dt_prop_get_def(np, "ibm,slot-location-code", NULL);
> +
> + if (!blcode) {
> + /* still not found, fall back to ibm,loc-code */
> + p = np->parent;
> + while (p) {
> + blcode = dt_prop_get_def(p, "ibm,loc-code", NULL);
> + if (blcode)
> + break;
> +
> + p = p->parent;
> + }
> + }
> +
> + if (!blcode)
> + return;
Should this print an error? On IBM systems we should always have a
location code so if we're missing one it's probably a bug.
> + /* ethernet devices get port codes */
> + class_code = dt_prop_get_u32(np, "class-code");
> + class = class_code >> 16;
> + sub = (class_code >> 8) & 0xff;
> +
> + if (class == 0x02 && sub == 0x00) {
> + /* There's usually several spaces at the end of the property.
> + Test for, but don't rely on, that being the case */
> + len = strlen(blcode);
> + for (pos = 0; pos < len; pos++)
> + if (blcode[pos] == ' ') break;
> + if (pos + 3 < len)
> + lcode = strdup(blcode);
> + else {
> + lcode = malloc(pos + 3);
> + memcpy(lcode, blcode, len);
> + }
> + lcode[pos++] = '-';
> + lcode[pos++] = 'T';
> + lcode[pos++] = (char)PCI_FUNC(pd->bdfn) + '1';
> + lcode[pos++] = '\0';
> + dt_add_property_string(np, "ibm,loc-code", lcode);
> + free(lcode);
> + } else
> + dt_add_property_string(np, "ibm,loc-code", blcode);
There should be braces around the else block. We follow the Linux
convention where you either have braces around every branch or a chain
of single line if .. else if .. else statements with no braces.
> +}
> diff --git a/platforms/ibm-fsp/firenze.c b/platforms/ibm-fsp/firenze.c
> index 26977450..8fc72e9b 100644
> --- a/platforms/ibm-fsp/firenze.c
> +++ b/platforms/ibm-fsp/firenze.c
> @@ -206,6 +206,7 @@ DECLARE_PLATFORM(firenze) = {
> .cec_reboot = ibm_fsp_cec_reboot,
> .pci_setup_phb = firenze_pci_setup_phb,
> .pci_get_slot_info = firenze_pci_get_slot_info,
> + .pci_get_loc_code = firenze_pci_get_loc_code,
> .pci_probe_complete = firenze_pci_send_inventory,
> .nvram_info = fsp_nvram_info,
> .nvram_start_read = fsp_nvram_start_read,
> diff --git a/platforms/ibm-fsp/ibm-fsp.h b/platforms/ibm-fsp/ibm-fsp.h
> index 16af68f8..122bbb41 100644
> --- a/platforms/ibm-fsp/ibm-fsp.h
> +++ b/platforms/ibm-fsp/ibm-fsp.h
> @@ -29,6 +29,8 @@ extern void firenze_pci_setup_phb(struct phb *phb,
> unsigned int index);
> extern void firenze_pci_get_slot_info(struct phb *phb,
> struct pci_device *pd);
> +extern void firenze_pci_get_loc_code(struct dt_node *np,
> + struct pci_device *pd);
>
> /* VPD support */
> void vpd_iohub_load(struct dt_node *hub_node);
> diff --git a/platforms/ibm-fsp/zz.c b/platforms/ibm-fsp/zz.c
> index f4aa85fa..a218b328 100644
> --- a/platforms/ibm-fsp/zz.c
> +++ b/platforms/ibm-fsp/zz.c
> @@ -184,6 +184,7 @@ DECLARE_PLATFORM(zz) = {
> .cec_reboot = ibm_fsp_cec_reboot,
> .pci_setup_phb = firenze_pci_setup_phb,
> .pci_get_slot_info = firenze_pci_get_slot_info,
> + .pci_get_loc_code = firenze_pci_get_loc_code,
> .pci_probe_complete = firenze_pci_send_inventory,
> .nvram_info = fsp_nvram_info,
> .nvram_start_read = fsp_nvram_start_read,
> --
> 2.17.1
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
More information about the Skiboot
mailing list