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

[olof@austin.ibm.com (Olof Johansson)]

Hi,

Some comments below.

On Thu, Apr 28, 2005 at 09:59:26AM +0200, Arnd Bergmann wrote:
> Index: linus-2.5/arch/ppc64/kernel/bpa_iommu.c
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linus-2.5/arch/ppc64/kernel/bpa_iommu.c	2005-04-22 07:01:39.000000000 +0200
> @@ -0,0 +1,433 @@

> +/* 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?

> +/* 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?

> +	nnpt++; /* XXX is this right? */

Well, does it work?  :-)

> +	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?

> +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?

> +/* compute the hashed 6 bit index for the 4-way associative pte cache */
> +static inline unsigned long
> +get_ioc_hash(ioste iost_entry, unsigned long io_address)
> +{
> +	unsigned long iopte = get_ioptep(iost_entry, io_address);
> +
> +	return ((iopte & 0x000000000000001f8ul) >> 3)
> +	     ^ ((iopte & 0x00000000000020000ul) >> 17)
> +	     ^ ((iopte & 0x00000000000010000ul) >> 15)
> +	     ^ ((iopte & 0x00000000000008000ul) >> 13)
> +	     ^ ((iopte & 0x00000000000004000ul) >> 11)
> +	     ^ ((iopte & 0x00000000000002000ul) >> 9)
> +	     ^ ((iopte & 0x00000000000001000ul) >> 7);

Can't you reverse the subword by just doing one XOR instead of 6?
That's what I did for the ext2 bitops on ppc64. See
http://www.ussg.iu.edu/hypermail/linux/kernel/0408.2/1321.html

> +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.

> +#ifdef __KERNEL__

Are we ever not __KERNEL__?

> +/* 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. 




-Olof