[PATCH 1/2] dt-bindings: remoteproc: imx_rproc: Document fsl,startup-delay-ms

[PATCH 1/2] dt-bindings: remoteproc: imx_rproc: Document fsl,startup-delay-ms

[Marek Vasut]

Document fsl,startup-delay-ms property which indicates how long
the system software should wait until attempting to communicate
with the CM firmware. This gives the CM firmware a bit of time
to boot and get ready for communication.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Bjorn Andersson <andersson@kernel.org>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: NXP Linux Team <linux-imx@nxp.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-remoteproc@vger.kernel.org
---
 .../devicetree/bindings/remoteproc/fsl,imx-rproc.yaml        | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
index 0c3910f152d1d..c940199ce89df 100644
--- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
@@ -76,6 +76,11 @@ properties:
       This property is to specify the resource id of the remote processor in SoC
       which supports SCFW
 
+  fsl,startup-delay-ms:
+    default: 0
+    description:
+      CM firmware start up delay.
+
 required:
   - compatible
 
-- 
2.40.1

[PATCH 2/2] remoteproc: imx_rproc: add start up delay

[Marek Vasut]

From: Peng Fan <peng.fan@nxp.com>

There is case that after remoteproc start remote processor[M4], the M4
runs slow and before M4 finish its own rpmsg framework initialization,
linux sends out vring kick message, then M4 firmware drops the kick
message. Some NXP released Cortex-M[x] images has such limitation that
it requires linux sends out vring kick message after M4 firmware finish
its rpmsg framework initialization.

Reviewed-by: Jacky Bai <ping.bai@nxp.com>
Reviewed-by: Ye Li <ye.li@nxp.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
Note: picked from NXP downstream LF-6630-2 remoteproc: imx_rproc: add start up delay
https://github.com/nxp-imx/linux-imx.git 0b1b91c95b291a3b60d6224b13f6a95a75896abf
---
Note: Literally all of the NXP BSP 2.13.0 firmware builds fail to boot
      without this being set to something like 50..500 ms , so this is
      rather useful to have.
---
Cc: Bjorn Andersson <andersson@kernel.org>
Cc: Conor Dooley <conor+dt@kernel.org>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: NXP Linux Team <linux-imx@nxp.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: devicetree@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-remoteproc@vger.kernel.org
---
 drivers/remoteproc/imx_rproc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
index f9874fc5a80ff..d0eb96d6a4fe1 100644
--- a/drivers/remoteproc/imx_rproc.c
+++ b/drivers/remoteproc/imx_rproc.c
@@ -6,6 +6,7 @@
 #include <dt-bindings/firmware/imx/rsrc.h>
 #include <linux/arm-smccc.h>
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/firmware/imx/sci.h>
 #include <linux/interrupt.h>
@@ -110,6 +111,7 @@ struct imx_rproc {
 	u32				core_index;
 	struct device                   **pd_dev;
 	struct device_link              **pd_dev_link;
+	u32				startup_delay;
 };
 
 static const struct imx_rproc_att imx_rproc_att_imx93[] = {
@@ -382,6 +384,9 @@ static int imx_rproc_start(struct rproc *rproc)
 	if (ret)
 		dev_err(dev, "Failed to enable remote core!\n");
 
+	if (priv->startup_delay)
+		msleep(priv->startup_delay);
+
 	return ret;
 }
 
@@ -1090,6 +1095,10 @@ static int imx_rproc_probe(struct platform_device *pdev)
 	if (rproc->state != RPROC_DETACHED)
 		rproc->auto_boot = of_property_read_bool(np, "fsl,auto-boot");
 
