On Mon, Apr 22, 2019 at 9:24 PM Marek Vasut <ma...@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.goldschm...@gmail.com> Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot