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

[Simon Goldschmidt]

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.

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?

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?

Thanks,
Simon