| Message ID | 20240424-smsm-mbox-v1-0-555f3f442841@z3ntu.xyz |
|---|---|
| Headers | show |
| Series | Support mailbox interface in qcom,smsm driver | expand |
On Mittwoch, 22. Mai 2024 08:49:43 MESZ Krzysztof Kozlowski wrote: > On 21/05/2024 22:35, Luca Weiss wrote: > > On Dienstag, 21. Mai 2024 10:58:07 MESZ Krzysztof Kozlowski wrote: > >> On 20/05/2024 17:11, Luca Weiss wrote: > >>> Hi Krzysztof > >>> > >>> Ack, sounds good. > >>> > >>> Maybe also from you, any opinion between these two binding styles? > >>> > >>> So first using index of mboxes for the numbering, where for the known > >>> usages the first element (and sometimes the 3rd - ipc-2) are empty <>. > >>> > >>> The second variant is using mbox-names to get the correct channel-mbox > >>> mapping. > >>> > >>> - qcom,ipc-1 = <&apcs 8 13>; > >>> - qcom,ipc-2 = <&apcs 8 9>; > >>> - qcom,ipc-3 = <&apcs 8 19>; > >>> + mboxes = <0>, <&apcs 13>, <&apcs 9>, <&apcs 19>; > >>> > >>> vs. > >>> > >>> - qcom,ipc-1 = <&apcs 8 13>; > >>> - qcom,ipc-2 = <&apcs 8 9>; > >>> - qcom,ipc-3 = <&apcs 8 19>; > >>> + mboxes = <&apcs 13>, <&apcs 9>, <&apcs 19>; > >>> + mbox-names = "ipc-1", "ipc-2", "ipc-3"; > >> > >> Sorry, don't get, ipc-1 is the first mailbox, so why would there be <0> > >> in first case? > > > > Actually not, ipc-0 would be permissible by the driver, used for the 0th host > > > > e.g. from: > > > > /* Iterate over all hosts to check whom wants a kick */ > > for (host = 0; host < smsm->num_hosts; host++) { > > hostp = &smsm->hosts[host]; > > > > Even though no mailbox is specified in any upstream dts for this 0th host I > > didn't want the bindings to restrict that, that's why in the first example > > there's an empty element (<0>) for the 0th smsm host > > > >> Anyway, the question is if you need to know that some > >> mailbox is missing. But then it is weird to name them "ipc-1" etc. > > > > In either case we'd just query the mbox (either by name or index) and then > > see if it's there? Not quite sure I understand the sentence.. > > Pretty sure either binding would work the same way. > > The question is: does the driver care only about having some mailboxes > or the driver cares about each specific mailbox? IOW, is skipping ipc-0 > important for the driver? There's nothing special from driver side about any mailbox. Some SoCs have a mailbox for e.g. hosts 1&2&3, some have only 1&3, and apq8064 even has 1&2&3&4. And if the driver doesn't find a mailbox for a host, it just ignores it but then of course it can't 'ring' the mailbox for that host when necessary. Not sure how much more I can add here, to be fair I barely understand what this driver is doing myself apart from the obvious. Regards Luca > > > Best regards, > Krzysztof > >
On Donnerstag, 23. Mai 2024 08:02:13 MESZ Krzysztof Kozlowski wrote: > On 22/05/2024 19:34, Luca Weiss wrote: > > On Mittwoch, 22. Mai 2024 08:49:43 MESZ Krzysztof Kozlowski wrote: > >> On 21/05/2024 22:35, Luca Weiss wrote: > >>> On Dienstag, 21. Mai 2024 10:58:07 MESZ Krzysztof Kozlowski wrote: > >>>> On 20/05/2024 17:11, Luca Weiss wrote: > >>>>> Hi Krzysztof > >>>>> > >>>>> Ack, sounds good. > >>>>> > >>>>> Maybe also from you, any opinion between these two binding styles? > >>>>> > >>>>> So first using index of mboxes for the numbering, where for the known > >>>>> usages the first element (and sometimes the 3rd - ipc-2) are empty <>. > >>>>> > >>>>> The second variant is using mbox-names to get the correct channel-mbox > >>>>> mapping. > >>>>> > >>>>> - qcom,ipc-1 = <&apcs 8 13>; > >>>>> - qcom,ipc-2 = <&apcs 8 9>; > >>>>> - qcom,ipc-3 = <&apcs 8 19>; > >>>>> + mboxes = <0>, <&apcs 13>, <&apcs 9>, <&apcs 19>; > >>>>> > >>>>> vs. > >>>>> > >>>>> - qcom,ipc-1 = <&apcs 8 13>; > >>>>> - qcom,ipc-2 = <&apcs 8 9>; > >>>>> - qcom,ipc-3 = <&apcs 8 19>; > >>>>> + mboxes = <&apcs 13>, <&apcs 9>, <&apcs 19>; > >>>>> + mbox-names = "ipc-1", "ipc-2", "ipc-3"; > >>>> > >>>> Sorry, don't get, ipc-1 is the first mailbox, so why would there be <0> > >>>> in first case? > >>> > >>> Actually not, ipc-0 would be permissible by the driver, used for the 0th host > >>> > >>> e.g. from: > >>> > >>> /* Iterate over all hosts to check whom wants a kick */ > >>> for (host = 0; host < smsm->num_hosts; host++) { > >>> hostp = &smsm->hosts[host]; > >>> > >>> Even though no mailbox is specified in any upstream dts for this 0th host I > >>> didn't want the bindings to restrict that, that's why in the first example > >>> there's an empty element (<0>) for the 0th smsm host > >>> > >>>> Anyway, the question is if you need to know that some > >>>> mailbox is missing. But then it is weird to name them "ipc-1" etc. > >>> > >>> In either case we'd just query the mbox (either by name or index) and then > >>> see if it's there? Not quite sure I understand the sentence.. > >>> Pretty sure either binding would work the same way. > >> > >> The question is: does the driver care only about having some mailboxes > >> or the driver cares about each specific mailbox? IOW, is skipping ipc-0 > >> important for the driver? > > > > There's nothing special from driver side about any mailbox. Some SoCs have > > a mailbox for e.g. hosts 1&2&3, some have only 1&3, and apq8064 even has > > 1&2&3&4. > > > > And if the driver doesn't find a mailbox for a host, it just ignores it > > but then of course it can't 'ring' the mailbox for that host when necessary. > > > > Not sure how much more I can add here, to be fair I barely understand what > > this driver is doing myself apart from the obvious. > > From what you said, it looks like it is enough to just list mailboxes, > e.g. for ipc-1, ipc-2 and ipc-4 (so no ipc-0 and ipc-3): No, for sure we need also the possibility to list ipc-3. And my point is that I'm not sure if any platform will ever need ipc-0, but the code to use that if it ever exists is there - the driver always tries getting an mbox (currently just syscon of course) for every host from 0 to n. These are the current (non-mbox-API) mboxes provided to smsm: $ git grep qcom,ipc- arch/ arch/arm/boot/dts/qcom/qcom-apq8064.dtsi: qcom,ipc-1 = <&l2cc 8 4>; arch/arm/boot/dts/qcom/qcom-apq8064.dtsi: qcom,ipc-2 = <&l2cc 8 14>; arch/arm/boot/dts/qcom/qcom-apq8064.dtsi: qcom,ipc-3 = <&l2cc 8 23>; arch/arm/boot/dts/qcom/qcom-apq8064.dtsi: qcom,ipc-4 = <&sps_sic_non_secure 0x4094 0>; arch/arm/boot/dts/qcom/qcom-msm8974.dtsi: qcom,ipc-1 = <&apcs 8 13>; arch/arm/boot/dts/qcom/qcom-msm8974.dtsi: qcom,ipc-2 = <&apcs 8 9>; arch/arm/boot/dts/qcom/qcom-msm8974.dtsi: qcom,ipc-3 = <&apcs 8 19>; arch/arm64/boot/dts/qcom/msm8916.dtsi: qcom,ipc-1 = <&apcs 8 13>; arch/arm64/boot/dts/qcom/msm8916.dtsi: qcom,ipc-3 = <&apcs 8 19>; arch/arm64/boot/dts/qcom/msm8939.dtsi: qcom,ipc-1 = <&apcs1_mbox 8 13>; arch/arm64/boot/dts/qcom/msm8939.dtsi: qcom,ipc-3 = <&apcs1_mbox 8 19>; arch/arm64/boot/dts/qcom/msm8953.dtsi: qcom,ipc-1 = <&apcs 8 13>; arch/arm64/boot/dts/qcom/msm8953.dtsi: qcom,ipc-3 = <&apcs 8 19>; arch/arm64/boot/dts/qcom/msm8976.dtsi: qcom,ipc-1 = <&apcs 8 13>; arch/arm64/boot/dts/qcom/msm8976.dtsi: qcom,ipc-2 = <&apcs 8 9>; arch/arm64/boot/dts/qcom/msm8976.dtsi: qcom,ipc-3 = <&apcs 8 19>; > > mboxes = <&apcs 13>, <&apcs 9>, <&apcs 19>; > > Best regards, > Krzysztof > >
On Samstag, 25. Mai 2024 18:47:08 MESZ Krzysztof Kozlowski wrote: > On 24/05/2024 19:55, Luca Weiss wrote: > > On Donnerstag, 23. Mai 2024 08:19:11 MESZ Krzysztof Kozlowski wrote: > >> On 23/05/2024 08:16, Luca Weiss wrote: > >>> On Donnerstag, 23. Mai 2024 08:02:13 MESZ Krzysztof Kozlowski wrote: > >>>> On 22/05/2024 19:34, Luca Weiss wrote: > >>>>> On Mittwoch, 22. Mai 2024 08:49:43 MESZ Krzysztof Kozlowski wrote: > >>>>>> On 21/05/2024 22:35, Luca Weiss wrote: > >>>>>>> On Dienstag, 21. Mai 2024 10:58:07 MESZ Krzysztof Kozlowski wrote: > >>>>>>>> On 20/05/2024 17:11, Luca Weiss wrote: > >>>>>>>>> Hi Krzysztof > >>>>>>>>> > >>>>>>>>> Ack, sounds good. > >>>>>>>>> > >>>>>>>>> Maybe also from you, any opinion between these two binding styles? > >>>>>>>>> > >>>>>>>>> So first using index of mboxes for the numbering, where for the known > >>>>>>>>> usages the first element (and sometimes the 3rd - ipc-2) are empty <>. > >>>>>>>>> > >>>>>>>>> The second variant is using mbox-names to get the correct channel-mbox > >>>>>>>>> mapping. > >>>>>>>>> > >>>>>>>>> - qcom,ipc-1 = <&apcs 8 13>; > >>>>>>>>> - qcom,ipc-2 = <&apcs 8 9>; > >>>>>>>>> - qcom,ipc-3 = <&apcs 8 19>; > >>>>>>>>> + mboxes = <0>, <&apcs 13>, <&apcs 9>, <&apcs 19>; > >>>>>>>>> > >>>>>>>>> vs. > >>>>>>>>> > >>>>>>>>> - qcom,ipc-1 = <&apcs 8 13>; > >>>>>>>>> - qcom,ipc-2 = <&apcs 8 9>; > >>>>>>>>> - qcom,ipc-3 = <&apcs 8 19>; > >>>>>>>>> + mboxes = <&apcs 13>, <&apcs 9>, <&apcs 19>; > >>>>>>>>> + mbox-names = "ipc-1", "ipc-2", "ipc-3"; > >>>>>>>> > >>>>>>>> Sorry, don't get, ipc-1 is the first mailbox, so why would there be <0> > >>>>>>>> in first case? > >>>>>>> > >>>>>>> Actually not, ipc-0 would be permissible by the driver, used for the 0th host > >>>>>>> > >>>>>>> e.g. from: > >>>>>>> > >>>>>>> /* Iterate over all hosts to check whom wants a kick */ > >>>>>>> for (host = 0; host < smsm->num_hosts; host++) { > >>>>>>> hostp = &smsm->hosts[host]; > >>>>>>> > >>>>>>> Even though no mailbox is specified in any upstream dts for this 0th host I > >>>>>>> didn't want the bindings to restrict that, that's why in the first example > >>>>>>> there's an empty element (<0>) for the 0th smsm host > >>>>>>> > >>>>>>>> Anyway, the question is if you need to know that some > >>>>>>>> mailbox is missing. But then it is weird to name them "ipc-1" etc. > >>>>>>> > >>>>>>> In either case we'd just query the mbox (either by name or index) and then > >>>>>>> see if it's there? Not quite sure I understand the sentence.. > >>>>>>> Pretty sure either binding would work the same way. > >>>>>> > >>>>>> The question is: does the driver care only about having some mailboxes > >>>>>> or the driver cares about each specific mailbox? IOW, is skipping ipc-0 > >>>>>> important for the driver? > >>>>> > >>>>> There's nothing special from driver side about any mailbox. Some SoCs have > >>>>> a mailbox for e.g. hosts 1&2&3, some have only 1&3, and apq8064 even has > >>>>> 1&2&3&4. > >>>>> > >>>>> And if the driver doesn't find a mailbox for a host, it just ignores it > >>>>> but then of course it can't 'ring' the mailbox for that host when necessary. > >>>>> > >>>>> Not sure how much more I can add here, to be fair I barely understand what > >>>>> this driver is doing myself apart from the obvious. > >>>> > >>>> From what you said, it looks like it is enough to just list mailboxes, > >>>> e.g. for ipc-1, ipc-2 and ipc-4 (so no ipc-0 and ipc-3): > >>> > >>> No, for sure we need also the possibility to list ipc-3. > >> > >> ? You can list it, what's the problem> > > > > Maybe we're talking past each other... > > > > You asked why this wouldn't work: > > > > e.g. for ipc-1, ipc-2 and ipc-4 (so no ipc-0 and ipc-3): > > mboxes = <&apcs 13>, <&apcs 9>, <&apcs 19>; > > > > How would we know that the 3rd mailbox (&apcs 19) is for the 4th host > > (previous ipc-4)? > > > > 1. If we use mboxes with indexes we'd need to have <0> values for > > "smsm hosts" where we don't have a mailbox for - this is at least > > for the 2nd smsm host (qcom,ipc-2) for a bunch of SoCs. > > > > 2. If we use mboxes with mbox-names then we could skip that since we > > can directly specify which "smsm host" a given mailbox is for. > > > > My only question really is whether 1. or 2. is a better idea. > > > > Is this clearer now or still not? > > > So again, does the driver care about missing entry? If so, why? What do you mean with "care"? I didn't change any behavior to what's happening now, if e.g. qcom,ipc-3 is not set right now then the driver is okay with that and just won't ring the mailbox for that smsm host. The behavior will be the same with mbox, if a mbox for e.g. the 3rd smsm host is not set, the driver is okay with that but then of course won't do anything for that host. See the driver patch for details, or is something unclear there? Regards Luca > > Best regards, > Krzysztof > >
Take a shot at converting the last driver that requires direct "qcom,ipc*" syscon references in devicetree by allowing the smsm driver to use the mailbox interface to achieve the same effect. I'm not very happy about how the devicetree change looks in the end. Perhaps it's better to use mbox-names to not have <0> elements in dt, and reference the items by name from the driver? e.g. this change for msm8226 could be represented differently. - qcom,ipc-1 = <&apcs 8 13>; - qcom,ipc-2 = <&apcs 8 9>; - qcom,ipc-3 = <&apcs 8 19>; + mboxes = <0>, <&apcs 13>, <&apcs 9>, <&apcs 19>; vs. for example: - qcom,ipc-1 = <&apcs 8 13>; - qcom,ipc-2 = <&apcs 8 9>; - qcom,ipc-3 = <&apcs 8 19>; + mboxes = <&apcs 13>, <&apcs 9>, <&apcs 19>; + mbox-names = "ipc-1", "ipc-2", "ipc-3"; But also here the name with 'ipc-N' is probably not particularly fitting? Please let me know your thoughts and any suggestions. Signed-off-by: Luca Weiss <luca@z3ntu.xyz> --- Luca Weiss (2): dt-bindings: soc: qcom,smsm: Allow specifying mboxes instead of qcom,ipc soc: qcom: smsm: Support using mailbox interface .../devicetree/bindings/soc/qcom/qcom,smsm.yaml | 48 ++++++++++++++++---- drivers/soc/qcom/smsm.c | 51 +++++++++++++++++++++- 2 files changed, 90 insertions(+), 9 deletions(-) --- base-commit: ed30a4a51bb196781c8058073ea720133a65596f change-id: 20240424-smsm-mbox-0666f35eae44 Best regards,