RE: [PATCH v6 03/11] firmware: xilinx: Add zynqmp IOCTL API for device control

[Jolly Shah]

Hi Sudeep,

> -----Original Message-----
> From: Sudeep Holla [mailto:sudeep.holla@xxxxxxx]
> Sent: Thursday, May 10, 2018 7:09 AM
> To: Jolly Shah <JOLLYS@xxxxxxxxxx>; ard.biesheuvel@xxxxxxxxxx;
> mingo@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; matt@xxxxxxxxxxxxxxxxxxx;
> hkallweit1@xxxxxxxxx; keescook@xxxxxxxxxxxx;
> dmitry.torokhov@xxxxxxxxx; mturquette@xxxxxxxxxxxx;
> sboyd@xxxxxxxxxxxxxx; michal.simek@xxxxxxxxxx; robh+dt@xxxxxxxxxx;
> mark.rutland@xxxxxxx; linux-clk@xxxxxxxxxxxxxxx
> Cc: Sudeep Holla <sudeep.holla@xxxxxxx>; Rajan Vaja <RAJANV@xxxxxxxxxx>;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; Jolly Shah <JOLLYS@xxxxxxxxxx>
> Subject: Re: [PATCH v6 03/11] firmware: xilinx: Add zynqmp IOCTL API for device
> control
> 
> 
> 
> On 10/04/18 20:38, Jolly Shah wrote:
> > From: Rajan Vaja <rajanv@xxxxxxxxxx>
> >
> > Add ZynqMP firmware IOCTL API to control and configure devices like
> > PLLs, SD, Gem, etc.
> >
> > Signed-off-by: Rajan Vaja <rajanv@xxxxxxxxxx>
> > Signed-off-by: Jolly Shah <jollys@xxxxxxxxxx>
> > ---
> >  drivers/firmware/xilinx/zynqmp.c     | 20 ++++++++++++++++++++
> >  include/linux/firmware/xlnx-zynqmp.h |  2 ++
> >  2 files changed, 22 insertions(+)
> >
> > diff --git a/drivers/firmware/xilinx/zynqmp.c
> > b/drivers/firmware/xilinx/zynqmp.c
> > index 490561a..44b43fa 100644
> > --- a/drivers/firmware/xilinx/zynqmp.c
> > +++ b/drivers/firmware/xilinx/zynqmp.c
> > @@ -239,8 +239,28 @@ static int get_set_conduit_method(struct
> device_node *np)
> >  	return 0;
> >  }
> >
> > +/**
> > + * zynqmp_pm_ioctl() - PM IOCTL API for device control and configs
> > + * @node_id:	Node ID of the device
> > + * @ioctl_id:	ID of the requested IOCTL
> > + * @arg1:	Argument 1 to requested IOCTL call
> > + * @arg2:	Argument 2 to requested IOCTL call
> > + * @out:	Returned output value
> > + *
> > + * This function calls IOCTL to firmware for device control and configuration.
> > + *
> > + * Return: Returns status, either success or error+reason  */ static
> > +int zynqmp_pm_ioctl(u32 node_id, u32 ioctl_id, u32 arg1, u32 arg2,
> > +			   u32 *out)
> > +{
> > +	return zynqmp_pm_invoke_fn(PM_IOCTL, node_id, ioctl_id,
> > +				   arg1, arg2, out);
> > +}
> > +
> >  static const struct zynqmp_eemi_ops eemi_ops = {
> >  	.get_api_version = zynqmp_pm_get_api_version,
> > +	.ioctl = zynqmp_pm_ioctl,
> >  };
> >
> >  /**
> > diff --git a/include/linux/firmware/xlnx-zynqmp.h
> > b/include/linux/firmware/xlnx-zynqmp.h
> > index cb63bed..2eec6e7 100644
> > --- a/include/linux/firmware/xlnx-zynqmp.h
> > +++ b/include/linux/firmware/xlnx-zynqmp.h
> > @@ -34,6 +34,7 @@
> >
> >  enum pm_api_id {
> >  	PM_GET_API_VERSION = 1,
> > +	PM_IOCTL = 34,
> 
> I am not for this API. IIUC there are more fined grained well defined APIs(if I am
> not wrong from enum 2 upto 33). This is open ended API which allows user to do
> whatever setting it needs. And that defeats the purpose of other APIs.
> 
> I will look through the series for the usage of this to understand this better, but I
> am guessing how it can be (ab)used.
> 
> --
> Regards,
> Sudeep

There are well defined APIs for general clock controls. Ioctl is for some specific operations which may not apply to all platforms.
For clock driver, ioctl is used to get/set Pll fraction/integer mode and data. 

Thanks,
Jolly Shah