* [RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API @ 2015-01-07 15:29 ` Lorenzo Pieralisi 0 siblings, 0 replies; 32+ messages in thread From: Lorenzo Pieralisi @ 2015-01-07 15:29 UTC (permalink / raw) To: linux-pci, linux-arm-kernel, devicetree Cc: Lorenzo Pieralisi, Arnd Bergmann, Liviu Dudau, Mohit Kumar, Jingoo Han, Bjorn Helgaas, Rob Herring The introduction of the DT API: of_pci_get_host_bridge_resources() allows to retrieve PCI resources from the device tree in a generic way. Some PCI host controllers, in addition to standard IO and MEM resources and corresponding CPU addresses, require to retrieve the addresses that are used for bus routing of IO/MEM regions as seen at the bus level corresponding to the bus segment the host controller is sitting on, so that the host controller can programme its inbound/outbound memory regions correctly to respond to bus transactions coming from the CPU on the host controller bus ports. The DT generic layer provides functions that carry out the PCI ranges <-> resources translation from PCI addresses to CPU addresses (as seen at the CPU top level bus, through all required hierarchical bus layers), but there is no generic API to retrieve the PCI range CPU untranslated addresses, so PCI host controllers have to do it in a driver specific way through the DT property parsing methods. This patchset converts the PCIe designware host controller driver to use the API: of_pci_get_host_bridge_resources() in order to retrieve the PCI regions resources. To achieve generality, it also augments the parsing API so that a new DT struct is created out of PCI ranges, that contains the PCI resource and the range parser itself so that it can be passed back to PCI host controllers drivers that can, if needed, use it to parse the CPU untranslated addresses corresponding to the resource in question. The first patch in the series is a fix/update for the parsing function: of_pci_get_host_bridge_resources() It is part of the RFC series since it is a controversial fix to be discussed before getting merged. Patch [2-3] augment the parsing API and convert the PCIe designware driver code. The current API stays unchanged, so that the existing drivers using it do not need patching and changes are limited. Tested on an iMX6 Sabrelite board. Cc: Arnd Bergmann <arnd@arndb.de> Cc: Liviu Dudau <liviu.dudau@arm.com> Cc: Mohit Kumar <mohit.kumar@st.com> Cc: Jingoo Han <jg1.han@samsung.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Rob Herring <robh+dt@kernel.org> Lorenzo Pieralisi (3): drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() drivers: of: of_pci_get_host_bridge_resources() range parsing update drivers: pci: host: update the pcie designware driver to new range parsing API drivers/of/of_pci.c | 36 +++++++-- drivers/pci/host/pcie-designware.c | 145 +++++++++++++++++-------------------- drivers/pci/host/pcie-designware.h | 5 +- include/linux/of_address.h | 5 ++ 4 files changed, 104 insertions(+), 87 deletions(-) -- 2.2.1 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API @ 2015-01-07 15:29 ` Lorenzo Pieralisi 0 siblings, 0 replies; 32+ messages in thread From: Lorenzo Pieralisi @ 2015-01-07 15:29 UTC (permalink / raw) To: linux-arm-kernel The introduction of the DT API: of_pci_get_host_bridge_resources() allows to retrieve PCI resources from the device tree in a generic way. Some PCI host controllers, in addition to standard IO and MEM resources and corresponding CPU addresses, require to retrieve the addresses that are used for bus routing of IO/MEM regions as seen at the bus level corresponding to the bus segment the host controller is sitting on, so that the host controller can programme its inbound/outbound memory regions correctly to respond to bus transactions coming from the CPU on the host controller bus ports. The DT generic layer provides functions that carry out the PCI ranges <-> resources translation from PCI addresses to CPU addresses (as seen at the CPU top level bus, through all required hierarchical bus layers), but there is no generic API to retrieve the PCI range CPU untranslated addresses, so PCI host controllers have to do it in a driver specific way through the DT property parsing methods. This patchset converts the PCIe designware host controller driver to use the API: of_pci_get_host_bridge_resources() in order to retrieve the PCI regions resources. To achieve generality, it also augments the parsing API so that a new DT struct is created out of PCI ranges, that contains the PCI resource and the range parser itself so that it can be passed back to PCI host controllers drivers that can, if needed, use it to parse the CPU untranslated addresses corresponding to the resource in question. The first patch in the series is a fix/update for the parsing function: of_pci_get_host_bridge_resources() It is part of the RFC series since it is a controversial fix to be discussed before getting merged. Patch [2-3] augment the parsing API and convert the PCIe designware driver code. The current API stays unchanged, so that the existing drivers using it do not need patching and changes are limited. Tested on an iMX6 Sabrelite board. Cc: Arnd Bergmann <arnd@arndb.de> Cc: Liviu Dudau <liviu.dudau@arm.com> Cc: Mohit Kumar <mohit.kumar@st.com> Cc: Jingoo Han <jg1.han@samsung.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Rob Herring <robh+dt@kernel.org> Lorenzo Pieralisi (3): drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() drivers: of: of_pci_get_host_bridge_resources() range parsing update drivers: pci: host: update the pcie designware driver to new range parsing API drivers/of/of_pci.c | 36 +++++++-- drivers/pci/host/pcie-designware.c | 145 +++++++++++++++++-------------------- drivers/pci/host/pcie-designware.h | 5 +- include/linux/of_address.h | 5 ++ 4 files changed, 104 insertions(+), 87 deletions(-) -- 2.2.1 ^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() 2015-01-07 15:29 ` Lorenzo Pieralisi @ 2015-01-07 15:29 ` Lorenzo Pieralisi -1 siblings, 0 replies; 32+ messages in thread From: Lorenzo Pieralisi @ 2015-01-07 15:29 UTC (permalink / raw) To: linux-pci, linux-arm-kernel, devicetree Cc: Lorenzo Pieralisi, Arnd Bergmann, Liviu Dudau, Mohit Kumar, Jingoo Han, Bjorn Helgaas, Rob Herring In the function of_pci_get_host_bridge_resources() if the parsing of ranges fails, previously allocated resources inclusive of bus_range are not freed and are not expected to be freed by the function caller on error return. This patch fixes the issues by adding code that properly frees resources and bus_range before exiting the function with an error return value. Cc: Arnd Bergmann <arnd@arndb.de> Cc: Liviu Dudau <liviu.dudau@arm.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Rob Herring <robh+dt@kernel.org> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> --- drivers/of/of_pci.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c index 88471d3..6fbfe99 100644 --- a/drivers/of/of_pci.c +++ b/drivers/of/of_pci.c @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, struct of_pci_range_parser parser; char range_type[4]; int err; + struct pci_host_bridge_window *window; if (io_base) *io_base = (resource_size_t)OF_BAD_ADDR; @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, conversion_failed: kfree(res); parse_failed: + list_for_each_entry(window, resources, list) + kfree(window->res); pci_free_resource_list(resources); + kfree(bus_range); return err; } EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); -- 2.2.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() @ 2015-01-07 15:29 ` Lorenzo Pieralisi 0 siblings, 0 replies; 32+ messages in thread From: Lorenzo Pieralisi @ 2015-01-07 15:29 UTC (permalink / raw) To: linux-arm-kernel In the function of_pci_get_host_bridge_resources() if the parsing of ranges fails, previously allocated resources inclusive of bus_range are not freed and are not expected to be freed by the function caller on error return. This patch fixes the issues by adding code that properly frees resources and bus_range before exiting the function with an error return value. Cc: Arnd Bergmann <arnd@arndb.de> Cc: Liviu Dudau <liviu.dudau@arm.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Rob Herring <robh+dt@kernel.org> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> --- drivers/of/of_pci.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c index 88471d3..6fbfe99 100644 --- a/drivers/of/of_pci.c +++ b/drivers/of/of_pci.c @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, struct of_pci_range_parser parser; char range_type[4]; int err; + struct pci_host_bridge_window *window; if (io_base) *io_base = (resource_size_t)OF_BAD_ADDR; @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, conversion_failed: kfree(res); parse_failed: + list_for_each_entry(window, resources, list) + kfree(window->res); pci_free_resource_list(resources); + kfree(bus_range); return err; } EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); -- 2.2.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() 2015-01-07 15:29 ` Lorenzo Pieralisi @ 2015-01-19 18:32 ` Liviu Dudau -1 siblings, 0 replies; 32+ messages in thread From: Liviu Dudau @ 2015-01-19 18:32 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: linux-pci, linux-arm-kernel, devicetree, Arnd Bergmann, Mohit Kumar, Jingoo Han, Bjorn Helgaas, Rob Herring On Wed, Jan 07, 2015 at 03:29:29PM +0000, Lorenzo Pieralisi wrote: > In the function of_pci_get_host_bridge_resources() if the parsing of > ranges fails, previously allocated resources inclusive of bus_range > are not freed and are not expected to be freed by the function caller > on error return. > > This patch fixes the issues by adding code that properly frees resources > and bus_range before exiting the function with an error return value. > > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Liviu Dudau <liviu.dudau@arm.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Rob Herring <robh+dt@kernel.org> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > --- > drivers/of/of_pci.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c > index 88471d3..6fbfe99 100644 > --- a/drivers/of/of_pci.c > +++ b/drivers/of/of_pci.c > @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > struct of_pci_range_parser parser; > char range_type[4]; > int err; > + struct pci_host_bridge_window *window; > > if (io_base) > *io_base = (resource_size_t)OF_BAD_ADDR; > @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > conversion_failed: > kfree(res); > parse_failed: > + list_for_each_entry(window, resources, list) > + kfree(window->res); > pci_free_resource_list(resources); > + kfree(bus_range); > return err; > } > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); Hi Lorenzo et all, Here is my personal view and I am happy to hear from others on the desired behaviour: When I wrote this function what I had in mind was that it will parse as much as possible from the device tree and return a list of resources that could be successfully converted. If the entire list of ranges could not be converted then an error code will be returned, but the caller still had the list as constructed up to the error. It was the job of the caller to free the list in either cases, as stated in the comment. The historical reason why the function was written that way was because at some moment after parsing I've had an additional step where arches could cleanup / veto the list and they could return an error value to signal their discontent. Also I was (am) not sure how lenient we could be with the device tree not being sane (at least one host bridge binding lists the config space as a range, which was accepted as broken). So, from that point of view, I would NAK this patch, as the function works as intended. If others find this mode of operation too convoluted, then the patch should probably make clear that cleanup only needs to be done on function returning success. Best regards, > -- > 2.2.1 > > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯ ^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() @ 2015-01-19 18:32 ` Liviu Dudau 0 siblings, 0 replies; 32+ messages in thread From: Liviu Dudau @ 2015-01-19 18:32 UTC (permalink / raw) To: linux-arm-kernel On Wed, Jan 07, 2015 at 03:29:29PM +0000, Lorenzo Pieralisi wrote: > In the function of_pci_get_host_bridge_resources() if the parsing of > ranges fails, previously allocated resources inclusive of bus_range > are not freed and are not expected to be freed by the function caller > on error return. > > This patch fixes the issues by adding code that properly frees resources > and bus_range before exiting the function with an error return value. > > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Liviu Dudau <liviu.dudau@arm.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Rob Herring <robh+dt@kernel.org> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > --- > drivers/of/of_pci.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c > index 88471d3..6fbfe99 100644 > --- a/drivers/of/of_pci.c > +++ b/drivers/of/of_pci.c > @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > struct of_pci_range_parser parser; > char range_type[4]; > int err; > + struct pci_host_bridge_window *window; > > if (io_base) > *io_base = (resource_size_t)OF_BAD_ADDR; > @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > conversion_failed: > kfree(res); > parse_failed: > + list_for_each_entry(window, resources, list) > + kfree(window->res); > pci_free_resource_list(resources); > + kfree(bus_range); > return err; > } > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); Hi Lorenzo et all, Here is my personal view and I am happy to hear from others on the desired behaviour: When I wrote this function what I had in mind was that it will parse as much as possible from the device tree and return a list of resources that could be successfully converted. If the entire list of ranges could not be converted then an error code will be returned, but the caller still had the list as constructed up to the error. It was the job of the caller to free the list in either cases, as stated in the comment. The historical reason why the function was written that way was because at some moment after parsing I've had an additional step where arches could cleanup / veto the list and they could return an error value to signal their discontent. Also I was (am) not sure how lenient we could be with the device tree not being sane (at least one host bridge binding lists the config space as a range, which was accepted as broken). So, from that point of view, I would NAK this patch, as the function works as intended. If others find this mode of operation too convoluted, then the patch should probably make clear that cleanup only needs to be done on function returning success. Best regards, > -- > 2.2.1 > > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ?\_(?)_/? ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() 2015-01-19 18:32 ` Liviu Dudau (?) @ 2015-01-20 10:49 ` Lorenzo Pieralisi -1 siblings, 0 replies; 32+ messages in thread From: Lorenzo Pieralisi @ 2015-01-20 10:49 UTC (permalink / raw) To: Liviu Dudau Cc: devicetree, Arnd Bergmann, linux-pci, Jingoo Han, Rob Herring, Mohit Kumar, Bjorn Helgaas, linux-arm-kernel On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote: [...] > > @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > struct of_pci_range_parser parser; > > char range_type[4]; > > int err; > > + struct pci_host_bridge_window *window; > > > > if (io_base) > > *io_base = (resource_size_t)OF_BAD_ADDR; > > @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > conversion_failed: > > kfree(res); > > parse_failed: > > + list_for_each_entry(window, resources, list) > > + kfree(window->res); > > pci_free_resource_list(resources); > > + kfree(bus_range); > > return err; > > } > > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); > > Hi Lorenzo et all, > > Here is my personal view and I am happy to hear from others on the desired > behaviour: > > When I wrote this function what I had in mind was that it will parse as > much as possible from the device tree and return a list of resources that > could be successfully converted. If the entire list of ranges could not > be converted then an error code will be returned, but the caller still > had the list as constructed up to the error. It was the job of the caller > to free the list in either cases, as stated in the comment. That's what I am questioning. If the function takes an error path, the windows list is freed, so the resource pointers are gone. There is no way the caller can grab those resource pointers and free them. So either way, the function needs patching. Either we do not free the windows list (we remove pci_free_resource_list) or we apply my fix (or we refactor the API which is likely to be what I will do). Lorenzo > > The historical reason why the function was written that way was because at > some moment after parsing I've had an additional step where arches could > cleanup / veto the list and they could return an error value to signal > their discontent. Also I was (am) not sure how lenient we could be with > the device tree not being sane (at least one host bridge binding lists the > config space as a range, which was accepted as broken). > > So, from that point of view, I would NAK this patch, as the function works > as intended. If others find this mode of operation too convoluted, then > the patch should probably make clear that cleanup only needs to be done on > function returning success. > > Best regards, > > > -- > > 2.2.1 > > > > > > -- > ==================== > | I would like to | > | fix the world, | > | but they're not | > | giving me the | > \ source code! / > --------------- > ¯\_(ツ)_/¯ _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() @ 2015-01-20 10:49 ` Lorenzo Pieralisi 0 siblings, 0 replies; 32+ messages in thread From: Lorenzo Pieralisi @ 2015-01-20 10:49 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote: [...] > > @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > struct of_pci_range_parser parser; > > char range_type[4]; > > int err; > > + struct pci_host_bridge_window *window; > > > > if (io_base) > > *io_base = (resource_size_t)OF_BAD_ADDR; > > @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > conversion_failed: > > kfree(res); > > parse_failed: > > + list_for_each_entry(window, resources, list) > > + kfree(window->res); > > pci_free_resource_list(resources); > > + kfree(bus_range); > > return err; > > } > > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); > > Hi Lorenzo et all, > > Here is my personal view and I am happy to hear from others on the desired > behaviour: > > When I wrote this function what I had in mind was that it will parse as > much as possible from the device tree and return a list of resources that > could be successfully converted. If the entire list of ranges could not > be converted then an error code will be returned, but the caller still > had the list as constructed up to the error. It was the job of the caller > to free the list in either cases, as stated in the comment. That's what I am questioning. If the function takes an error path, the windows list is freed, so the resource pointers are gone. There is no way the caller can grab those resource pointers and free them. So either way, the function needs patching. Either we do not free the windows list (we remove pci_free_resource_list) or we apply my fix (or we refactor the API which is likely to be what I will do). Lorenzo > > The historical reason why the function was written that way was because at > some moment after parsing I've had an additional step where arches could > cleanup / veto the list and they could return an error value to signal > their discontent. Also I was (am) not sure how lenient we could be with > the device tree not being sane (at least one host bridge binding lists the > config space as a range, which was accepted as broken). > > So, from that point of view, I would NAK this patch, as the function works > as intended. If others find this mode of operation too convoluted, then > the patch should probably make clear that cleanup only needs to be done on > function returning success. > > Best regards, > > > -- > > 2.2.1 > > > > > > -- > ==================== > | I would like to | > | fix the world, | > | but they're not | > | giving me the | > \ source code! / > --------------- > ?\_(?)_/? ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() @ 2015-01-20 10:49 ` Lorenzo Pieralisi 0 siblings, 0 replies; 32+ messages in thread From: Lorenzo Pieralisi @ 2015-01-20 10:49 UTC (permalink / raw) To: Liviu Dudau Cc: linux-pci, linux-arm-kernel, devicetree, Arnd Bergmann, Mohit Kumar, Jingoo Han, Bjorn Helgaas, Rob Herring On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote: [...] > > @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > struct of_pci_range_parser parser; > > char range_type[4]; > > int err; > > + struct pci_host_bridge_window *window; > > > > if (io_base) > > *io_base = (resource_size_t)OF_BAD_ADDR; > > @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > conversion_failed: > > kfree(res); > > parse_failed: > > + list_for_each_entry(window, resources, list) > > + kfree(window->res); > > pci_free_resource_list(resources); > > + kfree(bus_range); > > return err; > > } > > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); > > Hi Lorenzo et all, > > Here is my personal view and I am happy to hear from others on the desired > behaviour: > > When I wrote this function what I had in mind was that it will parse as > much as possible from the device tree and return a list of resources that > could be successfully converted. If the entire list of ranges could not > be converted then an error code will be returned, but the caller still > had the list as constructed up to the error. It was the job of the caller > to free the list in either cases, as stated in the comment. That's what I am questioning. If the function takes an error path, the windows list is freed, so the resource pointers are gone. There is no way the caller can grab those resource pointers and free them. So either way, the function needs patching. Either we do not free the windows list (we remove pci_free_resource_list) or we apply my fix (or we refactor the API which is likely to be what I will do). Lorenzo > > The historical reason why the function was written that way was because at > some moment after parsing I've had an additional step where arches could > cleanup / veto the list and they could return an error value to signal > their discontent. Also I was (am) not sure how lenient we could be with > the device tree not being sane (at least one host bridge binding lists the > config space as a range, which was accepted as broken). > > So, from that point of view, I would NAK this patch, as the function works > as intended. If others find this mode of operation too convoluted, then > the patch should probably make clear that cleanup only needs to be done on > function returning success. > > Best regards, > > > -- > > 2.2.1 > > > > > > -- > ==================== > | I would like to | > | fix the world, | > | but they're not | > | giving me the | > \ source code! / > --------------- > ¯\_(ツ)_/¯ ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() 2015-01-20 10:49 ` Lorenzo Pieralisi (?) @ 2015-01-20 11:20 ` Liviu Dudau -1 siblings, 0 replies; 32+ messages in thread From: Liviu Dudau @ 2015-01-20 11:20 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: devicetree, Arnd Bergmann, linux-pci, Jingoo Han, Liviu Dudau, Rob Herring, Mohit Kumar, Bjorn Helgaas, linux-arm-kernel On Tue, Jan 20, 2015 at 10:49:22AM +0000, Lorenzo Pieralisi wrote: > On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote: > > [...] > > > > @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > struct of_pci_range_parser parser; > > > char range_type[4]; > > > int err; > > > + struct pci_host_bridge_window *window; > > > > > > if (io_base) > > > *io_base = (resource_size_t)OF_BAD_ADDR; > > > @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > conversion_failed: > > > kfree(res); > > > parse_failed: > > > + list_for_each_entry(window, resources, list) > > > + kfree(window->res); > > > pci_free_resource_list(resources); > > > + kfree(bus_range); > > > return err; > > > } > > > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); > > > > Hi Lorenzo et all, > > > > Here is my personal view and I am happy to hear from others on the desired > > behaviour: > > > > When I wrote this function what I had in mind was that it will parse as > > much as possible from the device tree and return a list of resources that > > could be successfully converted. If the entire list of ranges could not > > be converted then an error code will be returned, but the caller still > > had the list as constructed up to the error. It was the job of the caller > > to free the list in either cases, as stated in the comment. > > That's what I am questioning. If the function takes an error path, the > windows list is freed, so the resource pointers are gone. There is no > way the caller can grab those resource pointers and free them. I stand corrected. Your patch is needed. Thanks, Liviu > > So either way, the function needs patching. Either we do not free the > windows list (we remove pci_free_resource_list) or we apply my fix (or > we refactor the API which is likely to be what I will do). > > Lorenzo > > > > > The historical reason why the function was written that way was because at > > some moment after parsing I've had an additional step where arches could > > cleanup / veto the list and they could return an error value to signal > > their discontent. Also I was (am) not sure how lenient we could be with > > the device tree not being sane (at least one host bridge binding lists the > > config space as a range, which was accepted as broken). > > > > So, from that point of view, I would NAK this patch, as the function works > > as intended. If others find this mode of operation too convoluted, then > > the patch should probably make clear that cleanup only needs to be done on > > function returning success. > > > > Best regards, > > > > > -- > > > 2.2.1 > > > > > > -- ------------------- .oooO ( ) \ ( Oooo. \_) ( ) ) / (_/ One small step for me ... ^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() @ 2015-01-20 11:20 ` Liviu Dudau 0 siblings, 0 replies; 32+ messages in thread From: Liviu Dudau @ 2015-01-20 11:20 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jan 20, 2015 at 10:49:22AM +0000, Lorenzo Pieralisi wrote: > On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote: > > [...] > > > > @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > struct of_pci_range_parser parser; > > > char range_type[4]; > > > int err; > > > + struct pci_host_bridge_window *window; > > > > > > if (io_base) > > > *io_base = (resource_size_t)OF_BAD_ADDR; > > > @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > conversion_failed: > > > kfree(res); > > > parse_failed: > > > + list_for_each_entry(window, resources, list) > > > + kfree(window->res); > > > pci_free_resource_list(resources); > > > + kfree(bus_range); > > > return err; > > > } > > > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); > > > > Hi Lorenzo et all, > > > > Here is my personal view and I am happy to hear from others on the desired > > behaviour: > > > > When I wrote this function what I had in mind was that it will parse as > > much as possible from the device tree and return a list of resources that > > could be successfully converted. If the entire list of ranges could not > > be converted then an error code will be returned, but the caller still > > had the list as constructed up to the error. It was the job of the caller > > to free the list in either cases, as stated in the comment. > > That's what I am questioning. If the function takes an error path, the > windows list is freed, so the resource pointers are gone. There is no > way the caller can grab those resource pointers and free them. I stand corrected. Your patch is needed. Thanks, Liviu > > So either way, the function needs patching. Either we do not free the > windows list (we remove pci_free_resource_list) or we apply my fix (or > we refactor the API which is likely to be what I will do). > > Lorenzo > > > > > The historical reason why the function was written that way was because at > > some moment after parsing I've had an additional step where arches could > > cleanup / veto the list and they could return an error value to signal > > their discontent. Also I was (am) not sure how lenient we could be with > > the device tree not being sane (at least one host bridge binding lists the > > config space as a range, which was accepted as broken). > > > > So, from that point of view, I would NAK this patch, as the function works > > as intended. If others find this mode of operation too convoluted, then > > the patch should probably make clear that cleanup only needs to be done on > > function returning success. > > > > Best regards, > > > > > -- > > > 2.2.1 > > > > > > -- ------------------- .oooO ( ) \ ( Oooo. \_) ( ) ) / (_/ One small step for me ... ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() @ 2015-01-20 11:20 ` Liviu Dudau 0 siblings, 0 replies; 32+ messages in thread From: Liviu Dudau @ 2015-01-20 11:20 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Liviu Dudau, linux-pci, linux-arm-kernel, devicetree, Arnd Bergmann, Mohit Kumar, Jingoo Han, Bjorn Helgaas, Rob Herring On Tue, Jan 20, 2015 at 10:49:22AM +0000, Lorenzo Pieralisi wrote: > On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote: > > [...] > > > > @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > struct of_pci_range_parser parser; > > > char range_type[4]; > > > int err; > > > + struct pci_host_bridge_window *window; > > > > > > if (io_base) > > > *io_base = (resource_size_t)OF_BAD_ADDR; > > > @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > conversion_failed: > > > kfree(res); > > > parse_failed: > > > + list_for_each_entry(window, resources, list) > > > + kfree(window->res); > > > pci_free_resource_list(resources); > > > + kfree(bus_range); > > > return err; > > > } > > > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); > > > > Hi Lorenzo et all, > > > > Here is my personal view and I am happy to hear from others on the desired > > behaviour: > > > > When I wrote this function what I had in mind was that it will parse as > > much as possible from the device tree and return a list of resources that > > could be successfully converted. If the entire list of ranges could not > > be converted then an error code will be returned, but the caller still > > had the list as constructed up to the error. It was the job of the caller > > to free the list in either cases, as stated in the comment. > > That's what I am questioning. If the function takes an error path, the > windows list is freed, so the resource pointers are gone. There is no > way the caller can grab those resource pointers and free them. I stand corrected. Your patch is needed. Thanks, Liviu > > So either way, the function needs patching. Either we do not free the > windows list (we remove pci_free_resource_list) or we apply my fix (or > we refactor the API which is likely to be what I will do). > > Lorenzo > > > > > The historical reason why the function was written that way was because at > > some moment after parsing I've had an additional step where arches could > > cleanup / veto the list and they could return an error value to signal > > their discontent. Also I was (am) not sure how lenient we could be with > > the device tree not being sane (at least one host bridge binding lists the > > config space as a range, which was accepted as broken). > > > > So, from that point of view, I would NAK this patch, as the function works > > as intended. If others find this mode of operation too convoluted, then > > the patch should probably make clear that cleanup only needs to be done on > > function returning success. > > > > Best regards, > > > > > -- > > > 2.2.1 > > > > > > -- ------------------- .oooO ( ) \ ( Oooo. \_) ( ) ) / (_/ One small step for me ... ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <20150120112031.GA342-hOhETlTuV5niMG9XS5x8Mg@public.gmane.org>]
* Re: [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() 2015-01-20 11:20 ` Liviu Dudau (?) @ 2015-01-26 11:21 ` Lorenzo Pieralisi -1 siblings, 0 replies; 32+ messages in thread From: Lorenzo Pieralisi @ 2015-01-26 11:21 UTC (permalink / raw) To: Liviu Dudau Cc: Liviu Dudau, linux-pci-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree-u79uwXL29TY76Z2rM5mHXA, Arnd Bergmann, Mohit Kumar, Jingoo Han, Bjorn Helgaas, Rob Herring On Tue, Jan 20, 2015 at 11:20:32AM +0000, Liviu Dudau wrote: > On Tue, Jan 20, 2015 at 10:49:22AM +0000, Lorenzo Pieralisi wrote: > > On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote: > > > > [...] > > > > > > @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > > struct of_pci_range_parser parser; > > > > char range_type[4]; > > > > int err; > > > > + struct pci_host_bridge_window *window; > > > > > > > > if (io_base) > > > > *io_base = (resource_size_t)OF_BAD_ADDR; > > > > @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > > conversion_failed: > > > > kfree(res); > > > > parse_failed: > > > > + list_for_each_entry(window, resources, list) > > > > + kfree(window->res); > > > > pci_free_resource_list(resources); > > > > + kfree(bus_range); > > > > return err; > > > > } > > > > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); > > > > > > Hi Lorenzo et all, > > > > > > Here is my personal view and I am happy to hear from others on the desired > > > behaviour: > > > > > > When I wrote this function what I had in mind was that it will parse as > > > much as possible from the device tree and return a list of resources that > > > could be successfully converted. If the entire list of ranges could not > > > be converted then an error code will be returned, but the caller still > > > had the list as constructed up to the error. It was the job of the caller > > > to free the list in either cases, as stated in the comment. > > > > That's what I am questioning. If the function takes an error path, the > > windows list is freed, so the resource pointers are gone. There is no > > way the caller can grab those resource pointers and free them. > > I stand corrected. Your patch is needed. Mind acking it ? I will ask Bjorn to take it then. Thanks, Lorenzo -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() @ 2015-01-26 11:21 ` Lorenzo Pieralisi 0 siblings, 0 replies; 32+ messages in thread From: Lorenzo Pieralisi @ 2015-01-26 11:21 UTC (permalink / raw) To: linux-arm-kernel On Tue, Jan 20, 2015 at 11:20:32AM +0000, Liviu Dudau wrote: > On Tue, Jan 20, 2015 at 10:49:22AM +0000, Lorenzo Pieralisi wrote: > > On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote: > > > > [...] > > > > > > @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > > struct of_pci_range_parser parser; > > > > char range_type[4]; > > > > int err; > > > > + struct pci_host_bridge_window *window; > > > > > > > > if (io_base) > > > > *io_base = (resource_size_t)OF_BAD_ADDR; > > > > @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > > conversion_failed: > > > > kfree(res); > > > > parse_failed: > > > > + list_for_each_entry(window, resources, list) > > > > + kfree(window->res); > > > > pci_free_resource_list(resources); > > > > + kfree(bus_range); > > > > return err; > > > > } > > > > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); > > > > > > Hi Lorenzo et all, > > > > > > Here is my personal view and I am happy to hear from others on the desired > > > behaviour: > > > > > > When I wrote this function what I had in mind was that it will parse as > > > much as possible from the device tree and return a list of resources that > > > could be successfully converted. If the entire list of ranges could not > > > be converted then an error code will be returned, but the caller still > > > had the list as constructed up to the error. It was the job of the caller > > > to free the list in either cases, as stated in the comment. > > > > That's what I am questioning. If the function takes an error path, the > > windows list is freed, so the resource pointers are gone. There is no > > way the caller can grab those resource pointers and free them. > > I stand corrected. Your patch is needed. Mind acking it ? I will ask Bjorn to take it then. Thanks, Lorenzo ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() @ 2015-01-26 11:21 ` Lorenzo Pieralisi 0 siblings, 0 replies; 32+ messages in thread From: Lorenzo Pieralisi @ 2015-01-26 11:21 UTC (permalink / raw) To: Liviu Dudau Cc: Liviu Dudau, linux-pci, linux-arm-kernel, devicetree, Arnd Bergmann, Mohit Kumar, Jingoo Han, Bjorn Helgaas, Rob Herring On Tue, Jan 20, 2015 at 11:20:32AM +0000, Liviu Dudau wrote: > On Tue, Jan 20, 2015 at 10:49:22AM +0000, Lorenzo Pieralisi wrote: > > On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote: > > > > [...] > > > > > > @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > > struct of_pci_range_parser parser; > > > > char range_type[4]; > > > > int err; > > > > + struct pci_host_bridge_window *window; > > > > > > > > if (io_base) > > > > *io_base = (resource_size_t)OF_BAD_ADDR; > > > > @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > > conversion_failed: > > > > kfree(res); > > > > parse_failed: > > > > + list_for_each_entry(window, resources, list) > > > > + kfree(window->res); > > > > pci_free_resource_list(resources); > > > > + kfree(bus_range); > > > > return err; > > > > } > > > > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); > > > > > > Hi Lorenzo et all, > > > > > > Here is my personal view and I am happy to hear from others on the desired > > > behaviour: > > > > > > When I wrote this function what I had in mind was that it will parse as > > > much as possible from the device tree and return a list of resources that > > > could be successfully converted. If the entire list of ranges could not > > > be converted then an error code will be returned, but the caller still > > > had the list as constructed up to the error. It was the job of the caller > > > to free the list in either cases, as stated in the comment. > > > > That's what I am questioning. If the function takes an error path, the > > windows list is freed, so the resource pointers are gone. There is no > > way the caller can grab those resource pointers and free them. > > I stand corrected. Your patch is needed. Mind acking it ? I will ask Bjorn to take it then. Thanks, Lorenzo ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() 2015-01-26 11:21 ` Lorenzo Pieralisi (?) @ 2015-01-26 13:06 ` Liviu Dudau -1 siblings, 0 replies; 32+ messages in thread From: Liviu Dudau @ 2015-01-26 13:06 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Liviu Dudau, linux-pci-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree-u79uwXL29TY76Z2rM5mHXA, Arnd Bergmann, Mohit Kumar, Jingoo Han, Bjorn Helgaas, Rob Herring On Mon, Jan 26, 2015 at 11:21:11AM +0000, Lorenzo Pieralisi wrote: > On Tue, Jan 20, 2015 at 11:20:32AM +0000, Liviu Dudau wrote: > > On Tue, Jan 20, 2015 at 10:49:22AM +0000, Lorenzo Pieralisi wrote: > > > On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote: > > > > > > [...] > > > > > > > > @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > > > struct of_pci_range_parser parser; > > > > > char range_type[4]; > > > > > int err; > > > > > + struct pci_host_bridge_window *window; > > > > > > > > > > if (io_base) > > > > > *io_base = (resource_size_t)OF_BAD_ADDR; > > > > > @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > > > conversion_failed: > > > > > kfree(res); > > > > > parse_failed: > > > > > + list_for_each_entry(window, resources, list) > > > > > + kfree(window->res); > > > > > pci_free_resource_list(resources); > > > > > + kfree(bus_range); > > > > > return err; > > > > > } > > > > > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); > > > > > > > > Hi Lorenzo et all, > > > > > > > > Here is my personal view and I am happy to hear from others on the desired > > > > behaviour: > > > > > > > > When I wrote this function what I had in mind was that it will parse as > > > > much as possible from the device tree and return a list of resources that > > > > could be successfully converted. If the entire list of ranges could not > > > > be converted then an error code will be returned, but the caller still > > > > had the list as constructed up to the error. It was the job of the caller > > > > to free the list in either cases, as stated in the comment. > > > > > > That's what I am questioning. If the function takes an error path, the > > > windows list is freed, so the resource pointers are gone. There is no > > > way the caller can grab those resource pointers and free them. > > > > I stand corrected. Your patch is needed. > > Mind acking it ? I will ask Bjorn to take it then. Sure. Acked-by: Liviu Dudau <Liviu.Dudau-5wv7dgnIgG8@public.gmane.org> > > Thanks, > Lorenzo > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯ -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() @ 2015-01-26 13:06 ` Liviu Dudau 0 siblings, 0 replies; 32+ messages in thread From: Liviu Dudau @ 2015-01-26 13:06 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jan 26, 2015 at 11:21:11AM +0000, Lorenzo Pieralisi wrote: > On Tue, Jan 20, 2015 at 11:20:32AM +0000, Liviu Dudau wrote: > > On Tue, Jan 20, 2015 at 10:49:22AM +0000, Lorenzo Pieralisi wrote: > > > On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote: > > > > > > [...] > > > > > > > > @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > > > struct of_pci_range_parser parser; > > > > > char range_type[4]; > > > > > int err; > > > > > + struct pci_host_bridge_window *window; > > > > > > > > > > if (io_base) > > > > > *io_base = (resource_size_t)OF_BAD_ADDR; > > > > > @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > > > conversion_failed: > > > > > kfree(res); > > > > > parse_failed: > > > > > + list_for_each_entry(window, resources, list) > > > > > + kfree(window->res); > > > > > pci_free_resource_list(resources); > > > > > + kfree(bus_range); > > > > > return err; > > > > > } > > > > > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); > > > > > > > > Hi Lorenzo et all, > > > > > > > > Here is my personal view and I am happy to hear from others on the desired > > > > behaviour: > > > > > > > > When I wrote this function what I had in mind was that it will parse as > > > > much as possible from the device tree and return a list of resources that > > > > could be successfully converted. If the entire list of ranges could not > > > > be converted then an error code will be returned, but the caller still > > > > had the list as constructed up to the error. It was the job of the caller > > > > to free the list in either cases, as stated in the comment. > > > > > > That's what I am questioning. If the function takes an error path, the > > > windows list is freed, so the resource pointers are gone. There is no > > > way the caller can grab those resource pointers and free them. > > > > I stand corrected. Your patch is needed. > > Mind acking it ? I will ask Bjorn to take it then. Sure. Acked-by: Liviu Dudau <Liviu.Dudau@arm.com> > > Thanks, > Lorenzo > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ?\_(?)_/? ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() @ 2015-01-26 13:06 ` Liviu Dudau 0 siblings, 0 replies; 32+ messages in thread From: Liviu Dudau @ 2015-01-26 13:06 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Liviu Dudau, linux-pci, linux-arm-kernel, devicetree, Arnd Bergmann, Mohit Kumar, Jingoo Han, Bjorn Helgaas, Rob Herring On Mon, Jan 26, 2015 at 11:21:11AM +0000, Lorenzo Pieralisi wrote: > On Tue, Jan 20, 2015 at 11:20:32AM +0000, Liviu Dudau wrote: > > On Tue, Jan 20, 2015 at 10:49:22AM +0000, Lorenzo Pieralisi wrote: > > > On Mon, Jan 19, 2015 at 06:32:29PM +0000, Liviu Dudau wrote: > > > > > > [...] > > > > > > > > @@ -146,6 +146,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > > > struct of_pci_range_parser parser; > > > > > char range_type[4]; > > > > > int err; > > > > > + struct pci_host_bridge_window *window; > > > > > > > > > > if (io_base) > > > > > *io_base = (resource_size_t)OF_BAD_ADDR; > > > > > @@ -225,7 +226,10 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, > > > > > conversion_failed: > > > > > kfree(res); > > > > > parse_failed: > > > > > + list_for_each_entry(window, resources, list) > > > > > + kfree(window->res); > > > > > pci_free_resource_list(resources); > > > > > + kfree(bus_range); > > > > > return err; > > > > > } > > > > > EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); > > > > > > > > Hi Lorenzo et all, > > > > > > > > Here is my personal view and I am happy to hear from others on the desired > > > > behaviour: > > > > > > > > When I wrote this function what I had in mind was that it will parse as > > > > much as possible from the device tree and return a list of resources that > > > > could be successfully converted. If the entire list of ranges could not > > > > be converted then an error code will be returned, but the caller still > > > > had the list as constructed up to the error. It was the job of the caller > > > > to free the list in either cases, as stated in the comment. > > > > > > That's what I am questioning. If the function takes an error path, the > > > windows list is freed, so the resource pointers are gone. There is no > > > way the caller can grab those resource pointers and free them. > > > > I stand corrected. Your patch is needed. > > Mind acking it ? I will ask Bjorn to take it then. Sure. Acked-by: Liviu Dudau <Liviu.Dudau@arm.com> > > Thanks, > Lorenzo > -- ==================== | I would like to | | fix the world, | | but they're not | | giving me the | \ source code! / --------------- ¯\_(ツ)_/¯ ^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC PATCH 2/3] drivers: of: of_pci_get_host_bridge_resources() range parsing update 2015-01-07 15:29 ` Lorenzo Pieralisi @ 2015-01-07 15:29 ` Lorenzo Pieralisi -1 siblings, 0 replies; 32+ messages in thread From: Lorenzo Pieralisi @ 2015-01-07 15:29 UTC (permalink / raw) To: linux-pci, linux-arm-kernel, devicetree Cc: Lorenzo Pieralisi, Arnd Bergmann, Liviu Dudau, Mohit Kumar, Jingoo Han, Bjorn Helgaas, Rob Herring Some host controllers require local bus physical addresses to programme inbound/outbound requests from the bus hierarchy to be routed properly through the PCI bus beyond the host controller. Owing to bus address size conversion, the bus local addresses may be different from the addresses as seen from the CPU (which are translated by DT core code), so the PCI range parsing function: of_pci_get_host_bridge_resources() should be augmented in order to store the range parser along with the parsed resource so that the CPU untranslated address can be retrieved by the driver from the corresponding PCI range if needed. Cc: Arnd Bergmann <arnd@arndb.de> Cc: Liviu Dudau <liviu.dudau@arm.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Rob Herring <robh+dt@kernel.org> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> --- drivers/of/of_pci.c | 38 +++++++++++++++++++++++++++++--------- include/linux/of_address.h | 5 +++++ 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c index 6fbfe99..f0e576f 100644 --- a/drivers/of/of_pci.c +++ b/drivers/of/of_pci.c @@ -144,6 +144,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, struct resource *bus_range; struct of_pci_range range; struct of_pci_range_parser parser; + struct of_pci_resource *of_pci_res; char range_type[4]; int err; struct pci_host_bridge_window *window; @@ -151,12 +152,14 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, if (io_base) *io_base = (resource_size_t)OF_BAD_ADDR; - bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL); - if (!bus_range) + of_pci_res = kzalloc(sizeof(*of_pci_res), GFP_KERNEL); + if (!of_pci_res) return -ENOMEM; pr_info("PCI host bridge %s ranges:\n", dev->full_name); + bus_range = &of_pci_res->res; + err = of_pci_parse_bus_range(dev, bus_range); if (err) { bus_range->start = busno; @@ -195,17 +198,29 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, if (range.cpu_addr == OF_BAD_ADDR || range.size == 0) continue; - res = kzalloc(sizeof(struct resource), GFP_KERNEL); - if (!res) { + of_pci_res = kzalloc(sizeof(*of_pci_res), GFP_KERNEL); + if (!of_pci_res) { err = -ENOMEM; goto parse_failed; } + res = &of_pci_res->res; err = of_pci_range_to_resource(&range, dev, res); if (err) goto conversion_failed; - if (resource_type(res) == IORESOURCE_IO) { + /* Stash the range parser */ + of_pci_res->parser = parser; + /* + * for_each_of_pci_range increments the range pointer + * in the parser, so that it is ready to parse the + * following range while looping; rewind the range pointer + * to its current value to pass it to the drivers with its + * initial value. + */ + of_pci_res->parser.range -= parser.np; + + if (resource_type(res) == IORESOURCE_IO) { if (!io_base) { pr_err("I/O range found for %s. Please provide an io_base pointer to save CPU base address\n", dev->full_name); @@ -224,12 +239,17 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, return 0; conversion_failed: - kfree(res); + kfree(of_pci_res); parse_failed: - list_for_each_entry(window, resources, list) - kfree(window->res); + list_for_each_entry(window, resources, list) { + of_pci_res = container_of(window->res, struct of_pci_resource, + res); + kfree(of_pci_res); + } pci_free_resource_list(resources); - kfree(bus_range); + of_pci_res = container_of(bus_range, struct of_pci_resource, + res); + kfree(of_pci_res); return err; } EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); diff --git a/include/linux/of_address.h b/include/linux/of_address.h index d88e81b..e4005be 100644 --- a/include/linux/of_address.h +++ b/include/linux/of_address.h @@ -20,6 +20,11 @@ struct of_pci_range { u32 flags; }; +struct of_pci_resource { + struct resource res; + struct of_pci_range_parser parser; +}; + #define for_each_of_pci_range(parser, range) \ for (; of_pci_range_parser_one(parser, range);) -- 2.2.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [RFC PATCH 2/3] drivers: of: of_pci_get_host_bridge_resources() range parsing update @ 2015-01-07 15:29 ` Lorenzo Pieralisi 0 siblings, 0 replies; 32+ messages in thread From: Lorenzo Pieralisi @ 2015-01-07 15:29 UTC (permalink / raw) To: linux-arm-kernel Some host controllers require local bus physical addresses to programme inbound/outbound requests from the bus hierarchy to be routed properly through the PCI bus beyond the host controller. Owing to bus address size conversion, the bus local addresses may be different from the addresses as seen from the CPU (which are translated by DT core code), so the PCI range parsing function: of_pci_get_host_bridge_resources() should be augmented in order to store the range parser along with the parsed resource so that the CPU untranslated address can be retrieved by the driver from the corresponding PCI range if needed. Cc: Arnd Bergmann <arnd@arndb.de> Cc: Liviu Dudau <liviu.dudau@arm.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Rob Herring <robh+dt@kernel.org> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> --- drivers/of/of_pci.c | 38 +++++++++++++++++++++++++++++--------- include/linux/of_address.h | 5 +++++ 2 files changed, 34 insertions(+), 9 deletions(-) diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c index 6fbfe99..f0e576f 100644 --- a/drivers/of/of_pci.c +++ b/drivers/of/of_pci.c @@ -144,6 +144,7 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, struct resource *bus_range; struct of_pci_range range; struct of_pci_range_parser parser; + struct of_pci_resource *of_pci_res; char range_type[4]; int err; struct pci_host_bridge_window *window; @@ -151,12 +152,14 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, if (io_base) *io_base = (resource_size_t)OF_BAD_ADDR; - bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL); - if (!bus_range) + of_pci_res = kzalloc(sizeof(*of_pci_res), GFP_KERNEL); + if (!of_pci_res) return -ENOMEM; pr_info("PCI host bridge %s ranges:\n", dev->full_name); + bus_range = &of_pci_res->res; + err = of_pci_parse_bus_range(dev, bus_range); if (err) { bus_range->start = busno; @@ -195,17 +198,29 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, if (range.cpu_addr == OF_BAD_ADDR || range.size == 0) continue; - res = kzalloc(sizeof(struct resource), GFP_KERNEL); - if (!res) { + of_pci_res = kzalloc(sizeof(*of_pci_res), GFP_KERNEL); + if (!of_pci_res) { err = -ENOMEM; goto parse_failed; } + res = &of_pci_res->res; err = of_pci_range_to_resource(&range, dev, res); if (err) goto conversion_failed; - if (resource_type(res) == IORESOURCE_IO) { + /* Stash the range parser */ + of_pci_res->parser = parser; + /* + * for_each_of_pci_range increments the range pointer + * in the parser, so that it is ready to parse the + * following range while looping; rewind the range pointer + * to its current value to pass it to the drivers with its + * initial value. + */ + of_pci_res->parser.range -= parser.np; + + if (resource_type(res) == IORESOURCE_IO) { if (!io_base) { pr_err("I/O range found for %s. Please provide an io_base pointer to save CPU base address\n", dev->full_name); @@ -224,12 +239,17 @@ int of_pci_get_host_bridge_resources(struct device_node *dev, return 0; conversion_failed: - kfree(res); + kfree(of_pci_res); parse_failed: - list_for_each_entry(window, resources, list) - kfree(window->res); + list_for_each_entry(window, resources, list) { + of_pci_res = container_of(window->res, struct of_pci_resource, + res); + kfree(of_pci_res); + } pci_free_resource_list(resources); - kfree(bus_range); + of_pci_res = container_of(bus_range, struct of_pci_resource, + res); + kfree(of_pci_res); return err; } EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); diff --git a/include/linux/of_address.h b/include/linux/of_address.h index d88e81b..e4005be 100644 --- a/include/linux/of_address.h +++ b/include/linux/of_address.h @@ -20,6 +20,11 @@ struct of_pci_range { u32 flags; }; +struct of_pci_resource { + struct resource res; + struct of_pci_range_parser parser; +}; + #define for_each_of_pci_range(parser, range) \ for (; of_pci_range_parser_one(parser, range);) -- 2.2.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
[parent not found: <1420644571-18928-1-git-send-email-lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>]
* [RFC PATCH 3/3] drivers: pci: host: update the pcie designware driver to new range parsing API 2015-01-07 15:29 ` Lorenzo Pieralisi (?) @ 2015-01-07 15:29 ` Lorenzo Pieralisi -1 siblings, 0 replies; 32+ messages in thread From: Lorenzo Pieralisi @ 2015-01-07 15:29 UTC (permalink / raw) To: linux-pci-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree-u79uwXL29TY76Z2rM5mHXA Cc: Lorenzo Pieralisi, Arnd Bergmann, Liviu Dudau, Mohit Kumar, Jingoo Han, Bjorn Helgaas, Rob Herring The new OF pci API of_pci_get_host_bridge_resources() implemented by: 'commit cbe4097f8ae699ebbdaf8c95 ("of/pci: Add support for parsing PCI host bridge resources from DT")' and subsequent changes allow to parse PCI DT ranges in a generic way through a new API that can be used to consolidate the DT range parsing for all PCIe controllers in the kernel that are configured through DT. This patch updates the PCIe designware driver in order for it to use the new OF range parsing API and remove driver specific parsing code. Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> Cc: Liviu Dudau <liviu.dudau-5wv7dgnIgG8@public.gmane.org> Cc: Mohit Kumar <mohit.kumar-qxv4g6HH51o@public.gmane.org> Cc: Jingoo Han <jg1.han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> Cc: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org> --- drivers/pci/host/pcie-designware.c | 145 +++++++++++++++++-------------------- drivers/pci/host/pcie-designware.h | 5 +- 2 files changed, 69 insertions(+), 81 deletions(-) diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c index df781cd..95cce40 100644 --- a/drivers/pci/host/pcie-designware.c +++ b/drivers/pci/host/pcie-designware.c @@ -69,8 +69,6 @@ static struct hw_pci dw_pci; -static unsigned long global_io_offset; - static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys) { BUG_ON(!sys->private_data); @@ -343,18 +341,21 @@ int __init dw_pcie_host_init(struct pcie_port *pp) { struct device_node *np = pp->dev->of_node; struct platform_device *pdev = to_platform_device(pp->dev); - struct of_pci_range range; - struct of_pci_range_parser parser; struct resource *cfg_res; - u32 val, na, ns; + u32 val, na; const __be32 *addrp; int i, index, ret; + resource_size_t io_base; + struct pci_host_bridge_window *win; + struct of_pci_resource *of_pci_res; - /* Find the address cell size and the number of cells in order to get - * the untranslated address. + /* + * Find the address cell size (ie must be 3 for DT PCI addresses) to + * get the untranslated address. */ of_property_read_u32(np, "#address-cells", &na); - ns = of_n_size_cells(np); + + INIT_LIST_HEAD(&pp->resources); cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config"); if (cfg_res) { @@ -366,84 +367,79 @@ int __init dw_pcie_host_init(struct pcie_port *pp) /* Find the untranslated configuration space address */ index = of_property_match_string(np, "reg-names", "config"); addrp = of_get_address(np, index, NULL, NULL); - pp->cfg0_mod_base = of_read_number(addrp, ns); + pp->cfg0_mod_base = of_read_number(addrp, of_n_addr_cells(np)); pp->cfg1_mod_base = pp->cfg0_mod_base + pp->cfg0_size; } else { dev_err(pp->dev, "missing *config* reg space\n"); } - if (of_pci_range_parser_init(&parser, np)) { - dev_err(pp->dev, "missing ranges property\n"); - return -EINVAL; - } + ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &pp->resources, + &io_base); + if (ret) + return ret; - /* Get the I/O and memory ranges from DT */ - for_each_of_pci_range(&parser, &range) { - unsigned long restype = range.flags & IORESOURCE_TYPE_BITS; - - if (restype == IORESOURCE_IO) { - of_pci_range_to_resource(&range, np, &pp->io); - pp->io.name = "I/O"; - pp->io.start = max_t(resource_size_t, - PCIBIOS_MIN_IO, - range.pci_addr + global_io_offset); - pp->io.end = min_t(resource_size_t, + list_for_each_entry(win, &pp->resources, list) { + u64 pci_addr; + struct resource *res = win->res; + + pci_addr = res->start - win->offset; + of_pci_res = container_of(res, struct of_pci_resource, res); + + if (resource_type(res) == IORESOURCE_IO) { + res->name = "I/O"; + res->start = max_t(resource_size_t, PCIBIOS_MIN_IO, + pci_addr); + res->end = min_t(resource_size_t, IO_SPACE_LIMIT, - range.pci_addr + range.size - + global_io_offset - 1); - pp->io_size = resource_size(&pp->io); - pp->io_bus_addr = range.pci_addr; - pp->io_base = range.cpu_addr; + pci_addr + resource_size(res) - 1); + pp->io_size = resource_size(res); + pp->io_bus_addr = pci_addr; + pp->io_base = io_base; /* Find the untranslated IO space address */ - pp->io_mod_base = of_read_number(parser.range - - parser.np + na, ns); + pp->io_mod_base = + of_read_number(of_pci_res->parser.range + na, + of_pci_res->parser.pna); + pci_remap_iospace(res, io_base); } - if (restype == IORESOURCE_MEM) { - of_pci_range_to_resource(&range, np, &pp->mem); - pp->mem.name = "MEM"; - pp->mem_size = resource_size(&pp->mem); - pp->mem_bus_addr = range.pci_addr; + + if (resource_type(res) == IORESOURCE_MEM) { + res->name = "MEM"; + pp->mem_size = resource_size(res); + pp->mem_bus_addr = pci_addr; + pp->mem_base = res->start; /* Find the untranslated MEM space address */ - pp->mem_mod_base = of_read_number(parser.range - - parser.np + na, ns); + pp->mem_mod_base = + of_read_number(of_pci_res->parser.range + na, + of_pci_res->parser.pna); } - if (restype == 0) { - of_pci_range_to_resource(&range, np, &pp->cfg); - pp->cfg0_size = resource_size(&pp->cfg)/2; - pp->cfg1_size = resource_size(&pp->cfg)/2; - pp->cfg0_base = pp->cfg.start; - pp->cfg1_base = pp->cfg.start + pp->cfg0_size; + + if (resource_type(res) == 0) { + pp->cfg0_size = resource_size(res)/2; + pp->cfg1_size = resource_size(res)/2; + pp->cfg0_base = res->start; + pp->cfg1_base = res->start + pp->cfg0_size; /* Find the untranslated configuration space address */ - pp->cfg0_mod_base = of_read_number(parser.range - - parser.np + na, ns); + pp->cfg0_mod_base = + of_read_number(of_pci_res->parser.range + na, + of_pci_res->parser.pna); pp->cfg1_mod_base = pp->cfg0_mod_base + pp->cfg0_size; - } - } - - ret = of_pci_parse_bus_range(np, &pp->busn); - if (ret < 0) { - pp->busn.name = np->name; - pp->busn.start = 0; - pp->busn.end = 0xff; - pp->busn.flags = IORESOURCE_BUS; - dev_dbg(pp->dev, "failed to parse bus-range property: %d, using default %pR\n", - ret, &pp->busn); - } - 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; + if (!pp->dbi_base) { + pp->dbi_base = devm_ioremap(pp->dev, + res->start, + resource_size(res)); + if (!pp->dbi_base) { + dev_err(pp->dev, "error with ioremap\n"); + return -ENOMEM; + } + } } - } - pp->mem_base = pp->mem.start; + } if (!pp->va_cfg0_base) { pp->va_cfg0_base = devm_ioremap(pp->dev, pp->cfg0_base, @@ -704,20 +700,15 @@ static struct pci_ops dw_pcie_ops = { static int dw_pcie_setup(int nr, struct pci_sys_data *sys) { struct pcie_port *pp; + struct pci_host_bridge_window *win, *tmp; pp = sys_to_pcie(sys); + /* remove the configuration aperture in case it is there */ + list_for_each_entry_safe(win, tmp, &pp->resources, list) + if (!resource_type(win->res)) + list_del(&win->list); - if (global_io_offset < SZ_1M && pp->io_size > 0) { - sys->io_offset = global_io_offset - pp->io_bus_addr; - pci_ioremap_io(global_io_offset, pp->io_base); - global_io_offset += SZ_64K; - pci_add_resource_offset(&sys->resources, &pp->io, - sys->io_offset); - } - - sys->mem_offset = pp->mem.start - pp->mem_bus_addr; - pci_add_resource_offset(&sys->resources, &pp->mem, sys->mem_offset); - pci_add_resource(&sys->resources, &pp->busn); + list_splice(&pp->resources, &sys->resources); return 1; } diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h index d0bbd27..2479e9c 100644 --- a/drivers/pci/host/pcie-designware.h +++ b/drivers/pci/host/pcie-designware.h @@ -42,10 +42,7 @@ struct pcie_port { u64 mem_mod_base; phys_addr_t mem_bus_addr; u32 mem_size; - struct resource cfg; - struct resource io; - struct resource mem; - struct resource busn; + struct list_head resources; int irq; u32 lanes; struct pcie_host_ops *ops; -- 2.2.1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [RFC PATCH 3/3] drivers: pci: host: update the pcie designware driver to new range parsing API @ 2015-01-07 15:29 ` Lorenzo Pieralisi 0 siblings, 0 replies; 32+ messages in thread From: Lorenzo Pieralisi @ 2015-01-07 15:29 UTC (permalink / raw) To: linux-arm-kernel The new OF pci API of_pci_get_host_bridge_resources() implemented by: 'commit cbe4097f8ae699ebbdaf8c95 ("of/pci: Add support for parsing PCI host bridge resources from DT")' and subsequent changes allow to parse PCI DT ranges in a generic way through a new API that can be used to consolidate the DT range parsing for all PCIe controllers in the kernel that are configured through DT. This patch updates the PCIe designware driver in order for it to use the new OF range parsing API and remove driver specific parsing code. Cc: Arnd Bergmann <arnd@arndb.de> Cc: Liviu Dudau <liviu.dudau@arm.com> Cc: Mohit Kumar <mohit.kumar@st.com> Cc: Jingoo Han <jg1.han@samsung.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Rob Herring <robh+dt@kernel.org> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> --- drivers/pci/host/pcie-designware.c | 145 +++++++++++++++++-------------------- drivers/pci/host/pcie-designware.h | 5 +- 2 files changed, 69 insertions(+), 81 deletions(-) diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c index df781cd..95cce40 100644 --- a/drivers/pci/host/pcie-designware.c +++ b/drivers/pci/host/pcie-designware.c @@ -69,8 +69,6 @@ static struct hw_pci dw_pci; -static unsigned long global_io_offset; - static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys) { BUG_ON(!sys->private_data); @@ -343,18 +341,21 @@ int __init dw_pcie_host_init(struct pcie_port *pp) { struct device_node *np = pp->dev->of_node; struct platform_device *pdev = to_platform_device(pp->dev); - struct of_pci_range range; - struct of_pci_range_parser parser; struct resource *cfg_res; - u32 val, na, ns; + u32 val, na; const __be32 *addrp; int i, index, ret; + resource_size_t io_base; + struct pci_host_bridge_window *win; + struct of_pci_resource *of_pci_res; - /* Find the address cell size and the number of cells in order to get - * the untranslated address. + /* + * Find the address cell size (ie must be 3 for DT PCI addresses) to + * get the untranslated address. */ of_property_read_u32(np, "#address-cells", &na); - ns = of_n_size_cells(np); + + INIT_LIST_HEAD(&pp->resources); cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config"); if (cfg_res) { @@ -366,84 +367,79 @@ int __init dw_pcie_host_init(struct pcie_port *pp) /* Find the untranslated configuration space address */ index = of_property_match_string(np, "reg-names", "config"); addrp = of_get_address(np, index, NULL, NULL); - pp->cfg0_mod_base = of_read_number(addrp, ns); + pp->cfg0_mod_base = of_read_number(addrp, of_n_addr_cells(np)); pp->cfg1_mod_base = pp->cfg0_mod_base + pp->cfg0_size; } else { dev_err(pp->dev, "missing *config* reg space\n"); } - if (of_pci_range_parser_init(&parser, np)) { - dev_err(pp->dev, "missing ranges property\n"); - return -EINVAL; - } + ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &pp->resources, + &io_base); + if (ret) + return ret; - /* Get the I/O and memory ranges from DT */ - for_each_of_pci_range(&parser, &range) { - unsigned long restype = range.flags & IORESOURCE_TYPE_BITS; - - if (restype == IORESOURCE_IO) { - of_pci_range_to_resource(&range, np, &pp->io); - pp->io.name = "I/O"; - pp->io.start = max_t(resource_size_t, - PCIBIOS_MIN_IO, - range.pci_addr + global_io_offset); - pp->io.end = min_t(resource_size_t, + list_for_each_entry(win, &pp->resources, list) { + u64 pci_addr; + struct resource *res = win->res; + + pci_addr = res->start - win->offset; + of_pci_res = container_of(res, struct of_pci_resource, res); + + if (resource_type(res) == IORESOURCE_IO) { + res->name = "I/O"; + res->start = max_t(resource_size_t, PCIBIOS_MIN_IO, + pci_addr); + res->end = min_t(resource_size_t, IO_SPACE_LIMIT, - range.pci_addr + range.size - + global_io_offset - 1); - pp->io_size = resource_size(&pp->io); - pp->io_bus_addr = range.pci_addr; - pp->io_base = range.cpu_addr; + pci_addr + resource_size(res) - 1); + pp->io_size = resource_size(res); + pp->io_bus_addr = pci_addr; + pp->io_base = io_base; /* Find the untranslated IO space address */ - pp->io_mod_base = of_read_number(parser.range - - parser.np + na, ns); + pp->io_mod_base = + of_read_number(of_pci_res->parser.range + na, + of_pci_res->parser.pna); + pci_remap_iospace(res, io_base); } - if (restype == IORESOURCE_MEM) { - of_pci_range_to_resource(&range, np, &pp->mem); - pp->mem.name = "MEM"; - pp->mem_size = resource_size(&pp->mem); - pp->mem_bus_addr = range.pci_addr; + + if (resource_type(res) == IORESOURCE_MEM) { + res->name = "MEM"; + pp->mem_size = resource_size(res); + pp->mem_bus_addr = pci_addr; + pp->mem_base = res->start; /* Find the untranslated MEM space address */ - pp->mem_mod_base = of_read_number(parser.range - - parser.np + na, ns); + pp->mem_mod_base = + of_read_number(of_pci_res->parser.range + na, + of_pci_res->parser.pna); } - if (restype == 0) { - of_pci_range_to_resource(&range, np, &pp->cfg); - pp->cfg0_size = resource_size(&pp->cfg)/2; - pp->cfg1_size = resource_size(&pp->cfg)/2; - pp->cfg0_base = pp->cfg.start; - pp->cfg1_base = pp->cfg.start + pp->cfg0_size; + + if (resource_type(res) == 0) { + pp->cfg0_size = resource_size(res)/2; + pp->cfg1_size = resource_size(res)/2; + pp->cfg0_base = res->start; + pp->cfg1_base = res->start + pp->cfg0_size; /* Find the untranslated configuration space address */ - pp->cfg0_mod_base = of_read_number(parser.range - - parser.np + na, ns); + pp->cfg0_mod_base = + of_read_number(of_pci_res->parser.range + na, + of_pci_res->parser.pna); pp->cfg1_mod_base = pp->cfg0_mod_base + pp->cfg0_size; - } - } - - ret = of_pci_parse_bus_range(np, &pp->busn); - if (ret < 0) { - pp->busn.name = np->name; - pp->busn.start = 0; - pp->busn.end = 0xff; - pp->busn.flags = IORESOURCE_BUS; - dev_dbg(pp->dev, "failed to parse bus-range property: %d, using default %pR\n", - ret, &pp->busn); - } - 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; + if (!pp->dbi_base) { + pp->dbi_base = devm_ioremap(pp->dev, + res->start, + resource_size(res)); + if (!pp->dbi_base) { + dev_err(pp->dev, "error with ioremap\n"); + return -ENOMEM; + } + } } - } - pp->mem_base = pp->mem.start; + } if (!pp->va_cfg0_base) { pp->va_cfg0_base = devm_ioremap(pp->dev, pp->cfg0_base, @@ -704,20 +700,15 @@ static struct pci_ops dw_pcie_ops = { static int dw_pcie_setup(int nr, struct pci_sys_data *sys) { struct pcie_port *pp; + struct pci_host_bridge_window *win, *tmp; pp = sys_to_pcie(sys); + /* remove the configuration aperture in case it is there */ + list_for_each_entry_safe(win, tmp, &pp->resources, list) + if (!resource_type(win->res)) + list_del(&win->list); - if (global_io_offset < SZ_1M && pp->io_size > 0) { - sys->io_offset = global_io_offset - pp->io_bus_addr; - pci_ioremap_io(global_io_offset, pp->io_base); - global_io_offset += SZ_64K; - pci_add_resource_offset(&sys->resources, &pp->io, - sys->io_offset); - } - - sys->mem_offset = pp->mem.start - pp->mem_bus_addr; - pci_add_resource_offset(&sys->resources, &pp->mem, sys->mem_offset); - pci_add_resource(&sys->resources, &pp->busn); + list_splice(&pp->resources, &sys->resources); return 1; } diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h index d0bbd27..2479e9c 100644 --- a/drivers/pci/host/pcie-designware.h +++ b/drivers/pci/host/pcie-designware.h @@ -42,10 +42,7 @@ struct pcie_port { u64 mem_mod_base; phys_addr_t mem_bus_addr; u32 mem_size; - struct resource cfg; - struct resource io; - struct resource mem; - struct resource busn; + struct list_head resources; int irq; u32 lanes; struct pcie_host_ops *ops; -- 2.2.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* [RFC PATCH 3/3] drivers: pci: host: update the pcie designware driver to new range parsing API @ 2015-01-07 15:29 ` Lorenzo Pieralisi 0 siblings, 0 replies; 32+ messages in thread From: Lorenzo Pieralisi @ 2015-01-07 15:29 UTC (permalink / raw) To: linux-pci, linux-arm-kernel, devicetree Cc: Lorenzo Pieralisi, Arnd Bergmann, Liviu Dudau, Mohit Kumar, Jingoo Han, Bjorn Helgaas, Rob Herring The new OF pci API of_pci_get_host_bridge_resources() implemented by: 'commit cbe4097f8ae699ebbdaf8c95 ("of/pci: Add support for parsing PCI host bridge resources from DT")' and subsequent changes allow to parse PCI DT ranges in a generic way through a new API that can be used to consolidate the DT range parsing for all PCIe controllers in the kernel that are configured through DT. This patch updates the PCIe designware driver in order for it to use the new OF range parsing API and remove driver specific parsing code. Cc: Arnd Bergmann <arnd@arndb.de> Cc: Liviu Dudau <liviu.dudau@arm.com> Cc: Mohit Kumar <mohit.kumar@st.com> Cc: Jingoo Han <jg1.han@samsung.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Rob Herring <robh+dt@kernel.org> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> --- drivers/pci/host/pcie-designware.c | 145 +++++++++++++++++-------------------- drivers/pci/host/pcie-designware.h | 5 +- 2 files changed, 69 insertions(+), 81 deletions(-) diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c index df781cd..95cce40 100644 --- a/drivers/pci/host/pcie-designware.c +++ b/drivers/pci/host/pcie-designware.c @@ -69,8 +69,6 @@ static struct hw_pci dw_pci; -static unsigned long global_io_offset; - static inline struct pcie_port *sys_to_pcie(struct pci_sys_data *sys) { BUG_ON(!sys->private_data); @@ -343,18 +341,21 @@ int __init dw_pcie_host_init(struct pcie_port *pp) { struct device_node *np = pp->dev->of_node; struct platform_device *pdev = to_platform_device(pp->dev); - struct of_pci_range range; - struct of_pci_range_parser parser; struct resource *cfg_res; - u32 val, na, ns; + u32 val, na; const __be32 *addrp; int i, index, ret; + resource_size_t io_base; + struct pci_host_bridge_window *win; + struct of_pci_resource *of_pci_res; - /* Find the address cell size and the number of cells in order to get - * the untranslated address. + /* + * Find the address cell size (ie must be 3 for DT PCI addresses) to + * get the untranslated address. */ of_property_read_u32(np, "#address-cells", &na); - ns = of_n_size_cells(np); + + INIT_LIST_HEAD(&pp->resources); cfg_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "config"); if (cfg_res) { @@ -366,84 +367,79 @@ int __init dw_pcie_host_init(struct pcie_port *pp) /* Find the untranslated configuration space address */ index = of_property_match_string(np, "reg-names", "config"); addrp = of_get_address(np, index, NULL, NULL); - pp->cfg0_mod_base = of_read_number(addrp, ns); + pp->cfg0_mod_base = of_read_number(addrp, of_n_addr_cells(np)); pp->cfg1_mod_base = pp->cfg0_mod_base + pp->cfg0_size; } else { dev_err(pp->dev, "missing *config* reg space\n"); } - if (of_pci_range_parser_init(&parser, np)) { - dev_err(pp->dev, "missing ranges property\n"); - return -EINVAL; - } + ret = of_pci_get_host_bridge_resources(np, 0, 0xff, &pp->resources, + &io_base); + if (ret) + return ret; - /* Get the I/O and memory ranges from DT */ - for_each_of_pci_range(&parser, &range) { - unsigned long restype = range.flags & IORESOURCE_TYPE_BITS; - - if (restype == IORESOURCE_IO) { - of_pci_range_to_resource(&range, np, &pp->io); - pp->io.name = "I/O"; - pp->io.start = max_t(resource_size_t, - PCIBIOS_MIN_IO, - range.pci_addr + global_io_offset); - pp->io.end = min_t(resource_size_t, + list_for_each_entry(win, &pp->resources, list) { + u64 pci_addr; + struct resource *res = win->res; + + pci_addr = res->start - win->offset; + of_pci_res = container_of(res, struct of_pci_resource, res); + + if (resource_type(res) == IORESOURCE_IO) { + res->name = "I/O"; + res->start = max_t(resource_size_t, PCIBIOS_MIN_IO, + pci_addr); + res->end = min_t(resource_size_t, IO_SPACE_LIMIT, - range.pci_addr + range.size - + global_io_offset - 1); - pp->io_size = resource_size(&pp->io); - pp->io_bus_addr = range.pci_addr; - pp->io_base = range.cpu_addr; + pci_addr + resource_size(res) - 1); + pp->io_size = resource_size(res); + pp->io_bus_addr = pci_addr; + pp->io_base = io_base; /* Find the untranslated IO space address */ - pp->io_mod_base = of_read_number(parser.range - - parser.np + na, ns); + pp->io_mod_base = + of_read_number(of_pci_res->parser.range + na, + of_pci_res->parser.pna); + pci_remap_iospace(res, io_base); } - if (restype == IORESOURCE_MEM) { - of_pci_range_to_resource(&range, np, &pp->mem); - pp->mem.name = "MEM"; - pp->mem_size = resource_size(&pp->mem); - pp->mem_bus_addr = range.pci_addr; + + if (resource_type(res) == IORESOURCE_MEM) { + res->name = "MEM"; + pp->mem_size = resource_size(res); + pp->mem_bus_addr = pci_addr; + pp->mem_base = res->start; /* Find the untranslated MEM space address */ - pp->mem_mod_base = of_read_number(parser.range - - parser.np + na, ns); + pp->mem_mod_base = + of_read_number(of_pci_res->parser.range + na, + of_pci_res->parser.pna); } - if (restype == 0) { - of_pci_range_to_resource(&range, np, &pp->cfg); - pp->cfg0_size = resource_size(&pp->cfg)/2; - pp->cfg1_size = resource_size(&pp->cfg)/2; - pp->cfg0_base = pp->cfg.start; - pp->cfg1_base = pp->cfg.start + pp->cfg0_size; + + if (resource_type(res) == 0) { + pp->cfg0_size = resource_size(res)/2; + pp->cfg1_size = resource_size(res)/2; + pp->cfg0_base = res->start; + pp->cfg1_base = res->start + pp->cfg0_size; /* Find the untranslated configuration space address */ - pp->cfg0_mod_base = of_read_number(parser.range - - parser.np + na, ns); + pp->cfg0_mod_base = + of_read_number(of_pci_res->parser.range + na, + of_pci_res->parser.pna); pp->cfg1_mod_base = pp->cfg0_mod_base + pp->cfg0_size; - } - } - - ret = of_pci_parse_bus_range(np, &pp->busn); - if (ret < 0) { - pp->busn.name = np->name; - pp->busn.start = 0; - pp->busn.end = 0xff; - pp->busn.flags = IORESOURCE_BUS; - dev_dbg(pp->dev, "failed to parse bus-range property: %d, using default %pR\n", - ret, &pp->busn); - } - 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; + if (!pp->dbi_base) { + pp->dbi_base = devm_ioremap(pp->dev, + res->start, + resource_size(res)); + if (!pp->dbi_base) { + dev_err(pp->dev, "error with ioremap\n"); + return -ENOMEM; + } + } } - } - pp->mem_base = pp->mem.start; + } if (!pp->va_cfg0_base) { pp->va_cfg0_base = devm_ioremap(pp->dev, pp->cfg0_base, @@ -704,20 +700,15 @@ static struct pci_ops dw_pcie_ops = { static int dw_pcie_setup(int nr, struct pci_sys_data *sys) { struct pcie_port *pp; + struct pci_host_bridge_window *win, *tmp; pp = sys_to_pcie(sys); + /* remove the configuration aperture in case it is there */ + list_for_each_entry_safe(win, tmp, &pp->resources, list) + if (!resource_type(win->res)) + list_del(&win->list); - if (global_io_offset < SZ_1M && pp->io_size > 0) { - sys->io_offset = global_io_offset - pp->io_bus_addr; - pci_ioremap_io(global_io_offset, pp->io_base); - global_io_offset += SZ_64K; - pci_add_resource_offset(&sys->resources, &pp->io, - sys->io_offset); - } - - sys->mem_offset = pp->mem.start - pp->mem_bus_addr; - pci_add_resource_offset(&sys->resources, &pp->mem, sys->mem_offset); - pci_add_resource(&sys->resources, &pp->busn); + list_splice(&pp->resources, &sys->resources); return 1; } diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h index d0bbd27..2479e9c 100644 --- a/drivers/pci/host/pcie-designware.h +++ b/drivers/pci/host/pcie-designware.h @@ -42,10 +42,7 @@ struct pcie_port { u64 mem_mod_base; phys_addr_t mem_bus_addr; u32 mem_size; - struct resource cfg; - struct resource io; - struct resource mem; - struct resource busn; + struct list_head resources; int irq; u32 lanes; struct pcie_host_ops *ops; -- 2.2.1 ^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API 2015-01-07 15:29 ` Lorenzo Pieralisi (?) @ 2015-01-19 16:40 ` Rob Herring -1 siblings, 0 replies; 32+ messages in thread From: Rob Herring @ 2015-01-19 16:40 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: linux-pci-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree-u79uwXL29TY76Z2rM5mHXA, Arnd Bergmann, Liviu Dudau, Mohit Kumar, Jingoo Han, Bjorn Helgaas, Rob Herring On Wed, Jan 7, 2015 at 9:29 AM, Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org> wrote: > The introduction of the DT API: > > of_pci_get_host_bridge_resources() > > allows to retrieve PCI resources from the device tree in a generic way. > > Some PCI host controllers, in addition to standard IO and MEM resources and > corresponding CPU addresses, require to retrieve the addresses that are used > for bus routing of IO/MEM regions as seen at the bus level corresponding > to the bus segment the host controller is sitting on, so that the host > controller can programme its inbound/outbound memory regions correctly to > respond to bus transactions coming from the CPU on the host controller bus > ports. > > The DT generic layer provides functions that carry out the PCI > ranges <-> resources translation from PCI addresses to CPU addresses (as > seen at the CPU top level bus, through all required hierarchical bus layers), > but there is no generic API to retrieve the PCI range CPU untranslated > addresses, so PCI host controllers have to do it in a driver specific way > through the DT property parsing methods. > > This patchset converts the PCIe designware host controller driver to > use the API: > > of_pci_get_host_bridge_resources() > > in order to retrieve the PCI regions resources. To achieve generality, it > also augments the parsing API so that a new DT struct is created out of > PCI ranges, that contains the PCI resource and the range parser itself > so that it can be passed back to PCI host controllers drivers that > can, if needed, use it to parse the CPU untranslated addresses > corresponding to the resource in question. I don't really like exposing ranges to host drivers. We've worked to not do that. So perhaps we need to rethink the API. I think we need to provide each range as a pair of resources which are the CPU address and PCI address. Perhaps an iterator is kind of pointless here. We do different things for each one. Are there cases with more than a single i/o space, non-prefetch memory and prefetch memory range? Perhaps we should just get the i/o and memory resources as separate calls. Just tossing out some ideas here. > The first patch in the series is a fix/update for the parsing function: > > of_pci_get_host_bridge_resources() This one stands on its own and should go to stable perhap? If so, send it separately. Rob > > It is part of the RFC series since it is a controversial fix to be > discussed before getting merged. > > Patch [2-3] augment the parsing API and convert the PCIe designware > driver code. > > The current API stays unchanged, so that the existing drivers using it > do not need patching and changes are limited. > > Tested on an iMX6 Sabrelite board. > > Cc: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> > Cc: Liviu Dudau <liviu.dudau-5wv7dgnIgG8@public.gmane.org> > Cc: Mohit Kumar <mohit.kumar-qxv4g6HH51o@public.gmane.org> > Cc: Jingoo Han <jg1.han-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> > Cc: Bjorn Helgaas <bhelgaas-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> > Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> > > Lorenzo Pieralisi (3): > drivers: of: fix resources freeing in > of_pci_get_host_bridge_resources() > drivers: of: of_pci_get_host_bridge_resources() range parsing update > drivers: pci: host: update the pcie designware driver to new range > parsing API > > drivers/of/of_pci.c | 36 +++++++-- > drivers/pci/host/pcie-designware.c | 145 +++++++++++++++++-------------------- > drivers/pci/host/pcie-designware.h | 5 +- > include/linux/of_address.h | 5 ++ > 4 files changed, 104 insertions(+), 87 deletions(-) > > -- > 2.2.1 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API @ 2015-01-19 16:40 ` Rob Herring 0 siblings, 0 replies; 32+ messages in thread From: Rob Herring @ 2015-01-19 16:40 UTC (permalink / raw) To: linux-arm-kernel On Wed, Jan 7, 2015 at 9:29 AM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > The introduction of the DT API: > > of_pci_get_host_bridge_resources() > > allows to retrieve PCI resources from the device tree in a generic way. > > Some PCI host controllers, in addition to standard IO and MEM resources and > corresponding CPU addresses, require to retrieve the addresses that are used > for bus routing of IO/MEM regions as seen at the bus level corresponding > to the bus segment the host controller is sitting on, so that the host > controller can programme its inbound/outbound memory regions correctly to > respond to bus transactions coming from the CPU on the host controller bus > ports. > > The DT generic layer provides functions that carry out the PCI > ranges <-> resources translation from PCI addresses to CPU addresses (as > seen at the CPU top level bus, through all required hierarchical bus layers), > but there is no generic API to retrieve the PCI range CPU untranslated > addresses, so PCI host controllers have to do it in a driver specific way > through the DT property parsing methods. > > This patchset converts the PCIe designware host controller driver to > use the API: > > of_pci_get_host_bridge_resources() > > in order to retrieve the PCI regions resources. To achieve generality, it > also augments the parsing API so that a new DT struct is created out of > PCI ranges, that contains the PCI resource and the range parser itself > so that it can be passed back to PCI host controllers drivers that > can, if needed, use it to parse the CPU untranslated addresses > corresponding to the resource in question. I don't really like exposing ranges to host drivers. We've worked to not do that. So perhaps we need to rethink the API. I think we need to provide each range as a pair of resources which are the CPU address and PCI address. Perhaps an iterator is kind of pointless here. We do different things for each one. Are there cases with more than a single i/o space, non-prefetch memory and prefetch memory range? Perhaps we should just get the i/o and memory resources as separate calls. Just tossing out some ideas here. > The first patch in the series is a fix/update for the parsing function: > > of_pci_get_host_bridge_resources() This one stands on its own and should go to stable perhap? If so, send it separately. Rob > > It is part of the RFC series since it is a controversial fix to be > discussed before getting merged. > > Patch [2-3] augment the parsing API and convert the PCIe designware > driver code. > > The current API stays unchanged, so that the existing drivers using it > do not need patching and changes are limited. > > Tested on an iMX6 Sabrelite board. > > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Liviu Dudau <liviu.dudau@arm.com> > Cc: Mohit Kumar <mohit.kumar@st.com> > Cc: Jingoo Han <jg1.han@samsung.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Rob Herring <robh+dt@kernel.org> > > Lorenzo Pieralisi (3): > drivers: of: fix resources freeing in > of_pci_get_host_bridge_resources() > drivers: of: of_pci_get_host_bridge_resources() range parsing update > drivers: pci: host: update the pcie designware driver to new range > parsing API > > drivers/of/of_pci.c | 36 +++++++-- > drivers/pci/host/pcie-designware.c | 145 +++++++++++++++++-------------------- > drivers/pci/host/pcie-designware.h | 5 +- > include/linux/of_address.h | 5 ++ > 4 files changed, 104 insertions(+), 87 deletions(-) > > -- > 2.2.1 > ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API @ 2015-01-19 16:40 ` Rob Herring 0 siblings, 0 replies; 32+ messages in thread From: Rob Herring @ 2015-01-19 16:40 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: linux-pci, linux-arm-kernel, devicetree, Arnd Bergmann, Liviu Dudau, Mohit Kumar, Jingoo Han, Bjorn Helgaas, Rob Herring On Wed, Jan 7, 2015 at 9:29 AM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > The introduction of the DT API: > > of_pci_get_host_bridge_resources() > > allows to retrieve PCI resources from the device tree in a generic way. > > Some PCI host controllers, in addition to standard IO and MEM resources and > corresponding CPU addresses, require to retrieve the addresses that are used > for bus routing of IO/MEM regions as seen at the bus level corresponding > to the bus segment the host controller is sitting on, so that the host > controller can programme its inbound/outbound memory regions correctly to > respond to bus transactions coming from the CPU on the host controller bus > ports. > > The DT generic layer provides functions that carry out the PCI > ranges <-> resources translation from PCI addresses to CPU addresses (as > seen at the CPU top level bus, through all required hierarchical bus layers), > but there is no generic API to retrieve the PCI range CPU untranslated > addresses, so PCI host controllers have to do it in a driver specific way > through the DT property parsing methods. > > This patchset converts the PCIe designware host controller driver to > use the API: > > of_pci_get_host_bridge_resources() > > in order to retrieve the PCI regions resources. To achieve generality, it > also augments the parsing API so that a new DT struct is created out of > PCI ranges, that contains the PCI resource and the range parser itself > so that it can be passed back to PCI host controllers drivers that > can, if needed, use it to parse the CPU untranslated addresses > corresponding to the resource in question. I don't really like exposing ranges to host drivers. We've worked to not do that. So perhaps we need to rethink the API. I think we need to provide each range as a pair of resources which are the CPU address and PCI address. Perhaps an iterator is kind of pointless here. We do different things for each one. Are there cases with more than a single i/o space, non-prefetch memory and prefetch memory range? Perhaps we should just get the i/o and memory resources as separate calls. Just tossing out some ideas here. > The first patch in the series is a fix/update for the parsing function: > > of_pci_get_host_bridge_resources() This one stands on its own and should go to stable perhap? If so, send it separately. Rob > > It is part of the RFC series since it is a controversial fix to be > discussed before getting merged. > > Patch [2-3] augment the parsing API and convert the PCIe designware > driver code. > > The current API stays unchanged, so that the existing drivers using it > do not need patching and changes are limited. > > Tested on an iMX6 Sabrelite board. > > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Liviu Dudau <liviu.dudau@arm.com> > Cc: Mohit Kumar <mohit.kumar@st.com> > Cc: Jingoo Han <jg1.han@samsung.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Rob Herring <robh+dt@kernel.org> > > Lorenzo Pieralisi (3): > drivers: of: fix resources freeing in > of_pci_get_host_bridge_resources() > drivers: of: of_pci_get_host_bridge_resources() range parsing update > drivers: pci: host: update the pcie designware driver to new range > parsing API > > drivers/of/of_pci.c | 36 +++++++-- > drivers/pci/host/pcie-designware.c | 145 +++++++++++++++++-------------------- > drivers/pci/host/pcie-designware.h | 5 +- > include/linux/of_address.h | 5 ++ > 4 files changed, 104 insertions(+), 87 deletions(-) > > -- > 2.2.1 > ^ permalink raw reply [flat|nested] 32+ messages in thread
[parent not found: <CAL_JsqLYg9vhFVmf=O4YLPfboj-dyoCR0H5dzBbMC3PeAEOwhQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
* Re: [RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API 2015-01-19 16:40 ` Rob Herring (?) @ 2015-01-19 16:59 ` Arnd Bergmann -1 siblings, 0 replies; 32+ messages in thread From: Arnd Bergmann @ 2015-01-19 16:59 UTC (permalink / raw) To: Rob Herring Cc: Lorenzo Pieralisi, linux-pci-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree-u79uwXL29TY76Z2rM5mHXA, Liviu Dudau, Mohit Kumar, Jingoo Han, Bjorn Helgaas, Rob Herring On Monday 19 January 2015 10:40:39 Rob Herring wrote: > > I don't really like exposing ranges to host drivers. We've worked to > not do that. So perhaps we need to rethink the API. I think we need to > provide each range as a pair of resources which are the CPU address > and PCI address. Perhaps an iterator is kind of pointless here. We do > different things for each one. Are there cases with more than a single > i/o space, non-prefetch memory and prefetch memory range? Perhaps we > should just get the i/o and memory resources as separate calls. Just > tossing out some ideas here. Nice idea, that could be similar to platform_get_resource(). We probably also need the distinction between CPU address and (parent) bus address here. In most drivers they are the same, but we actually need to program the latter one into the PCI host bridge registers. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API @ 2015-01-19 16:59 ` Arnd Bergmann 0 siblings, 0 replies; 32+ messages in thread From: Arnd Bergmann @ 2015-01-19 16:59 UTC (permalink / raw) To: linux-arm-kernel On Monday 19 January 2015 10:40:39 Rob Herring wrote: > > I don't really like exposing ranges to host drivers. We've worked to > not do that. So perhaps we need to rethink the API. I think we need to > provide each range as a pair of resources which are the CPU address > and PCI address. Perhaps an iterator is kind of pointless here. We do > different things for each one. Are there cases with more than a single > i/o space, non-prefetch memory and prefetch memory range? Perhaps we > should just get the i/o and memory resources as separate calls. Just > tossing out some ideas here. Nice idea, that could be similar to platform_get_resource(). We probably also need the distinction between CPU address and (parent) bus address here. In most drivers they are the same, but we actually need to program the latter one into the PCI host bridge registers. Arnd ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API @ 2015-01-19 16:59 ` Arnd Bergmann 0 siblings, 0 replies; 32+ messages in thread From: Arnd Bergmann @ 2015-01-19 16:59 UTC (permalink / raw) To: Rob Herring Cc: Lorenzo Pieralisi, linux-pci, linux-arm-kernel, devicetree, Liviu Dudau, Mohit Kumar, Jingoo Han, Bjorn Helgaas, Rob Herring On Monday 19 January 2015 10:40:39 Rob Herring wrote: > > I don't really like exposing ranges to host drivers. We've worked to > not do that. So perhaps we need to rethink the API. I think we need to > provide each range as a pair of resources which are the CPU address > and PCI address. Perhaps an iterator is kind of pointless here. We do > different things for each one. Are there cases with more than a single > i/o space, non-prefetch memory and prefetch memory range? Perhaps we > should just get the i/o and memory resources as separate calls. Just > tossing out some ideas here. Nice idea, that could be similar to platform_get_resource(). We probably also need the distinction between CPU address and (parent) bus address here. In most drivers they are the same, but we actually need to program the latter one into the PCI host bridge registers. Arnd ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API 2015-01-19 16:59 ` Arnd Bergmann (?) @ 2015-01-20 10:39 ` Lorenzo Pieralisi -1 siblings, 0 replies; 32+ messages in thread From: Lorenzo Pieralisi @ 2015-01-20 10:39 UTC (permalink / raw) To: Arnd Bergmann Cc: Rob Herring, linux-pci-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, devicetree-u79uwXL29TY76Z2rM5mHXA, Liviu Dudau, Mohit Kumar, Jingoo Han, Bjorn Helgaas, Rob Herring On Mon, Jan 19, 2015 at 04:59:00PM +0000, Arnd Bergmann wrote: > On Monday 19 January 2015 10:40:39 Rob Herring wrote: > > > > I don't really like exposing ranges to host drivers. We've worked to > > not do that. So perhaps we need to rethink the API. I think we need to > > provide each range as a pair of resources which are the CPU address > > and PCI address. Perhaps an iterator is kind of pointless here. We do > > different things for each one. Are there cases with more than a single > > i/o space, non-prefetch memory and prefetch memory range? Perhaps we > > should just get the i/o and memory resources as separate calls. Just > > tossing out some ideas here. > > Nice idea, that could be similar to platform_get_resource(). I like the idea too, it should simplify the API implementation. > We probably also need the distinction between CPU address and (parent) > bus address here. In most drivers they are the same, but we actually > need to program the latter one into the PCI host bridge registers. Yes, CPU untranslated addresses are a pain in the back. I wrote the series at it is to avoid changing the API, but I agree that's a bit convoluted, which means we should refactor it. I think the API should always return a pair of CPU-PCI resources as Rob said, and provide an "on-demand" request for untranslated addresses, since their usage is not that common (at the moment). I need to think about that, we might even return a triplet, I hate doing that since I fear it might be a one-off need for PCIe designware. Lorenzo -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API @ 2015-01-20 10:39 ` Lorenzo Pieralisi 0 siblings, 0 replies; 32+ messages in thread From: Lorenzo Pieralisi @ 2015-01-20 10:39 UTC (permalink / raw) To: linux-arm-kernel On Mon, Jan 19, 2015 at 04:59:00PM +0000, Arnd Bergmann wrote: > On Monday 19 January 2015 10:40:39 Rob Herring wrote: > > > > I don't really like exposing ranges to host drivers. We've worked to > > not do that. So perhaps we need to rethink the API. I think we need to > > provide each range as a pair of resources which are the CPU address > > and PCI address. Perhaps an iterator is kind of pointless here. We do > > different things for each one. Are there cases with more than a single > > i/o space, non-prefetch memory and prefetch memory range? Perhaps we > > should just get the i/o and memory resources as separate calls. Just > > tossing out some ideas here. > > Nice idea, that could be similar to platform_get_resource(). I like the idea too, it should simplify the API implementation. > We probably also need the distinction between CPU address and (parent) > bus address here. In most drivers they are the same, but we actually > need to program the latter one into the PCI host bridge registers. Yes, CPU untranslated addresses are a pain in the back. I wrote the series at it is to avoid changing the API, but I agree that's a bit convoluted, which means we should refactor it. I think the API should always return a pair of CPU-PCI resources as Rob said, and provide an "on-demand" request for untranslated addresses, since their usage is not that common (at the moment). I need to think about that, we might even return a triplet, I hate doing that since I fear it might be a one-off need for PCIe designware. Lorenzo ^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API @ 2015-01-20 10:39 ` Lorenzo Pieralisi 0 siblings, 0 replies; 32+ messages in thread From: Lorenzo Pieralisi @ 2015-01-20 10:39 UTC (permalink / raw) To: Arnd Bergmann Cc: Rob Herring, linux-pci, linux-arm-kernel, devicetree, Liviu Dudau, Mohit Kumar, Jingoo Han, Bjorn Helgaas, Rob Herring On Mon, Jan 19, 2015 at 04:59:00PM +0000, Arnd Bergmann wrote: > On Monday 19 January 2015 10:40:39 Rob Herring wrote: > > > > I don't really like exposing ranges to host drivers. We've worked to > > not do that. So perhaps we need to rethink the API. I think we need to > > provide each range as a pair of resources which are the CPU address > > and PCI address. Perhaps an iterator is kind of pointless here. We do > > different things for each one. Are there cases with more than a single > > i/o space, non-prefetch memory and prefetch memory range? Perhaps we > > should just get the i/o and memory resources as separate calls. Just > > tossing out some ideas here. > > Nice idea, that could be similar to platform_get_resource(). I like the idea too, it should simplify the API implementation. > We probably also need the distinction between CPU address and (parent) > bus address here. In most drivers they are the same, but we actually > need to program the latter one into the PCI host bridge registers. Yes, CPU untranslated addresses are a pain in the back. I wrote the series at it is to avoid changing the API, but I agree that's a bit convoluted, which means we should refactor it. I think the API should always return a pair of CPU-PCI resources as Rob said, and provide an "on-demand" request for untranslated addresses, since their usage is not that common (at the moment). I need to think about that, we might even return a triplet, I hate doing that since I fear it might be a one-off need for PCIe designware. Lorenzo ^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2015-01-26 13:06 UTC | newest] Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-01-07 15:29 [RFC PATCH 0/3] drivers: port PCIe designware to new DT parsing API Lorenzo Pieralisi 2015-01-07 15:29 ` Lorenzo Pieralisi 2015-01-07 15:29 ` [RFC PATCH 1/3] drivers: of: fix resources freeing in of_pci_get_host_bridge_resources() Lorenzo Pieralisi 2015-01-07 15:29 ` Lorenzo Pieralisi 2015-01-19 18:32 ` Liviu Dudau 2015-01-19 18:32 ` Liviu Dudau 2015-01-20 10:49 ` Lorenzo Pieralisi 2015-01-20 10:49 ` Lorenzo Pieralisi 2015-01-20 10:49 ` Lorenzo Pieralisi 2015-01-20 11:20 ` Liviu Dudau 2015-01-20 11:20 ` Liviu Dudau 2015-01-20 11:20 ` Liviu Dudau [not found] ` <20150120112031.GA342-hOhETlTuV5niMG9XS5x8Mg@public.gmane.org> 2015-01-26 11:21 ` Lorenzo Pieralisi 2015-01-26 11:21 ` Lorenzo Pieralisi 2015-01-26 11:21 ` Lorenzo Pieralisi 2015-01-26 13:06 ` Liviu Dudau 2015-01-26 13:06 ` Liviu Dudau 2015-01-26 13:06 ` Liviu Dudau 2015-01-07 15:29 ` [RFC PATCH 2/3] drivers: of: of_pci_get_host_bridge_resources() range parsing update Lorenzo Pieralisi 2015-01-07 15:29 ` Lorenzo Pieralisi [not found] ` <1420644571-18928-1-git-send-email-lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org> 2015-01-07 15:29 ` [RFC PATCH 3/3] drivers: pci: host: update the pcie designware driver to new range parsing API Lorenzo Pieralisi 2015-01-07 15:29 ` Lorenzo Pieralisi 2015-01-07 15:29 ` Lorenzo Pieralisi 2015-01-19 16:40 ` [RFC PATCH 0/3] drivers: port PCIe designware to new DT " Rob Herring 2015-01-19 16:40 ` Rob Herring 2015-01-19 16:40 ` Rob Herring [not found] ` <CAL_JsqLYg9vhFVmf=O4YLPfboj-dyoCR0H5dzBbMC3PeAEOwhQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2015-01-19 16:59 ` Arnd Bergmann 2015-01-19 16:59 ` Arnd Bergmann 2015-01-19 16:59 ` Arnd Bergmann 2015-01-20 10:39 ` Lorenzo Pieralisi 2015-01-20 10:39 ` Lorenzo Pieralisi 2015-01-20 10:39 ` Lorenzo Pieralisi
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.