On 21/03/2022 17:50, Sven Peter wrote:
> Apple SoCs such as the M1 come with multiple embedded co-processors
> running proprietary firmware. Communication with those is established
> over a simple mailbox using the RTKit IPC protocol.
>
> Signed-off-by: Sven Peter <[email protected]>
> ---
> drivers/soc/apple/Kconfig | 13 +
> drivers/soc/apple/Makefile | 3 +
> drivers/soc/apple/rtkit-crashlog.c | 147 +++++
> drivers/soc/apple/rtkit-internal.h | 76 +++
> drivers/soc/apple/rtkit.c | 842 +++++++++++++++++++++++++++++
> include/linux/soc/apple/rtkit.h | 203 +++++++
> 6 files changed, 1284 insertions(+)
Isn't this some implementation of a mailbox? If so, it should be in
drivers/mailbox. Please don't put all stuff in soc/apple, that's not how
Linux is organized. To drivers/soc usually we put drivers which do not
fit regular subsystems.
Best regards,
Krzysztof
On Wed, Mar 23, 2022, at 12:19, Krzysztof Kozlowski wrote:
> On 21/03/2022 17:50, Sven Peter wrote:
>> Apple SoCs such as the M1 come with multiple embedded co-processors
>> running proprietary firmware. Communication with those is established
>> over a simple mailbox using the RTKit IPC protocol.
>>
>> Signed-off-by: Sven Peter <[email protected]>
>> ---
>> drivers/soc/apple/Kconfig | 13 +
>> drivers/soc/apple/Makefile | 3 +
>> drivers/soc/apple/rtkit-crashlog.c | 147 +++++
>> drivers/soc/apple/rtkit-internal.h | 76 +++
>> drivers/soc/apple/rtkit.c | 842 +++++++++++++++++++++++++++++
>> include/linux/soc/apple/rtkit.h | 203 +++++++
>> 6 files changed, 1284 insertions(+)
>
> Isn't this some implementation of a mailbox? If so, it should be in
> drivers/mailbox. Please don't put all stuff in soc/apple, that's not how
> Linux is organized. To drivers/soc usually we put drivers which do not
> fit regular subsystems.
>
I put this into soc/apple because I don't think it fits within the mailbox
framework very well.
(It actually uses the mailbox framework for the actual communication
with the hardware with a driver that's already upstream.)
Essentially, the mailbox subsystem provides a common API to send and
receive messages over indepedent hardware channels and devicetree bindings
to describe the relationship between those channels and other drivers.
One of the features that doesn't really fit is that we need to be able
to start, shutdown and re-start these co-processors. The NVMe driver
actually doesn't need to send/receive any messages except those required
to setup the common syslog/crashlog/etc. interfaces.
The mailbox framework would have to be extended to support these specific
use cases.
Another thing that doesn't fit is the memory management: These co-processors
sometimes need shared memory buffers to e.g. send syslog messages.
They always request these buffers with an IPC message but then there are
different possibilities:
- For some processor the DMA API can just be used and an IOVA must be
sent back. For NVMe these buffers must additionally be allowed in this
SART address filter.
- At least one other processor (SMC) does not request such buffers but
instead just sends a pointer into MMIO space and the buffer must be
accessed using readl/writel. This MMIO memory region is used for
both the common buffers (syslog etc.) and for the actual shared buffers
used for communication, such that the resource would have to be shared
across drivers.
- And yet another coprocessor (for the display controller) requests some
buffers with an already existing IOVA that than need to be mapped
specifically inside the IOMMU.
Each of these co-processors also provides a single function and most
of them don't even have different endpoints. And even those that do (DCP) will
just become a single driver since all those endpoints are very much related.
While it's not impossible to do all that by extending and forcing this into the
mailbox framework at lest I think that it doesn't fit very well and would just
create unneccesarry impedance.
Best,
Sven
On Sat, Apr 02, 2022 at 03:51:46PM +0200, Sven Peter wrote:
> On Wed, Mar 23, 2022, at 12:19, Krzysztof Kozlowski wrote:
> > On 21/03/2022 17:50, Sven Peter wrote:
> >> Apple SoCs such as the M1 come with multiple embedded co-processors
> >> running proprietary firmware. Communication with those is established
> >> over a simple mailbox using the RTKit IPC protocol.
> >>
> >> Signed-off-by: Sven Peter <[email protected]>
> >> ---
> >> drivers/soc/apple/Kconfig | 13 +
> >> drivers/soc/apple/Makefile | 3 +
> >> drivers/soc/apple/rtkit-crashlog.c | 147 +++++
> >> drivers/soc/apple/rtkit-internal.h | 76 +++
> >> drivers/soc/apple/rtkit.c | 842 +++++++++++++++++++++++++++++
> >> include/linux/soc/apple/rtkit.h | 203 +++++++
> >> 6 files changed, 1284 insertions(+)
> >
> > Isn't this some implementation of a mailbox? If so, it should be in
> > drivers/mailbox. Please don't put all stuff in soc/apple, that's not how
> > Linux is organized. To drivers/soc usually we put drivers which do not
> > fit regular subsystems.
> >
>
> I put this into soc/apple because I don't think it fits within the mailbox
> framework very well.
> (It actually uses the mailbox framework for the actual communication
> with the hardware with a driver that's already upstream.)
>
> Essentially, the mailbox subsystem provides a common API to send and
> receive messages over indepedent hardware channels and devicetree bindings
> to describe the relationship between those channels and other drivers.
>
> One of the features that doesn't really fit is that we need to be able
> to start, shutdown and re-start these co-processors. The NVMe driver
remoteproc does that. Did you look at it? Most remoteproc drivers use
some combination of mailboxes and shared memory.
Rob
On 05/04/2022 00.02, Rob Herring wrote:
> On Sat, Apr 02, 2022 at 03:51:46PM +0200, Sven Peter wrote:
>> On Wed, Mar 23, 2022, at 12:19, Krzysztof Kozlowski wrote:
>>> On 21/03/2022 17:50, Sven Peter wrote:
>>>> Apple SoCs such as the M1 come with multiple embedded co-processors
>>>> running proprietary firmware. Communication with those is established
>>>> over a simple mailbox using the RTKit IPC protocol.
>>>>
>>>> Signed-off-by: Sven Peter <[email protected]>
>>>> ---
>>>> drivers/soc/apple/Kconfig | 13 +
>>>> drivers/soc/apple/Makefile | 3 +
>>>> drivers/soc/apple/rtkit-crashlog.c | 147 +++++
>>>> drivers/soc/apple/rtkit-internal.h | 76 +++
>>>> drivers/soc/apple/rtkit.c | 842 +++++++++++++++++++++++++++++
>>>> include/linux/soc/apple/rtkit.h | 203 +++++++
>>>> 6 files changed, 1284 insertions(+)
>>>
>>> Isn't this some implementation of a mailbox? If so, it should be in
>>> drivers/mailbox. Please don't put all stuff in soc/apple, that's not how
>>> Linux is organized. To drivers/soc usually we put drivers which do not
>>> fit regular subsystems.
>>>
>>
>> I put this into soc/apple because I don't think it fits within the mailbox
>> framework very well.
>> (It actually uses the mailbox framework for the actual communication
>> with the hardware with a driver that's already upstream.)
>>
>> Essentially, the mailbox subsystem provides a common API to send and
>> receive messages over indepedent hardware channels and devicetree bindings
>> to describe the relationship between those channels and other drivers.
>>
>> One of the features that doesn't really fit is that we need to be able
>> to start, shutdown and re-start these co-processors. The NVMe driver
>
> remoteproc does that. Did you look at it? Most remoteproc drivers use
> some combination of mailboxes and shared memory.
Remoteproc seems to be mostly about providing a standard interface for
loading firmware images and kickstarting somewhat generic remote
processors, as well as some high-level stuff for virtio. None of that is
useful to us as far as I can tell, because Linux doesn't load the
firmware for these things, it is pre-loaded by the bootloader and
therefore they might as well be fixed-function hardware as far as we're
concerned.
I can certainly see some similarities between the resourceproc API and
what we're doing, but I don't see what it would do for us other than
cause an impedance mismatch. Does it actually *do* something we can use?
Keep in mind these are Apple copros running Apple firmware that will
only ever talk to drivers we write for Apple machines, and we don't
control the firmware.
Impediance mismatches on first glance:
- The assumption that firmware is loaded by Linux seems to be hard-coded
into the subsystem
- Only one boot/shutdown path, while we need different power states and
boot modes depending on the specific instance
- The concept of the "resource table"; we have something similar for at
least one copro, but it's brokered via exchanged messages after it is
booted, and not something we can just look up in the firmware (our plan
was to just put the requested regions in the DT reg node and name them;
the firmware then *after boot* provides a list of mappings it wants from
that list and they can be mapped at that point). At least one other
copro does a subset of this an entirely different way altogether. Apple
aren't consistent and we can't do anything about that.
- Rproc trace buffers: Apple does syslogs but a different, incompatible way.
- ELF coredumps: even if this were useful for the blobs we get, the
blobs loaded by the bootloader themselves are Mach-O binaries, not ELF,
so that'd require some funny binary format conversion on one end or the
other to be able to line them up for postmortem debugging. And Apple's
crashdump format is a custom tag/value type thing.
I'm certainly willing to be convinced to use a kernel subsystem if it
actually does something useful for us, but last time we did that when it
wasn't entirely clear we should (mailbox, for the hardware underlying
these coprocessors) it just resulted in a bunch of headaches because
that subsystem is poorly designed and doesn't seem to have bought us
anything other than limitations. e.g. it is using suboptimal queueing
right now, and we had to switch to the atomic API to make SMC work,
which ends up even lower level, and isn't even properly documented and
different drivers interpret differently, so now it's just pure added
complexity and confusion for ~no gain over just reading/writing the
mailbox registers, which would've been *much* simpler (and more
performant than introducing additional queuing, since the hardware
*does* queuing now which we can't use because mailbox doesn't support
it). Remoteproc kind of seems like an even worse fit here...
Oh, and these device producer/consumer relations cannot be computed
statically as far as I can tell, so each one we introduce is another
special case in distro initramfs image building, since they need to
encode magic additional device-specific module dependency information
somehow since it isn't available in depmod, but only encoded in DTs
which are not necessarily known at initramfs build time.
--
Hector Martin ([email protected])
Public Key: https://mrcn.st/pub
On 02/04/2022 15:51, Sven Peter wrote:
> On Wed, Mar 23, 2022, at 12:19, Krzysztof Kozlowski wrote:
>> On 21/03/2022 17:50, Sven Peter wrote:
>>> Apple SoCs such as the M1 come with multiple embedded co-processors
>>> running proprietary firmware. Communication with those is established
>>> over a simple mailbox using the RTKit IPC protocol.
>>>
>>> Signed-off-by: Sven Peter <[email protected]>
>>> ---
>>> drivers/soc/apple/Kconfig | 13 +
>>> drivers/soc/apple/Makefile | 3 +
>>> drivers/soc/apple/rtkit-crashlog.c | 147 +++++
>>> drivers/soc/apple/rtkit-internal.h | 76 +++
>>> drivers/soc/apple/rtkit.c | 842 +++++++++++++++++++++++++++++
>>> include/linux/soc/apple/rtkit.h | 203 +++++++
>>> 6 files changed, 1284 insertions(+)
>>
>> Isn't this some implementation of a mailbox? If so, it should be in
>> drivers/mailbox. Please don't put all stuff in soc/apple, that's not how
>> Linux is organized. To drivers/soc usually we put drivers which do not
>> fit regular subsystems.
>>
>
> I put this into soc/apple because I don't think it fits within the mailbox
> framework very well.
> (It actually uses the mailbox framework for the actual communication
> with the hardware with a driver that's already upstream.)
>
> Essentially, the mailbox subsystem provides a common API to send and
> receive messages over indepedent hardware channels and devicetree bindings
> to describe the relationship between those channels and other drivers.
>
> One of the features that doesn't really fit is that we need to be able
> to start, shutdown and re-start these co-processors. The NVMe driver
> actually doesn't need to send/receive any messages except those required
> to setup the common syslog/crashlog/etc. interfaces.
> The mailbox framework would have to be extended to support these specific
> use cases.
>
> Another thing that doesn't fit is the memory management: These co-processors
> sometimes need shared memory buffers to e.g. send syslog messages.
> They always request these buffers with an IPC message but then there are
> different possibilities:
>
> - For some processor the DMA API can just be used and an IOVA must be
> sent back. For NVMe these buffers must additionally be allowed in this
> SART address filter.
> - At least one other processor (SMC) does not request such buffers but
> instead just sends a pointer into MMIO space and the buffer must be
> accessed using readl/writel. This MMIO memory region is used for
> both the common buffers (syslog etc.) and for the actual shared buffers
> used for communication, such that the resource would have to be shared
> across drivers.
> - And yet another coprocessor (for the display controller) requests some
> buffers with an already existing IOVA that than need to be mapped
> specifically inside the IOMMU.
>
> Each of these co-processors also provides a single function and most
> of them don't even have different endpoints. And even those that do (DCP) will
> just become a single driver since all those endpoints are very much related.
>
> While it's not impossible to do all that by extending and forcing this into the
> mailbox framework at lest I think that it doesn't fit very well and would just
> create unneccesarry impedance.
Thanks for explanation. I don't know the mailbox framework well enough
to advise you, so I don't mind keeping it in current location (drivers/soc).
Best regards,
Krzysztof