Re: [PATCH 4/6] ehea: header files

[Jan-Bernd Themann <ossthema@de.ibm.com>]

Michael Ellerman wrote:
> On Mon, 2006-08-14 at 14:53 +0200, Jan-Bernd Themann wrote:
> > Michael Ellerman wrote:
> > > > --- linux-2.6.18-rc4-orig/drivers/net/ehea/ehea.h	1969-12-31 16:00:00.000000000 -0800
> > > > +++ kernel/drivers/net/ehea/ehea.h	2006-08-08 23:59:39.927452928 -0700
> > > > +
> > > > +#define EHEA_PAGESHIFT  12
> > > > +#define EHEA_PAGESIZE   4096UL
> > > > +#define EHEA_CACHE_LINE 128
> > > This looks like a very bad idea, what happens if you're running on a
> > > machine with 64K pages?
> > The EHEA_PAGESIZE define is needed for queue management to hardware side.
>
> You mean the eHEA has its own concept of page size? Separate from the
> page size used by the MMU?

yes, the eHEA currently supports only 4K pages for queues

> > > > +/*
> > > > + *  h_galpa:
> > > > + *  for pSeries this is a 64bit memory address where
> > > > + *  I/O memory is mapped into CPU address space
> > > > + */
> > > > +
> > > > +struct h_galpa {
> > > > +	u64 fw_handle;
> > > > +};
> > > What is a h_galpa? And why does it need a struct if it's just a u64?
> > The eHEA chip is not PCI attached but directly connected to a proprietary
> > bus. Currently, we can access it by a simple 64 bit address, but this is not
> > true in all cases. Having a struct here allows us to encapsulate the chip
> > register access and to respond to changes to system hardware.
> >
> > We'll change the name to h_epa meaning "ehea physical address"
>
> Hmm, I'm not convinced. Having the struct doesn't really encapsulate
> much, because most of the places where you use the h_galpa struct just
> pull out the fw_handle anyway. So if you change the layout of the struct
> you're going to have to change most of the code anyway. And in the
> meantime it makes the code a lot less readable, most people understand
> what "u64 addr" is about, whereas "struct h_galpa" is much less
> meaningful. </2c>
>
> cheers

We already use the h_galpa struct for simulation purposes where we use
this encapsulation to add further required fields to be able to access
the registers (h_galpa has been renamed to h_epa). The name of the
fw_handle element will be changed. Instead of fw_handle, we'll call it
ebus_addr which will make it more readable.

Regards,
Jan-Bernd

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/