| Message ID | 20190107064148.10152-12-kishon@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | PCI: endpoint: Cleanup EPC features | expand |
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 >
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
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 >
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
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
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);
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(-)