[PATCH v3 0/6] Misc MHI fixes

[PATCH v3 0/6] Misc MHI fixes

[Jeffrey Hugo]

A few (independent) fixes to the MHI bus for issues that I have come across
while developing against the mainline code.

v3:
-reorder series to put changes which are ready, based on reviews, in front
-change error from EIO to ETIMEDOUT when sync_power_up fails
-add change to correct a conflict of channel device names

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 (6):
  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
  bus: mhi: core: Handle syserr during power_up
  bus: mhi: core: Fix channel device name conflict

 drivers/bus/mhi/core/init.c     |  7 +++----
 drivers/bus/mhi/core/internal.h |  3 ---
 drivers/bus/mhi/core/main.c     | 16 ++++------------
 drivers/bus/mhi/core/pm.c       | 26 +++++++++++++++++++++++++-
 include/linux/mhi.h             | 10 +++++++---
 5 files changed, 39 insertions(+), 23 deletions(-)

-- 
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH v3 1/6] bus: mhi: core: Make sure to powerdown if mhi_sync_power_up fails

[Jeffrey Hugo]

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 <jhugo@codeaurora.org>
---
 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 52690cb..dc83d65 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -902,7 +902,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 : -ETIMEDOUT;
+	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.

[PATCH v3 2/6] bus: mhi: core: Remove link_status() callback

[Jeffrey Hugo]

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 <jhugo@codeaurora.org>
Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 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.

[PATCH v3 3/6] bus: mhi: core: Offload register accesses to the controller

[Jeffrey Hugo]

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 appropriate 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 <jhugo@codeaurora.org>
---
 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.

[PATCH v3 4/6] bus: mhi: core: Fix typo in comment

[Jeffrey Hugo]

There is a typo - "runtimet" should be "runtime".  Fix it.

Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
---
 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.

[PATCH v3 5/6] bus: mhi: core: Handle syserr during power_up

[Jeffrey Hugo]

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 <jhugo@codeaurora.org>
---
 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 dc83d65..239619b 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.

[PATCH v3 6/6] bus: mhi: core: Fix channel device name conflict

[Jeffrey Hugo]

When multiple instances of the same MHI product are present in a system,
we can see a splat from mhi_create_devices() - "sysfs: cannot create
duplicate filename".

This is because the device names assigned to the MHI channel devices are
non-unique.  They consist of the channel's name, and the channel's pipe
id.  For identical products, each instance is going to have the same
set of channel (both in name and pipe id).

To fix this, we prepend the device name of the parent device that the
MHI channels belong to.  Since different instances of the same product
should have unique device names, this makes the MHI channel devices for
each product also unique.

Additionally, remove the pipe id from the MHI channel device name.  This
is an internal detail to the MHI product that provides little value, and
imposes too much device specific internal details to userspace.  It is
expected that channel with a specific name (ie "SAHARA") has a specific
client, and it does not matter what pipe id that channel is enumerated on.
The pipe id is an internal detail between the MHI bus, and the hardware.
The client is not expected to make decisions based on the pipe id, and to
do so would require the client to have intimate knowledge of the hardware,
which is inappropiate as it may violate the layering provided by the MHI
bus.  The limitation of doing this is that each product may only have one
instance of a channel by a unique name.  This limitation is appropriate
given the usecases of MHI channels.

Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
---
 drivers/bus/mhi/core/main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 580d72b..0ac0643 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -327,7 +327,8 @@ void mhi_create_devices(struct mhi_controller *mhi_cntrl)
 
 		/* Channel name is same for both UL and DL */
 		mhi_dev->chan_name = mhi_chan->name;
-		dev_set_name(&mhi_dev->dev, "%04x_%s", mhi_chan->chan,
+		dev_set_name(&mhi_dev->dev, "%s_%s",
+			     dev_name(mhi_cntrl->cntrl_dev),
 			     mhi_dev->chan_name);
 
 		/* Init wakeup source if available */
-- 
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v3 1/6] bus: mhi: core: Make sure to powerdown if mhi_sync_power_up fails

[Hemant Kumar]

On 4/27/20 8:59 AM, 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 <jhugo@codeaurora.org>
> ---
Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 2/6] bus: mhi: core: Remove link_status() callback

