| Message ID | 1394097598-17622-4-git-send-email-santosh.shilimkar@ti.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Mar 6, 2014 at 3:19 AM, Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: > 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. > > For the architectures which are fully dma coherent and don't need per device > property, it can enable CONFIG_ARCH_IS_DMA_COHERENT config option which > enables DMA coherent for all devices by default. This worries me. I killed off arch_is_coherent() for arm. Now we're adding something back. Also, we already have HAVE_GENERIC_DMA_COHERENT which is different, but the names will be confusing. MIPS also has DMA_NONCOHERENT. > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > 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> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > --- > drivers/of/platform.c | 31 +++++++++++++++++++++++++++++++ > include/linux/of_platform.h | 6 ++++++ > 2 files changed, 37 insertions(+) > > diff --git a/drivers/of/platform.c b/drivers/of/platform.c > index 2265a55..bd080db 100644 > --- a/drivers/of/platform.c > +++ b/drivers/of/platform.c > @@ -570,4 +570,35 @@ 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. > + */ > +#ifndef CONFIG_ARCH_IS_DMA_COHERENT > +bool of_dma_is_coherent(struct device_node *np) > +{ > + struct device_node *node = np; > + > + while (node) { This screams for a for_each_parent_of_node helper. > + 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); > +#else > +inline bool of_dma_is_coherent(struct device_node *np) This is in the header and should not be here. > +{ > + /* ARCH is fully dma coherent and don't need per device control */ > + return true; > +} > +EXPORT_SYMBOL_GPL(of_dma_is_coherent); > +#endif > #endif /* CONFIG_OF_ADDRESS */ > diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h > index ba7d3dc..48e0748 100644 > --- a/include/linux/of_platform.h > +++ b/include/linux/of_platform.h > @@ -74,6 +74,7 @@ extern int of_platform_populate(struct device_node *root, > struct device *parent); > 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_platform_populate(struct device_node *root, > const struct of_device_id *matches, > @@ -88,6 +89,11 @@ 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 I don't think you have the ifdefs right here. 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 07 March 2014 11:13 AM, Rob Herring wrote: > On Thu, Mar 6, 2014 at 3:19 AM, Santosh Shilimkar > <santosh.shilimkar@ti.com> wrote: >> 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. >> >> For the architectures which are fully dma coherent and don't need per device >> property, it can enable CONFIG_ARCH_IS_DMA_COHERENT config option which >> enables DMA coherent for all devices by default. > > This worries me. I killed off arch_is_coherent() for arm. Now we're > adding something back. Also, we already have HAVE_GENERIC_DMA_COHERENT > which is different, but the names will be confusing. MIPS also has > DMA_NONCOHERENT. > Thanks for comments Rob. I will address them in next version. Specifically about ARCH_IS_DMA_COHERENT, I wasn't very comfortable either while adding it. But as Arnd mentioned, there is a need to have a way for the arch's which are fully coherent to use coherent ops by default. Am not sure whats the best way to have such support without imposing any special updates on such arches. Arnd, Any better alternative here ? 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 Thu, Mar 6, 2014 at 9:44 PM, Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: > On Friday 07 March 2014 11:13 AM, Rob Herring wrote: >> On Thu, Mar 6, 2014 at 3:19 AM, Santosh Shilimkar >> <santosh.shilimkar@ti.com> wrote: >>> 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. >>> >>> For the architectures which are fully dma coherent and don't need per device >>> property, it can enable CONFIG_ARCH_IS_DMA_COHERENT config option which >>> enables DMA coherent for all devices by default. >> >> This worries me. I killed off arch_is_coherent() for arm. Now we're >> adding something back. Also, we already have HAVE_GENERIC_DMA_COHERENT >> which is different, but the names will be confusing. MIPS also has >> DMA_NONCOHERENT. >> > Thanks for comments Rob. I will address them in next version. > Specifically about ARCH_IS_DMA_COHERENT, I wasn't very comfortable either > while adding it. But as Arnd mentioned, there is a need to have a way > for the arch's which are fully coherent to use coherent ops by default. > > Am not sure whats the best way to have such support without imposing > any special updates on such arches. Thinking about this some more, if the arch is always coherent or always non-coherent, then the default ops are always fine. In that case set_arch_dma_coherent_ops is always a nop and of_dma_is_coherent is a don't care. 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 07 March 2014 11:55 AM, Rob Herring wrote: > On Thu, Mar 6, 2014 at 9:44 PM, Santosh Shilimkar > <santosh.shilimkar@ti.com> wrote: >> On Friday 07 March 2014 11:13 AM, Rob Herring wrote: >>> On Thu, Mar 6, 2014 at 3:19 AM, Santosh Shilimkar >>> <santosh.shilimkar@ti.com> wrote: >>>> 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. >>>> >>>> For the architectures which are fully dma coherent and don't need per device >>>> property, it can enable CONFIG_ARCH_IS_DMA_COHERENT config option which >>>> enables DMA coherent for all devices by default. >>> >>> This worries me. I killed off arch_is_coherent() for arm. Now we're >>> adding something back. Also, we already have HAVE_GENERIC_DMA_COHERENT >>> which is different, but the names will be confusing. MIPS also has >>> DMA_NONCOHERENT. >>> >> Thanks for comments Rob. I will address them in next version. >> Specifically about ARCH_IS_DMA_COHERENT, I wasn't very comfortable either >> while adding it. But as Arnd mentioned, there is a need to have a way >> for the arch's which are fully coherent to use coherent ops by default. >> >> Am not sure whats the best way to have such support without imposing >> any special updates on such arches. > > Thinking about this some more, if the arch is always coherent or > always non-coherent, then the default ops are always fine. In that > case set_arch_dma_coherent_ops is always a nop and of_dma_is_coherent > is a don't care. > Hmmm.. I guess you are right. In that case we can drop the need of config option. 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 07 March 2014, Santosh Shilimkar wrote: > On Friday 07 March 2014 11:55 AM, Rob Herring wrote: > > > Thinking about this some more, if the arch is always coherent or > > always non-coherent, then the default ops are always fine. In that > > case set_arch_dma_coherent_ops is always a nop and of_dma_is_coherent > > is a don't care. > > > Hmmm.. I guess you are right. In that case we can drop the need of > config option. A compile-time config option clearly would not work here, because it breaks multiplatform support when some platforms are different from others. I can still see the possible need for a global run-time setting though, to give some platform init code the ability to override the DT flags when it knows better. That is probably what you want to do on keystone without LPAE. 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
On Saturday 08 March 2014 12:09 AM, Arnd Bergmann wrote: > On Friday 07 March 2014, Santosh Shilimkar wrote: >> On Friday 07 March 2014 11:55 AM, Rob Herring wrote: >> >>> Thinking about this some more, if the arch is always coherent or >>> always non-coherent, then the default ops are always fine. In that >>> case set_arch_dma_coherent_ops is always a nop and of_dma_is_coherent >>> is a don't care. >>> >> Hmmm.. I guess you are right. In that case we can drop the need of >> config option. > > A compile-time config option clearly would not work here, because it > breaks multiplatform support when some platforms are different from > others. I can still see the possible need for a global run-time setting > though, to give some platform init code the ability to override the > DT flags when it knows better. That is probably what you want to do > on keystone without LPAE. > The compile time flag added was keeping in mind to be used per architecture like ARM, X86 or powerpc on need basis. I understand that it might be used by machines within the architectures but that was not my intention. In any case config bit isn't a good idea. Keystone without LPAE actually I don't need such an option. For non-LPAE case, I need to modify the memory node and same goes with dma-ranges info to use aliased address space. How about we care about such an option when we actually need it. As Rob pointed out, we are just fine for now with sane defaults. Regards, Santosh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
diff --git a/drivers/of/platform.c b/drivers/of/platform.c index 2265a55..bd080db 100644 --- a/drivers/of/platform.c +++ b/drivers/of/platform.c @@ -570,4 +570,35 @@ 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. + */ +#ifndef CONFIG_ARCH_IS_DMA_COHERENT +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); +#else +inline bool of_dma_is_coherent(struct device_node *np) +{ + /* ARCH is fully dma coherent and don't need per device control */ + return true; +} +EXPORT_SYMBOL_GPL(of_dma_is_coherent); +#endif #endif /* CONFIG_OF_ADDRESS */ diff --git a/include/linux/of_platform.h b/include/linux/of_platform.h index ba7d3dc..48e0748 100644 --- a/include/linux/of_platform.h +++ b/include/linux/of_platform.h @@ -74,6 +74,7 @@ extern int of_platform_populate(struct device_node *root, struct device *parent); 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_platform_populate(struct device_node *root, const struct of_device_id *matches, @@ -88,6 +89,11 @@ 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_PLATFORM_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. For the architectures which are fully dma coherent and don't need per device property, it can enable CONFIG_ARCH_IS_DMA_COHERENT config option which enables DMA coherent for all devices by default. Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Linus Walleij <linus.walleij@linaro.org> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com> --- drivers/of/platform.c | 31 +++++++++++++++++++++++++++++++ include/linux/of_platform.h | 6 ++++++ 2 files changed, 37 insertions(+)