[coreboot-gerrit] [XL] Change in coreboot[main]: soc/sifive/fu740: Add FU740 SOC

[Maximilian Brune (Code Review)]

Attention is currently required from: Martin L Roth, Philipp Hug, ron minnich.

Maximilian Brune has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/76689?usp=email )

Change subject: soc/sifive/fu740: Add FU740 SOC
......................................................................

Patch Set 13:

(9 comments)

File src/soc/sifive/fu740/Makefile.inc:

https://review.coreboot.org/c/coreboot/+/76689/comment/7841b30c_e593f5f9 :
PS12, Line 3: ifdef
> This needs to be `ifeq ($(CONFIG_SOC_SIFIVE_FU740),y)` […]

That was quite the blooper. I am wondering why it even compiled then.

File src/soc/sifive/fu740/cbmem.c:

https://review.coreboot.org/c/coreboot/+/76689/comment/793f52ba_fdd8254f :
PS12, Line 11: #define FU740_MAXDRAM 0x800000000ULL // 32 GiB
> Can we move this outside of the function?

I only put it in here to emphasis it is only used here and for all other operations `sdram_size()` is supposed to be the source of truth regarding dram size.

File src/soc/sifive/fu740/clint.c:

https://review.coreboot.org/c/coreboot/+/76689/comment/0b49f026_6d0af326 :
PS12, Line 11: (uint64_t *)(FU740_CLINT + 0xbff8);
             : 	HLS()->timecmp = (uint64_t *)(FU740_CLINT + 0x4000 + 8 * hart_id);
> Can the magic numbers be made into #defines?

Done

https://review.coreboot.org/c/coreboot/+/76689/comment/0d7131c0_e5f85ad4 :
PS12, Line 17:  4 * (uintptr_t)hartid
> I assume this means we're always 32-bit? If that's not the case, should hartid be cast to a 32-bit v […]

We are 64 bit always. Therefore before casting it to a (void *) I need to cast it to a 64 bit value.

File src/soc/sifive/fu740/clock.c:

https://review.coreboot.org/c/coreboot/+/76689/comment/5df3028d_d835770f :
PS12, Line 16: *
> Nit: Add some spaces around '*'?

Done

https://review.coreboot.org/c/coreboot/+/76689/comment/9c07af08_db2a32c5 :
PS12, Line 153: Oszillator
> oscillator?

I accidentally used the german word.

File src/soc/sifive/fu740/ddrregs.c:

https://review.coreboot.org/c/coreboot/+/76689/comment/a4da0bb0_0d41ad04 :
PS12, Line 1: /* SPDX-License-Identifier: Apache-2.0 */
> Did this file come from somewhere else? Why not just GPL?

First I copied the magic values from our fu540 implementation (that is where the apache license comes from), but that didn't work so I got my values from SiFive u-boot devicetree. Since that is also GPL licensed, I removed the Apache license now.

https://review.coreboot.org/c/coreboot/+/76689/comment/0092a354_92b24d89 :
PS12, Line 9: DENALI_PHY_00_DATA
> Why bother with the #defines? why not just include the data directly?

good point. They are magic values anyway.

File src/soc/sifive/fu740/spi_internal.h:

https://review.coreboot.org/c/coreboot/+/76689/comment/784c9867_5b069fbf :
PS12, Line 1: /* SPDX-License-Identifier: GPL-2.0-only */
> Put this into the include directory?

This file contains the SPI stuff that is not needed outside of the SOC code. `include/soc/spi.h` contains the SPI code needed for configurating things in mainboard code. I tried to separate it similar to the fu540 SOC for future SiFive SOC common code.