Messages in this thread | | | Date | Fri, 20 Jun 2014 14:47:27 -0400 | From | Murali Karicheri <> | Subject | Re: [PATCH v2 2/8] PCI: designware: refactor host init code to re-use on v3.65 DW pci hw |
| |
On 6/18/2014 3:05 AM, Pratyush Anand wrote: > Hi Murali, > > On Wed, Jun 11, 2014 at 02:51:21AM +0800, Murali Karicheri wrote: >> Current DW PCI host init code has code specific to newer hw such as >> ATU port specific resource parsing and map. v3.65 DW PCI host has > OK, Older version did not had standard viewport implementation, so patch 1 > of this series will help you to take care for that. > >> MSI controller in application space and requires different controller > Since MSI controller is implemented in application space, so this may > not be same for different older version dw controller users. > > Therefore, I would suggest to implement all application specific code > in your keystone driver only. Pratyush,
Thanks for the comments.
This is IP specific code and another driver that has this version of the IP will be able to re-use the code. This is also being discussed in another thread from Bjorn and Jingoo. Please discuss this in that thread.
>> initialization code. So refactor the msi host controller code into a >> separate function. Other common functions across both hw versions >> are moved to dw_pcie_common_host_init() and called from the newer and >> v3.65 hw host initialization code. >> >> Signed-off-by: Murali Karicheri <m-karicheri2@ti.com> >> >> CC: Santosh Shilimkar <santosh.shilimkar@ti.com> >> CC: Russell King <linux@arm.linux.org.uk> >> CC: Grant Likely <grant.likely@linaro.org> >> CC: Rob Herring <robh+dt@kernel.org> >> CC: Mohit Kumar <mohit.kumar@st.com> >> CC: Jingoo Han <jg1.han@samsung.com> >> CC: Bjorn Helgaas <bhelgaas@google.com> >> CC: Pratyush Anand <pratyush.anand@st.com> >> CC: Richard Zhu <r65037@freescale.com> >> CC: Kishon Vijay Abraham I <kishon@ti.com> >> CC: Marek Vasut <marex@denx.de> >> CC: Arnd Bergmann <arnd@arndb.de> >> CC: Pawel Moll <pawel.moll@arm.com> >> CC: Mark Rutland <mark.rutland@arm.com> >> CC: Ian Campbell <ijc+devicetree@hellion.org.uk> >> CC: Kumar Gala <galak@codeaurora.org> >> CC: Randy Dunlap <rdunlap@infradead.org> >> CC: Grant Likely <grant.likely@linaro.org> >> >> --- >> drivers/pci/host/pcie-designware.c | 136 ++++++++++++++++++++++++++---------- >> 1 file changed, 101 insertions(+), 35 deletions(-) >> >> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c >> index e8f5d8d..e4bd19a 100644 >> --- a/drivers/pci/host/pcie-designware.c >> +++ b/drivers/pci/host/pcie-designware.c >> @@ -389,13 +389,19 @@ static const struct irq_domain_ops msi_domain_ops = { >> .map = dw_pcie_msi_map, >> }; >> >> -int __init dw_pcie_host_init(struct pcie_port *pp) >> +/* >> + * dw_pcie_parse_resource() - Function to parse common resources >> + * >> + * @pp: ptr to pcie port >> + * >> + * Parse the range property for MEM, IO and cfg resources, and map >> + * the cfg register space. >> + */ > Why do you need this function? If you have some extra resource, you > can ioremap that before you call dw_pcie_host_init.
I assume you are referring to dw_pcie_parse_resource(). Keystone PCIe driver needs this to parse the range property for IORESOURCE_MEM and IORESOURCE_IO resources. So refactored this into a function and called from host_init() code for v3.65 and dw_pcie_host_init . But for Keystone PCI driver, no need to ioremap cfg1 and cfg2 as it doesn't have ATU ports. So host_init() code has to be little different. Idea is to have IP specific host_init() and refactor and re-use the common code on both.
> >> +int __init dw_pcie_parse_resource(struct pcie_port *pp) >> { >> struct device_node *np = pp->dev->of_node; >> struct of_pci_range range; >> struct of_pci_range_parser parser; >> - u32 val; >> - int i; >> >> if (of_pci_range_parser_init(&parser, np)) { >> dev_err(pp->dev, "missing ranges property\n"); >> @@ -431,41 +437,29 @@ int __init dw_pcie_host_init(struct pcie_port *pp) >> pp->config.cfg1_size = resource_size(&pp->cfg)/2; >> } >> } >> - >> - if (!pp->dbi_base) { >> - pp->dbi_base = devm_ioremap(pp->dev, pp->cfg.start, >> - resource_size(&pp->cfg)); >> - if (!pp->dbi_base) { >> - dev_err(pp->dev, "error with ioremap\n"); >> - return -ENOMEM; >> - } >> - } >> - >> - pp->cfg0_base = pp->cfg.start; >> - pp->cfg1_base = pp->cfg.start + pp->config.cfg0_size; >> pp->mem_base = pp->mem.start; >> >> - pp->va_cfg0_base = devm_ioremap(pp->dev, pp->cfg0_base, >> - pp->config.cfg0_size); >> - if (!pp->va_cfg0_base) { >> - dev_err(pp->dev, "error with ioremap in function\n"); >> - return -ENOMEM; >> - } >> - pp->va_cfg1_base = devm_ioremap(pp->dev, pp->cfg1_base, >> - pp->config.cfg1_size); >> - if (!pp->va_cfg1_base) { >> - dev_err(pp->dev, "error with ioremap\n"); >> - return -ENOMEM; >> - } >> + return 0; >> +} >> >> - if (of_property_read_u32(np, "num-lanes", &pp->lanes)) { >> - dev_err(pp->dev, "Failed to parse the number of lanes\n"); >> - return -EINVAL; >> - } >> +/* >> + * dw_pcie_msi_host_init() - Function to initialize msi host controller >> + * >> + * @pp: ptr to pcie port >> + * @np: device node ptr to msi irq controller >> + * @irq_msi_ops: ptr to MSI irq_domain_ops struct >> + * >> + * Function register irq domain for msi irq controller and create mappings >> + * for MSI irqs. >> + */ > > May be you can only do following to support your MSI chip: > > Initialize pp->irq_domain into your add_pcie_port function before you > call dw_pcie_host_init. > > In dw_pcie_host_init, > > if (IS_ENABLED(CONFIG_PCI_MSI) && !pp->irq_domain)
How do I pass the keystone specific msi irq_domain_ops? It looks clean the way it is implemented in this patch. Also since MSI host controller and legacy host controller are different, it make sense to add separate host_init for each. Let me know.
Regards,
Murali
> >> +int dw_pcie_msi_host_init(struct pcie_port *pp, struct device_node *np, >> + const struct irq_domain_ops *irq_msi_ops) >> +{ >> + int i; >> >> if (IS_ENABLED(CONFIG_PCI_MSI)) { >> pp->irq_domain = irq_domain_add_linear(pp->dev->of_node, >> - MAX_MSI_IRQS, &msi_domain_ops, >> + MAX_MSI_IRQS, irq_msi_ops, >> &dw_pcie_msi_chip); >> if (!pp->irq_domain) { >> dev_err(pp->dev, "irq domain init failed\n"); >> @@ -476,6 +470,29 @@ int __init dw_pcie_host_init(struct pcie_port *pp) >> irq_create_mapping(pp->irq_domain, i); >> } >> >> + return 0; >> +} >> + >> +/* >> + * dw_pcie_common_host_init() - common host init function across different >> + * versions of the designware PCI controller. >> + * @pp: ptr to pcie port >> + * @hw: ptr to hw_pci structure >> + * >> + * This functions parses common DT properties, call host_init() callback >> + * of the PCI controller driver. Also initialize the common RC configurations >> + * and call common pci core function to intialize the controller driver. >> + */ >> +int __init dw_pcie_common_host_init(struct pcie_port *pp, struct hw_pci *hw) >> +{ >> + struct device_node *np = pp->dev->of_node; >> + u32 val; >> + >> + if (of_property_read_u32(np, "num-lanes", &pp->lanes)) { >> + dev_err(pp->dev, "Failed to parse the number of lanes\n"); >> + return -EINVAL; >> + } >> + >> if (pp->ops->host_init) >> pp->ops->host_init(pp); >> >> @@ -488,10 +505,10 @@ int __init dw_pcie_host_init(struct pcie_port *pp) >> val |= PORT_LOGIC_SPEED_CHANGE; >> dw_pcie_wr_own_conf(pp, PCIE_LINK_WIDTH_SPEED_CONTROL, 4, val); >> >> - dw_pci.nr_controllers = 1; >> - dw_pci.private_data = (void **)&pp; >> + hw->nr_controllers = 1; >> + hw->private_data = (void **)&pp; >> >> - pci_common_init_dev(pp->dev, &dw_pci); >> + pci_common_init_dev(pp->dev, hw); >> pci_assign_unassigned_resources(); >> #ifdef CONFIG_PCI_DOMAINS >> dw_pci.domain++; >> @@ -500,6 +517,55 @@ int __init dw_pcie_host_init(struct pcie_port *pp) >> return 0; >> } >> >> +/* >> + * dw_pcie_host_init() - Host init function for new designware h/w >> + * >> + * @pp: ptr to pcie port >> + * >> + * The function parse the PCI resurces for IO, Memory and map the config >> + * space addresses. Also initliaze the MSI irq controller and call >> + * dw_pcie_common_host_init() to initialize the PCI controller. >> + */ >> +int __init dw_pcie_host_init(struct pcie_port *pp) >> +{ >> + int ret; >> + >> + ret = dw_pcie_parse_resource(pp); >> + if (ret) >> + return ret; >> + >> + if (!pp->dbi_base) { >> + pp->dbi_base = devm_ioremap(pp->dev, pp->cfg.start, >> + resource_size(&pp->cfg)); >> + if (!pp->dbi_base) { >> + dev_err(pp->dev, "error with ioremap\n"); >> + return -ENOMEM; >> + } >> + } >> + >> + pp->cfg0_base = pp->cfg.start; >> + pp->cfg1_base = pp->cfg.start + pp->config.cfg0_size; >> + >> + pp->va_cfg0_base = devm_ioremap(pp->dev, pp->cfg0_base, >> + pp->config.cfg0_size); >> + if (!pp->va_cfg0_base) { >> + dev_err(pp->dev, "error with ioremap in function\n"); >> + return -ENOMEM; >> + } >> + pp->va_cfg1_base = devm_ioremap(pp->dev, pp->cfg1_base, >> + pp->config.cfg1_size); >> + if (!pp->va_cfg1_base) { >> + dev_err(pp->dev, "error with ioremap\n"); >> + return -ENOMEM; >> + } >> + >> + ret = dw_pcie_msi_host_init(pp, pp->dev->of_node, &msi_domain_ops); >> + if (ret < 0) >> + return ret; >> + >> + return dw_pcie_common_host_init(pp, &dw_pci); >> +} >> + >> static void dw_pcie_prog_viewport_cfg0(struct pcie_port *pp, u32 busdev) >> { >> /* Program viewport 0 : OUTBOUND : CFG0 */ > Regards > Pratyush > >> -- >> 1.7.9.5
| |