From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: MIME-Version: 1.0 In-Reply-To: References: <1416486442-25200-1-git-send-email-Vincent.Yang@tw.fujitsu.com> <1416486872-25301-1-git-send-email-Vincent.Yang@tw.fujitsu.com> <54749429.9080505@arm.com> <5474C3DD.5060706@arm.com> Date: Wed, 26 Nov 2014 13:44:44 +0800 Message-ID: Subject: Re: [PATCH 2/9] mailbox: arm_mhu: add driver for ARM MHU controller From: Andy Green Content-Type: multipart/alternative; boundary=047d7bf1617e54b00c0508bc86ee To: Jassi Brar Cc: Vincent Yang , Vincent Yang , "devicetree@vger.kernel.org" , "linux@arm.linux.org.uk" , Sudeep Holla , "linux-arm-kernel@lists.infradead.org" , "galak@codeaurora.org" , "ijc+devicetree@hellion.org.uk" , "robh+dt@kernel.org" , "arnd@arndb.de" , Mark Rutland , Tetsuya Nuriya , "patches@linaro.org" , "olof@lixom.net" , Pawel Moll List-ID: --047d7bf1617e54b00c0508bc86ee Content-Type: text/plain; charset=UTF-8 On 26 Nov 2014 13:37, "Jassi Brar" wrote: > > On 25 November 2014 at 23:31, Sudeep Holla wrote: > > > > > > On 25/11/14 16:51, Jassi Brar wrote: > >> > >> On 25 November 2014 at 20:07, Sudeep Holla wrote: > >>> > >>> > >>> > >>> On 20/11/14 12:34, Vincent Yang wrote: > >>>> > >>>> > >>>> Add driver for the ARM Message-Handling-Unit (MHU). > >>>> > >>>> Signed-off-by: Andy Green > >>>> Signed-off-by: Jassi Brar > >>>> Signed-off-by: Vincent Yang > >>>> Signed-off-by: Tetsuya Nuriya > >>>> --- > >>>> .../devicetree/bindings/mailbox/arm-mhu.txt | 33 ++++ > >>>> drivers/mailbox/Kconfig | 7 + > >>>> drivers/mailbox/Makefile | 2 + > >>>> drivers/mailbox/arm_mhu.c | 196 > >>>> +++++++++++++++++++++ > >>>> 4 files changed, 238 insertions(+) > >>>> create mode 100644 > >>>> Documentation/devicetree/bindings/mailbox/arm-mhu.txt > >>>> create mode 100644 drivers/mailbox/arm_mhu.c > >>>> > >>>> diff --git a/Documentation/devicetree/bindings/mailbox/arm-mhu.txt > >>>> b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt > >>>> new file mode 100644 > >>>> index 0000000..b1b9888 > >>>> --- /dev/null > >>>> +++ b/Documentation/devicetree/bindings/mailbox/arm-mhu.txt > >>>> @@ -0,0 +1,33 @@ > >>>> +ARM MHU Mailbox Driver > >>>> +====================== > >>>> + > >>>> +The ARM's Message-Handling-Unit (MHU) is a mailbox controller that has > >>>> +3 independent channels/links to communicate with remote processor(s). > >>> > >>> > >>> > >>> I had reviewed this before and I see not all the comments are addressed. > >>> I had mentioned that you can't add support to _SECURE_ channel in Linux > >>> as you need to assume Linux runs in non-secure privilege(and I gather > >>> that's the case even on this platform from other email in the thread) > >>> > >> Please revisit the old thread. After some discussion you had > >> graciously allowed me to keep the secure channel ;) > >> [ > >> ... Even though I don't like you have secure channel access in Linux, you > >> have valid reasons. In case you decide to support it .... > >> ] > > > > > > Agreed but based on the other email in the same thread it looks like you > > want to run the same kernel both in secure and no-secure mode on this > > platform, in which case you _have_to_assume_ it's *non-secure only* always > > unless you come up with some DT magic. > > > Yes, the S vs NS mode should ideally be defined in DT. The kernel > image should remain the same. If this is desirable, we can edit the related dtb path in memory accordingly at the bootloader, since that knows at runtime if he's secure or nonsecure. -Andy > >> It seems you still don't get my point that the driver should manage > >> all channels - S & NS. If Linux is running in NS mode on a platform, > >> the DT will specify only some NS channel to be used. The controller > >> driver shouldn't be crippled just because you think Linux will never > >> be run in Secure mode. > >> > > > > Ok how do you handle that, I don't see that in the DT binding. As it > > stands, you can unconditionally try to access the secure channel and > > cause aborts if the platform is running in non-secure mode. > > > No. Please look at the dtsi again .... > > mhu: mailbox@2b1f0000 { > #mbox-cells = <1>; > compatible = "arm,mbox-mhu"; > reg = <0 0x2b1f0000 0x1000>; > interrupts = <0 36 4>, /* LP Non-Sec */ > <0 35 4>, /* HP Non-Sec */ > <0 37 4>; /* Secure */ > }; > > mhu_client: scb@2e000000 { > compatible = "fujitsu,mb86s70-scb-1.0"; > reg = <0 0x2e000000 0x4000>; /* SHM for IPC */ > mboxes = <&mhu 1>; > }; > > See the DT for mbox client specifies that it uses channel-1 which is > High-Priority_Non-Secure channel. > > -jassi --047d7bf1617e54b00c0508bc86ee Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable


On 26 Nov 2014 13:37, "Jassi Brar" <jaswinder.singh@linaro.org> wrote:
>
> On 25 November 2014 at 23:31, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> >
> > On 25/11/14 16:51, Jassi Brar wrote:
> >>
> >> On 25 November 2014 at 20:07, Sudeep Holla <sudeep.holla@arm.com> wrote:
> >>>
> >>>
> >>>
> >>> On 20/11/14 12:34, Vincent Yang wrote:
> >>>>
> >>>>
> >>>> Add driver for the ARM Message-Handling-Unit (MHU). > >>>>
> >>>> Signed-off-by: Andy Green <andy.green@linaro.org>
> >>>> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> >>>> Signed-off-by: Vincent Yang <Vincent.Yang@tw.fujitsu.com>
> >>>> Signed-off-by: Tetsuya Nuriya <nuriya.tetsuya@jp.fujitsu.com>
> >>>> ---
> >>>>=C2=A0 =C2=A0 .../devicetree/bindings/mailbox/arm-mhu.= txt=C2=A0 =C2=A0 =C2=A0 =C2=A0 |=C2=A0 33 ++++
> >>>>=C2=A0 =C2=A0 drivers/mailbox/Kconfig=C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 |=C2=A0 =C2=A07 +
> >>>>=C2=A0 =C2=A0 drivers/mailbox/Makefile=C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0|=C2=A0 =C2=A02 +
> >>>>=C2=A0 =C2=A0 drivers/mailbox/arm_mhu.c=C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 | 196
> >>>> +++++++++++++++++++++
> >>>>=C2=A0 =C2=A0 4 files changed, 238 insertions(+)
> >>>>=C2=A0 =C2=A0 create mode 100644
> >>>> Documentation/devicetree/bindings/mailbox/arm-mhu.txt=
> >>>>=C2=A0 =C2=A0 create mode 100644 drivers/mailbox/arm_m= hu.c
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/mailbo= x/arm-mhu.txt
> >>>> b/Documentation/devicetree/bindings/mailbox/arm-mhu.t= xt
> >>>> new file mode 100644
> >>>> index 0000000..b1b9888
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/mailbox/arm-m= hu.txt
> >>>> @@ -0,0 +1,33 @@
> >>>> +ARM MHU Mailbox Driver
> >>>> +=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D
> >>>> +
> >>>> +The ARM's Message-Handling-Unit (MHU) is a mailb= ox controller that has
> >>>> +3 independent channels/links to communicate with rem= ote processor(s).
> >>>
> >>>
> >>>
> >>> I had reviewed this before and I see not all the comments= are addressed.
> >>> I had mentioned that you can't add support to _SECURE= _ channel in Linux
> >>> as you need to assume Linux runs in non-secure privilege(= and I gather
> >>> that's the case even on this platform from other emai= l in the thread)
> >>>
> >> Please revisit the old thread. After some discussion you had<= br> > >> graciously allowed me to keep the secure channel ;)
> >> [
> >> ... Even though I don't like you have secure channel acce= ss in Linux, you
> >> have valid reasons. In case you decide to support it ....
> >> ]
> >
> >
> > Agreed but based on the other email in the same thread it looks l= ike you
> > want to run the same kernel both in secure and no-secure mode on = this
> > platform, in which case you _have_to_assume_ it's *non-secure= only* always
> > unless you come up with some DT magic.
> >
> Yes, the S vs NS mode should ideally be defined in DT. The kernel
> image should remain the same.

If this is desirable, we can edit the related dtb path in me= mory accordingly at the bootloader, since that knows at runtime if he's= secure or nonsecure.

-Andy

> >>=C2=A0 =C2=A0It seems you still don't get m= y point that the driver should manage
> >> all channels - S & NS. If Linux is running in NS mode on = a platform,
> >> the DT will specify only some NS channel to be used. The cont= roller
> >> driver shouldn't be crippled just because you think Linux= will never
> >> be run in Secure mode.
> >>
> >
> > Ok how do you handle that, I don't see that in the DT binding= . As it
> > stands, you can unconditionally try to access the secure channel = and
> > cause aborts if the platform is running in non-secure mode.
> >
> No. Please look at the dtsi again ....
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 mhu: mailbox@2b1f0000 {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 #mbox-cells = =3D <1>;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 compatible =3D= "arm,mbox-mhu";
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 reg =3D <0 = 0x2b1f0000 0x1000>;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 interrupts =3D= <0 36 4>, /* LP Non-Sec */
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0<0 35 4>, /* HP Non-Sec */
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0<0 37 4>; /* Secure */
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 };
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 mhu_client: scb@2e000000 {
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 compatible =3D= "fujitsu,mb86s70-scb-1.0";
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 reg =3D <0 = 0x2e000000 0x4000>; /* SHM for IPC */
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 mboxes =3D <= ;&mhu 1>;
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 };
>
> See the DT for mbox client specifies that it uses channel-1 which is > High-Priority_Non-Secure channel.
>
> -jassi

--047d7bf1617e54b00c0508bc86ee--