[Skiboot] [PATCH] Add nest_memory region to exports node

[Oliver]

On Tue, Sep 12, 2017 at 3:02 PM, Madhavan Srinivasan
<maddy at 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 at linux.vnet.ibm.com>
>> wrote:
>>>
>>> From: Madhavan Srinivasan <maddy at 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 at linux.vnet.ibm.com>
>>> Tested-by : Anju T Sudhakar <anju at 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 at lists.ozlabs.org
>>> https://lists.ozlabs.org/listinfo/skiboot
>
>