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;
Feedback-ID: i51094778:Fastmail
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
From:   Sven Peter <sven@svenpeter.dev>
Mime-Version: 1.0 (1.0)
Subject: Re: [PATCH v4 2/2] irqchip/apple-aic: Add support for A7-A11 SoCs
Date:   Sun, 9 Oct 2022 11:57:41 +0200
Message-Id: <6B4AB90E-49B4-443E-ABB1-632912D94CE3@svenpeter.dev>
References: <45b9c3f6-af45-e22b-06e6-ae2a2e5bba7a@somainline.org>
Cc:     ~postmarketos/upstreaming@lists.sr.ht, asahi@lists.linux.dev,
        Hector Martin <marcan@marcan.st>,
        Alyssa Rosenzweig <alyssa@rosenzweig.io>,
        martin.botka@somainline.org,
        angelogioacchino.delregno@somainline.org,
        marijn.suijten@somainline.org, jamipkettunen@somainline.org,
        Thomas Gleixner <tglx@linutronix.de>,
        Marc Zyngier <maz@kernel.org>,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
In-Reply-To: <45b9c3f6-af45-e22b-06e6-ae2a2e5bba7a@somainline.org>
To:     Konrad Dybcio <konrad.dybcio@somainline.org>
Precedence: bulk

Hi,

> On 8. Oct 2022, at 16:06, Konrad Dybcio <konrad.dybcio@somainline.org> wro=
te:
>=20
> =EF=BB=BF
>=20
>> On 8.10.2022 11:33, Sven Peter wrote:
>>=20
>> Hi,
>>=20
>>>> On 7. Oct 2022, at 22:00, Konrad Dybcio <konrad.dybcio@somainline.org> w=
rote:
>>>=20
>>> =EF=BB=BFAdd support for A7-A11 SoCs by if-ing out some features only pr=
esent
>>> on A11 & newer (implementation-defined IPI & UNCORE registers).
>>>=20
>>> Also, annotate IPI regs support in the aic struct so that the driver
>>> can tell whether the SoC supports these, as they are written to,
>>> even if fast IPI is disabled.
>>=20
>> No.
>>=20
>>> This in turn causes a crash on older
>>> platforms, as the implemention-defined registers either do
>>> something else or are not supposed to be touched - definitely not a
>>> NOP though.
>>=20
>> This entire description needs to be rewritten. All you want to do is guar=
d both fastipi and uncore reg access on pre-A11.
>>=20
>>>=20
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@somainline.org>
>>> ---
>>> Changes since v3:
>>> - Replace use_fast_ipi with has_uncore_ipi_regs in aic_init_cpu
>>> (logic error, this was written to regardless of FIPI usage before,
>>> but touching Sn_... regs on SoCs that don't explicitly use them for
>>> IPIs makes them sepuku..)
>>> - Drop A11 compatible
>>>=20
>>> drivers/irqchip/irq-apple-aic.c | 47 ++++++++++++++++++++++-----------
>>> 1 file changed, 32 insertions(+), 15 deletions(-)
>>>=20
>>> diff --git a/drivers/irqchip/irq-apple-aic.c b/drivers/irqchip/irq-apple=
-aic.c
>>> index 1c2813ad8bbe..2609d6b60487 100644
>>> --- a/drivers/irqchip/irq-apple-aic.c
>>> +++ b/drivers/irqchip/irq-apple-aic.c
>>> @@ -230,6 +230,9 @@
>>>=20
>>> static DEFINE_STATIC_KEY_TRUE(use_fast_ipi);
>>>=20
>>> +/* True if UNCORE/UNCORE2 and Sn_... IPI registers are present (A11+) *=
/
>>> +static DEFINE_STATIC_KEY_TRUE(has_uncore_ipi_regs);
>>> +
>>> struct aic_info {
>>>   int version;
>>>=20
>>> @@ -246,6 +249,7 @@ struct aic_info {
>>>=20
>>>   /* Features */
>>>   bool fast_ipi;
>>> +    bool uncore_ipi_regs;
>>=20
>> Why two flags? Didn=E2=80=99t we come to the conclusion last time that fa=
stipi and uncore were introduced at the same time? Below you also either hav=
e both true or both false so there=E2=80=99s really no need to track both of=
 them.
>>=20
>>=20
>>> };
>>>=20
>>> static const struct aic_info aic1_info =3D {
>>> @@ -261,6 +265,7 @@ static const struct aic_info aic1_fipi_info =3D {
>>>   .event        =3D AIC_EVENT,
>>>   .target_cpu    =3D AIC_TARGET_CPU,
>>>=20
>>> +    .uncore_ipi_regs    =3D true,
>>>   .fast_ipi    =3D true,
>>> };
>>>=20
>>> @@ -269,6 +274,7 @@ static const struct aic_info aic2_info =3D {
>>>=20
>>>   .irq_cfg    =3D AIC2_IRQ_CFG,
>>>=20
>>> +    .uncore_ipi_regs    =3D true,
>>>   .fast_ipi    =3D true,
>>> };
>>>=20
>>> @@ -524,12 +530,14 @@ static void __exception_irq_entry aic_handle_fiq(s=
truct pt_regs *regs)
>>>    * we check for everything here, even things we don't support yet.
>>>    */
>>>=20
>>> -    if (read_sysreg_s(SYS_IMP_APL_IPI_SR_EL1) & IPI_SR_PENDING) {
>>> -        if (static_branch_likely(&use_fast_ipi)) {
>>> -            aic_handle_ipi(regs);
>>> -        } else {
>>> -            pr_err_ratelimited("Fast IPI fired. Acking.\n");
>>> -            write_sysreg_s(IPI_SR_PENDING, SYS_IMP_APL_IPI_SR_EL1);
>>> +    if (static_branch_likely(&has_uncore_ipi_regs)) {
>>> +        if (read_sysreg_s(SYS_IMP_APL_IPI_SR_EL1) & IPI_SR_PENDING) {
>>> +            if (static_branch_likely(&use_fast_ipi)) {
>>> +                aic_handle_ipi(regs);
>>> +            } else {
>>> +                pr_err_ratelimited("Fast IPI fired. Acking.\n");
>>> +                write_sysreg_s(IPI_SR_PENDING, SYS_IMP_APL_IPI_SR_EL1);=

>>=20
>> This still can=E2=80=99t be reached because both static branches will alw=
ays have the same value. Didn=E2=80=99t we also realize a version or two ago=
 that this can just be dropped?
>>=20
> Ok, so I didn't realize you wanted this to become a single variable - I th=
ought it would have
> been useful to keep them separate, as A7-A10 *should* use fast IPIs as far=
 as I'm aware, but
> they don't use the impl-defined registers for that (or at least not the sa=
me ones).

What makes you think that?=20

>=20
> For the sake of this patch, I can squash it into one as all known users to=
 date set both in the
> current form. Also, before this patch, "apple,aic" used to essentially be h=
as_uncore_ipi_regs=3Dtrue,
> use_fast_ipi=3Dfalse, but since there are no users, I assume that combinat=
ion is not useful to keep
> around?
>=20

That combination just doesn=E2=80=99t make any sense. It translates to =E2=80=
=9Cwe know the chip supports fastipi but we=E2=80=99re not gonna use it beca=
use we want everything to be slower!=E2=80=9D.

As I said before, I=E2=80=99m pretty sure that else block was just a leftove=
r from when this driver didn=E2=80=99t support fastipi at all but we already=
 knew that M1 supported it.


Sven