Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Reply-To: thor.thayer@linux.intel.com
Subject: Re: [PATCH 1/3] fpga: altera-cvp: Discover Vendor Specific offset
To:     Moritz Fischer <mdf@kernel.org>
Cc:     richard.gong@intel.com, agust@denx.de, linux-fpga@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <1562877170-23931-1-git-send-email-thor.thayer@linux.intel.com>
 <1562877170-23931-2-git-send-email-thor.thayer@linux.intel.com>
 <20190714184019.GA9048@archbook>
From:   Thor Thayer <thor.thayer@linux.intel.com>
Message-ID: <8c17e787-3548-b057-0f5d-5646a0fa80a2@linux.intel.com>
Date:   Mon, 15 Jul 2019 08:39:09 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.2
MIME-Version: 1.0
In-Reply-To: <20190714184019.GA9048@archbook>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Moritz,

On 7/14/19 1:40 PM, Moritz Fischer wrote:
> On Thu, Jul 11, 2019 at 03:32:48PM -0500, thor.thayer@linux.intel.com wrote:
>> From: Thor Thayer <thor.thayer@linux.intel.com>
>>
>> Newer Intel FPGAs have different Vendor Specific offsets than
>> legacy parts. Use PCI discovery to find the CvP registers.
>> Since the register positions remain the same, change the hard
>> coded address to a more flexible way of indexing registers
>> from the offset.
>> Adding new PCI read and write abstraction functions to
>> handle the offset (altera_read_config_dword() and
>> altera_write_config_dword()).
>>
>> Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
>> ---
>>   drivers/fpga/altera-cvp.c | 91 +++++++++++++++++++++++++++++------------------
>>   1 file changed, 56 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/fpga/altera-cvp.c b/drivers/fpga/altera-cvp.c
>> index 770915fb97f9..04f2b2a072a7 100644
>> --- a/drivers/fpga/altera-cvp.c
>> +++ b/drivers/fpga/altera-cvp.c
>> @@ -22,10 +22,10 @@
>>   #define TIMEOUT_US	2000	/* CVP STATUS timeout for USERMODE polling */
>>   
>>   /* Vendor Specific Extended Capability Registers */
>> -#define VSE_PCIE_EXT_CAP_ID		0x200
>> +#define VSE_PCIE_EXT_CAP_ID		0x0
>>   #define VSE_PCIE_EXT_CAP_ID_VAL		0x000b	/* 16bit */
>>   
>> -#define VSE_CVP_STATUS			0x21c	/* 32bit */
>> +#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 */
>>   #define VSE_CVP_STATUS_CVP_EN		BIT(20)	/* ctrl block is enabling CVP */
>> @@ -33,18 +33,18 @@
>>   #define VSE_CVP_STATUS_CFG_DONE		BIT(23)	/* CVP_CONFIG_DONE */
>>   #define VSE_CVP_STATUS_PLD_CLK_IN_USE	BIT(24)	/* PLD_CLK_IN_USE */
>>   
>> -#define VSE_CVP_MODE_CTRL		0x220	/* 32bit */
>> +#define VSE_CVP_MODE_CTRL		0x20	/* 32bit */
>>   #define VSE_CVP_MODE_CTRL_CVP_MODE	BIT(0)	/* CVP (1) or normal mode (0) */
>>   #define VSE_CVP_MODE_CTRL_HIP_CLK_SEL	BIT(1) /* PMA (1) or fabric clock (0) */
>>   #define VSE_CVP_MODE_CTRL_NUMCLKS_OFF	8	/* NUMCLKS bits offset */
>>   #define VSE_CVP_MODE_CTRL_NUMCLKS_MASK	GENMASK(15, 8)
>>   
>> -#define VSE_CVP_DATA			0x228	/* 32bit */
>> -#define VSE_CVP_PROG_CTRL		0x22c	/* 32bit */
>> +#define VSE_CVP_DATA			0x28	/* 32bit */
>> +#define VSE_CVP_PROG_CTRL		0x2c	/* 32bit */
>>   #define VSE_CVP_PROG_CTRL_CONFIG	BIT(0)
>>   #define VSE_CVP_PROG_CTRL_START_XFER	BIT(1)
>>   
>> -#define VSE_UNCOR_ERR_STATUS		0x234	/* 32bit */
>> +#define VSE_UNCOR_ERR_STATUS		0x34	/* 32bit */
>>   #define VSE_UNCOR_ERR_CVP_CFG_ERR	BIT(5)	/* CVP_CONFIG_ERROR_LATCHED */
>>   
>>   #define DRV_NAME		"altera-cvp"
>> @@ -60,14 +60,27 @@ struct altera_cvp_conf {
>>   	void			(*write_data)(struct altera_cvp_conf *, u32);
>>   	char			mgr_name[64];
>>   	u8			numclks;
>> +	u32			vsec_offset;
>>   };
>>   
>> +static inline void altera_read_config_dword(struct altera_cvp_conf *conf,
>> +					    int where, u32 *val)
>> +{
>> +	pci_read_config_dword(conf->pci_dev, conf->vsec_offset + where, val);
>> +}
> 
> I think the compiler would inline those anyway.

