From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan-Bernd Themann Subject: Re: [PATCH 4/6] ehea: header files Date: Tue, 15 Aug 2006 11:44:05 +0200 Message-ID: <44E19765.7030301@de.ibm.com> References: <44D99F56.7010201@de.ibm.com> <1155190921.9801.43.camel@localhost.localdomain> <44E07267.7070007@de.ibm.com> <1155600499.9047.13.camel@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: netdev , Thomas Klein , linux-ppc , Christoph Raisch , linux-kernel , Marcus Eder Return-path: Received: from mtagate6.uk.ibm.com ([195.212.29.139]:40770 "EHLO mtagate6.uk.ibm.com") by vger.kernel.org with ESMTP id S965365AbWHOKXI (ORCPT ); Tue, 15 Aug 2006 06:23:08 -0400 To: michael@ellerman.id.au In-Reply-To: <1155600499.9047.13.camel@localhost.localdomain> Sender: netdev-owner@vger.kernel.org List-Id: netdev.vger.kernel.org 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. > > 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