[PATCH 0/5] ppc/pnv: fix Homer/Occ mappings on multichip systems

[PATCH 0/5] ppc/pnv: fix Homer/Occ mappings on multichip systems

[Balamuruhan S]

Hi All,

PowerNV fails to boot in multichip systems due to some misinterpretation
and mapping in Homer/Occ device models, this patchset fixes the
following,

 - Homer size is 4MB per chip and Occ common area size is 8MB
 - Bar masks are used to calculate sizes of Homer/Occ in skiboot so
   return appropriate value
 - Occ common area is in BAR 3 on Power8 but wrongly mapped to BAR 2
   currently
 - OCC common area is shared across chips and should be mapped only once
   for multichip systems

Request for your review and suggestions to make it better. I would like to
thank Cedric for his time and help to figure out the issues.

Balamuruhan S (5):
  hw/ppc/pnv: incorrect homer and occ common area size
  hw/ppc/pnv_xscom: PBA bar mask values are incorrect with homer/occ
    sizes
  hw/ppc/pnv_xscom: Power8 occ common area is in PBA BAR 3
  hw/ppc/pnv_xscom: occ common area to be mapped only once
  hw/ppc/pnv_xscom: add PBA BARs for Power8 slw image

 hw/ppc/pnv_occ.c     |  2 +-
 hw/ppc/pnv_xscom.c   | 37 +++++++++++++++++++++++++++----------
 include/hw/ppc/pnv.h | 12 ++++++++----
 3 files changed, 36 insertions(+), 15 deletions(-)

-- 
2.14.5

[PATCH 1/5] hw/ppc/pnv: incorrect homer and occ common area size

[Balamuruhan S]

Homer size is 4MB and OCC common area size is 8MB, but currently
it is assigned with bar mask value. Also pass on the occ sram
size 3 bits right shifted to initialize the size appropriately.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
---
 hw/ppc/pnv_occ.c     | 2 +-
 include/hw/ppc/pnv.h | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/ppc/pnv_occ.c b/hw/ppc/pnv_occ.c
index 785653bb67..05c51c9de0 100644
--- a/hw/ppc/pnv_occ.c
+++ b/hw/ppc/pnv_occ.c
@@ -276,7 +276,7 @@ static void pnv_occ_realize(DeviceState *dev, Error **errp)
 
     /* XScom region for OCC SRAM registers */
     pnv_xscom_region_init(&occ->sram_regs, OBJECT(dev), poc->sram_ops,
-                          occ, "occ-common-area", poc->sram_size);
+                          occ, "occ-common-area", poc->sram_size >> 3);
 }
 
 static void pnv_occ_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index 0b4c722e6b..e9ed8b928a 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -203,12 +203,12 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
 #define PNV_XSCOM_BASE(chip)                                            \
     (0x0003fc0000000000ull + ((uint64_t)(chip)->chip_id) * PNV_XSCOM_SIZE)
 
-#define PNV_OCC_COMMON_AREA_SIZE    0x0000000000700000ull
+#define PNV_OCC_COMMON_AREA_SIZE    0x0000000000800000ull
 #define PNV_OCC_COMMON_AREA(chip)                                       \
     (0x7fff800000ull + ((uint64_t)PNV_CHIP_INDEX(chip) * \
                          PNV_OCC_COMMON_AREA_SIZE))
 
-#define PNV_HOMER_SIZE              0x0000000000300000ull
+#define PNV_HOMER_SIZE              0x0000000000400000ull
 #define PNV_HOMER_BASE(chip)                                            \
     (0x7ffd800000ull + ((uint64_t)PNV_CHIP_INDEX(chip)) * PNV_HOMER_SIZE)
 
@@ -271,12 +271,12 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
 #define PNV9_XSCOM_SIZE              0x0000000400000000ull
 #define PNV9_XSCOM_BASE(chip)        PNV9_CHIP_BASE(chip, 0x00603fc00000000ull)
 
-#define PNV9_OCC_COMMON_AREA_SIZE    0x0000000000700000ull
+#define PNV9_OCC_COMMON_AREA_SIZE    0x0000000000800000ull
 #define PNV9_OCC_COMMON_AREA(chip)                                      \
     (0x203fff800000ull + ((uint64_t)PNV_CHIP_INDEX(chip) * \
                            PNV9_OCC_COMMON_AREA_SIZE))
 
-#define PNV9_HOMER_SIZE              0x0000000000300000ull
+#define PNV9_HOMER_SIZE              0x0000000000400000ull
 #define PNV9_HOMER_BASE(chip)                                           \
     (0x203ffd800000ull + ((uint64_t)PNV_CHIP_INDEX(chip)) * PNV9_HOMER_SIZE)
 #endif /* PPC_PNV_H */
-- 
2.14.5

[PATCH 2/5] hw/ppc/pnv_xscom: PBA bar mask values are incorrect with homer/occ sizes

[Balamuruhan S]

homer/occ sizes are calculated in skiboot with `(mask | 0xfffff) + 1`,
and from xscom access should return correct mask values instead of actual
sizes.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
---
 hw/ppc/pnv_xscom.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
index f01d788a65..cdd5fa356e 100644
--- a/hw/ppc/pnv_xscom.c
+++ b/hw/ppc/pnv_xscom.c
@@ -46,6 +46,10 @@
 #define P9_PBA_BARMASK0                 0x5012b04
 #define P9_PBA_BARMASK2                 0x5012b06
 
