[PATCH v1 1/2] imx-rproc: dt: provide new remote-nodes option

[PATCH v1 1/2] imx-rproc: dt: provide new remote-nodes option

[Oleksij Rempel]

On AMP systems we need to make sure that some device
nodes are not used by main system and reserved for
external system. Some of configuration should be
maintained by main system. For example clocks and pins.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 .../devicetree/bindings/remoteproc/imx-rproc.txt    | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt b/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
index fbcefd965dc4..40bec03e094c 100644
--- a/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
+++ b/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
@@ -15,6 +15,7 @@ Required properties:
 Optional properties:
 - memory-region		list of phandels to the reserved memory regions.
 			(See: ../reserved-memory/reserved-memory.txt)
+- remote-nodes		list of device node phandels used by remote system.
 
 Example:
 	m4_reserved_sysmem1: cm4@80000000 {
@@ -25,9 +26,21 @@ Example:
 		reg = <0x81000000 0x80000>;
 	};
 
+	/* node reserved for rproc */
+	&uart1 {
+		assigned-clock-rates = <240000000>;
+		status = "disabled";
+	};
+
+	&gpt2 {
+		assigned-clock-rates = <24000000>;
+		status = "disabled";
+	};
+
 	imx7d-cm4 {
 		compatible	= "fsl,imx7d-cm4";
 		memory-region	= <&m4_reserved_sysmem1>, <&m4_reserved_sysmem2>;
 		syscon		= <&src>;
 		clocks		= <&clks IMX7D_ARM_M4_ROOT_CLK>;
+		remote-nodes	= <&gpt2>, <&uart1>;
 	};
-- 
2.17.1

[PATCH v1 2/2] remoteproc: imx_rproc: assign other DT nodes to rproc node

[Oleksij Rempel]

At least on i.MX* most of devices are available to all CPUs.
In this case the master system should know early enough how system
should be split and preconfigured.

For example if we start Linux on other part of the system, we will need
properly configured clocks for some predefined rates. Pin, reset and
power state should be controlled by master system as well.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/remoteproc/imx_rproc.c | 160 +++++++++++++++++++++++++++++++++
 1 file changed, 160 insertions(+)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index 45acc2ce930d..1999e50f5d4c 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -47,6 +47,8 @@
 					 | IMX6SX_SW_M4C_RST)
 
 #define IMX7D_RPROC_MEM_MAX		8
+#define IMX7D_RPROC_RNODES_MAX		8
+#define IMX7D_RPROC_RNODE_CLKS_MAX	8
 
 /**
  * struct imx_rproc_mem - slim internal memory structure
@@ -81,6 +83,12 @@ struct imx_rproc_dcfg {
 	size_t				att_size;
 };
 
+struct imx_rproc_rnode {
+	struct device_node		*node;
+	struct clk			*clk[IMX7D_RPROC_RNODE_CLKS_MAX];
+	unsigned int			clks;
+};
+
 struct imx_rproc {
 	struct device			*dev;
 	struct regmap			*regmap;
@@ -88,6 +96,8 @@ struct imx_rproc {
 	const struct imx_rproc_dcfg	*dcfg;
 	struct imx_rproc_mem		mem[IMX7D_RPROC_MEM_MAX];
 	struct clk			*clk;
+	struct imx_rproc_rnode		rnode[IMX7D_RPROC_RNODES_MAX];
+	unsigned int			rnodes;
 };
 
 static const struct imx_rproc_att imx_rproc_att_imx7d[] = {
@@ -250,6 +260,142 @@ static const struct rproc_ops imx_rproc_ops = {
 	.da_to_va       = imx_rproc_da_to_va,
 };
 
+static int imx_rproc_set_clk_rates(struct device_node *node, bool clk_supplier)
+{
+	struct of_phandle_args clkspec;
+	struct property	*prop;
+	const __be32 *cur;
+	int rc, index = 0;
+	struct clk *clk;
+	u32 rate;
+
+	of_property_for_each_u32(node, "assigned-clock-rates", prop, cur, rate) {
+		if (rate) {
+			rc = of_parse_phandle_with_args(node, "clocks",
+					"#clock-cells",	index, &clkspec);
+			if (rc < 0) {
+				/* skip empty (null) phandles */
+				if (rc == -ENOENT)
+					continue;
+				else
+					return rc;
+			}
+			if (clkspec.np == node && !clk_supplier)
+				return 0;
+
+			clk = of_clk_get_from_provider(&clkspec);
+			if (IS_ERR(clk)) {
+				if (PTR_ERR(clk) != -EPROBE_DEFER)
+					pr_warn("clk: couldn't get clock %d for %s\n",
+						index, node->full_name);
+				return PTR_ERR(clk);
+			}
+
+			rc = clk_set_rate(clk, rate);
+			if (rc < 0)
+				pr_err("clk: couldn't set  clk rate to %u (%d), current rate: %lu\n",
+					rate, rc,
+				       clk_get_rate(clk));
+			clk_put(clk);
+		}
+		index++;
+	}
+	return 0;
+}
+
+static int imx_rproc_set_rclks(struct imx_rproc *priv, struct imx_rproc_rnode *rn)
+{
+	struct device_node *node = rn->node;
+	struct device *dev = priv->dev;
+	struct of_phandle_args clkspec;
+	int index, rc, ret, num_parents;
+	struct clk *clk;
+
+	imx_rproc_set_clk_rates(node, 0);
+	num_parents = of_count_phandle_with_args(node, "clocks", "#clock-cells");
+	if (num_parents == -EINVAL) {
+		dev_err(dev, "clk: invalid value of clocks property at %s\n",
+		       node->full_name);
+		return -EINVAL;
+	}
+
+	if (IMX7D_RPROC_RNODE_CLKS_MAX < num_parents) {
+		dev_err(dev, "unsupported count of remote clocks: %i, max: %i\n",
+			 num_parents, IMX7D_RPROC_RNODE_CLKS_MAX);
+		return -EINVAL;
+	}
+
+	for (index = 0; index < num_parents; index++) {
+		rc = of_parse_phandle_with_args(node, "clocks",
+					"#clock-cells", index, &clkspec);
+		if (rc < 0)
+			goto err;
+
+		if (clkspec.np == node) {
+			rc = 0;
+			goto err;
+		}
+
+		clk = of_clk_get_from_provider(&clkspec);
+		if (IS_ERR(clk)) {
+			if (PTR_ERR(clk) != -EPROBE_DEFER)
+				dev_warn(dev, "clk: couldn't get assigned clock %d for %s\n",
+					index, node->full_name);
+			rc = PTR_ERR(clk);
+			goto err;
+		}
+
+		rn->clk[index] = clk;
+		rn->clks++;
+		/*
+		 * clk for M4 block including memory. Should be
+		 * enabled before .start for FW transfer.
+		 */
+		ret = clk_prepare_enable(clk);
+		if (ret) {
+			dev_err(dev, "Failed to enable clock\n");
+			return ret;
+		}
+	}
+	return 0;
+err:
+	return rc;
+}
+
+static int imx_rproc_rnodes_init(struct imx_rproc *priv)
+{
+	struct device *dev = priv->dev;
+	struct device_node *node = dev->of_node;
+	int a, err, num_parents;
+
+	num_parents = of_count_phandle_with_args(node, "remote-nodes", NULL);
+	if (num_parents == -EINVAL) {
+		dev_err(dev, "rnote: invalid value of remote-node property at %s\n",
+		       node->full_name);
+		return -EINVAL;
+	}
+
+	if (IMX7D_RPROC_RNODES_MAX < num_parents) {
+		dev_err(dev, "unsupported count of remote node: %i, max: %i\n",
+			 num_parents, IMX7D_RPROC_RNODES_MAX);
+		return -EINVAL;
+	}
+
+	/* remap optional addresses */
+	for (a = 0; a < num_parents; a++) {
+		struct device_node *rn;
+
+		rn = of_parse_phandle(node, "remote-nodes", a);
+		priv->rnode[a].node = rn;
+		priv->rnodes++;
+		err = imx_rproc_set_rclks(priv, &priv->rnode[a]);
+		if (err)
+			break;
+	}
+
+	return err;
+}
+
 static int imx_rproc_addr_init(struct imx_rproc *priv,
 			       struct platform_device *pdev)
 {
@@ -374,6 +520,12 @@ static int imx_rproc_probe(struct platform_device *pdev)
 		goto err_put_rproc;
 	}
 
