Discussion about imx_dsp_rproc FW_READY policy

Discussion about imx_dsp_rproc FW_READY policy

[Daniel Baluta]

Hello all,

I want to start this thread in order to clarify what assumptions a
remoteproc driver is able to make
about a firmware loaded on a remote processor.

Discussion is generated by this thread:

[1] https://www.spinics.net/lists/kernel/msg4857733.html

imx_dsp_rproc driver assumes that the remote firmware will send a
notification once it has booted up and this is the default behavior.

This doesn't work well with Zephyr samples which do not send such notification!

I want to get an agreement for the following questions:

1) What should be the default behavior of a remote proc driver?

In my opinion it should not make any assumption about the remote part.
Thus by default the driver should not wait for any message!

2) How can we support various "protocols" of starting up. Eg (wait for
firmware / no wait for firmware).

In patch [1] Iulia proposed to add a flag that will select the correct
behavior. As per Mathieu's comments this doesn't
scale up, for next flags.

How can we solve this? In my opinion using a kernel module parameter
OR a device tree property should be enough.

What do you think?

thanks,
Daniel.

Re: Discussion about imx_dsp_rproc FW_READY policy

[Daniel Baluta]

Hi Mathieu, S.J,

Any comments about this?

I feel that the Linux kernel driver shouldn't enforce the policy of
waiting for a reply or confirmation that the firmware booted.

The Linux kernel driver should offer a mechanism for checking this and
the policy should be set either in userspace or via dts.

First option would be to have an ioctl but we need to also mirror this
in the sysfs interface. Second option would be to have a property in
the dts.

What do you think?

We are trying to fix this in the firmware side:

https://github.com/zephyrproject-rtos/zephyr/pull/61709

but we are getting some setbacks there too.


Daniel.

On Wed, Jul 26, 2023 at 11:26 AM Daniel Baluta <daniel.baluta@gmail.com> wrote:
>
> Hello all,
>
> I want to start this thread in order to clarify what assumptions a
> remoteproc driver is able to make
> about a firmware loaded on a remote processor.
>
> Discussion is generated by this thread:
>
> [1] https://www.spinics.net/lists/kernel/msg4857733.html
>
> imx_dsp_rproc driver assumes that the remote firmware will send a
> notification once it has booted up and this is the default behavior.
>
> This doesn't work well with Zephyr samples which do not send such notification!
>
> I want to get an agreement for the following questions:
>
> 1) What should be the default behavior of a remote proc driver?
>
> In my opinion it should not make any assumption about the remote part.
> Thus by default the driver should not wait for any message!
>
> 2) How can we support various "protocols" of starting up. Eg (wait for
> firmware / no wait for firmware).
>
> In patch [1] Iulia proposed to add a flag that will select the correct
> behavior. As per Mathieu's comments this doesn't
> scale up, for next flags.
>
> How can we solve this? In my opinion using a kernel module parameter
> OR a device tree property should be enough.
>
> What do you think?
>
> thanks,
> Daniel.

Re: Discussion about imx_dsp_rproc FW_READY policy

[Mathieu Poirier]

On Tue, Aug 22, 2023 at 05:12:55PM +0300, Daniel Baluta wrote:
> Hi Mathieu, S.J,
> 
> Any comments about this?
>

All the questions raised below have been answered in my exchange with Iuliana,
but I will complement herein. 

> I feel that the Linux kernel driver shouldn't enforce the policy of
> waiting for a reply or confirmation that the firmware booted.
> 

The protocol enacted between a remote processor and the host is very platform
dependent.  The need to wait for a reply in the IMX DSP driver predates my time
as maintainer of this subsystem and as such can't comment on the reasons it
was introduced.  That said I am very disappointed by the complete silence from
S.J and the rest of the people on the linux-imx mailing list regarding this
issue.

> The Linux kernel driver should offer a mechanism for checking this and
> the policy should be set either in userspace or via dts.
>

This has already been discussed.  Adapting the Linux kernel driver for all the
protocols that can be enacted by remote processors doesn't scale.  This is
something that needs to happen in the FW and Iuliana's approach in [1] is
appropriate.

I think the pushback is caused by a lack of understanding of the problem by the
maintainers.

[1]. https://github.com/zephyrproject-rtos/zephyr/pull/61709