[Hemant Kumar]

On 4/27/20 8:59 AM, Jeffrey Hugo wrote:
> 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 <jhugo@codeaurora.org>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 3/6] bus: mhi: core: Offload register accesses to the controller

[Hemant Kumar]

On 4/27/20 8:59 AM, Jeffrey Hugo wrote:
> 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 appropriate 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 <jhugo@codeaurora.org>
> ---
Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 4/6] bus: mhi: core: Fix typo in comment

[Hemant Kumar]

On 4/27/20 8:59 AM, Jeffrey Hugo wrote:
> There is a typo - "runtimet" should be "runtime".  Fix it.
> 
> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
> ---
Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 6/6] bus: mhi: core: Fix channel device name conflict

[Hemant Kumar]

On 4/27/20 8:59 AM, Jeffrey Hugo wrote:
> When multiple instances of the same MHI product are present in a system,
> we can see a splat from mhi_create_devices() - "sysfs: cannot create
> duplicate filename".
> 
> This is because the device names assigned to the MHI channel devices are
> non-unique.  They consist of the channel's name, and the channel's pipe
> id.  For identical products, each instance is going to have the same
> set of channel (both in name and pipe id).
> 
> To fix this, we prepend the device name of the parent device that the
> MHI channels belong to.  Since different instances of the same product
> should have unique device names, this makes the MHI channel devices for
> each product also unique.
> 
> Additionally, remove the pipe id from the MHI channel device name.  This
> is an internal detail to the MHI product that provides little value, and
> imposes too much device specific internal details to userspace.  It is
> expected that channel with a specific name (ie "SAHARA") has a specific
> client, and it does not matter what pipe id that channel is enumerated on.
> The pipe id is an internal detail between the MHI bus, and the hardware.
> The client is not expected to make decisions based on the pipe id, and to
> do so would require the client to have intimate knowledge of the hardware,
> which is inappropiate as it may violate the layering provided by the MHI
> bus.  The limitation of doing this is that each product may only have one
> instance of a channel by a unique name.  This limitation is appropriate
> given the usecases of MHI channels.
> 
> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>
> ---
Reviewed-by: Hemant Kumar <hemantk@codeaurora.org>

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 1/6] bus: mhi: core: Make sure to powerdown if mhi_sync_power_up fails

[Manivannan Sadhasivam]

On Mon, Apr 27, 2020 at 09:59:08AM -0600, 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 <jhugo@codeaurora.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  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 52690cb..dc83d65 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -902,7 +902,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 : -ETIMEDOUT;
> +	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.

Re: [PATCH v3 3/6] bus: mhi: core: Offload register accesses to the controller

[Manivannan Sadhasivam]

On Mon, Apr 27, 2020 at 09:59:10AM -0600, Jeffrey Hugo wrote:
> 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 appropriate 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 <jhugo@codeaurora.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

You know how much I like this patch ;)

Thanks,
Mani

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

Re: [PATCH v3 4/6] bus: mhi: core: Fix typo in comment

[Manivannan Sadhasivam]

On Mon, Apr 27, 2020 at 09:59:11AM -0600, Jeffrey Hugo wrote:
> There is a typo - "runtimet" should be "runtime".  Fix it.
> 
> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

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

Re: [PATCH v3 6/6] bus: mhi: core: Fix channel device name conflict

[Manivannan Sadhasivam]

