[Skiboot] [PATCH] platform/mihawk: support dynamic PCIe slot table
Joy_Chu at wistron.com
Joy_Chu at wistron.com
Tue Mar 24 21:17:00 AEDT 2020
Send my question again as below.
>
> Slot table auto-detection for different riser cards by using IPMI OEM
> command to communicate with BMC.
>
> Signed-off-by: Joy Chu <joy_chu at wistron.com>
> ---
> platforms/astbmc/mihawk.c | 229
> +++++++++++++++++++++++++++++++++++---
> 1 file changed, 214 insertions(+), 15 deletions(-)
>
> diff --git a/platforms/astbmc/mihawk.c b/platforms/astbmc/mihawk.c
> index d33d16bb..28c999aa 100644
> --- a/platforms/astbmc/mihawk.c
> +++ b/platforms/astbmc/mihawk.c
> @@ -15,8 +15,16 @@
> #include <pci.h>
> #include <pci-cfg.h>
>
> +#include <timebase.h>
> +
> #include "astbmc.h"
>
> +/* IPMI message code for Riser-F query (OEM). */
> +#define IPMI_RISERF_QUERY IPMI_CODE(0x32, 0x01)
> +
> +static bool mihawk_riserF_found = false; static bool
> +bmc_query_waiting = false;
> +
> /* nvme backplane slots */
> static const struct slot_table_entry hdd_bay_slots[] = {
> SW_PLUGGABLE("hdd0", 0x0),
> @@ -91,7 +99,7 @@ static const struct platform_ocapi mihawk_ocapi = {
> .ocapi_slot_label = mihawk_ocapi_slot_label,
> };
>
> -static const struct slot_table_entry P1E1A_x8_PLX8748_down[] = {
> +static const struct slot_table_entry P1E1A_x8_PLX8748_RiserA_down[] =
> +{
> SW_PLUGGABLE("Slot7", 0x10),
> SW_PLUGGABLE("Slot8", 0x8),
> SW_PLUGGABLE("Slot10", 0x9),
> @@ -99,25 +107,25 @@ static const struct slot_table_entry P1E1A_x8_PLX8748_down[] = {
> { .etype = st_end }
> };
>
> -static const struct slot_table_entry P1E1A_x8_PLX8748_up[] = {
> +static const struct slot_table_entry P1E1A_x8_PLX8748_RiserA_up[] = {
> {
> .etype = st_builtin_dev,
> .location = ST_LOC_DEVFN(0,0),
> - .children = P1E1A_x8_PLX8748_down,
> + .children = P1E1A_x8_PLX8748_RiserA_down,
> },
> { .etype = st_end }
> };
>
> -static const struct slot_table_entry p1phb1_slot[] = {
> +static const struct slot_table_entry p1phb1_rA_slot[] = {
> {
> .etype = st_builtin_dev,
> .location = ST_LOC_DEVFN(0,0),
> - .children = P1E1A_x8_PLX8748_up,
> + .children = P1E1A_x8_PLX8748_RiserA_up,
> },
> { .etype = st_end },
> };
>
> -static const struct slot_table_entry P0E1A_x8_PLX8748_down[] = {
> +static const struct slot_table_entry P0E1A_x8_PLX8748_RiserA_down[] =
> +{
> SW_PLUGGABLE("Slot2", 0x10),
> SW_PLUGGABLE("Slot3", 0x8),
> SW_PLUGGABLE("Slot5", 0x9),
> @@ -125,20 +133,120 @@ static const struct slot_table_entry P0E1A_x8_PLX8748_down[] = {
> { .etype = st_end }
> };
>
> -static const struct slot_table_entry P0E1A_x8_PLX8748_up[] = {
> +static const struct slot_table_entry P0E1A_x8_PLX8748_RiserA_up[] = {
> + {
> + .etype = st_builtin_dev,
> + .location = ST_LOC_DEVFN(0,0),
> + .children = P0E1A_x8_PLX8748_RiserA_down,
> + },
> + { .etype = st_end }
> +};
> +
> +static const struct slot_table_entry p0phb1_rA_slot[] = {
> + {
> + .etype = st_builtin_dev,
> + .location = ST_LOC_DEVFN(0,0),
> + .children = P0E1A_x8_PLX8748_RiserA_up,
> + },
> + { .etype = st_end },
> +};
> +
> +static const struct slot_table_entry P1E1A_x8_PLX8748_RiserF_down[] = {
> + SW_PLUGGABLE("Slot7", 0x10),
> + SW_PLUGGABLE("Slot10", 0x9),
> +
> + { .etype = st_end }
> +};
> +
> +static const struct slot_table_entry P1E1A_x8_PLX8748_RiserF_up[] = {
> + {
> + .etype = st_builtin_dev,
> + .location = ST_LOC_DEVFN(0,0),
> + .children = P1E1A_x8_PLX8748_RiserF_down,
> + },
> + { .etype = st_end }
> +};
> +
> +static const struct slot_table_entry p1phb1_rF_slot[] = {
> + {
> + .etype = st_builtin_dev,
> + .location = ST_LOC_DEVFN(0,0),
> + .children = P1E1A_x8_PLX8748_RiserF_up,
> + },
> + { .etype = st_end },
> +};
> +
> +static const struct slot_table_entry P0E1A_x8_PLX8748_RiserF_down[] = {
> + SW_PLUGGABLE("Slot2", 0x10),
> + SW_PLUGGABLE("Slot5", 0x9),
> +
> + { .etype = st_end }
> +};
> +
> +static const struct slot_table_entry P0E1A_x8_PLX8748_RiserF_up[] = {
> {
> .etype = st_builtin_dev,
> .location = ST_LOC_DEVFN(0,0),
> - .children = P0E1A_x8_PLX8748_down,
> + .children = P0E1A_x8_PLX8748_RiserF_down,
> },
> { .etype = st_end }
> };
>
> -static const struct slot_table_entry p0phb1_slot[] = {
> +static const struct slot_table_entry p0phb1_rF_slot[] = {
> {
> .etype = st_builtin_dev,
> .location = ST_LOC_DEVFN(0,0),
> - .children = P0E1A_x8_PLX8748_up,
> + .children = P0E1A_x8_PLX8748_RiserF_up,
> + },
> + { .etype = st_end },
> +};
> +
> +static const struct slot_table_entry P1E2_x16_Switch_down[] = {
> + SW_PLUGGABLE("Slot8", 0x1),
> + SW_PLUGGABLE("Slot9", 0x0),
> +
> + { .etype = st_end }
> +};
> +
> +static const struct slot_table_entry P1E2_x16_Switch_up[] = {
> + {
> + .etype = st_builtin_dev,
> + .location = ST_LOC_DEVFN(0,0),
> + .children = P1E2_x16_Switch_down,
> + },
> + { .etype = st_end }
> +};
> +
> +static const struct slot_table_entry p1phb3_switch_slot[] = {
> + {
> + .etype = st_builtin_dev,
> + .location = ST_LOC_DEVFN(0,0),
> + .children = P1E2_x16_Switch_up,
> + },
> + { .etype = st_end },
> +};
> +
> +static const struct slot_table_entry P0E2_x16_Switch_down[] = {
> + SW_PLUGGABLE("Slot3", 0x1),
> + SW_PLUGGABLE("Slot4", 0x0),
> +
> + { .etype = st_end }
> +};
> +
> +static const struct slot_table_entry P0E2_x16_Switch_up[] = {
> + {
> + .etype = st_builtin_dev,
> + .location = ST_LOC_DEVFN(0,0),
> + .children = P0E2_x16_Switch_down,
> + },
> + { .etype = st_end }
> +};
> +
> +static const struct slot_table_entry p0phb3_switch_slot[] = {
> + {
> + .etype = st_builtin_dev,
> + .location = ST_LOC_DEVFN(0,0),
> + .children = P0E2_x16_Switch_up,
> },
> { .etype = st_end },
You can use the ST_PLUGGABLE() and ST_BUILTIN_DEV() macros to remove most of the struct boilerplate. There's an example of to use them for the upstream port of a switch, etc platforms/qemu/qemu.c. That said, if you use them and need to backport this patch then you'll need to pick cherry-pick bbe5f0038786 ("platforms/astbmc: Add more slot helper
macros") too.
Joy: The slot helper macros example from qemu looks more simpler and clearer. We'll take this commit for consideration.
> };
> @@ -148,22 +256,38 @@ ST_PLUGGABLE(p0phb3_slot, "Slot4");
> ST_PLUGGABLE(p1phb0_slot, "Slot6"); ST_PLUGGABLE(p1phb3_slot,
> "Slot9");
>
> -static const struct slot_table_entry mihawk_phb_table[] = {
> +static const struct slot_table_entry mihawk_riserA_phb_table[] = {
> /* ==== CPU0 ==== */
> ST_PHB_ENTRY(0, 0, p0phb0_slot), /* P0E0_x16_Slot1 */
> - ST_PHB_ENTRY(0, 1, p0phb1_slot), /* P0E1A_x8_PLX8748-1_Slot2-3-5 */
> + ST_PHB_ENTRY(0, 1, p0phb1_rA_slot), /*
> + P0E1A_x8_PLX8748-1_Slot2-3-5 */
> //ST_PHB_ENTRY(0, 2, p0phb2_slot), /* P0E1B_x8_USBTI7340 */
> ST_PHB_ENTRY(0, 3, p0phb3_slot), /* P0E2_x16_Slot4 */
>
> /* ==== CPU1 ==== */
> ST_PHB_ENTRY(8, 0, p1phb0_slot), /* P1E0_x16_Slot6 */
> - ST_PHB_ENTRY(8, 1, p1phb1_slot), /* P1E1A_x8_PLX8748-2_Slot7-8-10 */
> + ST_PHB_ENTRY(8, 1, p1phb1_rA_slot), /*
> + P1E1A_x8_PLX8748-2_Slot7-8-10 */
> //ST_PHB_ENTRY(8, 2, p1phb2_slot), /* P1E1B_x8_NA */
> ST_PHB_ENTRY(8, 3, p1phb3_slot), /* P1E2_x16_Slot9 */
>
> { .etype = st_end },
> };
>
> +static const struct slot_table_entry mihawk_riserF_phb_table[] = {
> + /* ==== CPU0 ==== */
> + ST_PHB_ENTRY(0, 0, p0phb0_slot), /* P0E0_x16_Slot1 */
> + ST_PHB_ENTRY(0, 1, p0phb1_rF_slot), /* P0E1A_x8_PLX8748-1_Slot2-5 */
> + //ST_PHB_ENTRY(0, 2, p0phb2_slot), /* P0E1B_x8_USBTI7340 */
> + ST_PHB_ENTRY(0, 3, p0phb3_switch_slot),/*
> +P0E2_x16_SWITCH_Slot3-4 */
> +
> + /* ==== CPU1 ==== */
> + ST_PHB_ENTRY(8, 0, p1phb0_slot), /* P1E0_x16_Slot6 */
> + ST_PHB_ENTRY(8, 1, p1phb1_rF_slot), /* P1E1A_x8_PLX8748-2_Slot7-10 */
> + //ST_PHB_ENTRY(8, 2, p1phb2_slot), /* P1E1B_x8_NA */
> + ST_PHB_ENTRY(8, 3, p1phb3_switch_slot),/*
> + P1E2_x16_SWITCH_Slot8-9 */
> +
> + { .etype = st_end },
> +};
> +
> #define NPU_BASE 0x5011000
> #define NPU_SIZE 0x2c
> #define NPU_INDIRECT0 0x8000000009010c3fUL /* OB0 - no OB3 on Mihawk
> */ @@ -282,15 +406,90 @@ static bool mihawk_probe(void)
> mihawk_create_npu();
> mihawk_create_ocapi_i2c_bus();
>
> - slot_table_init(mihawk_phb_table);
> + if (mihawk_riserF_found)
> + slot_table_init(mihawk_riserF_phb_table);
Is setting the slot table here necessary? It'll be overwritten when
mihawk_init() is called later on and I don't think anything will reference the slot table until we start doing PCI probing.
Joy: I'll remove this.
> return true;
> }
>
> +static void mihawk_riser_query_complete(struct ipmi_msg *msg) {
> + if (msg->cc != IPMI_CC_NO_ERROR) {
> + prlog(PR_ERR, "Mihawk: IPMI riser query returned error. cmd=0x%02x,"
> + " netfn=0x%02x, rc=0x%x\n", msg->cmd, msg->netfn, msg->cc);
> + bmc_query_waiting = false;
> + ipmi_free_msg(msg);
> + return;
> + }
> +
> + prlog(PR_DEBUG, "Mihawk: IPMI Got riser query result. p0:%02x, p1:%02x\n"
> + , msg->data[0], msg->data[1]);
> +
> + uint8_t *riser_state = (uint8_t*)msg->user_data;
> + lwsync();
> + *riser_state = msg->data[0] << 4 | msg->data[1];
> +
> + bmc_query_waiting = false;
> + ipmi_free_msg(msg);
> +}
> +
> +static void mihawk_init(void)
> +{
> + struct ipmi_msg *ipmi_msg;
> + uint8_t riser_state = 0;
> + int timeout_ms = 3000;
> +
> + astbmc_init();
> +
> + /*
> + * We use IPMI to ask BMC if Riser-F is installed and set up the
> + * corresponding slot table.
> + */
> + if (!mihawk_riserF_found) {
Is mihawk_riserF_found ever going to be true at this point? Looks like its only ever set below
Joy: The value check of mihawk_riserF_found is necessary. I'll remove this.
> + ipmi_msg = ipmi_mkmsg(IPMI_DEFAULT_INTERFACE,
> + IPMI_RISERF_QUERY,
> + mihawk_riser_query_complete,
> + &riser_state, NULL, 0, 2);
> +
> + if (!ipmi_msg) {
> + prlog(PR_ERR, "Mihawk: Couldn't create ipmi msg.");
> + }
> + else {
> + ipmi_msg->error = mihawk_riser_query_complete;
> + ipmi_queue_msg(ipmi_msg);
> + bmc_query_waiting = true;
> +
> + prlog(PR_DEBUG, "Mihawk: Requesting IPMI_RISERF_QUERY (netfn "
> + "%02x, cmd %02x)\n", ipmi_msg->netfn,
> + ipmi_msg->cmd);
> +
> + while (bmc_query_waiting) {
> + time_wait_ms(10);
> + timeout_ms -= 10;
> +
> + if (timeout_ms == 0)
> + break;
> + }
> + }
Can you use ipmi_queue_msg_sync() instead of the callback and open-coded delay loop? The only problem I can see is that if the BMC doesn't respond we'd hit the timeout in the current code while
ipmi_queue_msg_sync() will wait forever. We could always add a timeout to the _sync version though.
Joy: To use ipmi_queue_msg_sync() with timeout, would you please share some example? We want to add the timeout to prevent from the long waiting and the increase of boot-up time.
I still have no idea about how to add the timeout when using ipmi_queue_msg_sync(). Would you please give us some hints?
> + prlog(PR_DEBUG, "Mihawk: IPMI_RISERF_QUERY finish. riser_state: %02x"
> + ", waiting: %d\n", riser_state,
> + bmc_query_waiting);
> +
> + if (riser_state != 0) {
> + mihawk_riserF_found = true;
> + slot_table_init(mihawk_riserF_phb_table);
> + prlog(PR_DEBUG, "Mihawk: Detect Riser-F via IPMI\n");
> + }
> + else {
no newline
> + slot_table_init(mihawk_riserA_phb_table);
> + prlog(PR_DEBUG, "Mihawk: No Riser-F found, use Riser-A table\n");
> + }
> + }
> +}
> +
> DECLARE_PLATFORM(mihawk) = {
> .name = "Mihawk",
> .probe = mihawk_probe,
> - .init = astbmc_init,
> + .init = mihawk_init,
> .start_preload_resource = flash_start_preload_resource,
> .resource_loaded = flash_resource_loaded,
> .bmc = &bmc_plat_ast2500_openbmc,
> --
> 2.17.1
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
More information about the Skiboot
mailing list