Re: [RFC PATCH 00/16] arm: Add Rockchip RK3588 support

[Eugen Hristev <eugen.hristev@collabora.com>]

On 3/13/23 16:21, Eugen Hristev wrote:
> On 3/13/23 12:00, Jonas Karlman wrote:
>> On 2023-03-13 09:42, Eugen Hristev wrote:
>>> On 3/13/23 00:34, Jonas Karlman wrote:
>>>> Hi Eugen,
>>>>
>>>> On 2023-03-08 09:57, Eugen Hristev wrote:
>>>>> On 1/29/23 11:04, Jonas Karlman wrote:
>>>>>> On 2023-01-27 14:21, Jagan Teki wrote:
>>>>>>> On Fri, 27 Jan 2023 at 05:13, Jonas Karlman <jonas@kwiboo.se> wrote:
>>>>>>>>
>>>>>>>> On 2023-01-26 23:16, Jonas Karlman wrote:
>>>>>>>>> Hi Jagan,
>>>>>>>>> On 2023-01-26 20:17, Jagan Teki wrote:
>>>>>>>>>> On Fri, 27 Jan 2023 at 00:33, Jonas Karlman <jonas@kwiboo.se> 
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On 2023-01-26 19:26, Jagan Teki wrote:
>>>>>>>>>>>> Hi Simon,
>>>>>>>>>>>>
>>>>>>>>>>>> On Thu, 26 Jan 2023 at 23:34, Simon Glass <sjg@chromium.org> 
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> Hi Jagan,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Thu, 26 Jan 2023 at 10:42, Jagan Teki <jagan@edgeble.ai> 
>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Thu, 26 Jan 2023 at 22:28, Jonas Karlman 
>>>>>>>>>>>>>> <jonas@kwiboo.se> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Hi Jagan,
>>>>>>>>>>>>>>> On 2023-01-26 17:51, Jagan Teki wrote:
>>>>>>>>>>>>>>>> Hi Jonas,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On Thu, 26 Jan 2023 at 04:17, Jonas Karlman 
>>>>>>>>>>>>>>>> <jonas@kwiboo.se> wrote:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Hi Jagan,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 2023-01-25 23:27, Jagan Teki wrote:
>>>>>>>>>>>>>>>>>> This series support Rockchip RK3588. All the device 
>>>>>>>>>>>>>>>>>> tree files are
>>>>>>>>>>>>>>>>>> synced from linux-next with the proper SHA1 mentioned 
>>>>>>>>>>>>>>>>>> in the commit
>>>>>>>>>>>>>>>>>> messages.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Unfortunately, the BL31 from rkbin is not compatible 
>>>>>>>>>>>>>>>>>> with U-Boot so
>>>>>>>>>>>>>>>>>> it is failing to load ATF entry from SPL and hang.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Verified below BL31 versions,
>>>>>>>>>>>>>>>>>>      bl31-v1.15
>>>>>>>>>>>>>>>>>>      bl31-v1.21
>>>>>>>>>>>>>>>>>>      bl31-v1.22
>>>>>>>>>>>>>>>>>>      bl31-v1.23
>>>>>>>>>>>>>>>>>>      bl31-v1.24
>>>>>>>>>>>>>>>>>>      bl31-v1.25
>>>>>>>>>>>>>>>>>>      bl31-v1.26
>>>>>>>>>>>>>>>>>>
>>>>>>>
>>>>>>> < snip >
>>>>>>>
>>>>>>>>>
>>>>>>>>> As you can see in the logs above there is timeout waiting for 
>>>>>>>>> data.
>>>>>>>>>
>>>>>>>>> I managed to find the issue and have a workaround that gets me 
>>>>>>>>> longer
>>>>>>>>> in the boot process, there still seem to be other issue with 
>>>>>>>>> the rk3588
>>>>>>>>> startup.
>>>>>>>>>
>>>>>>>>> --------
>>>>>>>>> U-Boot SPL 2023.01 (Jan 26 2023 - 22:03:00 +0000)
>>>>>>>>> Trying to boot from MMC1
>>>>>>>>> ## Checking hash(es) for config config-1 ... OK
>>>>>>>>> ## Checking hash(es) for Image atf-1 ... sha256+ OK
>>>>>>>>> ## Checking hash(es) for Image u-boot ... sha256+ OK
>>>>>>>>> ## Checking hash(es) for Image fdt-1 ... sha256+ OK
>>>>>>>>> ## Checking hash(es) for Image atf-2 ... sha256+ OK
>>>>>>>>> ## Checking hash(es) for Image atf-3 ... sha256+ OK
>>>>>>>>> INFO:    Preloader serial: 2
>>>>>>>>> NOTICE:  BL31: v2.3():v2.3-468-ge529a2760:derrick.huang
>>>>>>>>> NOTICE:  BL31: Built : 09:59:49, Nov 21 2022
>>>>>>>>> INFO:    spec: 0x1
>>>>>>>>> INFO:    ext 32k is not valid
>>>>>>>>> INFO:    ddr: stride-en 4CH
>>>>>>>>> INFO:    GICv3 without legacy support detected.
>>>>>>>>> INFO:    ARM GICv3 driver initialized in EL3
>>>>>>>>> INFO:    valid_cpu_msk=0xff bcore0_rst = 0x0, bcore1_rst = 0x0
>>>>>>>>> INFO:    system boots from cpu-hwid-0
>>>>>>>>> INFO:    idle_st=0x21fff, pd_st=0x11fff9, repair_st=0xfff70001
>>>>>>>>> INFO:    dfs DDR fsp_params[0].freq_mhz= 2112MHz
>>>>>>>>> INFO:    dfs DDR fsp_params[1].freq_mhz= 528MHz
>>>>>>>>> INFO:    dfs DDR fsp_params[2].freq_mhz= 1068MHz
>>>>>>>>> INFO:    dfs DDR fsp_params[3].freq_mhz= 1560MHz
>>>>>>>>> INFO:    BL31: Initialising Exception Handling Framework
>>>>>>>>> INFO:    BL31: Initializing runtime services
>>>>>>>>> WARNING: No OPTEE provided by BL2 boot loader, Booting device 
>>>>>>>>> without OPTEE initialization. SMC`s destined for OPTEE will 
>>>>>>>>> return SMC_UNK
>>>>>>>>> ERROR:   Error initializing runtime service opteed_fast
>>>>>>>>> INFO:    BL31: Preparing for EL3 exit to normal world
>>>>>>>>> INFO:    Entry point address = 0xa00000
>>>>>>>>> INFO:    SPSR = 0x3c9
>>>>>>>>> "Synchronous Abort" handler, esr 0x96000000
>>>>>>>>> elr: 0000000000a23650 lr : 0000000000a24d9c
>>>>>>>>> x0 : 0000000000b7fbe8 x1 : 350003402a0003f3
>>>>>>>>> x2 : 0000000000000000 x3 : 0000000000b80ff0
>>>>>>>>> x4 : 0000000000b80ff0 x5 : 0000000000b80e88
>>>>>>>>> x6 : 0000000000000054 x7 : 0000000000000044
>>>>>>>>> x8 : 000000000000000a x9 : 0000000000000000
>>>>>>>>> x10: 0000000000000034 x11: 0000000000000002
>>>>>>>>> x12: 0000000000001988 x13: 0000000000b7fadc
>>>>>>>>> x14: 0000000000a7e808 x15: 0000000000a7e808
>>>>>>>>> x16: 0000000000000000 x17: 0000000000000000
>>>>>>>>> x18: 0000000000b7fe50 x19: 0000000000b7fbe8
>>>>>>>>> x20: 000000003c14dc00 x21: 000000003c14dc00
>>>>>>>>> x22: 0000000000a7e808 x23: 0000000000000000
>>>>>>>>> x24: 0000000000000000 x25: 0000000000000000
>>>>>>>>> x26: 0000000000000000 x27: 0000000000000000
>>>>>>>>> x28: 0000000000000000 x29: 0000000000b7fb80
>>>>>>>>>
>>>>>>>>> Code: f90013f5 f9400001 b4000201 f9400021 (f9403435)
>>>>>>>>> Resetting CPU ...
>>>>>>>>> --------
>>>>>>>>>
>>>>>>>>> This was running on top of u-boot-dm/master 
>>>>>>>>> 060a65e899859dcbf42049a18be20ce7118e7c0e
>>>>>>>>> with some rk3568 patches and this series, see [1].
>>>>>>>>>
>>>>>>>>> The last 3 commits contains workaround to issue with sdmmc clock.
>>>>>>>>> dwmmc driver set sclk to (uint)-2, my workaround just adds a
>>>>>>>>> fallback to default 400khz clock.
>>>>>>>>>
>>>>>>>>> Next issue is the sync abort, looks it happens when u-boot
>>>>>>>>> tries to set clock rates based on devicetree. this is the
>>>>>>>>> last debug line before the crash.
>>>>>>>>>
>>>>>>>>> clk_set_rate(clk=0000000000b7fba8, rate=1008000000)
>>>>>>>>
>>>>>>>> With the commit at [2] I can now successfully run U-Boot proper.
>>>>>>>>
>>>>>>>> Source of the two main issues to get this series to run have 
>>>>>>>> been the scmi clocks.
>>>>>>>> Vendor u-boot first load its scmi driver before trying to set 
>>>>>>>> the cpu clocks.
>>>>>>>> We can just remove it and leave setting a faster cpu rate to linux.
>>>>>>>>
>>>>>>>> I also noticed that my sdram size series only detect the first 
>>>>>>>> two channels of memory,
>>>>>>>> will respin a v2 of that series to add detection of all 4 
>>>>>>>> channels of memory.
>>>>>>>>
>>>>>>>> [2] 
>>>>>>>> https://github.com/Kwiboo/u-boot-rockchip/commit/7e4fee945e725c0ec9ef3905c41ce367e77833b3
>>>>>>>>
>>>>>>> Okay. We need to find a way to handle the clock value 400Khz
>>>>>>> generically via the CLK framework or eMMC can be worth checking 
>>>>>>> as it
>>>>>>> doesn't involve SCMI and have a working patch set before MW. I did
>>>>>>> that and was able to detect eMMC in U-Boot proper but got some 
>>>>>>> issues
>>>>>>> while booting from eMMC [3]
>>>>>>
>>>>>> I have an updated branch at [4] that should support booting from 
>>>>>> sdmmc and sdhci.
>>>>>> Tested booting from both sd-card and emmc on my Radxa ROCK 5 Model B.
>>>>>>
>>>>>> This branch is based on u-boot/master 
>>>>>> f147aa80f52989c7455022ca1ab959e8545feccc
>>>>>> and I have included some rk3568 patches and your rk3588 rfc series.
>>>>>> I added a few fixup on top of that and a few additional patches, 
>>>>>> please see commit message
>>>>>> for a very brief note on why the change was needed.
>>>>>> Feel free to squash fixups and pick commits up to and possible 
>>>>>> including
>>>>>> "board: rockchip: Sync evb-rk3568_defconfig with 
>>>>>> evb-rk3588_defconfig"
>>>>>> for a v2 of this series.
>>>>>>
>>>>>> The remaining sdhci patches needs a little bit more work,
>>>>>> I can send a separate series with emmc patches once they are fully 
>>>>>> ready.
>>>>>>
>>>>>> The last commit adds a hack to mkimage to keep the data for atf-3 
>>>>>> embedded in the FIT.
>>>>>> This ensures we do not run into problems trying to use dma from 
>>>>>> emmc into secure pmu sram.
>>>>>> I think this is a more appropriate way to work around this issue, 
>>>>>> instead of patching
>>>>>> u-boot spl_fit or sdhci core to use bounce buffers in a very 
>>>>>> special case.
>>>>>>
>>>>>> [4] 
>>>>>> https://github.com/Kwiboo/u-boot-rockchip/commits/rk3588-master-v2
>>>>>>
>>>>> Hello Jagan, Jonas,
>>>>>
>>>>> I wanted to chip into this discussion, to ask whether you did anything
>>>>> more on the SD clock matter ?
>>>>
>>>> I have been busy this past week but have now had time to take a new 
>>>> look
>>>> at the sdmmc issue, along with completing some other fixes.
>>>>
>>>>>
>>>>> I am currently using your workaround that creates dummy clocks in 
>>>>> the DT
>>>>> and then the SD node just uses those, and it works, in the SPL, if and
>>>>> only if the bootrom also loads the SPL from SD.
>>>>> I tried to have the SPL inside the SPI flash, or load it to DDR using
>>>>> the rockusb protocol, and then, initializing the SD-Card from SPL 
>>>>> fails
>>>>> utterly, namely, it cannot communicate with the card.
>>>>> I did some changes to have the pinctrl working at SPL level, which
>>>>> appears to be fine, but I would like to see what can be done about the
>>>>> clocks.
>>>>> Having the cru and all the required drivers at SPL level is the way to
>>>>> go ? The SPL should run before SCMI is required so it should be 
>>>>> able to
>>>>> change all the clocks at the clock controller level ?
>>>>
>>>> I fully agree that we should have some sort of scmi clk driver so that
>>>> we can control the sdmmc clk in both SPL and U-Boot proper.
>>>>
>>>> As you have noticed the current workaround only works because bootrom
>>>> leave the clocks in a working state after it has loaded TPL/SPL from
>>>> the sdmmc device. When TPL/SPL is loaded from any other source it is
>>>> not be possible to read from the sdmmc device in u-boot.
>>>>
>>>> After having played around with the scmi agent driver and being 
>>>> inspired
>>>> by the dummy scmi clk driver in vendor u-boot I have managed to create
>>>> something that could work. See top three commits from [5] for a working
>>>> proof-of-concept.
>>>>
>>>> What I did was to enable the scmi agent driver for use in u-boot proper
>>>> and keeping it disabled in SPL build. Then added a new scmi clk driver
>>>> that is enabled in SPL build, the rk3588 clk driver hooks the new clk
>>>> driver to the scmi_clk ofnode. I only implemented the get/set rate ops
>>>> for the two sdmmc clocks. With this both SPL and U-Boot proper should
>>>> be able to configure the sdmmc clocks.
>>>>
>>>> The initial fixes commits in that branch should hit the list soon.
>>>> Will send the sdmmc related commits once I have had some time to do
>>>> more testing.
>>>>
>>>> [5] https://github.com/Kwiboo/u-boot-rockchip/commits/rk35xx-fixes
>>>>
>>> Hi Jonas,
>>>
>>> I managed to get it working in SPL as well, with a similar code inspired
>>> from the vendor tree.
>>> About the solution, I am not sure whether it should be called a scmi
>>> clock, and whether it should be part of clk_rk3588
>>> Honestly by looking at the DT node, I find the description of the clock
>>> tree wrong to begin with.
>>> The SD should not have the 2 clocks tied to the scmi agent, because this
>>> is true only if it's running in normal world with a secure world agent
>>> that will act as a middle man. So if we are running at a higher EL, as
>>> in the SPL case, the clocks should be tied directly to the CRU.
>>> So the node itself is described bad IMO.
>>>
>>> Anyway, if the node is set in stone from Linux, there isn;t much we 
>>> can do.
>>> I am also thinking whether we should have the SCMI enabled in SPL as
>>> well, but the agent/clock should just access the CRU directly, meaning
>>> to do something at the agent level :
>>> #ifdef CONFIG_SPL_BUILD
>>> -> access CRU
>>> #else
>>> -> send SCMI message
>>> #endif
>>> Does this make more sense ?
>>>
>>> Or have some kind of wrapper driver that would act as a dispatcher
>>> depending on the SPL/proper build ?
>>
>> Looks like the sdmmc clock is controlled by the securecru registers and I
>> expect that this can only be configured in secure world, not sure how 
>> this
>> could have been modeled differently.
> 
> Since it can work with a clock given by either scmi or securecru, I 
> would expect all clocks to be in the list, and the driver could start 
> the needed clock as per the EL level it's running into :)
> 
> e.g.
> clocks = <&scru CCLK_SD>, <&scru HCLK_SD>, <&scmi CCLK_SD>, <&scmi 
> HCLK_SD> , <&cru basic other clocks..>
> 
> Just my vision of how it could be modeled
> 
>>
>> With the approach that I took I think the normal clk framework will 
>> behave
>> as such dispatcher and clock gets tied to cru in SPL.
>>
>> #ifdef CONFIG_SPL_BUILD
>> -> bind protocol@14 to rk3588 securecru driver -> read/write securecru 
>> regs
>> #else
>> -> bind protocol@14 to scmi clk driver -> send SCMI message
>> #endif
> 
> This looks right, just that we would have to bring a lot of bloat to SPL 
> , firmware subsystem and things we may not want.
>>
>> Naming and placement of the SPL securecru driver could be improved.
>> Not sure if any other soc beside rk3588 will need this at this moment.
> 
> securecru sounds better for me, I don't think we can have a scmi clock 
> fake driver like in vendor uboot, it has nothing 'scmi' about it.
> 
>>
>>>
>>>
>>> Meanwhile, I will test your patches to see how they work on my setup, I
>>> also have some things in progress including pinctrl in SPL for the 
>>> rock5b.
>>> Thanks for your detailed description,
>>
>> Thanks for the hint at pinctrl, I made some updates to [5] and could 
>> verify
>> that sdmmc works when booting from emmc thanks to your pinctrl commit.
>> Also enabled SPI NOR and eMMC for rock5b in [6], and could do a sf probe
>> and build a spi firmware image.
>>
>> There is an issue with non-DMA access in sdhci that requires the HACK 
>> commit
>> for proper loading of atf, was hoping to disable SDMA in SPL like what 
>> is done
>> using fifo-mode for sdmmc. But only first sector is read successfully 
>> then it
>> fails, see below. Will take a closer look before I post eMMC series.
> 
> In your patches, I see this :
> https://github.com/Kwiboo/u-boot-rockchip/blob/rk35xx-fixes/arch/arm/dts/rk3588s-u-boot.dtsi#L61
> Perhaps the fifo-mode is intended for the sdhci(emmc) and not for 
> sdmmc(sd-card) ? My understanding was that sdhci cannot do DMA to SRAM, 
> but the sdmmc can ?
> one possible workaround is to have DMA to DRAM and then relocate it to 
> SRAM using the CPU. Having DMA disabled for the whole IP may have 
> downsides, but this is U-boot, we don't expect to have anything else to 
> do with the CPU while the DMA master works its magic, and the CPU should 
> be faster.
> 
> I tested your patches together with the series I sent now
> ( https://marc.info/?l=u-boot&m=167871206530072&w=2 ) and it appears to 
> boot correctly from SD-Card
> Once you send them to the ML I can retest them.
> Thanks !

Sorry I just realized I had something extra in my tree. It does not work 
without the dw_mmc reset after data error implementation.
I can say that it was the same for me when I had it done on my own.
I still have not figured it out why the first attempted SD mode does not 
work, and after reset, going to legacy mode, SD transfers work.
I will send a patch with the dw reset code

Eugen
> 
>>
>>    Trying to boot from MMC2
>>    1
>>       - 0 'mmc@fe2c0000'
>>       - 1 'mmc@fe2e0000'
>>       - found
>>    blk_find_device: uclass_id=67, devnum=1: mmc@fe2c0000.blk, 67, 0
>>    blk_find_device: uclass_id=67, devnum=1: mmc@fe2e0000.blk, 67, 1
>>    clock is disabled (0Hz)
>>    clock is enabled (400000Hz)
>>    clk_set_rate(clk=5000d8, rate=400000)
>>    size=200, ptr=3b8, limit=100000: 5001b8
>>    clock is enabled (25000000Hz)
>>    clk_set_rate(clk=5000d8, rate=25000000)
>>    clk_set_rate(clk=5000d8, rate=25000000)
>>    clock is enabled (52000000Hz)
>>    clk_set_rate(clk=5000d8, rate=52000000)
>>    spl: mmc boot mode: raw
>>    blk_find_device: uclass_id=67, devnum=1: mmc@fe2c0000.blk, 67, 0
>>    blk_find_device: uclass_id=67, devnum=1: mmc@fe2e0000.blk, 67, 1
>>    hdr read sector 4000, count=1
>>    Found FIT
>>    size=a00, ptr=dc0, limit=100000: aligned to 5003c0
>>    blk_find_device: uclass_id=67, devnum=1: mmc@fe2c0000.blk, 67, 0
>>    blk_find_device: uclass_id=67, devnum=1: mmc@fe2e0000.blk, 67, 1
>>    fit read sector 4000, sectors=5, dst=5003c0, count=0, size=0xa00
>>    mmc_load_image_raw_sector: mmc block read error
>>    spl: mmc boot mode: fs
>>    Trying to boot from MMC1
>>
>> [6] https://github.com/Kwiboo/u-boot-rockchip/commits/rk3588-emmc
>>
>> Regards,
>> Jonas
>>
>>
>>> Eugen
>>>
>>>>
>>>> Regards,
>>>> Jonas
>>>>
>>>>>
>>>>> Thanks,
>>>>> Eugen
>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Jonas
>>>>>>
>>>>>>>
>>>>>>> [3] 
>>>>>>> https://github.com/edgeble/u-boot/commit/1389822228ac3dfe8d3cd3513db6a32a5c898b7f
>>>>>>>
>>>>>>> Jagan.
>>>>>>
>>>>>
>>>>
>>>
>>
>