| Message ID | 1523389127-14243-10-git-send-email-jollys@xilinx.com (mailing list archive) |
|---|---|
| State | Not Applicable, 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.
SGkgU3VkZWVwLA0KDQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IFN1ZGVl cCBIb2xsYSBbbWFpbHRvOnN1ZGVlcC5ob2xsYUBhcm0uY29tXQ0KPiBTZW50OiBUaHVyc2RheSwg TWF5IDEwLCAyMDE4IDc6MzEgQU0NCj4gVG86IEpvbGx5IFNoYWggPEpPTExZU0B4aWxpbnguY29t PjsgYXJkLmJpZXNoZXV2ZWxAbGluYXJvLm9yZzsNCj4gbWluZ29Aa2VybmVsLm9yZzsgZ3JlZ2to QGxpbnV4Zm91bmRhdGlvbi5vcmc7IG1hdHRAY29kZWJsdWVwcmludC5jby51azsNCj4gaGthbGx3 ZWl0MUBnbWFpbC5jb207IGtlZXNjb29rQGNocm9taXVtLm9yZzsNCj4gZG1pdHJ5LnRvcm9raG92 QGdtYWlsLmNvbTsgbXR1cnF1ZXR0ZUBiYXlsaWJyZS5jb207DQo+IHNib3lkQGNvZGVhdXJvcmEu b3JnOyBtaWNoYWwuc2ltZWtAeGlsaW54LmNvbTsgcm9iaCtkdEBrZXJuZWwub3JnOw0KPiBtYXJr LnJ1dGxhbmRAYXJtLmNvbTsgbGludXgtY2xrQHZnZXIua2VybmVsLm9yZw0KPiBDYzogU3VkZWVw IEhvbGxhIDxzdWRlZXAuaG9sbGFAYXJtLmNvbT47IFJhamFuIFZhamEgPFJBSkFOVkB4aWxpbngu Y29tPjsNCj4gbGludXgtYXJtLWtlcm5lbEBsaXN0cy5pbmZyYWRlYWQub3JnOyBsaW51eC1rZXJu ZWxAdmdlci5rZXJuZWwub3JnOw0KPiBkZXZpY2V0cmVlQHZnZXIua2VybmVsLm9yZzsgSm9sbHkg U2hhaCA8Sk9MTFlTQHhpbGlueC5jb20+DQo+IFN1YmplY3Q6IFJlOiBbUEFUQ0ggdjYgMDkvMTFd IGZpcm13YXJlOiB4aWxpbng6IEFkZCBkZWJ1Z2ZzIGZvciBjbG9jayBjb250cm9sDQo+IEFQSXMN Cj4gDQo+IA0KPiANCj4gT24gMTAvMDQvMTggMjA6MzgsIEpvbGx5IFNoYWggd3JvdGU6DQo+ID4g RnJvbTogUmFqYW4gVmFqYSA8cmFqYW52QHhpbGlueC5jb20+DQo+ID4NCj4gPiBBZGQgZGVidWdm cyBmaWxlIHRvIGNvbnRyb2wgY2xvY2tzIHVzaW5nIGZpcm13YXJlIEFQSXMgdGhyb3VnaCBkZWJ1 Z2ZzDQo+ID4gaW50ZXJmYWNlLg0KPiA+DQo+ID4gU2lnbmVkLW9mZi1ieTogUmFqYW4gVmFqYSA8 cmFqYW52QHhpbGlueC5jb20+DQo+ID4gU2lnbmVkLW9mZi1ieTogSm9sbHkgU2hhaCA8am9sbHlz QHhpbGlueC5jb20+DQo+ID4gLS0tDQo+ID4gIGRyaXZlcnMvZmlybXdhcmUveGlsaW54L3p5bnFt cC1kZWJ1Zy5jIHwgNDgNCj4gPiArKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrDQo+ ID4gIDEgZmlsZSBjaGFuZ2VkLCA0OCBpbnNlcnRpb25zKCspDQo+ID4NCj4gPiBkaWZmIC0tZ2l0 IGEvZHJpdmVycy9maXJtd2FyZS94aWxpbngvenlucW1wLWRlYnVnLmMNCj4gPiBiL2RyaXZlcnMv ZmlybXdhcmUveGlsaW54L3p5bnFtcC1kZWJ1Zy5jDQo+ID4gaW5kZXggMWNiNjlmNy4uODM3ZmNk MSAxMDA2NDQNCj4gPiAtLS0gYS9kcml2ZXJzL2Zpcm13YXJlL3hpbGlueC96eW5xbXAtZGVidWcu Yw0KPiA+ICsrKyBiL2RyaXZlcnMvZmlybXdhcmUveGlsaW54L3p5bnFtcC1kZWJ1Zy5jDQo+ID4g QEAgLTM0LDYgKzM0LDE1IEBAIHN0YXRpYyBzdHJ1Y3QgcG1fYXBpX2luZm8gcG1fYXBpX2xpc3Rb XSA9IHsNCj4gPiAgCVBNX0FQSShQTV9HRVRfQVBJX1ZFUlNJT04pLA0KPiA+ICAJUE1fQVBJKFBN X0lPQ1RMKSwNCj4gPiAgCVBNX0FQSShQTV9RVUVSWV9EQVRBKSwNCj4gPiArCVBNX0FQSShQTV9D TE9DS19FTkFCTEUpLA0KPiA+ICsJUE1fQVBJKFBNX0NMT0NLX0RJU0FCTEUpLA0KPiA+ICsJUE1f QVBJKFBNX0NMT0NLX0dFVFNUQVRFKSwNCj4gPiArCVBNX0FQSShQTV9DTE9DS19TRVRESVZJREVS KSwNCj4gPiArCVBNX0FQSShQTV9DTE9DS19HRVRESVZJREVSKSwNCj4gPiArCVBNX0FQSShQTV9D TE9DS19TRVRSQVRFKSwNCj4gPiArCVBNX0FQSShQTV9DTE9DS19HRVRSQVRFKSwNCj4gPiArCVBN X0FQSShQTV9DTE9DS19TRVRQQVJFTlQpLA0KPiA+ICsJUE1fQVBJKFBNX0NMT0NLX0dFVFBBUkVO VCksDQo+ID4gIH07DQo+ID4NCj4gPiAgLyoqDQo+ID4gQEAgLTg3LDYgKzk2LDcgQEAgc3RhdGlj IGludCBwcm9jZXNzX2FwaV9yZXF1ZXN0KHUzMiBwbV9pZCwgdTY0DQo+ICpwbV9hcGlfYXJnLCB1 MzIgKnBtX2FwaV9yZXQpDQo+ID4gIAljb25zdCBzdHJ1Y3QgenlucW1wX2VlbWlfb3BzICplZW1p X29wcyA9DQo+IHp5bnFtcF9wbV9nZXRfZWVtaV9vcHMoKTsNCj4gPiAgCXUzMiBwbV9hcGlfdmVy c2lvbjsNCj4gPiAgCWludCByZXQ7DQo+ID4gKwl1NjQgcmF0ZTsNCj4gPg0KPiA+ICAJaWYgKCFl ZW1pX29wcykNCj4gPiAgCQlyZXR1cm4gLUVOWElPOw0KPiA+IEBAIC0xMzIsNiArMTQyLDQ0IEBA IHN0YXRpYyBpbnQgcHJvY2Vzc19hcGlfcmVxdWVzdCh1MzIgcG1faWQsIHU2NA0KPiAqcG1fYXBp X2FyZywgdTMyICpwbV9hcGlfcmV0KQ0KPiA+ICAJCQkJcG1fYXBpX3JldFsyXSwgcG1fYXBpX3Jl dFszXSk7DQo+ID4gIAkJYnJlYWs7DQo+ID4gIAl9DQo+ID4gKwljYXNlIFBNX0NMT0NLX0VOQUJM RToNCj4gPiArCQlyZXQgPSBlZW1pX29wcy0+Y2xvY2tfZW5hYmxlKHBtX2FwaV9hcmdbMF0pOw0K PiA+ICsJCWJyZWFrOw0KPiANCj4gQXMgSSBtZW50aW9uZWQgaW4gZWFybGllciBwYXRjaCwgSSBk b24ndCBzZWUgdGhlIG5lZWQgZm9yIHRoaXMgZGVidWdmcyBpbnRlcmZhY2UuDQo+IENsb2NrIGxh eSBoYXMgcmVhZC1vbmx5IGludGVyZmFjZSBpbiBkZWJ1Z2ZzIGFscmVhZHkuIEFsc28gaWYgeW91 IHdhbnQgdG8gZGVidWcNCj4gY2xvY2tzLCB5b3UgY2FuIGRvIHNvIHVzaW5nIHRoZSBkcml2ZXIg d2hpY2ggdXNlcyB0aGVzZSBjbG9ja3MuIERvIHlvdSByZWFsbHkNCj4gd2FudCB0byBtYW5hZ2Ug Y2xvY2tzIGluIHVzZXItc3BhY2UgPw0KPiBUaGUgd2hvbGUgaWRlYSBvZiBFRU1JIGtpbmQgb2Yg aW50ZXJmYWNlIGlzIHRvIGFic3RyYWN0IGFuZCBoaWRlIHRoZSBmaW5lIGRldGFpbHMNCj4gZXZl biBmcm9tIG5vbi10cnVzdGVkIHJpY2ggT1MgbGlrZSBMaW51eCBrZXJuZWwsIGJ1dCBieSBwcm92 aWRpbmcgdGhpcyB5b3UgYXJlDQo+IGRvaW5nIGV4YWN0bHkgb3Bwb3NpdGUuDQo+IA0KPiAtLQ0K PiBSZWdhcmRzLA0KPiBTdWRlZXANCg0KTm8gd2UgZG9uJ3Qgd2FudCB0byBtYW5hZ2UgY2xvY2tz IGluIHVzZXItc3BhY2UuIFRoaXMgZGVidWdmcyBpcyBtZWFudCBmb3IgZGV2ZWxvcGVyIHdobyB3 YW50cyB0byBkZWJ1ZyBBUElzIGJlaGF2aW9yIGluIGNhc2Ugc29tZXRoaW5nIGRvZXNuJ3Qgd29y ayBhcyBleHBlY3RlZC4gRGVidWdmcyBzaG91bGQgYmUgb2ZmIGJ5IGRlZmF1bHQgaW4gcHJvZHVj dGlvbiBpbWFnZXMuDQoNClRoYW5rcywNCkpvbGx5IFNoYWgNCg0K -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;