diff mbox series

platform/mihawk: support dynamic PCIe slot table

Message ID 1582681882-15254-1-git-send-email-joy_chu@wistron.com
State New
Headers show
Series platform/mihawk: support dynamic PCIe slot table | expand

Checks

Context Check Description
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco success Signed-off-by present
snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot fail Test snowpatch/job/snowpatch-skiboot on branch master
snowpatch_ozlabs/apply_patch success Successfully applied on branch master (f123417068e51842004bdc047c8c5107b70442ef)

Commit Message

Joy Chu Feb. 26, 2020, 1:51 a.m. UTC
Slot table auto-detection for different riser cards by using IPMI
OEM command to communicate with BMC.

Signed-off-by: Joy Chu <joy_chu@wistron.com>
---
 platforms/astbmc/mihawk.c | 229 +++++++++++++++++++++++++++++++++++---
 1 file changed, 214 insertions(+), 15 deletions(-)

Comments

Oliver O'Halloran Feb. 26, 2020, 4:22 a.m. UTC | #1
On Wed, Feb 26, 2020 at 12:57 PM Joy Chu <joy_chu@wistron.com> wrote:
>
> Slot table auto-detection for different riser cards by using IPMI
> OEM command to communicate with BMC.
>
> Signed-off-by: Joy Chu <joy_chu@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.

>  };
> @@ -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.

>         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


> +               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.

> +               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@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
Joy Chu March 3, 2020, 8:15 a.m. UTC | #2
Hi Oliver,

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

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.


Best Regards,
Joy Chu


-----Original Message-----
From: Oliver O'Halloran <oohall@gmail.com> 
Sent: Wednesday, February 26, 2020 12:23 PM
To: Joy Chu/WHQ/Wistron <Joy_Chu@wistron.com>
Cc: skiboot list <skiboot@lists.ozlabs.org>; Stewart Smith <stewart@linux.vnet.ibm.com>; Oliver OHalloran <oliveroh@au1.ibm.com>; chhank@tw.ibm.com; chcyjoy@gmail.com
Subject: Re: [Skiboot] [PATCH] platform/mihawk: support dynamic PCIe slot table

On Wed, Feb 26, 2020 at 12:57 PM Joy Chu <joy_chu@wistron.com> wrote:
>
> Slot table auto-detection for different riser cards by using IPMI OEM 
> command to communicate with BMC.
>
> Signed-off-by: Joy Chu <joy_chu@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.

>  };
> @@ -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.

>         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


> +               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.

> +               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@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
Joy Chu March 12, 2020, 9:05 a.m. UTC | #3
Hi Oliver,

As in your previous comment:
""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.""

I still have no idea about how to add the timeout when using ipmi_queue_msg_sync().
Would you please give us some hints?


Best Regards,
Joy Chu

-----Original Message-----
From: Joy Chu/WHQ/Wistron 
Sent: Tuesday, March 3, 2020 4:15 PM
To: 'Oliver O'Halloran' <oohall@gmail.com>
Cc: skiboot list <skiboot@lists.ozlabs.org>; Stewart Smith <stewart@linux.vnet.ibm.com>; Oliver OHalloran <oliveroh@au1.ibm.com>; chhank@tw.ibm.com; chcyjoy@gmail.com
Subject: RE: [Skiboot] [PATCH] platform/mihawk: support dynamic PCIe slot table

Hi Oliver,

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

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.


Best Regards,
Joy Chu


-----Original Message-----
From: Oliver O'Halloran <oohall@gmail.com>
Sent: Wednesday, February 26, 2020 12:23 PM
To: Joy Chu/WHQ/Wistron <Joy_Chu@wistron.com>
Cc: skiboot list <skiboot@lists.ozlabs.org>; Stewart Smith <stewart@linux.vnet.ibm.com>; Oliver OHalloran <oliveroh@au1.ibm.com>; chhank@tw.ibm.com; chcyjoy@gmail.com
Subject: Re: [Skiboot] [PATCH] platform/mihawk: support dynamic PCIe slot table

On Wed, Feb 26, 2020 at 12:57 PM Joy Chu <joy_chu@wistron.com> wrote:
>
> Slot table auto-detection for different riser cards by using IPMI OEM 
> command to communicate with BMC.
>
> Signed-off-by: Joy Chu <joy_chu@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.

>  };
> @@ -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.

>         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


> +               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.

> +               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@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
Joy Chu March 24, 2020, 10:17 a.m. UTC | #4
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@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@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
Joy Chu March 24, 2020, 10:40 a.m. UTC | #5
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@wistron.com<mailto:joy_chu@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@lists.ozlabs.org<mailto:Skiboot@lists.ozlabs.org>

> https://lists.ozlabs.org/listinfo/skiboot
Oliver O'Halloran March 24, 2020, 10:28 p.m. UTC | #6
On Thu, Mar 12, 2020 at 8:05 PM Joy Chu/WHQ/Wistron <Joy_Chu@wistron.com> wrote:
>
> Hi Oliver,
>
> As in your previous comment:
> ""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.""
>
> I still have no idea about how to add the timeout when using ipmi_queue_msg_sync().
> Would you please give us some hints?

Hi Joy,

Sorry about the delay. I thought I sent a reply the other day, but it
looks like it didn't actually send.

Anyway, currently there is no code to implement a synchronous message
with a timeout. What I was thinking was that rather than having the
platform code implement one by hand we should provide it as a part of
the core API. That said, I'm a bit iffy about the concept. If we have
a message that times out what should we be doing with the message on
the skiboot side? If the BMC doesn't respond we can't just leave the
message sitting on the IPMI queue. What is happening on the BMC side
that prevents it sending a timely response?

Oliver
Joy Chu March 25, 2020, 9:36 a.m. UTC | #7
Hi Oliver,

In our case, we want to setup the timeout to prevent from the wait forever. When it hit the timeout, I need to keep the skiboot process going. We will setup a default setting if we can't get the response from BMC in time. At that time, we could free the ipmi msg from the queue (oh, I forget to handle this in my patch). Then, we will use the default setting and continue the mihawk_init process.


Best Regards,
Joy Chu


-----Original Message-----
From: Oliver O'Halloran <oohall@gmail.com> 
Sent: Wednesday, March 25, 2020 6:28 AM
To: Joy Chu/WHQ/Wistron <Joy_Chu@wistron.com>
Cc: skiboot list <skiboot@lists.ozlabs.org>; chhank@tw.ibm.com
Subject: Re: [Skiboot] [PATCH] platform/mihawk: support dynamic PCIe slot table

On Thu, Mar 12, 2020 at 8:05 PM Joy Chu/WHQ/Wistron <Joy_Chu@wistron.com> wrote:
>
> Hi Oliver,
>
> As in your previous comment:
> ""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.""
>
> I still have no idea about how to add the timeout when using ipmi_queue_msg_sync().
> Would you please give us some hints?

Hi Joy,

Sorry about the delay. I thought I sent a reply the other day, but it looks like it didn't actually send.

Anyway, currently there is no code to implement a synchronous message with a timeout. What I was thinking was that rather than having the platform code implement one by hand we should provide it as a part of the core API. That said, I'm a bit iffy about the concept. If we have a message that times out what should we be doing with the message on the skiboot side? If the BMC doesn't respond we can't just leave the message sitting on the IPMI queue. What is happening on the BMC side that prevents it sending a timely response?

Oliver
Joy Chu March 26, 2020, 10:23 a.m. UTC | #8
Hi Oliver,

Sorry to bother you. For the timeout, do you have any other idea?
We plan to release a pnor with this dynamic pcie slot table feature next Wednesday.
I really hope this feature can be on time.


Best Regards,
Joy Chu

-----Original Message-----
From: Joy Chu/WHQ/Wistron 
Sent: Wednesday, March 25, 2020 5:37 PM
To: 'Oliver O'Halloran' <oohall@gmail.com>
Cc: skiboot list <skiboot@lists.ozlabs.org>; chhank@tw.ibm.com
Subject: RE: [Skiboot] [PATCH] platform/mihawk: support dynamic PCIe slot table

Hi Oliver,

In our case, we want to setup the timeout to prevent from the wait forever. When it hit the timeout, I need to keep the skiboot process going. We will setup a default setting if we can't get the response from BMC in time. At that time, we could free the ipmi msg from the queue (oh, I forget to handle this in my patch). Then, we will use the default setting and continue the mihawk_init process.


Best Regards,
Joy Chu


-----Original Message-----
From: Oliver O'Halloran <oohall@gmail.com>
Sent: Wednesday, March 25, 2020 6:28 AM
To: Joy Chu/WHQ/Wistron <Joy_Chu@wistron.com>
Cc: skiboot list <skiboot@lists.ozlabs.org>; chhank@tw.ibm.com
Subject: Re: [Skiboot] [PATCH] platform/mihawk: support dynamic PCIe slot table

On Thu, Mar 12, 2020 at 8:05 PM Joy Chu/WHQ/Wistron <Joy_Chu@wistron.com> wrote:
>
> Hi Oliver,
>
> As in your previous comment:
> ""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.""
>
> I still have no idea about how to add the timeout when using ipmi_queue_msg_sync().
> Would you please give us some hints?

Hi Joy,

Sorry about the delay. I thought I sent a reply the other day, but it looks like it didn't actually send.

