PCI question

PCI question

[Pat Gefre]

The usage of pci_dev.sysdata is a bit confusing to me. I had initially
thought that this was available for any system specific use,  but then
in pcibios_fixup_device_resources() it is specifically cast as a struct
pci_controller *:

#define PCI_CONTROLLER(busdev) ((struct pci_controller *) busdev->sysdata)

void __init
pcibios_fixup_device_resources (struct pci_dev *dev, struct pci_bus *bus)
{
   struct pci_controller *controller = PCI_CONTROLLER(dev);

In this funtion  the pci_controller elements 'window[]' and 'windows'
are also used.  This implies to me that these fields must be maintained
in any system specific structure ? i.e.

struct my_sysdata_struct {
   struct pci_controller dummy;
   /* my stuff would go here */
   struct my_real_sysdata_stuff;
};

Yet I don't see where anyone else is doing the above. Am I missing
something obvious ??

Thanks,
-- Pat

Re: PCI question

[Matthew Wilcox]

On Tue, Feb 03, 2004 at 04:35:26PM -0600, Pat Gefre wrote:
> The usage of pci_dev.sysdata is a bit confusing to me. I had initially
> thought that this was available for any system specific use,  but then
> in pcibios_fixup_device_resources() it is specifically cast as a struct
> pci_controller *:

It's up to the architecture how it's used.  ia64 has decided that it
shall be a struct pci_controller *.

> In this funtion  the pci_controller elements 'window[]' and 'windows'
> are also used.  This implies to me that these fields must be maintained
> in any system specific structure ? i.e.
> 
> struct my_sysdata_struct {
>    struct pci_controller dummy;
>    /* my stuff would go here */
>    struct my_real_sysdata_stuff;
> };
> 
> Yet I don't see where anyone else is doing the above. Am I missing
> something obvious ??

Urgh, don't do that.  What do you need to do that can't be added to the
pci_controller?

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: PCI question

[Patrick Gefre]

Matthew Wilcox wrote:

>On Tue, Feb 03, 2004 at 04:35:26PM -0600, Pat Gefre wrote:
>  
>
>>The usage of pci_dev.sysdata is a bit confusing to me. I had initially
>>thought that this was available for any system specific use,  but then
>>in pcibios_fixup_device_resources() it is specifically cast as a struct
>>pci_controller *:
>>    
>>
>
>It's up to the architecture how it's used.  ia64 has decided that it
>shall be a struct pci_controller *.
>
>  
>
>>In this funtion  the pci_controller elements 'window[]' and 'windows'
>>are also used.  This implies to me that these fields must be maintained
>>in any system specific structure ? i.e.
>>
>>struct my_sysdata_struct {
>>   struct pci_controller dummy;
>>   /* my stuff would go here */
>>   struct my_real_sysdata_stuff;
>>};
>>
>>Yet I don't see where anyone else is doing the above. Am I missing
>>something obvious ??
>>    
>>
>
>Urgh, don't do that.  What do you need to do that can't be added to the
>pci_controller?
>
>  
>
OK- good - thanks. Yes the other option is to add it to the 
pci_controller struct.

-- Pat

Re: PCI question

[Jesse Barnes]

On Tue, Feb 03, 2004 at 05:15:44PM -0600, Patrick Gefre wrote:
> >Urgh, don't do that.  What do you need to do that can't be added to the
> >pci_controller?
>
> OK- good - thanks. Yes the other option is to add it to the 
> pci_controller struct.

Add what?  How about pci_controller->platform_data?  That's private to
the subarch, right?

Jesse

Re: PCI question

[Patrick Gefre]

Jesse Barnes wrote:

>On Tue, Feb 03, 2004 at 05:15:44PM -0600, Patrick Gefre wrote:
>  
>
>>>Urgh, don't do that.  What do you need to do that can't be added to the
>>>pci_controller?
>>>      
>>>
>>OK- good - thanks. Yes the other option is to add it to the 
>>pci_controller struct.
>>    
>>
>
>Add what?  How about pci_controller->platform_data?  That's private to
>the subarch, right?
>
>Jesse
>  
>
But not in 2.6 ??

struct pci_controller {
        void *acpi_handle;
        void *iommu;
        int segment;

        unsigned int windows;
        struct pci_window *window;
};

Re: PCI question

[Jesse Barnes]

On Tue, Feb 03, 2004 at 05:36:53PM -0600, Patrick Gefre wrote:
> But not in 2.6 ??
> 
> struct pci_controller {
>        void *acpi_handle;
>        void *iommu;
>        int segment;
> 
>        unsigned int windows;
>        struct pci_window *window;
> };

Oops, I was looking at a 2.4 tree.  I wonder if we can overload
acpi_handle for our purposes though?  Since we don't have an ACPI
namespace, we obviously don't store things like the bridge base address
and PIO mapping information there, so we may be able to make this field
point to an sn2 specific structure that contains this info (I'm assuming
that's what we need).

What else do we need to keep track of?  The PCI to node mapping should
be stored seperately (I think mkp had a patch to implement that), but
I'm sure there are other things I'm missing...

Thanks,
Jesse

Re: PCI question

[Patrick Gefre]

Jesse Barnes wrote:

>On Tue, Feb 03, 2004 at 05:36:53PM -0600, Patrick Gefre wrote:
>  
>
>>But not in 2.6 ??
>>
>>struct pci_controller {
>>       void *acpi_handle;
>>       void *iommu;
>>       int segment;
>>
>>       unsigned int windows;
>>       struct pci_window *window;
>>};
>>    
>>
>
>Oops, I was looking at a 2.4 tree.  I wonder if we can overload
>acpi_handle for our purposes though?  Since we don't have an ACPI
>namespace, we obviously don't store things like the bridge base address
>and PIO mapping information there, so we may be able to make this field
>point to an sn2 specific structure that contains this info (I'm assuming
>that's what we need).
>
>What else do we need to keep track of?  The PCI to node mapping should
>be stored seperately (I think mkp had a patch to implement that), but
>I'm sure there are other things I'm missing...
>
>Thanks,
>Jesse
>  
>
I think the 2.4 definition will be fine - at least for now. Was there a 
reason it was changed in 2.6 ?

2.4 definition:
struct pci_controller {
        void *acpi_handle;
        void *iommu;
        int segment;

        unsigned int windows;
        struct pci_window *window;

        void *platform_data;
};


-- Pat

Re: PCI question

[Grant Grundler]

On Wed, Feb 04, 2004 at 11:04:42AM -0600, Patrick Gefre wrote:
> I think the 2.4 definition will be fine - at least for now. Was there a 
> reason it was changed in 2.6 ?

Because no one was using it?

Pat,
two (three?) people have asked you to state what you want to add.
Can you state the basic problem additional data fields would solve?

It might be easier to just add those fields directly to pci_controller
instead of messing with platform_data.

grant

Re: PCI question

[Patrick Gefre]

Grant Grundler wrote:

>On Wed, Feb 04, 2004 at 11:04:42AM -0600, Patrick Gefre wrote:
>  
>
>>I think the 2.4 definition will be fine - at least for now. Was there a 
>>reason it was changed in 2.6 ?
>>    
>>
>
>Because no one was using it?
>
>Pat,
>two (three?) people have asked you to state what you want to add.
>Can you state the basic problem additional data fields would solve?
>
>It might be easier to just add those fields directly to pci_controller
>instead of messing with platform_data.
>
>grant
>  
>
We keep track of several things that are specific to our system:

struct sn_device_sysdata {
        vertex_hdl_t            vhdl;
        pciio_provider_t        *pci_provider;
        pciio_intr_t            intr_handle;
        struct sn_flush_device_list *dma_flush_list;
        pciio_piomap_t          pio_map[PCI_ROM_RESOURCE];
};

The platform_data element is what we use in 2.4 and seems like the way 
to go for 2.6.

-- Pat

Re: PCI question

[Bjorn Helgaas]

On Wednesday 04 February 2004 12:10 pm, Patrick Gefre wrote:
> We keep track of several things that are specific to our system:
> 
> struct sn_device_sysdata {
>         vertex_hdl_t            vhdl;
>         pciio_provider_t        *pci_provider;
>         pciio_intr_t            intr_handle;
>         struct sn_flush_device_list *dma_flush_list;
>         pciio_piomap_t          pio_map[PCI_ROM_RESOURCE];
> };
> 
> The platform_data element is what we use in 2.4 and seems like the way 
> to go for 2.6.

If you make yourself a macro to go from a pci_dev to sn_device_sysdata,
you can make your code independent of where the SGI-specific stuff
actually lives.  Probably you already do that.

Re: PCI question

[Patrick Gefre]

Greg KH wrote:

>On Tue, Feb 03, 2004 at 04:27:09PM -0600, Patrick Gefre wrote:
>  
>
>>The usage of pci_dev.sysdata is a bit confusing to me. I had initially
>>thought that this was available for any system specific use,  but then
>>in pcibios_fixup_device_resources() it is specifically cast as a
>>struct pci_controller *:
>>
>>#define PCI_CONTROLLER(busdev) ((struct pci_controller *) busdev->sysdata)
>>
>>void __init
>>pcibios_fixup_device_resources (struct pci_dev *dev, struct pci_bus *bus)
>>    
>>
>
>What kernel are you looking at?  I don't see either of the above in the
>2.6.2 kernel tree.
>
>thanks,
>
>greg k-h
>  
>
In my bitkeeper tree : (bk parent: http://linux.bkbits.net/linux-2.5)

include/asm-ia64/pci.h:
#define PCI_CONTROLLER(busdev) ((struct pci_controller *) busdev->sysdata)

arch/ia64/pci/pci.c:
void __init
pcibios_fixup_device_resources (struct pci_dev *dev, struct pci_bus *bus)

Is there some ia64 patch I'm missing that removes this ?

-- Pat

RE: PCI question

[Cameron, Steve]

I wrote:
> Is it possible for a driver to know the size of each 
> the BARs which struct pci_dev->resource[x].start corresponds with?
[...]
> drivers/pci/pci.c:read_pci_bases() looks like it tests
> to see if BARs are 32 or 64 bit, but does not save this
> information in pci_dev->resource[x],[...]

Duh!  Now I see where it saves it in resource[x]->flags...
The low bits tell me this information.  (Sorry about the noise.)

-- steve

PCI question

[Stephen Cameron]

Is it possible for a driver to know the size of each 
the BARs which struct pci_dev->resource[x].start corresponds with?

(e.g. whether it is a 64 bit BAR, or a 32 bit BAR?)

Why do I need to do this?

Because I have a controller (cciss driver) where some 
BARS may be sometimes 32 bit, sometimes 64 bit (depending 
on the board).

There's a sequence that you have to go through while init'ing
the board where you get an offset from PCI BAR 0 to know what
BAR to use for a certain other bit of memory.
Rather than reading that BAR directly, I want to use what's
in struct pci_dev->resource[x].start, but I do not know what
'x' is.  (I need to find x.)

Currently the code assumes all the BARs are 32 bits, so it does
something like this.

	x = (my_bar_offset  - PCI_BASE_ADDRESS_0)/4;

Instead, I need to do something along the lines of:

	offset = 0;
	for (i=0;i<DEVICE_COUNT_RESOURCES;i++) {
		if offset == (my_bar_offset - PCI_BASE_ADDRESS_0) {
			x = offset;
			break;
		}
		if ( BAR i is 32 bit )
			offset += 4;
		else if ( BAR i is 64 bit )
			ofset += 8;
	}
	if (i==DEVICE_COUNT_RESOURCES)
		offset = -1; /* didn't find it */

But I need to be able to figure out how big each BAR is.
Can I do that without going thru all the rigamarole of 
writing all 0xfffff's to the BAR, then reading it back, 
etc?

I didn't see anywhere in struct pci_dev->resource[x] 
where the size of the bar was saved.  It didn't seem 
to be encoded in the flags that I could see...	

drivers/pci/pci.c:read_pci_bases() looks like it tests
to see if BARs are 32 or 64 bit, but does not save this
information in pci_dev->resource[x], except in the fact that
if fills in the topmost 32 bits of addr.

Any ideas?

Another idea is to just catalog the boards and
just "know" which boards have 64 bit BARs and where,
but I'd rather have more general code.

Thanks,

-- steve