Skip to content

Conversation

JamesH65
Copy link
Contributor

@JamesH65 JamesH65 commented Jan 3, 2020

Added the missing User OTP bits that can appear in the board revision.

@lurch
Copy link
Contributor

lurch commented Jan 3, 2020

Fixes #1298 ? ping @andreiw

@JamesH65
Copy link
Contributor Author

JamesH65 commented Jan 3, 2020

@lurch Indeed - just checking it's an appropriate fix with the bosses.

@lurch
Copy link
Contributor

lurch commented Jan 3, 2020

I wonder if this change should instead go in https://github.com/raspberrypi/documentation/blob/master/hardware/raspberrypi/otpbits.md since #1298 suggests that these bits don't appear in /proc/cpuinfo, and https://github.com/raspberrypi/documentation/blob/master/hardware/raspberrypi/revision-codes/README.md explicitly mentions cat /proc/cpuinfo ? 🤷‍♂️
Or maybe there's a better solution...

@JamesH65
Copy link
Contributor Author

JamesH65 commented Jan 3, 2020

@lurch. After checking the code, I think all three of the bits documented will appear in cpuinfo, I checked that the no overvoltage does appear. @ghollingworth has suggested a link to this page. https://www.raspberrypi.org/documentation/hardware/industrial/README.md

@lurch
Copy link
Contributor

lurch commented Jan 3, 2020

@ghollingworth has suggested a link to this page. https://www.raspberrypi.org/documentation/hardware/industrial/README.md

The "Locking the OTP changes" part of that page says "It is possible to lock the OTP changes to avoid them being edited again. ... Once locked, none of the customer OTP values can be accessed."

a) Should the last sentence say "... none of the customer OTP values can be edited." ? (as "accessed" might be misinterpreted to mean "read")
b) Does this imply that the documentation in this PR should say e.g. "customer OTP programming enabled" rather than "OTP programming enabled"
c) Are there separate bits that the user can set in the OTP to prevent both editing and/or reading the customer OTP values? (and what's the value in preventing the reading of the OTP?)

ping @ghollingworth

@JamesH65
Copy link
Contributor Author

JamesH65 commented Jan 3, 2020

a) Yes, I think so.
b) Maybe, but not sure
c) I believe so

@ghost
Copy link

ghost commented Jan 3, 2020

b) Does this imply that the documentation in this PR should say e.g. "customer OTP programming enabled" rather than "OTP programming enabled?

As I understand it, the only way to program the OTP from Linux is to set the requisite options in config.txt and reboot. Therefore only customer OTP bits are capable of being programmed. I personally don't see the need to make the table more wordy by adding 'customer' to this page: the reason being it is not a page about OTP, and it links to a page that describes 'customer' OTP bits and additionally which bits are publicly documented. On the other hand, it would be more consistent if all pages referred to the bits as 'customer'.

@JamesH65
Copy link
Contributor Author

JamesH65 commented Jan 7, 2020

b) Does this imply that the documentation in this PR should say e.g. "customer OTP programming enabled" rather than "OTP programming enabled?

As I understand it, the only way to program the OTP from Linux is to set the requisite options in config.txt and reboot. Therefore only customer OTP bits are capable of being programmed. I personally don't see the need to make the table more wordy by adding 'customer' to this page: the reason being it is not a page about OTP, and it links to a page that describes 'customer' OTP bits and additionally which bits are publicly documented. On the other hand, it would be more consistent if all pages referred to the bits as 'customer'.

I think there is a mailbox that will allow you to set bits, but need to check. Not got full answers to @lurch Q's above - will try to find out today.

@JamesH65
Copy link
Contributor Author

a) Yes, this prevent them being set in the future, they can still be read.
b) The mailbox only allows setting of customer OTP bits
c) Yes, but the disallow read options appears to be unpublished. Will check whether that is intentional.

@JamesH65
Copy link
Contributor Author

Added more detail in the linked OTP page https://github.com/raspberrypi/documentation/pull/1411/files

@lurch
Copy link
Contributor

lurch commented Feb 25, 2020

Are the 'N', 'O' and 'Q' bits documented in this PR visible in the output of vcgencmd otp_dump? If so, could/should they be added to https://github.com/raspberrypi/documentation/blob/master/hardware/raspberrypi/otpbits.md ?

@ghost
Copy link

ghost commented Feb 25, 2020

Are the 'N', 'O' and 'Q' bits documented in this PR visible in the output of vcgencmd otp_dump? If so, could/should they be added to https://github.com/raspberrypi/documentation/blob/master/hardware/raspberrypi/otpbits.md ?

OTP register location 30 would appear to be where the revision code (all 32 bits, including N, O and Q) is actually stored. Rather than duplicate the information that is in revision-codes/README.md in otpbits.md as well, what about making the description for location 30 in otpbits.md (which currently reads 'revision number') a link to revision-codes/README.md? That would be consistent with the line below it, which links to a page describing the customer OTP bits.

And maybe add a quick mention to revision-codes/README.md that the revision codes are stored in OTP register location 30, with a link to otpbits.md?