Anyway, currently there is no code to implement a synchronous message with a timeout. What I was thinking was that rather than having the platform code implement one by hand we should provide it as a part of the core API. That said, I'm a bit iffy about the concept. If we have a message that times out what should we be doing with the message on the skiboot side? If the BMC doesn't respond we can't just leave the message sitting on the IPMI queue. What is happening on the BMC side that prevents it sending a timely response?

Oliver
Joy Chu March 30, 2020, 8:13 a.m. UTC | #9
Hi Oliver,

Do you have any comment for this patch?


Best Regards,
Joy Chu


-----Original Message-----
From: Joy Chu/WHQ/Wistron 
Sent: Thursday, March 26, 2020 6:23 PM
To: 'Oliver O'Halloran' <oohall@gmail.com>
Cc: 'skiboot list' <skiboot@lists.ozlabs.org>; 'chhank@tw.ibm.com' <chhank@tw.ibm.com>
Subject: RE: [Skiboot] [PATCH] platform/mihawk: support dynamic PCIe slot table

Hi Oliver,

Sorry to bother you. For the timeout, do you have any other idea?
We plan to release a pnor with this dynamic pcie slot table feature next Wednesday.
I really hope this feature can be on time.


Best Regards,
Joy Chu

-----Original Message-----
From: Joy Chu/WHQ/Wistron
Sent: Wednesday, March 25, 2020 5:37 PM
To: 'Oliver O'Halloran' <oohall@gmail.com>
Cc: skiboot list <skiboot@lists.ozlabs.org>; chhank@tw.ibm.com
Subject: RE: [Skiboot] [PATCH] platform/mihawk: support dynamic PCIe slot table

Hi Oliver,

In our case, we want to setup the timeout to prevent from the wait forever. When it hit the timeout, I need to keep the skiboot process going. We will setup a default setting if we can't get the response from BMC in time. At that time, we could free the ipmi msg from the queue (oh, I forget to handle this in my patch). Then, we will use the default setting and continue the mihawk_init process.


Best Regards,
Joy Chu


-----Original Message-----
From: Oliver O'Halloran <oohall@gmail.com>
Sent: Wednesday, March 25, 2020 6:28 AM
To: Joy Chu/WHQ/Wistron <Joy_Chu@wistron.com>
Cc: skiboot list <skiboot@lists.ozlabs.org>; chhank@tw.ibm.com
Subject: Re: [Skiboot] [PATCH] platform/mihawk: support dynamic PCIe slot table

On Thu, Mar 12, 2020 at 8:05 PM Joy Chu/WHQ/Wistron <Joy_Chu@wistron.com> wrote:
>
> Hi Oliver,
>
> As in your previous comment:
> ""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.""
>
> I still have no idea about how to add the timeout when using ipmi_queue_msg_sync().
> Would you please give us some hints?

Hi Joy,

Sorry about the delay. I thought I sent a reply the other day, but it looks like it didn't actually send.

Anyway, currently there is no code to implement a synchronous message with a timeout. What I was thinking was that rather than having the platform code implement one by hand we should provide it as a part of the core API. That said, I'm a bit iffy about the concept. If we have a message that times out what should we be doing with the message on the skiboot side? If the BMC doesn't respond we can't just leave the message sitting on the IPMI queue. What is happening on the BMC side that prevents it sending a timely response?

Oliver
Oliver O'Halloran March 30, 2020, 10:26 a.m. UTC | #10
On Mon, Mar 30, 2020 at 7:13 PM Joy Chu/WHQ/Wistron <Joy_Chu@wistron.com> wrote:
>
> Hi Oliver,
>
> Do you have any comment for this patch?

Not really.  It sounds like the timeout is a just-in-case thing and
assuming the BMC is functional you will always get a response? Is that
correct? If so we can use the current patch.

Oliver
Joy Chu March 30, 2020, 10:38 a.m. UTC | #11
Hi Oliver, 

Yes. If this solution is OK, I'll send the patch v2 for some modification and the code base update.


Best Regards,
Joy Chu


-----Original Message-----
From: Oliver O'Halloran <oohall@gmail.com> 
Sent: Monday, March 30, 2020 6:27 PM
To: Joy Chu/WHQ/Wistron <Joy_Chu@wistron.com>
Cc: skiboot list <skiboot@lists.ozlabs.org>; chhank@tw.ibm.com
Subject: Re: [Skiboot] [PATCH] platform/mihawk: support dynamic PCIe slot table

On Mon, Mar 30, 2020 at 7:13 PM Joy Chu/WHQ/Wistron <Joy_Chu@wistron.com> wrote:
>
> Hi Oliver,
>
> Do you have any comment for this patch?

Not really.  It sounds like the timeout is a just-in-case thing and assuming the BMC is functional you will always get a response? Is that correct? If so we can use the current patch.

Oliver
diff mbox series

Patch

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