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.

Sign up for GitHub

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

Jump to bottom

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

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

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 36 additions & 3 deletions software/glasgow/applet/memory/_25x/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,11 @@ def elaborate(self, platform):


class Memory25xInterface:
def __init__(self, interface, logger):
def __init__(self, interface, logger, addressing="3byte"):
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 •
edited
Loading

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.


def _log(self, message, *args):
self._logger.log(self._level, "25x: " + message, *args)
Expand All @@ -71,6 +72,11 @@ async def _command(self, cmd, arg=[], dummy=0, ret=0):

return result

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.


async def wakeup(self):
self._log("wakeup")
await self._command(0xAB, dummy=4)
Expand All @@ -95,8 +101,23 @@ async def read_manufacturer_long_device_id(self):
await self._command(0x9F, ret=3))
return (manufacturer_id, device_id)

async def _prepare_addr(self, addr):
if addr >= (1<<24 if self.addressing == "3byte" else 1<<32):
raise Memory25xError("Address out of range")
if self.addressing == "EN4B":
# some devices require WREN before EN4B
await self.write_enable()
await self._command(0xb7)
if self.addressing == "WREAR":
ear = addr >> 24
await self.write_enable()
await self._command(0xc5, arg=bytes([ear]))

def _format_addr(self, addr):
return bytes([(addr >> 16) & 0xff, (addr >> 8) & 0xff, addr & 0xff])
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.


async def _read_command(self, address, length, chunk_size, cmd, dummy=0,
callback=lambda done, total, status: None):
Expand All @@ -106,6 +127,7 @@ async def _read_command(self, address, length, chunk_size, cmd, dummy=0,
data = bytearray()
while length > len(data):
callback(len(data), length, f"reading address {address:#08x}")
await self._prepare_addr(address)
chunk = await self._command(cmd, arg=self._format_addr(address),
dummy=dummy, ret=min(chunk_size, length - len(data)))
data += chunk
Expand Down Expand Up @@ -161,11 +183,13 @@ async def write_status(self, status):

async def sector_erase(self, address):
self._log("sector erase addr=%#08x", address)
await self._prepare_addr(address)
await self._command(0x20, arg=self._format_addr(address))
while await self.write_in_progress(command="SECTOR ERASE"): pass

async def block_erase(self, address):
self._log("block erase addr=%#08x", address)
await self._prepare_addr(address)
await self._command(0x52, arg=self._format_addr(address))
while await self.write_in_progress(command="BLOCK ERASE"): pass

Expand All @@ -177,6 +201,7 @@ async def chip_erase(self):
async def page_program(self, address, data):
data = bytes(data)
self._log("page program addr=%#08x data=<%s>", address, data.hex())
await self._prepare_addr(address)
await self._command(0x02, arg=self._format_addr(address) + data)
while await self.write_in_progress(command="PAGE PROGRAM"): pass

Expand Down Expand Up @@ -281,6 +306,13 @@ def add_build_arguments(cls, parser, access):
access.add_pin_argument(parser, "sck", default=True, required=True)
access.add_pin_argument(parser, "hold", default=True)

parser.add_argument(
"--addressing",
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.


def build_subtarget(self, target, args):
subtarget = super().build_subtarget(target, args)
if args.pin_hold is not None:
Expand All @@ -291,7 +323,7 @@ def build_subtarget(self, target, args):

async def run(self, device, args):
spi_iface = await self.run_lower(Memory25xApplet, device, args)
return Memory25xInterface(spi_iface, self.logger)
return Memory25xInterface(spi_iface, self.logger, addressing=args.addressing)

@classmethod
def add_interact_arguments(cls, parser):
Expand Down Expand Up @@ -402,6 +434,7 @@ def _show_progress(done, total, status):

async def interact(self, device, args, m25x_iface):
await m25x_iface.wakeup()
await m25x_iface.reset()

if args.operation in ("program-page", "program",
"erase-sector", "erase-block", "erase-chip",
Expand Down
Loading