| Message ID | 20200426194533.26606-1-klaus@linux.vnet.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | hdata/memory.c: Fix "Inconsistent MSAREA" warnings | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (0f1937ef40fca0c3212a9dff1010b832a24fb063) |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | success | Test snowpatch/job/snowpatch-skiboot on branch master |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot-dco | success | Signed-off-by present |
Oliver, I was thinking a bit about this and decided to send a simple 'trivial fix' since a more complete one would require some re-work I think (besides, a trivial fix should apply cleanly on skiboot-6.6). Right we're creating memory-buffer@<min-addr> nodes under root, and those min-addr are calculated as part of the mmio ranges structure. If we wanted to add a add_memory_controller_p9p() function, wouldn't we want to re-use the same memory-buffer@ nodes to add the address-range attributes? The amount of rework for us to be able to do that seems significant and not so clear unless we join add_memory_controller_p9p() with add_memory_buffer_mmio() Perhaps we should make things clearer and add memory-buffer@<physical-chip-id> when parsing the address range array, and add mmio@<min-addr> nodes underneath them when parsing the mmio range? If you agree, I can try helping with that implementation.. Thanks, -Klaus On 4/26/2020 4:45 PM, Klaus Heinrich Kiwi wrote: > add_memory_buffer_mmio() should be exclusive to P9P (AXONE). > Running it on non P9P systems resulted in warnings such as: > > MS AREA: Inconsistent MSAREA version 40 for P9P system > > So check for PVR and quietly return if not P9P. > > Fixes: 38b5c3179 (Add support for memory-buffer mmio) > Cc: skiboot-stable@lists.ozlabs.org > Cc: Oliver O'Halloran <oohall@gmail.com> > Signed-off-by: Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com> > --- > hdata/memory.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hdata/memory.c b/hdata/memory.c > index 7ce927502..f2c440540 100755 > --- a/hdata/memory.c > +++ b/hdata/memory.c > @@ -514,6 +514,9 @@ static void add_memory_buffer_mmio(const struct HDIF_common_hdr *msarea) > struct dt_node *membuf; > uint64_t *reg, *flags; > > + if (PVR_TYPE(mfspr(SPR_PVR)) != PVR_TYPE_P9P) > + return; > + > if (be32_to_cpu(msarea->version) < 0x50) { > prlog(PR_WARNING, "MS AREA: Inconsistent MSAREA version %x for P9P system", > be32_to_cpu(msarea->version)); >
On 4/27/20 1:15 AM, Klaus Heinrich Kiwi wrote: > add_memory_buffer_mmio() should be exclusive to P9P (AXONE). > Running it on non P9P systems resulted in warnings such as: > > MS AREA: Inconsistent MSAREA version 40 for P9P system > > So check for PVR and quietly return if not P9P. > > Fixes: 38b5c3179 (Add support for memory-buffer mmio) > Cc: skiboot-stable@lists.ozlabs.org > Cc: Oliver O'Halloran <oohall@gmail.com> > Signed-off-by: Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com> > --- > hdata/memory.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hdata/memory.c b/hdata/memory.c > index 7ce927502..f2c440540 100755 > --- a/hdata/memory.c > +++ b/hdata/memory.c > @@ -514,6 +514,9 @@ static void add_memory_buffer_mmio(const struct HDIF_common_hdr *msarea) > struct dt_node *membuf; > uint64_t *reg, *flags; > > + if (PVR_TYPE(mfspr(SPR_PVR)) != PVR_TYPE_P9P) > + return; > + Is this supported *only* on P9P? I thought we are going to support this in future processors as well. Better way to skip warning in below if condition and return. -Vasant > if (be32_to_cpu(msarea->version) < 0x50) { > prlog(PR_WARNING, "MS AREA: Inconsistent MSAREA version %x for P9P system", > be32_to_cpu(msarea->version)); >
On 4/27/2020 6:12 AM, Vasant Hegde wrote: >> >> + if (PVR_TYPE(mfspr(SPR_PVR)) != PVR_TYPE_P9P) >> + return; >> + > > Is this supported *only* on P9P? I thought we are going to support this > in future processors as well. > > Better way to skip warning in below if condition and return. I was trying to be consistent with add_memory_controller() here (see my other reply to this thread), and I guess there is nothing stopping us from having a P9 (or older) system using a HDAT with a MSAREA => 0x50? -Klaus
On Mon, Apr 27, 2020 at 7:54 AM Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com> wrote: > > Oliver, > > I was thinking a bit about this and decided to send a simple 'trivial > fix' since a more complete one would require some re-work I think > (besides, a trivial fix should apply cleanly on skiboot-6.6). > > Right we're creating memory-buffer@<min-addr> nodes under root, and > those min-addr are calculated as part of the mmio ranges structure. > > If we wanted to add a add_memory_controller_p9p() function, wouldn't we > want to re-use the same memory-buffer@ nodes to add the address-range > attributes? The amount of rework for us to be able to do that seems > significant and not so clear unless we join add_memory_controller_p9p() > with add_memory_buffer_mmio() It doesn't look like a significant rework would be required. Return the dt node from add_buffer_mmio and pass that to add_memory_controller() and you're 90% done. > Perhaps we should make things clearer and add > memory-buffer@<physical-chip-id> when parsing the address range array, > and add mmio@<min-addr> nodes underneath them when parsing the mmio range? The address space defined by the top level DT node is the system physical address space. Any nodes directly under the root need to have a unit address (the @<addr> bit) which is a system physical address. Adding nodes with thing@<random_index> is broken and although we do that in a few places we shouldn't be and I don't want to add any more. This is all covered in the DT spec: https://www.devicetree.org/ > If you agree, I can try helping with that implementation.. Preemptive nak Oliver
On Mon, Apr 27, 2020 at 5:45 AM Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com> wrote: > > add_memory_buffer_mmio() should be exclusive to P9P (AXONE). > Running it on non P9P systems resulted in warnings such as: > > MS AREA: Inconsistent MSAREA version 40 for P9P system > > So check for PVR and quietly return if not P9P. > > Fixes: 38b5c3179 (Add support for memory-buffer mmio) > Cc: skiboot-stable@lists.ozlabs.org > Cc: Oliver O'Halloran <oohall@gmail.com> > Signed-off-by: Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com> > --- > hdata/memory.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/hdata/memory.c b/hdata/memory.c > index 7ce927502..f2c440540 100755 > --- a/hdata/memory.c > +++ b/hdata/memory.c > @@ -514,6 +514,9 @@ static void add_memory_buffer_mmio(const struct HDIF_common_hdr *msarea) > struct dt_node *membuf; > uint64_t *reg, *flags; > > + if (PVR_TYPE(mfspr(SPR_PVR)) != PVR_TYPE_P9P) > + return; > + > if (be32_to_cpu(msarea->version) < 0x50) { > prlog(PR_WARNING, "MS AREA: Inconsistent MSAREA version %x for P9P system", > be32_to_cpu(msarea->version)); > -- > 2.17.1 > This is going to break on p10, but whatever. Merged as 11d12c6fb60af42b89930fe776958f0eb208dd23
diff --git a/hdata/memory.c b/hdata/memory.c index 7ce927502..f2c440540 100755 --- a/hdata/memory.c +++ b/hdata/memory.c @@ -514,6 +514,9 @@ static void add_memory_buffer_mmio(const struct HDIF_common_hdr *msarea) struct dt_node *membuf; uint64_t *reg, *flags; + if (PVR_TYPE(mfspr(SPR_PVR)) != PVR_TYPE_P9P) + return; + if (be32_to_cpu(msarea->version) < 0x50) { prlog(PR_WARNING, "MS AREA: Inconsistent MSAREA version %x for P9P system", be32_to_cpu(msarea->version));
add_memory_buffer_mmio() should be exclusive to P9P (AXONE). Running it on non P9P systems resulted in warnings such as: MS AREA: Inconsistent MSAREA version 40 for P9P system So check for PVR and quietly return if not P9P. Fixes: 38b5c3179 (Add support for memory-buffer mmio) Cc: skiboot-stable@lists.ozlabs.org Cc: Oliver O'Halloran <oohall@gmail.com> Signed-off-by: Klaus Heinrich Kiwi <klaus@linux.vnet.ibm.com> --- hdata/memory.c | 3 +++ 1 file changed, 3 insertions(+)