+	ret = imx_rproc_rnodes_init(priv);
+	if (ret) {
+		dev_err(dev, "filed on imx_rproc_rnodes_init\n");
+		goto err_put_rproc;
+	}
+
 	ret = rproc_add(rproc);
 	if (ret) {
 		dev_err(dev, "rproc_add failed\n");
@@ -394,6 +546,14 @@ static int imx_rproc_remove(struct platform_device *pdev)
 {
 	struct rproc *rproc = platform_get_drvdata(pdev);
 	struct imx_rproc *priv = rproc->priv;
+	int a, b;
+
+	for (a = 0; a < priv->rnodes; a++) {
+		struct imx_rproc_rnode *rn = &priv->rnode[a];
+
+		for (b = 0; b < rn->clks; b++)
+			clk_disable_unprepare(rn->clk[b]);
+	}
 
 	clk_disable_unprepare(priv->clk);
 	rproc_del(rproc);
-- 
2.17.1

Re: [PATCH v1 1/2] imx-rproc: dt: provide new remote-nodes option

[Arnaud Pouliquen]

Hi Oleksij,

Nice to see that we have the same needs.
We push several month ago an RFC based on something similar but i hope
more generic...
could you have a look?

https://www.spinics.net/lists/linux-remoteproc/msg01823.html

Could be nice if we could find a generic solution...

Best Regards
Arnaud

On 06/15/2018 01:57 PM, Oleksij Rempel wrote:
> On AMP systems we need to make sure that some device
> nodes are not used by main system and reserved for
> external system. Some of configuration should be
> maintained by main system. For example clocks and pins.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  .../devicetree/bindings/remoteproc/imx-rproc.txt    | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt b/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
> index fbcefd965dc4..40bec03e094c 100644
> --- a/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
> @@ -15,6 +15,7 @@ Required properties:
>  Optional properties:
>  - memory-region		list of phandels to the reserved memory regions.
>  			(See: ../reserved-memory/reserved-memory.txt)
> +- remote-nodes		list of device node phandels used by remote system.
>  
>  Example:
>  	m4_reserved_sysmem1: cm4@80000000 {
> @@ -25,9 +26,21 @@ Example:
>  		reg = <0x81000000 0x80000>;
>  	};
>  
> +	/* node reserved for rproc */
> +	&uart1 {
> +		assigned-clock-rates = <240000000>;
> +		status = "disabled";
> +	};
> +
> +	&gpt2 {
> +		assigned-clock-rates = <24000000>;
> +		status = "disabled";
> +	};
> +
>  	imx7d-cm4 {
>  		compatible	= "fsl,imx7d-cm4";
>  		memory-region	= <&m4_reserved_sysmem1>, <&m4_reserved_sysmem2>;
>  		syscon		= <&src>;
>  		clocks		= <&clks IMX7D_ARM_M4_ROOT_CLK>;
> +		remote-nodes	= <&gpt2>, <&uart1>;
>  	};
> 

Re: [PATCH v1 1/2] imx-rproc: dt: provide new remote-nodes option

[Oleksij Rempel]

[-- Attachment #1: Type: text/plain, Size: 2957 bytes --]

Hi Arnaud,

On Fri, Jun 15, 2018 at 03:21:19PM +0200, Arnaud Pouliquen wrote:
> Hi Oleksij,
> 
> Nice to see that we have the same needs.
> We push several month ago an RFC based on something similar but i hope
> more generic...
> could you have a look?
> 
> https://www.spinics.net/lists/linux-remoteproc/msg01823.html

I took a look at dt binding.
It would be really better to not redefine device nodes again.
DT is providing HW description and if it is still the same IP core
then most probably it is still the same from all CPUs. Most probably
there is different interrupt controller and memory offset, but all other
parts should be the same.
In long term it would be great to reduce duplicated information which is
needed to added system developer.

> Could be nice if we could find a generic solution...

I would be happy to have generic solution. 

> Best Regards
> Arnaud
> 
> On 06/15/2018 01:57 PM, Oleksij Rempel wrote:
> > On AMP systems we need to make sure that some device
> > nodes are not used by main system and reserved for
> > external system. Some of configuration should be
> > maintained by main system. For example clocks and pins.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  .../devicetree/bindings/remoteproc/imx-rproc.txt    | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt b/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
> > index fbcefd965dc4..40bec03e094c 100644
> > --- a/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
> > +++ b/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
> > @@ -15,6 +15,7 @@ Required properties:
> >  Optional properties:
> >  - memory-region		list of phandels to the reserved memory regions.
> >  			(See: ../reserved-memory/reserved-memory.txt)
> > +- remote-nodes		list of device node phandels used by remote system.
> >  
> >  Example:
> >  	m4_reserved_sysmem1: cm4@80000000 {
> > @@ -25,9 +26,21 @@ Example:
> >  		reg = <0x81000000 0x80000>;
> >  	};
> >  
> > +	/* node reserved for rproc */
> > +	&uart1 {
> > +		assigned-clock-rates = <240000000>;
> > +		status = "disabled";
> > +	};
> > +
> > +	&gpt2 {
> > +		assigned-clock-rates = <24000000>;
> > +		status = "disabled";
> > +	};
> > +
> >  	imx7d-cm4 {
> >  		compatible	= "fsl,imx7d-cm4";
> >  		memory-region	= <&m4_reserved_sysmem1>, <&m4_reserved_sysmem2>;
> >  		syscon		= <&src>;
> >  		clocks		= <&clks IMX7D_ARM_M4_ROOT_CLK>;
> > +		remote-nodes	= <&gpt2>, <&uart1>;
> >  	};
> > 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/2] imx-rproc: dt: provide new remote-nodes option

[Arnaud Pouliquen]

Hi Oleksij,

On 06/15/2018 06:37 PM, Oleksij Rempel wrote:
> Hi Arnaud,
> 
> On Fri, Jun 15, 2018 at 03:21:19PM +0200, Arnaud Pouliquen wrote:
>> Hi Oleksij,
>>
>> Nice to see that we have the same needs.
>> We push several month ago an RFC based on something similar but i hope
>> more generic...
>> could you have a look?
>>
>> https://www.spinics.net/lists/linux-remoteproc/msg01823.html
> 
> I took a look at dt binding.
> It would be really better to not redefine device nodes again.
> DT is providing HW description and if it is still the same IP core
> then most probably it is still the same from all CPUs. Most probably
> there is different interrupt controller and memory offset, but all other
> parts should be the same.
> In long term it would be great to reduce duplicated information which is
> needed to added system developer.
This is a valid point. We are also thinking about this. But just
disabling a peripheral that is used seems also not logic from our point
of view.

Furthermore how to you manage followings use cases:
- peripheral clock is not the same for master and remote processor
=> you potentially need to redefine the clocks
- clock, regulator or pin are not managed by the Linux if peripheral is
assigned to the remote processor, but controlled by the remote processor
directly (isolation, protection on shared resource access...).
=> In this case we must not handle the resource in Linux.
- Need a specific management of a peripheral, due to secure, isolation,
or any other reason related to the platform.
=> specific driver that can be bind as srm child (platform srm_dev).

To sum-up. We name shared (or system) resources every resources that
have to be shared between the master and the remote processor. The list
of these resources depends on the platform (and on peripheral of a
platform). That's why we decide to redefine the node.

In fact the good solution could be in the middle of this both design
solutions. Means choice between redefining the node properties or just
provide an handle to the soc one. For this we are thinking about a
phandle to the soc node.
something like ("parent-device" naming is just for the example)
	m4_uart1 {
		assigned-clock-rates = <240000000>;
		parent-device = { &uart1};
	};

Another advantage of a phandle would be to be able to check that the
device is disabled on Linux side and could offer the possibility to
switch the peripheral between master and slave during the runtime.

To finish, an additional information: We are implementing on top of SRM
a dynamic part based on rpmsg that allows to reconfigure the shared
resource to allows for instance to:
- change the clock rate
- change pin states
- change regulator constraints
According to first discussion with Bjorn, we need to share this part
also to present the global picture ,we would like to propose.

> 
>> Could be nice if we could find a generic solution...
> 
> I would be happy to have generic solution. 

Our solution is a base for discussion. If several companies are
interested in, any feedback and contributions to have a generic solution
is welcome.
And of course we need the approval of the maintainers on the design.

> 
>> Best Regards
>> Arnaud
>>
>> On 06/15/2018 01:57 PM, Oleksij Rempel wrote:
>>> On AMP systems we need to make sure that some device
>>> nodes are not used by main system and reserved for
>>> external system. Some of configuration should be
>>> maintained by main system. For example clocks and pins.
>>>
>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>>> ---
>>>  .../devicetree/bindings/remoteproc/imx-rproc.txt    | 13 +++++++++++++
>>>  1 file changed, 13 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt b/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
>>> index fbcefd965dc4..40bec03e094c 100644
>>> --- a/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
>>> +++ b/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
>>> @@ -15,6 +15,7 @@ Required properties:
>>>  Optional properties:
>>>  - memory-region		list of phandels to the reserved memory regions.
>>>  			(See: ../reserved-memory/reserved-memory.txt)
>>> +- remote-nodes		list of device node phandels used by remote system.
>>>  
>>>  Example:
>>>  	m4_reserved_sysmem1: cm4@80000000 {
>>> @@ -25,9 +26,21 @@ Example:
>>>  		reg = <0x81000000 0x80000>;
>>>  	};
>>>  
>>> +	/* node reserved for rproc */
>>> +	&uart1 {
>>> +		assigned-clock-rates = <240000000>;
>>> +		status = "disabled";
>>> +	};
>>> +
>>> +	&gpt2 {
>>> +		assigned-clock-rates = <24000000>;
>>> +		status = "disabled";
>>> +	};
>>> +
>>>  	imx7d-cm4 {
>>>  		compatible	= "fsl,imx7d-cm4";
>>>  		memory-region	= <&m4_reserved_sysmem1>, <&m4_reserved_sysmem2>;
>>>  		syscon		= <&src>;
>>>  		clocks		= <&clks IMX7D_ARM_M4_ROOT_CLK>;
>>> +		remote-nodes	= <&gpt2>, <&uart1>;
>>>  	};
>>>
>>
> 

Re: [PATCH v1 1/2] imx-rproc: dt: provide new remote-nodes option

[Oleksij Rempel]

[-- Attachment #1.1: Type: text/plain, Size: 6106 bytes --]

Hi Arnaud,

On 18.06.2018 11:32, Arnaud Pouliquen wrote:
> Hi Oleksij,
> 
> On 06/15/2018 06:37 PM, Oleksij Rempel wrote:
>> Hi Arnaud,
>>
>> On Fri, Jun 15, 2018 at 03:21:19PM +0200, Arnaud Pouliquen wrote:
>>> Hi Oleksij,
>>>
>>> Nice to see that we have the same needs.
>>> We push several month ago an RFC based on something similar but i hope
>>> more generic...
>>> could you have a look?
>>>
>>> https://www.spinics.net/lists/linux-remoteproc/msg01823.html
>>
>> I took a look at dt binding.
>> It would be really better to not redefine device nodes again.
>> DT is providing HW description and if it is still the same IP core
>> then most probably it is still the same from all CPUs. Most probably
>> there is different interrupt controller and memory offset, but all other
>> parts should be the same.
>> In long term it would be great to reduce duplicated information which is
>> needed to added system developer.
> This is a valid point. We are also thinking about this. But just
> disabling a peripheral that is used seems also not logic from our point
> of view.

Right now I don't see any thing bad on disabling it. For master CPU it
is indeed disabled and should not be accesable. What is your
argumentation about this?

> Furthermore how to you manage followings use cases:
> - peripheral clock is not the same for master and remote processor
> => you potentially need to redefine the clocks

IMO, not devicetree specific discussion

> - clock, regulator or pin are not managed by the Linux if peripheral is
> assigned to the remote processor, but controlled by the remote processor
> directly (isolation, protection on shared resource access...).
> => In this case we must not handle the resource in Linux.

IMO, not devicetree specific discussion

> - Need a specific management of a peripheral, due to secure, isolation,
> or any other reason related to the platform.
> => specific driver that can be bind as srm child (platform srm_dev).

IMO, not devicetree specific discussion

> To sum-up. We name shared (or system) resources every resources that
> have to be shared between the master and the remote processor. The list
> of these resources depends on the platform (and on peripheral of a
> platform). That's why we decide to redefine the node.

sorry, i can't follow here. I don't claim to replace your solution,
especially if you have working code I would be happy to take it over ASAP.

> In fact the good solution could be in the middle of this both design
> solutions. Means choice between redefining the node properties or just
> provide an handle to the soc one. For this we are thinking about a
> phandle to the soc node.
> something like ("parent-device" naming is just for the example)
> 	m4_uart1 {
> 		assigned-clock-rates = <240000000>;
> 		parent-device = { &uart1};
> 	};
> 
> Another advantage of a phandle would be to be able to check that the
> device is disabled on Linux side and could offer the possibility to
> switch the peripheral between master and slave during the runtime.

Is it a workaround for a system without devicetree overlay?
I don't see why we should provide extra container for not really extra
information. Or do I miss something?

> To finish, an additional information: We are implementing on top of SRM
> a dynamic part based on rpmsg that allows to reconfigure the shared
> resource to allows for instance to:
> - change the clock rate
> - change pin states
> - change regulator constraints
> According to first discussion with Bjorn, we need to share this part
> also to present the global picture ,we would like to propose.

Ok, so it looks like we are working on same thing. And you probably
already have most of the code to make this work. Did you made Linux
implementation only for Master or both (Master and Slave) parts?

>>
>>> Could be nice if we could find a generic solution...
>>
>> I would be happy to have generic solution. 
> 
> Our solution is a base for discussion. If several companies are
> interested in, any feedback and contributions to have a generic solution
> is welcome.
> And of course we need the approval of the maintainers on the design.

i'm sure, every thing will be OK ;)

>>
>>> Best Regards
>>> Arnaud
>>>
>>> On 06/15/2018 01:57 PM, Oleksij Rempel wrote:
>>>> On AMP systems we need to make sure that some device
>>>> nodes are not used by main system and reserved for
>>>> external system. Some of configuration should be
>>>> maintained by main system. For example clocks and pins.
>>>>
>>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>>>> ---
>>>>  .../devicetree/bindings/remoteproc/imx-rproc.txt    | 13 +++++++++++++
>>>>  1 file changed, 13 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt b/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
>>>> index fbcefd965dc4..40bec03e094c 100644
>>>> --- a/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
>>>> +++ b/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
>>>> @@ -15,6 +15,7 @@ Required properties:
>>>>  Optional properties:
>>>>  - memory-region		list of phandels to the reserved memory regions.
>>>>  			(See: ../reserved-memory/reserved-memory.txt)
>>>> +- remote-nodes		list of device node phandels used by remote system.
>>>>  
>>>>  Example:
>>>>  	m4_reserved_sysmem1: cm4@80000000 {
>>>> @@ -25,9 +26,21 @@ Example:
>>>>  		reg = <0x81000000 0x80000>;
>>>>  	};
>>>>  
>>>> +	/* node reserved for rproc */
>>>> +	&uart1 {
>>>> +		assigned-clock-rates = <240000000>;
>>>> +		status = "disabled";
>>>> +	};
>>>> +
>>>> +	&gpt2 {
>>>> +		assigned-clock-rates = <24000000>;
>>>> +		status = "disabled";
>>>> +	};
>>>> +
>>>>  	imx7d-cm4 {
>>>>  		compatible	= "fsl,imx7d-cm4";
>>>>  		memory-region	= <&m4_reserved_sysmem1>, <&m4_reserved_sysmem2>;
>>>>  		syscon		= <&src>;
>>>>  		clocks		= <&clks IMX7D_ARM_M4_ROOT_CLK>;
>>>> +		remote-nodes	= <&gpt2>, <&uart1>;
>>>>  	};
>>>>
>>>
>>
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/2] imx-rproc: dt: provide new remote-nodes option

[Arnaud Pouliquen]

On 06/18/2018 02:37 PM, Oleksij Rempel wrote:
> Hi Arnaud,
> 
> On 18.06.2018 11:32, Arnaud Pouliquen wrote:
>> Hi Oleksij,
>>
>> On 06/15/2018 06:37 PM, Oleksij Rempel wrote:
>>> Hi Arnaud,
>>>
>>> On Fri, Jun 15, 2018 at 03:21:19PM +0200, Arnaud Pouliquen wrote:
>>>> Hi Oleksij,
>>>>
>>>> Nice to see that we have the same needs.
>>>> We push several month ago an RFC based on something similar but i hope
>>>> more generic...
>>>> could you have a look?
>>>>
>>>> https://www.spinics.net/lists/linux-remoteproc/msg01823.html
>>>
>>> I took a look at dt binding.
>>> It would be really better to not redefine device nodes again.
>>> DT is providing HW description and if it is still the same IP core
>>> then most probably it is still the same from all CPUs. Most probably
>>> there is different interrupt controller and memory offset, but all other
>>> parts should be the same.
>>> In long term it would be great to reduce duplicated information which is
>>> needed to added system developer.
>> This is a valid point. We are also thinking about this. But just
>> disabling a peripheral that is used seems also not logic from our point
>> of view.
> 
> Right now I don't see any thing bad on disabling it. For master CPU it
> is indeed disabled and should not be accesable. What is your
> argumentation about this?
Perhaps It's more of a philosophical point of view than anything else...
But doing some action based on a disabled device looks from my POV a
nonsense. It look like a hack to disable the device instead of changing
the compatible (change compatible to rproc-srm-dev could be used for
this...).

For instance we can consider the pinctrl management. (Please tell me if
i'm wrong) if you disable your device the pinctrl will not configure
your pins. So you will need to force the pins management for a disabled
device...

> 
>> Furthermore how to you manage followings use cases:
>> - peripheral clock is not the same for master and remote processor
>> => you potentially need to redefine the clocks
> 
> IMO, not devicetree specific discussion
Could you clarify what you means by " not devicetree specific
discussion"? do you mean that you can directly update the device using
overlay?
> 
>> - clock, regulator or pin are not managed by the Linux if peripheral is
>> assigned to the remote processor, but controlled by the remote processor
>> directly (isolation, protection on shared resource access...).
>> => In this case we must not handle the resource in Linux.
> 
> IMO, not devicetree specific discussion
> 
>> - Need a specific management of a peripheral, due to secure, isolation,
>> or any other reason related to the platform.
>> => specific driver that can be bind as srm child (platform srm_dev).
> 
> IMO, not devicetree specific discussion
> 
>> To sum-up. We name shared (or system) resources every resources that
>> have to be shared between the master and the remote processor. The list
>> of these resources depends on the platform (and on peripheral of a
>> platform). That's why we decide to redefine the node.
> 
> sorry, i can't follow here. I don't claim to replace your solution,
> especially if you have working code I would be happy to take it over ASAP.

And i did not understand this:) just try to explain our design.
On our platform we need to configure at least clock regulator and GPIO,
to ensure coherence but also avoid concurrent access to some shared
registers.
Seems that on your platform you only need to handle the clock.

FYI you can find here some slides presented in OPENAMP weekly meeting
and shared with Bjorn. Document has just to be considered as a work base
 associated with the RFC. No decision done today...

http://openamp.github.io/docs/mca/remoteproc-resource-manager-overview.pdf

> 
>> In fact the good solution could be in the middle of this both design
>> solutions. Means choice between redefining the node properties or just
>> provide an handle to the soc one. For this we are thinking about a
>> phandle to the soc node.
>> something like ("parent-device" naming is just for the example)
>> 	m4_uart1 {
>> 		assigned-clock-rates = <240000000>;
>> 		parent-device = { &uart1};
>> 	};
>>
>> Another advantage of a phandle would be to be able to check that the
>> device is disabled on Linux side and could offer the possibility to
>> switch the peripheral between master and slave during the runtime.
> 
> Is it a workaround for a system without devicetree overlay?
Here i'm speaking about possibility to dynamically switch a peripheral
during the run time (for instance for low power purpose). If do not
consider the config fs overlay but only uboot overlay, the resource
manager could be also used to switch the peripheral responsibility from
one core to the other.

> I don't see why we should provide extra container for not really extra
> information. Or do I miss something?
First of all seems i missed your "remote-nodes" property...no need to
use the "parent-device".
 	
This is an open point, do we need extra information or not?

phandle should work for a simple device without child. Now if you have
childs, this can became tricky to parse the node in a generic way...

And the 3 points above need also to be considered, at least until you
clarify your POV.

Then we would like also to consider possibility to associate a specific
driver to handle a remote device. As example we have a platform on which
the remote processor is started before the Linux. It handles an I2C
waiting Linux boot and then free it for Linux usage. So we implemented a
specific driver that enable the soc device to probe the associated linux
driver.
That's why we offer possibility to define platform rproc_srm_dev...

Now everything is open and i also agree with you that it could be nice
to be able to provide also a phandle in simple usecase instead of
redefining the properties.
Then base on your example changing the compatible to the srm_dev instead
of disabling the device would be cleaner.

> 
>> To finish, an additional information: We are implementing on top of SRM
>> a dynamic part based on rpmsg that allows to reconfigure the shared
>> resource to allows for instance to:
>> - change the clock rate
>> - change pin states
>> - change regulator constraints
>> According to first discussion with Bjorn, we need to share this part
>> also to present the global picture ,we would like to propose.
> 
> Ok, so it looks like we are working on same thing. And you probably
> already have most of the code to make this work. Did you made Linux
> implementation only for Master or both (Master and Slave) parts?
we have something almost ready for the dynamic part on Linux (acting as
resource manager server). For Linux slave a new framework should answer:
the SCMI.

Now i don't know how you will want to go further on this topic, if you
are interested in...
Are you are interested in proposing some adaptation on top of the srm
patchset, to add phandle?
On our side we should be able to send a V2 including the dynamic part.
This could be a good entry point for maintainer feedback...

> 
>>>
>>>> Could be nice if we could find a generic solution...
>>>
>>> I would be happy to have generic solution. 
>>
>> Our solution is a base for discussion. If several companies are
>> interested in, any feedback and contributions to have a generic solution
>> is welcome.
>> And of course we need the approval of the maintainers on the design.
> 
> i'm sure, every thing will be OK ;)
At least the need is here and seems shared by several companies.This is
a good first step. :)
Then the way to properly implement it remains to be refined, especially
the DT part.

Best Regards
Arnaud

> 
>>>
>>>> Best Regards
>>>> Arnaud
>>>>
>>>> On 06/15/2018 01:57 PM, Oleksij Rempel wrote:
>>>>> On AMP systems we need to make sure that some device
>>>>> nodes are not used by main system and reserved for
>>>>> external system. Some of configuration should be
>>>>> maintained by main system. For example clocks and pins.
>>>>>
>>>>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>>>>> ---
>>>>>  .../devicetree/bindings/remoteproc/imx-rproc.txt    | 13 +++++++++++++
>>>>>  1 file changed, 13 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt b/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
>>>>> index fbcefd965dc4..40bec03e094c 100644
>>>>> --- a/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
>>>>> +++ b/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
>>>>> @@ -15,6 +15,7 @@ Required properties:
>>>>>  Optional properties:
>>>>>  - memory-region		list of phandels to the reserved memory regions.
>>>>>  			(See: ../reserved-memory/reserved-memory.txt)
>>>>> +- remote-nodes		list of device node phandels used by remote system.
>>>>>  
>>>>>  Example:
>>>>>  	m4_reserved_sysmem1: cm4@80000000 {
>>>>> @@ -25,9 +26,21 @@ Example:
>>>>>  		reg = <0x81000000 0x80000>;
>>>>>  	};
>>>>>  
>>>>> +	/* node reserved for rproc */
>>>>> +	&uart1 {
>>>>> +		assigned-clock-rates = <240000000>;
>>>>> +		status = "disabled";
>>>>> +	};
>>>>> +
>>>>> +	&gpt2 {
>>>>> +		assigned-clock-rates = <24000000>;
>>>>> +		status = "disabled";
>>>>> +	};
>>>>> +
>>>>>  	imx7d-cm4 {
>>>>>  		compatible	= "fsl,imx7d-cm4";
>>>>>  		memory-region	= <&m4_reserved_sysmem1>, <&m4_reserved_sysmem2>;
>>>>>  		syscon		= <&src>;
>>>>>  		clocks		= <&clks IMX7D_ARM_M4_ROOT_CLK>;
>>>>> +		remote-nodes	= <&gpt2>, <&uart1>;
>>>>>  	};
>>>>>
>>>>
>>>
>>
> 

Re: [PATCH v1 1/2] imx-rproc: dt: provide new remote-nodes option

[Oleksij Rempel]

[-- Attachment #1: Type: text/plain, Size: 10662 bytes --]

On Tue, Jun 19, 2018 at 03:58:53PM +0200, Arnaud Pouliquen wrote:
> 
> 
> On 06/18/2018 02:37 PM, Oleksij Rempel wrote:
> > Hi Arnaud,
> > 
> > On 18.06.2018 11:32, Arnaud Pouliquen wrote:
> >> Hi Oleksij,
> >>
> >> On 06/15/2018 06:37 PM, Oleksij Rempel wrote:
> >>> Hi Arnaud,
> >>>
> >>> On Fri, Jun 15, 2018 at 03:21:19PM +0200, Arnaud Pouliquen wrote:
> >>>> Hi Oleksij,
> >>>>
> >>>> Nice to see that we have the same needs.
> >>>> We push several month ago an RFC based on something similar but i hope
> >>>> more generic...
> >>>> could you have a look?
> >>>>
> >>>> https://www.spinics.net/lists/linux-remoteproc/msg01823.html
> >>>
> >>> I took a look at dt binding.
> >>> It would be really better to not redefine device nodes again.
> >>> DT is providing HW description and if it is still the same IP core
> >>> then most probably it is still the same from all CPUs. Most probably
> >>> there is different interrupt controller and memory offset, but all other
> >>> parts should be the same.
> >>> In long term it would be great to reduce duplicated information which is
> >>> needed to added system developer.
> >> This is a valid point. We are also thinking about this. But just
> >> disabling a peripheral that is used seems also not logic from our point
> >> of view.
> > 
> > Right now I don't see any thing bad on disabling it. For master CPU it
> > is indeed disabled and should not be accesable. What is your
> > argumentation about this?
> Perhaps It's more of a philosophical point of view than anything else...
> But doing some action based on a disabled device looks from my POV a
> nonsense. It look like a hack to disable the device instead of changing
> the compatible (change compatible to rproc-srm-dev could be used for
> this...).

Having one HW specific bindings vs application specific has at least one
important advantage:
- the hw will not change, but application will do.
So it is easier to approve/mainline one HW binding and
let SW decide how exactly it should be used.
About status usage, I'm not 100% sure.
I have seen products where different IP blocks was connected to the same
pins and enabled/routed to this pins sequentially within system start.
I have no idea how to reflect this within DT.
In case of remote proc, we change <cpu>-<ip-block>-<pins> relation.
I already can image customers who will require to control some thing
by master system until slave system will be started.
In this case, manually rewritten DT, which is reflecting some time point of system
configuration is not really productive.

> For instance we can consider the pinctrl management. (Please tell me if
> i'm wrong) if you disable your device the pinctrl will not configure
> your pins. So you will need to force the pins management for a disabled
> device...

yes, good point. I need to think about this.

> > 
> >> Furthermore how to you manage followings use cases:
> >> - peripheral clock is not the same for master and remote processor
> >> => you potentially need to redefine the clocks
> > 
> > IMO, not devicetree specific discussion
> Could you clarify what you means by " not devicetree specific
> discussion"? do you mean that you can directly update the device using
> overlay?

If some reusable ip block is controlled by some clock controller. It
will depend on this controller even after the ip block was exported to
separate CPU. On other hand the same ip block will depend on different
interrupt controller on other CPU.

> > 
> >> - clock, regulator or pin are not managed by the Linux if peripheral is
> >> assigned to the remote processor, but controlled by the remote processor
> >> directly (isolation, protection on shared resource access...).
> >> => In this case we must not handle the resource in Linux.
> > 
> > IMO, not devicetree specific discussion
> > 
> >> - Need a specific management of a peripheral, due to secure, isolation,
> >> or any other reason related to the platform.
> >> => specific driver that can be bind as srm child (platform srm_dev).
> > 
> > IMO, not devicetree specific discussion
> > 
> >> To sum-up. We name shared (or system) resources every resources that
> >> have to be shared between the master and the remote processor. The list
> >> of these resources depends on the platform (and on peripheral of a
> >> platform). That's why we decide to redefine the node.
> > 
> > sorry, i can't follow here. I don't claim to replace your solution,
> > especially if you have working code I would be happy to take it over ASAP.
> 
> And i did not understand this:) just try to explain our design.
> On our platform we need to configure at least clock regulator and GPIO,
> to ensure coherence but also avoid concurrent access to some shared
> registers.
> Seems that on your platform you only need to handle the clock.
> 
> FYI you can find here some slides presented in OPENAMP weekly meeting
> and shared with Bjorn. Document has just to be considered as a work base
>  associated with the RFC. No decision done today...
> 
> http://openamp.github.io/docs/mca/remoteproc-resource-manager-overview.pdf

I can agree with all topics within this overview.

I would like to add some challenges I faced within my project and seem
to be not reflected by this overview:
- both, the slave and master system on my project is linux. Same kernel
  source build for different ARCH: Cortext A7 and Cortex M4 (nommu).
- to make all parts work properly I need to keep in sync:
  - DTs for both systems.
  - linkage offset for Cortex M4 with rproc specific ELF header and both
    DT configurations.
  - clock configurations. Even if we will be able to manage all clocks
    dynamically by resource manager we need some agreement for initial
    clock configuration. At least for uart and system clock.

Already on this development stage, so many manually configured sync
points have been endless source of different errors.
This is why I would be really happy not to create new nodes special for
resource manager and autoMagically generate DT for the slave system,
just on fly.

From this POV, it is more practical to reference phandle which should be
exported, instead of redefining it.

> > 
> >> In fact the good solution could be in the middle of this both design
> >> solutions. Means choice between redefining the node properties or just
> >> provide an handle to the soc one. For this we are thinking about a
> >> phandle to the soc node.
> >> something like ("parent-device" naming is just for the example)
> >> 	m4_uart1 {
> >> 		assigned-clock-rates = <240000000>;
> >> 		parent-device = { &uart1};
> >> 	};
> >>
> >> Another advantage of a phandle would be to be able to check that the
> >> device is disabled on Linux side and could offer the possibility to
> >> switch the peripheral between master and slave during the runtime.
> > 
> > Is it a workaround for a system without devicetree overlay?
> Here i'm speaking about possibility to dynamically switch a peripheral
> during the run time (for instance for low power purpose). If do not
> consider the config fs overlay but only uboot overlay, the resource
> manager could be also used to switch the peripheral responsibility from
> one core to the other.
> 
> > I don't see why we should provide extra container for not really extra
> > information. Or do I miss something?
> First of all seems i missed your "remote-nodes" property...no need to
> use the "parent-device".
>  	
> This is an open point, do we need extra information or not?
> 
> phandle should work for a simple device without child. Now if you have
> childs, this can became tricky to parse the node in a generic way...
> 
> And the 3 points above need also to be considered, at least until you
> clarify your POV.
> 
> Then we would like also to consider possibility to associate a specific
> driver to handle a remote device. As example we have a platform on which
> the remote processor is started before the Linux. It handles an I2C
> waiting Linux boot and then free it for Linux usage. So we implemented a
> specific driver that enable the soc device to probe the associated linux
> driver.
> That's why we offer possibility to define platform rproc_srm_dev...

We was thinking about similar scenario. In this case the bootloader (in
my case barebox) will do same initialization as it is one by linux
rproc.
Since linux rproc seems to be responable for bootstrap the slave system,
do it make sense to affiliate SRM with rproc?

> Now everything is open and i also agree with you that it could be nice
> to be able to provide also a phandle in simple usecase instead of
> redefining the properties.
> Then base on your example changing the compatible to the srm_dev instead
> of disabling the device would be cleaner.
> 
> > 
> >> To finish, an additional information: We are implementing on top of SRM
> >> a dynamic part based on rpmsg that allows to reconfigure the shared
> >> resource to allows for instance to:
> >> - change the clock rate
> >> - change pin states
> >> - change regulator constraints
> >> According to first discussion with Bjorn, we need to share this part
> >> also to present the global picture ,we would like to propose.
> > 
> > Ok, so it looks like we are working on same thing. And you probably
> > already have most of the code to make this work. Did you made Linux
> > implementation only for Master or both (Master and Slave) parts?
> we have something almost ready for the dynamic part on Linux (acting as
> resource manager server). For Linux slave a new framework should answer:
> the SCMI.
> 
> Now i don't know how you will want to go further on this topic, if you
> are interested in...
> Are you are interested in proposing some adaptation on top of the srm
> patchset, to add phandle?
> On our side we should be able to send a V2 including the dynamic part.
> This could be a good entry point for maintainer feedback...

So far, I implemented only imx-rproc and imx-mailbox. If your SRM is
cleanly separated, then I should be able to replace my node claiming
patch by SRM. Correct? If so, lets us try it :)

And depending on the results, we will be able to see how to proceed on
this topic.

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v1 1/2] imx-rproc: dt: provide new remote-nodes option

