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

Mon Jan 14 20:59:48 UTC 2019

Am 14.01.2019 um 21:49 schrieb Marek Vasut:
> On 1/14/19 9:30 PM, Simon Goldschmidt wrote:
>> On Mon, Jan 14, 2019 at 9:23 PM Marek Vasut <marex at denx.de> wrote:
>>>
>>> On 1/14/19 9:12 PM, Simon Goldschmidt wrote:
>>>> 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?
>>>
>>> I wonder ... maybe an environment variable with a U_BOOT_ENV_CALLBACK()
>>> hook ?
>>
>> But when doing it like that, I'd still have to modify the U-Boot
>> sources to build a version of U-Boot that could successfully boot a
>> 4.9 kernel.
>> I wanted to be able to do that without changing the sources, that's
>> why I would have added a Kconfig option...
> 
> If you have an environment variable which allows you to indicate that
> your kernel is broken/old, you can set it and boot.

Fair enough. I'll try it like that.

> 
>>>>>> 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...
>>>
>>> My understanding is that the IOCSR ring is on the FPGA side and it's
>>> exactly the same as pinmux configuration for the FPGA IO, but I might be
>>> wrong.
>>
>> Hmm, I really don't know. Has anyone ever tried asking Intel for this?
>> Is it a known fact that they won't give away such specs?
> 
> Dinh is on CC :)

I know ;-)

Regards,
Simon