[PATCH v4 0/9] Introduce features and debugfs/sysfs entries for MHI

[PATCH v4 0/9] Introduce features and debugfs/sysfs entries for MHI

[Bhaumik Bhatt]

Introduce independent bus and device voting mechanism for clients and save
hardware information from BHI.
Allow reading and modifying some MHI variables for debug, test, and
informational purposes using debugfs.
Read values for device specific hardware information to be used by OEMs in
factory testing such as serial number and PK hash using sysfs.

This set of patches was tested on arm64 and x86.

v4:
-Removed bus: mhi: core: Introduce independent voting mechanism patch
-Removed bus vote function from debugfs due to independent voting removal
-Added helper resume APIs to aid consolidation of spread out code
-Added a clean-up patch and a missing host resume in voting API

v3:
-Add patch to check for pending packets in suspend as a dependency for the
independent voting mechanism introduction
-Include register dump entry for debugfs to dump MHI, BHI, and BHIe registers
-Update commit message for the debugfs patch
-Updated Documentation/ABI with the required info for sysfs
-Updated debugfs patch to include a new KConfig entry and dependencies
-Updated reviewed-by for some patches

v2:
-Added a new debugfs.c file for specific debugfs entries and code
-Updated commit text and addressed some comments for voting change
-Made sure sysfs is only used for serial number and OEM PK hash usage

Bhaumik Bhatt (9):
  bus: mhi: core: Remove double occurrence for mhi_ctrl_ev_task()
    declaration
  bus: mhi: core: Abort suspends due to outgoing pending packets
  bus: mhi: core: Use helper API to trigger a non-blocking host resume
  bus: mhi: core: Trigger a host resume when device vote is requested
  bus: mhi: core: Use generic name field for an MHI device
  bus: mhi: core: Introduce helper function to check device state
  bus: mhi: core: Introduce debugfs entries and counters for MHI
  bus: mhi: core: Read and save device hardware information from BHI
  bus: mhi: core: Introduce sysfs entries for MHI

 Documentation/ABI/stable/sysfs-bus-mhi |  25 ++
 MAINTAINERS                            |   1 +
 drivers/bus/mhi/Kconfig                |   8 +
 drivers/bus/mhi/core/Makefile          |   5 +-
 drivers/bus/mhi/core/boot.c            |  17 +-
 drivers/bus/mhi/core/debugfs.c         | 444 +++++++++++++++++++++++++++++++++
 drivers/bus/mhi/core/init.c            |  65 ++++-
 drivers/bus/mhi/core/internal.h        |  38 ++-
 drivers/bus/mhi/core/main.c            |  27 +-
 drivers/bus/mhi/core/pm.c              |  19 +-
 include/linux/mhi.h                    |  18 +-
 11 files changed, 633 insertions(+), 34 deletions(-)
 create mode 100644 Documentation/ABI/stable/sysfs-bus-mhi
 create mode 100644 drivers/bus/mhi/core/debugfs.c

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

[PATCH v4 1/9] bus: mhi: core: Remove double occurrence for mhi_ctrl_ev_task() declaration

[Bhaumik Bhatt]

mhi_ctrl_ev_task() in the internal header file occurred twice.
Remove one of the occurrences for clean-up.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/core/internal.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
index b1f640b..bcfa7b6 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -592,7 +592,6 @@ int mhi_queue_state_transition(struct mhi_controller *mhi_cntrl,
 void mhi_pm_sys_err_handler(struct mhi_controller *mhi_cntrl);
 void mhi_fw_load_worker(struct work_struct *work);
 int mhi_ready_state_transition(struct mhi_controller *mhi_cntrl);
-void mhi_ctrl_ev_task(unsigned long data);
 int mhi_pm_m0_transition(struct mhi_controller *mhi_cntrl);
 void mhi_pm_m1_transition(struct mhi_controller *mhi_cntrl);
 int mhi_pm_m3_transition(struct mhi_controller *mhi_cntrl);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 1/9] bus: mhi: core: Remove double occurrence for mhi_ctrl_ev_task() declaration

[Manivannan Sadhasivam]

On Mon, Jun 29, 2020 at 09:39:34AM -0700, Bhaumik Bhatt wrote:
> mhi_ctrl_ev_task() in the internal header file occurred twice.
> Remove one of the occurrences for clean-up.
> 
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>

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

Thanks,
Mani

> ---
>  drivers/bus/mhi/core/internal.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
> index b1f640b..bcfa7b6 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -592,7 +592,6 @@ int mhi_queue_state_transition(struct mhi_controller *mhi_cntrl,
>  void mhi_pm_sys_err_handler(struct mhi_controller *mhi_cntrl);
>  void mhi_fw_load_worker(struct work_struct *work);
>  int mhi_ready_state_transition(struct mhi_controller *mhi_cntrl);
> -void mhi_ctrl_ev_task(unsigned long data);
>  int mhi_pm_m0_transition(struct mhi_controller *mhi_cntrl);
>  void mhi_pm_m1_transition(struct mhi_controller *mhi_cntrl);
>  int mhi_pm_m3_transition(struct mhi_controller *mhi_cntrl);
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

[PATCH v4 2/9] bus: mhi: core: Abort suspends due to outgoing pending packets

[Bhaumik Bhatt]

Add the missing check to abort suspends if a client has pending outgoing
packets to send to the device. This allows better utilization of the MHI
bus wherein clients on the host are not left waiting for longer suspend
or resume cycles to finish for data transfers.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/core/pm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 7960980..661d704 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -686,7 +686,8 @@ int mhi_pm_suspend(struct mhi_controller *mhi_cntrl)
 		return -EIO;
 
 	/* Return busy if there are any pending resources */
-	if (atomic_read(&mhi_cntrl->dev_wake))
+	if (atomic_read(&mhi_cntrl->dev_wake) ||
+	    atomic_read(&mhi_cntrl->pending_pkts))
 		return -EBUSY;
 
 	/* Take MHI out of M2 state */
@@ -712,7 +713,8 @@ int mhi_pm_suspend(struct mhi_controller *mhi_cntrl)
 
 	write_lock_irq(&mhi_cntrl->pm_lock);
 
-	if (atomic_read(&mhi_cntrl->dev_wake)) {
+	if (atomic_read(&mhi_cntrl->dev_wake) ||
+	    atomic_read(&mhi_cntrl->pending_pkts)) {
 		write_unlock_irq(&mhi_cntrl->pm_lock);
 		return -EBUSY;
 	}
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 2/9] bus: mhi: core: Abort suspends due to outgoing pending packets

[Manivannan Sadhasivam]

On Mon, Jun 29, 2020 at 09:39:35AM -0700, Bhaumik Bhatt wrote:
> Add the missing check to abort suspends if a client has pending outgoing
> packets to send to the device. This allows better utilization of the MHI
> bus wherein clients on the host are not left waiting for longer suspend
> or resume cycles to finish for data transfers.
> 

Just one nitpick: Please always use the terms 'client drivers' referring to the
MHI client drivers in the kernel and 'client devices' referring to the physical
MHI client devices. The term 'client' creates ambiguity.

> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>

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

Thanks,
Mani

> ---
>  drivers/bus/mhi/core/pm.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index 7960980..661d704 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -686,7 +686,8 @@ int mhi_pm_suspend(struct mhi_controller *mhi_cntrl)
>  		return -EIO;
>  
>  	/* Return busy if there are any pending resources */
> -	if (atomic_read(&mhi_cntrl->dev_wake))
> +	if (atomic_read(&mhi_cntrl->dev_wake) ||
> +	    atomic_read(&mhi_cntrl->pending_pkts))
>  		return -EBUSY;
>  
>  	/* Take MHI out of M2 state */
> @@ -712,7 +713,8 @@ int mhi_pm_suspend(struct mhi_controller *mhi_cntrl)
>  
>  	write_lock_irq(&mhi_cntrl->pm_lock);
>  
> -	if (atomic_read(&mhi_cntrl->dev_wake)) {
> +	if (atomic_read(&mhi_cntrl->dev_wake) ||
> +	    atomic_read(&mhi_cntrl->pending_pkts)) {
>  		write_unlock_irq(&mhi_cntrl->pm_lock);
>  		return -EBUSY;
>  	}
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

[PATCH v4 3/9] bus: mhi: core: Use helper API to trigger a non-blocking host resume

[Bhaumik Bhatt]

Autonomous low power mode support requires the MHI host to resume from
multiple places and post a wakeup source to exit system suspend. This
needs to be done in a non-blocking manner. Introduce a helper API to
trigger the host resume for data transfers and other non-blocking use
cases while supporting implementation of autonomous low power modes.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/core/internal.h |  8 ++++++++
 drivers/bus/mhi/core/main.c     | 21 +++++++--------------
 drivers/bus/mhi/core/pm.c       |  6 ++----
 3 files changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
index bcfa7b6..cb32eaf 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -599,6 +599,14 @@ int mhi_queue_state_transition(struct mhi_controller *mhi_cntrl,
 int mhi_send_cmd(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan,
 		 enum mhi_cmd_type cmd);
 
+static inline void mhi_trigger_resume(struct mhi_controller *mhi_cntrl,
+				      bool hard_wakeup)
+{
+	pm_wakeup_dev_event(&mhi_cntrl->mhi_dev->dev, 0, hard_wakeup);
+	mhi_cntrl->runtime_get(mhi_cntrl);
+	mhi_cntrl->runtime_put(mhi_cntrl);
+}
+
 /* Register access methods */
 void mhi_db_brstmode(struct mhi_controller *mhi_cntrl, struct db_cfg *db_cfg,
 		     void __iomem *db_addr, dma_addr_t db_val);
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 1f622ce..8d6ec34 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -909,8 +909,7 @@ void mhi_ctrl_ev_task(unsigned long data)
 		 * process it since we are probably in a suspended state,
 		 * so trigger a resume.
 		 */
-		mhi_cntrl->runtime_get(mhi_cntrl);
-		mhi_cntrl->runtime_put(mhi_cntrl);
+		mhi_trigger_resume(mhi_cntrl, false);
 
 		return;
 	}
@@ -971,10 +970,8 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir,
 	}
 
 	/* we're in M3 or transitioning to M3 */
-	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
-		mhi_cntrl->runtime_get(mhi_cntrl);
-		mhi_cntrl->runtime_put(mhi_cntrl);
-	}
+	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
+		mhi_trigger_resume(mhi_cntrl, false);
 
 	/* Toggle wake to exit out of M2 */
 	mhi_cntrl->wake_toggle(mhi_cntrl);
@@ -1032,10 +1029,8 @@ int mhi_queue_dma(struct mhi_device *mhi_dev, enum dma_data_direction dir,
 	}
 
 	/* we're in M3 or transitioning to M3 */
-	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
-		mhi_cntrl->runtime_get(mhi_cntrl);
-		mhi_cntrl->runtime_put(mhi_cntrl);
-	}
+	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
+		mhi_trigger_resume(mhi_cntrl, false);
 
 	/* Toggle wake to exit out of M2 */
 	mhi_cntrl->wake_toggle(mhi_cntrl);
@@ -1147,10 +1142,8 @@ int mhi_queue_buf(struct mhi_device *mhi_dev, enum dma_data_direction dir,
 	read_lock_irqsave(&mhi_cntrl->pm_lock, flags);
 
 	/* we're in M3 or transitioning to M3 */
-	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
-		mhi_cntrl->runtime_get(mhi_cntrl);
-		mhi_cntrl->runtime_put(mhi_cntrl);
-	}
+	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
+		mhi_trigger_resume(mhi_cntrl, false);
 
 	/* Toggle wake to exit out of M2 */
 	mhi_cntrl->wake_toggle(mhi_cntrl);
diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 661d704..5e3994e 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -1139,10 +1139,8 @@ void mhi_device_put(struct mhi_device *mhi_dev)
 
 	mhi_dev->dev_wake--;
 	read_lock_bh(&mhi_cntrl->pm_lock);
-	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
-		mhi_cntrl->runtime_get(mhi_cntrl);
-		mhi_cntrl->runtime_put(mhi_cntrl);
-	}
+	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
+		mhi_trigger_resume(mhi_cntrl, false);
 
 	mhi_cntrl->wake_put(mhi_cntrl, false);
 	read_unlock_bh(&mhi_cntrl->pm_lock);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 3/9] bus: mhi: core: Use helper API to trigger a non-blocking host resume

[Manivannan Sadhasivam]

On Mon, Jun 29, 2020 at 09:39:36AM -0700, Bhaumik Bhatt wrote:
> Autonomous low power mode support requires the MHI host to resume from
> multiple places and post a wakeup source to exit system suspend. This
> needs to be done in a non-blocking manner. Introduce a helper API to
> trigger the host resume for data transfers and other non-blocking use
> cases while supporting implementation of autonomous low power modes.
> 

Why can't you use pm_wakeup_event() as done in __mhi_device_get_sync()?

Thanks,
Mani

> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>  drivers/bus/mhi/core/internal.h |  8 ++++++++
>  drivers/bus/mhi/core/main.c     | 21 +++++++--------------
>  drivers/bus/mhi/core/pm.c       |  6 ++----
>  3 files changed, 17 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
> index bcfa7b6..cb32eaf 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -599,6 +599,14 @@ int mhi_queue_state_transition(struct mhi_controller *mhi_cntrl,
>  int mhi_send_cmd(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan,
>  		 enum mhi_cmd_type cmd);
>  
> +static inline void mhi_trigger_resume(struct mhi_controller *mhi_cntrl,
> +				      bool hard_wakeup)
> +{
> +	pm_wakeup_dev_event(&mhi_cntrl->mhi_dev->dev, 0, hard_wakeup);
> +	mhi_cntrl->runtime_get(mhi_cntrl);
> +	mhi_cntrl->runtime_put(mhi_cntrl);
> +}
> +
>  /* Register access methods */
>  void mhi_db_brstmode(struct mhi_controller *mhi_cntrl, struct db_cfg *db_cfg,
>  		     void __iomem *db_addr, dma_addr_t db_val);
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 1f622ce..8d6ec34 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -909,8 +909,7 @@ void mhi_ctrl_ev_task(unsigned long data)
>  		 * process it since we are probably in a suspended state,
>  		 * so trigger a resume.
>  		 */
> -		mhi_cntrl->runtime_get(mhi_cntrl);
> -		mhi_cntrl->runtime_put(mhi_cntrl);
> +		mhi_trigger_resume(mhi_cntrl, false);
>  
>  		return;
>  	}
> @@ -971,10 +970,8 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir,
>  	}
>  
>  	/* we're in M3 or transitioning to M3 */
> -	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
> -		mhi_cntrl->runtime_get(mhi_cntrl);
> -		mhi_cntrl->runtime_put(mhi_cntrl);
> -	}
> +	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
> +		mhi_trigger_resume(mhi_cntrl, false);
>  
>  	/* Toggle wake to exit out of M2 */
>  	mhi_cntrl->wake_toggle(mhi_cntrl);
> @@ -1032,10 +1029,8 @@ int mhi_queue_dma(struct mhi_device *mhi_dev, enum dma_data_direction dir,
>  	}
>  
>  	/* we're in M3 or transitioning to M3 */
> -	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
> -		mhi_cntrl->runtime_get(mhi_cntrl);
> -		mhi_cntrl->runtime_put(mhi_cntrl);
> -	}
> +	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
> +		mhi_trigger_resume(mhi_cntrl, false);
>  
>  	/* Toggle wake to exit out of M2 */
>  	mhi_cntrl->wake_toggle(mhi_cntrl);
> @@ -1147,10 +1142,8 @@ int mhi_queue_buf(struct mhi_device *mhi_dev, enum dma_data_direction dir,
>  	read_lock_irqsave(&mhi_cntrl->pm_lock, flags);
>  
>  	/* we're in M3 or transitioning to M3 */
> -	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
> -		mhi_cntrl->runtime_get(mhi_cntrl);
> -		mhi_cntrl->runtime_put(mhi_cntrl);
> -	}
> +	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
> +		mhi_trigger_resume(mhi_cntrl, false);
>  
>  	/* Toggle wake to exit out of M2 */
>  	mhi_cntrl->wake_toggle(mhi_cntrl);
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index 661d704..5e3994e 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -1139,10 +1139,8 @@ void mhi_device_put(struct mhi_device *mhi_dev)
>  
>  	mhi_dev->dev_wake--;
>  	read_lock_bh(&mhi_cntrl->pm_lock);
> -	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
> -		mhi_cntrl->runtime_get(mhi_cntrl);
> -		mhi_cntrl->runtime_put(mhi_cntrl);
> -	}
> +	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
> +		mhi_trigger_resume(mhi_cntrl, false);
>  
>  	mhi_cntrl->wake_put(mhi_cntrl, false);
>  	read_unlock_bh(&mhi_cntrl->pm_lock);
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v4 3/9] bus: mhi: core: Use helper API to trigger a non-blocking host resume

[bbhatt]

On 2020-07-04 07:47, Manivannan Sadhasivam wrote:
> On Mon, Jun 29, 2020 at 09:39:36AM -0700, Bhaumik Bhatt wrote:
>> Autonomous low power mode support requires the MHI host to resume from
>> multiple places and post a wakeup source to exit system suspend. This
>> needs to be done in a non-blocking manner. Introduce a helper API to
>> trigger the host resume for data transfers and other non-blocking use
>> cases while supporting implementation of autonomous low power modes.
>> 
> 
> Why can't you use pm_wakeup_event() as done in __mhi_device_get_sync()?
> 
> Thanks,
> Mani
> 

I forgot to address the __mhi_device_get_sync() function. Thanks for 
pointing out.

Is it preferable to always post wakeup source with hard boolean set?
We do want to wakeup from Suspend-to-Idle if system suspend happens to 
go that route.

As of now, we just by default do regular wakeup event and not hard.
I figured at some point we might need to distinguish between hard vs 
regular, hence the option but
it can be eliminated in favor of one or another.

Thanks,
Bhaumik

