[v4,1/7] riscv: asm: alternative-macros: Introduce ALTERNATIVE_3() macro

  From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Introduce ALTERNATIVE_3() macro.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
RFC v3 -> v4
* New patch
---
 arch/riscv/include/asm/alternative-macros.h | 94 +++++++++++++++++++++
 1 file changed, 94 insertions(+)
  

Am Donnerstag, 24. November 2022, 18:22:01 CET schrieb Prabhakar:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Introduce ALTERNATIVE_3() macro.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

> ---
> RFC v3 -> v4
> * New patch
> ---
>  arch/riscv/include/asm/alternative-macros.h | 94 +++++++++++++++++++++
>  1 file changed, 94 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h
> index ec2f3f1b836f..1caf4306b3d6 100644
> --- a/arch/riscv/include/asm/alternative-macros.h
> +++ b/arch/riscv/include/asm/alternative-macros.h
> @@ -69,6 +69,34 @@
>  				   new_c_2, vendor_id_2, errata_id_2,	\
>  					IS_ENABLED(CONFIG_k_2)
>  
> +.macro __ALTERNATIVE_CFG_3 old_c, new_c_1, vendor_id_1, errata_id_1, enable_1, \
> +				  new_c_2, vendor_id_2, errata_id_2, enable_2, \
> +				  new_c_3, vendor_id_3, errata_id_3, enable_3
> +886 :
> +	.option push
> +	.option norvc
> +	.option norelax
> +	\old_c
> +	.option pop
> +887 :
> +	ALT_NEW_CONTENT \vendor_id_1, \errata_id_1, \enable_1, \new_c_1
> +	ALT_NEW_CONTENT \vendor_id_2, \errata_id_2, \enable_2, \new_c_2
> +	ALT_NEW_CONTENT \vendor_id_3, \errata_id_3, \enable_3, \new_c_3
> +.endm
> +
> +#define _ALTERNATIVE_CFG_3(old_c, new_c_1, vendor_id_1, errata_id_1,	\
> +					CONFIG_k_1,			\
> +				  new_c_2, vendor_id_2, errata_id_2,	\
> +					CONFIG_k_2,			\
> +				  new_c_3, vendor_id_3, errata_id_3,	\
> +					CONFIG_k_3)			\
> +	__ALTERNATIVE_CFG_3 old_c, new_c_1, vendor_id_1, errata_id_1,	\
> +					IS_ENABLED(CONFIG_k_1),		\
> +				   new_c_2, vendor_id_2, errata_id_2,	\
> +					IS_ENABLED(CONFIG_k_2),		\
> +				   new_c_3, vendor_id_3, errata_id_3,	\
> +					IS_ENABLED(CONFIG_k_3)
> +
>  #else /* !__ASSEMBLY__ */
>  
>  #include <asm/asm.h>
> @@ -135,6 +163,36 @@
>  				   new_c_2, vendor_id_2, errata_id_2,	\
>  					IS_ENABLED(CONFIG_k_2))
>  
> +#define __ALTERNATIVE_CFG_3(old_c, new_c_1, vendor_id_1, errata_id_1,	\
> +					enable_1,			\
> +				   new_c_2, vendor_id_2, errata_id_2,	\
> +					enable_2,			\
> +				   new_c_3, vendor_id_3, errata_id_3,	\
> +					enable_3)			\
> +	"886 :\n"							\
> +	".option push\n"						\
> +	".option norvc\n"						\
> +	".option norelax\n"						\
> +	old_c "\n"							\
> +	".option pop\n"							\
> +	"887 :\n"							\
> +	ALT_NEW_CONTENT(vendor_id_1, errata_id_1, enable_1, new_c_1)	\
> +	ALT_NEW_CONTENT(vendor_id_2, errata_id_2, enable_2, new_c_2)	\
> +	ALT_NEW_CONTENT(vendor_id_3, errata_id_3, enable_3, new_c_3)
> +
> +#define _ALTERNATIVE_CFG_3(old_c, new_c_1, vendor_id_1, errata_id_1,	\
> +					CONFIG_k_1,			\
> +				  new_c_2, vendor_id_2, errata_id_2,	\
> +					CONFIG_k_2,			\
> +				  new_c_3, vendor_id_3, errata_id_3,	\
> +					CONFIG_k_3)			\
> +	__ALTERNATIVE_CFG_3(old_c, new_c_1, vendor_id_1, errata_id_1,	\
> +					IS_ENABLED(CONFIG_k_1),		\
> +				   new_c_2, vendor_id_2, errata_id_2,	\
> +					IS_ENABLED(CONFIG_k_2),	\
> +				   new_c_3, vendor_id_3, errata_id_3,	\
> +					IS_ENABLED(CONFIG_k_3))
> +
>  #endif /* __ASSEMBLY__ */
>  
>  #else /* CONFIG_RISCV_ALTERNATIVE */
> @@ -153,6 +211,14 @@
>  					CONFIG_k_2)			\
>         __ALTERNATIVE_CFG old_c
>  
> +#define _ALTERNATIVE_CFG_3(old_c, new_c_1, vendor_id_1, errata_id_1,	\
> +					CONFIG_k_1,			\
> +				  new_c_2, vendor_id_2, errata_id_2,	\
> +					CONFIG_k_2,			\
> +				  new_c_3, vendor_id_3, errata_id_3,	\
> +					CONFIG_k_3)			\
> +       __ALTERNATIVE_CFG old_c
> +
>  #else /* !__ASSEMBLY__ */
>  
>  #define __ALTERNATIVE_CFG(old_c)  \
> @@ -167,6 +233,14 @@
>  					CONFIG_k_2) \
>         __ALTERNATIVE_CFG(old_c)
>  
> +#define _ALTERNATIVE_CFG_3(old_c, new_c_1, vendor_id_1, errata_id_1,	\
> +					CONFIG_k_1,			\
> +				  new_c_2, vendor_id_2, errata_id_2,	\
> +					CONFIG_k_2,			\
> +				  new_c_3, vendor_id_3, errata_id_3,	\
> +					CONFIG_k_3) \
> +       __ALTERNATIVE_CFG(old_c)
> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* CONFIG_RISCV_ALTERNATIVE */
>  
> @@ -202,4 +276,24 @@
>  					new_content_2, vendor_id_2,	\
>  					    errata_id_2, CONFIG_k_2)
>  
> +/*
> + * A vendor wants to replace an old_content, but another vendor has used
> + * ALTERNATIVE_2() to patch its customized content at the same location. In
> + * this case, this vendor can create a new macro ALTERNATIVE_3() based
> + * on the following sample code and then replace ALTERNATIVE_2() with
> + * ALTERNATIVE_3() to append its customized content.
> + */
> +#define ALTERNATIVE_3(old_content, new_content_1, vendor_id_1,		\
> +					errata_id_1, CONFIG_k_1,	\
> +				   new_content_2, vendor_id_2,		\
> +					errata_id_2, CONFIG_k_2,	\
> +				   new_content_3, vendor_id_3,		\
> +					errata_id_3, CONFIG_k_3)	\
> +	_ALTERNATIVE_CFG_3(old_content, new_content_1, vendor_id_1,	\
> +					    errata_id_1, CONFIG_k_1,	\
> +					new_content_2, vendor_id_2,	\
> +					    errata_id_2, CONFIG_k_2,	\
> +					new_content_3, vendor_id_3,	\
> +					    errata_id_3, CONFIG_k_3)
> +
>  #endif
>
  

