Re: [PATCH 3/4] ppc64: Add driver for BPA iommu

[Arnd Bergmann <arnd@arndb.de>]

On Dunnersdag 28 April 2005 16:05, Olof Johansson wrote:

> On Thu, Apr 28, 2005 at 09:59:26AM +0200, Arnd Bergmann wrote:
> > +/* some constants */
> > +enum {
> > +	/* segment table entries */
> [...]
> > +};
> 
> Hmm. I thought the benefit of enum was to be able to do type checking
> later on if it's a typed enum. Here you mix different definitions in
> the same large untyped enum declaration. Can they be moved to a
> bpa_iommu.h file and #defined instead?

I prefer to avoid macros altogether, and this is one of the ways to
do it. We have had the discussion about how to define constants
a few times before on lkml without reaching a conclusion.

> > +/* cause link error for invalid use */
> > +extern unsigned long __ioc_invalid_page_size;
> [...]
> > +	default: /* not a known compile time constant */
> > +		ps = __ioc_invalid_page_size;
> > +		break;
> > +	}
> 
> Why do we need to detect this at link time?

I want to avoid doing BUG() or something similar, so I
try to detect a user error as early as possible.

> > +	nnpt++; /* XXX is this right? */
> 
> Well, does it work?  :-)

Yes, but it seems to contradict the specs...

> > +	return (ioste) {
> > +		.val = IOST_VALID_MASK
> > +			| (iostep & IOST_PT_BASE_MASK)
> > +			| ((nnpt << 5) & IOST_NNPT_MASK)
> > +			| (ps & IOST_PS_MASK)
> > +		};
> 
> Can you create a mk_ioste() inline instead of doing this construct?

ok.
 
> > +static inline unsigned long
> > +get_ioptep(ioste iost_entry, unsigned long io_address)
> > +{
> > +	unsigned long iopt_base;
> > +	unsigned long ps;
> > +	unsigned long iopt_offset;
> > +
> > +	iopt_base = iost_entry.val & IOST_PT_BASE_MASK;
> > +	ps        = iost_entry.val & IOST_PS_MASK;
> > +
> > +	iopt_offset = ((io_address & 0x0fffffff) >> (7 + 2 * ps)) & 0x7fff8ul;
> 
> Magic. Can we get it explained either by defines instead of constants
> or by a comment?

This comes from a graphical representation in the specs. I'll add a comment
to point to that image.

> > +static inline ioste
> > +get_iost_cache(void __iomem *base, unsigned long index)
> > +{
> > +	unsigned long __iomem *p = (base + IOC_ST_CACHE_DIR);
> > +	return (ioste) { in_be64(&p[index]) };
> 
> mk_ioste() would be nice here too.

ok.

> > +#ifdef __KERNEL__
> 
> Are we ever not __KERNEL__?

Sorry, I thought I had removed that code already. This does not belong
there.

> > +/* initialize the iommu to support a simple linear mapping */
> > +static void bpa_map_iommu(void)
> > +{
> [...]
> > +	for (address = 0; address < 0x100000000ul; address += io_page_size) {
> 
> This looks like way more than the 512MB DMA window you mentioned in the
> beginning. 

True. This is probably a bug in the comment. The code will change as soon
as the firmware provides the correct dma-window properties.

	Arnd <><