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

[Qiang Yu <quic_qianyu@quicinc.com>]

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
>