| Message ID | 20191213084748.11210-4-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | Add support for PCIe controller to work in endpoint mode on R-Car SoCs | expand |
On Fri, Dec 13, 2019 at 2:48 AM Lad Prabhakar <prabhakar.csengg@gmail.com> wrote: > > From: "Lad, Prabhakar" <prabhakar.mahadev-lad.rj@bp.renesas.com> > > this patch adds support to parse PCI outbound-ranges, the > outbound-regions are similar to pci ranges except it doesn't > have pci address, below is the format for bar-ranges: > > outbound-ranges = <flags upper32_cpuaddr lower32_cpuaddr > upper32_size lower32_size>; You can't just make up a new ranges property. Especially one that doesn't follow how 'ranges' works. We already have 'dma-ranges' to translate device to memory addresses. Explain the problem or feature you need, not the solution you came up with. Why do you need this and other endpoint bindings haven't? Rob
On Fri, Dec 13, 2019 at 08:47:45AM +0000, Lad Prabhakar wrote: > From: "Lad, Prabhakar" <prabhakar.mahadev-lad.rj@bp.renesas.com> $ git log --oneline drivers/of/address.c 951d48855d86 of: Make of_dma_get_range() work on bus nodes 645c138636de of/address: Fix of_pci_range_parser_one translation of DMA addresses 81db12ee15cb of/address: Translate 'dma-ranges' for parent nodes missing 'dma-ranges' b68ac8dc22eb of: Factor out #{addr,size}-cells parsing c60bf3eb888a of: address: Follow DMA parent for "dma-coherent" 862ab5578f75 of/address: Introduce of_get_next_dma_parent() helper Make yours match. There are a few "of: address: " subjects, but the ones from Rob (the maintainer) use "of/address: ", so I'd use that. > this patch adds support to parse PCI outbound-ranges, the > outbound-regions are similar to pci ranges except it doesn't > have pci address, below is the format for bar-ranges: s/pci/PCI/ Capitalize sentences. Is "bar-range" an actual DT property? If it's supposed to be a generic description, "BAR range" would be better. > outbound-ranges = <flags upper32_cpuaddr lower32_cpuaddr > upper32_size lower32_size>;
Hi Rob, Thank you for the review. On Fri, Dec 13, 2019 at 8:37 PM Rob Herring <robh+dt@kernel.org> wrote: > > On Fri, Dec 13, 2019 at 2:48 AM Lad Prabhakar > <prabhakar.csengg@gmail.com> wrote: > > > > From: "Lad, Prabhakar" <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > this patch adds support to parse PCI outbound-ranges, the > > outbound-regions are similar to pci ranges except it doesn't > > have pci address, below is the format for bar-ranges: > > > > outbound-ranges = <flags upper32_cpuaddr lower32_cpuaddr > > upper32_size lower32_size>; > > You can't just make up a new ranges property. Especially one that > doesn't follow how 'ranges' works. We already have 'dma-ranges' to > translate device to memory addresses. > > Explain the problem or feature you need, not the solution you came up > with. Why do you need this and other endpoint bindings haven't? > rcar SoC's supports multiple outbound region for mapping the PCI address locally to the system. This lead to discussion where there exist controllers which support regions for high/low priority transfer and similarly regions for large/small memory allocations, as a result a new ranges property was added, where we can specify the flags which would indicate how the outbound region can be used during requests. The current endpoint controller drivers just support single region. Cheers, --Prabhakar
Hi Bjorn, Thank you for the review. On Fri, Dec 13, 2019 at 9:05 PM Bjorn Helgaas <helgaas@kernel.org> wrote: > > On Fri, Dec 13, 2019 at 08:47:45AM +0000, Lad Prabhakar wrote: > > From: "Lad, Prabhakar" <prabhakar.mahadev-lad.rj@bp.renesas.com> > > $ git log --oneline drivers/of/address.c > 951d48855d86 of: Make of_dma_get_range() work on bus nodes > 645c138636de of/address: Fix of_pci_range_parser_one translation of DMA addresses > 81db12ee15cb of/address: Translate 'dma-ranges' for parent nodes missing 'dma-ranges' > b68ac8dc22eb of: Factor out #{addr,size}-cells parsing > c60bf3eb888a of: address: Follow DMA parent for "dma-coherent" > 862ab5578f75 of/address: Introduce of_get_next_dma_parent() helper > > Make yours match. There are a few "of: address: " subjects, but the > ones from Rob (the maintainer) use "of/address: ", so I'd use that. > will do the same for next iteration. > > this patch adds support to parse PCI outbound-ranges, the > > outbound-regions are similar to pci ranges except it doesn't > > have pci address, below is the format for bar-ranges: > > s/pci/PCI/ > Capitalize sentences. > will fix that. > Is "bar-range" an actual DT property? If it's supposed to be a > generic description, "BAR range" would be better. > my bad, it should be outbound-range. Cheers, --Prabhakar > > outbound-ranges = <flags upper32_cpuaddr lower32_cpuaddr > > upper32_size lower32_size>;
On Mon, Dec 16, 2019 at 08:49:23AM +0000, Lad, Prabhakar wrote: > Hi Rob, > > Thank you for the review. > > On Fri, Dec 13, 2019 at 8:37 PM Rob Herring <robh+dt@kernel.org> wrote: > > > > On Fri, Dec 13, 2019 at 2:48 AM Lad Prabhakar > > <prabhakar.csengg@gmail.com> wrote: > > > > > > From: "Lad, Prabhakar" <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > this patch adds support to parse PCI outbound-ranges, the > > > outbound-regions are similar to pci ranges except it doesn't > > > have pci address, below is the format for bar-ranges: > > > > > > outbound-ranges = <flags upper32_cpuaddr lower32_cpuaddr > > > upper32_size lower32_size>; > > > > You can't just make up a new ranges property. Especially one that > > doesn't follow how 'ranges' works. We already have 'dma-ranges' to > > translate device to memory addresses. > > > > Explain the problem or feature you need, not the solution you came up > > with. Why do you need this and other endpoint bindings haven't? > > > rcar SoC's supports multiple outbound region for mapping the PCI address > locally to the system. This lead to discussion where there exist controllers > which support regions for high/low priority transfer and similarly regions > for large/small memory allocations, as a result a new ranges property was > added, where we can specify the flags which would indicate how the outbound > region can be used during requests. What are the flags? Rob
Hi Rob, On Thu, Dec 19, 2019 at 11:31 PM Rob Herring <robh@kernel.org> wrote: > > On Mon, Dec 16, 2019 at 08:49:23AM +0000, Lad, Prabhakar wrote: > > Hi Rob, > > > > Thank you for the review. > > > > On Fri, Dec 13, 2019 at 8:37 PM Rob Herring <robh+dt@kernel.org> wrote: > > > > > > On Fri, Dec 13, 2019 at 2:48 AM Lad Prabhakar > > > <prabhakar.csengg@gmail.com> wrote: > > > > > > > > From: "Lad, Prabhakar" <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > this patch adds support to parse PCI outbound-ranges, the > > > > outbound-regions are similar to pci ranges except it doesn't > > > > have pci address, below is the format for bar-ranges: > > > > > > > > outbound-ranges = <flags upper32_cpuaddr lower32_cpuaddr > > > > upper32_size lower32_size>; > > > > > > You can't just make up a new ranges property. Especially one that > > > doesn't follow how 'ranges' works. We already have 'dma-ranges' to > > > translate device to memory addresses. > > > > > > Explain the problem or feature you need, not the solution you came up > > > with. Why do you need this and other endpoint bindings haven't? > > > > > rcar SoC's supports multiple outbound region for mapping the PCI address > > locally to the system. This lead to discussion where there exist controllers > > which support regions for high/low priority transfer and similarly regions > > for large/small memory allocations, as a result a new ranges property was > > added, where we can specify the flags which would indicate how the outbound > > region can be used during requests. > > What are the flags? below are the flags which were discussed in first version of the series, but since the driver is currently using just PCI_EPC_WINDOW_FLAG_NON_MULTI_ALLOC flag I'll be dropping them in next version (suggested by Kishon) and rest will be added as and when required by the driver. * @PCI_EPC_WINDOW_FLAG_MULTI_ALLOC: Indicates multiple chunks of memory can be * allocated from same window * @PCI_EPC_WINDOW_FLAG_NON_MULTI_ALLOC: Indicates only single memory allocation * is possible on the window * @PCI_EPC_WINDOW_FLAG_LARGE_ALLOC: Window is used for large memory allocation * @PCI_EPC_WINDOW_FLAG_SMALL_ALLOC: Window is used for small memory allocation * @PCI_EPC_WINDOW_FLAG_HIGH_PRI_ALLOC: Window is used for high priority data * transfers * @PCI_EPC_WINDOW_FLAG_LOW_PRI_ALLOC: Window is used for low priority data * transfers Cheers, --Prabhakar
On Thu, Jan 2, 2020 at 1:44 AM Lad, Prabhakar <prabhakar.csengg@gmail.com> wrote: > > Hi Rob, > > On Thu, Dec 19, 2019 at 11:31 PM Rob Herring <robh@kernel.org> wrote: > > > > On Mon, Dec 16, 2019 at 08:49:23AM +0000, Lad, Prabhakar wrote: > > > Hi Rob, > > > > > > Thank you for the review. > > > > > > On Fri, Dec 13, 2019 at 8:37 PM Rob Herring <robh+dt@kernel.org> wrote: > > > > > > > > On Fri, Dec 13, 2019 at 2:48 AM Lad Prabhakar > > > > <prabhakar.csengg@gmail.com> wrote: > > > > > > > > > > From: "Lad, Prabhakar" <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > > > > > > > this patch adds support to parse PCI outbound-ranges, the > > > > > outbound-regions are similar to pci ranges except it doesn't > > > > > have pci address, below is the format for bar-ranges: > > > > > > > > > > outbound-ranges = <flags upper32_cpuaddr lower32_cpuaddr > > > > > upper32_size lower32_size>; > > > > > > > > You can't just make up a new ranges property. Especially one that > > > > doesn't follow how 'ranges' works. We already have 'dma-ranges' to > > > > translate device to memory addresses. > > > > > > > > Explain the problem or feature you need, not the solution you came up > > > > with. Why do you need this and other endpoint bindings haven't? > > > > > > > rcar SoC's supports multiple outbound region for mapping the PCI address > > > locally to the system. This lead to discussion where there exist controllers > > > which support regions for high/low priority transfer and similarly regions > > > for large/small memory allocations, as a result a new ranges property was > > > added, where we can specify the flags which would indicate how the outbound > > > region can be used during requests. > > > > What are the flags? > > below are the flags which were discussed in first version of the > series, but since the driver is > currently using just PCI_EPC_WINDOW_FLAG_NON_MULTI_ALLOC flag I'll be > dropping them in > next version (suggested by Kishon) and rest will be added as and when > required by the driver. > > * @PCI_EPC_WINDOW_FLAG_MULTI_ALLOC: Indicates multiple chunks of memory can be > * allocated from same window > * @PCI_EPC_WINDOW_FLAG_NON_MULTI_ALLOC: Indicates only single memory allocation > * is possible on the window > * @PCI_EPC_WINDOW_FLAG_LARGE_ALLOC: Window is used for large memory allocation > * @PCI_EPC_WINDOW_FLAG_SMALL_ALLOC: Window is used for small memory allocation > * @PCI_EPC_WINDOW_FLAG_HIGH_PRI_ALLOC: Window is used for high priority data > * transfers > * @PCI_EPC_WINDOW_FLAG_LOW_PRI_ALLOC: Window is used for low priority data > * transfers Looks like configuration or policy, not something that belongs in DT. Coupling driver features and DT changes is not good for ABI compatible changes either. I'm hesitant to accept any PCI endpoint binding additions because they don't seem to be completely thought out in terms of supporting different usecases. Rob
diff --git a/drivers/of/address.c b/drivers/of/address.c index 978427a..ca4643c 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -233,9 +233,9 @@ int of_pci_address_to_resource(struct device_node *dev, int bar, EXPORT_SYMBOL_GPL(of_pci_address_to_resource); static int parser_init(struct of_pci_range_parser *parser, - struct device_node *node, const char *name) + struct device_node *node, const char *name, + const int na, const int ns) { - const int na = 3, ns = 2; int rlen; parser->node = node; @@ -254,17 +254,30 @@ static int parser_init(struct of_pci_range_parser *parser, int of_pci_range_parser_init(struct of_pci_range_parser *parser, struct device_node *node) { - return parser_init(parser, node, "ranges"); + const int na = 3, ns = 2; + + return parser_init(parser, node, "ranges", na, ns); } EXPORT_SYMBOL_GPL(of_pci_range_parser_init); int of_pci_dma_range_parser_init(struct of_pci_range_parser *parser, struct device_node *node) { - return parser_init(parser, node, "dma-ranges"); + const int na = 3, ns = 2; + + return parser_init(parser, node, "dma-ranges", na, ns); } EXPORT_SYMBOL_GPL(of_pci_dma_range_parser_init); +int of_pci_outbound_range_parser_init(struct of_pci_range_parser *parser, + struct device_node *node) +{ + const int na = 1, ns = 2; + + return parser_init(parser, node, "outbound-ranges", na, ns); +} +EXPORT_SYMBOL_GPL(of_pci_outbound_range_parser_init); + struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser, struct of_pci_range *range) { @@ -310,6 +323,29 @@ struct of_pci_range *of_pci_range_parser_one(struct of_pci_range_parser *parser, } EXPORT_SYMBOL_GPL(of_pci_range_parser_one); +struct of_pci_outbound_range +*of_pci_outbound_range_parser_one(struct of_pci_range_parser *parser, + struct of_pci_outbound_range *range) +{ + const int na = 1, ns = 2; + + if (!range) + return NULL; + + if (!parser->range || parser->range + parser->np > parser->end) + return NULL; + + range->flags = be32_to_cpup(parser->range); + range->cpu_addr = of_translate_address(parser->node, + parser->range + na); + range->size = of_read_number(parser->range + parser->pna + na, ns); + + parser->range += parser->np; + + return range; +} +EXPORT_SYMBOL_GPL(of_pci_outbound_range_parser_one); + /* * of_pci_range_to_resource - Create a resource from an of_pci_range * @range: the PCI range that describes the resource diff --git a/include/linux/of_address.h b/include/linux/of_address.h index 30e40fb..93b3be3 100644 --- a/include/linux/of_address.h +++ b/include/linux/of_address.h @@ -22,9 +22,18 @@ struct of_pci_range { u32 flags; }; +struct of_pci_outbound_range { + u32 flags; + u64 cpu_addr; + u64 size; +}; + #define for_each_of_pci_range(parser, range) \ for (; of_pci_range_parser_one(parser, range);) +#define for_each_of_pci_outbound_range(parser, range) \ + for (; of_pci_outbound_range_parser_one(parser, range);) + /* Translate a DMA address from device space to CPU space */ extern u64 of_translate_dma_address(struct device_node *dev, const __be32 *in_addr); @@ -52,9 +61,14 @@ extern int of_pci_range_parser_init(struct of_pci_range_parser *parser, struct device_node *node); extern int of_pci_dma_range_parser_init(struct of_pci_range_parser *parser, struct device_node *node); +extern int of_pci_outbound_range_parser_init(struct of_pci_range_parser *parser, + struct device_node *node); extern struct of_pci_range *of_pci_range_parser_one( struct of_pci_range_parser *parser, struct of_pci_range *range); +extern struct of_pci_outbound_range +*of_pci_outbound_range_parser_one(struct of_pci_range_parser *parser, + struct of_pci_outbound_range *range); extern int of_dma_get_range(struct device_node *np, u64 *dma_addr, u64 *paddr, u64 *size); extern bool of_dma_is_coherent(struct device_node *np); @@ -97,6 +111,13 @@ static inline int of_pci_dma_range_parser_init(struct of_pci_range_parser *parse return -ENOSYS; } +static inline int +of_pci_outbound_range_parser_init(struct of_pci_range_parser *parser, + struct device_node *node) +{ + return -ENOSYS; +} + static inline struct of_pci_range *of_pci_range_parser_one( struct of_pci_range_parser *parser, struct of_pci_range *range)