Re: [U-Boot] [PATCH v2] pci: Allow for PCI addresses to be 64-bit

On Oct 22, 2008, at 9:19 AM, Kumar Gala wrote:

PCI bus is inherently 64-bit. While not all system require access to the full 64-bit PCI address range some do. This allows those systems to enable the full PCI address width via CONFIG_SYS_PCI_64BIT.

Signed-off-by: Kumar Gala galak@kernel.crashing.org

Fixed up all the other bits associated with 64-bit PCI support.

So, this is looking a bit more complete now :) A few minor comments inline.

-B

• k

drivers/pci/pci.c | 37 ++++++++++++++++++-------- drivers/pci/pci_auto.c | 67 ++++++++++++++++++++++++++++ +------------------ include/pci.h | 40 +++++++++++++++++----------- 3 files changed, 91 insertions(+), 53 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 41780db..d13a57e 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c

------snip--------

@@ -319,10 +321,19 @@ int pci_hose_config_device(struct pci_controller *hose, io = io + bar_size; } else { if ((bar_response & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==

• 		PCI_BASE_ADDRESS_MEM_TYPE_64)
  

• 		found_mem64 = 1;
  

• 		PCI_BASE_ADDRESS_MEM_TYPE_64) {
  

• 		u32 bar_response_upper;
  

• 		u64 bar64;
  

•   				pci_hose_write_config_dword(hose, dev, bar+4, 0xffffffff);
  

• 		pci_hose_read_config_dword(hose, dev, bar+4,  
  

&bar_response_upper);

Fix alignment of code.

• 	bar_size = ~(bar_response & PCI_BASE_ADDRESS_MEM_MASK) + 1;
  

• 		bar64 = ((u64)bar_response_upper << 32) | bar_response;
  

• 		bar_size = ~(bar64 & PCI_BASE_ADDRESS_MEM_MASK) + 1;
  

• 		found_mem64 = 1;
  

• 	} else {
  

• 		bar_size = (u32)(~(bar_response & PCI_BASE_ADDRESS_MEM_MASK) +  
  

1);

• 	}
  
  /* round up region base address to multiple of size */
  mem = ((mem - 1) | (bar_size - 1)) + 1;
  

@@ -332,11 +343,15 @@ int pci_hose_config_device(struct pci_controller *hose, }

/* Write it out and update our limit */

• pci_hose_write_config_dword (hose, dev, bar, bar_value);
  

• pci_hose_write_config_dword (hose, dev, bar, (u32)bar_value);
  

if (found_mem64) {
	bar += 4;

+#ifdef CONFIG_SYS_PCI_64BIT

• 	pci_hose_write_config_dword(hose, dev, bar, bar_value>>32);
  

spaces around ">>" ? Might want to cast the result to (u32) for consistency.

+#else pci_hose_write_config_dword (hose, dev, bar, 0x00000000); +#endif } }

diff --git a/drivers/pci/pci_auto.c b/drivers/pci/pci_auto.c index 3844359..c347e82 100644 --- a/drivers/pci/pci_auto.c +++ b/drivers/pci/pci_auto.c @@ -45,14 +45,14 @@ void pciauto_region_init(struct pci_region* res) res->bus_lower = res->bus_start ? res->bus_start : 0x1000; }

-void pciauto_region_align(struct pci_region *res, unsigned long size) +void pciauto_region_align(struct pci_region *res, pci_size_t size) { res->bus_lower = ((res->bus_lower - 1) | (size - 1)) + 1; }

-int pciauto_region_allocate(struct pci_region* res, unsigned int size, unsigned int *bar) +int pciauto_region_allocate(struct pci_region* res, pci_size_t size, pci_addr_t *bar) {

• unsigned long addr;

• pci_addr_t addr;

  if (!res) { DEBUGF("No resource");

@@ -68,13 +68,13 @@ int pciauto_region_allocate(struct pci_region* res, unsigned int size, unsigned

res->bus_lower = addr + size;

• DEBUGF("address=0x%lx bus_lower=%x", addr, res->bus_lower);

• DEBUGF("address=0x%llx bus_lower=%llx", (u64)addr, (u64)res-

...

bus_lower);

While you're changing this, can we add the "0x" to the bus_lower print?

*bar = addr; return 0;

error:

• *bar = 0xffffffff;

• *bar = (pci_addr_t)-1; return -1;

}

@@ -88,7 +88,9 @@ void pciauto_setup_device(struct pci_controller *hose, struct pci_region *prefetch, struct pci_region *io) {

• unsigned int bar_value, bar_response, bar_size;

• unsigned int bar_response;
• pci_addr_t bar_value;
• pci_size_t bar_size; unsigned int cmdstat = 0; struct pci_region *bar_res; int bar, bar_nr = 0;

@@ -114,33 +116,46 @@ void pciauto_setup_device(struct pci_controller *hose, & 0xffff) + 1; bar_res = io;

• 	DEBUGF("PCI Autoconfig: BAR %d, I/O, size=0x%x, ", bar_nr,  
  

bar_size);

• 	DEBUGF("PCI Autoconfig: BAR %d, I/O, size=0x%llx, ", bar_nr,  
  

(u64)bar_size); } else { if ( (bar_response & PCI_BASE_ADDRESS_MEM_TYPE_MASK) ==

• 	     PCI_BASE_ADDRESS_MEM_TYPE_64)
  

• 		found_mem64 = 1;
  

• 	     PCI_BASE_ADDRESS_MEM_TYPE_64) {
  

• 		u32 bar_response_upper;
  

• 		u64 bar64;
  

•   				pci_hose_write_config_dword(hose, dev, bar+4, 0xffffffff);
  

• 		pci_hose_read_config_dword(hose, dev, bar+4,  
  

&bar_response_upper);

• 		bar64 = ((u64)bar_response_upper << 32) | bar_response;
  
  

• 	bar_size = ~(bar_response & PCI_BASE_ADDRESS_MEM_MASK) + 1;
  

• 		bar_size = ~(bar64 & PCI_BASE_ADDRESS_MEM_MASK) + 1;
  

• 		found_mem64 = 1;
  

• 	} else {
  

• 		bar_size = (u32)(~(bar_response & PCI_BASE_ADDRESS_MEM_MASK) +  
  

1);

• 	}
  if (prefetch && (bar_response & PCI_BASE_ADDRESS_MEM_PREFETCH))
  	bar_res = prefetch;
  else
  	bar_res = mem;
  
  

• 	DEBUGF("PCI Autoconfig: BAR %d, Mem, size=0x%x, ", bar_nr,  
  

bar_size);

• 	DEBUGF("PCI Autoconfig: BAR %d, Mem, size=0x%llx, ", bar_nr,  
  

(u64)bar_size); }

if (pciauto_region_allocate(bar_res, bar_size, &bar_value) == 0) {
	/* Write it out and update our limit */

• 	pci_hose_write_config_dword(hose, dev, bar, bar_value);
  

• 	pci_hose_write_config_dword(hose, dev, bar, (u32)bar_value);
  
  

• 	/*
  

• 	 * If we are a 64-bit decoder then increment to the
  

• 	 * upper 32 bits of the bar and force it to locate
  

• 	 * in the lower 4GB of memory.
  

• 	 */
  if (found_mem64) {
  	bar += 4;
  

+#ifdef CONFIG_SYS_PCI_64BIT

• 		pci_hose_write_config_dword(hose, dev, bar, bar_value>>32);
  

same as earlier.....

+#else

• 		/*
  

• 		 * If we are a 64-bit decoder then increment to the
  

• 		 * upper 32 bits of the bar and force it to locate
  

• 		 * in the lower 4GB of memory.
  

• 		 */
  	pci_hose_write_config_dword(hose, dev, bar, 0x00000000);
  

+#endif }

	cmdstat |= (bar_response & PCI_BASE_ADDRESS_SPACE) ?

@@ -289,10 +304,10 @@ void pciauto_config_init(struct pci_controller *hose) if (hose->pci_mem) { pciauto_region_init(hose->pci_mem);

• DEBUGF("PCI Autoconfig: Bus Memory region: [%lx-%lx],\n"
  

• DEBUGF("PCI Autoconfig: Bus Memory region: [%llx-%llx],\n"
    "\t\tPhysical Memory [%x-%x]\n",
  

•     hose->pci_mem->bus_start,
  

•     hose->pci_mem->bus_start + hose->pci_mem->size - 1,
  

•     (u64)hose->pci_mem->bus_start,
  

•     (u64)(hose->pci_mem->bus_start + hose->pci_mem->size - 1),
  

  hose->pci_mem->phys_start, hose->pci_mem->phys_start + hose->pci_mem->size - 1); }

Don't the phys_start and size need to print as 64b? The region elements are defined as phys_addr_t and pci_size_t now. Also would prefer to see a "0x" prefix on those hex prints, since you're changing this anyway.

@@ -300,10 +315,10 @@ void pciauto_config_init(struct pci_controller *hose) if (hose->pci_prefetch) { pciauto_region_init(hose->pci_prefetch);

• DEBUGF("PCI Autoconfig: Bus Prefetchable Mem: [%lx-%lx],\n"
  

• DEBUGF("PCI Autoconfig: Bus Prefetchable Mem: [%llx-%llx],\n"
    "\t\tPhysical Memory [%x-%x]\n",
  

•     hose->pci_prefetch->bus_start,
  

•     hose->pci_prefetch->bus_start + hose->pci_prefetch->size - 1,
  

•     (u64)hose->pci_prefetch->bus_start,
  

•     (u64)(hose->pci_prefetch->bus_start + hose->pci_prefetch- 
  

...

size - 1),

    hose->pci_prefetch->phys_start,
    hose->pci_prefetch->phys_start +
		hose->pci_prefetch->size - 1);

64b prints of phys_start and size? "0x" prefix?

@@ -312,10 +327,10 @@ void pciauto_config_init(struct pci_controller *hose) if (hose->pci_io) { pciauto_region_init(hose->pci_io);

• DEBUGF("PCI Autoconfig: Bus I/O region: [%lx-%lx],\n"
  

• DEBUGF("PCI Autoconfig: Bus I/O region: [%llx-%llx],\n"
    "\t\tPhysical Memory: [%x-%x]\n",
  

•     hose->pci_io->bus_start,
  

•     hose->pci_io->bus_start + hose->pci_io->size - 1,
  

•     (u64)hose->pci_io->bus_start,
  

•     (u64)(hose->pci_io->bus_start + hose->pci_io->size - 1),
  

  hose->pci_io->phys_start, hose->pci_io->phys_start + hose->pci_io->size - 1);

phys_start/size should print as 64b? "0x" prefix?

diff --git a/include/pci.h b/include/pci.h index 1c8e216..eebe8a8 100644 --- a/include/pci.h +++ b/include/pci.h @@ -101,8 +101,8 @@ #define PCI_BASE_ADDRESS_MEM_TYPE_1M 0x02 /* Below 1M [obsolete] */ #define PCI_BASE_ADDRESS_MEM_TYPE_64 0x04 /* 64 bit address */ #define PCI_BASE_ADDRESS_MEM_PREFETCH 0x08 /* prefetchable? */ -#define PCI_BASE_ADDRESS_MEM_MASK (~0x0fUL) -#define PCI_BASE_ADDRESS_IO_MASK (~0x03UL) +#define PCI_BASE_ADDRESS_MEM_MASK (~0x0fULL) +#define PCI_BASE_ADDRESS_IO_MASK (~0x03ULL) /* bit 1 is reserved if address_space = 1 */

/* Header type 0 (normal devices) */ @@ -111,7 +111,7 @@ #define PCI_SUBSYSTEM_ID 0x2e #define PCI_ROM_ADDRESS 0x30 /* Bits 31..11 are address, 10..1 reserved */ #define PCI_ROM_ADDRESS_ENABLE 0x01 -#define PCI_ROM_ADDRESS_MASK (~0x7ffUL) +#define PCI_ROM_ADDRESS_MASK (~0x7ffULL)

#define PCI_CAPABILITY_LIST 0x34 /* Offset of first capability list entry */

@@ -312,13 +312,21 @@

#include <pci_ids.h>

+#ifdef CONFIG_SYS_PCI_64BIT +typedef u64 pci_addr_t; +typedef u64 pci_size_t; +#else +typedef u32 pci_addr_t; +typedef u32 pci_size_t; +#endif

struct pci_region {

• unsigned long bus_start; /* Start on the bus */
• phys_addr_t phys_start; /* Start in physical address space */
• unsigned long size; /* Size */
• unsigned long flags; /* Resource flags */

• pci_addr_t bus_start; /* Start on the bus */
• phys_addr_t phys_start; /* Start in physical address space */
• pci_size_t size; /* Size */
• unsigned long flags; /* Resource flags */

• unsigned long bus_lower;

• pci_addr_t bus_lower;

};

#define PCI_REGION_MEM 0x00000000 /* PCI memory space */ @@ -330,9 +338,9 @@ struct pci_region { #define PCI_REGION_RO 0x00000200 /* Read-only memory */

extern __inline__ void pci_set_region(struct pci_region *reg,

• 		      unsigned long bus_start,
  

• 		      pci_addr_t bus_start,
  	      phys_addr_t phys_start,
  

• 		      unsigned long size,
  

• 		      pci_size_t size,
  	      unsigned long flags) {
  

  reg->bus_start = bus_start; reg->phys_start = phys_start;

@@ -433,9 +441,9 @@ extern __inline__ void pci_set_ops(struct pci_controller *hose, extern void pci_setup_indirect(struct pci_controller* hose, u32 cfg_addr, u32 cfg_data);

extern phys_addr_t pci_hose_bus_to_phys(struct pci_controller* hose,

• 			unsigned long addr, unsigned long flags);
  

-extern unsigned long pci_hose_phys_to_bus(struct pci_controller* hose,

• 			  phys_addr_t addr, unsigned long flags);
  

• 			pci_addr_t addr, unsigned long flags);
  

check whitespace here

+extern pci_addr_t pci_hose_phys_to_bus(struct pci_controller* hose,

• 			phys_addr_t addr, unsigned long flags);
  
  

#define pci_phys_to_bus(dev, addr, flags) \ pci_hose_phys_to_bus(pci_bus_to_hose(PCI_BUS(dev)), (addr), (flags)) @@ -483,8 +491,8 @@ extern int pci_hose_scan(struct pci_controller *hose); extern int pci_hose_scan_bus(struct pci_controller *hose, int bus);

extern void pciauto_region_init(struct pci_region* res); -extern void pciauto_region_align(struct pci_region *res, unsigned long size); -extern int pciauto_region_allocate(struct pci_region* res, unsigned int size, unsigned int *bar); +extern void pciauto_region_align(struct pci_region *res, pci_size_t size); +extern int pciauto_region_allocate(struct pci_region* res, pci_size_t size, pci_addr_t *bar); extern void pciauto_setup_device(struct pci_controller *hose, pci_dev_t dev, int bars_num, struct pci_region *mem, @@ -500,7 +508,7 @@ extern pci_dev_t pci_find_class(int wanted_class, int wanted_sub_code, extern int pci_hose_config_device(struct pci_controller *hose, pci_dev_t dev, unsigned long io,

• 		  unsigned long mem,
  

• 		  pci_addr_t mem,
  	  unsigned long command);
  
  

#ifdef CONFIG_MPC824X

1.5.5.1