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