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

[Nazle Asmade, Muhammad Nazim Amirul]

On 27/6/2026 12:37 am, Xu Yilun wrote:
> [You don't often get email from yilun.xu@xxxxxxxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> On Thu, Jun 18, 2026 at 08:43:27PM -0700, muhammad.nazim.amirul.nazle.asmade@xxxxxxxxxx wrote:
>> From: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@xxxxxxxxxx>
>>
>> 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 <muhammad.nazim.amirul.nazle.asmade@xxxxxxxxxx>
>> ---
>>   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.
Hi Yilun,
Thanks for your review and sure I will take a look on the first patch 
first before proceed to fix on this patch.

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