| Message ID | 1713170945-44640-2-git-send-email-quic_qianyu@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add sysfs entry to EDL mode | expand |
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
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 >
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
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 >
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;
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(+)