From d9cbe818485bafaf460e5d2a414b0f72b5a200ee Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Wed, 13 Jan 2021 11:15:27 -0600 Subject: [PATCH 1/6] net: ipa: a few simple renames The return value of gsi_command() is true if successful or false if we time out waiting for a completion interrupt. Rename the variables in the three callers of gsi_command() to be "timeout", to make it more obvious that's the only reason for failure. In addition, add a "gsi_" prefix to evt_ring_command() so its name is consistent with the convention used for GSI channel and generic commands. Signed-off-by: Alex Elder Reviewed-by: Saeed Mahameed Signed-off-by: Jakub Kicinski --- drivers/net/ipa/gsi.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c index 14d9a791924b..b5913ce0bc94 100644 --- a/drivers/net/ipa/gsi.c +++ b/drivers/net/ipa/gsi.c @@ -326,13 +326,13 @@ gsi_evt_ring_state(struct gsi *gsi, u32 evt_ring_id) } /* Issue an event ring command and wait for it to complete */ -static void evt_ring_command(struct gsi *gsi, u32 evt_ring_id, - enum gsi_evt_cmd_opcode opcode) +static void gsi_evt_ring_command(struct gsi *gsi, u32 evt_ring_id, + enum gsi_evt_cmd_opcode opcode) { struct gsi_evt_ring *evt_ring = &gsi->evt_ring[evt_ring_id]; struct completion *completion = &evt_ring->completion; struct device *dev = gsi->dev; - bool success; + bool timeout; u32 val; /* We only perform one event ring command at a time, and event @@ -354,13 +354,13 @@ static void evt_ring_command(struct gsi *gsi, u32 evt_ring_id, val = u32_encode_bits(evt_ring_id, EV_CHID_FMASK); val |= u32_encode_bits(opcode, EV_OPCODE_FMASK); - success = gsi_command(gsi, GSI_EV_CH_CMD_OFFSET, val, completion); + timeout = !gsi_command(gsi, GSI_EV_CH_CMD_OFFSET, val, completion); /* Disable the interrupt again */ gsi_irq_type_disable(gsi, GSI_EV_CTRL); iowrite32(0, gsi->virt + GSI_CNTXT_SRC_EV_CH_IRQ_MSK_OFFSET); - if (success) + if (!timeout) return; dev_err(dev, "GSI command %u for event ring %u timed out, state %u\n", @@ -380,7 +380,7 @@ static int gsi_evt_ring_alloc_command(struct gsi *gsi, u32 evt_ring_id) return -EINVAL; } - evt_ring_command(gsi, evt_ring_id, GSI_EVT_ALLOCATE); + gsi_evt_ring_command(gsi, evt_ring_id, GSI_EVT_ALLOCATE); /* If successful the event ring state will have changed */ if (evt_ring->state == GSI_EVT_RING_STATE_ALLOCATED) @@ -405,7 +405,7 @@ static void gsi_evt_ring_reset_command(struct gsi *gsi, u32 evt_ring_id) return; } - evt_ring_command(gsi, evt_ring_id, GSI_EVT_RESET); + gsi_evt_ring_command(gsi, evt_ring_id, GSI_EVT_RESET); /* If successful the event ring state will have changed */ if (evt_ring->state == GSI_EVT_RING_STATE_ALLOCATED) @@ -426,7 +426,7 @@ static void gsi_evt_ring_de_alloc_command(struct gsi *gsi, u32 evt_ring_id) return; } - evt_ring_command(gsi, evt_ring_id, GSI_EVT_DE_ALLOC); + gsi_evt_ring_command(gsi, evt_ring_id, GSI_EVT_DE_ALLOC); /* If successful the event ring state will have changed */ if (evt_ring->state == GSI_EVT_RING_STATE_NOT_ALLOCATED) @@ -456,7 +456,7 @@ gsi_channel_command(struct gsi_channel *channel, enum gsi_ch_cmd_opcode opcode) u32 channel_id = gsi_channel_id(channel); struct gsi *gsi = channel->gsi; struct device *dev = gsi->dev; - bool success; + bool timeout; u32 val; /* We only perform one channel command at a time, and channel @@ -477,13 +477,13 @@ gsi_channel_command(struct gsi_channel *channel, enum gsi_ch_cmd_opcode opcode) val = u32_encode_bits(channel_id, CH_CHID_FMASK); val |= u32_encode_bits(opcode, CH_OPCODE_FMASK); - success = gsi_command(gsi, GSI_CH_CMD_OFFSET, val, completion); + timeout = !gsi_command(gsi, GSI_CH_CMD_OFFSET, val, completion); /* Disable the interrupt again */ gsi_irq_type_disable(gsi, GSI_CH_CTRL); iowrite32(0, gsi->virt + GSI_CNTXT_SRC_CH_IRQ_MSK_OFFSET); - if (success) + if (!timeout) return; dev_err(dev, "GSI command %u for channel %u timed out, state %u\n", @@ -1627,7 +1627,7 @@ static int gsi_generic_command(struct gsi *gsi, u32 channel_id, enum gsi_generic_cmd_opcode opcode) { struct completion *completion = &gsi->completion; - bool success; + bool timeout; u32 val; /* The error global interrupt type is always enabled (until we @@ -1650,12 +1650,12 @@ static int gsi_generic_command(struct gsi *gsi, u32 channel_id, val |= u32_encode_bits(channel_id, GENERIC_CHID_FMASK); val |= u32_encode_bits(GSI_EE_MODEM, GENERIC_EE_FMASK); - success = gsi_command(gsi, GSI_GENERIC_CMD_OFFSET, val, completion); + timeout = !gsi_command(gsi, GSI_GENERIC_CMD_OFFSET, val, completion); /* Disable the GP_INT1 IRQ type again */ iowrite32(BIT(ERROR_INT), gsi->virt + GSI_CNTXT_GLOB_IRQ_EN_OFFSET); - if (success) + if (!timeout) return gsi->result; dev_err(gsi->dev, "GSI generic command %u to channel %u timed out\n", From a60d0632f6e8e62584cd25d830c69dd09b0d4115 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Wed, 13 Jan 2021 11:15:28 -0600 Subject: [PATCH 2/6] net: ipa: introduce some interrupt helpers Create a new function gsi_irq_ev_ctrl_enable() that encapsulates enabling the event ring control GSI interrupt type, and enables a single event ring to signal that interrupt. When an event ring changes state as a result of an event ring command, it triggers this interrupt. Create an inverse function gsi_irq_ev_ctrl_disable() as well. Because only one event ring at a time is enabled for this interrupt, we can simply disable the interrupt for *all* channels. Create a pair of helpers that serve the same purpose for channel commands. Signed-off-by: Alex Elder Reviewed-by: Saeed Mahameed Signed-off-by: Jakub Kicinski --- drivers/net/ipa/gsi.c | 94 ++++++++++++++++++++++++++----------------- 1 file changed, 58 insertions(+), 36 deletions(-) diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c index b5913ce0bc94..e5681a39b5fc 100644 --- a/drivers/net/ipa/gsi.c +++ b/drivers/net/ipa/gsi.c @@ -220,6 +220,58 @@ static void gsi_irq_teardown(struct gsi *gsi) /* Nothing to do */ } +/* Event ring commands are performed one at a time. Their completion + * is signaled by the event ring control GSI interrupt type, which is + * only enabled when we issue an event ring command. Only the event + * ring being operated on has this interrupt enabled. + */ +static void gsi_irq_ev_ctrl_enable(struct gsi *gsi, u32 evt_ring_id) +{ + u32 val = BIT(evt_ring_id); + + /* There's a small chance that a previous command completed + * after the interrupt was disabled, so make sure we have no + * pending interrupts before we enable them. + */ + iowrite32(~0, gsi->virt + GSI_CNTXT_SRC_EV_CH_IRQ_CLR_OFFSET); + + iowrite32(val, gsi->virt + GSI_CNTXT_SRC_EV_CH_IRQ_MSK_OFFSET); + gsi_irq_type_enable(gsi, GSI_EV_CTRL); +} + +/* Disable event ring control interrupts */ +static void gsi_irq_ev_ctrl_disable(struct gsi *gsi) +{ + gsi_irq_type_disable(gsi, GSI_EV_CTRL); + iowrite32(0, gsi->virt + GSI_CNTXT_SRC_EV_CH_IRQ_MSK_OFFSET); +} + +/* Channel commands are performed one at a time. Their completion is + * signaled by the channel control GSI interrupt type, which is only + * enabled when we issue a channel command. Only the channel being + * operated on has this interrupt enabled. + */ +static void gsi_irq_ch_ctrl_enable(struct gsi *gsi, u32 channel_id) +{ + u32 val = BIT(channel_id); + + /* There's a small chance that a previous command completed + * after the interrupt was disabled, so make sure we have no + * pending interrupts before we enable them. + */ + iowrite32(~0, gsi->virt + GSI_CNTXT_SRC_CH_IRQ_CLR_OFFSET); + + iowrite32(val, gsi->virt + GSI_CNTXT_SRC_CH_IRQ_MSK_OFFSET); + gsi_irq_type_enable(gsi, GSI_CH_CTRL); +} + +/* Disable channel control interrupts */ +static void gsi_irq_ch_ctrl_disable(struct gsi *gsi) +{ + gsi_irq_type_disable(gsi, GSI_CH_CTRL); + iowrite32(0, gsi->virt + GSI_CNTXT_SRC_CH_IRQ_MSK_OFFSET); +} + static void gsi_irq_ieob_enable(struct gsi *gsi, u32 evt_ring_id) { bool enable_ieob = !gsi->ieob_enabled_bitmap; @@ -335,30 +387,15 @@ static void gsi_evt_ring_command(struct gsi *gsi, u32 evt_ring_id, bool timeout; u32 val; - /* We only perform one event ring command at a time, and event - * control interrupts should only occur when such a command - * is issued here. Only permit *this* event ring to trigger - * an interrupt, and only enable the event control IRQ type - * when we expect it to occur. - * - * There's a small chance that a previous command completed - * after the interrupt was disabled, so make sure we have no - * pending interrupts before we enable them. - */ - iowrite32(~0, gsi->virt + GSI_CNTXT_SRC_EV_CH_IRQ_CLR_OFFSET); - - val = BIT(evt_ring_id); - iowrite32(val, gsi->virt + GSI_CNTXT_SRC_EV_CH_IRQ_MSK_OFFSET); - gsi_irq_type_enable(gsi, GSI_EV_CTRL); + /* Enable the completion interrupt for the command */ + gsi_irq_ev_ctrl_enable(gsi, evt_ring_id); val = u32_encode_bits(evt_ring_id, EV_CHID_FMASK); val |= u32_encode_bits(opcode, EV_OPCODE_FMASK); timeout = !gsi_command(gsi, GSI_EV_CH_CMD_OFFSET, val, completion); - /* Disable the interrupt again */ - gsi_irq_type_disable(gsi, GSI_EV_CTRL); - iowrite32(0, gsi->virt + GSI_CNTXT_SRC_EV_CH_IRQ_MSK_OFFSET); + gsi_irq_ev_ctrl_disable(gsi); if (!timeout) return; @@ -459,29 +496,14 @@ gsi_channel_command(struct gsi_channel *channel, enum gsi_ch_cmd_opcode opcode) bool timeout; u32 val; - /* We only perform one channel command at a time, and channel - * control interrupts should only occur when such a command is - * issued here. So we only permit *this* channel to trigger - * an interrupt and only enable the channel control IRQ type - * when we expect it to occur. - * - * There's a small chance that a previous command completed - * after the interrupt was disabled, so make sure we have no - * pending interrupts before we enable them. - */ - iowrite32(~0, gsi->virt + GSI_CNTXT_SRC_CH_IRQ_CLR_OFFSET); - - val = BIT(channel_id); - iowrite32(val, gsi->virt + GSI_CNTXT_SRC_CH_IRQ_MSK_OFFSET); - gsi_irq_type_enable(gsi, GSI_CH_CTRL); + /* Enable the completion interrupt for the command */ + gsi_irq_ch_ctrl_enable(gsi, channel_id); val = u32_encode_bits(channel_id, CH_CHID_FMASK); val |= u32_encode_bits(opcode, CH_OPCODE_FMASK); timeout = !gsi_command(gsi, GSI_CH_CMD_OFFSET, val, completion); - /* Disable the interrupt again */ - gsi_irq_type_disable(gsi, GSI_CH_CTRL); - iowrite32(0, gsi->virt + GSI_CNTXT_SRC_CH_IRQ_MSK_OFFSET); + gsi_irq_ch_ctrl_disable(gsi); if (!timeout) return; From 74401946bdad6eb812d2d3c77f1ace849f963a6a Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Wed, 13 Jan 2021 11:15:29 -0600 Subject: [PATCH 3/6] net: ipa: use usleep_range() 65;6003;1c The use of msleep() for small periods (less than 20 milliseconds) is not recommended because the actual delay can be much different than expected. We use msleep(1) in several places in the IPA driver to insert short delays. Replace them with usleep_range calls, which should reliably delay a period in the range requested. Signed-off-by: Alex Elder Reviewed-by: Saeed Mahameed Signed-off-by: Jakub Kicinski --- drivers/net/ipa/gsi.c | 5 +++-- drivers/net/ipa/ipa_endpoint.c | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c index e5681a39b5fc..93b143ba92be 100644 --- a/drivers/net/ipa/gsi.c +++ b/drivers/net/ipa/gsi.c @@ -611,7 +611,8 @@ static void gsi_channel_reset_command(struct gsi_channel *channel) struct device *dev = channel->gsi->dev; enum gsi_channel_state state; - msleep(1); /* A short delay is required before a RESET command */ + /* A short delay is required before a RESET command */ + usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC); state = gsi_channel_state(channel); if (state != GSI_CHANNEL_STATE_STOPPED && @@ -900,7 +901,7 @@ int gsi_channel_stop(struct gsi *gsi, u32 channel_id) ret = gsi_channel_stop_command(channel); if (ret != -EAGAIN) break; - msleep(1); + usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC); } while (retries--); mutex_unlock(&gsi->mutex); diff --git a/drivers/net/ipa/ipa_endpoint.c b/drivers/net/ipa/ipa_endpoint.c index 9f4be9812a1f..688a3dd40510 100644 --- a/drivers/net/ipa/ipa_endpoint.c +++ b/drivers/net/ipa/ipa_endpoint.c @@ -1378,7 +1378,7 @@ static int ipa_endpoint_reset_rx_aggr(struct ipa_endpoint *endpoint) do { if (!ipa_endpoint_aggr_active(endpoint)) break; - msleep(1); + usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC); } while (retries--); /* Check one last time */ @@ -1399,7 +1399,7 @@ static int ipa_endpoint_reset_rx_aggr(struct ipa_endpoint *endpoint) */ gsi_channel_reset(gsi, endpoint->channel_id, true); - msleep(1); + usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC); goto out_suspend_again; From 59b5f45496253ae977792cbe82480686be46b005 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Wed, 13 Jan 2021 11:15:30 -0600 Subject: [PATCH 4/6] net: ipa: change GSI command timeout The GSI command timeout is currently 5 seconds, which is much higher than it should be. Express the timeout in milliseconds rather than seconds, and reduce it to 50 milliseconds. Signed-off-by: Alex Elder Reviewed-by: Saeed Mahameed Signed-off-by: Jakub Kicinski --- drivers/net/ipa/gsi.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c index 93b143ba92be..4de769166978 100644 --- a/drivers/net/ipa/gsi.c +++ b/drivers/net/ipa/gsi.c @@ -89,7 +89,7 @@ /* Delay period for interrupt moderation (in 32KHz IPA internal timer ticks) */ #define GSI_EVT_RING_INT_MODT (32 * 1) /* 1ms under 32KHz clock */ -#define GSI_CMD_TIMEOUT 5 /* seconds */ +#define GSI_CMD_TIMEOUT 50 /* milliseconds */ #define GSI_CHANNEL_STOP_RX_RETRIES 10 #define GSI_CHANNEL_MODEM_HALT_RETRIES 10 @@ -359,11 +359,13 @@ static u32 gsi_ring_index(struct gsi_ring *ring, u32 offset) static bool gsi_command(struct gsi *gsi, u32 reg, u32 val, struct completion *completion) { + unsigned long timeout = msecs_to_jiffies(GSI_CMD_TIMEOUT); + reinit_completion(completion); iowrite32(val, gsi->virt + reg); - return !!wait_for_completion_timeout(completion, GSI_CMD_TIMEOUT * HZ); + return !!wait_for_completion_timeout(completion, timeout); } /* Return the hardware's notion of the current state of an event ring */ From 3d60e15f6ead2f4a56903a8c737e7f522c867827 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Wed, 13 Jan 2021 11:15:31 -0600 Subject: [PATCH 5/6] net: ipa: change stop channel retry delay If a GSI stop channel command leaves the channel in STOP_IN_PROC state, we retry the stop command after a 1-2 millisecond delay. I have been told that a 3-5 millisecond delay is a better choice. Signed-off-by: Alex Elder Reviewed-by: Saeed Mahameed Signed-off-by: Jakub Kicinski --- drivers/net/ipa/gsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c index 4de769166978..5c163f9c0ea7 100644 --- a/drivers/net/ipa/gsi.c +++ b/drivers/net/ipa/gsi.c @@ -903,7 +903,7 @@ int gsi_channel_stop(struct gsi *gsi, u32 channel_id) ret = gsi_channel_stop_command(channel); if (ret != -EAGAIN) break; - usleep_range(USEC_PER_MSEC, 2 * USEC_PER_MSEC); + usleep_range(3 * USEC_PER_MSEC, 5 * USEC_PER_MSEC); } while (retries--); mutex_unlock(&gsi->mutex); From 057ef63f755f4ef15eb534f922c1d057c50d4571 Mon Sep 17 00:00:00 2001 From: Alex Elder Date: Wed, 13 Jan 2021 11:15:32 -0600 Subject: [PATCH 6/6] net: ipa: retry TX channel stop commands For RX channels we issue a stop command more than once if necessary to allow it to stop. It turns out that TX channels could also require retries. Retry channel stop commands if necessary regardless of the channel direction. Rename the symbol defining the retry count so it's not RX-specific. Signed-off-by: Alex Elder Reviewed-by: Saeed Mahameed Signed-off-by: Jakub Kicinski --- drivers/net/ipa/gsi.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/net/ipa/gsi.c b/drivers/net/ipa/gsi.c index 5c163f9c0ea7..5b29f7d9d6ac 100644 --- a/drivers/net/ipa/gsi.c +++ b/drivers/net/ipa/gsi.c @@ -91,7 +91,7 @@ #define GSI_CMD_TIMEOUT 50 /* milliseconds */ -#define GSI_CHANNEL_STOP_RX_RETRIES 10 +#define GSI_CHANNEL_STOP_RETRIES 10 #define GSI_CHANNEL_MODEM_HALT_RETRIES 10 #define GSI_MHI_EVENT_ID_START 10 /* 1st reserved event id */ @@ -889,14 +889,11 @@ int gsi_channel_start(struct gsi *gsi, u32 channel_id) int gsi_channel_stop(struct gsi *gsi, u32 channel_id) { struct gsi_channel *channel = &gsi->channel[channel_id]; - u32 retries; + u32 retries = GSI_CHANNEL_STOP_RETRIES; int ret; gsi_channel_freeze(channel); - /* RX channels might require a little time to enter STOPPED state */ - retries = channel->toward_ipa ? 0 : GSI_CHANNEL_STOP_RX_RETRIES; - mutex_lock(&gsi->mutex); do {