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

[Marek Vasut]

On 4/26/19 8:36 AM, Simon Goldschmidt wrote:
> 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>

All right, in that case, let me enqueue the current bulk for 2019.07.

-- 
Best regards,
Marek Vasut