| Message ID | 1523389127-14243-10-git-send-email-jollys@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/04/18 20:38, Jolly Shah wrote: > From: Rajan Vaja <rajanv@xilinx.com> > > Add debugfs file to control clocks using firmware APIs > through debugfs interface. > > Signed-off-by: Rajan Vaja <rajanv@xilinx.com> > Signed-off-by: Jolly Shah <jollys@xilinx.com> > --- > drivers/firmware/xilinx/zynqmp-debug.c | 48 ++++++++++++++++++++++++++++++++++ > 1 file changed, 48 insertions(+) > > diff --git a/drivers/firmware/xilinx/zynqmp-debug.c b/drivers/firmware/xilinx/zynqmp-debug.c > index 1cb69f7..837fcd1 100644 > --- a/drivers/firmware/xilinx/zynqmp-debug.c > +++ b/drivers/firmware/xilinx/zynqmp-debug.c > @@ -34,6 +34,15 @@ static struct pm_api_info pm_api_list[] = { > PM_API(PM_GET_API_VERSION), > PM_API(PM_IOCTL), > PM_API(PM_QUERY_DATA), > + PM_API(PM_CLOCK_ENABLE), > + PM_API(PM_CLOCK_DISABLE), > + PM_API(PM_CLOCK_GETSTATE), > + PM_API(PM_CLOCK_SETDIVIDER), > + PM_API(PM_CLOCK_GETDIVIDER), > + PM_API(PM_CLOCK_SETRATE), > + PM_API(PM_CLOCK_GETRATE), > + PM_API(PM_CLOCK_SETPARENT), > + PM_API(PM_CLOCK_GETPARENT), > }; > > /** > @@ -87,6 +96,7 @@ static int process_api_request(u32 pm_id, u64 *pm_api_arg, u32 *pm_api_ret) > const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops(); > u32 pm_api_version; > int ret; > + u64 rate; > > if (!eemi_ops) > return -ENXIO; > @@ -132,6 +142,44 @@ static int process_api_request(u32 pm_id, u64 *pm_api_arg, u32 *pm_api_ret) > pm_api_ret[2], pm_api_ret[3]); > break; > } > + case PM_CLOCK_ENABLE: > + ret = eemi_ops->clock_enable(pm_api_arg[0]); > + break; As I mentioned in earlier patch, I don't see the need for this debugfs interface. Clock lay has read-only interface in debugfs already. Also if you want to debug clocks, you can do so using the driver which uses these clocks. Do you really want to manage clocks in user-space ? The whole idea of EEMI kind of interface is to abstract and hide the fine details even from non-trusted rich OS like Linux kernel, but by providing this you are doing exactly opposite.
Hi Sudeep, > -----Original Message----- > From: Sudeep Holla [mailto:sudeep.holla@arm.com] > Sent: Thursday, May 10, 2018 7:31 AM > To: Jolly Shah <JOLLYS@xilinx.com>; ard.biesheuvel@linaro.org; > mingo@kernel.org; gregkh@linuxfoundation.org; matt@codeblueprint.co.uk; > hkallweit1@gmail.com; keescook@chromium.org; > dmitry.torokhov@gmail.com; mturquette@baylibre.com; > sboyd@codeaurora.org; michal.simek@xilinx.com; robh+dt@kernel.org; > mark.rutland@arm.com; linux-clk@vger.kernel.org > Cc: Sudeep Holla <sudeep.holla@arm.com>; Rajan Vaja <RAJANV@xilinx.com>; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org; Jolly Shah <JOLLYS@xilinx.com> > Subject: Re: [PATCH v6 09/11] firmware: xilinx: Add debugfs for clock control > APIs > > > > On 10/04/18 20:38, Jolly Shah wrote: > > From: Rajan Vaja <rajanv@xilinx.com> > > > > Add debugfs file to control clocks using firmware APIs through debugfs > > interface. > > > > Signed-off-by: Rajan Vaja <rajanv@xilinx.com> > > Signed-off-by: Jolly Shah <jollys@xilinx.com> > > --- > > drivers/firmware/xilinx/zynqmp-debug.c | 48 > > ++++++++++++++++++++++++++++++++++ > > 1 file changed, 48 insertions(+) > > > > diff --git a/drivers/firmware/xilinx/zynqmp-debug.c > > b/drivers/firmware/xilinx/zynqmp-debug.c > > index 1cb69f7..837fcd1 100644 > > --- a/drivers/firmware/xilinx/zynqmp-debug.c > > +++ b/drivers/firmware/xilinx/zynqmp-debug.c > > @@ -34,6 +34,15 @@ static struct pm_api_info pm_api_list[] = { > > PM_API(PM_GET_API_VERSION), > > PM_API(PM_IOCTL), > > PM_API(PM_QUERY_DATA), > > + PM_API(PM_CLOCK_ENABLE), > > + PM_API(PM_CLOCK_DISABLE), > > + PM_API(PM_CLOCK_GETSTATE), > > + PM_API(PM_CLOCK_SETDIVIDER), > > + PM_API(PM_CLOCK_GETDIVIDER), > > + PM_API(PM_CLOCK_SETRATE), > > + PM_API(PM_CLOCK_GETRATE), > > + PM_API(PM_CLOCK_SETPARENT), > > + PM_API(PM_CLOCK_GETPARENT), > > }; > > > > /** > > @@ -87,6 +96,7 @@ static int process_api_request(u32 pm_id, u64 > *pm_api_arg, u32 *pm_api_ret) > > const struct zynqmp_eemi_ops *eemi_ops = > zynqmp_pm_get_eemi_ops(); > > u32 pm_api_version; > > int ret; > > + u64 rate; > > > > if (!eemi_ops) > > return -ENXIO; > > @@ -132,6 +142,44 @@ static int process_api_request(u32 pm_id, u64 > *pm_api_arg, u32 *pm_api_ret) > > pm_api_ret[2], pm_api_ret[3]); > > break; > > } > > + case PM_CLOCK_ENABLE: > > + ret = eemi_ops->clock_enable(pm_api_arg[0]); > > + break; > > As I mentioned in earlier patch, I don't see the need for this debugfs interface. > Clock lay has read-only interface in debugfs already. Also if you want to debug > clocks, you can do so using the driver which uses these clocks. Do you really > want to manage clocks in user-space ? > The whole idea of EEMI kind of interface is to abstract and hide the fine details > even from non-trusted rich OS like Linux kernel, but by providing this you are > doing exactly opposite. > > -- > Regards, > Sudeep No we don't want to manage clocks in user-space. This debugfs is meant for developer who wants to debug APIs behavior in case something doesn't work as expected. Debugfs should be off by default in production images. Thanks, Jolly Shah
On 14/05/18 20:18, Jolly Shah wrote: > Hi Sudeep, [..] >> >> As I mentioned in earlier patch, I don't see the need for this >> debugfs interface. Clock lay has read-only interface in debugfs >> already. Also if you want to debug clocks, you can do so using the >> driver which uses these clocks. Do you really want to manage clocks >> in user-space ? The whole idea of EEMI kind of interface is to >> abstract and hide the fine details even from non-trusted rich OS >> like Linux kernel, but by providing this you are doing exactly >> opposite. > > No we don't want to manage clocks in user-space. This debugfs is > meant for developer who wants to debug APIs behavior in case > something doesn't work as expected. Debugfs should be off by default > in production images. > Good that it's not used in production image. The clock layer has *sufficient* debugfs support that will *help in debugging*. So please drop this Xilinx specific clock debugfs.
Hi Sudeep, > -----Original Message----- > From: Sudeep Holla [mailto:sudeep.holla@arm.com] > Sent: Tuesday, May 15, 2018 1:58 AM > To: Jolly Shah <JOLLYS@xilinx.com>; ard.biesheuvel@linaro.org; > mingo@kernel.org; gregkh@linuxfoundation.org; matt@codeblueprint.co.uk; > hkallweit1@gmail.com; keescook@chromium.org; > dmitry.torokhov@gmail.com; mturquette@baylibre.com; > sboyd@codeaurora.org; michal.simek@xilinx.com; robh+dt@kernel.org; > mark.rutland@arm.com; linux-clk@vger.kernel.org > Cc: Sudeep Holla <sudeep.holla@arm.com>; Rajan Vaja <RAJANV@xilinx.com>; > linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org > Subject: Re: [PATCH v6 09/11] firmware: xilinx: Add debugfs for clock control > APIs > > > > On 14/05/18 20:18, Jolly Shah wrote: > > Hi Sudeep, > > [..] > > >> > >> As I mentioned in earlier patch, I don't see the need for this > >> debugfs interface. Clock lay has read-only interface in debugfs > >> already. Also if you want to debug clocks, you can do so using the > >> driver which uses these clocks. Do you really want to manage clocks > >> in user-space ? The whole idea of EEMI kind of interface is to > >> abstract and hide the fine details even from non-trusted rich OS like > >> Linux kernel, but by providing this you are doing exactly opposite. > > > > No we don't want to manage clocks in user-space. This debugfs is meant > > for developer who wants to debug APIs behavior in case something > > doesn't work as expected. Debugfs should be off by default in > > production images. > > > > Good that it's not used in production image. The clock layer has > *sufficient* debugfs support that will *help in debugging*. So please drop this > Xilinx specific clock debugfs. > > -- > Regards, > Sudeep Ok. Will remove them in next version. Let me know if rest changes look ok and I can submit final version with suggested minor changes. Thanks, Jolly Shah
diff --git a/drivers/firmware/xilinx/zynqmp-debug.c b/drivers/firmware/xilinx/zynqmp-debug.c index 1cb69f7..837fcd1 100644 --- a/drivers/firmware/xilinx/zynqmp-debug.c +++ b/drivers/firmware/xilinx/zynqmp-debug.c @@ -34,6 +34,15 @@ static struct pm_api_info pm_api_list[] = { PM_API(PM_GET_API_VERSION), PM_API(PM_IOCTL), PM_API(PM_QUERY_DATA), + PM_API(PM_CLOCK_ENABLE), + PM_API(PM_CLOCK_DISABLE), + PM_API(PM_CLOCK_GETSTATE), + PM_API(PM_CLOCK_SETDIVIDER), + PM_API(PM_CLOCK_GETDIVIDER), + PM_API(PM_CLOCK_SETRATE), + PM_API(PM_CLOCK_GETRATE), + PM_API(PM_CLOCK_SETPARENT), + PM_API(PM_CLOCK_GETPARENT), }; /** @@ -87,6 +96,7 @@ static int process_api_request(u32 pm_id, u64 *pm_api_arg, u32 *pm_api_ret) const struct zynqmp_eemi_ops *eemi_ops = zynqmp_pm_get_eemi_ops(); u32 pm_api_version; int ret; + u64 rate; if (!eemi_ops) return -ENXIO; @@ -132,6 +142,44 @@ static int process_api_request(u32 pm_id, u64 *pm_api_arg, u32 *pm_api_ret) pm_api_ret[2], pm_api_ret[3]); break; } + case PM_CLOCK_ENABLE: + ret = eemi_ops->clock_enable(pm_api_arg[0]); + break; + case PM_CLOCK_DISABLE: + ret = eemi_ops->clock_disable(pm_api_arg[0]); + break; + case PM_CLOCK_GETSTATE: + ret = eemi_ops->clock_getstate(pm_api_arg[0], &pm_api_ret[0]); + if (!ret) + sprintf(debugfs_buf, "Clock state: %u\n", + pm_api_ret[0]); + break; + case PM_CLOCK_SETDIVIDER: + ret = eemi_ops->clock_setdivider(pm_api_arg[0], pm_api_arg[1]); + break; + case PM_CLOCK_GETDIVIDER: + ret = eemi_ops->clock_getdivider(pm_api_arg[0], &pm_api_ret[0]); + if (!ret) + sprintf(debugfs_buf, "Divider Value: %d\n", + pm_api_ret[0]); + break; + case PM_CLOCK_SETRATE: + ret = eemi_ops->clock_setrate(pm_api_arg[0], pm_api_arg[1]); + break; + case PM_CLOCK_GETRATE: + ret = eemi_ops->clock_getrate(pm_api_arg[0], &rate); + if (!ret) + sprintf(debugfs_buf, "Clock rate :%llu\n", rate); + break; + case PM_CLOCK_SETPARENT: + ret = eemi_ops->clock_setparent(pm_api_arg[0], pm_api_arg[1]); + break; + case PM_CLOCK_GETPARENT: + ret = eemi_ops->clock_getparent(pm_api_arg[0], &pm_api_ret[0]); + if (!ret) + sprintf(debugfs_buf, + "Clock parent Index: %u\n", pm_api_ret[0]); + break; default: sprintf(debugfs_buf, "Unsupported PM-API request\n"); ret = -EINVAL;