>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
>> ---
>>  drivers/bus/mhi/core/internal.h |  8 ++++++++
>>  drivers/bus/mhi/core/main.c     | 21 +++++++--------------
>>  drivers/bus/mhi/core/pm.c       |  6 ++----
>>  3 files changed, 17 insertions(+), 18 deletions(-)
>> 
>> diff --git a/drivers/bus/mhi/core/internal.h 
>> b/drivers/bus/mhi/core/internal.h
>> index bcfa7b6..cb32eaf 100644
>> --- a/drivers/bus/mhi/core/internal.h
>> +++ b/drivers/bus/mhi/core/internal.h
>> @@ -599,6 +599,14 @@ int mhi_queue_state_transition(struct 
>> mhi_controller *mhi_cntrl,
>>  int mhi_send_cmd(struct mhi_controller *mhi_cntrl, struct mhi_chan 
>> *mhi_chan,
>>  		 enum mhi_cmd_type cmd);
>> 
>> +static inline void mhi_trigger_resume(struct mhi_controller 
>> *mhi_cntrl,
>> +				      bool hard_wakeup)
>> +{
>> +	pm_wakeup_dev_event(&mhi_cntrl->mhi_dev->dev, 0, hard_wakeup);
>> +	mhi_cntrl->runtime_get(mhi_cntrl);
>> +	mhi_cntrl->runtime_put(mhi_cntrl);
>> +}
>> +
>>  /* Register access methods */
>>  void mhi_db_brstmode(struct mhi_controller *mhi_cntrl, struct db_cfg 
>> *db_cfg,
>>  		     void __iomem *db_addr, dma_addr_t db_val);
>> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
>> index 1f622ce..8d6ec34 100644
>> --- a/drivers/bus/mhi/core/main.c
>> +++ b/drivers/bus/mhi/core/main.c
>> @@ -909,8 +909,7 @@ void mhi_ctrl_ev_task(unsigned long data)
>>  		 * process it since we are probably in a suspended state,
>>  		 * so trigger a resume.
>>  		 */
>> -		mhi_cntrl->runtime_get(mhi_cntrl);
>> -		mhi_cntrl->runtime_put(mhi_cntrl);
>> +		mhi_trigger_resume(mhi_cntrl, false);
>> 
>>  		return;
>>  	}
>> @@ -971,10 +970,8 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, 
>> enum dma_data_direction dir,
>>  	}
>> 
>>  	/* we're in M3 or transitioning to M3 */
>> -	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
>> -		mhi_cntrl->runtime_get(mhi_cntrl);
>> -		mhi_cntrl->runtime_put(mhi_cntrl);
>> -	}
>> +	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
>> +		mhi_trigger_resume(mhi_cntrl, false);
>> 
>>  	/* Toggle wake to exit out of M2 */
>>  	mhi_cntrl->wake_toggle(mhi_cntrl);
>> @@ -1032,10 +1029,8 @@ int mhi_queue_dma(struct mhi_device *mhi_dev, 
>> enum dma_data_direction dir,
>>  	}
>> 
>>  	/* we're in M3 or transitioning to M3 */
>> -	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
>> -		mhi_cntrl->runtime_get(mhi_cntrl);
>> -		mhi_cntrl->runtime_put(mhi_cntrl);
>> -	}
>> +	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
>> +		mhi_trigger_resume(mhi_cntrl, false);
>> 
>>  	/* Toggle wake to exit out of M2 */
>>  	mhi_cntrl->wake_toggle(mhi_cntrl);
>> @@ -1147,10 +1142,8 @@ int mhi_queue_buf(struct mhi_device *mhi_dev, 
>> enum dma_data_direction dir,
>>  	read_lock_irqsave(&mhi_cntrl->pm_lock, flags);
>> 
>>  	/* we're in M3 or transitioning to M3 */
>> -	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
>> -		mhi_cntrl->runtime_get(mhi_cntrl);
>> -		mhi_cntrl->runtime_put(mhi_cntrl);
>> -	}
>> +	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
>> +		mhi_trigger_resume(mhi_cntrl, false);
>> 
>>  	/* Toggle wake to exit out of M2 */
>>  	mhi_cntrl->wake_toggle(mhi_cntrl);
>> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
>> index 661d704..5e3994e 100644
>> --- a/drivers/bus/mhi/core/pm.c
>> +++ b/drivers/bus/mhi/core/pm.c
>> @@ -1139,10 +1139,8 @@ void mhi_device_put(struct mhi_device *mhi_dev)
>> 
>>  	mhi_dev->dev_wake--;
>>  	read_lock_bh(&mhi_cntrl->pm_lock);
>> -	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
>> -		mhi_cntrl->runtime_get(mhi_cntrl);
>> -		mhi_cntrl->runtime_put(mhi_cntrl);
>> -	}
>> +	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
>> +		mhi_trigger_resume(mhi_cntrl, false);
>> 
>>  	mhi_cntrl->wake_put(mhi_cntrl, false);
>>  	read_unlock_bh(&mhi_cntrl->pm_lock);
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 

Re: [PATCH v4 3/9] bus: mhi: core: Use helper API to trigger a non-blocking host resume

[Manivannan Sadhasivam]

On Wed, Jul 08, 2020 at 01:53:49PM -0700, bbhatt@codeaurora.org wrote:
> On 2020-07-04 07:47, Manivannan Sadhasivam wrote:
> > On Mon, Jun 29, 2020 at 09:39:36AM -0700, Bhaumik Bhatt wrote:
> > > Autonomous low power mode support requires the MHI host to resume from
> > > multiple places and post a wakeup source to exit system suspend. This
> > > needs to be done in a non-blocking manner. Introduce a helper API to
> > > trigger the host resume for data transfers and other non-blocking use
> > > cases while supporting implementation of autonomous low power modes.
> > > 
> > 
> > Why can't you use pm_wakeup_event() as done in __mhi_device_get_sync()?
> > 
> > Thanks,
> > Mani
> > 
> 
> I forgot to address the __mhi_device_get_sync() function. Thanks for
> pointing out.
> 
> Is it preferable to always post wakeup source with hard boolean set?

A quick grep shows that this routine is not used extensively as compared to
the other one and hence the question.

For a bus driver like this I think we can just live without the hard wakeup.
I don't see a specific usecase where we would need a hard wakeup from suspend.

Thanks,
Mani

> We do want to wakeup from Suspend-to-Idle if system suspend happens to go
> that route.
> 
> As of now, we just by default do regular wakeup event and not hard.
> I figured at some point we might need to distinguish between hard vs
> regular, hence the option but
> it can be eliminated in favor of one or another.
> 



> Thanks,
> Bhaumik
> 
> > > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > > ---
> > >  drivers/bus/mhi/core/internal.h |  8 ++++++++
> > >  drivers/bus/mhi/core/main.c     | 21 +++++++--------------
> > >  drivers/bus/mhi/core/pm.c       |  6 ++----
> > >  3 files changed, 17 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/bus/mhi/core/internal.h
> > > b/drivers/bus/mhi/core/internal.h
> > > index bcfa7b6..cb32eaf 100644
> > > --- a/drivers/bus/mhi/core/internal.h
> > > +++ b/drivers/bus/mhi/core/internal.h
> > > @@ -599,6 +599,14 @@ int mhi_queue_state_transition(struct
> > > mhi_controller *mhi_cntrl,
> > >  int mhi_send_cmd(struct mhi_controller *mhi_cntrl, struct mhi_chan
> > > *mhi_chan,
> > >  		 enum mhi_cmd_type cmd);
> > > 
> > > +static inline void mhi_trigger_resume(struct mhi_controller
> > > *mhi_cntrl,
> > > +				      bool hard_wakeup)
> > > +{
> > > +	pm_wakeup_dev_event(&mhi_cntrl->mhi_dev->dev, 0, hard_wakeup);
> > > +	mhi_cntrl->runtime_get(mhi_cntrl);
> > > +	mhi_cntrl->runtime_put(mhi_cntrl);
> > > +}
> > > +
> > >  /* Register access methods */
> > >  void mhi_db_brstmode(struct mhi_controller *mhi_cntrl, struct
> > > db_cfg *db_cfg,
> > >  		     void __iomem *db_addr, dma_addr_t db_val);
> > > diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> > > index 1f622ce..8d6ec34 100644
> > > --- a/drivers/bus/mhi/core/main.c
> > > +++ b/drivers/bus/mhi/core/main.c
> > > @@ -909,8 +909,7 @@ void mhi_ctrl_ev_task(unsigned long data)
> > >  		 * process it since we are probably in a suspended state,
> > >  		 * so trigger a resume.
> > >  		 */
> > > -		mhi_cntrl->runtime_get(mhi_cntrl);
> > > -		mhi_cntrl->runtime_put(mhi_cntrl);
> > > +		mhi_trigger_resume(mhi_cntrl, false);
> > > 
> > >  		return;
> > >  	}
> > > @@ -971,10 +970,8 @@ int mhi_queue_skb(struct mhi_device *mhi_dev,
> > > enum dma_data_direction dir,
> > >  	}
> > > 
> > >  	/* we're in M3 or transitioning to M3 */
> > > -	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
> > > -		mhi_cntrl->runtime_get(mhi_cntrl);
> > > -		mhi_cntrl->runtime_put(mhi_cntrl);
> > > -	}
> > > +	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
> > > +		mhi_trigger_resume(mhi_cntrl, false);
> > > 
> > >  	/* Toggle wake to exit out of M2 */
> > >  	mhi_cntrl->wake_toggle(mhi_cntrl);
> > > @@ -1032,10 +1029,8 @@ int mhi_queue_dma(struct mhi_device *mhi_dev,
> > > enum dma_data_direction dir,
> > >  	}
> > > 
> > >  	/* we're in M3 or transitioning to M3 */
> > > -	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
> > > -		mhi_cntrl->runtime_get(mhi_cntrl);
> > > -		mhi_cntrl->runtime_put(mhi_cntrl);
> > > -	}
> > > +	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
> > > +		mhi_trigger_resume(mhi_cntrl, false);
> > > 
> > >  	/* Toggle wake to exit out of M2 */
> > >  	mhi_cntrl->wake_toggle(mhi_cntrl);
> > > @@ -1147,10 +1142,8 @@ int mhi_queue_buf(struct mhi_device *mhi_dev,
> > > enum dma_data_direction dir,
> > >  	read_lock_irqsave(&mhi_cntrl->pm_lock, flags);
> > > 
> > >  	/* we're in M3 or transitioning to M3 */
> > > -	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
> > > -		mhi_cntrl->runtime_get(mhi_cntrl);
> > > -		mhi_cntrl->runtime_put(mhi_cntrl);
> > > -	}
> > > +	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
> > > +		mhi_trigger_resume(mhi_cntrl, false);
> > > 
> > >  	/* Toggle wake to exit out of M2 */
> > >  	mhi_cntrl->wake_toggle(mhi_cntrl);
> > > diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> > > index 661d704..5e3994e 100644
> > > --- a/drivers/bus/mhi/core/pm.c
> > > +++ b/drivers/bus/mhi/core/pm.c
> > > @@ -1139,10 +1139,8 @@ void mhi_device_put(struct mhi_device *mhi_dev)
> > > 
> > >  	mhi_dev->dev_wake--;
> > >  	read_lock_bh(&mhi_cntrl->pm_lock);
> > > -	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state)) {
> > > -		mhi_cntrl->runtime_get(mhi_cntrl);
> > > -		mhi_cntrl->runtime_put(mhi_cntrl);
> > > -	}
> > > +	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
> > > +		mhi_trigger_resume(mhi_cntrl, false);
> > > 
> > >  	mhi_cntrl->wake_put(mhi_cntrl, false);
> > >  	read_unlock_bh(&mhi_cntrl->pm_lock);
> > > --
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> > > Forum,
> > > a Linux Foundation Collaborative Project
> > > 

[PATCH v4 4/9] bus: mhi: core: Trigger a host resume when device vote is requested

[Bhaumik Bhatt]

It is possible that the host may be suspending or suspended and may
not allow an outgoing device wake assert immediately if a client has
requested for it. Ensure that the host wakes up and allows for it so
the client does not have to wait for an external trigger or an
outgoing packet to be queued for the host resume to occur.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/core/pm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 5e3994e..74c5cb1 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -1115,6 +1115,9 @@ void mhi_device_get(struct mhi_device *mhi_dev)
 
 	mhi_dev->dev_wake++;
 	read_lock_bh(&mhi_cntrl->pm_lock);
+	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
+		mhi_trigger_resume(mhi_cntrl, false);
+
 	mhi_cntrl->wake_get(mhi_cntrl, true);
 	read_unlock_bh(&mhi_cntrl->pm_lock);
 }
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 4/9] bus: mhi: core: Trigger a host resume when device vote is requested

[Manivannan Sadhasivam]

On Mon, Jun 29, 2020 at 09:39:37AM -0700, Bhaumik Bhatt wrote:
> It is possible that the host may be suspending or suspended and may
> not allow an outgoing device wake assert immediately if a client has
> requested for it. Ensure that the host wakes up and allows for it so
> the client does not have to wait for an external trigger or an
> outgoing packet to be queued for the host resume to occur.
> 
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>

Remove the term 'vote' from the commit subject, it doesn't seem right. How about
"Trigger host resume if already suspended during mhi_device_get()"? With that
fixed,

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

Thanks,
Mani

> ---
>  drivers/bus/mhi/core/pm.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index 5e3994e..74c5cb1 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -1115,6 +1115,9 @@ void mhi_device_get(struct mhi_device *mhi_dev)
>  
>  	mhi_dev->dev_wake++;
>  	read_lock_bh(&mhi_cntrl->pm_lock);
> +	if (MHI_PM_IN_SUSPEND_STATE(mhi_cntrl->pm_state))
> +		mhi_trigger_resume(mhi_cntrl, false);
> +
>  	mhi_cntrl->wake_get(mhi_cntrl, true);
>  	read_unlock_bh(&mhi_cntrl->pm_lock);
>  }
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

[PATCH v4 5/9] bus: mhi: core: Use generic name field for an MHI device

[Bhaumik Bhatt]

An MHI device is not necessarily associated with only channels as we can
have one associated with the controller itself. Hence, the chan_name
field within the mhi_device structure should instead be replaced with a
generic name to accurately reflect any type of MHI device.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
---
 drivers/bus/mhi/core/init.c | 5 +++--
 drivers/bus/mhi/core/main.c | 6 +++---
 include/linux/mhi.h         | 8 ++++----
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index e43a190..e2011ec 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -904,6 +904,7 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
 	mhi_dev->dev_type = MHI_DEVICE_CONTROLLER;
 	mhi_dev->mhi_cntrl = mhi_cntrl;
 	dev_set_name(&mhi_dev->dev, "%s", dev_name(mhi_cntrl->cntrl_dev));
+	mhi_dev->name = dev_name(mhi_cntrl->cntrl_dev);
 
 	/* Init wakeup source */
 	device_init_wakeup(&mhi_dev->dev, true);
@@ -1249,7 +1250,7 @@ static int mhi_uevent(struct device *dev, struct kobj_uevent_env *env)
 	struct mhi_device *mhi_dev = to_mhi_device(dev);
 
 	return add_uevent_var(env, "MODALIAS=" MHI_DEVICE_MODALIAS_FMT,
-					mhi_dev->chan_name);
+					mhi_dev->name);
 }
 
 static int mhi_match(struct device *dev, struct device_driver *drv)
@@ -1266,7 +1267,7 @@ static int mhi_match(struct device *dev, struct device_driver *drv)
 		return 0;
 
 	for (id = mhi_drv->id_table; id->chan[0]; id++)
-		if (!strcmp(mhi_dev->chan_name, id->chan)) {
+		if (!strcmp(mhi_dev->name, id->chan)) {
 			mhi_dev->id = id;
 			return 1;
 		}
diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
index 8d6ec34..3af0ce6 100644
--- a/drivers/bus/mhi/core/main.c
+++ b/drivers/bus/mhi/core/main.c
@@ -249,7 +249,7 @@ int mhi_destroy_device(struct device *dev, void *data)
 		put_device(&mhi_dev->dl_chan->mhi_dev->dev);
 
 	dev_dbg(&mhi_cntrl->mhi_dev->dev, "destroy device for chan:%s\n",
-		 mhi_dev->chan_name);
+		 mhi_dev->name);
 
 	/* Notify the client and remove the device from MHI bus */
 	device_del(dev);
@@ -327,10 +327,10 @@ 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;
+		mhi_dev->name = mhi_chan->name;
 		dev_set_name(&mhi_dev->dev, "%s_%s",
 			     dev_name(mhi_cntrl->cntrl_dev),
-			     mhi_dev->chan_name);
+			     mhi_dev->name);
 
 		/* Init wakeup source if available */
 		if (mhi_dev->dl_chan && mhi_dev->dl_chan->wake_capable)
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index b008914..7ed785e 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -436,10 +436,10 @@ struct mhi_controller {
 };
 
 /**
- * struct mhi_device - Structure representing a MHI device which binds
- *                     to channels
+ * struct mhi_device - Structure representing an MHI device which binds
+ *                     to channels or is associated with controllers
  * @id: Pointer to MHI device ID struct
- * @chan_name: Name of the channel to which the device binds
+ * @name: Name of the associated MHI device
  * @mhi_cntrl: Controller the device belongs to
  * @ul_chan: UL channel for the device
  * @dl_chan: DL channel for the device
@@ -451,7 +451,7 @@ struct mhi_controller {
  */
 struct mhi_device {
 	const struct mhi_device_id *id;
-	const char *chan_name;
+	const char *name;
 	struct mhi_controller *mhi_cntrl;
 	struct mhi_chan *ul_chan;
 	struct mhi_chan *dl_chan;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 5/9] bus: mhi: core: Use generic name field for an MHI device

[Manivannan Sadhasivam]

On Mon, Jun 29, 2020 at 09:39:38AM -0700, Bhaumik Bhatt wrote:
> An MHI device is not necessarily associated with only channels as we can
> have one associated with the controller itself. Hence, the chan_name
> field within the mhi_device structure should instead be replaced with a
> generic name to accurately reflect any type of MHI device.
> 
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>

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

Thanks,
Mani

