fel-spiflash: Add support for V853 family #208
Conversation
Tested with V851S :)
Done! |
| @@ -166,7 +176,7 @@ static void gpio_set_cfgpin(feldev_handle *dev, int port_num, int pin_num, | |||
| uint32_t cfg_reg = port_base + 4 * (pin_num / 8); | |||
| uint32_t pin_idx = pin_num % 8; | |||
| uint32_t x = readl(cfg_reg); | |||
| x &= ~(0x7 << (pin_idx * 4)); | |||
| x &= ~(0xF << (pin_idx * 4)); | |||
There was a problem hiding this comment.
Apparently this SoC has the pin function select on 4 bits while others have it on 3 bits + 1 bit reserved.
It probably works to mask out the reserved bit in every case (especially since default values are 0 for other SoCs) but this should be mentioned it the commit log. Especially since the reset value is all 1s instead of all 0s like it was on other SoCs.
There was a problem hiding this comment.
I think that's fine, 0xf should work everywhere. Bit 3 in each field is not implemented on the "older" SoCs, so it's always 0, even if you write a 1 in there. And the reset value seems to 0x7 for quite a while now (after the A20 gen, I think).
| @@ -296,6 +312,11 @@ static bool spi0_init(feldev_handle *dev) | |||
| writel(0x00003180, SUNIV_AHB_APB_CFG); | |||
| /* divide by 32 */ | |||
| writel(CCM_SPI0_CLK_DIV_BY_32, SUN6I_SPI0_CCTL); | |||
| } else if (soc_info->soc_id == 0x1886) { /* V853 */ | |||
There was a problem hiding this comment.
The logic is the same as the next code block, only that you are using a different register address.
Please fold this into the next block and add variables and conditionals to set the right address for sun6i / v853 / sun4i, so we don't duplicate the logic.
There was a problem hiding this comment.
Yes, I agree. It looks a bit like some refactoring would help here, as I see all newer SoCs being in the same area, for instance the D1/T113 and A523/T527 require the very same SPI and GPIO changes.
There was a problem hiding this comment.
Hi,
thanks for sending a patch, much appreciated! The technical bits seem OK, but I think we can make this look nicer, see the comments inisde. I wonder if "NCAT2" is a better suited prefix for the new identifiers, that's how the BSPs describes the SoC using the "new" memory layout (and changed GPIO), as seen in the D1/T113-s and also the A523/T527.
| @@ -87,6 +87,10 @@ void fel_writel(feldev_handle *dev, uint32_t addr, uint32_t val); | |||
| #define H6_CCM_SPI_BGR (0x03001000 + 0x96C) | |||
| #define H6_CCM_SPI0_GATE_RESET (1 << 0 | 1 << 16) | |||
|
|
|||
| #define V853_CCM_SPI0_CLK (0x02001000 + 0x940) | |||
There was a problem hiding this comment.
Please align this the same way as the other lines before and after, so remove two spaces.
| @@ -126,6 +132,8 @@ static uint32_t gpio_base(feldev_handle *dev) | |||
| { | |||
| soc_info_t *soc_info = dev->soc_info; | |||
| switch (soc_info->soc_id) { | |||
| case 0x1886: /* V853 */ | |||
| return 0x02000018; | |||
There was a problem hiding this comment.
Sorry, but that looks like a horrible hack to accommodate for the larger per-port GPIO block of the newer SoCs (0x30 instead of 0x24 per port). We have support for that GPIO IP in uart0-helloworld-sdboot.c, please take some inspiration there for a proper solution. You should put the datasheet value of 0x02000000 here, as the other SoCs do.
| @@ -296,6 +312,11 @@ static bool spi0_init(feldev_handle *dev) | |||
| writel(0x00003180, SUNIV_AHB_APB_CFG); | |||
| /* divide by 32 */ | |||
| writel(CCM_SPI0_CLK_DIV_BY_32, SUN6I_SPI0_CCTL); | |||
| } else if (soc_info->soc_id == 0x1886) { /* V853 */ | |||
There was a problem hiding this comment.
Yes, I agree. It looks a bit like some refactoring would help here, as I see all newer SoCs being in the same area, for instance the D1/T113 and A523/T527 require the very same SPI and GPIO changes.
| @@ -166,7 +176,7 @@ static void gpio_set_cfgpin(feldev_handle *dev, int port_num, int pin_num, | |||
| uint32_t cfg_reg = port_base + 4 * (pin_num / 8); | |||
| uint32_t pin_idx = pin_num % 8; | |||
| uint32_t x = readl(cfg_reg); | |||
| x &= ~(0x7 << (pin_idx * 4)); | |||
| x &= ~(0xF << (pin_idx * 4)); | |||
There was a problem hiding this comment.
I think that's fine, 0xf should work everywhere. Bit 3 in each field is not implemented on the "older" SoCs, so it's always 0, even if you write a 1 in there. And the reset value seems to 0x7 for quite a while now (after the A20 gen, I think).
Tested with V851S on a "Wireless Carplay Adapter" dongle :)