On Mon, Apr 27, 2020 at 09:59:13AM -0600, Jeffrey Hugo wrote:
> When multiple instances of the same MHI product are present in a system,
> we can see a splat from mhi_create_devices() - "sysfs: cannot create
> duplicate filename".
> 
> This is because the device names assigned to the MHI channel devices are
> non-unique.  They consist of the channel's name, and the channel's pipe
> id.  For identical products, each instance is going to have the same
> set of channel (both in name and pipe id).
> 
> To fix this, we prepend the device name of the parent device that the
> MHI channels belong to.  Since different instances of the same product
> should have unique device names, this makes the MHI channel devices for
> each product also unique.
> 
> Additionally, remove the pipe id from the MHI channel device name.  This
> is an internal detail to the MHI product that provides little value, and
> imposes too much device specific internal details to userspace.  It is
> expected that channel with a specific name (ie "SAHARA") has a specific
> client, and it does not matter what pipe id that channel is enumerated on.
> The pipe id is an internal detail between the MHI bus, and the hardware.
> The client is not expected to make decisions based on the pipe id, and to
> do so would require the client to have intimate knowledge of the hardware,
> which is inappropiate as it may violate the layering provided by the MHI
> bus.  The limitation of doing this is that each product may only have one
> instance of a channel by a unique name.  This limitation is appropriate
> given the usecases of MHI channels.
> 
> Signed-off-by: Jeffrey Hugo <jhugo@codeaurora.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  drivers/bus/mhi/core/main.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 580d72b..0ac0643 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -327,7 +327,8 @@ void mhi_create_devices(struct mhi_controller *mhi_cntrl)
>  
>  		/* Channel name is same for both UL and DL */
>  		mhi_dev->chan_name = mhi_chan->name;
> -		dev_set_name(&mhi_dev->dev, "%04x_%s", mhi_chan->chan,
> +		dev_set_name(&mhi_dev->dev, "%s_%s",
> +			     dev_name(mhi_cntrl->cntrl_dev),
>  			     mhi_dev->chan_name);
>  
>  		/* Init wakeup source if available */
> -- 
> Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v3 0/6] Misc MHI fixes

[Manivannan Sadhasivam]

Hi Jeff,

On Mon, Apr 27, 2020 at 09:59:07AM -0600, Jeffrey Hugo wrote:
> A few (independent) fixes to the MHI bus for issues that I have come across
> while developing against the mainline code.
> 
> v3:
> -reorder series to put changes which are ready, based on reviews, in front
> -change error from EIO to ETIMEDOUT when sync_power_up fails
> -add change to correct a conflict of channel device names
> 
> 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 (6):
>   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
>   bus: mhi: core: Handle syserr during power_up
>   bus: mhi: core: Fix channel device name conflict
> 

I'll queue all these patches except [5/6] to mhi-next branch and also send
them along with one of my fix to Greg to be included in upcoming RC.

Thanks,
Mani

>  drivers/bus/mhi/core/init.c     |  7 +++----
>  drivers/bus/mhi/core/internal.h |  3 ---
>  drivers/bus/mhi/core/main.c     | 16 ++++------------
>  drivers/bus/mhi/core/pm.c       | 26 +++++++++++++++++++++++++-
>  include/linux/mhi.h             | 10 +++++++---
>  5 files changed, 39 insertions(+), 23 deletions(-)
> 
> -- 
> Qualcomm Technologies, Inc. is a member of the
> Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v3 0/6] Misc MHI fixes

[Jeffrey Hugo]

On 4/30/2020 11:20 AM, Manivannan Sadhasivam wrote:
> Hi Jeff,
> 
> On Mon, Apr 27, 2020 at 09:59:07AM -0600, Jeffrey Hugo wrote:
>> A few (independent) fixes to the MHI bus for issues that I have come across
>> while developing against the mainline code.
>>
>> v3:
>> -reorder series to put changes which are ready, based on reviews, in front
>> -change error from EIO to ETIMEDOUT when sync_power_up fails
>> -add change to correct a conflict of channel device names
>>
>> 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 (6):
>>    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
>>    bus: mhi: core: Handle syserr during power_up
>>    bus: mhi: core: Fix channel device name conflict
>>
> 
> I'll queue all these patches except [5/6] to mhi-next branch and also send
> them along with one of my fix to Greg to be included in upcoming RC.

Sounds good.  We have a conclusion on the discussions for 5/6, so I'll 
be respinning.  Probably in a week.
> 
> Thanks,
> Mani
> 
>>   drivers/bus/mhi/core/init.c     |  7 +++----
>>   drivers/bus/mhi/core/internal.h |  3 ---
>>   drivers/bus/mhi/core/main.c     | 16 ++++------------
>>   drivers/bus/mhi/core/pm.c       | 26 +++++++++++++++++++++++++-
>>   include/linux/mhi.h             | 10 +++++++---
>>   5 files changed, 39 insertions(+), 23 deletions(-)
>>
>> -- 
>> Qualcomm Technologies, Inc. is a member of the
>> Code Aurora Forum, a Linux Foundation Collaborative Project.


-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.