| Message ID | 20171030124221.20690-10-niklas.cassel@axis.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | dwc MSI fixes, ARTPEC-6 EP mode support, ARTPEC-7 SoC support | expand |
Hi, On Monday 30 October 2017 06:12 PM, Niklas Cassel wrote: > This way you will not build and include unused code > when only building for only one mode. > > Moved dra7xx_pcie_enable_wrapper_interrupts() in order > to avoid adding an extra ifdef block. > > Signed-off-by: Niklas Cassel <niklas.cassel@axis.com> > --- > V2: > * New patch in this series. > > drivers/pci/dwc/pci-dra7xx.c | 36 ++++++++++++++++++++++++++++-------- > 1 file changed, 28 insertions(+), 8 deletions(-) > > diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c > index 009f6aeeee1c..175544d6c3ab 100644 > --- a/drivers/pci/dwc/pci-dra7xx.c > +++ b/drivers/pci/dwc/pci-dra7xx.c > @@ -171,6 +171,15 @@ static int dra7xx_pcie_establish_link(struct dw_pcie *pci) > return 0; > } > > +static void dra7xx_pcie_enable_wrapper_interrupts(struct dra7xx_pcie *dra7xx) > +{ > + dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN, > + INTERRUPTS); > + dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MAIN, > + INTERRUPTS); > +} > + > +#ifdef CONFIG_PCI_DRA7XX_HOST > static void dra7xx_pcie_enable_msi_interrupts(struct dra7xx_pcie *dra7xx) > { > dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI, > @@ -181,14 +190,6 @@ static void dra7xx_pcie_enable_msi_interrupts(struct dra7xx_pcie *dra7xx) > MSI | LEG_EP_INTERRUPTS); > } > > -static void dra7xx_pcie_enable_wrapper_interrupts(struct dra7xx_pcie *dra7xx) > -{ > - dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN, > - INTERRUPTS); > - dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MAIN, > - INTERRUPTS); > -} > - > static void dra7xx_pcie_enable_interrupts(struct dra7xx_pcie *dra7xx) > { > dra7xx_pcie_enable_wrapper_interrupts(dra7xx); > @@ -276,6 +277,7 @@ static irqreturn_t dra7xx_pcie_msi_irq_handler(int irq, void *arg) > > return IRQ_HANDLED; > } > +#endif > > static irqreturn_t dra7xx_pcie_irq_handler(int irq, void *arg) > { > @@ -336,6 +338,7 @@ static irqreturn_t dra7xx_pcie_irq_handler(int irq, void *arg) > return IRQ_HANDLED; > } > > +#ifdef CONFIG_PCI_DRA7XX_EP > static void dra7xx_pcie_ep_init(struct dw_pcie_ep *ep) > { > struct dw_pcie *pci = to_dw_pcie_from_ep(ep); > @@ -427,7 +430,9 @@ static int __init dra7xx_add_pcie_ep(struct dra7xx_pcie *dra7xx, > > return 0; > } > +#endif > > +#ifdef CONFIG_PCI_DRA7XX_HOST This block can also be moved around the file so that there is a single +#ifdef CONFIG_PCI_DRA7XX_HOST in this file. > static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx, > struct platform_device *pdev) > { > @@ -470,6 +475,7 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx, > > return 0; > } > +#endif > > static const struct dw_pcie_ops dw_pcie_ops = { > .cpu_addr_fixup = dra7xx_pcie_cpu_addr_fixup, > @@ -517,23 +523,31 @@ static int dra7xx_pcie_enable_phy(struct dra7xx_pcie *dra7xx) > return ret; > } IMO all the #ifdef's after this point can be removed. > > +#ifdef CONFIG_PCI_DRA7XX_HOST > static const struct dra7xx_pcie_of_data dra7xx_pcie_rc_of_data = { > .mode = DW_PCIE_RC_TYPE, > }; > +#endif > > +#ifdef CONFIG_PCI_DRA7XX_EP > static const struct dra7xx_pcie_of_data dra7xx_pcie_ep_of_data = { > .mode = DW_PCIE_EP_TYPE, > }; > +#endif > > static const struct of_device_id of_dra7xx_pcie_match[] = { > +#ifdef CONFIG_PCI_DRA7XX_HOST > { > .compatible = "ti,dra7-pcie", > .data = &dra7xx_pcie_rc_of_data, > }, > +#endif > +#ifdef CONFIG_PCI_DRA7XX_EP > { > .compatible = "ti,dra7-pcie-ep", > .data = &dra7xx_pcie_ep_of_data, > }, > +#endif > {}, > }; > > @@ -548,6 +562,7 @@ static const struct of_device_id of_dra7xx_pcie_match[] = { > * > * To avoid this issue set PCIE_SS1_AXI2OCP_LEGACY_MODE_ENABLE to 1. > */ > +#ifdef CONFIG_PCI_DRA7XX_EP > static int dra7xx_pcie_ep_unaligned_memaccess(struct device *dev) > { > int ret; > @@ -578,6 +593,7 @@ static int dra7xx_pcie_ep_unaligned_memaccess(struct device *dev) > > return ret; > } > +#endif > > static int __init dra7xx_pcie_probe(struct platform_device *pdev) > { > @@ -681,6 +697,7 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev) > dra7xx->link_gen = 2; > > switch (mode) { > +#ifdef CONFIG_PCI_DRA7XX_HOST > case DW_PCIE_RC_TYPE: > dra7xx_pcie_writel(dra7xx, PCIECTRL_TI_CONF_DEVICE_TYPE, > DEVICE_TYPE_RC); > @@ -688,6 +705,8 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev) > if (ret < 0) > goto err_gpio; > break; > +#endif > +#ifdef CONFIG_PCI_DRA7XX_EP > case DW_PCIE_EP_TYPE: > dra7xx_pcie_writel(dra7xx, PCIECTRL_TI_CONF_DEVICE_TYPE, > DEVICE_TYPE_EP); > @@ -700,6 +719,7 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev) > if (ret < 0) > goto err_gpio; > break; > +#endif > default: > dev_err(dev, "INVALID device type %d\n", mode); > } > Thanks Kishon
On 10/31/2017 09:29 AM, Kishon Vijay Abraham I wrote: (snip) >> >> +#ifdef CONFIG_PCI_DRA7XX_HOST > > This block can also be moved around the file so that there is a single +#ifdef > CONFIG_PCI_DRA7XX_HOST in this file. Hello Kishon, Sure, will fix in V3. >> static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx, >> struct platform_device *pdev) >> { >> @@ -470,6 +475,7 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx, >> >> return 0; >> } >> +#endif >> >> static const struct dw_pcie_ops dw_pcie_ops = { >> .cpu_addr_fixup = dra7xx_pcie_cpu_addr_fixup, >> @@ -517,23 +523,31 @@ static int dra7xx_pcie_enable_phy(struct dra7xx_pcie *dra7xx) >> return ret; >> } > > IMO all the #ifdef's after this point can be removed. I will remove the ifdefs in the match table and the ifdefs surrounding the of match table data. >> >> +#ifdef CONFIG_PCI_DRA7XX_HOST >> static const struct dra7xx_pcie_of_data dra7xx_pcie_rc_of_data = { >> .mode = DW_PCIE_RC_TYPE, >> }; >> +#endif >> >> +#ifdef CONFIG_PCI_DRA7XX_EP >> static const struct dra7xx_pcie_of_data dra7xx_pcie_ep_of_data = { >> .mode = DW_PCIE_EP_TYPE, >> }; >> +#endif >> >> static const struct of_device_id of_dra7xx_pcie_match[] = { >> +#ifdef CONFIG_PCI_DRA7XX_HOST >> { >> .compatible = "ti,dra7-pcie", >> .data = &dra7xx_pcie_rc_of_data, >> }, >> +#endif >> +#ifdef CONFIG_PCI_DRA7XX_EP >> { >> .compatible = "ti,dra7-pcie-ep", >> .data = &dra7xx_pcie_ep_of_data, >> }, >> +#endif >> {}, >> }; >> >> @@ -548,6 +562,7 @@ static const struct of_device_id of_dra7xx_pcie_match[] = { >> * >> * To avoid this issue set PCIE_SS1_AXI2OCP_LEGACY_MODE_ENABLE to 1. >> */ >> +#ifdef CONFIG_PCI_DRA7XX_EP >> static int dra7xx_pcie_ep_unaligned_memaccess(struct device *dev) >> { >> int ret; >> @@ -578,6 +593,7 @@ static int dra7xx_pcie_ep_unaligned_memaccess(struct device *dev) >> >> return ret; >> } >> +#endif I will move this to the CONFIG_PCI_DRA7XX_EP block, so that there will only be a single CONFIG_PCI_DRA7XX_EP ifdef surrounding EP specific functions. >> >> static int __init dra7xx_pcie_probe(struct platform_device *pdev) >> { >> @@ -681,6 +697,7 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev) >> dra7xx->link_gen = 2; >> >> switch (mode) { >> +#ifdef CONFIG_PCI_DRA7XX_HOST >> case DW_PCIE_RC_TYPE: >> dra7xx_pcie_writel(dra7xx, PCIECTRL_TI_CONF_DEVICE_TYPE, >> DEVICE_TYPE_RC); >> @@ -688,6 +705,8 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev) >> if (ret < 0) >> goto err_gpio; >> break; >> +#endif >> +#ifdef CONFIG_PCI_DRA7XX_EP >> case DW_PCIE_EP_TYPE: >> dra7xx_pcie_writel(dra7xx, PCIECTRL_TI_CONF_DEVICE_TYPE, >> DEVICE_TYPE_EP); >> @@ -700,6 +719,7 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev) >> if (ret < 0) >> goto err_gpio; >> break; >> +#endif Actually, these ifdefs has to stay, otherwise we get build warnings, since we are calling functions that aren't defined (dra7xx_pcie_ep_unaligned_memaccess, dra7xx_add_pcie_ep, dra7xx_add_pcie_port). We could add dummy implementations for these inside an #else block following the ifdef blocks. However, I think that adding dummy implementations in the #else block is uglier and more verbose than keeping the ifdefs around the two cases. Regards, Niklas
>>> >>> static int __init dra7xx_pcie_probe(struct platform_device *pdev) >>> { >>> @@ -681,6 +697,7 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev) >>> dra7xx->link_gen = 2; >>> >>> switch (mode) { >>> +#ifdef CONFIG_PCI_DRA7XX_HOST >>> case DW_PCIE_RC_TYPE: >>> dra7xx_pcie_writel(dra7xx, PCIECTRL_TI_CONF_DEVICE_TYPE, >>> DEVICE_TYPE_RC); >>> @@ -688,6 +705,8 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev) >>> if (ret < 0) >>> goto err_gpio; >>> break; >>> +#endif >>> +#ifdef CONFIG_PCI_DRA7XX_EP >>> case DW_PCIE_EP_TYPE: >>> dra7xx_pcie_writel(dra7xx, PCIECTRL_TI_CONF_DEVICE_TYPE, >>> DEVICE_TYPE_EP); >>> @@ -700,6 +719,7 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev) >>> if (ret < 0) >>> goto err_gpio; >>> break; >>> +#endif > > Actually, these ifdefs has to stay, otherwise we get build warnings, since we > are calling functions that aren't defined (dra7xx_pcie_ep_unaligned_memaccess, > dra7xx_add_pcie_ep, dra7xx_add_pcie_port). > We could add dummy implementations for these inside an #else block following > the ifdef blocks. However, I think that adding dummy implementations in the > #else block is uglier and more verbose than keeping the ifdefs around the > two cases. > ..however, if you prefer dummy implementations inside the #else blocks, I will of course do that. Regards, Niklas
On 10/31/2017 10:38 PM, Niklas Cassel wrote: >>>> >>>> static int __init dra7xx_pcie_probe(struct platform_device *pdev) >>>> { >>>> @@ -681,6 +697,7 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev) >>>> dra7xx->link_gen = 2; >>>> >>>> switch (mode) { >>>> +#ifdef CONFIG_PCI_DRA7XX_HOST >>>> case DW_PCIE_RC_TYPE: >>>> dra7xx_pcie_writel(dra7xx, PCIECTRL_TI_CONF_DEVICE_TYPE, >>>> DEVICE_TYPE_RC); >>>> @@ -688,6 +705,8 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev) >>>> if (ret < 0) >>>> goto err_gpio; >>>> break; >>>> +#endif >>>> +#ifdef CONFIG_PCI_DRA7XX_EP >>>> case DW_PCIE_EP_TYPE: >>>> dra7xx_pcie_writel(dra7xx, PCIECTRL_TI_CONF_DEVICE_TYPE, >>>> DEVICE_TYPE_EP); >>>> @@ -700,6 +719,7 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev) >>>> if (ret < 0) >>>> goto err_gpio; >>>> break; >>>> +#endif >> >> Actually, these ifdefs has to stay, otherwise we get build warnings, since we >> are calling functions that aren't defined (dra7xx_pcie_ep_unaligned_memaccess, >> dra7xx_add_pcie_ep, dra7xx_add_pcie_port). >> We could add dummy implementations for these inside an #else block following >> the ifdef blocks. However, I think that adding dummy implementations in the >> #else block is uglier and more verbose than keeping the ifdefs around the >> two cases. >> > > ..however, if you prefer dummy implementations inside the #else blocks, > I will of course do that. > And after thinking about it for a while a decided to go with dummy implementations in the #else blocks anyway :) This is how we usually do it in kernel code, we have one implementation if a certain Kconfig is set, and a dummy one if the Kconfig is not set. This way we avoid seeing ifdefs sprinkled all over the code, which makes it easier when trying to follow the flow of the code. Regards, Niklas
diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c index 009f6aeeee1c..175544d6c3ab 100644 --- a/drivers/pci/dwc/pci-dra7xx.c +++ b/drivers/pci/dwc/pci-dra7xx.c @@ -171,6 +171,15 @@ static int dra7xx_pcie_establish_link(struct dw_pcie *pci) return 0; } +static void dra7xx_pcie_enable_wrapper_interrupts(struct dra7xx_pcie *dra7xx) +{ + dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN, + INTERRUPTS); + dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MAIN, + INTERRUPTS); +} + +#ifdef CONFIG_PCI_DRA7XX_HOST static void dra7xx_pcie_enable_msi_interrupts(struct dra7xx_pcie *dra7xx) { dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MSI, @@ -181,14 +190,6 @@ static void dra7xx_pcie_enable_msi_interrupts(struct dra7xx_pcie *dra7xx) MSI | LEG_EP_INTERRUPTS); } -static void dra7xx_pcie_enable_wrapper_interrupts(struct dra7xx_pcie *dra7xx) -{ - dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQSTATUS_MAIN, - INTERRUPTS); - dra7xx_pcie_writel(dra7xx, PCIECTRL_DRA7XX_CONF_IRQENABLE_SET_MAIN, - INTERRUPTS); -} - static void dra7xx_pcie_enable_interrupts(struct dra7xx_pcie *dra7xx) { dra7xx_pcie_enable_wrapper_interrupts(dra7xx); @@ -276,6 +277,7 @@ static irqreturn_t dra7xx_pcie_msi_irq_handler(int irq, void *arg) return IRQ_HANDLED; } +#endif static irqreturn_t dra7xx_pcie_irq_handler(int irq, void *arg) { @@ -336,6 +338,7 @@ static irqreturn_t dra7xx_pcie_irq_handler(int irq, void *arg) return IRQ_HANDLED; } +#ifdef CONFIG_PCI_DRA7XX_EP static void dra7xx_pcie_ep_init(struct dw_pcie_ep *ep) { struct dw_pcie *pci = to_dw_pcie_from_ep(ep); @@ -427,7 +430,9 @@ static int __init dra7xx_add_pcie_ep(struct dra7xx_pcie *dra7xx, return 0; } +#endif +#ifdef CONFIG_PCI_DRA7XX_HOST static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx, struct platform_device *pdev) { @@ -470,6 +475,7 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx, return 0; } +#endif static const struct dw_pcie_ops dw_pcie_ops = { .cpu_addr_fixup = dra7xx_pcie_cpu_addr_fixup, @@ -517,23 +523,31 @@ static int dra7xx_pcie_enable_phy(struct dra7xx_pcie *dra7xx) return ret; } +#ifdef CONFIG_PCI_DRA7XX_HOST static const struct dra7xx_pcie_of_data dra7xx_pcie_rc_of_data = { .mode = DW_PCIE_RC_TYPE, }; +#endif +#ifdef CONFIG_PCI_DRA7XX_EP static const struct dra7xx_pcie_of_data dra7xx_pcie_ep_of_data = { .mode = DW_PCIE_EP_TYPE, }; +#endif static const struct of_device_id of_dra7xx_pcie_match[] = { +#ifdef CONFIG_PCI_DRA7XX_HOST { .compatible = "ti,dra7-pcie", .data = &dra7xx_pcie_rc_of_data, }, +#endif +#ifdef CONFIG_PCI_DRA7XX_EP { .compatible = "ti,dra7-pcie-ep", .data = &dra7xx_pcie_ep_of_data, }, +#endif {}, }; @@ -548,6 +562,7 @@ static const struct of_device_id of_dra7xx_pcie_match[] = { * * To avoid this issue set PCIE_SS1_AXI2OCP_LEGACY_MODE_ENABLE to 1. */ +#ifdef CONFIG_PCI_DRA7XX_EP static int dra7xx_pcie_ep_unaligned_memaccess(struct device *dev) { int ret; @@ -578,6 +593,7 @@ static int dra7xx_pcie_ep_unaligned_memaccess(struct device *dev) return ret; } +#endif static int __init dra7xx_pcie_probe(struct platform_device *pdev) { @@ -681,6 +697,7 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev) dra7xx->link_gen = 2; switch (mode) { +#ifdef CONFIG_PCI_DRA7XX_HOST case DW_PCIE_RC_TYPE: dra7xx_pcie_writel(dra7xx, PCIECTRL_TI_CONF_DEVICE_TYPE, DEVICE_TYPE_RC); @@ -688,6 +705,8 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev) if (ret < 0) goto err_gpio; break; +#endif +#ifdef CONFIG_PCI_DRA7XX_EP case DW_PCIE_EP_TYPE: dra7xx_pcie_writel(dra7xx, PCIECTRL_TI_CONF_DEVICE_TYPE, DEVICE_TYPE_EP); @@ -700,6 +719,7 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev) if (ret < 0) goto err_gpio; break; +#endif default: dev_err(dev, "INVALID device type %d\n", mode); }
This way you will not build and include unused code when only building for only one mode. Moved dra7xx_pcie_enable_wrapper_interrupts() in order to avoid adding an extra ifdef block. Signed-off-by: Niklas Cassel <niklas.cassel@axis.com> --- V2: * New patch in this series. drivers/pci/dwc/pci-dra7xx.c | 36 ++++++++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 8 deletions(-)