| Message ID | 20240926-pci_fixup_addr-v2-1-e4524541edf4@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: dwc: opitimaze RC host pci_fixup_addr() | expand |
On Thu, Sep 26, 2024 at 12:47:13PM -0400, Frank Li wrote: > Introduce field 'cpu_untranslate_addr' in of_pci_range to retrieve > untranslated CPU address information. This is required for hardware like > i.MX8QXP to configure the PCIe controller ATU and eliminate the need for > workaround address fixups in drivers. Currently, many drivers use > hardcoded CPU addresses for fixups, but this information is already > described in the Device Tree. With correct hardware descriptions, such > fixups can be removed. > > ┌─────────┐ ┌────────────┐ > ┌─────┐ │ │ IA: 0x8ff0_0000 │ │ > │ CPU ├───►│ BUS ├─────────────────┐ │ PCI │ > └─────┘ │ │ IA: 0x8ff8_0000 │ │ │ > CPU Addr │ Fabric ├─────────────┐ │ │ Controller │ > 0x7000_0000 │ │ │ │ │ │ > │ │ │ │ │ │ PCI Addr > │ │ │ └──► CfgSpace ─┼────────────► > │ ├─────────┐ │ │ │ 0 > │ │ │ │ │ │ > └─────────┘ │ └──────► IOSpace ─┼────────────► > │ │ │ 0 > │ │ │ > └──────────► MemSpace ─┼────────────► > IA: 0x8000_0000 │ │ 0x8000_0000 > └────────────┘ > > bus@5f000000 { > compatible = "simple-bus"; > #address-cells = <1>; > #size-cells = <1>; > ranges = <0x5f000000 0x0 0x5f000000 0x21000000>, > <0x80000000 0x0 0x70000000 0x10000000>; > > pcieb: pcie@5f010000 { > compatible = "fsl,imx8q-pcie"; > reg = <0x5f010000 0x10000>, <0x8ff00000 0x80000>; > reg-names = "dbi", "config"; > #address-cells = <3>; > #size-cells = <2>; > device_type = "pci"; > bus-range = <0x00 0xff>; > ranges = <0x81000000 0 0x00000000 0x8ff80000 0 0x00010000>, > <0x82000000 0 0x80000000 0x80000000 0 0x0ff00000>; > ... > }; > }; > > 'cpu_untranslate_addr' in of_pci_range can indicate above diagram IA > address information. > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > Change from v1 to v2 > - add cpu_untranslate_addr in of_pci_range, instead adding new API. > --- > drivers/of/address.c | 2 ++ > include/linux/of_address.h | 1 + > 2 files changed, 3 insertions(+) > > diff --git a/drivers/of/address.c b/drivers/of/address.c > index 286f0c161e332..f4cb82f5313cf 100644 > --- a/drivers/of/address.c > +++ b/drivers/of/address.c > @@ -811,6 +811,8 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser, > else > range->cpu_addr = of_translate_address(parser->node, > parser->range + na); > + > + range->cpu_untranslate_addr = of_read_number(parser->range + na, parser->pna); > range->size = of_read_number(parser->range + parser->pna + na, ns); > > parser->range += np; > diff --git a/include/linux/of_address.h b/include/linux/of_address.h > index 26a19daf0d092..0683ce0c07f68 100644 > --- a/include/linux/of_address.h > +++ b/include/linux/of_address.h > @@ -26,6 +26,7 @@ struct of_pci_range { > u64 bus_addr; > }; > u64 cpu_addr; > + u64 cpu_untranslate_addr; Let's call it "parent_bus_addr" as it's not really the "cpu" address any more. With that, Reviewed-by: Rob Herring (Arm) <robh@kernel.org> Rob
On Thu, Sep 26, 2024 at 12:47:13PM -0400, Frank Li wrote: > Introduce field 'cpu_untranslate_addr' in of_pci_range to retrieve > untranslated CPU address information. This is required for hardware like > i.MX8QXP to configure the PCIe controller ATU and eliminate the need for > workaround address fixups in drivers. Currently, many drivers use > hardcoded CPU addresses for fixups, but this information is already > described in the Device Tree. With correct hardware descriptions, such > fixups can be removed. Instead of saying "required for hardware like i.MX8QXP", can we say something specific about what this kind of hardware *does* that requires this? I *think* the point is that there's some address translation being done between the primary and secondary sides of some bridge. I think "many drivers use hardcoded CPU addresses for fixups" basically means the .cpu_addr_fixup() callback hardcodes that translation in the code, e.g., "cpu_addr & CDNS_PLAT_CPU_TO_BUS_ADDR", "cpu_addr + BUS_IATU_OFFSET", etc, even though those translations *should* be described via DT. > ┌─────────┐ ┌────────────┐ > ┌─────┐ │ │ IA: 0x8ff0_0000 │ │ > │ CPU ├───►│ BUS ├─────────────────┐ │ PCI │ > └─────┘ │ │ IA: 0x8ff8_0000 │ │ │ > CPU Addr │ Fabric ├─────────────┐ │ │ Controller │ > 0x7000_0000 │ │ │ │ │ │ > │ │ │ │ │ │ PCI Addr > │ │ │ └──► CfgSpace ─┼────────────► > │ ├─────────┐ │ │ │ 0 > │ │ │ │ │ │ > └─────────┘ │ └──────► IOSpace ─┼────────────► > │ │ │ 0 > │ │ │ > └──────────► MemSpace ─┼────────────► > IA: 0x8000_0000 │ │ 0x8000_0000 > └────────────┘ What does "IA" stand for? I don't quite understand the mapping done by the "BUS Fabric" block. It looks like you're saying the CPU Addr 0x7000_0000 is translated to all three of IA 0x8ff0_0000, IA 0x8ff8_0000, and IA 0x8000_0000, but that doesn't seem right. > bus@5f000000 { > compatible = "simple-bus"; > #address-cells = <1>; > #size-cells = <1>; > ranges = <0x5f000000 0x0 0x5f000000 0x21000000>, > <0x80000000 0x0 0x70000000 0x10000000>; > > pcieb: pcie@5f010000 { > compatible = "fsl,imx8q-pcie"; > reg = <0x5f010000 0x10000>, <0x8ff00000 0x80000>; > reg-names = "dbi", "config"; > #address-cells = <3>; > #size-cells = <2>; > device_type = "pci"; > bus-range = <0x00 0xff>; > ranges = <0x81000000 0 0x00000000 0x8ff80000 0 0x00010000>, > <0x82000000 0 0x80000000 0x80000000 0 0x0ff00000>; > ... > }; > }; > > 'cpu_untranslate_addr' in of_pci_range can indicate above diagram IA > address information. > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > Change from v1 to v2 > - add cpu_untranslate_addr in of_pci_range, instead adding new API. > --- > drivers/of/address.c | 2 ++ > include/linux/of_address.h | 1 + > 2 files changed, 3 insertions(+) > > diff --git a/drivers/of/address.c b/drivers/of/address.c > index 286f0c161e332..f4cb82f5313cf 100644 > --- a/drivers/of/address.c > +++ b/drivers/of/address.c > @@ -811,6 +811,8 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser, > else > range->cpu_addr = of_translate_address(parser->node, > parser->range + na); > + > + range->cpu_untranslate_addr = of_read_number(parser->range + na, parser->pna); > range->size = of_read_number(parser->range + parser->pna + na, ns); > > parser->range += np; > diff --git a/include/linux/of_address.h b/include/linux/of_address.h > index 26a19daf0d092..0683ce0c07f68 100644 > --- a/include/linux/of_address.h > +++ b/include/linux/of_address.h > @@ -26,6 +26,7 @@ struct of_pci_range { > u64 bus_addr; > }; > u64 cpu_addr; > + u64 cpu_untranslate_addr; > u64 size; > u32 flags; > }; > > -- > 2.34.1 >
On Fri, Sep 27, 2024 at 06:51:17PM -0500, Bjorn Helgaas wrote: > On Thu, Sep 26, 2024 at 12:47:13PM -0400, Frank Li wrote: > > Introduce field 'cpu_untranslate_addr' in of_pci_range to retrieve > > untranslated CPU address information. This is required for hardware like > > i.MX8QXP to configure the PCIe controller ATU and eliminate the need for > > workaround address fixups in drivers. Currently, many drivers use > > hardcoded CPU addresses for fixups, but this information is already > > described in the Device Tree. With correct hardware descriptions, such > > fixups can be removed. > > Instead of saying "required for hardware like i.MX8QXP", can we say > something specific about what this kind of hardware *does* that > requires this? > > I *think* the point is that there's some address translation being > done between the primary and secondary sides of some bridge. > > I think "many drivers use hardcoded CPU addresses for fixups" > basically means the .cpu_addr_fixup() callback hardcodes that > translation in the code, e.g., "cpu_addr & CDNS_PLAT_CPU_TO_BUS_ADDR", > "cpu_addr + BUS_IATU_OFFSET", etc, even though those translations > *should* be described via DT. > > > ┌─────────┐ ┌────────────┐ > > ┌─────┐ │ │ IA: 0x8ff0_0000 │ │ > > │ CPU ├───►│ BUS ├─────────────────┐ │ PCI │ > > └─────┘ │ │ IA: 0x8ff8_0000 │ │ │ > > CPU Addr │ Fabric ├─────────────┐ │ │ Controller │ > > 0x7000_0000 │ │ │ │ │ │ > > │ │ │ │ │ │ PCI Addr > > │ │ │ └──► CfgSpace ─┼────────────► > > │ ├─────────┐ │ │ │ 0 > > │ │ │ │ │ │ > > └─────────┘ │ └──────► IOSpace ─┼────────────► > > │ │ │ 0 > > │ │ │ > > └──────────► MemSpace ─┼────────────► > > IA: 0x8000_0000 │ │ 0x8000_0000 > > └────────────┘ > > What does "IA" stand for? internal address > > I don't quite understand the mapping done by the "BUS Fabric" block. > It looks like you're saying the CPU Addr 0x7000_0000 is translated to > all three of IA 0x8ff0_0000, IA 0x8ff8_0000, and IA 0x8000_0000, but > that doesn't seem right. 0x7000_0000 --> 0x8000_0000 0x7ff0_0000 --> 0x8ff0_0000 0x7ff8_0000 --> 0x8ff8_0000 I can update diagram at next version > > > bus@5f000000 { > > compatible = "simple-bus"; > > #address-cells = <1>; > > #size-cells = <1>; > > ranges = <0x5f000000 0x0 0x5f000000 0x21000000>, > > <0x80000000 0x0 0x70000000 0x10000000>; > > > > pcieb: pcie@5f010000 { > > compatible = "fsl,imx8q-pcie"; > > reg = <0x5f010000 0x10000>, <0x8ff00000 0x80000>; > > reg-names = "dbi", "config"; > > #address-cells = <3>; > > #size-cells = <2>; > > device_type = "pci"; > > bus-range = <0x00 0xff>; > > ranges = <0x81000000 0 0x00000000 0x8ff80000 0 0x00010000>, > > <0x82000000 0 0x80000000 0x80000000 0 0x0ff00000>; > > ... > > }; > > }; > > > > 'cpu_untranslate_addr' in of_pci_range can indicate above diagram IA > > address information. > > > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > --- > > Change from v1 to v2 > > - add cpu_untranslate_addr in of_pci_range, instead adding new API. > > --- > > drivers/of/address.c | 2 ++ > > include/linux/of_address.h | 1 + > > 2 files changed, 3 insertions(+) > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c > > index 286f0c161e332..f4cb82f5313cf 100644 > > --- a/drivers/of/address.c > > +++ b/drivers/of/address.c > > @@ -811,6 +811,8 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser, > > else > > range->cpu_addr = of_translate_address(parser->node, > > parser->range + na); > > + > > + range->cpu_untranslate_addr = of_read_number(parser->range + na, parser->pna); > > range->size = of_read_number(parser->range + parser->pna + na, ns); > > > > parser->range += np; > > diff --git a/include/linux/of_address.h b/include/linux/of_address.h > > index 26a19daf0d092..0683ce0c07f68 100644 > > --- a/include/linux/of_address.h > > +++ b/include/linux/of_address.h > > @@ -26,6 +26,7 @@ struct of_pci_range { > > u64 bus_addr; > > }; > > u64 cpu_addr; > > + u64 cpu_untranslate_addr; > > u64 size; > > u32 flags; > > }; > > > > -- > > 2.34.1 > >
diff --git a/drivers/of/address.c b/drivers/of/address.c index 286f0c161e332..f4cb82f5313cf 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -811,6 +811,8 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser, else range->cpu_addr = of_translate_address(parser->node, parser->range + na); + + range->cpu_untranslate_addr = of_read_number(parser->range + na, parser->pna); range->size = of_read_number(parser->range + parser->pna + na, ns); parser->range += np; diff --git a/include/linux/of_address.h b/include/linux/of_address.h index 26a19daf0d092..0683ce0c07f68 100644 --- a/include/linux/of_address.h +++ b/include/linux/of_address.h @@ -26,6 +26,7 @@ struct of_pci_range { u64 bus_addr; }; u64 cpu_addr; + u64 cpu_untranslate_addr; u64 size; u32 flags; };
Introduce field 'cpu_untranslate_addr' in of_pci_range to retrieve untranslated CPU address information. This is required for hardware like i.MX8QXP to configure the PCIe controller ATU and eliminate the need for workaround address fixups in drivers. Currently, many drivers use hardcoded CPU addresses for fixups, but this information is already described in the Device Tree. With correct hardware descriptions, such fixups can be removed. ┌─────────┐ ┌────────────┐ ┌─────┐ │ │ IA: 0x8ff0_0000 │ │ │ CPU ├───►│ BUS ├─────────────────┐ │ PCI │ └─────┘ │ │ IA: 0x8ff8_0000 │ │ │ CPU Addr │ Fabric ├─────────────┐ │ │ Controller │ 0x7000_0000 │ │ │ │ │ │ │ │ │ │ │ │ PCI Addr │ │ │ └──► CfgSpace ─┼────────────► │ ├─────────┐ │ │ │ 0 │ │ │ │ │ │ └─────────┘ │ └──────► IOSpace ─┼────────────► │ │ │ 0 │ │ │ └──────────► MemSpace ─┼────────────► IA: 0x8000_0000 │ │ 0x8000_0000 └────────────┘ bus@5f000000 { compatible = "simple-bus"; #address-cells = <1>; #size-cells = <1>; ranges = <0x5f000000 0x0 0x5f000000 0x21000000>, <0x80000000 0x0 0x70000000 0x10000000>; pcieb: pcie@5f010000 { compatible = "fsl,imx8q-pcie"; reg = <0x5f010000 0x10000>, <0x8ff00000 0x80000>; reg-names = "dbi", "config"; #address-cells = <3>; #size-cells = <2>; device_type = "pci"; bus-range = <0x00 0xff>; ranges = <0x81000000 0 0x00000000 0x8ff80000 0 0x00010000>, <0x82000000 0 0x80000000 0x80000000 0 0x0ff00000>; ... }; }; 'cpu_untranslate_addr' in of_pci_range can indicate above diagram IA address information. Signed-off-by: Frank Li <Frank.Li@nxp.com> --- Change from v1 to v2 - add cpu_untranslate_addr in of_pci_range, instead adding new API. --- drivers/of/address.c | 2 ++ include/linux/of_address.h | 1 + 2 files changed, 3 insertions(+)