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

[Xu Yilun <yilun.xu@linux.intel.com>]

> > 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.

> 
> 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?

> 
> 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.

> 
> 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.