> ---
>  drivers/bus/mhi/core/init.c | 5 +++--
>  drivers/bus/mhi/core/main.c | 6 +++---
>  include/linux/mhi.h         | 8 ++++----
>  3 files changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index e43a190..e2011ec 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -904,6 +904,7 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
>  	mhi_dev->dev_type = MHI_DEVICE_CONTROLLER;
>  	mhi_dev->mhi_cntrl = mhi_cntrl;
>  	dev_set_name(&mhi_dev->dev, "%s", dev_name(mhi_cntrl->cntrl_dev));
> +	mhi_dev->name = dev_name(mhi_cntrl->cntrl_dev);
>  
>  	/* Init wakeup source */
>  	device_init_wakeup(&mhi_dev->dev, true);
> @@ -1249,7 +1250,7 @@ static int mhi_uevent(struct device *dev, struct kobj_uevent_env *env)
>  	struct mhi_device *mhi_dev = to_mhi_device(dev);
>  
>  	return add_uevent_var(env, "MODALIAS=" MHI_DEVICE_MODALIAS_FMT,
> -					mhi_dev->chan_name);
> +					mhi_dev->name);
>  }
>  
>  static int mhi_match(struct device *dev, struct device_driver *drv)
> @@ -1266,7 +1267,7 @@ static int mhi_match(struct device *dev, struct device_driver *drv)
>  		return 0;
>  
>  	for (id = mhi_drv->id_table; id->chan[0]; id++)
> -		if (!strcmp(mhi_dev->chan_name, id->chan)) {
> +		if (!strcmp(mhi_dev->name, id->chan)) {
>  			mhi_dev->id = id;
>  			return 1;
>  		}
> diff --git a/drivers/bus/mhi/core/main.c b/drivers/bus/mhi/core/main.c
> index 8d6ec34..3af0ce6 100644
> --- a/drivers/bus/mhi/core/main.c
> +++ b/drivers/bus/mhi/core/main.c
> @@ -249,7 +249,7 @@ int mhi_destroy_device(struct device *dev, void *data)
>  		put_device(&mhi_dev->dl_chan->mhi_dev->dev);
>  
>  	dev_dbg(&mhi_cntrl->mhi_dev->dev, "destroy device for chan:%s\n",
> -		 mhi_dev->chan_name);
> +		 mhi_dev->name);
>  
>  	/* Notify the client and remove the device from MHI bus */
>  	device_del(dev);
> @@ -327,10 +327,10 @@ 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;
> +		mhi_dev->name = mhi_chan->name;
>  		dev_set_name(&mhi_dev->dev, "%s_%s",
>  			     dev_name(mhi_cntrl->cntrl_dev),
> -			     mhi_dev->chan_name);
> +			     mhi_dev->name);
>  
>  		/* Init wakeup source if available */
>  		if (mhi_dev->dl_chan && mhi_dev->dl_chan->wake_capable)
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index b008914..7ed785e 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -436,10 +436,10 @@ struct mhi_controller {
>  };
>  
>  /**
> - * struct mhi_device - Structure representing a MHI device which binds
> - *                     to channels
> + * struct mhi_device - Structure representing an MHI device which binds
> + *                     to channels or is associated with controllers
>   * @id: Pointer to MHI device ID struct
> - * @chan_name: Name of the channel to which the device binds
> + * @name: Name of the associated MHI device
>   * @mhi_cntrl: Controller the device belongs to
>   * @ul_chan: UL channel for the device
>   * @dl_chan: DL channel for the device
> @@ -451,7 +451,7 @@ struct mhi_controller {
>   */
>  struct mhi_device {
>  	const struct mhi_device_id *id;
> -	const char *chan_name;
> +	const char *name;
>  	struct mhi_controller *mhi_cntrl;
>  	struct mhi_chan *ul_chan;
>  	struct mhi_chan *dl_chan;
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

[PATCH v4 6/9] bus: mhi: core: Introduce helper function to check device state

[Bhaumik Bhatt]

Introduce a helper function to determine whether the device is in a
powered ON state and resides in one of the active MHI states. This will
allow for some use cases where access can be pre-determined.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
---
 drivers/bus/mhi/core/internal.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
index cb32eaf..997c6e9 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -598,6 +598,11 @@ int mhi_queue_state_transition(struct mhi_controller *mhi_cntrl,
 int __mhi_device_get_sync(struct mhi_controller *mhi_cntrl);
 int mhi_send_cmd(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan,
 		 enum mhi_cmd_type cmd);
+static inline bool mhi_is_active(struct mhi_controller *mhi_cntrl)
+{
+	return (mhi_cntrl->dev_state >= MHI_STATE_M0 &&
+		mhi_cntrl->dev_state <= MHI_STATE_M3_FAST);
+}
 
 static inline void mhi_trigger_resume(struct mhi_controller *mhi_cntrl,
 				      bool hard_wakeup)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 6/9] bus: mhi: core: Introduce helper function to check device state

[Manivannan Sadhasivam]

On Mon, Jun 29, 2020 at 09:39:39AM -0700, Bhaumik Bhatt wrote:
> Introduce a helper function to determine whether the device is in a
> powered ON state and resides in one of the active MHI states. This will
> allow for some use cases where access can be pre-determined.
> 
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>

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

Thanks,
Mani

> ---
>  drivers/bus/mhi/core/internal.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
> index cb32eaf..997c6e9 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -598,6 +598,11 @@ int mhi_queue_state_transition(struct mhi_controller *mhi_cntrl,
>  int __mhi_device_get_sync(struct mhi_controller *mhi_cntrl);
>  int mhi_send_cmd(struct mhi_controller *mhi_cntrl, struct mhi_chan *mhi_chan,
>  		 enum mhi_cmd_type cmd);
> +static inline bool mhi_is_active(struct mhi_controller *mhi_cntrl)
> +{
> +	return (mhi_cntrl->dev_state >= MHI_STATE_M0 &&
> +		mhi_cntrl->dev_state <= MHI_STATE_M3_FAST);
> +}
>  
>  static inline void mhi_trigger_resume(struct mhi_controller *mhi_cntrl,
>  				      bool hard_wakeup)
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

[PATCH v4 7/9] bus: mhi: core: Introduce debugfs entries and counters for MHI

[Bhaumik Bhatt]

Introduce debugfs entries to show state, register, channel, and event
ring information. Add MHI state counters to keep track of the state
changes on the device. Also, allow the host to trigger a device reset,
issue votes, and change the MHI timeout to help in debug.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 drivers/bus/mhi/Kconfig         |   8 +
 drivers/bus/mhi/core/Makefile   |   5 +-
 drivers/bus/mhi/core/debugfs.c  | 444 ++++++++++++++++++++++++++++++++++++++++
 drivers/bus/mhi/core/init.c     |   7 +
 drivers/bus/mhi/core/internal.h |  24 +++
 drivers/bus/mhi/core/pm.c       |   4 +
 include/linux/mhi.h             |   4 +
 7 files changed, 493 insertions(+), 3 deletions(-)
 create mode 100644 drivers/bus/mhi/core/debugfs.c

diff --git a/drivers/bus/mhi/Kconfig b/drivers/bus/mhi/Kconfig
index a8bd9bd..6a217ff 100644
--- a/drivers/bus/mhi/Kconfig
+++ b/drivers/bus/mhi/Kconfig
@@ -12,3 +12,11 @@ config MHI_BUS
 	 communication protocol used by the host processors to control
 	 and communicate with modem devices over a high speed peripheral
 	 bus or shared memory.
+
+config MHI_BUS_DEBUG
+	bool "Debugfs support for the MHI bus"
+	depends on MHI_BUS && DEBUG_FS
+	help
+	 Enable debugfs support for use with the MHI transport. Allows
+	 reading and/or modifying some values within the MHI controller
+	 for debug and test purposes.
diff --git a/drivers/bus/mhi/core/Makefile b/drivers/bus/mhi/core/Makefile
index 66e2700..460a548 100644
--- a/drivers/bus/mhi/core/Makefile
+++ b/drivers/bus/mhi/core/Makefile
@@ -1,3 +1,2 @@
-obj-$(CONFIG_MHI_BUS) := mhi.o
-
-mhi-y := init.o main.o pm.o boot.o
+obj-$(CONFIG_MHI_BUS) := init.o main.o pm.o boot.o
+obj-$(CONFIG_MHI_BUS_DEBUG) += debugfs.o
diff --git a/drivers/bus/mhi/core/debugfs.c b/drivers/bus/mhi/core/debugfs.c
new file mode 100644
index 0000000..266cbf0
--- /dev/null
+++ b/drivers/bus/mhi/core/debugfs.c
@@ -0,0 +1,444 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ *
+ */
+
+#include <linux/debugfs.h>
+#include <linux/device.h>
+#include <linux/interrupt.h>
+#include <linux/list.h>
+#include <linux/mhi.h>
+#include "internal.h"
+
+static int mhi_debugfs_states_show(struct seq_file *m, void *d)
+{
+	struct mhi_controller *mhi_cntrl = m->private;
+
+	/* states */
+	seq_printf(m, "PM state:%s Device:%s MHI state:%s EE:%s wake:%s\n",
+		   to_mhi_pm_state_str(mhi_cntrl->pm_state),
+		   mhi_is_active(mhi_cntrl) ? "Active" : "Inactive",
+		   TO_MHI_STATE_STR(mhi_cntrl->dev_state),
+		   TO_MHI_EXEC_STR(mhi_cntrl->ee),
+		   mhi_cntrl->wake_set ? "true" : "false");
+
+	/* counters */
+	seq_printf(m, "M0:%u M2:%u M3:%u M3_Fast:%u", mhi_cntrl->M0,
+		   mhi_cntrl->M2, mhi_cntrl->M3, mhi_cntrl->M3_fast);
+
+	seq_printf(m, " device wake:%u pending packets:%u\n",
+		   atomic_read(&mhi_cntrl->dev_wake),
+		   atomic_read(&mhi_cntrl->pending_pkts));
+
+	return 0;
+}
+
+static int mhi_debugfs_events_show(struct seq_file *m, void *d)
+{
+	struct mhi_controller *mhi_cntrl = m->private;
+	struct mhi_event *mhi_event;
+	struct mhi_event_ctxt *er_ctxt;
+	int i;
+
+	if (!mhi_is_active(mhi_cntrl)) {
+		seq_puts(m, "Device not ready\n");
+		return -ENODEV;
+	}
+
+	er_ctxt = mhi_cntrl->mhi_ctxt->er_ctxt;
+	mhi_event = mhi_cntrl->mhi_event;
+	for (i = 0; i < mhi_cntrl->total_ev_rings;
+						i++, er_ctxt++, mhi_event++) {
+		struct mhi_ring *ring = &mhi_event->ring;
+
+		if (mhi_event->offload_ev) {
+			seq_printf(m, "Index:%d is an offload event ring\n", i);
+			continue;
+		}
+
+		seq_printf(m, "Index:%d intmod count:%lu time:%lu",
+			   i, (er_ctxt->intmod & EV_CTX_INTMODC_MASK) >>
+			   EV_CTX_INTMODC_SHIFT,
+			   (er_ctxt->intmod & EV_CTX_INTMODT_MASK) >>
+			   EV_CTX_INTMODT_SHIFT);
+
+		seq_printf(m, " base:0x%0llx len:0x%llx", er_ctxt->rbase,
+			   er_ctxt->rlen);
+
+		seq_printf(m,
+			   " rp:0x%llx wp:0x%llx local rp:0x%llx db:0x%llx\n",
+			   er_ctxt->rp, er_ctxt->wp, (u64)ring->rp,
+			   mhi_event->db_cfg.db_val);
+	}
+
+	return 0;
+}
+
+static int mhi_debugfs_channels_show(struct seq_file *m, void *d)
+{
+	struct mhi_controller *mhi_cntrl = m->private;
+	struct mhi_chan *mhi_chan;
+	struct mhi_chan_ctxt *chan_ctxt;
+	int i;
+
+	if (!mhi_is_active(mhi_cntrl)) {
+		seq_puts(m, "Device not ready\n");
+		return -ENODEV;
+	}
+
+	mhi_chan = mhi_cntrl->mhi_chan;
+	chan_ctxt = mhi_cntrl->mhi_ctxt->chan_ctxt;
+	for (i = 0; i < mhi_cntrl->max_chan; i++, chan_ctxt++, mhi_chan++) {
+		struct mhi_ring *ring = &mhi_chan->tre_ring;
+
+		if (mhi_chan->offload_ch) {
+			seq_printf(m, "%s(%u) is an offload channel\n",
+				   mhi_chan->name, mhi_chan->chan);
+			continue;
+		}
+
+		if (!mhi_chan->mhi_dev)
+			continue;
+
+		seq_printf(m,
+			   "%s(%u) state:0x%lx brstmode:0x%lx pollcfg:0x%lx",
+			   mhi_chan->name, mhi_chan->chan, (chan_ctxt->chcfg &
+			   CHAN_CTX_CHSTATE_MASK) >> CHAN_CTX_CHSTATE_SHIFT,
+			   (chan_ctxt->chcfg & CHAN_CTX_BRSTMODE_MASK) >>
+			   CHAN_CTX_BRSTMODE_SHIFT, (chan_ctxt->chcfg &
+			   CHAN_CTX_POLLCFG_MASK) >> CHAN_CTX_POLLCFG_SHIFT);
+
+		seq_printf(m, " type:0x%x event ring:%u", chan_ctxt->chtype,
+			   chan_ctxt->erindex);
+
+		seq_printf(m, " base:0x%llx len:0x%llx wp:0x%llx",
+			   chan_ctxt->rbase, chan_ctxt->rlen, chan_ctxt->wp);
+
+		seq_printf(m, " local rp:0x%llx local wp:0x%llx db:0x%llx\n",
+			   (u64)ring->rp, (u64)ring->wp,
+			   mhi_chan->db_cfg.db_val);
+	}
+
+	return 0;
+}
+
+static int mhi_device_votes_show(struct device *dev, void *data)
+{
+	struct mhi_device *mhi_dev;
+
+	if (dev->bus != &mhi_bus_type)
+		return 0;
+
+	mhi_dev = to_mhi_device(dev);
+
+	seq_printf((struct seq_file *)data, "%s: %u\n",
+		   mhi_dev->name, mhi_dev->dev_wake);
+
+	return 0;
+}
+
+static int mhi_debugfs_votes_show(struct seq_file *m, void *d)
+{
+	struct mhi_controller *mhi_cntrl = m->private;
+
+	if (!mhi_is_active(mhi_cntrl)) {
+		seq_puts(m, "Device not ready\n");
+		return -ENODEV;
+	}
+
+	device_for_each_child(mhi_cntrl->cntrl_dev, m, mhi_device_votes_show);
+
+	return 0;
+}
+
+static int mhi_debugfs_regdump_show(struct seq_file *m, void *d)
+{
+	struct mhi_controller *mhi_cntrl = m->private;
+	enum mhi_state state;
+	enum mhi_ee_type ee;
+	int i, ret = -EIO;
+	u32 val;
+	void __iomem *mhi_base = mhi_cntrl->regs;
+	void __iomem *bhi_base = mhi_cntrl->bhi;
+	void __iomem *bhie_base = mhi_cntrl->bhie;
+	void __iomem *wake_db = mhi_cntrl->wake_db;
+	struct {
+		const char *name;
+		int offset;
+		void __iomem *base;
+	} debug_regs[] = {
+		{ "MHI_CTRL", MHICTRL, mhi_base},
+		{ "MHI_STATUS", MHISTATUS, mhi_base},
+		{ "MHI_WAKE_DB", 0, wake_db},
+		{ "BHI_EXECENV", BHI_EXECENV, bhi_base},
+		{ "BHI_STATUS", BHI_STATUS, bhi_base},
+		{ "BHI_ERRCODE", BHI_ERRCODE, bhi_base},
+		{ "BHI_ERRDBG1", BHI_ERRDBG1, bhi_base},
+		{ "BHI_ERRDBG2", BHI_ERRDBG2, bhi_base},
+		{ "BHI_ERRDBG3", BHI_ERRDBG3, bhi_base},
+		{ "BHIE_TXVEC_DB", BHIE_TXVECDB_OFFS, bhie_base},
+		{ "BHIE_TXVEC_STATUS", BHIE_TXVECSTATUS_OFFS, bhie_base},
+		{ "BHIE_RXVEC_DB", BHIE_RXVECDB_OFFS, bhie_base},
+		{ "BHIE_RXVEC_STATUS", BHIE_RXVECSTATUS_OFFS, bhie_base},
+		{ NULL },
+	};
+
+	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state))
+		return ret;
+
+	seq_printf(m, "Host PM state:%s Device state:%s EE:%s\n",
+		   to_mhi_pm_state_str(mhi_cntrl->pm_state),
+		   TO_MHI_STATE_STR(mhi_cntrl->dev_state),
+		   TO_MHI_EXEC_STR(mhi_cntrl->ee));
+
+	state = mhi_get_mhi_state(mhi_cntrl);
+	ee = mhi_get_exec_env(mhi_cntrl);
+	seq_printf(m, "Device EE:%s state:%s\n", TO_MHI_EXEC_STR(ee),
+		   TO_MHI_STATE_STR(state));
+
+	for (i = 0; debug_regs[i].name; i++) {
+		if (!debug_regs[i].base)
+			continue;
+		ret = mhi_read_reg(mhi_cntrl, debug_regs[i].base,
+				   debug_regs[i].offset, &val);
+		if (ret)
+			continue;
+
+		seq_printf(m, "%s:0x%x\n", debug_regs[i].name, val);
+	}
+
+	return 0;
+}
+
+static int mhi_debugfs_device_vote_show(struct seq_file *m, void *d)
+{
+	struct mhi_controller *mhi_cntrl = m->private;
+	struct mhi_device *mhi_dev = mhi_cntrl->mhi_dev;
+
+	if (!mhi_is_active(mhi_cntrl)) {
+		seq_puts(m, "Device not ready\n");
+		return -ENODEV;
+	}
+
+	seq_printf(m,
+		   "Votes: %d\n%s\n", mhi_dev->dev_wake,
+		   "Usage: echo get/put > device_vote for vote/unvote");
+
+	return 0;
+}
+
+static ssize_t mhi_debugfs_device_vote_write(struct file *file,
+					     const char __user *ubuf,
+					     size_t count, loff_t *ppos)
+{
+	struct seq_file	*m = file->private_data;
+	struct mhi_controller *mhi_cntrl = m->private;
+	struct mhi_device *mhi_dev = mhi_cntrl->mhi_dev;
+	char buf[32];
+	int ret = -EINVAL;
+
+	if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
+		return -EFAULT;
+
+	if (!strncmp(buf, "get", 3)) {
+		ret = mhi_device_get_sync(mhi_dev);
+	} else if (!strncmp(buf, "put", 3)) {
+		mhi_device_put(mhi_dev);
+		ret = 0;
+	}
+
+	return ret ? ret : count;
+}
+
+static int mhi_debugfs_timeout_ms_show(struct seq_file *m, void *d)
+{
+	struct mhi_controller *mhi_cntrl = m->private;
+
+	seq_printf(m, "%u ms\n", mhi_cntrl->timeout_ms);
+
+	return 0;
+}
+
+static ssize_t mhi_debugfs_timeout_ms_write(struct file *file,
+					    const char __user *ubuf,
+					    size_t count, loff_t *ppos)
+{
+	struct seq_file	*m = file->private_data;
+	struct mhi_controller *mhi_cntrl = m->private;
+	u32 timeout_ms;
+
+	if (kstrtou32_from_user(ubuf, count, 0, &timeout_ms))
+		return -EINVAL;
+
+	mhi_cntrl->timeout_ms = timeout_ms;
+
+	return count;
+}
+
+static int mhi_debugfs_trigger_reset(void *data, u64 val)
+{
+	struct mhi_controller *mhi_cntrl = data;
+	struct mhi_device *mhi_dev = mhi_cntrl->mhi_dev;
+	struct device *dev = &mhi_dev->dev;
+	enum mhi_pm_state cur_state;
+	int ret = -EIO;
+
+	if (!mhi_is_active(mhi_cntrl))
+		return -ENODEV;
+
+	if (!val)
+		return -EINVAL;
+
+	ret = mhi_device_get_sync(mhi_dev);
+	if (ret) {
+		dev_err(dev, "Device did not enter M0 state, MHI:%s, PM:%s\n",
+			TO_MHI_STATE_STR(mhi_cntrl->dev_state),
+			to_mhi_pm_state_str(mhi_cntrl->pm_state));
+		return ret;
+	}
+
+	if (mhi_cntrl->rddm_image) {
+		ret = mhi_force_rddm_mode(mhi_cntrl);
+		goto exit_mhi_trigger_reset;
+	}
+
+	write_lock_irq(&mhi_cntrl->pm_lock);
+	cur_state = mhi_tryset_pm_state(mhi_cntrl, MHI_PM_SYS_ERR_DETECT);
+	write_unlock_irq(&mhi_cntrl->pm_lock);
+
+	if (cur_state != MHI_PM_SYS_ERR_DETECT)
+		goto exit_mhi_trigger_reset;
+
+	mhi_pm_sys_err_handler(mhi_cntrl);
+	ret = 0;
+
+exit_mhi_trigger_reset:
+	mhi_device_put(mhi_dev);
+
+	return ret;
+}
+
+static int mhi_debugfs_states_open(struct inode *inode, struct file *fp)
+{
+	return single_open(fp, mhi_debugfs_states_show, inode->i_private);
+}
+
+static int mhi_debugfs_events_open(struct inode *inode, struct file *fp)
+{
+	return single_open(fp, mhi_debugfs_events_show, inode->i_private);
+}
+
+static int mhi_debugfs_channels_open(struct inode *inode, struct file *fp)
+{
+	return single_open(fp, mhi_debugfs_channels_show, inode->i_private);
+}
+
+static int mhi_debugfs_votes_open(struct inode *inode, struct file *fp)
+{
+	return single_open(fp, mhi_debugfs_votes_show, inode->i_private);
+}
+
+static int mhi_debugfs_regdump_open(struct inode *inode, struct file *fp)
+{
+	return single_open(fp, mhi_debugfs_regdump_show, inode->i_private);
+}
+
+static int mhi_debugfs_device_vote_open(struct inode *inode, struct file *fp)
+{
+	return single_open(fp, mhi_debugfs_device_vote_show, inode->i_private);
+}
+
+static int mhi_debugfs_timeout_ms_open(struct inode *inode, struct file *fp)
+{
+	return single_open(fp, mhi_debugfs_timeout_ms_show, inode->i_private);
+}
+
+static const struct file_operations debugfs_states_fops = {
+	.open = mhi_debugfs_states_open,
+	.release = single_release,
+	.read = seq_read,
+};
+
+static const struct file_operations debugfs_events_fops = {
+	.open = mhi_debugfs_events_open,
+	.release = single_release,
+	.read = seq_read,
+};
+
+static const struct file_operations debugfs_channels_fops = {
+	.open = mhi_debugfs_channels_open,
+	.release = single_release,
+	.read = seq_read,
+};
+
+static const struct file_operations debugfs_votes_fops = {
+	.open = mhi_debugfs_votes_open,
+	.release = single_release,
+	.read = seq_read,
+};
+
+static const struct file_operations debugfs_regdump_fops = {
+	.open = mhi_debugfs_regdump_open,
+	.release = single_release,
+	.read = seq_read,
+};
+
+static const struct file_operations debugfs_device_vote_fops = {
+	.open = mhi_debugfs_device_vote_open,
+	.write = mhi_debugfs_device_vote_write,
+	.release = single_release,
+	.read = seq_read,
+};
+
+static const struct file_operations debugfs_timeout_ms_fops = {
+	.open = mhi_debugfs_timeout_ms_open,
+	.write = mhi_debugfs_timeout_ms_write,
+	.release = single_release,
+	.read = seq_read,
+};
+
+DEFINE_DEBUGFS_ATTRIBUTE(debugfs_reset_fops, NULL,
+			 mhi_debugfs_trigger_reset, "%llu\n");
+
+static struct dentry *mhi_debugfs_root;
+
+void mhi_create_debugfs(struct mhi_controller *mhi_cntrl)
+{
+	mhi_cntrl->debugfs_dentry =
+			debugfs_create_dir(dev_name(mhi_cntrl->cntrl_dev),
+					   mhi_debugfs_root);
+
+	debugfs_create_file("states", 0444, mhi_cntrl->debugfs_dentry,
+			    mhi_cntrl, &debugfs_states_fops);
+	debugfs_create_file("events", 0444, mhi_cntrl->debugfs_dentry,
+			    mhi_cntrl, &debugfs_events_fops);
+	debugfs_create_file("channels", 0444, mhi_cntrl->debugfs_dentry,
+			    mhi_cntrl, &debugfs_channels_fops);
+	debugfs_create_file("votes", 0444, mhi_cntrl->debugfs_dentry,
+			    mhi_cntrl, &debugfs_votes_fops);
+	debugfs_create_file("regdump", 0444, mhi_cntrl->debugfs_dentry,
+			    mhi_cntrl, &debugfs_regdump_fops);
+	debugfs_create_file("device_vote", 0644, mhi_cntrl->debugfs_dentry,
+			    mhi_cntrl, &debugfs_device_vote_fops);
+	debugfs_create_file("timeout_ms", 0644, mhi_cntrl->debugfs_dentry,
+			    mhi_cntrl, &debugfs_timeout_ms_fops);
+	debugfs_create_file("reset", 0444, mhi_cntrl->debugfs_dentry,
+			    mhi_cntrl, &debugfs_reset_fops);
+}
+
+void mhi_destroy_debugfs(struct mhi_controller *mhi_cntrl)
+{
+	debugfs_remove_recursive(mhi_cntrl->debugfs_dentry);
+	mhi_cntrl->debugfs_dentry = NULL;
+}
+
+void mhi_debugfs_init(void)
+{
+	mhi_debugfs_root = debugfs_create_dir(mhi_bus_type.name, NULL);
+}
+
+void mhi_debugfs_exit(void)
+{
+	debugfs_remove_recursive(mhi_debugfs_root);
+}
diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index e2011ec..d2c0f6e 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -4,6 +4,7 @@
  *
  */
 
