[U-Boot] [PATCH 4/4] ARM: socfpga: Add support for selecting bridges in bridge command

[Simon Goldschmidt]

On Mon, Apr 22, 2019 at 9:24 PM Marek Vasut <marex at denx.de> wrote:
>
> On 4/22/19 9:18 PM, Simon Goldschmidt wrote:
> >
> >
> > On 22.04.19 20:41, Marek Vasut wrote:
> >> On 4/22/19 8:22 PM, Simon Goldschmidt wrote:
> >>> Am 22.04.2019 um 20:01 schrieb Marek Vasut:
> >>>> On 4/19/19 10:00 PM, Simon Goldschmidt wrote:
> >>>>>
> >>>>>
> >>>>> On 17.04.19 22:15, Marek Vasut wrote:
> >>>>>> Add optional "mask" argument to the SoCFPGA bridge command, to select
> >>>>>> which bridges should be enabled/disabled. This allows the user to
> >>>>>> avoid
> >>>>>> enabling bridges which are not connected into the FPGA fabric.
> >>>>>> Default
> >>>>>> behavior is to enable/disable all bridges.
> >>>>>
> >>>>> So does this change the command? Seems like leaving away the new
> >>>>> 'mask'
> >>>>> argument would now lead to enabling all bridges by overwriting
> >>>>> whatever
> >>>>> the handoff values were before?
> >>>>
> >>>> That's how it behaved before though -- all the bridges were enabled.
> >>>> Now it's possible to explicitly select which bridges to enable/disable.
> >>>
> >>> As I read the code, before it wrote iswgrp_handoff[x] to the registers.
> >>
> >> Nope, before it was the SPL that wrote the iswgrp_handoff registers
> >> (iswgrp_handoff[0] to 0x0 and iswgrp_handoff[1] to 0x7)
> >>
> >>> The question is what is iswgrp_handoff[x].
> >>
> >> Just regular scratch registers with special name. The [0] is used to
> >> indicate to the software how the brg_mod_reset register is supposed to
> >> be configured. The [1] is used to indicate how the l3remap register is
> >> supposed to be configured.
> >>
> >> The SPL currently sets these registers as -- all bridges released out of
> >> reset ; all bridges mapped into the L3 space. I believe this patch does
> >> not change that behavior.
> >>
> >>> It's not the bridges status
> >>> from Quartus (as the "handoff" suffix might suggest). Instead (if I
> >>> remember correctly), it's either "all bridges" or "no bridges",
> >>> depending on the FPGA configuration status at SPL runtime.
> >>>
> >>> In this case, we're probably better off with leaving it to the command
> >>> line scripts to say which bridges shall be enabled...
> >>
> >> This patch only changes things in that it updates the handoff registers
> >> when the user selects a new mask, so that any software which runs after
> >> U-Boot would be aware of which bridges U-Boot configured.
> >>
> >> But maybe I'm missing something obvious, this stuff is so convoluted
> >> that I'd really appreciate if you could review thoroughly if there's
> >> something that doesn't seem right.
> >
> > Hmm, if you're not 100% sure yourself, let me take the time to check
> > again. What made me stumble accross this is that "bridge enable" did not
> > work when no FPGA had been configured during SPL.
>
> I'm reasonably sure it's OK, but another pair of eyeballs and all that ...
>
> If the FPGA isn't configured, you shouldn't even run bridge enable, the
> result of that is undefined.

Well, what I meant is FPGA is configured later. Not in SPL but before the
bridge command is run. But nevermind, I got it now...

>
> > And the duplication of names where U-Boot caches the handoff registers
> > doesn't really help to make it easy to follow...
>
> Look at arch/arm/mach-socfpga/misc_gen5.c do_bridge_reset() , that
> should clarify how the handoff registers are used. Keep in mind, they
> are only scratch registers , they have no real impact on the HW (except
> some are also read by BootROM during warm reset).

I know. What I was missing is that iswgrp_handoff[3] is never written and
keeps its reset value of 0, so yes, your patch shouldn't change the current
behaviour. So:

Reviewed-by: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>

Regards,
Simon