Skip to contentSkip to conversation

ppc440_pcix: Add dummy implementation of BRDGOPT registers

PATCH In review BALATON Zoltan 3 participants 4 messages Mar 7, 2018 – Mar 8, 2018

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 ppc440_pcix: Add dummy implementation of BRDGOPT registers

I don't know what should be the correct implementation for these so these are just stored and returned as is without doing anything for now only to silence warnings when u-boot accesses these registers.

---
 hw/ppc/ppc440_pcix.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
hw/ppc/ppc440_pcix.c +18 -0
index ab2626a..3f177d3 100644
@@ -55,6 +55,8 @@ typedef struct PPC440PCIXState {
     PCIDevice *dev;
     struct PLBOutMap pom[PPC440_PCIX_NR_POMS];
     struct PLBInMap pim[PPC440_PCIX_NR_PIMS];
+    uint32_t brdgopt1;
+    uint32_t brdgopt2;
     uint32_t sts;
     qemu_irq irq[PCI_NUM_PINS];
     AddressSpace bm_as;
@@ -95,6 +97,8 @@ typedef struct PPC440PCIXState {
 #define PCIX0_PIM0SAH       0xf8
 #define PCIX0_PIM2SAH       0xfc
 
+#define PCIX0_BRDGOPT1      0x40
+#define PCIX0_BRDGOPT2      0x44
 #define PCIX0_STS           0xe0
 
 #define PCI_ALL_SIZE        (PPC440_REG_BASE + PPC440_REG_SIZE)
@@ -270,6 +274,12 @@ static void ppc440_pcix_reg_write4(void *opaque, hwaddr addr,
         ppc440_pcix_update_pim(s, 2);
         break;
 
+    case PCIX0_BRDGOPT1:
+        s->brdgopt1 = val;
+        break;
+    case PCIX0_BRDGOPT2:
+        s->brdgopt2 = val;
+        break;
     case PCIX0_STS:
         s->sts = val;
         break;
@@ -365,6 +375,12 @@ static uint64_t ppc440_pcix_reg_read4(void *opaque, hwaddr addr,
         val = s->pim[2].la >> 32;
         break;
 
+    case PCIX0_BRDGOPT1:
+        val = s->brdgopt1;
+        break;
+    case PCIX0_BRDGOPT2:
+        val = s->brdgopt2;
+        break;
     case PCIX0_STS:
         val = s->sts;
         break;
@@ -408,6 +424,8 @@ static void ppc440_pcix_reset(DeviceState *dev)
     for (i = 0; i < PPC440_PCIX_NR_PIMS; i++) {
         s->pim[i].sa = 0xffffffff00000000ULL;
     }
+    s->brdgopt1 = 0;
+    s->brdgopt2 = 0;
     s->sts = 0;
 }
 
Signature · 1 line
2.7.6
Reviewed-by on v1 1/1
#1 lore raw
Quoted 58 lines · BALATON Zoltan

I don't know what should be the correct implementation for these so these are just stored and returned as is without doing anything for now only to silence warnings when u-boot accesses these registers.

---
 hw/ppc/ppc440_pcix.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
hw/ppc/ppc440_pcix.c +16 -0
index ab2626a..3f177d3 100644
@@ -55,6 +55,8 @@ typedef struct PPC440PCIXState {
     PCIDevice *dev;
     struct PLBOutMap pom[PPC440_PCIX_NR_POMS];
     struct PLBInMap pim[PPC440_PCIX_NR_PIMS];
+    uint32_t brdgopt1;
+    uint32_t brdgopt2;
     uint32_t sts;
     qemu_irq irq[PCI_NUM_PINS];
     AddressSpace bm_as;
@@ -95,6 +97,8 @@ typedef struct PPC440PCIXState {
 #define PCIX0_PIM0SAH       0xf8
 #define PCIX0_PIM2SAH       0xfc
 
+#define PCIX0_BRDGOPT1      0x40
+#define PCIX0_BRDGOPT2      0x44
 #define PCIX0_STS           0xe0
 
 #define PCI_ALL_SIZE        (PPC440_REG_BASE + PPC440_REG_SIZE)
@@ -270,6 +274,12 @@ static void ppc440_pcix_reg_write4(void *opaque, hwaddr addr,
         ppc440_pcix_update_pim(s, 2);
         break;
 
+    case PCIX0_BRDGOPT1:
+        s->brdgopt1 = val;
+        break;
+    case PCIX0_BRDGOPT2:
+        s->brdgopt2 = val;
+        break;
     case PCIX0_STS:
         s->sts = val;
         break;
@@ -365,6 +375,12 @@ static uint64_t ppc440_pcix_reg_read4(void *opaque, hwaddr addr,
         val = s->pim[2].la >> 32;
         break;
 
+    case PCIX0_BRDGOPT1:
+        val = s->brdgopt1;
+        break;
+    case PCIX0_BRDGOPT2:
+        val = s->brdgopt2;
+        break;
     case PCIX0_STS:
         val = s->sts;
         break;

hw/ppc/ppc440_pcix.c static void ppc440_pcix_reset(DeviceState *dev) · line 430

Quoted 7 lines
hw/ppc/ppc440_pcix.c +2 -0
@@ -408,6 +424,8 @@ static void ppc440_pcix_reset(DeviceState *dev)
     for (i = 0; i < PPC440_PCIX_NR_PIMS; i++) {
         s->pim[i].sa = 0xffffffff00000000ULL;
     }
+    s->brdgopt1 = 0;
+    s->brdgopt2 = 0;
     s->sts = 0;
 }

Looks sane, so:

... but I wonder whether the error_report() in the "default:" case should maybe rather be turned into a qemu_log_mask(LOG_UNIMP, ...) instead?

Thomas

#2 lore raw
In reply to BALATON Zoltan
Quoted 5 lines · BALATON Zoltan

I don't know what should be the correct implementation for these so these are just stored and returned as is without doing anything for now only to silence warnings when u-boot accesses these registers.

I'm a bit dubious about putting in a dummy register implementation. Specifically, I think the case needs to be made that the dummy implementation is preferable to just putting up with the test errors.

At the very least there should be a comment in the code indicating that it's just a dummy stub implementation.

Quoted 61 lines
---
 hw/ppc/ppc440_pcix.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
hw/ppc/ppc440_pcix.c +18 -0
index ab2626a..3f177d3 100644
@@ -55,6 +55,8 @@ typedef struct PPC440PCIXState {
     PCIDevice *dev;
     struct PLBOutMap pom[PPC440_PCIX_NR_POMS];
     struct PLBInMap pim[PPC440_PCIX_NR_PIMS];
+    uint32_t brdgopt1;
+    uint32_t brdgopt2;
     uint32_t sts;
     qemu_irq irq[PCI_NUM_PINS];
     AddressSpace bm_as;
@@ -95,6 +97,8 @@ typedef struct PPC440PCIXState {
 #define PCIX0_PIM0SAH       0xf8
 #define PCIX0_PIM2SAH       0xfc
 
+#define PCIX0_BRDGOPT1      0x40
+#define PCIX0_BRDGOPT2      0x44
 #define PCIX0_STS           0xe0
 
 #define PCI_ALL_SIZE        (PPC440_REG_BASE + PPC440_REG_SIZE)
@@ -270,6 +274,12 @@ static void ppc440_pcix_reg_write4(void *opaque, hwaddr addr,
         ppc440_pcix_update_pim(s, 2);
         break;
 
+    case PCIX0_BRDGOPT1:
+        s->brdgopt1 = val;
+        break;
+    case PCIX0_BRDGOPT2:
+        s->brdgopt2 = val;
+        break;
     case PCIX0_STS:
         s->sts = val;
         break;
@@ -365,6 +375,12 @@ static uint64_t ppc440_pcix_reg_read4(void *opaque, hwaddr addr,
         val = s->pim[2].la >> 32;
         break;
 
+    case PCIX0_BRDGOPT1:
+        val = s->brdgopt1;
+        break;
+    case PCIX0_BRDGOPT2:
+        val = s->brdgopt2;
+        break;
     case PCIX0_STS:
         val = s->sts;
         break;
@@ -408,6 +424,8 @@ static void ppc440_pcix_reset(DeviceState *dev)
     for (i = 0; i < PPC440_PCIX_NR_PIMS; i++) {
         s->pim[i].sa = 0xffffffff00000000ULL;
     }
+    s->brdgopt1 = 0;
+    s->brdgopt2 = 0;
     s->sts = 0;
 }
 
Signature · 4 lines
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson
#3 lore raw
Quoted 5 lines · David Gibson
Quoted 4 lines · BALATON Zoltan

I don't know what should be the correct implementation for these so these are just stored and returned as is without doing anything for now only to silence warnings when u-boot accesses these registers.

Quoted 8 lines
Quoted 1 line

I'm a bit dubious about putting in a dummy register implementation. Specifically, I think the case needs to be made that the dummy implementation is preferable to just putting up with the test errors.

At the very least there should be a comment in the code indicating that it's just a dummy stub implementation.

Let's go with changing the error_report to qemu_log_mask(LOG_UNIMP, ...) as suggested by Thomas instead. I'll send another patch.

Regards,
BALATON Zoltan