[Arnaud Pouliquen]

On 06/22/2018 08:24 AM, Oleksij Rempel wrote:
> On Tue, Jun 19, 2018 at 03:58:53PM +0200, Arnaud Pouliquen wrote:
>>
>>
>> On 06/18/2018 02:37 PM, Oleksij Rempel wrote:
>>> Hi Arnaud,
>>>
>>> On 18.06.2018 11:32, Arnaud Pouliquen wrote:
>>>> Hi Oleksij,
>>>>
>>>> On 06/15/2018 06:37 PM, Oleksij Rempel wrote:
>>>>> Hi Arnaud,
>>>>>
>>>>> On Fri, Jun 15, 2018 at 03:21:19PM +0200, Arnaud Pouliquen wrote:
>>>>>> Hi Oleksij,
>>>>>>
>>>>>> Nice to see that we have the same needs.
>>>>>> We push several month ago an RFC based on something similar but i hope
>>>>>> more generic...
>>>>>> could you have a look?
>>>>>>
>>>>>> https://www.spinics.net/lists/linux-remoteproc/msg01823.html
>>>>>
>>>>> I took a look at dt binding.
>>>>> It would be really better to not redefine device nodes again.
>>>>> DT is providing HW description and if it is still the same IP core
>>>>> then most probably it is still the same from all CPUs. Most probably
>>>>> there is different interrupt controller and memory offset, but all other
>>>>> parts should be the same.
>>>>> In long term it would be great to reduce duplicated information which is
>>>>> needed to added system developer.
>>>> This is a valid point. We are also thinking about this. But just
>>>> disabling a peripheral that is used seems also not logic from our point
>>>> of view.
>>>
>>> Right now I don't see any thing bad on disabling it. For master CPU it
>>> is indeed disabled and should not be accesable. What is your
>>> argumentation about this?
>> Perhaps It's more of a philosophical point of view than anything else...
>> But doing some action based on a disabled device looks from my POV a
>> nonsense. It look like a hack to disable the device instead of changing
>> the compatible (change compatible to rproc-srm-dev could be used for
>> this...).
> 
> Having one HW specific bindings vs application specific has at least one
> important advantage:
> - the hw will not change, but application will do.
> So it is easier to approve/mainline one HW binding and
> let SW decide how exactly it should be used.
> About status usage, I'm not 100% sure.
> I have seen products where different IP blocks was connected to the same
> pins and enabled/routed to this pins sequentially within system start.
> I have no idea how to reflect this within DT.
> In case of remote proc, we change <cpu>-<ip-block>-<pins> relation.
> I already can image customers who will require to control some thing
> by master system until slave system will be started.
> In this case, manually rewritten DT, which is reflecting some time point of system
> configuration is not really productive.
> 
>> For instance we can consider the pinctrl management. (Please tell me if
>> i'm wrong) if you disable your device the pinctrl will not configure
>> your pins. So you will need to force the pins management for a disabled
>> device...
> 
> yes, good point. I need to think about this.
> 
>>>
>>>> Furthermore how to you manage followings use cases:
>>>> - peripheral clock is not the same for master and remote processor
>>>> => you potentially need to redefine the clocks
>>>
>>> IMO, not devicetree specific discussion
>> Could you clarify what you means by " not devicetree specific
>> discussion"? do you mean that you can directly update the device using
>> overlay?
> 
> If some reusable ip block is controlled by some clock controller. It
> will depend on this controller even after the ip block was exported to
> separate CPU. On other hand the same ip block will depend on different
> interrupt controller on other CPU.
This is platform dependent. you can also have 2 separate clock trees
that can clock the Peripheral, with only one controlled by the local
clock controller.
For the interruption you will have 2 parent interrupt controller, but
you can have a common interrupt controller for a child (for instance IO
wakeup controller). Here i think it a point of attention, today in srm
we configure interruption an disable it. This allows to configure the
interrupt tree via the interrupt controller cascade but we disable it to
ensure that the local IRQ handler will not be called.

> 
>>>
>>>> - clock, regulator or pin are not managed by the Linux if peripheral is
>>>> assigned to the remote processor, but controlled by the remote processor
>>>> directly (isolation, protection on shared resource access...).
>>>> => In this case we must not handle the resource in Linux.
>>>
>>> IMO, not devicetree specific discussion
>>>
>>>> - Need a specific management of a peripheral, due to secure, isolation,
>>>> or any other reason related to the platform.
>>>> => specific driver that can be bind as srm child (platform srm_dev).
>>>
>>> IMO, not devicetree specific discussion
>>>
>>>> To sum-up. We name shared (or system) resources every resources that
>>>> have to be shared between the master and the remote processor. The list
>>>> of these resources depends on the platform (and on peripheral of a
>>>> platform). That's why we decide to redefine the node.
>>>
>>> sorry, i can't follow here. I don't claim to replace your solution,
>>> especially if you have working code I would be happy to take it over ASAP.
>>
>> And i did not understand this:) just try to explain our design.
>> On our platform we need to configure at least clock regulator and GPIO,
>> to ensure coherence but also avoid concurrent access to some shared
>> registers.
>> Seems that on your platform you only need to handle the clock.
>>
>> FYI you can find here some slides presented in OPENAMP weekly meeting
>> and shared with Bjorn. Document has just to be considered as a work base
>>  associated with the RFC. No decision done today...
>>
>> http://openamp.github.io/docs/mca/remoteproc-resource-manager-overview.pdf
> 
> I can agree with all topics within this overview.
> 
> I would like to add some challenges I faced within my project and seem
> to be not reflected by this overview:
> - both, the slave and master system on my project is linux. Same kernel
>   source build for different ARCH: Cortext A7 and Cortex M4 (nommu).
> - to make all parts work properly I need to keep in sync:
>   - DTs for both systems.
>   - linkage offset for Cortex M4 with rproc specific ELF header and both
>     DT configurations.
>   - clock configurations. Even if we will be able to manage all clocks
>     dynamically by resource manager we need some agreement for initial
>     clock configuration. At least for uart and system clock.
> 
> Already on this development stage, so many manually configured sync
> points have been endless source of different errors.
> This is why I would be really happy not to create new nodes special for
> resource manager and autoMagically generate DT for the slave system,
> just on fly.
> 
> From this POV, it is more practical to reference phandle which should be
> exported, instead of redefining it.
That seems reasonable, I think that in 80% of the case phandle allows to
do the job. i come back to the compatible solution, if you add phandle
management you could have something like that:

