Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
User-Agent: Cyrus-JMAP/3.5.0-alpha0-1229-g7ca81dfce5-fm-20210908.005-g7ca81dfc
Mime-Version: 1.0
Message-Id: <758441b2-9b04-4e6a-8182-741ae1f858ff@www.fastmail.com>
In-Reply-To: <YTfQGqLv6FLnUfv+@sunset>
References: <20210907145501.69161-1-sven@svenpeter.dev>
 <20210907145501.69161-3-sven@svenpeter.dev> <YTe17jGBobarePaK@sunset>
 <ff66c30d-1b43-43d3-a4b0-02fe7d346118@www.fastmail.com>
 <YTfQGqLv6FLnUfv+@sunset>
Date:   Wed, 08 Sep 2021 17:36:30 +0200
From:   "Sven Peter" <sven@svenpeter.dev>
To:     "Alyssa Rosenzweig" <alyssa@rosenzweig.io>
Cc:     "Jassi Brar" <jassisinghbrar@gmail.com>,
        "Rob Herring" <robh+dt@kernel.org>,
        "Mark Kettenis" <mark.kettenis@xs4all.nl>,
        "Hector Martin" <marcan@marcan.st>,
        "Mohamed Mediouni" <mohamed.mediouni@caramail.com>,
        "Stan Skowronek" <stan@corellium.com>, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/3] dt-bindings: mailbox: Add Apple mailbox bindings
Content-Type: text/plain;charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


On Tue, Sep 7, 2021, at 22:48, Alyssa Rosenzweig wrote:
> > > > +      - description:
> > > > +          M3 mailboxes are an older variant with a slightly dif=
ferent MMIO
> > > > +          interface still found on the M1.
> > > > +        items:
> > > > +          - const: apple,t8103-m3-mailbox
> > >=20
> > > Would be nice to document an example of where an M3 mailbox is fou=
nd.
> >=20
> > Sure, I can add a comment that this is used for the coprocessor cont=
rolling Thunderbolt.
>=20
> That raises another issue ... how do we know the M3 code works at all
> without TB support yet? It "looks" correct but some of the IRQ handling
> stuff is nontrivial.

Enabling the mailbox interface just requires a few clocks to be ungated.
Then I injected messages manually to verify that the code works.
In addition I also brought up parts of the Thunderbolt controller which
then allowed the co-processor on the other end of the mailbox to boot.
After that I was also able to successfully talk to that processor using
the same protocol used by most other processors.


>=20
> > > > +  interrupts:
> > > > +    minItems: 4
> > > > +    items:
> > > > +      - description: send fifo is empty interrupt
> > > > +      - description: send fifo is not empty interrupt
> > > > +      - description: receive fifo is empty interrupt
> > > > +      - description: receive fifo is not empty interrupt
> > > > +
> > > > +  interrupt-names:
> > > > +    minItems: 4
> > > > +    items:
> > > > +      - const: send-empty
> > > > +      - const: send-not-empty
> > > > +      - const: recv-empty
> > > > +      - const: recv-not-empty
> > >=20
> > > If the names became not-constant the asprintf thing goes away, not=
 sure
> > > that's better or worse.
> >=20
> > I'm not sure I understand your comment here. This property just give=
s a name
> > to the interrupts so that they can be referenced by that instead of =
a magic
> > number between 0 and 4 in the driver.
>=20
> D'oh, right, retracted. (Both this comment and the corresponding comme=
nt
> on the driver itself). Sorry about that.
>=20
> > > > +  clocks:
> > > > +    description:
> > > > +      Reference to the clock gate phandle(s) if required for th=
is mailbox.
> > > > +      Optional since not all mailboxes are attached to a clock =
gate.
> > >=20
> > > Do we do anything with the clocks at this point?
> > >=20
> >=20
> > The device tree bindings describe the hardware (as best as we can wi=
thout proper
> > documentation) and some of these mailboxes have clock gates which ne=
ed to be turned
> > on before accessing their MMIO. This driver already tries to do that=
 and works fine
> > with the downstream clock driver(s) we have.
>=20
> Good enough for me, thanks for clarifying =F0=9F=91=8D
>=20
> Commit r-b, though Rob will surely point out problems and I'll need to
> rereview =F0=9F=98=89
>=20

Thanks!


Sven