Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

risc-v/mpfs: ihc: cleanup DEBUGASSERTs and reboot #183

Merged
merged 1 commit into from
Nov 15, 2023
+105 −50
Merged

risc-v/mpfs: ihc: cleanup DEBUGASSERTs and reboot #183

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 69 additions & 32 deletions arch/risc-v/src/mpfs/mpfs_ihc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
* Pre-processor Definitions
****************************************************************************/

#ifdef CONFIG_MPFS_IHC_DEBUG
#ifdef CONFIG_DEBUG_ERROR
# define ihcerr _err
# define ihcwarn _warn
# define ihcinfo _info
Expand Down Expand Up @@ -337,9 +337,6 @@ static uint32_t mpfs_ihc_context_to_remote_hart_id(ihc_channel_t channel)
}
else
{
DEBUGASSERT(LIBERO_SETTING_CONTEXT_A_HART_EN > 0);
DEBUGASSERT(LIBERO_SETTING_CONTEXT_B_HART_EN > 0);

/* Determine context we are in */

if (channel == IHC_CHANNEL_TO_CONTEXTA)
Expand All @@ -350,10 +347,6 @@ static uint32_t mpfs_ihc_context_to_remote_hart_id(ihc_channel_t channel)
{
harts_in_context = LIBERO_SETTING_CONTEXT_B_HART_EN;
}
else
{
DEBUGPANIC();
pussuw marked this conversation as resolved.
Show resolved Hide resolved
}

hart_idx = 0;

Expand All @@ -369,8 +362,6 @@ static uint32_t mpfs_ihc_context_to_remote_hart_id(ihc_channel_t channel)
}
}

DEBUGASSERT(hart != UNDEFINED_HART_ID);

return hart;
}

Expand Down Expand Up @@ -431,8 +422,6 @@ static uint32_t mpfs_ihc_context_to_local_hart_id(ihc_channel_t channel)
}
}

DEBUGASSERT(hart < MPFS_NUM_HARTS);

return hart;
}

Expand Down Expand Up @@ -505,7 +494,10 @@ static void mpfs_ihc_worker(void *arg)
}
while ((ctrl_reg & (RMP_MESSAGE_PRESENT)) && --retries);

DEBUGASSERT(retries != 0);
if (retries == 0)
{
ihcerr("Could not send the message!\n");
}

modifyreg32(MPFS_IHC_CTRL(g_work_arg.mhartid,
g_work_arg.rhartid), 0, ACK_INT);
Expand Down Expand Up @@ -536,19 +528,25 @@ static void mpfs_ihc_rx_message(ihc_channel_t channel, uint32_t mhartid,
uint32_t rhartid = mpfs_ihc_context_to_remote_hart_id(channel);
uint32_t ctrl_reg = getreg32(MPFS_IHC_CTRL(mhartid, rhartid));

if (rhartid == UNDEFINED_HART_ID)
{
ihcerr("Remote hart not identified!\n");
return;
}

/* Check if we have a message */

if (mhartid == CONTEXTB_HARTID)
{
uintptr_t msg_in = MPFS_IHC_MSG_IN(mhartid, rhartid);
DEBUGASSERT(msg == NULL);
mpfs_ihc_rx_handler((uint32_t *)msg_in);
}
else
{
/* This path is meant for the OpenSBI vendor extension only */

DEBUGPANIC();
ihcerr("Wrong recipient!\n");
return;
}

/* Set MP to 0. Note this generates an interrupt on the other hart
Expand Down Expand Up @@ -683,15 +681,19 @@ static int mpfs_ihc_interrupt(int irq, void *context, void *arg)
* hart_to_configure - Hart to be configured
*
* Returned Value:
* None
* OK if all is good, a negated error code otherwise
*
****************************************************************************/

static void mpfs_ihc_local_context_init(uint32_t hart_to_configure)
static int mpfs_ihc_local_context_init(uint32_t hart_to_configure)
{
uint32_t rhartid = 0;

DEBUGASSERT(hart_to_configure < MPFS_NUM_HARTS);
if (hart_to_configure >= MPFS_NUM_HARTS)
{
ihcerr("Configuring too many harts!\n");
return -EINVAL;
}

while (rhartid < MPFS_NUM_HARTS)
{
Expand All @@ -701,6 +703,8 @@ static void mpfs_ihc_local_context_init(uint32_t hart_to_configure)

g_connected_harts = ihcia_remote_harts[hart_to_configure];
g_connected_hart_ints = ihcia_remote_hart_ints[hart_to_configure];

return OK;
}

/****************************************************************************
Expand Down Expand Up @@ -758,7 +762,24 @@ static int mpfs_ihc_tx_message(ihc_channel_t channel, uint32_t *message)
uint32_t ctrl_reg;
uint32_t retries = 10000;

DEBUGASSERT(message_size <= IHC_MAX_MESSAGE_SIZE);
if (mhartid >= MPFS_NUM_HARTS)
{
ihcerr("Problem finding proper mhartid\n");
return -EINVAL;
}

if (rhartid == UNDEFINED_HART_ID)
{
/* Something went wrong */

ihcerr("Remote hart not found!\n");
return -EINVAL;
}
else if (message_size > IHC_MAX_MESSAGE_SIZE)
{
ihcerr("Sent message too large!\n");
return -EINVAL;
}

/* Check if the system is busy. All we can try is wait. */

Expand Down Expand Up @@ -885,8 +906,6 @@ mpfs_rptun_get_resource(struct rptun_dev_s *dev)

/* Only slave supported so far */

DEBUGASSERT(!priv->master);

if (priv->shmem != NULL)
{
return &priv->shmem->rsc;
Expand Down Expand Up @@ -942,6 +961,12 @@ mpfs_rptun_get_resource(struct rptun_dev_s *dev)

g_rptun_initialized = true;

/* Don't enable this too early; if the master is already up, irqs will
* likely hang the system as no ACKs may be sent yet.
*/

up_enable_irq(g_plic_irq);

return &priv->shmem->rsc;
}

Expand Down Expand Up @@ -1063,7 +1088,10 @@ static int mpfs_rptun_notify(struct rptun_dev_s *dev, uint32_t notifyid)
}
while ((ret != OK) && --retries);

