| Message ID | 20190107064148.10152-1-kishon@ti.com |
|---|---|
| Headers | show |
| Series | PCI: endpoint: Cleanup EPC features | expand |
Am Montag, 7. Januar 2019, 07:41:33 CET schrieb Kishon Vijay Abraham I: > Hi Lorenzo, > > The Endpoint controller driver uses features member in 'struct pci_epc' > to advertise the list of supported features to the endpoint function > driver. > > There are a few shortcomings with this approach. > *) Certain endpoint controllers support fixed size BAR (e.g. TI's > AM654 uses Designware configuration with fixed size BAR). The > size of each BARs cannot be passed to the endpoint function > driver. > *) Too many macros for handling EPC features. > (EPC_FEATURE_NO_LINKUP_NOTIFIER, EPC_FEATURE_BAR_MASK, > EPC_FEATURE_MSIX_AVAILABLE, EPC_FEATURE_SET_BAR, > EPC_FEATURE_GET_BAR) > *) Endpoint controllers are directly modifying struct pci_epc > members. (I have plans to move struct pci_epc to > drivers/pci/endpoint so that pci_epc members are referenced > only by endpoint core). > > To overcome the above shortcomings, introduced pci_epc_get_features() > API, pci_epc_features structure and a ->get_features() callback. > > Also added a patch to set BAR flags in pci_epf_alloc_space and > remove it from pci-epf-test function driver. > > Tested on TI's DRA7xx platform. While I don't have that much PCI experience and hence cannot judge this cleanup as a whole, I can at least say, that my Rockchip rk3399 still does find its PCIE-connected wifi card, so this series on rk3399 Tested-by: Heiko Stuebner <heiko@sntech.de>
On Mon, Jan 07, 2019 at 12:11:33PM +0530, Kishon Vijay Abraham I wrote: > Hi Lorenzo, > > The Endpoint controller driver uses features member in 'struct pci_epc' > to advertise the list of supported features to the endpoint function > driver. > > There are a few shortcomings with this approach. > *) Certain endpoint controllers support fixed size BAR (e.g. TI's > AM654 uses Designware configuration with fixed size BAR). The > size of each BARs cannot be passed to the endpoint function > driver. > *) Too many macros for handling EPC features. > (EPC_FEATURE_NO_LINKUP_NOTIFIER, EPC_FEATURE_BAR_MASK, > EPC_FEATURE_MSIX_AVAILABLE, EPC_FEATURE_SET_BAR, > EPC_FEATURE_GET_BAR) > *) Endpoint controllers are directly modifying struct pci_epc > members. (I have plans to move struct pci_epc to > drivers/pci/endpoint so that pci_epc members are referenced > only by endpoint core). > > To overcome the above shortcomings, introduced pci_epc_get_features() > API, pci_epc_features structure and a ->get_features() callback. > > Also added a patch to set BAR flags in pci_epf_alloc_space and > remove it from pci-epf-test function driver. > > Tested on TI's DRA7xx platform. Hi Kishon, I have no major objections to this series but I would like to see some testing done in EP mode (on test platforms other than DRA7XX) to make sure it does not break anything. Please do help Kishon get some testing done. Thanks, Lorenzo > Thanks > Kishon > > Kishon Vijay Abraham I (15): > PCI: endpoint: Add new pci_epc_ops to get EPC features > PCI: dwc: Add ->get_features() callback function in dw_pcie_ep_ops > PCI: designware-plat: Populate ->get_features() dw_pcie_ep_ops > PCI: pci-dra7xx: Populate ->get_features() dw_pcie_ep_ops > PCI: rockchip: Populate ->get_features() dw_pcie_ep_ops > PCI: cadence: Populate ->get_features() cdns_pcie_epc_ops > PCI: endpoint: Add helper to get first unreserved BAR > PCI: endpoint: Fix pci_epf_alloc_space to set correct MEM TYPE flags > PCI: pci-epf-test: Remove setting epf_bar flags in function driver > PCI: pci-epf-test: Do not allocate next BARs memory if current BAR is > 64Bit > PCI: pci-epf-test: Use pci_epc_get_features to get EPC features > PCI: cadence: Remove pci_epf_linkup from Cadence EP driver > PCI: rockchip: Remove pci_epf_linkup from Rockchip EP driver > PCI: designware-plat: Remove setting epc->features in Designware plat > EP driver > PCI: endpoint: Remove features member in struct pci_epc > > drivers/pci/controller/dwc/pci-dra7xx.c | 13 +++ > .../pci/controller/dwc/pcie-designware-ep.c | 12 +++ > .../pci/controller/dwc/pcie-designware-plat.c | 17 +++- > drivers/pci/controller/dwc/pcie-designware.h | 1 + > drivers/pci/controller/pcie-cadence-ep.c | 25 +++--- > drivers/pci/controller/pcie-rockchip-ep.c | 16 +++- > drivers/pci/endpoint/functions/pci-epf-test.c | 80 ++++++++++--------- > drivers/pci/endpoint/pci-epc-core.c | 52 ++++++++++++ > drivers/pci/endpoint/pci-epf-core.c | 3 + > include/linux/pci-epc.h | 30 +++++-- > 10 files changed, 185 insertions(+), 64 deletions(-) > > -- > 2.17.1 >
Hi, On 24/01/19 8:22 PM, Lorenzo Pieralisi wrote: > On Mon, Jan 07, 2019 at 12:11:33PM +0530, Kishon Vijay Abraham I wrote: >> Hi Lorenzo, >> >> The Endpoint controller driver uses features member in 'struct pci_epc' >> to advertise the list of supported features to the endpoint function >> driver. >> >> There are a few shortcomings with this approach. >> *) Certain endpoint controllers support fixed size BAR (e.g. TI's >> AM654 uses Designware configuration with fixed size BAR). The >> size of each BARs cannot be passed to the endpoint function >> driver. >> *) Too many macros for handling EPC features. >> (EPC_FEATURE_NO_LINKUP_NOTIFIER, EPC_FEATURE_BAR_MASK, >> EPC_FEATURE_MSIX_AVAILABLE, EPC_FEATURE_SET_BAR, >> EPC_FEATURE_GET_BAR) >> *) Endpoint controllers are directly modifying struct pci_epc >> members. (I have plans to move struct pci_epc to >> drivers/pci/endpoint so that pci_epc members are referenced >> only by endpoint core). >> >> To overcome the above shortcomings, introduced pci_epc_get_features() >> API, pci_epc_features structure and a ->get_features() callback. >> >> Also added a patch to set BAR flags in pci_epf_alloc_space and >> remove it from pci-epf-test function driver. >> >> Tested on TI's DRA7xx platform. > > Hi Kishon, > > I have no major objections to this series but I would like to see some > testing done in EP mode (on test platforms other than DRA7XX) to make > sure it does not break anything. > > Please do help Kishon get some testing done. Please test v2 of this series please [1] [1] -> https://www.spinics.net/lists/arm-kernel/msg699787.html Thanks Kishon > > Thanks, > Lorenzo > >> Thanks >> Kishon >> >> Kishon Vijay Abraham I (15): >> PCI: endpoint: Add new pci_epc_ops to get EPC features >> PCI: dwc: Add ->get_features() callback function in dw_pcie_ep_ops >> PCI: designware-plat: Populate ->get_features() dw_pcie_ep_ops >> PCI: pci-dra7xx: Populate ->get_features() dw_pcie_ep_ops >> PCI: rockchip: Populate ->get_features() dw_pcie_ep_ops >> PCI: cadence: Populate ->get_features() cdns_pcie_epc_ops >> PCI: endpoint: Add helper to get first unreserved BAR >> PCI: endpoint: Fix pci_epf_alloc_space to set correct MEM TYPE flags >> PCI: pci-epf-test: Remove setting epf_bar flags in function driver >> PCI: pci-epf-test: Do not allocate next BARs memory if current BAR is >> 64Bit >> PCI: pci-epf-test: Use pci_epc_get_features to get EPC features >> PCI: cadence: Remove pci_epf_linkup from Cadence EP driver >> PCI: rockchip: Remove pci_epf_linkup from Rockchip EP driver >> PCI: designware-plat: Remove setting epc->features in Designware plat >> EP driver >> PCI: endpoint: Remove features member in struct pci_epc >> >> drivers/pci/controller/dwc/pci-dra7xx.c | 13 +++ >> .../pci/controller/dwc/pcie-designware-ep.c | 12 +++ >> .../pci/controller/dwc/pcie-designware-plat.c | 17 +++- >> drivers/pci/controller/dwc/pcie-designware.h | 1 + >> drivers/pci/controller/pcie-cadence-ep.c | 25 +++--- >> drivers/pci/controller/pcie-rockchip-ep.c | 16 +++- >> drivers/pci/endpoint/functions/pci-epf-test.c | 80 ++++++++++--------- >> drivers/pci/endpoint/pci-epc-core.c | 52 ++++++++++++ >> drivers/pci/endpoint/pci-epf-core.c | 3 + >> include/linux/pci-epc.h | 30 +++++-- >> 10 files changed, 185 insertions(+), 64 deletions(-) >> >> -- >> 2.17.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 >
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 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
Hi Lorenzo, The Endpoint controller driver uses features member in 'struct pci_epc' to advertise the list of supported features to the endpoint function driver. There are a few shortcomings with this approach. *) Certain endpoint controllers support fixed size BAR (e.g. TI's AM654 uses Designware configuration with fixed size BAR). The size of each BARs cannot be passed to the endpoint function driver. *) Too many macros for handling EPC features. (EPC_FEATURE_NO_LINKUP_NOTIFIER, EPC_FEATURE_BAR_MASK, EPC_FEATURE_MSIX_AVAILABLE, EPC_FEATURE_SET_BAR, EPC_FEATURE_GET_BAR) *) Endpoint controllers are directly modifying struct pci_epc members. (I have plans to move struct pci_epc to drivers/pci/endpoint so that pci_epc members are referenced only by endpoint core). To overcome the above shortcomings, introduced pci_epc_get_features() API, pci_epc_features structure and a ->get_features() callback. Also added a patch to set BAR flags in pci_epf_alloc_space and remove it from pci-epf-test function driver. Tested on TI's DRA7xx platform. Thanks Kishon Kishon Vijay Abraham I (15): PCI: endpoint: Add new pci_epc_ops to get EPC features PCI: dwc: Add ->get_features() callback function in dw_pcie_ep_ops PCI: designware-plat: Populate ->get_features() dw_pcie_ep_ops PCI: pci-dra7xx: Populate ->get_features() dw_pcie_ep_ops PCI: rockchip: Populate ->get_features() dw_pcie_ep_ops PCI: cadence: Populate ->get_features() cdns_pcie_epc_ops PCI: endpoint: Add helper to get first unreserved BAR PCI: endpoint: Fix pci_epf_alloc_space to set correct MEM TYPE flags PCI: pci-epf-test: Remove setting epf_bar flags in function driver PCI: pci-epf-test: Do not allocate next BARs memory if current BAR is 64Bit PCI: pci-epf-test: Use pci_epc_get_features to get EPC features PCI: cadence: Remove pci_epf_linkup from Cadence EP driver PCI: rockchip: Remove pci_epf_linkup from Rockchip EP driver PCI: designware-plat: Remove setting epc->features in Designware plat EP driver PCI: endpoint: Remove features member in struct pci_epc drivers/pci/controller/dwc/pci-dra7xx.c | 13 +++ .../pci/controller/dwc/pcie-designware-ep.c | 12 +++ .../pci/controller/dwc/pcie-designware-plat.c | 17 +++- drivers/pci/controller/dwc/pcie-designware.h | 1 + drivers/pci/controller/pcie-cadence-ep.c | 25 +++--- drivers/pci/controller/pcie-rockchip-ep.c | 16 +++- drivers/pci/endpoint/functions/pci-epf-test.c | 80 ++++++++++--------- drivers/pci/endpoint/pci-epc-core.c | 52 ++++++++++++ drivers/pci/endpoint/pci-epf-core.c | 3 + include/linux/pci-epc.h | 30 +++++-- 10 files changed, 185 insertions(+), 64 deletions(-) -- 2.17.1