On Thu, Nov 24, 2022 at 05:22:01PM +0000, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Introduce ALTERNATIVE_3() macro.

Bit perfunctory I think! There's a lovely comment down below that would
make for a better commit message if you were to yoink it.
Content looks about what I'd expect to see though.

> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> RFC v3 -> v4
> * New patch
> ---
>  arch/riscv/include/asm/alternative-macros.h | 94 +++++++++++++++++++++
>  1 file changed, 94 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h
> index ec2f3f1b836f..1caf4306b3d6 100644
> --- a/arch/riscv/include/asm/alternative-macros.h
> +++ b/arch/riscv/include/asm/alternative-macros.h
> @@ -69,6 +69,34 @@
>  				   new_c_2, vendor_id_2, errata_id_2,	\
>  					IS_ENABLED(CONFIG_k_2)
>  
> +.macro __ALTERNATIVE_CFG_3 old_c, new_c_1, vendor_id_1, errata_id_1, enable_1, \
> +				  new_c_2, vendor_id_2, errata_id_2, enable_2, \
> +				  new_c_3, vendor_id_3, errata_id_3, enable_3
> +886 :
> +	.option push
> +	.option norvc
> +	.option norelax
> +	\old_c
> +	.option pop
> +887 :
> +	ALT_NEW_CONTENT \vendor_id_1, \errata_id_1, \enable_1, \new_c_1
> +	ALT_NEW_CONTENT \vendor_id_2, \errata_id_2, \enable_2, \new_c_2
> +	ALT_NEW_CONTENT \vendor_id_3, \errata_id_3, \enable_3, \new_c_3
> +.endm
> +
> +#define _ALTERNATIVE_CFG_3(old_c, new_c_1, vendor_id_1, errata_id_1,	\
> +					CONFIG_k_1,			\
> +				  new_c_2, vendor_id_2, errata_id_2,	\
> +					CONFIG_k_2,			\
> +				  new_c_3, vendor_id_3, errata_id_3,	\
> +					CONFIG_k_3)			\
> +	__ALTERNATIVE_CFG_3 old_c, new_c_1, vendor_id_1, errata_id_1,	\
> +					IS_ENABLED(CONFIG_k_1),		\
> +				   new_c_2, vendor_id_2, errata_id_2,	\
> +					IS_ENABLED(CONFIG_k_2),		\
> +				   new_c_3, vendor_id_3, errata_id_3,	\
> +					IS_ENABLED(CONFIG_k_3)
> +
>  #else /* !__ASSEMBLY__ */
>  
>  #include <asm/asm.h>
> @@ -135,6 +163,36 @@
>  				   new_c_2, vendor_id_2, errata_id_2,	\
>  					IS_ENABLED(CONFIG_k_2))
>  
> +#define __ALTERNATIVE_CFG_3(old_c, new_c_1, vendor_id_1, errata_id_1,	\
> +					enable_1,			\
> +				   new_c_2, vendor_id_2, errata_id_2,	\
> +					enable_2,			\
> +				   new_c_3, vendor_id_3, errata_id_3,	\
> +					enable_3)			\
> +	"886 :\n"							\
> +	".option push\n"						\
> +	".option norvc\n"						\
> +	".option norelax\n"						\
> +	old_c "\n"							\
> +	".option pop\n"							\
> +	"887 :\n"							\
> +	ALT_NEW_CONTENT(vendor_id_1, errata_id_1, enable_1, new_c_1)	\
> +	ALT_NEW_CONTENT(vendor_id_2, errata_id_2, enable_2, new_c_2)	\
> +	ALT_NEW_CONTENT(vendor_id_3, errata_id_3, enable_3, new_c_3)
> +
> +#define _ALTERNATIVE_CFG_3(old_c, new_c_1, vendor_id_1, errata_id_1,	\
> +					CONFIG_k_1,			\
> +				  new_c_2, vendor_id_2, errata_id_2,	\
> +					CONFIG_k_2,			\
> +				  new_c_3, vendor_id_3, errata_id_3,	\
> +					CONFIG_k_3)			\
> +	__ALTERNATIVE_CFG_3(old_c, new_c_1, vendor_id_1, errata_id_1,	\
> +					IS_ENABLED(CONFIG_k_1),		\
> +				   new_c_2, vendor_id_2, errata_id_2,	\
> +					IS_ENABLED(CONFIG_k_2),	\
> +				   new_c_3, vendor_id_3, errata_id_3,	\
> +					IS_ENABLED(CONFIG_k_3))
> +
>  #endif /* __ASSEMBLY__ */
>  
>  #else /* CONFIG_RISCV_ALTERNATIVE */
> @@ -153,6 +211,14 @@
>  					CONFIG_k_2)			\
>         __ALTERNATIVE_CFG old_c
>  
> +#define _ALTERNATIVE_CFG_3(old_c, new_c_1, vendor_id_1, errata_id_1,	\
> +					CONFIG_k_1,			\
> +				  new_c_2, vendor_id_2, errata_id_2,	\
> +					CONFIG_k_2,			\
> +				  new_c_3, vendor_id_3, errata_id_3,	\
> +					CONFIG_k_3)			\
> +       __ALTERNATIVE_CFG old_c
> +
>  #else /* !__ASSEMBLY__ */
>  
>  #define __ALTERNATIVE_CFG(old_c)  \
> @@ -167,6 +233,14 @@
>  					CONFIG_k_2) \
>         __ALTERNATIVE_CFG(old_c)
>  
> +#define _ALTERNATIVE_CFG_3(old_c, new_c_1, vendor_id_1, errata_id_1,	\
> +					CONFIG_k_1,			\
> +				  new_c_2, vendor_id_2, errata_id_2,	\
> +					CONFIG_k_2,			\
> +				  new_c_3, vendor_id_3, errata_id_3,	\
> +					CONFIG_k_3) \
> +       __ALTERNATIVE_CFG(old_c)
> +
>  #endif /* __ASSEMBLY__ */
>  #endif /* CONFIG_RISCV_ALTERNATIVE */
>  
> @@ -202,4 +276,24 @@
>  					new_content_2, vendor_id_2,	\
>  					    errata_id_2, CONFIG_k_2)
>  
> +/*
> + * A vendor wants to replace an old_content, but another vendor has used
> + * ALTERNATIVE_2() to patch its customized content at the same location. In
> + * this case, this vendor can create a new macro ALTERNATIVE_3() based
> + * on the following sample code and then replace ALTERNATIVE_2() with
> + * ALTERNATIVE_3() to append its customized content.
> + */
> +#define ALTERNATIVE_3(old_content, new_content_1, vendor_id_1,		\
> +					errata_id_1, CONFIG_k_1,	\
> +				   new_content_2, vendor_id_2,		\
> +					errata_id_2, CONFIG_k_2,	\
> +				   new_content_3, vendor_id_3,		\
> +					errata_id_3, CONFIG_k_3)	\
> +	_ALTERNATIVE_CFG_3(old_content, new_content_1, vendor_id_1,	\
> +					    errata_id_1, CONFIG_k_1,	\
> +					new_content_2, vendor_id_2,	\
> +					    errata_id_2, CONFIG_k_2,	\
> +					new_content_3, vendor_id_3,	\
> +					    errata_id_3, CONFIG_k_3)
> +
>  #endif
> -- 
> 2.25.1
>
  

