| Message ID | 1262483450-15206-3-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
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 <agraf@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. > + } > + > + retval &= ~3u; > + retval |= (addr & 7); > + > + UNIN_DPRINTF("Converted config space accessor %08x/%08x -> %08x\n", > + reg, addr, retval); > + > + return retval; > +} > + > static int pci_unin_main_init_device(SysBusDevice *dev) > { > UNINState *s; > @@ -85,6 +119,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev) > s = FROM_SYSBUS(UNINState, dev); > > pci_host_init(&s->host_state); > + s->host_state.get_config_reg = unin_get_config_reg; This looks slightly ugly: let's make pci_host_init accept the conversion function instead? > pci_mem_config = pci_host_conf_register_mmio(&s->host_state); > pci_mem_data = pci_host_data_register_mmio(&s->host_state); > sysbus_init_mmio(dev, 0x1000, pci_mem_config); > -- > 1.6.0.2 > >
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 <agraf@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? :-) > >> + } >> + >> + retval &= ~3u; >> + retval |= (addr & 7); >> + >> + UNIN_DPRINTF("Converted config space accessor %08x/%08x -> %08x\n", >> + reg, addr, retval); >> + >> + return retval; >> +} >> + >> static int pci_unin_main_init_device(SysBusDevice *dev) >> { >> UNINState *s; >> @@ -85,6 +119,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev) >> s = FROM_SYSBUS(UNINState, dev); >> >> pci_host_init(&s->host_state); >> + s->host_state.get_config_reg = unin_get_config_reg; > > This looks slightly ugly: let's make pci_host_init accept > the conversion function instead? Hmm. My guess was that 99% of the host adapters don't need a replacement function, so I wanted to keep the defaults simple. If we later on add additional helpers, that would also be easier, as we wouldn't need to touch every initializer call but only the overriding ones. Alex
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 <agraf@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. > > > >> + } > >> + > >> + retval &= ~3u; > >> + retval |= (addr & 7); > >> + > >> + UNIN_DPRINTF("Converted config space accessor %08x/%08x -> %08x\n", > >> + reg, addr, retval); > >> + > >> + return retval; > >> +} > >> + > >> static int pci_unin_main_init_device(SysBusDevice *dev) > >> { > >> UNINState *s; > >> @@ -85,6 +119,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev) > >> s = FROM_SYSBUS(UNINState, dev); > >> > >> pci_host_init(&s->host_state); > >> + s->host_state.get_config_reg = unin_get_config_reg; > > > > This looks slightly ugly: let's make pci_host_init accept > > the conversion function instead? > > Hmm. My guess was that 99% of the host adapters don't need a replacement function, so I wanted to keep the defaults simple. If we later on add additional helpers, that would also be easier, as we wouldn't need to touch every initializer call but only the overriding ones. > > > Alex
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 <agraf@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? :-) > > > > >> + } > >> + > >> + retval &= ~3u; > >> + retval |= (addr & 7); > >> + > >> + UNIN_DPRINTF("Converted config space accessor %08x/%08x -> %08x\n", > >> + reg, addr, retval); > >> + > >> + return retval; > >> +} > >> + > >> static int pci_unin_main_init_device(SysBusDevice *dev) > >> { > >> UNINState *s; > >> @@ -85,6 +119,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev) > >> s = FROM_SYSBUS(UNINState, dev); > >> > >> pci_host_init(&s->host_state); > >> + s->host_state.get_config_reg = unin_get_config_reg; > > > > This looks slightly ugly: let's make pci_host_init accept > > the conversion function instead? > > Hmm. My guess was that 99% of the host adapters don't need a replacement function, so I wanted to keep the defaults simple. If we later on add additional helpers, that would also be easier, as we wouldn't need to touch every initializer call but only the overriding ones. > OK. > Alex
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 <agraf@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
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 <agraf@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".
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 <agraf@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
On Sun, Jan 03, 2010 at 05:35:01PM +0100, Alexander Graf wrote: > > 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 <agraf@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 Yes.
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); + } + + retval &= ~3u; + retval |= (addr & 7); + + UNIN_DPRINTF("Converted config space accessor %08x/%08x -> %08x\n", + reg, addr, retval); + + return retval; +} + static int pci_unin_main_init_device(SysBusDevice *dev) { UNINState *s; @@ -85,6 +119,7 @@ static int pci_unin_main_init_device(SysBusDevice *dev) s = FROM_SYSBUS(UNINState, dev); pci_host_init(&s->host_state); + s->host_state.get_config_reg = unin_get_config_reg; pci_mem_config = pci_host_conf_register_mmio(&s->host_state); pci_mem_data = pci_host_data_register_mmio(&s->host_state); sysbus_init_mmio(dev, 0x1000, pci_mem_config);
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 <agraf@suse.de> --- hw/unin_pci.c | 35 +++++++++++++++++++++++++++++++++++ 1 files changed, 35 insertions(+), 0 deletions(-)