[Skiboot] [PATCH] platform/mihawk: support dynamic PCIe slot table

Joy_Chu at wistron.com Joy_Chu at wistron.com
Tue Mar 24 21:17:00 AEDT 2020


Send my question again as below.
>
> Slot table auto-detection for different riser cards by using IPMI OEM 
> command to communicate with BMC.
>
> Signed-off-by: Joy Chu <joy_chu at wistron.com>
> ---
>  platforms/astbmc/mihawk.c | 229 
> +++++++++++++++++++++++++++++++++++---
>  1 file changed, 214 insertions(+), 15 deletions(-)
>
> diff --git a/platforms/astbmc/mihawk.c b/platforms/astbmc/mihawk.c 
> index d33d16bb..28c999aa 100644
> --- a/platforms/astbmc/mihawk.c
> +++ b/platforms/astbmc/mihawk.c
> @@ -15,8 +15,16 @@
>  #include <pci.h>
>  #include <pci-cfg.h>
>
> +#include <timebase.h>
> +
>  #include "astbmc.h"
>
> +/* IPMI message code for Riser-F query (OEM). */
> +#define IPMI_RISERF_QUERY      IPMI_CODE(0x32, 0x01)
> +
> +static bool mihawk_riserF_found = false; static bool 
> +bmc_query_waiting = false;
> +
>  /* nvme backplane slots */
>  static const struct slot_table_entry hdd_bay_slots[] = {
>          SW_PLUGGABLE("hdd0", 0x0),
> @@ -91,7 +99,7 @@ static const struct platform_ocapi mihawk_ocapi = {
>         .ocapi_slot_label    = mihawk_ocapi_slot_label,
>  };
>
> -static const struct slot_table_entry P1E1A_x8_PLX8748_down[] = {
> +static const struct slot_table_entry P1E1A_x8_PLX8748_RiserA_down[] = 
> +{
>          SW_PLUGGABLE("Slot7", 0x10),
>          SW_PLUGGABLE("Slot8", 0x8),
>          SW_PLUGGABLE("Slot10", 0x9),
> @@ -99,25 +107,25 @@ static const struct slot_table_entry P1E1A_x8_PLX8748_down[] = {
>          { .etype = st_end }
>  };
>
> -static const struct slot_table_entry P1E1A_x8_PLX8748_up[] = {
> +static const struct slot_table_entry P1E1A_x8_PLX8748_RiserA_up[] = {
>          {
>                  .etype = st_builtin_dev,
>                  .location = ST_LOC_DEVFN(0,0),
> -                .children = P1E1A_x8_PLX8748_down,
> +                .children = P1E1A_x8_PLX8748_RiserA_down,
>          },
>          { .etype = st_end }
>  };
>
> -static const struct slot_table_entry p1phb1_slot[] = {
> +static const struct slot_table_entry p1phb1_rA_slot[] = {
>          {
>                  .etype = st_builtin_dev,
>                  .location = ST_LOC_DEVFN(0,0),
> -                .children = P1E1A_x8_PLX8748_up,
> +                .children = P1E1A_x8_PLX8748_RiserA_up,
>          },
>          { .etype = st_end },
>  };
>
> -static const struct slot_table_entry P0E1A_x8_PLX8748_down[] = {
> +static const struct slot_table_entry P0E1A_x8_PLX8748_RiserA_down[] = 
> +{
>          SW_PLUGGABLE("Slot2", 0x10),
>          SW_PLUGGABLE("Slot3", 0x8),
>          SW_PLUGGABLE("Slot5", 0x9),
> @@ -125,20 +133,120 @@ static const struct slot_table_entry P0E1A_x8_PLX8748_down[] = {
>          { .etype = st_end }
>  };
>
> -static const struct slot_table_entry P0E1A_x8_PLX8748_up[] = {
> +static const struct slot_table_entry P0E1A_x8_PLX8748_RiserA_up[] = {
> +        {
> +                .etype = st_builtin_dev,
> +                .location = ST_LOC_DEVFN(0,0),
> +                .children = P0E1A_x8_PLX8748_RiserA_down,
> +        },
> +        { .etype = st_end }
> +};
> +
> +static const struct slot_table_entry p0phb1_rA_slot[] = {
> +        {
> +                .etype = st_builtin_dev,
> +                .location = ST_LOC_DEVFN(0,0),
> +                .children = P0E1A_x8_PLX8748_RiserA_up,
> +        },
> +        { .etype = st_end },
> +};
> +
> +static const struct slot_table_entry P1E1A_x8_PLX8748_RiserF_down[] = {
> +        SW_PLUGGABLE("Slot7", 0x10),
> +        SW_PLUGGABLE("Slot10", 0x9),
> +
> +        { .etype = st_end }
> +};
> +
> +static const struct slot_table_entry P1E1A_x8_PLX8748_RiserF_up[] = {
> +        {
> +                .etype = st_builtin_dev,
> +                .location = ST_LOC_DEVFN(0,0),
> +                .children = P1E1A_x8_PLX8748_RiserF_down,
> +        },
> +        { .etype = st_end }
> +};
> +
> +static const struct slot_table_entry p1phb1_rF_slot[] = {
> +        {
> +                .etype = st_builtin_dev,
> +                .location = ST_LOC_DEVFN(0,0),
> +                .children = P1E1A_x8_PLX8748_RiserF_up,
> +        },
> +        { .etype = st_end },
> +};
> +
> +static const struct slot_table_entry P0E1A_x8_PLX8748_RiserF_down[] = {
> +        SW_PLUGGABLE("Slot2", 0x10),
> +        SW_PLUGGABLE("Slot5", 0x9),
> +
> +        { .etype = st_end }
> +};
> +
> +static const struct slot_table_entry P0E1A_x8_PLX8748_RiserF_up[] = {
>          {
>                  .etype = st_builtin_dev,
>                  .location = ST_LOC_DEVFN(0,0),
> -                .children = P0E1A_x8_PLX8748_down,
> +                .children = P0E1A_x8_PLX8748_RiserF_down,
>          },
>          { .etype = st_end }
>  };
>
> -static const struct slot_table_entry p0phb1_slot[] = {
> +static const struct slot_table_entry p0phb1_rF_slot[] = {
>          {
>                  .etype = st_builtin_dev,
>                  .location = ST_LOC_DEVFN(0,0),
> -                .children = P0E1A_x8_PLX8748_up,
> +                .children = P0E1A_x8_PLX8748_RiserF_up,
> +        },
> +        { .etype = st_end },
> +};
> +
> +static const struct slot_table_entry P1E2_x16_Switch_down[] = {
> +        SW_PLUGGABLE("Slot8", 0x1),
> +        SW_PLUGGABLE("Slot9", 0x0),
> +
> +        { .etype = st_end }
> +};
> +
> +static const struct slot_table_entry P1E2_x16_Switch_up[] = {
> +        {
> +                .etype = st_builtin_dev,
> +                .location = ST_LOC_DEVFN(0,0),
> +                .children = P1E2_x16_Switch_down,
> +        },
> +        { .etype = st_end }
> +};
> +
> +static const struct slot_table_entry p1phb3_switch_slot[] = {
> +        {
> +                .etype = st_builtin_dev,
> +                .location = ST_LOC_DEVFN(0,0),
> +                .children = P1E2_x16_Switch_up,
> +        },
> +        { .etype = st_end },
> +};
> +
> +static const struct slot_table_entry P0E2_x16_Switch_down[] = {
> +        SW_PLUGGABLE("Slot3", 0x1),
> +        SW_PLUGGABLE("Slot4", 0x0),
> +
> +        { .etype = st_end }
> +};
> +
> +static const struct slot_table_entry P0E2_x16_Switch_up[] = {
> +        {
> +                .etype = st_builtin_dev,
> +                .location = ST_LOC_DEVFN(0,0),
> +                .children = P0E2_x16_Switch_down,
> +        },
> +        { .etype = st_end }
> +};
> +
> +static const struct slot_table_entry p0phb3_switch_slot[] = {
> +        {
> +                .etype = st_builtin_dev,
> +                .location = ST_LOC_DEVFN(0,0),
> +                .children = P0E2_x16_Switch_up,
>          },
>          { .etype = st_end },

You can use the ST_PLUGGABLE() and ST_BUILTIN_DEV() macros to remove most of the struct boilerplate. There's an example of to use them for the upstream port of a switch, etc platforms/qemu/qemu.c. That said, if you use them and need to backport this patch then you'll need to pick cherry-pick bbe5f0038786 ("platforms/astbmc: Add more slot helper
macros") too.

Joy: The slot helper macros example from qemu looks more simpler and clearer. We'll take this commit for consideration.

>  };
> @@ -148,22 +256,38 @@ ST_PLUGGABLE(p0phb3_slot, "Slot4");  
> ST_PLUGGABLE(p1phb0_slot, "Slot6");  ST_PLUGGABLE(p1phb3_slot, 
> "Slot9");
>
> -static const struct slot_table_entry mihawk_phb_table[] = {
> +static const struct slot_table_entry mihawk_riserA_phb_table[] = {
>          /* ==== CPU0 ==== */
>          ST_PHB_ENTRY(0, 0, p0phb0_slot),    /* P0E0_x16_Slot1 */
> -        ST_PHB_ENTRY(0, 1, p0phb1_slot),    /* P0E1A_x8_PLX8748-1_Slot2-3-5 */
> +        ST_PHB_ENTRY(0, 1, p0phb1_rA_slot), /* 
> + P0E1A_x8_PLX8748-1_Slot2-3-5 */
>          //ST_PHB_ENTRY(0, 2, p0phb2_slot),  /* P0E1B_x8_USBTI7340 */
>          ST_PHB_ENTRY(0, 3, p0phb3_slot),    /* P0E2_x16_Slot4 */
>
>          /* ==== CPU1 ==== */
>          ST_PHB_ENTRY(8, 0, p1phb0_slot),    /* P1E0_x16_Slot6 */
> -        ST_PHB_ENTRY(8, 1, p1phb1_slot),    /* P1E1A_x8_PLX8748-2_Slot7-8-10 */
> +        ST_PHB_ENTRY(8, 1, p1phb1_rA_slot), /* 
> + P1E1A_x8_PLX8748-2_Slot7-8-10 */
>          //ST_PHB_ENTRY(8, 2, p1phb2_slot),  /* P1E1B_x8_NA */
>          ST_PHB_ENTRY(8, 3, p1phb3_slot),    /* P1E2_x16_Slot9 */
>
>          { .etype = st_end },
>  };
>
> +static const struct slot_table_entry mihawk_riserF_phb_table[] = {
> +        /* ==== CPU0 ==== */
> +        ST_PHB_ENTRY(0, 0, p0phb0_slot),       /* P0E0_x16_Slot1 */
> +        ST_PHB_ENTRY(0, 1, p0phb1_rF_slot),    /* P0E1A_x8_PLX8748-1_Slot2-5 */
> +        //ST_PHB_ENTRY(0, 2, p0phb2_slot),     /* P0E1B_x8_USBTI7340 */
> +        ST_PHB_ENTRY(0, 3, p0phb3_switch_slot),/* 
> +P0E2_x16_SWITCH_Slot3-4 */
> +
> +        /* ==== CPU1 ==== */
> +        ST_PHB_ENTRY(8, 0, p1phb0_slot),       /* P1E0_x16_Slot6 */
> +        ST_PHB_ENTRY(8, 1, p1phb1_rF_slot),    /* P1E1A_x8_PLX8748-2_Slot7-10 */
> +        //ST_PHB_ENTRY(8, 2, p1phb2_slot),     /* P1E1B_x8_NA */
> +        ST_PHB_ENTRY(8, 3, p1phb3_switch_slot),/* 
> + P1E2_x16_SWITCH_Slot8-9 */
> +
> +        { .etype = st_end },
> +};
> +
>  #define NPU_BASE 0x5011000
>  #define NPU_SIZE 0x2c
>  #define NPU_INDIRECT0  0x8000000009010c3fUL /* OB0 - no OB3 on Mihawk 
> */ @@ -282,15 +406,90 @@ static bool mihawk_probe(void)
>         mihawk_create_npu();
>         mihawk_create_ocapi_i2c_bus();
>
> -       slot_table_init(mihawk_phb_table);
> +       if (mihawk_riserF_found)
> +               slot_table_init(mihawk_riserF_phb_table);

Is setting the slot table here necessary? It'll be overwritten when
mihawk_init() is called later on and I don't think anything will reference the slot table until we start doing PCI probing.

Joy: I'll remove this.

>         return true;
>  }
>
> +static void mihawk_riser_query_complete(struct ipmi_msg *msg) {
> +       if (msg->cc != IPMI_CC_NO_ERROR) {
> +               prlog(PR_ERR, "Mihawk: IPMI riser query returned error. cmd=0x%02x,"
> +                       " netfn=0x%02x, rc=0x%x\n", msg->cmd, msg->netfn, msg->cc);
> +               bmc_query_waiting = false;
> +               ipmi_free_msg(msg);
> +               return;
> +       }
> +
> +       prlog(PR_DEBUG, "Mihawk: IPMI Got riser query result. p0:%02x, p1:%02x\n"
> +               , msg->data[0], msg->data[1]);
> +
> +       uint8_t *riser_state = (uint8_t*)msg->user_data;
> +       lwsync();
> +       *riser_state = msg->data[0] << 4 | msg->data[1];
> +
> +       bmc_query_waiting = false;
> +       ipmi_free_msg(msg);
> +}
> +
> +static void mihawk_init(void)
> +{
> +       struct ipmi_msg *ipmi_msg;
> +       uint8_t riser_state = 0;
> +       int timeout_ms = 3000;
> +
> +       astbmc_init();
> +
> +       /*
> +        * We use IPMI to ask BMC if Riser-F is installed and set up the
> +        * corresponding slot table.
> +        */
> +       if (!mihawk_riserF_found) {

Is mihawk_riserF_found ever going to be true at this point? Looks like its only ever set below

Joy: The value check of mihawk_riserF_found is necessary. I'll remove this.

> +               ipmi_msg = ipmi_mkmsg(IPMI_DEFAULT_INTERFACE,
> +                                     IPMI_RISERF_QUERY,
> +                                     mihawk_riser_query_complete,
> +                                     &riser_state, NULL, 0, 2);
> +
> +               if (!ipmi_msg) {
> +                       prlog(PR_ERR, "Mihawk: Couldn't create ipmi msg.");
> +               }
> +               else {
> +                       ipmi_msg->error = mihawk_riser_query_complete;
> +                       ipmi_queue_msg(ipmi_msg);
> +                       bmc_query_waiting = true;
> +
> +                       prlog(PR_DEBUG, "Mihawk: Requesting IPMI_RISERF_QUERY (netfn "
> +                               "%02x, cmd %02x)\n", ipmi_msg->netfn, 
> + ipmi_msg->cmd);
> +
> +                       while (bmc_query_waiting) {
> +                               time_wait_ms(10);
> +                               timeout_ms -= 10;
> +
> +                               if (timeout_ms == 0)
> +                                       break;
> +                       }
> +               }

Can you use ipmi_queue_msg_sync() instead of the callback and open-coded delay loop? The only problem I can see is that if the BMC doesn't respond we'd hit the timeout in the current code while
ipmi_queue_msg_sync() will wait forever. We could always add a timeout to the _sync version though.

Joy: To use ipmi_queue_msg_sync() with timeout, would you please share some example? We want to add the timeout to prevent from the long waiting and the increase of boot-up time.
I still have no idea about how to add the timeout when using ipmi_queue_msg_sync(). Would you please give us some hints?

> +               prlog(PR_DEBUG, "Mihawk: IPMI_RISERF_QUERY finish. riser_state: %02x"
> +                       ", waiting: %d\n", riser_state, 
> + bmc_query_waiting);
> +
> +               if (riser_state != 0) {
> +                       mihawk_riserF_found = true;
> +                       slot_table_init(mihawk_riserF_phb_table);
> +                       prlog(PR_DEBUG, "Mihawk: Detect Riser-F via IPMI\n");
> +               }
> +               else {
no newline

> +                       slot_table_init(mihawk_riserA_phb_table);
> +                       prlog(PR_DEBUG, "Mihawk: No Riser-F found, use Riser-A table\n");
> +               }
> +       }
> +}
> +
>  DECLARE_PLATFORM(mihawk) = {
>         .name                   = "Mihawk",
>         .probe                  = mihawk_probe,
> -       .init                   = astbmc_init,
> +       .init                   = mihawk_init,
>         .start_preload_resource = flash_start_preload_resource,
>         .resource_loaded        = flash_resource_loaded,
>         .bmc                    = &bmc_plat_ast2500_openbmc,
> --
> 2.17.1
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot


More information about the Skiboot mailing list