[PATCH 18/19] rockchip: rk356x-generic: Add support for SPI flash and USB OTG

[Jonas Karlman]

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 at 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