Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

applet.memory.25x: support 4-byte addressing #666

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

duskwuff
Copy link

@duskwuff duskwuff commented Aug 9, 2024

Flash devices larger than 32 MB have a couple of different ways of supporting 32-bit addressing. Add support for two of the most common ones, and raise an error if attempting to access an address unsupported by the current addressing mode.

Since 4-byte mode and EAR both set volatile registers which will have an effect on subsequent commands, perform a reset on the flash chip at startup.

Flash devices larger than 32 MB have a couple of different ways of
supporting 32-bit addressing. Add support for two of the most common
ones, and raise an error if attempting to access an address unsupported
by the current addressing mode.

Since 4-byte mode and EAR both set volatile registers which will have an
effect on subsequent commands, perform a reset on the flash chip at
startup.
Copy link
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I think this requires some further work to be up to our practices.

async def reset(self):
self._log("reset")
await self._command(0x66)
await self._command(0x99)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this required for the addressing to work or is it separate? In the latter case, please put reset related changes into a separate commit.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It technically works without the reset but it's fragile - in particular, EN4B leaves the memory in a state where it'll expect four-byte addresses for all addressed commands, so attempting to interact with it with three-byte commands afterwards will behave unpredictably.

I'll go ahead and split this out into a separate commit.

self.lower = interface
self._logger = logger
self._level = logging.DEBUG if self._logger.name == __name__ else logging.TRACE
self.addressing = addressing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should definitely be a private member, I suggest self._addr_mode which reads a little nicer and also has the same total length. The value should be an enum, maybe Memory25XAddrMode, with the names of elements being something slightly more descriptive than the current string options (unless I misremember something and these come from some document as-is). The values can be enum.auto() I think.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha! I wasn't aware of enum (it's been a while since I've used Python). I'll use that - thanks.

The names are based on the command mnemonics used by Macronix. Comparing a couple of other datasheets doesn't show a lot of consistency in what names are used for these commands, e.g. Micron just calls command 0xB7 "ENTER 4-BYTE ADDRESS MODE", GigaDevice calls it "Enable 4-Byte Mode", Cypress uses the same name as Micron but abbreviates it as 4BAM… etc. The SFDP spec describes the addressing modes without naming them at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you describe the semantics of the addressing modes in text here? Then perhaps I can offer a better naming scheme.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • EN4B: issue command 0xB7, and all instructions which previously used a three-byte address now expect four bytes. Issue command 0xE9 or a software reset to return to three-byte addresses. Some parts require a write-enable to enter this mode; this implementation sends one unconditionally.

  • WREAR: The top eight bits of the address live in a volatile extended address register ("EAR"). Issue command 0xC5 with a one-byte argument to update that register. This register is ignored if four-byte addressing is enabled (in parts which support both).

JESD216F describes a couple of other modes which I haven't implemented support for yet:

  • One is similar to WREAR, except the top bit of the register enables four-byte addressing (in which case the rest of the register is ignored).
  • One is similar to EN4B, but is controlled by a bit in a nonvolatile (!!) configuration register.
  • One is "there are dedicated instructions for four-byte addressing".
  • One is "all addresses are always four bytes".

Copy link
Member

@whitequark whitequark Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it's one of those kinds of modes.

Okay, here's my proposal:

class Memory25XAddrMode(enum.Enum):
    #: Addresses are always 3-byte long.
    Default = enum.auto()

    #: Addresses are always 4-byte long.
    AlwaysWide = enum.auto()

    #: Addresses are 3-byte or 4-byte long, with 4-byte mode being entered
    #: by a sequence of EN4B, or WREN and EN4B.
    ModalCommand = enum.auto()

    #: Addresses are 3-byte or 4-byte long, with 4-byte mode being entered
    #: by writing a top bit of a mode register using WREN and WREAR.
    ModalRegister = enum.auto()

    #: Addresses are 3-byte or 4-byte long, with 4-byte mode being entered
    #: by writing a XXX bit of a non-voltatile register using YYY.
    ModalNonVolatileRegister = enum.auto()

    #: Addresses are 3-byte, concatenated with a 4th byte contained in
    #: a register that can be written to by WREN and WREAR.
    HighByteRegister = enum.auto()

    #: There are separate commands for 4-byte addressing modes.
    NonModalCommand = enum.auto()

Does this make sense? I've tried to make a classification reflecting the ground reality as best as I could.

if self.addressing == "EN4B":
return bytes([(addr >> 24) & 0xff, (addr >> 16) & 0xff, (addr >> 8) & 0xff, addr & 0xff])
else:
return bytes([(addr >> 16) & 0xff, (addr >> 8) & 0xff, addr & 0xff])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these two functions can be combined, and probably should be (when would you call one and not the other?)

You could return the address from _prepare_addr.

type=str, choices=["3byte", "EN4B", "WREAR"], default="3byte",
help="Use MODE (3byte, EN4B, WREAR) for addressing (default: 3byte)",
metavar="MODE",
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be a build argument but rather a run argument. (It doesn't change the gateware at all.)

Also, there is no explanation at all of what the various modes mean and when should they be used. I am not sure where or if it should go (using SFDP to cover most cases might just be enough), but in general it should probably be a part of the applet description, in or after the paragraph which invokes identify currently.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I misunderstood the distinction between the types of arguments.

Also, there is no explanation at all of what the various modes mean and when should they be used.

I don't think there's any compelling reason to use one four-byte addressing mode over another; if you have a device large enough to need it, you use any mode your device supports. Once this can be autodetected from SFDP it may not need to be an option at all; I would be very surprised if there are any serial flash devices large enough to need four-byte addressing, but which don't support SFDP.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like an unusual thing to have but one of my goals with Glasgow is to support really weird obscure devices where possible. E.g. I've seen devices which seem to have been programmed with faulty ID information at the factory. I still wanted to support them, and they are.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That said I'm not going to die on this proverbial hill, and maybe you're just right here.

@whitequark
Copy link
Member

This now has a merge conflict with memory-25x, could you rebase please?

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.

2 participants