RE: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware driver

[Jolly Shah]

Hi Mark,

> -Original Message-
> From: Mark Rutland [mailto:mark.rutl...@arm.com]
> Sent: Thursday, February 01, 2018 2:33 AM
> To: Jolly Shah <jol...@xilinx.com>
> Cc: ard.biesheu...@linaro.org; mi...@kernel.org;
> gre...@linuxfoundation.org; m...@codeblueprint.co.uk;
> sudeep.ho...@arm.com; hkallwe...@gmail.com; keesc...@chromium.org;
> dmitry.torok...@gmail.com; michal.si...@xilinx.com; robh...@kernel.org;
> linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org; Rajan Vaja <raj...@xilinx.com>
> Subject: Re: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware
> driver
> 
> On Thu, Feb 01, 2018 at 01:23:48AM +, Jolly Shah wrote:
> > Hi Mark,
> > Thanks for the review,
> >
> > > -Original Message-
> > > From: Mark Rutland [mailto:mark.rutl...@arm.com]
> > > Sent: Wednesday, January 31, 2018 10:20 AM
> > > To: Jolly Shah <jol...@xilinx.com>
> > > Cc: ard.biesheu...@linaro.org; mi...@kernel.org;
> > > gre...@linuxfoundation.org; m...@codeblueprint.co.uk;
> > > sudeep.ho...@arm.com; hkallwe...@gmail.com; keesc...@chromium.org;
> > > dmitry.torok...@gmail.com; michal.si...@xilinx.com;
> > > robh...@kernel.org; linux-arm-ker...@lists.infradead.org;
> > > linux-kernel@vger.kernel.org; devicet...@vger.kernel.org; Jolly Shah
> > > <jol...@xilinx.com>; Rajan Vaja <raj...@xilinx.com>
> > > Subject: Re: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP
> > > firmware driver
> > >
> > > On Wed, Jan 24, 2018 at 03:21:12PM -0800, Jolly Shah wrote:
> > > > This patch is adding communication layer with firmware.
> > > > Firmware driver provides an interface to firmware APIs.
> > > > Interface APIs can be used by any driver to communicate to
> > > > PMUFW(Platform Management Unit). All requests go through ATF.
> > >
> > > > +/**
> > > > + * zynqmp_pm_set_wakeup_source - PM call to specify the wakeup
> source
> > > > + * while suspended
> > > > + * @target:Node ID of the targeted PU or subsystem
> > > > + * @wakeup_node:Node ID of the wakeup peripheral
> > > > + * @enable:Enable or disable the specified peripheral as wake
> source
> > > > + *
> > > > + * Return: Returns status, either success or error+reason
> > > > + */
> > > > +static int zynqmp_pm_set_wakeup_source(const u32 target,
> > > > +  const u32 wakeup_node,
> > > > +  const u32 enable)
> > > > +{
> > > > +   return invoke_pm_fn(PM_SET_WAKEUP_SOURCE, target,
> > > > +   wakeup_node, enable, 0, NULL); }
> > >
> > > I see many functions take a "Node ID" parameter, but these don't
> > > appear to be in any DT binding, and drivers (other than the debugfs 
> > > driver)
> aren't using them.
> > >
> > > What's the plan for making use of these? Where are the node IDs
> > > going to come from in practice?
> > >
> > Node ids are defined in firmware.h. Node id refers to targeted
> PU/subsystem/peripheral for required action.
> 
> Ok. What I was asking was how a node id would be associated with particular
> peripheral instances (which are presumably going to be nodes in the DT).
> 
> e.g. if I have
> 
>   device@foo {
>   compatible = "xlnx,some-device";
>   reg = <0xf00 0x100>;
>   ...
>   };
> 
> ... how does the kernel know which node id(s) the device is associated with?
> 
> I assume that those will need something like a xlnx,eemi-node-id property.
> 
> [...]
> 

ZynqMP Power domain driver has node-ids defines under pd-id properties.(RFC 
patch below)
https://patchwork.kernel.org/patch/10150683/

Individual driver can have phandle to it.
For example,
power-domains {
pd_xx: pd_xx {
#power-domain-cells = <0x0>;
pd-id = <0x7>;
};
pd_yy: pd_yy {
#power-domain-cells = <0x0>;
pd-id = <0xf>;
};
};

device: dev@ffe0 {
compatible = "dev";
reg = <0x0 0xFFE0 0x0 0x2>;
pd-handle = <_xx>;
};


> > > > +/**
> > > > + * zynqmp_pm_pinctrl_request - Request Pin from 

RE: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware driver

[Jolly Shah]

Hi Mark,

> -Original Message-
> From: Mark Rutland [mailto:mark.rutl...@arm.com]
> Sent: Thursday, February 01, 2018 2:33 AM
> To: Jolly Shah 
> Cc: ard.biesheu...@linaro.org; mi...@kernel.org;
> gre...@linuxfoundation.org; m...@codeblueprint.co.uk;
> sudeep.ho...@arm.com; hkallwe...@gmail.com; keesc...@chromium.org;
> dmitry.torok...@gmail.com; michal.si...@xilinx.com; robh...@kernel.org;
> linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org; Rajan Vaja 
> Subject: Re: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware
> driver
> 
> On Thu, Feb 01, 2018 at 01:23:48AM +, Jolly Shah wrote:
> > Hi Mark,
> > Thanks for the review,
> >
> > > -Original Message-
> > > From: Mark Rutland [mailto:mark.rutl...@arm.com]
> > > Sent: Wednesday, January 31, 2018 10:20 AM
> > > To: Jolly Shah 
> > > Cc: ard.biesheu...@linaro.org; mi...@kernel.org;
> > > gre...@linuxfoundation.org; m...@codeblueprint.co.uk;
> > > sudeep.ho...@arm.com; hkallwe...@gmail.com; keesc...@chromium.org;
> > > dmitry.torok...@gmail.com; michal.si...@xilinx.com;
> > > robh...@kernel.org; linux-arm-ker...@lists.infradead.org;
> > > linux-kernel@vger.kernel.org; devicet...@vger.kernel.org; Jolly Shah
> > > ; Rajan Vaja 
> > > Subject: Re: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP
> > > firmware driver
> > >
> > > On Wed, Jan 24, 2018 at 03:21:12PM -0800, Jolly Shah wrote:
> > > > This patch is adding communication layer with firmware.
> > > > Firmware driver provides an interface to firmware APIs.
> > > > Interface APIs can be used by any driver to communicate to
> > > > PMUFW(Platform Management Unit). All requests go through ATF.
> > >
> > > > +/**
> > > > + * zynqmp_pm_set_wakeup_source - PM call to specify the wakeup
> source
> > > > + * while suspended
> > > > + * @target:Node ID of the targeted PU or subsystem
> > > > + * @wakeup_node:Node ID of the wakeup peripheral
> > > > + * @enable:Enable or disable the specified peripheral as wake
> source
> > > > + *
> > > > + * Return: Returns status, either success or error+reason
> > > > + */
> > > > +static int zynqmp_pm_set_wakeup_source(const u32 target,
> > > > +  const u32 wakeup_node,
> > > > +  const u32 enable)
> > > > +{
> > > > +   return invoke_pm_fn(PM_SET_WAKEUP_SOURCE, target,
> > > > +   wakeup_node, enable, 0, NULL); }
> > >
> > > I see many functions take a "Node ID" parameter, but these don't
> > > appear to be in any DT binding, and drivers (other than the debugfs 
> > > driver)
> aren't using them.
> > >
> > > What's the plan for making use of these? Where are the node IDs
> > > going to come from in practice?
> > >
> > Node ids are defined in firmware.h. Node id refers to targeted
> PU/subsystem/peripheral for required action.
> 
> Ok. What I was asking was how a node id would be associated with particular
> peripheral instances (which are presumably going to be nodes in the DT).
> 
> e.g. if I have
> 
>   device@foo {
>   compatible = "xlnx,some-device";
>   reg = <0xf00 0x100>;
>   ...
>   };
> 
> ... how does the kernel know which node id(s) the device is associated with?
> 
> I assume that those will need something like a xlnx,eemi-node-id property.
> 
> [...]
> 

ZynqMP Power domain driver has node-ids defines under pd-id properties.(RFC 
patch below)
https://patchwork.kernel.org/patch/10150683/

Individual driver can have phandle to it.
For example,
power-domains {
pd_xx: pd_xx {
#power-domain-cells = <0x0>;
pd-id = <0x7>;
};
pd_yy: pd_yy {
#power-domain-cells = <0x0>;
pd-id = <0xf>;
};
};

device: dev@ffe0 {
compatible = "dev";
reg = <0x0 0xFFE0 0x0 0x2>;
pd-handle = <_xx>;
};


> > > > +/**
> > > > + * zynqmp_pm_pinctrl_request - Request Pin from firmware
> > > > + * @pin:   Pin number to request
> > > > + *
> > >
> > > No DT binding for the pinctrl bits?
> > 

Re: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware driver

[Mark Rutland]

On Thu, Feb 01, 2018 at 01:23:48AM +, Jolly Shah wrote:
> Hi Mark,
> Thanks for the review,
> 
> > -Original Message-
> > From: Mark Rutland [mailto:mark.rutl...@arm.com]
> > Sent: Wednesday, January 31, 2018 10:20 AM
> > To: Jolly Shah <jol...@xilinx.com>
> > Cc: ard.biesheu...@linaro.org; mi...@kernel.org;
> > gre...@linuxfoundation.org; m...@codeblueprint.co.uk;
> > sudeep.ho...@arm.com; hkallwe...@gmail.com; keesc...@chromium.org;
> > dmitry.torok...@gmail.com; michal.si...@xilinx.com; robh...@kernel.org;
> > linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> > devicet...@vger.kernel.org; Jolly Shah <jol...@xilinx.com>; Rajan Vaja
> > <raj...@xilinx.com>
> > Subject: Re: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware
> > driver
> > 
> > On Wed, Jan 24, 2018 at 03:21:12PM -0800, Jolly Shah wrote:
> > > This patch is adding communication layer with firmware.
> > > Firmware driver provides an interface to firmware APIs.
> > > Interface APIs can be used by any driver to communicate to
> > > PMUFW(Platform Management Unit). All requests go through ATF.
> > 
> > > +/**
> > > + * zynqmp_pm_set_wakeup_source - PM call to specify the wakeup source
> > > + *   while suspended
> > > + * @target:  Node ID of the targeted PU or subsystem
> > > + * @wakeup_node:Node ID of the wakeup peripheral
> > > + * @enable:  Enable or disable the specified peripheral as wake 
> > > source
> > > + *
> > > + * Return:   Returns status, either success or error+reason
> > > + */
> > > +static int zynqmp_pm_set_wakeup_source(const u32 target,
> > > +const u32 wakeup_node,
> > > +const u32 enable)
> > > +{
> > > + return invoke_pm_fn(PM_SET_WAKEUP_SOURCE, target,
> > > + wakeup_node, enable, 0, NULL); }
> > 
> > I see many functions take a "Node ID" parameter, but these don't appear to 
> > be
> > in any DT binding, and drivers (other than the debugfs driver) aren't using 
> > them.
> > 
> > What's the plan for making use of these? Where are the node IDs going to 
> > come
> > from in practice?
> > 
> Node ids are defined in firmware.h. Node id refers to targeted 
> PU/subsystem/peripheral for required action.

Ok. What I was asking was how a node id would be associated with
particular peripheral instances (which are presumably going to be nodes
in the DT).

e.g. if I have

device@foo {
compatible = "xlnx,some-device";
reg = <0xf00 0x100>;
...
};

... how does the kernel know which node id(s) the device is associated
with?

I assume that those will need something like a xlnx,eemi-node-id
property.

[...]

> > > +/**
> > > + * zynqmp_pm_pinctrl_request - Request Pin from firmware
> > > + * @pin: Pin number to request
> > > + *
> > 
> > No DT binding for the pinctrl bits?
> > 
> > [...]
> It doesn't require any bindings. Calling drivers will have DT binding for 
> pins they use. 

For those drivers to be able to refer to the EEMI FW as a pin
controller, we'll need a pinctrl node in the DT (and hence a binding).

> > > +/**
> > > + * zynqmp_pm_clock_enable - Enable the clock for given id
> > > + * @clock_id:ID of the clock to be enabled
> > > + *
> > 
> > Likewise for the clocks?
> >
> It doesn't require bindings too. 

As with pinctrl, for drivers to be able to refer to these clocks, we'll
need a clock node in the DT (and hence a binding).

Thanks,
Mark.

Re: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware driver

[Mark Rutland]

On Thu, Feb 01, 2018 at 01:23:48AM +, Jolly Shah wrote:
> Hi Mark,
> Thanks for the review,
> 
> > -Original Message-
> > From: Mark Rutland [mailto:mark.rutl...@arm.com]
> > Sent: Wednesday, January 31, 2018 10:20 AM
> > To: Jolly Shah 
> > Cc: ard.biesheu...@linaro.org; mi...@kernel.org;
> > gre...@linuxfoundation.org; m...@codeblueprint.co.uk;
> > sudeep.ho...@arm.com; hkallwe...@gmail.com; keesc...@chromium.org;
> > dmitry.torok...@gmail.com; michal.si...@xilinx.com; robh...@kernel.org;
> > linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> > devicet...@vger.kernel.org; Jolly Shah ; Rajan Vaja
> > 
> > Subject: Re: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware
> > driver
> > 
> > On Wed, Jan 24, 2018 at 03:21:12PM -0800, Jolly Shah wrote:
> > > This patch is adding communication layer with firmware.
> > > Firmware driver provides an interface to firmware APIs.
> > > Interface APIs can be used by any driver to communicate to
> > > PMUFW(Platform Management Unit). All requests go through ATF.
> > 
> > > +/**
> > > + * zynqmp_pm_set_wakeup_source - PM call to specify the wakeup source
> > > + *   while suspended
> > > + * @target:  Node ID of the targeted PU or subsystem
> > > + * @wakeup_node:Node ID of the wakeup peripheral
> > > + * @enable:  Enable or disable the specified peripheral as wake 
> > > source
> > > + *
> > > + * Return:   Returns status, either success or error+reason
> > > + */
> > > +static int zynqmp_pm_set_wakeup_source(const u32 target,
> > > +const u32 wakeup_node,
> > > +const u32 enable)
> > > +{
> > > + return invoke_pm_fn(PM_SET_WAKEUP_SOURCE, target,
> > > + wakeup_node, enable, 0, NULL); }
> > 
> > I see many functions take a "Node ID" parameter, but these don't appear to 
> > be
> > in any DT binding, and drivers (other than the debugfs driver) aren't using 
> > them.
> > 
> > What's the plan for making use of these? Where are the node IDs going to 
> > come
> > from in practice?
> > 
> Node ids are defined in firmware.h. Node id refers to targeted 
> PU/subsystem/peripheral for required action.

Ok. What I was asking was how a node id would be associated with
particular peripheral instances (which are presumably going to be nodes
in the DT).

e.g. if I have

device@foo {
compatible = "xlnx,some-device";
reg = <0xf00 0x100>;
...
};

... how does the kernel know which node id(s) the device is associated
with?

I assume that those will need something like a xlnx,eemi-node-id
property.

[...]

> > > +/**
> > > + * zynqmp_pm_pinctrl_request - Request Pin from firmware
> > > + * @pin: Pin number to request
> > > + *
> > 
> > No DT binding for the pinctrl bits?
> > 
> > [...]
> It doesn't require any bindings. Calling drivers will have DT binding for 
> pins they use. 

For those drivers to be able to refer to the EEMI FW as a pin
controller, we'll need a pinctrl node in the DT (and hence a binding).

> > > +/**
> > > + * zynqmp_pm_clock_enable - Enable the clock for given id
> > > + * @clock_id:ID of the clock to be enabled
> > > + *
> > 
> > Likewise for the clocks?
> >
> It doesn't require bindings too. 

As with pinctrl, for drivers to be able to refer to these clocks, we'll
need a clock node in the DT (and hence a binding).

Thanks,
Mark.

RE: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware driver

[Jolly Shah]

Hi Mark,
Thanks for the review,

> -Original Message-
> From: Mark Rutland [mailto:mark.rutl...@arm.com]
> Sent: Wednesday, January 31, 2018 10:20 AM
> To: Jolly Shah <jol...@xilinx.com>
> Cc: ard.biesheu...@linaro.org; mi...@kernel.org;
> gre...@linuxfoundation.org; m...@codeblueprint.co.uk;
> sudeep.ho...@arm.com; hkallwe...@gmail.com; keesc...@chromium.org;
> dmitry.torok...@gmail.com; michal.si...@xilinx.com; robh...@kernel.org;
> linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org; Jolly Shah <jol...@xilinx.com>; Rajan Vaja
> <raj...@xilinx.com>
> Subject: Re: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware
> driver
> 
> On Wed, Jan 24, 2018 at 03:21:12PM -0800, Jolly Shah wrote:
> > This patch is adding communication layer with firmware.
> > Firmware driver provides an interface to firmware APIs.
> > Interface APIs can be used by any driver to communicate to
> > PMUFW(Platform Management Unit). All requests go through ATF.
> 
> > +/**
> > + * zynqmp_pm_set_wakeup_source - PM call to specify the wakeup source
> > + * while suspended
> > + * @target:Node ID of the targeted PU or subsystem
> > + * @wakeup_node:Node ID of the wakeup peripheral
> > + * @enable:Enable or disable the specified peripheral as wake 
> > source
> > + *
> > + * Return: Returns status, either success or error+reason
> > + */
> > +static int zynqmp_pm_set_wakeup_source(const u32 target,
> > +  const u32 wakeup_node,
> > +  const u32 enable)
> > +{
> > +   return invoke_pm_fn(PM_SET_WAKEUP_SOURCE, target,
> > +   wakeup_node, enable, 0, NULL); }
> 
> I see many functions take a "Node ID" parameter, but these don't appear to be
> in any DT binding, and drivers (other than the debugfs driver) aren't using 
> them.
> 
> What's the plan for making use of these? Where are the node IDs going to come
> from in practice?
> 
Node ids are defined in firmware.h. Node id refers to targeted 
PU/subsystem/peripheral for required action.

> > +/**
> > + * zynqmp_pm_system_shutdown - PM call to request a system shutdown or
> restart
> > + * @type:  Shutdown or restart? 0 for shutdown, 1 for restart
> > + * @subtype:   Specifies which system should be restarted or shut down
> > + *
> > + * Return: Returns status, either success or error+reason
> > + */
> > +static int zynqmp_pm_system_shutdown(const u32 type, const u32
> > +subtype) {
> > +   return invoke_pm_fn(PM_SYSTEM_SHUTDOWN, type, subtype, 0, 0,
> NULL);
> > +}
> 
> PSCI already has this functionality, so I'm a little confused by the 
> duplication.
> 
PSCI doesn't distinguish between shutdown scope. It can be APU/PS/System in 
this case.

> [...]
> 
> > +/**
> > + * zynqmp_pm_get_node_status - PM call to request a node's current power
> state
> > + * @node:  ID of the component or sub-system in question
> > + * @status:Current operating state of the requested node
> > + * @requirements:  Current requirements asserted on the node,
> > + * used for slave nodes only.
> > + * @usage: Usage information, used for slave nodes only:
> > + * 0 - No master is currently using the node
> > + * 1 - Only requesting master is currently using the node
> > + * 2 - Only other masters are currently using the node
> > + * 3 - Both the current and at least one other master
> > + * is currently using the node
> 
> These should probably have corresponding macros or enum values.
Will add macros in next version patch.

> 
> [...]
> 
> > +/**
> > + * zynqmp_pm_sha_hash - Access the SHA engine to calculate the hash
> > + * @address:   Address of the data/ Address of output buffer where
> > + * hash should be stored.
> > + * @size:  Size of the data.
> > + * @flags:
> > + * BIT(0) - Sha3 init (Here address and size inputs can be NULL)
> > + * BIT(1) - Sha3 update (address should holds the )
> 
> Missing "data", I guess?
Yes will update in next version patch.

> 
> > + * BIT(2) - Sha3 final (address should hold the address of
> > + *  buffer to store hash)
> 
> Is the SHA engine coherent? Or is cache maintenance necessary?
> 
> [...]
It is coherent. Update/Final has below significance here:
BIT(1) - To call Sha3_Update API which can be called multiple times when data 
is not contiguous.
BIT(

RE: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware driver

[Jolly Shah]

Hi Mark,
Thanks for the review,

> -Original Message-
> From: Mark Rutland [mailto:mark.rutl...@arm.com]
> Sent: Wednesday, January 31, 2018 10:20 AM
> To: Jolly Shah 
> Cc: ard.biesheu...@linaro.org; mi...@kernel.org;
> gre...@linuxfoundation.org; m...@codeblueprint.co.uk;
> sudeep.ho...@arm.com; hkallwe...@gmail.com; keesc...@chromium.org;
> dmitry.torok...@gmail.com; michal.si...@xilinx.com; robh...@kernel.org;
> linux-arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org; Jolly Shah ; Rajan Vaja
> 
> Subject: Re: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware
> driver
> 
> On Wed, Jan 24, 2018 at 03:21:12PM -0800, Jolly Shah wrote:
> > This patch is adding communication layer with firmware.
> > Firmware driver provides an interface to firmware APIs.
> > Interface APIs can be used by any driver to communicate to
> > PMUFW(Platform Management Unit). All requests go through ATF.
> 
> > +/**
> > + * zynqmp_pm_set_wakeup_source - PM call to specify the wakeup source
> > + * while suspended
> > + * @target:Node ID of the targeted PU or subsystem
> > + * @wakeup_node:Node ID of the wakeup peripheral
> > + * @enable:Enable or disable the specified peripheral as wake 
> > source
> > + *
> > + * Return: Returns status, either success or error+reason
> > + */
> > +static int zynqmp_pm_set_wakeup_source(const u32 target,
> > +  const u32 wakeup_node,
> > +  const u32 enable)
> > +{
> > +   return invoke_pm_fn(PM_SET_WAKEUP_SOURCE, target,
> > +   wakeup_node, enable, 0, NULL); }
> 
> I see many functions take a "Node ID" parameter, but these don't appear to be
> in any DT binding, and drivers (other than the debugfs driver) aren't using 
> them.
> 
> What's the plan for making use of these? Where are the node IDs going to come
> from in practice?
> 
Node ids are defined in firmware.h. Node id refers to targeted 
PU/subsystem/peripheral for required action.

> > +/**
> > + * zynqmp_pm_system_shutdown - PM call to request a system shutdown or
> restart
> > + * @type:  Shutdown or restart? 0 for shutdown, 1 for restart
> > + * @subtype:   Specifies which system should be restarted or shut down
> > + *
> > + * Return: Returns status, either success or error+reason
> > + */
> > +static int zynqmp_pm_system_shutdown(const u32 type, const u32
> > +subtype) {
> > +   return invoke_pm_fn(PM_SYSTEM_SHUTDOWN, type, subtype, 0, 0,
> NULL);
> > +}
> 
> PSCI already has this functionality, so I'm a little confused by the 
> duplication.
> 
PSCI doesn't distinguish between shutdown scope. It can be APU/PS/System in 
this case.