Am Donnerstag, 24. November 2022, 20:52:33 CET schrieb Conor Dooley:
> On Thu, Nov 24, 2022 at 05:22:01PM +0000, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > 
> > Introduce ALTERNATIVE_3() macro.
> 
> Bit perfunctory I think! There's a lovely comment down below that would
> make for a better commit message if you were to yoink it.
> Content looks about what I'd expect to see though.

Also both the comment on the original ALTERNATIVE_2 and the new ALTERNATIVE_3
should probably be merged into a single comment explaining this once for all
ALTERNATIVE_x variants.

Especially with the dma stuff, I'm pretty sure we'll get at least an ALTERNATIVE_4
if not even more ;-) . So we defnitly don't want to repeat this multiple times.


Heiko

> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> > RFC v3 -> v4
> > * New patch
> > ---
> >  arch/riscv/include/asm/alternative-macros.h | 94 +++++++++++++++++++++
> >  1 file changed, 94 insertions(+)
> > 
> > diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h
> > index ec2f3f1b836f..1caf4306b3d6 100644
> > --- a/arch/riscv/include/asm/alternative-macros.h
> > +++ b/arch/riscv/include/asm/alternative-macros.h
> > @@ -69,6 +69,34 @@
> >  				   new_c_2, vendor_id_2, errata_id_2,	\
> >  					IS_ENABLED(CONFIG_k_2)
> >  
> > +.macro __ALTERNATIVE_CFG_3 old_c, new_c_1, vendor_id_1, errata_id_1, enable_1, \
> > +				  new_c_2, vendor_id_2, errata_id_2, enable_2, \
> > +				  new_c_3, vendor_id_3, errata_id_3, enable_3
> > +886 :
> > +	.option push
> > +	.option norvc
> > +	.option norelax
> > +	\old_c
> > +	.option pop
> > +887 :
> > +	ALT_NEW_CONTENT \vendor_id_1, \errata_id_1, \enable_1, \new_c_1
> > +	ALT_NEW_CONTENT \vendor_id_2, \errata_id_2, \enable_2, \new_c_2
> > +	ALT_NEW_CONTENT \vendor_id_3, \errata_id_3, \enable_3, \new_c_3
> > +.endm
> > +
> > +#define _ALTERNATIVE_CFG_3(old_c, new_c_1, vendor_id_1, errata_id_1,	\
> > +					CONFIG_k_1,			\
> > +				  new_c_2, vendor_id_2, errata_id_2,	\
> > +					CONFIG_k_2,			\
> > +				  new_c_3, vendor_id_3, errata_id_3,	\
> > +					CONFIG_k_3)			\
> > +	__ALTERNATIVE_CFG_3 old_c, new_c_1, vendor_id_1, errata_id_1,	\
> > +					IS_ENABLED(CONFIG_k_1),		\
> > +				   new_c_2, vendor_id_2, errata_id_2,	\
> > +					IS_ENABLED(CONFIG_k_2),		\
> > +				   new_c_3, vendor_id_3, errata_id_3,	\
> > +					IS_ENABLED(CONFIG_k_3)
> > +
> >  #else /* !__ASSEMBLY__ */
> >  
> >  #include <asm/asm.h>
> > @@ -135,6 +163,36 @@
> >  				   new_c_2, vendor_id_2, errata_id_2,	\
> >  					IS_ENABLED(CONFIG_k_2))
> >  
> > +#define __ALTERNATIVE_CFG_3(old_c, new_c_1, vendor_id_1, errata_id_1,	\
> > +					enable_1,			\
> > +				   new_c_2, vendor_id_2, errata_id_2,	\
> > +					enable_2,			\
> > +				   new_c_3, vendor_id_3, errata_id_3,	\
> > +					enable_3)			\
> > +	"886 :\n"							\
> > +	".option push\n"						\
> > +	".option norvc\n"						\
> > +	".option norelax\n"						\
> > +	old_c "\n"							\
> > +	".option pop\n"							\
> > +	"887 :\n"							\
> > +	ALT_NEW_CONTENT(vendor_id_1, errata_id_1, enable_1, new_c_1)	\
> > +	ALT_NEW_CONTENT(vendor_id_2, errata_id_2, enable_2, new_c_2)	\
> > +	ALT_NEW_CONTENT(vendor_id_3, errata_id_3, enable_3, new_c_3)
> > +
> > +#define _ALTERNATIVE_CFG_3(old_c, new_c_1, vendor_id_1, errata_id_1,	\
> > +					CONFIG_k_1,			\
> > +				  new_c_2, vendor_id_2, errata_id_2,	\
> > +					CONFIG_k_2,			\
> > +				  new_c_3, vendor_id_3, errata_id_3,	\
> > +					CONFIG_k_3)			\
> > +	__ALTERNATIVE_CFG_3(old_c, new_c_1, vendor_id_1, errata_id_1,	\
> > +					IS_ENABLED(CONFIG_k_1),		\
> > +				   new_c_2, vendor_id_2, errata_id_2,	\
> > +					IS_ENABLED(CONFIG_k_2),	\
> > +				   new_c_3, vendor_id_3, errata_id_3,	\
> > +					IS_ENABLED(CONFIG_k_3))
> > +
> >  #endif /* __ASSEMBLY__ */
> >  
> >  #else /* CONFIG_RISCV_ALTERNATIVE */
> > @@ -153,6 +211,14 @@
> >  					CONFIG_k_2)			\
> >         __ALTERNATIVE_CFG old_c
> >  
> > +#define _ALTERNATIVE_CFG_3(old_c, new_c_1, vendor_id_1, errata_id_1,	\
> > +					CONFIG_k_1,			\
> > +				  new_c_2, vendor_id_2, errata_id_2,	\
> > +					CONFIG_k_2,			\
> > +				  new_c_3, vendor_id_3, errata_id_3,	\
> > +					CONFIG_k_3)			\
> > +       __ALTERNATIVE_CFG old_c
> > +
> >  #else /* !__ASSEMBLY__ */
> >  
> >  #define __ALTERNATIVE_CFG(old_c)  \
> > @@ -167,6 +233,14 @@
> >  					CONFIG_k_2) \
> >         __ALTERNATIVE_CFG(old_c)
> >  
> > +#define _ALTERNATIVE_CFG_3(old_c, new_c_1, vendor_id_1, errata_id_1,	\
> > +					CONFIG_k_1,			\
> > +				  new_c_2, vendor_id_2, errata_id_2,	\
> > +					CONFIG_k_2,			\
> > +				  new_c_3, vendor_id_3, errata_id_3,	\
> > +					CONFIG_k_3) \
> > +       __ALTERNATIVE_CFG(old_c)
> > +
> >  #endif /* __ASSEMBLY__ */
> >  #endif /* CONFIG_RISCV_ALTERNATIVE */
> >  
> > @@ -202,4 +276,24 @@
> >  					new_content_2, vendor_id_2,	\
> >  					    errata_id_2, CONFIG_k_2)
> >  
> > +/*
> > + * A vendor wants to replace an old_content, but another vendor has used
> > + * ALTERNATIVE_2() to patch its customized content at the same location. In
> > + * this case, this vendor can create a new macro ALTERNATIVE_3() based
> > + * on the following sample code and then replace ALTERNATIVE_2() with
> > + * ALTERNATIVE_3() to append its customized content.
> > + */
> > +#define ALTERNATIVE_3(old_content, new_content_1, vendor_id_1,		\
> > +					errata_id_1, CONFIG_k_1,	\
> > +				   new_content_2, vendor_id_2,		\
> > +					errata_id_2, CONFIG_k_2,	\
> > +				   new_content_3, vendor_id_3,		\
> > +					errata_id_3, CONFIG_k_3)	\
> > +	_ALTERNATIVE_CFG_3(old_content, new_content_1, vendor_id_1,	\
> > +					    errata_id_1, CONFIG_k_1,	\
> > +					new_content_2, vendor_id_2,	\
> > +					    errata_id_2, CONFIG_k_2,	\
> > +					new_content_3, vendor_id_3,	\
> > +					    errata_id_3, CONFIG_k_3)
> > +
> >  #endif
>
  

On Thu, Nov 24, 2022 at 08:58:41PM +0100, Heiko Stübner wrote:
> Am Donnerstag, 24. November 2022, 20:52:33 CET schrieb Conor Dooley:
> > On Thu, Nov 24, 2022 at 05:22:01PM +0000, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > 
> > > Introduce ALTERNATIVE_3() macro.
> > 
> > Bit perfunctory I think! There's a lovely comment down below that would
> > make for a better commit message if you were to yoink it.
> > Content looks about what I'd expect to see though.
> 
> Also both the comment on the original ALTERNATIVE_2 and the new ALTERNATIVE_3
> should probably be merged into a single comment explaining this once for all
> ALTERNATIVE_x variants.
> 
> Especially with the dma stuff, I'm pretty sure we'll get at least an ALTERNATIVE_4
> if not even more ;-) . So we defnitly don't want to repeat this multiple times.

Oh I can promise you that there'll be a #4 ;) I do find the comment's
wording to be quite odd though..

> + * A vendor wants to replace an old_content, but another vendor has used
> + * ALTERNATIVE_2() to patch its customized content at the same location. In

In particular this bit about "at the same location" does not make all
that much sense. What "at the same location" means in this context
should be expanded on imo. Effectively it boils down to someone else is
already replacing the same things you want to replace - it's just the
word "location" that might make sense if you're an old hand but not
otherwise?

