[11/15] PCI: pci-epf-test: Use pci_epc_get_features to get EPC features
diff mbox series

Message ID 20190107064148.10152-12-kishon@ti.com
State New
Headers show
Series
  • PCI: endpoint: Cleanup EPC features
Related show

Commit Message

Kishon Vijay Abraham I Jan. 7, 2019, 6:41 a.m. UTC
Use pci_epc_get_features to get EPC features such as linkup
notifier support, MSI/MSIX capable, BAR configuration etc and use it
for configuring pci-epf-test. Since these features are now obtained
directly from EPC driver, remove pci_epf_test_data which was initially
added to have EPC features in endpoint function driver.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 72 ++++++++++---------
 1 file changed, 39 insertions(+), 33 deletions(-)

Comments

Lorenzo Pieralisi Feb. 12, 2019, 3:07 p.m. UTC | #1
On Mon, Jan 07, 2019 at 12:11:44PM +0530, Kishon Vijay Abraham I wrote:

[...]

>  static int pci_epf_test_bind(struct pci_epf *epf)
>  {
>  	int ret;
>  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>  	struct pci_epf_header *header = epf->header;
> +	const struct pci_epc_features *epc_features;
> +	enum pci_barno test_reg_bar = BAR_0;
>  	struct pci_epc *epc = epf->epc;
>  	struct device *dev = &epf->dev;
> +	bool linkup_notifier = false;
> +	bool msix_capable = false;
> +	bool msi_capable = true;
>  
>  	if (WARN_ON_ONCE(!epc))
>  		return -EINVAL;
>  
> -	if (epc->features & EPC_FEATURE_NO_LINKUP_NOTIFIER)
> -		epf_test->linkup_notifier = false;
> -	else
> -		epf_test->linkup_notifier = true;
> -
> -	epf_test->msix_available = epc->features & EPC_FEATURE_MSIX_AVAILABLE;
> +	epc_features = pci_epc_get_features(epc, epf->func_no);

I think it would work out better if struct pci_epc_features was
allocated in the caller (stack) and pci_epc_get_features() take a
pointer parameter to it rather than the callee and the callee would just
have to fill it out, this also removes data in the driver that is not
really useful.

Is there any other reason behind the current design choice ?

Thanks,
Lorenzo

> +	if (!epc_features) {
> +		linkup_notifier = epc_features->linkup_notifier;
> +		msix_capable = epc_features->msix_capable;
> +		msi_capable = epc_features->msi_capable;
> +		test_reg_bar = pci_epc_get_first_free_bar(epc_features);
> +		pci_epf_configure_bar(epf, epc_features);
> +	}
>  
> -	epf_test->test_reg_bar = EPC_FEATURE_GET_BAR(epc->features);
> +	epf_test->test_reg_bar = test_reg_bar;
>  
>  	ret = pci_epc_write_header(epc, epf->func_no, header);
>  	if (ret) {
> @@ -492,13 +509,15 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>  	if (ret)
>  		return ret;
>  
> -	ret = pci_epc_set_msi(epc, epf->func_no, epf->msi_interrupts);
> -	if (ret) {
> -		dev_err(dev, "MSI configuration failed\n");
> -		return ret;
> +	if (msi_capable) {
> +		ret = pci_epc_set_msi(epc, epf->func_no, epf->msi_interrupts);
> +		if (ret) {
> +			dev_err(dev, "MSI configuration failed\n");
> +			return ret;
> +		}
>  	}
>  
> -	if (epf_test->msix_available) {
> +	if (msix_capable) {
>  		ret = pci_epc_set_msix(epc, epf->func_no, epf->msix_interrupts);
>  		if (ret) {
>  			dev_err(dev, "MSI-X configuration failed\n");
> @@ -506,7 +525,7 @@ static int pci_epf_test_bind(struct pci_epf *epf)
>  		}
>  	}
>  
> -	if (!epf_test->linkup_notifier)
> +	if (!linkup_notifier)
>  		queue_work(kpcitest_workqueue, &epf_test->cmd_handler.work);
>  
>  	return 0;
> @@ -523,17 +542,6 @@ static int pci_epf_test_probe(struct pci_epf *epf)
>  {
>  	struct pci_epf_test *epf_test;
>  	struct device *dev = &epf->dev;
> -	const struct pci_epf_device_id *match;
> -	struct pci_epf_test_data *data;
> -	enum pci_barno test_reg_bar = BAR_0;
> -	bool linkup_notifier = true;
> -
> -	match = pci_epf_match_device(pci_epf_test_ids, epf);
> -	data = (struct pci_epf_test_data *)match->driver_data;
> -	if (data) {
> -		test_reg_bar = data->test_reg_bar;
> -		linkup_notifier = data->linkup_notifier;
> -	}
>  
>  	epf_test = devm_kzalloc(dev, sizeof(*epf_test), GFP_KERNEL);
>  	if (!epf_test)
> @@ -541,8 +549,6 @@ static int pci_epf_test_probe(struct pci_epf *epf)
>  
>  	epf->header = &test_header;
>  	epf_test->epf = epf;
> -	epf_test->test_reg_bar = test_reg_bar;
> -	epf_test->linkup_notifier = linkup_notifier;
>  
>  	INIT_DELAYED_WORK(&epf_test->cmd_handler, pci_epf_test_cmd_handler);
>  
> -- 
> 2.17.1
>
Kishon Vijay Abraham I Feb. 13, 2019, 1:38 p.m. UTC | #2
Hi Lorenzo,

On 12/02/19 8:37 PM, Lorenzo Pieralisi wrote:
> On Mon, Jan 07, 2019 at 12:11:44PM +0530, Kishon Vijay Abraham I wrote:
> 
> [...]
> 
>>  static int pci_epf_test_bind(struct pci_epf *epf)
>>  {
>>  	int ret;
>>  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>>  	struct pci_epf_header *header = epf->header;
>> +	const struct pci_epc_features *epc_features;
>> +	enum pci_barno test_reg_bar = BAR_0;
>>  	struct pci_epc *epc = epf->epc;
>>  	struct device *dev = &epf->dev;
>> +	bool linkup_notifier = false;
>> +	bool msix_capable = false;
>> +	bool msi_capable = true;
>>  
>>  	if (WARN_ON_ONCE(!epc))
>>  		return -EINVAL;
>>  
>> -	if (epc->features & EPC_FEATURE_NO_LINKUP_NOTIFIER)
>> -		epf_test->linkup_notifier = false;
>> -	else
>> -		epf_test->linkup_notifier = true;
>> -
>> -	epf_test->msix_available = epc->features & EPC_FEATURE_MSIX_AVAILABLE;
>> +	epc_features = pci_epc_get_features(epc, epf->func_no);
> 
> I think it would work out better if struct pci_epc_features was
> allocated in the caller (stack) and pci_epc_get_features() take a
> pointer parameter to it rather than the callee and the callee would just
> have to fill it out, this also removes data in the driver that is not
> really useful.
> 
> Is there any other reason behind the current design choice ?

Some drivers are used by multiple platforms each with different features. In
such cases it's cleaner to have separate epc_feature table for each platform.

I think the driver should maintain some sort of data to even populate
pci_epc_features allocated by EP function driver.

Thanks
Kishon
Kishon Vijay Abraham I Feb. 13, 2019, 1:44 p.m. UTC | #3
Hi Lorenzo,

On 13/02/19 7:08 PM, Kishon Vijay Abraham I wrote:
> Hi Lorenzo,
> 
> On 12/02/19 8:37 PM, Lorenzo Pieralisi wrote:
>> On Mon, Jan 07, 2019 at 12:11:44PM +0530, Kishon Vijay Abraham I wrote:
>>
>> [...]
>>
>>>  static int pci_epf_test_bind(struct pci_epf *epf)
>>>  {
>>>  	int ret;
>>>  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>>>  	struct pci_epf_header *header = epf->header;
>>> +	const struct pci_epc_features *epc_features;
>>> +	enum pci_barno test_reg_bar = BAR_0;
>>>  	struct pci_epc *epc = epf->epc;
>>>  	struct device *dev = &epf->dev;
>>> +	bool linkup_notifier = false;
>>> +	bool msix_capable = false;
>>> +	bool msi_capable = true;
>>>  
>>>  	if (WARN_ON_ONCE(!epc))
>>>  		return -EINVAL;
>>>  
>>> -	if (epc->features & EPC_FEATURE_NO_LINKUP_NOTIFIER)
>>> -		epf_test->linkup_notifier = false;
>>> -	else
>>> -		epf_test->linkup_notifier = true;
>>> -
>>> -	epf_test->msix_available = epc->features & EPC_FEATURE_MSIX_AVAILABLE;
>>> +	epc_features = pci_epc_get_features(epc, epf->func_no);
>>
>> I think it would work out better if struct pci_epc_features was
>> allocated in the caller (stack) and pci_epc_get_features() take a
>> pointer parameter to it rather than the callee and the callee would just
>> have to fill it out, this also removes data in the driver that is not
>> really useful.
>>
>> Is there any other reason behind the current design choice ?
> 
> Some drivers are used by multiple platforms each with different features. In
> such cases it's cleaner to have separate epc_feature table for each platform.
> 
> I think the driver should maintain some sort of data to even populate
> pci_epc_features allocated by EP function driver.

Btw I found some issues in the v1 of this series, so I posted v2 [1]. Please
review that.

Thanks
Kishon

[1] -> https://lkml.org/lkml/2019/1/14/288
> 
> Thanks
> Kishon
>
Lorenzo Pieralisi Feb. 13, 2019, 2:36 p.m. UTC | #4
On Wed, Feb 13, 2019 at 07:08:18PM +0530, Kishon Vijay Abraham I wrote:
> Hi Lorenzo,
> 
> On 12/02/19 8:37 PM, Lorenzo Pieralisi wrote:
> > On Mon, Jan 07, 2019 at 12:11:44PM +0530, Kishon Vijay Abraham I wrote:
> > 
> > [...]
> > 
> >>  static int pci_epf_test_bind(struct pci_epf *epf)
> >>  {
> >>  	int ret;
> >>  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
> >>  	struct pci_epf_header *header = epf->header;
> >> +	const struct pci_epc_features *epc_features;
> >> +	enum pci_barno test_reg_bar = BAR_0;
> >>  	struct pci_epc *epc = epf->epc;
> >>  	struct device *dev = &epf->dev;
> >> +	bool linkup_notifier = false;
> >> +	bool msix_capable = false;
> >> +	bool msi_capable = true;
> >>  
> >>  	if (WARN_ON_ONCE(!epc))
> >>  		return -EINVAL;
> >>  
> >> -	if (epc->features & EPC_FEATURE_NO_LINKUP_NOTIFIER)
> >> -		epf_test->linkup_notifier = false;
> >> -	else
> >> -		epf_test->linkup_notifier = true;
> >> -
> >> -	epf_test->msix_available = epc->features & EPC_FEATURE_MSIX_AVAILABLE;
> >> +	epc_features = pci_epc_get_features(epc, epf->func_no);
> > 
> > I think it would work out better if struct pci_epc_features was
> > allocated in the caller (stack) and pci_epc_get_features() take a
> > pointer parameter to it rather than the callee and the callee would just
> > have to fill it out, this also removes data in the driver that is not
> > really useful.
> > 
> > Is there any other reason behind the current design choice ?
> 
> Some drivers are used by multiple platforms each with different features. In
> such cases it's cleaner to have separate epc_feature table for each platform.
> 
> I think the driver should maintain some sort of data to even populate
> pci_epc_features allocated by EP function driver.

You mean that every EP controller driver should keep a table of
pci_epc_features (instead of a single instance) to be matched using DT
compatible strings to detect the platform variations ?

Thanks,
Lorenzo
Kishon Vijay Abraham I Feb. 14, 2019, 5:06 a.m. UTC | #5
Hi Lorenzo,

On 13/02/19 8:06 PM, Lorenzo Pieralisi wrote:
> On Wed, Feb 13, 2019 at 07:08:18PM +0530, Kishon Vijay Abraham I wrote:
>> Hi Lorenzo,
>>
>> On 12/02/19 8:37 PM, Lorenzo Pieralisi wrote:
>>> On Mon, Jan 07, 2019 at 12:11:44PM +0530, Kishon Vijay Abraham I wrote:
>>>
>>> [...]
>>>
>>>>  static int pci_epf_test_bind(struct pci_epf *epf)
>>>>  {
>>>>  	int ret;
>>>>  	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
>>>>  	struct pci_epf_header *header = epf->header;
>>>> +	const struct pci_epc_features *epc_features;
>>>> +	enum pci_barno test_reg_bar = BAR_0;
>>>>  	struct pci_epc *epc = epf->epc;
>>>>  	struct device *dev = &epf->dev;
>>>> +	bool linkup_notifier = false;
>>>> +	bool msix_capable = false;
>>>> +	bool msi_capable = true;
>>>>  
>>>>  	if (WARN_ON_ONCE(!epc))
>>>>  		return -EINVAL;
>>>>  
>>>> -	if (epc->features & EPC_FEATURE_NO_LINKUP_NOTIFIER)
>>>> -		epf_test->linkup_notifier = false;
>>>> -	else
>>>> -		epf_test->linkup_notifier = true;
>>>> -
>>>> -	epf_test->msix_available = epc->features & EPC_FEATURE_MSIX_AVAILABLE;
>>>> +	epc_features = pci_epc_get_features(epc, epf->func_no);
>>>
>>> I think it would work out better if struct pci_epc_features was
>>> allocated in the caller (stack) and pci_epc_get_features() take a
>>> pointer parameter to it rather than the callee and the callee would just
>>> have to fill it out, this also removes data in the driver that is not
>>> really useful.
>>>
>>> Is there any other reason behind the current design choice ?
>>
>> Some drivers are used by multiple platforms each with different features. In
>> such cases it's cleaner to have separate epc_feature table for each platform.
>>
>> I think the driver should maintain some sort of data to even populate
>> pci_epc_features allocated by EP function driver.
> 
> You mean that every EP controller driver should keep a table of
> pci_epc_features (instead of a single instance) to be matched using DT
> compatible strings to detect the platform variations ?

Yes.

Thanks
Kishon

Patch
diff mbox series

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index ade296180383..a26f6ccaa322 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -47,8 +47,6 @@  struct pci_epf_test {
 	void			*reg[6];
 	struct pci_epf		*epf;
 	enum pci_barno		test_reg_bar;
-	bool			linkup_notifier;
-	bool			msix_available;
 	struct delayed_work	cmd_handler;
 };
 
@@ -71,11 +69,6 @@  static struct pci_epf_header test_header = {
 	.interrupt_pin	= PCI_INTERRUPT_INTA,
 };
 
-struct pci_epf_test_data {
-	enum pci_barno	test_reg_bar;
-	bool		linkup_notifier;
-};
-
 static size_t bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 };
 
 static int pci_epf_test_copy(struct pci_epf_test *epf_test)
@@ -458,25 +451,49 @@  static int pci_epf_test_alloc_space(struct pci_epf *epf)
 	return 0;
 }
 
+static void pci_epf_configure_bar(struct pci_epf *epf,
+				  const struct pci_epc_features *epc_features)
+{
+	struct pci_epf_bar *epf_bar;
+	bool bar_fixed_64bit;
+	int i;
+
+	for (i = BAR_0; i <= BAR_5; i++) {
+		epf_bar = &epf->bar[i];
+		bar_fixed_64bit = !!(epc_features->bar_fixed_64bit & (1 << i));
+		if (bar_fixed_64bit)
+			epf_bar->flags |= PCI_BASE_ADDRESS_MEM_TYPE_64;
+		if (epc_features->bar_fixed_size[i])
+			bar_size[i] = epc_features->bar_fixed_size[i];
+	}
+}
+
 static int pci_epf_test_bind(struct pci_epf *epf)
 {
 	int ret;
 	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
 	struct pci_epf_header *header = epf->header;
+	const struct pci_epc_features *epc_features;
+	enum pci_barno test_reg_bar = BAR_0;
 	struct pci_epc *epc = epf->epc;
 	struct device *dev = &epf->dev;
+	bool linkup_notifier = false;
+	bool msix_capable = false;
+	bool msi_capable = true;
 
 	if (WARN_ON_ONCE(!epc))
 		return -EINVAL;
 
-	if (epc->features & EPC_FEATURE_NO_LINKUP_NOTIFIER)
-		epf_test->linkup_notifier = false;
-	else
-		epf_test->linkup_notifier = true;
-
-	epf_test->msix_available = epc->features & EPC_FEATURE_MSIX_AVAILABLE;
+	epc_features = pci_epc_get_features(epc, epf->func_no);
+	if (!epc_features) {
+		linkup_notifier = epc_features->linkup_notifier;
+		msix_capable = epc_features->msix_capable;
+		msi_capable = epc_features->msi_capable;
+		test_reg_bar = pci_epc_get_first_free_bar(epc_features);
+		pci_epf_configure_bar(epf, epc_features);
+	}
 
-	epf_test->test_reg_bar = EPC_FEATURE_GET_BAR(epc->features);
+	epf_test->test_reg_bar = test_reg_bar;
 
 	ret = pci_epc_write_header(epc, epf->func_no, header);
 	if (ret) {
@@ -492,13 +509,15 @@  static int pci_epf_test_bind(struct pci_epf *epf)
 	if (ret)
 		return ret;
 
-	ret = pci_epc_set_msi(epc, epf->func_no, epf->msi_interrupts);
-	if (ret) {
-		dev_err(dev, "MSI configuration failed\n");
-		return ret;
+	if (msi_capable) {
+		ret = pci_epc_set_msi(epc, epf->func_no, epf->msi_interrupts);
+		if (ret) {
+			dev_err(dev, "MSI configuration failed\n");
+			return ret;
+		}
 	}
 
-	if (epf_test->msix_available) {
+	if (msix_capable) {
 		ret = pci_epc_set_msix(epc, epf->func_no, epf->msix_interrupts);
 		if (ret) {
 			dev_err(dev, "MSI-X configuration failed\n");
@@ -506,7 +525,7 @@  static int pci_epf_test_bind(struct pci_epf *epf)
 		}
 	}
 
-	if (!epf_test->linkup_notifier)
+	if (!linkup_notifier)
 		queue_work(kpcitest_workqueue, &epf_test->cmd_handler.work);
 
 	return 0;
@@ -523,17 +542,6 @@  static int pci_epf_test_probe(struct pci_epf *epf)
 {
 	struct pci_epf_test *epf_test;
 	struct device *dev = &epf->dev;
-	const struct pci_epf_device_id *match;
-	struct pci_epf_test_data *data;
-	enum pci_barno test_reg_bar = BAR_0;
-	bool linkup_notifier = true;
-
-	match = pci_epf_match_device(pci_epf_test_ids, epf);
-	data = (struct pci_epf_test_data *)match->driver_data;
-	if (data) {
-		test_reg_bar = data->test_reg_bar;
-		linkup_notifier = data->linkup_notifier;
-	}
 
 	epf_test = devm_kzalloc(dev, sizeof(*epf_test), GFP_KERNEL);
 	if (!epf_test)
@@ -541,8 +549,6 @@  static int pci_epf_test_probe(struct pci_epf *epf)
 
 	epf->header = &test_header;
 	epf_test->epf = epf;
-	epf_test->test_reg_bar = test_reg_bar;
-	epf_test->linkup_notifier = linkup_notifier;
 
 	INIT_DELAYED_WORK(&epf_test->cmd_handler, pci_epf_test_cmd_handler);