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

[Simon Goldschmidt]

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

>
> >>> 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?

Regards,
Simon