Re: [PATCH v3 2/4] soc: qcom: Introduce APCS IPC driver

[Jassi Brar]

On Thu, May 4, 2017 at 9:40 PM, Bjorn Andersson
<bjorn.andersson@xxxxxxxxxx> wrote:
> On Thu 04 May 00:54 PDT 2017, Jassi Brar wrote:
>
>> On Thu, May 4, 2017 at 11:15 AM, Bjorn Andersson
>> <bjorn.andersson@xxxxxxxxxx> wrote:
>> > On Wed 03 May 02:55 PDT 2017, Jassi Brar wrote:
>> >
>> >> Loic, thanks for adding me.
>> >>
>> >> On Wed, May 3, 2017 at 2:58 PM, Loic PALLARDY <loic.pallardy@xxxxxx> wrote:
>> >> >
>> >> >
>> >> >> -----Original Message-----
>> >> >> From: linux-remoteproc-owner@xxxxxxxxxxxxxxx [mailto:linux-remoteproc-
>> >> >> owner@xxxxxxxxxxxxxxx] On Behalf Of Bjorn Andersson
>> >> >> Sent: Wednesday, May 03, 2017 7:29 AM
>> >> >> To: Andy Gross <andy.gross@xxxxxxxxxx>; Rob Herring
>> >> >> <robh+dt@xxxxxxxxxx>; Mark Rutland <mark.rutland@xxxxxxx>; Ohad Ben-
>> >> >> Cohen <ohad@xxxxxxxxxx>
>> >> >> Cc: linux-arm-msm@xxxxxxxxxxxxxxx; linux-soc@xxxxxxxxxxxxxxx;
>> >> >> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
>> >> >> remoteproc@xxxxxxxxxxxxxxx
>> >> >> Subject: [PATCH v3 2/4] soc: qcom: Introduce APCS IPC driver
>> >> >>
>> >> >> This implements a driver that exposes the IPC bits found in the APCS Global
>> >> >> block in various Qualcomm platforms. The bits are used to signal inter-
>> >> >> processor communication signals from the application CPU to other masters.
>> >> >>
>> >> >> The driver implements the "doorbell" binding and could be used as basis for a
>> >> >> new Linux framework, if found useful outside Qualcomm.
>> >> >>
>> >> > Hi Bjorn,
>> >> >
>> >> > Even if Qualcom APCS IPC is limited, why don't you rely on existing mailbox framework.
>> >> > It is there to gather all IPC management under the same interface.
>> >> > No need to create a new one from my pov.
>> >> > If you don't provide message data, mailbox framework behaves as doorbell.
>> >> >
>> >> QCOM RPM reinvented the wheel for what mailbox framework already did,
>> >> despite my pointing it out =>
>> >> http://lkml.iu.edu/hypermail//linux/kernel/1406.2/03918.html
>> >>
>> >
>> > The RPM interface works by writing various information in shared DRAM
>> > and then invoking an interrupt on the remote processor.
>> >
>> This is what _most_ platforms do, and they use mailbox framework. If
>> mailbox framework doesn't suit RPM, then it doesn't suit most
>> platforms that use it.
>>
>> > My argumentation against using the mailbox framework for the RPM case
>> > was that the message model is a software construct and doesn't fit the
>> > mailbox framework.
>> >
>> Mailbox framework works beneath protocol layer (software construct).
>> As I said years ago, the h/w bits of the controller should be under
>> mailbox api, while the message structures and signals are protocol
>> specific and be in QCOM specific location.
>>
>
> Okay, now I finally get what you're saying. The RPM driver handles the
> protocol but the actual triggering of the remote interrupt should be
> done via the mailbox framework.
>
> That makes sense, until now I haven't seen the need for having a driver
> exposing the APCS IPC register (used by among others the RPM driver),
> but I will rewrite this patch as a mailbox controller and turn the RPM
> driver into a mailbox client.
>
Cool, thanks.

>>
>> > But which one of these would be appropriate for a "mailbox channel" that
>> > doesn't have any actual messages?
>> >
>> >   mbox_send_message(chan, NULL)
>> > or
>> >   const int one = 1;
>> >   mbox_send_message(chan, (void*)&one);
>> >
>> It depends upon your h/w.
>> If each physical channel is hardwired to work on a predefined single
>> bit, then the driver could do without that bit being explicitly
>> passed.
>> If a physical channel can be mapped onto any bit(s), then you do need
>> to pass that info via mbox_send_message().
>>
>
> It's a 32-bit register, writing a bit invokes the associated interrupt
> on some remote processor. I.e. bits 0, 1 and 2 represents different
> interrupts on the resource-power-management CPU while bit 12, 13, 14 and
> 15 will invoke interrupts on a modem CPU.
>
No problem. For such arrangement, you could have a channel per
interrupt/bit. Have the DT specify which bit goes as what interrupt to
which cpu. The controller driver would then associate each channel to
its 'bit' and a client would always acquire the right channel
(specified by DT) and need not know/pass which bit should be set for
its messages i.e, mbox_send_message(chan, NULL)

> I'll rework the proposed driver and we can look at the actual
> implementation instead.
>
>>
>> >> The driver bypassed mailbox framework and was pushed via another tree.
>> >> Same is being attempted now, only now it is more expensive to switch
>> >> to generic mailbox framework having spent so much time on QCOM
>> >> specific implementation of controller and protocol drivers inside
>> >> drivers/soc/qcom/
>> >
>> > I'm not sure I follow this, there's no extensive rework going on here -
>> > all I'm trying to do is abstract the "writel(BIT(x), ipc_reg)" line of
>> > the RPM driver, because it's being used in other clients as well.
>> >
>> No, I meant ideally RPM should have used mailbox api for the
>> controller programming, but moving to that now will be expensive
>> because you already developed huge code base around QCOM specific
>> mailbox implementation.
>>
>
> The part discussed above was implemented using syscon, so it's a few
> lines of code to grab hold of a handle and instead of
> mbox_send_message() there is a single call to regmap_write(). All the
> other parts of the implementation is protocol-specific, and as you say
> the next layer up. So the transition is quite straight forward!
>
Great!

Thanks.