OK. I will take both of these out.

>> +
>> +static inline void altera_write_config_dword(struct altera_cvp_conf *conf,
>> +					     int where, u32 val)
>> +{
>> +	pci_write_config_dword(conf->pci_dev, conf->vsec_offset + where, val);
>> +}
> 
> I think the compiler would inline those anyway.
>> +
>>   static enum fpga_mgr_states altera_cvp_state(struct fpga_manager *mgr)
>>   {
>>   	struct altera_cvp_conf *conf = mgr->priv;
>>   	u32 status;
>>   
>> -	pci_read_config_dword(conf->pci_dev, VSE_CVP_STATUS, &status);
>> +	altera_read_config_dword(conf, VSE_CVP_STATUS, &status);
>>   
>>   	if (status & VSE_CVP_STATUS_CFG_DONE)
>>   		return FPGA_MGR_STATE_OPERATING;
>> @@ -85,7 +98,8 @@ static void altera_cvp_write_data_iomem(struct altera_cvp_conf *conf, u32 val)
>>   
>>   static void altera_cvp_write_data_config(struct altera_cvp_conf *conf, u32 val)
>>   {
>> -	pci_write_config_dword(conf->pci_dev, VSE_CVP_DATA, val);
>> +	pci_write_config_dword(conf->pci_dev, conf->vsec_offset + VSE_CVP_DATA,
>> +			       val);
>>   }
>>   
>>   /* switches between CvP clock and internal clock */
>> @@ -95,10 +109,10 @@ static void altera_cvp_dummy_write(struct altera_cvp_conf *conf)
>>   	u32 val;
>>   
>>   	/* set 1 CVP clock cycle for every CVP Data Register Write */
>> -	pci_read_config_dword(conf->pci_dev, VSE_CVP_MODE_CTRL, &val);
>> +	altera_read_config_dword(conf, VSE_CVP_MODE_CTRL, &val);
>>   	val &= ~VSE_CVP_MODE_CTRL_NUMCLKS_MASK;
>>   	val |= 1 << VSE_CVP_MODE_CTRL_NUMCLKS_OFF;
>> -	pci_write_config_dword(conf->pci_dev, VSE_CVP_MODE_CTRL, val);
>> +	altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val);
>>   
>>   	for (i = 0; i < CVP_DUMMY_WR; i++)
>>   		conf->write_data(conf, 0); /* dummy data, could be any value */
>> @@ -115,7 +129,7 @@ static int altera_cvp_wait_status(struct altera_cvp_conf *conf, u32 status_mask,
>>   		retries++;
>>   
>>   	do {
>> -		pci_read_config_dword(conf->pci_dev, VSE_CVP_STATUS, &val);
>> +		altera_read_config_dword(conf, VSE_CVP_STATUS, &val);
>>   		if ((val & status_mask) == status_val)
>>   			return 0;
>>   
>> @@ -130,18 +144,17 @@ static int altera_cvp_teardown(struct fpga_manager *mgr,
>>   			       struct fpga_image_info *info)
>>   {
>>   	struct altera_cvp_conf *conf = mgr->priv;
>> -	struct pci_dev *pdev = conf->pci_dev;
>>   	int ret;
>>   	u32 val;
>>   
>>   	/* STEP 12 - reset START_XFER bit */
>> -	pci_read_config_dword(pdev, VSE_CVP_PROG_CTRL, &val);
>> +	altera_read_config_dword(conf, VSE_CVP_PROG_CTRL, &val);
>>   	val &= ~VSE_CVP_PROG_CTRL_START_XFER;
>> -	pci_write_config_dword(pdev, VSE_CVP_PROG_CTRL, val);
>> +	altera_write_config_dword(conf, VSE_CVP_PROG_CTRL, val);
>>   
>>   	/* STEP 13 - reset CVP_CONFIG bit */
>>   	val &= ~VSE_CVP_PROG_CTRL_CONFIG;
>> -	pci_write_config_dword(pdev, VSE_CVP_PROG_CTRL, val);
>> +	altera_write_config_dword(conf, VSE_CVP_PROG_CTRL, val);
>>   
>>   	/*
>>   	 * STEP 14
>> @@ -163,7 +176,6 @@ static int altera_cvp_write_init(struct fpga_manager *mgr,
>>   				 const char *buf, size_t count)
>>   {
>>   	struct altera_cvp_conf *conf = mgr->priv;
>> -	struct pci_dev *pdev = conf->pci_dev;
>>   	u32 iflags, val;
>>   	int ret;
>>   
>> @@ -183,7 +195,7 @@ static int altera_cvp_write_init(struct fpga_manager *mgr,
>>   		conf->numclks = 1; /* for uncompressed and unencrypted images */
>>   
>>   	/* STEP 1 - read CVP status and check CVP_EN flag */
>> -	pci_read_config_dword(pdev, VSE_CVP_STATUS, &val);
>> +	altera_read_config_dword(conf, VSE_CVP_STATUS, &val);
>>   	if (!(val & VSE_CVP_STATUS_CVP_EN)) {
>>   		dev_err(&mgr->dev, "CVP mode off: 0x%04x\n", val);
>>   		return -ENODEV;
>> @@ -201,14 +213,14 @@ static int altera_cvp_write_init(struct fpga_manager *mgr,
>>   	 * - set HIP_CLK_SEL and CVP_MODE (must be set in the order mentioned)
>>   	 */
>>   	/* switch from fabric to PMA clock */
>> -	pci_read_config_dword(pdev, VSE_CVP_MODE_CTRL, &val);
>> +	altera_read_config_dword(conf, VSE_CVP_MODE_CTRL, &val);
>>   	val |= VSE_CVP_MODE_CTRL_HIP_CLK_SEL;
>> -	pci_write_config_dword(pdev, VSE_CVP_MODE_CTRL, val);
>> +	altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val);
>>   
>>   	/* set CVP mode */
>> -	pci_read_config_dword(pdev, VSE_CVP_MODE_CTRL, &val);
>> +	altera_read_config_dword(conf, VSE_CVP_MODE_CTRL, &val);
>>   	val |= VSE_CVP_MODE_CTRL_CVP_MODE;
>> -	pci_write_config_dword(pdev, VSE_CVP_MODE_CTRL, val);
>> +	altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val);
>>   
>>   	/*
>>   	 * STEP 3
>> @@ -217,10 +229,10 @@ static int altera_cvp_write_init(struct fpga_manager *mgr,
>>   	altera_cvp_dummy_write(conf);
>>   
>>   	/* STEP 4 - set CVP_CONFIG bit */
>> -	pci_read_config_dword(pdev, VSE_CVP_PROG_CTRL, &val);
>> +	altera_read_config_dword(conf, VSE_CVP_PROG_CTRL, &val);
>>   	/* request control block to begin transfer using CVP */
>>   	val |= VSE_CVP_PROG_CTRL_CONFIG;
>> -	pci_write_config_dword(pdev, VSE_CVP_PROG_CTRL, val);
>> +	altera_write_config_dword(conf, VSE_CVP_PROG_CTRL, val);
>>   
>>   	/* STEP 5 - poll CVP_CONFIG READY for 1 with 10us timeout */
>>   	ret = altera_cvp_wait_status(conf, VSE_CVP_STATUS_CFG_RDY,
>> @@ -237,15 +249,15 @@ static int altera_cvp_write_init(struct fpga_manager *mgr,
>>   	altera_cvp_dummy_write(conf);
>>   
>>   	/* STEP 7 - set START_XFER */
>> -	pci_read_config_dword(pdev, VSE_CVP_PROG_CTRL, &val);
>> +	altera_read_config_dword(conf, VSE_CVP_PROG_CTRL, &val);
>>   	val |= VSE_CVP_PROG_CTRL_START_XFER;
>> -	pci_write_config_dword(pdev, VSE_CVP_PROG_CTRL, val);
>> +	altera_write_config_dword(conf, VSE_CVP_PROG_CTRL, val);
>>   
>>   	/* STEP 8 - start transfer (set CVP_NUMCLKS for bitstream) */
>> -	pci_read_config_dword(pdev, VSE_CVP_MODE_CTRL, &val);
>> +	altera_read_config_dword(conf, VSE_CVP_MODE_CTRL, &val);
>>   	val &= ~VSE_CVP_MODE_CTRL_NUMCLKS_MASK;
>>   	val |= conf->numclks << VSE_CVP_MODE_CTRL_NUMCLKS_OFF;
>> -	pci_write_config_dword(pdev, VSE_CVP_MODE_CTRL, val);
>> +	altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val);
>>   
>>   	return 0;
>>   }
>> @@ -256,7 +268,7 @@ static inline int altera_cvp_chk_error(struct fpga_manager *mgr, size_t bytes)
>>   	u32 val;
>>   
>>   	/* STEP 10 (optional) - check CVP_CONFIG_ERROR flag */
>> -	pci_read_config_dword(conf->pci_dev, VSE_CVP_STATUS, &val);
>> +	altera_read_config_dword(conf, VSE_CVP_STATUS, &val);
>>   	if (val & VSE_CVP_STATUS_CFG_ERR) {
>>   		dev_err(&mgr->dev, "CVP_CONFIG_ERROR after %zu bytes!\n",
>>   			bytes);
>> @@ -315,7 +327,6 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr,
>>   				     struct fpga_image_info *info)
>>   {
>>   	struct altera_cvp_conf *conf = mgr->priv;
>> -	struct pci_dev *pdev = conf->pci_dev;
>>   	int ret;
>>   	u32 mask;
>>   	u32 val;
>> @@ -325,17 +336,17 @@ static int altera_cvp_write_complete(struct fpga_manager *mgr,
>>   		return ret;
>>   
>>   	/* STEP 16 - check CVP_CONFIG_ERROR_LATCHED bit */
>> -	pci_read_config_dword(pdev, VSE_UNCOR_ERR_STATUS, &val);
>> +	altera_read_config_dword(conf, VSE_UNCOR_ERR_STATUS, &val);
>>   	if (val & VSE_UNCOR_ERR_CVP_CFG_ERR) {
>>   		dev_err(&mgr->dev, "detected CVP_CONFIG_ERROR_LATCHED!\n");
>>   		return -EPROTO;
>>   	}
>>   
>>   	/* STEP 17 - reset CVP_MODE and HIP_CLK_SEL bit */
>> -	pci_read_config_dword(pdev, VSE_CVP_MODE_CTRL, &val);
>> +	altera_read_config_dword(conf, VSE_CVP_MODE_CTRL, &val);
>>   	val &= ~VSE_CVP_MODE_CTRL_HIP_CLK_SEL;
>>   	val &= ~VSE_CVP_MODE_CTRL_CVP_MODE;
>> -	pci_write_config_dword(pdev, VSE_CVP_MODE_CTRL, val);
>> +	altera_write_config_dword(conf, VSE_CVP_MODE_CTRL, val);
>>   
>>   	/* STEP 18 - poll PLD_CLK_IN_USE and USER_MODE bits */
>>   	mask = VSE_CVP_STATUS_PLD_CLK_IN_USE | VSE_CVP_STATUS_USERMODE;
>> @@ -396,20 +407,27 @@ static int altera_cvp_probe(struct pci_dev *pdev,
>>   	struct fpga_manager *mgr;
>>   	u16 cmd, val;
>>   	u32 regval;
>> -	int ret;
>> +	int ret, offset;
> 
> Reverse tree, please:
>    	struct fpga_manager *mgr;
>   +	int ret, offset;
>    	u16 cmd, val;
>    	u32 regval;
>   -	int ret;
> 
OK. I'll make the change. I'll also move regval above the u16 line.

>> +
>> +	/* Discover the Vendor Specific Offset for this device */
>> +	offset = pci_find_next_ext_capability(pdev, 0, PCI_EXT_CAP_ID_VNDR);
>> +	if (!offset) {
>> +		dev_err(&pdev->dev, "No Vendor Specific Offset.\n");
>> +		return -ENODEV;
>> +	}
>>   
>>   	/*
>>   	 * First check if this is the expected FPGA device. PCI config
>>   	 * space access works without enabling the PCI device, memory
>>   	 * space access is enabled further down.
>>   	 */
>> -	pci_read_config_word(pdev, VSE_PCIE_EXT_CAP_ID, &val);
>> +	pci_read_config_word(pdev, offset + VSE_PCIE_EXT_CAP_ID, &val);
>>   	if (val != VSE_PCIE_EXT_CAP_ID_VAL) {
>>   		dev_err(&pdev->dev, "Wrong EXT_CAP_ID value 0x%x\n", val);
>>   		return -ENODEV;
>>   	}
>>   
>> -	pci_read_config_dword(pdev, VSE_CVP_STATUS, &regval);
>> +	pci_read_config_dword(pdev, offset + VSE_CVP_STATUS, &regval);
>>   	if (!(regval & VSE_CVP_STATUS_CVP_EN)) {
>>   		dev_err(&pdev->dev,
>>   			"CVP is disabled for this device: CVP_STATUS Reg 0x%x\n",
>> @@ -421,6 +439,9 @@ static int altera_cvp_probe(struct pci_dev *pdev,
>>   	if (!conf)
>>   		return -ENOMEM;
>>   
>> +	conf->vsec_offset = offset;
> 
> If you'd swap the order you could use your new helper earlier, right?
> not sure if that saves you some code. Your call.
> 
Yes. However, I figured it was cleaner to perform the 2 extra checks 
before commiting to the allocation of the altera_cvp_conf struct.

If you would prefer the move, I can do that in the next version.

Thanks for reviewing!

Thor

>> +	dev_dbg(&pdev->dev, "Vendor Specific Data Offset at 0x%x\n", offset);
>> +
>>   	/*
>>   	 * Enable memory BAR access. We cannot use pci_enable_device() here
>>   	 * because it will make the driver unusable with FPGA devices that
>> -- 
>> 2.7.4
>>
> 
> Thanks,
> Moritz
>