| Message ID | 1493733353-25812-3-git-send-email-sudeep.holla@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mailbox: arm_mhu: add support for subchannels | expand |
On Mon 08 May 10:07 PDT 2017, Sudeep Holla wrote: > > > On 08/05/17 17:46, Jassi Brar wrote: > > On Mon, May 8, 2017 at 9:40 PM, Rob Herring <robh@kernel.org> wrote: > >> +Bjorn > >> > >> On Tue, May 02, 2017 at 02:55:49PM +0100, Sudeep Holla wrote: > >>> The ARM MHU has mechanism to assert interrupt signals to facilitate > >>> inter-processor message based communication. It drives the signal using > >>> a 32-bit register, with all 32-bits logically ORed together. It also > >>> enables software to set, clear and check the status of each of the bits > >>> of this register independently. Each bit of the register can be > >>> associated with a type of event that can contribute to raising the > >>> interrupt thereby allowing it to be used as independent subchannels. > >>> > >>> Since the first version of this binding can't support sub-channels, > >>> this patch extends the existing binding to support them. > >>> > >>> Cc: Alexey Klimov <alexey.klimov@arm.com> > >>> Cc: Jassi Brar <jaswinder.singh@linaro.org> > >>> Cc: Rob Herring <robh+dt@kernel.org> > >>> Cc: devicetree@vger.kernel.org > >>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > >>> --- > >>> .../devicetree/bindings/mailbox/arm-mhu.txt | 44 ++++++++++++++++++++-- > >>> 1 file changed, 41 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt > >>> index 4971f03f0b33..86a66f7918e2 100644 > >>> --- a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt > >>> +++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt > >>> @@ -10,21 +10,40 @@ STAT register and the remote clears it after having read the data. > >>> The last channel is specified to be a 'Secure' resource, hence can't be > >>> used by Linux running NS. > >>> > >>> +The MHU drives the interrupt signal using a 32-bit register, with all > >>> +32-bits logically ORed together. It 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 per 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. > >> > >> Sounds like a doorbell? (i.e. a single bit mailbox). Bjorn is doing > >> something similar for QCom h/w. I guess the difference here is you have > >> 32 sources and 1 output. It seems to me these should be described > >> similarly. > >> > > Yes, QCom controller triggers different interrupt for each bit of a > > 32bits register i.e, each signal is associated with 1bit information. > > Whereas MHU signals 32bits at a time to the target cpu. > > Agreed. I had a look at Qcom driver, not entirely clear if each bit as > interrupt as I don't see any interrupt support there. Each of the (used) bits in the Qualcomm HW are routed to a interrupt controller in the remote processors. As the APCS IPC is one way and each incoming "channel" is exposed as a separate physical interrupt they are directly consumed by the higher levels as needed - hence there's no traces of interrupts in the APCS IPC. > Also, it just adds > all the 32 channels which I am trying to avoid as at-most 4-5 will be > used while we end up creating 64 channels. > In the Qualcomm platform I'm looking at right now 15 of the 32 bits are used by the local CPU, so the utilization isn't awesome but I didn't feel the waste was worth addressing in my case. You should be able to provide a custom of_xlate that hides the fact that your mailbox-space is sparse - i.e. only register the mailboxes you have but expose them with ids as expected by the clients. Regards, Bjorn
On Tue, May 9, 2017 at 4:23 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > On 09/05/17 11:31, Jassi Brar wrote: >> On Tue, May 9, 2017 at 3:28 PM, Sudeep Holla <sudeep.holla@arm.com> >> wrote: >> >>>> >>>> If it is still not clear, please share your client driver. I >>>> will adapt that to work with existing MHU driver & bindings. >>>> >>> >>> Just take example of SCPI in the mainline. Assume there's another >>> protocol SCMI which uses few more bits in the same channel and the >>> remote firmware implements both but both are totally independent >>> and not related/linked. Also be keep in mind that SCPI is used by >>> other platforms and so will be the new protocol. We simply make >>> SCPI or SCMI bindings aligned to ARM MHU. That's ruled out. >>> >> Not sure what you mean by "that's ruled out". > > 1. The mailbox client bindings should be independent of this ARM MHU > mailbox bindings > 2. All we need in client is a mailbox to point at and not any meta data > That's what I meant by ruled-out as both client and MHU can be used > independent of each other and *should not* be linked. > I am shocked at this coming from you. You design SCMI based upon MHU assumption of single bit "doorbell" and then you say a client should be independent of the underlying controller? Do you intend SCMI to work only over MHU? What if some controller does not support the simple "doorbell" and expects detailed info? For example, apart from SCMI, the remote also supports platform specific functions like thermal, watchdog, wakeup etc. The SCMI's would just be a subset of the full command set. You/SCMI can not dictate what numerical value the platform assigns to SCMI commands... all that is required is the remote firmware should uniquely identify which command is it and implement what the SCMI protocol expects. Have a look at mbox_send_message(struct mbox_chan *chan, void *mssg) The 'mssg' is the pointer to _controller-specified_ structure. The client driver (SCMI) _must_ know what the underlying controller expects and pass that info. For example of 'mssg', look at "struct brcm_message" in include/linux/mailbox/brcm-message.h The client driver must use that platform specific knowledge to send a message i.e, you can not make a mailbox client driver 100% provider agnostic. You need to divide SCMI into two parts - one that manages the SCMI protocol and the other platform specific glue that talks to the mailbox controller over the mailbox api. >> You have already shared this "v2" MHU driver, now please also share >> your client driver. I'll make it work with original MHU driver and >> that should settle your confusion. > > It should first work with SCPI in the mainline. Then we will add another > similar protocol soon. So I think you have all you need in the mainline. > Today we have hack in the SCPI driver to pass bit 0 set in data. But > that's broken as we may want different slot on some other platform. > Basically SCPI is designed with the use of doorbell and it should not > have any details on how to write that into a particular register as > along as we just choose the right channel. > > On digging more about different mailbox controllers, I found > mailbox-sti.c has exactly similar logic as what I have done in this series. > > Also don't mix implementation with the binding. I need a simple answer > in this binding. How do I represent specific bits if each bit is > implemented as a doorbell ? That's all. First let's agree on that when > we use this mailbox independently and please *don't mix* with any > client here. It's simple, this controller has 2-3 sets of 32 doorbell > bits. And I am aiming to come up with the binding for that as your > initial bindings didn't consider that. > Please send in whatever changes you plan to do, and I'll modify it so we don't have to bloat the MHU driver and add bindings for a software feature. Until then ... Cheers!
On 09/05/17 14:29, Jassi Brar wrote: > On Tue, May 9, 2017 at 6:11 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> On 09/05/17 12:55, Jassi Brar wrote: > >>>>>>> >>>>>>> If it is still not clear, please share your client driver. I >>>>>>> will adapt that to work with existing MHU driver & bindings. >>>>>>> >>>>>> >>>>>> Just take example of SCPI in the mainline. Assume there's another >>>>>> protocol SCMI which uses few more bits in the same channel and the >>>>>> remote firmware implements both but both are totally independent >>>>>> and not related/linked. Also be keep in mind that SCPI is used by >>>>>> other platforms and so will be the new protocol. We simply make >>>>>> SCPI or SCMI bindings aligned to ARM MHU. That's ruled out. >>>>>> >>>>> Not sure what you mean by "that's ruled out". >>>> >>>> 1. The mailbox client bindings should be independent of this ARM MHU >>>> mailbox bindings >>>> 2. All we need in client is a mailbox to point at and not any meta data >>>> That's what I meant by ruled-out as both client and MHU can be used >>>> independent of each other and *should not* be linked. >>>> >>> I am shocked at this coming from you. >>> >>> You design SCMI based upon MHU assumption of single bit "doorbell" and >>> then you say a client should be independent of the underlying >>> controller? Do you intend SCMI to work only over MHU? >>> >> >> No, I never said that. What I said is SCMI protocol will be on doorbell >> based. >> > What if a controller does not support your definition of "doorbell"? > Like PL320 from ARM and many others. > OK, why are we discussing that here ? >>> What if some controller does not support the simple "doorbell" and >>> expects detailed info? For example, apart from SCMI, the remote also >>> supports platform specific functions like thermal, watchdog, wakeup >>> etc. The SCMI's would just be a subset of the full command set. >>> You/SCMI can not dictate what numerical value the platform assigns to >>> SCMI commands... >> >> What ? That's the whole point of specification. The command set is >> *fixed* and can be implemented on any platform and have generic driver >> for that. >> > The code/value for commands in SHM data packet is SCMI specific. But > what a platform assigns to THIS_IS_SCMI_DOORBELL is going to be > platform specific i.e, not always BIT(x) > Platform which uses this as single bit doorbell has to just choose the tuple(bit and the register set) as shown in the example binding >>>> On digging more about different mailbox controllers, I found >>>> mailbox-sti.c has exactly similar logic as what I have done in this series. >>>> >> >> Did you look at this driver ? >> > Dude, I merged this driver upstream! I don't remember exactly about > STI controller, but it definitely is different from MHU. > Yes I can know and can see you have upstreamed the driver. I have spoken to the ARM MHU hardware IP designers and I know what it's designed for. And that's why I gave you example to look at STI driver to help you understand what I am trying to say faster. >>>> Also don't mix implementation with the binding. I need a simple answer >>>> in this binding. How do I represent specific bits if each bit is >>>> implemented as a doorbell ? That's all. First let's agree on that when >>>> we use this mailbox independently and please *don't mix* with any >>>> client here. It's simple, this controller has 2-3 sets of 32 doorbell >>>> bits. And I am aiming to come up with the binding for that as your >>>> initial bindings didn't consider that. >>>> >>> Please send in whatever changes you plan to do, and I'll modify it so >>> we don't have to bloat the MHU driver and add bindings for a software >>> feature. Until then ... Cheers! >>> >> >> Changes to what ? arm_mhu.c ? This series is complete and implements >> doorbell completely. >> > Send in the user/client driver that you think can not work with > existing driver/bindings. > Again for the 3rd time see arm_scpi.c ARM is now generalizing it with multiple vendors under the new name ARM SCMI. And Juno is implementing using few doorbell bits on the same channel as SCPI. -- Regards, Sudeep
diff --git a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt index 4971f03f0b33..86a66f7918e2 100644 --- a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt +++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt @@ -10,21 +10,40 @@ STAT register and the remote clears it after having read the data. The last channel is specified to be a 'Secure' resource, hence can't be used by Linux running NS. +The MHU drives the interrupt signal using a 32-bit register, with all +32-bits logically ORed together. It 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 per 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. + Mailbox Device Node: ==================== Required properties: -------------------- -- compatible: Shall be "arm,mhu" & "arm,primecell" +- compatible: Shall be "arm,primecell" and one of the below: + "arm,mhu" - if the controller doesn't support + subchannels + "arm,mhu-v2" - if the controller supports subchannels - reg: Contains the mailbox register address range (base address and length) -- #mbox-cells Shall be 1 - the index of the channel needed. +- #mbox-cells Shall be 1 - the index of the channel needed when + compatible is "arm,mhu" + Shall be 2 - the index of the channel needed, and + the index of the subchannel with the channel when + compatible is "arm,mhu-v2" - interrupts: Contains the interrupt information corresponding to - each of the 3 links of MHU. + each of the 3 physical channels of MHU namely low + priority non-secure, high priority non-secure and + secure channels. Example: -------- +1. Controller which doesn't support subchannels + mhu: mailbox@2b1f0000 { #mbox-cells = <1>; compatible = "arm,mhu", "arm,primecell"; @@ -41,3 +60,22 @@ used by Linux running NS. reg = <0 0x2e000000 0x4000>; mboxes = <&mhu 1>; /* HP-NonSecure */ }; + +2. Controller which supports subchannels + + mhu: mailbox@2b1f0000 { + #mbox-cells = <2>; + compatible = "arm,mhu-v2", "arm,primecell"; + reg = <0 0x2b1f0000 0x1000>; + interrupts = <0 36 4>, /* LP-NonSecure */ + <0 35 4>, /* HP-NonSecure */ + <0 37 4>; /* Secure */ + clocks = <&clock 0 2 1>; + clock-names = "apb_pclk"; + }; + + mhu_client: scb@2e000000 { + compatible = "arm,scpi"; + reg = <0 0x2e000000 0x200>; + mboxes = <&mhu 1 4>; /* HP-NonSecure 5th sub-channel */ + };
The ARM MHU has mechanism to assert interrupt signals to facilitate inter-processor message based communication. It drives the signal using a 32-bit register, with all 32-bits logically ORed together. It also enables software to set, clear and check the status of each of the bits of this register independently. Each bit of the register can be associated with a type of event that can contribute to raising the interrupt thereby allowing it to be used as independent subchannels. Since the first version of this binding can't support sub-channels, this patch extends the existing binding to support them. Cc: Alexey Klimov <alexey.klimov@arm.com> Cc: Jassi Brar <jaswinder.singh@linaro.org> Cc: Rob Herring <robh+dt@kernel.org> Cc: devicetree@vger.kernel.org Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> --- .../devicetree/bindings/mailbox/arm-mhu.txt | 44 ++++++++++++++++++++-- 1 file changed, 41 insertions(+), 3 deletions(-) -- 2.7.4