> First option would be to have an ioctl but we need to also mirror this
> in the sysfs interface. Second option would be to have a property in
> the dts.
> 
> What do you think?
> 
> We are trying to fix this in the firmware side:
> 
> https://github.com/zephyrproject-rtos/zephyr/pull/61709
> 
> but we are getting some setbacks there too.
> 
> 
> Daniel.
> 
> On Wed, Jul 26, 2023 at 11:26 AM Daniel Baluta <daniel.baluta@gmail.com> wrote:
> >
> > Hello all,
> >
> > I want to start this thread in order to clarify what assumptions a
> > remoteproc driver is able to make
> > about a firmware loaded on a remote processor.
> >
> > Discussion is generated by this thread:
> >
> > [1] https://www.spinics.net/lists/kernel/msg4857733.html
> >
> > imx_dsp_rproc driver assumes that the remote firmware will send a
> > notification once it has booted up and this is the default behavior.
> >
> > This doesn't work well with Zephyr samples which do not send such notification!
> >
> > I want to get an agreement for the following questions:
> >
> > 1) What should be the default behavior of a remote proc driver?
> >
> > In my opinion it should not make any assumption about the remote part.
> > Thus by default the driver should not wait for any message!
> >
> > 2) How can we support various "protocols" of starting up. Eg (wait for
> > firmware / no wait for firmware).
> >
> > In patch [1] Iulia proposed to add a flag that will select the correct
> > behavior. As per Mathieu's comments this doesn't
> > scale up, for next flags.
> >
> > How can we solve this? In my opinion using a kernel module parameter
> > OR a device tree property should be enough.
> >
> > What do you think?
> >
> > thanks,
> > Daniel.

Re: Discussion about imx_dsp_rproc FW_READY policy

[S.J. Wang]

> > Hi Mathieu, S.J,
> >
> > Any comments about this?
> >
> 
> All the questions raised below have been answered in my exchange with
> Iuliana, but I will complement herein.
> 
> > I feel that the Linux kernel driver shouldn't enforce the policy of
> > waiting for a reply or confirmation that the firmware booted.
> >
> 
> The protocol enacted between a remote processor and the host is very
> platform dependent.  The need to wait for a reply in the IMX DSP driver
> predates my time as maintainer of this subsystem and as such can't
> comment on the reasons it was introduced.  That said I am very disappointed
> by the complete silence from S.J and the rest of the people on the linux-imx
> mailing list regarding this issue.

Sorry for silence, because I don't know which is best solution.

The reason why wait for a reply in IMX DSP driver is to make sure the DSP
boot successfully and the firmware is correct. 

So for this issue my first initial intend is to add the reply in zephyr firmware, 
because as a driver owner I want the driver can be applied to all use case
without any module params. 

My opinion has obvious personal tendency😊

Best regards
Shengjiu Wang

> 
> > The Linux kernel driver should offer a mechanism for checking this and
> > the policy should be set either in userspace or via dts.
> >
> 
> This has already been discussed.  Adapting the Linux kernel driver for all the
> protocols that can be enacted by remote processors doesn't scale.  This is
> something that needs to happen in the FW and Iuliana's approach in [1] is
> appropriate.
> 
> I think the pushback is caused by a lack of understanding of the problem by
> the maintainers.
> 
> [1].
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub
> .com%2Fzephyrproject-
> rtos%2Fzephyr%2Fpull%2F61709&data=05%7C01%7Cshengjiu.wang%40nxp.c
> om%7Cfbb9fc0431484c112b4008dba3f418da%7C686ea1d3bc2b4c6fa92cd99
> c5c301635%7C0%7C0%7C638284041009229312%7CUnknown%7CTWFpbGZs
> b3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> 0%3D%7C3000%7C%7C%7C&sdata=37T3rJat0eCowxq3ekM%2BsEPdiY5Jkc1Zr
> VgjkhA66yw%3D&reserved=0
> 
> > First option would be to have an ioctl but we need to also mirror this
> > in the sysfs interface. Second option would be to have a property in
> > the dts.
> >
> > What do you think?
> >
> > We are trying to fix this in the firmware side:
> >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgith
> > ub.com%2Fzephyrproject-
> rtos%2Fzephyr%2Fpull%2F61709&data=05%7C01%7Cshe
> >
> ngjiu.wang%40nxp.com%7Cfbb9fc0431484c112b4008dba3f418da%7C686ea1
> d3bc2b
> >
> 4c6fa92cd99c5c301635%7C0%7C0%7C638284041009229312%7CUnknown%7
> CTWFpbGZs
> >
> b3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn
> 0%3D
> > %7C3000%7C%7C%7C&sdata=37T3rJat0eCowxq3ekM%2BsEPdiY5Jkc1ZrVgj
> khA66yw%3
> > D&reserved=0
> >
> > but we are getting some setbacks there too.
> >
> >
> > Daniel.
> >
> > On Wed, Jul 26, 2023 at 11:26 AM Daniel Baluta <daniel.baluta@gmail.com>
> wrote:
> > >
> > > Hello all,
> > >
> > > I want to start this thread in order to clarify what assumptions a
> > > remoteproc driver is able to make about a firmware loaded on a
> > > remote processor.
> > >
> > > Discussion is generated by this thread:
> > >
> > > [1]
> > >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
> > >
> w.spinics.net%2Flists%2Fkernel%2Fmsg4857733.html&data=05%7C01%7Cshe
> n
> > >
> gjiu.wang%40nxp.com%7Cfbb9fc0431484c112b4008dba3f418da%7C686ea1d
> 3bc2
> > >
> b4c6fa92cd99c5c301635%7C0%7C0%7C638284041009229312%7CUnknown%
> 7CTWFpb
> > >
> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> M
> > >
> n0%3D%7C3000%7C%7C%7C&sdata=V%2FsdlfFUtcnmYPSXPKMWtL7vLeZ%2F
> 3ANjHeZi
> > > uipLeK4%3D&reserved=0
> > >
> > > imx_dsp_rproc driver assumes that the remote firmware will send a
> > > notification once it has booted up and this is the default behavior.
> > >
> > > This doesn't work well with Zephyr samples which do not send such
> notification!
> > >
> > > I want to get an agreement for the following questions:
> > >
> > > 1) What should be the default behavior of a remote proc driver?
> > >
> > > In my opinion it should not make any assumption about the remote part.
> > > Thus by default the driver should not wait for any message!
> > >
> > > 2) How can we support various "protocols" of starting up. Eg (wait
> > > for firmware / no wait for firmware).
> > >
> > > In patch [1] Iulia proposed to add a flag that will select the
> > > correct behavior. As per Mathieu's comments this doesn't scale up,
> > > for next flags.
> > >
> > > How can we solve this? In my opinion using a kernel module parameter
> > > OR a device tree property should be enough.
> > >
> > > What do you think?
> > >
> > > thanks,
> > > Daniel.

