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

[Manivannan Sadhasivam <mani@kernel.org>]

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

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