| Message ID | 20191209092147.22901-8-kishon@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add PCIe support to TI's J721E SoC | expand |
On Mon, Dec 09, 2019 at 02:51:41PM +0530, Kishon Vijay Abraham I wrote: > Cadence driver uses "mem" memory resource to obtain the offset of > configuration space address region, memory space address region and > message space address region. The obtained offset is used to program > the Address Translation Unit (ATU). However certain platforms like TI's > J721E SoC require the absolute address to be programmed in the ATU and not > just the offset. > > The same problem was solved in designware driver using a platform specific > ops for CPU addr fixup in commit a660083eb06c5bb0 ("PCI: dwc: designware: Thanks for this reference, though this doesn't need to be in the commit log, please put such comments underneath a ---. > Add new *ops* for CPU addr fixup"). Follow a similar mechanism in > Cadence too instead of directly using "mem" memory resource in Cadence > PCIe core. > > Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> > --- > .../pci/controller/cadence/pcie-cadence-host.c | 15 ++++----------- > drivers/pci/controller/cadence/pcie-cadence.c | 8 ++++++-- > drivers/pci/controller/cadence/pcie-cadence.h | 1 + > 3 files changed, 11 insertions(+), 13 deletions(-) > > diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c > index 2efc33b1cade..cf817be237af 100644 > --- a/drivers/pci/controller/cadence/pcie-cadence-host.c > +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c > @@ -105,15 +105,14 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc) > static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc) > { > struct cdns_pcie *pcie = &rc->pcie; > - struct resource *mem_res = pcie->mem_res; > struct resource *bus_range = rc->bus_range; > struct resource *cfg_res = rc->cfg_res; > struct device *dev = pcie->dev; > struct device_node *np = dev->of_node; > struct of_pci_range_parser parser; > + u64 cpu_addr = cfg_res->start; > struct of_pci_range range; > u32 addr0, addr1, desc1; > - u64 cpu_addr; > int r, err; > > /* > @@ -126,7 +125,9 @@ static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc) > cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_PCI_ADDR1(0), addr1); > cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_DESC1(0), desc1); > > - cpu_addr = cfg_res->start - mem_res->start; > + if (pcie->ops->cpu_addr_fixup) > + cpu_addr = pcie->ops->cpu_addr_fixup(pcie, cpu_addr); > + Won't this patch cause a breakage for existing users that won't have defined a cpu_addr_fixup? The offset isn't being calculated and so cpu_addr will be wrong? Thanks, Andrew Murray > addr0 = CDNS_PCIE_AT_OB_REGION_CPU_ADDR0_NBITS(12) | > (lower_32_bits(cpu_addr) & GENMASK(31, 8)); > addr1 = upper_32_bits(cpu_addr); > @@ -264,14 +265,6 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc) > } > rc->cfg_res = res; > > - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mem"); > - if (!res) { > - dev_err(dev, "missing \"mem\"\n"); > - return -EINVAL; > - } > - > - pcie->mem_res = res; > - > ret = cdns_pcie_start_link(pcie, true); > if (ret) { > dev_err(dev, "Failed to start link\n"); > diff --git a/drivers/pci/controller/cadence/pcie-cadence.c b/drivers/pci/controller/cadence/pcie-cadence.c > index de5b3b06f2d0..bd93d0f92f55 100644 > --- a/drivers/pci/controller/cadence/pcie-cadence.c > +++ b/drivers/pci/controller/cadence/pcie-cadence.c > @@ -113,7 +113,9 @@ void cdns_pcie_set_outbound_region(struct cdns_pcie *pcie, u8 fn, > cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_DESC1(r), desc1); > > /* Set the CPU address */ > - cpu_addr -= pcie->mem_res->start; > + if (pcie->ops->cpu_addr_fixup) > + cpu_addr = pcie->ops->cpu_addr_fixup(pcie, cpu_addr); > + > addr0 = CDNS_PCIE_AT_OB_REGION_CPU_ADDR0_NBITS(nbits) | > (lower_32_bits(cpu_addr) & GENMASK(31, 8)); > addr1 = upper_32_bits(cpu_addr); > @@ -140,7 +142,9 @@ void cdns_pcie_set_outbound_region_for_normal_msg(struct cdns_pcie *pcie, u8 fn, > } > > /* Set the CPU address */ > - cpu_addr -= pcie->mem_res->start; > + if (pcie->ops->cpu_addr_fixup) > + cpu_addr = pcie->ops->cpu_addr_fixup(pcie, cpu_addr); > + > addr0 = CDNS_PCIE_AT_OB_REGION_CPU_ADDR0_NBITS(17) | > (lower_32_bits(cpu_addr) & GENMASK(31, 8)); > addr1 = upper_32_bits(cpu_addr); > diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h > index c879dd3d2893..ffa8b9f78ff8 100644 > --- a/drivers/pci/controller/cadence/pcie-cadence.h > +++ b/drivers/pci/controller/cadence/pcie-cadence.h > @@ -233,6 +233,7 @@ struct cdns_pcie_ops { > void (*write)(void __iomem *addr, int size, u32 value); > int (*start_link)(struct cdns_pcie *pcie, bool start); > bool (*is_link_up)(struct cdns_pcie *pcie); > + u64 (*cpu_addr_fixup)(struct cdns_pcie *pcie, u64 cpu_addr); > }; > > /** > -- > 2.17.1 >
Hi, On 17/12/19 6:10 pm, Andrew Murray wrote: > On Mon, Dec 09, 2019 at 02:51:41PM +0530, Kishon Vijay Abraham I wrote: >> Cadence driver uses "mem" memory resource to obtain the offset of >> configuration space address region, memory space address region and >> message space address region. The obtained offset is used to program >> the Address Translation Unit (ATU). However certain platforms like TI's >> J721E SoC require the absolute address to be programmed in the ATU and not >> just the offset. >> >> The same problem was solved in designware driver using a platform specific >> ops for CPU addr fixup in commit a660083eb06c5bb0 ("PCI: dwc: designware: > > Thanks for this reference, though this doesn't need to be in the commit > log, please put such comments underneath a ---. > >> Add new *ops* for CPU addr fixup"). Follow a similar mechanism in >> Cadence too instead of directly using "mem" memory resource in Cadence >> PCIe core. >> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> >> --- >> .../pci/controller/cadence/pcie-cadence-host.c | 15 ++++----------- >> drivers/pci/controller/cadence/pcie-cadence.c | 8 ++++++-- >> drivers/pci/controller/cadence/pcie-cadence.h | 1 + >> 3 files changed, 11 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c >> index 2efc33b1cade..cf817be237af 100644 >> --- a/drivers/pci/controller/cadence/pcie-cadence-host.c >> +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c >> @@ -105,15 +105,14 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc) >> static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc) >> { >> struct cdns_pcie *pcie = &rc->pcie; >> - struct resource *mem_res = pcie->mem_res; >> struct resource *bus_range = rc->bus_range; >> struct resource *cfg_res = rc->cfg_res; >> struct device *dev = pcie->dev; >> struct device_node *np = dev->of_node; >> struct of_pci_range_parser parser; >> + u64 cpu_addr = cfg_res->start; >> struct of_pci_range range; >> u32 addr0, addr1, desc1; >> - u64 cpu_addr; >> int r, err; >> >> /* >> @@ -126,7 +125,9 @@ static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc) >> cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_PCI_ADDR1(0), addr1); >> cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_DESC1(0), desc1); >> >> - cpu_addr = cfg_res->start - mem_res->start; >> + if (pcie->ops->cpu_addr_fixup) >> + cpu_addr = pcie->ops->cpu_addr_fixup(pcie, cpu_addr); >> + > > Won't this patch cause a breakage for existing users that won't have defined a > cpu_addr_fixup? The offset isn't being calculated and so cpu_addr will be wrong? Correct, this will need an additional patch in pcie-cadence-plat.c. Thanks Kishon
diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c b/drivers/pci/controller/cadence/pcie-cadence-host.c index 2efc33b1cade..cf817be237af 100644 --- a/drivers/pci/controller/cadence/pcie-cadence-host.c +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c @@ -105,15 +105,14 @@ static int cdns_pcie_host_init_root_port(struct cdns_pcie_rc *rc) static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc) { struct cdns_pcie *pcie = &rc->pcie; - struct resource *mem_res = pcie->mem_res; struct resource *bus_range = rc->bus_range; struct resource *cfg_res = rc->cfg_res; struct device *dev = pcie->dev; struct device_node *np = dev->of_node; struct of_pci_range_parser parser; + u64 cpu_addr = cfg_res->start; struct of_pci_range range; u32 addr0, addr1, desc1; - u64 cpu_addr; int r, err; /* @@ -126,7 +125,9 @@ static int cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc) cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_PCI_ADDR1(0), addr1); cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_DESC1(0), desc1); - cpu_addr = cfg_res->start - mem_res->start; + if (pcie->ops->cpu_addr_fixup) + cpu_addr = pcie->ops->cpu_addr_fixup(pcie, cpu_addr); + addr0 = CDNS_PCIE_AT_OB_REGION_CPU_ADDR0_NBITS(12) | (lower_32_bits(cpu_addr) & GENMASK(31, 8)); addr1 = upper_32_bits(cpu_addr); @@ -264,14 +265,6 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc) } rc->cfg_res = res; - res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mem"); - if (!res) { - dev_err(dev, "missing \"mem\"\n"); - return -EINVAL; - } - - pcie->mem_res = res; - ret = cdns_pcie_start_link(pcie, true); if (ret) { dev_err(dev, "Failed to start link\n"); diff --git a/drivers/pci/controller/cadence/pcie-cadence.c b/drivers/pci/controller/cadence/pcie-cadence.c index de5b3b06f2d0..bd93d0f92f55 100644 --- a/drivers/pci/controller/cadence/pcie-cadence.c +++ b/drivers/pci/controller/cadence/pcie-cadence.c @@ -113,7 +113,9 @@ void cdns_pcie_set_outbound_region(struct cdns_pcie *pcie, u8 fn, cdns_pcie_writel(pcie, CDNS_PCIE_AT_OB_REGION_DESC1(r), desc1); /* Set the CPU address */ - cpu_addr -= pcie->mem_res->start; + if (pcie->ops->cpu_addr_fixup) + cpu_addr = pcie->ops->cpu_addr_fixup(pcie, cpu_addr); + addr0 = CDNS_PCIE_AT_OB_REGION_CPU_ADDR0_NBITS(nbits) | (lower_32_bits(cpu_addr) & GENMASK(31, 8)); addr1 = upper_32_bits(cpu_addr); @@ -140,7 +142,9 @@ void cdns_pcie_set_outbound_region_for_normal_msg(struct cdns_pcie *pcie, u8 fn, } /* Set the CPU address */ - cpu_addr -= pcie->mem_res->start; + if (pcie->ops->cpu_addr_fixup) + cpu_addr = pcie->ops->cpu_addr_fixup(pcie, cpu_addr); + addr0 = CDNS_PCIE_AT_OB_REGION_CPU_ADDR0_NBITS(17) | (lower_32_bits(cpu_addr) & GENMASK(31, 8)); addr1 = upper_32_bits(cpu_addr); diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h index c879dd3d2893..ffa8b9f78ff8 100644 --- a/drivers/pci/controller/cadence/pcie-cadence.h +++ b/drivers/pci/controller/cadence/pcie-cadence.h @@ -233,6 +233,7 @@ struct cdns_pcie_ops { void (*write)(void __iomem *addr, int size, u32 value); int (*start_link)(struct cdns_pcie *pcie, bool start); bool (*is_link_up)(struct cdns_pcie *pcie); + u64 (*cpu_addr_fixup)(struct cdns_pcie *pcie, u64 cpu_addr); }; /**
Cadence driver uses "mem" memory resource to obtain the offset of configuration space address region, memory space address region and message space address region. The obtained offset is used to program the Address Translation Unit (ATU). However certain platforms like TI's J721E SoC require the absolute address to be programmed in the ATU and not just the offset. The same problem was solved in designware driver using a platform specific ops for CPU addr fixup in commit a660083eb06c5bb0 ("PCI: dwc: designware: Add new *ops* for CPU addr fixup"). Follow a similar mechanism in Cadence too instead of directly using "mem" memory resource in Cadence PCIe core. Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com> --- .../pci/controller/cadence/pcie-cadence-host.c | 15 ++++----------- drivers/pci/controller/cadence/pcie-cadence.c | 8 ++++++-- drivers/pci/controller/cadence/pcie-cadence.h | 1 + 3 files changed, 11 insertions(+), 13 deletions(-)