Message-ID: <CABb+yY3KKpDHTsTBescW_rXmqmLzJh-Ogaotk2n=nYRkfHy2cg@mail.gmail.com>
Date:   Fri, 29 May 2020 00:20:46 -0500
From:   Jassi Brar <jassisinghbrar@...il.com>
To:     Rob Herring <robh@...nel.org>
Cc:     Viresh Kumar <viresh.kumar@...aro.org>,
        Arnd Bergmann <arnd@...db.de>,
        Frank Rowand <frowand.list@...il.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Sudeep Holla <sudeep.holla@....com>,
        Devicetree List <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [RFC] dt-bindings: mailbox: add doorbell support to ARM MHU

On Thu, May 28, 2020 at 2:20 PM Rob Herring <robh@...nel.org> wrote:
>
> On Fri, May 15, 2020 at 10:47:38AM +0530, Viresh Kumar wrote:
> > From: Sudeep Holla <sudeep.holla@....com>
> >
> > Hi Rob, Arnd and Jassi,
> >
> > This stuff has been doing rounds on the mailing list since several years
> > now with no agreed conclusion by all the parties. And here is another
> > attempt to get some feedback from everyone involved to close this once
> > and for ever. Your comments will very much be appreciated.
> >
> > The ARM MHU is defined here in the TRM [1] for your reference, which
> > states following:
> >
> >       "The MHU drives the signal using a 32-bit register, with all 32
> >       bits logically ORed together. The MHU provides a set of
> >       registers to enable software to set, clear, and check the status
> >       of each of the bits of this register independently.  The use of
> >       32 bits for each interrupt line enables software to provide more
> >       information about the source of the interrupt. For example, each
> >       bit of the register can be associated with a type of event that
> >       can contribute to raising the interrupt."
> >
> > On few other platforms, like qcom, similar doorbell mechanism is present
> > with separate interrupt for each of the bits (that's how I understood
> > it), while in case of ARM MHU, there is a single interrupt line for all
> > the 32 bits. Also in case of ARM MHU, these registers and interrupts
> > have 3 copies for different priority levels, i.e. low priority
> > non-secure, high priority non-secure and secure channels.
> >
> > For ARM MHU, both the dt bindings and the Linux driver support 3
> > channels for the different priorities right now and support sending a 32
> > bit data on every transfer in a locked fashion, i.e. only one transfer
> > can be done at once and the other have to wait for it to finish first.
> >
> > Here are the point of view of the parties involved on this subject:
> >
> > Jassi's viewpoint:
> >
> > - Virtualization of channels should be discouraged in software based on
> >   specific usecases of the application. This may invite other mailbox
> >   driver authors to ask for doing virtualization in their drivers.
> >
> > - In mailbox's terminology, every channel is equivalent to a signal,
> >   since there is only one signal generated here by the MHU, there should
> >   be only one channel per priority level.
> >
> > - The clients should send data (of just setting 1 bit or many in the 32
> >   bit word) using the existing mechanism as the delays due to
> >   serialization shouldn't be significant anyway.
> >
> > - The driver supports 90% of the users with the current implementation
> >   and it shouldn't be extended to support doorbell and implement two
> >   different modes by changing value of #mbox-cells field in bindings.
> >
> > Sudeep (ARM) and myself as well to some extent:
> >
> > - The hardware gives us the capability to write the register in
> >   parallel, i.e. we can write 0x800 and 0x400 together without any
> >   software locks, and so these 32 bits should be considered as separate
> >   channel even if only one interrupt is issued by the hardware finally.
> >   This shouldn't be called as virtualization of the channels, as the
> >   hardware supports this (as clearly mentioned in the TRM) and it takes
> >   care of handling the signal properly.
> >
> > - With serialization, if we use only one channel as today at every
> >   priority, if there are 5 requests to send signal to the receiver and
> >   the dvfs request is the last one in queue (which may be called from
> >   scheduler's hot path with fast switching), it unnecessarily needs to
> >   wait for the first four transfers to finish due to the software
> >   locking imposed by the mailbox framework. This adds additional delay,
> >   maybe of few ms only, which isn't required by the hardware but just by
> >   the software and few ms can be important in scheduler's hotpath.
> >
> > - With the current approach it isn't possible to assign different bits
> >   (or doorbell numbers) to clients from DT and the only way of doing
> >   that without adding new bindings is by extending #mbox-cells to accept
> >   a value of 2 as done in this patch.
> >
> > Jassi and Sudeep, I hope I was able to represent both the view points
> > properly here. Please correct me if I have made a mistake here.
> >
> > This is it. It would be nice to get the views of everyone now on this
> > and how should this be handled.
>
> I am perfectly fine with adding another cell which seems appropriate
> here. You can have 5 cells for all I care if that makes sense for
> the h/w. That has nothing to do with the Linux design. Whether Linux
> requires serializing mailbox accesses is a separate issue. On that side,
> it seems silly to not allow driving the h/w in the most efficient way
> possible.
>
The fact that all these bits are backed by one physical signal makes
the firmware implement its own mux-demux'ing scheme. So the choice was
between demux and single signal at a time. Had there been one signal
per bit, the implementation would have definitely been 'efficient'.

And the first platform the driver was developed on, required message
passing over the registers. So now my approach is to make do with what
we have...unless it shows in numbers.

Anyways, if it comes to that, I'd rather a separate "doorbell' driver
than a driver working in two s/w modes.

Thanks.

Powered by blists - more mailing lists