Re: [PATCH V7 2/6] arm64/perf: Add BRBE registers and fields

From: Mark Rutland
Date: Thu Feb 09 2023 - 05:08:51 EST

Next message: Sverdlin, Alexander: "Re: [PATCH] net: fec: Defer probe if other FEC has deferred MDIO"
Previous message: Jani Nikula: "Re: [PATCH] drm/amdgpu/display: remove duplicate include header in link_dpms.c"
In reply to: Anshuman Khandual: "Re: [PATCH V7 2/6] arm64/perf: Add BRBE registers and fields"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Feb 09, 2023 at 11:19:04AM +0530, Anshuman Khandual wrote:
> On 2/9/23 00:52, Mark Rutland wrote:
> > I'd prefer that we fix gen-sysreg.awk to support Enum blocks within
> > SysregFields blocks (patch below), then use SysregFields as described above.
>
> The following patch did not apply cleanly on v6.2-rc7 but eventually did so after
> some changes. Is the patch against mainline or arm64-next ?

Sorry I forgot to say: that needs the arm64 for-next/sysreg-hwcaps branch
(which is merged into arm64 for-next/core).

> Nonetheless, it does
> solve the enum problem for SysregFields. With this patch in place, I was able to
>
> - Change Sysreg BRBINF_EL1 as SysregFields BRBINFx_EL1
> - Change BRBINF_EL1_XXX fields usage as BRBINFx_EL1_XXX fields

Nice!

> Should I take this patch with this series as an initial prerequisite patch or you
> would like to post this now for current merge window ?

I think for now it's best to add it to this series as a prerequisite.

Thanks,
Mark.

