I’ve had a look over that code file and I’ve a few general questions. I’m not qualified to review the code but when you see code doing things differently you have to ask, and probably learn something new. I was a bit confused by the fact that at the start of the file there’s a macro defined to read a register, (there’s another for writing):
That macro then calls another function in target.c:
__a = target_read_u32(target, ctrl_base + (a), &__v);
That function, target_read_u32(), uses two additional parameters, “target” and “ctrl_base”, so before the read reg macro can be used these have to be defined. I’m thinking that this must be due to the RiscV architecture and I obviously have to find out about it. But the the first example in the file is:
#define FESPI_DISABLE_HW_MODE() FESPI_WRITE_REG(FESPI_REG_FCTRL, \
FESPI_READ_REG(FESPI_REG_FCTRL) & ~FESPI_FCTRL_EN)
So that macro is reading and writing the FESPI_REG_SCTRL Register, which is defined as:
#define FESPI_REG_FCTRL 0x60
That’s just the SPI flash Interface Control register so there’s nothing special about this register, well as far as I can see. It’s at offset 0x60 from the QSPI0 Control, (or perhaps QSPI1 or QSPI2). So given that there are 3 SPI Controllers you could argue that this is reusable SPI code but i’d thought this file is specifically for the program flash? Every time there’s a read or write of a register there’s mathematical operations involved, as well as a function call, with all its overhead.
So this is more general code which could be used for any of the SPI Interfaces, but if you were writing code for upload from a JTAG device, to program the Program Flash you’re only interested in one SPI Interface controller and probably want a small piece of stand alone code? Actually I started this with a view to adding support for the BlackMagicProbe JTAG device. The advice I was given from BMP people was that the blob of code that is uploaded to the target device, to program the flash, shouldn’t even call functions as you’re not sure that the stack and other functionality will be available. So I’d be inclined to write code like:
uint_32 *fctrl = 0x10014060;
variable = *fctrl;
variable = variable | 0x01;
*fctrl = variable
That’s long winded version just to be clear. And to be honest if I could mess with a linker script I’d hardwire that register address so there’s no ‘*’ Pointer operation and it just becomes “fctrl = variable”. Can’t do that in this situation as this is an executable blob to be uploaded from the JTAG Device.
My other question would be about the use of bitwise operations, rather then bitfields. That could possibly be a religious war type discussion, but I’ve wondered about it. I think that generally bitfields are more readable then bitwise, but that’s just my opinion. I’ve seen comments where people have wanted bitfields removed from the C Programming language. People will often state that bitfields are more optimal but I’ve looked at code written for ARM and the gcc compiler has produced shorter bitfield code then bitwise. I must test the RiscV compiler actually.
All this is not helped by very sparse registers in which the remainder of the space is reserved. For example the SPI Serial Clock Mode Register, sckmode, used 2 bits with the remaining 30 bits reserved. I assume that means that you can’t do an operation like:
sckmode = 0b10;
Because you’ve just overwritten 30 bytes of reserved data?
My reluctance about bitwise operations increases when you have a multi-bit field. For example the 12bit “scale” divisor for the serial clock in the sckdiv register. To write to that section of the register you have to zero the bits, then a bitwise or operation with the required value:
sckdiv = sckdiv & ~0xFFF;
sckdiv = sckdiv | 0x123;
As opposed to a bitfield:
sckdiv.scale = 0x123;
And let the compiler sort it out, that’s what it’s there for
All of that is just my thoughts. The file has proved extremely helpful, thank you for pointing me in that direction, and much reading and coffee later it’s time I started my own attempt. No Function calls apparently. Hmm.
Just to check one last thing the Program Flash is on SPI-0, rather then 1 or 2.