[v2,09/17] remoteproc: Call the right core function based on synchronisation state
diff mbox series

Message ID 20200324214603.14979-10-mathieu.poirier@linaro.org
State New
Headers show
Series
  • remoteproc: Add support for synchronisation with MCU
Related show

Commit Message

Mathieu Poirier March 24, 2020, 9:45 p.m. UTC
Call the right core function based on whether we should synchronise
with an MCU or boot it from scratch.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/remoteproc/remoteproc_internal.h | 36 +++++++++++-------------
 1 file changed, 17 insertions(+), 19 deletions(-)

Comments

Suman Anna March 31, 2020, 3:10 p.m. UTC | #1
Hi Mathieu,

On 3/24/20 4:45 PM, Mathieu Poirier wrote:
> Call the right core function based on whether we should synchronise
> with an MCU or boot it from scratch.

This patch does generate some checkpatch warnings.

> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>  drivers/remoteproc/remoteproc_internal.h | 36 +++++++++++-------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> index 73ea32df0156..5f711ceb97ba 100644
> --- a/drivers/remoteproc/remoteproc_internal.h
> +++ b/drivers/remoteproc/remoteproc_internal.h
> @@ -106,38 +106,41 @@ static inline void rproc_set_mcu_sync_state(struct rproc *rproc,
>  	}
>  }
>  
> +#define RPROC_OPS_HELPER(__operation, ...)				\
> +	do {								\
> +		if (rproc_sync_with_mcu(rproc)) {			\

So this does make the logic a bit convoluted, since you have three
different flags for rproc_sync_with_mcu, and you apply them in common
for all the ops. This is what I meant in my comment on Patch 1.

> +			if (!rproc->sync_ops ||				\
> +			    !rproc->sync_ops->__operation)		\
> +				return 0;				\
> +			return rproc->sync_ops->__operation(__VA_ARGS__); \

Use the same semantics as the regular ops instead of two return
statements, the code should fallback to the common return 0 after the
RPROC_OPS_HELPER when neither of them are present.

regards
Suman

> +		} else if (rproc->ops && rproc->ops->__operation)	\
> +			return rproc->ops->__operation(__VA_ARGS__);	\
> +	} while (0)							\
> +
>  static inline
>  int rproc_fw_sanity_check(struct rproc *rproc, const struct firmware *fw)
>  {
> -	if (rproc->ops->sanity_check)
> -		return rproc->ops->sanity_check(rproc, fw);
> -
> +	RPROC_OPS_HELPER(sanity_check, rproc, fw);
>  	return 0;
>  }
>  
>  static inline
>  u32 rproc_get_boot_addr(struct rproc *rproc, const struct firmware *fw)
>  {
> -	if (rproc->ops->get_boot_addr)
> -		return rproc->ops->get_boot_addr(rproc, fw);
> -
> +	RPROC_OPS_HELPER(get_boot_addr, rproc, fw);
>  	return 0;
>  }
>  
>  static inline
>  int rproc_load_segments(struct rproc *rproc, const struct firmware *fw)
>  {
> -	if (rproc->ops->load)
> -		return rproc->ops->load(rproc, fw);
> -
> +	RPROC_OPS_HELPER(load, rproc, fw);
>  	return -EINVAL;
>  }
>  
>  static inline int rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
>  {
> -	if (rproc->ops->parse_fw)
> -		return rproc->ops->parse_fw(rproc, fw);
> -
> +	RPROC_OPS_HELPER(parse_fw, rproc, fw);
>  	return 0;
>  }
>  
> @@ -145,10 +148,7 @@ static inline
>  int rproc_handle_rsc(struct rproc *rproc, u32 rsc_type, void *rsc, int offset,
>  		     int avail)
>  {
> -	if (rproc->ops->handle_rsc)
> -		return rproc->ops->handle_rsc(rproc, rsc_type, rsc, offset,
> -					      avail);
> -
> +	RPROC_OPS_HELPER(handle_rsc, rproc, rsc_type, rsc, offset, avail);
>  	return RSC_IGNORED;
>  }
>  
> @@ -156,9 +156,7 @@ static inline
>  struct resource_table *rproc_find_loaded_rsc_table(struct rproc *rproc,
>  						   const struct firmware *fw)
>  {
> -	if (rproc->ops->find_loaded_rsc_table)
> -		return rproc->ops->find_loaded_rsc_table(rproc, fw);
> -
> +	RPROC_OPS_HELPER(find_loaded_rsc_table, rproc, fw);
>  	return NULL;
>  }
>  
>
Mathieu Poirier April 2, 2020, 8:16 p.m. UTC | #2
On Tue, Mar 31, 2020 at 10:10:50AM -0500, Suman Anna wrote:
> Hi Mathieu,
> 
> On 3/24/20 4:45 PM, Mathieu Poirier wrote:
> > Call the right core function based on whether we should synchronise
> > with an MCU or boot it from scratch.
> 
> This patch does generate some checkpatch warnings.

