On 03.01.2010, at 17:20, Michael S. Tsirkin wrote: > On Sun, Jan 03, 2010 at 05:13:12PM +0100, Alexander Graf wrote: >> >> On 03.01.2010, at 16:47, Michael S. Tsirkin wrote: >> >>> On Sun, Jan 03, 2010 at 04:40:12PM +0100, Alexander Graf wrote: >>>> >>>> On 03.01.2010, at 16:32, Michael S. Tsirkin wrote: >>>> >>>>> On Sun, Jan 03, 2010 at 02:50:46AM +0100, Alexander Graf wrote: >>>>>> As stated in the previous patch, the Uninorth PCI bridge requires >>>>>> different >>>>>> layouts in its PCI config space accessors. >>>>>> >>>>>> This patch introduces a conversion function that makes it compatible with >>>>>> the way Linux accesses it. >>>>>> >>>>>> I also kept an OpenBIOS compatibility hack in. I think it'd be better to >>>>>> take small steps here and do the config space access rework in OpenBIOS >>>>>> later on. When that's done we can remove that hack. >>>>>> >>>>>> Signed-off-by: Alexander Graf <ag...@suse.de> >>>>>> --- >>>>>> hw/unin_pci.c | 35 +++++++++++++++++++++++++++++++++++ >>>>>> 1 files changed, 35 insertions(+), 0 deletions(-) >>>>>> >>>>>> diff --git a/hw/unin_pci.c b/hw/unin_pci.c >>>>>> index fdb9401..1c49008 100644 >>>>>> --- a/hw/unin_pci.c >>>>>> +++ b/hw/unin_pci.c >>>>>> @@ -75,6 +75,40 @@ static void pci_unin_reset(void *opaque) >>>>>> { >>>>>> } >>>>>> >>>>>> +static uint32_t unin_get_config_reg(PCIHostState *s, uint32_t addr) >>>>>> +{ >>>>>> + uint32_t retval; >>>>>> + uint32_t reg = s->config_reg; >>>>>> + >>>>>> + if (reg & (1u << 31)) { >>>>>> + /* XXX OpenBIOS compatibility hack */ >>>>>> + retval = reg; >>>>>> + addr |= reg & 7; >>>>>> + } else if (reg & 1) { >>>>>> + /* Set upper valid bit and remove lower one */ >>>>>> + retval = (reg & ~3u) | (1u << 31); >>>>>> + } else { >>>>>> + uint32_t slot, func; >>>>>> + uint32_t devfn; >>>>>> + >>>>>> + /* Grab CFA0 style values */ >>>>>> + slot = ffs(reg & 0xfffff800) - 1; >>>>>> + func = (reg >> 8) & 7; >>>>>> + devfn = PCI_DEVFN(slot, func); >>>>>> + >>>>>> + /* ... and then convert them to x86 format */ >>>>>> + retval = (reg & 0xfc) | (devfn << 8) | (1u << 31); >>>>> >>>>> Is it a good idea to have a helper that encodes reg/dev/fn into a 32 bit >>>>> number? This way this encoding can be changed down the road. >>>> >>>> I don't think I understand this comment? :-) >>> >>> This puts reg+dev+fn in a format that pci_host can the understand >>> correct? So it would make sense to have an inline function >>> in pci host that gets 3 parameters and does the encoding. >> >> We're doing the reverse here. We get a uint32_t (host->config_reg) and need >> to do something with it. >> >> We could either call a helper that splits it into bus,dev,fn or we could >> just put all of them into a single uint32_t again that later on gets >> interpreted in a specified format. >> >> I figured I'd have to touch less code and keep things more stable for the >> other (non-uninorth) buses if I keep the x86 format as "default format" and >> just convert to it. Passing a uint32_t is also easier than passing 3 ints >> :-). >> >> Alex > > So what the comment above suggests, is adding helper routine > that gets register device, function and creates 32 bit value > in "default format".
Oh, so you mean that instead of returning a uint32_t that magically gets converted inside the conversion function, we'd create another function like this: uint32_t pci_host_config_address(int bus, int dev, int fn) { return (1u << 31) | (bus << 11) | (dev << 3) | fn; } which then would be called at the end of the conversion function? Alex