SPI flash rework #224
SPI flash rework #224
Conversation
Our soc_info struct so far has three boolean flags. Replace those separate boolean members with one combined "flags" bitfield member, to allow adding flags later more easily. Signed-off-by: Andre Przywara <osp@andrep.de>
Some sunxi-fel features like the SPI flash access need to program the GPIO/pinctrl and clock controllers, located at different MMIO base addresses across all the various SoC generations. Add a GPIO and clock controller base address to our soc_info struct, and add the respective base addresses for each supported SoC to the SoC table. Also add flags to mark the generation of each IP: the D1 pinctrl change (each GPIO port uses 48 bytes instead of 36), and the H6 clock change (moving clock gates and resets into per-peripheral registers). This is not used at the moment, but will be soon. Signed-off-by: Andre Przywara <osp@andrep.de>
Now that we store the base addresses of the GPIO and clock controllers for each SoC in our soc table, let's use those values instead of determining them ourselves, in the SPI flash code. This involves several changes: - removing the base offset from each register address - supporting the NCAT2 pin controller, with a larger per-port size - using info about clock and pinctrl generation from soc table This will simplify future SoC support, as we can simply put the respective base addresses in the soc table, without touching the SPI flash code. Signed-off-by: Andre Przywara <osp@andrep.de>
Use the main soc table to also store the first SPI controller's MMIO base address, so that we don't need to decide this in the SPI flash code. Only SoCs that are covered by fel-spiflash.c get a base address assigned, so this value being not 0 is used as an indicator that the SoC is supported. This will simplify supporting future generations of SoCs. Signed-off-by: Andre Przywara <osp@andrep.de>
The bootable SPI flash is always connected to PortC pins, though the exact pin numbers within PortC and their pinmux differs between the SoCs. We always need four pins (MOSI, MISO, CLK, CS), so pack their pin numbers into one 32-bit word and store that in our soc table. Also store the required pinmux (which is always the same for all four pins) in the table. Then use this information in the SPI flash code to extract the pins again and configure them accordingly. This does away with the fragile switch/case construct we used to configure the pins so far. Signed-off-by: Andre Przywara <osp@andrep.de>
|
@apritzel thanks for cleaning this mess. |
|
|
||
| /* Pack four pin numbers into one uint32_t, using one byte per pin */ | ||
| #define SPI_PINS(p1, p2, p3, p4) \ | ||
| ((p1) << 0 | (p2) << 8 | (p3) << 16 | (p4) << 24) |
There was a problem hiding this comment.
4 pins are enough for SPI. But T113 also have WP and HOLD signals that are controlled by SPI module.
My test HW have both of these signals connected to flash chip. It works fine without setting correct pinmux for WP and HOLD and leaving it in default state. But I prefer to properly configure these pins.
See https://github.com/dron0gus/sunxi-tools/blob/c6612c5157e78a474b5c2b2129a26b0da5b394a6/fel-spiflash.c#L285
May be extend spi_pins to uint64_t, unconditionally configure 4 first and optionally four other?
/* Setup SPI0 pins muxing */
for (size_t i = 0; i < 4; i++)
gpio_set_cfgpin(dev, PC, spi_pin(soc_info, i), soc_info->spi_pinmux);
for (size_t i = 4; i < 8; i++)
if (spi_pin(soc_info, i))
gpio_set_cfgpin(dev, PC, spi_pin(soc_info, i), soc_info->spi_pinmux);
There was a problem hiding this comment.
Wouldn't a struct with named attributes be better for this? We don't need to differentiate between them in the code (at least for now), but it would be easier to read and know if a pin is used or not.
For example:
#define SPI_PIN_NONE -1
// Struct members are signed in order to flag unused ones with -1
typedef struct {
int8_t cs;
int8_t clk;
int8_t mosi;
int8_t miso;
int8_t wp;
int8_t hold;
} spi_pins_t;
Just an idea, for me it would be easier to read at first glance. Let me know your thoughts!
There was a problem hiding this comment.
Well, I don't think the BootROM cares about HOLD and WP, and we try to mimic what the BROM does, for maximum compatibility. Since we don't use QSPI, for our operation both are certainly optional.
And also please keep in mind that sunxi-fel works on a SoC level, not on a board level. Other boards might not connect those pins, or even worse, connect PC7 to the relay that triggers the rocket launch ;-)
So do you really need those pins, for proper operation? Or did you just want to make it neat, by not leaving those pins unconfigured?
There was a problem hiding this comment.
@apritzel I agree with your arguments. No boards specific setup should be done here.
|
This PR is awesome. By adding into Allwinner V853. |
4 participants
The proposed patches to support SPI flash access on more SoCs (#218 and #208) have issues, and are mainly struggling with the slight differences between the SoCs when it comes to clock and pinmux setup. This PR reworks the SPI flash code, mainly by moving SoC specific information into the existing
soc_info_table, so the SPI flash code can just read say the GPIO controller base address, without reverting to hard-to-maintain switch/case functions. Also this introduces a flags field, to include the three existing boolean fields, but also add some generation info (likeH6_STYLE_CLOCKS), which can now be cleanly attributed to each SoC, again without fragile switch/case constructs.I started with a separate SoC table in fel-spiflash.c, with just the info we need, but later felt this is just duplicating what we already have. Plus we can use the GPIO and clock base address later, for upcoming features, like MMC support or GPIO access.
Let me know what you think and whether this is a good idea or not.
@iscle @dron0gus please have a look, I think this should simplify the code changes you need to support your SoCs.