Date:	Thu, 21 Jun 2012 08:46:11 +0200
From:	Hiroshi Doyu <hdoyu@...dia.com>
To:	"swarren@...dotorg.org" <swarren@...dotorg.org>,
	"joerg.roedel@....com" <joerg.roedel@....com>,
	"arnd@...db.de" <arnd@...db.de>
CC:	"iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
	"linux-tegra@...r.kernel.org" <linux-tegra@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"grant.likely@...retlab.ca" <grant.likely@...retlab.ca>,
	"rob.herring@...xeda.com" <rob.herring@...xeda.com>
Subject: Re: [PATCH 1/5] iommu/tegra: smmu: Add DMA window parser,
 of_get_dma_window()

Stephen Warren <swarren@...dotorg.org> wrote @ Wed, 20 Jun 2012 19:11:51 +0200:

> On 06/20/2012 01:16 AM, Hiroshi DOYU wrote:
> > From: Hiroshi Doyu <hdoyu@...dia.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@...dia.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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists