Received-SPF: pass (google.com: domain of linux-kernel+bounces-146284-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <a62cb28a-e6ab-4f60-9210-b7aa8e0c34e0@quicinc.com>
Date: Tue, 16 Apr 2024 13:45:18 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 1/3] bus: mhi: host: Add sysfs entry to force device to
 enter EDL
To: Manivannan Sadhasivam <mani@kernel.org>
CC: <quic_jhugo@quicinc.com>, <mhi@lists.linux.dev>,
        <linux-arm-msm@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <quic_cang@quicinc.com>, <quic_mrana@quicinc.com>
References: <1713170945-44640-1-git-send-email-quic_qianyu@quicinc.com>
 <1713170945-44640-2-git-send-email-quic_qianyu@quicinc.com>
 <20240415115616.GF7537@thinkpad>
Content-Language: en-US
From: Qiang Yu <quic_qianyu@quicinc.com>
In-Reply-To: <20240415115616.GF7537@thinkpad>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit


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
>