| Message ID | 1309357060-20872-1-git-send-email-dbaryshkov@gmail.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | f49a0c9c64a3f7eb15f72afbd754b4c13a8f0c49 |
| Headers | show |
> CPC925/CPC945 use special window to access host bridge > functionality of > u3-ht. Provide a way to access this device. Why? Is anything going to use it? > +static int u3_ht_root_read_config(struct pci_controller *hose, u8 > offset, > + int len, u32 *val) > +{ > + volatile void __iomem *addr; > + > + addr = hose->cfg_addr; > + addr += ((offset & ~3) << 2) + (4 - len - (offset & 3)); This will only work for len 1,2,4 with offset a multiple of len, is that guaranteed here? > +static int u3_ht_root_write_config(struct pci_controller *hose, u8 > offset, > + int len, u32 val) > +{ > + volatile void __iomem *addr; > + > + addr = hose->cfg_addr + ((offset & ~3) << 2) + (4 - len - (offset > & 3)); > + > + if (offset >= PCI_BASE_ADDRESS_0 && offset < PCI_CAPABILITY_LIST) > + return PCIBIOS_SUCCESSFUL; This is a workaround for something; at the very least it needs a comment, but probably it shouldn't be here at all. > static int u3_ht_read_config(struct pci_bus *bus, unsigned int devfn, > int offset, int len, u32 *val) > { > @@ -217,6 +265,9 @@ static int u3_ht_read_config(struct pci_bus > *bus, unsigned int devfn, > if (hose == NULL) > return PCIBIOS_DEVICE_NOT_FOUND; > > + if (bus->number == hose->first_busno && devfn == PCI_DEVFN(0, 0)) > + return u3_ht_root_read_config(hose, offset, len, val); > + > if (offset > 0xff) > return PCIBIOS_BAD_REGISTER_NUMBER; u3_ht_root_read_config() can get an offset out of range this way. > hose->cfg_data = ioremap(0xf2000000, 0x02000000); > + hose->cfg_addr = ioremap(0xf8070000, 0x1000); Eww. You could just make a global instead of abusing existing fields, there can be only one CPC9x5 in a system anyway. Segher
Hello, Thanks for the review. On 7/1/11, Segher Boessenkool <segher@kernel.crashing.org> wrote: >> CPC925/CPC945 use special window to access host bridge >> functionality of >> u3-ht. Provide a way to access this device. > > Why? Is anything going to use it? Hmmm. Why not? Initially I stumbled upon the fact that powermac provides such acces. Then I discovered that it provides configuration for memory windows and other parts. (I needed it for my hack to add AGP bridge on U3 in Linux, as I didn't want to change firmware). >> +static int u3_ht_root_read_config(struct pci_controller *hose, u8 >> offset, >> + int len, u32 *val) >> +{ >> + volatile void __iomem *addr; >> + >> + addr = hose->cfg_addr; >> + addr += ((offset & ~3) << 2) + (4 - len - (offset & 3)); > > This will only work for len 1,2,4 with offset a multiple of len, is that > guaranteed here? I have to check. I think there is no guarantee, but standard accessors are created exactly for 1, 2 and 4-byte access. And IIRC according to PCI specs, offset should be len-aligned. >> +static int u3_ht_root_write_config(struct pci_controller *hose, u8 >> offset, >> + int len, u32 val) >> +{ >> + volatile void __iomem *addr; >> + >> + addr = hose->cfg_addr + ((offset & ~3) << 2) + (4 - len - (offset >> & 3)); >> + >> + if (offset >= PCI_BASE_ADDRESS_0 && offset < PCI_CAPABILITY_LIST) >> + return PCIBIOS_SUCCESSFUL; > > This is a workaround for something; at the very least it needs a > comment, > but probably it shouldn't be here at all. I'll add a comment. Basically these registers are unimplemented on u3/u4 bridges and at least some kinds of access to them cause system freeze. I'll try to check what triggers what this night. >> static int u3_ht_read_config(struct pci_bus *bus, unsigned int devfn, >> int offset, int len, u32 *val) >> { >> @@ -217,6 +265,9 @@ static int u3_ht_read_config(struct pci_bus >> *bus, unsigned int devfn, >> if (hose == NULL) >> return PCIBIOS_DEVICE_NOT_FOUND; >> >> + if (bus->number == hose->first_busno && devfn == PCI_DEVFN(0, 0)) >> + return u3_ht_root_read_config(hose, offset, len, val); >> + >> if (offset > 0xff) >> return PCIBIOS_BAD_REGISTER_NUMBER; > > u3_ht_root_read_config() can get an offset out of range this way. Yes, as offsets for this host bridge can be larger then 0xff. U3/U4 have some HT configuration there, and I didn't want to impose a limit there. If you are against this, I can change the order of calls. > >> hose->cfg_data = ioremap(0xf2000000, 0x02000000); >> + hose->cfg_addr = ioremap(0xf8070000, 0x1000); > > Eww. You could just make a global instead of abusing existing fields, > there can be only one CPC9x5 in a system anyway. This was a copy-paste of corresponding PowerMac code. Do you really want for me to change this to global variable? BTW: I see a lot of code duplication between PowerMac and Maple pci.c files. Would you like a patch that merges those files to something like arch/powerpc/sysdev/u3-pci.c? I can try merging them...
>>> CPC925/CPC945 use special window to access host bridge >>> functionality of >>> u3-ht. Provide a way to access this device. >> >> Why? Is anything going to use it? > > Hmmm. Why not? Because if nothing uses it it is essentially dead code. > Initially I stumbled upon the fact that powermac provides > such acces. Then I discovered that it provides configuration for > memory windows > and other parts. There are no memory windows in there afaik. The main use is the HT link config stuff, which in a few places needs special handling to work :-/ > (I needed it for my hack to add AGP bridge on U3 in Linux, > as I didn't want to change firmware). Can you tell more about this? >>> +static int u3_ht_root_read_config(struct pci_controller *hose, u8 >>> offset, >>> + int len, u32 *val) >>> +{ >>> + volatile void __iomem *addr; >>> + >>> + addr = hose->cfg_addr; >>> + addr += ((offset & ~3) << 2) + (4 - len - (offset & 3)); >> >> This will only work for len 1,2,4 with offset a multiple of len, >> is that >> guaranteed here? > > I have to check. I think there is no guarantee, but standard accessors > are created exactly for 1, 2 and 4-byte access. And IIRC according > to PCI specs, offset should be len-aligned. If other places that need it do not do an alignment check, you don't need to either I suppose. But could you make the switch for length have explicit 1,2,4 and default error? >>> +static int u3_ht_root_write_config(struct pci_controller *hose, u8 >>> offset, >>> + int len, u32 val) >>> +{ >>> + volatile void __iomem *addr; >>> + >>> + addr = hose->cfg_addr + ((offset & ~3) << 2) + (4 - len - (offset >>> & 3)); >>> + >>> + if (offset >= PCI_BASE_ADDRESS_0 && offset < PCI_CAPABILITY_LIST) >>> + return PCIBIOS_SUCCESSFUL; >> >> This is a workaround for something; at the very least it needs a >> comment, >> but probably it shouldn't be here at all. > > I'll add a comment. Basically these registers are unimplemented on > u3/u4 > bridges and at least some kinds of access to them cause system freeze. > I'll try to check what triggers what this night. I don't think the hardware freezes, but probably Linux isn't happy when it tries to write to the non-existent BARs. Or something like that. >>> static int u3_ht_read_config(struct pci_bus *bus, unsigned int >>> devfn, >>> int offset, int len, u32 *val) >>> { >>> @@ -217,6 +265,9 @@ static int u3_ht_read_config(struct pci_bus >>> *bus, unsigned int devfn, >>> if (hose == NULL) >>> return PCIBIOS_DEVICE_NOT_FOUND; >>> >>> + if (bus->number == hose->first_busno && devfn == PCI_DEVFN(0, 0)) >>> + return u3_ht_root_read_config(hose, offset, len, val); >>> + >>> if (offset > 0xff) >>> return PCIBIOS_BAD_REGISTER_NUMBER; >> >> u3_ht_root_read_config() can get an offset out of range this way. > > Yes, as offsets for this host bridge can be larger then 0xff. U3/U4 > have > some HT configuration there, and I didn't want to impose a limit > there. > If you are against this, I can change the order of calls. The HT config is at (PCI) offset 0x40. There are no implemented registers at PCI offset >= 0x100. That corresponds to f8070000..f80703ff, there's one 4-byte PCI reg per 16 byte U3 bus address. >>> hose->cfg_data = ioremap(0xf2000000, 0x02000000); >>> + hose->cfg_addr = ioremap(0xf8070000, 0x1000); >> >> Eww. You could just make a global instead of abusing existing >> fields, >> there can be only one CPC9x5 in a system anyway. > > This was a copy-paste of corresponding PowerMac code. Do you really > want > for me to change this to global variable? It could use a comment at least. The addresses aren't "cfg_data" and "cfg_addr" at all. > BTW: I see a lot of code duplication between PowerMac and Maple pci.c > files. Would you like a patch that merges those files to something > like arch/powerpc/sysdev/u3-pci.c? I can try merging them... That would be most excellent! I hope there isn't much pmac special- casing needed for that. Thanks, Segher
On 7/3/11, Segher Boessenkool <segher@kernel.crashing.org> wrote: >>>> CPC925/CPC945 use special window to access host bridge >>>> functionality of >>>> u3-ht. Provide a way to access this device. >>> >>> Why? Is anything going to use it? >> >> Hmmm. Why not? > > Because if nothing uses it it is essentially dead code. 1) It provides some (possibly debug) information to users 2) It provides a way to configure (e.g. HT address mask (reg 80), some bridge control regs (around c0-ff), etc. >> Initially I stumbled upon the fact that powermac provides >> such acces. Then I discovered that it provides configuration for >> memory windows >> and other parts. > > There are no memory windows in there afaik. The main use is the HT > link config stuff, which in a few places needs special handling to > work :-/ Register 80 (0xf8070200) is an HT1 address mask, which configures which memory belongs to HT bridge & devices (contrary to AGP/PCIe) >> (I needed it for my hack to add AGP bridge on U3 in Linux, >> as I didn't want to change firmware). > > Can you tell more about this? I have a Mapple-D board (aka IBM PPC970FX ev. kit). It has an AGP slot, which I would like to use. The slot is unsupported in PIBS (the original firmware) (it doesn't enumerate devices behind the slot, it doesn't setup it, it doesn't put u3-agp in device tree). I didn't want to hack the PIBS (at least while it's possible and for initial debug of the hardware setup), so I had to develop a patch for Linux kernel which would: 1) configure hardware (memory windows, IRQs, etc). 2) tell Linux about the AGP bridge. Second part is more or less complete (with some hacks though). Basically I register an u3-agp slot if there is no u3-agp and u4-pcie devices. This still needs an improvement before I'd submit this code anywhere. I've mostly finished the HW setup (I can see devices behind the bridge). The radeon plugged into AGP slot starts to warm up, but it fails somewhere. The thing I still miss (or at least one of them) is the IRQ routing. Ben gave me some possible pointers, so I'll have to check if he is right or not. Unfortunately I still hand't time to accomplish this. I can show my (possibly a bit ugly) code, but it still doesn't fully work. >>>> +static int u3_ht_root_read_config(struct pci_controller *hose, u8 >>>> offset, >>>> + int len, u32 *val) >>>> +{ >>>> + volatile void __iomem *addr; >>>> + >>>> + addr = hose->cfg_addr; >>>> + addr += ((offset & ~3) << 2) + (4 - len - (offset & 3)); >>> >>> This will only work for len 1,2,4 with offset a multiple of len, >>> is that >>> guaranteed here? >> >> I have to check. I think there is no guarantee, but standard accessors >> are created exactly for 1, 2 and 4-byte access. And IIRC according >> to PCI specs, offset should be len-aligned. > > If other places that need it do not do an alignment check, you don't > need > to either I suppose. But could you make the switch for length have > explicit 1,2,4 and default error? Yes, of course. I wanted to overcome this, but of course I can split it back to the switch. BTW: the corresponding PowerMac code has a bug: try reading VID/DID as a byte. word and dword and compare the outcome with the output from typical PCI device. >>>> +static int u3_ht_root_write_config(struct pci_controller *hose, u8 >>>> offset, >>>> + int len, u32 val) >>>> +{ >>>> + volatile void __iomem *addr; >>>> + >>>> + addr = hose->cfg_addr + ((offset & ~3) << 2) + (4 - len - (offset >>>> & 3)); >>>> + >>>> + if (offset >= PCI_BASE_ADDRESS_0 && offset < PCI_CAPABILITY_LIST) >>>> + return PCIBIOS_SUCCESSFUL; >>> >>> This is a workaround for something; at the very least it needs a >>> comment, >>> but probably it shouldn't be here at all. >> >> I'll add a comment. Basically these registers are unimplemented on >> u3/u4 >> bridges and at least some kinds of access to them cause system freeze. >> I'll try to check what triggers what this night. > > I don't think the hardware freezes, but probably Linux isn't happy > when it > tries to write to the non-existent BARs. Or something like that. IIRC I've had problems even when reading. Will run an experiment tomorrow. >>>> static int u3_ht_read_config(struct pci_bus *bus, unsigned int >>>> devfn, >>>> int offset, int len, u32 *val) >>>> { >>>> @@ -217,6 +265,9 @@ static int u3_ht_read_config(struct pci_bus >>>> *bus, unsigned int devfn, >>>> if (hose == NULL) >>>> return PCIBIOS_DEVICE_NOT_FOUND; >>>> >>>> + if (bus->number == hose->first_busno && devfn == PCI_DEVFN(0, 0)) >>>> + return u3_ht_root_read_config(hose, offset, len, val); >>>> + >>>> if (offset > 0xff) >>>> return PCIBIOS_BAD_REGISTER_NUMBER; >>> >>> u3_ht_root_read_config() can get an offset out of range this way. >> >> Yes, as offsets for this host bridge can be larger then 0xff. U3/U4 >> have >> some HT configuration there, and I didn't want to impose a limit >> there. >> If you are against this, I can change the order of calls. > > The HT config is at (PCI) offset 0x40. There are no implemented > registers > at PCI offset >= 0x100. That corresponds to f8070000..f80703ff, there's > one 4-byte PCI reg per 16 byte U3 bus address. Hmmm. I saw the "HT Performance Monitor" part of the U4 spec, but didn't notice that all regs are marked as "not implemented". >>>> hose->cfg_data = ioremap(0xf2000000, 0x02000000); >>>> + hose->cfg_addr = ioremap(0xf8070000, 0x1000); >>> >>> Eww. You could just make a global instead of abusing existing >>> fields, >>> there can be only one CPC9x5 in a system anyway. >> >> This was a copy-paste of corresponding PowerMac code. Do you really >> want >> for me to change this to global variable? > > It could use a comment at least. The addresses aren't "cfg_data" and > "cfg_addr" at all. Fine, I'll add a comment here. >> BTW: I see a lot of code duplication between PowerMac and Maple pci.c >> files. Would you like a patch that merges those files to something >> like arch/powerpc/sysdev/u3-pci.c? I can try merging them... > > That would be most excellent! I hope there isn't much pmac special- > casing > needed for that. Still have to check. Please don't expect a patch soon.
Hello, On 03.07.2011 01:50, Segher Boessenkool wrote: >>>> +static int u3_ht_root_write_config(struct pci_controller *hose, u8 >>>> offset, >>>> + int len, u32 val) >>>> +{ >>>> + volatile void __iomem *addr; >>>> + >>>> + addr = hose->cfg_addr + ((offset & ~3) << 2) + (4 - len - (offset >>>> & 3)); >>>> + >>>> + if (offset >= PCI_BASE_ADDRESS_0 && offset < PCI_CAPABILITY_LIST) >>>> + return PCIBIOS_SUCCESSFUL; >>> >>> This is a workaround for something; at the very least it needs a >>> comment, >>> but probably it shouldn't be here at all. >> >> I'll add a comment. Basically these registers are unimplemented on u3/u4 >> bridges and at least some kinds of access to them cause system freeze. >> I'll try to check what triggers what this night. > > I don't think the hardware freezes, but probably Linux isn't happy when it > tries to write to the non-existent BARs. Or something like that. I've run several experiments. At least writing to the ROM_ADDRESS (0x30) causes some kind of strange reboot (I see 'Error: Magic number in message area NVRAM is not valid.') errors on the service console and Linux consoles are silent. Writing to other BARs seem to cause no direct effect (reg remains =0), but causes very strange behaviour during boot. All PCI access cycles seem to return 0, strange DRAM ECC error pops up, etc. I'd prefer to leave these register writes disabled, even if it's the problem of some hardware revision (?).
On Fri, 2011-07-01 at 18:44 +0200, Segher Boessenkool wrote: > > CPC925/CPC945 use special window to access host bridge > > functionality of > > u3-ht. Provide a way to access this device. > > Why? Is anything going to use it? > > > +static int u3_ht_root_read_config(struct pci_controller *hose, u8 > > offset, > > + int len, u32 *val) > > +{ > > + volatile void __iomem *addr; > > + > > + addr = hose->cfg_addr; > > + addr += ((offset & ~3) << 2) + (4 - len - (offset & 3)); > > This will only work for len 1,2,4 with offset a multiple of len, is that > guaranteed here? I think the upper layer does. Dbl check tho. > > hose->cfg_data = ioremap(0xf2000000, 0x02000000); > > + hose->cfg_addr = ioremap(0xf8070000, 0x1000); > > Eww. You could just make a global instead of abusing existing fields, > there can be only one CPC9x5 in a system anyway. Nah, that's fine, we abuse it that way regulary :-) Cheers, Ben.
diff --git a/arch/powerpc/platforms/maple/pci.c b/arch/powerpc/platforms/maple/pci.c index dd2e48b..401e3f3 100644 --- a/arch/powerpc/platforms/maple/pci.c +++ b/arch/powerpc/platforms/maple/pci.c @@ -207,6 +207,54 @@ static volatile void __iomem *u3_ht_cfg_access(struct pci_controller* hose, return hose->cfg_data + u3_ht_cfa1(bus, devfn, offset); } +static int u3_ht_root_read_config(struct pci_controller *hose, u8 offset, + int len, u32 *val) +{ + volatile void __iomem *addr; + + addr = hose->cfg_addr; + addr += ((offset & ~3) << 2) + (4 - len - (offset & 3)); + + switch (len) { + case 1: + *val = in_8(addr); + break; + case 2: + *val = in_be16(addr); + break; + default: + *val = in_be32(addr); + break; + } + + return PCIBIOS_SUCCESSFUL; +} + +static int u3_ht_root_write_config(struct pci_controller *hose, u8 offset, + int len, u32 val) +{ + volatile void __iomem *addr; + + addr = hose->cfg_addr + ((offset & ~3) << 2) + (4 - len - (offset & 3)); + + if (offset >= PCI_BASE_ADDRESS_0 && offset < PCI_CAPABILITY_LIST) + return PCIBIOS_SUCCESSFUL; + + switch (len) { + case 1: + out_8(addr, val); + break; + case 2: + out_be16(addr, val); + break; + default: + out_be32(addr, val); + break; + } + + return PCIBIOS_SUCCESSFUL; +} + static int u3_ht_read_config(struct pci_bus *bus, unsigned int devfn, int offset, int len, u32 *val) { @@ -217,6 +265,9 @@ static int u3_ht_read_config(struct pci_bus *bus, unsigned int devfn, if (hose == NULL) return PCIBIOS_DEVICE_NOT_FOUND; + if (bus->number == hose->first_busno && devfn == PCI_DEVFN(0, 0)) + return u3_ht_root_read_config(hose, offset, len, val); + if (offset > 0xff) return PCIBIOS_BAD_REGISTER_NUMBER; @@ -252,6 +303,9 @@ static int u3_ht_write_config(struct pci_bus *bus, unsigned int devfn, if (hose == NULL) return PCIBIOS_DEVICE_NOT_FOUND; + if (bus->number == hose->first_busno && devfn == PCI_DEVFN(0, 0)) + return u3_ht_root_write_config(hose, offset, len, val); + if (offset > 0xff) return PCIBIOS_BAD_REGISTER_NUMBER; @@ -428,6 +482,7 @@ static void __init setup_u3_ht(struct pci_controller* hose) * reg_property and using some accessor functions instead */ hose->cfg_data = ioremap(0xf2000000, 0x02000000); + hose->cfg_addr = ioremap(0xf8070000, 0x1000); hose->first_busno = 0; hose->last_busno = 0xef;
CPC925/CPC945 use special window to access host bridge functionality of u3-ht. Provide a way to access this device. Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> --- arch/powerpc/platforms/maple/pci.c | 55 ++++++++++++++++++++++++++++++++++++ 1 files changed, 55 insertions(+), 0 deletions(-)