>
> >
> > Thanks,
> > Mark.
> >
> > ---->8----
> >>From 0c194d92b0b9ff3b32f666a4610b077fdf1b4b93 Mon Sep 17 00:00:00 2001
> > From: Mark Rutland <mark.rutland@xxxxxxx>
> > Date: Wed, 8 Feb 2023 17:55:08 +0000
> > Subject: [PATCH] arm64/sysreg: allow *Enum blocks in SysregFields blocks
> >
> > We'd like to support Enum/SignedEnum/UnsignedEnum blocks within
> > SysregFields blocks, so that we can define enumerations for sets of
> > registers. This isn't currently supported by gen-sysreg.awk due to the
> > way we track the active block, which can't handle more than a single
> > layer of nesting, which imposes an awkward requirement that when ending
> > a block we know what the parent block is when calling change_block()
> >
> > Make this nicer by using a stack of active blocks, with block_push() to
> > start a block, and block_pop() to end a block. Doing so means hat when
> > ending a block we don't need to know the parent block type, and checks
> > of the active block become more consistent. On top of that, it's easy to
> > permit *Enum blocks within both Sysreg and SysregFields blocks.
> >
> > To aid debugging, the stack of active blocks is reported for fatal
> > errors, and an error is raised if the file is terminated without ending
> > the active block. For clarity I've renamed the top-level element from
> > "None" to "Root".
> >
> > The Fields element it intended only for use within Systeg blocks, and
> > does not make sense within SysregFields blocks, and so remains forbidden
> > within a SysregFields block.
> >
> > Signed-off-by: Mark Rutland <mark.rutland@xxxxxxx>
> > Cc: Anshuman Khandual <anshuman.khandual@xxxxxxx>
> > Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
> > Cc: Mark Brown <broonie@xxxxxxxxxx>
> > Cc: Will Deacon <will@xxxxxxxxxx>
> > ---
> > arch/arm64/tools/gen-sysreg.awk | 93 ++++++++++++++++++++-------------
> > 1 file changed, 57 insertions(+), 36 deletions(-)
> >
> > diff --git a/arch/arm64/tools/gen-sysreg.awk b/arch/arm64/tools/gen-sysreg.awk
> > index 7f27d66a17e1..066ebf5410fa 100755
> > --- a/arch/arm64/tools/gen-sysreg.awk
> > +++ b/arch/arm64/tools/gen-sysreg.awk
> > @@ -4,23 +4,35 @@
> > #
> > # Usage: awk -f gen-sysreg.awk sysregs.txt
> >
> > +function block_current() {
> > + return __current_block[__current_block_depth];
> > +}
> > +
> > # Log an error and terminate
> > function fatal(msg) {
> > print "Error at " NR ": " msg > "/dev/stderr"
> > +
> > + printf "Current block nesting:"
> > +
> > + for (i = 0; i <= __current_block_depth; i++) {
> > + printf " " __current_block[i]
> > + }
> > + printf "\n"
> > +
> > exit 1
> > }
> >
> > -# Sanity check that the start or end of a block makes sense at this point in
> > -# the file. If not, produce an error and terminate.
> > -#
> > -# @this - the $Block or $EndBlock
> > -# @prev - the only valid block to already be in (value of @block)
> > -# @new - the new value of @block
> > -function change_block(this, prev, new) {
> > - if (block != prev)
> > - fatal("unexpected " this " (inside " block ")")
> > -
> > - block = new
> > +# Enter a new block, setting the active block to @block
> > +function block_push(block) {
> > + __current_block[++__current_block_depth] = block
> > +}
> > +
> > +# Exit a block, setting the active block to the parent block
> > +function block_pop() {
> > + if (__current_block_depth == 0)
> > + fatal("error: block_pop() in root block")
> > +
> > + __current_block_depth--;
> > }
> >
> > # Sanity check the number of records for a field makes sense. If not, produce
> > @@ -84,10 +96,14 @@ BEGIN {
> > print "/* Generated file - do not edit */"
> > print ""
> >
> > - block = "None"
> > + __current_block_depth = 0
> > + __current_block[__current_block_depth] = "Root"
> > }
> >
> > END {
> > + if (__current_block_depth != 0)
> > + fatal("Missing terminator for " block_current() " block")
> > +
> > print "#endif /* __ASM_SYSREG_DEFS_H */"
> > }
> >
> > @@ -95,8 +111,9 @@ END {
> > /^$/ { next }
> > /^[\t ]*#/ { next }
> >
> > -/^SysregFields/ {
> > - change_block("SysregFields", "None", "SysregFields")
> > +/^SysregFields/ && block_current() == "Root" {
> > + block_push("SysregFields")
> > +
> > expect_fields(2)
> >
> > reg = $2
> > @@ -109,12 +126,10 @@ END {
> > next
> > }
> >
> > -/^EndSysregFields/ {
> > +/^EndSysregFields/ && block_current() == "SysregFields" {
> > if (next_bit > 0)
> > fatal("Unspecified bits in " reg)
> >
> > - change_block("EndSysregFields", "SysregFields", "None")
> > -
> > define(reg "_RES0", "(" res0 ")")
> > define(reg "_RES1", "(" res1 ")")
> > print ""
> > @@ -123,11 +138,13 @@ END {
> > res0 = null
> > res1 = null
> >
> > + block_pop()
> > next
> > }
> >
> > -/^Sysreg/ {
> > - change_block("Sysreg", "None", "Sysreg")
> > +/^Sysreg/ && block_current() == "Root" {
> > + block_push("Sysreg")
> > +
> > expect_fields(7)
> >
> > reg = $2
> > @@ -156,12 +173,10 @@ END {
> > next
> > }
> >
> > -/^EndSysreg/ {
> > +/^EndSysreg/ && block_current() == "Sysreg" {
> > if (next_bit > 0)
> > fatal("Unspecified bits in " reg)
> >
> > - change_block("EndSysreg", "Sysreg", "None")
> > -
> > if (res0 != null)
> > define(reg "_RES0", "(" res0 ")")
> > if (res1 != null)
> > @@ -178,12 +193,13 @@ END {
> > res0 = null
> > res1 = null
> >
> > + block_pop()
> > next
> > }
> >
> > # Currently this is effectivey a comment, in future we may want to emit
> > # defines for the fields.
> > -/^Fields/ && (block == "Sysreg") {
> > +/^Fields/ && block_current() == "Sysreg" {
> > expect_fields(2)
> >
> > if (next_bit != 63)
> > @@ -200,7 +216,7 @@ END {
> > }
> >
> >
> > -/^Res0/ && (block == "Sysreg" || block == "SysregFields") {
> > +/^Res0/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> > expect_fields(2)
> > parse_bitdef(reg, "RES0", $2)
> > field = "RES0_" msb "_" lsb
> > @@ -210,7 +226,7 @@ END {
> > next
> > }
> >
> > -/^Res1/ && (block == "Sysreg" || block == "SysregFields") {
> > +/^Res1/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> > expect_fields(2)
> > parse_bitdef(reg, "RES1", $2)
> > field = "RES1_" msb "_" lsb
> > @@ -220,7 +236,7 @@ END {
> > next
> > }
> >
> > -/^Field/ && (block == "Sysreg" || block == "SysregFields") {
> > +/^Field/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> > expect_fields(3)
> > field = $3
> > parse_bitdef(reg, field, $2)
> > @@ -231,15 +247,16 @@ END {
> > next
> > }
> >
> > -/^Raz/ && (block == "Sysreg" || block == "SysregFields") {
> > +/^Raz/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> > expect_fields(2)
> > parse_bitdef(reg, field, $2)
> >
> > next
> > }
> >
> > -/^SignedEnum/ {
> > - change_block("Enum<", "Sysreg", "Enum")
> > +/^SignedEnum/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> > + block_push("Enum")
> > +
> > expect_fields(3)
> > field = $3
> > parse_bitdef(reg, field, $2)
> > @@ -250,8 +267,9 @@ END {
> > next
> > }
> >
> > -/^UnsignedEnum/ {
> > - change_block("Enum<", "Sysreg", "Enum")
> > +/^UnsignedEnum/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> > + block_push("Enum")
> > +
> > expect_fields(3)
> > field = $3
> > parse_bitdef(reg, field, $2)
> > @@ -262,8 +280,9 @@ END {
> > next
> > }
> >
> > -/^Enum/ {
> > - change_block("Enum", "Sysreg", "Enum")
> > +/^Enum/ && (block_current() == "Sysreg" || block_current() == "SysregFields") {
> > + block_push("Enum")
> > +
> > expect_fields(3)
> > field = $3
> > parse_bitdef(reg, field, $2)
> > @@ -273,16 +292,18 @@ END {
> > next
> > }
> >
> > -/^EndEnum/ {
> > - change_block("EndEnum", "Enum", "Sysreg")
> > +/^EndEnum/ && block_current() == "Enum" {
> > +
> > field = null
> > msb = null
> > lsb = null
> > print ""
> > +
> > + block_pop()
> > next
> > }
> >
> > -/0b[01]+/ && block == "Enum" {
> > +/0b[01]+/ && block_current() == "Enum" {
> > expect_fields(2)
> > val = $1
> > name = $2

Next message: Sverdlin, Alexander: "Re: [PATCH] net: fec: Defer probe if other FEC has deferred MDIO"
Previous message: Jani Nikula: "Re: [PATCH] drm/amdgpu/display: remove duplicate include header in link_dpms.c"
In reply to: Anshuman Khandual: "Re: [PATCH V7 2/6] arm64/perf: Add BRBE registers and fields"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]