+/* Mask to calculate Homer/Occ size */
+#define HOMER_SIZE_MASK                 0x0000000000300000ull
+#define OCC_SIZE_MASK                   0x0000000000700000ull
+
 static void xscom_complete(CPUState *cs, uint64_t hmer_bits)
 {
     /*
@@ -90,9 +94,8 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
         return PNV_HOMER_BASE(chip);
 
     case P9_PBA_BARMASK0: /* P9 homer region size */
-        return PNV9_HOMER_SIZE;
     case P8_PBA_BARMASK0: /* P8 homer region size */
-        return PNV_HOMER_SIZE;
+        return HOMER_SIZE_MASK;
 
     case P9_PBA_BAR2: /* P9 occ common area */
         return PNV9_OCC_COMMON_AREA(chip);
@@ -100,9 +103,8 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
         return PNV_OCC_COMMON_AREA(chip);
 
     case P9_PBA_BARMASK2: /* P9 occ common area size */
-        return PNV9_OCC_COMMON_AREA_SIZE;
     case P8_PBA_BARMASK2: /* P8 occ common area size */
-        return PNV_OCC_COMMON_AREA_SIZE;
+        return OCC_SIZE_MASK;
 
     case 0x1010c00:     /* PIBAM FIR */
     case 0x1010c03:     /* PIBAM FIR MASK */
-- 
2.14.5

[PATCH 3/5] hw/ppc/pnv_xscom: Power8 occ common area is in PBA BAR 3

[Balamuruhan S]

Fix incorrect PBA BAR and BARMASK value for Power8 occ common area
region where skiboot enum declaration have it in BAR 3 and BARMASK
is calculated BARMASK0 + BAR,

enum P8_BAR {
        P8_BAR_HOMER = 0,
        P8_BAR_CENTAUR = 1,
        P8_BAR_SLW = 2,
        P8_BAR_OCC_COMMON = 3,
};

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
---
 hw/ppc/pnv_xscom.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
index cdd5fa356e..cb6d6bbcfc 100644
--- a/hw/ppc/pnv_xscom.c
+++ b/hw/ppc/pnv_xscom.c
@@ -38,9 +38,9 @@
 
 /* PBA BARs */
 #define P8_PBA_BAR0                     0x2013f00
-#define P8_PBA_BAR2                     0x2013f02
+#define P8_PBA_BAR3                     0x2013f03
 #define P8_PBA_BARMASK0                 0x2013f04
-#define P8_PBA_BARMASK2                 0x2013f06
+#define P8_PBA_BARMASK3                 0x2013f07
 #define P9_PBA_BAR0                     0x5012b00
 #define P9_PBA_BAR2                     0x5012b02
 #define P9_PBA_BARMASK0                 0x5012b04
@@ -99,11 +99,11 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
 
     case P9_PBA_BAR2: /* P9 occ common area */
         return PNV9_OCC_COMMON_AREA(chip);
-    case P8_PBA_BAR2: /* P8 occ common area */
+    case P8_PBA_BAR3: /* P8 occ common area */
         return PNV_OCC_COMMON_AREA(chip);
 
     case P9_PBA_BARMASK2: /* P9 occ common area size */
-    case P8_PBA_BARMASK2: /* P8 occ common area size */
+    case P8_PBA_BARMASK3: /* P8 occ common area size */
         return OCC_SIZE_MASK;
 
     case 0x1010c00:     /* PIBAM FIR */
@@ -126,9 +126,9 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
     case 0x202000f:     /* ADU stuff, receive status register*/
         return 0;
     case 0x2013f01:     /* PBA stuff */
-    case 0x2013f03:     /* PBA stuff */
+    case 0x2013f02:     /* PBA stuff */
     case 0x2013f05:     /* PBA stuff */
-    case 0x2013f07:     /* PBA stuff */
+    case 0x2013f06:     /* PBA stuff */
         return 0;
     case 0x2013028:     /* CAPP stuff */
     case 0x201302a:     /* CAPP stuff */
-- 
2.14.5

[PATCH 4/5] hw/ppc/pnv_xscom: occ common area to be mapped only once

[Balamuruhan S]

occ common area should be mapped once and disable it for every
other chip.

Signed-off-by: Cédric Le Goater <clg@kaod.org>
Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
---
 hw/ppc/pnv_xscom.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
index cb6d6bbcfc..f797a5ec7d 100644
--- a/hw/ppc/pnv_xscom.c
+++ b/hw/ppc/pnv_xscom.c
@@ -98,13 +98,22 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
         return HOMER_SIZE_MASK;
 
     case P9_PBA_BAR2: /* P9 occ common area */
-        return PNV9_OCC_COMMON_AREA(chip);
+        if (!PNV_CHIP_INDEX(chip)) {
+            return PNV9_OCC_COMMON_AREA(chip);
+        }
+        return 0;
     case P8_PBA_BAR3: /* P8 occ common area */
-        return PNV_OCC_COMMON_AREA(chip);
+        if (!PNV_CHIP_INDEX(chip)) {
+            return PNV_OCC_COMMON_AREA(chip);
+        }
+        return 0;
 
     case P9_PBA_BARMASK2: /* P9 occ common area size */
     case P8_PBA_BARMASK3: /* P8 occ common area size */
-        return OCC_SIZE_MASK;
+        if (!PNV_CHIP_INDEX(chip)) {
+            return OCC_SIZE_MASK;
+        }
+        return 0;
 
     case 0x1010c00:     /* PIBAM FIR */
     case 0x1010c03:     /* PIBAM FIR MASK */
-- 
2.14.5

[PATCH 5/5] hw/ppc/pnv_xscom: add PBA BARs for Power8 slw image

[Balamuruhan S]

slw base and size mask are accessed during boot in homer_init_chip(),
so include BAR2 and BARMASK2 for Power8.

Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
---
 hw/ppc/pnv_xscom.c   | 10 ++++++++--
 include/hw/ppc/pnv.h |  4 ++++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
index f797a5ec7d..828a2e2a5a 100644
--- a/hw/ppc/pnv_xscom.c
+++ b/hw/ppc/pnv_xscom.c
@@ -38,8 +38,10 @@
 
 /* PBA BARs */
 #define P8_PBA_BAR0                     0x2013f00
+#define P8_PBA_BAR2                     0x2013f02
 #define P8_PBA_BAR3                     0x2013f03
 #define P8_PBA_BARMASK0                 0x2013f04
+#define P8_PBA_BARMASK2                 0x2013f06
 #define P8_PBA_BARMASK3                 0x2013f07
 #define P9_PBA_BAR0                     0x5012b00
 #define P9_PBA_BAR2                     0x5012b02
@@ -49,6 +51,7 @@
 /* Mask to calculate Homer/Occ size */
 #define HOMER_SIZE_MASK                 0x0000000000300000ull
 #define OCC_SIZE_MASK                   0x0000000000700000ull
+#define SLW_SIZE_MASK                   0x0
 
 static void xscom_complete(CPUState *cs, uint64_t hmer_bits)
 {
@@ -115,6 +118,11 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
         }
         return 0;
 
+    case P8_PBA_BAR2: /* P8 slw image */
+        return PNV_SLW_IMAGE_BASE(chip);
+    case P8_PBA_BARMASK2: /* P8 slw image size is 1MB and mask is zero*/
+        return SLW_SIZE_MASK;
+
     case 0x1010c00:     /* PIBAM FIR */
     case 0x1010c03:     /* PIBAM FIR MASK */
 
@@ -135,9 +143,7 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
     case 0x202000f:     /* ADU stuff, receive status register*/
         return 0;
     case 0x2013f01:     /* PBA stuff */
-    case 0x2013f02:     /* PBA stuff */
     case 0x2013f05:     /* PBA stuff */
-    case 0x2013f06:     /* PBA stuff */
         return 0;
     case 0x2013028:     /* CAPP stuff */
     case 0x201302a:     /* CAPP stuff */
diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
index e9ed8b928a..bd22dbf8a9 100644
--- a/include/hw/ppc/pnv.h
+++ b/include/hw/ppc/pnv.h
@@ -212,6 +212,10 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
 #define PNV_HOMER_BASE(chip)                                            \
     (0x7ffd800000ull + ((uint64_t)PNV_CHIP_INDEX(chip)) * PNV_HOMER_SIZE)
 
+#define PNV_SLW_SIZE                0x0000000000100000ull
+#define PNV_SLW_IMAGE_BASE(chip)                                        \
+    (0x2ffda00000ull + ((uint64_t)PNV_CHIP_INDEX(chip)) * PNV_SLW_SIZE)
+
 
 /*
  * XSCOM 0x20109CA defines the ICP BAR:
-- 
2.14.5

Re: [PATCH 2/5] hw/ppc/pnv_xscom: PBA bar mask values are incorrect with homer/occ sizes

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2278 bytes --]

On Tue, Nov 19, 2019 at 11:20:53PM +0530, Balamuruhan S wrote:
> homer/occ sizes are calculated in skiboot with `(mask | 0xfffff) + 1`,
> and from xscom access should return correct mask values instead of actual
> sizes.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
> ---
>  hw/ppc/pnv_xscom.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> index f01d788a65..cdd5fa356e 100644
> --- a/hw/ppc/pnv_xscom.c
> +++ b/hw/ppc/pnv_xscom.c
> @@ -46,6 +46,10 @@
>  #define P9_PBA_BARMASK0                 0x5012b04
>  #define P9_PBA_BARMASK2                 0x5012b06
>  
> +/* Mask to calculate Homer/Occ size */
> +#define HOMER_SIZE_MASK                 0x0000000000300000ull
> +#define OCC_SIZE_MASK                   0x0000000000700000ull

Uuuhhhhh... AFAICT these defines have identical values to
PNV_HOMER_SIZE and PNV_OCC_COMMON_AREA_SIZE, so I don't see what this
patch is actually changing.


>  static void xscom_complete(CPUState *cs, uint64_t hmer_bits)
>  {
>      /*
> @@ -90,9 +94,8 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
>          return PNV_HOMER_BASE(chip);
>  
>      case P9_PBA_BARMASK0: /* P9 homer region size */
> -        return PNV9_HOMER_SIZE;
>      case P8_PBA_BARMASK0: /* P8 homer region size */
> -        return PNV_HOMER_SIZE;
> +        return HOMER_SIZE_MASK;
>  
>      case P9_PBA_BAR2: /* P9 occ common area */
>          return PNV9_OCC_COMMON_AREA(chip);
> @@ -100,9 +103,8 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
>          return PNV_OCC_COMMON_AREA(chip);
>  
>      case P9_PBA_BARMASK2: /* P9 occ common area size */
> -        return PNV9_OCC_COMMON_AREA_SIZE;
>      case P8_PBA_BARMASK2: /* P8 occ common area size */
> -        return PNV_OCC_COMMON_AREA_SIZE;
> +        return OCC_SIZE_MASK;
>  
>      case 0x1010c00:     /* PIBAM FIR */
>      case 0x1010c03:     /* PIBAM FIR MASK */

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/5] hw/ppc/pnv_xscom: PBA bar mask values are incorrect with homer/occ sizes

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2618 bytes --]

On Wed, Nov 20, 2019 at 08:56:18AM +1100, David Gibson wrote:
> On Tue, Nov 19, 2019 at 11:20:53PM +0530, Balamuruhan S wrote:
> > homer/occ sizes are calculated in skiboot with `(mask | 0xfffff) + 1`,
> > and from xscom access should return correct mask values instead of actual
> > sizes.
> > 
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
> > ---
> >  hw/ppc/pnv_xscom.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> > index f01d788a65..cdd5fa356e 100644
> > --- a/hw/ppc/pnv_xscom.c
> > +++ b/hw/ppc/pnv_xscom.c
> > @@ -46,6 +46,10 @@
> >  #define P9_PBA_BARMASK0                 0x5012b04
> >  #define P9_PBA_BARMASK2                 0x5012b06
> >  
> > +/* Mask to calculate Homer/Occ size */
> > +#define HOMER_SIZE_MASK                 0x0000000000300000ull
> > +#define OCC_SIZE_MASK                   0x0000000000700000ull
> 
> Uuuhhhhh... AFAICT these defines have identical values to
> PNV_HOMER_SIZE and PNV_OCC_COMMON_AREA_SIZE, so I don't see what this
> patch is actually changing.

Oh, sorry, missed that the values were changed in 1/5.  Would have
been easier to follow if the two patches were folded together, but
never mind.  Applied.

> 
> 
> >  static void xscom_complete(CPUState *cs, uint64_t hmer_bits)
> >  {
> >      /*
> > @@ -90,9 +94,8 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
> >          return PNV_HOMER_BASE(chip);
> >  
> >      case P9_PBA_BARMASK0: /* P9 homer region size */
> > -        return PNV9_HOMER_SIZE;
> >      case P8_PBA_BARMASK0: /* P8 homer region size */
> > -        return PNV_HOMER_SIZE;
> > +        return HOMER_SIZE_MASK;
> >  
> >      case P9_PBA_BAR2: /* P9 occ common area */
> >          return PNV9_OCC_COMMON_AREA(chip);
> > @@ -100,9 +103,8 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
> >          return PNV_OCC_COMMON_AREA(chip);
> >  
> >      case P9_PBA_BARMASK2: /* P9 occ common area size */
> > -        return PNV9_OCC_COMMON_AREA_SIZE;
> >      case P8_PBA_BARMASK2: /* P8 occ common area size */
> > -        return PNV_OCC_COMMON_AREA_SIZE;
> > +        return OCC_SIZE_MASK;
> >  
> >      case 0x1010c00:     /* PIBAM FIR */
> >      case 0x1010c03:     /* PIBAM FIR MASK */
> 



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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/5] hw/ppc/pnv_xscom: PBA bar mask values are incorrect with homer/occ sizes

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1804 bytes --]

On Wed, Nov 20, 2019 at 09:00:32AM +1100, David Gibson wrote:
> On Wed, Nov 20, 2019 at 08:56:18AM +1100, David Gibson wrote:
> > On Tue, Nov 19, 2019 at 11:20:53PM +0530, Balamuruhan S wrote:
> > > homer/occ sizes are calculated in skiboot with `(mask | 0xfffff) + 1`,
> > > and from xscom access should return correct mask values instead of actual
> > > sizes.
> > > 
> > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
> > > ---
> > >  hw/ppc/pnv_xscom.c | 10 ++++++----
> > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> > > index f01d788a65..cdd5fa356e 100644
> > > --- a/hw/ppc/pnv_xscom.c
> > > +++ b/hw/ppc/pnv_xscom.c
> > > @@ -46,6 +46,10 @@
> > >  #define P9_PBA_BARMASK0                 0x5012b04
> > >  #define P9_PBA_BARMASK2                 0x5012b06
> > >  
> > > +/* Mask to calculate Homer/Occ size */
> > > +#define HOMER_SIZE_MASK                 0x0000000000300000ull
> > > +#define OCC_SIZE_MASK                   0x0000000000700000ull
> > 
> > Uuuhhhhh... AFAICT these defines have identical values to
> > PNV_HOMER_SIZE and PNV_OCC_COMMON_AREA_SIZE, so I don't see what this
> > patch is actually changing.
> 
> Oh, sorry, missed that the values were changed in 1/5.  Would have
> been easier to follow if the two patches were folded together, but
> never mind.  Applied.

Ugh.. or not.  The first patch doesn't apply for me, looks like it
needs a rebase (or you have something else in your tree I don't know
about).

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/5] hw/ppc/pnv_xscom: PBA bar mask values are incorrect with homer/occ sizes

[Balamuruhan S]

On Wed, Nov 20, 2019 at 09:02:26AM +1100, David Gibson wrote:
> On Wed, Nov 20, 2019 at 09:00:32AM +1100, David Gibson wrote:
> > On Wed, Nov 20, 2019 at 08:56:18AM +1100, David Gibson wrote:
> > > On Tue, Nov 19, 2019 at 11:20:53PM +0530, Balamuruhan S wrote:
> > > > homer/occ sizes are calculated in skiboot with `(mask | 0xfffff) + 1`,
> > > > and from xscom access should return correct mask values instead of actual
> > > > sizes.
> > > > 
> > > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > > Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
> > > > ---
> > > >  hw/ppc/pnv_xscom.c | 10 ++++++----
> > > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> > > > index f01d788a65..cdd5fa356e 100644
> > > > --- a/hw/ppc/pnv_xscom.c
> > > > +++ b/hw/ppc/pnv_xscom.c
> > > > @@ -46,6 +46,10 @@
> > > >  #define P9_PBA_BARMASK0                 0x5012b04
> > > >  #define P9_PBA_BARMASK2                 0x5012b06
> > > >  
> > > > +/* Mask to calculate Homer/Occ size */
> > > > +#define HOMER_SIZE_MASK                 0x0000000000300000ull
> > > > +#define OCC_SIZE_MASK                   0x0000000000700000ull
> > > 
> > > Uuuhhhhh... AFAICT these defines have identical values to
> > > PNV_HOMER_SIZE and PNV_OCC_COMMON_AREA_SIZE, so I don't see what this
> > > patch is actually changing.
> > 
> > Oh, sorry, missed that the values were changed in 1/5.  Would have
> > been easier to follow if the two patches were folded together, but
> > never mind.  Applied.
> 
> Ugh.. or not.  The first patch doesn't apply for me, looks like it
> needs a rebase (or you have something else in your tree I don't know
> about).

I checked out from upstream master (commit: 8937f17249) and worked on
it.

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

Re: [PATCH 2/5] hw/ppc/pnv_xscom: PBA bar mask values are incorrect with homer/occ sizes

[Balamuruhan S]

On Wed, Nov 20, 2019 at 08:31:03AM +0530, Balamuruhan S wrote:
> On Wed, Nov 20, 2019 at 09:02:26AM +1100, David Gibson wrote:
> > On Wed, Nov 20, 2019 at 09:00:32AM +1100, David Gibson wrote:
> > > On Wed, Nov 20, 2019 at 08:56:18AM +1100, David Gibson wrote:
> > > > On Tue, Nov 19, 2019 at 11:20:53PM +0530, Balamuruhan S wrote:
> > > > > homer/occ sizes are calculated in skiboot with `(mask | 0xfffff) + 1`,
> > > > > and from xscom access should return correct mask values instead of actual
> > > > > sizes.
> > > > > 
> > > > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > > > Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
> > > > > ---
> > > > >  hw/ppc/pnv_xscom.c | 10 ++++++----
> > > > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> > > > > index f01d788a65..cdd5fa356e 100644
> > > > > --- a/hw/ppc/pnv_xscom.c
> > > > > +++ b/hw/ppc/pnv_xscom.c
> > > > > @@ -46,6 +46,10 @@
> > > > >  #define P9_PBA_BARMASK0                 0x5012b04
> > > > >  #define P9_PBA_BARMASK2                 0x5012b06
> > > > >  
> > > > > +/* Mask to calculate Homer/Occ size */
> > > > > +#define HOMER_SIZE_MASK                 0x0000000000300000ull
> > > > > +#define OCC_SIZE_MASK                   0x0000000000700000ull
> > > > 
> > > > Uuuhhhhh... AFAICT these defines have identical values to
> > > > PNV_HOMER_SIZE and PNV_OCC_COMMON_AREA_SIZE, so I don't see what this
> > > > patch is actually changing.
> > > 
> > > Oh, sorry, missed that the values were changed in 1/5.  Would have
> > > been easier to follow if the two patches were folded together, but
> > > never mind.  Applied.
> > 
> > Ugh.. or not.  The first patch doesn't apply for me, looks like it
> > needs a rebase (or you have something else in your tree I don't know
> > about).
> 
> I checked out from upstream master (commit: 8937f17249 ) and worked on
                                                 ^
						 |
ignore this its my first patch commit id (1/5)----

> it.

upstream commit id: f086f22d6c (VFIO fixes 2019-11-18), please let me
know if you would like me to respin the patches after rebase with v4.2.0
rc2 release.

-- Bala

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

Re: [PATCH 1/5] hw/ppc/pnv: incorrect homer and occ common area size

[Cédric Le Goater]

On 19/11/2019 18:50, Balamuruhan S wrote:
> Homer size is 4MB and OCC common area size is 8MB, but currently
> it is assigned with bar mask value. Also pass on the occ sram
> size 3 bits right shifted to initialize the size appropriately.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
> ---
>  hw/ppc/pnv_occ.c     | 2 +-
>  include/hw/ppc/pnv.h | 8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/ppc/pnv_occ.c b/hw/ppc/pnv_occ.c
> index 785653bb67..05c51c9de0 100644
> --- a/hw/ppc/pnv_occ.c
> +++ b/hw/ppc/pnv_occ.c
> @@ -276,7 +276,7 @@ static void pnv_occ_realize(DeviceState *dev, Error **errp)
>  
>      /* XScom region for OCC SRAM registers */
>      pnv_xscom_region_init(&occ->sram_regs, OBJECT(dev), poc->sram_ops,
> -                          occ, "occ-common-area", poc->sram_size);
> +                          occ, "occ-common-area", poc->sram_size >> 3);

the OCC common area seems to be accessed through MMIO also. Not only XSCOM.

In skiboot  : 

    bool occ_sensors_init(void)
    {
        ...
	occ_sensor_base = chip->occ_common_base + OCC_SENSOR_DATA_BLOCK_OFFSET;
        ...
    }

OCC would need two regions. One for the XSCOM access and one for the MMIO.

>  }
>  
>  static void pnv_occ_class_init(ObjectClass *klass, void *data)
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index 0b4c722e6b..e9ed8b928a 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -203,12 +203,12 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
>  #define PNV_XSCOM_BASE(chip)                                            \
>      (0x0003fc0000000000ull + ((uint64_t)(chip)->chip_id) * PNV_XSCOM_SIZE)
>  
> -#define PNV_OCC_COMMON_AREA_SIZE    0x0000000000700000ull
> +#define PNV_OCC_COMMON_AREA_SIZE    0x0000000000800000ull

ok. These are the BAR sizes. Can we deduce the barmask from the size ? 


>  #define PNV_OCC_COMMON_AREA(chip)                                       \
>      (0x7fff800000ull + ((uint64_t)PNV_CHIP_INDEX(chip) * \
>                           PNV_OCC_COMMON_AREA_SIZE))
>  
> -#define PNV_HOMER_SIZE              0x0000000000300000ull
> +#define PNV_HOMER_SIZE              0x0000000000400000ull
>  #define PNV_HOMER_BASE(chip)                                            \
>      (0x7ffd800000ull + ((uint64_t)PNV_CHIP_INDEX(chip)) * PNV_HOMER_SIZE)
>  
> @@ -271,12 +271,12 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
>  #define PNV9_XSCOM_SIZE              0x0000000400000000ull
>  #define PNV9_XSCOM_BASE(chip)        PNV9_CHIP_BASE(chip, 0x00603fc00000000ull)
>  
> -#define PNV9_OCC_COMMON_AREA_SIZE    0x0000000000700000ull
> +#define PNV9_OCC_COMMON_AREA_SIZE    0x0000000000800000ull
>  #define PNV9_OCC_COMMON_AREA(chip)                                      \
>      (0x203fff800000ull + ((uint64_t)PNV_CHIP_INDEX(chip) * \
>                             PNV9_OCC_COMMON_AREA_SIZE))
>  
> -#define PNV9_HOMER_SIZE              0x0000000000300000ull
> +#define PNV9_HOMER_SIZE              0x0000000000400000ull
>  #define PNV9_HOMER_BASE(chip)                                           \
>      (0x203ffd800000ull + ((uint64_t)PNV_CHIP_INDEX(chip)) * PNV9_HOMER_SIZE)
>  #endif /* PPC_PNV_H */
> 

Re: [PATCH 2/5] hw/ppc/pnv_xscom: PBA bar mask values are incorrect with homer/occ sizes

[Cédric Le Goater]

On 19/11/2019 18:50, Balamuruhan S wrote:
> homer/occ sizes are calculated in skiboot with `(mask | 0xfffff) + 1`,
> and from xscom access should return correct mask values instead of actual
> sizes.
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
> ---
>  hw/ppc/pnv_xscom.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> index f01d788a65..cdd5fa356e 100644
> --- a/hw/ppc/pnv_xscom.c
> +++ b/hw/ppc/pnv_xscom.c
> @@ -46,6 +46,10 @@
>  #define P9_PBA_BARMASK0                 0x5012b04
>  #define P9_PBA_BARMASK2                 0x5012b06
>  
> +/* Mask to calculate Homer/Occ size */
> +#define HOMER_SIZE_MASK                 0x0000000000300000ull
> +#define OCC_SIZE_MASK                   0x0000000000700000ull
> +

Can't we deduce these values from the size ? 

>  static void xscom_complete(CPUState *cs, uint64_t hmer_bits)
>  {
>      /*
> @@ -90,9 +94,8 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
>          return PNV_HOMER_BASE(chip);
>  
>      case P9_PBA_BARMASK0: /* P9 homer region size */
> -        return PNV9_HOMER_SIZE;
>      case P8_PBA_BARMASK0: /* P8 homer region size */
> -        return PNV_HOMER_SIZE;
> +        return HOMER_SIZE_MASK;

I would prefer to move all the HOMER accesses in a XSCOM region
under the PnvHomer model than expanding the default handlers. 
You will need a different set of handlers for P8 and P9 and a 
different mapping address also. 

Could you do that please ? 
  
>      case P9_PBA_BAR2: /* P9 occ common area */
>          return PNV9_OCC_COMMON_AREA(chip);
> @@ -100,9 +103,8 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
>          return PNV_OCC_COMMON_AREA(chip);
>  
>      case P9_PBA_BARMASK2: /* P9 occ common area size */
> -        return PNV9_OCC_COMMON_AREA_SIZE;
>      case P8_PBA_BARMASK2: /* P8 occ common area size */

Shouldn't that be PBA_*3 under P8 ? 

C. 

> -        return PNV_OCC_COMMON_AREA_SIZE;
> +        return OCC_SIZE_MASK;
>  
>      case 0x1010c00:     /* PIBAM FIR */
>      case 0x1010c03:     /* PIBAM FIR MASK */
> 

Re: [PATCH 3/5] hw/ppc/pnv_xscom: Power8 occ common area is in PBA BAR 3

[Cédric Le Goater]

On 19/11/2019 18:50, Balamuruhan S wrote:
> Fix incorrect PBA BAR and BARMASK value for Power8 occ common area
> region where skiboot enum declaration have it in BAR 3 and BARMASK
> is calculated BARMASK0 + BAR,
> 
> enum P8_BAR {
>         P8_BAR_HOMER = 0,
>         P8_BAR_CENTAUR = 1,
>         P8_BAR_SLW = 2,
>         P8_BAR_OCC_COMMON = 3,
> };
> 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Please remove my SoB.

> Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
> ---
>  hw/ppc/pnv_xscom.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> index cdd5fa356e..cb6d6bbcfc 100644
> --- a/hw/ppc/pnv_xscom.c
> +++ b/hw/ppc/pnv_xscom.c
> @@ -38,9 +38,9 @@
>  
>  /* PBA BARs */
>  #define P8_PBA_BAR0                     0x2013f00
> -#define P8_PBA_BAR2                     0x2013f02
> +#define P8_PBA_BAR3                     0x2013f03
>  #define P8_PBA_BARMASK0                 0x2013f04
> -#define P8_PBA_BARMASK2                 0x2013f06
> +#define P8_PBA_BARMASK3                 0x2013f07

Why are you removing the BAR2 definitions ? they are still valid.


>  #define P9_PBA_BAR0                     0x5012b00
>  #define P9_PBA_BAR2                     0x5012b02
>  #define P9_PBA_BARMASK0                 0x5012b04
> @@ -99,11 +99,11 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
>  
>      case P9_PBA_BAR2: /* P9 occ common area */
>          return PNV9_OCC_COMMON_AREA(chip);
> -    case P8_PBA_BAR2: /* P8 occ common area */
> +    case P8_PBA_BAR3: /* P8 occ common area */
>          return PNV_OCC_COMMON_AREA(chip);
>  
>      case P9_PBA_BARMASK2: /* P9 occ common area size */
> -    case P8_PBA_BARMASK2: /* P8 occ common area size */
> +    case P8_PBA_BARMASK3: /* P8 occ common area size */
>          return OCC_SIZE_MASK;
>  
>      case 0x1010c00:     /* PIBAM FIR */
> @@ -126,9 +126,9 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
>      case 0x202000f:     /* ADU stuff, receive status register*/
>          return 0;
>      case 0x2013f01:     /* PBA stuff */
> -    case 0x2013f03:     /* PBA stuff */
> +    case 0x2013f02:     /* PBA stuff */
>      case 0x2013f05:     /* PBA stuff */
> -    case 0x2013f07:     /* PBA stuff */
> +    case 0x2013f06:     /* PBA stuff */

We need defines for the above ^

>          return 0;
>      case 0x2013028:     /* CAPP stuff */
>      case 0x201302a:     /* CAPP stuff */
> 

Re: [PATCH 4/5] hw/ppc/pnv_xscom: occ common area to be mapped only once

[Cédric Le Goater]

On 19/11/2019 18:50, Balamuruhan S wrote:
> occ common area should be mapped once 

It's the same address on each chip. 

the question is how the HW knows from which chip the OCC access is 
being done ? How does it target the correct OCC if the address is 
the same ? 

> and disable it for every other chip.

On P8 OpenPOWER systems, the PBA3 registers are still set, not on
tuletas though (different hostboot I suppose). On OpenPOWER systems,
they are still set also.
 
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

nah. I didn't write any of this :) 

> Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
> ---
>  hw/ppc/pnv_xscom.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> index cb6d6bbcfc..f797a5ec7d 100644
> --- a/hw/ppc/pnv_xscom.c
> +++ b/hw/ppc/pnv_xscom.c
> @@ -98,13 +98,22 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
>          return HOMER_SIZE_MASK;
>  
>      case P9_PBA_BAR2: /* P9 occ common area */
> -        return PNV9_OCC_COMMON_AREA(chip);
> +        if (!PNV_CHIP_INDEX(chip)) {

Yes that it is the idea. XIVE uses directly 'chip->chip_id'. 


> +            return PNV9_OCC_COMMON_AREA(chip);
> +        }
> +        return 0;
>      case P8_PBA_BAR3: /* P8 occ common area */
> -        return PNV_OCC_COMMON_AREA(chip);
> +        if (!PNV_CHIP_INDEX(chip)) {
> +            return PNV_OCC_COMMON_AREA(chip);
> +        }
> +        return 0;
>  
>      case P9_PBA_BARMASK2: /* P9 occ common area size */
>      case P8_PBA_BARMASK3: /* P8 occ common area size */
> -        return OCC_SIZE_MASK;
> +        if (!PNV_CHIP_INDEX(chip)) {
> +            return OCC_SIZE_MASK;
> +        }
> +        return 0;
>  
>      case 0x1010c00:     /* PIBAM FIR */
>      case 0x1010c03:     /* PIBAM FIR MASK */
> 

Re: [PATCH 5/5] hw/ppc/pnv_xscom: add PBA BARs for Power8 slw image

[Cédric Le Goater]

On 19/11/2019 18:50, Balamuruhan S wrote:
> slw base and size mask are accessed during boot in homer_init_chip(),
> so include BAR2 and BARMASK2 for Power8.
> 
> Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
> ---
>  hw/ppc/pnv_xscom.c   | 10 ++++++++--
>  include/hw/ppc/pnv.h |  4 ++++
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> index f797a5ec7d..828a2e2a5a 100644
> --- a/hw/ppc/pnv_xscom.c
> +++ b/hw/ppc/pnv_xscom.c
> @@ -38,8 +38,10 @@
>  
>  /* PBA BARs */
>  #define P8_PBA_BAR0                     0x2013f00
> +#define P8_PBA_BAR2                     0x2013f02
>  #define P8_PBA_BAR3                     0x2013f03
>  #define P8_PBA_BARMASK0                 0x2013f04
> +#define P8_PBA_BARMASK2                 0x2013f06

and you add the definitions back ! :)

>  #define P8_PBA_BARMASK3                 0x2013f07
>  #define P9_PBA_BAR0                     0x5012b00
>  #define P9_PBA_BAR2                     0x5012b02
> @@ -49,6 +51,7 @@
>  /* Mask to calculate Homer/Occ size */
>  #define HOMER_SIZE_MASK                 0x0000000000300000ull
>  #define OCC_SIZE_MASK                   0x0000000000700000ull
> +#define SLW_SIZE_MASK                   0x0
>  
>  static void xscom_complete(CPUState *cs, uint64_t hmer_bits)
>  {
> @@ -115,6 +118,11 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
>          }
>          return 0;
>  
> +    case P8_PBA_BAR2: /* P8 slw image */
> +        return PNV_SLW_IMAGE_BASE(chip);
> +    case P8_PBA_BARMASK2: /* P8 slw image size is 1MB and mask is zero*/
> +        return SLW_SIZE_MASK;

We need a HOMER XSCOM region.