+	/* node reserved for rproc */
+	&uart1 {
+		compatible = "rproc-srm-dev";
+		assigned-clock-rates = <240000000>;
+	};
+
+	&gpt2 {
+		compatible = "rproc-srm-dev";
+		assigned-clock-rates = <24000000>;
+	};
+
 	imx7d-cm4 {
 		compatible	= "fsl,imx7d-cm4";
 		memory-region	= <&m4_reserved_sysmem1>, <&m4_reserved_sysmem2>;
 		syscon		= <&src>;
 		clocks		= <&clks IMX7D_ARM_M4_ROOT_CLK>;
+		system_resources {
+			compatible = "rproc-srm-core";
+			remote-nodes	= <&gpt2>, <&uart1>;
		}

> 
>>>
>>>> In fact the good solution could be in the middle of this both design
>>>> solutions. Means choice between redefining the node properties or just
>>>> provide an handle to the soc one. For this we are thinking about a
>>>> phandle to the soc node.
>>>> something like ("parent-device" naming is just for the example)
>>>> 	m4_uart1 {
>>>> 		assigned-clock-rates = <240000000>;
>>>> 		parent-device = { &uart1};
>>>> 	};
>>>>
>>>> Another advantage of a phandle would be to be able to check that the
>>>> device is disabled on Linux side and could offer the possibility to
>>>> switch the peripheral between master and slave during the runtime.
>>>
>>> Is it a workaround for a system without devicetree overlay?
>> Here i'm speaking about possibility to dynamically switch a peripheral
>> during the run time (for instance for low power purpose). If do not
>> consider the config fs overlay but only uboot overlay, the resource
>> manager could be also used to switch the peripheral responsibility from
>> one core to the other.
>>
>>> I don't see why we should provide extra container for not really extra
>>> information. Or do I miss something?
>> First of all seems i missed your "remote-nodes" property...no need to
>> use the "parent-device".
>>  	
>> This is an open point, do we need extra information or not?
>>
>> phandle should work for a simple device without child. Now if you have
>> childs, this can became tricky to parse the node in a generic way...
>>
>> And the 3 points above need also to be considered, at least until you
>> clarify your POV.
>>
>> Then we would like also to consider possibility to associate a specific
>> driver to handle a remote device. As example we have a platform on which
>> the remote processor is started before the Linux. It handles an I2C
>> waiting Linux boot and then free it for Linux usage. So we implemented a
>> specific driver that enable the soc device to probe the associated linux
>> driver.
>> That's why we offer possibility to define platform rproc_srm_dev...
> 
> We was thinking about similar scenario. In this case the bootloader (in
> my case barebox) will do same initialization as it is one by linux
> rproc.
> Since linux rproc seems to be responable for bootstrap the slave system,
> do it make sense to affiliate SRM with rproc?
Yes as the SRM is linked to the remote firmware. So make sense that
resource are allocated/free depending on remote firmware state.
Furthermore this allows to handle several co-processors.

> 
>> Now everything is open and i also agree with you that it could be nice
>> to be able to provide also a phandle in simple usecase instead of
>> redefining the properties.
>> Then base on your example changing the compatible to the srm_dev instead
>> of disabling the device would be cleaner.
>>
>>>
>>>> To finish, an additional information: We are implementing on top of SRM
>>>> a dynamic part based on rpmsg that allows to reconfigure the shared
>>>> resource to allows for instance to:
>>>> - change the clock rate
>>>> - change pin states
>>>> - change regulator constraints
>>>> According to first discussion with Bjorn, we need to share this part
>>>> also to present the global picture ,we would like to propose.
>>>
>>> Ok, so it looks like we are working on same thing. And you probably
>>> already have most of the code to make this work. Did you made Linux
>>> implementation only for Master or both (Master and Slave) parts?
>> we have something almost ready for the dynamic part on Linux (acting as
>> resource manager server). For Linux slave a new framework should answer:
>> the SCMI.
>>
>> Now i don't know how you will want to go further on this topic, if you
>> are interested in...
>> Are you are interested in proposing some adaptation on top of the srm
>> patchset, to add phandle?
>> On our side we should be able to send a V2 including the dynamic part.
>> This could be a good entry point for maintainer feedback...
> 
> So far, I implemented only imx-rproc and imx-mailbox. If your SRM is
> cleanly separated, then I should be able to replace my node claiming
> patch by SRM. Correct? If so, lets us try it :)
Yes SRM is optional. Just need to declare it as child of your rproc node
in RFC we add a patch (probably temporary) that probes the SRM in
rproc_core.
Then as a first step you would advise you to redeclare the node per
peripheral, before patching to introduce phandle.

