| Message ID | 20221124172207.153718-2-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Palmer Dabbelt |
| Headers | show |
| Series | AX45MP: Add support to non-coherent DMA | expand |
| Context | Check | Description |
|---|---|---|
| conchuod/patch_count | success | Link |
| conchuod/cover_letter | success | Series has a cover letter |
| conchuod/tree_selection | success | Guessed tree name to be for-next |
| conchuod/fixes_present | success | Fixes tag not required for -next series |
| conchuod/verify_signedoff | success | Signed-off-by tag matches author and committer |
| conchuod/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/module_param | success | Was 0 now: 0 |
| conchuod/build_rv32_defconfig | success | Build OK |
| conchuod/build_warn_rv64 | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/dtb_warn_rv64 | success | Errors and warnings before: 0 this patch: 0 |
| conchuod/header_inline | success | No static functions without inline keyword in header files |
| conchuod/checkpatch | fail | ERROR: Macros with complex values should be enclosed in parentheses WARNING: Avoid unnecessary line continuations WARNING: please, no spaces at the start of a line |
| conchuod/source_inline | success | Was 0 now: 0 |
| conchuod/build_rv64_nommu_k210_defconfig | success | Build OK |
| conchuod/verify_fixes | success | No Fixes tag |
| conchuod/build_rv64_nommu_virt_defconfig | success | Build OK |
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
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