[PATCH v2] fpga: zynq-fpga: fix setting pcap to max speed

[PATCH v2] fpga: zynq-fpga: fix setting pcap to max speed

[Koen Beel]

The PCAP interface should be set to max speed if the bitstream is not
encrypted. The code comments also show this was the original intended
behavior.

According to the "Zynq 7000 SoC Technical Reference Manual (UG585)" the
reset value of the devcfg control register (XDCFG_CTRL_OFFSET) should be
0x0C006000, so CTRL_PCAP_RATE_EN_MASK (bit 25) should be 0 (= max speed)
at bootup.
However, the devcfg control register reset value seems to be different
in reality, and CTRL_PCAP_RATE_EN_MASK seems to be 1 (= reduced speed)
at bootup.

On top, I don't think it's sane for the driver to just assume a register
has a certain initial value and depend on it.

Fixes: 37784706bf9e ("fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000")
Cc: stable@vger.kernel.org
Signed-off-by: Koen Beel <koen.beel@b-kalm.com>
---
 drivers/fpga/zynq-fpga.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
index b7629a0e4813..83030ec1f376 100644
--- a/drivers/fpga/zynq-fpga.c
+++ b/drivers/fpga/zynq-fpga.c
@@ -344,19 +344,14 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr,
 
 	/* set configuration register with following options:
 	 * - enable PCAP interface
-	 * - set throughput for maximum speed (if bistream not encrypted)
+	 * - set throughput for maximum speed (if bitstream not encrypted)
 	 * - set CPU in user mode
 	 */
 	ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
-	if (info->flags & FPGA_MGR_ENCRYPTED_BITSTREAM)
-		zynq_fpga_write(priv, CTRL_OFFSET,
-				(CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK
-				 | CTRL_PCAP_RATE_EN_MASK | ctrl));
-	else
-		zynq_fpga_write(priv, CTRL_OFFSET,
-				(CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK
-				 | ctrl));
-
+	ctrl |= CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK;
+	FIELD_MODIFY(CTRL_PCAP_RATE_EN_MASK, &ctrl,
+		     (info->flags & FPGA_MGR_ENCRYPTED_BITSTREAM) ? 1 : 0);
+	zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
 
 	/* We expect that the command queue is empty right now. */
 	status = zynq_fpga_read(priv, STATUS_OFFSET);
-- 
2.51.0

Re: [PATCH v2] fpga: zynq-fpga: fix setting pcap to max speed

[Xu Yilun]

On Fri, Oct 24, 2025 at 05:32:21PM +0200, Koen Beel wrote:
> The PCAP interface should be set to max speed if the bitstream is not

Imperative mood please.

> encrypted. The code comments also show this was the original intended
> behavior.

Please explicitly state your change in the first paragraph, even if it
is already stated in your subject.

> 
> According to the "Zynq 7000 SoC Technical Reference Manual (UG585)" the

Add the Link please.

> reset value of the devcfg control register (XDCFG_CTRL_OFFSET) should be
> 0x0C006000,

Delete the magic number, no value to reviewers.

> so CTRL_PCAP_RATE_EN_MASK (bit 25) should be 0 (= max speed)
> at bootup.
> However, the devcfg control register reset value seems to be different
> in reality, and CTRL_PCAP_RATE_EN_MASK seems to be 1 (= reduced speed)
> at bootup.

seems to be? Please be confirmative. And be clear what happens, why the
difference.

And please state what's the issue because of the difference?

> 
> On top, I don't think it's sane for the driver to just assume a register

Again, imperative mood and avoid avoid pronouns.

Please read the Documentation/process/ to gain a general understanding
for upstreaming, thanks.

> has a certain initial value and depend on it.

According to the code comment below, I don't see the intent of "assume
& depend on the initial value". Anyway, no need to bother about that. Just
say the fact, it fails to clear the CTRL_PCAP_RATE_EN_MASK bit when not
encrypted.

> 
> Fixes: 37784706bf9e ("fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000")
> Cc: stable@vger.kernel.org
> Signed-off-by: Koen Beel <koen.beel@b-kalm.com>
> ---
>  drivers/fpga/zynq-fpga.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> index b7629a0e4813..83030ec1f376 100644
> --- a/drivers/fpga/zynq-fpga.c
> +++ b/drivers/fpga/zynq-fpga.c
> @@ -344,19 +344,14 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr,
>  
>  	/* set configuration register with following options:
>  	 * - enable PCAP interface
> -	 * - set throughput for maximum speed (if bistream not encrypted)
> +	 * - set throughput for maximum speed (if bitstream not encrypted)
>  	 * - set CPU in user mode
>  	 */
>  	ctrl = zynq_fpga_read(priv, CTRL_OFFSET);
> -	if (info->flags & FPGA_MGR_ENCRYPTED_BITSTREAM)
> -		zynq_fpga_write(priv, CTRL_OFFSET,
> -				(CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK
> -				 | CTRL_PCAP_RATE_EN_MASK | ctrl));
> -	else
> -		zynq_fpga_write(priv, CTRL_OFFSET,
> -				(CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK
> -				 | ctrl));
> -
> +	ctrl |= CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK;
> +	FIELD_MODIFY(CTRL_PCAP_RATE_EN_MASK, &ctrl,

Do we need the head file?

Thanks,
Yilun

> +		     (info->flags & FPGA_MGR_ENCRYPTED_BITSTREAM) ? 1 : 0);
> +	zynq_fpga_write(priv, CTRL_OFFSET, ctrl);
>  
>  	/* We expect that the command queue is empty right now. */
>  	status = zynq_fpga_read(priv, STATUS_OFFSET);
> -- 
> 2.51.0
> 
> 
> 
> 
> 
>