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

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

[Heiko Schocher]

The current driver requests the optional CSI_B and RDWR_B GPIOs but
only configures their initial output state and never changes them
afterwards. As a result, CSI_B and RDWR_B remain inactive or active
throughout the configuration process.

This may work on systems with a single FPGA where these signals do not
need to be controlled by software, or where the signals are configured
to their active state. But this does not not support systems with
multiple FPGAs sharing a SelectMAP interface.

On systems with multiple FPGAs sharing the same SelectMAP data bus,
the driver must deassert this signals in probe, and actively control
them during configuration.

CSI_B (Chip Select, active low) selects the target FPGA. It is asserted
before configuration data is transferred and deasserted afterwards so
that only the intended device responds to bus transactions.

RDWR_B (Read/Write, active low) controls the transfer direction on the
SelectMAP interface. A low level selects write cycles, while a high
level selects read cycles. During FPGA configuration the driver drives
RDWR_B low before transferring the bitstream and restores it to its
reading (high state) afterwards.

With that info from the datasheet the driver is now changed to:

- deassert the CSI_B and RDWR_B pin on probe and store the
  optional GPIO descriptors in private driver data.

- toggle both signals around the configuration data transfer

This allows multiple FPGAs to safely share a single SelectMAP interface.

Signed-off-by: Heiko Schocher <hs@nabladev.com>
---

Changes in v4:
- add comments from Xu Yulin
  replace wrong gpiod_set_raw_value() with gpiod_set_value()
  deassert CSI_B and RDWR_B in probe as in patch version 2
  rework commit message (correct the description what current
  driver do on probe), why this is not a problem with one
  FPGA, and why it needs a change if you have N FPGAs
  sharing the same selectmap Interface (clk and data pins).

Changes in v3:
- use 0 (deasserted state) and 1 (asserted state) in gpio_set_value()
  as commented from Micahl
- rewrite commit message as requested from Xu Yilun
  - describe what rdwr_b and csi_b do, and why this change is needed
    for more than one FPGA.
- add comment before asserting the signals, why they are asserted
  in this order.

Changes in v2:
- add comments from Michal
  - skip check if gpio descriptor variables csi_b/rdwr_b are valid,
    as validate_desc() checks this in gpiod_set_value() call.
  - initialize the gpio variables csi_b/rdwr_b immediately with
    the return value from devm_gpiod_get_optional(), so we can
    drop local gpio variable at all

 drivers/fpga/xilinx-selectmap.c | 36 ++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/drivers/fpga/xilinx-selectmap.c b/drivers/fpga/xilinx-selectmap.c
index d0cbb5fdfe3a..e9b1d15ca054 100644
--- a/drivers/fpga/xilinx-selectmap.c
+++ b/drivers/fpga/xilinx-selectmap.c
@@ -19,6 +19,8 @@
 struct xilinx_selectmap_conf {
 	struct xilinx_fpga_core core;
 	void __iomem *base;
+	struct gpio_desc *csi_b;
+	struct gpio_desc *rdwr_b;
 };
 
 #define to_xilinx_selectmap_conf(obj) \
@@ -30,16 +32,30 @@ static int xilinx_selectmap_write(struct xilinx_fpga_core *core,
 	struct xilinx_selectmap_conf *conf = to_xilinx_selectmap_conf(core);
 	size_t i;
 
+	/*
+	 * Assert CSI_B and select write mode.
+	 *
+	 * UG570 states in note 4 in Figure "Continuous x8 SelectMAP Data
+	 * Loading", RDWR_B should be asserted before CSI_B to avoid
+	 * causing an ABORT on the next CCLK.
+	 *
+	 * To be sure, set first RDWR_B pin before activate CSI_B
+	 */
+	gpiod_set_value(conf->rdwr_b, 1);
+	gpiod_set_value(conf->csi_b, 1);
+
 	for (i = 0; i < count; ++i)
 		writeb(buf[i], conf->base);
 
+	gpiod_set_value(conf->csi_b, 0);
+	gpiod_set_value(conf->rdwr_b, 0);
+
 	return 0;
 }
 
 static int xilinx_selectmap_probe(struct platform_device *pdev)
 {
 	struct xilinx_selectmap_conf *conf;
-	struct gpio_desc *gpio;
 	void __iomem *base;
 
 	conf = devm_kzalloc(&pdev->dev, sizeof(*conf), GFP_KERNEL);
@@ -55,16 +71,18 @@ static int xilinx_selectmap_probe(struct platform_device *pdev)
 				     "ioremap error\n");
 	conf->base = base;
 
-	/* CSI_B is active low */
-	gpio = devm_gpiod_get_optional(&pdev->dev, "csi", GPIOD_OUT_HIGH);
-	if (IS_ERR(gpio))
-		return dev_err_probe(&pdev->dev, PTR_ERR(gpio),
+	/* CSI_B is active low, deassert signal */
+	conf->csi_b = devm_gpiod_get_optional(&pdev->dev, "csi",
+					      GPIOD_OUT_LOW);
+	if (IS_ERR(conf->csi_b))
+		return dev_err_probe(&pdev->dev, PTR_ERR(conf->csi_b),
 				     "Failed to get CSI_B gpio\n");
 
-	/* RDWR_B is active low */
-	gpio = devm_gpiod_get_optional(&pdev->dev, "rdwr", GPIOD_OUT_HIGH);
-	if (IS_ERR(gpio))
-		return dev_err_probe(&pdev->dev, PTR_ERR(gpio),
+	/* RDWR_B is active low, deassert signal */
+	conf->rdwr_b = devm_gpiod_get_optional(&pdev->dev, "rdwr",
+					       GPIOD_OUT_LOW);
+	if (IS_ERR(conf->rdwr_b))
+		return dev_err_probe(&pdev->dev, PTR_ERR(conf->rdwr_b),
 				     "Failed to get RDWR_B gpio\n");
 
 	return xilinx_core_probe(&conf->core);
---
base-commit: db2ddb87143519e20a95aa36c60b36107b736a58

-- 
2.55.0

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

[Heiko Schocher]

Hi!

On 10.08.26 08:20, Heiko Schocher wrote:
> The current driver requests the optional CSI_B and RDWR_B GPIOs but
> only configures their initial output state and never changes them
> afterwards. As a result, CSI_B and RDWR_B remain inactive or active
> throughout the configuration process.
> 
> This may work on systems with a single FPGA where these signals do not
> need to be controlled by software, or where the signals are configured
> to their active state. But this does not not support systems with
> multiple FPGAs sharing a SelectMAP interface.
> 
> On systems with multiple FPGAs sharing the same SelectMAP data bus,
> the driver must deassert this signals in probe, and actively control
> them during configuration.
> 
> CSI_B (Chip Select, active low) selects the target FPGA. It is asserted
> before configuration data is transferred and deasserted afterwards so
> that only the intended device responds to bus transactions.
> 
> RDWR_B (Read/Write, active low) controls the transfer direction on the
> SelectMAP interface. A low level selects write cycles, while a high
> level selects read cycles. During FPGA configuration the driver drives
> RDWR_B low before transferring the bitstream and restores it to its
> reading (high state) afterwards.
> 
> With that info from the datasheet the driver is now changed to:
> 
> - deassert the CSI_B and RDWR_B pin on probe and store the
>    optional GPIO descriptors in private driver data.
> 
> - toggle both signals around the configuration data transfer
> 
> This allows multiple FPGAs to safely share a single SelectMAP interface.
> 
> Signed-off-by: Heiko Schocher <hs@nabladev.com>
> ---
> 
> Changes in v4:
> - add comments from Xu Yulin
>    replace wrong gpiod_set_raw_value() with gpiod_set_value()
>    deassert CSI_B and RDWR_B in probe as in patch version 2
>    rework commit message (correct the description what current
>    driver do on probe), why this is not a problem with one
>    FPGA, and why it needs a change if you have N FPGAs
>    sharing the same selectmap Interface (clk and data pins).
> 
> Changes in v3:
> - use 0 (deasserted state) and 1 (asserted state) in gpio_set_value()
>    as commented from Micahl
> - rewrite commit message as requested from Xu Yilun
>    - describe what rdwr_b and csi_b do, and why this change is needed
>      for more than one FPGA.
> - add comment before asserting the signals, why they are asserted
>    in this order.
> 
> Changes in v2:
> - add comments from Michal
>    - skip check if gpio descriptor variables csi_b/rdwr_b are valid,
>      as validate_desc() checks this in gpiod_set_value() call.
>    - initialize the gpio variables csi_b/rdwr_b immediately with
>      the return value from devm_gpiod_get_optional(), so we can
>      drop local gpio variable at all
> 
>   drivers/fpga/xilinx-selectmap.c | 36 ++++++++++++++++++++++++---------
>   1 file changed, 27 insertions(+), 9 deletions(-)

gentle ping.

Any updates, comments on this patch?

Thanks!

bye,
Heiko
> 
> diff --git a/drivers/fpga/xilinx-selectmap.c b/drivers/fpga/xilinx-selectmap.c
> index d0cbb5fdfe3a..e9b1d15ca054 100644
> --- a/drivers/fpga/xilinx-selectmap.c
> +++ b/drivers/fpga/xilinx-selectmap.c
> @@ -19,6 +19,8 @@
>   struct xilinx_selectmap_conf {
>   	struct xilinx_fpga_core core;
>   	void __iomem *base;
> +	struct gpio_desc *csi_b;
> +	struct gpio_desc *rdwr_b;
>   };
>   
>   #define to_xilinx_selectmap_conf(obj) \
> @@ -30,16 +32,30 @@ static int xilinx_selectmap_write(struct xilinx_fpga_core *core,
>   	struct xilinx_selectmap_conf *conf = to_xilinx_selectmap_conf(core);
>   	size_t i;
>   
> +	/*
> +	 * Assert CSI_B and select write mode.
> +	 *
> +	 * UG570 states in note 4 in Figure "Continuous x8 SelectMAP Data
> +	 * Loading", RDWR_B should be asserted before CSI_B to avoid
> +	 * causing an ABORT on the next CCLK.
> +	 *
> +	 * To be sure, set first RDWR_B pin before activate CSI_B
> +	 */
> +	gpiod_set_value(conf->rdwr_b, 1);
> +	gpiod_set_value(conf->csi_b, 1);
> +
>   	for (i = 0; i < count; ++i)
>   		writeb(buf[i], conf->base);
>   
> +	gpiod_set_value(conf->csi_b, 0);
> +	gpiod_set_value(conf->rdwr_b, 0);
> +
>   	return 0;
>   }
>   
>   static int xilinx_selectmap_probe(struct platform_device *pdev)
>   {
>   	struct xilinx_selectmap_conf *conf;
> -	struct gpio_desc *gpio;
>   	void __iomem *base;
>   
>   	conf = devm_kzalloc(&pdev->dev, sizeof(*conf), GFP_KERNEL);
> @@ -55,16 +71,18 @@ static int xilinx_selectmap_probe(struct platform_device *pdev)
>   				     "ioremap error\n");
>   	conf->base = base;
>   
> -	/* CSI_B is active low */
> -	gpio = devm_gpiod_get_optional(&pdev->dev, "csi", GPIOD_OUT_HIGH);
> -	if (IS_ERR(gpio))
> -		return dev_err_probe(&pdev->dev, PTR_ERR(gpio),
> +	/* CSI_B is active low, deassert signal */
> +	conf->csi_b = devm_gpiod_get_optional(&pdev->dev, "csi",
> +					      GPIOD_OUT_LOW);
> +	if (IS_ERR(conf->csi_b))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(conf->csi_b),
>   				     "Failed to get CSI_B gpio\n");
>   
> -	/* RDWR_B is active low */
> -	gpio = devm_gpiod_get_optional(&pdev->dev, "rdwr", GPIOD_OUT_HIGH);
> -	if (IS_ERR(gpio))
> -		return dev_err_probe(&pdev->dev, PTR_ERR(gpio),
> +	/* RDWR_B is active low, deassert signal */
> +	conf->rdwr_b = devm_gpiod_get_optional(&pdev->dev, "rdwr",
> +					       GPIOD_OUT_LOW);
> +	if (IS_ERR(conf->rdwr_b))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(conf->rdwr_b),
>   				     "Failed to get RDWR_B gpio\n");
>   
>   	return xilinx_core_probe(&conf->core);
> ---
> base-commit: db2ddb87143519e20a95aa36c60b36107b736a58
> 

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

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

[Xu Yilun]

On Mon, Aug 10, 2026 at 08:20:51AM +0200, Heiko Schocher wrote:
> The current driver requests the optional CSI_B and RDWR_B GPIOs but
> only configures their initial output state and never changes them
> afterwards. As a result, CSI_B and RDWR_B remain inactive or active

I got even confused, the old code explicitly set them GPIOD_OUT_HIGH,
why "inactive or active"? And I also confused by your comments in v3,
why "inactive or active" depends on DTS?

Do you want to activate or inactivate the GPIOs in your code? You can't
say don't know, go check the DTS, is it?

> throughout the configuration process.
> 
> This may work on systems with a single FPGA where these signals do not
> need to be controlled by software, or where the signals are configured
> to their active state.

Could you just tell the actual problem, as you said in v3 "The gpios are
never used in mainline".

> But this does not not support systems with
> multiple FPGAs sharing a SelectMAP interface.
> 
> On systems with multiple FPGAs sharing the same SelectMAP data bus,
> the driver must deassert this signals in probe, and actively control
> them during configuration.
> 
> CSI_B (Chip Select, active low) selects the target FPGA. It is asserted
> before configuration data is transferred and deasserted afterwards so
> that only the intended device responds to bus transactions.
> 
> RDWR_B (Read/Write, active low) controls the transfer direction on the
> SelectMAP interface. A low level selects write cycles, while a high
> level selects read cycles. During FPGA configuration the driver drives
> RDWR_B low before transferring the bitstream and restores it to its
> reading (high state) afterwards.
> 
> With that info from the datasheet the driver is now changed to:
> 
> - deassert the CSI_B and RDWR_B pin on probe and store the
>   optional GPIO descriptors in private driver data.
> 
> - toggle both signals around the configuration data transfer
> 
> This allows multiple FPGAs to safely share a single SelectMAP interface.
> 
> Signed-off-by: Heiko Schocher <hs@nabladev.com>
> ---
> 
> Changes in v4:
> - add comments from Xu Yulin
>   replace wrong gpiod_set_raw_value() with gpiod_set_value()
>   deassert CSI_B and RDWR_B in probe as in patch version 2
>   rework commit message (correct the description what current
>   driver do on probe), why this is not a problem with one
>   FPGA, and why it needs a change if you have N FPGAs
>   sharing the same selectmap Interface (clk and data pins).
> 
> Changes in v3:
> - use 0 (deasserted state) and 1 (asserted state) in gpio_set_value()
>   as commented from Micahl
> - rewrite commit message as requested from Xu Yilun
>   - describe what rdwr_b and csi_b do, and why this change is needed
>     for more than one FPGA.
> - add comment before asserting the signals, why they are asserted
>   in this order.
> 
> Changes in v2:
> - add comments from Michal
>   - skip check if gpio descriptor variables csi_b/rdwr_b are valid,
>     as validate_desc() checks this in gpiod_set_value() call.
>   - initialize the gpio variables csi_b/rdwr_b immediately with
>     the return value from devm_gpiod_get_optional(), so we can
>     drop local gpio variable at all
> 
>  drivers/fpga/xilinx-selectmap.c | 36 ++++++++++++++++++++++++---------
>  1 file changed, 27 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/fpga/xilinx-selectmap.c b/drivers/fpga/xilinx-selectmap.c
> index d0cbb5fdfe3a..e9b1d15ca054 100644
> --- a/drivers/fpga/xilinx-selectmap.c
> +++ b/drivers/fpga/xilinx-selectmap.c
> @@ -19,6 +19,8 @@
>  struct xilinx_selectmap_conf {
>  	struct xilinx_fpga_core core;
>  	void __iomem *base;
> +	struct gpio_desc *csi_b;
> +	struct gpio_desc *rdwr_b;
>  };
>  
>  #define to_xilinx_selectmap_conf(obj) \
> @@ -30,16 +32,30 @@ static int xilinx_selectmap_write(struct xilinx_fpga_core *core,
>  	struct xilinx_selectmap_conf *conf = to_xilinx_selectmap_conf(core);
>  	size_t i;
>  
> +	/*
> +	 * Assert CSI_B and select write mode.
> +	 *
> +	 * UG570 states in note 4 in Figure "Continuous x8 SelectMAP Data
> +	 * Loading", RDWR_B should be asserted before CSI_B to avoid
> +	 * causing an ABORT on the next CCLK.
> +	 *
> +	 * To be sure, set first RDWR_B pin before activate CSI_B
> +	 */
> +	gpiod_set_value(conf->rdwr_b, 1);
> +	gpiod_set_value(conf->csi_b, 1);
> +
>  	for (i = 0; i < count; ++i)
>  		writeb(buf[i], conf->base);
>  
> +	gpiod_set_value(conf->csi_b, 0);
> +	gpiod_set_value(conf->rdwr_b, 0);
> +
>  	return 0;
>  }
>  
>  static int xilinx_selectmap_probe(struct platform_device *pdev)
>  {
>  	struct xilinx_selectmap_conf *conf;
> -	struct gpio_desc *gpio;
>  	void __iomem *base;
>  
>  	conf = devm_kzalloc(&pdev->dev, sizeof(*conf), GFP_KERNEL);
> @@ -55,16 +71,18 @@ static int xilinx_selectmap_probe(struct platform_device *pdev)
>  				     "ioremap error\n");
>  	conf->base = base;
>  
> -	/* CSI_B is active low */
> -	gpio = devm_gpiod_get_optional(&pdev->dev, "csi", GPIOD_OUT_HIGH);
> -	if (IS_ERR(gpio))
> -		return dev_err_probe(&pdev->dev, PTR_ERR(gpio),
> +	/* CSI_B is active low, deassert signal */

I'm not sure "active low" does any help here, it just makes more
confusion.

At first glance, "active low" && "deassert" => "set it high", then you
should use GPIOD_OUT_HIGH?? You are not reasoning your change.

Please elaborate on how these flags work before you send v5, thanks.

> +	conf->csi_b = devm_gpiod_get_optional(&pdev->dev, "csi",
> +					      GPIOD_OUT_LOW);
> +	if (IS_ERR(conf->csi_b))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(conf->csi_b),
>  				     "Failed to get CSI_B gpio\n");
>  
> -	/* RDWR_B is active low */
> -	gpio = devm_gpiod_get_optional(&pdev->dev, "rdwr", GPIOD_OUT_HIGH);
> -	if (IS_ERR(gpio))
> -		return dev_err_probe(&pdev->dev, PTR_ERR(gpio),
> +	/* RDWR_B is active low, deassert signal */

Same concern

> +	conf->rdwr_b = devm_gpiod_get_optional(&pdev->dev, "rdwr",
> +					       GPIOD_OUT_LOW);
> +	if (IS_ERR(conf->rdwr_b))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(conf->rdwr_b),
>  				     "Failed to get RDWR_B gpio\n");
>  
>  	return xilinx_core_probe(&conf->core);
> ---
> base-commit: db2ddb87143519e20a95aa36c60b36107b736a58
> 
> -- 
> 2.55.0
> 
> 

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

[Heiko Schocher]

Hello Xu,

thanks for the review, and sorry for the confusion. Answers below, I will
send the v5 after you agree...

On 06.09.26 18:45, Xu Yilun wrote:
> On Mon, Aug 10, 2026 at 08:20:51AM +0200, Heiko Schocher wrote:
>> The current driver requests the optional CSI_B and RDWR_B GPIOs but
>> only configures their initial output state and never changes them
>> afterwards. As a result, CSI_B and RDWR_B remain inactive or active
> 
> I got even confused, the old code explicitly set them GPIOD_OUT_HIGH,
> why "inactive or active"? And I also confused by your comments in v3,
> why "inactive or active" depends on DTS?
> 
> Do you want to activate or inactivate the GPIOs in your code? You can't
> say don't know, go check the DTS, is it?

What I want is, to deassert both signals in probe, and asserted them
only around the data transfer in xilinx_selectmap_write().

With v4 code does this already, just my comments / commit message is
misleading... sorry.

My above sentence in the commit message is wrong, you are correct. I drop
it in v5. Nothing here depends on the DTS. The flag handed to

devm_gpiod_get_optional()

is a logical value, not a line level. gpiod_direction_output_nonotify()
does

         if (test_bit(GPIOD_FLAG_ACTIVE_LOW, &flags))
                 value = !value;

and Documentation/driver-api/gpio/consumer.rst states it as well:

         Note that the initial value is *logical* and the physical line
         level depends on whether the line is configured active high or
         active low

So GPIOD_OUT_HIGH means "asserted", on every board. The current driver
asserts CSI_B and RDWR_B in probe and leaves them asserted for the
lifetime of the device, which is a bad idea in case you have more
devices sharing one bus, and this patch fixes this issue.

>> throughout the configuration process.
>>
>> This may work on systems with a single FPGA where these signals do not
>> need to be controlled by software, or where the signals are configured
>> to their active state.
> 
> Could you just tell the actual problem, as you said in v3 "The gpios are
> never used in mainline".

The problem is two (or more) FPGAs sharing one SelectMAP data bus, which
is the case CSI_B exists for. With current driver both of them are selected
after probe all the time, so every byte written for one is clocked into
the other as well, and neither can be configured on its own.

With a single FPGA on the port the permanent assertion is harmless. 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 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.

     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.

     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.

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

> 
>> But this does not not support systems with
>> multiple FPGAs sharing a SelectMAP interface.
>>
>> On systems with multiple FPGAs sharing the same SelectMAP data bus,
>> the driver must deassert this signals in probe, and actively control
>> them during configuration.
>>
>> CSI_B (Chip Select, active low) selects the target FPGA. It is asserted
>> before configuration data is transferred and deasserted afterwards so
>> that only the intended device responds to bus transactions.
>>
>> RDWR_B (Read/Write, active low) controls the transfer direction on the
>> SelectMAP interface. A low level selects write cycles, while a high
>> level selects read cycles. During FPGA configuration the driver drives
>> RDWR_B low before transferring the bitstream and restores it to its
>> reading (high state) afterwards.
>>
>> With that info from the datasheet the driver is now changed to:
>>
>> - deassert the CSI_B and RDWR_B pin on probe and store the
>>    optional GPIO descriptors in private driver data.
>>
>> - toggle both signals around the configuration data transfer
>>
>> This allows multiple FPGAs to safely share a single SelectMAP interface.
>>
>> Signed-off-by: Heiko Schocher <hs@nabladev.com>
>> ---
>>
>> Changes in v4:
>> - add comments from Xu Yulin
>>    replace wrong gpiod_set_raw_value() with gpiod_set_value()
>>    deassert CSI_B and RDWR_B in probe as in patch version 2
>>    rework commit message (correct the description what current
>>    driver do on probe), why this is not a problem with one
>>    FPGA, and why it needs a change if you have N FPGAs
>>    sharing the same selectmap Interface (clk and data pins).
>>
>> Changes in v3:
>> - use 0 (deasserted state) and 1 (asserted state) in gpio_set_value()
>>    as commented from Micahl
>> - rewrite commit message as requested from Xu Yilun
>>    - describe what rdwr_b and csi_b do, and why this change is needed
>>      for more than one FPGA.
>> - add comment before asserting the signals, why they are asserted
>>    in this order.
>>
>> Changes in v2:
>> - add comments from Michal
>>    - skip check if gpio descriptor variables csi_b/rdwr_b are valid,
>>      as validate_desc() checks this in gpiod_set_value() call.
>>    - initialize the gpio variables csi_b/rdwr_b immediately with
>>      the return value from devm_gpiod_get_optional(), so we can
>>      drop local gpio variable at all
>>
>>   drivers/fpga/xilinx-selectmap.c | 36 ++++++++++++++++++++++++---------
>>   1 file changed, 27 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/fpga/xilinx-selectmap.c b/drivers/fpga/xilinx-selectmap.c
>> index d0cbb5fdfe3a..e9b1d15ca054 100644
>> --- a/drivers/fpga/xilinx-selectmap.c
>> +++ b/drivers/fpga/xilinx-selectmap.c
>> @@ -19,6 +19,8 @@
>>   struct xilinx_selectmap_conf {
>>   	struct xilinx_fpga_core core;
>>   	void __iomem *base;
>> +	struct gpio_desc *csi_b;
>> +	struct gpio_desc *rdwr_b;
>>   };
>>   
>>   #define to_xilinx_selectmap_conf(obj) \
>> @@ -30,16 +32,30 @@ static int xilinx_selectmap_write(struct xilinx_fpga_core *core,
>>   	struct xilinx_selectmap_conf *conf = to_xilinx_selectmap_conf(core);
>>   	size_t i;
>>   
>> +	/*
>> +	 * Assert CSI_B and select write mode.
>> +	 *
>> +	 * UG570 states in note 4 in Figure "Continuous x8 SelectMAP Data
>> +	 * Loading", RDWR_B should be asserted before CSI_B to avoid
>> +	 * causing an ABORT on the next CCLK.
>> +	 *
>> +	 * To be sure, set first RDWR_B pin before activate CSI_B
>> +	 */
>> +	gpiod_set_value(conf->rdwr_b, 1);
>> +	gpiod_set_value(conf->csi_b, 1);
>> +
>>   	for (i = 0; i < count; ++i)
>>   		writeb(buf[i], conf->base);
>>   
>> +	gpiod_set_value(conf->csi_b, 0);
>> +	gpiod_set_value(conf->rdwr_b, 0);
>> +
>>   	return 0;
>>   }
>>   
>>   static int xilinx_selectmap_probe(struct platform_device *pdev)
>>   {
>>   	struct xilinx_selectmap_conf *conf;
>> -	struct gpio_desc *gpio;
>>   	void __iomem *base;
>>   
>>   	conf = devm_kzalloc(&pdev->dev, sizeof(*conf), GFP_KERNEL);
>> @@ -55,16 +71,18 @@ static int xilinx_selectmap_probe(struct platform_device *pdev)
>>   				     "ioremap error\n");
>>   	conf->base = base;
>>   
>> -	/* CSI_B is active low */
>> -	gpio = devm_gpiod_get_optional(&pdev->dev, "csi", GPIOD_OUT_HIGH);
>> -	if (IS_ERR(gpio))
>> -		return dev_err_probe(&pdev->dev, PTR_ERR(gpio),
>> +	/* CSI_B is active low, deassert signal */
> 
> I'm not sure "active low" does any help here, it just makes more
> confusion.
> 
> At first glance, "active low" && "deassert" => "set it high", then you
> should use GPIOD_OUT_HIGH?? You are not reasoning your change.
> 
> Please elaborate on how these flags work before you send v5, thanks.

Agreed, and that comment is what causes the confusion. Active low is a
property of the board, gpiolib hides it, and the driver never needs to
know it. GPIOD_OUT_LOW is the logical 0, which is the deasserted state,
and for a line the firmware marks active low gpiolib drives it high.
GPIOD_OUT_HIGH would assert the signal, which is what I want to fix.

So I will rewrite the comments in probe for v5 to:
"""
         /*
          * Request both signals deasserted, so a device sharing the SelectMAP
          * bus with others stays off that bus until its bitstream is written.
          *
          * The value in the gpiod flags is logical, gpiolib drives the line
          * high for GPIOD_OUT_LOW when the firmware describes it active low.
          */
         conf->csi_b = devm_gpiod_get_optional(&pdev->dev, "csi",
                                               GPIOD_OUT_LOW);
"""

Many thanks!

bye,
Heiko
> 
>> +	conf->csi_b = devm_gpiod_get_optional(&pdev->dev, "csi",
>> +					      GPIOD_OUT_LOW);
>> +	if (IS_ERR(conf->csi_b))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(conf->csi_b),
>>   				     "Failed to get CSI_B gpio\n");
>>   
>> -	/* RDWR_B is active low */
>> -	gpio = devm_gpiod_get_optional(&pdev->dev, "rdwr", GPIOD_OUT_HIGH);
>> -	if (IS_ERR(gpio))
>> -		return dev_err_probe(&pdev->dev, PTR_ERR(gpio),
>> +	/* RDWR_B is active low, deassert signal */
> 
> Same concern
> 
>> +	conf->rdwr_b = devm_gpiod_get_optional(&pdev->dev, "rdwr",
>> +					       GPIOD_OUT_LOW);
>> +	if (IS_ERR(conf->rdwr_b))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(conf->rdwr_b),
>>   				     "Failed to get RDWR_B gpio\n");
>>   
>>   	return xilinx_core_probe(&conf->core);
>> ---
>> base-commit: db2ddb87143519e20a95aa36c60b36107b736a58
>>
>> -- 
>> 2.55.0
>>
>>

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

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

[Xu Yilun]

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

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

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

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.

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

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

[Xu Yilun]

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

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

[Heiko Schocher]

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