Rework fuse handling? - Githubissues

cmfcmf commented 4 years ago

I think the fuse handling as it is now is confusing. It currently works by OR-ing FUSE_RES_BITS_HIGH with the individual fuse settings. I'll further detail my thinking with the PFS173 as an example.

I would expect that calling PDK_SET_FUSE(); would use the factory default values (i.e., no security, strong PB4/PB5, fast bootup). However, it currently sets a mix of factory default and non-default values:

non-default: security on, PB4/PB5 normal, bootup slow
~~default: all other code options: lvr, interrupt src0/1, comparator, pwm source, tmx source~~

If we later add a #define for the currently missing code options (such as lvr, comparator, ...), we would also need to change FUSE_RES_BITS_HIGH. This, in turn, would change the behavior of calling PDK_SET_FUSE();!

I suggest to rework the fuse handling like this:

#define FUSE_RES_BITS_HIGH  0b111111111111111

#define FUSE_SECURITY_ON    0b111111111111110
#define FUSE_SECURITY_OFF   0b111111111111111
#define FUSE_PB4_PB5_NORMAL 0b111111011111111
#define FUSE_PB4_PB5_STRONG 0b111111111111111
#define FUSE_BOOTUP_SLOW    0b110011111111111
#define FUSE_BOOTUP_FAST    0b111111111111111

// ...

#define PDK_SET_FUSE(f) \
    __asm__( \
      ".area FUSE (ABS)                               \n" \
      ".org ("_STR(FUSE_WORD_ADDR)"*2)                \n" \
      ".word ("_STR(FUSE_RES_BITS_HIGH)"&"_STR(f)")   \n" \ 
      ".area CODE                                     \n" \
    )

The macro would now work as expected:

PDK_SET_FUSE(); sets all to factory values, even if we later add definitions for more fuses
PDK_SET_FUSE(FUSE_SECURITY_ON); enabled security, leaves all other values at factory default
PDK_SET_FUSE(FUSE_SECURITY_ON & FUSE_PB4_PB5_NORMAL); enabled security and normal I/O, all bootup at factory default.

Note that you have to bitwise-and the fuse settings instead of bitwise-oring them. If we don't like that, we could also instead invert the fuse definitions:

#define FUSE_RES_BITS_HIGH  0b0111111111111111

#define FUSE_SECURITY_ON     0b000000000000001
#define FUSE_SECURITY_OFF    0b000000000000000
#define FUSE_PB4_PB5_NORMAL  0b000000100000000
#define FUSE_PB4_PB5_STRONG  0b000000000000000
#define FUSE_BOOTUP_SLOW     0b001100000000000
#define FUSE_BOOTUP_FAST     0b000000000000000

// ...

#define PDK_SET_FUSE(f) \
    __asm__( \
      ".area FUSE (ABS)                                    \n" \
      ".org ("_STR(FUSE_WORD_ADDR)"*2)                     \n" \
      ".word ("_STR(FUSE_RES_BITS_HIGH)" & ~("_STR(f)"))   \n" \ 
      ".area CODE                                          \n" \
    )

(I'm not entirely sure if that is how inline assembler works and if bitwise inverting 0b000000000000000 will lead 0b11111111 or 0b1111111111111111)

This would allow us to use PDK_SET_FUSE(FUSE_SECURITY_ON | FUSE_PB4_PB5_NORMAL);

cmfcmf commented 4 years ago

Apparently my assumption was wrong: "code option" != "easy pdk fuse" and all other code options are configured in other, undocumented registers.

The point about PDK_SET_FUSE(); not using the factory defaults still stands though.

serisman commented 4 years ago

@cmfcmf - Because of the incorrect assumptions regarding FUSE vs. MISC2/MISC_LVR/ROP in your first post, it's a little hard to figure out what exactly you are proposing, but I'll try to respond to some of your concerns.

First, the concept of 'default' value is a little ambiguous...

One interpretation of default value, would be to use the default value as defined in the Padauk IDE's .INC source files. The potential problem with this is they disagree a bit with each other. For instance, the default value for the PFS173 would include Security Off, but the default value for PMS152 uses Security On. That would potentially be a bit confusing to have the behavior change from IC to IC. And, sometimes you would have to OR in an option, other times you would have to AND it in, which would get complex rapidly.

Another interpretation of default value, would be to use the value as comes pre-installed on a 'blank IC. Because of the way ROM works, this means that every bit that matters starts out as a 1, and would have to be programmed down to 0 as needed. So, AND would have to be used instead of OR for FUSE options. This is also a bit confusing as sometimes (depending on the IC) the 'unused' bits that are supposed to be low are a 1, and it is unclear what happens if they aren't actually programmed to a 0.

So, currently we are using a mask that sets all the required low bits to 0 properly, and leaves all the option bits at 0, unless turned on by OR'ing an option's value. This means we are more consistent from IC to IC. Maybe it would make more sense to invert things and instead AND in an option's value, but that seems less consistent to the way we usually set up registers (almost always done using OR).

Sometimes just using PDK_SET_FUSE() without passing anything in might give the correct defaults, but that doesn't seem like something that should be relied on. I would think best practice is to always define each and every option that matters when setting a FUSE value. We could actually change the definition of PDK_SET_FUSE to require each option to be set, but that might be a little too restrictive.

Perhaps @freepdk also wants to weigh in on why the fuse values are set the way they are?

cmfcmf commented 4 years ago

Thank you for your detailed explanation. :+1: Given that there doesn't seem to be the defition of what exactly should be the default value, I'm fine with leaving everything as is.

We could actually change the definition of PDK_SET_FUSE to require each option to be set, but that might be a little too restrictive.

That would mean something along the lines of PDK_SET_FUSE(FUSE_SECURITY_ON, FUSE_PB4_PB5_NORMAL, FUSE_BOOTUP_SLOW);, so that the user cannot forget one of the fuse settings, right? The potential downside I'm seeing with this is that different µCs might be using a different number of fuses, therefore the PDK_SET_FUSE would potentially need to be defined per device.

serisman commented 4 years ago

Thank you for your detailed explanation. 👍 Given that there doesn't seem to be the defition of what exactly should be the default value, I'm fine with leaving everything as is.

Ok, sounds good. Go ahead and close this issue if you are happy with things as they are.

We could actually change the definition of PDK_SET_FUSE to require each option to be set, but that might be a little too restrictive.

That would mean something along the lines of PDK_SET_FUSE(FUSE_SECURITY_ON, FUSE_PB4_PB5_NORMAL, FUSE_BOOTUP_SLOW);, so that the user cannot forget one of the fuse settings, right?

Yes, exactly. Again, not recommended, but that is an option.

The potential downside I'm seeing with this is that different µCs might be using a different number of fuses, therefore the PDK_SET_FUSE would potentially need to be defined per device.

I agree with the downside you mentioned.

Another potential downside would be less portable code. For instance, what if your program only cares about FUSE_SECURITY_OFF and FUSE_BOOTUP_FAST. With PDK_SET_FUSE the way it currently is, the same code could be used across most (if not all) µCs, and the other more specific fuse options would just get set to their 'defaults' (all 1s). Changing to the above more restrictive PDK_SET_FUSE option, would mean a lot of #ifdef lines making for really messy source code.

free-pdk / pdk-includes

Rework fuse handling? #4