Also, some terminology is slightly inconsistent between these pages - OTP bits v. OTP values, revision codes v. revision number

@lurch
Copy link
Contributor

lurch commented Feb 25, 2020

what about making the description for location 30 in otpbits.md (which currently reads 'revision number') a link to revision-codes/README.md?

Done, good call 👍

And maybe add a quick mention to revision-codes/README.md that the revision codes are stored in OTP register location 30, with a link to otpbits.md?

I'm not sure if that'd be too confusing? Probably best to have that page only document /proc/cpuinfo as "the way" to get the revision code / value / number 😉

Also tidy up some table alignment
@JamesH65
Copy link
Contributor Author

JamesH65 commented Mar 9, 2020

Any final objections to merge?

@lurch
Copy link
Contributor

lurch commented Mar 9, 2020

Maybe another nitpick: whilst I think 1: Overvoltage disabled makes sense (and AIUI means "permanently disabled"), I'm not sure that 0: Overvoltage enabled is entirely clear. People might misinterpret it to mean "overvoltage is currently being used", whereas AFAIK it means "you can use the overvoltage options in config.txt if you want to"?

Maybe "allowed / disallowed" would make more sense here than "enabled / disabled"? 🤷‍♂️

@ghost
Copy link

ghost commented Mar 10, 2020

Maybe another nitpick: whilst I think 1: Overvoltage disabled makes sense (and AIUI means "permanently disabled"), I'm not sure that 0: Overvoltage enabled is entirely clear. People might misinterpret it to mean "overvoltage is currently being used", whereas AFAIK it means "you can use the overvoltage options in config.txt if you want to"?

Maybe "allowed / disallowed" would make more sense here than "enabled / disabled"? 🤷‍♂

I think the 4 new bits being documented could do with a short introduction above the table, since they are not actually part of the revision code as such. Something like "There are also 3 bits which can be used to disable certain features, and a single bit which is set by the firmware to indicate whether the warranty has been voided by overclocking".

Also, instead of the existing "overvoltage enable, overvoltage disabled" etc entries for the 3 feature bits, it would perhaps be more clear to instead state something line "When set, the firmware will prevent any overvoltage from being applied". Similarly with OTP programming and OTP reading. And a note somewhere that all Pis ship with these 3 bits unset.

@MichaIng
Copy link
Contributor

MichaIng commented Mar 16, 2020

Btw the "enabled/disabled" confused me as well and I exactly interpreted it as "is/was (not) overvolted" at first. allowed/disallowed sounds better to me.

As of #1348

  • Change bit 24 to:
    w | Pre-RPi2 warranty bit | 0: Warranty intact 1: Warranty voided
  • Add bit 26:
    S | Stress test bit | 0: Stress test succeeded 1: Stress test failed with more details outside the table like:
    *Devices which failed an internal stress test, sold to industrial customers only, with lower maximum clock rates at lower price. One should never get such device without knowing explicitly about its limitations.

@lurch
Copy link
Contributor

lurch commented Mar 16, 2020

ping @pelwell & @popcornmix - are the bits 24 and 26 above correct? Are they worth adding to the documentation?

@pelwell
Copy link
Contributor

pelwell commented Mar 16, 2020

We can work on the phrasing, but factually that is correct. And yes, they should be added.

@ghost
Copy link

ghost commented Mar 17, 2020

Just checked with Oxford Dictionaries (via lexicon.com/en) as "disallowed" didn't sound quite right to me. It seems "disallow" is not the opposite of "allow", in the sense we are trying to use it here. It only has one definition (unlike "allow" which has several). The definition of "disallow" is "refuse to declare valid" which is not what we're trying to say here.

The correct term here would be "not allowed".

@MichaIng
Copy link
Contributor

MichaIng commented Mar 17, 2020

@andrum99
Hmm, all dictionaries I checked have "To refuse to allow" as 1. or 2. definition, which is exactly what we want to say?

@ghost
Copy link

ghost commented Mar 17, 2020

@andrum99
Hmm, all dictionaries I checked have "To refuse to allow" as 1. or 2. definition, which is exactly what we want to say?

OK, but it does sound clunky. I withdraw my objection.

@lurch
Copy link
Contributor

lurch commented Mar 17, 2020

Even if it's not strictly speaking 100% grammatically correct, it would seem obvious from the way that it's being used here that it means exactly "not allowed" ??

@MichaIng
Copy link
Contributor

Otherwise, why not use "not allowed". While it IMO doesn't make anything better or worse, it at least matches usual coding and mathematical syntax to negate something with leading no or ! etc, something RPi customers are usually familiar with 😄.

@lurch
Copy link
Contributor

lurch commented Mar 18, 2020

🤷‍♂️ I just thought that "allowed / disallowed" nicely mirrored "enabled / disabled".
I don't care either way, but as @JamesH65 created this PR I'll let him have the final decision! 😜

@JamesH65 JamesH65 merged commit 1888b66 into master Apr 15, 2020
@JamesH65 JamesH65 deleted the JamesH65-patch-revision-usercodes branch May 5, 2020 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants