Re: [Qemu-devel] [PATCH] ppc440_pcix: Add dummy implementation of BRDGOPT registers

2018-03-08 Thread BALATON Zoltan 

On Thu, 8 Mar 2018, David Gibson wrote:

On Wed, Mar 07, 2018 at 09:43:59PM +0100, BALATON Zoltan wrote:

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.

Signed-off-by: BALATON Zoltan 


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

Re: [Qemu-devel] [PATCH] ppc440_pcix: Add dummy implementation of BRDGOPT registers

2018-03-08 Thread David Gibson 
On Wed, Mar 07, 2018 at 09:43:59PM +0100, BALATON Zoltan wrote:
> 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.
> 
> Signed-off-by: BALATON Zoltan 

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.

> ---
>  hw/ppc/ppc440_pcix.c | 18 ++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/hw/ppc/ppc440_pcix.c b/hw/ppc/ppc440_pcix.c
> index ab2626a..3f177d3 100644
> --- a/hw/ppc/ppc440_pcix.c
> +++ b/hw/ppc/ppc440_pcix.c
> @@ -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 = 0xULL;
>  }
> +s->brdgopt1 = 0;
> +s->brdgopt2 = 0;
>  s->sts = 0;
>  }
>  

-- 
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


signature.asc
Description: PGP signature

Re: [Qemu-devel] [PATCH] ppc440_pcix: Add dummy implementation of BRDGOPT registers

2018-03-07 Thread Thomas Huth 
On 07.03.2018 21:43, BALATON Zoltan wrote:
> 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.
> 
> Signed-off-by: BALATON Zoltan 
> ---
>  hw/ppc/ppc440_pcix.c | 18 ++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/hw/ppc/ppc440_pcix.c b/hw/ppc/ppc440_pcix.c
> index ab2626a..3f177d3 100644
> --- a/hw/ppc/ppc440_pcix.c
> +++ b/hw/ppc/ppc440_pcix.c
> @@ -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 = 0xULL;
>  }
> +s->brdgopt1 = 0;
> +s->brdgopt2 = 0;
>  s->sts = 0;
>  }

Looks sane, so:

Reviewed-by: Thomas Huth 

... 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

[Qemu-devel] [PATCH] ppc440_pcix: Add dummy implementation of BRDGOPT registers

2018-03-07 Thread 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.

Signed-off-by: BALATON Zoltan 
---
 hw/ppc/ppc440_pcix.c | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/hw/ppc/ppc440_pcix.c b/hw/ppc/ppc440_pcix.c
index ab2626a..3f177d3 100644
--- a/hw/ppc/ppc440_pcix.c
+++ b/hw/ppc/ppc440_pcix.c
@@ -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 = 0xULL;
 }
+s->brdgopt1 = 0;
+s->brdgopt2 = 0;
 s->sts = 0;
 }
 
-- 
2.7.6