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

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

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

-- 
Best regards,
Marek Vasut