[PATCH v3 0/3] Add sysfs entry to EDL mode

[PATCH v3 0/3] Add sysfs entry to EDL mode

[Qiang Yu]

Add EDL sysfs entry for mhi controller that provides edl_trigger callback.
Add mhi_pci_generic_edl_trigger for qualcomm sdx55,sdx65 and sdx75 as
edl_trigger callback.

v2->v3:
1. Update Documentation/ABI/stable/sysfs-bus-mhi with description of
   force_edl sysfs entry.

2. Add comments about edl_trigger callback in mhi_controller struct.

3. Follow reverse christmas tree in mhi_pci_generic_edl_trigger.

4. Add a new API in MHI to allow controller to get CHDB address and avoid
   duplicating the definition of CHDBOFF.

v1->v2:
1. Move all process needed by qualcomm sdx55,sdx65,sdx75 to enter EDL into
   mhi_pci_generic_edl_trigger() as the callback to edl_trigger.

2. MHI stack creates EDL sysfs entry to invoke edl_trigger callback so
   that devices need different mechanism to enter EDL can provide its own
   edl_trigger callabck .

Qiang Yu (3):
  bus: mhi: host: Add sysfs entry to force device to enter EDL
  bus: mhi: host: Add a new API for getting channel doorbell address
  bus: mhi: host: pci_generic: Add edl callback to enter EDL

 Documentation/ABI/stable/sysfs-bus-mhi | 11 ++++++++
 drivers/bus/mhi/host/init.c            | 35 +++++++++++++++++++++++++
 drivers/bus/mhi/host/main.c            | 17 ++++++++++++
 drivers/bus/mhi/host/pci_generic.c     | 47 ++++++++++++++++++++++++++++++++++
 include/linux/mhi.h                    |  9 +++++++
 5 files changed, 119 insertions(+)

-- 
2.7.4

[PATCH v3 1/3] bus: mhi: host: Add sysfs entry to force device to enter EDL

[Qiang Yu]

Add sysfs entry to allow users of MHI bus force device to enter EDL.
Considering that the way to enter EDL mode varies from device to device and
some devices even do not support EDL. Hence, add a callback edl_trigger in
mhi controller as part of the sysfs entry to be invoked and MHI core will
only create EDL sysfs entry for mhi controller that provides edl_trigger
callback. All of the process a specific device required to enter EDL mode
can be placed in this callback.

Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
---
 Documentation/ABI/stable/sysfs-bus-mhi | 11 +++++++++++
 drivers/bus/mhi/host/init.c            | 35 ++++++++++++++++++++++++++++++++++
 include/linux/mhi.h                    |  2 ++
 3 files changed, 48 insertions(+)

diff --git a/Documentation/ABI/stable/sysfs-bus-mhi b/Documentation/ABI/stable/sysfs-bus-mhi
index 1a47f9e..d0bf9ae 100644
--- a/Documentation/ABI/stable/sysfs-bus-mhi
+++ b/Documentation/ABI/stable/sysfs-bus-mhi
@@ -29,3 +29,14 @@ Description:	Initiates a SoC reset on the MHI controller.  A SoC reset is
                 This can be useful as a method of recovery if the device is
                 non-responsive, or as a means of loading new firmware as a
                 system administration task.
+
+What:           /sys/bus/mhi/devices/.../force_edl
+Date:           April 2024
+KernelVersion:  6.9
+Contact:        mhi@lists.linux.dev
+Description:    Force devices to enter EDL (emergency download) mode. Only MHI
+                controller that supports EDL mode and provides a mechanism for
+                manually triggering EDL contains this file. Once in EDL mode,
+                the flash programmer image can be downloaded to the device to
+                enter the flash programmer execution environment. This can be
+                useful if user wants to update firmware.
diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
index 44f9349..333ac94 100644
--- a/drivers/bus/mhi/host/init.c
+++ b/drivers/bus/mhi/host/init.c
@@ -127,6 +127,32 @@ static ssize_t soc_reset_store(struct device *dev,
 }
 static DEVICE_ATTR_WO(soc_reset);
 
