[U-Boot] [PATCH v1 1/4] arm: socfpga: imply SPL config instead of select

[Simon Goldschmidt]

Am 14.01.2019 um 21:01 schrieb Marek Vasut:
> On 1/14/19 8:43 PM, Simon Goldschmidt wrote:
>> Am 14.01.2019 um 20:33 schrieb Marek Vasut:
>>> On 1/14/19 7:58 PM, Simon Goldschmidt wrote:
>>>> Am 14.01.2019 um 19:31 schrieb Marek Vasut:
>>>>> On 1/14/19 5:05 PM, Simon Goldschmidt wrote:
>>>>>> Hi Dinh,
>>>>>
>>>>> Hi,
>>>>>
>>>>>> Am 14.01.2019 um 16:58 schrieb Dinh Nguyen:
>>>>>>> Hi Simon,
>>>>>>>
>>>>>>> On 1/14/19 9:50 AM, Simon Goldschmidt wrote:
>>>>>>>> Am 11.01.2019 um 23:02 schrieb Marek Vasut:
>>>>>>>>> On 1/11/19 9:39 PM, Simon Goldschmidt wrote:
>>>>>>>>>> Am 07.01.2019 um 23:53 schrieb Marek Vasut:
>>>>>>>>>>> On 1/7/19 10:14 PM, Simon Goldschmidt wrote:
>>>>>>>>>>>> In order to build a smaller SPL, let's imply SPL_DM_RESET and
>>>>>>>>>>>> SPL_WATCHDOG_SUPPORT instead of selecting them, so they can be
>>>>>>>>>>>> disabled
>>>>>>>>>>>> via defconfig.
>>>>>>>>>>>>
>>>>>>>>>>>> This also seems to be required to use OF_PLATDATA, as the reset
>>>>>>>>>>>> drivers
>>>>>>>>>>>> don't seem to work with it.
>>>>>>>>>>>
>>>>>>>>>>> How do you un-reset IP blocks if you disable the reset
>>>>>>>>>>> controller ?
>>>>>>>>>>
>>>>>>>>>> I found that out just now: there's the function
>>>>>>>>>> 'reset_deassert_peripherals_handoff()' in spl_gen5.c that should
>>>>>>>>>> "De-assert reset for peripherals and bridges based on handoff".
>>>>>>>>>> However,
>>>>>>>>>> at least for Gen5, it just writes a 0 to rstmgr->permodrst. By
>>>>>>>>>> doing
>>>>>>>>>> that, it enables *ALL* peripherals on the SoC (except for some DMA
>>>>>>>>>> channels that aren't really used) :-)
>>>>>>>>>>
>>>>>>>>>> I guess that needs some cleaning up as well ;-)
>>>>>>>>>
>>>>>>>>> Yes
>>>>>>>>>
>>>>>>>>>> I think the proper thing to do here would be to remove this
>>>>>>>>>> function and
>>>>>>>>>> convert all drivers to provide appropriate 'resets' properties in
>>>>>>>>>> the
>>>>>>>>>> dts?
>>>>>>>>>
>>>>>>>>> Indeed
>>>>>>>>
>>>>>>>> So I just did that and it works nice for SPL and U-Boot: By adding
>>>>>>>> some
>>>>>>>> "resets" properties the the main dtsi and adding reset bulk code to
>>>>>>>> the
>>>>>>>> cadence_qspi, denali_dt nand and drivers, I can nearly remove the
>>>>>>>> reset
>>>>>>>> code from arch/mach_socfpga.
>>>>>>>>
>>>>>>>> The problem would be that now Linux cannot use peripherals that
>>>>>>>> aren't
>>>>>>>> enabled by U-Boot because it relies on them being enabled. How are
>>>>>>>> such
>>>>>>>> dependencies solved? Because even if I would add reset support in
>>>>>>>> the
>>>>>>>> corresponding Linux drivers, we probably could not bootolder Kernels
>>>>>>>> (e.g. the Debian 9 kernel - v4.9.x) with a new U-Boot...
>>>>>>>>
>>>>>>>
>>>>>>> I added an early reset driver for SoCFPGA that should take care of
>>>>>>> this.
>>>>>>> The patch is in v5.0-rc2[1].
>>>>>>
>>>>>> OK, it's good to know that this work is already done, I haven't
>>>>>> monitored this close enough.
>>>>>
>>>>> We had the same problem with A10, indeed.
>>>>>
>>>>>> But am I correct that my above problem remains even in v5.0 as not all
>>>>>> peripherals in socfpga.dtsi have a "resets" property set (e.g. mmc and
>>>>>> qspi) and would thuse not be taken out of reset by Linux?
>>>>>>
>>>>>> Plus: should U-Boot work with older Linux kernels? Because if so, we
>>>>>> need fallback code in U-Boot to unreset peripherals when running
>>>>>> with an
>>>>>> older kernel...
>>>>>
>>>>> Yes, it'd break old broken kernels . The real question is, do we care ?
>>>>
>>>> Ok, so that at leat shows me I'm going into the right direction :-)
>>>>
>>>> There are some problems though:
>>>> - I do care (we're running 4.9 currently) *g*
>>>> - people running an RT kernel will care for a while (until the next
>>>> stable RT after fixing this will be released)
>>>> - we would currently be breaking *all* kernels, since no kernel should
>>>> yet be able to deassert reset for mmc and qspi (unless this is already
>>>> done by U-Boot)...
>>>>
>>>> So would it be OK to add a Kconfig option to U-Boot to keep the current
>>>> behaviour (for old broken kernels like you said) until that code is
>>>> spread widely enough? Or is that a no-go?
>>>
>>> Would be nice to be able to tweak the reset driver behavior at runtime,
>>> to unreset things before booting the kernel if the user desires so.
>>
>> Instead of tweaking the reset driver, we could just add a command that
>> does that 'rstmgr->permodrst = 0;' thing my patch would remove.
> 
> I don't want a new custom command.
> 
>> Since noone has complained so far, I think writing 0 should be OK here.
>> I don't think it would make too much sense to use the reset handoff
>> defines from Quartus output for such a command. I think the way Quartus
>> does this is strange anyway...
>>
>> The question is if defconfigs should be able to use this to
>> automatically build a U-Boot config for older kernels. If so, we'd still
>> need a Kconfig option?
> 
> I'd much rather have this runtime configurable.

Then I'm afraid I don't know what you mean by "runtime configurable". 
What should be the configuration source that is evaluated at runtime?

> 
>> Thinking further about cleanup: I guess the clock driver is not that
>> hard to implement, either. The only thing that's driving me mad is
>> pinmux. Is there any chance to get more info from Intel to write this
>> properly so we can get rid of that iocsr scanchain defines?
> 
> Clock driver should be easy, yes. Pinmux, I don't know, maybe project
> chibi has some information (the cyclone I documentation project).

Interesing, I didn't know that project. The only thing I found is a repo 
on github. But it seems like that one only contains FPGA-related stuff, 
nothing about the HPS side...

Regards,
Simon