Re: [PATCH] fpga: altera-cvp: Add Agilex5 CVP credit register support

On Thu, Jun 18, 2026 at 08:43:27PM -0700, [email protected] wrote:
> From: Nazim Amirul <[email protected]>
> 
> Extend the CvP driver to support Agilex5, which uses a different
> register layout than previous SoC FPGA variants for CVP bitstream
> transfer.
> 
> Agilex5 uses a different VSEC layout (VSEC length 0x60) and a
> dedicated CVP credit register at offset 0x5C with 12-bit width,
> compared to offset 0x49 with 8-bit width on other platforms.
> 
> Detect the device family at probe time by reading the VSEC specific
> header and checking the VSEC length field. Introduce
> device_family_type to distinguish V1, V2, and V2 Agilex5 variants.
> Use altera_read_config_dword() with a 0xFFF mask for Agilex5 credits
> and altera_read_config_byte() for all other devices.

> Fix the credit
> comparison by removing the incorrect u8 cast on sent_packets.

Is it a bug fix for existing cases? If yes, split it out to a separate
patch. Otherwise, don't say 'fix'.

> 
> Signed-off-by: Nazim Amirul <[email protected]>
> ---
>  drivers/fpga/altera-cvp.c | 48 +++++++++++++++++++++++++++++++++++----
>  1 file changed, 43 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
> index 44badfd11e1b..58128de2a8c5 100644
> --- a/drivers/fpga/altera-cvp.c
> +++ b/drivers/fpga/altera-cvp.c
> @@ -10,6 +10,7 @@
>   * Firmware must be in binary "rbf" format.
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/fpga/fpga-mgr.h>
> @@ -22,6 +23,9 @@
>  #define TIMEOUT_US	2000	/* CVP STATUS timeout for USERMODE polling */
>  
>  /* Vendor Specific Extended Capability Registers */
> +#define VSE_PCIE_SPECIFIC_HEADER	0x4	/* VSEC ID, Revision, length */
> +#define VSEC_LENGTH			GENMASK(31, 20)

Are they PCIe standard registers?

> +#define AGILEX5_VSEC_LENGTH		0x60	/* Agilex5 only */
>  #define VSE_CVP_STATUS			0x1c	/* 32bit */
>  #define VSE_CVP_STATUS_CFG_RDY		BIT(18)	/* CVP_CONFIG_READY */
>  #define VSE_CVP_STATUS_CFG_ERR		BIT(19)	/* CVP_CONFIG_ERROR */
> @@ -48,6 +52,7 @@
>  #define V1_VSEC_OFFSET			0x200	/* Vendor Specific Offset V1 */
>  /* V2 Defines */
>  #define VSE_CVP_TX_CREDITS		0x49	/* 8bit */
> +#define VSE_CVP_AG5_TX_CREDITS		0x5C	/* 12bit credits for Agilex5 */
>  
>  #define V2_CREDIT_TIMEOUT_US		40000
>  #define V2_CHECK_CREDIT_US		10
> @@ -58,6 +63,9 @@
>  
>  #define DRV_NAME		"altera-cvp"
>  #define ALTERA_CVP_MGR_NAME	"Altera CvP FPGA Manager"
> +#define SOCFPGA_CVP_V1_OTHERS	0x1
> +#define SOCFPGA_CVP_V2_OTHERS	0x2

I didn't see they are used anywhere.

