| Message ID | 1393535872-20915-5-git-send-email-santosh.shilimkar@ti.com |
|---|---|
| State | New |
| Headers | show |
On Friday 28 February 2014 04:39 AM, Arnd Bergmann wrote: > On Thursday 27 February 2014 16:17:49 Santosh Shilimkar wrote: >> + >> +/** >> + * of_dma_is_coherent - Check if device is coherent >> + * @np: device node >> + * >> + * It returns true if "dma-coherent" property was found >> + * for this device in DT. >> + */ >> +bool of_dma_is_coherent(struct device_node *np) >> +{ >> + struct device_node *node = np; >> + >> + while (node) { >> + if (of_property_read_bool(node, "dma-coherent")) { >> + of_node_put(node); >> + return true; >> + } >> + node = of_get_next_parent(node); >> + } >> + return false; >> +} >> +EXPORT_SYMBOL_GPL(of_dma_is_coherent); >> > > This won't work on architectures that are always coherent and > did not need 'dma-coherent' properties before, such as IBM > Power servers. > > That said, I think the property makes sense, and we already have > platforms using it (highbank is the one I'm aware of). > > We probably need ways to override this function in both ways: > "always coherent" (powerpc, x86), and "never coherent" (keystone > without LPAE) from platform code, and it would be nice to put > either option into DT in a global location as well. We may have > to go through a few iterations of this patch to get the best > algorithm, but I think the interface is good at least. > Probably we should discuss bit more next week at connect. The current 'dma-coherent' is a per device property. For arch's which are always coherent, the per device property doesn't make sense. BTW, the current users of this API is only ARM32 bit port and if this satisfies the ARM platforms, we should get this in kernel and then address other cases on need basis. Regards, Santosh -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 28, 2014 at 3:39 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Thursday 27 February 2014 16:17:49 Santosh Shilimkar wrote: >> + >> +/** >> + * of_dma_is_coherent - Check if device is coherent >> + * @np: device node >> + * >> + * It returns true if "dma-coherent" property was found >> + * for this device in DT. >> + */ >> +bool of_dma_is_coherent(struct device_node *np) >> +{ >> + struct device_node *node = np; >> + >> + while (node) { >> + if (of_property_read_bool(node, "dma-coherent")) { >> + of_node_put(node); >> + return true; >> + } >> + node = of_get_next_parent(node); >> + } >> + return false; >> +} >> +EXPORT_SYMBOL_GPL(of_dma_is_coherent); >> > > This won't work on architectures that are always coherent and > did not need 'dma-coherent' properties before, such as IBM > Power servers. > > That said, I think the property makes sense, and we already have > platforms using it (highbank is the one I'm aware of). > > We probably need ways to override this function in both ways: > "always coherent" (powerpc, x86), and "never coherent" (keystone > without LPAE) from platform code, and it would be nice to put > either option into DT in a global location as well. We may have > to go through a few iterations of this patch to get the best > algorithm, but I think the interface is good at least. I know Will D was not a fan of this property. Primarily I believe because you may need to describe more than just a boolean in more complex bus topologies. Effectively, highbank is always coherent. It was only PCI that is non-coherent, but I can safely say PCI will never be enabled at this point. There are no designs with PCI beyond 1 or 2 validation boards (total boards, not designs), and getting PCI to work was quite hacky due to only a 1MB window. The other masters are programmable, but only the coherent path is used as the non-coherent path actually has some issues. I had expected the opposite believing the ACP port would actually have issues which is also why I made it configurable. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 28 February 2014 10:14 AM, Rob Herring wrote: > On Fri, Feb 28, 2014 at 3:39 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> On Thursday 27 February 2014 16:17:49 Santosh Shilimkar wrote: >>> + >>> +/** >>> + * of_dma_is_coherent - Check if device is coherent >>> + * @np: device node >>> + * >>> + * It returns true if "dma-coherent" property was found >>> + * for this device in DT. >>> + */ >>> +bool of_dma_is_coherent(struct device_node *np) >>> +{ >>> + struct device_node *node = np; >>> + >>> + while (node) { >>> + if (of_property_read_bool(node, "dma-coherent")) { >>> + of_node_put(node); >>> + return true; >>> + } >>> + node = of_get_next_parent(node); >>> + } >>> + return false; >>> +} >>> +EXPORT_SYMBOL_GPL(of_dma_is_coherent); >>> >> >> This won't work on architectures that are always coherent and >> did not need 'dma-coherent' properties before, such as IBM >> Power servers. >> >> That said, I think the property makes sense, and we already have >> platforms using it (highbank is the one I'm aware of). >> >> We probably need ways to override this function in both ways: >> "always coherent" (powerpc, x86), and "never coherent" (keystone >> without LPAE) from platform code, and it would be nice to put >> either option into DT in a global location as well. We may have >> to go through a few iterations of this patch to get the best >> algorithm, but I think the interface is good at least. > > I know Will D was not a fan of this property. Primarily I believe > because you may need to describe more than just a boolean in more > complex bus topologies. > > Effectively, highbank is always coherent. It was only PCI that is > non-coherent, but I can safely say PCI will never be enabled at this > point. There are no designs with PCI beyond 1 or 2 validation boards > (total boards, not designs), and getting PCI to work was quite hacky > due to only a 1MB window. The other masters are programmable, but only > the coherent path is used as the non-coherent path actually has some > issues. I had expected the opposite believing the ACP port would > actually have issues which is also why I made it configurable. > I also like it to be configurable per device since you can handle the broken masters if any. On Keystone we have one of the master broken(non-PCI) and this helps to take care of that case. Regards, Santosh -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 28 February 2014 09:14:19 Rob Herring wrote: > > I know Will D was not a fan of this property. Primarily I believe > because you may need to describe more than just a boolean in more > complex bus topologies. I can't think of any example where it's not per-device. Do you think we can end up with a device that has multiple bus master ports, only some of which are coherent, or is there a different concern? > Effectively, highbank is always coherent. It was only PCI that is > non-coherent, but I can safely say PCI will never be enabled at this > point. There are no designs with PCI beyond 1 or 2 validation boards > (total boards, not designs), and getting PCI to work was quite hacky > due to only a 1MB window. The other masters are programmable, but only > the coherent path is used as the non-coherent path actually has some > issues. I had expected the opposite believing the ACP port would > actually have issues which is also why I made it configurable. Ok, I see. I still expect that we will see systems that are only partially coherent in the future, but it's good to know we don't really have to deal with backwards-compatibility as long as we can just hardcode highbank to be always coherent. I'm especially thankful we don't have to deal with the PCI implementation. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Adding Will... On Fri, Feb 28, 2014 at 9:24 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Friday 28 February 2014 09:14:19 Rob Herring wrote: >> >> I know Will D was not a fan of this property. Primarily I believe >> because you may need to describe more than just a boolean in more >> complex bus topologies. > > I can't think of any example where it's not per-device. Do you > think we can end up with a device that has multiple bus master > ports, only some of which are coherent, or is there a different > concern? Perhaps Will can post his slides from the ARM kernel summit or chime in here, but I believe it was more that coherency is just one aspect of bus master bus topology. The DT models the slave bus hierarchy and doesn't model the master side which is becoming more complex and needing to be described. In the case of highbank, you have an ACP bus with some number of masters on it. They are configurable, but it is not the device that is configurable, but really which bus they are connected to. > >> Effectively, highbank is always coherent. It was only PCI that is >> non-coherent, but I can safely say PCI will never be enabled at this >> point. There are no designs with PCI beyond 1 or 2 validation boards >> (total boards, not designs), and getting PCI to work was quite hacky >> due to only a 1MB window. The other masters are programmable, but only >> the coherent path is used as the non-coherent path actually has some >> issues. I had expected the opposite believing the ACP port would >> actually have issues which is also why I made it configurable. > > Ok, I see. I still expect that we will see systems that are only > partially coherent in the future, but it's good to know we don't really > have to deal with backwards-compatibility as long as we can just > hardcode highbank to be always coherent. > > I'm especially thankful we don't have to deal with the PCI implementation. Yeah, you and me both. Rob -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 03, 2014 at 02:04:17PM +0000, Rob Herring wrote: > Adding Will... Thanks Rob! > On Fri, Feb 28, 2014 at 9:24 AM, Arnd Bergmann <arnd@arndb.de> wrote: > > On Friday 28 February 2014 09:14:19 Rob Herring wrote: > >> > >> I know Will D was not a fan of this property. Primarily I believe > >> because you may need to describe more than just a boolean in more > >> complex bus topologies. > > > > I can't think of any example where it's not per-device. Do you > > think we can end up with a device that has multiple bus master > > ports, only some of which are coherent, or is there a different > > concern? > > Perhaps Will can post his slides from the ARM kernel summit or chime > in here, but I believe it was more that coherency is just one aspect > of bus master bus topology. The DT models the slave bus hierarchy and > doesn't model the master side which is becoming more complex and > needing to be described. In the case of highbank, you have an ACP bus > with some number of masters on it. They are configurable, but it is > not the device that is configurable, but really which bus they are > connected to. I'm not sure what happened to this slides but, yes, you have the right idea. It's not only DMA coherency that's directly related to master topology, but also SMMU configuration and MSI routing will depend on this information. Using a per-device, boolean property to describe DMA coherency may work for a simple, statically configured system, but it really just avoids the bigger issue. Will -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/dma/of-dma.c b/drivers/dma/of-dma.c index 9b51768..c5958d1 100644 --- a/drivers/dma/of-dma.c +++ b/drivers/dma/of-dma.c @@ -304,3 +304,25 @@ out: return ret; } EXPORT_SYMBOL_GPL(of_dma_get_range); + +/** + * of_dma_is_coherent - Check if device is coherent + * @np: device node + * + * It returns true if "dma-coherent" property was found + * for this device in DT. + */ +bool of_dma_is_coherent(struct device_node *np) +{ + struct device_node *node = np; + + while (node) { + if (of_property_read_bool(node, "dma-coherent")) { + of_node_put(node); + return true; + } + node = of_get_next_parent(node); + } + return false; +} +EXPORT_SYMBOL_GPL(of_dma_is_coherent); diff --git a/include/linux/of_dma.h b/include/linux/of_dma.h index f04171a..6191f02 100644 --- a/include/linux/of_dma.h +++ b/include/linux/of_dma.h @@ -44,6 +44,7 @@ extern struct dma_chan *of_dma_simple_xlate(struct of_phandle_args *dma_spec, extern int of_dma_get_range(struct device_node *np, dma_addr_t *dma_addr, phys_addr_t *paddr, phys_addr_t *size); +extern bool of_dma_is_coherent(struct device_node *np); #else static inline int of_dma_controller_register(struct device_node *np, struct dma_chan *(*of_dma_xlate) @@ -74,6 +75,10 @@ static inline int of_dma_get_range(struct device_node *np, dma_addr_t *dma_addr, { return -ENODEV; } +static inline bool of_dma_is_coherent(struct device_node *np) +{ + return false; +} #endif #endif /* __LINUX_OF_DMA_H */
The of_dma_is_coherent() helper parses the given DT device node to see if the "dma-coherent" property is supported and returns true or false accordingly. Cc: Russell King <linux@arm.linux.org.uk> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Olof Johansson <olof@lixom.net> Cc: Grant Likely <grant.likely@linaro.org> Cc: Rob Herring <robh+dt@kernel.org> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> --- drivers/dma/of-dma.c | 22 ++++++++++++++++++++++ include/linux/of_dma.h | 5 +++++ 2 files changed, 27 insertions(+)