> +
>      case 0x1010c00:     /* PIBAM FIR */
>      case 0x1010c03:     /* PIBAM FIR MASK */
>  
> @@ -135,9 +143,7 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
>      case 0x202000f:     /* ADU stuff, receive status register*/
>          return 0;
>      case 0x2013f01:     /* PBA stuff */
> -    case 0x2013f02:     /* PBA stuff */
>      case 0x2013f05:     /* PBA stuff */
> -    case 0x2013f06:     /* PBA stuff */
>          return 0;
>      case 0x2013028:     /* CAPP stuff */
>      case 0x201302a:     /* CAPP stuff */
> diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> index e9ed8b928a..bd22dbf8a9 100644
> --- a/include/hw/ppc/pnv.h
> +++ b/include/hw/ppc/pnv.h
> @@ -212,6 +212,10 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
>  #define PNV_HOMER_BASE(chip)                                            \
>      (0x7ffd800000ull + ((uint64_t)PNV_CHIP_INDEX(chip)) * PNV_HOMER_SIZE)
>  
> +#define PNV_SLW_SIZE                0x0000000000100000ull
> +#define PNV_SLW_IMAGE_BASE(chip)                                        \
> +    (0x2ffda00000ull + ((uint64_t)PNV_CHIP_INDEX(chip)) * PNV_SLW_SIZE)
> +
>  
>  /*
>   * XSCOM 0x20109CA defines the ICP BAR:
> 

Re: [PATCH 0/5] ppc/pnv: fix Homer/Occ mappings on multichip systems

[Cédric Le Goater]

Hello,

On 19/11/2019 18:50, Balamuruhan S wrote:
> Hi All,
> 
> PowerNV fails to boot in multichip systems due to some misinterpretation
> and mapping in Homer/Occ device models, this patchset fixes the
> following,
> 
>  - Homer size is 4MB per chip and Occ common area size is 8MB
>  - Bar masks are used to calculate sizes of Homer/Occ in skiboot so
>    return appropriate value
>  - Occ common area is in BAR 3 on Power8 but wrongly mapped to BAR 2
>    currently
>  - OCC common area is shared across chips and should be mapped only once
>    for multichip systems

The first thing to address is the HOMER XSCOM region. 

Introduce an empty skeleton for P8 and P9 with different mem ops handers
because the registers have a different layout. From there, add the support
for the different PBA* regs and move them out from the default XSCOM
handlers. That should fix most of the current problems and it will provide 
a nice framework for future extensions.

Why not add the associated HOMER MMIO region while we are it ? the PBA
registers have all the definitions we need and it will gives us access
to the pstate table.


Second is the OCC region. Do we need a XSCOM *or* a MMIO region ? This is 
not clear. Please check skiboot. I think a MMIO region should be enough
because this is how sensor data from the OCC is accessed. 

On that topic, we could define properties on the PnvOCC model for each 
sensor and tune the value from the QEMU monitor. It really shouldn't be
too complex.

Also the same address is used, we should only map it once but we need 
to invent something to know from which chip it is accessed. 


C.


> 
> Request for your review and suggestions to make it better. I would like to
> thank Cedric for his time and help to figure out the issues.
>
> Balamuruhan S (5):
>   hw/ppc/pnv: incorrect homer and occ common area size
>   hw/ppc/pnv_xscom: PBA bar mask values are incorrect with homer/occ
>     sizes
>   hw/ppc/pnv_xscom: Power8 occ common area is in PBA BAR 3
>   hw/ppc/pnv_xscom: occ common area to be mapped only once
>   hw/ppc/pnv_xscom: add PBA BARs for Power8 slw image
> 
>  hw/ppc/pnv_occ.c     |  2 +-
>  hw/ppc/pnv_xscom.c   | 37 +++++++++++++++++++++++++++----------
>  include/hw/ppc/pnv.h | 12 ++++++++----
>  3 files changed, 36 insertions(+), 15 deletions(-)
> 

Re: [PATCH 2/5] hw/ppc/pnv_xscom: PBA bar mask values are incorrect with homer/occ sizes

[Greg Kurz]

On Wed, 20 Nov 2019 08:46:51 +0530
Balamuruhan S <bala24@linux.ibm.com> wrote:

> On Wed, Nov 20, 2019 at 08:31:03AM +0530, Balamuruhan S wrote:
> > On Wed, Nov 20, 2019 at 09:02:26AM +1100, David Gibson wrote:
> > > On Wed, Nov 20, 2019 at 09:00:32AM +1100, David Gibson wrote:
> > > > On Wed, Nov 20, 2019 at 08:56:18AM +1100, David Gibson wrote:
> > > > > On Tue, Nov 19, 2019 at 11:20:53PM +0530, Balamuruhan S wrote:
> > > > > > homer/occ sizes are calculated in skiboot with `(mask | 0xfffff) + 1`,
> > > > > > and from xscom access should return correct mask values instead of actual
> > > > > > sizes.
> > > > > > 
> > > > > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > > > > Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
> > > > > > ---
> > > > > >  hw/ppc/pnv_xscom.c | 10 ++++++----
> > > > > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> > > > > > index f01d788a65..cdd5fa356e 100644
> > > > > > --- a/hw/ppc/pnv_xscom.c
> > > > > > +++ b/hw/ppc/pnv_xscom.c
> > > > > > @@ -46,6 +46,10 @@
> > > > > >  #define P9_PBA_BARMASK0                 0x5012b04
> > > > > >  #define P9_PBA_BARMASK2                 0x5012b06
> > > > > >  
> > > > > > +/* Mask to calculate Homer/Occ size */
> > > > > > +#define HOMER_SIZE_MASK                 0x0000000000300000ull
> > > > > > +#define OCC_SIZE_MASK                   0x0000000000700000ull
> > > > > 
> > > > > Uuuhhhhh... AFAICT these defines have identical values to
> > > > > PNV_HOMER_SIZE and PNV_OCC_COMMON_AREA_SIZE, so I don't see what this
> > > > > patch is actually changing.
> > > > 
> > > > Oh, sorry, missed that the values were changed in 1/5.  Would have
> > > > been easier to follow if the two patches were folded together, but
> > > > never mind.  Applied.
> > > 
> > > Ugh.. or not.  The first patch doesn't apply for me, looks like it
> > > needs a rebase (or you have something else in your tree I don't know
> > > about).
> > 
> > I checked out from upstream master (commit: 8937f17249 ) and worked on
>                                                  ^
> 						 |
> ignore this its my first patch commit id (1/5)----
> 
> > it.
> 
> upstream commit id: f086f22d6c (VFIO fixes 2019-11-18), please let me
> know if you would like me to respin the patches after rebase with v4.2.0
> rc2 release.
> 

Or maybe base your patches on David's ppc-for-5.0 branch available at
https://github.com/dgibson/qemu .

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

Re: [PATCH 1/5] hw/ppc/pnv: incorrect homer and occ common area size

[Balamuruhan S]

On Wed, Nov 20, 2019 at 08:13:50AM +0100, Cédric Le Goater wrote:
> On 19/11/2019 18:50, Balamuruhan S wrote:
> > Homer size is 4MB and OCC common area size is 8MB, but currently
> > it is assigned with bar mask value. Also pass on the occ sram
> > size 3 bits right shifted to initialize the size appropriately.
> > 
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
> > ---
> >  hw/ppc/pnv_occ.c     | 2 +-
> >  include/hw/ppc/pnv.h | 8 ++++----
> >  2 files changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv_occ.c b/hw/ppc/pnv_occ.c
> > index 785653bb67..05c51c9de0 100644
> > --- a/hw/ppc/pnv_occ.c
> > +++ b/hw/ppc/pnv_occ.c
> > @@ -276,7 +276,7 @@ static void pnv_occ_realize(DeviceState *dev, Error **errp)
> >  
> >      /* XScom region for OCC SRAM registers */
> >      pnv_xscom_region_init(&occ->sram_regs, OBJECT(dev), poc->sram_ops,
> > -                          occ, "occ-common-area", poc->sram_size);
> > +                          occ, "occ-common-area", poc->sram_size >> 3);
> 
> the OCC common area seems to be accessed through MMIO also. Not only XSCOM.
> 
> In skiboot  : 
> 
>     bool occ_sensors_init(void)
>     {
>         ...
> 	occ_sensor_base = chip->occ_common_base + OCC_SENSOR_DATA_BLOCK_OFFSET;
>         ...
>     }
> 
> OCC would need two regions. One for the XSCOM access and one for the MMIO.
> 
> >  }

Okay, I will follow it.

> >  
> >  static void pnv_occ_class_init(ObjectClass *klass, void *data)
> > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> > index 0b4c722e6b..e9ed8b928a 100644
> > --- a/include/hw/ppc/pnv.h
> > +++ b/include/hw/ppc/pnv.h
> > @@ -203,12 +203,12 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
> >  #define PNV_XSCOM_BASE(chip)                                            \
> >      (0x0003fc0000000000ull + ((uint64_t)(chip)->chip_id) * PNV_XSCOM_SIZE)
> >  
> > -#define PNV_OCC_COMMON_AREA_SIZE    0x0000000000700000ull
> > +#define PNV_OCC_COMMON_AREA_SIZE    0x0000000000800000ull
> 
> ok. These are the BAR sizes. Can we deduce the barmask from the size ? 

Correct! we can do that, will make the respective change.

> 
> 
> >  #define PNV_OCC_COMMON_AREA(chip)                                       \
> >      (0x7fff800000ull + ((uint64_t)PNV_CHIP_INDEX(chip) * \
> >                           PNV_OCC_COMMON_AREA_SIZE))
> >  
> > -#define PNV_HOMER_SIZE              0x0000000000300000ull
> > +#define PNV_HOMER_SIZE              0x0000000000400000ull
> >  #define PNV_HOMER_BASE(chip)                                            \
> >      (0x7ffd800000ull + ((uint64_t)PNV_CHIP_INDEX(chip)) * PNV_HOMER_SIZE)
> >  
> > @@ -271,12 +271,12 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
> >  #define PNV9_XSCOM_SIZE              0x0000000400000000ull
> >  #define PNV9_XSCOM_BASE(chip)        PNV9_CHIP_BASE(chip, 0x00603fc00000000ull)
> >  
> > -#define PNV9_OCC_COMMON_AREA_SIZE    0x0000000000700000ull
> > +#define PNV9_OCC_COMMON_AREA_SIZE    0x0000000000800000ull
> >  #define PNV9_OCC_COMMON_AREA(chip)                                      \
> >      (0x203fff800000ull + ((uint64_t)PNV_CHIP_INDEX(chip) * \
> >                             PNV9_OCC_COMMON_AREA_SIZE))
> >  
> > -#define PNV9_HOMER_SIZE              0x0000000000300000ull
> > +#define PNV9_HOMER_SIZE              0x0000000000400000ull
> >  #define PNV9_HOMER_BASE(chip)                                           \
> >      (0x203ffd800000ull + ((uint64_t)PNV_CHIP_INDEX(chip)) * PNV9_HOMER_SIZE)
> >  #endif /* PPC_PNV_H */
> > 
> 

