On 4/26/19 8:36 AM, Simon Goldschmidt wrote: > On Mon, Apr 22, 2019 at 9:24 PM Marek Vasut <[email protected]> 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 <[email protected]>
All right, in that case, let me enqueue the current bulk for 2019.07. -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list [email protected] https://lists.denx.de/listinfo/u-boot