DEBUGASSERT(ret == OK);
if (retries == 0)
{
return -EIO;
}
}

return ret;
Expand Down Expand Up @@ -1256,8 +1284,12 @@ static void mpfs_rptun_worker(void *arg)
return;
}

DEBUGASSERT((g_vq_idx - VRING0_NOTIFYID) < VRINGS);
info = &g_mpfs_virtqueue_table[g_vq_idx - VRING0_NOTIFYID];
if (g_vq_idx >= VRINGS)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is safe as it is. I'm just wondering, does it no longer support 2 channels / CONFIG_MPFS_IHC_RPMSG_CH2 (VRINGS = 2, VRING0_NOTIFYID = 2 or VRING1_NOTIFYID = 3) ? Not even sure we're ever going to need it, just out of curiosity.

Copy link
Author

@eenurkka eenurkka Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking of that also, but as 2x channels are currently not needed / hard to test, it's there waiting to be fixed if it will ever be needed. Maybe a comment should be written there.

{
return;
}

info = &g_mpfs_virtqueue_table[g_vq_idx];
virtqueue_notification((struct virtqueue *)info->data);
}
#endif
Expand Down Expand Up @@ -1293,8 +1325,12 @@ static int mpfs_rptun_thread(int argc, char *argv[])
return 0;
}

DEBUGASSERT((g_vq_idx - VRING0_NOTIFYID) < VRINGS);
info = &g_mpfs_virtqueue_table[g_vq_idx - VRING0_NOTIFYID];
if (g_vq_idx >= VRINGS)
{
return -EINVAL;
}

info = &g_mpfs_virtqueue_table[g_vq_idx];
virtqueue_notification((struct virtqueue *)info->data);

nxsem_wait(&g_mpfs_rx_sig);
Expand Down Expand Up @@ -1357,17 +1393,18 @@ int mpfs_ihc_init(void)

/* Initialize IHC FPGA module registers to a known state */

mpfs_ihc_local_context_init(mhartid);
ret = mpfs_ihc_local_context_init(mhartid);
if (ret != OK)
{
return ret;
}

mpfs_ihc_local_remote_config(mhartid, rhartid);

/* Attach and enable the applicable irq */

ret = irq_attach(g_plic_irq, mpfs_ihc_interrupt, NULL);
if (ret == OK)
{
up_enable_irq(g_plic_irq);
}
else
if (ret != OK)
{
ihcerr("ERROR: Not able to attach irq\n");
return ret;
Expand Down
54 changes: 36 additions & 18 deletions arch/risc-v/src/mpfs/mpfs_ihc_sbi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,6 @@ static uint32_t mpfs_ihc_sbi_context_to_remote_hart_id(ihc_channel_t channel)
hart_idx++;
}

DEBUGASSERT(hart != UNDEFINED_HART_ID);

return hart;
}

Expand All @@ -244,8 +242,6 @@ static uint32_t mpfs_ihc_sbi_context_to_local_hart_id(ihc_channel_t channel)
uint32_t harts_in_context = LIBERO_SETTING_CONTEXT_A_HART_EN;
uint32_t hart_next = 0;

DEBUGASSERT(channel > IHC_CHANNEL_TO_HART4);

hart_idx = 0;
while (hart_idx < MPFS_NUM_HARTS)
{
Expand All @@ -271,7 +267,6 @@ static uint32_t mpfs_ihc_sbi_context_to_local_hart_id(ihc_channel_t channel)
hart_idx++;
}

DEBUGASSERT(hart < MPFS_NUM_HARTS);
return hart;
}

Expand Down Expand Up @@ -303,8 +298,7 @@ static void mpfs_ihc_sbi_message_present_handler(uint32_t *message,
bool is_mp)
{
struct ihc_sbi_rx_msg_s *msg;
uintptr_t message_ihc = (uintptr_t)MPFS_IHC_MSG_IN(mhartid, rhartid);
uint32_t message_size_ihc = getreg32(MPFS_IHC_MSG_SIZE(mhartid, rhartid));
uintptr_t message_ihc = (uintptr_t)MPFS_IHC_MSG_IN(mhartid, rhartid);

msg = (struct ihc_sbi_rx_msg_s *)message;

Expand All @@ -324,8 +318,6 @@ static void mpfs_ihc_sbi_message_present_handler(uint32_t *message,
msg->irq_type = ACK_IRQ | MP_IRQ;
msg->ihc_msg = *(struct mpfs_ihc_msg_s *)message_ihc;
}

DEBUGASSERT(sizeof(msg->ihc_msg) >= message_size_ihc);
}

/****************************************************************************
Expand All @@ -350,17 +342,23 @@ static void mpfs_ihc_sbi_message_present_handler(uint32_t *message,
static void mpfs_ihc_sbi_rx_message(uint32_t rhartid, uint32_t mhartid,
bool is_ack, bool is_mp, uint32_t *msg)
{
/* Msg must be always present */

if (msg == NULL)
{
return;
}

if (is_ack && !is_mp)
{
if (mhartid == CONTEXTB_HARTID)
{
DEBUGPANIC();
return;
}
else
{
/* This path is meant for the OpenSBI vendor extension only */

DEBUGASSERT(msg != NULL);
mpfs_ihc_sbi_message_present_handler(msg, mhartid, rhartid,
is_ack, is_mp);

Expand All @@ -375,13 +373,12 @@ static void mpfs_ihc_sbi_rx_message(uint32_t rhartid, uint32_t mhartid,

if (mhartid == CONTEXTB_HARTID)
{
DEBUGPANIC();
return;
}
else
{
/* This path is meant for the OpenSBI vendor extension only */

DEBUGASSERT(msg != NULL);
mpfs_ihc_sbi_message_present_handler(msg, mhartid, rhartid,
is_ack, is_mp);
}
Expand All @@ -394,7 +391,6 @@ static void mpfs_ihc_sbi_rx_message(uint32_t rhartid, uint32_t mhartid,
}
else if (is_ack && is_mp)
{
DEBUGASSERT(msg != NULL);
mpfs_ihc_sbi_message_present_handler(msg, mhartid, rhartid,
is_ack, is_mp);

Expand Down Expand Up @@ -431,7 +427,8 @@ void mpfs_ihc_sbi_message_present_indirect_isr(ihc_channel_t channel,
uint32_t origin_hart = mpfs_ihc_sbi_parse_incoming_hartid(mhartid,
&is_ack,
&is_mp);
if (origin_hart != UNDEFINED_HART_ID)

if ((origin_hart != UNDEFINED_HART_ID) && (mhartid < MPFS_NUM_HARTS))
{
/* Process incoming packet */

Expand Down Expand Up @@ -460,8 +457,6 @@ static void mpfs_ihc_sbi_local_context_init(uint32_t hart_to_configure)
{
uint32_t rhartid = 0;

DEBUGASSERT(hart_to_configure < MPFS_NUM_HARTS);

while (rhartid < MPFS_NUM_HARTS)
{
putreg32(0, MPFS_IHC_CTRL(hart_to_configure, rhartid));
Expand Down Expand Up @@ -551,7 +546,18 @@ static int mpfs_ihc_sbi_tx_message(ihc_channel_t channel, uint32_t *message)
uint32_t ctrl_reg;
uint32_t retries = 10000;

DEBUGASSERT(message_size <= IHC_MAX_MESSAGE_SIZE);
if (message_size > IHC_MAX_MESSAGE_SIZE)
{
return -EINVAL;
}
else if (rhartid == UNDEFINED_HART_ID)
{
return -EINVAL;
}
else if (mhartid >= MPFS_NUM_HARTS)
{
return -EINVAL;
}

/* Check if the system is busy. All we can try is wait. */

Expand Down Expand Up @@ -643,7 +649,19 @@ int mpfs_ihc_sbi_ecall_handler(unsigned long funcid, uint32_t remote_channel,
/* mhartid = Linux hart id, rhartid = NuttX hart id */

mhartid = mpfs_ihc_sbi_context_to_local_hart_id(remote_channel);

if (mhartid >= MPFS_NUM_HARTS)
{
return -EINVAL;
}

rhartid = mpfs_ihc_sbi_context_to_remote_hart_id(remote_channel);

if (rhartid == UNDEFINED_HART_ID)
{
return -EINVAL;
}

if (remote_channel == IHC_CHANNEL_TO_CONTEXTB)
{
mpfs_ihc_sbi_local_context_init(mhartid);
Expand Down