Re: Discussion about imx_dsp_rproc FW_READY policy

[Iuliana Prodan]

On 8/23/2023 7:14 PM, Mathieu Poirier wrote:
> On Tue, Aug 22, 2023 at 05:12:55PM +0300, Daniel Baluta wrote:
>> Hi Mathieu, S.J,
>>
>> Any comments about this?
>>
> All the questions raised below have been answered in my exchange with Iuliana,
> but I will complement herein.
>
>> I feel that the Linux kernel driver shouldn't enforce the policy of
>> waiting for a reply or confirmation that the firmware booted.
>>
> The protocol enacted between a remote processor and the host is very platform
> dependent.  The need to wait for a reply in the IMX DSP driver predates my time
> as maintainer of this subsystem and as such can't comment on the reasons it
> was introduced.  That said I am very disappointed by the complete silence from
> S.J and the rest of the people on the linux-imx mailing list regarding this
> issue.
>
>> The Linux kernel driver should offer a mechanism for checking this and
>> the policy should be set either in userspace or via dts.
>>
> This has already been discussed.  Adapting the Linux kernel driver for all the
> protocols that can be enacted by remote processors doesn't scale.  This is
> something that needs to happen in the FW and Iuliana's approach in [1] is
> appropriate.

Thank you, Mathieu!
I'll try to fix it in fw, continuing with [1] PR.

Iulia

>
> I think the pushback is caused by a lack of understanding of the problem by the
> maintainers.
>
> [1]. https://github.com/zephyrproject-rtos/zephyr/pull/61709
>
>> First option would be to have an ioctl but we need to also mirror this
>> in the sysfs interface. Second option would be to have a property in
>> the dts.
>>
>> What do you think?
>>
>> We are trying to fix this in the firmware side:
>>
>> https://github.com/zephyrproject-rtos/zephyr/pull/61709
>>
>> but we are getting some setbacks there too.
>>
>>
>> Daniel.
>>
>> On Wed, Jul 26, 2023 at 11:26 AM Daniel Baluta <daniel.baluta@gmail.com> wrote:
>>> Hello all,
>>>
>>> I want to start this thread in order to clarify what assumptions a
>>> remoteproc driver is able to make
>>> about a firmware loaded on a remote processor.
>>>
>>> Discussion is generated by this thread:
>>>
>>> [1] https://www.spinics.net/lists/kernel/msg4857733.html
>>>
>>> imx_dsp_rproc driver assumes that the remote firmware will send a
>>> notification once it has booted up and this is the default behavior.
>>>
>>> This doesn't work well with Zephyr samples which do not send such notification!
>>>
>>> I want to get an agreement for the following questions:
>>>
>>> 1) What should be the default behavior of a remote proc driver?
>>>
>>> In my opinion it should not make any assumption about the remote part.
>>> Thus by default the driver should not wait for any message!
>>>
>>> 2) How can we support various "protocols" of starting up. Eg (wait for
>>> firmware / no wait for firmware).
>>>
>>> In patch [1] Iulia proposed to add a flag that will select the correct
>>> behavior. As per Mathieu's comments this doesn't
>>> scale up, for next flags.
>>>
>>> How can we solve this? In my opinion using a kernel module parameter
>>> OR a device tree property should be enough.
>>>
>>> What do you think?
>>>
>>> thanks,
>>> Daniel.