> + * this case, this vendor can create a new macro ALTERNATIVE_3() based

Also, using the word "can". Is it not a "must" rather than a "can",
since this stuff needs to be multiplatform?

> + * on the following sample code and then replace ALTERNATIVE_2() with
> + * ALTERNATIVE_3() to append its customized content.
  

On 24/11/2022 20:05, Conor Dooley wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On Thu, Nov 24, 2022 at 08:58:41PM +0100, Heiko Stübner wrote:
>> Am Donnerstag, 24. November 2022, 20:52:33 CET schrieb Conor Dooley:
>>> On Thu, Nov 24, 2022 at 05:22:01PM +0000, Prabhakar wrote:
>>>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>>>
>>>> Introduce ALTERNATIVE_3() macro.
>>>
>>> Bit perfunctory I think! There's a lovely comment down below that would
>>> make for a better commit message if you were to yoink it.
>>> Content looks about what I'd expect to see though.
>>
>> Also both the comment on the original ALTERNATIVE_2 and the new ALTERNATIVE_3
>> should probably be merged into a single comment explaining this once for all
>> ALTERNATIVE_x variants.
>>
>> Especially with the dma stuff, I'm pretty sure we'll get at least an ALTERNATIVE_4
>> if not even more ;-) . So we defnitly don't want to repeat this multiple times.
> 
> Oh I can promise you that there'll be a #4 ;) I do find the comment's
> wording to be quite odd though..
> 
>> + * A vendor wants to replace an old_content, but another vendor has used
>> + * ALTERNATIVE_2() to patch its customized content at the same location. In
> 
> In particular this bit about "at the same location" does not make all
> that much sense. What "at the same location" means in this context
> should be expanded on imo. Effectively it boils down to someone else is
> already replacing the same things you want to replace - it's just the
> word "location" that might make sense if you're an old hand but not
> otherwise?

