Hi Quentin, On 2024-04-02 12:38, Quentin Schulz wrote: > Hi Jonas, > > On 3/29/24 20:01, Jonas Karlman wrote: >> Extend the Generic RK3566/RK3568 target to also include support for SPI >> flash, USB OTG, RockUSB and UMS. >> >> Also fix sdmmc alias, include missing pinctrl and add broken-cd prop to >> fix use of SD-card in linux. >> > > I think we would have benefit with more and smaller commits there, but > since you're the one mainly maintaining those generic devices, up to you.
I can try to split it in a v2. > > [...] > >> &sdmmc0 { >> + broken-cd; >> bus-width = <4>; >> cap-sd-highspeed; >> disable-wp; >> no-mmc; >> no-sdio; >> pinctrl-names = "default"; >> - pinctrl-0 = <&sdmmc0_bus4 &sdmmc0_clk &sdmmc0_cmd>; >> + pinctrl-0 = <&sdmmc0_bus4 &sdmmc0_clk &sdmmc0_cmd &sdmmc0_det>; > > This is... surprising. > > `broken-cd` but we still mux the SDMMC_DET pin in the SD card detect > function? > > According to the dt binding, if broken-cd is provided, we should do > polling. If neither cd-gpios nor broken-cd is passed, host native card > detect will be used (which I assume means the SD card controller handles > this internally). > > c.f. > https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/mmc/mmc-controller.yaml#L33 > > What are we supposed to do there actually, because this seems to be > contradicting itself? The generic DTs is intended to be able to be use as control FDT in U-Boot with any board that mostly follows reference hw dedign as close as possible. broken-cd was added to fool U-Boot (and possible Linux) into thinking a card is present. And the sdmmc0_det pinctrl was added to satisfy the controller logic and auto jtag. When I tried to boot into linux with the control FDT prior to this, only eMMC was detected and working, after these changes SD-card was working. Will re-try without broken-cd for v2. > >> status = "okay"; >> }; >> >> +&sfc { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + status = "okay"; >> + >> + flash@0 { >> + compatible = "jedec,spi-nor"; >> + reg = <0>; >> + spi-max-frequency = <24000000>; > > Random thought, but shouldn't we update common/spl/spl_spi.c to read > this value instead of using CONFIG_SF_DEFAULT_SPEED? (Nothing to do in > this patch series though :) ). For U-Boot proper the value from this prop is used, SF_DEFAULT_SPEED is only used in SPL. > > [...] > >> diff --git a/configs/generic-rk3568_defconfig >> b/configs/generic-rk3568_defconfig >> index e7d5e55bbfd8..b458080cd539 100644 >> --- a/configs/generic-rk3568_defconfig >> +++ b/configs/generic-rk3568_defconfig >> @@ -3,17 +3,22 @@ CONFIG_SKIP_LOWLEVEL_INIT=y >> CONFIG_COUNTER_FREQUENCY=24000000 >> CONFIG_ARCH_ROCKCHIP=y >> CONFIG_NR_DRAM_BANKS=2 >> +CONFIG_SF_DEFAULT_SPEED=24000000 >> CONFIG_DEFAULT_DEVICE_TREE="rk3568-generic" >> CONFIG_ROCKCHIP_RK3568=y >> +CONFIG_ROCKCHIP_SPI_IMAGE=y >> CONFIG_SPL_SERIAL=y >> CONFIG_DEBUG_UART_BASE=0xFE660000 >> CONFIG_DEBUG_UART_CLOCK=24000000 >> +CONFIG_SPL_SPI_FLASH_SUPPORT=y >> +CONFIG_SPL_SPI=y >> CONFIG_SYS_LOAD_ADDR=0xc00800 >> CONFIG_DEBUG_UART=y >> CONFIG_FIT=y >> CONFIG_FIT_VERBOSE=y >> CONFIG_SPL_FIT_SIGNATURE=y >> CONFIG_SPL_LOAD_FIT=y >> +# CONFIG_BOOTMETH_VBE is not set >> CONFIG_LEGACY_IMAGE_FORMAT=y >> CONFIG_DEFAULT_FDT_FILE="rockchip/rk3568-generic.dtb" >> # CONFIG_DISPLAY_CPUINFO is not set >> @@ -21,32 +26,58 @@ CONFIG_DISPLAY_BOARDINFO_LATE=y >> CONFIG_SPL_MAX_SIZE=0x40000 >> CONFIG_SPL_PAD_TO=0x7f8000 >> # CONFIG_SPL_RAW_IMAGE_SUPPORT is not set >> +CONFIG_SPL_SPI_LOAD=y >> +CONFIG_SYS_SPI_U_BOOT_OFFS=0x60000 >> CONFIG_SPL_ATF=y >> CONFIG_CMD_GPIO=y >> CONFIG_CMD_GPT=y >> CONFIG_CMD_MMC=y >> +CONFIG_CMD_ROCKUSB=y >> +CONFIG_CMD_USB_MASS_STORAGE=y >> # CONFIG_CMD_SETEXPR is not set >> # CONFIG_SPL_DOS_PARTITION is not set >> CONFIG_SPL_OF_CONTROL=y >> CONFIG_OF_LIVE=y >> CONFIG_OF_SPL_REMOVE_PROPS="clock-names interrupt-parent assigned-clocks >> assigned-clock-rates assigned-clock-parents" >> +CONFIG_SYS_RELOC_GD_ENV_ADDR=y >> +# CONFIG_NET is not set > > This seems surprising, do you really want to get rid of network support > for the generic board defconfig? My intention with the generic targets is that they only contain bare minimum to boot from emmc/sd-card/spi flash. And a future possible use could be to replace vendor usbplug blob and/or for other flashing or recovery purposes. So network support should not be needed, also saves a little on binary size and boot speed. Regards, Jonas > > Cheers, > Quentin