Messages in this thread | | | Date | Tue, 9 Apr 2024 11:32:04 +0800 | Subject | Re: [PATCH] bus: mhi: host: Add sysfs entry to force device to enter EDL | From | Qiang Yu <> |
| |
On 4/8/2024 6:15 PM, Manivannan Sadhasivam wrote: > On Mon, Apr 08, 2024 at 04:10:40PM +0800, Qiang Yu wrote: >> On 4/3/2024 1:44 PM, Qiang Yu wrote: >>> On 4/2/2024 11:33 PM, Jeffrey Hugo wrote: >>>> On 4/2/2024 7:52 AM, Qiang Yu wrote: >>>>> On 4/2/2024 12:34 PM, Qiang Yu wrote: >>>>>> On 1/12/2024 3:08 AM, Jeffrey Hugo wrote: >>>>>>> On 1/9/2024 2:20 AM, Qiang Yu wrote: >>>>>>>> On 1/3/2024 12:52 AM, Manivannan Sadhasivam wrote: >>>>>>>>> On Tue, Jan 02, 2024 at 08:31:15AM -0700, Jeffrey Hugo wrote: >>>>>>>>>> On 12/25/2023 12:47 AM, Qiang Yu wrote: >>>>>>>>>>> From: Bhaumik Bhatt <quic_bbhatt@quicinc.com> >>>>>>>>>>> >>>>>>>>>>> Forcing the device (eg. SDX75) to enter >>>>>>>>>>> Emergency Download Mode involves >>>>>>>>>>> writing the 0xEDEDEDED cookie to the >>>>>>>>>>> channel 91 doorbell register and >>>>>>>>>>> forcing an SOC reset afterwards. Allow >>>>>>>>>>> users of the MHI bus to exercise the >>>>>>>>>>> sequence using a sysfs entry. >>>>>>>>>> I don't see this documented in the spec >>>>>>>>>> anywhere. Is this standard behavior >>>>>>>>>> for all MHI devices? >>>>>>>>>> >>>>>>>>>> What about devices that don't support EDL mode? >>>>>>>>>> >>>>>>>>>> How should the host avoid using this special >>>>>>>>>> cookie when EDL mode is not >>>>>>>>>> desired? >>>>>>>>>> >>>>>>>>> All points raised by Jeff are valid. I had >>>>>>>>> discussions with Hemant and Bhaumik >>>>>>>>> previously on allowing the devices to enter EDL >>>>>>>>> mode in a generic manner and we >>>>>>>>> didn't conclude on one final approach. >>>>>>>>> >>>>>>>>> Whatever way we come up with, it should be >>>>>>>>> properly described in the MHI spec >>>>>>>>> and _should_ be backwards compatible. >>>>>>>> Hi Mani, Jeff. The method of entering EDL mode is >>>>>>>> documented in MHI spec v1.2, Chapter 13.2. >>>>>>>> >>>>>>>> Could you please check once? >>>>>>> I do see it listed there. However that was a FR for >>>>>>> SDX55, so devices prior to that would not support this. >>>>>>> AIC100 predates this change and would not support the >>>>>>> functionality. I verified the AIC100 implementation is >>>>>>> not aware of this cookie. >>>>>>> >>>>>>> Also, that functionality depends on channel 91 being >>>>>>> reserved per the table 9-2, however that table only >>>>>>> applies to modem class devices as it is under chapter 9 >>>>>>> "Modem protocols over PCIe". Looking at the ath11k and >>>>>>> ath12k implementations in upstream, it looks like they >>>>>>> partially comply. Other devices have different MHI >>>>>>> channel definitions. >>>>>>> >>>>>>> Chapter 9 doesn't appear to be in older versions of the >>>>>>> spec that I have, so it is unclear if this functionality >>>>>>> is backwards compatible (was channel 91 used for another >>>>>>> purpose in pre-SDX55 modems). >>>>>>> >>>>>>> I'm not convinced this belongs in the MHI core. At a >>>>>>> minimum, the MHI controller(s) for the applicable >>>>>>> devices needs to opt-in to this. >>>>>>> >>>>>>> -Jeff >>>>>> Hi Jeff >>>>>> >>>>>> Sorry for reply so late. In older versions of the spec, >>>>>> there is no description about EDL doorbell. However, in MHI >>>>>> spec v1.2, section 13.2, >>>>>> It explicitly says "To set the EDL cookie, the host writes >>>>>> 0xEDEDEDED to channel doorbell 91." So I think every device >>>>>> based on MHI spec v1.2 >>>>>> should reserve channel doorbell 91 for EDL mode. >>>>>> >>>>>> So can we add another flag called mhi_ver in mhi controller >>>>>> to indicate its mhi version and then we can add mhi_ver >>>>>> checking to determine if this >>>>>> device supports EDL sysfs operation? >>>>>> >>>>>> Thanks, >>>>>> Qiang >>>>> I discussed with internal team, look like devices that reserve >>>>> channel doorbell 91 for EDL, thier MHIVER register value can >>>>> still be 1.0 instead >>>>> of 1.2. So even if we add a flag called mhi_ver to store the >>>>> value read from the MHIVER register. We still can not do EDL >>>>> support check depend on it. >>>>> >>>>> But I still think enter EDL mode by writing EDL cookie to >>>>> channel doorbell is a standard way. At least it's a standard way >>>>> from MHI spec V1.2. >>>>> >>>>> In mhi_controller, we have a variable edl_image representing the >>>>> name and path of firmware. But We still can not determine if the >>>>> device reserve >>>>> channel doorbell 91 by checking this because some devices may >>>>> enter EDL mode in different way. Mayebe we have to add a flag in >>>>> mhi_controller >>>>> called edl_support to do the check. >>>> So, not all devices support EDL mode (even v1.2 devices, which I >>>> know of one in development). Of the devices that support EDL mode, >>>> not all of them use the same mechanism to enter EDL mode. >>>> >>>> It appears all of this needs to be shoved to the controller. >>>> >>>> At best, I think the controller can provide an optional EDL >>>> callback. If the callback is provided, then MHI creates a sysfs >>>> entry (similar to soc_reset) for the purpose of entering EDL mode. >>>> If the sysfs entry is called, all MHI does is call the controller's >>>> callback. >>>> >>>> -Jeff >>> >>> Hi Jeff >>> >>> This idea looks good. We can add edl call back in mhi_pci_dev_info and >>> assgin it to mhi controller during probe. >>> Meanwhile, we can get edl doorbell address in this callback instead of >>> mhi_init_mmio. >>> >>> Mani, what do you think about it? Can I implement the EDL sysfs entry >>> like this? >>> >> Hi Mani, Jeff >> >> I plan to implement EDL sysfs entry as Jeff suggested. >> >> 1. Add an optional EDL callback in mhi_pci_dev_info and assign it to mhi >> controller during probe. All logic >> to enter EDL mode will be moved in this EDL callback. >> >> 2. Create EDL sysfs entry anyway, and check if EDL callback exists, run EDL >> callback, otherwise print log >> and return. >> > You should not print anything on unsupported platforms while introducing a new > feature. > > MHI stack should first check for the existence of the EDL callback and then only > it should try to create the sysfs entry. But the EDL callback varies from device > to device afaik, so I would've fancied to pass the callback from the > mhi_controller_config structure. But the config is meant to provide config > options as opposed to callbacks. > > So I think a neat way would be to add a new flag, > mhi_controller_config::edl_trigger. Then enable that flag in the config of > supported devices and during mhi_pci_probe(), pass the > mhi_pci_generic_edl_trigger() function as the callback to > mhi_controller::edl_trigger. > > In the future, if we happen to add more EDL triggering mechanisms (vendor > specific), then we can use bitfields to differentiate them. > > - Mani > > மணிவண்ணன் சதாசிவம்
Hi Mani, thanks for your comments, let me prepare next version patch as you and Jeff suggested.
| |