+#include <linux/debugfs.h>
 #include <linux/device.h>
 #include <linux/dma-direction.h>
 #include <linux/dma-mapping.h>
@@ -915,6 +916,8 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
 
 	mhi_cntrl->mhi_dev = mhi_dev;
 
+	mhi_create_debugfs(mhi_cntrl);
+
 	return 0;
 
 error_add_dev:
@@ -937,6 +940,8 @@ void mhi_unregister_controller(struct mhi_controller *mhi_cntrl)
 	struct mhi_chan *mhi_chan = mhi_cntrl->mhi_chan;
 	unsigned int i;
 
+	mhi_destroy_debugfs(mhi_cntrl);
+
 	kfree(mhi_cntrl->mhi_cmd);
 	kfree(mhi_cntrl->mhi_event);
 
@@ -1284,11 +1289,13 @@ struct bus_type mhi_bus_type = {
 
 static int __init mhi_init(void)
 {
+	mhi_debugfs_init();
 	return bus_register(&mhi_bus_type);
 }
 
 static void __exit mhi_exit(void)
 {
+	mhi_debugfs_exit();
 	bus_unregister(&mhi_bus_type);
 }
 
diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
index 997c6e9..368c442 100644
--- a/drivers/bus/mhi/core/internal.h
+++ b/drivers/bus/mhi/core/internal.h
@@ -570,6 +570,30 @@ struct mhi_chan {
 /* Default MHI timeout */
 #define MHI_TIMEOUT_MS (1000)
 
+/* debugfs related functions */
+#ifdef CONFIG_MHI_BUS_DEBUG
+void mhi_create_debugfs(struct mhi_controller *mhi_cntrl);
+void mhi_destroy_debugfs(struct mhi_controller *mhi_cntrl);
+void mhi_debugfs_init(void);
+void mhi_debugfs_exit(void);
+#else
+static inline void mhi_create_debugfs(struct mhi_controller *mhi_cntrl)
+{
+}
+
+static inline void mhi_destroy_debugfs(struct mhi_controller *mhi_cntrl)
+{
+}
+
+static inline void mhi_debugfs_init(void)
+{
+}
+
+static inline void mhi_debugfs_exit(void)
+{
+}
+#endif
+
 struct mhi_device *mhi_alloc_device(struct mhi_controller *mhi_cntrl);
 
 int mhi_destroy_device(struct device *dev, void *data);
diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
index 74c5cb1..c80d3553 100644
--- a/drivers/bus/mhi/core/pm.c
+++ b/drivers/bus/mhi/core/pm.c
@@ -256,6 +256,7 @@ int mhi_pm_m0_transition(struct mhi_controller *mhi_cntrl)
 		dev_err(dev, "Unable to transition to M0 state\n");
 		return -EIO;
 	}
+	mhi_cntrl->M0++;
 
 	/* Wake up the device */
 	read_lock_bh(&mhi_cntrl->pm_lock);
@@ -326,6 +327,8 @@ void mhi_pm_m1_transition(struct mhi_controller *mhi_cntrl)
 		mhi_cntrl->dev_state = MHI_STATE_M2;
 
 		write_unlock_irq(&mhi_cntrl->pm_lock);
+
+		mhi_cntrl->M2++;
 		wake_up_all(&mhi_cntrl->state_event);
 
 		/* If there are any pending resources, exit M2 immediately */
@@ -362,6 +365,7 @@ int mhi_pm_m3_transition(struct mhi_controller *mhi_cntrl)
 		return -EIO;
 	}
 