Or maybe I am just biased because I tried to explain this to someone
recently and the language in the comments didn't make sense to them,
and anyone meddling with this code should be able to understand it?

>> + * this case, this vendor can create a new macro ALTERNATIVE_3() based
> 
> Also, using the word "can". Is it not a "must" rather than a "can",
> since this stuff needs to be multiplatform?
> 
>> + * on the following sample code and then replace ALTERNATIVE_2() with
>> + * ALTERNATIVE_3() to append its customized content.
> 
>
  

Am Donnerstag, 24. November 2022, 21:08:17 CET schrieb Conor Dooley:
> On 24/11/2022 20:05, Conor Dooley wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Thu, Nov 24, 2022 at 08:58:41PM +0100, Heiko Stübner wrote:
> >> Am Donnerstag, 24. November 2022, 20:52:33 CET schrieb Conor Dooley:
> >>> On Thu, Nov 24, 2022 at 05:22:01PM +0000, Prabhakar wrote:
> >>>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >>>>
> >>>> Introduce ALTERNATIVE_3() macro.
> >>>
> >>> Bit perfunctory I think! There's a lovely comment down below that would
> >>> make for a better commit message if you were to yoink it.
> >>> Content looks about what I'd expect to see though.
> >>
> >> Also both the comment on the original ALTERNATIVE_2 and the new ALTERNATIVE_3
> >> should probably be merged into a single comment explaining this once for all
> >> ALTERNATIVE_x variants.
> >>
> >> Especially with the dma stuff, I'm pretty sure we'll get at least an ALTERNATIVE_4
> >> if not even more ;-) . So we defnitly don't want to repeat this multiple times.
> > 
> > Oh I can promise you that there'll be a #4 ;) I do find the comment's
> > wording to be quite odd though..
> > 
> >> + * A vendor wants to replace an old_content, but another vendor has used
> >> + * ALTERNATIVE_2() to patch its customized content at the same location. In
> > 
> > In particular this bit about "at the same location" does not make all
> > that much sense. What "at the same location" means in this context
> > should be expanded on imo. Effectively it boils down to someone else is
> > already replacing the same things you want to replace - it's just the
> > word "location" that might make sense if you're an old hand but not
> > otherwise?
> 
> Or maybe I am just biased because I tried to explain this to someone
> recently and the language in the comments didn't make sense to them,
> and anyone meddling with this code should be able to understand it?

