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

Joy_Chu at wistron.com Joy_Chu at wistron.com
Tue Mar 24 21:40:59 AEDT 2020



Send my question again as below and modify the typo from previous mail.

>

> 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<mailto: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(*typo; it should be “unnecessary”). 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<mailto:Skiboot at lists.ozlabs.org>

> https://lists.ozlabs.org/listinfo/skiboot
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/skiboot/attachments/20200324/e8eccec5/attachment-0001.htm>


More information about the Skiboot mailing list