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

[Koen Beel <koen.beel@b-kalm.com>]

(resending because of formatting error; rejected by ml)
Hi Xu,
Thanks for the feedback.

I just checked, and this bug has always been there in zynq-fpga.c
So I could add:
    Fixes: 37784706bf9e ("fpga manager: Adding FPGA Manager support for Xilinx Zynq 7000")
but does this make it any more clear? It just feels kind of strange to add a fixes tag for such an old commit (that was the initial commit for this driver).
This commit will be 10 years old next week, so I honestly doubt this patch fixes a critical bug that needs to go into stable.

Some more details about the failure:
Fpga programming for non-secure bitstreams is broken, unless some bootloader changes the reset value of PCAP_RATE_EN.
This is the error I get (after which the bitstream is _not_ programmed):
    # echo fpga.bin > /sys/class/fpga_manager/fpga0/firmware
    fpga_manager fpga0: writing fpga.bin to Xilinx Zynq FPGA Manager
    fpga_manager fpga0: Error after writing image data to FPGA
    sh: write error: Connection timed out

My board is a Trenz TE0715 + custom carrier board.
Now also tested on a standard ZedBoard. Same bug behavior and patch also fixes this.
Can't test secure bitstreams here (don't have any).

According to the "Zynq 7000 SoC Technical Reference Manual (UG585)" the reset value of the register XDCFG_CTRL_OFFSET (0xF8007000) should be 0x0C006000
So XDCFG_CTRL_PCAP_RATE_EN_MASK should have a reset value of 0.
However, I tested on both my board and ZedBoard, and the actual reset value of this register seems to be 0x4E00E07F ... (strange, AMD?)
So this PCA_RATE_EN_MASK (bit 25) is actually already set (=1) at bootup.
(I don't have a hardware debugger, but print this value very early in uboot SPL, right after debug_uart init, and have checked this register value is not overwritten by ps7_init).

And yes, this bit is also still set when reading XDCFG_CTRL_OFFSET in zynq-fpga.c
Ok, the problem I have is a timeout when writing the bitstream, and this timeout should not happen even if the bitstream write speed is reduced to 25% (PCAP_RATE_EN = 1 does this), unless it's a HW problem.
So this timeout may be another bug to look into, but at least this fix is a workaround for that timeout, and it's also a valid fix for an actual bug imo.

Someone else able to test this? And test with secure bitstreams?

No idea if this timeout has always been there, but I assume things have ever worked without this timeout problem for zynq-fpga.
And I assume I'm not the only one having this problem, but I know Xilinx FSBL does reset this bit (=0) when writing a bitstream before starting uboot. And doing this hides this bug in zynq-fpga. No idea who is using xlnx fsbl and use it to write a bitstream before uboot, but I assume many people, since this is the "default option".

Anyway, I don't think it's sane to just assume a register has a certain value and just depend on it.

I'm ok with the proposed change. That's indeed more clear. Should I send a v2?

Thanks,
Koen

On 06/10/2025 04:50, Xu Yilun wrote:
> On Thu, Oct 02, 2025 at 02:41:59PM +0200, Koen Beel wrote:
>> The PCAP interface should be set to max speed if the bitstream is not
>> encrypted.
>> The code comments mention this should be done, but it wasn't the case.
> 
> Use 'fixes' tag and Cc stable kernel if it is a BUG.
> 
>> On my board, this fixes failure of programming the (non-encrypted)
>> bitstream.
> 
> Could you elaborate on the details of the failure, rather than just "on
> my board it fails".
> 
>>
>> Signed-off-by: Koen Beel <koen.beel@b-kalm.com>
>> ---
>>  drivers/fpga/zynq-fpga.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
>> index b7629a0e4813..1872ce05b566 100644
>> --- a/drivers/fpga/zynq-fpga.c
>> +++ b/drivers/fpga/zynq-fpga.c
>> @@ -354,8 +354,8 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr,
>>  				 | CTRL_PCAP_RATE_EN_MASK | ctrl));
>>  	else
>>  		zynq_fpga_write(priv, CTRL_OFFSET,
>> -				(CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK
>> -				 | ctrl));
>> +				(CTRL_PCAP_PR_MASK | CTRL_PCAP_MODE_MASK | ctrl)
>> +				 & ~CTRL_PCAP_RATE_EN_MASK);
> 
> So do you mean the CTRL_PCAP_RATE_EN_MASK bit is already set in 'ctrl' when
> you read it out?
> 
> And it is harder for me to always check all masks for both
> encrypted/non-encrypted cases.  Only the CTRL_PCAP_RATE_EN_MASK is
> different so if the following works for you:
> 
> ------------8<--------------
> diff --git a/drivers/fpga/zynq-fpga.c b/drivers/fpga/zynq-fpga.c
> index b7629a0e4813..497ed4958b0a 100644
> --- a/drivers/fpga/zynq-fpga.c
> +++ b/drivers/fpga/zynq-fpga.c
> @@ -348,15 +348,10 @@ static int zynq_fpga_ops_write_init(struct fpga_manager *mgr,
>          * - 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);
> 
>>  
>>  
>>  	/* We expect that the command queue is empty right now. */
>> -- 
>> 2.51.0
>>
>>