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

[Simon Goldschmidt]

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.

And the duplication of names where U-Boot caches the handoff registers 
doesn't really help to make it easy to follow...

Regards,
Simon

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