When I first looked at the whole alternatives / patching thing, the whole thing
looked like dark magic to me ;-) .

But yeah, the comment here, but also the original one above ALTERNATIVE_2
could use improvements to explain better what it tries to do.


> >> + * this case, this vendor can create a new macro ALTERNATIVE_3() based
> > 
> > Also, using the word "can". Is it not a "must" rather than a "can",
> > since this stuff needs to be multiplatform?
> > 
> >> + * on the following sample code and then replace ALTERNATIVE_2() with
> >> + * ALTERNATIVE_3() to append its customized content.
> > 
> > 
> 
>
  

Hi Heiko,

On Thu, Nov 24, 2022 at 7:58 PM Heiko Stübner <heiko@sntech.de> wrote:
>
> Am Donnerstag, 24. November 2022, 20:52:33 CET schrieb Conor Dooley:
> > On Thu, Nov 24, 2022 at 05:22:01PM +0000, Prabhakar wrote:
> > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > >
> > > Introduce ALTERNATIVE_3() macro.
> >
> > Bit perfunctory I think! There's a lovely comment down below that would
> > make for a better commit message if you were to yoink it.
> > Content looks about what I'd expect to see though.
>
> Also both the comment on the original ALTERNATIVE_2 and the new ALTERNATIVE_3
> should probably be merged into a single comment explaining this once for all
> ALTERNATIVE_x variants.
>
> Especially with the dma stuff, I'm pretty sure we'll get at least an ALTERNATIVE_4
> if not even more ;-) . So we defnitly don't want to repeat this multiple times.
>
Do agree. How about the below?

/*
 * Similar to what ALTERNATIVE_2() macro does but with an additional
 * vendor content.
 */

So the other ALTERNATIVE_2+() macros will keep on building on it.

Cheers,
Prabhakar
  

Am Freitag, 25. November 2022, 11:02:21 CET schrieb Lad, Prabhakar:
> Hi Heiko,
> 
> On Thu, Nov 24, 2022 at 7:58 PM Heiko Stübner <heiko@sntech.de> wrote:
> >
> > Am Donnerstag, 24. November 2022, 20:52:33 CET schrieb Conor Dooley:
> > > On Thu, Nov 24, 2022 at 05:22:01PM +0000, Prabhakar wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > >
> > > > Introduce ALTERNATIVE_3() macro.
> > >
> > > Bit perfunctory I think! There's a lovely comment down below that would
> > > make for a better commit message if you were to yoink it.
> > > Content looks about what I'd expect to see though.
> >
> > Also both the comment on the original ALTERNATIVE_2 and the new ALTERNATIVE_3
> > should probably be merged into a single comment explaining this once for all
> > ALTERNATIVE_x variants.
> >
> > Especially with the dma stuff, I'm pretty sure we'll get at least an ALTERNATIVE_4
> > if not even more ;-) . So we defnitly don't want to repeat this multiple times.
> >
> Do agree. How about the below?
> 
> /*
>  * Similar to what ALTERNATIVE_2() macro does but with an additional
>  * vendor content.
>  */
> 
> So the other ALTERNATIVE_2+() macros will keep on building on it.

My idea was more like having _one_ comment block of something like

