Okay, so I realize the title of this one is Kind Of A Lot, but I really could not come up with a more concise way to explain the thing I'm trying to describe here.
Basically, "as you know", the PMBus standard has a concept of "pages". Pages allow a device which provides multiple output rails to expose all of the same PMBus registers about all of its output rails on the same I2C address. The controller can select the page corresponding to a particular power rail and then send other PMBus commands, such as reading the output voltage, and then get back the output voltage on the rail represented by that page. This is a good thing which is useful and not a problem.
On top of this, Hubris has constructed a bunch of codegen infrastructure which allows us to generate code that has a Rust identifier for each power rail on the board, allowing the programmer to essentially say "I want VDDCR_CPU0_A0" and get back a handle which, when used to send a PMBus command, automatically selects the right mux configuration and I2C address for the device that supplies that rail, and which knows the PMBus page that representas that rail. This part is also good and useful and not a problem.
The part where it gets "fun and exciting" is the way we are currently selecting rails. Our code does this using the PAGE command (§11.10 PMBus Specification, Part II, Version 1.3.1). The way this works is basically you send an I2C write transaction with the PAGE command code followed by a byte which is the page number that you want to select. Now, the device is "on that page", and when you send a subsequent command to read or write some value, you get the value from the page which you have previously selected. the part of this that becomes fun and exciting is that, well, once you have put the device on a page, now it is on that page until someone else comes along and puts it on a different one. What I am saying here is that this is a state. This means that if some other part of Hubris, which believes the device is on a different page, sends a command to the device, it might end up reading or writing to the other page without noticing this.
I think our code ought to be safe from this because our drivers for PMBus devices always send the PAGE command immediately prior to actually doing the thing that they want to do, using the I2C driver's write_write, write_read, write_write_read_reg, and other function names that sound suspiciously like song lyrics. This is basically fine because we are telling the I2C driver "hey, please do all these transactions next before you try and go do any other transactions that someone else asked you to do". But, it feels a little bit sketchy as we could accidentally sneak in an un-paged command that we meant to be paged, and have it accidentally behave correctly sometimes if we have happened to most recently put the device on the page we wanted anyway. That scares me a little bit.
I think we really ought to change most of our code from using the PAGE command, to instead use the PAGE_PLUS_WRITE and PAGE_PLUS_READ commands (§11.14 and §11.15 PMBus Specification, Part II, Version 1.3.1, respectively). These let you send one I2C transaction that atomically selects the page you want and sends the command and any command data, and read back any response, rather than having to do two separate writes. I'm not totally sure whether these will result in the page getting unset, or put back to the previously set page, once the I2C transaction has ended, because the PMBus standard doesn't really seem to explain this. But, they still seem a little bit less sketchy to me, so maybe we should switch to using them?
Again, I dunno if this is actually that big a risk, since we are using the nice I2C driver APIs for doing multiple txns in a row pretty pervasively. Essentially we have kind of reimplemented the "set page and send command in one atomic thing" in our I2C driver, except that it's only atomic with regards to our I2C driver and not, say, if someone else were to grab control of the I2C bus and do its own stuff (which shouldn't happen but ???).
Okay, so I realize the title of this one is Kind Of A Lot, but I really could not come up with a more concise way to explain the thing I'm trying to describe here.
Basically, "as you know", the PMBus standard has a concept of "pages". Pages allow a device which provides multiple output rails to expose all of the same PMBus registers about all of its output rails on the same I2C address. The controller can select the page corresponding to a particular power rail and then send other PMBus commands, such as reading the output voltage, and then get back the output voltage on the rail represented by that page. This is a good thing which is useful and not a problem.
On top of this, Hubris has constructed a bunch of codegen infrastructure which allows us to generate code that has a Rust identifier for each power rail on the board, allowing the programmer to essentially say "I want
VDDCR_CPU0_A0" and get back a handle which, when used to send a PMBus command, automatically selects the right mux configuration and I2C address for the device that supplies that rail, and which knows the PMBus page that representas that rail. This part is also good and useful and not a problem.The part where it gets "fun and exciting" is the way we are currently selecting rails. Our code does this using the
PAGEcommand (§11.10 PMBus Specification, Part II, Version 1.3.1). The way this works is basically you send an I2C write transaction with thePAGEcommand code followed by a byte which is the page number that you want to select. Now, the device is "on that page", and when you send a subsequent command to read or write some value, you get the value from the page which you have previously selected. the part of this that becomes fun and exciting is that, well, once you have put the device on a page, now it is on that page until someone else comes along and puts it on a different one. What I am saying here is that this is a state. This means that if some other part of Hubris, which believes the device is on a different page, sends a command to the device, it might end up reading or writing to the other page without noticing this.I think our code ought to be safe from this because our drivers for PMBus devices always send the
PAGEcommand immediately prior to actually doing the thing that they want to do, using the I2C driver'swrite_write,write_read,write_write_read_reg, and other function names that sound suspiciously like song lyrics. This is basically fine because we are telling the I2C driver "hey, please do all these transactions next before you try and go do any other transactions that someone else asked you to do". But, it feels a little bit sketchy as we could accidentally sneak in an un-paged command that we meant to be paged, and have it accidentally behave correctly sometimes if we have happened to most recently put the device on the page we wanted anyway. That scares me a little bit.I think we really ought to change most of our code from using the
PAGEcommand, to instead use thePAGE_PLUS_WRITEandPAGE_PLUS_READcommands (§11.14 and §11.15 PMBus Specification, Part II, Version 1.3.1, respectively). These let you send one I2C transaction that atomically selects the page you want and sends the command and any command data, and read back any response, rather than having to do two separate writes. I'm not totally sure whether these will result in the page getting unset, or put back to the previously set page, once the I2C transaction has ended, because the PMBus standard doesn't really seem to explain this. But, they still seem a little bit less sketchy to me, so maybe we should switch to using them?Again, I dunno if this is actually that big a risk, since we are using the nice I2C driver APIs for doing multiple txns in a row pretty pervasively. Essentially we have kind of reimplemented the "set page and send command in one atomic thing" in our I2C driver, except that it's only atomic with regards to our I2C driver and not, say, if someone else were to grab control of the I2C bus and do its own stuff (which shouldn't happen but ???).