Right, checkpatch is complaining but other than duplicating the same code for
all functions, I don't see another way to do this.

> 
> > 
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >  drivers/remoteproc/remoteproc_internal.h | 36 +++++++++++-------------
> >  1 file changed, 17 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
> > index 73ea32df0156..5f711ceb97ba 100644
> > --- a/drivers/remoteproc/remoteproc_internal.h
> > +++ b/drivers/remoteproc/remoteproc_internal.h
> > @@ -106,38 +106,41 @@ static inline void rproc_set_mcu_sync_state(struct rproc *rproc,
> >  	}
> >  }
> >  
> > +#define RPROC_OPS_HELPER(__operation, ...)				\
> > +	do {								\
> > +		if (rproc_sync_with_mcu(rproc)) {			\
> 
> So this does make the logic a bit convoluted, since you have three
> different flags for rproc_sync_with_mcu, and you apply them in common
> for all the ops. This is what I meant in my comment on Patch 1.

There is indeed 3 different flags but they are only valid in a specific state.
What "ops" are you referring to here?  I'm also not sure about the comment in
"patch 1" - which one would that be and how does it relate to the current block
of code.  Apologies, I need more clarifications.  

> 
> > +			if (!rproc->sync_ops ||				\
> > +			    !rproc->sync_ops->__operation)		\
> > +				return 0;				\
> > +			return rproc->sync_ops->__operation(__VA_ARGS__); \
> 
> Use the same semantics as the regular ops instead of two return
> statements, the code should fallback to the common return 0 after the
> RPROC_OPS_HELPER when neither of them are present.

Yes the tests are exactly the same, no reason to proceed differently.

> 
> regards
> Suman
> 
> > +		} else if (rproc->ops && rproc->ops->__operation)	\
> > +			return rproc->ops->__operation(__VA_ARGS__);	\
> > +	} while (0)							\
> > +
> >  static inline
> >  int rproc_fw_sanity_check(struct rproc *rproc, const struct firmware *fw)
> >  {
> > -	if (rproc->ops->sanity_check)
> > -		return rproc->ops->sanity_check(rproc, fw);
> > -
> > +	RPROC_OPS_HELPER(sanity_check, rproc, fw);
> >  	return 0;
> >  }
> >  
> >  static inline
> >  u32 rproc_get_boot_addr(struct rproc *rproc, const struct firmware *fw)
> >  {
> > -	if (rproc->ops->get_boot_addr)
> > -		return rproc->ops->get_boot_addr(rproc, fw);
> > -
> > +	RPROC_OPS_HELPER(get_boot_addr, rproc, fw);
> >  	return 0;
> >  }
> >  
> >  static inline
> >  int rproc_load_segments(struct rproc *rproc, const struct firmware *fw)
> >  {
> > -	if (rproc->ops->load)
> > -		return rproc->ops->load(rproc, fw);
> > -
> > +	RPROC_OPS_HELPER(load, rproc, fw);
> >  	return -EINVAL;
> >  }
> >  
> >  static inline int rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
> >  {
> > -	if (rproc->ops->parse_fw)
> > -		return rproc->ops->parse_fw(rproc, fw);
> > -
> > +	RPROC_OPS_HELPER(parse_fw, rproc, fw);
> >  	return 0;
> >  }
> >  
> > @@ -145,10 +148,7 @@ static inline
> >  int rproc_handle_rsc(struct rproc *rproc, u32 rsc_type, void *rsc, int offset,
> >  		     int avail)
> >  {
> > -	if (rproc->ops->handle_rsc)
> > -		return rproc->ops->handle_rsc(rproc, rsc_type, rsc, offset,
> > -					      avail);
> > -
> > +	RPROC_OPS_HELPER(handle_rsc, rproc, rsc_type, rsc, offset, avail);
> >  	return RSC_IGNORED;
> >  }
> >  
> > @@ -156,9 +156,7 @@ static inline
> >  struct resource_table *rproc_find_loaded_rsc_table(struct rproc *rproc,
> >  						   const struct firmware *fw)
> >  {
> > -	if (rproc->ops->find_loaded_rsc_table)
> > -		return rproc->ops->find_loaded_rsc_table(rproc, fw);
> > -
> > +	RPROC_OPS_HELPER(find_loaded_rsc_table, rproc, fw);
> >  	return NULL;
> >  }
> >  
> > 
>
Suman Anna April 9, 2020, 6:48 p.m. UTC | #3
On 4/2/20 3:16 PM, Mathieu Poirier wrote:
> On Tue, Mar 31, 2020 at 10:10:50AM -0500, Suman Anna wrote:
>> Hi Mathieu,
>>
>> On 3/24/20 4:45 PM, Mathieu Poirier wrote:
>>> Call the right core function based on whether we should synchronise
>>> with an MCU or boot it from scratch.
>>
>> This patch does generate some checkpatch warnings.
> 
> Right, checkpatch is complaining but other than duplicating the same code for
> all functions, I don't see another way to do this.
> 
>>
>>>
>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
>>> ---
>>>  drivers/remoteproc/remoteproc_internal.h | 36 +++++++++++-------------
>>>  1 file changed, 17 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
>>> index 73ea32df0156..5f711ceb97ba 100644
>>> --- a/drivers/remoteproc/remoteproc_internal.h
>>> +++ b/drivers/remoteproc/remoteproc_internal.h
>>> @@ -106,38 +106,41 @@ static inline void rproc_set_mcu_sync_state(struct rproc *rproc,
>>>  	}
>>>  }
>>>  
>>> +#define RPROC_OPS_HELPER(__operation, ...)				\
>>> +	do {								\
>>> +		if (rproc_sync_with_mcu(rproc)) {			\
>>
>> So this does make the logic a bit convoluted, since you have three
>> different flags for rproc_sync_with_mcu, and you apply them in common
>> for all the ops. This is what I meant in my comment on Patch 1.
> 
> There is indeed 3 different flags but they are only valid in a specific state.
> What "ops" are you referring to here?

All the rproc_ops callbacks, since only a subset of each might be valid
at each of the three different states. Granted this provides the maximum
flexibility for platform drivers, but it's a bit convoluted. Kinda goes
with Loic's comment on Patch 7 [1].

[1] https://patchwork.kernel.org/comment/23249975/


I'm also not sure about the comment in
> "patch 1" - which one would that be and how does it relate to the current block
> of code.  Apologies, I need more clarifications.  

I meant the following comment,
"And I am wondering if it is actually better to introduce the sync state
to check against here, rather than using the stored sync state and
return. The current way makes it confusing to read the state machine."

regards
Suman

> 
>>
>>> +			if (!rproc->sync_ops ||				\
>>> +			    !rproc->sync_ops->__operation)		\
>>> +				return 0;				\
>>> +			return rproc->sync_ops->__operation(__VA_ARGS__); \
>>
>> Use the same semantics as the regular ops instead of two return
>> statements, the code should fallback to the common return 0 after the
>> RPROC_OPS_HELPER when neither of them are present.
> 
> Yes the tests are exactly the same, no reason to proceed differently.
> 
>>
>> regards
>> Suman
>>
>>> +		} else if (rproc->ops && rproc->ops->__operation)	\
>>> +			return rproc->ops->__operation(__VA_ARGS__);	\
>>> +	} while (0)							\
>>> +
>>>  static inline
>>>  int rproc_fw_sanity_check(struct rproc *rproc, const struct firmware *fw)
>>>  {
>>> -	if (rproc->ops->sanity_check)
>>> -		return rproc->ops->sanity_check(rproc, fw);
>>> -
>>> +	RPROC_OPS_HELPER(sanity_check, rproc, fw);
>>>  	return 0;
>>>  }
>>>  
>>>  static inline
>>>  u32 rproc_get_boot_addr(struct rproc *rproc, const struct firmware *fw)
>>>  {
>>> -	if (rproc->ops->get_boot_addr)
>>> -		return rproc->ops->get_boot_addr(rproc, fw);
>>> -
>>> +	RPROC_OPS_HELPER(get_boot_addr, rproc, fw);
>>>  	return 0;
>>>  }
>>>  
>>>  static inline
>>>  int rproc_load_segments(struct rproc *rproc, const struct firmware *fw)
>>>  {
>>> -	if (rproc->ops->load)
>>> -		return rproc->ops->load(rproc, fw);
>>> -
>>> +	RPROC_OPS_HELPER(load, rproc, fw);
>>>  	return -EINVAL;
>>>  }
>>>  
>>>  static inline int rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
>>>  {
>>> -	if (rproc->ops->parse_fw)
>>> -		return rproc->ops->parse_fw(rproc, fw);
>>> -
>>> +	RPROC_OPS_HELPER(parse_fw, rproc, fw);
>>>  	return 0;
>>>  }
>>>  
>>> @@ -145,10 +148,7 @@ static inline
>>>  int rproc_handle_rsc(struct rproc *rproc, u32 rsc_type, void *rsc, int offset,
>>>  		     int avail)
>>>  {
>>> -	if (rproc->ops->handle_rsc)
>>> -		return rproc->ops->handle_rsc(rproc, rsc_type, rsc, offset,
>>> -					      avail);
>>> -
>>> +	RPROC_OPS_HELPER(handle_rsc, rproc, rsc_type, rsc, offset, avail);
>>>  	return RSC_IGNORED;
>>>  }
>>>  
>>> @@ -156,9 +156,7 @@ static inline
>>>  struct resource_table *rproc_find_loaded_rsc_table(struct rproc *rproc,
>>>  						   const struct firmware *fw)
>>>  {
>>> -	if (rproc->ops->find_loaded_rsc_table)
>>> -		return rproc->ops->find_loaded_rsc_table(rproc, fw);
>>> -
>>> +	RPROC_OPS_HELPER(find_loaded_rsc_table, rproc, fw);
>>>  	return NULL;
>>>  }
>>>  
>>>
>>

Patch
diff mbox series

diff --git a/drivers/remoteproc/remoteproc_internal.h b/drivers/remoteproc/remoteproc_internal.h
index 73ea32df0156..5f711ceb97ba 100644
--- a/drivers/remoteproc/remoteproc_internal.h
+++ b/drivers/remoteproc/remoteproc_internal.h
@@ -106,38 +106,41 @@  static inline void rproc_set_mcu_sync_state(struct rproc *rproc,
 	}
 }
 
+#define RPROC_OPS_HELPER(__operation, ...)				\
+	do {								\
+		if (rproc_sync_with_mcu(rproc)) {			\
+			if (!rproc->sync_ops ||				\
+			    !rproc->sync_ops->__operation)		\
+				return 0;				\
+			return rproc->sync_ops->__operation(__VA_ARGS__); \
+		} else if (rproc->ops && rproc->ops->__operation)	\
+			return rproc->ops->__operation(__VA_ARGS__);	\
+	} while (0)							\
+
 static inline
 int rproc_fw_sanity_check(struct rproc *rproc, const struct firmware *fw)
 {
-	if (rproc->ops->sanity_check)
-		return rproc->ops->sanity_check(rproc, fw);
-
+	RPROC_OPS_HELPER(sanity_check, rproc, fw);
 	return 0;
 }
 
 static inline
 u32 rproc_get_boot_addr(struct rproc *rproc, const struct firmware *fw)
 {
-	if (rproc->ops->get_boot_addr)
-		return rproc->ops->get_boot_addr(rproc, fw);
-
+	RPROC_OPS_HELPER(get_boot_addr, rproc, fw);
 	return 0;
 }
 
 static inline
 int rproc_load_segments(struct rproc *rproc, const struct firmware *fw)
 {
-	if (rproc->ops->load)
-		return rproc->ops->load(rproc, fw);
-
+	RPROC_OPS_HELPER(load, rproc, fw);
 	return -EINVAL;
 }
 
 static inline int rproc_parse_fw(struct rproc *rproc, const struct firmware *fw)
 {
-	if (rproc->ops->parse_fw)
-		return rproc->ops->parse_fw(rproc, fw);
-
+	RPROC_OPS_HELPER(parse_fw, rproc, fw);
 	return 0;
 }
 
@@ -145,10 +148,7 @@  static inline
 int rproc_handle_rsc(struct rproc *rproc, u32 rsc_type, void *rsc, int offset,
 		     int avail)
 {
-	if (rproc->ops->handle_rsc)
-		return rproc->ops->handle_rsc(rproc, rsc_type, rsc, offset,
-					      avail);
-
+	RPROC_OPS_HELPER(handle_rsc, rproc, rsc_type, rsc, offset, avail);
 	return RSC_IGNORED;
 }
 
@@ -156,9 +156,7 @@  static inline
 struct resource_table *rproc_find_loaded_rsc_table(struct rproc *rproc,
 						   const struct firmware *fw)
 {
-	if (rproc->ops->find_loaded_rsc_table)
-		return rproc->ops->find_loaded_rsc_table(rproc, fw);
-
+	RPROC_OPS_HELPER(find_loaded_rsc_table, rproc, fw);
 	return NULL;
 }