| Message ID | 20190327123342.GA14278@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | core/opal: Fix address-cells in opal node in DT | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch master (050d8165ab05b6d9cdf4bfee42d9776969c77029) |
| 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 |
Hello! Excuse me, could anyone see this patch? any notes? Thanks 27.03.2019 15:33, Maxim Polyakov пишет: > This patch fix this FWTS warning: > > Warning (reg_format): "reg" property in /ibm,opal/flash at 0 has > invalid length (8 bytes) (#address-cells == 0, #size-cells == 0) > > See more in https://github.com/open-power/skiboot/issues/47 > > It has been tested on Vesnin P8 server. > > Signed-off-by: Maxim Polyakov <m.polyakov@yadro.com> > --- > core/opal.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/core/opal.c b/core/opal.c > index 624e2a2..9271b07 100644 > --- a/core/opal.c > +++ b/core/opal.c > @@ -406,7 +406,7 @@ void add_opal_node(void) > (uint64_t)(cpu_max_pir + 1) * STACK_SIZE) - SKIBOOT_BASE; > > opal_node = dt_new_check(dt_root, "ibm,opal"); > - dt_add_property_cells(opal_node, "#address-cells", 0); > + dt_add_property_cells(opal_node, "#address-cells", 1); > dt_add_property_cells(opal_node, "#size-cells", 0); > > if (proc_gen < proc_gen_p9) >
On 03/27/2019 06:03 PM, Maxim Polyakov wrote: > This patch fix this FWTS warning: > > Warning (reg_format): "reg" property in /ibm,opal/flash at 0 has > invalid length (8 bytes) (#address-cells == 0, #size-cells == 0) > > See more in https://github.com/open-power/skiboot/issues/47 > > It has been tested on Vesnin P8 server. > > Signed-off-by: Maxim Polyakov <m.polyakov@yadro.com> > --- > core/opal.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/core/opal.c b/core/opal.c > index 624e2a2..9271b07 100644 > --- a/core/opal.c > +++ b/core/opal.c > @@ -406,7 +406,7 @@ void add_opal_node(void) > (uint64_t)(cpu_max_pir + 1) * STACK_SIZE) - SKIBOOT_BASE; > > opal_node = dt_new_check(dt_root, "ibm,opal"); > - dt_add_property_cells(opal_node, "#address-cells", 0); > + dt_add_property_cells(opal_node, "#address-cells", 1); Well, it will help to silence dtc warning. But as mentioned in github, `reg` property inside `flash` node has just `size` no address. So this is not correct. But if we add "#size-cells" = 1 then its like telling you have `size` without address.. which is also wrong. May be we can just add this patch to keep dtc happy. @jk, any thoughts? -Vasant > dt_add_property_cells(opal_node, "#size-cells", 0); > > if (proc_gen < proc_gen_p9) >
09.04.2019 19:36, Vasant Hegde пишет: > On 03/27/2019 06:03 PM, Maxim Polyakov wrote: >> This patch fix this FWTS warning: >> >> Warning (reg_format): "reg" property in /ibm,opal/flash at 0 has >> invalid length (8 bytes) (#address-cells == 0, #size-cells == 0) >> >> See more in https://github.com/open-power/skiboot/issues/47 >> >> It has been tested on Vesnin P8 server. >> >> Signed-off-by: Maxim Polyakov <m.polyakov@yadro.com> >> --- >> core/opal.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/core/opal.c b/core/opal.c >> index 624e2a2..9271b07 100644 >> --- a/core/opal.c >> +++ b/core/opal.c >> @@ -406,7 +406,7 @@ void add_opal_node(void) >> (uint64_t)(cpu_max_pir + 1) * STACK_SIZE) - SKIBOOT_BASE; >> >> opal_node = dt_new_check(dt_root, "ibm,opal"); >> - dt_add_property_cells(opal_node, "#address-cells", 0); >> + dt_add_property_cells(opal_node, "#address-cells", 1); > > Well, it will help to silence dtc warning. But as mentioned in github, `reg` property inside > `flash` node has just `size` no address. So this is not correct. > > But if we add "#size-cells" = 1 then its like telling you have `size` without address.. which is also wrong. > > May be we can just add this patch to keep dtc happy. It would be great for us, because this patch will solve our problem with firmware certification. > > @jk, any thoughts? ? > > -Vasant > >> dt_add_property_cells(opal_node, "#size-cells", 0); >> >> if (proc_gen < proc_gen_p9) >> >
Hi all, > > This patch fix this FWTS warning: > > > > Warning (reg_format): "reg" property in /ibm,opal/flash at 0 has > > invalid length (8 bytes) (#address-cells == 0, #size-cells == 0) This is a bit of a tricky one. The ibm,opal node doesn't reference a "bus" of any kind, so the #address-cells and #size-cells being zero is correct. What we should have done for the flash node is used some property (other than 'reg', which has a specific meaning) for the size of the flash device. However, even if we patches that in OPAL and Linux now, renaming the property would break compatibility with existing OSes. > But if we add "#size-cells" = 1 then its like telling you have `size` > without address.. which is also wrong. That's not too much of an issue; the implication that this introduces is that any 'reg' node under /ibm,opal is a one-cell size value - which may not be too bad a thing. > May be we can just add this patch to keep dtc happy. > > @jk, any thoughts? It's not great, but it's probably not too awful either. Silencing the warning might be a reasonable justification here, and it could be argued that nodes under ibm,opal could reasonably describe sizes of things? Cheers, Jeremy
On 04/15/2019 07:41 PM, Jeremy Kerr wrote: > Hi all, > >>> This patch fix this FWTS warning: >>> >>> Warning (reg_format): "reg" property in /ibm,opal/flash at 0 has >>> invalid length (8 bytes) (#address-cells == 0, #size-cells == 0) > > This is a bit of a tricky one. The ibm,opal node doesn't reference a > "bus" of any kind, so the #address-cells and #size-cells being zero is > correct. What we should have done for the flash node is used some > property (other than 'reg', which has a specific meaning) for the size > of the flash device. > > However, even if we patches that in OPAL and Linux now, renaming the > property would break compatibility with existing OSes. > >> But if we add "#size-cells" = 1 then its like telling you have `size` >> without address.. which is also wrong. > > That's not too much of an issue; the implication that this introduces is > that any 'reg' node under /ibm,opal is a one-cell size value - which may > not be too bad a thing. > >> May be we can just add this patch to keep dtc happy. >> >> @jk, any thoughts? > > It's not great, but it's probably not too awful either. Silencing the > warning might be a reasonable justification here, and it could be argued > that nodes under ibm,opal could reasonably describe sizes of things? Agree. Lets add size property as "1" and silence warning. @Maxim, Can you respin the patch? And fat comment explaining the rational behind this fix. -Vasant
19.04.2019 11:59, Vasant Hegde пишет: > On 04/15/2019 07:41 PM, Jeremy Kerr wrote: >> Hi all, >> >>>> This patch fix this FWTS warning: >>>> >>>> Warning (reg_format): "reg" property in /ibm,opal/flash at 0 has >>>> invalid length (8 bytes) (#address-cells == 0, #size-cells == 0) >> >> This is a bit of a tricky one. The ibm,opal node doesn't reference a >> "bus" of any kind, so the #address-cells and #size-cells being zero is >> correct. What we should have done for the flash node is used some >> property (other than 'reg', which has a specific meaning) for the size >> of the flash device. >> >> However, even if we patches that in OPAL and Linux now, renaming the >> property would break compatibility with existing OSes. >> >>> But if we add "#size-cells" = 1 then its like telling you have `size` >>> without address.. which is also wrong. >> >> That's not too much of an issue; the implication that this introduces is >> that any 'reg' node under /ibm,opal is a one-cell size value - which may >> not be too bad a thing. >> >>> May be we can just add this patch to keep dtc happy. >>> >>> @jk, any thoughts? >> >> It's not great, but it's probably not too awful either. Silencing the >> warning might be a reasonable justification here, and it could be argued >> that nodes under ibm,opal could reasonably describe sizes of things? > > Agree. Lets add size property as "1" and silence warning. > > @Maxim, Can you respin the patch? And fat comment explaining the rational behind this fix. Ok. I updated the patch in https://patchwork.ozlabs.org/patch/1090093/ > > > -Vasant >
diff --git a/core/opal.c b/core/opal.c index 624e2a2..9271b07 100644 --- a/core/opal.c +++ b/core/opal.c @@ -406,7 +406,7 @@ void add_opal_node(void) (uint64_t)(cpu_max_pir + 1) * STACK_SIZE) - SKIBOOT_BASE; opal_node = dt_new_check(dt_root, "ibm,opal"); - dt_add_property_cells(opal_node, "#address-cells", 0); + dt_add_property_cells(opal_node, "#address-cells", 1); dt_add_property_cells(opal_node, "#size-cells", 0); if (proc_gen < proc_gen_p9)
This patch fix this FWTS warning: Warning (reg_format): "reg" property in /ibm,opal/flash at 0 has invalid length (8 bytes) (#address-cells == 0, #size-cells == 0) See more in https://github.com/open-power/skiboot/issues/47 It has been tested on Vesnin P8 server. Signed-off-by: Maxim Polyakov <m.polyakov@yadro.com> --- core/opal.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)