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

[Madhavan Srinivasan]

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?

>
>> +       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 at lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/skiboot