> [...]
> 
> > +/**
> > + * zynqmp_pm_get_node_status - PM call to request a node's current power
> state
> > + * @node:  ID of the component or sub-system in question
> > + * @status:Current operating state of the requested node
> > + * @requirements:  Current requirements asserted on the node,
> > + * used for slave nodes only.
> > + * @usage: Usage information, used for slave nodes only:
> > + * 0 - No master is currently using the node
> > + * 1 - Only requesting master is currently using the node
> > + * 2 - Only other masters are currently using the node
> > + * 3 - Both the current and at least one other master
> > + * is currently using the node
> 
> These should probably have corresponding macros or enum values.
Will add macros in next version patch.

> 
> [...]
> 
> > +/**
> > + * zynqmp_pm_sha_hash - Access the SHA engine to calculate the hash
> > + * @address:   Address of the data/ Address of output buffer where
> > + * hash should be stored.
> > + * @size:  Size of the data.
> > + * @flags:
> > + * BIT(0) - Sha3 init (Here address and size inputs can be NULL)
> > + * BIT(1) - Sha3 update (address should holds the )
> 
> Missing "data", I guess?
Yes will update in next version patch.

> 
> > + * BIT(2) - Sha3 final (address should hold the address of
> > + *  buffer to store hash)
> 
> Is the SHA engine coherent? Or is cache maintenance necessary?
> 
> [...]
It is coherent. Update/Final has below significance here:
BIT(1) - To call Sha3_Update API which can be called multiple times when data 
is not contiguous.
BIT(2) - to get final hash of the whole updated data. Hash will be overwr

RE: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware driver

[Jolly Shah]

Hi Shubhrajyoti,
Thanks for the review,

> -Original Message-
> From: Shubhrajyoti Datta [mailto:shubhrajyoti.da...@gmail.com]
> Sent: Monday, January 29, 2018 9:06 PM
> To: Jolly Shah <jol...@xilinx.com>
> Cc: ard.biesheu...@linaro.org; mi...@kernel.org; Greg Kroah-Hartman
> <gre...@linuxfoundation.org>; m...@codeblueprint.co.uk;
> sudeep.ho...@arm.com; hkallwe...@gmail.com; keesc...@chromium.org;
> dmitry.torok...@gmail.com; Michal Simek <michal.si...@xilinx.com>; Rob
> Herring <robh...@kernel.org>; Mark Rutland <mark.rutl...@arm.com>; linux-
> arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org; Jolly Shah <jol...@xilinx.com>; Rajan Vaja
> <raj...@xilinx.com>
> Subject: Re: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware
> driver
> 
> Hi,
> 
> Thanks for the patch.
> A few questions below.
> 
> 
> On Thu, Jan 25, 2018 at 4:51 AM, Jolly Shah <jolly.s...@xilinx.com> wrote:
> > This patch is adding communication layer with firmware.
> > Firmware driver provides an interface to firmware APIs.
> > Interface APIs can be used by any driver to communicate to
> > PMUFW(Platform Management Unit). All requests go through ATF.
> >
> > Signed-off-by: Jolly Shah <jol...@xilinx.com>
> > Signed-off-by: Rajan Vaja <raj...@xilinx.com>
> > ---
> 
> >
> 
> > +/**
> > + * zynqmp_pm_clock_enable - Enable the clock for given id
> > + * @clock_id:  ID of the clock to be enabled
> 
> Does it enable all the parents also if they are disabled?
Current solution enables specified clock only. We are working on enhancing the 
solution to take care of other dependent clocks.

> 
> > + *
> > + * This function is used by master to enable the clock
> > + * including peripherals and PLL clocks.
> > + *
> > + * Return: Returns status, either success or error+reason.
> > + */
> > +static int zynqmp_pm_clock_enable(u32 clock_id) {
> > +   return invoke_pm_fn(PM_CLOCK_ENABLE, clock_id, 0, 0, 0, NULL);
> > +}
> > +
> > +/**
> > + * zynqmp_pm_clock_disable - Disable the clock for given id
> > + * @clock_id:  ID of the clock to be disable
> > + *
> > + * This function is used by master to disable the clock
> > + * including peripherals and PLL clocks.
> > + *
> > + * Return: Returns status, either success or error+reason.
> > + */
> > +static int zynqmp_pm_clock_disable(u32 clock_id) {
> > +   return invoke_pm_fn(PM_CLOCK_DISABLE, clock_id, 0, 0, 0,
> > +NULL); }
> > +
> > +/**
> > + * zynqmp_pm_clock_getstate - Get the clock state for given id
> > + * @clock_id:  ID of the clock to be queried
> > + * @state: 1/0 (Enabled/Disabled)
> > + *
> > + * This function is used by master to get the state of clock
> > + * including peripherals and PLL clocks.
> > + *
> > + * Return: Returns status, either success or error+reason.
> > + */
> > +static int zynqmp_pm_clock_getstate(u32 clock_id, u32 *state) {
> > +   u32 ret_payload[PAYLOAD_ARG_CNT];
> > +   int ret;
> > +
> > +   ret = invoke_pm_fn(PM_CLOCK_GETSTATE, clock_id, 0, 0, 0,
> ret_payload);
> > +   *state = ret_payload[1];
> > +
> > +   return ret;
> > +}
> > +
> > +/**
> > + * zynqmp_pm_clock_setdivider - Set the clock divider for given id
> > + * @clock_id:  ID of the clock
> > + * @div_type:  TYPE_DIV1: div1
> > + * TYPE_DIV2: div2
> div type didnt see in the signature.


Will be fixed in next version.

> 
> 
> 
> > + * @divider:   divider value.
> > + *
> > + * This function is used by master to set divider for any clock
> > + * to achieve desired rate.
> > + *
> > + * Return: Returns status, either success or error+reason.
> > + */
> > +static int zynqmp_pm_clock_setdivider(u32 clock_id, u32 divider) {
> > +   return invoke_pm_fn(PM_CLOCK_SETDIVIDER, clock_id, divider, 0,
> > +0, NULL); }
> > +
> > +/**
> > + * zynqmp_pm_clock_getdivider - Get the clock divider for given id
> > + * @clock_id:  ID of the clock
> > + * @div_type:  TYPE_DIV1: div1
> > + * TYPE_DIV2: div2
> didnt see this  below.
Will be fixed in next version.


> 
> > + * @divider:   divider value.
> > + *
> > + * This function is used by master to get divider values
> > + * for any clock.
> > + *
> > + * Return: Returns status, either success or error+reason.
> > + */
> > +static int zynqmp_pm_clock_getdivider(u32 c

RE: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware driver

[Jolly Shah]

Hi Shubhrajyoti,
Thanks for the review,

> -Original Message-
> From: Shubhrajyoti Datta [mailto:shubhrajyoti.da...@gmail.com]
> Sent: Monday, January 29, 2018 9:06 PM
> To: Jolly Shah 
> Cc: ard.biesheu...@linaro.org; mi...@kernel.org; Greg Kroah-Hartman
> ; m...@codeblueprint.co.uk;
> sudeep.ho...@arm.com; hkallwe...@gmail.com; keesc...@chromium.org;
> dmitry.torok...@gmail.com; Michal Simek ; Rob
> Herring ; Mark Rutland ; linux-
> arm-ker...@lists.infradead.org; linux-kernel@vger.kernel.org;
> devicet...@vger.kernel.org; Jolly Shah ; Rajan Vaja
> 
> Subject: Re: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware
> driver
> 
> Hi,
> 
> Thanks for the patch.
> A few questions below.
> 
> 
> On Thu, Jan 25, 2018 at 4:51 AM, Jolly Shah  wrote:
> > This patch is adding communication layer with firmware.
> > Firmware driver provides an interface to firmware APIs.
> > Interface APIs can be used by any driver to communicate to
> > PMUFW(Platform Management Unit). All requests go through ATF.
> >
> > Signed-off-by: Jolly Shah 
> > Signed-off-by: Rajan Vaja 
> > ---
> 
> >
> 
> > +/**
> > + * zynqmp_pm_clock_enable - Enable the clock for given id
> > + * @clock_id:  ID of the clock to be enabled
> 
> Does it enable all the parents also if they are disabled?
Current solution enables specified clock only. We are working on enhancing the 
solution to take care of other dependent clocks.

> 
> > + *
> > + * This function is used by master to enable the clock
> > + * including peripherals and PLL clocks.
> > + *
> > + * Return: Returns status, either success or error+reason.
> > + */
> > +static int zynqmp_pm_clock_enable(u32 clock_id) {
> > +   return invoke_pm_fn(PM_CLOCK_ENABLE, clock_id, 0, 0, 0, NULL);
> > +}
> > +
> > +/**
> > + * zynqmp_pm_clock_disable - Disable the clock for given id
> > + * @clock_id:  ID of the clock to be disable
> > + *
> > + * This function is used by master to disable the clock
> > + * including peripherals and PLL clocks.
> > + *
> > + * Return: Returns status, either success or error+reason.
> > + */
> > +static int zynqmp_pm_clock_disable(u32 clock_id) {
> > +   return invoke_pm_fn(PM_CLOCK_DISABLE, clock_id, 0, 0, 0,
> > +NULL); }
> > +
> > +/**
> > + * zynqmp_pm_clock_getstate - Get the clock state for given id
> > + * @clock_id:  ID of the clock to be queried
> > + * @state: 1/0 (Enabled/Disabled)
> > + *
> > + * This function is used by master to get the state of clock
> > + * including peripherals and PLL clocks.
> > + *
> > + * Return: Returns status, either success or error+reason.
> > + */
> > +static int zynqmp_pm_clock_getstate(u32 clock_id, u32 *state) {
> > +   u32 ret_payload[PAYLOAD_ARG_CNT];
> > +   int ret;
> > +
> > +   ret = invoke_pm_fn(PM_CLOCK_GETSTATE, clock_id, 0, 0, 0,
> ret_payload);
> > +   *state = ret_payload[1];
> > +
> > +   return ret;
> > +}
> > +
> > +/**
> > + * zynqmp_pm_clock_setdivider - Set the clock divider for given id
> > + * @clock_id:  ID of the clock
> > + * @div_type:  TYPE_DIV1: div1
> > + * TYPE_DIV2: div2
> div type didnt see in the signature.


Will be fixed in next version.

> 
> 
> 
> > + * @divider:   divider value.
> > + *
> > + * This function is used by master to set divider for any clock
> > + * to achieve desired rate.
> > + *
> > + * Return: Returns status, either success or error+reason.
> > + */
> > +static int zynqmp_pm_clock_setdivider(u32 clock_id, u32 divider) {
> > +   return invoke_pm_fn(PM_CLOCK_SETDIVIDER, clock_id, divider, 0,
> > +0, NULL); }
> > +
> > +/**
> > + * zynqmp_pm_clock_getdivider - Get the clock divider for given id
> > + * @clock_id:  ID of the clock
> > + * @div_type:  TYPE_DIV1: div1
> > + * TYPE_DIV2: div2
> didnt see this  below.
Will be fixed in next version.


> 
> > + * @divider:   divider value.
> > + *
> > + * This function is used by master to get divider values
> > + * for any clock.
> > + *
> > + * Return: Returns status, either success or error+reason.
> > + */
> > +static int zynqmp_pm_clock_getdivider(u32 clock_id, u32 *divider) {
> > +   u32 ret_payload[PAYLOAD_ARG_CNT];
> > +   int ret;
> > +
> > +   ret = invoke_pm_fn(PM_CLOCK_GETDIVIDER, clock_id, 0, 0, 0,
> ret_payload);
> > +   *divider = ret_payload[1];
> > +
> >

Re: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware driver

[Mark Rutland]

On Wed, Jan 24, 2018 at 03:21:12PM -0800, Jolly Shah wrote:
> This patch is adding communication layer with firmware.
> Firmware driver provides an interface to firmware APIs.
> Interface APIs can be used by any driver to communicate to
> PMUFW(Platform Management Unit). All requests go through ATF.

> +/**
> + * zynqmp_pm_set_wakeup_source - PM call to specify the wakeup source
> + *   while suspended
> + * @target:  Node ID of the targeted PU or subsystem
> + * @wakeup_node:Node ID of the wakeup peripheral
> + * @enable:  Enable or disable the specified peripheral as wake source
> + *
> + * Return:   Returns status, either success or error+reason
> + */
> +static int zynqmp_pm_set_wakeup_source(const u32 target,
> +const u32 wakeup_node,
> +const u32 enable)
> +{
> + return invoke_pm_fn(PM_SET_WAKEUP_SOURCE, target,
> + wakeup_node, enable, 0, NULL);
> +}

I see many functions take a "Node ID" parameter, but these don't appear
to be in any DT binding, and drivers (other than the debugfs driver)
aren't using them.

What's the plan for making use of these? Where are the node IDs going to
come from in practice?

> +/**
> + * zynqmp_pm_system_shutdown - PM call to request a system shutdown or 
> restart
> + * @type:Shutdown or restart? 0 for shutdown, 1 for restart
> + * @subtype: Specifies which system should be restarted or shut down
> + *
> + * Return:   Returns status, either success or error+reason
> + */
> +static int zynqmp_pm_system_shutdown(const u32 type, const u32 subtype)
> +{
> + return invoke_pm_fn(PM_SYSTEM_SHUTDOWN, type, subtype, 0, 0, NULL);
> +}

PSCI already has this functionality, so I'm a little confused by the
duplication.

[...]

> +/**
> + * zynqmp_pm_get_node_status - PM call to request a node's current power 
> state
> + * @node:ID of the component or sub-system in question
> + * @status:  Current operating state of the requested node
> + * @requirements:Current requirements asserted on the node,
> + *   used for slave nodes only.
> + * @usage:   Usage information, used for slave nodes only:
> + *   0 - No master is currently using the node
> + *   1 - Only requesting master is currently using the node
> + *   2 - Only other masters are currently using the node
> + *   3 - Both the current and at least one other master
> + *   is currently using the node

These should probably have corresponding macros or enum values.

[...]

> +/**
> + * zynqmp_pm_sha_hash - Access the SHA engine to calculate the hash
> + * @address: Address of the data/ Address of output buffer where
> + *   hash should be stored.
> + * @size:Size of the data.
> + * @flags:
> + *   BIT(0) - Sha3 init (Here address and size inputs can be NULL)
> + *   BIT(1) - Sha3 update (address should holds the )

Missing "data", I guess?

> + *   BIT(2) - Sha3 final (address should hold the address of
> + *buffer to store hash)

Is the SHA engine coherent? Or is cache maintenance necessary?

[...]

> +/**
> + * zynqmp_pm_pinctrl_request - Request Pin from firmware
> + * @pin: Pin number to request
> + *

No DT binding for the pinctrl bits?

[...]

> +/**
> + * zynqmp_pm_clock_enable - Enable the clock for given id
> + * @clock_id:ID of the clock to be enabled
> + *

Likewise for the clocks?

> +/**
> + * get_eemi_ops  - Get eemi ops functions
> + *
> + * Return:   - pointer of eemi_ops structure
> + */
> +const struct zynqmp_eemi_ops *get_eemi_ops(void)
> +{
> + return _ops;
> +}
> +EXPORT_SYMBOL_GPL(get_eemi_ops);
> +
> +static int __init zynqmp_plat_init(void)
> +{
> + struct device_node *np;
> + int ret = 0;
> +
> + np = of_find_compatible_node(NULL, NULL, "xlnx,zynqmp");
> + if (!np)
> + return 0;
> + of_node_put(np);
> +
> + /* We're running on a ZynqMP machine, the PM node is mandatory. */
> + np = of_find_compatible_node(NULL, NULL, "xlnx,zynqmp-firmware");
> + if (!np) {
> + pr_warn("%s: pm node not found\n", __func__);
> + return -ENXIO;
> + }
> +
> + ret = get_set_conduit_method(np);
> + if (ret) {
> + of_node_put(np);
> + return ret;
> + }
> +
> + /* Check PM API version number */
> + zynqmp_pm_get_api_version(_api_version);
> + if (pm_api_version != ZYNQMP_PM_VERSION) {
> + panic("%s power management API version error. Expected: v%d.%d 
> - Found: v%d.%d\n",
> +   __func__,
> +   ZYNQMP_PM_VERSION_MAJOR, ZYNQMP_PM_VERSION_MINOR,
> +   pm_api_version >> 16, pm_api_version & 0x);
> + }
> +
> + pr_info("%s Power management API v%d.%d\n", __func__,
> + ZYNQMP_PM_VERSION_MAJOR, ZYNQMP_PM_VERSION_MINOR);
> +
> +   

Re: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware driver

[Mark Rutland]

On Wed, Jan 24, 2018 at 03:21:12PM -0800, Jolly Shah wrote:
> This patch is adding communication layer with firmware.
> Firmware driver provides an interface to firmware APIs.
> Interface APIs can be used by any driver to communicate to
> PMUFW(Platform Management Unit). All requests go through ATF.

> +/**
> + * zynqmp_pm_set_wakeup_source - PM call to specify the wakeup source
> + *   while suspended
> + * @target:  Node ID of the targeted PU or subsystem
> + * @wakeup_node:Node ID of the wakeup peripheral
> + * @enable:  Enable or disable the specified peripheral as wake source
> + *
> + * Return:   Returns status, either success or error+reason
> + */
> +static int zynqmp_pm_set_wakeup_source(const u32 target,
> +const u32 wakeup_node,
> +const u32 enable)
> +{
> + return invoke_pm_fn(PM_SET_WAKEUP_SOURCE, target,
> + wakeup_node, enable, 0, NULL);
> +}

I see many functions take a "Node ID" parameter, but these don't appear
to be in any DT binding, and drivers (other than the debugfs driver)
aren't using them.

What's the plan for making use of these? Where are the node IDs going to
come from in practice?

> +/**
> + * zynqmp_pm_system_shutdown - PM call to request a system shutdown or 
> restart
> + * @type:Shutdown or restart? 0 for shutdown, 1 for restart
> + * @subtype: Specifies which system should be restarted or shut down
> + *
> + * Return:   Returns status, either success or error+reason
> + */
> +static int zynqmp_pm_system_shutdown(const u32 type, const u32 subtype)
> +{
> + return invoke_pm_fn(PM_SYSTEM_SHUTDOWN, type, subtype, 0, 0, NULL);
> +}

PSCI already has this functionality, so I'm a little confused by the
duplication.

[...]

> +/**
> + * zynqmp_pm_get_node_status - PM call to request a node's current power 
> state
> + * @node:ID of the component or sub-system in question
> + * @status:  Current operating state of the requested node
> + * @requirements:Current requirements asserted on the node,
> + *   used for slave nodes only.
> + * @usage:   Usage information, used for slave nodes only:
> + *   0 - No master is currently using the node
> + *   1 - Only requesting master is currently using the node
> + *   2 - Only other masters are currently using the node
> + *   3 - Both the current and at least one other master
> + *   is currently using the node

These should probably have corresponding macros or enum values.

[...]

> +/**
> + * zynqmp_pm_sha_hash - Access the SHA engine to calculate the hash
> + * @address: Address of the data/ Address of output buffer where
> + *   hash should be stored.
> + * @size:Size of the data.
> + * @flags:
> + *   BIT(0) - Sha3 init (Here address and size inputs can be NULL)
> + *   BIT(1) - Sha3 update (address should holds the )

Missing "data", I guess?

> + *   BIT(2) - Sha3 final (address should hold the address of
> + *buffer to store hash)

Is the SHA engine coherent? Or is cache maintenance necessary?

[...]

> +/**
> + * zynqmp_pm_pinctrl_request - Request Pin from firmware
> + * @pin: Pin number to request
> + *

No DT binding for the pinctrl bits?

[...]

> +/**
> + * zynqmp_pm_clock_enable - Enable the clock for given id
> + * @clock_id:ID of the clock to be enabled
> + *

Likewise for the clocks?

> +/**
> + * get_eemi_ops  - Get eemi ops functions
> + *
> + * Return:   - pointer of eemi_ops structure
> + */
> +const struct zynqmp_eemi_ops *get_eemi_ops(void)
> +{
> + return _ops;
> +}
> +EXPORT_SYMBOL_GPL(get_eemi_ops);
> +
> +static int __init zynqmp_plat_init(void)
> +{
> + struct device_node *np;
> + int ret = 0;
> +
> + np = of_find_compatible_node(NULL, NULL, "xlnx,zynqmp");
> + if (!np)
> + return 0;
> + of_node_put(np);
> +
> + /* We're running on a ZynqMP machine, the PM node is mandatory. */
> + np = of_find_compatible_node(NULL, NULL, "xlnx,zynqmp-firmware");
> + if (!np) {
> + pr_warn("%s: pm node not found\n", __func__);
> + return -ENXIO;
> + }
> +
> + ret = get_set_conduit_method(np);
> + if (ret) {
> + of_node_put(np);
> + return ret;
> + }
> +
> + /* Check PM API version number */
> + zynqmp_pm_get_api_version(_api_version);
> + if (pm_api_version != ZYNQMP_PM_VERSION) {
> + panic("%s power management API version error. Expected: v%d.%d 
> - Found: v%d.%d\n",
> +   __func__,
> +   ZYNQMP_PM_VERSION_MAJOR, ZYNQMP_PM_VERSION_MINOR,
> +   pm_api_version >> 16, pm_api_version & 0x);
> + }
> +
> + pr_info("%s Power management API v%d.%d\n", __func__,
> + ZYNQMP_PM_VERSION_MAJOR, ZYNQMP_PM_VERSION_MINOR);
> +
> +   

Re: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware driver

[Shubhrajyoti Datta]

Hi,

Thanks for the patch.
A few questions below.


On Thu, Jan 25, 2018 at 4:51 AM, Jolly Shah  wrote:
> This patch is adding communication layer with firmware.
> Firmware driver provides an interface to firmware APIs.
> Interface APIs can be used by any driver to communicate to
> PMUFW(Platform Management Unit). All requests go through ATF.
>
> Signed-off-by: Jolly Shah 
> Signed-off-by: Rajan Vaja 
> ---

>

> +/**
> + * zynqmp_pm_clock_enable - Enable the clock for given id
> + * @clock_id:  ID of the clock to be enabled

Does it enable all the parents also if they are disabled?

> + *
> + * This function is used by master to enable the clock
> + * including peripherals and PLL clocks.
> + *
> + * Return: Returns status, either success or error+reason.
> + */
> +static int zynqmp_pm_clock_enable(u32 clock_id)
> +{
> +   return invoke_pm_fn(PM_CLOCK_ENABLE, clock_id, 0, 0, 0, NULL);
> +}
> +
> +/**
> + * zynqmp_pm_clock_disable - Disable the clock for given id
> + * @clock_id:  ID of the clock to be disable
> + *
> + * This function is used by master to disable the clock
> + * including peripherals and PLL clocks.
> + *
> + * Return: Returns status, either success or error+reason.
> + */
> +static int zynqmp_pm_clock_disable(u32 clock_id)
> +{
> +   return invoke_pm_fn(PM_CLOCK_DISABLE, clock_id, 0, 0, 0, NULL);
> +}
> +
> +/**
> + * zynqmp_pm_clock_getstate - Get the clock state for given id
> + * @clock_id:  ID of the clock to be queried
> + * @state: 1/0 (Enabled/Disabled)
> + *
> + * This function is used by master to get the state of clock
> + * including peripherals and PLL clocks.
> + *
> + * Return: Returns status, either success or error+reason.
> + */
> +static int zynqmp_pm_clock_getstate(u32 clock_id, u32 *state)
> +{
> +   u32 ret_payload[PAYLOAD_ARG_CNT];
> +   int ret;
> +
> +   ret = invoke_pm_fn(PM_CLOCK_GETSTATE, clock_id, 0, 0, 0, ret_payload);
> +   *state = ret_payload[1];
> +
> +   return ret;
> +}
> +
> +/**
> + * zynqmp_pm_clock_setdivider - Set the clock divider for given id
> + * @clock_id:  ID of the clock
> + * @div_type:  TYPE_DIV1: div1
> + * TYPE_DIV2: div2
div type didnt see in the signature.



> + * @divider:   divider value.
> + *
> + * This function is used by master to set divider for any clock
> + * to achieve desired rate.
> + *
> + * Return: Returns status, either success or error+reason.
> + */
> +static int zynqmp_pm_clock_setdivider(u32 clock_id, u32 divider)
> +{
> +   return invoke_pm_fn(PM_CLOCK_SETDIVIDER, clock_id, divider, 0, 0, 
> NULL);
> +}
> +
> +/**
> + * zynqmp_pm_clock_getdivider - Get the clock divider for given id
> + * @clock_id:  ID of the clock
> + * @div_type:  TYPE_DIV1: div1
> + * TYPE_DIV2: div2
didnt see this  below.

> + * @divider:   divider value.
> + *
> + * This function is used by master to get divider values
> + * for any clock.
> + *
> + * Return: Returns status, either success or error+reason.
> + */
> +static int zynqmp_pm_clock_getdivider(u32 clock_id, u32 *divider)
> +{
> +   u32 ret_payload[PAYLOAD_ARG_CNT];
> +   int ret;
> +
> +   ret = invoke_pm_fn(PM_CLOCK_GETDIVIDER, clock_id, 0, 0, 0, 
> ret_payload);
> +   *divider = ret_payload[1];
> +
> +   return ret;
> +}
> +
> +/**
> + * zynqmp_pm_clock_setrate - Set the clock rate for given id
> + * @clock_id:  ID of the clock
> + * @rate:  rate value in hz
> + *
> + * This function is used by master to set rate for any clock.
> + *
> + * Return: Returns status, either success or error+reason.
> + */
So this can set rate only 4G max ?

> +static int zynqmp_pm_clock_setrate(u32 clock_id, u32 rate)
> +{
> +   return invoke_pm_fn(PM_CLOCK_SETRATE, clock_id, rate, 0, 0, NULL);
> +}
> +
> +/**
> + * zynqmp_pm_clock_getrate - Get the clock rate for given id
> + * @clock_id:  ID of the clock
> + * @rate:  rate value in hz
> + *
> + * This function is used by master to get rate
> + * for any clock.
> + *
> + * Return: Returns status, either success or error+reason.
> + */
Same question here?

> +static int zynqmp_pm_clock_getrate(u32 clock_id, u32 *rate)
> +{
> +   u32 ret_payload[PAYLOAD_ARG_CNT];
> +   int ret;
> +
> +   ret = invoke_pm_fn(PM_CLOCK_GETRATE, clock_id, 0, 0, 0, ret_payload);
> +   *rate = ret_payload[1];
> +
> +   return ret;
> +}
> +
Also  what is the difference between set rate and set divider?

Re: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware driver

[Shubhrajyoti Datta]

Hi,

Thanks for the patch.
A few questions below.


On Thu, Jan 25, 2018 at 4:51 AM, Jolly Shah  wrote:
> This patch is adding communication layer with firmware.
> Firmware driver provides an interface to firmware APIs.
> Interface APIs can be used by any driver to communicate to
> PMUFW(Platform Management Unit). All requests go through ATF.
>
> Signed-off-by: Jolly Shah 
> Signed-off-by: Rajan Vaja 
> ---

>

> +/**
> + * zynqmp_pm_clock_enable - Enable the clock for given id
> + * @clock_id:  ID of the clock to be enabled

Does it enable all the parents also if they are disabled?

> + *
> + * This function is used by master to enable the clock
> + * including peripherals and PLL clocks.
> + *
> + * Return: Returns status, either success or error+reason.
> + */
> +static int zynqmp_pm_clock_enable(u32 clock_id)
> +{
> +   return invoke_pm_fn(PM_CLOCK_ENABLE, clock_id, 0, 0, 0, NULL);
> +}
> +
> +/**
> + * zynqmp_pm_clock_disable - Disable the clock for given id
> + * @clock_id:  ID of the clock to be disable
> + *
> + * This function is used by master to disable the clock
> + * including peripherals and PLL clocks.
> + *
> + * Return: Returns status, either success or error+reason.
> + */
> +static int zynqmp_pm_clock_disable(u32 clock_id)
> +{
> +   return invoke_pm_fn(PM_CLOCK_DISABLE, clock_id, 0, 0, 0, NULL);
> +}
> +
> +/**
> + * zynqmp_pm_clock_getstate - Get the clock state for given id
> + * @clock_id:  ID of the clock to be queried
> + * @state: 1/0 (Enabled/Disabled)
> + *
> + * This function is used by master to get the state of clock
> + * including peripherals and PLL clocks.
> + *
> + * Return: Returns status, either success or error+reason.
> + */
> +static int zynqmp_pm_clock_getstate(u32 clock_id, u32 *state)
> +{
> +   u32 ret_payload[PAYLOAD_ARG_CNT];
> +   int ret;
> +
> +   ret = invoke_pm_fn(PM_CLOCK_GETSTATE, clock_id, 0, 0, 0, ret_payload);
> +   *state = ret_payload[1];
> +
> +   return ret;
> +}
> +
> +/**
> + * zynqmp_pm_clock_setdivider - Set the clock divider for given id
> + * @clock_id:  ID of the clock
> + * @div_type:  TYPE_DIV1: div1
> + * TYPE_DIV2: div2
div type didnt see in the signature.



> + * @divider:   divider value.
> + *
> + * This function is used by master to set divider for any clock
> + * to achieve desired rate.
> + *
> + * Return: Returns status, either success or error+reason.
> + */
> +static int zynqmp_pm_clock_setdivider(u32 clock_id, u32 divider)
> +{
> +   return invoke_pm_fn(PM_CLOCK_SETDIVIDER, clock_id, divider, 0, 0, 
> NULL);
> +}
> +
> +/**
> + * zynqmp_pm_clock_getdivider - Get the clock divider for given id
> + * @clock_id:  ID of the clock
> + * @div_type:  TYPE_DIV1: div1
> + * TYPE_DIV2: div2
didnt see this  below.

> + * @divider:   divider value.
> + *
> + * This function is used by master to get divider values
> + * for any clock.
> + *
> + * Return: Returns status, either success or error+reason.
> + */
> +static int zynqmp_pm_clock_getdivider(u32 clock_id, u32 *divider)
> +{
> +   u32 ret_payload[PAYLOAD_ARG_CNT];
> +   int ret;
> +
> +   ret = invoke_pm_fn(PM_CLOCK_GETDIVIDER, clock_id, 0, 0, 0, 
> ret_payload);
> +   *divider = ret_payload[1];
> +
> +   return ret;
> +}
> +
> +/**
> + * zynqmp_pm_clock_setrate - Set the clock rate for given id
> + * @clock_id:  ID of the clock
> + * @rate:  rate value in hz
> + *
> + * This function is used by master to set rate for any clock.
> + *
> + * Return: Returns status, either success or error+reason.
> + */
So this can set rate only 4G max ?

> +static int zynqmp_pm_clock_setrate(u32 clock_id, u32 rate)
> +{
> +   return invoke_pm_fn(PM_CLOCK_SETRATE, clock_id, rate, 0, 0, NULL);
> +}
> +
> +/**
> + * zynqmp_pm_clock_getrate - Get the clock rate for given id
> + * @clock_id:  ID of the clock
> + * @rate:  rate value in hz
> + *
> + * This function is used by master to get rate
> + * for any clock.
> + *
> + * Return: Returns status, either success or error+reason.
> + */
Same question here?

> +static int zynqmp_pm_clock_getrate(u32 clock_id, u32 *rate)
> +{
> +   u32 ret_payload[PAYLOAD_ARG_CNT];
> +   int ret;
> +
> +   ret = invoke_pm_fn(PM_CLOCK_GETRATE, clock_id, 0, 0, 0, ret_payload);
> +   *rate = ret_payload[1];
> +
> +   return ret;
> +}
> +
Also  what is the difference between set rate and set divider?

[PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware driver

[Jolly Shah]

This patch is adding communication layer with firmware.
Firmware driver provides an interface to firmware APIs.
Interface APIs can be used by any driver to communicate to
PMUFW(Platform Management Unit). All requests go through ATF.

Signed-off-by: Jolly Shah 
Signed-off-by: Rajan Vaja 
---
 arch/arm64/Kconfig.platforms|   1 +
 drivers/firmware/Kconfig|   1 +
 drivers/firmware/Makefile   |   1 +
 drivers/firmware/xilinx/Kconfig |   4 +
 drivers/firmware/xilinx/Makefile|   4 +
 drivers/firmware/xilinx/zynqmp/Kconfig  |  16 +
 drivers/firmware/xilinx/zynqmp/Makefile |   4 +
 drivers/firmware/xilinx/zynqmp/firmware.c   | 999 
 include/linux/firmware/xilinx/zynqmp/firmware.h | 576 ++
 9 files changed, 1606 insertions(+)
 create mode 100644 drivers/firmware/xilinx/Kconfig
 create mode 100644 drivers/firmware/xilinx/Makefile
 create mode 100644 drivers/firmware/xilinx/zynqmp/Kconfig
 create mode 100644 drivers/firmware/xilinx/zynqmp/Makefile
 create mode 100644 drivers/firmware/xilinx/zynqmp/firmware.c
 create mode 100644 include/linux/firmware/xilinx/zynqmp/firmware.h

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 2401373..3dd3ae9 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -273,6 +273,7 @@ config ARCH_ZX
 
 config ARCH_ZYNQMP
bool "Xilinx ZynqMP Family"
+   select ZYNQMP_FIRMWARE
help
  This enables support for Xilinx ZynqMP Family
 
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index fa87a055..18fc2a8 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -249,5 +249,6 @@ source "drivers/firmware/google/Kconfig"
 source "drivers/firmware/efi/Kconfig"
 source "drivers/firmware/meson/Kconfig"
 source "drivers/firmware/tegra/Kconfig"
+source "drivers/firmware/xilinx/Kconfig"
 
 endmenu
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index feaa890..43a24b5 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -30,3 +30,4 @@ obj-$(CONFIG_GOOGLE_FIRMWARE) += google/
 obj-$(CONFIG_EFI)  += efi/
 obj-$(CONFIG_UEFI_CPER)+= efi/
 obj-y  += tegra/
+obj-y  += xilinx/
diff --git a/drivers/firmware/xilinx/Kconfig b/drivers/firmware/xilinx/Kconfig
new file mode 100644
index 000..eb4cdcf
--- /dev/null
+++ b/drivers/firmware/xilinx/Kconfig
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Kconfig for Xilinx firmwares
+
+source "drivers/firmware/xilinx/zynqmp/Kconfig"
diff --git a/drivers/firmware/xilinx/Makefile b/drivers/firmware/xilinx/Makefile
new file mode 100644
index 000..beff5dc
--- /dev/null
+++ b/drivers/firmware/xilinx/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Makefile for Xilinx firmwares
+
+obj-$(CONFIG_ARCH_ZYNQMP) += zynqmp/
diff --git a/drivers/firmware/xilinx/zynqmp/Kconfig 
b/drivers/firmware/xilinx/zynqmp/Kconfig
new file mode 100644
index 000..8f7709d
--- /dev/null
+++ b/drivers/firmware/xilinx/zynqmp/Kconfig
@@ -0,0 +1,16 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Kconfig for Xilinx zynqmp firmware
+
+menu "Zynq MPSoC Firmware Drivers"
+   depends on ARCH_ZYNQMP
+
+config ZYNQMP_FIRMWARE
+   bool "Enable Xilinx Zynq MPSoC firmware interface"
+   help
+ Firmware interface driver is used by different to
+ communicate with the firmware for various platform
+ management services.
+ Say yes to enable zynqmp firmware interface driver.
+ In doubt, say N
+
+endmenu
diff --git a/drivers/firmware/xilinx/zynqmp/Makefile 
b/drivers/firmware/xilinx/zynqmp/Makefile
new file mode 100644
index 000..c3ec669
--- /dev/null
+++ b/drivers/firmware/xilinx/zynqmp/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Makefile for Xilinx firmwares
+
+obj-$(CONFIG_ZYNQMP_FIRMWARE) += firmware.o
diff --git a/drivers/firmware/xilinx/zynqmp/firmware.c 
b/drivers/firmware/xilinx/zynqmp/firmware.c
new file mode 100644
index 000..2313ae7
--- /dev/null
+++ b/drivers/firmware/xilinx/zynqmp/firmware.c
@@ -0,0 +1,999 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Xilinx Zynq MPSoC Firmware layer
+ *
+ *  Copyright (C) 2014-2018 Xilinx, Inc.
+ *
+ *  Michal Simek 
+ *  Davorin Mista 
+ *  Jolly Shah 
+ *  Rajan Vaja 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+/**
+ * zynqmp_pm_ret_code - Convert PMU-FW error codes to Linux error codes
+ * @ret_status:PMUFW return code
+ *
+ * Return: corresponding Linux error code
+ */
+int zynqmp_pm_ret_code(u32 ret_status)
+{
+   switch (ret_status) {
+   case XST_PM_SUCCESS:
+   

[PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware driver

[Jolly Shah]

This patch is adding communication layer with firmware.
Firmware driver provides an interface to firmware APIs.
Interface APIs can be used by any driver to communicate to
PMUFW(Platform Management Unit). All requests go through ATF.

Signed-off-by: Jolly Shah 
Signed-off-by: Rajan Vaja 
---
 arch/arm64/Kconfig.platforms|   1 +
 drivers/firmware/Kconfig|   1 +
 drivers/firmware/Makefile   |   1 +
 drivers/firmware/xilinx/Kconfig |   4 +
 drivers/firmware/xilinx/Makefile|   4 +
 drivers/firmware/xilinx/zynqmp/Kconfig  |  16 +
 drivers/firmware/xilinx/zynqmp/Makefile |   4 +
 drivers/firmware/xilinx/zynqmp/firmware.c   | 999 
 include/linux/firmware/xilinx/zynqmp/firmware.h | 576 ++
 9 files changed, 1606 insertions(+)
 create mode 100644 drivers/firmware/xilinx/Kconfig
 create mode 100644 drivers/firmware/xilinx/Makefile
 create mode 100644 drivers/firmware/xilinx/zynqmp/Kconfig
 create mode 100644 drivers/firmware/xilinx/zynqmp/Makefile
 create mode 100644 drivers/firmware/xilinx/zynqmp/firmware.c
 create mode 100644 include/linux/firmware/xilinx/zynqmp/firmware.h

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index 2401373..3dd3ae9 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -273,6 +273,7 @@ config ARCH_ZX
 
 config ARCH_ZYNQMP
bool "Xilinx ZynqMP Family"
+   select ZYNQMP_FIRMWARE
help
  This enables support for Xilinx ZynqMP Family
 
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index fa87a055..18fc2a8 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -249,5 +249,6 @@ source "drivers/firmware/google/Kconfig"
 source "drivers/firmware/efi/Kconfig"
 source "drivers/firmware/meson/Kconfig"
 source "drivers/firmware/tegra/Kconfig"
+source "drivers/firmware/xilinx/Kconfig"
 
 endmenu
diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
index feaa890..43a24b5 100644
--- a/drivers/firmware/Makefile
+++ b/drivers/firmware/Makefile
@@ -30,3 +30,4 @@ obj-$(CONFIG_GOOGLE_FIRMWARE) += google/
 obj-$(CONFIG_EFI)  += efi/
 obj-$(CONFIG_UEFI_CPER)+= efi/
 obj-y  += tegra/
+obj-y  += xilinx/
diff --git a/drivers/firmware/xilinx/Kconfig b/drivers/firmware/xilinx/Kconfig
new file mode 100644
index 000..eb4cdcf
--- /dev/null
+++ b/drivers/firmware/xilinx/Kconfig
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Kconfig for Xilinx firmwares
+
+source "drivers/firmware/xilinx/zynqmp/Kconfig"
diff --git a/drivers/firmware/xilinx/Makefile b/drivers/firmware/xilinx/Makefile
new file mode 100644
index 000..beff5dc
--- /dev/null
+++ b/drivers/firmware/xilinx/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Makefile for Xilinx firmwares
+
+obj-$(CONFIG_ARCH_ZYNQMP) += zynqmp/
diff --git a/drivers/firmware/xilinx/zynqmp/Kconfig 
b/drivers/firmware/xilinx/zynqmp/Kconfig
new file mode 100644
index 000..8f7709d
--- /dev/null
+++ b/drivers/firmware/xilinx/zynqmp/Kconfig
@@ -0,0 +1,16 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Kconfig for Xilinx zynqmp firmware
+
+menu "Zynq MPSoC Firmware Drivers"
+   depends on ARCH_ZYNQMP
+
+config ZYNQMP_FIRMWARE
+   bool "Enable Xilinx Zynq MPSoC firmware interface"
+   help
+ Firmware interface driver is used by different to
+ communicate with the firmware for various platform
+ management services.
+ Say yes to enable zynqmp firmware interface driver.
+ In doubt, say N
+
+endmenu
diff --git a/drivers/firmware/xilinx/zynqmp/Makefile 
b/drivers/firmware/xilinx/zynqmp/Makefile
new file mode 100644
index 000..c3ec669
--- /dev/null
+++ b/drivers/firmware/xilinx/zynqmp/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0+
+# Makefile for Xilinx firmwares
+
+obj-$(CONFIG_ZYNQMP_FIRMWARE) += firmware.o
diff --git a/drivers/firmware/xilinx/zynqmp/firmware.c 
b/drivers/firmware/xilinx/zynqmp/firmware.c
new file mode 100644
index 000..2313ae7
--- /dev/null
+++ b/drivers/firmware/xilinx/zynqmp/firmware.c
@@ -0,0 +1,999 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Xilinx Zynq MPSoC Firmware layer
+ *
+ *  Copyright (C) 2014-2018 Xilinx, Inc.
+ *
+ *  Michal Simek 
+ *  Davorin Mista 
+ *  Jolly Shah 
+ *  Rajan Vaja 
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+/**
+ * zynqmp_pm_ret_code - Convert PMU-FW error codes to Linux error codes
+ * @ret_status:PMUFW return code
+ *
+ * Return: corresponding Linux error code
+ */
+int zynqmp_pm_ret_code(u32 ret_status)
+{
+   switch (ret_status) {
+   case XST_PM_SUCCESS:
+   case XST_PM_DOUBLE_REQ:
+   return 0;
+   case XST_PM_NO_ACCESS:
+   return -EACCES;
+   case