Re: [PATCH 1/5] iommu/tegra: smmu: Add DMA window parser, of_get_dma_window()

[Hiroshi Doyu <hdoyu@nvidia.com>]

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?