Designware EP support and code clean up
Subsystems (5)
v2 cover Designware EP support and code clean up
The patch set was made against the Bjorn's next branch.
Adds support Designware EP support.
Increases the maximum number of interrupts allowed for Designware IP controller.
Does a code cleanup on Designware driver:
- Replaces magic numbers without a easy meaning by a well known define that helps the human compreension.
- Replaces a division by 2 by a simple right shift rotation of 1 bit.
- Fixes all first letter characters on comments and debug messages to upper case to maintain coherency.
Gustavo Pimentel (9):
bindings: PCI: designware: Example update
PCI: dwc: Add support for endpoint mode
bindings: PCI: designware: Add support for the EP in Designware driver
PCI: Adds device ID for Synopsys Sample Endpoint.
misc: pci_endpoint_test: Add designware EP entry
PCI: dwc: Define maximum number of vectors
PCI: dwc: Replace lower into upper case characters
PCI: dwc: Small computation improvement
PCI: dwc: Replace magic number by defines
.../devicetree/bindings/pci/designware-pcie.txt | 27 +++- drivers/misc/pci_endpoint_test.c | 1 + drivers/pci/dwc/Kconfig | 45 ++++-- drivers/pci/dwc/pcie-designware-ep.c | 20 +-- drivers/pci/dwc/pcie-designware-host.c | 77 +++++----- drivers/pci/dwc/pcie-designware-plat.c | 159 +++++++++++++++++++-- drivers/pci/dwc/pcie-designware.c | 22 +-- drivers/pci/dwc/pcie-designware.h | 1 + drivers/pci/endpoint/functions/pci-epf-test.c | 9 ++ include/linux/pci_ids.h | 1 + 10 files changed, 281 insertions(+), 81 deletions(-)
Signature · 1 line
2.7.4
PATCH v2 1/9 bindings: PCI: designware: Example update
Replaces "ctrlreg" reg-name by "dbi" to be coherent with similar drivers, however it still be compatible with any previous DT that uses the old reg-name.
Replaces the PCIe base address example by a real PCIe base address in use.
---
Changes v1->v2:
- Removed any iATU reference or changes to avoid confusion.
- Add "snps,dw-pcie" compatible string following Kishon's suggestion.
Documentation/devicetree/bindings/pci/designware-pcie.txt | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
Documentation/devicetree/bindings/pci/designware-pcie.txt +7 -7
@@ -1,7 +1,8 @@ * Synopsys DesignWare PCIe interface Required properties: -- compatible: should contain "snps,dw-pcie" to identify the core. +- compatible: + "snps,dw-pcie-rc", "snps,dw-pcie" for RC mode; - reg: Should contain the configuration address space. - reg-names: Must be "config" for the PCIe configuration space. (The old way of getting the configuration address space from "ranges"
@@ -41,11 +42,11 @@ EP mode: Example configuration: - pcie: pcie@dffff000 { - compatible = "snps,dw-pcie"; - reg = <0xdffff000 0x1000>, /* Controller registers */ - <0xd0000000 0x2000>; /* PCI config space */ - reg-names = "ctrlreg", "config"; + pcie: pcie@dfc00000 { + compatible = "snps,dw-pcie-rc", "snps,dw-pcie"; + reg = <0xdfc00000 0x0001000>, /* IP registers */ + <0xd0000000 0x0002000>; /* Configuration space */ + reg-names = "dbi", "config"; #address-cells = <3>; #size-cells = <2>; device_type = "pci";
@@ -54,5 +55,4 @@ Example configuration: interrupts = <25>, <24>; #interrupt-cells = <1>; num-lanes = <1>; - num-viewport = <3>; };
Signature · 1 line
2.7.4
PATCH v2 2/9 PCI: dwc: Add support for endpoint mode
In reply to Gustavo PimentelThe PCIe controller dual mode is capable of operating in host mode as well as endpoint mode by configuration, therefore this patch aims to add endpoint mode support to the designware driver.
---
Change v1->v2:
- Removed dw_plat_pcie_stop_link empty function.
- Implemented Kishon's suggestions about dw-pcie-rc and dw-pcie strings.
compatibility.
- Added second entry on pci_epf_test_ids structure.
drivers/pci/dwc/Kconfig | 45 ++++++-- drivers/pci/dwc/pcie-designware-ep.c | 4 +- drivers/pci/dwc/pcie-designware-plat.c | 153 ++++++++++++++++++++++++-- drivers/pci/endpoint/functions/pci-epf-test.c | 9 ++ 4 files changed, 190 insertions(+), 21 deletions(-)
drivers/pci/dwc/Kconfig +35 -10
@@ -7,8 +7,7 @@ config PCIE_DW config PCIE_DW_HOST bool - depends on PCI - depends on PCI_MSI_IRQ_DOMAIN + depends on PCI && PCI_MSI_IRQ_DOMAIN select PCIE_DW config PCIE_DW_EP
@@ -52,16 +51,42 @@ config PCI_DRA7XX_EP config PCIE_DW_PLAT bool "Platform bus based DesignWare PCIe Controller" - depends on PCI - depends on PCI_MSI_IRQ_DOMAIN - select PCIE_DW_HOST - ---help--- - This selects the DesignWare PCIe controller support. Select this if - you have a PCIe controller on Platform bus. + help + There are two instances of PCIe controller in Designware IP. + This controller can work either as EP or RC. In order to enable + host-specific features PCIE_DW_PLAT_HOST must be selected and in + order to enable device-specific features PCIE_DW_PLAT_EP must be + selected. - If you have a controller with this interface, say Y or M here. +config PCIE_DW_PLAT_HOST + bool "Platform bus based DesignWare PCIe Controller - Host mode" + depends on PCI && PCI_MSI_IRQ_DOMAIN + select PCIE_DW_HOST + select PCIE_DW_PLAT + default y + help + Enables support for the PCIe controller in the Designware IP to + work in host mode. There are two instances of PCIe controller in + Designware IP. + This controller can work either as EP or RC. In order to enable + host-specific features PCIE_DW_PLAT_HOST must be selected and in + order to enable device-specific features PCI_DW_PLAT_EP must be + selected. - If unsure, say N. +config PCIE_DW_PLAT_EP + bool "Platform bus based DesignWare PCIe Controller - Endpoint mode" + depends on PCI && PCI_MSI_IRQ_DOMAIN + depends on PCI_ENDPOINT + select PCIE_DW_EP + select PCIE_DW_PLAT + help + Enables support for the PCIe controller in the Designware IP to + work in endpoint mode. There are two instances of PCIe controller + in Designware IP. + This controller can work either as EP or RC. In order to enable + host-specific features PCIE_DW_PLAT_HOST must be selected and in + order to enable device-specific features PCI_DW_PLAT_EP must be + selected. config PCI_EXYNOS bool "Samsung Exynos PCIe controller"
drivers/pci/dwc/pcie-designware-ep.c +3 -1
@@ -15,8 +15,10 @@void dw_pcie_ep_linkup (struct dw_pcie_ep *ep) {struct pci_epc *epc = ep->epc; +struct pci_epf *epf; -pci_epc_linkup (epc); +list_for_each_entry (epf, &epc->pci_epf, list) +pci_epf_linkup (epf); } static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar,
drivers/pci/dwc/pcie-designware-plat.c +143 -10
215 lines not on this page · raw message
drivers/pci/endpoint/functions/pci-epf-test.c +9 -0
@@ -459,10 +459,19 @@ static int pci_epf_test_bind(struct pci_epf *epf)return 0 ; } +static const struct pci_epf_test_data data_cfg2 = { + .test_reg_bar =BAR_0 , + .linkup_notifier = false +}; +static const struct pci_epf_device_id pci_epf_test_ids[] = { { .name ="pci_epf_test" , }, + { + .name ="pci_epf_test_cfg2" , + .driver_data = (kernel_ulong_t)&data_cfg2, + }, {}, };
Signature · 1 line
2.7.4
Re: [PATCH v2 2/9] PCI: dwc: Add support for endpoint mode
Hi,
Quoted 88 lines · Gustavo Pimentel
The PCIe controller dual mode is capable of operating in host mode as well as endpoint mode by configuration, therefore this patch aims to add endpoint mode support to the designware driver.
Signed-off-by Gustavo Pimentel <[email protected]>---
Changev1->v2:
- Removeddw_plat_pcie_stop_linkempty function.
- Implemented Kishon's suggestions about dw-pcie-rc and dw-pcie strings.
compatibility.
- Added second entry onpci_epf_test_idsstructure.drivers/pci/dwc/Kconfig | 45 ++++++-- drivers/pci/dwc/pcie-designware-ep.c | 4 +- drivers/pci/dwc/pcie-designware-plat.c | 153 ++++++++++++++++++++++++-- drivers/pci/endpoint/functions/pci-epf-test.c | 9 ++ 4 files changed, 190 insertions(+), 21 deletions(-)drivers/pci/dwc/Kconfig +35
-10@@ -7,8 +7,7 @@ config PCIE_DW config PCIE_DW_HOST bool - depends on PCI - depends on PCI_MSI_IRQ_DOMAIN + depends on PCI && PCI_MSI_IRQ_DOMAIN select PCIE_DW config PCIE_DW_EP@@ -52,16 +51,42 @@ config PCI_DRA7XX_EP config PCIE_DW_PLAT bool "Platform bus based DesignWare PCIe Controller" - depends on PCI - depends on PCI_MSI_IRQ_DOMAIN - select PCIE_DW_HOST - ---help--- - This selects the DesignWare PCIe controller support. Select this if - you have a PCIe controller on Platform bus. + help + There are two instances of PCIe controller in Designware IP. + This controller can work either as EP or RC. In order to enable + host-specific features PCIE_DW_PLAT_HOST must be selected and in + order to enable device-specific features PCIE_DW_PLAT_EP must be + selected. - If you have a controller with this interface, say Y or M here. +config PCIE_DW_PLAT_HOST + bool "Platform bus based DesignWare PCIe Controller - Host mode" + depends on PCI && PCI_MSI_IRQ_DOMAIN + select PCIE_DW_HOST + select PCIE_DW_PLAT + default y + help + Enables support for the PCIe controller in the Designware IP to + work in host mode. There are two instances of PCIe controller in + Designware IP. + This controller can work either as EP or RC. In order to enable + host-specific features PCIE_DW_PLAT_HOST must be selected and in + order to enable device-specific features PCI_DW_PLAT_EP must be + selected. - If unsure, say N. +config PCIE_DW_PLAT_EP + bool "Platform bus based DesignWare PCIe Controller - Endpoint mode" + depends on PCI && PCI_MSI_IRQ_DOMAIN + depends on PCI_ENDPOINT + select PCIE_DW_EP + select PCIE_DW_PLAT + help + Enables support for the PCIe controller in the Designware IP to + work in endpoint mode. There are two instances of PCIe controller + in Designware IP. + This controller can work either as EP or RC. In order to enable + host-specific features PCIE_DW_PLAT_HOST must be selected and in + order to enable device-specific features PCI_DW_PLAT_EP must be + selected. config PCI_EXYNOS bool "Samsung Exynos PCIe controller"drivers/pci/dwc/pcie-designware-ep.c +0
-0
drivers/pci/dwc/pcie-designware-ep.c line 22
Quoted 9 lines
drivers/pci/dwc/pcie-designware-ep.c +3
-1@@ -15,8 +15,10 @@void dw_pcie_ep_linkup (struct dw_pcie_ep *ep) {struct pci_epc *epc = ep->epc; +struct pci_epf *epf; -pci_epc_linkup (epc); +list_for_each_entry (epf, &epc->pci_epf, list) +pci_epf_linkup (epf); }
This shouldn't be required anymore.
Quoted 110 lines
drivers/pci/dwc/pcie-designware-ep.c +0
-0static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar,drivers/pci/dwc/pcie-designware-plat.c +61
-5@@ -12,19 +12,29 @@#include <linux/interrupt.h> #include <linux/kernel.h> #include <linux/init.h> +#include <linux/of_device.h> #include <linux/of_gpio.h> #include <linux/pci.h> #include <linux/platform_device.h> #include <linux/resource.h> #include <linux/signal.h> #include <linux/types.h> +#include <linux/regmap.h> #include "pcie-designware.h" struct dw_plat_pcie { -struct dw_pcie *pci; +struct dw_pcie *pci; +struct regmap *regmap; +enum dw_pcie_device_mode mode; }; +struct dw_plat_pcie_of_data { +enum dw_pcie_device_mode mode; +}; + +static const struct of_device_id dw_plat_pcie_of_match[]; +static int dw_plat_pcie_host_init (struct pcie_port *pp) {struct dw_pcie *pci =to_dw_pcie_from_pp (pp);@@ -42,9 +52,53 @@ static const struct dw_pcie_host_ops dw_plat_pcie_host_ops = { .host_init = dw_plat_pcie_host_init, }; -static int dw_plat_add_pcie_port (struct pcie_port *pp, +static int dw_plat_pcie_establish_link (struct dw_pcie *pci) +{ +return 0 ; +} + +static const struct dw_pcie_ops dw_pcie_ops = { + .start_link = dw_plat_pcie_establish_link, +}; + +static void dw_plat_pcie_ep_init (struct dw_pcie_ep *ep) +{ +struct dw_pcie *pci =to_dw_pcie_from_ep (ep); +enum pci_barno bar; + +for (bar =BAR_0 ; bar <=BAR_5 ; bar++) +dw_pcie_ep_reset_bar (pci, bar); +} + +static int dw_plat_pcie_ep_raise_irq (struct dw_pcie_ep *ep,u8 func_no, +enum pci_epc_irq_type type, +u8 interrupt_num) +{ +struct dw_pcie *pci =to_dw_pcie_from_ep (ep); + +switch (type) { +case PCI_EPC_IRQ_LEGACY : +dev_err (pci->dev,"EP cannot trigger legacy IRQs\n" ); +return -EINVAL ; +case PCI_EPC_IRQ_MSI : +return dw_pcie_ep_raise_msi_irq (ep, func_no, interrupt_num); +default : +dev_err (pci->dev,"UNKNOWN IRQ type\n" ); + } + +return 0 ; +} + +static struct dw_pcie_ep_ops pcie_ep_ops = { + .ep_init = dw_plat_pcie_ep_init, + .raise_irq = dw_plat_pcie_ep_raise_irq, +}; + +static int dw_plat_add_pcie_port (struct dw_plat_pcie *dw_plat_pcie,struct platform_device *pdev) { +struct dw_pcie *pci = dw_plat_pcie->pci; +struct pcie_port *pp = &pci->pp;struct device *dev = &pdev->dev;int ret;@@ -63,15 +117,44 @@ static int dw_plat_add_pcie_port(struct pcie_port *pp, ret =dw_pcie_host_init (pp);if (ret) { -dev_err (dev,"failed to initialize host\n" ); +dev_err (dev,"Failed to initialize host\n" );return ret; }return 0 ; } -static const struct dw_pcie_ops dw_pcie_ops = { -}; +static int dw_plat_add_pcie_ep (struct dw_plat_pcie *dw_plat_pcie, +struct platform_device *pdev) +{ +int ret;
drivers/pci/dwc/pcie-designware-plat.c static int dw_plat_add_pcie_port(struct pcie_port *pp, · line 140
Quoted 10 lines
drivers/pci/dwc/pcie-designware-plat.c +10
-0+struct dw_pcie_ep *ep; +struct resource *res; +struct device *dev = &pdev->dev; +struct dw_pcie *pci = dw_plat_pcie->pci; + + ep = &pci->ep; + ep->ops = &pcie_ep_ops; + + res =platform_get_resource_byname (pdev,IORESOURCE_MEM ,"dbi2" ); + pci->dbi_base2 =devm_ioremap (dev, res->start,resource_size (res));
This can throw up a NULL pointer dereferencing error. Use devm_ioremap_resource instead.
drivers/pci/endpoint/functions/pci-epf-test.c
Quoted 110 lines
drivers/pci/dwc/pcie-designware-plat.c +72
-5+if (IS_ERR (pci->dbi_base2)) +return PTR_ERR (pci->dbi_base2); + + res =platform_get_resource_byname (pdev,IORESOURCE_MEM ,"addr_space" ); +if (!res) +return -EINVAL ; + + ep->phys_base = res->start; + ep->addr_size =resource_size (res); + + ret =dw_pcie_ep_init (ep); +if (ret) { +dev_err (dev,"Failed to initialize endpoint\n" ); +return ret; + } +return 0 ; +}static int dw_plat_pcie_probe (struct platform_device *pdev) {@@ -80,6 +163,16 @@ static int dw_plat_pcie_probe(struct platform_device *pdev)struct dw_pcie *pci;struct resource *res;/* Resource from DT */ int ret; +const struct of_device_id *match; +const struct dw_plat_pcie_of_data *data; +enum dw_pcie_device_mode mode; + + match =of_match_device (dw_plat_pcie_of_match, dev); +if (!match) +return -EINVAL ; + + data = (struct dw_plat_pcie_of_data *)match->data; + mode = (enum dw_pcie_device_mode )data->mode; dw_plat_pcie =devm_kzalloc (dev,sizeof (*dw_plat_pcie),GFP_KERNEL );if (!dw_plat_pcie)@@ -93,23 +186,63 @@ static int dw_plat_pcie_probe(struct platform_device *pdev) pci->ops = &dw_pcie_ops; dw_plat_pcie->pci = pci; + dw_plat_pcie->mode = mode; + + res =platform_get_resource_byname (pdev,IORESOURCE_MEM ,"dbi" ); +if (!res) + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); - res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); pci->dbi_base =devm_ioremap_resource (dev, res);if (IS_ERR (pci->dbi_base))return PTR_ERR (pci->dbi_base);platform_set_drvdata (pdev, dw_plat_pcie); - ret =dw_plat_add_pcie_port (&pci->pp, pdev); -if (ret <0 ) -return ret; +switch (dw_plat_pcie->mode) { +case DW_PCIE_RC_TYPE : +if (!IS_ENABLED (CONFIG_PCIE_DW_PLAT_HOST )) +return -ENODEV ; + + ret =dw_plat_add_pcie_port (dw_plat_pcie, pdev); +if (ret <0 ) +return ret; +break ; +case DW_PCIE_EP_TYPE : +if (!IS_ENABLED (CONFIG_PCIE_DW_PLAT_EP )) +return -ENODEV ; + + ret =dw_plat_add_pcie_ep (dw_plat_pcie, pdev); +if (ret <0 ) +return ret; +break ; +default : +dev_err (dev,"INVALID device type %d\n" , dw_plat_pcie->mode); + }return 0 ; } +static const struct dw_plat_pcie_of_data dw_plat_pcie_rc_of_data = { + .mode =DW_PCIE_RC_TYPE , +}; + +static const struct dw_plat_pcie_of_data dw_plat_pcie_ep_of_data = { + .mode =DW_PCIE_EP_TYPE , +}; +static const struct of_device_id dw_plat_pcie_of_match[] = { - { .compatible ="snps,dw-pcie" , }, + { + .compatible ="snps,dw-pcie" , + .data = &dw_plat_pcie_rc_of_data, + }, + { + .compatible ="snps,dw-pcie-rc" , + .data = &dw_plat_pcie_rc_of_data, + }, + { + .compatible ="snps,dw-pcie-ep" , + .data = &dw_plat_pcie_ep_of_data, + }, {}, };drivers/pci/endpoint/functions/pci-epf-test.c +0
-0
have a separate patch for this.
drivers/pci/endpoint/functions/pci-epf-test.c static int pci_epf_test_bind(struct pci_epf *epf) · line 463
Quoted 6 lines
drivers/pci/endpoint/functions/pci-epf-test.c +2
-0@@ -459,10 +459,19 @@ static int pci_epf_test_bind(struct pci_epf *epf)return 0 ; } +static const struct pci_epf_test_data data_cfg2 = { + .test_reg_bar =BAR_0 ,
default test_reg_bar is BAR_0. So you can skip this.
drivers/pci/endpoint/functions/pci-epf-test.c static int pci_epf_test_bind(struct pci_epf *epf) · line 466
Quoted 3 lines
drivers/pci/endpoint/functions/pci-epf-test.c +3
-0+ .linkup_notifier = false +}; +
Thanks
Kishon
Re: [PATCH v2 2/9] PCI: dwc: Add support for endpoint mode
Hi Kishon,
Quoted 96 lines · Kishon Vijay Abraham I
Hi,
Quoted 93 lines · Gustavo Pimentel
The PCIe controller dual mode is capable of operating in host mode as well as endpoint mode by configuration, therefore this patch aims to add endpoint mode support to the designware driver.
Signed-off-by Gustavo Pimentel <[email protected]>---
Changev1->v2:
- Removeddw_plat_pcie_stop_linkempty function.
- Implemented Kishon's suggestions about dw-pcie-rc and dw-pcie strings.
compatibility.
- Added second entry onpci_epf_test_idsstructure.drivers/pci/dwc/Kconfig | 45 ++++++-- drivers/pci/dwc/pcie-designware-ep.c | 4 +- drivers/pci/dwc/pcie-designware-plat.c | 153 ++++++++++++++++++++++++-- drivers/pci/endpoint/functions/pci-epf-test.c | 9 ++ 4 files changed, 190 insertions(+), 21 deletions(-)drivers/pci/dwc/Kconfig +35
-10@@ -7,8 +7,7 @@ config PCIE_DW config PCIE_DW_HOST bool - depends on PCI - depends on PCI_MSI_IRQ_DOMAIN + depends on PCI && PCI_MSI_IRQ_DOMAIN select PCIE_DW config PCIE_DW_EP@@ -52,16 +51,42 @@ config PCI_DRA7XX_EP config PCIE_DW_PLAT bool "Platform bus based DesignWare PCIe Controller" - depends on PCI - depends on PCI_MSI_IRQ_DOMAIN - select PCIE_DW_HOST - ---help--- - This selects the DesignWare PCIe controller support. Select this if - you have a PCIe controller on Platform bus. + help + There are two instances of PCIe controller in Designware IP. + This controller can work either as EP or RC. In order to enable + host-specific features PCIE_DW_PLAT_HOST must be selected and in + order to enable device-specific features PCIE_DW_PLAT_EP must be + selected. - If you have a controller with this interface, say Y or M here. +config PCIE_DW_PLAT_HOST + bool "Platform bus based DesignWare PCIe Controller - Host mode" + depends on PCI && PCI_MSI_IRQ_DOMAIN + select PCIE_DW_HOST + select PCIE_DW_PLAT + default y + help + Enables support for the PCIe controller in the Designware IP to + work in host mode. There are two instances of PCIe controller in + Designware IP. + This controller can work either as EP or RC. In order to enable + host-specific features PCIE_DW_PLAT_HOST must be selected and in + order to enable device-specific features PCI_DW_PLAT_EP must be + selected. - If unsure, say N. +config PCIE_DW_PLAT_EP + bool "Platform bus based DesignWare PCIe Controller - Endpoint mode" + depends on PCI && PCI_MSI_IRQ_DOMAIN + depends on PCI_ENDPOINT + select PCIE_DW_EP + select PCIE_DW_PLAT + help + Enables support for the PCIe controller in the Designware IP to + work in endpoint mode. There are two instances of PCIe controller + in Designware IP. + This controller can work either as EP or RC. In order to enable + host-specific features PCIE_DW_PLAT_HOST must be selected and in + order to enable device-specific features PCI_DW_PLAT_EP must be + selected. config PCI_EXYNOS bool "Samsung Exynos PCIe controller"drivers/pci/dwc/pcie-designware-ep.c +1
-0@@ -15,8 +15,10 @@void dw_pcie_ep_linkup (struct dw_pcie_ep *ep) {struct pci_epc *epc = ep->epc; +struct pci_epf *epf;
Quoted 6 lines
drivers/pci/dwc/pcie-designware-ep.c line 22
Quoted 4 lines
drivers/pci/dwc/pcie-designware-ep.c +2
-1-pci_epc_linkup (epc); +list_for_each_entry (epf, &epc->pci_epf, list) +pci_epf_linkup (epf); }This shouldn't be required anymore.
Ok. I'll revert it.
Quoted 115 lines
Quoted 115 lines
drivers/pci/dwc/pcie-designware-ep.c +0
-0static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar,drivers/pci/dwc/pcie-designware-plat.c +66
-5@@ -12,19 +12,29 @@#include <linux/interrupt.h> #include <linux/kernel.h> #include <linux/init.h> +#include <linux/of_device.h> #include <linux/of_gpio.h> #include <linux/pci.h> #include <linux/platform_device.h> #include <linux/resource.h> #include <linux/signal.h> #include <linux/types.h> +#include <linux/regmap.h> #include "pcie-designware.h" struct dw_plat_pcie { -struct dw_pcie *pci; +struct dw_pcie *pci; +struct regmap *regmap; +enum dw_pcie_device_mode mode; }; +struct dw_plat_pcie_of_data { +enum dw_pcie_device_mode mode; +}; + +static const struct of_device_id dw_plat_pcie_of_match[]; +static int dw_plat_pcie_host_init (struct pcie_port *pp) {struct dw_pcie *pci =to_dw_pcie_from_pp (pp);@@ -42,9 +52,53 @@ static const struct dw_pcie_host_ops dw_plat_pcie_host_ops = { .host_init = dw_plat_pcie_host_init, }; -static int dw_plat_add_pcie_port (struct pcie_port *pp, +static int dw_plat_pcie_establish_link (struct dw_pcie *pci) +{ +return 0 ; +} + +static const struct dw_pcie_ops dw_pcie_ops = { + .start_link = dw_plat_pcie_establish_link, +}; + +static void dw_plat_pcie_ep_init (struct dw_pcie_ep *ep) +{ +struct dw_pcie *pci =to_dw_pcie_from_ep (ep); +enum pci_barno bar; + +for (bar =BAR_0 ; bar <=BAR_5 ; bar++) +dw_pcie_ep_reset_bar (pci, bar); +} + +static int dw_plat_pcie_ep_raise_irq (struct dw_pcie_ep *ep,u8 func_no, +enum pci_epc_irq_type type, +u8 interrupt_num) +{ +struct dw_pcie *pci =to_dw_pcie_from_ep (ep); + +switch (type) { +case PCI_EPC_IRQ_LEGACY : +dev_err (pci->dev,"EP cannot trigger legacy IRQs\n" ); +return -EINVAL ; +case PCI_EPC_IRQ_MSI : +return dw_pcie_ep_raise_msi_irq (ep, func_no, interrupt_num); +default : +dev_err (pci->dev,"UNKNOWN IRQ type\n" ); + } + +return 0 ; +} + +static struct dw_pcie_ep_ops pcie_ep_ops = { + .ep_init = dw_plat_pcie_ep_init, + .raise_irq = dw_plat_pcie_ep_raise_irq, +}; + +static int dw_plat_add_pcie_port (struct dw_plat_pcie *dw_plat_pcie,struct platform_device *pdev) { +struct dw_pcie *pci = dw_plat_pcie->pci; +struct pcie_port *pp = &pci->pp;struct device *dev = &pdev->dev;int ret;@@ -63,15 +117,44 @@ static int dw_plat_add_pcie_port(struct pcie_port *pp, ret =dw_pcie_host_init (pp);if (ret) { -dev_err (dev,"failed to initialize host\n" ); +dev_err (dev,"Failed to initialize host\n" );return ret; }return 0 ; } -static const struct dw_pcie_ops dw_pcie_ops = { -}; +static int dw_plat_add_pcie_ep (struct dw_plat_pcie *dw_plat_pcie, +struct platform_device *pdev) +{ +int ret; +struct dw_pcie_ep *ep; +struct resource *res; +struct device *dev = &pdev->dev; +struct dw_pcie *pci = dw_plat_pcie->pci; +
Quoted 8 lines
drivers/pci/dwc/pcie-designware-plat.c static int dw_plat_add_pcie_port(struct pcie_port *pp, · line 140
Quoted 5 lines
drivers/pci/dwc/pcie-designware-plat.c +5
-0+ ep = &pci->ep; + ep->ops = &pcie_ep_ops; + + res =platform_get_resource_byname (pdev,IORESOURCE_MEM ,"dbi2" ); + pci->dbi_base2 =devm_ioremap (dev, res->start,resource_size (res));This can throw up a NULL pointer dereferencing error. Use
devm_ioremap_resourceinstead.
I have looked on [1] and [2] and both implementations uses devm_ioremap function, they are also wrong?
[1] -> https://elixir.bootlin.com/linux/latest/source/drivers/pci/dwc/pcie-artpec6.c
[2] -> https://elixir.bootlin.com/linux/latest/source/drivers/pci/dwc/pci-dra7xx.c
Quoted 3 lines
drivers/pci/dwc/pcie-designware-plat.c static int dw_plat_add_pcie_port(struct pcie_port *pp, · line 142
Quoted 3 lines
drivers/pci/dwc/pcie-designware-plat.c +2
-0+if (IS_ERR (pci->dbi_base2)) +return PTR_ERR (pci->dbi_base2);
However this validation is wrong. By using the devm_ioremap function the correct validation should be like this:
if (!pci->dbi_base2)
return -ENOMEM;
like exists on [1] and [2]. Can I maintain the coherency with [1] and [2]?
Quoted 104 lines
Quoted 104 lines
drivers/pci/dwc/pcie-designware-plat.c +70
-5+ + res =platform_get_resource_byname (pdev,IORESOURCE_MEM ,"addr_space" ); +if (!res) +return -EINVAL ; + + ep->phys_base = res->start; + ep->addr_size =resource_size (res); + + ret =dw_pcie_ep_init (ep); +if (ret) { +dev_err (dev,"Failed to initialize endpoint\n" ); +return ret; + } +return 0 ; +}static int dw_plat_pcie_probe (struct platform_device *pdev) {@@ -80,6 +163,16 @@ static int dw_plat_pcie_probe(struct platform_device *pdev)struct dw_pcie *pci;struct resource *res;/* Resource from DT */ int ret; +const struct of_device_id *match; +const struct dw_plat_pcie_of_data *data; +enum dw_pcie_device_mode mode; + + match =of_match_device (dw_plat_pcie_of_match, dev); +if (!match) +return -EINVAL ; + + data = (struct dw_plat_pcie_of_data *)match->data; + mode = (enum dw_pcie_device_mode )data->mode; dw_plat_pcie =devm_kzalloc (dev,sizeof (*dw_plat_pcie),GFP_KERNEL );if (!dw_plat_pcie)@@ -93,23 +186,63 @@ static int dw_plat_pcie_probe(struct platform_device *pdev) pci->ops = &dw_pcie_ops; dw_plat_pcie->pci = pci; + dw_plat_pcie->mode = mode; + + res =platform_get_resource_byname (pdev,IORESOURCE_MEM ,"dbi" ); +if (!res) + res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); - res =platform_get_resource (pdev,IORESOURCE_MEM ,0 ); pci->dbi_base =devm_ioremap_resource (dev, res);if (IS_ERR (pci->dbi_base))return PTR_ERR (pci->dbi_base);platform_set_drvdata (pdev, dw_plat_pcie); - ret =dw_plat_add_pcie_port (&pci->pp, pdev); -if (ret <0 ) -return ret; +switch (dw_plat_pcie->mode) { +case DW_PCIE_RC_TYPE : +if (!IS_ENABLED (CONFIG_PCIE_DW_PLAT_HOST )) +return -ENODEV ; + + ret =dw_plat_add_pcie_port (dw_plat_pcie, pdev); +if (ret <0 ) +return ret; +break ; +case DW_PCIE_EP_TYPE : +if (!IS_ENABLED (CONFIG_PCIE_DW_PLAT_EP )) +return -ENODEV ; + + ret =dw_plat_add_pcie_ep (dw_plat_pcie, pdev); +if (ret <0 ) +return ret; +break ; +default : +dev_err (dev,"INVALID device type %d\n" , dw_plat_pcie->mode); + }return 0 ; } +static const struct dw_plat_pcie_of_data dw_plat_pcie_rc_of_data = { + .mode =DW_PCIE_RC_TYPE , +}; + +static const struct dw_plat_pcie_of_data dw_plat_pcie_ep_of_data = { + .mode =DW_PCIE_EP_TYPE , +}; +static const struct of_device_id dw_plat_pcie_of_match[] = { - { .compatible ="snps,dw-pcie" , }, + { + .compatible ="snps,dw-pcie" , + .data = &dw_plat_pcie_rc_of_data, + }, + { + .compatible ="snps,dw-pcie-rc" , + .data = &dw_plat_pcie_rc_of_data, + }, + { + .compatible ="snps,dw-pcie-ep" , + .data = &dw_plat_pcie_ep_of_data, + }, {}, };
Quoted 6 lines
drivers/pci/endpoint/functions/pci-epf-test.c
Quoted 4 lines
drivers/pci/endpoint/functions/pci-epf-test.c +0
-0have a separate patch for this.
Ok.
Quoted 8 lines
drivers/pci/endpoint/functions/pci-epf-test.c static int pci_epf_test_bind(struct pci_epf *epf) · line 463
Quoted 6 lines
drivers/pci/endpoint/functions/pci-epf-test.c +2
-0@@ -459,10 +459,19 @@ static int pci_epf_test_bind(struct pci_epf *epf)return 0 ; } +static const struct pci_epf_test_data data_cfg2 = { + .test_reg_bar =BAR_0 ,default
test_reg_barisBAR_0. So you can skip this.
Ok, I'll remove it.
Quoted 7 lines
drivers/pci/endpoint/functions/pci-epf-test.c static int pci_epf_test_bind(struct pci_epf *epf) · line 466
Quoted 3 lines
drivers/pci/endpoint/functions/pci-epf-test.c +3
-0+ .linkup_notifier = false +}; +Thanks
Kishon
Regards,
Gustavo
Re: [PATCH v2 2/9] PCI: dwc: Add support for endpoint mode
Hi,
Quoted 232 lines · Gustavo Pimentel
Hi Kishon,
Quoted 96 lines · Kishon Vijay Abraham I
Hi,
Quoted 93 lines · Gustavo Pimentel
The PCIe controller dual mode is capable of operating in host mode as well as endpoint mode by configuration, therefore this patch aims to add endpoint mode support to the designware driver.
Signed-off-by Gustavo Pimentel <[email protected]>---
Changev1->v2:
- Removeddw_plat_pcie_stop_linkempty function.
- Implemented Kishon's suggestions about dw-pcie-rc and dw-pcie strings.
compatibility.
- Added second entry onpci_epf_test_idsstructure.drivers/pci/dwc/Kconfig | 45 ++++++-- drivers/pci/dwc/pcie-designware-ep.c | 4 +- drivers/pci/dwc/pcie-designware-plat.c | 153 ++++++++++++++++++++++++-- drivers/pci/endpoint/functions/pci-epf-test.c | 9 ++ 4 files changed, 190 insertions(+), 21 deletions(-)drivers/pci/dwc/Kconfig +35
-10@@ -7,8 +7,7 @@ config PCIE_DW config PCIE_DW_HOST bool - depends on PCI - depends on PCI_MSI_IRQ_DOMAIN + depends on PCI && PCI_MSI_IRQ_DOMAIN select PCIE_DW config PCIE_DW_EP@@ -52,16 +51,42 @@ config PCI_DRA7XX_EP config PCIE_DW_PLAT bool "Platform bus based DesignWare PCIe Controller" - depends on PCI - depends on PCI_MSI_IRQ_DOMAIN - select PCIE_DW_HOST - ---help--- - This selects the DesignWare PCIe controller support. Select this if - you have a PCIe controller on Platform bus. + help + There are two instances of PCIe controller in Designware IP. + This controller can work either as EP or RC. In order to enable + host-specific features PCIE_DW_PLAT_HOST must be selected and in + order to enable device-specific features PCIE_DW_PLAT_EP must be + selected. - If you have a controller with this interface, say Y or M here. +config PCIE_DW_PLAT_HOST + bool "Platform bus based DesignWare PCIe Controller - Host mode" + depends on PCI && PCI_MSI_IRQ_DOMAIN + select PCIE_DW_HOST + select PCIE_DW_PLAT + default y + help + Enables support for the PCIe controller in the Designware IP to + work in host mode. There are two instances of PCIe controller in + Designware IP. + This controller can work either as EP or RC. In order to enable + host-specific features PCIE_DW_PLAT_HOST must be selected and in + order to enable device-specific features PCI_DW_PLAT_EP must be + selected. - If unsure, say N. +config PCIE_DW_PLAT_EP + bool "Platform bus based DesignWare PCIe Controller - Endpoint mode" + depends on PCI && PCI_MSI_IRQ_DOMAIN + depends on PCI_ENDPOINT + select PCIE_DW_EP + select PCIE_DW_PLAT + help + Enables support for the PCIe controller in the Designware IP to + work in endpoint mode. There are two instances of PCIe controller + in Designware IP. + This controller can work either as EP or RC. In order to enable + host-specific features PCIE_DW_PLAT_HOST must be selected and in + order to enable device-specific features PCI_DW_PLAT_EP must be + selected. config PCI_EXYNOS bool "Samsung Exynos PCIe controller"drivers/pci/dwc/pcie-designware-ep.c +1
-0@@ -15,8 +15,10 @@void dw_pcie_ep_linkup (struct dw_pcie_ep *ep) {struct pci_epc *epc = ep->epc; +struct pci_epf *epf;Quoted 6 lines
drivers/pci/dwc/pcie-designware-ep.c line 22
Quoted 4 lines
drivers/pci/dwc/pcie-designware-ep.c +2
-1-pci_epc_linkup (epc); +list_for_each_entry (epf, &epc->pci_epf, list) +pci_epf_linkup (epf); }This shouldn't be required anymore.
Ok. I'll revert it.
Quoted 115 lines
Quoted 115 lines
drivers/pci/dwc/pcie-designware-ep.c +0
-0static void __dw_pcie_ep_reset_bar(struct dw_pcie *pci, enum pci_barno bar,drivers/pci/dwc/pcie-designware-plat.c +66
-5@@ -12,19 +12,29 @@#include <linux/interrupt.h> #include <linux/kernel.h> #include <linux/init.h> +#include <linux/of_device.h> #include <linux/of_gpio.h> #include <linux/pci.h> #include <linux/platform_device.h> #include <linux/resource.h> #include <linux/signal.h> #include <linux/types.h> +#include <linux/regmap.h> #include "pcie-designware.h" struct dw_plat_pcie { -struct dw_pcie *pci; +struct dw_pcie *pci; +struct regmap *regmap; +enum dw_pcie_device_mode mode; }; +struct dw_plat_pcie_of_data { +enum dw_pcie_device_mode mode; +}; + +static const struct of_device_id dw_plat_pcie_of_match[]; +static int dw_plat_pcie_host_init (struct pcie_port *pp) {struct dw_pcie *pci =to_dw_pcie_from_pp (pp);@@ -42,9 +52,53 @@ static const struct dw_pcie_host_ops dw_plat_pcie_host_ops = { .host_init = dw_plat_pcie_host_init, }; -static int dw_plat_add_pcie_port (struct pcie_port *pp, +static int dw_plat_pcie_establish_link (struct dw_pcie *pci) +{ +return 0 ; +} + +static const struct dw_pcie_ops dw_pcie_ops = { + .start_link = dw_plat_pcie_establish_link, +}; + +static void dw_plat_pcie_ep_init (struct dw_pcie_ep *ep) +{ +struct dw_pcie *pci =to_dw_pcie_from_ep (ep); +enum pci_barno bar; + +for (bar =BAR_0 ; bar <=BAR_5 ; bar++) +dw_pcie_ep_reset_bar (pci, bar); +} + +static int dw_plat_pcie_ep_raise_irq (struct dw_pcie_ep *ep,u8 func_no, +enum pci_epc_irq_type type, +u8 interrupt_num) +{ +struct dw_pcie *pci =to_dw_pcie_from_ep (ep); + +switch (type) { +case PCI_EPC_IRQ_LEGACY : +dev_err (pci->dev,"EP cannot trigger legacy IRQs\n" ); +return -EINVAL ; +case PCI_EPC_IRQ_MSI : +return dw_pcie_ep_raise_msi_irq (ep, func_no, interrupt_num); +default : +dev_err (pci->dev,"UNKNOWN IRQ type\n" ); + } + +return 0 ; +} + +static struct dw_pcie_ep_ops pcie_ep_ops = { + .ep_init = dw_plat_pcie_ep_init, + .raise_irq = dw_plat_pcie_ep_raise_irq, +}; + +static int dw_plat_add_pcie_port (struct dw_plat_pcie *dw_plat_pcie,struct platform_device *pdev) { +struct dw_pcie *pci = dw_plat_pcie->pci; +struct pcie_port *pp = &pci->pp;struct device *dev = &pdev->dev;int ret;@@ -63,15 +117,44 @@ static int dw_plat_add_pcie_port(struct pcie_port *pp, ret =dw_pcie_host_init (pp);if (ret) { -dev_err (dev,"failed to initialize host\n" ); +dev_err (dev,"Failed to initialize host\n" );return ret; }return 0 ; } -static const struct dw_pcie_ops dw_pcie_ops = { -}; +static int dw_plat_add_pcie_ep (struct dw_plat_pcie *dw_plat_pcie, +struct platform_device *pdev) +{ +int ret; +struct dw_pcie_ep *ep; +struct resource *res; +struct device *dev = &pdev->dev; +struct dw_pcie *pci = dw_plat_pcie->pci; +Quoted 8 lines
drivers/pci/dwc/pcie-designware-plat.c static int dw_plat_add_pcie_port(struct pcie_port *pp, · line 140
Quoted 5 lines
drivers/pci/dwc/pcie-designware-plat.c +5
-0+ ep = &pci->ep; + ep->ops = &pcie_ep_ops; + + res =platform_get_resource_byname (pdev,IORESOURCE_MEM ,"dbi2" ); + pci->dbi_base2 =devm_ioremap (dev, res->start,resource_size (res));This can throw up a NULL pointer dereferencing error. Use
devm_ioremap_resourceinstead.
Quoted 5 lines
I have looked on [1] and [2] and both implementations uses
devm_ioremapfunction, they are also wrong?[1] -> https://elixir.bootlin.com/linux/latest/source/drivers/pci/dwc/pcie-artpec6.c
[2] -> https://elixir.bootlin.com/linux/latest/source/drivers/pci/dwc/pci-dra7xx.c
yes, they are wrong. There was a patch to fix it but somehow dropped
https://patchwork.kernel.org/patch/10173831/.
I'll resend them.
Thanks
Kishon
PATCH v2 3/9 bindings: PCI: designware: Add support for the EP in Designware driver
In reply to Gustavo PimentelAdd device tree binding documentation for the Endpoint in PCIe Designware driver.
---
Change v1->v2:
- Add a missing log description.
- Add "snps,dw-pcie" compatible string following Kishon's suggestion.
Documentation/devicetree/bindings/pci/designware-pcie.txt | 13 +++++++++++++ 1 file changed, 13 insertions(+)
Documentation/devicetree/bindings/pci/designware-pcie.txt +13 -0
@@ -3,6 +3,7 @@ Required properties: - compatible: "snps,dw-pcie-rc", "snps,dw-pcie" for RC mode; + "snps,dw-pcie-ep", "snps,dw-pcie" for EP mode; - reg: Should contain the configuration address space. - reg-names: Must be "config" for the PCIe configuration space. (The old way of getting the configuration address space from "ranges"
@@ -56,3 +57,15 @@ Example configuration: #interrupt-cells = <1>; num-lanes = <1>; }; +or + pcie_ep: pcie_ep@dfc00000 { + compatible = "snps,dw-pcie-ep", "snps,dw_pcie"; + reg = <0xdfc00000 0x0001000>, /* IP registers 1 */ + <0xdfc01000 0x0001000>, /* IP registers 2 */ + <0xd0000000 0x2000000>; /* Configuration space */ + reg-names = "dbi", "dbi2", "addr_space"; + device_type = "pci"; + num-ib-windows = <6>; + num-ob-windows = <2>; + num-lanes = <1>; + };
Signature · 1 line
2.7.4
PATCH v2 4/9 PCI: Adds device ID for Synopsys Sample Endpoint.
In reply to Gustavo PimentelThe PCIe controller dual mode is capable of operating in host mode as well as endpoint mode by configuration.
---
Change v1->v2:
- Register new device id following Kishon's suggestion.
include/linux/pci_ids.h | 1 + 1 file changed, 1 insertion(+)
include/linux/pci_ids.h +1 -0
@@ -2349,6 +2349,7 @@#define PCI_DEVICE_ID_CENATEK_IDE 0x0001#define PCI_VENDOR_ID_SYNOPSYS 0x16c3 +#define PCI_DEVICE_ID_SYNOPSYS_SMPL_EP 0xedda#define PCI_VENDOR_ID_VITESSE 0x1725#define PCI_DEVICE_ID_VITESSE_VSC7174 0x7174
Signature · 1 line
2.7.4
PATCH v2 5/9 misc: pci_endpoint_test: Add designware EP entry
In reply to Gustavo PimentelAdds the designware EP device ID entry to pci_endpoint_test driver table to allow this device to be recognize and handle by the pci_endpoint_test driver.
---
Change v1->v2:
- Changed device id following Kishon's suggestion.
drivers/misc/pci_endpoint_test.c | 1 + 1 file changed, 1 insertion(+)
drivers/misc/pci_endpoint_test.c +1 -0
@@ -634,6 +634,7 @@ static void pci_endpoint_test_remove(struct pci_dev *pdev)static const struct pci_device_id pci_endpoint_test_tbl[] = { {PCI_DEVICE (PCI_VENDOR_ID_TI , PCI_DEVICE_ID_TI_DRA74x) }, {PCI_DEVICE (PCI_VENDOR_ID_TI , PCI_DEVICE_ID_TI_DRA72x) }, + {PCI_DEVICE (PCI_VENDOR_ID_SYNOPSYS ,PCI_DEVICE_ID_SYNOPSYS_SMPL_EP ) }, { } };MODULE_DEVICE_TABLE (pci, pci_endpoint_test_tbl);
Signature · 1 line
2.7.4
PATCH v2 6/9 PCI: dwc: Define maximum number of vectors
In reply to Gustavo PimentelAdds a callback that defines the maximum number of vectors that can be use by the Root Complex.
Since this is a parameter associated to each SoC IP setting, makes sense to be configurable and easily visible to future modifications.
The designware IP supports a maximum of 256 vectors.
---
Change v1->v2:
- Nothing changed, just to follow the patch set version.
drivers/pci/dwc/pcie-designware-plat.c | 6 ++++++ 1 file changed, 6 insertions(+)
drivers/pci/dwc/pcie-designware-plat.c +6 -0
@@ -48,8 +48,14 @@ static int dw_plat_pcie_host_init(struct pcie_port *pp)return 0 ; } +static void dw_plat_set_num_vectors (struct pcie_port *pp) +{ + pp->num_vectors =MAX_MSI_IRQS ; +} +static const struct dw_pcie_host_ops dw_plat_pcie_host_ops = { .host_init = dw_plat_pcie_host_init, + .set_num_vectors = dw_plat_set_num_vectors, };static int dw_plat_pcie_establish_link (struct dw_pcie *pci)
Signature · 1 line
2.7.4
Re: [PATCH v2 6/9] PCI: dwc: Define maximum number of vectors
Quoted 7 lines · Gustavo Pimentel
Adds a callback that defines the maximum number of vectors that can be use by the Root Complex.
Since this is a parameter associated to each SoC IP setting, makes sense to be configurable and easily visible to future modifications.
The designware IP supports a maximum of 256 vectors.
I think that a DT property instead of a callback would have made more sense - I struggle to see the point in defining a callback to initialize a variable, this can be done in the generic dwc code (and a DT binding).
Lorenzo
Quoted 31 lines
Signed-off-by Gustavo Pimentel <[email protected]>---
Changev1->v2:
- Nothing changed, just to follow the patch set version.drivers/pci/dwc/pcie-designware-plat.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/pci/dwc/pcie-designware-plat.c +6
-0@@ -48,8 +48,14 @@ static int dw_plat_pcie_host_init(struct pcie_port *pp)return 0 ; } +static void dw_plat_set_num_vectors (struct pcie_port *pp) +{ + pp->num_vectors =MAX_MSI_IRQS ; +} +static const struct dw_pcie_host_ops dw_plat_pcie_host_ops = { .host_init = dw_plat_pcie_host_init, + .set_num_vectors = dw_plat_set_num_vectors, };static int dw_plat_pcie_establish_link (struct dw_pcie *pci)Signature · 1 line
2.7.4
Re: [PATCH v2 6/9] PCI: dwc: Define maximum number of vectors
Hi Lorenzo,
Quoted 4 lines · Lorenzo Pieralisi
Quoted 3 lines · Gustavo Pimentel
Adds a callback that defines the maximum number of vectors that can be use by the Root Complex.
Quoted 8 lines
Quoted 4 lines
Since this is a parameter associated to each SoC IP setting, makes sense to be configurable and easily visible to future modifications.
The designware IP supports a maximum of 256 vectors.
I think that a DT property instead of a callback would have made more sense - I struggle to see the point in defining a callback to initialize a variable, this can be done in the generic dwc code (and a DT binding).
The addition of this callback was done in MSI-X patch series before I take over the PCIe Designware driver responsibility. However I remember a thread in which this subject was discussed (see [1]), maybe this could bring some light about the motive why is was done like this. If you don't agree I can do patch after this series only focusing on this topic in order to do like to suggested.
[1] -> https://www.spinics.net/lists/linux-pci/msg61835.html
Quoted 29 lines
Lorenzo
Quoted 26 lines
Signed-off-by Gustavo Pimentel <[email protected]>---
Changev1->v2:
- Nothing changed, just to follow the patch set version.drivers/pci/dwc/pcie-designware-plat.c | 6 ++++++ 1 file changed, 6 insertions(+)drivers/pci/dwc/pcie-designware-plat.c +6
-0@@ -48,8 +48,14 @@ static int dw_plat_pcie_host_init(struct pcie_port *pp)return 0 ; } +static void dw_plat_set_num_vectors (struct pcie_port *pp) +{ + pp->num_vectors =MAX_MSI_IRQS ; +} +static const struct dw_pcie_host_ops dw_plat_pcie_host_ops = { .host_init = dw_plat_pcie_host_init, + .set_num_vectors = dw_plat_set_num_vectors, };
Quoted 5 lines
Quoted 5 lines
drivers/pci/dwc/pcie-designware-plat.c +0
-0static int dw_plat_pcie_establish_link (struct dw_pcie *pci)Signature · 1 line
2.7.4
Regards,
Gustavo
Re: [PATCH v2 6/9] PCI: dwc: Define maximum number of vectors
Quoted 16 lines · Gustavo Pimentel
Hi Lorenzo,
Quoted 4 lines · Lorenzo Pieralisi
Quoted 3 lines · Gustavo Pimentel
Adds a callback that defines the maximum number of vectors that can be use by the Root Complex.
Quoted 8 lines
Quoted 4 lines
Since this is a parameter associated to each SoC IP setting, makes sense to be configurable and easily visible to future modifications.
The designware IP supports a maximum of 256 vectors.
I think that a DT property instead of a callback would have made more sense - I struggle to see the point in defining a callback to initialize a variable, this can be done in the generic dwc code (and a DT binding).
Quoted 7 lines
The addition of this callback was done in MSI-X patch series before I take over the PCIe Designware driver responsibility. However I remember a thread in which this subject was discussed (see [1]), maybe this could bring some light about the motive why is was done like this. If you don't agree I can do patch after this series only focusing on this topic in order to do like to suggested.
[1] -> https://www.spinics.net/lists/linux-pci/msg61835.html
Lucas has a point - it is fine to handle them as you do in this patch, it does not make much sense to add a property for something that strictly depends on the compatible string.
Thanks,
Lorenzo
PATCH v2 7/9 PCI: dwc: Replace lower into upper case characters
In reply to Gustavo PimentelReplaces lower into upper case characters in comments and debug printks.
This is an attempt to keep the messages coherent within the designware driver.
Also fixed code style on dw_pcie_irq_domain_free function.
---
Change v1->v2:
- Added an extra log description line about code style following Fabio Estevam suggestion.
drivers/pci/dwc/pcie-designware-ep.c | 16 ++++++++-------- drivers/pci/dwc/pcie-designware-host.c | 35 ++++++++++++++++++---------------- drivers/pci/dwc/pcie-designware.c | 22 ++++++++++----------- 3 files changed, 38 insertions(+), 35 deletions(-)
drivers/pci/dwc/pcie-designware-ep.c +8 -8
@@ -77,7 +77,7 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, enum pci_barno bar, free_win =find_first_zero_bit (ep->ib_window_map, ep->num_ib_windows);if (free_win >= ep->num_ib_windows) { -dev_err (pci->dev,"no free inbound window\n" ); +dev_err (pci->dev,"No free inbound window\n" );return -EINVAL ; }
@@ -102,7 +102,7 @@ static int dw_pcie_ep_outbound_atu(struct dw_pcie_ep *ep, phys_addr_t phys_addr, free_win =find_first_zero_bit (ep->ob_window_map, ep->num_ob_windows);if (free_win >= ep->num_ob_windows) { -dev_err (pci->dev,"no free outbound window\n" ); +dev_err (pci->dev,"No free outbound window\n" );return -EINVAL ; }
@@ -206,7 +206,7 @@ static int dw_pcie_ep_map_addr(struct pci_epc *epc, u8 func_no, ret =dw_pcie_ep_outbound_atu (ep, addr, pci_addr, size);if (ret) { -dev_err (pci->dev,"failed to enable address\n" ); +dev_err (pci->dev,"Failed to enable address\n" );return ret; }
@@ -350,21 +350,21 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) ret =of_property_read_u32 (np,"num-ib-windows" , &ep->num_ib_windows);if (ret <0 ) { -dev_err (dev,"unable to read *num-ib-windows* property\n" ); +dev_err (dev,"Unable to read *num-ib-windows* property\n" );return ret; }if (ep->num_ib_windows >MAX_IATU_IN ) { -dev_err (dev,"invalid *num-ib-windows*\n" ); +dev_err (dev,"Invalid *num-ib-windows*\n" );return -EINVAL ; } ret =of_property_read_u32 (np,"num-ob-windows" , &ep->num_ob_windows);if (ret <0 ) { -dev_err (dev,"unable to read *num-ob-windows* property\n" ); +dev_err (dev,"Unable to read *num-ob-windows* property\n" );return ret; }if (ep->num_ob_windows >MAX_IATU_OUT ) { -dev_err (dev,"invalid *num-ob-windows*\n" ); +dev_err (dev,"Invalid *num-ob-windows*\n" );return -EINVAL ; }
@@ -391,7 +391,7 @@ int dw_pcie_ep_init(struct dw_pcie_ep *ep) epc =devm_pci_epc_create (dev, &epc_ops);if (IS_ERR (epc)) { -dev_err (dev,"failed to create epc device\n" ); +dev_err (dev,"Failed to create epc device\n" );return PTR_ERR (epc); }
drivers/pci/dwc/pcie-designware-host.c +19 -16
@@ -248,8 +248,10 @@ static void dw_pcie_irq_domain_free(struct irq_domain *domain,unsigned long flags;raw_spin_lock_irqsave (&pp->lock, flags); +bitmap_release_region (pp->msi_irq_in_use, data->hwirq,order_base_2 (nr_irqs)); +raw_spin_unlock_irqrestore (&pp->lock, flags); }
@@ -266,7 +268,7 @@ int dw_pcie_allocate_domains(struct pcie_port *pp) pp->irq_domain =irq_domain_create_linear (fwnode, pp->num_vectors, &dw_pcie_msi_domain_ops, pp);if (!pp->irq_domain) { -dev_err (pci->dev,"failed to create IRQ domain\n" ); +dev_err (pci->dev,"Failed to create IRQ domain\n" );return -ENOMEM ; }
@@ -274,7 +276,7 @@ int dw_pcie_allocate_domains(struct pcie_port *pp) &dw_pcie_msi_domain_info, pp->irq_domain);if (!pp->msi_domain) { -dev_err (pci->dev,"failed to create MSI domain\n" ); +dev_err (pci->dev,"Failed to create MSI domain\n" );irq_domain_remove (pp->irq_domain);return -ENOMEM ; }
@@ -301,13 +303,13 @@ void dw_pcie_msi_init(struct pcie_port *pp) page =alloc_page (GFP_KERNEL ); pp->msi_data =dma_map_page (dev, page,0 ,PAGE_SIZE ,DMA_FROM_DEVICE );if (dma_mapping_error (dev, pp->msi_data)) { -dev_err (dev,"failed to map MSI data\n" ); +dev_err (dev,"Failed to map MSI data\n" );__free_page (page);return ; } msi_target = (u64 )pp->msi_data; -/* program the msi_data */ +/* Program the msi_data */ dw_pcie_wr_own_conf (pp,PCIE_MSI_ADDR_LO ,4 ,lower_32_bits (msi_target)); dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_HI, 4,
@@ -335,7 +337,7 @@ int dw_pcie_host_init(struct pcie_port *pp) pp->cfg0_base = cfg_res->start; pp->cfg1_base = cfg_res->start + pp->cfg0_size; }else if (!pp->va_cfg0_base) { -dev_err (dev,"missing *config* reg space\n" ); +dev_err (dev,"Missing *config* reg space\n" ); } bridge =pci_alloc_host_bridge (0 );
@@ -357,7 +359,7 @@ int dw_pcie_host_init(struct pcie_port *pp)case IORESOURCE_IO : ret =pci_remap_iospace (win->res, pp->io_base);if (ret) { -dev_warn (dev,"error %d: failed to map resource %pR\n" , +dev_warn (dev,"Error %d: failed to map resource %pR\n" , ret, win->res);resource_list_destroy_entry (win); }else {
@@ -391,7 +393,7 @@ int dw_pcie_host_init(struct pcie_port *pp) pp->cfg->start, resource_size(pp->cfg));if (!pci->dbi_base) { -dev_err (dev,"error with ioremap\n" ); +dev_err (dev,"Error with ioremap\n" ); ret = -ENOMEM ; goto error; }
@@ -403,7 +405,7 @@ int dw_pcie_host_init(struct pcie_port *pp) pp->va_cfg0_base =devm_pci_remap_cfgspace (dev, pp->cfg0_base, pp->cfg0_size);if (!pp->va_cfg0_base) { -dev_err (dev,"error with ioremap in function\n" ); +dev_err (dev,"Error with ioremap in function\n" ); ret = -ENOMEM ; goto error; }
@@ -414,7 +416,7 @@ int dw_pcie_host_init(struct pcie_port *pp) pp->cfg1_base, pp->cfg1_size);if (!pp->va_cfg1_base) { -dev_err (dev,"error with ioremap\n" ); +dev_err (dev,"Error with ioremap\n" ); ret = -ENOMEM ; goto error; }
@@ -586,7 +588,7 @@ static int dw_pcie_valid_device(struct pcie_port *pp, struct pci_bus *bus,return 0 ; } -/* access only one slot on each root port */ +/* Access only one slot on each root port */ if (bus->number == pp->root_bus_nr && dev >0 )return 0 ;
@@ -652,11 +654,12 @@ void dw_pcie_setup_rc(struct pcie_port *pp)for (ctrl =0 ; ctrl < num_ctrls; ctrl++)dw_pcie_rd_own_conf (pp,PCIE_MSI_INTR0_ENABLE + (ctrl *12 ),4 , &pp->irq_status[ctrl]); -/* setup RC BARs */ + +/* Setup RC BARs */ dw_pcie_writel_dbi (pci,PCI_BASE_ADDRESS_0 ,0x00000004 );dw_pcie_writel_dbi (pci,PCI_BASE_ADDRESS_1 ,0x00000000 ); -/* setup interrupt pins */ +/* Setup interrupt pins */ dw_pcie_dbi_ro_wr_en (pci); val =dw_pcie_readl_dbi (pci,PCI_INTERRUPT_LINE ); val &=0xffff00ff ;
@@ -664,13 +667,13 @@ void dw_pcie_setup_rc(struct pcie_port *pp)dw_pcie_writel_dbi (pci,PCI_INTERRUPT_LINE , val);dw_pcie_dbi_ro_wr_dis (pci); -/* setup bus numbers */ +/* Setup bus numbers */ val =dw_pcie_readl_dbi (pci,PCI_PRIMARY_BUS ); val &=0xff000000 ; val |=0x00010100 ;dw_pcie_writel_dbi (pci,PCI_PRIMARY_BUS , val); -/* setup command register */ +/* Setup command register */ val =dw_pcie_readl_dbi (pci,PCI_COMMAND ); val &=0xffff0000 ; val |= PCI_COMMAND_IO | PCI_COMMAND_MEMORY |
@@ -683,7 +686,7 @@ void dw_pcie_setup_rc(struct pcie_port *pp)* we should not program the ATU here. */ if (!pp->ops->rd_other_conf) { -/* get iATU unroll support */ +/* Get iATU unroll support */ pci->iatu_unroll_enabled =dw_pcie_iatu_unroll_enabled (pci);dev_dbg (pci->dev,"iATU unroll: %s\n" , pci->iatu_unroll_enabled ?"enabled" :"disabled" );
@@ -701,7 +704,7 @@ void dw_pcie_setup_rc(struct pcie_port *pp)/* Enable write permission for the DBI read-only register */ dw_pcie_dbi_ro_wr_en (pci); -/* program correct class for RC */ +/* Program correct class for RC */ dw_pcie_wr_own_conf (pp,PCI_CLASS_DEVICE ,2 ,PCI_CLASS_BRIDGE_PCI );/* Better disable write permission right after the update */ dw_pcie_dbi_ro_wr_dis (pci);
drivers/pci/dwc/pcie-designware.c +11 -11
@@ -69,7 +69,7 @@ u32 __dw_pcie_read_dbi(struct dw_pcie *pci, void __iomem *base, u32 reg, ret =dw_pcie_read (base + reg, size, &val);if (ret) -dev_err (pci->dev,"read DBI address failed\n" ); +dev_err (pci->dev,"Read DBI address failed\n" );return val; }
@@ -86,7 +86,7 @@ void __dw_pcie_write_dbi(struct dw_pcie *pci, void __iomem *base, u32 reg, ret =dw_pcie_write (base + reg, size, val);if (ret) -dev_err (pci->dev,"write DBI address failed\n" ); +dev_err (pci->dev,"Write DBI address failed\n" ); }static u32 dw_pcie_readl_ob_unroll (struct dw_pcie *pci,u32 index,u32 reg)
@@ -137,7 +137,7 @@ static void dw_pcie_prog_outbound_atu_unroll(struct dw_pcie *pci, int index,usleep_range (LINK_WAIT_IATU_MIN ,LINK_WAIT_IATU_MAX ); } -dev_err (pci->dev,"outbound iATU is not being enabled\n" ); +dev_err (pci->dev,"Outbound iATU is not being enabled\n" ); } void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
@@ -180,7 +180,7 @@ void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,usleep_range (LINK_WAIT_IATU_MIN ,LINK_WAIT_IATU_MAX ); } -dev_err (pci->dev,"outbound iATU is not being enabled\n" ); +dev_err (pci->dev,"Outbound iATU is not being enabled\n" ); }static u32 dw_pcie_readl_ib_unroll (struct dw_pcie *pci,u32 index,u32 reg)
@@ -238,7 +238,7 @@ static int dw_pcie_prog_inbound_atu_unroll(struct dw_pcie *pci, int index,usleep_range (LINK_WAIT_IATU_MIN ,LINK_WAIT_IATU_MAX ); } -dev_err (pci->dev,"inbound iATU is not being enabled\n" ); +dev_err (pci->dev,"Inbound iATU is not being enabled\n" );return -EBUSY ; }
@@ -284,7 +284,7 @@ int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int bar,usleep_range (LINK_WAIT_IATU_MIN ,LINK_WAIT_IATU_MAX ); } -dev_err (pci->dev,"inbound iATU is not being enabled\n" ); +dev_err (pci->dev,"Inbound iATU is not being enabled\n" );return -EBUSY ; }
@@ -313,16 +313,16 @@ int dw_pcie_wait_for_link(struct dw_pcie *pci) {int retries; -/* check if the link is up or not */ +/* Check if the link is up or not */ for (retries =0 ; retries <LINK_WAIT_MAX_RETRIES ; retries++) {if (dw_pcie_link_up (pci)) { -dev_info (pci->dev,"link up\n" ); +dev_info (pci->dev,"Link up\n" );return 0 ; }usleep_range (LINK_WAIT_USLEEP_MIN ,LINK_WAIT_USLEEP_MAX ); } -dev_err (pci->dev,"phy link never came up\n" ); +dev_err (pci->dev,"Phy link never came up\n" );return -ETIMEDOUT ; }
@@ -351,7 +351,7 @@ void dw_pcie_setup(struct dw_pcie *pci)if (ret) lanes =0 ; -/* set the number of lanes */ +/* Set the number of lanes */ val =dw_pcie_readl_dbi (pci,PCIE_PORT_LINK_CONTROL ); val &= ~PORT_LINK_MODE_MASK ;switch (lanes) {
@@ -373,7 +373,7 @@ void dw_pcie_setup(struct dw_pcie *pci) }dw_pcie_writel_dbi (pci,PCIE_PORT_LINK_CONTROL , val); -/* set link width speed control register */ +/* Set link width speed control register */ val =dw_pcie_readl_dbi (pci,PCIE_LINK_WIDTH_SPEED_CONTROL ); val &= ~PORT_LOGIC_LINK_WIDTH_MASK ;switch (lanes) {
Signature · 1 line
2.7.4
Re: [PATCH v2 7/9] PCI: dwc: Replace lower into upper case characters
Quoted 4 lines · Gustavo Pimentel
Replaces lower into upper case characters in comments and debug printks.
This is an attempt to keep the messages coherent within the designware driver.
It'd be nice to make all the dwc drivers use the same message wording.
For instance:
drivers/pci/dwc/pci-keystone.c: dev_err(dev, "phy link never came up\n");drivers/pci/dwc/pcie-designware.c: dev_err(pci->dev, "phy link never came up\n");drivers/pci/host/pcie-xilinx-nwl.c: dev_err(dev, "PHY link never came up\n");
Re: [PATCH v2 7/9] PCI: dwc: Replace lower into upper case characters
Hi Joe,
Quoted 6 lines · Joe Perches
Quoted 4 lines · Gustavo Pimentel
Replaces lower into upper case characters in comments and debug printks.
This is an attempt to keep the messages coherent within the designware driver.
Quoted 8 lines
It'd be nice to make all the dwc drivers use the same message wording.
For instance:
drivers/pci/dwc/pci-keystone.c:dev_err(dev, "phy link never came up\n");drivers/pci/dwc/pcie-designware.c:dev_err(pci->dev, "phy link never came up\n");drivers/pci/host/pcie-xilinx-nwl.c:dev_err(dev, "PHY link never came up\n");
I also agree, I added a task to my backlog to fix that. However, this task shouldn't be done in this patch series since it's a more broader task involving many drivers and different maintainers. Agree?
Quoted 1 line
Regards,
Gustavo
Re: [PATCH v2 7/9] PCI: dwc: Replace lower into upper case characters
Quoted 1 line · Gustavo Pimentel
Hi Joe,
Hello Gustavo.
Quoted 12 lines · Joe Perches
Quoted 11 lines · Gustavo Pimentel
Quoted 4 lines
Replaces lower into upper case characters in comments and debug printks.
This is an attempt to keep the messages coherent within the designware driver.
It'd be nice to make all the dwc drivers use the same message wording.
For instance:
Quoted 7 lines
Quoted 3 lines
drivers/pci/dwc/pci-keystone.c:dev_err(dev, "phy link never came up\n");drivers/pci/dwc/pcie-designware.c:dev_err(pci->dev, "phy link never came up\n");drivers/pci/host/pcie-xilinx-nwl.c:dev_err(dev, "PHY link never came up\n");I also agree, I added a task to my backlog to fix that. However, this task shouldn't be done in this patch series since it's a more broader task involving many drivers and different maintainers. Agree?
Right.
I was just highlighting the inconsistency.
So, no worries. Whenever.
Re: [PATCH v2 7/9] PCI: dwc: Replace lower into upper case characters
In reply to Gustavo PimentelQuoted 9 lines · Gustavo Pimentel
Replaces lower into upper case characters in comments and debug printks.
This is an attempt to keep the messages coherent within the designware driver.
Also fixed code style on
dw_pcie_irq_domain_freefunction.Signed-off-by Gustavo Pimentel <[email protected]>
Quoted 11 lines
---
Changev1->v2:
- Added an extra log description line about code style following Fabio Estevam suggestion.drivers/pci/dwc/pcie-designware-ep.c | 16 ++++++++-------- drivers/pci/dwc/pcie-designware-host.c | 35 ++++++++++++++++++----------- ----- drivers/pci/dwc/pcie-designware.c | 22 ++++++++++----------- 3 files changed, 38 insertions(+), 35 deletions(-)
[.....]
PATCH v2 8/9 PCI: dwc: Small computation improvement
In reply to Gustavo PimentelReplaces a simple division by 2 to a right shift rotation of 1 bit.
---
Change v1->v2:
- Nothing changed, just to follow the patch set version.
drivers/pci/dwc/pcie-designware-host.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/pci/dwc/pcie-designware-host.c +4 -4
@@ -332,8 +332,8 @@ int dw_pcie_host_init(struct pcie_port *pp) cfg_res =platform_get_resource_byname (pdev,IORESOURCE_MEM ,"config" );if (cfg_res) { - pp->cfg0_size =resource_size (cfg_res) /2 ; - pp->cfg1_size =resource_size (cfg_res) /2 ; + pp->cfg0_size =resource_size (cfg_res) >>1 ; + pp->cfg1_size =resource_size (cfg_res) >>1 ; pp->cfg0_base = cfg_res->start; pp->cfg1_base = cfg_res->start + pp->cfg0_size; }else if (!pp->va_cfg0_base) {
@@ -377,8 +377,8 @@ int dw_pcie_host_init(struct pcie_port *pp)break ;case 0 : pp->cfg = win->res; - pp->cfg0_size =resource_size (pp->cfg) /2 ; - pp->cfg1_size =resource_size (pp->cfg) /2 ; + pp->cfg0_size =resource_size (pp->cfg) >>1 ; + pp->cfg1_size =resource_size (pp->cfg) >>1 ; pp->cfg0_base = pp->cfg->start; pp->cfg1_base = pp->cfg->start + pp->cfg0_size;break ;
Signature · 1 line
2.7.4
Re: [PATCH v2 8/9] PCI: dwc: Small computation improvement
Quoted 2 lines · Gustavo Pimentel
Replaces a simple division by 2 to a right shift rotation of 1 bit.
It looks good. However, would you add a simple reason to the commit message?
Best regards,
Jingoo Han
Quoted 40 lines
Signed-off-by Gustavo Pimentel <[email protected]>---
Changev1->v2:
- Nothing changed, just to follow the patch set version.drivers/pci/dwc/pcie-designware-host.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)drivers/pci/dwc/pcie-designware-host.c +2
-2@@ -332,8 +332,8 @@ int dw_pcie_host_init(struct pcie_port *pp) cfg_res =platform_get_resource_byname (pdev,IORESOURCE_MEM ,"config" );if (cfg_res) { - pp->cfg0_size =resource_size (cfg_res) /2 ; - pp->cfg1_size =resource_size (cfg_res) /2 ; + pp->cfg0_size =resource_size (cfg_res) >>1 ; + pp->cfg1_size =resource_size (cfg_res) >>1 ; pp->cfg0_base = cfg_res->start; pp->cfg1_base = cfg_res->start + pp->cfg0_size;drivers/pci/dwc/pcie-designware-host.c +2
-3}else if (!pp->va_cfg0_base) {@@ -377,8 +377,8 @@ int dw_pcie_host_init(struct pcie_port *pp)break ;case 0 : pp->cfg = win->res; - pp->cfg0_size =resource_size (pp->cfg) /2 ; - pp->cfg1_size =resource_size (pp->cfg) /2 ; + pp->cfg0_size =resource_size (pp->cfg) >>1 ; + pp->cfg1_size =resource_size (pp->cfg) >>1 ; pp->cfg0_base = pp->cfg->start; pp->cfg1_base = pp->cfg->start + pp->cfg0_size;break ; --2.7.4
Re: [PATCH v2 8/9] PCI: dwc: Small computation improvement
Hi Jingoo,
Quoted 6 lines · Jingoo Han
Quoted 2 lines · Gustavo Pimentel
Replaces a simple division by 2 to a right shift rotation of 1 bit.
It looks good. However, would you add a simple reason to the commit message?
Sure.
Can be this one?
Probably any recent and decent compiler does this kind of substitution in order to improve code performance. Nevertheless it's a coding good practice whenever there is a division / multiplication by multiple of 2 to replace it by the equivalent operation in this case, the shift rotation.
Quoted 36 lines
Best regards,
Jingoo HanQuoted 32 lines
Signed-off-by Gustavo Pimentel <[email protected]>---
Changev1->v2:
- Nothing changed, just to follow the patch set version.drivers/pci/dwc/pcie-designware-host.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)drivers/pci/dwc/pcie-designware-host.c +2
-2@@ -332,8 +332,8 @@ int dw_pcie_host_init(struct pcie_port *pp) cfg_res =platform_get_resource_byname (pdev,IORESOURCE_MEM ,"config" );if (cfg_res) { - pp->cfg0_size =resource_size (cfg_res) /2 ; - pp->cfg1_size =resource_size (cfg_res) /2 ; + pp->cfg0_size =resource_size (cfg_res) >>1 ; + pp->cfg1_size =resource_size (cfg_res) >>1 ; pp->cfg0_base = cfg_res->start; pp->cfg1_base = cfg_res->start + pp->cfg0_size;drivers/pci/dwc/pcie-designware-host.c +0
-2}else if (!pp->va_cfg0_base) {@@ -377,8 +377,8 @@ int dw_pcie_host_init(struct pcie_port *pp)break ;case 0 : pp->cfg = win->res; - pp->cfg0_size =resource_size (pp->cfg) /2 ; - pp->cfg1_size =resource_size (pp->cfg) /2 ;
Quoted 10 lines
Quoted 8 lines
drivers/pci/dwc/pcie-designware-host.c +2
-1+ pp->cfg0_size =resource_size (pp->cfg) >>1 ; + pp->cfg1_size =resource_size (pp->cfg) >>1 ; pp->cfg0_base = pp->cfg->start; pp->cfg1_base = pp->cfg->start + pp->cfg0_size;break ; --2.7.4
Regards,
Gustavo
Re: [PATCH v2 8/9] PCI: dwc: Small computation improvement
Quoted 13 lines · Gustavo Pimentel
Hi Jingoo,
Quoted 6 lines · Jingoo Han
Quoted 2 lines · Gustavo Pimentel
Replaces a simple division by 2 to a right shift rotation of 1 bit.
It looks good. However, would you add a simple reason to the commit message?
Sure.
Quoted 7 lines
Can be this one?
Probably any recent and decent compiler does this kind of substitution in order to improve code performance. Nevertheless it's a coding good practice whenever there is a division / multiplication by multiple of 2 to replace it by the equivalent operation in this case, the shift rotation.
Yes, that's what I wanted. The most platforms using 'dwc' are based on ARM CPUs. So, the shift rotation can be better.
Thank you.
Best regards,
Jingoo Han
Quoted 50 lines
Quoted 37 lines
Best regards,
Jingoo HanQuoted 32 lines
Signed-off-by Gustavo Pimentel <[email protected]>---
Changev1->v2:
- Nothing changed, just to follow the patch set version.drivers/pci/dwc/pcie-designware-host.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)drivers/pci/dwc/pcie-designware-host.c +2
-2@@ -332,8 +332,8 @@ int dw_pcie_host_init(struct pcie_port *pp) cfg_res =platform_get_resource_byname (pdev,IORESOURCE_MEM ,"config" );if (cfg_res) { - pp->cfg0_size =resource_size (cfg_res) /2 ; - pp->cfg1_size =resource_size (cfg_res) /2 ; + pp->cfg0_size =resource_size (cfg_res) >>1 ; + pp->cfg1_size =resource_size (cfg_res) >>1 ; pp->cfg0_base = cfg_res->start; pp->cfg1_base = cfg_res->start + pp->cfg0_size;drivers/pci/dwc/pcie-designware-host.c +0
-2}else if (!pp->va_cfg0_base) {@@ -377,8 +377,8 @@ int dw_pcie_host_init(struct pcie_port *pp)break ;case 0 : pp->cfg = win->res; - pp->cfg0_size =resource_size (pp->cfg) /2 ; - pp->cfg1_size =resource_size (pp->cfg) /2 ;Quoted 10 lines
Quoted 8 lines
drivers/pci/dwc/pcie-designware-host.c +2
-1+ pp->cfg0_size =resource_size (pp->cfg) >>1 ; + pp->cfg1_size =resource_size (pp->cfg) >>1 ; pp->cfg0_base = pp->cfg->start; pp->cfg1_base = pp->cfg->start + pp->cfg0_size;break ; --2.7.4
Regards,
Gustavo
Re: [PATCH v2 8/9] PCI: dwc: Small computation improvement
Hi Jingoo,
Quoted 22 lines · Jingoo Han
Quoted 13 lines · Gustavo Pimentel
Hi Jingoo,
Quoted 6 lines · Jingoo Han
Quoted 2 lines · Gustavo Pimentel
Replaces a simple division by 2 to a right shift rotation of 1 bit.
It looks good. However, would you add a simple reason to the commit message?
Sure.
Quoted 7 lines
Can be this one?
Probably any recent and decent compiler does this kind of substitution in order to improve code performance. Nevertheless it's a coding good practice whenever there is a division / multiplication by multiple of 2 to replace it by the equivalent operation in this case, the shift rotation.
Quoted 3 lines
Yes, that's what I wanted. The most platforms using 'dwc' are based on ARM CPUs. So, the shift rotation can be better.
Thank you.
Can I get your acked-by on this?
Quoted 56 lines
Best regards,
Jingoo HanQuoted 50 lines
Quoted 37 lines
Best regards,
Jingoo HanQuoted 32 lines
Signed-off-by Gustavo Pimentel <[email protected]>---
Changev1->v2:
- Nothing changed, just to follow the patch set version.drivers/pci/dwc/pcie-designware-host.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)drivers/pci/dwc/pcie-designware-host.c +2
-2@@ -332,8 +332,8 @@ int dw_pcie_host_init(struct pcie_port *pp) cfg_res =platform_get_resource_byname (pdev,IORESOURCE_MEM ,"config" );if (cfg_res) { - pp->cfg0_size =resource_size (cfg_res) /2 ; - pp->cfg1_size =resource_size (cfg_res) /2 ; + pp->cfg0_size =resource_size (cfg_res) >>1 ; + pp->cfg1_size =resource_size (cfg_res) >>1 ; pp->cfg0_base = cfg_res->start; pp->cfg1_base = cfg_res->start + pp->cfg0_size;drivers/pci/dwc/pcie-designware-host.c +0
-2}else if (!pp->va_cfg0_base) {@@ -377,8 +377,8 @@ int dw_pcie_host_init(struct pcie_port *pp)break ;case 0 : pp->cfg = win->res; - pp->cfg0_size =resource_size (pp->cfg) /2 ; - pp->cfg1_size =resource_size (pp->cfg) /2 ;Quoted 10 lines
Quoted 8 lines
drivers/pci/dwc/pcie-designware-host.c +2
-1+ pp->cfg0_size =resource_size (pp->cfg) >>1 ; + pp->cfg1_size =resource_size (pp->cfg) >>1 ; pp->cfg0_base = pp->cfg->start; pp->cfg1_base = pp->cfg->start + pp->cfg0_size;break ; --2.7.4
Regards,
Gustavo
Regards,
Gustavo
Re: [PATCH v2 8/9] PCI: dwc: Small computation improvement
In reply to Gustavo PimentelHi Gustavo,
Quoted 7 lines · Gustavo Pimentel
Can be this one?
Probably any recent and decent compiler does this kind of substitution in order to improve code performance. Nevertheless it's a coding good practice whenever there is a division / multiplication by multiple of 2 to replace it by the equivalent operation in this case, the shift rotation.
Subject says that this patch provides a computation improvement, but as you said above the compiler
will perform the shift, so it doesn't seem we will get any benefit.
IMHO the original code has better readability as it makes easier to understand that pp->cfg0_size will get the half of resource_size(pp->cfg) size.
I would say it is better to drop this patch from the series.
Thanks
Re: [PATCH v2 8/9] PCI: dwc: Small computation improvement
Hi Fabio,
Quoted 13 lines · Fabio Estevam
Hi Gustavo,
Quoted 7 lines · Gustavo Pimentel
Can be this one?
Probably any recent and decent compiler does this kind of substitution in order to improve code performance. Nevertheless it's a coding good practice whenever there is a division / multiplication by multiple of 2 to replace it by the equivalent operation in this case, the shift rotation.
Quoted 3 lines
Subject says that this patch provides a computation improvement, but as you said above the compiler
will perform the shift, so it doesn't seem we will get any benefit.
In the worth case there is no performance, so it will not hurt having it. However depending the compiler and the platform this can bring some improvement. For instance like Jingoo said: "The most platforms using 'dwc' are based on ARM CPUs. So, the shift rotation can be better."
It's just a attempt to improve the code that doesn't bring any harm.
Quoted 4 lines
IMHO the original code has better readability as it makes easier to understand that
pp->cfg0_sizewill get the half ofresource_size(pp->cfg)size.
Personally I prefer the shift rotation rather the division by 2. But in my case I'm used to use/see this type of operation, but it's like I said it's my personal opinion.
Quoted 2 lines
I would say it is better to drop this patch from the series.
Let's see this patch inflicts pain on someone else, in that case I'll remove from the series.
Quoted 3 lines
Thanks
Regards,
Gustavo
PATCH v2 9/9 PCI: dwc: Replace magic number by defines
In reply to Gustavo PimentelReplace magic numbers by a well known define in order to make the code human readable and also facilitate the code reusability.
---
Change v1->v2:
- Nothing changed, just to follow the patch set version.
drivers/pci/dwc/pcie-designware-host.c | 34 ++++++++++++++++++++-------------- drivers/pci/dwc/pcie-designware.h | 1 + 2 files changed, 21 insertions(+), 14 deletions(-)
drivers/pci/dwc/pcie-designware-host.c +20 -14
@@ -83,18 +83,23 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp) num_ctrls = pp->num_vectors /MAX_MSI_IRQS_PER_CTRL ;for (i =0 ; i < num_ctrls; i++) { -dw_pcie_rd_own_conf (pp,PCIE_MSI_INTR0_STATUS + i *12 ,4 , - &val); +dw_pcie_rd_own_conf (pp,PCIE_MSI_INTR0_STATUS + + (i *MSI_REG_CTRL_BLOCK_SIZE ), +4 , &val);if (!val)continue ; ret =IRQ_HANDLED ; pos =0 ; -while ((pos =find_next_bit ((unsigned long *) &val,32 , - pos)) !=32 ) { - irq =irq_find_mapping (pp->irq_domain, i *32 + pos); +while ((pos =find_next_bit ((unsigned long *) &val, +MAX_MSI_IRQS_PER_CTRL , + pos)) !=MAX_MSI_IRQS_PER_CTRL ) { + irq =irq_find_mapping (pp->irq_domain, + (i *MAX_MSI_IRQS_PER_CTRL ) + + pos);generic_handle_irq (irq); -dw_pcie_wr_own_conf (pp,PCIE_MSI_INTR0_STATUS + i *12 , +dw_pcie_wr_own_conf (pp,PCIE_MSI_INTR0_STATUS + + (i *MSI_REG_CTRL_BLOCK_SIZE ),4 ,1 << pos); pos++; }
@@ -157,9 +162,9 @@ static void dw_pci_bottom_mask(struct irq_data *data)if (pp->ops->msi_clear_irq) { pp->ops->msi_clear_irq (pp, data->hwirq); }else { - ctrl = data->hwirq /32 ; - res = ctrl *12 ; - bit = data->hwirq %32 ; + ctrl = data->hwirq /MAX_MSI_IRQS_PER_CTRL ; + res = ctrl *MSI_REG_CTRL_BLOCK_SIZE ; + bit = data->hwirq %MAX_MSI_IRQS_PER_CTRL ; pp->irq_status[ctrl] &= ~(1 << bit); dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4,
@@ -180,9 +185,9 @@ static void dw_pci_bottom_unmask(struct irq_data *data)if (pp->ops->msi_set_irq) { pp->ops->msi_set_irq (pp, data->hwirq); }else { - ctrl = data->hwirq /32 ; - res = ctrl *12 ; - bit = data->hwirq %32 ; + ctrl = data->hwirq /MAX_MSI_IRQS_PER_CTRL ; + res = ctrl *MSI_REG_CTRL_BLOCK_SIZE ; + bit = data->hwirq %MAX_MSI_IRQS_PER_CTRL ; pp->irq_status[ctrl] |=1 << bit; dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4,
@@ -652,8 +657,9 @@ void dw_pcie_setup_rc(struct pcie_port *pp)/* Initialize IRQ Status array */ for (ctrl =0 ; ctrl < num_ctrls; ctrl++) -dw_pcie_rd_own_conf (pp,PCIE_MSI_INTR0_ENABLE + (ctrl *12 ),4 , - &pp->irq_status[ctrl]); +dw_pcie_rd_own_conf (pp,PCIE_MSI_INTR0_ENABLE + + (ctrl *MSI_REG_CTRL_BLOCK_SIZE ), +4 , &pp->irq_status[ctrl]);/* Setup RC BARs */ dw_pcie_writel_dbi (pci,PCI_BASE_ADDRESS_0 ,0x00000004 );
drivers/pci/dwc/pcie-designware.h +1 -0
@@ -110,6 +110,7 @@#define MAX_MSI_IRQS 256#define MAX_MSI_IRQS_PER_CTRL 32#define MAX_MSI_CTRLS (MAX_MSI_IRQS / MAX_MSI_IRQS_PER_CTRL) +#define MSI_REG_CTRL_BLOCK_SIZE 12#define MSI_DEF_NUM_VECTORS 32/* Maximum number of inbound/outbound iATUs */
Signature · 1 line
2.7.4
Re: [PATCH v2 9/9] PCI: dwc: Replace magic number by defines
Quoted 5 lines · Gustavo Pimentel
Replace magic numbers by a well known define in order to make the code human readable and also facilitate the code reusability.
Signed-off-by Gustavo Pimentel <[email protected]>
Quoted 9 lines
---
Changev1->v2:
- Nothing changed, just to follow the patch set version.drivers/pci/dwc/pcie-designware-host.c | 34 ++++++++++++++++++++--------- ----- drivers/pci/dwc/pcie-designware.h | 1 + 2 files changed, 21 insertions(+), 14 deletions(-)
[.....]