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

Issue 746471 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

ec: stm32f4: writing usb serial number broken

Project Member Reported by gwendal@chromium.org, Jul 19 2017

Issue description

Since STM32F4 has non uniform page size, PSTATE_BANK was disabled.
It prevents USB serial number to be written to the flash.

To fix the issue we can:
- use the first bank (16kB) to store pstate, but it has not been done before.
- use the OTP area of the STM32F4, assuming the serial number is written only once.
 
Owner: gwendal@chromium.org
Status: Started (was: Untriaged)
Talking to Nick, I am going the OTP route.
staff and hammer have the same problem.

No, we don't use serial numbers in PSTATE for staff and hammer.
Indeed, the serial is based on the "Device electronic signature" at register 0x1FFF 7A10.

Given the STM32F4 also have this register, maybe we can change the process to generate the USB serial number as hammer and staff do.
Actually I chatted with Nick about this, and he prefers a more user-friendly serial number (STM32F serial numbers are longer).
Verified on sweetberry that uniqueid exists, adding board_read_serial() from hammer to sweetberry.
 

> rw 0x1FFF7A10 
read 0x1fff7a10 = 0x00370041
> rw 0x1FFF7A14
read 0x1fff7a14 = 0x33345114
> rw 0x1FFF7A18
read 0x1fff7a18 = 0x30373031
> version
Chip:    stm stm32f446 
Board:   0
RO:      sweetberry_v1.1.6504-3a5e9a10e
...
Would strongly prefer the OTP route as these serial numbers need to be typed by people and set to match automation. The typical serial numbers are "marlin-7-A" and "marlin-7-B" or some such. 

Project Member

Comment 8 by bugdroid1@chromium.org, Jul 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/1b25735b732e7766aceb3f060e4ca205aba6d358

commit 1b25735b732e7766aceb3f060e4ca205aba6d358
Author: Gwendal Grignou <gwendal@chromium.org>
Date: Sat Jul 29 00:45:13 2017

Add OTP support

One Time Programmable memory can be used to store permanent data like
serial numbers.
Reorganize the code to support writing serial number to OTP, in
addition to pstate (if using its own memory bank) or autogenerate from
unique id (hammer).

+ Add CONFIG_OTP to enable OTP code
+ Add CONFIG_SERIALNO_LEN to indicate the size of the serial number
string.  Currently set to 28, when USB serial number is needed.
+ Expose flash_read|write_pstate_serial and add otp_read|write_serail,
remove more generic flash_read|write_serial.
+ Make board_read|write_serial generic, declared outside of USB subsystem.

Priority order to read|write serial string:
- board definition (like hammer)
- pstate location, if stored in its private memory bank
- otp area
If none of these methods are available, a compilation error is raised.

BUG= chromium:746471 
BRANCH=none
TEST=compile

Change-Id: I3d16125a6c0f424fb30e38123e63cf074b3cb2d3
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/580289
Reviewed-by: Nick Sanders <nsanders@chromium.org>

[modify] https://crrev.com/1b25735b732e7766aceb3f060e4ca205aba6d358/chip/stm32/usb.c
[modify] https://crrev.com/1b25735b732e7766aceb3f060e4ca205aba6d358/chip/g/usb.c
[modify] https://crrev.com/1b25735b732e7766aceb3f060e4ca205aba6d358/common/system.c
[add] https://crrev.com/1b25735b732e7766aceb3f060e4ca205aba6d358/include/otp.h
[modify] https://crrev.com/1b25735b732e7766aceb3f060e4ca205aba6d358/board/hammer/board.c
[modify] https://crrev.com/1b25735b732e7766aceb3f060e4ca205aba6d358/include/config.h
[modify] https://crrev.com/1b25735b732e7766aceb3f060e4ca205aba6d358/include/usb_descriptor.h
[modify] https://crrev.com/1b25735b732e7766aceb3f060e4ca205aba6d358/include/system.h
[modify] https://crrev.com/1b25735b732e7766aceb3f060e4ca205aba6d358/chip/stm32/usb_dwc.c
[modify] https://crrev.com/1b25735b732e7766aceb3f060e4ca205aba6d358/include/flash.h
[modify] https://crrev.com/1b25735b732e7766aceb3f060e4ca205aba6d358/common/flash.c

Project Member

Comment 9 by bugdroid1@chromium.org, Jul 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/a35218e20499a95ec7706f87cb48ff39c5451663