+static ssize_t force_edl_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	struct mhi_device *mhi_dev = to_mhi_device(dev);
+	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
+	unsigned long val;
+	int ret;
+
+	ret = kstrtoul(buf, 10, &val);
+	if (ret < 0) {
+		dev_err(dev, "Could not parse string: %d\n", ret);
+		return ret;
+	}
+
+	if (!val)
+		return count;
+
+	ret = mhi_cntrl->edl_trigger(mhi_cntrl);
+	if (ret)
+		return ret;
+
+	return count;
+}
+static DEVICE_ATTR_WO(force_edl);
+
 static struct attribute *mhi_dev_attrs[] = {
 	&dev_attr_serial_number.attr,
 	&dev_attr_oem_pk_hash.attr,
@@ -1018,6 +1044,12 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
 	if (ret)
 		goto err_release_dev;
 
+	if (mhi_cntrl->edl_trigger) {
+		ret = sysfs_create_file(&mhi_dev->dev.kobj, &dev_attr_force_edl.attr);
+		if (ret)
+			goto err_release_dev;
+	}
+
 	mhi_cntrl->mhi_dev = mhi_dev;
 
 	mhi_create_debugfs(mhi_cntrl);
@@ -1051,6 +1083,9 @@ void mhi_unregister_controller(struct mhi_controller *mhi_cntrl)
 	mhi_deinit_free_irq(mhi_cntrl);
 	mhi_destroy_debugfs(mhi_cntrl);
 
+	if (mhi_cntrl->edl_trigger)
+		sysfs_remove_file(&mhi_dev->dev.kobj, &dev_attr_force_edl.attr);
+
 	destroy_workqueue(mhi_cntrl->hiprio_wq);
 	kfree(mhi_cntrl->mhi_cmd);
 	kfree(mhi_cntrl->mhi_event);
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index cde01e1..8280545 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -353,6 +353,7 @@ struct mhi_controller_config {
  * @read_reg: Read a MHI register via the physical link (required)
  * @write_reg: Write a MHI register via the physical link (required)
  * @reset: Controller specific reset function (optional)
+ * @edl_trigger: CB function to enter EDL mode (optional)
  * @buffer_len: Bounce buffer length
  * @index: Index of the MHI controller instance
  * @bounce_buf: Use of bounce buffer
@@ -435,6 +436,7 @@ struct mhi_controller {
 	void (*write_reg)(struct mhi_controller *mhi_cntrl, void __iomem *addr,
 			  u32 val);
 	void (*reset)(struct mhi_controller *mhi_cntrl);
+	int (*edl_trigger)(struct mhi_controller *mhi_cntrl);
 
 	size_t buffer_len;
 	int index;
-- 
2.7.4

Re: [PATCH v3 1/3] bus: mhi: host: Add sysfs entry to force device to enter EDL

[Manivannan Sadhasivam]

On Mon, Apr 15, 2024 at 04:49:03PM +0800, Qiang Yu wrote:
> Add sysfs entry to allow users of MHI bus force device to enter EDL.
> Considering that the way to enter EDL mode varies from device to device and
> some devices even do not support EDL. Hence, add a callback edl_trigger in
> mhi controller as part of the sysfs entry to be invoked and MHI core will
> only create EDL sysfs entry for mhi controller that provides edl_trigger
> callback. All of the process a specific device required to enter EDL mode
> can be placed in this callback.
> 
> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
> ---
>  Documentation/ABI/stable/sysfs-bus-mhi | 11 +++++++++++
>  drivers/bus/mhi/host/init.c            | 35 ++++++++++++++++++++++++++++++++++
>  include/linux/mhi.h                    |  2 ++
>  3 files changed, 48 insertions(+)
> 
> diff --git a/Documentation/ABI/stable/sysfs-bus-mhi b/Documentation/ABI/stable/sysfs-bus-mhi
> index 1a47f9e..d0bf9ae 100644
> --- a/Documentation/ABI/stable/sysfs-bus-mhi
> +++ b/Documentation/ABI/stable/sysfs-bus-mhi
> @@ -29,3 +29,14 @@ Description:	Initiates a SoC reset on the MHI controller.  A SoC reset is
>                  This can be useful as a method of recovery if the device is
>                  non-responsive, or as a means of loading new firmware as a
>                  system administration task.
> +
> +What:           /sys/bus/mhi/devices/.../force_edl

s/force_edl/trigger_edl

> +Date:           April 2024
> +KernelVersion:  6.9
> +Contact:        mhi@lists.linux.dev
> +Description:    Force devices to enter EDL (emergency download) mode. Only MHI

How can the user trigger EDL mode? Writing to this file? If so, what is the
value?

'Emergency Download'

> +                controller that supports EDL mode and provides a mechanism for
> +                manually triggering EDL contains this file. Once in EDL mode,

'This entry only exists for devices capable of entering the EDL mode using the
standard EDL triggering mechanism defined in the MHI spec <insert the version>.'

> +                the flash programmer image can be downloaded to the device to
> +                enter the flash programmer execution environment. This can be
> +                useful if user wants to update firmware.

It'd be good to mention the OS tool like QDL that is used to download firmware
over EDL.

> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
> index 44f9349..333ac94 100644
> --- a/drivers/bus/mhi/host/init.c
> +++ b/drivers/bus/mhi/host/init.c
> @@ -127,6 +127,32 @@ static ssize_t soc_reset_store(struct device *dev,
>  }
>  static DEVICE_ATTR_WO(soc_reset);
>  
> +static ssize_t force_edl_store(struct device *dev,

s/force_edl/trigger_edl

> +			       struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct mhi_device *mhi_dev = to_mhi_device(dev);
> +	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> +	unsigned long val;
> +	int ret;
> +
> +	ret = kstrtoul(buf, 10, &val);
> +	if (ret < 0) {
> +		dev_err(dev, "Could not parse string: %d\n", ret);

No need to print error.

> +		return ret;
> +	}
> +
> +	if (!val)
> +		return count;

What does this mean?

> +
> +	ret = mhi_cntrl->edl_trigger(mhi_cntrl);
> +	if (ret)
> +		return ret;
> +
> +	return count;
> +}
> +static DEVICE_ATTR_WO(force_edl);
> +
>  static struct attribute *mhi_dev_attrs[] = {
>  	&dev_attr_serial_number.attr,
>  	&dev_attr_oem_pk_hash.attr,
> @@ -1018,6 +1044,12 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
>  	if (ret)
>  		goto err_release_dev;
>  
> +	if (mhi_cntrl->edl_trigger) {
> +		ret = sysfs_create_file(&mhi_dev->dev.kobj, &dev_attr_force_edl.attr);
> +		if (ret)
> +			goto err_release_dev;
> +	}
> +
>  	mhi_cntrl->mhi_dev = mhi_dev;
>  
>  	mhi_create_debugfs(mhi_cntrl);
> @@ -1051,6 +1083,9 @@ void mhi_unregister_controller(struct mhi_controller *mhi_cntrl)
>  	mhi_deinit_free_irq(mhi_cntrl);
>  	mhi_destroy_debugfs(mhi_cntrl);
>  
> +	if (mhi_cntrl->edl_trigger)
> +		sysfs_remove_file(&mhi_dev->dev.kobj, &dev_attr_force_edl.attr);
> +
>  	destroy_workqueue(mhi_cntrl->hiprio_wq);
>  	kfree(mhi_cntrl->mhi_cmd);
>  	kfree(mhi_cntrl->mhi_event);
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index cde01e1..8280545 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -353,6 +353,7 @@ struct mhi_controller_config {
>   * @read_reg: Read a MHI register via the physical link (required)
>   * @write_reg: Write a MHI register via the physical link (required)
>   * @reset: Controller specific reset function (optional)
> + * @edl_trigger: CB function to enter EDL mode (optional)

s/enter/trigger

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v3 1/3] bus: mhi: host: Add sysfs entry to force device to enter EDL

[Qiang Yu]

On 4/15/2024 7:56 PM, Manivannan Sadhasivam wrote:
> On Mon, Apr 15, 2024 at 04:49:03PM +0800, Qiang Yu wrote:
>> Add sysfs entry to allow users of MHI bus force device to enter EDL.
>> Considering that the way to enter EDL mode varies from device to device and
>> some devices even do not support EDL. Hence, add a callback edl_trigger in
>> mhi controller as part of the sysfs entry to be invoked and MHI core will
>> only create EDL sysfs entry for mhi controller that provides edl_trigger
>> callback. All of the process a specific device required to enter EDL mode
>> can be placed in this callback.
>>
>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>> ---
>>   Documentation/ABI/stable/sysfs-bus-mhi | 11 +++++++++++
>>   drivers/bus/mhi/host/init.c            | 35 ++++++++++++++++++++++++++++++++++
>>   include/linux/mhi.h                    |  2 ++
>>   3 files changed, 48 insertions(+)
>>
>> diff --git a/Documentation/ABI/stable/sysfs-bus-mhi b/Documentation/ABI/stable/sysfs-bus-mhi
>> index 1a47f9e..d0bf9ae 100644
>> --- a/Documentation/ABI/stable/sysfs-bus-mhi
>> +++ b/Documentation/ABI/stable/sysfs-bus-mhi
>> @@ -29,3 +29,14 @@ Description:	Initiates a SoC reset on the MHI controller.  A SoC reset is
>>                   This can be useful as a method of recovery if the device is
>>                   non-responsive, or as a means of loading new firmware as a
>>                   system administration task.
>> +
>> +What:           /sys/bus/mhi/devices/.../force_edl
> s/force_edl/trigger_edl
>
>> +Date:           April 2024
>> +KernelVersion:  6.9
>> +Contact:        mhi@lists.linux.dev
>> +Description:    Force devices to enter EDL (emergency download) mode. Only MHI
> How can the user trigger EDL mode? Writing to this file? If so, what is the
> value?

User can trigger EDL mode by writing a non-zero value to this file.

>
> 'Emergency Download'
>
>> +                controller that supports EDL mode and provides a mechanism for
>> +                manually triggering EDL contains this file. Once in EDL mode,
> 'This entry only exists for devices capable of entering the EDL mode using the
> standard EDL triggering mechanism defined in the MHI spec <insert the version>.'
>
>> +                the flash programmer image can be downloaded to the device to
>> +                enter the flash programmer execution environment. This can be
>> +                useful if user wants to update firmware.
> It'd be good to mention the OS tool like QDL that is used to download firmware
> over EDL.

OK, can I add an additionnal line like this
Users:          Any OS tools that is used to download firmware over EDL 
like QDL.

>
>> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
>> index 44f9349..333ac94 100644
>> --- a/drivers/bus/mhi/host/init.c
>> +++ b/drivers/bus/mhi/host/init.c
>> @@ -127,6 +127,32 @@ static ssize_t soc_reset_store(struct device *dev,
>>   }
>>   static DEVICE_ATTR_WO(soc_reset);
>>   
>> +static ssize_t force_edl_store(struct device *dev,
> s/force_edl/trigger_edl
>
>> +			       struct device_attribute *attr,
>> +			       const char *buf, size_t count)
>> +{
>> +	struct mhi_device *mhi_dev = to_mhi_device(dev);
>> +	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
>> +	unsigned long val;
>> +	int ret;
>> +
>> +	ret = kstrtoul(buf, 10, &val);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Could not parse string: %d\n", ret);
> No need to print error.
>
>> +		return ret;
>> +	}
>> +
>> +	if (!val)
>> +		return count;
> What does this mean?

If user want to trigger EDL mode,he has to write a non-zero value to this
file. If he writes zero, nothing will happen.

Do we need to limit the valid value to a specific value like 1?

>
>> +
>> +	ret = mhi_cntrl->edl_trigger(mhi_cntrl);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return count;
>> +}
>> +static DEVICE_ATTR_WO(force_edl);
>> +
>>   static struct attribute *mhi_dev_attrs[] = {
>>   	&dev_attr_serial_number.attr,
>>   	&dev_attr_oem_pk_hash.attr,
>> @@ -1018,6 +1044,12 @@ int mhi_register_controller(struct mhi_controller *mhi_cntrl,
>>   	if (ret)
>>   		goto err_release_dev;
>>   
>> +	if (mhi_cntrl->edl_trigger) {
>> +		ret = sysfs_create_file(&mhi_dev->dev.kobj, &dev_attr_force_edl.attr);
>> +		if (ret)
>> +			goto err_release_dev;
>> +	}
>> +
>>   	mhi_cntrl->mhi_dev = mhi_dev;
>>   
>>   	mhi_create_debugfs(mhi_cntrl);
>> @@ -1051,6 +1083,9 @@ void mhi_unregister_controller(struct mhi_controller *mhi_cntrl)
>>   	mhi_deinit_free_irq(mhi_cntrl);
>>   	mhi_destroy_debugfs(mhi_cntrl);
>>   
>> +	if (mhi_cntrl->edl_trigger)
>> +		sysfs_remove_file(&mhi_dev->dev.kobj, &dev_attr_force_edl.attr);
>> +
>>   	destroy_workqueue(mhi_cntrl->hiprio_wq);
>>   	kfree(mhi_cntrl->mhi_cmd);
>>   	kfree(mhi_cntrl->mhi_event);
>> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
>> index cde01e1..8280545 100644
>> --- a/include/linux/mhi.h
>> +++ b/include/linux/mhi.h
>> @@ -353,6 +353,7 @@ struct mhi_controller_config {
>>    * @read_reg: Read a MHI register via the physical link (required)
>>    * @write_reg: Write a MHI register via the physical link (required)
>>    * @reset: Controller specific reset function (optional)
>> + * @edl_trigger: CB function to enter EDL mode (optional)
> s/enter/trigger
>
> - Mani
>

Re: [PATCH v3 1/3] bus: mhi: host: Add sysfs entry to force device to enter EDL

[Manivannan Sadhasivam]

On Tue, Apr 16, 2024 at 01:45:18PM +0800, Qiang Yu wrote:
> 
> On 4/15/2024 7:56 PM, Manivannan Sadhasivam wrote:
> > On Mon, Apr 15, 2024 at 04:49:03PM +0800, Qiang Yu wrote:
> > > Add sysfs entry to allow users of MHI bus force device to enter EDL.
> > > Considering that the way to enter EDL mode varies from device to device and
> > > some devices even do not support EDL. Hence, add a callback edl_trigger in
> > > mhi controller as part of the sysfs entry to be invoked and MHI core will
> > > only create EDL sysfs entry for mhi controller that provides edl_trigger
> > > callback. All of the process a specific device required to enter EDL mode
> > > can be placed in this callback.
> > > 
> > > Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
> > > ---
> > >   Documentation/ABI/stable/sysfs-bus-mhi | 11 +++++++++++
> > >   drivers/bus/mhi/host/init.c            | 35 ++++++++++++++++++++++++++++++++++
> > >   include/linux/mhi.h                    |  2 ++
> > >   3 files changed, 48 insertions(+)
> > > 
> > > diff --git a/Documentation/ABI/stable/sysfs-bus-mhi b/Documentation/ABI/stable/sysfs-bus-mhi
> > > index 1a47f9e..d0bf9ae 100644
> > > --- a/Documentation/ABI/stable/sysfs-bus-mhi
> > > +++ b/Documentation/ABI/stable/sysfs-bus-mhi
> > > @@ -29,3 +29,14 @@ Description:	Initiates a SoC reset on the MHI controller.  A SoC reset is
> > >                   This can be useful as a method of recovery if the device is
> > >                   non-responsive, or as a means of loading new firmware as a
> > >                   system administration task.
> > > +
> > > +What:           /sys/bus/mhi/devices/.../force_edl
> > s/force_edl/trigger_edl
> > 
> > > +Date:           April 2024
> > > +KernelVersion:  6.9
> > > +Contact:        mhi@lists.linux.dev
> > > +Description:    Force devices to enter EDL (emergency download) mode. Only MHI
> > How can the user trigger EDL mode? Writing to this file? If so, what is the
> > value?
> 
> User can trigger EDL mode by writing a non-zero value to this file.
> 
> > 
> > 'Emergency Download'
> > 
> > > +                controller that supports EDL mode and provides a mechanism for
> > > +                manually triggering EDL contains this file. Once in EDL mode,
> > 'This entry only exists for devices capable of entering the EDL mode using the
> > standard EDL triggering mechanism defined in the MHI spec <insert the version>.'
> > 
> > > +                the flash programmer image can be downloaded to the device to
> > > +                enter the flash programmer execution environment. This can be
> > > +                useful if user wants to update firmware.
> > It'd be good to mention the OS tool like QDL that is used to download firmware
> > over EDL.
> 
> OK, can I add an additionnal line like this
> Users:          Any OS tools that is used to download firmware over EDL like
> QDL.
> 
> > 
> > > diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
> > > index 44f9349..333ac94 100644
> > > --- a/drivers/bus/mhi/host/init.c
> > > +++ b/drivers/bus/mhi/host/init.c
> > > @@ -127,6 +127,32 @@ static ssize_t soc_reset_store(struct device *dev,
> > >   }
> > >   static DEVICE_ATTR_WO(soc_reset);
> > > +static ssize_t force_edl_store(struct device *dev,
> > s/force_edl/trigger_edl
> > 
> > > +			       struct device_attribute *attr,
> > > +			       const char *buf, size_t count)
> > > +{
> > > +	struct mhi_device *mhi_dev = to_mhi_device(dev);
> > > +	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
> > > +	unsigned long val;
> > > +	int ret;
> > > +
> > > +	ret = kstrtoul(buf, 10, &val);
> > > +	if (ret < 0) {
> > > +		dev_err(dev, "Could not parse string: %d\n", ret);
> > No need to print error.
> > 
> > > +		return ret;
> > > +	}
> > > +
> > > +	if (!val)
> > > +		return count;
> > What does this mean?
> 
> If user want to trigger EDL mode,he has to write a non-zero value to this
> file. If he writes zero, nothing will happen.
> 

You need to throw -EINVAL for invalid inputs ie., <= 0.

> Do we need to limit the valid value to a specific value like 1?
> 

That's not needed.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v3 1/3] bus: mhi: host: Add sysfs entry to force device to enter EDL

[Qiang Yu]

On 4/22/2024 3:24 PM, Manivannan Sadhasivam wrote:
> On Tue, Apr 16, 2024 at 01:45:18PM +0800, Qiang Yu wrote:
>> On 4/15/2024 7:56 PM, Manivannan Sadhasivam wrote:
>>> On Mon, Apr 15, 2024 at 04:49:03PM +0800, Qiang Yu wrote:
>>>> Add sysfs entry to allow users of MHI bus force device to enter EDL.
>>>> Considering that the way to enter EDL mode varies from device to device and
>>>> some devices even do not support EDL. Hence, add a callback edl_trigger in
>>>> mhi controller as part of the sysfs entry to be invoked and MHI core will
>>>> only create EDL sysfs entry for mhi controller that provides edl_trigger
>>>> callback. All of the process a specific device required to enter EDL mode
>>>> can be placed in this callback.
>>>>
>>>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>>>> ---
>>>>    Documentation/ABI/stable/sysfs-bus-mhi | 11 +++++++++++
>>>>    drivers/bus/mhi/host/init.c            | 35 ++++++++++++++++++++++++++++++++++
>>>>    include/linux/mhi.h                    |  2 ++
>>>>    3 files changed, 48 insertions(+)
>>>>
>>>> diff --git a/Documentation/ABI/stable/sysfs-bus-mhi b/Documentation/ABI/stable/sysfs-bus-mhi
>>>> index 1a47f9e..d0bf9ae 100644
>>>> --- a/Documentation/ABI/stable/sysfs-bus-mhi
>>>> +++ b/Documentation/ABI/stable/sysfs-bus-mhi
>>>> @@ -29,3 +29,14 @@ Description:	Initiates a SoC reset on the MHI controller.  A SoC reset is
>>>>                    This can be useful as a method of recovery if the device is
>>>>                    non-responsive, or as a means of loading new firmware as a
>>>>                    system administration task.
>>>> +
>>>> +What:           /sys/bus/mhi/devices/.../force_edl
>>> s/force_edl/trigger_edl
>>>
>>>> +Date:           April 2024
>>>> +KernelVersion:  6.9
>>>> +Contact:        mhi@lists.linux.dev
>>>> +Description:    Force devices to enter EDL (emergency download) mode. Only MHI
>>> How can the user trigger EDL mode? Writing to this file? If so, what is the
>>> value?
>> User can trigger EDL mode by writing a non-zero value to this file.
>>
>>> 'Emergency Download'
>>>
>>>> +                controller that supports EDL mode and provides a mechanism for
>>>> +                manually triggering EDL contains this file. Once in EDL mode,
>>> 'This entry only exists for devices capable of entering the EDL mode using the
>>> standard EDL triggering mechanism defined in the MHI spec <insert the version>.'
>>>
>>>> +                the flash programmer image can be downloaded to the device to
>>>> +                enter the flash programmer execution environment. This can be
>>>> +                useful if user wants to update firmware.
>>> It'd be good to mention the OS tool like QDL that is used to download firmware
>>> over EDL.
>> OK, can I add an additionnal line like this
>> Users:          Any OS tools that is used to download firmware over EDL like
>> QDL.
>>
>>>> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
>>>> index 44f9349..333ac94 100644
>>>> --- a/drivers/bus/mhi/host/init.c
>>>> +++ b/drivers/bus/mhi/host/init.c
>>>> @@ -127,6 +127,32 @@ static ssize_t soc_reset_store(struct device *dev,
>>>>    }
>>>>    static DEVICE_ATTR_WO(soc_reset);
>>>> +static ssize_t force_edl_store(struct device *dev,
>>> s/force_edl/trigger_edl
>>>
>>>> +			       struct device_attribute *attr,
>>>> +			       const char *buf, size_t count)
>>>> +{
>>>> +	struct mhi_device *mhi_dev = to_mhi_device(dev);
>>>> +	struct mhi_controller *mhi_cntrl = mhi_dev->mhi_cntrl;
>>>> +	unsigned long val;
>>>> +	int ret;
>>>> +
>>>> +	ret = kstrtoul(buf, 10, &val);
>>>> +	if (ret < 0) {
>>>> +		dev_err(dev, "Could not parse string: %d\n", ret);
>>> No need to print error.
>>>
>>>> +		return ret;
>>>> +	}
>>>> +
>>>> +	if (!val)
>>>> +		return count;
>>> What does this mean?
>> If user want to trigger EDL mode,he has to write a non-zero value to this
>> file. If he writes zero, nothing will happen.
>>
> You need to throw -EINVAL for invalid inputs ie., <= 0.
Thanks for pointing this out. Will also address it in next version
patch.

Thanks,
Qiang
>> Do we need to limit the valid value to a specific value like 1?
>>
> That's not needed.
>
> - Mani
>

[PATCH v3 2/3] bus: mhi: host: Add a new API for getting channel doorbell address

[Qiang Yu]

Some controllers may want to know the address of a certain doorbell. Hence
add a new API where we read CHDBOFF register to get the base address of
doorbell, so that the controller can calculate the address of the doorbell
it wants by adding additional offset.

Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
---
 drivers/bus/mhi/host/main.c | 17 +++++++++++++++++
 include/linux/mhi.h         |  7 +++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
index 15d657a..947b5ec 100644
--- a/drivers/bus/mhi/host/main.c
+++ b/drivers/bus/mhi/host/main.c
@@ -1691,3 +1691,20 @@ void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
 	}
 }
 EXPORT_SYMBOL_GPL(mhi_unprepare_from_transfer);
+
+int mhi_get_channel_doorbell(struct mhi_controller *mhi_cntrl, u32 *chdb_offset)
+{
+	struct device *dev = &mhi_cntrl->mhi_dev->dev;
+	void __iomem *base = mhi_cntrl->regs;
+	int ret;
+
+	/* Read channel db offset */
+	ret = mhi_read_reg(mhi_cntrl, base, CHDBOFF, chdb_offset);
+	if (ret) {
+		dev_err(dev, "Unable to read CHDBOFF register\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(mhi_get_channel_doorbell);
diff --git a/include/linux/mhi.h b/include/linux/mhi.h
index 8280545..1135142 100644
--- a/include/linux/mhi.h
+++ b/include/linux/mhi.h
@@ -816,4 +816,11 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir,
  */
 bool mhi_queue_is_full(struct mhi_device *mhi_dev, enum dma_data_direction dir);
 
+/**
+ * mhi_get_channel_doorbell - read channel doorbell offset register to get
+ *                            channel doorbell address
+ * @mhi_cntrl: MHI controller
+ * @chdb_offset: channel doorbell address
+ */
+int mhi_get_channel_doorbell(struct mhi_controller *mhi_cntrl, u32 *chdb_offset);
 #endif /* _MHI_H_ */
-- 
2.7.4

Re: [PATCH v3 2/3] bus: mhi: host: Add a new API for getting channel doorbell address

[Manivannan Sadhasivam]

On Mon, Apr 15, 2024 at 04:49:04PM +0800, Qiang Yu wrote:
> Some controllers may want to know the address of a certain doorbell. Hence
> add a new API where we read CHDBOFF register to get the base address of
> doorbell, so that the controller can calculate the address of the doorbell
> it wants by adding additional offset.
> 
> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
> ---
>  drivers/bus/mhi/host/main.c | 17 +++++++++++++++++
>  include/linux/mhi.h         |  7 +++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c
> index 15d657a..947b5ec 100644
> --- a/drivers/bus/mhi/host/main.c
> +++ b/drivers/bus/mhi/host/main.c
> @@ -1691,3 +1691,20 @@ void mhi_unprepare_from_transfer(struct mhi_device *mhi_dev)
>  	}
>  }
>  EXPORT_SYMBOL_GPL(mhi_unprepare_from_transfer);
> +
> +int mhi_get_channel_doorbell(struct mhi_controller *mhi_cntrl, u32 *chdb_offset)

s/mhi_get_channel_doorbell/mhi_get_channel_doorbell_offset

> +{
> +	struct device *dev = &mhi_cntrl->mhi_dev->dev;
> +	void __iomem *base = mhi_cntrl->regs;
> +	int ret;
> +
> +	/* Read channel db offset */

No need of this comment.

> +	ret = mhi_read_reg(mhi_cntrl, base, CHDBOFF, chdb_offset);
> +	if (ret) {
> +		dev_err(dev, "Unable to read CHDBOFF register\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}

Why can't you use this API in mhi_init_mmio()?

> +EXPORT_SYMBOL_GPL(mhi_get_channel_doorbell);
> diff --git a/include/linux/mhi.h b/include/linux/mhi.h
> index 8280545..1135142 100644
> --- a/include/linux/mhi.h
> +++ b/include/linux/mhi.h
> @@ -816,4 +816,11 @@ int mhi_queue_skb(struct mhi_device *mhi_dev, enum dma_data_direction dir,
>   */
>  bool mhi_queue_is_full(struct mhi_device *mhi_dev, enum dma_data_direction dir);
>  
> +/**
> + * mhi_get_channel_doorbell - read channel doorbell offset register to get

'Get the channel doorbell offset'

> + *                            channel doorbell address
> + * @mhi_cntrl: MHI controller
> + * @chdb_offset: channel doorbell address

s/channel doorbell address/Channel doorbell offset

- Mani

-- 
மணிவண்ணன் சதாசிவம்

[PATCH v3 3/3] bus: mhi: host: pci_generic: Add edl callback to enter EDL

[Qiang Yu]

Add mhi_pci_generic_edl_trigger as edl_trigger for some devices (eg. SDX65)
to enter EDL mode by writing the 0xEDEDEDED cookie to the channel 91
doorbell register and forcing an SOC reset afterwards.

Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
---
 drivers/bus/mhi/host/pci_generic.c | 47 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
index 51639bf..cbf8a58 100644
--- a/drivers/bus/mhi/host/pci_generic.c
+++ b/drivers/bus/mhi/host/pci_generic.c
@@ -27,12 +27,19 @@
 #define PCI_VENDOR_ID_THALES	0x1269
 #define PCI_VENDOR_ID_QUECTEL	0x1eac
 
+#define MHI_EDL_DB			91
+#define MHI_EDL_COOKIE			0xEDEDEDED
+
+/* Device can enter EDL by first setting edl cookie then issuing inband reset*/
+#define MHI_PCI_GENERIC_EDL_TRIGGER	BIT(0)
+
 /**
  * struct mhi_pci_dev_info - MHI PCI device specific information
  * @config: MHI controller configuration
  * @name: name of the PCI module
  * @fw: firmware path (if any)
  * @edl: emergency download mode firmware path (if any)
+ * @edl_trigger: each bit represents a different way to enter EDL
  * @bar_num: PCI base address register to use for MHI MMIO register space
  * @dma_data_width: DMA transfer word size (32 or 64 bits)
  * @mru_default: default MRU size for MBIM network packets
@@ -44,6 +51,7 @@ struct mhi_pci_dev_info {
 	const char *name;
 	const char *fw;
 	const char *edl;
+	unsigned int edl_trigger;
 	unsigned int bar_num;
 	unsigned int dma_data_width;
 	unsigned int mru_default;
@@ -292,6 +300,7 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx75_info = {
 	.name = "qcom-sdx75m",
 	.fw = "qcom/sdx75m/xbl.elf",
 	.edl = "qcom/sdx75m/edl.mbn",
+	.edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
 	.config = &modem_qcom_v2_mhiv_config,
 	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
 	.dma_data_width = 32,
@@ -302,6 +311,7 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx65_info = {
 	.name = "qcom-sdx65m",
 	.fw = "qcom/sdx65m/xbl.elf",
 	.edl = "qcom/sdx65m/edl.mbn",
+	.edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
 	.config = &modem_qcom_v1_mhiv_config,
 	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
 	.dma_data_width = 32,
@@ -312,6 +322,7 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx55_info = {
 	.name = "qcom-sdx55m",
 	.fw = "qcom/sdx55m/sbl1.mbn",
 	.edl = "qcom/sdx55m/edl.mbn",
+	.edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
 	.config = &modem_qcom_v1_mhiv_config,
 	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
 	.dma_data_width = 32,
@@ -928,6 +939,39 @@ static void health_check(struct timer_list *t)
 	mod_timer(&mhi_pdev->health_check_timer, jiffies + HEALTH_CHECK_PERIOD);
 }
 
+static int mhi_pci_generic_edl_trigger(struct mhi_controller *mhi_cntrl)
+{
+	void __iomem *base = mhi_cntrl->regs;
+	void __iomem *edl_db;
+	int ret;
+	u32 val;
+
+	ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
+	if (ret) {
+		dev_err(mhi_cntrl->cntrl_dev, "Wake up device fail before trigger EDL\n");
+		return ret;
+	}
+
+	pm_wakeup_event(&mhi_cntrl->mhi_dev->dev, 0);
+	mhi_cntrl->runtime_get(mhi_cntrl);
+
+	ret = mhi_get_channel_doorbell(mhi_cntrl, &val);
+	if (ret)
+		return ret;
+
+	edl_db = base + val + (8 * MHI_EDL_DB);
+
+	mhi_cntrl->write_reg(mhi_cntrl, edl_db + 4, upper_32_bits(MHI_EDL_COOKIE));
+	mhi_cntrl->write_reg(mhi_cntrl, edl_db, lower_32_bits(MHI_EDL_COOKIE));
+
+	mhi_soc_reset(mhi_cntrl);
+
+	mhi_cntrl->runtime_put(mhi_cntrl);
+	mhi_device_put(mhi_cntrl->mhi_dev);
+
+	return 0;
+}
+
 static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data;
@@ -962,6 +1006,9 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	mhi_cntrl->runtime_put = mhi_pci_runtime_put;
 	mhi_cntrl->mru = info->mru_default;
 
+	if (info->edl_trigger & MHI_PCI_GENERIC_EDL_TRIGGER)
+		mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
+
 	if (info->sideband_wake) {
 		mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
 		mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
-- 
2.7.4

Re: [PATCH v3 3/3] bus: mhi: host: pci_generic: Add edl callback to enter EDL

[Manivannan Sadhasivam]

On Mon, Apr 15, 2024 at 04:49:05PM +0800, Qiang Yu wrote:

bus: mhi: host: pci_generic: Add support for triggering EDL mode in modems

> Add mhi_pci_generic_edl_trigger as edl_trigger for some devices (eg. SDX65)
> to enter EDL mode by writing the 0xEDEDEDED cookie to the channel 91
> doorbell register and forcing an SOC reset afterwards.
> 

'Some of the MHI modems like SDX65 based ones are capable of entering the EDL
mode as per the standard triggering mechanism defined in the MHI spec <enter
spec version>. So let's add a common mhi_pci_generic_edl_trigger() function that
triggers the EDL mode in the device when user writes to the <insert full sysfs
entry here> file.'

> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
> ---
>  drivers/bus/mhi/host/pci_generic.c | 47 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
> index 51639bf..cbf8a58 100644
> --- a/drivers/bus/mhi/host/pci_generic.c
> +++ b/drivers/bus/mhi/host/pci_generic.c
> @@ -27,12 +27,19 @@
>  #define PCI_VENDOR_ID_THALES	0x1269
>  #define PCI_VENDOR_ID_QUECTEL	0x1eac
>  
> +#define MHI_EDL_DB			91
> +#define MHI_EDL_COOKIE			0xEDEDEDED
> +
> +/* Device can enter EDL by first setting edl cookie then issuing inband reset*/
> +#define MHI_PCI_GENERIC_EDL_TRIGGER	BIT(0)

This is not needed as of now. Let the edl_trigger be bool for now. When vendors
want to add their own methods of triggering EDL, we can extend it.

> +
>  /**
>   * struct mhi_pci_dev_info - MHI PCI device specific information
>   * @config: MHI controller configuration
>   * @name: name of the PCI module
>   * @fw: firmware path (if any)
>   * @edl: emergency download mode firmware path (if any)
> + * @edl_trigger: each bit represents a different way to enter EDL

'capable of triggering EDL mode in the device (if supported)'

>   * @bar_num: PCI base address register to use for MHI MMIO register space
>   * @dma_data_width: DMA transfer word size (32 or 64 bits)
>   * @mru_default: default MRU size for MBIM network packets
> @@ -44,6 +51,7 @@ struct mhi_pci_dev_info {
>  	const char *name;
>  	const char *fw;
>  	const char *edl;
> +	unsigned int edl_trigger;
>  	unsigned int bar_num;
>  	unsigned int dma_data_width;
>  	unsigned int mru_default;
> @@ -292,6 +300,7 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx75_info = {
>  	.name = "qcom-sdx75m",
>  	.fw = "qcom/sdx75m/xbl.elf",
>  	.edl = "qcom/sdx75m/edl.mbn",
> +	.edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>  	.config = &modem_qcom_v2_mhiv_config,
>  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>  	.dma_data_width = 32,
> @@ -302,6 +311,7 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx65_info = {
>  	.name = "qcom-sdx65m",
>  	.fw = "qcom/sdx65m/xbl.elf",
>  	.edl = "qcom/sdx65m/edl.mbn",
> +	.edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>  	.config = &modem_qcom_v1_mhiv_config,
>  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>  	.dma_data_width = 32,
> @@ -312,6 +322,7 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx55_info = {
>  	.name = "qcom-sdx55m",
>  	.fw = "qcom/sdx55m/sbl1.mbn",
>  	.edl = "qcom/sdx55m/edl.mbn",
> +	.edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>  	.config = &modem_qcom_v1_mhiv_config,
>  	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>  	.dma_data_width = 32,
> @@ -928,6 +939,39 @@ static void health_check(struct timer_list *t)
>  	mod_timer(&mhi_pdev->health_check_timer, jiffies + HEALTH_CHECK_PERIOD);
>  }
>  
> +static int mhi_pci_generic_edl_trigger(struct mhi_controller *mhi_cntrl)
> +{
> +	void __iomem *base = mhi_cntrl->regs;
> +	void __iomem *edl_db;
> +	int ret;
> +	u32 val;
> +
> +	ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
> +	if (ret) {
> +		dev_err(mhi_cntrl->cntrl_dev, "Wake up device fail before trigger EDL\n");

'Failed to wakeup the device'

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v3 3/3] bus: mhi: host: pci_generic: Add edl callback to enter EDL

[Mayank Rana]

Hi Qiang

On 4/15/2024 1:49 AM, Qiang Yu wrote:
> Add mhi_pci_generic_edl_trigger as edl_trigger for some devices (eg. SDX65)
> to enter EDL mode by writing the 0xEDEDEDED cookie to the channel 91
> doorbell register and forcing an SOC reset afterwards.
> 
> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
> ---
>   drivers/bus/mhi/host/pci_generic.c | 47 ++++++++++++++++++++++++++++++++++++++
>   1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/bus/mhi/host/pci_generic.c b/drivers/bus/mhi/host/pci_generic.c
> index 51639bf..cbf8a58 100644
> --- a/drivers/bus/mhi/host/pci_generic.c
> +++ b/drivers/bus/mhi/host/pci_generic.c
> @@ -27,12 +27,19 @@
>   #define PCI_VENDOR_ID_THALES	0x1269
>   #define PCI_VENDOR_ID_QUECTEL	0x1eac
>   
> +#define MHI_EDL_DB			91
> +#define MHI_EDL_COOKIE			0xEDEDEDED
> +
> +/* Device can enter EDL by first setting edl cookie then issuing inband reset*/
> +#define MHI_PCI_GENERIC_EDL_TRIGGER	BIT(0)
> +
>   /**
>    * struct mhi_pci_dev_info - MHI PCI device specific information
>    * @config: MHI controller configuration
>    * @name: name of the PCI module
>    * @fw: firmware path (if any)
>    * @edl: emergency download mode firmware path (if any)
> + * @edl_trigger: each bit represents a different way to enter EDL
>    * @bar_num: PCI base address register to use for MHI MMIO register space
>    * @dma_data_width: DMA transfer word size (32 or 64 bits)
>    * @mru_default: default MRU size for MBIM network packets
> @@ -44,6 +51,7 @@ struct mhi_pci_dev_info {
>   	const char *name;
>   	const char *fw;
>   	const char *edl;
> +	unsigned int edl_trigger;
>   	unsigned int bar_num;
>   	unsigned int dma_data_width;
>   	unsigned int mru_default;
> @@ -292,6 +300,7 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx75_info = {
>   	.name = "qcom-sdx75m",
>   	.fw = "qcom/sdx75m/xbl.elf",
>   	.edl = "qcom/sdx75m/edl.mbn",
> +	.edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>   	.config = &modem_qcom_v2_mhiv_config,
>   	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>   	.dma_data_width = 32,
> @@ -302,6 +311,7 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx65_info = {
>   	.name = "qcom-sdx65m",
>   	.fw = "qcom/sdx65m/xbl.elf",
>   	.edl = "qcom/sdx65m/edl.mbn",
> +	.edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>   	.config = &modem_qcom_v1_mhiv_config,
>   	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>   	.dma_data_width = 32,
> @@ -312,6 +322,7 @@ static const struct mhi_pci_dev_info mhi_qcom_sdx55_info = {
>   	.name = "qcom-sdx55m",
>   	.fw = "qcom/sdx55m/sbl1.mbn",
>   	.edl = "qcom/sdx55m/edl.mbn",
> +	.edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>   	.config = &modem_qcom_v1_mhiv_config,
>   	.bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>   	.dma_data_width = 32,
> @@ -928,6 +939,39 @@ static void health_check(struct timer_list *t)
>   	mod_timer(&mhi_pdev->health_check_timer, jiffies + HEALTH_CHECK_PERIOD);
>   }
>   
> +static int mhi_pci_generic_edl_trigger(struct mhi_controller *mhi_cntrl)
> +{
> +	void __iomem *base = mhi_cntrl->regs;
> +	void __iomem *edl_db;
> +	int ret;
> +	u32 val;
> +
> +	ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
> +	if (ret) {
> +		dev_err(mhi_cntrl->cntrl_dev, "Wake up device fail before trigger EDL\n");
> +		return ret;
> +	}
> +
> +	pm_wakeup_event(&mhi_cntrl->mhi_dev->dev, 0);
> +	mhi_cntrl->runtime_get(mhi_cntrl);
> +
> +	ret = mhi_get_channel_doorbell(mhi_cntrl, &val);
> +	if (ret)
> +		return ret;
Don't we need error handling part here i.e. calling 
mhi_cntrl->runtime_put() as well mhi_device_put() ?
> +	edl_db = base + val + (8 * MHI_EDL_DB);
> +
> +	mhi_cntrl->write_reg(mhi_cntrl, edl_db + 4, upper_32_bits(MHI_EDL_COOKIE));
> +	mhi_cntrl->write_reg(mhi_cntrl, edl_db, lower_32_bits(MHI_EDL_COOKIE));
> +
> +	mhi_soc_reset(mhi_cntrl);
> +
> +	mhi_cntrl->runtime_put(mhi_cntrl);
> +	mhi_device_put(mhi_cntrl->mhi_dev);
> +
> +	return 0;
> +}
> +
>   static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   {
>   	const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info *) id->driver_data;
> @@ -962,6 +1006,9 @@ static int mhi_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>   	mhi_cntrl->runtime_put = mhi_pci_runtime_put;
>   	mhi_cntrl->mru = info->mru_default;
>   
> +	if (info->edl_trigger & MHI_PCI_GENERIC_EDL_TRIGGER)
> +		mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
> +
>   	if (info->sideband_wake) {
>   		mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
>   		mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
Regards,
Mayank

Re: [PATCH v3 3/3] bus: mhi: host: pci_generic: Add edl callback to enter EDL

[Qiang Yu]

On 4/16/2024 7:53 AM, Mayank Rana wrote:
> Hi Qiang
>
> On 4/15/2024 1:49 AM, Qiang Yu wrote:
>> Add mhi_pci_generic_edl_trigger as edl_trigger for some devices (eg. 
>> SDX65)
>> to enter EDL mode by writing the 0xEDEDEDED cookie to the channel 91
>> doorbell register and forcing an SOC reset afterwards.
>>
>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>> ---
>>   drivers/bus/mhi/host/pci_generic.c | 47 
>> ++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 47 insertions(+)
>>
>> diff --git a/drivers/bus/mhi/host/pci_generic.c 
>> b/drivers/bus/mhi/host/pci_generic.c
>> index 51639bf..cbf8a58 100644
>> --- a/drivers/bus/mhi/host/pci_generic.c
>> +++ b/drivers/bus/mhi/host/pci_generic.c
>> @@ -27,12 +27,19 @@
>>   #define PCI_VENDOR_ID_THALES    0x1269
>>   #define PCI_VENDOR_ID_QUECTEL    0x1eac
>>   +#define MHI_EDL_DB            91
>> +#define MHI_EDL_COOKIE            0xEDEDEDED
>> +
>> +/* Device can enter EDL by first setting edl cookie then issuing 
>> inband reset*/
>> +#define MHI_PCI_GENERIC_EDL_TRIGGER    BIT(0)
>> +
>>   /**
>>    * struct mhi_pci_dev_info - MHI PCI device specific information
>>    * @config: MHI controller configuration
>>    * @name: name of the PCI module
>>    * @fw: firmware path (if any)
>>    * @edl: emergency download mode firmware path (if any)
>> + * @edl_trigger: each bit represents a different way to enter EDL
>>    * @bar_num: PCI base address register to use for MHI MMIO register 
>> space
>>    * @dma_data_width: DMA transfer word size (32 or 64 bits)
>>    * @mru_default: default MRU size for MBIM network packets
>> @@ -44,6 +51,7 @@ struct mhi_pci_dev_info {
>>       const char *name;
>>       const char *fw;
>>       const char *edl;
>> +    unsigned int edl_trigger;
>>       unsigned int bar_num;
>>       unsigned int dma_data_width;
>>       unsigned int mru_default;
>> @@ -292,6 +300,7 @@ static const struct mhi_pci_dev_info 
>> mhi_qcom_sdx75_info = {
>>       .name = "qcom-sdx75m",
>>       .fw = "qcom/sdx75m/xbl.elf",
>>       .edl = "qcom/sdx75m/edl.mbn",
>> +    .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>       .config = &modem_qcom_v2_mhiv_config,
>>       .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>       .dma_data_width = 32,
>> @@ -302,6 +311,7 @@ static const struct mhi_pci_dev_info 
>> mhi_qcom_sdx65_info = {
>>       .name = "qcom-sdx65m",
>>       .fw = "qcom/sdx65m/xbl.elf",
>>       .edl = "qcom/sdx65m/edl.mbn",
>> +    .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>       .config = &modem_qcom_v1_mhiv_config,
>>       .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>       .dma_data_width = 32,
>> @@ -312,6 +322,7 @@ static const struct mhi_pci_dev_info 
>> mhi_qcom_sdx55_info = {
>>       .name = "qcom-sdx55m",
>>       .fw = "qcom/sdx55m/sbl1.mbn",
>>       .edl = "qcom/sdx55m/edl.mbn",
>> +    .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>       .config = &modem_qcom_v1_mhiv_config,
>>       .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>       .dma_data_width = 32,
>> @@ -928,6 +939,39 @@ static void health_check(struct timer_list *t)
>>       mod_timer(&mhi_pdev->health_check_timer, jiffies + 
>> HEALTH_CHECK_PERIOD);
>>   }
>>   +static int mhi_pci_generic_edl_trigger(struct mhi_controller 
>> *mhi_cntrl)
>> +{
>> +    void __iomem *base = mhi_cntrl->regs;
>> +    void __iomem *edl_db;
>> +    int ret;
>> +    u32 val;
>> +
>> +    ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
>> +    if (ret) {
>> +        dev_err(mhi_cntrl->cntrl_dev, "Wake up device fail before 
>> trigger EDL\n");
>> +        return ret;
>> +    }
>> +
>> +    pm_wakeup_event(&mhi_cntrl->mhi_dev->dev, 0);
>> +    mhi_cntrl->runtime_get(mhi_cntrl);
>> +
>> +    ret = mhi_get_channel_doorbell(mhi_cntrl, &val);
>> +    if (ret)
>> +        return ret;
> Don't we need error handling part here i.e. calling 
> mhi_cntrl->runtime_put() as well mhi_device_put() ?

Hi Mayank

After soc_reset, device will reboot to EDL mode and MHI state will be 
SYSERR. So host will fail to suspend
anyway. The "error handling" we need here is actually to enter EDL mode, 
this will be done by SYSERR irq.
Here, mhi_cntrl->runtime_put() and mhi_device_put() are only to balance 
mhi_cntrl->runtime_get() and
mhi_device_get_sync().

Thanks,
Qiang

>> +    edl_db = base + val + (8 * MHI_EDL_DB);
>> +
>> +    mhi_cntrl->write_reg(mhi_cntrl, edl_db + 4, 
>> upper_32_bits(MHI_EDL_COOKIE));
>> +    mhi_cntrl->write_reg(mhi_cntrl, edl_db, 
>> lower_32_bits(MHI_EDL_COOKIE));
>> +
>> +    mhi_soc_reset(mhi_cntrl);
>> +
>> +    mhi_cntrl->runtime_put(mhi_cntrl);
>> +    mhi_device_put(mhi_cntrl->mhi_dev);
>> +
>> +    return 0;
>> +}
>> +
>>   static int mhi_pci_probe(struct pci_dev *pdev, const struct 
>> pci_device_id *id)
>>   {
>>       const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info 
>> *) id->driver_data;
>> @@ -962,6 +1006,9 @@ static int mhi_pci_probe(struct pci_dev *pdev, 
>> const struct pci_device_id *id)
>>       mhi_cntrl->runtime_put = mhi_pci_runtime_put;
>>       mhi_cntrl->mru = info->mru_default;
>>   +    if (info->edl_trigger & MHI_PCI_GENERIC_EDL_TRIGGER)
>> +        mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
>> +
>>       if (info->sideband_wake) {
>>           mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
>>           mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
> Regards,
> Mayank

Re: [PATCH v3 3/3] bus: mhi: host: pci_generic: Add edl callback to enter EDL

[Mayank Rana]

On 4/15/2024 8:50 PM, Qiang Yu wrote:
> 
> On 4/16/2024 7:53 AM, Mayank Rana wrote:
>> Hi Qiang
>>
>> On 4/15/2024 1:49 AM, Qiang Yu wrote:
>>> Add mhi_pci_generic_edl_trigger as edl_trigger for some devices (eg. 
>>> SDX65)
>>> to enter EDL mode by writing the 0xEDEDEDED cookie to the channel 91
>>> doorbell register and forcing an SOC reset afterwards.
>>>
>>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>>> ---
>>>   drivers/bus/mhi/host/pci_generic.c | 47 
>>> ++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 47 insertions(+)
>>>
>>> diff --git a/drivers/bus/mhi/host/pci_generic.c 
>>> b/drivers/bus/mhi/host/pci_generic.c
>>> index 51639bf..cbf8a58 100644
>>> --- a/drivers/bus/mhi/host/pci_generic.c
>>> +++ b/drivers/bus/mhi/host/pci_generic.c
>>> @@ -27,12 +27,19 @@
>>>   #define PCI_VENDOR_ID_THALES    0x1269
>>>   #define PCI_VENDOR_ID_QUECTEL    0x1eac
>>>   +#define MHI_EDL_DB            91
>>> +#define MHI_EDL_COOKIE            0xEDEDEDED
>>> +
>>> +/* Device can enter EDL by first setting edl cookie then issuing 
>>> inband reset*/
>>> +#define MHI_PCI_GENERIC_EDL_TRIGGER    BIT(0)
>>> +
>>>   /**
>>>    * struct mhi_pci_dev_info - MHI PCI device specific information
>>>    * @config: MHI controller configuration
>>>    * @name: name of the PCI module
>>>    * @fw: firmware path (if any)
>>>    * @edl: emergency download mode firmware path (if any)
>>> + * @edl_trigger: each bit represents a different way to enter EDL
>>>    * @bar_num: PCI base address register to use for MHI MMIO register 
>>> space
>>>    * @dma_data_width: DMA transfer word size (32 or 64 bits)
>>>    * @mru_default: default MRU size for MBIM network packets
>>> @@ -44,6 +51,7 @@ struct mhi_pci_dev_info {
>>>       const char *name;
>>>       const char *fw;
>>>       const char *edl;
>>> +    unsigned int edl_trigger;
>>>       unsigned int bar_num;
>>>       unsigned int dma_data_width;
>>>       unsigned int mru_default;
>>> @@ -292,6 +300,7 @@ static const struct mhi_pci_dev_info 
>>> mhi_qcom_sdx75_info = {
>>>       .name = "qcom-sdx75m",
>>>       .fw = "qcom/sdx75m/xbl.elf",
>>>       .edl = "qcom/sdx75m/edl.mbn",
>>> +    .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>>       .config = &modem_qcom_v2_mhiv_config,
>>>       .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>>       .dma_data_width = 32,
>>> @@ -302,6 +311,7 @@ static const struct mhi_pci_dev_info 
>>> mhi_qcom_sdx65_info = {
>>>       .name = "qcom-sdx65m",
>>>       .fw = "qcom/sdx65m/xbl.elf",
>>>       .edl = "qcom/sdx65m/edl.mbn",
>>> +    .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>>       .config = &modem_qcom_v1_mhiv_config,
>>>       .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>>       .dma_data_width = 32,
>>> @@ -312,6 +322,7 @@ static const struct mhi_pci_dev_info 
>>> mhi_qcom_sdx55_info = {
>>>       .name = "qcom-sdx55m",
>>>       .fw = "qcom/sdx55m/sbl1.mbn",
>>>       .edl = "qcom/sdx55m/edl.mbn",
>>> +    .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>>       .config = &modem_qcom_v1_mhiv_config,
>>>       .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>>       .dma_data_width = 32,
>>> @@ -928,6 +939,39 @@ static void health_check(struct timer_list *t)
>>>       mod_timer(&mhi_pdev->health_check_timer, jiffies + 
>>> HEALTH_CHECK_PERIOD);
>>>   }
>>>   +static int mhi_pci_generic_edl_trigger(struct mhi_controller 
>>> *mhi_cntrl)
>>> +{
>>> +    void __iomem *base = mhi_cntrl->regs;
>>> +    void __iomem *edl_db;
>>> +    int ret;
>>> +    u32 val;
>>> +
>>> +    ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
>>> +    if (ret) {
>>> +        dev_err(mhi_cntrl->cntrl_dev, "Wake up device fail before 
>>> trigger EDL\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    pm_wakeup_event(&mhi_cntrl->mhi_dev->dev, 0);
>>> +    mhi_cntrl->runtime_get(mhi_cntrl);
>>> +
>>> +    ret = mhi_get_channel_doorbell(mhi_cntrl, &val);
>>> +    if (ret)
>>> +        return ret;
>> Don't we need error handling part here i.e. calling 
>> mhi_cntrl->runtime_put() as well mhi_device_put() ?
> 
> Hi Mayank
> 
> After soc_reset, device will reboot to EDL mode and MHI state will be 
> SYSERR. So host will fail to suspend
> anyway. The "error handling" we need here is actually to enter EDL mode, 
> this will be done by SYSERR irq.
> Here, mhi_cntrl->runtime_put() and mhi_device_put() are only to balance 
> mhi_cntrl->runtime_get() and
> mhi_device_get_sync().
> 
> Thanks,
> Qiang
I am saying is there possibility that mhi_get_channel_doorbell() returns 
error ?
If yes, then why don't we need error handling as part of it. you are 
exiting if this API return error without doing anything.
>>> +    edl_db = base + val + (8 * MHI_EDL_DB);
>>> +
>>> +    mhi_cntrl->write_reg(mhi_cntrl, edl_db + 4, 
>>> upper_32_bits(MHI_EDL_COOKIE));
>>> +    mhi_cntrl->write_reg(mhi_cntrl, edl_db, 
>>> lower_32_bits(MHI_EDL_COOKIE));
>>> +
>>> +    mhi_soc_reset(mhi_cntrl);
>>> +
>>> +    mhi_cntrl->runtime_put(mhi_cntrl);
>>> +    mhi_device_put(mhi_cntrl->mhi_dev);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static int mhi_pci_probe(struct pci_dev *pdev, const struct 
>>> pci_device_id *id)
>>>   {
>>>       const struct mhi_pci_dev_info *info = (struct mhi_pci_dev_info 
>>> *) id->driver_data;
>>> @@ -962,6 +1006,9 @@ static int mhi_pci_probe(struct pci_dev *pdev, 
>>> const struct pci_device_id *id)
>>>       mhi_cntrl->runtime_put = mhi_pci_runtime_put;
>>>       mhi_cntrl->mru = info->mru_default;
>>>   +    if (info->edl_trigger & MHI_PCI_GENERIC_EDL_TRIGGER)
>>> +        mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
>>> +
>>>       if (info->sideband_wake) {
>>>           mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
>>>           mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
>> Regards,
>> Mayank

Re: [PATCH v3 3/3] bus: mhi: host: pci_generic: Add edl callback to enter EDL

[Qiang Yu]

On 4/17/2024 2:12 AM, Mayank Rana wrote:
>
>
> On 4/15/2024 8:50 PM, Qiang Yu wrote:
>>
>> On 4/16/2024 7:53 AM, Mayank Rana wrote:
>>> Hi Qiang
>>>
>>> On 4/15/2024 1:49 AM, Qiang Yu wrote:
>>>> Add mhi_pci_generic_edl_trigger as edl_trigger for some devices 
>>>> (eg. SDX65)
>>>> to enter EDL mode by writing the 0xEDEDEDED cookie to the channel 91
>>>> doorbell register and forcing an SOC reset afterwards.
>>>>
>>>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>>>> ---
>>>>   drivers/bus/mhi/host/pci_generic.c | 47 
>>>> ++++++++++++++++++++++++++++++++++++++
>>>>   1 file changed, 47 insertions(+)
>>>>
>>>> diff --git a/drivers/bus/mhi/host/pci_generic.c 
>>>> b/drivers/bus/mhi/host/pci_generic.c
>>>> index 51639bf..cbf8a58 100644
>>>> --- a/drivers/bus/mhi/host/pci_generic.c
>>>> +++ b/drivers/bus/mhi/host/pci_generic.c
>>>> @@ -27,12 +27,19 @@
>>>>   #define PCI_VENDOR_ID_THALES    0x1269
>>>>   #define PCI_VENDOR_ID_QUECTEL    0x1eac
>>>>   +#define MHI_EDL_DB            91
>>>> +#define MHI_EDL_COOKIE            0xEDEDEDED
>>>> +
>>>> +/* Device can enter EDL by first setting edl cookie then issuing 
>>>> inband reset*/
>>>> +#define MHI_PCI_GENERIC_EDL_TRIGGER    BIT(0)
>>>> +
>>>>   /**
>>>>    * struct mhi_pci_dev_info - MHI PCI device specific information
>>>>    * @config: MHI controller configuration
>>>>    * @name: name of the PCI module
>>>>    * @fw: firmware path (if any)
>>>>    * @edl: emergency download mode firmware path (if any)
>>>> + * @edl_trigger: each bit represents a different way to enter EDL
>>>>    * @bar_num: PCI base address register to use for MHI MMIO 
>>>> register space
>>>>    * @dma_data_width: DMA transfer word size (32 or 64 bits)
>>>>    * @mru_default: default MRU size for MBIM network packets
>>>> @@ -44,6 +51,7 @@ struct mhi_pci_dev_info {
>>>>       const char *name;
>>>>       const char *fw;
>>>>       const char *edl;
>>>> +    unsigned int edl_trigger;
>>>>       unsigned int bar_num;
>>>>       unsigned int dma_data_width;
>>>>       unsigned int mru_default;
>>>> @@ -292,6 +300,7 @@ static const struct mhi_pci_dev_info 
>>>> mhi_qcom_sdx75_info = {
>>>>       .name = "qcom-sdx75m",
>>>>       .fw = "qcom/sdx75m/xbl.elf",
>>>>       .edl = "qcom/sdx75m/edl.mbn",
>>>> +    .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>>>       .config = &modem_qcom_v2_mhiv_config,
>>>>       .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>>>       .dma_data_width = 32,
>>>> @@ -302,6 +311,7 @@ static const struct mhi_pci_dev_info 
>>>> mhi_qcom_sdx65_info = {
>>>>       .name = "qcom-sdx65m",
>>>>       .fw = "qcom/sdx65m/xbl.elf",
>>>>       .edl = "qcom/sdx65m/edl.mbn",
>>>> +    .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>>>       .config = &modem_qcom_v1_mhiv_config,
>>>>       .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>>>       .dma_data_width = 32,
>>>> @@ -312,6 +322,7 @@ static const struct mhi_pci_dev_info 
>>>> mhi_qcom_sdx55_info = {
>>>>       .name = "qcom-sdx55m",
>>>>       .fw = "qcom/sdx55m/sbl1.mbn",
>>>>       .edl = "qcom/sdx55m/edl.mbn",
>>>> +    .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>>>       .config = &modem_qcom_v1_mhiv_config,
>>>>       .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>>>       .dma_data_width = 32,
>>>> @@ -928,6 +939,39 @@ static void health_check(struct timer_list *t)
>>>>       mod_timer(&mhi_pdev->health_check_timer, jiffies + 
>>>> HEALTH_CHECK_PERIOD);
>>>>   }
>>>>   +static int mhi_pci_generic_edl_trigger(struct mhi_controller 
>>>> *mhi_cntrl)
>>>> +{
>>>> +    void __iomem *base = mhi_cntrl->regs;
>>>> +    void __iomem *edl_db;
>>>> +    int ret;
>>>> +    u32 val;
>>>> +
>>>> +    ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
>>>> +    if (ret) {
>>>> +        dev_err(mhi_cntrl->cntrl_dev, "Wake up device fail before 
>>>> trigger EDL\n");
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    pm_wakeup_event(&mhi_cntrl->mhi_dev->dev, 0);
>>>> +    mhi_cntrl->runtime_get(mhi_cntrl);
>>>> +
>>>> +    ret = mhi_get_channel_doorbell(mhi_cntrl, &val);
>>>> +    if (ret)
>>>> +        return ret;
>>> Don't we need error handling part here i.e. calling 
>>> mhi_cntrl->runtime_put() as well mhi_device_put() ?
>>
>> Hi Mayank
>>
>> After soc_reset, device will reboot to EDL mode and MHI state will be 
>> SYSERR. So host will fail to suspend
>> anyway. The "error handling" we need here is actually to enter EDL 
>> mode, this will be done by SYSERR irq.
>> Here, mhi_cntrl->runtime_put() and mhi_device_put() are only to 
>> balance mhi_cntrl->runtime_get() and
>> mhi_device_get_sync().
>>
>> Thanks,
>> Qiang
> I am saying is there possibility that mhi_get_channel_doorbell() 
> returns error ?
> If yes, then why don't we need error handling as part of it. you are 
> exiting if this API return error without doing anything.

I think here mhi_get_channel_doorbell will not return error. But I still
add a error checking because it invoked mhi_read_reg, which is a must check
API. For modem mhi controller, this API eventually does a memory read.
This memory read will return a normal value if link is up and all f's if 
link
is bad.

Thanks,
Qiang
>>>> +    edl_db = base + val + (8 * MHI_EDL_DB);
>>>> +
>>>> +    mhi_cntrl->write_reg(mhi_cntrl, edl_db + 4, 
>>>> upper_32_bits(MHI_EDL_COOKIE));
>>>> +    mhi_cntrl->write_reg(mhi_cntrl, edl_db, 
>>>> lower_32_bits(MHI_EDL_COOKIE));
>>>> +
>>>> +    mhi_soc_reset(mhi_cntrl);
>>>> +
>>>> +    mhi_cntrl->runtime_put(mhi_cntrl);
>>>> +    mhi_device_put(mhi_cntrl->mhi_dev);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>   static int mhi_pci_probe(struct pci_dev *pdev, const struct 
>>>> pci_device_id *id)
>>>>   {
>>>>       const struct mhi_pci_dev_info *info = (struct 
>>>> mhi_pci_dev_info *) id->driver_data;
>>>> @@ -962,6 +1006,9 @@ static int mhi_pci_probe(struct pci_dev *pdev, 
>>>> const struct pci_device_id *id)
>>>>       mhi_cntrl->runtime_put = mhi_pci_runtime_put;
>>>>       mhi_cntrl->mru = info->mru_default;
>>>>   +    if (info->edl_trigger & MHI_PCI_GENERIC_EDL_TRIGGER)
>>>> +        mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
>>>> +
>>>>       if (info->sideband_wake) {
>>>>           mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
>>>>           mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
>>> Regards,
>>> Mayank

Re: [PATCH v3 3/3] bus: mhi: host: pci_generic: Add edl callback to enter EDL

[Qiang Yu]

On 4/17/2024 11:01 AM, Qiang Yu wrote:
>
> On 4/17/2024 2:12 AM, Mayank Rana wrote:
>>
>>
>> On 4/15/2024 8:50 PM, Qiang Yu wrote:
>>>
>>> On 4/16/2024 7:53 AM, Mayank Rana wrote:
>>>> Hi Qiang
>>>>
>>>> On 4/15/2024 1:49 AM, Qiang Yu wrote:
>>>>> Add mhi_pci_generic_edl_trigger as edl_trigger for some devices 
>>>>> (eg. SDX65)
>>>>> to enter EDL mode by writing the 0xEDEDEDED cookie to the channel 91
>>>>> doorbell register and forcing an SOC reset afterwards.
>>>>>
>>>>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>>>>> ---
>>>>>   drivers/bus/mhi/host/pci_generic.c | 47 
>>>>> ++++++++++++++++++++++++++++++++++++++
>>>>>   1 file changed, 47 insertions(+)
>>>>>
>>>>> diff --git a/drivers/bus/mhi/host/pci_generic.c 
>>>>> b/drivers/bus/mhi/host/pci_generic.c
>>>>> index 51639bf..cbf8a58 100644
>>>>> --- a/drivers/bus/mhi/host/pci_generic.c
>>>>> +++ b/drivers/bus/mhi/host/pci_generic.c
>>>>> @@ -27,12 +27,19 @@
>>>>>   #define PCI_VENDOR_ID_THALES    0x1269
>>>>>   #define PCI_VENDOR_ID_QUECTEL    0x1eac
>>>>>   +#define MHI_EDL_DB            91
>>>>> +#define MHI_EDL_COOKIE            0xEDEDEDED
>>>>> +
>>>>> +/* Device can enter EDL by first setting edl cookie then issuing 
>>>>> inband reset*/
>>>>> +#define MHI_PCI_GENERIC_EDL_TRIGGER    BIT(0)
>>>>> +
>>>>>   /**
>>>>>    * struct mhi_pci_dev_info - MHI PCI device specific information
>>>>>    * @config: MHI controller configuration
>>>>>    * @name: name of the PCI module
>>>>>    * @fw: firmware path (if any)
>>>>>    * @edl: emergency download mode firmware path (if any)
>>>>> + * @edl_trigger: each bit represents a different way to enter EDL
>>>>>    * @bar_num: PCI base address register to use for MHI MMIO 
>>>>> register space
>>>>>    * @dma_data_width: DMA transfer word size (32 or 64 bits)
>>>>>    * @mru_default: default MRU size for MBIM network packets
>>>>> @@ -44,6 +51,7 @@ struct mhi_pci_dev_info {
>>>>>       const char *name;
>>>>>       const char *fw;
>>>>>       const char *edl;
>>>>> +    unsigned int edl_trigger;
>>>>>       unsigned int bar_num;
>>>>>       unsigned int dma_data_width;
>>>>>       unsigned int mru_default;
>>>>> @@ -292,6 +300,7 @@ static const struct mhi_pci_dev_info 
>>>>> mhi_qcom_sdx75_info = {
>>>>>       .name = "qcom-sdx75m",
>>>>>       .fw = "qcom/sdx75m/xbl.elf",
>>>>>       .edl = "qcom/sdx75m/edl.mbn",
>>>>> +    .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>>>>       .config = &modem_qcom_v2_mhiv_config,
>>>>>       .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>>>>       .dma_data_width = 32,
>>>>> @@ -302,6 +311,7 @@ static const struct mhi_pci_dev_info 
>>>>> mhi_qcom_sdx65_info = {
>>>>>       .name = "qcom-sdx65m",
>>>>>       .fw = "qcom/sdx65m/xbl.elf",
>>>>>       .edl = "qcom/sdx65m/edl.mbn",
>>>>> +    .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>>>>       .config = &modem_qcom_v1_mhiv_config,
>>>>>       .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>>>>       .dma_data_width = 32,
>>>>> @@ -312,6 +322,7 @@ static const struct mhi_pci_dev_info 
>>>>> mhi_qcom_sdx55_info = {
>>>>>       .name = "qcom-sdx55m",
>>>>>       .fw = "qcom/sdx55m/sbl1.mbn",
>>>>>       .edl = "qcom/sdx55m/edl.mbn",
>>>>> +    .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>>>>       .config = &modem_qcom_v1_mhiv_config,
>>>>>       .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>>>>       .dma_data_width = 32,
>>>>> @@ -928,6 +939,39 @@ static void health_check(struct timer_list *t)
>>>>>       mod_timer(&mhi_pdev->health_check_timer, jiffies + 
>>>>> HEALTH_CHECK_PERIOD);
>>>>>   }
>>>>>   +static int mhi_pci_generic_edl_trigger(struct mhi_controller 
>>>>> *mhi_cntrl)
>>>>> +{
>>>>> +    void __iomem *base = mhi_cntrl->regs;
>>>>> +    void __iomem *edl_db;
>>>>> +    int ret;
>>>>> +    u32 val;
>>>>> +
>>>>> +    ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
>>>>> +    if (ret) {
>>>>> +        dev_err(mhi_cntrl->cntrl_dev, "Wake up device fail before 
>>>>> trigger EDL\n");
>>>>> +        return ret;
>>>>> +    }
>>>>> +
>>>>> +    pm_wakeup_event(&mhi_cntrl->mhi_dev->dev, 0);
>>>>> +    mhi_cntrl->runtime_get(mhi_cntrl);
>>>>> +
>>>>> +    ret = mhi_get_channel_doorbell(mhi_cntrl, &val);
>>>>> +    if (ret)
>>>>> +        return ret;
>>>> Don't we need error handling part here i.e. calling 
>>>> mhi_cntrl->runtime_put() as well mhi_device_put() ?
>>>
>>> Hi Mayank
>>>
>>> After soc_reset, device will reboot to EDL mode and MHI state will 
>>> be SYSERR. So host will fail to suspend
>>> anyway. The "error handling" we need here is actually to enter EDL 
>>> mode, this will be done by SYSERR irq.
>>> Here, mhi_cntrl->runtime_put() and mhi_device_put() are only to 
>>> balance mhi_cntrl->runtime_get() and
>>> mhi_device_get_sync().
>>>
>>> Thanks,
>>> Qiang
>> I am saying is there possibility that mhi_get_channel_doorbell() 
>> returns error ?
>> If yes, then why don't we need error handling as part of it. you are 
>> exiting if this API return error without doing anything.
>
> I think here mhi_get_channel_doorbell will not return error. But I still
> add a error checking because it invoked mhi_read_reg, which is a must 
> check
> API. For modem mhi controller, this API eventually does a memory read.
> This memory read will return a normal value if link is up and all f's 
> if link
> is bad.
>
> Thanks,
> Qiang

Actually, mhi_get_channel_doorbell should also be used in mhi_init_mmio to
replace the getting chdb operation by invoking mhi_read_reg as Mani 
commented.
In mhi_init_mmio, we invoke mhi_read_reg many times, but there is also not
additionnal error handling.

I'm not very sure the reason but perhaps if mhi_read_reg returns error 
(I don't
know which controller will provide a memory read callback returning 
errors), most
probably something is wrong in PCIe, which is not predictable by MHI and 
we can
not add err handling every time invoking mhi_read_reg. But we have a 
timer to
do health_check in pci_generic.c. If link is down, we will do 
pci_function_reset
to try to reovery.

Hi Mani, sorry, may I know the purpose of adding must_check attribute to
mhi_read_reg? In which case will mhi controller provide a callback that
returns error?

Thanks,
Qiang
>>>>> +    edl_db = base + val + (8 * MHI_EDL_DB);
>>>>> +
>>>>> +    mhi_cntrl->write_reg(mhi_cntrl, edl_db + 4, 
>>>>> upper_32_bits(MHI_EDL_COOKIE));
>>>>> +    mhi_cntrl->write_reg(mhi_cntrl, edl_db, 
>>>>> lower_32_bits(MHI_EDL_COOKIE));
>>>>> +
>>>>> +    mhi_soc_reset(mhi_cntrl);
>>>>> +
>>>>> +    mhi_cntrl->runtime_put(mhi_cntrl);
>>>>> +    mhi_device_put(mhi_cntrl->mhi_dev);
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>>   static int mhi_pci_probe(struct pci_dev *pdev, const struct 
>>>>> pci_device_id *id)
>>>>>   {
>>>>>       const struct mhi_pci_dev_info *info = (struct 
>>>>> mhi_pci_dev_info *) id->driver_data;
>>>>> @@ -962,6 +1006,9 @@ static int mhi_pci_probe(struct pci_dev 
>>>>> *pdev, const struct pci_device_id *id)
>>>>>       mhi_cntrl->runtime_put = mhi_pci_runtime_put;
>>>>>       mhi_cntrl->mru = info->mru_default;
>>>>>   +    if (info->edl_trigger & MHI_PCI_GENERIC_EDL_TRIGGER)
>>>>> +        mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
>>>>> +
>>>>>       if (info->sideband_wake) {
>>>>>           mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
>>>>>           mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
>>>> Regards,
>>>> Mayank
>

Re: [PATCH v3 3/3] bus: mhi: host: pci_generic: Add edl callback to enter EDL

[Mayank Rana]

On 4/16/2024 9:41 PM, Qiang Yu wrote:
> 
> On 4/17/2024 11:01 AM, Qiang Yu wrote:
>>
>> On 4/17/2024 2:12 AM, Mayank Rana wrote:
>>>
>>>
>>> On 4/15/2024 8:50 PM, Qiang Yu wrote:
>>>>
>>>> On 4/16/2024 7:53 AM, Mayank Rana wrote:
>>>>> Hi Qiang
>>>>>
>>>>> On 4/15/2024 1:49 AM, Qiang Yu wrote:
>>>>>> Add mhi_pci_generic_edl_trigger as edl_trigger for some devices 
>>>>>> (eg. SDX65)
>>>>>> to enter EDL mode by writing the 0xEDEDEDED cookie to the channel 91
>>>>>> doorbell register and forcing an SOC reset afterwards.
>>>>>>
>>>>>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>>>>>> ---
>>>>>>   drivers/bus/mhi/host/pci_generic.c | 47 
>>>>>> ++++++++++++++++++++++++++++++++++++++
>>>>>>   1 file changed, 47 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/bus/mhi/host/pci_generic.c 
>>>>>> b/drivers/bus/mhi/host/pci_generic.c
>>>>>> index 51639bf..cbf8a58 100644
>>>>>> --- a/drivers/bus/mhi/host/pci_generic.c
>>>>>> +++ b/drivers/bus/mhi/host/pci_generic.c
>>>>>> @@ -27,12 +27,19 @@
>>>>>>   #define PCI_VENDOR_ID_THALES    0x1269
>>>>>>   #define PCI_VENDOR_ID_QUECTEL    0x1eac
>>>>>>   +#define MHI_EDL_DB            91
>>>>>> +#define MHI_EDL_COOKIE            0xEDEDEDED
>>>>>> +
>>>>>> +/* Device can enter EDL by first setting edl cookie then issuing 
>>>>>> inband reset*/
>>>>>> +#define MHI_PCI_GENERIC_EDL_TRIGGER    BIT(0)
>>>>>> +
>>>>>>   /**
>>>>>>    * struct mhi_pci_dev_info - MHI PCI device specific information
>>>>>>    * @config: MHI controller configuration
>>>>>>    * @name: name of the PCI module
>>>>>>    * @fw: firmware path (if any)
>>>>>>    * @edl: emergency download mode firmware path (if any)
>>>>>> + * @edl_trigger: each bit represents a different way to enter EDL
>>>>>>    * @bar_num: PCI base address register to use for MHI MMIO 
>>>>>> register space
>>>>>>    * @dma_data_width: DMA transfer word size (32 or 64 bits)
>>>>>>    * @mru_default: default MRU size for MBIM network packets
>>>>>> @@ -44,6 +51,7 @@ struct mhi_pci_dev_info {
>>>>>>       const char *name;
>>>>>>       const char *fw;
>>>>>>       const char *edl;
>>>>>> +    unsigned int edl_trigger;
>>>>>>       unsigned int bar_num;
>>>>>>       unsigned int dma_data_width;
>>>>>>       unsigned int mru_default;
>>>>>> @@ -292,6 +300,7 @@ static const struct mhi_pci_dev_info 
>>>>>> mhi_qcom_sdx75_info = {
>>>>>>       .name = "qcom-sdx75m",
>>>>>>       .fw = "qcom/sdx75m/xbl.elf",
>>>>>>       .edl = "qcom/sdx75m/edl.mbn",
>>>>>> +    .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>>>>>       .config = &modem_qcom_v2_mhiv_config,
>>>>>>       .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>>>>>       .dma_data_width = 32,
>>>>>> @@ -302,6 +311,7 @@ static const struct mhi_pci_dev_info 
>>>>>> mhi_qcom_sdx65_info = {
>>>>>>       .name = "qcom-sdx65m",
>>>>>>       .fw = "qcom/sdx65m/xbl.elf",
>>>>>>       .edl = "qcom/sdx65m/edl.mbn",
>>>>>> +    .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>>>>>       .config = &modem_qcom_v1_mhiv_config,
>>>>>>       .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>>>>>       .dma_data_width = 32,
>>>>>> @@ -312,6 +322,7 @@ static const struct mhi_pci_dev_info 
>>>>>> mhi_qcom_sdx55_info = {
>>>>>>       .name = "qcom-sdx55m",
>>>>>>       .fw = "qcom/sdx55m/sbl1.mbn",
>>>>>>       .edl = "qcom/sdx55m/edl.mbn",
>>>>>> +    .edl_trigger = MHI_PCI_GENERIC_EDL_TRIGGER,
>>>>>>       .config = &modem_qcom_v1_mhiv_config,
>>>>>>       .bar_num = MHI_PCI_DEFAULT_BAR_NUM,
>>>>>>       .dma_data_width = 32,
>>>>>> @@ -928,6 +939,39 @@ static void health_check(struct timer_list *t)
>>>>>>       mod_timer(&mhi_pdev->health_check_timer, jiffies + 
>>>>>> HEALTH_CHECK_PERIOD);
>>>>>>   }
>>>>>>   +static int mhi_pci_generic_edl_trigger(struct mhi_controller 
>>>>>> *mhi_cntrl)
>>>>>> +{
>>>>>> +    void __iomem *base = mhi_cntrl->regs;
>>>>>> +    void __iomem *edl_db;
>>>>>> +    int ret;
>>>>>> +    u32 val;
>>>>>> +
>>>>>> +    ret = mhi_device_get_sync(mhi_cntrl->mhi_dev);
>>>>>> +    if (ret) {
>>>>>> +        dev_err(mhi_cntrl->cntrl_dev, "Wake up device fail before 
>>>>>> trigger EDL\n");
>>>>>> +        return ret;
>>>>>> +    }
>>>>>> +
>>>>>> +    pm_wakeup_event(&mhi_cntrl->mhi_dev->dev, 0);
>>>>>> +    mhi_cntrl->runtime_get(mhi_cntrl);
>>>>>> +
>>>>>> +    ret = mhi_get_channel_doorbell(mhi_cntrl, &val);
>>>>>> +    if (ret)
>>>>>> +        return ret;
>>>>> Don't we need error handling part here i.e. calling 
>>>>> mhi_cntrl->runtime_put() as well mhi_device_put() ?
>>>>
>>>> Hi Mayank
>>>>
>>>> After soc_reset, device will reboot to EDL mode and MHI state will 
>>>> be SYSERR. So host will fail to suspend
>>>> anyway. The "error handling" we need here is actually to enter EDL 
>>>> mode, this will be done by SYSERR irq.
>>>> Here, mhi_cntrl->runtime_put() and mhi_device_put() are only to 
>>>> balance mhi_cntrl->runtime_get() and
>>>> mhi_device_get_sync().
>>>>
>>>> Thanks,
>>>> Qiang
>>> I am saying is there possibility that mhi_get_channel_doorbell() 
>>> returns error ?
>>> If yes, then why don't we need error handling as part of it. you are 
>>> exiting if this API return error without doing anything.
>>
>> I think here mhi_get_channel_doorbell will not return error. But I still
>> add a error checking because it invoked mhi_read_reg, which is a must 
>> check
>> API. For modem mhi controller, this API eventually does a memory read.
>> This memory read will return a normal value if link is up and all f's 
>> if link
>> is bad.
>>
>> Thanks,
>> Qiang
> 
> Actually, mhi_get_channel_doorbell should also be used in mhi_init_mmio to
> replace the getting chdb operation by invoking mhi_read_reg as Mani 
> commented.
> In mhi_init_mmio, we invoke mhi_read_reg many times, but there is also not
> additionnal error handling.
I don't see that any specific step required within this API i.e. doing undo
as it is just reading registers. so there is nothing else needed to be 
performed here as part of register read failure scenario.

> I'm not very sure the reason but perhaps if mhi_read_reg returns error 
> (I don't
> know which controller will provide a memory read callback returning 
> errors), most
> probably something is wrong in PCIe, which is not predictable by MHI and 
> we can
> not add err handling every time invoking mhi_read_reg. But we have a 
> timer to
> do health_check in pci_generic.c. If link is down, we will do 
> pci_function_reset
> to try to reovery.

There are more than 21 places in MHI host stack mhi_read_reg() API based 
error handling
is performed. With this new code addition, why it is supposed to be 
exception here. So these are possible options available here:
1. If you don't want to perform error handling, provide good inline 
comment here saying why you don't want to perform error handling.
2. Remove __must_check annotation with mhi_read_reg() and don't check 
return value here.
3. Other option is to check if device is not into M0, then just return 
without performing anything here.

Regards,
Mayank

> Hi Mani, sorry, may I know the purpose of adding must_check attribute to
> mhi_read_reg? In which case will mhi controller provide a callback that
> returns error?
> 
> Thanks,
> Qiang
>>>>>> +    edl_db = base + val + (8 * MHI_EDL_DB);
>>>>>> +
>>>>>> +    mhi_cntrl->write_reg(mhi_cntrl, edl_db + 4, 
>>>>>> upper_32_bits(MHI_EDL_COOKIE));
>>>>>> +    mhi_cntrl->write_reg(mhi_cntrl, edl_db, 
>>>>>> lower_32_bits(MHI_EDL_COOKIE));
>>>>>> +
>>>>>> +    mhi_soc_reset(mhi_cntrl);
>>>>>> +
>>>>>> +    mhi_cntrl->runtime_put(mhi_cntrl);
>>>>>> +    mhi_device_put(mhi_cntrl->mhi_dev);
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>>   static int mhi_pci_probe(struct pci_dev *pdev, const struct 
>>>>>> pci_device_id *id)
>>>>>>   {
>>>>>>       const struct mhi_pci_dev_info *info = (struct 
>>>>>> mhi_pci_dev_info *) id->driver_data;
>>>>>> @@ -962,6 +1006,9 @@ static int mhi_pci_probe(struct pci_dev 
>>>>>> *pdev, const struct pci_device_id *id)
>>>>>>       mhi_cntrl->runtime_put = mhi_pci_runtime_put;
>>>>>>       mhi_cntrl->mru = info->mru_default;
>>>>>>   +    if (info->edl_trigger & MHI_PCI_GENERIC_EDL_TRIGGER)
>>>>>> +        mhi_cntrl->edl_trigger = mhi_pci_generic_edl_trigger;
>>>>>> +
>>>>>>       if (info->sideband_wake) {
>>>>>>           mhi_cntrl->wake_get = mhi_pci_wake_get_nop;
>>>>>>           mhi_cntrl->wake_put = mhi_pci_wake_put_nop;
>>>>> Regards,
>>>>> Mayank
>>

Re: [PATCH v3 3/3] bus: mhi: host: pci_generic: Add edl callback to enter EDL

[Manivannan Sadhasivam]

On Wed, Apr 17, 2024 at 12:41:25PM +0800, Qiang Yu wrote:

[...]

> > > > > > +    ret = mhi_get_channel_doorbell(mhi_cntrl, &val);
> > > > > > +    if (ret)
> > > > > > +        return ret;
> > > > > Don't we need error handling part here i.e. calling
> > > > > mhi_cntrl->runtime_put() as well mhi_device_put() ?
> > > > 
> > > > Hi Mayank
> > > > 
> > > > After soc_reset, device will reboot to EDL mode and MHI state
> > > > will be SYSERR. So host will fail to suspend
> > > > anyway. The "error handling" we need here is actually to enter
> > > > EDL mode, this will be done by SYSERR irq.
> > > > Here, mhi_cntrl->runtime_put() and mhi_device_put() are only to
> > > > balance mhi_cntrl->runtime_get() and
> > > > mhi_device_get_sync().
> > > > 
> > > > Thanks,
> > > > Qiang
> > > I am saying is there possibility that mhi_get_channel_doorbell()
> > > returns error ?
> > > If yes, then why don't we need error handling as part of it. you are
> > > exiting if this API return error without doing anything.
> > 
> > I think here mhi_get_channel_doorbell will not return error. But I still
> > add a error checking because it invoked mhi_read_reg, which is a must
> > check
> > API. For modem mhi controller, this API eventually does a memory read.
> > This memory read will return a normal value if link is up and all f's if
> > link
> > is bad.
> > 
> > Thanks,
> > Qiang
> 
> Actually, mhi_get_channel_doorbell should also be used in mhi_init_mmio to
> replace the getting chdb operation by invoking mhi_read_reg as Mani
> commented.
> In mhi_init_mmio, we invoke mhi_read_reg many times, but there is also not
> additionnal error handling.
> 
> I'm not very sure the reason but perhaps if mhi_read_reg returns error (I
> don't
> know which controller will provide a memory read callback returning errors),

Take a look at AIC100 driver: drivers/accel/qaic/mhi_controller.c

> most
> probably something is wrong in PCIe, which is not predictable by MHI and we
> can
> not add err handling every time invoking mhi_read_reg. But we have a timer
> to
> do health_check in pci_generic.c. If link is down, we will do
> pci_function_reset
> to try to reovery.
> 

Right, but the MHI stack is designed to be bus agnostic. So if we happen to use
it with other busses like USB, I2C etc... then read APIs may fail.

Even with PCIe, read transaction may return all 1 response and MHI needs to
treat it as an error condition. But sadly, both pci_generic and ath controllers
are not checking for invalid read value. But those need to be fixed.

Regarding Mayank's query, you should do error cleanups if
mhi_get_channel_doorbell() API fails.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH v3 3/3] bus: mhi: host: pci_generic: Add edl callback to enter EDL

[Qiang Yu]

On 4/22/2024 4:08 PM, Manivannan Sadhasivam wrote:
> On Wed, Apr 17, 2024 at 12:41:25PM +0800, Qiang Yu wrote:
>
> [...]
>
>>>>>>> +    ret = mhi_get_channel_doorbell(mhi_cntrl, &val);
>>>>>>> +    if (ret)
>>>>>>> +        return ret;
>>>>>> Don't we need error handling part here i.e. calling
>>>>>> mhi_cntrl->runtime_put() as well mhi_device_put() ?
>>>>> Hi Mayank
>>>>>
>>>>> After soc_reset, device will reboot to EDL mode and MHI state
>>>>> will be SYSERR. So host will fail to suspend
>>>>> anyway. The "error handling" we need here is actually to enter
>>>>> EDL mode, this will be done by SYSERR irq.
>>>>> Here, mhi_cntrl->runtime_put() and mhi_device_put() are only to
>>>>> balance mhi_cntrl->runtime_get() and
>>>>> mhi_device_get_sync().
>>>>>
>>>>> Thanks,
>>>>> Qiang
>>>> I am saying is there possibility that mhi_get_channel_doorbell()
>>>> returns error ?
>>>> If yes, then why don't we need error handling as part of it. you are
>>>> exiting if this API return error without doing anything.
>>> I think here mhi_get_channel_doorbell will not return error. But I still
>>> add a error checking because it invoked mhi_read_reg, which is a must
>>> check
>>> API. For modem mhi controller, this API eventually does a memory read.
>>> This memory read will return a normal value if link is up and all f's if
>>> link
>>> is bad.
>>>
>>> Thanks,
>>> Qiang
>> Actually, mhi_get_channel_doorbell should also be used in mhi_init_mmio to
>> replace the getting chdb operation by invoking mhi_read_reg as Mani
>> commented.
>> In mhi_init_mmio, we invoke mhi_read_reg many times, but there is also not
>> additionnal error handling.
>>
>> I'm not very sure the reason but perhaps if mhi_read_reg returns error (I
>> don't
>> know which controller will provide a memory read callback returning errors),
> Take a look at AIC100 driver: drivers/accel/qaic/mhi_controller.c
>
>> most
>> probably something is wrong in PCIe, which is not predictable by MHI and we
>> can
>> not add err handling every time invoking mhi_read_reg. But we have a timer
>> to
>> do health_check in pci_generic.c. If link is down, we will do
>> pci_function_reset
>> to try to reovery.
>>
> Right, but the MHI stack is designed to be bus agnostic. So if we happen to use
> it with other busses like USB, I2C etc... then read APIs may fail.
>
> Even with PCIe, read transaction may return all 1 response and MHI needs to
> treat it as an error condition. But sadly, both pci_generic and ath controllers
> are not checking for invalid read value. But those need to be fixed.
>
> Regarding Mayank's query, you should do error cleanups if
> mhi_get_channel_doorbell() API fails.
>
> - Mani
Hi Mani, Mayank

Checked drivers/accel/qaic/mhi_controller.c, the mhi_read_reg
callback of AIC100 does return -EIO if it reads out all'f.

Considering that pci_generic controller also needs to check for
invalid read value which is not fixed though. It's better to invoke
mhi_cntrl->runtime_put() and mhi_device_put() as error cleanups if
mhi_get_channel_doorbell returns error here.

Let me address all your comments and prepare next version patch.

Thanks,
Qiang
>