[PATCH] remoteproc: move rproc_da_to_va declaration to remoteproc.h

[PATCH] remoteproc: move rproc_da_to_va declaration to remoteproc.h

[Drew Fustini]

From: Suman Anna <s-anna@ti.com>

The rproc_da_to_va() API is an exported function, so move its
declaration from the remoteproc local remoteproc_internal.h
to the public remoteproc.h file.

This will allow drivers outside of the remoteproc folder to be
able to use this API.

Signed-off-by: Suman Anna <s-anna@ti.com>
Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
[adjusted line numbers to apply]
Signed-off-by: Drew Fustini <dfustini@baylibre.com>
---
Note: I previously posted this patch as part of a series:
[PATCH 00/11] soc: ti: wkup_m3_ipc: support vtt toggle, io isolation & voltage scaling
https://lore.kernel.org/linux-omap/20220219215328.485660-1-dfustini@baylibre.com/

I was advised to break that series up into smaller pieces, so I am
submitting this patch individually. I will in the future post a series
to support i2c voltage scaling which will utilize rproc_da_to_va() from 
drivers/soc/ti/wkup_m3_ipc.c

 drivers/remoteproc/remoteproc_internal.h | 1 -
 include/linux/remoteproc.h               | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index a328e634b1de..72d4d3d7d94d 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -84,7 +84,6 @@ static inline void  rproc_char_device_remove(struct rproc *rproc)
 void rproc_free_vring(struct rproc_vring *rvring);
 int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
 
-void *rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool *is_iomem);
 phys_addr_t rproc_va_to_pa(void *cpu_addr);
 int rproc_trigger_recovery(struct rproc *rproc);
 
diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
index e0600e1e5c17..cc9dc9aef0c0 100644
--- a/include/linux/remoteproc.h
+++ b/include/linux/remoteproc.h
@@ -673,6 +673,7 @@ void rproc_shutdown(struct rproc *rproc);
 int rproc_detach(struct rproc *rproc);
 int rproc_set_firmware(struct rproc *rproc, const char *fw_name);
 void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
+void *rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool *is_iomem);
 void rproc_coredump_using_sections(struct rproc *rproc);
 int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size);
 int rproc_coredump_add_custom_segment(struct rproc *rproc,
-- 
2.32.0

Re: [PATCH] remoteproc: move rproc_da_to_va declaration to remoteproc.h

[Bjorn Andersson]

On Tue 08 Mar 09:25 PST 2022, Drew Fustini wrote:

> From: Suman Anna <s-anna@ti.com>
> 
> The rproc_da_to_va() API is an exported function, so move its
> declaration from the remoteproc local remoteproc_internal.h
> to the public remoteproc.h file.
> 
> This will allow drivers outside of the remoteproc folder to be
> able to use this API.
> 

Can you explain why drivers outside of the remoteproc folder should be
able to poke straight into the memory of the remoteproc?

Your reasoning makes sense, but we've on purpose kept it out of
remoteproc.h because no one has had a proper reason for it and I sense
that we might open the door for some new creative solutions...

Regards,
Bjorn

> Signed-off-by: Suman Anna <s-anna@ti.com>
> Signed-off-by: Dave Gerlach <d-gerlach@ti.com>
> [adjusted line numbers to apply]
> Signed-off-by: Drew Fustini <dfustini@baylibre.com>
> ---
> Note: I previously posted this patch as part of a series:
> [PATCH 00/11] soc: ti: wkup_m3_ipc: support vtt toggle, io isolation & voltage scaling
> https://lore.kernel.org/linux-omap/20220219215328.485660-1-dfustini@baylibre.com/
> 
> I was advised to break that series up into smaller pieces, so I am
> submitting this patch individually. I will in the future post a series
> to support i2c voltage scaling which will utilize rproc_da_to_va() from 
> drivers/soc/ti/wkup_m3_ipc.c
> 
>  drivers/remoteproc/remoteproc_internal.h | 1 -
>  include/linux/remoteproc.h               | 1 +
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index a328e634b1de..72d4d3d7d94d 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -84,7 +84,6 @@ static inline void  rproc_char_device_remove(struct rproc *rproc)
>  void rproc_free_vring(struct rproc_vring *rvring);
>  int rproc_alloc_vring(struct rproc_vdev *rvdev, int i);
>  
> -void *rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool *is_iomem);
>  phys_addr_t rproc_va_to_pa(void *cpu_addr);
>  int rproc_trigger_recovery(struct rproc *rproc);
>  
> diff --git a/include/linux/remoteproc.h b/include/linux/remoteproc.h
> index e0600e1e5c17..cc9dc9aef0c0 100644
> --- a/include/linux/remoteproc.h
> +++ b/include/linux/remoteproc.h
> @@ -673,6 +673,7 @@ void rproc_shutdown(struct rproc *rproc);
>  int rproc_detach(struct rproc *rproc);
>  int rproc_set_firmware(struct rproc *rproc, const char *fw_name);
>  void rproc_report_crash(struct rproc *rproc, enum rproc_crash_type type);
> +void *rproc_da_to_va(struct rproc *rproc, u64 da, size_t len, bool *is_iomem);
>  void rproc_coredump_using_sections(struct rproc *rproc);
>  int rproc_coredump_add_segment(struct rproc *rproc, dma_addr_t da, size_t size);
>  int rproc_coredump_add_custom_segment(struct rproc *rproc,
> -- 
> 2.32.0
> 

Re: [PATCH] remoteproc: move rproc_da_to_va declaration to remoteproc.h

[Drew Fustini]

On Tue, Mar 08, 2022 at 10:16:54AM -0800, Bjorn Andersson wrote:
> On Tue 08 Mar 09:25 PST 2022, Drew Fustini wrote:
> 
> > From: Suman Anna <s-anna@ti.com>
> > 
> > The rproc_da_to_va() API is an exported function, so move its
> > declaration from the remoteproc local remoteproc_internal.h
> > to the public remoteproc.h file.
> > 
> > This will allow drivers outside of the remoteproc folder to be
> > able to use this API.
> > 
> 
> Can you explain why drivers outside of the remoteproc folder should be
> able to poke straight into the memory of the remoteproc?
> 
> Your reasoning makes sense, but we've on purpose kept it out of
> remoteproc.h because no one has had a proper reason for it and I sense
> that we might open the door for some new creative solutions...

rproc_da_to_va() is used in a patch for drivers/soc/ti/wkup_m3_ipc.c
that adds support for i2c voltage scaling [1].

wkup_m3_copy_aux_data() will copy auxiliary data to special region of
the Cortex M3 memory. It calls rproc_da_to_va() to get aux_data_addr
which is then used as a memcpy destination.

Does that seem like a reasonable way to do it?

I was going to submit the i2c voltage scaling patches later. However,
I could combine them into a series with this remoteproc patch if that
helps to justify the remoteproc.h change.

Thanks,
Drew

[1] https://lore.kernel.org/linux-omap/20220219215328.485660-9-dfustini@baylibre.com/

Re: [PATCH] remoteproc: move rproc_da_to_va declaration to remoteproc.h

[Bjorn Andersson]

On Wed 09 Mar 11:04 CST 2022, Drew Fustini wrote:

> On Tue, Mar 08, 2022 at 10:16:54AM -0800, Bjorn Andersson wrote:
> > On Tue 08 Mar 09:25 PST 2022, Drew Fustini wrote:
> > 
> > > From: Suman Anna <s-anna@ti.com>
> > > 
> > > The rproc_da_to_va() API is an exported function, so move its
> > > declaration from the remoteproc local remoteproc_internal.h
> > > to the public remoteproc.h file.
> > > 
> > > This will allow drivers outside of the remoteproc folder to be
> > > able to use this API.
> > > 
> > 
> > Can you explain why drivers outside of the remoteproc folder should be
> > able to poke straight into the memory of the remoteproc?
> > 
> > Your reasoning makes sense, but we've on purpose kept it out of
> > remoteproc.h because no one has had a proper reason for it and I sense
> > that we might open the door for some new creative solutions...
> 
> rproc_da_to_va() is used in a patch for drivers/soc/ti/wkup_m3_ipc.c
> that adds support for i2c voltage scaling [1].
> 
> wkup_m3_copy_aux_data() will copy auxiliary data to special region of
> the Cortex M3 memory. It calls rproc_da_to_va() to get aux_data_addr
> which is then used as a memcpy destination.
> 

So in essence it's an essential part for the "communication protocol"
used to communicate with the remoteproc...

> Does that seem like a reasonable way to do it?
> 

I have a concern about the life cycle of the pointer acquired by this
"independent" driver. But this is an extension of my existing concern
where the wkup driver uses the remoteproc core as a "firmware loader",
but it's not a standalone remoteproc driver.

I think it would have been nicer to model the remoteproc driver as the
parent of the wkup device, so we probe/remove the wkup device based on
the state of the remoteproc.

This would remove concerns about races between the remoteproc
starting/stopping/restarting and the other driver and it would help
clarify that the life cycle of the pointer returned by rproc_da_to_va()
lives from start to stop of the remoteproc.


This does however not change the need for exporting the symbol, so I'm
merging this patch.

Regards,
Bjorn

> I was going to submit the i2c voltage scaling patches later. However,
> I could combine them into a series with this remoteproc patch if that
> helps to justify the remoteproc.h change.
> 
> Thanks,
> Drew
> 
> [1] https://lore.kernel.org/linux-omap/20220219215328.485660-9-dfustini@baylibre.com/