> 
> And depending on the results, we will be able to see how to proceed on
> this topic.
> 
Agree, on our side we should send a V2 next week to align on last update
and add the dynamic part of the SRM.

Regards
Arnaud

Re: [PATCH v1 1/2] imx-rproc: dt: provide new remote-nodes option

[Bjorn Andersson]

On Fri 15 Jun 09:37 PDT 2018, Oleksij Rempel wrote:

> Hi Arnaud,
> 
> On Fri, Jun 15, 2018 at 03:21:19PM +0200, Arnaud Pouliquen wrote:
> > Hi Oleksij,
> > 
> > Nice to see that we have the same needs.
> > We push several month ago an RFC based on something similar but i hope
> > more generic...
> > could you have a look?
> > 
> > https://www.spinics.net/lists/linux-remoteproc/msg01823.html
> 
> I took a look at dt binding.
> It would be really better to not redefine device nodes again.

Defining new nodes next to the proper (disabled) ones would only
duplicate the data and doesn't provide the means for preventing
conflicting usage.

> DT is providing HW description and if it is still the same IP core
> then most probably it is still the same from all CPUs.

The problem I see is that for every convenient case there's a lot of
cases where this doesn't work.

> Most probably there is different interrupt controller and memory
> offset, but all other parts should be the same.

