* [PATCH 1/6 v3] PCI: export some functions and macros
@ 2008-09-27 8:27 Zhao, Yu
2008-09-27 12:59 ` Matthew Wilcox
2008-10-01 16:52 ` Jesse Barnes
0 siblings, 2 replies; 4+ messages in thread
From: Zhao, Yu @ 2008-09-27 8:27 UTC (permalink / raw)
To: linux-pci
Cc: Jesse Barnes, Randy Dunlap, Grant Grundler, Alex Chiang,
Matthew Wilcox, Roland Dreier, Greg KH, linux-kernel, kvm,
virtualization
Export some functions and move some macros from c file to header file.
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Cc: Randy Dunlap <randy.dunlap@oracle.com>
Cc: Grant Grundler <grundler@parisc-linux.org>
Cc: Alex Chiang <achiang@hp.com>
Cc: Matthew Wilcox <matthew@wil.cx>
Cc: Roland Dreier <rdreier@cisco.com>
Cc: Greg KH <greg@kroah.com>
Signed-off-by: Yu Zhao <yu.zhao@intel.com>
---
drivers/pci/pci-sysfs.c | 10 ++++----
drivers/pci/pci.c | 2 +-
drivers/pci/pci.h | 20 ++++++++++++++++++
drivers/pci/probe.c | 50 +++++++++++++++++++++-------------------------
drivers/pci/setup-bus.c | 4 +-
drivers/pci/setup-res.c | 2 +-
include/linux/pci.h | 4 +-
7 files changed, 54 insertions(+), 38 deletions(-)
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 9c71858..f99160d 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -696,7 +696,7 @@ static struct bin_attribute pci_config_attr = {
.name = "config",
.mode = S_IRUGO | S_IWUSR,
},
- .size = 256,
+ .size = PCI_CFG_SPACE_SIZE,
.read = pci_read_config,
.write = pci_write_config,
};
@@ -706,7 +706,7 @@ static struct bin_attribute pcie_config_attr = {
.name = "config",
.mode = S_IRUGO | S_IWUSR,
},
- .size = 4096,
+ .size = PCI_CFG_SPACE_EXP_SIZE,
.read = pci_read_config,
.write = pci_write_config,
};
@@ -724,7 +724,7 @@ int __must_check pci_create_sysfs_dev_files (struct pci_dev *pdev)
if (!sysfs_initialized)
return -EACCES;
- if (pdev->cfg_size < 4096)
+ if (pdev->cfg_size < PCI_CFG_SPACE_EXP_SIZE)
retval = sysfs_create_bin_file(&pdev->dev.kobj, &pci_config_attr);
else
retval = sysfs_create_bin_file(&pdev->dev.kobj, &pcie_config_attr);
@@ -795,7 +795,7 @@ err_vpd:
kfree(pdev->vpd->attr);
}
err_config_file:
- if (pdev->cfg_size < 4096)
+ if (pdev->cfg_size < PCI_CFG_SPACE_EXP_SIZE)
sysfs_remove_bin_file(&pdev->dev.kobj, &pci_config_attr);
else
sysfs_remove_bin_file(&pdev->dev.kobj, &pcie_config_attr);
@@ -820,7 +820,7 @@ void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
sysfs_remove_bin_file(&pdev->dev.kobj, pdev->vpd->attr);
kfree(pdev->vpd->attr);
}
- if (pdev->cfg_size < 4096)
+ if (pdev->cfg_size < PCI_CFG_SPACE_EXP_SIZE)
sysfs_remove_bin_file(&pdev->dev.kobj, &pci_config_attr);
else
sysfs_remove_bin_file(&pdev->dev.kobj, &pcie_config_attr);
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c9884bb..259eaff 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -216,7 +216,7 @@ int pci_find_ext_capability(struct pci_dev *dev, int cap)
int ttl = 480; /* 3840 bytes, minimum 8 bytes per capability */
int pos = 0x100;
- if (dev->cfg_size <= 256)
+ if (dev->cfg_size <= PCI_CFG_SPACE_SIZE)
return 0;
if (pci_read_config_dword(dev, pos, &header) != PCIBIOS_SUCCESSFUL)
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index d807cd7..596efa6 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -1,3 +1,9 @@
+#ifndef DRIVERS_PCI_H
+#define DRIVERS_PCI_H
+
+#define PCI_CFG_SPACE_SIZE 256
+#define PCI_CFG_SPACE_EXP_SIZE 4096
+
/* Functions internal to the PCI core code */
extern int pci_uevent(struct device *dev, struct kobj_uevent_env *env);
@@ -144,3 +150,17 @@ struct pci_slot_attribute {
};
#define to_pci_slot_attr(s) container_of(s, struct pci_slot_attribute, attr)
+enum pci_bar_type {
+ pci_bar_unknown, /* Standard PCI BAR probe */
+ pci_bar_io, /* An io port BAR */
+ pci_bar_mem32, /* A 32-bit memory BAR */
+ pci_bar_mem64, /* A 64-bit memory BAR */
+ pci_bar_rom, /* A ROM BAR */
+};
+
+extern int pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
+ struct resource *res, unsigned int reg);
+extern struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
+ struct pci_dev *bridge, int busnr);
+
+#endif /* DRIVERS_PCI_H */
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 36698e5..7cdb834 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -14,8 +14,6 @@
#define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */
#define CARDBUS_RESERVE_BUSNR 3
-#define PCI_CFG_SPACE_SIZE 256
-#define PCI_CFG_SPACE_EXP_SIZE 4096
/* Ugh. Need to stop exporting this to modules. */
LIST_HEAD(pci_root_buses);
@@ -203,13 +201,6 @@ static u64 pci_size(u64 base, u64 maxbase, u64 mask)
return size;
}
-enum pci_bar_type {
- pci_bar_unknown, /* Standard PCI BAR probe */
- pci_bar_io, /* An io port BAR */
- pci_bar_mem32, /* A 32-bit memory BAR */
- pci_bar_mem64, /* A 64-bit memory BAR */
-};
-
static inline enum pci_bar_type decode_bar(struct resource *res, u32 bar)
{
if ((bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) {
@@ -224,16 +215,21 @@ static inline enum pci_bar_type decode_bar(struct resource *res, u32 bar)
return pci_bar_mem32;
}
-/*
- * If the type is not unknown, we assume that the lowest bit is 'enable'.
- * Returns 1 if the BAR was 64-bit and 0 if it was 32-bit.
+/**
+ * pci_read_base - read a PCI BAR
+ * @dev: the PCI device
+ * @type: type of the BAR
+ * @res: resource buffer to be filled in
+ * @pos: BAR position in the config space
+ *
+ * Returns 1 if the BAR is 64-bit, or 0 if 32-bit.
*/
-static int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
+int pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
struct resource *res, unsigned int pos)
{
u32 l, sz, mask;
- mask = type ? ~PCI_ROM_ADDRESS_ENABLE : ~0;
+ mask = (type == pci_bar_rom) ? ~PCI_ROM_ADDRESS_ENABLE : ~0;
res->name = pci_name(dev);
@@ -258,7 +254,11 @@ static int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
if (l == 0xffffffff)
l = 0;
- if (type == pci_bar_unknown) {
+ if (type == pci_bar_rom) {
+ res->flags |= (l & IORESOURCE_ROM_ENABLE);
+ l &= PCI_ROM_ADDRESS_MASK;
+ mask = (u32)PCI_ROM_ADDRESS_MASK;
+ } else {
type = decode_bar(res, l);
res->flags |= pci_calc_resource_flags(l) | IORESOURCE_SIZEALIGN;
if (type == pci_bar_io) {
@@ -268,10 +268,6 @@ static int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
l &= PCI_BASE_ADDRESS_MEM_MASK;
mask = (u32)PCI_BASE_ADDRESS_MEM_MASK;
}
- } else {
- res->flags |= (l & IORESOURCE_ROM_ENABLE);
- l &= PCI_ROM_ADDRESS_MASK;
- mask = (u32)PCI_ROM_ADDRESS_MASK;
}
if (type == pci_bar_mem64) {
@@ -335,7 +331,7 @@ static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom)
for (pos = 0; pos < howmany; pos++) {
struct resource *res = &dev->resource[pos];
reg = PCI_BASE_ADDRESS_0 + (pos << 2);
- pos += __pci_read_base(dev, pci_bar_unknown, res, reg);
+ pos += pci_read_base(dev, pci_bar_unknown, res, reg);
}
if (rom) {
@@ -344,7 +340,7 @@ static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom)
res->flags = IORESOURCE_MEM | IORESOURCE_PREFETCH |
IORESOURCE_READONLY | IORESOURCE_CACHEABLE |
IORESOURCE_SIZEALIGN;
- __pci_read_base(dev, pci_bar_mem32, res, rom);
+ pci_read_base(dev, pci_bar_mem32, res, rom);
}
}
@@ -366,9 +362,6 @@ void __devinit pci_read_bridge_bases(struct pci_bus *child)
child->resource[i] = child->parent->resource[i - 3];
}
- for(i=0; i<3; i++)
- child->resource[i] = &dev->resource[PCI_BRIDGE_RESOURCES+i];
-
res = child->resource[0];
pci_read_config_byte(dev, PCI_IO_BASE, &io_base_lo);
pci_read_config_byte(dev, PCI_IO_LIMIT, &io_limit_lo);
@@ -461,7 +454,7 @@ static struct pci_bus * pci_alloc_bus(void)
return b;
}
-static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
+struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
struct pci_dev *bridge, int busnr)
{
struct pci_bus *child;
@@ -474,12 +467,10 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
if (!child)
return NULL;
- child->self = bridge;
child->parent = parent;
child->ops = parent->ops;
child->sysdata = parent->sysdata;
child->bus_flags = parent->bus_flags;
- child->bridge = get_device(&bridge->dev);
/* initialize some portions of the bus device, but don't register it
* now as the parent is not properly set up yet. This device will get
@@ -496,6 +487,11 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
child->primary = parent->secondary;
child->subordinate = 0xff;
+ if (!bridge)
+ return child;
+
+ child->self = bridge;
+ child->bridge = get_device(&bridge->dev);
/* Set up default resource pointers and names.. */
for (i = 0; i < 4; i++) {
child->resource[i] = &bridge->resource[PCI_BRIDGE_RESOURCES+i];
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
index 3abbfad..6c78cf8 100644
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -299,7 +299,7 @@ static void pbus_size_io(struct pci_bus *bus)
if (r->parent || !(r->flags & IORESOURCE_IO))
continue;
- r_size = r->end - r->start + 1;
+ r_size = resource_size(r);
if (r_size < 0x400)
/* Might be re-aligned for ISA */
@@ -350,7 +350,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, unsigned long
if (r->parent || (r->flags & mask) != type)
continue;
- r_size = r->end - r->start + 1;
+ r_size = resource_size(r);
/* For bridges size != alignment */
align = resource_alignment(r);
order = __ffs(align) - 20;
diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
index 1a5fc83..56e4042 100644
--- a/drivers/pci/setup-res.c
+++ b/drivers/pci/setup-res.c
@@ -133,7 +133,7 @@ int pci_assign_resource(struct pci_dev *dev, int resno)
resource_size_t size, min, align;
int ret;
- size = res->end - res->start + 1;
+ size = resource_size(res);
min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM;
align = resource_alignment(res);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 98dc624..cc78be6 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -456,8 +456,8 @@ struct pci_driver {
/**
* PCI_VDEVICE - macro used to describe a specific pci device in short form
- * @vend: the vendor name
- * @dev: the 16 bit PCI Device ID
+ * @vendor: the vendor name
+ * @device: the 16 bit PCI Device ID
*
* This macro is used to create a struct pci_device_id that matches a
* specific PCI device. The subvendor, and subdevice fields will be set
--
1.5.6.4
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/6 v3] PCI: export some functions and macros
2008-09-27 8:27 [PATCH 1/6 v3] PCI: export some functions and macros Zhao, Yu
@ 2008-09-27 12:59 ` Matthew Wilcox
2008-10-08 2:23 ` Zhao, Yu
2008-10-01 16:52 ` Jesse Barnes
1 sibling, 1 reply; 4+ messages in thread
From: Matthew Wilcox @ 2008-09-27 12:59 UTC (permalink / raw)
To: Zhao, Yu
Cc: linux-pci, Jesse Barnes, Randy Dunlap, Grant Grundler,
Alex Chiang, Roland Dreier, Greg KH, linux-kernel, kvm,
virtualization
On Sat, Sep 27, 2008 at 04:27:44PM +0800, Zhao, Yu wrote:
> Export some functions and move some macros from c file to header file.
That's absolutely not everything this patch does. You need to split
this into smaller pieces and explain what you're doing and why for each
of them.
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index d807cd7..596efa6 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -1,3 +1,9 @@
> +#ifndef DRIVERS_PCI_H
> +#define DRIVERS_PCI_H
Do we really need header guards on this file?
> -/*
> - * If the type is not unknown, we assume that the lowest bit is 'enable'.
> - * Returns 1 if the BAR was 64-bit and 0 if it was 32-bit.
> +/**
> + * pci_read_base - read a PCI BAR
> + * @dev: the PCI device
> + * @type: type of the BAR
> + * @res: resource buffer to be filled in
> + * @pos: BAR position in the config space
> + *
> + * Returns 1 if the BAR is 64-bit, or 0 if 32-bit.
> */
> -static int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
> +int pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
The original intent here was to have a pci_read_base() that called
__pci_read_base() and then did things like translate physical BAR
addresses to virtual ones. That patch is in the archives somewhere.
We ended up not including that patch because my user found out he could
get the address he wanted from elsewhere. I'm not sure we want to
remove the __ at this point.
The eventual goal is to fix up the BARs at this point, but there's
several architectures that will break if we do this now. It's on my
long-term todo list.
> struct resource *res, unsigned int pos)
> {
> u32 l, sz, mask;
>
> - mask = type ? ~PCI_ROM_ADDRESS_ENABLE : ~0;
> + mask = (type == pci_bar_rom) ? ~PCI_ROM_ADDRESS_ENABLE : ~0;
What's going on here? Why are you adding pci_bar_rom? For the rom we
use pci_bar_mem32. Take a look at, for example, the MCHBAR in the 965
spec (313053.pdf). That's something that uses the pci_bar_mem64 type
and definitely wants to use the PCI_ROM_ADDRESS_ENABLE mask.
>
> - if (type == pci_bar_unknown) {
> + if (type == pci_bar_rom) {
> + res->flags |= (l & IORESOURCE_ROM_ENABLE);
> + l &= PCI_ROM_ADDRESS_MASK;
> + mask = (u32)PCI_ROM_ADDRESS_MASK;
> + } else {
This looks wrong too.
> if (rom) {
> @@ -344,7 +340,7 @@ static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom)
> res->flags = IORESOURCE_MEM | IORESOURCE_PREFETCH |
> IORESOURCE_READONLY | IORESOURCE_CACHEABLE |
> IORESOURCE_SIZEALIGN;
> - __pci_read_base(dev, pci_bar_mem32, res, rom);
> + pci_read_base(dev, pci_bar_mem32, res, rom);
> }
And you don't even change the type here ... have you tested this code on
a system which has a ROM?
>
> - for(i=0; i<3; i++)
> - child->resource[i] = &dev->resource[PCI_BRIDGE_RESOURCES+i];
> -
Er, this is rather important. Why can you just delete it?
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/6 v3] PCI: export some functions and macros
2008-09-27 8:27 [PATCH 1/6 v3] PCI: export some functions and macros Zhao, Yu
2008-09-27 12:59 ` Matthew Wilcox
@ 2008-10-01 16:52 ` Jesse Barnes
1 sibling, 0 replies; 4+ messages in thread
From: Jesse Barnes @ 2008-10-01 16:52 UTC (permalink / raw)
To: Zhao, Yu
Cc: linux-pci, Randy Dunlap, Grant Grundler, Alex Chiang,
Matthew Wilcox, Roland Dreier, Greg KH, linux-kernel, kvm,
virtualization
On Saturday, September 27, 2008 1:27 am Zhao, Yu wrote:
> Export some functions and move some macros from c file to header file.
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 9c71858..f99160d 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -696,7 +696,7 @@ static struct bin_attribute pci_config_attr = {
> .name = "config",
> .mode = S_IRUGO | S_IWUSR,
> },
> - .size = 256,
> + .size = PCI_CFG_SPACE_SIZE,
> .read = pci_read_config,
> .write = pci_write_config,
I just pushed Yanmin's cleanups here, can you separate out the rest of your
config space size changes and push them separately?
> extern int pci_uevent(struct device *dev, struct kobj_uevent_env *env);
> @@ -144,3 +150,17 @@ struct pci_slot_attribute {
> };
> #define to_pci_slot_attr(s) container_of(s, struct pci_slot_attribute,
> attr)
>
> +enum pci_bar_type {
> + pci_bar_unknown, /* Standard PCI BAR probe */
> + pci_bar_io, /* An io port BAR */
> + pci_bar_mem32, /* A 32-bit memory BAR */
> + pci_bar_mem64, /* A 64-bit memory BAR */
> + pci_bar_rom, /* A ROM BAR */
> +};
> +
> +extern int pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
> + struct resource *res, unsigned int reg);
> +extern struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
> + struct pci_dev *bridge, int busnr);
> +
> +#endif /* DRIVERS_PCI_H */
See Matthew's comments here; the pci_read_base changes should be part of a
separate patch too.
> a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
> index 3abbfad..6c78cf8 100644
> --- a/drivers/pci/setup-bus.c
> +++ b/drivers/pci/setup-bus.c
> @@ -299,7 +299,7 @@ static void pbus_size_io(struct pci_bus *bus)
>
> if (r->parent || !(r->flags & IORESOURCE_IO))
> continue;
> - r_size = r->end - r->start + 1;
> + r_size = resource_size(r);
>
> if (r_size < 0x400)
> /* Might be re-aligned for ISA */
> @@ -350,7 +350,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned
> long mask, unsigned long
>
> if (r->parent || (r->flags & mask) != type)
> continue;
> - r_size = r->end - r->start + 1;
> + r_size = resource_size(r);
> /* For bridges size != alignment */
> align = resource_alignment(r);
> order = __ffs(align) - 20;
> diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c
> index 1a5fc83..56e4042 100644
> --- a/drivers/pci/setup-res.c
> +++ b/drivers/pci/setup-res.c
> @@ -133,7 +133,7 @@ int pci_assign_resource(struct pci_dev *dev, int resno)
> resource_size_t size, min, align;
> int ret;
>
> - size = res->end - res->start + 1;
> + size = resource_size(res);
> min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO :
> PCIBIOS_MIN_MEM;
>
> align = resource_alignment(res);
These resource_size changes seem like good cleanups by themselves, can you
separate them out into a separate patch?
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 98dc624..cc78be6 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -456,8 +456,8 @@ struct pci_driver {
>
> /**
> * PCI_VDEVICE - macro used to describe a specific pci device in short
> form - * @vend: the vendor name
> - * @dev: the 16 bit PCI Device ID
> + * @vendor: the vendor name
> + * @device: the 16 bit PCI Device ID
> *
> * This macro is used to create a struct pci_device_id that matches a
> * specific PCI device. The subvendor, and subdevice fields will be set
Another good standalone cleanup, please submit separately.
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH 1/6 v3] PCI: export some functions and macros
2008-09-27 12:59 ` Matthew Wilcox
@ 2008-10-08 2:23 ` Zhao, Yu
0 siblings, 0 replies; 4+ messages in thread
From: Zhao, Yu @ 2008-10-08 2:23 UTC (permalink / raw)
To: Matthew Wilcox
Cc: linux-pci, Jesse Barnes, Randy Dunlap, Grant Grundler,
Alex Chiang, Roland Dreier, Greg KH, linux-kernel, kvm,
virtualization
On Saturday, September 27, 2008 8:59 PM, Matthew Wilcox wrote:
>On Sat, Sep 27, 2008 at 04:27:44PM +0800, Zhao, Yu wrote:
>> Export some functions and move some macros from c file to header file.
>
>That's absolutely not everything this patch does. You need to split
>this into smaller pieces and explain what you're doing and why for each
>of them.
Sure, I'll split it.
>
>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index d807cd7..596efa6 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -1,3 +1,9 @@
>> +#ifndef DRIVERS_PCI_H
>> +#define DRIVERS_PCI_H
>
>Do we really need header guards on this file?
Maybe it's not necessary, but we use guards in almost all private headers. So I added this to make this file look not so different.
>
>> -/*
>> - * If the type is not unknown, we assume that the lowest bit is 'enable'.
>> - * Returns 1 if the BAR was 64-bit and 0 if it was 32-bit.
>> +/**
>> + * pci_read_base - read a PCI BAR
>> + * @dev: the PCI device
>> + * @type: type of the BAR
>> + * @res: resource buffer to be filled in
>> + * @pos: BAR position in the config space
>> + *
>> + * Returns 1 if the BAR is 64-bit, or 0 if 32-bit.
>> */
>> -static int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
>> +int pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
>
>The original intent here was to have a pci_read_base() that called
>__pci_read_base() and then did things like translate physical BAR
>addresses to virtual ones. That patch is in the archives somewhere.
>We ended up not including that patch because my user found out he could
>get the address he wanted from elsewhere. I'm not sure we want to
>remove the __ at this point.
I've studied your patch that adds wrapper of __pci_read_base. If you are going to push it again, I'm ok with keeping the name unchanged.
>
>The eventual goal is to fix up the BARs at this point, but there's
>several architectures that will break if we do this now. It's on my
>long-term todo list.
>
>> struct resource *res, unsigned int pos)
>> {
>> u32 l, sz, mask;
>>
>> - mask = type ? ~PCI_ROM_ADDRESS_ENABLE : ~0;
>> + mask = (type == pci_bar_rom) ? ~PCI_ROM_ADDRESS_ENABLE : ~0;
>
>What's going on here? Why are you adding pci_bar_rom? For the rom we
>use pci_bar_mem32. Take a look at, for example, the MCHBAR in the 965
>spec (313053.pdf). That's something that uses the pci_bar_mem64 type
>and definitely wants to use the PCI_ROM_ADDRESS_ENABLE mask.
Thanks for pointing this out. I wonder how the PC_ROM_ADDRESS_ENABLE mask is set for those non-standard BARs like MCHBAR after the probe stage -- I don't think pci_update_resource will take care of them. So how about adding BAR type checking again pci_bar_mem32 in pci_update_resource so we can set the bit there?
>
>>
>> - if (type == pci_bar_unknown) {
>> + if (type == pci_bar_rom) {
>> + res->flags |= (l & IORESOURCE_ROM_ENABLE);
>> + l &= PCI_ROM_ADDRESS_MASK;
>> + mask = (u32)PCI_ROM_ADDRESS_MASK;
>> + } else {
>
>This looks wrong too.
>
>> if (rom) {
>> @@ -344,7 +340,7 @@ static void pci_read_bases(struct pci_dev *dev, unsigned
>int howmany, int rom)
>> res->flags = IORESOURCE_MEM | IORESOURCE_PREFETCH |
>> IORESOURCE_READONLY | IORESOURCE_CACHEABLE
>|
>> IORESOURCE_SIZEALIGN;
>> - __pci_read_base(dev, pci_bar_mem32, res, rom);
>> + pci_read_base(dev, pci_bar_mem32, res, rom);
>> }
>
>And you don't even change the type here ... have you tested this code on
>a system which has a ROM?
Oh, you caught it.
>
>>
>> - for(i=0; i<3; i++)
>> - child->resource[i] =
>&dev->resource[PCI_BRIDGE_RESOURCES+i];
>> -
>
>Er, this is rather important. Why can you just delete it?
I guess pci_alloc_child_bus has done this so we don't have to do it again.
>
>--
>Matthew Wilcox Intel Open Source Technology Centre
>"Bill, look, we understand that you're interested in selling us this
>operating system, but compare it to ours. We can't possibly take such
>a retrograde step."
>--
>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2008-10-08 2:23 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-09-27 8:27 [PATCH 1/6 v3] PCI: export some functions and macros Zhao, Yu
2008-09-27 12:59 ` Matthew Wilcox
2008-10-08 2:23 ` Zhao, Yu
2008-10-01 16:52 ` Jesse Barnes
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).