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

[Heiko Schocher <hs@nabladev.com>]

Hello Xu Yilun,

On 08.09.26 20:59, Xu Yilun wrote:
>> I would change v5 commit message to:
>> """
>>      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 on every board,
>>      whatever polarity the device tree states.
> 
> Yeah, that's clear now.

Fine, sorry for not that clear from the beginning.

> 
>>
>>      Keeping them asserted works as long as a single FPGA owns the
>>      SelectMAP port. That FPGA is the only device on the bus, so it may
>>      stay selected, and the driver never reads from it, so the port may
>>      stay in write mode.
> 
> I think keeping them asserted is a bad idea even for single FPGA, isn't
> it?

You are right, both pins belong to the data transfer, not to the driver`s
lifetime. That a board with one FPGA does not visibly break is not the same
as simply behave correct.

I drop that paragraph, as my patch corrects this already.

>>      It stops working as soon as two FPGAs share one SelectMAP data bus,
>>      which is the case CSI_B exists for. Both devices are selected all
>>      the time, so every byte written for one of them is clocked into the
>>      other as well, and neither can be configured on its own.
>>
>>      CSI_B (Chip Select) selects the target FPGA. Assert it before the
>>      configuration data is transferred and deassert it afterwards, so that
>>      only the intended device sees the bus cycles.
> 
> 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.

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.

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?

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.

Thanks for your patience.

bye,
Heiko

> Thanks,
> Yilun
> 
>>
>>      RDWR_B (Read/Write) selects the transfer direction on the SelectMAP
>>      interface. Assert it for the write cycles that carry the bitstream and
>>      deassert it afterwards. UG570 wants RDWR_B settled before CSI_B is
>>      asserted, a change while the device is selected aborts the
>>      configuration on the next CCLK.
>>
>>      Both are requested with GPIOD_OUT_LOW now, the logical 0 that leaves
>>      them deasserted, and their descriptors are kept in the driver private
>>      data.
>>
>>      A board with one FPGA keeps working. SelectMAP allows the bitstream to
>>      be loaded non-continuously, with CSI_B deasserted between the data
>>      transfers, and a deselected device ignores the bus.
>> """

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