A few (independent) fixes to the MHI bus for issues that I have come across
while developing against the mainline code.
v2:
-fix syserr reset log message
-fix power up error check code style
-add change to remove pci assumptions for register accesses
-add comment typo fix
Jeffrey Hugo (5):
bus: mhi: core: Handle syserr during power_up
bus: mhi: core: Make sure to powerdown if mhi_sync_power_up fails
bus: mhi: core: Remove link_status() callback
bus: mhi: core: Offload register accesses to the controller
bus: mhi: core: Fix typo in comment
drivers/bus/mhi/core/init.c | 7 +++----
drivers/bus/mhi/core/internal.h | 3 ---
drivers/bus/mhi/core/main.c | 13 ++-----------
drivers/bus/mhi/core/pm.c | 26 +++++++++++++++++++++++++-
include/linux/mhi.h | 10 +++++++---
5 files changed, 37 insertions(+), 22 deletions(-)
--
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
The MHI device may be in the syserr state when we attempt to init it in
power_up(). Since we have no local state, the handling is simple -
reset the device and wait for it to transition out of the reset state.
Signed-off-by: Jeffrey Hugo <[email protected]>
---
drivers/bus/mhi/core/pm.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 52690cb..3285c9e 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -9,6 +9,7 @@
#include <linux/dma-direction.h>
#include <linux/dma-mapping.h>
#include <linux/interrupt.h>
+#include <linux/iopoll.h>
#include <linux/list.h>
#include <linux/mhi.h>
#include <linux/module.h>
@@ -760,6 +761,7 @@ static void mhi_deassert_dev_wake(struct mhi_controller *mhi_cntrl,
int mhi_async_power_up(struct mhi_controller *mhi_cntrl)
{
+ enum mhi_state state;
enum mhi_ee_type current_ee;
enum dev_st_transition next_state;
struct device *dev = &mhi_cntrl->mhi_dev->dev;
@@ -829,6 +831,24 @@ int mhi_async_power_up(struct mhi_controller *mhi_cntrl)
goto error_bhi_offset;
}
+ state = mhi_get_mhi_state(mhi_cntrl);
+ if (state == MHI_STATE_SYS_ERR) {
+ mhi_set_mhi_state(mhi_cntrl, MHI_STATE_RESET);
+ ret = readl_poll_timeout(mhi_cntrl->regs + MHICTRL, val,
+ !(val & MHICTRL_RESET_MASK), 1000,
+ mhi_cntrl->timeout_ms * 1000);
+ if (ret) {
+ dev_info(dev, "Failed to reset MHI due to syserr state\n");
+ goto error_bhi_offset;
+ }
+
+ /*
+ * device cleares INTVEC as part of RESET processing,
+ * re-program it
+ */
+ mhi_write_reg(mhi_cntrl, mhi_cntrl->bhi, BHI_INTVEC, 0);
+ }
+
/* Transition to next state */
next_state = MHI_IN_PBL(current_ee) ?
DEV_ST_TRANSITION_PBL : DEV_ST_TRANSITION_READY;
--
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
If the MHI core detects invalid data due to a PCI read, it calls into
the controller via link_status() to double check that the link is infact
down. All in all, this is pretty pointless, and racy. There are no good
reasons for this, and only drawbacks.
Its pointless because chances are, the controller is going to do the same
thing to determine if the link is down - attempt a PCI access and compare
the result. This does not make the link status decision any smarter.
Its racy because its possible that the link was down at the time of the
MHI core access, but then recovered before the controller access. In this
case, the controller will indicate the link is not down, and the MHI core
will precede to use a bad value as the MHI core does not attempt to retry
the access.
Retrying the access in the MHI core is a bad idea because again, it is
racy - what if the link is down again? Furthermore, there may be some
higher level state associated with the link status, that is now invalid
because the link went down.
The only reason why the MHI core could see "invalid" data when doing a PCI
access, that is actually valid, is if the register actually contained the
PCI spec defined sentinel for an invalid access. In this case, it is
arguable that the MHI implementation broken, and should be fixed, not
worked around.
Therefore, remove the link_status() callback before anyone attempts to
implement it.
Signed-off-by: Jeffrey Hugo <[email protected]>
Reviewed-by: Manivannan Sadhasivam <[email protected]>
---
drivers/bus/mhi/core/init.c | 6 ++----
drivers/bus/mhi/core/main.c | 5 ++---
include/linux/mhi.h | 2 --
3 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index b38359c..2af08d57 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -812,10 +812,8 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
if (!mhi_cntrl)
return -EINVAL;
- if (!mhi_cntrl->runtime_get || !mhi_cntrl->runtime_put)
- return -EINVAL;
-
- if (!mhi_cntrl->status_cb || !mhi_cntrl->link_status)
+ if (!mhi_cntrl->runtime_get || !mhi_cntrl->runtime_put ||
+ !mhi_cntrl->status_cb)
return -EINVAL;
ret = parse_config(mhi_cntrl, config);
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index eb4256b..473278b8 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -20,9 +20,8 @@ int __must_check mhi_read_reg(struct mhi_controller *mhi_cntrl,
{
u32 tmp = readl(base + offset);
- /* If there is any unexpected value, query the link status */
- if (PCI_INVALID_READ(tmp) &&
- mhi_cntrl->link_status(mhi_cntrl))
+ /* If the value is invalid, the link is down */
+ if (PCI_INVALID_READ(tmp))
return -EIO;
*out = tmp;
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index ad19960..be704a4 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -335,7 +335,6 @@ struct mhi_controller_config {
* @syserr_worker: System error worker
* @state_event: State change event
* @status_cb: CB function to notify power states of the device (required)
- * @link_status: CB function to query link status of the device (required)
* @wake_get: CB function to assert device wake (optional)
* @wake_put: CB function to de-assert device wake (optional)
* @wake_toggle: CB function to assert and de-assert device wake (optional)
@@ -417,7 +416,6 @@ struct mhi_controller {
void (*status_cb)(struct mhi_controller *mhi_cntrl,
enum mhi_callback cb);
- int (*link_status)(struct mhi_controller *mhi_cntrl);
void (*wake_get)(struct mhi_controller *mhi_cntrl, bool override);
void (*wake_put)(struct mhi_controller *mhi_cntrl, bool override);
void (*wake_toggle)(struct mhi_controller *mhi_cntrl);
--
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
There is a typo - "runtimet" should be "runtime". Fix it.
Signed-off-by: Jeffrey Hugo <[email protected]>
---
include/linux/mhi.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index 225a03a..effa172 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -339,7 +339,7 @@ struct mhi_controller_config {
* @wake_put: CB function to de-assert device wake (optional)
* @wake_toggle: CB function to assert and de-assert device wake (optional)
* @runtime_get: CB function to controller runtime resume (required)
- * @runtimet_put: CB function to decrement pm usage (required)
+ * @runtime_put: CB function to decrement pm usage (required)
* @map_single: CB function to create TRE buffer
* @unmap_single: CB function to destroy TRE buffer
* @read_reg: Read a MHI register via the physical link (required)
--
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
When reading or writing MHI registers, the core assumes that the physical
link is a memory mapped PCI link. This assumption may not hold for all
MHI devices. The controller knows what is the physical link (ie PCI, I2C,
SPI, etc), and therefore knows the proper methods to access that link.
The controller can also handle link specific error scenarios, such as
reading -1 when the PCI link went down.
Therefore, it is appropiate that the MHI core requests the controller to
make register accesses on behalf of the core, which abstracts the core
from link specifics, and end up removing an unnecessary assumption.
Signed-off-by: Jeffrey Hugo <[email protected]>
---
drivers/bus/mhi/core/init.c | 3 ++-
drivers/bus/mhi/core/internal.h | 3 ---
drivers/bus/mhi/core/main.c | 12 ++----------
include/linux/mhi.h | 6 ++++++
4 files changed, 10 insertions(+), 14 deletions(-)
diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index 2af08d57..eb2ab05 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -813,7 +813,8 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
return -EINVAL;
if (!mhi_cntrl->runtime_get || !mhi_cntrl->runtime_put ||
- !mhi_cntrl->status_cb)
+ !mhi_cntrl->status_cb || !mhi_cntrl->read_reg ||
+ !mhi_cntrl->write_reg)
return -EINVAL;
ret = parse_config(mhi_cntrl, config);
diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
index 5deadfa..095d95b 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -11,9 +11,6 @@
extern struct bus_type mhi_bus_type;
-/* MHI MMIO register mapping */
-#define PCI_INVALID_READ(val) (val == U32_MAX)
-
#define MHIREGLEN (0x0)
#define MHIREGLEN_MHIREGLEN_MASK (0xFFFFFFFF)
#define MHIREGLEN_MHIREGLEN_SHIFT (0)
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 473278b8..580d72b 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -18,15 +18,7 @@
int __must_check mhi_read_reg(struct mhi_controller *mhi_cntrl,
void __iomem *base, u32 offset, u32 *out)
{
- u32 tmp = readl(base + offset);
-
- /* If the value is invalid, the link is down */
- if (PCI_INVALID_READ(tmp))
- return -EIO;
-
- *out = tmp;
-
- return 0;
+ return mhi_cntrl->read_reg(mhi_cntrl, base + offset, out);
}
int __must_check mhi_read_reg_field(struct mhi_controller *mhi_cntrl,
@@ -48,7 +40,7 @@ int __must_check mhi_read_reg_field(struct mhi_controller *mhi_cntrl,
void mhi_write_reg(struct mhi_controller *mhi_cntrl, void __iomem *base,
u32 offset, u32 val)
{
- writel(val, base + offset);
+ mhi_cntrl->write_reg(mhi_cntrl, base + offset, val);
}
void mhi_write_reg_field(struct mhi_controller *mhi_cntrl, void __iomem *base,
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index be704a4..225a03a 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -342,6 +342,8 @@ struct mhi_controller_config {
* @runtimet_put: CB function to decrement pm usage (required)
* @map_single: CB function to create TRE buffer
* @unmap_single: CB function to destroy TRE buffer
+ * @read_reg: Read a MHI register via the physical link (required)
+ * @write_reg: Write a MHI register via the physical link (required)
* @buffer_len: Bounce buffer length
* @bounce_buf: Use of bounce buffer
* @fbc_download: MHI host needs to do complete image transfer (optional)
@@ -425,6 +427,10 @@ struct mhi_controller {
struct mhi_buf_info *buf);
void (*unmap_single)(struct mhi_controller *mhi_cntrl,
struct mhi_buf_info *buf);
+ int (*read_reg)(struct mhi_controller *mhi_cntrl, void __iomem *addr,
+ u32 *out);
+ void (*write_reg)(struct mhi_controller *mhi_cntrl, void __iomem *addr,
+ u32 val);
size_t buffer_len;
bool bounce_buf;
--
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
Powerdown is necessary if mhi_sync_power_up fails due to a timeout, to
clean up the resources. Otherwise a BUG could be triggered when
attempting to clean up MSIs because the IRQ is still active from a
request_irq().
Signed-off-by: Jeffrey Hugo <[email protected]>
---
drivers/bus/mhi/core/pm.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 3285c9e..fbffc6b 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -922,7 +922,11 @@ int mhi_sync_power_up(struct mhi_controller *mhi_cntrl)
MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state),
msecs_to_jiffies(mhi_cntrl->timeout_ms));
- return (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 : -EIO;
+ ret = (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 : -EIO;
+ if (ret)
+ mhi_power_down(mhi_cntrl, false);
+
+ return ret;
}
EXPORT_SYMBOL(mhi_sync_power_up);
--
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
On 2020-04-07 09:50, Jeffrey Hugo wrote:
> Powerdown is necessary if mhi_sync_power_up fails due to a timeout, to
> clean up the resources. Otherwise a BUG could be triggered when
> attempting to clean up MSIs because the IRQ is still active from a
> request_irq().
>
> Signed-off-by: Jeffrey Hugo <[email protected]>
> ---
> drivers/bus/mhi/core/pm.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index 3285c9e..fbffc6b 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -922,7 +922,11 @@ int mhi_sync_power_up(struct mhi_controller
> *mhi_cntrl)
> MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state),
> msecs_to_jiffies(mhi_cntrl->timeout_ms));
>
> - return (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 : -EIO;
> + ret = (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 : -EIO;
Does it make sense to return -ETIMEDOUT instead of -EIO if device fails
to move to mission mode?
Controller can use this info as mhi_async_power_up() would not return
-ETIMEDOUT.
> + if (ret)
> + mhi_power_down(mhi_cntrl, false);
> +
> + return ret;
> }
> EXPORT_SYMBOL(mhi_sync_power_up);
On 4/7/2020 7:34 PM, [email protected] wrote:
> On 2020-04-07 09:50, Jeffrey Hugo wrote:
>> Powerdown is necessary if mhi_sync_power_up fails due to a timeout, to
>> clean up the resources. Otherwise a BUG could be triggered when
>> attempting to clean up MSIs because the IRQ is still active from a
>> request_irq().
>>
>> Signed-off-by: Jeffrey Hugo <[email protected]>
>> ---
>> drivers/bus/mhi/core/pm.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
>> index 3285c9e..fbffc6b 100644
>> --- a/drivers/bus/mhi/core/pm.c
>> +++ b/drivers/bus/mhi/core/pm.c
>> @@ -922,7 +922,11 @@ int mhi_sync_power_up(struct mhi_controller
>> *mhi_cntrl)
>> MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state),
>> msecs_to_jiffies(mhi_cntrl->timeout_ms));
>>
>> - return (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 : -EIO;
>> + ret = (MHI_IN_MISSION_MODE(mhi_cntrl->ee)) ? 0 : -EIO;
>
> Does it make sense to return -ETIMEDOUT instead of -EIO if device fails
> to move to mission mode?
> Controller can use this info as mhi_async_power_up() would not return
> -ETIMEDOUT.
It seems sensible to change this to ETIMEDOUT. I'll queue that up for V3.
>
>> + if (ret)
>> + mhi_power_down(mhi_cntrl, false);
>> +
>> + return ret;
>> }
>> EXPORT_SYMBOL(mhi_sync_power_up);
--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
On 4/7/20 9:50 AM, Jeffrey Hugo wrote:
> The MHI device may be in the syserr state when we attempt to init it in
> power_up(). Since we have no local state, the handling is simple -
> reset the device and wait for it to transition out of the reset state.
>
> Signed-off-by: Jeffrey Hugo <[email protected]>
> ---
> drivers/bus/mhi/core/pm.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
>
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index 52690cb..3285c9e 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -9,6 +9,7 @@
> #include <linux/dma-direction.h>
> #include <linux/dma-mapping.h>
> #include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> #include <linux/list.h>
> #include <linux/mhi.h>
> #include <linux/module.h>
> @@ -760,6 +761,7 @@ static void mhi_deassert_dev_wake(struct mhi_controller *mhi_cntrl,
>
> int mhi_async_power_up(struct mhi_controller *mhi_cntrl)
> {
> + enum mhi_state state;
> enum mhi_ee_type current_ee;
> enum dev_st_transition next_state;
> struct device *dev = &mhi_cntrl->mhi_dev->dev;
> @@ -829,6 +831,24 @@ int mhi_async_power_up(struct mhi_controller *mhi_cntrl)
> goto error_bhi_offset;
> }
>
> + state = mhi_get_mhi_state(mhi_cntrl);
> + if (state == MHI_STATE_SYS_ERR) {
> + mhi_set_mhi_state(mhi_cntrl, MHI_STATE_RESET);
> + ret = readl_poll_timeout(mhi_cntrl->regs + MHICTRL, val,
> + !(val & MHICTRL_RESET_MASK), 1000,
> + mhi_cntrl->timeout_ms * 1000);
can we use this instead of polling because MSI is configures and int_vec
handler is registered
wait_event_timeout(mhi_cntrl->state_event,
MHI_PM_IN_FATAL_STATE(mhi_cntrl->pm_state) ||
mhi_read_reg_field(mhi_cntrl, base, MHICTRL,
MHICTRL_RESET_MASK,
MHICTRL_RESET_SHIFT, &reset) || !reset ,
msecs_to_jiffies(mhi_cntrl->timeout_ms));
1) In case of MHI_PM_IN_FATAL_STATE we would not be accessing MHI reg
2) Consistent with current MHI driver code.
> + if (ret) {
> + dev_info(dev, "Failed to reset MHI due to syserr state\n");
> + goto error_bhi_offset;
> + }
> +
> + /*
> + * device cleares INTVEC as part of RESET processing,
> + * re-program it
> + */
> + mhi_write_reg(mhi_cntrl, mhi_cntrl->bhi, BHI_INTVEC, 0);
> + }
> +
> /* Transition to next state */
> next_state = MHI_IN_PBL(current_ee) ?
> DEV_ST_TRANSITION_PBL : DEV_ST_TRANSITION_READY;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On 4/9/2020 6:55 PM, Hemant Kumar wrote:
>
> On 4/7/20 9:50 AM, Jeffrey Hugo wrote:
>> The MHI device may be in the syserr state when we attempt to init it in
>> power_up(). Since we have no local state, the handling is simple -
>> reset the device and wait for it to transition out of the reset state.
>>
>> Signed-off-by: Jeffrey Hugo <[email protected]>
>> ---
>> drivers/bus/mhi/core/pm.c | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
>> index 52690cb..3285c9e 100644
>> --- a/drivers/bus/mhi/core/pm.c
>> +++ b/drivers/bus/mhi/core/pm.c
>> @@ -9,6 +9,7 @@
>> #include <linux/dma-direction.h>
>> #include <linux/dma-mapping.h>
>> #include <linux/interrupt.h>
>> +#include <linux/iopoll.h>
>> #include <linux/list.h>
>> #include <linux/mhi.h>
>> #include <linux/module.h>
>> @@ -760,6 +761,7 @@ static void mhi_deassert_dev_wake(struct
>> mhi_controller *mhi_cntrl,
>> int mhi_async_power_up(struct mhi_controller *mhi_cntrl)
>> {
>> + enum mhi_state state;
>> enum mhi_ee_type current_ee;
>> enum dev_st_transition next_state;
>> struct device *dev = &mhi_cntrl->mhi_dev->dev;
>> @@ -829,6 +831,24 @@ int mhi_async_power_up(struct mhi_controller
>> *mhi_cntrl)
>> goto error_bhi_offset;
>> }
>> + state = mhi_get_mhi_state(mhi_cntrl);
>> + if (state == MHI_STATE_SYS_ERR) {
>> + mhi_set_mhi_state(mhi_cntrl, MHI_STATE_RESET);
>> + ret = readl_poll_timeout(mhi_cntrl->regs + MHICTRL, val,
>> + !(val & MHICTRL_RESET_MASK), 1000,
>> + mhi_cntrl->timeout_ms * 1000);
> can we use this instead of polling because MSI is configures and int_vec
> handler is registered
>
> wait_event_timeout(mhi_cntrl->state_event,
> MHI_PM_IN_FATAL_STATE(mhi_cntrl->pm_state) ||
> mhi_read_reg_field(mhi_cntrl, base, MHICTRL,
> MHICTRL_RESET_MASK,
> MHICTRL_RESET_SHIFT, &reset) || !reset ,
> msecs_to_jiffies(mhi_cntrl->timeout_ms));
>
> 1) In case of MHI_PM_IN_FATAL_STATE we would not be accessing MHI reg
> 2) Consistent with current MHI driver code.
I'm not sure this works in the way you intend.
state_event is linked to the intvec, which is the BHI interrupt. I
don't see that the state_event is triggered in the MHI interrupt path
(mhi_irq_handler). So, if we are in the PBL EE, we would expect to see
the BHI interrupt, but if we are in the AMSS EE, we would expect to see
a MHI interrupt.
Now, for my concerned usecase, those two interrupts happen to be the
same interrupt, so both will get triggered, but I don't expect that to
be the same for all usecases.
So, with the solution I propose, we exit the wait (poll loop) as soon as
we see the register change values.
With the solution you propose, if we only get the MHI interrupt, we'll
have to wait out the entire timeout value, and then check the register.
In this scenario, we are almost guaranteed to wait for longer than
necessary.
Did I miss something?
>> + if (ret) {
>> + dev_info(dev, "Failed to reset MHI due to syserr state\n");
>> + goto error_bhi_offset;
>> + }
>> +
>> + /*
>> + * device cleares INTVEC as part of RESET processing,
>> + * re-program it
>> + */
>> + mhi_write_reg(mhi_cntrl, mhi_cntrl->bhi, BHI_INTVEC, 0);
>> + }
>> +
>> /* Transition to next state */
>> next_state = MHI_IN_PBL(current_ee) ?
>> DEV_ST_TRANSITION_PBL : DEV_ST_TRANSITION_READY;
>
--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
Hi Jeff,
We will always have the mhi_intvec_handler registered and trigger your
wake_up state event when you write MHI RESET. BHI INTVEC IRQ using
mhi_cntrl->irq[0] is _not_ unregistered once you enter AMSS EE.
So, your below assumption is not true:
>>>So, if we are in the PBL EE, we would expect to see the BHI
interrupt, but if we are in the AMSS EE, we would expect to see a MHI
interrupt.
At the start of mhi_async_power_up(), you've already registered for the
BHI interrupt as we do setup for IRQ and it is only unregistered from
power down if power up on the same cycle was a success.
On 4/10/20 8:03 AM, Jeffrey Hugo wrote:
> On 4/9/2020 6:55 PM, Hemant Kumar wrote:
>>
>> On 4/7/20 9:50 AM, Jeffrey Hugo wrote:
>>> The MHI device may be in the syserr state when we attempt to init it in
>>> power_up(). Since we have no local state, the handling is simple -
>>> reset the device and wait for it to transition out of the reset state.
>>>
>>> Signed-off-by: Jeffrey Hugo <[email protected]>
>>> ---
>>> drivers/bus/mhi/core/pm.c | 20 ++++++++++++++++++++
>>> 1 file changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
>>> index 52690cb..3285c9e 100644
>>> --- a/drivers/bus/mhi/core/pm.c
>>> +++ b/drivers/bus/mhi/core/pm.c
>>> @@ -9,6 +9,7 @@
>>> #include <linux/dma-direction.h>
>>> #include <linux/dma-mapping.h>
>>> #include <linux/interrupt.h>
>>> +#include <linux/iopoll.h>
>>> #include <linux/list.h>
>>> #include <linux/mhi.h>
>>> #include <linux/module.h>
>>> @@ -760,6 +761,7 @@ static void mhi_deassert_dev_wake(struct
>>> mhi_controller *mhi_cntrl,
>>> int mhi_async_power_up(struct mhi_controller *mhi_cntrl)
>>> {
>>> + enum mhi_state state;
>>> enum mhi_ee_type current_ee;
>>> enum dev_st_transition next_state;
>>> struct device *dev = &mhi_cntrl->mhi_dev->dev;
>>> @@ -829,6 +831,24 @@ int mhi_async_power_up(struct mhi_controller
>>> *mhi_cntrl)
>>> goto error_bhi_offset;
>>> }
>>> + state = mhi_get_mhi_state(mhi_cntrl);
>>> + if (state == MHI_STATE_SYS_ERR) {
>>> + mhi_set_mhi_state(mhi_cntrl, MHI_STATE_RESET);
>>> + ret = readl_poll_timeout(mhi_cntrl->regs + MHICTRL, val,
>>> + !(val & MHICTRL_RESET_MASK), 1000,
>>> + mhi_cntrl->timeout_ms * 1000);
>> can we use this instead of polling because MSI is configures and
>> int_vec handler is registered
>>
>> wait_event_timeout(mhi_cntrl->state_event,
>> MHI_PM_IN_FATAL_STATE(mhi_cntrl->pm_state) ||
>> mhi_read_reg_field(mhi_cntrl, base, MHICTRL,
>> MHICTRL_RESET_MASK,
>> MHICTRL_RESET_SHIFT, &reset) || !reset ,
>> msecs_to_jiffies(mhi_cntrl->timeout_ms));
>>
>> 1) In case of MHI_PM_IN_FATAL_STATE we would not be accessing MHI reg
>> 2) Consistent with current MHI driver code.
>
> I'm not sure this works in the way you intend.
>
> state_event is linked to the intvec, which is the BHI interrupt. I
> don't see that the state_event is triggered in the MHI interrupt path
> (mhi_irq_handler). So, if we are in the PBL EE, we would expect to
> see the BHI interrupt, but if we are in the AMSS EE, we would expect
> to see a MHI interrupt.
>
> Now, for my concerned usecase, those two interrupts happen to be the
> same interrupt, so both will get triggered, but I don't expect that to
> be the same for all usecases.
>
> So, with the solution I propose, we exit the wait (poll loop) as soon
> as we see the register change values.
>
> With the solution you propose, if we only get the MHI interrupt, we'll
> have to wait out the entire timeout value, and then check the
> register. In this scenario, we are almost guaranteed to wait for
> longer than necessary.
>
> Did I miss something?
>
>>> + if (ret) {
>>> + dev_info(dev, "Failed to reset MHI due to syserr
>>> state\n");
>>> + goto error_bhi_offset;
>>> + }
>>> +
>>> + /*
>>> + * device cleares INTVEC as part of RESET processing,
>>> + * re-program it
>>> + */
>>> + mhi_write_reg(mhi_cntrl, mhi_cntrl->bhi, BHI_INTVEC, 0);
>>> + }
>>> +
>>> /* Transition to next state */
>>> next_state = MHI_IN_PBL(current_ee) ?
>>> DEV_ST_TRANSITION_PBL : DEV_ST_TRANSITION_READY;
>>
>
>
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
On 4/10/2020 2:37 PM, Bhaumik Vasav Bhatt wrote:
> Hi Jeff,
>
> We will always have the mhi_intvec_handler registered and trigger your
> wake_up state event when you write MHI RESET. BHI INTVEC IRQ using
> mhi_cntrl->irq[0] is _not_ unregistered once you enter AMSS EE.
I understand it is not unregistered. However mhi_cntrl->irq[0] may be
reserved for BHI, and thus only exercised by PBL EE. Where as,
mhi_cntrl->irq[1..N] may be only exercised by AMSS EE.
mhi_intvec_handler is not called in response to mhi_cntrl->irq[1..N].
Additionally, I re-reviewed the MHI spec, and I don't see where the spec
requires the device to issue an interrupt upon completion of the RESET
request.
Under section 3.5, step 11 states -
"The host must poll for the value of the RESET bit to detect the
completion of the reset procedure by the device (RESET is set to 0)."
So, by this, my proposed solution appears to be spec compliant, where as
what Hemant proposed is not.
>
> So, your below assumption is not true:
> >>>So, if we are in the PBL EE, we would expect to see the BHI
> interrupt, but if we are in the AMSS EE, we would expect to see a MHI
> interrupt.
>
> At the start of mhi_async_power_up(), you've already registered for the
> BHI interrupt as we do setup for IRQ and it is only unregistered from
> power down if power up on the same cycle was a success.
You seem to have misunderstood my point. If the BHI irq is only for BHI
activity, which is activity restricted to the PBL EE, and the MHI
interrupt(s) are restricted to MHI activity, which for the purposes of
this discussion only occur in the AMSS EE, then my assumption is
correct. When the device is in PBL EE, we should only observe BHI irqs,
and when the device is in AMSS EE, we should only observe MHI irqs.
This is a statement of what IRQ lines the device is raising, and not a
statement of what handlers the host has, or has not registered.
Again, if the BHI irq is only generated in the PBL EE, and we rely on
the BHI irq for "sensing" the state_event - we will never see the
state_event in the AMSS EE, unless the same IRQ line is used for both
MHI and BHI (which is only a select set of usecases, and not universal).
>
> On 4/10/20 8:03 AM, Jeffrey Hugo wrote:
>> On 4/9/2020 6:55 PM, Hemant Kumar wrote:
>>>
>>> On 4/7/20 9:50 AM, Jeffrey Hugo wrote:
>>>> The MHI device may be in the syserr state when we attempt to init it in
>>>> power_up(). Since we have no local state, the handling is simple -
>>>> reset the device and wait for it to transition out of the reset state.
>>>>
>>>> Signed-off-by: Jeffrey Hugo <[email protected]>
>>>> ---
>>>> drivers/bus/mhi/core/pm.c | 20 ++++++++++++++++++++
>>>> 1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
>>>> index 52690cb..3285c9e 100644
>>>> --- a/drivers/bus/mhi/core/pm.c
>>>> +++ b/drivers/bus/mhi/core/pm.c
>>>> @@ -9,6 +9,7 @@
>>>> #include <linux/dma-direction.h>
>>>> #include <linux/dma-mapping.h>
>>>> #include <linux/interrupt.h>
>>>> +#include <linux/iopoll.h>
>>>> #include <linux/list.h>
>>>> #include <linux/mhi.h>
>>>> #include <linux/module.h>
>>>> @@ -760,6 +761,7 @@ static void mhi_deassert_dev_wake(struct
>>>> mhi_controller *mhi_cntrl,
>>>> int mhi_async_power_up(struct mhi_controller *mhi_cntrl)
>>>> {
>>>> + enum mhi_state state;
>>>> enum mhi_ee_type current_ee;
>>>> enum dev_st_transition next_state;
>>>> struct device *dev = &mhi_cntrl->mhi_dev->dev;
>>>> @@ -829,6 +831,24 @@ int mhi_async_power_up(struct mhi_controller
>>>> *mhi_cntrl)
>>>> goto error_bhi_offset;
>>>> }
>>>> + state = mhi_get_mhi_state(mhi_cntrl);
>>>> + if (state == MHI_STATE_SYS_ERR) {
>>>> + mhi_set_mhi_state(mhi_cntrl, MHI_STATE_RESET);
>>>> + ret = readl_poll_timeout(mhi_cntrl->regs + MHICTRL, val,
>>>> + !(val & MHICTRL_RESET_MASK), 1000,
>>>> + mhi_cntrl->timeout_ms * 1000);
>>> can we use this instead of polling because MSI is configures and
>>> int_vec handler is registered
>>>
>>> wait_event_timeout(mhi_cntrl->state_event,
>>> MHI_PM_IN_FATAL_STATE(mhi_cntrl->pm_state) ||
>>> mhi_read_reg_field(mhi_cntrl, base, MHICTRL,
>>> MHICTRL_RESET_MASK,
>>> MHICTRL_RESET_SHIFT, &reset) || !reset ,
>>> msecs_to_jiffies(mhi_cntrl->timeout_ms));
>>>
>>> 1) In case of MHI_PM_IN_FATAL_STATE we would not be accessing MHI reg
>>> 2) Consistent with current MHI driver code.
>>
>> I'm not sure this works in the way you intend.
>>
>> state_event is linked to the intvec, which is the BHI interrupt. I
>> don't see that the state_event is triggered in the MHI interrupt path
>> (mhi_irq_handler). So, if we are in the PBL EE, we would expect to
>> see the BHI interrupt, but if we are in the AMSS EE, we would expect
>> to see a MHI interrupt.
>>
>> Now, for my concerned usecase, those two interrupts happen to be the
>> same interrupt, so both will get triggered, but I don't expect that to
>> be the same for all usecases.
>>
>> So, with the solution I propose, we exit the wait (poll loop) as soon
>> as we see the register change values.
>>
>> With the solution you propose, if we only get the MHI interrupt, we'll
>> have to wait out the entire timeout value, and then check the
>> register. In this scenario, we are almost guaranteed to wait for
>> longer than necessary.
>>
>> Did I miss something?
>>
>>>> + if (ret) {
>>>> + dev_info(dev, "Failed to reset MHI due to syserr
>>>> state\n");
>>>> + goto error_bhi_offset;
>>>> + }
>>>> +
>>>> + /*
>>>> + * device cleares INTVEC as part of RESET processing,
>>>> + * re-program it
>>>> + */
>>>> + mhi_write_reg(mhi_cntrl, mhi_cntrl->bhi, BHI_INTVEC, 0);
>>>> + }
>>>> +
>>>> /* Transition to next state */
>>>> next_state = MHI_IN_PBL(current_ee) ?
>>>> DEV_ST_TRANSITION_PBL : DEV_ST_TRANSITION_READY;
>>>
>>
>>
--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
On Fri, Apr 10, 2020 at 03:39:57PM -0600, Jeffrey Hugo wrote:
> On 4/10/2020 2:37 PM, Bhaumik Vasav Bhatt wrote:
> > Hi Jeff,
> >
> > We will always have the mhi_intvec_handler registered and trigger your
> > wake_up state event when you write MHI RESET. BHI INTVEC IRQ using
> > mhi_cntrl->irq[0] is _not_ unregistered once you enter AMSS EE.
>
> I understand it is not unregistered. However mhi_cntrl->irq[0] may be
> reserved for BHI, and thus only exercised by PBL EE. Where as,
> mhi_cntrl->irq[1..N] may be only exercised by AMSS EE. mhi_intvec_handler is
> not called in response to mhi_cntrl->irq[1..N].
>
> Additionally, I re-reviewed the MHI spec, and I don't see where the spec
> requires the device to issue an interrupt upon completion of the RESET
> request.
>
> Under section 3.5, step 11 states -
>
> "The host must poll for the value of the RESET bit to detect the completion
> of the reset procedure by the device (RESET is set to 0)."
>
If this is the scenario then we need to change all of the wait_event_timeout()
implementation for MHI RESET in current stack to polling.
Or the interrupt generation is not defined in spec (sheet) but part of the
existing implementation?
Thanks,
Mani
> So, by this, my proposed solution appears to be spec compliant, where as
> what Hemant proposed is not.
>
> >
> > So, your below assumption is not true:
> > >>>So, if we are in the PBL EE, we would expect to see the BHI
> > interrupt, but if we are in the AMSS EE, we would expect to see a MHI
> > interrupt.
> >
> > At the start of mhi_async_power_up(), you've already registered for the
> > BHI interrupt as we do setup for IRQ and it is only unregistered from
> > power down if power up on the same cycle was a success.
>
> You seem to have misunderstood my point. If the BHI irq is only for BHI
> activity, which is activity restricted to the PBL EE, and the MHI
> interrupt(s) are restricted to MHI activity, which for the purposes of this
> discussion only occur in the AMSS EE, then my assumption is correct. When
> the device is in PBL EE, we should only observe BHI irqs, and when the
> device is in AMSS EE, we should only observe MHI irqs.
>
> This is a statement of what IRQ lines the device is raising, and not a
> statement of what handlers the host has, or has not registered.
>
> Again, if the BHI irq is only generated in the PBL EE, and we rely on the
> BHI irq for "sensing" the state_event - we will never see the state_event in
> the AMSS EE, unless the same IRQ line is used for both MHI and BHI (which is
> only a select set of usecases, and not universal).
>
> >
> > On 4/10/20 8:03 AM, Jeffrey Hugo wrote:
> > > On 4/9/2020 6:55 PM, Hemant Kumar wrote:
> > > >
> > > > On 4/7/20 9:50 AM, Jeffrey Hugo wrote:
> > > > > The MHI device may be in the syserr state when we attempt to init it in
> > > > > power_up().? Since we have no local state, the handling is simple -
> > > > > reset the device and wait for it to transition out of the reset state.
> > > > >
> > > > > Signed-off-by: Jeffrey Hugo <[email protected]>
> > > > > ---
> > > > > ? drivers/bus/mhi/core/pm.c | 20 ++++++++++++++++++++
> > > > > ? 1 file changed, 20 insertions(+)
> > > > >
> > > > > diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> > > > > index 52690cb..3285c9e 100644
> > > > > --- a/drivers/bus/mhi/core/pm.c
> > > > > +++ b/drivers/bus/mhi/core/pm.c
> > > > > @@ -9,6 +9,7 @@
> > > > > ? #include <linux/dma-direction.h>
> > > > > ? #include <linux/dma-mapping.h>
> > > > > ? #include <linux/interrupt.h>
> > > > > +#include <linux/iopoll.h>
> > > > > ? #include <linux/list.h>
> > > > > ? #include <linux/mhi.h>
> > > > > ? #include <linux/module.h>
> > > > > @@ -760,6 +761,7 @@ static void mhi_deassert_dev_wake(struct
> > > > > mhi_controller *mhi_cntrl,
> > > > > ? int mhi_async_power_up(struct mhi_controller *mhi_cntrl)
> > > > > ? {
> > > > > +??? enum mhi_state state;
> > > > > ????? enum mhi_ee_type current_ee;
> > > > > ????? enum dev_st_transition next_state;
> > > > > ????? struct device *dev = &mhi_cntrl->mhi_dev->dev;
> > > > > @@ -829,6 +831,24 @@ int mhi_async_power_up(struct
> > > > > mhi_controller *mhi_cntrl)
> > > > > ????????? goto error_bhi_offset;
> > > > > ????? }
> > > > > +??? state = mhi_get_mhi_state(mhi_cntrl);
> > > > > +??? if (state == MHI_STATE_SYS_ERR) {
> > > > > +??????? mhi_set_mhi_state(mhi_cntrl, MHI_STATE_RESET);
> > > > > +??????? ret = readl_poll_timeout(mhi_cntrl->regs + MHICTRL, val,
> > > > > +???????????????????? !(val & MHICTRL_RESET_MASK), 1000,
> > > > > +???????????????????? mhi_cntrl->timeout_ms * 1000);
> > > > can we use this instead of polling because MSI is configures and
> > > > int_vec handler is registered
> > > >
> > > > ???? wait_event_timeout(mhi_cntrl->state_event,
> > > > ??????????????? MHI_PM_IN_FATAL_STATE(mhi_cntrl->pm_state) ||
> > > > ?????????????? mhi_read_reg_field(mhi_cntrl, base, MHICTRL,
> > > > ???? ??? ??? ??? ??? ????? MHICTRL_RESET_MASK,
> > > > ???? ??? ??? ??? ??? ????? MHICTRL_RESET_SHIFT, &reset) || !reset ,
> > > > ???? ??? ??? ?? msecs_to_jiffies(mhi_cntrl->timeout_ms));
> > > >
> > > > 1) In case of MHI_PM_IN_FATAL_STATE we would not be accessing MHI reg
> > > > 2) Consistent with current MHI driver code.
> > >
> > > I'm not sure this works in the way you intend.
> > >
> > > state_event is linked to the intvec, which is the BHI interrupt. I
> > > don't see that the state_event is triggered in the MHI interrupt
> > > path (mhi_irq_handler).? So, if we are in the PBL EE, we would
> > > expect to see the BHI interrupt, but if we are in the AMSS EE, we
> > > would expect to see a MHI interrupt.
> > >
> > > Now, for my concerned usecase, those two interrupts happen to be the
> > > same interrupt, so both will get triggered, but I don't expect that
> > > to be the same for all usecases.
> > >
> > > So, with the solution I propose, we exit the wait (poll loop) as
> > > soon as we see the register change values.
> > >
> > > With the solution you propose, if we only get the MHI interrupt,
> > > we'll have to wait out the entire timeout value, and then check the
> > > register. In this scenario, we are almost guaranteed to wait for
> > > longer than necessary.
> > >
> > > Did I miss something?
> > >
> > > > > +??????? if (ret) {
> > > > > +??????????? dev_info(dev, "Failed to reset MHI due to
> > > > > syserr state\n");
> > > > > +??????????? goto error_bhi_offset;
> > > > > +??????? }
> > > > > +
> > > > > +??????? /*
> > > > > +???????? * device cleares INTVEC as part of RESET processing,
> > > > > +???????? * re-program it
> > > > > +???????? */
> > > > > +??????? mhi_write_reg(mhi_cntrl, mhi_cntrl->bhi, BHI_INTVEC, 0);
> > > > > +??? }
> > > > > +
> > > > > ????? /* Transition to next state */
> > > > > ????? next_state = MHI_IN_PBL(current_ee) ?
> > > > > ????????? DEV_ST_TRANSITION_PBL : DEV_ST_TRANSITION_READY;
> > > >
> > >
> > >
>
>
> --
> Jeffrey Hugo
> Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.
On 4/13/2020 7:34 AM, Manivannan Sadhasivam wrote:
> On Fri, Apr 10, 2020 at 03:39:57PM -0600, Jeffrey Hugo wrote:
>> On 4/10/2020 2:37 PM, Bhaumik Vasav Bhatt wrote:
>>> Hi Jeff,
>>>
>>> We will always have the mhi_intvec_handler registered and trigger your
>>> wake_up state event when you write MHI RESET. BHI INTVEC IRQ using
>>> mhi_cntrl->irq[0] is _not_ unregistered once you enter AMSS EE.
>>
>> I understand it is not unregistered. However mhi_cntrl->irq[0] may be
>> reserved for BHI, and thus only exercised by PBL EE. Where as,
>> mhi_cntrl->irq[1..N] may be only exercised by AMSS EE. mhi_intvec_handler is
>> not called in response to mhi_cntrl->irq[1..N].
>>
>> Additionally, I re-reviewed the MHI spec, and I don't see where the spec
>> requires the device to issue an interrupt upon completion of the RESET
>> request.
>>
>> Under section 3.5, step 11 states -
>>
>> "The host must poll for the value of the RESET bit to detect the completion
>> of the reset procedure by the device (RESET is set to 0)."
>>
>
> If this is the scenario then we need to change all of the wait_event_timeout()
> implementation for MHI RESET in current stack to polling.
>
> Or the interrupt generation is not defined in spec (sheet) but part of the
> existing implementation?
It probably could be considered part of the existing implementation, but
I'd like to hear from Hemant/Bhaumik. Wherever we end up, I'd like to
have the spec match.
--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
On Mon, Apr 13, 2020 at 08:01:36AM -0600, Jeffrey Hugo wrote:
> On 4/13/2020 7:34 AM, Manivannan Sadhasivam wrote:
> > On Fri, Apr 10, 2020 at 03:39:57PM -0600, Jeffrey Hugo wrote:
> > > On 4/10/2020 2:37 PM, Bhaumik Vasav Bhatt wrote:
> > > > Hi Jeff,
> > > >
> > > > We will always have the mhi_intvec_handler registered and trigger your
> > > > wake_up state event when you write MHI RESET. BHI INTVEC IRQ using
> > > > mhi_cntrl->irq[0] is _not_ unregistered once you enter AMSS EE.
> > >
> > > I understand it is not unregistered. However mhi_cntrl->irq[0] may be
> > > reserved for BHI, and thus only exercised by PBL EE. Where as,
> > > mhi_cntrl->irq[1..N] may be only exercised by AMSS EE. mhi_intvec_handler is
> > > not called in response to mhi_cntrl->irq[1..N].
> > >
> > > Additionally, I re-reviewed the MHI spec, and I don't see where the spec
> > > requires the device to issue an interrupt upon completion of the RESET
> > > request.
> > >
> > > Under section 3.5, step 11 states -
> > >
> > > "The host must poll for the value of the RESET bit to detect the completion
> > > of the reset procedure by the device (RESET is set to 0)."
> > >
> >
> > If this is the scenario then we need to change all of the wait_event_timeout()
> > implementation for MHI RESET in current stack to polling.
> >
> > Or the interrupt generation is not defined in spec (sheet) but part of the
> > existing implementation?
>
> It probably could be considered part of the existing implementation, but I'd
> like to hear from Hemant/Bhaumik. Wherever we end up, I'd like to have the
> spec match.
Hemant/Bhaumik, can you please share your thoughts?
Thanks,
Mani
>
> --
> Jeffrey Hugo
> Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.
On 4/21/2020 12:08 AM, Manivannan Sadhasivam wrote:
> On Mon, Apr 13, 2020 at 08:01:36AM -0600, Jeffrey Hugo wrote:
>> On 4/13/2020 7:34 AM, Manivannan Sadhasivam wrote:
>>> On Fri, Apr 10, 2020 at 03:39:57PM -0600, Jeffrey Hugo wrote:
>>>> On 4/10/2020 2:37 PM, Bhaumik Vasav Bhatt wrote:
>>>>> Hi Jeff,
>>>>>
>>>>> We will always have the mhi_intvec_handler registered and trigger your
>>>>> wake_up state event when you write MHI RESET. BHI INTVEC IRQ using
>>>>> mhi_cntrl->irq[0] is _not_ unregistered once you enter AMSS EE.
>>>>
>>>> I understand it is not unregistered. However mhi_cntrl->irq[0] may be
>>>> reserved for BHI, and thus only exercised by PBL EE. Where as,
>>>> mhi_cntrl->irq[1..N] may be only exercised by AMSS EE. mhi_intvec_handler is
>>>> not called in response to mhi_cntrl->irq[1..N].
>>>>
>>>> Additionally, I re-reviewed the MHI spec, and I don't see where the spec
>>>> requires the device to issue an interrupt upon completion of the RESET
>>>> request.
>>>>
>>>> Under section 3.5, step 11 states -
>>>>
>>>> "The host must poll for the value of the RESET bit to detect the completion
>>>> of the reset procedure by the device (RESET is set to 0)."
>>>>
>>>
>>> If this is the scenario then we need to change all of the wait_event_timeout()
>>> implementation for MHI RESET in current stack to polling.
>>>
>>> Or the interrupt generation is not defined in spec (sheet) but part of the
>>> existing implementation?
>>
>> It probably could be considered part of the existing implementation, but I'd
>> like to hear from Hemant/Bhaumik. Wherever we end up, I'd like to have the
>> spec match.
>
> Hemant/Bhaumik, can you please share your thoughts?
Sorry. Hemant, Bhaumik, and I have had a few calls discussing this. We
are trying to come to a consensus on the expectation of the device
behavior, so that we can propose the best solution. Probably a few more
days yet while we await for a bit of clarification.
--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.