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

[Quentin Schulz]

Hi Jonas,

On 4/2/24 14:37, Jonas Karlman wrote:
> 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.
> 

You shouldn't need to fool auto jtag anymore on RK3588 since 
https://source.denx.de/u-boot/u-boot/-/commit/5d710738bb1e0ff2bb93ce7baf4c9691ce919b53 
since it would be disabled except if you enable it by hand via the symbol.

I assume you need a similar patch for RK3568.

Linux disables auto jtag automatically for RK3588 since 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/soc/rockchip/grf.c?id=6f6878ec6faf16a5f36761c93da6ea9cf09adb33. 
I guess you need a similar patch for RK3568?

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

I think it'd make sense to properly patch this by disabling autojtag 
feature instead :)

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

Yup, but I assume we have DM support for SPI flashes in SPL, so we could 
do this instead of having to rely on a defconfig symbol :)

>>
>> [...]
>>
>>> 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.
> 

netboot/pxe/tftp is nice for debugging, but I understand :)

Cheers,
Quentin