Message ID | 27c47334bab155b7219a6fcc4547701e2a6e2060.1415091929.git.hutao@cn.fujitsu.com |
---|---|
State | New |
Headers | show |
Hi, On Tue, 2014-11-04 at 17:12 +0800, Hu Tao wrote: > This makes code more readable. > > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> > --- > hw/mips/gt64xxx_pci.c | 4 ++-- > hw/pci/pci_host.c | 5 +++-- > include/hw/pci/pci.h | 2 ++ > 3 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c > index 1f2fe5f..a49dbd7 100644 > --- a/hw/mips/gt64xxx_pci.c > +++ b/hw/mips/gt64xxx_pci.c > @@ -564,7 +564,7 @@ static void gt64120_writel (void *opaque, hwaddr addr, > if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) { > val = bswap32(val); > } > - if (phb->config_reg & (1u << 31)) { > + if (PC_PCI_CONFIG_ENABLED(phb->config_reg)) I really like readable MACROS instead of magic numbers, however I have 3 suggestions: 1. PC_PCI_CONFIG_ENABLED is still not really clear, because of the "PC" prefix that is too wide in my IMHO (maybe PCI_HOST_BRIDGE_CONFIG_ENABLED) when this macro refers only to host bridges. 2. Maybe go the "extra mile" and let the macro receive a host bridge as parameter PCI_HOST_BRIDGE_CONFIG_ENABLED(host_bridge). 3. Maybe make it an inline function? Just wondering > pci_data_write(phb->bus, phb->config_reg, val, 4); > } > break; > @@ -804,7 +804,7 @@ static uint64_t gt64120_readl (void *opaque, > val = phb->config_reg; > break; > case GT_PCI0_CFGDATA: > - if (!(phb->config_reg & (1 << 31))) { > + if (!PC_PCI_CONFIG_ENABLED(phb->config_reg)) { > val = 0xffffffff; > } else { > val = pci_data_read(phb->bus, phb->config_reg, 4); > diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c > index 3e26f92..f2a69ea 100644 > --- a/hw/pci/pci_host.c > +++ b/hw/pci/pci_host.c > @@ -133,8 +133,9 @@ static void pci_host_data_write(void *opaque, hwaddr addr, > PCIHostState *s = opaque; > PCI_DPRINTF("write addr " TARGET_FMT_plx " len %d val %x\n", > addr, len, (unsigned)val); > - if (s->config_reg & (1u << 31)) > + if (PC_PCI_CONFIG_ENABLED(s->config_reg)) { > pci_data_write(s->bus, s->config_reg | (addr & 3), val, len); > + } > } > > static uint64_t pci_host_data_read(void *opaque, > @@ -142,7 +143,7 @@ static uint64_t pci_host_data_read(void *opaque, > { > PCIHostState *s = opaque; > uint32_t val; > - if (!(s->config_reg & (1U << 31))) { > + if (!PC_PCI_CONFIG_ENABLED(s->config_reg)) { > return 0xffffffff; > } > val = pci_data_read(s->bus, s->config_reg | (addr & 3), len); > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > index c352c7b..3d42d7f 100644 > --- a/include/hw/pci/pci.h > +++ b/include/hw/pci/pci.h > @@ -13,6 +13,8 @@ > > #include "hw/pci/pcie.h" > > +#define PC_PCI_CONFIG_ENABLED(addr) (addr & (1U << 31)) Again, maybe move this to "hw/pci/pci_host" since is specific to host bridges? Thanks, Marcel > + > /* PCI bus */ > > #define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07))
On Tue, Nov 04, 2014 at 03:41:11PM +0200, Marcel Apfelbaum wrote: > Hi, > > On Tue, 2014-11-04 at 17:12 +0800, Hu Tao wrote: > > This makes code more readable. > > > > Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> > > --- > > hw/mips/gt64xxx_pci.c | 4 ++-- > > hw/pci/pci_host.c | 5 +++-- > > include/hw/pci/pci.h | 2 ++ > > 3 files changed, 7 insertions(+), 4 deletions(-) > > > > diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c > > index 1f2fe5f..a49dbd7 100644 > > --- a/hw/mips/gt64xxx_pci.c > > +++ b/hw/mips/gt64xxx_pci.c > > @@ -564,7 +564,7 @@ static void gt64120_writel (void *opaque, hwaddr addr, > > if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) { > > val = bswap32(val); > > } > > - if (phb->config_reg & (1u << 31)) { > > + if (PC_PCI_CONFIG_ENABLED(phb->config_reg)) > I really like readable MACROS instead of magic numbers, > however I have 3 suggestions: > 1. PC_PCI_CONFIG_ENABLED is still not really clear, because > of the "PC" prefix that is too wide in my IMHO (maybe PCI_HOST_BRIDGE_CONFIG_ENABLED) > when this macro refers only to host bridges. > 2. Maybe go the "extra mile" and let the macro receive > a host bridge as parameter PCI_HOST_BRIDGE_CONFIG_ENABLED(host_bridge). Sounds reasonable. I changed it to an inline function and prefixed it with pci_host_ to follow the convention in pci_host.c. > 3. Maybe make it an inline function? Just wondering > > > pci_data_write(phb->bus, phb->config_reg, val, 4); > > } > > break; > > @@ -804,7 +804,7 @@ static uint64_t gt64120_readl (void *opaque, > > val = phb->config_reg; > > break; > > case GT_PCI0_CFGDATA: > > - if (!(phb->config_reg & (1 << 31))) { > > + if (!PC_PCI_CONFIG_ENABLED(phb->config_reg)) { > > val = 0xffffffff; > > } else { > > val = pci_data_read(phb->bus, phb->config_reg, 4); > > diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c > > index 3e26f92..f2a69ea 100644 > > --- a/hw/pci/pci_host.c > > +++ b/hw/pci/pci_host.c > > @@ -133,8 +133,9 @@ static void pci_host_data_write(void *opaque, hwaddr addr, > > PCIHostState *s = opaque; > > PCI_DPRINTF("write addr " TARGET_FMT_plx " len %d val %x\n", > > addr, len, (unsigned)val); > > - if (s->config_reg & (1u << 31)) > > + if (PC_PCI_CONFIG_ENABLED(s->config_reg)) { > > pci_data_write(s->bus, s->config_reg | (addr & 3), val, len); > > + } > > } > > > > static uint64_t pci_host_data_read(void *opaque, > > @@ -142,7 +143,7 @@ static uint64_t pci_host_data_read(void *opaque, > > { > > PCIHostState *s = opaque; > > uint32_t val; > > - if (!(s->config_reg & (1U << 31))) { > > + if (!PC_PCI_CONFIG_ENABLED(s->config_reg)) { > > return 0xffffffff; > > } > > val = pci_data_read(s->bus, s->config_reg | (addr & 3), len); > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h > > index c352c7b..3d42d7f 100644 > > --- a/include/hw/pci/pci.h > > +++ b/include/hw/pci/pci.h > > @@ -13,6 +13,8 @@ > > > > #include "hw/pci/pcie.h" > > > > +#define PC_PCI_CONFIG_ENABLED(addr) (addr & (1U << 31)) > Again, maybe move this to "hw/pci/pci_host" since is specific to host bridges? Done. Thanks! > > Thanks, > Marcel > > + > > /* PCI bus */ > > > > #define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07)) > > >
diff --git a/hw/mips/gt64xxx_pci.c b/hw/mips/gt64xxx_pci.c index 1f2fe5f..a49dbd7 100644 --- a/hw/mips/gt64xxx_pci.c +++ b/hw/mips/gt64xxx_pci.c @@ -564,7 +564,7 @@ static void gt64120_writel (void *opaque, hwaddr addr, if (!(s->regs[GT_PCI0_CMD] & 1) && (phb->config_reg & 0x00fff800)) { val = bswap32(val); } - if (phb->config_reg & (1u << 31)) { + if (PC_PCI_CONFIG_ENABLED(phb->config_reg)) { pci_data_write(phb->bus, phb->config_reg, val, 4); } break; @@ -804,7 +804,7 @@ static uint64_t gt64120_readl (void *opaque, val = phb->config_reg; break; case GT_PCI0_CFGDATA: - if (!(phb->config_reg & (1 << 31))) { + if (!PC_PCI_CONFIG_ENABLED(phb->config_reg)) { val = 0xffffffff; } else { val = pci_data_read(phb->bus, phb->config_reg, 4); diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c index 3e26f92..f2a69ea 100644 --- a/hw/pci/pci_host.c +++ b/hw/pci/pci_host.c @@ -133,8 +133,9 @@ static void pci_host_data_write(void *opaque, hwaddr addr, PCIHostState *s = opaque; PCI_DPRINTF("write addr " TARGET_FMT_plx " len %d val %x\n", addr, len, (unsigned)val); - if (s->config_reg & (1u << 31)) + if (PC_PCI_CONFIG_ENABLED(s->config_reg)) { pci_data_write(s->bus, s->config_reg | (addr & 3), val, len); + } } static uint64_t pci_host_data_read(void *opaque, @@ -142,7 +143,7 @@ static uint64_t pci_host_data_read(void *opaque, { PCIHostState *s = opaque; uint32_t val; - if (!(s->config_reg & (1U << 31))) { + if (!PC_PCI_CONFIG_ENABLED(s->config_reg)) { return 0xffffffff; } val = pci_data_read(s->bus, s->config_reg | (addr & 3), len); diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h index c352c7b..3d42d7f 100644 --- a/include/hw/pci/pci.h +++ b/include/hw/pci/pci.h @@ -13,6 +13,8 @@ #include "hw/pci/pcie.h" +#define PC_PCI_CONFIG_ENABLED(addr) (addr & (1U << 31)) + /* PCI bus */ #define PCI_DEVFN(slot, func) ((((slot) & 0x1f) << 3) | ((func) & 0x07))
This makes code more readable. Signed-off-by: Hu Tao <hutao@cn.fujitsu.com> --- hw/mips/gt64xxx_pci.c | 4 ++-- hw/pci/pci_host.c | 5 +++-- include/hw/pci/pci.h | 2 ++ 3 files changed, 7 insertions(+), 4 deletions(-)