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

[Xu Yilun]

> +static int altera_cvp_v2_read_credit(struct altera_cvp_conf *conf,
> +				     u32 *credits)
> +{
> +	u8 val;
> +	int ret;
> +
> +	ret = altera_read_config_byte(conf, VSE_CVP_TX_CREDITS, &val);
> +	if (!ret)
> +		*credits = val;
> +
> +	return ret;
> +}
> +
> +static int altera_cvp_ag5_read_credit(struct altera_cvp_conf *conf,
> +				      u32 *credits)
> +{
> +	return altera_read_config_dword(conf, VSE_CVP_AG5_TX_CREDITS, credits);
> +}
> +
>  static int altera_cvp_v2_wait_for_credit(struct fpga_manager *mgr,
>  					 u32 blocks)
>  {
>  	u32 timeout = V2_CREDIT_TIMEOUT_US / V2_CHECK_CREDIT_US;
>  	struct altera_cvp_conf *conf = mgr->priv;
> +	u32 mask = conf->priv->credit_mask;
>  	int ret;
> -	u8 val;
> +	u32 val;
>  
>  	do {
> -		ret = altera_read_config_byte(conf, VSE_CVP_TX_CREDITS, &val);
> +		ret = conf->priv->read_credit(conf, &val);
>  		if (ret) {
>  			dev_err(&conf->pci_dev->dev,
>  				"Error reading CVP Credit Register\n");
>  			return ret;
>  		}
>  
> +		val &= mask;

How about including the masking inside .read_credit()?

> +
>  		/* Return if there is space in FIFO */
> -		if (val - (u8)conf->sent_packets)
> +		if (val - (conf->sent_packets & mask))

Any reason to mask sent_packets here? The value would be bigger than the
mask? If so, why is it safe to mask it?

I'm wondering if we could remove the mask in this function, then we
don't need a .credit_mask in cvp_priv.

>  			return 0;
>  
>  		ret = altera_cvp_chk_error(mgr, blocks * ALTERA_CVP_V2_SIZE);
> @@ -559,6 +587,18 @@ static const struct cvp_priv cvp_priv_v1 = {
>  static const struct cvp_priv cvp_priv_v2 = {
>  	.clear_state	= altera_cvp_v2_clear_state,
>  	.wait_credit	= altera_cvp_v2_wait_for_credit,
> +	.read_credit	= altera_cvp_v2_read_credit,
> +	.credit_mask	= VSE_CVP_TX_CREDITS_MASK,
> +	.block_size	= ALTERA_CVP_V2_SIZE,
> +	.poll_time_us	= V2_POLL_TIMEOUT_US,
> +	.user_time_us	= V2_USER_TIMEOUT_US,
> +};
> +
> +static const struct cvp_priv cvp_priv_agilex5 = {
> +	.clear_state	= altera_cvp_v2_clear_state,
> +	.wait_credit	= altera_cvp_v2_wait_for_credit,
> +	.read_credit	= altera_cvp_ag5_read_credit,

We have same .wait_credit across, so the only point of it is to indicate
whether the credit can be read. The .read_credit can do the same job,
is it?

I think we can remove .wait_credit

> +	.credit_mask	= VSE_CVP_AG5_TX_CREDITS_MASK,
>  	.block_size	= ALTERA_CVP_V2_SIZE,
>  	.poll_time_us	= V2_POLL_TIMEOUT_US,
>  	.user_time_us	= V2_USER_TIMEOUT_US,
> @@ -658,10 +698,15 @@ 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)
> +	if (conf->vsec_offset == V1_VSEC_OFFSET) {
>  		conf->priv = &cvp_priv_v1;
> -	else
> -		conf->priv = &cvp_priv_v2;
> +	} else {
> +		pci_read_config_dword(pdev, offset + PCI_VNDR_HEADER, &regval);
> +		if (PCI_VNDR_HEADER_LEN(regval) == AGILEX5_VSEC_LENGTH)
> +			conf->priv = &cvp_priv_agilex5;
> +		else
> +			conf->priv = &cvp_priv_v2;
> +	}
>  
>  	conf->map = pci_iomap(pdev, CVP_BAR, 0);
>  	if (!conf->map) {
> -- 
> 2.43.7
> 
>