New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 660922 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 661258



Sign in to add a comment

servo_glados_overlay.xml has some conflicting controls with servo_micro.xml

Project Member Reported by kevcheng@chromium.org, Oct 31 2016

Issue description

raw_usbpd_uart_pty: servo_glados_overlay uses interface 1 while servo_micro uses interface 2
usbpd_ec3po_interp_connect: is not clobber_ok on servo_glados_overlay

Does the servo_glados_overlay.xml control file need these controls set this way?

Assigning to Duncan since he added those controls in: https://chromium-review.googlesource.com/#/c/327700/
 
Cc: tbroch@chromium.org
+Todd for first setting raw_usbpd_uart_pty (uart4_pty) to interface 1 in https://chromium-review.googlesource.com/#/c/246601/
Cc: autumn@chromium.org sbasi@chromium.org
Labels: -Pri-3 Pri-0
Status: Assigned (was: Untriaged)
Making this a P0 since this is now blocking deployments of Asuka/Sentry in the prometheus lab.
Owner: nsanders@chromium.org
Nick knows how to properly fix the situation with raw_usbpd_uart_pty.

usbpd_ec3po_interp_connect has an easy fix in flight already: https://chromium-review.googlesource.com/#/c/406009/
Blockedon: 661258
This is difficult because the glados xml contains a bunch of servo v3 specific configuration. Note that the above usbpd fix removes the warning but won't actually enable the pd uart.
Did a bit study. In servod, there are 2 kinds of configs:
 * one for servo board, like servo_v2_r1.xml, servo_v3_r0.xml, servo_v4.xml;
 * one for cros board, like servo_glados_overlay.xml.

In fact, the problematic raw_usbpd_uart_pty should be defined in servo board config, instead of cros board config. However, servo v2 doesn't support a third-uart, so Glados re-purposes JTAG to be the USB PD UART (Samus and some other boards re-purpose EC SPI (interface 5)). This rework is not a normal case for servo v2. So it doesn't make sense to put it to servo v2 board config. But putting it to cros board causes the conflict with servo v4 which uses different interfaces.

There are some ways to solve this issue, but as Nick said they are difficult. I can state if someone is interested.

IMO, the servo v2 way is a bit hacky and requires rework. The servo v4 + micro is the ideal approach. We may simply phase out the servo v2 way. If developers want the USB PD UART, use servo v4 + micro. Any comment?
I'm all for phasing out the servo v2 way but don't know the process of doing that gracefully (seems like all we can do is to remove it and print a message telling the user to switch to a v4/uServo if they want pdusb).

In the spirit of getting things working again sooner rather than later, would it be all right if I added in a servo_{micro,v4}_interface control into servo_glados_overlay.xml raw_usbpd_uart_pty to get servod working again and unblock deployments until a proper solution is decided upon and implemented?
Didn't know we can specify a servo_{micro,v4}_interface control. Looks good as a short term fix.
From my perspective all *_usbpd_* controls were only intended to be used for USBPD developers or for that matter any additional MCU uarts(sensor-hub?) that fit outside the servo V2 header spec.  

No automation IMO should EVER use these controls so providing your changes to servod don't break the currently supported USBPD developer access to servo V2 we can proceed and later address continued support.

Does anyone have a known-working glados based system I can try this on?
Project Member

Comment 12 by bugdroid1@chromium.org, Nov 2 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/b7962863752dd51678fef2823ddb8db96cbb7b31

commit b7962863752dd51678fef2823ddb8db96cbb7b31
Author: Kevin Cheng <kevcheng@chromium.org>
Date: Tue Nov 01 20:35:15 2016

servo_glados_overlay: Add in servo_{v4,micro}_interface to raw_usbpd_uart_pty.

servo_glados_overlay defines raw_usbpd_uart_pty on an interface
specifically for servo v2/v3 (that is reworked) that is not compatible
with servo v4/uServo.  This is a sticky situation since this control
really should belong in the servo v2/v3 overlay but at the same time
doesnt since those controls don't really exist without a rework on the
v2/v3.

A proper solution is still being hashed out but this is blocking servod
from starting which is blocking all boards that use this overlay from
deploying.  Right now just define a servo v4/uServo specific interface
for this control so that servod can get going.  It's expected that usbpd
won't work this way but we're not using it as part of deployments and the
majority of testing doesn't as well so we're ok with that.

BUG= chromium:660922 
TEST=Started servod with change with servo v4/uServo and asuka and did
not see the conflict.

Change-Id: If444c18460103df0b4d921163f8445424d398f1c
Reviewed-on: https://chromium-review.googlesource.com/405757
Commit-Ready: Kevin Cheng <kevcheng@chromium.org>
Tested-by: Kevin Cheng <kevcheng@chromium.org>
Reviewed-by: Wai-Hong Tam <waihong@google.com>

[modify] https://crrev.com/b7962863752dd51678fef2823ddb8db96cbb7b31/servo/data/servo_glados_overlay.xml

Project Member

Comment 13 by bugdroid1@chromium.org, Nov 2 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/db7bc11ead408c6a7c132f0bf7dbc87d7a1f6f88

commit db7bc11ead408c6a7c132f0bf7dbc87d7a1f6f88
Author: Kevin Cheng <kevcheng@chromium.org>
Date: Tue Nov 01 18:05:13 2016

servo_glados_overlay: Make usbpd_ec3po_interp_connect clobber'able.

servo_micro.xml also defines this control and in boards that inherit the
servo_glados_overlay (sentry, asuka) this causes issues when starting
servod.

Both control definitions share the same params so it's safe to clobber.

BUG= chromium:660922 
TEST=Started up servod and did not complain about control anymore.

