| Message ID | 20201029195913.5927-2-james.quinlan@broadcom.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | mailbox: Add Broadcom STB mailbox driver for SCMI | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | success |
On Thu, Oct 29, 2020 at 03:59:06PM -0400, Jim Quinlan wrote: > Bindings are added. Only one interrupt is needed because > we do not yet employ the SCMI p2a channel. I still don't understand what this is. To repeat from v1: I thought SCMI was a mailbox consumer, not provider? > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> > --- > .../bindings/mailbox/brcm,brcmstb-mbox.yaml | 39 +++++++++++++++++++ > 1 file changed, 39 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml > > diff --git a/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml b/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml > new file mode 100644 > index 000000000000..797c0cc609a3 > --- /dev/null > +++ b/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml > @@ -0,0 +1,39 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > +$id: http://devicetree.org/schemas/mailbox/brcm,brcmstb-mbox.yaml# > + > +title: Broadcom STB mailbox driver bindings > + > +maintainers: > + - Jim Quinlan <james.quinlan@broadcom.com> > + > +properties: > + compatible: > + enum: > + - brcm,brcmstb-mbox > + > + interrupts: > + items: > + - description: a2p return interrupt, indicates SCMI msg completion. > + > + "#mbox-cells": > + const: 1 > + > +required: > + - compatible > + - interrupts > + - "#mbox-cells" > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + mailbox { > + compatible = "brcm,brcmstb-mailbox"; > + #mbox-cells = <1>; > + interrupts = <GIC_SPI 0xc6 IRQ_TYPE_LEVEL_HIGH>; > + }; > +... > -- > 2.17.1 >
On Wed, Nov 4, 2020 at 4:50 PM Rob Herring <robh@kernel.org> wrote: > > On Thu, Oct 29, 2020 at 03:59:06PM -0400, Jim Quinlan wrote: > > Bindings are added. Only one interrupt is needed because > > we do not yet employ the SCMI p2a channel. > > I still don't understand what this is. To repeat from v1: I thought SCMI > was a mailbox consumer, not provider? Hi Rob, I'm not sure where I am implying that SCMI is a mailbox provider? Should I not mention "SCMI" in the subject line? This is just a mailbox driver, "consumed" by SCMI. Our SCMI DT node looks like this: brcm_scmi_mailbox: brcm_scmi_mailbox@0 { #mbox-cells = <1>; compatible = "brcm,brcmstb-mbox"; }; brcm_scmi@0 { compatible = "arm,scmi"; mboxes = <&brcm_scmi_mailbox 0>;; mbox-names = "tx"; shmem = <&NWMBOX>; /* ... */ }; Please advise, Jim Quinlan Broadcom STB > > > > > Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> > > --- > > .../bindings/mailbox/brcm,brcmstb-mbox.yaml | 39 +++++++++++++++++++ > > 1 file changed, 39 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml > > > > diff --git a/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml b/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml > > new file mode 100644 > > index 000000000000..797c0cc609a3 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml > > @@ -0,0 +1,39 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#" > > +$id: http://devicetree.org/schemas/mailbox/brcm,brcmstb-mbox.yaml# > > + > > +title: Broadcom STB mailbox driver bindings > > + > > +maintainers: > > + - Jim Quinlan <james.quinlan@broadcom.com> > > + > > +properties: > > + compatible: > > + enum: > > + - brcm,brcmstb-mbox > > + > > + interrupts: > > + items: > > + - description: a2p return interrupt, indicates SCMI msg completion. > > + > > + "#mbox-cells": > > + const: 1 > > + > > +required: > > + - compatible > > + - interrupts > > + - "#mbox-cells" > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + mailbox { > > + compatible = "brcm,brcmstb-mailbox"; > > + #mbox-cells = <1>; > > + interrupts = <GIC_SPI 0xc6 IRQ_TYPE_LEVEL_HIGH>; > > + }; > > +... > > -- > > 2.17.1 > > > >
On Wed, Nov 4, 2020 at 4:04 PM Jim Quinlan <james.quinlan@broadcom.com> wrote: > > On Wed, Nov 4, 2020 at 4:50 PM Rob Herring <robh@kernel.org> wrote: > > > > On Thu, Oct 29, 2020 at 03:59:06PM -0400, Jim Quinlan wrote: > > > Bindings are added. Only one interrupt is needed because > > > we do not yet employ the SCMI p2a channel. > > > > I still don't understand what this is. To repeat from v1: I thought SCMI > > was a mailbox consumer, not provider? > > Hi Rob, > > I'm not sure where I am implying that SCMI is a mailbox provider? > Should I not mention "SCMI" in the subject line? > > This is just a mailbox driver, "consumed" by SCMI. Our SCMI DT node > looks like this: > > brcm_scmi_mailbox: brcm_scmi_mailbox@0 { > #mbox-cells = <1>; > compatible = "brcm,brcmstb-mbox"; > }; > > brcm_scmi@0 { > compatible = "arm,scmi"; > mboxes = <&brcm_scmi_mailbox 0>;; > mbox-names = "tx"; > shmem = <&NWMBOX>; > /* ... */ > }; Okay, that makes more sense. Though it seems like this is just adding a pointless level of indirection to turn an interrupt into a mailbox. There's nothing more to 'the mailbox' is there? So why not either allow SCMI to have an interrupt directly or have a generic irq mailbox driver? Rob
On Thu, Nov 5, 2020 at 10:13 AM Rob Herring <robh@kernel.org> wrote: > > On Wed, Nov 4, 2020 at 4:04 PM Jim Quinlan <james.quinlan@broadcom.com> wrote: > > > > On Wed, Nov 4, 2020 at 4:50 PM Rob Herring <robh@kernel.org> wrote: > > > > > > On Thu, Oct 29, 2020 at 03:59:06PM -0400, Jim Quinlan wrote: > > > > Bindings are added. Only one interrupt is needed because > > > > we do not yet employ the SCMI p2a channel. > > > > > > I still don't understand what this is. To repeat from v1: I thought SCMI > > > was a mailbox consumer, not provider? > > > > Hi Rob, > > > > I'm not sure where I am implying that SCMI is a mailbox provider? > > Should I not mention "SCMI" in the subject line? > > > > This is just a mailbox driver, "consumed" by SCMI. Our SCMI DT node > > looks like this: > > > > brcm_scmi_mailbox: brcm_scmi_mailbox@0 { > > #mbox-cells = <1>; > > compatible = "brcm,brcmstb-mbox"; > > }; > > > > brcm_scmi@0 { > > compatible = "arm,scmi"; > > mboxes = <&brcm_scmi_mailbox 0>;; > > mbox-names = "tx"; > > shmem = <&NWMBOX>; > > /* ... */ > > }; > > Okay, that makes more sense. Though it seems like this is just adding > a pointless level of indirection to turn an interrupt into a mailbox. > There's nothing more to 'the mailbox' is there? Correct. Although you can see that it uses both interrupts and SMC calls to get the job done. > So why not either > allow SCMI to have an interrupt directly Not sure here -- perhaps the SCMI folks have an answer? > or have a generic irq mailbox > driver? The SCMI implementation doesn't offer a generic irq mailbox driver AFAICT. The SCMI folks recently provided an "smc transport" driver in "drivers/firmware/arm_scmi/smc.c" -- it is close to what we need but is missing interrupts. Regards, Jim Quinlan Broadcom STB > > Rob
On Thu, Nov 05, 2020 at 10:28:25AM -0500, Jim Quinlan wrote: > On Thu, Nov 5, 2020 at 10:13 AM Rob Herring <robh@kernel.org> wrote: > > > > On Wed, Nov 4, 2020 at 4:04 PM Jim Quinlan <james.quinlan@broadcom.com> wrote: > > > > > > On Wed, Nov 4, 2020 at 4:50 PM Rob Herring <robh@kernel.org> wrote: > > > > > > > > On Thu, Oct 29, 2020 at 03:59:06PM -0400, Jim Quinlan wrote: > > > > > Bindings are added. Only one interrupt is needed because > > > > > we do not yet employ the SCMI p2a channel. > > > > > > > > I still don't understand what this is. To repeat from v1: I thought SCMI > > > > was a mailbox consumer, not provider? > > > > > > Hi Rob, > > > > > > I'm not sure where I am implying that SCMI is a mailbox provider? > > > Should I not mention "SCMI" in the subject line? > > > > > > This is just a mailbox driver, "consumed" by SCMI. Our SCMI DT node > > > looks like this: > > > > > > brcm_scmi_mailbox: brcm_scmi_mailbox@0 { > > > #mbox-cells = <1>; > > > compatible = "brcm,brcmstb-mbox"; > > > }; > > > > > > brcm_scmi@0 { > > > compatible = "arm,scmi"; > > > mboxes = <&brcm_scmi_mailbox 0>;; > > > mbox-names = "tx"; > > > shmem = <&NWMBOX>; > > > /* ... */ > > > }; > > > > Okay, that makes more sense. Though it seems like this is just adding > > a pointless level of indirection to turn an interrupt into a mailbox. > > There's nothing more to 'the mailbox' is there? > > Correct. Although you can see that it uses both interrupts and SMC > calls to get the job done. > I was against having 2 separate solutions and would have raised my concern again. As I mentioned earlier, either extend what we have or move the existing SMC solution into this mailbox driver. Having 2 different solution for this just because you have extra interrupt to deal with is definite NACK from me as I had previously mentioned. > > So why not either > > allow SCMI to have an interrupt directly > Not sure here -- perhaps the SCMI folks have an answer? > I did ask why can't you extend the existing SCMI/SMC binding to add this as optional feature ? > > or have a generic irq mailbox driver? Fine with this too. > The SCMI implementation doesn't offer a generic irq mailbox driver > AFAICT. The SCMI folks recently provided an "smc transport" driver > in "drivers/firmware/arm_scmi/smc.c" -- it is close to what we need > but is missing interrupts. IIRC, you were using SGIs and it can't be represented and use today as is ? Am I missing something or anything has changed ? -- Regards, Sudeep
On Thu, Nov 5, 2020 at 1:27 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Thu, Nov 05, 2020 at 10:28:25AM -0500, Jim Quinlan wrote: > > On Thu, Nov 5, 2020 at 10:13 AM Rob Herring <robh@kernel.org> wrote: > > > > > > On Wed, Nov 4, 2020 at 4:04 PM Jim Quinlan <james.quinlan@broadcom.com> wrote: > > > > > > > > On Wed, Nov 4, 2020 at 4:50 PM Rob Herring <robh@kernel.org> wrote: > > > > > > > > > > On Thu, Oct 29, 2020 at 03:59:06PM -0400, Jim Quinlan wrote: > > > > > > Bindings are added. Only one interrupt is needed because > > > > > > we do not yet employ the SCMI p2a channel. > > > > > > > > > > I still don't understand what this is. To repeat from v1: I thought SCMI > > > > > was a mailbox consumer, not provider? > > > > > > > > Hi Rob, > > > > > > > > I'm not sure where I am implying that SCMI is a mailbox provider? > > > > Should I not mention "SCMI" in the subject line? > > > > > > > > This is just a mailbox driver, "consumed" by SCMI. Our SCMI DT node > > > > looks like this: > > > > > > > > brcm_scmi_mailbox: brcm_scmi_mailbox@0 { > > > > #mbox-cells = <1>; > > > > compatible = "brcm,brcmstb-mbox"; > > > > }; > > > > > > > > brcm_scmi@0 { > > > > compatible = "arm,scmi"; > > > > mboxes = <&brcm_scmi_mailbox 0>;; > > > > mbox-names = "tx"; > > > > shmem = <&NWMBOX>; > > > > /* ... */ > > > > }; > > > > > > Okay, that makes more sense. Though it seems like this is just adding > > > a pointless level of indirection to turn an interrupt into a mailbox. > > > There's nothing more to 'the mailbox' is there? > > > > Correct. Although you can see that it uses both interrupts and SMC > > calls to get the job done. > > > > I was against having 2 separate solutions and would have raised my concern > again. As I mentioned earlier, either extend what we have or move the > existing SMC solution into this mailbox driver. Having 2 different solution > for this just because you have extra interrupt to deal with is definite > NACK from me as I had previously mentioned. > > > > So why not either > > > allow SCMI to have an interrupt directly > > Not sure here -- perhaps the SCMI folks have an answer? > > > > I did ask why can't you extend the existing SCMI/SMC binding to add this > as optional feature ? Hi Sudeep, Looking at the email you said, "In that case any reason why you can't reuse the existing smc transport for SCMI." , and I replied with the reason. I did not interpret your statement above as what you are clearly saying now: "either extend what we have or move the existing SMC solution into this mailbox driver. " Fair enough, I will look into this. Regards, Jim > > > > or have a generic irq mailbox driver? > > Fine with this too. > > > The SCMI implementation doesn't offer a generic irq mailbox driver > > AFAICT. The SCMI folks recently provided an "smc transport" driver > > in "drivers/firmware/arm_scmi/smc.c" -- it is close to what we need > > but is missing interrupts. > > IIRC, you were using SGIs and it can't be represented and use today as > is ? Am I missing something or anything has changed ? > > -- > Regards, > Sudeep
On Thu, Nov 05, 2020 at 01:57:07PM -0500, Jim Quinlan wrote: > On Thu, Nov 5, 2020 at 1:27 PM Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Thu, Nov 05, 2020 at 10:28:25AM -0500, Jim Quinlan wrote: > > > On Thu, Nov 5, 2020 at 10:13 AM Rob Herring <robh@kernel.org> wrote: > > > > > > > > On Wed, Nov 4, 2020 at 4:04 PM Jim Quinlan <james.quinlan@broadcom.com> wrote: > > > > > > > > > > On Wed, Nov 4, 2020 at 4:50 PM Rob Herring <robh@kernel.org> wrote: > > > > > > > > > > > > On Thu, Oct 29, 2020 at 03:59:06PM -0400, Jim Quinlan wrote: > > > > > > > Bindings are added. Only one interrupt is needed because > > > > > > > we do not yet employ the SCMI p2a channel. > > > > > > > > > > > > I still don't understand what this is. To repeat from v1: I thought SCMI > > > > > > was a mailbox consumer, not provider? > > > > > > > > > > Hi Rob, > > > > > > > > > > I'm not sure where I am implying that SCMI is a mailbox provider? > > > > > Should I not mention "SCMI" in the subject line? > > > > > > > > > > This is just a mailbox driver, "consumed" by SCMI. Our SCMI DT node > > > > > looks like this: > > > > > > > > > > brcm_scmi_mailbox: brcm_scmi_mailbox@0 { > > > > > #mbox-cells = <1>; > > > > > compatible = "brcm,brcmstb-mbox"; > > > > > }; > > > > > > > > > > brcm_scmi@0 { > > > > > compatible = "arm,scmi"; > > > > > mboxes = <&brcm_scmi_mailbox 0>;; > > > > > mbox-names = "tx"; > > > > > shmem = <&NWMBOX>; > > > > > /* ... */ > > > > > }; > > > > > > > > Okay, that makes more sense. Though it seems like this is just adding > > > > a pointless level of indirection to turn an interrupt into a mailbox. > > > > There's nothing more to 'the mailbox' is there? > > > > > > Correct. Although you can see that it uses both interrupts and SMC > > > calls to get the job done. > > > > > > > I was against having 2 separate solutions and would have raised my concern > > again. As I mentioned earlier, either extend what we have or move the > > existing SMC solution into this mailbox driver. Having 2 different solution > > for this just because you have extra interrupt to deal with is definite > > NACK from me as I had previously mentioned. > > > > > > So why not either > > > > allow SCMI to have an interrupt directly > > > Not sure here -- perhaps the SCMI folks have an answer? > > > > > > > I did ask why can't you extend the existing SCMI/SMC binding to add this > > as optional feature ? > Hi Sudeep, > > Looking at the email you said, "In that case any reason why you can't > reuse the existing smc transport for SCMI." , and I replied with the > reason. I did not interpret your statement above as what you are > clearly saying now: "either extend what we have or move the existing > SMC solution into this mailbox driver. " > No, you are right. I didn't mention that explicitly. I wanted to, but thought I will wait until this driver got traction to ask you to merge them. Sorry for that. Anyways I am against having existing solution and a mailbox for SMC, they need to be merged at any cost. Where the final solution will be doesn't matter much to me, I am fine either way. -- Regards, Sudeep
diff --git a/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml b/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml new file mode 100644 index 000000000000..797c0cc609a3 --- /dev/null +++ b/Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml @@ -0,0 +1,39 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$schema: "http://devicetree.org/meta-schemas/core.yaml#" +$id: http://devicetree.org/schemas/mailbox/brcm,brcmstb-mbox.yaml# + +title: Broadcom STB mailbox driver bindings + +maintainers: + - Jim Quinlan <james.quinlan@broadcom.com> + +properties: + compatible: + enum: + - brcm,brcmstb-mbox + + interrupts: + items: + - description: a2p return interrupt, indicates SCMI msg completion. + + "#mbox-cells": + const: 1 + +required: + - compatible + - interrupts + - "#mbox-cells" + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/arm-gic.h> + mailbox { + compatible = "brcm,brcmstb-mailbox"; + #mbox-cells = <1>; + interrupts = <GIC_SPI 0xc6 IRQ_TYPE_LEVEL_HIGH>; + }; +...
Bindings are added. Only one interrupt is needed because we do not yet employ the SCMI p2a channel. Signed-off-by: Jim Quinlan <james.quinlan@broadcom.com> --- .../bindings/mailbox/brcm,brcmstb-mbox.yaml | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 Documentation/devicetree/bindings/mailbox/brcm,brcmstb-mbox.yaml