Re: [PATCH 2/5] hw/ppc/pnv_xscom: PBA bar mask values are incorrect with homer/occ sizes

[Balamuruhan S]

On Wed, Nov 20, 2019 at 08:59:40AM +0100, Greg Kurz wrote:
> On Wed, 20 Nov 2019 08:46:51 +0530
> Balamuruhan S <bala24@linux.ibm.com> wrote:
> 
> > On Wed, Nov 20, 2019 at 08:31:03AM +0530, Balamuruhan S wrote:
> > > On Wed, Nov 20, 2019 at 09:02:26AM +1100, David Gibson wrote:
> > > > On Wed, Nov 20, 2019 at 09:00:32AM +1100, David Gibson wrote:
> > > > > On Wed, Nov 20, 2019 at 08:56:18AM +1100, David Gibson wrote:
> > > > > > On Tue, Nov 19, 2019 at 11:20:53PM +0530, Balamuruhan S wrote:
> > > > > > > homer/occ sizes are calculated in skiboot with `(mask | 0xfffff) + 1`,
> > > > > > > and from xscom access should return correct mask values instead of actual
> > > > > > > sizes.
> > > > > > > 
> > > > > > > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > > > > > > Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
> > > > > > > ---
> > > > > > >  hw/ppc/pnv_xscom.c | 10 ++++++----
> > > > > > >  1 file changed, 6 insertions(+), 4 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> > > > > > > index f01d788a65..cdd5fa356e 100644
> > > > > > > --- a/hw/ppc/pnv_xscom.c
> > > > > > > +++ b/hw/ppc/pnv_xscom.c
> > > > > > > @@ -46,6 +46,10 @@
> > > > > > >  #define P9_PBA_BARMASK0                 0x5012b04
> > > > > > >  #define P9_PBA_BARMASK2                 0x5012b06
> > > > > > >  
> > > > > > > +/* Mask to calculate Homer/Occ size */
> > > > > > > +#define HOMER_SIZE_MASK                 0x0000000000300000ull
> > > > > > > +#define OCC_SIZE_MASK                   0x0000000000700000ull
> > > > > > 
> > > > > > Uuuhhhhh... AFAICT these defines have identical values to
> > > > > > PNV_HOMER_SIZE and PNV_OCC_COMMON_AREA_SIZE, so I don't see what this
> > > > > > patch is actually changing.
> > > > > 
> > > > > Oh, sorry, missed that the values were changed in 1/5.  Would have
> > > > > been easier to follow if the two patches were folded together, but
> > > > > never mind.  Applied.
> > > > 
> > > > Ugh.. or not.  The first patch doesn't apply for me, looks like it
> > > > needs a rebase (or you have something else in your tree I don't know
> > > > about).
> > > 
> > > I checked out from upstream master (commit: 8937f17249 ) and worked on
> >                                                  ^
> > 						 |
> > ignore this its my first patch commit id (1/5)----
> > 
> > > it.
> > 
> > upstream commit id: f086f22d6c (VFIO fixes 2019-11-18), please let me
> > know if you would like me to respin the patches after rebase with v4.2.0
> > rc2 release.
> > 
> 
> Or maybe base your patches on David's ppc-for-5.0 branch available at
> https://github.com/dgibson/qemu .

Thanks Greg, going forward I will base my patches on David's recent branch for
the release.

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

Re: [PATCH 2/5] hw/ppc/pnv_xscom: PBA bar mask values are incorrect with homer/occ sizes

[Balamuruhan S]

On Wed, Nov 20, 2019 at 08:18:38AM +0100, Cédric Le Goater wrote:
> On 19/11/2019 18:50, Balamuruhan S wrote:
> > homer/occ sizes are calculated in skiboot with `(mask | 0xfffff) + 1`,
> > and from xscom access should return correct mask values instead of actual
> > sizes.
> > 
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> > Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
> > ---
> >  hw/ppc/pnv_xscom.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> > index f01d788a65..cdd5fa356e 100644
> > --- a/hw/ppc/pnv_xscom.c
> > +++ b/hw/ppc/pnv_xscom.c
> > @@ -46,6 +46,10 @@
> >  #define P9_PBA_BARMASK0                 0x5012b04
> >  #define P9_PBA_BARMASK2                 0x5012b06
> >  
> > +/* Mask to calculate Homer/Occ size */
> > +#define HOMER_SIZE_MASK                 0x0000000000300000ull
> > +#define OCC_SIZE_MASK                   0x0000000000700000ull
> > +
> 
> Can't we deduce these values from the size ? 

yes, that's a better way.

> 
> >  static void xscom_complete(CPUState *cs, uint64_t hmer_bits)
> >  {
> >      /*
> > @@ -90,9 +94,8 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
> >          return PNV_HOMER_BASE(chip);
> >  
> >      case P9_PBA_BARMASK0: /* P9 homer region size */
> > -        return PNV9_HOMER_SIZE;
> >      case P8_PBA_BARMASK0: /* P8 homer region size */
> > -        return PNV_HOMER_SIZE;
> > +        return HOMER_SIZE_MASK;
> 
> I would prefer to move all the HOMER accesses in a XSCOM region
> under the PnvHomer model than expanding the default handlers. 
> You will need a different set of handlers for P8 and P9 and a 
> different mapping address also. 
> 
> Could you do that please ? 

Sure Cedric, I can work on it.

>   
> >      case P9_PBA_BAR2: /* P9 occ common area */
> >          return PNV9_OCC_COMMON_AREA(chip);
> > @@ -100,9 +103,8 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
> >          return PNV_OCC_COMMON_AREA(chip);
> >  
> >      case P9_PBA_BARMASK2: /* P9 occ common area size */
> > -        return PNV9_OCC_COMMON_AREA_SIZE;
> >      case P8_PBA_BARMASK2: /* P8 occ common area size */
> 
> Shouldn't that be PBA_*3 under P8 ? 

:( It's a miss from me. Thanks!

> 
> C. 
> 
> > -        return PNV_OCC_COMMON_AREA_SIZE;
> > +        return OCC_SIZE_MASK;
> >  
> >      case 0x1010c00:     /* PIBAM FIR */
> >      case 0x1010c03:     /* PIBAM FIR MASK */
> > 
> 

Re: [PATCH 3/5] hw/ppc/pnv_xscom: Power8 occ common area is in PBA BAR 3

[Balamuruhan S]

On Wed, Nov 20, 2019 at 08:20:35AM +0100, Cédric Le Goater wrote:
> On 19/11/2019 18:50, Balamuruhan S wrote:
> > Fix incorrect PBA BAR and BARMASK value for Power8 occ common area
> > region where skiboot enum declaration have it in BAR 3 and BARMASK
> > is calculated BARMASK0 + BAR,
> > 
> > enum P8_BAR {
> >         P8_BAR_HOMER = 0,
> >         P8_BAR_CENTAUR = 1,
> >         P8_BAR_SLW = 2,
> >         P8_BAR_OCC_COMMON = 3,
> > };
> > 
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> Please remove my SoB.

Okay.

> 
> > Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
> > ---
> >  hw/ppc/pnv_xscom.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> > index cdd5fa356e..cb6d6bbcfc 100644
> > --- a/hw/ppc/pnv_xscom.c
> > +++ b/hw/ppc/pnv_xscom.c
> > @@ -38,9 +38,9 @@
> >  
> >  /* PBA BARs */
> >  #define P8_PBA_BAR0                     0x2013f00
> > -#define P8_PBA_BAR2                     0x2013f02
> > +#define P8_PBA_BAR3                     0x2013f03
> >  #define P8_PBA_BARMASK0                 0x2013f04
> > -#define P8_PBA_BARMASK2                 0x2013f06
> > +#define P8_PBA_BARMASK3                 0x2013f07
> 
> Why are you removing the BAR2 definitions ? they are still valid.

Thought of clean it and add it as part of P8 slw patch.

> 
> 
> >  #define P9_PBA_BAR0                     0x5012b00
> >  #define P9_PBA_BAR2                     0x5012b02
> >  #define P9_PBA_BARMASK0                 0x5012b04
> > @@ -99,11 +99,11 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
> >  
> >      case P9_PBA_BAR2: /* P9 occ common area */
> >          return PNV9_OCC_COMMON_AREA(chip);
> > -    case P8_PBA_BAR2: /* P8 occ common area */
> > +    case P8_PBA_BAR3: /* P8 occ common area */
> >          return PNV_OCC_COMMON_AREA(chip);
> >  
> >      case P9_PBA_BARMASK2: /* P9 occ common area size */
> > -    case P8_PBA_BARMASK2: /* P8 occ common area size */
> > +    case P8_PBA_BARMASK3: /* P8 occ common area size */
> >          return OCC_SIZE_MASK;
> >  
> >      case 0x1010c00:     /* PIBAM FIR */
> > @@ -126,9 +126,9 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
> >      case 0x202000f:     /* ADU stuff, receive status register*/
> >          return 0;
> >      case 0x2013f01:     /* PBA stuff */
> > -    case 0x2013f03:     /* PBA stuff */
> > +    case 0x2013f02:     /* PBA stuff */
> >      case 0x2013f05:     /* PBA stuff */
> > -    case 0x2013f07:     /* PBA stuff */
> > +    case 0x2013f06:     /* PBA stuff */
> 
> We need defines for the above ^

will do.

> 
> >          return 0;
> >      case 0x2013028:     /* CAPP stuff */
> >      case 0x201302a:     /* CAPP stuff */
> > 
> 

Re: [PATCH 4/5] hw/ppc/pnv_xscom: occ common area to be mapped only once

[Balamuruhan S]

On Wed, Nov 20, 2019 at 08:30:03AM +0100, Cédric Le Goater wrote:
> On 19/11/2019 18:50, Balamuruhan S wrote:
> > occ common area should be mapped once 
> 
> It's the same address on each chip. 
> 
> the question is how the HW knows from which chip the OCC access is 
> being done ? How does it target the correct OCC if the address is 
> the same ? 

I am not aware of it for now to answer/comment, will try to find
it.

> 
> > and disable it for every other chip.
> 
> On P8 OpenPOWER systems, the PBA3 registers are still set, not on
> tuletas though (different hostboot I suppose). On OpenPOWER systems,
> they are still set also.

Will have to do some study on skiboot.

>  
> > Signed-off-by: Cédric Le Goater <clg@kaod.org>
> 
> nah. I didn't write any of this :) 
> 
> > Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
> > ---
> >  hw/ppc/pnv_xscom.c | 15 ++++++++++++---
> >  1 file changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> > index cb6d6bbcfc..f797a5ec7d 100644
> > --- a/hw/ppc/pnv_xscom.c
> > +++ b/hw/ppc/pnv_xscom.c
> > @@ -98,13 +98,22 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
> >          return HOMER_SIZE_MASK;
> >  
> >      case P9_PBA_BAR2: /* P9 occ common area */
> > -        return PNV9_OCC_COMMON_AREA(chip);
> > +        if (!PNV_CHIP_INDEX(chip)) {
> 
> Yes that it is the idea. XIVE uses directly 'chip->chip_id'. 

:)

> 
> 
> > +            return PNV9_OCC_COMMON_AREA(chip);
> > +        }
> > +        return 0;
> >      case P8_PBA_BAR3: /* P8 occ common area */
> > -        return PNV_OCC_COMMON_AREA(chip);
> > +        if (!PNV_CHIP_INDEX(chip)) {
> > +            return PNV_OCC_COMMON_AREA(chip);
> > +        }
> > +        return 0;
> >  
> >      case P9_PBA_BARMASK2: /* P9 occ common area size */
> >      case P8_PBA_BARMASK3: /* P8 occ common area size */
> > -        return OCC_SIZE_MASK;
> > +        if (!PNV_CHIP_INDEX(chip)) {
> > +            return OCC_SIZE_MASK;
> > +        }
> > +        return 0;
> >  
> >      case 0x1010c00:     /* PIBAM FIR */
> >      case 0x1010c03:     /* PIBAM FIR MASK */
> > 
> 

Re: [PATCH 5/5] hw/ppc/pnv_xscom: add PBA BARs for Power8 slw image

[Balamuruhan S]

On Wed, Nov 20, 2019 at 08:31:50AM +0100, Cédric Le Goater wrote:
> On 19/11/2019 18:50, Balamuruhan S wrote:
> > slw base and size mask are accessed during boot in homer_init_chip(),
> > so include BAR2 and BARMASK2 for Power8.
> > 
> > Signed-off-by: Balamuruhan S <bala24@linux.ibm.com>
> > ---
> >  hw/ppc/pnv_xscom.c   | 10 ++++++++--
> >  include/hw/ppc/pnv.h |  4 ++++
> >  2 files changed, 12 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/ppc/pnv_xscom.c b/hw/ppc/pnv_xscom.c
> > index f797a5ec7d..828a2e2a5a 100644
> > --- a/hw/ppc/pnv_xscom.c
> > +++ b/hw/ppc/pnv_xscom.c
> > @@ -38,8 +38,10 @@
> >  
> >  /* PBA BARs */
> >  #define P8_PBA_BAR0                     0x2013f00
> > +#define P8_PBA_BAR2                     0x2013f02
> >  #define P8_PBA_BAR3                     0x2013f03
> >  #define P8_PBA_BARMASK0                 0x2013f04
> > +#define P8_PBA_BARMASK2                 0x2013f06
> 
> and you add the definitions back ! :)

will make it clean.

> 
> >  #define P8_PBA_BARMASK3                 0x2013f07
> >  #define P9_PBA_BAR0                     0x5012b00
> >  #define P9_PBA_BAR2                     0x5012b02
> > @@ -49,6 +51,7 @@
> >  /* Mask to calculate Homer/Occ size */
> >  #define HOMER_SIZE_MASK                 0x0000000000300000ull
> >  #define OCC_SIZE_MASK                   0x0000000000700000ull
> > +#define SLW_SIZE_MASK                   0x0
> >  
> >  static void xscom_complete(CPUState *cs, uint64_t hmer_bits)
> >  {
> > @@ -115,6 +118,11 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
> >          }
> >          return 0;
> >  
> > +    case P8_PBA_BAR2: /* P8 slw image */
> > +        return PNV_SLW_IMAGE_BASE(chip);
> > +    case P8_PBA_BARMASK2: /* P8 slw image size is 1MB and mask is zero*/
> > +        return SLW_SIZE_MASK;
> 
> We need a HOMER XSCOM region.

okay.

> 
> > +
> >      case 0x1010c00:     /* PIBAM FIR */
> >      case 0x1010c03:     /* PIBAM FIR MASK */
> >  
> > @@ -135,9 +143,7 @@ static uint64_t xscom_read_default(PnvChip *chip, uint32_t pcba)
> >      case 0x202000f:     /* ADU stuff, receive status register*/
> >          return 0;
> >      case 0x2013f01:     /* PBA stuff */
> > -    case 0x2013f02:     /* PBA stuff */
> >      case 0x2013f05:     /* PBA stuff */
> > -    case 0x2013f06:     /* PBA stuff */
> >          return 0;
> >      case 0x2013028:     /* CAPP stuff */
> >      case 0x201302a:     /* CAPP stuff */
> > diff --git a/include/hw/ppc/pnv.h b/include/hw/ppc/pnv.h
> > index e9ed8b928a..bd22dbf8a9 100644
> > --- a/include/hw/ppc/pnv.h
> > +++ b/include/hw/ppc/pnv.h
> > @@ -212,6 +212,10 @@ void pnv_bmc_powerdown(IPMIBmc *bmc);
> >  #define PNV_HOMER_BASE(chip)                                            \
> >      (0x7ffd800000ull + ((uint64_t)PNV_CHIP_INDEX(chip)) * PNV_HOMER_SIZE)
> >  
> > +#define PNV_SLW_SIZE                0x0000000000100000ull
> > +#define PNV_SLW_IMAGE_BASE(chip)                                        \
> > +    (0x2ffda00000ull + ((uint64_t)PNV_CHIP_INDEX(chip)) * PNV_SLW_SIZE)
> > +
> >  
> >  /*
> >   * XSCOM 0x20109CA defines the ICP BAR:
> > 
> 

Re: [PATCH 0/5] ppc/pnv: fix Homer/Occ mappings on multichip systems

[Balamuruhan S]

On Wed, Nov 20, 2019 at 08:46:30AM +0100, Cédric Le Goater wrote:
> Hello,
> 
> On 19/11/2019 18:50, Balamuruhan S wrote:
> > Hi All,
> > 
> > PowerNV fails to boot in multichip systems due to some misinterpretation
> > and mapping in Homer/Occ device models, this patchset fixes the
> > following,
> > 
> >  - Homer size is 4MB per chip and Occ common area size is 8MB
> >  - Bar masks are used to calculate sizes of Homer/Occ in skiboot so
> >    return appropriate value
> >  - Occ common area is in BAR 3 on Power8 but wrongly mapped to BAR 2
> >    currently
> >  - OCC common area is shared across chips and should be mapped only once
> >    for multichip systems
> 
> The first thing to address is the HOMER XSCOM region. 
> 
> Introduce an empty skeleton for P8 and P9 with different mem ops handers
> because the registers have a different layout. From there, add the support
> for the different PBA* regs and move them out from the default XSCOM
> handlers. That should fix most of the current problems and it will provide 
> a nice framework for future extensions.

sure, I will work on it.

> 
> Why not add the associated HOMER MMIO region while we are it ? the PBA
> registers have all the definitions we need and it will gives us access
> to the pstate table.

so, idea is to have HOMER MMIO for us to use it accessing pstate table / data
and HOMER XSCOM for homer associated xscom access for PBA* registers to
P8 and P9 respectively.

> 
> 
> Second is the OCC region. Do we need a XSCOM *or* a MMIO region ? This is 
> not clear. Please check skiboot. I think a MMIO region should be enough
> because this is how sensor data from the OCC is accessed.

Okay, I will do the change for OCC to use MMIO, and will check skiboot
for making it better.

> 
> On that topic, we could define properties on the PnvOCC model for each 
> sensor and tune the value from the QEMU monitor. It really shouldn't be
> too complex.

How can we tune value from QEMU monitor ? This is new to me and will
need to check it. I remember you have advised this with the error
injection framework patches and Rashmica's patch that provides way to
use Qemu monitor to feed data, but I need to do some study.

> 
> Also the same address is used, we should only map it once but we need 
> to invent something to know from which chip it is accessed.

This is something need to check how real hardware handles it while
accessing shared occ region from different chip and think how to make it
for us.

Thanks a lot!

-- Bala

> 
> 
> C.
> 
> 
> > 
> > Request for your review and suggestions to make it better. I would like to
> > thank Cedric for his time and help to figure out the issues.
> >
> > Balamuruhan S (5):
> >   hw/ppc/pnv: incorrect homer and occ common area size
> >   hw/ppc/pnv_xscom: PBA bar mask values are incorrect with homer/occ
> >     sizes
> >   hw/ppc/pnv_xscom: Power8 occ common area is in PBA BAR 3
> >   hw/ppc/pnv_xscom: occ common area to be mapped only once
> >   hw/ppc/pnv_xscom: add PBA BARs for Power8 slw image
> > 
> >  hw/ppc/pnv_occ.c     |  2 +-
> >  hw/ppc/pnv_xscom.c   | 37 +++++++++++++++++++++++++++----------
> >  include/hw/ppc/pnv.h | 12 ++++++++----
> >  3 files changed, 36 insertions(+), 15 deletions(-)
> > 
> 

Re: [PATCH 0/5] ppc/pnv: fix Homer/Occ mappings on multichip systems

[Cédric Le Goater]

On 21/11/2019 10:11, Balamuruhan S wrote:
> On Wed, Nov 20, 2019 at 08:46:30AM +0100, Cédric Le Goater wrote:
>> Hello,
>>
>> On 19/11/2019 18:50, Balamuruhan S wrote:
>>> Hi All,
>>>
>>> PowerNV fails to boot in multichip systems due to some misinterpretation
>>> and mapping in Homer/Occ device models, this patchset fixes the
>>> following,
>>>
>>>  - Homer size is 4MB per chip and Occ common area size is 8MB
>>>  - Bar masks are used to calculate sizes of Homer/Occ in skiboot so
>>>    return appropriate value
>>>  - Occ common area is in BAR 3 on Power8 but wrongly mapped to BAR 2
>>>    currently
>>>  - OCC common area is shared across chips and should be mapped only once
>>>    for multichip systems
>>
>> The first thing to address is the HOMER XSCOM region. 
>>
>> Introduce an empty skeleton for P8 and P9 with different mem ops handers
>> because the registers have a different layout. From there, add the support
>> for the different PBA* regs and move them out from the default XSCOM
>> handlers. That should fix most of the current problems and it will provide 
>> a nice framework for future extensions.
> 
> sure, I will work on it.
> 
>>
>> Why not add the associated HOMER MMIO region while we are it ? the PBA
>> registers have all the definitions we need and it will gives us access
>> to the pstate table.
> 
> so, idea is to have HOMER MMIO for us to use it accessing pstate table / data
> and HOMER XSCOM for homer associated xscom access for PBA* registers to
> P8 and P9 respectively.

yes. 

>> Second is the OCC region. Do we need a XSCOM *or* a MMIO region ? This is 
>> not clear. Please check skiboot. I think a MMIO region should be enough
>> because this is how sensor data from the OCC is accessed.
> 
> Okay, I will do the change for OCC to use MMIO, and will check skiboot
> for making it better.
> 
>>
>> On that topic, we could define properties on the PnvOCC model for each 
>> sensor and tune the value from the QEMU monitor. It really shouldn't be
>> too complex.
> 
> How can we tune value from QEMU monitor ? This is new to me and will
> need to check it. I remember you have advised this with the error
> injection framework patches and Rashmica's patch that provides way to
> use Qemu monitor to feed data, but I need to do some study.


See Joel's patch which has a simple example :  

   patchwork.ozlabs.org/patch/1196519

It simply generates object properties : 


+    for (led = 0; led < s->nr_leds; led++) {
+        char *name;
+
+        name = g_strdup_printf("led%d", led);
+        object_property_add(obj, name, "bool", pca9552_get_led, pca9552_set_led,
+                            NULL, NULL, NULL);
+    }

with defined get and set accessors. 

We could do the same for the OCC sensors with a table describing the 
sensor layout. Accessors would just simply update the table. we could
even trigger the OCC interrupt if needed.

This is the initial table :

  https://github.com/open-power/occ/blob/master/src/occ_405/sensor/sensor_info.c

Linux should be able to grab the values through hwmon just as on real HW.
This is the case today for the DTS.

>>
>> Also the same address is used, we should only map it once but we need 
>> to invent something to know from which chip it is accessed.
> 
> This is something need to check how real hardware handles it while
> accessing shared occ region from different chip and think how to make it
> for us.

Yes. I suppose there is some chip id in the powerbus message.

C.

  
> 
> Thanks a lot!
> 
> -- Bala
> 
>>
>>
>> C.
>>
>>
>>>
>>> Request for your review and suggestions to make it better. I would like to
>>> thank Cedric for his time and help to figure out the issues.
>>>
>>> Balamuruhan S (5):
>>>   hw/ppc/pnv: incorrect homer and occ common area size
>>>   hw/ppc/pnv_xscom: PBA bar mask values are incorrect with homer/occ
>>>     sizes
>>>   hw/ppc/pnv_xscom: Power8 occ common area is in PBA BAR 3
>>>   hw/ppc/pnv_xscom: occ common area to be mapped only once
>>>   hw/ppc/pnv_xscom: add PBA BARs for Power8 slw image
>>>
>>>  hw/ppc/pnv_occ.c     |  2 +-
>>>  hw/ppc/pnv_xscom.c   | 37 +++++++++++++++++++++++++++----------
>>>  include/hw/ppc/pnv.h | 12 ++++++++----
>>>  3 files changed, 36 insertions(+), 15 deletions(-)
>>>
>>
> 

Re: [PATCH 0/5] ppc/pnv: fix Homer/Occ mappings on multichip systems

[Balamuruhan S]

On Thu, Nov 21, 2019 at 11:00:12AM +0100, Cédric Le Goater wrote:
> On 21/11/2019 10:11, Balamuruhan S wrote:
> > On Wed, Nov 20, 2019 at 08:46:30AM +0100, Cédric Le Goater wrote:
> >> Hello,
> >>
> >> On 19/11/2019 18:50, Balamuruhan S wrote:
> >>> Hi All,
> >>>
> >>> PowerNV fails to boot in multichip systems due to some misinterpretation
> >>> and mapping in Homer/Occ device models, this patchset fixes the
> >>> following,
> >>>
> >>>  - Homer size is 4MB per chip and Occ common area size is 8MB
> >>>  - Bar masks are used to calculate sizes of Homer/Occ in skiboot so
> >>>    return appropriate value
> >>>  - Occ common area is in BAR 3 on Power8 but wrongly mapped to BAR 2
> >>>    currently
> >>>  - OCC common area is shared across chips and should be mapped only once
> >>>    for multichip systems
> >>
> >> The first thing to address is the HOMER XSCOM region. 
> >>
> >> Introduce an empty skeleton for P8 and P9 with different mem ops handers
> >> because the registers have a different layout. From there, add the support
> >> for the different PBA* regs and move them out from the default XSCOM
> >> handlers. That should fix most of the current problems and it will provide 
> >> a nice framework for future extensions.
> > 
> > sure, I will work on it.
> > 
> >>
> >> Why not add the associated HOMER MMIO region while we are it ? the PBA
> >> registers have all the definitions we need and it will gives us access
> >> to the pstate table.
> > 
> > so, idea is to have HOMER MMIO for us to use it accessing pstate table / data
> > and HOMER XSCOM for homer associated xscom access for PBA* registers to
> > P8 and P9 respectively.
> 
> yes. 
> 
> >> Second is the OCC region. Do we need a XSCOM *or* a MMIO region ? This is 
> >> not clear. Please check skiboot. I think a MMIO region should be enough
> >> because this is how sensor data from the OCC is accessed.
> > 
> > Okay, I will do the change for OCC to use MMIO, and will check skiboot
> > for making it better.
> > 
> >>
> >> On that topic, we could define properties on the PnvOCC model for each 
> >> sensor and tune the value from the QEMU monitor. It really shouldn't be
> >> too complex.
> > 
> > How can we tune value from QEMU monitor ? This is new to me and will
> > need to check it. I remember you have advised this with the error
> > injection framework patches and Rashmica's patch that provides way to
> > use Qemu monitor to feed data, but I need to do some study.
> 
> 
> See Joel's patch which has a simple example :  
> 
>    patchwork.ozlabs.org/patch/1196519
> 
> It simply generates object properties : 
> 
> 
> +    for (led = 0; led < s->nr_leds; led++) {
> +        char *name;
> +
> +        name = g_strdup_printf("led%d", led);
> +        object_property_add(obj, name, "bool", pca9552_get_led, pca9552_set_led,
> +                            NULL, NULL, NULL);
> +    }
> 
> with defined get and set accessors. 
> 
> We could do the same for the OCC sensors with a table describing the 
> sensor layout. Accessors would just simply update the table. we could
> even trigger the OCC interrupt if needed.
> 
> This is the initial table :
> 
>   https://github.com/open-power/occ/blob/master/src/occ_405/sensor/sensor_info.c
> 
> Linux should be able to grab the values through hwmon just as on real HW.
> This is the case today for the DTS.

cool...

> 
> >>
> >> Also the same address is used, we should only map it once but we need 
> >> to invent something to know from which chip it is accessed.
> > 
> > This is something need to check how real hardware handles it while
> > accessing shared occ region from different chip and think how to make it
> > for us.
> 
> Yes. I suppose there is some chip id in the powerbus message.

:+1:

> 
> C.
> 
>   
> > 
> > Thanks a lot!
> > 
> > -- Bala
> > 
> >>
> >>
> >> C.
> >>
> >>
> >>>
> >>> Request for your review and suggestions to make it better. I would like to
> >>> thank Cedric for his time and help to figure out the issues.
> >>>
> >>> Balamuruhan S (5):
> >>>   hw/ppc/pnv: incorrect homer and occ common area size
> >>>   hw/ppc/pnv_xscom: PBA bar mask values are incorrect with homer/occ
> >>>     sizes
> >>>   hw/ppc/pnv_xscom: Power8 occ common area is in PBA BAR 3
> >>>   hw/ppc/pnv_xscom: occ common area to be mapped only once
> >>>   hw/ppc/pnv_xscom: add PBA BARs for Power8 slw image
> >>>
> >>>  hw/ppc/pnv_occ.c     |  2 +-
> >>>  hw/ppc/pnv_xscom.c   | 37 +++++++++++++++++++++++++++----------
> >>>  include/hw/ppc/pnv.h | 12 ++++++++----
> >>>  3 files changed, 36 insertions(+), 15 deletions(-)
> >>>
> >>
> > 
>