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

Hello Xu Yilun,

On 09.09.26 08:43, Xu Yilun wrote:

quoted

quoted

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.

quoted

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

quoted

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.

quoted

I am fine with both ...

quoted

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