Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220825-arm-spe-v8-7-v2-0-e37322d68ac0@kernel.org>
 <20220825-arm-spe-v8-7-v2-3-e37322d68ac0@kernel.org> <Y1FcRNhFpZxZ7AMx@sirena.org.uk>
In-Reply-To: <Y1FcRNhFpZxZ7AMx@sirena.org.uk>
From:   Rob Herring <robh@kernel.org>
Date:   Thu, 20 Oct 2022 14:51:02 -0500
Message-ID: <CAL_JsqLVjWH-x3jxGQpJ3ax_ddvBqHd-P2DE=DEnrknoHuq+Qg@mail.gmail.com>
Subject: Re: [PATCH v2 3/7] arm64/sysreg: Convert SPE registers to automatic generation
To:     Mark Brown <broonie@kernel.org>
Cc:     Namhyung Kim <namhyung@kernel.org>,
        James Morse <james.morse@arm.com>,
        Ingo Molnar <mingo@redhat.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Marc Zyngier <maz@kernel.org>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        Will Deacon <will@kernel.org>,
        Alexandru Elisei <alexandru.elisei@arm.com>,
        Catalin Marinas <catalin.marinas@arm.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Oliver Upton <oliver.upton@linux.dev>,
        Jiri Olsa <jolsa@kernel.org>, kvmarm@lists.cs.columbia.edu,
        kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
        linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Oct 20, 2022 at 9:33 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Wed, Oct 19, 2022 at 02:11:26PM -0500, Rob Herring wrote:
> > Convert all the SPE register defines to automatic generation. No
> > functional changes.
> >
> > New registers and fields for SPEv1.2 are added with the conversion.
> >
> > Some of the PMBSR MSS field defines are kept as the automatic generation
> > has no way to create multiple names for the same register bits. The
> > meaning of the MSS field depends on other bits.
>
> A few small things below from checking against DDI0487I.a, nothing
> major:

[...]

> > +Sysreg       PMSIDR_EL1      3       0       9       9       7
>
> > +Field        23:20   FORMAT
> > +Field        19:16   COUNTSIZE
> > +Field        15:12   MAXSIZE
> > +Field        11:8    INTERVAL
>
> These should really be enums.

Okay for COUNTSIZE and INTERVAL.

The only defined value for FORMAT is 'Format 0'. I assume next will be
'Format 1'. I don't think defines with the value in the name are too
useful. When this is used, we'll just be passing the value to
userspace via either sysfs or perf aux buffer.

MAXSIZE is the power of 2 encoded size and we just convert to bytes
(1<<MAXSIZE). So I can add them, but they will never be used.

> > +Sysreg       PMBLIMITR_EL1   3       0       9       10      0
> > +Enum 2:1     FM
> > +     0b0000  STOP_IRQ
> > +EndEnum
>
> DDI0487I.a also defines
>
>         0b01    DISCARD

STOP_IRQ doesn't seem like the best name either. It's not used, so I
changed it to 'FILL'.

> > +Sysreg       PMBIDR_EL1      3       0       9       10      7
> > +Res0 63:12
> > +Field        11:8    EA
>
> This looks like it should be described as an enum.

    0b0000    Not_Described
    0b0001    Ignored
    0b0010    SError

What's the preferred case here?

>
> > +Field        3:0     ALIGN
>
> This could potentially also be an enum.

Another power of 2 encoding.

> > +Sysreg       PMSCR_EL2       3       4       9       9       0
> > +Res0 63:8
> > +Field        7:6     PCT
>
> This lookslike it should be an enum.

Humm, PCT is a bit tricky. Here's what I came up with:

Enum    7:6    PCT
    0b00    VIRT
    0b01    PHYS
    0b11    GUEST
EndEnum

Again, CAPS or CarrotCase for new things?

Rob