Re: [PATCH v4] driver: fpga: xilinx-selectmap: add csi and rdwr support

[Heiko Schocher <hs@nabladev.com>]

Hello Xu Yilun,

On 09.09.26 08:43, Xu Yilun wrote:
>>> What if we re-program the 2 FPGAs at the same time? Is there still
>>> chance the 2 CS lines are all asserted? Can they be correctly
>>> re-programmed in this case?
>>
>> In the FPGA layer, yes. Each FPGA is its own fpga_manager with its own
>> CSI_B, and the only lock the core offers is mgr->ref_mutex, which
>> fpga_mgr_lock() takes per manager. Nothing there serializes two managers
>> that share one SelectMAP port, so both CS lines can be asserted at the
>> same time and then both devices take both bitstreams.
>>
>> But one layer higher, the only in-tree trigger for a SelectMAP device is
>> of_fpga_region_notify() calling fpga_region_program_fpga(), and
>> of_overlay_fdt_apply() holds of_overlay_phandle_mutex from beginning to
>> end, so two overlay applications cannot overlap, if I see this correct.
>>
>> I would rather keep this out of this patch.
> 
> Yes, that's the way to go.

Ok, I send the v5 soon.

>> If you want it handled, I can prepare a follow up patch, and it seems to
>> me this can be done with a simple mutex in this driver around the write
>> transfer in xilinx_selectmap_write(), with one lock for all ports.
> 
> "one lock for all ports" is still illogical to me. The FPGAs on
> different ports won't interfere each other, is it?

They do not, you are right. I first thought to solve it "easy" with
a mutex around the write... but nothing tells the driver which managers
share a port.

Two managers on one port and two managers on two ports look exactly the
same from inside the driver. And so we may we lock the write unnecessary...

I have to think about...

>>
>> The port does not have to be held for a whole bitstream, as documentation
>> says, that SelectMAP takes the configuration data non-continuously, with
>> CSI_B deasserted in between, so serializing single transfers already lets
>> two devices be programmed at the same time with each one seeing only its
>> own data.
>>
>> Should I send such a patch? And if yes, as a follow up to a v5 version of
>> this patch?
>>
>> Or add both patches into a v5 series?
> 
> No, fix the existing problem. 2 FPGAs are another topic.

Fine.

> 
>>
>> I am fine with both ...
>>
>>> I mean I think this patch does fix the problem of "always assertion",
>>> which is good to me. But the changelog seems stop me, it talks all about
>>> 2 FPGAs sharing the same bus, which seems more complex than just
>>> manipulating the CS.
>>
>> Agreed. The two FPGAs are how I ran into this, not what the patch is
>> about.
>>
>> So next proposal for the commit message is:
>> """
>> fpga: xilinx-selectmap: control CSI_B and RDWR_B during configuration
>>
>> The driver requests the optional CSI_B and RDWR_B GPIOs with
>> GPIOD_OUT_HIGH and never touches them again. That flag carries a
>> logical value, so both signals end up asserted from probe on, whatever
>> polarity the device tree states, and they stay asserted for the
>> lifetime of the device.
>>
>> Neither signal belongs to the driver's lifetime. CSI_B (Chip Select)
>> selects the device on the SelectMAP port, RDWR_B (Read/Write) selects
>> the transfer direction, so both belong to the data transfer. A device
>> that is never deselected never lets go of the port, and a port pinned
>> to write mode cannot be read back.
>>
>> Keep the two descriptors in the driver private data, request them
>> deasserted, and assert them only around the configuration data
>> transfer. RDWR_B is asserted first as UG570, note 4 of figure "Continuous
>> x8 SelectMAP Data Loading", warns that changing it while the device is
>> selected causes an ABORT on the next CCLK.
>> """
>>
>> If fine for you I can send v5, with no code changes, just some
>> comment changes as discussed.
> 
> Good to me.
> 

Thanks!

bye,
Heiko
-- 
Nabla Software Engineering
HRB 40522 Augsburg
Phone: +49 821 45592596
E-Mail: office@nabladev.com
Geschäftsführer : Stefano Babic