Change-Id: I0536537eaf57e04efda94480424a4773a538cc65
Reviewed-on: https://chromium-review.googlesource.com/406009
Commit-Ready: Kevin Cheng <kevcheng@chromium.org>
Tested-by: Kevin Cheng <kevcheng@chromium.org>
Reviewed-by: Todd Broch <tbroch@chromium.org>

[modify] https://crrev.com/db7bc11ead408c6a7c132f0bf7dbc87d7a1f6f88/servo/data/servo_glados_overlay.xml

The routing for uart3 for samus and glados are as follows, we'll need to make an init function for each:

samus: 
gpioset UART3_TX_SERVO_JTAG_TCK ALT
gpioset UART3_RX_JTAG_BUFFER_TO_SERVO_TDO ALT
gpioset SPI1_MUX_SEL 0 
gpioset SPI1_BUF_EN_L 0 
gpioset SPI1_VREF_18 1  


glados:
gpioset UART3_RX_JTAG_BUFFER_TO_SERVO_TDO ALT
gpioset UART3_TX_SERVO_JTAG_TCK ALT
gpioset JTAG_BUFIN_EN_L 0
gpioset SERVO_JTAG_TDO_BUFFER_EN 1
gpioset SERVO_JTAG_TDO_SEL 1
FYI this bug is blocked on code review of: https://chromium-review.googlesource.com/#/c/407539/
Project Member

Comment 16 by bugdroid1@chromium.org, Nov 8 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/292b645467b46ffdec4fca0dfab954f632731cd6

commit 292b645467b46ffdec4fca0dfab954f632731cd6
Author: Kevin Cheng <kevcheng@chromium.org>
Date: Thu Nov 03 06:30:17 2016

servo_lucidcosmos_overlay: Add in servo_{v4,micro}_interface for raw_ec_uart_pty.

BUG= chromium:660922 
TEST=start servod with v4/micro and servo_lucidcosmos_overlay and it
starts up successfully.

Change-Id: I1629c2c210b7263c73137812c407e42f546a1a3d
Reviewed-on: https://chromium-review.googlesource.com/407052
Commit-Ready: Kevin Cheng <kevcheng@chromium.org>
Tested-by: Kevin Cheng <kevcheng@chromium.org>
Reviewed-by: Wai-Hong Tam <waihong@google.com>

[modify] https://crrev.com/292b645467b46ffdec4fca0dfab954f632731cd6/servo/data/servo_lucidcosmos_overlay.xml

Project Member

Comment 17 by bugdroid1@chromium.org, Nov 9 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/hdctools/+/a194d9a5a2292492d782f6b6c1e184addfab3f91

commit a194d9a5a2292492d782f6b6c1e184addfab3f91
Author: Nick Sanders <nsanders@chromium.org>
Date: Fri Nov 04 02:50:38 2016

servo_micro: add UART3 routing in servod

This provides a driver and control for switching the usbpd uart
to spi lines (samus) or jtag lines (glados), allowing pd controller
interaction through servod.

BUG= chromium:660922 
TEST=can access PD uart on samus, chell after usbpd_uart_routing:glados/samus

Change-Id: Ie41d5b045a15bf640ee107f4f014b219c08b20a6
Reviewed-on: https://chromium-review.googlesource.com/407539
Commit-Ready: Nick Sanders <nsanders@chromium.org>
Tested-by: Nick Sanders <nsanders@chromium.org>
Reviewed-by: Kevin Cheng <kevcheng@chromium.org>

[modify] https://crrev.com/a194d9a5a2292492d782f6b6c1e184addfab3f91/servo/drv/__init__.py
[add] https://crrev.com/a194d9a5a2292492d782f6b6c1e184addfab3f91/servo/drv/ec3po_servo_micro.py
[modify] https://crrev.com/a194d9a5a2292492d782f6b6c1e184addfab3f91/servo/data/servo_micro.xml

Owner: kevcheng@chromium.org
usbpd_uart_routing:glados/samus servo contol is added

I believe Kevin is refactoring the xml, and can init this control to the correct value with the servo_micro + glados config xml.
Project Member

Comment 19 by sheriffbot@chromium.org, Nov 30 2016

Pri-0 bugs are critical regressions or serious emergencies, and this bug has not been updated in three days. Could you please provide an update, or adjust the priority to a more appropriate level if applicable?

If a fix is in active development, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 20 by sheriffbot@chromium.org, Dec 15 2016

Pri-0 bugs are critical regressions or serious emergencies, and this bug has not been updated in three days. Could you please provide an update, or adjust the priority to a more appropriate level if applicable?

If a fix is in active development, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Fixed?
Labels: -Pri-0 Pri-1
The conflict was resolved. Per c#18, still need to modify the overlay xml files to set the proper usbpd_uart_routing value to either glados or samus.
Labels: akeshet-pending-downgrade
ChromeOS Infra P1 Bugscrub.

P1 Bugs in this component should be important enough to get weekly status updates.

Is this already fixed?  -> Fixed
Is this no longer relevant? -> Archived or WontFix
Is this not a P1, based on go/chromeos-infra-bug-slo rubric? -> lower priority.
Is this a Feature Request rather than a bug? Type -> Feature
Is this missing important information or scope needed to decide how to proceed? -> Ask question on bug, possibly reassign.
Does this bug have the wrong owner? -> reassign.

Bugs that remain in this state next week will be downgraded to P2.

Comment 24 by sosa@chromium.org, Jul 18 2017

Owner: sbasi@chromium.org
Labels: -Pri-1 -akeshet-pending-downgrade Pri-2
Hi, this bug has not been updated recently. Please acknowledge the bug and provide status within two weeks (6/22/2018), or the bug will be closed. Thank you.
Status: Fixed (was: Assigned)
Should be all set here.

Sign in to add a comment