| Message ID | 20201010151235.20585-3-nsaenzjulienne@suse.de |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | None | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
Hi Nicolas, $SUBJECT is out of sync with the patch below. Also, for legibility, it helps if the commit log is intelligible by itself, rather than relying on $SUBJECT being the first line of the first paragraph. On Sat, 10 Oct 2020 at 17:12, Nicolas Saenz Julienne <nsaenzjulienne@suse.de> wrote: > > The function provides the CPU physical address addressable by the most > constrained bus in the system. It might be useful in order to > dynamically set up memory zones during boot. > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > --- > drivers/of/address.c | 34 ++++++++++++++++++++++++++++++++++ > include/linux/of.h | 7 +++++++ > 2 files changed, 41 insertions(+) > > diff --git a/drivers/of/address.c b/drivers/of/address.c > index eb9ab4f1e80b..755e97b65096 100644 > --- a/drivers/of/address.c > +++ b/drivers/of/address.c > @@ -1024,6 +1024,40 @@ int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map) > } > #endif /* CONFIG_HAS_DMA */ > > +/** > + * of_dma_safe_phys_limit - Get system wide DMA safe address space > + * > + * Gets the CPU physical address limit for safe DMA addressing system wide by > + * searching for the most constraining dma-range. Otherwise it returns ~0ULL. > + */ > +u64 __init of_dma_safe_phys_limit(void) I don't think 'safe' strikes the right tone here. You are looking for the highest CPU address that is addressable by all DMA masters in the system. Something like of_dma_get_max_cpu_address(void) perhaps? Also, since this is generic code, phys_addr_t is probably a better type to return. > +{ > + struct device_node *np = NULL; > + struct of_range_parser parser; > + const __be32 *ranges = NULL; I think you can drop these NULL initializers. > + u64 phys_dma_limit = ~0ULL; PHYS_ADDR_MAX > + struct of_range range; > + int len; > + > + for_each_of_allnodes(np) { > + dma_addr_t cpu_end = 0; > + > + ranges = of_get_property(np, "dma-ranges", &len); > + if (!ranges || !len) > + continue; > + > + of_dma_range_parser_init(&parser, np); > + for_each_of_range(&parser, &range) > + if (range.cpu_addr + range.size > cpu_end) > + cpu_end = range.cpu_addr + range.size; > + > + if (phys_dma_limit > cpu_end) > + phys_dma_limit = cpu_end; > + } > + > + return phys_dma_limit; > +} > + > /** > * of_dma_is_coherent - Check if device is coherent > * @np: device node > diff --git a/include/linux/of.h b/include/linux/of.h > index 481ec0467285..958c64cffa92 100644 > --- a/include/linux/of.h > +++ b/include/linux/of.h > @@ -558,6 +558,8 @@ int of_map_id(struct device_node *np, u32 id, > const char *map_name, const char *map_mask_name, > struct device_node **target, u32 *id_out); > > +u64 of_dma_safe_phys_limit(void); > + > #else /* CONFIG_OF */ > > static inline void of_core_init(void) > @@ -995,6 +997,11 @@ static inline int of_map_id(struct device_node *np, u32 id, > return -EINVAL; > } > > +static inline u64 of_dma_safe_phys_limit(void) > +{ > + return ~0ULL; > +} > + > #define of_match_ptr(_ptr) NULL > #define of_match_node(_matches, _node) NULL > #endif /* CONFIG_OF */ > -- > 2.28.0 >
On Sat, Oct 10, 2020 at 10:12 AM Nicolas Saenz Julienne <nsaenzjulienne@suse.de> wrote: > > The function provides the CPU physical address addressable by the most > constrained bus in the system. It might be useful in order to > dynamically set up memory zones during boot. > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > --- > drivers/of/address.c | 34 ++++++++++++++++++++++++++++++++++ > include/linux/of.h | 7 +++++++ > 2 files changed, 41 insertions(+) > > diff --git a/drivers/of/address.c b/drivers/of/address.c > index eb9ab4f1e80b..755e97b65096 100644 > --- a/drivers/of/address.c > +++ b/drivers/of/address.c > @@ -1024,6 +1024,40 @@ int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map) > } > #endif /* CONFIG_HAS_DMA */ > > +/** > + * of_dma_safe_phys_limit - Get system wide DMA safe address space > + * > + * Gets the CPU physical address limit for safe DMA addressing system wide by > + * searching for the most constraining dma-range. Otherwise it returns ~0ULL. > + */ > +u64 __init of_dma_safe_phys_limit(void) > +{ > + struct device_node *np = NULL; > + struct of_range_parser parser; > + const __be32 *ranges = NULL; > + u64 phys_dma_limit = ~0ULL; > + struct of_range range; > + int len; > + > + for_each_of_allnodes(np) { > + dma_addr_t cpu_end = 0; > + > + ranges = of_get_property(np, "dma-ranges", &len); > + if (!ranges || !len) > + continue; > + > + of_dma_range_parser_init(&parser, np); > + for_each_of_range(&parser, &range) > + if (range.cpu_addr + range.size > cpu_end) > + cpu_end = range.cpu_addr + range.size; This doesn't work if you have more than one level of dma-ranges. The address has to be translated first. It should be okay to do that on the start or end address (if not, your DT is broken). Please add/extend a unittest for this. > + > + if (phys_dma_limit > cpu_end) > + phys_dma_limit = cpu_end; > + } > + > + return phys_dma_limit; > +} > + > /** > * of_dma_is_coherent - Check if device is coherent > * @np: device node > diff --git a/include/linux/of.h b/include/linux/of.h > index 481ec0467285..958c64cffa92 100644 > --- a/include/linux/of.h > +++ b/include/linux/of.h > @@ -558,6 +558,8 @@ int of_map_id(struct device_node *np, u32 id, > const char *map_name, const char *map_mask_name, > struct device_node **target, u32 *id_out); > > +u64 of_dma_safe_phys_limit(void); > + > #else /* CONFIG_OF */ > > static inline void of_core_init(void) > @@ -995,6 +997,11 @@ static inline int of_map_id(struct device_node *np, u32 id, > return -EINVAL; > } > > +static inline u64 of_dma_safe_phys_limit(void) > +{ > + return ~0ULL; > +} > + > #define of_match_ptr(_ptr) NULL > #define of_match_node(_matches, _node) NULL > #endif /* CONFIG_OF */ > -- > 2.28.0 >
On Sun, 2020-10-11 at 09:47 +0200, Ard Biesheuvel wrote: > Hi Nicolas, > > $SUBJECT is out of sync with the patch below. Also, for legibility, it > helps if the commit log is intelligible by itself, rather than relying > on $SUBJECT being the first line of the first paragraph. Noted, I'll update all commit logs. > On Sat, 10 Oct 2020 at 17:12, Nicolas Saenz Julienne > <nsaenzjulienne@suse.de> wrote: > > The function provides the CPU physical address addressable by the most > > constrained bus in the system. It might be useful in order to > > dynamically set up memory zones during boot. > > > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > > --- > > drivers/of/address.c | 34 ++++++++++++++++++++++++++++++++++ > > include/linux/of.h | 7 +++++++ > > 2 files changed, 41 insertions(+) > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c > > index eb9ab4f1e80b..755e97b65096 100644 > > --- a/drivers/of/address.c > > +++ b/drivers/of/address.c > > @@ -1024,6 +1024,40 @@ int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map) > > } > > #endif /* CONFIG_HAS_DMA */ > > > > +/** > > + * of_dma_safe_phys_limit - Get system wide DMA safe address space > > + * > > + * Gets the CPU physical address limit for safe DMA addressing system wide by > > + * searching for the most constraining dma-range. Otherwise it returns ~0ULL. > > + */ > > +u64 __init of_dma_safe_phys_limit(void) > > I don't think 'safe' strikes the right tone here. You are looking for > the highest CPU address that is addressable by all DMA masters in the > system. > > Something like > > of_dma_get_max_cpu_address(void) > > perhaps? Also, since this is generic code, phys_addr_t is probably a > better type to return. Sonds good to me, I dindn't like the name I used either. Will use with phys_addr_t. Regards, Nicolas
Hi Rob, On Mon, 2020-10-12 at 10:25 -0500, Rob Herring wrote: > On Sat, Oct 10, 2020 at 10:12 AM Nicolas Saenz Julienne > <nsaenzjulienne@suse.de> wrote: > > The function provides the CPU physical address addressable by the most > > constrained bus in the system. It might be useful in order to > > dynamically set up memory zones during boot. > > > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > > --- > > drivers/of/address.c | 34 ++++++++++++++++++++++++++++++++++ > > include/linux/of.h | 7 +++++++ > > 2 files changed, 41 insertions(+) > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c > > index eb9ab4f1e80b..755e97b65096 100644 > > --- a/drivers/of/address.c > > +++ b/drivers/of/address.c > > @@ -1024,6 +1024,40 @@ int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map) > > } > > #endif /* CONFIG_HAS_DMA */ > > > > +/** > > + * of_dma_safe_phys_limit - Get system wide DMA safe address space > > + * > > + * Gets the CPU physical address limit for safe DMA addressing system wide by > > + * searching for the most constraining dma-range. Otherwise it returns ~0ULL. > > + */ > > +u64 __init of_dma_safe_phys_limit(void) > > +{ > > + struct device_node *np = NULL; > > + struct of_range_parser parser; > > + const __be32 *ranges = NULL; > > + u64 phys_dma_limit = ~0ULL; > > + struct of_range range; > > + int len; > > + > > + for_each_of_allnodes(np) { > > + dma_addr_t cpu_end = 0; > > + > > + ranges = of_get_property(np, "dma-ranges", &len); > > + if (!ranges || !len) > > + continue; > > + > > + of_dma_range_parser_init(&parser, np); > > + for_each_of_range(&parser, &range) > > + if (range.cpu_addr + range.size > cpu_end) > > + cpu_end = range.cpu_addr + range.size; > > This doesn't work if you have more than one level of dma-ranges. The > address has to be translated first. It should be okay to do that on > the start or end address (if not, your DT is broken). for_each_of_range() calls of_pci_range_parser_one() which utimately populates range.cpu_addr with of_translate_dma_address() results. Isn't that good enough? > Please add/extend a unittest for this. Will do. Regards, Nicolas
On Wed, Oct 14, 2020 at 6:52 AM Nicolas Saenz Julienne <nsaenzjulienne@suse.de> wrote: > > Hi Rob, > > On Mon, 2020-10-12 at 10:25 -0500, Rob Herring wrote: > > On Sat, Oct 10, 2020 at 10:12 AM Nicolas Saenz Julienne > > <nsaenzjulienne@suse.de> wrote: > > > The function provides the CPU physical address addressable by the most > > > constrained bus in the system. It might be useful in order to > > > dynamically set up memory zones during boot. > > > > > > Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> > > > --- > > > drivers/of/address.c | 34 ++++++++++++++++++++++++++++++++++ > > > include/linux/of.h | 7 +++++++ > > > 2 files changed, 41 insertions(+) > > > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c > > > index eb9ab4f1e80b..755e97b65096 100644 > > > --- a/drivers/of/address.c > > > +++ b/drivers/of/address.c > > > @@ -1024,6 +1024,40 @@ int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map) > > > } > > > #endif /* CONFIG_HAS_DMA */ > > > > > > +/** > > > + * of_dma_safe_phys_limit - Get system wide DMA safe address space > > > + * > > > + * Gets the CPU physical address limit for safe DMA addressing system wide by > > > + * searching for the most constraining dma-range. Otherwise it returns ~0ULL. > > > + */ > > > +u64 __init of_dma_safe_phys_limit(void) > > > +{ > > > + struct device_node *np = NULL; > > > + struct of_range_parser parser; > > > + const __be32 *ranges = NULL; > > > + u64 phys_dma_limit = ~0ULL; > > > + struct of_range range; > > > + int len; > > > + > > > + for_each_of_allnodes(np) { > > > + dma_addr_t cpu_end = 0; > > > + > > > + ranges = of_get_property(np, "dma-ranges", &len); > > > + if (!ranges || !len) > > > + continue; > > > + > > > + of_dma_range_parser_init(&parser, np); > > > + for_each_of_range(&parser, &range) > > > + if (range.cpu_addr + range.size > cpu_end) > > > + cpu_end = range.cpu_addr + range.size; > > > > This doesn't work if you have more than one level of dma-ranges. The > > address has to be translated first. It should be okay to do that on > > the start or end address (if not, your DT is broken). > > for_each_of_range() calls of_pci_range_parser_one() which utimately populates > range.cpu_addr with of_translate_dma_address() results. Isn't that good enough? Indeed. I guess I was remembering the cases not using for_each_of_range which forgot to do that... Rob
diff --git a/drivers/of/address.c b/drivers/of/address.c index eb9ab4f1e80b..755e97b65096 100644 --- a/drivers/of/address.c +++ b/drivers/of/address.c @@ -1024,6 +1024,40 @@ int of_dma_get_range(struct device_node *np, const struct bus_dma_region **map) } #endif /* CONFIG_HAS_DMA */ +/** + * of_dma_safe_phys_limit - Get system wide DMA safe address space + * + * Gets the CPU physical address limit for safe DMA addressing system wide by + * searching for the most constraining dma-range. Otherwise it returns ~0ULL. + */ +u64 __init of_dma_safe_phys_limit(void) +{ + struct device_node *np = NULL; + struct of_range_parser parser; + const __be32 *ranges = NULL; + u64 phys_dma_limit = ~0ULL; + struct of_range range; + int len; + + for_each_of_allnodes(np) { + dma_addr_t cpu_end = 0; + + ranges = of_get_property(np, "dma-ranges", &len); + if (!ranges || !len) + continue; + + of_dma_range_parser_init(&parser, np); + for_each_of_range(&parser, &range) + if (range.cpu_addr + range.size > cpu_end) + cpu_end = range.cpu_addr + range.size; + + if (phys_dma_limit > cpu_end) + phys_dma_limit = cpu_end; + } + + return phys_dma_limit; +} + /** * of_dma_is_coherent - Check if device is coherent * @np: device node diff --git a/include/linux/of.h b/include/linux/of.h index 481ec0467285..958c64cffa92 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -558,6 +558,8 @@ int of_map_id(struct device_node *np, u32 id, const char *map_name, const char *map_mask_name, struct device_node **target, u32 *id_out); +u64 of_dma_safe_phys_limit(void); + #else /* CONFIG_OF */ static inline void of_core_init(void) @@ -995,6 +997,11 @@ static inline int of_map_id(struct device_node *np, u32 id, return -EINVAL; } +static inline u64 of_dma_safe_phys_limit(void) +{ + return ~0ULL; +} + #define of_match_ptr(_ptr) NULL #define of_match_node(_matches, _node) NULL #endif /* CONFIG_OF */
The function provides the CPU physical address addressable by the most constrained bus in the system. It might be useful in order to dynamically set up memory zones during boot. Signed-off-by: Nicolas Saenz Julienne <nsaenzjulienne@suse.de> --- drivers/of/address.c | 34 ++++++++++++++++++++++++++++++++++ include/linux/of.h | 7 +++++++ 2 files changed, 41 insertions(+)