-----
/*
 * ALTERNATIVE_x macros allow providing multiple replacement options
 * for an ALTERNATIVE code section. This is helpful if multiple
 * implementation variants for the same functionality exist for
 * different cpu cores.
 *
 * Usage:
 *   ALTERNATIVE_x(old_content,
 *	new_content1, vendor_id1, errata_id1, CONFIG_k1,
 *	new_content2, vendor_id2, errata_id2, CONFIG_k2,
 *	...
 *	new_contentx, vendor_idx, errata_idx, CONFIG_kx)
 */

#define ALTERNATIVE_2(...)
#define ALTERNATIVE_3(...)
etc
-----

So this would include dropping the old comment over ALTERNATIVE2


Heiko
  

Hi Heiko,

On Fri, Nov 25, 2022 at 10:20 AM Heiko Stübner <heiko@sntech.de> wrote:
>
> Am Freitag, 25. November 2022, 11:02:21 CET schrieb Lad, Prabhakar:
> > Hi Heiko,
> >
> > On Thu, Nov 24, 2022 at 7:58 PM Heiko Stübner <heiko@sntech.de> wrote:
> > >
> > > Am Donnerstag, 24. November 2022, 20:52:33 CET schrieb Conor Dooley:
> > > > On Thu, Nov 24, 2022 at 05:22:01PM +0000, Prabhakar wrote:
> > > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > >
> > > > > Introduce ALTERNATIVE_3() macro.
> > > >
> > > > Bit perfunctory I think! There's a lovely comment down below that would
> > > > make for a better commit message if you were to yoink it.
> > > > Content looks about what I'd expect to see though.
> > >
> > > Also both the comment on the original ALTERNATIVE_2 and the new ALTERNATIVE_3
> > > should probably be merged into a single comment explaining this once for all
> > > ALTERNATIVE_x variants.
> > >
> > > Especially with the dma stuff, I'm pretty sure we'll get at least an ALTERNATIVE_4
> > > if not even more ;-) . So we defnitly don't want to repeat this multiple times.
> > >
> > Do agree. How about the below?
> >
> > /*
> >  * Similar to what ALTERNATIVE_2() macro does but with an additional
> >  * vendor content.
> >  */
> >
> > So the other ALTERNATIVE_2+() macros will keep on building on it.
>
> My idea was more like having _one_ comment block of something like
>
> -----
> /*
>  * ALTERNATIVE_x macros allow providing multiple replacement options
>  * for an ALTERNATIVE code section. This is helpful if multiple
>  * implementation variants for the same functionality exist for
>  * different cpu cores.
>  *
>  * Usage:
>  *   ALTERNATIVE_x(old_content,
>  *      new_content1, vendor_id1, errata_id1, CONFIG_k1,
>  *      new_content2, vendor_id2, errata_id2, CONFIG_k2,
>  *      ...
>  *      new_contentx, vendor_idx, errata_idx, CONFIG_kx)
>  */
>
> #define ALTERNATIVE_2(...)
> #define ALTERNATIVE_3(...)
> etc
> -----
>
LGTM, I'll include the above in the next version.

> So this would include dropping the old comment over ALTERNATIVE2
>
Agreed, I'll drop it in the same patch.

Cheers,
Prabhakar
  

On Thu, Nov 24, 2022 at 08:05:40PM +0000, Conor Dooley wrote:
> On Thu, Nov 24, 2022 at 08:58:41PM +0100, Heiko Stübner wrote:
> > Am Donnerstag, 24. November 2022, 20:52:33 CET schrieb Conor Dooley:
> > > On Thu, Nov 24, 2022 at 05:22:01PM +0000, Prabhakar wrote:
> > > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > > 
> > > > Introduce ALTERNATIVE_3() macro.
> > > 
> > > Bit perfunctory I think! There's a lovely comment down below that would
> > > make for a better commit message if you were to yoink it.
> > > Content looks about what I'd expect to see though.
> > 
> > Also both the comment on the original ALTERNATIVE_2 and the new ALTERNATIVE_3
> > should probably be merged into a single comment explaining this once for all
> > ALTERNATIVE_x variants.
> > 
> > Especially with the dma stuff, I'm pretty sure we'll get at least an ALTERNATIVE_4
> > if not even more ;-) . So we defnitly don't want to repeat this multiple times.
> 
> Oh I can promise you that there'll be a #4 ;)

I took a stab[*] at cleaning up alternative-macros.h a bit in order to
prepare for _3, _4, ..., _42. The idea was to try and find a way to
reduce as much duplication as possible, both in the current code and
in the new macros.

[*] https://lore.kernel.org/all/20221125113959.35328-1-ajones@ventanamicro.com/

Thanks,
drew