commit a35218e20499a95ec7706f87cb48ff39c5451663
Author: Gwendal Grignou <gwendal@chromium.org>
Date: Sat Jul 29 00:45:13 2017

stm32f4: Add OTP support.

Add support for OTP memory: if needed store serial number in first bank.

BUG= chromium:746471 
BRANCH=none
TEST=On sweetberry, check we can write serial number with serialno
command. Check serial number survive a firmware update.

First, check without write protect, check we can write 0s (but not 1s)
serialno
Serial number: NNNNNNNNNNNNNNNNNNNNNN
>
> serial set MMMMMMMMMMMMMMMMMMMMMMMMMMMMM
Saving serial number
Serial number: LLLLLLLLLLLLLLLLLLLLLL

After lock enabled, check we can not overwrite.
> serial set AMMMMMMMMMMMMMMMMMMMMMMMMMMMM
Saving serial number
Serial number: LLLLLLLLLLLLLLLLLLLLLL
Access Denied

Check that serialno returns "Uninitialized" if it was never set.

Change-Id: I9ab08486a7c3e1958e964649640d69b5b70947e3
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/580290
Reviewed-by: Nick Sanders <nsanders@chromium.org>

[modify] https://crrev.com/a35218e20499a95ec7706f87cb48ff39c5451663/chip/stm32/registers.h
[modify] https://crrev.com/a35218e20499a95ec7706f87cb48ff39c5451663/chip/stm32/build.mk
[add] https://crrev.com/a35218e20499a95ec7706f87cb48ff39c5451663/chip/stm32/otp-stm32f4.c
[modify] https://crrev.com/a35218e20499a95ec7706f87cb48ff39c5451663/chip/stm32/config-stm32f446.h

Project Member

Comment 10 by bugdroid1@chromium.org, Jul 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/734ebcbbb4e1e6f816225c59acef08ebd1094a2c

commit 734ebcbbb4e1e6f816225c59acef08ebd1094a2c
Author: Gwendal Grignou <gwendal@chromium.org>
Date: Sat Jul 29 00:45:12 2017

extra: usb_updater: include config.h

board.h and config-chip.h should only be called from config.h, otherwise
some #define may not be set properly.

BUG= chromium:746471 
BRANCH=none
TEST=Found a bug while compiling OTP changes (c/580289/)
(https://luci-milo.appspot.com/buildbot/chromiumos.tryserver/
         no_vmtest_pre_cq/81548)
The size of the serial number string is set in config.h when
CONFIG_USB_SERIALNO is needed.
Compile with ec-utils with cr50_onboard USE flag set.

Change-Id: I5a2306bd0dc1dea29265226f2986829b768cfb61
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/581887
Reviewed-by: Randall Spangler <rspangler@chromium.org>
Reviewed-by: Nicolas Boichat <drinkcat@chromium.org>

[modify] https://crrev.com/734ebcbbb4e1e6f816225c59acef08ebd1094a2c/extra/usb_updater/usb_updater.c
[modify] https://crrev.com/734ebcbbb4e1e6f816225c59acef08ebd1094a2c/extra/usb_updater/Makefile

Project Member

Comment 11 by bugdroid1@chromium.org, Aug 4 2017

Labels: merge-merged-firmware-cr50-9308.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/b47d67563678967437330d4d74b65ef313be7047

commit b47d67563678967437330d4d74b65ef313be7047
Author: Gwendal Grignou <gwendal@chromium.org>
Date: Fri Aug 04 17:01:31 2017

extra: usb_updater: include config.h

board.h and config-chip.h should only be called from config.h, otherwise
some #define may not be set properly.

 Conflicts:
     (checked out the master version, but dropped usb_updater2 target
	from it): extra/usb_updater/Makefile

BUG= chromium:746471 
BRANCH=none
TEST=Found a bug while compiling OTP changes (c/580289/)
(https://luci-milo.appspot.com/buildbot/chromiumos.tryserver/
         no_vmtest_pre_cq/81548)
The size of the serial number string is set in config.h when
CONFIG_USB_SERIALNO is needed.
Compile with ec-utils with cr50_onboard USE flag set.

Change-Id: I5a2306bd0dc1dea29265226f2986829b768cfb61
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/581887
Reviewed-by: Randall Spangler <rspangler@chromium.org>
Reviewed-by: Nicolas Boichat <drinkcat@chromium.org>
(cherry picked from commit 734ebcbbb4e1e6f816225c59acef08ebd1094a2c)
Signed-off-by: Vadim Bendebury <vbendeb@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/598526

