Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 11 Oct 2021 10:23:35 -0700
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Arnaud POULIQUEN <arnaud.pouliquen@foss.st.com>
Cc:     Ohad Ben-Cohen <ohad@wizery.com>,
        Mathieu Poirier <mathieu.poirier@linaro.org>,
        linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-stm32@st-md-mailman.stormreply.com,
        Rob Herring <robh@kernel.org>, Christoph Hellwig <hch@lst.de>,
        Stefano Stabellini <stefanos@xilinx.com>,
        Bruce Ashfield <bruce.ashfield@xilinx.com>
Subject: Re: [RFC PATCH 4/7] remoteproc: create the REMOTEPROC_VIRTIO config
Message-ID: <YWRzFyS5WX6m5rNd@ripper>
References: <20211001101234.4247-1-arnaud.pouliquen@foss.st.com>
 <20211001101234.4247-5-arnaud.pouliquen@foss.st.com>
 <YWEOIHrp4Z8+MHaE@builder.lan>
 <c16ca06a-96da-ac04-5ae7-bbbdf4b48ee5@foss.st.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c16ca06a-96da-ac04-5ae7-bbbdf4b48ee5@foss.st.com>
Precedence: bulk

On Mon 11 Oct 08:58 PDT 2021, Arnaud POULIQUEN wrote:

> 
> 
> On 10/9/21 5:36 AM, Bjorn Andersson wrote:
> > On Fri 01 Oct 05:12 CDT 2021, Arnaud Pouliquen wrote:
> > 
> >> Create the config to associate to the remoteproc virtio.
> >>
> >> Notice that the REMOTEPROC_VIRTIO config can not set to m. the reason
> >> is that it defines API that is used by the built-in remote proc core.
> >> Functions such are rproc_add_virtio_dev can be called during the
> >> Linux boot phase.
> >>
> > 
> > Please don't introduce new Kconfig options for everything. Consider that
> > the expectation should be that everyone runs the default defconfig on
> > their boards - and if someone actually needs this level of control, they
> > are welcome to present patches with numbers showing the benefit of the
> > savings.
> 
> My goal here was to decorrelate the remote virtio from the remote proc,
> so that platforms based on a non-virtio solution do not embed the code.
> By reading your commentary it jumps out at me that that's stupid.

I definitely don't think it's stupid. In a resource constraint
environment that want to use remoteproc but won't use virtio this makes
total sense.

But the added Kconfig creates complexity and people run into issues
because the default defconfig is incomplete, they got their defconfig
"wrong" or "make olddefconfig" misses the new config.

As such, I simply would like us to postpone the introduction of
configurability until there's someone that can show it's worth the
complexity.

> The REMOTEPROC_VIRTIO config is useless as the remoteproc_virtio must
> be kept built-in for legacy compatibility.
> 

Right, so we would have to make sure that in all "standard"
configurations REMOTEPROC_VIRTIO is included.

Regards,
Bjorn

> Thanks,
> Arnaud
> 
> > 
> > Thanks,
> > Bjorn
> > 
> >> Signed-off-by: Arnaud Pouliquen <arnaud.pouliquen@foss.st.com>
> >> ---
> >>  drivers/remoteproc/Kconfig               | 11 +++++++++-
> >>  drivers/remoteproc/Makefile              |  2 +-
> >>  drivers/remoteproc/remoteproc_internal.h | 28 ++++++++++++++++++++++++
> >>  3 files changed, 39 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
> >> index 9a6eedc3994a..f271552c0d84 100644
> >> --- a/drivers/remoteproc/Kconfig
> >> +++ b/drivers/remoteproc/Kconfig
> >> @@ -6,7 +6,7 @@ config REMOTEPROC
> >>  	depends on HAS_DMA
> >>  	select CRC32
> >>  	select FW_LOADER
> >> -	select VIRTIO
> >> +	select REMOTEPROC_VIRTIO
> >>  	select WANT_DEV_COREDUMP
> >>  	help
> >>  	  Support for remote processors (such as DSP coprocessors). These
> >> @@ -14,6 +14,15 @@ config REMOTEPROC
> >>  
> >>  if REMOTEPROC
> >>  
> >> +config REMOTEPROC_VIRTIO
> >> +	bool "Remoteproc virtio device "
> >> +	select VIRTIO
> >> +	help
> >> +	  Say y here to have a virtio device support for the remoteproc
> >> +	  communication.
> >> +
> >> +	  It's safe to say N if you don't use the virtio for the IPC.
> >> +
> >>  config REMOTEPROC_CDEV
> >>  	bool "Remoteproc character device interface"
> >>  	help
> >> diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
> >> index bb26c9e4ef9c..73d2384a76aa 100644
> >> --- a/drivers/remoteproc/Makefile
> >> +++ b/drivers/remoteproc/Makefile
> >> @@ -8,8 +8,8 @@ remoteproc-y				:= remoteproc_core.o
> >>  remoteproc-y				+= remoteproc_coredump.o
> >>  remoteproc-y				+= remoteproc_debugfs.o
> >>  remoteproc-y				+= remoteproc_sysfs.o
> >> -remoteproc-y				+= remoteproc_virtio.o
> >>  remoteproc-y				+= remoteproc_elf_loader.o
> >> +obj-$(CONFIG_REMOTEPROC_VIRTIO)		+= remoteproc_virtio.o
> >>  obj-$(CONFIG_REMOTEPROC_CDEV)		+= remoteproc_cdev.o
> >>  obj-$(CONFIG_IMX_REMOTEPROC)		+= imx_rproc.o
> >>  obj-$(CONFIG_INGENIC_VPU_RPROC)		+= ingenic_rproc.o
> >> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> >> index 152fe2e8668a..4ce012c353c0 100644
> >> --- a/drivers/remoteproc/remoteproc_internal.h
> >> +++ b/drivers/remoteproc/remoteproc_internal.h
> >> @@ -30,10 +30,38 @@ int rproc_of_parse_firmware(struct device *dev, int index,
> >>  			    const char **fw_name);
> >>  
> >>  /* from remoteproc_virtio.c */
> >> +#if IS_ENABLED(CONFIG_REMOTEPROC_VIRTIO)
> >> +
> >>  int rproc_rvdev_add_device(struct rproc_vdev *rvdev);
> >>  irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id);
> >>  void rproc_vdev_release(struct kref *ref);
> >>  
> >> +#else
> >> +
> >> +int rproc_rvdev_add_device(struct rproc_vdev *rvdev)
> >> +{
> >> +	/* This shouldn't be possible */
> >> +	WARN_ON(1);
> >> +
> >> +	return -ENXIO;
> >> +}
> >> +
> >> +static inline irqreturn_t rproc_vq_interrupt(struct rproc *rproc, int vq_id)
> >> +{
> >> +	/* This shouldn't be possible */
> >> +	WARN_ON(1);
> >> +
> >> +	return IRQ_NONE;
> >> +}
> >> +
> >> +static inline void rproc_vdev_release(struct kref *ref)
> >> +{
> >> +	/* This shouldn't be possible */
> >> +	WARN_ON(1);
> >> +}
> >> +
> >> +#endif
> >> +
> >>  /* from remoteproc_debugfs.c */
> >>  void rproc_remove_trace_file(struct dentry *tfile);
> >>  struct dentry *rproc_create_trace_file(const char *name, struct rproc *rproc,
> >> -- 
> >> 2.17.1
> >>