PDC cleanup and encapsulation
Subsystems (2)
PATCH 1/1 PDC cleanup and encapsulation
Anyone see a problem with committing this patch?
- Ryan
----- Forwarded message from Ryan Bradetich <[email protected]> -----
Envelope-to: [email protected]
From: "Ryan Bradetich" <[email protected]>
To: [email protected]
Reply-To: [email protected]
X-UIDL: 985854046.7659.154121.boispop1.bois.uswest.net
Subject: [parisc-linux] [PATCH] PDC cleanup and encapsulation
X-Mailman-Version: 1.1
List-Id: parisc-linux developers list <parisc-linux.lists.parisc-linux.org>
X-BeenThere: [email protected]
Hello parisc-linux hackers,
This is a fairly extensive patch which (almost) finishes the PDC cleanups I have been working on. The goal of this patch was to add kernel-docs and encapsulate the firmware calls so the caller does not have to worry about alignment issues, etc.
The last PDC/firware cleanup I have on my list is to rename asm/pdc.h to firmware.h. This will be a seperate patch after this patch has been reviewed/approved/committed.
I have tested this patch on the following machines:
- 712/80 using STI console
- C200 32-bit kernel using serial console
- A500 64-bit kernel using serial console
Since the majority of the changes are in firmware.c and pdc.h I have included include those files in their entirity to make them easier to read.
As always, feedback, concerns, and questions are welcome.
Thanks,
- Ryan
[patch snipped]
Origional message can be found at:
http://lists.parisc-linux.org/pipermail/parisc-linux/2001-March/012108.html
Looks good to me. A couple minor questions/comments, more for my edification than anything else:
- pdc.h contains a commented-out struct pdc_iodc. The comment suggests possible issues with using it with 64-bit firmware, but I think the fear is unfounded. There are several places that use pdc_iodc_read(), but they all seem to define their own equivalent of struct pdc_iodc. I'm not sure why they don't use struct pdc_iodc. Also, include/asm-parisc/hardware.h contains #defines for the IODC_TYPE field (HPHW_NPROC, etc); should these be defined alongside struct pdc_iodc?
- What are the guidelines for #ifdef __LP64__ vs CONFIG_PA20? You replaced __LP64__ with CONFIG_PA20 in some places but not others and I'm not clear on the difference.
- In firmware.c, the "yes 'int', not 'long' -- IODC I/O is always 32-bit stuff" comment moved from pdc.h seems obsolete, or at least, I can't figure out what the 'int' refers to. I'd remove the "This means Cxxx boxes can't run wide kernels right now." comment, since CONFIG_PDC_NARROW addresses that issue.
I attached a diff that fixes a few typos.
Bjorn
arch/parisc/kernel/firmware.c +5 -5
@@ -12,7 +12,7 @@* - the name of the pdc wrapper should match one of the macros * used for the first two arguments * - don't use caps for random parts of the name -* - use ASSERT_ALIGN to ensure the aligment of the arguments is +* - use ASSERT_ALIGN to ensure the alignment of the arguments is * correct * - use __pa() to convert virtual (kernel) pointers to physical * ones.
@@ -106,7 +106,7 @@* convert_to_wide - Convert the return buffer addresses into kernel addresses. * @address: The return buffer from PDC. * -* This fucntion is used to convert the return buffer addresses retrieve from PDC +* This function is used to convert the return buffer addresses retrieved from PDC * into kernel addresses when the PDC address size and kernel address size are * different. */
@@ -121,7 +121,7 @@ }/** -* pdc_add_valid - Verify address can be accessesd without causing a HPMC. +* pdc_add_valid - Verify address can be accessed without causing a HPMC. * @address: Address to be verified. * * This PDC call attempts to read from the specified address and verify
@@ -340,7 +340,7 @@int retval;spin_lock_irq (&pdc_lock); - pdc_result[0 ] =0 ;/* preset zero (call may not be implimented!) */ + pdc_result[0 ] =0 ;/* preset zero (call may not be implemented!) */ retval =mem_pdc_call (PDC_MODEL ,PDC_MODEL_CPU_ID ,__pa (pdc_result),0 );convert_to_wide (pdc_result); *cpu_id = pdc_result[0 ];
@@ -730,7 +730,7 @@* @r_addr: The return buffer. * @cell_num: The target cell. * -* This PDC functin returns the actual interrupt table for the specified cell. +* This PDC function returns the actual interrupt table for the specified cell. */ int pdc_pat_get_irt (void *r_addr,unsigned long cell_num) {
drivers/net/lasi_82596.c +1 -1
@@ -1172,7 +1172,7 @@if (pdc_lan_station_id ( (char *)ð_addr, (void *)dev->base_addr)) {for (i=0 ;i<6 ;i++) eth_addr[i] =gsc_readb (LAN_PROM_ADDR +i); -printk ("82596.c: MAC of HP700 LAN blindely read from the prom!\n" ); +printk ("82596.c: MAC of HP700 LAN blindly read from the prom!\n" ); } dev->mem_start = (int)pci_alloc_consistent( NULL,
include/asm-parisc/pdc.h +10 -10
@@ -12,7 +12,7 @@#define PDC_CHASSIS_DISP 0/* update chassis display */ #define PDC_CHASSIS_WARN 1/* return chassis warnings */ #define PDC_CHASSIS_DISPWARN 2/* update&return chassis status */ -#define PDC_RETURN_CHASSIS_INFO 128/* HVERSION dependend: return chassis LED/LCD info */ +#define PDC_RETURN_CHASSIS_INFO 128/* HVERSION dependent: return chassis LED/LCD info */ #define PDC_PIM 3/* Get PIM data */ #define PDC_PIM_HPMC 0/* Transfer HPMC data */
@@ -126,8 +126,8 @@#define PDC_REQ_ERR_1 2/* See above */ #define PDC_REQ_ERR_0 1/* Call would generate a requestor error */ #define PDC_OK 0/* Call completed successfully */ -#define PDC_BAD_PROC -1/* Called non-existant procedure */ -#define PDC_BAD_OPTION -2/* Called with non-existant option */ +#define PDC_BAD_PROC -1/* Called non-existent procedure */ +#define PDC_BAD_OPTION -2/* Called with non-existent option */ #define PDC_ERROR -3/* Call could not complete without an error */ #define PDC_INVALID_ARG -10/* Called with an invalid argument */ #define PDC_BUS_POW_WARN -12/* Call could not complete in allowed power budget */
@@ -138,7 +138,7 @@#define PDC_RET_OK 0L/* Call completed successfully */ #define PDC_RET_NE_PROC -1L/* Non-existent procedure */ -#define PDC_RET_NE_OPT -2L/* non-existant option - arg1 */ +#define PDC_RET_NE_OPT -2L/* non-existent option - arg1 */ #define PDC_RET_NE_MOD -5L/* Module not found */ #define PDC_RET_NE_CELL_MOD -7L/* Cell module not found */ #define PDC_RET_INV_ARG -10L/* Invalid argument */
@@ -151,7 +151,7 @@#define PDC_ADD_VALID_REQ_ERR_1 2/* See above */ #define PDC_ADD_VALID_REQ_ERR_0 1/* Call would generate a requestor error */ #define PDC_ADD_VALID_OK 0/* Call completed successfully */ -#define PDC_ADD_VALID_BAD_OPTION -2/* Called with non-existant option */ +#define PDC_ADD_VALID_BAD_OPTION -2/* Called with non-existent option */ #define PDC_ADD_VALID_ERROR -3/* Call could not complete without an error */ #define PDC_ADD_VALID_INVALID_ARG -10/* Called with an invalid argument */ #define PDC_ADD_VALID_BUS_POW_WARN -12/* Call could not complete in allowed power budget */
@@ -181,7 +181,7 @@#ifdef __LP64__/* PDC PAT CELL */ #define PDC_PAT_CELL 64L/* Interface for gaining and -* manipulatin g cell state within PD */ +* manipulating cell state within PD */ #define PDC_PAT_CELL_GET_NUMBER 0L/* Return Cell number */ #define PDC_PAT_CELL_GET_INFO 1L/* Returns info about Cell */ #define PDC_PAT_CELL_MODULE 2L/* Returns info about Module */
@@ -195,7 +195,7 @@#define PDC_PAT_CELL_GET_REMOTE_PDH_SZ 16L/* Return Remote PDH Buffer Size */ #define PDC_PAT_CELL_GET_REMOTE_PDH 17L/* Read Remote PDH Buffer */ #define PDC_PAT_CELL_GET_DBG_INFO 128L/* Return DBG Buffer Info */ -#define PDC_PAT_CELL_CHANGE_ALIAS 129L/* Change Non-Equivalent Alias Chacking */ +#define PDC_PAT_CELL_CHANGE_ALIAS 129L/* Change Non-Equivalent Alias Checking */ /* ** Arg to PDC_PAT_CELL_MODULE memaddr[4]
@@ -222,7 +222,7 @@#define PAT_GMMIO 2/* > 4G MMIO Space */ #define PAT_NPIOP 3/* Non Postable I/O Port Space */ #define PAT_PIOP 4/* Postable I/O Port Space */ -#define PAT_AHPA 5/* Addional HPA Space */ +#define PAT_AHPA 5/* Additional HPA Space */ #define PAT_UFO 6/* HPA Space (UFO for Mariposa) */ #define PAT_GNIP 7/* GNI Reserved Space */
@@ -266,7 +266,7 @@** Monarch Processor */ #define PDC_PAT_HPMC_RENDEZ_CPU 0L/* go into spin loop */ #define PDC_PAT_HPMC_SET_PARAMS 1L/* Allows OS to specify intr which PDC -* will use to interupt OS during machine +* will use to interrupt OS during machine * check rendezvous */ /* parameters for PDC_PAT_HPMC_SET_PARAMS: */
@@ -400,7 +400,7 @@#ifdef __LP64__ cc_padW:32,#endif - cc_alias:4, /* alias boundaries for virtual adresses */ + cc_alias:4, /* alias boundaries for virtual addresses */ cc_block: 4, /* to determine most efficient stride */ cc_line : 3, /* maximum amount written back as a result of store (multiple of 16 bytes) */ cc_pad0 : 2, /* reserved */
Quoted 3 lines · Bjorn Helgaas
- What are the guidelines for #ifdef
__LP64__vsCONFIG_PA20? You replaced__LP64__withCONFIG_PA20in some places but not others and I'm not clear on the difference.
Not all PA-2.0 processors are run in 64-bit mode. For example, a C3000 can be run in either 32 or 64 bit mode. At compile-time you can choose to optimise for a particular processor (and risk it not working on other machines).
Quoted 5 lines
- In
firmware.c, the "yes 'int', not 'long' -- IODC I/O is always 32-bit stuff" comment moved frompdc.hseems obsolete, or at least, I can't figure out what the 'int' refers to. I'd remove the "This means Cxxx boxes can't run wide kernels right now." comment, sinceCONFIG_PDC_NARROWaddresses that issue.
yup, some of the comments are definitely stale.
see linus' comments about disliking comments because of this issue :-)
Signature · 1 line
Revolutions do not require corporate support.
Quoted 9 lines · Matthew Wilcox
Quoted 3 lines · Bjorn Helgaas
- What are the guidelines for #ifdef
__LP64__vsCONFIG_PA20? You replaced__LP64__withCONFIG_PA20in some places but not others and I'm not clear on the difference.Not all PA-2.0 processors are run in 64-bit mode. For example, a C3000 can be run in either 32 or 64 bit mode. At compile-time you can choose to optimise for a particular processor (and risk it not working on other machines).
Ok, so let me see if I have this right:
CONFIG_PA20 =>
PA2.0 architecture, in either narrow or wide mode. Defined iff CONFIG_PA8X00 selected, not directly user-configurable.
CONFIG_PARISC64 =>
Only possible when CONFIG_PA20 selected Build 64-bit kernel by using $(CROSS_COMPILE) = hppa64-linux. cpp predefines __LP64__.
So is there a reason for testing __LP64__ rather than CONFIG_PARISC64 in the code? There are a zillion tests for the former and none for the latter, even though the latter seems more direct and obvious.
It seems to me that __LP64__ is overloaded for several things. Fundamentally, it is a property of the kernel, i.e., it is compiled to run in wide mode. But we also use it to decide whether to use PDC PAT, which seems like a slightly different question. If you run a narrow kernel on a C3000, the firmware still supports PDC PAT (although I suppose we would have to do some work to call it in wide mode).
Quoted 3 lines · Bjorn Helgaas
CONFIG_PA20=>
PA2.0 architecture, in either narrow or wide mode. Defined iffCONFIG_PA8X00selected, not directly user-configurable.
Correct. I test this rather than CONFIG_PA8X00 because it's conceivable we might want to split PA8000/8200 support from 8500/8600. Or something.
Quoted 4 lines
CONFIG_PARISC64=>Only possible when CONFIG_PA20 selected Build 64-bit kernel by using $(CROSS_COMPILE) = hppa64-linux. cpp predefines __LP64__.
Right.
Quoted 3 lines
So is there a reason for testing
__LP64__rather thanCONFIG_PARISC64in the code? There are a zillion tests for the former and none for the latter, even though the latter seems more direct and obvious.
Well.. it requires that you remember to include <linux/config.h>.
Quoted 6 lines
It seems to me that
__LP64__is overloaded for several things. Fundamentally, it is a property of the kernel, i.e., it is compiled to run in wide mode. But we also use it to decide whether to use PDC PAT, which seems like a slightly different question. If you run a narrow kernel on a C3000, the firmware still supports PDC PAT (although I suppose we would have to do some work to call it in wide mode).
Not quite true, C3k has 64-bit PDC, but it is not PAT.
Signature · 1 line
Revolutions do not require corporate support.
Quoted 3 lines · Bjorn Helgaas
CONFIG_PARISC64=>Only possible when CONFIG_PA20 selected Build 64-bit kernel by using $(CROSS_COMPILE) = hppa64-linux.
rhirst and I don't modify linux/Makefile for CROSS_COMPILE. Now automagically based on the .config options selected.
Quoted 5 lines
cpp predefines __LP64__.So is there a reason for testing
__LP64__rather thanCONFIG_PARISC64in the code? There are a zillion tests for the former and none for the latter, even though the latter seems more direct and obvious.
CONFIG_PARISC64 wasn't available when I wrote my code.
Most of my code uses __LP64__ though I think including linux./config.h is better since when it changes, the .o's get rebuilt.
Quoted 5 lines
It seems to me that
__LP64__is overloaded for several things. Fundamentally, it is a property of the kernel, i.e., it is compiled to run in wide mode. But we also use it to decide whether to use PDC PAT, which seems like a slightly different question. If you run a narrow kernel on a C3000, the firmware still supports PDC PAT
I didn't think so. A narrow kernel won't have PDC PAT *calls*
(eg do_inventory) in it.
Quoted 1 line
(although I suppose we would have to do some work to call it in wide mode).
We considered doing this but decided it wasn't worth the effort. Others are welcome to make this work if they have time to though...
grant
Grant Grundler
parisc-linux {PCI|IOMMU|SMP} hacker
+1.408.447.7253
Quoted 10 lines · Bjorn Helgaas
Looks good to me. A couple minor questions/comments, more for my edification than anything else:
-
pdc.hcontains a commented-out structpdc_iodc. The comment suggests possible issues with using it with 64-bit firmware, but I think the fear is unfounded. There are several places that usepdc_iodc_read(), but they all seem to define their own equivalent of structpdc_iodc. I'm not sure why they don't use structpdc_iodc. Also,include/asm-parisc/hardware.hcontains #defines for theIODC_TYPEfield (HPHW_NPROC, etc); should these be defined alongside structpdc_iodc?