RE: [PATCH v6 09/11] firmware: xilinx: Add debugfs for clock control APIs

[Jolly Shah]

Hi Sudeep,

> -Original Message-
> From: Sudeep Holla [mailto:sudeep.ho...@arm.com]
> Sent: Tuesday, May 15, 2018 1:58 AM
> To: Jolly Shah <jol...@xilinx.com>; ard.biesheu...@linaro.org;
> mi...@kernel.org; gre...@linuxfoundation.org; m...@codeblueprint.co.uk;
> hkallwe...@gmail.com; keesc...@chromium.org;
> dmitry.torok...@gmail.com; mturque...@baylibre.com;
> sb...@codeaurora.org; michal.si...@xilinx.com; robh...@kernel.org;
> mark.rutl...@arm.com; linux-...@vger.kernel.org
> Cc: Sudeep Holla <sudeep.ho...@arm.com>; Rajan Vaja <raj...@xilinx.com>;
> linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicet...@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

RE: [PATCH v6 09/11] firmware: xilinx: Add debugfs for clock control APIs

[Jolly Shah]

Hi Sudeep,

> -Original Message-
> From: Sudeep Holla [mailto:sudeep.ho...@arm.com]
> Sent: Tuesday, May 15, 2018 1:58 AM
> To: Jolly Shah ; ard.biesheu...@linaro.org;
> mi...@kernel.org; gre...@linuxfoundation.org; m...@codeblueprint.co.uk;
> hkallwe...@gmail.com; keesc...@chromium.org;
> dmitry.torok...@gmail.com; mturque...@baylibre.com;
> sb...@codeaurora.org; michal.si...@xilinx.com; robh...@kernel.org;
> mark.rutl...@arm.com; linux-...@vger.kernel.org
> Cc: Sudeep Holla ; Rajan Vaja ;
> linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicet...@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

Re: [PATCH v6 09/11] firmware: xilinx: Add debugfs for clock control APIs

[Sudeep Holla]

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

Re: [PATCH v6 09/11] firmware: xilinx: Add debugfs for clock control APIs

[Sudeep Holla]

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

RE: [PATCH v6 09/11] firmware: xilinx: Add debugfs for clock control APIs

[Jolly Shah]

Hi Sudeep,

> -Original Message-
> From: Sudeep Holla [mailto:sudeep.ho...@arm.com]
> Sent: Thursday, May 10, 2018 7:31 AM
> To: Jolly Shah <jol...@xilinx.com>; ard.biesheu...@linaro.org;
> mi...@kernel.org; gre...@linuxfoundation.org; m...@codeblueprint.co.uk;
> hkallwe...@gmail.com; keesc...@chromium.org;
> dmitry.torok...@gmail.com; mturque...@baylibre.com;
> sb...@codeaurora.org; michal.si...@xilinx.com; robh...@kernel.org;
> mark.rutl...@arm.com; linux-...@vger.kernel.org
> Cc: Sudeep Holla <sudeep.ho...@arm.com>; Rajan Vaja <raj...@xilinx.com>;
> linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org; Jolly Shah <jol...@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 <raj...@xilinx.com>
> >
> > Add debugfs file to control clocks using firmware APIs through debugfs
> > interface.
> >
> > Signed-off-by: Rajan Vaja <raj...@xilinx.com>
> > Signed-off-by: Jolly Shah <jol...@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

RE: [PATCH v6 09/11] firmware: xilinx: Add debugfs for clock control APIs

[Jolly Shah]

Hi Sudeep,

> -Original Message-
> From: Sudeep Holla [mailto:sudeep.ho...@arm.com]
> Sent: Thursday, May 10, 2018 7:31 AM
> To: Jolly Shah ; ard.biesheu...@linaro.org;
> mi...@kernel.org; gre...@linuxfoundation.org; m...@codeblueprint.co.uk;
> hkallwe...@gmail.com; keesc...@chromium.org;
> dmitry.torok...@gmail.com; mturque...@baylibre.com;
> sb...@codeaurora.org; michal.si...@xilinx.com; robh...@kernel.org;
> mark.rutl...@arm.com; linux-...@vger.kernel.org
> Cc: Sudeep Holla ; Rajan Vaja ;
> linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org; Jolly Shah 
> 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 
> >
> > Add debugfs file to control clocks using firmware APIs through debugfs
> > interface.
> >
> > Signed-off-by: Rajan Vaja 
> > Signed-off-by: Jolly Shah 
> > ---
> >  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

Re: [PATCH v6 09/11] firmware: xilinx: Add debugfs for clock control APIs

[Sudeep Holla]

On 10/04/18 20:38, Jolly Shah wrote:
> From: Rajan Vaja 
> 
> Add debugfs file to control clocks using firmware APIs
> through debugfs interface.
> 
> Signed-off-by: Rajan Vaja 
> Signed-off-by: Jolly Shah 
> ---
>  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

Re: [PATCH v6 09/11] firmware: xilinx: Add debugfs for clock control APIs

[Sudeep Holla]

On 10/04/18 20:38, Jolly Shah wrote:
> From: Rajan Vaja 
> 
> Add debugfs file to control clocks using firmware APIs
> through debugfs interface.
> 
> Signed-off-by: Rajan Vaja 
> Signed-off-by: Jolly Shah 
> ---
>  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