devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jolly Shah <JOLLYS-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
To: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Cc: "ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org"
	<ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>,
	"mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org"
	<mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
	"gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org"
	<gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>,
	"matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org"
	<matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org>,
	"sudeep.holla-5wv7dgnIgG8@public.gmane.org"
	<sudeep.holla-5wv7dgnIgG8@public.gmane.org>,
	"hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org"
	<hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
	"keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org"
	<keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>,
	"dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org"
	<dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
	"michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org"
	<michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>,
	"robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org"
	<robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>,
	"linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org"
	<linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org>,
	"linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	"devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>,
	Rajan Vaja <RAJANV-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
Subject: RE: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware driver
Date: Thu, 1 Feb 2018 01:23:48 +0000	[thread overview]
Message-ID: <CY1PR0201MB0764D88D409A35B8379323B4B8FA0@CY1PR0201MB0764.namprd02.prod.outlook.com> (raw)
In-Reply-To: <20180131182012.oshjmvahetaizlbu-agMKViyK24J5pKCnmE3YQBJ8xKzm50AiAL8bYrjMMd8@public.gmane.org>

Hi Mark,
Thanks for the review,

> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland-5wv7dgnIgG8@public.gmane.org]
> Sent: Wednesday, January 31, 2018 10:20 AM
> To: Jolly Shah <JOLLYS-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> Cc: ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org;
> gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org; matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org;
> sudeep.holla-5wv7dgnIgG8@public.gmane.org; hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org;
> dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org; robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org;
> linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Jolly Shah <JOLLYS-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>; Rajan Vaja
> <RAJANV-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> 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 overwritten at provided address with 48 bytes.

> 
> > +/**
> > + * 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. 


> 
> > +/**
> > + * 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. 
 
> > +/**
> > + * get_eemi_ops	- Get eemi ops functions
> > + *
> > + * Return:	- pointer of eemi_ops structure
> > + */
> > +const struct zynqmp_eemi_ops *get_eemi_ops(void) {
> > +	return &eemi_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(&pm_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 & 0xffff);
> > +	}
> > +
> > +	pr_info("%s Power management API v%d.%d\n", __func__,
> > +		ZYNQMP_PM_VERSION_MAJOR,
> ZYNQMP_PM_VERSION_MINOR);
> > +
> > +	of_node_put(np);
> > +
> > +	return ret;
> > +}
> > +
> > +early_initcall(zynqmp_plat_init);
> 
> Why does this need to be an early initcall? Can't we probe this as a platform
> device?
> 
> Thanks,
> Mark.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

  parent reply	other threads:[~2018-02-01  1:23 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-24 23:21 [PATCH v3 0/4] drivers: firmware: xilinx: Add firmware driver support Jolly Shah
     [not found] ` <1516836074-4149-1-git-send-email-jollys-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
2018-01-24 23:21   ` [PATCH v3 1/4] dt-bindings: firmware: Add bindings for ZynqMP firmware Jolly Shah
2018-01-30 17:08     ` Rob Herring
2018-01-31 18:03       ` Jolly Shah
2018-02-01 15:11         ` Rob Herring
2018-01-31 18:03     ` Mark Rutland
     [not found]       ` <20180131180354.mqf4gvvprdtycbn5-agMKViyK24J5pKCnmE3YQBJ8xKzm50AiAL8bYrjMMd8@public.gmane.org>
2018-02-01  1:04         ` Jolly Shah
     [not found]           ` <CY1PR0201MB076454E915A6E6947A39F04BB8FA0-w5HSbVcoX6AJQdFqRSmStRrHTHEw16jenBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
2018-02-01 10:27             ` Mark Rutland
2018-02-02 19:50               ` Jolly Shah
2018-01-24 23:21 ` [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware driver Jolly Shah
     [not found]   ` <1516836074-4149-3-git-send-email-jollys-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
2018-01-30  5:05     ` Shubhrajyoti Datta
2018-01-31 19:46       ` Jolly Shah
2018-01-31 18:20     ` Mark Rutland
     [not found]       ` <20180131182012.oshjmvahetaizlbu-agMKViyK24J5pKCnmE3YQBJ8xKzm50AiAL8bYrjMMd8@public.gmane.org>
2018-02-01  1:23         ` Jolly Shah [this message]
     [not found]           ` <CY1PR0201MB0764D88D409A35B8379323B4B8FA0-w5HSbVcoX6AJQdFqRSmStRrHTHEw16jenBOFsp37pqbUKgpGm//BTAC/G2K4zDHf@public.gmane.org>
2018-02-01 10:33             ` Mark Rutland
     [not found]               ` <20180201103321.fftn37mgzbk6oltl-agMKViyK24J5pKCnmE3YQBJ8xKzm50AiAL8bYrjMMd8@public.gmane.org>
2018-02-01 23:54                 ` Jolly Shah
2018-01-24 23:21 ` [PATCH v3 3/4] drivers: firmware: xilinx: Add sysfs interface Jolly Shah
2018-01-24 23:21 ` [PATCH v3 4/4] drivers: firmware: xilinx: Add debugfs interface Jolly Shah
2018-01-25  9:30   ` Greg KH
     [not found]     ` <20180125093057.GA1936-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
2018-01-31 19:48       ` Jolly Shah

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CY1PR0201MB0764D88D409A35B8379323B4B8FA0@CY1PR0201MB0764.namprd02.prod.outlook.com \
    --to=jollys-gjffaj9ahvfqt0dzr+alfa@public.gmane.org \
    --cc=RAJANV-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org \
    --cc=ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org \
    --cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
    --cc=gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org \
    --cc=hkallweit1-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \
    --cc=keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org \
    --cc=linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org \
    --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \
    --cc=mark.rutland-5wv7dgnIgG8@public.gmane.org \
    --cc=matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org \
    --cc=michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org \
    --cc=mingo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
    --cc=robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \
    --cc=sudeep.holla-5wv7dgnIgG8@public.gmane.org \
    --subject='RE: [PATCH v3 2/4] drivers: firmware: xilinx: Add ZynqMP firmware driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).