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@xxxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxxx
> > +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