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.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ppc/ppc440_pcix.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

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 <balaton@eik.bme.hu>
> ---
>  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 = 0xffffffff00000000ULL;
>      }
> +    s->brdgopt1 = 0;
> +    s->brdgopt2 = 0;
>      s->sts = 0;
>  }

Looks sane, so:

Reviewed-by: Thomas Huth <thuth@redhat.com>

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

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 <balaton@eik.bme.hu>

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 = 0xffffffff00000000ULL;
>      }
> +    s->brdgopt1 = 0;
> +    s->brdgopt2 = 0;
>      s->sts = 0;
>  }
>

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 <balaton@eik.bme.hu>
>
> 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