| Message ID | 1438587228-5967-4-git-send-email-maddy@linux.vnet.ibm.com |
|---|---|
| State | Changes Requested |
| Headers | show |
Hi, Apologies for having taken so long to get around to the device tree side of these. A few thoughts, relevant to both the kernel and skiboot side: Could we have a 'compatible' property like what we see in other parts of the DT? That way, when we get new chips, old kernels will be able to act appropriately, and future kernels with support for future chips will also work on old chips. I realise that 'compatible' is traditionally used for devices ([1]), but I think it will still be valid and useful in this case. Another thought along that line - is it possible to write the kernel side as a generic driver that just binds to things in the DT, rather than its current implementation? I'm not sure if this is possible with hooking into the perf infrastructure in the kernel, but if it is possible it'd be a much nicer solution. [1] http://devicetree.org/Device_Tree_Usage#Understanding_the_compatible_Property On Mon, 2015-08-03 at 13:03 +0530, Madhavan Srinivasan wrote: > OPAL sends two set of information to kernel. > 1) Information about the per-chip PORE_SLW_IMA HOMER offset to kernel, > 2) Information about each support Nest units and their events. > > Device tree Information Layout for Nest Instrumentation: > > /- > | > -ima-chip@<chip-id> > | > - Homer base + SLW Offset > - Size > - ... > -<Nest Units> > | > - <events> > - unit.<events>.unit > - scale.<events>.scale > - ... > > In this patch, code added to pass per-chip PORE_SLW_IMA HOMER offset to kernel > as part of device tree entry under "ima-chip@<chip-id>" directory > > Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> > --- > hw/nest.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/hw/nest.c b/hw/nest.c > index 9115e5c7fa26..3bc16aed49b5 100644 > --- a/hw/nest.c > +++ b/hw/nest.c > @@ -109,10 +109,56 @@ int load_catalogue_lid(int loaded) > > void nest_pmu_init(int loaded) > { > + struct proc_chip *chip; > + struct dt_node *dev, *chip_dev; > + u64 addr=0; > + > if (load_catalogue_lid(loaded)) { > printf("Nest_IMA: IMA Catalog lid failed to load\n"); > return; > } > > + /* > + * Now that we have Chip level events enabled, > + * lets create DT entries. This is a top level directory under / > + * for nest pmu units. > + */ > + dev = dt_new(dt_root, "nest-ima"); > + if (!dev) { > + prlog(PR_DEBUG,"Nest_IMA: nest-ima dev creation failed \n"); > + return; > + } > + > + /* > + * Start exposing per-chip SLW_IMA FW offset in HOMER Region. > + */ > + for_each_chip(chip) { > + chip_dev = dt_new_addr(dev, "ima-chip", chip->id); > + if (!chip_dev) { > + prlog(PR_DEBUG,"Nest_IMA: ima-chip dev creation failed \n"); > + return; > + } > + > + /* > + * Design: > + * PORE_SLW_IMA FW will program Nest counters with > + * pre-defined set of events (provided in catalog) and dump > + * counter data in a fixed HOMER offset. > + * > + * HOMER Region layout has reserved memory for > + * PORE_SLW_IMA FW starting from 0x320000 to 0x39FFFF > + * > + * PORE_SLW_IMA uses this region to dump different Nest unit > + * counter data. This address is passed to kernel as base > + * address to map. Individual counter offsets are passed > + * in the event file. > + */ > + dt_add_property_cells(chip_dev, "ibm,chip-id", chip->id); > + addr = chip->homer_base + SLW_IMA_OFFSET; > + dt_add_property_u64(chip_dev, "reg", addr); > + dt_add_property_cells(chip_dev, "size", SLW_IMA_SIZE); > + dt_add_property(chip_dev, "ibm,ima-chip", NULL, 0); > + } > + > return; > }
On Tuesday 11 August 2015 06:48 AM, Daniel Axtens wrote: > Hi, > > Apologies for having taken so long to get around to the device tree side > of these. No issues. Thanks for taking time and reviewing. > > A few thoughts, relevant to both the kernel and skiboot side: > > Could we have a 'compatible' property like what we see in other parts of > the DT? That way, when we get new chips, old kernels will be able to act > appropriately, and future kernels with support for future chips will > also work on old chips. > > I realise that 'compatible' is traditionally used for devices ([1]), but > I think it will still be valid and useful in this case. > > Another thought along that line - is it possible to write the kernel > side as a generic driver that just binds to things in the DT, rather > than its current implementation? I'm not sure if this is possible with Yes. Kernel side patchset is designed to have a generic driver code, which will parse the device tree and create nest pmu drivers. Kernel side patchset does not have any specific processor type checks, it only check for the device tree entries for the nest units created by OPAL side. This way kernel side will need no changes when a new nest unit is added or remove in the device tree. i.e, Today, OPAL side patchset creates device tree entries for nest units like MCS, PowerBus, Xlink and Alink. Tomorrow, if CAPP unit is added (by creating device tree entries) in OPAL side, kernel side will pick it up without any modifications. In the same way if we want to remove one of the units says Alink, OPAl side just need to remove device tree entries created for Alink. > hooking into the perf infrastructure in the kernel, but if it is > possible it'd be a much nicer solution. Kernel side patchset is intended for perf support. Cover letter in the kernel side patchset will explain the perf tool usage. On the 'compatible' property, Yes. we can add it to the device tree. But wondering how/when to use it, since kernel side does not have any 'processor type' checks currently and OPAL side patchset does not pass any data structures to kernel side. Maddy > > [1] > http://devicetree.org/Device_Tree_Usage#Understanding_the_compatible_Property > > > On Mon, 2015-08-03 at 13:03 +0530, Madhavan Srinivasan wrote: >> OPAL sends two set of information to kernel. >> 1) Information about the per-chip PORE_SLW_IMA HOMER offset to kernel, >> 2) Information about each support Nest units and their events. >> >> Device tree Information Layout for Nest Instrumentation: >> >> /- >> | >> -ima-chip@<chip-id> >> | >> - Homer base + SLW Offset >> - Size >> - ... >> -<Nest Units> >> | >> - <events> >> - unit.<events>.unit >> - scale.<events>.scale >> - ... >> >> In this patch, code added to pass per-chip PORE_SLW_IMA HOMER offset to kernel >> as part of device tree entry under "ima-chip@<chip-id>" directory >> >> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> >> --- >> hw/nest.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 46 insertions(+) >> >> diff --git a/hw/nest.c b/hw/nest.c >> index 9115e5c7fa26..3bc16aed49b5 100644 >> --- a/hw/nest.c >> +++ b/hw/nest.c >> @@ -109,10 +109,56 @@ int load_catalogue_lid(int loaded) >> >> void nest_pmu_init(int loaded) >> { >> + struct proc_chip *chip; >> + struct dt_node *dev, *chip_dev; >> + u64 addr=0; >> + >> if (load_catalogue_lid(loaded)) { >> printf("Nest_IMA: IMA Catalog lid failed to load\n"); >> return; >> } >> >> + /* >> + * Now that we have Chip level events enabled, >> + * lets create DT entries. This is a top level directory under / >> + * for nest pmu units. >> + */ >> + dev = dt_new(dt_root, "nest-ima"); >> + if (!dev) { >> + prlog(PR_DEBUG,"Nest_IMA: nest-ima dev creation failed \n"); >> + return; >> + } >> + >> + /* >> + * Start exposing per-chip SLW_IMA FW offset in HOMER Region. >> + */ >> + for_each_chip(chip) { >> + chip_dev = dt_new_addr(dev, "ima-chip", chip->id); >> + if (!chip_dev) { >> + prlog(PR_DEBUG,"Nest_IMA: ima-chip dev creation failed \n"); >> + return; >> + } >> + >> + /* >> + * Design: >> + * PORE_SLW_IMA FW will program Nest counters with >> + * pre-defined set of events (provided in catalog) and dump >> + * counter data in a fixed HOMER offset. >> + * >> + * HOMER Region layout has reserved memory for >> + * PORE_SLW_IMA FW starting from 0x320000 to 0x39FFFF >> + * >> + * PORE_SLW_IMA uses this region to dump different Nest unit >> + * counter data. This address is passed to kernel as base >> + * address to map. Individual counter offsets are passed >> + * in the event file. >> + */ >> + dt_add_property_cells(chip_dev, "ibm,chip-id", chip->id); >> + addr = chip->homer_base + SLW_IMA_OFFSET; >> + dt_add_property_u64(chip_dev, "reg", addr); >> + dt_add_property_cells(chip_dev, "size", SLW_IMA_SIZE); >> + dt_add_property(chip_dev, "ibm,ima-chip", NULL, 0); >> + } >> + >> return; >> }
> Yes. Kernel side patchset is designed to have a generic driver code, > which will parse the device tree and create nest pmu drivers. > Kernel side patchset does not have any specific processor type > checks, it only check for the device tree entries for the nest units > created by OPAL side. This way kernel side will need no changes > when a new nest unit is added or remove in the device tree. > This is not quite what I mean by a generic driver. By a generic driver I mean a driver that uses an of_device_id table to be automatically bound to devices in the device tree. I've found [1] to be a very useful introduction. If you can implement the kernel side as this, I think it will dramatically simplify things: it will automatically do detection for you, without you having to write code to to that in the kernel. It will require you to add a compatible property in Skiboot, and may require some changes to the structure, but I think it will be worth it. Restructuring things this way might also allow you to remove some of the static limits in your kernel side patches, which will help with forwards compatibility. The one possible complication that I was trying to explain but didn't do a very good job of in my previous email is the fact that you have to create PMU events, and I'm not sure if there are requirements on when in bootup they are created. Looking at patch 6 in your v6 kernel side patches, it looks like they don't need to be created really early, so this might work... Regards, Daniel [1] http://events.linuxfoundation.org/sites/events/files/slides/petazzoni-device-tree-dummies.pdf > i.e, Today, OPAL side patchset creates device tree entries > for nest units like MCS, PowerBus, Xlink and Alink. Tomorrow, if > CAPP unit is added (by creating device tree entries) in OPAL side, > kernel side will pick it up without any modifications. In the same way > if we want to remove one of the units says Alink, OPAl side just > need to remove device tree entries created for Alink. > > > hooking into the perf infrastructure in the kernel, but if it is > > possible it'd be a much nicer solution. > > Kernel side patchset is intended for perf support. Cover letter > in the kernel side patchset will explain the perf tool usage. > > > On the 'compatible' property, Yes. we can add it to the device tree. > But wondering how/when to use it, since kernel side does not > have any 'processor type' checks currently and OPAL side > patchset does not pass any data structures to kernel side. > > Maddy > > > > > [1] > > http://devicetree.org/Device_Tree_Usage#Understanding_the_compatible_Property > > > >
On Sat, 2015-08-15 at 12:33 +1000, Daniel Axtens wrote: > By a generic driver I mean a driver that uses an of_device_id table > to > be automatically bound to devices in the device tree. I've found [1] > to > be a very useful introduction. > > If you can implement the kernel side as this, I think it will > dramatically simplify things: it will automatically do detection for > you, without you having to write code to to that in the kernel. It > will > require you to add a compatible property in Skiboot, and may require > some changes to the structure, but I think it will be worth it. > Restructuring things this way might also allow you to remove some of > the > static limits in your kernel side patches, which will help with > forwards > compatibility. Right, you probably still need to create a platform device (like we do for other things in arch/powerpc/platforms/powernv/opal.c) but yes, that will give you automatic loading of the module by udev etc.. and generally speaking is the right way to do this. Cheers, Ben.
On Saturday 15 August 2015 08:03 AM, Daniel Axtens wrote: >> Yes. Kernel side patchset is designed to have a generic driver code, >> which will parse the device tree and create nest pmu drivers. >> Kernel side patchset does not have any specific processor type >> checks, it only check for the device tree entries for the nest units >> created by OPAL side. This way kernel side will need no changes >> when a new nest unit is added or remove in the device tree. >> > This is not quite what I mean by a generic driver. My bad. I did not look at this comment, but have sent out a new version of the code. I will look in it. Maddy > > By a generic driver I mean a driver that uses an of_device_id table to > be automatically bound to devices in the device tree. I've found [1] to > be a very useful introduction. > > If you can implement the kernel side as this, I think it will > dramatically simplify things: it will automatically do detection for > you, without you having to write code to to that in the kernel. It will > require you to add a compatible property in Skiboot, and may require > some changes to the structure, but I think it will be worth it. > Restructuring things this way might also allow you to remove some of the > static limits in your kernel side patches, which will help with forwards > compatibility. > > The one possible complication that I was trying to explain but didn't do > a very good job of in my previous email is the fact that you have to > create PMU events, and I'm not sure if there are requirements on when in > bootup they are created. Looking at patch 6 in your v6 kernel side > patches, it looks like they don't need to be created really early, so > this might work... > > Regards, > Daniel > > [1] http://events.linuxfoundation.org/sites/events/files/slides/petazzoni-device-tree-dummies.pdf > >> i.e, Today, OPAL side patchset creates device tree entries >> for nest units like MCS, PowerBus, Xlink and Alink. Tomorrow, if >> CAPP unit is added (by creating device tree entries) in OPAL side, >> kernel side will pick it up without any modifications. In the same way >> if we want to remove one of the units says Alink, OPAl side just >> need to remove device tree entries created for Alink. >> >>> hooking into the perf infrastructure in the kernel, but if it is >>> possible it'd be a much nicer solution. >> Kernel side patchset is intended for perf support. Cover letter >> in the kernel side patchset will explain the perf tool usage. >> >> >> On the 'compatible' property, Yes. we can add it to the device tree. >> But wondering how/when to use it, since kernel side does not >> have any 'processor type' checks currently and OPAL side >> patchset does not pass any data structures to kernel side. >> >> Maddy >> >>> [1] >>> http://devicetree.org/Device_Tree_Usage#Understanding_the_compatible_Property >>> >>>
diff --git a/hw/nest.c b/hw/nest.c index 9115e5c7fa26..3bc16aed49b5 100644 --- a/hw/nest.c +++ b/hw/nest.c @@ -109,10 +109,56 @@ int load_catalogue_lid(int loaded) void nest_pmu_init(int loaded) { + struct proc_chip *chip; + struct dt_node *dev, *chip_dev; + u64 addr=0; + if (load_catalogue_lid(loaded)) { printf("Nest_IMA: IMA Catalog lid failed to load\n"); return; } + /* + * Now that we have Chip level events enabled, + * lets create DT entries. This is a top level directory under / + * for nest pmu units. + */ + dev = dt_new(dt_root, "nest-ima"); + if (!dev) { + prlog(PR_DEBUG,"Nest_IMA: nest-ima dev creation failed \n"); + return; + } + + /* + * Start exposing per-chip SLW_IMA FW offset in HOMER Region. + */ + for_each_chip(chip) { + chip_dev = dt_new_addr(dev, "ima-chip", chip->id); + if (!chip_dev) { + prlog(PR_DEBUG,"Nest_IMA: ima-chip dev creation failed \n"); + return; + } + + /* + * Design: + * PORE_SLW_IMA FW will program Nest counters with + * pre-defined set of events (provided in catalog) and dump + * counter data in a fixed HOMER offset. + * + * HOMER Region layout has reserved memory for + * PORE_SLW_IMA FW starting from 0x320000 to 0x39FFFF + * + * PORE_SLW_IMA uses this region to dump different Nest unit + * counter data. This address is passed to kernel as base + * address to map. Individual counter offsets are passed + * in the event file. + */ + dt_add_property_cells(chip_dev, "ibm,chip-id", chip->id); + addr = chip->homer_base + SLW_IMA_OFFSET; + dt_add_property_u64(chip_dev, "reg", addr); + dt_add_property_cells(chip_dev, "size", SLW_IMA_SIZE); + dt_add_property(chip_dev, "ibm,ima-chip", NULL, 0); + } + return; }
OPAL sends two set of information to kernel. 1) Information about the per-chip PORE_SLW_IMA HOMER offset to kernel, 2) Information about each support Nest units and their events. Device tree Information Layout for Nest Instrumentation: /- | -ima-chip@<chip-id> | - Homer base + SLW Offset - Size - ... -<Nest Units> | - <events> - unit.<events>.unit - scale.<events>.scale - ... In this patch, code added to pass per-chip PORE_SLW_IMA HOMER offset to kernel as part of device tree entry under "ima-chip@<chip-id>" directory Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com> --- hw/nest.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)