| Message ID | 1504871765-15773-1-git-send-email-anju@linux.vnet.ibm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add nest_memory region to exports node | expand |
On Fri, Sep 8, 2017 at 9:56 PM, Anju T Sudhakar <anju@linux.vnet.ibm.com> wrote: > From: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> > > Adds nest_memory region to exports node, so that sysfs interface exposes > the nest-imc memory dump. This is really helpful incase of debugging nest-imc > counters data and verify whether the counter values are updated in the main > memory properly or not. > Also helps to analyse the values populated in the control block structure. > > Example: > > /proc/device-tree/ibm,opal/firmware/exports# ls > hdat_map imc_nest_chip_0 name phandle symbol_map > > /sys/firmware/opal/exports# ls > hdat_map imc_nest_chip_0 symbol_map > > /sys/firmware/opal/exports# hexdump imc_nest_chip_0 > 0000000 0000 0000 0000 47af 0000 0000 0000 b41e > 0000010 0000 0000 0400 7798 0000 0000 0000 0f6c > 0000020 0000 0000 0000 0000 0000 0000 0000 c206 > 0000030 0000 0000 0000 f17b 0000 0000 0000 9100 > 0000040 0000 0000 0000 0000 0000 0000 0000 b71e > 0000050 0000 0000 0400 6f98 0000 0000 0000 fc2b > *** > > Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> > Tested-by : Anju T Sudhakar <anju@linux.vnet.ibm.com> > --- > hw/imc.c | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/hw/imc.c b/hw/imc.c > index b29a2a1ba270..11df258601d1 100644 > --- a/hw/imc.c > +++ b/hw/imc.c > @@ -113,6 +113,7 @@ static char *compress_buf; > static size_t compress_buf_size; > const char **prop_to_fix(struct dt_node *node); > const char *props_to_fix[] = {"events", NULL}; > +static uint32_t imc_nest_offset; > > static bool is_nest_mem_initialized(struct imc_chip_cb *ptr) > { > @@ -416,6 +417,33 @@ static void imc_dt_update_nest_node(struct dt_node *dev) > } > } > > +static void add_nest_mem_exports_node(struct dt_node *root, struct dt_node *dev) > +{ > + struct proc_chip *chip; > + struct dt_node *node; > + uint64_t baddr; > + const struct dt_property *type; > + char namebuf[32]; > + int i=0; > + > + dt_for_each_compatible(dev, node, "ibm,imc-counters") { > + type = dt_find_property(node, "type"); > + if (type && is_nest_node(node)) > + imc_nest_offset = dt_prop_get_u32(node, "offset"); > + } > + > + node = dt_find_by_name(root, "exports"); You could remove the param "root" from this function and rewrite it as: dt_find_by_name(dt_opal, "exports"); > + if (!node) > + return; > + > + for_each_chip(chip) { > + snprintf(namebuf, sizeof(namebuf), "imc_nest_chip_%d", i++); Can you use the chip_id (in hex) here rather than a counter? > + baddr = chip->homer_base; > + baddr += imc_nest_offset; > + dt_add_property_u64s(node, namebuf, baddr, 0x40000); Why is the size hard coded rather than using the imc-nest-size property? > + } > +} > + > /* > * Load the IMC pnor partition and find the appropriate sub-partition > * based on the platform's PVR. > @@ -522,6 +550,8 @@ imc_mambo: > goto err; > } > > + add_nest_mem_exports_node(dt_root, dev); > + > free(compress_buf); > return; > err: > -- > 2.7.4 > > _______________________________________________ > Skiboot mailing list > Skiboot@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/skiboot
On Tuesday 12 September 2017 10:20 AM, Oliver wrote: > On Fri, Sep 8, 2017 at 9:56 PM, Anju T Sudhakar <anju@linux.vnet.ibm.com> wrote: >> From: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> >> >> Adds nest_memory region to exports node, so that sysfs interface exposes >> the nest-imc memory dump. This is really helpful incase of debugging nest-imc >> counters data and verify whether the counter values are updated in the main >> memory properly or not. >> Also helps to analyse the values populated in the control block structure. >> >> Example: >> >> /proc/device-tree/ibm,opal/firmware/exports# ls >> hdat_map imc_nest_chip_0 name phandle symbol_map >> >> /sys/firmware/opal/exports# ls >> hdat_map imc_nest_chip_0 symbol_map >> >> /sys/firmware/opal/exports# hexdump imc_nest_chip_0 >> 0000000 0000 0000 0000 47af 0000 0000 0000 b41e >> 0000010 0000 0000 0400 7798 0000 0000 0000 0f6c >> 0000020 0000 0000 0000 0000 0000 0000 0000 c206 >> 0000030 0000 0000 0000 f17b 0000 0000 0000 9100 >> 0000040 0000 0000 0000 0000 0000 0000 0000 b71e >> 0000050 0000 0000 0400 6f98 0000 0000 0000 fc2b >> *** >> >> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> >> Tested-by : Anju T Sudhakar <anju@linux.vnet.ibm.com> >> --- >> hw/imc.c | 30 ++++++++++++++++++++++++++++++ >> 1 file changed, 30 insertions(+) >> >> diff --git a/hw/imc.c b/hw/imc.c >> index b29a2a1ba270..11df258601d1 100644 >> --- a/hw/imc.c >> +++ b/hw/imc.c >> @@ -113,6 +113,7 @@ static char *compress_buf; >> static size_t compress_buf_size; >> const char **prop_to_fix(struct dt_node *node); >> const char *props_to_fix[] = {"events", NULL}; >> +static uint32_t imc_nest_offset; >> >> static bool is_nest_mem_initialized(struct imc_chip_cb *ptr) >> { >> @@ -416,6 +417,33 @@ static void imc_dt_update_nest_node(struct dt_node *dev) >> } >> } >> >> +static void add_nest_mem_exports_node(struct dt_node *root, struct dt_node *dev) >> +{ >> + struct proc_chip *chip; >> + struct dt_node *node; >> + uint64_t baddr; >> + const struct dt_property *type; >> + char namebuf[32]; >> + int i=0; >> + >> + dt_for_each_compatible(dev, node, "ibm,imc-counters") { >> + type = dt_find_property(node, "type"); >> + if (type && is_nest_node(node)) >> + imc_nest_offset = dt_prop_get_u32(node, "offset"); >> + } >> + >> + node = dt_find_by_name(root, "exports"); > You could remove the param "root" from this function and rewrite it as: > > dt_find_by_name(dt_opal, "exports"); Yes, make sense. B/w you mean "dt_root" right? > >> + if (!node) >> + return; >> + >> + for_each_chip(chip) { >> + snprintf(namebuf, sizeof(namebuf), "imc_nest_chip_%d", i++); > Can you use the chip_id (in hex) here rather than a counter? sure. > >> + baddr = chip->homer_base; >> + baddr += imc_nest_offset; >> + dt_add_property_u64s(node, namebuf, baddr, 0x40000); > Why is the size hard coded rather than using the imc-nest-size property? Yep fixed it in v2 of the patch Thanks for the review Maddy > >> + } >> +} >> + >> /* >> * Load the IMC pnor partition and find the appropriate sub-partition >> * based on the platform's PVR. >> @@ -522,6 +550,8 @@ imc_mambo: >> goto err; >> } >> >> + add_nest_mem_exports_node(dt_root, dev); >> + >> free(compress_buf); >> return; >> err: >> -- >> 2.7.4 >> >> _______________________________________________ >> Skiboot mailing list >> Skiboot@lists.ozlabs.org >> https://lists.ozlabs.org/listinfo/skiboot
On Tue, Sep 12, 2017 at 3:02 PM, Madhavan Srinivasan <maddy@linux.vnet.ibm.com> wrote: > > > On Tuesday 12 September 2017 10:20 AM, Oliver wrote: >> >> On Fri, Sep 8, 2017 at 9:56 PM, Anju T Sudhakar <anju@linux.vnet.ibm.com> >> wrote: >>> >>> From: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> >>> >>> Adds nest_memory region to exports node, so that sysfs interface exposes >>> the nest-imc memory dump. This is really helpful incase of debugging >>> nest-imc >>> counters data and verify whether the counter values are updated in the >>> main >>> memory properly or not. >>> Also helps to analyse the values populated in the control block >>> structure. >>> >>> Example: >>> >>> /proc/device-tree/ibm,opal/firmware/exports# ls >>> hdat_map imc_nest_chip_0 name phandle symbol_map >>> >>> /sys/firmware/opal/exports# ls >>> hdat_map imc_nest_chip_0 symbol_map >>> >>> /sys/firmware/opal/exports# hexdump imc_nest_chip_0 >>> 0000000 0000 0000 0000 47af 0000 0000 0000 b41e >>> 0000010 0000 0000 0400 7798 0000 0000 0000 0f6c >>> 0000020 0000 0000 0000 0000 0000 0000 0000 c206 >>> 0000030 0000 0000 0000 f17b 0000 0000 0000 9100 >>> 0000040 0000 0000 0000 0000 0000 0000 0000 b71e >>> 0000050 0000 0000 0400 6f98 0000 0000 0000 fc2b >>> *** >>> >>> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> >>> Tested-by : Anju T Sudhakar <anju@linux.vnet.ibm.com> >>> --- >>> hw/imc.c | 30 ++++++++++++++++++++++++++++++ >>> 1 file changed, 30 insertions(+) >>> >>> diff --git a/hw/imc.c b/hw/imc.c >>> index b29a2a1ba270..11df258601d1 100644 >>> --- a/hw/imc.c >>> +++ b/hw/imc.c >>> @@ -113,6 +113,7 @@ static char *compress_buf; >>> static size_t compress_buf_size; >>> const char **prop_to_fix(struct dt_node *node); >>> const char *props_to_fix[] = {"events", NULL}; >>> +static uint32_t imc_nest_offset; >>> >>> static bool is_nest_mem_initialized(struct imc_chip_cb *ptr) >>> { >>> @@ -416,6 +417,33 @@ static void imc_dt_update_nest_node(struct dt_node >>> *dev) >>> } >>> } >>> >>> +static void add_nest_mem_exports_node(struct dt_node *root, struct >>> dt_node *dev) >>> +{ >>> + struct proc_chip *chip; >>> + struct dt_node *node; >>> + uint64_t baddr; >>> + const struct dt_property *type; >>> + char namebuf[32]; >>> + int i=0; >>> + >>> + dt_for_each_compatible(dev, node, "ibm,imc-counters") { >>> + type = dt_find_property(node, "type"); >>> + if (type && is_nest_node(node)) >>> + imc_nest_offset = dt_prop_get_u32(node, >>> "offset"); >>> + } >>> + >>> + node = dt_find_by_name(root, "exports"); >> >> You could remove the param "root" from this function and rewrite it as: >> >> dt_find_by_name(dt_opal, "exports"); > > > Yes, make sense. B/w you mean "dt_root" right? No, but dt_root would also work. dt_opal refers to the /ibm,opal/ node so it'll be slightly faster since there's a smaller search space. It's possible we might add another node caleld "exports" elsewhere in the tree some day so using dt_opal is probably safer too. > >> >>> + if (!node) >>> + return; >>> + >>> + for_each_chip(chip) { >>> + snprintf(namebuf, sizeof(namebuf), "imc_nest_chip_%d", >>> i++); >> >> Can you use the chip_id (in hex) here rather than a counter? > > > sure. > >> >>> + baddr = chip->homer_base; >>> + baddr += imc_nest_offset; >>> + dt_add_property_u64s(node, namebuf, baddr, 0x40000); >> >> Why is the size hard coded rather than using the imc-nest-size property? > > > Yep fixed it in v2 of the patch > > Thanks for the review no worries. > Maddy > > >> >>> + } >>> +} >>> + >>> /* >>> * Load the IMC pnor partition and find the appropriate sub-partition >>> * based on the platform's PVR. >>> @@ -522,6 +550,8 @@ imc_mambo: >>> goto err; >>> } >>> >>> + add_nest_mem_exports_node(dt_root, dev); >>> + >>> free(compress_buf); >>> return; >>> err: >>> -- >>> 2.7.4 >>> >>> _______________________________________________ >>> Skiboot mailing list >>> Skiboot@lists.ozlabs.org >>> https://lists.ozlabs.org/listinfo/skiboot > >
On Tuesday 12 September 2017 10:37 AM, Oliver wrote: > On Tue, Sep 12, 2017 at 3:02 PM, Madhavan Srinivasan > <maddy@linux.vnet.ibm.com> wrote: >> >> On Tuesday 12 September 2017 10:20 AM, Oliver wrote: >>> On Fri, Sep 8, 2017 at 9:56 PM, Anju T Sudhakar <anju@linux.vnet.ibm.com> >>> wrote: >>>> From: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> >>>> >>>> Adds nest_memory region to exports node, so that sysfs interface exposes >>>> the nest-imc memory dump. This is really helpful incase of debugging >>>> nest-imc >>>> counters data and verify whether the counter values are updated in the >>>> main >>>> memory properly or not. >>>> Also helps to analyse the values populated in the control block >>>> structure. >>>> >>>> Example: >>>> >>>> /proc/device-tree/ibm,opal/firmware/exports# ls >>>> hdat_map imc_nest_chip_0 name phandle symbol_map >>>> >>>> /sys/firmware/opal/exports# ls >>>> hdat_map imc_nest_chip_0 symbol_map >>>> >>>> /sys/firmware/opal/exports# hexdump imc_nest_chip_0 >>>> 0000000 0000 0000 0000 47af 0000 0000 0000 b41e >>>> 0000010 0000 0000 0400 7798 0000 0000 0000 0f6c >>>> 0000020 0000 0000 0000 0000 0000 0000 0000 c206 >>>> 0000030 0000 0000 0000 f17b 0000 0000 0000 9100 >>>> 0000040 0000 0000 0000 0000 0000 0000 0000 b71e >>>> 0000050 0000 0000 0400 6f98 0000 0000 0000 fc2b >>>> *** >>>> >>>> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> >>>> Tested-by : Anju T Sudhakar <anju@linux.vnet.ibm.com> >>>> --- >>>> hw/imc.c | 30 ++++++++++++++++++++++++++++++ >>>> 1 file changed, 30 insertions(+) >>>> >>>> diff --git a/hw/imc.c b/hw/imc.c >>>> index b29a2a1ba270..11df258601d1 100644 >>>> --- a/hw/imc.c >>>> +++ b/hw/imc.c >>>> @@ -113,6 +113,7 @@ static char *compress_buf; >>>> static size_t compress_buf_size; >>>> const char **prop_to_fix(struct dt_node *node); >>>> const char *props_to_fix[] = {"events", NULL}; >>>> +static uint32_t imc_nest_offset; >>>> >>>> static bool is_nest_mem_initialized(struct imc_chip_cb *ptr) >>>> { >>>> @@ -416,6 +417,33 @@ static void imc_dt_update_nest_node(struct dt_node >>>> *dev) >>>> } >>>> } >>>> >>>> +static void add_nest_mem_exports_node(struct dt_node *root, struct >>>> dt_node *dev) >>>> +{ >>>> + struct proc_chip *chip; >>>> + struct dt_node *node; >>>> + uint64_t baddr; >>>> + const struct dt_property *type; >>>> + char namebuf[32]; >>>> + int i=0; >>>> + >>>> + dt_for_each_compatible(dev, node, "ibm,imc-counters") { >>>> + type = dt_find_property(node, "type"); >>>> + if (type && is_nest_node(node)) >>>> + imc_nest_offset = dt_prop_get_u32(node, >>>> "offset"); >>>> + } >>>> + >>>> + node = dt_find_by_name(root, "exports"); >>> You could remove the param "root" from this function and rewrite it as: >>> >>> dt_find_by_name(dt_opal, "exports"); >> >> Yes, make sense. B/w you mean "dt_root" right? > No, but dt_root would also work. dt_opal refers to the /ibm,opal/ node > so it'll be slightly faster since there's a smaller search space. It's Yep. Thats right. > possible we might add another node caleld "exports" elsewhere in the > tree some day so using dt_opal is probably safer too. Ok thanks for details. Will make change. Maddy > >>>> + if (!node) >>>> + return; >>>> + >>>> + for_each_chip(chip) { >>>> + snprintf(namebuf, sizeof(namebuf), "imc_nest_chip_%d", >>>> i++); >>> Can you use the chip_id (in hex) here rather than a counter? >> >> sure. >> >>>> + baddr = chip->homer_base; >>>> + baddr += imc_nest_offset; >>>> + dt_add_property_u64s(node, namebuf, baddr, 0x40000); >>> Why is the size hard coded rather than using the imc-nest-size property? >> >> Yep fixed it in v2 of the patch >> >> Thanks for the review > no worries. > >> Maddy >> >> >>>> + } >>>> +} >>>> + >>>> /* >>>> * Load the IMC pnor partition and find the appropriate sub-partition >>>> * based on the platform's PVR. >>>> @@ -522,6 +550,8 @@ imc_mambo: >>>> goto err; >>>> } >>>> >>>> + add_nest_mem_exports_node(dt_root, dev); >>>> + >>>> free(compress_buf); >>>> return; >>>> err: >>>> -- >>>> 2.7.4 >>>> >>>> _______________________________________________ >>>> Skiboot mailing list >>>> Skiboot@lists.ozlabs.org >>>> https://lists.ozlabs.org/listinfo/skiboot >>
diff --git a/hw/imc.c b/hw/imc.c index b29a2a1ba270..11df258601d1 100644 --- a/hw/imc.c +++ b/hw/imc.c @@ -113,6 +113,7 @@ static char *compress_buf; static size_t compress_buf_size; const char **prop_to_fix(struct dt_node *node); const char *props_to_fix[] = {"events", NULL}; +static uint32_t imc_nest_offset; static bool is_nest_mem_initialized(struct imc_chip_cb *ptr) { @@ -416,6 +417,33 @@ static void imc_dt_update_nest_node(struct dt_node *dev) } } +static void add_nest_mem_exports_node(struct dt_node *root, struct dt_node *dev) +{ + struct proc_chip *chip; + struct dt_node *node; + uint64_t baddr; + const struct dt_property *type; + char namebuf[32]; + int i=0; + + dt_for_each_compatible(dev, node, "ibm,imc-counters") { + type = dt_find_property(node, "type"); + if (type && is_nest_node(node)) + imc_nest_offset = dt_prop_get_u32(node, "offset"); + } + + node = dt_find_by_name(root, "exports"); + if (!node) + return; + + for_each_chip(chip) { + snprintf(namebuf, sizeof(namebuf), "imc_nest_chip_%d", i++); + baddr = chip->homer_base; + baddr += imc_nest_offset; + dt_add_property_u64s(node, namebuf, baddr, 0x40000); + } +} + /* * Load the IMC pnor partition and find the appropriate sub-partition * based on the platform's PVR. @@ -522,6 +550,8 @@ imc_mambo: goto err; } + add_nest_mem_exports_node(dt_root, dev); + free(compress_buf); return; err: