* [PATCH] rpmsg: Solve circular dependencies involving RPMSG_VIRTIO
@ 2017-06-27 6:43 Bjorn Andersson
2017-06-27 7:29 ` Arnd Bergmann
2017-06-27 19:08 ` Suman Anna
0 siblings, 2 replies; 7+ messages in thread
From: Bjorn Andersson @ 2017-06-27 6:43 UTC (permalink / raw)
To: Ohad Ben-Cohen, Bjorn Andersson, Peter Griffin,
Mauro Carvalho Chehab, Hugues Fruchet, Loic Pallardy
Cc: Arnd Bergmann, linux-media, linux-kernel, linux-remoteproc
While it's very common to use RPMSG for communicating with firmware
running on these remoteprocs there is no functional dependency on RPMSG.
As such RPMSG should be selected by the system integrator and not
automatically by the remoteproc drivers.
This does solve problems reported with circular Kconfig dependencies for
Davinci and Keystone remoteproc drivers.
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
drivers/media/platform/Kconfig | 2 +-
drivers/remoteproc/Kconfig | 4 ----
drivers/rpmsg/Kconfig | 20 +++++++++-----------
3 files changed, 10 insertions(+), 16 deletions(-)
diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 1313cd533436..cb2f31cd0088 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -382,10 +382,10 @@ config VIDEO_STI_DELTA_DRIVER
tristate
depends on VIDEO_STI_DELTA
depends on VIDEO_STI_DELTA_MJPEG
+ depends on RPMSG
default VIDEO_STI_DELTA_MJPEG
select VIDEOBUF2_DMA_CONTIG
select V4L2_MEM2MEM_DEV
- select RPMSG
endif # VIDEO_STI_DELTA
diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index b950e6cd4ba2..3b16f422d30c 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -21,7 +21,6 @@ config OMAP_REMOTEPROC
depends on REMOTEPROC
select MAILBOX
select OMAP2PLUS_MBOX
- select RPMSG_VIRTIO
help
Say y here to support OMAP's remote processors (dual M3
and DSP on OMAP4) via the remote processor framework.
@@ -53,7 +52,6 @@ config DA8XX_REMOTEPROC
depends on ARCH_DAVINCI_DA8XX
depends on REMOTEPROC
depends on DMA_CMA
- select RPMSG_VIRTIO
help
Say y here to support DA8xx/OMAP-L13x remote processors via the
remote processor framework.
@@ -76,7 +74,6 @@ config KEYSTONE_REMOTEPROC
depends on ARCH_KEYSTONE
depends on RESET_CONTROLLER
depends on REMOTEPROC
- select RPMSG_VIRTIO
help
Say Y here here to support Keystone remote processors (DSP)
via the remote processor framework.
@@ -133,7 +130,6 @@ config ST_REMOTEPROC
depends on REMOTEPROC
select MAILBOX
select STI_MBOX
- select RPMSG_VIRTIO
help
Say y here to support ST's adjunct processors via the remote
processor framework.
diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
index 2a5d2b446de2..46f3f2431d68 100644
--- a/drivers/rpmsg/Kconfig
+++ b/drivers/rpmsg/Kconfig
@@ -1,8 +1,5 @@
-menu "Rpmsg drivers"
-
-# RPMSG always gets selected by whoever wants it
-config RPMSG
- tristate
+menuconfig RPMSG
+ tristate "Rpmsg drivers"
config RPMSG_CHAR
tristate "RPMSG device interface"
@@ -15,7 +12,7 @@ config RPMSG_CHAR
config RPMSG_QCOM_GLINK_RPM
tristate "Qualcomm RPM Glink driver"
- select RPMSG
+ depends on RPMSG
depends on HAS_IOMEM
depends on MAILBOX
help
@@ -26,16 +23,17 @@ config RPMSG_QCOM_GLINK_RPM
config RPMSG_QCOM_SMD
tristate "Qualcomm Shared Memory Driver (SMD)"
depends on QCOM_SMEM
- select RPMSG
+ depends on RPMSG
help
Say y here to enable support for the Qualcomm Shared Memory Driver
providing communication channels to remote processors in Qualcomm
platforms.
config RPMSG_VIRTIO
- tristate
- select RPMSG
+ tristate "Virtio remote processor messaging driver (RPMSG)"
+ depends on RPMSG
select VIRTIO
select VIRTUALIZATION
-
-endmenu
+ help
+ Say y here to enable support for the Virtio remote processor
+ messaging protocol (RPMSG).
--
2.12.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] rpmsg: Solve circular dependencies involving RPMSG_VIRTIO
2017-06-27 6:43 [PATCH] rpmsg: Solve circular dependencies involving RPMSG_VIRTIO Bjorn Andersson
@ 2017-06-27 7:29 ` Arnd Bergmann
2017-06-27 19:08 ` Suman Anna
1 sibling, 0 replies; 7+ messages in thread
From: Arnd Bergmann @ 2017-06-27 7:29 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Ohad Ben-Cohen, Peter Griffin, Mauro Carvalho Chehab,
Hugues Fruchet, Loic Pallardy, Linux Media Mailing List,
Linux Kernel Mailing List, linux-remoteproc
On Tue, Jun 27, 2017 at 8:43 AM, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
> While it's very common to use RPMSG for communicating with firmware
> running on these remoteprocs there is no functional dependency on RPMSG.
> As such RPMSG should be selected by the system integrator and not
> automatically by the remoteproc drivers.
>
> This does solve problems reported with circular Kconfig dependencies for
> Davinci and Keystone remoteproc drivers.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
Looks good to me,
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rpmsg: Solve circular dependencies involving RPMSG_VIRTIO
2017-06-27 6:43 [PATCH] rpmsg: Solve circular dependencies involving RPMSG_VIRTIO Bjorn Andersson
@ 2017-06-27 19:08 ` Suman Anna
2017-06-27 19:08 ` Suman Anna
1 sibling, 0 replies; 7+ messages in thread
From: Suman Anna @ 2017-06-27 19:08 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen, Peter Griffin,
Mauro Carvalho Chehab, Hugues Fruchet, Loic Pallardy
Cc: Arnd Bergmann, linux-media, linux-kernel, linux-remoteproc
Hi Bjorn,
Thanks for the patch.
On 06/27/2017 01:43 AM, Bjorn Andersson wrote:
> While it's very common to use RPMSG for communicating with firmware
> running on these remoteprocs there is no functional dependency on RPMSG.
This is not entirely accurate though. RPMSG is the IPC transport on
these remoteprocs, you seem to suggest that there are alternatives for
these remoteprocs. Without RPMSG, you can boot, but you will not be able
to talk to the remoteprocs, so I would call it a functional dependency.
> As such RPMSG should be selected by the system integrator and not
> automatically by the remoteproc drivers.
>
> This does solve problems reported with circular Kconfig dependencies for
> Davinci and Keystone remoteproc drivers.
The Keystone one issue shows up on linux-next (and not on 4.12-rcX) due
to the differing options on RESET_CONTROLLER on VIDEO_QCOM_VENUS
(through QCOM_SCOM). This can also be resolved by changing the depends
on RESET_CONTROLLER to a select RESET_CONTROLLER or dropping the line.
The davinci one is tricky though, as I did change it from using a select
to a depends on dependency, and obviously ppc64_defconfig is something
that I would not check.
This patch definitely resolves both issues, but it is not obvious that
someone would also have to enable RPMSG_VIRTIO to have these remoteprocs
useful when looking at either of the menuconfig help.
regards
Suman
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> drivers/media/platform/Kconfig | 2 +-
> drivers/remoteproc/Kconfig | 4 ----
> drivers/rpmsg/Kconfig | 20 +++++++++-----------
> 3 files changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 1313cd533436..cb2f31cd0088 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -382,10 +382,10 @@ config VIDEO_STI_DELTA_DRIVER
> tristate
> depends on VIDEO_STI_DELTA
> depends on VIDEO_STI_DELTA_MJPEG
> + depends on RPMSG
> default VIDEO_STI_DELTA_MJPEG
> select VIDEOBUF2_DMA_CONTIG
> select V4L2_MEM2MEM_DEV
> - select RPMSG
>
> endif # VIDEO_STI_DELTA
>
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index b950e6cd4ba2..3b16f422d30c 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -21,7 +21,6 @@ config OMAP_REMOTEPROC
> depends on REMOTEPROC
> select MAILBOX
> select OMAP2PLUS_MBOX
> - select RPMSG_VIRTIO
> help
> Say y here to support OMAP's remote processors (dual M3
> and DSP on OMAP4) via the remote processor framework.
> @@ -53,7 +52,6 @@ config DA8XX_REMOTEPROC
> depends on ARCH_DAVINCI_DA8XX
> depends on REMOTEPROC
> depends on DMA_CMA
> - select RPMSG_VIRTIO
> help
> Say y here to support DA8xx/OMAP-L13x remote processors via the
> remote processor framework.
> @@ -76,7 +74,6 @@ config KEYSTONE_REMOTEPROC
> depends on ARCH_KEYSTONE
> depends on RESET_CONTROLLER
> depends on REMOTEPROC
> - select RPMSG_VIRTIO
> help
> Say Y here here to support Keystone remote processors (DSP)
> via the remote processor framework.
> @@ -133,7 +130,6 @@ config ST_REMOTEPROC
> depends on REMOTEPROC
> select MAILBOX
> select STI_MBOX
> - select RPMSG_VIRTIO
> help
> Say y here to support ST's adjunct processors via the remote
> processor framework.
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index 2a5d2b446de2..46f3f2431d68 100644
> --- a/drivers/rpmsg/Kconfig
> +++ b/drivers/rpmsg/Kconfig
> @@ -1,8 +1,5 @@
> -menu "Rpmsg drivers"
> -
> -# RPMSG always gets selected by whoever wants it
> -config RPMSG
> - tristate
> +menuconfig RPMSG
> + tristate "Rpmsg drivers"
>
> config RPMSG_CHAR
> tristate "RPMSG device interface"
> @@ -15,7 +12,7 @@ config RPMSG_CHAR
>
> config RPMSG_QCOM_GLINK_RPM
> tristate "Qualcomm RPM Glink driver"
> - select RPMSG
> + depends on RPMSG
> depends on HAS_IOMEM
> depends on MAILBOX
> help
> @@ -26,16 +23,17 @@ config RPMSG_QCOM_GLINK_RPM
> config RPMSG_QCOM_SMD
> tristate "Qualcomm Shared Memory Driver (SMD)"
> depends on QCOM_SMEM
> - select RPMSG
> + depends on RPMSG
> help
> Say y here to enable support for the Qualcomm Shared Memory Driver
> providing communication channels to remote processors in Qualcomm
> platforms.
>
> config RPMSG_VIRTIO
> - tristate
> - select RPMSG
> + tristate "Virtio remote processor messaging driver (RPMSG)"
> + depends on RPMSG
> select VIRTIO
> select VIRTUALIZATION
> -
> -endmenu
> + help
> + Say y here to enable support for the Virtio remote processor
> + messaging protocol (RPMSG).
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rpmsg: Solve circular dependencies involving RPMSG_VIRTIO
@ 2017-06-27 19:08 ` Suman Anna
0 siblings, 0 replies; 7+ messages in thread
From: Suman Anna @ 2017-06-27 19:08 UTC (permalink / raw)
To: Bjorn Andersson, Ohad Ben-Cohen, Peter Griffin,
Mauro Carvalho Chehab, Hugues Fruchet, Loic Pallardy
Cc: Arnd Bergmann, linux-media, linux-kernel, linux-remoteproc,
Arnd Bergmann
Hi Bjorn,
Thanks for the patch.
On 06/27/2017 01:43 AM, Bjorn Andersson wrote:
> While it's very common to use RPMSG for communicating with firmware
> running on these remoteprocs there is no functional dependency on RPMSG.
This is not entirely accurate though. RPMSG is the IPC transport on
these remoteprocs, you seem to suggest that there are alternatives for
these remoteprocs. Without RPMSG, you can boot, but you will not be able
to talk to the remoteprocs, so I would call it a functional dependency.
> As such RPMSG should be selected by the system integrator and not
> automatically by the remoteproc drivers.
>
> This does solve problems reported with circular Kconfig dependencies for
> Davinci and Keystone remoteproc drivers.
The Keystone one issue shows up on linux-next (and not on 4.12-rcX) due
to the differing options on RESET_CONTROLLER on VIDEO_QCOM_VENUS
(through QCOM_SCOM). This can also be resolved by changing the depends
on RESET_CONTROLLER to a select RESET_CONTROLLER or dropping the line.
The davinci one is tricky though, as I did change it from using a select
to a depends on dependency, and obviously ppc64_defconfig is something
that I would not check.
This patch definitely resolves both issues, but it is not obvious that
someone would also have to enable RPMSG_VIRTIO to have these remoteprocs
useful when looking at either of the menuconfig help.
regards
Suman
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
> drivers/media/platform/Kconfig | 2 +-
> drivers/remoteproc/Kconfig | 4 ----
> drivers/rpmsg/Kconfig | 20 +++++++++-----------
> 3 files changed, 10 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 1313cd533436..cb2f31cd0088 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -382,10 +382,10 @@ config VIDEO_STI_DELTA_DRIVER
> tristate
> depends on VIDEO_STI_DELTA
> depends on VIDEO_STI_DELTA_MJPEG
> + depends on RPMSG
> default VIDEO_STI_DELTA_MJPEG
> select VIDEOBUF2_DMA_CONTIG
> select V4L2_MEM2MEM_DEV
> - select RPMSG
>
> endif # VIDEO_STI_DELTA
>
> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> index b950e6cd4ba2..3b16f422d30c 100644
> --- a/drivers/remoteproc/Kconfig
> +++ b/drivers/remoteproc/Kconfig
> @@ -21,7 +21,6 @@ config OMAP_REMOTEPROC
> depends on REMOTEPROC
> select MAILBOX
> select OMAP2PLUS_MBOX
> - select RPMSG_VIRTIO
> help
> Say y here to support OMAP's remote processors (dual M3
> and DSP on OMAP4) via the remote processor framework.
> @@ -53,7 +52,6 @@ config DA8XX_REMOTEPROC
> depends on ARCH_DAVINCI_DA8XX
> depends on REMOTEPROC
> depends on DMA_CMA
> - select RPMSG_VIRTIO
> help
> Say y here to support DA8xx/OMAP-L13x remote processors via the
> remote processor framework.
> @@ -76,7 +74,6 @@ config KEYSTONE_REMOTEPROC
> depends on ARCH_KEYSTONE
> depends on RESET_CONTROLLER
> depends on REMOTEPROC
> - select RPMSG_VIRTIO
> help
> Say Y here here to support Keystone remote processors (DSP)
> via the remote processor framework.
> @@ -133,7 +130,6 @@ config ST_REMOTEPROC
> depends on REMOTEPROC
> select MAILBOX
> select STI_MBOX
> - select RPMSG_VIRTIO
> help
> Say y here to support ST's adjunct processors via the remote
> processor framework.
> diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> index 2a5d2b446de2..46f3f2431d68 100644
> --- a/drivers/rpmsg/Kconfig
> +++ b/drivers/rpmsg/Kconfig
> @@ -1,8 +1,5 @@
> -menu "Rpmsg drivers"
> -
> -# RPMSG always gets selected by whoever wants it
> -config RPMSG
> - tristate
> +menuconfig RPMSG
> + tristate "Rpmsg drivers"
>
> config RPMSG_CHAR
> tristate "RPMSG device interface"
> @@ -15,7 +12,7 @@ config RPMSG_CHAR
>
> config RPMSG_QCOM_GLINK_RPM
> tristate "Qualcomm RPM Glink driver"
> - select RPMSG
> + depends on RPMSG
> depends on HAS_IOMEM
> depends on MAILBOX
> help
> @@ -26,16 +23,17 @@ config RPMSG_QCOM_GLINK_RPM
> config RPMSG_QCOM_SMD
> tristate "Qualcomm Shared Memory Driver (SMD)"
> depends on QCOM_SMEM
> - select RPMSG
> + depends on RPMSG
> help
> Say y here to enable support for the Qualcomm Shared Memory Driver
> providing communication channels to remote processors in Qualcomm
> platforms.
>
> config RPMSG_VIRTIO
> - tristate
> - select RPMSG
> + tristate "Virtio remote processor messaging driver (RPMSG)"
> + depends on RPMSG
> select VIRTIO
> select VIRTUALIZATION
> -
> -endmenu
> + help
> + Say y here to enable support for the Virtio remote processor
> + messaging protocol (RPMSG).
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rpmsg: Solve circular dependencies involving RPMSG_VIRTIO
2017-06-27 19:08 ` Suman Anna
(?)
@ 2017-06-27 21:47 ` Bjorn Andersson
2017-06-27 23:16 ` Suman Anna
-1 siblings, 1 reply; 7+ messages in thread
From: Bjorn Andersson @ 2017-06-27 21:47 UTC (permalink / raw)
To: Suman Anna
Cc: Ohad Ben-Cohen, Peter Griffin, Mauro Carvalho Chehab,
Hugues Fruchet, Loic Pallardy, Arnd Bergmann, linux-media,
linux-kernel, linux-remoteproc
On Tue 27 Jun 12:08 PDT 2017, Suman Anna wrote:
> Hi Bjorn,
>
> Thanks for the patch.
>
> On 06/27/2017 01:43 AM, Bjorn Andersson wrote:
> > While it's very common to use RPMSG for communicating with firmware
> > running on these remoteprocs there is no functional dependency on RPMSG.
>
> This is not entirely accurate though. RPMSG is the IPC transport on
> these remoteprocs, you seem to suggest that there are alternatives for
> these remoteprocs. Without RPMSG, you can boot, but you will not be able
> to talk to the remoteprocs, so I would call it a functional dependency.
>
IMHO this functional dependency is not from the remoteproc driver but
your system (and firmware) design. It should be possible to write
firmware that exposes any other type of virtio device.
> > As such RPMSG should be selected by the system integrator and not
> > automatically by the remoteproc drivers.
> >
> > This does solve problems reported with circular Kconfig dependencies for
> > Davinci and Keystone remoteproc drivers.
>
> The Keystone one issue shows up on linux-next (and not on 4.12-rcX) due
> to the differing options on RESET_CONTROLLER on VIDEO_QCOM_VENUS
> (through QCOM_SCOM).
That's probably why I didn't see this when build testing before pushing
this.
> This can also be resolved by changing the depends on RESET_CONTROLLER
> to a select RESET_CONTROLLER or dropping the line.
>
Except that this would violate the general rule of "don't use 'select'
for user-selectable options".
> The davinci one is tricky though, as I did change it from using a select
> to a depends on dependency, and obviously ppc64_defconfig is something
> that I would not check.
>
While I hate the process of figuring out and enable all the dependencies
to be able to enable a particular config, this change does reduce the
risk of running into more of these cyclic dependencies.
> This patch definitely resolves both issues, but it is not obvious that
> someone would also have to enable RPMSG_VIRTIO to have these remoteprocs
> useful when looking at either of the menuconfig help.
>
This is a common issue we have with config options and while I hate to
add another item to the list of things that you can miss in your system
configuration I would prefer that my Kconfig is maintainable.
This also means that most remoteproc drivers should "depends on MAILBOX"
and not select either the framework or the specific drivers. But let's
try to ignore that until after the merge window.
Regards,
Bjorn
> regards
> Suman
>
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> > drivers/media/platform/Kconfig | 2 +-
> > drivers/remoteproc/Kconfig | 4 ----
> > drivers/rpmsg/Kconfig | 20 +++++++++-----------
> > 3 files changed, 10 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> > index 1313cd533436..cb2f31cd0088 100644
> > --- a/drivers/media/platform/Kconfig
> > +++ b/drivers/media/platform/Kconfig
> > @@ -382,10 +382,10 @@ config VIDEO_STI_DELTA_DRIVER
> > tristate
> > depends on VIDEO_STI_DELTA
> > depends on VIDEO_STI_DELTA_MJPEG
> > + depends on RPMSG
> > default VIDEO_STI_DELTA_MJPEG
> > select VIDEOBUF2_DMA_CONTIG
> > select V4L2_MEM2MEM_DEV
> > - select RPMSG
> >
> > endif # VIDEO_STI_DELTA
> >
> > diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> > index b950e6cd4ba2..3b16f422d30c 100644
> > --- a/drivers/remoteproc/Kconfig
> > +++ b/drivers/remoteproc/Kconfig
> > @@ -21,7 +21,6 @@ config OMAP_REMOTEPROC
> > depends on REMOTEPROC
> > select MAILBOX
> > select OMAP2PLUS_MBOX
> > - select RPMSG_VIRTIO
> > help
> > Say y here to support OMAP's remote processors (dual M3
> > and DSP on OMAP4) via the remote processor framework.
> > @@ -53,7 +52,6 @@ config DA8XX_REMOTEPROC
> > depends on ARCH_DAVINCI_DA8XX
> > depends on REMOTEPROC
> > depends on DMA_CMA
> > - select RPMSG_VIRTIO
> > help
> > Say y here to support DA8xx/OMAP-L13x remote processors via the
> > remote processor framework.
> > @@ -76,7 +74,6 @@ config KEYSTONE_REMOTEPROC
> > depends on ARCH_KEYSTONE
> > depends on RESET_CONTROLLER
> > depends on REMOTEPROC
> > - select RPMSG_VIRTIO
> > help
> > Say Y here here to support Keystone remote processors (DSP)
> > via the remote processor framework.
> > @@ -133,7 +130,6 @@ config ST_REMOTEPROC
> > depends on REMOTEPROC
> > select MAILBOX
> > select STI_MBOX
> > - select RPMSG_VIRTIO
> > help
> > Say y here to support ST's adjunct processors via the remote
> > processor framework.
> > diff --git a/drivers/rpmsg/Kconfig b/drivers/rpmsg/Kconfig
> > index 2a5d2b446de2..46f3f2431d68 100644
> > --- a/drivers/rpmsg/Kconfig
> > +++ b/drivers/rpmsg/Kconfig
> > @@ -1,8 +1,5 @@
> > -menu "Rpmsg drivers"
> > -
> > -# RPMSG always gets selected by whoever wants it
> > -config RPMSG
> > - tristate
> > +menuconfig RPMSG
> > + tristate "Rpmsg drivers"
> >
> > config RPMSG_CHAR
> > tristate "RPMSG device interface"
> > @@ -15,7 +12,7 @@ config RPMSG_CHAR
> >
> > config RPMSG_QCOM_GLINK_RPM
> > tristate "Qualcomm RPM Glink driver"
> > - select RPMSG
> > + depends on RPMSG
> > depends on HAS_IOMEM
> > depends on MAILBOX
> > help
> > @@ -26,16 +23,17 @@ config RPMSG_QCOM_GLINK_RPM
> > config RPMSG_QCOM_SMD
> > tristate "Qualcomm Shared Memory Driver (SMD)"
> > depends on QCOM_SMEM
> > - select RPMSG
> > + depends on RPMSG
> > help
> > Say y here to enable support for the Qualcomm Shared Memory Driver
> > providing communication channels to remote processors in Qualcomm
> > platforms.
> >
> > config RPMSG_VIRTIO
> > - tristate
> > - select RPMSG
> > + tristate "Virtio remote processor messaging driver (RPMSG)"
> > + depends on RPMSG
> > select VIRTIO
> > select VIRTUALIZATION
> > -
> > -endmenu
> > + help
> > + Say y here to enable support for the Virtio remote processor
> > + messaging protocol (RPMSG).
> >
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rpmsg: Solve circular dependencies involving RPMSG_VIRTIO
2017-06-27 21:47 ` Bjorn Andersson
@ 2017-06-27 23:16 ` Suman Anna
0 siblings, 0 replies; 7+ messages in thread
From: Suman Anna @ 2017-06-27 23:16 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Ohad Ben-Cohen, Peter Griffin, Mauro Carvalho Chehab,
Hugues Fruchet, Loic Pallardy, Arnd Bergmann, linux-media,
linux-kernel, linux-remoteproc
Hi Bjorn,
>>>> Thanks for the patch.
>>
>> On 06/27/2017 01:43 AM, Bjorn Andersson wrote:
>>> While it's very common to use RPMSG for communicating with firmware
>>> running on these remoteprocs there is no functional dependency on RPMSG.
>>
>> This is not entirely accurate though. RPMSG is the IPC transport on
>> these remoteprocs, you seem to suggest that there are alternatives for
>> these remoteprocs. Without RPMSG, you can boot, but you will not be able
>> to talk to the remoteprocs, so I would call it a functional dependency.
>>
>
> IMHO this functional dependency is not from the remoteproc driver but
> your system (and firmware) design. It should be possible to write
> firmware that exposes any other type of virtio device.
You may want to add this clarification to your commit description then.
>
>>> As such RPMSG should be selected by the system integrator and not
>>> automatically by the remoteproc drivers.
>>>
>>> This does solve problems reported with circular Kconfig dependencies for
>>> Davinci and Keystone remoteproc drivers.
>>
>> The Keystone one issue shows up on linux-next (and not on 4.12-rcX) due
>> to the differing options on RESET_CONTROLLER on VIDEO_QCOM_VENUS
>> (through QCOM_SCOM).
>
> That's probably why I didn't see this when build testing before pushing
> this.
>
>> This can also be resolved by changing the depends on RESET_CONTROLLER
>> to a select RESET_CONTROLLER or dropping the line.
>>
>
> Except that this would violate the general rule of "don't use 'select'
> for user-selectable options".
Yeah well, there seems to all sorts of usage w.r.t RESET_CONTROLLER and
VIRTIO. And if the ARCHs enable the ARCH_HAS_RESET_CONTROLLER, the
RESET_CONTROLLER dependencies are not even needed.
A quick grep on 4.12-rc7 yielded 20 occurrences that uses depends on and
21 for selects RESET_CONTROLLER. Similar numbers on VIRTIO are 9 and 9
(with a split between different VIRTIO drivers even).
>
>> The davinci one is tricky though, as I did change it from using a select
>> to a depends on dependency, and obviously ppc64_defconfig is something
>> that I would not check.
Posted a cleanup series that removes the VIRTUALIZATION dependency from
REMOTEPROC and RPMSG_VIRTIO options, the latter fixes the
ppc64_defconfig issue with davinci remoteproc.
>>
>
> While I hate the process of figuring out and enable all the dependencies
> to be able to enable a particular config, this change does reduce the
> risk of running into more of these cyclic dependencies.
>
>> This patch definitely resolves both issues, but it is not obvious that
>> someone would also have to enable RPMSG_VIRTIO to have these remoteprocs
>> useful when looking at either of the menuconfig help.
>>
>
> This is a common issue we have with config options and while I hate to
> add another item to the list of things that you can miss in your system
> configuration I would prefer that my Kconfig is maintainable.
>
> This also means that most remoteproc drivers should "depends on MAILBOX"
> and not select either the framework or the specific drivers. But let's
> try to ignore that until after the merge window.
Yeah ok, as long as you follow a consistent rule across all
remoteproc/rpmsg drivers, that's fine. In fact, other than RPMSG_VIRTIO,
the two drivers I added for this merge window do use/switches to a
depends on convention.
regards
Suman
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rpmsg: Solve circular dependencies involving RPMSG_VIRTIO
@ 2017-06-27 23:16 ` Suman Anna
0 siblings, 0 replies; 7+ messages in thread
From: Suman Anna @ 2017-06-27 23:16 UTC (permalink / raw)
To: Bjorn Andersson
Cc: Ohad Ben-Cohen, Peter Griffin, Mauro Carvalho Chehab,
Hugues Fruchet, Loic Pallardy, Arnd Bergmann, linux-media,
linux-kernel, linux-remoteproc
Hi Bjorn,
>>>> Thanks for the patch.
>>
>> On 06/27/2017 01:43 AM, Bjorn Andersson wrote:
>>> While it's very common to use RPMSG for communicating with firmware
>>> running on these remoteprocs there is no functional dependency on RPMSG.
>>
>> This is not entirely accurate though. RPMSG is the IPC transport on
>> these remoteprocs, you seem to suggest that there are alternatives for
>> these remoteprocs. Without RPMSG, you can boot, but you will not be able
>> to talk to the remoteprocs, so I would call it a functional dependency.
>>
>
> IMHO this functional dependency is not from the remoteproc driver but
> your system (and firmware) design. It should be possible to write
> firmware that exposes any other type of virtio device.
You may want to add this clarification to your commit description then.
>
>>> As such RPMSG should be selected by the system integrator and not
>>> automatically by the remoteproc drivers.
>>>
>>> This does solve problems reported with circular Kconfig dependencies for
>>> Davinci and Keystone remoteproc drivers.
>>
>> The Keystone one issue shows up on linux-next (and not on 4.12-rcX) due
>> to the differing options on RESET_CONTROLLER on VIDEO_QCOM_VENUS
>> (through QCOM_SCOM).
>
> That's probably why I didn't see this when build testing before pushing
> this.
>
>> This can also be resolved by changing the depends on RESET_CONTROLLER
>> to a select RESET_CONTROLLER or dropping the line.
>>
>
> Except that this would violate the general rule of "don't use 'select'
> for user-selectable options".
Yeah well, there seems to all sorts of usage w.r.t RESET_CONTROLLER and
VIRTIO. And if the ARCHs enable the ARCH_HAS_RESET_CONTROLLER, the
RESET_CONTROLLER dependencies are not even needed.
A quick grep on 4.12-rc7 yielded 20 occurrences that uses depends on and
21 for selects RESET_CONTROLLER. Similar numbers on VIRTIO are 9 and 9
(with a split between different VIRTIO drivers even).
>
>> The davinci one is tricky though, as I did change it from using a select
>> to a depends on dependency, and obviously ppc64_defconfig is something
>> that I would not check.
Posted a cleanup series that removes the VIRTUALIZATION dependency from
REMOTEPROC and RPMSG_VIRTIO options, the latter fixes the
ppc64_defconfig issue with davinci remoteproc.
>>
>
> While I hate the process of figuring out and enable all the dependencies
> to be able to enable a particular config, this change does reduce the
> risk of running into more of these cyclic dependencies.
>
>> This patch definitely resolves both issues, but it is not obvious that
>> someone would also have to enable RPMSG_VIRTIO to have these remoteprocs
>> useful when looking at either of the menuconfig help.
>>
>
> This is a common issue we have with config options and while I hate to
> add another item to the list of things that you can miss in your system
> configuration I would prefer that my Kconfig is maintainable.
>
> This also means that most remoteproc drivers should "depends on MAILBOX"
> and not select either the framework or the specific drivers. But let's
> try to ignore that until after the merge window.
Yeah ok, as long as you follow a consistent rule across all
remoteproc/rpmsg drivers, that's fine. In fact, other than RPMSG_VIRTIO,
the two drivers I added for this merge window do use/switches to a
depends on convention.
regards
Suman
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-06-27 23:16 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-27 6:43 [PATCH] rpmsg: Solve circular dependencies involving RPMSG_VIRTIO Bjorn Andersson
2017-06-27 7:29 ` Arnd Bergmann
2017-06-27 19:08 ` Suman Anna
2017-06-27 19:08 ` Suman Anna
2017-06-27 21:47 ` Bjorn Andersson
2017-06-27 23:16 ` Suman Anna
2017-06-27 23:16 ` Suman Anna
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.