Hi, sorry for a late reply but I couldn't answer earlier. 30.03.2021 19:40, Rob Herring пишет: > On Fri, Mar 19, 2021 at 10:23:20PM +0500, nikitos.tr@xxxxxxxxx wrote: >> From: Nikita Travkin <nikitos.tr@xxxxxxxxx> >> >> Add DT bindings for memshare: QMI service that allocates >> memory per remote processor request. >> >> Signed-off-by: Nikita Travkin <nikitos.tr@xxxxxxxxx> >> --- >> .../bindings/soc/qcom/qcom,memshare.yaml | 109 ++++++++++++++++++ >> include/dt-bindings/soc/qcom,memshare.h | 10 ++ >> 2 files changed, 119 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/soc/qcom/qcom,memshare.yaml >> create mode 100644 include/dt-bindings/soc/qcom,memshare.h >> >> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,memshare.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,memshare.yaml >> new file mode 100644 >> index 000000000000..ebdf128b066c >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,memshare.yaml >> @@ -0,0 +1,109 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: "http://devicetree.org/schemas/soc/qcom/qcom,memshare.yaml#"; >> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"; >> + >> +title: Qualcomm QMI Shared Memory Service > How many shared memory interfaces does Qcom have... > >> + >> +description: | >> + This driver provides a QMI service that allows remote processors (like modem) >> + to request additional memory. It is used for applications like GPS in modem. > If the memory region is defined in reserved-memory, how are you > allocating additional memory? Initially remoteproc is loaded into it's own reserved-memory region but qcom decided that they sometimes need more memory than that. Memshare driver in msm8916 downstream tree seem to blindly allocate DMA region for every request that it gets. Additionally for those clients described in the DT, they do the DMA allocation on boot time and never free the region. They call it "guaranteed" allocation. On msm8916 only one "guaranteed" client seem to be used so I decided to implement it with reserved-memory node. On newer platforms they seem to have more clients but I think that the driver can be easily extended to support dynamic allocation if someone really needs it. I tried to explain that in the cover letter but I think I made some mistake as I don't see it in the Patchwork. >> + >> +maintainers: >> + - Nikita Travkin <nikitos.tr@xxxxxxxxx> >> + >> +properties: >> + compatible: >> + const: qcom,memshare >> + >> + qcom,legacy-client: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: Phandle to a memshare client node used for legacy requests. >> + >> + "#address-cells": >> + const: 1 >> + >> + "#size-cells": >> + const: 0 >> + >> +patternProperties: >> + "^.*@[0-9]+$": >> + type: object >> + >> + properties: >> + reg: >> + description: Proc-ID for clients in this node. > What's Proc-ID? The requests from the remote nodes contain client-id and proc-id that are supposed to differentiate the clients. It's possible to find the values in downstream DT or by observing what messages are received by the memshare service (I left dev_dbg logging in the driver for that reason) I think I should reword it to make this more apparent, maybe "Proc-ID that clients in this node send."? > >> + >> + qcom,qrtr-node: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + description: Node from which the requests are expected. >> + >> + "#address-cells": >> + const: 1 >> + >> + "#size-cells": >> + const: 0 >> + >> + patternProperties: >> + "^.*@[0-9]+$": >> + type: object >> + >> + properties: >> + reg: >> + description: ID of this client. > How does one determine the ID? As with proc-id, maybe reword to "ID that this client sends."? I will change those in v2, I still expect comments on the driver itself, so I'll wait for that before submitting it with just a couple lines changed. > >> + >> + memory-region: >> + $ref: /schemas/types.yaml#/definitions/phandle >> + description: | >> + Reserved memory region that should be used for allocation. >> + >> + required: >> + - reg >> + >> + required: >> + - reg >> + - qcom,qrtr-node >> + >> +required: >> + - compatible >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/soc/qcom,memshare.h> >> + >> + reserved-memory { >> + >> + #address-cells = <2>; >> + #size-cells = <2>; >> + >> + gps_mem: gps@93c00000 { >> + reg = <0x0 0x93c00000 0x0 0x200000>; >> + no-map; > We support 'compatible' in reserved-memory nodes, can you simplify the > binding and put everything in here? If I understand this correctly, each reserved-memory node will then load a new instance of memshare. Since the driver registers a QMI service that handles multiple clients, there should be only one instance. Additionally, as I mentioned earlier, some clients may not need reserved-memory at all >> + }; >> + }; >> + >> + memshare { >> + compatible = "qcom,memshare"; >> + qcom,legacy-client = <&memshare_gps>; >> + >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + mpss@0 { >> + reg = <MEMSHARE_PROC_MPSS_V01>; >> + qcom,qrtr-node = <0>; >> + >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + memshare_gps: gps@0 { >> + reg = <0>; >> + memory-region = <&gps_mem>; >> + }; >> + }; >> + }; >> + >> +... >> diff --git a/include/dt-bindings/soc/qcom,memshare.h b/include/dt-bindings/soc/qcom,memshare.h >> new file mode 100644 >> index 000000000000..4cef1ef75d09 >> --- /dev/null >> +++ b/include/dt-bindings/soc/qcom,memshare.h >> @@ -0,0 +1,10 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> + >> +#ifndef __DT_QCOM_MEMSHARE_H__ >> +#define __DT_QCOM_MEMSHARE_H__ >> + >> +#define MEMSHARE_PROC_MPSS_V01 0 >> +#define MEMSHARE_PROC_ADSP_V01 1 >> +#define MEMSHARE_PROC_WCNSS_V01 2 >> + >> +#endif /* __DT_QCOM_MEMSHARE_H__ */ >> -- >> 2.27.0 >>
