Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[usbdev] aon_wake maintains pull up assertion over VBUS disconnection #18562

Open
alees24 opened this issue May 12, 2023 · 11 comments
Open

[usbdev] aon_wake maintains pull up assertion over VBUS disconnection #18562

alees24 opened this issue May 12, 2023 · 11 comments
Assignees
Labels
IP:usbdev Type:Enhancement Feature requests, enhancements Type:FutureRelease Not relevant to currently planned releases/milestones
Milestone

Comments

@alees24
Copy link
Contributor

alees24 commented May 12, 2023

Description

Re Disconnection/Interruption to VBUS/SENSE when OT is in Deep Sleep
(Imperfection rather than fault)

In the event that aon_wake has control of the pull ups and VBUS/SENSE is removed for a period, the pull up remains asserted and will be driven at the point of VBUS reappearing. If the host controller has spotted the disconnection because D+ was also affected (no longer pulled up) then it will discover the device presence and attempt to configure the device.

If this same interruption happens whilst usbdev has control, the pull up is removed when VBUS is lost (in usbdev_usbif qualifies the assertion of usb_pullup_en_o with the presence of usb_sense_i) and software can be expected to respond promptly and deassert the pull up, await the return of VBUS and then go through the normal startup/configuration sequence again.

In chip sim, with current test code, an observed delay of 2ms occurs between VBUS disconnection and software being ready to access/reconfigure usbdev. A shorter interruption could lead to unintended/unexpected behavior, although it's likely that the host controller will just keep retrying.

Suggest when software returns from Deep Sleep and discovers a Disconnection event, it should be aware that the host may or may not have spotted a disconnection, and thus introduce a deliberate disconnect period by ensuring that usbdev pull up is disabled before deactivating the aon_wake module. Only after a deliberate delay should the pull up then be enabled to indicate presence; TBC - this delay need only be of the order of microseconds to milliseconds, not even tens of milliseconds, and only in the case of a Disconnection having been reported.

Re future RTL, I would suggest these pull up assignments:

assign aon_dppullup_en_d = wake_detect_active_q ? aon_dppullup_en_q
: usbdev_dppullup_en_aon;
assign aon_dnpullup_en_d = wake_detect_active_q ? aon_dnpullup_en_q
: usbdev_dnpullup_en_aon;

should be:

assign aon_dppullup_en_d = wake_detect_active_q ? (aon_dppullup_en_q & ~event_sense_lost)
: usbdev_dppullup_en_aon;
assign aon_dnpullup_en_d = wake_detect_active_q ? (aon_dnpullup_en_q & ~event_sense_lost)
: usbdev_dnpullup_en_aon;

Pull up(s) that are asserted whilst the module is active will then become deasserted when the (filtered) sense signal disappears and the module decides to request wakeup.

@moidx
Copy link
Contributor

moidx commented May 12, 2023

@GregAC to assign priority and identify potential workarounds with @alees24.

@alees24 alees24 self-assigned this May 12, 2023
@GregAC
Copy link
Contributor

GregAC commented May 15, 2023

Following discussion with @alees24 there is no urgency with this change. It is nice to have but isn't a concern for M2.5.1

@moidx moidx added this to the Discrete: M2.5.Backlog milestone May 15, 2023
@moidx
Copy link
Contributor

moidx commented May 15, 2023

Moving this to M2.5.Backlog so that we can pick it up in a future release.

@alees24
Copy link
Contributor Author

alees24 commented May 16, 2023

After a bit more thought I think even the software workaround can be largely zero-impact. Upon waking from sleep and discovering a report of a Disconnection, check for the presence of VBUS/SENSE and immediately disconnect the pull up.

If VBUS/SENSE was discovered to be absent - the usual case, since the most probable cause is becoming physically unplugged - there is no further action required; we will just be notified in the event of any subsequent VBUS assertion.

If VBUS was asserted, then we could have detected a harmless interruption of VBUS from an electrical problem/similar, but we cannot trust the connection, and must de-assert the pullup for a long enough interval for the host/hubs to recognize a reconnection.

I think it's still worth including the proposed hardware modification in a subsequent revision of the RTL (once tested, but as far as I can see,it's robust).

@johngt
Copy link

johngt commented Mar 1, 2024

Covered by draft PR - #19270
Leaving open until that is merged.
Setting effort spent such that remaining is 0.

@andreaskurth
Copy link
Contributor

andreaskurth commented Mar 21, 2024

Current status captured in the D2 signoff issue:

@alees24: Now that #22019 has been merged and thus AFAIU the SW workaround is possible, do you plan to also complete #19270 for M2 or should that come in later? Put differently, should this issue stay in M2 or be postponed to M4?

@alees24
Copy link
Contributor Author

alees24 commented Mar 21, 2024

My feeling is that none of us has really worked out the full set of possible event sequences to be sure that the hardware behavior change would overall be an improvement.
Hopefully we'll get a chance to think fully about this and test it properly before M4, and if the hardware is changed it can be regarded as a bug fix; it would not necessitate changes to the register API etc, just annotation in the programmer's guide documentation.

@mundaym mundaym added the Triage: deprioritize? temporary label for triage; issue could be deprioritized label Apr 29, 2024
@andreaskurth
Copy link
Contributor

@andreaskurth to check this in light of CDC investigations

@andreaskurth
Copy link
Contributor

@johngt to raise this in discussions next Monday

@andreaskurth
Copy link
Contributor

discussed at the SiVal meeting, still needed for M4; Adrian is on it

@andreaskurth andreaskurth removed the Triage: deprioritize? temporary label for triage; issue could be deprioritized label May 24, 2024
@moidx moidx added the Type:FutureRelease Not relevant to currently planned releases/milestones label Jun 11, 2024
@moidx moidx modified the milestones: Earlgrey-PROD.M4, Backlog Jun 11, 2024
@johngt johngt removed the Earlgrey-PROD Candidate Temporary label to triage issues into Earlgrey-PROD Milestones label Jun 11, 2024
@moidx
Copy link
Contributor

moidx commented Jun 11, 2024

Marking this as a future release as we won't be able to land an RTL change for Earlgrey-PROD.M4.

@a-will: Response time after wake up is in the order of 100s of milliseconds due to the specification of V-BUS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IP:usbdev Type:Enhancement Feature requests, enhancements Type:FutureRelease Not relevant to currently planned releases/milestones
Projects
None yet
Development

No branches or pull requests

7 participants