[modify] https://crrev.com/b47d67563678967437330d4d74b65ef313be7047/extra/usb_updater/usb_updater.c
[modify] https://crrev.com/b47d67563678967437330d4d74b65ef313be7047/extra/usb_updater/Makefile

Project Member

Comment 12 by bugdroid1@chromium.org, Aug 19 2017

Labels: merge-merged-firmware-cr50-9308.24.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/7f44d34666ccdb3923b8d10e8167a0613024f3ac

commit 7f44d34666ccdb3923b8d10e8167a0613024f3ac
Author: Gwendal Grignou <gwendal@chromium.org>
Date: Sat Aug 19 01:28:38 2017

extra: usb_updater: include config.h

board.h and config-chip.h should only be called from config.h, otherwise
some #define may not be set properly.

 Conflicts:
	extra/usb_updater/Makefile

BUG= chromium:746471 
BRANCH=none
TEST=Found a bug while compiling OTP changes (c/580289/)
(https://luci-milo.appspot.com/buildbot/chromiumos.tryserver/
         no_vmtest_pre_cq/81548)
The size of the serial number string is set in config.h when
CONFIG_USB_SERIALNO is needed.
Compile with ec-utils with cr50_onboard USE flag set.

Change-Id: I5a2306bd0dc1dea29265226f2986829b768cfb61
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/581887
Reviewed-by: Randall Spangler <rspangler@chromium.org>
Reviewed-by: Nicolas Boichat <drinkcat@chromium.org>
(cherry picked from commit 734ebcbbb4e1e6f816225c59acef08ebd1094a2c)
Reviewed-on: https://chromium-review.googlesource.com/622410
Tested-by: Vadim Bendebury <vbendeb@chromium.org>
Commit-Queue: Vadim Bendebury <vbendeb@chromium.org>
Reviewed-by: Vadim Bendebury <vbendeb@chromium.org>

[modify] https://crrev.com/7f44d34666ccdb3923b8d10e8167a0613024f3ac/extra/usb_updater/usb_updater.c
[modify] https://crrev.com/7f44d34666ccdb3923b8d10e8167a0613024f3ac/extra/usb_updater/Makefile

Status: Fixed (was: Started)
gwendal fixed this with OTP serial numbers, and I've verified that it works great.

Comment 14 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)
Project Member

Comment 15 by bugdroid1@chromium.org, Feb 6 2018

Labels: merge-merged-factory-eve-9667.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/06eae4e50863fad7870b27c97501b81d7f2cbc51

commit 06eae4e50863fad7870b27c97501b81d7f2cbc51
Author: Gwendal Grignou <gwendal@chromium.org>
Date: Tue Feb 06 15:59:26 2018

extra: usb_updater: include config.h

board.h and config-chip.h should only be called from config.h, otherwise
some #define may not be set properly.

BUG= chromium:746471 
BRANCH=none
TEST=Found a bug while compiling OTP changes (c/580289/)
(https://luci-milo.appspot.com/buildbot/chromiumos.tryserver/
         no_vmtest_pre_cq/81548)
The size of the serial number string is set in config.h when
CONFIG_USB_SERIALNO is needed.
Compile with ec-utils with cr50_onboard USE flag set.

Change-Id: I5a2306bd0dc1dea29265226f2986829b768cfb61
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/581887
Reviewed-by: Randall Spangler <rspangler@chromium.org>
Reviewed-by: Nicolas Boichat <drinkcat@chromium.org>
(cherry picked from commit 734ebcbbb4e1e6f816225c59acef08ebd1094a2c)
Reviewed-on: https://chromium-review.googlesource.com/894138
Reviewed-by: Gwendal Grignou <gwendal@google.com>
Commit-Queue: Sam Hurst <shurst@google.com>
Tested-by: Sam Hurst <shurst@google.com>
Trybot-Ready: Sam Hurst <shurst@google.com>

[modify] https://crrev.com/06eae4e50863fad7870b27c97501b81d7f2cbc51/extra/usb_updater/usb_updater.c
[modify] https://crrev.com/06eae4e50863fad7870b27c97501b81d7f2cbc51/extra/usb_updater/Makefile

Sign in to add a comment