+	mhi_cntrl->M3++;
 	wake_up_all(&mhi_cntrl->state_event);
 
 	return 0;
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index 7ed785e..b1e8b4f 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -288,6 +288,7 @@ struct mhi_controller_config {
  * @cntrl_dev: Pointer to the struct device of physical bus acting as the MHI
  *            controller (required)
  * @mhi_dev: MHI device instance for the controller
+ * @debugfs_dentry: MHI controller debugfs directory
  * @regs: Base address of MHI MMIO register space (required)
  * @bhi: Points to base of MHI BHI register space
  * @bhie: Points to base of MHI BHIe register space
@@ -326,6 +327,7 @@ struct mhi_controller_config {
  * @dev_state: MHI device state
  * @dev_wake: Device wakeup count
  * @pending_pkts: Pending packets for the controller
+ * @M0, M2, M3, M3_fast: Counters to track number of device MHI state changes
  * @transition_list: List of MHI state transitions
  * @transition_lock: Lock for protecting MHI state transition list
  * @wlock: Lock for protecting device wakeup
@@ -364,6 +366,7 @@ struct mhi_controller_config {
 struct mhi_controller {
 	struct device *cntrl_dev;
 	struct mhi_device *mhi_dev;
+	struct dentry *debugfs_dentry;
 	void __iomem *regs;
 	void __iomem *bhi;
 	void __iomem *bhie;
@@ -405,6 +408,7 @@ struct mhi_controller {
 	enum mhi_state dev_state;
 	atomic_t dev_wake;
 	atomic_t pending_pkts;
+	u32 M0, M2, M3, M3_fast;
 	struct list_head transition_list;
 	spinlock_t transition_lock;
 	spinlock_t wlock;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 7/9] bus: mhi: core: Introduce debugfs entries and counters for MHI

[Manivannan Sadhasivam]

On Mon, Jun 29, 2020 at 09:39:40AM -0700, Bhaumik Bhatt wrote:
> Introduce debugfs entries to show state, register, channel, and event
> ring information. Add MHI state counters to keep track of the state
> changes on the device. Also, allow the host to trigger a device reset,
> issue votes, and change the MHI timeout to help in debug.
> 
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>  drivers/bus/mhi/Kconfig         |   8 +
>  drivers/bus/mhi/core/Makefile   |   5 +-
>  drivers/bus/mhi/core/debugfs.c  | 444 ++++++++++++++++++++++++++++++++++++++++
>  drivers/bus/mhi/core/init.c     |   7 +
>  drivers/bus/mhi/core/internal.h |  24 +++
>  drivers/bus/mhi/core/pm.c       |   4 +
>  include/linux/mhi.h             |   4 +
>  7 files changed, 493 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/bus/mhi/core/debugfs.c
> 
> diff --git a/drivers/bus/mhi/Kconfig b/drivers/bus/mhi/Kconfig
> index a8bd9bd..6a217ff 100644
> --- a/drivers/bus/mhi/Kconfig
> +++ b/drivers/bus/mhi/Kconfig
> @@ -12,3 +12,11 @@ config MHI_BUS
>  	 communication protocol used by the host processors to control
>  	 and communicate with modem devices over a high speed peripheral
>  	 bus or shared memory.
> +
> +config MHI_BUS_DEBUG
> +	bool "Debugfs support for the MHI bus"
> +	depends on MHI_BUS && DEBUG_FS
> +	help
> +	 Enable debugfs support for use with the MHI transport. Allows
> +	 reading and/or modifying some values within the MHI controller
> +	 for debug and test purposes.
> diff --git a/drivers/bus/mhi/core/Makefile b/drivers/bus/mhi/core/Makefile
> index 66e2700..460a548 100644
> --- a/drivers/bus/mhi/core/Makefile
> +++ b/drivers/bus/mhi/core/Makefile
> @@ -1,3 +1,2 @@
> -obj-$(CONFIG_MHI_BUS) := mhi.o
> -
> -mhi-y := init.o main.o pm.o boot.o
> +obj-$(CONFIG_MHI_BUS) := init.o main.o pm.o boot.o
> +obj-$(CONFIG_MHI_BUS_DEBUG) += debugfs.o
> diff --git a/drivers/bus/mhi/core/debugfs.c b/drivers/bus/mhi/core/debugfs.c
> new file mode 100644
> index 0000000..266cbf0
> --- /dev/null
> +++ b/drivers/bus/mhi/core/debugfs.c
> @@ -0,0 +1,444 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + *
> + */
> +
> +#include <linux/debugfs.h>
> +#include <linux/device.h>
> +#include <linux/interrupt.h>
> +#include <linux/list.h>
> +#include <linux/mhi.h>
> +#include "internal.h"
> +
> +static int mhi_debugfs_states_show(struct seq_file *m, void *d)
> +{
> +	struct mhi_controller *mhi_cntrl = m->private;
> +
> +	/* states */
> +	seq_printf(m, "PM state:%s Device:%s MHI state:%s EE:%s wake:%s\n",
> +		   to_mhi_pm_state_str(mhi_cntrl->pm_state),
> +		   mhi_is_active(mhi_cntrl) ? "Active" : "Inactive",
> +		   TO_MHI_STATE_STR(mhi_cntrl->dev_state),
> +		   TO_MHI_EXEC_STR(mhi_cntrl->ee),
> +		   mhi_cntrl->wake_set ? "true" : "false");

Nit: Always use a space after ":".

> +
> +	/* counters */
> +	seq_printf(m, "M0:%u M2:%u M3:%u M3_Fast:%u", mhi_cntrl->M0,
> +		   mhi_cntrl->M2, mhi_cntrl->M3, mhi_cntrl->M3_fast);
> +
> +	seq_printf(m, " device wake:%u pending packets:%u\n",
> +		   atomic_read(&mhi_cntrl->dev_wake),
> +		   atomic_read(&mhi_cntrl->pending_pkts));
> +
> +	return 0;
> +}
> +
> +static int mhi_debugfs_events_show(struct seq_file *m, void *d)
> +{
> +	struct mhi_controller *mhi_cntrl = m->private;
> +	struct mhi_event *mhi_event;
> +	struct mhi_event_ctxt *er_ctxt;
> +	int i;
> +
> +	if (!mhi_is_active(mhi_cntrl)) {
> +		seq_puts(m, "Device not ready\n");
> +		return -ENODEV;
> +	}
> +
> +	er_ctxt = mhi_cntrl->mhi_ctxt->er_ctxt;
> +	mhi_event = mhi_cntrl->mhi_event;
> +	for (i = 0; i < mhi_cntrl->total_ev_rings;
> +						i++, er_ctxt++, mhi_event++) {
> +		struct mhi_ring *ring = &mhi_event->ring;
> +
> +		if (mhi_event->offload_ev) {
> +			seq_printf(m, "Index:%d is an offload event ring\n", i);
> +			continue;
> +		}
> +
> +		seq_printf(m, "Index:%d intmod count:%lu time:%lu",
> +			   i, (er_ctxt->intmod & EV_CTX_INTMODC_MASK) >>
> +			   EV_CTX_INTMODC_SHIFT,
> +			   (er_ctxt->intmod & EV_CTX_INTMODT_MASK) >>
> +			   EV_CTX_INTMODT_SHIFT);
> +
> +		seq_printf(m, " base:0x%0llx len:0x%llx", er_ctxt->rbase,
> +			   er_ctxt->rlen);
> +
> +		seq_printf(m,
> +			   " rp:0x%llx wp:0x%llx local rp:0x%llx db:0x%llx\n",
> +			   er_ctxt->rp, er_ctxt->wp, (u64)ring->rp,
> +			   mhi_event->db_cfg.db_val);
> +	}
> +
> +	return 0;
> +}
> +
> +static int mhi_debugfs_channels_show(struct seq_file *m, void *d)
> +{
> +	struct mhi_controller *mhi_cntrl = m->private;
> +	struct mhi_chan *mhi_chan;
> +	struct mhi_chan_ctxt *chan_ctxt;
> +	int i;
> +
> +	if (!mhi_is_active(mhi_cntrl)) {
> +		seq_puts(m, "Device not ready\n");
> +		return -ENODEV;
> +	}
> +
> +	mhi_chan = mhi_cntrl->mhi_chan;
> +	chan_ctxt = mhi_cntrl->mhi_ctxt->chan_ctxt;
> +	for (i = 0; i < mhi_cntrl->max_chan; i++, chan_ctxt++, mhi_chan++) {
> +		struct mhi_ring *ring = &mhi_chan->tre_ring;
> +
> +		if (mhi_chan->offload_ch) {
> +			seq_printf(m, "%s(%u) is an offload channel\n",
> +				   mhi_chan->name, mhi_chan->chan);
> +			continue;
> +		}
> +
> +		if (!mhi_chan->mhi_dev)
> +			continue;
> +
> +		seq_printf(m,
> +			   "%s(%u) state:0x%lx brstmode:0x%lx pollcfg:0x%lx",
> +			   mhi_chan->name, mhi_chan->chan, (chan_ctxt->chcfg &
> +			   CHAN_CTX_CHSTATE_MASK) >> CHAN_CTX_CHSTATE_SHIFT,
> +			   (chan_ctxt->chcfg & CHAN_CTX_BRSTMODE_MASK) >>
> +			   CHAN_CTX_BRSTMODE_SHIFT, (chan_ctxt->chcfg &
> +			   CHAN_CTX_POLLCFG_MASK) >> CHAN_CTX_POLLCFG_SHIFT);
> +
> +		seq_printf(m, " type:0x%x event ring:%u", chan_ctxt->chtype,
> +			   chan_ctxt->erindex);
> +
> +		seq_printf(m, " base:0x%llx len:0x%llx wp:0x%llx",
> +			   chan_ctxt->rbase, chan_ctxt->rlen, chan_ctxt->wp);
> +
> +		seq_printf(m, " local rp:0x%llx local wp:0x%llx db:0x%llx\n",
> +			   (u64)ring->rp, (u64)ring->wp,
> +			   mhi_chan->db_cfg.db_val);
> +	}
> +
> +	return 0;
> +}
> +
> +static int mhi_device_votes_show(struct device *dev, void *data)
> +{
> +	struct mhi_device *mhi_dev;
> +
> +	if (dev->bus != &mhi_bus_type)
> +		return 0;
> +
> +	mhi_dev = to_mhi_device(dev);
> +
> +	seq_printf((struct seq_file *)data, "%s: %u\n",
> +		   mhi_dev->name, mhi_dev->dev_wake);
> +
> +	return 0;
> +}
> +
> +static int mhi_debugfs_votes_show(struct seq_file *m, void *d)
> +{
> +	struct mhi_controller *mhi_cntrl = m->private;
> +
> +	if (!mhi_is_active(mhi_cntrl)) {
> +		seq_puts(m, "Device not ready\n");
> +		return -ENODEV;
> +	}
> +
> +	device_for_each_child(mhi_cntrl->cntrl_dev, m, mhi_device_votes_show);
> +
> +	return 0;
> +}
> +
> +static int mhi_debugfs_regdump_show(struct seq_file *m, void *d)
> +{
> +	struct mhi_controller *mhi_cntrl = m->private;
> +	enum mhi_state state;
> +	enum mhi_ee_type ee;
> +	int i, ret = -EIO;
> +	u32 val;
> +	void __iomem *mhi_base = mhi_cntrl->regs;
> +	void __iomem *bhi_base = mhi_cntrl->bhi;
> +	void __iomem *bhie_base = mhi_cntrl->bhie;
> +	void __iomem *wake_db = mhi_cntrl->wake_db;
> +	struct {
> +		const char *name;
> +		int offset;
> +		void __iomem *base;
> +	} debug_regs[] = {
> +		{ "MHI_CTRL", MHICTRL, mhi_base},
> +		{ "MHI_STATUS", MHISTATUS, mhi_base},
> +		{ "MHI_WAKE_DB", 0, wake_db},
> +		{ "BHI_EXECENV", BHI_EXECENV, bhi_base},
> +		{ "BHI_STATUS", BHI_STATUS, bhi_base},
> +		{ "BHI_ERRCODE", BHI_ERRCODE, bhi_base},
> +		{ "BHI_ERRDBG1", BHI_ERRDBG1, bhi_base},
> +		{ "BHI_ERRDBG2", BHI_ERRDBG2, bhi_base},
> +		{ "BHI_ERRDBG3", BHI_ERRDBG3, bhi_base},
> +		{ "BHIE_TXVEC_DB", BHIE_TXVECDB_OFFS, bhie_base},
> +		{ "BHIE_TXVEC_STATUS", BHIE_TXVECSTATUS_OFFS, bhie_base},
> +		{ "BHIE_RXVEC_DB", BHIE_RXVECDB_OFFS, bhie_base},
> +		{ "BHIE_RXVEC_STATUS", BHIE_RXVECSTATUS_OFFS, bhie_base},
> +		{ NULL },
> +	};
> +
> +	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state))
> +		return ret;
> +
> +	seq_printf(m, "Host PM state:%s Device state:%s EE:%s\n",
> +		   to_mhi_pm_state_str(mhi_cntrl->pm_state),
> +		   TO_MHI_STATE_STR(mhi_cntrl->dev_state),
> +		   TO_MHI_EXEC_STR(mhi_cntrl->ee));
> +
> +	state = mhi_get_mhi_state(mhi_cntrl);
> +	ee = mhi_get_exec_env(mhi_cntrl);
> +	seq_printf(m, "Device EE:%s state:%s\n", TO_MHI_EXEC_STR(ee),
> +		   TO_MHI_STATE_STR(state));
> +
> +	for (i = 0; debug_regs[i].name; i++) {
> +		if (!debug_regs[i].base)
> +			continue;
> +		ret = mhi_read_reg(mhi_cntrl, debug_regs[i].base,
> +				   debug_regs[i].offset, &val);
> +		if (ret)
> +			continue;
> +
> +		seq_printf(m, "%s:0x%x\n", debug_regs[i].name, val);
> +	}
> +
> +	return 0;
> +}
> +
> +static int mhi_debugfs_device_vote_show(struct seq_file *m, void *d)
> +{

The term 'vote' is confusing here. Can you come up with something which portrays
device power mode here?

> +	struct mhi_controller *mhi_cntrl = m->private;
> +	struct mhi_device *mhi_dev = mhi_cntrl->mhi_dev;
> +
> +	if (!mhi_is_active(mhi_cntrl)) {
> +		seq_puts(m, "Device not ready\n");
> +		return -ENODEV;
> +	}
> +
> +	seq_printf(m,
> +		   "Votes: %d\n%s\n", mhi_dev->dev_wake,
> +		   "Usage: echo get/put > device_vote for vote/unvote");
> +
> +	return 0;
> +}
> +
> +static ssize_t mhi_debugfs_device_vote_write(struct file *file,
> +					     const char __user *ubuf,
> +					     size_t count, loff_t *ppos)
> +{
> +	struct seq_file	*m = file->private_data;
> +	struct mhi_controller *mhi_cntrl = m->private;
> +	struct mhi_device *mhi_dev = mhi_cntrl->mhi_dev;
> +	char buf[32];
> +	int ret = -EINVAL;
> +
> +	if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
> +		return -EFAULT;
> +
> +	if (!strncmp(buf, "get", 3)) {

Hmm, but the buffer size is 32?

> +		ret = mhi_device_get_sync(mhi_dev);
> +	} else if (!strncmp(buf, "put", 3)) {
> +		mhi_device_put(mhi_dev);
> +		ret = 0;
> +	}
> +
> +	return ret ? ret : count;
> +}
> +
> +static int mhi_debugfs_timeout_ms_show(struct seq_file *m, void *d)
> +{
> +	struct mhi_controller *mhi_cntrl = m->private;
> +
> +	seq_printf(m, "%u ms\n", mhi_cntrl->timeout_ms);
> +
> +	return 0;
> +}
> +
> +static ssize_t mhi_debugfs_timeout_ms_write(struct file *file,
> +					    const char __user *ubuf,
> +					    size_t count, loff_t *ppos)
> +{
> +	struct seq_file	*m = file->private_data;
> +	struct mhi_controller *mhi_cntrl = m->private;
> +	u32 timeout_ms;
> +
> +	if (kstrtou32_from_user(ubuf, count, 0, &timeout_ms))
> +		return -EINVAL;
> +
> +	mhi_cntrl->timeout_ms = timeout_ms;
> +
> +	return count;
> +}
> +
> +static int mhi_debugfs_trigger_reset(void *data, u64 val)

Triggering reset on host or device?

> +{
> +	struct mhi_controller *mhi_cntrl = data;
> +	struct mhi_device *mhi_dev = mhi_cntrl->mhi_dev;
> +	struct device *dev = &mhi_dev->dev;
> +	enum mhi_pm_state cur_state;
> +	int ret = -EIO;
> +
> +	if (!mhi_is_active(mhi_cntrl))
> +		return -ENODEV;
> +
> +	if (!val)
> +		return -EINVAL;
> +
> +	ret = mhi_device_get_sync(mhi_dev);
> +	if (ret) {
> +		dev_err(dev, "Device did not enter M0 state, MHI:%s, PM:%s\n",
> +			TO_MHI_STATE_STR(mhi_cntrl->dev_state),
> +			to_mhi_pm_state_str(mhi_cntrl->pm_state));
> +		return ret;
> +	}
> +
> +	if (mhi_cntrl->rddm_image) {
> +		ret = mhi_force_rddm_mode(mhi_cntrl);
> +		goto exit_mhi_trigger_reset;
> +	}
> +
> +	write_lock_irq(&mhi_cntrl->pm_lock);
> +	cur_state = mhi_tryset_pm_state(mhi_cntrl, MHI_PM_SYS_ERR_DETECT);

Looks like in host but how does this resets it?

> +	write_unlock_irq(&mhi_cntrl->pm_lock);
> +
> +	if (cur_state != MHI_PM_SYS_ERR_DETECT)
> +		goto exit_mhi_trigger_reset;

So this condition is also a success? PS: you're not setting ret here.

> +
> +	mhi_pm_sys_err_handler(mhi_cntrl);
> +	ret = 0;
> +
> +exit_mhi_trigger_reset:
> +	mhi_device_put(mhi_dev);
> +
> +	return ret;
> +}
> +
> +static int mhi_debugfs_states_open(struct inode *inode, struct file *fp)
> +{
> +	return single_open(fp, mhi_debugfs_states_show, inode->i_private);
> +}
> +
> +static int mhi_debugfs_events_open(struct inode *inode, struct file *fp)
> +{
> +	return single_open(fp, mhi_debugfs_events_show, inode->i_private);
> +}
> +
> +static int mhi_debugfs_channels_open(struct inode *inode, struct file *fp)
> +{
> +	return single_open(fp, mhi_debugfs_channels_show, inode->i_private);
> +}
> +
> +static int mhi_debugfs_votes_open(struct inode *inode, struct file *fp)
> +{
> +	return single_open(fp, mhi_debugfs_votes_show, inode->i_private);
> +}
> +
> +static int mhi_debugfs_regdump_open(struct inode *inode, struct file *fp)
> +{
> +	return single_open(fp, mhi_debugfs_regdump_show, inode->i_private);
> +}
> +
> +static int mhi_debugfs_device_vote_open(struct inode *inode, struct file *fp)
> +{
> +	return single_open(fp, mhi_debugfs_device_vote_show, inode->i_private);
> +}
> +
> +static int mhi_debugfs_timeout_ms_open(struct inode *inode, struct file *fp)
> +{
> +	return single_open(fp, mhi_debugfs_timeout_ms_show, inode->i_private);
> +}
> +
> +static const struct file_operations debugfs_states_fops = {
> +	.open = mhi_debugfs_states_open,
> +	.release = single_release,
> +	.read = seq_read,
> +};
> +
> +static const struct file_operations debugfs_events_fops = {
> +	.open = mhi_debugfs_events_open,
> +	.release = single_release,
> +	.read = seq_read,
> +};
> +
> +static const struct file_operations debugfs_channels_fops = {
> +	.open = mhi_debugfs_channels_open,
> +	.release = single_release,
> +	.read = seq_read,
> +};
> +
> +static const struct file_operations debugfs_votes_fops = {
> +	.open = mhi_debugfs_votes_open,
> +	.release = single_release,
> +	.read = seq_read,
> +};
> +
> +static const struct file_operations debugfs_regdump_fops = {
> +	.open = mhi_debugfs_regdump_open,
> +	.release = single_release,
> +	.read = seq_read,
> +};
> +
> +static const struct file_operations debugfs_device_vote_fops = {
> +	.open = mhi_debugfs_device_vote_open,
> +	.write = mhi_debugfs_device_vote_write,
> +	.release = single_release,
> +	.read = seq_read,
> +};
> +
> +static const struct file_operations debugfs_timeout_ms_fops = {
> +	.open = mhi_debugfs_timeout_ms_open,
> +	.write = mhi_debugfs_timeout_ms_write,
> +	.release = single_release,
> +	.read = seq_read,
> +};
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(debugfs_reset_fops, NULL,
> +			 mhi_debugfs_trigger_reset, "%llu\n");
> +
> +static struct dentry *mhi_debugfs_root;
> +
> +void mhi_create_debugfs(struct mhi_controller *mhi_cntrl)
> +{
> +	mhi_cntrl->debugfs_dentry =
> +			debugfs_create_dir(dev_name(mhi_cntrl->cntrl_dev),
> +					   mhi_debugfs_root);
> +
> +	debugfs_create_file("states", 0444, mhi_cntrl->debugfs_dentry,
> +			    mhi_cntrl, &debugfs_states_fops);
> +	debugfs_create_file("events", 0444, mhi_cntrl->debugfs_dentry,
> +			    mhi_cntrl, &debugfs_events_fops);
> +	debugfs_create_file("channels", 0444, mhi_cntrl->debugfs_dentry,
> +			    mhi_cntrl, &debugfs_channels_fops);
> +	debugfs_create_file("votes", 0444, mhi_cntrl->debugfs_dentry,
> +			    mhi_cntrl, &debugfs_votes_fops);
> +	debugfs_create_file("regdump", 0444, mhi_cntrl->debugfs_dentry,
> +			    mhi_cntrl, &debugfs_regdump_fops);
> +	debugfs_create_file("device_vote", 0644, mhi_cntrl->debugfs_dentry,
> +			    mhi_cntrl, &debugfs_device_vote_fops);
> +	debugfs_create_file("timeout_ms", 0644, mhi_cntrl->debugfs_dentry,
> +			    mhi_cntrl, &debugfs_timeout_ms_fops);
> +	debugfs_create_file("reset", 0444, mhi_cntrl->debugfs_dentry,
> +			    mhi_cntrl, &debugfs_reset_fops);
> +}
> +
> +void mhi_destroy_debugfs(struct mhi_controller *mhi_cntrl)
> +{
> +	debugfs_remove_recursive(mhi_cntrl->debugfs_dentry);
> +	mhi_cntrl->debugfs_dentry = NULL;
> +}
> +
> +void mhi_debugfs_init(void)
> +{
> +	mhi_debugfs_root = debugfs_create_dir(mhi_bus_type.name, NULL);
> +}
> +
> +void mhi_debugfs_exit(void)
> +{
> +	debugfs_remove_recursive(mhi_debugfs_root);
> +}
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index e2011ec..d2c0f6e 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -4,6 +4,7 @@
>   *
>   */
>  
> +#include <linux/debugfs.h>
>  #include <linux/device.h>
>  #include <linux/dma-direction.h>
>  #include <linux/dma-mapping.h>
> @@ -915,6 +916,8 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
>  
>  	mhi_cntrl->mhi_dev = mhi_dev;
>  
> +	mhi_create_debugfs(mhi_cntrl);
> +
>  	return 0;
>  
>  error_add_dev:
> @@ -937,6 +940,8 @@ void mhi_unregister_controller(struct mhi_controller *mhi_cntrl)
>  	struct mhi_chan *mhi_chan = mhi_cntrl->mhi_chan;
>  	unsigned int i;
>  
> +	mhi_destroy_debugfs(mhi_cntrl);
> +
>  	kfree(mhi_cntrl->mhi_cmd);
>  	kfree(mhi_cntrl->mhi_event);
>  
> @@ -1284,11 +1289,13 @@ struct bus_type mhi_bus_type = {
>  
>  static int __init mhi_init(void)
>  {
> +	mhi_debugfs_init();
>  	return bus_register(&mhi_bus_type);
>  }
>  
>  static void __exit mhi_exit(void)
>  {
> +	mhi_debugfs_exit();
>  	bus_unregister(&mhi_bus_type);
>  }
>  
> diff --git a/drivers/bus/mhi/core/internal.h b/drivers/bus/mhi/core/internal.h
> index 997c6e9..368c442 100644
> --- a/drivers/bus/mhi/core/internal.h
> +++ b/drivers/bus/mhi/core/internal.h
> @@ -570,6 +570,30 @@ struct mhi_chan {
>  /* Default MHI timeout */
>  #define MHI_TIMEOUT_MS (1000)
>  
> +/* debugfs related functions */
> +#ifdef CONFIG_MHI_BUS_DEBUG
> +void mhi_create_debugfs(struct mhi_controller *mhi_cntrl);
> +void mhi_destroy_debugfs(struct mhi_controller *mhi_cntrl);
> +void mhi_debugfs_init(void);
> +void mhi_debugfs_exit(void);
> +#else
> +static inline void mhi_create_debugfs(struct mhi_controller *mhi_cntrl)
> +{
> +}
> +
> +static inline void mhi_destroy_debugfs(struct mhi_controller *mhi_cntrl)
> +{
> +}
> +
> +static inline void mhi_debugfs_init(void)
> +{
> +}
> +
> +static inline void mhi_debugfs_exit(void)
> +{
> +}
> +#endif
> +
>  struct mhi_device *mhi_alloc_device(struct mhi_controller *mhi_cntrl);
>  
>  int mhi_destroy_device(struct device *dev, void *data);
> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> index 74c5cb1..c80d3553 100644
> --- a/drivers/bus/mhi/core/pm.c
> +++ b/drivers/bus/mhi/core/pm.c
> @@ -256,6 +256,7 @@ int mhi_pm_m0_transition(struct mhi_controller *mhi_cntrl)
>  		dev_err(dev, "Unable to transition to M0 state\n");
>  		return -EIO;
>  	}
> +	mhi_cntrl->M0++;

This change shouldn't be part of this patch.

Thanks,
Mani

>  
>  	/* Wake up the device */
>  	read_lock_bh(&mhi_cntrl->pm_lock);
> @@ -326,6 +327,8 @@ void mhi_pm_m1_transition(struct mhi_controller *mhi_cntrl)
>  		mhi_cntrl->dev_state = MHI_STATE_M2;
>  
>  		write_unlock_irq(&mhi_cntrl->pm_lock);
> +
> +		mhi_cntrl->M2++;
>  		wake_up_all(&mhi_cntrl->state_event);
>  
>  		/* If there are any pending resources, exit M2 immediately */
> @@ -362,6 +365,7 @@ int mhi_pm_m3_transition(struct mhi_controller *mhi_cntrl)
>  		return -EIO;
>  	}
>  
> +	mhi_cntrl->M3++;
>  	wake_up_all(&mhi_cntrl->state_event);
>  
>  	return 0;
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index 7ed785e..b1e8b4f 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -288,6 +288,7 @@ struct mhi_controller_config {
>   * @cntrl_dev: Pointer to the struct device of physical bus acting as the MHI
>   *            controller (required)
>   * @mhi_dev: MHI device instance for the controller
> + * @debugfs_dentry: MHI controller debugfs directory
>   * @regs: Base address of MHI MMIO register space (required)
>   * @bhi: Points to base of MHI BHI register space
>   * @bhie: Points to base of MHI BHIe register space
> @@ -326,6 +327,7 @@ struct mhi_controller_config {
>   * @dev_state: MHI device state
>   * @dev_wake: Device wakeup count
>   * @pending_pkts: Pending packets for the controller
> + * @M0, M2, M3, M3_fast: Counters to track number of device MHI state changes
>   * @transition_list: List of MHI state transitions
>   * @transition_lock: Lock for protecting MHI state transition list
>   * @wlock: Lock for protecting device wakeup
> @@ -364,6 +366,7 @@ struct mhi_controller_config {
>  struct mhi_controller {
>  	struct device *cntrl_dev;
>  	struct mhi_device *mhi_dev;
> +	struct dentry *debugfs_dentry;
>  	void __iomem *regs;
>  	void __iomem *bhi;
>  	void __iomem *bhie;
> @@ -405,6 +408,7 @@ struct mhi_controller {
>  	enum mhi_state dev_state;
>  	atomic_t dev_wake;
>  	atomic_t pending_pkts;
> +	u32 M0, M2, M3, M3_fast;
>  	struct list_head transition_list;
>  	spinlock_t transition_lock;
>  	spinlock_t wlock;
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

Re: [PATCH v4 7/9] bus: mhi: core: Introduce debugfs entries and counters for MHI

[bbhatt]

On 2020-07-04 08:41, Manivannan Sadhasivam wrote:
> On Mon, Jun 29, 2020 at 09:39:40AM -0700, Bhaumik Bhatt wrote:
>> Introduce debugfs entries to show state, register, channel, and event
>> ring information. Add MHI state counters to keep track of the state
>> changes on the device. Also, allow the host to trigger a device reset,
>> issue votes, and change the MHI timeout to help in debug.
>> 
>> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
>> ---
>>  drivers/bus/mhi/Kconfig         |   8 +
>>  drivers/bus/mhi/core/Makefile   |   5 +-
>>  drivers/bus/mhi/core/debugfs.c  | 444 
>> ++++++++++++++++++++++++++++++++++++++++
>>  drivers/bus/mhi/core/init.c     |   7 +
>>  drivers/bus/mhi/core/internal.h |  24 +++
>>  drivers/bus/mhi/core/pm.c       |   4 +
>>  include/linux/mhi.h             |   4 +
>>  7 files changed, 493 insertions(+), 3 deletions(-)
>>  create mode 100644 drivers/bus/mhi/core/debugfs.c
>> 
>> diff --git a/drivers/bus/mhi/Kconfig b/drivers/bus/mhi/Kconfig
>> index a8bd9bd..6a217ff 100644
>> --- a/drivers/bus/mhi/Kconfig
>> +++ b/drivers/bus/mhi/Kconfig
>> @@ -12,3 +12,11 @@ config MHI_BUS
>>  	 communication protocol used by the host processors to control
>>  	 and communicate with modem devices over a high speed peripheral
>>  	 bus or shared memory.
>> +
>> +config MHI_BUS_DEBUG
>> +	bool "Debugfs support for the MHI bus"
>> +	depends on MHI_BUS && DEBUG_FS
>> +	help
>> +	 Enable debugfs support for use with the MHI transport. Allows
>> +	 reading and/or modifying some values within the MHI controller
>> +	 for debug and test purposes.
>> diff --git a/drivers/bus/mhi/core/Makefile 
>> b/drivers/bus/mhi/core/Makefile
>> index 66e2700..460a548 100644
>> --- a/drivers/bus/mhi/core/Makefile
>> +++ b/drivers/bus/mhi/core/Makefile
>> @@ -1,3 +1,2 @@
>> -obj-$(CONFIG_MHI_BUS) := mhi.o
>> -
>> -mhi-y := init.o main.o pm.o boot.o
>> +obj-$(CONFIG_MHI_BUS) := init.o main.o pm.o boot.o
>> +obj-$(CONFIG_MHI_BUS_DEBUG) += debugfs.o
>> diff --git a/drivers/bus/mhi/core/debugfs.c 
>> b/drivers/bus/mhi/core/debugfs.c
>> new file mode 100644
>> index 0000000..266cbf0
>> --- /dev/null
>> +++ b/drivers/bus/mhi/core/debugfs.c
>> @@ -0,0 +1,444 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
>> + *
>> + */
>> +
>> +#include <linux/debugfs.h>
>> +#include <linux/device.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/list.h>
>> +#include <linux/mhi.h>
>> +#include "internal.h"
>> +
>> +static int mhi_debugfs_states_show(struct seq_file *m, void *d)
>> +{
>> +	struct mhi_controller *mhi_cntrl = m->private;
>> +
>> +	/* states */
>> +	seq_printf(m, "PM state:%s Device:%s MHI state:%s EE:%s wake:%s\n",
>> +		   to_mhi_pm_state_str(mhi_cntrl->pm_state),
>> +		   mhi_is_active(mhi_cntrl) ? "Active" : "Inactive",
>> +		   TO_MHI_STATE_STR(mhi_cntrl->dev_state),
>> +		   TO_MHI_EXEC_STR(mhi_cntrl->ee),
>> +		   mhi_cntrl->wake_set ? "true" : "false");
> 
> Nit: Always use a space after ":".
> 
Sure.
>> +
>> +	/* counters */
>> +	seq_printf(m, "M0:%u M2:%u M3:%u M3_Fast:%u", mhi_cntrl->M0,
>> +		   mhi_cntrl->M2, mhi_cntrl->M3, mhi_cntrl->M3_fast);
>> +
>> +	seq_printf(m, " device wake:%u pending packets:%u\n",
>> +		   atomic_read(&mhi_cntrl->dev_wake),
>> +		   atomic_read(&mhi_cntrl->pending_pkts));
>> +
>> +	return 0;
>> +}
>> +
>> +static int mhi_debugfs_events_show(struct seq_file *m, void *d)
>> +{
>> +	struct mhi_controller *mhi_cntrl = m->private;
>> +	struct mhi_event *mhi_event;
>> +	struct mhi_event_ctxt *er_ctxt;
>> +	int i;
>> +
>> +	if (!mhi_is_active(mhi_cntrl)) {
>> +		seq_puts(m, "Device not ready\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	er_ctxt = mhi_cntrl->mhi_ctxt->er_ctxt;
>> +	mhi_event = mhi_cntrl->mhi_event;
>> +	for (i = 0; i < mhi_cntrl->total_ev_rings;
>> +						i++, er_ctxt++, mhi_event++) {
>> +		struct mhi_ring *ring = &mhi_event->ring;
>> +
>> +		if (mhi_event->offload_ev) {
>> +			seq_printf(m, "Index:%d is an offload event ring\n", i);
>> +			continue;
>> +		}
>> +
>> +		seq_printf(m, "Index:%d intmod count:%lu time:%lu",
>> +			   i, (er_ctxt->intmod & EV_CTX_INTMODC_MASK) >>
>> +			   EV_CTX_INTMODC_SHIFT,
>> +			   (er_ctxt->intmod & EV_CTX_INTMODT_MASK) >>
>> +			   EV_CTX_INTMODT_SHIFT);
>> +
>> +		seq_printf(m, " base:0x%0llx len:0x%llx", er_ctxt->rbase,
>> +			   er_ctxt->rlen);
>> +
>> +		seq_printf(m,
>> +			   " rp:0x%llx wp:0x%llx local rp:0x%llx db:0x%llx\n",
>> +			   er_ctxt->rp, er_ctxt->wp, (u64)ring->rp,
>> +			   mhi_event->db_cfg.db_val);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int mhi_debugfs_channels_show(struct seq_file *m, void *d)
>> +{
>> +	struct mhi_controller *mhi_cntrl = m->private;
>> +	struct mhi_chan *mhi_chan;
>> +	struct mhi_chan_ctxt *chan_ctxt;
>> +	int i;
>> +
>> +	if (!mhi_is_active(mhi_cntrl)) {
>> +		seq_puts(m, "Device not ready\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	mhi_chan = mhi_cntrl->mhi_chan;
>> +	chan_ctxt = mhi_cntrl->mhi_ctxt->chan_ctxt;
>> +	for (i = 0; i < mhi_cntrl->max_chan; i++, chan_ctxt++, mhi_chan++) {
>> +		struct mhi_ring *ring = &mhi_chan->tre_ring;
>> +
>> +		if (mhi_chan->offload_ch) {
>> +			seq_printf(m, "%s(%u) is an offload channel\n",
>> +				   mhi_chan->name, mhi_chan->chan);
>> +			continue;
>> +		}
>> +
>> +		if (!mhi_chan->mhi_dev)
>> +			continue;
>> +
>> +		seq_printf(m,
>> +			   "%s(%u) state:0x%lx brstmode:0x%lx pollcfg:0x%lx",
>> +			   mhi_chan->name, mhi_chan->chan, (chan_ctxt->chcfg &
>> +			   CHAN_CTX_CHSTATE_MASK) >> CHAN_CTX_CHSTATE_SHIFT,
>> +			   (chan_ctxt->chcfg & CHAN_CTX_BRSTMODE_MASK) >>
>> +			   CHAN_CTX_BRSTMODE_SHIFT, (chan_ctxt->chcfg &
>> +			   CHAN_CTX_POLLCFG_MASK) >> CHAN_CTX_POLLCFG_SHIFT);
>> +
>> +		seq_printf(m, " type:0x%x event ring:%u", chan_ctxt->chtype,
>> +			   chan_ctxt->erindex);
>> +
>> +		seq_printf(m, " base:0x%llx len:0x%llx wp:0x%llx",
>> +			   chan_ctxt->rbase, chan_ctxt->rlen, chan_ctxt->wp);
>> +
>> +		seq_printf(m, " local rp:0x%llx local wp:0x%llx db:0x%llx\n",
>> +			   (u64)ring->rp, (u64)ring->wp,
>> +			   mhi_chan->db_cfg.db_val);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int mhi_device_votes_show(struct device *dev, void *data)
>> +{
>> +	struct mhi_device *mhi_dev;
>> +
>> +	if (dev->bus != &mhi_bus_type)
>> +		return 0;
>> +
>> +	mhi_dev = to_mhi_device(dev);
>> +
>> +	seq_printf((struct seq_file *)data, "%s: %u\n",
>> +		   mhi_dev->name, mhi_dev->dev_wake);
>> +
>> +	return 0;
>> +}
>> +
>> +static int mhi_debugfs_votes_show(struct seq_file *m, void *d)
>> +{
>> +	struct mhi_controller *mhi_cntrl = m->private;
>> +
>> +	if (!mhi_is_active(mhi_cntrl)) {
>> +		seq_puts(m, "Device not ready\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	device_for_each_child(mhi_cntrl->cntrl_dev, m, 
>> mhi_device_votes_show);
>> +
>> +	return 0;
>> +}
>> +
>> +static int mhi_debugfs_regdump_show(struct seq_file *m, void *d)
>> +{
>> +	struct mhi_controller *mhi_cntrl = m->private;
>> +	enum mhi_state state;
>> +	enum mhi_ee_type ee;
>> +	int i, ret = -EIO;
>> +	u32 val;
>> +	void __iomem *mhi_base = mhi_cntrl->regs;
>> +	void __iomem *bhi_base = mhi_cntrl->bhi;
>> +	void __iomem *bhie_base = mhi_cntrl->bhie;
>> +	void __iomem *wake_db = mhi_cntrl->wake_db;
>> +	struct {
>> +		const char *name;
>> +		int offset;
>> +		void __iomem *base;
>> +	} debug_regs[] = {
>> +		{ "MHI_CTRL", MHICTRL, mhi_base},
>> +		{ "MHI_STATUS", MHISTATUS, mhi_base},
>> +		{ "MHI_WAKE_DB", 0, wake_db},
>> +		{ "BHI_EXECENV", BHI_EXECENV, bhi_base},
>> +		{ "BHI_STATUS", BHI_STATUS, bhi_base},
>> +		{ "BHI_ERRCODE", BHI_ERRCODE, bhi_base},
>> +		{ "BHI_ERRDBG1", BHI_ERRDBG1, bhi_base},
>> +		{ "BHI_ERRDBG2", BHI_ERRDBG2, bhi_base},
>> +		{ "BHI_ERRDBG3", BHI_ERRDBG3, bhi_base},
>> +		{ "BHIE_TXVEC_DB", BHIE_TXVECDB_OFFS, bhie_base},
>> +		{ "BHIE_TXVEC_STATUS", BHIE_TXVECSTATUS_OFFS, bhie_base},
>> +		{ "BHIE_RXVEC_DB", BHIE_RXVECDB_OFFS, bhie_base},
>> +		{ "BHIE_RXVEC_STATUS", BHIE_RXVECSTATUS_OFFS, bhie_base},
>> +		{ NULL },
>> +	};
>> +
>> +	if (!MHI_REG_ACCESS_VALID(mhi_cntrl->pm_state))
>> +		return ret;
>> +
>> +	seq_printf(m, "Host PM state:%s Device state:%s EE:%s\n",
>> +		   to_mhi_pm_state_str(mhi_cntrl->pm_state),
>> +		   TO_MHI_STATE_STR(mhi_cntrl->dev_state),
>> +		   TO_MHI_EXEC_STR(mhi_cntrl->ee));
>> +
>> +	state = mhi_get_mhi_state(mhi_cntrl);
>> +	ee = mhi_get_exec_env(mhi_cntrl);
>> +	seq_printf(m, "Device EE:%s state:%s\n", TO_MHI_EXEC_STR(ee),
>> +		   TO_MHI_STATE_STR(state));
>> +
>> +	for (i = 0; debug_regs[i].name; i++) {
>> +		if (!debug_regs[i].base)
>> +			continue;
>> +		ret = mhi_read_reg(mhi_cntrl, debug_regs[i].base,
>> +				   debug_regs[i].offset, &val);
>> +		if (ret)
>> +			continue;
>> +
>> +		seq_printf(m, "%s:0x%x\n", debug_regs[i].name, val);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int mhi_debugfs_device_vote_show(struct seq_file *m, void *d)
>> +{
> 
> The term 'vote' is confusing here. Can you come up with something which 
> portrays
> device power mode here?
> 
I was hoping vote would be appropriate as we would like to "vote" for 
the device to be
in active state by doing a "get" on it. If a vote is present, it would 
mean that the
device is in its active/M0 power state which can be seen on the "states" 
debugfs entry.

Let me know what you think. I will consult Hemant on this as well once 
more.
>> +	struct mhi_controller *mhi_cntrl = m->private;
>> +	struct mhi_device *mhi_dev = mhi_cntrl->mhi_dev;
>> +
>> +	if (!mhi_is_active(mhi_cntrl)) {
>> +		seq_puts(m, "Device not ready\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	seq_printf(m,
>> +		   "Votes: %d\n%s\n", mhi_dev->dev_wake,
>> +		   "Usage: echo get/put > device_vote for vote/unvote");
>> +
>> +	return 0;
>> +}
>> +
>> +static ssize_t mhi_debugfs_device_vote_write(struct file *file,
>> +					     const char __user *ubuf,
>> +					     size_t count, loff_t *ppos)
>> +{
>> +	struct seq_file	*m = file->private_data;
>> +	struct mhi_controller *mhi_cntrl = m->private;
>> +	struct mhi_device *mhi_dev = mhi_cntrl->mhi_dev;
>> +	char buf[32];
>> +	int ret = -EINVAL;
>> +
>> +	if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, 
>> count)))
>> +		return -EFAULT;
>> +
>> +	if (!strncmp(buf, "get", 3)) {
> 
> Hmm, but the buffer size is 32?
> 
Yes, I referred to some other driver while writing this and buffer size 
was chosen
to be more than enough for comparison purpose. Any other way to handle 
this?
>> +		ret = mhi_device_get_sync(mhi_dev);
>> +	} else if (!strncmp(buf, "put", 3)) {
>> +		mhi_device_put(mhi_dev);
>> +		ret = 0;
>> +	}
>> +
>> +	return ret ? ret : count;
>> +}
>> +
>> +static int mhi_debugfs_timeout_ms_show(struct seq_file *m, void *d)
>> +{
>> +	struct mhi_controller *mhi_cntrl = m->private;
>> +
>> +	seq_printf(m, "%u ms\n", mhi_cntrl->timeout_ms);
>> +
>> +	return 0;
>> +}
>> +
>> +static ssize_t mhi_debugfs_timeout_ms_write(struct file *file,
>> +					    const char __user *ubuf,
>> +					    size_t count, loff_t *ppos)
>> +{
>> +	struct seq_file	*m = file->private_data;
>> +	struct mhi_controller *mhi_cntrl = m->private;
>> +	u32 timeout_ms;
>> +
>> +	if (kstrtou32_from_user(ubuf, count, 0, &timeout_ms))
>> +		return -EINVAL;
>> +
>> +	mhi_cntrl->timeout_ms = timeout_ms;
>> +
>> +	return count;
>> +}
>> +
>> +static int mhi_debugfs_trigger_reset(void *data, u64 val)
> 
> Triggering reset on host or device?
> 
Device. Will update the function and file name approprirately.
>> +{
>> +	struct mhi_controller *mhi_cntrl = data;
>> +	struct mhi_device *mhi_dev = mhi_cntrl->mhi_dev;
>> +	struct device *dev = &mhi_dev->dev;
>> +	enum mhi_pm_state cur_state;
>> +	int ret = -EIO;
>> +
>> +	if (!mhi_is_active(mhi_cntrl))
>> +		return -ENODEV;
>> +
>> +	if (!val)
>> +		return -EINVAL;
>> +
>> +	ret = mhi_device_get_sync(mhi_dev);
>> +	if (ret) {
>> +		dev_err(dev, "Device did not enter M0 state, MHI:%s, PM:%s\n",
>> +			TO_MHI_STATE_STR(mhi_cntrl->dev_state),
>> +			to_mhi_pm_state_str(mhi_cntrl->pm_state));
>> +		return ret;
>> +	}
>> +
>> +	if (mhi_cntrl->rddm_image) {
>> +		ret = mhi_force_rddm_mode(mhi_cntrl);
>> +		goto exit_mhi_trigger_reset;
>> +	}
>> +
>> +	write_lock_irq(&mhi_cntrl->pm_lock);
>> +	cur_state = mhi_tryset_pm_state(mhi_cntrl, MHI_PM_SYS_ERR_DETECT);
> 
> Looks like in host but how does this resets it?
> 
This is the bus internally moving the PM state to "sys error detect" 
state so we can
check if our state machine allows this move and force a sys error to the 
device.
>> +	write_unlock_irq(&mhi_cntrl->pm_lock);
>> +
>> +	if (cur_state != MHI_PM_SYS_ERR_DETECT)
>> +		goto exit_mhi_trigger_reset;
> 
> So this condition is also a success? PS: you're not setting ret here.
> 
Thanks for pointing out. It is not a success condition and should return 
-EPERM.
I will fix it.
>> +
>> +	mhi_pm_sys_err_handler(mhi_cntrl);
>> +	ret = 0;
>> +
>> +exit_mhi_trigger_reset:
>> +	mhi_device_put(mhi_dev);
>> +
>> +	return ret;
>> +}
>> +
>> +static int mhi_debugfs_states_open(struct inode *inode, struct file 
>> *fp)
>> +{
>> +	return single_open(fp, mhi_debugfs_states_show, inode->i_private);
>> +}
>> +
>> +static int mhi_debugfs_events_open(struct inode *inode, struct file 
>> *fp)
>> +{
>> +	return single_open(fp, mhi_debugfs_events_show, inode->i_private);
>> +}
>> +
>> +static int mhi_debugfs_channels_open(struct inode *inode, struct file 
>> *fp)
>> +{
>> +	return single_open(fp, mhi_debugfs_channels_show, inode->i_private);
>> +}
>> +
>> +static int mhi_debugfs_votes_open(struct inode *inode, struct file 
>> *fp)
>> +{
>> +	return single_open(fp, mhi_debugfs_votes_show, inode->i_private);
>> +}
>> +
>> +static int mhi_debugfs_regdump_open(struct inode *inode, struct file 
>> *fp)
>> +{
>> +	return single_open(fp, mhi_debugfs_regdump_show, inode->i_private);
>> +}
>> +
>> +static int mhi_debugfs_device_vote_open(struct inode *inode, struct 
>> file *fp)
>> +{
>> +	return single_open(fp, mhi_debugfs_device_vote_show, 
>> inode->i_private);
>> +}
>> +
>> +static int mhi_debugfs_timeout_ms_open(struct inode *inode, struct 
>> file *fp)
>> +{
>> +	return single_open(fp, mhi_debugfs_timeout_ms_show, 
>> inode->i_private);
>> +}
>> +
>> +static const struct file_operations debugfs_states_fops = {
>> +	.open = mhi_debugfs_states_open,
>> +	.release = single_release,
>> +	.read = seq_read,
>> +};
>> +
>> +static const struct file_operations debugfs_events_fops = {
>> +	.open = mhi_debugfs_events_open,
>> +	.release = single_release,
>> +	.read = seq_read,
>> +};
>> +
>> +static const struct file_operations debugfs_channels_fops = {
>> +	.open = mhi_debugfs_channels_open,
>> +	.release = single_release,
>> +	.read = seq_read,
>> +};
>> +
>> +static const struct file_operations debugfs_votes_fops = {
>> +	.open = mhi_debugfs_votes_open,
>> +	.release = single_release,
>> +	.read = seq_read,
>> +};
>> +
>> +static const struct file_operations debugfs_regdump_fops = {
>> +	.open = mhi_debugfs_regdump_open,
>> +	.release = single_release,
>> +	.read = seq_read,
>> +};
>> +
>> +static const struct file_operations debugfs_device_vote_fops = {
>> +	.open = mhi_debugfs_device_vote_open,
>> +	.write = mhi_debugfs_device_vote_write,
>> +	.release = single_release,
>> +	.read = seq_read,
>> +};
>> +
>> +static const struct file_operations debugfs_timeout_ms_fops = {
>> +	.open = mhi_debugfs_timeout_ms_open,
>> +	.write = mhi_debugfs_timeout_ms_write,
>> +	.release = single_release,
>> +	.read = seq_read,
>> +};
>> +
>> +DEFINE_DEBUGFS_ATTRIBUTE(debugfs_reset_fops, NULL,
>> +			 mhi_debugfs_trigger_reset, "%llu\n");
>> +
>> +static struct dentry *mhi_debugfs_root;
>> +
>> +void mhi_create_debugfs(struct mhi_controller *mhi_cntrl)
>> +{
>> +	mhi_cntrl->debugfs_dentry =
>> +			debugfs_create_dir(dev_name(mhi_cntrl->cntrl_dev),
>> +					   mhi_debugfs_root);
>> +
>> +	debugfs_create_file("states", 0444, mhi_cntrl->debugfs_dentry,
>> +			    mhi_cntrl, &debugfs_states_fops);
>> +	debugfs_create_file("events", 0444, mhi_cntrl->debugfs_dentry,
>> +			    mhi_cntrl, &debugfs_events_fops);
>> +	debugfs_create_file("channels", 0444, mhi_cntrl->debugfs_dentry,
>> +			    mhi_cntrl, &debugfs_channels_fops);
>> +	debugfs_create_file("votes", 0444, mhi_cntrl->debugfs_dentry,
>> +			    mhi_cntrl, &debugfs_votes_fops);
>> +	debugfs_create_file("regdump", 0444, mhi_cntrl->debugfs_dentry,
>> +			    mhi_cntrl, &debugfs_regdump_fops);
>> +	debugfs_create_file("device_vote", 0644, mhi_cntrl->debugfs_dentry,
>> +			    mhi_cntrl, &debugfs_device_vote_fops);
>> +	debugfs_create_file("timeout_ms", 0644, mhi_cntrl->debugfs_dentry,
>> +			    mhi_cntrl, &debugfs_timeout_ms_fops);
>> +	debugfs_create_file("reset", 0444, mhi_cntrl->debugfs_dentry,
>> +			    mhi_cntrl, &debugfs_reset_fops);
>> +}
>> +
>> +void mhi_destroy_debugfs(struct mhi_controller *mhi_cntrl)
>> +{
>> +	debugfs_remove_recursive(mhi_cntrl->debugfs_dentry);
>> +	mhi_cntrl->debugfs_dentry = NULL;
>> +}
>> +
>> +void mhi_debugfs_init(void)
>> +{
>> +	mhi_debugfs_root = debugfs_create_dir(mhi_bus_type.name, NULL);
>> +}
>> +
>> +void mhi_debugfs_exit(void)
>> +{
>> +	debugfs_remove_recursive(mhi_debugfs_root);
>> +}
>> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
>> index e2011ec..d2c0f6e 100644
>> --- a/drivers/bus/mhi/core/init.c
>> +++ b/drivers/bus/mhi/core/init.c
>> @@ -4,6 +4,7 @@
>>   *
>>   */
>> 
>> +#include <linux/debugfs.h>
>>  #include <linux/device.h>
>>  #include <linux/dma-direction.h>
>>  #include <linux/dma-mapping.h>
>> @@ -915,6 +916,8 @@ int mhi_register_controller(struct mhi_controller 
>> *mhi_cntrl,
>> 
>>  	mhi_cntrl->mhi_dev = mhi_dev;
>> 
>> +	mhi_create_debugfs(mhi_cntrl);
>> +
>>  	return 0;
>> 
>>  error_add_dev:
>> @@ -937,6 +940,8 @@ void mhi_unregister_controller(struct 
>> mhi_controller *mhi_cntrl)
>>  	struct mhi_chan *mhi_chan = mhi_cntrl->mhi_chan;
>>  	unsigned int i;
>> 
>> +	mhi_destroy_debugfs(mhi_cntrl);
>> +
>>  	kfree(mhi_cntrl->mhi_cmd);
>>  	kfree(mhi_cntrl->mhi_event);
>> 
>> @@ -1284,11 +1289,13 @@ struct bus_type mhi_bus_type = {
>> 
>>  static int __init mhi_init(void)
>>  {
>> +	mhi_debugfs_init();
>>  	return bus_register(&mhi_bus_type);
>>  }
>> 
>>  static void __exit mhi_exit(void)
>>  {
>> +	mhi_debugfs_exit();
>>  	bus_unregister(&mhi_bus_type);
>>  }
>> 
>> diff --git a/drivers/bus/mhi/core/internal.h 
>> b/drivers/bus/mhi/core/internal.h
>> index 997c6e9..368c442 100644
>> --- a/drivers/bus/mhi/core/internal.h
>> +++ b/drivers/bus/mhi/core/internal.h
>> @@ -570,6 +570,30 @@ struct mhi_chan {
>>  /* Default MHI timeout */
>>  #define MHI_TIMEOUT_MS (1000)
>> 
>> +/* debugfs related functions */
>> +#ifdef CONFIG_MHI_BUS_DEBUG
>> +void mhi_create_debugfs(struct mhi_controller *mhi_cntrl);
>> +void mhi_destroy_debugfs(struct mhi_controller *mhi_cntrl);
>> +void mhi_debugfs_init(void);
>> +void mhi_debugfs_exit(void);
>> +#else
>> +static inline void mhi_create_debugfs(struct mhi_controller 
>> *mhi_cntrl)
>> +{
>> +}
>> +
>> +static inline void mhi_destroy_debugfs(struct mhi_controller 
>> *mhi_cntrl)
>> +{
>> +}
>> +
>> +static inline void mhi_debugfs_init(void)
>> +{
>> +}
>> +
>> +static inline void mhi_debugfs_exit(void)
>> +{
>> +}
>> +#endif
>> +
>>  struct mhi_device *mhi_alloc_device(struct mhi_controller 
>> *mhi_cntrl);
>> 
>>  int mhi_destroy_device(struct device *dev, void *data);
>> diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
>> index 74c5cb1..c80d3553 100644
>> --- a/drivers/bus/mhi/core/pm.c
>> +++ b/drivers/bus/mhi/core/pm.c
>> @@ -256,6 +256,7 @@ int mhi_pm_m0_transition(struct mhi_controller 
>> *mhi_cntrl)
>>  		dev_err(dev, "Unable to transition to M0 state\n");
>>  		return -EIO;
>>  	}
>> +	mhi_cntrl->M0++;
> 
> This change shouldn't be part of this patch.
> 
> Thanks,
> Mani
> 
I remember you said this last time as well but I've introduced counters 
and
used them in the same patch. It is mentioned in the commit subject and 
text.
If you still have concerns, please let me know.

Thanks,
Bhaumik
>> 
>>  	/* Wake up the device */
>>  	read_lock_bh(&mhi_cntrl->pm_lock);
>> @@ -326,6 +327,8 @@ void mhi_pm_m1_transition(struct mhi_controller 
>> *mhi_cntrl)
>>  		mhi_cntrl->dev_state = MHI_STATE_M2;
>> 
>>  		write_unlock_irq(&mhi_cntrl->pm_lock);
>> +
>> +		mhi_cntrl->M2++;
>>  		wake_up_all(&mhi_cntrl->state_event);
>> 
>>  		/* If there are any pending resources, exit M2 immediately */
>> @@ -362,6 +365,7 @@ int mhi_pm_m3_transition(struct mhi_controller 
>> *mhi_cntrl)
>>  		return -EIO;
>>  	}
>> 
>> +	mhi_cntrl->M3++;
>>  	wake_up_all(&mhi_cntrl->state_event);
>> 
>>  	return 0;
>> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
>> index 7ed785e..b1e8b4f 100644
>> --- a/include/linux/mhi.h
>> +++ b/include/linux/mhi.h
>> @@ -288,6 +288,7 @@ struct mhi_controller_config {
>>   * @cntrl_dev: Pointer to the struct device of physical bus acting as 
>> the MHI
>>   *            controller (required)
>>   * @mhi_dev: MHI device instance for the controller
>> + * @debugfs_dentry: MHI controller debugfs directory
>>   * @regs: Base address of MHI MMIO register space (required)
>>   * @bhi: Points to base of MHI BHI register space
>>   * @bhie: Points to base of MHI BHIe register space
>> @@ -326,6 +327,7 @@ struct mhi_controller_config {
>>   * @dev_state: MHI device state
>>   * @dev_wake: Device wakeup count
>>   * @pending_pkts: Pending packets for the controller
>> + * @M0, M2, M3, M3_fast: Counters to track number of device MHI state 
>> changes
>>   * @transition_list: List of MHI state transitions
>>   * @transition_lock: Lock for protecting MHI state transition list
>>   * @wlock: Lock for protecting device wakeup
>> @@ -364,6 +366,7 @@ struct mhi_controller_config {
>>  struct mhi_controller {
>>  	struct device *cntrl_dev;
>>  	struct mhi_device *mhi_dev;
>> +	struct dentry *debugfs_dentry;
>>  	void __iomem *regs;
>>  	void __iomem *bhi;
>>  	void __iomem *bhie;
>> @@ -405,6 +408,7 @@ struct mhi_controller {
>>  	enum mhi_state dev_state;
>>  	atomic_t dev_wake;
>>  	atomic_t pending_pkts;
>> +	u32 M0, M2, M3, M3_fast;
>>  	struct list_head transition_list;
>>  	spinlock_t transition_lock;
>>  	spinlock_t wlock;
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project
>> 

Re: [PATCH v4 7/9] bus: mhi: core: Introduce debugfs entries and counters for MHI

[Manivannan Sadhasivam]

On Thu, Jul 09, 2020 at 12:33:02PM -0700, bbhatt@codeaurora.org wrote:
> On 2020-07-04 08:41, Manivannan Sadhasivam wrote:
> > On Mon, Jun 29, 2020 at 09:39:40AM -0700, Bhaumik Bhatt wrote:
> > > Introduce debugfs entries to show state, register, channel, and event
> > > ring information. Add MHI state counters to keep track of the state
> > > changes on the device. Also, allow the host to trigger a device reset,
> > > issue votes, and change the MHI timeout to help in debug.
> > > 
> > > Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> > > ---
> > >  drivers/bus/mhi/Kconfig         |   8 +
> > >  drivers/bus/mhi/core/Makefile   |   5 +-
> > >  drivers/bus/mhi/core/debugfs.c  | 444
> > > ++++++++++++++++++++++++++++++++++++++++
> > >  drivers/bus/mhi/core/init.c     |   7 +
> > >  drivers/bus/mhi/core/internal.h |  24 +++
> > >  drivers/bus/mhi/core/pm.c       |   4 +
> > >  include/linux/mhi.h             |   4 +
> > >  7 files changed, 493 insertions(+), 3 deletions(-)
> > >  create mode 100644 drivers/bus/mhi/core/debugfs.c
> > > 
> > > diff --git a/drivers/bus/mhi/Kconfig b/drivers/bus/mhi/Kconfig
> > > index a8bd9bd..6a217ff 100644
> > > --- a/drivers/bus/mhi/Kconfig
> > > +++ b/drivers/bus/mhi/Kconfig
> > > @@ -12,3 +12,11 @@ config MHI_BUS
> > >  	 communication protocol used by the host processors to control
> > >  	 and communicate with modem devices over a high speed peripheral
> > >  	 bus or shared memory.
> > > +
> > > +config MHI_BUS_DEBUG
> > > +	bool "Debugfs support for the MHI bus"
> > > +	depends on MHI_BUS && DEBUG_FS
> > > +	help
> > > +	 Enable debugfs support for use with the MHI transport. Allows
> > > +	 reading and/or modifying some values within the MHI controller
> > > +	 for debug and test purposes.
> > > diff --git a/drivers/bus/mhi/core/Makefile
> > > b/drivers/bus/mhi/core/Makefile
> > > index 66e2700..460a548 100644
> > > --- a/drivers/bus/mhi/core/Makefile
> > > +++ b/drivers/bus/mhi/core/Makefile
> > > @@ -1,3 +1,2 @@
> > > -obj-$(CONFIG_MHI_BUS) := mhi.o
> > > -
> > > -mhi-y := init.o main.o pm.o boot.o
> > > +obj-$(CONFIG_MHI_BUS) := init.o main.o pm.o boot.o
> > > +obj-$(CONFIG_MHI_BUS_DEBUG) += debugfs.o
> > > diff --git a/drivers/bus/mhi/core/debugfs.c
> > > b/drivers/bus/mhi/core/debugfs.c
> > > new file mode 100644
> > > index 0000000..266cbf0
> > > --- /dev/null
> > > +++ b/drivers/bus/mhi/core/debugfs.c
> > > @@ -0,0 +1,444 @@
> > > +static int mhi_debugfs_device_vote_show(struct seq_file *m, void *d)
> > > +{

[...]

> > 
> > The term 'vote' is confusing here. Can you come up with something which
> > portrays
> > device power mode here?
> > 
> I was hoping vote would be appropriate as we would like to "vote" for the
> device to be
> in active state by doing a "get" on it. If a vote is present, it would mean
> that the
> device is in its active/M0 power state which can be seen on the "states"
> debugfs entry.
> 
> Let me know what you think. I will consult Hemant on this as well once more.

If you want to go with the term 'vote', then please add a prefix like power.
This makes it more explicit. Simply saying 'device vote' doesn't mean power
state.

> > > +	struct mhi_controller *mhi_cntrl = m->private;
> > > +	struct mhi_device *mhi_dev = mhi_cntrl->mhi_dev;
> > > +
> > > +	if (!mhi_is_active(mhi_cntrl)) {
> > > +		seq_puts(m, "Device not ready\n");
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	seq_printf(m,
> > > +		   "Votes: %d\n%s\n", mhi_dev->dev_wake,
> > > +		   "Usage: echo get/put > device_vote for vote/unvote");
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static ssize_t mhi_debugfs_device_vote_write(struct file *file,
> > > +					     const char __user *ubuf,
> > > +					     size_t count, loff_t *ppos)
> > > +{
> > > +	struct seq_file	*m = file->private_data;
> > > +	struct mhi_controller *mhi_cntrl = m->private;
> > > +	struct mhi_device *mhi_dev = mhi_cntrl->mhi_dev;
> > > +	char buf[32];
> > > +	int ret = -EINVAL;
> > > +
> > > +	if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1,
> > > count)))
> > > +		return -EFAULT;
> > > +
> > > +	if (!strncmp(buf, "get", 3)) {
> > 
> > Hmm, but the buffer size is 32?
> > 
> Yes, I referred to some other driver while writing this and buffer size was
> chosen
> to be more than enough for comparison purpose. Any other way to handle this?

AFAIK the requirement for the src buffer is to be 16byte aligned so that the
load and store won't cross the cache boundary. So just use 16 in that case.

> > > +		ret = mhi_device_get_sync(mhi_dev);
> > > +	} else if (!strncmp(buf, "put", 3)) {
> > > +		mhi_device_put(mhi_dev);
> > > +		ret = 0;
> > > +	}
> > > +
> > > +	return ret ? ret : count;
> > > +}
> > > +
> > > +static int mhi_debugfs_timeout_ms_show(struct seq_file *m, void *d)
> > > +{
> > > +	struct mhi_controller *mhi_cntrl = m->private;
> > > +
> > > +	seq_printf(m, "%u ms\n", mhi_cntrl->timeout_ms);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static ssize_t mhi_debugfs_timeout_ms_write(struct file *file,
> > > +					    const char __user *ubuf,
> > > +					    size_t count, loff_t *ppos)
> > > +{
> > > +	struct seq_file	*m = file->private_data;
> > > +	struct mhi_controller *mhi_cntrl = m->private;
> > > +	u32 timeout_ms;
> > > +
> > > +	if (kstrtou32_from_user(ubuf, count, 0, &timeout_ms))
> > > +		return -EINVAL;
> > > +
> > > +	mhi_cntrl->timeout_ms = timeout_ms;
> > > +
> > > +	return count;
> > > +}
> > > +
> > > +static int mhi_debugfs_trigger_reset(void *data, u64 val)
> > 
> > Triggering reset on host or device?
> > 
> Device. Will update the function and file name approprirately.
> > > +{
> > > +	struct mhi_controller *mhi_cntrl = data;
> > > +	struct mhi_device *mhi_dev = mhi_cntrl->mhi_dev;
> > > +	struct device *dev = &mhi_dev->dev;
> > > +	enum mhi_pm_state cur_state;
> > > +	int ret = -EIO;
> > > +
> > > +	if (!mhi_is_active(mhi_cntrl))
> > > +		return -ENODEV;
> > > +
> > > +	if (!val)
> > > +		return -EINVAL;
> > > +
> > > +	ret = mhi_device_get_sync(mhi_dev);
> > > +	if (ret) {
> > > +		dev_err(dev, "Device did not enter M0 state, MHI:%s, PM:%s\n",
> > > +			TO_MHI_STATE_STR(mhi_cntrl->dev_state),
> > > +			to_mhi_pm_state_str(mhi_cntrl->pm_state));
> > > +		return ret;
> > > +	}
> > > +
> > > +	if (mhi_cntrl->rddm_image) {
> > > +		ret = mhi_force_rddm_mode(mhi_cntrl);
> > > +		goto exit_mhi_trigger_reset;
> > > +	}
> > > +
> > > +	write_lock_irq(&mhi_cntrl->pm_lock);
> > > +	cur_state = mhi_tryset_pm_state(mhi_cntrl, MHI_PM_SYS_ERR_DETECT);
> > 
> > Looks like in host but how does this resets it?
> > 
> This is the bus internally moving the PM state to "sys error detect" state
> so we can
> check if our state machine allows this move and force a sys error to the
> device.

But how does this reset the client device? I don't find anything related to this
in the MHI spec available to me.

> > > +	write_unlock_irq(&mhi_cntrl->pm_lock);
> > > +
> > > +	if (cur_state != MHI_PM_SYS_ERR_DETECT)
> > > +		goto exit_mhi_trigger_reset;
> > 
> > So this condition is also a success? PS: you're not setting ret here.
> > 
> Thanks for pointing out. It is not a success condition and should return
> -EPERM.
> I will fix it.

[...]

> > > diff --git a/drivers/bus/mhi/core/pm.c b/drivers/bus/mhi/core/pm.c
> > > index 74c5cb1..c80d3553 100644
> > > --- a/drivers/bus/mhi/core/pm.c
> > > +++ b/drivers/bus/mhi/core/pm.c
> > > @@ -256,6 +256,7 @@ int mhi_pm_m0_transition(struct mhi_controller
> > > *mhi_cntrl)
> > >  		dev_err(dev, "Unable to transition to M0 state\n");
> > >  		return -EIO;
> > >  	}
> > > +	mhi_cntrl->M0++;
> > 
> > This change shouldn't be part of this patch.
> > 
> > Thanks,
> > Mani
> > 
> I remember you said this last time as well but I've introduced counters and
> used them in the same patch. It is mentioned in the commit subject and text.
> If you still have concerns, please let me know.

But still these two are separate functionalities. So please split into two
patches.

Thanks,
Mani

> 
> Thanks,
> Bhaumik
> > > 
> > >  	/* Wake up the device */
> > >  	read_lock_bh(&mhi_cntrl->pm_lock);
> > > @@ -326,6 +327,8 @@ void mhi_pm_m1_transition(struct mhi_controller
> > > *mhi_cntrl)
> > >  		mhi_cntrl->dev_state = MHI_STATE_M2;
> > > 
> > >  		write_unlock_irq(&mhi_cntrl->pm_lock);
> > > +
> > > +		mhi_cntrl->M2++;
> > >  		wake_up_all(&mhi_cntrl->state_event);
> > > 
> > >  		/* If there are any pending resources, exit M2 immediately */
> > > @@ -362,6 +365,7 @@ int mhi_pm_m3_transition(struct mhi_controller
> > > *mhi_cntrl)
> > >  		return -EIO;
> > >  	}
> > > 
> > > +	mhi_cntrl->M3++;
> > >  	wake_up_all(&mhi_cntrl->state_event);
> > > 
> > >  	return 0;
> > > diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> > > index 7ed785e..b1e8b4f 100644
> > > --- a/include/linux/mhi.h
> > > +++ b/include/linux/mhi.h
> > > @@ -288,6 +288,7 @@ struct mhi_controller_config {
> > >   * @cntrl_dev: Pointer to the struct device of physical bus acting
> > > as the MHI
> > >   *            controller (required)
> > >   * @mhi_dev: MHI device instance for the controller
> > > + * @debugfs_dentry: MHI controller debugfs directory
> > >   * @regs: Base address of MHI MMIO register space (required)
> > >   * @bhi: Points to base of MHI BHI register space
> > >   * @bhie: Points to base of MHI BHIe register space
> > > @@ -326,6 +327,7 @@ struct mhi_controller_config {
> > >   * @dev_state: MHI device state
> > >   * @dev_wake: Device wakeup count
> > >   * @pending_pkts: Pending packets for the controller
> > > + * @M0, M2, M3, M3_fast: Counters to track number of device MHI
> > > state changes
> > >   * @transition_list: List of MHI state transitions
> > >   * @transition_lock: Lock for protecting MHI state transition list
> > >   * @wlock: Lock for protecting device wakeup
> > > @@ -364,6 +366,7 @@ struct mhi_controller_config {
> > >  struct mhi_controller {
> > >  	struct device *cntrl_dev;
> > >  	struct mhi_device *mhi_dev;
> > > +	struct dentry *debugfs_dentry;
> > >  	void __iomem *regs;
> > >  	void __iomem *bhi;
> > >  	void __iomem *bhie;
> > > @@ -405,6 +408,7 @@ struct mhi_controller {
> > >  	enum mhi_state dev_state;
> > >  	atomic_t dev_wake;
> > >  	atomic_t pending_pkts;
> > > +	u32 M0, M2, M3, M3_fast;
> > >  	struct list_head transition_list;
> > >  	spinlock_t transition_lock;
> > >  	spinlock_t wlock;
> > > --
> > > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
> > > Forum,
> > > a Linux Foundation Collaborative Project
> > > 

[PATCH v4 8/9] bus: mhi: core: Read and save device hardware information from BHI

[Bhaumik Bhatt]

Device hardware specific information such as serial number and the OEM
PK hash can be read using BHI and saved on host to identify the
endpoint.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>
---
 drivers/bus/mhi/core/boot.c | 17 ++++++++++++++++-
 include/linux/mhi.h         |  6 ++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
index 0b38014..24422f5 100644
--- a/drivers/bus/mhi/core/boot.c
+++ b/drivers/bus/mhi/core/boot.c
@@ -392,13 +392,28 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
 	void *buf;
 	dma_addr_t dma_addr;
 	size_t size;
-	int ret;
+	int i, ret;
 
 	if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)) {
 		dev_err(dev, "Device MHI is not in valid state\n");
 		return;
 	}
 
+	/* save hardware info from BHI */
+	ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->bhi, BHI_SERIALNU,
+			   &mhi_cntrl->serial_number);
+	if (ret)
+		dev_err(dev, "Could not capture serial number via BHI\n");
+
+	for (i = 0; i < ARRAY_SIZE(mhi_cntrl->oem_pk_hash); i++) {
+		ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->bhi, BHI_OEMPKHASH(i),
+				   &mhi_cntrl->oem_pk_hash[i]);
+		if (ret) {
+			dev_err(dev, "Could not capture OEM PK HASH via BHI\n");
+			break;
+		}
+	}
+
 	/* If device is in pass through, do reset to ready state transition */
 	if (mhi_cntrl->ee == MHI_EE_PTHRU)
 		goto fw_load_ee_pthru;
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index b1e8b4f..dad6246 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -16,6 +16,8 @@
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 
+#define MHI_MAX_OEM_PK_HASH_SEGMENTS 16
+
 struct mhi_chan;
 struct mhi_event;
 struct mhi_ctxt;
@@ -315,6 +317,8 @@ struct mhi_controller_config {
  * @device_number: MHI controller device number
  * @major_version: MHI controller major revision number
  * @minor_version: MHI controller minor revision number
+ * @serial_number: MHI controller serial number obtained from BHI
+ * @oem_pk_hash: MHI controller OEM PK Hash obtained from BHI
  * @mhi_event: MHI event ring configurations table
  * @mhi_cmd: MHI command ring configurations table
  * @mhi_ctxt: MHI device context, shared memory between host and device
@@ -394,6 +398,8 @@ struct mhi_controller {
 	u32 device_number;
 	u32 major_version;
 	u32 minor_version;
+	u32 serial_number;
+	u32 oem_pk_hash[MHI_MAX_OEM_PK_HASH_SEGMENTS];
 
 	struct mhi_event *mhi_event;
 	struct mhi_cmd *mhi_cmd;
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 8/9] bus: mhi: core: Read and save device hardware information from BHI

[Manivannan Sadhasivam]

On Mon, Jun 29, 2020 at 09:39:41AM -0700, Bhaumik Bhatt wrote:
> Device hardware specific information such as serial number and the OEM
> PK hash can be read using BHI and saved on host to identify the
> endpoint.
> 
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org>

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

Thanks,
Mani

> ---
>  drivers/bus/mhi/core/boot.c | 17 ++++++++++++++++-
>  include/linux/mhi.h         |  6 ++++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/mhi/core/boot.c b/drivers/bus/mhi/core/boot.c
> index 0b38014..24422f5 100644
> --- a/drivers/bus/mhi/core/boot.c
> +++ b/drivers/bus/mhi/core/boot.c
> @@ -392,13 +392,28 @@ void mhi_fw_load_handler(struct mhi_controller *mhi_cntrl)
>  	void *buf;
>  	dma_addr_t dma_addr;
>  	size_t size;
> -	int ret;
> +	int i, ret;
>  
>  	if (MHI_PM_IN_ERROR_STATE(mhi_cntrl->pm_state)) {
>  		dev_err(dev, "Device MHI is not in valid state\n");
>  		return;
>  	}
>  
> +	/* save hardware info from BHI */
> +	ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->bhi, BHI_SERIALNU,
> +			   &mhi_cntrl->serial_number);
> +	if (ret)
> +		dev_err(dev, "Could not capture serial number via BHI\n");
> +
> +	for (i = 0; i < ARRAY_SIZE(mhi_cntrl->oem_pk_hash); i++) {
> +		ret = mhi_read_reg(mhi_cntrl, mhi_cntrl->bhi, BHI_OEMPKHASH(i),
> +				   &mhi_cntrl->oem_pk_hash[i]);
> +		if (ret) {
> +			dev_err(dev, "Could not capture OEM PK HASH via BHI\n");
> +			break;
> +		}
> +	}
> +
>  	/* If device is in pass through, do reset to ready state transition */
>  	if (mhi_cntrl->ee == MHI_EE_PTHRU)
>  		goto fw_load_ee_pthru;
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index b1e8b4f..dad6246 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -16,6 +16,8 @@
>  #include <linux/wait.h>
>  #include <linux/workqueue.h>
>  
> +#define MHI_MAX_OEM_PK_HASH_SEGMENTS 16
> +
>  struct mhi_chan;
>  struct mhi_event;
>  struct mhi_ctxt;
> @@ -315,6 +317,8 @@ struct mhi_controller_config {
>   * @device_number: MHI controller device number
>   * @major_version: MHI controller major revision number
>   * @minor_version: MHI controller minor revision number
> + * @serial_number: MHI controller serial number obtained from BHI
> + * @oem_pk_hash: MHI controller OEM PK Hash obtained from BHI
>   * @mhi_event: MHI event ring configurations table
>   * @mhi_cmd: MHI command ring configurations table
>   * @mhi_ctxt: MHI device context, shared memory between host and device
> @@ -394,6 +398,8 @@ struct mhi_controller {
>  	u32 device_number;
>  	u32 major_version;
>  	u32 minor_version;
> +	u32 serial_number;
> +	u32 oem_pk_hash[MHI_MAX_OEM_PK_HASH_SEGMENTS];
>  
>  	struct mhi_event *mhi_event;
>  	struct mhi_cmd *mhi_cmd;
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
> 

[PATCH v4 9/9] bus: mhi: core: Introduce sysfs entries for MHI

[Bhaumik Bhatt]

Introduce sysfs entries to enable userspace clients the ability to read
the serial number and the OEM PK Hash values obtained from BHI. OEMs
need to read these device-specific hardware information values through
userspace for factory testing purposes and cannot be exposed via degbufs
as it may remain disabled for performance reasons. Also, update the
documentation for ABI to include these entries.

Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
---
 Documentation/ABI/stable/sysfs-bus-mhi | 25 ++++++++++++++++
 MAINTAINERS                            |  1 +
 drivers/bus/mhi/core/init.c            | 53 ++++++++++++++++++++++++++++++++++
 3 files changed, 79 insertions(+)
 create mode 100644 Documentation/ABI/stable/sysfs-bus-mhi

diff --git a/Documentation/ABI/stable/sysfs-bus-mhi b/Documentation/ABI/stable/sysfs-bus-mhi
new file mode 100644
index 0000000..65ef711
--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-bus-mhi
@@ -0,0 +1,25 @@
+What:		/sys/bus/mhi/devices/.../serialnumber
+Date:		May 2020
+KernelVersion:  5.8
+Contact:	Bhaumik Bhatt <bbhatt@codeaurora.org>
+Description:
+		The file holds the serial number of the endpoint device obtained
+		using a BHI (Boot Host Interface) register read after at least
+		one attempt to power up the device has been done. If read
+		without having the device power on at least once, the file will
+		read all 0's.
+Users:		Any userspace application or clients interested in the device
+		hardware information.
+
+What:		/sys/bus/mhi/devices/.../oem_pk_hash
+Date:		May 2020
+KernelVersion:  5.8
+Contact:	Bhaumik Bhatt <bbhatt@codeaurora.org>
+Description:
+		The file holds the OEM PK Hash value of the endpoint device
+		obtained using a BHI (Boot Host Interface) register read after
+		at least one attempt to power up the device has been done. If
+		read without having the device power on at least once, the file
+		will read all 0's.
+Users:		Any userspace application or clients interested in the device
+		hardware information.
diff --git a/MAINTAINERS b/MAINTAINERS
index e64e5db..5e49316 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11018,6 +11018,7 @@ M:	Hemant Kumar <hemantk@codeaurora.org>
 L:	linux-arm-msm@vger.kernel.org
 S:	Maintained
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/mani/mhi.git
+F:	Documentation/ABI/stable/sysfs-bus-mhi
 F:	Documentation/mhi/
 F:	drivers/bus/mhi/
 F:	include/linux/mhi.h
diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
index d2c0f6e..745e146 100644
--- a/drivers/bus/mhi/core/init.c
+++ b/drivers/bus/mhi/core/init.c
@@ -76,6 +76,56 @@ const char *to_mhi_pm_state_str(enum mhi_pm_state state)
 	return mhi_pm_state_str[index];
 }
 
+static ssize_t serial_number_show(struct device *dev,
+				  struct device_attribute *attr,
+				  char *buf)
+{
+	struct mhi_device *mhi_dev = to_mhi_device(dev);
+	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
+
+	return snprintf(buf, PAGE_SIZE, "Serial Number:%u\n",
+			mhi_cntrl->serial_number);
+}
+static DEVICE_ATTR_RO(serial_number);
+
+static ssize_t oem_pk_hash_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct mhi_device *mhi_dev = to_mhi_device(dev);
+	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
+	int i, cnt = 0;
+
+	for (i = 0; i < ARRAY_SIZE(mhi_cntrl->oem_pk_hash); i++)
+		cnt += snprintf(buf + cnt, PAGE_SIZE - cnt,
+				"OEMPKHASH[%d]:0x%x\n", i,
+				mhi_cntrl->oem_pk_hash[i]);
+
+	return cnt;
+}
+static DEVICE_ATTR_RO(oem_pk_hash);
+
+static struct attribute *mhi_sysfs_attrs[] = {
+	&dev_attr_serial_number.attr,
+	&dev_attr_oem_pk_hash.attr,
+	NULL,
+};
+
+static const struct attribute_group mhi_sysfs_group = {
+	.attrs = mhi_sysfs_attrs,
+};
+
+static int mhi_create_sysfs(struct mhi_controller *mhi_cntrl)
+{
+	return sysfs_create_group(&mhi_cntrl->mhi_dev->dev.kobj,
+				  &mhi_sysfs_group);
+}
+
+static void mhi_destroy_sysfs(struct mhi_controller *mhi_cntrl)
+{
+	sysfs_remove_group(&mhi_cntrl->mhi_dev->dev.kobj, &mhi_sysfs_group);
+}
+
 /* MHI protocol requires the transfer ring to be aligned with ring length */
 static int mhi_alloc_aligned_ring(struct mhi_controller *mhi_cntrl,
 				  struct mhi_ring *ring,
@@ -917,6 +967,8 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
 	mhi_cntrl->mhi_dev = mhi_dev;
 
 	mhi_create_debugfs(mhi_cntrl);
+	if (mhi_create_sysfs(mhi_cntrl))
+		dev_err(mhi_cntrl->cntrl_dev, "Failed to create sysfs entries\n");
 
 	return 0;
 
@@ -940,6 +992,7 @@ void mhi_unregister_controller(struct mhi_controller *mhi_cntrl)
 	struct mhi_chan *mhi_chan = mhi_cntrl->mhi_chan;
 	unsigned int i;
 
+	mhi_destroy_sysfs(mhi_cntrl);
 	mhi_destroy_debugfs(mhi_cntrl);
 
 	kfree(mhi_cntrl->mhi_cmd);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 9/9] bus: mhi: core: Introduce sysfs entries for MHI

[Manivannan Sadhasivam]

On Mon, Jun 29, 2020 at 09:39:42AM -0700, Bhaumik Bhatt wrote:
> Introduce sysfs entries to enable userspace clients the ability to read
> the serial number and the OEM PK Hash values obtained from BHI. OEMs
> need to read these device-specific hardware information values through
> userspace for factory testing purposes and cannot be exposed via degbufs
> as it may remain disabled for performance reasons. Also, update the
> documentation for ABI to include these entries.
> 
> Signed-off-by: Bhaumik Bhatt <bbhatt@codeaurora.org>
> ---
>  Documentation/ABI/stable/sysfs-bus-mhi | 25 ++++++++++++++++
>  MAINTAINERS                            |  1 +
>  drivers/bus/mhi/core/init.c            | 53 ++++++++++++++++++++++++++++++++++
>  3 files changed, 79 insertions(+)
>  create mode 100644 Documentation/ABI/stable/sysfs-bus-mhi
> 
> diff --git a/Documentation/ABI/stable/sysfs-bus-mhi b/Documentation/ABI/stable/sysfs-bus-mhi
> new file mode 100644
> index 0000000..65ef711
> --- /dev/null
> +++ b/Documentation/ABI/stable/sysfs-bus-mhi
> @@ -0,0 +1,25 @@
> +What:		/sys/bus/mhi/devices/.../serialnumber
> +Date:		May 2020

July?

> +KernelVersion:  5.8
> +Contact:	Bhaumik Bhatt <bbhatt@codeaurora.org>
> +Description:
> +		The file holds the serial number of the endpoint device obtained

Don't use the term endpoint here. Just say MHI client device.

> +		using a BHI (Boot Host Interface) register read after at least
> +		one attempt to power up the device has been done. If read
> +		without having the device power on at least once, the file will
> +		read all 0's.
> +Users:		Any userspace application or clients interested in the device
> +		hardware information.
> +
> +What:		/sys/bus/mhi/devices/.../oem_pk_hash
> +Date:		May 2020
> +KernelVersion:  5.8
> +Contact:	Bhaumik Bhatt <bbhatt@codeaurora.org>
> +Description:
> +		The file holds the OEM PK Hash value of the endpoint device
> +		obtained using a BHI (Boot Host Interface) register read after
> +		at least one attempt to power up the device has been done. If
> +		read without having the device power on at least once, the file
> +		will read all 0's.
> +Users:		Any userspace application or clients interested in the device
> +		hardware information.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e64e5db..5e49316 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -11018,6 +11018,7 @@ M:	Hemant Kumar <hemantk@codeaurora.org>
>  L:	linux-arm-msm@vger.kernel.org
>  S:	Maintained
>  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/mani/mhi.git
> +F:	Documentation/ABI/stable/sysfs-bus-mhi
>  F:	Documentation/mhi/
>  F:	drivers/bus/mhi/
>  F:	include/linux/mhi.h
> diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c
> index d2c0f6e..745e146 100644
> --- a/drivers/bus/mhi/core/init.c
> +++ b/drivers/bus/mhi/core/init.c
> @@ -76,6 +76,56 @@ const char *to_mhi_pm_state_str(enum mhi_pm_state state)
>  	return mhi_pm_state_str[index];
>  }
>  
> +static ssize_t serial_number_show(struct device *dev,
> +				  struct device_attribute *attr,
> +				  char *buf)
> +{
> +	struct mhi_device *mhi_dev = to_mhi_device(dev);
> +	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> +
> +	return snprintf(buf, PAGE_SIZE, "Serial Number:%u\n",
> +			mhi_cntrl->serial_number);

Space after ':'

> +}
> +static DEVICE_ATTR_RO(serial_number);
> +
> +static ssize_t oem_pk_hash_show(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct mhi_device *mhi_dev = to_mhi_device(dev);
> +	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> +	int i, cnt = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(mhi_cntrl->oem_pk_hash); i++)
> +		cnt += snprintf(buf + cnt, PAGE_SIZE - cnt,
> +				"OEMPKHASH[%d]:0x%x\n", i,
> +				mhi_cntrl->oem_pk_hash[i]);

Same here.

Thanks,
Mani

> +
> +	return cnt;
> +}
> +static DEVICE_ATTR_RO(oem_pk_hash);
> +
> +static struct attribute *mhi_sysfs_attrs[] = {
> +	&dev_attr_serial_number.attr,
> +	&dev_attr_oem_pk_hash.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group mhi_sysfs_group = {
> +	.attrs = mhi_sysfs_attrs,
> +};
> +
> +static int mhi_create_sysfs(struct mhi_controller *mhi_cntrl)
> +{
> +	return sysfs_create_group(&mhi_cntrl->mhi_dev->dev.kobj,
> +				  &mhi_sysfs_group);
> +}
> +
> +static void mhi_destroy_sysfs(struct mhi_controller *mhi_cntrl)
> +{
> +	sysfs_remove_group(&mhi_cntrl->mhi_dev->dev.kobj, &mhi_sysfs_group);
> +}
> +
>  /* MHI protocol requires the transfer ring to be aligned with ring length */
>  static int mhi_alloc_aligned_ring(struct mhi_controller *mhi_cntrl,
>  				  struct mhi_ring *ring,
> @@ -917,6 +967,8 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
>  	mhi_cntrl->mhi_dev = mhi_dev;
>  
>  	mhi_create_debugfs(mhi_cntrl);
> +	if (mhi_create_sysfs(mhi_cntrl))
> +		dev_err(mhi_cntrl->cntrl_dev, "Failed to create sysfs entries\n");
>  
>  	return 0;
>  
> @@ -940,6 +992,7 @@ void mhi_unregister_controller(struct mhi_controller *mhi_cntrl)
>  	struct mhi_chan *mhi_chan = mhi_cntrl->mhi_chan;
>  	unsigned int i;
>  
> +	mhi_destroy_sysfs(mhi_cntrl);
>  	mhi_destroy_debugfs(mhi_cntrl);
>  
>  	kfree(mhi_cntrl->mhi_cmd);
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>