This conclusion is what I've come to as well; clocks seems simple and
then it gets complicated. So what we would end up with is a mechanism
where we rely on some properties of the remote node representing the
resources of the remote processor, but other doesn't.

This becomes very complex to maintain...

> In long term it would be great to reduce duplicated information which is
> needed to added system developer.
> 

Agreed, but I don't think that adding a partial view of the remote
processor to the local system's hardware definition is going to make it
easier for said developer.

> > Could be nice if we could find a generic solution...
> 
> I would be happy to have generic solution. 
> 

Me too, and based on our discussions on the subject I think it's
important that we consider how to handle dynamically controlled
resources (though some RPMSG based resource management protocol) as
well - I don't want one mechanism for static clocks and a completely
different for dynamically controlled clocks.

Regards,
Bjorn

Re: [PATCH v1 1/2] imx-rproc: dt: provide new remote-nodes option

[Bjorn Andersson]

On Fri 15 Jun 04:57 PDT 2018, Oleksij Rempel wrote:

> On AMP systems we need to make sure that some device
> nodes are not used by main system and reserved for
> external system. Some of configuration should be
> maintained by main system. For example clocks and pins.
> 

I think you should reverse the order of this message, because what you
really care about is the clock rate for some resources, then you see
that there's a chance that you would like pinctrl states and lastly it
would be nice to help the system integrator not accidentally using the
same hardware from the two sides.

> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  .../devicetree/bindings/remoteproc/imx-rproc.txt    | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt b/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
> index fbcefd965dc4..40bec03e094c 100644
> --- a/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
> +++ b/Documentation/devicetree/bindings/remoteproc/imx-rproc.txt
> @@ -15,6 +15,7 @@ Required properties:
>  Optional properties:
>  - memory-region		list of phandels to the reserved memory regions.
>  			(See: ../reserved-memory/reserved-memory.txt)
> +- remote-nodes		list of device node phandels used by remote system.
>  
>  Example:
>  	m4_reserved_sysmem1: cm4@80000000 {
> @@ -25,9 +26,21 @@ Example:
>  		reg = <0x81000000 0x80000>;
>  	};
>  
> +	/* node reserved for rproc */
> +	&uart1 {
> +		assigned-clock-rates = <240000000>;
> +		status = "disabled";
> +	};
> +
> +	&gpt2 {
> +		assigned-clock-rates = <24000000>;
> +		status = "disabled";
> +	};
> +
>  	imx7d-cm4 {
>  		compatible	= "fsl,imx7d-cm4";
>  		memory-region	= <&m4_reserved_sysmem1>, <&m4_reserved_sysmem2>;
>  		syscon		= <&src>;
>  		clocks		= <&clks IMX7D_ARM_M4_ROOT_CLK>;
> +		remote-nodes	= <&gpt2>, <&uart1>;

As I told Arnaud I think it would be better to describe resources used
by the remoteproc firmware as those of the remoteproc node, i.e. add the
uart and gpt clocks to the remoteproc node directly.

The reason for this is that even though your system does have many of
the peripherals accessible from all instances of the AMP you're still
going to run them off different views of the hardware.

We typically say "DeviceTree should represent the hardware", but when
taking AMP in consideration I see this needs to be "DeviceTree should
represent the hardware, as seen from this particular OS instance".

Regards,
Bjorn