> +#define SOCFPGA_CVP_V2_AGILEX5	0x3
>  
>  /* Write block sizes */
>  #define ALTERA_CVP_V1_SIZE	4
> @@ -78,6 +86,7 @@ struct altera_cvp_conf {
>  	u32			sent_packets;
>  	u32			vsec_offset;
>  	const struct cvp_priv	*priv;
> +	u32			device_family_type;
>  };
>  
>  struct cvp_priv {
> @@ -228,18 +237,32 @@ static int altera_cvp_v2_wait_for_credit(struct fpga_manager *mgr,
>  	u32 timeout = V2_CREDIT_TIMEOUT_US / V2_CHECK_CREDIT_US;
>  	struct altera_cvp_conf *conf = mgr->priv;
>  	int ret;
> -	u8 val;
> +	u32 val;
> +	u32 credit_mask = GENMASK(7, 0);
> +	u32 vse_cvp_tx_credits_offset = VSE_CVP_TX_CREDITS;

I'm not strict on reverse xmas tree, but don't make a complete mess.

> +
> +	if (conf->device_family_type == SOCFPGA_CVP_V2_AGILEX5) {
> +		vse_cvp_tx_credits_offset = VSE_CVP_AG5_TX_CREDITS;
> +		credit_mask = GENMASK(11, 0);
> +	}
>  
>  	do {
> -		ret = altera_read_config_byte(conf, VSE_CVP_TX_CREDITS, &val);
> +		/* READ DWORD is required for Agilex5 but READ BYTE is required for non-Agilex5 */
> +		if (conf->device_family_type == SOCFPGA_CVP_V2_AGILEX5)
> +			ret = altera_read_config_dword(conf, vse_cvp_tx_credits_offset, &val);
> +		else
> +			ret = altera_read_config_byte(conf, vse_cvp_tx_credits_offset, (u8 *)&val);
> +
>  		if (ret) {
>  			dev_err(&conf->pci_dev->dev,
>  				"Error reading CVP Credit Register\n");
>  			return ret;
>  		}
>  
> +		val = val & credit_mask;

Don't you have struct cvp_priv to describe HW differences? Why not use
it? Why Bifurcating code like this?

> +
>  		/* Return if there is space in FIFO */
> -		if (val - (u8)conf->sent_packets)
> +		if (val - conf->sent_packets)
>  			return 0;
>  
>  		ret = altera_cvp_chk_error(mgr, blocks * ALTERA_CVP_V2_SIZE);
> @@ -507,6 +530,8 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr,
>  				     conf->priv->user_time_us);
>  	if (ret)
>  		dev_err(&mgr->dev, "PLD_CLK_IN_USE|USERMODE timeout\n");
> +	else
> +		dev_notice(&mgr->dev, "CVP write completed successfully.\n");

Why adding this print, anything to do with "support Agilex5"?

>  
>  	return ret;
>  }
> @@ -627,10 +652,23 @@ static int altera_cvp_probe(struct pci_dev *pdev,
>  	conf->pci_dev = pdev;
>  	conf->write_data = altera_cvp_write_data_iomem;
>  
> -	if (conf->vsec_offset == V1_VSEC_OFFSET)
> +	/* To differentiate the target SOCFPGA */
> +	if (conf->vsec_offset == V1_VSEC_OFFSET) {
>  		conf->priv = &cvp_priv_v1;
> -	else
> +		conf->device_family_type = SOCFPGA_CVP_V1_OTHERS;

Why add another board type indicator, conf->priv won't work?

> +		dev_notice(&pdev->dev, "V1 target SOCFPGA detected.\n");

No news is good news, a good driver keeps quiet.

> +	} else {
> +		/* Agilex7, Stratix10, Agilex5*/
>  		conf->priv = &cvp_priv_v2;
> +		pci_read_config_dword(pdev, offset + VSE_PCIE_SPECIFIC_HEADER, &regval);
> +		if (FIELD_GET(VSEC_LENGTH, regval) == AGILEX5_VSEC_LENGTH) {
> +			conf->device_family_type = SOCFPGA_CVP_V2_AGILEX5;
> +			dev_notice(&pdev->dev, "V2 target SOCFPGA Agilex5 detected.\n");
> +		} else {
> +			conf->device_family_type = SOCFPGA_CVP_V2_OTHERS;
> +			dev_notice(&pdev->dev, "V2 target SOCFPGA detected.\n");
> +		}
> +	}

BTW: I really suggest you don't send multiple threads at a time. Getting the
first patch right, then I'll have confidence to move to the next.

>  
>  	conf->map = pci_iomap(pdev, CVP_BAR, 0);
>  	if (!conf->map) {
> -- 
> 2.43.7
> 
>