| Message ID | 1340176620-13012-1-git-send-email-hdoyu@nvidia.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 06/20/2012 01:16 AM, Hiroshi DOYU wrote: > From: Hiroshi Doyu <hdoyu@nvidia.com> > > This code was based on: > "arch/microblaze/kernel/prom_parse.c" > "arch/powerpc/kernel/prom_parse.c" > > Can be promoted as a global function for general use to replace > "of_parse_dma_window()" in the above. This supports different formats > flexibly. "prefix" can be configured if any. "busno" and "index" are > optionally specified. Set NULL and 0 if not used. > > Signed-off-by: Hiroshi DOYU <hdoyu@nvidia.com> > --- > Based on the discussion: > http://marc.info/?l=linux-tegra&m=133732046606458&w=2 Hmmm. This function really should be in some common location and available for all drivers to use. Can't we add it to that common location from the start? What prevented the earlier patch that did this from getting merged into 3.5? One thing that might help here would be /not/ to add the common code to drivers/of/of_dma.c as was done in the earlier revisions of this patch - I believe that Grant has been trying to push subsystem-specific OF functionality into files in those individual subsystems, so that drivers/of can be kept for core support. Perhaps this patch should create drivers/iommu/of_iommu.c or similar? But I wonder: Is this function likely to be useful outside of drivers/iommu/ - you mentioned that similar code already exists in the two arch-specific prom_parse.c files; where are the existing users of those functions. If not in drivers/iommu/, then probably drivers/iommu/ isn't a good place to put the new common function... > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c > +static int of_get_dma_window(struct device_node *dn, > + const char *prefix, int index, > + unsigned long *busno, > + dma_addr_t *addr, size_t *size) > + const char *s = ""; > + if (prefix) > + s = prefix; One minor nit, you could just do this and remove variable s: if (!prefix) prefix = ""; -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Stephen Warren <swarren@wwwdotorg.org> wrote @ Wed, 20 Jun 2012 19:11:51 +0200: > On 06/20/2012 01:16 AM, Hiroshi DOYU wrote: > > From: Hiroshi Doyu <hdoyu@nvidia.com> > > > > This code was based on: > > "arch/microblaze/kernel/prom_parse.c" > > "arch/powerpc/kernel/prom_parse.c" > > > > Can be promoted as a global function for general use to replace > > "of_parse_dma_window()" in the above. This supports different formats > > flexibly. "prefix" can be configured if any. "busno" and "index" are > > optionally specified. Set NULL and 0 if not used. > > > > Signed-off-by: Hiroshi DOYU <hdoyu@nvidia.com> > > --- > > Based on the discussion: > > http://marc.info/?l=linux-tegra&m=133732046606458&w=2 > > Hmmm. This function really should be in some common location and > available for all drivers to use. Can't we add it to that common > location from the start? What prevented the earlier patch that did this > from getting merged into 3.5? It's because there was no feedback against the original patches(the common location ones, *1,*2) from DT side. *1: http://article.gmane.org/gmane.linux.ports.tegra/4468 *2: https://lkml.org/lkml/2012/4/30/153 > One thing that might help here would be /not/ to add the common code to > drivers/of/of_dma.c as was done in the earlier revisions of this patch - > I believe that Grant has been trying to push subsystem-specific OF > functionality into files in those individual subsystems, so that > drivers/of can be kept for core support. Perhaps this patch should > create drivers/iommu/of_iommu.c or similar? "drivers/iommu/of_iommu.c" seems quite reasonable for this function. > But I wonder: Is this function likely to be useful outside of > drivers/iommu/ - you mentioned that similar code already exists in the > two arch-specific prom_parse.c files; where are the existing users of > those functions. If not in drivers/iommu/, then probably drivers/iommu/ > isn't a good place to put the new common function... There are the following 3 users of of_parse_dma_window() as below. All of them are IOMMU related, but they are architecture specific ones, not for the standard IOMMU API. I guess that the current trend is to convert Arch specific IOMMU API to the standard one basically. arch/powerpc/kernel/vio.c of_parse_dma_window(dev->dev.of_node, dma_window, arch/powerpc/platforms/cell/iommu.c of_parse_dma_window(np, dma_window, &index, base, size); arch/powerpc/platforms/pseries/iommu.c of_parse_dma_window(dn, dma_window, &tbl->it_index, &offset, &size); I think that the common "dma-window" DT parser is necessary for the standard IOMMU because "dma-window" info is dealt as DOMAIN_ATTR_GEOMETRY in the following Joerg's patch too. [PATCH 0/5] IOMMU: Make IOMMU-API ready for GART-like hardware https://lkml.org/lkml/2012/1/19/170 If it's ok to have of_get_dma_window() in "drivers/iommu/of_iommu.c", I'll post that version. Any comment? -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/21/2012 12:46 AM, Hiroshi Doyu wrote: ... > There are the following 3 users of of_parse_dma_window() as below. All > of them are IOMMU related, but they are architecture specific ones, > not for the standard IOMMU API. I guess that the current trend is to > convert Arch specific IOMMU API to the standard one basically. > > arch/powerpc/kernel/vio.c of_parse_dma_window(dev->dev.of_node, dma_window, > arch/powerpc/platforms/cell/iommu.c of_parse_dma_window(np, dma_window, &index, base, size); > arch/powerpc/platforms/pseries/iommu.c of_parse_dma_window(dn, dma_window, &tbl->it_index, &offset, &size); > > I think that the common "dma-window" DT parser is necessary for the > standard IOMMU because "dma-window" info is dealt as > DOMAIN_ATTR_GEOMETRY in the following Joerg's patch too. > > [PATCH 0/5] IOMMU: Make IOMMU-API ready for GART-like hardware > https://lkml.org/lkml/2012/1/19/170 > > If it's ok to have of_get_dma_window() in "drivers/iommu/of_iommu.c", > I'll post that version. Sounds sane to me. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" 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/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c index ecd6790..6f87ae4 100644 --- a/drivers/iommu/tegra-smmu.c +++ b/drivers/iommu/tegra-smmu.c @@ -30,6 +30,7 @@ #include <linux/sched.h> #include <linux/iommu.h> #include <linux/io.h> +#include <linux/of.h> #include <asm/page.h> #include <asm/cacheflush.h> @@ -858,6 +859,75 @@ static struct iommu_ops smmu_iommu_ops = { .pgsize_bitmap = SMMU_IOMMU_PGSIZES, }; +/** + * of_get_dma_window - Parse *dma-window property and returns 0 if found. + * + * @dn: device node + * @prefix: prefix for property name if any + * @index: index to start to parse + * @busno: Returns busno if supported. Otherwise pass NULL + * @addr: Returns address that DMA starts + * @size: Returns the range that DMA can handle + * + * This supports different formats flexibly. "prefix" can be + * configured if any. "busno" and "index" are optionally + * specified. Set 0(or NULL) if not used. + */ +static int of_get_dma_window(struct device_node *dn, + const char *prefix, int index, + unsigned long *busno, + dma_addr_t *addr, size_t *size) +{ + const __be32 *dma_window, *end; + int bytes, cur_index = 0; + char propname[NAME_MAX], addrname[NAME_MAX], sizename[NAME_MAX]; + const char *s = ""; + + if (!dn || !addr || !size) + return -EINVAL; + + if (prefix) + s = prefix; + snprintf(propname, sizeof(propname), "%sdma-window", s); + snprintf(addrname, sizeof(addrname), "%s#dma-address-cells", s); + snprintf(sizename, sizeof(sizename), "%s#dma-size-cells", s); + + dma_window = of_get_property(dn, propname, &bytes); + if (!dma_window) + return -ENODEV; + end = dma_window + bytes / sizeof(*dma_window); + + while (dma_window < end) { + u32 cells; + const void *prop; + + /* busno is one cell if supported */ + if (busno) + *busno = be32_to_cpup(dma_window++); + + prop = of_get_property(dn, addrname, NULL); + if (!prop) + prop = of_get_property(dn, "#address-cells", NULL); + + cells = prop ? be32_to_cpup(prop) : of_n_addr_cells(dn); + if (!cells) + return -EINVAL; + *addr = of_read_number(dma_window, cells); + dma_window += cells; + + prop = of_get_property(dn, sizename, NULL); + cells = prop ? be32_to_cpup(prop) : of_n_size_cells(dn); + if (!cells) + return -EINVAL; + *size = of_read_number(dma_window, cells); + dma_window += cells; + + if (cur_index++ == index) + break; + } + return 0; +} + static int tegra_smmu_suspend(struct device *dev) { struct smmu_device *smmu = dev_get_drvdata(dev);