+	ret = of_property_read_u32(dev->of_node, "fsl,startup-delay-ms", &priv->startup_delay);
+	if (ret)
+		priv->startup_delay = 0;
+
 	ret = rproc_add(rproc);
 	if (ret) {
 		dev_err(dev, "rproc_add failed\n");
-- 
2.40.1

Re: [PATCH 2/2] remoteproc: imx_rproc: add start up delay

[Mathieu Poirier]

On Sat, Jul 08, 2023 at 01:24:44AM +0200, Marek Vasut wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> There is case that after remoteproc start remote processor[M4], the M4
> runs slow and before M4 finish its own rpmsg framework initialization,
> linux sends out vring kick message, then M4 firmware drops the kick
> message. Some NXP released Cortex-M[x] images has such limitation that
> it requires linux sends out vring kick message after M4 firmware finish
> its rpmsg framework initialization.
> 
> Reviewed-by: Jacky Bai <ping.bai@nxp.com>
> Reviewed-by: Ye Li <ye.li@nxp.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> Note: picked from NXP downstream LF-6630-2 remoteproc: imx_rproc: add start up delay
> https://github.com/nxp-imx/linux-imx.git 0b1b91c95b291a3b60d6224b13f6a95a75896abf
> ---
> Note: Literally all of the NXP BSP 2.13.0 firmware builds fail to boot
>       without this being set to something like 50..500 ms , so this is
>       rather useful to have.

My stance on this hasn't changed - hacks such as these do not scale and are a
nightmare to maintain.  The problem should be fixed in the M4's firmware.

> ---
> Cc: Bjorn Andersson <andersson@kernel.org>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-remoteproc@vger.kernel.org
> ---
>  drivers/remoteproc/imx_rproc.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/remoteproc/imx_rproc.c b/drivers/remoteproc/imx_rproc.c
> index f9874fc5a80ff..d0eb96d6a4fe1 100644
> --- a/drivers/remoteproc/imx_rproc.c
> +++ b/drivers/remoteproc/imx_rproc.c
> @@ -6,6 +6,7 @@
>  #include <dt-bindings/firmware/imx/rsrc.h>
>  #include <linux/arm-smccc.h>
>  #include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/firmware/imx/sci.h>
>  #include <linux/interrupt.h>
> @@ -110,6 +111,7 @@ struct imx_rproc {
>  	u32				core_index;
>  	struct device                   **pd_dev;
>  	struct device_link              **pd_dev_link;
> +	u32				startup_delay;
>  };
>  
>  static const struct imx_rproc_att imx_rproc_att_imx93[] = {
> @@ -382,6 +384,9 @@ static int imx_rproc_start(struct rproc *rproc)
>  	if (ret)
>  		dev_err(dev, "Failed to enable remote core!\n");
>  
> +	if (priv->startup_delay)
> +		msleep(priv->startup_delay);
> +
>  	return ret;
>  }
>  
> @@ -1090,6 +1095,10 @@ static int imx_rproc_probe(struct platform_device *pdev)
>  	if (rproc->state != RPROC_DETACHED)
>  		rproc->auto_boot = of_property_read_bool(np, "fsl,auto-boot");
>  
> +	ret = of_property_read_u32(dev->of_node, "fsl,startup-delay-ms", &priv->startup_delay);
> +	if (ret)
> +		priv->startup_delay = 0;
> +
>  	ret = rproc_add(rproc);
>  	if (ret) {
>  		dev_err(dev, "rproc_add failed\n");
> -- 
> 2.40.1
> 

Re: [PATCH 2/2] remoteproc: imx_rproc: add start up delay

[Marek Vasut]

On 7/10/23 17:53, Mathieu Poirier wrote:
> On Sat, Jul 08, 2023 at 01:24:44AM +0200, Marek Vasut wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>>
>> There is case that after remoteproc start remote processor[M4], the M4
>> runs slow and before M4 finish its own rpmsg framework initialization,
>> linux sends out vring kick message, then M4 firmware drops the kick
>> message. Some NXP released Cortex-M[x] images has such limitation that
>> it requires linux sends out vring kick message after M4 firmware finish
>> its rpmsg framework initialization.
>>
>> Reviewed-by: Jacky Bai <ping.bai@nxp.com>
>> Reviewed-by: Ye Li <ye.li@nxp.com>
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>> Note: picked from NXP downstream LF-6630-2 remoteproc: imx_rproc: add start up delay
>> https://github.com/nxp-imx/linux-imx.git 0b1b91c95b291a3b60d6224b13f6a95a75896abf
>> ---
>> Note: Literally all of the NXP BSP 2.13.0 firmware builds fail to boot
>>        without this being set to something like 50..500 ms , so this is
>>        rather useful to have.
> 
> My stance on this hasn't changed - hacks such as these do not scale and are a
> nightmare to maintain.  The problem should be fixed in the M4's firmware.

If the firmware cannot be updated, how do you propose that would be 
fixed in the firmware ?

Frankly, I do not see much of an issue in maintaining driver-specific 
mdelay().

RE: [PATCH 1/2] dt-bindings: remoteproc: imx_rproc: Document fsl,startup-delay-ms

[Peng Fan]

> Subject: [PATCH 1/2] dt-bindings: remoteproc: imx_rproc: Document
> fsl,startup-delay-ms
> 
> Document fsl,startup-delay-ms property which indicates how long the
> system software should wait until attempting to communicate with the CM
> firmware. This gives the CM firmware a bit of time to boot and get ready for
> communication.

This property was rejected before, but anyway I would still hope we could
get this in.

Thanks,
Peng.

> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Bjorn Andersson <andersson@kernel.org>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-remoteproc@vger.kernel.org
> ---
>  .../devicetree/bindings/remoteproc/fsl,imx-rproc.yaml        | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/fsl,imx-
> rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-
> rproc.yaml
> index 0c3910f152d1d..c940199ce89df 100644
> --- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> @@ -76,6 +76,11 @@ properties:
>        This property is to specify the resource id of the remote processor in
> SoC
>        which supports SCFW
> 
> +  fsl,startup-delay-ms:
> +    default: 0
> +    description:
> +      CM firmware start up delay.
> +
>  required:
>    - compatible
> 
> --
> 2.40.1

Re: [PATCH 1/2] dt-bindings: remoteproc: imx_rproc: Document fsl,startup-delay-ms

[Marek Vasut]

On 7/10/23 03:22, Peng Fan wrote:
>> Subject: [PATCH 1/2] dt-bindings: remoteproc: imx_rproc: Document
>> fsl,startup-delay-ms
>>
>> Document fsl,startup-delay-ms property which indicates how long the
>> system software should wait until attempting to communicate with the CM
>> firmware. This gives the CM firmware a bit of time to boot and get ready for
>> communication.
> 
> This property was rejected before, but anyway I would still hope we could
> get this in.

How so ?

Re: [PATCH 1/2] dt-bindings: remoteproc: imx_rproc: Document fsl,startup-delay-ms

[Krzysztof Kozlowski]

On 08/07/2023 01:24, Marek Vasut wrote:
> Document fsl,startup-delay-ms property which indicates how long
> the system software should wait until attempting to communicate
> with the CM firmware. This gives the CM firmware a bit of time
> to boot and get ready for communication.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Bjorn Andersson <andersson@kernel.org>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> Cc: NXP Linux Team <linux-imx@nxp.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-remoteproc@vger.kernel.org
> ---
>  .../devicetree/bindings/remoteproc/fsl,imx-rproc.yaml        | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> index 0c3910f152d1d..c940199ce89df 100644
> --- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> +++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> @@ -76,6 +76,11 @@ properties:
>        This property is to specify the resource id of the remote processor in SoC
>        which supports SCFW
>  
> +  fsl,startup-delay-ms:
> +    default: 0
> +    description:
> +      CM firmware start up delay.

I don't see particular improvements from v2 and no responses addressing
my comment:
https://lore.kernel.org/all/20221102112451.128110-2-peng.fan@oss.nxp.com/


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: remoteproc: imx_rproc: Document fsl,startup-delay-ms

[Marek Vasut]

On 7/10/23 10:12, Krzysztof Kozlowski wrote:
> On 08/07/2023 01:24, Marek Vasut wrote:
>> Document fsl,startup-delay-ms property which indicates how long
>> the system software should wait until attempting to communicate
>> with the CM firmware. This gives the CM firmware a bit of time
>> to boot and get ready for communication.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> Cc: Bjorn Andersson <andersson@kernel.org>
>> Cc: Conor Dooley <conor+dt@kernel.org>
>> Cc: Fabio Estevam <festevam@gmail.com>
>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>> Cc: NXP Linux Team <linux-imx@nxp.com>
>> Cc: Peng Fan <peng.fan@nxp.com>
>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>> Cc: Shawn Guo <shawnguo@kernel.org>
>> Cc: devicetree@vger.kernel.org
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-remoteproc@vger.kernel.org
>> ---
>>   .../devicetree/bindings/remoteproc/fsl,imx-rproc.yaml        | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>> index 0c3910f152d1d..c940199ce89df 100644
>> --- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>> +++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>> @@ -76,6 +76,11 @@ properties:
>>         This property is to specify the resource id of the remote processor in SoC
>>         which supports SCFW
>>   
>> +  fsl,startup-delay-ms:
>> +    default: 0
>> +    description:
>> +      CM firmware start up delay.
> 
> I don't see particular improvements from v2 and no responses addressing
> my comment:
> https://lore.kernel.org/all/20221102112451.128110-2-peng.fan@oss.nxp.com/

I wasn't aware of this being submitted before, esp. since I wrote the 
binding document from scratch. Which comment is not addressed, the type 
ref is not present and the sentence starts with caps, so what is missing ?

Re: [PATCH 1/2] dt-bindings: remoteproc: imx_rproc: Document fsl,startup-delay-ms

[Krzysztof Kozlowski]

On 10/07/2023 11:18, Marek Vasut wrote:
> On 7/10/23 10:12, Krzysztof Kozlowski wrote:
>> On 08/07/2023 01:24, Marek Vasut wrote:
>>> Document fsl,startup-delay-ms property which indicates how long
>>> the system software should wait until attempting to communicate
>>> with the CM firmware. This gives the CM firmware a bit of time
>>> to boot and get ready for communication.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> ---
>>> Cc: Bjorn Andersson <andersson@kernel.org>
>>> Cc: Conor Dooley <conor+dt@kernel.org>
>>> Cc: Fabio Estevam <festevam@gmail.com>
>>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> Cc: NXP Linux Team <linux-imx@nxp.com>
>>> Cc: Peng Fan <peng.fan@nxp.com>
>>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>>> Cc: Shawn Guo <shawnguo@kernel.org>
>>> Cc: devicetree@vger.kernel.org
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: linux-remoteproc@vger.kernel.org
>>> ---
>>>   .../devicetree/bindings/remoteproc/fsl,imx-rproc.yaml        | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>> index 0c3910f152d1d..c940199ce89df 100644
>>> --- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>> +++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>> @@ -76,6 +76,11 @@ properties:
>>>         This property is to specify the resource id of the remote processor in SoC
>>>         which supports SCFW
>>>   
>>> +  fsl,startup-delay-ms:
>>> +    default: 0
>>> +    description:
>>> +      CM firmware start up delay.
>>
>> I don't see particular improvements from v2 and no responses addressing
>> my comment:
>> https://lore.kernel.org/all/20221102112451.128110-2-peng.fan@oss.nxp.com/
> 
> I wasn't aware of this being submitted before, esp. since I wrote the 
> binding document from scratch. Which comment is not addressed, the type 
> ref is not present and the sentence starts with caps, so what is missing ?


That the property looks like a hacky solution to some SW problem. Why
this delay should be different on different boards?

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: remoteproc: imx_rproc: Document fsl,startup-delay-ms

[Marek Vasut]

On 7/10/23 14:52, Krzysztof Kozlowski wrote:
> On 10/07/2023 11:18, Marek Vasut wrote:
>> On 7/10/23 10:12, Krzysztof Kozlowski wrote:
>>> On 08/07/2023 01:24, Marek Vasut wrote:
>>>> Document fsl,startup-delay-ms property which indicates how long
>>>> the system software should wait until attempting to communicate
>>>> with the CM firmware. This gives the CM firmware a bit of time
>>>> to boot and get ready for communication.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> ---
>>>> Cc: Bjorn Andersson <andersson@kernel.org>
>>>> Cc: Conor Dooley <conor+dt@kernel.org>
>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
>>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>> Cc: NXP Linux Team <linux-imx@nxp.com>
>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>>>> Cc: Shawn Guo <shawnguo@kernel.org>
>>>> Cc: devicetree@vger.kernel.org
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> Cc: linux-remoteproc@vger.kernel.org
>>>> ---
>>>>    .../devicetree/bindings/remoteproc/fsl,imx-rproc.yaml        | 5 +++++
>>>>    1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>>> index 0c3910f152d1d..c940199ce89df 100644
>>>> --- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>>> +++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>>> @@ -76,6 +76,11 @@ properties:
>>>>          This property is to specify the resource id of the remote processor in SoC
>>>>          which supports SCFW
>>>>    
>>>> +  fsl,startup-delay-ms:
>>>> +    default: 0
>>>> +    description:
>>>> +      CM firmware start up delay.
>>>
>>> I don't see particular improvements from v2 and no responses addressing
>>> my comment:
>>> https://lore.kernel.org/all/20221102112451.128110-2-peng.fan@oss.nxp.com/
>>
>> I wasn't aware of this being submitted before, esp. since I wrote the
>> binding document from scratch. Which comment is not addressed, the type
>> ref is not present and the sentence starts with caps, so what is missing ?
> 
> 
> That the property looks like a hacky solution to some SW problem. Why
> this delay should be different on different boards?

It probably depends more on the CM4 firmware that is being launched. The 
ones I tested were fine with 50..500ms delay, but the delay was always 
needed.

Sure, it is a defect of the NXP provided SDK firmware, but that may not 
be fixable in all cases.

Re: [PATCH 1/2] dt-bindings: remoteproc: imx_rproc: Document fsl,startup-delay-ms

[Krzysztof Kozlowski]

On 10/07/2023 15:46, Marek Vasut wrote:
> On 7/10/23 14:52, Krzysztof Kozlowski wrote:
>> On 10/07/2023 11:18, Marek Vasut wrote:
>>> On 7/10/23 10:12, Krzysztof Kozlowski wrote:
>>>> On 08/07/2023 01:24, Marek Vasut wrote:
>>>>> Document fsl,startup-delay-ms property which indicates how long
>>>>> the system software should wait until attempting to communicate
>>>>> with the CM firmware. This gives the CM firmware a bit of time
>>>>> to boot and get ready for communication.
>>>>>
>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>> ---
>>>>> Cc: Bjorn Andersson <andersson@kernel.org>
>>>>> Cc: Conor Dooley <conor+dt@kernel.org>
>>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
>>>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>>> Cc: NXP Linux Team <linux-imx@nxp.com>
>>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
>>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>>>>> Cc: Shawn Guo <shawnguo@kernel.org>
>>>>> Cc: devicetree@vger.kernel.org
>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>> Cc: linux-remoteproc@vger.kernel.org
>>>>> ---
>>>>>    .../devicetree/bindings/remoteproc/fsl,imx-rproc.yaml        | 5 +++++
>>>>>    1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>>>> index 0c3910f152d1d..c940199ce89df 100644
>>>>> --- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>>>> +++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>>>> @@ -76,6 +76,11 @@ properties:
>>>>>          This property is to specify the resource id of the remote processor in SoC
>>>>>          which supports SCFW
>>>>>    
>>>>> +  fsl,startup-delay-ms:
>>>>> +    default: 0
>>>>> +    description:
>>>>> +      CM firmware start up delay.
>>>>
>>>> I don't see particular improvements from v2 and no responses addressing
>>>> my comment:
>>>> https://lore.kernel.org/all/20221102112451.128110-2-peng.fan@oss.nxp.com/
>>>
>>> I wasn't aware of this being submitted before, esp. since I wrote the
>>> binding document from scratch. Which comment is not addressed, the type
>>> ref is not present and the sentence starts with caps, so what is missing ?
>>
>>
>> That the property looks like a hacky solution to some SW problem. Why
>> this delay should be different on different boards?
> 
> It probably depends more on the CM4 firmware that is being launched. The 
> ones I tested were fine with 50..500ms delay, but the delay was always 
> needed.

If this is for some official remoteproc FW running on M4, then probably
this could be implied by compatible. Otherwise, if this depends on
actual M4 firmware which can totally vary between each board of the same
type (I can run my own FW on M4, right?), then it is not suitable DT
property. How it would even look like? You add here 500 ms for all known
firmwares and then someone comes with FW requiring delay of 600 ms.

> 
> Sure, it is a defect of the NXP provided SDK firmware, but that may not 
> be fixable in all cases.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: remoteproc: imx_rproc: Document fsl,startup-delay-ms

[Marek Vasut]

On 7/10/23 22:00, Krzysztof Kozlowski wrote:
> On 10/07/2023 15:46, Marek Vasut wrote:
>> On 7/10/23 14:52, Krzysztof Kozlowski wrote:
>>> On 10/07/2023 11:18, Marek Vasut wrote:
>>>> On 7/10/23 10:12, Krzysztof Kozlowski wrote:
>>>>> On 08/07/2023 01:24, Marek Vasut wrote:
>>>>>> Document fsl,startup-delay-ms property which indicates how long
>>>>>> the system software should wait until attempting to communicate
>>>>>> with the CM firmware. This gives the CM firmware a bit of time
>>>>>> to boot and get ready for communication.
>>>>>>
>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>> ---
>>>>>> Cc: Bjorn Andersson <andersson@kernel.org>
>>>>>> Cc: Conor Dooley <conor+dt@kernel.org>
>>>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>>>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
>>>>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>>>> Cc: NXP Linux Team <linux-imx@nxp.com>
>>>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>>>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
>>>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>>>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>>>>>> Cc: Shawn Guo <shawnguo@kernel.org>
>>>>>> Cc: devicetree@vger.kernel.org
>>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>>> Cc: linux-remoteproc@vger.kernel.org
>>>>>> ---
>>>>>>     .../devicetree/bindings/remoteproc/fsl,imx-rproc.yaml        | 5 +++++
>>>>>>     1 file changed, 5 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>>>>> index 0c3910f152d1d..c940199ce89df 100644
>>>>>> --- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>>>>> @@ -76,6 +76,11 @@ properties:
>>>>>>           This property is to specify the resource id of the remote processor in SoC
>>>>>>           which supports SCFW
>>>>>>     
>>>>>> +  fsl,startup-delay-ms:
>>>>>> +    default: 0
>>>>>> +    description:
>>>>>> +      CM firmware start up delay.
>>>>>
>>>>> I don't see particular improvements from v2 and no responses addressing
>>>>> my comment:
>>>>> https://lore.kernel.org/all/20221102112451.128110-2-peng.fan@oss.nxp.com/
>>>>
>>>> I wasn't aware of this being submitted before, esp. since I wrote the
>>>> binding document from scratch. Which comment is not addressed, the type
>>>> ref is not present and the sentence starts with caps, so what is missing ?
>>>
>>>
>>> That the property looks like a hacky solution to some SW problem. Why
>>> this delay should be different on different boards?
>>
>> It probably depends more on the CM4 firmware that is being launched. The
>> ones I tested were fine with 50..500ms delay, but the delay was always
>> needed.
> 
> If this is for some official remoteproc FW running on M4

It is not, it is some SDK which can be downloaded from NXP website, 
which can then be used to compile the firmware blob. The license is 
BSD-3 however, so it is conductive to producing binaries without 
matching sources ...

>, then probably
> this could be implied by compatible. Otherwise, if this depends on
> actual M4 firmware which can totally vary between each board of the same
> type (I can run my own FW on M4, right?

Yeah

> ), then it is not suitable DT
> property. How it would even look like? You add here 500 ms for all known
> firmwares and then someone comes with FW requiring delay of 600 ms.

I would say, some sane default and if some firmware is specifically 
weird, just up the delay. It is better than no firmware working at all.

Do you have a better hint how to deal with this ?

Re: [PATCH 1/2] dt-bindings: remoteproc: imx_rproc: Document fsl,startup-delay-ms

[Mathieu Poirier]

On Mon, 10 Jul 2023 at 15:53, Marek Vasut <marex@denx.de> wrote:
>
> On 7/10/23 22:00, Krzysztof Kozlowski wrote:
> > On 10/07/2023 15:46, Marek Vasut wrote:
> >> On 7/10/23 14:52, Krzysztof Kozlowski wrote:
> >>> On 10/07/2023 11:18, Marek Vasut wrote:
> >>>> On 7/10/23 10:12, Krzysztof Kozlowski wrote:
> >>>>> On 08/07/2023 01:24, Marek Vasut wrote:
> >>>>>> Document fsl,startup-delay-ms property which indicates how long
> >>>>>> the system software should wait until attempting to communicate
> >>>>>> with the CM firmware. This gives the CM firmware a bit of time
> >>>>>> to boot and get ready for communication.
> >>>>>>
> >>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>>> ---
> >>>>>> Cc: Bjorn Andersson <andersson@kernel.org>
> >>>>>> Cc: Conor Dooley <conor+dt@kernel.org>
> >>>>>> Cc: Fabio Estevam <festevam@gmail.com>
> >>>>>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> >>>>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> >>>>>> Cc: NXP Linux Team <linux-imx@nxp.com>
> >>>>>> Cc: Peng Fan <peng.fan@nxp.com>
> >>>>>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> >>>>>> Cc: Rob Herring <robh+dt@kernel.org>
> >>>>>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> >>>>>> Cc: Shawn Guo <shawnguo@kernel.org>
> >>>>>> Cc: devicetree@vger.kernel.org
> >>>>>> Cc: linux-arm-kernel@lists.infradead.org
> >>>>>> Cc: linux-remoteproc@vger.kernel.org
> >>>>>> ---
> >>>>>>     .../devicetree/bindings/remoteproc/fsl,imx-rproc.yaml        | 5 +++++
> >>>>>>     1 file changed, 5 insertions(+)
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> >>>>>> index 0c3910f152d1d..c940199ce89df 100644
> >>>>>> --- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> >>>>>> +++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> >>>>>> @@ -76,6 +76,11 @@ properties:
> >>>>>>           This property is to specify the resource id of the remote processor in SoC
> >>>>>>           which supports SCFW
> >>>>>>
> >>>>>> +  fsl,startup-delay-ms:
> >>>>>> +    default: 0
> >>>>>> +    description:
> >>>>>> +      CM firmware start up delay.
> >>>>>
> >>>>> I don't see particular improvements from v2 and no responses addressing
> >>>>> my comment:
> >>>>> https://lore.kernel.org/all/20221102112451.128110-2-peng.fan@oss.nxp.com/
> >>>>
> >>>> I wasn't aware of this being submitted before, esp. since I wrote the
> >>>> binding document from scratch. Which comment is not addressed, the type
> >>>> ref is not present and the sentence starts with caps, so what is missing ?
> >>>
> >>>
> >>> That the property looks like a hacky solution to some SW problem. Why
> >>> this delay should be different on different boards?
> >>
> >> It probably depends more on the CM4 firmware that is being launched. The
> >> ones I tested were fine with 50..500ms delay, but the delay was always
> >> needed.
> >
> > If this is for some official remoteproc FW running on M4
>
> It is not, it is some SDK which can be downloaded from NXP website,
> which can then be used to compile the firmware blob. The license is
> BSD-3 however, so it is conductive to producing binaries without
> matching sources ...
>

Why can't the SDK be upgraded to provide some kind of hand-shake
mechanism, as suggested when I first reviewed this patchset?

> >, then probably
> > this could be implied by compatible. Otherwise, if this depends on
> > actual M4 firmware which can totally vary between each board of the same
> > type (I can run my own FW on M4, right?
>
> Yeah
>
> > ), then it is not suitable DT
> > property. How it would even look like? You add here 500 ms for all known
> > firmwares and then someone comes with FW requiring delay of 600 ms.
>
> I would say, some sane default and if some firmware is specifically
> weird, just up the delay. It is better than no firmware working at all.
>
> Do you have a better hint how to deal with this ?

Re: [PATCH 1/2] dt-bindings: remoteproc: imx_rproc: Document fsl,startup-delay-ms

[Marek Vasut]

On 7/11/23 00:01, Mathieu Poirier wrote:
> On Mon, 10 Jul 2023 at 15:53, Marek Vasut <marex@denx.de> wrote:
>>
>> On 7/10/23 22:00, Krzysztof Kozlowski wrote:
>>> On 10/07/2023 15:46, Marek Vasut wrote:
>>>> On 7/10/23 14:52, Krzysztof Kozlowski wrote:
>>>>> On 10/07/2023 11:18, Marek Vasut wrote:
>>>>>> On 7/10/23 10:12, Krzysztof Kozlowski wrote:
>>>>>>> On 08/07/2023 01:24, Marek Vasut wrote:
>>>>>>>> Document fsl,startup-delay-ms property which indicates how long
>>>>>>>> the system software should wait until attempting to communicate
>>>>>>>> with the CM firmware. This gives the CM firmware a bit of time
>>>>>>>> to boot and get ready for communication.
>>>>>>>>
>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>> ---
>>>>>>>> Cc: Bjorn Andersson <andersson@kernel.org>
>>>>>>>> Cc: Conor Dooley <conor+dt@kernel.org>
>>>>>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>>>>>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
>>>>>>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>>>>>> Cc: NXP Linux Team <linux-imx@nxp.com>
>>>>>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>>>>>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
>>>>>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>>>>>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>>>>>>>> Cc: Shawn Guo <shawnguo@kernel.org>
>>>>>>>> Cc: devicetree@vger.kernel.org
>>>>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>>>>> Cc: linux-remoteproc@vger.kernel.org
>>>>>>>> ---
>>>>>>>>      .../devicetree/bindings/remoteproc/fsl,imx-rproc.yaml        | 5 +++++
>>>>>>>>      1 file changed, 5 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>>>>>>> index 0c3910f152d1d..c940199ce89df 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>>>>>>> +++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>>>>>>> @@ -76,6 +76,11 @@ properties:
>>>>>>>>            This property is to specify the resource id of the remote processor in SoC
>>>>>>>>            which supports SCFW
>>>>>>>>
>>>>>>>> +  fsl,startup-delay-ms:
>>>>>>>> +    default: 0
>>>>>>>> +    description:
>>>>>>>> +      CM firmware start up delay.
>>>>>>>
>>>>>>> I don't see particular improvements from v2 and no responses addressing
>>>>>>> my comment:
>>>>>>> https://lore.kernel.org/all/20221102112451.128110-2-peng.fan@oss.nxp.com/
>>>>>>
>>>>>> I wasn't aware of this being submitted before, esp. since I wrote the
>>>>>> binding document from scratch. Which comment is not addressed, the type
>>>>>> ref is not present and the sentence starts with caps, so what is missing ?
>>>>>
>>>>>
>>>>> That the property looks like a hacky solution to some SW problem. Why
>>>>> this delay should be different on different boards?
>>>>
>>>> It probably depends more on the CM4 firmware that is being launched. The
>>>> ones I tested were fine with 50..500ms delay, but the delay was always
>>>> needed.
>>>
>>> If this is for some official remoteproc FW running on M4
>>
>> It is not, it is some SDK which can be downloaded from NXP website,
>> which can then be used to compile the firmware blob. The license is
>> BSD-3 however, so it is conductive to producing binaries without
>> matching sources ...
>>
> 
> Why can't the SDK be upgraded to provide some kind of hand-shake
> mechanism, as suggested when I first reviewed this patchset?

I'd argue because of legacy firmware that is already deployed.
New firmware builds can, old ones probably cannot be fixed.

Do you have a suggestion how such a mechanism should look like?
As far as I can tell, the MX8M SDK stuff looks very similar to the STM32 
Cube stuff, so maybe the mechanism is already there ?

Re: [PATCH 1/2] dt-bindings: remoteproc: imx_rproc: Document fsl,startup-delay-ms

[Frieder Schrempf]

On 11.07.23 00:23, Marek Vasut wrote:
> On 7/11/23 00:01, Mathieu Poirier wrote:
>> On Mon, 10 Jul 2023 at 15:53, Marek Vasut <marex@denx.de> wrote:
>>>
>>> On 7/10/23 22:00, Krzysztof Kozlowski wrote:
>>>> On 10/07/2023 15:46, Marek Vasut wrote:
>>>>> On 7/10/23 14:52, Krzysztof Kozlowski wrote:
>>>>>> On 10/07/2023 11:18, Marek Vasut wrote:
>>>>>>> On 7/10/23 10:12, Krzysztof Kozlowski wrote:
>>>>>>>> On 08/07/2023 01:24, Marek Vasut wrote:
>>>>>>>>> Document fsl,startup-delay-ms property which indicates how long
>>>>>>>>> the system software should wait until attempting to communicate
>>>>>>>>> with the CM firmware. This gives the CM firmware a bit of time
>>>>>>>>> to boot and get ready for communication.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>>> ---
>>>>>>>>> Cc: Bjorn Andersson <andersson@kernel.org>
>>>>>>>>> Cc: Conor Dooley <conor+dt@kernel.org>
>>>>>>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>>>>>>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
>>>>>>>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>>>>>>> Cc: NXP Linux Team <linux-imx@nxp.com>
>>>>>>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>>>>>>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
>>>>>>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>>>>>>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>>>>>>>>> Cc: Shawn Guo <shawnguo@kernel.org>
>>>>>>>>> Cc: devicetree@vger.kernel.org
>>>>>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>>>>>> Cc: linux-remoteproc@vger.kernel.org
>>>>>>>>> ---
>>>>>>>>>     
>>>>>>>>> .../devicetree/bindings/remoteproc/fsl,imx-rproc.yaml        |
>>>>>>>>> 5 +++++
>>>>>>>>>      1 file changed, 5 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git
>>>>>>>>> a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>>>>>>>> index 0c3910f152d1d..c940199ce89df 100644
>>>>>>>>> ---
>>>>>>>>> a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>>>>>>>> +++
>>>>>>>>> b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>>>>>>>> @@ -76,6 +76,11 @@ properties:
>>>>>>>>>            This property is to specify the resource id of the
>>>>>>>>> remote processor in SoC
>>>>>>>>>            which supports SCFW
>>>>>>>>>
>>>>>>>>> +  fsl,startup-delay-ms:
>>>>>>>>> +    default: 0
>>>>>>>>> +    description:
>>>>>>>>> +      CM firmware start up delay.
>>>>>>>>
>>>>>>>> I don't see particular improvements from v2 and no responses
>>>>>>>> addressing
>>>>>>>> my comment:
>>>>>>>> https://lore.kernel.org/all/20221102112451.128110-2-peng.fan@oss.nxp.com/
>>>>>>>
>>>>>>> I wasn't aware of this being submitted before, esp. since I wrote
>>>>>>> the
>>>>>>> binding document from scratch. Which comment is not addressed,
>>>>>>> the type
>>>>>>> ref is not present and the sentence starts with caps, so what is
>>>>>>> missing ?
>>>>>>
>>>>>>
>>>>>> That the property looks like a hacky solution to some SW problem. Why
>>>>>> this delay should be different on different boards?
>>>>>
>>>>> It probably depends more on the CM4 firmware that is being
>>>>> launched. The
>>>>> ones I tested were fine with 50..500ms delay, but the delay was always
>>>>> needed.
>>>>
>>>> If this is for some official remoteproc FW running on M4
>>>
>>> It is not, it is some SDK which can be downloaded from NXP website,
>>> which can then be used to compile the firmware blob. The license is
>>> BSD-3 however, so it is conductive to producing binaries without
>>> matching sources ...
>>>
>>
>> Why can't the SDK be upgraded to provide some kind of hand-shake
>> mechanism, as suggested when I first reviewed this patchset?
> 
> I'd argue because of legacy firmware that is already deployed.
> New firmware builds can, old ones probably cannot be fixed.
> 
> Do you have a suggestion how such a mechanism should look like?
> As far as I can tell, the MX8M SDK stuff looks very similar to the STM32
> Cube stuff, so maybe the mechanism is already there ?

I also stumbled upon this problem [1] and I also wonder why this is
specific to NXP or how other AMP systems like STM32MP1 solve this!?

The problem is that the CM4 firmware first needs to register some IRQs
for the mailbox before it can handle the incoming rpmsg messages. No
matter how the firmware is implemented, there will always be a delay
between starting the app and having the IRQ handlers registered.

Without looking at the details, the proper solution would probably be to
implement a way of signaling that the CM4 firmware is ready for incoming
messages and the rproc driver to wait for this signal before trying to
initialize the rpmsg.

[1]
http://lists.infradead.org/pipermail/linux-arm-kernel/2023-March/822285.html

Re: [PATCH 1/2] dt-bindings: remoteproc: imx_rproc: Document fsl,startup-delay-ms

[Mathieu Poirier]

On Tue, Jul 11, 2023 at 12:23:02AM +0200, Marek Vasut wrote:
> On 7/11/23 00:01, Mathieu Poirier wrote:
> > On Mon, 10 Jul 2023 at 15:53, Marek Vasut <marex@denx.de> wrote:
> > > 
> > > On 7/10/23 22:00, Krzysztof Kozlowski wrote:
> > > > On 10/07/2023 15:46, Marek Vasut wrote:
> > > > > On 7/10/23 14:52, Krzysztof Kozlowski wrote:
> > > > > > On 10/07/2023 11:18, Marek Vasut wrote:
> > > > > > > On 7/10/23 10:12, Krzysztof Kozlowski wrote:
> > > > > > > > On 08/07/2023 01:24, Marek Vasut wrote:
> > > > > > > > > Document fsl,startup-delay-ms property which indicates how long
> > > > > > > > > the system software should wait until attempting to communicate
> > > > > > > > > with the CM firmware. This gives the CM firmware a bit of time
> > > > > > > > > to boot and get ready for communication.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Marek Vasut <marex@denx.de>
> > > > > > > > > ---
> > > > > > > > > Cc: Bjorn Andersson <andersson@kernel.org>
> > > > > > > > > Cc: Conor Dooley <conor+dt@kernel.org>
> > > > > > > > > Cc: Fabio Estevam <festevam@gmail.com>
> > > > > > > > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > > > > > > > > Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> > > > > > > > > Cc: NXP Linux Team <linux-imx@nxp.com>
> > > > > > > > > Cc: Peng Fan <peng.fan@nxp.com>
> > > > > > > > > Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> > > > > > > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > > > > > > Cc: Sascha Hauer <s.hauer@pengutronix.de>
> > > > > > > > > Cc: Shawn Guo <shawnguo@kernel.org>
> > > > > > > > > Cc: devicetree@vger.kernel.org
> > > > > > > > > Cc: linux-arm-kernel@lists.infradead.org
> > > > > > > > > Cc: linux-remoteproc@vger.kernel.org
> > > > > > > > > ---
> > > > > > > > >      .../devicetree/bindings/remoteproc/fsl,imx-rproc.yaml        | 5 +++++
> > > > > > > > >      1 file changed, 5 insertions(+)
> > > > > > > > > 
> > > > > > > > > diff --git a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> > > > > > > > > index 0c3910f152d1d..c940199ce89df 100644
> > > > > > > > > --- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> > > > > > > > > +++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
> > > > > > > > > @@ -76,6 +76,11 @@ properties:
> > > > > > > > >            This property is to specify the resource id of the remote processor in SoC
> > > > > > > > >            which supports SCFW
> > > > > > > > > 
> > > > > > > > > +  fsl,startup-delay-ms:
> > > > > > > > > +    default: 0
> > > > > > > > > +    description:
> > > > > > > > > +      CM firmware start up delay.
> > > > > > > > 
> > > > > > > > I don't see particular improvements from v2 and no responses addressing
> > > > > > > > my comment:
> > > > > > > > https://lore.kernel.org/all/20221102112451.128110-2-peng.fan@oss.nxp.com/
> > > > > > > 
> > > > > > > I wasn't aware of this being submitted before, esp. since I wrote the
> > > > > > > binding document from scratch. Which comment is not addressed, the type
> > > > > > > ref is not present and the sentence starts with caps, so what is missing ?
> > > > > > 
> > > > > > 
> > > > > > That the property looks like a hacky solution to some SW problem. Why
> > > > > > this delay should be different on different boards?
> > > > > 
> > > > > It probably depends more on the CM4 firmware that is being launched. The
> > > > > ones I tested were fine with 50..500ms delay, but the delay was always
> > > > > needed.
> > > > 
> > > > If this is for some official remoteproc FW running on M4
> > > 
> > > It is not, it is some SDK which can be downloaded from NXP website,
> > > which can then be used to compile the firmware blob. The license is
> > > BSD-3 however, so it is conductive to producing binaries without
> > > matching sources ...
> > > 
> > 
> > Why can't the SDK be upgraded to provide some kind of hand-shake
> > mechanism, as suggested when I first reviewed this patchset?
> 
> I'd argue because of legacy firmware that is already deployed.
> New firmware builds can, old ones probably cannot be fixed.
> 
> Do you have a suggestion how such a mechanism should look like?
> As far as I can tell, the MX8M SDK stuff looks very similar to the STM32
> Cube stuff, so maybe the mechanism is already there ?

Either with a flag in the config space of the resource table or implicit
synchronization using the mailbox.  I suggest to have a look at struct
mbox_client where tx_block, knows_txdone and tx_done should be useful.  I'd use
those with a completion in rproc::ops::prepare() or rproc_ops::start().

Re: [PATCH 1/2] dt-bindings: remoteproc: imx_rproc: Document fsl,startup-delay-ms

[Marek Vasut]

On 7/11/23 18:21, Mathieu Poirier wrote:
> On Tue, Jul 11, 2023 at 12:23:02AM +0200, Marek Vasut wrote:
>> On 7/11/23 00:01, Mathieu Poirier wrote:
>>> On Mon, 10 Jul 2023 at 15:53, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>> On 7/10/23 22:00, Krzysztof Kozlowski wrote:
>>>>> On 10/07/2023 15:46, Marek Vasut wrote:
>>>>>> On 7/10/23 14:52, Krzysztof Kozlowski wrote:
>>>>>>> On 10/07/2023 11:18, Marek Vasut wrote:
>>>>>>>> On 7/10/23 10:12, Krzysztof Kozlowski wrote:
>>>>>>>>> On 08/07/2023 01:24, Marek Vasut wrote:
>>>>>>>>>> Document fsl,startup-delay-ms property which indicates how long
>>>>>>>>>> the system software should wait until attempting to communicate
>>>>>>>>>> with the CM firmware. This gives the CM firmware a bit of time
>>>>>>>>>> to boot and get ready for communication.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>>>> ---
>>>>>>>>>> Cc: Bjorn Andersson <andersson@kernel.org>
>>>>>>>>>> Cc: Conor Dooley <conor+dt@kernel.org>
>>>>>>>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>>>>>>>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
>>>>>>>>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>>>>>>>> Cc: NXP Linux Team <linux-imx@nxp.com>
>>>>>>>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>>>>>>>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
>>>>>>>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>>>>>>>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>>>>>>>>>> Cc: Shawn Guo <shawnguo@kernel.org>
>>>>>>>>>> Cc: devicetree@vger.kernel.org
>>>>>>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>>>>>>> Cc: linux-remoteproc@vger.kernel.org
>>>>>>>>>> ---
>>>>>>>>>>       .../devicetree/bindings/remoteproc/fsl,imx-rproc.yaml        | 5 +++++
>>>>>>>>>>       1 file changed, 5 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>>>>>>>>> index 0c3910f152d1d..c940199ce89df 100644
>>>>>>>>>> --- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>>>>>>>>> +++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>>>>>>>>> @@ -76,6 +76,11 @@ properties:
>>>>>>>>>>             This property is to specify the resource id of the remote processor in SoC
>>>>>>>>>>             which supports SCFW
>>>>>>>>>>
>>>>>>>>>> +  fsl,startup-delay-ms:
>>>>>>>>>> +    default: 0
>>>>>>>>>> +    description:
>>>>>>>>>> +      CM firmware start up delay.
>>>>>>>>>
>>>>>>>>> I don't see particular improvements from v2 and no responses addressing
>>>>>>>>> my comment:
>>>>>>>>> https://lore.kernel.org/all/20221102112451.128110-2-peng.fan@oss.nxp.com/
>>>>>>>>
>>>>>>>> I wasn't aware of this being submitted before, esp. since I wrote the
>>>>>>>> binding document from scratch. Which comment is not addressed, the type
>>>>>>>> ref is not present and the sentence starts with caps, so what is missing ?
>>>>>>>
>>>>>>>
>>>>>>> That the property looks like a hacky solution to some SW problem. Why
>>>>>>> this delay should be different on different boards?
>>>>>>
>>>>>> It probably depends more on the CM4 firmware that is being launched. The
>>>>>> ones I tested were fine with 50..500ms delay, but the delay was always
>>>>>> needed.
>>>>>
>>>>> If this is for some official remoteproc FW running on M4
>>>>
>>>> It is not, it is some SDK which can be downloaded from NXP website,
>>>> which can then be used to compile the firmware blob. The license is
>>>> BSD-3 however, so it is conductive to producing binaries without
>>>> matching sources ...
>>>>
>>>
>>> Why can't the SDK be upgraded to provide some kind of hand-shake
>>> mechanism, as suggested when I first reviewed this patchset?
>>
>> I'd argue because of legacy firmware that is already deployed.
>> New firmware builds can, old ones probably cannot be fixed.
>>
>> Do you have a suggestion how such a mechanism should look like?
>> As far as I can tell, the MX8M SDK stuff looks very similar to the STM32
>> Cube stuff, so maybe the mechanism is already there ?
> 
> Either with a flag in the config space of the resource table or implicit
> synchronization using the mailbox.  I suggest to have a look at struct
> mbox_client where tx_block, knows_txdone and tx_done should be useful.  I'd use
> those with a completion in rproc::ops::prepare() or rproc_ops::start().

I added the following to the CM7 BSP from NXP, which removes the need 
for the extra delay. I believe that is also the proper fix. Whether NXP 
will pick it up in some form, is up to NXP.

This whole startup-delay patch is now unnecessary for me, i.e. I stop here.

"
Run RPMSG init with IRQs globally disabled

The rpmsg_lite_remote_init() function runs in parallel with Linux side
rpmsg_probe()->virtqueue_notify()->rproc_virtio_notify() which raises an
IRQ on CM7 side. Unless IRQs are disabled during rpmsg_lite_remote_init()
time, it is possible the kick from CA53 side would be received and end up
in MU_M7_IRQHandler()->env_isr()->virtqueue_notification() for virtqueue
which is not yet fully initialized. Such IRQ would then be discarded or
mishandled, and rpmsg_lite_wait_for_link_up() would never complete. The
firmware would then fail to communicate with CA53 side.

Fix this by running the RPMSG initialization with global IRQs off, which
delays the reception of IRQ from CA53 side until after the virtqueues are
fully and properly initialized, and the IRQ can be properly handled.

diff --git 
a/boards/evkmimx8mn/multicore_examples/rpmsg_lite_str_echo_rtos/main_remote.c 
b/boards/evkmimx8mn/multicore_examples/rpmsg_lite_str_echo_rtos/main_remote.c
index 655287c..936822e 100644
--- 
a/boards/evkmimx8mn/multicore_examples/rpmsg_lite_str_echo_rtos/main_remote.c
+++ 
b/boards/evkmimx8mn/multicore_examples/rpmsg_lite_str_echo_rtos/main_remote.c
@@ -87,6 +87,7 @@ void app_task(void *param)
      /* Print the initial banner */
      PRINTF("\r\nRPMSG String Echo FreeRTOS RTOS API Demo...\r\n");

+    __disable_irq();
  #ifdef MCMGR_USED
      uint32_t startupData;

@@ -100,6 +101,7 @@ void app_task(void *param)
  #else
      my_rpmsg = rpmsg_lite_remote_init((void *)RPMSG_LITE_SHMEM_BASE, 
RPMSG_LITE_LINK_ID, RL_NO_FLAGS);
  #endif /* MCMGR_USED */
+    __enable_irq();

      rpmsg_lite_wait_for_link_up(my_rpmsg, RL_BLOCK);
"

Re: [PATCH 1/2] dt-bindings: remoteproc: imx_rproc: Document fsl,startup-delay-ms

[Krzysztof Kozlowski]

On 10/07/2023 23:52, Marek Vasut wrote:
> On 7/10/23 22:00, Krzysztof Kozlowski wrote:
>> On 10/07/2023 15:46, Marek Vasut wrote:
>>> On 7/10/23 14:52, Krzysztof Kozlowski wrote:
>>>> On 10/07/2023 11:18, Marek Vasut wrote:
>>>>> On 7/10/23 10:12, Krzysztof Kozlowski wrote:
>>>>>> On 08/07/2023 01:24, Marek Vasut wrote:
>>>>>>> Document fsl,startup-delay-ms property which indicates how long
>>>>>>> the system software should wait until attempting to communicate
>>>>>>> with the CM firmware. This gives the CM firmware a bit of time
>>>>>>> to boot and get ready for communication.
>>>>>>>
>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>> ---
>>>>>>> Cc: Bjorn Andersson <andersson@kernel.org>
>>>>>>> Cc: Conor Dooley <conor+dt@kernel.org>
>>>>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>>>>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
>>>>>>> Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
>>>>>>> Cc: NXP Linux Team <linux-imx@nxp.com>
>>>>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>>>>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
>>>>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>>>>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>>>>>>> Cc: Shawn Guo <shawnguo@kernel.org>
>>>>>>> Cc: devicetree@vger.kernel.org
>>>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>>>> Cc: linux-remoteproc@vger.kernel.org
>>>>>>> ---
>>>>>>>     .../devicetree/bindings/remoteproc/fsl,imx-rproc.yaml        | 5 +++++
>>>>>>>     1 file changed, 5 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>>>>>> index 0c3910f152d1d..c940199ce89df 100644
>>>>>>> --- a/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>>>>>> +++ b/Documentation/devicetree/bindings/remoteproc/fsl,imx-rproc.yaml
>>>>>>> @@ -76,6 +76,11 @@ properties:
>>>>>>>           This property is to specify the resource id of the remote processor in SoC
>>>>>>>           which supports SCFW
>>>>>>>     
>>>>>>> +  fsl,startup-delay-ms:
>>>>>>> +    default: 0
>>>>>>> +    description:
>>>>>>> +      CM firmware start up delay.
>>>>>>
>>>>>> I don't see particular improvements from v2 and no responses addressing
>>>>>> my comment:
>>>>>> https://lore.kernel.org/all/20221102112451.128110-2-peng.fan@oss.nxp.com/
>>>>>
>>>>> I wasn't aware of this being submitted before, esp. since I wrote the
>>>>> binding document from scratch. Which comment is not addressed, the type
>>>>> ref is not present and the sentence starts with caps, so what is missing ?
>>>>
>>>>
>>>> That the property looks like a hacky solution to some SW problem. Why
>>>> this delay should be different on different boards?
>>>
>>> It probably depends more on the CM4 firmware that is being launched. The
>>> ones I tested were fine with 50..500ms delay, but the delay was always
>>> needed.
>>
>> If this is for some official remoteproc FW running on M4
> 
> It is not, it is some SDK which can be downloaded from NXP website, 
> which can then be used to compile the firmware blob. The license is 
> BSD-3 however, so it is conductive to producing binaries without 
> matching sources ...
> 
>> , then probably
>> this could be implied by compatible. Otherwise, if this depends on
>> actual M4 firmware which can totally vary between each board of the same
>> type (I can run my own FW on M4, right?
> 
> Yeah
> 
>> ), then it is not suitable DT
>> property. How it would even look like? You add here 500 ms for all known
>> firmwares and then someone comes with FW requiring delay of 600 ms.
> 
> I would say, some sane default and if some firmware is specifically 
> weird, just up the delay. It is better than no firmware working at all.
> 
> Do you have a better hint how to deal with this ?

Put some working value in the driver. If this does not help, complain to
NXP about their SDK firmware. I know how that would work - NXP does not
really care about customers and open-source - but that should not be
really an excuse for us.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: remoteproc: imx_rproc: Document fsl,startup-delay-ms

[Marek Vasut]

On 7/11/23 08:02, Krzysztof Kozlowski wrote:

[...]

>>> ), then it is not suitable DT
>>> property. How it would even look like? You add here 500 ms for all known
>>> firmwares and then someone comes with FW requiring delay of 600 ms.
>>
>> I would say, some sane default and if some firmware is specifically
>> weird, just up the delay. It is better than no firmware working at all.
>>
>> Do you have a better hint how to deal with this ?
> 
> Put some working value in the driver. If this does not help, complain to
> NXP about their SDK firmware.I know how that would work - NXP does not
> really care about customers and open-source - but that should not be
> really an excuse for us.

Yes, so, I'll just stick half a second delay into the driver for now.